Skip to content

ci: test pull requests against the merge ref, not the stale head - #272

Merged
vpavlin merged 1 commit into
mainfrom
ci/test-pr-against-merge-ref
Sep 9, 2026
Merged

ci: test pull requests against the merge ref, not the stale head#272
vpavlin merged 1 commit into
mainfrom
ci/test-pr-against-merge-ref

Conversation

@vpavlin

@vpavlin vpavlin commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

The symptom

#266 is failing CI on a file it could not possibly have:

scripts/multisig-e2e-test.sh: No such file or directory
Process completed with exit code 127

That script arrived in #267, which merged after #266 branched. The PR's own change — Vec argument parsing — is unrelated and its unit and E2E jobs pass.

The cause

Five sequencer-backed jobs override the checkout:

- uses: actions/checkout@v4
  with:
    ref: ${{ github.event.pull_request.head.sha || github.sha }}

On a pull_request event the workflow definition comes from main, so the job list is current and knows about the new test. The working tree is the PR's branch tip, which predates it. unit-tests and e2e-tests never had the override and check out the merge ref, which is exactly why they went green on the same commit — making this look like the contributor's fault when it isn't.

This is not specific to #266. Nine other open PRs (#249, #248, #227, #213, #212, #174, #137, #86, #84) lack that script at their head today and would fail identically. It recurs every time a test script is added, and no contributor can prevent it.

The quieter half: those jobs test each PR against whatever base it branched from, so a PR can go green while being broken against current main.

The change

Checkout — drop the override. actions/checkout with no ref: already resolves to the merge ref on pull_request and the pushed commit on push, which is what the || expression was hand-rolling, minus the staleness.

SPEL_REF — moved from /head to /merge in the same four jobs. This one is load-bearing rather than cosmetic: it flows to spel init --spel-rev and lands in the scaffolded guest manifest as

spel-framework = { git = "...", rev = "refs/pull/N/head" }

so it decides which framework the guest binary compiles against. Left at /head while the checkout moved to /merge, a run would take its scripts from the merged tree and its framework from the stale head. The two halves disagreeing is worse than both being stale, so they move together.

Cargo resolves the merge ref — checked before proposing it:

source = "git+https://github.com/logos-co/spel.git?rev=refs%2Fpull%2F266%2Fmerge#bfa407f851ccb6..."

bfa407f8 is the commit git ls-remote https://github.com/logos-co/spel.git refs/pull/266/merge reports.

One deliberate behaviour change

refs/pull/N/merge exists only while a PR is conflict-free. A conflicted PR will now fail to resolve the ref instead of testing against a base it no longer merges into.

I think that is right — a green check on a conflicted PR describes a tree nobody will ever ship — but it is a change rather than a fix, and worth agreeing to explicitly rather than meeting in the wild. Happy to keep the old fallback for that case if you would rather it degrade quietly.

Verification

The proof is this PR's own checks: it exercises the changed workflow. Beyond that I would want one green run on #266 (which needs this merged first, since workflow changes on a PR branch do not apply to other PRs).

Nothing here changes what is tested, only which tree it is tested on — the PR's code, merged with current main, consistently on both the script and framework sides.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S9qsH6Um6shweCPEN3Z6ph

Five sequencer-backed jobs checked out
`github.event.pull_request.head.sha`, so they ran the PR's branch tip
while the workflow definition came from main. A PR opened before a new
test script lands therefore fails on a file it never could have had:

    scripts/multisig-e2e-test.sh: No such file or directory
    Process completed with exit code 127

That is #266's current red check, and every other open PR is one
commit-add away from the same thing — nine of them lack that script at
their head right now. The unit and E2E jobs never had the override, so
they went green on the same commit, which is what makes the failure
look like the contributor's fault.

The quieter half of the problem is that those jobs tested each PR
against whatever base it branched from, so a PR could pass while being
broken against current main.

`actions/checkout` with no `ref:` already does the right thing: the
merge ref on pull_request, the pushed commit on push — which is what
the `||` expression was hand-rolling, minus the staleness.

SPEL_REF has to move with it. It reaches `spel init --spel-rev` and
lands in the scaffolded guest manifest as

    spel-framework = { git = "...", rev = "refs/pull/N/head" }

so it decides which framework the guest compiles against. Left at
/head while the checkout moved to /merge, a run would take its scripts
from the merged tree and its framework from the stale head — the two
halves disagreeing is worse than both being stale. Cargo resolves the
merge ref fine; verified it locks to the same commit `git ls-remote`
reports for refs/pull/N/merge.

One deliberate behaviour change: refs/pull/N/merge exists only while a
PR is conflict-free, so a conflicted PR now fails to resolve the ref
instead of testing against a base it no longer merges into. That seems
right — a green check on a conflicted PR is describing a tree nobody
will ever ship — but it is a change, not a fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S9qsH6Um6shweCPEN3Z6ph
@vpavlin
vpavlin merged commit 4ccb1cf into main Sep 9, 2026
10 checks passed
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