Preserve custom provider lineage during ID migrations#1025
Merged
PeterDaveHello merged 1 commit intoJul 23, 2026
Merged
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
📝 WalkthroughWalkthroughProvider migration now preserves legacy provider IDs, normalizes endpoint matching, handles reserved builtin ID collisions and secrets, and improves runtime provider resolution across API modes, sessions, disabled providers, and imports. ChangesProvider migration and resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant resolveOpenAICompatibleRequest
participant ProviderRegistry
participant CustomApiMode
Session->>resolveOpenAICompatibleRequest: provide providerId, customUrl, and mode metadata
resolveOpenAICompatibleRequest->>ProviderRegistry: match current and legacy provider IDs
ProviderRegistry->>CustomApiMode: filter candidates by stable mode label
CustomApiMode-->>resolveOpenAICompatibleRequest: return an unambiguous provider
resolveOpenAICompatibleRequest-->>Session: return providerId, requestUrl, and apiKey
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Contributor
PR Summary by QodoPreserve custom provider lineage during ID migrations
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
![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: 5422198963
ℹ️ 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".
Comment on lines
+1155
to
+1157
| const legacyProviderIds = getLegacyProviderIds( | ||
| [...(Array.isArray(provider.legacyProviderIds) ? provider.legacyProviderIds : []), originalId], | ||
| id, |
There was a problem hiding this comment.
When two stored providers canonicalize to the same ID (for example, My Proxy and my-proxy), the first remains my-proxy while the second is renamed to my-proxy-2 and receives my-proxy as a legacy ID here. Historical sessions using my-proxy without a distinguishing URL or unique mode label then match both providers in resolveOpenAICompatibleRequest and fail closed, so those conversations can no longer send requests. Reserve already-declared/derived legacy IDs while assigning IDs (or avoid retaining an alias already owned by another provider) so lineage does not create an ambiguous current-ID collision.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR strengthens backward-compatible resolution for custom OpenAI-compatible providers when provider IDs change or become newly reserved by built-in providers, preserving API keys and maintaining deterministic recovery for historical sessions while failing closed in ambiguous/disabled cases.
Changes:
- Introduces provider ID lineage (
legacyProviderIds) and canonical-equivalence matching to keep sessions and UI selections linked across ID migrations. - Adds URL- and label-based disambiguation to resolve collisions safely, including explicit “fail closed” behavior when matches are ambiguous or disabled.
- Persists and replays one-time migration markers (
completedBuiltinProviderIdMigrations) during config migration and import normalization.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/utils/model-name-convert.test.mjs | Adds coverage for provider-ID equivalence and URL-based disambiguation in API mode selection helpers. |
| tests/unit/services/apis/provider-registry.test.mjs | Adds coverage for collision disambiguation (URL/label) and disabled-provider fail-closed behavior in request resolution. |
| tests/unit/popup/import-data-cleanup.test.mjs | Verifies import normalization behavior for builtin provider ID migration markers. |
| tests/unit/popup/api-modes-provider-utils.test.mjs | Adds coverage for legacy provider IDs affecting provider ID generation, referencing, and conversation labeling. |
| tests/unit/config/migrate-user-config.test.mjs | Adds extensive coverage for provider ID collision migrations, secret preservation, and lineage persistence. |
| src/utils/model-name-convert.mjs | Implements canonical provider ID equivalence and URL disambiguation for session-compatible API mode selection. |
| src/services/apis/provider-registry.mjs | Enhances custom provider resolution to handle normalized/legacy ID collisions with URL/label-based disambiguation and fail-closed rules. |
| src/popup/sections/import-data-cleanup.mjs | Ensures imported provider state triggers builtin provider ID migration replay when markers are missing. |
| src/popup/sections/ApiModes.jsx | Uses canonical provider ID equivalence when deciding whether to clear provider-derived fields on edit. |
| src/popup/sections/api-modes-provider-utils.mjs | Adds provider ID equivalence utilities, expands collision handling using URL, and avoids reusing legacy provider IDs. |
| src/config/index.mjs | Extends user-config migration to preserve lineage, normalize paths, migrate colliding IDs/secrets safely, and persist migration completion markers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
5 rules 1. Import overwrites builtin secret
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Safely migrate custom providers whose IDs become reserved by built-in providers without losing API keys or historical session routing. Retain legacy provider IDs for deterministic recovery. Preserve completed migration state across partial imports, and keep existing built-in secrets from being assigned to newly imported custom providers. Use exact raw provider IDs when canonical siblings collide so label and secret recovery stays isolated. Keep ambiguous or disabled matches fail-closed.
PeterDaveHello
force-pushed
the
harden-provider-model-routing
branch
from
July 22, 2026 21:14
5422198 to
d76707a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/apis/provider-registry.mjs (1)
392-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated canonical-collision check into a shared helper. The
getAllOpenAIProviders(config).filter(p => normalizeProviderId(p.id) === normalizedProviderId).length > 1computation is repeated verbatim in three places; since this drives secret routing under reserved-ID collisions, divergence between copies would silently misroute secrets. A singlehasCanonicalProviderIdCollision(config, normalizedProviderId)helper keeps them in lock-step.
src/services/apis/provider-registry.mjs#L392-L401: haveresolveSecretProviderIdcall the new helper.src/services/apis/provider-registry.mjs#L213-L216: replace the inline filter ingetConfiguredCustomApiModesForProviderwith the helper.src/services/apis/provider-registry.mjs#L462-L465: replace the inline filter ingetProviderSecretwith the helper (the boolean is still needed locally forcanUseApiModeApiKey).🤖 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/services/apis/provider-registry.mjs` around lines 392 - 401, Extract the repeated canonical-collision computation into a shared hasCanonicalProviderIdCollision(config, normalizedProviderId) helper. Update resolveSecretProviderId, getConfiguredCustomApiModesForProvider, and getProviderSecret to call it, preserving the local boolean in getProviderSecret for canUseApiModeApiKey. Apply this in src/services/apis/provider-registry.mjs at lines 392-401, 213-216, and 462-465 respectively.
🤖 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.
Nitpick comments:
In `@src/services/apis/provider-registry.mjs`:
- Around line 392-401: Extract the repeated canonical-collision computation into
a shared hasCanonicalProviderIdCollision(config, normalizedProviderId) helper.
Update resolveSecretProviderId, getConfiguredCustomApiModesForProvider, and
getProviderSecret to call it, preserving the local boolean in getProviderSecret
for canUseApiModeApiKey. Apply this in src/services/apis/provider-registry.mjs
at lines 392-401, 213-216, and 462-465 respectively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a038b7fa-2038-4335-be71-fd87731eb1a6
📒 Files selected for processing (11)
src/config/index.mjssrc/popup/sections/ApiModes.jsxsrc/popup/sections/api-modes-provider-utils.mjssrc/popup/sections/import-data-cleanup.mjssrc/services/apis/provider-registry.mjssrc/utils/model-name-convert.mjstests/unit/config/migrate-user-config.test.mjstests/unit/popup/api-modes-provider-utils.test.mjstests/unit/popup/import-data-cleanup.test.mjstests/unit/services/apis/provider-registry.test.mjstests/unit/utils/model-name-convert.test.mjs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/popup/sections/ApiModes.jsx
- tests/unit/utils/model-name-convert.test.mjs
- tests/unit/services/apis/provider-registry.test.mjs
- src/utils/model-name-convert.mjs
- tests/unit/popup/api-modes-provider-utils.test.mjs
- tests/unit/config/migrate-user-config.test.mjs
- src/popup/sections/api-modes-provider-utils.mjs
- src/config/index.mjs
| assert.equal(resolved.apiKey, 'proxy-key') | ||
| }) | ||
|
|
||
| test('resolveOpenAICompatibleRequest disambiguates legacy and normalized ID matches by URL', () => { |
Contributor
There was a problem hiding this comment.
1. Long test() description line 📘 Rule violation ⚙ Maintainability
A newly added test() declaration line exceeds the 100-character maximum, reducing readability and increasing diff noise. This violates the repository line-length compliance requirement.
Agent Prompt
## Issue description
One or more newly added lines exceed the 100 character line-length limit.
## Issue Context
The compliance checklist requires source lines to be <= 100 characters.
## Fix Focus Areas
- tests/unit/services/apis/provider-registry.test.mjs[134-134]
- tests/unit/services/apis/provider-registry.test.mjs[189-189]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+45
to
+77
| const legacyKeys = relevantProviderIds.map( | ||
| (providerId) => LEGACY_API_KEY_FIELD_BY_PROVIDER_ID[providerId], | ||
| ) | ||
| const stored = await storageArea.get([ | ||
| 'completedBuiltinProviderIdMigrations', | ||
| 'customOpenAIProviders', | ||
| 'providerSecrets', | ||
| ...legacyKeys, | ||
| ]) | ||
| const completedMigrations = new Set( | ||
| Array.isArray(stored.completedBuiltinProviderIdMigrations) | ||
| ? stored.completedBuiltinProviderIdMigrations.map(normalizeProviderId).filter(Boolean) | ||
| : [], | ||
| ) | ||
| const existingProviderIds = new Set( | ||
| (Array.isArray(stored.customOpenAIProviders) ? stored.customOpenAIProviders : []) | ||
| .map((provider) => normalizeProviderId(provider?.id)) | ||
| .filter(Boolean), | ||
| ) | ||
| const existingProviderSecrets = | ||
| stored.providerSecrets && typeof stored.providerSecrets === 'object' | ||
| ? stored.providerSecrets | ||
| : {} | ||
|
|
||
| for (const providerId of relevantProviderIds) { | ||
| const legacyKey = LEGACY_API_KEY_FIELD_BY_PROVIDER_ID[providerId] | ||
| if ( | ||
| completedMigrations.has(providerId) && | ||
| !existingProviderIds.has(providerId) && | ||
| Object.hasOwn(existingProviderSecrets, providerId) | ||
| ) { | ||
| normalizedData[legacyKey] = existingProviderSecrets[providerId] | ||
| } |
Contributor
There was a problem hiding this comment.
2. Import overwrites builtin secret 🐞 Bug ≡ Correctness
preserveExistingBuiltinSecrets() only restores stored builtin secrets from providerSecrets[providerId] and ignores the legacy-key values it already fetches, so a provider-only import can overwrite a valid legacy-only builtin key with an empty/stale imported legacy field. This can silently drop API credentials for newly reserved builtin providers after import.
Agent Prompt
### Issue description
`preserveExistingBuiltinSecrets()` fetches legacy builtin key fields (e.g., `xaiApiKey`) from storage but never uses their stored values. If the existing correct secret is only present in the legacy key field (or `providerSecrets[providerId]` is missing/stale) and the import payload contains that legacy key as empty/stale, `importDataIntoStorage()` will overwrite the correct value.
### Issue Context
- This helper runs specifically when importing `customOpenAIProviders` **without** importing `providerSecrets`, which is exactly when protecting existing secrets matters most.
- The code already requests `...legacyKeys` from storage, so the intent appears to be to preserve those values, but the implementation only consults `stored.providerSecrets`.
### Fix Focus Areas
- src/popup/sections/import-data-cleanup.mjs[27-79]
### Suggested fix
- When iterating `relevantProviderIds`, compute the existing builtin secret with a precedence such as:
1) `stored.providerSecrets[providerId]` if present and non-empty, else
2) `stored[legacyKey]` (trimmed) if present and non-empty.
- If that existing secret is non-empty and the import would otherwise set `normalizedData[legacyKey]` to empty/stale (at minimum: empty/missing), write `normalizedData[legacyKey] = existingSecret`.
- Add/extend a unit test for the case where:
- `completedBuiltinProviderIdMigrations` contains the providerId,
- `stored.providerSecrets` is missing the providerId entry,
- `stored[legacyKey]` has the real secret,
- import payload would set `legacyKey` to `''` (or includes it as empty),
- and verify the stored secret is preserved after import + subsequent `getUserConfig()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
196
to
202
| const signature = JSON.stringify({ | ||
| groupName: toStringOrEmpty(apiMode.groupName).trim(), | ||
| itemName: toStringOrEmpty(apiMode.itemName).trim(), | ||
| isCustom: Boolean(apiMode.isCustom), | ||
| customName: toStringOrEmpty(apiMode.customName).trim(), | ||
| providerId: normalizeProviderId(apiMode.providerId), | ||
| providerId: toStringOrEmpty(apiMode.providerId).trim(), | ||
| }) |
Contributor
|
Code review by qodo was updated up to the latest commit d76707a |
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
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.
Safely migrate custom providers whose IDs become reserved by built-in providers without losing API keys or historical session routing.
Retain legacy provider IDs for deterministic recovery and keep ambiguous or disabled matches fail-closed.
Summary by CodeRabbit