diff --git a/docs/design/experimental-session-plan-review.md b/docs/design/experimental-session-plan-review.md index 4664e373daf..2dee067e962 100644 --- a/docs/design/experimental-session-plan-review.md +++ b/docs/design/experimental-session-plan-review.md @@ -38,8 +38,8 @@ approving exits Plan Mode. `exit_plan_mode` approval request. - Resolve the approval DAG from that identity instead of the latest active Todo list. -- Preserve the approved plan identity while later snapshots and Agent - executions update its status. +- Reuse the existing plan ID lineage so later snapshots and Agent executions + continue updating the same Workflow without another store. - Fall back to the existing text-only approval when no matching snapshot is available. diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index b32942d4968..c20f5df4d8d 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -1834,6 +1834,7 @@ describe('QwenAgent MCP SSE/HTTP support', () => { restoreHistory: ReturnType; rewindToTurn: ReturnType; getRewindableUserTurnCount: ReturnType; + clearActiveTodoPlanRevision: ReturnType; clearTodoStopGuardTrust: ReturnType; hardSuspendTodoStopGuard: ReturnType; beginCloseIfAvailable: ReturnType; @@ -3390,6 +3391,7 @@ describe('QwenAgent MCP SSE/HTTP support', () => { .fn() .mockReturnValue({ targetTurnIndex: 1, apiTruncateIndex: 2 }), getRewindableUserTurnCount: vi.fn().mockReturnValue(1), + clearActiveTodoPlanRevision: vi.fn(), clearTodoStopGuardTrust: vi.fn(), hardSuspendTodoStopGuard: vi.fn(), releaseTodoStopGuardQueuedPromptWait: vi.fn().mockReturnValue(true), @@ -3792,7 +3794,24 @@ describe('QwenAgent MCP SSE/HTTP support', () => { mode: 'plan', }), ).resolves.toEqual({ previous: 'default', current: 'plan' }); + expect( + lastSessionMock?.clearActiveTodoPlanRevision, + ).toHaveBeenCalledOnce(); expect(lastSessionMock?.clearTodoStopGuardTrust).toHaveBeenCalledOnce(); + + // Re-selecting plan (the Web Shell /plan path) must keep the revision + // captured during the current plan cycle, while the stop guard trust + // still clears, as it does on every transition into plan. + await expect( + agent.extMethod(SERVE_CONTROL_EXT_METHODS.sessionApprovalMode, { + sessionId, + mode: 'plan', + }), + ).resolves.toEqual({ previous: 'plan', current: 'plan' }); + expect( + lastSessionMock?.clearActiveTodoPlanRevision, + ).toHaveBeenCalledOnce(); + expect(lastSessionMock?.clearTodoStopGuardTrust).toHaveBeenCalledTimes(2); } finally { approvalModes.splice(0, approvalModes.length, ...originalApprovalModes); } @@ -13681,6 +13700,7 @@ describe('QwenAgent loadSession / unstable_resumeSession', () => { cancelPendingPrompt: vi.fn().mockResolvedValue(undefined), assertCanStartTurn: vi.fn().mockResolvedValue(undefined), sendUpdate: vi.fn().mockResolvedValue(undefined), + clearActiveTodoPlanRevision: vi.fn(), dispose: vi.fn(), }; lastSessionMock = sessionMock; @@ -14222,8 +14242,13 @@ describe('QwenAgent loadSession / unstable_resumeSession', () => { }, }); const replayUpdate = { - sessionUpdate: 'agent_message_chunk', - _meta: { timestamp: 4242 }, + sessionUpdate: 'plan', + entries: [{ content: 'Ship', priority: 'medium', status: 'pending' }], + _meta: { + timestamp: 4242, + qwenTodoPlan: { id: 'plan-1' }, + qwenTranscript: { planToolCallId: 'todo-call-1' }, + }, }; mockHistoryReplay.mockImplementation( async ( @@ -14305,6 +14330,66 @@ describe('QwenAgent loadSession / unstable_resumeSession', () => { await agentPromise; }); + it('clears the replayed Todo plan revision on a live-session load', async () => { + const messages = [{ role: 'user', parts: [{ text: 'hi' }] }]; + const innerConfig = makeRestoreInnerConfig({ + resumedConversation: { messages }, + }); + innerConfig.getApprovalMode.mockReturnValue('plan'); + innerConfig.getSessionService.mockReturnValue({ + loadSession: vi + .fn() + .mockImplementation(() => innerConfig.getResumedSessionData()), + }); + vi.mocked(loadSettings).mockReturnValue(makeRestoreSettings()); + const replayUpdate = { + sessionUpdate: 'plan', + entries: [{ content: 'Old plan', priority: 'medium', status: 'done' }], + _meta: { + timestamp: 4242, + qwenTodoPlan: { id: 'old-plan' }, + qwenTranscript: { planToolCallId: 'old-call' }, + }, + }; + mockHistoryReplay.mockImplementation(async (context: unknown) => { + await ( + context as { sendUpdate: (update: unknown) => Promise } + ).sendUpdate(replayUpdate); + }); + const liveSession = { + getId: vi.fn().mockReturnValue('persisted-1'), + getConfig: vi.fn().mockReturnValue(innerConfig), + assertCanStartTurn: vi.fn().mockResolvedValue(undefined), + beginClose: vi.fn().mockReturnValue(vi.fn()), + waitForActiveTurnsToSettle: vi.fn().mockResolvedValue(undefined), + sendUpdate: vi.fn().mockResolvedValue(undefined), + clearActiveTodoPlanRevision: vi.fn(), + }; + const { agent, agentPromise } = await spawnAgent(); + (agent as unknown as { sessions: Map }).sessions.set( + 'persisted-1', + liveSession, + ); + + await agent.loadSession({ + cwd: '/tmp', + sessionId: 'persisted-1', + mcpServers: [], + }); + + expect(liveSession.sendUpdate).toHaveBeenCalledWith({ + ...replayUpdate, + timestamp: 4242, + }); + expect(liveSession.clearActiveTodoPlanRevision).toHaveBeenCalledOnce(); + expect( + liveSession.clearActiveTodoPlanRevision.mock.invocationCallOrder[0], + ).toBeGreaterThan(liveSession.sendUpdate.mock.invocationCallOrder.at(-1)!); + + mockConnectionState.resolve(); + await agentPromise; + }); + it('loadSession limits bulk replay to complete recent turns', async () => { const makeMessage = ( uuid: string, @@ -16635,6 +16720,7 @@ describe('sessionLanguage multi-session propagation', () => { }), setDisabledTools: vi.fn(), }); + const clearActiveTodoPlanRevision = vi.fn(); const clearTodoStopGuardTrust = vi.fn(); vi.mocked(loadSettings).mockReturnValue(settings); @@ -16645,6 +16731,7 @@ describe('sessionLanguage multi-session propagation', () => { getId: vi.fn().mockReturnValue('s-plan-reload'), getConfig: vi.fn().mockReturnValue(cfg), isIdle: vi.fn().mockReturnValue(true), + clearActiveTodoPlanRevision, clearTodoStopGuardTrust, sendAvailableCommandsUpdate: vi.fn().mockResolvedValue(undefined), installRewriter: vi.fn(), @@ -16685,6 +16772,7 @@ describe('sessionLanguage multi-session propagation', () => { (cfg as typeof cfg & { setApprovalMode: ReturnType }) .setApprovalMode, ).toHaveBeenCalledWith('plan'); + expect(clearActiveTodoPlanRevision).toHaveBeenCalledOnce(); expect(clearTodoStopGuardTrust).toHaveBeenCalledOnce(); } finally { approvalModes.splice(0, approvalModes.length, ...originalApprovalModes); diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index e02d84b7cb6..a2d811d40e2 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -4537,8 +4537,18 @@ class QwenAgent implements Agent { logger: debugLogger, }); if (!bulkReplay) { - for (const update of replay.updates) { - await liveSession.sendUpdate(update); + try { + for (const update of replay.updates) { + await liveSession.sendUpdate(update); + } + } finally { + // Replayed plan updates re-stamp the revision via sendUpdate; + // drop it so a replayed snapshot cannot bind a later approval + // (same rule Session.replayHistory applies to cold loads), + // even if delivery fails part-way. The bulk path keeps a live + // binding on purpose: it hands the updates to the client + // instead of replaying them through this session. + liveSession.clearActiveTodoPlanRevision(); } if (replay.replayError !== undefined) { throw RequestError.internalError(undefined, replay.replayError); @@ -9161,6 +9171,9 @@ class QwenAgent implements Agent { } const current = config.getApprovalMode(); if (current === 'plan') { + if (previous !== 'plan') { + session.clearActiveTodoPlanRevision(); + } session.clearTodoStopGuardTrust(); } return { previous, current }; @@ -10640,6 +10653,7 @@ class QwenAgent implements Agent { try { config.setApprovalMode(newMode as ApprovalMode); if (newMode === 'plan') { + session.clearActiveTodoPlanRevision(); session.clearTodoStopGuardTrust(); } } catch (err) { diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index b7e9498ff8e..282fc0ad27a 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -43,6 +43,7 @@ import type { PromptRequest, RequestPermissionResponse, SessionNotification, + SessionUpdate, } from '@agentclientprotocol/sdk'; import type { LoadedSettings } from '../../config/settings.js'; import * as nonInteractiveCliCommands from '../../nonInteractiveCliCommands.js'; @@ -1919,6 +1920,83 @@ describe('Session', () => { }); }); + // Runs a full exit_plan_mode approval turn and returns the permission + // request the client received, so tests can assert the observable + // `_meta.qwenTodoApproval` binding instead of poking the private + // `activeTodoPlanRevision` field. Mirrors the it.each harness in the + // prompt describe block. + async function runExitPlanModeApprovalPrompt(): Promise< + Parameters[0] + > { + let mode = ApprovalMode.PLAN; + const hookSpy = vi + .spyOn(core, 'firePermissionRequestHook') + .mockResolvedValue({ + hasDecision: true, + shouldAllow: true, + updatedInput: { plan: 'Hook-replaced plan' }, + denyMessage: undefined, + }); + const invocation = { + params: { plan: 'Original plan' }, + requiresUserInteraction: vi.fn().mockReturnValue(true), + getDefaultPermission: vi.fn().mockResolvedValue('ask'), + getConfirmationDetails: vi.fn().mockResolvedValue({ + type: 'plan', + title: 'Approve plan', + plan: 'Original plan', + hideAlwaysAllow: true, + onConfirm: vi.fn().mockResolvedValue(undefined), + }), + getDescription: vi.fn().mockReturnValue('Plan:'), + toolLocations: vi.fn().mockReturnValue([]), + execute: vi.fn().mockImplementation(async () => { + mode = ApprovalMode.DEFAULT; + return { llmContent: 'approved', returnDisplay: 'approved' }; + }), + }; + const tool = { + name: core.ToolNames.EXIT_PLAN_MODE, + kind: core.Kind.Think, + build: vi.fn().mockReturnValue(invocation), + }; + + mockToolRegistry.getTool.mockReturnValue(tool); + mockConfig.getApprovalMode = vi.fn(() => mode); + mockConfig.getPermissionManager = vi.fn().mockReturnValue(null); + mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(false); + mockConfig.getMessageBus = vi.fn().mockReturnValue({}); + mockChat.sendMessageStream = vi.fn().mockResolvedValue( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'call-exit-plan', + name: core.ToolNames.EXIT_PLAN_MODE, + args: { plan: 'Original plan' }, + }, + ], + }, + }, + ]), + ); + + try { + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'approve the plan' }], + }); + } finally { + hookSpy.mockRestore(); + } + + const calls = vi.mocked(mockClient.requestPermission).mock.calls; + expect(calls.length).toBeGreaterThan(0); + return calls.at(-1)![0]; + } + describe('setMode', () => { it.each([ ['plan', ApprovalMode.PLAN], @@ -1965,6 +2043,56 @@ describe('Session', () => { expect.anything(), ); }); + + it('clears the active Todo plan revision when transitioning into plan mode', async () => { + mockConfig.getApprovalMode = vi + .fn() + .mockReturnValue(ApprovalMode.DEFAULT); + await session.sendUpdate({ + sessionUpdate: 'plan', + entries: [{ content: 'Ship', priority: 'medium', status: 'pending' }], + _meta: { + qwenTodoPlan: { id: 'plan-1' }, + qwenTranscript: { planToolCallId: 'todo-call-1' }, + }, + }); + + await session.setMode({ + sessionId: 'test-session-id', + modeId: 'plan', + }); + + const request = await runExitPlanModeApprovalPrompt(); + expect(request.toolCall._meta).toEqual( + expect.not.objectContaining({ + qwenTodoApproval: expect.anything(), + }), + ); + }); + + it('preserves the active Todo plan revision when re-selecting plan mode', async () => { + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.PLAN); + await session.sendUpdate({ + sessionUpdate: 'plan', + entries: [{ content: 'Ship', priority: 'medium', status: 'pending' }], + _meta: { + qwenTodoPlan: { id: 'plan-1' }, + qwenTranscript: { planToolCallId: 'todo-call-1' }, + }, + }); + + await session.setMode({ + sessionId: 'test-session-id', + modeId: 'plan', + }); + + const request = await runExitPlanModeApprovalPrompt(); + expect(request.toolCall._meta).toEqual( + expect.objectContaining({ + qwenTodoApproval: { planId: 'plan-1', sourceCallId: 'todo-call-1' }, + }), + ); + }); }); describe('sendCurrentModeUpdateNotification', () => { @@ -2017,7 +2145,7 @@ describe('Session', () => { }); describe('rewindToTurn', () => { - it('truncates model history before the requested user turn and records rewind', () => { + it('truncates model history before the requested user turn and records rewind', async () => { const history: Content[] = [ { role: 'user', parts: [{ text: 'first' }] }, { role: 'model', parts: [{ text: 'first reply' }] }, @@ -2026,12 +2154,27 @@ describe('Session', () => { ]; vi.mocked(mockChat.getHistory).mockReturnValue(history); vi.mocked(mockChat.getHistoryShallow).mockReturnValue(history); + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.PLAN); + await session.sendUpdate({ + sessionUpdate: 'plan', + entries: [{ content: 'old', priority: 'medium', status: 'pending' }], + _meta: { + qwenTodoPlan: { id: 'old-plan' }, + qwenTranscript: { planToolCallId: 'old-call' }, + }, + }); const result = session.rewindToTurn(1); expect(result).toEqual({ targetTurnIndex: 1, apiTruncateIndex: 2 }); expect(mockChat.truncateHistory).toHaveBeenCalledWith(2); expect(mockChat.stripThoughtsFromHistory).toHaveBeenCalled(); + const request = await runExitPlanModeApprovalPrompt(); + expect(request.toolCall._meta).toEqual( + expect.not.objectContaining({ + qwenTodoApproval: expect.anything(), + }), + ); expect(mockChatRecordingService.rewindRecording).toHaveBeenCalledWith( 1, { truncatedCount: 2 }, @@ -2317,6 +2460,32 @@ describe('Session', () => { expect(mockChat.getHistory).not.toHaveBeenCalled(); }); + it('clears the active Todo plan revision when restoring history', async () => { + await session.sendUpdate({ + sessionUpdate: 'plan', + entries: [{ content: 'old', priority: 'medium', status: 'pending' }], + _meta: { + qwenTodoPlan: { id: 'old-plan' }, + qwenTranscript: { planToolCallId: 'old-call' }, + }, + }); + const bound = await runExitPlanModeApprovalPrompt(); + expect(bound.toolCall._meta).toEqual( + expect.objectContaining({ + qwenTodoApproval: { planId: 'old-plan', sourceCallId: 'old-call' }, + }), + ); + + session.restoreHistory([]); + + const restored = await runExitPlanModeApprovalPrompt(); + expect(restored.toolCall._meta).toEqual( + expect.not.objectContaining({ + qwenTodoApproval: expect.anything(), + }), + ); + }); + it('rejects history restore while a prompt is running', () => { (session as unknown as { pendingPrompt: AbortController }).pendingPrompt = new AbortController(); @@ -13614,45 +13783,230 @@ describe('Session', () => { ); }); - it('keeps exit_plan_mode in PLAN until ACP approval executes and then notifies once', async () => { - let mode = ApprovalMode.PLAN; - const hookSpy = vi - .spyOn(core, 'firePermissionRequestHook') - .mockResolvedValue({ - hasDecision: true, - shouldAllow: true, - updatedInput: { plan: 'Hook-replaced plan' }, - denyMessage: undefined, + it.each([ + ['live update', 'live', true], + ['history replay', 'replay', false], + ['failed replacement', 'failed', false], + ['mode transition', 'cleared', false], + ['empty plan update', 'empty-entries', false], + ['plan update without identity', 'missing-meta', false], + ] as const)( + 'keeps exit_plan_mode approval revision correct after %s', + async (_label, revisionSource, expectsRevision) => { + let mode = ApprovalMode.PLAN; + const hookSpy = vi + .spyOn(core, 'firePermissionRequestHook') + .mockResolvedValue({ + hasDecision: true, + shouldAllow: true, + updatedInput: { plan: 'Hook-replaced plan' }, + denyMessage: undefined, + }); + const onConfirmSpy = vi.fn().mockResolvedValue(undefined); + const executeSpy = vi.fn().mockImplementation(async () => { + const updatesBeforeExecute = vi + .mocked(mockClient.sessionUpdate) + .mock.calls.filter( + ([params]) => + params.update.sessionUpdate === 'current_mode_update', + ); + expect(mode).toBe(ApprovalMode.PLAN); + expect(updatesBeforeExecute).toHaveLength(0); + mode = ApprovalMode.DEFAULT; + return { llmContent: 'approved', returnDisplay: 'approved' }; }); - const onConfirmSpy = vi.fn().mockResolvedValue(undefined); - const executeSpy = vi.fn().mockImplementation(async () => { - const updatesBeforeExecute = vi + const invocation = { + params: { plan: 'Original plan' }, + requiresUserInteraction: vi.fn().mockReturnValue(true), + getDefaultPermission: vi.fn().mockResolvedValue('ask'), + getConfirmationDetails: vi.fn().mockResolvedValue({ + type: 'plan', + title: 'Approve plan', + plan: 'Original plan', + hideAlwaysAllow: true, + onConfirm: onConfirmSpy, + }), + getDescription: vi.fn().mockReturnValue('Plan:'), + toolLocations: vi.fn().mockReturnValue([]), + execute: executeSpy, + }; + const tool = { + name: core.ToolNames.EXIT_PLAN_MODE, + kind: core.Kind.Think, + build: vi.fn().mockReturnValue(invocation), + }; + + mockToolRegistry.getTool.mockReturnValue(tool); + mockConfig.getApprovalMode = vi.fn(() => mode); + mockConfig.getPermissionManager = vi.fn().mockReturnValue(null); + mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(false); + mockConfig.getMessageBus = vi.fn().mockReturnValue({}); + mockChat.sendMessageStream = vi.fn().mockResolvedValue( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'call-exit-plan', + name: core.ToolNames.EXIT_PLAN_MODE, + args: { plan: 'Original plan' }, + }, + ], + }, + }, + ]), + ); + const planUpdate: SessionUpdate = { + sessionUpdate: 'plan', + entries: [ + { + content: 'Ship', + priority: 'medium', + status: 'pending', + }, + ], + _meta: { + qwenTodoPlan: { id: 'plan-1' }, + qwenTranscript: { planToolCallId: 'todo-call-1' }, + }, + }; + if (revisionSource === 'replay') { + // A reloaded session replays the previous cycle's todo_write result; + // the replayed plan update must not bind the next approval. + await session.replayHistory([ + chatRecord({ + uuid: 'todo-exec-1', + type: 'tool_result', + message: { + parts: [ + { + functionResponse: { + name: core.ToolNames.TODO_WRITE, + id: 'stale-call', + response: {}, + }, + }, + ], + }, + toolCallResult: { + callId: 'stale-call', + resultDisplay: { + type: 'todo_list', + planId: 'stale-plan', + todos: [ + { id: '1', content: 'Done task', status: 'completed' }, + ], + }, + }, + }), + ]); + } else if (revisionSource === 'empty-entries') { + await session.sendUpdate({ ...planUpdate, entries: [] }); + } else if (revisionSource === 'missing-meta') { + await session.sendUpdate({ + sessionUpdate: 'plan', + entries: planUpdate.entries, + }); + } else { + await session.sendUpdate(planUpdate); + } + if (revisionSource === 'failed') { + vi.mocked(mockClient.sessionUpdate).mockRejectedValueOnce( + new Error('connection lost'), + ); + await expect( + session.sendUpdate({ + ...planUpdate, + _meta: { + qwenTodoPlan: { id: 'plan-2' }, + qwenTranscript: { planToolCallId: 'todo-call-2' }, + }, + }), + ).rejects.toThrow('connection lost'); + } + if (revisionSource === 'cleared') { + session.clearActiveTodoPlanRevision(); + } + + try { + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'approve the plan' }], + }); + } finally { + hookSpy.mockRestore(); + } + + expect(mockClient.requestPermission).toHaveBeenCalledWith( + expect.objectContaining({ + options: [ + expect.objectContaining({ + kind: 'allow_once', + optionId: core.ToolConfirmationOutcome.RestorePrevious, + }), + expect.objectContaining({ + kind: 'allow_always', + optionId: core.ToolConfirmationOutcome.ProceedAlways, + }), + expect.objectContaining({ + kind: 'allow_once', + optionId: core.ToolConfirmationOutcome.ProceedOnce, + }), + expect.objectContaining({ + kind: 'reject_once', + optionId: core.ToolConfirmationOutcome.Cancel, + }), + ], + toolCall: expect.objectContaining({ + kind: 'switch_mode', + rawInput: { plan: 'Original plan' }, + _meta: expectsRevision + ? expect.objectContaining({ + qwenTodoApproval: { + planId: 'plan-1', + sourceCallId: 'todo-call-1', + }, + }) + : expect.not.objectContaining({ + qwenTodoApproval: expect.anything(), + }), + }), + }), + ); + expect(onConfirmSpy).toHaveBeenCalledWith( + core.ToolConfirmationOutcome.ProceedOnce, + { answers: undefined }, + ); + expect(invocation.params).toEqual({ plan: 'Original plan' }); + const modeUpdates = vi .mocked(mockClient.sessionUpdate) .mock.calls.filter( ([params]) => params.update.sessionUpdate === 'current_mode_update', ); - expect(mode).toBe(ApprovalMode.PLAN); - expect(updatesBeforeExecute).toHaveLength(0); - mode = ApprovalMode.DEFAULT; - return { llmContent: 'approved', returnDisplay: 'approved' }; + expect(modeUpdates).toHaveLength(1); + expect(modeUpdates[0]?.[0].update).toMatchObject({ + currentModeId: ApprovalMode.DEFAULT, + }); + }, + ); + + it('clears the captured revision when enter_plan_mode execution enters plan mode', async () => { + let mode = ApprovalMode.DEFAULT; + const executeSpy = vi.fn().mockImplementation(async () => { + mode = ApprovalMode.PLAN; + return { llmContent: 'entered', returnDisplay: 'entered' }; }); const invocation = { - params: { plan: 'Original plan' }, - requiresUserInteraction: vi.fn().mockReturnValue(true), - getDefaultPermission: vi.fn().mockResolvedValue('ask'), - getConfirmationDetails: vi.fn().mockResolvedValue({ - type: 'plan', - title: 'Approve plan', - plan: 'Original plan', - hideAlwaysAllow: true, - onConfirm: onConfirmSpy, - }), - getDescription: vi.fn().mockReturnValue('Plan:'), + params: {}, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getConfirmationDetails: vi.fn(), + getDescription: vi.fn().mockReturnValue('Enter plan mode'), toolLocations: vi.fn().mockReturnValue([]), execute: executeSpy, }; const tool = { - name: core.ToolNames.EXIT_PLAN_MODE, + name: core.ToolNames.ENTER_PLAN_MODE, kind: core.Kind.Think, build: vi.fn().mockReturnValue(invocation), }; @@ -13660,7 +14014,7 @@ describe('Session', () => { mockToolRegistry.getTool.mockReturnValue(tool); mockConfig.getApprovalMode = vi.fn(() => mode); mockConfig.getPermissionManager = vi.fn().mockReturnValue(null); - mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(false); + mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(true); mockConfig.getMessageBus = vi.fn().mockReturnValue({}); mockChat.sendMessageStream = vi.fn().mockResolvedValue( createStreamWithChunks([ @@ -13669,65 +14023,38 @@ describe('Session', () => { value: { functionCalls: [ { - id: 'call-exit-plan', - name: core.ToolNames.EXIT_PLAN_MODE, - args: { plan: 'Original plan' }, + id: 'call-enter-plan', + name: core.ToolNames.ENTER_PLAN_MODE, + args: {}, }, ], }, }, ]), ); + await session.sendUpdate({ + sessionUpdate: 'plan', + entries: [ + { content: 'Old cycle', priority: 'medium', status: 'pending' }, + ], + _meta: { + qwenTodoPlan: { id: 'old-plan' }, + qwenTranscript: { planToolCallId: 'old-call' }, + }, + }); - try { - await session.prompt({ - sessionId: 'test-session-id', - prompt: [{ type: 'text', text: 'approve the plan' }], - }); - } finally { - hookSpy.mockRestore(); - } + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'plan this' }], + }); - expect(mockClient.requestPermission).toHaveBeenCalledWith( - expect.objectContaining({ - options: [ - expect.objectContaining({ - kind: 'allow_once', - optionId: core.ToolConfirmationOutcome.RestorePrevious, - }), - expect.objectContaining({ - kind: 'allow_always', - optionId: core.ToolConfirmationOutcome.ProceedAlways, - }), - expect.objectContaining({ - kind: 'allow_once', - optionId: core.ToolConfirmationOutcome.ProceedOnce, - }), - expect.objectContaining({ - kind: 'reject_once', - optionId: core.ToolConfirmationOutcome.Cancel, - }), - ], - toolCall: expect.objectContaining({ - kind: 'switch_mode', - rawInput: { plan: 'Original plan' }, - }), + expect(executeSpy).toHaveBeenCalled(); + const request = await runExitPlanModeApprovalPrompt(); + expect(request.toolCall._meta).toEqual( + expect.not.objectContaining({ + qwenTodoApproval: expect.anything(), }), ); - expect(onConfirmSpy).toHaveBeenCalledWith( - core.ToolConfirmationOutcome.ProceedOnce, - { answers: undefined }, - ); - expect(invocation.params).toEqual({ plan: 'Original plan' }); - const modeUpdates = vi - .mocked(mockClient.sessionUpdate) - .mock.calls.filter( - ([params]) => params.update.sessionUpdate === 'current_mode_update', - ); - expect(modeUpdates).toHaveLength(1); - expect(modeUpdates[0]?.[0].update).toMatchObject({ - currentModeId: ApprovalMode.DEFAULT, - }); }); it('routes ACP protected L4 allow writes through AUTO review', async () => { diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 39a91ec35cc..51711297687 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -1332,6 +1332,10 @@ export class Session implements SessionContext { private resolveCloseGate: (() => void) | null = null; private unsubscribeChatRecordingFailure?: () => void; private readonly workflowApprovalAbortController = new AbortController(); + private activeTodoPlanRevision?: { + planId: string; + sourceCallId: string; + }; // Modular components private readonly historyReplayer: HistoryReplayer; @@ -1644,6 +1648,10 @@ export class Session implements SessionContext { this.#clearTodoStopGuardTrustAndDrainAutomaticQueues(); } + clearActiveTodoPlanRevision(): void { + this.activeTodoPlanRevision = undefined; + } + hardSuspendTodoStopGuard(): void { this.#clearTodoStopGuardQueuedPromptWait(); this.todoStopGuardDrainAutomaticQueuesWhenIdle = false; @@ -2202,7 +2210,15 @@ export class Session implements SessionContext { gaps?: HistoryGap[], ): Promise { this.primeTurnFromHistory(records); - await this.historyReplayer.replay(records, gaps); + try { + await this.historyReplayer.replay(records, gaps); + } finally { + // Replayed plan updates re-stamp the revision via sendUpdate, but they + // belong to finished cycles; only live updates may bind the next + // exit_plan_mode approval, so a replayed session starts text-only — + // even when the replay fails part-way. + this.activeTodoPlanRevision = undefined; + } } rewindToTurn( @@ -2242,6 +2258,7 @@ export class Session implements SessionContext { chat.truncateHistory(apiTruncateIndex); chat.stripThoughtsFromHistory(); + this.activeTodoPlanRevision = undefined; const preserveQueuedPromptPriority = this.todoStopGuardQueuedPromptPriority; const shouldDrainAutomaticQueues = (this.todoStopGuard.blocksUnrelatedAutomaticTurns || @@ -2307,6 +2324,7 @@ export class Session implements SessionContext { .getGeminiClient()! .getChat() .setHistory(structuredClone(history)); + this.activeTodoPlanRevision = undefined; this.#clearTodoStopGuardTrustAndDrainAutomaticQueues(); } @@ -4447,7 +4465,36 @@ export class Session implements SessionContext { update, }; + if (update.sessionUpdate === 'plan') { + // Clear before delivery: a plan update the client never receives + // must not stay bound to the next exit_plan_mode approval. The + // capture below re-stamps only after delivery succeeds. + this.activeTodoPlanRevision = undefined; + } await this.client.sessionUpdate(params); + if (update.sessionUpdate === 'plan') { + this.#captureTodoPlanRevision(update); + } + } + + #captureTodoPlanRevision( + update: Extract, + ): void { + const meta = isRecord(update['_meta']) ? update['_meta'] : undefined; + const plan = isRecord(meta?.['qwenTodoPlan']) + ? meta['qwenTodoPlan'] + : undefined; + const transcript = isRecord(meta?.['qwenTranscript']) + ? meta['qwenTranscript'] + : undefined; + const planId = plan?.['id']; + const sourceCallId = transcript?.['planToolCallId']; + this.activeTodoPlanRevision = + typeof planId === 'string' && + typeof sourceCallId === 'string' && + update.entries.length > 0 + ? { planId, sourceCallId } + : undefined; } #scheduleChannelDelivery(params: Record): void { @@ -6468,8 +6515,14 @@ export class Session implements SessionContext { `Unknown approval mode: ${params.modeId}`, ); } + const previousApprovalMode = this.config.getApprovalMode(); this.config.setApprovalMode(approvalMode); if (approvalMode === ApprovalMode.PLAN) { + if (previousApprovalMode !== ApprovalMode.PLAN) { + // A redundant plan re-select keeps the revision captured by the + // live cycle; only a fresh entry starts a new approval cycle. + this.activeTodoPlanRevision = undefined; + } this.clearTodoStopGuardTrust(); } @@ -8213,6 +8266,11 @@ export class Session implements SessionContext { _meta: { toolName, ...interactionMetaFields(confirmationDetails), + ...(isExitPlanModeTool && this.activeTodoPlanRevision + ? { + qwenTodoApproval: this.activeTodoPlanRevision, + } + : {}), }, }, }; @@ -8733,6 +8791,7 @@ export class Session implements SessionContext { ) { await this.sendCurrentModeUpdateNotification(); if (this.config.getApprovalMode() === ApprovalMode.PLAN) { + this.activeTodoPlanRevision = undefined; this.#clearTodoStopGuardTrustAndDrainAutomaticQueues(); } } diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index 3321f803f0b..3c9e2710316 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -723,10 +723,18 @@ vi.mock('./components/dialogs/GitDiffDialog', async () => { vi.mock('./components/dialogs/DialogShell', async () => { const React = await import('react'); return { - DialogShell: (props: { children?: React.ReactNode }) => + DialogShell: (props: { + children?: React.ReactNode; + title?: React.ReactNode; + }) => React.createElement( 'div', - { 'data-testid': 'dialog-shell' }, + { + 'data-testid': 'dialog-shell', + ...(typeof props.title === 'string' + ? { 'data-dialog-title': props.title } + : {}), + }, props.children, ), }; @@ -2151,7 +2159,12 @@ async function triggerAutoRecap(): Promise<{ // array, so the ask-user variant carries a toolCall.input.questions payload // (getPermissionRawInput reads toolCall.input) — a bare toolName isn't enough. function makePendingPermissionBlock( - overrides: { resolved?: boolean; toolName?: string; kind?: string } = {}, + overrides: { + resolved?: boolean; + toolName?: string; + kind?: string; + todoPlan?: { planId: string; sourceCallId: string }; + } = {}, ): unknown { const toolName = overrides.toolName ?? 'run_shell_command'; const isAskUser = toolName === 'ask_user_question'; @@ -2164,7 +2177,10 @@ function makePendingPermissionBlock( toolCall: { toolCallId: 'tc-1', kind: overrides.kind ?? (isAskUser ? 'other' : 'execute'), - _meta: { toolName }, + _meta: { + toolName, + ...(overrides.todoPlan ? { qwenTodoApproval: overrides.todoPlan } : {}), + }, ...(isAskUser ? { input: { questions: [{ question: 'Pick one', options: [] }] } } : {}), @@ -2380,26 +2396,66 @@ afterEach(() => { describe('App plan todos', () => { it('gates the exit-plan workflow on the experimental setting', async () => { + const approvedEntries = [ + { + content: 'Prepare', + status: 'completed', + _meta: { qwenTodo: { id: 'prepare' } }, + }, + { + content: 'Ship', + status: 'pending', + _meta: { qwenTodo: { id: 'ship', blockedBy: ['prepare'] } }, + }, + ]; testState.messages = [ { - id: 'plan', - role: 'plan', - todos: [ - { id: 'prepare', content: 'Prepare', status: 'completed' }, + id: 'approved-plan', + role: 'tool_group', + tools: [ { - id: 'ship', - content: 'Ship', - status: 'pending', - blockedBy: ['prepare'], + callId: 'todo-approved', + toolName: 'todo_write', + status: 'completed', + rawOutput: { + entries: approvedEntries, + plan: { id: 'plan-1' }, + }, }, ], }, { id: 'revision', role: 'user', content: 'Revise the wording' }, + { + id: 'newer-plan', + role: 'tool_group', + tools: [ + { + callId: 'todo-newer', + toolName: 'todo_write', + status: 'completed', + rawOutput: { + entries: [ + ...approvedEntries.map((entry) => ({ + ...entry, + status: 'completed', + })), + { + content: 'Deploy', + status: 'pending', + _meta: { qwenTodo: { id: 'deploy' } }, + }, + ], + plan: { id: 'plan-1' }, + }, + }, + ], + }, ]; testState.blocks = [ makePendingPermissionBlock({ toolName: 'exit_plan_mode', kind: 'switch_mode', + todoPlan: { planId: 'plan-1', sourceCallId: 'todo-approved' }, }), ]; @@ -2495,6 +2551,43 @@ describe('App plan todos', () => { testState.latestTasksStatusProps?.agentTools?.map((tool) => tool.callId), ).toEqual(['agent-call']); }); + + it('keeps the tasks dialog plain when Session Workflow is off', async () => { + testState.messages = [ + { + id: 'plan', + role: 'plan', + todos: [{ id: 'work', content: 'Work', status: 'in_progress' }], + }, + { + id: 'agents', + role: 'tool_group', + tools: [ + { + callId: 'agent-call', + toolName: 'Agent', + status: 'in_progress', + args: { todo_id: 'work' }, + }, + ], + }, + ]; + const { container } = renderApp(); + await flush(); + + await act(async () => { + testState.latestTodoPanelOnOpen?.(); + await Promise.resolve(); + }); + + expect(testState.latestTasksStatusProps?.planTodos).toEqual([]); + expect(testState.latestTasksStatusProps?.agentTools).toEqual([]); + expect( + container + .querySelector('[data-testid="dialog-shell"]') + ?.getAttribute('data-dialog-title'), + ).toBe('Background tasks'); + }); }); describe('App composer footer renderer', () => { diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index 0ec17b82cc1..61aaa26d216 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -258,7 +258,8 @@ import { computeTodoTimeline, getAgentToolsForPlan, getFloatingTodos, - getLatestActiveTodos, + getActiveTodosForPlanRevision, + isExitPlanApprovalRequest, todoDetailSignature, todoTimelineSignature, type TodoDetail, @@ -3234,8 +3235,11 @@ export function App({ [messages], ); const approvalPlanTodos = useMemo( - () => getLatestActiveTodos(messages), - [messages], + () => + isExitPlanApprovalRequest(pendingToolApproval) + ? getActiveTodosForPlanRevision(messages, pendingToolApproval?.todoPlan) + : [], + [messages, pendingToolApproval], ); // Keep the timeline Map referentially stable across streaming ticks that // don't touch any todo snapshot. The Map is a context value, so a fresh diff --git a/packages/web-shell/client/adapters/transcriptAdapter.test.ts b/packages/web-shell/client/adapters/transcriptAdapter.test.ts index 0ac9f6cfb97..60d93716472 100644 --- a/packages/web-shell/client/adapters/transcriptAdapter.test.ts +++ b/packages/web-shell/client/adapters/transcriptAdapter.test.ts @@ -157,7 +157,13 @@ describe('extractPendingPermission', () => { toolCall: { toolCallId: 'call-plan', kind: 'switch_mode', - _meta: { toolName: 'exit_plan_mode' }, + _meta: { + toolName: 'exit_plan_mode', + qwenTodoApproval: { + planId: 'plan-1', + sourceCallId: 'todo-call-1', + }, + }, content: [ { type: 'content', @@ -174,6 +180,7 @@ describe('extractPendingPermission', () => { expect(extractPendingPermission(state([permission]).blocks)).toMatchObject({ toolKind: 'switch_mode', toolName: 'exit_plan_mode', + todoPlan: { planId: 'plan-1', sourceCallId: 'todo-call-1' }, content: [{ type: 'text', text: '1. Prepare\n2. Ship' }], }); }); diff --git a/packages/web-shell/client/adapters/transcriptAdapter.ts b/packages/web-shell/client/adapters/transcriptAdapter.ts index 725b518e20a..1fcb4f5f0c3 100644 --- a/packages/web-shell/client/adapters/transcriptAdapter.ts +++ b/packages/web-shell/client/adapters/transcriptAdapter.ts @@ -33,6 +33,9 @@ export function extractPendingPermission( typeof metaRecord?.['toolName'] === 'string' ? metaRecord['toolName'] : undefined; + const todoApproval = getRecord(metaRecord?.['qwenTodoApproval']); + const planId = getString(todoApproval, 'planId'); + const sourceCallId = getString(todoApproval, 'sourceCallId'); return { id: perm.requestId, sessionId: perm.sessionId, @@ -40,6 +43,7 @@ export function extractPendingPermission( title: perm.title, toolKind, toolName, + ...(planId && sourceCallId ? { todoPlan: { planId, sourceCallId } } : {}), content: getPermissionContent(toolCallRecord, perm.title), options: perm.options.map((opt) => ({ id: opt.optionId, @@ -102,6 +106,14 @@ function getRecord(value: unknown): Record | undefined { return value as Record; } +function getString( + record: Record | undefined, + key: string, +): string | undefined { + const value = record?.[key]; + return typeof value === 'string' && value.length > 0 ? value : undefined; +} + function getPermissionOptionKind( raw: unknown, ): PermissionOptionKind | undefined { diff --git a/packages/web-shell/client/adapters/types.ts b/packages/web-shell/client/adapters/types.ts index 2157a7d097f..9393686d6fb 100644 --- a/packages/web-shell/client/adapters/types.ts +++ b/packages/web-shell/client/adapters/types.ts @@ -108,6 +108,10 @@ export interface PermissionRequest { toolKind?: string; /** Canonical tool name (from the ACP frame's `_meta.toolName`). */ toolName?: string; + todoPlan?: { + planId: string; + sourceCallId: string; + }; content: ContentBlock[]; options: PermissionOption[]; rawInput?: Record; diff --git a/packages/web-shell/client/components/ChatEditor.test.tsx b/packages/web-shell/client/components/ChatEditor.test.tsx index 638ba4282d5..1c63df17ff6 100644 --- a/packages/web-shell/client/components/ChatEditor.test.tsx +++ b/packages/web-shell/client/components/ChatEditor.test.tsx @@ -257,6 +257,7 @@ function renderChatEditor(props: { currentMode?: string; currentModel?: string; availableModels?: Array<{ id: string; label?: string }>; + sessionWorkflowEnabled?: boolean; onSelectMode?: (mode: string) => void; onSelectModel?: (model: string) => void; onAttachmentsChange?: (hasAttachments: boolean) => void; @@ -789,6 +790,53 @@ describe('ChatEditor top composer tag tooltip', () => { }); }); +describe('ChatEditor Session Workflow mode rename', () => { + it('renames only the plan entry in the mode dropdown while enabled', () => { + const container = renderChatEditor({ + visibleToolbarActions: ['approvalMode'], + sessionWorkflowEnabled: true, + }); + + act(() => { + container + .querySelector('[data-web-shell-mode-button]') + ?.click(); + }); + + const popover = document.querySelector('[data-web-shell-toolbar-popover]'); + expect(popover).not.toBeNull(); + const labels = Array.from(popover?.querySelectorAll('button') ?? []).map( + (button) => button.textContent ?? '', + ); + expect(labels.some((label) => label.includes('Plan & Review (plan)'))).toBe( + true, + ); + expect( + labels.some((label) => label.includes('Ask Approval (default)')), + ).toBe(true); + expect(labels.some((label) => label.includes('Plan (plan)'))).toBe(false); + }); + + it('renames the active plan mode chip while enabled', () => { + const withWorkflow = renderChatEditor({ + currentMode: 'plan', + sessionWorkflowEnabled: true, + }); + expect( + withWorkflow + .querySelector('[data-toolbar-measure="mode:expanded"]') + ?.textContent?.includes('Plan & Review'), + ).toBe(true); + + const withoutWorkflow = renderChatEditor({ currentMode: 'plan' }); + expect( + withoutWorkflow + .querySelector('[data-toolbar-measure="mode:expanded"]') + ?.textContent?.includes('Plan & Review'), + ).toBe(false); + }); +}); + describe('ChatEditor toolbar popovers', () => { it('opens the approval mode popover and restores editor focus after selection', async () => { const onSelectMode = vi.fn(); diff --git a/packages/web-shell/client/components/ChatPane.test.tsx b/packages/web-shell/client/components/ChatPane.test.tsx index 818577da41e..f8424ffb3fe 100644 --- a/packages/web-shell/client/components/ChatPane.test.tsx +++ b/packages/web-shell/client/components/ChatPane.test.tsx @@ -979,15 +979,51 @@ describe('ChatPane', () => { it('passes this pane workflow to its exit-plan approval', () => { messagesState = [ { - id: 'plan', - role: 'plan', - todos: [ - { id: 'prepare', content: 'Prepare', status: 'completed' }, + id: 'plan-update', + role: 'tool_group', + tools: [ { - id: 'ship', - content: 'Ship', - status: 'pending', - blockedBy: ['prepare'], + callId: 'todo-call-1', + toolName: 'todo_write', + status: 'completed', + rawOutput: { + entries: [ + { + content: 'Prepare', + status: 'completed', + _meta: { qwenTodo: { id: 'prepare' } }, + }, + { + content: 'Ship', + status: 'pending', + _meta: { + qwenTodo: { id: 'ship', blockedBy: ['prepare'] }, + }, + }, + ], + plan: { id: 'plan-1' }, + }, + }, + ], + }, + { + id: 'plan-update-newer', + role: 'tool_group', + tools: [ + { + callId: 'todo-call-2', + toolName: 'todo_write', + status: 'completed', + rawOutput: { + entries: [ + { + content: 'Ship v2', + status: 'pending', + _meta: { qwenTodo: { id: 'ship-v2' } }, + }, + ], + plan: { id: 'plan-1' }, + }, }, ], }, @@ -1001,6 +1037,7 @@ describe('ChatPane', () => { id: 'perm-plan', toolKind: 'switch_mode', toolName: 'exit_plan_mode', + todoPlan: { planId: 'plan-1', sourceCallId: 'todo-call-1' }, rawInput: {}, }; @@ -1011,6 +1048,37 @@ describe('ChatPane', () => { ); }); + it('keeps the exit-plan approval text-only when Session Workflow is off', () => { + messagesState = [ + { + id: 'plan-update', + role: 'tool_group', + tools: [ + { + callId: 'todo-call-1', + toolName: 'todo_write', + status: 'completed', + rawOutput: { + entries: [{ content: 'Ship', status: 'pending' }], + plan: { id: 'plan-1' }, + }, + }, + ], + }, + ]; + pendingPermission = { + id: 'perm-plan', + toolKind: 'switch_mode', + toolName: 'exit_plan_mode', + todoPlan: { planId: 'plan-1', sourceCallId: 'todo-call-1' }, + rawInput: {}, + }; + + render(); + + expect(testid('tool-approval')?.getAttribute('data-plan-todos')).toBe('[]'); + }); + it('reflects streaming state on the composer', () => { streamingStateValue = 'responding'; render(); diff --git a/packages/web-shell/client/components/ChatPane.tsx b/packages/web-shell/client/components/ChatPane.tsx index fbd425fdef5..e308e15d2d1 100644 --- a/packages/web-shell/client/components/ChatPane.tsx +++ b/packages/web-shell/client/components/ChatPane.tsx @@ -47,7 +47,10 @@ import { isAskUserPermission } from '../utils/askUserPermission'; import { isDaemonApprovalMode } from '../utils/sessionPreparation'; import { isVisibleComposerModel } from '../utils/composerModels'; import { shouldBlockComposerSubmit } from '../utils/composerInputState'; -import { getLatestActiveTodos } from '../utils/todos'; +import { + getActiveTodosForPlanRevision, + isExitPlanApprovalRequest, +} from '../utils/todos'; import { findMonitorTaskForTool } from '../utils/monitorTasks'; import { invokeSlashCommandHandler } from '../utils/slash-command-action'; import type { WebShellSlashCommandHandler } from '../App'; @@ -346,15 +349,13 @@ export function ChatPane({ pendingApproval && !isAskUser ? pendingApproval : null; const pendingAskUserApproval = pendingApproval && isAskUser ? pendingApproval : null; - const isExitPlanApproval = - pendingToolApproval?.toolKind === 'switch_mode' && - pendingToolApproval?.toolName?.toLowerCase() === 'exit_plan_mode'; + const isExitPlanApproval = isExitPlanApprovalRequest(pendingToolApproval); const planTodos = useMemo( () => sessionWorkflowEnabled && isExitPlanApproval - ? getLatestActiveTodos(messages) + ? getActiveTodosForPlanRevision(messages, pendingToolApproval?.todoPlan) : [], - [isExitPlanApproval, messages, sessionWorkflowEnabled], + [isExitPlanApproval, messages, pendingToolApproval, sessionWorkflowEnabled], ); // Tracked in a ref so an async approval-mode switch (handleSelectMode) reads // the approval current when setApprovalMode *resolves*, not a stale one diff --git a/packages/web-shell/client/components/artifacts/SideTaskPanel.test.tsx b/packages/web-shell/client/components/artifacts/SideTaskPanel.test.tsx index f20e028f860..d895b8955bd 100644 --- a/packages/web-shell/client/components/artifacts/SideTaskPanel.test.tsx +++ b/packages/web-shell/client/components/artifacts/SideTaskPanel.test.tsx @@ -264,6 +264,22 @@ it('renders a restored side task as a full chat pane', () => { }); }); +it('threads sessionWorkflowEnabled to its chat pane', () => { + connection.sessionId = 'side-session-1'; + connection.displayName = 'Investigate flaky tests'; + connection.status = 'connected'; + transcript.blocks = [{ kind: 'user' }]; + container = document.createElement('div'); + document.body.appendChild(container); + root = createRoot(container); + + act(() => { + renderSideTask({ sessionWorkflowEnabled: true }); + }); + + expect(latestChatPaneProps.current?.sessionWorkflowEnabled).toBe(true); +}); + it('names a restored empty side task from its first prompt', async () => { connection.sessionId = 'side-session-1'; connection.displayName = 'Side task'; diff --git a/packages/web-shell/client/components/dialogs/ApprovalModeDialog.test.tsx b/packages/web-shell/client/components/dialogs/ApprovalModeDialog.test.tsx index 77f4e5179a3..863cc3b8f2f 100644 --- a/packages/web-shell/client/components/dialogs/ApprovalModeDialog.test.tsx +++ b/packages/web-shell/client/components/dialogs/ApprovalModeDialog.test.tsx @@ -71,6 +71,12 @@ describe('ApprovalModeDialog', () => { expect( container!.querySelector('[data-mode-id="plan"]')?.textContent, ).toContain('Plan & Review (plan)'); + expect( + container!.querySelector('[data-mode-id="default"]')?.textContent, + ).toContain('Ask Approval (default)'); + expect( + container!.querySelector('[data-mode-id="yolo"]')?.textContent, + ).toContain('Full Access (yolo)'); }); it('opens with the highlight on the current mode and confirms on Enter', () => { diff --git a/packages/web-shell/client/components/messages/ToolApproval.tsx b/packages/web-shell/client/components/messages/ToolApproval.tsx index aa736d0c5ce..ac97d2ba23e 100644 --- a/packages/web-shell/client/components/messages/ToolApproval.tsx +++ b/packages/web-shell/client/components/messages/ToolApproval.tsx @@ -11,6 +11,7 @@ import { isAgentTool } from '@qwen-code/webui/daemon-react-sdk'; import type { PermissionRequest, TodoItem } from '../../adapters/types'; import { useI18n } from '../../i18n'; import { PlanExecutionView } from './PlanExecutionView'; +import { isExitPlanApprovalRequest } from '../../utils/todos'; import { localizeToolDisplayName } from './toolFormatting'; import styles from './ToolApproval.module.css'; @@ -372,9 +373,7 @@ export function ToolApproval({ const showsCommandBlock = Boolean( (isExec && command) || (contentText && contentText !== request.title), ); - const isExitPlanApproval = - request.toolKind === 'switch_mode' && - request.toolName?.toLowerCase() === 'exit_plan_mode'; + const isExitPlanApproval = isExitPlanApprovalRequest(request); const showsPlanWorkflow = planTodos.length > 0 && isExitPlanApproval; const questionText = isAgent ? t('approval.launchAgentQuestion') diff --git a/packages/web-shell/client/utils/todos.test.ts b/packages/web-shell/client/utils/todos.test.ts index 44c8b194325..c75f1900312 100644 --- a/packages/web-shell/client/utils/todos.test.ts +++ b/packages/web-shell/client/utils/todos.test.ts @@ -8,9 +8,10 @@ import { extractTodosFromToolCall, getAgentToolsForPlan, getFloatingTodos, - getLatestActiveTodos, + getActiveTodosForPlanRevision, getTodoStatusIcon, getTodoWindow, + isExitPlanApprovalRequest, isTodoWriteToolName, todoDetailSignature, todoStateKey, @@ -38,6 +39,7 @@ function todoWriteMessage( id: string, todos: TodoItem[], stats?: TodoStatsSnapshot, + planId?: string, ): Message { const tool: ACPToolCall = { callId: `call-${id}`, @@ -45,7 +47,14 @@ function todoWriteMessage( status: 'completed', kind: 'think', args: { todos }, - ...(stats ? { rawOutput: { stats } } : {}), + ...(stats || planId + ? { + rawOutput: { + ...(stats ? { stats } : {}), + ...(planId ? { plan: { id: planId } } : {}), + }, + } + : {}), }; return { id, role: 'tool_group', tools: [tool] }; } @@ -230,29 +239,43 @@ describe('getFloatingTodos', () => { }); }); -describe('getLatestActiveTodos', () => { - it('keeps the persisted workflow available after a later user message', () => { +describe('getActiveTodosForPlanRevision', () => { + it('returns only the snapshot named by the approval revision', () => { const todos = [todo('1', 'in_progress')]; + const revisedTodos = [todo('2', 'pending')]; expect( - getLatestActiveTodos([ - todoWriteMessage('m1', todos), - userMessage('revision'), - ]), - ).toEqual(todos); + getActiveTodosForPlanRevision( + [ + todoWriteMessage('m1', todos, undefined, 'plan-1'), + userMessage('revision'), + todoWriteMessage('m2', revisedTodos, undefined, 'plan-1'), + ], + { planId: 'plan-1', sourceCallId: 'call-m2' }, + ), + ).toEqual(revisedTodos); }); - it('honors an explicit clear and ignores a terminal-only snapshot', () => { + it('rejects missing and mismatched revisions but preserves terminal ones', () => { + expect(getActiveTodosForPlanRevision([], undefined)).toEqual([]); expect( - getLatestActiveTodos([ - todoWriteMessage('m1', [todo('1', 'in_progress')]), - todoWriteMessage('clear', []), - ]), + getActiveTodosForPlanRevision( + [todoWriteMessage('m1', [todo('1', 'pending')], undefined, 'plan-1')], + undefined, + ), ).toEqual([]); expect( - getLatestActiveTodos([ - todoWriteMessage('done', [todo('1', 'completed')]), - ]), + getActiveTodosForPlanRevision( + [todoWriteMessage('m1', [todo('1', 'pending')], undefined, 'plan-1')], + { planId: 'plan-other', sourceCallId: 'call-m1' }, + ), ).toEqual([]); + const completed = [todo('1', 'completed')]; + expect( + getActiveTodosForPlanRevision( + [todoWriteMessage('done', completed, undefined, 'plan-1')], + { planId: 'plan-1', sourceCallId: 'call-done' }, + ), + ).toEqual(completed); }); }); @@ -618,6 +641,38 @@ describe('isTodoWriteToolName', () => { }); }); +describe('isExitPlanApprovalRequest', () => { + it('matches a switch_mode exit_plan_mode request', () => { + expect( + isExitPlanApprovalRequest({ + toolKind: 'switch_mode', + toolName: 'Exit_Plan_Mode', + }), + ).toBe(true); + }); + + it('rejects a mismatched kind or name', () => { + expect( + isExitPlanApprovalRequest({ + toolKind: 'execute', + toolName: 'exit_plan_mode', + }), + ).toBe(false); + expect( + isExitPlanApprovalRequest({ + toolKind: 'switch_mode', + toolName: 'read_file', + }), + ).toBe(false); + }); + + it('rejects a missing or null request', () => { + expect(isExitPlanApprovalRequest(undefined)).toBe(false); + expect(isExitPlanApprovalRequest(null)).toBe(false); + expect(isExitPlanApprovalRequest({})).toBe(false); + }); +}); + describe('extractTodosFromToolCall', () => { function toolCall(overrides: Partial): ACPToolCall { return { diff --git a/packages/web-shell/client/utils/todos.ts b/packages/web-shell/client/utils/todos.ts index a52d2705367..ae1d67f45d9 100644 --- a/packages/web-shell/client/utils/todos.ts +++ b/packages/web-shell/client/utils/todos.ts @@ -1,4 +1,9 @@ -import type { ACPToolCall, Message, TodoItem } from '../adapters/types'; +import type { + ACPToolCall, + Message, + PermissionRequest, + TodoItem, +} from '../adapters/types'; import { isSubAgentToolCall } from '../adapters/toolClassification'; /** @@ -11,6 +16,20 @@ export function isTodoWriteToolName(name: string): boolean { return normalized === 'todo_write' || normalized === 'todowrite'; } +/** + * The full exit-plan approval rule: the switch_mode frame kind plus the + * exit_plan_mode wire name. Shared by App, ChatPane, and ToolApproval so the + * surfaces that gate the revision-bound approval UI never drift. + */ +export function isExitPlanApprovalRequest( + request: Pick | null | undefined, +): boolean { + return ( + request?.toolKind === 'switch_mode' && + request?.toolName?.toLowerCase() === 'exit_plan_mode' + ); +} + export function parseTodoItemsFromEntries( entries: readonly unknown[], ): TodoItem[] { @@ -140,20 +159,25 @@ export function getFloatingTodos( return { todos, planId, allCompleted, sourceMessageId }; } -export function getLatestActiveTodos(messages: readonly Message[]): TodoItem[] { - let todos: TodoItem[] = []; +export function getActiveTodosForPlanRevision( + messages: readonly Message[], + revision: { planId: string; sourceCallId: string } | null | undefined, +): TodoItem[] { + if (!revision) return []; for (const message of messages) { - if (message.role === 'plan') { - todos = message.todos; - continue; - } if (message.role !== 'tool_group') continue; for (const tool of message.tools) { - const nextTodos = extractTodosFromToolCall(tool); - if (nextTodos !== undefined) todos = nextTodos; + if ( + tool.callId !== revision.sourceCallId || + getTodoPlanId(tool) !== revision.planId + ) { + continue; + } + const todos = extractTodosFromToolCall(tool) ?? []; + return todos; } } - return hasActiveTodos(todos) ? todos : []; + return []; } export function getAgentToolsForPlan(