feat(core): retain finished foreground agents in BackgroundTaskRegistry - #3911
feat(core): retain finished foreground agents in BackgroundTaskRegistry#3911tanzhenxin wants to merge 1 commit into
Conversation
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.
E2E test reportAll four groups of the PR 2 test plan ran against
Reproduction sketch (Group A — most representative):
Group B reproduction note: the failure path needs a deliberately-failing subagent. The test-engineer used a custom user-level definition with Coverage gaps (covered by unit tests, not by E2E):
Each is exercised by the registry unit tests (63 total: 4 updated foreground tests + 4 new retention tests + the prior 55 covering register / complete / fail / cancel / activity / paths). |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] BackgroundTaskEntry.flavor JSDoc is outdated — still says "unregistered when the tool-call returns" but the PR changes behavior to retain entries after settling. The file header and settleForeground method JSDoc were correctly updated, but the interface-level JSDoc at background-tasks.ts:132 was missed.
[Critical] Missing agent-level integration tests for ERROR/TIMEOUT/MAX_TURNS/SHUTDOWN terminate modes. The new try/finally block in agent.ts sets terminalStatus = 'failed' with terminalError for four terminate modes, but agent.test.ts only covers GOAL and CANCELLED. The settleForeground(id, 'failed', {error: ...}) path has zero assertion coverage at the integration level. Add foreground subagent tests that mock getTerminateMode() returning each of these modes.
— deepseek-v4-pro via Qwen Code /review
| @@ -1567,6 +1573,19 @@ class AgentToolInvocation extends BaseToolInvocation<AgentParams, ToolResult> { | |||
| returnDisplay: this.currentDisplay!, | |||
There was a problem hiding this comment.
[Suggestion] AgentTerminateMode.SHUTDOWN mapped to 'failed' is misleading.
SHUTDOWN is described as "gracefully shut down (e.g., arena/team session ended)" — a normal, expected termination. Mapping it to 'failed' shows a red X in the dialog, misleading users into thinking something went wrong.
| returnDisplay: this.currentDisplay!, | |
| if (terminateMode === AgentTerminateMode.GOAL) { | |
| terminalStatus = 'completed'; | |
| } else if (terminateMode === AgentTerminateMode.SHUTDOWN) { | |
| terminalStatus = 'cancelled'; // graceful external shutdown | |
| } else { | |
| terminalStatus = 'failed'; | |
| terminalError = | |
| finalText || `Agent terminated with mode: ${terminateMode}`; | |
| } |
— deepseek-v4-pro via Qwen Code /review
| case 'agent': { | ||
| const label = buildBackgroundEntryLabel(entry, { includePrefix: false }); | ||
| return entry.flavor === 'foreground' | ||
| const isLive = entry.status === 'running' || entry.status === 'paused'; |
There was a problem hiding this comment.
[Suggestion] Variable name isLive is ambiguous.
The core BackgroundTaskRegistry consistently uses "terminal" / "non-terminal" terminology. isLive has no precedent in the codebase and could be misread as "row is currently rendered in the dialog" rather than "execution is active". isActive or isNonTerminal would be clearer.
| const isLive = entry.status === 'running' || entry.status === 'paused'; | |
| const isActive = entry.status === 'running' || entry.status === 'paused'; | |
| return entry.flavor === 'foreground' && isActive |
— deepseek-v4-pro via Qwen Code /review
| // 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 |
There was a problem hiding this comment.
[Suggestion] String literal 'Subagent execution failed.' is duplicated in the same try block.
It appears once for terminalError and once for llmContent fallback. If someone updates one but not the other, they'll diverge.
| // catch, AFTER the inner finally has already run) still settles the | |
| const FAILED_DEFAULT = 'Subagent execution failed.'; | |
| if (terminateMode === AgentTerminateMode.ERROR) { | |
| terminalStatus = 'failed'; | |
| terminalError = finalText || FAILED_DEFAULT; | |
| return { | |
| llmContent: finalText || FAILED_DEFAULT, |
— deepseek-v4-pro via Qwen Code /review
| a.startTime - b.startTime, | ||
| ); | ||
| while (terminal.length > MAX_RETAINED_TERMINAL_BACKGROUND_TASKS) { | ||
| const oldest = terminal.shift(); |
There was a problem hiding this comment.
[Suggestion] pruneTerminalEntries() evicts entries silently — no debug log.
When a terminal entry is evicted by the 128-entry cap, there's no debugLogger output. Troubleshooting "why did my agent disappear from the dialog?" becomes a guessing game between "pruned", "never registered", and "bug".
| const oldest = terminal.shift(); | |
| while (terminal.length > MAX_RETAINED_TERMINAL_BACKGROUND_TASKS) { | |
| const oldest = terminal.shift(); | |
| if (oldest) { | |
| debugLogger.info( | |
| `Pruning terminal entry: ${oldest.agentId} (status=${oldest.status})`, | |
| ); | |
| this.agents.delete(oldest.agentId); | |
| } | |
| } |
— deepseek-v4-pro via Qwen Code /review
| // skips prune + emit and avoids triggering a redundant UI refresh. | ||
| let mutated = false; | ||
|
|
||
| if (entry.status === 'running') { |
There was a problem hiding this comment.
[Suggestion] settleForeground guards on entry.status === 'running' rather than !isTerminalStatus(entry.status).
If a future non-terminal status (e.g., 'pausing') is added to BackgroundTaskStatus, settleForeground would silently skip the transition, leaving the entry stuck indefinitely. Either switch to !isTerminalStatus() or add a comment documenting this constraining assumption.
— deepseek-v4-pro via Qwen Code /review
| 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 |
There was a problem hiding this comment.
[Suggestion] terminalError stays undefined when runFramed() throws an unexpected exception.
The outer catch has the actual error message, but runs AFTER the inner finally has already called settleForeground with error: undefined. The dialog shows a failed entry with no error reason. Consider capturing the error via a closure variable that the finally block reads.
| // Tracked across try/finally so the finally can settle the registry | |
| let settleError: string | undefined; | |
| try { | |
| await runFramed(); | |
| // ... existing code ... | |
| } finally { | |
| // ... | |
| registry.settleForeground(hookOpts.agentId, terminalStatus, { | |
| error: terminalError ?? settleError, | |
| }); | |
| } | |
| } catch (error) { | |
| settleError = error instanceof Error ? error.message : String(error); |
— deepseek-v4-pro via Qwen Code /review
| }), | ||
| ); | ||
| expect(mockRegistry.unregisterForeground).toHaveBeenCalledWith( | ||
| expect(mockRegistry.settleForeground).toHaveBeenCalledWith( |
There was a problem hiding this comment.
[Suggestion] GOAL test uses expect.any(Object) for settleForeground details — no concrete stats assertion.
The details object with {totalTokens, toolUses, durationMs} is never verified. If getExecutionSummary() returns the wrong shape or fgLiveToolCallCount is stale, no test catches it.
| expect(mockRegistry.settleForeground).toHaveBeenCalledWith( | |
| expect(mockRegistry.settleForeground).toHaveBeenCalledWith( | |
| expect.stringContaining('file-search-'), | |
| 'completed', | |
| expect.objectContaining({ | |
| error: undefined, | |
| stats: expect.objectContaining({ | |
| totalTokens: expect.any(Number), | |
| toolUses: expect.any(Number), | |
| durationMs: expect.any(Number), | |
| }), | |
| }), | |
| ); |
— deepseek-v4-pro via Qwen Code /review
|
Closing — not pursuing this approach. |
2 participants
Summary
completed/failed/cancelled) and bounded by a 128-entry cap. The dialog row drops the[in turn]warning prefix once the entry is settled. OnlyGOALterminations settle as completed;TIMEOUT,MAX_TURNS,SHUTDOWN, andERRORall settle as failed and surface the reason in the dialog detail view.[in turn] ...with no terminal icon, and the pill stays at1 local agentfor the rest of the session. This refactor fixes both issues with the same change. PR 3 of the umbrella refactor depends on this retention to drill into finished foreground agents from the committed scrollback path.cancel()call lands first, then the tool-call'sfinallydelivers the authoritative final stats — the settle path needs to attach those stats without overwriting the cancel verdict); the cancelled-not-notified guard in the prune (a background entry that's been cancelled but hasn't yet emitted its terminal task-notification must not be evicted, otherwise the SDK contract drops a notification and the headless wait loop strands); and the cap-enforcement triggering surface (prune now fires from every terminal-transition path, not just foreground settles, so a background-only session also stays bounded).Validation
max_turns: 1agent to reliably hitMAX_TURNS); standard "summarize README" prompts for the happy-path E2E.MAX_TURNS), C (pill transitions), D (drill-in detail) all pass against the local bundle. A separate E2E test summary will follow as a comment.node dist/cli.js, ask for a one-shot subagent summary of any file, then press Down → Enter to open the background-tasks dialog. Expect to see the agent listed with a terminal check icon (no[in turn]prefix) and the footer pill reading1 task done. Press Enter on the row to confirm the detail view shows verb + stats + the activity log + the prompt.Scope / Risk
endTime. Mirrors the existing monitor-registry behavior, and the umbrella registry-unification work (feat(cli): background-agent UI — pill, combined dialog, detail view #3488) will revisit it. The[in turn]prefix removal on settled rows is a UX change — reviewers familiar with the prior look should expect settled foreground rows to read identically to settled background rows now.unregisterForegroundmethod has been renamed tosettleForeground(id, status, details)with a different signature. Internal-only — the only call site is the agent tool's foreground finally path, which has been updated. No external callers in this repo or the SDK surface.Testing Matrix
MAX_TURNStriggers → dialog + error reason)