From 158635198e8fc8b158c61935ac3dd972d2f32272 Mon Sep 17 00:00:00 2001 From: easonLiangWorldedtech Date: Tue, 6 Oct 2026 00:50:32 +0800 Subject: [PATCH 01/23] 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/23] 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/23] 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() From 4d2365d52fb8e75f347074231015f329915c0fc6 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Thu, 5 Nov 2026 03:15:36 +0800 Subject: [PATCH 23/23] test(checkpoints): stop the checkpoint suite from outrunning its hook budget on Windows platform-unit-test (windows-latest) failed test:services with 5 failures in ShadowCheckpointService.spec.ts, every one of them a hook timeout in the shared beforeEach. One setup there spawns about 14 git subprocesses (workspace repo init plus shadow repo init), and the ~40 tests that share those hooks never released the repositories they created, so the shared temp dir carried one git repository pair per test until the afterAll. vitest.config.ts gives this lane two workers only on Windows CI, and every repository that is still on disk is extra cost on every spawn, which is what pushed the hook past the 20s hookTimeout there and nowhere else. Release each test's pair in the afterEach, the way the standalone tests in this file already release their own scratch repos, and give the setup and cleanup hooks the same 60s allowance the afterAll cleanup already carries. Nothing was skipped and no suppression was added. Two tests pin the release: a marker written into the repository the hooks created has to be gone before the next test runs. With the two rmScratch calls commented out the suite reports 1 failed | 43 passed (44) and the failure reads "AssertionError: expected true to be false // Object.is equality. With them restored the suite reports 44 passed. --- .../__tests__/ShadowCheckpointService.spec.ts | 45 +++++++++++++++++-- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts index 1da478900c..e2486edc0b 100644 --- a/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts +++ b/src/services/checkpoints/__tests__/ShadowCheckpointService.spec.ts @@ -41,6 +41,15 @@ afterAll(() => { } }) +// Each test gets its own shadow + workspace repo, so the shared temp dir would otherwise +// hold one git repository pair per test until the afterAll. Removing them as soon as the test +// ends is what keeps the git-heavy setup below inside its hook budget on Windows CI workers, +// where the retry absorbs the window in which git's own handles and antivirus scans still +// hold the just-used repository. +const rmScratch = async (dir: string) => { + await fs.rm(dir, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }) +} + const initWorkspaceRepo = async ({ workspaceDir, userName = "Roo Code", @@ -83,10 +92,16 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( let workspaceGit: SimpleGit let testFile: string let service: RepoPerTaskCheckpointService + let shadowDir: string + let workspaceDir: string + // One test setup spawns about 14 git subprocesses: workspace repo init plus shadow repo + // init. On a Windows CI worker, where vitest.config.ts gives this lane two workers and + // every spawn is scanned, that can outrun the 20s hook budget from vitest.config.ts, so + // setup and cleanup get the same 60s allowance the afterAll cleanup already carries. beforeEach(async () => { - const shadowDir = path.join(tmpDir, `${prefix}-${Date.now()}`) - const workspaceDir = path.join(tmpDir, `workspace-${Date.now()}`) + shadowDir = path.join(tmpDir, `${prefix}-${Date.now()}`) + workspaceDir = path.join(tmpDir, `workspace-${Date.now()}`) const repo = await initWorkspaceRepo({ workspaceDir }) workspaceGit = repo.git @@ -94,11 +109,17 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( service = await klass.create({ taskId, shadowDir, workspaceDir, log: () => {} }) await service.initShadowGit() - }) + }, 60_000) afterEach(async () => { vitest.restoreAllMocks() - }) + + // The standalone tests below already delete the scratch repos they create; this hook + // does the same for the pair created above, so the temp dir never accumulates one + // repository pair per test for the ~40 tests that share these hooks. + await rmScratch(shadowDir) + await rmScratch(workspaceDir) + }, 60_000) afterAll(async () => { await fs.rm(tmpDir, { recursive: true, force: true }) @@ -1166,6 +1187,22 @@ describe.each([[RepoPerTaskCheckpointService, "RepoPerTaskCheckpointService"]])( } }) }) + + describe(`${klass.name}#scratchCleanup`, () => { + let markedFile: string + + it("writes a marker into the repository its hooks created", async () => { + markedFile = path.join(workspaceDir, "scratch-marker.txt") + + await fs.writeFile(markedFile, "written by the marker test") + + expect(await fileExistsAtPath(markedFile)).toBe(true) + }) + + it("removes the marker with the repository before the next test runs", async () => { + expect(await fileExistsAtPath(markedFile)).toBe(false) + }) + }) }, )