From cdc247d7e181d05b9449c0ef6bfadbea8c7b3af1 Mon Sep 17 00:00:00 2001 From: "roomote[bot]" Date: Fri, 18 Sep 2026 03:14:22 +0000 Subject: [PATCH 1/2] fix(ci): preserve awaiting-author until maintainer re-review (#1671) --- .github/workflows/label-pr-review-state.yml | 86 +++- .../pr-review-state-workflow.test.ts | 389 +++++++++++++++++- 2 files changed, 467 insertions(+), 8 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 0df64ffd2b..e632301826 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -14,7 +14,7 @@ on: # This workflow only reads PR metadata and never checks out or executes PR code. # pull_request_target gives fork PRs a token that can update labels and comments. pull_request_target: - types: [opened, reopened, ready_for_review, synchronize, review_requested, labeled, unlabeled] + types: [opened, reopened, ready_for_review, synchronize, review_requested, review_request_removed, labeled, unlabeled] pull_request_review: types: [submitted, dismissed] # Fork review events have a read-only token. CodeRabbit's status-comment update @@ -339,7 +339,7 @@ jobs: 'coderabbit-changes': 'Address automated review findings and push fixes.', coderabbit: 'Required CI passed. Waiting for automated review of the latest commit.', 'draft-approved': 'Automated review complete for the latest commit. Mark the draft ready.', - 'maintainer-changes': 'Address maintainer or CODEOWNER feedback, then push an update.', + 'maintainer-changes': 'Address maintainer or CODEOWNER feedback, push an update, then re-request review from the blocking maintainer.', maintainer: 'Awaiting fresh human maintainer or CODEOWNER approval.', approved: 'The required review sequence passed. Remaining merge requirements apply.', }; @@ -641,6 +641,81 @@ jobs: } } + // Durable per-maintainer change-request blockers (issue #1671). + // Unlike approvals and CodeRabbit reviews, a human maintainer's + // CHANGES_REQUESTED stays binding across author pushes, base-branch + // merges, CI runs, and CodeRabbit reviews until that same + // maintainer's blocker is cleared by one of: + // 1. the PR author explicitly re-requesting review from them, + // 2. a newer review from that maintainer (its state decides), or + // 3. GitHub dismissing the blocking review. + // Latest state per reviewer is keyed by review id (monotonically + // increasing) so reordered or duplicate history cannot change the + // result. COMMENTED reviews are neutral and never clear a blocker; + // a DISMISSED latest review clears it. + const latestHumanReview = new Map(); + for (const r of reviews) { + const reviewer = r.user?.login?.toLowerCase(); + if (!reviewer || + r.user?.type === 'Bot' || + codeRabbitLogins.has(reviewer) || + reviewer === pr.user?.login?.toLowerCase() || + r.state === 'COMMENTED') { + continue; + } + const previous = latestHumanReview.get(reviewer); + if (!previous || r.id > previous.id) { + latestHumanReview.set(reviewer, r); + } + } + const maintainerBlockers = new Map(); + for (const [reviewer, review] of latestHumanReview) { + if (review.state !== 'CHANGES_REQUESTED') continue; + if (['admin', 'maintain', 'write'].includes(await permissionFor(review.user.login))) { + maintainerBlockers.set(reviewer, review); + } + } + + // Clear blockers the author explicitly re-requested. Only a + // review_requested timeline event whose actor is the PR author and + // whose requested reviewer is the blocking maintainer clears that + // maintainer's blocker. Team requests carry no requested_reviewer + // and never clear an individual blocker; review_request_removed + // events only trigger reconciliation and are not clearing evidence. + // If the timeline cannot be reconstructed, fail closed: keep every + // blocker so awaiting-author is preserved. + if (maintainerBlockers.size > 0) { + let timelineEvents = null; + try { + timelineEvents = await github.paginate(github.rest.issues.listEventsForTimeline, { + owner, repo, issue_number: pr.number, per_page: 100, + }); + } catch (error) { + core.warning( + `PR #${pr.number}: could not reconstruct review-request history; ` + + `preserving maintainer blockers: ${error.message}` + ); + } + if (timelineEvents) { + const authorLogin = pr.user?.login?.toLowerCase(); + for (const event of timelineEvents) { + if (event.event !== 'review_requested') continue; + if (event.actor?.login?.toLowerCase() !== authorLogin) continue; + const requested = event.requested_reviewer?.login?.toLowerCase(); + if (!requested) continue; + const blocker = maintainerBlockers.get(requested); + if (!blocker) continue; + const requestedAt = Date.parse(event.created_at ?? ''); + const blockedAt = Date.parse(blocker.submitted_at ?? ''); + // A re-request only clears blockers it follows; missing or + // unparsable timestamps fail closed and keep the blocker. + if (!Number.isNaN(requestedAt) && !Number.isNaN(blockedAt) && requestedAt >= blockedAt) { + maintainerBlockers.delete(requested); + } + } + } + } + const codeRabbitReview = latest.get(codeRabbitLogin); const freshCodeRabbitReview = codeRabbitReview?.commit_id === pr.head.sha ? codeRabbitReview @@ -657,9 +732,6 @@ jobs: freshMaintainerReviews.push(review); } } - const maintainerChangeRequest = freshMaintainerReviews.find( - review => review.state === 'CHANGES_REQUESTED' - ); const automatedAuthor = pr.user?.type === 'Bot'; const codeRabbitEligibleAuthor = !automatedAuthor || codeRabbitEligibleBotLogins.has(pr.user?.login.toLowerCase()); @@ -676,7 +748,7 @@ jobs: let phase; let activateCodeRabbit = false; let recycleCodeRabbitLabel = false; - if (codeRabbitChangesRequested || maintainerChangeRequest) { + if (codeRabbitChangesRequested || maintainerBlockers.size > 0) { desiredLabel = 'awaiting-author'; phase = codeRabbitChangesRequested ? 'coderabbit-changes' : 'maintainer-changes'; } else if (!codeRabbitEligibleAuthor) { @@ -740,7 +812,7 @@ jobs: core.info( `PR #${pr.number}: CI passing, reviews=${latest.size}, ` + `coderabbit=${freshCodeRabbitReview?.state ?? (codeRabbitEligibleAuthor ? 'pending' : 'optional')}, ` + - `maintainer=${maintainerApproval?.state ?? 'pending'} → ${desiredLabel ?? '(none)'}` + `maintainer=${maintainerApproval?.state ?? 'pending'}, blockers=${maintainerBlockers.size} → ${desiredLabel ?? '(none)'}` ); const readyForMaintainer = phase === 'maintainer' || phase === 'approved'; diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index b7329719c6..ac2c7c98d7 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -54,7 +54,16 @@ interface HarnessOptions { state: ReviewState submittedAt: number commitId?: string + id?: number + }> + timelineEvents?: Array<{ + event: string + actor?: string + requestedReviewer?: string + requestedTeam?: string + createdAt?: number }> + timelineErrorStatus?: number permissions?: Record permissionErrorStatus?: number requiredContexts?: string[] @@ -141,7 +150,7 @@ async function runWorkflow(options: HarnessOptions = {}) { : []), ] const reviews = (options.reviews ?? []).map((review, index) => ({ - id: index + 1, + id: review.id ?? index + 1, state: review.state, commit_id: review.commitId ?? SHA, submitted_at: new Date(review.submittedAt).toISOString(), @@ -210,6 +219,19 @@ async function runWorkflow(options: HarnessOptions = {}) { } return existingComments }) + const listEventsForTimeline = vi.fn(async () => { + if (options.timelineErrorStatus) { + throw Object.assign(new Error("List timeline events failed"), { status: options.timelineErrorStatus }) + } + return (options.timelineEvents ?? []).map((event, index) => ({ + id: index + 1, + event: event.event, + created_at: new Date(event.createdAt ?? REVIEWED_AT).toISOString(), + actor: event.actor ? { login: event.actor } : undefined, + requested_reviewer: event.requestedReviewer ? { login: event.requestedReviewer } : undefined, + requested_team: event.requestedTeam ? { slug: event.requestedTeam } : undefined, + })) + }) const createCommitStatus = vi.fn( async (args: { sha: string; state: string; context: string; description: string; target_url: string }) => { if (options.createCommitStatusErrorStatus) { @@ -295,6 +317,7 @@ async function runWorkflow(options: HarnessOptions = {}) { removeLabel, addLabels, listComments, + listEventsForTimeline, createComment, updateComment, }, @@ -393,6 +416,7 @@ async function runWorkflow(options: HarnessOptions = {}) { createComment, updateComment, listComments, + listEventsForTimeline, createCommitStatus, createLabel, setFailed, @@ -1829,4 +1853,367 @@ describe("PR review-state workflow", () => { expect(result.addLabels).not.toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) expect(latestGateStatus(result)?.state).toBe("pending") }) + + describe("maintainer change-request blockers (#1671)", () => { + const coderabbitApproval = { + login: "coderabbitai[bot]", + type: "Bot" as const, + state: "APPROVED" as const, + submittedAt: REVIEWED_AT, + } + const staleMaintainerChangeRequest = { + login: "maintainer", + type: "User" as const, + state: "CHANGES_REQUESTED" as const, + submittedAt: REVIEWED_AT + 1_000, + commitId: OLD_SHA, + } + const authorReRequest = { + event: "review_requested", + actor: "contributor", + requestedReviewer: "maintainer", + createdAt: REVIEWED_AT + 2_000, + } + + it("reconciles on review_request_removed without treating removal as clearing evidence", async () => { + expect(workflow.on.pull_request_target.types).toContain("review_request_removed") + + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [coderabbitApproval, staleMaintainerChangeRequest], + timelineEvents: [ + { + event: "review_request_removed", + actor: "contributor", + requestedReviewer: "maintainer", + createdAt: REVIEWED_AT + 2_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(result.setFailed).not.toHaveBeenCalled() + }) + + it("keeps awaiting-author after an author push without a re-request", async () => { + const result = await runWorkflow({ + labels: ["awaiting-author"], + permissions: { maintainer: "write" }, + reviews: [coderabbitApproval, staleMaintainerChangeRequest], + }) + + expect(result.addLabels).not.toHaveBeenCalledWith( + expect.objectContaining({ labels: ["awaiting-maintainer"] }), + ) + expect(result.removeLabel).not.toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-author" })) + expect(latestGuide(result)).toContain("re-request review") + expect(latestGateStatus(result)?.state).toBe("pending") + }) + + it("keeps the blocker after a base-only merge", async () => { + const result = await runWorkflow({ + eventName: "push", + labels: ["awaiting-author"], + permissions: { maintainer: "write" }, + reviews: [coderabbitApproval, staleMaintainerChangeRequest], + }) + + expect(result.removeLabel).not.toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-author" })) + expect(result.addLabels).not.toHaveBeenCalledWith( + expect.objectContaining({ labels: ["awaiting-maintainer"] }), + ) + }) + + it("does not clear a human blocker with a current-head CodeRabbit approval", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [coderabbitApproval, staleMaintainerChangeRequest], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + }) + + it("does not clear the blocker when another maintainer approves", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write", approver: "admin" }, + reviews: [ + coderabbitApproval, + staleMaintainerChangeRequest, + { + login: "approver", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT + 2_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(latestGateStatus(result)?.state).toBe("pending") + }) + + it("clears the blocker when the author re-requests review from the blocking maintainer", async () => { + const result = await runWorkflow({ + labels: ["awaiting-author"], + permissions: { maintainer: "write" }, + reviews: [coderabbitApproval, staleMaintainerChangeRequest], + timelineEvents: [authorReRequest], + }) + + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-author" })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + expect(latestGateStatus(result)?.state).toBe("success") + }) + + it("clears the blocker when the blocking maintainer submits a newer approval", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [ + coderabbitApproval, + staleMaintainerChangeRequest, + { + login: "maintainer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT + 2_000, + }, + ], + }) + + expect(result.addLabels).not.toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(latestGateStatus(result)?.state).toBe("success") + expect(latestGateStatus(result)?.description).toContain("required review sequence passed") + }) + + it("keeps the blocker when the blocking maintainer only comments", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [ + coderabbitApproval, + staleMaintainerChangeRequest, + { + login: "maintainer", + type: "User", + state: "COMMENTED", + submittedAt: REVIEWED_AT + 2_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + }) + + it("re-blocks when the maintainer's newer review also requests changes", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [ + coderabbitApproval, + staleMaintainerChangeRequest, + { + login: "maintainer", + type: "User", + state: "CHANGES_REQUESTED", + submittedAt: REVIEWED_AT + 3_000, + }, + ], + timelineEvents: [authorReRequest], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + }) + + it("clears the blocker when the blocking review is dismissed", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [ + coderabbitApproval, + { + login: "maintainer", + type: "User", + state: "DISMISSED", + submittedAt: REVIEWED_AT + 1_000, + commitId: OLD_SHA, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + expect(result.addLabels).not.toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + }) + + it("keeps multiple maintainer blockers independent", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write", "second-maintainer": "maintain" }, + reviews: [ + coderabbitApproval, + staleMaintainerChangeRequest, + { + login: "second-maintainer", + type: "User", + state: "CHANGES_REQUESTED", + submittedAt: REVIEWED_AT + 1_500, + commitId: OLD_SHA, + }, + ], + timelineEvents: [authorReRequest], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(latestGateStatus(result)?.state).toBe("pending") + }) + + it("produces the same blocker state for reordered and duplicate timeline events", async () => { + const reordered = await runWorkflow({ + labels: ["awaiting-author"], + permissions: { maintainer: "write" }, + reviews: [coderabbitApproval, staleMaintainerChangeRequest], + timelineEvents: [ + authorReRequest, + { + event: "review_requested", + actor: "contributor", + requestedReviewer: "maintainer", + createdAt: REVIEWED_AT + 2_000, + }, + { + event: "review_request_removed", + actor: "maintainer", + requestedReviewer: "maintainer", + createdAt: REVIEWED_AT + 3_000, + }, + authorReRequest, + ], + }) + + expect(reordered.addLabels).toHaveBeenCalledWith( + expect.objectContaining({ labels: ["awaiting-maintainer"] }), + ) + expect(reordered.setFailed).not.toHaveBeenCalled() + }) + + it("produces the same blocker state for reordered review history", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [ + { + id: 3, + login: "maintainer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT + 2_000, + }, + { ...staleMaintainerChangeRequest, id: 2 }, + { ...coderabbitApproval, id: 1 }, + ], + }) + + expect(result.addLabels).not.toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(latestGateStatus(result)?.state).toBe("success") + }) + + it("fails closed when timeline reconstruction is incomplete", async () => { + const result = await runWorkflow({ + labels: ["awaiting-author"], + permissions: { maintainer: "write" }, + timelineErrorStatus: 500, + reviews: [coderabbitApproval, staleMaintainerChangeRequest], + }) + + expect(result.warning).toHaveBeenCalledWith( + expect.stringContaining("could not reconstruct review-request history"), + ) + expect(result.removeLabel).not.toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-author" })) + expect(result.addLabels).not.toHaveBeenCalledWith( + expect.objectContaining({ labels: ["awaiting-maintainer"] }), + ) + expect(result.setFailed).not.toHaveBeenCalled() + }) + + it("does not clear an individual blocker for a team review request", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [coderabbitApproval, staleMaintainerChangeRequest], + timelineEvents: [ + { + event: "review_requested", + actor: "contributor", + requestedTeam: "maintainers", + createdAt: REVIEWED_AT + 2_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + }) + + it("does not clear the blocker when a non-author re-requests review", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write", "other-maintainer": "write" }, + reviews: [coderabbitApproval, staleMaintainerChangeRequest], + timelineEvents: [ + { + event: "review_requested", + actor: "other-maintainer", + requestedReviewer: "maintainer", + createdAt: REVIEWED_AT + 2_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + }) + + it("does not clear the blocker for a re-request that predates the blocking review", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [coderabbitApproval, staleMaintainerChangeRequest], + timelineEvents: [ + { + event: "review_requested", + actor: "contributor", + requestedReviewer: "maintainer", + createdAt: REVIEWED_AT + 500, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + }) + + it("does not clear the blocker for a re-request naming a different reviewer", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write", "other-maintainer": "write" }, + reviews: [coderabbitApproval, staleMaintainerChangeRequest], + timelineEvents: [ + { + event: "review_requested", + actor: "contributor", + requestedReviewer: "other-maintainer", + createdAt: REVIEWED_AT + 2_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + }) + + it("ignores blockers from non-collaborator reviewers", async () => { + const result = await runWorkflow({ + reviews: [ + coderabbitApproval, + { + login: "drive-by-reviewer", + type: "User", + state: "CHANGES_REQUESTED", + submittedAt: REVIEWED_AT + 1_000, + commitId: OLD_SHA, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + expect(result.addLabels).not.toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(result.setFailed).not.toHaveBeenCalled() + }) + }) }) From f105a0ba7c502c9c11a8bfdc7fd83547bf0834a1 Mon Sep 17 00:00:00 2001 From: "roomote[bot]" Date: Fri, 18 Sep 2026 03:51:20 +0000 Subject: [PATCH 2/2] perf(ci): memoize collaborator permission lookups in review-state workflow --- .github/workflows/label-pr-review-state.yml | 12 +++++++++++- .../pr-review-state-workflow.test.ts | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index e632301826..98f7428c7a 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -316,14 +316,24 @@ jobs: return match?.[1] ?? null; } + // Collaborator permissions are repository-level, so memoize them for + // the whole run: a maintainer's current-head CHANGES_REQUESTED reaches + // both review loops, and scheduled sweeps reconcile every open PR. + const permissionCache = new Map(); async function permissionFor(username) { + const key = username.toLowerCase(); + if (permissionCache.has(key)) return permissionCache.get(key); try { const result = await github.rest.repos.getCollaboratorPermissionLevel({ owner, repo, username, }); + permissionCache.set(key, result.data.permission); return result.data.permission; } catch (error) { - if (error.status === 404) return 'none'; + if (error.status === 404) { + permissionCache.set(key, 'none'); + return 'none'; + } throw error; } } diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index ac2c7c98d7..9cb178f774 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -425,6 +425,7 @@ async function runWorkflow(options: HarnessOptions = {}) { getPullRequest, listPullRequests: github.rest.pulls.list, listCommitStatusesForRef: github.rest.repos.listCommitStatusesForRef, + permissionFor, } } @@ -2197,6 +2198,24 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) }) + it("memoizes collaborator permission lookups across both review loops", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [ + coderabbitApproval, + { + login: "maintainer", + type: "User", + state: "CHANGES_REQUESTED", + submittedAt: REVIEWED_AT + 1_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(result.permissionFor.mock.calls.filter(([args]) => args.username === "maintainer")).toHaveLength(1) + }) + it("ignores blockers from non-collaborator reviewers", async () => { const result = await runWorkflow({ reviews: [