Skip to content

fix(server): honor the provider update opt-out when settings cannot be read - #8370

Open
shuveb wants to merge 1 commit into
pingdotgg:mainfrom
shuveb:fix/model-manifest-opt-out-fails-open
Open

fix(server): honor the provider update opt-out when settings cannot be read#8370
shuveb wants to merge 1 commit into
pingdotgg:mainfrom
shuveb:fix/model-manifest-opt-out-fails-open

Conversation

@shuveb

@shuveb shuveb commented Aug 27, 2026

Copy link
Copy Markdown

Problem

enableProviderUpdateChecks is the switch that stops the server phoning home. In ModelManifest.refresh the guard reads:

const settings = yield* settingsService.getSettings.pipe(
  Effect.catchCause(() => Effect.succeed(null)),
);
if (settings !== null && !settings.enableProviderUpdateChecks) return manifest;

The catchCause is there so unreadable settings don't fail a refresh, and settings !== null && reads naturally as a null-guard. But the condition is what gates the early return, so a null doesn't just skip the guard — it skips the opt-out. The GET to raw.githubusercontent.com then goes out for a user who explicitly turned the setting off.

This is reachable, not theoretical: getSettings is typed Effect.Effect<ServerSettings, ServerSettingsError>, and the live implementation reads through a disk-backed Cache and runs materializeProviderEnvironmentSecrets, which has its own error channel. A locked keychain or a corrupt settings.json is enough to land on the null branch.

Fix

One line — an unreadable setting now resolves to not making the request:

if (settings === null || !settings.enableProviderUpdateChecks) return manifest;

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 existing does not fetch when provider update checks are disabled case but with a getSettings that fails with a ServerSettingsError (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 getSettings fails. Previously, catchCause turned read errors into null, and the guard only skipped the fetch when settings existed and opted out—so unreadable settings (corrupt file, keychain errors) still triggered a GET to 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 read stubs getSettings with ServerSettingsError and 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

  • Changes the fetch gate in ModelManifest make factory to return early when settings are null (unreadable) or enableProviderUpdateChecks is false, instead of only skipping when the opt-out is explicitly false
  • Adds an unreadableSettingsLayer test utility and a live test asserting refresh() returns the bundled manifest with zero network requests when settings cannot be read
  • Behavioral Change: refresh() no longer attempts a remote fetch when ServerSettingsService.getSettings fails; it returns the current bundled or cached manifest instead

Macroscope summarized 09ccfd3.

…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>
@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7a6964d0-9168-4b5a-a384-80f7833a56f8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Aug 27, 2026
@macroscopeapp

macroscopeapp Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 09ccfd3

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant