Fix five review findings in the update surface that merged before their fixes - #7787
Fix five review findings in the update surface that merged before their fixes#7787jdalton wants to merge 1 commit into
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe update surface now defers configuration warnings, uses cached status after check timeouts, throttles notifications by release version, omits empty release URLs, and protects cache mutations with unique temporary files and optional locks. ChangesUpdate surface
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant main.rs
participant update_checker
participant UpdatePolicy
main.rs->>update_checker: Load cached update status
main.rs->>UpdatePolicy: Evaluate notification with timestamp and version
UpdatePolicy-->>main.rs: Return notification decision
main.rs->>update_checker: Record notified release version
Possibly related PRs
Suggested reviewers: 🚥 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry/src/main.rs (1)
566-582: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake the notification claim atomic.
Two processes can load the same old cache state, both pass
should_notify, and both print the same notice.record_notificationlocks only after the external effect has occurred.Add a locked check-and-record operation. Re-read the cache while holding that lock, evaluate the throttle there, and retain the lock until the notice state is recorded.
🤖 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 566 - 582, The update notification flow around update_policy::should_notify must atomically claim the notification before printing it. Add and use a locked check-and-record operation in update_checker that rereads the cache, reevaluates the throttle while holding the lock, records the latest notification state, and keeps the lock through the claim; only call update_checker::print_update_notice when that operation succeeds, removing the separate unlocked record_notification call.
🤖 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-55: Rename the changelog fragment currently using the REVIEW-
prefix to the required PR-key format, preserving its existing contents and slug
as changelog.d/7787-update-surface-followups.md.
---
Outside diff comments:
In `@crates/perry/src/main.rs`:
- Around line 566-582: The update notification flow around
update_policy::should_notify must atomically claim the notification before
printing it. Add and use a locked check-and-record operation in update_checker
that rereads the cache, reevaluates the throttle while holding the lock, records
the latest notification state, and keeps the lock through the claim; only call
update_checker::print_update_notice when that operation succeeds, removing the
separate unlocked record_notification call.
🪄 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: 589cc2c3-cccf-4c9c-aa92-1e6cc22a8672
📒 Files selected for processing (4)
changelog.d/REVIEW-update-surface-followups.mdcrates/perry/src/main.rscrates/perry/src/update_checker.rscrates/perry/src/update_policy.rs
prompt and auto now reach the existing signed self-updater, but never on a package-managed install: Homebrew, npm, apt and winget each track what they installed, and overwriting the binary underneath leaves that record lying, so the owner is detected and its own upgrade command named instead. npm says more, because Perry ships as a wrapper plus a per-platform binary package, so a replacement also desyncs the wrapper that launched it. Neither mode offers anything after a command that failed -- the user is reading an error, and an unattended install would bury it -- and an unwritable install directory is reported before anything is downloaded rather than failing halfway through a rename. Perry never escalates on its own. Channel detection fails open: every rule answers 'definitely managed?', so an unrecognized layout is treated as ours to replace. Guessing the other way would refuse to update a tarball install, the majority case and the one with no alternative path. Paths are canonicalized first because Homebrew's bin entry is a symlink into the Cellar, and apt requires both a dpkg file list and a dpkg-owned path because dpkg does not own /usr/local. perry update --mode saves the setting through the shared loader, and doctor reports the effective mode plus the owning package manager. Carries PerryTS#7787's review fixes as its base; those collapse out when it merges.
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.
c825e35 to
a16f33f
Compare
|
Pushed as a single commit rebased on current What changed, and why each one matteredThe config file could be erased while you were fixing a typo. The teardown cache write could hang someone's terminal. The comment above A slow check was being read as good news. The startup check gets 100 ms to answer. When it did not, the notice path treated that silence as "no update available" and printed nothing — discarding a notice a previous run had already earned and stored. A timeout says the network was slow, not that your version is current, so it now falls back to the cached status. |
✅ Action performedComments resolved. Approval is disabled; enable |
…wn, or hiding a notice The [update] table in ~/.perry/config.toml was silently erased on every save because it was not a field on the struct the saver serializes. The teardown cache write took a blocking lock in a path documented to proceed unlocked, so one perry could hang another's terminal. A background check that had not answered within its 100 ms budget was read as "no update", throwing away a notice the previous run had already earned.
a16f33f to
f54773b
Compare
|
The latest review round raised nothing against this branch, and the two findings it raised against the branches above it do not apply here: this layer has no config writer and no teardown action, so there is no install for a notice throttle to hold back and no writer to erase a config. The one change carried in is the changelog fragment — the suite-count line is gone, because each fragment in the stack recorded the total at the moment its own slice was written and the assembled release notes reported five different figures for one release. Force-pushed as one commit on current |
✅ 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
Five defects in the update surface, all found in review of #7749 — which merged before the fixes were pushed, so all five are live on
maintoday.One commit, based on current
main.The config warning escaped the rules meant to silence it
The "unrecognized
[update] mode" line was printed insideUpdatePolicy::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 swallowed the next release
The documented contract is that
notify_interval_hoursthrottles repeats of the same update. Keyed only on a timestamp, it also suppressed a different version arriving inside the window.The consequence is the opposite of the intent: 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 i64goes 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. Now unsigned.Two
perryprocesses could corrupt the cacheEvery write used one shared
*.json.tmp. Two writers each wrote it and each renamed it, so the loser's rename landed a file the winner was still writing into.Each write now builds its own temporary name from the process id and a per-process counter.
A refresh could erase a notice recorded while its request was in flight
fetch_latest_versionread the notice state before issuing its request and wrote it back afterwards, overwriting anything recorded in between — so the user would be told about the same release twice.The read-modify-write pairs are now serialized by a lock file next to the cache, and the refresh re-reads inside that lock immediately before replacing. If the lock cannot be taken the caller proceeds unlocked: losing a cache update is better than refusing to update a cache, since the next run simply re-checks.
Tests
Two new contract tests, both sabotage-verified — reverting either fix turns its test red:
The existing interval tests are unchanged in intent. They now call a helper that holds the announced version constant, so they still exercise only the interval arithmetic — which is what they were written for.
cargo test -p perry: 904 passed, 0 failed.Credit
All five were raised by CodeRabbit on #7749. I had replied there saying they were fixed; that was true of the branch and not of the merge, which is what this pull request corrects.
No version bump — the maintainer bumps at merge, per the external-contributor flow.
Summary by CodeRabbit
Bug Fixes
Tests