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/7785-update-check-sources.md b/changelog.d/7785-update-check-sources.md new file mode 100644 index 0000000000..eeb0bd0107 --- /dev/null +++ b/changelog.d/7785-update-check-sources.md @@ -0,0 +1,92 @@ +### Added + +**Where Perry asks "what is the latest version?" is now a choice.** It used to +walk one fixed list — an override, the config, Perry Hub, then the GitHub +releases API — and read a GitHub-releases-shaped document from whichever +answered first. That is fine while everyone installs the same way, and wrong as +soon as they do not: an npm user's "latest" is whatever the registry's `latest` +dist-tag says, and asking GitHub instead can announce a version their package +manager cannot install yet. + +```toml +[update] +source = "npm" # gh-releases | npm | gh-registry | custom +package = "@perryts/perry" # npm-shaped sources; defaults to Perry's own +registry = "..." # npm-shaped sources; defaults to the public registry +server = "..." # the URL for `custom`, and the mirror override +``` + +Unset keeps the historical ladder, so nothing changes for anyone who does not +set it — except on an **npm-managed install**, which now defaults to asking npm, +because that is the version its own package manager can actually install. + +
+The split that matters: checking is not downloading + +A check source answers one question and returns a version, a link, a publish +time and a headline. It does **not** decide where the binary comes from. +Artifacts and their signed manifest always resolve from the release +infrastructure, whatever the check source is. + +That separation is load-bearing rather than tidy. The manifest — Ed25519 over +the artifact's digest and version — is what makes a self-update trustworthy, +and a check source is a URL a user can point anywhere. Letting it redirect the +download would turn a configuration setting into a way to install an arbitrary +binary. Whoever answers "what is new?" never gets to answer "what should I +run?", and there is a test that fails if a source ever leaks into the artifact +ladder. + +The old `get_update_servers` and its private config reader are **deleted** +rather than left beside the new code, so the compiler enforces that both call +sites moved. A new abstraction with the old ladder still wired up underneath is +the shape where four sources exist, pass their own tests, and are never +reached. +
+ +
+Credentials go to exactly one of the four + +The npm shapes ask for the *abbreviated* packument +(`Accept: application/vnd.npm.install-v1+json`) — smaller, cacheable, and the +document npm itself requests for this question. It also avoids GitHub's +unauthenticated API rate limit, which the old ladder shared with everything +else on the machine. + +The public registry is asked **without credentials**, and a test asserts no +`Authorization` header is sent: a token there would be a leak, not a +convenience. GitHub Packages does need one, so that shape reads `GH_TOKEN` / +`GITHUB_TOKEN` and fails with a sentence naming the fix when neither is set, +rather than retrying anonymously and reporting the resulting 404 as "up to +date". + +A configured source does not fall back to the ladder when it errors. Somebody +who said "ask npm" and got a failure wants to hear that, not a version from +somewhere they never named. +
+ +
+Tests + +11 new, all parsing real response shapes from string fixtures so no network is +involved: + +- a GitHub release document, including that the `v` prefix is stripped; +- an abbreviated packument, which has no `time` map — so the publish date reads + "unknown" rather than being invented, which matters because the release + cooldown in the next slice depends on it; +- a full packument, which does supply it; +- a custom manifest with only a `version`, and one with every optional field; +- that each shape **rejects the others' documents** rather than reading a field + that happens to be present — a registry answering a gh-releases request must + be an error, not a version of `""`; +- that a scoped package's `/` is percent-encoded, or the registry reads the + scope as a path segment and answers 404; +- that an unknown `source` name falls back instead of failing, so a config + written by a newer Perry does not break an older one; +- that `custom` with no URL is treated as a missing key rather than a default; +- that an npm install defaults to npm and every other channel keeps the ladder; +- that no check source can reach the artifact ladder; +- and both credential rules. + +`cargo test -p perry`: 925 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..d55ea7ca07 100644 --- a/crates/perry/src/main.rs +++ b/crates/perry/src/main.rs @@ -4,8 +4,10 @@ mod commands; mod compat_reports; +mod install_channel; #[cfg(test)] mod panic_profile_contract; +mod release_source; mod shadow_layout_contract; mod telemetry; #[cfg(test)] @@ -542,12 +544,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 +567,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/release_source.rs b/crates/perry/src/release_source.rs new file mode 100644 index 0000000000..6068820999 --- /dev/null +++ b/crates/perry/src/release_source.rs @@ -0,0 +1,642 @@ +//! Where Perry asks "what is the latest version?". +//! +//! Until now there was one answer: walk a fixed list of URLs — an override, the +//! config, Perry Hub, then the GitHub releases API — and read a +//! GitHub-releases-shaped document from whichever replied first. That is fine +//! when everyone installs the same way, and wrong as soon as they do not: an +//! npm user's "latest" is whatever the registry's `latest` dist-tag says, and +//! asking GitHub instead can announce a version their package manager cannot +//! yet install. +//! +//! So the source is now a choice, with four shapes. +//! +//! # The split that matters: checking is not downloading +//! +//! A check source answers one question and returns [`VersionProbe`]. It does +//! **not** decide where the binary comes from. Artifacts and their signed +//! manifest always resolve from the release infrastructure, through +//! [`release_info_servers`], whatever the check source is. +//! +//! That separation is deliberate and load-bearing. The manifest is what makes +//! a self-update trustworthy — Ed25519 over the artifact's digest and version — +//! and a check source is a URL a user can point anywhere. Letting the check +//! source redirect the download would turn a configuration setting into a way +//! to install an arbitrary binary. Whoever answers "what is new?" never gets to +//! answer "what should I run?". +//! +//! # Why the npm shapes send no credentials to a public registry +//! +//! The public registry needs no auth, and the abbreviated packument +//! (`Accept: application/vnd.npm.install-v1+json`) is the cheap, cacheable +//! document intended for exactly this question. GitHub Packages does need a +//! token, so that shape reads `GH_TOKEN` / `GITHUB_TOKEN` — the same variables +//! `gh` and every CI job already set — and simply fails when neither is +//! present rather than retrying without them. + +use anyhow::{Context, Result}; +use serde::Deserialize; + +/// The public npm registry, used when a source names a package but no registry. +const NPM_REGISTRY: &str = "https://registry.npmjs.org"; +/// GitHub Packages' npm endpoint. +const GH_REGISTRY: &str = "https://npm.pkg.github.com"; +/// The npm package Perry publishes its wrapper as. +const PERRY_NPM_PACKAGE: &str = "@perryts/perry"; + +/// What every source returns, whatever document it read. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct VersionProbe { + pub(crate) latest_version: String, + /// Somewhere a human can read about this version. Never used to download. + pub(crate) release_url: String, + /// RFC3339 publish time, when the source reports one. Feeds the release + /// cooldown: a version too fresh to have been noticed by anyone yet is not + /// one to install unattended. + pub(crate) published_at: Option, + /// A one-line title, when the source has one. + pub(crate) headline: Option, +} + +/// Which document to read, and where. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum CheckSource { + /// A GitHub releases API URL. The historical behaviour, and the only shape + /// that also carries the assets the installer needs. + GhReleases { url: String }, + /// An npm registry packument — the public registry unless told otherwise. + Npm { package: String, registry: String }, + /// GitHub Packages, which is npm-shaped but always authenticated. + GhRegistry { package: String, registry: String }, + /// Any HTTPS URL returning `{ "version": ..., "release_url": ... }`. + Custom { url: String }, +} + +/// Parse a configured `source` name into a source, given the other keys. +/// +/// Returns `None` for a name this build does not know, so the caller can fall +/// back rather than fail — a config written by a newer Perry must not break an +/// older one's update check. +pub(crate) fn from_config( + source: Option<&str>, + package: Option<&str>, + registry: Option<&str>, + server: Option<&str>, +) -> Option { + let package = || package.unwrap_or(PERRY_NPM_PACKAGE).to_string(); + match source?.trim().to_ascii_lowercase().as_str() { + "gh-releases" | "github" => Some(CheckSource::GhReleases { + url: server + .unwrap_or(super::update_checker::GITHUB_URL) + .to_string(), + }), + "npm" | "npm-registry" => Some(CheckSource::Npm { + package: package(), + registry: registry.unwrap_or(NPM_REGISTRY).to_string(), + }), + "gh-registry" | "github-packages" => Some(CheckSource::GhRegistry { + package: package(), + registry: registry.unwrap_or(GH_REGISTRY).to_string(), + }), + // `custom` without a URL is meaningless rather than harmless: silently + // treating it as "the default ladder" would hide the missing key. + "custom" => server.map(|url| CheckSource::Custom { + url: url.to_string(), + }), + _ => None, + } +} + +/// The source to use when the config names none. +/// +/// An npm-managed install asks npm, because that is the version its own +/// package manager can actually install — announcing a GitHub release the +/// wrapper package has not published yet is worse than saying nothing. Every +/// other install falls back to the historical ladder, which is what +/// [`release_info_servers`] walks. +pub(crate) fn default_for_channel( + channel: crate::install_channel::InstallChannel, +) -> Option { + match channel { + crate::install_channel::InstallChannel::Npm => Some(CheckSource::Npm { + package: PERRY_NPM_PACKAGE.to_string(), + registry: NPM_REGISTRY.to_string(), + }), + _ => None, + } +} + +/// The release-infrastructure URLs, in preference order. +/// +/// This is the ARTIFACT ladder as well as the fallback check ladder, and it is +/// the only thing the installer reads: it is where the signed `.update.json` +/// manifest lives. `PERRY_UPDATE_SERVER` and `[update] server` still come +/// first, which is what makes a private mirror work. +pub(crate) fn release_info_servers() -> Vec { + let mut servers = Vec::new(); + // A configured server is dropped unless it is HTTPS or loopback. Plain HTTP + // here is not only readable in transit — anyone who can answer the request + // can suppress updates indefinitely by reporting the running version as the + // latest one, which is a silent way to keep a machine on a vulnerable build. + if let Ok(url) = std::env::var("PERRY_UPDATE_SERVER") { + if !url.is_empty() && url_is_secure(&url) { + servers.push(url); + } + } + if servers.is_empty() { + if let Some(url) = crate::commands::publish::load_config() + .update + .and_then(|u| u.server) + { + if url_is_secure(&url) { + servers.push(url); + } + } + } + servers.push(super::update_checker::HUB_URL.to_string()); + servers.push(super::update_checker::GITHUB_URL.to_string()); + servers +} + +/// Is this URL safe to send a version check to? +/// +/// HTTPS, or loopback so a local test server still works. Loopback needs a `:`, +/// a `/`, or the end of the string after the prefix, or `http://localhost.evil` +/// would pass as localhost. +pub(crate) fn url_is_secure(url: &str) -> bool { + let lower = url.to_ascii_lowercase(); + if lower.starts_with("https://") { + return true; + } + ["http://127.0.0.1", "http://localhost", "http://[::1]"] + .iter() + .any(|prefix| { + lower.strip_prefix(prefix).is_some_and(|rest| { + rest.is_empty() || rest.starts_with(':') || rest.starts_with('/') + }) + }) +} + +/// The source this run should use: the config's choice, else the install +/// channel's default, else none — meaning "walk the historical ladder". +pub(crate) fn resolve() -> Option { + let config = crate::commands::publish::load_config() + .update + .unwrap_or_default(); + from_config( + config.source.as_deref(), + config.package.as_deref(), + config.registry.as_deref(), + config.server.as_deref(), + ) + .or_else(|| default_for_channel(crate::install_channel::detect())) +} + +impl CheckSource { + /// The URL to request, and the headers this shape needs. + /// Reject anything that is not an absolute HTTPS URL without credentials. + /// + /// The artifact path already required this; the CHECK path did not, and it is + /// the more dangerous of the two for `gh-registry`: an `http://` registry + /// would have had `Authorization: Bearer ` attached to a plaintext + /// request, putting a GitHub token on the wire. Loopback is exempt so a local + /// test server still works. + fn require_secure(label: &str, url: &str) -> Result<()> { + let lower = url.to_ascii_lowercase(); + if !url_is_secure(url) { + anyhow::bail!( + "[update] {label} must be an https:// URL (got `{url}`). \ + Loopback http:// is allowed for local testing." + ); + } + // Credentials in a URL are sent to whatever host follows the `@`, and + // land in logs besides. + let authority = lower + .split_once("://") + .map(|(_, rest)| rest.split('/').next().unwrap_or_default()) + .unwrap_or_default(); + if authority.contains('@') { + anyhow::bail!("[update] {label} must not embed credentials: `{url}`"); + } + Ok(()) + } + + pub(crate) fn request(&self) -> Result<(String, Vec<(&'static str, String)>)> { + match self { + Self::GhReleases { url } | Self::Custom { url } => { + Self::require_secure("server", url)?; + Ok((url.clone(), Vec::new())) + } + Self::Npm { package, registry } => Ok(( + { + Self::require_secure("registry", registry)?; + packument_url(registry, package) + }, + // The abbreviated document: smaller, cacheable, and the one npm + // itself asks for. No credentials — the public registry wants + // none, and sending a token to it would be a leak, not a + // convenience. + vec![("Accept", "application/vnd.npm.install-v1+json".to_string())], + )), + Self::GhRegistry { package, registry } => { + let token = std::env::var("GH_TOKEN") + .or_else(|_| std::env::var("GITHUB_TOKEN")) + .ok() + .filter(|t| !t.is_empty()) + .context( + "GitHub Packages needs a token: set GH_TOKEN or GITHUB_TOKEN, \ + or use `source = \"npm\"` for the public registry", + )?; + // Checked BEFORE the token is attached, not after. + Self::require_secure("registry", registry)?; + Ok(( + packument_url(registry, package), + vec![ + ("Accept", "application/vnd.npm.install-v1+json".to_string()), + ("Authorization", format!("Bearer {token}")), + ], + )) + } + } + } + + /// Turn this shape's response body into a probe. + pub(crate) fn parse(&self, body: &str) -> Result { + match self { + Self::GhReleases { .. } => parse_gh_release(body), + Self::Npm { package, .. } | Self::GhRegistry { package, .. } => { + parse_packument(body, package) + } + Self::Custom { .. } => parse_custom(body), + } + } + + pub(crate) fn label(&self) -> &'static str { + match self { + Self::GhReleases { .. } => "gh-releases", + Self::Npm { .. } => "npm", + Self::GhRegistry { .. } => "gh-registry", + Self::Custom { .. } => "custom", + } + } +} + +/// npm requires the scope's `/` to be percent-encoded in a packument path. +fn packument_url(registry: &str, package: &str) -> String { + format!( + "{}/{}", + registry.trim_end_matches('/'), + package.replace('/', "%2F") + ) +} + +fn parse_gh_release(body: &str) -> Result { + #[derive(Deserialize)] + struct Release { + tag_name: String, + html_url: String, + #[serde(default)] + name: Option, + #[serde(default)] + published_at: Option, + } + let release: Release = serde_json::from_str(body) + .context("update server returned a document that is not a release")?; + Ok(VersionProbe { + latest_version: release + .tag_name + .strip_prefix('v') + .unwrap_or(&release.tag_name) + .to_string(), + release_url: release.html_url, + published_at: release.published_at, + headline: release.name.filter(|n| !n.trim().is_empty()), + }) +} + +fn parse_packument(body: &str, package: &str) -> Result { + #[derive(Deserialize)] + struct Packument { + #[serde(rename = "dist-tags")] + dist_tags: DistTags, + /// Present in the FULL packument, absent from the abbreviated one, so + /// the cooldown falls back to "unknown" rather than to a wrong answer. + #[serde(default)] + time: std::collections::HashMap, + } + #[derive(Deserialize)] + struct DistTags { + latest: String, + } + let packument: Packument = serde_json::from_str(body) + .context("registry returned a document without a `dist-tags.latest`")?; + let latest = packument.dist_tags.latest; + let published_at = packument.time.get(&latest).cloned(); + Ok(VersionProbe { + release_url: format!("https://www.npmjs.com/package/{package}/v/{latest}"), + latest_version: latest, + published_at, + headline: None, + }) +} + +fn parse_custom(body: &str) -> Result { + #[derive(Deserialize)] + struct Manifest { + version: String, + #[serde(default)] + release_url: Option, + #[serde(default)] + published_at: Option, + #[serde(default)] + notes: Option, + } + let manifest: Manifest = serde_json::from_str(body) + .context("a custom update source must return {\"version\": \"...\"}")?; + Ok(VersionProbe { + release_url: manifest.release_url.unwrap_or_default(), + latest_version: manifest + .version + .strip_prefix('v') + .unwrap_or(&manifest.version) + .to_string(), + published_at: manifest.published_at, + headline: manifest.notes.filter(|n| !n.trim().is_empty()), + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::install_channel::InstallChannel; + + #[test] + fn a_github_release_document_yields_a_probe() { + let probe = parse_gh_release( + r#"{ + "tag_name": "v0.5.1447", + "html_url": "https://github.com/PerryTS/perry/releases/tag/v0.5.1447", + "name": "Faster incremental builds", + "published_at": "2026-08-10T09:00:00Z" + }"#, + ) + .expect("parse"); + assert_eq!( + probe.latest_version, "0.5.1447", + "the `v` prefix is stripped" + ); + assert_eq!(probe.published_at.as_deref(), Some("2026-08-10T09:00:00Z")); + assert_eq!(probe.headline.as_deref(), Some("Faster incremental builds")); + } + + /// The abbreviated packument is what npm itself asks for and has no `time` + /// map, so the cooldown must read "unknown" rather than inventing a date. + #[test] + fn an_abbreviated_packument_yields_a_version_without_a_date() { + let probe = parse_packument( + r#"{"dist-tags":{"latest":"0.5.1447"},"versions":{}}"#, + "@perryts/perry", + ) + .expect("parse"); + assert_eq!(probe.latest_version, "0.5.1447"); + assert_eq!(probe.published_at, None); + assert!(probe.release_url.contains("@perryts/perry")); + } + + #[test] + fn a_full_packument_supplies_the_publish_time() { + let probe = parse_packument( + r#"{ + "dist-tags": {"latest": "0.5.1447"}, + "time": {"0.5.1446": "2026-08-01T00:00:00Z", "0.5.1447": "2026-08-10T09:00:00Z"} + }"#, + "@perryts/perry", + ) + .expect("parse"); + assert_eq!(probe.published_at.as_deref(), Some("2026-08-10T09:00:00Z")); + } + + #[test] + fn a_custom_manifest_needs_only_a_version() { + let probe = parse_custom(r#"{"version":"1.2.3"}"#).expect("parse"); + assert_eq!(probe.latest_version, "1.2.3"); + assert_eq!(probe.release_url, ""); + + let full = parse_custom( + r#"{"version":"v1.2.3","release_url":"https://example.test/1.2.3", + "published_at":"2026-08-10T09:00:00Z","notes":"Bug fixes"}"#, + ) + .expect("parse"); + assert_eq!(full.latest_version, "1.2.3"); + assert_eq!(full.headline.as_deref(), Some("Bug fixes")); + } + + /// Each shape must reject the others' documents rather than reading a + /// field that happens to be there. A registry answering a gh-releases + /// request must be an error, not a version of `""`. + #[test] + fn a_source_rejects_another_shapes_document() { + assert!(parse_gh_release(r#"{"dist-tags":{"latest":"1.0.0"}}"#).is_err()); + assert!(parse_packument(r#"{"tag_name":"v1.0.0","html_url":"x"}"#, "p").is_err()); + assert!(parse_custom(r#"{"tag_name":"v1.0.0"}"#).is_err()); + assert!(parse_gh_release("not json at all").is_err()); + } + + /// A scoped package's `/` has to be percent-encoded, or the registry reads + /// the scope as a path segment and answers 404. + #[test] + fn a_scoped_package_is_percent_encoded_in_the_path() { + assert_eq!( + packument_url("https://registry.npmjs.org/", "@perryts/perry"), + "https://registry.npmjs.org/@perryts%2Fperry" + ); + } + + #[test] + fn config_names_map_to_sources_and_unknown_names_fall_back() { + assert!(matches!( + from_config(Some("npm"), None, None, None), + Some(CheckSource::Npm { .. }) + )); + assert!(matches!( + from_config(Some("gh-registry"), None, None, None), + Some(CheckSource::GhRegistry { .. }) + )); + assert!(matches!( + from_config(Some("gh-releases"), None, None, None), + Some(CheckSource::GhReleases { .. }) + )); + assert!(matches!( + from_config(Some("custom"), None, None, Some("https://example.test/v")), + Some(CheckSource::Custom { .. }) + )); + // Unknown to THIS build — fall back rather than fail, so a config + // written by a newer Perry does not break an older one. + assert_eq!(from_config(Some("carrier-pigeon"), None, None, None), None); + assert_eq!(from_config(None, None, None, None), None); + // `custom` with no URL is a missing key, not a default. + assert_eq!(from_config(Some("custom"), None, None, None), None); + } + + /// An npm-managed install asks npm, because that is the version its own + /// package manager can install. Announcing a GitHub release the wrapper + /// package has not published yet is worse than saying nothing. + #[test] + fn an_npm_install_defaults_to_the_npm_registry() { + assert!(matches!( + default_for_channel(InstallChannel::Npm), + Some(CheckSource::Npm { .. }) + )); + for channel in [ + InstallChannel::SelfManaged, + InstallChannel::Homebrew, + InstallChannel::Apt, + InstallChannel::Winget, + ] { + assert_eq!( + default_for_channel(channel), + None, + "{} keeps the historical ladder", + channel.label() + ); + } + } + + /// ★ The separation that keeps a config setting from becoming a way to + /// install an arbitrary binary: no check source may name where the + /// artifact comes from. The installer reads `release_info_servers` only. + #[test] + fn no_check_source_can_redirect_the_artifact_download() { + let custom = CheckSource::Custom { + url: "https://attacker.test/version.json".to_string(), + }; + let (url, _) = custom.request().expect("request"); + assert_eq!(url, "https://attacker.test/version.json"); + + // The artifact ladder is built from the release infrastructure and the + // operator's own override, and knows nothing about the source above. + let servers = release_info_servers(); + assert!( + !servers.iter().any(|s| s.contains("attacker.test")), + "a check source leaked into the artifact ladder: {servers:?}" + ); + assert!( + servers + .iter() + .any(|s| s == crate::update_checker::GITHUB_URL), + "the release infrastructure must stay in the ladder: {servers:?}" + ); + } + + /// A plaintext URL is refused BEFORE the token goes on the request. + /// + /// The check path is the dangerous one for `gh-registry`: an `http://` + /// registry would have carried `Authorization: Bearer ` in clear + /// text. Checking after building the headers would still leak on the retry. + #[test] + fn a_plaintext_registry_is_refused_before_a_token_is_attached() { + let _lock = crate::test_env_lock::env_lock(); + let saved = std::env::var("GH_TOKEN").ok(); + std::env::set_var("GH_TOKEN", "super-secret"); + + let insecure = CheckSource::GhRegistry { + package: "@perryts/perry".to_string(), + registry: "http://npm.internal.test".to_string(), + }; + let error = insecure.request().expect_err("plaintext must be refused"); + let text = format!("{error:#}"); + assert!(text.contains("https://"), "{text}"); + assert!( + !text.contains("super-secret"), + "the error must not echo the token: {text}" + ); + + // Every other shape is checked too, not just the authenticated one. + for source in [ + CheckSource::Custom { + url: "http://updates.test/v".into(), + }, + CheckSource::GhReleases { + url: "http://api.test/latest".into(), + }, + CheckSource::Npm { + package: "p".into(), + registry: "http://registry.test".into(), + }, + ] { + assert!( + source.request().is_err(), + "{} accepted a plaintext URL", + source.label() + ); + } + + // Loopback still works, so a local test server is usable. + let local = CheckSource::Custom { + url: "http://127.0.0.1:8080/v".into(), + }; + assert!(local.request().is_ok()); + + // And credentials in the URL are refused: they would be sent to whatever + // host follows the `@`, and would land in logs besides. + let creds = CheckSource::Custom { + url: "https://user:pw@evil.test/v".into(), + }; + assert!( + creds.request().is_err(), + "embedded credentials must be refused" + ); + + match saved { + Some(v) => std::env::set_var("GH_TOKEN", v), + None => std::env::remove_var("GH_TOKEN"), + } + } + + /// GitHub Packages always needs a token; failing with a sentence beats + /// retrying unauthenticated and reporting a 404 as "up to date". + #[test] + fn gh_registry_without_a_token_fails_with_an_explanation() { + let _lock = crate::test_env_lock::env_lock(); + let saved = ( + std::env::var("GH_TOKEN").ok(), + std::env::var("GITHUB_TOKEN").ok(), + ); + std::env::remove_var("GH_TOKEN"); + std::env::remove_var("GITHUB_TOKEN"); + + let source = CheckSource::GhRegistry { + package: "@perryts/perry".to_string(), + registry: GH_REGISTRY.to_string(), + }; + let error = source.request().expect_err("no token must be an error"); + let text = format!("{error:#}"); + assert!( + text.contains("GH_TOKEN"), + "the error must name the fix: {text}" + ); + + if let Some(v) = saved.0 { + std::env::set_var("GH_TOKEN", v); + } + if let Some(v) = saved.1 { + std::env::set_var("GITHUB_TOKEN", v); + } + } + + /// And the public registry must never be sent one. + #[test] + fn the_public_registry_is_asked_without_credentials() { + let source = CheckSource::Npm { + package: "@perryts/perry".to_string(), + registry: NPM_REGISTRY.to_string(), + }; + let (_, headers) = source.request().expect("request"); + assert!( + !headers.iter().any(|(name, _)| *name == "Authorization"), + "a token was sent to the public registry: {headers:?}" + ); + assert!(headers + .iter() + .any(|(name, value)| *name == "Accept" && value.contains("install-v1"))); + } +} diff --git a/crates/perry/src/update_checker.rs b/crates/perry/src/update_checker.rs index 43ab5efb84..8dc33592f1 100644 --- a/crates/perry/src/update_checker.rs +++ b/crates/perry/src/update_checker.rs @@ -15,8 +15,8 @@ use std::sync::mpsc; use std::thread::JoinHandle; use std::time::{Duration, Instant}; -const HUB_URL: &str = "https://hub.perryts.com/api/v1/version/latest"; -const GITHUB_URL: &str = "https://api.github.com/repos/PerryTS/perry/releases/latest"; +pub(crate) const HUB_URL: &str = "https://hub.perryts.com/api/v1/version/latest"; +pub(crate) const GITHUB_URL: &str = "https://api.github.com/repos/PerryTS/perry/releases/latest"; const CACHE_MAX_AGE: Duration = Duration::from_secs(24 * 60 * 60); const CONNECT_TIMEOUT: Duration = Duration::from_secs(5); const REQUEST_TIMEOUT: Duration = Duration::from_secs(10); @@ -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); } @@ -283,42 +327,6 @@ pub fn compare_versions(a: &str, b: &str) -> Result { Ok(a.cmp_precedence(&b)) } -fn get_update_servers() -> Vec { - let mut servers = Vec::new(); - - // 1. Environment variable (highest priority) - if let Ok(url) = std::env::var("PERRY_UPDATE_SERVER") { - if !url.is_empty() { - servers.push(url); - } - } - - // 2. Config file - if servers.is_empty() { - if let Some(url) = load_config_update_server() { - servers.push(url); - } - } - - // 3. Perry Hub - servers.push(HUB_URL.to_string()); - - // 4. GitHub API - servers.push(GITHUB_URL.to_string()); - - servers -} - -/// The configured update server, read through the SHARED config loader. -/// -/// This used to parse `~/.perry/config.toml` again into a private pair of -/// structs. That is what let `[update]` be silently erased: the real -/// `PerryConfig` had no field for it, so every `save_config` reconstructed the -/// file without it and threw the section away. -fn load_config_update_server() -> Option { - crate::commands::publish::load_config().update?.server -} - fn fetch_latest_version() -> Result { let client = reqwest::blocking::Client::builder() .connect_timeout(CONNECT_TIMEOUT) @@ -327,10 +335,59 @@ fn fetch_latest_version() -> Result { .build() .context("Failed to create HTTP client")?; - let servers = get_update_servers(); let mut last_err = None; - let prior_notification = load_cache().and_then(|c| c.last_notification); + // A configured source answers on its own. Nothing falls back to the ladder + // after it: a user who said "ask npm" and got an error wants to hear that, + // not a version from somewhere they did not name. + if let Some(source) = crate::release_source::resolve() { + let (url, headers) = source.request()?; + let mut request = client.get(&url); + for (name, value) in &headers { + request = request.header(*name, value); + } + let response = request + .send() + .with_context(|| format!("{} check failed ({url})", source.label()))?; + if !response.status().is_success() { + bail!( + "{} check failed: HTTP {} from {url}", + source.label(), + response.status() + ); + } + let body = response.text().context("update source returned no body")?; + let probe = source.parse(&body)?; + parse_version(&probe.latest_version).with_context(|| { + format!( + "{} returned an invalid version: {}", + source.label(), + probe.latest_version + ) + })?; + // Same discipline as the fallback ladder below: take the lock, then + // re-read the notice state. Reading it before the request would let a + // notice recorded while the request was in flight be overwritten with a + // minutes-old value, and telling the user twice about one release is the + // exact thing the throttle exists to prevent. Both notice fields carry + // forward — dropping `last_notified_version` reset the version-keyed + // throttle on every refresh. + let _guard = lock_cache(); + let prior = load_cache(); + let cache = UpdateCache { + last_check: now_rfc3339(), + latest_version: probe.latest_version, + release_url: probe.release_url, + 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); + } + + let servers = crate::release_source::release_info_servers(); for url in &servers { match client.get(url).send() { Ok(resp) if resp.status().is_success() => match resp.json::() { @@ -347,16 +404,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 +492,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); + } } } @@ -713,7 +786,10 @@ pub fn perform_self_update(output: UpdateOutput) -> Result<()> { .build()?; let mut release_info = None; let mut last_err = None; - let servers = get_update_servers(); + // The ARTIFACT ladder, deliberately not the check source: this is where + // the signed `.update.json` manifest lives, and no configured check source + // may redirect it. See `release_source`'s module docs. + let servers = crate::release_source::release_info_servers(); for url in &servers { match client.get(url).send() { @@ -1553,6 +1629,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..5f1d6e11fe 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), @@ -94,6 +104,17 @@ pub(crate) struct UpdateConfig { /// What Enter means at the `prompt` mode question. Default false. #[serde(skip_serializing_if = "Option::is_none")] pub(crate) prompt_default: Option, + /// Which document to read to learn the latest version: + /// `gh-releases`, `npm`, `gh-registry` or `custom`. Unset walks the + /// historical ladder. See `release_source`. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) source: Option, + /// Package name for the npm-shaped sources. Defaults to Perry's own. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) package: Option, + /// Registry base URL for the npm-shaped sources. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) registry: Option, /// Keys this build does not know about. /// /// Without this, a `[update]` key written by a NEWER Perry — or by hand, @@ -120,10 +141,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 +259,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 +315,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 +339,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 +853,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 +877,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"