Brummel/Skills
1
0
Fork 0
Code Issues 3 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(implement-loop): minor-only quality rounds surface as concerns, not BLOCKED

Browse Source 
A quality changes_requested verdict whose findings are all Minor,
arriving after at least one repair dispatch, now breaks the loop with
the findings surfaced as concerns instead of consuming the retry budget
toward a hard BLOCKED.

Grounding (aura runs, 2026-07-12/13): each quality round reads the diff
fresh, so late rounds keep surfacing NEW polish with severity trending
down (Important -> Minor -> comment-only Minor). Run sab-246-t56
(wf_643ee9fa) fixed its one Important finding in round 1, then hard-
BLOCKed an 85-minute green task in round 3 over two stale comment
cross-references; the sibling task was never attempted and the
re-dispatch cost another 32 minutes. Three of 14 runs across the last
three cycles ended review-loop-exhausted with no commit-blocking
residue.

Mechanically, the QUAL_SCHEMA issues array is now structured as
{severity: 'important'|'minor', text} so the valve keys on a declared
classification instead of parsing free-text prefixes; the fingerprint
backstop and the repair-brief prompt render the new shape, and the
quality-reviewer persona documents the honest-severity requirement.
Important findings still consume the full retry budget and BLOCK.
This commit is contained in:
Claude
2026-07-13 11:45:47 +02:00
parent cf708632c2
commit 5606ca0213
3 changed files with 68 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
implement/SKILL.md
+4 -1
View File
@@ -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`
implement/agents/quality-reviewer.md
+11
View File
@@ -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 |
implement/workflows/implement-loop.js
+53 -5
View File
@@ -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: '<file>:<line> — 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