From e72ff6b9d7f62529b41fd3da8a4de36a02993847 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Thu, 30 Apr 2026 16:38:25 +0800 Subject: [PATCH 1/2] feat(cli): route foreground subagents through pill+dialog while running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Foreground (synchronous) subagents currently render a live AgentExecutionDisplay inside the parent's pendingHistoryItems block. The frame mutates on every tool call and approval; once it grows past the terminal height (verbose mode, parallel subagents, long tool-call lists) the live-area repaint flickers visibly. This change extends BackgroundTaskRegistry with a flavor: 'foreground' | 'background' discriminator. Foreground entries register at the start of the synchronous tool-call and unregister in its finally path. The pill counts them; the dialog drills into their activity. The inline frame is suppressed during the live phase — only an active, focus-locked approval prompt renders, as a small banner labeled with the originating agent. Once the parent turn commits, the full AgentExecutionDisplay appears in scrollback via Ink's , exactly as before. Foreground entries skip the XML task-notification (the parent receives the result through the normal tool-result channel) and skip the headless holdback (the parent's await already pins the loop). The dialog gates per-agent cancellation behind a two-step confirm so a stray 'x' can't end the user's current turn. --- .../src/ui/components/HistoryItemDisplay.tsx | 1 + .../BackgroundTasksDialog.test.tsx | 67 +++++++- .../background-view/BackgroundTasksDialog.tsx | 73 +++++++-- .../components/messages/ToolGroupMessage.tsx | 10 ++ .../components/messages/ToolMessage.test.tsx | 133 +++++++++++++++ .../ui/components/messages/ToolMessage.tsx | 81 ++++++++-- .../core/src/agents/background-tasks.test.ts | 152 ++++++++++++++++++ packages/core/src/agents/background-tasks.ts | 64 +++++++- packages/core/src/tools/agent/agent.test.ts | 58 ++++++- packages/core/src/tools/agent/agent.ts | 71 +++++++- 10 files changed, 674 insertions(+), 36 deletions(-) diff --git a/packages/cli/src/ui/components/HistoryItemDisplay.tsx b/packages/cli/src/ui/components/HistoryItemDisplay.tsx index 5344da6a1aa..4c00e7b2a11 100644 --- a/packages/cli/src/ui/components/HistoryItemDisplay.tsx +++ b/packages/cli/src/ui/components/HistoryItemDisplay.tsx @@ -210,6 +210,7 @@ const HistoryItemDisplayComponent: React.FC = ({ availableTerminalHeight={availableTerminalHeight} contentWidth={contentWidth} isFocused={isFocused} + isPending={isPending} activeShellPtyId={activeShellPtyId} embeddedShellFocused={embeddedShellFocused} memoryWriteCount={itemForDisplay.memoryWriteCount} diff --git a/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.test.tsx b/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.test.tsx index 2641b440a9b..c865822ed67 100644 --- a/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.test.tsx +++ b/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.test.tsx @@ -133,8 +133,14 @@ function setup(initial: readonly DialogEntry[]): Harness { act(() => handle.current!.setEntries(next)); }, pressKey(key) { + // Real `useKeypress` unbinds the previous callback on rerender, so + // only the most recently registered closure should run. Calling all + // accumulated handlers misses state updates that happened between + // renders (the older closures see stale state) — the symptom looks + // like a re-render race in production code that doesn't exist. act(() => { - for (const h of handlers) h(key); + const latest = handlers[handlers.length - 1]; + if (latest) latest(key); }); }, call(fn) { @@ -199,6 +205,65 @@ describe('BackgroundTasksDialog', () => { expect(h.probe.current!.state.dialogMode).toBe('detail'); }); + it('foreground cancel requires two `x` presses to confirm (one-press is a no-op)', () => { + // Foreground entries block the parent's tool-call: cancelling one ends + // the current turn with a partial result for that subagent. The dialog + // gates the destructive action behind a confirm step so the user can't + // wipe out their turn with a stray keypress. + const fg = entry({ + agentId: 'fg-1', + status: 'running', + flavor: 'foreground', + }); + const h = setup([fg]); + + h.call(() => h.probe.current!.actions.openDialog()); + + h.pressKey({ sequence: 'x' }); + expect(h.cancel).not.toHaveBeenCalled(); + + h.pressKey({ sequence: 'x' }); + expect(h.cancel).toHaveBeenCalledWith('fg-1'); + }); + + it('background cancel still fires on the first `x` press (no confirm)', () => { + // Backwards compatibility: the existing background-only cancel UX + // stays one-shot. Adding a confirm there would regress every workflow + // that relies on quickly cancelling a long-running async agent. + const bg = entry({ + agentId: 'bg-1', + status: 'running', + flavor: 'background', + }); + const h = setup([bg]); + + h.call(() => h.probe.current!.actions.openDialog()); + + h.pressKey({ sequence: 'x' }); + expect(h.cancel).toHaveBeenCalledWith('bg-1'); + }); + + it('Esc backs out of an armed foreground cancel without closing the dialog', () => { + const fg = entry({ + agentId: 'fg-1', + status: 'running', + flavor: 'foreground', + }); + const h = setup([fg]); + + h.call(() => h.probe.current!.actions.openDialog()); + + h.pressKey({ sequence: 'x' }); + h.pressKey({ name: 'escape' }); + // Dialog still open — Esc on the armed cancel resets the confirm + // state instead of nuking the dialog. + expect(h.probe.current!.state.dialogOpen).toBe(true); + + // After the Esc reset, the next `x` arms again rather than confirming. + h.pressKey({ sequence: 'x' }); + expect(h.cancel).not.toHaveBeenCalled(); + }); + it('clamps selectedIndex when entries shrink', () => { const a = entry({ agentId: 'a' }); const b = entry({ agentId: 'b' }); diff --git a/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx b/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx index 5f6828269d6..c48f2b25535 100644 --- a/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx +++ b/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx @@ -96,15 +96,24 @@ function terminalStatusPresentation( } } +// Foreground agent rows get this prefix so users can tell at a glance +// that cancelling one will end the parent's current turn — a much heavier +// consequence than cancelling a truly async background entry. +const FOREGROUND_ROW_PREFIX = '[in turn]'; +const SHELL_ROW_PREFIX = '[shell]'; + function rowLabel(entry: DialogEntry): string { if (entry.kind === 'agent') { - return buildBackgroundEntryLabel(entry, { includePrefix: false }); + const label = buildBackgroundEntryLabel(entry, { includePrefix: false }); + return entry.flavor === 'foreground' + ? `${FOREGROUND_ROW_PREFIX} ${label}` + : label; } - // Shell row: `[shell] `. Prefix mirrors the dialog's "section" - // visual hint without needing per-kind section headers (which would - // complicate the windowing math). The command itself is plain text and - // already truncated by the row renderer's MaxSizedBox. - return `[shell] ${entry.command}`; + // Shell prefix mirrors the dialog's "section" visual hint without needing + // per-kind section headers (which would complicate the windowing math). + // The command itself is plain text and already truncated by the row + // renderer's MaxSizedBox. + return `${SHELL_ROW_PREFIX} ${entry.command}`; } function elapsedFor(entry: { startTime: number; endTime?: number }): string { @@ -495,6 +504,15 @@ export const BackgroundTasksDialog: React.FC = ({ // those transitions — so we re-read from the registry here. const [activityTick, setActivityTick] = useState(0); + // Two-step cancel for foreground entries: cancelling one ends the + // parent's current turn with a partial result for that subagent — + // a much heavier consequence than cancelling a background async task. + // `pendingCancelEntryId` records the entry that has been armed for + // cancellation; the next `x` press confirms. Esc resets. + const [pendingCancelEntryId, setPendingCancelEntryId] = useState< + string | null + >(null); + const selectedEntry = useMemo(() => { const fromSnapshot = entries[selectedIndex] ?? null; if (!fromSnapshot || fromSnapshot.kind !== 'agent') return fromSnapshot; @@ -588,6 +606,22 @@ export const BackgroundTasksDialog: React.FC = ({ } }, [dialogOpen, dialogMode, selectedEntryId, selectedStatus, exitDetail]); + // Encapsulates the cancel flow with the foreground confirm-step. + // Foreground entries: first `x` arms; second `x` confirms. Background + // and shell entries: one-shot cancel (no behavior change). + const handleCancelKey = () => { + if (!selectedEntry) return; + const entryKey = entryId(selectedEntry); + const isForegroundAgent = + selectedEntry.kind === 'agent' && selectedEntry.flavor === 'foreground'; + if (isForegroundAgent && pendingCancelEntryId !== entryKey) { + setPendingCancelEntryId(entryKey); + return; + } + setPendingCancelEntryId(null); + cancelSelected(); + }; + useKeypress( (key) => { if (!dialogOpen) return; @@ -595,10 +629,12 @@ export const BackgroundTasksDialog: React.FC = ({ if (dialogMode === 'list') { if (key.name === 'up') { moveSelectionUp(); + setPendingCancelEntryId(null); return; } if (key.name === 'down') { moveSelectionDown(); + setPendingCancelEntryId(null); return; } if (key.name === 'return') { @@ -606,11 +642,16 @@ export const BackgroundTasksDialog: React.FC = ({ return; } if (key.name === 'escape' || key.name === 'left') { + if (pendingCancelEntryId) { + // Esc backs out of the confirm step before closing the dialog. + setPendingCancelEntryId(null); + return; + } closeDialog(); return; } if (key.sequence === 'x' && !key.ctrl && !key.meta) { - cancelSelected(); + handleCancelKey(); return; } // Note: the "stop all agents" chord (ctrl+x ctrl+k in claw-code) @@ -631,11 +672,15 @@ export const BackgroundTasksDialog: React.FC = ({ key.name === 'return' || key.name === 'space' ) { + if (pendingCancelEntryId && key.name === 'escape') { + setPendingCancelEntryId(null); + return; + } closeDialog(); return; } if (key.sequence === 'x' && !key.ctrl && !key.meta) { - cancelSelected(); + handleCancelKey(); return; } }, @@ -645,8 +690,18 @@ export const BackgroundTasksDialog: React.FC = ({ if (!dialogOpen) return null; // Hint footer — context-sensitive. + const selectedEntryKey = selectedEntry ? entryId(selectedEntry) : null; + const showCancelConfirmHint = + pendingCancelEntryId !== null && pendingCancelEntryId === selectedEntryKey; const hints: string[] = []; - if (dialogMode === 'list') { + if (showCancelConfirmHint) { + // Force the confirmation step into the hint row so the user sees + // exactly what the next `x` will do. + hints.push( + 'x again to confirm stop \u00b7 ends current turn', + 'Esc cancel', + ); + } else if (dialogMode === 'list') { hints.push('\u2191/\u2193 select', 'Enter view'); if (selectedEntry?.status === 'running') hints.push('x stop'); hints.push('\u2190/Esc close'); diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index 71385c533b4..b81e354c1c6 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -36,6 +36,14 @@ interface ToolGroupMessageProps { availableTerminalHeight?: number; contentWidth: number; isFocused?: boolean; + /** + * True when this tool group is being rendered live (in + * `pendingHistoryItems`). False once it commits to Ink's ``. + * The subagent renderer uses this to suppress the live frame for + * foreground subagents (the pill+dialog handle live drill-down) while + * keeping the committed scrollback render unchanged. + */ + isPending?: boolean; activeShellPtyId?: number | null; embeddedShellFocused?: boolean; onShellInputSubmit?: (input: string) => void; @@ -58,6 +66,7 @@ export const ToolGroupMessage: React.FC = ({ availableTerminalHeight, contentWidth, isFocused = true, + isPending = false, activeShellPtyId, embeddedShellFocused, memoryWriteCount, @@ -292,6 +301,7 @@ export const ToolGroupMessage: React.FC = ({ isAgentWithPendingConfirmation(tool.resultDisplay) } isFocused={isSubagentFocused} + isPending={isPending} isWaitingForOtherApproval={isWaitingForOtherApproval} /> diff --git a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx index 8e2fb0d3397..051020c55e1 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx @@ -114,6 +114,13 @@ vi.mock('../subagents/index.js', () => ({ ); }, })); +vi.mock('./ToolConfirmationMessage.js', () => ({ + ToolConfirmationMessage: function MockToolConfirmationMessage() { + // Sentinel string lets `isPending && pendingConfirmation` tests + // assert the banner renders (instead of being suppressed). + return MockApprovalPrompt; + }, +})); // Mock settings const mockSettings: LoadedSettings = { @@ -316,6 +323,132 @@ describe('', () => { expect(output).toContain('Search for files matching pattern'); // Actual task description }); + describe('subagent live-render gating (isPending)', () => { + // The redesign hides the inline AgentExecutionDisplay while a + // foreground subagent runs (the pill+dialog handle drill-down). + // Only an active, focused approval prompt renders inline. + const buildProps = (overrides: { + data: { + subagentName: string; + taskDescription: string; + taskPrompt: string; + status: 'running' | 'completed'; + pendingConfirmation?: object; + }; + isPending?: boolean; + isFocused?: boolean; + }): ToolMessageProps => + ({ + name: 'task', + description: 'Delegate task to subagent', + resultDisplay: { + type: 'task_execution' as const, + ...overrides.data, + }, + status: ToolCallStatus.Executing, + contentWidth: 80, + callId: 'gated-task-call', + confirmationDetails: undefined, + config: mockConfig, + forceShowResult: true, // mirror ToolGroupMessage's forceShowResult + isPending: overrides.isPending, + isFocused: overrides.isFocused, + }) as ToolMessageProps; + + it('isPending && no pendingConfirmation → no inline frame', () => { + const { lastFrame } = renderWithContext( + , + StreamingState.Responding, + ); + const output = lastFrame() ?? ''; + // The mocked AgentExecutionDisplay tags itself with '🤖' — its + // absence proves the inline frame was suppressed. + expect(output).not.toContain('🤖'); + expect(output).not.toContain('MockApprovalPrompt'); + }); + + it('isPending && pendingConfirmation && isFocused → renders banner with agent label', () => { + const { lastFrame } = renderWithContext( + , + StreamingState.Responding, + ); + const output = lastFrame() ?? ''; + // Banner shows up with the originating agent identified, and the + // approval prompt itself renders. + expect(output).toContain('Approval requested by'); + expect(output).toContain('fg-agent'); + expect(output).toContain('MockApprovalPrompt'); + // The full agent frame (header / tool-call list) stays suppressed. + expect(output).not.toContain('🤖'); + }); + + it('isPending && pendingConfirmation && !isFocused → renders nothing (queued in dialog)', () => { + const { lastFrame } = renderWithContext( + , + StreamingState.Responding, + ); + const output = lastFrame() ?? ''; + expect(output).not.toContain('Approval requested by'); + expect(output).not.toContain('MockApprovalPrompt'); + expect(output).not.toContain('🤖'); + }); + + it('!isPending → committed render shows full inline frame', () => { + const { lastFrame } = renderWithContext( + , + StreamingState.Idle, + ); + const output = lastFrame() ?? ''; + // -rendered scrollback: full frame, no flicker concern. + expect(output).toContain('🤖'); + expect(output).toContain('committed-agent'); + }); + }); + it('renders AnsiOutputText for AnsiOutput results', () => { const ansiResult: AnsiOutput = [ [ diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index 417c00010b3..663727b8e2d 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -24,6 +24,7 @@ import type { McpToolProgressData, } from '@qwen-code/qwen-code-core'; import { AgentExecutionDisplay } from '../subagents/index.js'; +import { ToolConfirmationMessage } from './ToolConfirmationMessage.js'; import { PlanSummaryDisplay } from '../PlanSummaryDisplay.js'; import { ShellInputPrompt } from '../ShellInputPrompt.js'; import { SHELL_COMMAND_NAME, SHELL_NAME } from '../../constants.js'; @@ -238,7 +239,19 @@ const PlanResultRenderer: React.FC<{ ); /** - * Component to render subagent execution results + * Component to render subagent execution results. + * + * Live (`isPending===true`): the inline frame is suppressed — running + * subagents are surfaced through the footer pill + dialog instead, which + * removes the live-area flicker that occurred when the frame's tool-call + * list grew past the terminal height. The one exception is an active + * approval prompt that holds the focus lock: that renders as a small + * banner with an agent-name label, since hiding it would block the run + * silently. + * + * Committed (`isPending===false`): renders the full `AgentExecutionDisplay` + * exactly as before. Ink's `` is append-only, so committed frames + * never flicker even when verbose. */ const SubagentExecutionRenderer: React.FC<{ data: AgentResultDisplay; @@ -246,6 +259,7 @@ const SubagentExecutionRenderer: React.FC<{ childWidth: number; config: Config; isFocused?: boolean; + isPending?: boolean; isWaitingForOtherApproval?: boolean; }> = ({ data, @@ -253,17 +267,53 @@ const SubagentExecutionRenderer: React.FC<{ childWidth, config, isFocused, + isPending, isWaitingForOtherApproval, -}) => ( - -); +}) => { + if (isPending) { + // Surface only the approval prompt, and only for the subagent + // currently holding the focus lock. Queued approvals stay invisible + // here — the user can see them in the dialog. + if (data.pendingConfirmation && isFocused) { + const agentLabel = data.subagentName || 'agent'; + return ( + + + Approval requested by + + {agentLabel} + + : + + {isWaitingForOtherApproval && ( + + ⏳ Waiting for other approval... + + )} + + + ); + } + return null; + } + return ( + + ); +}; /** * Component to render string results (markdown or plain text) @@ -344,6 +394,13 @@ export interface ToolMessageProps extends IndividualToolCallDisplay { forceShowResult?: boolean; /** Whether this tool's subagent confirmation prompt should respond to keyboard input. */ isFocused?: boolean; + /** + * True when rendering inside `pendingHistoryItems` (live area), false once + * committed to ``. Foreground subagents suppress their inline + * frame in the live phase — the pill+dialog handle drill-down — but + * always render in scrollback. + */ + isPending?: boolean; /** Whether another subagent's approval currently holds the focus lock, blocking this one. */ isWaitingForOtherApproval?: boolean; } @@ -363,6 +420,7 @@ export const ToolMessage: React.FC = ({ config, forceShowResult, isFocused, + isPending, isWaitingForOtherApproval, executionStartTime, }) => { @@ -518,6 +576,7 @@ export const ToolMessage: React.FC = ({ childWidth={innerWidth} config={config} isFocused={isFocused} + isPending={isPending} isWaitingForOtherApproval={isWaitingForOtherApproval} /> )} diff --git a/packages/core/src/agents/background-tasks.test.ts b/packages/core/src/agents/background-tasks.test.ts index e69155dd93b..77e958c3639 100644 --- a/packages/core/src/agents/background-tasks.test.ts +++ b/packages/core/src/agents/background-tasks.test.ts @@ -773,4 +773,156 @@ describe('BackgroundTaskRegistry', () => { expect(modelText).not.toContain(''); }); }); + + describe('foreground flavor', () => { + it('does not emit a task-notification on complete', () => { + const callback = vi.fn(); + registry.setNotificationCallback(callback); + + registry.register({ + agentId: 'fg-1', + description: 'sync agent', + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + + registry.complete('fg-1', 'result text'); + + // Foreground entries deliver their result through the parent's normal + // tool-result channel; emitting the XML envelope on top would feed + // the parent model the same payload twice. + expect(callback).not.toHaveBeenCalled(); + // The status mutation still happens — internal invariants intact. + expect(registry.get('fg-1')!.status).toBe('completed'); + expect(registry.get('fg-1')!.notified).toBe(true); + }); + + it('does not emit a task-notification on fail', () => { + const callback = vi.fn(); + registry.setNotificationCallback(callback); + + registry.register({ + agentId: 'fg-2', + description: 'sync agent', + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + + registry.fail('fg-2', 'oops'); + + expect(callback).not.toHaveBeenCalled(); + }); + + it('is excluded from hasUnfinalizedTasks()', () => { + registry.register({ + agentId: 'fg-3', + description: 'sync agent', + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + + // A still-running foreground entry must NOT keep the headless + // event loop alive — the parent's tool-call await already does that. + expect(registry.hasUnfinalizedTasks()).toBe(false); + }); + + it('cancel does not schedule the grace timer', () => { + // The grace-timer fallback only matters for background entries that + // might not see their natural completion handler fire. Foreground + // entries unregister themselves in agent.ts's finally path. + vi.useFakeTimers(); + try { + const callback = vi.fn(); + registry.setNotificationCallback(callback); + + registry.register({ + agentId: 'fg-4', + description: 'sync agent', + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + + registry.cancel('fg-4'); + + // Advance well past the 5s grace window — no notification should fire. + vi.advanceTimersByTime(60_000); + expect(callback).not.toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + + it('unregisterForeground removes the entry and emits a status change', () => { + const onStatusChange = vi.fn(); + registry.setStatusChangeCallback(onStatusChange); + + registry.register({ + agentId: 'fg-5', + description: 'sync agent', + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + onStatusChange.mockClear(); + + registry.unregisterForeground('fg-5'); + + expect(registry.get('fg-5')).toBeUndefined(); + expect(onStatusChange).toHaveBeenCalledTimes(1); + }); + + it('unregisterForeground throws if asked to remove a background entry', () => { + // Background entries must terminate via complete/fail/finalizeCancelled + // so the task-notification + headless holdback invariants stay intact. + // A silent no-op would mask caller bugs, so this throws. + registry.register({ + agentId: 'bg-1', + description: 'async agent', + flavor: 'background', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + + expect(() => registry.unregisterForeground('bg-1')).toThrow( + /non-foreground entry bg-1/, + ); + expect(registry.get('bg-1')).toBeDefined(); + }); + + it('unregisterForeground is a no-op for unknown agent ids', () => { + // Idempotent for already-unregistered/never-registered ids — the + // foreground finally path runs unconditionally and shouldn't throw + // if a parallel cancel already cleared the entry. + expect(() => registry.unregisterForeground('missing')).not.toThrow(); + }); + + it('default flavor (absent) behaves as background for emitNotification', () => { + // Older callers omit the flavor field. Backwards compatibility: + // missing flavor is treated as background everywhere. + const callback = vi.fn(); + registry.setNotificationCallback(callback); + + registry.register({ + agentId: 'legacy-1', + description: 'legacy agent', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + + registry.complete('legacy-1', 'done'); + + expect(callback).toHaveBeenCalledOnce(); + }); + }); }); diff --git a/packages/core/src/agents/background-tasks.ts b/packages/core/src/agents/background-tasks.ts index 8e30e182806..82ea514c844 100644 --- a/packages/core/src/agents/background-tasks.ts +++ b/packages/core/src/agents/background-tasks.ts @@ -5,11 +5,19 @@ */ /** - * @fileoverview BackgroundTaskRegistry — tracks background (async) sub-agents. + * @fileoverview BackgroundTaskRegistry — tracks background (async) sub-agents + * and, with `flavor: 'foreground'`, the currently-running synchronous + * sub-agents whose UI is routed through the same pill+dialog while the + * parent turn waits on them. The two flavors share the registry (and the + * dialog wiring) but differ in lifecycle: * - * When the Agent tool is called with `run_in_background: true`, the sub-agent - * runs asynchronously. This registry tracks the lifecycle of each background - * agent so the parent can be notified on completion. + * - `background` entries persist across turns, emit a `` + * on terminal status (the parent's only return channel), and contribute to + * `hasUnfinalizedTasks()` so headless callers keep their loop alive. + * - `foreground` entries live for the duration of the parent's tool-call, + * are unregistered as soon as `execute()` returns, deliver their result + * through the normal tool-result channel (no XML envelope), and don't + * participate in the headless holdback. */ import { createDebugLogger } from '../utils/debugLogger.js'; @@ -98,10 +106,19 @@ export interface BackgroundActivity { at: number; } +export type BackgroundTaskFlavor = 'foreground' | 'background'; + export interface BackgroundTaskEntry { agentId: string; description: string; subagentType?: string; + /** + * `'background'` — async, persists across turns, emits XML notification. + * `'foreground'` — synchronous, unregistered when the tool-call returns, + * delivers results via the normal tool-result channel. + * Defaults to `'background'` when absent (older callers). + */ + flavor?: BackgroundTaskFlavor; status: BackgroundTaskStatus; startTime: number; endTime?: number; @@ -218,6 +235,29 @@ export class BackgroundTaskRegistry { this.emitStatusChange(entry); } + /** + * Remove a foreground entry from the registry without emitting any + * terminal notification. Called by the foreground tool-call's `finally` + * path, which has already delivered the result through the tool-result + * channel — the registry entry has served its UI-surfacing purpose. + * Background entries must go through complete/fail/finalizeCancelled + * instead, so this throws if asked to remove one. + */ + unregisterForeground(agentId: string): void { + const entry = this.agents.get(agentId); + if (!entry) return; + if (entry.flavor !== 'foreground') { + throw new Error( + `unregisterForeground called on non-foreground entry ${agentId} ` + + `(flavor=${entry.flavor ?? 'undefined'}). ` + + `Background entries must terminate via complete/fail/finalizeCancelled.`, + ); + } + this.agents.delete(agentId); + debugLogger.info(`Unregistered foreground agent: ${agentId}`); + this.emitStatusChange(entry); + } + // See complete() for the cancelled → terminal path rationale. fail(agentId: string, error: string, stats?: AgentCompletionStats): void { const entry = this.agents.get(agentId); @@ -253,6 +293,11 @@ export class BackgroundTaskRegistry { debugLogger.info(`Background agent cancelled: ${agentId}`); this.emitStatusChange(entry); + // Foreground entries don't emit XML notifications and unregister + // themselves in the tool-call's finally path, so the grace timer + // would only ever no-op for them. + if (entry.flavor === 'foreground') return; + const timer = setTimeout(() => { this.finalizeCancellationIfPending(agentId); }, CANCEL_GRACE_MS); @@ -337,6 +382,11 @@ export class BackgroundTaskRegistry { */ hasUnfinalizedTasks(): boolean { for (const entry of this.agents.values()) { + // Foreground entries block the parent tool-call synchronously, so the + // headless event loop is already pinned by the `await` on the caller's + // promise — counting them here would be redundant and would also keep + // the loop alive for entries that don't even emit a notification. + if (entry.flavor === 'foreground') continue; if (!entry.notified) return true; } return false; @@ -414,6 +464,12 @@ export class BackgroundTaskRegistry { if (entry.notified) return; entry.notified = true; + // Foreground entries return their result through the parent's normal + // tool-result channel (the `returnDisplay` field on the synchronous + // tool-call). Emitting the XML envelope on top would feed the parent + // model the same payload twice. + if (entry.flavor === 'foreground') return; + if (!this.notificationCallback) return; const statusText = diff --git a/packages/core/src/tools/agent/agent.test.ts b/packages/core/src/tools/agent/agent.test.ts index ac0c55c2ea5..ca47f914197 100644 --- a/packages/core/src/tools/agent/agent.test.ts +++ b/packages/core/src/tools/agent/agent.test.ts @@ -88,7 +88,24 @@ describe('AgentTool', () => { // Setup fake timers vi.useFakeTimers(); - // Create mock config + // Create mock config. The outer describe covers foreground execution + // paths, which now register/unregister in the BackgroundTaskRegistry + // to surface the run in the pill+dialog. A no-op stub registry is + // enough for these tests — they don't assert on registry behavior. + const stubRegistry = { + register: vi.fn(), + unregisterForeground: vi.fn(), + complete: vi.fn(), + fail: vi.fn(), + finalizeCancelled: vi.fn(), + finalizeCancellationIfPending: vi.fn(), + cancel: vi.fn(), + get: vi.fn(), + getAll: vi.fn().mockReturnValue([]), + drainMessages: vi.fn().mockReturnValue([]), + queueMessage: vi.fn(), + appendActivity: vi.fn(), + }; config = { getProjectRoot: vi.fn().mockReturnValue('/test/project'), getSessionId: vi.fn().mockReturnValue('test-session-id'), @@ -99,6 +116,7 @@ describe('AgentTool', () => { getTranscriptPath: vi.fn().mockReturnValue('/test/transcript'), getApprovalMode: vi.fn().mockReturnValue('default'), isTrustedFolder: vi.fn().mockReturnValue(true), + getBackgroundTaskRegistry: vi.fn().mockReturnValue(stubRegistry), } as unknown as Config; changeListeners = []; @@ -407,9 +425,13 @@ describe('AgentTool', () => { expect.any(Object), // config (may be approval-mode override) expect.any(Object), // eventEmitter parameter ); + // Foreground subagents now run with a composed AbortSignal so the + // dialog's per-agent cancel can abort just this child without aborting + // the parent turn. The signal received by the subagent is the + // controller's signal, not whatever the caller passed in. expect(mockAgent.execute).toHaveBeenCalledWith( mockContextState, - undefined, // signal parameter (undefined when not provided) + expect.any(AbortSignal), ); const llmText = partToString(result.llmContent); @@ -733,7 +755,10 @@ describe('AgentTool', () => { expect.stringContaining('file-search-'), 'file-search', PermissionMode.AutoEdit, - undefined, + // Foreground subagents now run with a composed signal (so the + // dialog can cancel just this child) — the hook receives the + // composed signal, not the caller-supplied one. + expect.any(AbortSignal), ); }); @@ -915,7 +940,8 @@ describe('AgentTool', () => { 'Task completed successfully', false, PermissionMode.AutoEdit, - undefined, + // Foreground subagents now run with a composed signal. + expect.any(AbortSignal), ); }); @@ -960,7 +986,8 @@ describe('AgentTool', () => { 'Task completed successfully', true, PermissionMode.AutoEdit, - undefined, + // Foreground subagents now run with a composed signal. + expect.any(AbortSignal), ); }); @@ -1414,10 +1441,12 @@ describe('AgentTool', () => { let mockContextState: ContextState; let mockRegistry: { register: ReturnType; + unregisterForeground: ReturnType; complete: ReturnType; fail: ReturnType; finalizeCancelled: ReturnType; drainMessages: ReturnType; + appendActivity: ReturnType; }; const bgSubagent: SubagentConfig = { @@ -1449,10 +1478,12 @@ describe('AgentTool', () => { mockRegistry = { register: vi.fn(), + unregisterForeground: vi.fn(), complete: vi.fn(), fail: vi.fn(), finalizeCancelled: vi.fn(), drainMessages: vi.fn().mockReturnValue([]), + appendActivity: vi.fn(), }; vi.mocked(config.getApprovalMode).mockReturnValue(ApprovalMode.DEFAULT); @@ -1552,7 +1583,22 @@ describe('AgentTool', () => { const llmText = partToString(result.llmContent); expect(llmText).not.toContain('Background agent launched'); - expect(mockRegistry.register).not.toHaveBeenCalled(); + // Foreground subagents register in the same registry with + // flavor: 'foreground' so the pill+dialog can surface them while + // the parent's tool-call awaits, then unregister in the finally + // path once the call returns. (The tool-result is the durable + // record — the entry does not persist.) + expect(mockRegistry.register).toHaveBeenCalledWith( + expect.objectContaining({ + flavor: 'foreground', + description: 'Search files', + subagentType: 'file-search', + status: 'running', + }), + ); + expect(mockRegistry.unregisterForeground).toHaveBeenCalledWith( + expect.stringContaining('file-search-'), + ); }); it('should allow background in non-interactive mode (headless support)', async () => { diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index ec2ed6e3f27..7b4224abde7 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -1118,6 +1118,7 @@ class AgentToolInvocation extends BaseToolInvocation { agentId: hookOpts.agentId, description: this.params.description, subagentType: subagentConfig.name, + flavor: 'background', status: 'running', startTime: Date.now(), abortController: bgAbortController, @@ -1268,20 +1269,72 @@ class AgentToolInvocation extends BaseToolInvocation { // Same agent-identity frame as the background path: a foreground // subagent can also launch nested agents, and those nested launches // need to see this subagent's id as their `parentAgentId`. - const runFramed = () => - runWithAgentContext({ agentId: hookOpts.agentId }, () => - this.runSubagentWithHooks(subagent, contextState, hookOpts), - ); if (isFork) { // Background fork execution. Run under an AsyncLocalStorage frame so // nested `agent` tool calls by the fork's model can be detected. - void runInForkContext(runFramed); + // Forks run async (return a placeholder); skip foreground registration. + const runFramedFork = () => + runWithAgentContext({ agentId: hookOpts.agentId }, () => + this.runSubagentWithHooks(subagent, contextState, hookOpts), + ); + void runInForkContext(runFramedFork); return { llmContent: [{ text: FORK_PLACEHOLDER_RESULT }], returnDisplay: this.currentDisplay!, }; + } + + // ── Foreground (synchronous) execution path ──────────────── + // Compose a child AbortController so the dialog's per-agent cancel + // can abort just this subagent without aborting the parent turn. + // Parent abort still propagates down (so ESC at the parent kills + // the subagent), but child abort does NOT propagate up. + const fgAbortController = new AbortController(); + const onParentAbort = () => fgAbortController.abort(); + if (signal?.aborted) { + fgAbortController.abort(); } else { + signal?.addEventListener('abort', onParentAbort, { once: true }); + } + + const fgHookOpts = { ...hookOpts, signal: fgAbortController.signal }; + const runFramed = () => + runWithAgentContext({ agentId: hookOpts.agentId }, () => + this.runSubagentWithHooks(subagent, contextState, fgHookOpts), + ); + + // Register in BackgroundTaskRegistry with flavor:'foreground' so the + // pill counts the run and the dialog can drill in. Foreground entries + // skip XML notification and headless-holdback (see the registry for + // the gating logic). + const registry = this.config.getBackgroundTaskRegistry(); + registry.register({ + agentId: hookOpts.agentId, + description: this.params.description, + subagentType: hookOpts.agentType, + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: fgAbortController, + prompt: this.params.prompt, + toolUseId: this.callId, + }); + + // Mirror the background path's progress wiring so the dialog detail + // body has live tool-call activity to render. Re-using the same + // mechanism keeps the dialog code path agnostic of flavor. + const onFgToolCall = (...args: unknown[]) => { + const event = args[0] as AgentToolCallEvent; + registry.appendActivity(hookOpts.agentId, { + name: event.name, + description: event.description, + at: event.timestamp, + }); + }; + this.eventEmitter.on(AgentEventType.TOOL_CALL, onFgToolCall); + + try { await runFramed(); const finalText = subagent.getFinalText(); const terminateMode = subagent.getTerminateMode(); @@ -1295,6 +1348,14 @@ class AgentToolInvocation extends BaseToolInvocation { llmContent: [{ text: finalText }], returnDisplay: this.currentDisplay!, }; + } finally { + this.eventEmitter.off(AgentEventType.TOOL_CALL, onFgToolCall); + signal?.removeEventListener('abort', onParentAbort); + // Foreground entries leave the registry as soon as the tool-call + // returns — the parent's tool-result is the durable record. Doing + // this in finally guarantees we clean up on success, failure, + // cancel, AND any unexpected throw inside runFramed. + registry.unregisterForeground(hookOpts.agentId); } } catch (error) { const errorMessage = From 0a0c17f7ec77ee6a724cdc107f5728cc87fa13ce Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Wed, 6 May 2026 10:07:23 +0800 Subject: [PATCH 2/2] fix(cli): address review findings on foreground subagent routing - Gate `registerCallback` on background flavor so foreground entries don't leak orphaned `task_started` SDK events without a matching terminal notification. - Render a queued-approval marker for non-focus subagents instead of returning null, so a queued approval is visible in the main view. - Move `emitStatusChange` before `agents.delete` in `unregisterForeground` to match the ordering used by complete/fail/cancel/finalize. - Prefix the foreground tool result with a cancel marker when `terminateMode === CANCELLED`, so the parent model can distinguish a user-cancelled run from a successful completion. - Mirror the background path's stats wiring on the foreground path so `entry.stats` stays current and the dialog detail subtitle shows tool count + tokens for foreground runs. - Remove the unreachable `isWaitingForOtherApproval` branch (subsumed by the queued-approval marker above). - Reset the foreground confirm-step on detail-mode `left` and ignore `x` on terminal entries so an armed cancel can't carry into list mode and the hint footer/handler stay in sync. - Test factory uses a `baseProps` spread instead of `as` cast so a future required field on `ToolMessageProps` is a compile-time miss. --- .../BackgroundTasksDialog.test.tsx | 49 ++++++++++++++ .../background-view/BackgroundTasksDialog.tsx | 14 ++++ .../components/messages/ToolMessage.test.tsx | 37 +++++++---- .../ui/components/messages/ToolMessage.tsx | 25 ++++--- .../core/src/agents/background-tasks.test.ts | 65 ++++++++++++++++++- packages/core/src/agents/background-tasks.ts | 12 +++- packages/core/src/tools/agent/agent.test.ts | 35 ++++++++++ packages/core/src/tools/agent/agent.ts | 51 ++++++++++++++- 8 files changed, 264 insertions(+), 24 deletions(-) diff --git a/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.test.tsx b/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.test.tsx index cebe2de6756..7f7d0eda6c9 100644 --- a/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.test.tsx +++ b/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.test.tsx @@ -317,6 +317,55 @@ describe('BackgroundTasksDialog', () => { expect(h.cancel).toHaveBeenCalledWith('bg-1'); }); + it('ignores `x` on a terminal foreground entry (no arm, no cancel call)', () => { + // A foreground entry briefly stays visible after settling but before + // the tool-call's finally path unregisters it. The dialog's hint + // footer drops "x stop" once status leaves 'running', but without + // gating handleCancelKey itself, the first `x` would still arm a + // confirm step on the (now-terminal) entry — surfacing a misleading + // "x again to confirm stop" line that does nothing. + const completed = entry({ + agentId: 'fg-done', + status: 'completed', + flavor: 'foreground', + }); + const h = setup([completed]); + + h.call(() => h.probe.current!.actions.openDialog()); + + h.pressKey({ sequence: 'x' }); + expect(h.lastFrame()).not.toContain('x again to confirm stop'); + + h.pressKey({ sequence: 'x' }); + expect(h.cancel).not.toHaveBeenCalled(); + }); + + it('detail-mode left clears any armed foreground cancel before exiting', () => { + // Detail-mode `x` arms the foreground confirm step on the focused + // entry. If the user presses `left` to back out without confirming, + // the armed state must NOT carry into list mode — otherwise the + // hint bar still shows "x again to confirm stop" and the next `x` + // unintentionally cancels the run. + const fg = entry({ + agentId: 'fg-1', + status: 'running', + flavor: 'foreground', + }); + const h = setup([fg]); + + h.call(() => h.probe.current!.actions.openDialog()); + h.call(() => h.probe.current!.actions.enterDetail()); + + h.pressKey({ sequence: 'x' }); + h.pressKey({ name: 'left' }); + expect(h.probe.current!.state.dialogMode).toBe('list'); + + // Back in list mode, the next `x` arms again rather than confirming + // a stale armed state inherited from detail mode. + h.pressKey({ sequence: 'x' }); + expect(h.cancel).not.toHaveBeenCalled(); + }); + it('Esc backs out of an armed foreground cancel without closing the dialog', () => { const fg = entry({ agentId: 'fg-1', diff --git a/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx b/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx index 5239b6f8696..b048ac40ea4 100644 --- a/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx +++ b/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx @@ -772,6 +772,16 @@ export const BackgroundTasksDialog: React.FC = ({ // and shell entries: one-shot cancel (no behavior change). const handleCancelKey = () => { if (!selectedEntry) return; + // `x` only has a meaning for entries the user can still act on: + // `running` → cancel, `paused` (agent kind) → abandon. Terminal + // statuses (completed/failed/cancelled) ignore the keypress so a + // foreground entry that just settled can't display the misleading + // "x again to confirm stop" line during the brief window before it + // unregisters. + const isCancelable = selectedEntry.status === 'running'; + const isAbandonable = + selectedEntry.kind === 'agent' && selectedEntry.status === 'paused'; + if (!isCancelable && !isAbandonable) return; const entryKey = entryId(selectedEntry); const isForegroundAgent = selectedEntry.kind === 'agent' && selectedEntry.flavor === 'foreground'; @@ -829,6 +839,10 @@ export const BackgroundTasksDialog: React.FC = ({ // detail mode if (key.name === 'left') { + // Reset the foreground confirm-step before leaving detail so the + // armed state can't carry into list mode and turn a stray `x` into + // an unintended cancel on the same entry. + setPendingCancelEntryId(null); exitDetail(); return; } diff --git a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx index 051020c55e1..568c9c12c13 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx @@ -337,23 +337,30 @@ describe('', () => { }; isPending?: boolean; isFocused?: boolean; - }): ToolMessageProps => - ({ + isWaitingForOtherApproval?: boolean; + }): ToolMessageProps => { + // Spread the existing typed defaults so any future required field + // on `ToolMessageProps` becomes a compile-time miss instead of + // silently defaulting to undefined. Only the agent-specific + // `resultDisplay` shape uses a cast — its `pendingConfirmation` + // intentionally accepts a loose `object` fixture for these tests. + const resultDisplay = { + type: 'task_execution' as const, + ...overrides.data, + } as ToolMessageProps['resultDisplay']; + return { + ...baseProps, name: 'task', description: 'Delegate task to subagent', - resultDisplay: { - type: 'task_execution' as const, - ...overrides.data, - }, + resultDisplay, status: ToolCallStatus.Executing, - contentWidth: 80, callId: 'gated-task-call', - confirmationDetails: undefined, - config: mockConfig, forceShowResult: true, // mirror ToolGroupMessage's forceShowResult isPending: overrides.isPending, isFocused: overrides.isFocused, - }) as ToolMessageProps; + isWaitingForOtherApproval: overrides.isWaitingForOtherApproval, + }; + }; it('isPending && no pendingConfirmation → no inline frame', () => { const { lastFrame } = renderWithContext( @@ -404,7 +411,11 @@ describe('', () => { expect(output).not.toContain('🤖'); }); - it('isPending && pendingConfirmation && !isFocused → renders nothing (queued in dialog)', () => { + it('isPending && pendingConfirmation && !isFocused → renders queued marker (one-line)', () => { + // Without this marker, a subagent waiting on another subagent's + // approval would be invisible in the main view — the user would + // have no inline signal that an approval is queued and would have + // to open the dialog to discover it. const { lastFrame } = renderWithContext( ', () => { StreamingState.Responding, ); const output = lastFrame() ?? ''; + expect(output).toContain('Queued approval:'); + expect(output).toContain('queued-agent'); + // The full prompt + frame stay suppressed — only the focus-holder + // renders the active prompt above this row. expect(output).not.toContain('Approval requested by'); expect(output).not.toContain('MockApprovalPrompt'); expect(output).not.toContain('🤖'); diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index 4b4a668892c..ebdfed81211 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -271,10 +271,9 @@ const SubagentExecutionRenderer: React.FC<{ isWaitingForOtherApproval, }) => { if (isPending) { - // Surface only the approval prompt, and only for the subagent - // currently holding the focus lock. Queued approvals stay invisible - // here — the user can see them in the dialog. if (data.pendingConfirmation && isFocused) { + // Active approval prompt for the focus-holding subagent — render + // inline so the user can act on it without opening the dialog. const agentLabel = data.subagentName || 'agent'; return ( @@ -285,11 +284,6 @@ const SubagentExecutionRenderer: React.FC<{ : - {isWaitingForOtherApproval && ( - - ⏳ Waiting for other approval... - - )} ); } + if (data.pendingConfirmation) { + // Queued approval — another subagent currently holds the focus lock. + // A one-line marker keeps the user aware that something is waiting + // without opening the dialog; the full prompt renders on the + // focus-holder above and inside `BackgroundTasksDialog`. + const agentLabel = data.subagentName || 'agent'; + return ( + + + ⏳ Queued approval:{' '} + + {agentLabel} + + ); + } return null; } return ( diff --git a/packages/core/src/agents/background-tasks.test.ts b/packages/core/src/agents/background-tasks.test.ts index 54e373b99fa..3c781ec2836 100644 --- a/packages/core/src/agents/background-tasks.test.ts +++ b/packages/core/src/agents/background-tasks.test.ts @@ -5,7 +5,10 @@ */ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { BackgroundTaskRegistry } from './background-tasks.js'; +import { + BackgroundTaskRegistry, + type BackgroundTaskEntry, +} from './background-tasks.js'; import * as transcript from './agent-transcript.js'; describe('BackgroundTaskRegistry', () => { @@ -1042,6 +1045,66 @@ describe('BackgroundTaskRegistry', () => { expect(() => registry.unregisterForeground('missing')).not.toThrow(); }); + it('does not invoke the register callback for foreground entries', () => { + // Non-interactive bridges setRegisterCallback to a `task_started` + // SDK event. Foreground entries never produce a paired terminal + // task-notification (see emitNotification's flavor gate), so letting + // them fire `task_started` would leak orphaned in-flight tasks to + // SDK consumers. + const onRegister = vi.fn(); + registry.setRegisterCallback(onRegister); + + registry.register({ + agentId: 'fg-no-register-cb', + description: 'sync agent', + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + + expect(onRegister).not.toHaveBeenCalled(); + + // Background entries still fire it. + registry.register({ + agentId: 'bg-fires-register-cb', + description: 'async agent', + flavor: 'background', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + expect(onRegister).toHaveBeenCalledTimes(1); + expect(onRegister.mock.calls[0]![0].agentId).toBe('bg-fires-register-cb'); + }); + + it('unregisterForeground emits status change before removing the entry', () => { + // Mirrors the ordering used by complete/fail/cancel/finalize so a + // statusChange callback that re-reads `registry.get(agentId)` from + // inside the callback sees the entry across every terminal path. + registry.register({ + agentId: 'fg-unregister-order', + description: 'sync agent', + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + + let observedFromCallback: BackgroundTaskEntry | undefined; + registry.setStatusChangeCallback((entry) => { + if (entry?.agentId === 'fg-unregister-order') { + observedFromCallback = registry.get(entry.agentId); + } + }); + + registry.unregisterForeground('fg-unregister-order'); + + expect(observedFromCallback).toBeDefined(); + expect(observedFromCallback!.agentId).toBe('fg-unregister-order'); + expect(registry.get('fg-unregister-order')).toBeUndefined(); + }); + it('default flavor (absent) behaves as background for emitNotification', () => { // Older callers omit the flavor field. Backwards compatibility: // missing flavor is treated as background everywhere. diff --git a/packages/core/src/agents/background-tasks.ts b/packages/core/src/agents/background-tasks.ts index ecda4f0f0bb..8ef752f7013 100644 --- a/packages/core/src/agents/background-tasks.ts +++ b/packages/core/src/agents/background-tasks.ts @@ -214,7 +214,12 @@ export class BackgroundTaskRegistry { this.agents.set(entry.agentId, entry); debugLogger.info(`Registered background agent: ${entry.agentId}`); - if (this.registerCallback) { + // Foreground entries are paired with a synchronous tool-call result on + // the parent's response and never emit a terminal `task_notification` + // (see emitNotification's flavor gate). Letting them fire the register + // callback would emit a `task_started` SDK event without a matching + // completion event, breaking the lifecycle contract for SDK consumers. + if (entry.flavor !== 'foreground' && this.registerCallback) { try { this.registerCallback(entry); } catch (error) { @@ -270,9 +275,12 @@ export class BackgroundTaskRegistry { `Background entries must terminate via complete/fail/finalizeCancelled.`, ); } + // Emit before delete so any future BackgroundStatusChangeCallback that + // re-reads `registry.get(agentId)` from inside the callback sees the + // entry, matching the ordering used by complete/fail/cancel/finalize. + this.emitStatusChange(entry); this.agents.delete(agentId); debugLogger.info(`Unregistered foreground agent: ${agentId}`); - this.emitStatusChange(entry); } // See complete() for the cancelled → terminal path rationale. diff --git a/packages/core/src/tools/agent/agent.test.ts b/packages/core/src/tools/agent/agent.test.ts index 11a80cccd21..150b0f03c94 100644 --- a/packages/core/src/tools/agent/agent.test.ts +++ b/packages/core/src/tools/agent/agent.test.ts @@ -1646,6 +1646,41 @@ describe('AgentTool', () => { ); }); + it('foreground CANCELLED prefixes the partial result so the parent sees the cancel', async () => { + // Without this prefix, a user-cancelled foreground subagent returns + // the same `{ llmContent: [{ text: finalText }] }` shape as a + // successful run, leaving the parent model unable to tell that the + // partial result is incomplete. The background path surfaces this + // through the registry's `cancelled` XML envelope; + // the foreground path has no equivalent envelope, so the marker + // rides the llmContent payload itself. + const fgSubagent: SubagentConfig = { + ...bgSubagent, + name: 'file-search', + background: undefined, + }; + vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue(fgSubagent); + vi.mocked(mockAgent.getFinalText).mockReturnValue('halfway through'); + vi.mocked(mockAgent.getTerminateMode).mockReturnValue( + AgentTerminateMode.CANCELLED, + ); + + const params: AgentParams = { + description: 'Search files', + prompt: 'Find all TypeScript files', + subagent_type: 'file-search', + }; + + const invocation = ( + agentTool as AgentToolWithProtectedMethods + ).createInvocation(params); + const result = await invocation.execute(); + + const llmText = partToString(result.llmContent); + expect(llmText).toContain('Agent was cancelled by the user.'); + expect(llmText).toContain('halfway through'); + }); + it('should allow background in non-interactive mode (headless support)', async () => { vi.mocked( config.isInteractive as ReturnType, diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index e6ac422aa4b..d66376d8dce 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -1388,17 +1388,43 @@ class AgentToolInvocation extends BaseToolInvocation { }); // Mirror the background path's progress wiring so the dialog detail - // body has live tool-call activity to render. Re-using the same - // mechanism keeps the dialog code path agnostic of flavor. + // body has live tool-call activity AND a current `entry.stats` + // subtitle (`N tools · X tokens · Ys`). Without this, foreground + // entries collapse to elapsed-only in the dialog while background + // entries show full stats — strictly less information for the same + // runtime events. + // + // This is a separate listener from setupEventListeners' TOOL_CALL + // handler (which feeds `currentDisplay.toolCalls` for the committed + // inline frame). They consume different state — committed inline UI + // vs. live registry stats — and setupEventListeners runs before we + // know the flavor or the registry id, so folding them is awkward. + let fgLiveToolCallCount = 0; + const refreshFgLiveStats = () => { + const entry = registry.get(hookOpts.agentId); + if (!entry || entry.status !== 'running') return; + const summary = subagent.getExecutionSummary(); + entry.stats = { + totalTokens: summary.totalTokens, + toolUses: fgLiveToolCallCount, + durationMs: summary.totalDurationMs, + }; + }; const onFgToolCall = (...args: unknown[]) => { const event = args[0] as AgentToolCallEvent; + fgLiveToolCallCount += 1; + refreshFgLiveStats(); registry.appendActivity(hookOpts.agentId, { name: event.name, description: event.description, at: event.timestamp, }); }; + const onFgUsageMetadata = () => { + refreshFgLiveStats(); + }; this.eventEmitter.on(AgentEventType.TOOL_CALL, onFgToolCall); + this.eventEmitter.on(AgentEventType.USAGE_METADATA, onFgUsageMetadata); try { await runFramed(); @@ -1410,12 +1436,33 @@ class AgentToolInvocation extends BaseToolInvocation { returnDisplay: this.currentDisplay!, }; } + if (terminateMode === AgentTerminateMode.CANCELLED) { + // Distinguish a user-cancelled run from a successful complete in + // the parent model's tool result. Without this prefix, a cancel + // collapses into the same `{ llmContent: [{ text: finalText }] }` + // shape as a successful run — the parent can't tell that the + // partial result is incomplete and may act on it as if the agent + // had finished. The background path surfaces this via the + // `cancelled` XML envelope; the foreground path + // has no equivalent envelope, so the marker has to ride the + // llmContent payload itself. + const partial = finalText || '(no partial result captured)'; + return { + llmContent: [ + { + text: `Agent was cancelled by the user. Partial result follows:\n\n${partial}`, + }, + ], + returnDisplay: this.currentDisplay!, + }; + } return { llmContent: [{ text: finalText }], returnDisplay: this.currentDisplay!, }; } finally { this.eventEmitter.off(AgentEventType.TOOL_CALL, onFgToolCall); + this.eventEmitter.off(AgentEventType.USAGE_METADATA, onFgUsageMetadata); signal?.removeEventListener('abort', onParentAbort); // Foreground entries leave the registry as soon as the tool-call // returns — the parent's tool-result is the durable record. Doing