From a7b23cff7f394e16a77d02bc727be958365b8ae9 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Mon, 7 Sep 2026 14:51:21 +0200 Subject: [PATCH 1/5] fix(review-gate): derive the lens verdict from its own Blockers section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two failures in one day, both from the same root: the verdict token is a separate judgement from the findings, and the two consumers ask different questions. **1. A verdict that contradicted its own review.** On #227 the maintainability lens printed: ### Blockers None. The invariants that could break silently do fail closed ... REVIEW_FAILED No blockers, and REVIEW_FAILED. A caller cannot appeal that: lens-runner.sh makes the exit code authoritative on purpose, because a substring gate would be fail-open. So a broken review blocks finished work with no recourse. The prompt now makes the token DERIVED rather than chosen: head a section exactly `### Blockers`, write None when there are none, and the token follows from that section. Concerns and notes are explicitly not blockers and must not change it. The runner also detects the contradiction and labels it: PRESWARM_: CONTRADICTION — review says 'Blockers: None' but emitted REVIEW_FAILED; treating as NO_VERDICT (gate defect, not a finding) This NEVER upgrades a verdict. Exit stays 1. Turning a failure into a pass on a substring is exactly the fail-open the classifier refuses; relabelling one so a branch is not blamed for a gate defect is not. **2. Prompt drift between the two consumers.** `lens-runner.sh` carried detailed prompts while `review-swarm.yaml` carried one-line summaries with every specific instruction stripped — and auto-merge acts on the swarm, the weaker of the two. That is how #215 merged with defects the local run had named. The three roles now carry the same clauses as the runner, including the Blockers-derivation rule. Verified the detector against six shapes, including the two that matter: "### Blockers\nNone. The invariants..." -> NONE (the real #227 text) "### Blockers\n1. real\n### Concerns\nNone." -> HAS (not fooled by a later None) no Blockers section at all -> HAS (fail-closed) "### Blockers\n\nNone." -> NONE (blank line tolerated) "**None** — nothing blocking" -> NONE (bold tolerated) two numbered blockers -> HAS `bash -n` clean; review-swarm.yaml still parses. Does not consolidate the prompts into one file both consumers read — that is the end state #218 proposes and needs the swarm spec to load role text from disk. This makes them agree and adds the derivation rule; the single source of truth is still open. Refs #218, #227, #215 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR --- ops/preswarm-check/lens-runner.sh | 34 +++++++++++++++++++++++++++++++ workflows/review-swarm.yaml | 27 +++++++++++++++++++++--- 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/ops/preswarm-check/lens-runner.sh b/ops/preswarm-check/lens-runner.sh index e8afbedc2..3f49261ad 100755 --- a/ops/preswarm-check/lens-runner.sh +++ b/ops/preswarm-check/lens-runner.sh @@ -182,6 +182,20 @@ $(cat "$DIFF_FILE") Produce a concise review (200-500 words). Cite specific files and line ranges from the diff. Name blockers vs concerns vs notes. +Structure the review with a section headed EXACTLY: + + ### Blockers + +If there are no blockers, the first word under that heading must be `None`. + +Your final token is DERIVED from that section — it is not a separate judgement: + - `### Blockers` says None -> you MUST end with REVIEW_PASSED + - `### Blockers` lists one or more -> you MUST end with REVIEW_FAILED + +A token that disagrees with your own Blockers section is a defect in the review, +not a stricter verdict. Concerns and notes are NOT blockers and must not change +the token. + END your output with EXACTLY ONE of these tokens on its own line: REVIEW_PASSED — no blockers REVIEW_FAILED — at least one blocker @@ -239,8 +253,28 @@ printf '%s\n' "$OUTPUT" # - LAST_VERDICT == "REVIEW_PASSED" AND CLI_RC != 0 → exit 1 (NO_VERDICT — a CLI that emitted PASSED then errored is untrustworthy) # - LAST_VERDICT missing (no anchored line at all) → exit 1 (NO_VERDICT) LAST_VERDICT=$(printf '%s\n' "$OUTPUT" | grep -E '^REVIEW_(PASSED|FAILED)$' | tail -1) + +# Does the review's own Blockers section say there are none? Read the first +# non-blank line under the LAST `### Blockers` heading. This NEVER upgrades a +# verdict — it only relabels REVIEW_FAILED as CONTRADICTION, and the exit code +# stays 1. Turning a failure into a pass on a substring would be exactly the +# fail-open the classifier above refuses. +blockers_say_none() { + printf '%s\n' "$OUTPUT" \ + | awk '/^#+[[:space:]]*Blockers[[:space:]]*$/{f=1;next} f&&NF{print;exit}' \ + | grep -qiE '^\**None\b' +} + case "$LAST_VERDICT" in REVIEW_FAILED) + if blockers_say_none; then + # The lens found nothing blocking and still emitted REVIEW_FAILED. That is + # a broken review, not a stricter one, and a caller cannot appeal it: the + # exit code is authoritative by design. Say so plainly so the branch is not + # blamed for a gate defect. See flows#218. + echo "PRESWARM_${LENS}: CONTRADICTION — review says 'Blockers: None' but emitted REVIEW_FAILED; treating as NO_VERDICT (gate defect, not a finding)" >&2 + exit 1 + fi echo "PRESWARM_${LENS}: REVIEW_FAILED" exit 1 ;; diff --git a/workflows/review-swarm.yaml b/workflows/review-swarm.yaml index df2139777..571253d48 100644 --- a/workflows/review-swarm.yaml +++ b/workflows/review-swarm.yaml @@ -24,15 +24,36 @@ agents: - name: maintainability cli: claude preset: reviewer - role: Reviews for maintainability — will a stranger understand and safely change this in six months? + role: >- + MAINTAINABILITY lens. Could a stranger read this diff in six months and + change it safely? Name unclear boundaries, implicit contracts, missing + failure handling, comments that assert what the code does not do, and + tests that would not fail if the behavior broke. Head a section exactly + `### Blockers`; write None under it when there are none. Your final + REVIEW_PASSED/REVIEW_FAILED token is DERIVED from that section — concerns + and notes are not blockers and must not change it. - name: history cli: codex preset: reviewer - role: Reviews the change against the story of the code — does it fit what the repo has been becoming? + role: >- + HISTORY lens. Does this fit the story of the code? Reject ONLY when the + diff repeats a mistake ops/DRIVE-LOG.md records, or introduces a NEW + contradiction with a settled RFC-0001 decision, or claims evidence it did + not produce. Head a section exactly `### Blockers`; write None under it + when there are none. Your final REVIEW_PASSED/REVIEW_FAILED token is + DERIVED from that section — concerns and notes must not change it. - name: structure cli: opencode preset: reviewer - role: Reviews structure — boundaries, coupling, whether the shape matches the contract in RFC-0001. + role: >- + STRUCTURE lens. Boundaries, coupling, file size, single purpose. Does the + shape match RFC-0001 (closed kernel vocabulary, helpers over primitives, + fail-closed, completionReason discipline) and AGENTS.md? Name anything + that puts product logic in the kernel, adds a primitive instead of a + helper, or grows a file past its purpose. Head a section exactly + `### Blockers`; write None under it when there are none. Your final + REVIEW_PASSED/REVIEW_FAILED token is DERIVED from that section — concerns + and notes must not change it. workflows: - name: review-pr From b15a7356be34139007f9250a36f6c34d6c216f2f Mon Sep 17 00:00:00 2001 From: kjgbot Date: Tue, 8 Sep 2026 00:43:53 +0200 Subject: [PATCH 2/5] fix(review-gate): a PASS that lists blockers is also a contradiction An independent spec review found a fail-open in my own fix. The REVIEW_PASSED arm checked only the CLI exit code and never consulted the Blockers section, so a review that enumerated blockers -- unauthorized writes among them -- and ended in REVIEW_PASSED exited 0. The comment above that arm claims the classifier "NEVER upgrades a verdict", and it does not. That was the wrong safety property to reason about. One- directional safety guards fail->pass, which fails CLOSED anyway, and leaves the fail-OPEN direction unguarded, which is the only direction a gate cannot afford to get wrong. I wrote that comment as a proof of safety; it was a proof about the harmless half. `blockers_are_listed` is deliberately NOT the negation of `blockers_say_none`: an ABSENT Blockers section returns false, so a review that never emitted the section keeps its previous behaviour rather than newly failing. That closes the unambiguous hole without changing the blast radius for non-conforming lenses. Verified across all five arms: blockers listed + PASSED -> CONTRADICTION (exit 1) was: exit 0 Blockers: None + PASSED -> REVIEW_PASSED (exit 0) no section + PASSED -> REVIEW_PASSED (exit 0) unchanged Blockers: None + FAILED -> CONTRADICTION (exit 1) blockers listed + FAILED -> REVIEW_FAILED (exit 1) Direction of the change is strictly tightening: it can only turn a pass into a non-verdict, never a failure into a pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR --- ops/preswarm-check/lens-runner.sh | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/ops/preswarm-check/lens-runner.sh b/ops/preswarm-check/lens-runner.sh index 3f49261ad..bf4e39ff8 100755 --- a/ops/preswarm-check/lens-runner.sh +++ b/ops/preswarm-check/lens-runner.sh @@ -259,6 +259,20 @@ LAST_VERDICT=$(printf '%s\n' "$OUTPUT" | grep -E '^REVIEW_(PASSED|FAILED)$' | ta # verdict — it only relabels REVIEW_FAILED as CONTRADICTION, and the exit code # stays 1. Turning a failure into a pass on a substring would be exactly the # fail-open the classifier above refuses. +# True when the LAST `### Blockers` section exists AND its first non-blank line +# is something other than "None". Deliberately NOT the negation of +# blockers_say_none: an ABSENT section returns false here, so a review that +# omits the section keeps its previous behaviour instead of newly failing. This +# closes the unambiguous fail-open without changing the blast radius for +# reviews that never emitted the section at all. +blockers_are_listed() { + first="$(printf '%s\n' "$OUTPUT" \ + | awk '/^#+[[:space:]]*Blockers[[:space:]]*$/{f=1;next} f&&NF{print;exit}')" + [ -n "$first" ] || return 1 + printf '%s' "$first" | grep -qiE '^\**None\b' && return 1 + return 0 +} + blockers_say_none() { printf '%s\n' "$OUTPUT" \ | awk '/^#+[[:space:]]*Blockers[[:space:]]*$/{f=1;next} f&&NF{print;exit}' \ @@ -280,6 +294,21 @@ case "$LAST_VERDICT" in ;; REVIEW_PASSED) if [ "$CLI_RC" -eq 0 ]; then + if blockers_are_listed; then + # A review that enumerates blockers and still emits REVIEW_PASSED is the + # same defect as the REVIEW_FAILED arm above, in the direction that + # actually matters. That arm relabels a contradiction which already + # fails CLOSED; this one would have let a review naming unauthorized + # writes exit 0. + # + # The comment above claims this classifier "NEVER upgrades a verdict", + # and it does not. That was the wrong safety property to reason about: + # one-directional safety left the fail-OPEN direction unguarded, which + # is the only direction a gate cannot afford to get wrong. Found by an + # independent spec review, not by the author. + echo "PRESWARM_${LENS}: CONTRADICTION — review listed blockers but emitted REVIEW_PASSED; treating as NO_VERDICT (gate defect, not a pass)" >&2 + exit 1 + fi echo "PRESWARM_${LENS}: REVIEW_PASSED" exit 0 fi From 55670e57b9266194d6ed531f3603dd980f4b7043 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Tue, 8 Sep 2026 00:57:02 +0200 Subject: [PATCH 3/5] fix(review-gate): read the LAST Blockers section, not the first The P2 from the same spec review, and it defeated the P1 fix I shipped an hour ago. Both helpers used: awk '/^#+[[:space:]]*Blockers[[:space:]]*$/{f=1;next} f&&NF{print;exit}' which flags on the FIRST matching heading and exits at its first body line. A review with an early "Blockers: None" summary and a later real section is read as "None": first-match awk -> None (guard passes the review) last-match awk -> - unauthorized write (guard blocks it) So the fail-open I closed was still reachable through a differently-shaped review, and `blockers_are_listed` inherited the flaw the moment I wrote it on top of the same pattern. The comment above these helpers has said "the LAST `### Blockers` heading" since the original change. The code never did that. A comment describing intent rather than behaviour is worse than no comment: I read it twice while fixing P1 and took it as a description of what the code did. Both helpers now accumulate to the last matching section. Verified across seven arms, including the two multi-section cases that motivated this: early None + LATER real blockers + PASSED -> CONTRADICTION (exit 1) early real + LATER None + FAILED -> CONTRADICTION (exit 1) blockers listed + PASSED -> CONTRADICTION (exit 1) Blockers: None + PASSED -> REVIEW_PASSED (exit 0) no section + PASSED -> REVIEW_PASSED (exit 0) Blockers: None + FAILED -> CONTRADICTION (exit 1) blockers listed + FAILED -> REVIEW_FAILED (exit 1) Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR --- ops/preswarm-check/lens-runner.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ops/preswarm-check/lens-runner.sh b/ops/preswarm-check/lens-runner.sh index bf4e39ff8..65ca57007 100755 --- a/ops/preswarm-check/lens-runner.sh +++ b/ops/preswarm-check/lens-runner.sh @@ -267,7 +267,7 @@ LAST_VERDICT=$(printf '%s\n' "$OUTPUT" | grep -E '^REVIEW_(PASSED|FAILED)$' | ta # reviews that never emitted the section at all. blockers_are_listed() { first="$(printf '%s\n' "$OUTPUT" \ - | awk '/^#+[[:space:]]*Blockers[[:space:]]*$/{f=1;next} f&&NF{print;exit}')" + | awk '/^#+[[:space:]]*Blockers[[:space:]]*$/{f=1;buf="";next} f&&NF&&buf==""{buf=$0} END{print buf}')" [ -n "$first" ] || return 1 printf '%s' "$first" | grep -qiE '^\**None\b' && return 1 return 0 @@ -275,7 +275,7 @@ blockers_are_listed() { blockers_say_none() { printf '%s\n' "$OUTPUT" \ - | awk '/^#+[[:space:]]*Blockers[[:space:]]*$/{f=1;next} f&&NF{print;exit}' \ + | awk '/^#+[[:space:]]*Blockers[[:space:]]*$/{f=1;buf="";next} f&&NF&&buf==""{buf=$0} END{print buf}' \ | grep -qiE '^\**None\b' } From 747a61b8a7445daddd9f637210dc86b1ff11051d Mon Sep 17 00:00:00 2001 From: kjgbot Date: Tue, 8 Sep 2026 04:07:36 +0200 Subject: [PATCH 4/5] fix(review-gate): a missing Blockers section is NO_VERDICT, not a pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cubic P1 on #229. A lens that emits REVIEW_PASSED with no `### Blockers` section cleared the gate, because `blockers_are_listed` returns false for an absent section and the PASSED arm read that as "no blockers". My own comment defended this: an absent section should "keep its previous behaviour instead of newly failing", to avoid widening the blast radius. That was protecting a case the prompt already forbids — it requires the heading and says the first word under it must be `None` when there are none. A review without it has not answered the question the gate asks. Add `blockers_section_present` as a separate guard rather than overloading the existing boolean, so the log distinguishes "the lens contradicted itself" from "the lens ignored the output contract"; those need different fixes. Verified against all four cases: no section + PASSED -> NO_VERDICT; None + PASSED -> PASSED; blockers listed + PASSED -> CONTRADICTION; and a second Blockers section listing one still reads the LAST section. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR --- ops/preswarm-check/lens-runner.sh | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/ops/preswarm-check/lens-runner.sh b/ops/preswarm-check/lens-runner.sh index 65ca57007..5d3c31af8 100755 --- a/ops/preswarm-check/lens-runner.sh +++ b/ops/preswarm-check/lens-runner.sh @@ -260,11 +260,11 @@ LAST_VERDICT=$(printf '%s\n' "$OUTPUT" | grep -E '^REVIEW_(PASSED|FAILED)$' | ta # stays 1. Turning a failure into a pass on a substring would be exactly the # fail-open the classifier above refuses. # True when the LAST `### Blockers` section exists AND its first non-blank line -# is something other than "None". Deliberately NOT the negation of -# blockers_say_none: an ABSENT section returns false here, so a review that -# omits the section keeps its previous behaviour instead of newly failing. This -# closes the unambiguous fail-open without changing the blast radius for -# reviews that never emitted the section at all. +# is something other than "None". An ABSENT section returns false here, which is +# why it cannot be the only guard: absence is handled separately by +# blockers_section_present, so the two diagnoses stay distinguishable in the +# log. Do not collapse them — "the lens contradicted itself" and "the lens +# ignored the output contract" need different fixes. blockers_are_listed() { first="$(printf '%s\n' "$OUTPUT" \ | awk '/^#+[[:space:]]*Blockers[[:space:]]*$/{f=1;buf="";next} f&&NF&&buf==""{buf=$0} END{print buf}')" @@ -273,6 +273,16 @@ blockers_are_listed() { return 0 } +# The prompt does not treat `### Blockers` as optional: it requires the heading +# and says the first word under it must be `None` when there are none. A review +# that omits it entirely has not answered the question the gate asks, and +# accepting it as a pass is a fail-open — flows#229, cubic P1. My earlier note +# here argued absence should "keep its previous behaviour instead of newly +# failing". That was protecting a case the prompt already forbids. +blockers_section_present() { + printf '%s\n' "$OUTPUT" | grep -qE '^#+[[:space:]]*Blockers[[:space:]]*$' +} + blockers_say_none() { printf '%s\n' "$OUTPUT" \ | awk '/^#+[[:space:]]*Blockers[[:space:]]*$/{f=1;buf="";next} f&&NF&&buf==""{buf=$0} END{print buf}' \ @@ -294,6 +304,10 @@ case "$LAST_VERDICT" in ;; REVIEW_PASSED) if [ "$CLI_RC" -eq 0 ]; then + if ! blockers_section_present; then + echo "PRESWARM_${LENS}: CONTRADICTION — review emitted REVIEW_PASSED with no '### Blockers' section; the prompt requires one, so this review is malformed (NO_VERDICT, not a pass)" >&2 + exit 1 + fi if blockers_are_listed; then # A review that enumerates blockers and still emits REVIEW_PASSED is the # same defect as the REVIEW_FAILED arm above, in the direction that From 528c5a9782d25a1f9566cd66d76035c4a6cb933f Mon Sep 17 00:00:00 2001 From: kjgbot Date: Tue, 8 Sep 2026 07:56:57 +0200 Subject: [PATCH 5/5] fix(review-gate): require the exact `### Blockers` heading level cubic P2 on #229, on the guard I added an hour earlier. `blockers_section_present` matched `^#+`, so a review headed `# Blockers` or `#### Blockers` satisfied the missing-section guard and could still pass. A review at the wrong heading level has not followed the output contract the prompt states. Deliberately stricter than `blockers_are_listed` and `blockers_say_none`, which keep matching `^#+`. That asymmetry is the point: this function decides whether a section COUNTS, so it must fail closed on a wrong level, while those two only DETECT blockers, where being permissive also fails closed. Verified: `###` + None passes; `#`, `##`, `####` and no section are all NO_VERDICT; `###` + a listed blocker is still CONTRADICTION. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR --- ops/preswarm-check/lens-runner.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/ops/preswarm-check/lens-runner.sh b/ops/preswarm-check/lens-runner.sh index 5d3c31af8..4959cd553 100755 --- a/ops/preswarm-check/lens-runner.sh +++ b/ops/preswarm-check/lens-runner.sh @@ -280,7 +280,16 @@ blockers_are_listed() { # here argued absence should "keep its previous behaviour instead of newly # failing". That was protecting a case the prompt already forbids. blockers_section_present() { - printf '%s\n' "$OUTPUT" | grep -qE '^#+[[:space:]]*Blockers[[:space:]]*$' + # Exactly `### Blockers`, the level the prompt specifies -- not `^#+`. A + # review headed `# Blockers` or `#### Blockers` has not followed the output + # contract, and accepting it here would let the missing-section guard admit + # an invalid section as a valid one (flows#229, cubic P2). + # + # Deliberately stricter than blockers_are_listed and blockers_say_none, which + # keep matching `^#+`: this decides whether a section COUNTS, so it fails + # closed on a wrong level, while those two only DETECT blockers, where being + # permissive also fails closed. + printf '%s\n' "$OUTPUT" | grep -qE '^###[[:space:]]*Blockers[[:space:]]*$' } blockers_say_none() {