Repository navigation
feat(models): newly discovered provider models arrive disabled (#2464) - #2609
Conversation
reconcileSuccessfulModelDiscoveries reported changed=true for every authoritative provider, so convergence persisted config.json on every catalog write - including the common case where a provider's roster is byte-identical to the stored baseline. That is write amplification with no user-visible cause, and it moves the config generation other writers revalidate against. Compare only the fields that carry meaning. updatedAt cannot participate: it moves on every successful fetch, so including it would report a change every time. When nothing else moved, the previous timestamp is kept too, or the very next comparison would look dirty and reintroduce the rewrite. Falsified: forcing changed=true unconditionally fails the new steady-state test; the genuine-arrival test stays green either way, which is what pins that the guard narrows rather than disables the policy.
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughThe pull request adds persisted model-discovery baselines and configurable global or provider policies. It updates catalog convergence, management APIs, CLI commands, the Models GUI, localized strings, initialization defaults, documentation, and automated tests. ChangesModel discovery policy
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Provider
participant CatalogConvergence
participant ModelPolicy
participant Config
participant ModelsGUI
Provider->>CatalogConvergence: return authoritative model roster
CatalogConvergence->>ModelPolicy: reconcile successful discoveries
ModelPolicy->>Config: update baseline and disabledModels
CatalogConvergence->>Config: persist after successful commit
ModelsGUI->>Config: load discovery state
ModelsGUI->>Config: save global or provider policy
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
⏳ DRAFT
What to do
Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4cb64fcee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const models = await fetchAllModels(config); | ||
| const known = config.modelDiscovery.knownModels ??= {}; | ||
| const at = new Date().toISOString(); | ||
| for (const name of Object.keys(config.providers)) { | ||
| known[name] ??= { ids: [...new Set(models.filter(m => m.provider === name).map(m => m.id))].sort(), removed: [], updatedAt: at }; |
There was a problem hiding this comment.
When an existing install first switches the global policy from its absent/on-by-default state to off while provider discovery is degraded, fetchAllModels can return stale or static fallback rows, but this block records those rows as the successful baseline for every provider. On the next authoritative recovery, any live IDs omitted from that fallback are classified as new and appended to disabledModels, potentially hiding most of the provider's catalog. Defer baseline creation to convergence's authoritative provider outcomes rather than bootstrapping from the aggregate fallback result.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| removed.add(id); | ||
| } else missing[id] = count; | ||
| } | ||
| for (const id of discovered) active.add(id); |
There was a problem hiding this comment.
After a tombstoned model reappears, it is added back to active without being removed from removed, so the sets overlap and this sum counts that ID twice. For a large aggregator with enough restored/flapping IDs, the computed size can exceed 2,000 while the distinct union is still within the limit; reconcileSuccessfulModelDiscoveries then skips the entire result, including arrivals and auto-disables, allowing genuinely new models through. Compute the size of the actual set union or keep the sets disjoint.
Useful? React with 👍 / 👎.
| <div className="row models-provider-hint"> | ||
| <span className="muted text-label">{t("models.newPolicyProvider")}</span> | ||
| <div className="segmented models-segmented" role="radiogroup" aria-label={t("models.newPolicyProvider")}> | ||
| {(["off", "on"] as const).map(mode => ( |
There was a problem hiding this comment.
Once a user selects an explicit per-provider on or off value, later global-policy changes no longer affect that provider, but this control renders only those two explicit modes even though the configuration and DTO include inherit; the PUT endpoint also rejects inherit. The override therefore cannot be cleared through the dashboard or CLI without manually editing config.json. Add the inherited option and normalize it by deleting the provider override.
AGENTS.md reference: gui/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
…-jun#2464) (lidge-jun#2609) * feat(models): disable newly discovered models by policy * fix(models): do not rewrite config when the roster is unchanged reconcileSuccessfulModelDiscoveries reported changed=true for every authoritative provider, so convergence persisted config.json on every catalog write - including the common case where a provider's roster is byte-identical to the stored baseline. That is write amplification with no user-visible cause, and it moves the config generation other writers revalidate against. Compare only the fields that carry meaning. updatedAt cannot participate: it moves on every successful fetch, so including it would report a change every time. When nothing else moved, the previous timestamp is kept too, or the very next comparison would look dirty and reintroduce the rewrite. Falsified: forcing changed=true unconditionally fails the new steady-state test; the genuine-arrival test stays green either way, which is what pins that the guard narrows rather than disables the policy.
…-jun#2464) (lidge-jun#2609) * feat(models): disable newly discovered models by policy * fix(models): do not rewrite config when the roster is unchanged reconcileSuccessfulModelDiscoveries reported changed=true for every authoritative provider, so convergence persisted config.json on every catalog write - including the common case where a provider's roster is byte-identical to the stored baseline. That is write amplification with no user-visible cause, and it moves the config generation other writers revalidate against. Compare only the fields that carry meaning. updatedAt cannot participate: it moves on every successful fetch, so including it would report a change every time. When nothing else moved, the previous timestamp is kept too, or the very next comparison would look dirty and reintroduce the rewrite. Falsified: forcing changed=true unconditionally fails the new steady-state test; the genuine-arrival test stays green either way, which is what pins that the guard narrows rather than disables the policy.
Summary
Closes #2464. A provider with
liveModels: truere-fetches its/modelsendpoint on every catalog sync, so any model the vendor publishes overnight appears in the Codex picker silently. For a proxy that routes real spend, the user should opt models IN as they arrive.The mechanism is a persisted per-provider baseline: a model is new iff it is discovered now and absent from the last successful baseline. New arrivals get a
disabledModelsrow instead of aselectedModelsseed — the blocklist composes with an empty allowlist, whereas seeding the allowlist with the full current roster would convert "all on" into a frozen snapshot and silently change what the user's existing state means.Three properties carry the design:
Each id can be auto-disabled at most once, ever. A model that disappears moves to a tombstone after three consecutive successful fetches without it, and newness is
discovered − ids − removed. Sinceids ∪ removedgrows monotonically, the flapping case holds: a user enables a new arrival, the vendor drops it long enough to prune, the vendor restores it — it comes back enabled. A rename still counts as new, which is the safe reading since pricing may have changed with it.A degraded fetch can never shrink the baseline. Reconciliation runs only for providers whose gather outcome is
authoritative. A provider outage would otherwise mark its entire catalog "new" on recovery.Bootstrap hides nothing. The first run with no baseline marks everything known. Existing installs have no
modelDiscoverykey at all, which means policy "on" — today's behavior exactly, with zero change until the user opts in.Preset mode (#2465) is a deliberate, tested no-op: a non-empty allowlist already excludes arrivals, so the blocklist branch would be redundant state to reconcile later. That interaction is the reason #2464 had to land after #2465 rather than beside it.
Verification
Falsified per hunk: bootstrap reporting the initial catalog as new, auto-disable slugs suppressed, preset-mode providers auto-disabled, degraded providers reconciled, and each API route hunk removed — every one turned its test red, and all were restored before the final run.
The GUI was rendered against a stubbed management API rather than trusted to typecheck; the first render exposed header crowding, which moved the per-provider override into the expanded body.
Review fix included: reconciliation originally reported
changed = truefor every authoritative provider, so convergence rewroteconfig.jsonon every catalog write even when the roster was byte-identical. That is write amplification that also moves the config generation other writers revalidate against. Now only meaningful fields are compared —updatedAtcannot participate, since it moves on every fetch and would report a change every time. Falsified: forcingchanged = truefails the steady-state test while the genuine-arrival test stays green, which pins that the guard narrows the write rather than disabling the policy.The convergence-call count in
codex-convergence-contract.test.tsgoes 8 → 10 with a companion assertion naming the two new model-discovery write paths, following the pattern the preset routes established — a bare count that only ever rises stops being a contract.Checklist
devdocs-site/updatedSummary by CodeRabbit
New Features
Documentation