[PR #141] [MERGED] ci(bump-callers): add preflight.sh ancestry direction guard (BE-6675) #151

New issue

Closed

opened 2026-10-26 22:36:49 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:49 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/141
Author: @mattmillerai
Created: 8/6/2026
Status: ✅ Merged
Merged: 8/11/2026
Merged by: @mattmillerai

Base: matt/be-6475-bump-callers-preflight ← Head: matt/be-6675-preflight-ancestry-guard

📝 Commits (2)

21b7cce ci(bump-callers): add preflight.sh ancestry direction guard (BE-6675)
93d0f78 fix(bump-callers): measure the direction of BOTH tip moves in preflight (BE-6675)

📊 Changes

3 files changed (+352 additions, -21 deletions)

View changed files

📝 .github/bump-callers/README.md (+58 -8)
📝 .github/bump-callers/preflight.sh (+119 -13)
📝 .github/bump-callers/tests/test_preflight.sh (+175 -0)

📄 Description

STACKED — merging lands on matt/be-6475-bump-callers-preflight (owned by @mattmillerai, PR #136), NOT main. preflight.sh only exists on that branch; #136 must merge first. GitHub retargets this PR to main automatically when #136 lands.

ELI-5

preflight.sh is the guard that decides whether a bump run should fan a new pin out to every consumer repo. When it notices that main has moved since this run started, it compares the two and decides "stale re-run, do nothing" or "still the only run — pin the tip instead."

It never checked which direction main moved. If main went backwards — someone force-pushed, a revert-reset landed, or a stale read replica answered the tip lookup — the guard happily compared against a commit that came before this run, and both outcomes were silently wrong:

watched content differs at the older tip → logs "the newer commit has its own run" (about an older commit) and exits green, so the change is never pinned and the whole fleet freezes behind a run that will never come;
watched content matches → the re-point hands new_sha the older tip, pinning every caller backwards.

This adds one line of git — "is the fetched tip a descendant of this run's commit?" — and makes a no a loud ::error:: + exit 1 instead of a quiet green skip.

What changed

Direction guard (git merge-base --is-ancestor HEAD FETCH_HEAD), placed after the existing FETCH_HEAD verification and the main_tip="$fetched_tip" assignment, and before any blob/tree comparison — so both failure modes above become loud, not just one.
The fetch now deepens. --depth=1 was replaced with a .git/shallow probe + --unshallow. This is not cosmetic: against a --depth=1 graft --is-ancestor returns false even for a legitimate forward move, so the naive one-liner would hard-fail every re-point. --unshallow errors out on an already-complete clone, hence the probe rather than an unconditional flag.
refs/heads/main is kept (the bump-pr-risk-callers.yml reference copy fetches a bare main). The existing tag-shadowing comment above the fetch explains why; I added an explicit "do not align with pr-risk here" note so the next person doesn't regress it.
Header + README updated. The "bump-pr-risk-callers.yml needs a decision rather than a swap" note is now the decision (BE-6670): direction guard adopted for all fleets; git rev-list deliberately not ported.

Why `rev-list` is not ported

pr-risk's rev-list test exists because that fleet has no re-point: on a land-then-revert (net content change of zero) its content comparison would call this run the only one for the change and pin callers backwards to github.sha. preflight.sh already answers that case by pinning the verified tip — which on a land-then-revert is the revert commit, i.e. forward. Layering rev-list on top would only add a stricter staleness verdict that skips a run whose content the tip still needs pinned. New test land_then_revert documents this mechanism.

Verification (both claims falsified empirically, not just asserted)

bash .github/bump-callers/tests/test_preflight.sh → 94 passed, 0 failed. shellcheck -x clean on all four bump-callers scripts. test_bump_callers.sh → 175 passed, 0 failed. check_agents_md.py passes.

Four new cases, and I confirmed each one actually tests the new behavior by reverting the change and re-running:

Guard removed → 10 failures, reproducing both documented bugs exactly: the content-differs case exits 0 with proceed=false and the false stale run/re-run log; the content-equal case emits new_sha = the older SHA (backwards re-point).
--depth=1 restored, guard kept → 38 failures, including the shallow happy path and every full-clone re-point case. That is the spike's empirical finding reproduced here: a shallow FETCH_HEAD makes --is-ancestor reject legitimate forward moves. The deepening is load-bearing, not incidental.
Capability preserved, not just denied. Since this diff adds a deny path, I also verified it does not block the legitimate cases it must still allow: forward re-point from a shallow checkout (shallow_repoint, Actions' real actions/checkout shape — the existing full-clone helper never exercised the --unshallow arm), land-then-revert, tag-shadow, and the benign ls-remote→fetch race all still pass.

Judgment calls

Stacked, not handed back. The ticket says to stop if #136 is unmerged. #136 is open but green, MERGEABLE, and carries preflight.sh, so the standing always-stack directive applies: I built on its branch rather than blocking. Nothing else changes.
merge-base exit codes are not split. resolve_oid splits "absent" (exit 1) from "lookup failed" (exit >1) because those verdicts diverge — absence exits 0 silently. Here they converge (both are ::error:: + exit 1), so splitting would only reword a log line. Documented in a comment rather than adding an untestable branch.
Ordering vs. the own-commit decommission warning. With the guard before the blob reads, a backwards-moved main now errors even when the watched file is absent at this run's own commit — i.e. the direction error wins over the "decommissioned" warning. That is intended: a tip that contradicts history is not trustworthy evidence for a decommission verdict either.
Blast radius is zero at merge. Nothing consumes preflight.sh yet (the entrypoints still carry their inline copies), so this hardens the script ahead of the swap rather than changing any fleet's live behavior.

Provenance

Ticket BE-6675, implementing the decision from spike BE-6670. Reference implementation: bump-pr-risk-callers.yml:196-231 on main.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/141 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/6/2026 **Status:** ✅ Merged **Merged:** 8/11/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `matt/be-6475-bump-callers-preflight` ← **Head:** `matt/be-6675-preflight-ancestry-guard` --- ### 📝 Commits (2) - [`21b7cce`](https://github.com/Comfy-Org/github-workflows/commit/21b7cce66d9ccb4dcf2863efe556c841a8af230f) ci(bump-callers): add preflight.sh ancestry direction guard (BE-6675) - [`93d0f78`](https://github.com/Comfy-Org/github-workflows/commit/93d0f7892bbb36c51a8e7dff5645d5530a6e2c1b) fix(bump-callers): measure the direction of BOTH tip moves in preflight (BE-6675) ### 📊 Changes **3 files changed** (+352 additions, -21 deletions) <details> <summary>View changed files</summary> 📝 `.github/bump-callers/README.md` (+58 -8) 📝 `.github/bump-callers/preflight.sh` (+119 -13) 📝 `.github/bump-callers/tests/test_preflight.sh` (+175 -0) </details> ### 📄 Description **STACKED — merging lands on `matt/be-6475-bump-callers-preflight` (owned by @mattmillerai, PR #136), NOT `main`.** `preflight.sh` only exists on that branch; #136 must merge first. GitHub retargets this PR to `main` automatically when #136 lands. ## ELI-5 `preflight.sh` is the guard that decides whether a bump run should fan a new pin out to every consumer repo. When it notices that `main` has moved since this run started, it compares the two and decides "stale re-run, do nothing" or "still the only run — pin the tip instead." It never checked which *direction* main moved. If main went **backwards** — someone force-pushed, a revert-reset landed, or a stale read replica answered the tip lookup — the guard happily compared against a commit that came *before* this run, and both outcomes were silently wrong: - watched content **differs** at the older tip → logs "the newer commit has its own run" (about an *older* commit) and exits **green**, so the change is never pinned and the whole fleet freezes behind a run that will never come; - watched content **matches** → the re-point hands `new_sha` the **older** tip, pinning every caller backwards. This adds one line of git — "is the fetched tip a descendant of this run's commit?" — and makes a `no` a loud `::error::` + exit 1 instead of a quiet green skip. ## What changed 1. **Direction guard** (`git merge-base --is-ancestor HEAD FETCH_HEAD`), placed after the existing `FETCH_HEAD` verification and the `main_tip="$fetched_tip"` assignment, and **before** any blob/tree comparison — so both failure modes above become loud, not just one. 2. **The fetch now deepens.** `--depth=1` was replaced with a `.git/shallow` probe + `--unshallow`. This is not cosmetic: against a `--depth=1` graft `--is-ancestor` returns **false even for a legitimate forward move**, so the naive one-liner would hard-fail every re-point. `--unshallow` errors out on an already-complete clone, hence the probe rather than an unconditional flag. 3. **`refs/heads/main` is kept** (the `bump-pr-risk-callers.yml` reference copy fetches a bare `main`). The existing tag-shadowing comment above the fetch explains why; I added an explicit "do not align with pr-risk here" note so the next person doesn't regress it. 4. **Header + README updated.** The "`bump-pr-risk-callers.yml` needs a decision rather than a swap" note is now the decision (BE-6670): direction guard adopted for **all** fleets; `git rev-list` deliberately **not** ported. ### Why `rev-list` is not ported pr-risk's `rev-list` test exists because that fleet has **no re-point**: on a land-then-revert (net content change of zero) its content comparison would call this run the only one for the change and pin callers backwards to `github.sha`. `preflight.sh` already answers that case by pinning the *verified tip* — which on a land-then-revert **is** the revert commit, i.e. forward. Layering `rev-list` on top would only add a stricter staleness verdict that skips a run whose content the tip still needs pinned. New test `land_then_revert` documents this mechanism. ## Verification (both claims falsified empirically, not just asserted) `bash .github/bump-callers/tests/test_preflight.sh` → **94 passed, 0 failed**. `shellcheck -x` clean on all four bump-callers scripts. `test_bump_callers.sh` → 175 passed, 0 failed. `check_agents_md.py` passes. Four new cases, and I confirmed each one actually tests the new behavior by reverting the change and re-running: - **Guard removed** → 10 failures, reproducing *both* documented bugs exactly: the content-differs case exits 0 with `proceed=false` and the false `stale run/re-run` log; the content-equal case emits `new_sha` = the older SHA (backwards re-point). - **`--depth=1` restored, guard kept** → **38 failures**, including the shallow happy path and every full-clone re-point case. That is the spike's empirical finding reproduced here: a shallow FETCH_HEAD makes `--is-ancestor` reject legitimate forward moves. The deepening is load-bearing, not incidental. - **Capability preserved, not just denied.** Since this diff adds a deny path, I also verified it does **not** block the legitimate cases it must still allow: forward re-point from a shallow checkout (`shallow_repoint`, Actions' real `actions/checkout` shape — the existing full-clone helper never exercised the `--unshallow` arm), land-then-revert, tag-shadow, and the benign ls-remote→fetch race all still pass. ## Judgment calls - **Stacked, not handed back.** The ticket says to stop if #136 is unmerged. #136 is open but green, `MERGEABLE`, and carries `preflight.sh`, so the standing always-stack directive applies: I built on its branch rather than blocking. Nothing else changes. - **`merge-base` exit codes are not split.** `resolve_oid` splits "absent" (exit 1) from "lookup failed" (exit >1) because those verdicts *diverge* — absence exits 0 silently. Here they converge (both are `::error::` + exit 1), so splitting would only reword a log line. Documented in a comment rather than adding an untestable branch. - **Ordering vs. the own-commit decommission warning.** With the guard before the blob reads, a backwards-moved main now errors even when the watched file is absent at this run's own commit — i.e. the direction error wins over the "decommissioned" warning. That is intended: a tip that contradicts history is not trustworthy evidence for a decommission verdict either. - **Blast radius is zero at merge.** Nothing consumes `preflight.sh` yet (the entrypoints still carry their inline copies), so this hardens the script ahead of the swap rather than changing any fleet's live behavior. ## Provenance Ticket BE-6675, implementing the decision from spike BE-6670. Reference implementation: `bump-pr-risk-callers.yml:196-231` on `main`. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:49 +00:00