Skip to content

fix(core): identify the test merge from the merge ref, not the payload - #372

Merged
gregberge merged 1 commit into
mainfrom
fix/test-merge-ref
Sep 4, 2026
Merged

fix(core): identify the test merge from the merge ref, not the payload#372
gregberge merged 1 commit into
mainfrom
fix/test-merge-ref

Conversation

@gregberge

Copy link
Copy Markdown
Member

Description

Follow-up to #371, which reported "older baseline" symptoms in production: pull request builds baselined against their fork point, as if the branch had never been merged with the base branch. No orphan builds — just a stale baseline, so every visual change merged into the base branch since the branch was created came back as a change of the pull request. Exactly the bug #371 set out to fix.

#371 itself is not at fault. getTestMergeBaseCommitSha() recognises GitHub's test-merge commit by comparing its second parent against pull_request.head.sha read from the event payload. That value can lag behind the merge ref the runner actually checked out: GitHub recomputes refs/pull/<n>/merge when either branch moves, and does not always fire a new event for it. When a push lands between the event and the recompute, the payload still carries the previous head while the checkout is the newer test-merge commit. The comparison then fails, the guard rejects a genuine test-merge build, and the fallback silently baselines it against the merge base.

The staleness is not theoretical — the verification run below shows the payload reporting base.sha = c0f449d3 while main was already at 78fe66e7. #371's own description hit the same thing from the other side: PR #360's payload base.sha was 26 commits behind the merge ref's real first parent.

GITHUB_REF is set by the runner from the ref the run was triggered on, so on a pull_request event it names the merge ref itself and cannot go stale. Together with the existing check that the build runs on GITHUB_SHA, it identifies the test merge without reading the payload at all. The head.sha comparison stays as the fallback for when the merge ref is absent, where it is still the only way to tell GitHub's test merge apart from a merge the author made.

The parents.length !== 2 check is also split out of the combined condition, so a non-merge HEAD is rejected before either identification path runs — the merge ref alone must not be trusted when the checkout is not a merge.

Type of changes

bug

Checklist

  • I have read the CONTRIBUTING doc
  • The commits message follows the Conventional Commits' policy
  • Lint and unit tests pass locally
  • I have added tests if needed

Further comments

Verified on a real runner, against a real pull request

Fixtures cannot reproduce this: it depends on shallow clones, hidden commit parents, refs/pull/<n>/merge, and payload staleness that only exist on a GitHub-hosted runner. The verification harness lives in argos-ci/argos-test-repository#21. It checks out this repository into the workflow, bundles packages/core/src/ci-environment and find-reference-commit.ts with esbuild, and calls the real exported functions — no re-implementation, and no ARGOS_TOKEN needed. A matrix over ref: runs main and this branch side by side in a single run, and the expected value is read from the GitHub API by the workflow so the assertion does not depend on the code under test.

The test branch is deliberately never rebased: main was advanced twice past its fork point, so the merge base (e5ba9c18) and the commit GitHub merges in (78fe66e7) are different commits.

main (12a8c62) this branch
normal payload 78fe66e7 78fe66e7
stale pull_request.head.sha e5ba9c18 ❌ fork point 78fe66e7

On main, with a stale head:

@argos-ci/core 06b875ec… is not a test-merge commit
resolved with a stale payload : e5ba9c18   ← the fork point
the commit GitHub merged in   : 78fe66e7

What was ruled out along the way

The same harness confirmed the rest of the path is sound, so the fix could stay narrow:

  • Every realistic checkout configuration already workedactions/checkout@v6 and @v4 defaults, fetch-depth: 0, fetch-depth: 50, persist-credentials: false all resolve 78fe66e7. The only configuration that falls back is ref: refs/pull/<n>/head, which is correct: those screenshots do not contain the base branch changes, so the merge base is the right baseline for them.
  • The --depth=2 deepening fetch in getCommitParents() is load-bearing and works. On a default fetch-depth: 1 checkout git rev-list --parents returns the commit alone; the parents only appear after the fetch.
  • resolveBaseline() offers the server a complete candidate list. With /baseline stubbed to find nothing, the CLI sends referenceCommit = 78fe66e7 plus every ancestor down past the fork point.

Reviewer notes

  • Worth a look separately: when /baseline does return a build, resolveBaseline() sends parentCommits: null (find-reference-commit.ts:176). Server-side, listParentCommitShas then falls through to the git provider, which returns [] for a light app installation. So for light-app projects the server gets a single commit and no ancestor chain — if getBaseBucketForBuildAndCommit misses on it, the build has no fallback at all. That is pre-existing and independent of this fix, but fix(core): baseline PR builds against the commit GitHub merged in #371 made its input riskier: the reference commit is now the base branch tip, the commit most likely to still be building or unapproved, where before it was an old settled fork point.
  • I could not force GitHub to race a push against a merge-ref recompute on demand, so the stale payload is reproduced by rewriting head.sha in the event file. The mechanism is confirmed by the real base.sha staleness observed in the same run.

🤖 Generated with Claude Code

@vercel

vercel Bot commented Sep 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
argos-js-sdk-reference Ready Ready Preview Sep 4, 2026 1:25pm UTC

Request Review

getTestMergeBaseCommitSha() recognised GitHub's test-merge commit by
comparing its second parent against pull_request.head.sha. That value can
lag behind the merge ref the runner checked out: GitHub recomputes
refs/pull/<n>/merge when either branch moves, without always firing a new
event. When a push lands in between, the payload still carries the previous
head while the checkout is the newer test-merge commit.

The guard then rejected a genuine test-merge build and fell back to the
merge base, baselining the pull request against its fork point and
reporting every change merged into the base branch since as its own.

GITHUB_REF is set by the runner from the ref the run was triggered on, so on
a pull_request event it names the merge ref and cannot go stale. Together
with the existing check that the build runs on GITHUB_SHA it identifies the
test merge without the payload; the head.sha comparison stays as the
fallback when the merge ref is absent.

The parents.length check is split out so a non-merge HEAD is rejected before
either identification path runs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@gregberge
gregberge merged commit a34cf2e into main Sep 4, 2026
70 checks passed
@gregberge
gregberge deleted the fix/test-merge-ref branch September 4, 2026 14:10
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.

2 participants