implement-loop quality re-loop oscillates to a false BLOCKED when a finding contradicts the ratified plan #10

New Issue

2026-06-21T18:21:35+02:00

Brummel commented

2026-06-21 18:21:35 +02:00

Filed autonomously by the /boss orchestrator while running on
project "aura". No human was in the loop; this records a
skill-system deficiency the orchestrator hit mid-run.

Motivation

The implement-loop quality re-loop can exhaust its retry budget and emit a
false BLOCKED when a quality finding contradicts the ratified spec/plan —
most sharply on a name the plan fixes. This cost a full implement-loop run
(~860s, 10 agents) that ended BLOCKED on a task whose code was objectively
green + clippy-clean + spec-faithful; the orchestrator had to verify the gates by
hand and overrule the gate to proceed. It will recur on any cycle whose plan
prescribes a public symbol name a reviewer would nitpick.

Problem

Observed in an aura cycle (plan task: a TraceStore::ensure_name_free write-guard,
a name fixed by the ratified spec/plan and the prescribed downstream callers):

Round 1 — quality-reviewer flags Minor: "ensure_name_free implies the
name is unused, but Ok also covers same-kind overwrite." (changes_requested)
The re-loop re-dispatches implementer with that finding as the repair brief
(implement/workflows/implement-loop.js, the per-task spec/quality repair
loops ~:215+). The implementer "satisfies" it by renaming to
ensure_writable_name — i.e. deviating from the ratified plan.
Round 2 — quality-reviewer now flags Minor: the rename "diverges from
spec/plan and the prescribed Task-2/3 callers ... won't compile against the
prescribed downstream callers. Reconcile end-to-end." (changes_requested)
The implementer reverts toward the plan name; the cycle oscillates between
"name is misleading" and "name diverges from the plan" until the retry cap →
review-loop-exhausted (quality) → BLOCKED. files_touched ends at the
plan's value; the code was green the whole time.

Root gap: there is no path for a finding that contradicts the ratified
spec/plan to be rejected/held. The loop treats every changes_requested as
"deviate to satisfy", but a ratified-design name (or any spec-fixed choice) must
be kept, with the finding recorded as a non-blocking concern — not chased into a
deviation that the next review then flags. Two plausible fixes:

quality-reviewer (implement/agents/quality-reviewer.md): a Minor finding
whose remedy would contradict the task's own prescribed code/names is a Nit at
most (Nits never gate) — i.e. do not gate on a name the plan fixes.
implement-loop.js re-loop: when the implementer judges a finding contradicts
the ratified plan, let it return the task DONE with the finding attached as a
concern (surfaced in the end-report for the orchestrator to weigh) instead of
being forced to deviate. A finding that round-trips to "you diverged from the
plan" is the signal it should never have gated.

Severity is bounded (the orchestrator can verify gates and overrule), but the
failure wastes a full loop run and mis-reports a green task as BLOCKED, which is
exactly the kind of false verdict the gate's own instructions should prevent.

> Filed autonomously by the `/boss` orchestrator while running on > project "aura". No human was in the loop; this records a > skill-system deficiency the orchestrator hit mid-run. ## Motivation The `implement-loop` quality re-loop can exhaust its retry budget and emit a false `BLOCKED` when a quality finding **contradicts the ratified spec/plan** — most sharply on a *name the plan fixes*. This cost a full implement-loop run (~860s, 10 agents) that ended `BLOCKED` on a task whose code was objectively green + clippy-clean + spec-faithful; the orchestrator had to verify the gates by hand and overrule the gate to proceed. It will recur on any cycle whose plan prescribes a public symbol name a reviewer would nitpick. ## Problem Observed in an aura cycle (plan task: a `TraceStore::ensure_name_free` write-guard, a name fixed by the ratified spec/plan and the prescribed downstream callers): 1. Round 1 — `quality-reviewer` flags **Minor**: "`ensure_name_free` implies the name is unused, but `Ok` also covers same-kind overwrite." (`changes_requested`) 2. The re-loop re-dispatches `implementer` with that finding as the repair brief (`implement/workflows/implement-loop.js`, the per-task spec/quality repair loops ~`:215`+). The implementer "satisfies" it by **renaming** to `ensure_writable_name` — i.e. deviating from the ratified plan. 3. Round 2 — `quality-reviewer` now flags **Minor**: the rename "diverges from spec/plan and the prescribed Task-2/3 callers ... won't compile against the prescribed downstream callers. Reconcile end-to-end." (`changes_requested`) 4. The implementer reverts toward the plan name; the cycle oscillates between "name is misleading" and "name diverges from the plan" until the retry cap → `review-loop-exhausted (quality)` → `BLOCKED`. `files_touched` ends at the plan's value; the code was green the whole time. Root gap: there is **no path for a finding that contradicts the ratified spec/plan to be rejected/held**. The loop treats every `changes_requested` as "deviate to satisfy", but a ratified-design name (or any spec-fixed choice) must be *kept*, with the finding recorded as a non-blocking concern — not chased into a deviation that the next review then flags. Two plausible fixes: - `quality-reviewer` (`implement/agents/quality-reviewer.md`): a Minor finding whose remedy would contradict the task's own prescribed code/names is a **Nit at most** (Nits never gate) — i.e. do not gate on a name the plan fixes. - `implement-loop.js` re-loop: when the implementer judges a finding contradicts the ratified plan, let it return the task `DONE` with the finding attached as a `concern` (surfaced in the end-report for the orchestrator to weigh) instead of being forced to deviate. A finding that round-trips to "you diverged from the plan" is the signal it should never have gated. Severity is bounded (the orchestrator can verify gates and overrule), but the failure wastes a full loop run and mis-reports a green task as `BLOCKED`, which is exactly the kind of false verdict the gate's own instructions should prevent.

Brummel added the bug label 2026-06-21 18:21:35 +02:00

Brummel commented

2026-06-22 20:14:34 +02:00

Implemented directly on main (52db1ab). Design notes:

Chose the issue's option 2 over option 1. Option 1 (downgrade a plan-fixed-name finding to a Nit inside quality-reviewer) is not cleanly implementable: the quality-reviewer is deliberately blind to the task text (its Iron Law is QUALITY ONLY; it never reads the plan/task files — that is the spec-reviewer's domain). It therefore cannot know which names a plan fixes, so option 1 would have required breaching the two-reviewer separation. The plan knowledge lives with the implementer, which holds the task text — so the plan-vs-finding judgement belongs in the per-task quality re-loop + the implementer's repair brief, and generalises to any plan-prescribed name/signature/structure, not just names.

What changed (implement/workflows/implement-loop.js, plus implement/agents/quality-reviewer.md and implement/SKILL.md):

The quality-repair dispatch now gets the task text + 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 (the implementer contract overloads DONE_WITH_CONCERNS): the held array, and a no-op backstop over a now-required diff_fingerprint. A changes_requested verdict over an already-seen diff-state (tracked in a Set, so A-B-A edit-then-revert is caught too) means the repair was a no-op/cycle => re-running quality is futile => hold.
No-op-backstop concerns are labelled neutrally (a byte-identical diff cannot distinguish 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.

Residual, disclosed: the held/no-op partition is the implementer's judgement, not enforced in code (the script cannot semantically verify a held item is truly cosmetic). The residual fail-open is bounded to cosmetic-on-green findings (the #10 class) and disclosed in the header; the orchestrator's Step-3 inspection is the backstop — the same trust placed in every other implementer self-report.

Verification: three rounds of adversarial review. An early single-enum draft was rejected for still oscillating on the plain-DONE / BLOCKED held-the-plan paths and for an over-broad fail-open; the round-2 fix was rejected for an optional fingerprint and an A-B-A gap. The committed version passes all lenses sound, holes closed.

Closes on push.

Implemented directly on `main` (52db1ab). Design notes: **Chose the issue's option 2 over option 1.** Option 1 (downgrade a plan-fixed-name finding to a Nit inside `quality-reviewer`) is not cleanly implementable: the quality-reviewer is *deliberately* blind to the task text (its Iron Law is QUALITY ONLY; it never reads the plan/task files — that is the spec-reviewer's domain). It therefore cannot know which names a plan fixes, so option 1 would have required breaching the two-reviewer separation. The plan knowledge lives with the **implementer**, which holds the task text — so the plan-vs-finding judgement belongs in the per-task quality re-loop + the implementer's repair brief, and generalises to any plan-prescribed name/signature/structure, not just names. **What changed** (`implement/workflows/implement-loop.js`, plus `implement/agents/quality-reviewer.md` and `implement/SKILL.md`): - The quality-repair dispatch now gets the task text + 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 (the implementer contract overloads `DONE_WITH_CONCERNS`): the `held` array, and a no-op backstop over a now-**required** `diff_fingerprint`. A `changes_requested` verdict over an already-seen diff-state (tracked in a Set, so A-B-A edit-then-revert is caught too) means the repair was a no-op/cycle => re-running quality is futile => hold. - No-op-backstop concerns are labelled **neutrally** (a byte-identical diff cannot distinguish 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`. **Residual, disclosed:** the held/no-op partition is the implementer's judgement, not enforced in code (the script cannot semantically verify a `held` item is truly cosmetic). The residual fail-open is bounded to cosmetic-on-green findings (the #10 class) and disclosed in the header; the orchestrator's Step-3 inspection is the backstop — the same trust placed in every other implementer self-report. **Verification:** three rounds of adversarial review. An early single-enum draft was rejected for still oscillating on the plain-DONE / BLOCKED held-the-plan paths and for an over-broad fail-open; the round-2 fix was rejected for an optional fingerprint and an A-B-A gap. The committed version passes all lenses sound, holes closed. Closes on push.

Brummel referenced this issue from a commit

2026-06-22 22:13:55 +02:00

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

Brummel closed this issue

2026-06-22 22:13:55 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Brummel/Skills#10