fix(runtime): start dependency replay after the prefix the worktree already holds

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(),
@@ -426,6 +431,64 @@ 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. 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.
+ *
+ * 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.
+ *
+ * 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,
+  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 = 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;
+}
+
 function materializeWorkspacePatchDependencies(
   task: (typeof taskSpecs)[number],
   workspaceRoot: string,
@@ -446,7 +509,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 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.
+  const captures = dependencies.map((dependency) => {
     const patchPath = resolveRegularArtifactFile(
       dependency,
       path.join(dependency, "workspace.patch"),
@@ -465,10 +532,30 @@ function materializeWorkspacePatchDependencies(
         cause: error
       });
     }
-    const capture = {
+    return {
       patch: readFileSync(patchPath, "utf8"),
       manifest: manifest as Parameters<typeof applyWorkspacePatch>[1]["manifest"]
     };
+  });
+  // 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
       // exact dependency result tree is absent and the clean base tree is

packages/runtime/src/workspace-handoff.ts

@@ -175,18 +175,52 @@ 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.
+ *
+ * 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);
   if (Buffer.byteLength(capture.patch, "utf8") > MAX_PATCH_BYTES) {
     throw new Error(`workspace patch exceeds ${MAX_PATCH_BYTES} bytes`);
   }
   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 +233,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);

packages/runtime/test/generated-workflow-verifier.test.ts

@@ -1642,6 +1642,22 @@ 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, /replayWorkspacePatches && captures\.length > 0/u);
+  assert.match(helper, /firstDependencyRequiringReplay\(\s*captureWorkspaceTree\(workspaceRoot\),/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,
     /const result = await agent\.generate\(args\);[\s\S]*?prepareArtifactMirror\(task, \{ replayWorkspacePatches: false \}\);/u