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

fix(implement-loop): hold plan-contradicting quality findings instead of oscillating to a false BLOCKED

Browse Source 
The per-task quality re-loop treated every `changes_requested` as
"deviate to satisfy", and the quality-repair dispatch was not even
given the task text. When a finding's only remedy contradicted a
plan-prescribed name/signature, the implementer renamed off-plan, the
next review flagged "diverges from plan", and the loop oscillated
name-misleads <-> diverges-from-plan until the retry cap, emitting a
false BLOCKED on code that was green the whole time.

Fix (issue's option 2 — the plan knowledge lives with the implementer,
which holds the task text; the quality-reviewer is deliberately blind
to it, so option 1 would have breached that separation):

- The quality-repair dispatch now receives the task text and a HOLD
  CLAUSE. A cosmetic finding (plan kept => build+tests green) is KEPT
  and recorded in a new `held` field; the loop surfaces it as a concern
  instead of chasing a deviation. A finding the implementer judges
  correctness-breaking escalates to BLOCKED, never a silent hold.
- The hold is keyed on two structural signals, never on a self-reported
  status enum (which the implementer contract overloads): the `held`
  array, and a no-op backstop over a required `diff_fingerprint` (a
  changes_requested verdict over an already-seen diff-state means the
  repair was a no-op or cycled back — re-running quality is futile).
  Fingerprints are tracked in a Set so A-B-A edit-then-revert is caught.
- A no-op-backstop concern is labelled neutrally (a byte-identical diff
  cannot tell a principled plan-hold from an ignored bug); implement
  SKILL.md Step 3 now routes a held/unresolved quality finding to
  orchestrator hand-verification before committing, even under /boss.

The held/no-op partition is the implementer's judgement, not enforced
in code — the residual fail-open is bounded to cosmetic-on-green
findings and disclosed, with the orchestrator's Step-3 inspection as
the backstop, the same trust placed in its other self-reports. Verified
across three rounds of adversarial review (held-the-plan paths, fail-
open laundering, schema/Set mechanics) — all closed.

closes #10
This commit is contained in:
Brummel
2026-06-22 20:11:06 +02:00
parent bbb217d9a8
commit 52db1abe10
3 changed files with 117 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
implement/workflows/implement-loop.js
+102 -7
View File
@@ -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 = []