Wire the prompt and auto update modes, behind three refusals - #7784
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe update system adds configurable update modes, installation-channel detection, prompted and automatic installation handling, version-aware notification throttling, synchronized cache updates, safer configuration persistence, and expanded ChangesUpdate policy flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PerryCLI
participant UpdatePolicy
participant InstallChannel
participant UpdateCache
User->>PerryCLI: Run update check or set --mode
PerryCLI->>UpdatePolicy: Resolve configured mode
UpdatePolicy->>InstallChannel: Detect installation channel
PerryCLI->>UpdateCache: Read update and notification state
UpdateCache-->>PerryCLI: Return cached or refreshed status
PerryCLI->>UpdatePolicy: Evaluate teardown action
UpdatePolicy-->>PerryCLI: Return notice, prompt, install, or silence
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
crates/perry/src/main.rs (1)
577-590: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRun the update teardown after the report and telemetry flushes.
Before this change, this block only printed a notice. It can now block on an interactive
dialoguerprompt (Ask) or download and install a new binary in place (Install).compat_reports::flush()at Line 595 can itself prompt the user, andtelemetry::flush()at Line 598 still has work to do.Two consequences follow. First, the user can meet the update prompt and then the compatibility-report prompt back to back, with an unrelated download between them. Second,
compat_reports::flush()andtelemetry::flush()now run afterperform_self_updatehas replaced this executable andlibperry_runtime.a/libperry_stdlib.aon disk.Move the update teardown below both flushes so all reporting for this run completes before anything on disk is replaced.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/main.rs` around lines 577 - 590, Move the update_policy::run_teardown_action call out of its current block and place it after both compat_reports::flush() and telemetry::flush() complete. Preserve its existing arguments and ensure reporting and telemetry finish before any interactive update prompt or self-update replaces files on disk.crates/perry/src/commands/update.rs (1)
141-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
Unknownguard.
UpdateMode::parsematches onlyoff,notify,prompt, andauto, and returnsNonefor everything else. It never returnsUpdateMode::Unknown. The second check is therefore dead, and it duplicates the error string.♻️ Proposed cleanup
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."); - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/update.rs` around lines 141 - 146, Remove the redundant UpdateMode::Unknown conditional after the UpdateMode::parse call in the update-mode handling flow. Keep the existing anyhow error from the let-else branch for invalid raw values and leave successful parsed modes unchanged.crates/perry/src/update_policy.rs (1)
425-433: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCompute the channel and the writability probe only for the modes that need them.
decide_teardownreturns early forUpdateMode::Notify, but this call site evaluates everyTeardownEnvfield first.install_dir_is_writablecreates and deletes.perry-write-probein the install directory.notifyis the default mode, so every run that has an available update writes a probe file into the install directory — for example/usr/local/bin— and callscanonicalizeplus a dpkg stat, for a decision that ignores all of it. If the process is killed between the create and the remove, the probe file stays behind.Gate the two filesystem probes on the active modes.
♻️ Proposed refactor
- 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(), - }, - ); + // `Prompt` and `Auto` are the only modes that read these, and both probes + // touch the filesystem — `install_dir_is_writable` writes into the install + // directory. `notify` is the default, so it must not pay for them. + let needs_install_facts = matches!(policy.mode, UpdateMode::Prompt | UpdateMode::Auto); + let action = decide_teardown( + policy.mode, + TeardownEnv { + command_succeeded, + stdin_is_terminal: std::io::stdin().is_terminal(), + channel: if needs_install_facts { + crate::install_channel::detect() + } else { + crate::install_channel::InstallChannel::SelfManaged + }, + install_dir_writable: !needs_install_facts + || crate::install_channel::install_dir_is_writable(), + }, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/update_policy.rs` around lines 425 - 433, Update the teardown decision setup around decide_teardown so channel detection and install_dir_is_writable are evaluated only for modes that use them, while preserving command_succeeded and stdin_is_terminal for all modes. Avoid invoking either filesystem probe when policy.mode is UpdateMode::Notify, and provide the existing expected values for modes that require those fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.d/A2-update-prompt-auto-and-channels.md`:
- Around line 1-105: Rename the changelog fragment from its A2-prefixed name to
the PR-keyed filename 7784-update-prompt-auto-and-channels.md, preserving the
existing body unchanged and without adding a version header.
In `@crates/perry/src/commands/doctor.rs`:
- Around line 289-302: Update the doctor context block around
UpdatePolicy::resolve so it reports the persisted/configured update mode rather
than the run-suppressed mode. Name any active suppression separately when stderr
is non-interactive, machine-readable output is requested, or CI is detected,
while preserving the existing channel and upgrade-command details.
In `@crates/perry/src/update_checker.rs`:
- Around line 129-137: Update lock_cache() to call fslock::LockFile::try_lock()
instead of the blocking lock() method, preserving the existing Option-based
fallback when acquisition fails so teardown can proceed without waiting.
- Line 370: Remove the unused prior_notification binding and its load_cache call
from the update-check flow, leaving the later locked cache read into prior and
its existing uses unchanged.
---
Nitpick comments:
In `@crates/perry/src/commands/update.rs`:
- Around line 141-146: Remove the redundant UpdateMode::Unknown conditional
after the UpdateMode::parse call in the update-mode handling flow. Keep the
existing anyhow error from the let-else branch for invalid raw values and leave
successful parsed modes unchanged.
In `@crates/perry/src/main.rs`:
- Around line 577-590: Move the update_policy::run_teardown_action call out of
its current block and place it after both compat_reports::flush() and
telemetry::flush() complete. Preserve its existing arguments and ensure
reporting and telemetry finish before any interactive update prompt or
self-update replaces files on disk.
In `@crates/perry/src/update_policy.rs`:
- Around line 425-433: Update the teardown decision setup around decide_teardown
so channel detection and install_dir_is_writable are evaluated only for modes
that use them, while preserving command_succeeded and stdin_is_terminal for all
modes. Avoid invoking either filesystem probe when policy.mode is
UpdateMode::Notify, and provide the existing expected values for modes that
require those fields.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 26eca302-824e-4d08-a783-253a2e09da1a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CLAUDE.mdCargo.tomlchangelog.d/7749-update-config-surface.mdchangelog.d/A2-update-prompt-auto-and-channels.mdcrates/perry/src/commands/doctor.rscrates/perry/src/commands/publish/saved_config.rscrates/perry/src/commands/update.rscrates/perry/src/install_channel.rscrates/perry/src/main.rscrates/perry/src/update_checker.rscrates/perry/src/update_policy.rs
b7e6988 to
36a2ba5
Compare
The check walked one fixed ladder and read a GitHub-releases-shaped document from whichever URL answered. That is wrong as soon as people install differently: an npm user's latest is the registry's latest dist-tag, and announcing a GitHub release their package manager cannot install yet is worse than saying nothing. [update] source now selects gh-releases, npm, gh-registry or custom; unset keeps today's ladder, and an npm-managed install defaults to asking npm. Checking is kept separate from downloading. A source returns a version, a link, a publish time and a headline, and never says where the binary comes from -- artifacts and their signed manifest always resolve from the release infrastructure. The manifest is what makes a self-update trustworthy and a source is a URL a user can point anywhere, so letting it redirect the download would turn a config setting into arbitrary code execution. A test fails if a source ever reaches the artifact ladder. 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. Four sources that pass their own tests while the old ladder still runs underneath is the failure this avoids. The npm shapes request the abbreviated packument, which is cheaper and dodges GitHub's unauthenticated rate limit. The public registry is asked with no credentials and a test asserts it; GitHub Packages requires a token and fails naming the fix rather than reading a 404 as up to date. A configured source does not fall back to the ladder on error. Carries PerryTS#7787 and PerryTS#7784 as its base; those collapse out as they merge.
…nd docs auto now waits min_age_hours (default 24) before installing a release; notify and prompt are unaffected because they tell a human who can decide. A release published by mistake, pulled soon after, or published by someone who should not have been able to is most dangerous in its first hours, and waiting means this machine is not the one that finds out. An UNKNOWN publish date counts as too fresh rather than old enough. The abbreviated npm packument carries no dates, so the other choice would switch the cooldown off for exactly the users on the cheapest source -- present in the config, absent in effect. min_age_hours = 0 disables it deliberately. The prompt gains a third answer. 'No' and 'never tell me about this one' are different intentions, and with two answers someone who dislikes one release has to switch the mode off, which then hides the release that fixes it. The cache records its schema and a foreign value -- including an absent one -- is discarded rather than migrated. Keeping every field optional forever so older shapes load buys one saved request for a CACHE in exchange for Option fields that only describe versions nobody runs. Check sources lose their aliases for the same reason. Docs: a new cli/updates.md covering the default, every [update] key, the modes and their refusals, the four sources, the cooldown, skipping, what a check transmits and where state lives; commands.md rewritten around --mode; installation.md gains the per-package-manager upgrade table. Carries PerryTS#7787, PerryTS#7784 and PerryTS#7785 as its base; those collapse out as they merge.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog.d/REVIEW-update-surface-followups.md`:
- Around line 1-4: Rename the changelog file from
REVIEW-update-surface-followups.md to a PR-keyed <current PR id>-<slug>.md
filename, or remove this entry if it belongs to PR `#7749`.
- Line 54: Clarify the scope of the “cargo test -p perry” result in
REVIEW-update-surface-followups.md: either update the documented 904 count to
the command’s current full-suite result, or explicitly state that 904 passed
tests cover only these review follow-ups.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e2a02abd-eee1-480b-ba8a-1b209916abe5
📒 Files selected for processing (1)
changelog.d/REVIEW-update-surface-followups.md
36a2ba5 to
3091293
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry/src/commands/publish/saved_config.rs`:
- Around line 132-138: Update load_config_checked to return
PerryConfig::default() only for fs::read_to_string errors with
ErrorKind::NotFound, while propagating all other read errors and TOML parse
failures. Change load_config’s fallback behavior so these errors are not
converted back into defaults. Update the telemetry, login, setup, and publish
read-modify-write paths to use checked loading, and revise the error at the
existing lines 149-150 to describe files that could not be read or parsed.
In `@crates/perry/src/main.rs`:
- Around line 569-589: The update flow around update_policy::run_teardown_action
must resolve the configured teardown action before applying notify_interval
throttling. Ensure eligible self-managed auto updates always execute
TeardownAction::Install, while should_notify remains applied only to notice or
prompt actions that produce user-facing notifications.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: efbb96f7-91b5-40a6-b84e-d34e1f7a47c0
📒 Files selected for processing (9)
changelog.d/7784-update-prompt-auto-and-channels.mdchangelog.d/7787-update-surface-followups.mdcrates/perry/src/commands/doctor.rscrates/perry/src/commands/publish/mod.rscrates/perry/src/commands/publish/saved_config.rscrates/perry/src/commands/update.rscrates/perry/src/main.rscrates/perry/src/update_checker.rscrates/perry/src/update_policy.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/perry/src/commands/update.rs
- crates/perry/src/commands/doctor.rs
- crates/perry/src/update_checker.rs
- crates/perry/src/update_policy.rs
3091293 to
ad55186
Compare
|
Pushed as a single commit rebased on current What changed, and why each one mattered
Two smaller ones carried up from the base branch: the teardown cache lock is |
✅ Action performedComments resolved. Approval is disabled; enable |
ad55186 to
f0743e0
Compare
…ing package manager Adds the two remaining rungs of the update ladder. `prompt` asks once, after a command that succeeded, on a real terminal only. `auto` installs at the end of the run, but only for a binary Perry itself installed: a Homebrew, npm, apt or winget copy prints that tool's own upgrade command instead of overwriting a file the package manager owns. `perry update --mode <mode>` writes the setting, and `perry doctor` reports it.
f0743e0 to
16cd868
Compare
|
Both findings from the latest review are real, and both are fixed. Force-pushed as one commit on current Auto mode was being silently blocked by the notice throttleThis was the more serious of the two, and it is a good catch. The throttle is now passed down into if notice_throttled && throttle_applies(&action) {
return;
}
An unreadable config still resolved to defaultsCorrect, and this reopened the exact data-loss case the function was added to close. Worth noting how the test for this had to change. My first version just asserted that the write failed — and that test passed even with the fix reverted, because a file at mode `configured_mode` was hiding config typosAlso right, and the reason is a nice one. The changelog fragment's "reports the effective mode" wording was wrong for the same reason and now says "configured mode". |
✅ Action performedComments resolved. Approval is disabled; enable |
Covers the 32 PRs admin-merged in one pass (audited in principle at the maintainer's direction): PerryTS#7768 PerryTS#7772 PerryTS#7779 PerryTS#7784 PerryTS#7785 PerryTS#7786 PerryTS#7788 PerryTS#7789 PerryTS#7797 PerryTS#7798 PerryTS#7801 PerryTS#7802 PerryTS#7804 PerryTS#7805 PerryTS#7806 PerryTS#7807 PerryTS#7808 PerryTS#7810 PerryTS#7811 PerryTS#7815 PerryTS#7816 PerryTS#7818 PerryTS#7819 PerryTS#7820 PerryTS#7821 PerryTS#7822 PerryTS#7823 PerryTS#7824 PerryTS#7825 PerryTS#7826 PerryTS#7827 PerryTS#7828. (PerryTS#7787 closed as already-landed via the PerryTS#7786 stack.) Per-change history lives in each PR's changelog.d fragment as usual. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
2 participants
Stacked on #7749. Until that merges this branch's diff includes its commit; I will rebase once it lands so this collapses to just the new work.
The
promptandautomodes now do something — and refuse to do the wrong thing#7749 made the modes configurable but deliberately left them inert, because replacing the binary somebody is currently running deserved its own review. This is that review.
Refusal 1: a package-managed install is never replaced in place
perry updateoverwrites the running executable. That is correct for a tarball orinstall.shinstall, 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. The nextbrew upgradeeither reinstalls over the top or reports a version that is not what is on disk.So
promptandautodetect the owner and name that owner's command instead:brew upgrade perryts/perry/perrynpm install -g @perryts/perry@latestsudo apt update && sudo apt install --only-upgrade perrywinget upgrade PerryTS.Perrynpm 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.
Refusal 2: 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.
Refusal 3: an unwritable install directory is reported, not attempted
install.shtargets/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 namingsudo perry updaterather than a download that fails at the final rename. Perry never escalates on its own.Also
perry update --mode <off|notify|prompt|auto>saves the setting and exits, so the one thing people are most likely to change does not mean hand-editing TOML.perry doctornow reports the effective mode and the owning package manager, which are the two questions behind "why did it not update".Why the channel detection deliberately fails open
Every rule answers "is this definitely managed?", never "is this definitely unmanaged?" — an unrecognised layout resolves to self-managed.
That asymmetry is the point. Guessing "managed" wrongly would refuse to self-update a plain tarball install, which is 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 the user can recover from by running that manager.
Two details worth knowing:
perryin/usr/local/binis a symlink into the Cellar, so classifying the link rather than its target would miss every Homebrew install there is./usr/local, which isinstall.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.debinstalled elsewhere. It is a file-existence check rather than adpkg -Ssubprocess, since this runs on the update path of every command.Prompting needs stdin, not just stderr
The mode gate from #7749 already requires stderr to be a terminal, but 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 treat whatever the pipe happened to contain as consent.
promptdegrades to a plain notice when stdin is not a terminal.autoasks nothing, so it does not need stdin — but it still requires a successful command, 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, which is why it is a separate function from carrying it out: every refusal is then asserted directly, rather than living inside an
ifin the middle of a teardown path where nothing can reach it.promptdegrades without stdin whileautodoes not need it.The channel table covers Homebrew under all three prefixes, npm for global / nvm / 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 instead of using
Path::components, so the winget cases actually run on every host rather than only on Windows — otherwise that half of the table would be untested everywhere CI runs it.Verified end to end. Writing
modeinto a real config file that already contained alicense_keyand an unknown[update] future_keyleft both intact:cargo test -p perry: 914 passed, 0 failed.scripts/check_file_size.shpasses.No version bump — the maintainer bumps at merge, per the external-contributor flow.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation