From 6d554d5f9c722fd4cc010226de3a11f0d97b3429 Mon Sep 17 00:00:00 2001 From: Mike Pitre <12040919+mikepitre@users.noreply.github.com> Date: Tue, 13 Oct 2026 20:18:18 -0400 Subject: [PATCH 1/2] test(expo): publish for a session that started on a commit of the base branch The verify-attach workflow published only when the commit a session started on was a commit of the pull request. A session that comes up on the commit the branch came from, so that it can make the before run, started on a commit of the base branch. Its evidence was refused, and the only way out was a second session on the pushed change. The workflow now also takes a start commit that the base branch of the pull request holds. It asks GitHub to compare that commit with the base of the pull request, and takes only `ahead` or `identical` as a yes. The commit of the run must still be a commit of the pull request, and every other check is unchanged. `attach` checks the same rule before it hands off. The workflow file and the session agent of such a session come from the base branch, so the change trusts no code that the old rule did not. A session that started on a commit of neither the pull request nor its base branch is still refused. One session now does the before run and the run of the change, so the skill gives one order of work and says to open the pull request after the last run. The script, the two files under src/core/remote/ and the test files are the bytes of clerk-ios. Co-Authored-By: Claude Opus 5.5 --- .../expo-verify-session-start-on-base.md | 2 + .claude/skills/verify-clerk-expo/SKILL.md | 6 +- .../verify-clerk-expo/references/remote.md | 4 +- integration/expo-native/src/core/MANIFEST | 4 +- .../expo-native/src/core/remote/backend.ts | 13 ++- .../expo-native/src/core/remote/handoff.ts | 25 +++-- integration/expo-native/test/handoff.test.ts | 89 +++++++++++---- scripts/verify-attach.mjs | 62 +++++++++-- scripts/verify-attach.test.mjs | 104 ++++++++++++++++-- 9 files changed, 251 insertions(+), 58 deletions(-) create mode 100644 .changeset/expo-verify-session-start-on-base.md diff --git a/.changeset/expo-verify-session-start-on-base.md b/.changeset/expo-verify-session-start-on-base.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/expo-verify-session-start-on-base.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/.claude/skills/verify-clerk-expo/SKILL.md b/.claude/skills/verify-clerk-expo/SKILL.md index 39121b1c946..4c31cd67ff0 100644 --- a/.claude/skills/verify-clerk-expo/SKILL.md +++ b/.claude/skills/verify-clerk-expo/SKILL.md @@ -219,15 +219,13 @@ The first `attach` of a platform adds its block after the description. A later ` On a machine whose `gh` can attach, `attach` edits the description itself and prints `posted`. That needs gh 2.99.0 or newer, whose `gh pr edit` has `--attach`. It reads the description first. When the description already holds the block that this `attach` would write, it uploads nothing and prints `already posted`. When the block there is of another run or another choice of files, or there is none, it uploads the files and writes the block. -`attach` always tries `gh pr edit --attach` itself first, on every kind of machine, and hands off only when that cannot work. On a machine whose `gh` cannot attach, such as a cloud sandbox, it hands the files of a remote run to the session's runner and prints `handed off`. With `--before`, the files of the before run go with them, from that run's directory in this checkout. Run it before `down`. The evidence reaches the description after `down` ends the session, when the `verify-attach` workflow publishes it, in the same tables. Its line says that the session reported the result, links the session's run, and names the account that started the session and the commit the run was made at. With `--before`, the line says that the session reports the before run, and it links that run's commit. That workflow publishes only to the open pull request of the session's branch, and only when the commit the session started on and the commit of the run are both commits of that pull request. The commit of the before run must be the commit of the run or a commit before it in its history, such as the one the branch started from. So open the pull request before `up`. A later push does not lose the evidence: the line then says that the pull request has newer commits. To show the newer commit, `run` and `attach` again, which replaces the block. `attach` checks the pull request first, and fails with the reason when the workflow would refuse it. Each platform has its own session, so `attach` the run of each platform before the `down` that ends its session. +`attach` always tries `gh pr edit --attach` itself first, on every kind of machine, and hands off only when that cannot work. On a machine whose `gh` cannot attach, such as a cloud sandbox, it hands the files of a remote run to the session's runner and prints `handed off`. With `--before`, the files of the before run go with them, from that run's directory in this checkout. Run it before `down`. The evidence reaches the description after `down` ends the session, when the `verify-attach` workflow publishes it, in the same tables. Its line says that the session reported the result, links the session's run, and names the account that started the session and the commit the run was made at. With `--before`, the line says that the session reports the before run, and it links that run's commit. That workflow publishes only to the open pull request of the session's branch, and only when the commit of the run is a commit of that pull request. The commit the session started on must be one too, or a commit that the base branch holds, such as the one the branch came from. The commit of the before run must be the commit of the run or a commit before it in its history. A later push does not lose the evidence: the line then says that the pull request has newer commits. To show the newer commit, `run` and `attach` again, which replaces the block. `attach` checks the pull request first, and fails with the reason when the workflow would refuse it. Each platform has its own session, so `attach` the run of each platform before the `down` that ends its session. When this machine cannot attach and there is no session to hand the files to, or the hand-off fails, `attach` fails and says which. Then name the run id in the PR and say that the evidence was not attached. A run on a local device has no session, so its fix is a newer `gh`. [Remote devices](references/remote.md) has what the repository needs before a hand-off can be published, and its limits. `attach` writes only to an open pull request, and fails on one that is closed or merged. It refuses a run that is tainted, that has a failing spec or no passing one, or whose `app.log` names a user that the run did not create. A before run can have failing specs. `attach` refuses a before run that is tainted, that was never sealed, or whose `app.log` names a user that the run did not create. -On a local device, work in this order: `run` the spec on the build without your change when the change is to something that exists, make the change, commit, `run` the spec, `git push`, open the pull request, `attach`, then `down`. Commit before the second `run`, so that the line `attach` writes names a commit the pull request holds. In the pull request, name the run id and the specs you ran, and say what you did not run. - -On a remote session, make the before run before you push the change. A session builds the commit it was started on and later commits of the branch, and never an earlier one. When the branch has a pushed commit that does not hold the change, such as the commit that adds the spec, one session does both runs: open the pull request, `up`, `run` the spec, push the change, `run` the spec again, `attach` with `--before`, then `down`. When the change is the only commit, it takes two sessions, because the workflow publishes only for a session that started on a commit of the pull request: push the branch, `up`, `run` the spec, `down`, push the change, open the pull request, `up`, `run` the spec again, `attach` with `--before`, then `down`. +Work in this order, with one `up` for both runs: make the branch and `git push` it, `up`, `run` the spec on the build without your change when the change is to something that exists, make the change, commit, `git push`, `run` the spec again, open the pull request, `attach`, then `down`. Open the pull request after the last run, so that it is not left open without its evidence. On a local device `attach` writes the description itself. On a remote session the evidence reaches the description about a minute after `down`. A remote session builds the commit it was started on and later commits of the branch, and never an earlier one, so there the before run comes before the push of the change. Commit before the second `run`, so that the line `attach` writes names a commit the pull request holds. In the pull request, name the run id and the specs you ran, and say what you did not run. Attach the run of your own change. Run your new or changed spec on its own and attach that run, so the screenshots or the video show only what the change is about. You do not owe a regression run: PR CI (`.github/workflows/verify-e2e.yml`) runs every golden spec on the pull request and reports them there. If you ran other golden specs anyway, cite that run's id in the PR and leave its video in `.verify/runs/`. diff --git a/.claude/skills/verify-clerk-expo/references/remote.md b/.claude/skills/verify-clerk-expo/references/remote.md index bcab9ce7e3b..4bc99aee84a 100644 --- a/.claude/skills/verify-clerk-expo/references/remote.md +++ b/.claude/skills/verify-clerk-expo/references/remote.md @@ -97,7 +97,7 @@ The workflow publishes only when all of these hold. Otherwise it prints the reas - The pull request that `--pr` names is open, and its branch is in this repository, not in a fork. - Its branch is the branch the session was started on. - The commit the run was made at is one of the pull request's commits. -- The commit that branch had when `up` started the session is one of them too. GitHub lists the first 250 commits of a pull request, and the workflow reads no further. When a longer pull request has either commit past the first 250, the workflow says so and publishes nothing. Squash or rebase the branch to 250 commits or fewer, or attach from a machine whose `gh` can attach. +- The commit that branch had when `up` started the session is one of them too, or a commit that the base branch of the pull request holds, such as the commit the branch came from. The workflow asks GitHub to compare that commit with the base of the pull request. GitHub lists the first 250 commits of a pull request, and the workflow reads no further. When a longer pull request has either commit past the first 250, the workflow says so and publishes nothing. Squash or rebase the branch to 250 commits or fewer, or attach from a machine whose `gh` can attach. - With `--before`, the commit of the before run is the commit of the run or a commit before it in its history. The workflow asks GitHub to compare the two commits. - The hand-off has at least one file. - The description has the two evidence comments of the platform once each, or neither. @@ -105,6 +105,8 @@ The workflow publishes only when all of these hold. Otherwise it prints the reas `attach` checks the same rules before it sends anything, and fails with the reason. +So one session does the before run and the run of the change: push the branch, `up`, `run`, push the change, `run` again, open the pull request, `attach`, `down`. Open the pull request after the last run, so that it is not left open without its evidence, which reaches the description about a minute after `down`. + A before run is the build without the change, so its commit is often the one the branch started from, which is no commit of the pull request. The workflow therefore does not look for that commit among the commits of the pull request. It asks GitHub whether that commit is the commit of the run or a commit before it in its history, and it publishes nothing when the answer is no. After a rebase that rewrote the commit of the before run, `run` the spec again on a commit that the branch still has, and pass that run as `--before`. The workflow cannot check that the files of the before run came from a build of that commit, which is why the line says that the session reports the before run. The manifest of a hand-off has a version, and the workflow publishes only the version of the script on the default branch. A branch whose verify code is older than the default branch's hands off a manifest that the workflow refuses. The refusal names both versions in the run of `verify-attach`, and nothing on the pull request shows it. Merge the default branch into the branch before you `attach`, then `down`, `up`, `run`, and `attach`. A session takes only the version of its own code. When the checkout changed version after `up`, `attach` fails, names both versions, and says to `down` and `up`. diff --git a/integration/expo-native/src/core/MANIFEST b/integration/expo-native/src/core/MANIFEST index 21de99791be..22b2604ad27 100644 --- a/integration/expo-native/src/core/MANIFEST +++ b/integration/expo-native/src/core/MANIFEST @@ -35,9 +35,9 @@ cf6aa68be73e4abaf3b63d5d84c2e9eee6197412f607ec53d0f05f7b058db420 src/core/insta a0ee9e4bfd34d4659685108e5e54eb1ec2aff6248236c358e6d3b14b2e2ff6a0 src/core/ledgers.ts afab94a970286c40d6857b03ae4fa39ec59786274beacf6f1e77f6530bf46c54 src/core/manifest.ts 7294bbab3d892f0d9aee1e863f84e32a7f5dc3c467a7b6177be7e4ff2f29fecc src/core/publish.ts -76de0f98013b8a5d011dee8cf51cce63fcb3d7e68acfee51945fa5cecf2e7763 src/core/remote/backend.ts +8302ab57092212ec057f357c38bffec97101ff23c546a76941d898c1612d58b0 src/core/remote/backend.ts 37fc42707be7d8bd5f0313a664a8465287ebb0970c520c0e0617f6ff6c9a614e src/core/remote/github.ts -cbd837689f2e0eff48205709a8c99508a0bcd0bea76f2647a93faa8866a76ae0 src/core/remote/handoff.ts +1557283a67138c35cb896c2eb1c821be06dc786b8a6aa93a3c9f23f447689f38 src/core/remote/handoff.ts 2ec436b2c53b34269e0b62ab54e41e4d9a55146cccc46f0f31591db05cdbf5c1 src/core/remote/preflight.ts bb69fe1f3581942285894c0901d02889c08b4293e4d9db77934fe3cdd2d4a399 src/core/remote/protocol.ts 5a4ee0e7ed20feaeb711b1527083299b432d3fc0b7f428b636678a8a2f57581f src/core/remote/recipe.ts diff --git a/integration/expo-native/src/core/remote/backend.ts b/integration/expo-native/src/core/remote/backend.ts index 23a839c923f..e989a272057 100644 --- a/integration/expo-native/src/core/remote/backend.ts +++ b/integration/expo-native/src/core/remote/backend.ts @@ -31,6 +31,14 @@ async function pullRequestCommits(hub: GitHub, pr: number): Promise { + if (typeof base !== 'string' || !/^[0-9a-f]{40}$/.test(base)) return false; + const compared = await hub.api('GET', `/compare/${commit}...${base}?per_page=1`); + if (compared.status !== 200) throw new VerifyFailure('NOT_READY', `could not compare the commit the session started on with the base branch of PR #${pr}: ${apiMessage(compared)}`, 'retry {cli} attach'); + const status = (compared.json as { readonly status?: unknown } | null)?.status; + return status === 'ahead' || status === 'identical'; +} + export function remoteBackend(settings: RemoteSettings, deps: RemoteDeps = { env: process.env, runner: run }): DeviceBackend { const { platform } = settings; let opened: Promise | undefined; @@ -245,10 +253,11 @@ export function remoteBackend(settings: RemoteSettings, deps: RemoteDeps = { env if (session.status !== 200) throw new VerifyFailure('NOT_READY', `could not read run ${lease.providerRef}: ${apiMessage(session)}`, `open ${runUrl(lease.providerRef)}`); if (pull.status !== 200) throw new VerifyFailure('NOT_READY', `could not read PR #${bundle.pr} of ${settings.repo}: ${apiMessage(pull)}`, 'pass the number of an open pull request of this branch as --pr'); const started = session.json as { readonly head_branch: string; readonly head_sha: string }; - const pr = pull.json as { readonly state: string; readonly body: string | null; readonly commits: number; readonly head: { readonly ref: string; readonly sha: string; readonly repo: { readonly full_name: string } | null } }; - const mismatch = pullRequestMismatch( + const pr = pull.json as { readonly state: string; readonly body: string | null; readonly commits: number; readonly base?: { readonly sha?: unknown }; readonly head: { readonly ref: string; readonly sha: string; readonly repo: { readonly full_name: string } | null } }; + const mismatch = await pullRequestMismatch( { state: pr.state, headRepo: pr.head.repo?.full_name ?? null, headRef: pr.head.ref, commits: await pullRequestCommits(hub, bundle.pr), commitCount: pr.commits }, { repo: settings.repo, headBranch: started.head_branch, startedOn: started.head_sha, runCommit: bundle.summary.commit }, + () => baseHolds(hub, bundle.pr, pr.base?.sha, started.head_sha), ); if (mismatch !== null) throw wouldNot(mismatch.why, mismatch.fix); if (bundle.before !== null) { diff --git a/integration/expo-native/src/core/remote/handoff.ts b/integration/expo-native/src/core/remote/handoff.ts index b923a6dd128..57d34791c94 100644 --- a/integration/expo-native/src/core/remote/handoff.ts +++ b/integration/expo-native/src/core/remote/handoff.ts @@ -187,22 +187,29 @@ export interface PullRequestMismatch { } const ITS_OWN_PULL_REQUEST = - "it accepts only the open pull request of the session's branch, and only when the commit the session started on and the commit of the run are both commits of that pull request; git push, then {cli} run again and attach, or after a rewrite of the branch's history {cli} down and {cli} up first"; + "it accepts only the open pull request of the session's branch, and only when the commit of the run is a commit of that pull request and the commit the session started on is one too or is a commit of its base branch; git push, then {cli} run again and attach, or after a rewrite of the branch's history {cli} down and {cli} up first"; const FEWER_COMMITS = `it can check a commit only against the first ${MOST_COMMITS_GITHUB_LISTS} commits of a pull request; squash or rebase the branch to ${MOST_COMMITS_GITHUB_LISTS} commits or fewer, git push, then {cli} down, {cli} up, run again and attach, or run {cli} attach on a machine whose gh can attach`; -export function pullRequestMismatch(pull: PullRequestView, session: { readonly repo: string; readonly headBranch: string; readonly startedOn: string; readonly runCommit: string }): PullRequestMismatch | null { +export async function pullRequestMismatch( + pull: PullRequestView, + session: { readonly repo: string; readonly headBranch: string; readonly startedOn: string; readonly runCommit: string }, + baseHoldsStart: () => Promise, +): Promise { const refused = (why: string): PullRequestMismatch => ({ why, fix: ITS_OWN_PULL_REQUEST }); if (pull.state !== 'open') return refused(`it is ${pull.state}`); if (pull.headRepo !== session.repo) return refused(`its branch is in ${pull.headRepo ?? 'a deleted fork'}, not in ${session.repo}`); if (pull.headRef !== session.headBranch) return refused(`its branch is ${pull.headRef} and the session runs on ${session.headBranch}`); - for (const [what, commit] of [['the session started on', session.startedOn], ['the run was made at', session.runCommit]] as const) { - if (pull.commits.includes(commit)) continue; - if (pull.commitCount > MOST_COMMITS_GITHUB_LISTS) { - return { why: `it has ${pull.commitCount} commits, GitHub lists only the first ${MOST_COMMITS_GITHUB_LISTS}, and ${what} ${commit.slice(0, 12)}, which is not among those`, fix: FEWER_COMMITS }; - } - return refused(`${what} ${commit.slice(0, 12)}, which is not one of its commits`); + const notHeld = + !pull.commits.includes(session.startedOn) && !(await baseHoldsStart()) + ? { commit: `the session started on ${session.startedOn.slice(0, 12)}`, among: 'one of its commits or a commit of its base branch' } + : !pull.commits.includes(session.runCommit) + ? { commit: `the run was made at ${session.runCommit.slice(0, 12)}`, among: 'one of its commits' } + : null; + if (notHeld === null) return null; + if (pull.commitCount > MOST_COMMITS_GITHUB_LISTS) { + return { why: `it has ${pull.commitCount} commits, GitHub lists only the first ${MOST_COMMITS_GITHUB_LISTS}, and ${notHeld.commit}, which is not among those`, fix: FEWER_COMMITS }; } - return null; + return refused(`${notHeld.commit}, which is not ${notHeld.among}`); } const sha256File = (path: string): string => createHash('sha256').update(readFileSync(path)).digest('hex'); diff --git a/integration/expo-native/test/handoff.test.ts b/integration/expo-native/test/handoff.test.ts index 310667604c1..32777fd1b86 100644 --- a/integration/expo-native/test/handoff.test.ts +++ b/integration/expo-native/test/handoff.test.ts @@ -168,36 +168,50 @@ describe('the rule for which pull request a session may publish to', () => { const STARTED_ON = 'd'.repeat(40); const session = { repo: 'clerk/clerk-ios', headBranch: 'feature/change', startedOn: STARTED_ON, runCommit: COMMIT }; const pull = { state: 'open', headRepo: 'clerk/clerk-ios', headRef: 'feature/change', commits: [STARTED_ON, COMMIT], commitCount: 2 }; - const why = (...asked: Parameters): string | undefined => pullRequestMismatch(...asked)?.why; + const notOnBase = async (): Promise => false; + const onBase = async (): Promise => true; + const neverAsked = async (): Promise => assert.fail('the base branch was asked about a commit of the pull request'); + const why = async (...asked: Parameters): Promise => (await pullRequestMismatch(...asked))?.why; - it('accepts the open pull request of the session branch that holds the commit the session started on and the commit of the run', () => { - assert.equal(pullRequestMismatch(pull, session), null); - assert.equal(pullRequestMismatch({ ...pull, commits: [COMMIT] }, { ...session, startedOn: COMMIT }), null, 'a run at the commit the session started on'); + it('accepts the open pull request of the session branch that holds the commit the session started on and the commit of the run', async () => { + assert.equal(await pullRequestMismatch(pull, session, neverAsked), null); + assert.equal(await pullRequestMismatch({ ...pull, commits: [COMMIT] }, { ...session, startedOn: COMMIT }, neverAsked), null, 'a run at the commit the session started on'); }); - it('accepts a pull request that has newer commits than the run', () => { - assert.equal(pullRequestMismatch({ ...pull, commits: [STARTED_ON, COMMIT, 'b'.repeat(40)] }, session), null); + it('accepts a session that started on a commit the base branch holds, when the commit of the run is a commit of the pull request', async () => { + assert.equal(await pullRequestMismatch({ ...pull, commits: [COMMIT] }, session, onBase), null); + assert.equal(await why({ ...pull, commits: [COMMIT] }, session, notOnBase), 'the session started on dddddddddddd, which is not one of its commits or a commit of its base branch'); }); - it('refuses a closed one, a fork, another branch, and one that lacks either commit, and says which', () => { - assert.equal(why({ ...pull, state: 'closed' }, session), 'it is closed'); - assert.equal(why({ ...pull, headRepo: 'someone/clerk-ios' }, session), 'its branch is in someone/clerk-ios, not in clerk/clerk-ios'); - assert.equal(why({ ...pull, headRepo: null }, session), 'its branch is in a deleted fork, not in clerk/clerk-ios'); - assert.equal(why({ ...pull, headRef: 'main' }, session), 'its branch is main and the session runs on feature/change'); - assert.equal(why({ ...pull, commits: [COMMIT] }, session), 'the session started on dddddddddddd, which is not one of its commits'); - assert.equal(why({ ...pull, commits: [STARTED_ON] }, session), 'the run was made at 0f50b597b1c2, which is not one of its commits'); + it('refuses a run made at a commit that only the base branch holds, whether or not the session started there', async () => { + assert.equal(await why({ ...pull, commits: [STARTED_ON] }, session, onBase), 'the run was made at 0f50b597b1c2, which is not one of its commits'); + assert.equal(await why({ ...pull, commits: ['e'.repeat(40)] }, { ...session, startedOn: COMMIT }, onBase), 'the run was made at 0f50b597b1c2, which is not one of its commits'); }); - it('says that GitHub lists only the first 250 commits when a longer pull request does not show a commit, and gives a fix that can change that', () => { + it('accepts a pull request that has newer commits than the run', async () => { + assert.equal(await pullRequestMismatch({ ...pull, commits: [STARTED_ON, COMMIT, 'b'.repeat(40)] }, session, neverAsked), null); + }); + + it('refuses a closed one, a fork, another branch, and one that lacks either commit, and says which', async () => { + assert.equal(await why({ ...pull, state: 'closed' }, session, neverAsked), 'it is closed'); + assert.equal(await why({ ...pull, headRepo: 'someone/clerk-ios' }, session, neverAsked), 'its branch is in someone/clerk-ios, not in clerk/clerk-ios'); + assert.equal(await why({ ...pull, headRepo: null }, session, neverAsked), 'its branch is in a deleted fork, not in clerk/clerk-ios'); + assert.equal(await why({ ...pull, headRef: 'main' }, session, neverAsked), 'its branch is main and the session runs on feature/change'); + assert.equal(await why({ ...pull, commits: [COMMIT] }, session, notOnBase), 'the session started on dddddddddddd, which is not one of its commits or a commit of its base branch'); + assert.equal(await why({ ...pull, commits: [STARTED_ON] }, session, neverAsked), 'the run was made at 0f50b597b1c2, which is not one of its commits'); + }); + + it('says that GitHub lists only the first 250 commits when a longer pull request does not show a commit, and gives a fix that can change that', async () => { const listed = Array.from({ length: 250 }, (_, i) => i.toString(16).padStart(40, '0')); const long = { ...pull, commits: listed, commitCount: 300 }; - const refused = pullRequestMismatch(long, session); + const refused = await pullRequestMismatch(long, session, notOnBase); assert.equal(refused?.why, 'it has 300 commits, GitHub lists only the first 250, and the session started on dddddddddddd, which is not among those'); assert.match(refused?.fix ?? '', /squash or rebase the branch to 250 commits or fewer, .* or run \{cli\} attach on a machine whose gh can attach$/); - assert.equal(why({ ...long, commits: [STARTED_ON, ...listed.slice(1)] }, session), 'it has 300 commits, GitHub lists only the first 250, and the run was made at 0f50b597b1c2, which is not among those'); - assert.equal(pullRequestMismatch({ ...long, commits: [STARTED_ON, COMMIT, ...listed.slice(2)] }, session), null, 'both commits are among the first 250'); - const short = pullRequestMismatch({ ...long, commitCount: 250 }, session); - assert.equal(short?.why, 'the session started on dddddddddddd, which is not one of its commits'); + assert.equal(await why({ ...long, commits: [STARTED_ON, ...listed.slice(1)] }, session, neverAsked), 'it has 300 commits, GitHub lists only the first 250, and the run was made at 0f50b597b1c2, which is not among those'); + assert.equal(await pullRequestMismatch({ ...long, commits: [STARTED_ON, COMMIT, ...listed.slice(2)] }, session, neverAsked), null, 'both commits are among the first 250'); + assert.equal(await pullRequestMismatch({ ...long, commits: [COMMIT, ...listed.slice(1)] }, session, onBase), null, 'the commit of the run is among the first 250 and the base branch holds the other'); + const short = await pullRequestMismatch({ ...long, commitCount: 250 }, session, notOnBase); + assert.equal(short?.why, 'the session started on dddddddddddd, which is not one of its commits or a commit of its base branch'); assert.notEqual(short?.fix, refused?.fix); }); }); @@ -529,7 +543,8 @@ describe('attach on a machine whose gh cannot attach', () => { describe('the remote backend before it sends evidence', () => { const settings: RemoteSettings = { platform: 'ios', repo: 'clerk/clerk-ios', workflow: 'verify-remote.yml', sessionsDir: '/nonexistent', runner: 'xcode-27', plumbingRunner: 'ubuntu-latest', device: 'iPhone Air', idleMinutes: 15, capMinutes: 60, requirement: 'a pushed branch' }; const STARTED_ON = 'd'.repeat(40); - const OPEN = { state: 'open', body: '## Summary\r\n\r\nFixes the button.\r\n', head: { ref: 'feature/change', sha: COMMIT, repo: { full_name: 'clerk/clerk-ios' } } }; + const BASE = 'ba5e'.repeat(10); + const OPEN = { state: 'open', body: '## Summary\r\n\r\nFixes the button.\r\n', base: { sha: BASE }, head: { ref: 'feature/change', sha: COMMIT, repo: { full_name: 'clerk/clerk-ios' } } }; interface Case { readonly pull?: unknown; @@ -538,6 +553,7 @@ describe('the remote backend before it sends evidence', () => { readonly files?: EvidenceBundle['files']; readonly before?: string; readonly compared?: { readonly status: number; readonly json: unknown }; + readonly baseCompared?: { readonly status: number; readonly json: unknown }; } async function handOff(options: Case = {}) { @@ -561,6 +577,7 @@ describe('the remote backend before it sends evidence', () => { asked.push(path); const page = /^\/pulls\/669\/commits\?per_page=100&page=(\d+)$/.exec(path); if (page !== null) return { status: 200, json: commits.slice((Number(page[1]) - 1) * 100, Number(page[1]) * 100).map((sha) => ({ sha })), headers: new Headers() }; + if (path.startsWith(`/compare/${STARTED_ON}...`)) return { ...(options.baseCompared ?? { status: 200, json: { status: 'diverged' } }), headers: new Headers() }; if (path.startsWith('/compare/')) return { ...(options.compared ?? { status: 200, json: { status: 'ahead' } }), headers: new Headers() }; return { status: 200, json: path.startsWith('/pulls/') ? (options.pull ?? OPEN) : { head_branch: 'feature/change', head_sha: STARTED_ON }, headers: new Headers() }; }, @@ -645,14 +662,44 @@ describe('the remote backend before it sends evidence', () => { assert.deepEqual(closed.sent, []); }); + it('sends a run of a session that started on a commit the base branch holds, as one that came up before the first push of the change did', async () => { + for (const status of ['ahead', 'identical']) { + const { error, sent, asked } = await handOff({ commits: [COMMIT], baseCompared: { status: 200, json: { status } } }); + assert.equal(error, null, status); + assert.deepEqual(sent, ['/__sim/evidence/begin', '/__sim/evidence/chunk', '/__sim/evidence/commit'], status); + assert.deepEqual(asked.filter((path) => path.startsWith('/compare/')), [`/compare/${STARTED_ON}...${BASE}?per_page=1`], status); + } + }); + + it('sends nothing when the base branch does not hold the commit the session started on, or GitHub does not say that it does', async () => { + const message = `${wouldNot}the session started on dddddddddddd, which is not one of its commits or a commit of its base branch`; + for (const baseCompared of [{ status: 200, json: { status: 'behind' } }, { status: 200, json: { status: 'diverged' } }, { status: 200, json: { status: 'AHEAD' } }, { status: 200, json: null }]) { + const refused = await refusal({ commits: [COMMIT], baseCompared }); + assert.deepEqual({ message: refused.error.message, sent: refused.sent }, { message, sent: [] }, JSON.stringify(baseCompared)); + } + for (const base of [undefined, {}, { sha: 'main' }, { sha: `${BASE.slice(0, 39)}?` }]) { + const { error, asked } = await handOff({ commits: [COMMIT], pull: { ...OPEN, base }, baseCompared: { status: 200, json: { status: 'ahead' } } }); + assert.equal(error?.message, message, JSON.stringify(base)); + assert.deepEqual(asked.filter((path) => path.startsWith('/compare/')), [], 'no question is asked about a base that is not a commit id'); + } + for (const status of [404, 502]) { + const unread = await refusal({ commits: [COMMIT], baseCompared: { status, json: { message: 'No answer' } } }); + assert.deepEqual({ code: unread.error.code, fix: unread.error.fix, sent: unread.sent }, { code: 'NOT_READY', fix: 'retry {cli} attach', sent: [] }, String(status)); + assert.match(unread.error.message, /^could not compare the commit the session started on with the base branch of PR #669: /, String(status)); + } + }); + it('sends nothing when the commit the session started on, or the commit of the run, is not a commit of the pull request', async () => { const rewritten = await refusal({ commits: ['e'.repeat(40), COMMIT] }); - assert.equal(rewritten.error.message, `${wouldNot}the session started on dddddddddddd, which is not one of its commits`); + assert.equal(rewritten.error.message, `${wouldNot}the session started on dddddddddddd, which is not one of its commits or a commit of its base branch`); assert.match(rewritten.error.fix, /git push, then \{cli\} run again and attach, or after a rewrite of the branch's history \{cli\} down and \{cli\} up first$/); assert.deepEqual(rewritten.sent, []); const unpushed = await refusal({ commits: [STARTED_ON, 'e'.repeat(40)] }); assert.equal(unpushed.error.message, `${wouldNot}the run was made at 0f50b597b1c2, which is not one of its commits`); assert.deepEqual(unpushed.sent, []); + const beforeOnly = await refusal({ commits: ['e'.repeat(40)], baseCompared: { status: 200, json: { status: 'identical' } } }); + assert.equal(beforeOnly.error.message, `${wouldNot}the run was made at 0f50b597b1c2, which is not one of its commits`, 'a run at a commit that only the base branch holds'); + assert.deepEqual(beforeOnly.sent, []); }); it('finds the commit the session started on in a later page of a long pull request', async () => { diff --git a/scripts/verify-attach.mjs b/scripts/verify-attach.mjs index 29a7285256f..892ff141764 100644 --- a/scripts/verify-attach.mjs +++ b/scripts/verify-attach.mjs @@ -382,12 +382,15 @@ function pullRequestMismatch(pull, session) { return null; } -function commitNotListed(commits, session) { - if (!commits.includes(session.startedOn)) { - return 'the commit the session started on'; +function commitNotHeld(commits, session, baseHoldsStart) { + if (!commits.includes(session.startedOn) && !baseHoldsStart()) { + return { + commit: 'the commit the session started on', + why: 'is not one of its commits or a commit of its base branch', + }; } if (!commits.includes(session.runCommit)) { - return 'the commit the verify run was made at'; + return { commit: 'the commit the verify run was made at', why: 'is not one of its commits' }; } return null; } @@ -704,7 +707,16 @@ const COMMITS_PER_PAGE = 100; const COMMIT_PAGES = 3; const MOST_COMMITS_GITHUB_LISTS = 250; -function publishStep({ env, say, gh = realGh, dryRun = false, pullFile, commitsFile, nonce = unguessable }) { +function publishStep({ + env, + say, + gh = realGh, + dryRun = false, + pullFile, + commitsFile, + compareFile, + nonce = unguessable, +}) { const { repo, runId, work, filesDir, manifestFile } = settings(env); const startedOn = shaped(env.HEAD_SHA, SHAPE.commit, 'HEAD_SHA'); const actor = shaped(env.RUN_ACTOR, SHAPE.actor, 'RUN_ACTOR'); @@ -767,6 +779,31 @@ function publishStep({ env, say, gh = realGh, dryRun = false, pullFile, commitsF } return shas; }; + const baseHoldsStart = () => { + const base = pull.base?.sha; + if (typeof base !== 'string' || !SHAPE.commit.test(base)) { + return false; + } + const compareJson = compareFile ?? path.join(work, 'compare.json'); + if (compareFile === undefined) { + const read = gh(['api', `repos/${repo}/compare/${startedOn}...${base}?per_page=1`], { + cwd: work, + stdoutFile: compareJson, + }); + if (read.status !== 0) { + throw new Failed( + `gh could not compare the commit the session started on with the base branch of pull request #${manifest.pr}\n${printable(read.stderr)}`, + ); + } + } + let status; + try { + status = JSON.parse(fs.readFileSync(compareJson, 'utf8'))?.status; + } catch { + status = null; + } + return status === 'ahead' || status === 'identical'; + }; const notThisRun = why => { say( 'notice', @@ -781,16 +818,16 @@ function publishStep({ env, say, gh = realGh, dryRun = false, pullFile, commitsF if (mismatch !== null) { return notThisRun(mismatch); } - const notListed = commitNotListed(readCommits(), session); - if (notListed !== null && pull.commits > MOST_COMMITS_GITHUB_LISTS) { + const notHeld = commitNotHeld(readCommits(), session, baseHoldsStart); + if (notHeld !== null && pull.commits > MOST_COMMITS_GITHUB_LISTS) { say( 'notice', - `Pull request #${manifest.pr} has ${pull.commits} commits and GitHub lists only the first ${MOST_COMMITS_GITHUB_LISTS}, which do not hold ${notListed}, so run ${runId} cannot be checked against it and nothing is published.`, + `Pull request #${manifest.pr} has ${pull.commits} commits and GitHub lists only the first ${MOST_COMMITS_GITHUB_LISTS}, which do not hold ${notHeld.commit}, so run ${runId} cannot be checked against it and nothing is published.`, ); return 'refused'; } - if (notListed !== null) { - return notThisRun(`${notListed} is not one of its commits`); + if (notHeld !== null) { + return notThisRun(`${notHeld.commit} ${notHeld.why}`); } let next = null; for (let attempt = 0; attempt < 2 && next === null; attempt += 1) { @@ -905,10 +942,13 @@ async function main(argv, env, log = console.log) { dryRun: rest.includes('--dry-run'), pullFile: flag('--pull'), commitsFile: flag('--commits'), + compareFile: flag('--compare'), }); return 0; } - log('usage: verify-attach.mjs fetch [--zip ] | publish [--dry-run] [--pull --commits ]'); + log( + 'usage: verify-attach.mjs fetch [--zip ] | publish [--dry-run] [--pull --commits [--compare ]]', + ); return 2; } catch (error) { if (error instanceof Refused) { diff --git a/scripts/verify-attach.test.mjs b/scripts/verify-attach.test.mjs index 237a1d8bb74..583dab27070 100644 --- a/scripts/verify-attach.test.mjs +++ b/scripts/verify-attach.test.mjs @@ -1195,11 +1195,12 @@ describe('the step that fetches the evidence', () => { }); describe('the step that edits the description', () => { + const BASE = 'ba5e'.repeat(10); const PULL = { state: 'open', body: '## Summary\r\n\r\nFixes the button.\r\n', head: { ref: 'feature/change', sha: COMMIT, repo: { full_name: REPO } }, - base: { repo: { full_name: REPO } }, + base: { sha: BASE, repo: { full_name: REPO } }, }; async function fetched(manifest = GOOD, media = MEDIA) { @@ -1213,7 +1214,7 @@ describe('the step that edits the description', () => { const asset = n => `https://github.com/user-attachments/assets/00000000-0000-4000-8000-${String(n).padStart(12, '0')}`; - function fakeGh(pull = PULL, onRead = () => undefined, commits = COMMITS) { + function fakeGh(pull = PULL, onRead = () => undefined, commits = COMMITS, compared = '{"status":"diverged"}') { const state = { pull: structuredClone(pull), reads: 0, @@ -1230,6 +1231,10 @@ describe('the step that edits the description', () => { fs.writeFileSync(options.stdoutFile, JSON.stringify(commits.slice(from, from + 100))); return { status: 0, stderr: '' }; } + if (args[0] === 'api' && /\/compare\//.test(args[1] ?? '')) { + fs.writeFileSync(options.stdoutFile, compared); + return { status: 0, stderr: '' }; + } if (args[0] === 'api') { state.reads += 1; onRead(state); @@ -1384,6 +1389,26 @@ describe('the step that edits the description', () => { ); }); + it('reads the comparison with the base branch from a file in a dry run that names one', async () => { + const dry = async status => { + const env = { ...(await fetched()), HEAD_SHA: 'd'.repeat(40) }; + const file = name => path.join(path.dirname(env.WORK), name); + fs.writeFileSync(file('pull.json'), JSON.stringify(PULL)); + fs.writeFileSync(file('commits.json'), JSON.stringify([{ sha: COMMIT }])); + fs.writeFileSync(file('compare.json'), JSON.stringify({ status })); + const lines = []; + const files = ['--pull', file('pull.json'), '--commits', file('commits.json'), '--compare', file('compare.json')]; + assert.equal(await main(['publish', '--dry-run', ...files], env, line => lines.push(line)), 0); + return lines.map(line => line.replace(/-[0-9a-f]{16}-/g, '--').replaceAll(env.WORK, '')); + }; + assert.deepEqual(await dry('ahead'), [ + 'would run, in /files:\ngh pr edit 669 --repo clerk/clerk-ios --body-file /body.md --attach ./evidence--1.mp4 --attach ./evidence--2.png', + ]); + assert.deepEqual(await dry('diverged'), [ + '::notice title=verify-attach::Pull request #669 is not the pull request of run 37731397352 (the commit the session started on is not one of its commits or a commit of its base branch), so nothing is published.', + ]); + }); + const SHOT = [['profile.png', PNG]]; const WAS_SHOT = [['before-profile.png', WAS_PNG]]; const CLIP = [['video.mp4', MP4]]; @@ -1768,10 +1793,16 @@ describe('the step that edits the description', () => { ]); const early = fakeGh(long, undefined, [{ sha: COMMIT }, ...listed.slice(1)]); assert.equal(publish(await fetched(), early).outcome, 'published', 'both commits are among the first 250'); + const onBase = fakeGh(long, undefined, [{ sha: COMMIT }, ...listed.slice(1)], '{"status":"ahead"}'); + assert.equal( + publish(await startedEarlier(), onBase).outcome, + 'published', + 'the run commit is among the first 250 and the base branch holds the commit the session started on', + ); const short = fakeGh({ ...PULL, commits: 250 }, undefined, listed); assert.match( publish(await fetched(), short).said[0], - /is not the pull request of run 37731397352 \(the commit the session started on is not one of its commits\)/, + /is not the pull request of run 37731397352 \(the commit the session started on is not one of its commits or a commit of its base branch\)/, ); }); @@ -1781,7 +1812,7 @@ describe('the step that edits the description', () => { assert.equal(outcome, 'refused'); assert.equal(fake.state.writtenBody, null); assert.deepEqual(said, [ - 'notice: Pull request #669 is not the pull request of run 37731397352 (the commit the session started on is not one of its commits), so nothing is published.', + 'notice: Pull request #669 is not the pull request of run 37731397352 (the commit the session started on is not one of its commits or a commit of its base branch), so nothing is published.', ]); const other = fakeGh(PULL, undefined, [{ sha: COMMIT }]); const refused = publish(await fetched(manifestFor(MEDIA, { commit: 'c'.repeat(40) })), other); @@ -1791,16 +1822,73 @@ describe('the step that edits the description', () => { assert.equal(other.state.writtenBody, null); }); - it('publishes nothing when the commit the session started on is not a commit of the pull request', async () => { - const fake = fakeGh(PULL, undefined, [{ sha: 'e'.repeat(40) }, { sha: COMMIT }]); - const { outcome, said } = publish(await startedEarlier(), fake); + const comparisons = fake => fake.state.calls.filter(call => /\/compare\//.test(call.args[1] ?? '')); + const NOT_STARTED_HERE = + 'notice: Pull request #669 is not the pull request of run 37731397352 (the commit the session started on is not one of its commits or a commit of its base branch), so nothing is published.'; + + it('publishes a run of a session that started on a commit the base branch holds, the commit the branch came from', async () => { + for (const status of ['ahead', 'identical']) { + const fake = fakeGh(PULL, undefined, [{ sha: COMMIT }], JSON.stringify({ status })); + const { outcome } = publish(await startedEarlier(), fake); + assert.equal(outcome, 'published', status); + assert.deepEqual( + comparisons(fake).map(call => call.args), + [['api', `repos/clerk/clerk-ios/compare/${STARTED_ON}...${BASE}?per_page=1`]], + status, + ); + } + const listed = fakeGh(PULL, undefined, [{ sha: STARTED_ON }, { sha: COMMIT }]); + assert.equal(publish(await startedEarlier(), listed).outcome, 'published'); + assert.deepEqual(comparisons(listed), [], 'a commit of the pull request needs no question'); + }); + + it('publishes nothing when the commit the session started on is not a commit of the pull request, and the base branch does not hold it', async () => { + const answers = ['{"status":"behind"}', '{"status":"diverged"}', '{"status":"AHEAD"}', '{}', 'null', '']; + for (const compared of answers) { + const fake = fakeGh(PULL, undefined, [{ sha: 'e'.repeat(40) }, { sha: COMMIT }], compared); + const { outcome, said } = publish(await startedEarlier(), fake); + assert.equal(outcome, 'refused', compared); + assert.equal(fake.state.writtenBody, null, compared); + assert.deepEqual(said, [NOT_STARTED_HERE], compared); + assert.equal(comparisons(fake).length, 1, compared); + } + for (const base of [ + { repo: PULL.base.repo }, + { ...PULL.base, sha: `${BASE.slice(0, 39)}?` }, + { ...PULL.base, sha: 'main' }, + ]) { + const fake = fakeGh({ ...PULL, base }, undefined, [{ sha: COMMIT }], '{"status":"ahead"}'); + assert.deepEqual(publish(await startedEarlier(), fake).said, [NOT_STARTED_HERE], JSON.stringify(base)); + assert.deepEqual(comparisons(fake), [], 'no question is asked about a base that is not a commit id'); + } + }); + + it('publishes nothing for a run made at a commit that only the base branch holds, such as a before run alone', async () => { + const fake = fakeGh(PULL, undefined, [{ sha: 'e'.repeat(40) }], '{"status":"identical"}'); + const { outcome, said } = publish(await fetched(), fake); assert.equal(outcome, 'refused'); assert.equal(fake.state.writtenBody, null); assert.deepEqual(said, [ - 'notice: Pull request #669 is not the pull request of run 37731397352 (the commit the session started on is not one of its commits), so nothing is published.', + 'notice: Pull request #669 is not the pull request of run 37731397352 (the commit the verify run was made at is not one of its commits), so nothing is published.', ]); }); + it('fails when GitHub cannot compare the commit the session started on with the base branch, and writes nothing', async () => { + const fake = fakeGh(PULL, undefined, [{ sha: COMMIT }], '{"status":"ahead"}'); + const failing = { + gh: (args, options) => + /\/compare\//.test(args[1] ?? '') + ? { status: 1, stderr: 'gh: Not Found (HTTP 404)\n' } + : fake.gh(args, options), + }; + const env = await startedEarlier(); + assert.throws( + () => publish(env, failing), + /^Error: gh could not compare the commit the session started on with the base branch of pull request #669\ngh: gh: Not Found \(HTTP 404\)$/, + ); + assert.equal(fake.state.writtenBody, null); + }); + it('reads every page of the commits GitHub lists, and stops at the last one', async () => { const many = Array.from({ length: 230 }, (_, i) => ({ sha: i.toString(16).padStart(40, '0') })); const pagesRead = fake => fake.state.calls.filter(call => /\/commits\?/.test(call.args[1] ?? '')).length; From e2d48927275f5481a8db6afc7e53d0f3ab33afb1 Mon Sep 17 00:00:00 2001 From: Mike Pitre <12040919+mikepitre@users.noreply.github.com> Date: Tue, 13 Oct 2026 20:42:19 -0400 Subject: [PATCH 2/2] test(expo): refuse a session whose start commit GitHub does not find GitHub answers 404 to the comparison of a start commit that the repository no longer has. The publish job failed on that answer, and `attach` said to retry, which cannot change it. The job now prints a notice that names the reason and publishes nothing. `attach` fails with the reason and says to start a session on a commit of the pull request or of its base branch. Any other failed comparison still fails the job, and `attach` still says to retry. `publish --dry-run` with `--compare` and no file after it asked GitHub instead of reading a file. It now prints the usage and exits with 2. The order of work in the skill names `--before ` at the `attach` step again. What differs between a local device and a remote session has a paragraph of its own. Co-Authored-By: Claude Opus 5.5 --- .claude/skills/verify-clerk-expo/SKILL.md | 4 +- .../verify-clerk-expo/references/remote.md | 2 +- integration/expo-native/src/core/MANIFEST | 4 +- .../expo-native/src/core/remote/backend.ts | 3 +- .../expo-native/src/core/remote/handoff.ts | 7 ++- integration/expo-native/test/handoff.test.ts | 23 +++++++- scripts/verify-attach.mjs | 18 +++++-- scripts/verify-attach.test.mjs | 54 ++++++++++++++----- 8 files changed, 92 insertions(+), 23 deletions(-) diff --git a/.claude/skills/verify-clerk-expo/SKILL.md b/.claude/skills/verify-clerk-expo/SKILL.md index 4c31cd67ff0..75a85314417 100644 --- a/.claude/skills/verify-clerk-expo/SKILL.md +++ b/.claude/skills/verify-clerk-expo/SKILL.md @@ -225,7 +225,9 @@ When this machine cannot attach and there is no session to hand the files to, or `attach` writes only to an open pull request, and fails on one that is closed or merged. It refuses a run that is tainted, that has a failing spec or no passing one, or whose `app.log` names a user that the run did not create. A before run can have failing specs. `attach` refuses a before run that is tainted, that was never sealed, or whose `app.log` names a user that the run did not create. -Work in this order, with one `up` for both runs: make the branch and `git push` it, `up`, `run` the spec on the build without your change when the change is to something that exists, make the change, commit, `git push`, `run` the spec again, open the pull request, `attach`, then `down`. Open the pull request after the last run, so that it is not left open without its evidence. On a local device `attach` writes the description itself. On a remote session the evidence reaches the description about a minute after `down`. A remote session builds the commit it was started on and later commits of the branch, and never an earlier one, so there the before run comes before the push of the change. Commit before the second `run`, so that the line `attach` writes names a commit the pull request holds. In the pull request, name the run id and the specs you ran, and say what you did not run. +Work in this order, with one `up` for both runs: make the branch and `git push` it, `up`, `run` the spec without your change when it changes something that exists, make the change, commit, `git push`, `run` the spec again, open the pull request, `attach` with the before run as `--before `, `down`. Open the pull request after the last run, so that it is not left open without its evidence. Commit before the second `run`, so that the line `attach` writes names a commit the pull request holds. In the pull request, name the run id and the specs you ran, and say what you did not run. + +On a local device `attach` writes the description itself, and no run needs a push. A remote session builds only the commit it was started on and later commits of the branch, so its before run comes before the push of the change. Its evidence reaches the description about a minute after `down`. Attach the run of your own change. Run your new or changed spec on its own and attach that run, so the screenshots or the video show only what the change is about. You do not owe a regression run: PR CI (`.github/workflows/verify-e2e.yml`) runs every golden spec on the pull request and reports them there. If you ran other golden specs anyway, cite that run's id in the PR and leave its video in `.verify/runs/`. diff --git a/.claude/skills/verify-clerk-expo/references/remote.md b/.claude/skills/verify-clerk-expo/references/remote.md index 4bc99aee84a..c2d7c7376ba 100644 --- a/.claude/skills/verify-clerk-expo/references/remote.md +++ b/.claude/skills/verify-clerk-expo/references/remote.md @@ -105,7 +105,7 @@ The workflow publishes only when all of these hold. Otherwise it prints the reas `attach` checks the same rules before it sends anything, and fails with the reason. -So one session does the before run and the run of the change: push the branch, `up`, `run`, push the change, `run` again, open the pull request, `attach`, `down`. Open the pull request after the last run, so that it is not left open without its evidence, which reaches the description about a minute after `down`. +So one session does both runs: push the branch, `up`, `run` for the before run, push the change, `run` again, open the pull request, `attach` with `--before `, `down`. Open the pull request after the last run, so that it is not left open without its evidence, which reaches the description about a minute after `down`. A before run is the build without the change, so its commit is often the one the branch started from, which is no commit of the pull request. The workflow therefore does not look for that commit among the commits of the pull request. It asks GitHub whether that commit is the commit of the run or a commit before it in its history, and it publishes nothing when the answer is no. After a rebase that rewrote the commit of the before run, `run` the spec again on a commit that the branch still has, and pass that run as `--before`. The workflow cannot check that the files of the before run came from a build of that commit, which is why the line says that the session reports the before run. diff --git a/integration/expo-native/src/core/MANIFEST b/integration/expo-native/src/core/MANIFEST index 22b2604ad27..7b42cc7d377 100644 --- a/integration/expo-native/src/core/MANIFEST +++ b/integration/expo-native/src/core/MANIFEST @@ -35,9 +35,9 @@ cf6aa68be73e4abaf3b63d5d84c2e9eee6197412f607ec53d0f05f7b058db420 src/core/insta a0ee9e4bfd34d4659685108e5e54eb1ec2aff6248236c358e6d3b14b2e2ff6a0 src/core/ledgers.ts afab94a970286c40d6857b03ae4fa39ec59786274beacf6f1e77f6530bf46c54 src/core/manifest.ts 7294bbab3d892f0d9aee1e863f84e32a7f5dc3c467a7b6177be7e4ff2f29fecc src/core/publish.ts -8302ab57092212ec057f357c38bffec97101ff23c546a76941d898c1612d58b0 src/core/remote/backend.ts +83a0a19bc3cf6ce0c1435f6624204a17060000064cf6ca49b8fa39143d7871b6 src/core/remote/backend.ts 37fc42707be7d8bd5f0313a664a8465287ebb0970c520c0e0617f6ff6c9a614e src/core/remote/github.ts -1557283a67138c35cb896c2eb1c821be06dc786b8a6aa93a3c9f23f447689f38 src/core/remote/handoff.ts +d9f7e8d2f69960aba668dc2e18d6d61481c80967459922ee9fe33265b27c1735 src/core/remote/handoff.ts 2ec436b2c53b34269e0b62ab54e41e4d9a55146cccc46f0f31591db05cdbf5c1 src/core/remote/preflight.ts bb69fe1f3581942285894c0901d02889c08b4293e4d9db77934fe3cdd2d4a399 src/core/remote/protocol.ts 5a4ee0e7ed20feaeb711b1527083299b432d3fc0b7f428b636678a8a2f57581f src/core/remote/recipe.ts diff --git a/integration/expo-native/src/core/remote/backend.ts b/integration/expo-native/src/core/remote/backend.ts index e989a272057..6edd24db87c 100644 --- a/integration/expo-native/src/core/remote/backend.ts +++ b/integration/expo-native/src/core/remote/backend.ts @@ -31,9 +31,10 @@ async function pullRequestCommits(hub: GitHub, pr: number): Promise { +async function baseHolds(hub: GitHub, pr: number, base: unknown, commit: string): Promise { if (typeof base !== 'string' || !/^[0-9a-f]{40}$/.test(base)) return false; const compared = await hub.api('GET', `/compare/${commit}...${base}?per_page=1`); + if (compared.status === 404) return 'unknown'; if (compared.status !== 200) throw new VerifyFailure('NOT_READY', `could not compare the commit the session started on with the base branch of PR #${pr}: ${apiMessage(compared)}`, 'retry {cli} attach'); const status = (compared.json as { readonly status?: unknown } | null)?.status; return status === 'ahead' || status === 'identical'; diff --git a/integration/expo-native/src/core/remote/handoff.ts b/integration/expo-native/src/core/remote/handoff.ts index 57d34791c94..b7c68928bc1 100644 --- a/integration/expo-native/src/core/remote/handoff.ts +++ b/integration/expo-native/src/core/remote/handoff.ts @@ -188,19 +188,22 @@ export interface PullRequestMismatch { const ITS_OWN_PULL_REQUEST = "it accepts only the open pull request of the session's branch, and only when the commit of the run is a commit of that pull request and the commit the session started on is one too or is a commit of its base branch; git push, then {cli} run again and attach, or after a rewrite of the branch's history {cli} down and {cli} up first"; +const A_COMMIT_GITHUB_FINDS = '{cli} down, then {cli} up on a commit of the pull request or of its base branch, run again and attach'; const FEWER_COMMITS = `it can check a commit only against the first ${MOST_COMMITS_GITHUB_LISTS} commits of a pull request; squash or rebase the branch to ${MOST_COMMITS_GITHUB_LISTS} commits or fewer, git push, then {cli} down, {cli} up, run again and attach, or run {cli} attach on a machine whose gh can attach`; export async function pullRequestMismatch( pull: PullRequestView, session: { readonly repo: string; readonly headBranch: string; readonly startedOn: string; readonly runCommit: string }, - baseHoldsStart: () => Promise, + baseHoldsStart: () => Promise, ): Promise { const refused = (why: string): PullRequestMismatch => ({ why, fix: ITS_OWN_PULL_REQUEST }); if (pull.state !== 'open') return refused(`it is ${pull.state}`); if (pull.headRepo !== session.repo) return refused(`its branch is in ${pull.headRepo ?? 'a deleted fork'}, not in ${session.repo}`); if (pull.headRef !== session.headBranch) return refused(`its branch is ${pull.headRef} and the session runs on ${session.headBranch}`); + const startHeld = pull.commits.includes(session.startedOn) || (await baseHoldsStart()); + if (startHeld === 'unknown') return { why: `the session started on ${session.startedOn.slice(0, 12)}, which GitHub does not find in ${session.repo}`, fix: A_COMMIT_GITHUB_FINDS }; const notHeld = - !pull.commits.includes(session.startedOn) && !(await baseHoldsStart()) + !startHeld ? { commit: `the session started on ${session.startedOn.slice(0, 12)}`, among: 'one of its commits or a commit of its base branch' } : !pull.commits.includes(session.runCommit) ? { commit: `the run was made at ${session.runCommit.slice(0, 12)}`, among: 'one of its commits' } diff --git a/integration/expo-native/test/handoff.test.ts b/integration/expo-native/test/handoff.test.ts index 32777fd1b86..7891727337b 100644 --- a/integration/expo-native/test/handoff.test.ts +++ b/integration/expo-native/test/handoff.test.ts @@ -214,6 +214,14 @@ describe('the rule for which pull request a session may publish to', () => { assert.equal(short?.why, 'the session started on dddddddddddd, which is not one of its commits or a commit of its base branch'); assert.notEqual(short?.fix, refused?.fix); }); + + it('refuses a session that started on a commit GitHub does not find, in a pull request of any length, and says where to start one', async () => { + const unknown = async (): Promise<'unknown'> => 'unknown'; + const refused = { why: 'the session started on dddddddddddd, which GitHub does not find in clerk/clerk-ios', fix: '{cli} down, then {cli} up on a commit of the pull request or of its base branch, run again and attach' }; + assert.deepEqual(await pullRequestMismatch({ ...pull, commits: [COMMIT] }, session, unknown), refused); + assert.deepEqual(await pullRequestMismatch({ ...pull, commits: [COMMIT], commitCount: 300 }, session, unknown), refused); + assert.equal(await pullRequestMismatch(pull, session, neverAsked), null); + }); }); describe('the receiver in the session job', () => { @@ -682,13 +690,26 @@ describe('the remote backend before it sends evidence', () => { assert.equal(error?.message, message, JSON.stringify(base)); assert.deepEqual(asked.filter((path) => path.startsWith('/compare/')), [], 'no question is asked about a base that is not a commit id'); } - for (const status of [404, 502]) { + for (const status of [403, 502]) { const unread = await refusal({ commits: [COMMIT], baseCompared: { status, json: { message: 'No answer' } } }); assert.deepEqual({ code: unread.error.code, fix: unread.error.fix, sent: unread.sent }, { code: 'NOT_READY', fix: 'retry {cli} attach', sent: [] }, String(status)); assert.match(unread.error.message, /^could not compare the commit the session started on with the base branch of PR #669: /, String(status)); } }); + it('sends nothing when GitHub does not find the commit the session started on, and says where to start a session', async () => { + const { error, sent } = await refusal({ commits: [COMMIT], baseCompared: { status: 404, json: { message: 'Not Found' } } }); + assert.deepEqual( + { code: error.code, message: error.message, fix: error.fix, sent }, + { + code: 'NOT_READY', + message: `${wouldNot}the session started on dddddddddddd, which GitHub does not find in clerk/clerk-ios`, + fix: '{cli} down, then {cli} up on a commit of the pull request or of its base branch, run again and attach', + sent: [], + }, + ); + }); + it('sends nothing when the commit the session started on, or the commit of the run, is not a commit of the pull request', async () => { const rewritten = await refusal({ commits: ['e'.repeat(40), COMMIT] }); assert.equal(rewritten.error.message, `${wouldNot}the session started on dddddddddddd, which is not one of its commits or a commit of its base branch`); diff --git a/scripts/verify-attach.mjs b/scripts/verify-attach.mjs index 892ff141764..5763f64f2cf 100644 --- a/scripts/verify-attach.mjs +++ b/scripts/verify-attach.mjs @@ -383,7 +383,15 @@ function pullRequestMismatch(pull, session) { } function commitNotHeld(commits, session, baseHoldsStart) { - if (!commits.includes(session.startedOn) && !baseHoldsStart()) { + const startHeld = commits.includes(session.startedOn) || baseHoldsStart(); + if (startHeld === 'unknown') { + return { + commit: 'the commit the session started on', + why: 'is not a commit GitHub finds in this repository', + unknown: true, + }; + } + if (!startHeld) { return { commit: 'the commit the session started on', why: 'is not one of its commits or a commit of its base branch', @@ -790,6 +798,9 @@ function publishStep({ cwd: work, stdoutFile: compareJson, }); + if (read.status !== 0 && / \(HTTP 404\)$/m.test(read.stderr)) { + return 'unknown'; + } if (read.status !== 0) { throw new Failed( `gh could not compare the commit the session started on with the base branch of pull request #${manifest.pr}\n${printable(read.stderr)}`, @@ -819,7 +830,7 @@ function publishStep({ return notThisRun(mismatch); } const notHeld = commitNotHeld(readCommits(), session, baseHoldsStart); - if (notHeld !== null && pull.commits > MOST_COMMITS_GITHUB_LISTS) { + if (notHeld !== null && !notHeld.unknown && pull.commits > MOST_COMMITS_GITHUB_LISTS) { say( 'notice', `Pull request #${manifest.pr} has ${pull.commits} commits and GitHub lists only the first ${MOST_COMMITS_GITHUB_LISTS}, which do not hold ${notHeld.commit}, so run ${runId} cannot be checked against it and nothing is published.`, @@ -927,6 +938,7 @@ async function main(argv, env, log = console.log) { const say = (kind, text) => log(kind === 'plain' ? text : workflowCommand(kind, text)); const [step, ...rest] = argv; const flag = name => (rest.includes(name) ? rest[rest.indexOf(name) + 1] : undefined); + const namesNoFile = name => rest.includes(name) && flag(name) === undefined; try { if (step === 'fetch') { const found = await fetchStep({ env, say, fetch, zipFile: flag('--zip') }); @@ -935,7 +947,7 @@ async function main(argv, env, log = console.log) { } return 0; } - if (step === 'publish') { + if (step === 'publish' && !namesNoFile('--compare')) { publishStep({ env, say, diff --git a/scripts/verify-attach.test.mjs b/scripts/verify-attach.test.mjs index 583dab27070..a734c4ca961 100644 --- a/scripts/verify-attach.test.mjs +++ b/scripts/verify-attach.test.mjs @@ -1409,6 +1409,19 @@ describe('the step that edits the description', () => { ]); }); + it('prints the usage and compares nothing when --compare names no file', async () => { + const env = await fetched(); + const file = name => path.join(path.dirname(env.WORK), name); + fs.writeFileSync(file('pull.json'), JSON.stringify(PULL)); + fs.writeFileSync(file('commits.json'), JSON.stringify(COMMITS)); + const lines = []; + const files = ['--pull', file('pull.json'), '--commits', file('commits.json'), '--compare']; + assert.equal(await main(['publish', '--dry-run', ...files], env, line => lines.push(line)), 2); + assert.deepEqual(lines, [ + 'usage: verify-attach.mjs fetch [--zip ] | publish [--dry-run] [--pull --commits [--compare ]]', + ]); + }); + const SHOT = [['profile.png', PNG]]; const WAS_SHOT = [['before-profile.png', WAS_PNG]]; const CLIP = [['video.mp4', MP4]]; @@ -1873,20 +1886,37 @@ describe('the step that edits the description', () => { ]); }); - it('fails when GitHub cannot compare the commit the session started on with the base branch, and writes nothing', async () => { + const comparing = (fake, stderr) => ({ + ...fake, + gh: (args, options) => (/\/compare\//.test(args[1] ?? '') ? { status: 1, stderr } : fake.gh(args, options)), + }); + + it('publishes nothing, and does not fail, when GitHub does not find the commit the session started on', async () => { + const notFound = 'gh: Not Found (HTTP 404)\n'; const fake = fakeGh(PULL, undefined, [{ sha: COMMIT }], '{"status":"ahead"}'); - const failing = { - gh: (args, options) => - /\/compare\//.test(args[1] ?? '') - ? { status: 1, stderr: 'gh: Not Found (HTTP 404)\n' } - : fake.gh(args, options), - }; - const env = await startedEarlier(); - assert.throws( - () => publish(env, failing), - /^Error: gh could not compare the commit the session started on with the base branch of pull request #669\ngh: gh: Not Found \(HTTP 404\)$/, - ); + const { outcome, said } = publish(await startedEarlier(), comparing(fake, notFound)); + assert.equal(outcome, 'refused'); assert.equal(fake.state.writtenBody, null); + assert.deepEqual(said, [ + 'notice: Pull request #669 is not the pull request of run 37731397352 (the commit the session started on is not a commit GitHub finds in this repository), so nothing is published.', + ]); + const long = fakeGh({ ...PULL, commits: 300 }, undefined, [{ sha: COMMIT }], '{"status":"ahead"}'); + assert.deepEqual(publish(await startedEarlier(), comparing(long, notFound)).said, said, 'a long pull request'); + }); + + it('fails when GitHub does not answer the comparison of the commit the session started on with the base branch, and writes nothing', async () => { + for (const stderr of ['gh: Bad Gateway (HTTP 502)\n', 'error connecting to api.github.com\n', '']) { + const fake = fakeGh(PULL, undefined, [{ sha: COMMIT }], '{"status":"ahead"}'); + const env = await startedEarlier(); + assert.throws( + () => publish(env, comparing(fake, stderr)), + error => + error.message === + `gh could not compare the commit the session started on with the base branch of pull request #669\n${stderr === '' ? '' : `gh: ${stderr.trimEnd()}`}`, + stderr, + ); + assert.equal(fake.state.writtenBody, null, stderr); + } }); it('reads every page of the commits GitHub lists, and stops at the last one', async () => {