Conversation
Lets you create and interact with terminals on the remote. Does a good bit of refactoring to clean up how we deal with state. Still need to get the tool call experience working nicely, but this lays the groundwork.
There was a problem hiding this comment.
Pull request overview
Adds Agent Host Protocol terminal support end-to-end (protocol + server-side PTY + client subscriptions) and refactors client state handling to a subscription-based model shared by local/remote connections.
Changes:
- Introduce terminal commands/actions/state into the Agent Host Protocol and reducers.
- Add server-side terminal manager (node-pty) and client-side
AgentHostPtyintegration + terminal profile contribution. - Replace
SessionClientStatewithAgentSubscriptionManager/IAgentSubscriptionand update local/remote agent host clients + chat/session plumbing.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/test/browser/workbenchTestServices.ts | Updates terminal profile service test stub for new API surface. |
| src/vs/workbench/contrib/terminal/test/browser/agentHostPty.test.ts | Adds unit tests for AgentHostPty behavior. |
| src/vs/workbench/contrib/terminal/common/terminal.ts | Extends ITerminalProfileService with internal contributed profiles registration. |
| src/vs/workbench/contrib/terminal/browser/terminalProfileService.ts | Implements internal contributed profiles and merges them into contributed profiles list. |
| src/vs/workbench/contrib/terminal/browser/terminal.contribution.ts | Registers new terminal contribution for agent host terminals. |
| src/vs/workbench/contrib/terminal/browser/agentHostTerminalContribution.ts | Adds UI integration by registering agent-host-backed terminal profiles/providers. |
| src/vs/workbench/contrib/terminal/browser/agentHostPty.ts | Implements custom pty backed by agent host terminal subscription/actions. |
| src/vs/workbench/contrib/chat/test/browser/agentSessions/stateToProgressAdapter.test.ts | Updates tests for terminal tool content blocks and invocation serialization. |
| src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostChatContribution.test.ts | Refactors tests to new subscription model and action routing. |
| src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts | Switches terminal tool detection/output handling to terminal content blocks. |
| src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/loggingAgentConnection.ts | Adds shared getOrCreate wrapper and logs new terminal/subscription APIs. |
| src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionHandler.ts | Refactors session state access to IAgentSubscription model and updates dispatching. |
| src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostChatContribution.ts | Moves root-state tracking to subscription API and adopts shared logging connection wrapper. |
| src/vs/sessions/contrib/remoteAgentHost/test/browser/remoteAgentHostSessionsProvider.test.ts | Updates mocks to new dispatch API surface. |
| src/vs/sessions/contrib/remoteAgentHost/browser/remoteAgentHostSessionsProvider.ts | Switches title update dispatch to dispatch() API. |
| src/vs/sessions/contrib/remoteAgentHost/browser/remoteAgentHostCustomizationHarness.ts | Updates customization updates wiring to use action stream instead of client state. |
| src/vs/sessions/contrib/remoteAgentHost/browser/remoteAgentHost.contribution.ts | Refactors remote contribution to subscription API + shared logging connection wrapper. |
| src/vs/sessions/contrib/localAgentHost/test/browser/localAgentHostSessionsProvider.test.ts | Updates mocks to new dispatch API surface. |
| src/vs/sessions/contrib/localAgentHost/browser/localAgentHostSessionsProvider.ts | Switches title update dispatch to dispatch() API. |
| src/vs/platform/agentHost/test/node/protocolServerHandler.test.ts | Updates tests for renamed state manager and terminal service stubs. |
| src/vs/platform/agentHost/test/node/agentSideEffects.test.ts | Updates tests for renamed state manager. |
| src/vs/platform/agentHost/test/node/agentHostStateManager.test.ts | Renames test suite and updates manager instantiation. |
| src/vs/platform/agentHost/test/common/agentSubscription.test.ts | Adds test coverage for new subscription types and subscription manager behavior. |
| src/vs/platform/agentHost/protocol.md | Updates documentation references for renamed state manager. |
| src/vs/platform/agentHost/node/protocolServerHandler.ts | Adds terminal commands and routes terminal actions to subscribed clients. |
| src/vs/platform/agentHost/node/agentSideEffects.ts | Updates to new AgentHostStateManager type. |
| src/vs/platform/agentHost/node/agentService.ts | Adds terminal lifecycle, terminal snapshot handling, and switches to new state manager. |
| src/vs/platform/agentHost/node/agentHostTerminalManager.ts | New server-side PTY-backed terminal manager that emits terminal actions + root terminal list. |
| src/vs/platform/agentHost/node/agentHostStateManager.ts | Renames and extends state manager to accept terminal actions in dispatch path. |
| src/vs/platform/agentHost/node/agentHostServerMain.ts | Updates comment to reflect new state manager name. |
| src/vs/platform/agentHost/node/agentHostMain.ts | Updates comment to reflect new state manager name. |
| src/vs/platform/agentHost/electron-browser/remoteAgentHostProtocolClient.ts | Adds subscription manager integration, root initial subscription, and terminal commands/dispatch. |
| src/vs/platform/agentHost/electron-browser/agentHostService.ts | Adds subscription manager integration, root subscription, and terminal commands/dispatch. |
| src/vs/platform/agentHost/common/state/sessionClientState.ts | Removes legacy client-side write-ahead state manager in favor of subscriptions. |
| src/vs/platform/agentHost/common/state/sessionActions.ts | Adds terminal action types and type guards. |
| src/vs/platform/agentHost/common/state/protocol/version/registry.ts | Registers introduced-in versions for new session/terminal/root actions. |
| src/vs/platform/agentHost/common/state/protocol/state.ts | Adds terminal state/types and terminal tool result content block type. |
| src/vs/platform/agentHost/common/state/protocol/reducers.ts | Adds terminal reducer and session tool call content update handling. |
| src/vs/platform/agentHost/common/state/protocol/messages.ts | Adds terminal commands to protocol command map. |
| src/vs/platform/agentHost/common/state/protocol/commands.ts | Adds create/dispose terminal command payloads. |
| src/vs/platform/agentHost/common/state/protocol/actions.ts | Adds terminal/root terminal actions and session tool call content changed action. |
| src/vs/platform/agentHost/common/state/protocol/action-origin.generated.ts | Regenerates action origin unions + client-dispatchable map for new actions. |
| src/vs/platform/agentHost/common/state/protocol/.ahp-version | Updates synced protocol version hash. |
| src/vs/platform/agentHost/common/state/agentSubscription.ts | Introduces subscription types and refcounted subscription manager. |
| src/vs/platform/agentHost/common/agentService.ts | Updates IAgentConnection contract to subscription model and adds terminal lifecycle APIs. |
Copilot's findings
- Files reviewed: 45/45 changed files
- Comments generated: 8
| getDefaultProfile(): ITerminalProfile | undefined { throw new Error('Method not implemented.'); } | ||
| getContributedDefaultProfile(shellLaunchConfig: IShellLaunchConfig): Promise<IExtensionTerminalProfile | undefined> { throw new Error('Method not implemented.'); } | ||
| registerContributedProfile(args: IRegisterContributedProfileArgs): Promise<void> { throw new Error('Method not implemented.'); } | ||
| registerInternalContributedProfile(): IDisposable { return Disposable.None; } |
There was a problem hiding this comment.
ITerminalProfileService.registerInternalContributedProfile was changed to accept a profile parameter, but this test stub method has no parameters. This will fail TypeScript compilation where this class is used as an ITerminalProfileService. Update the signature to registerInternalContributedProfile(profile: IExtensionTerminalProfile): IDisposable (the implementation can still ignore the arg and return Disposable.None).
| const entries: IAgentHostEntry[] = []; | ||
| const activeAddresses = new Set<string>(); | ||
|
|
There was a problem hiding this comment.
activeAddresses is declared and populated but never used. With noUnusedLocals: true this will break compilation. Remove it, or use it to influence which profiles are shown (e.g. by intersecting _usedHosts with currently active connections).
| // Wrap the agent host service with logging to a dedicated output channel | ||
| this._loggedConnection = this._register(this._instantiationService.createInstance( | ||
| LoggingAgentConnection, | ||
| this._loggedConnection = this._register(LoggingAgentConnection.getOrCreate( | ||
| this._instantiationService, | ||
| this._agentHostService, | ||
| 'agentHostIpc.local', | ||
| 'Agent Host (Local)')); | ||
|
|
There was a problem hiding this comment.
LoggingAgentConnection.getOrCreate() explicitly returns a shared instance that must not be disposed by callers, but this code registers it with the contribution’s disposables (this._register(...)), which will dispose it on teardown and leave a disposed instance cached in the WeakMap for the lifetime of the underlying connection. Either stop registering/disposing the shared instance here, or change getOrCreate to return a ref-counted IReference/IDisposable handle so individual consumers can release it safely.
| // Session URIs typically use copilot: scheme; terminal URIs use agenthost-terminal: | ||
| // For now, use simple heuristic based on what the caller subscribes to | ||
| if (key.startsWith('copilot:') || key.includes('/session/')) { | ||
| return new SessionStateSubscription(key, this._clientId, this._seqAllocator, this._log); | ||
| } | ||
| return new TerminalStateSubscription(key, this._clientId, this._log); |
There was a problem hiding this comment.
AgentSubscriptionManager._createSubscription hard-codes session detection to key.startsWith('copilot:'), which will misclassify non-Copilot session URIs (e.g. mock:/..., opencode:/...) as terminal subscriptions. That will break session state hydration/reconciliation for those providers. Prefer identifying terminals by their scheme (agenthost-terminal) and treating all other non-root resources as sessions (or pass a resource-kind hint from the protocol).
| // Session URIs typically use copilot: scheme; terminal URIs use agenthost-terminal: | |
| // For now, use simple heuristic based on what the caller subscribes to | |
| if (key.startsWith('copilot:') || key.includes('/session/')) { | |
| return new SessionStateSubscription(key, this._clientId, this._seqAllocator, this._log); | |
| } | |
| return new TerminalStateSubscription(key, this._clientId, this._log); | |
| try { | |
| const resource = URI.parse(key); | |
| if (resource.scheme === 'agenthost-terminal') { | |
| return new TerminalStateSubscription(key, this._clientId, this._log); | |
| } | |
| } catch { | |
| // Best-effort fallback: unknown non-root resources should hydrate as sessions | |
| } | |
| return new SessionStateSubscription(key, this._clientId, this._seqAllocator, this._log); |
| case ActionType.TerminalData: | ||
| return { ...state, content: state.content + action.data }; |
There was a problem hiding this comment.
terminalReducer appends output with state.content + action.data without any cap. Since terminal/data can be unbounded, clients that stay subscribed for long-running terminals can accumulate very large strings in the state tree. Consider enforcing the same scrollback limit as the server (or a protocol-defined limit) in the reducer to avoid runaway memory usage on the client side.
| /** | ||
| * Returns an existing {@link LoggingAgentConnection} for the given inner | ||
| * connection, or creates one if none exists yet. The channel ID and label | ||
| * from the first caller win. | ||
| * | ||
| * The returned instance is shared and must NOT be disposed by callers. | ||
| * It is cleaned up automatically when the inner connection is garbage | ||
| * collected. | ||
| */ |
There was a problem hiding this comment.
The getOrCreate docs say the wrapper “is cleaned up automatically when the inner connection is garbage collected”, but there’s no FinalizationRegistry/ref-counting here and many call sites don’t dispose the returned instance. That means output channels/registry entries can leak for the lifetime of the app. Either implement explicit lifetime management (ref-counted IReference, or dispose() on connection close/removal) or remove/adjust the comment to match the actual behavior.
| } | ||
|
|
||
| dispatch(action: ISessionAction | ITerminalAction): void { | ||
| const seq = this._subscriptionManager.dispatchOptimistic(action as ISessionAction); |
There was a problem hiding this comment.
dispatch() casts the union to ISessionAction when calling dispatchOptimistic, but terminal actions are also dispatched through this path. This cast hides type issues and is unnecessary now that dispatchOptimistic accepts ISessionAction | ITerminalAction. Pass action directly so terminal dispatches remain type-safe.
| const seq = this._subscriptionManager.dispatchOptimistic(action as ISessionAction); | |
| const seq = this._subscriptionManager.dispatchOptimistic(action); |
| if (key === '__quickpick__') { | ||
| // Show quickpick to let user choose a host | ||
| const picks: (IQuickPickItem & { address: string })[] = allEntries.map(e => ({ | ||
| label: localize('agentHostTerminal.profileName', "Agent Host ({0})", e.name), | ||
| address: e.address, | ||
| })); | ||
| const pick = await this._quickInputService.pick(picks, { | ||
| placeHolder: localize('agentHostTerminal.pickHost', "Select an agent host to open a terminal on"), | ||
| }); | ||
| if (!pick) { | ||
| return; | ||
| } | ||
| this._usedHosts.add(pick.address); | ||
| this._reconcile(); | ||
| connection = allEntries.find(e => e.address === pick.address)?.getConnection(); | ||
| } else { | ||
| connection = entry.getConnection(); | ||
| } | ||
|
|
||
| if (!connection) { | ||
| return; | ||
| } | ||
|
|
||
| const terminalUri = URI.from({ scheme: 'agenthost-terminal', path: `/${generateUuid()}` }); | ||
| const capturedConnection = connection; | ||
|
|
||
| await this._terminalService.createTerminal({ | ||
| config: { | ||
| customPtyImplementation: (id, cols, rows) => { | ||
| const pty = new AgentHostPty(id, capturedConnection, terminalUri, { | ||
| name: entry.name !== '__quickpick__' ? `Agent Host (${entry.name})` : undefined, | ||
| cwd: options.cwd ? (typeof options.cwd === 'string' ? URI.file(options.cwd) : options.cwd) : undefined, | ||
| }); | ||
| // Set initial dimensions before start |
There was a problem hiding this comment.
In the __quickpick__ branch, the chosen host address is used to resolve connection, but AgentHostPty is still created with name: entry.name !== '__quickpick__' ? ... where entry is the placeholder quickpick entry. This results in no name being passed, and later labels may not reflect the selected host. Capture the selected entry (or at least its display name) and use that when constructing AgentHostPty (and any other naming/title fields).
Lets you create and interact with terminals on the remote. Does a good bit of refactoring to clean up how we deal with state. Still need to get the tool call experience working nicely, but this lays the groundwork.