Skip to content

ci: admit the routine marker's push parent provisionally; bind the exact head at merge (Refs #5189) - #5191

Merged
EHotwagner merged 2 commits into
mainfrom
routine/routine-marker-push-race
Nov 1, 2026
Merged

EHotwagner merged 2 commits into
mainfrom
routine/routine-marker-push-race

Conversation

@EHotwagner

@EHotwagner EHotwagner commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Refs #5189

Problem

The routine-development marker's head= field must name the PR head. A push cannot carry a marker for a head that does not exist yet. The synchronize run of routine-eligibility therefore always reads the old marker and fails. The owner then edits the body, and the edit starts its own set of runs.

Measured on #5188, head 1eb3ae15, with the GitHub runs API:

  • the push started 32 runs, and routine-eligibility failed (run 41297777859);
  • the rebind edit, 5 s later, started 7 more: architecture-map, claim-generation, closing-keywords, landed-revert-guard, fsgg-claim-fence, board-linkage and routine-eligibility.

no-silent-revert does not read the routine marker. On #5188 its only non-success was a cancelled run on the superseded head. Its extra cost is the edit-triggered run counted above.

Decision: (a) push-parent allowance, with the exact binding enforced at merge

On synchronize only, the workflow passes GitHub's own github.event.before (the PR's previous head) to the validator as a provisional binding. The author cannot set this value. It is passed only when the push merely added commits on top of it: bounded deepening (0/8/64/256) has to prove before is an ancestor of the new head.

The validator then admits a marker that names exactly that parent. It prints a ::warning:: [routine-rebind-owed] and the verdict routine delivery provisionally admitted ... rebind the marker to head=<new> before merge. Every other routine check still runs against the new head: one marker, no duplicate fields, an allowed and unprotected operation, the live-base policy, and a non-empty change measured from the merge base.

The exact binding stays where the merge happens. tools/routine-delivery.py routine_contract reads the live PR body and calls validate_routine_body(body, branch, expected_head). That call refuses any head= other than the merged head before the eligibility check is even consulted. This already happens on main; this PR adds a causal control that pins it.

So an owner pushes repairs during review without editing the body, and rebinds once before merge. The rebind is a body edit (7 runs on .github), so the saving is one body edit per intermediate push, plus the red routine-eligibility run each push used to produce.

Option Safety CI cost Why not chosen
(a) push parent, rebind before merge Exact binding unchanged at the merge route. A provisional pass needs the GitHub-authored before and proven fast-forward ancestry, and is limited to one push. Push: same run count, now green. Edits: 1 per PR instead of 1 per push. Chosen.
(b) read the live body Same as today. Still red on every push: the live body is still stale when the push run starts, and the rebind edit still runs. It addresses the ordering race tracked in #5166, not this defect, and stays with that Delivery.
(c) head-keyed artifact (bot check or comment) Possible, but it needs a writer credential. The org's CI inventory has no checks: write App grant (.fsgg/routine-eligibility-activation.json). No edit runs. It changes where the marker lives. Every reader would have to move: pr-lane-admission, routine-delivery, board-currency, ContextView.fs, the skills, and the 9 receivers' installed five-file closure. That is a contract migration, not a minimal fix.

Security property kept. The marker still binds the author's declaration to an exact head, and that exact head is required at merge. A stale or forged marker cannot admit a different head:

  • an unrelated head is refused;
  • a marker two pushes behind is refused;
  • a forced push offers no parent;
  • an absent or forged before offers no parent;
  • edited, opened and reopened runs have no trusted parent;
  • duplicate or protected markers are not rescued;
  • the envelope proves the ancestry again, and so does the validator;
  • the validator refuses --routine-previous-head outside the trusted --routine-policy-ref route.

Unchanged.

Residual. A gh pr merge or UI merge that bypasses the routine merge route is gated only by branch protection. That was already true: routine-eligibility was never a required context. Before this change, though, a stale marker showed up as a red check. Now it shows up as a green check with a routine-rebind-owed warning.

Changes

  • .github/workflows/routine-eligibility.yml: passes EVENT_ACTION and EVENT_BEFORE, proves fast-forward ancestry by bounded deepening on a synchronize of routine/*, and forwards --previous-head.
  • scripts/check-routine-eligibility-envelope.py: optional --previous-head. It must be lowercase 40-hex, differ from the head, and be an ancestor of the head; otherwise it refuses. It is forwarded as --routine-previous-head.
  • scripts/check-claim-generation.py: the provisional admission and warning; the flag is refused without --routine-policy-ref. Census pin updated in docs/coordination/v1-writer-census.json. The sink kinds did not change.
  • tests/routine-eligibility/push_parent.py (new): 15 causal scenarios, wired into run.sh. moved_base.py's fixture now accepts event fields. fixtures/pre-5189-routine-eligibility.workflow.txt is the mutation control.
  • routine-eligibility-selftest.yml paths now include scripts/check-claim-generation.py, tools/routine-delivery.py and tools/pr-lane-admission.py, so validator or merge-route changes re-run these controls.

Controls

Generator output from python3 tests/routine-eligibility/push_parent.py: routine-eligibility push-parent: scenarios=15 passed=15 admit=5 refuse=9 mutation-controls=1.

These run the real workflow shell, the real base-loaded envelope and the real validator and policy over a shallow file:// transport. The clock is not used, and the sandbox is a temporary directory per scenario.

Required control Scenario(s)
stale marker from an unrelated head stale-marker-from-an-unrelated-head-refuses, marker-two-pushes-behind-refuses
forced push forced-push-offers-no-parent-and-refuses
rebind rebind-edit-is-admitted-exactly-not-provisionally, rebound-marker-on-a-synchronize-run-is-exact
forged marker forged-marker-and-before-naming-an-absent-commit-refuse, duplicate-or-protected-marker-is-not-rescued-by-the-parent, envelope-re-proves-the-parent-and-refuses-a-forged-one, validator-refuses-a-previous-head-outside-the-trusted-route, push-parent-marker-on-an-edited-run-refuses
merge with an unrebound marker merge-with-an-unrebound-marker-is-refused-by-the-live-body-check calls the real routine_contract with a recording API. It refuses with stale/missing head and never consults eligibility. Once rebound, it returns passed.
the defect itself, with a mutation control push-parent-marker-is-admitted-provisionally-with-a-rebind-warning; pre-5189-workflow-refuses-the-same-push uses the same inputs with the pre-change workflow and gets routine-changed-head. Also multi-commit-push-connects-by-bounded-deepening and push-parent-with-a-moved-base-is-still-admitted.

A manual validator mutation that disables the allowance makes the admit scenarios fail (expected ok=True with 'routine delivery provisionally admitted'; got rc=1). It was reverted, and that mutated state is not in the diff.

Pipeline preflight

This is a single linear job. The interaction is event order (push, then edit), and the concurrency did not change. I chose static invariants (run.sh require lines) plus the existing real-shell causal fixture, extended. A custom Quint model is deferred because the ordering surface is already modelled by check-pr-concurrency.py, which is unchanged and green, and a new model would duplicate it.

Kit, receivers and release

These five files are the canonical routine producer closure: .fsgg/routine-development.json, .github/workflows/routine-eligibility.yml, scripts/check-claim-generation.py, scripts/check-routine-eligibility-envelope.py and scripts/lib/gate.py. FS.GG.Drivers ships them (src/FS.GG.Drivers/workspace-inventory.json sources them from this repository), and receivers carry installed copies.

Receivers are not changed by this PR. On origin/main, Audio, Coordination, Governance, Rendering, SDD, Templates, FsQuint, FSBarV2 and Game each carry their own installed copy. Most use an older event-base workflow. They keep failing once per push until they adopt the new closure. Adoption is a separate effect: a Drivers package release (coherent-set version bump) and a per-receiver closure update through the existing routine-producer repair route. The new envelope argument is optional and the workflow sends it only when it is valid, so a receiver that updates all five files together stays consistent. Mixing old and new files is not supported, as with any closure update.

Interim practice: rebind before pushing

Root asked whether the worker workaround (edit the marker to the locally known new SHA, then push) becomes documented practice. My answer:

  • Until a repository runs this closure (.github before this PR merges, and every receiver until it adopts it), it is the right interim practice. It needs no code. The push event carries the bound marker, so the new head's routine-eligibility run is green. This PR's own second head 03a77d1b was pushed that way.
  • Its costs and conditions:
    • It still costs one body edit per push (7 runs on .github).
    • The edit run on the superseded head goes red.
    • Read the body back before you push. If the edit lands after the push event, the push carries the old body.
    • If the push is then rejected, the body names a head that does not exist. Re-edit it.
  • Once the closure is live, the workaround is unnecessary for intermediate pushes. They need no edit at all. One rebind is owed before merge. Doing that final rebind with this same before-push ordering is still the cheapest way when one last push is needed, because the final head is exact from its first run.

The skill text (pnext-item: "A moved head must be reviewed and rebound") is compatible with both ways of working and is not changed here. It is a FS.GG.Kit member, so wording the interim practice there needs its own kit-bump PR. I recommend one only if Root wants the interim practice written down before receivers adopt the closure.

Moved base (Root scope addition, FS.GG.Game#755)

Game#755 failed routine-eligibility with fetched base <new> differs from event base <old> after an unrelated merge moved main (run 41300940959). That refusal exists only in Game's installed workflow (f1d6a436, line 66), which is the pre-#4994 event-base copy.

The canonical closure on .github main already tolerates this:

  • the workflow resolves the live base at run time;
  • the event base must be an ancestor of the live base;
  • the head must not already be contained in the live base;
  • the change is measured as merge-base(live, head)..head.

tests/routine-eligibility/moved_base.py (23 scenarios, generator: scenarios=23 passed=23 admit=8 refuse=13 mutation-controls=2) is the causal control. Its mutation control moved-base-legacy-refuses-with-the-reported-message reproduces Game's exact message with the legacy bytes. Its admit scenario moved-base-unchanged-routine-change-passes-without-rebase passes the same PR under the canonical workflow.

This PR leaves that path unchanged and adds push-parent-with-a-moved-base-is-still-admitted for the combined case: main moved and the push marker names the parent.

Game#755 is fixed by the same remainder as #5189's receivers: adopting the current canonical five-file closure. Game's envelope and validator differ from canonical too (680b7cbe, f78531e3 vs 7bcf7a9e, 7995b533 on .github main before this PR).

Local gates (this head)

All of these were run locally on this head (the runner script runs each and records its exit code and last output line):

routine-eligibility rc=0 ok [refuse] merge-with-an-unrebound-marker-is-refused-by-the-live-body-check
claim-generation rc=0 claim-generation fixture: 60 passed, 0 failed
claim-producer rc=0 claim-producer fixture: 22 passed, 0 failed
pr-concurrency-fixture rc=0 pr-concurrency fixture — OK
pr-concurrency rc=0 ok: 63 governed workflow(s) (+4 PR-event-only) declare the PR-keyed shape, 8 of them head-keyed for a non-head trigger; 1369 causal scenario(s) hold (cancel on supersede, no cancel
paths-coherence-fixture rc=0 paths-coherence fixture — OK
paths-coherence rc=0 ok: 122 workflow(s) declaring both filters agree; 120 declared project subject(s) cover their `ProjectReference` closure; 20 declared gate script surface(s) covered; 4 exception(s)
timeouts rc=0 ok: every job declares a timeout — 317 job(s) audited.
timeouts-fixture rc=0 workflow-timeouts fixture — OK
job-uniqueness-fixture rc=0 job-uniqueness fixture — OK
reusable-job-ids-fixture rc=0 reusable-job-ids fixture — OK
harness-identity-fixture rc=0 harness-identity fixture: 14 passed, 0 failed
harness-identity rc=0 check-harness-identity: OK — 168 shell file(s) audited (161 `--worker` site(s)), 217 F# file(s) audited (17 argv-driven binding(s)) — every identity-dependent invocation decided th
generator-list rc=0 -------- 59 passed, 0 failed
worker-id-attractor-fixture rc=0 worker-id-attractor fixture — OK
worker-id-attractor rc=0 ok: no literal worker id, and exactly one mint idiom — 56 worker-id mention(s) audited, 18 showing the sanctioned mint; 9 printed remedy(s) in src/FS.GG.Coord.Cli + src/FS.GG.Coord
sparse-checkout-closure rc=0 check-sparse-checkout-closure: OK — 4 sparse pattern(s) across 3 fully-graded cross-repo checkout(s) in 211 workflow(s) of FS-GG/.github; 19 full clone(s) not graded; 1 step(s) UNG
sparse-checkout-fixture rc=0 sparse-checkout-closure fixture: 38 passed, 0 failed.
scheduled-engine-prov rc=0 check-scheduled-engine-provisioning: OK — 104 job(s) in schedule-only workflows; every one that runs the engine builds it first
test-selector rc=0 test-selector fixture — OK
v1-census-structural rc=0 v1-writer-census: PASS (structural; 54 command roots: 20 always, 6 conditional, 28 never; 9 out-of-catalogue arms; 74 source identities; 339 tracked executables)
pr-lane-admission rc=0 OK
main-health-plan rc=0 excluded: telemetry-host-package.yml: reads workflow inputs
policy-runner rc=0 policy subject: ok: m6-cutover-acceptance-fixture
recipe-landable rc=0 recipe-landable fixture — OK
repos-registry rc=0 repos-registry fixture — OK
dispatch-broker rc=0 GS2-08.9 dispatch/repair routes are read-only or local-only; historical SHA capability is explicitly unresolved
workflow-permissions-fixture rc=0 workflow-permissions fixture — OK
change-completeness rc=0 change-completeness fixture: 30 passed, 0 failed
drivers-verify rc=0 verify-package: OK

30/30 exit 0. drivers-verify ran verify-package.sh without a prior locked dotnet restore. CI runs both.

🤖 Generated with Claude Code

rev and others added 2 commits November 1, 2026 11:46
…act head at merge (Refs #5189)

A push cannot carry a marker naming a head that did not exist when the body was written, so every
push to a routine PR failed routine-eligibility once and cost a body edit (7 runs on .github) to
clear. On synchronize only, the workflow now offers GitHub's own `before` to the validator when
bounded deepening proves the push only added commits on top of it. A marker naming exactly that
parent is admitted provisionally with a routine-rebind-owed warning; every other routine check still
runs against the new head. The exact binding stays at the merge route, whose routine_contract reads
the live body and refuses any head= other than the merged head.

Controls (tests/routine-eligibility/push_parent.py, 15 scenarios): stale marker from an unrelated
head, two pushes behind, forced push, rebind, forged marker/before, duplicate and protected markers,
envelope and validator re-proof, unrebound merge refused by the real routine_contract, and the
pre-change workflow as the mutation control. Census pin for check-claim-generation.py updated.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The first head's change-completeness and declared-closes read a PR body whose option table contained
a closing keyword. change-completeness reads the body from the push event payload and does not run on
`edited`, so a fresh head is the only way it reads the corrected body.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@EHotwagner
EHotwagner marked this pull request as ready for review November 1, 2026 13:15
@EHotwagner

Copy link
Copy Markdown
Contributor Author

Root/fdev: independent adversarial review APPROVED 03a77d1b.

  • Attacks refused: a forged or zero previous head, force-push, a non-ancestor, a deep push past the search bound, and a duplicate or split marker.
  • Trust boundary: pull_request_target makes before GitHub-controlled.
  • Merge path: routine_contract enforces exact binding against the live body, and the merge sends sha=expectedHead.
  • Counts: push_parent 15/15 and moved_base 23/23, plus 10 adversarial scenarios.

Triage

Merging. #5189 continues with receiver adoption. Refs #5189

@EHotwagner
EHotwagner merged commit 808900e into main Nov 1, 2026
83 of 84 checks passed
@EHotwagner
EHotwagner deleted the routine/routine-marker-push-race branch November 1, 2026 13:22
This was referenced Nov 1, 2026
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