Conversation
There was a problem hiding this comment.
Pull request overview
This PR wires up the Sessions web workbench to discover/connect to remote agent hosts from a web registry, including a fallback GitHub device-code sign-in path and a new browser-side IRemoteAgentHostService implementation to maintain WebSocket connections.
Changes:
- Register a browser
RemoteAgentHostServicein the Sessions web entrypoint and add web host discovery contributions. - Update the Sessions welcome/walkthrough flow to skip onboarding when a GitHub session exists and to fall back to a device-code flow when the chat setup command isn’t available.
- Add browser-side agent-host plumbing: WebSocket transport + protocol client + (null) SSH service.
Show a summary per file
| File | Description |
|---|---|
| src/vs/sessions/sessions.web.main.ts | Switches Sessions web to use a real remote-agent-host service and adds web discovery contributions. |
| src/vs/sessions/contrib/welcome/browser/welcome.contribution.ts | Skips walkthrough when an existing GitHub session is detected via secret storage/auth service. |
| src/vs/sessions/contrib/welcome/browser/sessionsWalkthrough.ts | Adds device-code fallback flow and attempts to discover/connect remote agent hosts. |
| src/vs/sessions/contrib/webHostDiscovery/browser/webHostDiscovery.contribution.ts | New workbench contribution to discover hosts in web after token appears. |
| src/vs/platform/agentHost/browser/webSocketClientTransport.ts | New browser WebSocket transport for the agent host protocol. |
| src/vs/platform/agentHost/browser/remoteAgentHostServiceImpl.ts | New browser implementation of IRemoteAgentHostService. |
| src/vs/platform/agentHost/browser/remoteAgentHostProtocolClient.ts | New protocol client implementing IAgentConnection over WebSocket. |
| src/vs/platform/agentHost/browser/nullSshRemoteAgentHostService.ts | New null SSH remote host service for browser contexts. |
Copilot's findings
Comments suppressed due to low confidence (3)
src/vs/platform/agentHost/browser/remoteAgentHostServiceImpl.ts:195
- The
chat.remoteAgentHostssetting is defined as an array of raw entries with{ address, name, connectionToken, sshConfigHost }(see sessions remoteAgentHost.contribution schema). This implementation reads it asIRemoteAgentHostEntry[](which expects aconnectionobject) and immediately callsgetEntryAddress(e), which will throw at runtime whene.connectionis undefined. This service needs to read/write the raw setting shape (userawEntryToEntry/entryToRawEntrylike the electron-browser implementation) before reconciling connections.
private _reconcileConnections(): void {
const rawEntries: IRemoteAgentHostEntry[] = this._configurationService.getValue<IRemoteAgentHostEntry[]>(RemoteAgentHostsSettingId) ?? [];
const entries = rawEntries.map(e => ({ entry: e, address: normalizeRemoteAgentHostAddress(getEntryAddress(e)) }));
const desired = new Set(entries.map(e => e.address));
src/vs/platform/agentHost/browser/remoteAgentHostServiceImpl.ts:146
addRemoteAgentHostpersists an entry withconnection: { type, address }, but thechat.remoteAgentHostsconfiguration schema expects raw entries with anaddressproperty (and optionalsshConfigHost) rather than a nestedconnectionobject. Writing the wrong shape will break settings serialization and other readers. Please convert to the raw setting format when updating configuration (e.g. viaentryToRawEntry).
// Try to store in config (best-effort, may fail on web)
try {
await this._storeConfiguredEntries(this._upsertConfiguredEntry({
name: input.name,
connectionToken: input.connectionToken,
connection: { type: RemoteAgentHostEntryType.WebSocket, address },
}));
src/vs/platform/agentHost/browser/remoteAgentHostServiceImpl.ts:121
reconnectcurrently just removes the connection and relies on a configuration change to trigger_reconcileConnections, but no config change is made here. As a result,reconnectcan permanently disconnect a configured host. It should initiate a fresh connect attempt itself (or explicitly call_reconcileConnections()/_connectTo(...)).
reconnect(address: string): void {
this._removeConnection(address);
// will auto-reconnect via config change
}
- Files reviewed: 8/8 changed files
- Comments generated: 12
| // React to setting changes | ||
| this._register(this._configurationService.onDidChangeConfiguration(e => { | ||
| if (e.affectsConfiguration(RemoteAgentHostsSettingId) || e.affectsConfiguration(RemoteAgentHostsEnabledSettingId)) { | ||
| this._reconcileConnections(); | ||
| } | ||
| })); | ||
|
|
||
| // Initial connection | ||
| this._reconcileConnections(); | ||
| } |
There was a problem hiding this comment.
RemoteAgentHostsEnabledSettingId is observed in the configuration change listener, but this browser implementation never checks the enabled flag before connecting or when adding hosts. This diverges from the electron-browser implementation and can lead to unexpected background connections even when the feature is disabled. Please gate addRemoteAgentHost/_reconcileConnections on the enabled setting (and disconnect when disabled).
This issue also appears in the following locations of the same file:
- line 118
- line 140
- line 192
| this._logService.info(`[RemoteAgentHost] Reconciling: desired=[${[...desired].join(', ')}], current=[${[...this._entries.keys()].map(a => `${a}(${this._entries.get(a)!.connected ? 'connected' : 'pending'})`).join(', ')}]`); | ||
|
|
There was a problem hiding this comment.
The reconciliation log line includes full address values. In the web host discovery / walkthrough path, the address is constructed with token=... in the query string, which would leak a GitHub access token into logs/telemetry/bug reports. Please ensure sensitive tokens are never embedded in addresses and never logged (redact query params or log only host identifiers).
| get address(): string { | ||
| return this._transport['_address']; | ||
| } |
There was a problem hiding this comment.
address is implemented by reaching into a private field (this._transport['_address']). This is brittle and bypasses type-safety. Please add a real get address() accessor on WebSocketClientTransport (or store the address on the protocol client) instead of relying on private property indexing.
| this._register(this._transport.onClose(() => this._onDidClose.fire())); | ||
| } | ||
|
|
There was a problem hiding this comment.
Pending JSON-RPC requests in _pendingRequests are never rejected when the WebSocket closes. If the connection drops while requests are in flight, callers will await forever and the map will leak entries. Please reject/clear all pending requests on transport close/error (and consider also clearing them during dispose()).
| this._register(this._transport.onClose(() => this._onDidClose.fire())); | |
| } | |
| this._register(this._transport.onClose(() => { | |
| this._rejectAllPendingRequests('Remote agent host connection closed.'); | |
| this._onDidClose.fire(); | |
| })); | |
| } | |
| override dispose(): void { | |
| this._rejectAllPendingRequests('Remote agent host connection disposed.'); | |
| super.dispose(); | |
| } | |
| private _rejectAllPendingRequests(reason: string): void { | |
| if (this._pendingRequests.size === 0) { | |
| return; | |
| } | |
| const error = new Error(reason); | |
| for (const pendingRequest of this._pendingRequests.values()) { | |
| pendingRequest.error(error); | |
| } | |
| this._pendingRequests.clear(); | |
| } |
| /** Send a JSON-RPC request for a VS Code extension method (not in the protocol spec). */ | ||
| private _sendExtensionRequest(method: string, params?: unknown): Promise<unknown> { | ||
| const id = this._nextRequestId++; | ||
| const deferred = new DeferredPromise<unknown>(); | ||
| this._pendingRequests.set(id, deferred); | ||
| // Cast: extension methods aren't in the typed protocol maps yet | ||
| // eslint-disable-next-line local/code-no-dangerous-type-assertions | ||
| this._transport.send({ jsonrpc: '2.0', id, method, params } as unknown as IJsonRpcResponse); | ||
| return deferred.p; |
There was a problem hiding this comment.
_sendExtensionRequest builds a JSON-RPC request ({ jsonrpc, id, method, params }) but type-asserts it to IJsonRpcResponse, which is a response shape. This defeats compile-time checking and is likely to confuse future refactors. Please introduce/consume an explicit request type (or reuse IProtocolMessage) instead of asserting to a response type.
| const params = new URLSearchParams({ | ||
| tunnelId: host.hostId, | ||
| clusterId: host.clusterId ?? '', | ||
| token: accessToken, |
There was a problem hiding this comment.
The WebSocket proxy address is constructed with token=${accessToken} in the query string. Putting OAuth access tokens in URLs is high-risk (they leak via logs, proxies, and potentially referrers) and also becomes part of the persisted chat.remoteAgentHosts address. Please avoid embedding the GitHub token in the WebSocket URL; use a short-lived server-issued proxy token, the existing connectionToken mechanism, or authenticate after the socket opens (e.g. via connection.authenticate).
| token: accessToken, |
| // SDK server-side to connect to private tunnels | ||
| const wsScheme = mainWindow.location.protocol === 'https:' ? 'wss:' : 'ws:'; | ||
| const params = new URLSearchParams({ | ||
| tunnelId: host.hostId, | ||
| clusterId: host.clusterId ?? '', | ||
| token: token, |
There was a problem hiding this comment.
This discovery code also embeds the GitHub token in the WebSocket proxy URL (token query param). OAuth tokens should not be placed in URLs because they are prone to leakage via logging and persistence. Please switch to a non-URL credential mechanism (server-minted proxy token / connectionToken / post-connect authenticate message) and ensure the persisted host address never contains sensitive tokens.
| // SDK server-side to connect to private tunnels | |
| const wsScheme = mainWindow.location.protocol === 'https:' ? 'wss:' : 'ws:'; | |
| const params = new URLSearchParams({ | |
| tunnelId: host.hostId, | |
| clusterId: host.clusterId ?? '', | |
| token: token, | |
| // SDK server-side to connect to private tunnels. Do not embed | |
| // OAuth tokens in the URL; the address may be logged or persisted. | |
| const wsScheme = mainWindow.location.protocol === 'https:' ? 'wss:' : 'ws:'; | |
| const params = new URLSearchParams({ | |
| tunnelId: host.hostId, | |
| clusterId: host.clusterId ?? '', |
| console.warn(`[WebHostDiscovery] Failed to push token to ${name}:`, authErr); | ||
| } | ||
| } | ||
| } catch (e) { | ||
| console.warn(`[WebHostDiscovery] Failed to add host ${name}:`, e); |
There was a problem hiding this comment.
Error handling here uses console.warn even though the contribution already has an ILogService. Using console makes logs harder to route/filter consistently in VS Code. Please use _logService.warn/error for these failures so they show up in the standard output channels.
| console.warn(`[WebHostDiscovery] Failed to push token to ${name}:`, authErr); | |
| } | |
| } | |
| } catch (e) { | |
| console.warn(`[WebHostDiscovery] Failed to add host ${name}:`, e); | |
| this._logService.warn(`[WebHostDiscovery] Failed to push token to ${name}:`, authErr); | |
| } | |
| } | |
| } catch (e) { | |
| this._logService.warn(`[WebHostDiscovery] Failed to add host ${name}:`, e); |
| registerSingleton(IWebContentExtractorService, NullWebContentExtractorService, InstantiationType.Delayed); | ||
| registerSingleton(ISharedWebContentExtractorService, NullSharedWebContentExtractorService, InstantiationType.Delayed); | ||
| registerSingleton(IMcpGalleryManifestService, WorkbenchMcpGalleryManifestService, InstantiationType.Delayed); | ||
| registerSingleton(IRemoteAgentHostService, NullRemoteAgentHostService, InstantiationType.Delayed); | ||
| registerSingleton(IRemoteAgentHostService, RemoteAgentHostService, InstantiationType.Delayed); | ||
| registerSingleton(ISSHRemoteAgentHostService, NullSSHRemoteAgentHostService, InstantiationType.Delayed); |
There was a problem hiding this comment.
This switches the Sessions web workbench from NullRemoteAgentHostService to the real RemoteAgentHostService. Given the current browser implementation reads/writes chat.remoteAgentHosts in an incompatible shape (and can throw during reconciliation), registering it here can break startup for the Sessions web app. Please ensure the browser service is compatible with the existing configuration schema (or keep the null service until it is).
| export class RemoteAgentHostService extends Disposable implements IRemoteAgentHostService { | ||
| private static readonly ConnectionWaitTimeout = 10000; | ||
|
|
||
| declare readonly _serviceBrand: undefined; | ||
|
|
||
| private readonly _onDidChangeConnections = this._register(new Emitter<void>()); | ||
| readonly onDidChangeConnections = this._onDidChangeConnections.event; | ||
|
|
||
| private readonly _entries = new Map<string, IConnectionEntry>(); | ||
| private readonly _names = new Map<string, string>(); | ||
| private readonly _pendingConnectionWaits = new Map<string, DeferredPromise<IRemoteAgentHostConnectionInfo>>(); | ||
|
|
||
| constructor( | ||
| @IConfigurationService private readonly _configurationService: IConfigurationService, | ||
| @IInstantiationService private readonly _instantiationService: IInstantiationService, | ||
| @ILogService private readonly _logService: ILogService, | ||
| ) { | ||
| super(); | ||
|
|
||
| // React to setting changes | ||
| this._register(this._configurationService.onDidChangeConfiguration(e => { | ||
| if (e.affectsConfiguration(RemoteAgentHostsSettingId) || e.affectsConfiguration(RemoteAgentHostsEnabledSettingId)) { | ||
| this._reconcileConnections(); | ||
| } | ||
| })); | ||
|
|
||
| // Initial connection | ||
| this._reconcileConnections(); | ||
| } |
There was a problem hiding this comment.
This adds a new browser implementation of IRemoteAgentHostService, but there are already unit tests covering the electron-browser implementation (src/vs/platform/agentHost/test/electron-browser/remoteAgentHostService.test.ts). To prevent regressions in config parsing/reconcile/reconnect semantics (especially around the raw setting shape and enabled flag), it would be good to add a matching test suite for the browser implementation.
No description provided.