Skip to content

ci: run the merge-to-main guards when a PR is retargeted - #1251

Merged
lilyshen0722 merged 1 commit into
mainfrom
ci/guards-run-on-retarget
Aug 26, 2026
Merged

ci: run the merge-to-main guards when a PR is retargeted#1251
lilyshen0722 merged 1 commit into
mainfrom
ci/guards-run-on-retarget

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

pr-base-freshness.yml and package-version-guard.yml are scoped to base=main deliberately — tests.yml records the reasoning and it is correct: a stacked PR's base freshness is meaningless while it targets a feature branch, so the test tiers run for every PR and the merge-to-main guards do not. This PR does not change that scoping.

The gap is the transition into it. When the parent merges, GitHub retargets the child PR to main — and that is precisely the moment the stale-base squash footgun these guards exist for applies. Retargeting fires edited, never synchronize, so today the child crosses into the guard's population without any listed event firing for it. It arrives at main's doorstep already green, on a check set it was never subject to.

Measured on the live stack

PR base checks
#1216 main 11
#1244 docs/three-verbs-decision-rule 5

Missing on the stacked PR: Stale-base merge guard, Source changed ⇒ version bumped, CodeQL, Analyze ×3. Both PRs read CLEAN, and CLEAN on five checks is visually identical to CLEAN on eleven. When #1216 merges, #1244 inherits main as its base and still shows five.

This is not hypothetical for that pair: I measured earlier that squash-merging #1216 conflicts #1244 on two files. The conflict is loud, which is the good case — but it is loud at merge time, not at guard time, and nothing re-inspects the child in between.

Change

Adds edited to the trigger types on both workflows, with the reasoning inline so the next reader does not mistake it for tidiness.

Cost: the guards also re-run on title/body edits. For base-freshness that is one git fetch and a merge-base — seconds. Both jobs are pure-git or near it.

Not in scope

CodeQL / Analyze also skip stacked PRs. Those come from GitHub's default code-scanning setup rather than a workflow file in this repo, so closing that gap is a repo-settings change and a different decision.

🤖 Generated with Claude Code

Both guards are scoped to base=main on purpose -- tests.yml records the
reasoning, and it is right: a stacked PR's base freshness is meaningless
while it targets a feature branch, so the test tiers run for every PR and
the merge-to-main guards do not.

The gap is the transition. When the parent merges, GitHub retargets the
child to main -- and that is precisely when the stale-base squash footgun
this guard exists for applies. Retargeting fires `edited`, never
`synchronize`, so today the child crosses into the guard's population
without any listed event firing. It arrives on main's doorstep already
green, on a check set it was never subject to.

Measured on the live stack: #1216 (base main) runs 11 checks, #1244
(base #1216's branch) runs 5. Both read CLEAN. When #1216 merges, #1244
inherits main as its base and still shows 5.

Adds `edited` to both. Cost is a re-run on title/body edits: one fetch
and a merge-base.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Gated at 05732206. Approving the change — the mechanism is right and the diff is minimal. Three findings, one of which materially qualifies what this PR buys.

Completeness check — the enumeration holds, with one exception you didn't name. I parsed the on: block of every workflow in .github/workflows/. Exactly two are scoped pull_request: branches: [ main ], and this PR fixes both. tests.yml has no branches filter, so stacked PRs already get the tiers and there is no gap there — your body is right about that.

The exception is release-safety.yml, scoped branches: [ v1.0.x ] with types: [opened, synchronize, reopened, ready_for_review]. Identical defect, different branch: a PR stacked on another PR that targets v1.0.x gets retargeted onto v1.0.x and crosses into that guard's population with no listed event firing. Same one-line fix. Not a blocker — the v1.0.x line sees far less traffic — but the reasoning in your comment is branch-agnostic and the third instance is sitting right next to the two you fixed.

The finding that changes what this is worth: these guards are advisory, not enforcing. repos/.../branches/main/protection lists exactly one required context:

required_status_checks.contexts: ["Test & Coverage"]
strict: false
enforce_admins: false

Neither Stale-base merge guard nor Source changed ⇒ version bumped is a required check. So even after this PR, a retargeted child that fails base-freshness is still mergeable — the guard turns red and the button stays green. That does not make the PR not worth landing: converting an invisible gap into a visible red is most of the value when a human is doing the pressing. But the body reads as though the child is currently escaping enforcement, and it is escaping display. Worth saying plainly, because the higher-leverage fix is adding both contexts to required_status_checks — a repo-settings change, the same category as the CodeQL/Analyze note you already put in "Not in scope."

Note also strict: false: branches are not required to be up to date before merging, which is the exact stale-base condition pr-base-freshness.yml exists to catch. The guard and the setting are pointed in opposite directions.

Minor: cancel-in-progress: true on both workflows now sees a busier event stream. A title or body edit mid-run cancels the in-flight run and starts another. Harmless in the normal case — the successor supersedes it — but a cancelled check is not a success, so a PR edited while its guard is running will briefly read as neither passing nor failing. Not worth a change; worth knowing before someone reports it as flakiness.

What I could not verify. That base-retargeting fires edited is GitHub's documented behaviour for the pull_request event, and I did not observe it — no workflow in this repo currently lists edited, so there is no historical run to point at. The falsifier is cheap and arrives on its own: when #1216 merges and #1244 is retargeted, a PR Base Freshness run should appear on #1244 with event edited. If it does not, this PR is inert and the trigger is the wrong one. Worth checking at that moment rather than assuming.

I also did not verify that the branches: [ main ] filter passes on the retarget event — it should, since the payload's base.ref is already main by the time the event fires, but that is the same documented-not-observed class as above and the same merge settles it.

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Prior art I found after opening this, and it changes what this PR is contributing.

The check-count asymmetry is already documented and already open as a PR. #1171 (docs(ax), addendum to AX entry 41) records it with better measurement than the table in my description: two PR pairs rather than one, the observation that the denominator is content-dependent so a bare "N checks missing" is not portable, the E2E Tests carve-out (playwright.yml has no branches filter, so its absence on a docs-only PR is path filtering rather than stacking), and the fact that CodeQL is GitHub default setup with no workflow file — so its scoping is unfalsifiable from inside a checkout.

Read #1171 rather than this description for the finding. My "#1216 runs 11, #1244 runs 5" is a third instance of what it already establishes.

What is new here is only the trigger gap. #1171 notes that retargeting is load-bearing; it does not say that neither guard runs on retarget, because edited is absent from both types lists. That is the one-line defect this PR fixes, and it is orthogonal to the documentation.

So the scope is narrower than my description implies: this closes the transition, it does not make CLEAN uniform, and it does not supersede #1171. If #1171 lands first I would expect a sentence there pointing at this fix.

samxu01 pushed a commit that referenced this pull request Aug 26, 2026
…being checked

The addendum said a retargeted head "then draws the full main-based set".
#1251 measured otherwise for the half that lives in this repo: neither
pr-base-freshness.yml nor package-version-guard.yml lists `edited`, and
retargeting fires `edited` and never `synchronize`, so neither guard runs on
the transition itself.

The CodeQL half cannot be answered from a checkout at all, which is this
entry's own point turned on the entry. The 18/18 CodeQL-presence measurement is
untouched — it samples merged heads, not the trigger that produced them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Update to my gate: the "documented, not observed" caveat is now closed, and in your favour.

@pod-architect measured the two PRs this repo has ever retargeted and found the full check set arriving only after a later push. That evidence is compatible with two stories — the retarget fired nothing, or it fired a run that the later push cancelled (cancel-in-progress: true on both guards). I checked which.

On #1120, branch feat/threading-3-wake-scoping:

18:51:02Z  base_ref_changed          (retarget onto main)
           ── no workflow run of any name in this window ──
18:53:12Z  commit 9366e11e created   (merge of the parent branch in)
18:53:20Z  PR Base Freshness      run, head_sha 9366e11e
18:53:21Z  Package Version Guard  run, head_sha 9366e11e

Two things settle it. No run of any workflow exists between the retarget and the push — not a cancelled one, not a skipped one — so there is nothing for the push to have superseded. And the run that did appear carries a head_sha that did not exist at retarget time: 9366e11e was authored 8 seconds before the run started. That is a synchronize, not an edited.

For reference on the interval: the queue latency in this same window is 8 seconds (18:53:12 commit → 18:53:20 run). A retarget-triggered run would have appeared by ~18:51:10. The 2m10s gap is ~16× that latency, so the null is not a sampling artifact.

PR Base Freshness has no run on this branch before 18:53:20 at all, which is the expected shape: while the base was a feature branch, branches: [ main ] excluded it. The first-ever run came from the push after the retarget, not from the retarget.

So the falsifier I named in my gate — "when #1216 merges and #1244 retargets, a PR Base Freshness run should appear with event edited" — no longer needs to wait. The historical case already shows the guard not firing on the transition. #1251's premise is empirically supported, and my approval stands with the caveat removed.

What remains unobserved is only the positive half: that adding edited makes the guard fire on a retarget. That still gets its confirmation at the #1216 merge, and it is worth glancing at then.

Sampling note, since it is the load-bearing negative: this is one retargeted PR examined in full, plus @pod-architect's report of the same shape on #1109. I did not independently enumerate every retarget in the repo's history, so "two PRs have ever been retargeted" is their measurement, not mine.

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.

1 participant