diff --git a/implement/SKILL.md b/implement/SKILL.md index 96c583e..01677bb 100644 --- a/implement/SKILL.md +++ b/implement/SKILL.md @@ -178,6 +178,13 @@ is no `BLOCKED.md` (DONE never writes one). that the diff itself does not: the *why*, the alternatives considered and rejected, the verification steps run, and any concerns that remain. Detail-fill comes from the end-report. + A concern phrased as a *held* or *unresolved quality finding* (the + per-task quality gate kept a plan-prescribed element against a + reviewer finding, or a repair was a no-op) is the gate handing you + the judgement it could not make in code: verify that finding by + hand against the diff before committing — do not auto-sign past it, + even under `/boss`. Keep it if it is a principled plan-hold; if it + is an unaddressed defect, send the iter back rather than commit. 4. Stage + commit the code edits and the stats file. 5. If the trigger is done-state and the user is away, run the project's configured notification command per `../boss/SKILL.md` "Done-state diff --git a/implement/agents/quality-reviewer.md b/implement/agents/quality-reviewer.md index 1812fb2..c097f96 100644 --- a/implement/agents/quality-reviewer.md +++ b/implement/agents/quality-reviewer.md @@ -177,6 +177,14 @@ At most 250 words, structured: Important + Minor, re-review` / `infra problem, see status`. +When the `implement-loop` workflow dispatches you, its +structured schema additionally requires a `diff_fingerprint`: +the sha256 hex of the FULL `git diff HEAD` (the whole +working-tree diff, `git diff HEAD | sha256sum` — not narrowed +to the task footprint). The loop compares it across re-review +rounds to detect a no-op repair and stop oscillating; compute +it from the same full diff you reviewed, every round. + ## Common Rationalisations | Excuse | Reality | diff --git a/implement/workflows/implement-loop.js b/implement/workflows/implement-loop.js index c3c9f81..c3dc48a 100644 --- a/implement/workflows/implement-loop.js +++ b/implement/workflows/implement-loop.js @@ -25,6 +25,20 @@ // • Spec-compliance is gated BEFORE quality, per task. // • Re-loop limit: 2 repair retries per failure-mode per task; the 3rd // unresolved attempt escalates to BLOCKED. +// • Quality re-loop hold (issue #10): a quality finding whose only remedy +// would contradict the ratified plan (a name/signature/structure the +// task prescribes) is HELD, not chased into a deviation the next review +// then re-flags. The repair implementer keeps the plan and records the +// finding in `held`; a no-op backstop holds it anyway when a repair +// leaves the diff unchanged (or cycles back) yet quality still flags. +// Either way the finding surfaces as a concern, not a false BLOCKED on a +// green task — and the orchestrator weighs it on its Step-3 inspection. +// A finding the implementer judges correctness-breaking (would not +// build / a test fails / a prescribed caller breaks) escalates to +// BLOCKED instead of holding. The held/no-op partition is the +// implementer's call, not enforced in code (the script cannot tell a +// cosmetic hold from an ignored bug) — hence the neutral concern label +// and the orchestrator backstop, same trust placed in its other reports. // • The script has no filesystem or shell access of its own — every // working-tree mutation (code, stats.json, BLOCKED.md) happens inside // an agent() call. @@ -107,6 +121,24 @@ const IMPL_SCHEMA = { summary: { type: 'string', description: 'one line: what changed (paths + functions)' }, concerns: { type: 'array', items: { type: 'string' } }, reason: { type: 'string', description: 'on BLOCKED/NEEDS_CONTEXT: the blocker' }, + held: { + type: 'array', + description: + 'quality-repair dispatch only: quality findings KEPT against the ratified plan because honouring them ' + + 'would deviate from a plan-prescribed name/signature/structure while the tree stays green. Each is a ' + + 'concern to surface, not a deviation to chase. A finding that would break correctness is NOT held — it ' + + 'is escalated via status BLOCKED.', + items: { + type: 'object', + additionalProperties: false, + required: ['finding', 'kept', 'why'], + properties: { + finding: { type: 'string', description: 'the quality finding being kept (held)' }, + kept: { type: 'string', description: 'the plan-prescribed element preserved (name/signature/structure)' }, + why: { type: 'string', description: 'why honouring the finding would contradict the ratified plan' }, + }, + }, + }, }, } const SPEC_SCHEMA = { @@ -122,10 +154,16 @@ const SPEC_SCHEMA = { const QUAL_SCHEMA = { type: 'object', additionalProperties: false, - required: ['status'], + required: ['status', 'diff_fingerprint'], properties: { status: { type: 'string', enum: ['approved', 'changes_requested', 'infra_blocked'] }, issues: { type: 'array', items: { type: 'string' }, description: 'Important + Minor only (Nits never gate)' }, + diff_fingerprint: { + type: 'string', + description: + 'sha256 hex of the FULL `git diff HEAD` (the whole working-tree diff, not a footprint subset) — REQUIRED; ' + + 'the loop compares it across rounds to detect a no-op/cyclic repair and stop oscillating to a false BLOCKED', + }, }, } const E2E_SCHEMA = { @@ -232,24 +270,81 @@ async function runTask(task) { ) } - // 2.3 — quality check, with repair re-loop + // 2.3 — quality check, with repair re-loop. + // The plan-vs-finding judgement lives here, with the implementer that holds + // the task text — the quality-reviewer is deliberately blind to it. The hold + // is keyed on two structural signals, never on a self-reported status enum + // (which the implementer's contract overloads): the `held` array the repair + // returns, and a no-op backstop (a changes_requested verdict over a diff + // whose fingerprint was already reviewed-and-flagged this task means the + // intervening repair changed nothing or cycled back, so re-running quality + // is futile). + let heldConcerns = [] + 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). ` + 'Read every chunk of `git diff HEAD` in this task\'s footprint. Report only Important and Minor issues; ' + - 'Nits never gate.', + 'Nits never gate. Also report `diff_fingerprint` (REQUIRED): the sha256 hex of the FULL `git diff HEAD` ' + + '(`git diff HEAD | sha256sum` — the whole working-tree diff, NOT narrowed to this task\'s footprint). ' + + 'The loop compares it across rounds to detect a no-op/cyclic repair.', { agentType: 'quality-reviewer', label: `qual:${task.id}`, phase: 'Per-task loop', schema: QUAL_SCHEMA }, ) if (qual && qual.status === 'approved') break + // No-op / cyclic-repair backstop: a changes_requested verdict over a diff-state already reviewed-and-flagged + // this task means the intervening repair changed nothing (or cycled back to an earlier state) — the + // implementer kept the plan, or rejected the finding. Re-running quality would only re-flag identical code, + // so stop here rather than burning the retry budget down to a false BLOCKED. A byte-identical diff cannot + // distinguish a principled plan-hold from an ignored finding, so the concern is labelled neutrally and left + // for the orchestrator to adjudicate on its Step-3 inspection. + if (qual && qual.status === 'changes_requested' && qual.diff_fingerprint && seenFingerprints.has(qual.diff_fingerprint)) { + heldConcerns = (qual.issues && qual.issues.length ? qual.issues : ['(finding not reported)']).map( + (i) => + 'unresolved quality finding — the repair left the diff unchanged (a kept plan-prescribed element, or a ' + + `finding the implementer rejected); verify by hand on inspection: ${i}`, + ) + break + } if (round >= 2) return { id: task.id, title: task.title, outcome: 'BLOCKED', reason: 'review-loop-exhausted (quality)' } - await agent( - `${STANDING}\n\nFix the quality issues on task ${task.id}. Address EACH Important/Minor issue; do not widen ` + - 'scope. Keep build + tests green; leave edits unstaged.\n\nISSUES:\n- ' + (qual?.issues || ['(none reported)']).join('\n- '), + 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; ` + + '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 ' + + 'prescribes), do NOT deviate. Two cases:\n' + + ' • The finding is cosmetic — keeping the plan leaves the build and tests GREEN (e.g. a name reads ' + + 'slightly off but everything compiles and passes). KEEP the plan, fix every other issue, and record each ' + + 'kept finding in `held` (finding / kept / why). Do not rename or restructure to chase it.\n' + + ' • Honouring the plan would make the code WRONG — it would not build, a test would fail, or a ' + + 'prescribed caller would break. That is a real plan-vs-correctness conflict, not a cosmetic hold: report ' + + 'status BLOCKED with the conflict in `reason`. Never bury a correctness defect in `held`.\n' + + 'A finding whose only remedy is to diverge from the ratified plan must be held or escalated, never chased ' + + 'into a deviation the next review flags.\n\n' + + `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.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. + return { + id: task.id, + title: task.title, + outcome: 'BLOCKED', + reason: `quality/plan conflict: ${fix.reason || 'honouring a quality finding would break the ratified plan'}`, + } + } + if (fix && fix.held && fix.held.length) { + // The implementer kept plan-prescribed elements against cosmetic findings. Stop chasing — re-running + // quality would only re-flag the unchanged code. Surface the held findings as concerns to weigh. + heldConcerns = fix.held.map( + (h) => `quality finding held against the ratified plan: ${h.finding} — kept ${h.kept}; honouring it would ${h.why}`, + ) + break + } } - return { id: task.id, title: task.title, outcome: 'DONE', concerns } + return { id: task.id, title: task.title, outcome: 'DONE', concerns: [...concerns, ...heldConcerns] } } const results = []