feat(scripts): tell a real CodeRabbit review from a green tick - #2986
Conversation
A CodeRabbit pass does not mean the diff was reviewed. Three states render as the same check: reviewed and found nothing, reviewed only the newest commit, or never ran because the org is rate limited. Under Fair Usage the third is green on every open PR at once, so 'all our PRs show CodeRabbit passing' is a symptom rather than reassurance. Two signals are needed and neither works alone. The rate-limit sentinel alone gives false positives: a PR can carry it verbatim and still have genuine inline findings posted minutes later, because the summary comment is not rewritten when a later pass succeeds. So the sentinel is conclusive only when the inline thread count is also zero. Note gh pr view --json comments does not return inline review threads at all, so the count comes from reviewGraphQL threads; without that every PR reads as having no findings. Classification is a pure function with unit tests over synthetic bodies. Not wired into CI, and should not be: the answer depends on transient GitHub state, so a required check on it would fail for reasons unrelated to the diff. Empty target list exits non-zero rather than reporting a vacuous all-clear.
|
Warning Review limit reached
Next review available in: 32 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CodeRabbit ran locally (the check on this PR was a false green)The hosted CodeRabbit is rate limited under Fair Usage, so it posted no review and the GitHub check still renders as pass. This PR was never reviewed by it. Below is a local CLI completed normally, 3 findings. I judge 2 real and 1 wrong. Real:
This is reachable from the real caller. It also contradicts the module's own rule one branch down: STALE-SUMMARY exists because inline findings prove a review ran whatever the summary says. The same argument applies when there is no summary at all. The test file has no case for this input, so nothing catches it. The failure direction is a false alarm, not a false green, so the guard stays safe in the direction that matters. Still worth Real but low:
Wrong: " Refuted by running it. gh 2.97.0: Two pages, one merged JSON array, parse succeeds. gh merges top-level array pages for you. The suggested |
|
I made this exact bug an hour ago while auditing the board by hand, so this is offered as a concrete test case rather than a review nit. The reported issue, with a live counterexample on this board
Two properties make this harder than it looks, both learned the hard way today: 1. The marker is a snapshot, not a history. CodeRabbit edits its summary comment in place. The marker means the latest push went unreviewed; it cannot tell you the PR was never reviewed, and it appears and disappears as later pushes are reviewed or refused. I watched #2967's marker be present, then absent after a later push got reviewed. 2. Inline review comments are authored by That second one is worth a test here, because the failure was silent in the direction that reads as "no signal" rather than as an error. The structural pointA detector that returns NO-REVIEW for a reviewed PR has the same failure direction as the tick it exists to catch: the classifier disagreeing with reality in the way that is hardest to notice. A hollow tick says "reviewed" for something that was not; this says "not reviewed" for something that was. Both are only visible if you already know the answer. Concretely, the query that survives an edit is: Worth pinningA fixture where Not blocking, but out of scope for this PR to fixThe real-world numbers, for calibration: of 20 green PRs on the board, 16 were never reviewed and 1 (#2957) carried a marker while having been reviewed. So marker-counting overstates by one here, and would overstate more on a board with more re-pushed PRs. |
|
Re-checked at head But the fix overshoots in the other direction, and the new failure is the more dangerous one. The new false REVIEWED
That is the exact case this script exists to catch. Commit B is unreviewed, and the tool says the PR is reviewed because commit A was. The reason this matters more than the bug it replaced: a false NO-REVIEW is loud and self-correcting, someone looks and sees the review. A false REVIEWED is silent and terminal, nobody looks again. Trading one for the other is not neutral. Line 61 is also still there: The fixCompare the newest CodeRabbit thread's timestamp against the head commit's date, and report REVIEWED only when the review is not older than the code. That is what "reviewed" has to mean for a merge gate; anything else is reviewing a commit that is no longer the one being merged. Concretely on this board right now: #2967 was reviewed at 07:09, pushed again at 09:58, reviewed again at 09:51 for the middle commit. Reading it at 10:00 without comparing times would have called the newest commit reviewed when it was not. I know because I made that call and had to correct it publicly. Still worth three values, not twoPresence of inline threads proves a review happened. Absence proves nothing, because a clean review leaves none. So the honest output is REVIEWED / NEVER-REVIEWED / UNKNOWN, and a merge gate should treat UNKNOWN as "go look", not as either verdict. Collapsing UNKNOWN into |
|
Independent confirmation of the
// The sentinel is stale: findings exist, so a pass did run afterwards.Findings existing proves a review ran at some point. It does not prove one ran after the sentinel, and it does not prove one ran after the latest push. Nothing in that branch compares thread time to head time. Measured on #2967 as it stood earlier today: Sentinel present, threads > 0, so this branch reports That case is not hypothetical in a second sense: I reviewed #2967 by hand at that head, found a blocker in it (the WASM Why this direction is worse than the one it replacedThe
A detector whose whole purpose is distinguishing "reviewed" from "green tick with nothing behind it" must not itself return the reassuring answer on absence of evidence. Comparing the newest thread's timestamp against the head commit's date closes it, and both values are already in the API responses the script fetches. Line 61 is still open too
Worth stating for the record: I made the mirror of that mistake reading the board today. I counted rate-limit markers and called 17 PRs unreviewed; #2957 had two inline threads and two review records and was genuinely reviewed, with the marker meaning only that its latest push was not. Both halves of this bug are easy to walk into, which is the argument for the tool existing — and for it not shipping with either half. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Re-checked at head 8863520. The only change since the hold is a merge of main; no fix commit landed, and both directions are still live. Direction 1, false NO-REVIEW. classifyReviewState({ bodies: [], inlineThreadCount: 3 })
// -> { state: 'NO-REVIEW', reviewed: false, why: 'CodeRabbit posted no comment at all' }Three CodeRabbit inline threads mean a review ran and left findings; the tool reports that none did. CodeRabbit can post inline findings while the summary comment is absent or was deleted, so this input occurs in practice. Direction 2, false The fix needs the review's time (thread createdAt or the review's submittedAt) compared against the head commit's pushed/committed date, and the empty-bodies branch has to consult threads before concluding NO-REVIEW. Holding on both. |
…once
The classifier returned NO-REVIEW whenever the CodeRabbit comment list was
empty, before the inline thread count was consulted. CodeRabbit posts inline
findings while its summary comment is absent, deleted, or rewritten in place,
so three live threads read as "posted no comment at all". Only the absence of
both signals is absence of review.
And no timestamp reached the classifier at all: threads were fetched for their
author, comments for their body, and the input was {bodies, inlineThreadCount}.
A review from two pushes ago therefore certified commits the bot never saw. The
GraphQL query now also returns each thread's first-comment createdAt, each
CodeRabbit review's submittedAt, and the head commit's pushedDate (falling back
to committedDate), and evidence older than its head commit reports
PRE-PUSH-REVIEW rather than REVIEWED.
The two mistakes are not symmetric. A false "not reviewed" is loud and
self-correcting; a false "reviewed" is silent and terminal, because nobody
looks again and the tool has certified the thing it exists to catch. So the
ambiguous cases now resolve to reviewed: false -- a comment with none of the
markers (INCONCLUSIVE), and a timestamp that is missing or unparseable
(UNDATED). The parse returns null rather than NaN on purpose: every comparison
against NaN is false in both directions, so a bare `<` would have reached the
reassuring branch by accident.
Tests go 10 -> 19, covering both directions, both composed, a genuinely
reviewed-at-head PR still reading REVIEWED, and each ambiguous case. The file
was never run by CI; it now has a step.
|
Both directions fixed, pushed as Direction 2's inputs were simply ignored — the old signature was Direction 1
Direction 2 — new data, still one round tripThe existing GraphQL call is widened rather than a second one added:
REST
Missing or unparseable timestamps — a decision, not an accident
One judgement call beyond your brief, flagged rather than slipped inI also flipped INCONCLUSIVE — a CodeRabbit comment carrying none of the markers — from The mirror mistake is guarded, and the tool now names yours
Live run of the real script: #2957 is exactly the case you recorded getting wrong, and the tool now separates it from both errors: reviewed, but not at head. Neither "unreviewed" nor "reviewed" was the right answer, which is why it needed a third state. Counts10 pass before, 19 after, 0 fail. With the production file reverted to Also wired a CI step in: the PR's test file was connected to nothing and had never run in CI, which for a detector shipping with both halves of its own bug is not a detail I want to leave. The PR body still describes the two-signal version and says nothing about freshness — updating it now. |
… machine-readably The header already explained in prose why this must not gate CI -- its verdict depends on transient GitHub state -- but prose is not something #3071's wiring checker can read. That checker accepts exactly one declaration, an `@unwired-by-design <reason>` line, and reports every other unreferenced scripts/check-*.mjs as an accidental omission. Say it in the form the gate reads, so the deliberate exception stays visible and the genuine omissions stand out. Comment only. Its classifier tests still run: node --test scripts/lib/coderabbit-review-state.test.mjs -> 19 pass, 0 fail
|
Wiring status resolved, pushed The header said "NOT wired into CI and should not be" in prose. It now carries That is the right call and it is exactly the distinction #3071 encodes: the failure is a gate nobody invokes silently, not a gate deliberately kept out of CI with its reason recorded. Its test file is wired — this PR adds a named step in Worth noting what the fixed classifier has been doing in practice today: it correctly separates the three states on live PRs, including |
|
Flagging that this PR has no CI at all rather than a passing CI: The cause is the merge state. This PR is To be precise rather than alarming: of the six currently-dirty PRs, four have zero runs and two (#2971, #2970) have runs, because those two went dirty after their runs happened. So a PR dirty at push time gets no CI; one that goes dirty later keeps runs computed against a base that no longer exists. Both look green from outside and need different fixes. Resolving the conflict is all that is needed to get a real signal here. |
…review-check # Conflicts: # .github/workflows/test.yml
|
Merged current One conflict in Both directions this PR exists for verified still live after the merge:
19 pass, 0 fail. Fitting that this PR was itself invisible to a board sweep for a reason its own subject describes — a state that renders as green because nothing ran, rather than because something passed. |
…not green
`check-coderabbit-review.mjs` answers one question per PR: did CodeRabbit
read the diff at the head commit. A sweep across all 36 open PRs found
the failure modes it was built for, plus four it does not cover — each of
which renders as green, or as "nothing failing", in any report that
counts only failing and pending checks:
- `headRepositoryOwner.login` is not ours. Every other column is then a
report about a branch we cannot push to.
- `mergeStateStatus == DIRTY` WITH runs present: green checks over a
merge commit that can no longer be formed. #2970 and #2971 are in
exactly that state, and both show `fail=0, pending=0`.
- ZERO workflow runs at the head commit. A PR that was already DIRTY at
push time never gets a run, so `statusCheckRollup` comes back EMPTY —
and an empty rollup counts up to the same `fail=0, pending=0` as a
fully passing one. This is the vacuous-pass shape, so the run count
is a first-class signal here rather than something derived.
- The newest run on the BRANCH is against a superseded commit, while
the rollup still shows that older commit's green.
The sweep that found these was a throwaway script, which makes it a habit
someone has to remember. It lands here as a second entry point,
`scripts/check-pr-green.mjs`, over a new pure module
`scripts/lib/pr-green-sweep.mjs`. `coderabbit-review-state.mjs` is reused
unchanged — the review verdict is one column of the table, not a
reimplementation.
The disqualifier order is worst-first and each earlier reason invalidates
the later ones AS EVIDENCE: a fork's counts are not ours to read, a DIRTY
base means the green ran on a merge that cannot be formed, zero runs
means the counts are empty rather than passing, a stale run means they
describe a commit that is no longer the head. `not ours` takes precedence
as a disqualifier but ranks LOWEST for sorting, because it is the one
verdict that is not a task; both halves of that deliberate disagreement
are pinned by tests.
It must not pass vacuously, so three cases are hard failures with their
own `kind` and their own message rather than a shorter, cleaner report:
zero PRs returned (what a wrong `--author`, a wrong repo and a truncated
response all look like), an unreachable API — including mid-sweep, where
a swallowed per-PR failure would drop a row from a report that still
looks complete — and a malformed response, which covers a non-JSON body,
an empty body, a runs payload with no numeric `total_count`, and a
GraphQL payload with no `pullRequest` node. A missing `total_count` is
reported as malformed, never defaulted to 0: zero runs is a real verdict
about the PR, and defaulting would manufacture it out of a broken sweep.
24 tests in the style of the existing classifier's. Verified by mutation:
dropping the zero-runs disqualifier fails 2, returning `[]` for an empty
PR list fails 1, swallowing a transport failure fails 2, tolerating a
runs payload with no `total_count` fails 1, disabling stale-run detection
fails 3, and removing the not-ours precedence fails 3.
Wiring: the tests run in CI as a named step in `node-tests` (and were
already covered by that job's `scripts/lib/*.test.mjs` catch-all). The
entry point itself is marked `@unwired-by-design` per the convention
#3071 establishes — every verdict it produces is transient GitHub state
(queued runs, review latency, a base that is DIRTY only until the next
rebase, rate limiting), so a required check built on it would fail for
reasons unrelated to the diff under test.
|
Extended this PR with the sweep it was always half of, pushed This PR's classifier answers "did CodeRabbit actually review this". A board sweep this morning found three more states that render as green and are not, none of which the classifier can see:
New entry point It cannot pass vacuouslyEach proven both as a unit test and live at the entry point, exit 1 every time: Plus mid-sweep unreachability, since a swallowed per-PR failure would shrink the report while it still looks complete. A missing 24 tests, mutation-proved rather than assumed:
Wired, per #3071's conventionThe classifier's tests get a named step in Live run36 open PRs swept, 0 not ours, 32 disqualified. It independently reproduced the two DIRTY-with-green-tick PRs (#2970, #2971, both No |
|
Confirming the fix worked, so the loop is closed on the record: this PR now has real CI. The conflict was the cause: GitHub cannot compute Green here now means what it says. |
2 participants
A
CodeRabbit: passdoes not mean the diff was reviewed. Three states render as the same green tick:Under rate limiting the third is green on every open PR at once, so "all our PRs show CodeRabbit passing" is a symptom rather than reassurance.
scripts/check-coderabbit-review.mjstells them apart.Measured right now, on this repo
Seventeen — independently matching the count reported on #2971 from a different direction.
Two signals, and neither works alone
The rate-limit HTML sentinel alone gives false positives. #2899 carried it verbatim — "Review limit reached", "Next review available in 51 minutes", a Run ID — and yet
reviewThreadsheld two genuine inline findings timestamped four minutes after the notice, flatly contradicting the 51-minute wait. A review did run; the summary comment was never rewritten.So the sentinel is conclusive only when the inline thread count is also zero. The script reports that case separately as
STALE-SUMMARYrather than calling the PR unreviewed.Reproduced against known cases:
One trap worth knowing regardless of this script
gh pr view --json commentsdoes not return inline review threads at all. Any detector built on it concludes "no findings" on every PR. The count has to come fromreviewThreadsvia GraphQL.Design
Classification is a pure function in
scripts/lib/coderabbit-review-state.mjswith 10 unit tests over synthetic bodies (no real comment text, percheck-source-text-assertions.mjs, which passes:OK (7 allowlisted, 0 new)). The script is the GitHub plumbing.An unrecognised comment returns
INCONCLUSIVEand is not counted as unreviewed — guessing there would flag real reviews and the report would stop being believed.Empty target list exits non-zero rather than printing a clean summary. This repo has shipped three scripts that exited 0 having checked nothing; this is not a fourth.
Deliberately not wired into CI. The answer depends on transient GitHub state — rate limiting clears by itself — so a required check on it would fail for reasons unrelated to the diff. It is a thing you run before claiming a PR is reviewed.
🤖 Generated with Claude Code
Added after review: three signals, and a review can be real but stale
The two-signal version described above shipped with both halves of the bug it exists to catch, found in review.
False NO-REVIEW.
bodies.length === 0short-circuited beforeinlineThreadCountwas consulted, so{bodies: [], inlineThreadCount: 3}returned NO-REVIEW. Three inline threads mean a review ran and left findings. CodeRabbit can post inline findings while its summary comment is absent or deleted, so this input occurs in practice. NO-REVIEW now requiresbodies.length === 0 && threads === 0.False REVIEWED after a push. There was no timestamp anywhere in the pipeline — not in the GraphQL selection, not in the REST fetch, not in the classifier's input — so a thread posted before the latest push classified as REVIEWED for commits the bot never saw. The classifier now takes
latestReviewAtandheadCommitAt, and evidence older than the head commit yieldsPRE-PUSH-REVIEW,reviewed: false.Named distinctly from
STALE-SUMMARYdeliberately: that state isreviewed: true, and two similar names in the same output column would be the next trap.Which way to fail
A false NO-REVIEW is loud and self-correcting — someone looks, sees the review, moves on. A false REVIEWED is silent and terminal: nobody looks again, and the tool has certified the thing it exists to catch. So every ambiguous input resolves to unreviewed.
That is a decision with tests behind it, not an accident of
undefined.parseInstantreturns null, never NaN, because NaN comparisons are false in both directions and a bare<would reach the reassuring branch by accident. Missing, empty or unparseable on either side yieldsUNDATED,reviewed: false.INCONCLUSIVE— a CodeRabbit comment with none of the markers — also flipped fromreviewed: truetofalsefor the same reason.The REST fetch reads
created_at, notupdated_at: CodeRabbit rewrites summaries in place, so an edit time would date an old summary as fresh.created_atcan only understate.Both mistakes are guarded
a review after the head commit does cover itpins a genuinely-reviewed-at-head PR as REVIEWED. It passes against the old classifier too — it is the guard, not the fix.Live run over three real PRs:
#2957 is the case that motivated the third state: neither "unreviewed" nor "reviewed" is the right answer for it.
19 tests, 0 fail (was 10). Reverting the production file to its pre-fix version reds 8 of them.
Also wired into CI — the test file was connected to nothing and had never run.