From 36f8813cfa5d9156a71eca30b92f1fafcacffb49 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Fri, 27 Mar 2026 16:40:44 -0700 Subject: [PATCH] address PR review comments --- .../common/state/protocol/.ahp-version | 2 +- .../common/state/protocol/commands.ts | 1 + .../agentHost/common/state/protocol/errors.ts | 5 + .../common/state/protocol/version/registry.ts | 107 ++++++++++++++++++ .../agentHost/node/agentSideEffects.ts | 17 ++- .../agentHost/agentHostEditingSession.ts | 30 ++++- .../agentHost/agentHostSessionHandler.ts | 7 +- 7 files changed, 155 insertions(+), 14 deletions(-) create mode 100644 src/vs/platform/agentHost/common/state/protocol/version/registry.ts diff --git a/src/vs/platform/agentHost/common/state/protocol/.ahp-version b/src/vs/platform/agentHost/common/state/protocol/.ahp-version index 1e7e823363b..eda0ecd4b8b 100644 --- a/src/vs/platform/agentHost/common/state/protocol/.ahp-version +++ b/src/vs/platform/agentHost/common/state/protocol/.ahp-version @@ -1 +1 @@ -101c091 +95cbb57 diff --git a/src/vs/platform/agentHost/common/state/protocol/commands.ts b/src/vs/platform/agentHost/common/state/protocol/commands.ts index 2f1ea9241f4..96ea242ae2e 100644 --- a/src/vs/platform/agentHost/common/state/protocol/commands.ts +++ b/src/vs/platform/agentHost/common/state/protocol/commands.ts @@ -301,6 +301,7 @@ export interface IFetchContentResult { * @version 1 * @throws `NotFound` (`-32008`) if the parent directory does not exist. * @throws `PermissionDenied` (`-32009`) if the client is not permitted to write to the path. + * @throws `AlreadyExists` (`-32010`) if `createOnly` is set and the file already exists. * @example * ```jsonc * // Client → Server diff --git a/src/vs/platform/agentHost/common/state/protocol/errors.ts b/src/vs/platform/agentHost/common/state/protocol/errors.ts index f582cdae0b2..d22126e9222 100644 --- a/src/vs/platform/agentHost/common/state/protocol/errors.ts +++ b/src/vs/platform/agentHost/common/state/protocol/errors.ts @@ -66,6 +66,11 @@ export const AhpErrorCodes = { * directory or workspace roots). */ PermissionDenied: -32009, + /** + * The target resource already exists and the operation does not allow + * overwriting (e.g. `writeFile` with `createOnly: true`). + */ + AlreadyExists: -32010, } as const; /** Union type of all AHP application error codes. */ diff --git a/src/vs/platform/agentHost/common/state/protocol/version/registry.ts b/src/vs/platform/agentHost/common/state/protocol/version/registry.ts new file mode 100644 index 00000000000..c3d45ae3dfa --- /dev/null +++ b/src/vs/platform/agentHost/common/state/protocol/version/registry.ts @@ -0,0 +1,107 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +// allow-any-unicode-comment-file +// DO NOT EDIT -- auto-generated by scripts/sync-agent-host-protocol.ts + +import { ActionType, type IStateAction } from '../actions.js'; +import { NotificationType, type IProtocolNotification } from '../notifications.js'; + +// ─── Protocol Version Constants ────────────────────────────────────────────── + +/** The current protocol version that new code speaks. */ +export const PROTOCOL_VERSION = 1; + +/** The oldest protocol version the implementation maintains compatibility with. */ +export const MIN_PROTOCOL_VERSION = 1; + +// ─── Exhaustive Action → Version Map ───────────────────────────────────────── + +/** + * Maps every action type to the protocol version that introduced it. + * Adding a new action to `IStateAction` without adding it here is a compile error. + */ +export const ACTION_INTRODUCED_IN: { readonly [K in IStateAction['type']]: number } = { + [ActionType.RootAgentsChanged]: 1, + [ActionType.RootActiveSessionsChanged]: 1, + [ActionType.SessionReady]: 1, + [ActionType.SessionCreationFailed]: 1, + [ActionType.SessionTurnStarted]: 1, + [ActionType.SessionDelta]: 1, + [ActionType.SessionResponsePart]: 1, + [ActionType.SessionToolCallStart]: 1, + [ActionType.SessionToolCallDelta]: 1, + [ActionType.SessionToolCallReady]: 1, + [ActionType.SessionToolCallConfirmed]: 1, + [ActionType.SessionToolCallComplete]: 1, + [ActionType.SessionToolCallResultConfirmed]: 1, + [ActionType.SessionTurnComplete]: 1, + [ActionType.SessionTurnCancelled]: 1, + [ActionType.SessionError]: 1, + [ActionType.SessionTitleChanged]: 1, + [ActionType.SessionUsage]: 1, + [ActionType.SessionReasoning]: 1, + [ActionType.SessionModelChanged]: 1, + [ActionType.SessionServerToolsChanged]: 1, + [ActionType.SessionActiveClientChanged]: 1, + [ActionType.SessionActiveClientToolsChanged]: 1, + [ActionType.SessionPendingMessageSet]: 1, + [ActionType.SessionPendingMessageRemoved]: 1, + [ActionType.SessionQueuedMessagesReordered]: 1, + [ActionType.SessionCustomizationsChanged]: 1, + [ActionType.SessionCustomizationToggled]: 1, +}; + +/** + * Returns whether the given action type is known to the specified protocol version. + */ +export function isActionKnownToVersion(action: IStateAction, clientVersion: number): boolean { + return ACTION_INTRODUCED_IN[action.type] <= clientVersion; +} + +// ─── Exhaustive Notification → Version Map ───────────────────────────────── + +/** + * Maps every notification type to the protocol version that introduced it. + * Adding a new notification to `IProtocolNotification` without adding it here + * is a compile error. + */ +export const NOTIFICATION_INTRODUCED_IN: { readonly [K in IProtocolNotification['type']]: number } = { + [NotificationType.SessionAdded]: 1, + [NotificationType.SessionRemoved]: 1, + [NotificationType.AuthRequired]: 1, +}; + +/** + * Returns whether the given notification type is known to the specified protocol version. + */ +export function isNotificationKnownToVersion(notification: IProtocolNotification, clientVersion: number): boolean { + return NOTIFICATION_INTRODUCED_IN[notification.type] <= clientVersion; +} + +// ─── Capabilities ──────────────────────────────────────────────────────────── + +/** + * Feature capabilities gated by protocol version. + */ +export interface ProtocolCapabilities { + /** v1 — always present */ + readonly sessions: true; + /** v1 — always present */ + readonly tools: true; + /** v1 — always present */ + readonly permissions: true; +} + +/** + * Derives capabilities from a protocol version number. + */ +export function capabilitiesForVersion(_version: number): ProtocolCapabilities { + return { + sessions: true, + tools: true, + permissions: true, + }; +} diff --git a/src/vs/platform/agentHost/node/agentSideEffects.ts b/src/vs/platform/agentHost/node/agentSideEffects.ts index c2cff8c8f25..51288d5d1e1 100644 --- a/src/vs/platform/agentHost/node/agentSideEffects.ts +++ b/src/vs/platform/agentHost/node/agentSideEffects.ts @@ -9,7 +9,7 @@ import { Disposable, DisposableStore, IDisposable } from '../../../base/common/l import { autorun, IObservable } from '../../../base/common/observable.js'; import { URI } from '../../../base/common/uri.js'; import { generateUuid } from '../../../base/common/uuid.js'; -import { IFileService } from '../../files/common/files.js'; +import { FileSystemProviderErrorCode, IFileService, toFileSystemProviderErrorCode } from '../../files/common/files.js'; import { ILogService } from '../../log/common/log.js'; import { IAgent, IAgentAttachment, IAgentMessageEvent, IAgentToolCompleteEvent, IAgentToolStartEvent, IAuthenticateParams, IAuthenticateResult, IResourceMetadata } from '../common/agentService.js'; import { ISessionDataService } from '../common/sessionDataService.js'; @@ -594,9 +594,20 @@ export class AgentSideEffects extends Disposable implements IProtocolSideEffectH content = VSBuffer.fromString(params.data); } try { - await this._fileService.writeFile(fileUri, content); + if (params.createOnly) { + await this._fileService.createFile(fileUri, content, { overwrite: false }); + } else { + await this._fileService.writeFile(fileUri, content); + } return {}; - } catch (_e) { + } catch (e) { + const code = toFileSystemProviderErrorCode(e as Error); + if (code === FileSystemProviderErrorCode.FileExists) { + throw new ProtocolError(AhpErrorCodes.AlreadyExists, `File already exists: ${fileUri.toString()}`); + } + if (code === FileSystemProviderErrorCode.NoPermissions) { + throw new ProtocolError(AhpErrorCodes.PermissionDenied, `Permission denied: ${fileUri.toString()}`); + } throw new ProtocolError(AhpErrorCodes.NotFound, `Failed to write file: ${fileUri.toString()}`); } } diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostEditingSession.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostEditingSession.ts index 46075aef564..019e7459e33 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostEditingSession.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostEditingSession.ts @@ -246,10 +246,28 @@ export class AgentHostEditingSession extends Disposable implements IChatEditingS // ---- Snapshots ---------------------------------------------------------- - async restoreSnapshot(requestId: string, _stopId: string | undefined): Promise { - const idx = this._checkpoints.findIndex(cp => cp.requestId === requestId); + private _findCheckpointIndex(requestId: string, stopId: string | undefined): number { + if (stopId !== undefined) { + return this._checkpoints.findIndex(cp => cp.requestId === requestId && cp.undoStopId === stopId); + } + // No specific stop: use the last checkpoint for this request + for (let i = this._checkpoints.length - 1; i >= 0; i--) { + if (this._checkpoints[i].requestId === requestId) { + return i; + } + } + return -1; + } + + private _findCheckpoint(requestId: string, stopId: string | undefined): IAgentHostCheckpoint | undefined { + const idx = this._findCheckpointIndex(requestId, stopId); + return idx >= 0 ? this._checkpoints[idx] : undefined; + } + + async restoreSnapshot(requestId: string, stopId: string | undefined): Promise { + const idx = this._findCheckpointIndex(requestId, stopId); if (idx < 0) { - this._logService.warn(`[AgentHostEditingSession] No checkpoint found for requestId=${requestId}`); + this._logService.warn(`[AgentHostEditingSession] No checkpoint found for requestId=${requestId}${stopId ? `, stopId=${stopId}` : ''}`); return; } @@ -274,7 +292,7 @@ export class AgentHostEditingSession extends Disposable implements IChatEditingS } getSnapshotUri(requestId: string, uri: URI, stopId: string | undefined): URI | undefined { - const cp = this._checkpoints.find(c => c.requestId === requestId); + const cp = this._findCheckpoint(requestId, stopId); if (!cp) { return undefined; } @@ -292,8 +310,8 @@ export class AgentHostEditingSession extends Disposable implements IChatEditingS }); } - async getSnapshotContents(requestId: string, uri: URI, _stopId: string | undefined): Promise { - const cp = this._checkpoints.find(c => c.requestId === requestId); + async getSnapshotContents(requestId: string, uri: URI, stopId: string | undefined): Promise { + const cp = this._findCheckpoint(requestId, stopId); if (!cp) { return undefined; } diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionHandler.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionHandler.ts index 8cb4c25df5f..80dfe0d46d9 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionHandler.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionHandler.ts @@ -7,7 +7,7 @@ import { Throttler } from '../../../../../../base/common/async.js'; import { CancellationToken, CancellationTokenSource } from '../../../../../../base/common/cancellation.js'; import { Emitter, Event } from '../../../../../../base/common/event.js'; import { MarkdownString } from '../../../../../../base/common/htmlContent.js'; -import { Disposable, DisposableMap, DisposableResourceMap, DisposableStore, MutableDisposable, toDisposable, type IDisposable } from '../../../../../../base/common/lifecycle.js'; +import { Disposable, DisposableResourceMap, DisposableStore, MutableDisposable, toDisposable, type IDisposable } from '../../../../../../base/common/lifecycle.js'; import { ResourceMap } from '../../../../../../base/common/map.js'; import { observableValue } from '../../../../../../base/common/observable.js'; import { isEqual } from '../../../../../../base/common/resources.js'; @@ -168,7 +168,7 @@ export class AgentHostSessionHandler extends Disposable implements IChatSessionC /** Per-session subscription to chat model pending request changes. */ private readonly _pendingMessageSubscriptions = this._register(new DisposableResourceMap()); /** Per-session subscription watching for server-initiated turns. */ - private readonly _serverTurnWatchers = this._register(new DisposableMap()); + private readonly _serverTurnWatchers = this._register(new DisposableResourceMap()); /** Per-session writeFile listeners for agent host editing sessions. */ private readonly _editingSessionListeners = this._register(new DisposableResourceMap()); /** Historical turns with file edits, pending hydration into the editing session. */ @@ -497,7 +497,6 @@ export class AgentHostSessionHandler extends Disposable implements IChatSessionC * if applicable, and pipes turn progress through `progressObs`. */ private _watchForServerInitiatedTurns(backendSession: URI, sessionResource: URI): void { - const resourceKey = sessionResource.path.substring(1); const sessionStr = backendSession.toString(); // Seed from the current state so we don't treat any pre-existing active @@ -561,7 +560,7 @@ export class AgentHostSessionHandler extends Disposable implements IChatSessionC this._trackServerTurnProgress(backendSession, activeTurn.id, chatSession, sessionResource, turnStore); })); - this._serverTurnWatchers.set(resourceKey, disposables); + this._serverTurnWatchers.set(sessionResource, disposables); } /**