fix(web): disambiguate MCP language model selection for ask_codebase#1414
Open
Harsh23Kashyap wants to merge 4 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR fixes MCP ChangesLanguage model selection fix
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant askCodebase
participant selectConfiguredLanguageModel
Client->>askCodebase: request with languageModel (provider, model)
askCodebase->>selectConfiguredLanguageModel: configuredModels, requestedLanguageModel
alt no match
selectConfiguredLanguageModel-->>askCodebase: ServiceError (not configured)
else displayName mismatch
selectConfiguredLanguageModel-->>askCodebase: ServiceError (wrong displayName)
else multiple matches, no displayName
selectConfiguredLanguageModel-->>askCodebase: ServiceError (ambiguous, need displayName)
else single match
selectConfiguredLanguageModel-->>askCodebase: languageModelConfig
end
askCodebase-->>Client: ServiceError or selected config
Possibly related PRs
🚥 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? |
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 sameproviderandmodelTest plan
yarn workspace @sourcebot/web test --run "src/ee/features/mcp/askCodebase.test.ts"yarn workspace @sourcebot/web lint "src/ee/features/mcp/askCodebase.ts" "src/ee/features/mcp/askCodebase.test.ts"ReadLintsclean for the touched filesnode_modulesstate file)Summary by CodeRabbit
displayNameonly when multiple configured entries would otherwise be ambiguous.displayName, and invaliddisplayNamevalues.