Skip to content

Fix the base fetch so a rewind cannot abort the job (#807) - #25

Merged
Birfy merged 1 commit into
mainfrom
fix/807-fetch-base-refspec
Oct 23, 2026
Merged

Birfy merged 1 commit into
mainfrom
fix/807-fetch-base-refspec

Conversation

@Birfy

@Birfy Birfy commented Oct 23, 2026 •

Copy link
Copy Markdown
Owner

Reproduce

At 61a6af6f, on Birfy/FlyBot: a push to main killed the Fetch origin/main for arch-docs check step of .github/actions/flybot-tests/action.yml before lint and the suite ran (run 39898972351, job 120235871280):

 ! [rejected]        main       -> origin/main  (non-fast-forward)
##[error]Process completed with exit code 1.

Local reproduction of the same ordering, no network: bare origin with A then B on main; depth-1 clone (what checkout leaves behind, refs/remotes/origin/main at B); main moved back to A; then the step's fetch. Unforced, it is refused and set -euo pipefail aborts the job:

$ refs/remotes/origin/main at 99fedbf, origin/main at 617ba42
 ! [rejected]        main       -> origin/main  (non-fast-forward)

Forced, the same step completes in 235 ms and resolves the base to the commit it fetched:

 + 99fedbf...617ba42 main       -> origin/main  (forced update)
refs/remotes/origin/main = 617ba423686b7d3e1da6f42c323dc2f0d5e583ab
step exit=0

What changed

  • .github/actions/flybot-tests/action.yml: the base fetch is now git fetch --no-tags --depth=1 origin +main:refs/remotes/origin/main, with a three-line comment saying why the + is required. Nothing else in the step moves: set -euo pipefail stays, the --deepen/--unshallow fallbacks stay, the fetch is not wrapped in || true, and git merge-base origin/main HEAD plus git rev-parse --verify origin/main^{commit} still verify the base before arch-docs uses it.
  • .github/scripts/fetch-base.test.mjs (new): the check the issue asks for, run by the existing node --test ".github/scripts/*.test.mjs" step. It reads the actual run: block out of the action (no copy of the command in the test) and runs it as bash in a scratch repository. Three tests: the refspec stays forced and the fetch is not swallowed while both verification lines stay; a base older than the checkout ref does not abort the step and the resolved base is the fetched commit; a base that cannot be fetched fails the job instead of passing on the stale ref.
  • Other git fetch call sites: action.yml:50-51 (--deepen) and :54 (--unshallow, then origin main as an argument) never update a named ref — they write FETCH_HEAD only — so there is no ref-forwarding risk there. live-e2e.yml:35 and nightly.yml:36 use fetch-depth: 0 and no fetch of their own. Repository-wide grep for git fetch finds no other ref-updating call.

What I ran

At this tip 717d63547308931e605c77be49093d026833945d, one commit on main 10e26bb6 (diff: action.yml +4/−1, fetch-base.test.mjs +110):

  • node --test .github/scripts/fetch-base.test.mjs → 3/3 pass.
  • node --test ".github/scripts/*.test.mjs" (git on PATH) → 245/245 pass, 0 skipped.
  • node .github/scripts/testing-layers.mjs --check → 319 test file(s) in 12 workflow(s), every test file and workflow classified; no manifest edit was needed because .github/scripts/*.test.mjs already covers the new file.
  • node .github/scripts/changelog.mjs check → 6 fragment(s) … are well formed (no fragment added: this changes no user-visible behaviour).
  • Fork CI at this tip (Test, run 39904363604): all 9 checks success, tests-ok green. In the ubuntu-latest log the fixed step is Fetch origin/main for arch-docs check → success, * [new branch] main -> origin/main, then base 10e26bb6eabd0a8abf9fa2520dd3446160e6cc9b, and the job continues into Lint (green) and the suite. package: "Tests of the repo scripts (risk tier, labels)" and "Lint the workflows (actionlint)" both success; plugins (social), plugins (tree), plugins (timeline) success.

Red without the source change

Each kill condition from the issue, applied to the action and run against the new test:

  1. Unforced main:refs/remotes/origin/main → 1 pass / 2 fail: the refspec needs +: checkout leaves the ref at the tip, so an older base is a non-fast-forward, and the runtime test aborts with the verbatim ! [rejected] main -> origin/main (non-fast-forward).
  2. +main:… || true → 1 pass / 2 fail: a failed base fetch must fail the job, not be swallowed, and the missing-base test fails with the step must fail when the base cannot be fetched — with || true the step passes against the stale ref, which is the failure mode the issue calls worse than red.
  3. Remove git merge-base origin/main HEAD >/dev/null and git rev-parse --verify origin/main^{commit} → 2 pass / 1 fail: the base must stay verified.

Not run

  • actionlint locally: this machine cannot reach github.com:443, so the pinned 1.7.7 release cannot be downloaded here. It ran in fork CI at this tip (step 12, success), which is the evidence used above.
  • No end-to-end proof that a real two-push race is absorbed: the new test reproduces the ordering (ref at the tip, base moves back) deterministically instead of racing two pushes.
  • core lint, build-client --check, plugin suites: untouched paths, no code change under core/ or plugins/.

Risk

  • The forced update can rewind refs/remotes/origin/main when the run's base is older than the ref checkout left behind. That ref is used in this job only by the arch-docs check (merge-base, then its own diff against origin/main), so what the check needs is exactly the fetched base — and the existing merge-base/rev-parse lines still refuse a base that does not contain HEAD, and a missing base still fails the job.
  • If main is genuinely force-pushed backwards, the base fetch now succeeds where it used to abort; the following git merge-base origin/main HEAD is what decides, so the outcome is a real red in arch-docs, not a silent pass.
  • Cost: three extra local git scenarios in the package job, about 0.6 s total.

Fixes ThinkFlowLab#807
Tip SHA: 717d635

`git fetch origin main:refs/remotes/origin/main` without `+` only updates the
ref as a fast-forward. actions/checkout leaves that ref at the tip it saw, so a
push whose base is not a descendant of it made the step die with
`! [rejected] ... (non-fast-forward)` before lint and the suite ran.

Fetch with an explicit force refspec instead. The fetch still fails loudly when
the base cannot be fetched, and the merge-base and rev-parse checks stay, so the
arch-docs check never runs against an unverified base.

`.github/scripts/fetch-base.test.mjs` reads the real step out of the action and
runs it against a scratch repository: a base older than the checkout ref must not
abort, a base that cannot be fetched must fail, and the refspec must stay forced.
@Birfy
Birfy force-pushed the fix/807-fetch-base-refspec branch from 265ec44 to 717d635 Compare October 23, 2026 05:11
@Birfy

Birfy commented Oct 23, 2026

Copy link
Copy Markdown
Owner Author

Verdict: PASS at 717d635
Written-by: Reviewer2

Ran:

  • CI at this tip, Test run 39904363604 (pull_request): ubuntu-latest · Node 24 · DSH 0.2.0-rc.2 success, package success, tests-ok success, plugins (social/tree/timeline), merge-gate, reviewed tip, risk tier success — 9/9, all completed.
  • package job log: the three new tests ran for real in CI, none skipped — the step fetches the base with an explicit force and still verifies it 0.98 ms, a base older than the checkout ref does not abort the step 301 ms, a base that cannot be fetched fails instead of passing on the stale ref 103 ms.
  • Node 24 job log: the step's own command is now git fetch --no-tags --depth=1 origin +main:refs/remotes/origin/main, and the step completed; the only ! [rejected] / non-fast-forward text in that log is the new comment being echoed.
  • Local (no git on this machine, so the two runtime cases skip): cd .github/scripts && node --test fetch-base.test.mjs → 1 pass, 2 skipped.
  • Kill 1 — drop the +: only the refspec needs \+`: checkout leaves the ref at the tip, so an older base is a non-fast-forward` red.
  • Kill 2 — append || true: only a failed base fetch must fail the job, not be swallowed red.
  • Kill 3 — delete the closing git merge-base origin/main HEAD >/dev/null: only the base must stay verified red.
  • Restored: 1 pass / 2 skip locally, and the three cases that CI ran above are green at this tip.

What I checked:

  • The test reads the step's run: block out of .github/actions/flybot-tests/action.yml and executes it, rather than keeping a copy of the command; so it cannot drift from the action.
  • Nothing else in the step was weakened: set -euo pipefail stays, both --deepen fallbacks and the --unshallow fallback stay, and git merge-base origin/main HEAD plus git rev-parse --verify origin/main^{commit} still refuse a base that does not contain HEAD or does not resolve.
  • Other fetch call sites are as the body says: the --deepen/--unshallow calls write FETCH_HEAD only, and live-e2e.yml:35 / nightly.yml:36 use fetch-depth: 0 with no fetch of their own.
  • The three negative cases are behaviour, not text: the older-base case runs the real step in a scratch repository where refs/remotes/origin/main is ahead of the base, and the missing-base case deletes main on origin so a swallowed failure would leave the stale ref to pass.

Non-blocking:

  • The forced refspec can rewind refs/remotes/origin/main; the step then relies on merge-base/rev-parse for correctness, so a genuinely force-pushed-backwards main now fails in arch-docs instead of aborting at the fetch. That is the trade the body states, and it is the right side of it.
  • Once this is in main, the "Fetch origin/main red means no evidence" exception closes: any red in that step becomes evidence again.

@github-actions github-actions Bot added the pass label Oct 23, 2026
@Birfy
Birfy merged commit cede842 into main Oct 23, 2026
9 checks passed
@Birfy
Birfy deleted the fix/807-fetch-base-refspec branch October 23, 2026 05:29
@github-actions github-actions Bot removed the agent-pr An agent opened this pull request label Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A main push can abort in Fetch origin/main (! [rejected] … non-fast-forward) before lint and the suite run

1 participant