fix(web): disambiguate MCP language model selection for ask_codebase#1414
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates MCP ChangesLanguage model selection fix
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant askCodebase as askCodebase
participant selectConfiguredLanguageModel as selectConfiguredLanguageModel
participant configuredModels as configuredModels
askCodebase->>selectConfiguredLanguageModel: resolve(requestedLanguageModel, configuredModels)
selectConfiguredLanguageModel->>configuredModels: filter by provider and model
configuredModels-->>selectConfiguredLanguageModel: matching configured entries
selectConfiguredLanguageModel-->>askCodebase: resolved languageModelConfig or ServiceError
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/ee/features/mcp/askCodebase.test.ts (1)
19-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for the "not configured" (zero candidates) branch.
The suite covers omitted-displayName match, displayName disambiguation, multiple-match error, and displayName-mismatch error, but doesn't test the case where no configured model matches
provider+modelat all (candidateModels.length === 0branch inselectConfiguredLanguageModel, packages/web/src/ee/features/mcp/askCodebase.ts Lines 56-64).✅ Suggested additional test
+ it("returns an error when no configured model matches provider and model", () => { + const configuredModel = createConfiguredModel(); + + const result = selectConfiguredLanguageModel( + [configuredModel], + { provider: 'openai', model: 'gpt-5' } + ); + + expect(result).toEqual({ + error: { + statusCode: StatusCodes.BAD_REQUEST, + errorCode: ErrorCode.INVALID_REQUEST_BODY, + message: "Language model 'openai/gpt-5' is not configured.", + }, + }); + });🤖 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 `@packages/web/src/ee/features/mcp/askCodebase.test.ts` around lines 19 - 85, Add a test in selectConfiguredLanguageModel’s describe block to cover the zero-candidate branch where no configured model matches the requested provider/model. Use createConfiguredModel with a non-matching provider or model, call selectConfiguredLanguageModel with that request, and assert it returns the expected not-configured error shape rather than a languageModelConfig. This should sit alongside the existing cases for disambiguation and displayName mismatch so the candidateModels.length === 0 path is covered.
🤖 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 `@packages/web/src/ee/features/mcp/askCodebase.test.ts`:
- Around line 19-85: Add a test in selectConfiguredLanguageModel’s describe
block to cover the zero-candidate branch where no configured model matches the
requested provider/model. Use createConfiguredModel with a non-matching provider
or model, call selectConfiguredLanguageModel with that request, and assert it
returns the expected not-configured error shape rather than a
languageModelConfig. This should sit alongside the existing cases for
disambiguation and displayName mismatch so the candidateModels.length === 0 path
is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f43ad8e7-f916-49f8-a763-7d2b548aa79c
📒 Files selected for processing (3)
CHANGELOG.mdpackages/web/src/ee/features/mcp/askCodebase.test.tspackages/web/src/ee/features/mcp/askCodebase.ts
|
Can someone take a look when you get a chance? |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/web/src/ee/features/mcp/selectConfiguredLanguageModel.ts (1)
12-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMatching logic looks correct; consider a discriminated union for the return type.
Provider/model-first matching with displayName only for disambiguation matches the PR objective and the test cases exactly. One optional improvement: since every branch sets exactly one of
languageModelConfig/error, a discriminated union return type would let TypeScript enforce that exclusivity and remove the "impossible"!selectedLanguageModelfallback branch currently needed inaskCodebase.ts.♻️ Optional refactor
-): { - languageModelConfig?: T; - error?: ServiceError; -} => { +): ( + | { languageModelConfig: T; error?: never } + | { languageModelConfig?: never; error: ServiceError } +) => {🤖 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 `@packages/web/src/ee/features/mcp/selectConfiguredLanguageModel.ts` around lines 12 - 62, The matching logic in selectConfiguredLanguageModel is fine, but its return shape currently allows both languageModelConfig and error to be optional, which forces downstream fallback handling. Refactor selectConfiguredLanguageModel to return a discriminated union with either a success variant carrying languageModelConfig or an error variant carrying error, and update callers such as askCodebase.ts to branch on that discriminator instead of using an impossible fallback check.
🤖 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 `@packages/web/src/ee/features/mcp/selectConfiguredLanguageModel.ts`:
- Around line 12-62: The matching logic in selectConfiguredLanguageModel is
fine, but its return shape currently allows both languageModelConfig and error
to be optional, which forces downstream fallback handling. Refactor
selectConfiguredLanguageModel to return a discriminated union with either a
success variant carrying languageModelConfig or an error variant carrying error,
and update callers such as askCodebase.ts to branch on that discriminator
instead of using an impossible fallback check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16ff9d9f-da1a-41db-a287-410ddbedba61
📒 Files selected for processing (3)
packages/web/src/ee/features/mcp/askCodebase.test.tspackages/web/src/ee/features/mcp/askCodebase.tspackages/web/src/ee/features/mcp/selectConfiguredLanguageModel.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/ee/features/mcp/askCodebase.test.ts
…del result; askCodebase drops impossible fallback branch
|
Both nitpicks fixed in two commits:
|
|
Can someone take a look when you have a chance? |
1 similar comment
|
Can someone take a look when you have a chance? |
…mcp-language-model-matching-1137
|
Can someone take a look when you have a chance? |
|
Can someone take a look when you have a chance? |
1 similar comment
|
Can someone take a look when you have a chance? |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit af2ce2b. Configure here.
|
Can someone take a look when you have a chance? |
|
Can someone take a look when you have a chance? |
1 participant
Fixes #1137
Summary
askCodebaserequests byproviderandmodelbefore consideringdisplayNamedisplayNameonly when multiple configured models share the sameproviderandmodelselectConfiguredLanguageModelinto a server-free module so vitest can run withoutserver-onlyimportsselectConfiguredLanguageModelto a discriminated union soaskCodebasecan drop the impossible fallback branchTest plan
yarn workspace @sourcebot/web test --run src/ee/features/mcp/askCodebase.test.ts(5 passed: match by provider+model, displayName disambiguation, multi-match error, displayName-mismatch error, not-configured error)yarn workspace @sourcebot/web linton touched files (0 errors)yarn build:depsbefore test runReadLintsclean for the touched filesSummary by CodeRabbit
Note
Low Risk
Narrow validation change on the MCP/programmatic ask path with clear error responses and unit tests; no auth or data-model changes.
Overview
Fixes
ask_codebase(and the sharedaskCodebasepath) so MCP clients can pick a configured model withprovider+modelalone, instead of requiring an exact match on the old composite key that always includeddisplayName.selectConfiguredLanguageModelis a new server-free helper: it filters configured models by provider/model, usesdisplayNameonly when several entries share that pair, and returns structured 400 errors for not configured, ambiguous, or wrong display name cases.askCodebasewires this in and dropsgetLanguageModelKeyfor that flow.Unit tests cover match, disambiguation, and error paths; the changelog notes the EE fix ([#1414]).
Reviewed by Cursor Bugbot for commit de01419. Bugbot is set up for automated code reviews on this repo. Configure here.