From ea0085c6ee15e48bfdcffa0bf8a6ac7fcb66d787 Mon Sep 17 00:00:00 2001 From: Antonio Viggiano <3029017+aviggiano@users.noreply.github.com> Date: Fri, 7 Aug 2026 05:32:56 +0000 Subject: [PATCH 1/4] fix(runtime): start dependency replay after the prefix the worktree already holds `prepare:stateful-invariant-implement-properties` replays every dependency's workspace patch. That is correct on a fresh run -- the task worktree starts at the pinned baseline, so each patch's declared `base_tree` is satisfied in turn down the chain. A RESUMED run breaks the precondition: the worktree lives on a durable volume and still holds the previous attempt's state. Two production runs died on it, at the same node, with the same expected tree: R48 expected 2dd4efef..., got bf324c39... R49 expected 2dd4efef..., got b8d46f13... `2dd4efef` is `setup-foundry`'s pristine base. The chain, dumped from the durable volume, is linear: setup-foundry 2dd4efef -> 4fefa8df 1 file base-test-setup 4fefa8df -> 4fefa8df 0 files stateful-invariant-setup 4fefa8df -> fd4e3c2c 7 files handlers fd4e3c2c -> bf324c39 7 files coverage bf324c39 -> bf324c39 0 files The worktree sat at the END of that chain. Every dependency's content was already present, folded in by later links; `applyWorkspacePatch` threw only because it compares the worktree against one patch's own `base_tree` in isolation and cannot see the chain that patch belongs to. Replay now starts after the prefix the worktree already equals. The rule is sound for one reason: a tree id is a content hash of the WHOLE snapshot, so equality with a dependency's declared `result_tree` means the workspace is byte-identical to that dependency's output, and everything at or before it in replay order is already materialized. That is an identity, not an inference about intent -- which is what the two earlier attempts lacked. One relaxed `applyWorkspacePatch` (from inside that function a legitimate fan-in is indistinguishable from real drift); one consulted recorded provenance (a task that throws inside the replay loop never persists a preparation record, so the evidence does not exist when the decision is made). Both were rejected by review. It never suppresses an error. A worktree matching no declared output replays from the start and `applyWorkspacePatch` raises its base-tree mismatch exactly as before, so genuine drift stays loud instead of quietly producing a corrupt workspace. Manifests are now read before any patch is applied, because the decision is about the chain as a whole. Scoped to the replay path only. Post-agent preparation (`replayWorkspacePatches: false`) keeps its own long-standing rule and is not second-guessed; a test discriminates the two, since a worktree at `4fefa8df` is the declared base of two different dependencies there. Nine tests, and each of five mutations fails at least one of them: forcing `replayFrom` to 0, dropping the replay-path guard, scanning the chain forward instead of backward, returning the matched index instead of the one after it, and comparing base trees instead of result trees. The rule alone was not enough -- deleting the wiring left the first five tests green -- so `materializeWorkspacePatchDependencies` is lifted out of the template and run against stubbed collaborators too. Fixes #312. Co-Authored-By: Claude Opus 5 --- .../templates/smithers/workflows/workflow.tsx | 50 +++- .../test/generated-workflow-verifier.test.ts | 10 + .../test/workspace-patch-replay.test.ts | 234 ++++++++++++++++++ 3 files changed, 292 insertions(+), 2 deletions(-) create mode 100644 packages/runtime/test/workspace-patch-replay.test.ts diff --git a/packages/runtime/src/templates/smithers/workflows/workflow.tsx b/packages/runtime/src/templates/smithers/workflows/workflow.tsx index b119039b6..3fe31d8de 100644 --- a/packages/runtime/src/templates/smithers/workflows/workflow.tsx +++ b/packages/runtime/src/templates/smithers/workflows/workflow.tsx @@ -426,6 +426,37 @@ function taskPublishesWorkspacePatch(task: (typeof taskSpecs)[number]): boolean ); } +/** + * Where replay of a dependency chain should START, given the worktree it is replaying into (issue #312). + * + * Replaying every dependency patch unconditionally is correct on a fresh run: the task worktree begins at + * the pinned baseline, so each patch's declared `base_tree` is satisfied in turn down the chain. A RESUMED + * run breaks that precondition, because the worktree lives on a durable volume and still holds the + * previous attempt's state. Two production runs died on it at the same node with the same expected tree — + * R48 (`got bf324c39…`) and R49 (`got b8d46f13…`), both against `setup-foundry`'s pristine `2dd4efef…`. + * In both, the worktree was at the END of the chain: every dependency's content was already present, and + * `applyWorkspacePatch` threw only because it compares the worktree against one patch's own `base_tree` in + * isolation and cannot see the chain that patch belongs to. + * + * The rule is sound for one reason, and it is worth being precise about it: a tree id is a content hash of + * the WHOLE snapshot. If the worktree's tree equals a dependency's declared `result_tree`, the workspace + * is byte-identical to that dependency's output, so that dependency and every one before it in replay + * order are already materialized. That is an identity, not an inference about what someone intended. + * + * It also never suppresses an error. A worktree matching no declared output replays from the start and + * `applyWorkspacePatch` raises its base-tree mismatch exactly as before, which is what keeps genuine drift + * loud instead of quietly producing a corrupt workspace. + */ +function firstDependencyRequiringReplay(currentTree: string, resultTrees: readonly string[]): number { + // Scan from the end: with a no-op dependency in the chain (`base-test-setup` declares base == result) + // two adjacent entries share an output, and resuming after the LAST of them is the honest reading of + // "everything up to here is already present". + for (let index = resultTrees.length - 1; index >= 0; index -= 1) { + if (resultTrees[index] === currentTree) return index + 1; + } + return 0; +} + function materializeWorkspacePatchDependencies( task: (typeof taskSpecs)[number], workspaceRoot: string, @@ -446,7 +477,11 @@ function materializeWorkspacePatchDependencies( const rightIndex = taskSpecs.findIndex((candidate) => candidate.attemptId === path.basename(right)); return leftIndex - rightIndex || left.localeCompare(right); }); - for (const dependency of dependencies) { + // Read every manifest BEFORE applying any of them. The decision below is about the chain as a whole -- + // whether a LATER dependency's output already describes this worktree -- and that cannot be made one + // patch at a time. Reading first also keeps the artifact-contract failures ordered by dependency rather + // than interleaved with partially applied patches. + const captures = dependencies.map((dependency) => { const patchPath = resolveRegularArtifactFile( dependency, path.join(dependency, "workspace.patch"), @@ -465,10 +500,21 @@ function materializeWorkspacePatchDependencies( cause: error }); } - const capture = { + return { patch: readFileSync(patchPath, "utf8"), manifest: manifest as Parameters[1]["manifest"] }; + }); + // On a RESUME the worktree lives on a durable volume and still holds the previous attempt's state, so + // it can already sit at -- or past -- some of these dependencies' outputs. Skip the prefix the worktree + // already equals byte for byte (issue #312); on a fresh run nothing matches and this is a no-op. + const replayFrom = replayWorkspacePatches + ? firstDependencyRequiringReplay( + captureWorkspaceTree(workspaceRoot), + captures.map((entry) => entry.manifest.result_tree) + ) + : 0; + for (const capture of captures.slice(replayFrom)) { if (!replayWorkspacePatches) { // Post-agent preparation may see a dirty worktree. Replay only when the // exact dependency result tree is absent and the clean base tree is diff --git a/packages/runtime/test/generated-workflow-verifier.test.ts b/packages/runtime/test/generated-workflow-verifier.test.ts index 989527f22..bff6e9e53 100644 --- a/packages/runtime/test/generated-workflow-verifier.test.ts +++ b/packages/runtime/test/generated-workflow-verifier.test.ts @@ -1642,6 +1642,16 @@ test("generated Smithers preserves setup-patch baselines across post-agent prepa assert.match(helper, /writeWorkspacePatchBaseline\(task, baselineTree\)/u); assert.match(helper, /persistedPreparation === undefined && !replayWorkspacePatches/u); assert.match(helper, /taskPublishesWorkspacePatch\(task\) && !workspacePatchBaselineTrees\.has/u); + // #312: a RESUMED task worktree can already sit at -- or past -- some dependencies' outputs, because it + // lives on a durable volume and still holds the previous attempt's state. Replay must therefore start + // after the prefix the worktree already equals byte for byte, and must do so ONLY on the replay path; + // post-agent preparation has its own rule and must not be second-guessed. Two production runs (R48, + // R49) died at `prepare:stateful-invariant-implement-properties` without this. + assert.match(source, /function firstDependencyRequiringReplay\(/u); + assert.match(helper, /const replayFrom = replayWorkspacePatches/u); + assert.match(helper, /firstDependencyRequiringReplay\(\s*captureWorkspaceTree\(workspaceRoot\),/u); + assert.match(helper, /captures\.map\(\(entry\) => entry\.manifest\.result_tree\)/u); + assert.match(helper, /captures\.slice\(replayFrom\)/u); assert.match( source, /const result = await agent\.generate\(args\);[\s\S]*?prepareArtifactMirror\(task, \{ replayWorkspacePatches: false \}\);/u diff --git a/packages/runtime/test/workspace-patch-replay.test.ts b/packages/runtime/test/workspace-patch-replay.test.ts new file mode 100644 index 000000000..2b573b04f --- /dev/null +++ b/packages/runtime/test/workspace-patch-replay.test.ts @@ -0,0 +1,234 @@ +import assert from "node:assert/strict"; +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import test from "node:test"; + +import ts from "typescript"; + +/** + * Replaying a dependency chain into a RESUMED task worktree (issue #312). + * + * `prepare:stateful-invariant-implement-properties` fans in the workspace patches of every dependency and + * replays them. That is correct on a fresh run: the task worktree starts at the pinned baseline, so each + * patch's declared `base_tree` is satisfied in turn down the chain. A resumed run breaks the precondition, + * because the worktree lives on a durable volume and still holds the previous attempt's state. + * + * Two production runs died on exactly this, at the same node, with the same expected tree: + * + * R48 expected 2dd4efefbb8b667bf6cabf4f1ec6e990e35c11ab, got bf324c395d095250396c77f57c165eee92554701 + * R49 expected 2dd4efefbb8b667bf6cabf4f1ec6e990e35c11ab, got b8d46f13b8f3988a6a94a4eff1555e73ebff61f3 + * + * `2dd4efef` is `setup-foundry`'s pristine base. The chain observed on the volume was linear: + * + * setup-foundry 2dd4efef -> 4fefa8df 1 file + * base-test-setup 4fefa8df -> 4fefa8df 0 files + * stateful-invariant-setup 4fefa8df -> fd4e3c2c 7 files + * handlers fd4e3c2c -> bf324c39 7 files + * coverage bf324c39 -> bf324c39 0 files + * + * The worktree sat at the END of that chain, so every dependency's content was already present. + * `applyWorkspacePatch` still threw, because it compares the worktree against one patch's own `base_tree` + * in isolation and cannot see the chain it belongs to. + * + * The rule under test decides where replay should START. It is sound for one reason worth stating + * plainly: a tree id is a content hash of the WHOLE snapshot. If the worktree's tree equals a + * dependency's declared `result_tree`, the workspace is byte-identical to that dependency's output, so + * that dependency and every one before it in replay order are already materialized. That is an identity, + * not an inference about intent — which is what two earlier attempts at this issue lacked. One tried to + * relax `applyWorkspacePatch` (from inside that function a fan-in is indistinguishable from real drift) + * and one tried to consult recorded provenance (a task that throws inside the replay loop never persists + * a preparation record, so the evidence does not exist at the moment the decision is made). + */ + +const runtimePackageRoot = findRuntimePackageRoot(path.dirname(fileURLToPath(import.meta.url))); +const workflowTemplatePath = path.join(runtimePackageRoot, "src", "templates", "smithers", "workflows", "workflow.tsx"); + +function findRuntimePackageRoot(startDir: string): string { + let current = startDir; + for (;;) { + if (fs.existsSync(path.join(current, "package.json"))) { + const manifest = JSON.parse(fs.readFileSync(path.join(current, "package.json"), "utf8")) as { name?: string }; + if (manifest.name === "@ultrafuzz/runtime") return current; + } + const parent = path.dirname(current); + if (parent === current) throw new Error("unable to locate the @ultrafuzz/runtime package root"); + current = parent; + } +} + +/** Lift one top-level helper out of the template, type annotations erased, and evaluate it. */ +function loadHelper(name: string): (currentTree: string, resultTrees: readonly string[]) => number { + const source = fs.readFileSync(workflowTemplatePath, "utf8"); + const start = source.indexOf(`\nfunction ${name}(`); + assert.ok(start >= 0, `the template does not declare a top-level ${name}`); + const end = source.indexOf("\n}\n", start); + assert.ok(end > start, `unterminated helper ${name}`); + const declaration = `${source.slice(start, end + 3)}\nreturn ${name};`; + const emitted = ts.transpileModule(declaration, { + compilerOptions: { target: ts.ScriptTarget.ES2022, module: ts.ModuleKind.ESNext } + }).outputText; + return new Function(emitted)() as (currentTree: string, resultTrees: readonly string[]) => number; +} + +// The chain exactly as dumped from R48's durable volume, so the fixture is production shape rather than +// something invented to suit the rule. +const PRISTINE = "2dd4efefbb8b667bf6cabf4f1ec6e990e35c11ab"; +const SETUP_FOUNDRY = "4fefa8df7da3000000000000000000000000000a"; +const INVARIANT_SETUP = "fd4e3c2cd38f000000000000000000000000000b"; +const COVERAGE = "bf324c395d09000000000000000000000000000c"; +const CHAIN = [SETUP_FOUNDRY, SETUP_FOUNDRY, INVARIANT_SETUP, COVERAGE, COVERAGE] as const; + +test("#312 a fresh worktree replays the whole dependency chain", () => { + const firstDependencyRequiringReplay = loadHelper("firstDependencyRequiringReplay"); + // The pristine baseline matches no dependency's output, so nothing has been materialized yet. This is + // the case that must not change: R46 completed this node on a fresh run, and R48 and R49 each passed + // more than twenty nodes before ever resuming. + assert.equal(firstDependencyRequiringReplay(PRISTINE, CHAIN), 0); +}); + +test("#312 a worktree at the end of the chain replays nothing", () => { + const firstDependencyRequiringReplay = loadHelper("firstDependencyRequiringReplay"); + // This is the production failure. The worktree is byte-identical to the last dependency's output, so + // every dependency in the chain is already present and replaying `setup-foundry` over it can only throw. + assert.equal(firstDependencyRequiringReplay(COVERAGE, CHAIN), CHAIN.length); +}); + +test("#312 a worktree part-way along the chain replays only the remainder", () => { + const firstDependencyRequiringReplay = loadHelper("firstDependencyRequiringReplay"); + assert.equal(firstDependencyRequiringReplay(INVARIANT_SETUP, CHAIN), 3); +}); + +test("#312 a repeated result tree resumes after its LAST occurrence", () => { + const firstDependencyRequiringReplay = loadHelper("firstDependencyRequiringReplay"); + // `base-test-setup` declares base == result, so two adjacent dependencies share an output. Resuming + // after the first would replay a no-op patch whose base tree still matches, which is harmless, but + // resuming after the LAST is the honest reading of "everything up to here is already present". + assert.equal(firstDependencyRequiringReplay(SETUP_FOUNDRY, CHAIN), 2); +}); + +test("#312 an unrecognised worktree tree replays everything and lets the patch check speak", () => { + const firstDependencyRequiringReplay = loadHelper("firstDependencyRequiringReplay"); + // Genuine drift must still be caught. The rule never suppresses an error: when the worktree matches no + // declared output it replays from the start, and `applyWorkspacePatch` raises the base-tree mismatch + // exactly as it does today. Silence here would turn a loud failure into a corrupt workspace. + assert.equal(firstDependencyRequiringReplay("deadbeef".repeat(5), CHAIN), 0); + assert.equal(firstDependencyRequiringReplay(PRISTINE, []), 0); +}); + +/** + * Lift `materializeWorkspacePatchDependencies` itself out of the template and run it against stubbed + * collaborators, so the WIRING is pinned and not just the rule. + * + * Without this, deleting the `firstDependencyRequiringReplay` call and replaying from zero leaves every + * test above green — the exact shape of gap that survived mutation testing on the previous change here. + */ +function loadMaterializer( + collaborators: Record +): (task: unknown, workspaceRoot: string, replay: boolean) => void { + const source = fs.readFileSync(workflowTemplatePath, "utf8"); + const slice = (name: string): string => { + const start = source.indexOf(`\nfunction ${name}(`); + assert.ok(start >= 0, `the template does not declare a top-level ${name}`); + const end = source.indexOf("\n}\n", start); + assert.ok(end > start, `unterminated helper ${name}`); + return source.slice(start, end + 3); + }; + const declaration = [ + slice("firstDependencyRequiringReplay"), + slice("materializeWorkspacePatchDependencies"), + "\nreturn materializeWorkspacePatchDependencies;" + ].join("\n"); + const emitted = ts.transpileModule(declaration, { + compilerOptions: { target: ts.ScriptTarget.ES2022, module: ts.ModuleKind.ESNext } + }).outputText; + const names = Object.keys(collaborators); + return new Function(...names, emitted)(...names.map((name) => collaborators[name])) as ( + task: unknown, + workspaceRoot: string, + replay: boolean + ) => void; +} + +function replayScenario(worktreeTree: string, replay = true): string[] { + const applied: string[] = []; + const chain = [ + { dir: "/dep/setup-foundry", base: PRISTINE, result: SETUP_FOUNDRY }, + { dir: "/dep/base-test-setup", base: SETUP_FOUNDRY, result: SETUP_FOUNDRY }, + { dir: "/dep/stateful-invariant-setup", base: SETUP_FOUNDRY, result: INVARIANT_SETUP }, + { dir: "/dep/stateful-invariant-handlers", base: INVARIANT_SETUP, result: COVERAGE }, + { dir: "/dep/stateful-invariant-coverage", base: COVERAGE, result: COVERAGE } + ]; + const byPath = new Map(chain.map((entry) => [entry.dir, entry])); + const task = { + attemptId: "stateful-invariant-implement-properties", + dependencyArtifactDirs: chain.map((entry) => entry.dir), + outputs: [], + metadata: { artifacts: { dir: "/artifacts/implement-properties" } } + }; + const materialize = loadMaterializer({ + path, + taskSpecs: chain.map((entry) => ({ attemptId: entry.dir.split("/").pop() })), + existsSync: () => true, + readFileSync: (target: string) => { + const entry = byPath.get(path.dirname(target)); + assert.ok(entry !== undefined, target); + if (target.endsWith("workspace-patch.json")) { + return JSON.stringify({ base_tree: entry.base, result_tree: entry.result }); + } + return `patch for ${entry.dir}`; + }, + resolveRegularArtifactFile: (_dir: string, candidate: string) => candidate, + captureWorkspaceTree: () => worktreeTree, + applyWorkspacePatch: (_root: string, capture: { patch: string }) => { + applied.push(capture.patch); + }, + workspacePatchPreparationTrees: new Map(), + workspacePatchBaselineTrees: new Map(), + // Post-agent preparation fails closed without a durable preparation record (#219), so the stub must + // supply one. It plays no part in the replay decision under test. + readWorkspacePatchPreparation: () => "preparation-tree", + writeWorkspacePatchPreparation: () => undefined, + readWorkspacePatchBaseline: () => undefined, + writeWorkspacePatchBaseline: () => undefined, + taskPublishesWorkspacePatch: () => false + }); + materialize(task, "/workspace", replay); + return applied; +} + +test("#312 replay applies every dependency when the worktree is at the pinned baseline", () => { + assert.deepEqual(replayScenario(PRISTINE), [ + "patch for /dep/setup-foundry", + "patch for /dep/base-test-setup", + "patch for /dep/stateful-invariant-setup", + "patch for /dep/stateful-invariant-handlers", + "patch for /dep/stateful-invariant-coverage" + ]); +}); + +test("#312 replay applies NOTHING when the worktree already holds the whole chain", () => { + // The production failure, end to end through the real helper: R48 and R49 both died here because + // `setup-foundry`'s patch was applied over a worktree that already contained its file, folded in by + // three later dependencies. Applying even one patch in this state is the bug. + assert.deepEqual(replayScenario(COVERAGE), []); +}); + +test("#312 replay resumes mid-chain from the first dependency the worktree does not already hold", () => { + assert.deepEqual(replayScenario(INVARIANT_SETUP), [ + "patch for /dep/stateful-invariant-handlers", + "patch for /dep/stateful-invariant-coverage" + ]); +}); + +test("#312 post-agent preparation keeps its own rule and is not second-guessed by the chain skip", () => { + // `replayWorkspacePatches: false` runs AFTER the agent, over a deliberately dirty worktree, and has its + // own long-standing rule: apply a dependency only when its base tree is exactly what is on disk. The + // chain skip must not run there. This fixture discriminates the two: a worktree at `SETUP_FOUNDRY` is + // the declared base of BOTH `base-test-setup` and `stateful-invariant-setup`, so the post-agent rule + // applies two patches -- while the chain skip would resume past index 1 and apply only one. + assert.deepEqual(replayScenario(SETUP_FOUNDRY, false), [ + "patch for /dep/base-test-setup", + "patch for /dep/stateful-invariant-setup" + ]); +}); From fe178b34e5aabef15bbd4a9f896ef0f41decfb12 Mon Sep 17 00:00:00 2001 From: Antonio Viggiano <3029017+aviggiano@users.noreply.github.com> Date: Fri, 7 Aug 2026 06:04:31 +0000 Subject: [PATCH 2/4] fix(runtime): only skip a dependency prefix that is genuinely a chain Both reviews of ea0085c returned NO-MERGE on the same defect, and they were right. My soundness argument had two steps and I only justified the first. 1. worktree tree == dependency i's result_tree => the workspace equals that dependency's output. This IS a hash identity. 2. => everything before i is already materialized. This is NOT. It holds only if the sorted dependency list is a chain, and nothing checked that. The adversarial reviewer reproduced the consequence end to end against real git, and it is worse than the bug being fixed -- it corrupts a FRESH run: setup-foundry-0: base 32263f5d -> result e1507a29 (writes the scaffold) setup-foundry-1: base 32263f5d -> result 32263f5d (agent changed nothing) worktree: 32263f5d (the pinned baseline) origin/main : 2 patches applied, loud base-tree mismatch ea0085c : 0 patches applied, NO error, empty workspace The backward scan matched the no-op sibling at the last index and skipped the entire list. A partial skip is self-validating -- the next applyWorkspacePatch re-checks base_tree and throws -- but a TOTAL skip validates nothing, which is exactly what a sibling fan-in produces. Reachable in one line of topology: the `strategies` group defaults to `loops: 3, loop_mode: parallel` and every patch publisher is held to one copy only by an explicit `loops: 1`, with nothing validating that a workspace-patch publisher may not fan out. The prefix is now checked link by link and replay falls back to 0 when it is not a chain, so we fail the way we already fail. Silent garbage findings are far worse than a crash. Also from review: - "It never suppresses an error" was false as written. The manifest schema, object ids, patch digest, symlink/submodule rejection and sensitive-path rejection all live inside applyWorkspacePatch, so a skipped patch was never validated at all -- and the skip decision reads result_tree, so an unchecked field was steering it. Split out validateWorkspacePatchCapture, called for EVERY capture regardless of the skip. The base-commit check moved there too: a re-pinned target between attempts would otherwise go unnoticed on a skip. - The R49 half of the causal claim is withdrawn. Only R48's manifests were ever dumped; b8d46f13 appears in no manifest anywhere because the R49 volume walk timed out. R49 failing at the same node with the same expected tree is measured; its worktree being at the end of ITS chain was an inference stated as fact, in a comment, which is the specific habit this project has already paid for twice. - "byte-identical" overstated what captureWorkspaceTree measures: it hashes the staged snapshot, excluding untracked paths under the runtime and generated roots. The decision is still sound -- captureWorkspacePatch stages through the same function, so no patch can carry a path outside the hash -- but the comment now says what is actually compared. - captureWorkspaceTree no longer runs for tasks with no patch dependencies. - The claim that a fresh run never matches was too strong; a leading zero-file publisher declares base == result and does match. Tests 9 -> 13. New: the reviewer's fresh-run sibling fixture; a non-chain fan-in; validation of skipped captures; and replay order, which the sort alone determines and which nothing pinned -- `.sort(() => 0)` passed the entire suite before. The stubbed worktree tree now ADVANCES as patches land, as it does on a real run. A frozen stub hid a surviving mutation (hoisting the per-iteration tree read out of the post-agent loop) and had also made me assert a post-agent result of two patches where the real path applies four. Full runtime suite: 371 tests, 356 pass, 0 fail. Co-Authored-By: Claude Opus 5 --- .../templates/smithers/workflows/workflow.tsx | 97 ++++++--- packages/runtime/src/workspace-handoff.ts | 26 ++- .../test/generated-workflow-verifier.test.ts | 10 +- .../test/workspace-patch-replay.test.ts | 195 ++++++++++++++++-- 4 files changed, 282 insertions(+), 46 deletions(-) diff --git a/packages/runtime/src/templates/smithers/workflows/workflow.tsx b/packages/runtime/src/templates/smithers/workflows/workflow.tsx index 3fe31d8de..b1d85fdeb 100644 --- a/packages/runtime/src/templates/smithers/workflows/workflow.tsx +++ b/packages/runtime/src/templates/smithers/workflows/workflow.tsx @@ -38,8 +38,13 @@ const { validateInvariantSourceProofSchema, writeFileDurable } = await import(artifactsModule); -const { applyWorkspacePatch, captureWorkspacePatch, captureWorkspaceTree, normalizeFinalReportSeverityRecord } = - await import(runtimeModule); +const { + applyWorkspacePatch, + captureWorkspacePatch, + captureWorkspaceTree, + normalizeFinalReportSeverityRecord, + validateWorkspacePatchCapture +} = await import(runtimeModule); const inputTaskSchema = z.object({ id: z.string(), @@ -432,27 +437,54 @@ function taskPublishesWorkspacePatch(task: (typeof taskSpecs)[number]): boolean * Replaying every dependency patch unconditionally is correct on a fresh run: the task worktree begins at * the pinned baseline, so each patch's declared `base_tree` is satisfied in turn down the chain. A RESUMED * run breaks that precondition, because the worktree lives on a durable volume and still holds the - * previous attempt's state. Two production runs died on it at the same node with the same expected tree — - * R48 (`got bf324c39…`) and R49 (`got b8d46f13…`), both against `setup-foundry`'s pristine `2dd4efef…`. - * In both, the worktree was at the END of the chain: every dependency's content was already present, and - * `applyWorkspacePatch` threw only because it compares the worktree against one patch's own `base_tree` in - * isolation and cannot see the chain that patch belongs to. + * previous attempt's state. R48 died on it at this node with `expected 2dd4efef… got bf324c39…`, and its + * dependency manifests, dumped from the volume, show why: the worktree was at the END of the chain, so + * every dependency's content was already present and `applyWorkspacePatch` threw only because it compares + * the worktree against one patch's own `base_tree` in isolation. R49 failed at the same node with the same + * expected tree (`got b8d46f13…`), but its manifests were never dumped, so its worktree being at the end + * of ITS chain is a hypothesis, not a measurement. + * + * Two conditions are needed, and only the first is a hash identity: + * + * 1. The worktree's tree equals dependency `i`'s declared `result_tree`. A tree id is a content hash, so + * this means the workspace is identical to that dependency's output over the snapshot the hash covers + * — every path `stageWorkspaceTree` stages. Content outside it is content no patch can carry either, + * because `captureWorkspacePatch` diffs the same staged index, so nothing patch-delivered is missed. + * 2. The dependencies BEFORE `i` form a chain into it, each one's `result_tree` being the next one's + * `base_tree`. Without this, "everything before `i` is already materialized" is an inference about + * topology rather than a fact about content — and a false one for a fan-in of siblings that share a + * base and diverge, where skipping to the end would silently drop a sibling's work and leave a hole + * that every descendant then inherits through this task's own published patch. * - * The rule is sound for one reason, and it is worth being precise about it: a tree id is a content hash of - * the WHOLE snapshot. If the worktree's tree equals a dependency's declared `result_tree`, the workspace - * is byte-identical to that dependency's output, so that dependency and every one before it in replay - * order are already materialized. That is an identity, not an inference about what someone intended. + * Condition 2 is why this checks the prefix instead of trusting the ordering. A partial skip is + * self-validating (the next `applyWorkspacePatch` re-checks `base_tree` and throws), but a TOTAL skip + * validates nothing at all, and that is exactly the case a non-chain fan-in produces. * - * It also never suppresses an error. A worktree matching no declared output replays from the start and - * `applyWorkspacePatch` raises its base-tree mismatch exactly as before, which is what keeps genuine drift - * loud instead of quietly producing a corrupt workspace. + * Today's topology pins `loops: 1` on every patch publisher, so a sibling fan-in is not reachable; but + * `loop_mode` defaults to `parallel`, nothing validates linearity of `workspace-patch@1` publishers, and + * the dependency sort follows topology DECLARATION order, which is not required to be causal order. A + * one-line topology change should not silently corrupt a workspace. + * + * When the prefix is not a chain this returns 0: replay everything, and let `applyWorkspacePatch` raise + * its base-tree mismatch exactly as it does today. Failing the way we already fail is the safe direction. */ -function firstDependencyRequiringReplay(currentTree: string, resultTrees: readonly string[]): number { - // Scan from the end: with a no-op dependency in the chain (`base-test-setup` declares base == result) +function firstDependencyRequiringReplay( + currentTree: string, + manifests: readonly { base_tree: string; result_tree: string }[] +): number { + // Scan from the end: with a no-op dependency in the chain (`base-test-setup` declared base == result) // two adjacent entries share an output, and resuming after the LAST of them is the honest reading of // "everything up to here is already present". - for (let index = resultTrees.length - 1; index >= 0; index -= 1) { - if (resultTrees[index] === currentTree) return index + 1; + for (let index = manifests.length - 1; index >= 0; index -= 1) { + if (manifests[index]?.result_tree !== currentTree) continue; + let chained = true; + for (let link = 1; link <= index; link += 1) { + if (manifests[link]?.base_tree !== manifests[link - 1]?.result_tree) { + chained = false; + break; + } + } + return chained ? index + 1 : 0; } return 0; } @@ -477,7 +509,7 @@ function materializeWorkspacePatchDependencies( const rightIndex = taskSpecs.findIndex((candidate) => candidate.attemptId === path.basename(right)); return leftIndex - rightIndex || left.localeCompare(right); }); - // Read every manifest BEFORE applying any of them. The decision below is about the chain as a whole -- + // Read every manifest and patch BEFORE applying any of them. The decision below is about the chain as a whole -- // whether a LATER dependency's output already describes this worktree -- and that cannot be made one // patch at a time. Reading first also keeps the artifact-contract failures ordered by dependency rather // than interleaved with partially applied patches. @@ -505,15 +537,24 @@ function materializeWorkspacePatchDependencies( manifest: manifest as Parameters[1]["manifest"] }; }); - // On a RESUME the worktree lives on a durable volume and still holds the previous attempt's state, so - // it can already sit at -- or past -- some of these dependencies' outputs. Skip the prefix the worktree - // already equals byte for byte (issue #312); on a fresh run nothing matches and this is a no-op. - const replayFrom = replayWorkspacePatches - ? firstDependencyRequiringReplay( - captureWorkspaceTree(workspaceRoot), - captures.map((entry) => entry.manifest.result_tree) - ) - : 0; + // Validate EVERY capture, including any the replay below decides to skip. All of these checks -- the + // manifest schema, the object ids, the patch digest, the symlink/submodule rejection and the + // sensitive-path rejection -- used to live inside `applyWorkspacePatch`, so skipping a patch meant + // skipping its validation entirely, and the skip decision reads `result_tree` from a manifest nothing + // had checked was even well formed. + for (const capture of captures) validateWorkspacePatchCapture(workspaceRoot, capture); + // On a RESUME the worktree lives on a durable volume and still holds the previous attempt's state, so it + // can already sit at -- or past -- some of these dependencies' outputs. Skip the prefix the worktree + // already holds (issue #312). On a fresh run at the pinned baseline nothing normally matches, though a + // leading dependency that published a zero-file patch declares `base_tree === result_tree` and so can + // match; skipping that one is a no-op, since `applyWorkspacePatch` already early-returns on it. + const replayFrom = + replayWorkspacePatches && captures.length > 0 + ? firstDependencyRequiringReplay( + captureWorkspaceTree(workspaceRoot), + captures.map((entry) => entry.manifest) + ) + : 0; for (const capture of captures.slice(replayFrom)) { if (!replayWorkspacePatches) { // Post-agent preparation may see a dirty worktree. Replay only when the diff --git a/packages/runtime/src/workspace-handoff.ts b/packages/runtime/src/workspace-handoff.ts index 38125c718..6ec27cc04 100644 --- a/packages/runtime/src/workspace-handoff.ts +++ b/packages/runtime/src/workspace-handoff.ts @@ -175,7 +175,17 @@ export function captureWorkspacePatch(workspaceRoot: string, baselineTree: strin } /** Apply one validated dependency patch to a clean downstream worktree. */ -export function applyWorkspacePatch(workspaceRoot: string, capture: WorkspacePatchCapture): void { +/** + * Every check on a capture that does not depend on the worktree it will be applied to. + * + * Split out of `applyWorkspacePatch` so a caller that decides NOT to apply a patch can still validate it. + * A replaying caller may legitimately skip a patch whose content the worktree already holds (issue #312), + * and before this existed, skipping meant the manifest schema, the digest, the object ids and the + * sensitive-path rejection were never checked for that dependency at all — so a manifest naming `.env`, + * or a `patch_sha256` that does not match its bytes, would be accepted in silence. Worse, the skip + * decision itself reads `result_tree`, so an unvalidated field was steering it. + */ +export function validateWorkspacePatchCapture(workspaceRoot: string, capture: WorkspacePatchCapture): void { validateManifest(capture.manifest); if (Buffer.byteLength(capture.patch, "utf8") > MAX_PATCH_BYTES) { throw new Error(`workspace patch exceeds ${MAX_PATCH_BYTES} bytes`); @@ -183,10 +193,21 @@ export function applyWorkspacePatch(workspaceRoot: string, capture: WorkspacePat if (sha256(capture.patch) !== capture.manifest.patch_sha256) { throw new Error("workspace patch digest mismatch"); } + if (/\b(?:new|old) file mode (?:120000|160000)\b|\b(?:new|old) mode 160000\b/u.test(capture.patch)) { + throw new Error("workspace patch contains a symlink or submodule entry"); + } + // The pinned commit is part of the contract, not of the application: a dependency captured against a + // different `HEAD` describes a different target, and that is worth rejecting whether or not this patch + // is going to be applied. A caller that skips a patch because the worktree already holds its content + // would otherwise never notice the target had been re-pinned between attempts. const head = runGit(workspaceRoot, ["rev-parse", "HEAD"]).trim(); if (head !== capture.manifest.base_commit) { throw new Error(`workspace patch base commit mismatch: expected ${capture.manifest.base_commit}, got ${head}`); } +} + +export function applyWorkspacePatch(workspaceRoot: string, capture: WorkspacePatchCapture): void { + validateWorkspacePatchCapture(workspaceRoot, capture); const currentTree = captureWorkspaceTree(workspaceRoot); if (currentTree === capture.manifest.result_tree) return; if (currentTree !== capture.manifest.base_tree) { @@ -199,9 +220,6 @@ export function applyWorkspacePatch(workspaceRoot: string, capture: WorkspacePat } return; } - if (/\b(?:new|old) file mode (?:120000|160000)\b|\b(?:new|old) mode 160000\b/u.test(capture.patch)) { - throw new Error("workspace patch contains a symlink or submodule entry"); - } runGit(workspaceRoot, ["apply", "--check", "--binary", "--whitespace=nowarn", "-"], undefined, capture.patch); runGit(workspaceRoot, ["apply", "--binary", "--whitespace=nowarn", "-"], undefined, capture.patch); const appliedTree = captureWorkspaceTree(workspaceRoot); diff --git a/packages/runtime/test/generated-workflow-verifier.test.ts b/packages/runtime/test/generated-workflow-verifier.test.ts index bff6e9e53..ab9aae526 100644 --- a/packages/runtime/test/generated-workflow-verifier.test.ts +++ b/packages/runtime/test/generated-workflow-verifier.test.ts @@ -1648,9 +1648,15 @@ test("generated Smithers preserves setup-patch baselines across post-agent prepa // post-agent preparation has its own rule and must not be second-guessed. Two production runs (R48, // R49) died at `prepare:stateful-invariant-implement-properties` without this. assert.match(source, /function firstDependencyRequiringReplay\(/u); - assert.match(helper, /const replayFrom = replayWorkspacePatches/u); + assert.match(helper, /replayWorkspacePatches && captures\.length > 0/u); assert.match(helper, /firstDependencyRequiringReplay\(\s*captureWorkspaceTree\(workspaceRoot\),/u); - assert.match(helper, /captures\.map\(\(entry\) => entry\.manifest\.result_tree\)/u); + assert.match(helper, /captures\.map\(\(entry\) => entry\.manifest\)/u); + // Every capture is validated even when replay skips it: the manifest schema, object ids, digest and + // sensitive-path checks all live inside `applyWorkspacePatch`, so a skipped patch would otherwise go + // entirely unchecked while its `result_tree` steered the skip decision. + assert.match(helper, /for \(const capture of captures\) validateWorkspacePatchCapture\(workspaceRoot, capture\);/u); + // The skip is only sound when the skipped prefix is a real chain; a sibling fan-in must replay. + assert.match(source, /return chained \? index \+ 1 : 0;/u); assert.match(helper, /captures\.slice\(replayFrom\)/u); assert.match( source, diff --git a/packages/runtime/test/workspace-patch-replay.test.ts b/packages/runtime/test/workspace-patch-replay.test.ts index 2b573b04f..dd4bb064e 100644 --- a/packages/runtime/test/workspace-patch-replay.test.ts +++ b/packages/runtime/test/workspace-patch-replay.test.ts @@ -27,9 +27,11 @@ import ts from "typescript"; * handlers fd4e3c2c -> bf324c39 7 files * coverage bf324c39 -> bf324c39 0 files * - * The worktree sat at the END of that chain, so every dependency's content was already present. - * `applyWorkspacePatch` still threw, because it compares the worktree against one patch's own `base_tree` - * in isolation and cannot see the chain it belongs to. + * That chain is R48's, dumped from its volume. Its worktree sat at the END of it, so every dependency's + * content was already present and `applyWorkspacePatch` threw only because it compares the worktree + * against one patch's own `base_tree` in isolation. R49 failed at the same node with the same expected + * tree, but its own manifests were never dumped -- the volume walk timed out -- so R49's worktree being at + * the end of ITS chain is a hypothesis. The fixtures below are R48's measured shape. * * The rule under test decides where replay should START. It is sound for one reason worth stating * plainly: a tree id is a content hash of the WHOLE snapshot. If the worktree's tree equals a @@ -58,7 +60,9 @@ function findRuntimePackageRoot(startDir: string): string { } /** Lift one top-level helper out of the template, type annotations erased, and evaluate it. */ -function loadHelper(name: string): (currentTree: string, resultTrees: readonly string[]) => number { +type Manifest = { base_tree: string; result_tree: string }; + +function loadHelper(name: string): (currentTree: string, manifests: readonly Manifest[]) => number { const source = fs.readFileSync(workflowTemplatePath, "utf8"); const start = source.indexOf(`\nfunction ${name}(`); assert.ok(start >= 0, `the template does not declare a top-level ${name}`); @@ -68,7 +72,7 @@ function loadHelper(name: string): (currentTree: string, resultTrees: readonly s const emitted = ts.transpileModule(declaration, { compilerOptions: { target: ts.ScriptTarget.ES2022, module: ts.ModuleKind.ESNext } }).outputText; - return new Function(emitted)() as (currentTree: string, resultTrees: readonly string[]) => number; + return new Function(emitted)() as (currentTree: string, manifests: readonly Manifest[]) => number; } // The chain exactly as dumped from R48's durable volume, so the fixture is production shape rather than @@ -77,7 +81,13 @@ const PRISTINE = "2dd4efefbb8b667bf6cabf4f1ec6e990e35c11ab"; const SETUP_FOUNDRY = "4fefa8df7da3000000000000000000000000000a"; const INVARIANT_SETUP = "fd4e3c2cd38f000000000000000000000000000b"; const COVERAGE = "bf324c395d09000000000000000000000000000c"; -const CHAIN = [SETUP_FOUNDRY, SETUP_FOUNDRY, INVARIANT_SETUP, COVERAGE, COVERAGE] as const; +const CHAIN: readonly Manifest[] = [ + { base_tree: PRISTINE, result_tree: SETUP_FOUNDRY }, + { base_tree: SETUP_FOUNDRY, result_tree: SETUP_FOUNDRY }, + { base_tree: SETUP_FOUNDRY, result_tree: INVARIANT_SETUP }, + { base_tree: INVARIANT_SETUP, result_tree: COVERAGE }, + { base_tree: COVERAGE, result_tree: COVERAGE } +]; test("#312 a fresh worktree replays the whole dependency chain", () => { const firstDependencyRequiringReplay = loadHelper("firstDependencyRequiringReplay"); @@ -166,6 +176,11 @@ function replayScenario(worktreeTree: string, replay = true): string[] { outputs: [], metadata: { artifacts: { dir: "/artifacts/implement-properties" } } }; + // The worktree tree ADVANCES as patches land, exactly as it does on a real run. A stub that returns a + // constant hides a real defect: the post-agent branch re-reads the tree every iteration precisely + // because each applied patch changes it, and with a frozen stub, hoisting that read out of the loop is + // indistinguishable from leaving it in. + let currentTree = worktreeTree; const materialize = loadMaterializer({ path, taskSpecs: chain.map((entry) => ({ attemptId: entry.dir.split("/").pop() })), @@ -179,9 +194,10 @@ function replayScenario(worktreeTree: string, replay = true): string[] { return `patch for ${entry.dir}`; }, resolveRegularArtifactFile: (_dir: string, candidate: string) => candidate, - captureWorkspaceTree: () => worktreeTree, - applyWorkspacePatch: (_root: string, capture: { patch: string }) => { + captureWorkspaceTree: () => currentTree, + applyWorkspacePatch: (_root: string, capture: { patch: string; manifest: { result_tree: string } }) => { applied.push(capture.patch); + currentTree = capture.manifest.result_tree; }, workspacePatchPreparationTrees: new Map(), workspacePatchBaselineTrees: new Map(), @@ -191,6 +207,7 @@ function replayScenario(worktreeTree: string, replay = true): string[] { writeWorkspacePatchPreparation: () => undefined, readWorkspacePatchBaseline: () => undefined, writeWorkspacePatchBaseline: () => undefined, + validateWorkspacePatchCapture: () => undefined, taskPublishesWorkspacePatch: () => false }); materialize(task, "/workspace", replay); @@ -224,11 +241,165 @@ test("#312 replay resumes mid-chain from the first dependency the worktree does test("#312 post-agent preparation keeps its own rule and is not second-guessed by the chain skip", () => { // `replayWorkspacePatches: false` runs AFTER the agent, over a deliberately dirty worktree, and has its // own long-standing rule: apply a dependency only when its base tree is exactly what is on disk. The - // chain skip must not run there. This fixture discriminates the two: a worktree at `SETUP_FOUNDRY` is - // the declared base of BOTH `base-test-setup` and `stateful-invariant-setup`, so the post-agent rule - // applies two patches -- while the chain skip would resume past index 1 and apply only one. + // chain skip must not run there. This fixture discriminates the two: starting at `SETUP_FOUNDRY` the + // post-agent rule applies four patches, walking the tree forward one dependency at a time, whereas the + // chain skip would resume past index 1 and apply only three. assert.deepEqual(replayScenario(SETUP_FOUNDRY, false), [ "patch for /dep/base-test-setup", - "patch for /dep/stateful-invariant-setup" + "patch for /dep/stateful-invariant-setup", + "patch for /dep/stateful-invariant-handlers", + "patch for /dep/stateful-invariant-coverage" + ]); +}); + +test("#312 a NON-chain fan-in replays everything instead of silently dropping a sibling", () => { + const firstDependencyRequiringReplay = loadHelper("firstDependencyRequiringReplay"); + // Two siblings share a base and diverge: A: T0->T1, B: T1->T2, C: T1->T3, worktree at T3. + // Matching C's output says nothing about whether B's content is present -- "everything before it is + // materialized" is a claim about TOPOLOGY, and it is false here. Skipping to the end would leave a hole + // that no check catches: a total skip validates nothing, and this task's own published patch would then + // carry the hole to every descendant with manifests that all still verify. + const T0 = "0".repeat(40); + const T1 = "1".repeat(40); + const T2 = "2".repeat(40); + const T3 = "3".repeat(40); + const fanIn: readonly Manifest[] = [ + { base_tree: T0, result_tree: T1 }, + { base_tree: T1, result_tree: T2 }, + { base_tree: T1, result_tree: T3 } + ]; + // Replay everything and let `applyWorkspacePatch` raise its base-tree mismatch, exactly as today. + // Failing the way we already fail is the safe direction; a wrong workspace is worse than a crash. + assert.equal(firstDependencyRequiringReplay(T3, fanIn), 0); + // The chained prefix of the same list is still honoured when the match is inside it. + assert.equal(firstDependencyRequiringReplay(T1, fanIn), 1); +}); + +test("#312 every dependency capture is validated even when replay skips it", () => { + // All of the capture checks -- manifest schema, object ids, digest, symlink/submodule, sensitive paths + // -- used to live inside `applyWorkspacePatch`, so a skipped patch was never validated at all, and the + // skip decision itself reads `result_tree` from a manifest nothing had checked was well formed. + const validated: string[] = []; + const applied: string[] = []; + const chain = [ + { dir: "/dep/setup-foundry", base: PRISTINE, result: SETUP_FOUNDRY }, + { dir: "/dep/stateful-invariant-handlers", base: SETUP_FOUNDRY, result: COVERAGE } + ]; + const byPath = new Map(chain.map((entry) => [entry.dir, entry])); + const materialize = loadMaterializer({ + path, + taskSpecs: chain.map((entry) => ({ attemptId: entry.dir.split("/").pop() })), + existsSync: () => true, + readFileSync: (target: string) => { + const entry = byPath.get(path.dirname(target)); + assert.ok(entry !== undefined, target); + return target.endsWith("workspace-patch.json") + ? JSON.stringify({ base_tree: entry.base, result_tree: entry.result }) + : `patch for ${entry.dir}`; + }, + resolveRegularArtifactFile: (_dir: string, candidate: string) => candidate, + captureWorkspaceTree: () => COVERAGE, + applyWorkspacePatch: (_root: string, capture: { patch: string }) => void applied.push(capture.patch), + validateWorkspacePatchCapture: (_root: string, capture: { patch: string }) => void validated.push(capture.patch), + workspacePatchPreparationTrees: new Map(), + workspacePatchBaselineTrees: new Map(), + readWorkspacePatchPreparation: () => "preparation-tree", + writeWorkspacePatchPreparation: () => undefined, + readWorkspacePatchBaseline: () => undefined, + writeWorkspacePatchBaseline: () => undefined, + taskPublishesWorkspacePatch: () => false + }); + materialize( + { + attemptId: "stateful-invariant-implement-properties", + dependencyArtifactDirs: chain.map((entry) => entry.dir), + outputs: [], + metadata: { artifacts: { dir: "/artifacts/implement-properties" } } + }, + "/workspace", + true + ); + assert.deepEqual(applied, [], "the whole chain is already present, so nothing should be applied"); + assert.deepEqual( + validated, + ["patch for /dep/setup-foundry", "patch for /dep/stateful-invariant-handlers"], + "every capture must be validated even though replay skipped both" + ); +}); + +test("#312 a no-op sibling sorting LAST does not wipe out an earlier sibling's work", () => { + const firstDependencyRequiringReplay = loadHelper("firstDependencyRequiringReplay"); + // Reported by review, reproduced end to end against real git: two siblings share a base, the one that + // sorts LAST published a zero-file patch because its agent changed nothing, and the worktree is at the + // PINNED BASELINE -- a FRESH run, no resume involved. A backward scan without the chain check matches + // the no-op sibling at the last index, skips the entire list, and the harness is built from an empty + // workspace with no error anywhere. Before this change that was a loud crash; silent garbage findings + // are far worse than a crash. + // + // Reachable in one line of topology: the `strategies` group defaults to `loops: 3, loop_mode: parallel` + // and every patch publisher is held to a single copy only by an explicit `loops: 1`, with nothing + // validating that a `workspace-patch@1` publisher may not fan out. + const base = "32263f5d".padEnd(40, "0"); + const siblingResult = "e1507a29".padEnd(40, "0"); + const siblings: readonly Manifest[] = [ + { base_tree: base, result_tree: siblingResult }, + { base_tree: base, result_tree: base } + ]; + assert.equal(firstDependencyRequiringReplay(base, siblings), 0); +}); + +test("#312 dependencies are replayed in task order regardless of the order they arrive in", () => { + // The sort IS the replay order, and the skip rule's correctness depends entirely on it, yet nothing + // pinned it: replacing the comparator with `() => 0` passed the whole suite. The fixture hands the + // dependency directories over shuffled, so a comparator that does not order by task index is caught. + const applied: string[] = []; + const chain = [ + { dir: "/dep/setup-foundry", base: PRISTINE, result: SETUP_FOUNDRY }, + { dir: "/dep/stateful-invariant-setup", base: SETUP_FOUNDRY, result: INVARIANT_SETUP }, + { dir: "/dep/stateful-invariant-handlers", base: INVARIANT_SETUP, result: COVERAGE } + ]; + const byPath = new Map(chain.map((entry) => [entry.dir, entry])); + const materialize = loadMaterializer({ + path, + taskSpecs: chain.map((entry) => ({ attemptId: entry.dir.split("/").pop() })), + existsSync: () => true, + readFileSync: (target: string) => { + const entry = byPath.get(path.dirname(target)); + assert.ok(entry !== undefined, target); + return target.endsWith("workspace-patch.json") + ? JSON.stringify({ base_tree: entry.base, result_tree: entry.result }) + : `patch for ${entry.dir}`; + }, + resolveRegularArtifactFile: (_dir: string, candidate: string) => candidate, + captureWorkspaceTree: () => PRISTINE, + applyWorkspacePatch: (_root: string, capture: { patch: string }) => void applied.push(capture.patch), + validateWorkspacePatchCapture: () => undefined, + workspacePatchPreparationTrees: new Map(), + workspacePatchBaselineTrees: new Map(), + readWorkspacePatchPreparation: () => "preparation-tree", + writeWorkspacePatchPreparation: () => undefined, + readWorkspacePatchBaseline: () => undefined, + writeWorkspacePatchBaseline: () => undefined, + taskPublishesWorkspacePatch: () => false + }); + materialize( + { + attemptId: "stateful-invariant-implement-properties", + // Deliberately shuffled relative to task order. + dependencyArtifactDirs: [ + "/dep/stateful-invariant-handlers", + "/dep/setup-foundry", + "/dep/stateful-invariant-setup" + ], + outputs: [], + metadata: { artifacts: { dir: "/artifacts/implement-properties" } } + }, + "/workspace", + true + ); + assert.deepEqual(applied, [ + "patch for /dep/setup-foundry", + "patch for /dep/stateful-invariant-setup", + "patch for /dep/stateful-invariant-handlers" ]); }); From dd9df13c3d89121f0b4277ba0cfdca2ffe8d8e78 Mon Sep 17 00:00:00 2001 From: Antonio Viggiano <3029017+aviggiano@users.noreply.github.com> Date: Fri, 7 Aug 2026 06:06:25 +0000 Subject: [PATCH 3/4] docs(runtime): correct the replay test header to the two-condition rule The source comment was fixed when review falsified the one-step soundness argument; this file's header still asserted it. It claimed the worktree matching a dependency's result_tree meant 'that dependency and every one before it in replay order are already materialized. That is an identity' -- which is exactly the step that is NOT an identity, and exactly what a sibling fan-in breaks on a fresh run. Fixing the code and leaving the prose is how a file ends up documenting a claim its own tests disprove. Co-Authored-By: Claude Opus 5 --- .../test/workspace-patch-replay.test.ts | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/runtime/test/workspace-patch-replay.test.ts b/packages/runtime/test/workspace-patch-replay.test.ts index dd4bb064e..837cc6b5b 100644 --- a/packages/runtime/test/workspace-patch-replay.test.ts +++ b/packages/runtime/test/workspace-patch-replay.test.ts @@ -14,7 +14,7 @@ import ts from "typescript"; * patch's declared `base_tree` is satisfied in turn down the chain. A resumed run breaks the precondition, * because the worktree lives on a durable volume and still holds the previous attempt's state. * - * Two production runs died on exactly this, at the same node, with the same expected tree: + * Two production runs died at this node with the same expected tree: * * R48 expected 2dd4efefbb8b667bf6cabf4f1ec6e990e35c11ab, got bf324c395d095250396c77f57c165eee92554701 * R49 expected 2dd4efefbb8b667bf6cabf4f1ec6e990e35c11ab, got b8d46f13b8f3988a6a94a4eff1555e73ebff61f3 @@ -33,14 +33,22 @@ import ts from "typescript"; * tree, but its own manifests were never dumped -- the volume walk timed out -- so R49's worktree being at * the end of ITS chain is a hypothesis. The fixtures below are R48's measured shape. * - * The rule under test decides where replay should START. It is sound for one reason worth stating - * plainly: a tree id is a content hash of the WHOLE snapshot. If the worktree's tree equals a - * dependency's declared `result_tree`, the workspace is byte-identical to that dependency's output, so - * that dependency and every one before it in replay order are already materialized. That is an identity, - * not an inference about intent — which is what two earlier attempts at this issue lacked. One tried to - * relax `applyWorkspacePatch` (from inside that function a fan-in is indistinguishable from real drift) - * and one tried to consult recorded provenance (a task that throws inside the replay loop never persists - * a preparation record, so the evidence does not exist at the moment the decision is made). + * The rule under test decides where replay should START, and it needs TWO conditions. Only the first is a + * hash identity, and an earlier revision of this file claimed both were: + * + * 1. The worktree's tree equals dependency `i`'s declared `result_tree`. A tree id is a content hash, so + * the workspace is identical to that dependency's output over the snapshot the hash covers — every + * path `stageWorkspaceTree` stages. Content outside it is content no patch can carry either, since + * `captureWorkspacePatch` diffs the same staged index, so nothing patch-delivered is missed. + * 2. The dependencies BEFORE `i` chain into it, each one's `result_tree` being the next one's + * `base_tree`. Without this, "everything before `i` is already materialized" is a claim about + * TOPOLOGY, not about content — and review demonstrated it false for a sibling fan-in, on a FRESH + * run, with no error raised anywhere. That case has its own test below. + * + * Two earlier attempts at this issue were rejected for needing evidence that does not exist: relaxing + * `applyWorkspacePatch` (from inside that function a fan-in is indistinguishable from real drift) and + * consulting recorded provenance (a task that throws inside the replay loop never persists a preparation + * record, so there is nothing to consult at the moment the decision is made). */ const runtimePackageRoot = findRuntimePackageRoot(path.dirname(fileURLToPath(import.meta.url))); From 823787ae524ceaf2c3f5aebbd685233386cad270 Mon Sep 17 00:00:00 2001 From: Antonio Viggiano <3029017+aviggiano@users.noreply.github.com> Date: Fri, 7 Aug 2026 06:37:19 +0000 Subject: [PATCH 4/4] docs(runtime): scope the skipped-capture guarantee to what is actually checked Review pointed out that the validator's doc comment implied more coverage than it delivers. The checks that need only the capture and HEAD moved into `validateWorkspacePatchCapture`; `assertPatchPathsMatchManifest`, the empty-patch consistency check and the result-tree verification stayed in `applyWorkspacePatch`, because they are meaningful only against a worktree the patch is being applied to. So a SKIPPED capture is validated less thoroughly than an applied one: its manifest cannot name a sensitive path, but nothing cross-checks the paths in its patch BODY against that manifest. Acceptable, because the body is never applied -- but "sensitive-path rejection" without that qualifier reads as a guarantee the code does not make. Also records that validating every capture up front and then applying some of them double-validates those, and why that is the deliberate trade: the skip decision reads `result_tree`, so validation has to precede the decision, and re-running inside `applyWorkspacePatch` keeps that function safe for any caller. Every check is pure and idempotent. Full runtime suite: 371 tests, 356 pass, 0 fail. Co-Authored-By: Claude Opus 5 --- packages/runtime/src/workspace-handoff.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/runtime/src/workspace-handoff.ts b/packages/runtime/src/workspace-handoff.ts index 6ec27cc04..6e8ce0edd 100644 --- a/packages/runtime/src/workspace-handoff.ts +++ b/packages/runtime/src/workspace-handoff.ts @@ -184,6 +184,19 @@ export function captureWorkspacePatch(workspaceRoot: string, baselineTree: strin * sensitive-path rejection were never checked for that dependency at all — so a manifest naming `.env`, * or a `patch_sha256` that does not match its bytes, would be accepted in silence. Worse, the skip * decision itself reads `result_tree`, so an unvalidated field was steering it. + * + * These are the checks that need nothing but the capture and the repository's `HEAD`. The rest — + * `assertPatchPathsMatchManifest`, the empty-patch consistency check and the result-tree verification — + * stay in `applyWorkspacePatch` because they are meaningful only against a worktree the patch is being + * applied to. So a SKIPPED capture is validated less thoroughly than an applied one: its manifest cannot + * name a sensitive path, but nothing cross-checks the paths in its patch BODY against that manifest. That + * is acceptable only because the body is never applied, and it is stated here so the guarantee is not + * read as broader than it is. + * + * A caller that validates every capture up front and then applies some of them will validate those twice. + * That is deliberate, not an oversight: the skip decision reads `result_tree`, so validation has to + * precede the decision, and re-running it inside `applyWorkspacePatch` keeps that function safe for any + * caller. Every check here is pure and idempotent; the cost is one extra digest and one extra `rev-parse`. */ export function validateWorkspacePatchCapture(workspaceRoot: string, capture: WorkspacePatchCapture): void { validateManifest(capture.manifest);