fix(server): turning off provider update checks no longer hides new models #19343 · pingdotgg/t3code

Added 1 commit25d ago

fix(server): turning off provider update checks no longer freezes the model catalogb6a00d4

MmacroscopeappBotapproved these changes25d ago

Approvability

Verdict: Approved at b6a00d4

Macroscope's review found this PR approvable — This is a focused server bug fix that decouples model-catalog refreshes from CLI version checks without changing defaults or cache/retry behavior. A targeted test verifies that disabled CLI checks still allow the remote catalog to refresh and expose new models.

^{You can add or adjust custom eligibility rules. Learn more.}

CcoderabbitaiBot25d ago

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.config.ts
Review profile: CHILL
Plan: Team
Run ID: ce378511-a72c-4f5e-811b-2b559cf9b667

📥 Commits

Reviewing files that changed from the base of the PR and between 9a01524f3faca628699cdf65b43ad020b40b86d8 and b6a00d42dc89d5cd36d2e915150937e4cb68846a.

📒 Files selected for processing: 2

apps/server/src/provider/ModelManifest.test.ts
apps/server/src/provider/ModelManifest.ts

Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour.

📝 Walkthrough

Provider catalog refreshes now attempt a remote fetch regardless of the provider update-check setting, after freshness and retry checks pass. Updated tests verify that refresh and forceRefresh return the remote manifest when the setting is disabled.

Changes: Provider catalog refresh

Layer / File(s)	Summary
Ungated catalog refresh `apps/server/src/provider/ModelManifest.ts`, `apps/server/src/provider/ModelManifest.test.ts`	Removed the settings dependency and update-check gate from catalog refreshes. Tests now expect `refresh` and `forceRefresh` to return the remote manifest and make two requests when provider update checks are disabled.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to b6a00

Disabling CLI version checks no longer prevents catalog refreshes, and the supplied review context identifies no material regression. No merge-blocking risk is evident.

Pre-merge checks: ^Passed 3 · ^Failed 1

^Failed Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Description check	Warning	The description explains the problem, implementation, scope, and verification results. However, the required explicit maintainer approval is missing; the description states that approval is still need…	Add a link to the triaged issue or discussion with an explicit maintainer approval comment for this behavior change. If the change qualifies as an obvious bug fix without prior approval, explain that exemption instead.

^Passed Passed checks: 3 passed

Check name	Status	Explanation
Linked Issues check	Passed	The change satisfies the coding requirement in `#13244`. `ModelManifest.ts` no longer reads or applies `enableProviderUpdateChecks` before the catalog fetch. The existing freshness and retry checks rema…
Out of Scope Changes check	Passed	The implementation change is limited to decoupling the remote model-catalog fetch from the CLI version-check setting. The test update verifies the behavior required by `#13244`. No unrelated source or t…
Title check	Passed	The title clearly and concisely describes the main change: disabling provider update checks no longer hides new models.

Full details: Description check

Explanation

The description explains the problem, implementation, scope, and verification results. However, the required explicit maintainer approval is missing; the description states that approval is still needed before merge.

Fix all pre-merge checks with AI

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Ggithub-actionsBot25d ago

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider	Metric	Main baseline	This PR	Impact	PR ceiling
Codex	Total thread wire	4.9 KiB	4.9 KiB	0 B (0.0%)	6.8 KiB	✅
Codex	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Codex	Live turn WebSocket wire	1.2 KiB	1.2 KiB	0 B (0.0%)	2.0 KiB	✅
Codex	Live turn WebSocket decoded	20.8 KiB	20.8 KiB	0 B (0.0%)	29.3 KiB	✅
Codex	Live turn messages	1	1	0 (0.0%)	8	✅
Claude	Total thread wire	5.0 KiB	5.0 KiB	−24 B (−0.5%)	6.8 KiB	✅
Claude	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Claude	Live turn WebSocket wire	1.2 KiB	1.2 KiB	−24 B (−2.0%)	2.0 KiB	✅
Claude	Live turn WebSocket decoded	21.2 KiB	21.2 KiB	−41 B (−0.2%)	29.3 KiB	✅
Claude	Live turn messages	2	1	−1 (−50.0%)	8	✅

Baseline: 9a01524 · PR result: b6a00d4 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

Codex decoded thread snapshot: 108.5 KiB
Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

Ttheo25d ago

Note

🤖 Claude Opus 5.5 responding on behalf of Theo

Closing in favor of the copy-only fix in #18208. The triage on #13244 approved either clearer settings copy or a separate control, not removing the catalog gate. We are going with the copy fix.

fix(server): turning off provider update checks no longer hides new models#19343

Fix

Proof

Checks

Approvability

Thread transfer impact