sessions: show the approvals picker for agent hosts without an approvals config property - #333926
Draft
Osvaldo Ortega (osortega) wants to merge 3 commits into
Draft
sessions: show the approvals picker for agent hosts without an approvals config property#333926Osvaldo Ortega (osortega) wants to merge 3 commits into
Osvaldo Ortega (osortega) wants to merge 3 commits into
Conversation
…als config property The session-level approvals picker requires the session's config schema to declare a well-known `autoApprove` string enum. A host that drives approvals some other way therefore had no picker at all — the chip silently never rendered, even though a mode chip beside it did. Add a fallback for those hosts. When the resolved schema declares no approvals property but the connection can issue the Copilot approve-all extension, back the picker with that instead and offer the two levels a boolean can express (Manual permissions / Allow all). Hosts that do declare an approvals property are untouched and keep their full level set. The extension's value is not reported back in session state, so the applied value is recorded locally and re-applied when a session's state subscription is established — a host that restarts or resumes comes back with it cleared. Only values the host confirms are recorded, so a rejected call leaves the picker showing what is actually in effect rather than what was requested. Failing to widen approvals is the safe direction: the user keeps being asked. Two ordering hazards are guarded explicitly. The persisted record is read-modify-write against storage rather than cached, because every agent-host provider in the window shares one profile-scoped key and a cached copy would let the last writer drop the others' entries. And a toggle already on the wire suppresses the re-apply, so a subscription established mid-flight cannot re-send a stale `true` after the user has just turned approvals off. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Osvaldo Ortega (osortega)
September 2, 2026 01:42
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Local forwarding, schema selection, lifecycle restoration, policy handling, rollback, and request ordering contain unresolved defects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 2
New issues introduced by this change (5)
| Severity | Finding |
|---|---|
 |
src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts — The set tracks only membership; it neither serializes multiple quick user selections nor includes… |
|
src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts — A remembered true is replayed without checking whether enterprise policy now forbids elevated… |
 |
src/vs/platform/agentHost/common/agentService.ts — Because this method is optional, LocalAgentHostServiceClient still satisfies IAgentHostService… |
|
src/vs/sessions/contrib/providers/agentHost/browser/agentHostPermissionPickerDelegate.ts — PermissionPicker._selectLevel updates its trigger optimistically before invoking this void… |
|
src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts — This executes when the provider acquires its local reference, not when the wire subscription is… |
What changed in this PR
Adds a two-level approval picker fallback for Agent Hosts without an approvals configuration property.
Changes:
- Adds the approve-all extension request and provider APIs.
- Persists and reapplies confirmed approve-all selections.
- Adds fallback, persistence, rejection, and provider-isolation tests.
| File | Description |
|---|---|
src/vs/platform/agentHost/common/agentService.ts |
Extends the connection contract. |
src/vs/platform/agentHost/common/agentHostExtensionProtocol.ts |
Defines the extension request. |
src/vs/platform/agentHost/browser/agentHostProtocolClient.ts |
Sends approve-all requests. |
src/vs/sessions/common/agentHostSessionsProvider.ts |
Exposes provider approval operations. |
src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts |
Persists and reapplies approval state. |
src/vs/sessions/contrib/providers/agentHost/browser/agentHostPermissionPickerDelegate.ts |
Selects and drives the fallback picker. |
src/vs/sessions/contrib/providers/agentHost/test/browser/localAgentHostSessionsProvider.test.ts |
Tests persistence and provider isolation. |
src/vs/sessions/contrib/providers/agentHost/test/browser/agentHost/agentHostPermissionPickerDelegate.test.ts |
Tests fallback picker behavior. |
Suppressed comments (1)
src/vs/sessions/contrib/providers/agentHost/browser/agentHostPermissionPickerDelegate.ts:280
- This also selects the fallback when an
autoApproveproperty exists but has a noncanonical schema. That property still renders through the generic config picker (agentHostSessionConfigPicker.ts:499-503), so such hosts get two approval pickers backed by different mechanisms. The fallback is specifically for an absent property; leave every declared property on the existing config path.
return !schema || !isWellKnownAutoApproveSchema(schema);
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+3855
to
+3859
| this._approveAllInFlight.add(sessionId); | ||
| try { | ||
| await connection.setSessionApproveAll(cached.backendUri, enabled); | ||
| } finally { | ||
| this._approveAllInFlight.delete(sessionId); |
Comment on lines
+3878
to
+3882
| if (!connection.setSessionApproveAll || this._approveAllInFlight.has(sessionId) || !this.getSessionApproveAll(sessionId)) { | ||
| return; | ||
| } | ||
| connection.setSessionApproveAll(sessionUri, true).catch(err => { | ||
| this._logService.warn(`[${this.id}] Failed to re-apply approve-all for ${sessionId}: ${err}`); |
| * method — a connection that cannot serve it simply omits it, and callers | ||
| * fall back to the session-config approvals path. | ||
| */ | ||
| setSessionApproveAll?(session: URI, enabled: boolean): Promise<void>; |
Comment on lines
+198
to
+199
| provider.setSessionApproveAll(session.sessionId, level === ChatPermissionLevel.AutoApprove) | ||
| .catch(err => this._logService.warn(`[AgentHostPermissionPicker] Failed to set approve-all: ${err}`)); |
|
|
||
| this._hydrateAgentFromDraft(connection, cached, sessionId, sessionUri, store); | ||
| this._hydrateModelFromDraft(connection, cached, sessionId, sessionUri, store); | ||
| this._reassertSessionApproveAll(connection, sessionId, sessionUri); |
The fallback keyed only off the absence of a well-known `autoApprove` property. Claude omits that property deliberately — its schema comment notes that workbench pickers key off property names, so leaving it out suppresses the default UI — which meant a remote Claude or Codex session was read as "no approvals axis" and claimed by the fallback. That stacked a second approvals chip beside the agent's own picker and would have sent a Copilot-shaped request to an agent that never asked for one. Gate on the Copilot agent instead. The request is a Copilot host extension, so that is exactly the set of sessions where it means anything, and gating positively fails closed: an agent this client has never heard of now gets no picker rather than the wrong one. The connection and schema checks still apply on top, so the in-process CLI session (which shares the `copilotcli` agent name but whose connection cannot issue the request) and Copilot on hosts that do declare approvals both stay on the config path with their full level set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ll writes Fixes the approvals picker not appearing on a real cloud sandbox session. The agent gate only accepted `copilotcli`, the provider id used by the agent host bundled with VS Code. The Copilot host advertises its agent as `copilot`, so a sandbox session type (`remote-<authority>-copilot`) failed the gate and the picker stayed hidden. Accept both ids, and pin an observed sandbox session type in a test so the two spellings cannot drift apart again. Also addresses three review findings on the write path: Enterprise policy is now enforced here as it already is on the session-config path. Enabling approve-all is refused outright under a restricting policy, and a value remembered from an earlier run is dropped rather than replayed when policy has since turned elevated approvals off — previously the replay was a way around a restriction the rest of the UI honours. Writes for a session are serialized and only the newest one's result is kept, so toggling quickly settles on what the user asked for last rather than on whichever request happened to resolve last. Previously two in-flight writes could resolve out of order, and an early completion cleared the in-flight marker while another write was still pending. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
The problem
The session-level approvals picker only renders when the session's config schema declares a well-known
autoApprovestring enum. Any agent host that drives approvals a different way gets no picker at all — the chip silently never appears.That is what this looks like today: the mode chip renders, and the approvals chip that should sit beside it is simply absent. There is no error and no fallback, so the session runs on whatever approval behavior the host defaults to and the user has no way to see or change it.
What this changes
Adds a fallback path for hosts with no approvals property in their session config.
The picker is backed by that request when three things hold: the session runs the Copilot agent, the connection can issue the request, and the resolved schema declares no approvals property. Because the request carries a boolean, the picker then offers the two levels a boolean can express:
Everything else is untouched. Hosts that declare an approvals property keep the existing config path and their full three-level set including Assisted; agents with their own approvals axis (Claude, Codex) keep their dedicated pickers; and the in-process CLI session, which shares the Copilot agent name but whose connection cannot issue the request, also stays on the config path. Gating on the agent positively means an agent this client has never heard of gets no picker rather than the wrong one. Tests cover each of these.
Why the extra state handling
The extension's resulting value is not reported back in session state, so a client that wants to display it has to remember what it applied. Three consequences, each guarded:
Only confirmed values are recorded. The record is written after the host accepts, never optimistically. A rejected call leaves the picker showing what is actually in effect rather than what was requested. This fails in the safe direction — a dropped enable means the user keeps being asked.
The value is re-applied when a session's state subscription is established. A host that restarts, or a remote environment that suspends and resumes, comes back with the flag cleared while the local record still says otherwise. Re-applying on subscribe keeps the two in step.
Two ordering hazards are guarded explicitly:
trueafter the user'sfalse, leaving a session auto-approving right after they turned it off.Reviewing
The load-bearing code is the set of guards in
_usesApproveAll. The!configcheck matters because an unresolved schema must not be mistaken for "this host has no approvals property", or the picker would briefly offer two levels to a host that turns out to offer three. The agent check matters because an agent that omitsautoApprovemay be doing so deliberately to suppress the default UI, which is not the same as having no approvals axis.canRequestSessionApproveAll()is named for what it checks. It confirms the connection implements the optional method; it does not prove the host handles it, because this vendor extension is not advertised in the host'sinitializecapabilities the way our own extensions are gated. Until it is, the schema check is the real discriminator. A host with neither would surface a picker whose writes fail — again in the safe direction.Tests
15 new tests. Notably: the value the host confirmed vs. the one requested, the host-rejects path, surviving a reload and being re-applied on resubscribe, one provider not dropping another's record, hosts that declare an approvals property staying on the existing path, agents with their own approvals axis being left alone, and the agent name alone not being sufficient to claim a session.
Verified:
typecheck-client,valid-layers-check,hygiene,vs/sessions(2915 passing),AgentHost(2694 passing).Not verified: I have not exercised this against a live remote host — no observation of the request succeeding over a real connection, of the re-apply firing after a genuine suspend/resume, or of the rendered two-level picker. Worth a manual pass before this leaves draft.
Known limits