Repository navigation
ci(review-gate): assert a human LGTM as well as an automated review - #298
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
The gate has only ever asked whether a machine reviewed the PR. The half that actually matters to the person merging - "have I read this one myself yet?" - was carried in his head, across a dozen open PRs, and it was getting dropped: "I always need to do a review myself and add a LGTM ... sometimes I forget or get confused. It would be much easier if there was just an action that highlighted that." So the gate now asserts two things, and names them separately when they fail: an automated review exists and finished (bin/check-review-posted.sh, unchanged), and the repository owner has submitted a review saying LGTM (bin/check-human-lgtm.sh, new). Both halves run on every gate run even when the other has already failed, so one look at the checks list says which is outstanding rather than only the first one to go red. WHAT COUNTS, AND WHY IT WAS DECIDED BY LOOKING The matching rule was settled against every LGTM this repo has actually received rather than against taste, because a memory aid that rejects the habit it is meant to remind you of is a nuisance people route around. All 18 of them, from #210 to #292, are the lower-case bare word `lgtm`, submitted as a COMMENTED review - not an approval, and never in the ordinary comment box. So: a submitted pull-request review, by astubbs, whose body contains `lgtm` as a whole word in any case, outside code fences and blockquotes, not followed by `?` and not preceded by a negation. Review state is not consulted, except that a PENDING review is ignored. All sixteen historical LGTM PRs pass the new checker against live API data; #287, #296 and #297 - reviewed, or not, but never stamped - fail it. Two of those clauses are load-bearing rather than fussy. An APPROVED review with an empty body does NOT count, because a silent Approve is the reflex this exists to interrupt. A DISMISSED review DOES, because the master ruleset dismisses approvals on push: had the rule consulted state it would have been head-sensitive through the back door, going green and then quietly red again after an unrelated typo fix. HEAD-INSENSITIVE, DELIBERATELY An LGTM on any commit of the PR counts for the whole PR, permanently. That matches the automated half, which abandoned freshness for reasons recorded in docs/inflight/parked-strict-review-gate-freshness.md, and it matches what was asked for: "it doesn't need to be on the latest head because I won't say LGTM until I'm confident that we're near the merge point anyway". A stamp that evaporates on the next push trains you to stop typing it. Two self-test cases assert this as a positive rather than as deleted code, so re-adding freshness breaks the suite. RENAME, AND WHY THERE ARE TEMPORARILY TWO JOBS `claude-review` no longer describes a check that also wants a human, so the job is now named `review: bot + human LGTM` - repo convention for a required check (`shell: sigpipe`, `deps: vulnerabilities`), with the two operands naming the two halves so a red entry in the checks list says which question is open before you open anything. The job name is a required status check matched by name, so the rename cannot be done in one step: drop the old name and the ruleset requires a context nothing produces, which blocks every open PR including this one. A `needs:`-based mirror was rejected outright - a job that skips when its dependency fails reports `skipped`, which GitHub counts as satisfying a required check, so the mirror would have gone green exactly when the gate went red. Instead a transitional job keeps reporting `claude-review` under the old contract, with duplicated steps and a stated deletion condition. The ruleset is swapped in one atomic PUT while both names report, and only then is the old job deleted; the commands are in the PR description. The gate stays read-only (`contents: read`, `pull-requests: read` - the reviews endpoint needs no new scope), bot-raised PRs stay exempt, and a fork PR still cannot satisfy it. A `pull_request_review` trigger, which would clear the human half the moment the LGTM lands, is deliberately NOT added and the reasoning is written into the workflow: on that event the sender is the reviewer, so a bot-submitted review would skip the job on its existing sender guard, and a skipped job satisfies a required check. That is this repo's most-repeated defect class pointed the wrong way. The LGTM therefore needs the same re-run the automated half already documents, unless it is left before the review is asked for. The self-test carries 42 cases, over half of them negative controls, including four that read the workflow file and assert it still feeds this checker the reviews endpoint with the marker the checker parses - a gate fed the wrong stream would report success without having asked the question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhF637Ywr7MiKkxaR11Up3
ac8ed48 to
e3f924d
Compare
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac8ed48b3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…correct the survey it was wrong about Review findings on this branch, all four verified by running before and after. CORRECTION TO THE PREVIOUS COMMIT ON THIS BRANCH. Its message says "18 owner LGTMs" in one place and "sixteen" in another. Both are wrong, and so was every copy of that claim in the tree. The real figure, re-derived from repos/astubbs/parallel-consumer/pulls/<n>/reviews over all 181 PRs, is 50 owner LGTM reviews across 38 PRs, from #63 to #292. That commit cannot be rewritten, so this one states the correction. 1. THE SELF-TEST COULD NOT SEE A DISABLED STEP, ONLY A DELETED ONE The four coupling cases were `grep -F` substring searches over the workflow TEXT, and a substring search cannot tell a step that RUNS from one that merely APPEARS. Measured on a scratch copy: `if: false` on the step, `|| true` on its pipeline, `; true`, `set +e`, `continue-on-error: true`, and commenting the entire step out ALL left the suite green at 42/42. One case - `grep -F 'bin/check-human-lgtm.sh'` - was satisfied by two header comments alone, so it stayed green with the step deleted outright. That is the failure class in docs/solutions/workflow-issues/a-check-that-reports-success-without-having-run.md occurring inside the guard written to prevent it, which is why it is worth more than its blast radius suggests. The coupling section now parses the step out of the workflow - comments dropped first, so a commented-out step reads as an absent one - and asserts on its structure: it exists, it reads the reviews endpoint, its marker matches, its `if:` is neither a never-true constant nor changed from the intended guard, nothing in it swallows the checker's exit status, and the checker invocation is its last command. All eight sabotages above now go red; the unsabotaged copy stays green. Deliberately awk rather than python3 + PyYAML, though PyYAML does import here. This suite runs as a step of a REQUIRED check, ahead of the gate it protects, so a dependency of it is a thing that can brick every open PR by being absent from a runner image. The indentation rules it needs are the only YAML involved. 2. THE EMPIRICAL CLAIM JUSTIFYING CASE-INSENSITIVITY WAS FALSE, IN FOUR PLACES Claimed: 18 LGTMs, #210 to #292, all the lower-case bare word. Actual: 50, across 38 PRs, #63 to #292. Forty-nine are `lgtm`; ONE, on #84, is `Lgtm`. Forty-six are the bare word alone on a line; four carry a trailing clause, one of which - #73's "lgtm, @claude how about you?" - ends in a question mark. The true data argues for the design harder than the false data did. `Lgtm` is a live counterexample proving case-insensitivity is load-bearing rather than merely generous: without it this repo has a stamp on record that the checker would call missing. #73 is a real passing body containing a `?`, which is why the `?` clause rejects only a `?` touching the token. Both are now self- test cases (8c, 8d). The one clause that WAS true and is load-bearing - all 50 are COMMENTED reviews, not approvals - is kept. The count also sat inside an assertion LABEL, so a passing test printed a false claim on every run. Labels now name the PR the body came from instead. 3. README DROPPED `claude-review` FROM "JOB NAMES ARE AN API" That list named only the new job, while `claude-review` is the context ruleset 15055005 actually requires throughout cutover steps 1-3. Someone reading it before performing step 5 could conclude the old name is no longer an API and delete the transitional job BEFORE the ruleset swap - the exact bricking the workflow header warns about, arrived at by following the docs. Restored, marked transitional, with the ordering spelled out. 4. THE CANONICAL CONTRACT WAS OWNED TWICE docs/ci.md declares itself canonical and says everything else links to it; this branch then added a seven-clause restatement of a rule bin/check-human-lgtm.sh heads "THE MATCHING RULE, IN FULL". Both cannot own it, and the drift was not hypothetical - the false survey above was pasted into four files and rotted in all four inside one PR. Split by altitude: docs/ci.md owns the GATE contract (what satisfies the gate), bin/check-human-lgtm.sh owns the MATCHING RULE (what satisfies the human half). The script wins the second because it is the executable truth - its prose sits beside the awk implementing it, and prose and code in one file cannot drift unnoticed. AGENTS.md's substantive clause, which sat two lines above the sentence forbidding exactly that, is reduced to rule-plus-pointer. FOUR CHECKER BUGS, EACH PINNED BY A CASE PROVEN TO FAIL WITHOUT THE FIX - A marker line bearing this run's token but a lost field was read as more of the PREVIOUS review's body, merging the next reviewer's words into the previous segment. A six-field marker for `mallory` after an `astubbs` segment reported "astubbs submitted a review containing LGTM". Not reachable from today's --jq, but one `; next` removes the forgery path. (21c) - scan() advanced PAST each token it examined, discarding the character the next candidate needs to see, so glued repeats walked through the whole-word guard: `LGTMLGTM` passed a rule under which neither half of it does, and `xLGTMLGTM` passed one that refuses `xLGTM`. (11b, 11c) - A trailing `\r` defeated the fence-close test, so a CRLF body never closed a fence and swallowed every LGTM after it. Not live - 0 of 365 owner bodies carry a CR - which is why it needed a case rather than a wait. (12f) - Three clauses survived deletion with the suite green: the marker NAME check, the fence info-string clause, and the fence-character clause (no `~~~` fence appeared anywhere in the suite). Inputs added that flip under each. (21b, 12d, 12e) Also: docs/ci.md said there were three human-half reds when there are four - the catch-all "Could not scan this PR's reviews" does not start with NO HUMAN LGTM ON THIS PR, which defeated the "tell them apart without opening the job" promise in the same paragraph. The fourth is now listed, and named as the one that means the instrument broke rather than the work is outstanding. DELIBERATELY NOT CHANGED. `not LGTM/LGTM` returns 0. The negator rule considers only the word TOUCHING the token, and the second token's preceding character is a slash; making it 1 needs a negation detector that reads past an intervening token, which is the unbounded cleverness the script refuses by name. Pinned as case 10e so nobody widens it while fixing the glued-repeat bug beside it. VERIFIED. 55 self-test cases green. Both gate self-tests, check-shell-sigpipe, check-copyright-headers, check-issue-refs, check-docs-data, check-action- versions: all pass. Eight workflow sabotages red after, six of them green before. Seven checker mutations each turn exactly the intended new case red. Replayed against live API data for all 78 PRs carrying reviews: 38 green, zero false positives, zero false negatives against an independently written matcher. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhF637Ywr7MiKkxaR11Up3
…s a list marker or a line break walked through Three findings from the automated review of #298, all of them real, all of them the same shape: a rule that was stated correctly in prose and implemented one step narrower. THE JOB GUARD READ THE SENDER, NOT THE AUTHOR - and this is the serious one, because it is a false GREEN on a required check rather than a false green on half of one. `github.event.sender` is whoever triggered the event, which on `synchronize`, `reopened` and `ready_for_review` is frequently not the PR's author: a bot pushing to a human's branch - and this repo has one that does - made the sender a Bot, which skipped the whole job. A skipped job reports a `skipped` check run, and GitHub counts that as satisfying a required check. So the documented exemption ("a bot-RAISED PR is not held to the review requirement") silently meant "a bot-TOUCHED PR", and any bot push turned the gate green with neither half asserted. That is this repo's most-repeated defect - a check that reports success without having run - landing inside the gate written to prevent it. `github.event.pull_request.user.type` is the author, which no event's sender can change, so the guard now means what the sentence above it always claimed. Fixed on the transitional `claude-review` job too: that is the job the ruleset currently requires, so a hole left there is a hole in practice for as long as the cutover takes, and fixing it does not make that job stricter than the contract it is frozen at - it makes it run when that contract says it should. Pinned by a new self-test case 26, for the same reason 25e/25f pin the step's guard: the job guard is the one line in this workflow that can turn the gate green with nothing asserted. Proven against four sabotages (back to `sender.type` on either job, `if: false`, guard deleted). THE BLOCKQUOTE AND FENCE EXCLUSIONS DID NOT SURVIVE A LIST MARKER. CommonMark lets a block start straight after one, so `- > LGTM` is a quote and ``- ```py `` opens a fence - but both tests only allowed leading whitespace, so a leading `- ` carried a quoted LGTM past the exclusion written to catch exactly that. Continuation lines of a list item are plain indentation and always worked; it was the marker line itself that did not. Markers are now stripped before either test, bounded at eight levels, and only for an OPENING fence: between two fences every line is literal code, where `- ` is text. Cases 12g, 12h, 13b-13f. THE `?` AND NEGATION CLAUSES DID NOT SEE ACROSS A SOFT LINE BREAK, though Markdown renders one as a space. A body reading "not\nLGTM" or "LGTM\n?" is displayed to every human as "not LGTM" and "LGTM ?", and hard-wrapped prose is the ordinary way to write it - yet both stamped the PR. The scanner now runs one line behind the reader so each candidate can see its neighbours, and splices exactly one line in, only when that side of the token is otherwise empty. A blank line is a paragraph break and still separates them, which is what keeps this a rule you can state in a sentence rather than the unbounded sentiment analysis the checker refuses. Cases 9d/9e, 10f/10g. Every new case was run against the pre-fix checker first: six of them go red there. The four that pin where a rule deliberately STOPS - 9e, 10g, 13e, 13f - cannot, so they were proven against the plausible over-reaching fix instead (strip markers on closing fences too; drop the whitespace a marker needs after it; carry the previous line across blank ones; widen the lookaround). Eleven sabotages, eleven reds. The seven existing coupling sabotages were re-run against the edited workflow and all still bite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhF637Ywr7MiKkxaR11Up3
|
@claude review this Context for the review: the branch has moved twice since the last round.
The one worth a fresh pair of eyes is finding 1, because the fix reaches beyond the reported line: the job guard read Please pay particular attention to:
Not in scope: the ruleset itself (untouched, and the cutover checklist in the description is for @astubbs to run at merge time), and the transitional |
|
Claude finished @astubbs's task in 7m 24s —— View job Review
A caveat up front: this sandbox's Bash tool refused every attempt to actually execute a script ( The job-guard fix (finding 1) — looks correct
The The awk rewrite — the two claimed fixes hold up; one gap does notI hand-traced the soft-line-break splice ( What I don't think is covered: blockquote lazy continuation. The blockquote exclusion ( open && (fenced || strip_markers($0) ~ /^>/) {
break_para()
if ($0 ~ /[Ll][Gg][Tt][Mm]/) note_near("quoted")
next
}only recognizes "inside a blockquote" by a literal
But this body renders on GitHub as a single blockquote — "Someone said this LGTM" — which is exactly the "review quoting somebody else's LGTM ... does not stamp by accident" property this rule set out to guarantee (stated in the header at Since I couldn't execute anything, I'd treat this as plausible, not confirmed by a live run — please sanity-check the trace against the real script before treating it as certain, though I went through the state transitions fairly carefully. On the self-test suite failing "for the right reasons"I couldn't execute On the three thread repliesAll three read accurate against the code as I traced it — the Not reviewed (per your scope note)The ruleset cutover steps and the transitional |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85d38097e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…LGTM out of a quote, and say where that stops
Second automated review round, two findings, both upheld and both the same class as
the first round's: text that renders as quoted or fenced to every human who opens the
PR, but that a line-at-a-time scan read as prose and counted as a stamp.
LAZY BLOCKQUOTE CONTINUATION. A paragraph inside a blockquote continues onto the
following lines even when they drop the `>` - CommonMark calls it laziness - so
> Bob asked whether this was fine and I said
LGTM
is ONE quoted paragraph with nothing of the owner's own outside it, and it stamped the
PR. The scanner now carries the quote across the continuation, ending it at a blank
line, at a fence, or at a line starting a block a paragraph cannot lazily continue
into (a list item, an ATX heading). Both terminators have a positive control - 13h and
13i - because "refuse everything after a `>`" passes the negative case just as well
and would refuse most real reviews, which quote a line and then answer it.
THE CLOSING-FENCE INDENT. A closing fence may carry at most three spaces; the fourth
makes it code and the block stays OPEN. Allowing any leading whitespace ended the
block on a line CommonMark keeps inside it and put every following LGTM into open
prose. Now `^ {0,3}`, with 12j pinning that three spaces still closes - the over-tight
fix (refuse all indentation) is as wrong as the loose one, and only that case says so.
AND A STATEMENT OF WHERE THIS STOPS, which is the part that is not a bug fix. Three
rounds have each found a real CommonMark shape the previous rule missed, and the
supply is not exhausted: an indented code block, an HTML block, a setext heading and
several others are still not excluded. A parser would close that list, and this file
cannot have one - it runs as a step of a REQUIRED check ahead of the gate it protects,
so a dependency present on today's runner image and absent from tomorrow's bricks
every open PR. The header now names those gaps rather than leaving them to be found,
and states the argument that makes them acceptable: the direction of the error, and
the size of the hole they are a corner of. This check already cannot tell a review
GIVING an LGTM from one TALKING about one - "I would want an lgtm from someone else
first" stamps the PR today - so each of these fixes is worth its cost only while it is
cheap and exact. If false positives ever matter more than that, the answer is not more
awk here but a tighter TOKEN rule (the bare word alone on a line, which 46 of the 50
real stamps already are), and that changes what the owner has to type, so it is his
call.
VERIFICATION. Both new negative cases go red against the previous commit's checker.
The four boundary controls cannot, so each was proven against the over-reaching fix:
laziness with no terminators, laziness ignoring block starts, the indent limit removed,
and the indent limit tightened to zero - five sabotages, five reds.
Then the replay, which is what makes the whole round trustworthy rather than merely
tested: the checker as it now stands and the checker as it was before either review
round were both run over every review this repository has ever received - 78 PRs that
carry one. Both return green on exactly the same 38 PRs. Nothing regressed, nothing
newly passed. Two rounds of fixes changed the verdict on zero real reviews, which is
the claim that matters: they closed synthetic holes without touching the habit the
rule was measured against.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BhF637Ywr7MiKkxaR11Up3
|
Thanks — replying to the one substantive finding, and flagging a limitation of this review round that @astubbs should know about. The lazy-blockquote-continuation gap: correct, and already closedYour trace is right. I can confirm it by execution, which you could not: against the checker as it stood at exits 0 — a stamp. It renders as one quoted paragraph. So the state transitions you walked through by hand were accurate, and "plausible, not confirmed by a live run" understates it. It is fixed in
Sabotage-proved: 13g goes red against The class, rather than the instanceThat was the third CommonMark shape found in three rounds — list nesting, the closing-fence indent, and now this. The supply is not exhausted: an indented code block ( The verification you could not run, run
|
…have missed being deleted The laziness rule ends a blockquote's continuation at a blank line, a fence, a list item or a heading. Three of those four had a case; the heading did not, so that half of `starts_block` could have been deleted with the whole suite still green - the condition bin/AGENTS.md names as proving nothing. 13j is that case, and it goes red when the heading branch is removed. Found by spot-checking the clause by hand rather than by a failure, which is the reason it is worth writing down: the sabotage sweep proved every case that EXISTED was load-bearing, and could not say anything about a clause that had no case at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhF637Ywr7MiKkxaR11Up3
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a04a9cfcf3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…d stop the README calling an advisory check required
Third review round. Two of the three findings taken; the third is declined with a
counter-example, and that one is the interesting entry.
LAZINESS NOW STARTS FROM A PARAGRAPH AND FROM NOTHING ELSE. CommonMark will not
lazily continue a heading, a fence, a list or an empty `>` line, so
> # context from the linked issue
LGTM
puts the LGTM OUTSIDE the quote, where it is a real stamp. The previous commit set the
quote state after every `>` line and refused it. That is a FALSE NEGATIVE, and it is
the direction of error this whole check exists to avoid - a memory aid that rejects a
stamp the owner really gave is a nuisance people route around, which is the argument
that settled case-insensitivity in the first place. It also fixes `> a` / `>` / `LGTM`,
which was refused for the same reason. Cases 13k, 13l, 13m; both of the first two go
red against the previous commit, and 13m against dropping the fence clause alone.
THE WORKFLOW README CALLED AN ADVISORY CHECK REQUIRED. It listed `review: bot + human
LGTM` among the contexts the master ruleset requires, while the sub-item directly
below it said `claude-review` is the one currently required - two statements that
cannot both be true, in the one document whose job is to stop somebody getting the
cutover wrong. Producing a context and being required are now stated as the different
things they are: `claude-review` is required and asserts the automated half only;
`review: bot + human LGTM` is produced but advisory and asserts both. Renaming either
is still a ruleset migration, which is the point the list is actually about.
DECLINED, WITH EVIDENCE: capping the OPENING fence at three spaces of indentation.
The finding is right that a four-space-indented backtick run is indented code rather
than a fence, so today a real stamp after one is swallowed. But the fix trades that
rare false negative for a FALSE POSITIVE, which is the wrong direction. Measured, not
argued: with the cap applied, this body
- outer
- inner
```
LGTM
```
stops being recognised as a fence and STAMPS the PR - because a fence inside a nested
list item is legitimately indented four spaces, and telling that apart from indented
code needs the list's content column, which needs a real CommonMark parser. That is
exactly the boundary the previous commit wrote down, and this is the first finding to
cross it. Left as documented behaviour, with the counter-example now in the reply
thread rather than only in a reviewer's head.
Verification: 78 cases green; four sabotages on the laziness rule (carry from every
quote line, carry from none, drop the empty-`>` clause, drop the fence clause) all red.
And the replay again - the checker as it now stands and the checker as it was before
any of the three rounds both go green on exactly the same 38 of the 78 PRs that carry
a review. Three rounds, nine findings, and still zero real reviews judged differently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BhF637Ywr7MiKkxaR11Up3
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f215d3f87f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…and stop docs/ci.md calling an advisory check required
Fourth review round, three findings, all three taken. Two of them are the same defect
in different places: a regex written WIDER than the prose directly above it.
THE CONTRACTION CLAUSE DID NOT REQUIRE THE N. It read "any word, any punctuation, a
`t`", while the documented rule has always said the contraction is the
n-apostrophe-t one. So an ordinary technical token of that shape immediately before
the stamp - a path segment, a hyphenated identifier, a flag - was read as a negation
and a real stamp was refused: `Traced it as far as src/t: LGTM` went red. A false
negative, from a pattern nobody had compared against its own sentence. Cases 10h, 10i.
STARTING A LIST AND INTERRUPTING A PARAGRAPH ARE DIFFERENT QUESTIONS, and one
predicate was answering both. CommonMark lets an ordered list interrupt a paragraph
only when it starts at 1 - otherwise a stray "2." in running prose would silently
become a list - so
> Bob asked whether this was fine
2. LGTM
leaves that LGTM inside the quote, and ending the quote on any numeral stamped it.
That is a false positive on the one exclusion that exists because a review of THIS PR
necessarily quotes the token. `starts_block()` keeps the broad rule, where it is
correct (is the quoted content a paragraph?), and a new `interrupts_paragraph()`
carries the narrow one. Cases 13n and 13o, plus 13i as the control that keeps the fix
from swallowing the bullet form - proven by sabotage in all three directions.
DOCS/CI.MD STILL CALLED AN ADVISORY CHECK REQUIRED. The previous commit fixed that
contradiction in .github/workflows/README.md and left the canonical CI document
saying the opposite, which is worse than not having fixed either: the two documents a
maintainer would consult during the cutover now disagreed. Both the prose and the
lanes table now say what is true - `claude-review` is required and gates the merge;
`review: bot + human LGTM` is produced but advisory until the documented PUT.
Also recorded, because it cost real time: the awk program is a single-quoted shell
string, so an apostrophe in one of its comments ENDS IT. Writing "n't" in the comment
explaining this fix broke the script outright. The self-test caught it in seconds -
its harness self-check refuses to run any case until a must-pass and a must-fail
stream disagree, and here they agreed at exit 2. That check has now paid for itself,
and the comment says not to reintroduce the apostrophe.
Verification: 82 cases green; five sabotages this round (any-numeral interruption,
no-numeral interruption, no-bullet interruption, the n dropped, the contraction clause
removed) all red. Replay unchanged for the fourth time: the current checker and the
checker from before any review round both go green on exactly the same 38 of the 78
PRs that carry a review. Four rounds, eleven findings, zero real reviews judged
differently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BhF637Ywr7MiKkxaR11Up3
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e72b72eed7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…d finish the required-vs-advisory sweep Fifth review round, two findings, both taken. THE SPLICE CROSSED A HARD BREAK, WHICH RENDERS AS THE OPPOSITE OF A SOFT ONE. Two trailing spaces (or a trailing backslash) render an actual line break, so the words either side are NOT adjacent in the rendered text - the exact inverse of the soft break the splice was built for. Joining them refused real stamps: `not<2 spaces>` then `LGTM` reads to a human as two lines with the negator nowhere near the token, and it went red. So did `LGTM<2 spaces>` then `?`. `hard_break()` now gates the splice in both directions. Cases 9f, 10j and 10k are the negatives-turned-positives; 9g is the boundary that keeps it honest, because ONE trailing space is not a hard break and 9d must still refuse that. Three sabotages - ignore the two-space form, ignore the backslash form, treat any single trailing space as a break - each turn a different one of those four red. AND THE REQUIRED-VS-ADVISORY CLAIM IS NOW SWEPT RATHER THAN PATCHED. This was the THIRD round to report the same defect in a different file: the workflow README (round three), docs/ci.md (round four), and now AGENTS.md. Fixing them one per round was the error - the whole point of that correction is that the documents a maintainer consults mid-cutover must agree, and fixing one at a time guaranteed they did not. So this commit greps for every occurrence instead: AGENTS.md and the workflow index table both now say `claude-review` is the context the ruleset requires and `review: bot + human LGTM` is advisory until the swap, matching docs/ci.md and the workflow header. The remaining mentions are the job's own `name:`, the transitional job's deletion condition, and the two corrected passages themselves. Verification: 86 cases green; three sabotages this round, three reds. The replay is unchanged for the fifth time - the current checker and the one from before any review round both go green on exactly the same 38 of the 78 PRs that carry a review. Five rounds, thirteen findings, and still zero real reviews judged differently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhF637Ywr7MiKkxaR11Up3
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6ab0f6980
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
593 lines of checker and 855 of self-test, to answer "did Antony say lgtm". It parsed code fences, blockquotes, negation forms, glued repeats, CRLF line endings and typographic apostrophes - defences against an attacker who is also the only person the check protects. The last thing the previous round was doing was widening a bracket class because a typographic apostrophe is three UTF-8 bytes. The rule as stated: a review by the owner whose body contains lgtm, any case, anywhere. That is now what the code says. The marker machinery went with it, and that is the interesting part. It existed because the workflow streamed every review as flat text - marker line, body, marker line, body - so a body could forge a segment header and mint an owner LGTM out of a stranger's comment. Hence an unguessable token per run. Filtering on .user.login with jq BEFORE any text is looked at removes the attack, so the token defends nothing and is gone. The reviews endpoint gives "a review, not a comment" for free. Behaviour is unchanged where it matters: case-insensitive, anywhere in the body, submitted reviews only, and still not head-sensitive - review state is not consulted, because the ruleset dismisses stale reviews on push and consulting state would silently un-stamp a PR the owner had already stamped. Verified against the real data rather than fixtures alone: PR #206 reads LGTM-present, #298 and #299 read absent, which is correct in all three cases. The self-test keeps the two real spellings the repo's history contains - Lgtm on #84, and the mid-sentence form ending in a question mark on #73 - plus a negative control proving the check can fail at all. 15 lines and 11 cases, from 593 and ~55.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea6b45df7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e-review alone Reverts the rename, the transitional duplicate job and the ruleset cutover. The ask was to add an LGTM requirement, not to restructure the existing one, and folding the two together turned a new check into a migration. Two checks rather than one combined check, for two reasons. The first is the point of the thing: the checks list now says WHICH half is missing without opening anything - "claude-review" red means no automated review, "review: human LGTM" red means I have not read it myself yet. A combined check is red either way, which is exactly the question the human half exists to answer. The second is blast radius. claude-review is a required status check matched BY NAME in the master ruleset, so folding this in meant renaming that job, adding a transitional duplicate so the old name kept reporting, and an ordered ruleset swap that bricks the repo if the steps are done in the wrong order - a required check that never reports blocks every PR. Adding a job costs none of that. claude-review is byte-identical to master; the workflow diff is 60 added lines and zero removed. The new context becomes required by adding it to the ruleset once it has merged and is reporting, which is additive and reversible. Also drops the docs restructuring that went with the rename: the seven- clause contract restatement, the AGENTS.md edit, and the bin/check-review- posted.sh comment churn are all reverted. docs/ci.md gains one section describing the new check, and the README API list gains its name.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbc5f886b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The human half had inherited the automated half's bot guard. Two problems with that, and removing it fixes both at once. It was the wrong requirement. The ask is "I review everything myself", and a Dependabot PR is still a change going in - a dependency bump is exactly where a human should look. The two jobs assert different things, so they should not share an exemption: a bot-raised PR genuinely does not need an AUTOMATED review, which is why claude-review keeps its guard and is otherwise untouched. It was also the wrong FIELD, as Codex pointed out. github.event.sender is the event ACTOR, not the PR author, so a human-authored PR synchronized or reopened by a bot or a GitHub App would have skipped this job - and a skipped job SATISFIES a required status check, so the context would have gone green having asserted nothing. The mirror case loses the exemption for a bot PR reopened by a human. Keying on pull_request.user.type would fix the direction and keep the skip. Deleting the guard removes the skip, so there is nothing to satisfy the check by accident. One line fewer, and the whole class goes. Not reachable in this repo today - only release.yml holds contents: write and it is workflow_dispatch, and there are zero skipped gate runs in the last hundred - but that is a property of today's workflows, not of the guard. Consequence, accepted deliberately: bot PRs now sit red until stamped. If that proves too noisy the exemption comes back on purpose, keyed on the author.
Description
The review gate has only ever asked whether a machine reviewed the PR. The half that actually decides a merge here — "have I read this one myself yet?" — was carried in your head across a dozen open PRs, and was getting dropped. This adds it as the second half of the same check, so it is visible on the PR instead of remembered.
The gate now asserts two things, and says which one is missing:
bin/check-review-posted.sh(unchanged)This PR has never been reviewed: claude[bot] ...bin/check-human-lgtm.sh(new)NO HUMAN LGTM ON THIS PR ...Both halves run on every gate run even when the other has already failed, so one look at the checks list tells you everything outstanding rather than only the first thing to go red. The second half is skipped only when the first never ran at all (fork refusal, failed self-test), where a second error would be noise.
The exact matching rule
Canonical home:
bin/check-human-lgtm.sh, and only there.docs/ci.mdowns the gate contract (both halves must pass); the script owns the matching rule below, because its prose sits beside the awk that implements it and the two cannot drift apart unnoticed. What follows is a description of the change, not a fifth copy to maintain.A PR satisfies the human half when at least one submitted pull-request review exists such that:
repos/<repo>/pulls/<n>/reviews. A review, not an issue comment: "lgtm" typed into the ordinary comment box does not count, and neither does an inline comment on a line of code. The stamp is the thing you press "Submit review" for..user.loginisastubbs, compared case-insensitively (GitHub logins are unique case-insensitively, so folding cannot admit a second person — and a required check that could brick the repo over a capital letter is worse than the problem it prevents). Taken from the API, never from anything written in a body.lgtm:lgtm,LGTM,Lgtmall count;ALGTM,lgtm2,xLGTMdo not match; punctuation either side is fine ((lgtm),LGTM!,-- lgtm);?(optionally after spaces) —lgtm?asks, it does not stamp;not,no,never, or ann'tcontraction — soNOT LGTM,not lgtm,isn't LGTMare refused. Only the word touching it is considered; a general negation detector is unbounded, and half of one firing unpredictably is worse than a rule you can state in a sentence;PENDING(never-submitted) review is ignored.Why case-insensitive, and why state is ignored — both decided by looking
I checked the rule against every LGTM this repo has actually received rather than against taste, because a memory aid that rejects the habit it is meant to remind you of is a nuisance people route around.
50 owner LGTM reviews, across 38 PRs, from #63 to #292 — and all 50 are
COMMENTEDreviews, not approvals. A rule that wantedAPPROVEDwould have gone red on every stamp this repo has ever been given.Two of them are why the rule is shaped the way it is, and both are now self-test cases:
lgtm. One — ci: quarantine lane - @Quarantined tag + enforced registry so green checks mean mergeable #84 — isLgtm. Case-insensitivity is therefore not a courtesy: without it this repo has a stamp on record that the checker would call missing.lgtm to tryon tooling(rename): the package-rename script, and the copyright fix it needs to run #280, and three of the formlgtm, @claude can you take a look please). One of those — build(deps): refresh dependencies + build plugins to latest non-major #73'slgtm, @claude how about you?— ends in a question mark and must still pass, which is why the?clause rejects only a?touching the token rather than one anywhere in the body.Two consequences of "state is not consulted" are load-bearing rather than pedantic:
APPROVEDreview with an empty body does NOT count. The request was for a visible, deliberate word; a silent Approve is exactly the reflex this exists to interrupt.DISMISSEDreview still DOES count. The master ruleset setsdismiss_stale_reviews_on_push: true, so an approval here is dismissed by the next push. Had the check consulted state, it would have been head-sensitive through the back door — green, then quietly red again after an unrelated typo fix.CHANGES_REQUESTEDreview saying LGTM counts. A contradiction you wrote about your own PR is not this check's to arbitrate, and unresolved threads block the merge by a separate rule in the same ruleset.Why it is head-insensitive
An LGTM on any commit of the PR counts for the whole PR, permanently. Two independent reasons:
archive/review-gate-strict-head-freshness, reasoning indocs/inflight/parked-strict-review-gate-freshness.md). Two halves of one check disagreeing about what "reviewed" means would be worse than either rule alone.Two self-test cases pin this as positive assertions rather than as deleted code, so silently re-adding freshness breaks the suite.
Rename:
claude-review→review: bot + human LGTMclaude-reviewnamed the implementation of one half. It is wrong twice now: the check is not only about the Claude review, and the name tells you nothing about which half is red.review: bot + human LGTMfollows the repo's required-check convention (shell: sigpipe,deps: vulnerabilities,docs data: audit), and the two operands are the two halves — so a red entry in the checks list already says which question is open, and the word LGTM is visible on the PR page without opening anything. That visibility is the whole request.Verification
bin/test-check-human-lgtm.sh: 86 cases, over half negative controls. Every clause above has a case that fails without it: non-owner, login merely containing the owner's, LGTM in an issue comment,PENDING,lgtm?,LGTM ?,NOT LGTM,never LGTM,isn't LGTM, glued tokens, three fenced-block variants, a blockquote, an emptyAPPROVED, a forged marker. The harness also proves itself able to fail before running any case..github/workflows/claude-code-review.ymland assert on its structure: it exists as a real (uncommented) step, reads the reviews endpoint, emits the marker the checker parses, carries a guard that is neither never-true nor changed from the intended one, swallows the checker's exit status nowhere, and ends with the checker invocation — and, at the level above, that both of the workflow's jobs are still guarded on the PR's author rather than the event's sender. A gate fed the wrong stream — or one whose step cannot run — would report success without having asked the question, this repo's most-repeated defect.grep -Fsubstring searches, which could not see a neutered step. Measured on a scratch copy:if: falseon the step,|| trueon its pipeline,; true,set +e,continue-on-error: true, and commenting the entire step out all left the suite green at 42/42, and one case was satisfied by two header comments alone. All eight sabotages now go red; the unsabotaged copy stays green. That was the failure class above occurring inside the guard written to prevent it.bin/check-copyright-headers.sh,bin/check-issue-refs.sh,bin/check-docs-data.sh,bin/check-shell-sigpipe.sh,bin/check-action-versions.sh, and both gate self-tests: all pass locally.What the automated review rounds found
Thirteen findings across five rounds, twelve taken and one declined with a measured counter-example, all upheld, all the same shape: a rule stated correctly in prose and implemented one step narrower than it reads.
1. The job guard read the event's sender, not the PR's author — a false GREEN on a required check. This is the serious one, and it predates this PR.
github.event.senderis whoever triggered the event, which onsynchronize,reopenedandready_for_reviewis frequently not the author: a bot pushing to a human's branch — and this repo has one that does — made the sender aBot, which skipped the entire job, and a skipped job satisfies a required check. The documented exemption ("a bot-raised PR is not held to the review requirement") silently meant "a bot-touched PR", so any bot push turned the gate green with neither half asserted. That is this repo's most-repeated defect landing inside the gate written to prevent it. Nowgithub.event.pull_request.user.type, which no event's sender can change — on the transitionalclaude-reviewjob too, because that is the job the ruleset requires today, so a hole left there is a hole in practice for as long as the cutover takes. New case 26 pins both guards, for the same reason 25e/25f pin the step's.2. The blockquote and fence exclusions did not survive a list marker. CommonMark lets a block start straight after one, so
- > LGTMis a blockquote and- ```pyopens a fence — but both tests only allowed leading whitespace, so a leading-carried a quoted LGTM straight past the exclusion written to catch it. (Continuation lines of a list item are plain indentation and always worked; it was the marker line itself that did not.) Markers are now stripped before either test, bounded at eight levels, and only for an opening fence: between two fences every line is literal code, where-is text. Cases 12g, 12h, 13b–13f.Round one
3. The
?and negation clauses did not see across a soft line break, though Markdown does. A newline inside a paragraph renders as a space, so a body readingnot\nLGTMorLGTM\n?is displayed to every human as "not LGTM" and "LGTM ?" — and hard-wrapped prose is the ordinary way to write it. Both stamped the PR. The scanner now runs one line behind the reader so each candidate can see its neighbours, and splices in exactly one line, only when that side of the token is otherwise empty. A blank line is a paragraph break and still separates them, which keeps this a rule you can state in a sentence rather than the unbounded sentiment analysis the checker refuses. Cases 9d/9e, 10f/10g.Round two
4. A lazy blockquote continuation escaped the quote exclusion. CommonMark lets a paragraph inside a blockquote continue onto lines that drop the
>, so> Bob said\nLGTMis one quoted paragraph with nothing of the owner's own outside it — and it stamped the PR. This is the likeliest of the three quote findings to be hit in practice, because quoting a line and answering it without a blank line is how people write. The quote now carries across the continuation, ending at a blank line, a fence, or a line starting a block a paragraph cannot lazily continue into. Both terminators have a positive control (13h, 13i), because "refuse everything after a>" passes the negative case just as well and would refuse most real reviews.5. A four-space-indented closing fence closed the block. It must not: the fourth space makes the line code and the block stays open, so everything after it — including an LGTM — was read as prose. This one was a regression introduced by fix 2, which split opening from closing fences and carried
^[ \t]*across the closing side unexamined. Now^ {0,3}, with 12j pinning that three spaces still closes: the over-tight fix (refuse all indentation) passes 12i perfectly well and is just as wrong.Round three
6. Laziness was carried from every quoted line, not only from a paragraph — a false negative, and the first one these rounds produced. CommonMark will not lazily continue a heading, a fence, a list or an empty
>, so> # contextfollowed by a bareLGTMputs that LGTM outside the quote, where it is a real stamp. Fix 4 refused it. This direction of error is the one the whole check exists to avoid — a memory aid that rejects a stamp the owner really gave is a nuisance people route around, which is the argument that settled case-insensitivity in the first place. Cases 13k–13m.7.
.github/workflows/README.mdlisted an advisory check as required. It namedreview: bot + human LGTMamong the contexts the master ruleset requires, while the sub-item directly below saidclaude-reviewis the one currently required — two statements that cannot both be true, in the one document whose job is to stop somebody getting the cutover wrong. Producing a context and being required are now stated as the different things they are.8. Capping the opening fence at three spaces — DECLINED, and this thread is left open for @astubbs. The premise is correct: a four-space-indented backtick run is indented code, not a fence, so a real stamp after one is swallowed today. But the fix trades that rare false negative for a false positive, which is the wrong direction — and this was measured rather than argued. With the cap applied, a fence inside a nested list item (legitimately indented four spaces, because that is its content column) stops being recognised, and a fenced LGTM stamps the PR. Telling those apart needs every open list item's content column — a real block parser. That is precisely the boundary written down below, and this is the first finding to cross it. The escalation, which is why the thread is open: if this class should be closed properly, the answer is not more awk — it is to tighten the token rule so the stamp must be the bare word alone on a line (46 of the 50 real stamps already are), which makes every block-structure question moot. That changes what you have to type, so it is your call.
Round four — two regexes that were wider than their own prose
9. The
n'tcontraction clause did not require then. It read any word, any punctuation, at— while the rule documented two lines above it has always said the contraction is the n-apostrophe-t one. So an ordinary technical token of that shape before the stamp was read as a negation and a real stamp refused:Traced it as far as src/t: LGTMwent red. Another false negative, from a pattern nobody had compared against its own sentence. Cases 10h, 10i.10. Starting a list and interrupting a paragraph are different questions, and one predicate was answering both. CommonMark lets an ordered list interrupt a paragraph only when it starts at
1, so> quoted\n2. LGTMleaves that LGTM inside the quote — and ending the quote on any numeral stamped it. A false positive on the one exclusion that exists because a review of this PR necessarily quotes the token. Split intostarts_block()(broad, for "is the quoted content a paragraph?") andinterrupts_paragraph()(narrow). Cases 13n, 13o, with 13i as the control.11.
docs/ci.mdstill called the advisory check required — round three fixed that contradiction in the workflow README and left the canonical CI document saying the opposite, which is worse than fixing neither. Both the prose and the lanes table now say what is true.Also worth recording, because it cost real time: the awk program is a single-quoted shell string, so an apostrophe in one of its comments ends it. Writing
n'tin the comment explaining fix 9 broke the script outright. The self-test caught it in seconds — its harness refuses to run any case until a must-pass and a must-fail stream disagree, and here they agreed at exit 2. That guard exists because an assertion which has never failed proves nothing; this is the first time it has fired.Round five
12. The cross-line splice crossed a hard break, which renders as the exact opposite of a soft one. Two trailing spaces (or a trailing backslash) render an actual line break, so the words either side are not adjacent — and joining them refused real stamps:
not··thenLGTMwent red, as didLGTM··then?. Fix 3 reached one line further than the rendering does. Cases 9f, 10j, 10k — with 9g as the boundary, because one trailing space is not a hard break and 9d must still refuse that; without it, "treat any trailing space as a break" passes the new cases and silently reopens the round-two hole. The splice now simply follows the rendered text: it crosses a soft break, stops at a blank line, stops at a hard break.13.
AGENTS.mdstill called the advisory context required — and this was the third round to report that same defect in a third file. Fixing them one per round was itself the mistake: the whole point of the correction is that the documents a maintainer consults mid-cutover must agree with each other, and patching one at a time guaranteed they did not. This round greps for the claim instead, which caught a fourth occurrence nobody had reported — the index table at the top of.github/workflows/README.md, whose conventions section had been fixed in round three while the table above it was missed.And a stated boundary, which is not a fix
Three rounds have each found a real CommonMark shape the previous rule missed, and the supply is not exhausted — an indented code block (
LGTM) is not excluded either, nor are HTML blocks or setext headings. Rather than meet those one round at a time,bin/check-human-lgtm.shnow names them as known, deliberate non-exclusions and states why that is acceptable: a parser would close the list and this file cannot have one (it is a step of a required check, so a runner-image dependency that vanishes bricks every open PR — the same argument that keeps the self-test on awk rather than PyYAML); the error direction is toward calling text quoted, which refuses a stamp and says why; and the hole they are a corner of is already far wider by design, since the checker cannot tell a review giving an LGTM from one talking about one. Each fix is worth its cost while it is cheap and exact. If false positives ever matter more than the memory-aid framing admits, the answer is a tighter token rule — the bare word alone on a line, which 46 of the 50 real stamps already are — and that changes what @astubbs has to type, so it is his call.Preserved deliberately
Read-only (
contents: read,pull-requests: read— the reviews endpoint needs no new scope) · bot-raised PRs still exempt, and now only those · a fork PR still cannot satisfy the gate · no skip word, label or escape · the job still posts nothing.A
pull_request_reviewtrigger is still deliberately NOT added, though the reason has changed and the old one is worth recording. It looks like the obvious way to clear the human half the moment the LGTM lands. It used to be a trap: on that event the sender is the reviewer, so a review submitted by a bot would have skipped the job on itssender.type != 'Bot'guard — and a skipped job reports askippedcheck run, which GitHub counts as satisfying a required check.That trap is closed, because the guard bug it depended on turned out to be a live defect in its own right and is fixed below. The trigger is nevertheless left unwired here: it changes when a required check runs, which is its own decision rather than a side effect of this one. Until it is made, the LGTM needs the same re-run the automated half already documents — or just leave the LGTM before asking for the automated review, and the reviewer's
refresh-gatejob re-runs the gate with both halves already true.I have not touched the ruleset.
claude-reviewis currently required in ruleset15055005, and a required context that nothing reports blocks every open PR with no way to clear it. So this PR does not remove the old name: it adds a transitional second job that keeps reportingclaude-reviewunder the old contract (automated review only), so both names report during the swap and no window exists where a required context is unproduced.The transitional job deliberately does not use
needs:on the real job. A job that skips when its dependency fails reportsskipped, which GitHub counts as satisfying a required check — the mirror would have gone green exactly when the gate went red. Hence duplicated steps, with a stated deletion condition in its header.Step 1 — merge this PR
Nothing changes for anyone.
claude-reviewis still required and still reports, under the same contract it has today. The new contextreview: bot + human LGTMreports alongside it as an advisory canary asserting both halves.Step 2 — watch the canary (optional, recommended)
Open or push a PR and look at the new check. It should be red with
NO HUMAN LGTM ON THIS PR ...; submit a review sayinglgtm, re-run the gate run, and it should go green.Step 3 — swap the required context (one atomic PUT)
Both contexts are reporting at this point, so there is no gap. Snapshot first — this file is the rollback.
I dry-ran steps 1–3 locally: the transform touches exactly one string and preserves its
integration_id: 15368. I did not run step 4.Step 4 — verify the swap
Expect
review: bot + human LGTMpresent andclaude-reviewabsent.Then unblock the PRs that were already open. For
pull_requestevents the workflow file comes from the merge ref, so an open PR only starts producing the new context after a fresh event. Re-running an existing run replays the old workflow file and will not help. For each open PR, push a commit or close-and-reopen it (reopenedis in the gate's trigger list):Doing the swap when few PRs are open makes this cheap.
Step 5 — delete the transitional job (a follow-up PR, after step 4)
Remove the
claude-reviewjob from.github/workflows/claude-code-review.yml, and the transitional paragraphs in its header and indocs/ci.md. Doing this before step 3 is what bricks the repo.Rollback, at any point before step 5
rs.before.jsonis the exact prior state, so this is a true revert. After step 5 it is not — the old job no longer exists to reportclaude-review, so roll the code back first.Checklist
docs/ci.mdnow states both halves and the full matching rule; the red-gate section, the workflow table,AGENTS.mdand.github/workflows/README.mdfollow the renameN/A - CI/repo tooling, not a user-facing library feature— nodocs/features/entrybin/test-check-human-lgtm.sh(86 cases, over half negative controls, including ten structural workflow-coupling assertions), run by the gate before it trusts the checker