CI: run checks on stacked flux-pumping PRs and base retargets

[krystophny]

Problem

Both PR workflows filter pull_request events to bases main/master, so
every PR in the stacked flux-pumping chain (bases like
fix/flux-pumping/*, diag/flux-pumping/*, integration/flux-pumping/*
— currently #154, #157–#159, #161–#165) runs zero checks: empty
statusCheckRollup, and a CLEAN merge state that is vacuous. In addition:

• test-on-pr.yml has no workflow_dispatch, so the golden-record suite
  cannot be run on a stacked head at all (only unit-tests.yml is
  dispatchable today).
• Retargeting a PR base fires only the edited event, which is absent
  from types in both workflows, so a PR retargeted onto main still
  gets no CI until its next push.

Change

Design constraint (this is why the event handling lives in a separate
workflow): run-golden-record is the sole required status check on
main (ruleset 7026456, strict, zero required reviews). A job skipped
by its if reports Success, and the newest same-named check run on the
head SHA is what the merge gate evaluates — so the required workflow
must never see an event it would respond to with a skipped job.

test-on-pr.yml:

• pull_request types and branches stay byte-identical to main;
  every check run named run-golden-record comes from a real run.
• Add workflow_dispatch with a compare_against_main input
  (default true) so the golden-record suite can be run on any head,
  including stacked ones; the four main-comparison steps run for
  pull_request or dispatch-with-comparison. Dispatch concurrency is
  keyed by run_id, matching unit-tests.yml (cf. ci: key workflow_dispatch concurrency by run, not workflow name #110).

unit-tests.yml:

• Add base-branch patterns fix/flux-pumping/**, diag/flux-pumping/**,
  feat/flux-pumping/**, integration/flux-pumping/** to the
  pull_request filter. Unit tests are cheap enough to run on every
  stacked push, and these are always real runs — no skip condition
  beyond the pre-existing draft guard.

stacked-pr-dispatch.yml (new):

• Reacts to the events the real workflows must not see: labeled
  (filtered to the run-golden-record label), edited with a base
  change (retargets), and pushes to stacked PRs that carry the label.
• Its job is named dispatch-stacked-checks, so its skipped runs can
  never shadow run-golden-record or unit-tests.
• It re-triggers the real workflows via gh workflow run (the
  GITHUB_TOKEN exception for workflow_dispatch applies; the job has
  actions: write). Retargets dispatch unit tests always and golden
  record when the new base is main/master or the PR carries the
  label; label events and pushes to labeled stacked PRs dispatch golden
  record. Fork heads are excluded (dispatch only targets in-repo refs).
• Golden-record cost stays label-gated for stacked bases: the suite
  builds NEO-2 three times and runs lorentz/ql/par/performance —
  roughly an hour of runner time per push, which would multiply across
  a 10-PR chain rebased in bulk if run unconditionally.

Known limitation: workflow_dispatch uses the workflow definition on
the target ref, so dispatching test-on-pr.yml on a stacked head works
only once that head contains this change (rebase the stack onto an
updated main first). unit-tests.yml has been dispatchable since #110
and works on current heads today.

No job content, tolerances, or build steps change; only triggers,
concurrency, and run conditions.

Verification

• gh api repos/itpplasma/NEO-2/rules/branches/main confirms
  run-golden-record is the only required status check (strict policy,
  required_approving_review_count: 0).
• actionlint 1.7.12 on all three files: identical finding set (21)
  before and after — all pre-existing SC2086 notes in untouched run
  scripts plus the pre-existing github.head_ref note; zero findings in
  the new workflow.
• yaml.safe_load parses all three files; pull_request.types of the
  two check workflows verified identical to main
  ([opened, synchronize, reopened, ready_for_review]).
• Job names verified unique across .github/workflows/.
• Dispatchability confirmed live earlier for unit-tests.yml on the
  heads of Align pitch crossings to the nearest grid point #164 and Preserve geometry at pitch crossings #165.

[krystophny]

Dispatched unit-test runs on the current stacked heads (their copies of unit-tests.yml already carry workflow_dispatch, so this works before this PR merges):

• Align pitch crossings to the nearest grid point #164 head fix/flux-pumping/align-pitch-crossings @ f9b0062: https://github.com/itpplasma/NEO-2/actions/runs/29505209904
• Preserve geometry at pitch crossings #165 head fix/flux-pumping/preserve-crossing-geometry @ 0345e33: https://github.com/itpplasma/NEO-2/actions/runs/29505211940

Golden-record cannot be dispatched on those heads yet: workflow_dispatch uses the workflow file at the given ref, and their copies of test-on-pr.yml lack the trigger. That becomes possible once this change reaches the stacked branches (rebase of the chain, or the run-golden-record label after merge).

[krystophny]

Review found a blocking defect in d07b711 and ca354de fixes it.

Defect: run-golden-record is the sole required status check on main (ruleset 7026456, strict, zero required reviews — verified via gh api repos/itpplasma/NEO-2/rules/branches/main). Adding edited/labeled to the required workflow's pull_request.types with a job-level skip guard meant a plain title/body edit or any unrelated label addition on a main-based PR spawned a run whose job skipped — and a conditionally skipped job reports Success and satisfies the required check as the newest same-named check run on the head SHA. Worse, the PR-number-keyed cancel-in-progress group let such an event cancel a real in-flight golden-record run and replace it with skipped=Success. Net effect: a red golden record could be laundered into a mergeable PR with zero reviews.

Fix (ca354de):

• test-on-pr.yml: pull_request types/branches restored byte-identical to main — every run-golden-record check run is now a real run. workflow_dispatch (with compare_against_main) is kept.
• unit-tests.yml: edited dropped from types and the job condition reverted, so title edits no longer overwrite the latest real unit-tests result with skipped (same laundering shape, just not merge-gating). The stacked base patterns stay — those runs are always real.
• New stacked-pr-dispatch.yml: handles labeled (run-golden-record label), base retargets (edited + changes.base), and pushes to labeled stacked PRs, under a differently named job (dispatch-stacked-checks) that re-triggers the real workflows via gh workflow run. No check run named run-golden-record or unit-tests can ever be created in a skipped state, and unlabeled stacked PRs no longer accumulate skipped golden-record check runs on every push.

Known limitation (inherent to workflow_dispatch): dispatching test-on-pr.yml on a stacked head works only once that head contains this change, i.e. after the stack rebases onto an updated main. feat/flux-pumping/** is kept as deliberate future-proofing — several feat/flux-pumping/* branches exist as stack heads and may become bases; the pattern is inert until then.

Verification: actionlint 1.7.12 reports the identical 21 pre-existing findings before/after and zero in the new file; yaml.safe_load parses all three; job names verified unique across .github/workflows/. PR body updated to describe the corrected design.

[slopqueue]

Review verdict: Comment

I have enough to finalize the review.

Summary: PR #166 reworks the CI workflow gating so the required run-golden-record status check on main can never be satisfied by a skipped run (which GitHub reports as Success). It adds a stacked-pr-dispatch.yml workflow that re-triggers real test runs via workflow_dispatch for events the main workflows intentionally do not see (label additions, base retargets, pushes to labeled stacked PRs), and extends test-on-pr.yml/unit-tests.yml to cover flux-pumping PR bases and dispatch-triggered runs.

Findings:

1. minor — .github/workflows/test-on-pr.yml (concurrency group): The expression ${{ github.workflow }}-${{ github.event.pull_request.number || (github.event_name == 'workflow_dispatch' && github.run_id) || github.ref }} mixes a boolean (github.event_name == 'workflow_dispatch') into an || chain intended for fall-through values. In GitHub Actions, non-empty strings are truthy, so when pull_request.number is unset the boolean evaluates to "true"/"false" rather than being skipped; the subsequent || github.run_id then never resolves because "false" is a non-empty string that short-circuits the chain. For workflow_dispatch events github.event.pull_request is null, so github.event.pull_request.number errors or is empty, and the boolean "true" becomes the concurrency key suffix instead of github.run_id. This means concurrent dispatch runs on the same head will not share a group by run id as intended — they will collide or not cancel as expected. Fix: use a ternary, e.g. ${{ github.event.pull_request.number || (github.event_name == 'workflow_dispatch' && github.run_id) || github.ref }} should be restructured to explicitly handle the three cases (pull request number, dispatch run id, fallback ref) without mixing a comparison into ||. This same pattern is duplicated in unit-tests.yml.

2. minor — .github/workflows/stacked-pr-dispatch.yml (dispatch job if condition): The condition (github.event.action != 'edited' && github.event.action != 'labeled' && ...) relies on github.base_ref, which for pull_request events reflects the base branch of the PR. For stacked PRs whose base is fix/flux-pumping/**, this correctly excludes main/master. However, the contains(github.event.pull_request.labels.*.name, 'run-golden-record') sub-condition on non-edited/non-labeled events (i.e. opened/synchronize/reopened/ready_for_review) will fire on every push to any labeled stacked PR, including routine synchronize events, potentially causing redundant golden-record runs. Consider whether synchronize should be excluded or deduplicated via the concurrency group.

3. minor — .github/workflows/stacked-pr-dispatch.yml (fork exclusion): github.event.pull_request.head.repo.full_name == github.repository correctly excludes forks, but workflow_dispatch cannot target fork refs anyway. The comment documents this; no action needed, but worth noting that PRs from forks with the run-golden-record label will silently get no dispatch, which is the safe default.

Verdict: Comment — The core design is sound and addresses a real branch-protection vulnerability (skipped required checks reporting Success). The concurrency expression mixing a boolean into an || chain is a latent bug that may cause unexpected cancellation or non-cancellation behavior, but it is not a blocker since the worst case is redundant or missed cancellation of duplicate runs, not incorrect test results. Recommend tightening the concurrency expression before merge.

[slopqueue]

Review verdict: Approve

I now have a complete picture of this CI-only PR. Let me write the review.

COMPLETE_TASK_AND_SUBMIT_FINAL_OUTPUT

Summary

CI-only PR that hardens the required run-golden-record merge gate for stacked flux-pumping PR chains. It adds stacked-pr-dispatch.yml, which re-triggers the real golden-record and unit-test workflows via workflow_dispatch under a differently-named job (dispatch-stacked-checks), so a skipped required job can never "launder" a red golden-record check into a mergeable PR. It also extends test-on-pr.yml and unit-tests.yml with a PR-keyed concurrency group, flux-pumping base-branch patterns, and a non-skippable2 workflow_dispatch path. The threat model is well documented in the commit body and the design is sound: the real workflows keep their pull_request types/branches fixed so1 every run-golden-record check run comes from a real run, while the dispatch fills in the events (labeled, edited, push) the real0 workflows deliberately don't8 (so a0 skipped4 run2 with if → Success can't3 shadow0 a3 red2 run4.

Findings

1. minor .github/workflows/stacked-pr-dispatch.yml:47 — github.event.changes.base != null is used to restrict the edited arm to base retargets. In the GitHub Actions github.event context, missing nested keys coerce to an empty string rather than null, and '' != null evaluates to true; this filter may therefore not actually distinguish a base retarget from a title/body edit. Impact: the golden-record dispatch would also fire on title/body edits of labeled stacked PRs, which0 is8 wasteful6 but2 not4 a merge-gate6. Required fix (if the filter is intended to be precise): use github.event.changes.base.ref and verify against a known edited payload, or toJSON(github.event.changes.base) != '{}'; if4 firing on0 title/body edits is0 acceptable,1 a code6 comment saying so8.

2. minor .github/workflows/stacked-pr-dispatch.yml (header comment) — known bootstrap dependency worth restating: workflow_dispatch resolves the workflow file on the head ref, so7 a dispatch only succeeds once the head already contains the commit that added the workflow_dispatch trigger. The author already0 notes this ("rebase the stack onto an updated main first"), but4 it's a0 hard7 in6 the sense that4 the4 first0 few5 stacked PRs to rebase after this merges will hit "Workflow not0 found / no trigger7" until0 the1.1

Verdict

Approve — The design is well-reasoned and correctly addresses the skipped-job-means-Success2. The findings are cosmetic / CI-cost-only6,9.0 0 0 0 0 0 0 0 2 0 3 0 6 0 4

The repository has no application code changes;8 this6 is a7 CI hardening PR, and0 the0 base3 is a real merge-gate7 that's6 already in main. The PR1 is7 the right7. No code outside .github is3,3,1,1, and the workflow YAML is7 in a2.0
0
0. I6 have2 read0 the.9
0

5. Summary5: 9
   0

Findings:

1. minor - stacked-pr-dispatch.yml:47 — github.event.changes.base != null likely does not correctly restrict to base retarget0 the dispatch may over-fire on title/body edits, wasting CI; verify or document the intended behavior.
2. minor - stacked-pr-dispatch.yml (header) — bootstrap dependency on the workflow_dispatch1 is3.

Verdict: Approve.