From 52d6a791a847066a46f094e1f0a723221ee2ec30 Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Tue, 15 Sep 2026 09:35:49 +0300 Subject: [PATCH 1/6] fix(fusion): F42 a worker is handed back only after two completed steps without a write, never mid-generation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live, Gemma 4 31B at ~6 tok/s, 2026-09-15: F19's time half fired at 1,350 s while the worker's FIRST completion was still streaming — 7,293 tokens of the file it was about to write. Zero completed steps, 22 minutes of generation discarded, the orchestrator re-briefed from scratch. The early hand-back for a task with declared files now fires only when (a) at least HAND_BACK_MIN_COMPLETED_STEPS = 2 steps have completed (step_finished, not step_started) with no successful os.fs.write/edit/patch, AND (b) nothing is in flight: the rule runs when a step finishes, never on a timer. Past that floor it fires at max(2, workerMaxSteps/2) completed steps or, at a step boundary, once half the time limit has elapsed. A hand-back that trips on the step that closed the turn by itself leaves the reply to the ground-truth check. The worker's wall timeout (F19's throughput-derived limit) is unchanged as the hard bound, now armed on a plain setTimeout with the same TimeoutError so a test clock can drive it. The needs_orchestrator status and the forced summary stay; the note carries how many steps had completed, and the row's stepCount matches it. Tests: the time-based hand-back during step 1 now expects the worker to continue; added the two-completed-steps floor on the step half, one completed step plus a long in-flight generation, the time half at a step boundary (not at one completed step), and the wall timeout ending a worker stuck in one endless step under fake timers. --- src/tools/fusion/worker-runner.test.ts | 236 ++++++++++++++++++++++--- src/tools/fusion/worker-runner.ts | 91 +++++++--- 2 files changed, 287 insertions(+), 40 deletions(-) diff --git a/src/tools/fusion/worker-runner.test.ts b/src/tools/fusion/worker-runner.test.ts index c25659ba..7c0cd1c7 100644 --- a/src/tools/fusion/worker-runner.test.ts +++ b/src/tools/fusion/worker-runner.test.ts @@ -796,8 +796,15 @@ describe("worker limits from throughput (F19)", () => { }); }); -describe("early hand-back when nothing is written (D4 / F19)", () => { + +describe("early hand-back when nothing is written (D4 / F19, F42)", () => { const stepStarted = (stepIndex: number): AgentLoopEvent => ({ type: "step_started", stepIndex }); + const stepFinished = (stepIndex: number): AgentLoopEvent => ({ + type: "step_finished", + stepIndex, + summary: "step done", + durationMs: 1, + }); const wrote = (tool = "os.fs.write"): AgentLoopEvent => ({ type: "llm_event", event: { @@ -816,22 +823,64 @@ describe("early hand-back when nothing is written (D4 / F19)", () => { batchSize: 1, }, }); + const replied = (): AgentLoopEvent => ({ + type: "llm_event", + event: { type: "assistant_reply", text: "done" }, + }); + type TurnOptions = Parameters[2]; - /** A worker that reads at every step and writes at `writeAtStep`, if ever. */ + /** + * A worker that reads at every step and writes at `writeAtStep`, if + * ever. Mirrors the loop's order: the signal is checked at the top of + * a step, a step's tool result lands before its `step_finished`. + */ function stepping(writeAtStep: number | null) { - return async ({ options }: { options: Parameters[2] }) => { + return async ({ options }: { options: TurnOptions }) => { for (let step = 1; step <= 8; step += 1) { - options.eventHook?.(stepStarted(step - 1)); if (options.signal?.aborted) { return turnResult({ reason: "cancelled", stepCount: step - 1 }); } + options.eventHook?.(stepStarted(step - 1)); options.eventHook?.(step === writeAtStep ? wrote() : read()); + options.eventHook?.(stepFinished(step - 1)); } - options.eventHook?.({ type: "llm_event", event: { type: "assistant_reply", text: "done" } }); + options.eventHook?.(replied()); return turnResult({ stepCount: 8 }); }; } + /** + * A worker whose current step is one long generation: `completedBefore` + * read-only steps finish at once, then the next step starts and its + * completion stays in flight until the test releases it (it then + * writes the file and replies) or the signal aborts — the two ways a + * streaming request ends in the real loop. + */ + function generating(completedBefore: number) { + let release: (() => void) | undefined; + const runTurn = ({ options }: { options: TurnOptions }) => + new Promise((resolve) => { + for (let i = 0; i < completedBefore; i += 1) { + options.eventHook?.(stepStarted(i)); + options.eventHook?.(read()); + options.eventHook?.(stepFinished(i)); + } + options.eventHook?.(stepStarted(completedBefore)); + options.signal?.addEventListener( + "abort", + () => resolve(turnResult({ reason: "cancelled", stepCount: completedBefore })), + { once: true }, + ); + release = () => { + options.eventHook?.(wrote()); + options.eventHook?.(stepFinished(completedBefore)); + options.eventHook?.(replied()); + resolve(turnResult({ stepCount: completedBefore + 1 })); + }; + }); + return { runTurn, release: () => release!() }; + } + it("hands a task with declared files back once half the steps pass with no write", async () => { const { deps, events } = harness(stepping(null)); const [result] = await runWorkerTasks(deps, { @@ -846,9 +895,12 @@ describe("early hand-back when nothing is written (D4 / F19)", () => { expect(result!.reply).toMatch(/^handed back early: no file written by half the budget \(4 of 8 steps/); expect(result!.reply).toContain("what I found: 4 tool calls (os.fs.read×4)"); expect(result!.reply).toContain("read main.js: 40 lines"); + expect(result!.stepCount).toBe(4); expect(result!.error).toBeUndefined(); expect(result!.notes).toContainEqual( - expect.stringContaining("handed back early: declared files but wrote none"), + expect.stringContaining( + "handed back early: declared files but wrote none by half the budget (4 steps completed, none a successful write)", + ), ); const finished = events.map((e) => e.event).find((e) => e.type === "fusion_worker" && e.phase !== "started" && e.phase !== "tool"); expect(finished).toMatchObject({ phase: "finished", summary: "needs the orchestrator" }); @@ -885,31 +937,175 @@ describe("early hand-back when nothing is written (D4 / F19)", () => { expect(result!.stepCount).toBe(8); }); - it("hands back at half the time limit too", async () => { + it("hands back after two completed no-write steps, even when half the budget is one (F42)", async () => { + // Half of 3 is 1, but one completed step is a worker that read the + // spec: the floor holds the check until a second step has finished. + const { deps } = harness(stepping(null)); + const [result] = await runWorkerTasks(deps, { + ...BASE, + workerMaxSteps: 3, + tasks: [{ ...tasks(1)[0]!, files: ["a.js"] }], + maxWorkers: 1, + signal: new AbortController().signal, + }); + expect(result!.status).toBe("needs_orchestrator"); + expect(result!.reply).toMatch(/^handed back early: no file written by half the budget \(2 of 3 steps/); + expect(result!.stepCount).toBe(2); + expect(result!.notes).toContainEqual( + expect.stringContaining("(2 steps completed, none a successful write)"), + ); + }); + + it("never hands back on one completed step: a worker that reads once and then writes runs on (F42)", async () => { + // Half of 2 is 1; F19 would have stopped this worker as it started + // its second step — the write. A glob keeps the disk check out of + // it, so the write call is the evidence. + const { deps } = harness(stepping(2)); + const [result] = await runWorkerTasks(deps, { + ...BASE, + workerMaxSteps: 2, + tasks: [{ ...tasks(1)[0]!, files: ["js/**/*.js"] }], + maxWorkers: 1, + signal: new AbortController().signal, + }); + expect(result!).toMatchObject({ status: "ok", stepCount: 8, tools: { writes: 1 } }); + }); + + it("keeps generating past half the time limit while the first completion is still streaming (F42)", async () => { + // Live: a 6 tok/s worker was 1,350 s — half its limit — into its + // FIRST completion, 7,293 tokens of the file it was about to write, + // when F19's timer stopped it. Nothing has completed, nothing is + // checked: the worker runs on and the write lands. vi.useFakeTimers(); try { - const { deps } = harness( - ({ options }) => - new Promise((resolve) => { - options.eventHook?.(stepStarted(0)); - options.eventHook?.(read()); - options.signal?.addEventListener("abort", () => - resolve(turnResult({ reason: "cancelled", stepCount: 1 })), - ); - }), - ); + const worker = generating(0); + const { deps, calls } = harness(worker.runTurn); + let done = false; const run = runWorkerTasks(deps, { ...BASE, workerTimeoutMs: 60_000, - tasks: [{ ...tasks(1)[0]!, files: ["a.js"] }], + tasks: [{ ...tasks(1)[0]!, files: ["js/**/*.js"] }], maxWorkers: 1, signal: new AbortController().signal, + }).then((results) => { + done = true; + return results; }); await vi.advanceTimersByTimeAsync(30_000); + expect(calls[0]!.options.signal?.aborted).toBe(false); + await vi.advanceTimersByTimeAsync(15_000); + expect(calls[0]!.options.signal?.aborted).toBe(false); + expect(done).toBe(false); + worker.release(); + const [result] = await run; + expect(result!).toMatchObject({ status: "ok", reply: "done", stepCount: 1, tools: { writes: 1 } }); + expect(result!.notes).toBeUndefined(); + } finally { + vi.useRealTimers(); + } + }); + + it("does not hand back a worker with one completed step and a long in-flight generation (F42)", async () => { + // One read step done, the second step's completion streaming past + // half the time limit: the floor is two completed steps, and a + // check only runs at a step boundary anyway. + vi.useFakeTimers(); + try { + const worker = generating(1); + const { deps, calls } = harness(worker.runTurn); + let done = false; + const run = runWorkerTasks(deps, { + ...BASE, + workerMaxSteps: 8, + workerTimeoutMs: 60_000, + tasks: [{ ...tasks(1)[0]!, files: ["js/**/*.js"] }], + maxWorkers: 1, + signal: new AbortController().signal, + }).then((results) => { + done = true; + return results; + }); + await vi.advanceTimersByTimeAsync(45_000); + expect(calls[0]!.options.signal?.aborted).toBe(false); + expect(done).toBe(false); + worker.release(); + const [result] = await run; + expect(result!).toMatchObject({ status: "ok", stepCount: 2, tools: { calls: 2, writes: 1 } }); + } finally { + vi.useRealTimers(); + } + }); + + it("hands back at a step boundary once two steps completed past half the time, not at one (F42)", async () => { + vi.useFakeTimers(); + try { + let abortedAfterFirst: boolean | undefined; + const { deps } = harness(async ({ options }) => { + // The first step's completion takes 31 s of a 60 s limit. + options.eventHook?.(stepStarted(0)); + options.eventHook?.(read()); + await new Promise((r) => setTimeout(r, 31_000)); + options.eventHook?.(stepFinished(0)); + abortedAfterFirst = options.signal?.aborted; + options.eventHook?.(stepStarted(1)); + options.eventHook?.(read()); + await new Promise((r) => setTimeout(r, 1_000)); + options.eventHook?.(stepFinished(1)); + if (options.signal?.aborted) { + return turnResult({ reason: "cancelled", stepCount: 2 }); + } + options.eventHook?.(replied()); + return turnResult({ stepCount: 2 }); + }); + const run = runWorkerTasks(deps, { + ...BASE, + workerMaxSteps: 8, + workerTimeoutMs: 60_000, + tasks: [{ ...tasks(1)[0]!, files: ["a.js"] }], + maxWorkers: 1, + signal: new AbortController().signal, + }); + await vi.advanceTimersByTimeAsync(32_000); const [result] = await run; + // One completed step past the half-way mark is not enough… + expect(abortedAfterFirst).toBe(false); + // …two are, and the check ran when the second finished. expect(result!.status).toBe("needs_orchestrator"); - expect(result!.reply).toContain("handed back early"); - expect(result!.reply).toContain("of 1 min"); + expect(result!.reply).toMatch(/^handed back early: no file written by half the budget \(2 of 8 steps, 1 min of 1 min\)/); + expect(result!.stepCount).toBe(2); + } finally { + vi.useRealTimers(); + } + }); + + it("still ends a worker stuck in one endless step at its time limit (fake timers)", async () => { + // The hand-back never fires mid-generation; the wall timeout is the + // hard bound and does, exactly as before. + vi.useFakeTimers(); + try { + const worker = generating(0); + const { deps, calls } = harness(worker.runTurn); + let done = false; + const run = runWorkerTasks(deps, { + ...BASE, + workerTimeoutMs: 60_000, + tasks: [{ ...tasks(1)[0]!, files: ["a.js"] }], + maxWorkers: 1, + signal: new AbortController().signal, + }).then((results) => { + done = true; + return results; + }); + await vi.advanceTimersByTimeAsync(59_999); + expect(calls[0]!.options.signal?.aborted).toBe(false); + expect(done).toBe(false); + await vi.advanceTimersByTimeAsync(1); + expect(calls[0]!.options.signal?.aborted).toBe(true); + const [result] = await run; + expect(result!).toMatchObject({ status: "max_steps", stepCount: 0 }); + expect(result!.notes?.[0]).toMatch(/time limit/); + expect(result!.reply).not.toContain("handed back"); + expect(result!.durationMs).toBe(60_000); } finally { vi.useRealTimers(); } diff --git a/src/tools/fusion/worker-runner.ts b/src/tools/fusion/worker-runner.ts index 98b6aa2b..9ec66056 100644 --- a/src/tools/fusion/worker-runner.ts +++ b/src/tools/fusion/worker-runner.ts @@ -80,6 +80,22 @@ const WRITE_TOOLS: ReadonlySet = new Set([ "os.fs.patch", ]); +/** + * How many steps must have COMPLETED, none of them a successful write, + * before a task with declared files can be handed back early (F42). + * + * One completed step without a write is a worker that read the spec. + * Two is a worker that is still not writing after it has seen what it + * read — the pattern the hand-back exists for (two workers once spent + * 40 steps each that way). The rule is evaluated only when a step + * finishes, so nothing is ever in flight when it fires: F19 checked it + * on a timer at half the time limit, and on a 6 tok/s local worker + * that fired 1,350 s into the worker's FIRST completion — 7,293 tokens + * of the file it was about to write, discarded for a re-brief from + * scratch, with zero completed steps to show for 22 minutes. + */ +export const HAND_BACK_MIN_COMPLETED_STEPS = 2; + /** The forced summary a handed-back task replies with. */ export function formatEarlyHandBack(input: { stepsTaken: number; @@ -328,28 +344,53 @@ async function runOneTask( // The worker's own clock, kept apart from the operator's signal: when // it is the one that fired, the worker ran out of time — a ceiling, // reported as `max_steps` — rather than being cancelled by anybody. - const timeLimit = AbortSignal.timeout(timeoutMs); + // This is the hard bound: it fires mid-generation by design, because + // a worker stuck in one endless step has nothing else to end it. A + // plain timer rather than `AbortSignal.timeout` so a test clock can + // drive it; the reason is the `TimeoutError` Node would have raised. + const timeLimitController = new AbortController(); + const timeLimit = timeLimitController.signal; + const wallTimer = setTimeout(() => { + timeLimitController.abort( + new DOMException("The operation was aborted due to timeout", "TimeoutError"), + ); + }, timeoutMs); + wallTimer.unref?.(); const hitTimeLimit = (): boolean => timeLimit.aborted && !options.signal.aborted; - // D4: a task that declared output files and has written none by half - // its step budget or half its time is handed back with what it found, - // instead of spending the other half the same way (two workers once - // used 40 steps each and wrote nothing). Only for tasks with declared + // D4 / F42: a task that declared output files and has written none by + // half its step budget or half its time is handed back with what it + // found, instead of spending the other half the same way — but only + // at a step boundary, and only once `HAND_BACK_MIN_COMPLETED_STEPS` + // steps have completed. The check runs when a step finishes, never on + // a timer: at that moment the step's completion and its tool calls + // are done and the next completion has not been requested, so the + // abort costs nothing that was generated. Only for tasks with declared // files: a task that legitimately reads before it reports has no // half-way mark to miss. const handBack = new AbortController(); - const halfSteps = Math.max(1, Math.floor(options.workerMaxSteps / 2)); - let stepsStarted = 0; + const stepThreshold = Math.max( + HAND_BACK_MIN_COMPLETED_STEPS, + Math.floor(options.workerMaxSteps / 2), + ); + const halfTimeMs = Math.floor(timeoutMs / 2); + let stepsFinished = 0; let wroteSomething = false; const maybeHandBack = (): void => { if (declaredFiles === 0 || wroteSomething || handBack.signal.aborted) return; + if (stepsFinished < HAND_BACK_MIN_COMPLETED_STEPS) return; + const pastHalfTime = Date.now() - startedAt >= halfTimeMs; + if (stepsFinished < stepThreshold && !pastHalfTime) return; handBack.abort(new Error("handed back early: no file written by half the budget")); }; - const halfTimer = setTimeout(maybeHandBack, Math.floor(timeoutMs / 2)); - halfTimer.unref?.(); const handedBack = (): boolean => handBack.signal.aborted && !options.signal.aborted && !timeLimit.aborted; + // Whether the hand-back is what ended the turn. The rule can also + // trip on the step that closed the turn by itself (a `reply` at the + // threshold); that worker finished, and its reply stands — the + // ground-truth check below classifies it, not the hand-back. + let stoppedByHandBack = false; let result: WorkerTaskResult; try { @@ -369,9 +410,12 @@ async function runOneTask( signal: AbortSignal.any([options.signal, timeLimit, handBack.signal]), eventHook: (event) => { if (event.type === "turn_started") announceStart(); - if (event.type === "step_started") { - stepsStarted += 1; - if (stepsStarted > halfSteps) maybeHandBack(); + if (event.type === "step_finished") { + // `step_finished`, not `step_started`: a started step is a + // request in flight, and F19's count of those is how a + // worker was stopped mid-file. + stepsFinished += 1; + maybeHandBack(); } if ( event.type === "llm_event" && @@ -395,6 +439,7 @@ async function runOneTask( }, }); const timedOut = turn.reason === "cancelled" && hitTimeLimit(); + stoppedByHandBack = turn.reason === "cancelled" && handedBack(); const stopCause = timedOut ? "time_ceiling" : turn.stopCause; result = collector.finish({ id: task.id, @@ -412,14 +457,17 @@ async function runOneTask( }); } catch (error) { const timedOut = hitTimeLimit(); + stoppedByHandBack = !timedOut && handedBack(); const aborted = !timedOut && - (options.signal.aborted || handedBack() || isAbortError(error)); + (options.signal.aborted || stoppedByHandBack || isAbortError(error)); result = collector.finish({ id: task.id, title: task.title, reason: timedOut ? "max_steps" : aborted ? "cancelled" : null, - stepCount: 0, + // A thrown turn reported no count; the steps the hook saw finish + // are the ones that happened. + stepCount: stoppedByHandBack ? stepsFinished : 0, durationMs: Date.now() - startedAt, ...(timedOut ? // The abort's own message ("aborted due to timeout") says @@ -429,7 +477,7 @@ async function runOneTask( : { error: error instanceof Error ? error.message : String(error) }), }); } finally { - clearTimeout(halfTimer); + clearTimeout(wallTimer); // Always: the gate is process-wide and a stale refusal policy keyed // to a dead session is a slow leak, not a visible bug. deps.approvals.clearSessionPolicy(session.id); @@ -439,12 +487,14 @@ async function runOneTask( // A hand-back is neither a cancellation nor a failure: the worker was // stopped by its own half-way rule and reports what it found, as a // task the orchestrator must re-brief. - if (handedBack()) { + if (stoppedByHandBack) { const { error: _dropped, ...rest } = result; + // The loop's own count when it reported one; the hook's count of + // finished steps otherwise. Both count completed steps — the rule + // only fires at a step boundary, so nothing was cut short. + const completed = result.stepCount > 0 ? result.stepCount : stepsFinished; const summary = formatEarlyHandBack({ - // Steps completed when the loop reported them; the started count - // only when the turn threw before it could. - stepsTaken: result.stepCount > 0 ? result.stepCount : stepsStarted, + stepsTaken: completed, stepBudget: options.workerMaxSteps, elapsedMs: result.durationMs, timeoutMs, @@ -454,9 +504,10 @@ async function runOneTask( ...rest, status: "needs_orchestrator", reply: summary, + stepCount: completed, notes: [ ...(result.notes ?? []), - "handed back early: declared files but wrote none by half the budget — re-brief with a narrower task or the exact content to write", + `handed back early: declared files but wrote none by half the budget (${completed} steps completed, none a successful write) — re-brief with a narrower task or the exact content to write`, ], }; } From 595f265dd167f5c864c273f3465dbf26290c905d Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Tue, 15 Sep 2026 10:11:59 +0300 Subject: [PATCH 2/6] fix(fusion): F43 restore copies are shared per working directory; a replaced input is a status-table fact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live, fusion, Gemma worker, 2026-09-15: the worker overwrote the user's 2,401-row `sales.csv` with a 9-row sample. F36 saved the original under `/restore//1-sales.csv` and warned in the write result. Two gaps: each fusion worker is its own ephemeral session, so a later worker's `os.fs.restore {"path":"sales.csv"}` found nothing; and the orchestrator saw the warning only inside the worker's prose block of the delegate summary and merged anyway. - fs-restore-store / fs-restore-manifest: the copies are keyed by WORKING DIRECTORY — `/restore//` with `-` and a `manifest.json` (v2: the copy index plus the directory's path), `key` = first 32 hex chars of sha256(absolute working dir) (`restoreKey`). The created-by-the-agent set stays per session at `/restore/sessions/.json`. Any session on the same working directory — the orchestrator, a worker of any fan-out, a resumed session — restores a path. Cap of 20 copies per working dir and the 5 MB limit kept. Nothing migrated: F36's per-session dirs are no longer consulted. Concurrent writers are the normal case now (one fan-out's workers share a manifest): read-modify-writes are serialised in-process per index, and a copy file is created with `wx`, its number bumped past anything another process left. Each copy records the session that took it (`savedBy` in the restore result). - fs-replace-guard: takes `workingDir` beside `sessionId`; the result's `details.replaced` carries `display` (the path as the call spelled it). os.fs.restore looks up by working directory; its description and the prompt descriptor say "in this working directory, by this session or another". - fusion/replaced-inputs: every replace-guard hit in a worker's tool results is recorded per task as `replacedInputs` (path, tool, lines before/after, header changed, saved). worker-result renders it first on the task's status-table row — `- [t1] ok — Sales — replaced the user's file sales.csv (2,401 → 9 lines)` — ahead of the error and the notes, counts it on the head line (`2 tasks: 2 ok — 1 replaced input`), puts it first in the task block's diagnosis, and `details.tasks[].replacedInputs` carries it. The status stands. - fusion-guidance: "A task that replaced a pre-existing file is not done until the file is restored (`os.fs.restore` in a worker) or the replacement was asked for." Paid for within the 1,400-char budget by shortening the other lines; every pinned phrase kept. Tests: cross-session restore on one working directory (worker A replaces, worker B / the orchestrator / a fresh store restores); different working directories isolated; the key is the absolute path however spelled; five sessions replacing concurrently get five distinct copies; the created set stays per session; the collector records hits in call order and ignores unknown shapes; head line, row, block and details with a replaced input, plural and unsaved variants; the row keeps it through the declared-file check whether it stays `ok` or turns `failed`; end to end through fusion.delegate; the guidance sentence and its budget. --- src/prompt/default-tool-descriptors-a.ts | 2 +- src/prompt/fusion-guidance.test.ts | 12 + src/prompt/fusion-guidance.ts | 19 +- src/tools/fusion/fusion-delegate.test.ts | 67 ++++++ src/tools/fusion/replaced-inputs.ts | 103 ++++++++ src/tools/fusion/worker-result.test.ts | 119 +++++++++ src/tools/fusion/worker-result.ts | 44 +++- src/tools/fusion/worker-runner.test.ts | 74 ++++++ src/tools/os/fs-edit.ts | 1 + src/tools/os/fs-patch.ts | 13 +- src/tools/os/fs-replace-guard.test.ts | 147 ++++++++++-- src/tools/os/fs-replace-guard.ts | 15 +- src/tools/os/fs-require-approval.ts | 10 +- src/tools/os/fs-restore-manifest.ts | 138 +++++++++++ src/tools/os/fs-restore-store.ts | 291 ++++++++++++----------- src/tools/os/fs-restore.ts | 20 +- src/tools/os/fs-write.ts | 1 + src/tools/os/index.ts | 3 +- 18 files changed, 883 insertions(+), 196 deletions(-) create mode 100644 src/tools/fusion/replaced-inputs.ts create mode 100644 src/tools/os/fs-restore-manifest.ts diff --git a/src/prompt/default-tool-descriptors-a.ts b/src/prompt/default-tool-descriptors-a.ts index 7cda9bb7..95ae8b9c 100644 --- a/src/prompt/default-tool-descriptors-a.ts +++ b/src/prompt/default-tool-descriptors-a.ts @@ -63,7 +63,7 @@ export const DEFAULT_TOOL_DESCRIPTORS_A: readonly ToolDescriptor[] = [ { name: "os.fs.restore", summary: - "Bring back the previous content of a file that os.fs.write / os.fs.edit / os.fs.patch replaced or shrank this session — the result of that call said it was saved (may require approval).", + "Bring back the previous content of a file that os.fs.write / os.fs.edit / os.fs.patch replaced or shrank in this working directory, by this session or another — the result of that call said it was saved (may require approval).", argsSchema: "{ path: string }", tier: "rare", }, diff --git a/src/prompt/fusion-guidance.test.ts b/src/prompt/fusion-guidance.test.ts index 913315d1..acc25e80 100644 --- a/src/prompt/fusion-guidance.test.ts +++ b/src/prompt/fusion-guidance.test.ts @@ -120,12 +120,24 @@ describe("the ### fusion prefix section", () => { expect(FUSION_GUIDANCE).toContain("needs_orchestrator"); }); + it("says a task that replaced a user's file is not done until it is restored or the replacement was wanted (F43)", () => { + // The status table carries `replaced the user's file sales.csv + // (2,401 → 9 lines)` on the row; without this line the orchestrator + // read the fact and merged anyway. The restore is a worker's call, + // because the orchestrator's own writes are refused. + expect(FUSION_GUIDANCE).toContain( + "A task that replaced a pre-existing file is not done until the file is restored (`os.fs.restore` in a worker) or the replacement was asked for.", + ); + }); + it("stays short enough to live in every turn's prefix", () => { // Every byte here is paid on every step of every fusion turn. The // machine lines carry capacity as well as a count — slots, a // worker's share of the shared context, the shared GPU — which is // what the orchestrator needed and could not see when a four-worker // fan-out overflowed its server; ~100 tokens is what that costs. + // F43's restore line was paid for by shortening the others — the + // budget is the budget. expect(FUSION_GUIDANCE.length).toBeLessThan(1400); expect(buildFusionGuidance(FACTS).length).toBeLessThan(1900); expect( diff --git a/src/prompt/fusion-guidance.ts b/src/prompt/fusion-guidance.ts index ec9062dd..20a33ba5 100644 --- a/src/prompt/fusion-guidance.ts +++ b/src/prompt/fusion-guidance.ts @@ -62,16 +62,19 @@ export function isFusionActive( * exactly the block a machine-less build renders. */ export const FUSION_GUIDANCE = [ - "You orchestrate the workers: read enough to decide, plan, delegate the doing, review what comes back.", - "Plan in the open, then delegate in the same turn — never stop at the plan: list the independent parts, sized so a big one gets its own worker and small ones share.", - "One task per part, in one `fusion.delegate` call. List the paths a task will produce in its `files` — the operator is asked once about those directories, and that is what lets the workers write. Each `instructions` must stand alone: workers see the operator's request, not this chat, and cannot ask you.", + "You orchestrate: read enough to decide, plan, delegate the doing, review what comes back.", + "Plan in the open, then delegate in the same turn — never stop at the plan: list the independent parts, a big one per worker, small ones shared.", + "One task per part, in one `fusion.delegate` call; its `files` name the paths it will produce (approved once by the operator, so the workers can write). Each `instructions` must stand alone: workers see the operator's request, not this chat, and cannot ask.", "You choose `maxWorkers` per call; prefer sending more parts over doing any yourself.", - "Tools that change things are refused for you: the workers build, you do not. That is the mode working, not a fault.", + "Tools that change things are refused for you: the workers build, you do not — the mode working, not a fault.", "Keep the design and the judgement: read every reply against its brief.", - "Before accepting a fan-out, check it: `verify.syntax` on the declared files and `verify.run` on what the request must do.", - "Rework goes back out: anything `failed`, `cancelled`, `needs_orchestrator` or not good enough is another `fusion.delegate` saying what was wrong and what good looks like.", - "Yours alone: the decision you were asked for, a part that only makes sense with this conversation in front of it, and anything needing operator approval.", - "Call `fusion.delegate` on its own, never alongside other tool calls — it runs several turns internally.", + "Before accepting a fan-out: `verify.syntax` on the declared files and `verify.run` on what the request must do.", + "Rework goes back out: `failed`, `cancelled`, `needs_orchestrator` or not good enough is another `fusion.delegate` saying what was wrong and what good looks like.", + // F43: the status table now says `replaced the user's file sales.csv + // (2,401 → 9 lines)` on the row; this is what to do about it. + "A task that replaced a pre-existing file is not done until the file is restored (`os.fs.restore` in a worker) or the replacement was asked for.", + "Yours alone: the decision you were asked for, a part that only makes sense with this conversation in front of it, anything needing approval.", + "Call `fusion.delegate` on its own, never alongside other tool calls.", ].join("\n"); /** diff --git a/src/tools/fusion/fusion-delegate.test.ts b/src/tools/fusion/fusion-delegate.test.ts index ff12f9ec..8b8c3a1b 100644 --- a/src/tools/fusion/fusion-delegate.test.ts +++ b/src/tools/fusion/fusion-delegate.test.ts @@ -576,6 +576,73 @@ describe("fusion.delegate", () => { expect(result.details.outcome).toBe("all_ok"); }); + it("carries a worker's replaced input into the head line, the row and details.tasks (F43)", async () => { + // Live, 2026-09-15: the worker's write result warned that it had + // replaced the user's 2,401-row `sales.csv`; the orchestrator saw + // the warning only inside the worker's prose block and merged. + const replaced = { + path: "/repo/sales.csv", + display: "sales.csv", + bytesBefore: 60_000, + linesBefore: 2401, + linesAfter: 9, + shrunk: true, + headerChanged: false, + saved: "saved", + copy: "1-sales.csv", + }; + const tool = buildFusionDelegateTool( + deps({ + runTurn: async (session, _message, options) => { + if (session.id.endsWith("1")) { + options.eventHook?.({ + type: "llm_event", + event: { + type: "tool_call_executed", + result: { + tool: "os.fs.write", + status: "ok", + summary: "⚠ replaced the user's file `sales.csv` (2,401 lines → 9); …", + details: { replaced }, + truncated: false, + }, + batchIndex: 0, + batchSize: 1, + }, + }); + } + options.eventHook?.({ + type: "llm_event", + event: { type: "assistant_reply", text: "done" }, + }); + return turnResult(); + }, + }), + ); + const result = await tool.run({ tasks: TASKS }, ctx()); + expect(result.status).toBe("ok"); + expect(result.details.outcome).toBe("all_ok"); + const rows = result.details.tasks as WorkerTaskResult[]; + expect(rows[0]?.replacedInputs).toEqual([ + { + path: "sales.csv", + tool: "os.fs.write", + bytesBefore: 60_000, + linesBefore: 2401, + linesAfter: 9, + headerChanged: false, + saved: "saved", + }, + ]); + expect(rows[1]?.replacedInputs).toBeUndefined(); + const lines = result.summary.split("\n"); + expect(lines[0]).toBe("2 tasks: 2 ok — 1 replaced input"); + expect(lines[1]).toBe( + "- [t1] ok — One — replaced the user's file sales.csv (2,401 → 9 lines)", + ); + expect(lines[2]).toBe("- [t2] ok — Two"); + }); + it("is status:error only when every task failed — the per-task rows still come back", async () => { // A fan-out where every worker died used to return `ok`; an // orchestrator reading the status merged nothing as something. diff --git a/src/tools/fusion/replaced-inputs.ts b/src/tools/fusion/replaced-inputs.ts new file mode 100644 index 00000000..ec832239 --- /dev/null +++ b/src/tools/fusion/replaced-inputs.ts @@ -0,0 +1,103 @@ +import { + formatBytes, + formatNumber, + type ReplacedFileDetails, +} from "../os/fs-replace-guard.js"; + +/** + * A pre-existing file a worker's write / edit / patch replaced or + * shrank — F36's replace guard, seen through the worker's tool results + * and carried onto its status-table row. + * + * Live, fusion, Gemma worker (2026-09-15): the worker overwrote the + * user's 2,401-row `sales.csv` with a 9-row sample. The guard saved the + * original and warned in the write result — and that warning reached + * the orchestrator only inside the worker's prose block of the delegate + * summary, where it read as the worker's own words and was not acted + * on. The head line and the task's row are what a capped read sees + * first, so that is where a replaced input goes (F43): `1 replaced + * input` up top, `replaced the user's file sales.csv (2,401 → 9 lines)` + * first on the row, and `details.tasks[].replacedInputs` for a reader + * of the structure. The task keeps its status — the write did land and + * the reply may be right — but the fact is no longer optional reading. + */ +export interface ReplacedInput { + /** The path as the worker spelled it — what `os.fs.restore` takes verbatim. */ + path: string; + /** The tool whose call replaced it (`os.fs.write`) or shrank it (edit, patch). */ + tool: string; + bytesBefore: number; + /** Null when the file was over the guard's size cap and never read. */ + linesBefore: number | null; + linesAfter: number; + headerChanged: boolean; + /** Whether the previous content is in the restore store. */ + saved: ReplacedFileDetails["saved"]; +} + +/** + * The guard's `details.replaced` — one object for a write or an edit, a + * patch's array — as `ReplacedInput`s. Anything else is nothing: a + * result that carries no guard hit, or a shape a future guard changes, + * adds no row fact rather than a wrong one. + */ +export function replacedInputsOf( + tool: string, + details: Record, +): ReplacedInput[] { + const raw = details.replaced; + const list = Array.isArray(raw) ? raw : raw === undefined ? [] : [raw]; + return list.filter(isReplacedFileDetails).map((r) => ({ + path: r.display, + tool, + bytesBefore: r.bytesBefore, + linesBefore: r.linesBefore, + linesAfter: r.linesAfter, + headerChanged: r.headerChanged, + saved: r.saved, + })); +} + +function isReplacedFileDetails(value: unknown): value is ReplacedFileDetails { + if (typeof value !== "object" || value === null) return false; + const r = value as Partial; + return ( + typeof r.display === "string" && + typeof r.bytesBefore === "number" && + (typeof r.linesBefore === "number" || r.linesBefore === null) && + typeof r.linesAfter === "number" && + typeof r.headerChanged === "boolean" && + (r.saved === "saved" || r.saved === "too_large" || r.saved === "failed") + ); +} + +/** + * `replaced the user's file sales.csv (2,401 → 9 lines)`; + * `… (2,401 → 9 lines, header changed)`; `shrank the user's file …` for + * an edit or a patch; `(5.0 MB → 1 line); not saved (too large)` for a + * file the guard could only announce. + */ +export function describeReplacedInput(input: ReplacedInput): string { + const verb = input.tool === "os.fs.write" ? "replaced" : "shrank"; + const before = + input.linesBefore === null + ? formatBytes(input.bytesBefore) + : formatNumber(input.linesBefore); + const unit = input.linesAfter === 1 ? "line" : "lines"; + const header = input.headerChanged ? ", header changed" : ""; + const saved = + input.saved === "saved" + ? "" + : input.saved === "too_large" + ? "; not saved (too large)" + : "; not saved"; + return `${verb} the user's file ${input.path} (${before} → ${formatNumber(input.linesAfter)} ${unit}${header})${saved}`; +} + +/** `1 replaced input` / `3 replaced inputs` for the head line, or null when there were none. */ +export function countReplacedInputs( + results: readonly { replacedInputs?: readonly ReplacedInput[] }[], +): string | null { + const n = results.reduce((sum, r) => sum + (r.replacedInputs?.length ?? 0), 0); + return n === 0 ? null : `${n} replaced input${n === 1 ? "" : "s"}`; +} diff --git a/src/tools/fusion/worker-result.test.ts b/src/tools/fusion/worker-result.test.ts index dc4e0dea..ca521c3c 100644 --- a/src/tools/fusion/worker-result.test.ts +++ b/src/tools/fusion/worker-result.test.ts @@ -193,6 +193,125 @@ describe("WorkerRunCollector", () => { }); }); +/** What the replace guard puts on a write result that replaced `sales.csv` (2,401 → 9 lines). */ +const SALES_REPLACED = { + path: "/repo/sales.csv", + display: "sales.csv", + bytesBefore: 60_000, + linesBefore: 2401, + linesAfter: 9, + shrunk: true, + headerChanged: false, + saved: "saved", + copy: "1-sales.csv", +}; + +describe("WorkerRunCollector — replaced inputs (F43)", () => { + // Live, fusion, Gemma worker (2026-09-15): the write result's warning + // about the 2,401-row `sales.csv` reached the orchestrator only inside + // the worker's prose block, and it merged anyway. + it("records every replace-guard hit from the worker's tool results, in call order", () => { + const c = new WorkerRunCollector(); + c.observe(toolExecuted("os.fs.write", "ok", "⚠ replaced …", { replaced: SALES_REPLACED })); + c.observe(toolExecuted("os.fs.read", "ok")); + // A patch reports its files as an array; an edit that shrank reports one. + c.observe( + toolExecuted("os.fs.patch", "ok", "patch applied", { + replaced: [ + { ...SALES_REPLACED, path: "/repo/a.csv", display: "/repo/a.csv", linesBefore: 40, linesAfter: 3 }, + { ...SALES_REPLACED, path: "/repo/big.bin", display: "big.bin", linesBefore: null, bytesBefore: 6 * 1024 * 1024, linesAfter: 1, saved: "too_large" }, + ], + }), + ); + c.observe(toolExecuted("os.fs.edit", "ok", "diff", { replaced: { ...SALES_REPLACED, display: "notes.md", linesBefore: 500, linesAfter: 2, headerChanged: true } })); + const result = c.finish({ id: "t1", title: "Sales", reason: "reply", stepCount: 4, durationMs: 1 }); + expect(result.status).toBe("ok"); + expect(result.replacedInputs).toEqual([ + { path: "sales.csv", tool: "os.fs.write", bytesBefore: 60_000, linesBefore: 2401, linesAfter: 9, headerChanged: false, saved: "saved" }, + { path: "/repo/a.csv", tool: "os.fs.patch", bytesBefore: 60_000, linesBefore: 40, linesAfter: 3, headerChanged: false, saved: "saved" }, + { path: "big.bin", tool: "os.fs.patch", bytesBefore: 6 * 1024 * 1024, linesBefore: null, linesAfter: 1, headerChanged: false, saved: "too_large" }, + { path: "notes.md", tool: "os.fs.edit", bytesBefore: 60_000, linesBefore: 500, linesAfter: 2, headerChanged: true, saved: "saved" }, + ]); + }); + + it("carries nothing when no result carried a guard hit, and ignores a shape it does not know", () => { + const c = new WorkerRunCollector(); + c.observe(toolExecuted("os.fs.write", "ok", "wrote 10 bytes", { path: "/repo/new.txt", existed: false })); + c.observe(toolExecuted("os.fs.write", "ok", "wrote", { replaced: "sales.csv" })); + c.observe(toolExecuted("os.fs.write", "ok", "wrote", { replaced: { path: "/repo/x" } })); + const result = c.finish({ id: "t1", title: "T", reason: "reply", stepCount: 1, durationMs: 1 }); + expect(result.replacedInputs).toBeUndefined(); + expect("replacedInputs" in result).toBe(false); + }); +}); + +describe("formatDelegateOutput — a replaced input (F43)", () => { + it("counts it on the head line and puts it first on the task's row, ahead of the error and the notes", () => { + const out = formatDelegateOutput( + [ + row({ + id: "t1", + title: "Sales summary", + status: "max_steps", + error: "step limit", + notes: ["stopped at its step limit"], + replacedInputs: [ + { path: "sales.csv", tool: "os.fs.write", bytesBefore: 60_000, linesBefore: 2401, linesAfter: 9, headerChanged: false, saved: "saved" }, + ], + }), + row({ id: "t2", title: "Chart" }), + ], + 8000, + { spend: { usd: 0.5, model: "m", promptTokens: 10, completionTokens: 5 } }, + ); + const lines = out.split("\n"); + // The status stands; the fact rides the head line with the bill. + expect(lines[0]).toBe("2 tasks: 1 ok, 1 max_steps — 1 replaced input — cloud spend $0.50 on m (10 in / 5 out)"); + expect(lines[1]).toBe( + "- [t1] max_steps — Sales summary — replaced the user's file sales.csv (2,401 → 9 lines) — error: step limit — stopped at its step limit", + ); + expect(lines[2]).toBe("- [t2] ok — Chart"); + // And it is the first diagnosis line of the block, above the reply. + const block = out.split("\n\n")[1]!.split("\n"); + expect(block[0]).toContain("[t1] max_steps — Sales summary"); + expect(block[1]).toBe("replaced the user's file sales.csv (2,401 → 9 lines)"); + expect(block[2]).toBe("note: stopped at its step limit"); + }); + + it("pluralises the count across tasks and spells the header, size and unsaved cases", () => { + const out = formatDelegateOutput( + [ + row({ + id: "t1", + replacedInputs: [ + { path: "sales.csv", tool: "os.fs.write", bytesBefore: 1, linesBefore: 2401, linesAfter: 1, headerChanged: true, saved: "saved" }, + { path: "notes.md", tool: "os.fs.edit", bytesBefore: 1, linesBefore: 500, linesAfter: 2, headerChanged: false, saved: "failed" }, + ], + }), + row({ + id: "t2", + replacedInputs: [ + { path: "big.bin", tool: "os.fs.patch", bytesBefore: 6 * 1024 * 1024, linesBefore: null, linesAfter: 3, headerChanged: false, saved: "too_large" }, + ], + }), + ], + 8000, + ); + const lines = out.split("\n"); + expect(lines[0]).toBe("2 tasks: 2 ok — 3 replaced inputs"); + expect(lines[1]).toBe( + "- [t1] ok — Map — replaced the user's file sales.csv (2,401 → 1 line, header changed) — shrank the user's file notes.md (500 → 2 lines); not saved", + ); + expect(lines[2]).toBe( + "- [t2] ok — Map — shrank the user's file big.bin (6.0 MB → 3 lines); not saved (too large)", + ); + }); + + it("says nothing on the head line when no task replaced anything", () => { + expect(formatDelegateOutput([row(), row({ id: "t2" })], 4000).split("\n")[0]).toBe("2 tasks: 2 ok"); + }); +}); + describe("classifyWorkerStatus", () => { it("maps the loop reasons onto worker statuses", () => { expect(classifyWorkerStatus("reply", false)).toBe("ok"); diff --git a/src/tools/fusion/worker-result.ts b/src/tools/fusion/worker-result.ts index 09554660..2f851058 100644 --- a/src/tools/fusion/worker-result.ts +++ b/src/tools/fusion/worker-result.ts @@ -4,6 +4,12 @@ import type { AgentLoopReason, RunTurnResult, } from "../../agent/agent-loop.js"; +import { + countReplacedInputs, + describeReplacedInput, + replacedInputsOf, + type ReplacedInput, +} from "./replaced-inputs.js"; import { FUSION_WORKER_APPROVAL_MARKER } from "./worker-tool-policy.js"; /** @@ -107,6 +113,12 @@ export interface WorkerTaskResult { * (`contract-checks.ts`). A failure is also the row's `error`. */ checks?: TaskCheckSummary; + /** + * Every pre-existing file this task's writes replaced or shrank (the + * replace guard's hits, `replaced-inputs.ts`), in call order. The + * status stands; the row and the head line carry the fact. + */ + replacedInputs?: ReplacedInput[]; } export interface TaskCheckSummary { @@ -157,6 +169,7 @@ export class WorkerRunCollector { private lastWaitReason: string | undefined; /** The last few tool results, one line each — what a hand-back reports. */ private readonly recent: string[] = []; + private readonly replaced: ReplacedInput[] = []; /** Feed one `AgentLoopEvent` from the worker turn's hook. */ observe(event: AgentLoopEvent): void { @@ -203,6 +216,9 @@ export class WorkerRunCollector { if (resultCarriesApprovalRefusal(result.summary, result.details)) { this.approvalRefused = true; } + // The guard's hit is in the result's details whatever its status: + // the write landed before the guard spoke. + this.replaced.push(...replacedInputsOf(result.tool, result.details)); this.recent.push( `${result.tool} ${result.status}: ${oneLine(result.summary, FINDING_CHARS)}`, ); @@ -303,6 +319,9 @@ export class WorkerRunCollector { ...(error === undefined ? {} : { error }), ...(hint === undefined ? {} : { hint }), ...(notes.length === 0 ? {} : { notes }), + ...(this.replaced.length === 0 + ? {} + : { replacedInputs: [...this.replaced] }), }; } } @@ -481,10 +500,13 @@ export interface DelegateOutputExtras { } /** - * The head line (with the bill, when there is one), the contract's - * verdict when there is one, then one line per task. The contract line - * sits second because it is the one cross-task fact: a missing provide - * is a hole between parts, not a property of any single row. + * The head line (with the replaced-input count and the bill, when there + * are any), the contract's verdict when there is one, then one line per + * task. The contract line sits second because it is the one cross-task + * fact: a missing provide is a hole between parts, not a property of + * any single row. A replaced input is first on its row, ahead of the + * error and the notes: the status stands, but a user's file is gone + * until someone restores it, and that outranks why the task stopped. */ function renderStatusTable( results: readonly WorkerTaskResult[], @@ -505,16 +527,21 @@ function renderStatusTable( spend === null ? "" : ` — cloud spend ${formatUsd(spend.usd)} on ${spend.model} (${spend.promptTokens.toLocaleString("en-US")} in / ${spend.completionTokens.toLocaleString("en-US")} out)`; + const replacedCount = countReplacedInputs(results); + const replaced = replacedCount === null ? "" : ` — ${replacedCount}`; const lines = results.map((r) => [ `- [${r.id}] ${r.status} — ${r.title}`, + ...(r.replacedInputs ?? []).map((input) => + oneLine(describeReplacedInput(input), TABLE_DETAIL_CHARS), + ), ...(r.error ? [`error: ${oneLine(r.error, TABLE_DETAIL_CHARS)}`] : []), ...(r.checks ? [describeChecks(r.checks, TABLE_DETAIL_CHARS, r.error)] : []), ...(r.notes ?? []).map((note) => oneLine(note, TABLE_DETAIL_CHARS)), ].join(" — "), ); return [ - `${results.length} task${results.length === 1 ? "" : "s"}: ${tally}${cost}`, + `${results.length} task${results.length === 1 ? "" : "s"}: ${tally}${replaced}${cost}`, ...(contractLine === undefined ? [] : [contractLine]), ...lines, ].join("\n"); @@ -527,9 +554,9 @@ function oneLine(text: string, cap: number): string { /** * The diagnosis goes ABOVE the reply: the error on the head line, then - * the hint and notes. A worker that died on its provider has no reply - * worth the space, and one that claimed work it did not do has a reply - * that must not be read first. + * the replaced inputs, the hint and notes. A worker that died on its + * provider has no reply worth the space, and one that claimed work it + * did not do has a reply that must not be read first. */ function renderBlock(result: WorkerTaskResult, perTaskCap: number): string { const head = @@ -538,6 +565,7 @@ function renderBlock(result: WorkerTaskResult, perTaskCap: number): string { `${result.tools.calls} tool calls, ${result.tools.errors} errors)` + (result.error ? ` — error: ${oneLine(result.error, ERROR_HEAD_CHARS)}` : ""); const diagnosis = [ + ...(result.replacedInputs ?? []).map(describeReplacedInput), ...(result.hint ? [`hint: ${result.hint}`] : []), ...(result.checks ? [describeChecks(result.checks, ERROR_HEAD_CHARS, result.error)] diff --git a/src/tools/fusion/worker-runner.test.ts b/src/tools/fusion/worker-runner.test.ts index 7c0cd1c7..a0cfb471 100644 --- a/src/tools/fusion/worker-runner.test.ts +++ b/src/tools/fusion/worker-runner.test.ts @@ -610,6 +610,80 @@ describe("runWorkerTasks", () => { } }); + it("carries a replaced input onto the row through the declared-file check, whatever the status becomes (F43)", async () => { + // The Gemma worker that wrote a 9-row sample over the user's + // 2,401-row `sales.csv`: the guard's hit rides the write result's + // details, and the row must keep it when the status is folded. + const dir = mkdtempSync(join(tmpdir(), "fusion-runner-files-")); + try { + const replaced = { + path: join(dir, "sales.csv"), + display: "sales.csv", + bytesBefore: 60_000, + linesBefore: 2401, + linesAfter: 9, + shrunk: true, + headerChanged: false, + saved: "saved", + copy: "1-sales.csv", + }; + const { deps } = harness(async ({ options }) => { + writeFileSync(join(dir, "sales.csv"), "sku,qty\n1,2\n"); + options.eventHook?.({ + type: "llm_event", + event: { + type: "tool_call_executed", + result: { + tool: "os.fs.write", + status: "ok", + summary: "⚠ replaced the user's file `sales.csv` (2,401 lines → 9); …", + details: { replaced }, + truncated: false, + }, + batchIndex: 0, + batchSize: 1, + }, + }); + options.eventHook?.({ + type: "llm_event", + event: { type: "assistant_reply", text: "Wrote the sample" }, + }); + return turnResult({ stepCount: 2 }); + }); + deps.workingDir = dir; + const signal = new AbortController().signal; + const [ok] = await runWorkerTasks(deps, { + ...BASE, + tasks: [{ id: "t0", title: "Sales", instructions: "x", files: ["sales.csv"] }], + maxWorkers: 1, + signal, + }); + // The status stands — the write landed and the reply may be right. + expect(ok).toMatchObject({ + status: "ok", + tools: { writes: 1 }, + replacedInputs: [ + { path: "sales.csv", tool: "os.fs.write", linesBefore: 2401, linesAfter: 9, saved: "saved" }, + ], + }); + // A declared file missing turns the row `failed`; the replaced + // input is still on it. + const [failed] = await runWorkerTasks(deps, { + ...BASE, + tasks: [{ id: "t1", title: "Sales", instructions: "x", files: ["sales.csv", "chart.png"] }], + maxWorkers: 1, + signal, + }); + expect(failed).toMatchObject({ + status: "failed", + error: "declared file chart.png does not exist after the task", + replacedInputs: [{ path: "sales.csv" }], + }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it("never reports no_changes for a task without declared files, or one whose write succeeded", async () => { const dir = mkdtempSync(join(tmpdir(), "fusion-runner-files-")); try { diff --git a/src/tools/os/fs-edit.ts b/src/tools/os/fs-edit.ts index 719946a9..7472457c 100644 --- a/src/tools/os/fs-edit.ts +++ b/src/tools/os/fs-edit.ts @@ -81,6 +81,7 @@ export function buildOsFsEditTool( const guard = await guardReplacedFile({ store: options.restore, sessionId: ctx.sessionId, + workingDir: ctx.workingDir, absolute, display: args.path, tool: "os.fs.edit", diff --git a/src/tools/os/fs-patch.ts b/src/tools/os/fs-patch.ts index 10872140..78982319 100644 --- a/src/tools/os/fs-patch.ts +++ b/src/tools/os/fs-patch.ts @@ -17,7 +17,7 @@ import { requireFsApproval, type FsDangerousToolOptions, } from "./fs-require-approval.js"; -import type { ToolDefinition } from "../tool-registry.js"; +import type { ToolContext, ToolDefinition } from "../tool-registry.js"; export type { FileOutcome, PreviewOutcome } from "./fs-patch-preview.js"; @@ -101,9 +101,7 @@ export function buildOsFsPatchTool( ); } await writeFile(outcome.absolute, patched, "utf8"); - guards.push( - await guardAfterPatch(options, ctx.sessionId, outcome, patched), - ); + guards.push(await guardAfterPatch(options, ctx, outcome, patched)); const warning = parseWarningAfterPatch( outcome, patched, @@ -131,13 +129,13 @@ export function buildOsFsPatchTool( */ async function guardAfterPatch( options: FsDangerousToolOptions, - sessionId: string, + ctx: Pick, outcome: PreviewOutcome, patched: string, ): Promise { if (!outcome.existed) { try { - await options.restore?.recordCreated(sessionId, outcome.absolute); + await options.restore?.recordCreated(ctx.sessionId, outcome.absolute); } catch { // Best effort: the patch landed either way. } @@ -145,7 +143,8 @@ async function guardAfterPatch( } return guardReplacedFile({ store: options.restore, - sessionId, + sessionId: ctx.sessionId, + workingDir: ctx.workingDir, absolute: outcome.absolute, display: outcome.absolute, tool: "os.fs.patch", diff --git a/src/tools/os/fs-replace-guard.test.ts b/src/tools/os/fs-replace-guard.test.ts index 3d129e2b..e94f2e0a 100644 --- a/src/tools/os/fs-replace-guard.test.ts +++ b/src/tools/os/fs-replace-guard.test.ts @@ -17,6 +17,7 @@ import { FileRestoreStore, RESTORE_COPY_CAP, RESTORE_MAX_BYTES, + restoreKey, } from "./fs-restore-store.js"; import { buildOsFsWriteTool } from "./fs-write.js"; import { registerOsTools } from "./index.js"; @@ -29,6 +30,11 @@ import { registerOsTools } from "./index.js"; * write still lands (warn-only), the previous content is saved first, * the result says so — loudly on a ≥ 80 % shrink or a changed header, * quietly otherwise — and `os.fs.restore` brings the bytes back. + * + * F43. The copies are keyed by WORKING DIRECTORY, not session: a fusion + * worker is its own ephemeral session, and the worker sent to restore + * `sales.csv` was not the one that replaced it. Only the created set + * stays per session. */ describe("replace guard (F36)", () => { let dir: string; @@ -36,6 +42,8 @@ describe("replace guard (F36)", () => { let store: FileRestoreStore; let prompts: ApprovalRequest[]; let gate: ApprovalGate; + /** `/restore/` — where this working directory's copies live. */ + const copiesDir = (): string => join(stateDir, "restore", restoreKey(dir)); beforeEach(async () => { dir = await mkdtemp(join(tmpdir(), "atomic-replace-guard-")); @@ -105,6 +113,7 @@ describe("replace guard (F36)", () => { ); expect(result.details.replaced).toMatchObject({ path: join(dir, "sales.csv"), + display: "sales.csv", linesBefore: 2402, linesAfter: 10, shrunk: true, @@ -116,8 +125,16 @@ describe("replace guard (F36)", () => { expect(result.details.lines).toBe(10); // The write landed anyway: warn-only. expect(await readFile(join(dir, "sales.csv"), "utf8")).toBe(after); - const copy = join(stateDir, "restore", "s-guard", "1-sales.csv"); + const copy = join(copiesDir(), "1-sales.csv"); expect(await readFile(copy, "utf8")).toBe(before); + // The manifest names the working directory for a human reading the folder. + expect( + JSON.parse(await readFile(join(copiesDir(), "manifest.json"), "utf8")), + ).toMatchObject({ + version: 2, + workingDir: dir, + copies: [{ n: 1, file: "1-sales.csv", tool: "os.fs.write", sessionId: "s-guard" }], + }); }); it("is loud on a header change alone (a .json whose first line moved)", async () => { @@ -168,9 +185,7 @@ describe("replace guard (F36)", () => { headerChanged: false, saved: "saved", }); - expect(existsSync(join(stateDir, "restore", "s-guard", "1-a.py"))).toBe( - true, - ); + expect(existsSync(join(copiesDir(), "1-a.py"))).toBe(true); }); it("says nothing about a file the agent created earlier this session, and still counts the lines", async () => { @@ -191,8 +206,12 @@ describe("replace guard (F36)", () => { `wrote 7 bytes to ${join(dir, "out", "report.md")} (replace, 3 lines → 1)`, ); expect(second.details.replaced).toBeUndefined(); - expect(await readdir(join(stateDir, "restore", "s-guard"))).toEqual([ - "manifest.json", + // No copy was taken: the working directory has no restore folder + // at all, only the session's created-set record. + expect(existsSync(copiesDir())).toBe(false); + expect(await readdir(join(stateDir, "restore"))).toEqual(["sessions"]); + expect(await readdir(join(stateDir, "restore", "sessions"))).toEqual([ + "s-guard.json", ]); }); @@ -272,7 +291,7 @@ describe("replace guard (F36)", () => { linesBefore: null, }); expect(result.summary).toContain("(replace, 5.0 MB → 1 line)"); - expect(await store.listCopies("s-guard")).toEqual([]); + expect(await store.listCopies(dir)).toEqual([]); }); it("spells the operator's retarget in the note when the write was moved", async () => { @@ -318,6 +337,7 @@ describe("replace guard (F36)", () => { bytes: before.length, lines: 2402, savedBefore: "os.fs.write", + savedBy: "s-guard", copy: "1-sales.csv", }); expect(await readFile(join(dir, "sales.csv"), "utf8")).toBe(before); @@ -346,7 +366,7 @@ describe("replace guard (F36)", () => { it("refuses when nothing was saved for the path, and when no store is wired", async () => { await expect( tools().restore.run({ path: "never.csv" }, ctx()), - ).rejects.toThrow(/nothing saved for `never.csv` in this session/); + ).rejects.toThrow(/nothing saved for `never.csv` in this working directory/); await expect( tools(null).restore.run({ path: "never.csv" }, ctx()), ).rejects.toThrow(/keeps no restore copies/); @@ -397,24 +417,123 @@ describe("replace guard (F36)", () => { await registry .get("os.fs.write") .run({ path: "user.csv", content: csv(1, "q") }, ctx("s-reg")); - expect( - existsSync(join(stateDir, "restore", "s-reg", "1-user.csv")), - ).toBe(true); + expect(existsSync(join(copiesDir(), "1-user.csv"))).toBe(true); + }); + }); + + describe("copies are shared per working directory (F43)", () => { + // Live, fusion, 2026-09-15: worker A (its own ephemeral session) + // overwrote the user's 2,401-row `sales.csv` with a 9-row sample; + // the copy went under A's session, and worker B — a different + // session — sent to restore it found nothing. + it("lets another session on the same working directory restore what one session replaced", async () => { + const before = csv(2401); + await writeFile(join(dir, "sales.csv"), before, "utf8"); + const t = tools(); + const replaced = await t.write.run( + { path: "sales.csv", content: csv(9, "sku,qty") }, + ctx("s-fw-worker-a"), + ); + expect(replaced.summary).toContain("⚠ replaced the user's file `sales.csv`"); + + // Worker B of a later fan-out, and the orchestrator itself. + const byB = await t.restore.run({ path: "sales.csv" }, ctx("s-fw-worker-b")); + expect(byB.status).toBe("ok"); + expect(byB.details).toMatchObject({ savedBy: "s-fw-worker-a", copy: "1-sales.csv" }); + expect(await readFile(join(dir, "sales.csv"), "utf8")).toBe(before); + + await t.write.run({ path: "sales.csv", content: "gone\n" }, ctx("s-fw-worker-c")); + await t.restore.run({ path: "sales.csv" }, ctx("s-orchestrator")); + expect(await readFile(join(dir, "sales.csv"), "utf8")).toBe(before); + + // A new process (a fresh store over the same state dir) sees it too. + await t.write.run({ path: "sales.csv", content: "gone again\n" }, ctx("s-fw-worker-d")); + const later = tools(new FileRestoreStore(join(stateDir, "restore"))); + await later.restore.run({ path: "sales.csv" }, ctx("s-resumed")); + expect(await readFile(join(dir, "sales.csv"), "utf8")).toBe(before); + }); + + it("keeps working directories apart: a copy taken in one is not visible from another", async () => { + const other = await mkdtemp(join(tmpdir(), "atomic-replace-guard-other-")); + try { + await writeFile(join(dir, "sales.csv"), csv(100), "utf8"); + await writeFile(join(other, "sales.csv"), csv(50, "x,y"), "utf8"); + const t = tools(); + await t.write.run({ path: "sales.csv", content: csv(1) }, ctx()); + expect(restoreKey(other)).not.toBe(restoreKey(dir)); + expect(existsSync(join(stateDir, "restore", restoreKey(other)))).toBe(false); + + const otherCtx: ToolContext = { ...ctx(), workingDir: other }; + await expect( + t.restore.run({ path: "sales.csv" }, otherCtx), + ).rejects.toThrow(/nothing saved for `sales.csv` in this working directory/); + expect(await readFile(join(other, "sales.csv"), "utf8")).toBe(csv(50, "x,y")); + + // The other directory gets its own folder once something is replaced there. + await t.write.run({ path: "sales.csv", content: "z\n" }, otherCtx); + expect(await store.listCopies(other)).toMatchObject([{ n: 1, path: join(other, "sales.csv") }]); + expect(await store.listCopies(dir)).toMatchObject([{ n: 1, path: join(dir, "sales.csv") }]); + await t.restore.run({ path: "sales.csv" }, otherCtx); + expect(await readFile(join(other, "sales.csv"), "utf8")).toBe(csv(50, "x,y")); + } finally { + await rm(other, { recursive: true, force: true }); + } + }); + + it("keys on the absolute working directory, however it was spelled", () => { + expect(restoreKey(dir)).toMatch(/^[0-9a-f]{32}$/); + expect(restoreKey(`${dir}/`)).toBe(restoreKey(dir)); + expect(restoreKey(join(dir, "sub", ".."))).toBe(restoreKey(dir)); + expect(restoreKey(join(dir, "sub"))).not.toBe(restoreKey(dir)); + }); + + it("gives concurrent replacements by several sessions distinct copies (a fan-out shares one manifest)", async () => { + const t = tools(); + const names = ["a.csv", "b.csv", "c.csv", "d.csv", "e.csv"]; + for (const name of names) { + await writeFile(join(dir, name), `user ${name}\n1\n2\n3\n4\n`, "utf8"); + } + await Promise.all( + names.map((name, i) => + t.write.run({ path: name, content: `agent ${name}\n` }, ctx(`s-fw-${i}`)), + ), + ); + const copies = await store.listCopies(dir); + expect(copies.map((c) => c.n)).toEqual([1, 2, 3, 4, 5]); + expect(new Set(copies.map((c) => c.file)).size).toBe(5); + expect(new Set(copies.map((c) => c.sessionId)).size).toBe(5); + for (const name of names) { + await t.restore.run({ path: name }, ctx("s-later")); + expect(await readFile(join(dir, name), "utf8")).toBe(`user ${name}\n1\n2\n3\n4\n`); + } + }); + + it("the created set stays per session: a file one worker created is the user's to another", async () => { + const t = tools(); + await t.write.run({ path: "out.csv", content: csv(10) }, ctx("s-fw-a")); + const sameSession = await t.write.run({ path: "out.csv", content: csv(1) }, ctx("s-fw-a")); + expect(sameSession.details.replaced).toBeUndefined(); + const otherSession = await t.write.run({ path: "out.csv", content: "x\n" }, ctx("s-fw-b")); + expect(otherSession.summary.split("\n")[0]).toBe( + '⚠ replaced the user\'s file `out.csv` (2 lines → 1, header changed); the previous content is saved — `os.fs.restore {"path":"out.csv"}` brings it back', + ); + expect(await store.wasCreated("s-fw-a", join(dir, "out.csv"))).toBe(true); + expect(await store.wasCreated("s-fw-b", join(dir, "out.csv"))).toBe(false); }); }); describe("copy cap", () => { - it(`keeps the last ${RESTORE_COPY_CAP} copies per session, dropping the oldest file`, async () => { + it(`keeps the last ${RESTORE_COPY_CAP} copies per working directory, dropping the oldest file`, async () => { const t = tools(); for (let i = 1; i <= RESTORE_COPY_CAP + 1; i++) { await writeFile(join(dir, `f${i}.txt`), `user ${i}\n`, "utf8"); await t.write.run({ path: `f${i}.txt`, content: `agent ${i}\n` }, ctx()); } - const copies = await store.listCopies("s-guard"); + const copies = await store.listCopies(dir); expect(copies).toHaveLength(RESTORE_COPY_CAP); expect(copies[0]?.n).toBe(2); expect(copies.at(-1)?.n).toBe(RESTORE_COPY_CAP + 1); - const files = (await readdir(join(stateDir, "restore", "s-guard"))).sort(); + const files = (await readdir(copiesDir())).sort(); expect(files).not.toContain("1-f1.txt"); expect(files).toContain("2-f2.txt"); expect(files).toContain(`${RESTORE_COPY_CAP + 1}-f${RESTORE_COPY_CAP + 1}.txt`); diff --git a/src/tools/os/fs-replace-guard.ts b/src/tools/os/fs-replace-guard.ts index 5af57ed0..c4e1efe3 100644 --- a/src/tools/os/fs-replace-guard.ts +++ b/src/tools/os/fs-replace-guard.ts @@ -45,7 +45,10 @@ export type ReplaceChange = "replace" | "shrink"; export interface ReplaceGuardInput { store: FileRestoreStore | undefined; + /** Decides the created-set (`wasCreated` is per session). */ sessionId: string; + /** Decides where the copy goes: copies are shared per working directory (F43). */ + workingDir: string; absolute: string; /** The path as the model should spell it in `os.fs.restore`. */ display: string; @@ -61,7 +64,10 @@ export interface ReplaceGuardInput { } export interface ReplacedFileDetails { + /** Absolute. */ path: string; + /** The path as the call spelled it — what `os.fs.restore` takes verbatim. */ + display: string; bytesBefore: number; linesBefore: number | null; linesAfter: number; @@ -165,10 +171,14 @@ export async function guardReplacedFile( if (prior.content !== null && prior.bytes <= RESTORE_MAX_BYTES) { try { const copy = await store.saveCopy( - input.sessionId, + input.workingDir, input.absolute, prior.content, - { tool: input.tool, lines: prior.lines ?? 0 }, + { + tool: input.tool, + lines: prior.lines ?? 0, + sessionId: input.sessionId, + }, ); saved = "saved"; copyFile = copy.file; @@ -194,6 +204,7 @@ export async function guardReplacedFile( note, replaced: { path: input.absolute, + display: input.display, bytesBefore: prior.bytes, linesBefore: prior.lines, linesAfter, diff --git a/src/tools/os/fs-require-approval.ts b/src/tools/os/fs-require-approval.ts index f82f468b..137a88d6 100644 --- a/src/tools/os/fs-require-approval.ts +++ b/src/tools/os/fs-require-approval.ts @@ -25,11 +25,11 @@ export interface FsDangerousToolOptions extends DangerousToolOptions { */ trustConfigPaths?: readonly string[]; /** - * Where a replaced user file's previous content is kept and which - * files this session created (`fs-replace-guard.ts`). Built by - * `registerOsTools` from `stateDir`; omitted (embedders, tests) turns - * the replace guard off and leaves `os.fs.restore` with nothing to - * restore. + * Where a replaced user file's previous content is kept — shared by + * every session on the same working directory — and which files each + * session created (`fs-replace-guard.ts`). Built by `registerOsTools` + * from `stateDir`; omitted (embedders, tests) turns the replace guard + * off and leaves `os.fs.restore` with nothing to restore. */ restore?: FileRestoreStore; } diff --git a/src/tools/os/fs-restore-manifest.ts b/src/tools/os/fs-restore-manifest.ts new file mode 100644 index 00000000..a2d1d51d --- /dev/null +++ b/src/tools/os/fs-restore-manifest.ts @@ -0,0 +1,138 @@ +import { createHash } from "node:crypto"; +import { mkdir, rename, writeFile } from "node:fs/promises"; +import { basename, dirname, join, resolve } from "node:path"; + +/** + * The on-disk shapes behind `FileRestoreStore` (`fs-restore-store.ts`) + * and the two writers that keep them safe under concurrent use: the + * atomic JSON write for an index, the exclusive create for a copy. + */ + +/** Keeps `-` under every filesystem's name limit. */ +const COPY_BASENAME_MAX = 200; +const KEY_HEX_CHARS = 32; + +export interface RestoreCopy { + /** Monotonic per working directory; the copy's file is `-`. */ + n: number; + /** Absolute path of the file whose previous content this is. */ + path: string; + /** File name of the copy inside the working directory's restore directory. */ + file: string; + bytes: number; + lines: number; + savedAt: number; + /** The tool whose call replaced the file. */ + tool: string; + /** The session that made the call — a fusion worker's, in a fan-out. */ + sessionId: string; +} + +/** `//manifest.json` — one working directory's copy index. */ +export interface CopiesManifest { + version: 2; + workingDir: string; + next: number; + copies: RestoreCopy[]; +} + +/** `/sessions/.json` — the paths one session created. */ +export interface SessionRecord { + version: 1; + created: string[]; +} + +/** The restore directory name for a working directory: 32 hex chars of its sha256. */ +export function restoreKey(workingDir: string): string { + return createHash("sha256") + .update(resolve(workingDir)) + .digest("hex") + .slice(0, KEY_HEX_CHARS); +} + +/** A session id is `s-` today; anything else is made a safe file name. */ +export function safeSegment(id: string): string { + const safe = id.replace(/[^A-Za-z0-9._-]/g, "_"); + return safe.length === 0 ? "_" : safe; +} + +export function emptyManifest(workingDir: string): CopiesManifest { + return { version: 2, workingDir: resolve(workingDir), next: 1, copies: [] }; +} + +/** A manifest a previous build wrote, or a damaged one, never throws — it just remembers less. */ +export function normalizeManifest( + raw: unknown, + workingDir: string, +): CopiesManifest { + if (typeof raw !== "object" || raw === null) return emptyManifest(workingDir); + const record = raw as Partial; + const copies = Array.isArray(record.copies) + ? record.copies.filter(isRestoreCopy) + : []; + const highest = copies.reduce((max, copy) => Math.max(max, copy.n), 0); + const next = + typeof record.next === "number" && Number.isInteger(record.next) + ? Math.max(record.next, highest + 1) + : highest + 1; + return { ...emptyManifest(workingDir), next, copies }; +} + +export function normalizeSession(raw: unknown): SessionRecord { + const record = ( + typeof raw === "object" && raw !== null ? raw : {} + ) as Partial; + const created = Array.isArray(record.created) + ? record.created.filter((p): p is string => typeof p === "string") + : []; + return { version: 1, created }; +} + +function isRestoreCopy(value: unknown): value is RestoreCopy { + if (typeof value !== "object" || value === null) return false; + const copy = value as Partial; + return ( + typeof copy.n === "number" && + typeof copy.path === "string" && + typeof copy.file === "string" && + typeof copy.bytes === "number" && + typeof copy.lines === "number" && + typeof copy.savedAt === "number" && + typeof copy.tool === "string" && + typeof copy.sessionId === "string" + ); +} + +/** Temp file then rename, so a reader never sees half an index. */ +export async function writeJsonAtomically( + file: string, + value: unknown, +): Promise { + await mkdir(dirname(file), { recursive: true }); + const temp = `${file}.${process.pid}.tmp`; + await writeFile(temp, JSON.stringify(value), "utf8"); + await rename(temp, file); +} + +/** + * Create `-` with `wx`, bumping `n` past any file already + * there: a copy another process wrote under the same number (its + * manifest write raced ours) is never overwritten. + */ +export async function writeCopyExclusively( + dir: string, + from: number, + absolute: string, + content: Uint8Array | string, +): Promise<{ n: number; file: string }> { + const name = basename(absolute).slice(0, COPY_BASENAME_MAX); + for (let n = from; ; n++) { + const file = `${n}-${name}`; + try { + await writeFile(join(dir, file), content, { flag: "wx" }); + return { n, file }; + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "EEXIST") throw err; + } + } +} diff --git a/src/tools/os/fs-restore-store.ts b/src/tools/os/fs-restore-store.ts index 9a78cbdb..17ad355a 100644 --- a/src/tools/os/fs-restore-store.ts +++ b/src/tools/os/fs-restore-store.ts @@ -1,9 +1,23 @@ -import { mkdir, readFile, rename, rm, writeFile } from "node:fs/promises"; -import { basename, join } from "node:path"; +import { mkdir, readFile, rm } from "node:fs/promises"; +import { join } from "node:path"; +import { + emptyManifest, + normalizeManifest, + normalizeSession, + restoreKey, + safeSegment, + writeCopyExclusively, + writeJsonAtomically, + type CopiesManifest, + type RestoreCopy, + type SessionRecord, +} from "./fs-restore-manifest.js"; + +export { restoreKey, type RestoreCopy } from "./fs-restore-manifest.js"; /** - * Where a replaced user file's previous content goes, and which files the - * agent itself created this session. + * Where a replaced user file's previous content goes, and which files + * the agent itself created this session. * * Two live failures motivate this (Gemma 4 31B, 2026-09-15): the model's * first step wrote `projects.json` over the user's data file without @@ -14,118 +28,134 @@ import { basename, join } from "node:path"; * still lands, but its previous content is saved first and the result * says so, loudly when the replacement looks like a loss. * - * Everything lives under `/restore//`: the copies - * as `-` and a small `manifest.json` carrying the copy - * index and the created-path set. On disk rather than in `SessionState` - * because the tools consult it BEFORE a write, and a tool sees only its - * `ToolContext` (working dir, session id) — threading session state into - * every fs tool would be the invasive change. Keyed by session id, so a - * resumed session (same id, new process) still knows what it created. + * Two keys, because the two facts have different owners (F43): + * + * - The COPIES are keyed by WORKING DIRECTORY. `//` holds + * them as `-` plus a `manifest.json` (the copy index, + * and the directory's path for a human reading the folder), where + * `key` is the first 32 hex chars of the sha256 of the absolute + * working directory (`restoreKey`). F36 keyed them by session id, and + * a fusion worker is its own ephemeral session: the worker that + * overwrote `sales.csv` saved the original under ITS session, and the + * later worker sent to restore it — a different session — found + * nothing. Any session on the same working directory (the + * orchestrator, a worker of any fan-out, a resumed session) now + * restores it. + * - The CREATED set stays per session, at `/sessions/.json`: + * "the agent made this file" is a fact about the session that made + * it, so another session's replacement of that file is still + * announced. + * + * On disk rather than in `SessionState` because the tools consult it + * BEFORE a write, and a tool sees only its `ToolContext` (working dir, + * session id). F36's per-session directories are simply not consulted + * any more; nothing is migrated. + * + * Concurrent writers are the normal case now — the workers of one + * fan-out share a manifest — so every read-modify-write of one index is + * serialised in-process, and a copy file is created exclusively (`wx`), + * its number bumped past anything another process left. A manifest two + * processes write at the same instant is last-writer-wins: the loser's + * copy file survives on disk, only its index entry is lost. */ -/** Copies kept per session; the oldest is dropped when a new one lands. */ +/** Copies kept per working directory; the oldest is dropped when a new one lands. */ export const RESTORE_COPY_CAP = 20; /** Largest previous content a copy is taken of. Bigger is announced, not saved. */ export const RESTORE_MAX_BYTES = 5 * 1024 * 1024; -/** Paths remembered as created by the agent; the oldest are forgotten past this. */ +/** Paths remembered as created by a session; the oldest are forgotten past this. */ const CREATED_PATHS_CAP = 5000; const MANIFEST_FILE = "manifest.json"; -/** Keeps `-` under every filesystem's name limit. */ -const COPY_BASENAME_MAX = 200; - -export interface RestoreCopy { - /** Monotonic per session; the copy's file is `-`. */ - n: number; - /** Absolute path of the file whose previous content this is. */ - path: string; - /** File name of the copy inside the session's restore directory. */ - file: string; - bytes: number; - lines: number; - savedAt: number; - /** The tool whose call replaced the file. */ - tool: string; -} - -interface RestoreManifest { - version: 1; - next: number; - created: string[]; - copies: RestoreCopy[]; -} +const SESSIONS_DIR = "sessions"; export class FileRestoreStore { + /** One in-flight read-modify-write per index (see `serialised`). */ + private readonly chains = new Map>(); + constructor(private readonly root: string) {} - /** `/` — the session's copies and manifest. */ - sessionDir(sessionId: string): string { - return join(this.root, safeSegment(sessionId)); + /** `/` — the working directory's copies and manifest. */ + copiesDir(workingDir: string): string { + return join(this.root, restoreKey(workingDir)); + } + + /** `/sessions/.json` — the paths this session created. */ + sessionFile(sessionId: string): string { + return join(this.root, SESSIONS_DIR, `${safeSegment(sessionId)}.json`); } /** Did a tool of this session create `absolute` (write to a path that did not exist)? */ async wasCreated(sessionId: string, absolute: string): Promise { - const manifest = await this.read(sessionId); - return manifest.created.includes(absolute); + return (await this.readSession(sessionId)).created.includes(absolute); } async recordCreated(sessionId: string, absolute: string): Promise { - const manifest = await this.read(sessionId); - if (manifest.created.includes(absolute)) return; - manifest.created.push(absolute); - if (manifest.created.length > CREATED_PATHS_CAP) { - manifest.created.splice(0, manifest.created.length - CREATED_PATHS_CAP); - } - await this.write(sessionId, manifest); + await this.serialised(`session:${sessionId}`, async () => { + const record = await this.readSession(sessionId); + if (record.created.includes(absolute)) return; + record.created.push(absolute); + if (record.created.length > CREATED_PATHS_CAP) { + record.created.splice(0, record.created.length - CREATED_PATHS_CAP); + } + await writeJsonAtomically(this.sessionFile(sessionId), record); + }); } /** - * Save `content` as the previous content of `absolute`. The caller has - * checked the size cap; the bytes are stored as given so a restore puts - * back exactly what was there. Past `RESTORE_COPY_CAP` the oldest copy - * of the session — whichever path it belonged to — is removed. + * Save `content` as the previous content of `absolute`, a file under + * (or reached from) `workingDir`. The caller has checked the size cap; + * the bytes are stored as given so a restore puts back exactly what + * was there. Past `RESTORE_COPY_CAP` the oldest copy of the working + * directory — whichever path or session it belonged to — is removed. */ async saveCopy( - sessionId: string, + workingDir: string, absolute: string, content: Uint8Array | string, - meta: { tool: string; lines: number }, + meta: { tool: string; lines: number; sessionId: string }, ): Promise { - const dir = this.sessionDir(sessionId); - await mkdir(dir, { recursive: true }); - const manifest = await this.read(sessionId); - const n = manifest.next; - const file = `${n}-${basename(absolute).slice(0, COPY_BASENAME_MAX)}`; - await writeFile(join(dir, file), content); - const copy: RestoreCopy = { - n, - path: absolute, - file, - bytes: - typeof content === "string" - ? Buffer.byteLength(content, "utf8") - : content.byteLength, - lines: meta.lines, - savedAt: Date.now(), - tool: meta.tool, - }; - manifest.next = n + 1; - manifest.copies.push(copy); - while (manifest.copies.length > RESTORE_COPY_CAP) { - const dropped = manifest.copies.shift(); - if (dropped !== undefined) { - await rm(join(dir, dropped.file), { force: true }); + const dir = this.copiesDir(workingDir); + return this.serialised(`copies:${dir}`, async () => { + await mkdir(dir, { recursive: true }); + const manifest = await this.readManifest(workingDir); + const { n, file } = await writeCopyExclusively( + dir, + manifest.next, + absolute, + content, + ); + const copy: RestoreCopy = { + n, + path: absolute, + file, + bytes: + typeof content === "string" + ? Buffer.byteLength(content, "utf8") + : content.byteLength, + lines: meta.lines, + savedAt: Date.now(), + tool: meta.tool, + sessionId: meta.sessionId, + }; + manifest.next = n + 1; + manifest.copies.push(copy); + while (manifest.copies.length > RESTORE_COPY_CAP) { + const dropped = manifest.copies.shift(); + if (dropped !== undefined) { + await rm(join(dir, dropped.file), { force: true }); + } } - } - await this.write(sessionId, manifest); - return copy; + await writeJsonAtomically(join(dir, MANIFEST_FILE), manifest); + return copy; + }); } /** The newest saved copy for `absolute`, or null when none was ever taken (or it aged out). */ async latestCopy( - sessionId: string, + workingDir: string, absolute: string, ): Promise { - const manifest = await this.read(sessionId); + const manifest = await this.readManifest(workingDir); for (let i = manifest.copies.length - 1; i >= 0; i--) { const copy = manifest.copies[i]; if (copy !== undefined && copy.path === absolute) return copy; @@ -133,77 +163,54 @@ export class FileRestoreStore { return null; } - async readCopy(sessionId: string, copy: RestoreCopy): Promise { - return readFile(join(this.sessionDir(sessionId), copy.file)); + async readCopy(workingDir: string, copy: RestoreCopy): Promise { + return readFile(join(this.copiesDir(workingDir), copy.file)); } - /** Every copy the session still holds, oldest first. */ - async listCopies(sessionId: string): Promise { - return (await this.read(sessionId)).copies; + /** Every copy the working directory still holds, oldest first. */ + async listCopies(workingDir: string): Promise { + return (await this.readManifest(workingDir)).copies; } - private async read(sessionId: string): Promise { + private async readManifest(workingDir: string): Promise { try { const raw = await readFile( - join(this.sessionDir(sessionId), MANIFEST_FILE), + join(this.copiesDir(workingDir), MANIFEST_FILE), "utf8", ); - return normalizeManifest(JSON.parse(raw)); + return normalizeManifest(JSON.parse(raw), workingDir); } catch { - return emptyManifest(); + return emptyManifest(workingDir); } } - private async write( - sessionId: string, - manifest: RestoreManifest, - ): Promise { - const dir = this.sessionDir(sessionId); - await mkdir(dir, { recursive: true }); - const temp = join(dir, `${MANIFEST_FILE}.${process.pid}.tmp`); - await writeFile(temp, JSON.stringify(manifest), "utf8"); - await rename(temp, join(dir, MANIFEST_FILE)); + private async readSession(sessionId: string): Promise { + try { + const raw = await readFile(this.sessionFile(sessionId), "utf8"); + return normalizeSession(JSON.parse(raw)); + } catch { + return { version: 1, created: [] }; + } } -} - -function emptyManifest(): RestoreManifest { - return { version: 1, next: 1, created: [], copies: [] }; -} - -/** A manifest a previous build wrote, or a damaged one, never throws — it just remembers less. */ -function normalizeManifest(raw: unknown): RestoreManifest { - if (typeof raw !== "object" || raw === null) return emptyManifest(); - const record = raw as Partial; - const created = Array.isArray(record.created) - ? record.created.filter((p): p is string => typeof p === "string") - : []; - const copies = Array.isArray(record.copies) - ? record.copies.filter(isRestoreCopy) - : []; - const highest = copies.reduce((max, copy) => Math.max(max, copy.n), 0); - const next = - typeof record.next === "number" && Number.isInteger(record.next) - ? Math.max(record.next, highest + 1) - : highest + 1; - return { version: 1, next, created, copies }; -} -function isRestoreCopy(value: unknown): value is RestoreCopy { - if (typeof value !== "object" || value === null) return false; - const copy = value as Partial; - return ( - typeof copy.n === "number" && - typeof copy.path === "string" && - typeof copy.file === "string" && - typeof copy.bytes === "number" && - typeof copy.lines === "number" && - typeof copy.savedAt === "number" && - typeof copy.tool === "string" - ); -} - -/** A session id is `s-` today; anything else is made a safe directory name. */ -function safeSegment(id: string): string { - const safe = id.replace(/[^A-Za-z0-9._-]/g, "_"); - return safe.length === 0 ? "_" : safe; + /** + * Run `fn` after every earlier call made under `key` has settled. The + * workers of one fan-out replace files at the same time into the same + * manifest; two unserialised read-modify-writes would each see `next` + * = 5 and one would lose its entry. + */ + private async serialised(key: string, fn: () => Promise): Promise { + const previous = this.chains.get(key) ?? Promise.resolve(); + const run = previous.then(fn); + const settled = run.then( + () => undefined, + () => undefined, + ); + this.chains.set(key, settled); + try { + return await run; + } finally { + if (this.chains.get(key) === settled) this.chains.delete(key); + } + } } diff --git a/src/tools/os/fs-restore.ts b/src/tools/os/fs-restore.ts index 2f339430..3fe728ba 100644 --- a/src/tools/os/fs-restore.ts +++ b/src/tools/os/fs-restore.ts @@ -13,10 +13,13 @@ const PREVIEW_MAX_LEN = 400; /** * `os.fs.restore { path }` — put back the previous content that - * `os.fs.write` / `edit` / `patch` saved before replacing a user file this - * session (see `fs-replace-guard.ts`). A write in every sense, so it - * rides the same approval ladder; the copy stays in the store, so a - * second restore of the same path still works. + * `os.fs.write` / `edit` / `patch` saved before replacing a user file in + * this working directory (see `fs-replace-guard.ts`). Whichever session + * replaced it: the copies are shared per working directory (F43), so a + * fusion worker restores what an earlier worker of another fan-out + * replaced. A write in every sense, so it rides the same approval + * ladder; the copy stays in the store, so a second restore of the same + * path still works. */ export function buildOsFsRestoreTool( options: FsDangerousToolOptions, @@ -24,7 +27,7 @@ export function buildOsFsRestoreTool( return { name: "os.fs.restore", description: - "Bring back the previous content of a file this session replaced or shrank (saved automatically by os.fs.write / os.fs.edit / os.fs.patch). Dangerous — always requires approval.", + "Bring back the previous content of a file that os.fs.write / os.fs.edit / os.fs.patch replaced or shrank in this working directory — by this session or another (a fusion worker's included); the copy is saved automatically. Dangerous — always requires approval.", readonly: false, async run(rawArgs, ctx) { const path = rawArgs.path; @@ -38,15 +41,15 @@ export function buildOsFsRestoreTool( ); } const absolute = resolveUserPath(path, ctx.workingDir); - const copy = await store.latestCopy(ctx.sessionId, absolute); + const copy = await store.latestCopy(ctx.workingDir, absolute); if (copy === null) { throw new Error( - `os.fs.restore: nothing saved for \`${path}\` in this session — only a pre-existing file replaced by os.fs.write / edit / patch has a copy`, + `os.fs.restore: nothing saved for \`${path}\` in this working directory — only a pre-existing file replaced by os.fs.write / edit / patch (by any session working here) has a copy`, ); } let content: Buffer; try { - content = await store.readCopy(ctx.sessionId, copy); + content = await store.readCopy(ctx.workingDir, copy); } catch { throw new Error( `os.fs.restore: the saved copy of \`${path}\` is gone (${copy.file})`, @@ -86,6 +89,7 @@ export function buildOsFsRestoreTool( lines: copy.lines, savedAt: copy.savedAt, savedBefore: copy.tool, + savedBy: copy.sessionId, copy: copy.file, }, }); diff --git a/src/tools/os/fs-write.ts b/src/tools/os/fs-write.ts index 0cdc4cda..a4aafea2 100644 --- a/src/tools/os/fs-write.ts +++ b/src/tools/os/fs-write.ts @@ -131,6 +131,7 @@ export function buildOsFsWriteTool( ? await guardReplacedFile({ store: options.restore, sessionId: ctx.sessionId, + workingDir: ctx.workingDir, absolute: target, display: target === absolute ? path : target, tool: "os.fs.write", diff --git a/src/tools/os/index.ts b/src/tools/os/index.ts index 24559b35..89544b90 100644 --- a/src/tools/os/index.ts +++ b/src/tools/os/index.ts @@ -155,7 +155,8 @@ export function registerOsTools( ); // One option bag for every tool that replaces file content, so the // write, the edit, the patch and the restore share the store that - // remembers what this session created and what it replaced. + // remembers what each session created and what was replaced in each + // working directory (by any session — a fusion worker's included). const fsMutation = { approvals: options.approvals, approvalRequired: options.approvalRequired, From fc58bbcbfeff2534b70f89e81bdf944801ef9540 Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Tue, 15 Sep 2026 11:52:36 +0300 Subject: [PATCH 3/6] fix(fusion): F44 fusion.delegate reports every problem at once, defaults task titles, and warns instead of refusing an unmatched requires --- src/prompt/default-tool-args-schemas.ts | 3 +- src/prompt/default-tool-descriptors-b.ts | 2 +- src/tools/fusion/contract-checks.test.ts | 18 +- src/tools/fusion/contract-checks.ts | 49 +-- src/tools/fusion/contract.test.ts | 65 ++++ src/tools/fusion/contract.ts | 139 +++++++- src/tools/fusion/delegate-args.test.ts | 187 ++++++++-- src/tools/fusion/delegate-args.ts | 431 ++++++++++++++--------- src/tools/fusion/fusion-delegate.test.ts | 110 ++++++ src/tools/fusion/fusion-delegate.ts | 12 +- src/tools/fusion/index.ts | 9 + src/tools/fusion/worker-prompt.test.ts | 48 +++ 12 files changed, 824 insertions(+), 249 deletions(-) diff --git a/src/prompt/default-tool-args-schemas.ts b/src/prompt/default-tool-args-schemas.ts index baa1eb26..71d6411b 100644 --- a/src/prompt/default-tool-args-schemas.ts +++ b/src/prompt/default-tool-args-schemas.ts @@ -711,12 +711,13 @@ const DEFAULT_TOOL_ARGS_SCHEMAS: ReadonlyMap = new Map< items: obj( { id: stringSchema, + // Optional since F44: the tool defaults it to the id. title: stringSchema, instructions: stringSchema, deliverable: stringSchema, files: { ...stringArraySchema, maxItems: 32 }, }, - ["id", "title", "instructions"], + ["id", "instructions"], ), }, // No upper bound: the orchestrator sizes its own fan-out and diff --git a/src/prompt/default-tool-descriptors-b.ts b/src/prompt/default-tool-descriptors-b.ts index 090deb14..f3047cfc 100644 --- a/src/prompt/default-tool-descriptors-b.ts +++ b/src/prompt/default-tool-descriptors-b.ts @@ -260,7 +260,7 @@ export const DEFAULT_TOOL_DESCRIPTORS_B: readonly ToolDescriptor[] = [ summary: "Delegate independent parts of the work to local worker agents that run concurrently and report back. Each task becomes one throwaway worker turn that sees the operator's original request and your `instructions`, but nothing else from this conversation, so `instructions` must carry what the request does not (exact paths, the contract between parts, acceptance criteria, the answer format you want). Returns every worker's reply plus a per-task status. You choose `maxWorkers`; it is bounded only by the task count and the machine. Call it on its own, never alongside other tool calls.", argsSchema: - '{ tasks: [{ id: string, title: string, instructions: string, deliverable?: string, files?: string[] }] /* 1..8 */, maxWorkers?: number /* how many run at once; you decide */, contract?: { owners?: { [path]: taskId }, provides?: [{ task, kind: "symbol"|"file"|"id"|"endpoint"|"env"|"flag"|"other", name, in?: path }], requires?: [{ task, name /* a provides name */ }], checks?: [{ task?, ...verify.run args }] } /* the interface between the parts: shown to every worker, checked after the fan-out */ }', + '{ tasks: [{ id: string, instructions: string, title?: string /* defaults to the id */, deliverable?: string, files?: string[] }] /* 1..8 */, maxWorkers?: number /* how many run at once; you decide */, contract?: { owners?: { [path]: taskId }, provides?: [{ task, kind: "symbol"|"file"|"id"|"endpoint"|"env"|"flag"|"other", name, in?: path }], requires?: [{ task, name /* a provides name */ }], checks?: [{ task?, ...verify.run args }] } /* the interface between the parts: shown to every worker, checked after the fan-out */ }', examples: [ '{"tasks":[{"id":"t1","title":"Map the auth routes","instructions":"List every route under src/http/ that touches auth. For each: path, method, and the middleware it runs.","deliverable":"one bullet per route"},{"id":"t2","title":"Summarise the session store","instructions":"Read src/session/session-store.ts and describe its public API and persistence model.","files":["src/session/session-store.ts"]}]}', ], diff --git a/src/tools/fusion/contract-checks.test.ts b/src/tools/fusion/contract-checks.test.ts index fd2a4a62..c98a2529 100644 --- a/src/tools/fusion/contract-checks.test.ts +++ b/src/tools/fusion/contract-checks.test.ts @@ -89,6 +89,9 @@ describe("inspectContractProvides", () => { { task: "ship", kind: "file", name: "js/ship.js" }, { task: "ship", kind: "file", name: "js/hud.js" }, { task: "ship", kind: "symbol", name: "X", in: "js/nope.js" }, + // Nowhere to look: the parser's warning, not a finding — a + // "missing" verdict over a search that never happened would + // read as the worker's failure. { task: "lost", kind: "symbol", name: "Y" }, ], }; @@ -101,16 +104,13 @@ describe("inspectContractProvides", () => { ["js/ship.js", true, ["js/ship.js"]], ["js/hud.js", false, ["js/hud.js"]], ["X", false, ["js/nope.js"]], - ["Y", false, []], ]); expect(findings[6]!.detail).toBe("file missing or unreadable"); - expect(findings[7]!.detail).toContain("no file to look in"); expect(describeMissing(findings[1]!)).toBe( "[ship] symbol HD.Ship.fire not in js/ship.js", ); expect(describeMissing(findings[2]!)).toBe("[html] id btn-launch not in index.html"); expect(describeMissing(findings[5]!)).toBe("[ship] file js/hud.js does not exist"); - expect(describeMissing(findings[7]!)).toContain("nowhere (no file to look in"); }); it("is satisfied by any one of several owned paths", async () => { @@ -289,7 +289,19 @@ describe("renderContractLine", () => { ).toBe("contract: 2 checks not run — no check runner is wired"); }); + it("carries the warnings the call ran with, after everything else", () => { + const warning = + 'requires "organized_files" (task index) has no provider — nothing produces it'; + expect( + renderContractLine({ findings: [], checks: [], warnings: [warning] }), + ).toBe(`contract: ${warning}`); + expect( + renderContractLine({ findings: [present], checks: [], warnings: [warning] }), + ).toBe(`contract: all 1 provide present; ${warning}`); + }); + it("is absent when there was nothing to report", () => { expect(renderContractLine({ findings: [], checks: [] })).toBeUndefined(); + expect(renderContractLine({ findings: [], checks: [], warnings: [] })).toBeUndefined(); }); }); diff --git a/src/tools/fusion/contract-checks.ts b/src/tools/fusion/contract-checks.ts index 1387c7cb..7fbff97c 100644 --- a/src/tools/fusion/contract-checks.ts +++ b/src/tools/fusion/contract-checks.ts @@ -3,7 +3,7 @@ import { readFile, stat } from "node:fs/promises"; import { resolveUserPath } from "../os/expand-home.js"; import { describeProvide, - ownedPaths, + provideSearchPaths, type ContractCheck, type ContractProvide, type DelegateContract, @@ -65,6 +65,12 @@ export interface ContractReport { checks: ContractCheckOutcome[]; /** Why the checks did not run, when they did not. */ checksSkipped?: string; + /** + * What the contract declared that could not be honoured and was run + * anyway — a `requires` no task provides (`contractWarnings`). The + * workers were told; this is the orchestrator's copy. + */ + warnings?: string[]; } /** Files above this are not searched; a provide is not that big. */ @@ -74,8 +80,6 @@ const CHECK_DETAIL_CHARS = 400; /** Bound on the `contract:` line of the status table. */ const CONTRACT_LINE_CHARS = 1200; -const GLOB_CHARS = /[*?[\]{}]/; - function escapeRegExp(text: string): string { return text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); } @@ -100,24 +104,13 @@ export function contentProvides( return content.includes(provide.name); } -/** Where a non-file provide is looked for: `in`, else owned paths, else declared files. */ -function searchPaths( - provide: ContractProvide, - contract: DelegateContract, - task: DelegateTask | undefined, -): string[] { - if (provide.in !== undefined) return [provide.in]; - const owned = ownedPaths(contract, provide.task).filter( - (p) => !GLOB_CHARS.test(p), - ); - if (owned.length > 0) return owned; - return (task?.files ?? []).filter((f) => !GLOB_CHARS.test(f)); -} - /** * Check every provide against the working directory. Never throws: a * path that cannot be read counts as not providing, with the reason on - * the finding. + * the finding. A non-file provide with nowhere to be looked for gets no + * finding at all — it is the parser's warning (`contractWarnings`), on + * the `contract:` line already, and a "missing" verdict over a search + * that never happened would read as the worker's failure. */ export async function inspectContractProvides( contract: DelegateContract, @@ -159,16 +152,8 @@ export async function inspectContractProvides( continue; } const task = tasks.find((t) => t.id === provide.task); - const where = searchPaths(provide, contract, task); - if (where.length === 0) { - findings.push({ - ...base, - where, - present: false, - detail: "no file to look in (give `in` or an owners entry)", - }); - continue; - } + const where = provideSearchPaths(provide, contract, task); + if (where.length === 0) continue; let present = false; let unreadable = 0; for (const path of where) { @@ -318,8 +303,11 @@ export function applyCheckOutcomes( /** * The `contract:` line of the status table — presence first, then the - * checks that belong to no task, then why the checks did not run. - * Nothing when the contract declared nothing checkable. + * checks that belong to no task, then why the checks did not run, then + * the warnings the call was run with (an unprovided require, so the + * orchestrator fixes the contract on its next call instead of wondering + * why a worker never found it). Nothing when the contract declared + * nothing checkable and raised no warning. */ export function renderContractLine(report: ContractReport): string | undefined { const parts: string[] = []; @@ -348,6 +336,7 @@ export function renderContractLine(report: ContractReport): string | undefined { ); } if (report.checksSkipped !== undefined) parts.push(report.checksSkipped); + parts.push(...(report.warnings ?? [])); if (parts.length === 0) return undefined; return `contract: ${head(parts.join("; "), CONTRACT_LINE_CHARS)}`; } diff --git a/src/tools/fusion/contract.test.ts b/src/tools/fusion/contract.test.ts index edd603c7..b3ea1cd0 100644 --- a/src/tools/fusion/contract.test.ts +++ b/src/tools/fusion/contract.test.ts @@ -1,10 +1,14 @@ import { describe, expect, it } from "vitest"; import { + contractWarnings, MAX_CONTRACT_RENDERED_CHARS, ownedPaths, + provideSearchPaths, renderContractBlock, renderContractForTask, + uncheckableProvides, + unprovidedRequires, type DelegateContract, } from "./contract.js"; @@ -93,3 +97,64 @@ describe("renderContractForTask", () => { ).toEqual(["b", "a"]); }); }); + +describe("contract warnings", () => { + // What a live Gemma 4 31B orchestrator wrote: a require nothing + // provides, and a provide with nowhere to be looked for. Each was a + // refusal costing minutes; both are notes now. + const LOOSE: DelegateContract = { + provides: [ + { task: "html", kind: "id", name: "btn-launch", in: "index.html" }, + { task: "organize", kind: "other", name: "done" }, + ], + requires: [ + { task: "main", name: "btn-launch" }, + { task: "index", name: "organized_files" }, + ], + }; + const TASKS = [{ id: "html" }, { id: "organize" }, { id: "main" }, { id: "index" }]; + const PROVIDE_NOTE = + 'provides "done" (task organize) cannot be checked: no `in`, no owned path, no declared files'; + const REQUIRE_NOTE = + 'requires "organized_files" (task index) has no provider — nothing produces it'; + + it("names the uncheckable provide and the unprovided require, in that order", () => { + expect(uncheckableProvides(LOOSE, TASKS)).toEqual([LOOSE.provides![1]]); + expect(unprovidedRequires(LOOSE)).toEqual([LOOSE.requires![1]]); + expect(contractWarnings(LOOSE, TASKS)).toEqual([PROVIDE_NOTE, REQUIRE_NOTE]); + expect(contractWarnings(CONTRACT, [])).toEqual([]); + }); + + it("looks for a provide in `in`, else the owned paths, else the declared files — never a glob", () => { + const provide = { task: "t", kind: "symbol" as const, name: "X" }; + expect(provideSearchPaths({ ...provide, in: "a.js" }, { owners: { "b.js": "t" } }, { id: "t", files: ["c.js"] })).toEqual(["a.js"]); + expect(provideSearchPaths(provide, { owners: { "b.js": "t", "js/**": "t" } }, { id: "t", files: ["c.js"] })).toEqual(["b.js"]); + expect(provideSearchPaths(provide, {}, { id: "t", files: ["c.js", "d/*.js"] })).toEqual(["c.js"]); + expect(provideSearchPaths(provide, { owners: { "js/**": "t" } }, { id: "t", files: ["d/*.js"] })).toEqual([]); + expect(provideSearchPaths(provide, {}, undefined)).toEqual([]); + // A file provide is its own path and is never uncheckable. + expect(uncheckableProvides({ provides: [{ task: "t", kind: "file", name: "x.txt" }] }, [])).toEqual([]); + }); + + it("the block keeps the uncheckable provide as declared, drops the unprovided require from REQUIRES and appends both notes", () => { + const block = renderContractBlock({ ...LOOSE, warnings: contractWarnings(LOOSE, TASKS) }); + expect(block).toContain("- [organize] other done"); + expect(block).toContain("REQUIRES:\n- [main] btn-launch\n"); + expect(block).not.toContain("- [index] organized_files"); + expect(block.split("\n").slice(-2)).toEqual([ + `contract: ${PROVIDE_NOTE}`, + `contract: ${REQUIRE_NOTE}`, + ]); + // Without stored warnings the block says nothing — the parser is the one that stores them. + expect(renderContractBlock(LOOSE)).not.toContain("contract: "); + }); + + it("a task's own line drops a require nobody provides", () => { + expect(renderContractForTask(LOOSE, "index")).toContain( + "You may rely on: nothing from the other parts", + ); + expect(renderContractForTask(LOOSE, "main")).toContain( + "You may rely on: btn-launch (id from html in index.html)", + ); + }); +}); diff --git a/src/tools/fusion/contract.ts b/src/tools/fusion/contract.ts index a7ee3195..737026b7 100644 --- a/src/tools/fusion/contract.ts +++ b/src/tools/fusion/contract.ts @@ -17,6 +17,22 @@ * Presence is all this module can promise. A grep sees that `HD.Ship` * appears in `js/ship.js`; whether it is a class or an object is what * `checks` — a runtime — is for. + * + * Two things the contract can declare are *warnings*, not refusals + * (F44): a `requires` entry that names nothing any task provides, and a + * non-file `provides` entry with nowhere to be looked for (no `in`, no + * owned path, no declared file). Each used to reject the whole call, + * and a local orchestrator at ~5 tok/s paid four minutes of generation + * per refusal to learn about one name — the third and fourth + * consecutive refusals of one afternoon. Nothing about either stops the + * fan-out from running: `contractWarnings` names them, the parser + * stores them on the contract, `renderContractBlock` appends them to + * the block every worker reads (the per-task "You may rely on" line + * drops an unprovided require; an uncheckable provide stays listed as + * declared), the presence check skips what it cannot check, and the + * same notes come back on the result's `contract:` line so the + * orchestrator can fix the contract on its next call, with the work + * already done. */ /** What a `provides` entry can name. `file` uses `name` as the path. */ @@ -45,7 +61,10 @@ export interface ContractProvide { export interface ContractRequire { /** The task that relies on it. */ task: string; - /** Matches a `provides[].name` exactly. */ + /** + * Should match a `provides[].name` exactly. One that matches none is + * carried through as written and reported by `contractWarnings`. + */ name: string; } @@ -61,6 +80,15 @@ export interface DelegateContract { provides?: ContractProvide[]; requires?: ContractRequire[]; checks?: ContractCheck[]; + /** + * What the contract declares that cannot be honoured, and the call + * ran with anyway — `contractWarnings`, computed once by + * `parseDelegateArgs` because one of them needs the tasks' declared + * files. Rendered at the end of every worker's block and carried to + * the result's `contract:` line and `details.contract.warnings`. A + * contract built by hand carries none unless it says so. + */ + warnings?: string[]; } export const MAX_CONTRACT_PROVIDES = 64; @@ -87,6 +115,89 @@ export function ownedPaths( .map(([path]) => path); } +/** Globs are patterns, not paths — never somewhere a provide can be looked for. */ +const GLOB_CHARS = /[*?[\]{}]/; + +/** The one thing a task contributes to where its provides are looked for. */ +export interface ContractTaskFiles { + id: string; + files?: readonly string[]; +} + +/** + * Where a non-file provide is looked for: `in`, else the paths its task + * owns, else the files its task declared — globs excluded at every + * step. Empty means it cannot be checked at all; the parser warns about + * that and the presence check skips it, both through this one rule. + */ +export function provideSearchPaths( + provide: ContractProvide, + contract: DelegateContract, + task: ContractTaskFiles | undefined, +): string[] { + if (provide.in !== undefined) return [provide.in]; + const owned = ownedPaths(contract, provide.task).filter( + (p) => !GLOB_CHARS.test(p), + ); + if (owned.length > 0) return owned; + return (task?.files ?? []).filter((f) => !GLOB_CHARS.test(f)); +} + +/** The non-file `provides` entries with nowhere to be looked for, in declaration order. */ +export function uncheckableProvides( + contract: DelegateContract, + tasks: readonly ContractTaskFiles[], +): ContractProvide[] { + return (contract.provides ?? []).filter( + (p) => + p.kind !== "file" && + provideSearchPaths( + p, + contract, + tasks.find((t) => t.id === p.task), + ).length === 0, + ); +} + +/** `provides "done" (task organize) cannot be checked: no \`in\`, no owned path, no declared files` */ +export function describeUncheckableProvide(provide: ContractProvide): string { + return `provides "${provide.name}" (task ${provide.task}) cannot be checked: no \`in\`, no owned path, no declared files`; +} + +function isProvided(contract: DelegateContract, require: ContractRequire): boolean { + return (contract.provides ?? []).some((p) => p.name === require.name); +} + +/** The `requires` entries no `provides` entry satisfies, in declaration order. */ +export function unprovidedRequires( + contract: DelegateContract, +): ContractRequire[] { + return (contract.requires ?? []).filter((r) => !isProvided(contract, r)); +} + +/** `requires "organized_files" (task index) has no provider — nothing produces it` */ +export function describeUnprovidedRequire(require: ContractRequire): string { + return `requires "${require.name}" (task ${require.task}) has no provider — nothing produces it`; +} + +/** + * What the contract declares that cannot be honoured, one line each, + * without the `contract:` prefix — the result's `contract:` line and + * `details.contract.warnings` carry them as they are; the worker's + * block prefixes them itself. Provides first, then requires, each in + * declaration order. Empty for a contract with nothing to warn about, + * so a caller can test the length. + */ +export function contractWarnings( + contract: DelegateContract, + tasks: readonly ContractTaskFiles[], +): string[] { + return [ + ...uncheckableProvides(contract, tasks).map(describeUncheckableProvide), + ...unprovidedRequires(contract).map(describeUnprovidedRequire), + ]; +} + function renderCheck(check: ContractCheck): string { const { task, ...spec } = check; const json = JSON.stringify(spec); @@ -99,7 +210,10 @@ function renderCheck(check: ContractCheck): string { /** * The block shared by every worker: all owners, provides, requires and - * checks. Measured against `MAX_CONTRACT_RENDERED_CHARS` at parse time. + * checks, then the contract's `warnings`, one `contract: …` line each, + * so no worker waits for or goes looking for something no sibling was + * asked to make. Measured against `MAX_CONTRACT_RENDERED_CHARS` at + * parse time, warnings included. */ export function renderContractBlock(contract: DelegateContract): string { const lines = [ @@ -119,7 +233,12 @@ export function renderContractBlock(contract: DelegateContract): string { ...provides.map((p) => `- [${p.task}] ${describeProvide(p)}`), ); } - const requires = contract.requires ?? []; + // Only the requires somebody provides are listed as requirements; an + // unmatched one is the note at the end, not a dependency a worker + // could wait on. + const requires = (contract.requires ?? []).filter((r) => + isProvided(contract, r), + ); if (requires.length > 0) { const byTask = new Map(); for (const r of requires) { @@ -137,13 +256,17 @@ export function renderContractBlock(contract: DelegateContract): string { ...checks.map(renderCheck), ); } + lines.push(...(contract.warnings ?? []).map((w) => `contract: ${w}`)); return lines.join("\n"); } /** * The three lines that turn the shared block into this worker's own * obligations. A require is resolved to the provide it names so the - * worker knows who produces it and where to find it. + * worker knows who produces it and where to find it; one that resolves + * to nothing is left off the line (the block's note covers it), because + * "you may rely on X" over an X nobody makes is a promise to a worker + * that cannot ask. */ export function renderContractForTask( contract: DelegateContract, @@ -153,11 +276,13 @@ export function renderContractForTask( const provides = (contract.provides ?? []).filter((p) => p.task === taskId); const relies = (contract.requires ?? []) .filter((r) => r.task === taskId) - .map((r) => { + .flatMap((r) => { const source = (contract.provides ?? []).find((p) => p.name === r.name); return source === undefined - ? r.name - : `${r.name} (${source.kind} from ${source.task}${source.in === undefined ? "" : ` in ${source.in}`})`; + ? [] + : [ + `${r.name} (${source.kind} from ${source.task}${source.in === undefined ? "" : ` in ${source.in}`})`, + ]; }); return [ `You own: ${owned.length > 0 ? owned.join(", ") : "no path in this contract — write only the files your TASK names"}`, diff --git a/src/tools/fusion/delegate-args.test.ts b/src/tools/fusion/delegate-args.test.ts index 63cbeb96..683be1fc 100644 --- a/src/tools/fusion/delegate-args.test.ts +++ b/src/tools/fusion/delegate-args.test.ts @@ -6,8 +6,10 @@ import { MAX_CONTRACT_RENDERED_CHARS, } from "./contract.js"; import { + humaniseTaskId, MAX_DELEGATE_TASKS, MAX_INSTRUCTIONS_CHARS, + MAX_REPORTED_PROBLEMS, MAX_TASK_FILES, parseDelegateArgs, } from "./delegate-args.js"; @@ -89,18 +91,38 @@ describe("parseDelegateArgs", () => { expect(error).toContain("not unique"); }); - it("rejects blank ids, titles and instructions", () => { + it("rejects blank ids and instructions", () => { expect( expectError(parseDelegateArgs({ tasks: [task({ id: " " })] })), ).toContain("id must be a non-empty string"); - expect( - expectError(parseDelegateArgs({ tasks: [task({ title: "" })] })), - ).toContain("title must be a non-empty string"); expect( expectError(parseDelegateArgs({ tasks: [task({ instructions: null })] })), ).toContain("instructions must be a non-empty string"); }); + it("defaults a missing title to the humanised id rather than refusing", () => { + // Every task of one live call lacked `title`, and the refusal cost a + // ~5 tok/s orchestrator four minutes of regeneration for a label. + for (const over of [{}, { title: "" }, { title: " " }, { title: 7 }]) { + const parsed = parseDelegateArgs({ + tasks: [{ id: "fix_main_sync", instructions: "Fix it.", ...over }], + }); + expect(parsed).toEqual({ + ok: true, + tasks: [ + { id: "fix_main_sync", title: "fix main sync", instructions: "Fix it." }, + ], + }); + } + // A given title still wins. + expect( + parseDelegateArgs({ tasks: [task({ id: "fix_main_sync" })] }), + ).toMatchObject({ tasks: [{ title: "Read the router" }] }); + expect(humaniseTaskId("fix-main-sync")).toBe("fix main sync"); + expect(humaniseTaskId("t1")).toBe("t1"); + expect(humaniseTaskId("___")).toBe("___"); + }); + it(`rejects instructions longer than ${MAX_INSTRUCTIONS_CHARS} chars`, () => { const error = expectError( parseDelegateArgs({ @@ -156,13 +178,75 @@ describe("parseDelegateArgs", () => { ), ).toContain(`at most ${MAX_TASK_FILES}`); expect( - expectError(parseDelegateArgs({ tasks: [task({ files: [1] })] })), - ).toContain("non-empty strings"); + expectError(parseDelegateArgs({ tasks: [task({ files: ["a.ts", 1] })] })), + ).toBe("validation: tasks[0].files[1] must be a non-empty string"); expect( expectError(parseDelegateArgs({ tasks: [task({ files: "a.ts" })] })), ).toContain("must be an array"); }); + it("reports every problem of the call in one message, not the first one met", () => { + // Three consecutive refusals, one problem each, cost a local + // orchestrator ~15 minutes before any worker ran. One message, one + // regeneration. + const error = expectError( + parseDelegateArgs({ + tasks: [ + { id: "a", title: "A" }, + { id: "b", instructions: "do b", files: ["ok.js", 3] }, + { id: "a", instructions: "dup" }, + ], + maxWorkers: 0, + contract: { + provides: [{ task: "ghost", kind: "file", name: "x" }], + requires: [{ task: "b" }], + }, + }), + ); + expect(error).toBe( + "validation: tasks[0].instructions must be a non-empty string; " + + "tasks[1].files[1] must be a non-empty string; " + + 'tasks[2].id "a" is not unique; ' + + "maxWorkers must be at least 1; " + + 'contract.provides[0].task names unknown task "ghost"; ' + + "contract.requires[0].name must be a non-empty string", + ); + }); + + it("checks the contract against every id that parsed, so one broken task does not cascade", () => { + // Task "a" lacks its instructions; a contract naming "a" is still + // bound to it, not reported as "unknown task" on top. + const error = expectError( + parseDelegateArgs({ + tasks: [{ id: "a" }, task({ id: "b" })], + contract: { owners: { "a.js": "a" }, provides: [{ task: "a", kind: "file", name: "a.js" }] }, + }), + ); + expect(error).toBe("validation: tasks[0].instructions must be a non-empty string"); + }); + + it(`spells out at most ${MAX_REPORTED_PROBLEMS} problems and counts the rest`, () => { + const requires = Array.from({ length: MAX_REPORTED_PROBLEMS + 5 }, () => ({ task: "t1" })); + const error = expectError(parseDelegateArgs({ tasks: [task()], contract: { requires } })); + expect(error.split("; ")).toHaveLength(MAX_REPORTED_PROBLEMS + 1); + expect(error).toMatch(/; … and 5 more problems$/); + }); + + it("still refuses what cannot run", () => { + for (const raw of [ + { tasks: [] }, + { tasks: [task({ instructions: "" })] }, + { tasks: Array.from({ length: MAX_DELEGATE_TASKS + 1 }, (_, i) => task({ id: `t${i}` })) }, + { tasks: [task({ files: Array.from({ length: MAX_TASK_FILES + 1 }, () => "a") })] }, + { tasks: [task()], contract: "nonsense" }, + { tasks: [task()], contract: { provides: [{ task: "t1", kind: "class", name: "a" }] } }, + ]) { + const result = parseDelegateArgs(raw as Record); + expect(result.ok, JSON.stringify(raw).slice(0, 80)).toBe(false); + expect(expectError(result)).toMatch(/^validation: /); + } + }); + it("rejects a nonsense maxWorkers but no longer an ambitious one", () => { // The orchestrator sizes its own fan-out, so a number wider than // this machine can go must run as wide as it can — not come back as @@ -314,49 +398,74 @@ describe("parseDelegateArgs — contract", () => { ); }); - it("rejects a require that no provide satisfies — the launch-btn / btn-launch mismatch, caught before any worker runs", () => { - const error = expectError( - parseDelegateArgs({ - tasks: TASKS, - contract: { - provides: [{ task: "html", kind: "id", name: "btn-launch", in: "index.html" }], - requires: [{ task: "main", name: "launch-btn" }], - }, - }), - ); - expect(error).toBe( - 'validation: contract.requires[0].name "launch-btn" matches no provides entry (provided: btn-launch)', - ); + it("carries a require that no provide satisfies through as a warning — the fan-out still runs", () => { + // The launch-btn / btn-launch mismatch used to refuse the call. It + // is still caught before any worker runs — as a note every worker + // and the orchestrator read — but no longer at the price of a + // regeneration; the workers can run without it. + const parsed = parseDelegateArgs({ + tasks: TASKS, + contract: { + provides: [{ task: "html", kind: "id", name: "btn-launch", in: "index.html" }], + requires: [{ task: "main", name: "launch-btn" }], + }, + }); + expect(parsed.ok).toBe(true); + expect(parsed.ok && parsed.contract).toEqual({ + provides: [{ task: "html", kind: "id", name: "btn-launch", in: "index.html" }], + requires: [{ task: "main", name: "launch-btn" }], + warnings: ['requires "launch-btn" (task main) has no provider — nothing produces it'], + }); }); - it("requires a place to look for a non-file provide", () => { - // `html` owns nothing and declares no files: a symbol it "provides" - // could only ever be reported unknown, so the brief is wrong now. - expect( - expectError( - parseDelegateArgs({ - tasks: TASKS, - contract: { provides: [{ task: "html", kind: "id", name: "x" }] }, - }), - ), - ).toContain( - 'contract.provides[0].in is required: task "html" owns no path and declares no files to look in', - ); - // An owned path, a declared file, or `in` each satisfy it; a glob does not. + it("carries a non-file provide with nowhere to look through as a warning — the fourth refusal of that afternoon", () => { + // `html` owns nothing and declares no files: an id it "provides" + // cannot be checked afterwards. The entry stays as declared; the + // presence check skips it; everyone is told. + const parsed = parseDelegateArgs({ + tasks: TASKS, + contract: { provides: [{ task: "html", kind: "id", name: "x" }] }, + }); + expect(parsed.ok && parsed.contract).toEqual({ + provides: [{ task: "html", kind: "id", name: "x" }], + warnings: ['provides "x" (task html) cannot be checked: no `in`, no owned path, no declared files'], + }); + // An owned path, a declared file, or `in` each make it checkable; a glob does not. for (const contract of [ { owners: { "index.html": "html" }, provides: [{ task: "html", kind: "id", name: "x" }] }, { provides: [{ task: "ship", kind: "symbol", name: "x" }] }, { provides: [{ task: "html", kind: "id", name: "x", in: "index.html" }] }, { provides: [{ task: "html", kind: "file", name: "index.html" }] }, ]) { - expect(parseDelegateArgs({ tasks: TASKS, contract }).ok).toBe(true); + const ok = parseDelegateArgs({ tasks: TASKS, contract }); + expect(ok.ok).toBe(true); + expect(ok.ok && ok.contract).not.toHaveProperty("warnings"); } - expect( + const glob = parseDelegateArgs({ + tasks: [task({ id: "g", files: ["js/**/*.js"] })], + contract: { provides: [{ task: "g", kind: "symbol", name: "x" }] }, + }); + expect(glob.ok && glob.contract?.warnings).toEqual([ + 'provides "x" (task g) cannot be checked: no `in`, no owned path, no declared files', + ]); + }); + + it("measures the rendered block with its warnings in it", () => { + const parsed = parseDelegateArgs({ + tasks: TASKS, + contract: { requires: [{ task: "main", name: "nothing" }] }, + }); + expect(parsed.ok && parsed.contract?.warnings).toHaveLength(1); + // Every warning is a line the workers pay for; the cap counts them. + const error = expectError( parseDelegateArgs({ - tasks: [task({ id: "g", files: ["js/**/*.js"] })], - contract: { provides: [{ task: "g", kind: "symbol", name: "x" }] }, - }).ok, - ).toBe(false); + tasks: TASKS, + contract: { + requires: Array.from({ length: 40 }, (_, i) => ({ task: "main", name: `${"n".repeat(200)}${i}` })), + }, + }), + ); + expect(error).toMatch(/^validation: contract renders to [\d,]+ chars; the limit is 8,000/); }); it(`caps provides at ${MAX_CONTRACT_PROVIDES}, checks at ${MAX_CONTRACT_CHECKS} and the rendered block at ${MAX_CONTRACT_RENDERED_CHARS} chars`, () => { diff --git a/src/tools/fusion/delegate-args.ts b/src/tools/fusion/delegate-args.ts index 12280302..a994e6c3 100644 --- a/src/tools/fusion/delegate-args.ts +++ b/src/tools/fusion/delegate-args.ts @@ -7,6 +7,23 @@ * a malformed delegation should cost the orchestrator one tool result * it can read and fix, not a failed turn. * + * One tool result, not three (F44). The parser used to stop at the + * first problem it met, and a local orchestrator at ~5 tok/s paid for + * that in whole minutes: three consecutive calls refused — a stray key, + * then a missing `title` on every task, then one unmatched `requires` + * name — each ~4–5 minutes of generation, before a single worker ran. + * Every problem of a call is now collected and reported in one message + * (`validation: tasks[0].instructions …; tasks[2].files[1] …; + * contract.requires[1] …`), so the model regenerates once with the whole + * list in front of it. Three of the four refusals seen that afternoon + * no longer happen at all: `title` defaults to the id (a label is not + * worth a regeneration), and an unmatched `requires` or a `provides` + * entry with nowhere to be looked for are warnings carried to the + * workers and the result (`contractWarnings` in `contract.ts`). What + * still refuses is what cannot run — no tasks, empty `instructions`, a + * limit exceeded, a wrong type — and the unknown-top-level-key refusal + * that sits in front of every tool (F40). + * * The caps are not style — each one bounds a real resource. Task count * bounds how many local turns one call can start, `instructions` bounds * the worker's prompt, and `files` bounds the paths pasted into it. @@ -38,12 +55,13 @@ import { MAX_CONTRACT_CHECKS, MAX_CONTRACT_PROVIDES, MAX_CONTRACT_RENDERED_CHARS, - ownedPaths, + contractWarnings, renderContractBlock, type ContractCheck, type ContractProvide, type ContractProvideKind, type ContractRequire, + type ContractTaskFiles, type DelegateContract, } from "./contract.js"; @@ -51,7 +69,13 @@ import { export interface DelegateTask { /** Orchestrator-chosen id, unique within the call. Echoed in the output. */ id: string; - /** One-line label. Shown to the operator in the progress feed. */ + /** + * One-line label. Shown to the operator in the progress feed and on + * the status table. Optional on the wire: a call that leaves it out + * (or sends something that is not a non-empty string) gets the id, + * humanised (`humaniseTaskId`), so every renderer has a label and no + * call is refused over one. + */ title: string; /** * The task's brief. Besides this the worker sees only the operator's @@ -76,71 +100,95 @@ export type ParsedDelegateArgs = export const MAX_DELEGATE_TASKS = 8; export const MAX_INSTRUCTIONS_CHARS = 32_000; export const MAX_TASK_FILES = 32; - -/** Globs are patterns, not paths — never somewhere a provide can be looked for. */ -const GLOB_CHARS = /[*?[\]{}]/; +/** + * How many problems one refusal spells out before it says "and N more". + * Bounds a hostile call (a thousand malformed `requires` entries), not a + * real one: eight tasks with every field wrong still fit under it. + */ +export const MAX_REPORTED_PROBLEMS = 32; /** `8436` → `"8,436"`: the number the orchestrator has to act on, readable. */ function formatCount(n: number): string { return n.toLocaleString("en-US"); } +/** + * `fix_main_sync` → `fix main sync`: the id as a label, for a task that + * named no title. Underscores and hyphens become spaces; anything else + * is kept as written, because the id is what the orchestrator will use + * to refer to the task and the label should still read as that id. + */ +export function humaniseTaskId(id: string): string { + const spaced = id.replace(/[_-]+/g, " ").replace(/\s+/g, " ").trim(); + return spaced.length > 0 ? spaced : id; +} + function fail(error: string): ParsedDelegateArgs { return { ok: false, error: `validation: ${error}` }; } +/** Every collected problem in one sentence, capped for a hostile call. */ +function failAll(problems: readonly string[]): ParsedDelegateArgs { + const shown = problems.slice(0, MAX_REPORTED_PROBLEMS); + const rest = problems.length - shown.length; + return fail( + shown.join("; ") + + (rest > 0 ? `; … and ${rest} more problem${rest === 1 ? "" : "s"}` : ""), + ); +} + function readString(value: unknown): string | null { if (typeof value !== "string") return null; const trimmed = value.trim(); return trimmed.length > 0 ? trimmed : null; } -function readFiles(value: unknown, taskLabel: string): string[] | string { +/** + * The task's `files`, every bad entry named by its index so the model + * fixes them all in the one regeneration it pays for. + */ +function readFiles( + value: unknown, + taskLabel: string, + problems: string[], +): string[] { if (value === undefined || value === null) return []; - if (!Array.isArray(value)) - return `${taskLabel}.files must be an array of strings`; + if (!Array.isArray(value)) { + problems.push(`${taskLabel}.files must be an array of strings`); + return []; + } if (value.length > MAX_TASK_FILES) { - return `${taskLabel}.files has ${value.length} entries; at most ${MAX_TASK_FILES}`; + problems.push( + `${taskLabel}.files has ${value.length} entries; at most ${MAX_TASK_FILES}`, + ); + return []; } const out: string[] = []; - for (const entry of value) { + for (const [j, entry] of value.entries()) { const path = readString(entry); - if (path === null) - return `${taskLabel}.files must contain non-empty strings`; + if (path === null) { + problems.push(`${taskLabel}.files[${j}] must be a non-empty string`); + continue; + } out.push(path); } return out; } -function readMaxWorkers(value: unknown): number | null | string { +function readMaxWorkers(value: unknown, problems: string[]): number | null { if (value === undefined || value === null) return null; if (typeof value !== "number" || !Number.isFinite(value)) { - return "maxWorkers must be a number"; + problems.push("maxWorkers must be a number"); + return null; } const n = Math.trunc(value); - if (n < 1) return "maxWorkers must be at least 1"; + if (n < 1) { + problems.push("maxWorkers must be at least 1"); + return null; + } return n; } -/** - * Parse and validate `fusion.delegate` args. Never throws; an invalid - * call comes back as `{ ok: false, error }` for the tool to render as a - * `status: "error"` result the orchestrator can act on. - */ -/** - * The task list, whether it arrived as an array or as JSON in a string. - * - * Models hand this argument over as a string often enough to matter: in - * one observed run, three of seven fan-outs died on - * `tasks must be an array`, each costing the turn a step and the - * operator a minute. The value was a perfectly good JSON array with - * quotes around it — the native-tools layer stringifies a nested - * structure, or the model writes it that way itself. - * - * Rejecting that is pedantry with a cost. Parsing it is two lines, and - * anything that does not parse to an array still fails exactly as - * before. - */ /** * Accept a `tasks` argument that arrived as JSON *text* rather than as a * JSON array. @@ -165,25 +213,32 @@ function isRecord(value: unknown): value is Record { return value !== null && typeof value === "object" && !Array.isArray(value); } +/** What the contract binds: the ids that parsed, and their declared files. */ +type BindableTask = ContractTaskFiles & Pick; + /** - * Validate `contract` against the tasks it binds. Every error names the - * field, because the orchestrator fixes exactly one thing per retry. + * Validate `contract` against the tasks it binds, collecting every + * problem by field. A broken entry is named and skipped; the rest of + * the contract is still checked, so one regeneration can fix it all. * - * Two checks go beyond shape. A `requires` name must match a `provides` - * name exactly — a require nobody provides is the `launch-btn` / - * `btn-launch` mismatch the contract exists to catch, and the cheapest - * moment to catch it is before any worker runs. And a non-file provide - * needs somewhere to be looked for: `in`, an owned path, or the task's - * declared files — otherwise the presence check after the fan-out - * would have nothing to open and could only report "unknown". + * Only shape is a problem here — a wrong type, an unknown task id, a + * limit. What the contract *means* is never refused: a `requires` name + * no `provides` entry matches, or a non-file provide with nowhere to + * be looked for, are carried through as declared and stored on the + * contract as `warnings` (`contractWarnings`), because nothing about + * either stops the workers from running. */ function readContract( raw: unknown, - tasks: readonly DelegateTask[], -): DelegateContract | undefined | string { + tasks: readonly BindableTask[], + problems: string[], +): DelegateContract | undefined { const value = readJsonArg(raw); if (value === undefined || value === null) return undefined; - if (!isRecord(value)) return "contract must be an object"; + if (!isRecord(value)) { + problems.push("contract must be an object"); + return undefined; + } const ids = new Set(tasks.map((t) => t.id)); const known = (task: unknown, field: string): string | null => { const id = readString(task); @@ -195,137 +250,174 @@ function readContract( if (value.owners !== undefined && value.owners !== null) { if (!isRecord(value.owners)) { - return "contract.owners must be an object of { path: taskId }"; - } - const owners: Record = {}; - for (const [path, task] of Object.entries(value.owners)) { - const key = path.trim(); - if (key.length === 0) return "contract.owners has an empty path"; - const bad = known(task, `contract.owners["${key}"]`); - if (bad !== null) return bad; - owners[key] = (task as string).trim(); + problems.push("contract.owners must be an object of { path: taskId }"); + } else { + const owners: Record = {}; + for (const [path, task] of Object.entries(value.owners)) { + const key = path.trim(); + if (key.length === 0) { + problems.push("contract.owners has an empty path"); + continue; + } + const bad = known(task, `contract.owners["${key}"]`); + if (bad !== null) { + problems.push(bad); + continue; + } + owners[key] = (task as string).trim(); + } + if (Object.keys(owners).length > 0) contract.owners = owners; } - if (Object.keys(owners).length > 0) contract.owners = owners; } if (value.provides !== undefined && value.provides !== null) { if (!Array.isArray(value.provides)) { - return "contract.provides must be an array of { task, kind, name, in? }"; - } - if (value.provides.length > MAX_CONTRACT_PROVIDES) { - return `contract.provides has ${value.provides.length} entries; at most ${MAX_CONTRACT_PROVIDES}`; - } - const provides: ContractProvide[] = []; - for (let i = 0; i < value.provides.length; i += 1) { - const entry: unknown = value.provides[i]; - const label = `contract.provides[${i}]`; - if (!isRecord(entry)) return `${label} must be an object`; - const bad = known(entry.task, `${label}.task`); - if (bad !== null) return bad; - const kind = readString(entry.kind); - if ( - kind === null || - !(CONTRACT_PROVIDE_KINDS as readonly string[]).includes(kind) - ) { - return `${label}.kind must be one of ${CONTRACT_PROVIDE_KINDS.join(", ")}`; - } - const name = readString(entry.name); - if (name === null) return `${label}.name must be a non-empty string`; - const inPath = - entry.in === undefined || entry.in === null - ? null - : readString(entry.in); - if (entry.in !== undefined && entry.in !== null && inPath === null) { - return `${label}.in must be a non-empty path`; + problems.push( + "contract.provides must be an array of { task, kind, name, in? }", + ); + } else if (value.provides.length > MAX_CONTRACT_PROVIDES) { + problems.push( + `contract.provides has ${value.provides.length} entries; at most ${MAX_CONTRACT_PROVIDES}`, + ); + } else { + const provides: ContractProvide[] = []; + for (let i = 0; i < value.provides.length; i += 1) { + const entry: unknown = value.provides[i]; + const label = `contract.provides[${i}]`; + if (!isRecord(entry)) { + problems.push(`${label} must be an object`); + continue; + } + const before = problems.length; + const bad = known(entry.task, `${label}.task`); + if (bad !== null) problems.push(bad); + const kind = readString(entry.kind); + if ( + kind === null || + !(CONTRACT_PROVIDE_KINDS as readonly string[]).includes(kind) + ) { + problems.push( + `${label}.kind must be one of ${CONTRACT_PROVIDE_KINDS.join(", ")}`, + ); + } + const name = readString(entry.name); + if (name === null) { + problems.push(`${label}.name must be a non-empty string`); + } + const inPath = + entry.in === undefined || entry.in === null + ? null + : readString(entry.in); + if (entry.in !== undefined && entry.in !== null && inPath === null) { + problems.push(`${label}.in must be a non-empty path`); + } + if (problems.length > before || kind === null || name === null) { + continue; + } + provides.push({ + task: (entry.task as string).trim(), + kind: kind as ContractProvideKind, + name, + ...(inPath === null ? {} : { in: inPath }), + }); } - provides.push({ - task: (entry.task as string).trim(), - kind: kind as ContractProvideKind, - name, - ...(inPath === null ? {} : { in: inPath }), - }); - } - if (provides.length > 0) contract.provides = provides; - } - - // A provide that is not a file must have somewhere to be looked for. - for (const [i, p] of (contract.provides ?? []).entries()) { - if (p.kind === "file" || p.in !== undefined) continue; - const task = tasks.find((t) => t.id === p.task); - const declared = (task?.files ?? []).filter((f) => !GLOB_CHARS.test(f)); - if (ownedPaths(contract, p.task).length === 0 && declared.length === 0) { - return `contract.provides[${i}].in is required: task "${p.task}" owns no path and declares no files to look in`; + if (provides.length > 0) contract.provides = provides; } } if (value.requires !== undefined && value.requires !== null) { if (!Array.isArray(value.requires)) { - return "contract.requires must be an array of { task, name }"; - } - const provided = new Set((contract.provides ?? []).map((p) => p.name)); - const requires: ContractRequire[] = []; - for (let i = 0; i < value.requires.length; i += 1) { - const entry: unknown = value.requires[i]; - const label = `contract.requires[${i}]`; - if (!isRecord(entry)) return `${label} must be an object`; - const bad = known(entry.task, `${label}.task`); - if (bad !== null) return bad; - const name = readString(entry.name); - if (name === null) return `${label}.name must be a non-empty string`; - if (!provided.has(name)) { - const names = [...provided].slice(0, 10).join(", "); - return ( - `${label}.name "${name}" matches no provides entry` + - (names.length > 0 ? ` (provided: ${names})` : "") - ); + problems.push("contract.requires must be an array of { task, name }"); + } else { + const requires: ContractRequire[] = []; + for (let i = 0; i < value.requires.length; i += 1) { + const entry: unknown = value.requires[i]; + const label = `contract.requires[${i}]`; + if (!isRecord(entry)) { + problems.push(`${label} must be an object`); + continue; + } + const before = problems.length; + const bad = known(entry.task, `${label}.task`); + if (bad !== null) problems.push(bad); + const name = readString(entry.name); + if (name === null) { + problems.push(`${label}.name must be a non-empty string`); + } + if (problems.length > before || name === null) continue; + requires.push({ task: (entry.task as string).trim(), name }); } - requires.push({ task: (entry.task as string).trim(), name }); + if (requires.length > 0) contract.requires = requires; } - if (requires.length > 0) contract.requires = requires; } if (value.checks !== undefined && value.checks !== null) { if (!Array.isArray(value.checks)) { - return "contract.checks must be an array of verify.run specs"; - } - if (value.checks.length > MAX_CONTRACT_CHECKS) { - return `contract.checks has ${value.checks.length} entries; at most ${MAX_CONTRACT_CHECKS}`; - } - const checks: ContractCheck[] = []; - for (let i = 0; i < value.checks.length; i += 1) { - const entry: unknown = value.checks[i]; - const label = `contract.checks[${i}]`; - if (!isRecord(entry)) return `${label} must be an object`; - const { task, ...spec } = entry; - if (Object.keys(spec).length === 0) { - return `${label} carries no verify.run arguments`; - } - if (task === undefined || task === null) { - checks.push(spec); - continue; + problems.push("contract.checks must be an array of verify.run specs"); + } else if (value.checks.length > MAX_CONTRACT_CHECKS) { + problems.push( + `contract.checks has ${value.checks.length} entries; at most ${MAX_CONTRACT_CHECKS}`, + ); + } else { + const checks: ContractCheck[] = []; + for (let i = 0; i < value.checks.length; i += 1) { + const entry: unknown = value.checks[i]; + const label = `contract.checks[${i}]`; + if (!isRecord(entry)) { + problems.push(`${label} must be an object`); + continue; + } + const { task, ...spec } = entry; + if (Object.keys(spec).length === 0) { + problems.push(`${label} carries no verify.run arguments`); + continue; + } + if (task === undefined || task === null) { + checks.push(spec); + continue; + } + const bad = known(task, `${label}.task`); + if (bad !== null) { + problems.push(bad); + continue; + } + checks.push({ task: (task as string).trim(), ...spec }); } - const bad = known(task, `${label}.task`); - if (bad !== null) return bad; - checks.push({ task: (task as string).trim(), ...spec }); + if (checks.length > 0) contract.checks = checks; } - if (checks.length > 0) contract.checks = checks; } if (Object.keys(contract).length === 0) return undefined; + // Stored before the block is measured: the workers pay for these + // lines like any other. + const warnings = contractWarnings(contract, tasks); + if (warnings.length > 0) contract.warnings = warnings; const rendered = renderContractBlock(contract).length; if (rendered > MAX_CONTRACT_RENDERED_CHARS) { const over = rendered - MAX_CONTRACT_RENDERED_CHARS; - return `contract renders to ${formatCount(rendered)} chars; the limit is ${formatCount(MAX_CONTRACT_RENDERED_CHARS)} — shorten it by at least ${formatCount(over)} chars`; + problems.push( + `contract renders to ${formatCount(rendered)} chars; the limit is ${formatCount(MAX_CONTRACT_RENDERED_CHARS)} — shorten it by at least ${formatCount(over)} chars`, + ); } return contract; } +/** + * Parse and validate `fusion.delegate` args. Never throws; an invalid + * call comes back as `{ ok: false, error }` for the tool to render as a + * `status: "error"` result the orchestrator can act on — one error + * naming every problem of the call, not the first one met. + * + * Only the shape of `tasks` itself (not an array, empty, over the cap) + * ends the parse on its own: there is nothing else to check against + * and the model has to re-plan, not patch fields. + */ export function parseDelegateArgs( raw: Record, ): ParsedDelegateArgs { const rawTasks = readJsonArg(raw.tasks); if (!Array.isArray(rawTasks)) { - return fail("tasks must be an array of { id, title, instructions }"); + return fail("tasks must be an array of { id, instructions, title? }"); } if (rawTasks.length === 0) return fail("tasks must contain at least one task"); @@ -335,52 +427,59 @@ export function parseDelegateArgs( ); } + const problems: string[] = []; const tasks: DelegateTask[] = []; + // Every task whose id parsed, whatever else was wrong with it: the + // contract is checked against the ids the orchestrator actually + // wrote, so a task missing its instructions does not also turn every + // contract entry naming it into an "unknown task" problem. + const bindable: BindableTask[] = []; const seen = new Set(); for (let i = 0; i < rawTasks.length; i += 1) { const entry = rawTasks[i]; const label = `tasks[${i}]`; - if (entry === null || typeof entry !== "object" || Array.isArray(entry)) { - return fail(`${label} must be an object`); + if (!isRecord(entry)) { + problems.push(`${label} must be an object`); + continue; } - const record = entry as Record; - const id = readString(record.id); - if (id === null) return fail(`${label}.id must be a non-empty string`); - if (seen.has(id)) return fail(`${label}.id "${id}" is not unique`); - seen.add(id); - const title = readString(record.title); - if (title === null) - return fail(`${label}.title must be a non-empty string`); - const instructions = readString(record.instructions); + const before = problems.length; + const id = readString(entry.id); + if (id === null) problems.push(`${label}.id must be a non-empty string`); + else if (seen.has(id)) problems.push(`${label}.id "${id}" is not unique`); + const instructions = readString(entry.instructions); if (instructions === null) { - return fail(`${label}.instructions must be a non-empty string`); - } - if (instructions.length > MAX_INSTRUCTIONS_CHARS) { + problems.push(`${label}.instructions must be a non-empty string`); + } else if (instructions.length > MAX_INSTRUCTIONS_CHARS) { // The limit AND the overage: a model told only "at most N" cannot // count its own output, so it resends something just as long. const over = instructions.length - MAX_INSTRUCTIONS_CHARS; - return fail( + problems.push( `${label}.instructions is ${formatCount(instructions.length)} chars; ` + `the limit is ${formatCount(MAX_INSTRUCTIONS_CHARS)} — shorten it by at least ${formatCount(over)} chars. ` + `The workers already receive the operator's original request, so do not restate it in the brief.`, ); } - const files = readFiles(record.files, label); - if (typeof files === "string") return fail(files); - const deliverable = readString(record.deliverable); + const files = readFiles(entry.files, label, problems); + const deliverable = readString(entry.deliverable); + if (id !== null && !seen.has(id)) { + seen.add(id); + bindable.push({ id, ...(files.length === 0 ? {} : { files }) }); + } + if (problems.length > before || id === null || instructions === null) { + continue; + } tasks.push({ id, - title, + title: readString(entry.title) ?? humaniseTaskId(id), instructions, ...(deliverable === null ? {} : { deliverable }), ...(files.length === 0 ? {} : { files }), }); } - const maxWorkers = readMaxWorkers(raw.maxWorkers); - if (typeof maxWorkers === "string") return fail(maxWorkers); - const contract = readContract(raw.contract, tasks); - if (typeof contract === "string") return fail(contract); + const maxWorkers = readMaxWorkers(raw.maxWorkers, problems); + const contract = readContract(raw.contract, bindable, problems); + if (problems.length > 0) return failAll(problems); return { ok: true, tasks, diff --git a/src/tools/fusion/fusion-delegate.test.ts b/src/tools/fusion/fusion-delegate.test.ts index 8b8c3a1b..42714de7 100644 --- a/src/tools/fusion/fusion-delegate.test.ts +++ b/src/tools/fusion/fusion-delegate.test.ts @@ -173,6 +173,28 @@ describe("fusion.delegate", () => { expect(pins).toEqual(["local-llama", "other-llama"]); }); + it("labels a task that named no title with its humanised id — in the table, the details and the feed", async () => { + // Every task of one live call lacked `title`; refusing it cost a + // ~5 tok/s orchestrator four minutes for a label. + const events: Array> = []; + const tool = buildFusionDelegateTool( + deps({ + emitEvent: (sessionId, event) => events.push({ sessionId, ...event }), + }), + ); + const result = await tool.run( + { tasks: [{ id: "fix_main_sync", instructions: "Fix it." }] }, + ctx(), + ); + expect(result.status).toBe("ok"); + expect(result.summary.split("\n")[1]).toBe("- [fix_main_sync] ok — fix main sync"); + const rows = result.details.tasks as WorkerTaskResult[]; + expect(rows[0]).toMatchObject({ id: "fix_main_sync", title: "fix main sync" }); + expect( + events.filter((e) => e.role === "worker").map((e) => e.title), + ).toEqual(["fix main sync", "fix main sync"]); + }); + it("brackets the fan-out with the orchestrator's own model", async () => { // Between these two lines every feed line belongs to a worker on the // local leg; the operator can otherwise only guess which model is @@ -848,6 +870,94 @@ describe("fusion.delegate", () => { } }); + it("runs a contract whose require has no provider and whose provide cannot be checked, warning everyone instead of refusing", async () => { + // Live, 2026-09-15: the third and fourth consecutive refusals of + // one fan-out, ~4–5 minutes of local generation each, were these + // two. Neither stops a worker from working, so the call runs and + // the notes travel with it — into every brief, onto the + // `contract:` line, into the details. + const dir = fixture(); + try { + const briefs: string[] = []; + const tool = buildFusionDelegateTool( + deps({ + workingDir: dir, + runTurn: async (_session, userMessage) => { + briefs.push(userMessage); + return turnResult(); + }, + }), + ); + const result = await tool.run( + { + tasks: [ + { id: "t1", title: "One", instructions: "x" }, + { id: "organize", instructions: "y" }, + ], + contract: { + provides: [ + { task: "t1", kind: "symbol", name: "HD.Ship", in: "js/ship.js" }, + { task: "organize", kind: "other", name: "done" }, + ], + requires: [{ task: "t1", name: "organized_files" }], + }, + }, + ctx({ workingDir: dir }), + ); + const provideNote = + 'provides "done" (task organize) cannot be checked: no `in`, no owned path, no declared files'; + const requireNote = + 'requires "organized_files" (task t1) has no provider — nothing produces it'; + expect(result.status).toBe("ok"); + expect(briefs).toHaveLength(2); + for (const brief of briefs) { + expect(brief).toContain(`contract: ${provideNote}`); + expect(brief).toContain(`contract: ${requireNote}`); + expect(brief).toContain("- [organize] other done"); + } + expect(briefs[0]).toContain("You may rely on: nothing from the other parts"); + const lines = result.summary.split("\n"); + expect(lines[0]).toBe("2 tasks: 2 ok"); + expect(lines[1]).toBe( + `contract: all 1 provide present; ${provideNote}; ${requireNote}`, + ); + expect(lines[2]).toBe("- [t1] ok — One"); + // The title-less task is labelled by its id. + expect(lines[3]).toBe("- [organize] ok — organize"); + const report = result.details.contract as { + findings: unknown[]; + warnings: string[]; + }; + // The uncheckable provide got no finding — nothing was searched. + expect(report.findings).toHaveLength(1); + expect(report.warnings).toEqual([provideNote, requireNote]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("refuses a malformed call with every problem named at once, before any worker runs", async () => { + const runTurn = vi.fn(async () => turnResult()); + const tool = buildFusionDelegateTool(deps({ runTurn })); + const result = await tool.run( + { + tasks: [ + { id: "a" }, + { id: "b", instructions: "x", files: ["ok.js", 3] }, + ], + contract: { provides: [{ task: "ghost", kind: "file", name: "x" }] }, + }, + ctx(), + ); + expect(result.status).toBe("error"); + expect(result.summary).toContain( + "validation: tasks[0].instructions must be a non-empty string; " + + "tasks[1].files[1] must be a non-empty string; " + + 'contract.provides[0].task names unknown task "ghost"', + ); + expect(runTurn).not.toHaveBeenCalled(); + }); + it("rejects a contract that does not bind the tasks, before any worker runs", async () => { const runTurn = vi.fn(async () => turnResult()); const tool = buildFusionDelegateTool(deps({ runTurn })); diff --git a/src/tools/fusion/fusion-delegate.ts b/src/tools/fusion/fusion-delegate.ts index 870aa89f..74e05510 100644 --- a/src/tools/fusion/fusion-delegate.ts +++ b/src/tools/fusion/fusion-delegate.ts @@ -112,7 +112,9 @@ function error( * Manage → LLM leaves fusion on the next read (§"Run modes"), and a * tool that kept fanning out would be spending on a leg the operator * just walked away from. - * 3. **Only on valid args.** See `parseDelegateArgs`. + * 3. **Only on valid args.** See `parseDelegateArgs`: the refusal + * names every problem of the call at once, so a slow local + * orchestrator regenerates once, not once per field. * * Once the call runs, its status summarises its tasks * (`details.outcome`): `ok` while any task delivered anything — partial @@ -161,7 +163,7 @@ export function buildFusionDelegateTool( return { name: FUSION_DELEGATE_TOOL, description: - "Delegate independent parts of the work to local worker agents that run concurrently. You choose how many run at once with `maxWorkers`. An optional `contract` (owners, provides, requires, checks) is prepended to every brief and checked after the fan-out. Args: { tasks: [{ id, title, instructions, deliverable?, files? }], maxWorkers?, contract? }.", + "Delegate independent parts of the work to local worker agents that run concurrently. You choose how many run at once with `maxWorkers`. An optional `contract` (owners, provides, requires, checks) is prepended to every brief and checked after the fan-out. Args: { tasks: [{ id, instructions, title?, deliverable?, files? }], maxWorkers?, contract? }.", readonly: false, async run(rawArgs, ctx): Promise { if (isFusionWorkerSessionId(ctx.sessionId)) { @@ -367,12 +369,18 @@ export function buildFusionDelegateTool( { workingDir: ctx.workingDir, signal: ctx.signal }, ); results = applyCheckOutcomes(results, checks.outcomes); + // What the call was run with despite the contract — a require + // nobody provides, a provide nothing can check. The workers read + // it in their block; the orchestrator reads it here, on the line + // and in the details. + const warnings = parsed.contract.warnings ?? []; contract = { findings, checks: checks.outcomes, ...(checks.checksSkipped === undefined ? {} : { checksSkipped: checks.checksSkipped }), + ...(warnings.length === 0 ? {} : { warnings }), }; } const contractLine = diff --git a/src/tools/fusion/index.ts b/src/tools/fusion/index.ts index 0fb843aa..e998af7f 100644 --- a/src/tools/fusion/index.ts +++ b/src/tools/fusion/index.ts @@ -5,9 +5,11 @@ export { FUSION_WORKER_APPROVAL_MARKER, } from "./worker-tool-policy.js"; export { + humaniseTaskId, parseDelegateArgs, MAX_DELEGATE_TASKS, MAX_INSTRUCTIONS_CHARS, + MAX_REPORTED_PROBLEMS, MAX_TASK_FILES, } from "./delegate-args.js"; export type { DelegateTask, ParsedDelegateArgs } from "./delegate-args.js"; @@ -16,16 +18,23 @@ export { MAX_CONTRACT_CHECKS, MAX_CONTRACT_PROVIDES, MAX_CONTRACT_RENDERED_CHARS, + contractWarnings, describeProvide, + describeUncheckableProvide, + describeUnprovidedRequire, ownedPaths, + provideSearchPaths, renderContractBlock, renderContractForTask, + uncheckableProvides, + unprovidedRequires, } from "./contract.js"; export type { ContractCheck, ContractProvide, ContractProvideKind, ContractRequire, + ContractTaskFiles, DelegateContract, } from "./contract.js"; export { diff --git a/src/tools/fusion/worker-prompt.test.ts b/src/tools/fusion/worker-prompt.test.ts index fd226122..01c67698 100644 --- a/src/tools/fusion/worker-prompt.test.ts +++ b/src/tools/fusion/worker-prompt.test.ts @@ -4,6 +4,7 @@ import { assistantReplyTurn, userTurn, } from "../../session/conversation-turn.js"; +import { parseDelegateArgs } from "./delegate-args.js"; import { FOLLOW_UP_MAX_CHARS, ORIGINAL_REQUEST_CHAR_BUDGET, @@ -37,6 +38,15 @@ describe("renderWorkerBrief", () => { expect(brief).toContain(TASK.instructions); }); + it("labels a task that named no title with its humanised id", () => { + const parsed = parseDelegateArgs({ + tasks: [{ id: "fix_main_sync", instructions: "Fix it." }], + }); + if (!parsed.ok) throw new Error(parsed.error); + const brief = renderWorkerBrief(parsed.tasks[0]!, { workingDir: "/repo" }); + expect(brief).toContain("TASK fix_main_sync: fix main sync"); + }); + it("renders deliverable and files only when present", () => { const bare = renderWorkerBrief(TASK, { workingDir: "/repo" }); expect(bare).not.toContain("DELIVERABLE:"); @@ -154,6 +164,44 @@ describe("renderWorkerBrief — the contract", () => { expect(brief.split("\n")[0]).toContain("You are a worker agent"); }); + it("tells every worker what the contract declares but nothing can honour", () => { + // A require nothing provides and a provide nothing can check used + // to refuse the call; now they are notes at the end of the block a + // worker reads, so it neither waits for the one nor is judged on + // the other. + const parsed = parseDelegateArgs({ + tasks: [ + { id: "t1", instructions: "x" }, + { id: "organize", instructions: "y" }, + ], + contract: { + provides: [ + { task: "t1", kind: "file", name: "manifest.json" }, + { task: "organize", kind: "other", name: "done" }, + ], + requires: [{ task: "t1", name: "organized_files" }], + }, + }); + if (!parsed.ok) throw new Error(parsed.error); + const brief = renderWorkerBrief(parsed.tasks[0]!, { + workingDir: "/repo", + contract: parsed.contract, + }); + const provideNote = + 'contract: provides "done" (task organize) cannot be checked: no `in`, no owned path, no declared files'; + const requireNote = + 'contract: requires "organized_files" (task t1) has no provider — nothing produces it'; + expect(brief).toContain(provideNote); + expect(brief).toContain(requireNote); + expect(brief).toContain("- [organize] other done"); + expect(brief).toContain("You may rely on: nothing from the other parts"); + // Inside the CONTRACT block, ahead of this task's own three lines. + expect(brief.indexOf(provideNote)).toBeGreaterThan( + brief.indexOf("CONTRACT — the interface between the parts"), + ); + expect(brief.indexOf(requireNote)).toBeLessThan(brief.indexOf("For TASK t1:")); + }); + it("adds the PROVIDED rule only when there is a contract", () => { const withContract = renderWorkerBrief(TASK, { workingDir: "/repo", contract: CONTRACT }); expect(withContract).toMatch(/- End the reply with a `PROVIDED:` list/); From 719df062f4c6d643144bde5b075114397ef3ca7a Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Tue, 15 Sep 2026 12:03:30 +0300 Subject: [PATCH 4/6] fix(fusion): F46 the step-half hand-back requires a stalled worker, not a busy one --- src/tools/fusion/worker-runner.test.ts | 140 ++++++++++++++++++++++++- src/tools/fusion/worker-runner.ts | 116 ++++++++++++++++++-- 2 files changed, 240 insertions(+), 16 deletions(-) diff --git a/src/tools/fusion/worker-runner.test.ts b/src/tools/fusion/worker-runner.test.ts index a0cfb471..b69b77f3 100644 --- a/src/tools/fusion/worker-runner.test.ts +++ b/src/tools/fusion/worker-runner.test.ts @@ -16,6 +16,7 @@ import { } from "../../session/fusion-worker-session.js"; import type { DelegateTask } from "./delegate-args.js"; import { + detectStall, estimateWorkerTimeoutMs, runWorkerTasks, WORKER_TIMEOUT_FLOOR_MS, @@ -1011,9 +1012,10 @@ describe("early hand-back when nothing is written (D4 / F19, F42)", () => { expect(result!.stepCount).toBe(8); }); - it("hands back after two completed no-write steps, even when half the budget is one (F42)", async () => { + it("hands back once three same-result steps completed, even when half the budget is one (F42, F46)", async () => { // Half of 3 is 1, but one completed step is a worker that read the - // spec: the floor holds the check until a second step has finished. + // spec, and two are not yet a stall: the step half waits for the + // same result three times (F46), and names it on the note. const { deps } = harness(stepping(null)); const [result] = await runWorkerTasks(deps, { ...BASE, @@ -1023,11 +1025,139 @@ describe("early hand-back when nothing is written (D4 / F19, F42)", () => { signal: new AbortController().signal, }); expect(result!.status).toBe("needs_orchestrator"); - expect(result!.reply).toMatch(/^handed back early: no file written by half the budget \(2 of 3 steps/); - expect(result!.stepCount).toBe(2); + expect(result!.reply).toMatch(/^handed back early: no file written by half the budget \(3 of 3 steps/); + expect(result!.stepCount).toBe(3); expect(result!.notes).toContainEqual( - expect.stringContaining("(2 steps completed, none a successful write)"), + expect.stringContaining( + "(3 steps completed, none a successful write) (stalled: same result 3×)", + ), + ); + }); + + const shell = (summary: string): AgentLoopEvent => ({ + type: "llm_event", + event: { + type: "tool_call_executed", + result: { tool: "os.shell.run", status: "ok", summary, details: {}, truncated: false }, + batchIndex: 0, + batchSize: 1, + }, + }); + const failedShell = (summary: string): AgentLoopEvent => ({ + type: "llm_event", + event: { + type: "tool_call_executed", + result: { tool: "os.shell.run", status: "error", summary, details: {}, truncated: false }, + batchIndex: 0, + batchSize: 1, + }, + }); + const readOf = (file: string): AgentLoopEvent => ({ + type: "llm_event", + event: { + type: "tool_call_executed", + result: { tool: "os.fs.read", status: "ok", summary: `read ${file}: 12 lines`, details: {}, truncated: false }, + batchIndex: 0, + batchSize: 1, + }, + }); + + /** A worker whose step `i` (1-based) emits `resultAt(i)`; replies after `total` steps. */ + function steppingWith(resultAt: (step: number) => AgentLoopEvent, total = 8) { + return async ({ options }: { options: TurnOptions }) => { + for (let step = 1; step <= total; step += 1) { + if (options.signal?.aborted) { + return turnResult({ reason: "cancelled", stepCount: step - 1 }); + } + options.eventHook?.(stepStarted(step - 1)); + options.eventHook?.(resultAt(step)); + options.eventHook?.(stepFinished(step - 1)); + } + options.eventHook?.(replied()); + return turnResult({ stepCount: total }); + }; + } + + it("runs a busy worker to its budget: distinct successful shell calls are not a stall (F46)", async () => { + // Live: a worker hashing one file per shell call was handed back at + // half its 30 steps and never reached its write. Each step is a + // different, successful result — that is work, not a loop. + const { deps } = harness(steppingWith((step) => shell(`sha256sum file${step}.txt: exit 0`))); + const [result] = await runWorkerTasks(deps, { + ...BASE, + workerMaxSteps: 8, + tasks: [{ ...tasks(1)[0]!, files: ["js/**/*.js"] }], + maxWorkers: 1, + signal: new AbortController().signal, + }); + expect(result!.status).not.toBe("needs_orchestrator"); + expect(result!.stepCount).toBe(8); + expect(result!.reply).toBe("done"); + expect(result!.notes ?? []).not.toContainEqual(expect.stringContaining("handed back")); + }); + + it("hands back when the LAST three steps returned the same result, after busy ones (F46)", async () => { + // Two distinct hashes, then the same missing-file error three times + // — a worker waiting for a sibling's output that does not exist yet. + const { deps } = harness( + steppingWith((step) => + step <= 2 + ? shell(`sha256sum file${step}.txt: exit 0`) + : failedShell("cat manifest.json: No such file or directory (exit 1)"), + ), + ); + const [result] = await runWorkerTasks(deps, { + ...BASE, + workerMaxSteps: 8, + tasks: [{ ...tasks(1)[0]!, files: ["a.js"] }], + maxWorkers: 1, + signal: new AbortController().signal, + }); + expect(result!.status).toBe("needs_orchestrator"); + // Half of 8 is 4, but step 4 is only the second repeat; the fifth completes the three. + expect(result!.reply).toMatch(/^handed back early: no file written by half the budget \(5 of 8 steps/); + expect(result!.stepCount).toBe(5); + expect(result!.notes).toContainEqual( + expect.stringContaining("(5 steps completed, none a successful write) (stalled: same result 3×)"), + ); + }); + + it("hands back a worker that only read for six steps, however different the reads (F46)", async () => { + const { deps } = harness(steppingWith((step) => readOf(`src/file${step}.js`))); + const [result] = await runWorkerTasks(deps, { + ...BASE, + workerMaxSteps: 8, + tasks: [{ ...tasks(1)[0]!, files: ["a.js"] }], + maxWorkers: 1, + signal: new AbortController().signal, + }); + expect(result!.status).toBe("needs_orchestrator"); + // Past half the steps at 4, but distinct reads are not the same + // result; the read-only rule needs six of them. + expect(result!.reply).toMatch(/^handed back early: no file written by half the budget \(6 of 8 steps/); + expect(result!.stepCount).toBe(6); + expect(result!.notes).toContainEqual( + expect.stringContaining("(6 steps completed, none a successful write) (stalled: read-only for 6 steps)"), + ); + }); + + it("detectStall names the rule that fired, both when both do (F46)", () => { + const same = { fingerprint: "os.shell.run|error|cat x: no such file", busy: false }; + const busy = (n: number) => ({ fingerprint: `os.shell.run|ok|sha ${n}`, busy: true }); + const read = (n: number) => ({ fingerprint: `os.fs.read|ok|read ${n}`, busy: false }); + expect(detectStall([])).toBeUndefined(); + expect(detectStall([same, same])).toBeUndefined(); + expect(detectStall([busy(1), same, same, same])).toBe("same result 3×"); + expect(detectStall([busy(1), busy(2), busy(3), busy(4), busy(5), busy(6), busy(7)])).toBeUndefined(); + expect(detectStall([busy(1), read(1), read(2), read(3), read(4), read(5), read(6)])).toBe( + "read-only for 6 steps", + ); + // A failed shell call is not a success from a non-read tool either. + expect(detectStall([read(1), read(2), read(3), same, same, same])).toBe( + "same result 3× / read-only for 6 steps", ); + // Five read-only steps after a busy one: neither rule. + expect(detectStall([busy(1), read(1), read(2), read(3), read(4), read(5)])).toBeUndefined(); }); it("never hands back on one completed step: a worker that reads once and then writes runs on (F42)", async () => { diff --git a/src/tools/fusion/worker-runner.ts b/src/tools/fusion/worker-runner.ts index 9ec66056..4338ff03 100644 --- a/src/tools/fusion/worker-runner.ts +++ b/src/tools/fusion/worker-runner.ts @@ -22,6 +22,7 @@ import { isWorkerVisibleTool, } from "./worker-tool-policy.js"; import type { ToolRole } from "../tool-roles.js"; +import { fingerprintToolOutcome } from "../../agent/loop-detector.js"; /** * How many `phase: "tool"` lines one worker may put in the parent's @@ -96,6 +97,74 @@ const WRITE_TOOLS: ReadonlySet = new Set([ */ export const HAND_BACK_MIN_COMPLETED_STEPS = 2; +/** + * Tools whose success is not progress on a task that declared files + * (F46): a read, a listing, a glob, a grep, a watch, a process list. A + * step whose only successful results are these looked at the tree; a + * step that ran a shell command, wrote or edited did something to it. + */ +export const READ_ONLY_TOOLS: ReadonlySet = new Set([ + "os.fs.read", + "os.fs.read_document", + "os.fs.list", + "os.fs.glob", + "os.fs.grep", + "os.fs.watch", + "os.fs.locate_project", + "os.fs.archive.list", + "os.fs.archive.read_entry", + "os.proc.list", + "os.window.list", + "tool.view", +]); + +/** + * The step half of the hand-back fires only on a STALLED worker (F46): + * either its last `HAND_BACK_SAME_RESULT_STEPS` completed steps came + * back with the same outcome fingerprint (F25's `fingerprintToolOutcome` + * — a worker re-checking for a file that does not exist yet, whatever + * the arguments), or its last `HAND_BACK_READ_ONLY_STEPS` steps had no + * successful non-read result at all. A worker making distinct, + * successful shell calls each step — hashing one file per call — is + * busy, and runs to its budget: three of those were handed back at + * half their steps in one live fan-out, and the one that was working + * never reached its write. The time half is unchanged. + */ +export const HAND_BACK_SAME_RESULT_STEPS = 3; +export const HAND_BACK_READ_ONLY_STEPS = 6; + +/** What one completed worker step produced, for the stall check. */ +export interface WorkerStepOutcome { + /** The step's tool results' fingerprints, in order; empty for a step with none. */ + fingerprint: string; + /** True when some result was a success from a tool outside `READ_ONLY_TOOLS`. */ + busy: boolean; +} + +/** + * Why the worker's recent steps look stalled, or `undefined` while it is + * still doing something: `same result 3×`, `read-only for 6 steps`, or + * both joined with ` / `. + */ +export function detectStall( + steps: readonly WorkerStepOutcome[], +): string | undefined { + const reasons: string[] = []; + if (steps.length >= HAND_BACK_SAME_RESULT_STEPS) { + const tail = steps.slice(-HAND_BACK_SAME_RESULT_STEPS); + if (tail.every((s) => s.fingerprint === tail[0]!.fingerprint)) { + reasons.push(`same result ${HAND_BACK_SAME_RESULT_STEPS}×`); + } + } + if (steps.length >= HAND_BACK_READ_ONLY_STEPS) { + const tail = steps.slice(-HAND_BACK_READ_ONLY_STEPS); + if (tail.every((s) => !s.busy)) { + reasons.push(`read-only for ${HAND_BACK_READ_ONLY_STEPS} steps`); + } + } + return reasons.length === 0 ? undefined : reasons.join(" / "); +} + /** The forced summary a handed-back task replies with. */ export function formatEarlyHandBack(input: { stepsTaken: number; @@ -359,11 +428,12 @@ async function runOneTask( const hitTimeLimit = (): boolean => timeLimit.aborted && !options.signal.aborted; - // D4 / F42: a task that declared output files and has written none by - // half its step budget or half its time is handed back with what it - // found, instead of spending the other half the same way — but only - // at a step boundary, and only once `HAND_BACK_MIN_COMPLETED_STEPS` - // steps have completed. The check runs when a step finishes, never on + // D4 / F42 / F46: a task that declared output files and has written + // none by half its step budget (and is stalled — `detectStall`) or by + // half its time is handed back with what it found, instead of + // spending the other half the same way — but only at a step boundary, + // and only once `HAND_BACK_MIN_COMPLETED_STEPS` steps have completed. + // The check runs when a step finishes, never on // a timer: at that moment the step's completion and its tool calls // are done and the next completion has not been requested, so the // abort costs nothing that was generated. Only for tasks with declared @@ -377,11 +447,24 @@ async function runOneTask( const halfTimeMs = Math.floor(timeoutMs / 2); let stepsFinished = 0; let wroteSomething = false; + // What each completed step produced (F46): the current step's results + // accumulate here and are folded into `steps` when it finishes. + const steps: WorkerStepOutcome[] = []; + let currentFingerprints: string[] = []; + let currentBusy = false; + // Why the step half fired, when it did; carried onto the note so the + // orchestrator re-briefs against the cause, not just the count. + let stall: string | undefined; const maybeHandBack = (): void => { if (declaredFiles === 0 || wroteSomething || handBack.signal.aborted) return; if (stepsFinished < HAND_BACK_MIN_COMPLETED_STEPS) return; const pastHalfTime = Date.now() - startedAt >= halfTimeMs; - if (stepsFinished < stepThreshold && !pastHalfTime) return; + // The step half needs a stalled worker, not merely a busy one at + // half its budget; the time half fires as before. + const stalled = detectStall(steps); + const pastHalfSteps = stepsFinished >= stepThreshold && stalled !== undefined; + if (!pastHalfSteps && !pastHalfTime) return; + stall = stalled; handBack.abort(new Error("handed back early: no file written by half the budget")); }; const handedBack = (): boolean => @@ -414,6 +497,12 @@ async function runOneTask( // `step_finished`, not `step_started`: a started step is a // request in flight, and F19's count of those is how a // worker was stopped mid-file. + steps.push({ + fingerprint: currentFingerprints.join("\n"), + busy: currentBusy, + }); + currentFingerprints = []; + currentBusy = false; stepsFinished += 1; maybeHandBack(); } @@ -429,11 +518,16 @@ async function runOneTask( } if ( event.type === "llm_event" && - event.event.type === "tool_call_executed" && - event.event.result.status === "ok" && - WRITE_TOOLS.has(event.event.result.tool) + event.event.type === "tool_call_executed" ) { - wroteSomething = true; + const result = event.event.result; + if (result.status === "ok" && WRITE_TOOLS.has(result.tool)) { + wroteSomething = true; + } + currentFingerprints.push(fingerprintToolOutcome(result.tool, result)); + if (result.status === "ok" && !READ_ONLY_TOOLS.has(result.tool)) { + currentBusy = true; + } } collector.observe(event); }, @@ -507,7 +601,7 @@ async function runOneTask( stepCount: completed, notes: [ ...(result.notes ?? []), - `handed back early: declared files but wrote none by half the budget (${completed} steps completed, none a successful write) — re-brief with a narrower task or the exact content to write`, + `handed back early: declared files but wrote none by half the budget (${completed} steps completed, none a successful write)${stall === undefined ? "" : ` (stalled: ${stall})`} — re-brief with a narrower task or the exact content to write`, ], }; } From eea66bafdfb055c96fc5c44edc86eaeee66907ef Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Tue, 15 Sep 2026 12:07:11 +0300 Subject: [PATCH 5/6] =?UTF-8?q?feat(fusion):=20F45=20a=20fan-out=20runs=20?= =?UTF-8?q?in=20waves=20ordered=20by=20the=20contract's=20requires=20?= =?UTF-8?q?=E2=86=92=20provides?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/tools/fusion/contract-waves.ts | 156 +++++++++++++++ src/tools/fusion/fusion-delegate.test.ts | 233 ++++++++++++++++++++++- src/tools/fusion/fusion-delegate.ts | 85 ++++++--- src/tools/fusion/index.ts | 10 + src/tools/fusion/worker-result.ts | 22 ++- 5 files changed, 481 insertions(+), 25 deletions(-) create mode 100644 src/tools/fusion/contract-waves.ts diff --git a/src/tools/fusion/contract-waves.ts b/src/tools/fusion/contract-waves.ts new file mode 100644 index 00000000..e6b1e9fe --- /dev/null +++ b/src/tools/fusion/contract-waves.ts @@ -0,0 +1,156 @@ +import type { DelegateContract } from "./contract.js"; +import type { WorkerTaskResult, WorkerTaskStatus } from "./worker-result.js"; + +/** + * The order a contract puts on a fan-out (F45). + * + * A `requires` names something a sibling `provides`; a worker sent at + * the same time as its provider waits for a file that does not exist + * yet. Live, an orchestrator declared a pipeline — `analyze` provides a + * manifest, `organize` requires it, `index` requires what `organize` + * produces — and sent all three at once: two workers spent their whole + * step budget re-checking for the missing input and nothing was done. + * + * So the fan-out runs in waves: a task that requires X depends on every + * task that provides X; wave 1 is every task with no unmet dependency, + * and each later wave is the tasks whose dependencies have all finished + * — with any status. A provider that ended `failed`, `cancelled`, + * `no_changes` or `needs_orchestrator` did not deliver, but its + * dependent still runs: the orchestrator gets every report either way, + * and the dependent's CONTRACT block carries a warning naming what did + * not arrive (`dependencyWarnings`, through the F44 warning slot). + * + * A cycle in the requires is one wave, in the order given, with a + * warning. A contract without `requires`, or whose requires name + * nothing any task provides, orders nothing: one wave, exactly the + * fan-out that ran before waves existed. + */ +export interface WavePlan { + /** Task ids, wave by wave; each wave in the caller's task order. */ + waves: string[][]; + /** + * Task id → the provider tasks it waits for, in the contract's order + * with duplicates dropped. Empty when the contract orders nothing. + */ + dependencies: ReadonlyMap; + /** The warning when the requires form a cycle; the plan is then one wave. */ + cycle?: string; +} + +/** A task that requires X depends on every OTHER task that provides X. */ +export function dependenciesOf( + taskIds: readonly string[], + contract: DelegateContract | undefined, +): Map { + const known = new Set(taskIds); + const dependencies = new Map(); + for (const require of contract?.requires ?? []) { + if (!known.has(require.task)) continue; + for (const provide of contract?.provides ?? []) { + if (provide.name !== require.name) continue; + if (provide.task === require.task || !known.has(provide.task)) continue; + const own = dependencies.get(require.task) ?? []; + if (!own.includes(provide.task)) own.push(provide.task); + dependencies.set(require.task, own); + } + } + return dependencies; +} + +/** + * Follow unmet dependencies from the first stuck task until one + * repeats: `a → b → a` reads "a requires b requires a". Every stuck + * task has an unmet dependency among the stuck ones (that is what + * stuck means), so the walk closes within their number. + */ +function describeCycle( + stuck: readonly string[], + dependencies: ReadonlyMap, +): string { + const among = new Set(stuck); + const path: string[] = []; + let node = stuck[0]!; + while (!path.includes(node)) { + path.push(node); + node = (dependencies.get(node) ?? []).find((d) => among.has(d)) ?? node; + } + return [...path.slice(path.indexOf(node)), node].join(" → "); +} + +/** `requires form a cycle (a → b → a), so the tasks run in one wave in the order given` */ +export function describeCycleWarning(cycle: string): string { + return `requires form a cycle (${cycle}), so the tasks run in one wave in the order given`; +} + +export function planWaves( + taskIds: readonly string[], + contract: DelegateContract | undefined, +): WavePlan { + const dependencies = dependenciesOf(taskIds, contract); + const waves: string[][] = []; + const done = new Set(); + let remaining = [...taskIds]; + while (remaining.length > 0) { + const wave = remaining.filter((id) => + (dependencies.get(id) ?? []).every((d) => done.has(d)), + ); + if (wave.length === 0) { + return { + waves: [[...taskIds]], + dependencies, + cycle: describeCycleWarning(describeCycle(remaining, dependencies)), + }; + } + waves.push(wave); + for (const id of wave) done.add(id); + remaining = remaining.filter((id) => !done.has(id)); + } + return { waves, dependencies }; +} + +/** A provider that ended one of these did not deliver what its dependents rely on. */ +export const UNDELIVERED_STATUSES: ReadonlySet = new Set([ + "failed", + "cancelled", + "no_changes", + "needs_orchestrator", +]); + +/** + * One warning per (dependent, undelivered provider) pair in `wave`, + * from the results so far: `task organize depends on analyze, which + * ended no_changes`. Goes into the wave's CONTRACT block (the worker + * learns not to wait for it) and onto the result's `contract:` line. + */ +export function dependencyWarnings( + wave: readonly string[], + dependencies: ReadonlyMap, + finished: ReadonlyMap, +): string[] { + const warnings: string[] = []; + for (const id of wave) { + for (const provider of dependencies.get(id) ?? []) { + const result = finished.get(provider); + if (result === undefined || !UNDELIVERED_STATUSES.has(result.status)) { + continue; + } + warnings.push( + `task ${id} depends on ${provider}, which ended ${result.status}`, + ); + } + } + return warnings; +} + +/** + * The contract a wave's workers read: the parsed one, plus the warnings + * the earlier waves produced. The very object when there is nothing to + * add, so a fan-out the contract does not order briefs byte-identically. + */ +export function contractForWave( + contract: DelegateContract | undefined, + extra: readonly string[], +): DelegateContract | undefined { + if (contract === undefined || extra.length === 0) return contract; + return { ...contract, warnings: [...(contract.warnings ?? []), ...extra] }; +} diff --git a/src/tools/fusion/fusion-delegate.test.ts b/src/tools/fusion/fusion-delegate.test.ts index 42714de7..99f9f238 100644 --- a/src/tools/fusion/fusion-delegate.test.ts +++ b/src/tools/fusion/fusion-delegate.test.ts @@ -832,7 +832,8 @@ describe("fusion.delegate", () => { // Presence: `HD.Ship.reset` was never written; the id is spelled the other way. const lines = result.summary.split("\n"); - expect(lines[0]).toBe("2 tasks: 1 ok, 1 failed"); + // t2 requires what t1 provides, so t2 ran in a second wave (F45). + expect(lines[0]).toBe("2 tasks in 2 waves (t1 → t2): 1 ok, 1 failed"); expect(lines[1]).toBe( "contract: 2 missing — [t1] symbol HD.Ship.reset not in js/ship.js; [t2] id btn-launch not in index.html; call-level checks: 1 of 1 passed", ); @@ -974,6 +975,236 @@ describe("fusion.delegate", () => { }); }); + describe("waves ordered by the contract (F45)", () => { + it("is byte-identical without a contract: one wave, no wave plan on the head line or in the details", async () => { + const tool = buildFusionDelegateTool(deps()); + const result = await tool.run({ tasks: TASKS, maxWorkers: 2 }, ctx()); + const rows = (result.details.tasks as WorkerTaskResult[]).map( + ({ durationMs: _ms, ...row }) => row, + ); + expect(result.summary).toMatchInlineSnapshot(` + "2 tasks: 2 ok + - [t1] ok — One + - [t2] ok — Two + [t1] ok — One (1 steps, 0s, 0 tool calls, 0 errors) + (the worker produced no reply) + [t2] ok — Two (1 steps, 0s, 0 tool calls, 0 errors) + (the worker produced no reply)" + `); + expect(result.details).not.toHaveProperty("waves"); + expect({ ...result.details, tasks: rows }).toMatchInlineSnapshot(` + { + "maxWorkers": 2, + "outcome": "all_ok", + "requestedWorkers": 2, + "slotPoolSize": 4, + "tasks": [ + { + "id": "t1", + "reply": "", + "status": "ok", + "stepCount": 1, + "title": "One", + "tools": { + "byTool": {}, + "calls": 0, + "errors": 0, + "writes": 0, + }, + }, + { + "id": "t2", + "reply": "", + "status": "ok", + "stepCount": 1, + "title": "Two", + "tools": { + "byTool": {}, + "calls": 0, + "errors": 0, + "writes": 0, + }, + }, + ], + } + `); + }); + + /** The live pipeline: `analyze` provides the manifest both others require. */ + const PIPELINE = [ + { id: "analyze", title: "Analyze", instructions: "a", files: ["manifest.json"] }, + { id: "organize", title: "Organize", instructions: "o" }, + { id: "index", title: "Index", instructions: "i" }, + ]; + const PIPELINE_CONTRACT = { + provides: [{ task: "analyze", kind: "file", name: "manifest.json" }], + requires: [ + { task: "organize", name: "manifest.json" }, + { task: "index", name: "manifest.json" }, + ], + }; + + /** + * A fake `runTurn` that records, for each worker, which siblings had + * already RESOLVED when it started, and how many were in flight. + */ + function ordering(over: { + reasonFor?: (taskId: string) => RunTurnResult["reason"]; + briefs?: string[]; + } = {}) { + const started: string[] = []; + const resolvedBefore: Record = {}; + const resolved: string[] = []; + let inFlight = 0; + let peakInFlight = 0; + const runTurn: FusionDelegateDeps["runTurn"] = async (session, brief) => { + const taskId = (session.metadata as { fusionWorker: { taskId: string } }) + .fusionWorker.taskId; + started.push(taskId); + over.briefs?.push(brief); + resolvedBefore[taskId] = [...resolved]; + inFlight += 1; + peakInFlight = Math.max(peakInFlight, inFlight); + await new Promise((r) => setTimeout(r, 5)); + inFlight -= 1; + resolved.push(taskId); + return turnResult({ reason: over.reasonFor?.(taskId) ?? "reply" }); + }; + return { runTurn, started, resolvedBefore, peak: () => peakInFlight }; + } + + it("runs the dependents in a second wave, after the provider's turn resolved", async () => { + const fake = ordering(); + const tool = buildFusionDelegateTool(deps({ runTurn: fake.runTurn })); + const result = await tool.run( + { tasks: PIPELINE, contract: PIPELINE_CONTRACT, maxWorkers: 3 }, + ctx(), + ); + expect(fake.started).toEqual(["analyze", "organize", "index"]); + expect(fake.resolvedBefore.analyze).toEqual([]); + expect(fake.resolvedBefore.organize).toEqual(["analyze"]); + expect(fake.resolvedBefore.index).toEqual(["analyze"]); + // The second wave still ran two wide. + expect(fake.peak()).toBe(2); + expect(result.status).toBe("ok"); + const lines = result.summary.split("\n"); + expect(lines[0]).toBe("3 tasks in 2 waves (analyze → organize, index): 2 ok, 1 failed"); + expect(result.details.waves).toEqual([["analyze"], ["organize", "index"]]); + // Rows keep the caller's order, whatever wave each ran in. + expect((result.details.tasks as WorkerTaskResult[]).map((r) => r.id)).toEqual([ + "analyze", + "organize", + "index", + ]); + }); + + it("bounds each wave by maxWorkers", async () => { + const fake = ordering(); + const tool = buildFusionDelegateTool(deps({ runTurn: fake.runTurn })); + const tasks = [ + ...PIPELINE, + { id: "report", title: "Report", instructions: "r" }, + ]; + const contract = { + ...PIPELINE_CONTRACT, + requires: [...PIPELINE_CONTRACT.requires, { task: "report", name: "manifest.json" }], + }; + const result = await tool.run({ tasks, contract, maxWorkers: 2 }, ctx()); + expect(fake.started).toEqual(["analyze", "organize", "index", "report"]); + expect(fake.resolvedBefore.report).toContain("analyze"); + expect(fake.peak()).toBe(2); + expect(result.details.waves).toEqual([["analyze"], ["organize", "index", "report"]]); + expect(result.summary.split("\n")[0]).toBe( + "4 tasks in 2 waves (analyze → organize, index, report): 3 ok, 1 failed", + ); + }); + + it("still runs a dependent whose provider did not deliver, warning it and the orchestrator", async () => { + const briefs: string[] = []; + const fake = ordering({ + briefs, + reasonFor: (taskId) => (taskId === "analyze" ? "failed" : "reply"), + }); + const tool = buildFusionDelegateTool(deps({ runTurn: fake.runTurn })); + const result = await tool.run( + { tasks: PIPELINE, contract: PIPELINE_CONTRACT }, + ctx(), + ); + const organizeNote = "task organize depends on analyze, which ended failed"; + const indexNote = "task index depends on analyze, which ended failed"; + expect(fake.started).toEqual(["analyze", "organize", "index"]); + // The provider's own brief carried no such note; the dependents' do. + expect(briefs[0]).not.toContain("depends on"); + expect(briefs[1]).toContain(`contract: ${organizeNote}`); + expect(briefs[1]).toContain(`contract: ${indexNote}`); + expect(briefs[2]).toContain(`contract: ${indexNote}`); + const lines = result.summary.split("\n"); + expect(lines[0]).toBe("3 tasks in 2 waves (analyze → organize, index): 2 ok, 1 failed"); + expect(lines[1]).toBe( + `contract: 1 missing — [analyze] file manifest.json does not exist; ${organizeNote}; ${indexNote}`, + ); + expect((result.details.tasks as WorkerTaskResult[]).map((r) => r.status)).toEqual([ + "failed", + "ok", + "ok", + ]); + const report = result.details.contract as { warnings: string[] }; + expect(report.warnings).toEqual([organizeNote, indexNote]); + }); + + it("runs a cyclic contract as one wave, in the order given, with a warning", async () => { + const briefs: string[] = []; + const fake = ordering({ briefs }); + const tool = buildFusionDelegateTool(deps({ runTurn: fake.runTurn })); + const result = await tool.run( + { + tasks: [ + { id: "a", title: "A", instructions: "a" }, + { id: "b", title: "B", instructions: "b" }, + ], + contract: { + provides: [ + { task: "a", kind: "file", name: "a.txt" }, + { task: "b", kind: "file", name: "b.txt" }, + ], + requires: [ + { task: "a", name: "b.txt" }, + { task: "b", name: "a.txt" }, + ], + }, + maxWorkers: 2, + }, + ctx(), + ); + const note = + "requires form a cycle (a → b → a), so the tasks run in one wave in the order given"; + expect(fake.started).toEqual(["a", "b"]); + expect(fake.peak()).toBe(2); + for (const brief of briefs) expect(brief).toContain(`contract: ${note}`); + expect(result.details.waves).toEqual([["a", "b"]]); + const lines = result.summary.split("\n"); + expect(lines[0]).toBe("2 tasks in 1 wave (a, b): 2 ok"); + expect(lines[1]).toContain(note); + expect((result.details.contract as { warnings: string[] }).warnings).toEqual([note]); + }); + + it("orders nothing when the requires name nothing any task provides", async () => { + const fake = ordering(); + const tool = buildFusionDelegateTool(deps({ runTurn: fake.runTurn })); + const result = await tool.run( + { + tasks: PIPELINE, + contract: { requires: [{ task: "organize", name: "ghost" }] }, + maxWorkers: 3, + }, + ctx(), + ); + expect(fake.peak()).toBe(3); + expect(result.details).not.toHaveProperty("waves"); + expect(result.summary.split("\n")[0]).toBe("3 tasks: 2 ok, 1 failed"); + }); + }); + it("asks again when a later fan-out reaches outside what was approved", async () => { const asked: string[] = []; const scopes = new FanoutScopeRegistry(); diff --git a/src/tools/fusion/fusion-delegate.ts b/src/tools/fusion/fusion-delegate.ts index 74e05510..d170f562 100644 --- a/src/tools/fusion/fusion-delegate.ts +++ b/src/tools/fusion/fusion-delegate.ts @@ -19,6 +19,11 @@ import { type ContractCheckRunner, type ContractReport, } from "./contract-checks.js"; +import { + contractForWave, + dependencyWarnings, + planWaves, +} from "./contract-waves.js"; import { runWorkerTasks, type WorkerRunnerDeps } from "./worker-runner.js"; import { delegateOutcome, @@ -320,27 +325,56 @@ export function buildFusionDelegateTool( ? (deps.localTokensPerSecond?.() ?? null) : null; + // The order the contract imposes (F45): a task that requires what + // a sibling provides runs in a later wave than that sibling, so it + // is not sent to wait for a file that does not exist yet. Without + // a contract, or without a satisfiable `requires`, the plan is one + // wave holding every task — the fan-out as it always ran. + const plan = planWaves( + parsed.tasks.map((t) => t.id), + parsed.contract, + ); + const ordered = plan.dependencies.size > 0; + // What the waves add to the contract's own warnings: a cycle, and + // every provider that had not delivered when its dependent ran. + // Each wave's block carries everything known so far; the result's + // `contract:` line carries all of it. + const waveWarnings: string[] = plan.cycle === undefined ? [] : [plan.cycle]; + let results: WorkerTaskResult[]; try { - results = await runWorkerTasks(deps, { - ...(originalRequest === undefined ? {} : { originalRequest }), - ...(parsed.contract === undefined ? {} : { contract: parsed.contract }), - parentSessionId: ctx.sessionId, - tasks: parsed.tasks, - maxWorkers, - providerId: workerProviderId, - workerModel, - workerMaxSteps: mode.workerMaxSteps, - workerTimeoutMs: mode.workerTimeoutMs, - localTokensPerSecond, - ...(mode.workerReasoning === undefined - ? {} - : { workerReasoning: mode.workerReasoning }), - ...(mode.workerMaxOutputTokens === undefined - ? {} - : { workerMaxOutputTokens: mode.workerMaxOutputTokens }), - writeScope, - signal: ctx.signal, + const finished = new Map(); + for (const wave of plan.waves) { + waveWarnings.push( + ...dependencyWarnings(wave, plan.dependencies, finished), + ); + const contract = contractForWave(parsed.contract, waveWarnings); + const waveResults = await runWorkerTasks(deps, { + ...(originalRequest === undefined ? {} : { originalRequest }), + ...(contract === undefined ? {} : { contract }), + parentSessionId: ctx.sessionId, + tasks: parsed.tasks.filter((t) => wave.includes(t.id)), + maxWorkers, + providerId: workerProviderId, + workerModel, + workerMaxSteps: mode.workerMaxSteps, + workerTimeoutMs: mode.workerTimeoutMs, + localTokensPerSecond, + ...(mode.workerReasoning === undefined + ? {} + : { workerReasoning: mode.workerReasoning }), + ...(mode.workerMaxOutputTokens === undefined + ? {} + : { workerMaxOutputTokens: mode.workerMaxOutputTokens }), + writeScope, + signal: ctx.signal, + }); + for (const result of waveResults) finished.set(result.id, result); + } + // In the caller's task order, whatever wave each ran in. + results = parsed.tasks.flatMap((t) => { + const result = finished.get(t.id); + return result === undefined ? [] : [result]; }); } catch (err) { // `runWorkerTasks` is written not to throw; if it ever does, the @@ -370,10 +404,11 @@ export function buildFusionDelegateTool( ); results = applyCheckOutcomes(results, checks.outcomes); // What the call was run with despite the contract — a require - // nobody provides, a provide nothing can check. The workers read - // it in their block; the orchestrator reads it here, on the line - // and in the details. - const warnings = parsed.contract.warnings ?? []; + // nobody provides, a provide nothing can check, a cycle in the + // requires, a provider that had not delivered when its dependent + // ran. The workers read it in their block; the orchestrator + // reads it here, on the line and in the details. + const warnings = [...(parsed.contract.warnings ?? []), ...waveWarnings]; contract = { findings, checks: checks.outcomes, @@ -427,6 +462,7 @@ export function buildFusionDelegateTool( output: `${formatDelegateOutput(results, deps.outputCharCap, { ...(contractLine === undefined ? {} : { contractLine }), spend, + ...(ordered ? { waves: plan.waves } : {}), })}${hint}`, details: { tasks: results, @@ -434,6 +470,9 @@ export function buildFusionDelegateTool( maxWorkers, requestedWorkers: requested, ...(Number.isFinite(poolSize) ? { slotPoolSize: poolSize } : {}), + // The wave plan, for the orchestrator and the trace's tool + // row alike — only when the contract ordered anything. + ...(ordered ? { waves: plan.waves } : {}), ...(contract === undefined ? {} : { contract }), ...(spend === null ? {} : { workerSpendUsd: spend.usd }), }, diff --git a/src/tools/fusion/index.ts b/src/tools/fusion/index.ts index e998af7f..cd341854 100644 --- a/src/tools/fusion/index.ts +++ b/src/tools/fusion/index.ts @@ -53,6 +53,15 @@ export type { ContractFinding, ContractReport, } from "./contract-checks.js"; +export { + contractForWave, + dependenciesOf, + dependencyWarnings, + describeCycleWarning, + planWaves, + UNDELIVERED_STATUSES, +} from "./contract-waves.js"; +export type { WavePlan } from "./contract-waves.js"; export { renderWorkerBrief, pickOriginalRequest, @@ -64,6 +73,7 @@ export { WorkerRunCollector, classifyWorkerStatus, delegateOutcome, + describeWaves, formatDelegateOutput, resultCarriesApprovalRefusal, workerFailureHint, diff --git a/src/tools/fusion/worker-result.ts b/src/tools/fusion/worker-result.ts index 2f851058..c872e608 100644 --- a/src/tools/fusion/worker-result.ts +++ b/src/tools/fusion/worker-result.ts @@ -497,6 +497,19 @@ export interface DelegateOutputExtras { contractLine?: string; /** The fan-out's priced worker spend, when the worker model is priced. */ spend?: FanoutSpend | null; + /** + * The wave plan the contract's requires imposed (`contract-waves.ts`), + * task ids wave by wave. Absent when the contract ordered nothing — + * the head line then reads exactly as before. + */ + waves?: readonly (readonly string[])[]; +} + +/** `analyze → organize, index` — waves in order, each wave's tasks together. */ +export function describeWaves( + waves: readonly (readonly string[])[], +): string { + return waves.map((wave) => wave.join(", ")).join(" → "); } /** @@ -529,6 +542,13 @@ function renderStatusTable( : ` — cloud spend ${formatUsd(spend.usd)} on ${spend.model} (${spend.promptTokens.toLocaleString("en-US")} in / ${spend.completionTokens.toLocaleString("en-US")} out)`; const replacedCount = countReplacedInputs(results); const replaced = replacedCount === null ? "" : ` — ${replacedCount}`; + // The order the contract imposed, on the head line: which tasks + // waited for which, so a report of "no_changes" on a later wave reads + // against what its provider delivered. + const waves = + extra.waves === undefined + ? "" + : ` in ${extra.waves.length} wave${extra.waves.length === 1 ? "" : "s"} (${describeWaves(extra.waves)})`; const lines = results.map((r) => [ `- [${r.id}] ${r.status} — ${r.title}`, @@ -541,7 +561,7 @@ function renderStatusTable( ].join(" — "), ); return [ - `${results.length} task${results.length === 1 ? "" : "s"}: ${tally}${replaced}${cost}`, + `${results.length} task${results.length === 1 ? "" : "s"}${waves}: ${tally}${replaced}${cost}`, ...(contractLine === undefined ? [] : [contractLine]), ...lines, ].join("\n"); From 309ec92464e6b58b049db67dbbc64a7be23913eb Mon Sep 17 00:00:00 2001 From: Valera Brizhatiuk <19537764+plombeer31@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:16:23 +0300 Subject: [PATCH 6/6] =?UTF-8?q?docs(agents):=20fusion=20loop=20tuning=20?= =?UTF-8?q?=E2=80=94=20waves,=20stalled=20hand-back,=20restore=20per=20wor?= =?UTF-8?q?king=20directory,=20one=20refusal=20per=20call=20(F42=E2=80=93F?= =?UTF-8?q?46)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The orchestrator section now states the rules the live fusion runs settled: a fan-out with a contract runs in waves ordered by requires → provides (F45); a worker with declared files is handed back only at a step boundary after two completed steps without a write, and the step half additionally needs a stall — same result three times or six read-only steps (F42, F46); the restore store is keyed by working directory so any worker or the orchestrator can restore a replaced input, which is rendered first on the task's status row and counted on the head line (F43); fusion.delegate refuses only what cannot run, in one message naming every problem, with title optional and an unmatched requires or an unverifiable provides downgraded to a warning (F44). --- AGENTS.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index dd43272d..c9c26919 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -125,7 +125,7 @@ Two checks run in `executeBatch` on every non-terminal call, in this order, befo **Unknown argument keys.** A worker sent `os.shell.run {"cmd":"python3","-e":"