diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 5fc9455a7..997333a62 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -4,6 +4,7 @@ const fs = require("node:fs"); const path = require("node:path"); const { describe, it } = require("node:test"); const assert = require("node:assert/strict"); +const { latestCodeRabbitReviewForHead } = require("./pr-quality-state.cjs"); describe("enforce-pr-target workflow", () => { const workflowPath = path.join(__dirname, "../workflows/enforce-pr-target.yml"); @@ -133,27 +134,50 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /legacyReadinessComment/); }); - it("checks out trusted base-branch scripts only (never PR head)", () => { + it("checks out scripts from the event-specific trusted boundary (never PR head)", () => { // Scope the assertions to the checkout step itself, so a stray `ref:` on // another step cannot satisfy the pin while the checkout stays mutable. const checkoutStep = workflow .split("- name: Checkout trusted PR-quality scripts")[1] .split(/\n {6}- name:/)[0]; assert.match(checkoutStep, /actions\/checkout@[0-9a-f]{40}/); - // `pull_request_target` pins the PR's base SHA so the scripts match the - // event's base revision. An `issue_comment` event has no PR payload, so - // the ref falls back to the integration branch `dev` (the gate's only - // allowed base) — still trusted, and never the PR head. + // `pull_request_target` pins the PR base SHA. Privileged `issue_comment` + // runs must source scripts from the repository default branch, matching + // the branch that supplied the workflow itself; unpromoted `dev` scripts + // must never execute under the write-capable token. assert.match( checkoutStep, - /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\|\|\s*'dev'\s*\}\}/, + /ref:\s*\$\{\{\s*github\.event_name\s*==\s*'issue_comment'\s*&&\s*github\.event\.repository\.default_branch\s*\|\|\s*github\.event\.pull_request\.base\.sha\s*\}\}/, ); + assert.doesNotMatch(checkoutStep, /\|\|\s*'dev'/); // The readiness ping reads MAINTAINERS.md from the same trusted checkout. assert.match(checkoutStep, /sparse-checkout:\s*\|\s*\n\s*\.github\/scripts\n\s*MAINTAINERS\.md/); assert.match(checkoutStep, /persist-credentials:\s*false/); assert.doesNotMatch(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.head/); }); + it("orders same-head CodeRabbit reviews deterministically without timestamps", () => { + const head = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + const latest = latestCodeRabbitReviewForHead({ + reviews: [ + { + id: 41, + commit_id: head, + user: { login: "coderabbitai[bot]" }, + body: "older", + }, + { + id: 42, + commit_id: head, + user: { login: "coderabbitai[bot]" }, + body: "newer", + }, + ], + liveHeadSha: head, + }); + assert.equal(latest?.id, 42); + }); + it("loads pr-quality via require from the checked-out scripts", () => { assert.match(workflow, /pr-quality\.cjs/); assert.match(workflow, /collectPrQualityFailures/); diff --git a/.github/scripts/pr-quality-outside-diff.test.cjs b/.github/scripts/pr-quality-outside-diff.test.cjs new file mode 100644 index 000000000..656db2976 --- /dev/null +++ b/.github/scripts/pr-quality-outside-diff.test.cjs @@ -0,0 +1,136 @@ +"use strict"; + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { + coderabbitOutsideDiffFindingIds, + latestCodeRabbitReviewForHead, + unresolvedFindingsClaim, +} = require("./pr-quality-state.cjs"); + +const HEAD = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; +const OUTSIDE_A = "cr-comment:v1:1d258eb2f6791036acf724b1"; +const OUTSIDE_B = "cr-comment:v1:7a8b9c001122334455667788"; + +function review(overrides = {}) { + return { + id: 9001, + commit_id: HEAD, + submitted_at: "2026-08-07T06:00:00Z", + user: { login: "coderabbitai[bot]" }, + body: `**Actionable comments posted: 4**\n\n> [!CAUTION]\n> Some comments are outside the diff and can’t be posted inline due to platform limitations.\n>
\n> ⚠️ Outside diff range comments (2)\n> \n> \n>
`, + ...overrides, + }; +} + +describe("durable CodeRabbit outside-diff findings", () => { + it("uses CodeRabbit's stable cr-comment markers as finding identities", () => { + assert.deepEqual( + coderabbitOutsideDiffFindingIds({ reviews: [review()], liveHeadSha: HEAD }), + [OUTSIDE_A, OUTSIDE_B], + ); + }); + + it("keeps standalone outside-diff findings active without an inline thread", () => { + assert.deepEqual( + unresolvedFindingsClaim({ + threads: [], + reviews: [review()], + liveHeadSha: HEAD, + }), + { + code: "review_findings", + unresolved: 2, + byBot: { "coderabbitai[bot]": 2 }, + }, + ); + }); + + it("adds outside-diff markers without double-counting the review actionable total", () => { + assert.deepEqual( + unresolvedFindingsClaim({ + threads: [ + { isResolved: false, author: { login: "coderabbitai[bot]" } }, + ], + reviews: [review()], + liveHeadSha: HEAD, + }), + { + code: "review_findings", + unresolved: 3, + byBot: { "coderabbitai[bot]": 3 }, + }, + ); + }); + + it("a later clean CodeRabbit review on the same head clears older markers", () => { + assert.deepEqual( + unresolvedFindingsClaim({ + threads: [], + reviews: [ + review({ id: 9000, submitted_at: "2026-08-07T05:00:00Z" }), + review({ + id: 9002, + submitted_at: "2026-08-07T07:00:00Z", + body: "**Actionable comments posted: 0**", + }), + ], + liveHeadSha: HEAD, + }), + { code: null, unresolved: 0, byBot: {} }, + ); + }); + + it("uses review id as a deterministic tie-breaker when timestamps are missing", () => { + const latest = latestCodeRabbitReviewForHead({ + reviews: [ + review({ id: 9001, submitted_at: undefined, body: `Outside diff range comments (1)\n` }), + review({ id: 9002, submitted_at: undefined, body: "**Actionable comments posted: 0**" }), + ], + liveHeadSha: HEAD, + }); + + assert.equal(latest?.id, 9002); + assert.deepEqual( + coderabbitOutsideDiffFindingIds({ + reviews: [ + review({ id: 9001, submitted_at: undefined, body: `Outside diff range comments (1)\n` }), + review({ id: 9002, submitted_at: undefined, body: "**Actionable comments posted: 0**" }), + ], + liveHeadSha: HEAD, + }), + [], + ); + }); + + it("ignores CodeRabbit markers from an older head and from human reviews", () => { + assert.deepEqual( + coderabbitOutsideDiffFindingIds({ + reviews: [ + review({ commit_id: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" }), + review({ + id: 9002, + user: { login: "maintainer" }, + submitted_at: "2026-08-07T07:00:00Z", + }), + ], + liveHeadSha: HEAD, + }), + [], + ); + }); + + it("deduplicates repeated markers in the review body", () => { + assert.deepEqual( + coderabbitOutsideDiffFindingIds({ + reviews: [ + review({ + body: `Outside diff range comments (1)\n\n`, + }), + ], + liveHeadSha: HEAD, + }), + [OUTSIDE_A], + ); + }); +}); diff --git a/.github/scripts/pr-quality-state.cjs b/.github/scripts/pr-quality-state.cjs index d3fa49802..917e45e75 100644 --- a/.github/scripts/pr-quality-state.cjs +++ b/.github/scripts/pr-quality-state.cjs @@ -238,44 +238,73 @@ const REVIEW_FINDINGS_BOT_LOGINS = [ const CODE_RABBIT_LOGIN = "coderabbitai[bot]"; /** - * CodeRabbit's review-body line that reports actionable inline findings. The - * gate reads this to count findings that CodeRabbit posts only as review-body - * text ("outside the diff range") rather than as inline review threads. + * CodeRabbit's review-body line that reports all actionable findings. This is + * kept as a compatibility fallback for older review bodies that predate the + * stable outside-diff markers below. */ const CODE_RABBIT_ACTIONABLE_RE = /\*\*Actionable comments posted:\s*(\d+)\*\*/i; -/** - * Pull-request reviews (from `pulls.listReviews`) that carry CodeRabbit - * findings. CodeRabbit posts some findings that cannot be attached inline - * ("outside the diff range") in the review body with the line - * `**Actionable comments posted: N**`; those never become review threads, so - * the thread check alone would miss them. This supplements the thread check: - * a CodeRabbit review of the live head whose body reports actionable comments - * counts as an unresolved finding. Only CodeRabbit's own reviews are read — - * a human review quoting the same line must not count. - * - * Only the most recent review for the live head is considered (the head a - * findings-review covers is the head that must be clean), so an older review - * of a superseded commit cannot keep the box unticked forever. Reviews with a - * missing or unparsable `submitted_at` sort last deterministically so the - * "most recent" pick is never arbitrary. - */ -function coderabbitOutsideDiffFindings({ reviews = [], liveHeadSha }) { +/** Stable identity CodeRabbit embeds with each finding it cannot attach inline. */ +const CODE_RABBIT_OUTSIDE_DIFF_MARKER_RE = + //gi; + +function submittedAt(review) { + const parsed = Date.parse(String(review?.submitted_at ?? "")); + return Number.isNaN(parsed) ? -Infinity : parsed; +} + +/** Latest CodeRabbit review for the exact head the readiness claim covers. */ +function latestCodeRabbitReviewForHead({ reviews = [], liveHeadSha }) { if (!liveHeadSha || !Array.isArray(reviews) || reviews.length === 0) { - return { code: null, unresolved: 0, byBot: {} }; + return null; } - const submittedAt = review => { - const parsed = Date.parse(String(review?.submitted_at ?? "")); - return Number.isNaN(parsed) ? -Infinity : parsed; - }; - const latestForHead = reviews + return reviews .filter( review => review?.commit_id === liveHeadSha && review?.user?.login === CODE_RABBIT_LOGIN ) - .sort((a, b) => submittedAt(b) - submittedAt(a))[0]; + .sort((a, b) => { + const aTime = submittedAt(a); + const bTime = submittedAt(b); + if (aTime > bTime) return -1; + if (aTime < bTime) return 1; + return Number(b?.id ?? -1) - Number(a?.id ?? -1); + })[0] ?? null; +} + +/** + * Stable identities for CodeRabbit findings outside the current diff. Real + * outside-diff findings in CodeRabbit review bodies carry a + * `cr-comment:v1:` marker. Only the latest CodeRabbit review for the live + * head is authoritative: markers present there are active; a later same-head + * review that omits a marker is the bot-controlled resolution signal. + */ +function coderabbitOutsideDiffFindingIds({ reviews = [], liveHeadSha }) { + const latestForHead = latestCodeRabbitReviewForHead({ reviews, liveHeadSha }); + const body = String(latestForHead?.body ?? ""); + if (!/outside diff range comments/i.test(body)) return []; + + const ids = []; + const seen = new Set(); + for (const match of body.matchAll(CODE_RABBIT_OUTSIDE_DIFF_MARKER_RE)) { + const id = match[1].toLowerCase(); + if (seen.has(id)) continue; + seen.add(id); + ids.push(id); + } + return ids; +} + +/** + * Compatibility parser for older CodeRabbit review bodies that expose only + * `Actionable comments posted: N`. New outside-diff accounting uses the stable + * `cr-comment` identities above, because the actionable total also includes + * normal inline findings and therefore is not itself an outside-diff count. + */ +function coderabbitOutsideDiffFindings({ reviews = [], liveHeadSha }) { + const latestForHead = latestCodeRabbitReviewForHead({ reviews, liveHeadSha }); const body = String(latestForHead?.body ?? ""); const match = CODE_RABBIT_ACTIONABLE_RE.exec(body); if (!match) return { code: null, unresolved: 0, byBot: {} }; @@ -289,18 +318,15 @@ function coderabbitOutsideDiffFindings({ reviews = [], liveHeadSha }) { } /** - * Verify the Codex/CodeRabbit findings claim. The primary signal is the - * pull-request review threads the GraphQL `pullRequestReviewThreads` query - * returns: a thread authored by a review bot that is not explicitly resolved - * is an unresolved finding. CodeRabbit additionally reports some findings - * only in its review body (outside the diff range); those are added by the - * `coderabbitOutsideDiffFindings` supplement so they cannot slip through. - * The supplement is subordinate: it never subtracts, only adds unresolved - * counts for the live head while a bot thread is still open. A review body is - * immutable, so the count can never fall to zero on its own once posted; the - * supplement therefore only counts while an unresolved bot thread exists — the - * author resolves that thread to clear the box, matching the checklist wording - * ("I resolved all correct ... findings") without requiring an empty commit. + * Verify the Codex/CodeRabbit findings claim. Inline findings come from the + * pull-request review threads GraphQL query. CodeRabbit findings that cannot + * attach inline are independent: the latest CodeRabbit review for the live + * head exposes stable `cr-comment:v1:` markers for them, so a standalone + * outside-diff finding remains active even when every inline thread is already + * resolved. A later same-head CodeRabbit review that omits the marker clears + * it without an empty commit. Older CodeRabbit bodies without stable markers + * retain the previous actionable-count supplement while an inline bot thread + * is unresolved. */ function unresolvedFindingsClaim({ threads = [], reviews = [], liveHeadSha }) { const byBot = {}; @@ -313,7 +339,16 @@ function unresolvedFindingsClaim({ threads = [], reviews = [], liveHeadSha }) { unresolved += 1; } } - if (unresolved > 0) { + + const outsideIds = coderabbitOutsideDiffFindingIds({ reviews, liveHeadSha }); + if (outsideIds.length > 0) { + byBot[CODE_RABBIT_LOGIN] = + (byBot[CODE_RABBIT_LOGIN] ?? 0) + outsideIds.length; + unresolved += outsideIds.length; + } else if (unresolved > 0) { + // Legacy fallback for older CodeRabbit review bodies that did not expose + // stable outside-diff identities. Keep the old bounded behavior so an + // immutable aggregate count cannot block a PR forever by itself. const outside = coderabbitOutsideDiffFindings({ reviews, liveHeadSha }); if (outside.code) { for (const [login, count] of Object.entries(outside.byBot)) { @@ -322,6 +357,7 @@ function unresolvedFindingsClaim({ threads = [], reviews = [], liveHeadSha }) { } } } + return unresolved > 0 ? { code: "review_findings", unresolved, byBot } : { code: null, unresolved: 0, byBot }; @@ -372,6 +408,9 @@ module.exports = { REVIEW_FINDINGS_BOT_LOGINS, CODE_RABBIT_LOGIN, CODE_RABBIT_ACTIONABLE_RE, + CODE_RABBIT_OUTSIDE_DIFF_MARKER_RE, + latestCodeRabbitReviewForHead, + coderabbitOutsideDiffFindingIds, coderabbitOutsideDiffFindings, parseState, stateMarker, diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 910baacd6..cd32bdce1 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -9,10 +9,10 @@ on: - ready_for_review - synchronize # A maintainer issue comment ("not touching gui") waives the GUI-screenshot - # gate. `pull_request_target` types do not include issue comments, so a - # separate `issue_comment` trigger re-runs the gate the moment the waiver is - # posted. The gate is idempotent — it re-reads the live PR and updates the - # single consolidated comment — so a comment cannot race or double-mutate. + # gate. CodeRabbit also edits its normal PR status comment when a review + # finishes, which gives this privileged workflow a safe signal to re-check + # review findings even for fork PRs. `pull_request_target` types do not + # include issue comments, so both cases use this separate trigger. issue_comment: types: - created @@ -36,14 +36,16 @@ concurrency: jobs: enforce-target: - # `issue_comment` fires for comments on ANY issue, PR or not, from ANY - # user. This gate is PR-only and write-capable, so a comment on a plain - # issue — or from a non-maintainer — must not start it. Only a maintainer - # comment on a PR (the GUI-waiver case) may re-run the gate. + # `issue_comment` fires for comments on ANY issue or PR. This gate is + # write-capable, so only two trusted sources may start that path: a + # canonical maintainer (GUI-waiver case) or CodeRabbit's own PR status + # comment, whose create/edit event is used only as a signal to re-read the + # live review threads. All other pull_request_target events run normally. if: >- github.event_name != 'issue_comment' || (github.event.issue.pull_request != null && - (github.event.comment.author_association == 'OWNER' || + (github.event.comment.user.login == 'coderabbitai[bot]' || + github.event.comment.author_association == 'OWNER' || github.event.comment.author_association == 'COLLABORATOR' || github.event.comment.author_association == 'MEMBER')) runs-on: ubuntu-latest @@ -52,17 +54,13 @@ jobs: - name: Checkout trusted PR-quality scripts uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: - # The event's base commit, not the repository default: pull_request_target - # runs this workflow from the base revision, and the scripts must come - # from the same revision or a merged gate would run against the - # pre-promotion scripts on `main`. The immutable SHA pins the checkout - # to the exact base commit the event was built against. On an - # `issue_comment` event there is no PR payload, so the checkout falls - # back to the integration branch `dev` — the branch this gate enforces - # and the source of the gate's own scripts. The repository default - # (`main`) can lag `dev`, which would make a comment-triggered run - # evaluate with stale helpers. - ref: ${{ github.event.pull_request.base.sha || 'dev' }} + # `pull_request_target` runs from the PR base revision, so use that + # immutable base SHA for the trusted scripts. `issue_comment` runs the + # privileged workflow from the repository default branch; source its + # scripts and MAINTAINERS.md from that same promoted trust boundary. + # This prevents unpromoted `dev` script changes from executing with + # the workflow's write-capable token. + ref: ${{ github.event_name == 'issue_comment' && github.event.repository.default_branch || github.event.pull_request.base.sha }} persist-credentials: false sparse-checkout: | .github/scripts @@ -145,6 +143,7 @@ jobs: const LEGACY_COMMENT_MARKER = ""; const REVIEW_READY_LABEL = "review-ready"; const MAINTAINERS_FILE = "MAINTAINERS.md"; + const CODE_RABBIT_LOGIN = "coderabbitai[bot]"; const { owner, repo } = context.repo; // `issue_comment` events carry the PR's issue object, not a @@ -155,32 +154,39 @@ jobs: context.payload.issue?.number; // Defensive re-check of the job-level guard. `issue_comment` events - // carry a `comment` object with the author's association; a comment - // on a plain issue has no `issue.pull_request`, and a comment from - // anyone but a maintainer must not re-run this write-capable gate. + // carry a `comment` object with the author's association. A normal + // user comment is trusted only when it comes from a canonical + // maintainer; CodeRabbit's own PR status comment is separately + // allowed as a signal to re-read live review threads. The comment + // body itself is never trusted as gate evidence. if (context.eventName === "issue_comment") { const isPrComment = context.payload.issue?.pull_request != null; const association = context.payload.comment?.author_association; + const commenter = context.payload.comment?.user?.login; + const isCodeRabbit = commenter === CODE_RABBIT_LOGIN; // The association is a cheap prefilter, but OWNER/COLLABORATOR/ // MEMBER is broader than this repository's canonical maintainer // list. A collaborator or member who is not a maintainer must not - // start this write-capable gate, so require the commenter's - // login to be in the trusted MAINTAINERS.md list too. + // start this write-capable gate. const maintainerLogins = new Set( readMaintainerLogins().map(login => login.toLowerCase()) ); - const commenter = context.payload.comment?.user?.login; const isCanonicalMaintainer = typeof commenter === "string" && maintainerLogins.has(commenter.toLowerCase()); if ( !isPrComment || - !["OWNER", "COLLABORATOR", "MEMBER"].includes(association) || - !isCanonicalMaintainer + (!isCodeRabbit && + (![ + "OWNER", + "COLLABORATOR", + "MEMBER" + ].includes(association) || + !isCanonicalMaintainer)) ) { core.info( - "issue_comment not from a canonical maintainer on a PR; skipping the gate." + "issue_comment is neither CodeRabbit nor a canonical maintainer on a PR; skipping the gate." ); return; } @@ -1230,4 +1236,4 @@ jobs: "All PR quality gates passed and there is no active bot state." ); return; - } + } \ No newline at end of file diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index fd783368b..67be862ea 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -914,6 +914,7 @@ describe("GitHub Actions hardening", () => { expect(job["runs-on"]).toBe("ubuntu-latest"); expect(job["if"]).toContain("github.event_name != 'issue_comment'"); expect(job["if"]).toContain("github.event.issue.pull_request != null"); + expect(job["if"]).toContain("coderabbitai[bot]"); expect(job["if"]).toContain("'OWNER'"); expect(job["if"]).toContain("'COLLABORATOR'"); expect(job["if"]).toContain("'MEMBER'"); @@ -936,7 +937,8 @@ describe("GitHub Actions hardening", () => { // runs this workflow from the base revision, and the scripts must match // it — a merged gate would otherwise run against pre-promotion `main` // scripts. The immutable SHA pins the checkout to the event's base commit. - ref: "${{ github.event.pull_request.base.sha || 'dev' }}", + ref: + "${{ github.event_name == 'issue_comment' && github.event.repository.default_branch || github.event.pull_request.base.sha }}", "persist-credentials": false, // MAINTAINERS.md rides along so the completion ping reads the canonical // maintainer list from the same trusted base revision as the scripts. diff --git a/tests/zz-pr-coderabbit-readiness-revalidation.test.ts b/tests/zz-pr-coderabbit-readiness-revalidation.test.ts new file mode 100644 index 000000000..e65183cb8 --- /dev/null +++ b/tests/zz-pr-coderabbit-readiness-revalidation.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, test } from "bun:test"; + +type Workflow = { + on?: { + issue_comment?: { types?: string[] }; + }; + jobs?: Record< + string, + { + if?: string; + steps?: Array<{ + name?: string; + with?: Record; + }>; + } + >; +}; + +describe("CodeRabbit readiness revalidation", () => { + test("CodeRabbit PR status comments can rerun the findings gate", async () => { + const text = await Bun.file( + new URL("../.github/workflows/enforce-pr-target.yml", import.meta.url), + ).text(); + const workflow = Bun.YAML.parse(text) as Workflow; + + expect(workflow.on?.issue_comment?.types).toEqual(["created", "edited"]); + + const job = workflow.jobs?.["enforce-target"]; + expect(job).toBeDefined(); + expect(job?.if).toContain("github.event.issue.pull_request != null"); + expect(job?.if).toContain("github.event.comment.user.login == 'coderabbitai[bot]'"); + + const checkoutStep = job?.steps?.find( + step => step.name === "Checkout trusted PR-quality scripts", + ); + expect(checkoutStep?.with?.ref).toBe( + "${{ github.event_name == 'issue_comment' && github.event.repository.default_branch || github.event.pull_request.base.sha }}", + ); + + const gateStep = job?.steps?.find( + step => step.name === "Enforce PR target, ancestry, and description", + ); + const script = gateStep?.with?.script ?? ""; + + expect(script).toContain('const CODE_RABBIT_LOGIN = "coderabbitai[bot]"'); + expect(script).toContain("const isCodeRabbit = commenter === CODE_RABBIT_LOGIN"); + expect(script).toContain("!isCodeRabbit"); + expect(script).toContain("isCanonicalMaintainer"); + expect(script).toMatch( + /!isPrComment\s*\|\|\s*\(\s*!isCodeRabbit\s*&&\s*\(\s*!\[[\s\S]{0,300}?\.includes\(association\)\s*\|\|\s*!isCanonicalMaintainer\s*\)\s*\)/, + ); + expect(script).toContain("unresolvedFindingsClaim"); + }); +});