fix(web): disambiguate MCP language model selection for ask_codebase#1414
Open
Harsh23Kashyap wants to merge 5 commits into
Open
Conversation
Contributor
WalkthroughThis PR updates MCP ChangesLanguage model selection fix
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant askCodebase
participant selectConfiguredLanguageModel
askCodebase->>selectConfiguredLanguageModel: configuredModels, requestedLanguageModel
alt no match
selectConfiguredLanguageModel-->>askCodebase: ServiceError
else displayName mismatch
selectConfiguredLanguageModel-->>askCodebase: ServiceError
else multiple matches without displayName
selectConfiguredLanguageModel-->>askCodebase: ServiceError
else single match
selectConfiguredLanguageModel-->>askCodebase: languageModelConfig
end
Possibly related PRs
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}
Contributor
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
Author
|
Can someone take a look when you get a chance? |
Contributor
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
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.
Fixes #1137
Summary
askCodebaserequests byproviderandmodelbefore consideringdisplayNamedisplayNameonly when multiple configured models share the sameproviderandmodelselectConfiguredLanguageModelinto a server-free module so vitest can run withoutserver-onlyimportsTest plan
yarn workspace @sourcebot/web test --run src/ee/features/mcp/askCodebase.test.ts(4 passed)yarn workspace @sourcebot/web linton touched files (0 errors)yarn build:depsbefore test runReadLintsclean for the touched files