Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 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
11 changes: 6 additions & 5 deletions .github/scripts/enforce-pr-target.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,11 @@ describe("enforce-pr-target workflow", () => {
assert.match(workflow, /synchronize/);
});

it("checks out trusted default-branch scripts only (never PR head)", () => {
it("checks out trusted base-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, /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.ref\s*\}\}/);
// 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
125 changes: 122 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,101 @@ 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);
// 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).
if (start === -1 || end === -1 || end <= start) {
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;
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 +360,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