diff --git a/docs/design/active-todo-context.md b/docs/design/active-todo-context.md index d50b809d7a7..3baf8ae4a75 100644 --- a/docs/design/active-todo-context.md +++ b/docs/design/active-todo-context.md @@ -10,15 +10,26 @@ live control state because it can outlive the work chain that created it. ## Design After a successful `todo_write`, keep a reminder containing only unfinished -items under a stable work-chain owner. Prompt IDs used by retries and related -automatic turns resolve to that owner, so concurrent notification branches do -not move or overwrite the foreground reminder. Background tasks and loop -wakeups capture the owner when they are created and carry it back with their -automatic turn; unrelated cron and notification turns use an isolated owner -that is removed when the turn ends. Inject the reminder on the first request of -a retry or related automatic turn and after function responses on later tool -turns. Clear it when all todos complete, a new ordinary work chain starts, or -the session changes. +items under a stable work-chain owner. Prompt IDs used by retries, related +automatic turns, and ordinary user turns that arrive while a reminder is still +registered resolve to that owner, so concurrent notification branches do not +move or overwrite the foreground reminder. Background tasks and loop wakeups +capture the owner when they are created and carry it back with their automatic +turn; unrelated cron and notification turns use an isolated owner that is +removed when the turn ends. Inject the reminder on the first request of a retry +or related automatic turn and after function responses on later tool turns. +Clear it when all todos complete, when an ordinary turn starts with no reminder +registered, or when the session changes. + +A registered reminder is the signal that the plan still has unfinished items, +because `todo_write` deletes it once the list completes. An ordinary user turn +therefore continues the chain instead of discarding the context of work that is +still running: the turn that asks how the work is going is the turn that needs +the plan. The accepted cost is that an abandoned plan keeps resurfacing until a +later `todo_write` completes or clears it, while a genuinely new task replaces +the plan on its first write. Both frontends apply this, and in ACP the +todo-stop-guard lineage reset stays keyed to the retry/continue flag alone, so +carrying a plan never widens the guard's trust (#10953). Every injected copy is recorded permanently in chat history, so per-turn injection would grow the live context linearly with tool turns. Tool-turn @@ -28,6 +39,18 @@ turn-start injections always fire and reset that cadence. The payload is a compact `- [status] content` line list capped at 800 characters. History stays append-only, so provider prefix caching is unaffected. +A tool-turn count is a poor proxy for elapsed work when the turn is a delegated +run: a parent blocked on one foreground subagent earns a single tool turn for +the whole execution, so the cadence on its own leaves the plan stale for as +long as the subagent ran — 55 minutes in the report that motivated this rule +(#10953). A tool-result batch that carries a top-level Agent result therefore +forces the reminder due at that boundary, on both frontends, instead of waiting +out the cadence. Forcing stays bounded by registration: with no reminder +registered there is nothing to inject, so a delegation outside an active plan +costs nothing. This re-times delivery of the existing reminder only. It does +not derive plan state from the execution, which remains the model's job through +`todo_write`, and the Agent tool's optional `todo_id` stays observational. + This does not change stop semantics or enable `todoStopGuard`. The guard remains an optional bounded recovery after a model has already tried to stop; this change instead preserves task context before that decision. @@ -38,6 +61,10 @@ change instead preserves task context before that decision. - A completed list clears it. - Core and ACP tool-result messages append the reminder after function results. - ACP mid-turn user input remains last and therefore keeps precedence. -- An ordinary new prompt clears stale state while retry/continue retains it. +- An ordinary new prompt retains the reminder while items are unfinished and + clears stale state when none is registered; retry/continue always retains it. + Both frontends behave the same. +- A tool-result batch carrying a top-level Agent result forces the reminder due + even though the turn budget is not filled; a batch without one stays budgeted. - Independent automatic turns are isolated; related automatic turns inherit. - Terminal automatic turns release their temporary ownership state. diff --git a/packages/cli/src/acp-integration/session/Session.review-lease.test.ts b/packages/cli/src/acp-integration/session/Session.review-lease.test.ts index 59ca8b19663..808e95daa49 100644 --- a/packages/cli/src/acp-integration/session/Session.review-lease.test.ts +++ b/packages/cli/src/acp-integration/session/Session.review-lease.test.ts @@ -108,6 +108,7 @@ describe('Session review-worktree lease sweep', () => { getModel: vi.fn().mockReturnValue('qwen3'), getSessionId: vi.fn().mockReturnValue(SESSION_ID), takeActiveTodoReminder: vi.fn().mockReturnValue(undefined), + getActiveTodoReminder: vi.fn().mockReturnValue(undefined), getActiveTodoWorkChainOwner: vi.fn((promptId: string) => promptId), setActiveTodoReminder: vi.fn(), startActiveTodoWorkChain: vi.fn(), diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 851c239d9b7..7d459b4e70f 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -905,6 +905,7 @@ describe('Session', () => { isProvisionalWorkspace: vi.fn().mockReturnValue(false), setLiveAppendSystemPrompt: vi.fn(), takeActiveTodoReminder: vi.fn().mockReturnValue(undefined), + getActiveTodoReminder: vi.fn().mockReturnValue(undefined), // The restore-ask_user_question prompt path is gated on this flag; // the restore describe block overrides to true. getRestoreAskUserQuestion: vi.fn().mockReturnValue(false), @@ -2833,6 +2834,99 @@ describe('Session', () => { ); }); + it('continues the todo work chain on an ordinary prompt while a reminder is registered', async () => { + mockChat.sendMessageStream = vi + .fn() + .mockImplementation(async () => createEmptyStream()); + // A registered reminder means the plan still has unfinished items + // (todo_write deletes it on completion). + vi.mocked(mockConfig.getActiveTodoReminder).mockReturnValue( + 'unfinished todo: delegated node', + ); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'start work' }], + }); + + // The first prompt has no previous chain to continue. + expect(mockConfig.startActiveTodoWorkChain).toHaveBeenCalledWith( + 'test-session-id########1', + undefined, + ); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'how is progress going?' }], + }); + + // The follow-up turn must continue the previous chain instead of + // discarding the plan context it asks about (#10953). + expect(mockConfig.startActiveTodoWorkChain).toHaveBeenLastCalledWith( + 'test-session-id########2', + 'test-session-id########1', + ); + }); + + it('forces the active todo reminder due when an Agent tool result returns', async () => { + const reminder = + 'unfinished todo: follow up on the delegated node'; + // Mimic the real budget: nothing is due under the ordinary cadence + // (the delegation consumed the only tool turn), only forcing delivers. + vi.mocked(mockConfig.takeActiveTodoReminder).mockImplementation( + (_promptId: string, force = false) => (force ? reminder : undefined), + ); + const execute = vi.fn().mockResolvedValue({ + llmContent: 'agent done', + returnDisplay: 'agent done', + }); + mockToolRegistry.getTool.mockReturnValue({ + name: 'agent', + kind: core.Kind.Execute, + displayName: 'Agent', + description: 'Delegates work to a subagent', + build: vi.fn().mockReturnValue({ + params: {}, + execute, + getDefaultPermission: vi.fn().mockResolvedValue('allow'), + getDescription: vi.fn().mockReturnValue('Agent'), + toolLocations: vi.fn().mockReturnValue([]), + }), + canUpdateOutput: false, + isOutputMarkdown: true, + }); + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(true); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [{ id: 'call-agent-1', name: 'agent', args: {} }], + }, + }, + ]), + ) + .mockResolvedValueOnce(createEmptyStream()); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'delegate the work' }], + }); + + expect(execute).toHaveBeenCalledTimes(1); + expect(mockConfig.takeActiveTodoReminder).toHaveBeenCalledWith( + 'test-session-id########1', + true, + ); + const toolResultCall = vi + .mocked(mockChat.sendMessageStream) + .mock.calls.at(-1)?.[1] as { message: Part[] }; + expect(textParts(toolResultCall.message)).toContain(reminder); + }); + it('includes active Todo context on the first retry request', async () => { const reminder = 'unfinished todo: run tests'; diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 969b87c8fe5..1af5caf36f7 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -5575,9 +5575,19 @@ export class Session implements SessionContext { if (!continuesCurrentWorkChain && !this.todoStopGuard.enabled) { this.#resetTodoStopGuardBackgroundLineage(); } + // A registered reminder means the previous chain's plan still + // has unfinished items (todo_write deletes it on completion): + // continue that chain instead of discarding its context with the + // very turn that may be asking about it (#10953). + const continuesTodoWorkChain = + continuesCurrentWorkChain || + (this.activeTodoWorkChainPromptId !== undefined && + this.config.getActiveTodoReminder( + this.activeTodoWorkChainPromptId, + ) !== undefined); this.config.startActiveTodoWorkChain( promptId, - continuesCurrentWorkChain + continuesTodoWorkChain ? this.activeTodoWorkChainPromptId : undefined, ); @@ -7908,7 +7918,18 @@ export class Session implements SessionContext { if (hadMidTurnUserInput) { this.todoStopGuard.acceptMidTurnUserInput(); } - const activeTodoReminder = this.config.takeActiveTodoReminder(promptId); + // A top-level Agent tool result means a delegated execution just + // returned (#10953): real work advanced while the parent earned a + // single tool turn, so the turn budget cannot come due on its own. + // Force the reminder exactly where the progress information arrives. + const carriesAgentToolResult = toolRun.parts.some( + (part) => + canonicalToolName(part.functionResponse?.name ?? '') === + ToolNames.AGENT, + ); + const activeTodoReminder = carriesAgentToolResult + ? this.config.takeActiveTodoReminder(promptId, true) + : this.config.takeActiveTodoReminder(promptId); if (abortSignal.aborted) { return { message: { diff --git a/packages/cli/src/acp-integration/session/Session.worktree.test.ts b/packages/cli/src/acp-integration/session/Session.worktree.test.ts index 679adba1743..332e444bc90 100644 --- a/packages/cli/src/acp-integration/session/Session.worktree.test.ts +++ b/packages/cli/src/acp-integration/session/Session.worktree.test.ts @@ -118,6 +118,7 @@ describe('Session.pendingWorktreeNotice', () => { getModel: vi.fn().mockReturnValue('qwen3'), getSessionId: vi.fn().mockReturnValue(SESSION_ID), takeActiveTodoReminder: vi.fn().mockReturnValue(undefined), + getActiveTodoReminder: vi.fn().mockReturnValue(undefined), getActiveTodoWorkChainOwner: vi.fn((promptId: string) => promptId), setActiveTodoReminder: vi.fn(), startActiveTodoWorkChain: vi.fn(), diff --git a/packages/core/src/core/client-goal.test.ts b/packages/core/src/core/client-goal.test.ts index b552872b1b5..179633504ff 100644 --- a/packages/core/src/core/client-goal.test.ts +++ b/packages/core/src/core/client-goal.test.ts @@ -244,6 +244,7 @@ function setupGoalClient() { startAutomaticActiveTodoWorkChain: vi.fn(), endAutomaticActiveTodoWorkChain: vi.fn(), takeActiveTodoReminder: vi.fn(() => undefined), + getActiveTodoReminder: vi.fn(() => undefined), getContentGeneratorConfig: vi.fn(() => undefined), hasHooksForEvent: vi.fn(() => false), getStopHookBlockingCap: vi.fn(() => 8), diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index d4f42044e72..273ab363bc5 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -595,6 +595,7 @@ describe('Gemini Client (client.ts)', () => { getFullContext: vi.fn().mockReturnValue(false), getSessionId: vi.fn().mockReturnValue('test-session-id'), takeActiveTodoReminder: vi.fn().mockReturnValue(undefined), + getActiveTodoReminder: vi.fn().mockReturnValue(undefined), getActiveTodoWorkChainOwner: vi.fn((promptId: string) => promptId), startActiveTodoWorkChain: vi.fn(), startAutomaticActiveTodoWorkChain: vi.fn(), @@ -2195,8 +2196,11 @@ describe('Gemini Client (client.ts)', () => { await runTurn(SendMessageType.UserQuery); + // No reminder is registered here (getActiveTodoReminder returns + // undefined), so the ordinary user turn still starts a fresh chain. expect(mockConfig.startActiveTodoWorkChain).toHaveBeenCalledWith( 'prompt-userQuery', + undefined, ); await runTurn(SendMessageType.Cron); @@ -2217,6 +2221,101 @@ describe('Gemini Client (client.ts)', () => { ); }); + it.each(['agent', 'task'])( + 'forces the active todo reminder due when a %s tool result returns', + async (agentToolName) => { + const reminder = + 'unfinished todo: follow up on the delegated node'; + vi.mocked(mockConfig.takeActiveTodoReminder).mockReturnValue(reminder); + mockTurnRunFn.mockReturnValue( + (async function* () { + yield { type: LlmEventType.Content, value: 'response' }; + })(), + ); + + const stream = client.sendMessageStream( + [ + { + functionResponse: { + name: agentToolName, + response: { ok: true }, + }, + }, + ], + new AbortController().signal, + 'prompt-agent-result', + { type: SendMessageType.ToolResult }, + ); + for await (const _ of stream) { + // drain + } + + // A delegated execution returning is the progress signal the + // turn budget cannot see (#10953): the reminder must be due now, + // not after three parent tool turns that never come. + expect(mockConfig.takeActiveTodoReminder).toHaveBeenCalledWith( + 'prompt-agent-result', + true, + ); + const request = mockTurnRunFn.mock.lastCall?.[1] as unknown[]; + expect(request).toContain(reminder); + }, + ); + + it('keeps the turn budget for tool results without an Agent execution', async () => { + vi.mocked(mockConfig.takeActiveTodoReminder).mockReturnValue(undefined); + mockTurnRunFn.mockReturnValue( + (async function* () { + yield { type: LlmEventType.Content, value: 'response' }; + })(), + ); + + const stream = client.sendMessageStream( + [{ functionResponse: { name: 'shell', response: { ok: true } } }], + new AbortController().signal, + 'prompt-plain-result', + { type: SendMessageType.ToolResult }, + ); + for await (const _ of stream) { + // drain + } + + expect(mockConfig.takeActiveTodoReminder).toHaveBeenCalledWith( + 'prompt-plain-result', + ); + }); + + it('continues the todo work chain on a user turn while a reminder is registered', async () => { + mockTurnRunFn.mockReturnValue( + (async function* () { + yield { type: LlmEventType.Content, value: 'response' }; + })(), + ); + await runTurn(SendMessageType.UserQuery); + // A registered reminder means the plan still has unfinished items + // (todo_write deletes it on completion). + vi.mocked(mockConfig.getActiveTodoReminder).mockReturnValue( + 'unfinished todo: delegated node', + ); + + const stream = client.sendMessageStream( + [{ text: 'how is progress going?' }], + new AbortController().signal, + 'prompt-user-followup', + { type: SendMessageType.UserQuery }, + ); + for await (const _ of stream) { + // drain + } + + // The follow-up turn must continue the previous chain instead of + // discarding the plan context it asks about (#10953). + expect(mockConfig.startActiveTodoWorkChain).toHaveBeenLastCalledWith( + 'prompt-user-followup', + 'prompt-userQuery', + ); + }); + it('includes active Todo context on the first retry request', async () => { const reminder = 'unfinished todo: run tests'; diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 2630a262b14..c0f5df6acfb 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -94,7 +94,7 @@ import { AUTO_SKILL_THRESHOLD } from '../memory/manager.js'; import { buildRelevantAutoMemoryPrompt } from '../memory/recall.js'; import { isManagedMemoryPath } from '../memory/paths.js'; import { isProjectSkillPath } from '../skills/skill-paths.js'; -import { ToolNames } from '../tools/tool-names.js'; +import { ToolNames, canonicalToolName } from '../tools/tool-names.js'; // Telemetry import { @@ -3384,7 +3384,17 @@ export class LlmClient { // LoopDetected early on the notification turn. if (messageType === SendMessageType.UserQuery) { this.activeAutomaticTodoWorkChainPromptIds.clear(); - this.config.startActiveTodoWorkChain(prompt_id); + // A registered reminder means the previous chain's plan still has + // unfinished items (todo_write deletes it on completion): continue + // that chain instead of discarding its context with the very turn + // that may be asking about it (#10953). + const continuedFrom = + this.activeTodoWorkChainPromptId !== undefined && + this.config.getActiveTodoReminder(this.activeTodoWorkChainPromptId) !== + undefined + ? this.activeTodoWorkChainPromptId + : undefined; + this.config.startActiveTodoWorkChain(prompt_id, continuedFrom); this.activeTodoWorkChainPromptId = prompt_id; } else if (messageType === SendMessageType.Retry) { this.config.startActiveTodoWorkChain( @@ -3907,8 +3917,20 @@ export class LlmClient { // text as a separate user message after the tool messages. requestToSend = [...requestToSend, toolResultMemory.prompt]; } - const activeTodoReminder = - this.config.takeActiveTodoReminder(prompt_id); + // A top-level Agent tool result means a delegated execution just + // returned (#10953): real work advanced while the parent earned a + // single tool turn, so the turn budget cannot come due on its own. + // Force the reminder exactly where the progress information arrives. + const carriesAgentToolResult = requestToSend.some( + (part) => + typeof part === 'object' && + part !== null && + canonicalToolName(part.functionResponse?.name ?? '') === + ToolNames.AGENT, + ); + const activeTodoReminder = carriesAgentToolResult + ? this.config.takeActiveTodoReminder(prompt_id, true) + : this.config.takeActiveTodoReminder(prompt_id); if (activeTodoReminder) { const insertAt = requestToSend.findIndex( (part) =>