From 5d1cffab7699d1a15c1408b1f03608626ed2dc59 Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 21 Jul 2026 10:09:41 +0200 Subject: [PATCH] fix(implement-loop): make resume survive plan edits and interim subset commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two interacting defects broke the documented PARTIAL repair flow (fix the plan, commit the known-good subset, rm BLOCKED.md, resume) — observed on run wf_81a13333-be3 (aura): the resume replayed all 78 agents from cache in 33 ms and returned the byte-identical stale end-report. 1. Plan content was invisible to the resume cache key. The substrate caches per (prompt, opts) and every plan-reading prompt carried only the plan PATH, so a plan edit changed no prompt and the extraction replayed stale text. New required standard-mode carrier field plan_sha256 (recomputed by the orchestrator at every invocation) is stamped into all three plan-reading prompts (plan-index, plan-extract-all, plan-extract:): a plan edit re-runs the extraction, and a downstream task replays from cache iff its re-extracted text is byte-identical — worst case a redundant live re-run, never stale text. Alternative considered: documenting "plan edits need a fresh run with task_range" — rejected, it keeps the trap armed and forfeits the cache replay a resume exists for. 2. The cross-task discard guard compared boundary path sets across a moved HEAD. Boundary sets are diffs AGAINST HEAD; after the sanctioned interim subset commit, the first live boundary measures against the new HEAD while the replayed boundary carries the old one — the entire committed subset read as "lost paths" and hard-BLOCKed. The snapshot agent now also reports `git rev-parse HEAD`; a boundary pair straddling a HEAD move resets the guard with a concern (which also flags a rogue mid-run commit) instead of comparing. Same-HEAD discards still hard-BLOCK; a blank sha falls through to the comparison so a flaky report cannot disarm the guard. Verified with a zero-token mock harness (AsyncFunction + stubbed agent()): fail-fast on missing plan_sha256, hash present in all three extraction prompts (batched and fan-out paths), same-HEAD discard still BLOCKs, HEAD-move resets with a concern, mini mode unaffected. Adversarially reviewed (4-verifier workflow): SOUND. Known residuals: the script cannot verify the hash matches the plan bytes (operator obligation, documented in the carrier contract and SKILL.md), and a coincident HEAD-move + discard surfaces as a concern rather than a BLOCK. closes #32 --- implement/SKILL.md | 29 ++++++- implement/workflows/implement-loop.js | 115 +++++++++++++++++++++----- 2 files changed, 121 insertions(+), 23 deletions(-) diff --git a/implement/SKILL.md b/implement/SKILL.md index 41d93e1..f682093 100644 --- a/implement/SKILL.md +++ b/implement/SKILL.md @@ -200,7 +200,13 @@ verdict means *adjudicate against the snapshot*, not proven malice. Renames do not trip it (the boundary diff runs `--no-renames`, so a renamed file's old path stays listed as a deletion). Untracked files sit outside the threat model (`git checkout -- ` cannot discard -them). Cost: one extra sonnet/medium snapshot call per task of a +them). Boundaries are HEAD-aware (issue #32): each snapshot records +the HEAD its diff ran against, and a boundary pair straddling a HEAD +move — the sanctioned commit-the-subset-then-resume repair — resets +the guard with a concern instead of comparing, so a replayed +pre-commit boundary never counts the committed subset as lost paths; +in a single live run the same concern flags an agent that moved HEAD +mid-run. Cost: one extra sonnet/medium snapshot call per task of a multi-task run; single-task and mini runs skip the guard. The detection details live in `workflows/implement-loop.js`, the single source. @@ -220,6 +226,7 @@ Workflow({ name: "implement-loop", args: { mode: "standard", iter_id: "", plan_path: "", + plan_sha256: "", // recompute at EVERY (re-)invocation (issue #32) task_range: [3, 8], // optional repo_root: "" // strongly recommended; effectively required in worktree sessions (issue #25) }}) @@ -238,6 +245,14 @@ Workflow({ name: "implement-loop", args: { }}) ``` +`plan_sha256` content-addresses the plan (issue #32): a resume replays +agents cached per (prompt, opts), and with only the plan *path* in the +extraction prompts an edit-and-resume would replay the stale pre-edit +task text — the repair silently no-ops. Compute the hash fresh +(`sha256sum `) at every invocation, including a +`resumeFromRunId` re-invocation after a plan repair; never reuse the +old value. + `repo_root` is stamped into every stage prompt as a `git -C` anchor: in worktree sessions a dispatched agent's cwd can resolve to the primary checkout and review the wrong tree (false `infra_blocked` @@ -332,7 +347,17 @@ orchestrator decides what to do with the dirty working tree: before re-running the workflow — its preflight clean-tree check counts the file as dirt. Either stash everything and re-run on a clean tree, or commit the known-good subset, then `rm BLOCKED.md`, - then re-run. + then re-run. The re-run is either a fresh invocation with + `task_range` covering the remaining tasks, or a resume + (`resumeFromRunId` on the same script) — for a resume, recompute + `plan_sha256` from the edited plan so the extraction prompts + change: the repaired task runs live, and a completed task replays + from cache when its re-extracted text is byte-identical (the + usual case; worst case it re-runs redundantly, never stale — + issue #32). The discard guard + tolerates the interim subset commit: boundaries are HEAD-aware, + so the moved baseline resets the guard with a concern instead of + a false lost-paths `BLOCKED`. - **Discard:** `git checkout -- .` to drop unstaged file changes; `git clean -fd` / `rm BLOCKED.md` plus anything else new. main HEAD does NOT move. diff --git a/implement/workflows/implement-loop.js b/implement/workflows/implement-loop.js index 0360dda..d4dea6b 100644 --- a/implement/workflows/implement-loop.js +++ b/implement/workflows/implement-loop.js @@ -73,7 +73,18 @@ // `stash create` (a bare `git stash` would move the tree; the following // boundary would catch that wipe, but only after the fact). Untracked // files sit outside the threat model (`git checkout -- ` cannot -// discard them). +// discard them). Boundaries are HEAD-aware (issue #32): each snapshot +// records the HEAD its diff ran against, and a boundary pair straddling +// a HEAD move — the sanctioned commit-the-subset-then-resume repair — is +// reset-and-surfaced as a concern instead of compared, so a replayed +// pre-commit boundary can never count the committed subset as lost paths. +// • Resume content-addressing (issue #32): the carrier's plan_sha256 rides +// every plan-reading prompt, so the documented PARTIAL repair (edit the +// plan, commit the good subset, resume) re-runs the extraction instead +// of replaying the stale pre-edit plan from cache. Downstream, a task +// replays iff its re-extracted text is byte-identical — typically every +// unchanged task; the failure mode is a redundant live re-run, never +// stale text. // • The script has no filesystem or shell access of its own — every // working-tree mutation (code, tests) happens inside an agent() call; // the stats.json / BLOCKED.md contents are built in-script and ride @@ -99,6 +110,20 @@ // mode: 'standard' | 'mini' // iter_id: e.g. 'ct.2.3' (standard) or 'bugfix-' / 'feat-' (mini) // plan_path: (standard) path under docs/plans +// plan_sha256: (standard) sha256 hex of the plan file's CURRENT bytes +// (`sha256sum `). Stamped into every plan-reading +// prompt as a content-address (issue #32): a resume replays +// agents cached per (prompt, opts), and a prompt that +// carries only the plan PATH replays a stale extraction +// after a plan edit — the documented edit-and-resume repair +// silently no-ops. With the bytes hashed into the prompt, a +// plan edit re-runs the extraction; downstream, a task +// replays from cache iff its re-extracted text is +// byte-identical — typically every unchanged task, worst +// case a redundant live re-run, never stale text. An +// unchanged plan replays fully from cache. Recompute it at +// EVERY (re-)invocation — a stale or constant value +// re-opens the no-op resume. // task_range: (standard, optional) [from, to] inclusive, 1-based // red_test_path: (mini) absolute path to the RED test from debug / tdd // cause_summary: (mini) 1–2 sentences (debugger cause, or tdd spec_summary) @@ -160,6 +185,7 @@ const { mode = 'standard', iter_id, plan_path, + plan_sha256, task_range = null, red_test_path, cause_summary, @@ -180,7 +206,7 @@ if (mode !== 'standard' && mode !== 'mini') { // The required carrier fields per mode (the header comment above is the // contract). Missing or blank fields fail fast by name — the loop must never // glob for a plan or brief an implementer with the literal string "undefined". -const requiredByMode = mode === 'mini' ? { iter_id, red_test_path, cause_summary } : { iter_id, plan_path } +const requiredByMode = mode === 'mini' ? { iter_id, red_test_path, cause_summary } : { iter_id, plan_path, plan_sha256 } const missingFields = Object.entries(requiredByMode) .filter(([, v]) => typeof v !== 'string' || !v.trim()) .map(([k]) => k) @@ -218,6 +244,21 @@ const STANDING_NONE = 'Schema-bound mechanical stage: NO standing reading (no CLAUDE.md, no git log) — do exactly what is instructed ' + 'below, nothing else.' +// Plan content-address (issue #32): appended to EVERY prompt that reads the +// plan file (plan-index, plan-extract-all, plan-extract:). The resume +// cache keys on (prompt, opts); with only the plan PATH in the prompt, an +// edit-and-resume replays the pre-edit task text from cache and the repair +// silently no-ops. Hashing the plan's bytes into each reading prompt makes a +// plan edit re-run the extraction — and, because the extracted text flows +// verbatim into the downstream per-task prompts, a task replays from cache +// iff its re-extracted text is byte-identical (typically every unchanged +// task; the failure mode is a redundant live re-run, never stale text). +const PLAN_ADDRESS = + mode === 'standard' + ? `\n\nPlan content-address: sha256 ${plan_sha256} — binds this extraction to the plan's current bytes for ` + + 'resume caching; informational only, nothing to verify or act on.' + : '' + // Working-directory anchor (issue #25): in worktree sessions a dispatched // agent's cwd can resolve to the PRIMARY checkout instead of the worktree the // loop operates on — observed as a per-dispatch race producing false @@ -508,7 +549,7 @@ const STD_VERIFY_SCHEMA = { const SNAP_SCHEMA = { type: 'object', additionalProperties: false, - required: ['snapshot_sha', 'modified_paths'], + required: ['snapshot_sha', 'modified_paths', 'head_sha'], properties: { snapshot_sha: { type: 'string', @@ -524,6 +565,12 @@ const SNAP_SCHEMA = { '`--no-renames` keeps a renamed file\'s old path listed as a deletion; untracked files are invisible ' + 'here by design)', }, + head_sha: { + type: 'string', + description: + 'output of `git rev-parse HEAD` — the baseline modified_paths was diffed against; the guard compares two ' + + 'boundaries only when their baselines match (issue #32)', + }, }, } // ---- Phase 0 — clean-tree preflight (mini mode only) -------------------- @@ -614,7 +661,7 @@ if (mode === 'mini') { : 'ids of EVERY task') + ' that the plan actually defines, each with its one-line title. Return ONLY ids and titles — do NOT include ' + 'any task body or code block. Do not summarise, invent, reorder, or pad the list. Separately report ' + - '`total_tasks`: the count of ALL tasks the entire plan defines, ignoring any range filter. Do not edit anything.', + `\`total_tasks\`: the count of ALL tasks the entire plan defines, ignoring any range filter. Do not edit anything.${PLAN_ADDRESS}`, { model: 'sonnet', effort: 'medium', label: 'plan-index', phase: 'Per-task loop', schema: TASK_INDEX_SCHEMA }, ) if (!index) { @@ -728,7 +775,7 @@ if (mode === 'mini') { const all = await agent( `${STANDING_NONE}${ANCHOR}\n\nRead the plan at ${plan_path}. Extract EXACTLY these tasks (1-based ids): ${expectedIds.join(', ')} — ` + 'EACH as its VERBATIM block: the full task text as written, including any code blocks. Do not summarise, ' + - 'truncate, reorder, or merge tasks into each other. Return exactly those tasks, nothing else. Do not edit anything.', + `truncate, reorder, or merge tasks into each other. Return exactly those tasks, nothing else. Do not edit anything.${PLAN_ADDRESS}`, { model: 'sonnet', effort: 'medium', label: 'plan-extract-all', phase: 'Per-task loop', schema: TASK_TEXTS_SCHEMA }, ) const returned = all && Array.isArray(all.tasks) ? all.tasks : [] @@ -746,7 +793,7 @@ if (mode === 'mini') { agent( `${STANDING_NONE}${ANCHOR}\n\nRead the plan at ${plan_path}. Extract EXACTLY task ${id} (1-based) as its VERBATIM block — ` + 'the full task text as written, including any code blocks. Do not summarise, truncate, reorder, or merge in ' + - 'any other task. Return that one task only. Do not edit anything.', + `any other task. Return that one task only. Do not edit anything.${PLAN_ADDRESS}`, { model: 'sonnet', effort: 'medium', label: `plan-extract:${id}`, phase: 'Per-task loop', schema: TASK_TEXT_SCHEMA }, ).then((t) => ({ reqId: id, t })), ), @@ -1011,37 +1058,63 @@ for (const task of tasks) { results.push(r) if (tasks.length > 1) { const snap = await agent( - `${STANDING_NONE}${ANCHOR}\n\nRecord a working-tree snapshot boundary WITHOUT changing anything. Run exactly these two ` + + `${STANDING_NONE}${ANCHOR}\n\nRecord a working-tree snapshot boundary WITHOUT changing anything. Run exactly these three ` + 'commands and report their output:\n' + `1. \`git stash create "iter ${iter_id} post-task ${task.id}"\` — prints a sha (a dangling commit; it ` + 'touches neither HEAD, nor the index, nor the working tree). Report it as snapshot_sha; empty string if ' + 'it printed nothing.\n' + '2. `git diff HEAD --name-only --no-renames` — report every printed path as modified_paths.\n' + + '3. `git rev-parse HEAD` — report it as head_sha.\n' + 'NEVER run bare `git stash` or `git stash push/pop/apply/drop` — those MOVE the changes. Do NOT edit, ' + 'stage, or commit anything.', { model: 'sonnet', effort: 'medium', label: `snapshot:${task.id}`, phase: 'Per-task loop', schema: SNAP_SCHEMA }, ) if (snap && prevBoundary) { - const now = new Set(snap.modified_paths || []) - const lost = [...prevBoundary.paths].filter((p) => !now.has(p)) - if (lost.length) { - // A path that was HEAD-modified at the previous boundary is - // HEAD-identical now: a file-level checkout/restore discarded - // sibling-task work mid-run. Hard stop — the per-task DONE reports - // and the tree have diverged, so nothing downstream is trustworthy. - discard = { - lost_paths: lost, - modified_through_task: prevBoundary.taskId, - discarded_during_task: task.id, - recovery_snapshot: prevBoundary.sha || null, + // HEAD-aware baseline (issue #32): each boundary's path set is a diff + // AGAINST HEAD, so two boundaries are comparable only when they were + // measured against the SAME HEAD. The sanctioned PARTIAL repair + // (commit the known-good subset, then resume) moves HEAD between the + // replayed boundaries and the fresh ones — comparing across that move + // would report the entire committed subset as "lost" and hard-BLOCK + // the first live task. On a baseline move the guard resets instead of + // comparing, and surfaces the move: the loop itself never commits, so + // a HEAD move inside a single live run means some agent committed + // mid-run — worth eyes either way. A blank/missing sha falls through + // to the comparison — a flaky report must not quietly disarm the guard. + const prevHead = typeof prevBoundary.head === 'string' ? prevBoundary.head.trim() : '' + const nowHead = typeof snap.head_sha === 'string' ? snap.head_sha.trim() : '' + if (prevHead && nowHead && prevHead !== nowHead) { + guardConcerns.push( + `discard-guard baseline moved between task #${prevBoundary.taskId} and task #${task.id} ` + + `(HEAD ${prevHead} -> ${nowHead}) — expected only on a resume after the orchestrator committed a ` + + 'known-good subset (the sanctioned PARTIAL repair); in a single live run with no interim commit it ' + + 'means an agent moved HEAD mid-run — investigate. The guard reset its baseline; this boundary pair ' + + 'went uncompared.', + ) + } else { + const now = new Set(snap.modified_paths || []) + const lost = [...prevBoundary.paths].filter((p) => !now.has(p)) + if (lost.length) { + // A path that was HEAD-modified at the previous boundary is + // HEAD-identical now: a file-level checkout/restore discarded + // sibling-task work mid-run. Hard stop — the per-task DONE reports + // and the tree have diverged, so nothing downstream is trustworthy. + discard = { + lost_paths: lost, + modified_through_task: prevBoundary.taskId, + discarded_during_task: task.id, + recovery_snapshot: prevBoundary.sha || null, + } + break } - break } } if (!snap) guardConcerns.push(`discard-guard snapshot after task #${task.id} did not return — that boundary went unchecked`) // A non-returning snapshot agent skips the NEXT comparison (prevBoundary // null) rather than false-positiving against a stale boundary. - prevBoundary = snap ? { taskId: task.id, sha: snap.snapshot_sha || null, paths: new Set(snap.modified_paths || []) } : null + prevBoundary = snap + ? { taskId: task.id, sha: snap.snapshot_sha || null, paths: new Set(snap.modified_paths || []), head: snap.head_sha || null } + : null } if (r.outcome === 'BLOCKED') break // do not skip ahead — task ordering dependencies are unknown }