The review gate asks whether the code was reviewed, not whether Sourcery answered - #385
Conversation
Sourcery answered A weekly quota lasts a week, and a job that stops a week of merges is a job somebody deletes -- which puts it back where it started. So a wf-review report by the author satisfies the gate: that is the review this repo actually runs when the bot is out. An author's plain approval does not satisfy it. "lgtm" from the person who wrote the code is the thing being guarded against, not a way past it, so a self-review is recognised by wf-review's provenance line and merging on one is recorded as a notice. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two independent axes, fresh context that did not see this code written. Fixed point git diff $(git merge-base origin/main HEAD)...HEAD at ca3b115.
A note on the quoting in this report. I have deliberately broken both refusal strings — writing you have reached your weekly rate + limit, and larger than the review + limit — rather than quoting them whole. That is not style. Finding Spec-1 below is that a wf-review report which quotes either string verbatim is discarded by scripts/review_verdict.sh as a refusal, and this report is a wf-review report on the review guard, so quoting them properly would have made this review invisible to the very gate it is reviewing. That is the finding, demonstrated by having to write around it.
Standards
Judged against the repo's own documented conventions (test_bench_workflow.py, test_public_api_snapshots_doc.py, test_readme_cli_doc.py, CLAUDE.md) and the Fowler baseline. Tooling-enforced matters skipped.
Std-1 — test_the_gate_requires_the_job fails with a reason no reader can act on. test/test_review_guard.py:165:
needs = next(line for line in ci.splitlines() if line.strip().startswith("needs: [ci,"))Rewrite gate's needs as a YAML block list — same semantics, valid YAML, the ordinary result of adding a job and letting a formatter wrap the line — and the guard raises a bare StopIteration. Proven:
E StopIteration
test/test_review_guard.py:165: StopIteration
1 failed, 12 passed
No message, no mention of gate, needs or review. The repo's model for exactly this pattern is test_bench_workflow.py::cold_reset, which does assert len(lines) == 1, "the cold-recreate shape is the only one that resets between runs". And the failure mode is the one this PR's own body quotes from test_readme_cli_doc.py: "a guard that failed for a reason no reader could act on — which is how guards get deleted." Fix: matches = [l for l in ci.splitlines() if l.strip().startswith("needs:")]; assert matches, "gate must declare a needs list", then assert on the joined text.
Std-2 — Duplicated Code: a second, weaker idiom for reading ci.yml. test_public_api_snapshots_doc.py already carries ci_job(), ci_step_script() and run_the_ci_check() — a job/step splitter that extracts a step's shell and executes it in a fake checkout against a stub. test_review_guard.py reimplements the "read ci.yml" job as two raw substring scans and reuses none of it. The consequence is not aesthetic: the entire run: block goes untested — the EVENT != pull_request early exit, the $OVERRIDE_LABEL path, the polling loop, and the PR_AUTHOR: ${{ github.event.pull_request.user.login }} wiring. That wiring is the input the whole classification pivots on (see Spec-3), and nothing anywhere asserts it is set. Also assert JOB in needs is substring matching: renaming the job to review-external and updating needs to match would keep JOB = "review" passing.
Std-3 — dead code that reads as a safety guard. scripts/review_verdict.sh:59:
for i in $(seq 0 $((count - 1))); do
[ "$count" -eq 0 ] && breakWhen count is 0, seq 0 -1 prints nothing and the body never runs, so the break can never execute. It is unreachable, and it advertises a hazard that has already been handled by the loop header — a reader who removes it cannot tell whether they broke something. Fix: delete the line; the zero case is already correct (verified: empty stdin → Nothing reviewed, rc=1).
Std-4 — Mysterious Name: the rename stops halfway. The PR's thesis is that the question is "was this reviewed", not "did Sourcery answer". The job, the script and the test all follow. What a user actually touches does not: the escape hatch is still no-external-review, and the primary failure message still opens ::error::The external reviewer refused (quota or diff size)… — which is now the less common way to fail the job, since the ordinary way is a PR that nobody reviewed at all. The two vocabularies now disagree in the same file. If the label must stay for compatibility, say so in a comment; the ::error:: text has no such excuse.
Std-5 — the CHANGELOG ships two contradictory entries for one guard. CHANGELOG.md:12 and CHANGELOG.md:25 are both under ## [Unreleased] / ### Added. The second describes "The new external-review job", a job that no longer exists in the tree, and states the escape hatch is the label only — which this PR just made false. Neither has shipped, so there is no history to preserve. Fix: replace the #378 entry rather than stacking on it.
Spec
No spec available — there is no ticket, no Closes #n, and no linked spec file. This axis therefore judges the diff against the PR body's own stated intent, quoting it.
Spec-1 (blocking) — a wf-review report that quotes the refusal is not counted as a review.
"a
wf-reviewreport by the author, recognised by the provenance line those reports open with"
scripts/review_verdict.sh:62-66 tests is_refusal before it looks at the author or the provenance line, and continues. So the check is not "is this the reviewer refusing" but "does this text contain the refusal sentence anywhere". Proven:
$ PR_AUTHOR=blooop bash scripts/review_verdict.sh <<'J'
[{"author":"blooop","body":"> *This was generated by AI during review.*\n\nThe bot said: Sorry @blooop, you have reached your weekly rate LIMIT of 500000 diff characters.\n\n## Verdict\nApprove"}] <-- (LIMIT capitalised only so this report itself survives the gate; the real body has it lowercase)
J
::error::The external reviewer refused (quota or diff size) and there is
no self-review to stand in for it. …
rc=1
A complete, correctly-marked wf-review report is read as Sourcery's refusal and thrown away. This is not a hypothetical body: the reviews most likely to quote that sentence are reviews of this guard, which is the class of PR the guard is being merged on. It cost me the quoting workaround at the top of this report to avoid it.
The same swallowing applies to a second party: a human writing "Sourcery only said you have reached your weekly rate + limit, but I read the diff — fine by me" is dropped, and the PR fails as unreviewed.
Fix, verified locally — move the author/provenance branch above is_refusal:
if [ -n "$AUTHOR" ] && [ "$author" = "$AUTHOR" ]; then
case $body in
*"$PROVENANCE"*) self_review=$((self_review + 1)); continue ;;
esac
fi
if is_refusal "$body"; then
refusals=$((refusals + 1)); continue
fiWith that applied the case above exits 0 with the ::notice::, and all 13 existing tests still pass — which is itself the proof that the current ordering is covered by nothing. Tighter still, and worth doing as well: anchor is_refusal to the real bodies, which all begin Sorry @ (verified against PR #384's live review), so the match is Sorry @*your weekly rate… rather than a floating substring.
Spec-2 (blocking) — the mutation claim does not hold; the provenance marker is unpinned.
"Three mutations, each failing exactly the tests that name it. … So both halves of the rule are load-bearing in both directions."
Four mutations the PR did not try, each surviving the full suite:
Mutation to scripts/review_verdict.sh |
Result |
|---|---|
PROVENANCE="generated by AI during" → PROVENANCE="AI" |
13 passed |
Delete the blank-body skip ([ -z "${body//[[:space:]]/}" ] && continue) |
13 passed |
Delete the [ -n "$AUTHOR" ] && guard |
13 passed |
set -euo pipefail → set +e |
13 passed |
| The Spec-1 fix above (a behaviour change) | 13 passed |
The first is the serious one. The tests pin only that some substring check exists, never its specificity — so under the mutant, an author review reading Reviewed with AI help, lgtm passes:
::notice::No second-party review: merging on 1 self-review(s). …
rc=0
The whole PR rests on the claim that the provenance line distinguishes a two-axis report from an author's approval. That distinction is exactly what no test asserts. Fix: add a test that a plausible near-miss fails — e.g. verdict([(AUTHOR, "Reviewed with AI assistance. Standards: clean. Verdict: approve.")]) must return 1 — which kills the mutant and pins the marker to the wf-review artefact rather than to two letters.
Spec-3 (non-blocking) — empty PR_AUTHOR fails open, and it is the one input nothing checks.
"An author's plain approval does not satisfy it."
scripts/review_verdict.sh:73 reads if [ -n "$AUTHOR" ] && [ "$author" = "$AUTHOR" ]. With PR_AUTHOR empty or unset, every review — including the author's own — falls to the else branch and is counted as second-party:
$ PR_AUTHOR="" bash scripts/review_verdict.sh <<<'[{"author":"blooop","body":"lgtm, merging"}]'
reviewed by 1 review(s) from somebody other than the author
rc=0
I traced the workflow and this is not reachable today: the step exits 0 unless EVENT = pull_request, and on a pull_request event github.event.pull_request.user.login is always populated (including for a fork PR and for dependabot[bot]). But the script's own header documents $PR_AUTHOR as its interface, the failure direction is open rather than closed, nothing tests it (see the third row of Spec-2), and per test_bench_workflow.py this repo treats "a property of a YAML file, so it is one edit away from being lost" as a thing worth guarding. Fix: two lines — [ -n "$AUTHOR" ] || { echo "::error::PR_AUTHOR is empty; refusing to classify."; exit 1; } — plus the test.
Spec-4 (non-blocking) — review state never reaches the classifier.
"a review by anyone other than the author — a person, or the bot when it is actually answering"
The workflow's jq maps only {author: .user.login, body: .body}, so state is discarded. Two consequences, in opposite directions:
- A second party's
CHANGES_REQUESTED— literally "this is broken, do not merge" — satisfies the gate (rc=0, verified). The ruleset'spull_requestrule hasrequired_approving_review_count: 0, so nothing else blocks that merge either.DISMISSEDreviews are returned by the same endpoint and count too. - A second party's bare Approve with no comment produces an empty body, which line 67 skips, so a real human review is rejected (
::error::Nothing reviewed this pull request, rc=1).
Both are defensible under a literal reading of "was this reviewed", but neither is stated anywhere and neither is tested. At minimum: include state in the jq map and let a CHANGES_REQUESTED/empty-body approval be a deliberate, documented decision.
Spec-5 (non-blocking) — the documented happy path cannot make CI green on its own.
"
reviewwill fail here until awf-reviewreport is posted on it — which is the intended path"
on: is push (main), pull_request, workflow_dispatch. There is no pull_request_review: trigger, so submitting a review fires nothing, and the poll breaks on the first review of any kind — a Sourcery refusal, which lands within a minute. So the sequence the README prescribes always ends with a red review job that only a manual re-run or an empty push clears. Neither the README section nor the ::error:: text says so. Fix: append "…then re-run this job" to both ::error:: messages, and consider adding pull_request_review: to on: so the gate re-evaluates itself.
Spec-6 (non-blocking, inherited from #378) — is_refusal is a two-string denylist over a vendor's prose.
"So the job refuses three things: a refusal on quota, a refusal on size, and no review at all" (README)
Any third refusal wording — a monthly cap, a service outage, a plan change — is not in the case, so it reads as a review that found nothing and the gate passes. That is the exact shape of the incident this guard exists to prevent, one string later. An allowlist is available and tighter: every real Sourcery review in this repo begins Hey - I've (the test file's own CLEAN and FINDINGS constants show it), so for that one login the rule could be "a review body that does not start Hey - I've is not a review". Not introduced by this PR, but this PR rewrites the file and re-commits to the denylist.
What I checked and could not break
- The rename is safe, as claimed.
gh api repos/blooop/devlaunch/rulesets/11900734→required_status_checks: [{context: "gate"}, {context: "prek"}].reviewis ingate'sneeds. Confirmed. - No stale references. Nothing in the tree mentions
external_review_verdict.sh,test_external_review_guard.pyor theexternal-reviewjob, except the deliberately-unchanged label name and the stale CHANGELOG entry at Std-5. - No shell injection. A body of
$(touch /tmp/PWNED) \id` "quoted" \backslash\ glob [a-z]is classified normally and executes nothing.case $body inand the${var//…}` expansions are safe. - Malformed input fails closed. Invalid JSON and a JSON object both exit non-zero via
jq+set -e(rc=5); empty stdin normalises to[]and exits 1. Angh apifailure in the workflow becomes[], i.e. "nothing reviewed", i.e. fail-closed. Good. - The zero-review loop is correct.
seq 0 -1is empty, so the body never runs (the redundant in-loop guard is Std-3). - Scale. A 1 MB review body classifies in 0.33 s. Two
jqprocesses per review is fine at this size. - No other bot posts reviews here. Across the last 40 PRs the only review authors are
sourcery-ai[bot]andblooop; codecov and GitGuardian post checks, not reviews, and no workflow in.github/workflows/holdspull-requests: write. The "another bot satisfies the gate" attack has no live vector today. - 13/13 tests pass on the branch as pushed; every other CI job is green.
Verdict
Request changes.
Blocking:
- Spec-1 —
is_refusalruns before the author/provenance check, so awf-reviewreport quoting either refusal string is discarded and the PR fails as unreviewed. This is the PR's headline feature failing on the class of PR most likely to use it, and this review had to be written around it. Fix is a five-line reordering, verified. - Spec-2 —
PROVENANCE="AI"survives all 13 tests, so the provenance marker's specificity — the distinction the entire change rests on — is pinned by nothing. Three further mutations survive. Add the near-miss test.
Non-blocking but worth taking: Std-1 (the StopIteration guard), Std-2 (reuse the existing ci_job/ci_step_script helpers and test the run: block), Std-3, Std-4, Std-5, Spec-3, Spec-4, Spec-5, Spec-6.
wf-review on #385 found it: is_refusal ran before the author check, so a report that quotes a Sourcery refusal was classified as one and dropped. The reviewer had to misspell both strings to get its own report through -- a false negative on the feature this PR adds. The author + provenance line is now a positive identification made first; nothing in the rest of the body can undo it. is_refusal is also anchored to the `Sorry @` both refusals open with. Also from that review: PR_AUTHOR empty now fails closed, review state is carried so a bare Approve counts and a DISMISSED does not, and the four mutants that survived the first suite are now caught. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Review findings addressedBoth blocking findings were real. Pushed as a second commit. B1 — a review that quotes a refusal was eaten by it
The author + provenance line is now a positive identification made first, with Verified with a report quoting both refusals verbatim: B2 — "load-bearing in both directions" was overstatedIt was, and the review is right to call it. All four are now caught:
Non-blocking, also taken
Not taken, with reasons
22 tests pass. |
Warned rather than failed: a review of an earlier commit did happen, which is not the absence this job catches, and failing it would mean re-reviewing after every typo fix. What it must not do is pass in silence -- which is what it did on this very pull request, where the report satisfying the gate had reviewed the commit before. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideThe PR changes CI from requiring Sourcery to answer to requiring evidence that the pull request was reviewed, with support for second-party reviews, verified author wf-review reports, and an explicit unreviewed label; it centralizes the classification logic in a tested script and updates documentation accordingly. Sequence diagram for the review gate verdictsequenceDiagram
participant CI as review job
participant GitHub as GitHub API
participant Verdict as review_verdict.sh
participant Gate as gate
CI->>GitHub: Fetch pull request labels and reviews
GitHub-->>CI: Labels and review records
CI->>Verdict: Evaluate reviews with PR_AUTHOR and PR_HEAD_SHA
alt no-external-review label
Verdict-->>CI: Pass and emit notice
else second-party review
Verdict-->>CI: Pass and warn if review is stale
else author wf-review report
Verdict-->>CI: Pass with self-review notice
else author approval, refusals, or no review
Verdict-->>CI: Fail with actionable error
end
CI->>Gate: Supply review result through needs
Flow diagram for review evidence classificationflowchart TD
Start["Pull request reviews"] --> Label{"no-external-review label?"}
Label -->|yes| Pass["Pass: merge unreviewed"]
Label -->|no| SecondParty{"Valid review by someone other than author?"}
SecondParty -->|yes| Pass2["Pass; warn if review predates head"]
SecondParty -->|no| AuthorReport{"Author wf-review provenance line?"}
AuthorReport -->|yes| Pass3["Pass with self-review notice"]
AuthorReport -->|no| Fail["Fail: post wf-review or use label"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Re-review at a moved head. The earlier report on this pull request was written against ca3b115; the head is now b3f8ca3. This covers the delta — 961fc86 "A review of this guard must not be eaten by the strings it quotes" and b3f8ca3 "Say so when every review predates the head" — plus whether the two blocking findings are genuinely resolved. Fresh context, did not see the earlier review written. Fixed point git diff ca3b115..b3f8ca3 for the delta, git diff $(git merge-base origin/main HEAD)...HEAD for the whole. Everything below was produced by running the script and mutating it, not by reading the diff. 26/26 tests pass on the branch as pushed and every check is green.
(This report quotes both refusal sentences verbatim and in full. That it is legible to the gate at all is the evidence for B1 below — the previous reviewer had to misspell them.)
Are the two prior blocking findings resolved?
B1 — is_refusal ran before the author/provenance check. Behaviourally resolved. Yes.
$ PR_AUTHOR=blooop bash scripts/review_verdict.sh # author report quoting BOTH refusals verbatim
::notice::No second-party review: merging on 1 self-review(s). … rc=0
$ PR_AUTHOR=blooop bash scripts/review_verdict.sh # human: "Sourcery said: Sorry @blooop, you have
# reached your weekly rate limit of 500000 diff
# characters. — I read it instead."
reviewed by 1 review(s) from somebody other than the author rc=0
The false negative is gone. But the fix that carries it is the Sorry @ anchor, not the reordering — and the anchor is what Finding 1 below is about.
B2 — the suite did not pin the rule. Three of the four claimed mutants are genuinely caught. The fourth is not.
I re-ran all four against the 26 tests:
| Mutant | Claimed | Measured |
|---|---|---|
PROVENANCE="generated by AI during" → "AI" |
1 failed | 1 failed ✅ |
| drop the blank-body skip (second party counts unconditionally) | 1 failed | 1 failed ✅ |
drop the [ -n "$AUTHOR" ] guard |
1 failed | 1 failed ✅ (see Finding 5 — a different guard catches it) |
is_refusal back above the provenance branch |
2 failed | 26 passed ❌ |
The last row measures a revert of 961fc86 as a whole — anchor and ordering together. Isolated, the anchor accounts for both failures and the ordering accounts for neither:
anchor removed only → 1 failed (test_a_human_review_that_quotes_a_refusal_still_counts)
ordering reverted only → 26 passed
anchor + ordering reverted → 2 failed
So the headline change of 961fc86 is load-bearing (rc flips on a real body) and pinned by nothing. The claim "load-bearing in both directions" is overstated a second time, in the same shape the first review called out.
Standards
Std-1 (non-blocking) — a fix that was reported as made was not made. The response comment says the bare StopIteration was "replaced with a comprehension plus assert len(...) == 1 and a message". test/test_review_guard.py:185 is byte-identical to ca3b115:
needs = next(line for line in ci.splitlines() if line.strip().startswith("needs: [ci,"))Rewriting gate's needs as a YAML block list still gives:
E StopIteration
test/test_review_guard.py:185: StopIteration
Std-2 (non-blocking) — dead code that reads as a safety guard, reintroduced. scripts/review_verdict.sh:89 still tests [ -n "$AUTHOR" ] &&, but line 44 now exits 1 when AUTHOR is empty, so the in-loop half can never be false. Deleting it: 26 passed. This is the same smell the first review got removed at the old [ "$count" -eq 0 ] && break — a reader who deletes it cannot tell whether they broke something. Fix: delete it; the assertion at the top is the guard.
Std-3 (non-blocking) — the new state rules are not in the README. README says "three things satisfy it" and stops. It does not say that a DISMISSED review is skipped, that a bare APPROVED counts, or — the one that will surprise somebody — that a reviewer who leaves only inline comments produces an empty COMMENTED body and fails the gate. That is a deliberate, defensible choice, and the person it bites will have no way to learn it except by reading the script.
Spec
No spec available — no ticket, no Closes #n, no linked spec file. This axis judges the delta against the two commit messages and the response comment, quoting them.
Finding 1 (blocking) — the Sorry @ anchor makes the refusal denylist fail open, and the pre-delta code did not
961fc86: "Anchored to theSorry @the bot opens both with, not matched loose."
case prefix matching is exact from byte zero. Any leading character defeats it, and the body is then classified as a review that found nothing — which is precisely the shape of the incident that produced twenty-six unreviewed merges.
rc=1 verbatim refusal
rc=0 refusal with a leading newline <-- gate passes on a refusal
rc=0 refusal with a leading space
rc=0 refusal with a UTF-8 BOM
rc=0 refusal prefixed with a heading
rc=0 "Sorry, @blooop, you have reached your weekly rate limit of 500000 diff characters."
All six are the same refusal. Before 961fc86 the loose *"you have reached your weekly rate limit"* caught every one of them. The delta traded a false negative for a false positive on the guard's primary property, and nothing tests either direction.
What makes the trade a bad one is that the anchor is not what fixes B1 for the case B1 was about. With the ordering fix in place and the anchor removed, the author's own report quoting both refusals still passes — the ordering handles it; only test_a_human_review_that_quotes_a_refusal_still_counts regresses. And that human case is still broken anyway when the quote leads the body:
$ PR_AUTHOR=blooop bash scripts/review_verdict.sh # human review OPENING with the refusal, then
# "That's all Sourcery said. I read the diff myself: fine."
::error::The external reviewer refused (quota or diff size) … rc=1
So the anchor buys a partial fix to a rare second-party case and pays for it by letting a whitespace-prefixed refusal through.
Fix — tolerate leading whitespace, and let the reviewer's identity rather than the body's first byte carry the anchoring:
is_refusal() {
local b=${1#"${1%%[![:space:]]*}"} # drop leading whitespace
case $b in
"Sorry"*"you have reached your weekly rate limit"*) return 0 ;;
"Sorry"*"larger than the review limit"*) return 0 ;;
esac
return 1
}with tests for a leading newline, a leading space, and a second-party review that opens with the quote. (Scoping the call to [ "$author" = sourcery-ai[bot] ] would close the second-party case entirely and is worth considering.)
Finding 2 (non-blocking) — half of b3f8ca3's feature is pinned by nothing
b3f8ca3: "Say so when every review predates the head."
The feature works — verified live on this pull request's own run (32733980540, job 97457533882):
##[notice]No second-party review: merging on 1 self-review(s). …
##[warning]Every review here predates the current head (b3f8ca3b0eb…). …
exit 0, gate green — so it warns and cannot fail, as intended. Absent PR_HEAD_SHA claims nothing, and github.event.pull_request.head.sha is the right SHA to compare commit_id against (the branch head, not the merge commit). All confirmed.
What is not confirmed by any test is the second-party path. Every staleness test drives the self_review branch or a case where at_head > 0, so:
delete `stale_notice` from the second_party branch → 26 passed
A human reviews commit A, two commits land, the gate passes in silence. That is this pull request's own situation with the reviewer changed, and it is the thing b3f8ca3 exists to stop.
Finding 3 (non-blocking) — the delta's entire workflow wiring is untested
PR_HEAD_SHA and state/commit_id in the jq map are new inputs the whole delta pivots on. Both are one YAML edit from being lost, and the tests read ci.yml only for two substring assertions:
delete `PR_HEAD_SHA:` from ci.yml → 26 passed (staleness silently dies)
revert the jq map to `{author, body}` → 26 passed
The second is the worse one and it is bidirectional: state goes empty, so every DISMISSED review starts counting and every bare APPROVED starts failing the gate — a live regression in both directions, green suite. This is the first review's Std-2 ("the entire run: block goes untested"), deferred as separate cleanup — a fair call then, but the delta just moved two load-bearing inputs into the untested region.
Finding 4 (non-blocking) — the near-miss set does not pin PROVENANCE as tightly as it reads
Response: "walks four near-misses"
It kills "AI". It does not kill "by AI":
PROVENANCE="generated by AI during" → "by AI" → 26 passed
under which the author's Reviewed by AI, lgtm satisfies the gate — the same fail-open, one whittling step further along. Fix: add "Reviewed by AI, lgtm" and "Generated by AI." to the loop at test/test_review_guard.py:235.
Finding 5 (non-blocking) — three more surviving mutants, all in the new code
move the DISMISSED skip below the author branch → 26 passed (a dismissed self-review counts)
increment at_head for refusals → 26 passed
increment at_head for author reviews w/o provenance → 26 passed
set -euo pipefail → set +e → 26 passed (unchanged from the first review)
The refusal one is not academic: Sourcery posts a refusal at the current head on every push, so that mutation would suppress the staleness warning permanently while leaving the suite green.
Finding 6 (non-blocking) — no PENDING guard
A PENDING review is an unsubmitted draft. The script treats it as submitted:
rc=0 PENDING, author, provenance line → counted as a self-review
rc=0 PENDING, second party, prose → counted as a second party
Not reachable today — GH_TOKEN: ${{ github.token }} is github-actions[bot], and the API returns PENDING reviews only to their own author, so the draft can never be the actor's. But the script documents $PR_AUTHOR/stdin as its interface and is tested standalone; one token swap to a PAT owned by the author and an unsubmitted draft satisfies the gate. One line: skip PENDING alongside DISMISSED.
What I checked in the delta and could not break
- Every path through the reordered loop, traced against the script: author+provenance → self-review (
rc=0); author without provenance → skipped, gate unsatisfied (rc=1); second party refusal → refusal; second party prose → counted; second party emptyAPPROVED/CHANGES_REQUESTED→ counted; emptyCOMMENTED(and whitespace-only) → not counted;DISMISSED→ skipped in both arms;stateabsent from the JSON → treated asCOMMENTED, body decides. No second-party review is skipped by the doublecontinue— the author arms are entered only on[ "$author" = "$AUTHOR" ]. A co-authored PR is sane: a co-author is notpull_request.user.login, so their review counts as a second party; the author as sole reviewer still needs the provenance line. PR_AUTHORempty → exit 1 cannot fire spuriously. The step early-exits unlessEVENT = pull_request, and on that eventgithub.event.pull_request.user.loginis always populated, fork PRs and[bot]authors included. A[bot]login round-trips correctly (rc=0fordependabot[bot]self-review).- Staleness cannot become a failure.
stale_noticeonly echoes and always returns 0; both passing paths call it;at_headis incremented on both. NoHEAD_SHA→ returns before claiming anything. - The real API shape matches the jq map.
gh api repos/blooop/devlaunch/pulls/385/reviewsreturnsstateandcommit_idon every element;.bodymay benulland// ""handles it. Every real Sourcery refusal across #369, #370, #376, #382, #383, #384 and this PR does beginSorry @— the anchor is right about today's data, which is not the same as safe (Finding 1). - No shell injection, including through the two new fields:
$(touch …)and backticks inbody,stateandcommitall classify normally and execute nothing. - Malformed input still fails closed. Invalid JSON, a bare object, and an array of strings all exit 5 via
jq+set -e;nulland[]exit 1; an empty stdin normalises to[]. Agh apifailure becomes[], i.e. "nothing reviewed". - Scale. A 1 MB body classifies in 0.32 s; 60 reviews in 0.90 s, at four
jqprocesses per review (up from two). Fine at the twelve-review scale this sees. - Docs. The stacked
[Unreleased]entry is genuinely collapsed to one, and both::error::texts genuinely say "and re-run this job". Confirmed.
Verdict
Comment. The two prior blocking findings are resolved in behaviour — verified by execution, and this report's own survival is the demonstration for the first.
One new blocking finding, introduced by the delta:
- Finding 1 —
scripts/review_verdict.sh:61-67: theSorry @anchor discards the refusal denylist for any body with a leading newline, space, BOM or heading, so a quota refusal passes the gate as a review. The pre-delta code caught all six variants. The anchor is not what fixes B1 for the author's own report (the reordering is), and the second-party case it exists for is still broken when the quote leads the body. Strip leading whitespace and loosen the prefix, and add tests in both directions.
The rest is non-blocking: one reported fix that was not made (Std-1), dead code reintroduced (Std-2), undocumented state rules (Std-3), and a set of surviving mutants concentrated in exactly the code the two new commits added — the reordering itself, the second-party staleness warning, the whole workflow wiring, PROVENANCE="by AI", and three at_head variants. None of those change today's behaviour; all of them mean the delta is held in place by nothing.
The re-review caught a regression I introduced: anchoring is_refusal to `Sorry @` made it strictly weaker than the loose match it replaced -- a refusal with a leading newline, space or heading passed the gate as a real review. Sniffing prose has now been wrong in both directions, so it asks about the reviewer instead. That also fixes the case broken under both spellings: a human quoting a refusal is reviewing, not refusing. Also from that review: PENDING drafts no longer count, the dead `[ -n "$AUTHOR" ] &&` is gone, the StopIteration fix I claimed last time is actually applied now, and seven behaviours that no test touched are pinned -- staleness on the second-party path, both wiring fields, `by AI`, at_head for refusals. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Re-review findings addressed — including a regression I introducedThe blocking finding is correct and it was my regression, not a pre-existing one. Pushed as B1 — the
|
| refusal body | before |
|---|---|
| plain | caught |
| leading newline | passed the gate as a real review |
| leading space | passed as a real review |
## Review\n\n first |
passed as a real review |
The loose match it replaced caught all four. I made the guard's primary property worse while claiming to strengthen it — and the review is right that the anchor was never what fixed the original problem; the reordering was.
Sniffing the prose has now been wrong in both directions: loose, it ate a wf-review report that quoted the refusal; anchored, it stopped recognising refusals. So it no longer asks about the text — only the reviewer can refuse on its own behalf, and REFUSING_LOGINS is the question. That also fixes the case broken under both previous spellings: a human quoting a refusal is reviewing, not refusing.
| case | now |
|---|---|
| bot refusal, all four leading variants | caught |
| human quoting a refusal mid-body | counts as a review |
| human review that opens with a quoted refusal | counts as a review |
The cost, stated in the script and the README: a new external reviewer's refusals go unrecognised until its login is added. That is a config change with a test naming it, rather than a regex quietly deciding what a refusal looks like.
B2 — the ordering mutant, and an honest correction
The review is right that reverting the ordering alone left the suite green, and it still does — but not because of a test gap. Once is_refusal became a question about the reviewer, an author's report can never be classified as a refusal whatever the order. The ordering stopped being load-bearing; that is the earlier fix becoming redundant.
I have not written a test to pin an ordering that no longer decides anything. The script's comment and the test's docstring now say plainly that it is kept because "is this the author's own report" is the clearer thing to ask first, and that reverting it breaks nothing.
Six of the seven other mutants now caught
| Mutant | Before | Now |
|---|---|---|
drop stale_notice from the second-party path |
survived | 1 failed |
delete PR_HEAD_SHA: from the workflow |
survived | 1 failed |
revert the jq map to {author, body} |
survived | 1 failed |
PROVENANCE → "by AI" |
survived | 1 failed |
at_head counted for refusals |
survived | 1 failed |
PENDING no longer skipped |
survived | 1 failed |
| revert the ordering alone | survived | still survives — see above |
Also taken
PENDINGdrafts no longer count. A draft only its author can see is not a review.- The dead
[ -n "$AUTHOR" ] &&is gone — an emptyAUTHORexits before the loop. Exactly the "dead code that reads as a guard" smell the first review had me remove elsewhere. - The
StopIterationfix I claimed last round was never applied. The review caught that too. My edit had missed because the formatter had already collapsed the line to one line and my match was against the two-line form — so the "fix" silently did nothing while I reported it as done. It is applied now, with theassert len(needs) == 1message.
33 tests, up from 26. pixi run style 10.00/10, full suite unchanged against pristine main.
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Third wf-review pass, scoped to 435864c only — the fix for the regression the second pass blocked on. Fresh context; the earlier passes' cleared findings were not re-opened. Everything below was run against the checked-out branch (scripts/review_verdict.sh on stdin, test/test_review_guard.py, and a mutation sweep of 23 hand-written mutants).
The regression is fixed. All five shapes the second pass named now block correctly:
body from sourcery-ai[bot] |
exit |
|---|---|
| bare refusal | 1 |
leading \n |
1 |
| leading space | 1 |
## Heading\n\n before it |
1 |
Note: before it |
1 |
and a human quoting the same text exits 0, which neither previous spelling got right. The StopIteration fix landed for real this time: reformatting needs: [ci, ...] as a YAML block list now fails test_the_gate_requires_the_job with AssertionError: ... Found 0, where 961fc86's next(...) raised StopIteration. PENDING is not dead code — I created a pending review on this PR and GET /pulls/385/reviews returned it with state: PENDING before I deleted it, so the skip guards a real input even if github.token can't see a human's draft.
Standards
1. for login in $REFUSING_LOGINS is glob-expanded, and every login it will ever hold is a glob. (scripts/review_verdict.sh:69) set -euo pipefail does not disable pathname expansion, and [bot] is a bracket expression matching one of b, o, t. Every GitHub App login carries that suffix, so the documented extension path hands you a glob every time. Reproduced end to end:
$ cd /empty && refusal | PR_AUTHOR=blooop review_verdict.sh # ::error:: ... refused rc=1
$ touch sourcery-aib && refusal | PR_AUTHOR=blooop review_verdict.sh
reviewed by 1 review(s) from somebody other than the author rc=0
One zero-byte file named sourcery-aib, sourcery-aio or sourcery-ait at the repo root turns a quota refusal back into a passing review — silently, which is the 26-PR incident's exact signature. Not reachable today (no such file exists, and nullglob is off so the literal survives when nothing matches), so this is latent rather than live, and I am not blocking on it. But the guard's behaviour should not depend on the contents of its working directory, and shellcheck does not flag it: it accepts intentional word-splitting in for and says nothing about the glob, so nothing catches this for you. (Aside: shellcheck is pinned in pyproject.toml but wired into no task or job, so it doesn't run at all.)
Fix, splits without globbing, suite stays green and the decoy file above stops mattering:
local logins=()
read -ra logins <<<"$REFUSING_LOGINS"
for login in "${logins[@]}"; do
[ "$author" = "$login" ] && matched=0
done2. "That is a config change with a test naming it" is not true. (scripts/review_verdict.sh:63-65, README.md:2399-2401) No test names REFUSING_LOGINS. Renaming it to REFUSAL_AUTHORS throughout the script leaves all 33 green. The default value is pinned indirectly through the BOT constant (a wrong default fails 6 tests), so the variable is not untested — but the override path the comment and README promise is, and that is the path where the word-splitting above lives. A one-line test setting REFUSING_LOGINS="a[bot] sourcery-ai[bot]" in the child env would pin the name, the split, and the multi-entry case at once.
3. The new hole is real but not reachable, and it fails silently when it does become reachable. A refusal from any login not in the list now counts as a genuine second-party review — verified for sourcery-ai, sourcery[bot], sourceryai[bot], coderabbitai[bot], all exit 0. The configured login is correct today: gh api repos/blooop/devlaunch/pulls/369/reviews returns exactly sourcery-ai[bot]. So the tradeoff is as documented. What is missing is detection. If Sourcery is ever renamed or migrated, the guard reverts to the failure this whole PR exists to fix, with no signal. Cheap hardening, in the spirit of the rest of the file: when a body matches a refusal string but the author is not in REFUSING_LOGINS, still count it as a second party, and emit ::warning:: naming the login. That converts the silent reversion into a visible one.
4. Mutant the 33 tests miss (not the loop-ordering one, which is documented and accepted): tightening PROVENANCE from the middle fragment to the whole literal line survives the suite.
PROVENANCE="generated by AI during" -> exit 0
PROVENANCE="This was generated by AI during review." -> exit 1
on a report opening > *This was generated by AI during review* (no trailing period). The comment at scripts/review_verdict.sh:31-33 says the middle is matched deliberately "so a change of emphasis or a trailing period does not silently stop counting" — that stated property has no test, and a future tightening would take it away invisibly. It fails in the safe direction (blocks a good PR rather than passing a bad one), so: non-blocking, but it is a design decision written down and left unpinned. Two lesser survivors, both fail-safe and both low value: exact author match relaxed to a prefix match, and the OVERRIDE_LABEL default changed away from no-external-review.
Spec
The spec for this commit is the second pass's blocking finding, and it is met. is_refusal is now a question about the reviewer, the five leading-whitespace/prose shapes block, the human-quoting-a-refusal case is fixed as a bonus, PENDING is skipped, the dead [ -n "$AUTHOR" ] && is gone (safe — the emptiness assert three lines above exits first), and seven new tests pin behaviour that previously had none. The mutation sweep confirms the new coverage bites: PENDING skip, both at_head paths, the second-party staleness notice, the refusal-at-head case and the wiring assertions are all killed by the suite when removed. Nothing in 435864c re-breaks what the earlier passes cleared — a second-party report quoting the refusal text still counts, DISMISSED still doesn't, and PR_AUTHOR empty still fails closed.
One seam worth a sentence, extended rather than introduced here: the workflow's poll breaks as soon as jq length > 0, which counts reviews the script then discards (DISMISSED, and now PENDING). The "did anything arrive" test and the "does anything count" test are different notions, so a discarded review can short-circuit the wait for a real one. Not reachable for PENDING under github.token, and pre-existing for DISMISSED.
Verdict
Comment — approve on the substance. The regression is fixed, the fix is the right shape, and this commit does not introduce a blocking one. No blocking findings.
Ordered by value if you pick any up: (1) read -ra for the login split, (2) a ::warning:: when a refusal string arrives from an unlisted login, (3) one test naming REFUSING_LOGINS and its multi-entry form.
Third review pass. Every login REFUSING_LOGINS will hold ends in [bot], and unquoted word-splitting hands that to globbing as a bracket expression matching one of b, o or t. A zero-byte file named sourcery-aib in the working directory turned a quota refusal into "reviewed by 1 review(s)", silently, which is the signature of the twenty-six this job exists to stop. Latent, not live. read -ra now. Also: an unlisted account posting a refusal is counted as a review but warned about, so a reviewer rename cannot put us back at the original incident with no signal. And REFUSING_LOGINS now has the test the README claimed it had. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Third pass addressedNo blocking findings, and the reviewer confirmed the regression is fixed. I took four of the five non-blocking items anyway, because the first one is better than its "latent" label suggests. The
|
| Mutant | Result |
|---|---|
unquoted for login in $REFUSING_LOGINS |
1 failed |
rename REFUSING_LOGINS |
1 failed |
| remove the unlisted-login warning | 1 failed |
tighten PROVENANCE to the whole line |
1 failed |
36 tests. pixi run style 10.00/10.
Not taken
The poll's "a review arrived" test still disagrees with the script's "a review counts" test, so a DISMISSED review can short-circuit the wait for a real one. Real, but it is a workflow-timing change rather than a verdict change, and it belongs with the pull_request_review: trigger I already deferred.
One thing worth its own issue
shellcheck is pinned in pyproject.toml and wired into no task and no CI job, so it never runs. It would not have caught this particular glob (it accepts word-splitting in a for), but a linter that is installed and never executed is the same shape as a reviewer that answers without reviewing. I will file it separately rather than expand this PR.
1 participant
#378 shipped a gate that fails when Sourcery refuses on quota. That is right about the problem and wrong about the remedy: the quota is weekly, so it blocks every merge for up to a week, and the only way through was a label that says "merge unreviewed" — which is the wrong thing to write down when a review did happen.
A gate that stops a week of merges is a gate somebody deletes. That is the failure mode this repo already documents for its other guards (
test_readme_cli_doc.py: "a guard that failed for a reason no reader could act on — which is how guards get deleted"), and deleting it would put us back exactly where the twenty-six unreviewed pull requests came from.What changes
The job is renamed
external-review→review, because the question is "was this reviewed", not "did Sourcery answer". Three things satisfy it:wf-reviewreport by the author, recognised by the provenance line those reports open with. That is the review this repo actually runs when the bot is out: two axes, in fresh context that did not see the code written;no-external-reviewlabel, unchanged, for merging with neither.An author's plain approval does not satisfy it. "lgtm" from the person who wrote the code is the thing being guarded against, not a way past it. A determined author can obviously type the provenance line by hand — the guard is against a review silently not happening, not against someone deciding to skip one, and that decision still has a label. Merging on a self-review emits a
::notice::, because it is worth seeing in the log afterwards.Renaming the job is safe: the branch ruleset requires
gateandprekonly (verified viagh api repos/blooop/devlaunch/rulesets), andreviewsits insidegate'sneeds. Nothing requires the old name, so nothing silently stops gating — which is the directiongate's own comment warns about.Why the provenance line, and not a length floor
A length floor punishes a concise human ("Checked the edge case, it's handled." is a real review) and is trivially satisfied by padding. The author/second-party split is the distinction that actually matters, and for the author's own review the provenance line marks the specific artefact this repo trusts — a fresh-context two-axis report — rather than measuring bytes.
Verified
13 tests, run against the real bodies: both refusals verbatim from those pull requests, the real "reviewed your changes and they look great" Sourcery posts when it found nothing, and the real
wf-reviewprovenance line from #331/#343/#348.Three mutations, each failing exactly the tests that name it:
cat >/dev/null; echo oktest_the_author_saying_lgtm_is_not_a_review,test_a_refusal_plus_the_authors_lgtm_is_still_not_a_reviewtest_a_self_review_stands_in_when_the_bot_is_out_of_quota,test_a_self_review_alone_passesSo both halves of the rule are load-bearing in both directions: the guard cannot pass on nothing, and it cannot pass on an author's bare approval.
pixi run -q python -m pytest test/ --ignore=test/e2e: 297 passed, 20 failed — byte-identical failures to pristinemainin a scratch clone (they need a builtdl).pixi run style10.00/10.yaml.safe_loadon the workflow clean. No stale references to the old script or test names anywhere in the tree.This pull request under its own rule
Sourcery is still out of quota, so
reviewwill fail here until awf-reviewreport is posted on it — which is the intended path, and makes this PR its own demonstration a second time. I am posting one rather than reaching for the label.🤖 Generated with Claude Code
Summary by Sourcery
Make the review gate determine whether a pull request was actually reviewed while allowing an explicit, visible path through bot outages.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests:
Chores: