diff --git a/packages/coding-agent/src/features/step-subagent.ts b/packages/coding-agent/src/features/step-subagent.ts index 9a76293..77efd10 100644 --- a/packages/coding-agent/src/features/step-subagent.ts +++ b/packages/coding-agent/src/features/step-subagent.ts @@ -220,7 +220,7 @@ const StepAgentScopeSchema = StringEnum(["user", "project", "both"] as const, { const StepSubscribeSchema = StringEnum(["final", "progress", "none"] as const, { description: - 'Notification level for background lanes. "final" (default) sends one completion notification, "progress" adds throttled progress updates, "none" is fire-and-forget.', + 'Notification level for background lanes. "final" (default) sends one completion notification, "progress" adds throttled progress updates, "none" is fire-and-forget. Except with "none", completion, failure, and needs-input notifications wake you automatically, so never sleep or poll to wait for a lane.', default: "final", }); @@ -549,9 +549,9 @@ export function createStepSubagentExtension(options: StepSubagentExtensionOption const updateLaneWidget = (lane: BackgroundAgentLane): void => { if (!lane.ctx.hasUI) return; shownLanes.add(lane.id); - const batch = [...shownLanes] - .map((id) => lanes.get(id)) - .filter((entry): entry is BackgroundAgentLane => entry !== undefined); + // `lanes` iterates in creation order; `shownLanes` in first-update order, + // which depends on which child streams first and would shuffle the rows. + const batch = [...lanes.values()].filter((entry) => shownLanes.has(entry.id)); try { if (batch.every((entry) => entry.status !== "running")) { shownLanes.clear(); @@ -635,7 +635,19 @@ export function createStepSubagentExtension(options: StepSubagentExtensionOption const monitorHint = lane.subscribe === "none" ? "Fire-and-forget lane: no notifications will be sent." - : `Lane events arrive automatically as messages (subscribe: ${lane.subscribe}).`; + : [ + // Without this the parent, told to "wait for the lanes", has no + // wait primitive and falls back to `sleep` loops: the lane result + // then sits behind a sleep of up to minutes, and each progress + // notice landing mid-sleep keeps the loop going. + "Do not wait for it with sleep or polling: end your turn, or carry on with other work.", + "You are woken automatically with an when the lane finishes, fails, or needs input.", + lane.subscribe === "progress" + ? "Progress messages are informational; they need no reply." + : "", + ] + .filter(Boolean) + .join(" "); return makeToolResult( details, `Started background agent ${lane.id}${lane.alias ? ` (${lane.alias})` : ""}. ${monitorHint}`, diff --git a/packages/coding-agent/src/features/subagent/lane-events.ts b/packages/coding-agent/src/features/subagent/lane-events.ts index abc807f..21f8454 100644 --- a/packages/coding-agent/src/features/subagent/lane-events.ts +++ b/packages/coding-agent/src/features/subagent/lane-events.ts @@ -39,6 +39,14 @@ export interface AgentNotificationDetails { detail?: string; } +/** + * Cap on the lane output a final notification carries. The notification is the + * parent's only copy of the result (agent_send can reply or stop, not fetch), + * so it matches what a blocking call returns per task instead of a short + * preview the parent would have to dig back out of the child's session file. + */ +const LANE_OUTPUT_MAX_CHARS = 50_000; + /** Minimum interval between background_progress notifications per lane. */ const PROGRESS_NOTIFY_INTERVAL_MS = 15_000; @@ -103,11 +111,18 @@ export function notifyLaneEvent( // A lane usually settles while the parent sits idle, waiting on it. A // steer alone only appends the message then, so the model never reads the // result until the user types again. Terminal and needs-input events wake - // the parent; progress and restart notices ride along with the next turn - // instead of waking it every 15s per lane. They leave triggerTurn unset - // rather than false: an explicit false would defer them to the end of a - // running turn instead of steering into it. - WAKING_EVENTS.has(event) ? { deliverAs: "steer", triggerTurn: true } : { deliverAs: "steer" }, + // the parent with a steer. + // + // Progress and restart notices must stay out of the steering queue. It + // drains one message per turn by default ("one-at-a-time"), and progress + // arrives every 15s per lane: while the parent sat in a long tool call + // (a 3-minute sleep, say) dozens queued up, the parent read one per turn, + // and the lanes' completion notices waited behind that backlog for over + // half an hour. A steer also forces another model call after the parent + // meant to stop. triggerTurn: false instead lands them as context: + // appended at once while idle, or batched in at the end of the running + // turn, which is the same boundary a steer would be injected at. + WAKING_EVENTS.has(event) ? { deliverAs: "steer", triggerTurn: true } : { triggerTurn: false }, ); } @@ -145,7 +160,7 @@ export function notifyLaneFinal(pi: ExtensionAPI, lane: BackgroundAgentLane): vo .map(({ label, cause }) => `- ${label}: ${truncateText(cause, 400)}`) : []; const detail = [ - output ? truncateText(output, 2_000) : "", + output ? truncateText(output, LANE_OUTPUT_MAX_CHARS) : "", failureReasons.length > 0 ? `Failure reasons:\n${failureReasons.join("\n")}` : "", ] .filter(Boolean) diff --git a/packages/coding-agent/test/step-subagent-events.test.ts b/packages/coding-agent/test/step-subagent-events.test.ts index 5e775fe..3595e72 100644 --- a/packages/coding-agent/test/step-subagent-events.test.ts +++ b/packages/coding-agent/test/step-subagent-events.test.ts @@ -161,6 +161,10 @@ test("background lane completion steers an escaped agent-notification", async () createContext("/workspace"), )) as AgentToolResult<{ agentId?: string }>; expect(result.content[0]).toMatchObject({ type: "text" }); + // The parent must end its turn rather than sleep-poll; the lane wakes it. + const started = result.content[0].type === "text" ? result.content[0].text : ""; + expect(started).toContain("Do not wait for it with sleep or polling"); + expect(started).toContain("woken automatically"); await waitFor(() => sent.length >= 1); const done = sent[0]; expect(done.customType).toBe("agent-notification"); @@ -494,12 +498,36 @@ test("progress notifications ride along without waking an idle parent", async () notifyLaneEvent(pi, lane, "background_needs_input", "which file?"); notifyLaneEvent(pi, lane, "background_failed", "boom"); - // Non-waking events must leave triggerTurn unset, not false: AgentSession - // defers an explicit false to the end of a running turn instead of steering. + // Non-waking events must stay out of the steering queue: it drains one + // message per turn, so a progress backlog would hold completions behind it. expect(sent).toEqual([ - { event: "background_progress", options: { deliverAs: "steer" } }, - { event: "background_restarted", options: { deliverAs: "steer" } }, + { event: "background_progress", options: { triggerTurn: false } }, + { event: "background_restarted", options: { triggerTurn: false } }, { event: "background_needs_input", options: { deliverAs: "steer", triggerTurn: true } }, { event: "background_failed", options: { deliverAs: "steer", triggerTurn: true } }, ]); }); + +test("a lane's final notification carries a long report in full", async () => { + const { api, tools, sent } = createApi(); + // Well past the old 2,000-character cap, with the conclusion at the end. + const report = `${"analysis line\n".repeat(600)}SUMMARY: all files reviewed`; + createStepSubagentExtension({ + includeBuiltinAgents: true, + agentDir: "/tmp/step-agent-test", + runner: async () => textResult(report), + })(api); + await tools + .get("subagent")! + .execute( + "call", + { agent: "general", task: "review", run_in_background: true } as never, + undefined, + undefined, + createContext("/workspace"), + ); + await waitFor(() => sent.some((message) => message.details?.event === "background_done")); + const done = sent.find((message) => message.details?.event === "background_done")!; + expect(done.content).toContain("SUMMARY: all files reviewed"); + expect(done.content).not.toContain("[output truncated]"); +}); diff --git a/packages/coding-agent/test/suite/regressions/subagent-lane-progress-backlog.test.ts b/packages/coding-agent/test/suite/regressions/subagent-lane-progress-backlog.test.ts new file mode 100644 index 0000000..0f70612 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/subagent-lane-progress-backlog.test.ts @@ -0,0 +1,81 @@ +import type { AgentMessage, AgentTool } from "@step-harness/agent-core"; +import { fauxAssistantMessage, fauxToolCall } from "@step-harness/providers"; +import { Type } from "typebox"; +import { afterEach, describe, expect, it } from "vitest"; +import type { ExtensionAPI } from "../../../src/core/extensions/types.ts"; +import type { BackgroundAgentLane } from "../../../src/features/step-subagent.ts"; +import { notifyLaneEvent } from "../../../src/features/subagent/lane-events.ts"; +import { createHarness, type Harness } from "../harness.ts"; + +function laneEvents(messages: AgentMessage[]): string[] { + return messages.flatMap((message) => { + if (message.role !== "custom" || message.customType !== "agent-notification") return []; + const event = (message.details as { event?: string } | undefined)?.event; + return event ? [event] : []; + }); +} + +// Session 01a123da: four progress-subscribed lanes ran while the parent sat in +// minutes-long tool calls. Progress was steered, the steering queue drains one +// message per turn, and the lanes' completion notices waited behind dozens of +// queued progress notices for over half an hour. +describe("background lane notifications during a long tool call", () => { + const harnesses: Harness[] = []; + + afterEach(() => { + while (harnesses.length > 0) { + harnesses.pop()?.cleanup(); + } + }); + + it("delivers a completion on the next model call, not behind queued progress", async () => { + let duringTool: (() => void) | undefined; + const slowTool: AgentTool = { + name: "wait", + label: "Wait", + description: "A long tool call, e.g. sleep", + parameters: Type.Object({}), + execute: async () => { + duringTool?.(); + return { content: [{ type: "text", text: "waited" }], details: {} }; + }, + }; + + const harness = await createHarness({ tools: [slowTool] }); + harnesses.push(harness); + const pi = { + sendMessage: (message: Parameters[0], options?: never) => { + void harness.session.sendCustomMessage(message, options); + }, + } as unknown as ExtensionAPI; + const lane = { + id: "96e529c1", + subscribe: "progress", + status: "running", + details: { results: [{ agent: "general" }] }, + } as unknown as BackgroundAgentLane; + duringTool = () => { + for (let turn = 1; turn <= 12; turn++) { + notifyLaneEvent(pi, lane, "background_progress", `step 1/1; turns ${turn}`); + } + lane.status = "completed"; + notifyLaneEvent(pi, lane, "background_done", "REPORT"); + }; + + harness.setResponses([ + fauxAssistantMessage([fauxToolCall("wait", {})], { stopReason: "toolUse" }), + fauxAssistantMessage("got the report"), + ]); + + await harness.session.prompt("start the lanes and wait"); + + const messages = harness.session.messages; + const reply = messages.findIndex((message) => message.role === "assistant" && message.stopReason === "stop"); + const beforeReply = laneEvents(messages.slice(0, reply)); + expect(beforeReply).toContain("background_done"); + expect(beforeReply.filter((event) => event === "background_progress")).toHaveLength(12); + // Nothing is left to replay after the parent's reply: no further model calls. + expect(laneEvents(messages.slice(reply + 1))).toEqual([]); + expect(messages.at(-1)).toMatchObject({ role: "assistant", stopReason: "stop" }); + }); +});