diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 646d3ba29a..2c8224e4fa 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -5,6 +5,9 @@ tone_instructions: >- Prioritize correctness, security, data loss, lifecycle, and regressions; avoid speculative style comments and unrelated refactors. +chat: + allow_non_org_members: false + knowledge_base: web_search: enabled: true @@ -24,6 +27,7 @@ reviews: enabled: false drafts: false auto_incremental_review: true + auto_pause_after_reviewed_commits: 0 labels: - "coderabbit-review-active" @@ -125,6 +129,7 @@ reviews: and deprioritize prose-only nits that do not affect correctness or usability. pre_merge_checks: + override_requested_reviewers_only: true custom_checks: - name: Regression evidence mode: warning diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index b0bb560d98..4127ff26c9 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -15,12 +15,19 @@ on: types: [opened, reopened, ready_for_review, synchronize, review_requested, labeled, unlabeled] pull_request_review: types: [submitted, dismissed] + # Fork review events have a read-only token. CodeRabbit's status-comment update + # provides a trusted base-repository event that can reconcile those PRs promptly. + issue_comment: + types: [created, edited] workflow_run: workflows: ["Code QA Roo Code", "E2E Tests (Mocked)", "Webview Visual Regression", "CodeQL Advanced"] types: [completed] permissions: - pull-requests: write + # This privileged workflow only reads PR/check metadata and writes issue labels, + # comments, and commit statuses. All unspecified permissions, including contents, + # are none; no fork code or configuration is checked out or executed. + pull-requests: read issues: write checks: read statuses: write @@ -32,6 +39,9 @@ concurrency: jobs: reconcile: name: Zoo Code / reconcile PR review state + if: >- + github.event_name != 'issue_comment' || + (github.event.issue.pull_request && github.event.comment.user.login == 'coderabbitai[bot]') runs-on: ubuntu-latest steps: - name: Reconcile PR review state labels @@ -52,17 +62,17 @@ jobs: { name: 'awaiting-coderabbit', color: '5319e7', - description: 'Waiting for CodeRabbit to approve the latest commit', + description: 'Waiting for automated review of the latest commit', }, { name: 'awaiting-ready', color: '1d76db', - description: 'CodeRabbit approved; waiting for the draft to be marked ready', + description: 'Automated review complete; waiting for the draft to be marked ready', }, { name: 'awaiting-maintainer', color: '0e8a16', - description: 'CodeRabbit approved; waiting for a human maintainer', + description: 'Waiting for fresh human maintainer or CODEOWNER approval', }, { name: 'coderabbit-review-active', @@ -77,12 +87,21 @@ jobs: const reviewGateName = 'Zoo Code / PR review gate'; const reconciliationCheckName = 'Zoo Code / reconcile PR review state'; + if (context.eventName === 'issue_comment' && + (!context.payload.issue?.pull_request || + context.payload.comment?.user?.login?.toLowerCase() !== codeRabbitLogin)) { + core.info('Ignoring untrusted issue comment event'); + return; + } + // When triggered by a single PR event, only reconcile that PR. // The hourly schedule and workflow_dispatch reconcile all open PRs. let prs; let eventPrNumbers = []; if (context.payload.pull_request?.number) { eventPrNumbers = [context.payload.pull_request.number]; + } else if (context.eventName === 'issue_comment') { + eventPrNumbers = [context.payload.issue.number]; } else if (context.eventName === 'workflow_dispatch') { eventPrNumbers = [Number(context.payload.inputs.pull_request_number)]; } else if (context.payload.workflow_run?.pull_requests) { @@ -305,16 +324,16 @@ jobs: function phaseMessage(phase) { const messages = { - draft: 'Mark the PR ready to start CodeRabbit after required CI passes.', + draft: 'Mark the PR ready. Required CI must pass before CodeRabbit starts.', conflict: 'Resolve the merge conflicts. The review sequence resumes after the branch is mergeable.', - 'ci-pending': 'Wait for the required CI checks to finish.', - 'ci-failed': 'Fix the failing required CI checks and push an update.', + 'ci-pending': 'Wait for required CI checks; awaiting-maintainer requires CI and automated review completion.', + 'ci-failed': 'Fix the failing required CI checks; awaiting-maintainer requires CI and automated review completion.', 'configuration-error': 'Repository rules must not require this advisory workflow\'s own gate or reconciliation job.', - 'coderabbit-changes': 'Address CodeRabbit findings and push an update. Review restarts after CI passes.', - coderabbit: 'Required CI passed. Wait for CodeRabbit to approve the latest commit.', - 'draft-approved': 'CodeRabbit approved the latest commit. Mark the draft ready.', - 'maintainer-changes': 'Address the maintainer feedback, push an update, and request another review.', - maintainer: 'Ready for human maintainer review and approval.', + '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: 'Awaiting fresh human maintainer or CODEOWNER approval.', approved: 'The required review sequence passed. Remaining merge requirements apply.', }; return messages[phase]; @@ -373,12 +392,18 @@ jobs: ? `\n${codeRabbitLabelMarkerPrefix}${pr.head.sha}${activationPending ? ':pending' : ''} -->` : ''; - return `${guideMarker}\n### Review process\n\n${authorNote}\n\n` + - '1. Required CI checks pass.\n' + - '2. The workflow starts CodeRabbit automatically.\n' + - '3. For eligible human-authored PRs, CodeRabbit reviews and approves the latest commit.\n' + - '4. A human maintainer reviews and approves after CodeRabbit.\n\n' + - `**Current step:** ${phaseMessage(phase)}${labelMarker}`; + const phaseHelp = phase === 'coderabbit-changes' + ? '\n\nAfter fixes are pushed and required CI passes, automated review restarts.' + : phase === 'coderabbit' + ? '\n\nIf automated review does not start, a maintainer must restart it.' + : phase === 'maintainer' && !automatedAuthor + ? '\n\nAutomated review is complete for the latest commit but does not replace human approval.' + : ''; + + return `${guideMarker}\n### Review status\n\n${authorNote}\n\n` + + `**Current step:** ${phaseMessage(phase)}${phaseHelp}\n\n` + + 'Review-state labels are managed by this workflow; do not edit them manually.' + + labelMarker; } async function updateReviewGuide(pr, phase, existingGuide = null, activationPending = false) { @@ -627,12 +652,12 @@ jobs: review => review.state === 'CHANGES_REQUESTED' ); const automatedAuthor = pr.user?.type === 'Bot'; - const codeRabbitApproved = freshCodeRabbitReview?.state === 'APPROVED'; + const codeRabbitReviewComplete = freshCodeRabbitReview?.state === 'APPROVED'; const codeRabbitChangesRequested = freshCodeRabbitReview?.state === 'CHANGES_REQUESTED'; const maintainerApproval = freshMaintainerReviews .filter(review => review.state === 'APPROVED') .sort((a, b) => b.id - a.id)[0]; - const maintainerApprovedAfterCodeRabbit = codeRabbitApproved && + const maintainerApprovedAfterAutomatedReview = codeRabbitReviewComplete && maintainerApproval && maintainerApproval.id > freshCodeRabbitReview.id; @@ -654,7 +679,7 @@ jobs: desiredLabel = null; phase = 'approved'; } - } else if (!codeRabbitApproved) { + } else if (!codeRabbitReviewComplete) { if (pr.draft) { desiredLabel = null; phase = 'draft'; @@ -667,7 +692,7 @@ jobs: } else if (pr.draft) { desiredLabel = 'awaiting-ready'; phase = 'draft-approved'; - } else if (!maintainerApprovedAfterCodeRabbit) { + } else if (!maintainerApprovedAfterAutomatedReview) { desiredLabel = 'awaiting-maintainer'; phase = 'maintainer'; } else { diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d6d5e3fcde..f22cf96e9a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -134,9 +134,9 @@ pnpm install ### Review Process -Ready-for-review PRs must pass required CI checks, address actionable review feedback, and receive maintainer approval. Automated review may add a guidance comment and managed state labels; contributors should follow the indicated next step rather than editing those labels directly. New commits may reset the review state for the updated code. +Ready-for-review PRs must pass required CI checks, address actionable review feedback, and receive fresh maintainer or CODEOWNER approval after automated review completes for the latest commit. Automated review may add a guidance comment and managed state labels; contributors should follow the indicated next step rather than editing those labels directly. New commits reset review state for the updated code. -Automated review supports maintainers but does not replace their judgment. Warnings are advisory unless repository policy says otherwise, and native GitHub required-check and review protections remain authoritative for merging. +Automated review supports maintainers but does not replace human approval. Outside contributors cannot direct CodeRabbit through comments; eligible reviews restart automatically after required CI passes, and maintainers handle any exceptional restart. Warnings are advisory unless repository policy says otherwise, and native GitHub required-check, CODEOWNER, and review protections remain authoritative for merging. - **Daily Triage:** Quick checks by maintainers. - **Weekly In-depth Review:** Comprehensive assessment. diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 9b93b428c2..0374eb2e37 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -10,6 +10,7 @@ const repositoryRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)) const workflow = parse( fs.readFileSync(path.join(repositoryRoot, ".github/workflows/label-pr-review-state.yml"), "utf8"), ) +const codeRabbitConfig = parse(fs.readFileSync(path.join(repositoryRoot, ".coderabbit.yaml"), "utf8")) const workflowScript = workflow.jobs.reconcile.steps[0].with.script as string const SHA = "a".repeat(40) @@ -26,6 +27,7 @@ interface HarnessOptions { mergeableState?: string fork?: boolean eventName?: string + issueCommentActor?: string workflowRunAssociated?: boolean workflowRunFallback?: "match" | "sha-mismatch" | "base-mismatch" | "none" workflowRunHeadBranch?: string @@ -331,28 +333,33 @@ async function runWorkflow(options: HarnessOptions = {}) { const payload = eventName === "schedule" ? {} - : eventName === "workflow_dispatch" - ? { inputs: { pull_request_number: String(options.workflowDispatchPrNumber ?? 1437) } } - : eventName === "workflow_run" - ? { - workflow_run: { - pull_requests: options.workflowRunAssociated === false ? [] : [{ number: 1437 }], - head_repository: - options.workflowRunMissing === "repository" - ? null - : { owner: { login: options.fork ? "contributor" : "Zoo-Code-Org" } }, - head_branch: - options.workflowRunMissing === "branch" - ? null - : (options.workflowRunHeadBranch ?? "feature/test"), - head_sha: options.workflowRunMissing === "sha" ? null : SHA, - id: 123456, - }, - } - : { - action: "ready_for_review", - pull_request: pullRequestPayload, - } + : eventName === "issue_comment" + ? { + issue: { number: 1437, pull_request: {} }, + comment: { user: { login: options.issueCommentActor ?? "coderabbitai[bot]" } }, + } + : eventName === "workflow_dispatch" + ? { inputs: { pull_request_number: String(options.workflowDispatchPrNumber ?? 1437) } } + : eventName === "workflow_run" + ? { + workflow_run: { + pull_requests: options.workflowRunAssociated === false ? [] : [{ number: 1437 }], + head_repository: + options.workflowRunMissing === "repository" + ? null + : { owner: { login: options.fork ? "contributor" : "Zoo-Code-Org" } }, + head_branch: + options.workflowRunMissing === "branch" + ? null + : (options.workflowRunHeadBranch ?? "feature/test"), + head_sha: options.workflowRunMissing === "sha" ? null : SHA, + id: 123456, + }, + } + : { + action: "ready_for_review", + pull_request: pullRequestPayload, + } const context = { eventName, repo: { owner: "Zoo-Code-Org", repo: "Zoo-Code" }, @@ -397,6 +404,24 @@ function latestGateStatus(result: Awaited>) { } describe("PR review-state workflow", () => { + it("uses supported CodeRabbit access and review controls", () => { + expect(codeRabbitConfig.chat.allow_non_org_members).toBe(false) + expect(codeRabbitConfig.reviews.pre_merge_checks.override_requested_reviewers_only).toBe(true) + expect(codeRabbitConfig.reviews.auto_review.auto_pause_after_reviewed_commits).toBe(0) + expect(codeRabbitConfig.reviews.auto_review.labels).toEqual(["coderabbit-review-active"]) + }) + + it("keeps privileged event handling metadata-only and least-privilege", () => { + expect(workflow.permissions).toEqual({ + "pull-requests": "read", + issues: "write", + checks: "read", + statuses: "write", + }) + expect(JSON.stringify(workflow.jobs.reconcile.steps)).not.toMatch(/actions\/checkout|\bpnpm\b|\bnpm\b/) + expect(workflowScript).not.toContain("@coderabbitai") + }) + it("ignores events for closed pull requests", async () => { const result = await runWorkflow({ prState: "closed" }) @@ -426,6 +451,48 @@ describe("PR review-state workflow", () => { expect(result.createCommitStatus).toHaveBeenCalled() }) + it("reconciles CodeRabbit status comments with the canonical bot identity", async () => { + expect(workflow.on.issue_comment.types).toEqual(["created", "edited"]) + expect(workflow.jobs.reconcile.if).toContain("github.event.comment.user.login == 'coderabbitai[bot]'") + + const result = await runWorkflow({ + eventName: "issue_comment", + fork: true, + labels: ["awaiting-maintainer", "coderabbit-review-active"], + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "CHANGES_REQUESTED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(result.getPullRequest).toHaveBeenCalledTimes(1) + expect(result.listPullRequests).not.toHaveBeenCalled() + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-maintainer" })) + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "coderabbit-review-active" })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(result.setFailed).not.toHaveBeenCalled() + }) + + it("ignores contributor comments before privileged reconciliation", async () => { + const result = await runWorkflow({ + eventName: "issue_comment", + issueCommentActor: "outside-contributor", + fork: true, + labels: ["awaiting-maintainer"], + }) + + expect(result.getPullRequest).not.toHaveBeenCalled() + expect(result.listPullRequests).not.toHaveBeenCalled() + expect(result.addLabels).not.toHaveBeenCalled() + expect(result.removeLabel).not.toHaveBeenCalled() + expect(result.createComment).not.toHaveBeenCalled() + expect(result.createCommitStatus).not.toHaveBeenCalled() + }) + it("reconciles fork PRs from pull_request_target", async () => { const result = await runWorkflow({ eventName: "pull_request_target", fork: true }) @@ -455,6 +522,7 @@ describe("PR review-state workflow", () => { expect(result.addLabels).not.toHaveBeenCalled() expect(latestGuide(result)).toContain("Mark the PR ready") + expect(latestGuide(result)).toContain("Review-state labels are managed by this workflow") }) it("routes bot-authored PRs directly to maintainer review", async () => { @@ -467,7 +535,7 @@ describe("PR review-state workflow", () => { expect.objectContaining({ labels: ["coderabbit-review-active"] }), ) expect(latestGateStatus(result)?.state).toBe("success") - expect(latestGateStatus(result)?.description).toContain("Ready for human maintainer") + expect(latestGateStatus(result)?.description).toContain("Awaiting fresh human maintainer") }) it("completes bot-authored PR review after human maintainer approval", async () => { @@ -527,6 +595,7 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) expect(latestGuide(result)).toContain(`coderabbit-review-label:${SHA}`) + expect(latestGuide(result)).toContain("a maintainer must restart it") }) it("invalidates the gate before fallible metadata updates", async () => { @@ -844,9 +913,13 @@ describe("PR review-state workflow", () => { expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "coderabbit-review-active" })) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(latestGuide(result)).toContain( + "After fixes are pushed and required CI passes, automated review restarts.", + ) + expect(latestGuide(result)).not.toContain("@coderabbitai") }) - it("moves approved ready PRs to maintainer review", async () => { + it("treats a fresh bot approval as automated review completion only", async () => { const result = await runWorkflow({ reviews: [ { @@ -859,8 +932,23 @@ describe("PR review-state workflow", () => { }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + expect(latestGuide(result)).toContain("Automated review is complete for the latest commit") + expect(latestGuide(result)).toContain("does not replace human approval") }) + it.each([ + ["in_progress", undefined, "Wait for required CI checks"], + ["completed", "failure", "Fix the failing required CI checks"], + ] as const)( + "explains why %s required CI blocks maintainer handoff", + async (requiredStatus, requiredConclusion, text) => { + const result = await runWorkflow({ requiredStatus, requiredConclusion }) + + expect(latestGuide(result)).toContain(text) + expect(latestGuide(result)).toContain("awaiting-maintainer") + }, + ) + it("recognizes CodeRabbit regardless of login casing", async () => { const result = await runWorkflow({ reviews: [ @@ -1137,7 +1225,7 @@ describe("PR review-state workflow", () => { { context: "Zoo Code / PR review gate", state: "pending", - description: "Required CI passed. Wait for CodeRabbit to approve the latest commit.", + description: "Required CI passed. Waiting for automated review of the latest commit.", targetUrl: "https://github.com/Zoo-Code-Org/Zoo-Code/pull/1437", }, ], @@ -1429,7 +1517,7 @@ describe("PR review-state workflow", () => { }) expect(latestGateStatus(result)?.state).toBe("success") - expect(latestGateStatus(result)?.description).toContain("Ready for human maintainer") + expect(latestGateStatus(result)?.description).toContain("Awaiting fresh human maintainer") }) it("fails closed when branch rules are unavailable", async () => { @@ -1565,8 +1653,9 @@ describe("PR review-state workflow", () => { expect(result.createCommitStatus).not.toHaveBeenCalled() }) - it("ignores CodeRabbit reviews from an older head", async () => { + it("invalidates prior automated and human reviews after a new push", async () => { const result = await runWorkflow({ + permissions: { maintainer: "write" }, reviews: [ { login: "coderabbitai[bot]", @@ -1575,9 +1664,19 @@ describe("PR review-state workflow", () => { submittedAt: REVIEWED_AT, commitId: OLD_SHA, }, + { + login: "maintainer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT + 1_000, + commitId: OLD_SHA, + }, ], }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) + expect(result.addLabels).not.toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + expect(latestGateStatus(result)?.state).toBe("pending") }) })