feat(catalog): per-provider retainModels opt-in (#1690, carries #2860) - #3206
Conversation
Operators can pin a model id so live discovery keeps it even when the upstream catalog omits it. The new opt-in is purely additive: built-in kimi/xai tables and the Vertex default continue to win, unknown ids are never inflated, and the existing one-line diagnostic still flags ids the live catalog dropped. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
) Builds on the carried #2860 predicate. retainModels ids now enter the configured seed (ordered union with the Vertex default and models), so a retain-only id survives live discovery, liveModels:false, and gets the same provider hints; selectedModels precedence is unchanged. Adds zod schema and superRefine validation, management PATCH/DTO/safe-config plumbing, ocx provider edit --retain-models <ids|->, rename-migration coverage, discovery-flight fingerprint, and docs. Design input: #2122 (union, schema, MODEL_ID_LISTS).
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ 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: Team Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds a provider-level ChangesProvider model retention
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ProviderRoutes
participant ProviderConfig
participant fetchProviderModels
participant LiveDiscovery
participant RoutedCatalog
CLI->>ProviderRoutes: PATCH retainModels list or null
ProviderRoutes->>ProviderConfig: validate, normalize, and persist
fetchProviderModels->>LiveDiscovery: request provider models
LiveDiscovery-->>fetchProviderModels: return live model ids
fetchProviderModels->>ProviderConfig: read models and retainModels
fetchProviderModels->>RoutedCatalog: merge retained ids with live models
Suggested reviewers: ✨ Finishing Touches📝 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 |
![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: 847571a934
ℹ️ 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".
| if (Object.hasOwn(rawBody, "retainModels")) { | ||
| const value = rawBody.retainModels; |
There was a problem hiding this comment.
When an operator sets this field with ocx provider edit and later submits the same provider name through the dashboard's Add Provider flow, gui/src/provider-payload.ts omits retainModels and the POST route replaces the entire provider row. Unlike other fields absent from that payload, the overwrite logic does not carry this list forward, so the save silently clears the opt-in and the retained model disappears after catalog convergence. Preserve the existing list when a POST omits it, while allowing an explicitly submitted value to win.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| Use `retainModels` for the opposite problem: a provider whose `/models` endpoint omits an id that is | ||
| still callable (a private deployment, a preview id, an OpenAI-compatible gateway with a partial | ||
| listing). Listed ids are kept in the routed catalog with the same context and effort hints as | ||
| `models`, and they survive `liveModels: false` too. `selectedModels` still narrows what is visible, |
There was a problem hiding this comment.
This paragraph correctly says retained IDs survive liveModels: false, but lines 570–571 in the same section still state that this mode exposes only models and exposes nothing when models is absent. A retainModels-only configuration now exposes retained rows, as the new runtime path and regression test demonstrate, so the contradictory earlier statement can mislead operators about which models remain visible; describe the static result as the union of models and retainModels.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
2 participants
Summary
retainModelsopt-in from [Feature]: config-level retainModels allow-list so operators keep configured models in the authoritative live catalog #1690: ids listed there stay in the routed catalog even when live/modelsdiscovery omits them. Carries PR feat(catalog): honor per-provider retainModels opt-in (closes #1690) #2860 (rrmlima, source commit12e69c200) as the retention predicate (shouldRetainConfiguredProviderModel+modelInList), and takes the config/migration design from PR feat(catalog): config-level retainModels allowlist for authoritative discovery (#1690) #2122 (chilung-cgu) as input.retainModelsis self-sufficient: ids no longer need to be repeated inmodels. The configured seed infetchProviderModelsWithAuthis now an ordered dedupe union of the Vertex default,models, andretainModels, so a retain-only id survives live discovery,liveModels: false, the degraded fallback, and gets the same context/effort hints as amodelsentry.selectedModelsprecedence is unchanged (retain does not imply visible).superRefinevalidation insrc/config.ts(hand-edited"retainModels": "x"fails with a path), management PATCH (null/empty clears, non-blank array validated) and DTO echo,providerManagementConfigErrorand safe-config key list, andocx provider edit <name> --retain-models <id,id|->(-clears).retainModelsjoinsMODEL_ID_LISTSso a retired id is renamed instead of resurrected as a ghost row, and joins the discovery-flight fingerprint.retainModelsproduces the same catalog as today; the built-in kimi/xai tables still win. Not in this PR: the antigravitygemini-3.7-flashproduct default, a dashboard field, and the 404-time warning from feat(catalog): config-level retainModels allowlist for authoritative discovery (#1690) #2122.retainModelsrow and paragraph inreference/configuration/providers.md(English).Closes #1690
Verification
bun x tsc --noEmitclean.bun run privacy:scanpassed.bun test tests/catalog-retain-models.test.ts tests/model-rename-migration.test.ts tests/cli-headless-parity.test.ts tests/management-provider-validation.test.ts tests/codex-catalog.test.ts→ 412 pass / 0 fail. New cases cover the realgatherRoutedModelspath (retain-only id survives live omission with hints, single row when live also returns it,liveModels: false, absent list keeps today's drop), config load normalization/rejection, PATCH set/clear round-trip, CLI csv and-, and rename migration.Checklist
Summary by CodeRabbit
retainModelsprovider setting to keep specified models visible when live discovery omits them.