Fix custom/BYOK model selection when spawning subagents - #333288
Fix custom/BYOK model selection when spawning subagents#333288And (logical-and) wants to merge 4 commits into
Conversation
The subagent (task) tool could not be pinned to a custom/BYOK model on
either subagent surface.
Classic workbench (runSubagentTool): the `model` parameter only accepted
a qualified name ("Model Name (Vendor)"). Every model configured under a
custom endpoint shares one vendor, so their qualified names collide and a
request could resolve to a sibling model — or fail. Accept the model
identifier as well, and advertise the unambiguous identifier for models
whose qualified name is not unique.
Agents window (agent-host BYOK bridge): the Copilot CLI runtime matches a
requested subagent model against the session's model list by folding the
candidate ids to lower case but not the requested id. Any selection id
carrying an upper-case character — typically a configured provider group
such as `Tokengate` — could therefore never match itself, and was
reported as unavailable by an error that listed it as available. Advertise
the selection id case-folded so both sides of that comparison agree, and
resolve ids case-insensitively on the way back so the bridge still routes
to the source model. Model visibility keyed on the pre-folding id is
honoured for both forms to avoid disturbing existing sessions.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds unambiguous custom/BYOK model selection for subagents across the workbench and Agents window.
Changes:
- Resolves subagent models by qualified name or identifier.
- Case-folds agent-host BYOK selection IDs and restores source-model routing.
- Adds regression tests for both paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
runSubagentTool.ts |
Supports identifier-based model selection. |
runSubagentTool.test.ts |
Tests custom-model resolution and collisions. |
agentHostByokLm.ts |
Adds folded and legacy BYOK ID helpers. |
agentHostByokLmHandler.ts |
Resolves folded IDs and preserves visibility state. |
agentHostByokLmHandler.test.ts |
Tests folded-ID routing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Follow-up to the PR review comments: - Fold the full agent-facing id (vendor included), not just the provider-local suffix, and match the vendor case-insensitively on the bridge, so an upper-case vendor cannot re-introduce the self-mismatch. - Detect genuine post-fold selection-id collisions (two distinct models differing only in case) in the session launcher and warn instead of silently dropping one without explanation. - In the subagent "available models" error, advertise a qualified name only when it is unique among agent-mode models AND resolves back to that identifier, since lookupLanguageModelByQualifiedName scans the whole cache and a same-named sibling could otherwise win. - Update the existing copilotSessionLauncher/copilotAgent node tests to the folded ids they now emit, and rework the subagent precedence test so it actually exercises identifier-before-qualified-name resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the review, all five were valid. Addressed in 4769b91:
Re-ran after the changes: agentHost node suite 5056 passing / 53 pending, handler suite 14, subagent-tool suite 39, On point 2 there's a real tradeoff worth surfacing: I chose collision detection over a lowercase-safe encoding because an encoding would change the ids people type into the Worth repeating that this is still a workaround. The underlying issue is a case-sensitivity asymmetry in the closed-source |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
And yeah: |
Problem
You can't pin a subagent (the built-in
task/runSubagenttool) to a custom or BYOK model. It's broken on both subagent surfaces, for two unrelated reasons.Classic workbench. The
modelparameter only accepted a display-style qualified name,"Model Name (Vendor)". Every model configured under a single custom endpoint shares one vendor, so those qualified names collide. A request could quietly resolve to a sibling model, or not resolve at all.Agents window (agent-host BYOK bridge). Spawning a subagent on a custom model fails with an error that contradicts itself, listing the requested model as available:
Root cause
In the classic workbench, resolution went only through
lookupLanguageModelByQualifiedName, which is ambiguous for BYOK models by construction.The Agents window case is more interesting. The model id advertised to the runtime keeps the configured provider group verbatim (
Acmeabove), so the selection id carries an uppercase character. The runtime's subagent-model validator lowercases the candidate ids but not the requested id, so a model like that can never match itself. Hence "not available" for something the same message lists as available.Fix
runSubagentToolnow accepts either the qualified name or the model identifier (<vendor>/<group>/<id>), and advertises the unambiguous identifier for any model whose qualified name isn't safe to use. A name is only advertised when it's unique within the agent-mode list andlookupLanguageModelByQualifiedNameresolves it back to that same identifier, since that lookup scans the whole model cache and a same-named model outside the list could otherwise win. Both the explicit-modelsite and the agent-pinned resolution loop go through this.The agent-host BYOK bridge advertises the selection id case-folded so both sides of the runtime's comparison agree, and resolves ids case-insensitively on the way back so the bridge still routes to the source model. The whole
vendor/[group/]idis folded, not just the suffix, because the vendor is part of the string the runtime compares. Model-visibility state keyed on the pre-folding id is honoured for both forms, so existing sessions aren't disturbed.Folding isn't injective, so two models differing only in case now collapse to one selection id. Rather than let one silently vanish,
resolveByokSessionConfigdetects that collision and logs a warning naming both ids while keeping the first. I preferred this over a lowercase-safe encoding because an encoding changes the ids people type into themodelparameter, which is the usability problem this PR set out to fix. Happy to switch if reviewers would rather have the hard uniqueness guarantee.Testing
Unit tests added and updated for both surfaces, including collision handling and identifier-before-name precedence. The
agentHostnode suite is at 5056 passing / 53 pending, the BYOK handler suite at 14, and therunSubagentToolsuite at 39.typecheck-clientis clean.Also verified end to end in the Agents window against a real custom endpoint: a fresh session spawns a subagent pinned to the previously-failing model and runs to completion, with the AHP protocol log showing real inference (
usage.modelis the pinned model) and no rejections.Note
The Agents-window half is a workaround. The actual bug is the case-sensitivity asymmetry in the closed-source Copilot CLI runtime's subagent-model validator, which lowercases the candidate list but not the requested id. This PR makes the id VS Code advertises match what that validator can resolve, but it'd be better to fix the comparison in the runtime, at which point the folding here can be removed.