From 16cd868a0face98b571821882485edc1c63b0b8f Mon Sep 17 00:00:00 2001 From: jdalton Date: Mon, 10 Aug 2026 14:34:55 -0700 Subject: [PATCH] feat(cli): add prompt and auto update modes, and defer to the installing package manager Adds the two remaining rungs of the update ladder. `prompt` asks once, after a command that succeeded, on a real terminal only. `auto` installs at the end of the run, but only for a binary Perry itself installed: a Homebrew, npm, apt or winget copy prints that tool's own upgrade command instead of overwriting a file the package manager owns. `perry update --mode ` writes the setting, and `perry doctor` reports it. --- .../7784-update-prompt-auto-and-channels.md | 105 ++++ changelog.d/7787-update-surface-followups.md | 55 ++ crates/perry/src/commands/doctor.rs | 29 +- crates/perry/src/commands/publish/mod.rs | 3 +- .../src/commands/publish/saved_config.rs | 142 +++++- crates/perry/src/commands/update.rs | 47 +- crates/perry/src/install_channel.rs | 281 ++++++++++ crates/perry/src/main.rs | 52 +- crates/perry/src/update_checker.rs | 90 +++- crates/perry/src/update_policy.rs | 482 +++++++++++++++++- 10 files changed, 1229 insertions(+), 57 deletions(-) create mode 100644 changelog.d/7784-update-prompt-auto-and-channels.md create mode 100644 changelog.d/7787-update-surface-followups.md create mode 100644 crates/perry/src/install_channel.rs diff --git a/changelog.d/7784-update-prompt-auto-and-channels.md b/changelog.d/7784-update-prompt-auto-and-channels.md new file mode 100644 index 0000000000..cb56b36f5f --- /dev/null +++ b/changelog.d/7784-update-prompt-auto-and-channels.md @@ -0,0 +1,105 @@ +### Added + +**`prompt` and `auto` update modes now do something, and refuse to do the wrong +thing.** The previous slice made the modes configurable; this wires them to the +existing signed self-updater, behind three refusals. + +**A package-managed install is never replaced in place.** `perry update` +overwrites the running executable, which is right for a tarball or `install.sh` +install and wrong for every managed one: Homebrew, npm, apt and winget each keep +their own record of what is installed and at what version, and overwriting the +file underneath leaves that record lying. `prompt` and `auto` now detect the +owner and name that owner's command instead: + +| owner | what Perry says to run | +|---|---| +| Homebrew | `brew upgrade perryts/perry/perry` | +| npm | `npm install -g @perryts/perry@latest` | +| apt | `sudo apt update && sudo apt install --only-upgrade perry` | +| winget | `winget upgrade PerryTS.Perry` | + +npm gets an extra sentence, because it is the worst case: Perry ships as a +wrapper package plus a per-platform binary package, so replacing the binary also +desyncs it from the wrapper that launched it. + +**Nothing is offered after a command that failed.** The user is looking at an +error; a question about upgrading is noise at the worst possible moment, and an +unattended install would bury the error under progress output. Both active modes +fall back to a plain notice. + +**An unwritable install directory is reported, not attempted.** `install.sh` +targets `/usr/local/bin`, which is root-owned on a default macOS and most Linux +boxes. That is now checked *before* anything is downloaded, so the outcome is +one sentence naming `sudo perry update` rather than a half-finished install. Perry +never escalates on its own. + +**`perry update --mode `** saves the setting and exits, +so the one thing people are most likely to change does not require hand-editing +TOML. It is a read-modify-write through the shared loader, so the rest of the +file comes back out the way it went in. + +**`perry doctor`** now reports the effective mode and, when there is one, the +package manager that owns the binary — the two questions behind "why did it not +update". + +
+Why the channel detection fails open + +Every rule answers "is this definitely managed?", never "is this definitely +unmanaged?", and an unrecognised layout resolves to self-managed. + +That asymmetry is deliberate. Guessing "managed" wrongly would refuse to +self-update a plain tarball install — the majority case, and the one with no +other upgrade path. Guessing "self-managed" wrongly costs an in-place update on +a machine that had a package manager available, which is recoverable by running +that manager. + +The paths are canonicalized before classification, because Homebrew's `perry` in +`/usr/local/bin` is a symlink into the Cellar; classifying the link rather than +its target would miss every Homebrew install there is. + +apt requires **both** a dpkg file list and a dpkg-owned path, because dpkg does +not own `/usr/local` — that is `install.sh`'s directory. The path alone would +misclassify a hand-placed binary; the dpkg list alone would claim a tarball +install on a machine that also has the `.deb` installed somewhere else. The check +is a file-existence test rather than a `dpkg -S` subprocess, since this runs on +the update path of every command. +
+ +
+Prompting needs stdin, not just stderr + +The mode gate already requires stderr to be a terminal. That is not enough to +ask a question: stdin can be a pipe while stderr is a tty, and reading from it +would either block the command or take whatever the pipe happened to contain as +consent. `prompt` degrades to a plain notice when stdin is not a terminal. + +`auto` asks nothing, so it does not need stdin — but it does still require the +command to have succeeded, an unmanaged install, and a writable directory. +
+ +
+Tests + +24 new, all in the required per-pull-request job. The decision is a pure +function of the mode plus four facts about the machine, so every refusal is +asserted directly rather than left inside an `if` in the middle of a teardown +path: + +- both active modes downgrade to a notice after a failed command; +- both refuse on all four managed channels, and name a command for each; +- both report elevation rather than attempting an unwritable install; +- `prompt` degrades without stdin while `auto` does not need it. + +The channel table covers Homebrew under all three prefixes, npm for global, nvm +and project-local layouts, apt with and without each half of its rule, both +winget delivery shapes, and four unrecognised layouts that must fail open. +Classification splits on both path separators rather than using +`Path::components`, so the winget cases run on every host instead of only on +Windows. + +Verified end to end: writing `mode` into a real config file that already had a +`license_key` and an unknown `[update] future_key` left both intact. + +`cargo test -p perry`: 914 passed, 0 failed. +
diff --git a/changelog.d/7787-update-surface-followups.md b/changelog.d/7787-update-surface-followups.md new file mode 100644 index 0000000000..0875a51024 --- /dev/null +++ b/changelog.d/7787-update-surface-followups.md @@ -0,0 +1,55 @@ +### Fixed + +Five defects in the update surface, all found in review of +[#7749](https://github.com/PerryTS/perry/pull/7749) after it had merged. + +**The config warning escaped the rules that were meant to silence it.** The +"unrecognized `[update] mode`" line was printed inside `UpdatePolicy::resolve`, +before the precedence rules it sits behind had been applied — so it reached +stderr during `--format json`, in CI, with a piped stderr, and under `--quiet`. +Those rules exist to keep exactly those runs silent, and the one line whose job +was to report a config problem was the one line ignoring them. It is now held on +the policy and emitted at the single point where the run is known to be speaking +at all. + +**The notify interval throttled on time alone, so it swallowed the next +release.** The documented contract is that the interval throttles repeats of +*the same* update. Keyed only on a timestamp, it also suppressed a **different** +version that arrived inside the window — so somebody setting a week-long +interval to stop being nagged about one release would also have been denied the +release that fixed it. The cache now records which version it announced, and a +different version is announced regardless of the interval. + +**The interval comparison was signed.** `Duration::as_secs() as i64` goes +negative for a large enough configured value, and a negative interval reads as +already-elapsed — so an absurd value would have notified on *every* run instead +of suppressing. The comparison is unsigned. + +**Two `perry` processes could corrupt the cache.** Every write used one shared +`*.json.tmp`, so two writers each wrote it and each renamed it: the loser's +rename landed a file the winner was still writing into. Each write now builds +its own temporary name. + +**A refresh could erase a notice recorded while its request was in flight.** +`fetch_latest_version` read the notice state *before* issuing its request and +wrote it back afterwards, overwriting anything recorded in between — telling the +user about the same release twice. The read-modify-write pairs are now +serialized by a lock file, and the refresh re-reads inside that lock immediately +before replacing. + +
+Tests + +Two new contract tests, both sabotage-verified — reverting either fix turns its +test red: + +- a different version is announced regardless of the interval, and never having + announced anything counts as "not this version"; +- an enormous interval still suppresses rather than wrapping into notifying. + +The existing interval tests are unchanged in intent: they now go through a +helper that holds the announced version constant, so they still exercise only +the interval arithmetic. + +`cargo test -p perry`: 904 passed, 0 failed. +
diff --git a/crates/perry/src/commands/doctor.rs b/crates/perry/src/commands/doctor.rs index e9159edd3f..4b8d99c3fc 100644 --- a/crates/perry/src/commands/doctor.rs +++ b/crates/perry/src/commands/doctor.rs @@ -286,16 +286,41 @@ fn check_runtime_library() -> CheckResult { } fn check_update_available() -> CheckResult { + // Say which mode is in effect and who owns this binary. Both are things a + // user asks `doctor` about precisely when updates are not behaving as they + // expect — "why did it not install" is almost always one of the two. + let policy = crate::update_policy::UpdatePolicy::resolve(); + let channel = crate::install_channel::detect(); + // Configured, not effective: `doctor --format json` and `doctor | less` both + // suppress the update surface for their own run, so reporting the effective + // mode would answer "off" to the very question being asked. + let mut detail = format!(" (mode: {}", policy.configured_mode.label()); + if !policy.is_active() && policy.configured_mode != crate::update_policy::UpdateMode::Off { + detail.push_str("; suppressed for this run"); + } + if let Some(command) = channel.upgrade_command() { + detail.push_str(&format!( + "; installed by {} — upgrade with `{}`", + channel.label(), + command + )); + } + detail.push(')'); + let context = detail; + match update_checker::check_cached_status() { update_checker::UpdateStatus::UpdateAvailable { latest, .. } => CheckResult { name: "update status".to_string(), status: CheckStatus::Warning, - details: Some(format!("v{} available — run `perry update`", latest)), + details: Some(format!( + "v{} available — run `perry update`{}", + latest, context + )), }, update_checker::UpdateStatus::UpToDate => CheckResult { name: "update status".to_string(), status: CheckStatus::Ok, - details: Some("up to date".to_string()), + details: Some(format!("up to date{}", context)), }, update_checker::UpdateStatus::CheckFailed => CheckResult { name: "update status".to_string(), diff --git a/crates/perry/src/commands/publish/mod.rs b/crates/perry/src/commands/publish/mod.rs index dfa651ca1e..f497288f03 100644 --- a/crates/perry/src/commands/publish/mod.rs +++ b/crates/perry/src/commands/publish/mod.rs @@ -35,7 +35,8 @@ pub use args::PublishArgs; #[cfg(test)] pub(crate) use saved_config::IosSavedConfig; // consumed only by tests pub(crate) use saved_config::{ - check_beta_consent, config_path, is_interactive, load_config, prompt_input, report_beta_error, + check_beta_consent, config_path, is_interactive, load_config, load_config_checked, + prompt_input, report_beta_error, update_config_file, save_config, AndroidSavedConfig, AppleSavedConfig, HarmonyosSavedConfig, PerryConfig, }; pub(crate) use tarball::create_project_tarball_with_filters; diff --git a/crates/perry/src/commands/publish/saved_config.rs b/crates/perry/src/commands/publish/saved_config.rs index ea7d52bf54..366de978b0 100644 --- a/crates/perry/src/commands/publish/saved_config.rs +++ b/crates/perry/src/commands/publish/saved_config.rs @@ -119,12 +119,47 @@ pub(crate) fn config_path() -> PathBuf { } pub(crate) fn load_config() -> PerryConfig { + load_config_checked().unwrap_or_default() +} + +/// `load_config`, but able to say WHY it produced nothing. +/// +/// `Err` means the file exists and does not parse — the case a plain +/// `unwrap_or_default()` cannot tell apart from "no file yet". The difference +/// matters at save time, because writing a default struct over a damaged but +/// hand-recoverable config destroys the user's license key and tokens along with +/// the syntax error they were about to fix. +pub(crate) fn load_config_checked() -> std::result::Result { let path = config_path(); - if let Ok(content) = fs::read_to_string(&path) { - toml::from_str(&content).unwrap_or_default() - } else { - PerryConfig::default() - } + let content = match fs::read_to_string(&path) { + Ok(content) => content, + // A missing file is the ONLY read failure that means "no config yet". + // A permission change or a transient I/O error must not be answered with + // defaults, because `update_config_file` would accept those defaults and + // write them over a file that still holds the license key and tokens — + // the very loss this function exists to prevent. + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + return Ok(PerryConfig::default()); + } + Err(error) => return Err(format!("{} could not be read: {error}", path.display())), + }; + toml::from_str(&content).map_err(|error| error.to_string()) +} + +/// Read, modify, write — refusing to write when the read failed. +/// +/// Every setting-writer goes through this. Handing `load_config`'s default +/// struct to `save_config` is how a damaged file becomes an erased one, and a +/// caller cannot tell the two apart on its own. +pub(crate) fn update_config_file(edit: impl FnOnce(&mut PerryConfig)) -> Result<()> { + let mut config = load_config_checked().map_err(|error| { + anyhow::anyhow!( + "~/.perry/config.toml could not be loaded, so it was left untouched: \ + {error}. Fix the file (or delete it) and try again." + ) + })?; + edit(&mut config); + save_config(&config) } pub(crate) fn save_config(config: &PerryConfig) -> Result<()> { @@ -326,3 +361,100 @@ check_interval_hours = 6 assert!(written.contains("keep-me") && written.contains("[telemetry]")); } } + +#[cfg(test)] +mod tests { + use super::*; + + /// A config that does not parse must be left alone, not replaced by defaults. + /// + /// This is the data-loss case. `load_config` cannot tell "no file yet" from + /// "damaged file", so a writer built on it turns one stray character into an + /// erased license key — the user's own tokens, gone while they were fixing a + /// typo. + #[test] + fn a_damaged_config_is_never_overwritten_with_defaults() { + let _lock = crate::test_env_lock::env_lock(); + let home = tempfile::tempdir().expect("tempdir"); + let saved = std::env::var_os("HOME"); + std::env::set_var("HOME", home.path()); + + let path = config_path(); + std::fs::create_dir_all(path.parent().expect("parent")).expect("mkdir"); + let damaged = "license_key = \"keep-me\"\nthis is not toml\n"; + std::fs::write(&path, damaged).expect("write"); + + let error = update_config_file(|config| { + config.update.get_or_insert_with(Default::default).mode = + Some(crate::update_policy::UpdateMode::Notify); + }) + .expect_err("a damaged config must refuse the write"); + assert!( + format!("{error:#}").contains("left untouched"), + "the message must say the file was not written: {error:#}" + ); + assert_eq!( + std::fs::read_to_string(&path).expect("read"), + damaged, + "the file was rewritten despite the refusal" + ); + + // A file that exists but cannot be read is the same danger wearing a + // different hat: treating a permission error as "no config yet" would + // serialize defaults over a file whose contents we never saw. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let kept = "license_key = \"keep-me\"\n"; + std::fs::write(&path, kept).expect("write"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o000)) + .expect("chmod"); + // Root ignores the mode bits, so there is nothing to prove when the + // suite runs as root. Skip in that case rather than fail. + let still_readable = std::fs::read_to_string(&path).is_ok(); + let outcome = update_config_file(|config| { + config.update.get_or_insert_with(Default::default).mode = + Some(crate::update_policy::UpdateMode::Notify); + }); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .expect("restore"); + if !still_readable { + let error = outcome.expect_err("an unreadable config must refuse the write"); + let text = format!("{error:#}"); + // The REASON matters, not just the failure. A write that fails on + // the same permissions would also produce an error, and a test + // that accepts any error passes whether or not the read is + // checked at all. + assert!( + text.contains("could not be read"), + "the refusal must name the read failure, not a later write \ + failure: {text}" + ); + assert!(text.contains("left untouched"), "{text}"); + assert_eq!( + std::fs::read_to_string(&path).expect("read"), + kept, + "the file was rewritten despite being unreadable" + ); + } + } + + // A missing file is still the "use defaults" case, so a first-time write + // has to succeed. + std::fs::remove_file(&path).expect("remove"); + update_config_file(|config| { + config.update.get_or_insert_with(Default::default).mode = + Some(crate::update_policy::UpdateMode::Notify); + }) + .expect("a fresh config must be writable"); + assert!( + std::fs::read_to_string(&path).expect("read").contains("mode"), + "the setting was not persisted" + ); + + match saved { + Some(v) => std::env::set_var("HOME", v), + None => std::env::remove_var("HOME"), + } + } +} diff --git a/crates/perry/src/commands/update.rs b/crates/perry/src/commands/update.rs index 45c41070b8..028e3361e9 100644 --- a/crates/perry/src/commands/update.rs +++ b/crates/perry/src/commands/update.rs @@ -15,6 +15,14 @@ pub struct UpdateArgs { /// Ignore cache, always fetch from server #[arg(long)] pub force: bool, + + /// Save how Perry should handle updates from now on, then exit. + /// + /// This is the writable half of `[update] mode` in ~/.perry/config.toml — + /// there to save people hand-editing TOML for the one setting they are + /// most likely to want to change. + #[arg(long, value_name = "off|notify|prompt|auto")] + pub mode: Option, } pub fn run( @@ -24,6 +32,10 @@ pub fn run( verbose: u8, quiet: bool, ) -> Result<()> { + if let Some(raw) = args.mode.as_deref() { + return set_mode(raw); + } + let current = env!("CARGO_PKG_VERSION"); let status = if !args.force && !update_checker::is_cache_stale() { @@ -68,7 +80,9 @@ pub fn run( } else { println!("Update available: {} -> {}", cur, latest); } - println!(" Release: {}", release_url); + if !release_url.is_empty() { + println!(" Release: {}", release_url); + } } OutputFormat::Text => {} } @@ -119,3 +133,34 @@ pub fn run( Ok(()) } + +/// Persist `[update] mode`, and nothing else. +/// +/// Read-modify-write through the shared loader so the rest of the file — the +/// license key, the telemetry section, anything a newer Perry wrote — comes +/// back out the way it went in. +fn set_mode(raw: &str) -> Result<()> { + let Some(mode) = crate::update_policy::UpdateMode::parse(raw) else { + anyhow::bail!("unknown update mode `{raw}`. Valid values: off, notify, prompt, auto."); + }; + if mode == crate::update_policy::UpdateMode::Unknown { + anyhow::bail!("unknown update mode `{raw}`. Valid values: off, notify, prompt, auto."); + } + + crate::commands::publish::update_config_file(|config| { + config.update.get_or_insert_with(Default::default).mode = Some(mode); + })?; + + let path = crate::commands::publish::config_path(); + println!("Update mode set to \"{raw}\" ({}).", path.display()); + if mode == crate::update_policy::UpdateMode::Auto { + // Say the limits up front rather than letting someone discover them + // the first time an update does not happen. + println!( + "Perry will install updates at the end of a successful run — except \ + on a package-manager-managed install, where it names that manager's \ + command instead." + ); + } + Ok(()) +} diff --git a/crates/perry/src/install_channel.rs b/crates/perry/src/install_channel.rs new file mode 100644 index 0000000000..abd87cda34 --- /dev/null +++ b/crates/perry/src/install_channel.rs @@ -0,0 +1,281 @@ +//! Which package manager, if any, owns this `perry` binary. +//! +//! `perry update` replaces the running executable in place. That is right for +//! a tarball or `install.sh` install, and wrong for every managed one: a +//! Homebrew formula, an npm package, a `.deb` and a winget package each keep +//! their own record of what is installed and what version it is. Overwriting +//! the file underneath them leaves that record lying, so the next +//! `brew upgrade` or `npm install -g` either reinstalls over the top or +//! reports a version that is not what is on disk. +//! +//! The npm case is the worst of them, because Perry is published as a wrapper +//! package plus a per-platform binary package. Replacing the binary desyncs it +//! from the wrapper that launched it. +//! +//! So the rule is: detect the owner, and when there is one, tell the user the +//! command that owner understands rather than doing it for them. +//! +//! # Failing open +//! +//! Every heuristic here answers "is this definitely managed?", never "is this +//! definitely unmanaged?". An unrecognised layout resolves to +//! [`InstallChannel::SelfManaged`], which is the permissive answer. Getting +//! that wrong costs an in-place update on a machine that could have used a +//! package manager; getting the opposite wrong would refuse to self-update a +//! plain tarball install, which is the majority case and the one with no +//! alternative path. + +use std::path::{Path, PathBuf}; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum InstallChannel { + /// A tarball, `install.sh`, or a locally built binary. Ours to replace. + SelfManaged, + Homebrew, + Npm, + Apt, + Winget, +} + +impl InstallChannel { + /// What the user should run instead, when this channel owns the binary. + pub(crate) fn upgrade_command(self) -> Option<&'static str> { + match self { + Self::SelfManaged => None, + Self::Homebrew => Some("brew upgrade perryts/perry/perry"), + Self::Npm => Some("npm install -g @perryts/perry@latest"), + Self::Apt => Some("sudo apt update && sudo apt install --only-upgrade perry"), + Self::Winget => Some("winget upgrade PerryTS.Perry"), + } + } + + pub(crate) fn label(self) -> &'static str { + match self { + Self::SelfManaged => "self-managed", + Self::Homebrew => "Homebrew", + Self::Npm => "npm", + Self::Apt => "apt", + Self::Winget => "winget", + } + } + + /// The extra sentence worth saying for channels where "just run the other + /// command" undersells why we refused. + pub(crate) fn refusal_detail(self) -> Option<&'static str> { + match self { + Self::Npm => Some( + "replacing the binary would also desync it from the \ + @perryts/perry wrapper package that launched it", + ), + _ => None, + } + } +} + +/// Classify the running binary's install channel. +pub(crate) fn detect() -> InstallChannel { + let Ok(exe) = std::env::current_exe() else { + return InstallChannel::SelfManaged; + }; + // Resolve symlinks BEFORE classifying. Homebrew's `perry` in + // `/usr/local/bin` is a symlink into the Cellar, and `install.sh` may + // leave one too — classifying the link rather than its target would miss + // every Homebrew install there is. + let resolved = std::fs::canonicalize(&exe).unwrap_or(exe); + classify(&resolved, dpkg_owns_perry()) +} + +/// Does dpkg have a file list for a `perry` package? +/// +/// A plain existence check rather than shelling out to `dpkg -S`: this runs on +/// the update path of every command, so it must not spawn a process, and it +/// must not fail noisily in a sandbox that has no `dpkg` on `PATH`. +fn dpkg_owns_perry() -> bool { + cfg!(target_os = "linux") && Path::new("/var/lib/dpkg/info/perry.list").exists() +} + +/// The classification itself, with the filesystem answer passed in so the +/// whole table is testable without one. +pub(crate) fn classify(exe: &Path, dpkg_owns: bool) -> InstallChannel { + // Split on BOTH separators rather than using `Path::components`, which is + // platform-dependent: a Windows path handed to a Unix build comes back as + // one component, so the winget table below could only ever be exercised on + // Windows. The rules here are about names in the path, not about path + // semantics, so a uniform split is both simpler and testable everywhere. + let text = exe.to_string_lossy().replace('\\', "/"); + let components: Vec<&str> = text.split('/').filter(|c| !c.is_empty()).collect(); + let has = |name: &str| components.iter().any(|c| *c == name); + + // Homebrew: everything lives under a Cellar, whatever the prefix is + // (`/opt/homebrew` on Apple silicon, `/usr/local` on Intel, + // `/home/linuxbrew/.linuxbrew` on Linux). + if has("Cellar") { + return InstallChannel::Homebrew; + } + + // npm: a global install lands in `/lib/node_modules/...`, and nvm, + // pnpm and a project-local install all keep the same component. Perry's + // launcher execs the platform binary out of an optional dependency, so the + // running executable is inside `node_modules` in every one of those. + if has("node_modules") { + return InstallChannel::Npm; + } + + // winget puts portable packages under its own Packages directory, and + // store-delivered ones under WindowsApps. + if has("WindowsApps") || has("WinGet") { + return InstallChannel::Winget; + } + + // apt: dpkg owns `/usr/bin` and `/usr/lib`, and specifically does NOT own + // `/usr/local`, which is where `install.sh` puts things. Both halves are + // required — the path alone would misclassify a hand-placed binary, and + // the dpkg list alone would claim a tarball install on a machine that + // happens to also have the package installed elsewhere. + let under_usr = text.starts_with("/usr/bin/") || text.starts_with("/usr/lib/"); + if dpkg_owns && under_usr { + return InstallChannel::Apt; + } + + InstallChannel::SelfManaged +} + +/// Is the directory holding the binary writable by this process? +/// +/// `install.sh` installs into `/usr/local/bin`, which is root-owned on a +/// default macOS and most Linux boxes. Discovering that only when the install +/// tries to rename over the executable means failing halfway through, so this +/// is checked before anything is downloaded. +pub(crate) fn install_dir_is_writable() -> bool { + let Ok(exe) = std::env::current_exe() else { + return false; + }; + let Some(dir) = exe.parent().map(PathBuf::from) else { + return false; + }; + let probe = dir.join(".perry-write-probe"); + match std::fs::File::create(&probe) { + Ok(_) => { + let _ = std::fs::remove_file(&probe); + true + } + Err(_) => false, + } +} + +/// Running under `sudo`, i.e. this process's `$HOME` may not be the invoking +/// user's. +/// +/// Writing the update cache here would leave a root-owned file in that user's +/// `~/.perry`, and every later non-root run would fail to update it — the +/// check would then re-run on every invocation forever. +pub(crate) fn running_via_sudo() -> bool { + std::env::var_os("SUDO_USER").is_some() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn channel_of(path: &str, dpkg_owns: bool) -> InstallChannel { + classify(Path::new(path), dpkg_owns) + } + + #[test] + fn homebrew_is_detected_under_every_prefix() { + for path in [ + "/opt/homebrew/Cellar/perry/1.0/bin/perry", + "/usr/local/Cellar/perry/1.0/bin/perry", + "/home/linuxbrew/.linuxbrew/Cellar/perry/1.0/bin/perry", + ] { + assert_eq!(channel_of(path, false), InstallChannel::Homebrew, "{path}"); + } + } + + /// The launcher execs the platform binary out of an optional dependency, + /// so the running executable is inside `node_modules` for a global + /// install, an nvm install and a project-local one alike. + #[test] + fn npm_is_detected_wherever_node_modules_appears() { + for path in [ + "/usr/local/lib/node_modules/@perryts/perry-darwin-arm64/bin/perry", + "/home/u/.nvm/versions/node/v26.5.1/lib/node_modules/@perryts/perry/bin/perry", + "/home/u/project/node_modules/@perryts/perry-linux-x64/bin/perry", + ] { + assert_eq!(channel_of(path, false), InstallChannel::Npm, "{path}"); + } + } + + /// Both halves are required. `/usr/local` is where `install.sh` puts + /// things and dpkg never owns it, so a machine with the .deb installed + /// elsewhere must not have its tarball binary claimed by apt. + #[test] + fn apt_needs_both_a_dpkg_list_and_a_dpkg_owned_path() { + assert_eq!(channel_of("/usr/bin/perry", true), InstallChannel::Apt); + assert_eq!( + channel_of("/usr/bin/perry", false), + InstallChannel::SelfManaged, + "no dpkg list means no apt package, whatever the path" + ); + assert_eq!( + channel_of("/usr/local/bin/perry", true), + InstallChannel::SelfManaged, + "dpkg does not own /usr/local — that is install.sh's directory" + ); + } + + #[test] + fn winget_is_detected_for_both_delivery_shapes() { + assert_eq!( + channel_of( + r"C:\Program Files\WindowsApps\PerryTS.Perry_1.0\perry.exe", + false + ), + InstallChannel::Winget + ); + assert_eq!( + channel_of( + r"C:\Users\u\AppData\Local\Microsoft\WinGet\Packages\PerryTS.Perry_x\perry.exe", + false + ), + InstallChannel::Winget + ); + } + + /// The permissive default. An unrecognised layout must be treated as ours + /// to replace, because refusing to self-update a tarball install would + /// break the majority case — the one with no other upgrade path. + #[test] + fn an_unrecognized_layout_fails_open_to_self_managed() { + for path in [ + "/usr/local/bin/perry", + "/home/u/tools/perry", + "/home/u/perry/target/release/perry", + "/opt/perry/bin/perry", + ] { + assert_eq!( + channel_of(path, false), + InstallChannel::SelfManaged, + "{path}" + ); + } + } + + /// Every managed channel must be able to tell the user what to run + /// instead. A refusal with no alternative is a dead end. + #[test] + fn every_managed_channel_offers_a_command_and_self_managed_does_not() { + for channel in [ + InstallChannel::Homebrew, + InstallChannel::Npm, + InstallChannel::Apt, + InstallChannel::Winget, + ] { + let command = channel + .upgrade_command() + .unwrap_or_else(|| panic!("{} must offer an upgrade command", channel.label())); + assert!(!command.is_empty()); + } + assert_eq!(InstallChannel::SelfManaged.upgrade_command(), None); + } +} diff --git a/crates/perry/src/main.rs b/crates/perry/src/main.rs index 36105e894e..bdc1431215 100644 --- a/crates/perry/src/main.rs +++ b/crates/perry/src/main.rs @@ -4,6 +4,7 @@ mod commands; mod compat_reports; +mod install_channel; #[cfg(test)] mod panic_profile_contract; mod shadow_layout_contract; @@ -542,12 +543,19 @@ fn main_inner() -> Result<()> { // Print update notice if available (to stderr, non-blocking) if update_surface_active { + // The config complaint, if any, goes out only now — this is the first + // point at which we know the run is allowed to say anything at all. + if let Some(warning) = update_policy.config_warning { + eprintln!("{warning}"); + } let use_stderr_color = !cli.no_color && std::io::stderr().is_terminal(); - let status = if let Some(rx) = bg_check { - rx.recv_timeout(std::time::Duration::from_millis(100)).ok() - } else { - Some(update_checker::check_cached_status()) - }; + // A background check that has not answered within 100 ms falls back to + // the cache rather than saying nothing. Reading the timeout as "no + // update" suppressed a notice the previous run had already earned — the + // check being slow is not evidence that the version is current. + let status = bg_check + .and_then(|rx| rx.recv_timeout(std::time::Duration::from_millis(100)).ok()) + .or_else(|| Some(update_checker::check_cached_status())); if let Some(update_checker::UpdateStatus::UpdateAvailable { current, @@ -558,20 +566,30 @@ fn main_inner() -> Result<()> { // `notify_interval_hours` throttles repeats of the SAME available // update. It defaults to 0 — a notice every run, which is what // Perry did before — so this is inert until someone asks for it. - let last = update_checker::load_cache().and_then(|c| c.last_notification); - if update_policy::should_notify( + let cached = update_checker::load_cache(); + // Passed DOWN rather than wrapped around the call below. Wrapping it + // threw away `auto` mode's install along with the repeat notice. + let notice_throttled = !update_policy::should_notify( update_policy.notify_interval, - last.as_deref(), + cached.as_ref().and_then(|c| c.last_notification.as_deref()), + cached + .as_ref() + .and_then(|c| c.last_notified_version.as_deref()), + &latest, &update_checker::now_rfc3339_public(), - ) { - update_checker::print_update_notice( - ¤t, - &latest, - &release_url, - use_stderr_color, - ); - update_checker::record_notification(); - } + ); + update_policy::run_teardown_action( + &update_policy, + &update_checker::UpdateStatus::UpdateAvailable { + current, + latest, + release_url, + }, + result.is_ok(), + use_stderr_color, + cli.verbose > 0, + notice_throttled, + ); } } diff --git a/crates/perry/src/update_checker.rs b/crates/perry/src/update_checker.rs index 43ab5efb84..d216fd87d0 100644 --- a/crates/perry/src/update_checker.rs +++ b/crates/perry/src/update_checker.rs @@ -33,6 +33,14 @@ pub struct UpdateCache { /// always was. #[serde(default, skip_serializing_if = "Option::is_none")] pub last_notification: Option, + /// Which version that notice was about. + /// + /// Without this the notify interval throttles on time alone, which + /// swallows the NEXT release when it lands inside the window — so a + /// week-long interval set to stop nagging about one version would also hide + /// the one that fixed it. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub last_notified_version: Option, } #[derive(Debug, Deserialize)] @@ -90,7 +98,14 @@ fn save_cache(cache: &UpdateCache) { // `replace_path` rather than `fs::rename`: on Windows a rename onto an // EXISTING file fails, so every write after the first would silently do // nothing and the throttle would never advance. - let tmp = path.with_extension("json.tmp"); + // A per-write name. With one shared `*.json.tmp`, two `perry` processes + // each write it and each rename it: the loser's rename lands a file the + // winner is still writing into, and the cache ends up truncated or mixed. + let tmp = path.with_extension(format!( + "json.tmp.{}.{}", + std::process::id(), + NEXT_TMP.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + )); if fs::write(&tmp, content).is_err() { let _ = fs::remove_file(&tmp); return; @@ -100,15 +115,44 @@ fn save_cache(cache: &UpdateCache) { } } +/// Distinguishes the temporary files of concurrent writes in one process. +static NEXT_TMP: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + +/// Take the cross-process lock guarding read-modify-write of the cache. +/// +/// A background refresh and a notice can be recorded at the same moment, and +/// each is a load-mutate-store: without a lock the later store overwrites the +/// earlier one's field, so a notice recorded while a request was in flight +/// vanishes and the user is told twice. Returns `None` when the lock cannot be +/// taken, in which case the caller proceeds unlocked — losing a cache update is +/// better than refusing to update a cache. +fn lock_cache() -> Option { + let path = cache_path().with_extension("json.lock"); + if let Some(parent) = path.parent() { + let _ = fs::create_dir_all(parent); + } + let mut lock = fslock::LockFile::open(&path).ok()?; + // `try_lock`, NOT `lock`. This runs at teardown, after the command the user + // asked for has finished — so blocking here would hang their terminal on + // another `perry`'s cache write, for a cache. The doc above promises we + // proceed unlocked rather than wait, and `lock()` did not honour it. + match lock.try_lock() { + Ok(true) => Some(lock), + _ => None, + } +} + /// Record that the user has just been told about an available update. /// /// A no-op when there is no cache: the notice can only have come from one, and /// inventing a file here would fabricate a `last_check` that never happened. -pub fn record_notification() { +pub fn record_notification(version: &str) { + let _guard = lock_cache(); let Some(mut cache) = load_cache() else { return; }; cache.last_notification = Some(now_rfc3339()); + cache.last_notified_version = Some(version.to_string()); save_cache(&cache); } @@ -329,7 +373,6 @@ fn fetch_latest_version() -> Result { let servers = get_update_servers(); let mut last_err = None; - let prior_notification = load_cache().and_then(|c| c.last_notification); for url in &servers { match client.get(url).send() { @@ -347,16 +390,22 @@ fn fetch_latest_version() -> Result { )); continue; } + // Re-read the notice state INSIDE the lock rather than + // before the request. This struct is rebuilt from scratch, + // and a notice recorded while the request was in flight + // would otherwise be overwritten with the stale value read + // minutes earlier — telling the user twice about the same + // release. + let _guard = lock_cache(); + let prior = load_cache(); let cache = UpdateCache { last_check: now_rfc3339(), latest_version: version, release_url: info.html_url, - // Carry the notice timestamp across the refresh. This - // struct is rebuilt from scratch, so dropping the field - // here would reset the notify throttle on every check - // and `notify_interval_hours` would silently do nothing - // beyond one check interval. - last_notification: prior_notification.clone(), + last_notification: prior.as_ref().and_then(|c| c.last_notification.clone()), + last_notified_version: prior + .as_ref() + .and_then(|c| c.last_notified_version.clone()), }; save_cache(&cache); return Ok(cache); @@ -429,14 +478,24 @@ pub fn print_update_notice(current: &str, latest: &str, url: &str, use_color: bo current, console::style(latest).green().bold(), ); - eprintln!( - " Run {} to update, or visit {}", - console::style("perry update").cyan(), - url, - ); + // A custom manifest may carry only `version`, and "or visit " with + // nothing after it reads like a bug. + if url.is_empty() { + eprintln!(" Run {} to update", console::style("perry update").cyan()); + } else { + eprintln!( + " Run {} to update, or visit {}", + console::style("perry update").cyan(), + url, + ); + } } else { eprintln!("\nUpdate: {} -> {} available", current, latest); - eprintln!(" Run `perry update` to update, or visit {}", url); + if url.is_empty() { + eprintln!(" Run `perry update` to update"); + } else { + eprintln!(" Run `perry update` to update, or visit {}", url); + } } } @@ -1553,6 +1612,7 @@ mod tests { latest_version: "0.2.171".to_string(), release_url: "https://github.com/PerryTS/perry/releases/tag/v0.2.171".to_string(), last_notification: Some("2025-01-15T11:00:00Z".to_string()), + last_notified_version: Some("0.2.171".to_string()), }; let json = serde_json::to_string(&cache).unwrap(); diff --git a/crates/perry/src/update_policy.rs b/crates/perry/src/update_policy.rs index 261e801bae..7405c71090 100644 --- a/crates/perry/src/update_policy.rs +++ b/crates/perry/src/update_policy.rs @@ -62,6 +62,16 @@ pub(crate) enum UpdateMode { } impl UpdateMode { + pub(crate) fn label(self) -> &'static str { + match self { + Self::Off => "off", + Self::Notify => "notify", + Self::Prompt => "prompt", + Self::Auto => "auto", + Self::Unknown => "notify (unrecognized value in config)", + } + } + pub(crate) fn parse(raw: &str) -> Option { match raw.trim().to_ascii_lowercase().as_str() { "off" => Some(Self::Off), @@ -120,10 +130,29 @@ impl UpdateConfig { pub(crate) struct UpdatePolicy { /// Already accounts for the environment, CI, and whether stderr is a /// terminal — so a caller never has to re-derive "should I be quiet". + /// Use [`Self::configured_mode`] when REPORTING the setting rather than + /// acting on it: this is the decision for one run, and a suppressed run does + /// not mean the user configured `off`. pub(crate) mode: UpdateMode, + /// What the config (or `PERRY_UPDATE_MODE`) says, before this run's + /// suppression rules. + /// + /// `perry doctor` exists to answer "what is my setting?", and answering with + /// the effective mode made `doctor --format json`, `doctor | less` and every + /// CI run report `off` regardless of the file — the exact question asked. + pub(crate) configured_mode: UpdateMode, pub(crate) check_interval: Duration, pub(crate) notify_interval: Duration, pub(crate) prompt_default: bool, + /// A complaint about the config, to print only if this run is going to + /// speak at all. + /// + /// Emitting it from `resolve` would write to stderr before the precedence + /// rules below have been applied — so `--format json`, `CI`, a piped stderr + /// or `--quiet` would each get a stray line in the middle of output nobody + /// asked to be interrupted. The whole point of those rules is that this run + /// stays silent. + pub(crate) config_warning: Option<&'static str>, } /// The environment inputs, gathered in one place so the decision itself is a @@ -219,20 +248,26 @@ impl UpdatePolicy { let config = crate::commands::publish::load_config() .update .unwrap_or_default(); - if matches!(config.mode, Some(UpdateMode::Unknown)) { - // One line, once, on the way past. Loud enough to fix, quiet - // enough not to be the thing the user remembers about the run. - eprintln!( - "warning: unrecognized `[update] mode` in ~/.perry/config.toml; \ - using \"notify\". Valid values: off, notify, prompt, auto." - ); - } + // Held, not printed. Emitting here would write to stderr before the + // precedence rules above have been applied, so a `--format json` run, + // a CI job, a piped stderr or `--quiet` would each get a stray line in + // the middle of output nobody asked to have interrupted. + let config_warning = matches!(config.mode, Some(UpdateMode::Unknown)).then_some( + "warning: unrecognized `[update] mode` in ~/.perry/config.toml; using \ + \"notify\". Valid values: off, notify, prompt, auto.", + ); Self { mode: resolve_mode(env, config.mode), + // `Unknown` is PRESERVED here rather than collapsed to `Notify`. + // Its label is "notify (unrecognized value in config)", which is + // exactly what `doctor` should say when the config has a typo in it; + // collapsing it reported a clean "notify" and hid the typo. + configured_mode: config.mode.unwrap_or(UpdateMode::Notify), check_interval: config.check_interval(), notify_interval: config.notify_interval(), prompt_default: config.prompt_default.unwrap_or(false), + config_warning, } } @@ -269,11 +304,21 @@ fn structured_output_selected() -> bool { pub(crate) fn should_notify( notify_interval: Duration, last_notification: Option<&str>, + last_notified_version: Option<&str>, + latest: &str, now_rfc3339: &str, ) -> bool { if notify_interval.is_zero() { return true; } + // The interval throttles repeats of the SAME update, which is what it has + // always said it does. Keying it on time alone silently swallowed the next + // release whenever it arrived inside the window — so a user who set a + // week-long interval to stop being nagged about one version would also miss + // the one that fixed it. + if last_notified_version != Some(latest) { + return true; + } let (Some(last), Some(now)) = ( crate::update_checker::parse_rfc3339(last_notification.unwrap_or("")), crate::update_checker::parse_rfc3339(now_rfc3339), @@ -283,7 +328,350 @@ pub(crate) fn should_notify( // cache would hide updates indefinitely. return true; }; - now.saturating_sub(last) >= notify_interval.as_secs() as i64 + now.saturating_sub(last).max(0) as u64 >= notify_interval.as_secs() +} + +/// What the update surface should do at the end of a run, given the mode and +/// what the machine will allow. +/// +/// Pure, and separated from doing it, because the interesting decisions here +/// are all refusals — and a refusal that only exists inside an `if` in the +/// middle of a teardown path is a refusal nobody can test. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum TeardownAction { + /// Say nothing at all. + Silent, + /// Print the notice and stop. + Notify, + /// Print the notice, then ask. + Ask, + /// Print a line and install without asking. + Install, + /// Print the notice, then name the command this channel understands. + DeferToChannel(crate::install_channel::InstallChannel), + /// Print the notice, then say the install directory is not writable. + NeedsElevation, +} + +/// Inputs to [`decide_teardown`] that come from the machine rather than from +/// the user's configuration. +#[derive(Debug, Clone, Copy)] +pub(crate) struct TeardownEnv { + /// Did the command the user actually asked for succeed? + pub(crate) command_succeeded: bool, + pub(crate) stdin_is_terminal: bool, + pub(crate) channel: crate::install_channel::InstallChannel, + pub(crate) install_dir_writable: bool, +} + +pub(crate) fn decide_teardown(mode: UpdateMode, env: TeardownEnv) -> TeardownAction { + use crate::install_channel::InstallChannel; + + match mode { + UpdateMode::Off | UpdateMode::Unknown => return TeardownAction::Silent, + UpdateMode::Notify => return TeardownAction::Notify, + UpdateMode::Prompt | UpdateMode::Auto => {} + } + + // Never offer to install after the command failed. The user is looking at + // an error; a question about upgrading is noise at the worst possible + // moment, and an unattended install would bury the error entirely. + if !env.command_succeeded { + return TeardownAction::Notify; + } + + // A managed install is not ours to replace, whichever mode asked. Say what + // the owner understands instead — a refusal with no alternative is a dead + // end. + if env.channel != InstallChannel::SelfManaged { + return TeardownAction::DeferToChannel(env.channel); + } + + // Discovered before anything is downloaded, so the failure is a sentence + // rather than a half-finished install. + if !env.install_dir_writable { + return TeardownAction::NeedsElevation; + } + + match mode { + // A prompt needs somewhere to read the answer from. stderr already + // being a terminal is not enough — stdin can be a pipe while stderr is + // a tty, and reading from it would either block or take whatever the + // pipe held as consent. + UpdateMode::Prompt if env.stdin_is_terminal => TeardownAction::Ask, + UpdateMode::Prompt => TeardownAction::Notify, + UpdateMode::Auto => TeardownAction::Install, + _ => TeardownAction::Notify, + } +} + +/// Carry out [`decide_teardown`]'s answer. +/// +/// Never changes the command's exit status: an update is something that +/// happens *after* the work the user asked for, so a failure here is a warning +/// on stderr and nothing more. +/// Does the notice throttle apply to this action? +/// +/// Only to the ones that say something. An install is not a notice: the user +/// asked for `auto`, and having been told about the release earlier is no reason +/// to keep running the old binary. +pub(crate) fn throttle_applies(action: &TeardownAction) -> bool { + !matches!(action, TeardownAction::Install) +} + +pub(crate) fn run_teardown_action( + policy: &UpdatePolicy, + status: &crate::update_checker::UpdateStatus, + command_succeeded: bool, + use_color: bool, + verbose: bool, + notice_throttled: bool, +) { + let crate::update_checker::UpdateStatus::UpdateAvailable { + current, + latest, + release_url, + } = status + else { + return; + }; + + let notice = || { + crate::update_checker::print_update_notice(current, latest, release_url, use_color); + // Only record when we actually said something, or the throttle would + // suppress the next notice on the strength of one nobody saw. + if !crate::install_channel::running_via_sudo() { + // The version, so the interval throttles repeats of THIS release + // rather than of "some release" — see `should_notify`. + crate::update_checker::record_notification(latest); + } + }; + + let action = decide_teardown( + policy.mode, + TeardownEnv { + command_succeeded, + stdin_is_terminal: std::io::stdin().is_terminal(), + channel: crate::install_channel::detect(), + install_dir_writable: crate::install_channel::install_dir_is_writable(), + }, + ); + + // The throttle is applied HERE, to the resolved action, rather than around + // the whole call. `notify_interval_hours` silences repeats of a notice, and + // gating the call gated the install with it: in `auto` mode a notice printed + // an hour ago stopped the new version from ever landing, which is the one + // thing `auto` promises to do. + if notice_throttled && throttle_applies(&action) { + return; + } + + match action { + TeardownAction::Silent => {} + TeardownAction::Notify => notice(), + TeardownAction::DeferToChannel(channel) => { + notice(); + if let Some(command) = channel.upgrade_command() { + eprintln!( + " This perry was installed by {}, so `perry update` would \ + overwrite it behind that tool's back. Run `{}` instead.", + channel.label(), + command + ); + if let Some(detail) = channel.refusal_detail() { + eprintln!(" ({detail})"); + } + } + } + TeardownAction::NeedsElevation => { + notice(); + eprintln!( + " The install directory is not writable by this user, so the \ + update was not attempted. Run `sudo perry update`." + ); + } + TeardownAction::Ask => { + notice(); + let accepted = dialoguer::Confirm::new() + .with_prompt(format!("Update perry to {latest} now?")) + .default(policy.prompt_default) + .interact() + .unwrap_or(false); + if accepted { + install_now(use_color, verbose); + } else { + eprintln!( + " Skipped. Set `[update] mode = \"notify\"` in \ + ~/.perry/config.toml to stop being asked." + ); + } + } + TeardownAction::Install => { + eprintln!(" Installing perry {latest}..."); + install_now(use_color, verbose); + } + } +} + +fn install_now(use_color: bool, verbose: bool) { + if let Err(error) = + crate::update_checker::perform_self_update(crate::update_checker::UpdateOutput { + verbose, + quiet: false, + color: use_color, + }) + { + // A warning, never an exit status: the command the user asked for has + // already finished, and its result is the one that matters. + eprintln!("warning: update failed: {error}"); + } +} + +#[cfg(test)] +mod teardown_tests { + use super::*; + use crate::install_channel::InstallChannel; + + /// ★ The notice throttle silences notices, not installs. + /// + /// `notify_interval_hours` used to gate the whole teardown call, so in + /// `auto` mode a notice printed an hour earlier stopped the update from + /// landing at all — the one thing `auto` exists to do. + #[test] + fn the_notice_throttle_never_holds_back_an_install() { + assert!( + !throttle_applies(&TeardownAction::Install), + "an install is not a notice and must not be throttled" + ); + for action in [ + TeardownAction::Notify, + TeardownAction::Ask, + TeardownAction::DeferToChannel(crate::install_channel::InstallChannel::Homebrew), + ] { + assert!( + throttle_applies(&action), + "{action:?} speaks, so the throttle applies to it" + ); + } + } + + + /// An interactive terminal, a successful command, an unmanaged install + /// with a writable directory — the only shape in which anything installs. + fn ideal() -> TeardownEnv { + TeardownEnv { + command_succeeded: true, + stdin_is_terminal: true, + channel: InstallChannel::SelfManaged, + install_dir_writable: true, + } + } + + #[test] + fn off_says_nothing_and_notify_only_notifies() { + assert_eq!( + decide_teardown(UpdateMode::Off, ideal()), + TeardownAction::Silent + ); + assert_eq!( + decide_teardown(UpdateMode::Notify, ideal()), + TeardownAction::Notify + ); + } + + #[test] + fn prompt_asks_and_auto_installs_when_everything_allows_it() { + assert_eq!( + decide_teardown(UpdateMode::Prompt, ideal()), + TeardownAction::Ask + ); + assert_eq!( + decide_teardown(UpdateMode::Auto, ideal()), + TeardownAction::Install + ); + } + + /// ★ After a failed command, neither mode may do anything but notify. The + /// user is reading an error; a question about upgrading is noise, and an + /// unattended install would bury the error under progress output. + #[test] + fn a_failed_command_downgrades_both_active_modes_to_a_notice() { + let failed = TeardownEnv { + command_succeeded: false, + ..ideal() + }; + assert_eq!( + decide_teardown(UpdateMode::Prompt, failed), + TeardownAction::Notify + ); + assert_eq!( + decide_teardown(UpdateMode::Auto, failed), + TeardownAction::Notify + ); + } + + /// ★ The refusal that matters most. A package manager owns its record of + /// what is installed; overwriting the binary underneath leaves that record + /// lying. + #[test] + fn a_managed_install_is_never_replaced_in_place() { + for channel in [ + InstallChannel::Homebrew, + InstallChannel::Npm, + InstallChannel::Apt, + InstallChannel::Winget, + ] { + let env = TeardownEnv { channel, ..ideal() }; + for mode in [UpdateMode::Prompt, UpdateMode::Auto] { + assert_eq!( + decide_teardown(mode, env), + TeardownAction::DeferToChannel(channel), + "{:?} on {} must defer, not install", + mode, + channel.label() + ); + } + } + } + + /// Checked before anything is downloaded, so a root-owned + /// `/usr/local/bin` produces one sentence rather than a half-finished + /// install. Perry never escalates on its own. + #[test] + fn an_unwritable_install_directory_asks_for_elevation_instead_of_trying() { + let env = TeardownEnv { + install_dir_writable: false, + ..ideal() + }; + assert_eq!( + decide_teardown(UpdateMode::Auto, env), + TeardownAction::NeedsElevation + ); + assert_eq!( + decide_teardown(UpdateMode::Prompt, env), + TeardownAction::NeedsElevation + ); + } + + /// stderr being a terminal is not enough to ask a question: stdin can be a + /// pipe at the same time, and reading from it would either block or treat + /// whatever the pipe held as consent. + #[test] + fn prompt_degrades_to_notify_when_stdin_is_not_a_terminal() { + let env = TeardownEnv { + stdin_is_terminal: false, + ..ideal() + }; + assert_eq!( + decide_teardown(UpdateMode::Prompt, env), + TeardownAction::Notify + ); + assert_eq!( + decide_teardown(UpdateMode::Auto, env), + TeardownAction::Install, + "auto asks nothing, so it does not need stdin" + ); + } } #[cfg(test)] @@ -454,10 +842,20 @@ mod tests { ); } + /// The interval alone, with the announced version held constant — which is + /// what these cases were written to exercise. + fn notified_before(interval: Duration, last_notification: Option<&str>, now: &str) -> bool { + should_notify(interval, last_notification, Some("1.0.0"), "1.0.0", now) + } + #[test] fn the_notify_throttle_defaults_to_every_run() { - assert!(should_notify(Duration::ZERO, None, "2026-08-10T00:00:00Z")); - assert!(should_notify( + assert!(notified_before( + Duration::ZERO, + None, + "2026-08-10T00:00:00Z" + )); + assert!(notified_before( Duration::ZERO, Some("2026-08-10T00:00:00Z"), "2026-08-10T00:00:01Z" @@ -468,27 +866,79 @@ mod tests { fn the_notify_throttle_honours_its_interval() { let day = Duration::from_secs(24 * 3600); assert!( - !should_notify(day, Some("2026-08-10T00:00:00Z"), "2026-08-10T01:00:00Z"), + !notified_before(day, Some("2026-08-10T00:00:00Z"), "2026-08-10T01:00:00Z"), "an hour into a one-day throttle must stay quiet" ); assert!( - should_notify(day, Some("2026-08-09T00:00:00Z"), "2026-08-10T01:00:00Z"), + notified_before(day, Some("2026-08-09T00:00:00Z"), "2026-08-10T01:00:00Z"), "past the interval it must speak up" ); } + /// ★ The interval throttles repeats of the SAME update, which is what it + /// always claimed. Keyed on time alone it swallowed the NEXT release + /// whenever that landed inside the window — so a week-long interval set to + /// stop nagging about one version would also hide the version that fixed + /// it. + #[test] + fn a_different_version_is_announced_regardless_of_the_interval() { + let week = Duration::from_secs(7 * 24 * 3600); + // One minute into a week-long throttle: the same version stays quiet... + assert!(!should_notify( + week, + Some("2026-08-10T00:00:00Z"), + Some("1.0.0"), + "1.0.0", + "2026-08-10T00:01:00Z" + )); + // ...and a different one is announced anyway. + assert!(should_notify( + week, + Some("2026-08-10T00:00:00Z"), + Some("1.0.0"), + "1.0.1", + "2026-08-10T00:01:00Z" + )); + // Never having announced anything is also "not this version". + assert!(should_notify( + week, + Some("2026-08-10T00:00:00Z"), + None, + "1.0.0", + "2026-08-10T00:01:00Z" + )); + } + + /// An enormous configured interval must still suppress, not wrap around + /// into announcing every run. `as i64` on a `Duration`'s seconds can go + /// negative, and a negative interval compares as "already elapsed". + #[test] + fn an_enormous_interval_still_suppresses() { + let absurd = Duration::from_secs(u64::MAX); + assert!( + !should_notify( + absurd, + Some("2026-08-10T00:00:00Z"), + Some("1.0.0"), + "1.0.0", + "2026-08-10T00:01:00Z" + ), + "a signed conversion here would read as already-elapsed and notify" + ); + } + /// A cache this build cannot read must not silence the notice forever — /// that would turn one bad write into a permanently muted checker. #[test] fn an_unreadable_timestamp_notifies_rather_than_staying_silent() { let day = Duration::from_secs(24 * 3600); - assert!(should_notify(day, None, "2026-08-10T00:00:00Z")); - assert!(should_notify( + assert!(notified_before(day, None, "2026-08-10T00:00:00Z")); + assert!(notified_before( day, Some("not-a-date"), "2026-08-10T00:00:00Z" )); - assert!(should_notify( + assert!(notified_before( day, Some("2026-08-10T00:00:00Z"), "also-not-a-date"