Resolve execution subagent model by exact CAPI id before family fallback#324859
Draft
piyushmadan wants to merge 1 commit into
Draft
Resolve execution subagent model by exact CAPI id before family fallback#324859piyushmadan wants to merge 1 commit into
piyushmadan wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a model-routing bug for the github.copilot.chat.executionSubagent.model setting. Previously the subagent model was resolved only by CAPI capabilities.family, which always selects the first model in a family (in /models order), so a specific model id sharing a family with others could never be targeted (breaking MAI routing such as mai-code-1-flash-picker). It adds an exact-model-id resolution path that runs before the existing family fallback.
Changes:
- Adds
getChatModelFromCapiId(id)toIModelMetadataFetcher/ModelMetadataFetcher(chat-model-only,undefinedon miss) andgetChatEndpointById(id)toIEndpointProvider/ProductionEndpointProvider. - Updates
ExecutionSubagentToolCallingLoop.getEndpointto try exact-id resolution first, then family resolution, then the main-agent endpoint, with trace logs on fallback (agentic-proxy path untouched). - Adds unit tests (
modelMetadataFetcher.spec.ts,endpointProviderImpl.spec.ts) and updates all mock/null endpoint-provider and model-fetcher implementations to satisfy the widened interfaces.
Show a summary per file
| File | Description |
|---|---|
platform/endpoint/node/modelMetadataFetcher.ts |
Adds getChatModelFromCapiId (interface + impl) iterating _familyMap for an exact chat-model id match |
platform/endpoint/common/endpointProvider.ts |
Adds getChatEndpointById to IEndpointProvider with JSDoc |
extension/prompt/vscode-node/endpointProviderImpl.ts |
Implements getChatEndpointById via the fetcher, wrapping with getOrCreateChatEndpointInstance |
extension/prompt/node/executionSubagentToolCallingLoop.ts |
Exact-id-first resolution in getEndpoint, with tool-call-support and error fallbacks + trace logs |
platform/endpoint/test/node/modelMetadataFetcher.spec.ts |
New tests: exact-id wins over first-in-family, family unchanged, unknown id → undefined |
extension/prompt/vscode-node/test/endpointProviderImpl.spec.ts |
New tests for getChatEndpointById id resolution / miss and family fallback |
platform/endpoint/test/node/testEndpointProvider.ts |
Implements getChatEndpointById against existing metadata flow |
platform/endpoint/test/node/copilotChatEndpoint.spec.ts |
Fake fetcher implements getChatModelFromCapiId |
extension/test/vscode-node/endpoints.test.ts |
Two fake fetchers implement getChatModelFromCapiId |
lib/node/chatLibMain.ts |
NullEndpointProvider.getChatEndpointById returns undefined |
lib/vscode-node/test/getInlineCompletions.spec.ts, extension/tools/node/test/searchToolTestUtils.ts, extension/intents/node/test/searchSubagentGating.spec.ts, extension/prompts/node/panel/test/fileVariable.spec.ts, extension/prompts/node/base/test/promptRenderer.spec.ts, extension/prompt/node/test/feedbackGenerator.spec.ts, extension/chatSessions/claude/node/test/claudeCodeModels.spec.ts |
Mock providers updated to satisfy the widened IEndpointProvider interface |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Medium
Comment on lines
+135
to
+143
| const byId = await this.endpointProvider.getChatEndpointById(modelName); | ||
| if (byId) { | ||
| if (byId.supportsToolCalls) { | ||
| return byId; | ||
| } | ||
| // Resolved model does not support tool calls, fallback to main agent endpoint | ||
| this._logService.trace(`[ExecutionSubagent] Model '${modelName}' does not support tool calls; falling back to main agent endpoint.`); | ||
| return await this.endpointProvider.getChatEndpoint(this.options.request); | ||
| } |
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
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.
Summary
The
github.copilot.chat.executionSubagent.modelsetting was resolved only by CAPIcapabilities.family, never by exact modelid. When multiple model ids share a single family, family-only resolution always selects the first model in that family (in/modelsresponse order), so a specific id could not be targeted.This caused MAI routing failures:
executionSubagent.model: mai-code-1-flash-pickerdid not match a family key, so it silently fell back to the main agent model.executionSubagent.model: oswe-vscode-modelDresolved the family, but always selected the first entry (lark-debug-picker), not the intended MAI model.This change adds exact model-id resolution first, keeping the existing family resolution as a fallback.
Changes
modelMetadataFetcher.ts: addedgetChatModelFromCapiId(id)toIModelMetadataFetcherandModelMetadataFetcher. It uses the same fetch guard as the other methods, iterates the family map, and returns the first chat model whoseidmatches exactly (orundefinedon miss).getChatModelFromCapiFamilyis unchanged.endpointProvider.ts: addedgetChatEndpointById(id): Promise<IChatEndpoint | undefined>toIEndpointProvider.endpointProviderImpl.ts:ProductionEndpointProvider.getChatEndpointByIdresolves viagetChatModelFromCapiIdand wraps withgetOrCreateChatEndpointInstance, returningundefinedon miss.getChatEndpoint(...)is unchanged.testEndpointProvider.ts: implementedgetChatEndpointByIdagainst its existing model metadata flow.executionSubagentToolCallingLoop.ts: the non-proxy path now attempts exact-id resolution first, then falls back to family resolution, then to the main agent endpoint (with trace logs on fallback). The agentic-proxy path is untouched.IEndpointProvider/IModelMetadataFetcherimplementations to satisfy the widened interfaces.Tests
modelMetadataFetcher.spec.ts(new): exact-id resolution wins over the first-in-family model; family resolution still selects the first member; unknown id returnsundefined.endpointProviderImpl.spec.ts(new):getChatEndpointByIdresolves the exact id and returnsundefinedon miss; family fallback viagetChatEndpointunchanged.copilotChatEndpoint.spec.tsfake to implement the new method.Validation
npx tsgo --noEmitpasses for bothtsconfig.jsonandtsconfig.worker.json.Non-goals
getChatModelFromCapiFamilybehavior.