feat(core): resolve the base commit from the GitHub API by gregberge · Pull Request #376 · argos-ci/argos-javascript

gregberge · 2026-09-07T18:21:42Z

Description

Stacked on #375 — review that one first; this PR's diff is the second commit.

Without a Git provider connected, the base commit is worked out from the local repository: read the test-merge commit's parents, else git merge-base. That makes the answer depend on the shape the checkout happens to have — how shallow the clone is, whether the graft hides the parents, whether the merge ref still resolves — and every one of those has to go right for the baseline to be right. Chasing an "older baseline" report, I proposed three different local-git failure modes and disproved all three on real runners, without ever identifying the cause.

A project that granted content access has none of these problems, because the server asks GitHub. On GitHub Actions the CLI can ask the same question, of the same compare endpoint, with the token the workflow already has.

The token is the workflow's own, so nothing third-party gains access to the repository — which is the reason a project picks the light app in the first place.

Asked about the checkout, not about `GITHUB_SHA`

That is what makes one query correct for both shapes of pull request build, with no guard telling them apart. Verified against the API:

`compare/main...<head>`	returns	correct because
test-merge commit	`78fe66e7` — the base tip merged in, though `main` had moved to `8a2b41bb`	the screenshots contain the base branch up to that tip
pull request head	`e5ba9c18` — the fork point	those screenshots contain no base branch changes

getTestMergeBaseCommitSha's guard ladder — event name, payload present, base-ref match, HEAD === GITHUB_SHA, two parents, merge-ref match — exists to distinguish those two. One request does it from the commit graph instead.

The server does the walk

resolveBaseline() now sends the base commit with the ancestors the server searches, instead of picking a commit itself and sending parentCommits: null. Picking here freezes the baseline to whichever builds happen to be complete at that instant, and the server stops at the reference commit as soon as it has a bucket for it — so without the ancestors it has nothing to fall back on. On the project investigated, 0 of 4379 builds carried parentCommits, so the server could never walk. This makes the path resolve the way the content-access path already does.

Verified on a real runner

argos-ci/argos-test-repository#22, on the fetch-depth: 1 checkout CI actually has, against a base tip read from a different endpoint:

expected (parents[0] of github.sha):  8a2b41bb

with a token:     Fetching the merge base of main...13e716f1 from the GitHub API
                  Merge base from the GitHub API 8a2b41bb              → PASS
without a token:  No GitHub token, falling back to git to find the merge base
                  Found base commit from the test-merge commit 8a2b41bb → PASS

The fallback is intact and silent. Unlike the pull request lookup, this path does not nag about a missing token (notifyWithoutToken: false) — it has somewhere to fall back to, so a working build stays quiet.

Type of changes

feature

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

Two bugs the new tests caught while being written

GITHUB_API_URL on GitHub Enterprise. new URL("/repos/…", "https://github.acme.com/api/v3") silently drops /api/v3 — an absolute path replaces the base path. Making the base URL configurable would have broken all three endpoints in the file. Now joined by hand, with a test pinning it.
The cache key was wrong. Keyed on base...head alone, it leaked across eight tests that all returned one constant. The answer also depends on the repository and the resolved HEAD; keyed on all four now.

Decisions worth a second opinion

The ancestor list still comes from git, not the API. commits?sha= caps at 100 per page, so matching the git path's reach (up to MAX_COMMITS) means paginating and spending rate limit, and the merge base is the part that was ever in doubt. Easy to add if you want full parity.
The token is not harvested from .git/config. actions/checkout leaves it there as an extraheader with persist-credentials: true (the default), so it is recoverable — but taking a credential the user did not hand to Argos is your call, not mine. The consequence is that this only helps projects that set GITHUB_TOKEN, which the CLI already asks for but not universally.
Fork pull requests are unvalidated. The token is read-only and scoped to the base repo; the test-merge commit lives there, so compare should resolve. This probe runs on a same-repo branch, so it does not exercise it. A 403/404 falls through to git, so nothing regresses.
getHeadSha() runs before the cache lookup, since the resolved HEAD is part of the key — one local git rev-parse HEAD per upload() even on a hit.
This does not settle the original report. If a59619df and 400f4496 are on parallel lines, a correct merge base does not change which commits have a build for a given build name, and the walk still lands where it did — but it would then be provably correct rather than unexplained.

🤖 Generated with Claude Code

vercel · 2026-09-07T18:21:48Z

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

Project	Deployment	Actions	Updated
argos-js-sdk-reference	Ready	Preview	Sep 7, 2026 6:48pm UTC

Without a Git provider connected, the base commit was worked out from the local repository: read the test-merge commit's parents, or fall back to git merge-base. That makes the answer depend on the shape the checkout happens to have - how shallow the clone is, whether the graft hides the parents, whether the merge ref still resolves - and every one of those has to be got right for the baseline to be right. A project that granted content access has none of these problems, because the server asks GitHub. On GitHub Actions the CLI can ask the same question, of the same endpoint, with the token the workflow already has: one "compare" call returns the commit of the base branch the build content is derived from. It is asked about the commit checked out, not about GITHUB_SHA, and that is what makes a single query right for both shapes of pull request build - the test-merge commit answers with the base branch tip merged in, even once the base branch has moved past it, and a pull request head checkout answers with the fork point, which is the commit those screenshots really are derived from. No guard has to tell the two apart. The token is the workflow's own, so nothing third-party gains access to the repository - which is the reason a project picks the light app in the first place. Without a token, or if the API cannot answer, the git path runs exactly as before, and unlike the pull request lookup this one keeps quiet about it: it has somewhere to fall back to. The result is cached for the process, keyed on the repository, the refs and the checked out commit. upload() runs once per build name, so a project with a build name per browser would otherwise pay for the query, and for the fetches behind the git fallback, once per name. resolveBaseline() now hands the server the base commit with the ancestors it searches, instead of picking a commit itself and sending parentCommits: null. Picking here freezes the baseline to the builds that happen to be complete at that instant, and the server stops at the reference commit as soon as it has a bucket for it - so without the ancestors it has nothing to fall back on. Sending them makes this path resolve the way the content-access path already does. GITHUB_API_URL is honoured too, so this works on GitHub Enterprise. The base URL is joined by hand: passing it to the URL constructor as a base drops the path an appliance is mounted under. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The server picks the baseline by walking the candidate commits in order, so that list has to be complete: a commit missing from it is a baseline that cannot be chosen. It came from the local repository - a shallow fetch, then git log - and a shallow clone holds whatever history the fetch happened to bring. That is not hypothetical. On the reported project, three unrelated pull requests were baselined against the same commit 85 minutes back, and all three reported the same six screenshots as changed - a change none of them made. A nearer commit was an ancestor of their merge base and had a complete, eligible build: nine builds that did use it report none of those six diffs. It was simply absent from the list the candidates were picked from. So list them the way the server does for a project that granted content access, from the same endpoint. Completeness is the whole point; ordering and depth were never the problem. Paging is capped, because a search that finds nothing must not spend the hourly request budget, and the result is cached for the process: upload() runs once per build name and asks for the same listing every time. Without a token, or if the API cannot answer, git lists them exactly as before - and a partial answer is kept when a later page fails, since the pages already read are a correct prefix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

gregberge · 2026-09-07T20:05:41Z

The bug this was chasing is server-side, and it's now fixed there: argos-ci/argos#2586.

The candidate list the CLI sends to /baseline was never at fault. The server ranks it through a VALUES join binding each commit with its position — and a bound parameter reaches Postgres untyped, so the position column resolves as text: 0, 1, 10, 11, 2, 20. A match at position 2 loses to any match at 10–19. That is exactly the production pattern: the two kits whose match sat at position 0/1 were right, the other 21 got a commit 85 minutes older. Replayed on the real builds, casting the rank to int flips the answer to the nearer, compatible baseline.

So a GITHUB_TOKEN is not required to fix the report, which changes what this PR is for:

528beb29 (merge base from the API) still stands on its own merits — optional, silent fallback, removes real local-git fragility — but it's a hardening, not a fix.
ec40a50c (candidate list from the API) was justified only by the theory that the list was incomplete. It wasn't. I'd drop it.

I'll leave the call to @gregberge rather than close it myself.

gregberge · 2026-09-07T20:08:21Z

Closed: the report it targeted was a server-side ranking bug, fixed in argos-ci/argos#2586. Nothing here is needed for it, and it would have made a GITHUB_TOKEN look required.

gregberge requested a review from jsfez September 7, 2026 18:23

gregberge force-pushed the feat/merge-base-from-github-api branch from 15014a2 to 528beb2 Compare September 7, 2026 18:28

vercel Bot deployed to Preview September 7, 2026 18:30 View deployment

vercel Bot deployed to Preview September 7, 2026 18:48 View deployment

jsfez approved these changes Sep 7, 2026

View reviewed changes

gregberge closed this Sep 7, 2026

gregberge deleted the feat/merge-base-from-github-api branch September 7, 2026 20:22

Navigation Menu

feat(core): resolve the base commit from the GitHub API - #376

feat(core): resolve the base commit from the GitHub API#376
gregberge wants to merge 2 commits into
fix/test-merge-parents-on-shallow-clonefrom
feat/merge-base-from-github-api

gregberge commented Sep 7, 2026

Uh oh!

vercel Bot commented Sep 7, 2026 •

edited

Loading

Uh oh!

gregberge commented Sep 7, 2026

Uh oh!

gregberge commented Sep 7, 2026

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

gregberge commented Sep 7, 2026

Description

Asked about the checkout, not about GITHUB_SHA

The server does the walk

Verified on a real runner

Type of changes

Checklist

Further comments

Two bugs the new tests caught while being written

Decisions worth a second opinion

Uh oh!

vercel Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregberge commented Sep 7, 2026

Uh oh!

gregberge commented Sep 7, 2026

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Asked about the checkout, not about `GITHUB_SHA`

vercel Bot commented Sep 7, 2026 •

edited

Loading