Skip to content

feat(ci): watch for PRs bricked by a required check that never reports - #239

Merged
LukasWodka merged 32 commits into
developfrom
feat/1721-bricked-pr-watcher
Aug 13, 2026
Merged

feat(ci): watch for PRs bricked by a required check that never reports#239
LukasWodka merged 32 commits into
developfrom
feat/1721-bricked-pr-watcher

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Closes tracebloc/backend#1721 · epic tracebloc/backend#1646

The one CI failure mode with no red signal

A required status check that never runs leaves its context at "Expected — waiting for status to be reported". The PR is approved, has nothing red to point at, and is permanently unmergeable. Nobody is notified and no reviewer sees a problem, because there is no failure — only an absence. Nothing inside a PR can detect that, which is why this is a watcher.

Empirical, not static — and that is the whole design

Reasoning about "can this check ever report here" means modelling path filters × matrix expansion × reusable-workflow inputs × their defaults × if: expressions. Run across 19 repos, that approach produced three separate classes of false positive before it produced anything true.

Comparing a branch's required contexts against the contexts actually present on the PR needs none of that modelling. Required-but-absent is bricked, whatever the cause.

Required contexts come from caller-drift.py's read_protection, imported rather than reimplemented — it already unions classic protection with rulesets, and a ruleset-only branch 404s on the classic API (backend#1276). A second copy of that logic is how one of them silently stops reading half the picture.

The distinction that makes it readable, learned the hard way here

An absent context and a not-yet-started one are identical at a glance. The first version of this reported release-train#67 bricked three minutes after a push — and it had every context a few minutes later.

So a head younger than 60 minutes is not judged, and an unreadable age counts as young. A false brick is what makes a report ignorable, which is precisely the failure mode of the always-red check one repo over.

It found a fourth cause on its first real run

The ticket lists three causes. There is a fourth, and it was live:

A conflicted PR. GitHub cannot compute a merge commit, so pull_request workflows never run at all and every required context stays absent.

release-train#67, 82 minutes after its last push: 0 workflow runs on its head sha, only a Bugbot verdict (which reviews the diff, not via a pull_request trigger), mergeStateStatus: DIRTY.

The remedy is a rebase, not a protection change — so the report labels that cause separately rather than sending someone to edit branch protection.

Fails closed throughout

An unreadable branch, head age, or PR list is reported as could not audit and exits 2. Reporting "0 bricked" while part of the fleet was never read is the exact shape this exists to remove.

exitmeaning
0every repo read, no PR missing a required context
1at least one PR is bricked
2could not evaluate — something was unreadable

Evidence

  • 10-case offline selftest (no network, no token), wired to a path-filtered selftest workflow like blocked-gate-selftest.yml. It asserts the three-way distinction, drafts, legacy context statuses, the conflict label, and every fail-closed path.
  • Full-fleet run over all 20 inventory repos: 94s, reporting exactly one finding — release-train#67, above. That matches the ticket's acceptance criterion (0 bricked as of 2026-08-11) plus the one new real case.
  • Cron every 4 hours plus workflow_dispatch.

🤖 Generated with Claude Code


Note

Medium Risk
New org-wide cron uses a privileged token to read protection and PRs across repos; logic is careful (grace window, fail-closed) but false positives/negatives would affect merge visibility fleet-wide.

Overview
Adds org-wide detection for PRs that look merge-ready but stay blocked because a required status context never appears on the PR—an invisible failure mode no in-PR check can catch.

New scripts/bricked-prs.py walks the repo inventory, resolves develop/staging/prod branches, loads required contexts via caller-drift.py's read_protection (not duplicated), and compares them to contexts present on open non-draft PRs. It reports required-but-absent after a 60-minute head grace window (oldest check-suite time, with safe fallbacks), skips drafts, treats PR list cap hits and unreadable data as could not audit (exit 2), and labels merge-conflicted PRs (DIRTY) separately from true never-reported checks. Human/--json output plus optional GitHub step summary table.

CI:.github/workflows/bricked-prs.yml runs the audit every 4 hours (and on workflow_dispatch) with PROJECTS_KANBAN_TOKEN. .github/workflows/bricked-prs-selftest.yml runs scripts/tests/bricked-prs-selftest.py offline (stubbed gh, no token) on path changes—mirroring the blocked-gate selftest pattern.

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

LukasWodkaand others added 30 commits August 1, 2026 16:30
promote: develop -> main (D30 self-signoff relaxation)
chore(promote): develop → main — fr-pass exit codes, fail-closed mutators, caller comment (backend#1412, #1413)
chore(promote): develop → main — wire the Done column, and give the closer lookup a token (backend#1411)
A required status check that never runs leaves its context at "Expected -
waiting for status to be reported". The PR is then approved, has nothing red to
point at, and is permanently unmergeable -- 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. Nothing inside a PR can detect that, so it needs
a watcher.
EMPIRICAL, NOT STATIC. Reasoning about "can this check ever report here" means
modelling path filters x matrix expansion x reusable inputs x their defaults x
`if:` expressions; run across 19 repos that produced three separate classes of
false positive before it produced anything true. Comparing a branch's required
contexts against the contexts actually PRESENT on the PR needs none of it:
required-but-absent is bricked, whatever the cause.
Required contexts come from caller-drift.py's `read_protection`, imported rather
than reimplemented -- it already unions classic protection with rulesets, and a
ruleset-only branch 404s on the classic API (backend#1276). A second copy is how
one of them silently stops reading half the picture.
THE DISTINCTION THAT MAKES IT READABLE, learned the hard way here: an absent
context and a not-yet-started one are identical at a glance. The first version
reported release-train#67 bricked three minutes after a push, and it had every
context minutes later. A head younger than 60 minutes is not judged, and an
unreadable age counts as young -- a false brick is what makes a report ignorable,
which is the failure mode of the always-red check one repo over.
FOUND A FOURTH CAUSE ON ITS FIRST REAL RUN, one the ticket does not list: a
CONFLICTED PR. GitHub cannot compute a merge commit, so pull_request workflows
never run and every required context stays absent. release-train#67, 82 minutes
after its last push, had 0 workflow runs on its head sha. The fix is a rebase,
not a protection change, so the report names that cause separately.
Fails closed throughout: an unreadable branch, age or PR list is reported as
"could not audit" and exits 2. Reporting "0 bricked" while part of the fleet was
never read is the shape this exists to remove.
Evidence: 10-case offline selftest passes; full-fleet run over all 20 inventory
repos takes 94s and reports exactly one finding (release-train#67, above).
Closestracebloc/backend#1721
@LukasWodkaLukasWodka self-assigned this Aug 13, 2026
Comment threadscripts/bricked-prs.py
Comment threadscripts/bricked-prs.py
@LukasWodka
LukasWodka changed the base branch from main to developAugust 13, 2026 09:02
Two findings, both the shape this watcher exists to report, in the watcher.
A CAPPED PR LIST READ AS CLEAN. `gh pr list` truncates at --limit and says
nothing about it, so a repo with more open PRs than the cap was audited
PARTIALLY and reported clean for the ones it never saw. The cap is now 200 --
high enough that no repo here approaches it, the busiest having ~10 open PRs
against one base -- and reaching it raises "could not audit" instead of
returning a partial view.
THE GRACE WINDOW USED THE WRONG CLOCK. A commit's committer date is when it was
WRITTEN. A force-push can put a long-dated commit on a branch a second ago,
which would then be judged immediately -- producing exactly the false "bricked"
the window exists to prevent.
The honest clock is when CI first saw the head: the OLDEST check suite on the
sha, since GitHub creates a suite per app as soon as it has work for that head.
The commit date remains as a fallback for a head with no suite at all, which is
the conflicted case -- there, no suite will ever exist and the commit date is
the only clock there is.
Also drops an unused `timedelta` import that failed `quality / ruff`.
13 selftest cases, up from 10. The two new ones needed the real functions put
back first: earlier cases swap `open_prs` and `head_age_minutes` out wholesale,
so a later case that forgets silently tests the previous case's stub -- this
file's own subject matter, and it caught me on the first run.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Both findings fixed in 12dea48 — and both were the shape this watcher exists to report, inside the watcher.

Capped PR list read as clean.gh pr list truncates at --limit and says nothing about it, so a repo with more open PRs than the cap was audited partially and reported clean for the ones it never saw. The cap is now 200 — high enough that nothing here approaches it, the busiest base having ~10 open PRs — and reaching it raises could not audit rather than returning a partial view.

The grace window used the wrong clock. A commit's committer date is when it was written; a force-push can put a long-dated commit on a branch a second ago, which would be judged immediately — the exact false "bricked" the window exists to prevent.

The honest clock is when CI first saw the head: the oldest check suite on the sha, since GitHub creates a suite per app as soon as it has work for that head. Commit date stays as the fallback for a head with no suite at all — the conflicted case, where no suite will ever exist and it is the only clock there is.

Also dropped an unused timedelta import that failed quality / ruff.

13 selftest cases, up from 10. Worth noting how the two new ones failed first: earlier cases swap open_prs and head_age_minutes out wholesale, so a case that forgets to restore them silently tests the previous case's stub. That is this file's own subject matter, and it caught me on the first run.


Separately: this PR and #237 were targeting main, and that was my error..github is a train repo with a develop branch, so promotions into main go through the release train — fr-gate refused both, correctly. Retargeted to develop.

Comment threadscripts/bricked-prs.py
saadqbal
saadqbal previously approved these changes Aug 13, 2026

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

Really careful PR 👍 The empirical required-vs-present comparison sidesteps the whole path-filter × matrix × reusable-input modelling trap, the 60-min grace + oldest-check-suite clock kill the obvious false positives (a running check is present in the rollup by name, so it's never flagged), and it's strictly read-only — reusing read_protection rather than forking it. Ran the selftest offline: 13/13 green, and non-vacuous.

Two non-blocking nits:

  • head_age_minutes returning None drops the candidate silently (treated as young) with no could-not-audit signal — that one path is fail-open, unlike the rest of the script. It's a deliberate, documented tradeoff and the next run catches it, so fine to keep; just flagging it's the single spot the "fails closed throughout" framing doesn't literally hold, since it needs both the check-suites and commit endpoints to fail for one sha.

  • mergeStateStatus can come back UNKNOWN before GitHub finishes computing mergeability, so a freshly-conflicted PR would be labelled never-reported rather than conflicted (only == "DIRTY" is checked). Cosmetic — it's still flagged as bricked, just with the wrong remediation hint, and it self-corrects well inside the 60-min window.

A failed check-suites API read (502/403/rate-limit) fell through to the
commit committer date, which a force-push can set to any past instant --
so an unreadable CI clock plus an old commit still reported BRICKED, the
exact false-brick the grace window exists to prevent. Distinguish a failed
read (return None -> caller treats the head as young and skips) from a
genuinely empty one (conflicted PR, no suite will ever exist -> commit date
is the only clock). Adds a selftest case for the failed-read path.
Bugbot, .github#239.
@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 1e25d2d. Configure here.

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

Selftest runs 14/14 locally.

The empirical-over-static choice is the right one, and the PR is honest about how it was learned. Modelling "can this check ever report here" means path filters × matrix expansion × reusable-workflow inputs × their defaults × if: expressions, across 19 repos — and reportedly produced three classes of false positive before one true finding. Comparing required contexts against contexts actually present needs none of that: required-but-absent is bricked whatever the cause. That's a genuinely better problem decomposition, not just a simpler one.

Importing caller_drift.read_protection rather than reimplementing it is the right instinct, and I confirmed it's a real import (via importlib.util.spec_from_file_location, since the hyphenated filename isn't importable by name) rather than a copied function. It already unions classic protection with rulesets, and a ruleset-only branch 404s on the classic API — a second copy is exactly how one of them silently stops reading half the picture.

The selftest cases are where this earns trust. Three in particular are the difference between a useful watcher and an alarm nobody believes:

  • a force-pushed old commit is dated by CI's clock, not the commit's — dating by commit timestamp would let a force-push of an old commit read as instantly stale.
  • an unreadable check-suites read is undateable (None), not the commit date — a 502 reads as young, so a transient API error can never brick a 7-day-old commit. That's failing in the safe direction, and it's the direction that matters for a tool whose whole value is being believed.
  • a PR list that hits the cap raises rather than returning a partial view — a partial list would silently under-report, which for a watcher is indistinguishable from "all clear".

Also checked the blast radius, since a cron with an org-scoped token is worth looking at: permissions: contents: read, and the job only reports — it files no issues, posts no comments, and mutates nothing. cancel-in-progress: false is correct for an audit you don't want half-finished.

Four-hourly is well reasoned in the comment — not urgent, but invisible, so the cost is human hours spent not realising.

LGTM.

@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-approve at 1e25d2d. The check-suites fix is right: a failed read now returns None → treated as young → skipped, so a force-pushed old commit can't fall through to its committer date and brick falsely; the genuinely-empty (conflicted, no suite) case still uses the commit date. Verified the distinction with the selftest (14/14 pass, incl. the new 502-on-suites case). Fleet-safe/read-only, still fail-closed to exit 2 on unreadable branch/PR-list/cap. Nice.

@LukasWodka
LukasWodka merged commit e1bce83 into developAug 13, 2026
10 checks passed
@LukasWodka
LukasWodka deleted the feat/1721-bricked-pr-watcher branch August 13, 2026 14:21
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@shujaatTracebloc