From 22948cf3b84f55c185073de16c8d9f0e516065bf Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 14:58:58 +0000 Subject: [PATCH 01/29] feat: require human-triggered CodeRabbit reviews --- .coderabbit.yaml | 6 +- .github/workflows/label-pr-review-state.yml | 482 +++++++++++++++++--- CONTRIBUTING.md | 15 +- 3 files changed, 428 insertions(+), 75 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 6d8232e6ac..e32968bc7a 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -17,9 +17,11 @@ reviews: poem: false auto_review: - enabled: true + enabled: false drafts: false - auto_incremental_review: true + auto_incremental_review: false + labels: + - "coderabbit-review-active" path_filters: - "!**/node_modules/**" diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 040ad91d00..69bbdeaf39 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -2,16 +2,23 @@ name: Label PR review state on: schedule: - - cron: '0 * * * *' # hourly fallback + - cron: "0 * * * *" # hourly fallback workflow_dispatch: - pull_request: - types: [opened, reopened, ready_for_review, synchronize, review_requested] + # 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] pull_request_review: types: [submitted, dismissed] + workflow_run: + workflows: ["Code QA Roo Code", "E2E Tests (Mocked)", "Webview Visual Regression", "CodeQL Advanced"] + types: [completed] permissions: pull-requests: write - checks: read + issues: write + checks: write + statuses: read concurrency: group: label-pr-review-state @@ -26,35 +33,89 @@ jobs: with: script: | const { owner, repo } = context.repo; - const stateLabels = ['awaiting-author', 'awaiting-review', 'has-conflicts']; + const stateLabels = [ + 'awaiting-author', + 'awaiting-review-trigger', + 'awaiting-coderabbit', + 'awaiting-maintainer', + 'awaiting-review', // Legacy label removed during reconciliation. + 'has-conflicts', + ]; + const labelDefinitions = [ + { + name: 'awaiting-review-trigger', + color: 'fbca04', + description: 'CI passed; waiting for a human maintainer to request CodeRabbit', + }, + { + name: 'awaiting-coderabbit', + color: '5319e7', + description: 'Waiting for CodeRabbit to approve the latest commit', + }, + { + name: 'awaiting-maintainer', + color: '0e8a16', + description: 'CodeRabbit approved; waiting for a human maintainer', + }, + { + name: 'request-coderabbit-review', + color: 'd4c5f9', + description: 'Maintainer request to start CodeRabbit review', + }, + { + name: 'coderabbit-review-active', + color: '5319e7', + description: 'Validated human request; CodeRabbit review is active', + }, + ]; + const guideMarker = ''; + const triggerMarkerPrefix = '/ + ); + if (!match || match[1] !== headSha) return null; + return { sha: match[1], actor: match[2], requestedAt: Number(match[3]) }; + } + + async function resolveAcceptedTrigger(pr, existingGuide) { + let acceptedTrigger = parseAcceptedTrigger(existingGuide, pr.head.sha); + const isRequestEvent = context.eventName === 'pull_request_target' && + context.payload.action === 'labeled' && + context.payload.label?.name === requestLabel; + + if (!isRequestEvent) return acceptedTrigger; + + if (context.payload.pull_request?.head?.sha !== pr.head.sha) { + await setControlLabel(pr, requestLabel, false); + core.warning(`PR #${pr.number}: rejected stale CodeRabbit trigger event`); + return acceptedTrigger; + } + + const actor = context.payload.sender; + let permission = 'none'; + if (actor?.type !== 'Bot' && actor?.login !== pr.user?.login) { + const result = await github.rest.repos.getCollaboratorPermissionLevel({ + owner, repo, username: actor.login, + }); + permission = result.data.permission; + } + const allowed = actor?.type !== 'Bot' && + actor?.login !== pr.user?.login && + ['admin', 'maintain', 'write'].includes(permission); + + await setControlLabel(pr, requestLabel, false); + if (!allowed) { + core.warning( + `PR #${pr.number}: rejected CodeRabbit trigger from ${actor?.login ?? 'unknown'} (${permission})` + ); + return acceptedTrigger; + } + + acceptedTrigger = { + sha: pr.head.sha, + actor: actor.login, + requestedAt: Date.now(), + }; + core.info(`PR #${pr.number}: accepted CodeRabbit trigger from ${actor.login}`); + return acceptedTrigger; + } + + function reviewGuideBody(pr, phase, acceptedTrigger) { + const automatedAuthor = pr.user?.type === 'Bot'; + const authorNote = automatedAuthor + ? 'This PR was opened by an automated account. A human maintainer must verify the change intent, provenance, and validation before merging.' + : 'Thanks for contributing. This comment tracks the review sequence and the next action.'; + + const phaseCopy = { + draft: 'Mark the PR ready for review when it is complete. CodeRabbit does not automatically review drafts.', + 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.', + trigger: 'A human maintainer must apply the `request-coderabbit-review` label to start the AI review.', + 'coderabbit-changes': 'Address CodeRabbit findings and push an update. A maintainer must request another review for the new commit.', + coderabbit: `CodeRabbit review was requested by @${acceptedTrigger?.actor}. Wait for it to approve the latest commit.`, + 'maintainer-changes': 'Address the maintainer feedback, push an update, and request another review.', + maintainer: 'CodeRabbit approved the latest commit. A human maintainer must now review and approve it.', + approved: 'CodeRabbit and a human maintainer approved the latest commit. The PR is ready for the remaining merge requirements.', + }; + + const triggerMarker = acceptedTrigger + ? `\n${triggerMarkerPrefix}${acceptedTrigger.sha}:${acceptedTrigger.actor}:${acceptedTrigger.requestedAt} -->` + : ''; + + return `${guideMarker}\n### Review process\n\n${authorNote}\n\n` + + '1. Required CI checks pass.\n' + + '2. A human maintainer explicitly requests CodeRabbit review.\n' + + '3. CodeRabbit reviews and approves the latest commit.\n' + + '4. A human maintainer reviews and approves after CodeRabbit.\n\n' + + `**Current step:** ${phaseCopy[phase]}${triggerMarker}`; + } + + async function updateReviewGuide(pr, phase, acceptedTrigger, existingGuide = null) { + if (isReadOnlyRun && isForkPR(pr)) { + core.info(`PR #${pr.number}: fork PR on a read-only run — skipping guide update`); + return; + } + + const body = reviewGuideBody(pr, phase, acceptedTrigger); + const existing = existingGuide ?? await findReviewGuide(pr); + + if (!existing) { + await github.rest.issues.createComment({ + owner, repo, issue_number: pr.number, body, + }); + } else if (existing.body !== body) { + await github.rest.issues.updateComment({ + owner, repo, comment_id: existing.id, body, + }); + } + } + // Fetch required status check names from the branch ruleset. // Uses the public /rules/branches endpoint — no admin token needed. // Falls back to blocking on all checks if the endpoint is unavailable. - let requiredCheckNames = null; + let requiredChecks = null; try { - const { data: rules } = await github.request( + const rules = await github.paginate( 'GET /repos/{owner}/{repo}/rules/branches/{branch}', - { owner, repo, branch: 'main' }, + { owner, repo, branch: 'main', per_page: 100 }, ); - const statusRule = rules.find(r => r.type === 'required_status_checks'); - if (statusRule) { - requiredCheckNames = new Set( - statusRule.parameters.required_status_checks.map(c => c.context), + const statusRules = rules.filter(rule => rule.type === 'required_status_checks'); + requiredChecks = []; + if (statusRules.length > 0) { + const uniqueChecks = new Map(); + for (const rule of statusRules) { + for (const check of rule.parameters.required_status_checks) { + const integrationId = check.integration_id ?? null; + uniqueChecks.set(`${check.context}:${integrationId ?? 'any'}`, { + context: check.context, + integrationId, + }); + } + } + requiredChecks = [...uniqueChecks.values()]; + core.info( + `Required checks: ${requiredChecks.map(check => + `${check.context}${check.integrationId ? `@${check.integrationId}` : ''}` + ).join(', ')}` ); - core.info(`Required checks: ${[...requiredCheckNames].join(', ')}`); } } catch (err) { core.warning(`Could not fetch branch rules, falling back to all checks: ${err.message}`); @@ -118,10 +355,18 @@ jobs: for (const pr of prs) { try { + const existingGuide = await findReviewGuide(pr); + let acceptedTrigger = parseAcceptedTrigger(existingGuide, pr.head.sha); + if (acceptedTrigger?.sha !== pr.head.sha) acceptedTrigger = null; + // Draft PRs never get a state label. if (pr.draft) { core.info(`PR #${pr.number}: draft — stripping state labels`); await reconcileLabels(pr, null); + await setControlLabel(pr, activeLabel, false); + await setControlLabel(pr, requestLabel, false); + await updateReviewGuide(pr, 'draft', null, existingGuide); + await updateReviewGate(pr, 'draft', false); continue; } @@ -131,25 +376,29 @@ jobs: // single-PR fetch can return `null`/"unknown" if the merge check hasn't // finished yet. Re-fetch the single PR to get a fresh value, and treat // "unknown" as not-yet-computed rather than as conflicting. - const prDetail = prNumber + const prDetail = eventPrNumbers.length > 0 ? pr : (await github.rest.pulls.get({ owner, repo, pull_number: pr.number })).data; if (prDetail.mergeable === false && prDetail.mergeable_state === 'dirty') { core.info(`PR #${pr.number}: has merge conflicts — labeling has-conflicts`); await reconcileLabels(pr, 'has-conflicts'); + await setControlLabel(pr, activeLabel, false); + await setControlLabel(pr, requestLabel, false); + await updateReviewGuide(pr, 'conflict', acceptedTrigger, existingGuide); + await updateReviewGate(pr, 'conflict', false); continue; } // Check CI status for required checks on the PR's head commit only. // Scoping to required checks avoids advisory checks (e.g. codecov/patch) // incorrectly blocking label assignment on otherwise-ready PRs. - const [checkRuns, commitStatusRes] = await Promise.all([ + const [checkRuns, commitStatuses] = await Promise.all([ github.paginate(github.rest.checks.listForRef, { owner, repo, ref: pr.head.sha, per_page: 100, }), - github.rest.repos.getCombinedStatusForRef({ - owner, repo, ref: pr.head.sha, + github.paginate(github.rest.repos.listCommitStatusesForRef, { + owner, repo, ref: pr.head.sha, per_page: 100, }), ]); @@ -165,34 +414,73 @@ jobs: // run can have started_at: null, which would lose a string comparison // against an older completed run's timestamp). const latestByName = new Map(); + const latestByNameAndApp = new Map(); for (const run of checkRuns) { const prev = latestByName.get(run.name); if (!prev || run.id > prev.id) { latestByName.set(run.name, run); } + const appKey = `${run.name}:${run.app?.id ?? 'none'}`; + const previousAppRun = latestByNameAndApp.get(appKey); + if (!previousAppRun || run.id > previousAppRun.id) { + latestByNameAndApp.set(appKey, run); + } + } + + const latestStatusByContext = new Map(); + for (const status of commitStatuses) { + const previous = latestStatusByContext.get(status.context); + if (!previous || status.id > previous.id) { + latestStatusByContext.set(status.context, status); + } } // Filter to required checks only (or all checks if rules unavailable). // Always exclude this workflow's own run to avoid self-referential loops. - const relevantRuns = [...latestByName.values()].filter(run => { - if (run.name === 'Reconcile PR review state labels') return false; - return requiredCheckNames ? requiredCheckNames.has(run.name) : true; + const excludedCheckNames = new Set([ + 'Reconcile PR review state labels', + reviewGateName, + ]); + const requiredSpecs = requiredChecks === null + ? null + : requiredChecks.filter(check => !excludedCheckNames.has(check.context)); + const requiredResults = requiredSpecs?.map(check => { + const run = check.integrationId === null + ? latestByName.get(check.context) + : latestByNameAndApp.get(`${check.context}:${check.integrationId}`); + const status = check.integrationId === null && !run + ? latestStatusByContext.get(check.context) + : null; + return { check, run, status }; }); + const relevantRuns = requiredSpecs === null + ? [...latestByName.values()].filter(run => !excludedCheckNames.has(run.name)) + : requiredResults.map(result => result.run).filter(Boolean); + const relevantStatuses = requiredSpecs === null + ? [...latestStatusByContext.values()] + : requiredResults.map(result => result.status).filter(Boolean); + const missingRequiredChecks = requiredSpecs === null + ? [] + : requiredResults + .filter(result => !result.run && !result.status) + .map(result => + `${result.check.context}${result.check.integrationId ? `@${result.check.integrationId}` : ''}` + ); - // For commit statuses (external CIs), there's no per-status name filtering - // available from getCombinedStatusForRef — it aggregates all statuses. - // If required checks are known, we only use commitStatus as a signal when - // no required check runs exist for this ref (i.e. pure status-based CI). - const useCommitStatus = !requiredCheckNames || relevantRuns.length === 0; - - core.debug(`PR #${pr.number}: ${relevantRuns.length} required check run(s), commit status=${commitStatusRes.data.state} (used=${useCommitStatus})`); + core.debug( + `PR #${pr.number}: ${relevantRuns.length} check run(s), ` + + `${relevantStatuses.length} commit status(es), ` + + `missing=[${missingRequiredChecks.join(', ')}]` + ); for (const run of relevantRuns) { core.debug(` check: "${run.name}" status=${run.status} conclusion=${run.conclusion}`); } - const ciPending = relevantRuns.some( - run => run.status === 'queued' || run.status === 'in_progress', - ) || (useCommitStatus && commitStatusRes.data.state === 'pending'); + const ciPending = requiredChecks === null || missingRequiredChecks.length > 0 || relevantRuns.some( + run => run.status !== 'completed', + ) || relevantStatuses.some( + status => status.state === 'pending' + ); const ciFailed = !ciPending && ( relevantRuns.some( @@ -200,10 +488,9 @@ jobs: run.conclusion !== 'success' && run.conclusion !== 'skipped' && run.conclusion !== 'neutral', - ) || (useCommitStatus && ( - commitStatusRes.data.state === 'failure' || - commitStatusRes.data.state === 'error' - )) + ) || relevantStatuses.some( + status => status.state === 'failure' || status.state === 'error' + ) ); // While CI is running or has failed, remove state labels and move on. @@ -211,9 +498,22 @@ jobs: if (ciPending || ciFailed) { core.info(`PR #${pr.number}: CI ${ciPending ? 'pending' : 'failed'} — stripping state labels`); await reconcileLabels(pr, null); + await setControlLabel(pr, activeLabel, false); + await setControlLabel(pr, requestLabel, false); + await updateReviewGuide( + pr, + ciPending ? 'ci-pending' : 'ci-failed', + acceptedTrigger, + existingGuide + ); + await updateReviewGate(pr, ciPending ? 'ci-pending' : 'ci-failed', false); continue; } + // A review request is accepted only after CI passes. The accepted request is + // bound to this head SHA in the managed guide comment. + acceptedTrigger = await resolveAcceptedTrigger(pr, existingGuide); + // CI is passing. Now determine review state. const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: pr.number, per_page: 100, @@ -225,44 +525,82 @@ jobs: // block the PR or indicate the author needs to act. const latest = new Map(); for (const r of reviews) { - if (r.state !== 'COMMENTED' && r.state !== 'DISMISSED') { + if (r.state === 'DISMISSED') { + latest.delete(r.user.login); + } else if (r.state !== 'COMMENTED') { latest.set(r.user.login, r); } } - const requestedReviewers = new Set( - pr.requested_reviewers.map(r => r.login), + const codeRabbitReview = latest.get(codeRabbitLogin); + const freshCodeRabbitReview = codeRabbitReview?.commit_id === pr.head.sha + ? codeRabbitReview + : null; + const freshMaintainerReviews = []; + for (const review of latest.values()) { + if (review.commit_id !== pr.head.sha || + review.user?.type === 'Bot' || + review.user?.login === pr.user?.login) { + continue; + } + const permissionResult = await github.rest.repos.getCollaboratorPermissionLevel({ + owner, repo, username: review.user.login, + }); + if (['admin', 'maintain', 'write'].includes(permissionResult.data.permission)) { + freshMaintainerReviews.push(review); + } + } + const maintainerChangeRequest = freshMaintainerReviews.find( + review => review.state === 'CHANGES_REQUESTED' ); - - const changeRequesters = [...latest.entries()] - .filter(([, r]) => r.state === 'CHANGES_REQUESTED') - .map(([login]) => login); + const codeRabbitReviewAfterTrigger = acceptedTrigger && freshCodeRabbitReview && + Date.parse(freshCodeRabbitReview.submitted_at) > acceptedTrigger.requestedAt; + const codeRabbitApproved = codeRabbitReviewAfterTrigger && + freshCodeRabbitReview.state === 'APPROVED'; + const maintainerApproval = freshMaintainerReviews + .filter(review => review.state === 'APPROVED') + .sort((a, b) => Date.parse(b.submitted_at) - Date.parse(a.submitted_at))[0]; + const maintainerApprovedAfterCodeRabbit = codeRabbitApproved && + maintainerApproval && + Date.parse(maintainerApproval.submitted_at) > Date.parse(freshCodeRabbitReview.submitted_at); let desiredLabel; - if (changeRequesters.length > 0) { - // If every change-requester has been re-requested for review, - // the author has addressed feedback and re-opened it for review. - desiredLabel = changeRequesters.every(login => requestedReviewers.has(login)) - ? 'awaiting-review' - : 'awaiting-author'; + let phase; + if (codeRabbitReviewAfterTrigger && freshCodeRabbitReview.state === 'CHANGES_REQUESTED') { + desiredLabel = 'awaiting-author'; + phase = 'coderabbit-changes'; + await setControlLabel(pr, activeLabel, false); + } else if (!acceptedTrigger) { + desiredLabel = 'awaiting-review-trigger'; + phase = 'trigger'; + await setControlLabel(pr, activeLabel, false); + } else if (!codeRabbitApproved) { + desiredLabel = 'awaiting-coderabbit'; + phase = 'coderabbit'; + await setControlLabel(pr, activeLabel, true); + } else if (maintainerChangeRequest) { + desiredLabel = 'awaiting-author'; + phase = 'maintainer-changes'; + await setControlLabel(pr, activeLabel, false); + } else if (!maintainerApprovedAfterCodeRabbit) { + desiredLabel = 'awaiting-maintainer'; + phase = 'maintainer'; + await setControlLabel(pr, activeLabel, false); } else { - // No outstanding change requests: awaiting first review, or all approved. - // awaiting-review if: there are pending requested reviewers, or nobody - // has given a meaningful review yet. null (approved) if everyone approved. - const allApproved = latest.size > 0 && - [...latest.values()].every(r => r.state === 'APPROVED') && - requestedReviewers.size === 0; - - desiredLabel = allApproved ? null : 'awaiting-review'; + desiredLabel = null; + phase = 'approved'; + await setControlLabel(pr, activeLabel, false); } core.info( `PR #${pr.number}: CI passing, reviews=${latest.size}, ` + - `changeRequesters=[${changeRequesters.join(',')}], ` + - `requestedReviewers=[${[...requestedReviewers].join(',')}] → ${desiredLabel ?? '(none)'}` + `coderabbit=${freshCodeRabbitReview?.state ?? 'pending'}, ` + + `maintainer=${maintainerApproval?.state ?? 'pending'} → ${desiredLabel ?? '(none)'}` ); await reconcileLabels(pr, desiredLabel); + await updateReviewGuide(pr, phase, acceptedTrigger, existingGuide); + await updateReviewGate(pr, phase, phase === 'approved', acceptedTrigger); } catch (error) { const detail = error.status ? `${error.message} (HTTP ${error.status}${error.response?.data?.message ? `: ${error.response.data.message}` : ''})` diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2ce81bd722..35d4c4fb8f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -134,6 +134,19 @@ pnpm install ### Review Process +Ready-for-review PRs move through these gates in order: + +1. Required CI checks must pass. +2. A human maintainer applies the `request-coderabbit-review` label to start CodeRabbit. PR-author and automated-account actions cannot satisfy this gate. +3. CodeRabbit reviews the latest commit. Address any findings and ask a maintainer to trigger a new review after pushing updates. +4. After CodeRabbit approval, a human maintainer performs the final review and approval. + +An automated comment on each PR shows the current gate and next action. The `awaiting-review-trigger`, `awaiting-coderabbit`, `awaiting-maintainer`, `awaiting-author`, and `has-conflicts` labels make the same state visible in the PR list. A new commit invalidates the trigger and approvals, so a maintainer must request CodeRabbit again before the final human review. + +The `PR review gate` check passes only after the sequence completes. Repository administrators should configure it as a required status check on `main`; the labels and comment explain the state, while the required check enforces it. + +PRs opened by bots or other automated accounts follow the same sequence. Automated authorship never substitutes for human accountability: a maintainer must verify the change's intent, provenance, and validation before merging. + - **Daily Triage:** Quick checks by maintainers. - **Weekly In-depth Review:** Comprehensive assessment. - **Iterate promptly** based on feedback. @@ -153,7 +166,7 @@ Maintainers may close PRs that are incomplete, too broad, inactive, not aligned PRs are also closed automatically by bot: - **60-day inactivity:** A PR with no activity for 60 days is marked stale and closed after a further 7 days if there is still no activity. Any new comment, commit, or review resets the timer. -- **14-day author inactivity:** After a reviewer requests changes, the PR is labelled `awaiting-author`. Author activity resets the inactivity timer. Once the changes are ready, re-request review from the reviewer; the PR will move to `awaiting-review` and is no longer eligible for automatic closure under this policy. +- **14-day author inactivity:** After CodeRabbit or a maintainer requests changes, the PR is labelled `awaiting-author`. Author activity resets the inactivity timer. After an update, the PR moves to `awaiting-review-trigger`; after a maintainer starts CodeRabbit and it approves, the PR moves to `awaiting-maintainer`. These waiting labels are not eligible for automatic closure under this policy. To opt a PR out of automatic closure, apply the `do-not-close`, `pinned`, or `work-in-progress` label. From 9fbb36854c6b2a0e324024de8571bf86b508d6e2 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 15:15:14 +0000 Subject: [PATCH 02/29] fix: allow authors to request CodeRabbit reviews --- .coderabbit.yaml | 2 - .github/workflows/label-pr-review-state.yml | 90 ++++++--------------- CONTRIBUTING.md | 8 +- 3 files changed, 27 insertions(+), 73 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index e32968bc7a..ff86c635ef 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -20,8 +20,6 @@ reviews: enabled: false drafts: false auto_incremental_review: false - labels: - - "coderabbit-review-active" path_filters: - "!**/node_modules/**" diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 69bbdeaf39..607ba9efcc 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -10,6 +10,8 @@ on: types: [opened, reopened, ready_for_review, synchronize, review_requested, labeled, unlabeled] pull_request_review: types: [submitted, dismissed] + issue_comment: + types: [created] workflow_run: workflows: ["Code QA Roo Code", "E2E Tests (Mocked)", "Webview Visual Regression", "CodeQL Advanced"] types: [completed] @@ -45,7 +47,7 @@ jobs: { name: 'awaiting-review-trigger', color: 'fbca04', - description: 'CI passed; waiting for a human maintainer to request CodeRabbit', + description: 'CI passed; waiting for a human to request CodeRabbit', }, { name: 'awaiting-coderabbit', @@ -57,36 +59,29 @@ jobs: color: '0e8a16', description: 'CodeRabbit approved; waiting for a human maintainer', }, - { - name: 'request-coderabbit-review', - color: 'd4c5f9', - description: 'Maintainer request to start CodeRabbit review', - }, - { - name: 'coderabbit-review-active', - color: '5319e7', - description: 'Validated human request; CodeRabbit review is active', - }, ]; const guideMarker = ''; const triggerMarkerPrefix = '` : ''; @@ -263,7 +293,7 @@ jobs: '2. A human explicitly requests CodeRabbit review.\n' + '3. CodeRabbit reviews and approves the latest commit.\n' + '4. A human maintainer reviews and approves after CodeRabbit.\n\n' + - `**Current step:** ${phaseCopy[phase]}${triggerMarker}`; + `**Current step:** ${phaseMessage(phase, acceptedTrigger)}${triggerMarker}`; } async function updateReviewGuide(pr, phase, acceptedTrigger, existingGuide = null) { @@ -288,7 +318,7 @@ jobs: // Fetch required status check names from the branch ruleset. // Uses the public /rules/branches endpoint — no admin token needed. - // Falls back to blocking on all checks if the endpoint is unavailable. + // Fails closed if the endpoint is unavailable. let requiredChecks = null; try { const rules = await github.paginate( @@ -316,7 +346,7 @@ jobs: ); } } catch (err) { - core.warning(`Could not fetch branch rules, falling back to all checks: ${err.message}`); + core.warning(`Could not fetch branch rules; review gate remains pending: ${err.message}`); } const failures = []; @@ -327,15 +357,6 @@ jobs: let acceptedTrigger = parseAcceptedTrigger(existingGuide, pr.head.sha); if (acceptedTrigger?.sha !== pr.head.sha) acceptedTrigger = null; - // Draft PRs never get a state label. - if (pr.draft) { - core.info(`PR #${pr.number}: draft — stripping state labels`); - await reconcileLabels(pr, null); - await updateReviewGuide(pr, 'draft', null, existingGuide); - await updateReviewGate(pr, 'draft', false); - continue; - } - // `mergeable`/`mergeable_state` are only returned by the single-PR GET // endpoint, and are computed asynchronously by GitHub — a PR fetched via // pulls.list (schedule/workflow_dispatch runs) never has them, and even a @@ -402,13 +423,12 @@ jobs: // Filter to required checks only (or all checks if rules unavailable). // Always exclude this workflow's own run to avoid self-referential loops. const excludedCheckNames = new Set([ - 'Reconcile PR review state labels', + 'reconcile', reviewGateName, ]); - const requiredSpecs = requiredChecks === null - ? null - : requiredChecks.filter(check => !excludedCheckNames.has(check.context)); - const requiredResults = requiredSpecs?.map(check => { + const requiredSpecs = (requiredChecks ?? []) + .filter(check => !excludedCheckNames.has(check.context)); + const requiredResults = requiredSpecs.map(check => { const run = check.integrationId === null ? latestByName.get(check.context) : latestByNameAndApp.get(`${check.context}:${check.integrationId}`); @@ -417,19 +437,13 @@ jobs: : null; return { check, run, status }; }); - const relevantRuns = requiredSpecs === null - ? [...latestByName.values()].filter(run => !excludedCheckNames.has(run.name)) - : requiredResults.map(result => result.run).filter(Boolean); - const relevantStatuses = requiredSpecs === null - ? [...latestStatusByContext.values()] - : requiredResults.map(result => result.status).filter(Boolean); - const missingRequiredChecks = requiredSpecs === null - ? [] - : requiredResults - .filter(result => !result.run && !result.status) - .map(result => - `${result.check.context}${result.check.integrationId ? `@${result.check.integrationId}` : ''}` - ); + const relevantRuns = requiredResults.map(result => result.run).filter(Boolean); + const relevantStatuses = requiredResults.map(result => result.status).filter(Boolean); + const missingRequiredChecks = requiredResults + .filter(result => !result.run && !result.status) + .map(result => + `${result.check.context}${result.check.integrationId ? `@${result.check.integrationId}` : ''}` + ); core.debug( `PR #${pr.number}: ${relevantRuns.length} check run(s), ` + @@ -457,6 +471,12 @@ jobs: ) ); + const ciCompletedAt = Math.max( + 0, + ...relevantRuns.map(run => Date.parse(run.completed_at ?? run.started_at)), + ...relevantStatuses.map(status => Date.parse(status.updated_at ?? status.created_at)), + ); + // While CI is running or has failed, remove state labels and move on. // CI failure is its own signal; the label would add noise, not clarity. if (ciPending || ciFailed) { @@ -474,7 +494,7 @@ jobs: // A review request is accepted only after CI passes. The accepted request is // bound to this head SHA in the managed guide comment. - acceptedTrigger = await resolveAcceptedTrigger(pr, existingGuide); + acceptedTrigger = await resolveAcceptedTrigger(pr, existingGuide, ciCompletedAt); // CI is passing. Now determine review state. const reviews = await github.paginate(github.rest.pulls.listReviews, { @@ -505,10 +525,7 @@ jobs: review.user?.login === pr.user?.login) { continue; } - const permissionResult = await github.rest.repos.getCollaboratorPermissionLevel({ - owner, repo, username: review.user.login, - }); - if (['admin', 'maintain', 'write'].includes(permissionResult.data.permission)) { + if (['admin', 'maintain', 'write'].includes(await permissionFor(review.user.login))) { freshMaintainerReviews.push(review); } } @@ -537,6 +554,9 @@ jobs: } else if (!codeRabbitApproved) { desiredLabel = 'awaiting-coderabbit'; phase = 'coderabbit'; + } else if (pr.draft) { + desiredLabel = 'awaiting-ready'; + phase = 'draft-approved'; } else if (maintainerChangeRequest) { desiredLabel = 'awaiting-author'; phase = 'maintainer-changes'; diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4b8ee190d7..bf349cca63 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -134,18 +134,18 @@ pnpm install ### Review Process -Ready-for-review PRs move through these gates in order: +Draft and ready-for-review PRs move through these gates in order: 1. Required CI checks must pass. -2. The PR author or another human comments `@coderabbitai review` to start CodeRabbit. For bot-authored PRs, this request must come from a human maintainer with write access. +2. The PR author or another account GitHub identifies as non-bot comments `@coderabbitai review` to start CodeRabbit. For bot-authored PRs, this request must come from a non-bot maintainer account with write access. 3. CodeRabbit reviews the latest commit. Address any findings and request another review after pushing updates. -4. After CodeRabbit approval, a human maintainer performs the final review and approval. +4. Draft PRs are marked ready after CodeRabbit approval, then a non-author maintainer account with write access performs the final review and approval. -An automated comment on each PR shows the current gate and next action. The `awaiting-review-trigger`, `awaiting-coderabbit`, `awaiting-maintainer`, `awaiting-author`, and `has-conflicts` labels make the same state visible in the PR list. A new commit invalidates the trigger and approvals, so a human must request CodeRabbit again before the final maintainer review. +An automated comment on each PR shows the current gate and next action. The `awaiting-review-trigger`, `awaiting-coderabbit`, `awaiting-ready`, `awaiting-maintainer`, `awaiting-author`, and `has-conflicts` labels make the same state visible in the PR list. A new commit invalidates the trigger and approvals, so CodeRabbit must be requested again before the final maintainer review. The `PR review gate` check passes only after the sequence completes. Repository administrators should configure it as a required status check on `main`; the labels and comment explain the state, while the required check enforces it. -PRs opened by bots or other automated accounts follow the same sequence. Automated authorship never substitutes for human accountability: a maintainer must verify the change's intent, provenance, and validation before merging. +PRs opened by bots or other automated accounts follow the same sequence. GitHub identifies actor accounts as `User` or `Bot`; it cannot distinguish a person's action from automation using that person's token. Final approval therefore requires a non-author, non-bot account with write access, and maintainers remain responsible for verifying the change's intent, provenance, and validation before 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 new file mode 100644 index 0000000000..5b3dd0b9c9 --- /dev/null +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -0,0 +1,351 @@ +import fs from "node:fs" +import path from "node:path" + +import { describe, expect, it, vi } from "vitest" +import { parse } from "yaml" + +const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor +const repositoryRoot = path.resolve(process.cwd(), "..") +const workflow = parse( + fs.readFileSync(path.join(repositoryRoot, ".github/workflows/label-pr-review-state.yml"), "utf8"), +) +const workflowScript = workflow.jobs.reconcile.steps[0].with.script as string + +const SHA = "a".repeat(40) +const CI_COMPLETED_AT = Date.parse("2026-08-29T15:00:00Z") +const REQUESTED_AT = Date.parse("2026-08-29T15:01:00Z") +const REVIEWED_AT = Date.parse("2026-08-29T15:02:00Z") + +type ReviewState = "APPROVED" | "CHANGES_REQUESTED" | "COMMENTED" | "DISMISSED" + +interface HarnessOptions { + prState?: "open" | "closed" + draft?: boolean + authorType?: "Bot" | "User" + eventName?: string + commentCreatedAt?: number + senderType?: "Bot" | "User" + existingGuide?: string + reviews?: Array<{ + login: string + type: "Bot" | "User" + state: ReviewState + submittedAt: number + commitId?: string + }> + permissions?: Record + requiredContexts?: string[] + branchRulesFail?: boolean +} + +function triggerMarker(requestedAt = REQUESTED_AT, actor = "maintainer") { + return `` +} + +function guideWithTrigger(requestedAt = REQUESTED_AT) { + return `\n${triggerMarker(requestedAt)}\n**Current step:** Waiting` +} + +async function runWorkflow(options: HarnessOptions = {}) { + const pr = { + number: 1437, + state: options.prState ?? "open", + draft: options.draft ?? false, + html_url: "https://github.com/Zoo-Code-Org/Zoo-Code/pull/1437", + user: { login: options.authorType === "User" ? "author" : "zoomote[bot]", type: options.authorType ?? "Bot" }, + head: { sha: SHA, repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, + base: { ref: "main", repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, + labels: [] as Array<{ name: string }>, + mergeable: true, + mergeable_state: "clean", + } + const requiredContexts = options.requiredContexts ?? ["tests"] + const checkRuns = requiredContexts + .filter((name) => name !== "reconcile") + .map((name, index) => ({ + id: index + 1, + name, + status: "completed", + conclusion: "success", + started_at: new Date(CI_COMPLETED_AT - 1_000).toISOString(), + completed_at: new Date(CI_COMPLETED_AT).toISOString(), + app: { id: 15368, slug: "github-actions" }, + })) + const reviews = (options.reviews ?? []).map((review, index) => ({ + id: index + 1, + state: review.state, + commit_id: review.commitId ?? SHA, + submitted_at: new Date(review.submittedAt).toISOString(), + user: { login: review.login, type: review.type }, + })) + const existingComments = options.existingGuide + ? [{ id: 10, user: { login: "github-actions[bot]" }, body: options.existingGuide }] + : [] + + const addLabels = vi.fn(async (_args: unknown) => undefined) + const createComment = vi.fn(async (_args: unknown) => undefined) + const updateComment = vi.fn(async (_args: unknown) => undefined) + const createCheck = vi.fn(async (_args: unknown) => undefined) + const updateCheck = vi.fn(async (_args: unknown) => undefined) + const setFailed = vi.fn() + const permissionFor = vi.fn(async ({ username }: { username: string }) => { + const permission = options.permissions?.[username] + if (!permission) { + throw Object.assign(new Error("Not Found"), { status: 404 }) + } + return { data: { permission } } + }) + + const github = { + paginate: vi.fn(async (target: unknown, args: unknown) => { + if (typeof target === "string") { + if (options.branchRulesFail) throw new Error("rules unavailable") + return [ + { + type: "required_status_checks", + parameters: { + required_status_checks: requiredContexts.map((context) => ({ + context, + integration_id: 15368, + })), + }, + }, + ] + } + if (typeof target !== "function") throw new Error("Unexpected paginate target") + return target(args) + }), + rest: { + pulls: { + get: vi.fn(async () => ({ data: pr })), + list: vi.fn(async () => [pr]), + listReviews: vi.fn(async () => reviews), + }, + issues: { + getLabel: vi.fn(async () => ({ data: {} })), + createLabel: vi.fn(async () => undefined), + removeLabel: vi.fn(async () => undefined), + addLabels, + listComments: vi.fn(async () => existingComments), + createComment, + updateComment, + }, + checks: { + listForRef: vi.fn(async (args: { check_name?: string }) => (args.check_name ? [] : checkRuns)), + create: createCheck, + update: updateCheck, + }, + repos: { + listCommitStatusesForRef: vi.fn(async () => []), + getCollaboratorPermissionLevel: permissionFor, + }, + }, + } + const context = { + eventName: options.eventName ?? "issue_comment", + repo: { owner: "Zoo-Code-Org", repo: "Zoo-Code" }, + payload: { + action: "created", + issue: { number: 1437, pull_request: {} }, + comment: { + body: "@coderabbitai review", + created_at: new Date(options.commentCreatedAt ?? REQUESTED_AT).toISOString(), + }, + sender: { login: "maintainer", type: options.senderType ?? "User" }, + }, + } + const core = { + info: vi.fn(), + debug: vi.fn(), + warning: vi.fn(), + error: vi.fn(), + setFailed, + } + + await new AsyncFunction("github", "context", "core", workflowScript)(github, context, core) + + return { + addLabels, + createComment, + updateComment, + createCheck, + updateCheck, + setFailed, + permissionFor, + } +} + +function latestGuide(result: Awaited>) { + const created = result.createComment.mock.calls.at(-1)?.[0] as { body?: string } | undefined + const updated = result.updateComment.mock.calls.at(-1)?.[0] as { body?: string } | undefined + return updated?.body ?? created?.body ?? "" +} + +function latestCheck(result: Awaited>) { + const created = result.createCheck.mock.calls.at(-1)?.[0] as + | { conclusion?: string; output?: { summary?: string } } + | undefined + const updated = result.updateCheck.mock.calls.at(-1)?.[0] as + | { conclusion?: string; output?: { summary?: string } } + | undefined + return updated ?? created +} + +describe("PR review-state workflow", () => { + it("ignores comment events for closed pull requests", async () => { + const result = await runWorkflow({ prState: "closed" }) + + expect(result.createComment).not.toHaveBeenCalled() + expect(result.createCheck).not.toHaveBeenCalled() + }) + + it("records draft review requests and waits for CodeRabbit", async () => { + const result = await runWorkflow({ draft: true, permissions: { maintainer: "write" } }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) + expect(latestGuide(result)).toContain(triggerMarker()) + expect(latestCheck(result)?.output?.summary).not.toContain("coderabbit-review-trigger") + }) + + it("moves approved draft reviews to awaiting-ready", async () => { + const result = await runWorkflow({ + draft: true, + permissions: { maintainer: "write" }, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-ready"] })) + expect(latestGuide(result)).toContain("Mark the draft ready for maintainer review") + }) + + it("keeps the first accepted trigger for repeated commands", async () => { + const result = await runWorkflow({ + draft: true, + commentCreatedAt: REVIEWED_AT + 1_000, + existingGuide: guideWithTrigger(), + permissions: { maintainer: "write" }, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-ready"] })) + expect(latestGuide(result)).toContain(triggerMarker()) + expect(latestGuide(result)).not.toContain(String(REVIEWED_AT + 1_000)) + }) + + it("rejects commands created before required CI completed", async () => { + const result = await runWorkflow({ + draft: true, + commentCreatedAt: CI_COMPLETED_AT - 1, + permissions: { maintainer: "write" }, + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-review-trigger"] })) + expect(latestGuide(result)).not.toContain("coderabbit-review-trigger") + }) + + it("rejects bot-authored review commands", async () => { + const result = await runWorkflow({ + draft: true, + senderType: "Bot", + permissions: { maintainer: "write" }, + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-review-trigger"] })) + expect(latestGuide(result)).not.toContain("coderabbit-review-trigger") + }) + + it("routes CodeRabbit change requests back to the author", async () => { + const result = await runWorkflow({ + draft: true, + permissions: { maintainer: "write" }, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "CHANGES_REQUESTED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + }) + + it("treats non-collaborator reviews as non-maintainer input", async () => { + const result = await runWorkflow({ + authorType: "User", + eventName: "pull_request_review", + existingGuide: guideWithTrigger(), + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "drive-by-reviewer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT + 1_000, + }, + ], + }) + + expect(result.setFailed).not.toHaveBeenCalled() + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + }) + + it("excludes the reconciliation job from required checks", async () => { + const result = await runWorkflow({ requiredContexts: ["tests", "reconcile"] }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-review-trigger"] })) + }) + + it("passes only after a later non-author maintainer approval", async () => { + const result = await runWorkflow({ + authorType: "User", + eventName: "pull_request_review", + existingGuide: guideWithTrigger(), + permissions: { maintainer: "write" }, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "maintainer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT + 1_000, + }, + ], + }) + + expect(latestCheck(result)?.conclusion).toBe("success") + expect(result.addLabels).not.toHaveBeenCalled() + }) + + it("fails closed when branch rules are unavailable", async () => { + const result = await runWorkflow({ branchRulesFail: true }) + + expect(result.addLabels).not.toHaveBeenCalled() + expect(latestCheck(result)?.output?.summary).toContain("required CI checks") + }) +}) From c18e59391593c98285ca0388ad2a695e6b98793e Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 16:54:50 +0000 Subject: [PATCH 04/29] refactor: queue CodeRabbit after required CI --- .coderabbit.yaml | 2 + .github/workflows/label-pr-review-state.yml | 179 ++++++------- CONTRIBUTING.md | 16 +- .../pr-review-state-workflow.test.ts | 239 +++++++++++------- 4 files changed, 231 insertions(+), 205 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index ff86c635ef..e32968bc7a 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -20,6 +20,8 @@ reviews: enabled: false drafts: false auto_incremental_review: false + labels: + - "coderabbit-review-active" path_filters: - "!**/node_modules/**" diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 65168339cd..2b43d5238b 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -10,8 +10,6 @@ on: types: [opened, reopened, ready_for_review, synchronize, review_requested, labeled, unlabeled] pull_request_review: types: [submitted, dismissed] - issue_comment: - types: [created] workflow_run: workflows: ["Code QA Roo Code", "E2E Tests (Mocked)", "Webview Visual Regression", "CodeQL Advanced"] types: [completed] @@ -37,7 +35,6 @@ jobs: const { owner, repo } = context.repo; const stateLabels = [ 'awaiting-author', - 'awaiting-review-trigger', 'awaiting-coderabbit', 'awaiting-ready', 'awaiting-maintainer', @@ -45,11 +42,6 @@ jobs: 'has-conflicts', ]; const labelDefinitions = [ - { - name: 'awaiting-review-trigger', - color: 'fbca04', - description: 'CI passed; waiting for a human to request CodeRabbit', - }, { name: 'awaiting-coderabbit', color: '5319e7', @@ -65,10 +57,16 @@ jobs: color: '0e8a16', description: 'CodeRabbit approved; waiting for a human maintainer', }, + { + name: 'coderabbit-review-active', + color: '5319e7', + description: 'Required CI passed; CodeRabbit review is active', + }, ]; const guideMarker = ''; - const triggerMarkerPrefix = '/); + return match?.[1] ?? null; + } + async function permissionFor(username) { try { const result = await github.rest.repos.getCollaboratorPermissionLevel({ @@ -178,14 +206,14 @@ jobs: } } - function phaseMessage(phase, acceptedTrigger) { + function phaseMessage(phase) { const messages = { + draft: 'Mark the PR ready to start CodeRabbit after required CI passes.', 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.', - trigger: 'Comment `@coderabbitai review` to start the AI review.', - 'coderabbit-changes': 'Address CodeRabbit findings and push an update. Request another review for the new commit.', - coderabbit: `CodeRabbit review was requested by @${acceptedTrigger?.actor}. Wait for it to approve the latest commit.`, + 'coderabbit-changes': 'Address CodeRabbit findings and push an update. Review restarts after required 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 for maintainer review.', 'maintainer-changes': 'Address the maintainer feedback, push an update, and request another review.', maintainer: 'CodeRabbit approved the latest commit. A maintainer must now review and approve it.', @@ -194,7 +222,7 @@ jobs: return messages[phase]; } - async function updateReviewGate(pr, phase, passed, acceptedTrigger = null) { + async function updateReviewGate(pr, phase, passed) { if (isReadOnlyRun && isForkPR(pr)) return; const checkRuns = await github.paginate(github.rest.checks.listForRef, { @@ -212,7 +240,7 @@ jobs: details_url: pr.html_url, output: { title: passed ? 'Review sequence complete' : 'Review sequence incomplete', - summary: phaseMessage(phase, acceptedTrigger), + summary: phaseMessage(phase), }, }; @@ -225,84 +253,31 @@ jobs: } } - function parseAcceptedTrigger(comment, headSha) { - const match = comment?.body?.match( - // - ); - if (!match || match[1] !== headSha) return null; - return { sha: match[1], actor: match[2], requestedAt: Number(match[3]) }; - } - - async function resolveAcceptedTrigger(pr, existingGuide, ciCompletedAt) { - let acceptedTrigger = parseAcceptedTrigger(existingGuide, pr.head.sha); - const isRequestEvent = context.eventName === 'issue_comment' && - context.payload.action === 'created' && - Boolean(context.payload.issue?.pull_request) && - /^@coderabbitai\s+(?:full\s+)?review\b/im.test(context.payload.comment?.body ?? ''); - - if (!isRequestEvent) return acceptedTrigger; - - const actor = context.payload.sender; - let permission = 'none'; - const botAuthored = pr.user?.type === 'Bot'; - if (actor?.type !== 'Bot' && botAuthored) { - permission = await permissionFor(actor.login); - } - const allowed = actor?.type !== 'Bot' && - (!botAuthored || ['admin', 'maintain', 'write'].includes(permission)); - - if (!allowed) { - core.warning( - `PR #${pr.number}: rejected CodeRabbit trigger from ${actor?.login ?? 'unknown'} (${permission})` - ); - return acceptedTrigger; - } - - const requestedAt = Date.parse(context.payload.comment.created_at); - if (requestedAt < ciCompletedAt) { - core.warning(`PR #${pr.number}: rejected review request made before CI completed`); - return acceptedTrigger; - } - - if (acceptedTrigger) { - core.info(`PR #${pr.number}: trigger already accepted for ${pr.head.sha}`); - return acceptedTrigger; - } - - acceptedTrigger = { - sha: pr.head.sha, - actor: actor.login, - requestedAt, - }; - core.info(`PR #${pr.number}: accepted CodeRabbit trigger from ${actor.login}`); - return acceptedTrigger; - } - - function reviewGuideBody(pr, phase, acceptedTrigger) { + function reviewGuideBody(pr, phase) { const automatedAuthor = pr.user?.type === 'Bot'; const authorNote = automatedAuthor ? 'This PR was opened by an automated account. A human maintainer must verify the change intent, provenance, and validation before merging.' : 'Thanks for contributing. This comment tracks the review sequence and the next action.'; - const triggerMarker = acceptedTrigger - ? `\n${triggerMarkerPrefix}${acceptedTrigger.sha}:${acceptedTrigger.actor}:${acceptedTrigger.requestedAt} -->` + const labelMarker = phase === 'coderabbit' + ? `\n${codeRabbitLabelMarkerPrefix}${pr.head.sha} -->` : ''; return `${guideMarker}\n### Review process\n\n${authorNote}\n\n` + '1. Required CI checks pass.\n' + - '2. A human explicitly requests CodeRabbit review.\n' + + '2. The workflow starts CodeRabbit automatically.\n' + '3. CodeRabbit reviews and approves the latest commit.\n' + '4. A human maintainer reviews and approves after CodeRabbit.\n\n' + - `**Current step:** ${phaseMessage(phase, acceptedTrigger)}${triggerMarker}`; + `**Current step:** ${phaseMessage(phase)}${labelMarker}`; } - async function updateReviewGuide(pr, phase, acceptedTrigger, existingGuide = null) { + async function updateReviewGuide(pr, phase, existingGuide = null) { if (isReadOnlyRun && isForkPR(pr)) { core.info(`PR #${pr.number}: fork PR on a read-only run — skipping guide update`); return; } - const body = reviewGuideBody(pr, phase, acceptedTrigger); + const body = reviewGuideBody(pr, phase); const existing = existingGuide ?? await findReviewGuide(pr); if (!existing) { @@ -354,8 +329,6 @@ jobs: for (const pr of prs) { try { const existingGuide = await findReviewGuide(pr); - let acceptedTrigger = parseAcceptedTrigger(existingGuide, pr.head.sha); - if (acceptedTrigger?.sha !== pr.head.sha) acceptedTrigger = null; // `mergeable`/`mergeable_state` are only returned by the single-PR GET // endpoint, and are computed asynchronously by GitHub — a PR fetched via @@ -369,8 +342,9 @@ jobs: if (prDetail.mergeable === false && prDetail.mergeable_state === 'dirty') { core.info(`PR #${pr.number}: has merge conflicts — labeling has-conflicts`); + await setCodeRabbitReviewActive(pr, false); await reconcileLabels(pr, 'has-conflicts'); - await updateReviewGuide(pr, 'conflict', acceptedTrigger, existingGuide); + await updateReviewGuide(pr, 'conflict', existingGuide); await updateReviewGate(pr, 'conflict', false); continue; } @@ -471,31 +445,21 @@ jobs: ) ); - const ciCompletedAt = Math.max( - 0, - ...relevantRuns.map(run => Date.parse(run.completed_at ?? run.started_at)), - ...relevantStatuses.map(status => Date.parse(status.updated_at ?? status.created_at)), - ); - // While CI is running or has failed, remove state labels and move on. // CI failure is its own signal; the label would add noise, not clarity. if (ciPending || ciFailed) { core.info(`PR #${pr.number}: CI ${ciPending ? 'pending' : 'failed'} — stripping state labels`); + await setCodeRabbitReviewActive(pr, false); await reconcileLabels(pr, null); await updateReviewGuide( pr, ciPending ? 'ci-pending' : 'ci-failed', - acceptedTrigger, existingGuide ); await updateReviewGate(pr, ciPending ? 'ci-pending' : 'ci-failed', false); continue; } - // A review request is accepted only after CI passes. The accepted request is - // bound to this head SHA in the managed guide comment. - acceptedTrigger = await resolveAcceptedTrigger(pr, existingGuide, ciCompletedAt); - // CI is passing. Now determine review state. const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: pr.number, per_page: 100, @@ -532,10 +496,7 @@ jobs: const maintainerChangeRequest = freshMaintainerReviews.find( review => review.state === 'CHANGES_REQUESTED' ); - const codeRabbitReviewAfterTrigger = acceptedTrigger && freshCodeRabbitReview && - Date.parse(freshCodeRabbitReview.submitted_at) > acceptedTrigger.requestedAt; - const codeRabbitApproved = codeRabbitReviewAfterTrigger && - freshCodeRabbitReview.state === 'APPROVED'; + const codeRabbitApproved = freshCodeRabbitReview?.state === 'APPROVED'; const maintainerApproval = freshMaintainerReviews .filter(review => review.state === 'APPROVED') .sort((a, b) => Date.parse(b.submitted_at) - Date.parse(a.submitted_at))[0]; @@ -545,25 +506,35 @@ jobs: let desiredLabel; let phase; - if (codeRabbitReviewAfterTrigger && freshCodeRabbitReview.state === 'CHANGES_REQUESTED') { + if (freshCodeRabbitReview?.state === 'CHANGES_REQUESTED') { + await setCodeRabbitReviewActive(pr, false); desiredLabel = 'awaiting-author'; phase = 'coderabbit-changes'; - } else if (!acceptedTrigger) { - desiredLabel = 'awaiting-review-trigger'; - phase = 'trigger'; } else if (!codeRabbitApproved) { - desiredLabel = 'awaiting-coderabbit'; - phase = 'coderabbit'; + if (pr.draft) { + await setCodeRabbitReviewActive(pr, false); + desiredLabel = null; + phase = 'draft'; + } else { + const recycleLabel = codeRabbitLabelHead(existingGuide) !== pr.head.sha; + await setCodeRabbitReviewActive(pr, true, recycleLabel); + desiredLabel = 'awaiting-coderabbit'; + phase = 'coderabbit'; + } } else if (pr.draft) { + await setCodeRabbitReviewActive(pr, false); desiredLabel = 'awaiting-ready'; phase = 'draft-approved'; } else if (maintainerChangeRequest) { + await setCodeRabbitReviewActive(pr, false); desiredLabel = 'awaiting-author'; phase = 'maintainer-changes'; } else if (!maintainerApprovedAfterCodeRabbit) { + await setCodeRabbitReviewActive(pr, false); desiredLabel = 'awaiting-maintainer'; phase = 'maintainer'; } else { + await setCodeRabbitReviewActive(pr, false); desiredLabel = null; phase = 'approved'; } @@ -575,8 +546,8 @@ jobs: ); await reconcileLabels(pr, desiredLabel); - await updateReviewGuide(pr, phase, acceptedTrigger, existingGuide); - await updateReviewGate(pr, phase, phase === 'approved', acceptedTrigger); + await updateReviewGuide(pr, phase, existingGuide); + await updateReviewGate(pr, phase, phase === 'approved'); } catch (error) { const detail = error.status ? `${error.message} (HTTP ${error.status}${error.response?.data?.message ? `: ${error.response.data.message}` : ''})` diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bf349cca63..d5d7051367 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -134,18 +134,20 @@ pnpm install ### Review Process -Draft and ready-for-review PRs move through these gates in order: +Ready-for-review PRs move through these gates in order: 1. Required CI checks must pass. -2. The PR author or another account GitHub identifies as non-bot comments `@coderabbitai review` to start CodeRabbit. For bot-authored PRs, this request must come from a non-bot maintainer account with write access. -3. CodeRabbit reviews the latest commit. Address any findings and request another review after pushing updates. -4. Draft PRs are marked ready after CodeRabbit approval, then a non-author maintainer account with write access performs the final review and approval. +2. The workflow automatically starts CodeRabbit review for the latest commit. +3. Address any CodeRabbit findings and push updates; review restarts automatically after required CI passes again. +4. After CodeRabbit approval, a non-author maintainer account with write access performs the final review and approval. -An automated comment on each PR shows the current gate and next action. The `awaiting-review-trigger`, `awaiting-coderabbit`, `awaiting-ready`, `awaiting-maintainer`, `awaiting-author`, and `has-conflicts` labels make the same state visible in the PR list. A new commit invalidates the trigger and approvals, so CodeRabbit must be requested again before the final maintainer review. +An automated comment on each PR shows the current gate and next action. The `awaiting-coderabbit`, `awaiting-maintainer`, `awaiting-author`, and `has-conflicts` labels make the same state visible in the PR list. A new commit invalidates prior approvals; required CI and CodeRabbit rerun for that commit before maintainer review. The `PR review gate` check passes only after the sequence completes. Repository administrators should configure it as a required status check on `main`; the labels and comment explain the state, while the required check enforces it. -PRs opened by bots or other automated accounts follow the same sequence. GitHub identifies actor accounts as `User` or `Bot`; it cannot distinguish a person's action from automation using that person's token. Final approval therefore requires a non-author, non-bot account with write access, and maintainers remain responsible for verifying the change's intent, provenance, and validation before merging. +Optional checks such as Codecov do not delay CodeRabbit unless repository rules make them required. Draft PRs are not reviewed automatically, but authors can still request an early review with `@coderabbitai review`. + +PRs opened by bots or other automated accounts follow the same CI and CodeRabbit gates. Final approval requires a non-author, non-bot account with write access, and maintainers remain responsible for verifying the change's intent, provenance, and validation before merging. - **Daily Triage:** Quick checks by maintainers. - **Weekly In-depth Review:** Comprehensive assessment. @@ -166,7 +168,7 @@ Maintainers may close PRs that are incomplete, too broad, inactive, not aligned PRs are also closed automatically by bot: - **60-day inactivity:** A PR with no activity for 60 days is marked stale and closed after a further 7 days if there is still no activity. Any new comment, commit, or review resets the timer. -- **14-day author inactivity:** After CodeRabbit or a maintainer requests changes, the PR is labelled `awaiting-author`. Author activity resets the inactivity timer. After an update, the PR moves to `awaiting-review-trigger`; after a human starts CodeRabbit and it approves, the PR moves to `awaiting-maintainer`. These waiting labels are not eligible for automatic closure under this policy. +- **14-day author inactivity:** After CodeRabbit or a maintainer requests changes, the PR is labelled `awaiting-author`. Author activity resets the inactivity timer. After an update, required CI and CodeRabbit rerun automatically; after CodeRabbit approval, the PR moves to `awaiting-maintainer`. These waiting states are not eligible for automatic closure under this policy. To opt a PR out of automatic closure, apply the `do-not-close`, `pinned`, or `work-in-progress` label. diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 5b3dd0b9c9..58ac4aa0bc 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -12,8 +12,7 @@ const workflow = parse( const workflowScript = workflow.jobs.reconcile.steps[0].with.script as string const SHA = "a".repeat(40) -const CI_COMPLETED_AT = Date.parse("2026-08-29T15:00:00Z") -const REQUESTED_AT = Date.parse("2026-08-29T15:01:00Z") +const OLD_SHA = "b".repeat(40) const REVIEWED_AT = Date.parse("2026-08-29T15:02:00Z") type ReviewState = "APPROVED" | "CHANGES_REQUESTED" | "COMMENTED" | "DISMISSED" @@ -21,11 +20,10 @@ type ReviewState = "APPROVED" | "CHANGES_REQUESTED" | "COMMENTED" | "DISMISSED" interface HarnessOptions { prState?: "open" | "closed" draft?: boolean - authorType?: "Bot" | "User" eventName?: string - commentCreatedAt?: number - senderType?: "Bot" | "User" - existingGuide?: string + existingGuide?: boolean + existingGuideHead?: string + labels?: string[] reviews?: Array<{ login: string type: "Bot" | "User" @@ -35,42 +33,54 @@ interface HarnessOptions { }> permissions?: Record requiredContexts?: string[] + requiredStatus?: "queued" | "in_progress" | "completed" + requiredConclusion?: "success" | "failure" + includeFailedCodecov?: boolean branchRulesFail?: boolean } -function triggerMarker(requestedAt = REQUESTED_AT, actor = "maintainer") { - return `` -} - -function guideWithTrigger(requestedAt = REQUESTED_AT) { - return `\n${triggerMarker(requestedAt)}\n**Current step:** Waiting` -} - async function runWorkflow(options: HarnessOptions = {}) { const pr = { number: 1437, state: options.prState ?? "open", draft: options.draft ?? false, html_url: "https://github.com/Zoo-Code-Org/Zoo-Code/pull/1437", - user: { login: options.authorType === "User" ? "author" : "zoomote[bot]", type: options.authorType ?? "Bot" }, + user: { login: "zoomote[bot]", type: "Bot" }, head: { sha: SHA, repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, base: { ref: "main", repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, - labels: [] as Array<{ name: string }>, + labels: (options.labels ?? []).map((name) => ({ name })), mergeable: true, mergeable_state: "clean", } const requiredContexts = options.requiredContexts ?? ["tests"] - const checkRuns = requiredContexts + const requiredRuns = requiredContexts .filter((name) => name !== "reconcile") .map((name, index) => ({ id: index + 1, name, - status: "completed", - conclusion: "success", - started_at: new Date(CI_COMPLETED_AT - 1_000).toISOString(), - completed_at: new Date(CI_COMPLETED_AT).toISOString(), + status: options.requiredStatus ?? "completed", + conclusion: + (options.requiredStatus ?? "completed") === "completed" + ? (options.requiredConclusion ?? "success") + : null, + started_at: "2026-08-29T15:00:00Z", + completed_at: (options.requiredStatus ?? "completed") === "completed" ? "2026-08-29T15:01:00Z" : null, app: { id: 15368, slug: "github-actions" }, })) + const checkRuns = options.includeFailedCodecov + ? [ + ...requiredRuns, + { + id: 100, + name: "codecov/patch", + status: "completed", + conclusion: "failure", + started_at: "2026-08-29T15:00:00Z", + completed_at: "2026-08-29T15:01:00Z", + app: { id: 254, slug: "codecov" }, + }, + ] + : requiredRuns const reviews = (options.reviews ?? []).map((review, index) => ({ id: index + 1, state: review.state, @@ -78,11 +88,23 @@ async function runWorkflow(options: HarnessOptions = {}) { submitted_at: new Date(review.submittedAt).toISOString(), user: { login: review.login, type: review.type }, })) - const existingComments = options.existingGuide - ? [{ id: 10, user: { login: "github-actions[bot]" }, body: options.existingGuide }] - : [] + const existingComments = + options.existingGuide || options.existingGuideHead + ? [ + { + id: 10, + user: { login: "github-actions[bot]" }, + body: + "\n**Current step:** Waiting" + + (options.existingGuideHead + ? `\n` + : ""), + }, + ] + : [] const addLabels = vi.fn(async (_args: unknown) => undefined) + const removeLabel = vi.fn(async (_args: unknown) => undefined) const createComment = vi.fn(async (_args: unknown) => undefined) const updateComment = vi.fn(async (_args: unknown) => undefined) const createCheck = vi.fn(async (_args: unknown) => undefined) @@ -90,9 +112,7 @@ async function runWorkflow(options: HarnessOptions = {}) { const setFailed = vi.fn() const permissionFor = vi.fn(async ({ username }: { username: string }) => { const permission = options.permissions?.[username] - if (!permission) { - throw Object.assign(new Error("Not Found"), { status: 404 }) - } + if (!permission) throw Object.assign(new Error("Not Found"), { status: 404 }) return { data: { permission } } }) @@ -124,7 +144,7 @@ async function runWorkflow(options: HarnessOptions = {}) { issues: { getLabel: vi.fn(async () => ({ data: {} })), createLabel: vi.fn(async () => undefined), - removeLabel: vi.fn(async () => undefined), + removeLabel, addLabels, listComments: vi.fn(async () => existingComments), createComment, @@ -141,17 +161,17 @@ async function runWorkflow(options: HarnessOptions = {}) { }, }, } + const eventName = options.eventName ?? "pull_request_target" const context = { - eventName: options.eventName ?? "issue_comment", + eventName, repo: { owner: "Zoo-Code-Org", repo: "Zoo-Code" }, payload: { - action: "created", - issue: { number: 1437, pull_request: {} }, - comment: { - body: "@coderabbitai review", - created_at: new Date(options.commentCreatedAt ?? REQUESTED_AT).toISOString(), + action: "ready_for_review", + pull_request: { + number: 1437, + head: { repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, + base: { repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, }, - sender: { login: "maintainer", type: options.senderType ?? "User" }, }, } const core = { @@ -166,12 +186,12 @@ async function runWorkflow(options: HarnessOptions = {}) { return { addLabels, + removeLabel, createComment, updateComment, createCheck, updateCheck, setFailed, - permissionFor, } } @@ -192,45 +212,92 @@ function latestCheck(result: Awaited>) { } describe("PR review-state workflow", () => { - it("ignores comment events for closed pull requests", async () => { + it("ignores events for closed pull requests", async () => { const result = await runWorkflow({ prState: "closed" }) expect(result.createComment).not.toHaveBeenCalled() expect(result.createCheck).not.toHaveBeenCalled() }) - it("records draft review requests and waits for CodeRabbit", async () => { - const result = await runWorkflow({ draft: true, permissions: { maintainer: "write" } }) + it("does not start automatic review for drafts", async () => { + const result = await runWorkflow({ draft: true }) + + expect(result.addLabels).not.toHaveBeenCalled() + expect(latestGuide(result)).toContain("Mark the PR ready") + }) + + it("removes the CodeRabbit label while required CI is pending", async () => { + const result = await runWorkflow({ + labels: ["coderabbit-review-active"], + requiredStatus: "in_progress", + }) + + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "coderabbit-review-active" })) + expect(latestCheck(result)?.output?.summary).toContain("required CI checks") + }) + + it("does not start CodeRabbit when required CI fails", async () => { + const result = await runWorkflow({ requiredConclusion: "failure" }) + + expect(result.addLabels).not.toHaveBeenCalled() + expect(latestCheck(result)?.output?.summary).toContain("Fix the failing required CI checks") + }) + + it("starts CodeRabbit automatically after required CI passes", async () => { + const result = await runWorkflow() + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) - expect(latestGuide(result)).toContain(triggerMarker()) - expect(latestCheck(result)?.output?.summary).not.toContain("coderabbit-review-trigger") + expect(latestGuide(result)).toContain(`coderabbit-review-label:${SHA}`) }) - it("moves approved draft reviews to awaiting-ready", async () => { + it("recycles a CodeRabbit label left over from an older head", async () => { const result = await runWorkflow({ - draft: true, - permissions: { maintainer: "write" }, + labels: ["coderabbit-review-active"], + existingGuideHead: OLD_SHA, + }) + + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "coderabbit-review-active" })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + + it("keeps a CodeRabbit label already bound to the current head", async () => { + const result = await runWorkflow({ + labels: ["coderabbit-review-active"], + existingGuideHead: SHA, + }) + + expect(result.removeLabel).not.toHaveBeenCalled() + expect(result.addLabels).not.toHaveBeenCalledWith( + expect.objectContaining({ labels: ["coderabbit-review-active"] }), + ) + }) + + it("ignores a failed optional Codecov check", async () => { + const result = await runWorkflow({ includeFailedCodecov: true }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + + it("routes CodeRabbit change requests back to the author", async () => { + const result = await runWorkflow({ + labels: ["coderabbit-review-active"], reviews: [ { login: "coderabbitai[bot]", type: "Bot", - state: "APPROVED", + state: "CHANGES_REQUESTED", submittedAt: REVIEWED_AT, }, ], }) - expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-ready"] })) - expect(latestGuide(result)).toContain("Mark the draft ready for maintainer review") + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "coderabbit-review-active" })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) }) - it("keeps the first accepted trigger for repeated commands", async () => { + it("moves approved ready PRs to maintainer review", async () => { const result = await runWorkflow({ - draft: true, - commentCreatedAt: REVIEWED_AT + 1_000, - existingGuide: guideWithTrigger(), - permissions: { maintainer: "write" }, reviews: [ { login: "coderabbitai[bot]", @@ -241,55 +308,27 @@ describe("PR review-state workflow", () => { ], }) - expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-ready"] })) - expect(latestGuide(result)).toContain(triggerMarker()) - expect(latestGuide(result)).not.toContain(String(REVIEWED_AT + 1_000)) - }) - - it("rejects commands created before required CI completed", async () => { - const result = await runWorkflow({ - draft: true, - commentCreatedAt: CI_COMPLETED_AT - 1, - permissions: { maintainer: "write" }, - }) - - expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-review-trigger"] })) - expect(latestGuide(result)).not.toContain("coderabbit-review-trigger") - }) - - it("rejects bot-authored review commands", async () => { - const result = await runWorkflow({ - draft: true, - senderType: "Bot", - permissions: { maintainer: "write" }, - }) - - expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-review-trigger"] })) - expect(latestGuide(result)).not.toContain("coderabbit-review-trigger") + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) }) - it("routes CodeRabbit change requests back to the author", async () => { + it("preserves manual draft approvals until the PR is ready", async () => { const result = await runWorkflow({ draft: true, - permissions: { maintainer: "write" }, reviews: [ { login: "coderabbitai[bot]", type: "Bot", - state: "CHANGES_REQUESTED", + state: "APPROVED", submittedAt: REVIEWED_AT, }, ], }) - expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-ready"] })) }) it("treats non-collaborator reviews as non-maintainer input", async () => { const result = await runWorkflow({ - authorType: "User", - eventName: "pull_request_review", - existingGuide: guideWithTrigger(), reviews: [ { login: "coderabbitai[bot]", @@ -310,17 +349,8 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) }) - it("excludes the reconciliation job from required checks", async () => { - const result = await runWorkflow({ requiredContexts: ["tests", "reconcile"] }) - - expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-review-trigger"] })) - }) - it("passes only after a later non-author maintainer approval", async () => { const result = await runWorkflow({ - authorType: "User", - eventName: "pull_request_review", - existingGuide: guideWithTrigger(), permissions: { maintainer: "write" }, reviews: [ { @@ -339,7 +369,12 @@ describe("PR review-state workflow", () => { }) expect(latestCheck(result)?.conclusion).toBe("success") - expect(result.addLabels).not.toHaveBeenCalled() + }) + + it("excludes the reconciliation job from required checks", async () => { + const result = await runWorkflow({ requiredContexts: ["tests", "reconcile"] }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) it("fails closed when branch rules are unavailable", async () => { @@ -348,4 +383,20 @@ describe("PR review-state workflow", () => { expect(result.addLabels).not.toHaveBeenCalled() expect(latestCheck(result)?.output?.summary).toContain("required CI checks") }) + + it("ignores CodeRabbit reviews from an older head", async () => { + const result = await runWorkflow({ + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + commitId: OLD_SHA, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) }) From 627ed3352a066290f76a7dfe3fffce1611cf548f Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 18:37:04 +0000 Subject: [PATCH 05/29] test: model PR review labels with TLA+ --- .github/tla/PrReviewLabels.cfg | 16 ++ .github/tla/PrReviewLabels.tla | 330 +++++++++++++++++++++++++++++++++ .github/tla/README.md | 30 +++ 3 files changed, 376 insertions(+) create mode 100644 .github/tla/PrReviewLabels.cfg create mode 100644 .github/tla/PrReviewLabels.tla create mode 100644 .github/tla/README.md diff --git a/.github/tla/PrReviewLabels.cfg b/.github/tla/PrReviewLabels.cfg new file mode 100644 index 0000000000..b18c86f884 --- /dev/null +++ b/.github/tla/PrReviewLabels.cfg @@ -0,0 +1,16 @@ +CONSTANT MaxHead = 2 + +INIT Init +NEXT Next + +INVARIANT TypeOK +INVARIANT SettledGateConsistency +INVARIANT SettledLabelConsistency +INVARIANT SettledControlLabelConsistency +INVARIANT AwaitingCodeRabbitSafety +INVARIANT AwaitingMaintainerSafety +INVARIANT AwaitingReadySafety +INVARIANT AwaitingAuthorSafety +INVARIANT ConflictLabelSafety +INVARIANT CodeRabbitActivationSafety +INVARIANT ApprovedStateHasNoLabel diff --git a/.github/tla/PrReviewLabels.tla b/.github/tla/PrReviewLabels.tla new file mode 100644 index 0000000000..70ffe59de0 --- /dev/null +++ b/.github/tla/PrReviewLabels.tla @@ -0,0 +1,330 @@ +--------------------------- MODULE PrReviewLabels --------------------------- +EXTENDS Integers, Naturals, TLC + +(*************************************************************************** +The model separates environment changes (pushes, CI, and reviews) from the +metadata workflow's Reconcile action. dirty = TRUE means GitHub has newer +source-of-truth state than the labels currently show. +***************************************************************************) + +CONSTANT MaxHead + +Heads == 1..MaxHead +CIStates == {"pending", "failed", "passed"} +ReviewStates == {"none", "changes", "approved"} +StateLabels == { + "none", + "has-conflicts", + "awaiting-coderabbit", + "awaiting-author", + "awaiting-ready", + "awaiting-maintainer" +} + +VARIABLES + head, + draft, + conflict, + ci, + crHead, + crState, + maintHead, + maintState, + maintAfterCR, + crLabelHead, + stateLabel, + gatePassed, + dirty + +vars == << + head, + draft, + conflict, + ci, + crHead, + crState, + maintHead, + maintState, + maintAfterCR, + crLabelHead, + stateLabel, + gatePassed, + dirty +>> + +CurrentCRApproved == crHead = head /\ crState = "approved" +CurrentCRChanges == crHead = head /\ crState = "changes" +CurrentMaintChanges == maintHead = head /\ maintState = "changes" +ValidMaintApproval == + maintHead = head /\ + maintState = "approved" /\ + maintAfterCR + +DesiredStateLabel == + CASE conflict -> "has-conflicts" + [] ci # "passed" -> "none" + [] CurrentCRChanges -> "awaiting-author" + [] draft /\ CurrentCRApproved -> "awaiting-ready" + [] draft -> "none" + [] ~CurrentCRApproved -> "awaiting-coderabbit" + [] CurrentMaintChanges -> "awaiting-author" + [] ~ValidMaintApproval -> "awaiting-maintainer" + [] OTHER -> "none" + +DesiredCRLabelHead == + IF ~conflict /\ + ci = "passed" /\ + ~draft /\ + ~CurrentCRApproved /\ + ~CurrentCRChanges + THEN head + ELSE 0 + +DesiredGatePassed == + ~conflict /\ + ci = "passed" /\ + ~draft /\ + CurrentCRApproved /\ + ValidMaintApproval + +Init == + /\ head = 1 + /\ draft = TRUE + /\ conflict = FALSE + /\ ci = "pending" + /\ crHead = 0 + /\ crState = "none" + /\ maintHead = 0 + /\ maintState = "none" + /\ maintAfterCR = FALSE + /\ crLabelHead = 0 + /\ stateLabel = "none" + /\ gatePassed = FALSE + /\ dirty = TRUE + +Push == + /\ head < MaxHead + /\ head' = head + 1 + /\ ci' = "pending" + /\ gatePassed' = FALSE + /\ dirty' = TRUE + /\ UNCHANGED << + draft, + conflict, + crHead, + crState, + maintHead, + maintState, + maintAfterCR, + crLabelHead, + stateLabel + >> + +MarkReady == + /\ draft + /\ draft' = FALSE + /\ dirty' = TRUE + /\ UNCHANGED << + head, + conflict, + ci, + crHead, + crState, + maintHead, + maintState, + maintAfterCR, + crLabelHead, + stateLabel, + gatePassed + >> + +ConvertToDraft == + /\ ~draft + /\ draft' = TRUE + /\ gatePassed' = FALSE + /\ dirty' = TRUE + /\ UNCHANGED << + head, + conflict, + ci, + crHead, + crState, + maintHead, + maintState, + maintAfterCR, + crLabelHead, + stateLabel + >> + +SetConflict(value) == + /\ value \in BOOLEAN + /\ conflict' = value + /\ IF value THEN gatePassed' = FALSE ELSE UNCHANGED gatePassed + /\ dirty' = TRUE + /\ UNCHANGED << + head, + draft, + ci, + crHead, + crState, + maintHead, + maintState, + maintAfterCR, + crLabelHead, + stateLabel + >> + +RestartCI == + /\ ci' = "pending" + /\ gatePassed' = FALSE + /\ dirty' = TRUE + /\ UNCHANGED << + head, + draft, + conflict, + crHead, + crState, + maintHead, + maintState, + maintAfterCR, + crLabelHead, + stateLabel + >> + +CompleteCI(result) == + /\ result \in {"failed", "passed"} + /\ ci' = result + /\ IF result = "failed" THEN gatePassed' = FALSE ELSE UNCHANGED gatePassed + /\ dirty' = TRUE + /\ UNCHANGED << + head, + draft, + conflict, + crHead, + crState, + maintHead, + maintState, + maintAfterCR, + crLabelHead, + stateLabel + >> + +CodeRabbitReview(result) == + /\ result \in {"changes", "approved"} + /\ \/ draft + \/ (~draft /\ ci = "passed" /\ crLabelHead = head) + /\ crHead' = head + /\ crState' = result + /\ dirty' = TRUE + /\ UNCHANGED << + head, + draft, + conflict, + ci, + maintHead, + maintState, + maintAfterCR, + crLabelHead, + stateLabel, + gatePassed + >> + +MaintainerReview(result) == + /\ result \in {"changes", "approved"} + /\ maintHead' = head + /\ maintState' = result + /\ maintAfterCR' = CurrentCRApproved + /\ dirty' = TRUE + /\ UNCHANGED << + head, + draft, + conflict, + ci, + crHead, + crState, + crLabelHead, + stateLabel, + gatePassed + >> + +Reconcile == + /\ stateLabel' = DesiredStateLabel + /\ crLabelHead' = DesiredCRLabelHead + /\ gatePassed' = DesiredGatePassed + /\ dirty' = FALSE + /\ UNCHANGED << + head, + draft, + conflict, + ci, + crHead, + crState, + maintHead, + maintState, + maintAfterCR + >> + +Next == + \/ Push + \/ MarkReady + \/ ConvertToDraft + \/ \E value \in BOOLEAN : SetConflict(value) + \/ RestartCI + \/ \E result \in {"failed", "passed"} : CompleteCI(result) + \/ \E result \in {"changes", "approved"} : CodeRabbitReview(result) + \/ \E result \in {"changes", "approved"} : MaintainerReview(result) + \/ Reconcile + +Spec == Init /\ [][Next]_vars + +TypeOK == + /\ head \in Heads + /\ draft \in BOOLEAN + /\ conflict \in BOOLEAN + /\ ci \in CIStates + /\ crHead \in 0..MaxHead + /\ crState \in ReviewStates + /\ maintHead \in 0..MaxHead + /\ maintState \in ReviewStates + /\ maintAfterCR \in BOOLEAN + /\ crLabelHead \in 0..MaxHead + /\ stateLabel \in StateLabels + /\ gatePassed \in BOOLEAN + /\ dirty \in BOOLEAN + +SettledGateConsistency == + ~dirty => gatePassed = DesiredGatePassed + +SettledLabelConsistency == + ~dirty => stateLabel = DesiredStateLabel + +SettledControlLabelConsistency == + ~dirty => crLabelHead = DesiredCRLabelHead + +AwaitingCodeRabbitSafety == + (~dirty /\ stateLabel = "awaiting-coderabbit") => + (~draft /\ ci = "passed" /\ ~conflict /\ ~CurrentCRApproved /\ ~CurrentCRChanges) + +AwaitingMaintainerSafety == + (~dirty /\ stateLabel = "awaiting-maintainer") => + (~draft /\ ci = "passed" /\ CurrentCRApproved /\ ~ValidMaintApproval) + +AwaitingReadySafety == + (~dirty /\ stateLabel = "awaiting-ready") => + (draft /\ ci = "passed" /\ CurrentCRApproved) + +AwaitingAuthorSafety == + (~dirty /\ stateLabel = "awaiting-author") => + (CurrentCRChanges \/ (CurrentCRApproved /\ CurrentMaintChanges)) + +ConflictLabelSafety == + (~dirty /\ stateLabel = "has-conflicts") => conflict + +CodeRabbitActivationSafety == + (~dirty /\ crLabelHead # 0) => + (crLabelHead = head /\ ~draft /\ ci = "passed" /\ ~conflict) + +ApprovedStateHasNoLabel == + gatePassed => stateLabel = "none" + +============================================================================= diff --git a/.github/tla/README.md b/.github/tla/README.md new file mode 100644 index 0000000000..6fd15e58e1 --- /dev/null +++ b/.github/tla/README.md @@ -0,0 +1,30 @@ +# PR review label model + +`PrReviewLabels.tla` models the review workflow as two independently scheduled systems: + +- GitHub changes the PR head, draft state, conflicts, required CI, and reviews. +- The metadata workflow reconciles those facts into one state label, the CodeRabbit activation label, and the advisory review gate. + +The `dirty` variable allows webhook delivery and reconciliation to lag. Label and advisory-gate consistency are required whenever reconciliation has settled. The model intentionally does not treat the custom check as an instantaneous enforcement boundary: GitHub's native CI and review state can change before the metadata workflow processes the corresponding webhook. + +The finite model checks two head commits and covers: + +- pushes and stale reviews; +- draft/ready conversion; +- conflicts; +- required CI pending, failure, success, and reruns; +- automatic and manual-draft CodeRabbit reviews; +- maintainer reviews before and after CodeRabbit; +- delayed or out-of-order reconciliation. + +## Run TLC + +Download the pinned TLA+ tools release, then run TLC from this directory: + +```bash +curl -fsSLO https://github.com/tlaplus/tlaplus/releases/download/v1.7.4/tla2tools.jar +printf '%s %s\n' bee4a54f3ee3d4afc347c3240ec2d9e93b075104 tla2tools.jar | sha1sum --check +java -cp tla2tools.jar tlc2.TLC -config PrReviewLabels.cfg PrReviewLabels.tla +``` + +The JAR is a local tool and must not be committed. From 8f76ffa916b231f1dc3ea23b82a74f8335a85312 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 18:38:06 +0000 Subject: [PATCH 06/29] docs: clarify advisory review gate --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d5d7051367..3b9eceee8e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -143,7 +143,7 @@ Ready-for-review PRs move through these gates in order: An automated comment on each PR shows the current gate and next action. The `awaiting-coderabbit`, `awaiting-maintainer`, `awaiting-author`, and `has-conflicts` labels make the same state visible in the PR list. A new commit invalidates prior approvals; required CI and CodeRabbit rerun for that commit before maintainer review. -The `PR review gate` check passes only after the sequence completes. Repository administrators should configure it as a required status check on `main`; the labels and comment explain the state, while the required check enforces it. +The `PR review gate` check passes only after the sequence completes. It is advisory by default; repository administrators can make it required on `main` if enforcement is desired. The labels and managed comment remain the maintainer-facing review queue either way. Optional checks such as Codecov do not delay CodeRabbit unless repository rules make them required. Draft PRs are not reviewed automatically, but authors can still request an early review with `@coderabbitai review`. From 11ac8e5afb7033f63e6143832c6bad9ca7906f1f Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 19:59:51 +0000 Subject: [PATCH 07/29] fix: address review gate feedback --- .github/tla/PrReviewLabels.cfg | 5 +- .github/tla/PrReviewLabels.tla | 5 +- .github/tla/README.md | 4 +- .github/workflows/label-pr-review-state.yml | 13 +- CONTRIBUTING.md | 2 +- .../pr-review-state-workflow.test.ts | 141 +++++++++++++++--- 6 files changed, 143 insertions(+), 27 deletions(-) diff --git a/.github/tla/PrReviewLabels.cfg b/.github/tla/PrReviewLabels.cfg index b18c86f884..3a7a9aff21 100644 --- a/.github/tla/PrReviewLabels.cfg +++ b/.github/tla/PrReviewLabels.cfg @@ -1,7 +1,8 @@ CONSTANT MaxHead = 2 -INIT Init -NEXT Next +SPECIFICATION Spec + +PROPERTY EventualReconciliation INVARIANT TypeOK INVARIANT SettledGateConsistency diff --git a/.github/tla/PrReviewLabels.tla b/.github/tla/PrReviewLabels.tla index 70ffe59de0..15d4bc6062 100644 --- a/.github/tla/PrReviewLabels.tla +++ b/.github/tla/PrReviewLabels.tla @@ -211,6 +211,7 @@ CompleteCI(result) == CodeRabbitReview(result) == /\ result \in {"changes", "approved"} + /\ crHead # head /\ \/ draft \/ (~draft /\ ci = "passed" /\ crLabelHead = head) /\ crHead' = head @@ -275,7 +276,9 @@ Next == \/ \E result \in {"changes", "approved"} : MaintainerReview(result) \/ Reconcile -Spec == Init /\ [][Next]_vars +Spec == Init /\ [][Next]_vars /\ WF_vars(Reconcile) + +EventualReconciliation == []<>(~dirty) TypeOK == /\ head \in Heads diff --git a/.github/tla/README.md b/.github/tla/README.md index 6fd15e58e1..a48a628d83 100644 --- a/.github/tla/README.md +++ b/.github/tla/README.md @@ -23,8 +23,8 @@ Download the pinned TLA+ tools release, then run TLC from this directory: ```bash curl -fsSLO https://github.com/tlaplus/tlaplus/releases/download/v1.7.4/tla2tools.jar -printf '%s %s\n' bee4a54f3ee3d4afc347c3240ec2d9e93b075104 tla2tools.jar | sha1sum --check +printf '%s %s\n' 936a262061c914694dfd669a543be24573c45d5aa0ff20a8b96b23d01e050e88 tla2tools.jar | sha256sum --check java -cp tla2tools.jar tlc2.TLC -config PrReviewLabels.cfg PrReviewLabels.tla ``` -The JAR is a local tool and must not be committed. +The JAR is a local tool and must not be committed. Weak fairness on reconciliation checks that metadata eventually converges after asynchronous GitHub events; the model does not claim that CI or reviewers must eventually approve a PR. diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 2b43d5238b..329064751a 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -167,9 +167,13 @@ jobs: const hasLabel = pr.labels.some(label => label.name === codeRabbitActiveLabel); if (enabled && hasLabel && recycle) { - await github.rest.issues.removeLabel({ - owner, repo, issue_number: pr.number, name: codeRabbitActiveLabel, - }); + try { + await github.rest.issues.removeLabel({ + owner, repo, issue_number: pr.number, name: codeRabbitActiveLabel, + }); + } catch (error) { + if (error.status !== 404) throw error; + } pr.labels = pr.labels.filter(label => label.name !== codeRabbitActiveLabel); } if (enabled && (!hasLabel || recycle)) { @@ -190,7 +194,8 @@ jobs: } function codeRabbitLabelHead(comment) { - const match = comment?.body?.match(//); + const markerPattern = new RegExp(`${codeRabbitLabelMarkerPrefix}([a-f0-9]{40}) -->`); + const match = comment?.body?.match(markerPattern); return match?.[1] ?? null; } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3b9eceee8e..da1a992a13 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -168,7 +168,7 @@ Maintainers may close PRs that are incomplete, too broad, inactive, not aligned PRs are also closed automatically by bot: - **60-day inactivity:** A PR with no activity for 60 days is marked stale and closed after a further 7 days if there is still no activity. Any new comment, commit, or review resets the timer. -- **14-day author inactivity:** After CodeRabbit or a maintainer requests changes, the PR is labelled `awaiting-author`. Author activity resets the inactivity timer. After an update, required CI and CodeRabbit rerun automatically; after CodeRabbit approval, the PR moves to `awaiting-maintainer`. These waiting states are not eligible for automatic closure under this policy. +- **14-day author inactivity:** After CodeRabbit or a maintainer requests changes, the PR is labelled `awaiting-author`. If there is no author activity for 14 days, it is marked stale and closes 7 days later without new activity. Author activity resets that timer. After an update, required CI and CodeRabbit rerun automatically; after CodeRabbit approval, the PR moves to `awaiting-maintainer`, which remains subject to the general 60-day inactivity plus 7-day closure policy. To opt a PR out of automatic closure, apply the `do-not-close`, `pinned`, or `work-in-progress` label. diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 58ac4aa0bc..2acaa654d4 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -1,11 +1,12 @@ import fs from "node:fs" import path from "node:path" +import { fileURLToPath } from "node:url" import { describe, expect, it, vi } from "vitest" import { parse } from "yaml" const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor -const repositoryRoot = path.resolve(process.cwd(), "..") +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"), ) @@ -20,10 +21,13 @@ type ReviewState = "APPROVED" | "CHANGES_REQUESTED" | "COMMENTED" | "DISMISSED" interface HarnessOptions { prState?: "open" | "closed" draft?: boolean + conflict?: boolean eventName?: string existingGuide?: boolean existingGuideHead?: string + existingGate?: boolean labels?: string[] + removeLabelStatus?: number reviews?: Array<{ login: string type: "Bot" | "User" @@ -32,9 +36,13 @@ interface HarnessOptions { commitId?: string }> permissions?: Record + permissionErrorStatus?: number requiredContexts?: string[] + requiredIntegrationId?: number | null requiredStatus?: "queued" | "in_progress" | "completed" requiredConclusion?: "success" | "failure" + omitRequiredRuns?: boolean + commitStatusContexts?: string[] includeFailedCodecov?: boolean branchRulesFail?: boolean } @@ -49,11 +57,11 @@ async function runWorkflow(options: HarnessOptions = {}) { head: { sha: SHA, repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, base: { ref: "main", repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, labels: (options.labels ?? []).map((name) => ({ name })), - mergeable: true, - mergeable_state: "clean", + mergeable: options.conflict ? false : true, + mergeable_state: options.conflict ? "dirty" : "clean", } const requiredContexts = options.requiredContexts ?? ["tests"] - const requiredRuns = requiredContexts + const requiredRuns = (options.omitRequiredRuns ? [] : requiredContexts) .filter((name) => name !== "reconcile") .map((name, index) => ({ id: index + 1, @@ -104,13 +112,20 @@ async function runWorkflow(options: HarnessOptions = {}) { : [] const addLabels = vi.fn(async (_args: unknown) => undefined) - const removeLabel = vi.fn(async (_args: unknown) => undefined) + const removeLabel = vi.fn(async (_args: unknown) => { + if (options.removeLabelStatus) { + throw Object.assign(new Error("Remove label failed"), { status: options.removeLabelStatus }) + } + }) const createComment = vi.fn(async (_args: unknown) => undefined) const updateComment = vi.fn(async (_args: unknown) => undefined) const createCheck = vi.fn(async (_args: unknown) => undefined) const updateCheck = vi.fn(async (_args: unknown) => undefined) const setFailed = vi.fn() const permissionFor = vi.fn(async ({ username }: { username: string }) => { + if (options.permissionErrorStatus) { + throw Object.assign(new Error("Permission lookup failed"), { status: options.permissionErrorStatus }) + } const permission = options.permissions?.[username] if (!permission) throw Object.assign(new Error("Not Found"), { status: 404 }) return { data: { permission } } @@ -126,7 +141,8 @@ async function runWorkflow(options: HarnessOptions = {}) { parameters: { required_status_checks: requiredContexts.map((context) => ({ context, - integration_id: 15368, + integration_id: + options.requiredIntegrationId === undefined ? 15368 : options.requiredIntegrationId, })), }, }, @@ -151,28 +167,46 @@ async function runWorkflow(options: HarnessOptions = {}) { updateComment, }, checks: { - listForRef: vi.fn(async (args: { check_name?: string }) => (args.check_name ? [] : checkRuns)), + listForRef: vi.fn(async (args: { check_name?: string }) => + args.check_name + ? options.existingGate + ? [{ id: 500, name: "PR review gate", app: { slug: "github-actions" } }] + : [] + : checkRuns, + ), create: createCheck, update: updateCheck, }, repos: { - listCommitStatusesForRef: vi.fn(async () => []), + listCommitStatusesForRef: vi.fn(async () => + (options.commitStatusContexts ?? []).map((context, index) => ({ + id: index + 1, + context, + state: "success", + created_at: "2026-08-29T15:00:00Z", + updated_at: "2026-08-29T15:01:00Z", + })), + ), getCollaboratorPermissionLevel: permissionFor, }, }, } const eventName = options.eventName ?? "pull_request_target" + const pullRequestPayload = { + number: 1437, + head: { repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, + base: { repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, + } const context = { eventName, repo: { owner: "Zoo-Code-Org", repo: "Zoo-Code" }, - payload: { - action: "ready_for_review", - pull_request: { - number: 1437, - head: { repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, - base: { repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, - }, - }, + payload: + eventName === "schedule" + ? {} + : { + action: "ready_for_review", + pull_request: pullRequestPayload, + }, } const core = { info: vi.fn(), @@ -192,6 +226,7 @@ async function runWorkflow(options: HarnessOptions = {}) { createCheck, updateCheck, setFailed, + listPullRequests: github.rest.pulls.list, } } @@ -213,10 +248,15 @@ function latestCheck(result: Awaited>) { describe("PR review-state workflow", () => { it("ignores events for closed pull requests", async () => { - const result = await runWorkflow({ prState: "closed" }) + const result = await runWorkflow({ prState: "closed", existingGate: true }) + expect(result.addLabels).not.toHaveBeenCalled() + expect(result.removeLabel).not.toHaveBeenCalled() expect(result.createComment).not.toHaveBeenCalled() + expect(result.updateComment).not.toHaveBeenCalled() expect(result.createCheck).not.toHaveBeenCalled() + expect(result.updateCheck).not.toHaveBeenCalled() + expect(result.setFailed).not.toHaveBeenCalled() }) it("does not start automatic review for drafts", async () => { @@ -261,6 +301,17 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) + it("tolerates an already-removed CodeRabbit label while recycling", async () => { + const result = await runWorkflow({ + labels: ["coderabbit-review-active"], + existingGuideHead: OLD_SHA, + removeLabelStatus: 404, + }) + + expect(result.setFailed).not.toHaveBeenCalled() + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + it("keeps a CodeRabbit label already bound to the current head", async () => { const result = await runWorkflow({ labels: ["coderabbit-review-active"], @@ -279,6 +330,26 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) + it("uses a legacy commit status for an unpinned required context", async () => { + const result = await runWorkflow({ + requiredIntegrationId: null, + omitRequiredRuns: true, + commitStatusContexts: ["tests"], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + + it("removes CodeRabbit activation when the PR has conflicts", async () => { + const result = await runWorkflow({ + conflict: true, + labels: ["coderabbit-review-active"], + }) + + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "coderabbit-review-active" })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["has-conflicts"] })) + }) + it("routes CodeRabbit change requests back to the author", async () => { const result = await runWorkflow({ labels: ["coderabbit-review-active"], @@ -371,6 +442,35 @@ describe("PR review-state workflow", () => { expect(latestCheck(result)?.conclusion).toBe("success") }) + it("updates an existing review-gate check run", async () => { + const result = await runWorkflow({ existingGate: true }) + + expect(result.updateCheck).toHaveBeenCalledWith(expect.objectContaining({ check_run_id: 500 })) + expect(result.createCheck).not.toHaveBeenCalled() + }) + + it("reports non-404 permission lookup failures", async () => { + const result = await runWorkflow({ + permissionErrorStatus: 500, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "maintainer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT + 1_000, + }, + ], + }) + + expect(result.setFailed).toHaveBeenCalled() + }) + it("excludes the reconciliation job from required checks", async () => { const result = await runWorkflow({ requiredContexts: ["tests", "reconcile"] }) @@ -384,6 +484,13 @@ describe("PR review-state workflow", () => { expect(latestCheck(result)?.output?.summary).toContain("required CI checks") }) + it("lists open PRs during scheduled reconciliation", async () => { + const result = await runWorkflow({ eventName: "schedule" }) + + expect(result.listPullRequests).toHaveBeenCalled() + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + it("ignores CodeRabbit reviews from an older head", async () => { const result = await runWorkflow({ reviews: [ From 1f67e3f798ac3edf85108565c57ea49024653505 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 20:57:02 +0000 Subject: [PATCH 08/29] refactor: publish review gate as commit status --- .github/workflows/label-pr-review-state.yml | 59 +++---- CONTRIBUTING.md | 2 +- .../pr-review-state-workflow.test.ts | 152 +++++++++++++----- 3 files changed, 131 insertions(+), 82 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 329064751a..9b7173bfde 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -17,8 +17,8 @@ on: permissions: pull-requests: write issues: write - checks: write - statuses: read + checks: read + statuses: write concurrency: group: label-pr-review-state @@ -230,32 +230,15 @@ jobs: async function updateReviewGate(pr, phase, passed) { if (isReadOnlyRun && isForkPR(pr)) return; - const checkRuns = await github.paginate(github.rest.checks.listForRef, { - owner, repo, ref: pr.head.sha, check_name: reviewGateName, per_page: 100, - }); - const existing = checkRuns - .filter(run => run.name === reviewGateName && run.app?.slug === 'github-actions') - .sort((a, b) => b.id - a.id)[0]; - const params = { + await github.rest.repos.createCommitStatus({ owner, repo, - name: reviewGateName, - status: 'completed', - conclusion: passed ? 'success' : 'action_required', - details_url: pr.html_url, - output: { - title: passed ? 'Review sequence complete' : 'Review sequence incomplete', - summary: phaseMessage(phase), - }, - }; - - if (existing) { - await github.rest.checks.update({ - owner, repo, check_run_id: existing.id, ...params, - }); - } else { - await github.rest.checks.create({ ...params, head_sha: pr.head.sha }); - } + sha: pr.head.sha, + state: passed ? 'success' : 'failure', + context: reviewGateName, + description: phaseMessage(phase), + target_url: pr.html_url, + }); } function reviewGuideBody(pr, phase) { @@ -347,10 +330,10 @@ jobs: if (prDetail.mergeable === false && prDetail.mergeable_state === 'dirty') { core.info(`PR #${pr.number}: has merge conflicts — labeling has-conflicts`); + await updateReviewGate(pr, 'conflict', false); await setCodeRabbitReviewActive(pr, false); await reconcileLabels(pr, 'has-conflicts'); await updateReviewGuide(pr, 'conflict', existingGuide); - await updateReviewGate(pr, 'conflict', false); continue; } @@ -454,6 +437,7 @@ jobs: // CI failure is its own signal; the label would add noise, not clarity. if (ciPending || ciFailed) { core.info(`PR #${pr.number}: CI ${ciPending ? 'pending' : 'failed'} — stripping state labels`); + await updateReviewGate(pr, ciPending ? 'ci-pending' : 'ci-failed', false); await setCodeRabbitReviewActive(pr, false); await reconcileLabels(pr, null); await updateReviewGuide( @@ -461,7 +445,6 @@ jobs: ciPending ? 'ci-pending' : 'ci-failed', existingGuide ); - await updateReviewGate(pr, ciPending ? 'ci-pending' : 'ci-failed', false); continue; } @@ -511,35 +494,31 @@ jobs: let desiredLabel; let phase; + let activateCodeRabbit = false; + let recycleCodeRabbitLabel = false; if (freshCodeRabbitReview?.state === 'CHANGES_REQUESTED') { - await setCodeRabbitReviewActive(pr, false); desiredLabel = 'awaiting-author'; phase = 'coderabbit-changes'; } else if (!codeRabbitApproved) { if (pr.draft) { - await setCodeRabbitReviewActive(pr, false); desiredLabel = null; phase = 'draft'; } else { - const recycleLabel = codeRabbitLabelHead(existingGuide) !== pr.head.sha; - await setCodeRabbitReviewActive(pr, true, recycleLabel); + activateCodeRabbit = true; + recycleCodeRabbitLabel = codeRabbitLabelHead(existingGuide) !== pr.head.sha; desiredLabel = 'awaiting-coderabbit'; phase = 'coderabbit'; } } else if (pr.draft) { - await setCodeRabbitReviewActive(pr, false); desiredLabel = 'awaiting-ready'; phase = 'draft-approved'; } else if (maintainerChangeRequest) { - await setCodeRabbitReviewActive(pr, false); desiredLabel = 'awaiting-author'; phase = 'maintainer-changes'; } else if (!maintainerApprovedAfterCodeRabbit) { - await setCodeRabbitReviewActive(pr, false); desiredLabel = 'awaiting-maintainer'; phase = 'maintainer'; } else { - await setCodeRabbitReviewActive(pr, false); desiredLabel = null; phase = 'approved'; } @@ -550,9 +529,15 @@ jobs: `maintainer=${maintainerApproval?.state ?? 'pending'} → ${desiredLabel ?? '(none)'}` ); + if (phase !== 'approved') { + await updateReviewGate(pr, phase, false); + } + await setCodeRabbitReviewActive(pr, activateCodeRabbit, recycleCodeRabbitLabel); await reconcileLabels(pr, desiredLabel); await updateReviewGuide(pr, phase, existingGuide); - await updateReviewGate(pr, phase, phase === 'approved'); + if (phase === 'approved') { + await updateReviewGate(pr, phase, true); + } } catch (error) { const detail = error.status ? `${error.message} (HTTP ${error.status}${error.response?.data?.message ? `: ${error.response.data.message}` : ''})` diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index da1a992a13..05acfae631 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -143,7 +143,7 @@ Ready-for-review PRs move through these gates in order: An automated comment on each PR shows the current gate and next action. The `awaiting-coderabbit`, `awaiting-maintainer`, `awaiting-author`, and `has-conflicts` labels make the same state visible in the PR list. A new commit invalidates prior approvals; required CI and CodeRabbit rerun for that commit before maintainer review. -The `PR review gate` check passes only after the sequence completes. It is advisory by default; repository administrators can make it required on `main` if enforcement is desired. The labels and managed comment remain the maintainer-facing review queue either way. +The `PR review gate` commit status passes only after the sequence completes. It is advisory by default; repository administrators can make it required on `main` if enforcement is desired. The labels and managed comment remain the maintainer-facing review queue either way. Optional checks such as Codecov do not delay CodeRabbit unless repository rules make them required. Draft PRs are not reviewed automatically, but authors can still request an early review with `@coderabbitai review`. diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 2acaa654d4..c7d3189e24 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -22,11 +22,14 @@ interface HarnessOptions { prState?: "open" | "closed" draft?: boolean conflict?: boolean + fork?: boolean eventName?: string + workflowRunAssociated?: boolean existingGuide?: boolean existingGuideHead?: string - existingGate?: boolean labels?: string[] + addLabelsStatus?: number + labelLookupStatus?: number removeLabelStatus?: number reviews?: Array<{ login: string @@ -47,14 +50,16 @@ interface HarnessOptions { branchRulesFail?: boolean } +/** Executes the embedded github-script workflow against deterministic GitHub API doubles. */ async function runWorkflow(options: HarnessOptions = {}) { + const headRepository = options.fork ? "contributor/Zoo-Code" : "Zoo-Code-Org/Zoo-Code" const pr = { number: 1437, state: options.prState ?? "open", draft: options.draft ?? false, html_url: "https://github.com/Zoo-Code-Org/Zoo-Code/pull/1437", user: { login: "zoomote[bot]", type: "Bot" }, - head: { sha: SHA, repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, + head: { sha: SHA, repo: { full_name: headRepository } }, base: { ref: "main", repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, labels: (options.labels ?? []).map((name) => ({ name })), mergeable: options.conflict ? false : true, @@ -111,7 +116,11 @@ async function runWorkflow(options: HarnessOptions = {}) { ] : [] - const addLabels = vi.fn(async (_args: unknown) => undefined) + const addLabels = vi.fn(async (_args: unknown) => { + if (options.addLabelsStatus) { + throw Object.assign(new Error("Add labels failed"), { status: options.addLabelsStatus }) + } + }) const removeLabel = vi.fn(async (_args: unknown) => { if (options.removeLabelStatus) { throw Object.assign(new Error("Remove label failed"), { status: options.removeLabelStatus }) @@ -119,8 +128,8 @@ async function runWorkflow(options: HarnessOptions = {}) { }) const createComment = vi.fn(async (_args: unknown) => undefined) const updateComment = vi.fn(async (_args: unknown) => undefined) - const createCheck = vi.fn(async (_args: unknown) => undefined) - const updateCheck = vi.fn(async (_args: unknown) => undefined) + const createCommitStatus = vi.fn(async (_args: unknown) => undefined) + const createLabel = vi.fn(async (_args: unknown) => undefined) const setFailed = vi.fn() const permissionFor = vi.fn(async ({ username }: { username: string }) => { if (options.permissionErrorStatus) { @@ -158,8 +167,13 @@ async function runWorkflow(options: HarnessOptions = {}) { listReviews: vi.fn(async () => reviews), }, issues: { - getLabel: vi.fn(async () => ({ data: {} })), - createLabel: vi.fn(async () => undefined), + getLabel: vi.fn(async () => { + if (options.labelLookupStatus) { + throw Object.assign(new Error("Label lookup failed"), { status: options.labelLookupStatus }) + } + return { data: {} } + }), + createLabel, removeLabel, addLabels, listComments: vi.fn(async () => existingComments), @@ -167,17 +181,10 @@ async function runWorkflow(options: HarnessOptions = {}) { updateComment, }, checks: { - listForRef: vi.fn(async (args: { check_name?: string }) => - args.check_name - ? options.existingGate - ? [{ id: 500, name: "PR review gate", app: { slug: "github-actions" } }] - : [] - : checkRuns, - ), - create: createCheck, - update: updateCheck, + listForRef: vi.fn(async () => checkRuns), }, repos: { + createCommitStatus, listCommitStatusesForRef: vi.fn(async () => (options.commitStatusContexts ?? []).map((context, index) => ({ id: index + 1, @@ -194,19 +201,26 @@ async function runWorkflow(options: HarnessOptions = {}) { const eventName = options.eventName ?? "pull_request_target" const pullRequestPayload = { number: 1437, - head: { repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, + head: { repo: { full_name: headRepository } }, base: { repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, } - const context = { - eventName, - repo: { owner: "Zoo-Code-Org", repo: "Zoo-Code" }, - payload: - eventName === "schedule" - ? {} + const payload = + eventName === "schedule" + ? {} + : eventName === "workflow_run" + ? { + workflow_run: { + pull_requests: options.workflowRunAssociated === false ? [] : [{ number: 1437 }], + }, + } : { action: "ready_for_review", pull_request: pullRequestPayload, - }, + } + const context = { + eventName, + repo: { owner: "Zoo-Code-Org", repo: "Zoo-Code" }, + payload, } const core = { info: vi.fn(), @@ -223,42 +237,60 @@ async function runWorkflow(options: HarnessOptions = {}) { removeLabel, createComment, updateComment, - createCheck, - updateCheck, + createCommitStatus, + createLabel, setFailed, listPullRequests: github.rest.pulls.list, } } +/** Returns the most recently created or updated managed guidance comment body. */ function latestGuide(result: Awaited>) { const created = result.createComment.mock.calls.at(-1)?.[0] as { body?: string } | undefined const updated = result.updateComment.mock.calls.at(-1)?.[0] as { body?: string } | undefined return updated?.body ?? created?.body ?? "" } -function latestCheck(result: Awaited>) { - const created = result.createCheck.mock.calls.at(-1)?.[0] as - | { conclusion?: string; output?: { summary?: string } } +/** Returns the latest advisory gate commit-status payload. */ +function latestGateStatus(result: Awaited>) { + return result.createCommitStatus.mock.calls.at(-1)?.[0] as + | { state?: string; description?: string; context?: string; sha?: string } | undefined - const updated = result.updateCheck.mock.calls.at(-1)?.[0] as - | { conclusion?: string; output?: { summary?: string } } - | undefined - return updated ?? created } describe("PR review-state workflow", () => { it("ignores events for closed pull requests", async () => { - const result = await runWorkflow({ prState: "closed", existingGate: true }) + const result = await runWorkflow({ prState: "closed" }) expect(result.addLabels).not.toHaveBeenCalled() expect(result.removeLabel).not.toHaveBeenCalled() expect(result.createComment).not.toHaveBeenCalled() expect(result.updateComment).not.toHaveBeenCalled() - expect(result.createCheck).not.toHaveBeenCalled() - expect(result.updateCheck).not.toHaveBeenCalled() + expect(result.createCommitStatus).not.toHaveBeenCalled() expect(result.setFailed).not.toHaveBeenCalled() }) + it("keeps fork review events read-only", async () => { + const result = await runWorkflow({ eventName: "pull_request_review", fork: true }) + + expect(result.addLabels).not.toHaveBeenCalled() + expect(result.removeLabel).not.toHaveBeenCalled() + expect(result.createComment).not.toHaveBeenCalled() + expect(result.updateComment).not.toHaveBeenCalled() + expect(result.createCommitStatus).not.toHaveBeenCalled() + expect(result.setFailed).not.toHaveBeenCalled() + }) + + it("creates missing workflow labels", async () => { + const result = await runWorkflow({ labelLookupStatus: 404 }) + + expect(result.createLabel).toHaveBeenCalledTimes(4) + }) + + it("propagates non-404 label lookup failures", async () => { + await expect(runWorkflow({ labelLookupStatus: 500 })).rejects.toThrow("Label lookup failed") + }) + it("does not start automatic review for drafts", async () => { const result = await runWorkflow({ draft: true }) @@ -273,14 +305,14 @@ describe("PR review-state workflow", () => { }) expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "coderabbit-review-active" })) - expect(latestCheck(result)?.output?.summary).toContain("required CI checks") + expect(latestGateStatus(result)?.description).toContain("required CI checks") }) it("does not start CodeRabbit when required CI fails", async () => { const result = await runWorkflow({ requiredConclusion: "failure" }) expect(result.addLabels).not.toHaveBeenCalled() - expect(latestCheck(result)?.output?.summary).toContain("Fix the failing required CI checks") + expect(latestGateStatus(result)?.description).toContain("Fix the failing required CI checks") }) it("starts CodeRabbit automatically after required CI passes", async () => { @@ -291,6 +323,16 @@ describe("PR review-state workflow", () => { expect(latestGuide(result)).toContain(`coderabbit-review-label:${SHA}`) }) + it("invalidates the gate before fallible metadata updates", async () => { + const result = await runWorkflow({ addLabelsStatus: 500 }) + + expect(result.setFailed).toHaveBeenCalled() + expect(latestGateStatus(result)?.state).toBe("failure") + expect(result.createCommitStatus.mock.invocationCallOrder[0]).toBeLessThan( + result.addLabels.mock.invocationCallOrder[0], + ) + }) + it("recycles a CodeRabbit label left over from an older head", async () => { const result = await runWorkflow({ labels: ["coderabbit-review-active"], @@ -439,14 +481,15 @@ describe("PR review-state workflow", () => { ], }) - expect(latestCheck(result)?.conclusion).toBe("success") + expect(latestGateStatus(result)?.state).toBe("success") }) - it("updates an existing review-gate check run", async () => { - const result = await runWorkflow({ existingGate: true }) + it("publishes the review gate as a standalone commit status", async () => { + const result = await runWorkflow() - expect(result.updateCheck).toHaveBeenCalledWith(expect.objectContaining({ check_run_id: 500 })) - expect(result.createCheck).not.toHaveBeenCalled() + expect(latestGateStatus(result)).toEqual( + expect.objectContaining({ context: "PR review gate", sha: SHA, state: "failure" }), + ) }) it("reports non-404 permission lookup failures", async () => { @@ -481,7 +524,7 @@ describe("PR review-state workflow", () => { const result = await runWorkflow({ branchRulesFail: true }) expect(result.addLabels).not.toHaveBeenCalled() - expect(latestCheck(result)?.output?.summary).toContain("required CI checks") + expect(latestGateStatus(result)?.description).toContain("required CI checks") }) it("lists open PRs during scheduled reconciliation", async () => { @@ -491,6 +534,27 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) + it("reconciles the PR associated with a workflow run", async () => { + const result = await runWorkflow({ eventName: "workflow_run" }) + + expect(result.listPullRequests).not.toHaveBeenCalled() + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + + it("ignores closed PRs associated with workflow runs", async () => { + const result = await runWorkflow({ eventName: "workflow_run", prState: "closed" }) + + expect(result.createCommitStatus).not.toHaveBeenCalled() + expect(result.addLabels).not.toHaveBeenCalled() + }) + + it("does not list every PR when a workflow run has no associated PR", async () => { + const result = await runWorkflow({ eventName: "workflow_run", workflowRunAssociated: false }) + + expect(result.listPullRequests).not.toHaveBeenCalled() + expect(result.createCommitStatus).not.toHaveBeenCalled() + }) + it("ignores CodeRabbit reviews from an older head", async () => { const result = await runWorkflow({ reviews: [ From 3ac71c5e64c1b8a6c05c1d730a0e145e8e2c3448 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 21:54:19 +0000 Subject: [PATCH 09/29] fix: harden PR review reconciliation --- .github/workflows/label-pr-review-state.yml | 44 ++-- CONTRIBUTING.md | 6 +- .../pr-review-state-workflow.test.ts | 233 +++++++++++++++++- 3 files changed, 255 insertions(+), 28 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 9b7173bfde..667c5b6dda 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -241,14 +241,14 @@ jobs: }); } - function reviewGuideBody(pr, phase) { + function reviewGuideBody(pr, phase, activationPending = false) { const automatedAuthor = pr.user?.type === 'Bot'; const authorNote = automatedAuthor ? 'This PR was opened by an automated account. A human maintainer must verify the change intent, provenance, and validation before merging.' : 'Thanks for contributing. This comment tracks the review sequence and the next action.'; const labelMarker = phase === 'coderabbit' - ? `\n${codeRabbitLabelMarkerPrefix}${pr.head.sha} -->` + ? `\n${codeRabbitLabelMarkerPrefix}${pr.head.sha}${activationPending ? ':pending' : ''} -->` : ''; return `${guideMarker}\n### Review process\n\n${authorNote}\n\n` + @@ -259,24 +259,27 @@ jobs: `**Current step:** ${phaseMessage(phase)}${labelMarker}`; } - async function updateReviewGuide(pr, phase, existingGuide = null) { + async function updateReviewGuide(pr, phase, existingGuide = null, activationPending = false) { if (isReadOnlyRun && isForkPR(pr)) { core.info(`PR #${pr.number}: fork PR on a read-only run — skipping guide update`); return; } - const body = reviewGuideBody(pr, phase); + const body = reviewGuideBody(pr, phase, activationPending); const existing = existingGuide ?? await findReviewGuide(pr); if (!existing) { - await github.rest.issues.createComment({ + const { data: created } = await github.rest.issues.createComment({ owner, repo, issue_number: pr.number, body, }); + return created; } else if (existing.body !== body) { await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body, }); + return { ...existing, body }; } + return existing; } // Fetch required status check names from the branch ruleset. @@ -316,7 +319,7 @@ jobs: for (const pr of prs) { try { - const existingGuide = await findReviewGuide(pr); + let existingGuide = await findReviewGuide(pr); // `mergeable`/`mergeable_state` are only returned by the single-PR GET // endpoint, and are computed asynchronously by GitHub — a PR fetched via @@ -459,10 +462,11 @@ jobs: // block the PR or indicate the author needs to act. const latest = new Map(); for (const r of reviews) { + const reviewer = r.user.login.toLowerCase(); if (r.state === 'DISMISSED') { - latest.delete(r.user.login); + latest.delete(reviewer); } else if (r.state !== 'COMMENTED') { - latest.set(r.user.login, r); + latest.set(reviewer, r); } } @@ -474,7 +478,7 @@ jobs: for (const review of latest.values()) { if (review.commit_id !== pr.head.sha || review.user?.type === 'Bot' || - review.user?.login === pr.user?.login) { + review.user?.login.toLowerCase() === pr.user?.login.toLowerCase()) { continue; } if (['admin', 'maintain', 'write'].includes(await permissionFor(review.user.login))) { @@ -487,10 +491,10 @@ jobs: const codeRabbitApproved = freshCodeRabbitReview?.state === 'APPROVED'; const maintainerApproval = freshMaintainerReviews .filter(review => review.state === 'APPROVED') - .sort((a, b) => Date.parse(b.submitted_at) - Date.parse(a.submitted_at))[0]; + .sort((a, b) => b.id - a.id)[0]; const maintainerApprovedAfterCodeRabbit = codeRabbitApproved && maintainerApproval && - Date.parse(maintainerApproval.submitted_at) > Date.parse(freshCodeRabbitReview.submitted_at); + maintainerApproval.id > freshCodeRabbitReview.id; let desiredLabel; let phase; @@ -509,12 +513,12 @@ jobs: desiredLabel = 'awaiting-coderabbit'; phase = 'coderabbit'; } - } else if (pr.draft) { - desiredLabel = 'awaiting-ready'; - phase = 'draft-approved'; } else if (maintainerChangeRequest) { desiredLabel = 'awaiting-author'; phase = 'maintainer-changes'; + } else if (pr.draft) { + desiredLabel = 'awaiting-ready'; + phase = 'draft-approved'; } else if (!maintainerApprovedAfterCodeRabbit) { desiredLabel = 'awaiting-maintainer'; phase = 'maintainer'; @@ -532,9 +536,19 @@ jobs: if (phase !== 'approved') { await updateReviewGate(pr, phase, false); } + const recyclingActiveLabel = activateCodeRabbit && recycleCodeRabbitLabel && + pr.labels.some(label => label.name === codeRabbitActiveLabel); + existingGuide = await updateReviewGuide( + pr, + phase, + existingGuide, + recyclingActiveLabel, + ); await setCodeRabbitReviewActive(pr, activateCodeRabbit, recycleCodeRabbitLabel); + if (recyclingActiveLabel) { + await updateReviewGuide(pr, phase, existingGuide); + } await reconcileLabels(pr, desiredLabel); - await updateReviewGuide(pr, phase, existingGuide); if (phase === 'approved') { await updateReviewGate(pr, phase, true); } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 05acfae631..4766392c1a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -137,16 +137,18 @@ pnpm install Ready-for-review PRs move through these gates in order: 1. Required CI checks must pass. -2. The workflow automatically starts CodeRabbit review for the latest commit. +2. The workflow automatically applies the managed `coderabbit-review-active` label to start CodeRabbit review for the latest commit. Contributors and maintainers should not manage this label manually. 3. Address any CodeRabbit findings and push updates; review restarts automatically after required CI passes again. 4. After CodeRabbit approval, a non-author maintainer account with write access performs the final review and approval. -An automated comment on each PR shows the current gate and next action. The `awaiting-coderabbit`, `awaiting-maintainer`, `awaiting-author`, and `has-conflicts` labels make the same state visible in the PR list. A new commit invalidates prior approvals; required CI and CodeRabbit rerun for that commit before maintainer review. +An automated comment on each PR shows the current gate and next action. The `awaiting-coderabbit`, `awaiting-ready`, `awaiting-maintainer`, `awaiting-author`, and `has-conflicts` labels make the same state visible in the PR list. A new commit invalidates prior approvals; required CI and CodeRabbit rerun for that commit before maintainer review. The `PR review gate` commit status passes only after the sequence completes. It is advisory by default; repository administrators can make it required on `main` if enforcement is desired. The labels and managed comment remain the maintainer-facing review queue either way. Optional checks such as Codecov do not delay CodeRabbit unless repository rules make them required. Draft PRs are not reviewed automatically, but authors can still request an early review with `@coderabbitai review`. +The workflow reads required checks from the `main` branch ruleset. If those rules cannot be read, the gate fails closed and waits for the hourly reconciliation or a manual workflow run after the ruleset is available again. + PRs opened by bots or other automated accounts follow the same CI and CodeRabbit gates. Final approval requires a non-author, non-bot account with write access, and maintainers remain responsible for verifying the change's intent, provenance, and validation before merging. - **Daily Triage:** Quick checks by maintainers. diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index c7d3189e24..286e21f839 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -27,7 +27,9 @@ interface HarnessOptions { workflowRunAssociated?: boolean existingGuide?: boolean existingGuideHead?: string + existingGuidePendingHead?: string labels?: string[] + prAuthor?: { login: string; type: "Bot" | "User" } addLabelsStatus?: number labelLookupStatus?: number removeLabelStatus?: number @@ -42,10 +44,11 @@ interface HarnessOptions { permissionErrorStatus?: number requiredContexts?: string[] requiredIntegrationId?: number | null + requiredRunAppId?: number requiredStatus?: "queued" | "in_progress" | "completed" requiredConclusion?: "success" | "failure" omitRequiredRuns?: boolean - commitStatusContexts?: string[] + commitStatuses?: Array<{ context: string; state: "pending" | "success" | "failure" | "error"; id?: number }> includeFailedCodecov?: boolean branchRulesFail?: boolean } @@ -58,7 +61,7 @@ async function runWorkflow(options: HarnessOptions = {}) { state: options.prState ?? "open", draft: options.draft ?? false, html_url: "https://github.com/Zoo-Code-Org/Zoo-Code/pull/1437", - user: { login: "zoomote[bot]", type: "Bot" }, + user: options.prAuthor ?? { login: "zoomote[bot]", type: "Bot" }, head: { sha: SHA, repo: { full_name: headRepository } }, base: { ref: "main", repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, labels: (options.labels ?? []).map((name) => ({ name })), @@ -78,7 +81,7 @@ async function runWorkflow(options: HarnessOptions = {}) { : null, started_at: "2026-08-29T15:00:00Z", completed_at: (options.requiredStatus ?? "completed") === "completed" ? "2026-08-29T15:01:00Z" : null, - app: { id: 15368, slug: "github-actions" }, + app: { id: options.requiredRunAppId ?? 15368, slug: "github-actions" }, })) const checkRuns = options.includeFailedCodecov ? [ @@ -102,7 +105,7 @@ async function runWorkflow(options: HarnessOptions = {}) { user: { login: review.login, type: review.type }, })) const existingComments = - options.existingGuide || options.existingGuideHead + options.existingGuide || options.existingGuideHead || options.existingGuidePendingHead ? [ { id: 10, @@ -111,7 +114,9 @@ async function runWorkflow(options: HarnessOptions = {}) { "\n**Current step:** Waiting" + (options.existingGuideHead ? `\n` - : ""), + : options.existingGuidePendingHead + ? `\n` + : ""), }, ] : [] @@ -126,7 +131,9 @@ async function runWorkflow(options: HarnessOptions = {}) { throw Object.assign(new Error("Remove label failed"), { status: options.removeLabelStatus }) } }) - const createComment = vi.fn(async (_args: unknown) => undefined) + const createComment = vi.fn(async (args: { body: string }) => ({ + data: { id: 11, user: { login: "github-actions[bot]" }, body: args.body }, + })) const updateComment = vi.fn(async (_args: unknown) => undefined) const createCommitStatus = vi.fn(async (_args: unknown) => undefined) const createLabel = vi.fn(async (_args: unknown) => undefined) @@ -186,10 +193,10 @@ async function runWorkflow(options: HarnessOptions = {}) { repos: { createCommitStatus, listCommitStatusesForRef: vi.fn(async () => - (options.commitStatusContexts ?? []).map((context, index) => ({ - id: index + 1, - context, - state: "success", + (options.commitStatuses ?? []).map((status, index) => ({ + id: status.id ?? index + 1, + context: status.context, + state: status.state, created_at: "2026-08-29T15:00:00Z", updated_at: "2026-08-29T15:01:00Z", })), @@ -331,6 +338,9 @@ describe("PR review-state workflow", () => { expect(result.createCommitStatus.mock.invocationCallOrder[0]).toBeLessThan( result.addLabels.mock.invocationCallOrder[0], ) + expect(result.createComment.mock.invocationCallOrder[0]).toBeLessThan( + result.addLabels.mock.invocationCallOrder[0], + ) }) it("recycles a CodeRabbit label left over from an older head", async () => { @@ -343,6 +353,18 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) + it("retries a CodeRabbit label recycle left in the pending state", async () => { + const result = await runWorkflow({ + labels: ["coderabbit-review-active"], + existingGuidePendingHead: SHA, + }) + + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "coderabbit-review-active" })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + expect(latestGuide(result)).toContain(`coderabbit-review-label:${SHA} -->`) + expect(latestGuide(result)).not.toContain(":pending") + }) + it("tolerates an already-removed CodeRabbit label while recycling", async () => { const result = await runWorkflow({ labels: ["coderabbit-review-active"], @@ -376,12 +398,46 @@ describe("PR review-state workflow", () => { const result = await runWorkflow({ requiredIntegrationId: null, omitRequiredRuns: true, - commitStatusContexts: ["tests"], + commitStatuses: [{ context: "tests", state: "success" }], }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) + it("uses only the latest legacy status for a required context", async () => { + const result = await runWorkflow({ + requiredIntegrationId: null, + omitRequiredRuns: true, + commitStatuses: [ + { id: 1, context: "tests", state: "failure" }, + { id: 2, context: "tests", state: "success" }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + + it("does not use a legacy status for an integration-pinned check", async () => { + const result = await runWorkflow({ + requiredRunAppId: 999, + commitStatuses: [{ context: "tests", state: "success" }], + }) + + expect(result.addLabels).not.toHaveBeenCalledWith( + expect.objectContaining({ labels: ["coderabbit-review-active"] }), + ) + expect(latestGateStatus(result)?.description).toContain("required CI checks") + }) + + it("keeps the gate pending when a required check has not reported", async () => { + const result = await runWorkflow({ omitRequiredRuns: true }) + + expect(result.addLabels).not.toHaveBeenCalledWith( + expect.objectContaining({ labels: ["coderabbit-review-active"] }), + ) + expect(latestGateStatus(result)?.description).toContain("required CI checks") + }) + it("removes CodeRabbit activation when the PR has conflicts", async () => { const result = await runWorkflow({ conflict: true, @@ -424,6 +480,63 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) }) + it("recognizes CodeRabbit regardless of login casing", async () => { + const result = await runWorkflow({ + reviews: [ + { + login: "CodeRabbitAI[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + }) + + it("preserves CodeRabbit approval after a later comment", async () => { + const result = await runWorkflow({ + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "coderabbitai[bot]", + type: "Bot", + state: "COMMENTED", + submittedAt: REVIEWED_AT + 1_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + }) + + it("invalidates a dismissed CodeRabbit approval", async () => { + const result = await runWorkflow({ + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "coderabbitai[bot]", + type: "Bot", + state: "DISMISSED", + submittedAt: REVIEWED_AT + 1_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) + }) + it("preserves manual draft approvals until the PR is ready", async () => { const result = await runWorkflow({ draft: true, @@ -462,6 +575,82 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) }) + it("does not count the PR author's own approval", async () => { + const result = await runWorkflow({ + prAuthor: { login: "author", type: "User" }, + permissions: { author: "write" }, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "Author", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT + 1_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + expect(latestGateStatus(result)?.state).toBe("failure") + }) + + it("keeps awaiting-author when any maintainer requests changes", async () => { + const result = await runWorkflow({ + permissions: { reviewer: "write", approver: "maintain" }, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "reviewer", + type: "User", + state: "CHANGES_REQUESTED", + submittedAt: REVIEWED_AT + 1_000, + }, + { + 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("failure") + }) + + it("keeps draft PRs awaiting the author when a maintainer requests changes", async () => { + const result = await runWorkflow({ + draft: true, + permissions: { maintainer: "write" }, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "maintainer", + type: "User", + state: "CHANGES_REQUESTED", + submittedAt: REVIEWED_AT + 1_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + }) + it("passes only after a later non-author maintainer approval", async () => { const result = await runWorkflow({ permissions: { maintainer: "write" }, @@ -484,6 +673,28 @@ describe("PR review-state workflow", () => { expect(latestGateStatus(result)?.state).toBe("success") }) + it("uses review order when approvals share the same timestamp", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "maintainer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(latestGateStatus(result)?.state).toBe("success") + }) + it("publishes the review gate as a standalone commit status", async () => { const result = await runWorkflow() From 5c31f23d3bb71a63d6e3825aea7d26b61c3930f0 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 22:18:13 +0000 Subject: [PATCH 10/29] fix: retry transient review workflow API failures --- .github/workflows/label-pr-review-state.yml | 18 +++++++++++------- .../__tests__/pr-review-state-workflow.test.ts | 2 ++ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 667c5b6dda..f227134e5d 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -31,6 +31,7 @@ jobs: - name: Reconcile PR review state labels uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8 with: + retries: 3 script: | const { owner, repo } = context.repo; const stateLabels = [ @@ -343,14 +344,17 @@ jobs: // Check CI status for required checks on the PR's head commit only. // Scoping to required checks avoids advisory checks (e.g. codecov/patch) // incorrectly blocking label assignment on otherwise-ready PRs. - const [checkRuns, commitStatuses] = await Promise.all([ - github.paginate(github.rest.checks.listForRef, { - owner, repo, ref: pr.head.sha, per_page: 100, - }), - github.paginate(github.rest.repos.listCommitStatusesForRef, { + const checkRuns = await github.paginate(github.rest.checks.listForRef, { + owner, repo, ref: pr.head.sha, per_page: 100, + }); + const needsLegacyStatuses = requiredChecks?.some( + check => check.integrationId === null + ); + const commitStatuses = needsLegacyStatuses + ? await github.paginate(github.rest.repos.listCommitStatusesForRef, { owner, repo, ref: pr.head.sha, per_page: 100, - }), - ]); + }) + : []; // listForRef returns every check run ever recorded on the ref, including // stale superseded ones (e.g. a failed run later re-run green). Branch diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 286e21f839..0b2376e47f 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -248,6 +248,7 @@ async function runWorkflow(options: HarnessOptions = {}) { createLabel, setFailed, listPullRequests: github.rest.pulls.list, + listCommitStatusesForRef: github.rest.repos.listCommitStatusesForRef, } } @@ -426,6 +427,7 @@ describe("PR review-state workflow", () => { expect(result.addLabels).not.toHaveBeenCalledWith( expect.objectContaining({ labels: ["coderabbit-review-active"] }), ) + expect(result.listCommitStatusesForRef).not.toHaveBeenCalled() expect(latestGateStatus(result)?.description).toContain("required CI checks") }) From 9506ab8e5370a08d92d575ccc3efe2dd2f722639 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 22:36:32 +0000 Subject: [PATCH 11/29] fix: keep advisory review gate pending --- .github/workflows/label-pr-review-state.yml | 2 +- src/services/__tests__/pr-review-state-workflow.test.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index f227134e5d..7deeeaa6e9 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -235,7 +235,7 @@ jobs: owner, repo, sha: pr.head.sha, - state: passed ? 'success' : 'failure', + state: passed ? 'success' : 'pending', context: reviewGateName, description: phaseMessage(phase), target_url: pr.html_url, diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 0b2376e47f..f40bcd5ed3 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -335,7 +335,7 @@ describe("PR review-state workflow", () => { const result = await runWorkflow({ addLabelsStatus: 500 }) expect(result.setFailed).toHaveBeenCalled() - expect(latestGateStatus(result)?.state).toBe("failure") + expect(latestGateStatus(result)?.state).toBe("pending") expect(result.createCommitStatus.mock.invocationCallOrder[0]).toBeLessThan( result.addLabels.mock.invocationCallOrder[0], ) @@ -598,7 +598,7 @@ describe("PR review-state workflow", () => { }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) - expect(latestGateStatus(result)?.state).toBe("failure") + expect(latestGateStatus(result)?.state).toBe("pending") }) it("keeps awaiting-author when any maintainer requests changes", async () => { @@ -627,7 +627,7 @@ describe("PR review-state workflow", () => { }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) - expect(latestGateStatus(result)?.state).toBe("failure") + expect(latestGateStatus(result)?.state).toBe("pending") }) it("keeps draft PRs awaiting the author when a maintainer requests changes", async () => { @@ -701,7 +701,7 @@ describe("PR review-state workflow", () => { const result = await runWorkflow() expect(latestGateStatus(result)).toEqual( - expect.objectContaining({ context: "PR review gate", sha: SHA, state: "failure" }), + expect.objectContaining({ context: "PR review gate", sha: SHA, state: "pending" }), ) }) From 19db556545e419cad91446f929815aed739b2325 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 22:56:12 +0000 Subject: [PATCH 12/29] fix: deduplicate PR review gate statuses --- .github/workflows/label-pr-review-state.yml | 36 ++++++--- .../pr-review-state-workflow.test.ts | 76 +++++++++++++++++-- 2 files changed, 96 insertions(+), 16 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 7deeeaa6e9..0c1c00b3a8 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -231,15 +231,33 @@ jobs: async function updateReviewGate(pr, phase, passed) { if (isReadOnlyRun && isForkPR(pr)) return; - await github.rest.repos.createCommitStatus({ - owner, - repo, - sha: pr.head.sha, - state: passed ? 'success' : 'pending', - context: reviewGateName, - description: phaseMessage(phase), - target_url: pr.html_url, - }); + const state = passed ? 'success' : 'pending'; + const description = phaseMessage(phase); + try { + const { data: combinedStatus } = await github.rest.repos.getCombinedStatusForRef({ + owner, repo, ref: pr.head.sha, + }); + const latestGateStatus = combinedStatus.statuses + .filter(status => status.context === reviewGateName) + .sort((a, b) => b.id - a.id)[0]; + if (latestGateStatus?.state === state && + latestGateStatus.description === description && + latestGateStatus.target_url === pr.html_url) { + return; + } + + await github.rest.repos.createCommitStatus({ + owner, + repo, + sha: pr.head.sha, + state, + context: reviewGateName, + description, + target_url: pr.html_url, + }); + } catch (error) { + core.warning(`PR #${pr.number}: could not publish ${reviewGateName}: ${error.message}`); + } } function reviewGuideBody(pr, phase, activationPending = false) { diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index f40bcd5ed3..00c3992cbe 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -49,6 +49,15 @@ interface HarnessOptions { requiredConclusion?: "success" | "failure" omitRequiredRuns?: boolean commitStatuses?: Array<{ context: string; state: "pending" | "success" | "failure" | "error"; id?: number }> + gateStatuses?: Array<{ + context: string + state: "pending" | "success" | "failure" | "error" + description: string + targetUrl: string + id?: number + }> + gateStatusLookupErrorStatus?: number + createCommitStatusErrorStatus?: number includeFailedCodecov?: boolean branchRulesFail?: boolean } @@ -134,8 +143,19 @@ async function runWorkflow(options: HarnessOptions = {}) { const createComment = vi.fn(async (args: { body: string }) => ({ data: { id: 11, user: { login: "github-actions[bot]" }, body: args.body }, })) - const updateComment = vi.fn(async (_args: unknown) => undefined) - const createCommitStatus = vi.fn(async (_args: unknown) => undefined) + const updateComment = vi.fn(async (args: { comment_id: number; body: string }) => ({ + data: { id: args.comment_id, user: { login: "github-actions[bot]" }, body: args.body }, + })) + const createCommitStatus = vi.fn( + async (args: { sha: string; state: string; context: string; description: string; target_url: string }) => { + if (options.createCommitStatusErrorStatus) { + throw Object.assign(new Error("Commit status failed"), { + status: options.createCommitStatusErrorStatus, + }) + } + return { data: args } + }, + ) const createLabel = vi.fn(async (_args: unknown) => undefined) const setFailed = vi.fn() const permissionFor = vi.fn(async ({ username }: { username: string }) => { @@ -201,6 +221,24 @@ async function runWorkflow(options: HarnessOptions = {}) { updated_at: "2026-08-29T15:01:00Z", })), ), + getCombinedStatusForRef: vi.fn(async () => { + if (options.gateStatusLookupErrorStatus) { + throw Object.assign(new Error("Gate status lookup failed"), { + status: options.gateStatusLookupErrorStatus, + }) + } + return { + data: { + statuses: (options.gateStatuses ?? []).map((status, index) => ({ + id: status.id ?? index + 1, + context: status.context, + state: status.state, + description: status.description, + target_url: status.targetUrl, + })), + }, + } + }), getCollaboratorPermissionLevel: permissionFor, }, }, @@ -247,6 +285,7 @@ async function runWorkflow(options: HarnessOptions = {}) { createCommitStatus, createLabel, setFailed, + warning: core.warning, listPullRequests: github.rest.pulls.list, listCommitStatusesForRef: github.rest.repos.listCommitStatusesForRef, } @@ -254,16 +293,14 @@ async function runWorkflow(options: HarnessOptions = {}) { /** Returns the most recently created or updated managed guidance comment body. */ function latestGuide(result: Awaited>) { - const created = result.createComment.mock.calls.at(-1)?.[0] as { body?: string } | undefined - const updated = result.updateComment.mock.calls.at(-1)?.[0] as { body?: string } | undefined + const created = result.createComment.mock.calls.at(-1)?.[0] + const updated = result.updateComment.mock.calls.at(-1)?.[0] return updated?.body ?? created?.body ?? "" } /** Returns the latest advisory gate commit-status payload. */ function latestGateStatus(result: Awaited>) { - return result.createCommitStatus.mock.calls.at(-1)?.[0] as - | { state?: string; description?: string; context?: string; sha?: string } - | undefined + return result.createCommitStatus.mock.calls.at(-1)?.[0] } describe("PR review-state workflow", () => { @@ -705,6 +742,31 @@ describe("PR review-state workflow", () => { ) }) + it("does not republish an unchanged review gate status", async () => { + const result = await runWorkflow({ + gateStatuses: [ + { + context: "PR review gate", + state: "pending", + description: "Required CI passed. Wait for CodeRabbit to approve the latest commit.", + targetUrl: "https://github.com/Zoo-Code-Org/Zoo-Code/pull/1437", + }, + ], + }) + + expect(result.createCommitStatus).not.toHaveBeenCalled() + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) + }) + + it("continues metadata reconciliation when gate publication fails", async () => { + const result = await runWorkflow({ createCommitStatusErrorStatus: 500 }) + + expect(result.setFailed).not.toHaveBeenCalled() + expect(result.warning).toHaveBeenCalledWith(expect.stringContaining("could not publish PR review gate")) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + expect(latestGuide(result)).toContain(`coderabbit-review-label:${SHA}`) + }) + it("reports non-404 permission lookup failures", async () => { const result = await runWorkflow({ permissionErrorStatus: 500, From f64a0f92a7a3f6aa78b5d1e51ac60086ddb5ef22 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sat, 29 Aug 2026 23:52:13 +0000 Subject: [PATCH 13/29] fix: preserve required reconcile checks --- .github/workflows/label-pr-review-state.yml | 26 ++++++++-------- .../pr-review-state-workflow.test.ts | 30 +++++++++++++++++++ 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 0c1c00b3a8..061a4da57a 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -233,19 +233,24 @@ jobs: const state = passed ? 'success' : 'pending'; const description = phaseMessage(phase); + let latestGateStatus = null; try { const { data: combinedStatus } = await github.rest.repos.getCombinedStatusForRef({ owner, repo, ref: pr.head.sha, }); - const latestGateStatus = combinedStatus.statuses + latestGateStatus = combinedStatus.statuses .filter(status => status.context === reviewGateName) .sort((a, b) => b.id - a.id)[0]; - if (latestGateStatus?.state === state && - latestGateStatus.description === description && - latestGateStatus.target_url === pr.html_url) { - return; - } + } catch (error) { + core.warning(`PR #${pr.number}: could not inspect ${reviewGateName}: ${error.message}`); + } + if (latestGateStatus?.state === state && + latestGateStatus.description === description && + latestGateStatus.target_url === pr.html_url) { + return; + } + try { await github.rest.repos.createCommitStatus({ owner, repo, @@ -408,13 +413,10 @@ jobs: } // Filter to required checks only (or all checks if rules unavailable). - // Always exclude this workflow's own run to avoid self-referential loops. - const excludedCheckNames = new Set([ - 'reconcile', - reviewGateName, - ]); + // Always exclude this workflow's own run to avoid self-referential loops. const requiredSpecs = (requiredChecks ?? []) - .filter(check => !excludedCheckNames.has(check.context)); + .filter(check => check.context !== reviewGateName) + .filter(check => !(check.context === 'reconcile' && check.integrationId === 15368)); const requiredResults = requiredSpecs.map(check => { const run = check.integrationId === null ? latestByName.get(check.context) diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 00c3992cbe..ff39849edc 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -326,6 +326,14 @@ describe("PR review-state workflow", () => { expect(result.setFailed).not.toHaveBeenCalled() }) + it("reconciles fork PRs from pull_request_target", async () => { + const result = await runWorkflow({ eventName: "pull_request_target", fork: true }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + expect(result.createCommitStatus).toHaveBeenCalled() + expect(result.setFailed).not.toHaveBeenCalled() + }) + it("creates missing workflow labels", async () => { const result = await runWorkflow({ labelLookupStatus: 404 }) @@ -767,6 +775,15 @@ describe("PR review-state workflow", () => { expect(latestGuide(result)).toContain(`coderabbit-review-label:${SHA}`) }) + it("publishes the gate and continues reconciliation when status lookup fails", async () => { + const result = await runWorkflow({ gateStatusLookupErrorStatus: 500 }) + + expect(result.setFailed).not.toHaveBeenCalled() + expect(result.warning).toHaveBeenCalledWith(expect.stringContaining("could not inspect PR review gate")) + expect(result.createCommitStatus).toHaveBeenCalled() + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + it("reports non-404 permission lookup failures", async () => { const result = await runWorkflow({ permissionErrorStatus: 500, @@ -795,6 +812,19 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) + it("does not exclude a reconcile check from another integration", async () => { + const result = await runWorkflow({ + requiredContexts: ["reconcile"], + requiredIntegrationId: 999, + requiredRunAppId: 15368, + }) + + expect(result.addLabels).not.toHaveBeenCalledWith( + expect.objectContaining({ labels: ["coderabbit-review-active"] }), + ) + expect(latestGateStatus(result)?.description).toContain("required CI checks") + }) + it("fails closed when branch rules are unavailable", async () => { const result = await runWorkflow({ branchRulesFail: true }) From 2d866c6d2f4dc09f27c7be0896106cea5f417819 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 00:21:43 +0000 Subject: [PATCH 14/29] fix: enforce CodeRabbit pre-merge checks --- .github/workflows/label-pr-review-state.yml | 79 +++++-- CONTRIBUTING.md | 6 +- .../pr-review-state-workflow.test.ts | 204 +++++++++++++++--- 3 files changed, 245 insertions(+), 44 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 061a4da57a..8a133161a8 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -4,6 +4,11 @@ on: schedule: - cron: "0 * * * *" # hourly fallback workflow_dispatch: + inputs: + pull_request_number: + description: Pull request number to reconcile + required: true + type: number # 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: @@ -65,6 +70,7 @@ jobs: }, ]; const guideMarker = ''; + const codeRabbitSummaryMarker = ''; const codeRabbitLabelMarkerPrefix = '')) { + return 'pending'; + } + + const errorCount = Number(body.match(/Failed checks \((\d+) errors?/i)?.[1] ?? 0); + return errorCount > 0 ? 'failed' : 'passed'; + } + async function setCodeRabbitReviewActive(pr, enabled, recycle = false) { if (isReadOnlyRun && isForkPR(pr)) return; @@ -218,17 +248,17 @@ jobs: 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.', - 'coderabbit-changes': 'Address CodeRabbit findings and push an update. Review restarts after required 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 for maintainer review.', + 'coderabbit-changes': 'Address CodeRabbit findings and blocking pre-merge checks, then push an update.', + coderabbit: 'Wait for CodeRabbit approval and blocking pre-merge checks to pass.', + 'draft-approved': 'CodeRabbit approval and blocking pre-merge checks passed. Mark the draft ready.', 'maintainer-changes': 'Address the maintainer feedback, push an update, and request another review.', - maintainer: 'CodeRabbit approved the latest commit. A maintainer must now review and approve it.', - approved: 'CodeRabbit and a maintainer approved the latest commit. The PR is ready for the remaining merge requirements.', + maintainer: 'CodeRabbit approval and blocking pre-merge checks passed. A maintainer must now approve.', + approved: 'CodeRabbit pre-merge checks and both required approvals passed. Remaining merge requirements apply.', }; return messages[phase]; } - async function updateReviewGate(pr, phase, passed) { + async function updateReviewGate(pr, phase, passed, required = false) { if (isReadOnlyRun && isForkPR(pr)) return; const state = passed ? 'success' : 'pending'; @@ -261,6 +291,7 @@ jobs: target_url: pr.html_url, }); } catch (error) { + if (required) throw error; core.warning(`PR #${pr.number}: could not publish ${reviewGateName}: ${error.message}`); } } @@ -278,7 +309,7 @@ jobs: return `${guideMarker}\n### Review process\n\n${authorNote}\n\n` + '1. Required CI checks pass.\n' + '2. The workflow starts CodeRabbit automatically.\n' + - '3. CodeRabbit reviews and approves the latest commit.\n' + + '3. CodeRabbit approves the latest commit with no blocking pre-merge check errors.\n' + '4. A human maintainer reviews and approves after CodeRabbit.\n\n' + `**Current step:** ${phaseMessage(phase)}${labelMarker}`; } @@ -476,6 +507,8 @@ jobs: } // CI is passing. Now determine review state. + const codeRabbitSummary = await findCodeRabbitSummary(pr); + const codeRabbitPreMerge = codeRabbitPreMergeState(codeRabbitSummary, pr.head.sha); const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: pr.number, per_page: 100, }); @@ -512,7 +545,10 @@ jobs: const maintainerChangeRequest = freshMaintainerReviews.find( review => review.state === 'CHANGES_REQUESTED' ); - const codeRabbitApproved = freshCodeRabbitReview?.state === 'APPROVED'; + const codeRabbitApproved = freshCodeRabbitReview?.state === 'APPROVED' && + codeRabbitPreMerge === 'passed'; + const codeRabbitChangesRequested = freshCodeRabbitReview?.state === 'CHANGES_REQUESTED' || + codeRabbitPreMerge === 'failed'; const maintainerApproval = freshMaintainerReviews .filter(review => review.state === 'APPROVED') .sort((a, b) => b.id - a.id)[0]; @@ -524,7 +560,7 @@ jobs: let phase; let activateCodeRabbit = false; let recycleCodeRabbitLabel = false; - if (freshCodeRabbitReview?.state === 'CHANGES_REQUESTED') { + if (codeRabbitChangesRequested) { desiredLabel = 'awaiting-author'; phase = 'coderabbit-changes'; } else if (!codeRabbitApproved) { @@ -553,7 +589,7 @@ jobs: core.info( `PR #${pr.number}: CI passing, reviews=${latest.size}, ` + - `coderabbit=${freshCodeRabbitReview?.state ?? 'pending'}, ` + + `coderabbit=${freshCodeRabbitReview?.state ?? 'pending'}/${codeRabbitPreMerge}, ` + `maintainer=${maintainerApproval?.state ?? 'pending'} → ${desiredLabel ?? '(none)'}` ); @@ -577,11 +613,30 @@ jobs: await updateReviewGate(pr, phase, true); } } catch (error) { + let invalidationError = null; + let metadataError = null; + try { + await updateReviewGate(pr, 'ci-pending', false, true); + } catch (gateError) { + invalidationError = gateError; + } + try { + await setCodeRabbitReviewActive(pr, false); + await reconcileLabels(pr, null); + } catch (cleanupError) { + metadataError = cleanupError; + } const detail = error.status ? `${error.message} (HTTP ${error.status}${error.response?.data?.message ? `: ${error.response.data.message}` : ''})` : error.message; - failures.push(`#${pr.number}: ${detail}`); - core.error(`Failed to reconcile PR #${pr.number}: ${detail}`); + const gateDetail = invalidationError + ? `; could not invalidate ${reviewGateName}: ${invalidationError.message}` + : ''; + const metadataDetail = metadataError + ? `; could not clear review metadata: ${metadataError.message}` + : ''; + failures.push(`#${pr.number}: ${detail}${gateDetail}${metadataDetail}`); + core.error(`Failed to reconcile PR #${pr.number}: ${detail}${gateDetail}${metadataDetail}`); } } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4766392c1a..1e1bd85915 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -138,11 +138,13 @@ Ready-for-review PRs move through these gates in order: 1. Required CI checks must pass. 2. The workflow automatically applies the managed `coderabbit-review-active` label to start CodeRabbit review for the latest commit. Contributors and maintainers should not manage this label manually. -3. Address any CodeRabbit findings and push updates; review restarts automatically after required CI passes again. -4. After CodeRabbit approval, a non-author maintainer account with write access performs the final review and approval. +3. Address CodeRabbit findings and every error in its persistent **Pre-merge checks** summary. Warnings are advisory unless repository policy says otherwise. Review restarts automatically after required CI passes again. +4. After CodeRabbit approves the current commit and its blocking pre-merge checks pass, a non-author maintainer account with write access performs the final review and approval. An automated comment on each PR shows the current gate and next action. The `awaiting-coderabbit`, `awaiting-ready`, `awaiting-maintainer`, `awaiting-author`, and `has-conflicts` labels make the same state visible in the PR list. A new commit invalidates prior approvals; required CI and CodeRabbit rerun for that commit before maintainer review. +CodeRabbit's green commit status only means its review completed; it does not prove that custom pre-merge checks passed. Contributors, maintainers, and automated PR fixers must inspect CodeRabbit's persistent summary comment, resolve all **Error** entries under **Pre-merge checks**, and report that state explicitly in PR update comments. + The `PR review gate` commit status passes only after the sequence completes. It is advisory by default; repository administrators can make it required on `main` if enforcement is desired. The labels and managed comment remain the maintainer-facing review queue either way. Optional checks such as Codecov do not delay CodeRabbit unless repository rules make them required. Draft PRs are not reviewed automatically, but authors can still request an early review with `@coderabbitai review`. diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index ff39849edc..18f27e9963 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -22,12 +22,17 @@ interface HarnessOptions { prState?: "open" | "closed" draft?: boolean conflict?: boolean + mergeable?: boolean | null + mergeableState?: string fork?: boolean eventName?: string workflowRunAssociated?: boolean + workflowDispatchPrNumber?: number existingGuide?: boolean existingGuideHead?: string existingGuidePendingHead?: string + codeRabbitSummaryHead?: string | null + codeRabbitPreMergeErrors?: number labels?: string[] prAuthor?: { login: string; type: "Bot" | "User" } addLabelsStatus?: number @@ -59,6 +64,13 @@ interface HarnessOptions { gateStatusLookupErrorStatus?: number createCommitStatusErrorStatus?: number includeFailedCodecov?: boolean + additionalCheckRuns?: Array<{ + id: number + name: string + status: "queued" | "in_progress" | "completed" + conclusion: "success" | "failure" | null + appId: number + }> branchRulesFail?: boolean } @@ -74,8 +86,8 @@ async function runWorkflow(options: HarnessOptions = {}) { head: { sha: SHA, repo: { full_name: headRepository } }, base: { ref: "main", repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, labels: (options.labels ?? []).map((name) => ({ name })), - mergeable: options.conflict ? false : true, - mergeable_state: options.conflict ? "dirty" : "clean", + mergeable: options.mergeable !== undefined ? options.mergeable : options.conflict ? false : true, + mergeable_state: options.mergeableState ?? (options.conflict ? "dirty" : "clean"), } const requiredContexts = options.requiredContexts ?? ["tests"] const requiredRuns = (options.omitRequiredRuns ? [] : requiredContexts) @@ -92,20 +104,31 @@ async function runWorkflow(options: HarnessOptions = {}) { completed_at: (options.requiredStatus ?? "completed") === "completed" ? "2026-08-29T15:01:00Z" : null, app: { id: options.requiredRunAppId ?? 15368, slug: "github-actions" }, })) - const checkRuns = options.includeFailedCodecov - ? [ - ...requiredRuns, - { - id: 100, - name: "codecov/patch", - status: "completed", - conclusion: "failure", - started_at: "2026-08-29T15:00:00Z", - completed_at: "2026-08-29T15:01:00Z", - app: { id: 254, slug: "codecov" }, - }, - ] - : requiredRuns + const checkRuns = [ + ...requiredRuns, + ...(options.additionalCheckRuns ?? []).map((run) => ({ + id: run.id, + name: run.name, + status: run.status, + conclusion: run.conclusion, + started_at: "2026-08-29T14:00:00Z", + completed_at: run.status === "completed" ? "2026-08-29T14:01:00Z" : null, + app: { id: run.appId, slug: "test-app" }, + })), + ...(options.includeFailedCodecov + ? [ + { + id: 100, + name: "codecov/patch", + status: "completed", + conclusion: "failure", + started_at: "2026-08-29T15:00:00Z", + completed_at: "2026-08-29T15:01:00Z", + app: { id: 254, slug: "codecov" }, + }, + ] + : []), + ] const reviews = (options.reviews ?? []).map((review, index) => ({ id: index + 1, state: review.state, @@ -113,8 +136,8 @@ async function runWorkflow(options: HarnessOptions = {}) { submitted_at: new Date(review.submittedAt).toISOString(), user: { login: review.login, type: review.type }, })) - const existingComments = - options.existingGuide || options.existingGuideHead || options.existingGuidePendingHead + const existingComments = [ + ...(options.existingGuide || options.existingGuideHead || options.existingGuidePendingHead ? [ { id: 10, @@ -128,7 +151,21 @@ async function runWorkflow(options: HarnessOptions = {}) { : ""), }, ] - : [] + : []), + ...(options.codeRabbitSummaryHead === null + ? [] + : [ + { + id: 20, + user: { login: "coderabbitai[bot]" }, + body: + "\n" + + `**Merge Risk:** Moderate · up to \`${options.codeRabbitSummaryHead ?? SHA}\`\n` + + "\n" + + `### ❌ Failed checks (${options.codeRabbitPreMergeErrors ?? 0} errors, 0 warnings)`, + }, + ]), + ] const addLabels = vi.fn(async (_args: unknown) => { if (options.addLabelsStatus) { @@ -252,16 +289,18 @@ async function runWorkflow(options: HarnessOptions = {}) { const payload = eventName === "schedule" ? {} - : eventName === "workflow_run" - ? { - workflow_run: { - pull_requests: options.workflowRunAssociated === false ? [] : [{ number: 1437 }], - }, - } - : { - action: "ready_for_review", - pull_request: pullRequestPayload, - } + : eventName === "workflow_dispatch" + ? { inputs: { pull_request_number: String(options.workflowDispatchPrNumber ?? 1437) } } + : eventName === "workflow_run" + ? { + workflow_run: { + pull_requests: options.workflowRunAssociated === false ? [] : [{ number: 1437 }], + }, + } + : { + action: "ready_for_review", + pull_request: pullRequestPayload, + } const context = { eventName, repo: { owner: "Zoo-Code-Org", repo: "Zoo-Code" }, @@ -440,6 +479,35 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) + it("uses the latest run for the required integration", async () => { + const result = await runWorkflow({ + additionalCheckRuns: [{ id: 0, name: "tests", status: "completed", conclusion: "failure", appId: 15368 }], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + + it("ignores a newer same-name run from another integration", async () => { + const result = await runWorkflow({ + additionalCheckRuns: [{ id: 100, name: "tests", status: "completed", conclusion: "failure", appId: 999 }], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + + it("allows an empty required-check ruleset", async () => { + const result = await runWorkflow({ requiredContexts: [] }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + + it("does not treat unknown mergeability as a conflict", async () => { + const result = await runWorkflow({ mergeable: null, mergeableState: "unknown" }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + expect(result.addLabels).not.toHaveBeenCalledWith(expect.objectContaining({ labels: ["has-conflicts"] })) + }) + it("uses a legacy commit status for an unpinned required context", async () => { const result = await runWorkflow({ requiredIntegrationId: null, @@ -527,6 +595,39 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) }) + it("routes blocking CodeRabbit pre-merge errors back to the author", async () => { + const result = await runWorkflow({ + codeRabbitPreMergeErrors: 1, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(latestGateStatus(result)?.description).toContain("blocking pre-merge checks") + }) + + it("ignores CodeRabbit pre-merge results from an older head", async () => { + const result = await runWorkflow({ + codeRabbitSummaryHead: OLD_SHA, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) + }) + it("recognizes CodeRabbit regardless of login casing", async () => { const result = await runWorkflow({ reviews: [ @@ -646,6 +747,30 @@ describe("PR review-state workflow", () => { expect(latestGateStatus(result)?.state).toBe("pending") }) + it("ignores maintainer approvals from an older head", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "maintainer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT + 1_000, + commitId: OLD_SHA, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + expect(latestGateStatus(result)?.state).toBe("pending") + }) + it("keeps awaiting-author when any maintainer requests changes", async () => { const result = await runWorkflow({ permissions: { reviewer: "write", approver: "maintain" }, @@ -756,7 +881,7 @@ describe("PR review-state workflow", () => { { context: "PR review gate", state: "pending", - description: "Required CI passed. Wait for CodeRabbit to approve the latest commit.", + description: "Wait for CodeRabbit approval and blocking pre-merge checks to pass.", targetUrl: "https://github.com/Zoo-Code-Org/Zoo-Code/pull/1437", }, ], @@ -786,6 +911,15 @@ describe("PR review-state workflow", () => { it("reports non-404 permission lookup failures", async () => { const result = await runWorkflow({ + labels: ["awaiting-maintainer", "coderabbit-review-active"], + gateStatuses: [ + { + context: "PR review gate", + state: "success", + description: "Approved", + targetUrl: "https://github.com/Zoo-Code-Org/Zoo-Code/pull/1437", + }, + ], permissionErrorStatus: 500, reviews: [ { @@ -804,6 +938,9 @@ describe("PR review-state workflow", () => { }) expect(result.setFailed).toHaveBeenCalled() + expect(latestGateStatus(result)?.state).toBe("pending") + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-maintainer" })) + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "coderabbit-review-active" })) }) it("excludes the reconciliation job from required checks", async () => { @@ -839,6 +976,13 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) + it("reconciles only the requested PR during manual dispatch", async () => { + const result = await runWorkflow({ eventName: "workflow_dispatch", workflowDispatchPrNumber: 1437 }) + + expect(result.listPullRequests).not.toHaveBeenCalled() + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + it("reconciles the PR associated with a workflow run", async () => { const result = await runWorkflow({ eventName: "workflow_run" }) From 02a0ff7ed72a7ffdf33bd329494dd1c54c599fe0 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 00:51:29 +0000 Subject: [PATCH 15/29] fix: make CodeRabbit optional for bot PRs --- .github/workflows/label-pr-review-state.yml | 67 +++----- CONTRIBUTING.md | 10 +- .../pr-review-state-workflow.test.ts | 154 ++++++++++++------ 3 files changed, 133 insertions(+), 98 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 8a133161a8..b0d6e0f59e 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -70,7 +70,6 @@ jobs: }, ]; const guideMarker = ''; - const codeRabbitSummaryMarker = ''; const codeRabbitLabelMarkerPrefix = '')) { - return 'pending'; - } - - const errorCount = Number(body.match(/Failed checks \((\d+) errors?/i)?.[1] ?? 0); - return errorCount > 0 ? 'failed' : 'passed'; - } - async function setCodeRabbitReviewActive(pr, enabled, recycle = false) { if (isReadOnlyRun && isForkPR(pr)) return; @@ -248,12 +225,12 @@ jobs: 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.', - 'coderabbit-changes': 'Address CodeRabbit findings and blocking pre-merge checks, then push an update.', - coderabbit: 'Wait for CodeRabbit approval and blocking pre-merge checks to pass.', - 'draft-approved': 'CodeRabbit approval and blocking pre-merge checks passed. Mark the draft ready.', + '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: 'CodeRabbit approval and blocking pre-merge checks passed. A maintainer must now approve.', - approved: 'CodeRabbit pre-merge checks and both required approvals passed. Remaining merge requirements apply.', + maintainer: 'A human maintainer must now review and approve the latest commit.', + approved: 'The required review sequence passed. Remaining merge requirements apply.', }; return messages[phase]; } @@ -264,16 +241,19 @@ jobs: const state = passed ? 'success' : 'pending'; const description = phaseMessage(phase); let latestGateStatus = null; + let lookupSucceeded = false; try { const { data: combinedStatus } = await github.rest.repos.getCombinedStatusForRef({ owner, repo, ref: pr.head.sha, }); + lookupSucceeded = true; latestGateStatus = combinedStatus.statuses .filter(status => status.context === reviewGateName) .sort((a, b) => b.id - a.id)[0]; } catch (error) { core.warning(`PR #${pr.number}: could not inspect ${reviewGateName}: ${error.message}`); } + if (!lookupSucceeded && !required) return; if (latestGateStatus?.state === state && latestGateStatus.description === description && latestGateStatus.target_url === pr.html_url) { @@ -309,7 +289,7 @@ jobs: return `${guideMarker}\n### Review process\n\n${authorNote}\n\n` + '1. Required CI checks pass.\n' + '2. The workflow starts CodeRabbit automatically.\n' + - '3. CodeRabbit approves the latest commit with no blocking pre-merge check errors.\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}`; } @@ -507,8 +487,6 @@ jobs: } // CI is passing. Now determine review state. - const codeRabbitSummary = await findCodeRabbitSummary(pr); - const codeRabbitPreMerge = codeRabbitPreMergeState(codeRabbitSummary, pr.head.sha); const reviews = await github.paginate(github.rest.pulls.listReviews, { owner, repo, pull_number: pr.number, per_page: 100, }); @@ -545,10 +523,9 @@ jobs: const maintainerChangeRequest = freshMaintainerReviews.find( review => review.state === 'CHANGES_REQUESTED' ); - const codeRabbitApproved = freshCodeRabbitReview?.state === 'APPROVED' && - codeRabbitPreMerge === 'passed'; - const codeRabbitChangesRequested = freshCodeRabbitReview?.state === 'CHANGES_REQUESTED' || - codeRabbitPreMerge === 'failed'; + const automatedAuthor = pr.user?.type === 'Bot'; + const codeRabbitApproved = 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]; @@ -560,9 +537,20 @@ jobs: let phase; let activateCodeRabbit = false; let recycleCodeRabbitLabel = false; - if (codeRabbitChangesRequested) { + if (codeRabbitChangesRequested || maintainerChangeRequest) { desiredLabel = 'awaiting-author'; - phase = 'coderabbit-changes'; + phase = codeRabbitChangesRequested ? 'coderabbit-changes' : 'maintainer-changes'; + } else if (automatedAuthor) { + if (pr.draft) { + desiredLabel = null; + phase = 'draft'; + } else if (!maintainerApproval) { + desiredLabel = 'awaiting-maintainer'; + phase = 'maintainer'; + } else { + desiredLabel = null; + phase = 'approved'; + } } else if (!codeRabbitApproved) { if (pr.draft) { desiredLabel = null; @@ -573,9 +561,6 @@ jobs: desiredLabel = 'awaiting-coderabbit'; phase = 'coderabbit'; } - } else if (maintainerChangeRequest) { - desiredLabel = 'awaiting-author'; - phase = 'maintainer-changes'; } else if (pr.draft) { desiredLabel = 'awaiting-ready'; phase = 'draft-approved'; @@ -589,7 +574,7 @@ jobs: core.info( `PR #${pr.number}: CI passing, reviews=${latest.size}, ` + - `coderabbit=${freshCodeRabbitReview?.state ?? 'pending'}/${codeRabbitPreMerge}, ` + + `coderabbit=${freshCodeRabbitReview?.state ?? (automatedAuthor ? 'optional' : 'pending')}, ` + `maintainer=${maintainerApproval?.state ?? 'pending'} → ${desiredLabel ?? '(none)'}` ); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1e1bd85915..b6673c0255 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -137,13 +137,13 @@ pnpm install Ready-for-review PRs move through these gates in order: 1. Required CI checks must pass. -2. The workflow automatically applies the managed `coderabbit-review-active` label to start CodeRabbit review for the latest commit. Contributors and maintainers should not manage this label manually. -3. Address CodeRabbit findings and every error in its persistent **Pre-merge checks** summary. Warnings are advisory unless repository policy says otherwise. Review restarts automatically after required CI passes again. -4. After CodeRabbit approves the current commit and its blocking pre-merge checks pass, a non-author maintainer account with write access performs the final review and approval. +2. For eligible human-authored PRs, the workflow automatically applies the managed `coderabbit-review-active` label to start CodeRabbit review for the latest commit. Contributors and maintainers should not manage this label manually. +3. Address CodeRabbit findings and every error in its persistent **Pre-merge checks** summary. Warnings are advisory unless repository policy says otherwise. CodeRabbit's error-mode checks use its native changes-requested review to block merging. +4. After CodeRabbit approves an eligible human-authored PR, a non-author maintainer account with write access performs the final review and approval. An automated comment on each PR shows the current gate and next action. The `awaiting-coderabbit`, `awaiting-ready`, `awaiting-maintainer`, `awaiting-author`, and `has-conflicts` labels make the same state visible in the PR list. A new commit invalidates prior approvals; required CI and CodeRabbit rerun for that commit before maintainer review. -CodeRabbit's green commit status only means its review completed; it does not prove that custom pre-merge checks passed. Contributors, maintainers, and automated PR fixers must inspect CodeRabbit's persistent summary comment, resolve all **Error** entries under **Pre-merge checks**, and report that state explicitly in PR update comments. +CodeRabbit's green commit status only means its review completed; it does not prove that custom pre-merge checks passed. Contributors, maintainers, and automated PR fixers must inspect CodeRabbit's persistent summary comment, resolve all **Error** entries under **Pre-merge checks**, and report that state explicitly in PR update comments. Use `@coderabbitai run pre-merge checks` to rerun those checks and `@coderabbitai approve` to resolve CodeRabbit threads and request its approval after fixes. The `PR review gate` commit status passes only after the sequence completes. It is advisory by default; repository administrators can make it required on `main` if enforcement is desired. The labels and managed comment remain the maintainer-facing review queue either way. @@ -151,7 +151,7 @@ Optional checks such as Codecov do not delay CodeRabbit unless repository rules The workflow reads required checks from the `main` branch ruleset. If those rules cannot be read, the gate fails closed and waits for the hourly reconciliation or a manual workflow run after the ruleset is available again. -PRs opened by bots or other automated accounts follow the same CI and CodeRabbit gates. Final approval requires a non-author, non-bot account with write access, and maintainers remain responsible for verifying the change's intent, provenance, and validation before merging. +PRs opened by bots skip automatic CodeRabbit activation because author exclusions take precedence over label opt-in. They move from required CI directly to human maintainer review. A human may optionally request an incremental `@coderabbitai review` or a fresh `@coderabbitai full review`; once CodeRabbit requests changes, that native review must be resolved or dismissed before merge. Final approval still requires a non-author, non-bot account with write access, and maintainers remain responsible for verifying the change's intent, provenance, and validation. - **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 18f27e9963..0c13d5ba1f 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -31,8 +31,6 @@ interface HarnessOptions { existingGuide?: boolean existingGuideHead?: string existingGuidePendingHead?: string - codeRabbitSummaryHead?: string | null - codeRabbitPreMergeErrors?: number labels?: string[] prAuthor?: { login: string; type: "Bot" | "User" } addLabelsStatus?: number @@ -82,7 +80,7 @@ async function runWorkflow(options: HarnessOptions = {}) { state: options.prState ?? "open", draft: options.draft ?? false, html_url: "https://github.com/Zoo-Code-Org/Zoo-Code/pull/1437", - user: options.prAuthor ?? { login: "zoomote[bot]", type: "Bot" }, + user: options.prAuthor ?? { login: "contributor", type: "User" }, head: { sha: SHA, repo: { full_name: headRepository } }, base: { ref: "main", repo: { full_name: "Zoo-Code-Org/Zoo-Code" } }, labels: (options.labels ?? []).map((name) => ({ name })), @@ -152,19 +150,6 @@ async function runWorkflow(options: HarnessOptions = {}) { }, ] : []), - ...(options.codeRabbitSummaryHead === null - ? [] - : [ - { - id: 20, - user: { login: "coderabbitai[bot]" }, - body: - "\n" + - `**Merge Risk:** Moderate · up to \`${options.codeRabbitSummaryHead ?? SHA}\`\n` + - "\n" + - `### ❌ Failed checks (${options.codeRabbitPreMergeErrors ?? 0} errors, 0 warnings)`, - }, - ]), ] const addLabels = vi.fn(async (_args: unknown) => { @@ -365,6 +350,13 @@ describe("PR review-state workflow", () => { expect(result.setFailed).not.toHaveBeenCalled() }) + it("reconciles same-repository review events", async () => { + const result = await runWorkflow({ eventName: "pull_request_review" }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + expect(result.createCommitStatus).toHaveBeenCalled() + }) + it("reconciles fork PRs from pull_request_target", async () => { const result = await runWorkflow({ eventName: "pull_request_target", fork: true }) @@ -390,6 +382,50 @@ describe("PR review-state workflow", () => { expect(latestGuide(result)).toContain("Mark the PR ready") }) + it("routes bot-authored PRs directly to maintainer review", async () => { + const result = await runWorkflow({ + prAuthor: { login: "zoomote[bot]", type: "Bot" }, + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + expect(result.addLabels).not.toHaveBeenCalledWith( + expect.objectContaining({ labels: ["coderabbit-review-active"] }), + ) + }) + + it("completes bot-authored PR review after human maintainer approval", async () => { + const result = await runWorkflow({ + prAuthor: { login: "zoomote[bot]", type: "Bot" }, + permissions: { maintainer: "write" }, + reviews: [ + { + login: "maintainer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(latestGateStatus(result)?.state).toBe("success") + }) + + it("honors manually requested CodeRabbit changes on bot-authored PRs", async () => { + const result = await runWorkflow({ + prAuthor: { login: "zoomote[bot]", type: "Bot" }, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "CHANGES_REQUESTED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + }) + it("removes the CodeRabbit label while required CI is pending", async () => { const result = await runWorkflow({ labels: ["coderabbit-review-active"], @@ -595,39 +631,6 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) }) - it("routes blocking CodeRabbit pre-merge errors back to the author", async () => { - const result = await runWorkflow({ - codeRabbitPreMergeErrors: 1, - reviews: [ - { - login: "coderabbitai[bot]", - type: "Bot", - state: "APPROVED", - submittedAt: REVIEWED_AT, - }, - ], - }) - - expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) - expect(latestGateStatus(result)?.description).toContain("blocking pre-merge checks") - }) - - it("ignores CodeRabbit pre-merge results from an older head", async () => { - const result = await runWorkflow({ - codeRabbitSummaryHead: OLD_SHA, - reviews: [ - { - login: "coderabbitai[bot]", - type: "Bot", - state: "APPROVED", - submittedAt: REVIEWED_AT, - }, - ], - }) - - expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) - }) - it("recognizes CodeRabbit regardless of login casing", async () => { const result = await runWorkflow({ reviews: [ @@ -867,6 +870,29 @@ describe("PR review-state workflow", () => { expect(latestGateStatus(result)?.state).toBe("success") }) + it("requires maintainer approval after CodeRabbit approval", async () => { + const result = await runWorkflow({ + permissions: { maintainer: "write" }, + reviews: [ + { + login: "maintainer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT + 1_000, + }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + expect(latestGateStatus(result)?.state).toBe("pending") + }) + it("publishes the review gate as a standalone commit status", async () => { const result = await runWorkflow() @@ -881,7 +907,7 @@ describe("PR review-state workflow", () => { { context: "PR review gate", state: "pending", - description: "Wait for CodeRabbit approval and blocking pre-merge checks to pass.", + description: "Required CI passed. Wait for CodeRabbit to approve the latest commit.", targetUrl: "https://github.com/Zoo-Code-Org/Zoo-Code/pull/1437", }, ], @@ -900,15 +926,39 @@ describe("PR review-state workflow", () => { expect(latestGuide(result)).toContain(`coderabbit-review-label:${SHA}`) }) - it("publishes the gate and continues reconciliation when status lookup fails", async () => { + it("skips gate publication and continues reconciliation when status lookup fails", async () => { const result = await runWorkflow({ gateStatusLookupErrorStatus: 500 }) expect(result.setFailed).not.toHaveBeenCalled() expect(result.warning).toHaveBeenCalledWith(expect.stringContaining("could not inspect PR review gate")) - expect(result.createCommitStatus).toHaveBeenCalled() + expect(result.createCommitStatus).not.toHaveBeenCalled() expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) + it("forces gate invalidation after a reconciliation failure even when status lookup fails", async () => { + const result = await runWorkflow({ + gateStatusLookupErrorStatus: 500, + permissionErrorStatus: 500, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "maintainer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT + 1_000, + }, + ], + }) + + expect(result.setFailed).toHaveBeenCalled() + expect(latestGateStatus(result)?.state).toBe("pending") + }) + it("reports non-404 permission lookup failures", async () => { const result = await runWorkflow({ labels: ["awaiting-maintainer", "coderabbit-review-active"], From 18ef7aefe85e87b00a5d7164b601659f8d7babd8 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 01:20:13 +0000 Subject: [PATCH 16/29] fix: keep fork review gates advisory --- .github/workflows/label-pr-review-state.yml | 9 ++- CONTRIBUTING.md | 2 + .../pr-review-state-workflow.test.ts | 78 ++++++++++++++++++- 3 files changed, 84 insertions(+), 5 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index b0d6e0f59e..1b1e7c3a85 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -228,6 +228,7 @@ jobs: '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.', + 'fork-approved': 'Fork review completed. Native GitHub review protections remain authoritative.', 'maintainer-changes': 'Address the maintainer feedback, push an update, and request another review.', maintainer: 'A human maintainer must now review and approve the latest commit.', approved: 'The required review sequence passed. Remaining merge requirements apply.', @@ -253,7 +254,7 @@ jobs: } catch (error) { core.warning(`PR #${pr.number}: could not inspect ${reviewGateName}: ${error.message}`); } - if (!lookupSucceeded && !required) return; + if (!lookupSucceeded && passed) return; if (latestGateStatus?.state === state && latestGateStatus.description === description && latestGateStatus.target_url === pr.html_url) { @@ -595,7 +596,11 @@ jobs: } await reconcileLabels(pr, desiredLabel); if (phase === 'approved') { - await updateReviewGate(pr, phase, true); + if (isForkPR(pr)) { + await updateReviewGate(pr, 'fork-approved', false); + } else { + await updateReviewGate(pr, phase, true); + } } } catch (error) { let invalidationError = null; diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b6673c0255..ff5cf70aa8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -147,6 +147,8 @@ CodeRabbit's green commit status only means its review completed; it does not pr The `PR review gate` commit status passes only after the sequence completes. It is advisory by default; repository administrators can make it required on `main` if enforcement is desired. The labels and managed comment remain the maintainer-facing review queue either way. +Fork PRs keep the advisory gate pending even after review completes because GitHub gives fork-originated review events a read-only token that cannot reliably invalidate persisted metadata. Native GitHub required-review and required-check protections remain authoritative for merging forks. + Optional checks such as Codecov do not delay CodeRabbit unless repository rules make them required. Draft PRs are not reviewed automatically, but authors can still request an early review with `@coderabbitai review`. The workflow reads required checks from the `main` branch ruleset. If those rules cannot be read, the gate fails closed and waits for the hourly reconciliation or a manual workflow run after the ruleset is available again. diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 0c13d5ba1f..37c4a382c2 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -35,6 +35,7 @@ interface HarnessOptions { prAuthor?: { login: string; type: "Bot" | "User" } addLabelsStatus?: number labelLookupStatus?: number + createLabelStatus?: number removeLabelStatus?: number reviews?: Array<{ login: string @@ -178,7 +179,11 @@ async function runWorkflow(options: HarnessOptions = {}) { return { data: args } }, ) - const createLabel = vi.fn(async (_args: unknown) => undefined) + const createLabel = vi.fn(async (_args: unknown) => { + if (options.createLabelStatus) { + throw Object.assign(new Error("Create label failed"), { status: options.createLabelStatus }) + } + }) const setFailed = vi.fn() const permissionFor = vi.fn(async ({ username }: { username: string }) => { if (options.permissionErrorStatus) { @@ -371,6 +376,12 @@ describe("PR review-state workflow", () => { expect(result.createLabel).toHaveBeenCalledTimes(4) }) + it("fails closed when a managed label cannot be created", async () => { + await expect(runWorkflow({ labelLookupStatus: 404, createLabelStatus: 500 })).rejects.toThrow( + "Create label failed", + ) + }) + it("propagates non-404 label lookup failures", async () => { await expect(runWorkflow({ labelLookupStatus: 500 })).rejects.toThrow("Label lookup failed") }) @@ -474,6 +485,30 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) + it.each(["awaiting-ready", "awaiting-maintainer"])("removes stale %s state", async (staleLabel) => { + const result = await runWorkflow({ labels: [staleLabel] }) + + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: staleLabel })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-coderabbit"] })) + }) + + it("removes stale awaiting-coderabbit after CodeRabbit approval", async () => { + const result = await runWorkflow({ + labels: ["awaiting-coderabbit"], + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "awaiting-coderabbit" })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + }) + it("retries a CodeRabbit label recycle left in the pending state", async () => { const result = await runWorkflow({ labels: ["coderabbit-review-active"], @@ -926,12 +961,12 @@ describe("PR review-state workflow", () => { expect(latestGuide(result)).toContain(`coderabbit-review-label:${SHA}`) }) - it("skips gate publication and continues reconciliation when status lookup fails", async () => { + it("publishes a pending gate and continues reconciliation when status lookup fails", async () => { const result = await runWorkflow({ gateStatusLookupErrorStatus: 500 }) expect(result.setFailed).not.toHaveBeenCalled() expect(result.warning).toHaveBeenCalledWith(expect.stringContaining("could not inspect PR review gate")) - expect(result.createCommitStatus).not.toHaveBeenCalled() + expect(latestGateStatus(result)?.state).toBe("pending") expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) @@ -1012,6 +1047,43 @@ describe("PR review-state workflow", () => { expect(latestGateStatus(result)?.description).toContain("required CI checks") }) + it("accepts a completed reconcile check from another integration", async () => { + const result = await runWorkflow({ + requiredContexts: ["reconcile"], + requiredIntegrationId: 999, + additionalCheckRuns: [ + { id: 100, name: "reconcile", status: "completed", conclusion: "success", appId: 999 }, + ], + }) + + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) + }) + + it("keeps fork review gates pending after approval", async () => { + const result = await runWorkflow({ + eventName: "schedule", + fork: true, + permissions: { maintainer: "write" }, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + { + login: "maintainer", + type: "User", + state: "APPROVED", + submittedAt: REVIEWED_AT + 1_000, + }, + ], + }) + + expect(latestGateStatus(result)?.state).toBe("pending") + expect(latestGateStatus(result)?.description).toContain("Native GitHub review protections") + }) + it("fails closed when branch rules are unavailable", async () => { const result = await runWorkflow({ branchRulesFail: true }) From f08e285a10fde9978aa71e1f3a37aa3dfb2f0555 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 01:36:38 +0000 Subject: [PATCH 17/29] test: model bot and fork review policy --- .github/tla/PrReviewLabels.cfg | 2 ++ .github/tla/PrReviewLabels.tla | 59 ++++++++++++++++++++++++++++------ .github/tla/README.md | 11 +++++-- 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/.github/tla/PrReviewLabels.cfg b/.github/tla/PrReviewLabels.cfg index 3a7a9aff21..cac023efea 100644 --- a/.github/tla/PrReviewLabels.cfg +++ b/.github/tla/PrReviewLabels.cfg @@ -14,4 +14,6 @@ INVARIANT AwaitingReadySafety INVARIANT AwaitingAuthorSafety INVARIANT ConflictLabelSafety INVARIANT CodeRabbitActivationSafety +INVARIANT BotNeverAwaitsCodeRabbit +INVARIANT ForkGateNeverPasses INVARIANT ApprovedStateHasNoLabel diff --git a/.github/tla/PrReviewLabels.tla b/.github/tla/PrReviewLabels.tla index 15d4bc6062..da6ec99a1d 100644 --- a/.github/tla/PrReviewLabels.tla +++ b/.github/tla/PrReviewLabels.tla @@ -23,6 +23,8 @@ StateLabels == { VARIABLES head, + botAuthor, + forkPR, draft, conflict, ci, @@ -38,6 +40,8 @@ VARIABLES vars == << head, + botAuthor, + forkPR, draft, conflict, ci, @@ -58,16 +62,19 @@ CurrentMaintChanges == maintHead = head /\ maintState = "changes" ValidMaintApproval == maintHead = head /\ maintState = "approved" /\ - maintAfterCR + (botAuthor \/ maintAfterCR) DesiredStateLabel == CASE conflict -> "has-conflicts" [] ci # "passed" -> "none" [] CurrentCRChanges -> "awaiting-author" + [] CurrentMaintChanges -> "awaiting-author" + [] botAuthor /\ draft -> "none" + [] botAuthor /\ ~ValidMaintApproval -> "awaiting-maintainer" + [] botAuthor -> "none" [] draft /\ CurrentCRApproved -> "awaiting-ready" [] draft -> "none" [] ~CurrentCRApproved -> "awaiting-coderabbit" - [] CurrentMaintChanges -> "awaiting-author" [] ~ValidMaintApproval -> "awaiting-maintainer" [] OTHER -> "none" @@ -75,6 +82,7 @@ DesiredCRLabelHead == IF ~conflict /\ ci = "passed" /\ ~draft /\ + ~botAuthor /\ ~CurrentCRApproved /\ ~CurrentCRChanges THEN head @@ -84,11 +92,15 @@ DesiredGatePassed == ~conflict /\ ci = "passed" /\ ~draft /\ - CurrentCRApproved /\ + ~forkPR /\ + ~CurrentCRChanges /\ + (botAuthor \/ CurrentCRApproved) /\ ValidMaintApproval Init == /\ head = 1 + /\ botAuthor \in BOOLEAN + /\ forkPR \in BOOLEAN /\ draft = TRUE /\ conflict = FALSE /\ ci = "pending" @@ -109,6 +121,8 @@ Push == /\ gatePassed' = FALSE /\ dirty' = TRUE /\ UNCHANGED << + botAuthor, + forkPR, draft, conflict, crHead, @@ -126,6 +140,8 @@ MarkReady == /\ dirty' = TRUE /\ UNCHANGED << head, + botAuthor, + forkPR, conflict, ci, crHead, @@ -145,6 +161,8 @@ ConvertToDraft == /\ dirty' = TRUE /\ UNCHANGED << head, + botAuthor, + forkPR, conflict, ci, crHead, @@ -163,6 +181,8 @@ SetConflict(value) == /\ dirty' = TRUE /\ UNCHANGED << head, + botAuthor, + forkPR, draft, ci, crHead, @@ -180,6 +200,8 @@ RestartCI == /\ dirty' = TRUE /\ UNCHANGED << head, + botAuthor, + forkPR, draft, conflict, crHead, @@ -198,6 +220,8 @@ CompleteCI(result) == /\ dirty' = TRUE /\ UNCHANGED << head, + botAuthor, + forkPR, draft, conflict, crHead, @@ -211,14 +235,16 @@ CompleteCI(result) == CodeRabbitReview(result) == /\ result \in {"changes", "approved"} - /\ crHead # head - /\ \/ draft + /\ \/ botAuthor + \/ draft \/ (~draft /\ ci = "passed" /\ crLabelHead = head) /\ crHead' = head /\ crState' = result /\ dirty' = TRUE /\ UNCHANGED << head, + botAuthor, + forkPR, draft, conflict, ci, @@ -238,6 +264,8 @@ MaintainerReview(result) == /\ dirty' = TRUE /\ UNCHANGED << head, + botAuthor, + forkPR, draft, conflict, ci, @@ -255,6 +283,8 @@ Reconcile == /\ dirty' = FALSE /\ UNCHANGED << head, + botAuthor, + forkPR, draft, conflict, ci, @@ -282,6 +312,8 @@ EventualReconciliation == []<>(~dirty) TypeOK == /\ head \in Heads + /\ botAuthor \in BOOLEAN + /\ forkPR \in BOOLEAN /\ draft \in BOOLEAN /\ conflict \in BOOLEAN /\ ci \in CIStates @@ -306,26 +338,33 @@ SettledControlLabelConsistency == AwaitingCodeRabbitSafety == (~dirty /\ stateLabel = "awaiting-coderabbit") => - (~draft /\ ci = "passed" /\ ~conflict /\ ~CurrentCRApproved /\ ~CurrentCRChanges) + (~botAuthor /\ ~draft /\ ci = "passed" /\ ~conflict /\ ~CurrentCRApproved /\ ~CurrentCRChanges) AwaitingMaintainerSafety == (~dirty /\ stateLabel = "awaiting-maintainer") => - (~draft /\ ci = "passed" /\ CurrentCRApproved /\ ~ValidMaintApproval) + (~draft /\ ci = "passed" /\ (botAuthor \/ CurrentCRApproved) /\ ~ValidMaintApproval) AwaitingReadySafety == (~dirty /\ stateLabel = "awaiting-ready") => - (draft /\ ci = "passed" /\ CurrentCRApproved) + (~botAuthor /\ draft /\ ci = "passed" /\ CurrentCRApproved) AwaitingAuthorSafety == (~dirty /\ stateLabel = "awaiting-author") => - (CurrentCRChanges \/ (CurrentCRApproved /\ CurrentMaintChanges)) + (CurrentCRChanges \/ CurrentMaintChanges) ConflictLabelSafety == (~dirty /\ stateLabel = "has-conflicts") => conflict CodeRabbitActivationSafety == (~dirty /\ crLabelHead # 0) => - (crLabelHead = head /\ ~draft /\ ci = "passed" /\ ~conflict) + (crLabelHead = head /\ ~botAuthor /\ ~draft /\ ci = "passed" /\ ~conflict) + +BotNeverAwaitsCodeRabbit == + (~dirty /\ botAuthor) => + (stateLabel # "awaiting-coderabbit" /\ crLabelHead = 0) + +ForkGateNeverPasses == + forkPR => ~gatePassed ApprovedStateHasNoLabel == gatePassed => stateLabel = "none" diff --git a/.github/tla/README.md b/.github/tla/README.md index a48a628d83..40fbf92f3e 100644 --- a/.github/tla/README.md +++ b/.github/tla/README.md @@ -1,11 +1,13 @@ # PR review label model -`PrReviewLabels.tla` models the review workflow as two independently scheduled systems: +`PrReviewLabels.tla` is a bounded policy/interleaving model with two independently scheduled systems: - GitHub changes the PR head, draft state, conflicts, required CI, and reviews. - The metadata workflow reconciles those facts into one state label, the CodeRabbit activation label, and the advisory review gate. -The `dirty` variable allows webhook delivery and reconciliation to lag. Label and advisory-gate consistency are required whenever reconciliation has settled. The model intentionally does not treat the custom check as an instantaneous enforcement boundary: GitHub's native CI and review state can change before the metadata workflow processes the corresponding webhook. +The `dirty` variable allows source state and reconciliation to lag. Label and advisory-gate consistency are required whenever reconciliation has settled. `Reconcile` deliberately abstracts metadata writes as one successful atomic action, so this model verifies policy precedence rather than the GitHub API adapter. + +The model assumes review events have already been normalized and metadata writes succeed. It does not model token permissions, API errors or limits, webhook delivery guarantees, integration identity, check-run ordering, or changes to third-party comment/status formats. Those boundaries are covered by the executable workflow harness and live branch validation. Weak fairness means reconciliation becomes clean infinitely often; it does not claim permanent convergence while GitHub continues changing the PR. The finite model checks two head commits and covers: @@ -14,6 +16,9 @@ The finite model checks two head commits and covers: - conflicts; - required CI pending, failure, success, and reruns; - automatic and manual-draft CodeRabbit reviews; +- bot-authored PRs that bypass required CodeRabbit review; +- fork PRs whose advisory gate never passes; +- same-head CodeRabbit approval replacement or retraction; - maintainer reviews before and after CodeRabbit; - delayed or out-of-order reconciliation. @@ -27,4 +32,4 @@ printf '%s %s\n' 936a262061c914694dfd669a543be24573c45d5aa0ff20a8b96b23d01e050e java -cp tla2tools.jar tlc2.TLC -config PrReviewLabels.cfg PrReviewLabels.tla ``` -The JAR is a local tool and must not be committed. Weak fairness on reconciliation checks that metadata eventually converges after asynchronous GitHub events; the model does not claim that CI or reviewers must eventually approve a PR. +The JAR is a local tool and must not be committed. The model does not claim that CI or reviewers eventually approve a PR, or that external APIs eventually accept a metadata write. From b2da3faa306d18d219f576f87a9549663d4cda1c Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 02:09:32 +0000 Subject: [PATCH 18/29] fix: invalidate stale review gate success --- .github/workflows/label-pr-review-state.yml | 4 +- .../pr-review-state-workflow.test.ts | 67 +++++++++++++++++-- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 1b1e7c3a85..512caf1def 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -260,6 +260,8 @@ jobs: latestGateStatus.target_url === pr.html_url) { return; } + const mustInvalidateSuccess = !passed && + (!lookupSucceeded || latestGateStatus?.state === 'success'); try { await github.rest.repos.createCommitStatus({ @@ -272,7 +274,7 @@ jobs: target_url: pr.html_url, }); } catch (error) { - if (required) throw error; + if (required || mustInvalidateSuccess) throw error; core.warning(`PR #${pr.number}: could not publish ${reviewGateName}: ${error.message}`); } } diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 37c4a382c2..051c749730 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -36,6 +36,9 @@ interface HarnessOptions { addLabelsStatus?: number labelLookupStatus?: number createLabelStatus?: number + listCommentsErrorStatus?: number + createCommentErrorStatus?: number + updateCommentErrorStatus?: number removeLabelStatus?: number reviews?: Array<{ login: string @@ -163,12 +166,18 @@ async function runWorkflow(options: HarnessOptions = {}) { throw Object.assign(new Error("Remove label failed"), { status: options.removeLabelStatus }) } }) - const createComment = vi.fn(async (args: { body: string }) => ({ - data: { id: 11, user: { login: "github-actions[bot]" }, body: args.body }, - })) - const updateComment = vi.fn(async (args: { comment_id: number; body: string }) => ({ - data: { id: args.comment_id, user: { login: "github-actions[bot]" }, body: args.body }, - })) + const createComment = vi.fn(async (args: { body: string }) => { + if (options.createCommentErrorStatus) { + throw Object.assign(new Error("Create comment failed"), { status: options.createCommentErrorStatus }) + } + return { data: { id: 11, user: { login: "github-actions[bot]" }, body: args.body } } + }) + const updateComment = vi.fn(async (args: { comment_id: number; body: string }) => { + if (options.updateCommentErrorStatus) { + throw Object.assign(new Error("Update comment failed"), { status: options.updateCommentErrorStatus }) + } + return { data: { id: args.comment_id, user: { login: "github-actions[bot]" }, body: args.body } } + }) const createCommitStatus = vi.fn( async (args: { sha: string; state: string; context: string; description: string; target_url: string }) => { if (options.createCommitStatusErrorStatus) { @@ -230,7 +239,14 @@ async function runWorkflow(options: HarnessOptions = {}) { createLabel, removeLabel, addLabels, - listComments: vi.fn(async () => existingComments), + listComments: vi.fn(async () => { + if (options.listCommentsErrorStatus) { + throw Object.assign(new Error("List comments failed"), { + status: options.listCommentsErrorStatus, + }) + } + return existingComments + }), createComment, updateComment, }, @@ -961,6 +977,43 @@ describe("PR review-state workflow", () => { expect(latestGuide(result)).toContain(`coderabbit-review-label:${SHA}`) }) + it("fails closed when a successful gate cannot be invalidated", async () => { + const result = await runWorkflow({ + createCommitStatusErrorStatus: 500, + gateStatuses: [ + { + context: "PR review gate", + state: "success", + description: "Approved", + targetUrl: "https://github.com/Zoo-Code-Org/Zoo-Code/pull/1437", + }, + ], + }) + + expect(result.setFailed).toHaveBeenCalledWith(expect.stringContaining("could not invalidate PR review gate")) + }) + + it("fails closed when review-guide comments cannot be listed", async () => { + const result = await runWorkflow({ listCommentsErrorStatus: 500 }) + + expect(result.setFailed).toHaveBeenCalledWith(expect.stringContaining("List comments failed")) + expect(latestGateStatus(result)?.state).toBe("pending") + }) + + it("fails closed when the review-guide comment cannot be created", async () => { + const result = await runWorkflow({ createCommentErrorStatus: 500 }) + + expect(result.setFailed).toHaveBeenCalledWith(expect.stringContaining("Create comment failed")) + expect(latestGateStatus(result)?.state).toBe("pending") + }) + + it("fails closed when the review-guide comment cannot be updated", async () => { + const result = await runWorkflow({ existingGuide: true, updateCommentErrorStatus: 500 }) + + expect(result.setFailed).toHaveBeenCalledWith(expect.stringContaining("Update comment failed")) + expect(latestGateStatus(result)?.state).toBe("pending") + }) + it("publishes a pending gate and continues reconciliation when status lookup fails", async () => { const result = await runWorkflow({ gateStatusLookupErrorStatus: 500 }) From e171f344f11a655903a71ffb57e3d637601f0b1b Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 02:27:18 +0000 Subject: [PATCH 19/29] fix: preserve external review gate checks --- .github/workflows/label-pr-review-state.yml | 10 ++++--- CONTRIBUTING.md | 2 +- .../pr-review-state-workflow.test.ts | 27 +++++++++++++++++++ 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 512caf1def..ae7dc4fb68 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -74,6 +74,7 @@ jobs: const codeRabbitLogin = 'coderabbitai[bot]'; const codeRabbitActiveLabel = 'coderabbit-review-active'; const reviewGateName = 'PR review gate'; + const githubActionsIntegrationId = 15368; // When triggered by a single PR event, only reconcile that PR. // The hourly schedule and workflow_dispatch reconcile all open PRs. @@ -427,10 +428,13 @@ jobs: } // Filter to required checks only (or all checks if rules unavailable). - // Always exclude this workflow's own run to avoid self-referential loops. + // Exclude only this workflow's GitHub Actions checks. A same-named check + // from another or unpinned integration remains a real requirement. const requiredSpecs = (requiredChecks ?? []) - .filter(check => check.context !== reviewGateName) - .filter(check => !(check.context === 'reconcile' && check.integrationId === 15368)); + .filter(check => !( + check.integrationId === githubActionsIntegrationId && + (check.context === reviewGateName || check.context === 'reconcile') + )); const requiredResults = requiredSpecs.map(check => { const run = check.integrationId === null ? latestByName.get(check.context) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ff5cf70aa8..bb01fbf8b1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -145,7 +145,7 @@ An automated comment on each PR shows the current gate and next action. The `awa CodeRabbit's green commit status only means its review completed; it does not prove that custom pre-merge checks passed. Contributors, maintainers, and automated PR fixers must inspect CodeRabbit's persistent summary comment, resolve all **Error** entries under **Pre-merge checks**, and report that state explicitly in PR update comments. Use `@coderabbitai run pre-merge checks` to rerun those checks and `@coderabbitai approve` to resolve CodeRabbit threads and request its approval after fixes. -The `PR review gate` commit status passes only after the sequence completes. It is advisory by default; repository administrators can make it required on `main` if enforcement is desired. The labels and managed comment remain the maintainer-facing review queue either way. +The `PR review gate` commit status passes only after the sequence completes. It is advisory by default; repository administrators can make it required on `main` if enforcement is desired. Pin that required-status rule to the GitHub Actions integration so the reconciler can distinguish its own gate from an external check with the same context. The labels and managed comment remain the maintainer-facing review queue either way. Fork PRs keep the advisory gate pending even after review completes because GitHub gives fork-originated review events a read-only token that cannot reliably invalidate persisted metadata. Native GitHub required-review and required-check protections remain authoritative for merging forks. diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 051c749730..0a0afc88c6 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -631,6 +631,33 @@ describe("PR review-state workflow", () => { expect(latestGateStatus(result)?.description).toContain("required CI checks") }) + it("keeps an external same-name review gate pending when it has not reported", async () => { + const result = await runWorkflow({ + requiredContexts: ["PR review gate"], + requiredIntegrationId: 999, + omitRequiredRuns: true, + }) + + expect(result.addLabels).not.toHaveBeenCalledWith( + expect.objectContaining({ labels: ["coderabbit-review-active"] }), + ) + expect(latestGateStatus(result)?.description).toContain("required CI checks") + }) + + it("keeps an external same-name review gate blocked when it fails", async () => { + const result = await runWorkflow({ + requiredContexts: ["PR review gate"], + requiredIntegrationId: 999, + requiredRunAppId: 999, + requiredConclusion: "failure", + }) + + expect(result.addLabels).not.toHaveBeenCalledWith( + expect.objectContaining({ labels: ["coderabbit-review-active"] }), + ) + expect(latestGateStatus(result)?.description).toContain("failing required CI checks") + }) + it("keeps the gate pending when a required check has not reported", async () => { const result = await runWorkflow({ omitRequiredRuns: true }) From 2f004582dd2c5a9afbed19d364281a323c2eea54 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 02:49:49 +0000 Subject: [PATCH 20/29] fix: reserve review gate status context --- .github/workflows/label-pr-review-state.yml | 2 +- CONTRIBUTING.md | 2 +- .../pr-review-state-workflow.test.ts | 26 ++++++++++++------- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index ae7dc4fb68..07a49e545c 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -73,7 +73,7 @@ jobs: const codeRabbitLabelMarkerPrefix = '`) + expect(latestGuide(result)).not.toContain(":pending") + }) + + it("fails closed when a recycled CodeRabbit activation label cannot be restored", async () => { + const result = await runWorkflow({ + labels: ["coderabbit-review-active"], + existingGuideHead: OLD_SHA, + addLabelsStatus: 500, + }) + + expect( + result.addLabels.mock.calls.filter(([args]) => args.labels.includes("coderabbit-review-active")), + ).toHaveLength(2) + expect(result.setFailed).toHaveBeenCalledWith(expect.stringContaining("Add labels failed")) + expect(result.removeLabel).toHaveBeenCalledWith(expect.objectContaining({ name: "coderabbit-review-active" })) + expect(latestGuide(result)).toContain(`coderabbit-review-label:${SHA}:pending -->`) + }) + it("tolerates an already-removed CodeRabbit label while recycling", async () => { const result = await runWorkflow({ labels: ["coderabbit-review-active"], From 333508c25c5b5d0d198baf9afd66f034631848ea Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 12:42:59 +0000 Subject: [PATCH 25/29] chore: remove PR review TLA model --- .github/tla/PrReviewLabels.cfg | 19 -- .github/tla/PrReviewLabels.tla | 372 --------------------------------- .github/tla/README.md | 35 ---- 3 files changed, 426 deletions(-) delete mode 100644 .github/tla/PrReviewLabels.cfg delete mode 100644 .github/tla/PrReviewLabels.tla delete mode 100644 .github/tla/README.md diff --git a/.github/tla/PrReviewLabels.cfg b/.github/tla/PrReviewLabels.cfg deleted file mode 100644 index cac023efea..0000000000 --- a/.github/tla/PrReviewLabels.cfg +++ /dev/null @@ -1,19 +0,0 @@ -CONSTANT MaxHead = 2 - -SPECIFICATION Spec - -PROPERTY EventualReconciliation - -INVARIANT TypeOK -INVARIANT SettledGateConsistency -INVARIANT SettledLabelConsistency -INVARIANT SettledControlLabelConsistency -INVARIANT AwaitingCodeRabbitSafety -INVARIANT AwaitingMaintainerSafety -INVARIANT AwaitingReadySafety -INVARIANT AwaitingAuthorSafety -INVARIANT ConflictLabelSafety -INVARIANT CodeRabbitActivationSafety -INVARIANT BotNeverAwaitsCodeRabbit -INVARIANT ForkGateNeverPasses -INVARIANT ApprovedStateHasNoLabel diff --git a/.github/tla/PrReviewLabels.tla b/.github/tla/PrReviewLabels.tla deleted file mode 100644 index da6ec99a1d..0000000000 --- a/.github/tla/PrReviewLabels.tla +++ /dev/null @@ -1,372 +0,0 @@ ---------------------------- MODULE PrReviewLabels --------------------------- -EXTENDS Integers, Naturals, TLC - -(*************************************************************************** -The model separates environment changes (pushes, CI, and reviews) from the -metadata workflow's Reconcile action. dirty = TRUE means GitHub has newer -source-of-truth state than the labels currently show. -***************************************************************************) - -CONSTANT MaxHead - -Heads == 1..MaxHead -CIStates == {"pending", "failed", "passed"} -ReviewStates == {"none", "changes", "approved"} -StateLabels == { - "none", - "has-conflicts", - "awaiting-coderabbit", - "awaiting-author", - "awaiting-ready", - "awaiting-maintainer" -} - -VARIABLES - head, - botAuthor, - forkPR, - draft, - conflict, - ci, - crHead, - crState, - maintHead, - maintState, - maintAfterCR, - crLabelHead, - stateLabel, - gatePassed, - dirty - -vars == << - head, - botAuthor, - forkPR, - draft, - conflict, - ci, - crHead, - crState, - maintHead, - maintState, - maintAfterCR, - crLabelHead, - stateLabel, - gatePassed, - dirty ->> - -CurrentCRApproved == crHead = head /\ crState = "approved" -CurrentCRChanges == crHead = head /\ crState = "changes" -CurrentMaintChanges == maintHead = head /\ maintState = "changes" -ValidMaintApproval == - maintHead = head /\ - maintState = "approved" /\ - (botAuthor \/ maintAfterCR) - -DesiredStateLabel == - CASE conflict -> "has-conflicts" - [] ci # "passed" -> "none" - [] CurrentCRChanges -> "awaiting-author" - [] CurrentMaintChanges -> "awaiting-author" - [] botAuthor /\ draft -> "none" - [] botAuthor /\ ~ValidMaintApproval -> "awaiting-maintainer" - [] botAuthor -> "none" - [] draft /\ CurrentCRApproved -> "awaiting-ready" - [] draft -> "none" - [] ~CurrentCRApproved -> "awaiting-coderabbit" - [] ~ValidMaintApproval -> "awaiting-maintainer" - [] OTHER -> "none" - -DesiredCRLabelHead == - IF ~conflict /\ - ci = "passed" /\ - ~draft /\ - ~botAuthor /\ - ~CurrentCRApproved /\ - ~CurrentCRChanges - THEN head - ELSE 0 - -DesiredGatePassed == - ~conflict /\ - ci = "passed" /\ - ~draft /\ - ~forkPR /\ - ~CurrentCRChanges /\ - (botAuthor \/ CurrentCRApproved) /\ - ValidMaintApproval - -Init == - /\ head = 1 - /\ botAuthor \in BOOLEAN - /\ forkPR \in BOOLEAN - /\ draft = TRUE - /\ conflict = FALSE - /\ ci = "pending" - /\ crHead = 0 - /\ crState = "none" - /\ maintHead = 0 - /\ maintState = "none" - /\ maintAfterCR = FALSE - /\ crLabelHead = 0 - /\ stateLabel = "none" - /\ gatePassed = FALSE - /\ dirty = TRUE - -Push == - /\ head < MaxHead - /\ head' = head + 1 - /\ ci' = "pending" - /\ gatePassed' = FALSE - /\ dirty' = TRUE - /\ UNCHANGED << - botAuthor, - forkPR, - draft, - conflict, - crHead, - crState, - maintHead, - maintState, - maintAfterCR, - crLabelHead, - stateLabel - >> - -MarkReady == - /\ draft - /\ draft' = FALSE - /\ dirty' = TRUE - /\ UNCHANGED << - head, - botAuthor, - forkPR, - conflict, - ci, - crHead, - crState, - maintHead, - maintState, - maintAfterCR, - crLabelHead, - stateLabel, - gatePassed - >> - -ConvertToDraft == - /\ ~draft - /\ draft' = TRUE - /\ gatePassed' = FALSE - /\ dirty' = TRUE - /\ UNCHANGED << - head, - botAuthor, - forkPR, - conflict, - ci, - crHead, - crState, - maintHead, - maintState, - maintAfterCR, - crLabelHead, - stateLabel - >> - -SetConflict(value) == - /\ value \in BOOLEAN - /\ conflict' = value - /\ IF value THEN gatePassed' = FALSE ELSE UNCHANGED gatePassed - /\ dirty' = TRUE - /\ UNCHANGED << - head, - botAuthor, - forkPR, - draft, - ci, - crHead, - crState, - maintHead, - maintState, - maintAfterCR, - crLabelHead, - stateLabel - >> - -RestartCI == - /\ ci' = "pending" - /\ gatePassed' = FALSE - /\ dirty' = TRUE - /\ UNCHANGED << - head, - botAuthor, - forkPR, - draft, - conflict, - crHead, - crState, - maintHead, - maintState, - maintAfterCR, - crLabelHead, - stateLabel - >> - -CompleteCI(result) == - /\ result \in {"failed", "passed"} - /\ ci' = result - /\ IF result = "failed" THEN gatePassed' = FALSE ELSE UNCHANGED gatePassed - /\ dirty' = TRUE - /\ UNCHANGED << - head, - botAuthor, - forkPR, - draft, - conflict, - crHead, - crState, - maintHead, - maintState, - maintAfterCR, - crLabelHead, - stateLabel - >> - -CodeRabbitReview(result) == - /\ result \in {"changes", "approved"} - /\ \/ botAuthor - \/ draft - \/ (~draft /\ ci = "passed" /\ crLabelHead = head) - /\ crHead' = head - /\ crState' = result - /\ dirty' = TRUE - /\ UNCHANGED << - head, - botAuthor, - forkPR, - draft, - conflict, - ci, - maintHead, - maintState, - maintAfterCR, - crLabelHead, - stateLabel, - gatePassed - >> - -MaintainerReview(result) == - /\ result \in {"changes", "approved"} - /\ maintHead' = head - /\ maintState' = result - /\ maintAfterCR' = CurrentCRApproved - /\ dirty' = TRUE - /\ UNCHANGED << - head, - botAuthor, - forkPR, - draft, - conflict, - ci, - crHead, - crState, - crLabelHead, - stateLabel, - gatePassed - >> - -Reconcile == - /\ stateLabel' = DesiredStateLabel - /\ crLabelHead' = DesiredCRLabelHead - /\ gatePassed' = DesiredGatePassed - /\ dirty' = FALSE - /\ UNCHANGED << - head, - botAuthor, - forkPR, - draft, - conflict, - ci, - crHead, - crState, - maintHead, - maintState, - maintAfterCR - >> - -Next == - \/ Push - \/ MarkReady - \/ ConvertToDraft - \/ \E value \in BOOLEAN : SetConflict(value) - \/ RestartCI - \/ \E result \in {"failed", "passed"} : CompleteCI(result) - \/ \E result \in {"changes", "approved"} : CodeRabbitReview(result) - \/ \E result \in {"changes", "approved"} : MaintainerReview(result) - \/ Reconcile - -Spec == Init /\ [][Next]_vars /\ WF_vars(Reconcile) - -EventualReconciliation == []<>(~dirty) - -TypeOK == - /\ head \in Heads - /\ botAuthor \in BOOLEAN - /\ forkPR \in BOOLEAN - /\ draft \in BOOLEAN - /\ conflict \in BOOLEAN - /\ ci \in CIStates - /\ crHead \in 0..MaxHead - /\ crState \in ReviewStates - /\ maintHead \in 0..MaxHead - /\ maintState \in ReviewStates - /\ maintAfterCR \in BOOLEAN - /\ crLabelHead \in 0..MaxHead - /\ stateLabel \in StateLabels - /\ gatePassed \in BOOLEAN - /\ dirty \in BOOLEAN - -SettledGateConsistency == - ~dirty => gatePassed = DesiredGatePassed - -SettledLabelConsistency == - ~dirty => stateLabel = DesiredStateLabel - -SettledControlLabelConsistency == - ~dirty => crLabelHead = DesiredCRLabelHead - -AwaitingCodeRabbitSafety == - (~dirty /\ stateLabel = "awaiting-coderabbit") => - (~botAuthor /\ ~draft /\ ci = "passed" /\ ~conflict /\ ~CurrentCRApproved /\ ~CurrentCRChanges) - -AwaitingMaintainerSafety == - (~dirty /\ stateLabel = "awaiting-maintainer") => - (~draft /\ ci = "passed" /\ (botAuthor \/ CurrentCRApproved) /\ ~ValidMaintApproval) - -AwaitingReadySafety == - (~dirty /\ stateLabel = "awaiting-ready") => - (~botAuthor /\ draft /\ ci = "passed" /\ CurrentCRApproved) - -AwaitingAuthorSafety == - (~dirty /\ stateLabel = "awaiting-author") => - (CurrentCRChanges \/ CurrentMaintChanges) - -ConflictLabelSafety == - (~dirty /\ stateLabel = "has-conflicts") => conflict - -CodeRabbitActivationSafety == - (~dirty /\ crLabelHead # 0) => - (crLabelHead = head /\ ~botAuthor /\ ~draft /\ ci = "passed" /\ ~conflict) - -BotNeverAwaitsCodeRabbit == - (~dirty /\ botAuthor) => - (stateLabel # "awaiting-coderabbit" /\ crLabelHead = 0) - -ForkGateNeverPasses == - forkPR => ~gatePassed - -ApprovedStateHasNoLabel == - gatePassed => stateLabel = "none" - -============================================================================= diff --git a/.github/tla/README.md b/.github/tla/README.md deleted file mode 100644 index 40fbf92f3e..0000000000 --- a/.github/tla/README.md +++ /dev/null @@ -1,35 +0,0 @@ -# PR review label model - -`PrReviewLabels.tla` is a bounded policy/interleaving model with two independently scheduled systems: - -- GitHub changes the PR head, draft state, conflicts, required CI, and reviews. -- The metadata workflow reconciles those facts into one state label, the CodeRabbit activation label, and the advisory review gate. - -The `dirty` variable allows source state and reconciliation to lag. Label and advisory-gate consistency are required whenever reconciliation has settled. `Reconcile` deliberately abstracts metadata writes as one successful atomic action, so this model verifies policy precedence rather than the GitHub API adapter. - -The model assumes review events have already been normalized and metadata writes succeed. It does not model token permissions, API errors or limits, webhook delivery guarantees, integration identity, check-run ordering, or changes to third-party comment/status formats. Those boundaries are covered by the executable workflow harness and live branch validation. Weak fairness means reconciliation becomes clean infinitely often; it does not claim permanent convergence while GitHub continues changing the PR. - -The finite model checks two head commits and covers: - -- pushes and stale reviews; -- draft/ready conversion; -- conflicts; -- required CI pending, failure, success, and reruns; -- automatic and manual-draft CodeRabbit reviews; -- bot-authored PRs that bypass required CodeRabbit review; -- fork PRs whose advisory gate never passes; -- same-head CodeRabbit approval replacement or retraction; -- maintainer reviews before and after CodeRabbit; -- delayed or out-of-order reconciliation. - -## Run TLC - -Download the pinned TLA+ tools release, then run TLC from this directory: - -```bash -curl -fsSLO https://github.com/tlaplus/tlaplus/releases/download/v1.7.4/tla2tools.jar -printf '%s %s\n' 936a262061c914694dfd669a543be24573c45d5aa0ff20a8b96b23d01e050e88 tla2tools.jar | sha256sum --check -java -cp tla2tools.jar tlc2.TLC -config PrReviewLabels.cfg PrReviewLabels.tla -``` - -The JAR is a local tool and must not be committed. The model does not claim that CI or reviewers eventually approve a PR, or that external APIs eventually accept a metadata write. From 7fbf7d3fcd50e3006447bb1dabeede5e3b76cb77 Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 12:46:11 +0000 Subject: [PATCH 26/29] chore: calibrate CodeRabbit trust check --- .coderabbit.yaml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index e32968bc7a..8165d36c3e 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -128,7 +128,11 @@ reviews: unvalidated input, bypasses approval or allowlist controls, can lose persisted state due to a missing await, non-atomic write, or omitted default propagation, or leaks lifecycle resources. Cite the path and a plausible triggering scenario; pass when no such changed - path exists. + path exists. For advisory PR state labels and guidance comments, drift is not an error + when native merge requirements remain intact and the review gate remains pending. Still + fail PR-review automation defects that meet the trust violations above, including native + enforcement bypass, a false-success gate, sensitive-data exposure, or authoritative + review-state loss. tools: eslint: From 573cf6d503aec326ead0bede64416915aabfdd2e Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 12:51:24 +0000 Subject: [PATCH 27/29] Revert "chore: calibrate CodeRabbit trust check" This reverts commit 7fbf7d3fcd50e3006447bb1dabeede5e3b76cb77. --- .coderabbit.yaml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 8165d36c3e..e32968bc7a 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -128,11 +128,7 @@ reviews: unvalidated input, bypasses approval or allowlist controls, can lose persisted state due to a missing await, non-atomic write, or omitted default propagation, or leaks lifecycle resources. Cite the path and a plausible triggering scenario; pass when no such changed - path exists. For advisory PR state labels and guidance comments, drift is not an error - when native merge requirements remain intact and the review gate remains pending. Still - fail PR-review automation defects that meet the trust violations above, including native - enforcement bypass, a false-success gate, sensitive-data exposure, or authoritative - review-state loss. + path exists. tools: eslint: From 046c4568b731166de7d8a54425988cc34504629d Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 13:26:33 +0000 Subject: [PATCH 28/29] fix: pass review gate at maintainer handoff --- .github/workflows/label-pr-review-state.yml | 7 ++++--- .../__tests__/pr-review-state-workflow.test.ts | 11 +++++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.github/workflows/label-pr-review-state.yml b/.github/workflows/label-pr-review-state.yml index 76ec5219a3..80c0a01ac5 100644 --- a/.github/workflows/label-pr-review-state.yml +++ b/.github/workflows/label-pr-review-state.yml @@ -291,7 +291,7 @@ jobs: 'draft-approved': 'CodeRabbit approved the latest commit. Mark the draft ready.', 'fork-approved': 'Fork review completed. Native GitHub review protections remain authoritative.', 'maintainer-changes': 'Address the maintainer feedback, push an update, and request another review.', - maintainer: 'A human maintainer must now review and approve the latest commit.', + maintainer: 'Ready for human maintainer review and approval.', approved: 'The required review sequence passed. Remaining merge requirements apply.', }; return messages[phase]; @@ -658,7 +658,8 @@ jobs: `maintainer=${maintainerApproval?.state ?? 'pending'} → ${desiredLabel ?? '(none)'}` ); - if (phase !== 'approved') { + const readyForMaintainer = phase === 'maintainer' || phase === 'approved'; + if (!readyForMaintainer) { await updateReviewGate(pr, phase, false); } const recyclingActiveLabel = activateCodeRabbit && recycleCodeRabbitLabel && @@ -674,7 +675,7 @@ jobs: await updateReviewGuide(pr, phase, existingGuide); } await reconcileLabels(pr, desiredLabel); - if (phase === 'approved') { + if (readyForMaintainer) { if (isForkPR(pr)) { await updateReviewGate(pr, 'fork-approved', false); } else { diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index e6b7d3b89b..9091e371c9 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -438,6 +438,8 @@ describe("PR review-state workflow", () => { expect(result.addLabels).not.toHaveBeenCalledWith( expect.objectContaining({ labels: ["coderabbit-review-active"] }), ) + expect(latestGateStatus(result)?.state).toBe("success") + expect(latestGateStatus(result)?.description).toContain("Ready for human maintainer") }) it("completes bot-authored PR review after human maintainer approval", async () => { @@ -471,6 +473,7 @@ describe("PR review-state workflow", () => { }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-author"] })) + expect(latestGateStatus(result)?.state).toBe("pending") }) it("removes the CodeRabbit label while required CI is pending", async () => { @@ -946,7 +949,7 @@ describe("PR review-state workflow", () => { }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) - expect(latestGateStatus(result)?.state).toBe("pending") + expect(latestGateStatus(result)?.state).toBe("success") }) it("ignores maintainer approvals from an older head", async () => { @@ -970,7 +973,7 @@ describe("PR review-state workflow", () => { }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) - expect(latestGateStatus(result)?.state).toBe("pending") + expect(latestGateStatus(result)?.state).toBe("success") }) it("keeps awaiting-author when any maintainer requests changes", async () => { @@ -1069,7 +1072,7 @@ describe("PR review-state workflow", () => { expect(latestGateStatus(result)?.state).toBe("success") }) - it("requires maintainer approval after CodeRabbit approval", async () => { + it("passes once CodeRabbit approval makes the PR ready for maintainer review", async () => { const result = await runWorkflow({ permissions: { maintainer: "write" }, reviews: [ @@ -1089,7 +1092,7 @@ describe("PR review-state workflow", () => { }) expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) - expect(latestGateStatus(result)?.state).toBe("pending") + expect(latestGateStatus(result)?.state).toBe("success") }) it("publishes the review gate as a standalone commit status", async () => { From 9514ed40939364db2593cd13aadaecf4fcf5ddac Mon Sep 17 00:00:00 2001 From: Roomote Date: Sun, 30 Aug 2026 13:49:44 +0000 Subject: [PATCH 29/29] test: cover gate lookup failure at handoff --- .../pr-review-state-workflow.test.ts | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 9091e371c9..00122bd9c8 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -1180,6 +1180,27 @@ describe("PR review-state workflow", () => { expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["coderabbit-review-active"] })) }) + it("does not publish success when gate status lookup fails at the maintainer handoff", async () => { + const result = await runWorkflow({ + gateStatusLookupErrorStatus: 500, + reviews: [ + { + login: "coderabbitai[bot]", + type: "Bot", + state: "APPROVED", + submittedAt: REVIEWED_AT, + }, + ], + }) + + expect(result.setFailed).not.toHaveBeenCalled() + expect(result.warning).toHaveBeenCalledWith( + expect.stringContaining("could not inspect Zoo Code / PR review gate"), + ) + expect(result.createCommitStatus).not.toHaveBeenCalledWith(expect.objectContaining({ state: "success" })) + expect(result.addLabels).toHaveBeenCalledWith(expect.objectContaining({ labels: ["awaiting-maintainer"] })) + }) + it("forces gate invalidation after a reconciliation failure even when status lookup fails", async () => { const result = await runWorkflow({ gateStatusLookupErrorStatus: 500,