Skip to content

[Fix] Review-state label on fork PRs does not update after a new review #2134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zoomote wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[Fix] Review-state label on fork PRs does not update after a new review #2134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd03f21
fix(ci): dispatch fork review follow-up when head_repository is the b…
roomote Oct 21, 2026
f1f9632
test: reject incorrect head filters in fork review regression
edelauna Oct 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 35 additions & 34 deletions .github/workflows/label-pr-review-state-fork-review-followup.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions .github/workflows/label-pr-review-state.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
98 changes: 61 additions & 37 deletions src/services/__tests__/fork-review-followup-workflow.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" } } }]
}
Expand All @@ -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,
},
},
}
Expand All @@ -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"])
Expand All @@ -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)
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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()
Expand Down
Loading