Stop a stale branch from skipping the CI gates it was approved under - #95
Open
vahid-ahmadi wants to merge 1 commit into
Open
Stop a stale branch from skipping the CI gates it was approved under#95vahid-ahmadi wants to merge 1 commit into
vahid-ahmadi wants to merge 1 commit into
Conversation
GitHub runs a pull_request workflow from the PULL REQUEST'S OWN branch, not from the base. So a branch that forked before a gate was added keeps running the workflow WITHOUT it — and its green tick looks identical to a full one while meaning strictly less. That is not hypothetical. #74 made the database a derived artifact and added a determinism check and a no-drift gate. Branches forked before it kept the pre-#74 workflow, so neither gate had ever run against them, and they were reviewed and approved on the understanding that both had. An audit of every open PR found four in that state; two were mine (#49, #64, since fixed) and two are still open. gate-freshness.yml runs from the BASE via pull_request_target, so a stale head cannot skip it: the check is defined by main and applies to every PR regardless of what its own .github looks like. The gate set is DERIVED FROM THE BASE rather than hardcoded — it reads main's ci.yml and requires every determinism/no-drift line it finds to be present in the head's — so a gate added later is enforced on every open PR without anyone remembering to update the guard. It also refuses a branch that still tracks data/scorecard.db, whose build cannot have been from-scratch. Security: pull_request_target runs in the base repo's context, so this job NEVER checks out or executes pull-request code. It reads git metadata only and holds contents:read. A test asserts that, and asserts the guard cannot quietly become a pull_request trigger. Verified by running the exact logic against all 16 open PRs: 14 pass and exactly the two known-stale branches fail, with the reasons named. Suite 270 passed, ruff format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016HuXJFVme8HRbnke2Ey3Me
This was referenced Aug 25, 2026
Contributor
Author
|
Review request — @MaxGhenis @DTrim99. This one is part of a batch; the whole queue, with a suggested merge order and what is blocked on whom, is in #104 so you can triage in one place rather than PR by PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gap
GitHub runs a
pull_requestworkflow from the pull request's own branch, not from the base. So a branch that forked before a gate was added keeps running the workflow without that gate — and its green tick looks identical to a full one while meaning strictly less.This is not hypothetical. #74 made the database a derived artifact and added two gates to
ci.yml:git diff --exit-codeafter the build and after the suiteBranches that forked before it kept the pre-#74 workflow, so neither gate had ever run against them — and they were reviewed and approved on the understanding that both had. They also still track the committed database #74 removed.
The audit
I ran the check against all 16 open PRs. Four were in that state:
uk/ukmod-cases-schemauk/calculator-oraclesobr-costings-mode2us/kff-medicaidMax — #56 and #69 still track
data/scorecard.dband theirci.ymlhas neither gate. The fix is agit merge origin/main, then rebuild and confirm a clean tree; nothing about the branches' own changes needs to move. I have deliberately not pushed to your branches. Note #56 matters beyond itself — #67 consumes itsCOMPARISON.csv.The fix
gate-freshness.ymlruns from the base viapull_request_target, so a stale head cannot skip it: the check is defined bymainand applies to every PR regardless of what its own.githublooks like.The gate set is derived from the base, not hardcoded. It reads
main'sci.yml, extracts every determinism/no-drift line, and requires each to be present in the head's — so a gate added later is enforced on every open PR without anyone remembering to update this guard. It also refuses a branch that still tracksdata/scorecard.db, whose build cannot have been from-scratch.The failure message says exactly what to do, because a gate that fails without a fix is one people learn to ignore.
Security
pull_request_targetruns in the base repo's context, so this job never checks out or executes pull-request code. It reads git metadata only and holdscontents: read. Tests assert both, and assert it cannot quietly be downgraded to apull_requesttrigger — which would reintroduce the exact hole it closes.Verification
Ran the exact logic locally against all 16 open PRs: 14 pass, and exactly the two known-stale branches fail, with their reasons named. Suite 270 passed, ruff format clean.
Reviewers
@MaxGhenis @DTrim99 — one thing worth a second opinion:
pull_request_targetis the right trigger for this but it is the sharp one, so the "never touch PR code" property is the thing to check hardest. The alternative is branch protection's Require branches to be up to date before merging, which is a repo setting rather than a reviewable file and forces a merge on every PR whenever main moves at all; I preferred a check that fires only when the tick genuinely means less.