From fa80a2218b43864bd58d62877ccc5574c1fedd91 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 28 May 2026 21:44:23 +1000 Subject: [PATCH 1/2] feat: enhance session changeset handling with deferred refresh logic --- .../node/agentHostChangesetCoordinator.ts | 49 +++++++++++++------ .../agentHostChangesetCoordinator.test.ts | 34 ++++++++++++- 2 files changed, 66 insertions(+), 17 deletions(-) diff --git a/src/vs/platform/agentHost/node/agentHostChangesetCoordinator.ts b/src/vs/platform/agentHost/node/agentHostChangesetCoordinator.ts index 5bde8ef302377..00a8db7da29fc 100644 --- a/src/vs/platform/agentHost/node/agentHostChangesetCoordinator.ts +++ b/src/vs/platform/agentHost/node/agentHostChangesetCoordinator.ts @@ -56,9 +56,9 @@ export const CHANGESET_DB_METADATA_KEYS: Record = { * {@link IAgentHostChangesetService} (which owns compute / publish / * persist primitives). * - * Owns the deferred uncommitted-refresh state machine — refreshes that - * fire before the session's working directory is known are queued and - * drained from {@link onSessionMaterialized} / {@link onSessionRestored}. + * Owns the deferred static-refresh state machine — refreshes that fire + * before the session's working directory is known are queued and drained + * from {@link onSessionMaterialized} / {@link onSessionRestored}. * * No per-session controllers — the cross-cutting concerns (listSessions * overlay, subscribe URI routing) inherently span sessions, so a single @@ -73,6 +73,12 @@ export class ChangesetSessionCoordinator extends Disposable { * {@link onSessionRestored} once the working directory is set. */ private readonly _pendingUncommittedRefreshes = new Set(); + /** + * Sessions that subscribed to their session-wide branch changeset before + * the working directory was known. Drained alongside uncommitted refreshes + * once restore / materialization has populated the session summary. + */ + private readonly _pendingSessionRefreshes = new Set(); /** * Per-session set of turn ids that have at least one live subscriber to @@ -148,7 +154,7 @@ export class ChangesetSessionCoordinator extends Disposable { /** * Called when a provisional session is materialized (working directory - * becomes known). Drains any uncommitted refresh that was deferred + * becomes known). Drains any static changeset refresh that was deferred * because the working directory was not yet known. */ onSessionMaterialized(sessionStr: string): void { @@ -162,6 +168,7 @@ export class ChangesetSessionCoordinator extends Disposable { */ onSessionDisposed(sessionStr: string): void { this._pendingUncommittedRefreshes.delete(sessionStr); + this._pendingSessionRefreshes.delete(sessionStr); this._subscribedTurns.delete(sessionStr); this._changesetFileMonitor.onSessionDisposed(sessionStr); } @@ -173,9 +180,9 @@ export class ChangesetSessionCoordinator extends Disposable { // ---- Subscription hooks ------------------------------------------------- /** - * Called on every `addSubscriber` 0→1 transition. When `resource` is - * the uncommitted changeset URI, triggers the first git-diff refresh - * (or queues it for later if the working directory is not yet known). + * Called on every `addSubscriber` 0→1 transition. When `resource` is a + * static changeset URI, triggers the first git-diff refresh (or queues + * it for later if the working directory is not yet known). * * Both {@link AgentService.subscribe} and the handshake fast-path * (`ProtocolServerHandler.initialSubscriptions`) call into @@ -190,10 +197,7 @@ export class ChangesetSessionCoordinator extends Disposable { return; } if (parsed?.kind === ChangesetKind.Session) { - // Session-changeset compute uses git when a working dir is - // available and falls back to the SDK edit-tracker otherwise, - // so it doesn't need the same deferral as uncommitted. - this._changesets.refreshSessionChangeset(parsed.sessionUri); + this._triggerSessionRefresh(parsed.sessionUri); this._changesetFileMonitor.trackSessionChanges(resourceStr, parsed.sessionUri); return; } @@ -219,15 +223,15 @@ export class ChangesetSessionCoordinator extends Disposable { // to the changeset URIs directly, and the user has been // editing files manually in the working tree. this._triggerUncommittedRefresh(resourceStr); - this._changesets.refreshSessionChangeset(resourceStr); + this._triggerSessionRefresh(resourceStr); this._changesetFileMonitor.trackSessionChanges(resourceStr, resourceStr); } } /** * Called when a resource's last subscriber drops. Cleans up any - * deferred uncommitted refresh queued for that session — if no one is - * subscribed anymore, there's no point firing it on materialize. + * deferred refresh queued for that session — if no one is subscribed anymore, + * there's no point firing it on materialize. */ onLastSubscriber(resource: URI): void { const resourceStr = resource.toString(); @@ -238,6 +242,7 @@ export class ChangesetSessionCoordinator extends Disposable { return; } if (parsed?.kind === ChangesetKind.Session) { + this._pendingSessionRefreshes.delete(parsed.sessionUri); this._changesetFileMonitor.untrackSessionChanges(resourceStr); return; } @@ -260,8 +265,8 @@ export class ChangesetSessionCoordinator extends Disposable { * parent session is not already live. Non-changeset URIs are ignored. * * This is intentionally narrower than {@link tryHandleSubscribe}: it does - * not compute per-turn / compare changesets and does not register static - * changesets. It exists for the AgentService subscribe path where + * becomes known). Drains any refresh that was deferred because the working + * directory was not yet known. * `addSubscriber` may have already created a placeholder changeset snapshot * before the parent session restore had a chance to apply persisted diffs. */ @@ -432,9 +437,21 @@ export class ChangesetSessionCoordinator extends Disposable { this._changesets.refreshUncommittedChangeset(sessionStr); } + private _triggerSessionRefresh(sessionStr: string): void { + const wd = this._configurationService.getEffectiveWorkingDirectory(sessionStr); + if (!wd) { + this._pendingSessionRefreshes.add(sessionStr); + return; + } + this._changesets.refreshSessionChangeset(sessionStr); + } + private _drainPendingRefresh(sessionStr: string): void { if (this._pendingUncommittedRefreshes.delete(sessionStr)) { this._triggerUncommittedRefresh(sessionStr); } + if (this._pendingSessionRefreshes.delete(sessionStr)) { + this._triggerSessionRefresh(sessionStr); + } } } diff --git a/src/vs/platform/agentHost/test/node/agentHostChangesetCoordinator.test.ts b/src/vs/platform/agentHost/test/node/agentHostChangesetCoordinator.test.ts index 84aeaceb4389a..5579b628aaa25 100644 --- a/src/vs/platform/agentHost/test/node/agentHostChangesetCoordinator.test.ts +++ b/src/vs/platform/agentHost/test/node/agentHostChangesetCoordinator.test.ts @@ -10,7 +10,7 @@ import { URI } from '../../../../base/common/uri.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; import { NullLogService } from '../../../log/common/log.js'; import { AgentSession, IAgentSessionMetadata } from '../../common/agentService.js'; -import { buildDefaultChangesetCatalogue, buildUncommittedChangesetUri } from '../../common/changesetUri.js'; +import { buildDefaultChangesetCatalogue, buildSessionChangesetUri, buildUncommittedChangesetUri } from '../../common/changesetUri.js'; import { ActionType } from '../../common/state/sessionActions.js'; import { buildSubagentSessionUri, SessionStatus, type ISessionFileDiff } from '../../common/state/sessionState.js'; import { AgentConfigurationService } from '../../node/agentConfigurationService.js'; @@ -127,6 +127,38 @@ suite('ChangesetSessionCoordinator', () => { }); }); + test('defers session changeset refresh until the working directory is known', async () => { + const session = AgentSession.uri('mock', 'session-1').toString(); + const environment = createEnvironment(); + createSession(environment.stateManager, session, undefined, false); + + environment.coordinator.onFirstSubscriber(URI.parse(buildSessionChangesetUri(session))); + await tick(); + + const summary = environment.stateManager.getSessionState(session)!.summary; + environment.stateManager.markSessionPersisted(session, { ...summary, workingDirectory: 'file:///repo/worktree' }); + environment.coordinator.onSessionMaterialized(session); + await tick(); + + assert.deepStrictEqual(environment.changesets.sessionRefreshes, [session]); + }); + + test('drops pending session changeset refresh when the last subscriber leaves', async () => { + const session = AgentSession.uri('mock', 'session-1').toString(); + const environment = createEnvironment(); + const changeset = buildSessionChangesetUri(session); + createSession(environment.stateManager, session, undefined, false); + + environment.coordinator.onFirstSubscriber(URI.parse(changeset)); + environment.coordinator.onLastSubscriber(URI.parse(changeset)); + const summary = environment.stateManager.getSessionState(session)!.summary; + environment.stateManager.markSessionPersisted(session, { ...summary, workingDirectory: 'file:///repo/worktree' }); + environment.coordinator.onSessionMaterialized(session); + await tick(); + + assert.deepStrictEqual(environment.changesets.sessionRefreshes, []); + }); + test('does not attach root state when watcher acquisition fails', async () => { const session = AgentSession.uri('mock', 'session-1').toString(); const environment = createEnvironment(); From b2f3bf797abde2e23e95ababf23be5bd001a7e9a Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Thu, 28 May 2026 22:11:41 +1000 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../platform/agentHost/node/agentHostChangesetCoordinator.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vs/platform/agentHost/node/agentHostChangesetCoordinator.ts b/src/vs/platform/agentHost/node/agentHostChangesetCoordinator.ts index 00a8db7da29fc..ae70dd72b1ba1 100644 --- a/src/vs/platform/agentHost/node/agentHostChangesetCoordinator.ts +++ b/src/vs/platform/agentHost/node/agentHostChangesetCoordinator.ts @@ -265,8 +265,8 @@ export class ChangesetSessionCoordinator extends Disposable { * parent session is not already live. Non-changeset URIs are ignored. * * This is intentionally narrower than {@link tryHandleSubscribe}: it does - * becomes known). Drains any refresh that was deferred because the working - * directory was not yet known. + * not compute per-turn / compare changesets and does not register static + * changesets. It exists for the AgentService subscribe path where * `addSubscriber` may have already created a placeholder changeset snapshot * before the parent session restore had a chance to apply persisted diffs. */