diff --git a/implement/SKILL.md b/implement/SKILL.md index 96139e9..30162dc 100644 --- a/implement/SKILL.md +++ b/implement/SKILL.md @@ -102,7 +102,9 @@ WITHIN THIS LOOP, MAIN HEAD IS SACROSANCT — NO RESET, NO REVERT; THE LOOP ONLY PER-TASK PHASES ARE SEPARATE AGENT CALLS IN THE WORKFLOW SCRIPT — implementer, then spec-compliance, then quality. SPEC COMPLIANCE IS GATED BEFORE QUALITY. TASKS RUN SEQUENTIALLY; A BLOCKED TASK STOPS THE LOOP — NO SKIP-AHEAD (TASK ORDERING DEPENDENCIES ARE UNKNOWN). NEVER PUSH PAST `BLOCKED` BY HAND. -ON `PARTIAL` OR `BLOCKED`, THE SCRIPT WRITES `BLOCKED.md` AT THE REPO ROOT — UNCOMMITTED BY CONVENTION. ON `DONE`, NO SEPARATE FILE. +ON `PARTIAL` OR `BLOCKED`, THE SCRIPT WRITES `BLOCKED.md` AT THE REPO ROOT — UNCOMMITTED BY CONVENTION. ON `DONE`, NO SEPARATE FILE. (ONE EXCEPTION: A NO-OP ITERATION IS `BLOCKED` WITH NO `BLOCKED.md` — ITS TREE IS ALREADY CLEAN, SO THE STATUS + REASON RIDE THE END-REPORT AND THERE IS NOTHING TO CLEAN UP.) +`DONE` REQUIRES POSITIVE EVIDENCE THAT AN EDIT LANDED — A NO-OP ITERATION (`git status --porcelain` EMPTY, untracked files INCLUDED) IS NEVER `DONE`, IT IS `BLOCKED` (the issue #11 vacuous-green shape, guarded on the main path). THE VERDICT IS THE GIT TREE COUNT, NEVER A SELF-REPORT. +IN MINI MODE THE RED->GREEN IS OBSERVED, NOT ASSERTED: AN INDEPENDENT VERIFY RE-RUNS THE HANDED-OFF RED TEST AND THE SUITE; A STILL-RED TEST OR A REGRESSION IS `BLOCKED` (route back to debug, RED-first). THE COMPILER-DRIVEN ARM COMMITS ONLY ON CLEAN BUILD + SUITE GREEN UNCHANGED; ELSE IT BOUNCES (specify for a design hole, tdd for discovered new behaviour, debug for a regression). ``` @@ -120,6 +122,37 @@ BLOCKED. This logic lives as the loops in `workflows/implement-loop.js` — the single source; it is deliberately not restated as prose elsewhere, so the two cannot drift. +**Positive-evidence precondition on `DONE`** (issue #12 — the issue #11 +vacuous-green shape on the main path). `build green AND suite green` is +not enough: it is satisfiable by doing nothing. So `DONE` also requires +positive evidence that an edit landed. The verdict is taken from **git +ground truth, never a self-report** — a self-report must never be able to +*fail* a run that actually did the work: + +- **Iteration gate (standard mode)** — a dedicated `tree-check` agent runs + `git status --porcelain | wc -l` after the per-task loop but **before** + E2E and finalize. A `DONE` outcome with a zero count is a no-op iteration + → `BLOCKED`. `git status --porcelain` (not `git diff HEAD`) is used so a + brand-new **untracked** file — the implementer leaves edits unstaged — + still counts; running before E2E/finalize keeps fixtures and the + stats/`BLOCKED.md` artefacts from inflating it; and a non-returning agent + is treated as `BLOCKED` (infra), never waved through. A clean-tree no-op + writes no `BLOCKED.md` (nothing to clean up — see Step 4). +- **Mini mode gate** — an independent `mini-verify` agent re-runs the + handed-off RED test and the suite and checks the tree is dirty + (`git status --porcelain`); a still-red test, a regression, or a clean + tree is `BLOCKED`. The GREEN is observed, not taken from the + implementer's self-report. +- **Per-task `applied_changes`** (self-report, OR-ed across the initial + dispatch and every repair) does NOT gate the outcome — a self-report must + never be able to *fail* a run that actually did the work (e.g. the initial + implementer no-ops but a repair writes the files). It feeds only a neutral + concern when a task wrote nothing (it may be genuinely already-satisfied — + the orchestrator weighs it). + +The E2E phase's `status` is also read now (not just its fixtures): a +non-`DONE` E2E status or a zero-fixture run surfaces as a concern. + ## The Process — orchestrator side ### Step 1 — Run the `implement-loop` workflow @@ -203,6 +236,13 @@ The workflow returns with whatever work-in-progress it managed plus committed. The orchestrator decides what to do with the dirty working tree: +0. **No-op `BLOCKED` special case** — if the end-report's status is + `BLOCKED` with `blocked_md` null and a no-op `blocked_detail` (the reason + names a no-op iteration or a clean-tree no-op fix; `files_touched` is 0 in + standard mode, null in mini), there is **no `BLOCKED.md`** and the tree is + already clean. The diagnostic is in the end-report's `blocked_detail`, not + a file. Nothing to clean up or stash; re-plan or re-run from the same + clean tree. 1. Read `BLOCKED.md` — `## What did not` names the failure mode and the blocked task's reason. Read `git diff` to see what was attempted. 2. Decide: diff --git a/implement/workflows/implement-loop.js b/implement/workflows/implement-loop.js index c3dc48a..d8a2cb2 100644 --- a/implement/workflows/implement-loop.js +++ b/implement/workflows/implement-loop.js @@ -59,6 +59,7 @@ export const meta = { phases: [ { title: 'Preflight' }, { title: 'Per-task loop' }, + { title: 'Verify' }, { title: 'E2E coverage' }, { title: 'Report' }, ], @@ -115,10 +116,18 @@ const TASKS_SCHEMA = { const IMPL_SCHEMA = { type: 'object', additionalProperties: false, - required: ['status', 'summary'], + required: ['status', 'summary', 'applied_changes'], properties: { status: { type: 'string', enum: ['DONE', 'DONE_WITH_CONCERNS', 'NEEDS_CONTEXT', 'BLOCKED'] }, summary: { type: 'string', description: 'one line: what changed (paths + functions)' }, + applied_changes: { + type: 'boolean', + description: + 'GROUND TRUTH, not intent: did THIS task write to at least one file? true if you called Edit/Write (or ' + + 'equivalent) on any file; false if you made no file change at all — the code was already in the target ' + + 'state, or you skipped/misunderstood the task. Report it honestly even when status is DONE: a DONE that ' + + 'touched nothing is not a contribution, and the loop surfaces it for the orchestrator to weigh.', + }, concerns: { type: 'array', items: { type: 'string' } }, reason: { type: 'string', description: 'on BLOCKED/NEEDS_CONTEXT: the blocker' }, held: { @@ -176,6 +185,49 @@ const E2E_SCHEMA = { note: { type: 'string' }, }, } +// mini-mode only: independently OBSERVE that the RED test is now GREEN, rather +// than trusting the implementer's self-reported DONE. The whole telos of a +// mini-mode handoff is to drive one specific RED test to GREEN; a no-op leaves +// it RED while the implementer can still report DONE. This is the implement-loop +// analogue of compiler-driven-edit's verify agent (build+suite re-run + +// working_tree_dirty) — observe, do not assert. +const MINI_VERIFY_SCHEMA = { + type: 'object', + additionalProperties: false, + required: ['red_test_now_green', 'suite_green', 'working_tree_dirty'], + properties: { + red_test_now_green: { + type: 'boolean', + description: 'the handed-off RED test, re-run by name, now PASSES (it was red before the fix; confirm it is green now)', + }, + suite_green: { type: 'boolean', description: 'the full suite passes — the fix introduced no regression' }, + working_tree_dirty: { + type: 'boolean', + description: 'GROUND TRUTH from `git status --porcelain` being non-empty (NOT `git diff HEAD`, which omits brand-new untracked files): a fix actually landed (a clean tree means the RED test, if green, was never red, or nothing was done)', + }, + detail: { type: 'string', description: 'on any false: which test is still red, what regressed, or that the tree is clean' }, + }, +} +// Standard-mode no-op gate (issue #12, facets 1+2): the authoritative ground +// truth that the iteration's per-task work actually landed. Runs after the loop +// but BEFORE E2E/finalize, so neither E2E fixtures nor the stats/BLOCKED.md +// artefacts can inflate the count. Uses `git status --porcelain` (counts brand-new +// UNTRACKED files — the implementer leaves edits unstaged, so a new-file deliverable +// is untracked and `git diff HEAD` would miss it). A self-report can never reach +// this verdict; it is git ground truth. +const TREE_SCHEMA = { + type: 'object', + additionalProperties: false, + required: ['files_touched'], + properties: { + files_touched: { + type: 'integer', + description: + 'the line count of `git status --porcelain` (every changed/added/untracked path). Run exactly that — do NOT ' + + 'use `git diff`, which omits untracked new files. Do NOT edit anything. 0 means the iteration is a no-op.', + }, + }, +} const FINALIZE_SCHEMA = { type: 'object', additionalProperties: false, @@ -183,7 +235,6 @@ const FINALIZE_SCHEMA = { properties: { stats_path: { type: 'string' }, wrote_blocked_md: { type: 'boolean' }, - files_touched: { type: 'integer' }, }, } @@ -249,6 +300,12 @@ async function runTask(task) { if (impl.status === 'BLOCKED') return { id: task.id, title: task.title, outcome: 'BLOCKED', reason: impl.reason || 'worker-blocked' } if (impl.status === 'NEEDS_CONTEXT') return { id: task.id, title: task.title, outcome: 'BLOCKED', reason: `context-exhausted: ${impl.reason || ''}` } const concerns = impl.concerns || [] + // Did THIS task write a file, across the initial dispatch AND any repair? The + // initial implementer may legitimately no-op (the code was already in shape) + // while a spec/quality repair does the real writing — so OR the self-reports + // across every dispatch. This feeds a concern and the E2E gate; it is NOT the + // no-op verdict (the git file count is — see the iteration guard). + let wroteAnyFile = impl.applied_changes === true // 2.2 — spec-compliance check (gated before quality), with repair re-loop for (let round = 0; ; round++) { @@ -262,12 +319,13 @@ async function runTask(task) { if (spec && spec.status === 'unclear') return { id: task.id, title: task.title, outcome: 'BLOCKED', reason: 'spec-ambiguous', detail: spec.ambiguity } 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 - await agent( + const specFix = await agent( `${STANDING}\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', label: `impl-fix-spec:${task.id}`, phase: 'Per-task loop', schema: IMPL_SCHEMA }, ) + if (specFix && specFix.applied_changes === true) wroteAnyFile = true } // 2.3 — quality check, with repair re-loop. @@ -324,6 +382,7 @@ async function runTask(task) { `TASK ${task.id} — ${task.title}\n${task.text}\n\nISSUES:\n- ` + (qual?.issues || ['(none reported)']).join('\n- '), { agentType: 'implementer', label: `impl-fix-qual:${task.id}`, phase: 'Per-task loop', schema: IMPL_SCHEMA }, ) + if (fix && fix.applied_changes === true) wroteAnyFile = true if (fix && (fix.status === 'BLOCKED' || fix.status === 'NEEDS_CONTEXT')) { // The implementer judged a finding a real plan-vs-correctness conflict. Escalate loudly so it is // adjudicated, rather than letting that judgement ride a green commit as a quiet concern. @@ -344,7 +403,26 @@ async function runTask(task) { } } - return { id: task.id, title: task.title, outcome: 'DONE', concerns: [...concerns, ...heldConcerns] } + // Per-task no-op surfacing (issue #12, facet 1): a task that reports DONE yet + // wrote no file across any dispatch is suspicious — it was either genuinely + // already satisfied, or skipped/misunderstood. The script cannot tell those + // apart (same trust placed in the held/no-op partition above), so it does NOT + // gate on it — it surfaces a neutral concern for the orchestrator's Step-3 + // inspection. The iteration-level git file count is the gate, never this + // self-report (issue #12 review: the self-report must not be able to fail a + // run that actually did the work). + const noopConcern = !wroteAnyFile + ? [ + `task #${task.id} reported ${impl.status} but wrote no file — confirm it was genuinely already ` + + 'satisfied by the baseline (or earlier tasks), not skipped or misunderstood', + ] + : [] + return { + id: task.id, + title: task.title, + outcome: 'DONE', + concerns: [...concerns, ...heldConcerns, ...noopConcern], + } } const results = [] @@ -361,7 +439,78 @@ if (!blocked) outcome = 'DONE' else if (completed.length > 0) outcome = 'PARTIAL' else outcome = 'BLOCKED' +// An iteration-level guard (issue #12) may downgrade a DONE outcome to BLOCKED on +// positive-evidence grounds. It runs BEFORE finalize so the persisted stats.json +// records the final outcome (not a stale DONE), and so E2E does not run over a +// no-op. The verdict is git ground truth, never a per-task self-report — a +// self-report must never be able to FAIL a run that actually did the work. It +// carries its own blocked_detail (no single offending task) and a `noopClean` +// flag: a clean-tree no-op needs no BLOCKED.md (nothing to explain). +let synthBlock = null +let noopClean = false + +// ---- Phase 2.5 — verify (issue #12, facets 1+2+4) ---------------------- +// mini mode: independently OBSERVE the handed-off RED test is now GREEN (a no-op +// leaves it RED while the implementer can still report DONE). standard mode: the +// git file count is the authoritative no-op gate. Both are git ground truth, the +// implement-loop analogue of compiler-driven-edit's verify agent. +let miniVerify = null +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 ` + + `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 ' + + 'all (a clean tree means no fix landed). Do NOT edit anything; do NOT commit.', + { label: 'mini-verify', phase: 'Verify', schema: MINI_VERIFY_SCHEMA }, + ) + if (!miniVerify || !miniVerify.red_test_now_green || !miniVerify.suite_green || !miniVerify.working_tree_dirty) { + outcome = 'BLOCKED' + synthBlock = { + task: null, + reason: !miniVerify + ? 'mini-verify agent did not return' + : !miniVerify.working_tree_dirty + ? 'no-op fix: the working tree is clean — the RED->GREEN handoff produced no change' + : !miniVerify.red_test_now_green + ? 'the handed-off RED test is still red — the fix did not land; route back to debug (RED-first)' + : 'the fix regressed the suite — not behaviour-correct; route back to debug (RED-first)', + detail: miniVerify ? miniVerify.detail || null : null, + } + // a verified-clean tree is a clean no-op (no BLOCKED.md); a still-red/regression + // tree is genuinely dirty (BLOCKED.md explains it). A null agent is infra, not a + // known-clean tree — leave noopClean false so a BLOCKED.md is written if dirty. + if (miniVerify && !miniVerify.working_tree_dirty) noopClean = true + } +} else if (mode === 'standard' && outcome === 'DONE') { + phase('Verify') + treeState = await agent( + `${STANDING}\n\nReport the working-tree footprint of an implement iteration WITHOUT changing anything. 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. Do NOT edit, ' + + 'do NOT commit.', + { label: 'tree-check', phase: 'Verify', schema: TREE_SCHEMA }, + ) + if (!treeState || treeState.files_touched === 0) { + outcome = 'BLOCKED' + synthBlock = { + task: null, + reason: !treeState + ? 'infra: tree-check agent did not return — cannot confirm the iteration landed; treat as not-done' + : 'no-op iteration: `git status --porcelain` is empty — every task reported DONE but nothing landed in the working tree', + detail: null, + } + // a confirmed-empty tree is a clean no-op (no BLOCKED.md); a null agent is infra + // (tree state unknown — possibly dirty), so leave noopClean false. + if (treeState && treeState.files_touched === 0) noopClean = true + } +} + // ---- Phase 3 — E2E coverage (standard mode, only on a full clean run) -- +// outcome is DONE only past the no-op gate above, so E2E never runs over a no-op +// and cannot inflate the (already-taken) file count. let e2e = null if (mode === 'standard' && outcome === 'DONE') { phase('E2E coverage') @@ -374,8 +523,30 @@ if (mode === 'standard' && outcome === 'DONE') { ) } +// E2E status is load-bearing (issue #12, facet 3): the phase was previously +// dispatched but only its `fixtures` were read, so a tester that returned a +// non-DONE status or wrote zero fixtures silently left the iteration reading +// DONE with no protecting test. Surface it as a concern (never a gate — the +// coverage gap is the orchestrator's call, not a reason to fail a green run). +const e2eConcerns = [] +if (e2e) { + if (e2e.status && e2e.status !== 'DONE') { + e2eConcerns.push( + `E2E coverage did not complete cleanly (status ${e2e.status})` + + (e2e.note ? `: ${e2e.note}` : '') + + " — verify the iteration's invariants are actually protected", + ) + } else if (!(e2e.fixtures && e2e.fixtures.length)) { + e2eConcerns.push('E2E coverage reported DONE but added no fixtures — the iteration shipped without a protecting test') + } +} + // ---- Phase 4/5 — finalize: stats.json always, BLOCKED.md on non-DONE ---- phase('Report') +// A clean-tree no-op (mini or standard) gets NO BLOCKED.md (issue #12 / +// consistency): there is no dirty tree to explain, so the status + reason ride +// the end-report. `noopClean` was set by whichever no-op gate fired above. +const writeBlockedMd = outcome !== 'DONE' && !noopClean const aggregate = { iter_id, mode, @@ -384,8 +555,10 @@ const aggregate = { tasks_total: tasks.length, tasks_completed: completed.length, task_summaries: results.map((r) => ({ id: r.id, title: r.title, outcome: r.outcome })), - concerns: results.flatMap((r) => r.concerns || []), - blocked_detail: blocked ? { task: blocked.id, reason: blocked.reason, detail: blocked.detail || null } : null, + concerns: [...results.flatMap((r) => r.concerns || []), ...e2eConcerns], + blocked_detail: blocked + ? { task: blocked.id, reason: blocked.reason, detail: blocked.detail || null } + : synthBlock, } const fin = await agent( @@ -393,17 +566,24 @@ const fin = await agent( '1. Write a stats file at `/tmp/iter-' + iter_id + '/stats.json` (or under the project stats dir if its facts ' + 'declare one) containing: iter_id, date (from `date +%F`), mode, outcome, tasks_total, tasks_completed, and ' + 'blocked_reason (or null).\n' + - (outcome === 'DONE' - ? '2. Do NOT write BLOCKED.md — this run is DONE.\n' + (!writeBlockedMd + ? '2. Do NOT write BLOCKED.md — ' + + (outcome === 'DONE' ? 'this run is DONE.\n' : 'this is a clean-tree no-op; there is nothing to explain.\n') : '2. Write `BLOCKED.md` at the repo ROOT (uncommitted by convention; never staged). Sections: a `# BLOCKED — ' + 'iter ' + iter_id + '` header with Date/Started-from/Status/Tasks-completed, `## What ran` (one line per DONE ' + 'task), `## What did not` (the blocked task, its reason, and a one-line suggested next step), `## Concerns`, ' + - 'and `## Files touched` (from `git diff --name-only HEAD`).\n') + - 'Report the stats path, whether you wrote BLOCKED.md, and the count from `git diff --name-only HEAD | wc -l`. ' + - 'Do NOT commit anything.\n\nAGGREGATE:\n' + JSON.stringify(aggregate, null, 2), + 'and `## Files touched` (from `git status --porcelain`).\n') + + 'Report the stats path and whether you wrote BLOCKED.md. Do NOT commit anything.\n\nAGGREGATE:\n' + + JSON.stringify(aggregate, null, 2), { label: 'finalize', phase: 'Report', schema: FINALIZE_SCHEMA }, ) +// The no-op verdict was already taken before finalize (the tree-check / mini-verify +// gates set outcome + synthBlock + aggregate.blocked_detail), so the end-report +// just relays the final outcome. files_touched comes from the standard-mode +// tree-check (git status --porcelain, untracked-aware); it is null for mini / +// PARTIAL / BLOCKED, where the orchestrator reads BLOCKED.md and inspects the tree. + // ---- End-report — the orchestrator reads this, inspects the tree, commits. return { status: outcome, @@ -417,6 +597,6 @@ return { e2e_fixtures: e2e ? e2e.fixtures || [] : [], stats_path: fin ? fin.stats_path : null, blocked_md: fin && fin.wrote_blocked_md ? 'BLOCKED.md (uncommitted)' : null, - files_touched: fin ? fin.files_touched : null, + files_touched: treeState ? treeState.files_touched : null, blocked_detail: aggregate.blocked_detail, }