Make the update check source a choice of four - #7785
Conversation
|
Warning Review limit reached
Next review available in: 12 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesThe update system now supports configurable update modes, installation-channel detection, selectable release metadata sources, version-aware notification throttling, locked cache persistence, and enhanced command output. Update system
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant main
participant UpdatePolicy
participant update_checker
participant CheckSource
participant UpdateCache
main->>UpdatePolicy: resolve policy and notification settings
main->>update_checker: request cached or background update
update_checker->>CheckSource: build request and parse release metadata
CheckSource-->>update_checker: return VersionProbe
update_checker->>UpdateCache: persist update status and notification state
update_checker-->>main: return update status
main->>UpdatePolicy: decide notification or teardown action
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 (2)
crates/perry/src/update_policy.rs (1)
249-251: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider passing the loaded
UpdateConfigto the source resolver.
UpdatePolicy::resolve_withreadsload_config()here, andrelease_source::resolvereads it again on the same run.doctoradds a third read. Each read parses~/.perry/config.tomlin full. One read, threaded through, removes the duplicated I/O and removes the chance that the two readers see different file contents.🤖 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 249 - 251, Load the update configuration once in UpdatePolicy::resolve_with, then pass that UpdateConfig into release_source::resolve instead of having the resolver call load_config() independently. Thread the same loaded value through the doctor path as well, preserving existing defaults while eliminating repeated config reads.crates/perry/src/commands/update.rs (1)
18-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider declaring the flag conflicts, and folding the duplicated error string.
Two small points on the new
--modepath:
perry update --mode auto --forceaccepts both flags and silently ignores--force, because line 36 returns before the check.#[arg(long, conflicts_with_all = ["force", "check_only"])]turns that into a usage error.UpdateMode::parsereturnsNonefor every unrecognized spelling and never returnsUpdateMode::Unknown, so the guard at lines 144-146 is unreachable and repeats the same message.♻️ Proposed change
- #[arg(long, value_name = "off|notify|prompt|auto")] + #[arg( + long, + value_name = "off|notify|prompt|auto", + conflicts_with_all = ["force", "check_only"] + )] pub mode: Option<String>,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."); - }Also applies to: 140-146
🤖 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 18 - 25, Update the `mode` argument declaration to conflict with both `force` and `check_only`, so incompatible flags are rejected before update handling returns early. In the `UpdateMode::parse` handling, remove the unreachable `UpdateMode::Unknown` branch and consolidate the duplicated invalid-mode error message into the single applicable validation path.
🤖 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/A3-update-check-sources.md`:
- Around line 1-92: Rename changelog.d/A3-update-check-sources.md to
changelog.d/<PR-number>-update-check-sources.md and
changelog.d/A2-update-prompt-auto-and-channels.md to
changelog.d/<PR-number>-update-prompt-auto-and-channels.md, using this pull
request’s actual number. Keep both file bodies unchanged and preserve the
existing PR-keyed changeset format.
In `@crates/perry/src/commands/doctor.rs`:
- Around line 292-302: Update the diagnostic context construction around
UpdatePolicy::resolve and the mode.label() calls so doctor reports the
configured update mode independently of output-format, CI, and terminal-based
suppression. Do not use the resolved suppression mode for this diagnostic; if
suppression details are retained, report them separately from the configured
mode while preserving the existing channel and upgrade-command information.
In `@crates/perry/src/release_source.rs`:
- Around line 171-200: Validate the URL in release_source request construction
before creating any headers, covering GhReleases, Custom, Npm, and GhRegistry
variants. Require an absolute HTTPS URL with no embedded credentials, reject
invalid URLs, and only then attach the GH_TOKEN Authorization header for
GhRegistry.
In `@crates/perry/src/update_checker.rs`:
- Around line 333-370: Update the configured-source branch’s UpdateCache
initializer to include all five fields, reusing the existing cached values for
both last_notification and last_notified_version. Keep the newly fetched
last_check, latest_version, and release_url values unchanged, matching the
fallback branch’s cache construction.
---
Nitpick comments:
In `@crates/perry/src/commands/update.rs`:
- Around line 18-25: Update the `mode` argument declaration to conflict with
both `force` and `check_only`, so incompatible flags are rejected before update
handling returns early. In the `UpdateMode::parse` handling, remove the
unreachable `UpdateMode::Unknown` branch and consolidate the duplicated
invalid-mode error message into the single applicable validation path.
In `@crates/perry/src/update_policy.rs`:
- Around line 249-251: Load the update configuration once in
UpdatePolicy::resolve_with, then pass that UpdateConfig into
release_source::resolve instead of having the resolver call load_config()
independently. Thread the same loaded value through the doctor path as well,
preserving existing defaults while eliminating repeated config reads.
🪄 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: 03b7417b-f3a3-4367-865d-fbd70c0a94d1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
CLAUDE.mdCargo.tomlchangelog.d/7749-update-config-surface.mdchangelog.d/A2-update-prompt-auto-and-channels.mdchangelog.d/A3-update-check-sources.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/release_source.rscrates/perry/src/update_checker.rscrates/perry/src/update_policy.rs
bbc8c0d to
2c376b9
Compare
…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: 1
🤖 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 fragment from
REVIEW-update-surface-followups.md to a filename using the current PR number and
the existing update-surface-followups slug, preserving the ### Fixed section
content unchanged.
🪄 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: 105412da-7c31-4360-9626-deeeb3c4ec6a
📒 Files selected for processing (1)
changelog.d/REVIEW-update-surface-followups.md
2c376b9 to
5f2c609
Compare
|
Pushed as a single commit rebased on current A token could have been sent in clear textThe download path already required HTTPS. The check path did not, and for the
The new check path was quietly resetting the notice throttleThe configured-source refresh had its own copy of the cache rebuild, and that copy did two things the older fallback path had already learned not to do. It read the notice state before making the request, so a notice recorded while the request was in flight got overwritten with a minutes-old value — which means you would be told twice about the same release, the exact thing the throttle exists to prevent. And it dropped Both paths now do the same thing: take the lock, re-read, carry both notice fields forward. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
crates/perry/src/release_source.rs (1)
170-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
requestdoc line offrequire_secure.Line 170 reads "The URL to request, and the headers this shape needs." That sentence documents
request, notrequire_secure. It now sits inrequire_secure's doc comment, sorequestat line 205 has no documentation andrequire_secure's doc opens with a description of a different function.♻️ Proposed doc split
- /// The URL to request, and the headers this shape needs. - /// Reject anything that is not an absolute HTTPS URL without credentials. + /// Reject anything that is not an absolute HTTPS URL without credentials. /// /// The artifact path already required this; the CHECK path did not, and it isAnd above
request:+ /// The URL to request, and the headers this shape needs. pub(crate) fn request(&self) -> Result<(String, Vec<(&'static str, String)>)> {🤖 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/release_source.rs` around lines 170 - 177, Move the “URL to request, and the headers this shape needs” documentation from require_secure to the request definition, then keep require_secure’s doc comment focused on rejecting non-absolute HTTPS URLs and its loopback exemption.crates/perry/src/commands/publish/saved_config.rs (1)
369-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree tests restore process-wide environment variables after assertions that can panic. Each test saves a variable, overwrites it, asserts, and restores at the end of the function body.
expect_err,expect, andassert!unwind past the restoration, so a single failing assertion leaks the overwritten value into every later test in the same binary. Theenv_lock()guard serializes access but does not restore state. Use aDropguard for the save-and-restore in each test.
crates/perry/src/commands/publish/saved_config.rs#L369-L411: restoreHOMEfrom aDropguard, and delete the trailing manualmatch savedblock. This site matters most, becauseconfig_path()readsHOMEand the temporary directory is deleted when the test ends.crates/perry/src/release_source.rs#L518-L575: restoreGH_TOKENfrom the same kind of guard instead of the trailingmatch savedblock.crates/perry/src/release_source.rs#L580-L606: restore bothGH_TOKENandGITHUB_TOKENfrom a guard instead of the two trailingif let Some(v)blocks, which also fail to remove a variable that was unset before the test.🤖 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/publish/saved_config.rs` around lines 369 - 411, Replace the manual environment restoration with Drop-based guards in all three tests: in crates/perry/src/commands/publish/saved_config.rs:369-411, guard HOME and remove the trailing saved-value match; in crates/perry/src/release_source.rs:518-575, guard GH_TOKEN and remove its trailing match; in crates/perry/src/release_source.rs:580-606, guard both GH_TOKEN and GITHUB_TOKEN and remove the trailing restoration blocks. Each guard must restore the original value or remove the variable when it was previously unset, including during assertion unwinding.
🤖 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/7784-update-prompt-auto-and-channels.md`:
- Around line 41-43: Update the changelog wording to say `perry doctor` reports
the configured mode instead of the effective mode, matching the
`policy.configured_mode.label()` behavior in the doctor command.
In `@changelog.d/7787-update-surface-followups.md`:
- Line 54: Update the test-total lines in
changelog.d/7787-update-surface-followups.md:54,
changelog.d/7784-update-prompt-auto-and-channels.md:104, and
changelog.d/7785-update-check-sources.md:91 so all use the final PR total of 932
passed, 0 failed, or remove all three lines consistently.
In `@crates/perry/src/commands/publish/saved_config.rs`:
- Around line 132-139: Update load_config_checked to return
PerryConfig::default() only when fs::read_to_string fails with
ErrorKind::NotFound; propagate every other read error as an Err containing its
details. Preserve the existing TOML parsing error handling so update_config_file
cannot overwrite an unreadable existing file with defaults.
In `@crates/perry/src/release_source.rs`:
- Around line 324-347: Update parse_custom so an absent or empty release_url
remains omitted rather than becoming an empty string, using the existing
VersionProbe release_url representation. Ensure print_update_notice and
commands::update::run only render release URL messages when a non-empty URL is
present, while preserving current output for valid URLs.
- Around line 134-152: The release_info_servers function adds configured update
servers without validating their scheme. Apply require_secure to both
PERRY_UPDATE_SERVER and [update].server values before pushing them, preserving
the existing loopback exception and leaving the built-in HUB_URL and GITHUB_URL
entries unchanged.
In `@crates/perry/src/update_policy.rs`:
- Around line 266-280: Update the `configured_mode` field initialization in
`Self` to preserve `UpdateMode::Unknown` by using
`config.mode.unwrap_or(UpdateMode::Notify)` instead of converting `Unknown`
through the current match. Keep the `mode` field resolved via `resolve_mode` and
retain `config_warning` unchanged.
---
Nitpick comments:
In `@crates/perry/src/commands/publish/saved_config.rs`:
- Around line 369-411: Replace the manual environment restoration with
Drop-based guards in all three tests: in
crates/perry/src/commands/publish/saved_config.rs:369-411, guard HOME and remove
the trailing saved-value match; in crates/perry/src/release_source.rs:518-575,
guard GH_TOKEN and remove its trailing match; in
crates/perry/src/release_source.rs:580-606, guard both GH_TOKEN and GITHUB_TOKEN
and remove the trailing restoration blocks. Each guard must restore the original
value or remove the variable when it was previously unset, including during
assertion unwinding.
In `@crates/perry/src/release_source.rs`:
- Around line 170-177: Move the “URL to request, and the headers this shape
needs” documentation from require_secure to the request definition, then keep
require_secure’s doc comment focused on rejecting non-absolute HTTPS URLs and
its loopback exemption.
🪄 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: 64419e79-f2a9-4608-873e-5d403263696e
📒 Files selected for processing (11)
changelog.d/7784-update-prompt-auto-and-channels.mdchangelog.d/7785-update-check-sources.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/release_source.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/doctor.rs
- crates/perry/src/commands/update.rs
- crates/perry/src/main.rs
- crates/perry/src/update_checker.rs
✅ Action performedComments resolved. Approval is disabled; enable |
5f2c609 to
07f0aae
Compare
…ub Packages, or a custom URL The version check and the download used to be one setting, so pointing the check at npm meant pointing the download there too. They are now separate: `source` picks where the version number is read from, while the binary and its signed manifest always come from Perry's release infrastructure, so a check source cannot redirect an install. An npm-installed Perry defaults to asking npm, which is both the cheapest request and the one that matches what the user will run to upgrade.
07f0aae to
d8e9caf
Compare
|
All findings from the latest review are real and fixed. Force-pushed as one commit on current The old fallback ladder took a configured server without checking its schemeThis is the same class of problem I had just fixed on the new check sources, and the reviewer is right that I stopped one function short. The interesting risk here is not eavesdropping. Anyone who can answer a plaintext version check can suppress updates indefinitely by reporting the running version as the latest one, which is a quiet way to hold a machine on a vulnerable build. Both entries now go through a shared An empty release URL printed a dangling sentence
The fragments contradicted each other on test countsA fair point about assembled release notes. Each fragment recorded the suite total at the moment its own slice was written — 904, 914, 925, 929, 930 — so the folded notes reported five different totals for one release, and the numbers appeared to go down across entries that build on each other. The count is a development detail rather than release-note material, so the line is gone from all five fragments. I deliberately left the counts in fragments already on |
✅ 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 #7784, which is stacked on #7749. Until those merge this branch's diff includes their commits; I will rebase as each lands.
Where Perry asks "what is the latest version?" becomes 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
latestdist-tag says, so asking GitHub instead can announce a version their package manager cannot install yet — a notice telling someone to upgrade to something they cannot get.Leaving
sourceunset keeps today's behaviour exactly, 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 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 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
sourceis a URL a user can point anywhere. If it could redirect the download, this setting would be a way to install arbitrary code. 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.Why the old ladder is deleted rather than left underneath
get_update_serversand its private config reader are gone, not kept beside the new code.Leaving them would produce the shape where four sources exist, pass their own unit tests, and are never actually reached, because the call sites still walk the old list. Deleting them makes the compiler prove both sites moved — there is no version of this change that builds while the new abstraction is dead.
Credentials go to exactly one of the four
The npm shapes request the abbreviated packument (
Accept: application/vnd.npm.install-v1+json) — smaller, cacheable, and the document npm itself asks for. It also sidesteps 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
Authorizationheader is sent: a token there would be a leak, not a convenience. GitHub Packages does need one, so that shape readsGH_TOKEN/GITHUB_TOKENand 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:
vprefix is stripped;timemap — so the publish date reads "unknown" rather than being invented, which matters because the release cooldown in the next slice depends on it;version, and one with every optional field;"";/is percent-encoded, or the registry reads the scope as a path segment and answers 404;sourcefalls back to the default instead of failing, because an update check is the wrong place to turn a config typo into a hard error;customwith no URL is a missing key rather than a default;cargo test -p perry: 932 passed, 0 failed.No version bump — the maintainer bumps at merge, per the external-contributor flow.
Summary by CodeRabbit
off,notify,prompt, orautousingperry update --mode.