Skip to content

fix(2284): the gate tolerates a review that never came, and says so - #356

Merged
LukasWodka merged 4 commits into
developfrom
fix/2284-tolerate-a-review-that-never-came
Aug 27, 2026
Merged

fix(2284): the gate tolerates a review that never came, and says so#356
LukasWodka merged 4 commits into
developfrom
fix/2284-tolerate-a-review-that-never-came

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Two absences were one verdict, and they mean opposite things

PENDING covered both "Bugbot is still running" and "Bugbot never showed up", and the timeout failed both identically. They are not the same claim:

  • a check that started and never finished is a review that broke — worth blocking on, and rare;
  • a check that never appeared is Bugbot dropping the PR — something this repo cannot fix, retry, or wait out.

This splits them. UNCLAIMED is a new verdict; PENDING keeps its meaning and keeps blocking.

The measurement

Human-authored PRs on 2026-08-26, all well past the measured p50 164s / max 635s:

12:22 backend#2584 reviewed (2 min)
13:23 e2e-test-agent#273 NONE (2h+)
14:29 .github#349 NONE (57 min)
14:31 .github#350 NONE (55 min)
14:41 .github#351 reviewed (3 min)
14:46 .github#352 NONE (40 min)
14:49 .github#353 NONE (37 min)
14:54 .github#354 NONE (32 min)

6 of 9 never got a check at all — and #351, opened between two that got nothing, was reviewed in three minutes. Not latency, not the seat limit, not the author. backend#2114 closed COMPLETED saying "no discriminator survives the data"; the drop it describes is still live.

bugbot run cannot recover it either — Cursor refuses it on a seat limit, and the App will not be given one (decision, 2026-08-26).

So requiring this context while failing UNCLAIMED would block roughly two thirds of all PRs for the full 900s and then fail them, with no available remedy. The gate would look broken while behaving exactly as written.

This is a real weakening, and it is stated rather than hidden

UNCLAIMED exits 0 so the context can be required. It is not a pass:

  • its own banner — Bugbot review gate: UNREVIEWED (not blocked, not clean)
  • a summary line saying the head is unreviewed and "read the diff yourself before approving"
  • the run says explicitly that it is "recording that nothing looked at it, and declining to block on something no one here can fix"

An open finding still fails. A claimed-but-unfinished check still fails. The asymmetry is the whole change.

Also corrected

The vocabulary comment said "Only PENDING is waitable", which this makes false. main now readsWAITABLE rather than restating it — the same rule-9 discipline that comment was written to enforce.

Tests

82 selftest assertions, four of them through main, because the exit code is the behaviour and evaluate alone cannot show it. The split is asserted in both directions — a running check must not read as never-claimed, and vice versa.

Registry: 1 stale marker fixed, 5 mutations added — the collapse each way, the tolerance leaking onto PENDING, the tolerance removed, and the banner reading as a pass. 35 mutations: 0 stale, 0 uncaught.

The banner mutation came back UNCAUGHT first time: every other assertion checks a verdict or an exit code, and the banner is neither — yet it is the one line a skimmer reads. Pinned in a follow-up commit.

What this does and does not unblock

It makes arming the required context possible. It does not make Bugbot reliable — the drop is Cursor-side and #2114 never characterised it. Arming remains a separate, deliberate step.

Part of tracebloc/backend#2284


Note

Medium Risk
Changes merge-blocking CI behavior for PRs where Bugbot never posts a check (now non-blocking with UNREVIEWED reporting) while preserving fail-closed paths for open findings and unfinished claimed checks; incorrect ordering or verdict wiring could let serious findings slip or block healthy PRs.

Overview
The Bugbot review gate splits two kinds of “no verdict yet” that used to share PENDING: UNCLAIMED (no Cursor check run on the head) vs PENDING (check started but not terminal). Both stay in WAITABLE while polling; at the deadline UNCLAIMED exits 0 so the context can be required without blocking merges when Bugbot never claims the head, while PENDING still exits 1 for a stuck/broken review.

That tolerance is explicit, not a silent pass: summaries use an UNREVIEWED banner and prose that the head was not reviewed. evaluate now applies the severity threshold before classifying the head, so an unclaimed head with an open High from a prior review still fails instead of being laundered through the unclaimed exit path. Findings output is centralized in _finding_lines.

Selftests assert main exit codes, banner text, and laundering cases; the mutation harness adds regressions for collapsing the split, leaking tolerance onto PENDING, and the UNREVIEWED banner reading as pass.

Reviewed by Cursor Bugbot for commit c327f1f. Bugbot is set up for automated code reviews on this repo. Configure here.

LukasWodkaand others added 3 commits August 27, 2026 07:04
Two absences were one verdict, and they mean opposite things.
`PENDING` covered both "Bugbot is still running" and "Bugbot never showed
up", and the timeout failed both identically. A check that STARTED and never
finished is a review that BROKE -- worth blocking on. A check that never
appeared is Bugbot dropping the PR, which this repo cannot fix, retry, or
wait out.
Measured 2026-08-26 on human-authored PRs, well past p50 164s / max 635s:
six of nine never got a check at all -- .github#349 (57 min), #350 (55),
#352 (40), #353 (37), #354 (32), e2e-test-agent#273 (2h+) -- while #351,
opened BETWEEN two of them, was reviewed in three minutes. Not latency, not
the seat limit, not the author. backend#2114 closed COMPLETED saying "no
discriminator survives the data"; the drop is still live.
`bugbot run` cannot recover it: Cursor refuses on a seat limit and the App
will not be given one (decision, 2026-08-26).
So requiring this context while failing UNCLAIMED would block roughly two
thirds of PRs for the full wait and then fail them with no remedy -- the
gate would look broken while behaving exactly as written.
WHAT THIS IS NOT: it is not a pass. UNCLAIMED exits 0 so the context can be
required, and every other word says the head is UNREVIEWED -- its own banner,
its own summary line, and an explicit "read the diff yourself". The honest
report is "nothing looked at this", not "this is clean".
PENDING still blocks, and that asymmetry is the whole change.
Also corrected: the vocabulary comment said "Only PENDING is waitable",
which this makes false. `main` now READS `WAITABLE` rather than restating it.
79 selftest assertions (4 new through `main`, since the exit code is the
behaviour and `evaluate` alone cannot show it). Registry: 1 stale marker
fixed, 5 mutations added -- the collapse in both directions, the tolerance
leaking onto PENDING, the tolerance removed, and the banner reading as a
pass.
Part of tracebloc/backend#2284
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The mutation `the UNREVIEWED banner reads as a pass` came back UNCAUGHT.
Every other assertion in this suite checks a verdict or an exit code, and
the banner is neither -- it is the one line a human skimming the summary
actually reads, and the only thing standing between "exits 0" and "looks
clean".
Three cases: the UNCLAIMED headline says UNREVIEWED, does not contain the
word pass, and a genuine pass still does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
E402/E401 from the banner test added in the previous commit. `make lint` is
where I should have run it before pushing, not after.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Aug 27, 2026
Comment threadscripts/bugbot-gate.py
Bugbot on #356, High: `evaluate` returned UNCLAIMED as soon as the head had
no Bugbot check, without applying the open-finding threshold to the threads it
had ALREADY loaded one block earlier. So the tolerance this PR adds for a
dropped review would launder a finding that had already come -- review head A,
get a High, push head B, Bugbot never claims B, and the gate reports
UNREVIEWED-but-not-blocked over an open High at exit 0.
The same hole was in PENDING, and fixing only the branch Bugbot named would
have left its twin. So `blocking` is now computed before `bugbot_check` is
consulted at all, and an absence of either kind with a blocking finding is a
FAIL that names the finding and does NOT wait -- the answer would not change.
Both absence verdicts now also state that the threshold was checked, so the
report says which question was answered.
The tolerance itself is intact and asserted: a Low on an unreviewed head is
still UNCLAIMED, exit 0, and so is a resolved High -- otherwise the remedy the
FAIL message names would not clear it.
Not a shipped bug: `required_conversation_resolution` is true on every train
branch, so the merge was blocked anyway. It was a gate naming the wrong reason,
which is the failure mode this file exists to prevent.
One renderer (`_finding_lines`) feeds both paths, because two would drift and
one of them would quietly stop saying OPEN (rule 9).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

set-status / closing-ref is red and is being left red deliberately, not overlooked.

Its only satisfying form is a closing keyword (it reads closingIssuesReferences, which nothing but Closes/Fixes/Resolves populates). This PR does not finish backend#2284 — arming the required context is the ticket, and the body says outright that this only makes arming possible. So the two available remedies are to add a false Closes, or to drop (2284) from the title and pass the check by deleting the traceability it exists to enforce. Both make the record worse than the red does.

Filed as backend#2616 with the suggested shape (accept a non-closing link, keep the refusal for a title that links nothing at all). closing-ref is not a required context on develop — measured today, the required set is actionlint, gate, quality / *, selftests — so it blocks nothing here.

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit c327f1f. Configure here.

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The asymmetry is right and the thing that makes it safe is the ordering, which I checked rather than took: if not claimed and blocking: return FAIL comes before the check is None branch, so UNCLAIMED cannot swallow an open finding carried over from an earlier review. And "The absence of a review on THIS head is tolerated; the findings are not" plus "This does not wait for the missing review: the answer would not change" means it neither blocks on the unfixable nor burns 900s on a foregone conclusion.

Both absence messages asserting "checked before this verdict, not after" is the detail I'd have asked for and wouldn't have expected — the evaluation-order guarantee is in the log line, so someone reading a run can see it held rather than trusting that it did.

The mutation targets are aimed at the load-bearing line, not around it. 'if not claimed and blocking:''if False and not claimed and blocking:' is the one that matters: it makes "UNCLAIMED + an open High exits 1" a proof rather than a hope. The banner mutation (UNCLAIMED"pass") and WAITABLE = frozenset({PENDING}) cover the two other ways this goes quietly wrong, and the note about which targets would be decorative without the others is the right kind of paranoia about vacuous mutations.

Correcting myself on the ticket. My contribution to #2284 was that Bugbot posts no check run on bot-authored PRs, and I framed the drop as author-conditional. Your data says otherwise and is better: 6 of 9 human-authored PRs got nothing, and #351 — opened between two that got nothing — was reviewed in three minutes. So it isn't the author, and backend#2114's "no discriminator survives the data" was right in a way my narrower reading wasn't. Worth having that on the record here, since the fix is correctly scoped to the general case rather than the one I described.

Reading WAITABLE in main instead of restating "only PENDING is waitable" is the same discipline the comment existed to enforce — good catch that the change falsified its own doc.

Green, no threads. 👍

@LukasWodka
LukasWodka merged commit 1f6f1c6 into developAug 27, 2026
12 checks passed
@LukasWodka
LukasWodka deleted the fix/2284-tolerate-a-review-that-never-came branch August 27, 2026 05:46
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LukasWodka@saadqbal