Skip to content

Stop a stale branch from skipping the CI gates it was approved under - #95

Open
vahid-ahmadi wants to merge 1 commit into
mainfrom
ci/gate-freshness
Open

Stop a stale branch from skipping the CI gates it was approved under#95
vahid-ahmadi wants to merge 1 commit into
mainfrom
ci/gate-freshness

Conversation

@vahid-ahmadi

Copy link
Copy Markdown
Contributor

The gap

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 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:

  • a determinism check — two builds must agree on the content hash
  • a no-drift gategit diff --exit-code after the build and after the suite

Branches 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:

PR branch author
#49 uk/ukmod-cases-schema vahid-ahmadi fixed — main merged, now builds + passes under the gates
#64 uk/calculator-oracles vahid-ahmadi fixed — same
#56 obr-costings-mode2 @MaxGhenis still open
#69 us/kff-medicaid @MaxGhenis still open

Max — #56 and #69 still track data/scorecard.db and their ci.yml has neither gate. The fix is a git 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 its COMPARISON.csv.

The fix

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, not hardcoded. It reads main's ci.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 tracks data/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_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. Tests assert both, and assert it cannot quietly be downgraded to a pull_request trigger — 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_target is 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.

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
@vahid-ahmadi

Copy link
Copy Markdown
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.

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

Verdict: approve, with one caveat to confirm. The pull_request_target mechanism genuinely closes the gap described — unlike a pull_request workflow it runs from the base branch, so main defines the gate set for every open PR regardless of what the head's .github contains. The gate list is derived by grepping base ci.yml (the nondeterministic assert + git diff --exit-code), so new gates auto-enforce. Security is sound: the job checks out only base.sha, fetches the head SHA as data, and reads it via git show / git ls-tree — it never checks out or executes PR code (permissions: contents: read).

The one thing to verify before treating the gap as closed

  • A pull_request_target check only turns red/green — it does not block a merge unless branch protection lists this check as a required status check. That setting isn't in the diff (the branch-protection API returned 404), so the guard logically relies on a repo setting outside this PR. If that required-check isn't configured, a stale PR can still be merged past an advisory red. Please confirm it's set (and ideally note it in the PR).

Minor (non-blocking)

  • The determinism gate is fingerprinted by the assert ... nondeterministic ... line inside ci.yml's heredoc — rewording that assert on main (without weakening it) flips every open PR to STALE. Brittle but fail-safe (errs toward blocking), so acceptable.
  • test_ci_gates.py splits on the literal "Fetch the head commit as data" step name to isolate executable YAML from heredoc help text — renaming that step silently breaks the danger-scan.
  • Consider having gate-freshness run tests/test_ci_gates.py from base too, so the guard's own invariants don't ride the very ci.yml a stale branch may lack.

🤖 review via Claude Code

vahid-ahmadi pushed a commit that referenced this pull request Aug 26, 2026
The probe workflow runs both registries, but pipeline/validate_budget_2026_registry.py
arrives with #100 — so this branch failed on a missing file rather than
on anything it owns.

Guarded on existence, and the skip is ANNOUNCED via ::notice:: rather
than silent. A gate that quietly does nothing reads as a gate that
passed, which is exactly what #74/#95 were about.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016HuXJFVme8HRbnke2Ey3Me
vahid-ahmadi pushed a commit that referenced this pull request Aug 26, 2026
The previous commit guarded pipeline/validate_budget_2026_registry.py
and left validate_cgt_reform_spec.py (#102) failing the same way — a
validator that lands on its own branch, absent on this ref.

Folded both into one loop that skips a missing validator, announces
every skip via ::notice::, and reports how many actually ran. Each
starts enforcing the moment its PR merges, with no edit here.

Announcing the skip is the point. A gate that quietly validates
nothing reads as a gate that passed — the #74/#95 failure mode.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016HuXJFVme8HRbnke2Ey3Me
Sign up for free to 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