fix(server): honor the provider update opt-out when settings cannot be read - #8370
fix(server): honor the provider update opt-out when settings cannot be read#8370shuveb wants to merge 1 commit into
Conversation
…e read The `enableProviderUpdateChecks` guard in `ModelManifest.refresh` skips the opt-out when settings are unreadable, so the manifest fetch goes out for a user who turned the setting off. Invert the guard so an unreadable setting resolves to not making the request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a small, self-contained server bug fix that changes only the unreadable-settings branch of manifest refresh, preventing an unintended remote request while preserving cached or bundled data. A focused live regression test covers the new behavior, with no schema, API, security, billing, or infrastructure impact. You can add or adjust custom eligibility rules. Learn more. |
Problem
enableProviderUpdateChecksis the switch that stops the server phoning home. InModelManifest.refreshthe guard reads:The
catchCauseis there so unreadable settings don't fail a refresh, andsettings !== null &&reads naturally as a null-guard. But the condition is what gates the early return, so anulldoesn't just skip the guard — it skips the opt-out. TheGETtoraw.githubusercontent.comthen goes out for a user who explicitly turned the setting off.This is reachable, not theoretical:
getSettingsis typedEffect.Effect<ServerSettings, ServerSettingsError>, and the live implementation reads through a disk-backedCacheand runsmaterializeProviderEnvironmentSecrets, which has its own error channel. A locked keychain or a corruptsettings.jsonis enough to land on thenullbranch.Fix
One line — an unreadable setting now resolves to not making the request:
Nothing else changes. The preference order (remote → disk cache → bundle), the TTL and retry window, the timeout, and the background fiber are all untouched, and a failed refresh still never fails a provider check.
Test
Adds
does not fetch when the opt-out setting cannot be read, mirroring the existingdoes not fetch when provider update checks are disabledcase but with agetSettingsthat fails with aServerSettingsError(operation: "read-secret"). It asserts the bundled manifest is returned and that the stubbed HTTP client was called zero times.Verified both ways: the new test fails on
main(the refresh fetches and returns the remote manifest) and passes with the guard inverted. The other seven cases in the file are unaffected.No UI change, so no screenshots.
🤖 Generated with Claude Code
Note
Low Risk
Single guard inversion in manifest refresh with a focused regression test; reduces unintended network calls when settings are unreadable.
Overview
Model manifest refresh no longer phones home when
getSettingsfails. Previously,catchCauseturned read errors intonull, and the guard only skipped the fetch when settings existed and opted out—so unreadable settings (corrupt file, keychain errors) still triggered aGETto GitHub.The early-return condition is now
settings === null || !settings.enableProviderUpdateChecks, treating “can’t read the don’t-phone-home switch” the same as opting out. TTL, disk cache, bundled fallback, and non-failing refresh behavior are unchanged.A live test
does not fetch when the opt-out setting cannot be readstubsgetSettingswithServerSettingsErrorand asserts zero HTTP calls and the bundled manifest.Reviewed by Cursor Bugbot for commit 09ccfd3. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Skip remote provider manifest fetch when settings are unreadable
ModelManifestmakefactory to return early when settings arenull(unreadable) orenableProviderUpdateChecksisfalse, instead of only skipping when the opt-out is explicitlyfalseunreadableSettingsLayertest utility and a live test assertingrefresh()returns the bundled manifest with zero network requests when settings cannot be readrefresh()no longer attempts a remote fetch whenServerSettingsService.getSettingsfails; it returns the current bundled or cached manifest insteadMacroscope summarized 09ccfd3.