From 75fc37e9289383a7bb8d5a83a7c1f2cfa64cf68f Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Wed, 8 Jul 2026 21:22:46 -0700 Subject: [PATCH] fix(review): verify PR head before E2E test commits --- src/github/e2e-test-commit.ts | 9 +++++++ test/unit/e2e-test-commit.test.ts | 42 +++++++++++++++++++++++++++++-- test/unit/queue.test.ts | 6 +++-- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/github/e2e-test-commit.ts b/src/github/e2e-test-commit.ts index 49321a2244..ecd7383a45 100644 --- a/src/github/e2e-test-commit.ts +++ b/src/github/e2e-test-commit.ts @@ -62,6 +62,15 @@ export async function commitE2eTestToPrBranch( return await withInstallationTokenRetry(env, args.installationId, async (token) => { const octokit = makeInstallationOctokit(env, token, args.mode, githubRateLimitAdmissionKeyForInstallation(args.installationId)); + const livePr = await octokit.request("GET /repos/{owner}/{repo}/pulls/{pull_number}", { owner, repo, pull_number: args.prNumber }); + const liveHead = (livePr.data as { head?: { ref?: string | null; sha?: string | null; repo?: { full_name?: string | null } | null } }).head; + if (liveHead?.repo?.full_name !== args.repoFullName) { + return { status: "declined", reason: "commit delivery is only supported for same-repository PR branches" }; + } + if (liveHead.ref !== args.headRef || liveHead.sha !== args.headSha) { + return { status: "declined", reason: "the live PR head no longer matches the cached branch/commit — try the command again" }; + } + const headCommit = await octokit.request("GET /repos/{owner}/{repo}/git/commits/{commit_sha}", { owner, repo, commit_sha: args.headSha }); const baseTreeSha = (headCommit.data as { tree: { sha: string } }).tree.sha; diff --git a/test/unit/e2e-test-commit.test.ts b/test/unit/e2e-test-commit.test.ts index 3fa0fd2280..40107d4465 100644 --- a/test/unit/e2e-test-commit.test.ts +++ b/test/unit/e2e-test-commit.test.ts @@ -52,6 +52,7 @@ describe("commitE2eTestToPrBranch (#4197)", () => { const url = input.toString(); if (TOKEN_URL.test(url)) return Response.json({ token: "t" }); const method = init?.method ?? "GET"; + if (url.endsWith("/pulls/42") && method === "GET") return Response.json({ head: { ref: "feature/my-branch", sha: "head-commit-sha", repo: { full_name: REPO } } }); calls.push({ method, url, body: init?.body ? JSON.parse(String(init.body)) : {} }); if (url.endsWith("/git/commits/head-commit-sha") && method === "GET") return Response.json({ tree: { sha: "base-tree-sha" } }); if (url.endsWith("/git/trees") && method === "POST") return Response.json({ sha: "new-tree-sha" }); @@ -86,6 +87,7 @@ describe("commitE2eTestToPrBranch (#4197)", () => { const url = input.toString(); if (TOKEN_URL.test(url)) return Response.json({ token: "t" }); const method = init?.method ?? "GET"; + if (url.endsWith("/pulls/42") && method === "GET") return Response.json({ head: { ref: "feature/my-branch", sha: "head-commit-sha", repo: { full_name: REPO } } }); if (url.endsWith("/git/commits/head-commit-sha") && method === "GET") return Response.json({ tree: { sha: "base-tree-sha" } }); if (url.endsWith("/git/trees") && method === "POST") { treeBody = init?.body ? JSON.parse(String(init.body)) : {}; @@ -101,12 +103,47 @@ describe("commitE2eTestToPrBranch (#4197)", () => { expect((treeBody.tree as Array<{ path: string }>)[0]?.path).toBe("test/e2e/custom.spec.ts"); }); + it("declines fork PR commit delivery before any git write", async () => { + const env = envWithKey(); + const calls: Array<{ method: string; url: string }> = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (TOKEN_URL.test(url)) return Response.json({ token: "t" }); + const method = init?.method ?? "GET"; + calls.push({ method, url }); + if (url.endsWith("/pulls/42") && method === "GET") return Response.json({ head: { ref: "feature/my-branch", sha: "head-commit-sha", repo: { full_name: "fork/widgets" } } }); + return new Response("unexpected", { status: 500 }); + }); + + const result = await commitE2eTestToPrBranch(env, baseArgs); + + expect(result).toEqual({ status: "declined", reason: "commit delivery is only supported for same-repository PR branches" }); + expect(calls.some((call) => call.url.includes("/git/"))).toBe(false); + }); + + it("declines when the live PR head no longer matches the cached branch and sha", async () => { + const env = envWithKey(); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (TOKEN_URL.test(url)) return Response.json({ token: "t" }); + const method = init?.method ?? "GET"; + if (url.endsWith("/pulls/42") && method === "GET") return Response.json({ head: { ref: "feature/new", sha: "new-head-sha", repo: { full_name: REPO } } }); + return new Response("unexpected", { status: 500 }); + }); + + const result = await commitE2eTestToPrBranch(env, baseArgs); + + expect(result).toMatchObject({ status: "declined" }); + if (result.status !== "declined") throw new Error("unreachable"); + expect(result.reason).toContain("live PR head no longer matches"); + }); + it("declines with a clear reason on a 403/404 (no write access -- fork without maintainer edits)", async () => { const env = envWithKey(); vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { const url = input.toString(); if (TOKEN_URL.test(url)) return Response.json({ token: "t" }); - if (url.endsWith("/git/commits/head-commit-sha")) return new Response("forbidden", { status: 403 }); + if (url.endsWith("/pulls/42")) return new Response("forbidden", { status: 403 }); return new Response("unexpected", { status: 500 }); }); @@ -122,6 +159,7 @@ describe("commitE2eTestToPrBranch (#4197)", () => { const url = input.toString(); if (TOKEN_URL.test(url)) return Response.json({ token: "t" }); const method = init?.method ?? "GET"; + if (url.endsWith("/pulls/42") && method === "GET") return Response.json({ head: { ref: "feature/my-branch", sha: "head-commit-sha", repo: { full_name: REPO } } }); if (url.endsWith("/git/commits/head-commit-sha") && method === "GET") return Response.json({ tree: { sha: "base-tree-sha" } }); if (url.endsWith("/git/trees") && method === "POST") return Response.json({ sha: "new-tree-sha" }); if (url.endsWith("/git/commits") && method === "POST") return Response.json({ sha: "new-commit-sha" }); @@ -140,7 +178,7 @@ describe("commitE2eTestToPrBranch (#4197)", () => { vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { const url = input.toString(); if (TOKEN_URL.test(url)) return Response.json({ token: "t" }); - if (url.endsWith("/git/commits/head-commit-sha")) return new Response("server exploded", { status: 500 }); + if (url.endsWith("/pulls/42")) return new Response("server exploded", { status: 500 }); return new Response("unexpected", { status: 500 }); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 9b69174e03..999e72cfb5 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -25600,6 +25600,7 @@ describe("queue processors", () => { const method = init?.method ?? "GET"; if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" }); + if (url.endsWith("/pulls/4207") && method === "GET") return Response.json({ head: { ref: "feature/checkout-retry", sha: "commit-ok-head-sha", repo: { full_name: repoFullName } } }); if (url.endsWith("/git/commits/commit-ok-head-sha") && method === "GET") return Response.json({ tree: { sha: "base-tree-sha" } }); if (url.endsWith("/git/trees") && method === "POST") return Response.json({ sha: "new-tree-sha" }); if (url.endsWith("/git/commits") && method === "POST") return Response.json({ sha: "committed-sha-123" }); @@ -25667,7 +25668,7 @@ describe("queue processors", () => { const method = init?.method ?? "GET"; if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" }); - if (url.endsWith("/git/commits/declined-head-sha") && method === "GET") return new Response("forbidden", { status: 403 }); + if (url.endsWith("/pulls/4209") && method === "GET") return new Response("forbidden", { status: 403 }); if (url.includes("/issues/4209/comments") && method === "GET") return Response.json([]); if (url.includes("/issues/4209/comments") && method === "POST") { postedBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); return Response.json({ id: 42090 }); } return new Response("not found", { status: 404 }); @@ -25733,7 +25734,7 @@ describe("queue processors", () => { if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" }); // Neither a 403/404 (no write access) nor a 422/409 (branch moved) -- a genuinely unexpected 500, // which commitE2eTestToPrBranch maps to status: "error" rather than "declined". - if (url.endsWith("/git/commits/error-mapped-head-sha") && method === "GET") return new Response("server exploded", { status: 500 }); + if (url.endsWith("/pulls/4213") && method === "GET") return new Response("server exploded", { status: 500 }); if (url.includes("/issues/4213/comments") && method === "GET") return Response.json([]); if (url.includes("/issues/4213/comments") && method === "POST") { postedBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); return Response.json({ id: 42130 }); } return new Response("not found", { status: 404 }); @@ -25771,6 +25772,7 @@ describe("queue processors", () => { const method = init?.method ?? "GET"; if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); if (url.includes("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" }); + if (url.endsWith("/pulls/4214") && method === "GET") return Response.json({ head: { ref: "feature/checkout-retry", sha: "no-author-head-sha", repo: { full_name: repoFullName } } }); if (url.endsWith("/git/commits/no-author-head-sha") && method === "GET") return Response.json({ tree: { sha: "base-tree-sha" } }); if (url.endsWith("/git/trees") && method === "POST") return Response.json({ sha: "new-tree-sha" }); if (url.endsWith("/git/commits") && method === "POST") return Response.json({ sha: "no-author-commit-sha" });