diff --git a/src/cli/App.tsx b/src/cli/App.tsx index ba40d14..03d7ce1 100644 --- a/src/cli/App.tsx +++ b/src/cli/App.tsx @@ -212,6 +212,10 @@ import { interruptActiveSubagents, subscribe as subscribeToSubagents, } from "./helpers/subagentState"; +import { + flushEligibleLinesBeforeReentry, + shouldClearCompletedSubagentsOnTurnStart, +} from "./helpers/subagentTurnStart"; import { extractTaskNotificationsForDisplay } from "./helpers/taskNotifications"; import { getRandomPastTenseVerb, @@ -2960,9 +2964,13 @@ export default function App({ // Reset interrupted flag since we're starting a fresh stream buffersRef.current.interrupted = false; - // Clear completed subagents from the UI when starting a new turn, - // but only if no subagents are still running. - if (!hasActiveSubagents()) { + // Clear completed subagents only on true new turns. + if ( + shouldClearCompletedSubagentsOnTurnStart( + allowReentry, + hasActiveSubagents(), + ) + ) { clearCompletedSubagents(); } @@ -8773,6 +8781,13 @@ ${SYSTEM_REMINDER_CLOSE} } else if (hadNotifications) { refreshDerived(); } + // Flush finished items synchronously before reentry. This avoids a + // race where deferred non-Task commits delay Task grouping while the + // reentry path continues. + flushEligibleLinesBeforeReentry( + commitEligibleLines, + buffersRef.current, + ); toolResultsInFlightRef.current = true; await processConversation(input, { allowReentry: true }); toolResultsInFlightRef.current = false; @@ -8808,6 +8823,7 @@ ${SYSTEM_REMINDER_CLOSE} syncTrajectoryElapsedBase, closeTrajectorySegment, openTrajectorySegment, + commitEligibleLines, ], ); diff --git a/src/cli/helpers/subagentTurnStart.ts b/src/cli/helpers/subagentTurnStart.ts new file mode 100644 index 0000000..162aefc --- /dev/null +++ b/src/cli/helpers/subagentTurnStart.ts @@ -0,0 +1,27 @@ +import type { Buffers } from "./accumulator.js"; + +/** + * Completed subagents should only be cleared on true new turns. + * During allowReentry (post-approval continuation), completed subagents + * must remain available so deferred Task grouping can still resolve. + */ +export function shouldClearCompletedSubagentsOnTurnStart( + allowReentry: boolean, + hasActiveSubagents: boolean, +): boolean { + return !allowReentry && !hasActiveSubagents; +} + +/** + * Flush static-eligible lines before reentry so Task grouping is not delayed + * by deferred non-Task tool commits. + */ +export function flushEligibleLinesBeforeReentry( + commitEligibleLines: ( + b: Buffers, + opts?: { deferToolCalls?: boolean }, + ) => void, + buffers: Buffers, +): void { + commitEligibleLines(buffers, { deferToolCalls: false }); +} diff --git a/src/tests/cli/subagentTurnStart.test.ts b/src/tests/cli/subagentTurnStart.test.ts new file mode 100644 index 0000000..8eba994 --- /dev/null +++ b/src/tests/cli/subagentTurnStart.test.ts @@ -0,0 +1,241 @@ +import { beforeEach, describe, expect, test } from "bun:test"; +import { + collectFinishedTaskToolCalls, + hasInProgressTaskToolCalls, +} from "../../cli/helpers/subagentAggregation"; +import { + clearAllSubagents, + clearCompletedSubagents, + completeSubagent, + getSubagentByToolCallId, + registerSubagent, +} from "../../cli/helpers/subagentState"; +import { + flushEligibleLinesBeforeReentry, + shouldClearCompletedSubagentsOnTurnStart, +} from "../../cli/helpers/subagentTurnStart"; + +type MinimalToolCallLine = { + kind: "tool_call"; + id: string; + name: string; + phase: "finished"; + toolCallId: string; + resultOk: boolean; +}; + +function simulateCommitPass( + order: string[], + byId: Map, + emitted: Set, + deferredCommits: Map, + now: number, + deferToolCalls: boolean, +): { + blockedByDeferred: boolean; + grouped: boolean; + taskFallbackCommitted: boolean; +} { + const hasInProgress = hasInProgressTaskToolCalls( + order, + byId as unknown as Map, + emitted, + ); + const finishedTaskToolCalls = collectFinishedTaskToolCalls( + order, + byId as unknown as Map, + emitted, + hasInProgress, + ); + + let blockedByDeferred = false; + let taskFallbackCommitted = false; + const TASK_DEFER_MS = 50; + + for (const id of order) { + if (emitted.has(id)) continue; + const ln = byId.get(id); + if (!ln) continue; + + if (ln.name === "Task" || ln.name === "task") { + const hasSubagentData = finishedTaskToolCalls.some( + (tc) => tc.lineId === id, + ); + if (!hasSubagentData) { + emitted.add(id); + taskFallbackCommitted = true; + } + continue; + } + + if (deferToolCalls) { + const commitAt = deferredCommits.get(id); + if (commitAt === undefined) { + deferredCommits.set(id, now + TASK_DEFER_MS); + blockedByDeferred = true; + break; + } + if (commitAt > now) { + blockedByDeferred = true; + break; + } + deferredCommits.delete(id); + } + + emitted.add(id); + } + + const grouped = !blockedByDeferred && finishedTaskToolCalls.length > 0; + if (grouped) { + for (const tc of finishedTaskToolCalls) { + emitted.add(tc.lineId); + } + } + + return { blockedByDeferred, grouped, taskFallbackCommitted }; +} + +describe("subagent turn-start reentry safeguards", () => { + beforeEach(() => { + clearAllSubagents(); + }); + + test("shouldClearCompletedSubagentsOnTurnStart preserves completed agents during allowReentry", () => { + expect(shouldClearCompletedSubagentsOnTurnStart(true, false)).toBe(false); + expect(shouldClearCompletedSubagentsOnTurnStart(false, false)).toBe(true); + expect(shouldClearCompletedSubagentsOnTurnStart(false, true)).toBe(false); + }); + + test("deferred first pass + explicit pre-reentry flush preserves Task grouping", () => { + registerSubagent("sub-1", "explore", "Find symbols", "tc-task", false); + completeSubagent("sub-1", { success: true, totalTokens: 42 }); + + const order = ["line-read", "line-task"]; + const byId = new Map([ + [ + "line-read", + { + kind: "tool_call", + id: "line-read", + name: "Read", + phase: "finished", + toolCallId: "tc-read", + resultOk: true, + }, + ], + [ + "line-task", + { + kind: "tool_call", + id: "line-task", + name: "Task", + phase: "finished", + toolCallId: "tc-task", + resultOk: true, + }, + ], + ]); + const emitted = new Set(); + const deferred = new Map(); + + // First commit pass gets blocked by deferred non-Task tool call. + const first = simulateCommitPass( + order, + byId, + emitted, + deferred, + 1_000, + true, + ); + expect(first.blockedByDeferred).toBe(true); + expect(first.grouped).toBe(false); + expect(getSubagentByToolCallId("tc-task")).toBeDefined(); + + // During reentry we should preserve completed subagents. + if (shouldClearCompletedSubagentsOnTurnStart(true, false)) { + clearCompletedSubagents(); + } + expect(getSubagentByToolCallId("tc-task")).toBeDefined(); + + // Explicit non-deferred flush before reentry should request deferToolCalls=false. + let flushCalled = false; + let capturedOpts: { deferToolCalls?: boolean } | undefined; + flushEligibleLinesBeforeReentry((_b, opts) => { + flushCalled = true; + capturedOpts = opts; + }, {} as never); + expect(flushCalled).toBe(true); + expect(capturedOpts?.deferToolCalls).toBe(false); + + // And with defer disabled, Task grouping should happen instead of fallback. + const flushed = simulateCommitPass( + order, + byId, + emitted, + deferred, + 1_001, + false, + ); + expect(flushed.blockedByDeferred).toBe(false); + expect(flushed.grouped).toBe(true); + expect(flushed.taskFallbackCommitted).toBe(false); + }); + + test("clearing completed agents before second pass reproduces Task fallback", () => { + registerSubagent("sub-1", "explore", "Find symbols", "tc-task", false); + completeSubagent("sub-1", { success: true }); + + const order = ["line-read", "line-task"]; + const byId = new Map([ + [ + "line-read", + { + kind: "tool_call", + id: "line-read", + name: "Read", + phase: "finished", + toolCallId: "tc-read", + resultOk: true, + }, + ], + [ + "line-task", + { + kind: "tool_call", + id: "line-task", + name: "Task", + phase: "finished", + toolCallId: "tc-task", + resultOk: true, + }, + ], + ]); + const emitted = new Set(); + const deferred = new Map(); + + const first = simulateCommitPass( + order, + byId, + emitted, + deferred, + 2_000, + true, + ); + expect(first.blockedByDeferred).toBe(true); + + // This mirrors the old problematic behavior. + clearCompletedSubagents(); + expect(getSubagentByToolCallId("tc-task")).toBeUndefined(); + + const second = simulateCommitPass( + order, + byId, + emitted, + deferred, + 2_100, + false, + ); + expect(second.grouped).toBe(false); + expect(second.taskFallbackCommitted).toBe(true); + }); +});