fix(providers): drop a removed provider's custom models (#1273) - #1293
Merged
Conversation
Removing a provider deleted only `config.providers[name]`. Its rows in `config.customModels` stayed behind, and neither consumer filters them: `/api/models` lists every row and the generated Codex catalog emits every row keyed by slug, so the dashboard kept advertising models that resolved to a provider which no longer existed. This is an inconsistency rather than a missing feature. Provider *rename* already maintains the array — `rewriteProviderReferences` rewrites `customModels[].provider` alongside combo targets and Claude tier maps — so the array is meant to track the provider lifecycle, and one of the two sibling operations simply did not. `dropProviderCustomModels` is therefore placed next to the rename pass, so the two stay visible to each other. Both removal paths call it and report the count: `ocx provider remove` prints it and adds `droppedCustomModels` to `--json`, and the management DELETE returns the same field. An emptied list drops the `customModels` field, matching the add/remove routes; the `customModelCatalogMigration` marker is deliberately preserved, since rewriting it would change an older binary's view of one-time row ownership. Partial fix for #1273. The second defect in that report — a stale in-memory config re-persisting deleted rows through a whole-document write — is not addressed here and remains open; it needs a reconciliation design of its own, keyed on the immutable `OcxCustomModel.id` rather than on a slug. Reported by @gdxnpy with a reproduction and a before/after config diff that showed the cooperating save path itself was healthy, which is what made the second defect findable rather than "settings sometimes revert".
Contributor
📝 WalkthroughWalkthroughProvider deletion now removes custom models owned by the deleted provider. CLI and management API responses report the number removed. Migration metadata and models owned by other providers remain unchanged. ChangesProvider Custom Model Cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant ProviderCLI
participant ProviderRoutes
participant Cleanup as dropProviderCustomModels
participant Configuration
Operator->>ProviderCLI: Remove provider
ProviderCLI->>Cleanup: Remove provider-owned custom models
Cleanup->>Configuration: Update customModels
Cleanup-->>ProviderCLI: Return removed count
ProviderCLI->>Configuration: Save configuration
ProviderCLI-->>Operator: Report removal count
ProviderRoutes->>Cleanup: Remove provider-owned custom models
Cleanup-->>ProviderRoutes: Return removed count
ProviderRoutes->>Configuration: Save configuration
ProviderRoutes-->>Operator: Return droppedCustomModels
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/provider.ts`:
- Around line 317-318: Always include droppedCustomModels in successful deletion
results: update src/cli/provider.ts lines 317-318 to emit 0 when no custom
models are removed, and update src/server/management/provider-routes.ts lines
626-630 to do the same for every successful response. Add zero-removal
assertions to both integration tests, while preserving the existing
positive-count reporting.
In `@src/server/management/provider-routes.ts`:
- Around line 618-621: The provider-removal flow must load
dropProviderCustomModels before mutating config so no await occurs between
provider deletion and cleanup. Update the surrounding provider deletion handler
to use a static import or pre-load the helper with
saveConfigPreservingClaudeCode, then perform provider deletion,
dropProviderCustomModels, setProviderContextCap, and save in one synchronous
mutation section.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8aed220f-3c9b-404c-bf26-d5a04af41670
📒 Files selected for processing (6)
src/cli/provider.tssrc/providers/provider-id-rewrite.tssrc/server/management/provider-routes.tstests/cli-provider.test.tstests/management-provider-validation.test.tstests/provider-id-rewrite.test.ts
Comment on lines
+317
to
318
| ...(droppedCustomModels > 0 ? { droppedCustomModels } : {}), | ||
| }, null, 2)); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Always return droppedCustomModels.
When a provider has no custom models, both responses omit droppedCustomModels. Clients cannot distinguish a zero cleanup count from an older server that does not implement this lifecycle cleanup.
src/cli/provider.ts#L317-L318: EmitdroppedCustomModels: 0when no rows are removed.src/server/management/provider-routes.ts#L626-L630: EmitdroppedCustomModels: 0in every successful deletion response.
Add zero-removal assertions to both integration tests.
As per PR objectives, “Report the number removed via CLI output, --json.droppedCustomModels, and the API response”.
📍 Affects 2 files
src/cli/provider.ts#L317-L318(this comment)src/server/management/provider-routes.ts#L626-L630
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/provider.ts` around lines 317 - 318, Always include
droppedCustomModels in successful deletion results: update src/cli/provider.ts
lines 317-318 to emit 0 when no custom models are removed, and update
src/server/management/provider-routes.ts lines 626-630 to do the same for every
successful response. Add zero-removal assertions to both integration tests,
while preserving the existing positive-count reporting.
Comment on lines
+618
to
621
| const { dropProviderCustomModels } = await import("../../providers/provider-id-rewrite"); | ||
| const droppedCustomModels = dropProviderCustomModels(config, name); | ||
| setProviderContextCap(config, name, false); | ||
| save(config); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Import the cleanup helper before mutating config.
Line 617 removes the provider from the live configuration. Line 618 then yields on a dynamic import before it removes the provider-owned custom models. A concurrently resuming request can observe a deleted provider with its orphaned custom models still present.
Load dropProviderCustomModels with saveConfigPreservingClaudeCode before line 616, or use a static import. Keep provider deletion, custom-model cleanup, context-cap cleanup, and saving in one synchronous mutation section.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/management/provider-routes.ts` around lines 618 - 621, The
provider-removal flow must load dropProviderCustomModels before mutating config
so no await occurs between provider deletion and cleanup. Update the surrounding
provider deletion handler to use a static import or pre-load the helper with
saveConfigPreservingClaudeCode, then perform provider deletion,
dropProviderCustomModels, setProviderContextCap, and save in one synchronous
mutation section.
This was referenced Aug 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Partial fix for #1273 — the first of the two defects in that report. It does not close the issue; see "What this does not fix" below.
Removing a provider deleted only
config.providers[name]. Its rows inconfig.customModelsstayed behind, and neither consumer filters them:/api/modelslists every row (src/server/management/model-rows.ts) and the generated Codex catalog emits every row keyed by slug (src/codex/catalog/provider-fetch.ts). The dashboard kept advertising models that resolved to a provider which no longer existed.This is an inconsistency rather than a missing feature. Provider rename already maintains the array —
rewriteProviderReferencesrewritescustomModels[].provideralongside combo targets and Claude tier maps — so the array is meant to track the provider lifecycle, and one of the two sibling operations simply did not.dropProviderCustomModelsis placed directly beside the rename pass so the two stay visible to each other.Both removal paths call it and report the count:
ocx provider removeprints it and addsdroppedCustomModelsto--json; the managementDELETE /api/providersreturns the same field.Two deliberate details:
customModelsfield rather than leaving[], matching the add/remove routes.customModelCatalogMigrationmarker is preserved unchanged. It records one-time ownership of pre-marker rows, and rewriting it would change an older binary's view of what it may delete — the migration module warns against exactly that. Both integration tests assert the marker survives in the persistedconfig.json.What this does not fix
The second defect in #1273 remains open: a stale in-memory config re-persisting deleted rows through a whole-document write.
saveConfigPreservingClaudeCodereconcilesclaudeCodeand the live server binding against its pre-write disk read, but notcustomModels, so a process holding a pre-deletion config resurrects the rows.That needs a reconciliation design of its own, keyed on the immutable
OcxCustomModel.idrather than onroutedSlug(which is an encoding — bothproviderandmodelIdare mutable, so a renamed row reads as a delete plus an insert). It also has to coverprovidersitself, since the same stale write resurrects the provider record. Diagnosis and the writer-path inventory are recorded on the issue.Verification
bun run test— 10009 pass / 7 skip / 0 fail across 626 filesbun test tests/provider-id-rewrite.test.ts tests/management-provider-validation.test.ts— 62 pass / 0 failbun test tests/cli-provider.test.ts— 30 pass / 0 failbun run typecheck— cleanbun run privacy:scan— passedEach path proved independently, since a combined ablation would only have proved one of them:
src/server/management/provider-routes.ts→ 61 pass / 1 fail, failing the new management API testsrc/cli/provider.ts→ 29 pass / 1 fail, failing the new CLI test, which spawns the realocx provider removeand reads the persisted configChecklist
droppedCustomModelsfield is additive and reported only when non-zero.)Reported by @gdxnpy, whose reproduction included a before/after config diff showing the cooperating save path itself was healthy — that detail is what made the second defect findable rather than "settings sometimes revert".
Summary by CodeRabbit
Bug Fixes
CLI & API