fix(implement-loop): require positive edit evidence before DONE
The #11 vacuous-green shape on the main path: `outcome = DONE` was set purely from the absence of a BLOCKED task, with no check that the iteration actually wrote anything. `files_touched` was even computed by the finalize agent but never asserted > 0, `e2e.status` was never read, and in mini mode the handed-off RED test was never independently re-run (GREEN was asserted from the implementer's self-report, never observed). Add a positive-evidence precondition on DONE, taken from git ground truth — never a self-report, which must not be able to fail a run that actually did the work: - Standard mode: a dedicated `tree-check` agent runs `git status --porcelain | wc -l` after the per-task loop but BEFORE E2E/finalize. Zero (or a non-returning agent) downgrades DONE to BLOCKED. `git status --porcelain` (not `git diff HEAD`) so a brand-new untracked file — the implementer leaves edits unstaged — still counts; running pre-E2E/finalize keeps fixtures and the stats/BLOCKED.md artefacts from inflating it. - Mini mode: an independent `mini-verify` agent re-runs the RED test by name and the suite and checks the tree is dirty; a still-red test, a regression, or a clean tree is BLOCKED (route back to debug). - Per-task `applied_changes` (OR-ed across the initial dispatch and every repair) feeds only a neutral concern, never the outcome. - `e2e.status` is now read: a non-DONE status or a zero-fixture run surfaces as a concern. A clean-tree no-op is BLOCKED with no BLOCKED.md (nothing to clean up); the status + reason ride the end-report. SKILL.md documents the precondition, the no-BLOCKED.md carve-out, and the Step-4 handling. closes #12
This commit is contained in:
@@ -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,
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user