Repository navigation
refactor(shared): resolve profile model IDs through provider definitions - #2239
Conversation
ProfileValidator kept a private switch that duplicated the provider model-ID mapping already owned by the provider definitions. Replace it with getModelId() so the allow-list check has a single source of truth. This also closes gaps in the old switch: - Providers it never handled (poe, moonshot, kimiCode, minimax, mimo, openaiCodex, qwenCode, zai, baseten, geminiCli, vercelAiGateway, zooGateway, nanogpt, kenari, opencodeGo) resolved no model and could only pass via a provider-level allowAll. They are now subject to the model allow-list. - fakeAi returned undefined; it now resolves apiModelId. - Retired providers returned undefined; they now resolve through the legacy modelIdKeys precedence. - getModelId() validates the profile against the provider schema, so an invalid profile resolves no model and fails closed unless the provider is allowAll. Extend the tests to cover the newly handled providers and each of these behavioral deltas.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Shared model resolution and validation src/shared/ProfileValidator.ts, src/shared/__tests__/ProfileValidator.spec.ts |
ProfileValidator uses getModelId instead of a provider-specific lookup. Tests add provider model-field cases and check allow-list behavior for fake AI, a retired Groq identifier, and an OpenAI profile with invalid openAiExtraBody. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Refactor
Possibly related PRs
- Zoo-Code-Org/Zoo-Code#2117: Enforces
ProfileValidator.isProfileAllowedduringupsertProviderProfile, so it uses the validator contract changed here. - Zoo-Code-Org/Zoo-Code#2192: Replaces a CLI model-ID switch with the same
getModelIdutility. - Zoo-Code-Org/Zoo-Code#2229: Replaces webview provider-specific model-field reads with
getModelId.
Merge Risk: 
21b16
-
The shared resolver replaces duplicated model lookup logic, with coverage for additional providers and invalid-profile allow-list behavior. No actionable merge-blocking issue remains.
Pre-merge checks: 
-
Check name Status Explanation Resolution Description check The description explains the refactor, test procedure, and checklist status. However, the required approved GitHub Issue link is missing, and the description explicitly states that no issue exists. Link this PR to an approved GitHub Issue and replace the 'No issue, simple refactor' entry with the issue reference, such as 'Closes: #123'.-
Check name Status Explanation Title check The title clearly summarizes the main change: resolving profile model IDs through provider definitions. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Regression Evidence PASS. The changed call to getModelId(profile)insrc/shared/ProfileValidator.ts:17has focused coverage. The updated table tests canonical providers with both allowed and rejected model IDs (`Prof…Security Boundaries No changed path bypasses the allowlist or executes unvalidated input. ProfileValidatorstill checks the provider and exact model membership, while the changed call delegates model resolution to `get…Persistence Integrity No changed persistence path exists. The production diff only replaces a model-ID lookup in ProfileValidator.isProfileAllowed; it adds no writes, async operations, transactions, rollback logic, or pe…Lifecycle Resource Cleanup No changed lifecycle path exists. The PR only replaces a synchronous model-ID switch in ProfileValidator.isProfileAllowedwithgetModelId(profile)and adds validation tests.getModelIdperforms …Full details: Regression Evidence
-
Explanation
PASS. The changed call to
getModelId(profile)insrc/shared/ProfileValidator.ts:17has focused coverage. The updated table tests canonical providers with both allowed and rejected model IDs (ProfileValidator.spec.ts:14-75). Existing tests cover VS Code LM and GitHub Copilot selectors, unset-model fallback, provider/model rejection, and missing model lists. New tests cover fake-AI, retired-provider precedence, schema-validation failure, and provider-levelallowAll(ProfileValidator.spec.ts:399-469). No UI files changed, so the snapshot requirement does not apply.
Full details: Security Boundaries
-
Explanation
No changed path bypasses the allowlist or executes unvalidated input.
ProfileValidatorstill checks the provider and exact model membership, while the changed call delegates model resolution togetModelId(). Canonical provider definitions schema-validate settings and return no model on failure; the validator then allows only an explicitly configured provider-levelallowAll. Retired-provider resolution only selects a legacy model field for the same exact allowlist comparison. The changed tests cover fake-AI, retired-provider, and invalid-profile cases.
Full details: Persistence Integrity
-
Explanation
No changed persistence path exists. The production diff only replaces a model-ID lookup in
ProfileValidator.isProfileAllowed; it adds no writes, async operations, transactions, rollback logic, or persisted defaults. The test diff only adds validation cases.
Full details: Lifecycle Resource Cleanup
-
Explanation
No changed lifecycle path exists. The PR only replaces a synchronous model-ID switch in
ProfileValidator.isProfileAllowedwithgetModelId(profile)and adds validation tests.getModelIdperforms schema parsing and field lookup; the changed files add no listeners, watchers, timers, tasks, subscriptions, disposal, cancellation, or restart logic.
-
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Review statusThanks for contributing. This comment tracks the review sequence and the next action. Current step: The required review sequence passed. Remaining merge requirements apply. Review-state labels are managed by this workflow; do not edit them manually. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
coderabbit-review-active
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-actions
Bot
added
awaiting-maintainer
github-actions
Bot
removed
the
awaiting-maintainer
…ons (#2239) ProfileValidator kept a private switch that duplicated the provider model-ID mapping already owned by the provider definitions. Replace it with getModelId() so the allow-list check has a single source of truth. This also closes gaps in the old switch: - Providers it never handled (poe, moonshot, kimiCode, minimax, mimo, openaiCodex, qwenCode, zai, baseten, geminiCli, vercelAiGateway, zooGateway, nanogpt, kenari, opencodeGo) resolved no model and could only pass via a provider-level allowAll. They are now subject to the model allow-list. - fakeAi returned undefined; it now resolves apiModelId. - Retired providers returned undefined; they now resolve through the legacy modelIdKeys precedence. - getModelId() validates the profile against the provider schema, so an invalid profile resolves no model and fails closed unless the provider is allowAll. Extend the tests to cover the newly handled providers and each of these behavioral deltas.
Related GitHub Issue
No issue, simple refactor.
Description
ProfileValidator kept a private switch that duplicated the provider model-ID mapping already owned by the provider definitions. Replace it with getModelId() so the allow-list check has a single source of truth.
Test Procedure
Unit, integration tests
Pre-Submission Checklist
*.visual.tsxsnapshot inwebview-ui/. Seewebview-ui/AGENTS.md→ "When a UI change needs a snapshot".Visual Snapshots
n/a
Videos (interaction / animation only)
n/a
Documentation Updates
n/a