From d5a2c1367e57760e0c2c57a1e5b17b4157ce3d7f Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 7 May 2026 21:47:51 +0800 Subject: [PATCH 1/9] fix(cli,core): isPending gate on subagent scrollback summary + post-delete statusChange emit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from PR #3909 review. 1. **Re-introduce `isPending` gate on `SubagentExecutionRenderer`'s scrollback summary** (Copilot finding on PRRT_kwDOPB-92c6AUQHn). The verbose inline frame retirement collapsed `SubagentExecutionRenderer` to "render the summary whenever a subagent reaches a terminal status" — but with `isPending` removed in #3909, that fired in BOTH live (pendingHistoryItems) AND committed (Static) phases. Live-phase rendering duplicated the row LiveAgentPanel already paints below the composer until the parent turn committed. Add `isPending` back to `ToolMessageProps` purely as a gate for this one render path: the summary fires only when `!isPending` (committed). `ToolGroupMessage` forwards the flag (it kept the prop on its own interface for upstream compat the whole time). Test gap closed by the new `live (isPending) terminal subagent → no scrollback summary (panel owns the row)` case. 2. **Emit `statusChange` AFTER delete in `unregisterForeground`** (Copilot finding on PRRT_kwDOPB-92c6AUQGc + the panel-only reconciliation it spawned). The shared snapshot in `useBackgroundTaskView` only refreshes on `statusChange`, and `unregisterForeground` previously fired exactly once — BEFORE delete — so the snapshot froze with the agent as "running" while `registry.get()` returned undefined. Result: `BackgroundTasksDialog` list mode showed a ghost "running" row with cancel hints whose `x` was a no-op, contradicting what the panel already showed (synthesized neutral terminal). Fire `statusChange` a second time AFTER `agents.delete()` so snapshot consumers see the registry-less state and stop surfacing the agent. The first emit still mirrors complete/fail/cancel/finalize ordering (callbacks that re-read `registry.get` see the entry); the second emit is the new contract for snapshot-based views. React batches the two resulting setState calls into one re-render so consumers re-render exactly once. Updated the existing "emits status change before removing the entry" test to capture both emits and explicitly assert that the second observes the registry-less state. Added a sibling test covering the post-delete `getAll()` count. Coverage: 190 passing tests across core + cli (background-view + ToolMessage + ToolGroupMessage + useBackgroundTaskView). --- .../components/messages/ToolGroupMessage.tsx | 6 +- .../components/messages/ToolMessage.test.tsx | 32 +++++++- .../ui/components/messages/ToolMessage.tsx | 39 +++++++++- .../core/src/agents/background-tasks.test.ts | 78 ++++++++++++++++--- packages/core/src/agents/background-tasks.ts | 17 +++- 5 files changed, 150 insertions(+), 22 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index b3562623cb8..a42e0e67842 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -78,10 +78,7 @@ export const ToolGroupMessage: React.FC = ({ availableTerminalHeight, contentWidth, isFocused = true, - // `isPending` stays on the props interface for upstream compat - // (HistoryItemDisplay et al. forward it) but the group body no - // longer reads it. Skip the destructure so TS catches accidental - // re-introductions of dead state. + isPending = false, activeShellPtyId, embeddedShellFocused, memoryWriteCount, @@ -327,6 +324,7 @@ export const ToolGroupMessage: React.FC = ({ isAgentWithPendingConfirmation(tool.resultDisplay) } isFocused={isSubagentFocused} + isPending={isPending} /> {tool.status === ToolCallStatus.Confirming && diff --git a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx index 403983f34a3..4af205d5019 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx @@ -317,6 +317,7 @@ describe('', () => { terminateReason?: string; }; isFocused?: boolean; + isPending?: boolean; }): ToolMessageProps => { const resultDisplay = { type: 'task_execution' as const, @@ -331,6 +332,7 @@ describe('', () => { callId: 'gated-task-call', forceShowResult: true, // mirror ToolGroupMessage's forceShowResult isFocused: overrides.isFocused, + isPending: overrides.isPending, }; }; @@ -355,7 +357,7 @@ describe('', () => { expect(output).not.toContain('Queued approval:'); }); - it('completed subagent → renders a one-line scrollback summary', () => { + it('committed (`!isPending`) terminal subagent → renders a one-line scrollback summary', () => { // The verbose 15-row inline frame is retired (it caused // scrollback flicker), but the conversation history needs to // keep a permanent record after the panel's 8s window expires @@ -370,6 +372,7 @@ describe('', () => { taskPrompt: 'Already done', status: 'completed', }, + isPending: false, })} />, StreamingState.Idle, @@ -384,6 +387,33 @@ describe('', () => { expect(output).not.toContain('MockApprovalPrompt'); }); + it('live (`isPending`) terminal subagent → no scrollback summary (panel owns the row)', () => { + // While the parent turn is still in `pendingHistoryItems`, + // LiveAgentPanel below the composer renders the synthesized / + // terminal row — emitting the summary here too would duplicate + // it visually. The summary fires only when the tool message + // commits to (`isPending=false`), guaranteeing exactly + // one inline copy in the eventual scrollback record. + const { lastFrame } = renderWithContext( + , + StreamingState.Responding, + ); + const output = lastFrame() ?? ''; + // No inline summary — the panel renders this row instead. + expect(output).not.toContain('✔'); + expect(output).not.toContain('Just finished mid-turn'); + }); + it('failed subagent → renders summary with terminate reason', () => { const { lastFrame } = renderWithContext( : · N tools · Xs · Yk tokens`. + * + * The committed-phase summary is gated on `!isPending` — without the + * gate the summary would also paint into `pendingHistoryItems` + * (the live area) the moment a subagent terminates, duplicating the + * row LiveAgentPanel already renders synthesized below the composer. + * `isPending=true` means we're still in the live area; `isPending=false` + * means the item has committed to `` and the summary is the + * one and only place the user can find this run after the panel evicts. */ const SubagentExecutionRenderer: React.FC<{ data: AgentResultDisplay; @@ -269,7 +277,15 @@ const SubagentExecutionRenderer: React.FC<{ childWidth: number; config: Config; isFocused?: boolean; -}> = ({ data, availableHeight, childWidth, config, isFocused }) => { + isPending?: boolean; +}> = ({ + data, + availableHeight, + childWidth, + config, + isFocused, + isPending = false, +}) => { if (data.pendingConfirmation && isFocused) { const agentLabel = data.subagentName || 'agent'; return ( @@ -308,10 +324,14 @@ const SubagentExecutionRenderer: React.FC<{ // 8s visibility window expires (LiveAgentPanel evicts terminal rows; // BackgroundTasksDialog only retains them while open). Skip // `running` / `background` since the panel + dialog cover those. + // ALSO skip while `isPending` — the live area is already painted by + // the panel, so emitting the summary into `pendingHistoryItems` would + // duplicate the row visually until the parent turn commits. if ( - data.status === 'completed' || - data.status === 'failed' || - data.status === 'cancelled' + !isPending && + (data.status === 'completed' || + data.status === 'failed' || + data.status === 'cancelled') ) { return ; } @@ -478,6 +498,15 @@ export interface ToolMessageProps extends IndividualToolCallDisplay { * sibling subagents render a dim "Queued approval" marker instead. */ isFocused?: boolean; + /** + * True while the tool message is rendered inside `pendingHistoryItems` + * (live area), false once committed to ``. Used by the + * subagent renderer to gate the scrollback summary so the live area + * stays panel-only — without this gate the summary would duplicate + * the synthesized row LiveAgentPanel already renders below the + * composer the moment a subagent terminates. + */ + isPending?: boolean; } export const ToolMessage: React.FC = ({ @@ -495,6 +524,7 @@ export const ToolMessage: React.FC = ({ config, forceShowResult, isFocused, + isPending, executionStartTime, }) => { const settings = useSettings(); @@ -649,6 +679,7 @@ export const ToolMessage: React.FC = ({ childWidth={innerWidth} config={config} isFocused={isFocused} + isPending={isPending} /> )} {effectiveDisplayRenderer.type === 'diff' && ( diff --git a/packages/core/src/agents/background-tasks.test.ts b/packages/core/src/agents/background-tasks.test.ts index 3c781ec2836..08fe4f4ae1e 100644 --- a/packages/core/src/agents/background-tasks.test.ts +++ b/packages/core/src/agents/background-tasks.test.ts @@ -999,7 +999,7 @@ describe('BackgroundTaskRegistry', () => { } }); - it('unregisterForeground removes the entry and emits a status change', () => { + it('unregisterForeground removes the entry and emits status change twice (pre + post delete)', () => { const onStatusChange = vi.fn(); registry.setStatusChangeCallback(onStatusChange); @@ -1016,7 +1016,56 @@ describe('BackgroundTaskRegistry', () => { registry.unregisterForeground('fg-5'); expect(registry.get('fg-5')).toBeUndefined(); - expect(onStatusChange).toHaveBeenCalledTimes(1); + // Two emits: the pre-delete one matches the + // complete/fail/cancel/finalize ordering so callbacks that + // re-read `registry.get(agentId)` see the entry; the post-delete + // one lets snapshot-based views (`useBackgroundTaskView` calling + // `registry.getAll()`) observe the entry-less state without + // needing per-tick reconciliation. Both fire synchronously, so + // React batches the resulting setState calls into one re-render. + expect(onStatusChange).toHaveBeenCalledTimes(2); + }); + + it('unregisterForeground post-delete emit observes registry-less state', () => { + // The second emit is the contract that lets snapshot consumers + // (e.g. BackgroundTasksDialog list mode) drop ghost rows for + // foreground subagents that just unregistered. Without it the + // snapshot would freeze at the pre-delete entry-as-running state + // until the next unrelated transition. + registry.register({ + agentId: 'fg-post-delete', + description: 'sync agent', + flavor: 'foreground', + status: 'running', + startTime: Date.now(), + abortController: new AbortController(), + }); + + const observations: Array<{ + getAtCallback: BackgroundTaskEntry | undefined; + getAllCount: number; + }> = []; + registry.setStatusChangeCallback((entry) => { + if (entry?.agentId !== 'fg-post-delete') return; + observations.push({ + getAtCallback: registry.get('fg-post-delete'), + getAllCount: registry + .getAll() + .filter((e) => e.agentId === 'fg-post-delete').length, + }); + }); + + registry.unregisterForeground('fg-post-delete'); + + expect(observations).toHaveLength(2); + // First emit (pre-delete): entry still in the registry. + expect(observations[0].getAtCallback?.agentId).toBe('fg-post-delete'); + expect(observations[0].getAllCount).toBe(1); + // Second emit (post-delete): entry gone. This is what + // `useBackgroundTaskView.refresh()` sees, so its merged + // entries no longer include the agent. + expect(observations[1].getAtCallback).toBeUndefined(); + expect(observations[1].getAllCount).toBe(0); }); it('unregisterForeground throws if asked to remove a background entry', () => { @@ -1078,10 +1127,15 @@ 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('unregisterForeground first emit sees entry; later emits see registry-less state', () => { + // The first emit 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. The second emit (added + // for snapshot consumers like `useBackgroundTaskView`) sees the + // registry-less state — both paths are documented + tested + // separately so a future refactor that collapsed the two emits + // would surface here. registry.register({ agentId: 'fg-unregister-order', description: 'sync agent', @@ -1091,17 +1145,21 @@ describe('BackgroundTaskRegistry', () => { abortController: new AbortController(), }); - let observedFromCallback: BackgroundTaskEntry | undefined; + const observed: Array = []; registry.setStatusChangeCallback((entry) => { if (entry?.agentId === 'fg-unregister-order') { - observedFromCallback = registry.get(entry.agentId); + observed.push(registry.get(entry.agentId)); } }); registry.unregisterForeground('fg-unregister-order'); - expect(observedFromCallback).toBeDefined(); - expect(observedFromCallback!.agentId).toBe('fg-unregister-order'); + expect(observed).toHaveLength(2); + // First emit: pre-delete, callback sees the entry. + expect(observed[0]).toBeDefined(); + expect(observed[0]!.agentId).toBe('fg-unregister-order'); + // Second emit: post-delete, callback sees the registry-less state. + expect(observed[1]).toBeUndefined(); expect(registry.get('fg-unregister-order')).toBeUndefined(); }); diff --git a/packages/core/src/agents/background-tasks.ts b/packages/core/src/agents/background-tasks.ts index 8ef752f7013..4950a544ddc 100644 --- a/packages/core/src/agents/background-tasks.ts +++ b/packages/core/src/agents/background-tasks.ts @@ -275,11 +275,22 @@ 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. + // Emit BEFORE delete so callbacks that re-read `registry.get(agentId)` + // from inside see the entry — matches the ordering used by + // complete/fail/cancel/finalize. This is the "agent reached its + // last live state" signal. this.emitStatusChange(entry); this.agents.delete(agentId); + // Emit AGAIN after delete so views that re-snapshot the registry + // on statusChange (e.g. `useBackgroundTaskView.refresh()` calling + // `registry.getAll()`) see the entry-less state and stop showing + // a stale "running" row. Without this second emit the snapshot + // freezes at the pre-delete state until the next unrelated + // statusChange event, leaving ghost rows in `BackgroundTasksDialog` + // list mode and forcing each consumer to re-implement registry + // re-pull. The two emits are batched by React's setState batching, + // so consumers only re-render once. + this.emitStatusChange(entry); debugLogger.info(`Unregistered foreground agent: ${agentId}`); } From 5f8515f612cf982f916fa4deeed2b3157abf8784 Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 7 May 2026 22:49:03 +0800 Subject: [PATCH 2/9] fix(cli,core): compact-mode terminal subagent expansion + statusChange context flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five review findings on PR #3919: 1. **Compact mode bypassed the scrollback summary** (gpt-5.5 via /qreview, ToolGroupMessage:324). `ToolGroupMessage` returns `CompactToolGroupDisplay` before the ToolMessage path when `compactMode === true`, so the new `isPending` gate on `SubagentExecutionRenderer` only protected the expanded path — committed terminal subagents in compact mode never reached `SubagentScrollbackSummary` and the LiveAgentPanel → committed- summary handoff broke for users who turned compact mode on. Force-expand the group when `!isPending` AND any tool call has a terminal `task_execution` resultDisplay. Stay compact while the parent turn is still live (`isPending`) — the panel below the composer owns that surface and an inline summary would duplicate it. Coverage: 4 new ToolGroupMessage cases (compact + completed-committed expands; compact + running-live stays compact; compact + completed-live stays compact; compact + failed-committed expands). 2. **Snapshot-coupled comment in `packages/core`** (Copilot, background-tasks.ts:292). The comment named CLI/UI consumers (`useBackgroundTaskView`, `BackgroundTasksDialog`) and asserted React batching guarantees from a core file. Reword to "snapshot-style consumers that re-pull `getAll()` from inside the callback" and drop the framework-specific batching claim. 3. **Two-phase emit needed an explicit signal** (Copilot, background-tasks.ts:283). Emitting `statusChange` twice without distinguishing the phases forced consumers to either do duplicate work or risk persisting a stale `entry` from the second callback. Add an optional second arg `context?: { removed?: boolean }` to `BackgroundStatusChangeCallback`; the post-delete emit passes `{ removed: true }` so consumers can disambiguate without re-querying the registry. Backwards compatible — existing callbacks ignore the new arg. Tests updated to assert both `mock.calls[0][1] === undefined` and `mock.calls[1][1] === { removed: true }`. 4. **`isPending` doc clarified** (Copilot, ToolMessage.tsx:507). Made the default semantics explicit: omitted/undefined is treated as committed (not pending); live-area renderers MUST pass `true` explicitly to suppress the scrollback summary. 5. (4 of the threads were duplicate Copilot fires of #2 + #3.) Coverage: 219 test files / 3369 passing across cli/ui + core/agents. --- .../messages/ToolGroupMessage.test.tsx | 104 ++++++++++++++++++ .../components/messages/ToolGroupMessage.tsx | 24 +++- .../ui/components/messages/ToolMessage.tsx | 12 +- .../core/src/agents/background-tasks.test.ts | 71 ++++++++---- packages/core/src/agents/background-tasks.ts | 48 +++++--- 5 files changed, 214 insertions(+), 45 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx index 9610f277103..83f6ee63899 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx @@ -18,6 +18,7 @@ import type { } from '@qwen-code/qwen-code-core'; import { TOOL_STATUS } from '../../constants.js'; import { ConfigContext } from '../../contexts/ConfigContext.js'; +import { CompactModeProvider } from '../../contexts/CompactModeContext.js'; // Mock child components to isolate ToolGroupMessage behavior vi.mock('./ToolMessage.js', () => ({ @@ -566,4 +567,107 @@ describe('', () => { expect(lastFrame()).toMatchSnapshot(); }); }); + + describe('Compact mode + terminal subagent expansion', () => { + // Helper that wraps the group with `compactMode: true` so the + // `showCompact` branch is exercised. Verifies the safety net that + // forces the group to expand when it carries a committed terminal + // subagent — without it, `CompactToolGroupDisplay` would skip the + // ToolMessage path and `SubagentScrollbackSummary` would never + // surface in scrollback. The committed-summary handoff promised + // by the LiveAgentPanel design depends on this. + const renderCompact = (component: React.ReactElement, compactMode = true) => + render( + + + {component} + + , + ); + + const subagentCall = ( + status: 'running' | 'completed' | 'failed' | 'cancelled', + ): IndividualToolCallDisplay => + createToolCall({ + callId: `task-${status}`, + name: 'task', + description: 'Delegate task to subagent', + status: + status === 'running' + ? ToolCallStatus.Executing + : status === 'completed' + ? ToolCallStatus.Success + : ToolCallStatus.Error, + resultDisplay: { + type: 'task_execution', + subagentName: 'researcher', + taskDescription: 'investigate the change', + taskPrompt: 'investigate', + status, + } as AgentResultDisplay, + }); + + it('compact mode: committed group with completed subagent forces expand', () => { + // isPending=false (committed) + completed subagent → expand, + // routing through ToolMessage so the scrollback summary lands + // in the persistent record. + const { lastFrame } = renderCompact( + , + ); + const frame = lastFrame() ?? ''; + // The MockToolMessage's `MockSubagent[task-completed]` sentinel + // proves we routed through the expanded path; absence would + // mean CompactToolGroupDisplay swallowed the call. + expect(frame).toContain('MockSubagent[task-completed]'); + }); + + it('compact mode: live group with running subagent stays compact', () => { + // isPending=true (live) → panel below the composer owns the + // row; staying compact keeps scrollback quiet until the parent + // turn commits. + const { lastFrame } = renderCompact( + , + ); + // Compact path renders the group header / count, NOT the + // expanded MockToolMessage sentinel. + expect(lastFrame() ?? '').not.toContain('MockSubagent[task-running]'); + }); + + it('compact mode: live group with completed subagent stays compact', () => { + // The subagent terminated mid-turn but the parent is still + // running. Panel shows the synthesized / terminal row; the + // group should NOT yet expand to render the scrollback summary + // (that happens when the parent commits, isPending=false). + const { lastFrame } = renderCompact( + , + ); + expect(lastFrame() ?? '').not.toContain('MockSubagent[task-completed]'); + }); + + it('compact mode: committed group with failed subagent forces expand', () => { + // Same as the completed case — the scrollback summary needs to + // land for failed / cancelled foreground subagents too so the + // user has a permanent record of the run's outcome. + const { lastFrame } = renderCompact( + , + ); + expect(lastFrame() ?? '').toContain('MockSubagent[task-failed]'); + }); + }); }); diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index a42e0e67842..c8dd4245da3 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -164,15 +164,35 @@ export const ToolGroupMessage: React.FC = ({ // Compact mode: entire group → single line summary // Force-expand when: user must interact (Confirming or subagent pending - // confirmation), tool errored, shell is focused, or user-initiated + // confirmation), tool errored, shell is focused, or user-initiated. + // Also force-expand when this group has just committed and contains + // a terminal subagent — `CompactToolGroupDisplay` doesn't know about + // `task_execution` results, so the compact path would skip + // `SubagentScrollbackSummary` and the user would lose the persistent + // record promised by the LiveAgentPanel → committed-summary handoff. + // Stay compact while the parent turn is still live (`isPending`), + // because the panel below the composer is still showing the row. const hasSubagentPendingConfirmation = subagentsAwaitingApproval.length > 0; + const hasCommittedTerminalSubagent = + !isPending && + toolCalls.some((t) => { + const display = t.resultDisplay; + if (!display || typeof display !== 'object') return false; + if (!('type' in display) || display.type !== 'task_execution') + return false; + const status = (display as { status?: string }).status; + return ( + status === 'completed' || status === 'failed' || status === 'cancelled' + ); + }); const showCompact = compactMode && !hasConfirmingTool && !hasSubagentPendingConfirmation && !hasErrorTool && !isEmbeddedShellFocused && - !isUserInitiated; + !isUserInitiated && + !hasCommittedTerminalSubagent; if (showCompact) { return ( diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index 19993027e12..03aea91b9b3 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -500,11 +500,13 @@ export interface ToolMessageProps extends IndividualToolCallDisplay { isFocused?: boolean; /** * True while the tool message is rendered inside `pendingHistoryItems` - * (live area), false once committed to ``. Used by the - * subagent renderer to gate the scrollback summary so the live area - * stays panel-only — without this gate the summary would duplicate - * the synthesized row LiveAgentPanel already renders below the - * composer the moment a subagent terminates. + * (live area), false (or omitted — undefined is treated as false) + * once committed to ``. Live-area renderers MUST pass + * `true` explicitly; committed renderers can omit it. Used by the + * subagent renderer to gate the scrollback summary so the live + * area stays panel-only — without this gate the summary would + * duplicate the synthesized row LiveAgentPanel already renders + * below the composer the moment a subagent terminates. */ isPending?: boolean; } diff --git a/packages/core/src/agents/background-tasks.test.ts b/packages/core/src/agents/background-tasks.test.ts index 08fe4f4ae1e..e4bcec0fa03 100644 --- a/packages/core/src/agents/background-tasks.test.ts +++ b/packages/core/src/agents/background-tasks.test.ts @@ -999,7 +999,7 @@ describe('BackgroundTaskRegistry', () => { } }); - it('unregisterForeground removes the entry and emits status change twice (pre + post delete)', () => { + it('unregisterForeground emits status change twice with removed flag on second emit', () => { const onStatusChange = vi.fn(); registry.setStatusChangeCallback(onStatusChange); @@ -1016,14 +1016,25 @@ describe('BackgroundTaskRegistry', () => { registry.unregisterForeground('fg-5'); expect(registry.get('fg-5')).toBeUndefined(); - // Two emits: the pre-delete one matches the - // complete/fail/cancel/finalize ordering so callbacks that - // re-read `registry.get(agentId)` see the entry; the post-delete - // one lets snapshot-based views (`useBackgroundTaskView` calling - // `registry.getAll()`) observe the entry-less state without - // needing per-tick reconciliation. Both fire synchronously, so - // React batches the resulting setState calls into one re-render. + // Two emits: the first omits `removed` (entry still present in + // the registry — matches complete/fail/cancel/finalize); the + // second sets `removed: true` (entry gone). Snapshot-style + // consumers can ignore the first; consumers that key off the + // entry's last live state can ignore the second. Both fire + // synchronously so a typical React-based consumer's setState + // calls collapse to a single re-render. expect(onStatusChange).toHaveBeenCalledTimes(2); + // First call: pre-delete, no context flag (context arg is + // undefined since `emitStatusChange(entry)` doesn't pass one). + expect(onStatusChange.mock.calls[0][0]).toMatchObject({ + agentId: 'fg-5', + }); + expect(onStatusChange.mock.calls[0][1]).toBeUndefined(); + // Second call: post-delete, `removed: true` flag set. + expect(onStatusChange.mock.calls[1][0]).toMatchObject({ + agentId: 'fg-5', + }); + expect(onStatusChange.mock.calls[1][1]).toEqual({ removed: true }); }); it('unregisterForeground post-delete emit observes registry-less state', () => { @@ -1042,12 +1053,14 @@ describe('BackgroundTaskRegistry', () => { }); const observations: Array<{ + removed: boolean; getAtCallback: BackgroundTaskEntry | undefined; getAllCount: number; }> = []; - registry.setStatusChangeCallback((entry) => { + registry.setStatusChangeCallback((entry, context) => { if (entry?.agentId !== 'fg-post-delete') return; observations.push({ + removed: context?.removed ?? false, getAtCallback: registry.get('fg-post-delete'), getAllCount: registry .getAll() @@ -1058,12 +1071,15 @@ describe('BackgroundTaskRegistry', () => { registry.unregisterForeground('fg-post-delete'); expect(observations).toHaveLength(2); - // First emit (pre-delete): entry still in the registry. + // First emit (pre-delete): entry still in the registry, no + // `removed` flag → callbacks that re-read see the entry. + expect(observations[0].removed).toBe(false); expect(observations[0].getAtCallback?.agentId).toBe('fg-post-delete'); expect(observations[0].getAllCount).toBe(1); - // Second emit (post-delete): entry gone. This is what - // `useBackgroundTaskView.refresh()` sees, so its merged - // entries no longer include the agent. + // Second emit (post-delete): `removed: true` flag, entry gone. + // This is what `useBackgroundTaskView.refresh()` sees, so its + // merged entries no longer include the agent. + expect(observations[1].removed).toBe(true); expect(observations[1].getAtCallback).toBeUndefined(); expect(observations[1].getAllCount).toBe(0); }); @@ -1135,7 +1151,9 @@ describe('BackgroundTaskRegistry', () => { // for snapshot consumers like `useBackgroundTaskView`) sees the // registry-less state — both paths are documented + tested // separately so a future refactor that collapsed the two emits - // would surface here. + // would surface here. Consumers that need to distinguish the + // two emits without re-querying the registry can read + // `context.removed`. registry.register({ agentId: 'fg-unregister-order', description: 'sync agent', @@ -1145,21 +1163,30 @@ describe('BackgroundTaskRegistry', () => { abortController: new AbortController(), }); - const observed: Array = []; - registry.setStatusChangeCallback((entry) => { + const observed: Array<{ + live: BackgroundTaskEntry | undefined; + removed: boolean; + }> = []; + registry.setStatusChangeCallback((entry, context) => { if (entry?.agentId === 'fg-unregister-order') { - observed.push(registry.get(entry.agentId)); + observed.push({ + live: registry.get(entry.agentId), + removed: context?.removed ?? false, + }); } }); registry.unregisterForeground('fg-unregister-order'); expect(observed).toHaveLength(2); - // First emit: pre-delete, callback sees the entry. - expect(observed[0]).toBeDefined(); - expect(observed[0]!.agentId).toBe('fg-unregister-order'); - // Second emit: post-delete, callback sees the registry-less state. - expect(observed[1]).toBeUndefined(); + // First emit: pre-delete, callback sees the entry, `removed` absent. + expect(observed[0].live).toBeDefined(); + expect(observed[0].live!.agentId).toBe('fg-unregister-order'); + expect(observed[0].removed).toBe(false); + // Second emit: post-delete, callback sees the registry-less + // state, `removed: true` flag set. + expect(observed[1].live).toBeUndefined(); + expect(observed[1].removed).toBe(true); expect(registry.get('fg-unregister-order')).toBeUndefined(); }); diff --git a/packages/core/src/agents/background-tasks.ts b/packages/core/src/agents/background-tasks.ts index 4950a544ddc..470e9d36bf0 100644 --- a/packages/core/src/agents/background-tasks.ts +++ b/packages/core/src/agents/background-tasks.ts @@ -188,13 +188,26 @@ interface BackgroundTaskCancelOptions { } /** - * Fires on entry status transitions — register, complete, fail, cancel. - * Intentionally does NOT fire on `appendActivity` so consumers that only - * care about the pill / roster (Footer, AppContainer) don't re-render - * on every tool call a background agent makes. + * Fires on entry status transitions — register, complete, fail, cancel, + * unregister. Intentionally does NOT fire on `appendActivity` so + * consumers that only care about the roster don't re-render on every + * tool call a background agent makes. + * + * `entry` reflects the registry's view at the moment of emission. + * `unregisterForeground` fires twice for a single logical removal: + * - first emit: `removed === false` (or absent), entry still in the + * registry. Mirrors the complete/fail/cancel/finalize ordering so + * callbacks that re-read `registry.get(entry.agentId)` from inside + * the callback see the entry across every terminal path. + * - second emit: `removed === true`, the registry has already + * deleted the entry. Snapshot-style consumers (those that + * re-pull `getAll()` from inside the callback) need this signal + * to drop the row; consumers that only care about the entry's + * last live state can ignore it. */ export type BackgroundStatusChangeCallback = ( entry?: BackgroundTaskEntry, + context?: { removed?: boolean }, ) => void; /** Fires on `appendActivity` — scoped to detail-view consumers. */ @@ -278,19 +291,19 @@ export class BackgroundTaskRegistry { // Emit BEFORE delete so callbacks that re-read `registry.get(agentId)` // from inside see the entry — matches the ordering used by // complete/fail/cancel/finalize. This is the "agent reached its - // last live state" signal. + // last live state" signal; the entry is still present in the + // registry, so `removed` is omitted (treated as false). this.emitStatusChange(entry); this.agents.delete(agentId); - // Emit AGAIN after delete so views that re-snapshot the registry - // on statusChange (e.g. `useBackgroundTaskView.refresh()` calling - // `registry.getAll()`) see the entry-less state and stop showing - // a stale "running" row. Without this second emit the snapshot + // Emit AGAIN after delete with `removed: true` so snapshot-style + // consumers — those that re-pull `getAll()` from inside the + // callback — see the entry-less state and stop surfacing the + // row. Consumers that only care about the entry's last live + // state can ignore the second emit (or check `context.removed` + // and skip duplicate work). Without this signal a snapshot // freezes at the pre-delete state until the next unrelated - // statusChange event, leaving ghost rows in `BackgroundTasksDialog` - // list mode and forcing each consumer to re-implement registry - // re-pull. The two emits are batched by React's setState batching, - // so consumers only re-render once. - this.emitStatusChange(entry); + // status transition. + this.emitStatusChange(entry, { removed: true }); debugLogger.info(`Unregistered foreground agent: ${agentId}`); } @@ -627,10 +640,13 @@ export class BackgroundTaskRegistry { } } - private emitStatusChange(entry?: BackgroundTaskEntry): void { + private emitStatusChange( + entry?: BackgroundTaskEntry, + context?: { removed?: boolean }, + ): void { if (!this.statusChangeCallback) return; try { - this.statusChangeCallback(entry); + this.statusChangeCallback(entry, context); } catch (error) { debugLogger.error('Failed to emit background status change:', error); } From 09d6fc506c30513b2d67d1d93358ed50b83e6937 Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 8 May 2026 00:43:07 +0800 Subject: [PATCH 3/9] docs(cli): update ToolGroupMessageProps.isPending JSDoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous prop comment claimed `isPending` was "not consumed by the group body" — true at the time, but the body now reads it for two real purposes (compact-mode gating + forwarding to ToolMessage). Update the doc so future callers / tests don't treat it as legacy. Addresses Copilot finding on PRRT_kwDOPB-92c6AYE0V. --- .../src/ui/components/messages/ToolGroupMessage.tsx | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index c8dd4245da3..76eccd0afba 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -51,9 +51,14 @@ interface ToolGroupMessageProps { /** * True when this tool group is being rendered live (in * `pendingHistoryItems`). False once it commits to Ink's ``. - * Currently consumed by upstream callers but not by the group body - * itself — the subagent renderer used to gate its live frame on - * this; that gating moved to LiveAgentPanel + BackgroundTasksDialog. + * + * Read by the group body to (1) force-expand a compact group when + * it has just committed AND carries a terminal subagent, so + * `SubagentScrollbackSummary` lands in the persistent record, and + * (2) forward to `ToolMessage` so the same renderer can decide + * whether to emit the inline summary or stay panel-only. Live + * progress / drill-down for running subagents themselves still + * happens via `LiveAgentPanel` + `BackgroundTasksDialog`. */ isPending?: boolean; activeShellPtyId?: number | null; From 602fdf279c74b87001062deda765997e446b8e4f Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 8 May 2026 01:01:08 +0800 Subject: [PATCH 4/9] =?UTF-8?q?fix(cli):=20hide=20live-phase=20subagent=20?= =?UTF-8?q?tool=20entries=20=E2=80=94=20LiveAgentPanel=20owns=20the=20row?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User report: with compact mode OFF, a running subagent shows up twice — once as the parent tool group's `task` row (status icon + name + description), once as the LiveAgentPanel row beneath the composer. Same agent, two surfaces, redundant. Filter `task_execution` tool entries out of the expanded `ToolGroupMessage` while `isPending=true` so the panel is the single source of truth for in-flight subagents. The entry returns once the parent turn commits (`isPending=false`), letting `SubagentScrollbackSummary` land inside the parent's tool group as a persistent audit trail. Exception: subagents with a pending approval still render, because the focus-routed banner / queued marker is the only inline surface that lets users answer the prompt without opening the dialog. If a group is purely panel-owned (e.g. a single Task call with no sibling tools), the entire `ToolGroupMessage` returns `null` so an empty bordered container doesn't float above the panel. Coverage: +4 ToolGroupMessage cases — running entry hidden in live phase / mixed group keeps siblings / pending-approval entry still renders / committed entry comes back for the audit trail. --- .../messages/ToolGroupMessage.test.tsx | 88 +++++++++++++++++++ .../components/messages/ToolGroupMessage.tsx | 55 ++++++++++++ 2 files changed, 143 insertions(+) diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx index 83f6ee63899..a4e5ab11fa6 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx @@ -656,6 +656,94 @@ describe('', () => { expect(lastFrame() ?? '').not.toContain('MockSubagent[task-completed]'); }); + it('live phase (non-compact): running subagent tool entry is hidden — panel owns the row', () => { + // Without this filter the user sees the same subagent twice — + // once as the parent tool group's `task` row, once as the + // `LiveAgentPanel` row beneath the composer. Hide the inline + // entry while `isPending=true` so the panel is the single + // source of truth for in-flight subagents. + const { lastFrame } = renderWithProviders( + , + ); + // Pure-subagent group with everything panel-owned → entire + // group is hidden so an empty bordered container doesn't + // float above the panel. + expect(lastFrame() ?? '').toBe(''); + }); + + it('live phase (non-compact): mixed group still renders sibling tools', () => { + // Only the subagent entry is hidden in live phase — sibling + // tools (Read / Edit / Bash) keep rendering normally so the + // parent's tool stream stays continuous. + const sibling = createToolCall({ + callId: 'read-1', + name: 'read_file', + description: 'read config.yaml', + status: ToolCallStatus.Success, + }); + const { lastFrame } = renderWithProviders( + , + ); + const frame = lastFrame() ?? ''; + // Sibling shown. + expect(frame).toContain('read_file'); + // Subagent hidden — panel owns the live row. + expect(frame).not.toContain('MockSubagent[task-running]'); + }); + + it('live phase (non-compact): subagent with pending approval still renders', () => { + // The focus-routed approval banner / queued marker is the + // only inline surface that lets users answer the prompt + // without opening the dialog, so the entry must NOT be + // hidden when the subagent is awaiting confirmation. + const pending = createToolCall({ + callId: 'task-pending', + name: 'task', + description: 'Delegate task to subagent', + status: ToolCallStatus.Executing, + resultDisplay: { + type: 'task_execution', + subagentName: 'researcher', + taskDescription: 'investigate the change', + taskPrompt: 'investigate', + status: 'running', + pendingConfirmation: { type: 'info', title: 't', prompt: 'p' }, + } as AgentResultDisplay, + }); + const { lastFrame } = renderWithProviders( + , + ); + // Subagent entry rendered (banner / marker fires inside + // ToolMessage); panel sits below as ambient progress. + expect(lastFrame() ?? '').toContain('MockSubagent[task-pending]'); + }); + + it('committed phase (non-compact): subagent tool entry comes back for the audit trail', () => { + // Once the parent turn commits the panel evicts the row and + // the inline entry returns so SubagentScrollbackSummary lands + // inside the parent's tool group as a permanent record. + const { lastFrame } = renderWithProviders( + , + ); + expect(lastFrame() ?? '').toContain('MockSubagent[task-completed]'); + }); + it('compact mode: committed group with failed subagent forces expand', () => { // Same as the completed case — the scrollback summary needs to // land for failed / cancelled foreground subagents too so the diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index 76eccd0afba..209931f525c 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -42,6 +42,23 @@ function isRunningAgent( ); } +/** + * Tool entry whose live (`isPending=true`) display would duplicate + * the LiveAgentPanel row. Used to hide the inline tool-group entry + * during the live phase so the panel is the single source of truth; + * once the parent turn commits (`isPending=false`) the entry returns, + * carrying the SubagentScrollbackSummary as its persistent record. + */ +function isLiveSubagentTool(tool: IndividualToolCallDisplay): boolean { + const rd = tool.resultDisplay; + return ( + typeof rd === 'object' && + rd !== null && + 'type' in rd && + (rd as AgentResultDisplay).type === 'task_execution' + ); +} + interface ToolGroupMessageProps { groupId: number; toolCalls: IndividualToolCallDisplay[]; @@ -209,6 +226,24 @@ export const ToolGroupMessage: React.FC = ({ ); } + // Live phase: if every tool in the group is a panel-owned subagent + // call (no pending approval to gate on), skip the entire group + // container — otherwise the bordered Box would render empty under + // the panel. Once the parent turn commits the call sites stop + // passing `isPending=true` and the group reappears with the + // committed audit trail. + const allEntriesPanelOwned = + isPending && + toolCalls.length > 0 && + toolCalls.every( + (tool) => + isLiveSubagentTool(tool) && + !isAgentWithPendingConfirmation(tool.resultDisplay), + ); + if (allEntriesPanelOwned) { + return null; + } + // Full expanded view const hasPending = !toolCalls.every( (t) => t.status === ToolCallStatus.Success, @@ -312,6 +347,26 @@ export const ToolGroupMessage: React.FC = ({ ); })()} {toolCalls.map((tool) => { + // Live phase: hide subagent (`task_execution`) tool entries + // entirely so LiveAgentPanel below the composer is the single + // source of truth for in-flight subagents. Otherwise the + // user sees the same agent twice — once as the parent tool + // group's row, once as the panel row. Once the parent turn + // commits (`isPending=false`) the row reappears so the + // SubagentScrollbackSummary lands inside the parent's tool + // group and the audit trail stays intact. + // + // EXCEPT when the subagent has a pending approval: the + // ToolMessage path renders the focus-routed banner / queued + // marker, which is the only way the user can answer the + // prompt without opening the dialog. + if ( + isPending && + isLiveSubagentTool(tool) && + !isAgentWithPendingConfirmation(tool.resultDisplay) + ) { + return null; + } const isConfirming = toolAwaitingApproval?.callId === tool.callId; // A subagent's inline approval prompt should only receive keyboard // focus when (1) there is no direct tool-level confirmation active From 4509556ac37d493a3ca7382d5fa9e45894260811 Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 8 May 2026 01:13:01 +0800 Subject: [PATCH 5/9] refactor(cli): tighten subagent-tool helper naming + ANSI-safe scrollback summary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-audit + independent review found 5 cleanup items on the live-phase hide path; all addressed in one commit since none are behavioral changes: 1. **Move `allEntriesPanelOwned` short-circuit BEFORE `showCompact`** so a pure-subagent group in compact mode is also hidden during the live phase (previously CompactToolGroupDisplay rendered a single summary line above the panel — a mild duplicate on top of what the non-compact path already fixed). 2. **Rename `isLiveSubagentTool` → `isSubagentToolEntry`.** The helper identifies a tool's resultDisplay shape; it doesn't check live-state. The previous name conflated "predicate" with "use case" and read as if it returned true only during the live phase. 3. **DRY up `hasCommittedTerminalSubagent`** to use `isSubagentToolEntry` instead of inlining its own type-narrowing. 4. **ANSI-escape `subagentName` / `taskDescription` / `terminateReason`** in `SubagentScrollbackSummary`. Same threat model as the panel rows and HistoryItemDisplay — these strings come from subagent config (user-authored) and LLM output and could carry terminal control sequences. The stats fields (tool count / duration / tokens) flow through trusted formatters and don't need escaping. 5. **Doc comments updated** to reflect the four real responsibilities of `isPending` on `ToolGroupMessageProps` (hide pure groups, force-expand committed compact, per-tool filter, forward to ToolMessage), to clarify that the keyboard-focused subagent id can point at a hidden tool harmlessly (the iterator returns `null` before the focus prop is computed), and to drop the redundant "EXCEPT" clause on the per-tool filter in favor of a single sentence. Coverage unchanged: 251 passing tests across messages / background-view / core/agents; broader 3374-test sweep clean; TS clean on both cli and core packages. --- .../components/messages/ToolGroupMessage.tsx | 128 ++++++++++-------- .../ui/components/messages/ToolMessage.tsx | 22 ++- 2 files changed, 89 insertions(+), 61 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index 209931f525c..1e76e0b5a48 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -43,13 +43,16 @@ function isRunningAgent( } /** - * Tool entry whose live (`isPending=true`) display would duplicate - * the LiveAgentPanel row. Used to hide the inline tool-group entry - * during the live phase so the panel is the single source of truth; - * once the parent turn commits (`isPending=false`) the entry returns, - * carrying the SubagentScrollbackSummary as its persistent record. + * Predicate: tool entry whose `resultDisplay` is an `AgentResultDisplay` + * (i.e. a `task_execution` subagent invocation). Used by the group + * body to: + * - hide such entries during the live phase (LiveAgentPanel owns + * the row, so leaving them inline duplicates the display) + * - force-expand a committed compact group containing one with a + * terminal status, so SubagentScrollbackSummary lands in the + * persistent record */ -function isLiveSubagentTool(tool: IndividualToolCallDisplay): boolean { +function isSubagentToolEntry(tool: IndividualToolCallDisplay): boolean { const rd = tool.resultDisplay; return ( typeof rd === 'object' && @@ -69,13 +72,24 @@ interface ToolGroupMessageProps { * True when this tool group is being rendered live (in * `pendingHistoryItems`). False once it commits to Ink's ``. * - * Read by the group body to (1) force-expand a compact group when - * it has just committed AND carries a terminal subagent, so - * `SubagentScrollbackSummary` lands in the persistent record, and - * (2) forward to `ToolMessage` so the same renderer can decide - * whether to emit the inline summary or stay panel-only. Live - * progress / drill-down for running subagents themselves still - * happens via `LiveAgentPanel` + `BackgroundTasksDialog`. + * Read by the group body to: + * 1. Hide the entire group when every tool entry is a panel-owned + * subagent (`task_execution` without pending approval) — the + * LiveAgentPanel below the composer is the single source of + * truth for in-flight subagents. Group reappears once the parent + * turn commits with the SubagentScrollbackSummary as the + * persistent audit trail. + * 2. Force-expand a compact group when committed AND carrying a + * terminal subagent, so `SubagentScrollbackSummary` actually + * lands in the persistent record (CompactToolGroupDisplay is + * otherwise unaware of `task_execution` results). + * 3. Filter individual subagent entries out of the inline list + * during the live phase — same panel-ownership rule as #1, but + * applies per-tool when the group is mixed (subagent + sibling + * tools), so siblings keep rendering normally. + * 4. Forward to `ToolMessage` so the same renderer can decide + * whether to emit the inline scrollback summary or stay + * panel-only. */ isPending?: boolean; activeShellPtyId?: number | null; @@ -172,9 +186,17 @@ export const ToolGroupMessage: React.FC = ({ const focusedSubagentCallId = focusedSubagentRef.current; // When no subagent has a pending confirmation, fall back to the *first* - // running subagent for Ctrl+E/Ctrl+F shortcut focus. "First" (array order) - // is the oldest — the one most likely to have accumulated tool calls and - // display the "+N more (ctrl+e to expand)" hint. + // running subagent for keyboard focus. "First" (array order) is the + // oldest — the one most likely to be the focal subagent. The legacy + // Ctrl+E / Ctrl+F display shortcuts retired with the inline frame, so + // the fallback is now mostly inert; it stays here so a future + // re-introduction of inline keyboard surfaces has a focus target. + // Note: during the live phase running subagent entries are filtered + // out of the rendered map (`isPending && isSubagentToolEntry && + // !pending`), so this id can point at a hidden tool. That's harmless + // — `isSubagentFocused` is only read inside the map iteration which + // already returned `null` for the hidden entry, so no focus prop + // ever reaches a missing DOM node. const runningSubagentCallId = useMemo( () => toolCalls.find((tc) => isRunningAgent(tc.resultDisplay))?.callId ?? null, @@ -184,6 +206,24 @@ export const ToolGroupMessage: React.FC = ({ const keyboardFocusedSubagentCallId = focusedSubagentCallId ?? runningSubagentCallId; + // Decide whether the entire group is panel-owned during the live + // phase: a pure-task_execution batch with no pending approval has + // nothing inline left to render once the subagent rows themselves + // are filtered out (LiveAgentPanel handles them). Hide the whole + // group so the bordered container / compact summary line don't + // float above the panel as a duplicate. + const allEntriesPanelOwned = + isPending && + toolCalls.length > 0 && + toolCalls.every( + (tool) => + isSubagentToolEntry(tool) && + !isAgentWithPendingConfirmation(tool.resultDisplay), + ); + if (allEntriesPanelOwned) { + return null; + } + // Compact mode: entire group → single line summary // Force-expand when: user must interact (Confirming or subagent pending // confirmation), tool errored, shell is focused, or user-initiated. @@ -197,16 +237,13 @@ export const ToolGroupMessage: React.FC = ({ const hasSubagentPendingConfirmation = subagentsAwaitingApproval.length > 0; const hasCommittedTerminalSubagent = !isPending && - toolCalls.some((t) => { - const display = t.resultDisplay; - if (!display || typeof display !== 'object') return false; - if (!('type' in display) || display.type !== 'task_execution') - return false; - const status = (display as { status?: string }).status; - return ( - status === 'completed' || status === 'failed' || status === 'cancelled' - ); - }); + toolCalls.some( + (t) => + isSubagentToolEntry(t) && + ((t.resultDisplay as AgentResultDisplay).status === 'completed' || + (t.resultDisplay as AgentResultDisplay).status === 'failed' || + (t.resultDisplay as AgentResultDisplay).status === 'cancelled'), + ); const showCompact = compactMode && !hasConfirmingTool && @@ -226,24 +263,6 @@ export const ToolGroupMessage: React.FC = ({ ); } - // Live phase: if every tool in the group is a panel-owned subagent - // call (no pending approval to gate on), skip the entire group - // container — otherwise the bordered Box would render empty under - // the panel. Once the parent turn commits the call sites stop - // passing `isPending=true` and the group reappears with the - // committed audit trail. - const allEntriesPanelOwned = - isPending && - toolCalls.length > 0 && - toolCalls.every( - (tool) => - isLiveSubagentTool(tool) && - !isAgentWithPendingConfirmation(tool.resultDisplay), - ); - if (allEntriesPanelOwned) { - return null; - } - // Full expanded view const hasPending = !toolCalls.every( (t) => t.status === ToolCallStatus.Success, @@ -348,21 +367,16 @@ export const ToolGroupMessage: React.FC = ({ })()} {toolCalls.map((tool) => { // Live phase: hide subagent (`task_execution`) tool entries - // entirely so LiveAgentPanel below the composer is the single - // source of truth for in-flight subagents. Otherwise the - // user sees the same agent twice — once as the parent tool - // group's row, once as the panel row. Once the parent turn - // commits (`isPending=false`) the row reappears so the - // SubagentScrollbackSummary lands inside the parent's tool - // group and the audit trail stays intact. - // - // EXCEPT when the subagent has a pending approval: the - // ToolMessage path renders the focus-routed banner / queued - // marker, which is the only way the user can answer the - // prompt without opening the dialog. + // unless the subagent is awaiting user approval. The inline + // banner / queued marker is the only way the user can answer + // the approval prompt without opening the dialog, so those + // entries always render. Other subagent entries are hidden + // because LiveAgentPanel below the composer paints them; the + // entry returns once the parent turn commits and carries the + // SubagentScrollbackSummary as the persistent audit trail. if ( isPending && - isLiveSubagentTool(tool) && + isSubagentToolEntry(tool) && !isAgentWithPendingConfirmation(tool.resultDisplay) ) { return null; diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index 03aea91b9b3..fbd20529d49 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -33,7 +33,11 @@ import { theme } from '../../semantic-colors.js'; import { useSettings } from '../../contexts/SettingsContext.js'; import type { LoadedSettings } from '../../../config/settings.js'; import { useCompactMode } from '../../contexts/CompactModeContext.js'; -import { getCachedStringWidth, toCodePoints } from '../../utils/textUtils.js'; +import { + escapeAnsiCtrlCodes, + getCachedStringWidth, + toCodePoints, +} from '../../utils/textUtils.js'; import { ToolStatusIndicator, @@ -376,18 +380,28 @@ const SubagentScrollbackSummary: React.FC<{ if (stats?.totalTokens && stats.totalTokens > 0) { parts.push(`${formatTokenCount(stats.totalTokens)} tokens`); } + // Sanitize every user/LLM-controlled string before it reaches Ink. + // `subagentName` is subagent config (user-authored or model-chosen), + // `taskDescription` is LLM-generated, `terminateReason` is whatever + // the agent emitted on failure. All can carry terminal control + // sequences that would otherwise bleed through Ink's `` and + // corrupt scrollback chrome — same threat model as the panel rows + // and HistoryItemDisplay's user-facing content. const tail = parts.length > 0 ? ` · ${parts.join(' · ')}` : ''; - const typePrefix = data.subagentName ? `${data.subagentName}: ` : ''; + const typePrefix = data.subagentName + ? `${escapeAnsiCtrlCodes(data.subagentName)}: ` + : ''; + const safeDescription = escapeAnsiCtrlCodes(data.taskDescription ?? ''); const reason = data.status !== 'completed' && data.terminateReason - ? ` · ${data.terminateReason}` + ? ` · ${escapeAnsiCtrlCodes(data.terminateReason)}` : ''; return ( {`${glyph} `} {typePrefix} - {data.taskDescription} + {safeDescription} {tail} {reason} From b4e4b17873deda9d9d4333f34bf2778782b86b31 Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 8 May 2026 06:57:25 +0800 Subject: [PATCH 6/9] fix(cli,core): address 3 critical review findings + ANSI/doc cleanups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three real bugs flagged by gpt-5.5 via /qreview, plus 4 doc / sanitization nits from Copilot. All 7 threads close together since they share the same surfaces. ## Critical fixes 1. **Foreground subagents disappeared mid-parent-turn** (PRRT_kwDOPB-92c6AYvL9). Post-#3921 swap-order, `unregisterForeground` drops the entry from the panel snapshot the moment the subagent finishes. The previous round's `!isPending` gate on `SubagentScrollbackSummary` then suppressed the inline summary too, leaving the user with nothing on screen for the run until the parent committed. - Drop the `!isPending` gate — `unregisterForeground` already removes the row from the panel, so the inline summary can fire in BOTH live and committed phases without duplicating it. - Tighten the `ToolGroupMessage` live-phase hide so it only filters `running` / `paused` / `background` task entries (`isPanelOwnedSubagentTool`), not terminal ones. Terminal entries pass through immediately so the summary lands. - The "panel-owned" predicate is now distinct from the broader "subagent tool entry" predicate (`isSubagentToolEntry`) and the "terminal subagent" predicate (`isTerminalSubagentTool`); each usage site picks the one it actually means. 2. **Compact mode dropped the scrollback summary** (PRRT_kwDOPB-92c6AYvLw). Force-expanding the group made the container go through the expanded path, but `ToolMessage`'s own compact-mode gate (`!compactMode || forceShowResult ? renderer : 'none'`) still suppressed the result block, so `SubagentScrollbackSummary` never rendered for compact-mode users. Pass `forceShowResult={true}` for terminal subagent tool entries so the result block is always rendered. 3. **`mergeCompactToolGroups.isForceExpandGroup` didn't know about terminal subagents** (PRRT_kwDOPB-92c6AYvMC). The committed- history preprocessor merged adjacent tool_groups before render, so a terminal `task_execution` group could be absorbed into a compact batch (its `tool_use_summary` label dropped), and the render-time force-expand check never got a chance to override. Mirror the `hasCommittedTerminalSubagent` predicate inside `isForceExpandGroup` so preprocessing and rendering agree. ## Doc / sanitization nits - `BackgroundStatusChangeCallback` doc now lists every emitter (register / complete / fail / cancel / finalizeCancelled / finalizeCancellationIfPending / abandon / unregisterForeground / reset) and groups them by ordering camp (keeps-the-entry vs removes-the-entry — `reset` joins `unregisterForeground` in the delete-then-emit camp). - ANSI-escape `data.subagentName` in the focus-holder banner and the queued marker (`SubagentExecutionRenderer`) — same threat model as the panel rows and `SubagentScrollbackSummary`. ## Coverage delta - New ToolMessage case: live-phase terminal subagent now renders inline (replaces the prior "no scrollback summary" assertion that was the symptom of the AYvL9 bug). - New ToolGroupMessage cases: terminal subagent in live phase renders inline; `forceShowResult=true` propagates for terminal subagent tools (mock now exposes the prop). - New mergeCompactToolGroups parametrized cases: terminal subagent in any of completed / failed / cancelled stays its own batch. 280 tests pass across cli messages + utils + background-view + core/agents. TS clean. --- .../messages/ToolGroupMessage.test.tsx | 48 +++++++++- .../components/messages/ToolGroupMessage.tsx | 93 ++++++++++++------- .../components/messages/ToolMessage.test.tsx | 22 +++-- .../ui/components/messages/ToolMessage.tsx | 81 ++++++++-------- .../ui/utils/mergeCompactToolGroups.test.ts | 42 +++++++++ .../src/ui/utils/mergeCompactToolGroups.ts | 27 ++++++ packages/core/src/agents/background-tasks.ts | 30 +++--- 7 files changed, 252 insertions(+), 91 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx index a4e5ab11fa6..d9d7417a612 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx @@ -30,6 +30,7 @@ vi.mock('./ToolMessage.js', () => ({ emphasis, resultDisplay, isFocused, + forceShowResult, }: { callId: string; name: string; @@ -38,6 +39,7 @@ vi.mock('./ToolMessage.js', () => ({ emphasis: string; resultDisplay?: unknown; isFocused?: boolean; + forceShowResult?: boolean; }) { // Use the same constants as the real component const statusSymbolMap: Record = { @@ -54,9 +56,13 @@ vi.mock('./ToolMessage.js', () => ({ typeof resultDisplay === 'object' && (resultDisplay as { type?: string }).type === 'task_execution' ) { + // `forceShowResult` is the gate that lets `SubagentScrollbackSummary` + // render in compact mode — surfaced in the mock so tests can + // assert it was passed for terminal subagent tools. return ( - MockSubagent[{callId}]: focused={String(isFocused)} + MockSubagent[{callId}]: focused={String(isFocused)} force= + {String(Boolean(forceShowResult))} ); } @@ -730,6 +736,27 @@ describe('', () => { expect(lastFrame() ?? '').toContain('MockSubagent[task-pending]'); }); + it('live phase (non-compact): TERMINAL subagent renders inline (panel snapshot already dropped)', () => { + // Post-#3921 swap-order, `unregisterForeground` removes the + // foreground entry from the panel snapshot the moment the + // subagent finishes. If the inline path also stayed hidden in + // the live phase, the user would see nothing for the run + // until the parent commits — `SubagentScrollbackSummary` has + // to bridge that gap. Live-phase hide applies only to + // running / paused / background entries. + const { lastFrame } = renderWithProviders( + , + ); + // Terminal entry rendered → MockSubagent sentinel from the + // ToolMessage mock; if the entry were still hidden the frame + // would be empty. + expect(lastFrame() ?? '').toContain('MockSubagent[task-completed]'); + }); + it('committed phase (non-compact): subagent tool entry comes back for the audit trail', () => { // Once the parent turn commits the panel evicts the row and // the inline entry returns so SubagentScrollbackSummary lands @@ -744,6 +771,25 @@ describe('', () => { expect(lastFrame() ?? '').toContain('MockSubagent[task-completed]'); }); + it('terminal subagent tool receives forceShowResult so the summary renders in compact mode', () => { + // Force-expanding the group is necessary but not sufficient — + // `ToolMessage`'s own compact-mode gate + // (`!compactMode || forceShowResult`) would otherwise drop the + // result block, so the inner SubagentScrollbackSummary never + // gets a chance to render. ToolGroupMessage must propagate + // `forceShowResult=true` for terminal subagent tools. + const { lastFrame } = renderCompact( + , + ); + const frame = lastFrame() ?? ''; + expect(frame).toContain('MockSubagent[task-completed]'); + expect(frame).toContain('force=true'); + }); + it('compact mode: committed group with failed subagent forces expand', () => { // Same as the completed case — the scrollback summary needs to // land for failed / cancelled foreground subagents too so the diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index 1e76e0b5a48..1fd427b842f 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -44,13 +44,7 @@ function isRunningAgent( /** * Predicate: tool entry whose `resultDisplay` is an `AgentResultDisplay` - * (i.e. a `task_execution` subagent invocation). Used by the group - * body to: - * - hide such entries during the live phase (LiveAgentPanel owns - * the row, so leaving them inline duplicates the display) - * - force-expand a committed compact group containing one with a - * terminal status, so SubagentScrollbackSummary lands in the - * persistent record + * (i.e. a `task_execution` subagent invocation), regardless of status. */ function isSubagentToolEntry(tool: IndividualToolCallDisplay): boolean { const rd = tool.resultDisplay; @@ -62,6 +56,36 @@ function isSubagentToolEntry(tool: IndividualToolCallDisplay): boolean { ); } +/** + * Predicate: subagent tool entry whose live UI is owned by + * `LiveAgentPanel`. Only running / paused / background entries + * should be hidden during the live phase — terminal entries (the + * subagent already finished while the parent turn is still + * running) are NOT panel-owned: the panel snapshot drops them on + * `unregisterForeground`'s post-delete emit, so the inline path + * needs to render `SubagentScrollbackSummary` immediately so the + * user keeps a record of the run instead of seeing nothing. + */ +function isPanelOwnedSubagentTool(tool: IndividualToolCallDisplay): boolean { + if (!isSubagentToolEntry(tool)) return false; + const status = (tool.resultDisplay as AgentResultDisplay).status; + return status === 'running' || status === 'paused' || status === 'background'; +} + +/** + * Predicate: tool entry whose subagent has reached a terminal state + * (`completed` / `failed` / `cancelled`). Used to force-expand the + * group + force the inner ToolMessage to render its result block in + * compact mode, so `SubagentScrollbackSummary` actually lands. + */ +function isTerminalSubagentTool(tool: IndividualToolCallDisplay): boolean { + if (!isSubagentToolEntry(tool)) return false; + const status = (tool.resultDisplay as AgentResultDisplay).status; + return ( + status === 'completed' || status === 'failed' || status === 'cancelled' + ); +} + interface ToolGroupMessageProps { groupId: number; toolCalls: IndividualToolCallDisplay[]; @@ -207,17 +231,21 @@ export const ToolGroupMessage: React.FC = ({ focusedSubagentCallId ?? runningSubagentCallId; // Decide whether the entire group is panel-owned during the live - // phase: a pure-task_execution batch with no pending approval has - // nothing inline left to render once the subagent rows themselves - // are filtered out (LiveAgentPanel handles them). Hide the whole - // group so the bordered container / compact summary line don't - // float above the panel as a duplicate. + // phase: a pure-running-subagent batch with no pending approval + // has nothing inline left to render once the subagent rows + // themselves are filtered out (LiveAgentPanel handles them). + // Hide the whole group so the bordered container / compact + // summary line don't float above the panel as a duplicate. + // Terminal subagents (completed / failed / cancelled) are NOT + // panel-owned — `unregisterForeground`'s post-delete emit drops + // them from the panel snapshot, so the inline path must render + // their `SubagentScrollbackSummary` instead. const allEntriesPanelOwned = isPending && toolCalls.length > 0 && toolCalls.every( (tool) => - isSubagentToolEntry(tool) && + isPanelOwnedSubagentTool(tool) && !isAgentWithPendingConfirmation(tool.resultDisplay), ); if (allEntriesPanelOwned) { @@ -236,14 +264,7 @@ export const ToolGroupMessage: React.FC = ({ // because the panel below the composer is still showing the row. const hasSubagentPendingConfirmation = subagentsAwaitingApproval.length > 0; const hasCommittedTerminalSubagent = - !isPending && - toolCalls.some( - (t) => - isSubagentToolEntry(t) && - ((t.resultDisplay as AgentResultDisplay).status === 'completed' || - (t.resultDisplay as AgentResultDisplay).status === 'failed' || - (t.resultDisplay as AgentResultDisplay).status === 'cancelled'), - ); + !isPending && toolCalls.some(isTerminalSubagentTool); const showCompact = compactMode && !hasConfirmingTool && @@ -366,17 +387,19 @@ export const ToolGroupMessage: React.FC = ({ ); })()} {toolCalls.map((tool) => { - // Live phase: hide subagent (`task_execution`) tool entries - // unless the subagent is awaiting user approval. The inline - // banner / queued marker is the only way the user can answer - // the approval prompt without opening the dialog, so those - // entries always render. Other subagent entries are hidden - // because LiveAgentPanel below the composer paints them; the - // entry returns once the parent turn commits and carries the - // SubagentScrollbackSummary as the persistent audit trail. + // Live phase: hide ONLY panel-owned subagent entries (running / + // paused / background) so LiveAgentPanel below the composer + // is the single source of truth for in-flight subagents. + // Terminal subagents (completed / failed / cancelled) ARE + // rendered inline immediately so `SubagentScrollbackSummary` + // lands the moment the subagent finishes — `unregisterForeground` + // already dropped the panel row, leaving inline as the only + // place the user can see the run's outcome until commit. + // Pending-approval entries also pass through so the inline + // banner / queued marker can answer the prompt. if ( isPending && - isSubagentToolEntry(tool) && + isPanelOwnedSubagentTool(tool) && !isAgentWithPendingConfirmation(tool.resultDisplay) ) { return null; @@ -415,7 +438,15 @@ export const ToolGroupMessage: React.FC = ({ isUserInitiated || tool.status === ToolCallStatus.Confirming || tool.status === ToolCallStatus.Error || - isAgentWithPendingConfirmation(tool.resultDisplay) + isAgentWithPendingConfirmation(tool.resultDisplay) || + // Terminal subagents need their result block to render + // even in compact mode — that's where + // `SubagentScrollbackSummary` lands. ToolMessage's + // compact-mode gate + // (`!compactMode || forceShowResult ? renderer : 'none'`) + // would otherwise drop the result block, leaving the + // committed audit trail empty for compact-mode users. + isTerminalSubagentTool(tool) } isFocused={isSubagentFocused} isPending={isPending} diff --git a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx index 4af205d5019..f559c554913 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx @@ -387,13 +387,16 @@ describe('', () => { expect(output).not.toContain('MockApprovalPrompt'); }); - it('live (`isPending`) terminal subagent → no scrollback summary (panel owns the row)', () => { - // While the parent turn is still in `pendingHistoryItems`, - // LiveAgentPanel below the composer renders the synthesized / - // terminal row — emitting the summary here too would duplicate - // it visually. The summary fires only when the tool message - // commits to (`isPending=false`), guaranteeing exactly - // one inline copy in the eventual scrollback record. + it('live (`isPending`) terminal subagent → renders summary inline (panel snapshot already dropped)', () => { + // After `unregisterForeground`'s post-delete emit (#3921 swap- + // order), the panel snapshot drops the foreground entry as soon + // as the subagent finishes — even while the parent turn is + // still in `pendingHistoryItems`. If the inline summary were + // also gated on `!isPending`, a foreground subagent that + // finishes mid-turn would simply disappear from screen until + // commit. Render the summary in BOTH live and committed phases; + // the live-phase filter in `ToolGroupMessage` already keeps + // running entries from reaching this renderer. const { lastFrame } = renderWithContext( ', () => { StreamingState.Responding, ); const output = lastFrame() ?? ''; - // No inline summary — the panel renders this row instead. - expect(output).not.toContain('✔'); - expect(output).not.toContain('Just finished mid-turn'); + expect(output).toContain('✔'); + expect(output).toContain('Just finished mid-turn'); }); it('failed subagent → renders summary with terminate reason', () => { diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index fbd20529d49..ce22100d163 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -256,24 +256,27 @@ const PlanResultRenderer: React.FC<{ * * The verbose inline frame has been retired. Three surfaces remain: * - * - **Live phase (running)**: nothing inline — `LiveAgentPanel` (the - * always-on bottom roster) and `BackgroundTasksDialog` (Down-arrow - * detail view) own progress reporting. - * - **Approval prompt (focus-locked)**: full inline approval banner so - * the user can answer without context-switching into the dialog; + * - **Running**: nothing inline — `LiveAgentPanel` (the always-on + * bottom roster) and `BackgroundTasksDialog` (Down-arrow detail + * view) own progress reporting. `ToolGroupMessage` filters + * running task entries out of the live phase entirely so the + * group container doesn't even attempt to render this renderer. + * - **Approval prompt (focus-locked)**: full inline approval banner + * so the user can answer without context-switching into the dialog; * sibling subagents render a queued marker. - * - **Committed phase (terminal — completed / failed / cancelled)**: a - * single-line scrollback summary so the conversation history retains - * a permanent record after the panel's 8s window expires and the - * dialog closes. Format: ` : · N tools · Xs · Yk tokens`. + * - **Terminal (completed / failed / cancelled)**: a single-line + * scrollback summary so the conversation history retains a + * permanent record after the panel evicts. Fires regardless of + * `isPending` — `unregisterForeground`'s post-delete emit drops + * the panel snapshot row immediately, so the inline summary is + * the only surface that bridges the moment a foreground subagent + * finishes mid-parent-turn until the parent commits. + * Format: ` : · N tools · Xs · Yk tokens`. * - * The committed-phase summary is gated on `!isPending` — without the - * gate the summary would also paint into `pendingHistoryItems` - * (the live area) the moment a subagent terminates, duplicating the - * row LiveAgentPanel already renders synthesized below the composer. - * `isPending=true` means we're still in the live area; `isPending=false` - * means the item has committed to `` and the summary is the - * one and only place the user can find this run after the panel evicts. + * `isPending` is no longer used as a render gate here; the live-phase + * filter in `ToolGroupMessage` handles the running case before this + * renderer is reached. The prop is kept on the signature for future + * needs and parity with sibling renderers. */ const SubagentExecutionRenderer: React.FC<{ data: AgentResultDisplay; @@ -282,16 +285,17 @@ const SubagentExecutionRenderer: React.FC<{ config: Config; isFocused?: boolean; isPending?: boolean; -}> = ({ - data, - availableHeight, - childWidth, - config, - isFocused, - isPending = false, -}) => { + // `isPending` stays on the prop signature for parity with sibling + // renderers and possible future gating, but isn't read here — the + // live-phase filter in `ToolGroupMessage` already keeps running + // entries from reaching this renderer (so the terminal-summary path + // is the only thing left to gate, and it should fire in both phases). +}> = ({ data, availableHeight, childWidth, config, isFocused }) => { if (data.pendingConfirmation && isFocused) { - const agentLabel = data.subagentName || 'agent'; + // `subagentName` is user-authored / model-chosen and may carry + // ANSI control sequences; escape before rendering into Ink Text + // (matches LiveAgentPanel + SubagentScrollbackSummary). + const agentLabel = escapeAnsiCtrlCodes(data.subagentName || 'agent'); return ( @@ -313,7 +317,10 @@ const SubagentExecutionRenderer: React.FC<{ ); } if (data.pendingConfirmation) { - const agentLabel = data.subagentName || 'agent'; + // `subagentName` is user-authored / model-chosen and may carry + // ANSI control sequences; escape before rendering into Ink Text + // (matches LiveAgentPanel + SubagentScrollbackSummary). + const agentLabel = escapeAnsiCtrlCodes(data.subagentName || 'agent'); return ( @@ -324,18 +331,18 @@ const SubagentExecutionRenderer: React.FC<{ ); } // Terminal phase: render a single-line scrollback summary so the - // conversation history keeps a permanent record after the panel's - // 8s visibility window expires (LiveAgentPanel evicts terminal rows; - // BackgroundTasksDialog only retains them while open). Skip - // `running` / `background` since the panel + dialog cover those. - // ALSO skip while `isPending` — the live area is already painted by - // the panel, so emitting the summary into `pendingHistoryItems` would - // duplicate the row visually until the parent turn commits. + // conversation history keeps a permanent record. Fires in BOTH + // live and committed phases — `unregisterForeground`'s post-delete + // emit drops the panel snapshot row immediately, so without an + // inline render here a foreground subagent that finishes + // mid-parent-turn would simply disappear from screen until commit. + // No duplication risk because the panel never re-resurrects a + // dropped foreground entry. Skip `running` / `background` since the + // panel + dialog cover those. if ( - !isPending && - (data.status === 'completed' || - data.status === 'failed' || - data.status === 'cancelled') + data.status === 'completed' || + data.status === 'failed' || + data.status === 'cancelled' ) { return ; } diff --git a/packages/cli/src/ui/utils/mergeCompactToolGroups.test.ts b/packages/cli/src/ui/utils/mergeCompactToolGroups.test.ts index 405516234e7..a131b5a67e5 100644 --- a/packages/cli/src/ui/utils/mergeCompactToolGroups.test.ts +++ b/packages/cli/src/ui/utils/mergeCompactToolGroups.test.ts @@ -203,6 +203,48 @@ describe('mergeCompactToolGroups', () => { // Group 2 with subagent pending confirmation stays separate }); + it.each([ + ['completed', 'completed'], + ['failed', 'failed'], + ['cancelled', 'cancelled'], + ] as const)( + 'does NOT merge tool_group with terminal subagent (%s)', + (_label, status) => { + // Terminal task_execution groups must not be absorbed: their + // SubagentScrollbackSummary lands inline as the persistent + // record of the run's outcome, and the compact path can't + // surface it. Mirrors `hasCommittedTerminalSubagent` in + // `ToolGroupMessage.showCompact`. + const subagentResult = { + type: 'task_execution', + subagentName: 'test-agent', + taskDescription: 'test task', + status, + }; + const items: HistoryItem[] = [ + createToolGroup(1, [createTool('c1', 'Shell', ToolCallStatus.Success)]), + createToolGroup(2, [ + createTool( + 'c2', + 'Agent', + status === 'failed' ? ToolCallStatus.Error : ToolCallStatus.Success, + subagentResult, + ), + ]), + createToolGroup(3, [createTool('c3', 'Shell', ToolCallStatus.Success)]), + ]; + const merged = mergeCompactToolGroups(items); + // Three separate groups: terminal subagent stays its own batch + // so SubagentScrollbackSummary renders as a standalone entry. + expect(merged.length).toBe(3); + const ids = merged + .filter(isToolGroup) + .map((g) => g.id) + .sort(); + expect(ids).toEqual([1, 2, 3]); + }, + ); + it('does NOT merge focused executing shell', () => { const items: HistoryItem[] = [ createToolGroup(1, [createTool('c1', 'Shell', ToolCallStatus.Success)]), diff --git a/packages/cli/src/ui/utils/mergeCompactToolGroups.ts b/packages/cli/src/ui/utils/mergeCompactToolGroups.ts index a5540e178cb..26eeb3b9a35 100644 --- a/packages/cli/src/ui/utils/mergeCompactToolGroups.ts +++ b/packages/cli/src/ui/utils/mergeCompactToolGroups.ts @@ -70,6 +70,33 @@ export function isForceExpandGroup( return true; } + // Terminal subagent tool calls must show — the inline + // `SubagentScrollbackSummary` is the persistent record of the + // run's outcome (LiveAgentPanel evicts terminal rows after its + // visibility window). If the group merged into a compact batch, + // the summary would never render and the user would lose the + // committed audit trail. Mirrors the `hasCommittedTerminalSubagent` + // predicate in `ToolGroupMessage.showCompact`. + if ( + tools.some((t) => { + const rd = t.resultDisplay; + if ( + !rd || + typeof rd !== 'object' || + !('type' in rd) || + (rd as { type?: string }).type !== 'task_execution' + ) { + return false; + } + const status = (rd as { status?: string }).status; + return ( + status === 'completed' || status === 'failed' || status === 'cancelled' + ); + }) + ) { + return true; + } + // Active focused shell must be visible if ( embeddedShellFocused && diff --git a/packages/core/src/agents/background-tasks.ts b/packages/core/src/agents/background-tasks.ts index 983bd596ae1..7af879a5661 100644 --- a/packages/core/src/agents/background-tasks.ts +++ b/packages/core/src/agents/background-tasks.ts @@ -188,19 +188,25 @@ interface BackgroundTaskCancelOptions { } /** - * Fires on entry status transitions — register, complete, fail, cancel, - * unregister. Intentionally does NOT fire on `appendActivity` so - * consumers that only care about the roster don't re-render on every - * tool call a background agent makes. + * Fires on entry status transitions: `register`, `complete`, `fail`, + * `cancel`, `finalizeCancelled`, `finalizeCancellationIfPending`, + * `abandon`, `unregisterForeground`, and `reset`. Intentionally does + * NOT fire on `appendActivity` so consumers that only care about the + * roster don't re-render on every tool call a background agent makes. * - * Ordering relative to the registry mutation: - * - register / complete / fail / cancel / finalize: emit AFTER the - * Map mutation but with the entry still present, so a callback - * that re-reads `registry.get(entry.agentId)` sees the entry. - * - unregisterForeground: deletes BEFORE emitting so snapshot-style - * consumers (those that re-pull `getAll()` from inside the - * callback) drop the row. The `entry` arg still carries the - * agent's last live state for log / display consumers. + * Ordering relative to the registry mutation falls into two camps: + * - **Keeps the entry around** (`register` / `complete` / `fail` / + * `cancel` / `finalizeCancelled` / + * `finalizeCancellationIfPending` / `abandon`): emit while the + * entry is still in the Map (the status field has been mutated + * in place to its terminal value), so a callback that re-reads + * `registry.get(entry.agentId)` sees the entry. Snapshot-style + * consumers calling `getAll()` see the new status too. + * - **Removes the entry** (`unregisterForeground`, `reset`): + * deletes from the Map BEFORE emitting so snapshot-style + * consumers drop the row. The `entry` arg carries the agent's + * last live state for log / display consumers; `registry.get` + * and `getAll` already reflect the deletion. */ export type BackgroundStatusChangeCallback = ( entry?: BackgroundTaskEntry, From 4350c2b5b3449bd978fbf17617348a47a45ddf0e Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 8 May 2026 07:29:40 +0800 Subject: [PATCH 7/9] =?UTF-8?q?fix(cli):=20drop=20`'paused'`=20arm=20from?= =?UTF-8?q?=20isPanelOwnedSubagentTool=20=E2=80=94=20not=20in=20AgentResul?= =?UTF-8?q?tDisplay=20union?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI Lint failed with TS2367: the previous round's `isPanelOwnedSubagentTool` checked for `status === 'paused'` but `AgentResultDisplay.status` (the tool-result-side type) only carries `'running' | 'completed' | 'failed' | 'cancelled' | 'background'`. The `'paused'` status lives on the registry-side `BackgroundTaskStatus` union and is only ever surfaced through `LiveAgentPanel` directly, never through a `task_execution` payload. Drop the dead arm and add a comment so a future "let's also check paused here" doesn't get re-introduced. --- .../ui/components/messages/ToolGroupMessage.tsx | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index 1fd427b842f..ba30be5859d 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -58,18 +58,23 @@ function isSubagentToolEntry(tool: IndividualToolCallDisplay): boolean { /** * Predicate: subagent tool entry whose live UI is owned by - * `LiveAgentPanel`. Only running / paused / background entries - * should be hidden during the live phase — terminal entries (the - * subagent already finished while the parent turn is still - * running) are NOT panel-owned: the panel snapshot drops them on + * `LiveAgentPanel`. Only running / background entries should be + * hidden during the live phase — terminal entries (the subagent + * already finished while the parent turn is still running) are NOT + * panel-owned: the panel snapshot drops them on * `unregisterForeground`'s post-delete emit, so the inline path * needs to render `SubagentScrollbackSummary` immediately so the * user keeps a record of the run instead of seeing nothing. + * + * Note: `AgentResultDisplay.status` does NOT carry `'paused'` — that + * status lives on the registry-side `BackgroundTaskStatus` and is + * surfaced through the panel directly, never through a tool-result + * `task_execution` payload. So this predicate has no `paused` arm. */ function isPanelOwnedSubagentTool(tool: IndividualToolCallDisplay): boolean { if (!isSubagentToolEntry(tool)) return false; const status = (tool.resultDisplay as AgentResultDisplay).status; - return status === 'running' || status === 'paused' || status === 'background'; + return status === 'running' || status === 'background'; } /** From 77bb2d1b8945faf8487934896fd3ae6242167c36 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Fri, 8 May 2026 08:58:04 +0800 Subject: [PATCH 8/9] fix(cli): apply panel-ownership filter once before compact-mode decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mixed live groups (running subagent + sibling tool) leaked the panel-owned subagent into `CompactToolGroupDisplay`'s count and `getActiveTool` selection, because `showCompact` returned BEFORE the inline `.map()` filter ran. Compact-mode users would see e.g. `task × 2 Delegate task to subagent` even though LiveAgentPanel already owned the subagent row below the composer. Derive `inlineToolCalls` once via `useMemo` immediately after the existing hook block and use it consistently for the compact summary, sizing math, and the render map. The early-return for "all-entries-panel-owned" collapses into `inlineToolCalls.length === 0` (gated on `isPending` so the legacy empty-input committed-phase snapshot is preserved). Remove the inner `.map()` filter — the upstream derivation already excluded the same entries. JSDoc updates: - `ToolGroupMessageProps.isPending` now describes the real flow (build inlineToolCalls / force-expand / forward to ToolMessage for parity). - `ToolMessageProps.isPending` is documented as forwarded-but-inert (`SubagentExecutionRenderer` doesn't gate on it; the live-phase filter and the unconditional terminal summary do the actual work). Regression test: live mixed group in compact mode → sibling wins active-tool, count collapses to 1, no `× 2` suffix, no subagent description in the header. Addresses Copilot review comments 3205262972 / 3205263020 (doc/code mismatch) and gpt-5.5 critical 3205288299 (compact-mode leak). --- .../messages/ToolGroupMessage.test.tsx | 36 +++++ .../components/messages/ToolGroupMessage.tsx | 131 ++++++++++-------- .../ui/components/messages/ToolMessage.tsx | 13 +- 3 files changed, 113 insertions(+), 67 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx index d9d7417a612..a2ed81781d8 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx @@ -803,5 +803,41 @@ describe('', () => { ); expect(lastFrame() ?? '').toContain('MockSubagent[task-failed]'); }); + + it('compact mode: live mixed group filters panel-owned subagent out of count + active tool', () => { + // Regression: in compact mode, the per-tool live-phase filter + // used to live inside the expanded `.map()`, which `showCompact` + // returned BEFORE. So a mixed live group (running subagent + + // sibling tool) sent the unfiltered list to + // `CompactToolGroupDisplay`, where the running subagent could + // (a) inflate the count to N (`× N` suffix), and (b) win + // `getActiveTool` (Executing beats sibling's Success / Pending), + // overriding the header with the subagent's name. The fix + // derives `inlineToolCalls` ONCE before any compact decision so + // both the count and the active-tool selection see only what + // will actually render inline. + const sibling = createToolCall({ + callId: 'read-1', + name: 'read_file', + description: 'read config.yaml', + status: ToolCallStatus.Success, + }); + const { lastFrame } = renderCompact( + , + ); + const frame = lastFrame() ?? ''; + // Sibling is the only inline survivor → wins active-tool, count + // collapses to 1 (no `× N` suffix). + expect(frame).toContain('read_file'); + expect(frame).not.toMatch(/× 2/); + // Sibling description should appear; subagent description + // should not. + expect(frame).toContain('read config.yaml'); + expect(frame).not.toContain('Delegate task to subagent'); + }); }); }); diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index ba30be5859d..40c57825959 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -102,23 +102,27 @@ interface ToolGroupMessageProps { * `pendingHistoryItems`). False once it commits to Ink's ``. * * Read by the group body to: - * 1. Hide the entire group when every tool entry is a panel-owned - * subagent (`task_execution` without pending approval) — the - * LiveAgentPanel below the composer is the single source of - * truth for in-flight subagents. Group reappears once the parent - * turn commits with the SubagentScrollbackSummary as the - * persistent audit trail. + * 1. Build `inlineToolCalls` — drop panel-owned subagent entries + * (running / background `task_execution` without pending + * approval) so LiveAgentPanel below the composer is the single + * source of truth for in-flight subagents. Mixed groups still + * render their non-subagent siblings; pure-panel-owned groups + * collapse to nothing and the whole bordered container is + * hidden. Terminal subagents (completed / failed / cancelled) + * pass through because `unregisterForeground`'s post-delete + * emit already drops them from the panel snapshot, and the + * inline path must render `SubagentScrollbackSummary` + * immediately so the user keeps a record of the run. * 2. Force-expand a compact group when committed AND carrying a * terminal subagent, so `SubagentScrollbackSummary` actually * lands in the persistent record (CompactToolGroupDisplay is * otherwise unaware of `task_execution` results). - * 3. Filter individual subagent entries out of the inline list - * during the live phase — same panel-ownership rule as #1, but - * applies per-tool when the group is mixed (subagent + sibling - * tools), so siblings keep rendering normally. - * 4. Forward to `ToolMessage` so the same renderer can decide - * whether to emit the inline scrollback summary or stay - * panel-only. + * 3. Forward to `ToolMessage` for parity with sibling renderers + * and possible future gating; the prop is currently inert at + * that layer (the live-phase filter at #1 already prevents + * panel-owned entries from reaching the renderer, and the + * terminal scrollback summary fires in BOTH live and committed + * phases to bridge `unregisterForeground` → parent commit). */ isPending?: boolean; activeShellPtyId?: number | null; @@ -189,6 +193,26 @@ export const ToolGroupMessage: React.FC = ({ [toolCalls], ); + // Live-phase panel-ownership filter applied ONCE so every downstream + // decision (compact summary, sizing, render map) sees the same list. + // Without this, mixed live groups (running subagent + sibling tool) + // could leak the panel-owned subagent into `CompactToolGroupDisplay`'s + // count / active-tool selection, reintroducing the duplicate UI the + // LiveAgentPanel hand-off was designed to prevent. Pending-approval + // subagents pass through (the inline banner / queued marker is the + // only surface that lets users answer the prompt). + const inlineToolCalls = useMemo( + () => + isPending + ? toolCalls.filter( + (tool) => + !isPanelOwnedSubagentTool(tool) || + isAgentWithPendingConfirmation(tool.resultDisplay), + ) + : toolCalls, + [isPending, toolCalls], + ); + // Determine which subagent tools currently have a pending confirmation. // Must be called unconditionally (Rules of Hooks) — before any early return. const subagentsAwaitingApproval = useMemo( @@ -221,11 +245,11 @@ export const ToolGroupMessage: React.FC = ({ // the fallback is now mostly inert; it stays here so a future // re-introduction of inline keyboard surfaces has a focus target. // Note: during the live phase running subagent entries are filtered - // out of the rendered map (`isPending && isSubagentToolEntry && - // !pending`), so this id can point at a hidden tool. That's harmless - // — `isSubagentFocused` is only read inside the map iteration which - // already returned `null` for the hidden entry, so no focus prop - // ever reaches a missing DOM node. + // out of `inlineToolCalls` (LiveAgentPanel owns those rows), so this + // id can point at a tool that won't be rendered. That's harmless — + // `isSubagentFocused` is only consumed inside the `inlineToolCalls` + // map iteration; the hidden entry is never iterated, so no focus + // prop ever reaches a missing DOM node. const runningSubagentCallId = useMemo( () => toolCalls.find((tc) => isRunningAgent(tc.resultDisplay))?.callId ?? null, @@ -235,25 +259,21 @@ export const ToolGroupMessage: React.FC = ({ const keyboardFocusedSubagentCallId = focusedSubagentCallId ?? runningSubagentCallId; - // Decide whether the entire group is panel-owned during the live - // phase: a pure-running-subagent batch with no pending approval - // has nothing inline left to render once the subagent rows - // themselves are filtered out (LiveAgentPanel handles them). - // Hide the whole group so the bordered container / compact - // summary line don't float above the panel as a duplicate. - // Terminal subagents (completed / failed / cancelled) are NOT - // panel-owned — `unregisterForeground`'s post-delete emit drops - // them from the panel snapshot, so the inline path must render - // their `SubagentScrollbackSummary` instead. - const allEntriesPanelOwned = - isPending && - toolCalls.length > 0 && - toolCalls.every( - (tool) => - isPanelOwnedSubagentTool(tool) && - !isAgentWithPendingConfirmation(tool.resultDisplay), - ); - if (allEntriesPanelOwned) { + // Hide the entire group when the live-phase filter leaves nothing + // inline to render — i.e. a pure-running-subagent batch with no + // pending approval. LiveAgentPanel below the composer is the + // single source of truth for those rows; an empty bordered + // container floating above the panel would just be a duplicate + // chrome line. Terminal subagents (completed / failed / cancelled) + // pass through `inlineToolCalls` because `unregisterForeground`'s + // post-delete emit already dropped them from the panel snapshot, + // and the inline path must render `SubagentScrollbackSummary` + // immediately so the user keeps a record of the run. + // (Gate on `isPending` so a degenerate empty `toolCalls=[]` in the + // committed phase still falls through to the legacy empty-border + // snapshot — the suppression is specifically about live-phase + // panel ownership, not about hiding empty inputs in general.) + if (isPending && inlineToolCalls.length === 0) { return null; } @@ -269,7 +289,7 @@ export const ToolGroupMessage: React.FC = ({ // because the panel below the composer is still showing the row. const hasSubagentPendingConfirmation = subagentsAwaitingApproval.length > 0; const hasCommittedTerminalSubagent = - !isPending && toolCalls.some(isTerminalSubagentTool); + !isPending && inlineToolCalls.some(isTerminalSubagentTool); const showCompact = compactMode && !hasConfirmingTool && @@ -282,7 +302,7 @@ export const ToolGroupMessage: React.FC = ({ if (showCompact) { return ( @@ -290,10 +310,10 @@ export const ToolGroupMessage: React.FC = ({ } // Full expanded view - const hasPending = !toolCalls.every( + const hasPending = !inlineToolCalls.every( (t) => t.status === ToolCallStatus.Success, ); - const isShellCommand = toolCalls.some( + const isShellCommand = inlineToolCalls.some( (t) => t.name === SHELL_COMMAND_NAME || t.name === SHELL_NAME, ); const borderColor = @@ -308,12 +328,13 @@ export const ToolGroupMessage: React.FC = ({ const innerWidth = contentWidth - 4; let countToolCallsWithResults = 0; - for (const tool of toolCalls) { + for (const tool of inlineToolCalls) { if (tool.resultDisplay !== undefined && tool.resultDisplay !== '') { countToolCallsWithResults++; } } - const countOneLineToolCalls = toolCalls.length - countToolCallsWithResults; + const countOneLineToolCalls = + inlineToolCalls.length - countToolCallsWithResults; const availableTerminalHeightPerToolMessage = availableTerminalHeight ? Math.max( Math.floor( @@ -391,24 +412,12 @@ export const ToolGroupMessage: React.FC = ({ ); })()} - {toolCalls.map((tool) => { - // Live phase: hide ONLY panel-owned subagent entries (running / - // paused / background) so LiveAgentPanel below the composer - // is the single source of truth for in-flight subagents. - // Terminal subagents (completed / failed / cancelled) ARE - // rendered inline immediately so `SubagentScrollbackSummary` - // lands the moment the subagent finishes — `unregisterForeground` - // already dropped the panel row, leaving inline as the only - // place the user can see the run's outcome until commit. - // Pending-approval entries also pass through so the inline - // banner / queued marker can answer the prompt. - if ( - isPending && - isPanelOwnedSubagentTool(tool) && - !isAgentWithPendingConfirmation(tool.resultDisplay) - ) { - return null; - } + {inlineToolCalls.map((tool) => { + // `inlineToolCalls` already excludes panel-owned subagent + // entries during the live phase (LiveAgentPanel owns those + // rows). Terminal subagents and pending-approval subagents + // pass through the filter and render inline so the + // scrollback summary / approval banner lands. const isConfirming = toolAwaitingApproval?.callId === tool.callId; // A subagent's inline approval prompt should only receive keyboard // focus when (1) there is no direct tool-level confirmation active diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index ce22100d163..cd68df0b74c 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -522,12 +522,13 @@ export interface ToolMessageProps extends IndividualToolCallDisplay { /** * True while the tool message is rendered inside `pendingHistoryItems` * (live area), false (or omitted — undefined is treated as false) - * once committed to ``. Live-area renderers MUST pass - * `true` explicitly; committed renderers can omit it. Used by the - * subagent renderer to gate the scrollback summary so the live - * area stays panel-only — without this gate the summary would - * duplicate the synthesized row LiveAgentPanel already renders - * below the composer the moment a subagent terminates. + * once committed to ``. Forwarded for parity with sibling + * renderers and possible future gating; currently inert inside this + * component. The live-phase filter for panel-owned subagent entries + * lives in `ToolGroupMessage` (the only call site), and the terminal + * `SubagentScrollbackSummary` fires regardless of `isPending` so the + * inline path can bridge the gap between `unregisterForeground`'s + * post-delete panel-snapshot drop and the parent turn committing. */ isPending?: boolean; } From 48d9af8bc1d176c8f6a6c314f5163d3e2a284bb1 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Fri, 8 May 2026 10:39:08 +0800 Subject: [PATCH 9/9] fix(cli): force-expand compact groups on terminal subagent in live phase too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolved comment 3203286936 codified the design intent that `SubagentScrollbackSummary` "fires in BOTH live and committed phases" to bridge `unregisterForeground`'s post-delete panel-snapshot drop and the parent turn committing. Non-compact mode honored that contract (terminal subagents render the summary inline whenever they appear in `inlineToolCalls`), but compact mode still gated `hasCommittedTerminalSubagent` on `!isPending`, so a foreground subagent finishing mid-turn under compact mode produced NOTHING inline until the parent committed — exactly the gap the bridge was meant to close. Drop the `!isPending` arm and rename `hasCommittedTerminalSubagent` → `hasTerminalSubagent`. The force-expand now applies to terminal subagents in either phase; compact-mode users see the same outcome line non-compact users already get. Mirrors `SubagentExecutionRenderer`'s ungated terminal-summary path and `mergeCompactToolGroups.isForceExpandGroup`'s no-isPending-gate preprocessing rule. Tests: - Flip "compact mode: live group with completed subagent stays compact" → "force-expands so the summary bridges the panel-snapshot drop". Update rationale to reflect post-#3921 reality (panel evicts terminal foreground rows immediately). - Add "compact mode: live mixed group with terminal subagent + sibling force-expands and renders both" — covers the bridge in mixed groups. - Update two stale `hasCommittedTerminalSubagent` cross-references in `mergeCompactToolGroups.{ts,test.ts}` comments. --- .../messages/ToolGroupMessage.test.tsx | 46 ++++++++++++++++--- .../components/messages/ToolGroupMessage.tsx | 26 +++++++---- .../ui/utils/mergeCompactToolGroups.test.ts | 2 +- .../src/ui/utils/mergeCompactToolGroups.ts | 2 +- 4 files changed, 58 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx index a2ed81781d8..43cdb447f69 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.test.tsx @@ -647,11 +647,18 @@ describe('', () => { expect(lastFrame() ?? '').not.toContain('MockSubagent[task-running]'); }); - it('compact mode: live group with completed subagent stays compact', () => { - // The subagent terminated mid-turn but the parent is still - // running. Panel shows the synthesized / terminal row; the - // group should NOT yet expand to render the scrollback summary - // (that happens when the parent commits, isPending=false). + it('compact mode: live group with completed subagent force-expands so the summary bridges the panel-snapshot drop', () => { + // The subagent terminated mid-turn while the parent is still + // running. After #3921 swapped the order in + // `unregisterForeground` (delete-then-emit), the panel snapshot + // has already evicted the row by the time we render — so if the + // group stayed compact, the user would see NOTHING for the run + // until the parent commits. Force-expand here so + // `SubagentScrollbackSummary` lands inline immediately and + // bridges the gap. Mirrors `SubagentExecutionRenderer`'s + // ungated terminal-summary path and + // `mergeCompactToolGroups.isForceExpandGroup`'s no-isPending-gate + // committed-history rule. const { lastFrame } = renderCompact( ', () => { isPending={true} />, ); - expect(lastFrame() ?? '').not.toContain('MockSubagent[task-completed]'); + expect(lastFrame() ?? '').toContain('MockSubagent[task-completed]'); }); it('live phase (non-compact): running subagent tool entry is hidden — panel owns the row', () => { @@ -804,6 +811,33 @@ describe('', () => { expect(lastFrame() ?? '').toContain('MockSubagent[task-failed]'); }); + it('compact mode: live mixed group with terminal subagent + sibling force-expands and renders both', () => { + // Terminal subagent (drops from the panel snapshot the moment + // it finishes) + sibling tool, in live + compact. The group + // must force-expand so `SubagentScrollbackSummary` lands inline + // for the subagent, while the sibling continues to render + // through the normal ToolMessage path. Without this, the + // sibling alone would have appeared in `CompactToolGroupDisplay` + // and the subagent's outcome would have stayed invisible until + // parent commit. + const sibling = createToolCall({ + callId: 'edit-1', + name: 'edit_file', + description: 'apply diff to handler.ts', + status: ToolCallStatus.Success, + }); + const { lastFrame } = renderCompact( + , + ); + const frame = lastFrame() ?? ''; + expect(frame).toContain('MockSubagent[task-completed]'); + expect(frame).toContain('MockTool[edit-1]'); + }); + it('compact mode: live mixed group filters panel-owned subagent out of count + active tool', () => { // Regression: in compact mode, the per-tool live-phase filter // used to live inside the expanded `.map()`, which `showCompact` diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index 40c57825959..59f55b993b5 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -280,16 +280,22 @@ export const ToolGroupMessage: React.FC = ({ // Compact mode: entire group → single line summary // Force-expand when: user must interact (Confirming or subagent pending // confirmation), tool errored, shell is focused, or user-initiated. - // Also force-expand when this group has just committed and contains - // a terminal subagent — `CompactToolGroupDisplay` doesn't know about - // `task_execution` results, so the compact path would skip - // `SubagentScrollbackSummary` and the user would lose the persistent - // record promised by the LiveAgentPanel → committed-summary handoff. - // Stay compact while the parent turn is still live (`isPending`), - // because the panel below the composer is still showing the row. + // Also force-expand when this group carries a terminal subagent — + // `CompactToolGroupDisplay` doesn't know about `task_execution` + // results, so the compact path would skip `SubagentScrollbackSummary` + // entirely. Applies in BOTH live and committed phases: + // - committed phase: the summary is the persistent audit trail. + // - live phase: `unregisterForeground`'s post-delete emit has + // already evicted the panel snapshot row by the time a foreground + // subagent reaches a terminal status, so the inline summary is + // the only surface that carries the run's outcome until the + // parent commits. Mirrors the renderer-side decision in + // `SubagentExecutionRenderer` (terminal summary fires regardless + // of `isPending`) and the preprocessor in + // `mergeCompactToolGroups.isForceExpandGroup` (no `isPending` + // gate either). const hasSubagentPendingConfirmation = subagentsAwaitingApproval.length > 0; - const hasCommittedTerminalSubagent = - !isPending && inlineToolCalls.some(isTerminalSubagentTool); + const hasTerminalSubagent = inlineToolCalls.some(isTerminalSubagentTool); const showCompact = compactMode && !hasConfirmingTool && @@ -297,7 +303,7 @@ export const ToolGroupMessage: React.FC = ({ !hasErrorTool && !isEmbeddedShellFocused && !isUserInitiated && - !hasCommittedTerminalSubagent; + !hasTerminalSubagent; if (showCompact) { return ( diff --git a/packages/cli/src/ui/utils/mergeCompactToolGroups.test.ts b/packages/cli/src/ui/utils/mergeCompactToolGroups.test.ts index a131b5a67e5..52444c9547a 100644 --- a/packages/cli/src/ui/utils/mergeCompactToolGroups.test.ts +++ b/packages/cli/src/ui/utils/mergeCompactToolGroups.test.ts @@ -213,7 +213,7 @@ describe('mergeCompactToolGroups', () => { // Terminal task_execution groups must not be absorbed: their // SubagentScrollbackSummary lands inline as the persistent // record of the run's outcome, and the compact path can't - // surface it. Mirrors `hasCommittedTerminalSubagent` in + // surface it. Mirrors `hasTerminalSubagent` in // `ToolGroupMessage.showCompact`. const subagentResult = { type: 'task_execution', diff --git a/packages/cli/src/ui/utils/mergeCompactToolGroups.ts b/packages/cli/src/ui/utils/mergeCompactToolGroups.ts index 26eeb3b9a35..0dcf7a3fe20 100644 --- a/packages/cli/src/ui/utils/mergeCompactToolGroups.ts +++ b/packages/cli/src/ui/utils/mergeCompactToolGroups.ts @@ -75,7 +75,7 @@ export function isForceExpandGroup( // run's outcome (LiveAgentPanel evicts terminal rows after its // visibility window). If the group merged into a compact batch, // the summary would never render and the user would lose the - // committed audit trail. Mirrors the `hasCommittedTerminalSubagent` + // committed audit trail. Mirrors the `hasTerminalSubagent` // predicate in `ToolGroupMessage.showCompact`. if ( tools.some((t) => {