Skip to content

feat(2114): a missing Bugbot review is a finding, not a clean PR - #282

Merged
LukasWodka merged 6 commits into
developfrom
feat/2114-bugbot-absent-guard
Aug 19, 2026
Merged

feat(2114): a missing Bugbot review is a finding, not a clean PR#282
LukasWodka merged 6 commits into
developfrom
feat/2114-bugbot-absent-guard

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Closes backend#2114.

The failure, measured

Cursor Bugbot's auto-trigger silently drops PRs. On 2026-08-17, five open PRs across three repos carried no Cursor Bugbot check at all, while PRs opened minutes before and hours after were reviewed normally.

No discriminator survived:

hypothesiscounter-example
diff size1-file/66-line PR reviewed; 1-file/29-line PR not
file typetwo 3-file Go diffs, same repo — one reviewed, one not
repothree repos affected, each with reviewed PRs too
timecli#521 10:54 absent, client#738 10:55 present
quotaPRs opened hours later reviewed fine
author / draftall human, all non-draft

Posting bugbot run started a review within a minute and both came back clean. The reviews were not failing — they were never starting.

Why it belongs in this watcher specifically

bricked-prs.py's own header already makes the argument:

A required status check that never reports... is the one CI failure mode with NO red signal at all — nobody is notified, and no reviewer sees a problem, because there is no failure, only an absence. So it needs a watcher; nothing inside a PR can detect it.

That describes this bug exactly, one class over — checks then, the reviewer now.

A per-PR required check would be the wrong shape: it would sit pending for the minutes Bugbot legitimately takes, and a check that is pending-by-design is precisely what this repo refuses to ship.

Four rules, each earned from the measurement

  • A skipped Bugbot counts as present. It ran and decided; that is a verdict. Only total absence is the silent drop.
  • Bot authors are exempt, keyed on GitHub's [bot] suffix rather than a hand-kept list that would go stale. dependabot[bot]'s absence was the one legitimate case in the sample.
  • Same young-head rule as the required-check case, and an unreadable age reads as young — a false finding is what makes a report ignorable.
  • Reported even when every required check is present, because that is exactly the case that renders as a completely clean PR.

Its own label, and this is the part I nearly got wrong

The renderer sent anything not conflicted to BRICKED. A missing review is not a bricked PR — the PR merges perfectly well. Labelling it BRICKED would send the reader to branch protection for something one comment fixes: the same true-count-false-name defect this file already avoids for conflicted.

It now renders as UNREVIEWED, names bugbot run as the remedy, and the summary says the quiet part:

a bricked PR cannot merge; an unreviewed one merges perfectly well, which is the worse of the two.

That rendering is asserted from the source, because it lives in main() and no decision-table case reaches it.

Verification

Six mutations, all caught:

mutationresult
the Bugbot check removed2 fail
bot exemption disabled1 fail
young-head guard dropped1 fail
cause collapsed into never-reported2 fail
label collapsed to BRICKED1 fail
remedy text removed1 fail

bricked-prs selftest 21 passed (was 14) · make selftests41 passed · actionlint clean.

The default PR fixture now carries a Bugbot row, so a normal PR is the baseline and absence has to be constructed deliberately — otherwise every existing case would trip the new finding and the two would be untestable apart.


Note

Low Risk
Changes are confined to the offline bricked-PR audit script and its selftests; behavior is heavily specified in tests with no production runtime or auth surface.

Overview
Extends bricked-prs.py so PRs with no Cursor Bugbot check are surfaced even when all required status checks are green—addressing silent auto-trigger drops (backend#2114).

Bugbot detection: Fetches author on PR list; flags human PRs missing the Bugbot context after the same 60-minute head-age grace. Bot authors are exempt via author.is_bot (not login suffix). bugbot-absent is reported separately from missing required checks, including on branches with no required checks (removed early continue on empty protection).

Audit loop: Classifies unreviewed / missing before head_age_minutes so healthy PRs pay zero age API calls and dual findings share one lookup. Undateable heads emit COULD NOT AUDIT errors instead of silent young skips. rollup_truncated (≥100 rollup entries) refuses audit only when an absence would be reported—not for healthy PRs at the cap.

Output: CLI/GitHub summary use UNREVIEWED (not BRICKED) and print bugbot run as the fix. Selftests grow coverage for bot exemption shape, lookup counts, rollup pagination, and rendered remedy text.

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

…kend#2114)
Cursor Bugbot's auto-trigger silently drops PRs. Measured 2026-08-17: five open PRs
across cli, release-train and .github carried no `Cursor Bugbot` check at all, while
PRs opened minutes before and hours after were reviewed normally.
No discriminator survived. Not diff size -- a 1-file/66-line PR was reviewed and a
1-file/29-line one was not. Not file type: two 3-file Go diffs in the same repo, one
reviewed, one not. Not repo, not time (10:54 absent, 10:55 present), not quota (PRs
hours later were fine), not author or draft state. Posting `bugbot run` started a
review within a minute and both came back clean -- the reviews were not failing,
they were never starting.
WHY IT BELONGS IN THIS WATCHER. bricked-prs.py exists because a required check that
never reports "is the one CI failure mode with NO red signal at all -- nobody is
notified, and no reviewer sees a problem, because there is no failure, only an
absence. So it needs a watcher; nothing inside a PR can detect it." That paragraph
describes this bug exactly, one class over: checks then, the reviewer now. A per-PR
required check would be wrong here -- it would sit pending for the minutes Bugbot
legitimately takes, which is the thing this repo refuses to ship.
Four rules, each earned from the measurement:
- a SKIPPED Bugbot counts as PRESENT. It ran and decided; that is a verdict. Only
total absence is the silent drop.
- bot authors are exempt, keyed on GitHub's `[bot]` suffix rather than a list of
names that would go stale. dependabot[bot]'s absence was the one legitimate case
in the sample.
- the same young-head rule as the required-check case, and an unreadable age reads
as young: a false finding is what makes a report ignorable.
- reported EVEN WHEN every required check is present, because that is precisely
the case that renders as a completely clean PR.
AND ITS OWN LABEL. The renderer sent anything not `conflicted` to "BRICKED", so a
missing review would have been announced as a bricked PR -- sending the reader to
branch protection for something one comment fixes. It renders as UNREVIEWED, names
`bugbot run` as the remedy, and the summary now says the quiet part: a bricked PR
cannot merge, an unreviewed one merges perfectly well, which is the worse of the two.
That rendering is asserted from the source, because it lives in main() and no
decision-table case reaches it.
Six mutations, all caught: the check removed, the bot exemption disabled, the
young-head guard dropped, the cause collapsed into `never-reported`, the label
collapsed to BRICKED, and the remedy text removed.
Verified: bricked-prs selftest 21 passed (was 14); `make selftests` 41 passed;
actionlint clean.
Closes backend#2114
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment threadscripts/bricked-prs.py Outdated
…Bugbot, .github#282)
High severity and correct. The exemption tested `login.endswith("[bot]")`, which
NEVER MATCHES: `gh pr list --json author` returns
{"is_bot": true, "login": "app/dependabot"}
The `owner[bot]` form is what the REST API and the web UI show; it is not what this
command returns. So every Dependabot PR without a Bugbot review would have been
reported UNREVIEWED -- noise on exactly the PRs where absence is CORRECT, which is
how a new report gets triaged as broken and then ignored.
THE TEST WAS THE REAL DEFECT. It fixtured `dependabot[bot]` -- the shape I assumed
rather than the shape I measured -- so the exemption case passed while the exemption
could not fire. A fixture invented instead of measured asserts its author's belief
and nothing else. That is the failure this guard exists to catch, reproduced one
level in, in the same PR that added it.
Now keyed on `is_bot`, which is in the JSON payload already being fetched, so it
costs nothing and cannot drift the way name-matching does.
Three cases, and two of them exist to stop it drifting back:
- a bot-authored PR with no Bugbot is exempt (the real shape, is_bot=true)
- a login that merely LOOKS bot-ish (`dependabot[bot]`) but is NOT flagged is_bot
is STILL reported -- asserting that the name is not what exempts
- `is_bot` alone exempts whatever the login looks like (`some-app`)
Mutation-proved both ways: reverting to login-suffix matching reddens 3 cases,
breaking the field name reddens 2.
23 passed / 0 failed (was 21). make selftests green.
Refs backend#2114
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@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 is_bot fix in 7c7d9ae is right — I'd independently confirmed app/dependabot on backend and cli before you pushed it, and keying on the field rather than the login form is the correct call. Polarity and the young-head rule read fine, and Cursor Bugbot is the exact check-run name in the rollup (verified on this PR and on cli/backend), so detection is anchored on the real thing. Two things left, below.

One more that isn't on a diffed line: gh fetches at most 100 rollup contexts and says nothing when it truncates. client#746 is already at 77 (81 check runs on the sha). Under the old polarity a truncated rollup gave a false BRICKED; under the new one it gives a false UNREVIEWED on a repo that's within ~20 checks of the cap — worth the same explicit refuse-rather-than-guess treatment you gave PR_LIST_LIMIT.

Comment threadscripts/bricked-prs.py Outdated
Comment threadscripts/tests/bricked-prs-selftest.py Outdated
Comment threadscripts/bricked-prs.py Outdated
Comment threadscripts/bricked-prs.py Outdated
LukasWodkaand others added 2 commits August 18, 2026 16:44
…282)
Four findings, all correct. Two are structural and two are about my own tests
proving less than I claimed.
1. THE CHECK WAS IN THE WRONG PLACE (saadqbal + Bugbot). It sat below
`if not required: continue`, so on a branch with zero required contexts
`open_prs` was never called and a missing review was STRUCTURALLY
UNREPORTABLE -- the watcher going quiet on exactly the branches with the least
protection, and saying nothing about having skipped them. The reviewer check does
not depend on branch protection, so the early continue is gone and the
required-check block carries its own `if required:` guard instead.
Selftest case 8 ("a branch requiring nothing produces no findings") could not
catch it, because the default fixture now carries a Bugbot row -- so the case
passed for a reason unrelated to what it tests. Two new cases: a zero-required
branch must still report a missing review, and must stay quiet when the review is
present.
2. MY REMEDY ASSERTION WAS INERT (saadqbal). It tested `'bugbot run' in _src`, and
that phrase also appears in this module's comments -- so it matched whether or not
the remedy was ever PRINTED. Deleting the print left the suite green.
My mutation missed it for a worse reason: it replaced EVERY occurrence of the
phrase, comments included, so it failed for the wrong reason and read as
coverage. A mutation that is too broad proves nothing about the line it names.
Now asserts the rendered string, and a targeted deletion of just the print
reddens it.
3. ONE AGE LOOKUP PER PR (saadqbal). A PR missing both a review and a required
context called `head_age_minutes` twice -- two uncached API calls -- and made this
function's own docstring ("one or two API calls per candidate") stale. Computed
once and reused by both checks.
Mutation-proved both corrections: re-gating the reviewer check on `required`
reddens 1 case, and deleting ONLY the printed remedy line now reddens 1 where it
previously passed.
25 passed / 0 failed (was 23). make selftests 41 passed.
Refs backend#2114
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`gh pr list --json statusCheckRollup` asks GraphQL for `contexts(first: 100)`
and says nothing when a head carries more. A context dropped by pagination is
indistinguishable from one that never ran, so a truncated rollup reports
`Cursor Bugbot` absent on a PR Bugbot reviewed -- fail-open in the exact
direction this watcher exists to close, and confidently so.
Same treatment PR_LIST_LIMIT already gets one level up: an at-or-over-cap PR
becomes an error naming the PR and saying it was NOT audited, and is kept out
of the findings table entirely.
Not hypothetical. Measured 2026-08-18: client#746 carries 77 distinct contexts
(81 check runs; the rollup is latest-per-context, so the two numbers differing
is correct and not truncation). 23 away from the cap.
The constant is gh's page size, not a number derived from an observed failure
-- I could not construct a >100 head. Said plainly in the comment rather than
dressed up, and the selftest builds a synthetic 100-context rollup so the
branch is proven reachable regardless.
The fixture is built so that WITHOUT the guard it yields two findings
(bugbot-absent + never-reported), both confident and wrong, so a pass here
cannot come from a harmless scenario. Mutation-proved three ways: removing the
guard, flipping >= to >, and zeroing the cap each redden the suite.
Raised by saadqbal on #282.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Third finding addressed in 91b0e5b — you were right that it deserved the same treatment as PR_LIST_LIMIT, and it is worse than that one because it fails in the direction this file exists to close.

Measured your client#746 number, and it needs a caveat. The rollup shows 77 contexts; REST reports 81 check runs on that sha. The gap is not truncation — the rollup is latest-per-context, so 81 runs collapsing to 77 names is correct. 77 distinct contexts is the real figure, and it is 23 from the cap.

The guard: at-or-over ROLLUP_CONTEXT_CAP = 100, the PR becomes an error naming it and saying it was NOT audited, and is skipped before either check. >= rather than > because at exactly 100 there is no way to tell 100 contexts from 140, and "cannot tell" is a finding.

Where I could not derive it. The constant is gh's page size, not something I observed failing — I could not construct a >100-context head. That is said plainly in the comment rather than dressed up as a measurement. What I could do is make the branch provably reachable: the selftest builds a synthetic 100-context rollup, and the fixture is deliberately shaped so that without the guard it produces two findings (bugbot-absent + never-reported), both confident and both false. A pass there cannot come from the scenario being harmless.

Mutation-proved three ways, each reddening the suite: guard removed (27/2), >=> (27/2), cap zeroed (18/11). The last one is what keeps the boundary meaningful — without the one-under-cap test the guard could be >= 0 and the first test would still pass.

make check green, 41 selftests.

Comment threadscripts/bricked-prs.py Outdated
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@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!

1 issue from previous review remains unresolved.

Fix All in Cursor

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

Reviewed by Cursor Bugbot for commit 4366ecf. Configure here.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

@saadqbal re-requesting review — your CHANGES_REQUESTED predates the fix, so this reads as blocked when it isn't.

Timeline: your review landed 2026-08-18 14:33Z; the third finding (the rollup page-size guard) was pushed in 91b0e5b afterwards. Current head 4366ecf:

every checkgreen — gate, audit, selftest, selftests, actionlint, all five quality/*
Cursor BugbotNEUTRAL (terminal, no findings)
unresolved threads0

All three findings addressed: the is_bot fix you'd independently confirmed, the empty-required-set continue that made a missing review structurally unreportable on the least-protected branches, and the rollup truncation guard. One correction to your client#746 figure, on the thread: the rollup shows 77 distinct contexts, not 81 — the rollup is latest-per-context, so 81 check runs collapsing to 77 names is correct rather than truncation. 77 is still 23 from the cap, so the finding stands.

This one is also now on the critical path: #284 (the last of the 19 add-to-kanban repos) is open, and the #1979 stale-backlog reusable branch was cut from this branch — so its diff isn't clean until this merges.

Two review findings on #282 pull in opposite directions at the same line, and
the ORDER is what reconciles them -- no cache needed.
saadqbal: calling head_age_minutes inside each check cost TWO uncached API
calls for a PR missing both a review and a required context.
Bugbot: hoisting it above both fixed that and broke the other half -- every
HEALTHY PR then paid a lookup whose answer nothing consumed, on the
very shared credential backend#2036 exists because it was measured
exhausted (client-runtime run 31776053792).
Deciding what looks wrong BEFORE dating the head satisfies both: at most one
lookup per PR, none at all for a PR with nothing to report. It also restores
this loop's agreement with head_age_minutes's own docstring, "Called ONLY for a
PR that already looks bricked" -- which the hoist had quietly made false.
AN UNDATEABLE HEAD IS NOW UNKNOWN, NOT YOUNG. This is the substantive half of
Bugbot's finding and it changes behaviour. It used to fold into `young` and drop
the PR silently, which is the exact shape this watcher exists to remove: a
candidate that looks wrong, cannot be judged, and produces nothing at all.
head_age_minutes returns None only when a read FAILED -- an empty suite list
falls back to the commit date -- so None is a real API failure, and a persistent
one meant the finding never surfaced. Now it is an error naming the PR and what
it looked like, out of the findings table. Same treatment as a truncated rollup.
A genuinely young candidate is still skipped silently, and that is now asserted:
without it this fix could have turned every young PR into an error.
THE CALL COUNT IS ASSERTED DIRECTLY. Neither reviewer's requirement is visible
in a finding count, so counting findings would have proved nothing about either.
The counter wraps head_age_minutes in the test rather than hiding in the helper,
because a test that cannot see the call cannot assert when it happens.
Also fixed: the "unreadable check-suites" test's own description said a 502
"must read as young", which this commit makes false. Updated in the same commit
rather than left as a test whose name lies about what it pins.
Mutation-proved, each reddening the suite:
remove the candidates-only early exit 32/1 healthy PR pays zero
the OLD polarity (undateable -> skip) 32/1 undateable is UNKNOWN
undateable judged as OLD 31/2 + no finding on undateable
remove the young-head skip 30/3 three young-head cases
selftests 33/33 - make check green.
Addresses Bugbot on #282.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@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 6bf6eac. Configure here.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

@saadqbal — the rollup-truncation gap you flagged is closed in 4366ecf: a rollup at gh's 100-context page size is now classified UNKNOWN — an error that names the PR and keeps it out of the findings table — the same refuse-rather-than-guess treatment PR_LIST_LIMIT gets one level up. A context paginated away is indistinguishable from one that never ran, so under the new polarity it would give a false UNREVIEWED, failing open in exactly the direction this watcher exists to close. Measured client#746 at 77 contexts, 23 from the cap. The selftest builds a synthetic 100-context rollup and is mutation-proved three ways: remove the guard, flip >= to >, zero the cap.

The other points are in: is_bot keyed on the field not the login form (7c7d9ae), reviewer check no longer gated on branch protection (31ecc2c), classify-first-then-date-the-head (6bf6eac). CI green, Bugbot clean on the head. Re-requesting your review to clear the change request.

@LukasWodka
LukasWodka requested review from aptracebloc and removed request for saadqbalAugust 19, 2026 11:47

@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.

Re-derived the control flow at 6bf6eac rather than spot-checking the fixed case, and "classify first, date the head after" is the right shape — one lookup for a candidate, none for a healthy PR, no cached flag to go stale, and nothing left over from the young hoist. Every cell I walked lands where it should: zero-required + human + no Bugbot + old head reports bugbot-absent; zero-required + Bugbot present stays quiet and costs no API call; a bot PR is still bricked-checked for required contexts; a skipped Bugbot still counts as present, since present_contexts reads names and ignores conclusions; missing = ... if required else [] keeps the required block from firing on an empty set. The one PR that produces two findings (missing review + missing check) already did before and the suite asserts it deliberately, so it is not new double-reporting. Detection is unchanged and still keys on the Cursor Bugbot check-run name in the rollup, not the cursor review author.

Ran the suite off the PR head: 33 passed. Deleting the fix: comment ... print now reddens it (32 passed, 1 failed), so the remedy assertion is genuinely anchored to the printed line this time.

One blocking thing on the truncation guard, below. Three smaller notes that aren't on diffed lines:

  • head_age_minutes's docstring at line 220 still says "the caller treats an undateable head as young and skips". 6bf6eac made that false — the caller now turns None into an error. Same class of stale statement you fixed one paragraph up in this push.
  • The module exit-code contract at lines 49-51 is now stale in both directions: 1 is no longer only "bricked", and 2 now covers a truncated rollup and an undateable head, not just "an unreadable repo, branch, or PR list".
  • The fork / docs-only escape hatch I raised last round isn't addressed by this push. Bugbot doesn't run on fork PRs, and the public repos can get them, so those land as UNREVIEWED with no override. Still not a blocker — just noting it hasn't silently dropped off the list.

Comment threadscripts/bricked-prs.py Outdated
saadqbal, second round on the same guard. It was right and placed one step too
early, and the cost was not one row -- it was every run.
TRUNCATION IS MONOTONE. `gh` pages the rollup at 100 and only ever REMOVES
names, so `unreviewed` and `missing` can be falsely TRUE and never falsely
false. A PR that already looks healthy on the partial list would look healthy on
the full one; there is nothing to be unsure about. Refusing it was a guard firing
on the innocent case -- the shape this repo deletes rather than fixes.
AND ONE HEALTHY PR POISONED THE FLEET RUN. `main()` returns 2 when `errors` is
non-empty, BEFORE `return 1 if findings`. So a single healthy big-rollup PR
anywhere in the 19 repos turned every four-hourly run into "could not evaluate"
and demoted the real findings to a second-class exit code, until someone raised
the cap. That is precisely the report-gets-ignored failure this file argues
against, arriving through the guard meant to prevent it.
Measured before and after, on a 100-context PR with Bugbot AND `build` present:
before findings=[] errors=['... 100 rollup contexts ... NOT audited']
after findings=[] errors=[]
The still-sick case at the cap continues to refuse, so the guard did not lose
its job -- it lost its false positives.
TWO SIDE EFFECTS, both wanted. The error now names WHICH absence is uncertain
("missing review" vs "missing context"), which it could not do above the
classification -- and that dependency makes the old placement structurally
impossible rather than merely discouraged: moved back up, `unreviewed` is
undefined and the module raises. It also now sits before the age lookup, so a PR
it refuses costs no API call.
Mutation-proved, each reddening the suite:
guard moved back above the classification 33/1 healthy-at-cap is silent
guard removed entirely 32/2 the two truncation cases
>= flipped to > at the cap 32/2 the two truncation cases
The first needed the conditional wording dropped as well, because the naive move
crashes instead of passing. A mutation that crashes proves nothing about the
test, so it was made faithful to what a real revert would look like.
selftests 34/34 - make check green.
Addresses saadqbal on #282.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@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 83d8020. Configure here.

@aptraceblocaptracebloc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving at 83d8020. The guard move is correct and its reasoning holds.

The change:rollup_truncated(pr) moved from above the classification to below it. Three consequences, all good:

  • A healthy PR at the 100-context cap is now silent instead of an error. That closes a real operational problem: main() returns 2 on any non-empty errorsbeforereturn 1 if findings, so a single healthy big-rollup PR anywhere across the 19 repos turned every four-hourly run into "could not evaluate" and demoted genuine findings to a second-class exit code.
  • The refusal message can now name which absence is uncertain, which is also what makes the old placement structurally impossible — above the classification, unreviewed is undefined.
  • Refused PRs now cost zero age lookups.

The monotonicity argument is sound.present_contexts only ever shrinks under truncation, so missing = required - present only grows and unreviewed only becomes more likely true. Both can be falsely TRUE, never falsely false, so healthy-on-partial genuinely implies healthy-on-full.

I reconstructed the module and ran it: 34 passed, and reproduced the commit message's mutation table exactly — guard back above classification → 33/1, guard removed → 32/2, >=> → 32/2. Also re-probed the still-running-Bugbot case across QUEUED/null, IN_PROGRESS, SKIPPED, FAILURE, NEUTRAL and a legacy PENDING status: all produce zero findings, only true absence fires bugbot-absent. No false finding on a fresh PR.

Non-blocking, but the last one is worth taking before merge

The PR body. It becomes the merge commit message, and it now contradicts itself. Still says bots are exempt "keyed on GitHub's [bot] suffix" while the Cursor summary in the same body correctly says author.is_bot "not login suffix"; still says "an unreadable age reads as young", which is now reversed; still describes "Four rules" and never mentions the truncation cap, COULD NOT AUDIT, or classify-first. And every count is stale — the file has 34 tests against a claimed 21, and I measured the mutation numbers: young-head guard dropped → 3 fail (body: 1), bot exemption disabled → 2 (body: 1), cause collapsed → 6 (body: 2).

The boundary test doesn't pin the boundary (scripts/tests/bricked-prs-selftest.py:330-338). It's commented "One under the cap" and framed as "what makes the boundary mean something", but builds ROLLUP_CONTEXT_CAP - 2 contexts — 98, two under. Verified: mutating the comparison to >= CAP - 1 gives 34/34 green, and >= CAP - 2 also gives 34/34 green. Someone tightening the cap by one turns every 99-context PR into a permanent COULD NOT AUDIT → exit 2, with the suite fully green. One-character fix: build CAP - 1 fillers.

Three stale contracts still standing from my earlier pass, all in the class of thing this PR is about:

  • bricked-prs-selftest.py:178-180 — comment says the exemption is keyed on the [bot] suffix; the code keys on is_bot, and the case two lines down asserts the login form is not what exempts.
  • bricked-prs.py:220head_age_minutes's docstring still ends "the caller treats an undateable head as young and skips"; the caller errors and reports NOT audited.
  • bricked-prs-selftest.py:109-113 — case 3 passes for a different reason than it documents. Proved inert: reverting age is None to a silent continue fails only the dedicated test at :294; case 3 stays green. not f and len(e) == 1 fixes it, or delete it as superseded.

Smaller: the GITHUB_STEP_SUMMARY line at :517 still says only "No PR is missing a required context" while stdout says "or a review" — the Actions summary under-claims what was audited. LABELS is still local to main(), so a change to the dispatch key alone leaves both source-greps green while nothing renders (hoisting it and asserting bp.LABELS[f[0]["cause"]] == "UNREVIEWED" against a real finding closes it). The ROLLUP_CONTEXT_CAP comment still names no operator escape hatch for a genuinely-sick PR stuck above the cap. And there's a stray double blank line at :294-295 where the guard was cut out — ruff doesn't select E3, so nothing catches it.

For the record on review state: the CHANGES_REQUESTED currently showing is @saadqbal's second review, filed 11:52:44Z against 6bf6eac — a live ask at the time, answered by this commit four minutes later. Not the 08-18 review persisting. All 7 threads are resolved.

— drafted with Claude Code

@LukasWodka
LukasWodka merged commit f29a127 into developAug 19, 2026
11 checks passed
@LukasWodka
LukasWodka deleted the feat/2114-bugbot-absent-guard branch August 19, 2026 12:54
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

/fr-pass

Best-effort FR passed (triage; behavioral evidence limited while e2e journey red — backend#2206). Advancing to Ready for prod.

LukasWodka added a commit that referenced this pull request Aug 20, 2026
Bugbot, High, and it makes the previous commit worse than a no-op. `read_repo` read
`meta.get("release_train")`, but production `meta` comes from `list_active_repos`,
which returns ONLY:
{"visibility": ..., "default_branch": ...}
No `release_train`. So the flag was always None -> False, EVERY repo was audited on
its default branch, and develop-first was silently deleted for the train repos it
exists for. Today that happens to be harmless -- every train repo currently defaults
to `develop`, measured -- but the policy was gone, and the next repo to default to
main/master would have under-reported work in flight with nothing saying so.
THE SELFTEST PASSED BECAUSE THE FIXTURE CARRIED A KEY PRODUCTION NEVER SETS. I added
`release_train: True` to META in the same commit that started reading it from there.
A fixture richer than the real payload is a test asserting its author's assumption,
and this is the THIRD time that shape has appeared in this epic -- `is_bot` on #282,
the look-alike action name on #287, this.
THE FIX IS A PARAMETER, NOT A LOOKUP. `on_train` is required and positional, so a
caller that forgets it raises TypeError; defaulting it to False would reproduce the
bug with better manners. `main()` passes `bool(entry.get("release_train"))` from the
INVENTORY row -- schema-required, and cross-checked against release-train/repos.yml
by load_release_train -- so the ref this guard reads is derived from a fact it
independently verifies.
META IS NOW EXACTLY THE PRODUCER'S SHAPE, and a case asserts that by reading
`list_active_repos`'s own source rather than a list written in the test: it writes
`visibility` and `default_branch` and nothing else. That case reddens if the producer
starts setting `release_train`, which is the only honest way to keep the two in step.
AND THE WIRING IS PINNED SEPARATELY, because the behavioural cases could not see it.
They call read_repo directly, so mutations hardcoding the argument to True or False
left all 196 green -- and #289's bug WAS the wiring, not the resolver. A source
assertion on main() closes it. Weaker than behavioural and said so in the comment:
driving main() needs the org listing, the inventory and the train file stubbed
together, which this suite has no harness for.
That assertion was ALSO wrong on its first attempt: a paren-matching regex allowing
one level of nesting could not match `bool(entry.get(...))` across two lines, so it
found nothing and failed on the correct code while every mutation "passed". An
extractor that cannot find the thing reports the same as a defect. Line-based now.
7 mutations, all applied and caught:
on_train read from meta again 194/2 the train-branch case
call site passes False 196/1 the wiring case
call site passes True 196/1 the wiring case
call site reverts to meta 196/1 the wiring case
plain develop-first restored 193/1 the non-train case
always the default branch 192/2 the train case
list_active_repos sets release_train 194/2 the producer-shape case
197 cases green. make check green.
Addresses Bugbot on .github#289.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka added a commit that referenced this pull request Aug 20, 2026
…evelop (#289)
* fix(2214): audit the branch a repo ships from, not any branch named develop
`caller-drift` preferred `develop` wherever that branch existed. `rfcs` is the only
repo in the org whose default is `main`, so the backend#2157 sweep landed there --
where rfcs actually ships -- and the audit read a `develop` that lags, then reported
drift against a change that was correctly in place:
rfcs/main 2a3a432 the default branch, where the change landed
rfcs/develop 07283e3 what the audit read
IT AGREED BY COINCIDENCE FOR MONTHS, and that is the part worth recording. Both
refs held the same blob because nothing had ever changed that file, so a wrong ref
and a right ref were indistinguishable. A resolver pointing at the wrong branch
produces a correct-looking result for exactly as long as the two branches match,
then emits a false finding the first time real work lands.
THE RULE IS NOW DERIVED, NOT LISTED. Develop-first exists because TRAIN repos
default to main/master while work lands on develop, so a default-branch audit
under-reports work in flight. A NON-TRAIN repo has no promotion pipeline: its
default branch IS where it ships, and preferring a stray `develop` audits a branch
nobody merges to. The discriminator is `release_train`, which the inventory already
carries and which `load_release_train` already verifies against
release-train/repos.yml -- so the audited ref is derived from a fact this guard
independently checks, rather than from a hand-maintained `audit_ref` field or an
exception row for rfcs. A second place to be wrong is what an override would buy.
Measured across all 19 repos before writing it: the new rule changes exactly ONE
answer, rfcs develop -> main. Nothing else moves.
`audit_branch` is renamed develop-first -> develop-first-on-train in the inventory
AND in SUPPORTED_AUDIT_BRANCH, because the schema check compares them and the
semantics changed. The one-line description above the key said "develop where that
branch exists, else the default" -- exactly the behaviour being removed -- and now
states the rule and names the discriminator.
BOTH DIRECTIONS ARE PINNED, because neither case can fail alone: assert only the
train side and the non-train path is untested; assert only the non-train side and a
resolver that always uses the default branch passes. Each reddens a different
mutation. 4 mutations, all applied and caught:
reverts to plain develop-first 193/1 the non-train case
always the default branch 192/2 the train case
the train flag read inverted 191/3 the train case + two more
audit_branch no longer enforced 193/1 the inventory positive control
AND ONE FIX TO THE SUITE ITSELF. The `_good` positive control RAISED
AssertionError when the tree was fetched off develop, which aborts the whole run --
so two of those mutations first reported "CRASH" and hid every other case they also
broke. It now returns an empty tree, failing cleanly. A suite that dies on the
first surprise cannot tell you the shape of a regression.
The fixture META also gained `release_train: True`, which is now load-bearing:
without it every existing case would have silently exercised the non-train path
while being written about develop-first.
NOT FIXED HERE, because it is not in this repo: the workspace CLAUDE.md states
"Verified 2026-08-06: every active repo's default branch is now `develop` (all
20)". That is false for rfcs and is the line a reader would use to conclude this
guard reads the right branch. It lives outside any git repo, so it cannot ride a
PR; flagged on backend#2214.
make check green: 194 caller-drift cases, all selftests, coverage gate, actionlint.
Closes backend#2214.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(2214): the train flag never reached the resolver
Bugbot, High, and it makes the previous commit worse than a no-op. `read_repo` read
`meta.get("release_train")`, but production `meta` comes from `list_active_repos`,
which returns ONLY:
{"visibility": ..., "default_branch": ...}
No `release_train`. So the flag was always None -> False, EVERY repo was audited on
its default branch, and develop-first was silently deleted for the train repos it
exists for. Today that happens to be harmless -- every train repo currently defaults
to `develop`, measured -- but the policy was gone, and the next repo to default to
main/master would have under-reported work in flight with nothing saying so.
THE SELFTEST PASSED BECAUSE THE FIXTURE CARRIED A KEY PRODUCTION NEVER SETS. I added
`release_train: True` to META in the same commit that started reading it from there.
A fixture richer than the real payload is a test asserting its author's assumption,
and this is the THIRD time that shape has appeared in this epic -- `is_bot` on #282,
the look-alike action name on #287, this.
THE FIX IS A PARAMETER, NOT A LOOKUP. `on_train` is required and positional, so a
caller that forgets it raises TypeError; defaulting it to False would reproduce the
bug with better manners. `main()` passes `bool(entry.get("release_train"))` from the
INVENTORY row -- schema-required, and cross-checked against release-train/repos.yml
by load_release_train -- so the ref this guard reads is derived from a fact it
independently verifies.
META IS NOW EXACTLY THE PRODUCER'S SHAPE, and a case asserts that by reading
`list_active_repos`'s own source rather than a list written in the test: it writes
`visibility` and `default_branch` and nothing else. That case reddens if the producer
starts setting `release_train`, which is the only honest way to keep the two in step.
AND THE WIRING IS PINNED SEPARATELY, because the behavioural cases could not see it.
They call read_repo directly, so mutations hardcoding the argument to True or False
left all 196 green -- and #289's bug WAS the wiring, not the resolver. A source
assertion on main() closes it. Weaker than behavioural and said so in the comment:
driving main() needs the org listing, the inventory and the train file stubbed
together, which this suite has no harness for.
That assertion was ALSO wrong on its first attempt: a paren-matching regex allowing
one level of nesting could not match `bool(entry.get(...))` across two lines, so it
found nothing and failed on the correct code while every mutation "passed". An
extractor that cannot find the thing reports the same as a defect. Line-based now.
7 mutations, all applied and caught:
on_train read from meta again 194/2 the train-branch case
call site passes False 196/1 the wiring case
call site passes True 196/1 the wiring case
call site reverts to meta 196/1 the wiring case
plain develop-first restored 193/1 the non-train case
always the default branch 192/2 the train case
list_active_repos sets release_train 194/2 the producer-shape case
197 cases green. make check green.
Addresses Bugbot on .github#289.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(2214): ruff — the previous push was red, and I masked it
Three ruff errors (E402 import-not-at-top, two E741 ambiguous `l`) shipped in the
commit before this one. They shipped because I ran
make check 2>&1 | tail -3 && git commit && git push
and a pipeline exits with the status of its LAST command. `tail` returned 0, so the
`&&` chain treated a failed `make check` as a pass and pushed anyway. The output was
even visible -- "make: *** [ruff] Error 1" was in the three lines I printed -- and
the chain ran on regardless.
Recorded rather than quietly fixed, because it is the same defect this repo keeps
finding in its own guards: a check whose result nothing actually reads. Mine was in
the shell, one layer out from the code.
`make check` now run with its exit status captured (exit=0), not piped.
197 selftest cases green, 41 suites green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.

3 participants

@LukasWodka@saadqbal@aptracebloc