fix(mcp): match language model by provider+model, fall back when displayName absent#1153
Open
dmitrievav wants to merge 2 commits intosourcebot-dev:mainfrom
Open
fix(mcp): match language model by provider+model, fall back when displayName absent#1153dmitrievav wants to merge 2 commits intosourcebot-dev:mainfrom
dmitrievav wants to merge 2 commits intosourcebot-dev:mainfrom
Conversation
Contributor
|
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:
WalkthroughaskCodebase now selects models by matching Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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/features/mcp/askCodebase.ts (1)
60-72: Matching semantics now diverge from other call sites; consider consolidating.The new predicate fixes the MCP case correctly, but
packages/web/src/app/api/(server)/chat/route.tsandpackages/web/src/features/chat/useSelectedLanguageModel.tsstill match viagetLanguageModelKey(strict provider+model+displayName). With this change, the same{provider, model}payload that resolves successfully in MCP could be rejected (or reset) on those paths, and vice‑versa when two configured models share provider+model but differ indisplayName.Two small follow-ups worth considering:
- Extract a shared matcher (e.g.
matchesLanguageModel(configured, requested)) in@/features/chat/utilsand reuse it from both the chat route anduseSelectedLanguageModel, so MCP and the web client agree on what "same model" means.- The error message on line 70 omits
displayNameeven when the caller supplied one, which makes "configured but displayName mismatch" hard to diagnose — including it when present would help.♻️ Suggested error-message tweak
- if (!matchingModel) { - return { - statusCode: StatusCodes.BAD_REQUEST, - errorCode: ErrorCode.INVALID_REQUEST_BODY, - message: `Language model '${requestedLanguageModel.provider}/${requestedLanguageModel.model}' is not configured.`, - } satisfies ServiceError; - } + if (!matchingModel) { + const suffix = requestedLanguageModel.displayName + ? ` (displayName: '${requestedLanguageModel.displayName}')` + : ''; + return { + statusCode: StatusCodes.BAD_REQUEST, + errorCode: ErrorCode.INVALID_REQUEST_BODY, + message: `Language model '${requestedLanguageModel.provider}/${requestedLanguageModel.model}'${suffix} is not configured.`, + } satisfies ServiceError; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/mcp/askCodebase.ts` around lines 60 - 72, The current matching predicate used when finding matchingModel (checking provider+model and optional displayName) diverges from other call sites that use getLanguageModelKey and can cause inconsistent behavior; replace the inline predicate by extracting and reusing a single matcher (e.g. matchesLanguageModel(configured, requested)) from a shared utilities module (suggested name: matchesLanguageModel in features/chat/utils) and update configuredModels.find to use it, then update the chat route and useSelectedLanguageModel to call the same matcher so all paths agree on equality semantics; also enhance the error message constructed for missing matchingModel to include requestedLanguageModel.displayName when present so displayName mismatches are clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/web/src/features/mcp/askCodebase.ts`:
- Around line 60-72: The current matching predicate used when finding
matchingModel (checking provider+model and optional displayName) diverges from
other call sites that use getLanguageModelKey and can cause inconsistent
behavior; replace the inline predicate by extracting and reusing a single
matcher (e.g. matchesLanguageModel(configured, requested)) from a shared
utilities module (suggested name: matchesLanguageModel in features/chat/utils)
and update configuredModels.find to use it, then update the chat route and
useSelectedLanguageModel to call the same matcher so all paths agree on equality
semantics; also enhance the error message constructed for missing matchingModel
to include requestedLanguageModel.displayName when present so displayName
mismatches are clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1891842-b341-4835-aa56-856add137e4b
📒 Files selected for processing (1)
packages/web/src/features/mcp/askCodebase.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/web/src/features/mcp/askCodebase.ts (1)
73-87: Minor: extract the duplicatedavailableconstruction.The
availabledisplayName list is built identically in both thedisplayNameProvidedand the multi-candidate branches. A small helper would remove the duplication and keep the branches focused on message wording.♻️ Suggested refactor
- if (!matchingModel) { - let message: string; - if (candidates.length === 0) { - message = `Language model '${requestedLanguageModel.provider}/${requestedLanguageModel.model}' is not configured.`; - } else if (displayNameProvided) { - const available = candidates - .map((m) => m.displayName) - .filter((n): n is string => n !== undefined) - .map((n) => `"${n}"`) - .join(', '); - message = `Language model '${requestedLanguageModel.provider}/${requestedLanguageModel.model}' is configured but displayName '${requestedLanguageModel.displayName}' was not found.` - + (available ? ` Available: ${available}.` : ''); - } else { - const available = candidates - .map((m) => m.displayName) - .filter((n): n is string => n !== undefined) - .map((n) => `"${n}"`) - .join(', '); - message = `Multiple configurations found for '${requestedLanguageModel.provider}/${requestedLanguageModel.model}'. Provide a displayName to disambiguate.` - + (available ? ` Available: ${available}.` : ''); - } + if (!matchingModel) { + const modelId = `${requestedLanguageModel.provider}/${requestedLanguageModel.model}`; + const available = candidates + .map((m) => m.displayName) + .filter((n): n is string => n !== undefined) + .map((n) => `"${n}"`) + .join(', '); + const availableSuffix = available ? ` Available: ${available}.` : ''; + let message: string; + if (candidates.length === 0) { + message = `Language model '${modelId}' is not configured.`; + } else if (displayNameProvided) { + message = `Language model '${modelId}' is configured but displayName '${requestedLanguageModel.displayName}' was not found.${availableSuffix}`; + } else { + message = `Multiple configurations found for '${modelId}'. Provide a displayName to disambiguate.${availableSuffix}`; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/mcp/askCodebase.ts` around lines 73 - 87, Extract the duplicated logic that builds the available displayName list into a small helper function (e.g., buildAvailableList or formatCandidateDisplayNames) and call it from both branches instead of repeating the map/filter/map/join chain; update the code paths that currently compute `available` (the blocks that reference `candidates` and produce the quoted names used in the message for both the `displayNameProvided` and multi-candidate branches) to call that helper and keep the message construction lines unchanged except for using the helper's result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/features/mcp/askCodebase.ts`:
- Around line 80-87: The message shown when multiple configs match (constructed
around message, candidates, available, and requestedLanguageModel) is misleading
if all candidates have undefined displayName; update the logic that builds
available and message so that when available is empty you set a specific hint
like “Multiple configurations found for '<provider>/<model>' but none define a
displayName; add a unique displayName to each config to disambiguate” instead of
asking the caller to supply a displayName, otherwise keep the existing hint that
lists available displayNames when present.
---
Nitpick comments:
In `@packages/web/src/features/mcp/askCodebase.ts`:
- Around line 73-87: Extract the duplicated logic that builds the available
displayName list into a small helper function (e.g., buildAvailableList or
formatCandidateDisplayNames) and call it from both branches instead of repeating
the map/filter/map/join chain; update the code paths that currently compute
`available` (the blocks that reference `candidates` and produce the quoted names
used in the message for both the `displayNameProvided` and multi-candidate
branches) to call that helper and keep the message construction lines unchanged
except for using the helper's result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f391926-a0db-4522-91df-b0f6fa2218c2
📒 Files selected for processing (1)
packages/web/src/features/mcp/askCodebase.ts
…essages
The MCP package (@sourcebot/mcp) omits displayName from the ask_codebase
request schema (.omit({ displayName: true })), so LLM agents can only
pass {provider, model}. The previous getLanguageModelKey-based match
included displayName in the key, causing a permanent 400 for any explicit
model selection.
New matching logic:
- filter configured models by provider+model to collect candidates
- displayName check uses !== undefined (not truthiness) so "" is treated
as an explicit value, not absent
- if displayName provided: exact match within candidates — preserves
disambiguation between same-model entries with different displayName
(e.g. two opus-4-7 with different reasoningEffort)
- if displayName absent and exactly one candidate: use it
- if displayName absent and multiple candidates: return 400 with hint
Three distinct 400 messages:
- provider+model not found: "Language model 'X/Y' is not configured."
- displayName mismatch: "… is configured but displayName 'Z' was not
found. Available: "A", "B"."
- ambiguous: "Multiple configurations found for 'X/Y'. Provide a
displayName to disambiguate. Available: "A", "B"."
Available displayNames are quoted and filtered so undefined values never
appear in error messages.
Also removes the now-unused getLanguageModelKey import.
Fixes sourcebot-dev#1137
dmitrievav
force-pushed
the
fix/ask-codebase-language-model-match
branch
from
8f3ff73 to
37d8793
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/web/src/features/mcp/askCodebase.ts (1)
80-83:⚠️ Potential issue | 🟡 MinorMulti-candidate hint is still unactionable when no candidate has a
displayName.When two configs share
provider+modeland neither defines adisplayName,availableis empty and the message becomes"Multiple configurations found for '…'. Provide a displayName to disambiguate."— but the caller cannot, since none exist server-side. This branch should detect the empty-availablesub-case and instruct the operator to add uniquedisplayNames in config.🛠️ Suggested tweak
} else { - message = `Multiple configurations found for '${requestedLanguageModel.provider}/${requestedLanguageModel.model}'. Provide a displayName to disambiguate.` - + (available ? ` Available: ${available}.` : ''); + message = available + ? `Multiple configurations found for '${requestedLanguageModel.provider}/${requestedLanguageModel.model}'. Provide a displayName to disambiguate. Available: ${available}.` + : `Multiple configurations found for '${requestedLanguageModel.provider}/${requestedLanguageModel.model}' but none define a displayName; add a unique displayName to each config to disambiguate.`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/mcp/askCodebase.ts` around lines 80 - 83, The message for multiple matching configs (built where the code sets message for requestedLanguageModel.provider/model) must handle the case when available is empty; update the else branch that currently composes message = `Multiple configurations found for '${requestedLanguageModel.provider}/${requestedLanguageModel.model}'...` so that if available is falsy/empty it produces a clear operator-actionable string like "Multiple configurations found for '<provider>/<model>' but none expose a displayName — add unique displayName fields to your server-side configs to disambiguate." Otherwise keep the existing message that lists available displayNames; adjust the condition around the available variable used there (in askCodebase.ts) to choose the appropriate message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/features/mcp/askCodebase.ts`:
- Around line 60-67: The chat route currently compares models using
getLanguageModelKey(languageModel) which fails when displayName is undefined;
update the matching logic in the chat route (where configuredModels,
languageModel, and getLanguageModelKey are used) to mirror the MCP fix: filter
configuredModels by provider and model to produce candidates, check if
displayName is provided on languageModel, and if so find the candidate with the
same displayName, otherwise only accept a match when candidates.length === 1;
replace the current getLanguageModelKey equality check with this
displayName-aware candidate selection to avoid `"...-undefined"` mismatches.
---
Duplicate comments:
In `@packages/web/src/features/mcp/askCodebase.ts`:
- Around line 80-83: The message for multiple matching configs (built where the
code sets message for requestedLanguageModel.provider/model) must handle the
case when available is empty; update the else branch that currently composes
message = `Multiple configurations found for
'${requestedLanguageModel.provider}/${requestedLanguageModel.model}'...` so that
if available is falsy/empty it produces a clear operator-actionable string like
"Multiple configurations found for '<provider>/<model>' but none expose a
displayName — add unique displayName fields to your server-side configs to
disambiguate." Otherwise keep the existing message that lists available
displayNames; adjust the condition around the available variable used there (in
askCodebase.ts) to choose the appropriate message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26680aa1-457a-4d7f-8436-ef1970515593
📒 Files selected for processing (1)
packages/web/src/features/mcp/askCodebase.ts
Comment on lines
+60
to
+67
| const candidates = configuredModels.filter( | ||
| (m) => m.provider === requestedLanguageModel.provider && | ||
| m.model === requestedLanguageModel.model | ||
| ); | ||
| const displayNameProvided = requestedLanguageModel.displayName !== undefined; | ||
| const matchingModel = displayNameProvided | ||
| ? candidates.find((m) => m.displayName === requestedLanguageModel.displayName) | ||
| : candidates.length === 1 ? candidates[0] : undefined; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the chat route and its request schema to see if the same key-based
# match has the same displayName-undefined failure mode.
fd -t f 'route.ts' packages/web/src/app/api | xargs rg -nP -C5 'getLanguageModelKey'
rg -nP -C3 'languageModel\s*[:?]' packages/web/src/app/api/\(server\)/chat
rg -nP -C3 'displayName' packages/web/src/app/api/\(server\)/chatRepository: sourcebot-dev/sourcebot
Length of output: 4415
🏁 Script executed:
# First, let's understand what getLanguageModelKey does
rg -nP -A10 'function getLanguageModelKey|export.*getLanguageModelKey|const getLanguageModelKey' packages/web/src/features/chat/utils.tsRepository: sourcebot-dev/sourcebot
Length of output: 629
🏁 Script executed:
# Check the full schema definition for languageModel in the chat request
rg -nP -A20 'additionalChatRequestParamsSchema' packages/web/src/features/chat/types.tsRepository: sourcebot-dev/sourcebot
Length of output: 342
🏁 Script executed:
# Look at the languageModelInfoSchema to understand displayName handling
rg -nP -B5 -A15 'languageModelInfoSchema' packages/web/src/features/chat/types.tsRepository: sourcebot-dev/sourcebot
Length of output: 1251
🏁 Script executed:
# Compare the MCP request schema for askCodebase
rg -nP -B5 -A15 'languageModel' packages/web/src/features/mcp/types.ts | head -60Repository: sourcebot-dev/sourcebot
Length of output: 49
🏁 Script executed:
# Look at the actual MCP askCodebase.ts around lines 60-67 to see the fix
cat -n packages/web/src/features/mcp/askCodebase.ts | sed -n '50,80p'Repository: sourcebot-dev/sourcebot
Length of output: 2263
🏁 Script executed:
# Check if there are other routes/API handlers using getLanguageModelKey with similar pattern
rg -nP 'getLanguageModelKey' packages/web/src --type ts --type tsx -B3 -A3Repository: sourcebot-dev/sourcebot
Length of output: 95
🏁 Script executed:
# Look for any tests or comments explaining the bug that was fixed
rg -nP 'displayName|getLanguageModelKey' packages/web/src/features/chat/utils.ts -B5 -A5 | head -40Repository: sourcebot-dev/sourcebot
Length of output: 1070
Chat route has the same bug and needs the same fix.
The chat route at packages/web/src/app/api/(server)/chat/route.ts:70 uses getLanguageModelKey(model) === getLanguageModelKey(languageModel) which breaks when displayName is undefined and multiple configured models share the same provider/model. Since getLanguageModelKey includes displayName in the key (${provider}-${model}-${displayName}), an undefined value creates a key like "provider-model-undefined" that never matches any configured model, causing a 400 error.
The MCP fix at lines 64-67 explicitly handles this by checking if displayName is provided and only accepting a single candidate when it's not. The chat route should be updated to use the same logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/mcp/askCodebase.ts` around lines 60 - 67, The chat
route currently compares models using getLanguageModelKey(languageModel) which
fails when displayName is undefined; update the matching logic in the chat route
(where configuredModels, languageModel, and getLanguageModelKey are used) to
mirror the MCP fix: filter configuredModels by provider and model to produce
candidates, check if displayName is provided on languageModel, and if so find
the candidate with the same displayName, otherwise only accept a match when
candidates.length === 1; replace the current getLanguageModelKey equality check
with this displayName-aware candidate selection to avoid `"...-undefined"`
mismatches.
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.
Problem
ask_codebaserejects any explicitlanguageModelwith a 400, even whenlist_language_modelsreturns the exact model. Reported in #1137.Root cause:
askCodebase.tsusesgetLanguageModelKeyto match the requested model against configured ones. That key includesdisplayName:But the MCP package (
@sourcebot/mcp) explicitly omitsdisplayNamefrom theask_codebaserequest schema:So an LLM agent can only pass
{provider, model}—displayNameis alwaysundefinedon the caller's side. Match always fails:anthropic-claude-opus-4-7-Claude Opus 4.7 (slow, highest quality)anthropic-claude-opus-4-7-undefinedFix
Replace the
getLanguageModelKey-based match with a two-step approach:provider + model→candidates.displayName !== undefined(strict, not truthiness — so""is treated as an explicit value).displayNameprovided → exact match within candidates. Preserves disambiguation between same-model entries differing indisplayName(e.g. twoopus-4-7with differentreasoningEffort).displayNameabsent and exactly one candidate → use it.displayNameabsent and multiple candidates → 400 with one of two distinct messages:displayName→ list them and ask caller to disambiguatedisplayName→ tell the operator to adddisplayNameto the configThree distinct 400 messages:
provider+modelnot foundLanguage model 'X/Y' is not configured.provider+modelfound, wrongdisplayName… is configured but displayName 'Z' was not found. Available: "A", "B".displayName, some havedisplayNameMultiple configurations found for 'X/Y'. Provide a displayName to disambiguate. Available: "A", "B".displayNameMultiple configurations found for 'X/Y' but none define a displayName. Add a unique displayName to each configuration entry to enable disambiguation.Also removes the now-unused
getLanguageModelKeyimport.Test plan
{provider, model}, one matching config, nodisplayName→ succeeds{provider, model, displayName: ""}→ treated as explicit empty-string match{provider, model, displayName}→ exact match on all three fields{provider, model, displayName}wrong name → 400 listing available displayNamesprovider+model, differentdisplayName, nodisplayNamesent → 400 with hint listing available namesprovider+model, differentdisplayName, correctdisplayNamesent → correct model selectedprovider+model, neither hasdisplayName→ 400 telling operator to adddisplayNameto configprovider+modelnot configured at all → plain "not configured" messagelanguageModelparam → falls back to first configured model (unchanged)