Skip to content

fix(fr-gate): an ancestry-only promotion has nothing to gate - #109

Merged
LukasWodka merged 3 commits into
developfrom
fix/fr-gate-ancestry-only
Jul 30, 2026
Merged

fix(fr-gate): an ancestry-only promotion has nothing to gate#109
LukasWodka merged 3 commits into
developfrom
fix/fr-gate-ancestry-only

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Fixes the deadlock that kept data-ingestors out of today's prod hop.

The deadlock

The no-items fallback did this:

# If commit subjects yielded no PR refs, gate the promotion PR itself.
NUMBERS="$PROMOTION_PR"

which is unsatisfiable by construction: set-pr-status parks a promotion PR in Code review, and the only way out of that column is merging — which this gate is what blocks. So the train could never merge that repo, and the failure read as a legitimate governance block (❌ #423 — Status='Code review', required 'Ready for prod') rather than as a bug.

Why the item set was legitimately empty

data-ingestors' master...staging range contained exactly one commit — the mirror merge from the staging hop — and 0 changed files, because a manual promotion (data-ingestors#421) had already carried that content to master. Both PRs associated with that commit were correctly filtered as train plumbing (#415 by its release-train/* head, #423 by still being open). So there was no cargo, because the promotion genuinely ships nothing.

The fix

Distinguish the two causes of an empty item set by whether the range touches any file:

RangeMeaningVerdict
0 files changedancestry-only — nothing shippedpass, with a notice
>0 files changedreal content we couldn't attributegate the promotion PR → fail closed

An unset file count falls into the fail-closed branch, so failing to compute it can never become a pass. The existing unattributable-commit check is untouched, so a direct push into the range still fails closed on its own terms.

Verification

actionlint + shellcheck clean at CI-pinned versions. All four decision paths exercised locally:

PRS='' files=0 -> PASS (ancestry-only) # the DI case
PRS='' files=7 -> gate promotion PR (fails closed) # attribution broke
PRS='1364 …' files=12 -> verify items # normal promotion
PRS='' files='' -> gate promotion PR (fails closed) # unset defaults safe

Confirmed against the live repo that data-ingestors' range really is ahead=1, files_changed=0 — I checked that the "nothing to gate" premise holds empirically before relying on it, rather than assuming an ancestry-only range implies no content.


Note

Medium Risk
Changes promotion merge gating logic; the early-pass path is narrow (0 files + empty UNATTRIB) but a wrong file count could theoretically allow or block merges incorrectly.

Overview
Fixes a release-train deadlock when the FR gate found no attributable PRs and always fell back to gating the promotion PR itself—an unsatisfiable loop because that PR sits in Code review until merge.

The Discover items step now counts how many files the promotion range actually contributes using the GitHub compare API with three-dot semantics (BASE...HEAD), with retries and fail-closed behavior if the count cannot be obtained. That count is exposed as changed_files.

The Verify step uses that signal when the attributed PR list is empty: if changed_files is 0 and there are no unattributable commits, the workflow passes with a notice (ancestry-only mirror merge, no new content to review). If files changed or attribution is incomplete, it still gates the promotion PR and fails closed as before.

Reviewed by Cursor Bugbot for commit ce45d7f. Bugbot is set up for automated code reviews on this repo. Configure here.

The no-items fallback gated the promotion PR itself, which is
unsatisfiable BY CONSTRUCTION: set-pr-status parks that PR in 'Code
review', and it can only leave that column by merging -- which this gate
blocks. data-ingestors deadlocked on exactly this in today's prod hop.
It happened because the range's only new commit was the train's own
mirror merge, whose content master already held (a manual promotion had
carried it there first): 1 commit ahead, 0 files changed. Both associated
PRs were correctly filtered as train plumbing, leaving no cargo -- so the
fallback fired on a promotion that ships nothing.
Now distinguished by whether the range changes any file:
0 files -> ancestry-only, nothing shipped, nothing to gate -> pass
>0 files -> real content we could not attribute -> gate the promotion
PR, i.e. fail closed and force a human look
An unset file count defaults to the fail-closed branch, so a failure to
compute it can never turn into a pass.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

👋 Heads-up — Code review queue is at 35 / 30

Above the WIP limit. The team convention is to review existing PRs before opening new work.

Open PRs currently in Code review (oldest first):

Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.)

Comment thread.github/workflows/fr-gate.yml
The early exit sat ~150 lines above the UNATTRIB fail-closed check, so a
tree-identical range containing a commit we could not verify (e.g.
commits/{sha}/pulls erroring three times) would have passed the gate --
and my PR body wrongly claimed that check was untouched.
0 files changed means 'nothing shipped' only if every commit was
successfully accounted for, so the ancestry pass now requires UNATTRIB to
be empty as well.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Valid, and a real fail-open in my own fix — fixed.

The early exit sat ~150 lines above the UNATTRIB fail-closed check (my exit 0 at ~273, that check at ~431), so it jumped straight over it. A tree-identical range containing a commit we could not verifycommits/{sha}/pulls erroring three times, say — would have passed the gate. And my PR body asserted "the existing unattributable-commit check is untouched", which was simply wrong: an early return in the same step is exactly how you touch it.

The premise needed tightening too: 0 files changed means "nothing shipped" only if every commit was successfully accounted for. An unverifiable commit means we don't actually know what the range contains, so the file count proves nothing. The ancestry pass now requires UNATTRIB to be empty as well.

Re-tested every path:

PRS='' files=0 unattrib='' -> PASS (ancestry-only) # the DI case
PRS='' files=0 unattrib='abc1234' -> FAIL CLOSED # this finding
PRS='' files=7 unattrib='' -> FAIL CLOSED # attribution broke
PRS='1364' files=12 unattrib='' -> verify items # normal
PRS='' files='' unattrib='' -> FAIL CLOSED # unset defaults safe

actionlint + shellcheck clean. This is the third time today Bugbot has caught a fail-open in a guard I wrote — worth noting the pattern: my guards keep failing open on the error path while reading correctly on the happy path.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d5c8518. Configure here.

Comment thread.github/workflows/fr-gate.yml Outdated
The emptiness check used a two-dot git diff, which counts files the BASE
moved on alone -- so it measured 'how do the two tips differ', not 'what
did this promotion contribute'. On data-ingestors that reports 6 files
(master's own commits that staging lacks) where the correct three-dot
answer is 0, so the ancestry-only branch would never have been taken and
the deadlock this PR exists to fix would have survived.
I had verified the premise with the compare API (three-dot) but
implemented local two-dot git -- validating with one method and shipping
another. Now uses the compare API directly, so the check and its
verification are the same operation.
The API also avoids local three-dot's silent merge-base pick: this repo's
history has multiple merge bases after the mirror reconciliations. Any
failure to count yields -1, which takes the fail-closed branch.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Valid, and worse than the description suggests — my fix would not have fixed its own motivating case. Fixed.

I used two-dot git diff BASE..HEAD, which compares the two tips and therefore counts files the base moved on alone. Measured on the actual data-ingestors case:

two-dot master..staging = 6 files <- what I shipped
three-dot master...staging = 0 files <- the correct answer

Those 6 are all base-only: .cursor/BUGBOT.md, auto-release-on-master.yml, code-quality-caller.yml, release-image.yml, RELEASING.md, tracebloc_ingestor/__init__.py — master's own #421 content that staging lacks. So the check would have returned 6, taken the fail-closed branch, and the DI deadlock would have persisted despite this PR.

The underlying mistake is worth naming: I verified the premise with the compare API (three-dot) and then implemented local two-dot git. Validating with one operation and shipping another is how a fix passes review while not working. It now calls the compare API directly, so the check and the thing I verified are literally the same operation.

That also sidesteps a second trap: local git diff A...Bsilently picks one merge base, and this history has multiple after the mirror reconciliations (git even warns: multiple merge bases, using 07ff47f1). The API has no such ambiguity.

Any failure to count now yields -1, which takes the fail-closed branch — an uncounted range is never treated as empty.

Re-verified against the live repos:

data-ingestors compare(master...staging).files = 0 -> ancestry pass ✓
client compare(main...staging).files = 36 -> will not pass ✓
files=-1 (API failed) -> FAIL CLOSED
files=0 + unattrib commit -> FAIL CLOSED
files=8 -> FAIL CLOSED

actionlint + shellcheck clean.

bugbot run

@LukasWodka
LukasWodka merged commit 7c4ffed into developJul 30, 2026
2 checks passed
@LukasWodka
LukasWodka deleted the fix/fr-gate-ancestry-only branch July 30, 2026 13:05
Sign up for freeto 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

@LukasWodka