Wire the prompt and auto update modes, behind three refusals

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 <off|notify|prompt|auto>`** 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".
+
+<details>
+<summary><b>Why the channel detection fails open</b></summary>
+
+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.
+</details>
+
+<details>
+<summary><b>Prompting needs stdin, not just stderr</b></summary>
+
+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.
+</details>
+
+<details>
+<summary><b>Tests</b></summary>
+
+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.
+</details>

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.
+
+<details>
+<summary><b>Tests</b></summary>
+
+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.
+</details>

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(),

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;

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<PerryConfig, String> {
     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"),
+        }
+    }
+}