Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions .github/scripts/enforce-pr-target.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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/);
Expand Down
136 changes: 136 additions & 0 deletions .github/scripts/pr-quality-outside-diff.test.cjs
Original file line number Diff line number Diff line change
@@ -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> <details>\n> <summary>⚠️ Outside diff range comments (2)</summary>\n> <!-- ${OUTSIDE_A} -->\n> <!-- ${OUTSIDE_B} -->\n> </details>`,
...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<!-- ${OUTSIDE_A} -->` }),
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<!-- ${OUTSIDE_A} -->` }),
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<!-- ${OUTSIDE_A} -->\n<!-- ${OUTSIDE_A} -->`,
}),
],
liveHeadSha: HEAD,
}),
[OUTSIDE_A],
);
});
});
119 changes: 79 additions & 40 deletions .github/scripts/pr-quality-state.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
/<!--\s*(cr-comment:v1:[a-f0-9]+)\s*-->/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;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

/**
* Stable identities for CodeRabbit findings outside the current diff. Real
* outside-diff findings in CodeRabbit review bodies carry a
* `cr-comment:v1:<id>` 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: {} };
Expand All @@ -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:<id>` 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 = {};
Expand All @@ -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)) {
Expand All @@ -322,6 +357,7 @@ function unresolvedFindingsClaim({ threads = [], reviews = [], liveHeadSha }) {
}
}
}

return unresolved > 0
? { code: "review_findings", unresolved, byBot }
: { code: null, unresolved: 0, byBot };
Expand Down Expand Up @@ -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,
Expand Down
Loading
Loading