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
20 changes: 13 additions & 7 deletions .github/scripts/enforce-pr-target.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,17 @@ describe("enforce-pr-target workflow", () => {
assert.match(workflow, /synchronize/);
});

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/);
assert.match(workflow, /persist-credentials:\s*false/);
it("checks out trusted base-branch scripts only (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}/);
assert.match(checkoutStep, /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}/);
// 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/);
});

Expand All @@ -75,9 +81,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
142 changes: 139 additions & 3 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 @@ -109,13 +125,14 @@ function stripPrTemplateBoilerplate(text) {
}

function assessPrDescription(body) {
if (typeof body !== "string" || !body.trim()) {
const withoutReadiness = stripReviewReadinessSection(body);
if (typeof withoutReadiness !== "string" || !withoutReadiness.trim()) {
return { ok: false, reason: "empty" };
}
if (hasEscapedNewlines(body)) {
if (hasEscapedNewlines(withoutReadiness)) {
return { ok: false, reason: "escaped_newlines" };
}
const withoutTemplate = stripPrTemplateBoilerplate(body);
const withoutTemplate = stripPrTemplateBoilerplate(withoutReadiness);
const cleaned = clean(withoutTemplate);
if (!cleaned) {
const strippedComments = withoutTemplate.replace(/<!--[\s\S]*?-->/g, "").trim();
Expand Down Expand Up @@ -191,6 +208,118 @@ 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,
items: [],
};
}
const start = body.indexOf(REVIEW_READINESS_START);
const end = body.indexOf(REVIEW_READINESS_END);
const startCount = body.split(REVIEW_READINESS_START).length - 1;
const endCount = body.split(REVIEW_READINESS_END).length - 1;
// Any marker presence counts as present: an author-edited section that is
// inverted or partial must never trigger another append, or every `edited`
// event would stack a second checklist (and a second body write). Exactly
// one marker pair is required: duplicates are malformed, not complete.
if (
start === -1 ||
end === -1 ||
end <= start ||
startCount !== 1 ||
endCount !== 1
) {
return {
present: start !== -1 || end !== -1,
complete: false,
checked: 0,
total: 0,
items: [],
};
}
const section = body.slice(start + REVIEW_READINESS_START.length, end);
const boxes = [...section.matchAll(/^\s*[-*]\s+\[([ xX])\]\s+/gm)];
const total = boxes.length;
const items = boxes.map((match) => ({ checked: match[1] !== " " }));
const checked = items.filter((item) => item.checked).length;
return {
present: true,
complete:
total === REVIEW_READINESS_ITEMS.length && checked === total,
checked,
total,
items,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
};
}

/**
* 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`;
}

/**
* Remove the bot-managed readiness section from a body. Used so the bot's own
* checklist never counts as author-written description substance, and so a
* confirmed maintainer's body can retire the injected section.
*/
function stripReviewReadinessSection(body) {
if (typeof body !== "string") return body;
const start = body.indexOf(REVIEW_READINESS_START);
const end = body.indexOf(REVIEW_READINESS_END);
if (start === -1 || end === -1 || end <= start) return body;
// Malformed marker sets (duplicates, extra pairs) stay untouched: removing
// only one section would leave the body half-cleaned and still marked.
if (
body.split(REVIEW_READINESS_START).length - 1 !== 1 ||
body.split(REVIEW_READINESS_END).length - 1 !== 1
) {
return body;
}
const stripped =
body.slice(0, start) + body.slice(end + REVIEW_READINESS_END.length);
return stripped.replace(/\n{3,}/g, "\n\n").trimEnd();
}

function collectPrQualityFailures({
baseRef,
allowedBases,
Expand Down Expand Up @@ -248,11 +377,18 @@ 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,
stripReviewReadinessSection,
collectPrQualityFailures,
hasEscapedNewlines,
stripPrTemplateBoilerplate,
Expand Down
Loading
Loading