evals: attach an existing Agent Host session - #333454
evals: attach an existing Agent Host session#333454Ross Wollman (rwoll) wants to merge 1 commit into
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Robo (@deepak1556)Matched files:
|
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 3
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
src/vs/sessions/contrib/providers/remoteAgentHost/electron-browser/evaluationSessionAttachment.contribution.ts — This resolves services through IInstantiationService.invokeFunction as a service locator. VS Code… |
|
src/vs/sessions/contrib/providers/remoteAgentHost/electron-browser/evaluationSessionAttachment.contribution.ts — This callback displays every attachment failure in the notification UI, but the errors created by… |
|
src/vs/sessions/contrib/providers/remoteAgentHost/browser/evaluationSessionAttachment.ts — These Pick-based service facades create production-only subset interfaces to simplify the test… |
What changed in this PR
Adds a private CLI flow for attaching an Agents window to an existing remote Agent Host evaluation session.
Changes:
- Routes the new argument to an Agents window and validates attachment identity.
- Coordinates tool publication and external approval ownership.
- Suppresses local draft synchronization and adds focused coverage.
| File | Description |
|---|---|
src/vs/code/electron-main/app.ts |
Routes initial attachment launches. |
src/vs/platform/launch/electron-main/launchMainService.ts |
Routes forwarded attachment launches. |
src/vs/platform/environment/common/argv.ts |
Defines the argument and routing helper. |
src/vs/platform/environment/node/argv.ts |
Registers CLI parsing. |
src/vs/platform/environment/test/common/argv.test.ts |
Tests CLI routing. |
src/vs/sessions/sessions.desktop.main.ts |
Loads the attachment contribution. |
src/vs/sessions/contrib/providers/remoteAgentHost/electron-browser/evaluationSessionAttachment.contribution.ts |
Starts attachment during restoration. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/evaluationSessionAttachment.ts |
Implements validation and lifecycle. |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/evaluationSessionAttachmentService.ts |
Tracks attachment identity and readiness. |
src/vs/sessions/contrib/providers/agentHost/browser/baseAgentHostSessionsProvider.ts |
Adds a publication barrier hook. |
src/vs/sessions/contrib/providers/remoteAgentHost/browser/remoteAgentHostSessionsProvider.ts |
Gates remote active-client publication. |
src/vs/workbench/contrib/chat/browser/tools/clientToolSetsContribution.ts |
Exposes tool-set reconciliation. |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionHandler.ts |
Defers confirmations and draft publication. |
src/vs/sessions/contrib/providers/remoteAgentHost/test/electron-browser/evaluationSessionAttachment.test.ts |
Covers attachment lifecycle and identity. |
src/vs/sessions/contrib/providers/remoteAgentHost/test/browser/remoteAgentHostSessionsProvider.test.ts |
Covers publication gating and invalidation. |
src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostClientTools.test.ts |
Covers inventory and tool ownership. |
src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostChatContribution.test.ts |
Covers draft suppression. |
src/vs/platform/agentHost/test/node/agentSideEffects.test.ts |
Verifies canonical approved execution requests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return instantiationService.invokeFunction((accessor: ServicesAccessor) => ({ | ||
| sessionsManagementService: accessor.get(ISessionsManagementService), | ||
| sessionsService: accessor.get(ISessionsService), | ||
| connectionsService: accessor.get(IAgentHostConnectionsService), | ||
| attachmentService: accessor.get(IEvaluationSessionAttachmentService), |
There was a problem hiding this comment.
This lazy resolution is intentional. The contribution returns before invoking getStartupServices when the private argument is absent, which keeps the no-flag path from resolving or touching the session/connection services. invokeFunction is used here as a narrow, flag-gated lifetime boundary rather than as a general service locator.
| readonly sessionsManagementService: Pick<ISessionsManagementService, 'getSession' | 'onDidChangeSessions'>; | ||
| readonly sessionsService: Pick<ISessionsService, 'canOpenSession' | 'openSession'> & { readonly activeSession: IObservable<ISession | undefined>; readonly initialRestoreComplete: IObservable<boolean> }; | ||
| readonly connectionsService: Pick<IAgentHostConnectionsService, 'connections' | 'resolveSessionResource'>; |
There was a problem hiding this comment.
This interface is a functional dependency bundle for the lifecycle helper, not a registered DI service. Production supplies the canonical services; the Pick types make the helper’s actual dependencies explicit and keep its unit tests focused. This pattern is also used elsewhere in the codebase, so I am keeping the narrower contract.
6e6880c to
2315fba
Compare
2315fba to
ed1847e
Compare
ed1847e to
ef555c7
Compare
Example:
code-insiders --attach-to-evaluation-session \
'remote-example-provider:/session-id'
The private argument opens the exact session in an Agents window while the external driver retains turn and tool-approval ownership.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ef555c7 to
da3e36e
Compare
Critical issues🤖 [P1] Attached sessions remain writable. 🤖 [P1] Workspace trust fails open while metadata is unhydrated. 🤖 [P1] A restored target can publish its active client before attachment restrictions exist. 🤖 [P1] Valid startup attachments can fail during connection hydration. |
| const services = getServices(); | ||
| let attachment: IEvaluationSessionAttachment | undefined; | ||
| try { | ||
| await waitForState(services.sessionsService.initialRestoreComplete, complete => complete, undefined, token); |
There was a problem hiding this comment.
AI Review: Waiting for initialRestoreComplete lets persisted restore activate this target before the attachment gate exists, so the provider can publish its first active-client snapshot ungated. Reopening the same restored session may not emit another active-session change, and readiness changes do not trigger a resync, leaving the external driver with an incomplete inventory. Register a provisional gate before restore can activate the target, then bind the exact backend identity later, or explicitly resynchronize publication after binding.
| if (chatURI !== undefined) { | ||
| this._ensurePendingMessageSubscription(sessionResource, resolvedSession); | ||
| this._ensureDraftSyncSubscription(sessionResource, resolvedSession, chatURI); | ||
| if (!this._isEvaluationSessionAttached(resolvedSession)) { |
There was a problem hiding this comment.
AI Review: This attachment check suppresses only draft synchronization. The session remains locally interactive, so Submit, Stop, and Rename can still dispatch ChatTurnStarted, ChatTurnCancelled, and SessionTitleChanged, racing the external evaluation driver. Gate these mutation paths for the exact attached identity while preserving execution of externally preapproved Running client tools.
| throw new Error(localize('evaluationSessionAttachment.changedBeforePublication', "The active evaluation session changed before publication was ready.")); | ||
| } | ||
| services.reconcileClientToolSets(); | ||
| attachment.markActiveClientPublicationReady(); |
There was a problem hiding this comment.
AI Review: whenWorkbenchRestored excludes Eventually contributions. With Automations enabled, AutomationToolsContribution has not registered its four client tools when reconciliation runs, yet this releases the first active-client snapshot that signals the driver to start its turn. Wait for required late tool producers, or instantiate them explicitly, before marking publication ready.
| return; | ||
| } | ||
|
|
||
| const preparation = this._prepareActiveClientPublication(cached, token); |
There was a problem hiding this comment.
AI Review: For a sessionSchemeAlias provider, this new gate keys readiness to cached.backendUri, but the later publication reconstructs the channel from cached.agentProvider and rawId. Those schemes differ for alias-backed sessions, so the inventory can be sent to the UI URI instead of the attached backend AHP channel and never reach the driver. Publish to cached.backendUri and add an alias-provider regression test.
| try { | ||
| await waitForState(services.sessionsService.initialRestoreComplete, complete => complete, undefined, token); | ||
| const session = await waitForExactSession(services.sessionsManagementService, resource, token); | ||
| const identity = resolveEvaluationSessionIdentity(resource, session, services.connectionsService); |
There was a problem hiding this comment.
AI Review: waitForExactSession can return a persisted or seeded session before its remote connection is live, but resolveSessionResource is live-only. This one-shot resolution then fails before openSession can trigger on-demand connection, and the lifecycle never retries. Establish or await the owning provider connection before resolving and validating the exact identity.
| } | ||
| shouldDeferConfirmation(identity: IEvaluationSessionIdentity & { readonly clientId: string }, request: { readonly kind: SessionInputRequestKind; readonly clientId: string; readonly toolCall: { readonly status: ToolCallStatus } }): boolean { | ||
| return this.isAttached(identity) | ||
| && request.kind === SessionInputRequestKind.ToolClientExecution |
There was a problem hiding this comment.
AI Review: This defers only client ToolClientExecution requests. Server-owned tools use ToolConfirmation and still reach _awaitToolConfirmation, allowing this attached window to dispatch ChatToolCallConfirmed before the external driver; the first answer changes the run. Keep rendering server tools, but suppress local confirmation dispatch for the exact evaluation attachment and add coverage for that path.
| await waitForState(services.sessionsService.initialRestoreComplete, complete => complete, undefined, token); | ||
| const session = await waitForExactSession(services.sessionsManagementService, resource, token); | ||
| const identity = resolveEvaluationSessionIdentity(resource, session, services.connectionsService); | ||
| if (!await raceCancellationError(services.sessionsService.canOpenSession(session), token)) { |
There was a problem hiding this comment.
AI Review: canOpenSession is not a fail-closed trust gate here: it returns true for an already-active restored session and while workspace metadata is undefined. A driver-approved Running client-tool request can then execute without passing through the normal send-time folder-trust check. Wait for authoritative workspace or quick-chat metadata and perform an attachment-specific trust check before attaching or publishing client tools.
Summary
Adds a private, unlisted
--attach-to-evaluation-session <uri>argument for evaluation harnesses.The argument opens the exact existing remote Agent Host session in an Agents window as a secondary client. The window contributes its client tools, including the integrated browser tools, while the external driver retains ownership of evaluated turns and tool approval decisions.
When the argument is omitted, existing window routing, session handling, approval behavior, and active-client publication remain unchanged.
Example
code-insiders --attach-to-evaluation-session \ 'remote-example-provider:/session-id'Implementation
Validation
openBrowserPageandreadPage, returned controlled page content, and executed no controller-owned tools.