From 49e42afd8343728d53f1169d2eac8973a806d6c7 Mon Sep 17 00:00:00 2001 From: xuyunfang Date: Fri, 9 Oct 2026 17:31:01 +0800 Subject: [PATCH] fix(subagent): return the chain's final result and mark unstarted steps A blocking chain returned records[0] as the tool content, so the parent model only saw the first step's output, and a failed or aborted chain still read as the first step's success. Unstarted steps were seeded as "running" and stayed that way after the chain stopped. - Chain content now carries a per-step status line plus the last step's output on success, or the stopping step's error and the last completed output on failure/abort. - Add "queued" for steps not yet dispatched (later chain steps, parallel tasks behind maxConcurrency) and "skipped" for chain steps after a stop. - Partial updates keep mode "chain" instead of flipping to "single". - TUI headings and widget counts handle queued/skipped; the chain heading no longer reads results[0]. - Background lanes treat queued records as still running. Fixes #220 --- .../src/features/step-subagent.ts | 7 +- .../src/features/subagent/execute.ts | 54 ++++- .../src/features/subagent/lane-lifecycle.ts | 2 +- .../src/features/subagent/rendering.ts | 32 ++- .../test/subagent-chain-result.test.ts | 220 ++++++++++++++++++ 5 files changed, 305 insertions(+), 10 deletions(-) create mode 100644 packages/coding-agent/test/subagent-chain-result.test.ts diff --git a/packages/coding-agent/src/features/step-subagent.ts b/packages/coding-agent/src/features/step-subagent.ts index a634f948..bd2d5c5e 100644 --- a/packages/coding-agent/src/features/step-subagent.ts +++ b/packages/coding-agent/src/features/step-subagent.ts @@ -156,7 +156,12 @@ export interface StepSubagentResultRecord extends StepSubagentRunResult { agent: string; agentSource: StepAgentConfig["source"] | "unknown"; task: string; - status: "running" | "completed" | "failed" | "aborted"; + /** + * `queued`: not started yet (a later chain step, or a parallel task waiting + * on maxConcurrency). `skipped`: a chain step that never started because an + * earlier step did not complete. + */ + status: "queued" | "running" | "completed" | "failed" | "aborted" | "skipped"; step?: number; worktreePath?: string; worktreeBranch?: string; diff --git a/packages/coding-agent/src/features/subagent/execute.ts b/packages/coding-agent/src/features/subagent/execute.ts index 960004e2..a1b578ea 100644 --- a/packages/coding-agent/src/features/subagent/execute.ts +++ b/packages/coding-agent/src/features/subagent/execute.ts @@ -81,16 +81,47 @@ function makeEmptyResult(agent: string, task: string): StepSubagentResultRecord agent, agentSource: "unknown", task, - status: "running", + status: "queued", exitCode: -1, messages: [], stderr: "", usage: emptyUsage(), - startedAt: Date.now(), updatedAt: Date.now(), }; } +function makeSkippedResult(record: StepSubagentResultRecord): StepSubagentResultRecord { + return { ...record, status: "skipped", updatedAt: Date.now() }; +} + +/** + * The parent model only sees `content`, never `details`, so a chain's text must + * carry both the outcome and the output that matters: the last completed step + * on success, the stopping step's error otherwise. Earlier step bodies stay in + * `details.results` instead of piling into the parent's context. + */ +function chainResultText(records: readonly StepSubagentResultRecord[]): string { + const steps = records.map((record, index) => `${index + 1}. ${record.agent}: ${record.status}`).join("\n"); + const stopped = records.findIndex((record) => record.status !== "completed"); + if (stopped === -1) { + const last = records.at(-1); + if (!last) return "(no output)"; + return `Chain: ${records.length}/${records.length} steps completed\n${steps}\n\n### Final output (step ${records.length}, ${last.agent})\n\n${resultText(last)}`; + } + const failed = records[stopped]; + const sections = [ + `Chain stopped at step ${stopped + 1}/${records.length} (${failed.agent}: ${failed.status}); the chain did not complete.\n${steps}`, + `### Error (step ${stopped + 1}, ${failed.agent})\n\n${resultText(failed)}`, + ]; + const previous = stopped > 0 ? records[stopped - 1] : undefined; + if (previous) { + sections.push( + `### Last completed output (step ${stopped}, ${previous.agent})\n\n${truncateText(resultText(previous), 50_000)}`, + ); + } + return sections.join("\n\n"); +} + function resultDetailsText(details: StepSubagentDetails): string { return details.results .map((result) => { @@ -320,6 +351,16 @@ export async function executeSubagent( return failed; } reportCreated(agent, index); + // Flip the queued placeholder before spawning, so the step reads as running + // (with a clock) from the moment it is dispatched, not from its first event. + records[index] = { + ...records[index], + agentSource: agent.source, + status: "running", + startedAt: Date.now(), + updatedAt: Date.now(), + }; + emit(mode); const baseCwd = path.resolve(ctx.cwd, task.cwd ?? "."); let worktree: StepWorktreeLease | undefined; let childCwd = baseCwd; @@ -334,7 +375,7 @@ export async function executeSubagent( worktreePath: worktree?.path, worktreeBranch: worktree?.branch, }); - emit(parallel ? "parallel" : "single"); + emit(mode); }; const child = await options.runner({ agent, @@ -394,7 +435,11 @@ export async function executeSubagent( }; const record = await runOne(task, index); previous = finalOutput(record.messages) || resultText(record); - if (record.status !== "completed") break; + if (record.status !== "completed") { + for (let rest = index + 1; rest < chain.length; rest++) records[rest] = makeSkippedResult(records[rest]); + emit(mode); + break; + } } } else await runOne(tasks[0], 0); const finalDetails = buildDetails(mode, discovery, scope, records); @@ -408,6 +453,7 @@ export async function executeSubagent( `Parallel: ${success}/${records.length} succeeded\n\n${summaries.join("\n\n---\n\n")}`, ); } + if (mode === "chain") return makeToolResult(finalDetails, chainResultText(records)); const record = records[0]; return makeToolResult(finalDetails, record ? resultText(record) : "(no output)"); } diff --git a/packages/coding-agent/src/features/subagent/lane-lifecycle.ts b/packages/coding-agent/src/features/subagent/lane-lifecycle.ts index fec7876f..08b06bd3 100644 --- a/packages/coding-agent/src/features/subagent/lane-lifecycle.ts +++ b/packages/coding-agent/src/features/subagent/lane-lifecycle.ts @@ -81,7 +81,7 @@ function getLiveLaneSessions(sessionId: string | undefined): StepSubagentRpcSess } function laneStatusFromDetails(details: StepSubagentDetails): BackgroundAgentLane["status"] { - if (details.results.some((record) => record.status === "running")) { + if (details.results.some((record) => record.status === "running" || record.status === "queued")) { return "running"; } if (details.results.some((record) => record.status === "failed")) { diff --git a/packages/coding-agent/src/features/subagent/rendering.ts b/packages/coding-agent/src/features/subagent/rendering.ts index 1f3d492b..f7bbedb4 100644 --- a/packages/coding-agent/src/features/subagent/rendering.ts +++ b/packages/coding-agent/src/features/subagent/rendering.ts @@ -53,14 +53,31 @@ function formatElapsed(record: StepSubagentResultRecord): string { function statusIcon(status: StepSubagentResultRecord["status"], theme: Theme): string { if (status === "running") return theme.fg("warning", "~"); if (status === "completed") return theme.fg("success", "\u2713"); + if (status === "queued") return theme.fg("dim", "\u00b7"); + if (status === "skipped") return theme.fg("dim", "-"); return theme.fg("error", "x"); } +function chainHeading(records: readonly StepSubagentResultRecord[]): string { + const index = records.findIndex( + (record) => record.status !== "completed" && record.status !== "queued" && record.status !== "skipped", + ); + if (index !== -1) return `${records[index].status} at step ${index + 1}/${records.length}`; + const completed = records.filter((record) => record.status === "completed").length; + return completed === records.length + ? `${completed}/${records.length} steps completed` + : `queued at step ${completed + 1}/${records.length}`; +} + function renderRecordSummary(record: StepSubagentResultRecord, theme: Theme): string { const output = record.activeText || finalOutput(record.messages) || - (record.status === "running" ? "(running...)" : resultText(record)); + (record.status === "running" + ? "(running...)" + : record.status === "queued" || record.status === "skipped" + ? `(${record.status})` + : resultText(record)); const lines = output.split(/\r?\n/u).filter((line) => line.trim().length > 0); const preview = lines.slice(-COLLAPSED_OUTPUT_LINES).join("\n"); const omitted = Math.max(0, lines.length - COLLAPSED_OUTPUT_LINES); @@ -201,10 +218,14 @@ export class SubagentListWidget implements Component { const theme = this.theme; const running = records.filter((record) => record.status === "running").length; const completed = records.filter((record) => record.status === "completed").length; - const failed = records.length - running - completed; + const queued = records.filter((record) => record.status === "queued").length; + const skipped = records.filter((record) => record.status === "skipped").length; + const failed = records.length - running - completed - queued - skipped; const summary = [`${completed}/${records.length} complete`]; if (running > 0) summary.push(`${running} running`); + if (queued > 0) summary.push(`${queued} queued`); if (failed > 0) summary.push(`${failed} failed`); + if (skipped > 0) summary.push(`${skipped} skipped`); const header = ` ${theme.fg("toolTitle", theme.bold("subagent"))} ${theme.fg("accent", summary.join(", "))}`; const lines = [visibleWidth(header) > width ? truncateToWidth(header, width, "\u2026") : header]; @@ -255,11 +276,14 @@ export function renderSubagentResult( return new Text(text?.type === "text" ? text.text : "(no output)", 0, 0); } const running = details.results.filter((record) => record.status === "running").length; + const queued = details.results.filter((record) => record.status === "queued").length; const completed = details.results.filter((record) => record.status === "completed").length; const heading = details.mode === "parallel" - ? `${completed}/${details.results.length} complete${running > 0 ? `, ${running} running` : ""}` - : (details.results[0]?.status ?? "done"); + ? `${completed}/${details.results.length} complete${running > 0 ? `, ${running} running` : ""}${queued > 0 ? `, ${queued} queued` : ""}` + : details.mode === "chain" + ? chainHeading(details.results) + : (details.results[0]?.status ?? "done"); if (options.expanded) { const container = new Container(); container.addChild(new Text(`${theme.bold("agent")} ${theme.fg("accent", heading)}`, 0, 0)); diff --git a/packages/coding-agent/test/subagent-chain-result.test.ts b/packages/coding-agent/test/subagent-chain-result.test.ts new file mode 100644 index 00000000..66a9158c --- /dev/null +++ b/packages/coding-agent/test/subagent-chain-result.test.ts @@ -0,0 +1,220 @@ +import type { AgentToolResult } from "@step-harness/agent-core"; +import { expect, test } from "vitest"; +import type { ExtensionContext } from "../src/core/extensions/types.ts"; +import type { + StepSubagentDetails, + StepSubagentRunInput, + StepSubagentRunResult, + SubagentParams, +} from "../src/features/step-subagent.ts"; +import { executeSubagent } from "../src/features/subagent/execute.ts"; + +// Deterministic runner outcome keyed by the step's task text. +type Outcome = { text: string; exitCode?: number; stopReason?: string; errorMessage?: string }; + +function runResult(outcome: Outcome): StepSubagentRunResult { + const exitCode = outcome.exitCode ?? 0; + return { + messages: [ + { + role: "assistant", + content: [{ type: "text", text: outcome.text }], + api: "anthropic-messages", + provider: "step", + model: "step-3.7-flash", + usage: { + input: 1, + output: 2, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 3, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + stopReason: outcome.stopReason ?? "stop", + timestamp: Date.now(), + }, + ], + stderr: "", + exitCode, + stopReason: outcome.stopReason, + errorMessage: outcome.errorMessage, + usage: { input: 1, output: 2, cacheRead: 0, cacheWrite: 0, cost: 0, contextTokens: 3, turns: 1 }, + startedAt: Date.now(), + updatedAt: Date.now(), + } as unknown as StepSubagentRunResult; +} + +const ctx = { + mode: "print", + hasUI: false, + cwd: "/tmp", + model: undefined, + thinkingLevel: "high", + isProjectTrusted: () => true, +} as unknown as ExtensionContext; + +async function run( + params: SubagentParams, + respond: (input: StepSubagentRunInput, index: number) => Outcome, + options: { maxConcurrency?: number } = {}, +): Promise<{ + result: AgentToolResult; + tasks: string[]; + partialModes: StepSubagentDetails["mode"][]; + partialStatuses: string[][]; +}> { + const tasks: string[] = []; + const partialModes: StepSubagentDetails["mode"][] = []; + const partialStatuses: string[][] = []; + const result = await executeSubagent( + params, + undefined, + (partial) => { + if (!partial.details) return; + partialModes.push(partial.details.mode); + partialStatuses.push(partial.details.results.map((record) => record.status)); + }, + ctx, + { + agentDir: "/tmp/step-agent-test", + configDirName: ".step", + includeBuiltinAgents: true, + maxParallelTasks: 8, + maxConcurrency: options.maxConcurrency ?? 4, + worktreeManager: { allocate: async () => Promise.reject(new Error("no worktrees in this test")) }, + runner: async (input) => { + const index = tasks.length; + tasks.push(input.task); + const partial = runResult({ text: "" }); + input.onUpdate?.({ ...partial, exitCode: -1, messages: [] }); + return runResult(respond(input, index)); + }, + }, + ); + return { result, tasks, partialModes, partialStatuses }; +} + +function contentText(result: AgentToolResult): string { + return result.content.map((block) => (block.type === "text" ? block.text : "")).join(""); +} + +const threeSteps: SubagentParams = { + chain: [ + { agent: "explore", task: "return STEP_1_SOURCE" }, + { agent: "explore", task: "summarize {previous} as STEP_2_SUMMARY" }, + { agent: "general", task: "from {previous} return STEP_3_FINAL" }, + ], +} as SubagentParams; + +test("a completed chain returns the last step's output with a per-step summary", async () => { + const outputs = ["STEP_1_SOURCE", "STEP_2_SUMMARY", "STEP_3_FINAL"]; + const { result, tasks, partialModes } = await run(threeSteps, (_input, index) => ({ text: outputs[index] })); + + expect(tasks[1]).toBe("summarize STEP_1_SOURCE as STEP_2_SUMMARY"); + expect(tasks[2]).toBe("from STEP_2_SUMMARY return STEP_3_FINAL"); + const text = contentText(result); + expect(text).toContain("3/3 steps completed"); + expect(text).toContain("STEP_3_FINAL"); + // Earlier bodies stay in details, not in the parent's context. + expect(text).not.toContain("STEP_1_SOURCE"); + expect(result.details?.mode).toBe("chain"); + expect(result.details?.results.map((record) => record.status)).toEqual(["completed", "completed", "completed"]); + expect(new Set(partialModes)).toEqual(new Set(["chain"])); +}); + +test("a failed step surfaces its error in content and marks later steps skipped", async () => { + const { result, tasks } = await run(threeSteps, (_input, index) => + index === 0 + ? { text: "STEP_1_SOURCE" } + : { text: "", exitCode: 2, stopReason: "error", errorMessage: "STEP_2_BROKE" }, + ); + + expect(tasks).toHaveLength(2); + const text = contentText(result); + expect(text).toContain("Chain stopped at step 2/3"); + expect(text).toContain("did not complete"); + expect(text).toContain("STEP_2_BROKE"); + expect(text).toContain("3. general: skipped"); + expect(text).toContain("STEP_1_SOURCE"); + const records = result.details?.results ?? []; + expect(records.map((record) => record.status)).toEqual(["completed", "failed", "skipped"]); + expect(records[2].startedAt).toBeUndefined(); +}); + +test("an aborted step is reported as aborted, not as success", async () => { + const { result } = await run(threeSteps, (_input, index) => + index === 0 ? { text: "STEP_1_SOURCE" } : { text: "", exitCode: 1, stopReason: "aborted" }, + ); + + expect(contentText(result)).toContain("Chain stopped at step 2/3 (explore: aborted)"); + expect(result.details?.results.map((record) => record.status)).toEqual(["completed", "aborted", "skipped"]); +}); + +test("a failing first step skips the rest", async () => { + const { result, tasks } = await run(threeSteps, () => ({ + text: "", + exitCode: 1, + stopReason: "error", + errorMessage: "STEP_1_BROKE", + })); + + expect(tasks).toHaveLength(1); + const text = contentText(result); + expect(text).toContain("Chain stopped at step 1/3"); + expect(text).toContain("STEP_1_BROKE"); + expect(text).not.toContain("Last completed output"); + expect(result.details?.results.map((record) => record.status)).toEqual(["failed", "skipped", "skipped"]); +}); + +test("single mode still returns the step's own output", async () => { + const { result } = await run({ agent: "general", task: "solo" } as SubagentParams, () => ({ text: "SOLO_OUT" })); + + expect(contentText(result)).toBe("SOLO_OUT"); + expect(result.details?.mode).toBe("single"); +}); + +test("parallel mode still aggregates every task", async () => { + const { result } = await run( + { + tasks: [ + { agent: "explore", task: "a" }, + { agent: "explore", task: "b" }, + ], + } as SubagentParams, + (input) => ({ text: `OUT_${input.task}` }), + ); + + const text = contentText(result); + expect(text).toContain("Parallel: 2/2 succeeded"); + expect(text).toContain("OUT_a"); + expect(text).toContain("OUT_b"); +}); + +test("chain steps wait as queued, not running, until dispatched", async () => { + const { partialStatuses } = await run(threeSteps, (_input, index) => ({ text: `OUT_${index}` })); + + expect(partialStatuses[0]).toEqual(["queued", "queued", "queued"]); + expect(partialStatuses).toContainEqual(["running", "queued", "queued"]); + expect(partialStatuses).toContainEqual(["completed", "running", "queued"]); + // A step that has not been dispatched never reads as running. + for (const statuses of partialStatuses) { + const running = statuses.indexOf("running"); + if (running !== -1) expect(statuses.slice(running + 1).every((status) => status === "queued")).toBe(true); + } +}); + +test("parallel tasks beyond maxConcurrency wait as queued", async () => { + const { partialStatuses } = await run( + { + tasks: [ + { agent: "explore", task: "a" }, + { agent: "explore", task: "b" }, + ], + } as SubagentParams, + (input) => ({ text: `OUT_${input.task}` }), + { maxConcurrency: 1 }, + ); + + expect(partialStatuses).toContainEqual(["running", "queued"]); + expect(partialStatuses.at(-1)).toEqual(["completed", "completed"]); +});