From eda6d7bd65e6fd4403eb761e443e26fbbea86cfe Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Thu, 7 May 2026 11:02:47 +0000 Subject: [PATCH] feat(core): retain finished foreground agents in BackgroundTaskRegistry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Foreground subagents used to disappear from the registry the moment their tool-call returned, which meant the background-tasks dialog could only ever show a stale "running" snapshot of finished work — the entry was deleted but the React state was never re-broadcast, so it lingered with `[in turn]` and no terminal icon for the rest of the session. Replace `unregisterForeground` with `settleForeground(id, status, details)`, which transitions the entry to a terminal status, attaches the final stats from the tool-call's `getExecutionSummary()`, and retains the entry up to a 128-cap (mirrors `MonitorRegistry`). Eviction is FIFO by `endTime` and triggers from every terminal-transition path so a background-only session also stays bounded. Background entries that are cancelled-but-not-finalized are excluded from prune to protect the notification contract. The dialog row now drops the `[in turn]` prefix once status leaves `running` so settled entries read cleanly. Only `AgentTerminateMode.GOAL` settles as completed; `TIMEOUT` / `MAX_TURNS` / `SHUTDOWN` / `ERROR` all settle as failed with the reason on the entry, so the dialog detail view shows accurate outcomes instead of a green check on a run that hit a turn limit. --- .../BackgroundTasksDialog.test.tsx | 26 ++ .../background-view/BackgroundTasksDialog.tsx | 10 +- .../core/src/agents/background-tasks.test.ts | 325 +++++++++++++++++- packages/core/src/agents/background-tasks.ts | 123 ++++++- packages/core/src/tools/agent/agent.test.ts | 16 +- packages/core/src/tools/agent/agent.ts | 46 ++- 6 files changed, 491 insertions(+), 55 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 d38d5e30310..914bbe97cb5 100644 --- a/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.test.tsx +++ b/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.test.tsx @@ -340,6 +340,32 @@ describe('BackgroundTasksDialog', () => { expect(h.cancel).toHaveBeenCalledWith('bg-1'); }); + it('drops the [in turn] prefix on settled foreground rows', () => { + // The prefix exists to warn users that cancelling will end the parent's + // turn. Once the entry is terminal there is nothing to cancel, so the + // warning would just be noise alongside the dimmed-row presentation. + const running = entry({ + agentId: 'fg-running', + status: 'running', + flavor: 'foreground', + description: 'still going', + }); + const completed = entry({ + agentId: 'fg-done', + status: 'completed', + flavor: 'foreground', + description: 'finished work', + }); + const h = setup([running, completed]); + h.call(() => h.probe.current!.actions.openDialog()); + + const frame = h.lastFrame() ?? ''; + expect(frame).toContain('[in turn] still going'); + // The settled row keeps the description but loses the prefix. + expect(frame).toContain('finished work'); + expect(frame).not.toContain('[in turn] finished work'); + }); + 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 diff --git a/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx b/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx index 59f2aee887a..b09b2dc524b 100644 --- a/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx +++ b/packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx @@ -90,9 +90,10 @@ 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. +// Live foreground rows get this prefix to warn the user that cancelling +// one ends the parent's current turn — a much heavier consequence than +// cancelling a truly async background entry. Settled foreground rows +// drop the prefix because there's nothing left to cancel. const FOREGROUND_ROW_PREFIX = '[in turn]'; const SHELL_ROW_PREFIX = '[shell]'; @@ -100,7 +101,8 @@ function rowLabel(entry: DialogEntry): string { switch (entry.kind) { case 'agent': { const label = buildBackgroundEntryLabel(entry, { includePrefix: false }); - return entry.flavor === 'foreground' + const isLive = entry.status === 'running' || entry.status === 'paused'; + return entry.flavor === 'foreground' && isLive ? `${FOREGROUND_ROW_PREFIX} ${label}` : label; } diff --git a/packages/core/src/agents/background-tasks.test.ts b/packages/core/src/agents/background-tasks.test.ts index 7ad2fd06683..a9b6ff36068 100644 --- a/packages/core/src/agents/background-tasks.test.ts +++ b/packages/core/src/agents/background-tasks.test.ts @@ -7,6 +7,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { BackgroundTaskRegistry, + MAX_RETAINED_TERMINAL_BACKGROUND_TASKS, type BackgroundTaskEntry, } from './background-tasks.js'; import * as transcript from './agent-transcript.js'; @@ -1047,7 +1048,7 @@ describe('BackgroundTaskRegistry', () => { } }); - it('unregisterForeground removes the entry and emits a status change', () => { + it('settleForeground transitions to terminal status, retains the entry, and emits a status change', () => { const onStatusChange = vi.fn(); registry.setStatusChangeCallback(onStatusChange); @@ -1061,13 +1062,41 @@ describe('BackgroundTaskRegistry', () => { }); onStatusChange.mockClear(); - registry.unregisterForeground('fg-5'); + registry.settleForeground('fg-5', 'completed'); - expect(registry.get('fg-5')).toBeUndefined(); + const settled = registry.get('fg-5'); + expect(settled).toBeDefined(); + expect(settled!.status).toBe('completed'); + expect(settled!.endTime).toBeGreaterThan(0); expect(onStatusChange).toHaveBeenCalledTimes(1); }); - it('unregisterForeground throws if asked to remove a background entry', () => { + it('settleForeground attaches details (error, stats) for failed runs', () => { + registry.register({ + agentId: 'fg-failed', + description: 'sync agent', + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + + registry.settleForeground('fg-failed', 'failed', { + error: 'tool error: syntax', + stats: { totalTokens: 42, toolUses: 3, durationMs: 1200 }, + }); + + const settled = registry.get('fg-failed'); + expect(settled!.status).toBe('failed'); + expect(settled!.error).toBe('tool error: syntax'); + expect(settled!.stats).toEqual({ + totalTokens: 42, + toolUses: 3, + durationMs: 1200, + }); + }); + + it('settleForeground throws if asked to settle 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. @@ -1080,17 +1109,50 @@ describe('BackgroundTaskRegistry', () => { abortController: new AbortController(), }); - expect(() => registry.unregisterForeground('bg-1')).toThrow( + expect(() => registry.settleForeground('bg-1', 'completed')).toThrow( /non-foreground entry bg-1/, ); - expect(registry.get('bg-1')).toBeDefined(); + // Background entry's status is unchanged. + expect(registry.get('bg-1')!.status).toBe('running'); }); - it('unregisterForeground is a no-op for unknown agent ids', () => { + it('settleForeground 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(); + expect(() => + registry.settleForeground('missing', 'completed'), + ).not.toThrow(); + }); + + it('settleForeground is idempotent on already-terminal entries', () => { + // Already-terminal entries early-return: no mutation, no prune, + // no status-change emit — avoids a redundant UI refresh in the + // double-settle case (external `cancel()` racing the tool-call's + // finally). + const onStatusChange = vi.fn(); + registry.setStatusChangeCallback(onStatusChange); + + registry.register({ + agentId: 'fg-twice', + description: 'sync agent', + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + + registry.settleForeground('fg-twice', 'completed', { error: 'first' }); + const firstEnd = registry.get('fg-twice')!.endTime; + onStatusChange.mockClear(); + + registry.settleForeground('fg-twice', 'failed', { error: 'second' }); + + const settled = registry.get('fg-twice'); + expect(settled!.status).toBe('completed'); + expect(settled!.endTime).toBe(firstEnd); + expect(settled!.error).toBe('first'); + expect(onStatusChange).not.toHaveBeenCalled(); }); it('does not invoke the register callback for foreground entries', () => { @@ -1126,12 +1188,14 @@ describe('BackgroundTaskRegistry', () => { 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. + it('settleForeground prunes before emitting so subscribers see post-prune state', () => { + // Subscribers that snapshot `registry.getAll()` from inside the + // status-change callback must observe the registry with the cap + // already enforced — otherwise an over-cap settle would briefly + // expose a phantom entry that is gone from the registry by the + // next read. Mirrors `MonitorRegistry.settle()`'s order. registry.register({ - agentId: 'fg-unregister-order', + agentId: 'fg-settle-order', description: 'sync agent', flavor: 'foreground', status: 'running', @@ -1141,16 +1205,18 @@ describe('BackgroundTaskRegistry', () => { let observedFromCallback: BackgroundTaskEntry | undefined; registry.setStatusChangeCallback((entry) => { - if (entry?.agentId === 'fg-unregister-order') { + if (entry?.agentId === 'fg-settle-order') { observedFromCallback = registry.get(entry.agentId); } }); - registry.unregisterForeground('fg-unregister-order'); + registry.settleForeground('fg-settle-order', 'completed'); + // The just-settled entry has the newest endTime, so prune never + // evicts it. The callback sees it with terminal status. expect(observedFromCallback).toBeDefined(); - expect(observedFromCallback!.agentId).toBe('fg-unregister-order'); - expect(registry.get('fg-unregister-order')).toBeUndefined(); + expect(observedFromCallback!.status).toBe('completed'); + expect(registry.get('fg-settle-order')).toBeDefined(); }); it('default flavor (absent) behaves as background for emitNotification', () => { @@ -1172,4 +1238,229 @@ describe('BackgroundTaskRegistry', () => { expect(callback).toHaveBeenCalledOnce(); }); }); + + describe('terminal entry retention (cap + FIFO eviction)', () => { + function registerForeground( + id: string, + startTime: number, + ): BackgroundTaskEntry { + const entry: BackgroundTaskEntry = { + agentId: id, + description: id, + flavor: 'foreground', + status: 'running', + startTime, + abortController: new AbortController(), + }; + registry.register(entry); + return entry; + } + + it('retains terminal foreground entries up to the cap', () => { + const cap = MAX_RETAINED_TERMINAL_BACKGROUND_TASKS; + // Sanity-check the constant matches the design intent before + // committing to a slow eviction test below. + expect(cap).toBe(128); + + for (let i = 0; i < cap; i++) { + registerForeground(`fg-${i}`, 1_000 + i); + registry.settleForeground(`fg-${i}`, 'completed'); + } + expect(registry.getAll().length).toBe(cap); + // Spot-check the first and last entries are still retained. + expect(registry.get('fg-0')).toBeDefined(); + expect(registry.get(`fg-${cap - 1}`)).toBeDefined(); + }); + + it('evicts the oldest terminal entry when the cap is exceeded (FIFO by endTime)', () => { + vi.useFakeTimers(); + try { + const cap = MAX_RETAINED_TERMINAL_BACKGROUND_TASKS; + // Settle each entry at a strictly increasing endTime so FIFO order + // is unambiguous. After cap+1 settles, the very first one (oldest + // endTime) must be the eviction victim. + for (let i = 0; i < cap + 1; i++) { + vi.setSystemTime(new Date(2_000_000_000_000 + i * 1000)); + registerForeground(`fg-${i}`, 2_000_000_000_000 + i * 1000); + registry.settleForeground(`fg-${i}`, 'completed'); + } + expect(registry.getAll().length).toBe(cap); + expect(registry.get('fg-0')).toBeUndefined(); + expect(registry.get('fg-1')).toBeDefined(); + expect(registry.get(`fg-${cap}`)).toBeDefined(); + } finally { + vi.useRealTimers(); + } + }); + + it('eviction is uniform across foreground and background flavors', () => { + vi.useFakeTimers(); + try { + const cap = MAX_RETAINED_TERMINAL_BACKGROUND_TASKS; + // Settle one background entry first (oldest), then `cap` foreground + // entries. The background entry should be evicted because its + // endTime is the smallest. + vi.setSystemTime(new Date(3_000_000_000_000)); + registry.register({ + agentId: 'bg-oldest', + description: 'bg', + flavor: 'background', + status: 'running', + startTime: 3_000_000_000_000, + abortController: new AbortController(), + }); + registry.complete('bg-oldest', 'done'); + + for (let i = 0; i < cap; i++) { + vi.setSystemTime(new Date(3_000_000_000_000 + (i + 1) * 1000)); + registerForeground(`fg-${i}`, 3_000_000_000_000 + (i + 1) * 1000); + registry.settleForeground(`fg-${i}`, 'completed'); + } + + expect(registry.getAll().length).toBe(cap); + expect(registry.get('bg-oldest')).toBeUndefined(); + expect(registry.get('fg-0')).toBeDefined(); + } finally { + vi.useRealTimers(); + } + }); + + it('does not evict a cancelled-but-not-finalized background entry', () => { + // `cancel()` sets status to 'cancelled' but does NOT emit the terminal + // task-notification — the natural handler (or grace timer) does that + // later via `finalizeCancelled` / `finalizeCancellationIfPending`. + // Pruning the entry before finalization would orphan the + // task-notification and strand any headless caller waiting on the + // holdback. The guard requires `notified === true` for background + // entries before they're considered prunable. + vi.useFakeTimers(); + try { + const cap = MAX_RETAINED_TERMINAL_BACKGROUND_TASKS; + vi.setSystemTime(new Date(5_000_000_000_000)); + registry.register({ + agentId: 'bg-cancelling', + description: 'mid-cancel', + flavor: 'background', + status: 'running', + startTime: 5_000_000_000_000, + abortController: new AbortController(), + }); + // Plain cancel() schedules the grace timer; the entry is now + // status='cancelled' but notified is still false. (Passing + // notify:false would set notified=true intentionally — that's + // the session-reset path, not what we're modeling here.) + registry.cancel('bg-cancelling'); + + // Push the cap with foreground settles. Without the guard, the + // oldest (cancelled-not-notified) bg entry would be evicted. + for (let i = 0; i < cap + 5; i++) { + vi.setSystemTime(new Date(5_000_000_000_000 + (i + 1) * 1000)); + registerForeground(`fg-${i}`, 5_000_000_000_000 + (i + 1) * 1000); + registry.settleForeground(`fg-${i}`, 'completed'); + } + + // Cancelled-not-notified entry survives; foreground entries beyond + // the cap are evicted instead. + expect(registry.get('bg-cancelling')).toBeDefined(); + expect(registry.get('bg-cancelling')!.status).toBe('cancelled'); + expect(registry.get('bg-cancelling')!.notified).not.toBe(true); + } finally { + vi.useRealTimers(); + } + }); + + it('background terminal transitions trigger the cap (no foreground required)', () => { + // A session that runs many background agents to completion without + // ever spawning a foreground subagent must still enforce the cap — + // otherwise `complete()` / `fail()` / `finalizeCancelled()` would + // accumulate forever. + vi.useFakeTimers(); + try { + const cap = MAX_RETAINED_TERMINAL_BACKGROUND_TASKS; + for (let i = 0; i < cap + 3; i++) { + vi.setSystemTime(new Date(6_000_000_000_000 + i * 1000)); + registry.register({ + agentId: `bg-${i}`, + description: `bg ${i}`, + flavor: 'background', + status: 'running', + startTime: 6_000_000_000_000 + i * 1000, + abortController: new AbortController(), + }); + registry.complete(`bg-${i}`, 'done'); + } + expect(registry.getAll().length).toBe(cap); + // Oldest 3 evicted, newest cap retained. + expect(registry.get('bg-0')).toBeUndefined(); + expect(registry.get('bg-1')).toBeUndefined(); + expect(registry.get('bg-2')).toBeUndefined(); + expect(registry.get(`bg-${cap + 2}`)).toBeDefined(); + } finally { + vi.useRealTimers(); + } + }); + + it('settleForeground after cancel() attaches final stats and runs prune', () => { + // Cancel-then-settle race: the dialog's `x` confirms cancellation + // (which sets entry.status='cancelled' synchronously), then the + // tool-call's finally calls settleForeground with the agent's + // authoritative final stats. The settle must NOT skip on the + // already-terminal status — otherwise the dialog row would render + // with the live-refresh's last snapshot rather than the final + // execution-summary numbers. + registry.register({ + agentId: 'fg-cancel-race', + description: 'sync agent', + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + stats: { totalTokens: 100, toolUses: 1, durationMs: 50 }, + }); + + registry.cancel('fg-cancel-race'); + // Foreground entries don't notify, but cancel set status='cancelled'. + expect(registry.get('fg-cancel-race')!.status).toBe('cancelled'); + + const finalStats = { totalTokens: 250, toolUses: 3, durationMs: 200 }; + registry.settleForeground('fg-cancel-race', 'cancelled', { + stats: finalStats, + }); + + const settled = registry.get('fg-cancel-race'); + // Status preserved (user intent wins), but final stats attached. + expect(settled!.status).toBe('cancelled'); + expect(settled!.stats).toEqual(finalStats); + }); + + it('does not evict still-running entries', () => { + vi.useFakeTimers(); + try { + const cap = MAX_RETAINED_TERMINAL_BACKGROUND_TASKS; + // A long-running background entry registered at t=0 (oldest of all) + // must survive even when `cap+5` foreground entries settle around it. + vi.setSystemTime(new Date(4_000_000_000_000)); + registry.register({ + agentId: 'bg-running', + description: 'still running', + flavor: 'background', + status: 'running', + startTime: 4_000_000_000_000, + abortController: new AbortController(), + }); + for (let i = 0; i < cap + 5; i++) { + vi.setSystemTime(new Date(4_000_000_000_000 + (i + 1) * 1000)); + registerForeground(`fg-${i}`, 4_000_000_000_000 + (i + 1) * 1000); + registry.settleForeground(`fg-${i}`, 'completed'); + } + + // Running entry stays; total is cap (terminal) + 1 (running). + expect(registry.get('bg-running')).toBeDefined(); + expect(registry.get('bg-running')!.status).toBe('running'); + expect(registry.getAll().length).toBe(cap + 1); + } finally { + vi.useRealTimers(); + } + }); + }); }); diff --git a/packages/core/src/agents/background-tasks.ts b/packages/core/src/agents/background-tasks.ts index 75273eec220..f52b8fcfb4e 100644 --- a/packages/core/src/agents/background-tasks.ts +++ b/packages/core/src/agents/background-tasks.ts @@ -14,10 +14,11 @@ * - `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. + * - `foreground` entries deliver their result through the normal tool-result + * channel (no XML envelope), don't participate in the headless holdback, + * and on tool-call return are settled into a terminal status and retained + * (bounded by `MAX_RETAINED_TERMINAL_BACKGROUND_TASKS`) so the dialog can + * drill into finished foreground agents. */ import { createDebugLogger } from '../utils/debugLogger.js'; @@ -29,6 +30,13 @@ const debugLogger = createDebugLogger('BACKGROUND_TASKS'); const MAX_DESCRIPTION_LENGTH = 40; const MAX_RECENT_ACTIVITIES = 5; +// Mirrors `MonitorRegistry.MAX_RETAINED_TERMINAL_MONITORS`. Foreground +// agents leave their entry in the registry after their tool-call returns +// (in a terminal status) so the dialog can drill into them — but the cap +// keeps memory bounded across long sessions. Eviction is FIFO over all +// terminal entries (foreground + background), oldest endTime first. +export const MAX_RETAINED_TERMINAL_BACKGROUND_TASKS = 128; + // Grace period after cancel() before emitting a fallback cancelled // notification. The natural handler (bgBody) almost always settles and // emits the terminal notification with the real partial result well @@ -81,6 +89,16 @@ export type BackgroundTaskStatus = | 'failed' | 'cancelled'; +const TERMINAL_STATUSES: ReadonlySet = new Set([ + 'completed', + 'failed', + 'cancelled', +]); + +function isTerminalStatus(status: BackgroundTaskStatus): boolean { + return TERMINAL_STATUSES.has(status); +} + export interface AgentCompletionStats { totalTokens: number; toolUses: number; @@ -255,33 +273,73 @@ export class BackgroundTaskRegistry { debugLogger.info(`Background agent completed: ${agentId}`); this.emitNotification(entry); + this.pruneTerminalEntries(); 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. + * Transition a foreground entry to a terminal status and retain it + * (bounded by `MAX_RETAINED_TERMINAL_BACKGROUND_TASKS`). Called by the + * foreground tool-call's `finally` path, which has already delivered + * the result through the tool-result channel — the registry entry now + * sticks around so the dialog can drill into it. Background entries + * must go through complete/fail/finalizeCancelled instead, so this + * throws if asked to settle one. + * + * Cancel-then-settle race: external `cancel()` may have already flipped + * the entry to `'cancelled'` before this finally runs. In that case the + * status stays at the cancel's verdict (user intent wins), but the + * final stats from `getExecutionSummary()` and any error string are + * still attached — the live-refresh path stops once status leaves + * `'running'`, so without this re-attach the dialog would render with + * potentially stale token / tool counts. */ - unregisterForeground(agentId: string): void { + settleForeground( + agentId: string, + status: 'completed' | 'failed' | 'cancelled', + details: { error?: string; stats?: AgentCompletionStats } = {}, + ): void { const entry = this.agents.get(agentId); if (!entry) return; if (entry.flavor !== 'foreground') { throw new Error( - `unregisterForeground called on non-foreground entry ${agentId} ` + + `settleForeground called on non-foreground entry ${agentId} ` + `(flavor=${entry.flavor ?? 'undefined'}). ` + `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. + + // Track mutation so a pure no-op (no status change, no new details) + // skips prune + emit and avoids triggering a redundant UI refresh. + let mutated = false; + + if (entry.status === 'running') { + entry.status = status; + entry.endTime = Date.now(); + mutated = true; + debugLogger.info(`Settled foreground agent: ${agentId} (${status})`); + } + // First-write-wins for error so a prior `cancel()` reason (if any + // future caller sets one) isn't clobbered by a later settle. + if (details.error !== undefined && entry.error === undefined) { + entry.error = details.error; + mutated = true; + } + // Final stats always win — `getExecutionSummary()` from the + // tool-call's finally is authoritative even if `cancel()` raced + // ahead of it. + if (details.stats !== undefined) { + entry.stats = details.stats; + mutated = true; + } + + if (!mutated) return; + + // Prune before emit so subscribers that snapshot `registry.getAll()` + // from inside the callback see the post-prune state. Mirrors + // `MonitorRegistry.settle()`'s order. + this.pruneTerminalEntries(); this.emitStatusChange(entry); - this.agents.delete(agentId); - debugLogger.info(`Unregistered foreground agent: ${agentId}`); } // See complete() for the cancelled → terminal path rationale. @@ -298,6 +356,7 @@ export class BackgroundTaskRegistry { debugLogger.info(`Background agent failed: ${agentId}`); this.emitNotification(entry); + this.pruneTerminalEntries(); this.emitStatusChange(entry); } @@ -359,6 +418,7 @@ export class BackgroundTaskRegistry { entry.endTime = Date.now(); entry.notified = true; debugLogger.info(`Abandoned paused background agent: ${agentId}`); + this.pruneTerminalEntries(); this.emitStatusChange(entry); } @@ -382,6 +442,7 @@ export class BackgroundTaskRegistry { if (partialResult) entry.result = partialResult; entry.stats = stats; this.emitNotification(entry); + this.pruneTerminalEntries(); this.emitStatusChange(entry); } @@ -394,6 +455,7 @@ export class BackgroundTaskRegistry { const entry = this.agents.get(agentId); if (!entry || entry.status !== 'cancelled' || entry.notified) return; this.emitNotification(entry); + this.pruneTerminalEntries(); this.emitStatusChange(entry); } @@ -639,6 +701,33 @@ export class BackgroundTaskRegistry { } } + /** + * Cap retained terminal entries at `MAX_RETAINED_TERMINAL_BACKGROUND_TASKS`, + * FIFO by `endTime`. Background entries are only eligible once + * `notified === true`, so a `cancelled`-but-not-finalized entry + * survives until its terminal task-notification fires (otherwise we'd + * orphan the SDK-contract notification and strand headless callers). + * Foreground entries never emit a notification, so they're eligible + * the moment they reach a terminal status. + */ + private pruneTerminalEntries(): void { + const terminal = Array.from(this.agents.values()) + .filter((e) => { + if (!isTerminalStatus(e.status)) return false; + if (e.flavor !== 'foreground' && !e.notified) return false; + return true; + }) + .sort( + (a, b) => + (a.endTime ?? a.startTime) - (b.endTime ?? b.startTime) || + a.startTime - b.startTime, + ); + while (terminal.length > MAX_RETAINED_TERMINAL_BACKGROUND_TASKS) { + const oldest = terminal.shift(); + if (oldest) this.agents.delete(oldest.agentId); + } + } + private emitActivityChange(entry: BackgroundTaskEntry): void { if (this.activityChangeListeners.size === 0) return; // Snapshot before iterating so a listener that adds or removes diff --git a/packages/core/src/tools/agent/agent.test.ts b/packages/core/src/tools/agent/agent.test.ts index d7b6ec1893a..0a3371a9c36 100644 --- a/packages/core/src/tools/agent/agent.test.ts +++ b/packages/core/src/tools/agent/agent.test.ts @@ -99,7 +99,7 @@ describe('AgentTool', () => { // enough for these tests — they don't assert on registry behavior. const stubRegistry = { register: vi.fn(), - unregisterForeground: vi.fn(), + settleForeground: vi.fn(), complete: vi.fn(), fail: vi.fn(), finalizeCancelled: vi.fn(), @@ -1510,7 +1510,7 @@ describe('AgentTool', () => { let mockContextState: ContextState; let mockRegistry: { register: ReturnType; - unregisterForeground: ReturnType; + settleForeground: ReturnType; complete: ReturnType; fail: ReturnType; finalizeCancelled: ReturnType; @@ -1547,7 +1547,7 @@ describe('AgentTool', () => { mockRegistry = { register: vi.fn(), - unregisterForeground: vi.fn(), + settleForeground: vi.fn(), complete: vi.fn(), fail: vi.fn(), finalizeCancelled: vi.fn(), @@ -1694,9 +1694,9 @@ describe('AgentTool', () => { expect(llmText).not.toContain('Background agent launched'); // 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.) + // the parent's tool-call awaits. After the call returns the entry + // is settled to a terminal status and retained (bounded by the + // registry cap) so the dialog can drill into it. expect(mockRegistry.register).toHaveBeenCalledWith( expect.objectContaining({ flavor: 'foreground', @@ -1705,8 +1705,10 @@ describe('AgentTool', () => { status: 'running', }), ); - expect(mockRegistry.unregisterForeground).toHaveBeenCalledWith( + expect(mockRegistry.settleForeground).toHaveBeenCalledWith( expect.stringContaining('file-search-'), + 'completed', + expect.any(Object), ); }); diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index 1f45a045e75..6d11dae1d86 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -1443,11 +1443,7 @@ class AgentToolInvocation extends BaseToolInvocation { const runFramedFork = () => runWithAgentContext({ agentId: hookOpts.agentId }, async () => { try { - await this.runSubagentWithHooks( - subagent, - contextState, - hookOpts, - ); + await this.runSubagentWithHooks(subagent, contextState, hookOpts); } finally { void agentConfig .getToolRegistry() @@ -1537,11 +1533,20 @@ class AgentToolInvocation extends BaseToolInvocation { this.eventEmitter.on(AgentEventType.TOOL_CALL, onFgToolCall); this.eventEmitter.on(AgentEventType.USAGE_METADATA, onFgUsageMetadata); + // Tracked across try/finally so the finally can settle the registry + // entry with the right terminal status. Defaults to `'failed'` so an + // unexpected throw inside `runFramed` (which lands in the outer + // catch, AFTER the inner finally has already run) still settles the + // entry as failed rather than leaving it stuck on `running`. + let terminalStatus: 'completed' | 'failed' | 'cancelled' = 'failed'; + let terminalError: string | undefined; try { await runFramed(); const finalText = subagent.getFinalText(); const terminateMode = subagent.getTerminateMode(); if (terminateMode === AgentTerminateMode.ERROR) { + terminalStatus = 'failed'; + terminalError = finalText || 'Subagent execution failed.'; return { llmContent: finalText || 'Subagent execution failed.', returnDisplay: this.currentDisplay!, @@ -1557,6 +1562,7 @@ class AgentToolInvocation extends BaseToolInvocation { // `cancelled` XML envelope; the foreground path // has no equivalent envelope, so the marker has to ride the // llmContent payload itself. + terminalStatus = 'cancelled'; const partial = finalText || '(no partial result captured)'; return { llmContent: [ @@ -1567,6 +1573,19 @@ class AgentToolInvocation extends BaseToolInvocation { returnDisplay: this.currentDisplay!, }; } + // Only `GOAL` is a true success. `TIMEOUT`, `MAX_TURNS`, and + // `SHUTDOWN` end execution early without reaching the goal — the + // dialog should surface them as failures so users aren't misled + // by a green ✓ on a run that hit a turn limit or got killed. + // The LLM-facing return shape stays unchanged (today's behavior) + // so this is purely a UI accuracy fix on the registry side. + if (terminateMode === AgentTerminateMode.GOAL) { + terminalStatus = 'completed'; + } else { + terminalStatus = 'failed'; + terminalError = + finalText || `Agent terminated with mode: ${terminateMode}`; + } return { llmContent: [{ text: finalText }], returnDisplay: this.currentDisplay!, @@ -1575,11 +1594,18 @@ class AgentToolInvocation extends BaseToolInvocation { 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 - // this in finally guarantees we clean up on success, failure, - // cancel, AND any unexpected throw inside runFramed. - registry.unregisterForeground(hookOpts.agentId); + // Pass authoritative final stats so the retained dialog row + // doesn't depend on a last-event race against settle — the live + // refresh path stops updating once the entry leaves `running`. + const finalSummary = subagent.getExecutionSummary(); + registry.settleForeground(hookOpts.agentId, terminalStatus, { + error: terminalError, + stats: { + totalTokens: finalSummary.totalTokens, + toolUses: fgLiveToolCallCount, + durationMs: finalSummary.totalDurationMs, + }, + }); // Release the per-subagent ToolRegistry so any AgentTool / // SkillTool the model instantiated during execution disposes // its change-listeners on shared SubagentManager / SkillManager.