Surface sandbox model fallbacks and honour confirmation editability - #334133
Merged
Osvaldo Ortega (osortega) merged 3 commits intoSep 2, 2026
Merged
Conversation
Three follow-ups to the sandbox model and approval work in #333897 and #333883, all found while re-reviewing those changes against the agent host protocol. Report when a sandbox turn does not use the model the user picked. The three give-up paths logged and sent the turn with no model, which the protocol reads as "the host decides" - so the turn ran at a capability and price the user had not chosen and nothing said so. Keep a host's context-tier option instead of dropping it when the workbench cannot label it. Partially reverts #333897: the token-count labels paired two independent catalogues by array position, which mislabels as soon as either list changes, and dropping the property outright hid a control the session supports whenever the model was unknown to the workbench. The host's own labels are used instead. Honour editability on tool confirmation inputs. The confirmation editor writes back into rawInput, which ILanguageModelToolsService applies for extension tools, but an agent-host confirmation never returns it - so a user could edit a command they were approving and watch the original run. Adds an optional editable flag, defaulting to editable so existing tools are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Osvaldo Ortega (osortega)
September 2, 2026 22:23
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Terminal confirmations can still accept discarded edits, potentially executing the original command.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts — The read-only safeguard only covers the generic-input branch; terminal confirmations built… |
 |
src/vs/sessions/contrib/providers/copilotChatSessions/browser/copilotChatSessionsProvider.ts — This warning is emitted before sendRequest, so “was used” is false when dispatch subsequently… |
What changed in this PR
Surfaces sandbox model fallbacks, preserves host-provided context tiers, and adds read-only controls for agent-host confirmations.
Changes:
- Warns when sandbox model selection falls back.
- Uses host-owned context-tier labels.
- Adds confirmation input editability controls.
| File | Review |
|---|---|
src/vs/workbench/contrib/chat/test/browser/agentSessions/stateToProgressAdapter.test.ts |
Tests read-only generic confirmations. |
src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostLanguageModelProvider.test.ts |
Tests host context-tier metadata. |
src/vs/workbench/contrib/chat/common/chatService/chatService.ts |
Adds the input editability contract. |
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolConfirmationSubPart.ts |
Honors generic input editability. |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts |
Critical: Terminal confirmations remain editable although edited commands are not dispatched. |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostLanguageModelProvider.ts |
Preserves host configuration properties. |
src/vs/sessions/contrib/providers/copilotChatSessions/test/browser/copilotChatSessionsProvider.test.ts |
Tests model fallback notifications. |
src/vs/sessions/contrib/providers/copilotChatSessions/browser/copilotChatSessionsProvider.ts |
Nit: Warning claims a fallback model “was used” before dispatch succeeds. |
src/vs/platform/agentHost/common/state/sessionReducers.ts |
Clarifies tool-kind fallback behavior. |
src/vs/platform/agentHost/common/meta/agentPermissionRequestMeta.ts |
Documents compatibility metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The comment claimed the join could be removed once the agent host publishes its own token counts and billing metadata. The billing half is not coming: pricing is deliberately kept off the protocol as operator-sensitive, so removing the join means dropping pricing for these models rather than tidying up after a migration. Separates the two halves: token limits are native protocol fields a host can populate, pricing is a product decision. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The read-only safeguard only covered the generic-input branch. A terminal confirmation renders its own editor, writes edits into commandLine.userEdited, and only the built-in terminal tool reads that back when it runs the command. An agent-host confirmation returns nothing, so the edit was discarded and the original command ran - the same bug as the generic input, on the path that actually carries shell commands. Adds editable to the terminal invocation data, sets it false in the agent-host adapter, and honours it alongside the existing presentationOverrides read-only case. Also states the model fallback in the future tense: the warning is raised before the turn is dispatched, so the fallback has not been used yet at that point. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Osvaldo Ortega (osortega)
marked this pull request as ready for review
September 2, 2026 23:31
Osvaldo Ortega (osortega)
enabled auto-merge (squash)
September 2, 2026 23:31
Dmitriy Vasyura (dmitrivMS)
approved these changes
Sep 2, 2026
Osvaldo Ortega (osortega)
deleted the
osortega/sandbox-model-selection-and-confirmation-fixes
branch
September 2, 2026 23:45
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.
Three follow-ups to #333897 and #333883, found while re-reviewing both against the agent host protocol. They are independent and can be reviewed in any order.
Important
Part of this intentionally reverts #333897 — see "2. Context tier" below. That deletion is the point of the change, not an accident. Flagging it so it isn't reported as a regression.
1. A sandbox turn silently ran on a model the user did not pick
_carryModelToSandboxhas three paths that give up on the user's selection: no model target, the model isn't advertised, or the catalog doesn't arrive within the wait. All three logged and then dispatched the turn with no model at all.An absent
modelon the message means the agent decides — for a session that has never run, that is its default. So the turn ran at a capability and a price the user hadn't chosen, and because that is a legitimate protocol state, nothing downstream had any reason to report it.Each path now tells the user which model was dropped. Still a fallback rather than a hard failure: a sandbox has already been provisioned by this point, and failing the send would strand it.
2. Context tier was hidden whenever the workbench couldn't decorate it
This partially reverts #333897. That change labelled a host's
contextTiervalues with token counts (264K/1M) taken from the workbench's own model catalogue, and dropped the property entirely when it couldn't find them.Two problems:
The host's own labels (
Default/Long context) are now used. This is a deliberate downgrade in label quality — real token counts should come from the host describing its own tiers, not from a client-side join.3. Confirmations invited edits that were thrown away
A tool confirmation renders its input in an editor and writes changes back. Two separate paths do this, and both are affected:
rawInputis written back, andILanguageModelToolsServiceassigns it to the tool's parameters after confirmation.commandLine.userEdited, which only the built-in terminal tool reads when it runs the command.Agent-host confirmations reach neither. They confirm through the protocol, which does define a field for an edited input — but nothing in the workbench populates it. So a user could edit a shell command they were approving and watch the original one run.
Adds an optional
editableflag to both invocation-data shapes:false, and the write-back handler isn't attached at all.The terminal case sits alongside the existing
presentationOverridesread-only rule rather than replacing it.The adapter hardcodes
falserather than forwarding the protocol's owneditable, because forwarding atruewould recreate the exact bug until the edited-input round trip is implemented. Noted in a comment and pinned by tests.Testing
npm run typecheck-clientclean. 3,765 chat tests pass.New: two tests for the unavailable-model and timeout notifications; two for context-tier labelling and retention; one asserting a generic confirmation is read-only, and one for the terminal confirmation.
One existing assertion in
stateToProgressAdapter.test.tsupdated for the neweditable: false— it failed first, which is the test doing its job.Not verified
No UI run. The read-only editors and the notification are covered by unit tests and by reading the render paths, not by observation. The 5-second model wait is unchanged and still unvalidated — this only makes its expiry visible.
Not included