diff --git a/docs/agent-template.md b/docs/agent-template.md index 25fb355..4711087 100644 --- a/docs/agent-template.md +++ b/docs/agent-template.md @@ -159,8 +159,8 @@ pin exists to remove. Effort follows the model split: Workflow scripts pin a third tier inline: schema-bound extraction/verification stages that author no code (preflight, -plan-index, plan-extract, snapshot, mini-verify, tree-check, finalize, -build/suite verify) run `medium` via the `agent()` call's `effort:` option. As with +plan-index, plan-extract[-all], snapshot, mini-verify, the standard +end-verify, build/suite verify) run `medium` via the `agent()` call's `effort:` option. As with `model:`, every `agent()` call passes `effort:` explicitly — whether frontmatter effort propagates through an `agentType` dispatch is undocumented, so the scripts do not rely on it (see the policy header diff --git a/implement/SKILL.md b/implement/SKILL.md index 0576ec5..1cd8674 100644 --- a/implement/SKILL.md +++ b/implement/SKILL.md @@ -210,7 +210,8 @@ Workflow({ name: "implement-loop", args: { mode: "standard", iter_id: "", plan_path: "", - task_range: [3, 8] // optional + task_range: [3, 8], // optional + repo_root: "" // strongly recommended; effectively required in worktree sessions (issue #25) }}) ``` @@ -222,10 +223,19 @@ Workflow({ name: "implement-loop", args: { iter_id: "bugfix-", // tdd: "feat-" red_test_path: "", cause_summary: "<1-2 sentences from debugger>", // tdd: the desired-behaviour line - constraint: "minimal fix, no surrounding cleanup" // tdd: "minimal feature, no surrounding scope" + constraint: "minimal fix, no surrounding cleanup", // tdd: "minimal feature, no surrounding scope" + repo_root: "" // strongly recommended; effectively required in worktree sessions (issue #25) }}) ``` +`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` +quality reviews, a false no-op `BLOCKED` from the end-verify — issue +#25). Without the field the first dispatched stage probes +`git rev-parse --show-toplevel` as a fallback, which is subject to the +same race — pass the field. + The carrier fields are defined authoritatively at the top of `workflows/implement-loop.js`. The load-bearing `iter_id` rule is there too: it names the scratch dir and the stats filename, NOT a branch diff --git a/implement/workflows/compiler-driven-edit.js b/implement/workflows/compiler-driven-edit.js index 8a1603d..07f6568 100644 --- a/implement/workflows/compiler-driven-edit.js +++ b/implement/workflows/compiler-driven-edit.js @@ -50,6 +50,12 @@ // Carrier — Workflow `args`: // edit_description: what type/signature change to make // def_site: the definition site (file + symbol) it originates from +// repo_root: optional but strongly recommended (required in worktree +// sessions — issue #25): absolute path of the working tree +// this edit operates on, stamped into both stage prompts +// as a `git -C` anchor so a dispatched agent whose cwd +// resolves to the primary checkout cannot verify the +// wrong tree. // A non-object carrier or a missing required field fails fast as an infra // BLOCKED before any agent is dispatched (issue #24). @@ -87,7 +93,7 @@ if ((carrier !== undefined && carrier !== null && typeof carrier !== 'object') | } } -const { edit_description, def_site } = carrier || {} +const { edit_description, def_site, repo_root } = carrier || {} const missingFields = Object.entries({ edit_description, def_site }) .filter(([, v]) => typeof v !== 'string' || !v.trim()) @@ -107,6 +113,18 @@ const STANDING = '(its "## Skills plugin: project facts" name the build and test commands) and ' + '`git log -10 --format=full`. Resolve the build/test commands from those facts.' +// Working-directory anchor (issue #25): same defect class as implement-loop — +// in a worktree session a dispatched agent's cwd can resolve to the primary +// checkout and verify the wrong (clean) tree. Carrier-supplied only here (two +// stages, no cheap probe slot); '' when absent. +const ANCHOR = + typeof repo_root === 'string' && repo_root.trim() + ? `\n\nWORKING-DIRECTORY ANCHOR (do not skip): the working tree lives at ${repo_root.trim()}. Run EVERY git ` + + `command as \`git -C ${repo_root.trim()} ...\` and run the build/test commands from that directory. An empty ` + + '`git diff HEAD` or an unexpected HEAD sha means you are in the WRONG directory — re-run against the anchor; ' + + 'never report a clean tree from an unanchored cwd.' + : '' + const EDIT_SCHEMA = { type: 'object', additionalProperties: false, @@ -159,7 +177,7 @@ const VERIFY_SCHEMA = { // ---- Phase 1 — make the edit and let the compiler enumerate the sites ---- phase('Edit + propagate') const edit = await agent( - `${STANDING}\n\nMake this behaviour-preserving type/signature edit at its definition site, then propagate it ` + + `${STANDING}${ANCHOR}\n\nMake this behaviour-preserving type/signature edit at its definition site, then propagate it ` + 'MECHANICALLY across every site the build flags — let the type checker enumerate the edit sites. Introduce NO ' + 'new behaviour, add NO new test, change NO existing test. If a flagged site cannot be resolved without making a ' + 'design decision, OR resolving it would encode new behaviour, STOP and report it as a hole rather than guessing. ' + @@ -207,7 +225,7 @@ if (!edit.applied_changes) { // ---- Phase 2 — verify (the independently-invokable phase) --------------- phase('Verify') const verify = await agent( - `${STANDING}\n\nVerify the edit is behaviour-preserving WITHOUT changing any code or test. Run the project build ` + + `${STANDING}${ANCHOR}\n\nVerify the edit is behaviour-preserving WITHOUT changing any code or test. Run the project build ` + 'and the full test suite. Report whether the build is clean and whether the suite is green AND UNCHANGED — i.e. ' + 'every test passes and NO test was added, removed, or weakened versus the pre-edit baseline (compare `git diff ' + 'HEAD` for test-file changes). Also report working_tree_dirty — whether `git status --porcelain` / `git diff HEAD` ' + diff --git a/implement/workflows/implement-loop.js b/implement/workflows/implement-loop.js index b1b826a..9c43aac 100644 --- a/implement/workflows/implement-loop.js +++ b/implement/workflows/implement-loop.js @@ -86,8 +86,8 @@ // • Effort policy mirrors the model policy: every agent() call pins an // explicit effort — never the session inherit. Code-writing and review // steps run high, the opus quality gate runs xhigh, and schema-bound -// extraction/check steps (preflight, plan-index, plan-extract, snapshot, -// mini-verify, tree-check) run medium. See +// extraction/check steps (preflight, plan-index, plan-extract[-all], +// snapshot, mini-verify, verify) run medium. See // docs/agent-template.md § effort. // // Carrier — passed as the Workflow `args` object by the orchestrator: @@ -98,6 +98,17 @@ // 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) // constraint: (mini) e.g. 'minimal fix, no surrounding cleanup' +// repo_root: (both modes; optional but STRONGLY recommended, and +// effectively required in worktree sessions — issue #25) +// absolute path of the working tree this loop operates on. +// Stamped into every stage prompt as a `git -C` anchor so a +// dispatched agent whose cwd resolves to the PRIMARY checkout +// (the observed per-dispatch race) cannot review the wrong +// tree — false infra_blocked quality reviews, a false no-op +// BLOCKED from the end-verify. When absent, the first +// dispatched stage probes `git rev-parse --show-toplevel` as +// a fallback (itself subject to the same race the carrier +// field exists to remove — pass the field). // A non-object carrier, an unknown mode, or a missing required field fails // fast as an infra BLOCKED before any agent is dispatched (issue #24). @@ -148,6 +159,7 @@ const { red_test_path, cause_summary, constraint = 'minimal change, no surrounding cleanup', + repo_root, } = carrier || {} // Blank-safe echo for the guard verdicts below: never mirror a blank/non-string // iter_id into the end-report while also naming it missing. @@ -183,15 +195,35 @@ const STANDING = 'design ledger) and `git log -10 --format=full`. Resolve the build/test commands ' + 'from those facts — do not assume a toolchain.' +// 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 +// infra_blocked quality reviews (empty `git diff HEAD`, the sha256-of-empty +// fingerprint) and a false no-op BLOCKED from the end-verify counting the +// wrong (clean) tree. The remedy validated on issue #25: stamp the loop's +// repo root into every stage prompt as a `git -C` instruction. The root comes +// from the carrier (deterministic — preferred) or, as a fallback, from the +// first dispatched stage's `git rev-parse --show-toplevel` probe. +const anchorText = (root) => + root + ? `\n\nWORKING-DIRECTORY ANCHOR (do not skip): the working tree under review lives at ${root}. Run EVERY git ` + + `command as \`git -C ${root} ...\` and run the build/test commands from that directory. An empty ` + + '`git diff HEAD`, an unexpected HEAD sha, or a clean tree right after edits were reported means you are in ' + + 'the WRONG directory — re-run against the anchor; never report a clean tree from an unanchored cwd.' + : '' +const carrierRoot = typeof repo_root === 'string' && repo_root.trim() ? repo_root.trim() : '' +let ANCHOR = anchorText(carrierRoot) + // JSON Schemas — force structured verdicts so the script can branch on real outcomes. const PREFLIGHT_SCHEMA = { type: 'object', additionalProperties: false, - required: ['clean', 'head_sha', 'branch'], + required: ['clean', 'head_sha', 'branch', 'repo_root'], properties: { clean: { type: 'boolean', description: '`git status --porcelain` produced no output' }, head_sha: { type: 'string' }, branch: { type: 'string' }, + repo_root: { type: 'string', description: 'output of `git rev-parse --show-toplevel` (the anchor fallback, issue #25)' }, dirty_paths: { type: 'array', items: { type: 'string' } }, }, } @@ -217,6 +249,7 @@ const TASK_INDEX_SCHEMA = { clean: { type: 'boolean', description: '`git status --porcelain` produced no output' }, head_sha: { type: 'string' }, branch: { type: 'string' }, + repo_root: { type: 'string', description: 'output of `git rev-parse --show-toplevel` (the anchor fallback, issue #25)' }, dirty_paths: { type: 'array', items: { type: 'string' } }, total_tasks: { type: 'integer', @@ -464,9 +497,11 @@ let startSha = null if (mode === 'mini') { phase('Preflight') const pre = await agent( - `${STANDING}\n\nYou are the clean-tree preflight for an implement iteration. ` + - 'Run `git status --porcelain`, `git rev-parse HEAD`, and `git rev-parse --abbrev-ref HEAD`. ' + - 'Report whether the tree is clean (no porcelain output), the HEAD sha, the branch, and any dirty paths. ' + + `${STANDING}${ANCHOR}\n\nYou are the clean-tree preflight for an implement iteration. ` + + 'Run `git status --porcelain`, `git rev-parse HEAD`, `git rev-parse --abbrev-ref HEAD`, and ' + + '`git rev-parse --show-toplevel`. ' + + 'Report whether the tree is clean (no porcelain output), the HEAD sha, the branch, the repo_root, and any ' + + 'dirty paths. ' + 'Do NOT modify anything, do NOT commit. The HEAD sha is an informational anchor only — never a reset target.', { model: 'sonnet', effort: 'medium', label: 'preflight', phase: 'Preflight', schema: PREFLIGHT_SCHEMA }, ) @@ -480,6 +515,7 @@ if (mode === 'mini') { } } startSha = pre.head_sha + if (!carrierRoot && typeof pre.repo_root === 'string' && pre.repo_root.trim()) ANCHOR = anchorText(pre.repo_root.trim()) } // ---- Phase 1 — assemble the task list ---------------------------------- @@ -529,8 +565,9 @@ if (mode === 'mini') { // narrower than the closed across-tasks vector and rests on the planner's // bite-sized-task invariant; it is the one truncation slice left open here. const index = await agent( - `${STANDING}\n\nFIRST the folded clean-tree preflight: run \`git status --porcelain\`, \`git rev-parse HEAD\`, ` + - 'and `git rev-parse --abbrev-ref HEAD`; report `clean` (no porcelain output), `head_sha`, `branch`, and any ' + + `${STANDING}${ANCHOR}\n\nFIRST the folded clean-tree preflight: run \`git status --porcelain\`, \`git rev-parse HEAD\`, ` + + '`git rev-parse --abbrev-ref HEAD`, and `git rev-parse --show-toplevel`; report `clean` (no porcelain output), ' + + '`head_sha`, `branch`, `repo_root`, and any ' + '`dirty_paths`. The HEAD sha is an informational anchor only — never a reset target. If the tree is DIRTY, ' + 'stop there: return task_ids: [] and total_tasks: 0 without reading the plan.\n\n' + `Otherwise read the plan at ${plan_path}. List the task ` + @@ -555,6 +592,7 @@ if (mode === 'mini') { } } startSha = index.head_sha + if (!carrierRoot && typeof index.repo_root === 'string' && index.repo_root.trim()) ANCHOR = anchorText(index.repo_root.trim()) if (!index.task_ids || index.task_ids.length === 0) { return { status: 'NEEDS_CONTEXT', iter_id, reason: `no tasks found in ${plan_path}` } } @@ -650,7 +688,7 @@ if (mode === 'mini') { let extractedTasks = null if (expectedIds.length > 1 && totalApprox !== null && totalApprox <= EXTRACT_ALL_BYTE_CEILING) { const all = await agent( - `${STANDING}\n\nRead the plan at ${plan_path}. Extract EXACTLY these tasks (1-based ids): ${expectedIds.join(', ')} — ` + + `${STANDING}${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.', { model: 'sonnet', effort: 'medium', label: 'plan-extract-all', phase: 'Per-task loop', schema: TASK_TEXTS_SCHEMA }, @@ -668,7 +706,7 @@ if (mode === 'mini') { extractedTasks = await parallel( expectedIds.map((id) => () => agent( - `${STANDING}\n\nRead the plan at ${plan_path}. Extract EXACTLY task ${id} (1-based) as its VERBATIM block — ` + + `${STANDING}${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.', { model: 'sonnet', effort: 'medium', label: `plan-extract:${id}`, phase: 'Per-task loop', schema: TASK_TEXT_SCHEMA }, @@ -712,7 +750,7 @@ phase('Per-task loop') async function runTask(task) { // 2.1 — implementer phase const impl = await agent( - `${STANDING}\n\nImplement this task exactly, RED-first if it adds behaviour (TDD is an independent ` + + `${STANDING}${ANCHOR}\n\nImplement this task exactly, RED-first if it adds behaviour (TDD is an independent ` + 'inner-loop discipline — add the failing test inline even if the task text forgot to script it). ' + 'Build and test must be green before you report DONE. Leave all edits UNSTAGED; never commit.\n\n' + `mode: ${mode}\niter_id: ${iter_id}\n\nTASK ${task.id} — ${task.title}\n${task.text}`, @@ -732,7 +770,7 @@ async function runTask(task) { // 2.2 — spec-compliance check (gated before quality), with repair re-loop for (let round = 0; ; round++) { const spec = await agent( - `${STANDING}\n\nSpec-compliance review of task ${task.id}. Compare \`git diff HEAD\` against the task ` + + `${STANDING}${ANCHOR}\n\nSpec-compliance review of task ${task.id}. Compare \`git diff HEAD\` against the task ` + 'text ONLY — list missing requirements and unrequested extras. No quality opinions, no fix proposals. ' + 'If a requirement can only be judged missing because its scripted literal is contradicted by reality ' + 'you have verified yourself (the actual behaviour provably differs from what the task text asserts), or ' + @@ -746,7 +784,7 @@ async function runTask(task) { if (round >= 2) return { id: task.id, title: task.title, outcome: 'BLOCKED', reason: 'review-loop-exhausted (spec-compliance)' } // repair: re-dispatch the implementer with the review findings as the repair brief const specFix = await agent( - `${STANDING}\n\nRepair task ${task.id} to satisfy the spec-compliance review. Address EACH finding; ` + + `${STANDING}${ANCHOR}\n\nRepair task ${task.id} to satisfy the spec-compliance review. Address EACH finding; ` + 'do not widen scope. Keep build + tests green; leave edits unstaged.\n\n' + `TASK ${task.id} — ${task.title}\n${task.text}\n\nFINDINGS:\n- ${(spec?.findings || ['(none reported)']).join('\n- ')}`, { agentType: 'implementer', model: 'sonnet', effort: 'high', label: `impl-fix-spec:${task.id}`, phase: 'Per-task loop', schema: IMPL_SCHEMA }, @@ -767,7 +805,7 @@ async function runTask(task) { const seenFingerprints = new Set() for (let round = 0; ; round++) { const qual = await agent( - `${STANDING}\n\nQuality review of task ${task.id} (spec-compliance is already green — do not re-check it). ` + + `${STANDING}${ANCHOR}\n\nQuality review of task ${task.id} (spec-compliance is already green — do not re-check it). ` + 'Read every chunk of `git diff HEAD` in this task\'s footprint. Report only Important and Minor issues, ' + 'each with its `severity` classified honestly per your severity definitions; Nits never gate. ' + 'Also report `diff_fingerprint` (REQUIRED): the sha256 hex of the FULL `git diff HEAD` ' + @@ -828,7 +866,7 @@ async function runTask(task) { if (round >= 2) return { id: task.id, title: task.title, outcome: 'BLOCKED', reason: 'review-loop-exhausted (quality)' } if (qual && qual.diff_fingerprint) seenFingerprints.add(qual.diff_fingerprint) const fix = await agent( - `${STANDING}\n\nThe quality review of task ${task.id} requested changes. Address EACH Important/Minor issue; ` + + `${STANDING}${ANCHOR}\n\nThe quality review of task ${task.id} requested changes. Address EACH Important/Minor issue; ` + 'do not widen scope; keep build + tests green; leave edits unstaged. The task text below is the ratified plan.\n\n' + 'HOLD CLAUSE — most issues you simply fix. But if honouring an issue would force you to deviate from a ' + 'name, signature, or structure THIS TASK prescribes (or to break the downstream callers the plan ' + @@ -901,7 +939,7 @@ for (const task of tasks) { results.push(r) if (tasks.length > 1) { const snap = await agent( - `${STANDING}\n\nRecord a working-tree snapshot boundary WITHOUT changing anything. Run exactly these two ` + + `${STANDING}${ANCHOR}\n\nRecord a working-tree snapshot boundary WITHOUT changing anything. Run exactly these two ` + '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 ' + @@ -986,7 +1024,7 @@ let treeState = null if (mode === 'mini' && outcome === 'DONE') { phase('Verify') miniVerify = await agent( - `${STANDING}\n\nVerify a bug/feature fix WITHOUT changing any code or test. The fix was meant to drive this ` + + `${STANDING}${ANCHOR}\n\nVerify a bug/feature fix WITHOUT changing any code or test. The fix was meant to drive this ` + `RED test to GREEN: ${red_test_path}\n\nRun that test by name and confirm it now PASSES (it was RED before the ` + 'fix). Then run the full suite and confirm it is green (no regression). Also report working_tree_dirty — ' + 'whether `git status --porcelain` (NOT `git diff HEAD`, which misses new untracked files) shows any change at ' + @@ -1014,7 +1052,7 @@ if (mode === 'mini' && outcome === 'DONE') { } else if (mode === 'standard' && outcome === 'DONE') { phase('Verify') treeState = await agent( - `${STANDING}\n\nEnd-verify an implement iteration WITHOUT changing any code or test. Two independent checks:\n` + + `${STANDING}${ANCHOR}\n\nEnd-verify an implement iteration WITHOUT changing any code or test. Two independent checks:\n` + '1. Run `git status --porcelain` and report files_touched = its line count (every changed, added, or ' + 'untracked path). Use `git status --porcelain`, NOT `git diff`, so brand-new untracked files are counted.\n' + '2. Run the FULL project suite (the test command from the CLAUDE.md project facts) and report suite_green — ' + @@ -1048,7 +1086,7 @@ let e2e = null if (mode === 'standard' && outcome === 'DONE') { phase('E2E coverage') e2e = await agent( - `${STANDING}\n\nThe iteration completed. Identify 1–3 invariants worth protecting and write the smallest ` + + `${STANDING}${ANCHOR}\n\nThe iteration completed. Identify 1–3 invariants worth protecting and write the smallest ` + 'E2E fixtures for them in the project\'s canonical fixture form; each test\'s doc comment names the property ' + 'it protects. Run the project\'s test command — all green. Leave edits unstaged; never commit.\n\n' + `iter_id: ${iter_id}\nshipped: ${completed.map((c) => `#${c.id} ${c.title}`).join('; ')}`,