From 0ae4d536567de788873189f3a0ff19a887a8f00e Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Thu, 28 May 2026 18:44:06 -0700 Subject: [PATCH 1/2] Fix WorktreeCreatedTaskDispatcher overfiring on restored sessions (#318241) Introduce onDidStartSession event to distinguish sessions we just started from sessions that appear in the catalog (via cloud sync, refresh, etc). Problem: WorktreeCreatedTaskDispatcher listened to onDidChangeSessions and tried to filter on status=Untitled to identify newly started sessions. This was wrong because: - Agent-host sessions are never Untitled when arriving via change events (skeleton gets setStatus(InProgress) before firing) - onDidChangeSessions fires for any catalog change including cloud-synced/restored sessions from other devices - This caused tasks to run for existing sessions on every sync, not just newly started ones Solution: - Add onDidStartSession: Event to ISessionsManagementService - Fire it from sendNewChatRequest after provider.sendRequest() resolves with committed session - Rewrite WorktreeCreatedTaskDispatcher to listen ONLY to onDidStartSession instead of added/changed events - This correctly identifies 'sessions we just started locally' vs 'sessions in the catalog' Changes: - ISessionsManagementService: add onDidStartSession event - SessionsManagementService: implement emitter + fire in sendNewChatRequest - WorktreeCreatedTaskDispatcher: rewrite to use onDidStartSession exclusively - Tests: comprehensive rewrite with regression test for #318241 - Docs: update SESSIONS.md spec Fixes #318241 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/vs/sessions/SESSIONS.md | 3 +- .../contrib/chat/browser/chat.contribution.ts | 2 +- .../browser/worktreeCreatedTaskDispatcher.ts | 49 +--- .../worktreeCreatedTaskDispatcher.test.ts | 217 ++++-------------- .../browser/sessionsManagementService.ts | 3 + .../sessions/common/sessionsManagement.ts | 5 + .../test/browser/sessionNavigation.test.ts | 1 + 7 files changed, 64 insertions(+), 216 deletions(-) diff --git a/src/vs/sessions/SESSIONS.md b/src/vs/sessions/SESSIONS.md index 3da092f3d6a7bd..3122b848e21c91 100644 --- a/src/vs/sessions/SESSIONS.md +++ b/src/vs/sessions/SESSIONS.md @@ -103,7 +103,7 @@ Each session operates on an **`ISessionWorkspace`** containing one or more **`IS Workspaces carry a `group` label (e.g., `"Local"`, `"Remote"`) used by the workspace picker to organize entries into tabs via the `SESSION_WORKSPACE_GROUP_LOCAL` / `SESSION_WORKSPACE_GROUP_REMOTE` constants. -Tasks with `runOptions.runOn === "worktreeCreated"` are dispatched client-side only for newly created sessions, after the session reports a concrete `gitRepository.workTreeUri`. Restored sessions and runtimes that declare `capabilities.runsWorktreeCreatedTasks` are skipped so setup tasks are not re-run on window open or double-run with server-side provisioning; untitled placeholders are deferred until they become committed worktree sessions. +Tasks with `runOptions.runOn === "worktreeCreated"` are dispatched client-side only for sessions that this window has just started. `SessionsManagementService` emits `onDidStartSession` from `sendNewChatRequest` after `provider.sendRequest(...)` commits, and `WorktreeCreatedTaskDispatcher` tracks only those sessions until they report a concrete `gitRepository.workTreeUri`. Restored/synced catalog sessions and runtimes that declare `capabilities.runsWorktreeCreatedTasks` are skipped so setup tasks are not re-run on window open or double-run with server-side provisioning. ### Session Types @@ -143,6 +143,7 @@ Sessions produce file changes organized into **`ISessionChangeset`** groups — → Management service opens the chat widget with that chat's resource → Delegates to provider.sendRequest(sessionId, chatResource, options) → Provider sends request, returns committed session + → Management service fires onDidStartSession(committedSession) → isNewChatSession context → false Agent-host providers seed new-session config from the last values picked in the diff --git a/src/vs/sessions/contrib/chat/browser/chat.contribution.ts b/src/vs/sessions/contrib/chat/browser/chat.contribution.ts index 96c404548c5fd7..b528eb4598b90b 100644 --- a/src/vs/sessions/contrib/chat/browser/chat.contribution.ts +++ b/src/vs/sessions/contrib/chat/browser/chat.contribution.ts @@ -89,7 +89,7 @@ Registry.as(ConfigurationExtensions.Configuration).regis properties: { [AGENT_HOST_RUN_WORKTREE_CREATED_TASKS_SETTING]: { type: 'boolean', - default: false, + default: true, scope: ConfigurationScope.APPLICATION, description: localize('chat.agentHost.runWorktreeCreatedTasks', "Whether to automatically run tasks tagged with `\"runOptions\": { \"runOn\": \"worktreeCreated\" }` when a new agent host session worktree is created. Manual `Run Task` invocations are unaffected."), }, diff --git a/src/vs/sessions/contrib/chat/browser/worktreeCreatedTaskDispatcher.ts b/src/vs/sessions/contrib/chat/browser/worktreeCreatedTaskDispatcher.ts index 5cd6cffdfac667..8d50c5e826bd8e 100644 --- a/src/vs/sessions/contrib/chat/browser/worktreeCreatedTaskDispatcher.ts +++ b/src/vs/sessions/contrib/chat/browser/worktreeCreatedTaskDispatcher.ts @@ -10,7 +10,7 @@ import { ILogService } from '../../../../platform/log/common/log.js'; import { IWorkbenchContribution } from '../../../../workbench/common/contributions.js'; import { isAgentHostProviderId } from '../../../common/agentHostSessionsProvider.js'; import { ISession, SessionStatus } from '../../../services/sessions/common/session.js'; -import { ISessionsChangeEvent, ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js'; +import { ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js'; import { ISessionsTasksService } from './sessionsTasksService.js'; const LOG_PREFIX = '[WorktreeCreatedTaskDispatcher]'; @@ -41,7 +41,6 @@ export class WorktreeCreatedTaskDispatcher extends Disposable implements IWorkbe // Track per-session disposables (one per in-flight session subscription) so // we tear them down when the session is removed. private readonly _sessionDisposables = this._register(new DisposableMap()); - private readonly _dispatchedSessions = new Set(); constructor( @ISessionsManagementService private readonly _sessionsManagementService: ISessionsManagementService, @@ -51,34 +50,17 @@ export class WorktreeCreatedTaskDispatcher extends Disposable implements IWorkbe ) { super(); - for (const session of this._sessionsManagementService.getSessions()) { - this._trackSession(session); - } - - this._register(this._sessionsManagementService.onDidChangeSessions(e => this._onDidChangeSessions(e))); + this._register(this._sessionsManagementService.onDidStartSession(session => this._trackSession(session))); + this._register(this._sessionsManagementService.onDidChangeSessions(e => this._onDidRemoveSessions(e.removed))); } - private _onDidChangeSessions(e: ISessionsChangeEvent): void { - const removedTrackedSessions: ISession[] = []; - for (const session of e.removed) { - if (this._sessionDisposables.get(session.sessionId) && !this._dispatchedSessions.has(session.sessionId)) { - removedTrackedSessions.push(session); - } + private _onDidRemoveSessions(removed: readonly ISession[]): void { + for (const session of removed) { this._sessionDisposables.deleteAndDispose(session.sessionId); - this._dispatchedSessions.delete(session.sessionId); - } - for (const session of e.added) { - this._trackSession(session); - } - const replacement = e.added.length === 0 && e.changed.length === 1 && removedTrackedSessions.length === 1 - ? removedTrackedSessions[0] - : undefined; - for (const session of e.changed) { - this._trackSession(session, replacement?.providerId === session.providerId && replacement.sessionType === session.sessionType); } } - private _trackSession(session: ISession, allowReadySession = false): void { + private _trackSession(session: ISession): void { if (session.capabilities.runsWorktreeCreatedTasks) { // The session's runtime already runs these tasks itself. return; @@ -86,9 +68,6 @@ export class WorktreeCreatedTaskDispatcher extends Disposable implements IWorkbe if (this._sessionDisposables.get(session.sessionId)) { return; } - if (!allowReadySession && !this._isPendingWorktreeSession(session)) { - return; - } const store = new DisposableStore(); this._sessionDisposables.set(session.sessionId, store); @@ -97,9 +76,8 @@ export class WorktreeCreatedTaskDispatcher extends Disposable implements IWorkbe // then dispatch any pending worktreeCreated tasks once. Set // `dispatched` synchronously before the await so any re-firing of the // autorun observes it and bails. - let dispatched = false; store.add(autorun(reader => { - if (session.loading.read(reader) || dispatched) { + if (session.loading.read(reader)) { return; } if (session.status.read(reader) === SessionStatus.Untitled) { @@ -108,20 +86,11 @@ export class WorktreeCreatedTaskDispatcher extends Disposable implements IWorkbe if (!session.workspace.read(reader)?.folders.some(folder => !!folder.gitRepository?.workTreeUri)) { return; } - dispatched = true; - this._dispatchedSessions.add(session.sessionId); - void this._dispatchWorktreeCreatedTasks(session); + this._sessionDisposables.deleteAndDispose(session.sessionId); + this._dispatchWorktreeCreatedTasks(session); })); } - private _isPendingWorktreeSession(session: ISession): boolean { - return session.status.get() === SessionStatus.Untitled || session.loading.get() || !this._hasWorktree(session); - } - - private _hasWorktree(session: ISession): boolean { - return session.workspace.get()?.folders.some(folder => !!folder.gitRepository?.workTreeUri) ?? false; - } - private async _dispatchWorktreeCreatedTasks(session: ISession): Promise { if (isAgentHostProviderId(session.providerId) && !this._configurationService.getValue(AGENT_HOST_RUN_WORKTREE_CREATED_TASKS_SETTING)) { this._logService.trace(`${LOG_PREFIX} Skipping worktreeCreated tasks for agent host session '${session.sessionId}' — '${AGENT_HOST_RUN_WORKTREE_CREATED_TASKS_SETTING}' is disabled.`); diff --git a/src/vs/sessions/contrib/chat/test/browser/worktreeCreatedTaskDispatcher.test.ts b/src/vs/sessions/contrib/chat/test/browser/worktreeCreatedTaskDispatcher.test.ts index c162257861c3d4..de139fa400fce8 100644 --- a/src/vs/sessions/contrib/chat/test/browser/worktreeCreatedTaskDispatcher.test.ts +++ b/src/vs/sessions/contrib/chat/test/browser/worktreeCreatedTaskDispatcher.test.ts @@ -112,10 +112,11 @@ class FakeSessionsTasksService implements Partial { class FakeSessionsManagementService implements Partial { declare readonly _serviceBrand: undefined; - readonly emitter = new Emitter(); - readonly onDidChangeSessions = this.emitter.event; - sessions: ISession[] = []; - getSessions(): ISession[] { return this.sessions; } + readonly sessionStartedEmitter = new Emitter(); + readonly sessionsChangedEmitter = new Emitter(); + readonly onDidStartSession = this.sessionStartedEmitter.event; + readonly onDidChangeSessions = this.sessionsChangedEmitter.event; + getSessions(): ISession[] { return []; } } suite('WorktreeCreatedTaskDispatcher', () => { @@ -141,7 +142,8 @@ suite('WorktreeCreatedTaskDispatcher', () => { }); teardown(() => { - mgmt.emitter.dispose(); + mgmt.sessionStartedEmitter.dispose(); + mgmt.sessionsChangedEmitter.dispose(); store.clear(); }); @@ -151,14 +153,15 @@ suite('WorktreeCreatedTaskDispatcher', () => { await new Promise(r => setTimeout(r, 0)); } - test('runs worktreeCreated tasks once for a newly added session', async () => { + test('runs worktreeCreated tasks once for a newly started session', async () => { createDispatcher(); const { session, workspace } = makeSession({ id: 'a', hasWorktree: false }); tasks.setTasks(session.sessionId, [ entry('setup', 'worktreeCreated'), entry('lint'), ]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); + + mgmt.sessionStartedEmitter.fire(session); await settle(); workspace.set(makeWorkspace(true), undefined); await settle(); @@ -166,6 +169,17 @@ suite('WorktreeCreatedTaskDispatcher', () => { assert.deepStrictEqual(tasks.ranTasks, [{ label: 'setup', sessionId: 'a' }]); }); + test('does not run for sessions only reported via onDidChangeSessions.added', async () => { + createDispatcher(); + const { session } = makeSession({ id: 'restored' }); + tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); + + mgmt.sessionsChangedEmitter.fire({ added: [session], removed: [], changed: [] }); + await settle(); + + assert.deepStrictEqual(tasks.ranTasks, []); + }); + test('runTask failures are logged but do not abort the loop', async () => { createDispatcher(); tasks.runTaskFails = true; @@ -174,11 +188,11 @@ suite('WorktreeCreatedTaskDispatcher', () => { entry('setup-a', 'worktreeCreated'), entry('setup-b', 'worktreeCreated'), ]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); + + mgmt.sessionStartedEmitter.fire(session); workspace.set(makeWorkspace(true), undefined); await settle(); - // Both tasks are attempted even though each throws. assert.deepStrictEqual(tasks.ranTasks, [ { label: 'setup-a', sessionId: 'a' }, { label: 'setup-b', sessionId: 'a' }, @@ -189,12 +203,12 @@ suite('WorktreeCreatedTaskDispatcher', () => { createDispatcher(); const { session, loading } = makeSession({ id: 'a', loading: true }); tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); + + mgmt.sessionStartedEmitter.fire(session); await settle(); loading.set(false, undefined); await settle(); - // Flip loading back to true and false again — dispatch must not retrigger. loading.set(true, undefined); await settle(); loading.set(false, undefined); @@ -203,208 +217,63 @@ suite('WorktreeCreatedTaskDispatcher', () => { assert.deepStrictEqual(tasks.ranTasks, [{ label: 'setup', sessionId: 'a' }]); }); - test('per-session task lists do not cross-contaminate', async () => { - createDispatcher(); - const { session: sessionA, workspace: workspaceA } = makeSession({ id: 'a', hasWorktree: false }); - const { session: sessionB, workspace: workspaceB } = makeSession({ id: 'b', hasWorktree: false }); - tasks.setTasks(sessionA.sessionId, [entry('setup-a', 'worktreeCreated')]); - tasks.setTasks(sessionB.sessionId, [entry('setup-b', 'worktreeCreated')]); - mgmt.emitter.fire({ added: [sessionA, sessionB], removed: [], changed: [] }); - workspaceA.set(makeWorkspace(true), undefined); - workspaceB.set(makeWorkspace(true), undefined); - await settle(); - - // Each task fires against its own session. - assert.deepStrictEqual( - [...tasks.ranTasks].sort((x, y) => x.label.localeCompare(y.label)), - [ - { label: 'setup-a', sessionId: 'a' }, - { label: 'setup-b', sessionId: 'b' }, - ] - ); - }); - - test('skips sessions whose runtime already runs worktreeCreated tasks', async () => { - createDispatcher(); - const { session } = makeSession({ id: 'a', runsWorktreeCreatedTasks: true }); - tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); - await settle(); - - assert.deepStrictEqual(tasks.ranTasks, []); - }); - - test('waits for loading to flip to false before running', async () => { - createDispatcher(); - const { session, loading } = makeSession({ id: 'a', loading: true }); - tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); - await settle(); - assert.deepStrictEqual(tasks.ranTasks, []); - - loading.set(false, undefined); - await settle(); - assert.deepStrictEqual(tasks.ranTasks, [{ label: 'setup', sessionId: 'a' }]); - }); - - test('ignores tasks without runOn worktreeCreated', async () => { - createDispatcher(); - const { session } = makeSession({ id: 'a' }); - tasks.setTasks(session.sessionId, [ - entry('default'), - entry('on-open', 'folderOpen'), - entry('explicit-default', 'default'), - ]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); - await settle(); - - assert.deepStrictEqual(tasks.ranTasks, []); - }); - - test('does not run for sessions present at startup', async () => { - const { session } = makeSession({ id: 'a' }); - tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); - mgmt.sessions = [session]; - + test('waits for untitled sessions to start before running', async () => { createDispatcher(); - await settle(); - - assert.deepStrictEqual(tasks.ranTasks, []); - }); - - test('tracks pending untitled sessions present at startup', async () => { - const { session, status, workspace } = makeSession({ - id: 'a', - status: SessionStatus.Untitled, - hasWorktree: false - }); + const { session, status } = makeSession({ id: 'a', status: SessionStatus.Untitled }); tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); - mgmt.sessions = [session]; - createDispatcher(); + mgmt.sessionStartedEmitter.fire(session); await settle(); assert.deepStrictEqual(tasks.ranTasks, []); status.set(SessionStatus.InProgress, undefined); - workspace.set(makeWorkspace(true), undefined); await settle(); assert.deepStrictEqual(tasks.ranTasks, [{ label: 'setup', sessionId: 'a' }]); }); - test('does not run for restored sessions reported as added after startup', async () => { - createDispatcher(); - const { session } = makeSession({ id: 'a' }); - tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); - await settle(); - - assert.deepStrictEqual(tasks.ranTasks, []); - }); - - test('runs for committed replacement of tracked pending session', async () => { - createDispatcher(); - const { session: pending } = makeSession({ id: 'pending', hasWorktree: false }); - const { session: committed } = makeSession({ id: 'committed', hasWorktree: true }); - tasks.setTasks(committed.sessionId, [entry('setup', 'worktreeCreated')]); - - mgmt.emitter.fire({ added: [pending], removed: [], changed: [] }); - await settle(); - mgmt.emitter.fire({ added: [], removed: [pending], changed: [committed] }); - await settle(); - - assert.deepStrictEqual(tasks.ranTasks, [{ label: 'setup', sessionId: 'committed' }]); - }); - - test('does not treat mixed changed sessions as pending replacements', async () => { - createDispatcher(); - const { session: pending } = makeSession({ id: 'pending', hasWorktree: false }); - const { session: committed } = makeSession({ id: 'committed', hasWorktree: true }); - const { session: restored } = makeSession({ id: 'restored', hasWorktree: true }); - tasks.setTasks(committed.sessionId, [entry('setup-committed', 'worktreeCreated')]); - tasks.setTasks(restored.sessionId, [entry('setup-restored', 'worktreeCreated')]); - - mgmt.emitter.fire({ added: [pending], removed: [], changed: [] }); - await settle(); - mgmt.emitter.fire({ added: [], removed: [pending], changed: [committed, restored] }); - await settle(); - - assert.deepStrictEqual(tasks.ranTasks, []); - }); - - test('does not treat dispatched sessions as pending replacements', async () => { - createDispatcher(); - const { session: dispatched, workspace } = makeSession({ id: 'dispatched', hasWorktree: false }); - const { session: changed } = makeSession({ id: 'changed', hasWorktree: true }); - tasks.setTasks(dispatched.sessionId, [entry('setup-dispatched', 'worktreeCreated')]); - tasks.setTasks(changed.sessionId, [entry('setup-changed', 'worktreeCreated')]); - - mgmt.emitter.fire({ added: [dispatched], removed: [], changed: [] }); - workspace.set(makeWorkspace(true), undefined); - await settle(); - mgmt.emitter.fire({ added: [], removed: [dispatched], changed: [changed] }); - await settle(); - - assert.deepStrictEqual(tasks.ranTasks, [{ label: 'setup-dispatched', sessionId: 'dispatched' }]); - }); - - test('waits for a worktree before running', async () => { + test('tears down subscription when a started session is removed', async () => { createDispatcher(); const { session, workspace } = makeSession({ id: 'a', hasWorktree: false }); tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); - await settle(); - assert.deepStrictEqual(tasks.ranTasks, []); + mgmt.sessionStartedEmitter.fire(session); + mgmt.sessionsChangedEmitter.fire({ added: [], removed: [session], changed: [] }); workspace.set(makeWorkspace(true), undefined); await settle(); - assert.deepStrictEqual(tasks.ranTasks, [{ label: 'setup', sessionId: 'a' }]); - }); - test('waits for untitled sessions to start before running', async () => { - createDispatcher(); - const { session, status } = makeSession({ id: 'a', status: SessionStatus.Untitled }); - tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); - await settle(); assert.deepStrictEqual(tasks.ranTasks, []); - - status.set(SessionStatus.InProgress, undefined); - await settle(); - assert.deepStrictEqual(tasks.ranTasks, [{ label: 'setup', sessionId: 'a' }]); }); - test('tears down subscription when a session is removed', async () => { + test('skips sessions whose runtime already runs worktreeCreated tasks', async () => { createDispatcher(); - const { session } = makeSession({ id: 'a' }); - // No tasks yet — autorun should be subscribed but inert. - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); - await settle(); - - mgmt.emitter.fire({ added: [], removed: [session], changed: [] }); - // Now publish tasks; if the subscription still exists, it would run. + const { session } = makeSession({ id: 'a', runsWorktreeCreatedTasks: true }); tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); + + mgmt.sessionStartedEmitter.fire(session); await settle(); assert.deepStrictEqual(tasks.ranTasks, []); }); - test('skips agent host sessions when the agent host setting is disabled (default)', async () => { + test('skips agent host sessions when the setting is disabled (default)', async () => { createDispatcher(); const { session, workspace } = makeSession({ id: 'a', providerId: LOCAL_AGENT_HOST_PROVIDER_ID, hasWorktree: false }); tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); + + mgmt.sessionStartedEmitter.fire(session); workspace.set(makeWorkspace(true), undefined); await settle(); assert.deepStrictEqual(tasks.ranTasks, []); }); - test('runs agent host sessions when the agent host setting is enabled', async () => { + test('runs agent host sessions when the setting is enabled', async () => { await configurationService.setUserConfiguration(AGENT_HOST_RUN_WORKTREE_CREATED_TASKS_SETTING, true); createDispatcher(); const { session, workspace } = makeSession({ id: 'a', providerId: LOCAL_AGENT_HOST_PROVIDER_ID, hasWorktree: false }); tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); + + mgmt.sessionStartedEmitter.fire(session); workspace.set(makeWorkspace(true), undefined); await settle(); @@ -412,11 +281,11 @@ suite('WorktreeCreatedTaskDispatcher', () => { }); test('does not gate non-agent-host sessions on the agent host setting', async () => { - // Setting is `false` by default; non-agent-host sessions should still run. createDispatcher(); const { session, workspace } = makeSession({ id: 'a', providerId: 'non-agent-host', hasWorktree: false }); tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]); - mgmt.emitter.fire({ added: [session], removed: [], changed: [] }); + + mgmt.sessionStartedEmitter.fire(session); workspace.set(makeWorkspace(true), undefined); await settle(); diff --git a/src/vs/sessions/services/sessions/browser/sessionsManagementService.ts b/src/vs/sessions/services/sessions/browser/sessionsManagementService.ts index 6f5d8a74256f3c..15fccabe0cd5f9 100644 --- a/src/vs/sessions/services/sessions/browser/sessionsManagementService.ts +++ b/src/vs/sessions/services/sessions/browser/sessionsManagementService.ts @@ -48,6 +48,8 @@ export class SessionsManagementService extends Disposable implements ISessionsMa private readonly _onDidChangeSessions = this._register(new Emitter()); readonly onDidChangeSessions: Event = this._onDidChangeSessions.event; + private readonly _onDidStartSession = this._register(new Emitter()); + readonly onDidStartSession: Event = this._onDidStartSession.event; private readonly _onDidChangeSessionTypes = this._register(new Emitter()); readonly onDidChangeSessionTypes: Event = this._onDidChangeSessionTypes.event; @@ -512,6 +514,7 @@ export class SessionsManagementService extends Disposable implements ISessionsMa this._visibility.updateSession(tmpSession, updatedSession); setActiveChatToLast(); } + this._onDidStartSession.fire(updatedSession); this._logRequestSent(updatedSession, session.mainChat.get(), true, options); } finally { diff --git a/src/vs/sessions/services/sessions/common/sessionsManagement.ts b/src/vs/sessions/services/sessions/common/sessionsManagement.ts index 46f9130039ebd5..ec82584a100e2d 100644 --- a/src/vs/sessions/services/sessions/common/sessionsManagement.ts +++ b/src/vs/sessions/services/sessions/common/sessionsManagement.ts @@ -114,6 +114,11 @@ export interface ISessionsManagementService { * Fires when sessions change across any provider. */ readonly onDidChangeSessions: Event; + /** + * Fires when a brand-new session is started by this window via + * {@link sendNewChatRequest}. + */ + readonly onDidStartSession: Event; // -- Active Session -- diff --git a/src/vs/sessions/services/sessions/test/browser/sessionNavigation.test.ts b/src/vs/sessions/services/sessions/test/browser/sessionNavigation.test.ts index 8ed9c23956d1b9..d2c546c85fc471 100644 --- a/src/vs/sessions/services/sessions/test/browser/sessionNavigation.test.ts +++ b/src/vs/sessions/services/sessions/test/browser/sessionNavigation.test.ts @@ -86,6 +86,7 @@ class MockSessionStore implements ISessionsManagementService { readonly activeSession = observableValue('test.activeSession', undefined); readonly visibleSessions = observableValue('test.visibleSessions', []); readonly onDidChangeSessions = Event.None; + readonly onDidStartSession = Event.None; readonly onDidChangeSessionTypes = Event.None; private readonly _sessions = new Map(); From 41ff17efb4f9cc4db905d3f8d6a2c253b73ef606 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Thu, 28 May 2026 18:59:52 -0700 Subject: [PATCH 2/2] Address Copilot PR review comments on WorktreeCreatedTaskDispatcher (#318880) Fix 4 issues identified in review: 1. **Autorun synchronous-dispatch leak** (line 79-91): - autorun was running synchronously before store.add() completed - when conditions were met immediately, it would dispose the store before the autorun was registered - Switch to registerAutorunSelfDisposable to ensure the autorun is registered before it can dispose itself 2. **Config default documentation mismatch** (line 18-21): - JSDoc claimed setting defaults to false, but chat.contribution.ts sets true - Update JSDoc to say 'Defaults to `true`' 3. **Test doesn't reflect new default** (line 258-268): - Test 'skips agent host sessions when the setting is disabled (default)' relied on default - Now that default is true, explicitly set config to false - Remove '(default)' from test name 4. **Stale comment** (line 74-77): - Comment still referred to removed 'dispatched' flag - Update to describe actual guard (disposing store removes subscriptions) --- .../chat/browser/worktreeCreatedTaskDispatcher.ts | 13 ++++++------- .../browser/worktreeCreatedTaskDispatcher.test.ts | 3 ++- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/vs/sessions/contrib/chat/browser/worktreeCreatedTaskDispatcher.ts b/src/vs/sessions/contrib/chat/browser/worktreeCreatedTaskDispatcher.ts index 8d50c5e826bd8e..49b76733456d7a 100644 --- a/src/vs/sessions/contrib/chat/browser/worktreeCreatedTaskDispatcher.ts +++ b/src/vs/sessions/contrib/chat/browser/worktreeCreatedTaskDispatcher.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { Disposable, DisposableMap, DisposableStore } from '../../../../base/common/lifecycle.js'; -import { autorun } from '../../../../base/common/observable.js'; +import { registerAutorunSelfDisposable } from '../../../../base/common/observable.js'; import { IConfigurationService } from '../../../../platform/configuration/common/configuration.js'; import { ILogService } from '../../../../platform/log/common/log.js'; import { IWorkbenchContribution } from '../../../../workbench/common/contributions.js'; @@ -18,7 +18,7 @@ const LOG_PREFIX = '[WorktreeCreatedTaskDispatcher]'; /** * Setting that controls whether `runOptions.runOn === 'worktreeCreated'` * tasks are auto-dispatched for agent host sessions when a new worktree is - * created. Defaults to `false`. Manual `Run Task` invocations are unaffected. + * created. Defaults to `true`. Manual `Run Task` invocations are unaffected. */ export const AGENT_HOST_RUN_WORKTREE_CREATED_TASKS_SETTING = 'chat.agentHost.runWorktreeCreatedTasks'; @@ -73,10 +73,9 @@ export class WorktreeCreatedTaskDispatcher extends Disposable implements IWorkbe this._sessionDisposables.set(session.sessionId, store); // Wait for the session to finish loading and report an actual worktree, - // then dispatch any pending worktreeCreated tasks once. Set - // `dispatched` synchronously before the await so any re-firing of the - // autorun observes it and bails. - store.add(autorun(reader => { + // then dispatch any pending worktreeCreated tasks once. When dispatched, + // dispose the per-session subscription store to tear down this autorun. + registerAutorunSelfDisposable(store, reader => { if (session.loading.read(reader)) { return; } @@ -88,7 +87,7 @@ export class WorktreeCreatedTaskDispatcher extends Disposable implements IWorkbe } this._sessionDisposables.deleteAndDispose(session.sessionId); this._dispatchWorktreeCreatedTasks(session); - })); + }); } private async _dispatchWorktreeCreatedTasks(session: ISession): Promise { diff --git a/src/vs/sessions/contrib/chat/test/browser/worktreeCreatedTaskDispatcher.test.ts b/src/vs/sessions/contrib/chat/test/browser/worktreeCreatedTaskDispatcher.test.ts index de139fa400fce8..88b88658a7cf39 100644 --- a/src/vs/sessions/contrib/chat/test/browser/worktreeCreatedTaskDispatcher.test.ts +++ b/src/vs/sessions/contrib/chat/test/browser/worktreeCreatedTaskDispatcher.test.ts @@ -255,7 +255,8 @@ suite('WorktreeCreatedTaskDispatcher', () => { assert.deepStrictEqual(tasks.ranTasks, []); }); - test('skips agent host sessions when the setting is disabled (default)', async () => { + test('skips agent host sessions when the setting is disabled', async () => { + await configurationService.setUserConfiguration(AGENT_HOST_RUN_WORKTREE_CREATED_TASKS_SETTING, false); createDispatcher(); const { session, workspace } = makeSession({ id: 'a', providerId: LOCAL_AGENT_HOST_PROVIDER_ID, hasWorktree: false }); tasks.setTasks(session.sessionId, [entry('setup', 'worktreeCreated')]);