diff --git a/.github/workflows/label-pr-review-state-fork-review-followup.yml b/.github/workflows/label-pr-review-state-fork-review-followup.yml index 7df5fda00a..3bcd55dc21 100644 --- a/.github/workflows/label-pr-review-state-fork-review-followup.yml +++ b/.github/workflows/label-pr-review-state-fork-review-followup.yml @@ -52,43 +52,44 @@ jobs: return; } - // Same-repository review runs already ran with a privileged - // token and wrote their labels; only fork runs need the - // follow-up. - if (run.head_repository && run.head_repository.fork === false) { - core.info('Ignoring same-repository pull_request_review run'); - return; - } - - // Resolve which PRs the completed run targeted. Fork runs - // backfill pull_requests empty, so fall back to the exact head - // owner, branch, and SHA lookup used by the resolve job of - // label-pr-review-state.yml. Keep both copies in sync. + // GitHub reports the base repository as head_repository on + // every pull_request_review run, fork PR or not (issue + // #2133), so the payload cannot tell fork from same-repo + // runs. Dispatching for a same-repo PR is idempotent: the + // reconciliation writes the same labels, and its completion + // carries event 'workflow_dispatch', which the guard above + // filters out. + // + // Resolve which PRs the completed run targeted. Review runs + // backfill pull_requests empty for fork PRs, and the head + // owner is not recoverable from head_repository, so the + // fallback lists open PRs and matches the exact head branch, + // SHA, and base repository. The workflow_run branch of + // label-pr-review-state.yml keeps an owner-and-branch query + // instead: its runs are CI runs triggered by pull_request, + // whose head_repository is the real fork. let prNumbers = []; if (run.pull_requests?.length > 0) { prNumbers = run.pull_requests.map(pr => pr.number); - } else { - const headOwner = run.head_repository?.owner?.login; - if (headOwner && run.head_branch && run.head_sha) { - const candidates = await github.paginate(github.rest.pulls.list, { - owner, - repo, - state: 'open', - head: `${headOwner}:${run.head_branch}`, - per_page: 100, - }); - const baseRepository = `${owner}/${repo}`.toLowerCase(); - prNumbers = candidates - .filter(pr => - pr.head?.sha === run.head_sha && - pr.base?.repo?.full_name?.toLowerCase() === baseRepository - ) - .map(pr => pr.number); - core.info( - `Resolved workflow_run ${run.id ?? '(unknown)'} to ` + - `${prNumbers.length} PR(s) by exact head owner, branch, and SHA` - ); - } + } else if (run.head_branch && run.head_sha) { + const candidates = await github.paginate(github.rest.pulls.list, { + owner, + repo, + state: 'open', + per_page: 100, + }); + const baseRepository = `${owner}/${repo}`.toLowerCase(); + prNumbers = candidates + .filter(pr => + pr.head?.sha === run.head_sha && + pr.head?.ref === run.head_branch && + pr.base?.repo?.full_name?.toLowerCase() === baseRepository + ) + .map(pr => pr.number); + core.info( + `Resolved workflow_run ${run.id ?? '(unknown)'} to ` + + `${prNumbers.length} PR(s) by exact head branch, SHA, and base repository` + ); } // Each dispatch starts a privileged workflow_dispatch run of the diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index de57d915ea..421f3b4b3b 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -138,8 +138,13 @@ jobs: // number in the payload; sweep triggers (hourly schedule, pushes to // main) list every open PR; workflow_run events use the run's // associated PRs, with a fork-safe backfill by exact head owner, - // branch, and SHA when GitHub left pull_requests empty. Keep the - // backfill in sync with label-pr-review-state-fork-review-followup.yml. + // branch, and SHA when GitHub left pull_requests empty. The + // workflows listed in workflow_run trigger on pull_request, whose + // head_repository is the real fork, so the owner-and-branch query + // is safe here. The follow-up workflow's copy of this backfill + // matches by head branch, SHA, and base repository instead: its + // pull_request_review runs report the base repository as + // head_repository (issue #2133). let prNumbers = []; if (context.payload.pull_request?.number) { prNumbers = [context.payload.pull_request.number]; diff --git a/src/services/__tests__/fork-review-followup-workflow.test.ts b/src/services/__tests__/fork-review-followup-workflow.test.ts index 6c35158e21..b681e4d7d9 100644 --- a/src/services/__tests__/fork-review-followup-workflow.test.ts +++ b/src/services/__tests__/fork-review-followup-workflow.test.ts @@ -15,36 +15,43 @@ const workflow = parse( ) const dispatchScript = workflow.jobs.dispatch.steps[0].with.script as string -const SHA = "a".repeat(40) +// Head commit and branch of fork PR #1662 (fork-owner/Zoo-Code), the run +// documented in issue #2133. +const SHA = "029ea76b91f0c784087989470c81ade55b13c82f" +const HEAD_BRANCH = "fix/1591-openai-compatible-cache-reads" const OLD_SHA = "b".repeat(40) interface HarnessOptions { workflowRunEvent?: string - fork?: boolean associatedPrNumbers?: number[] - fallback?: "match" | "sha-mismatch" | "base-mismatch" | "none" - headRepository?: null - headSha?: string + fallback?: "match" | "sha-mismatch" | "ref-mismatch" | "base-mismatch" | "none" + headBranch?: string | null + headSha?: string | null failDispatchFor?: number } function buildForkPr() { return { - number: 1437, + number: 1662, state: "open", - head: { sha: SHA, repo: { full_name: "contributor/Zoo-Code" } }, + head: { ref: HEAD_BRANCH, sha: SHA, repo: { full_name: "fork-owner/Zoo-Code" } }, base: { ref: "main", repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, } } /** Executes the embedded github-script follow-up against deterministic GitHub API doubles. */ async function runFollowup(options: HarnessOptions = {}) { - const fork = options.fork ?? true const pr = buildForkPr() - const listPullRequests = vi.fn(async ({ state }: { state?: string }) => { + // Like the GitHub API, honor a `head` filter ("owner:branch"). A base-owner filter + // matches no fork PR, so the broken request cannot pass the regression scenario. + const listPullRequests = vi.fn(async ({ state, head }: { state?: string; head?: string }) => { if (state !== "open") return [] + if (head !== undefined && head !== `fork-owner:${HEAD_BRANCH}`) return [] if (options.fallback === "match") return [pr] if (options.fallback === "sha-mismatch") return [{ ...pr, head: { ...pr.head, sha: OLD_SHA } }] + if (options.fallback === "ref-mismatch") { + return [{ ...pr, head: { ...pr.head, ref: "some-other-branch" } }] + } if (options.fallback === "base-mismatch") { return [{ ...pr, base: { ...pr.base, repo: { full_name: "someone-else/Zoo-Code" } } }] } @@ -69,18 +76,21 @@ async function runFollowup(options: HarnessOptions = {}) { repo: { owner: "Zoo-Code-Org", repo: "Zoo-Code" }, payload: { workflow_run: { - id: 987654, + // Fields copied from workflow run 39534658962, the read-only fork + // review run for PR #1662 documented in issue #2133. Fields the + // script never reads are trimmed. GitHub reports the base + // repository as head_repository on every pull_request_review run, + // even for fork PRs, and backfills pull_requests empty for them. + id: 39534658962, event: options.workflowRunEvent ?? "pull_request_review", pull_requests: (options.associatedPrNumbers ?? []).map((number) => ({ number })), - head_repository: - options.headRepository === null - ? null - : { - owner: { login: fork ? "contributor" : "Zoo-Code-Org" }, - fork, - }, - head_branch: "feature/test", - head_sha: options.headSha ?? SHA, + head_repository: { + full_name: "Zoo-Code-Org/Zoo-Code", + owner: { login: "Zoo-Code-Org" }, + fork: false, + }, + head_branch: options.headBranch === undefined ? HEAD_BRANCH : options.headBranch, + head_sha: options.headSha === undefined ? SHA : options.headSha, }, }, } @@ -97,7 +107,7 @@ async function runFollowup(options: HarnessOptions = {}) { return { listPullRequests, createWorkflowDispatch, info: core.info, setFailed: core.setFailed } } -describe("fork review follow-up workflow (#2046, #2086)", () => { +describe("fork review follow-up workflow (#2046, #2086, #2133)", () => { it("listens to the label workflow without listing itself", () => { expect(workflow.name).toBe("Label PR review state fork review follow-up") expect(workflow.on.workflow_run.workflows).toEqual(["Label PR review state"]) @@ -121,26 +131,30 @@ describe("fork review follow-up workflow (#2046, #2086)", () => { }) it("dispatches a privileged reconciliation for the fork PR a read-only review run targeted", async () => { - // Fork runs backfill pull_requests empty, so the follow-up resolves the - // PR by exact head owner, branch, and SHA. + // Fork runs backfill pull_requests empty, and head_repository is the + // base repository (issue #2133), so the follow-up resolves the PR by + // exact head branch, SHA, and base repository. const result = await runFollowup({ associatedPrNumbers: [], fallback: "match" }) - expect(result.listPullRequests).toHaveBeenCalledWith( - expect.objectContaining({ head: "contributor:feature/test", state: "open" }), - ) + expect(result.listPullRequests).toHaveBeenCalledWith({ + owner: "Zoo-Code-Org", + repo: "Zoo-Code", + state: "open", + per_page: 100, + }) expect(result.createWorkflowDispatch).toHaveBeenCalledTimes(1) expect(result.createWorkflowDispatch).toHaveBeenCalledWith({ owner: "Zoo-Code-Org", repo: "Zoo-Code", workflow_id: "label-pr-review-state.yml", ref: "main", - inputs: { pull_request_number: "1437" }, + inputs: { pull_request_number: "1662" }, }) expect(result.setFailed).not.toHaveBeenCalled() }) it("dispatches directly for runs whose pull_requests are populated", async () => { - const result = await runFollowup({ associatedPrNumbers: [1437] }) + const result = await runFollowup({ associatedPrNumbers: [1662] }) expect(result.listPullRequests).not.toHaveBeenCalled() expect(result.createWorkflowDispatch).toHaveBeenCalledTimes(1) @@ -169,18 +183,28 @@ describe("fork review follow-up workflow (#2046, #2086)", () => { } }) - it("ignores a same-repository review run, which already wrote its labels", async () => { - const result = await runFollowup({ fork: false, associatedPrNumbers: [1437] }) + it("dispatches for a same-repository review run, whose payload is indistinguishable from a fork run", async () => { + // Every pull_request_review run reports head_repository as the base + // repository with fork: false (issue #2133), so fork-ness cannot gate + // the dispatch. Same-repo PRs appear in pull_requests, but resolving + // through the fallback works for them too, and the dispatch is + // idempotent: the reconciliation writes the same labels. + const result = await runFollowup({ fallback: "match" }) - expect(result.listPullRequests).not.toHaveBeenCalled() - expect(result.createWorkflowDispatch).not.toHaveBeenCalled() + expect(result.createWorkflowDispatch).toHaveBeenCalledTimes(1) + expect(result.createWorkflowDispatch).toHaveBeenCalledWith( + expect.objectContaining({ inputs: { pull_request_number: "1662" } }), + ) + expect(result.setFailed).not.toHaveBeenCalled() }) - it("dispatches nothing when no open PR matches the run's exact head", async () => { - const result = await runFollowup({ fallback: "sha-mismatch" }) + it("dispatches nothing when no open PR matches the run's exact head branch and SHA", async () => { + for (const fallback of ["sha-mismatch", "ref-mismatch", "none"] as const) { + const result = await runFollowup({ fallback }) - expect(result.listPullRequests).toHaveBeenCalledTimes(1) - expect(result.createWorkflowDispatch).not.toHaveBeenCalled() + expect(result.listPullRequests).toHaveBeenCalledTimes(1) + expect(result.createWorkflowDispatch).not.toHaveBeenCalled() + } }) it("dispatches nothing when the matching PR targets another base repository", async () => { @@ -190,8 +214,8 @@ describe("fork review follow-up workflow (#2046, #2086)", () => { expect(result.createWorkflowDispatch).not.toHaveBeenCalled() }) - it("dispatches nothing when the run has no head repository or head SHA to resolve", async () => { - for (const options of [{ headRepository: null }, { headSha: "" }] as const) { + it("dispatches nothing when the run has no head branch or head SHA to resolve", async () => { + for (const options of [{ headBranch: null }, { headSha: null }] as const) { const result = await runFollowup({ ...options, fallback: "match" }) expect(result.listPullRequests).not.toHaveBeenCalled()