diff --git a/implement/SKILL.md b/implement/SKILL.md index 98188ae..4b0c2a7 100644 --- a/implement/SKILL.md +++ b/implement/SKILL.md @@ -117,7 +117,10 @@ The script runs, per task: implementer phase, then spec-compliance, then quality — gated in that order. Each is a separate `agent()` call with a structured-output schema, so the script branches on a real verdict. The re-loop limit is **2 repair retries per failure-mode per -task; the 3rd unresolved attempt escalates to `BLOCKED`**. A +task; the 3rd unresolved attempt escalates to `BLOCKED`** — except that +a quality round whose findings are all `Minor`, arriving after at least +one repair dispatch, surfaces them as concerns and closes the task +instead of gating (cosmetic residue must not discard a green task). A `non_compliant` / `changes_requested` verdict re-dispatches the `implementer` with the review findings as the repair brief; `unclear` task text → `spec-ambiguous` BLOCKED; a tooling failure → `infra` diff --git a/implement/agents/quality-reviewer.md b/implement/agents/quality-reviewer.md index 30e7f1f..1836c99 100644 --- a/implement/agents/quality-reviewer.md +++ b/implement/agents/quality-reviewer.md @@ -187,6 +187,17 @@ 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. +The workflow schema also structures `issues`: each entry is +`{severity, text}` with severity `important` or `minor` +(Nits are not reported there — they never gate). Classify +severity honestly against the definitions above. After the +first repair round the loop stops gating on a minor-only +report — the findings surface as concerns instead of +forcing another round or a BLOCKED. That valve only works +if a `Minor` is never inflated to `Important` to force +another round; report what you see, the loop decides what +gates. + ## Common Rationalisations | Excuse | Reality | diff --git a/implement/workflows/implement-loop.js b/implement/workflows/implement-loop.js index 543a46d..9f4b4de 100644 --- a/implement/workflows/implement-loop.js +++ b/implement/workflows/implement-loop.js @@ -25,6 +25,14 @@ // • 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. +// • Minor-only quality rounds stop gating after the first repair: a +// changes_requested verdict carrying no Important finding, arriving +// after at least one quality-repair dispatch, surfaces its findings as +// concerns and lets the task close DONE. Each round's reviewer reads +// the diff fresh, so late rounds keep surfacing NEW polish (severity +// trending down) — without this valve a green task hard-BLOCKs on +// cosmetic residue and the whole iteration gets re-dispatched. +// Important findings still consume the full retry budget and BLOCK. // • 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 @@ -280,7 +288,26 @@ const QUAL_SCHEMA = { 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)' }, + issues: { + type: 'array', + description: 'Important + Minor only (Nits never gate)', + items: { + type: 'object', + additionalProperties: false, + required: ['severity', 'text'], + properties: { + severity: { + type: 'string', + enum: ['important', 'minor'], + description: + 'honest classification per the quality-reviewer severity definitions — the loop stops gating on ' + + 'minor-only residue after the first repair round, and that valve only works if a minor is never ' + + 'inflated to important', + }, + text: { type: 'string', description: ': — the issue' }, + }, + }, + }, diff_fingerprint: { type: 'string', description: @@ -614,8 +641,9 @@ async function runTask(task) { 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. Also report `diff_fingerprint` (REQUIRED): the sha256 hex of the FULL `git diff HEAD` ' + + '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` ' + '(`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.', // opus by design: the loop's last correctness gate (spec-reviewer only checks @@ -630,13 +658,32 @@ async function runTask(task) { // 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( + heldConcerns = (qual.issues && qual.issues.length ? qual.issues.map((i) => `${i.severity}: ${i.text}`) : ['(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 } + // Minor-only valve: after at least one repair dispatch, a changes_requested + // verdict with no Important finding is polish, not a gate. Each round's + // reviewer reads the diff fresh, so late rounds keep surfacing NEW minor + // residue (severity trending down) — chasing it exhausts the retry budget + // and hard-BLOCKs a green task over cosmetics, discarding the iteration's + // remaining tasks. Surface the findings as concerns for the orchestrator's + // Step-3 inspection and close the task instead. + if ( + qual && + qual.status === 'changes_requested' && + round >= 1 && + (qual.issues || []).length > 0 && + qual.issues.every((i) => i.severity === 'minor') + ) { + heldConcerns = qual.issues.map( + (i) => `minor-only quality residue (not gating after a repair round; fix on inspection if desired): ${i.text}`, + ) + break + } 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( @@ -653,7 +700,8 @@ async function runTask(task) { '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- '), + `TASK ${task.id} — ${task.title}\n${task.text}\n\nISSUES:\n- ` + + (qual?.issues?.length ? qual.issues.map((i) => `[${i.severity}] ${i.text}`) : ['(none reported)']).join('\n- '), { agentType: 'implementer', model: 'sonnet', effort: 'high', label: `impl-fix-qual:${task.id}`, phase: 'Per-task loop', schema: IMPL_SCHEMA }, ) if (fix && fix.applied_changes === true) wroteAnyFile = true