Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
7 changes: 4 additions & 3 deletions .github/scripts/enforce-pr-target.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ describe("enforce-pr-target workflow", () => {
it("checks out trusted default-branch scripts only (never PR head)", () => {
assert.match(workflow, /actions\/checkout@[0-9a-f]{40}/);
assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.repository\.default_branch\s*\}\}/);
assert.match(workflow, /sparse-checkout:\s*\.github\/scripts/);
// The readiness ping reads MAINTAINERS.md from the same trusted checkout.
assert.match(workflow, /sparse-checkout:\s*\|\s*\n\s*\.github\/scripts\n\s*MAINTAINERS\.md/);
assert.match(workflow, /persist-credentials:\s*false/);
assert.doesNotMatch(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.head/);
});
Expand All @@ -75,9 +76,9 @@ describe("enforce-pr-target workflow", () => {

it("strips stale WRONG BRANCH prefix on failure when base is corrected", () => {
const failureBlock = workflow.match(
/if \(failures\.length > 0\) \{([\s\S]*?)core\.setFailed\(/,
/if \(mustDraft\) \{([\s\S]*?)core\.setFailed\(/,
);
assert.ok(failureBlock, "workflow must have a failure path");
assert.ok(failureBlock, "workflow must have a draft path");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const failurePath = failureBlock[1];
assert.match(failurePath, /shouldStripTitlePrefix/);
assert.match(failurePath, /!hasWrongBase/);
Expand Down
85 changes: 85 additions & 0 deletions .github/scripts/pr-quality.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,22 @@ const MIN_RICH_SECTIONS = 2;
const UNSTRUCTURED_MIN_LEN = 120;
const UNSTRUCTURED_MIN_BLOCKS = 2;

/** HTML markers bounding the bot-managed review-readiness checklist in the PR body. */
const REVIEW_READINESS_START = "<!-- pr-quality-readiness-checklist:start -->";
const REVIEW_READINESS_END = "<!-- pr-quality-readiness-checklist:end -->";

/**
* The four self-attestation boxes a non-maintainer author must tick before the
* gate lifts the draft. The final box is intentionally set off by a blank line
* so the "ready" claim reads as the closing confirmation, not a fourth task.
*/
const REVIEW_READINESS_ITEMS = [
"All CI tests are green on my local testing.",
"I pushed my PR to the latest dev commit.",
"I fixed all correct Codex and CodeRabbit findings.",
"My PR is ready for review.",
];

/**
* Exact instruction / checklist lines from `.github/PULL_REQUEST_TEMPLATE.md`.
* Untouched templates must not count as substance.
Expand Down Expand Up @@ -191,6 +207,69 @@ function hasScreenshotEvidence(body) {
return hasRenderableReferenceImage(visible);
}

/**
* The tickable checklist section injected into the PR description. It lives in
* the body (the author can tick it) and is bounded by HTML markers so the gate
* can find exactly this section and ignore any other task list in the body.
*/
function buildReviewReadinessSection() {
const items = REVIEW_READINESS_ITEMS.flatMap((item, index) =>
index === REVIEW_READINESS_ITEMS.length - 1
? ["", `- [ ] ${item}`]
: [`- [ ] ${item}`],
);
return [
REVIEW_READINESS_START,
"## Review readiness checklist",
"",
"This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:",
"",
...items,
REVIEW_READINESS_END,
].join("\n");
}

/**
* Read the checklist section the bot manages. `present` means the marker pair
* exists; `complete` means the section contains exactly the four boxes and all
* of them are checked. Anything else (missing markers, fewer or extra boxes,
* unchecked boxes) keeps the gate closed. The author can reword an item, but
* the box count and the checked state are the contract.
*/
function extractReviewReadiness(body) {
if (typeof body !== "string") {
return { present: false, complete: false, checked: 0, total: 0 };
}
const start = body.indexOf(REVIEW_READINESS_START);
const end = body.indexOf(REVIEW_READINESS_END);
if (start === -1 || end === -1 || end <= start) {
return { present: false, complete: false, checked: 0, total: 0 };
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
const section = body.slice(start + REVIEW_READINESS_START.length, end);
const boxes = [...section.matchAll(/^\s*[-*]\s+\[([ xX])\]\s+/gm)];
const checked = boxes.filter((match) => match[1] !== " ").length;
const total = boxes.length;
return {
present: true,
complete:
total === REVIEW_READINESS_ITEMS.length && checked === total,
checked,
total,
};
}

/**
* Append the checklist section to a PR body. Idempotent: a body that already
* carries the marker pair is returned unchanged, so a re-run can never stack a
* second checklist (or feed the `edited` event endless body churn).
*/
function appendReviewReadinessSection(body) {
if (extractReviewReadiness(body).present) return body;
const section = buildReviewReadinessSection();
if (typeof body !== "string" || !body.trim()) return `${section}\n`;
return `${body.trimEnd()}\n\n${section}\n`;
}

function collectPrQualityFailures({
baseRef,
allowedBases,
Expand Down Expand Up @@ -248,11 +327,17 @@ function collectPrQualityFailures({
module.exports = {
ANCESTRY_BEHIND_THRESHOLD,
ANCESTRY_AHEAD_MAIN_MAX,
REVIEW_READINESS_ITEMS,
REVIEW_READINESS_START,
REVIEW_READINESS_END,
isWrongAncestry,
authorHasPushPermission,
assessPrDescription,
hasGuiCue,
hasScreenshotEvidence,
buildReviewReadinessSection,
extractReviewReadiness,
appendReviewReadinessSection,
collectPrQualityFailures,
hasEscapedNewlines,
stripPrTemplateBoilerplate,
Expand Down
101 changes: 101 additions & 0 deletions .github/scripts/pr-quality.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,15 @@ const { describe, it } = require("node:test");
const assert = require("node:assert/strict");
const {
ANCESTRY_BEHIND_THRESHOLD,
REVIEW_READINESS_ITEMS,
isWrongAncestry,
authorHasPushPermission,
assessPrDescription,
hasGuiCue,
hasScreenshotEvidence,
buildReviewReadinessSection,
extractReviewReadiness,
appendReviewReadinessSection,
collectPrQualityFailures,
} = require("./pr-quality.cjs");

Expand Down Expand Up @@ -215,6 +219,103 @@ describe("hasScreenshotEvidence", () => {
});
});

describe("review readiness checklist", () => {
const SECTION = buildReviewReadinessSection();

it("builds exactly the four required boxes inside the markers", () => {
assert.ok(SECTION.includes("<!-- pr-quality-readiness-checklist:start -->"));
assert.ok(SECTION.includes("<!-- pr-quality-readiness-checklist:end -->"));
assert.equal((SECTION.match(/\[ \]/g) || []).length, 4);
assert.equal((SECTION.match(/\[x\]/g) || []).length, 0);
assert.equal(REVIEW_READINESS_ITEMS.length, 4);
});

it("keeps the closing 'ready for review' box separated by a blank line", () => {
const lines = SECTION.split("\n");
const readyIndex = lines.findIndex((line) =>
line.includes("My PR is ready for review."),
);
assert.ok(readyIndex > 0);
assert.equal(lines[readyIndex - 1], "");
});

it("reports absent when the body has no markers", () => {
assert.deepEqual(extractReviewReadiness("## Summary\n\nplain body"), {
present: false,
complete: false,
checked: 0,
total: 0,
});
assert.deepEqual(extractReviewReadiness(null), {
present: false,
complete: false,
checked: 0,
total: 0,
});
});

it("counts checked boxes and requires all four for completion", () => {
const body = [
"## Summary",
"Change.",
SECTION.replaceAll("- [ ] ", "- [x] "),
].join("\n\n");
assert.deepEqual(extractReviewReadiness(body), {
present: true,
complete: true,
checked: 4,
total: 4,
});

const partial = body.replace("- [x] My PR is ready for review.", "- [ ] My PR is ready for review.");
assert.deepEqual(extractReviewReadiness(partial), {
present: true,
complete: false,
checked: 3,
total: 4,
});
});

it("treats a reworded but complete section as complete", () => {
const reworded = SECTION
.replace("All CI tests are green on my local testing.", "Local suite green.")
.replaceAll("- [ ] ", "- [x] ");
const result = extractReviewReadiness(reworded);
assert.equal(result.present, true);
assert.equal(result.complete, true);
assert.equal(result.checked, 4);
});

it("stays incomplete for fewer or extra boxes inside the markers", () => {
const fewer = SECTION.replace("- [ ] My PR is ready for review.", "");
assert.equal(extractReviewReadiness(fewer).complete, false);
assert.equal(extractReviewReadiness(fewer).total, 3);

const extra = SECTION.replace(
"<!-- pr-quality-readiness-checklist:end -->",
"- [x] An extra box.\n<!-- pr-quality-readiness-checklist:end -->",
);
const result = extractReviewReadiness(extra);
assert.equal(result.complete, false);
assert.equal(result.total, 5);
});

it("appends once and is idempotent", () => {
const first = appendReviewReadinessSection("## Summary\n\nBody.");
assert.equal(extractReviewReadiness(first).present, true);
assert.equal(extractReviewReadiness(first).total, 4);
const second = appendReviewReadinessSection(first);
assert.equal(second, first);
assert.equal((second.match(/pr-quality-readiness-checklist:start/g) || []).length, 1);
});

it("appends cleanly to an empty body", () => {
const body = appendReviewReadinessSection("");
assert.equal(extractReviewReadiness(body).present, true);
assert.ok(body.startsWith("<!-- pr-quality-readiness-checklist:start -->"));
});
});

describe("collectPrQualityFailures", () => {
const allowed = ["dev"];

Expand Down
Loading
Loading