From 158635198e8fc8b158c61935ac3dd972d2f32272 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 00:50:32 +0800 Subject: [PATCH 01/22] feat(label): mark stacked pull requests so reviewers can filter them A stacked unit's head commit has exactly one parent, and that parent is another open pull request's head. The reconcile workflow already reads PR metadata, so it can identify that relationship and apply a `stacked` label, which lets reviewers filter the PR list to standalone PRs. The label is orthogonal to the review-state labels, so it is reconciled separately and never removed by reconcileLabels. A stale label is removed when the parent is no longer another open PR head. The open-PR map is read only when there is a PR to reconcile, so an unassociated workflow run still resolves to zero reads. A commit lookup failure is an advisory, not a failure. This is the labelling half of #1923; the measurement half is #1924. --- .github/workflows/label-pr-review-state.yml | 54 ++++++++++++++++ .../pr-review-state-workflow.test.ts | 64 +++++++++++++++++-- 2 files changed, 113 insertions(+), 5 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 08ecf2e3fe..f9fb20c2fd 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -33,6 +33,8 @@ permissions: issues: write checks: read statuses: write + # Reading a commit's parents is metadata only; no PR code is checked out or executed. + contents: read concurrency: group: label-pr-review-state @@ -86,6 +88,11 @@ jobs: color: 'c2e0c6', description: 'Fresh community approval on the current head; maintainer review still required', }, + { + name: 'stacked', + color: 'c2c2c2', + description: 'Head commit sits on another open PR, so its diff is only this unit', + }, ]; const guideMarker = ''; const codeRabbitLabelMarkerPrefix = ''; diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index cdf4e07e61..4b888efaf5 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -1082,7 +1082,7 @@ describe("PR review-state workflow", () => { expect.objectContaining({ name: "stacked", color: "c2c2c2", - description: "Head commit sits on another open PR, so its diff is only this unit", + description: "Head commit also sits on another open PR (detected from the commit graph)", }), ) }) @@ -2713,8 +2713,11 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) - it.each(["opened", "reopened", "synchronize"])( - "reads the head map on a %s event, which can publish a head commit", + // labeled/unlabeled are here because such a run shares the PR's concurrency group: it can + // replace a pending synchronize run for the same PR, and a replacement that skipped the map + // would leave the stacked label stale until the hourly sweep. + it.each(["opened", "reopened", "synchronize", "labeled", "unlabeled"])( + "reads the head map on a %s event, which can publish a head commit or replace the run that would", async (action) => { const resolution = await runResolve({ eventName: "pull_request_target", action }) From 97cbeb76f2b2a39df8e64127080df64e702361fc Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Wed, 4 Nov 2026 05:03:21 +0800 Subject: [PATCH 21/22] fix(ci): describe the stacked label by the parent relationship it detects The stacked label description said the head commit "also sits on another open PR", but the detection rule checks that the head commit's sole parent is another open PR's head - the head commit itself is not on that PR. The description now names the parent relationship, and the workflow test that pins the exact text is updated with it. pr-review-state-workflow.test.ts passes. Documented in the review thread that the detection rule is unchanged; only the reviewer-facing text moves. --- .github/workflows/label-pr-review-state.yml | 2 +- src/services/__tests__/pr-review-state-workflow.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 8a6a45b86c..af645926b3 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -363,7 +363,7 @@ jobs: { name: 'stacked', color: 'c2c2c2', - description: 'Head commit also sits on another open PR (detected from the commit graph)', + description: 'Head commit sits directly on another open PR head (detected from the commit graph)', }, ]; const guideMarker = ''; diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 4b888efaf5..65982a5947 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -1082,7 +1082,7 @@ describe("PR review-state workflow", () => { expect.objectContaining({ name: "stacked", color: "c2c2c2", - description: "Head commit also sits on another open PR (detected from the commit graph)", + description: "Head commit sits directly on another open PR head (detected from the commit graph)", }), ) }) From 3535359cb614245436605f3551a01845b6c35dd2 Mon Sep 17 00:00:00 2001 From: Eason Liang <136036952+easonLiangWorldedtech@users.noreply.github.com> Date: Wed, 4 Nov 2026 05:05:03 +0800 Subject: [PATCH 22/22] fix(ci): read the open-PR head map for every run that targets a PR Reviews, bot comments, manual dispatches and workflow_run completions share the PR's concurrency group with synchronize runs, so a displaced synchronize used to leave its stacked label stale until the hourly sweep: those runs passed a not-evaluated sentinel instead of reading the map. The gate is gone, every targeted run reads the map (the sweep still reuses openPrsCache and pays nothing extra), and the sentinel is removed with it. Two tests pinned the old contract and now assert the read: 'reconciles only the requested PR during manual dispatch and still reads the head map' and 'reconciles the PR associated with a workflow run and reads the head map'. Control: restoring a gate that skips the read makes them fail again; pr-review-state-workflow.test.ts 246 passed with the change. --- .github/workflows/label-pr-review-state.yml | 17 +++++------- .../pr-review-state-workflow.test.ts | 27 ++++++++++--------- 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index af645926b3..302c8b1a1c 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -226,11 +226,13 @@ jobs: // // An empty output means the map was unavailable this run; shards then skip the // stacked label rather than deciding from an empty map. - const stackMayHaveChanged = - context.eventName === 'pull_request_target' && - ['opened', 'reopened', 'synchronize', 'labeled', 'unlabeled'].includes(context.payload.action); + // Every execution that targets a PR reads the map: reviews, comments, manual + // dispatches and workflow_run completions share this PR's concurrency group with + // synchronize runs, so a displaced synchronize leaves its stacked label stale unless + // the replacement run evaluates it too. The sweep already holds the list in + // `openPrsCache`, so its cost is unchanged. let openPrHeadsOutput = ''; - if (prNumbers.length > 0 && (openPrsCache || stackMayHaveChanged)) { + if (prNumbers.length > 0) { try { const openPrsForMap = openPrsCache ?? (await github.paginate(github.rest.pulls.list, { owner, repo, state: 'open', per_page: 100, @@ -246,13 +248,6 @@ jobs: } catch (error) { core.warning(`Could not read the open pull request map: ${error.message}; shards will skip the stacked label this run`); } - } else if (prNumbers.length > 0) { - // Sentinel: this event cannot change a stacked relationship, so the label was - // not evaluated rather than found unavailable. Shards skip it silently. - openPrHeadsOutput = 'not-evaluated'; - core.info( - `Open-PR head map not read: ${context.eventName} cannot change a stacked unit; shards skip the stacked label.` - ); } core.setOutput('open_pr_heads', openPrHeadsOutput); diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 65982a5947..4529215688 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -2691,20 +2691,19 @@ describe("PR review-state workflow", () => { expect(pages[0].info).toHaveBeenCalledWith(expect.stringContaining("450")) }) - it("reconciles only the requested PR during manual dispatch and does not read the head map", async () => { + it("reconciles only the requested PR during manual dispatch and still reads the head map", async () => { const resolution = await runResolve({ eventName: "workflow_dispatch", workflowDispatchPrNumber: 1437 }) - // A manual dispatch cannot publish a head commit, so it cannot change a stacked relationship: - // the paginated open-PR read is skipped and shards are told the label was not evaluated. - expect(resolution.listPullRequests).not.toHaveBeenCalled() + // A manual dispatch shares the PR's concurrency group with synchronize runs, so it can replace + // a pending run: it reads the map instead of passing a not-evaluated sentinel. + expect(resolution.listPullRequests).toHaveBeenCalledTimes(1) expect(resolution.prNumbers()).toEqual([1437]) - expect(resolution.openPrHeadsRaw()).toBe("not-evaluated") - expect(resolution.info).toHaveBeenCalledWith(expect.stringContaining("cannot change a stacked unit")) + expect(resolution.openPrHeads()).toEqual({ [SHA.toLowerCase()]: 1437 }) const result = await runWorkflow({ eventName: "workflow_dispatch", workflowDispatchPrNumber: 1437, - openPrHeadsEnv: "not-evaluated", + openPrHeadsEnv: JSON.stringify({ [SHA.toLowerCase()]: 1437 }), }) // The shard receives the sentinel through the env; only the requested PR is reconciled. @@ -2833,16 +2832,18 @@ describe("PR review-state workflow", () => { } }) - it("reconciles the PR associated with a workflow run", async () => { + it("reconciles the PR associated with a workflow run and reads the head map", async () => { const resolution = await runResolve({ eventName: "workflow_run" }) - // Associated runs need no lookup, and a workflow_run cannot publish a head commit, so the - // head map is not read here either. - expect(resolution.listPullRequests).not.toHaveBeenCalled() - expect(resolution.openPrHeadsRaw()).toBe("not-evaluated") + // A run completion shares the PR's concurrency group, so it evaluates the map too. + expect(resolution.listPullRequests).toHaveBeenCalledTimes(1) + expect(resolution.openPrHeads()).toEqual({ [SHA.toLowerCase()]: 1437 }) expect(resolution.prNumbers()).toEqual([1437]) - const result = await runWorkflow({ eventName: "workflow_run" }) + const result = await runWorkflow({ + eventName: "workflow_run", + openPrHeadsEnv: JSON.stringify({ [SHA.toLowerCase()]: 1437 }), + }) // The shard receives the map through the env; only the requested PR is reconciled. expect(result.listPullRequests).not.toHaveBeenCalled()