diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index c09f633265..840c2e4049 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -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/); }); @@ -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"); const failurePath = failureBlock[1]; assert.match(failurePath, /shouldStripTitlePrefix/); assert.match(failurePath, /!hasWrongBase/); diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index 4131d303b9..79768fbc9b 100644 --- a/.github/scripts/pr-quality.cjs +++ b/.github/scripts/pr-quality.cjs @@ -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 = ""; +const REVIEW_READINESS_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. @@ -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(//g, "").trim(); @@ -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, + }; +} + +/** + * 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, @@ -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, diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index 53586e1e62..4645cea988 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -4,11 +4,16 @@ 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, + stripReviewReadinessSection, collectPrQualityFailures, } = require("./pr-quality.cjs"); @@ -215,6 +220,213 @@ describe("hasScreenshotEvidence", () => { }); }); +describe("review readiness checklist", () => { + const SECTION = buildReviewReadinessSection(); + + it("builds exactly the four required boxes inside the markers", () => { + assert.ok(SECTION.includes("")); + assert.ok(SECTION.includes("")); + 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, + items: [], + }); + assert.deepEqual(extractReviewReadiness(null), { + present: false, + complete: false, + checked: 0, + total: 0, + items: [], + }); + }); + + 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, + items: [ + { checked: true }, + { checked: true }, + { checked: true }, + { checked: true }, + ], + }); + + 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, + items: [ + { checked: true }, + { checked: true }, + { checked: true }, + { checked: false }, + ], + }); + }); + + it("reports per-item state so the mirror marks the right boxes", () => { + const body = SECTION.replace( + "- [ ] My PR is ready for review.", + "- [x] My PR is ready for review.", + ); + const result = extractReviewReadiness(body); + assert.equal(result.checked, 1); + assert.deepEqual(result.items, [ + { checked: false }, + { checked: false }, + { checked: false }, + { checked: true }, + ]); + }); + + 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); + assert.equal(extractReviewReadiness(fewer).items.length, 3); + + const extra = SECTION.replace( + "", + "- [x] An extra box.\n", + ); + const result = extractReviewReadiness(extra); + assert.equal(result.complete, false); + assert.equal(result.total, 5); + assert.equal(result.items.length, 5); + }); + + it("treats inverted or partial markers as present-but-incomplete, never appends again", () => { + const inverted = [ + "## Summary", + "Body.", + "", + "residue", + "", + "- [x] orphan box", + ].join("\n"); + const extracted = extractReviewReadiness(inverted); + assert.equal(extracted.present, true); + assert.equal(extracted.complete, false); + assert.equal(appendReviewReadinessSection(inverted), inverted); + + const orphanEnd = "body\n"; + assert.equal(extractReviewReadiness(orphanEnd).present, true); + assert.equal(extractReviewReadiness(orphanEnd).complete, false); + assert.equal(appendReviewReadinessSection(orphanEnd), orphanEnd); + + const duplicate = SECTION + SECTION; + const duplicated = extractReviewReadiness(duplicate); + assert.equal(duplicated.present, true); + // A second marker pair is malformed, never complete — even when the first + // section's boxes would parse as checked (CodeRabbit round 3). + assert.equal(duplicated.complete, false); + assert.equal(duplicated.total, 0); + assert.equal(appendReviewReadinessSection(duplicate), duplicate); + assert.equal(stripReviewReadinessSection(duplicate), duplicate); + }); + + 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("")); + }); + + it("strips the marker-bounded section and leaves the rest intact", () => { + const body = [ + "## Summary", + "Author content.", + "", + SECTION, + "", + "## Test plan", + "- Ran the suite.", + ].join("\n"); + const stripped = stripReviewReadinessSection(body); + assert.equal(extractReviewReadiness(stripped).present, false); + assert.ok(stripped.includes("Author content.")); + assert.ok(stripped.includes("## Test plan")); + assert.ok(!stripped.includes("Review readiness checklist")); + }); + + it("strips a section-only body to empty and leaves markerless bodies alone", () => { + assert.equal(stripReviewReadinessSection(SECTION), ""); + assert.equal(stripReviewReadinessSection("plain body"), "plain body"); + assert.equal(stripReviewReadinessSection(null), null); + }); +}); + +describe("assessPrDescription with the readiness section", () => { + const SUBSTANTIAL = [ + "## Summary", + "", + "This change adds enough substantive detail for reviewers to understand the motivation and approach taken.", + "", + "## Test plan", + "", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"); + + it("never counts the injected checklist as description substance", () => { + // The bot's own injected section must not clear the description gate for + // an author who wrote nothing (Codex review round 2). + assert.equal(assessPrDescription(buildReviewReadinessSection()).ok, false); + assert.equal( + assessPrDescription("fix stuff\n\n" + buildReviewReadinessSection()).reason, + "thin", + ); + assert.equal( + assessPrDescription(SUBSTANTIAL + "\n\n" + buildReviewReadinessSection()).ok, + true, + ); + }); +}); + describe("collectPrQualityFailures", () => { const allowed = ["dev"]; diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index e7c475606a..856f55f9e1 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -29,16 +29,31 @@ jobs: - name: Checkout trusted PR-quality scripts uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: - ref: ${{ github.event.repository.default_branch }} + # 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. + ref: ${{ github.event.pull_request.base.sha }} persist-credentials: false - sparse-checkout: .github/scripts + sparse-checkout: | + .github/scripts + MAINTAINERS.md - name: Enforce PR target, ancestry, and description uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 with: script: | const path = require("path"); - const { collectPrQualityFailures } = require( + const fs = require("node:fs"); + const { + collectPrQualityFailures, + authorHasPushPermission, + extractReviewReadiness, + appendReviewReadinessSection, + stripReviewReadinessSection, + REVIEW_READINESS_ITEMS + } = require( path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), ); @@ -49,6 +64,11 @@ jobs: const LEGACY_COMMENT_MARKER = ""; const STATE_PATTERN = //; + const READINESS_MARKER = ""; + const READINESS_STATE_PATTERN = + //; + const READINESS_STATE_VERSION = 1; + const MAINTAINERS_FILE = "MAINTAINERS.md"; const { owner, repo } = context.repo; const pull_number = context.payload.pull_request.number; @@ -77,6 +97,16 @@ jobs: ); let botCommentId = botComment?.id ?? null; + const readinessComment = comments.find( + comment => + comment.user?.login === "github-actions[bot]" && + comment.body?.includes(READINESS_MARKER) + ); + let readinessCommentId = readinessComment?.id ?? null; + const storedReadinessState = parseReadinessState( + readinessComment?.body + ); + function parseState(body) { const match = body?.match(STATE_PATTERN); @@ -103,6 +133,145 @@ jobs: ); } + function parseReadinessState(body) { + const match = body?.match(READINESS_STATE_PATTERN); + + if (!match) { + return null; + } + + try { + return JSON.parse(match[1]); + } catch (error) { + core.warning( + `Could not parse stored readiness state: ${error.message}` + ); + + return null; + } + } + + function readinessStateMarker(state) { + return ( + "" + ); + } + + /** The enforcer comment state after every quality gate clears. */ + function clearedEnforcerState() { + return { + version: 1, + active: false, + autoDraftedByBot: false, + titlePrefixedByBot: false, + ancestryFailed: false, + descriptionFailed: false, + screenshotFailed: false + }; + } + + /** Fresh checklist-message state for a contributor PR. */ + function defaultReadinessState() { + return { + version: READINESS_STATE_VERSION, + autoDraftedByBot: false, + maintainersPinged: false + }; + } + + /** + * Maintainers from `MAINTAINERS.md` on the trusted default branch + * (checked out sparse by the step above). The file is the canonical + * list; mentioning these logins on the PR notifies them. + */ + function readMaintainerLogins() { + try { + const text = fs.readFileSync( + path.join(process.cwd(), MAINTAINERS_FILE), + "utf8" + ); + // Only the current-maintainers table is authoritative; the + // change log below it can mention retired accounts. + const sectionStart = text.indexOf("## Current maintainers"); + const nextHeading = text.indexOf( + "\n## ", + sectionStart + "## Current maintainers".length + ); + const section = + sectionStart === -1 + ? text + : text.slice( + sectionStart, + nextHeading === -1 ? text.length : nextHeading + ); + const logins = [ + ...section.matchAll( + /\[\@([A-Za-z0-9_-]+)\]\(https:\/\/github\.com\/[^)]*\)/g + ) + ].map(match => match[1]); + + return [...new Set(logins)]; + } catch (error) { + core.warning( + `Could not read ${MAINTAINERS_FILE}: ${error.message}` + ); + + return []; + } + } + + function readinessChecklistLines(readiness) { + return REVIEW_READINESS_ITEMS.map( + (item, index) => + `- ${readiness.items?.[index]?.checked ? "✅" : "⬜"} ${item}` + ); + } + + async function upsertReadinessComment(state, readiness, extra) { + const complete = readiness.present && readiness.complete; + const lines = [ + READINESS_MARKER, + readinessStateMarker(state), + "", + "## Review readiness checklist", + "", + readiness.present + ? "This PR is kept in **draft** until every requirement below is fulfilled. The tickable checklist has been added to your PR description — tick all four boxes there." + : "This PR is ready for review; the review readiness checklist is not required for this author.", + "", + ...(readiness.present ? readinessChecklistLines(readiness) : []), + "", + readiness.present + ? complete + ? "✅ **4/4** boxes ticked." + : `**${readiness.checked}/${readiness.total}** boxes ticked.` + : "", + "", + ...extra + ]; + + if (readinessCommentId) { + await github.rest.issues.updateComment({ + owner, + repo, + comment_id: readinessCommentId, + body: lines.join("\n") + }); + + return; + } + + const created = await github.rest.issues.createComment({ + owner, + repo, + issue_number: pull_number, + body: lines.join("\n") + }); + readinessCommentId = created.data.id; + } + function inlineCode(value) { return `\`${String(value).replaceAll("`", "\\`")}\``; } @@ -372,12 +541,57 @@ jobs: stackedBase }); - if (failures.length > 0) { - const hasWrongBase = failures.some( - failure => failure.code === "wrong_base" - ); - const willPrefixTitle = - hasWrongBase && !pr.title.startsWith(TITLE_PREFIX); + // The readiness gate applies to contributors (no push permission). + // Maintainers keep the failure-only contract: draft while quality + // gates fail, ready again once they clear. A failed permission + // lookup fails closed — the PR is treated as a contributor PR. + const authorIsMaintainer = + !permissionLookupFailed && authorHasPushPermission(authorPermission); + const checklistRequired = !authorIsMaintainer; + + // A confirmed maintainer does not need the bot's checklist: retire + // the injected section from the body so it stops rendering as a + // gate on later runs. + let readiness = extractReviewReadiness(pr.body); + if (!checklistRequired && readiness.present) { + const strippedBody = stripReviewReadinessSection(pr.body ?? ""); + if (strippedBody !== pr.body) { + await github.rest.pulls.update({ + owner, + repo, + pull_number, + body: strippedBody + }); + } + readiness = extractReviewReadiness(strippedBody); + } + + // The tickable checklist lives in the PR body, because only the PR + // author can edit it. Inject it once; the HTML markers make the + // injection idempotent, so the `edited` event this write triggers + // cannot churn the body on every run. + if (checklistRequired && !readiness.present) { + const injectedBody = appendReviewReadinessSection(pr.body ?? ""); + await github.rest.pulls.update({ + owner, + repo, + pull_number, + body: injectedBody + }); + readiness = extractReviewReadiness(injectedBody); + } + const checklistComplete = readiness.present && readiness.complete; + + // A contributor PR stays a draft while the checklist is open, even + // when every quality gate already passes. + const mustDraft = + failures.length > 0 || (checklistRequired && !checklistComplete); + + if (mustDraft) { + let draftConverted = false; + const readinessState = storedReadinessState + ? { ...storedReadinessState } + : defaultReadinessState(); const state = storedState?.active ? { ...storedState } : { @@ -389,17 +603,11 @@ jobs: descriptionFailed: false, screenshotFailed: false }; - - state.ancestryFailed = failures.some( - failure => failure.code === "wrong_ancestry" - ); - state.descriptionFailed = failures.some( - failure => failure.code === "bad_description" - ); - state.screenshotFailed = failures.some( - failure => failure.code === "missing_ui_screenshot" + const hasWrongBase = failures.some( + failure => failure.code === "wrong_base" ); - + const willPrefixTitle = + hasWrongBase && !pr.title.startsWith(TITLE_PREFIX); const shouldStripTitlePrefix = !hasWrongBase && state.titlePrefixedByBot && @@ -411,28 +619,9 @@ jobs: state.titlePrefixedByBot = true; } - let draftConversionFailed = false; - const failureSections = buildFailureSections(failures); - - await upsertComment( - [ - COMMENT_MARKER, - stateMarker(state), - "", - ...failureSections, - "", - "Recording ownership state before applying title/draft changes…" - ].join("\n") - ); - - if (willPrefixTitle) { - await github.rest.pulls.update({ - owner, - repo, - pull_number, - title: `${TITLE_PREFIX}${pr.title}` - }); - } else if (shouldStripTitlePrefix) { + // A stale bot-owned prefix comes off once, whichever path owns + // the draft: the branch is fixed even when the checklist is not. + if (shouldStripTitlePrefix) { await github.rest.pulls.update({ owner, repo, @@ -440,22 +629,47 @@ jobs: title: pr.title.slice(TITLE_PREFIX.length) }); state.titlePrefixedByBot = false; - await upsertComment( + } + + if (checklistRequired && !pr.draft && !checklistComplete) { + // Claim draft ownership before the mutation so a successful + // convert followed by a failed comment still restores later + // (same checkpoint discipline as the quality-failure path). + readinessState.autoDraftedByBot = true; + await upsertReadinessComment( + readinessState, + readiness, [ - COMMENT_MARKER, - stateMarker(state), - "", - ...failureSections, - "", - "Stale title prefix removed; continuing…" - ].join("\n") + "This PR stays in draft until every box above is ticked." + ] ); } - if (!pr.draft) { - // Claim draft ownership before the mutation so a successful - // convert followed by a failed comment still restores later. - state.autoDraftedByBot = true; + if (failures.length > 0) { + state.ancestryFailed = failures.some( + failure => failure.code === "wrong_ancestry" + ); + state.descriptionFailed = failures.some( + failure => failure.code === "bad_description" + ); + state.screenshotFailed = failures.some( + failure => failure.code === "missing_ui_screenshot" + ); + + let draftConversionFailed = false; + const failureSections = buildFailureSections(failures); + + if (checklistRequired && !checklistComplete) { + failureSections.push( + "", + "⏳ **Review readiness checklist**", + "", + `This pull request stays in draft until all four boxes of the readiness checklist in the description are ticked (currently ${readiness.checked}/${readiness.total}).`, + "", + `@${pr.user.login} Tick the boxes once your local CI is green, your branch is on the latest ${inlineCode(DEFAULT_BASE)} commit, and every correct Codex and CodeRabbit finding is fixed.` + ); + } + await upsertComment( [ COMMENT_MARKER, @@ -463,11 +677,23 @@ jobs: "", ...failureSections, "", - "Draft conversion pending…" + "Recording ownership state before applying title/draft changes…" ].join("\n") ); - try { - await convertToDraft(); + + if (willPrefixTitle) { + await github.rest.pulls.update({ + owner, + repo, + pull_number, + title: `${TITLE_PREFIX}${pr.title}` + }); + } + + if (!pr.draft) { + // Claim draft ownership before the mutation so a successful + // convert followed by a failed comment still restores later. + state.autoDraftedByBot = true; await upsertComment( [ COMMENT_MARKER, @@ -475,49 +701,149 @@ jobs: "", ...failureSections, "", - "Draft conversion succeeded; finalising explanation…" + "Draft conversion pending…" ].join("\n") ); - } catch (error) { - draftConversionFailed = true; - state.autoDraftedByBot = false; - core.warning( - `Could not convert pull request to draft: ${error.message}` + try { + await convertToDraft(); + draftConverted = true; + readinessState.autoDraftedByBot = true; + await upsertComment( + [ + COMMENT_MARKER, + stateMarker(state), + "", + ...failureSections, + "", + "Draft conversion succeeded; finalising explanation…" + ].join("\n") + ); + } catch (error) { + draftConversionFailed = true; + state.autoDraftedByBot = false; + core.warning( + `Could not convert pull request to draft: ${error.message}` + ); + } + } + + const draftExplanation = draftConversionFailed + ? "Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required `enforce-target` check will keep failing until every issue above is resolved." + : state.autoDraftedByBot + ? "This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again." + : "This pull request was already a draft. Its draft status will be preserved after every issue above is resolved."; + + const finalSections = [...failureSections]; + + if (hasWrongBase && state.titlePrefixedByBot) { + finalSections.push( + "", + `Its title has been prefixed with ${inlineCode(TITLE_PREFIX.trim())}.` + ); + } + + await upsertComment( + [ + COMMENT_MARKER, + stateMarker(state), + "", + ...finalSections, + "", + draftExplanation + ].join("\n") + ); + + if (checklistRequired) { + await upsertReadinessComment( + readinessState, + readiness, + [ + checklistComplete + ? "✅ **All four boxes are ticked.** This PR still stays in draft until the issues above are resolved." + : pr.draft || draftConverted + ? "This PR stays in draft until every box above is ticked." + : "Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked." + ] ); } + + core.setFailed(`PR quality gate failed: ${failureSummary(failures)}`); + return; } - const draftExplanation = draftConversionFailed - ? "Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required `enforce-target` check will keep failing until every issue above is resolved." - : state.autoDraftedByBot - ? "This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again." - : "This pull request was already a draft. Its draft status will be preserved after every issue above is resolved."; - - const finalSections = [...failureSections]; - - if (hasWrongBase && state.titlePrefixedByBot) { - finalSections.push( - "", - `Its title has been prefixed with ${inlineCode(TITLE_PREFIX.trim())}.` + // No quality failure; the draft is owed by the open checklist. + // Prior enforcer history (title prefix, earlier failures) gets a + // closing confirmation; the readiness comment owns the draft now. + if (storedState?.active || botComment) { + const prefixResult = shouldStripTitlePrefix + ? `The ${inlineCode(TITLE_PREFIX.trim())} title prefix has been removed.` + : "The title was left unchanged."; + await upsertComment( + [ + COMMENT_MARKER, + stateMarker(clearedEnforcerState()), + "", + "✅ **PR quality gates passed**", + "", + `This pull request now targets ${inlineCode(pr.base.ref)} with acceptable ancestry, description, and UI screenshot coverage. It stays in draft until the review readiness checklist is complete.`, + "", + `${prefixResult} The draft is owned by the checklist message below.` + ].join("\n") ); } - await upsertComment( + if (!pr.draft && !draftConverted) { + try { + await convertToDraft(); + draftConverted = true; + } catch (error) { + readinessState.autoDraftedByBot = false; + core.warning( + `Could not convert pull request to draft: ${error.message}` + ); + core.setFailed( + "PR quality gate failed: could not convert the pull request to draft while the review readiness checklist is open." + ); + } + } + + await upsertReadinessComment( + readinessState, + readiness, [ - COMMENT_MARKER, - stateMarker(state), - "", - ...finalSections, - "", - draftExplanation - ].join("\n") + pr.draft || draftConverted + ? "This PR stays in draft until every box above is ticked." + : "Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked." + ] ); - - core.setFailed(`PR quality gate failed: ${failureSummary(failures)}`); return; } - if (!storedState?.active) { + if (!storedState?.active && !checklistRequired) { + // A maintainer PR drafted while the permission lookup was failing + // (fail-closed) gets restored once the lookup recovers. + if (storedReadinessState?.autoDraftedByBot && pr.draft) { + let recoveryFailed = false; + try { + await markReadyForReview(); + } catch (error) { + recoveryFailed = true; + core.warning( + `Could not mark pull request ready for review: ${error.message}` + ); + } + await upsertReadinessComment( + recoveryFailed + ? { ...storedReadinessState } + : { ...storedReadinessState, autoDraftedByBot: false }, + readiness, + [ + recoveryFailed + ? "Automatic ready-for-review conversion failed; the PR stays a draft and will be retried on the next run." + : "✅ This PR is ready for review." + ] + ); + } core.info( "All PR quality gates passed and there is no active bot state." ); @@ -526,7 +852,7 @@ jobs: } if ( - storedState.titlePrefixedByBot && + storedState?.titlePrefixedByBot && pr.title.startsWith(TITLE_PREFIX) ) { await github.rest.pulls.update({ @@ -538,12 +864,14 @@ jobs: } let readyConversionFailed = false; - if ( - storedState.autoDraftedByBot && - pr.draft - ) { + let readyConverted = false; + const shouldMarkReady = + (storedState?.active && storedState.autoDraftedByBot) || + (checklistRequired && checklistComplete); + if (shouldMarkReady && pr.draft) { try { await markReadyForReview(); + readyConverted = true; } catch (error) { readyConversionFailed = true; core.warning( @@ -552,45 +880,75 @@ jobs: } } - const completedState = readyConversionFailed - ? { - version: 1, - active: true, - autoDraftedByBot: true, - titlePrefixedByBot: false, - ancestryFailed: false, - descriptionFailed: false, - screenshotFailed: false - } - : { - version: 1, - active: false, - autoDraftedByBot: false, - titlePrefixedByBot: false, - ancestryFailed: false, - descriptionFailed: false, - screenshotFailed: false - }; - - const titleResult = storedState.titlePrefixedByBot - ? `The ${inlineCode(TITLE_PREFIX.trim())} title prefix has been removed.` - : "The title was left unchanged."; - - const draftResult = readyConversionFailed - ? "Automatic ready-for-review conversion failed; please mark the pull request ready manually if it is still a draft." - : storedState.autoDraftedByBot - ? "The pull request has been marked ready for review again." - : "Its existing draft status has been preserved."; - - await upsertComment( - [ - COMMENT_MARKER, - stateMarker(completedState), - "", - "✅ **PR quality gates passed**", - "", - `This pull request now targets ${inlineCode(pr.base.ref)} with acceptable ancestry, description, and UI screenshot coverage.`, - "", - `${titleResult} ${draftResult}` - ].join("\n") - ); + // The enforcer comment only exists when there was something to say; + // a clean contributor PR that never failed a quality gate has none. + if (storedState?.active || botComment) { + const completedState = readyConversionFailed + ? { ...clearedEnforcerState(), active: true, autoDraftedByBot: true } + : clearedEnforcerState(); + + const titleResult = storedState?.titlePrefixedByBot + ? `The ${inlineCode(TITLE_PREFIX.trim())} title prefix has been removed.` + : "The title was left unchanged."; + + const draftResult = readyConversionFailed + ? "Automatic ready-for-review conversion failed; please mark the pull request ready manually if it is still a draft." + : storedState?.autoDraftedByBot + ? "The pull request has been marked ready for review again." + : "Its existing draft status has been preserved."; + + await upsertComment( + [ + COMMENT_MARKER, + stateMarker(completedState), + "", + "✅ **PR quality gates passed**", + "", + `This pull request now targets ${inlineCode(pr.base.ref)} with acceptable ancestry, description, and UI screenshot coverage.${ + checklistRequired + ? " The review readiness checklist is complete." + : "" + }`, + "", + `${titleResult} ${draftResult}` + ].join("\n") + ); + } + + // Checklist completion lifts the contributor draft and pings the + // maintainers from `MAINTAINERS.md` (minus the PR author). + if (checklistRequired && checklistComplete) { + const readinessState = storedReadinessState + ? { ...storedReadinessState } + : defaultReadinessState(); + const maintainers = readMaintainerLogins().filter( + login => login !== pr.user.login + ); + let notified = false; + if (!readinessState.maintainersPinged && maintainers.length > 0) { + readinessState.maintainersPinged = true; + notified = true; + } + + await upsertReadinessComment( + readinessState, + readiness, + [ + "✅ **All four boxes are ticked.**", + readyConverted + ? "This pull request has been marked Ready for Review." + : pr.draft + ? "Automatic ready-for-review conversion failed; please mark the pull request ready manually." + : "This pull request is already Ready for Review.", + notified && maintainers.length > 0 + ? `Maintainers notified: ${maintainers + .map(login => `@${login}`) + .join(" ")}` + : maintainers.length > 0 + ? `Maintainers: ${maintainers + .map(login => `@${login}`) + .join(" ")}` + : "Maintainers will be notified." + ] + ); + } diff --git a/AGENTS.md b/AGENTS.md index 245961ee39..d7c0c2cf86 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -178,6 +178,12 @@ The **`enforce-target`** CI check rejects pull requests whose head ancestry sits on the **`main`** tip while far behind **`dev`**, and rejects empty, thin, or malformed descriptions; PRs whose title or description mentions `gui` must include a screenshot of the UI change in the description. +Contributor PRs (authors without repository push permission) open in draft and +stay there until a four-box review-readiness checklist in the description is +complete: local CI green, branch on the latest `dev` commit, all correct Codex +and CodeRabbit findings fixed, and the ready-for-review confirmation. When all +four boxes are ticked the gate marks the PR ready and notifies the maintainers +listed in `MAINTAINERS.md` (excluding the author). Authors with repository push permission skip the ancestry heuristic only. As with approval requirements in [`MAINTAINERS.md`](./MAINTAINERS.md), this is enforced by convention until branch protection is configured. diff --git a/MAINTAINERS.md b/MAINTAINERS.md index 87fb669dc3..c8a94010c6 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -25,6 +25,12 @@ see [The retired `dev2-go` line](#the-retired-dev2-go-line). ancestry sits on the **`main`** tip while far behind **`dev`**, and rejects empty, thin, or malformed descriptions; PRs whose title or description mentions `gui` must include a screenshot of the UI change in the description. + Contributor PRs (authors without repository push permission) open in draft + and stay there until a four-box review-readiness checklist in the + description is complete: local CI green, branch on the latest `dev` commit, + all correct Codex and CodeRabbit findings fixed, and the ready-for-review + confirmation. When all four boxes are ticked the gate marks the PR ready and + notifies the maintainers listed in `MAINTAINERS.md` (excluding the author). Authors with repository push permission skip the ancestry heuristic only. As with the approval requirement above, this is enforced by convention until branch protection is configured (see the note under the change log). diff --git a/docs-site/src/content/docs/contributing/pr-quality.md b/docs-site/src/content/docs/contributing/pr-quality.md index 6e365e1fc7..ae6a27cd44 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -40,6 +40,15 @@ tells you exactly what to change: plan** (or equivalent substance). When the title or description mentions `gui`, the description must include a screenshot of the UI change; the check keeps the PR a draft and comments until the screenshot is present. + Contributor PRs (authors without repository push permission) open in draft + and stay there until a four-box review-readiness checklist in the + description is complete: local CI green, the branch on the latest `dev` + commit, all correct Codex and CodeRabbit findings fixed, and the + ready-for-review confirmation. Once every box is ticked the check marks the + PR ready for review and notifies the maintainers listed in `MAINTAINERS.md` + (excluding the author). A retarget to `dev` clears the wrong-branch message + automatically and is remembered by the gate; the draft stays until the + checklist is complete. - **Hygiene.** Behavior changes need a test; new lint or type suppressions, focused or skipped tests, empty catch blocks, edited generated output, and a diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index d808000520..e35788cb10 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -10,10 +10,34 @@ import { /** Final enforcer comment body after pending/draft checkpoints. */ function lastEnforcerCommentBody(result: HarnessResult): string { - const updates = callsTo(result, "issues.updateComment") as Array<{ body: string }>; + const marker = ""; + const legacyMarker = ""; + const updates = (callsTo(result, "issues.updateComment") as Array<{ body: string }>) + .filter(call => call.body.includes(marker) || call.body.includes(legacyMarker)); if (updates.length > 0) return updates[updates.length - 1]!.body; const creates = callsTo(result, "issues.createComment") as Array<{ body: string }>; - return creates[creates.length - 1]!.body; + const enforcerCreates = creates.filter( + call => call.body.includes(marker) || call.body.includes(legacyMarker), + ); + const chosen = enforcerCreates.length > 0 ? enforcerCreates : creates; + if (chosen.length === 0) { + throw new Error("scenario recorded no enforcer comment"); + } + return chosen[chosen.length - 1]!.body; +} + +/** Final review-readiness comment body (the checklist message). */ +function lastReadinessCommentBody(result: HarnessResult): string { + const marker = ""; + const updates = (callsTo(result, "issues.updateComment") as Array<{ body: string }>) + .filter(call => call.body.includes(marker)); + if (updates.length > 0) return updates[updates.length - 1]!.body; + const creates = callsTo(result, "issues.createComment") as Array<{ body: string }>; + const readinessCreates = creates.filter(call => call.body.includes(marker)); + if (readinessCreates.length === 0) { + throw new Error("scenario recorded no readiness comment"); + } + return readinessCreates[readinessCreates.length - 1]!.body; } const root = new URL("../", import.meta.url); @@ -639,7 +663,7 @@ describe("GitHub Actions hardening", () => { return { workflow, jobs, steps: steps!, allSteps, script }; } - const SCRIPT_LOAD = ["require", "require"] as const; + const SCRIPT_LOAD = ["require", "require", "require"] as const; /** Reads every allowed-base PR performs before any enforcement writes. */ function readsAllowedBase(tail: string[] = []): string[] { @@ -816,9 +840,15 @@ describe("GitHub Actions hardening", () => { "sparse-checkout", ]); expect(checkout.with).toEqual({ - ref: "${{ github.event.repository.default_branch }}", + // The event's base commit, not the repository default: pull_request_target + // 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 }}", "persist-credentials": false, - "sparse-checkout": ".github/scripts", + // MAINTAINERS.md rides along so the completion ping reads the canonical + // maintainer list from the same trusted base revision as the scripts. + "sparse-checkout": ".github/scripts\nMAINTAINERS.md\n", }); expect(Object.keys(scriptStep).sort()).toEqual(["name", "uses", "with"]); @@ -946,21 +976,27 @@ describe("GitHub Actions hardening", () => { return found; } - // The two title rewrites — one adds the prefix, one removes it on a correct - // retarget. `base`, `state`, and `body` are all accepted by this endpoint - // and none of them belong here. + // Five `pulls.update` sites: the maintainer checklist retirement and the + // checklist injection (body only), plus the prefix add, the stale-prefix + // strip, and the restore-half strip. `base` and `state` are accepted by + // this endpoint and none of them belong anywhere here. expect(callArgs("github.rest.pulls.update")).toEqual([ + ["body", "owner", "pull_number", "repo"], + ["body", "owner", "pull_number", "repo"], ["owner", "pull_number", "repo", "title"], ["owner", "pull_number", "repo", "title"], ["owner", "pull_number", "repo", "title"], ]); - // Both comment writes address the PR being enforced, by its own number. + // Both comment families (quality enforcer + readiness checklist) address + // the PR being enforced, by its own number. expect(callArgs("github.rest.issues.createComment")).toEqual([ ["body", "issue_number", "owner", "repo"], + ["body", "issue_number", "owner", "repo"], ]); expect(callArgs("github.rest.issues.updateComment")).toEqual([ ["body", "comment_id", "owner", "repo"], + ["body", "comment_id", "owner", "repo"], ]); // …and the number is `pull_number`, not a literal. `issue_number: 1` has the @@ -1029,6 +1065,97 @@ describe("GitHub Actions hardening", () => { const BOT = "github-actions[bot]"; const MARKER = ""; const LEGACY_MARKER = ""; + const READINESS_MARKER = ""; + const CHECKLIST_START = ""; + const CHECKLIST_END = ""; + const CHECKLIST_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.", + ]; + const CONTRIBUTOR_BODY = [ + "## Summary", + "", + "This change adds enough substantive detail for reviewers to understand the motivation and approach taken.", + "", + "## Test plan", + "", + "- Run `bun test tests/ci-workflows.test.ts`", + ].join("\n"); + /** + * Fixture for the trusted `MAINTAINERS.md`: the current-maintainers table + * plus a change-log mention, so the section scoping of the ping is proven + * and the scenario does not depend on the live repository file. + */ + const MAINTAINERS_FIXTURE = [ + "## Current maintainers", + "", + "| GitHub account | Project role | Responsibilities |", + "| --- | --- | --- |", + "| [@lidge-jun](https://github.com/lidge-jun) | Project owner | x |", + "| [@Ingwannu](https://github.com/Ingwannu) | Maintainer | x |", + "| [@Wibias](https://github.com/Wibias) | Maintainer | x |", + "", + "## Change log", + "", + "- [@Wibias](https://github.com/Wibias) was added as a maintainer.", + ].join("\n"); + + /** A PR body whose readiness checklist has exactly `checked` boxes ticked. */ + function readinessChecklistBody(checked: number, base = CONTRIBUTOR_BODY): string { + const boxes = CHECKLIST_ITEMS.map((item, index) => + (index === CHECKLIST_ITEMS.length - 1 ? "\n" : "") + + `- [${index < checked ? "x" : " "}] ${item}`, + ); + return [ + base, + CHECKLIST_START, + "## Review readiness checklist", + "", + ...boxes, + CHECKLIST_END, + ].join("\n"); + } + + function readinessComment(state: Record): Comment { + return { + id: 8, + user: { login: BOT }, + body: [ + READINESS_MARKER, + ``, + "about readiness", + ].join("\n"), + }; + } + + /** + * The writes a fresh contributor PR triggers on `dev` with no quality + * failures: inject the checklist, then draft it with the checklist message. + */ + const CONTRIBUTOR_CLEAN_TAIL = [ + "pulls.update", + "issues.createComment", + "graphql", + "issues.updateComment", + ]; + + /** + * The writes a fresh wrong-base contributor PR triggers: inject the + * checklist, then the existing enforcer sequence plus the checklist message. + */ + const CONTRIBUTOR_WRONG_BASE_TAIL = [ + "pulls.update", + "issues.createComment", + "issues.createComment", + "pulls.update", + "issues.updateComment", + "graphql", + "issues.updateComment", + "issues.updateComment", + "issues.updateComment", + ]; function botComment(state: Record, title = "Add a thing") { return { @@ -1043,13 +1170,331 @@ describe("GitHub Actions hardening", () => { } test("a PR targeting dev is left completely alone", async () => { - const result = await run({ pr: { base: { ref: "dev" } } }); + const result = await run({ + pr: { base: { ref: "dev" } }, + authorPermission: "write", + }); // Reads only. If a rewrite adds a write here, it appears in this list. expect(methodsOf(result)).toEqual(readsAllowedBase()); expect(result.logs.join(" ")).toContain("All PR quality gates passed"); }); + test("a contributor PR targeting dev is drafted with a readiness checklist", async () => { + const result = await run({ pr: { base: { ref: "dev" } } }); + + // The gate keeps the PR in draft until the four-box checklist in the + // description is complete — even though every quality gate passes. The + // check itself stays green: no setFailed for a pending checklist. + expect(methodsOf(result)).toEqual(readsAllowedBase(CONTRIBUTOR_CLEAN_TAIL)); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + + const [injected] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(injected.body).toContain(CHECKLIST_START); + expect(injected.body).toContain(CHECKLIST_END); + expect(injected.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(injected.body).toContain("- [ ] My PR is ready for review."); + + const [draft] = callsTo(result, "graphql") as [{ query: string }]; + expect(draft.query).toContain("convertPullRequestToDraft"); + + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("**0/4** boxes ticked"); + expect(readinessBody).toContain(READINESS_MARKER); + expect(readinessBody).toContain('"maintainersPinged":false'); + }); + + test("a failed checklist draft conversion fails the check closed", async () => { + // The enforcer path soft-fails a failed draft conversion with a red + // check. The readiness path must fail closed the same way: a contributor + // PR that stays ready with an open checklist is exactly the state the + // gate exists to prevent. + const { script } = await readEnforcePrTarget(); + const result = await runEnforcePrTarget(script, { + pr: { base: { ref: "dev" }, draft: false }, + failOn: ["graphql"], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "pulls.update", + "issues.createComment", + "graphql", + "issues.updateComment", + ])); + // Ownership is checkpointed before the mutation (pending claim), then + // cleared when the conversion fails, so a later permission recovery + // cannot leave the bot-created draft in place forever. + const [pending] = callsTo(result, "issues.createComment") as [{ body: string }]; + expect(pending.body).toContain('"autoDraftedByBot":true'); + expect(lastReadinessCommentBody(result)).toContain('"autoDraftedByBot":false'); + expect(lastReadinessCommentBody(result)).toContain( + "Automatic draft conversion failed", + ); + expect( + result.warnings.some(w => + w.includes("could not convert the pull request to draft while the review readiness checklist is open"), + ), + ).toBe(true); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(true); + }); + + test("ticking every checklist box marks the contributor PR ready and pings maintainers", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + }); + + // No prior enforcer history: the checklist completion alone lifts the + // draft and notifies the maintainers from MAINTAINERS.md. + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "graphql", + "issues.createComment", + ])); + const [ready] = callsTo(result, "graphql") as [{ query: string }]; + expect(ready.query).toContain("markPullRequestReadyForReview"); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("**4/4** boxes ticked"); + expect(readinessBody).toContain("Maintainers notified: @lidge-jun @Ingwannu @Wibias"); + expect(readinessBody).toContain('"maintainersPinged":true'); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("an empty PR cannot be laundered into ready by ticking the injected boxes", async () => { + // The unit tests pin `assessPrDescription` against the injected section. + // This pins the sequence that would exploit it end to end, because the + // exploit needs two runs and a body the bot itself wrote in between: + // open with no description (run one injects), tick the four boxes the + // bot just added (run two), and the PR is ready for review with the + // author having written nothing at all. + const { script } = await readEnforcePrTarget(); + const injectRun = await runEnforcePrTarget(script, { + pr: { base: { ref: "dev" }, draft: false, body: "" }, + authorPermission: "read", + }); + expect( + injectRun.warnings.some( + w => w.startsWith("setFailed:") && w.includes("bad description"), + ), + ).toBe(true); + + // Take the body the bot actually wrote, not a hand-built fixture: the + // exploit is only real if the injected text is what gets ticked. + const injectedBody = ( + callsTo(injectRun, "pulls.update") as Array<{ body?: string }> + ).find(call => typeof call.body === "string")?.body; + expect(injectedBody).toContain(CHECKLIST_START); + + const tickedRun = await runEnforcePrTarget(script, { + pr: { + base: { ref: "dev" }, + draft: true, + body: injectedBody!.replace(/- \[ \]/g, "- [x]"), + }, + authorPermission: "read", + maintainersFile: MAINTAINERS_FIXTURE, + }); + expect( + tickedRun.warnings.some( + w => w.startsWith("setFailed:") && w.includes("bad description"), + ), + ).toBe(true); + // `markPullRequestReadyForReview` goes out over `graphql`. Four ticked + // boxes must not summon it while the description gate is still failing. + expect(methodsOf(tickedRun)).not.toContain("graphql"); + }); + + test("the injected checklist does not satisfy the gui screenshot gate", async () => { + // Same laundering shape on the screenshot axis: the injected section adds + // renderable structure but no image, so a gui-cued contributor PR with a + // complete checklist and no screenshot must still fail and stay drafted. + const guiBody = [ + "## Summary", + "", + "Reworks the gui settings panel so the provider list keeps its scroll", + "position when a preset is applied from the sidebar.", + "", + "## Test plan", + "", + "`bun run build:gui` — pass; verified by hand in the dashboard.", + ].join("\n"); + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4, guiBody), + }, + authorPermission: "read", + maintainersFile: MAINTAINERS_FIXTURE, + }); + expect( + result.warnings.some( + w => w.startsWith("setFailed:") && w.includes("screenshot"), + ), + ).toBe(true); + expect(methodsOf(result)).not.toContain("graphql"); + }); + + test("a maintainer PR drafted during a permission-lookup failure is restored once the lookup recovers", async () => { + // The permission lookup fails closed: a clean maintainer PR is treated + // as a contributor PR, gets the checklist and a draft. When the lookup + // recovers, the early return must not leave that PR drafted forever — + // the readiness state records the bot's draft, and the recovery path + // undoes it. + const { script } = await readEnforcePrTarget(); + const duringFailure = await runEnforcePrTarget(script, { + pr: { base: { ref: "dev" }, draft: false }, + failPermissionLookup: true, + }); + expect(methodsOf(duringFailure)).toEqual(readsAllowedBase([ + "pulls.update", + "issues.createComment", + "graphql", + "issues.updateComment", + ])); + expect(lastReadinessCommentBody(duringFailure)).toContain('"autoDraftedByBot":true'); + + const recovered = await runEnforcePrTarget(script, { + pr: { base: { ref: "dev" }, draft: true, body: readinessChecklistBody(0) }, + authorPermission: "write", + maintainersFile: MAINTAINERS_FIXTURE, + comments: [readinessComment({ + version: 1, + autoDraftedByBot: true, + maintainersPinged: false, + })], + }); + expect(methodsOf(recovered)).toEqual(readsAllowedBase([ + "pulls.update", + "graphql", + "issues.updateComment", + ])); + // The injected checklist is retired from the maintainer's body. + const [stripped] = callsTo(recovered, "pulls.update") as [{ body: string }]; + expect(stripped.body).not.toContain(CHECKLIST_START); + const [ready] = callsTo(recovered, "graphql") as [{ query: string }]; + expect(ready.query).toContain("markPullRequestReadyForReview"); + const readinessBody = lastReadinessCommentBody(recovered); + expect(readinessBody).toContain("not required for this author"); + expect(readinessBody).not.toContain("kept in **draft**"); + expect(readinessBody).not.toContain("⬜"); + expect(readinessBody).toContain('"autoDraftedByBot":false'); + expect(recovered.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("permission recovery keeps draft ownership when ready conversion fails", async () => { + // If markReadyForReview fails transiently during recovery, the readiness + // state must keep autoDraftedByBot so a later run retries — otherwise + // the maintainer's PR stays a draft forever with a comment claiming it + // is ready. + const { script } = await readEnforcePrTarget(); + const result = await runEnforcePrTarget(script, { + pr: { base: { ref: "dev" }, draft: true, body: readinessChecklistBody(0) }, + authorPermission: "write", + maintainersFile: MAINTAINERS_FIXTURE, + comments: [readinessComment({ + version: 1, + autoDraftedByBot: true, + maintainersPinged: false, + })], + failOn: ["graphql"], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "pulls.update", + "graphql", + "issues.updateComment", + ])); + expect(result.warnings.some(w => + w.includes("Could not mark pull request ready for review"), + )).toBe(true); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain('"autoDraftedByBot":true'); + expect(readinessBody).toContain("will be retried on the next run"); + expect(readinessBody).not.toContain("✅ This PR is ready for review."); + }); + + test("a contributor completing the checklist with a corrupted enforcer comment still completes", async () => { + // `parseState` returns null for malformed state, but the bot comment + // still exists — the restore path must not dereference `storedState` + // unguarded (CodeRabbit critical + Codex review P2). + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + comments: [{ + id: 7, + user: { login: BOT }, + body: `${MARKER}\n`, + }], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "graphql", + "issues.updateComment", + "issues.createComment", + ])); + const [ready] = callsTo(result, "graphql") as [{ query: string }]; + expect(ready.query).toContain("markPullRequestReadyForReview"); + const [updated] = callsTo(result, "issues.updateComment") as [{ body: string }]; + expect(updated.body).toContain('"active":false'); + expect(updated.body).toContain("The title was left unchanged."); + expect(result.warnings.join(" ")).toContain("Could not parse stored workflow state"); + }); + + test("a complete checklist does not lift the draft while the base is wrong", async () => { + const result = await run({ + pr: { + base: { ref: "main" }, + draft: true, + title: "Add a thing", + body: readinessChecklistBody(4), + }, + }); + + expect(methodsOf(result)).toEqual(readsWrongBase([ + "issues.createComment", + "pulls.update", + "issues.updateComment", + "issues.createComment", + ])); + expect(callsTo(result, "graphql")).toEqual([]); + expect(lastEnforcerCommentBody(result)).toContain("Wrong target branch"); + expect(lastReadinessCommentBody(result)).toContain("All four boxes are ticked"); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(true); + }); + + test("a maintainer PR on the wrong base gets no readiness checklist", async () => { + const result = await run({ + pr: { base: { ref: "main" }, title: "Add a thing", draft: false }, + authorPermission: "write", + }); + + // The maintainer contract is unchanged: draft on failure, explain, and + // nothing else — no checklist injection, no readiness message. + expect(methodsOf(result)).toEqual(readsWrongBase([ + "issues.createComment", + "pulls.update", + "issues.updateComment", + "graphql", + "issues.updateComment", + "issues.updateComment", + ])); + expect(callsTo(result, "pulls.update")).toEqual([ + { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "[WRONG BRANCH] Add a thing" }, + ]); + expect( + (callsTo(result, "issues.createComment") as [{ body: string }]) + .every(call => !call.body.includes(READINESS_MARKER)), + ).toBe(true); + }); + const HEAD_SHA = "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"; const ANCESTRY_FAIL_COMPARES = { [`main...${HEAD_SHA}`]: { ahead_by: 1, behind_by: 0 }, @@ -1063,17 +1508,22 @@ describe("GitHub Actions hardening", () => { compareByBasehead: ANCESTRY_FAIL_COMPARES, }); - expect(callsTo(result, "pulls.update")).toEqual([]); + // No wrong base, so no title write — the checklist injection is the only + // `pulls.update`, and the contributor flow adds the readiness message. expect(methodsOf(result)).toEqual(readsAllowedBase([ + "pulls.update", + "issues.createComment", "issues.createComment", "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", + "issues.updateComment", ])); const commentBody = lastEnforcerCommentBody(result); expect(commentBody).toContain("Wrong branch ancestry"); expect(commentBody).not.toContain("Wrong target branch"); + expect(commentBody).toContain("Review readiness checklist"); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); expect(result.warnings.some((w) => w.includes("wrong ancestry"))).toBe(true); }); @@ -1095,7 +1545,13 @@ describe("GitHub Actions hardening", () => { expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); expect(lastEnforcerCommentBody(result)).toContain("Pull request description"); expect(lastEnforcerCommentBody(result)).toContain("body is empty"); + // The bot also injects the checklist, so the draft conversion is the only + // GraphQL mutation. expect(callsTo(result, "graphql")).toHaveLength(1); + const [draft] = callsTo(result, "graphql") as [{ query: string }]; + expect(draft.query).toContain("convertPullRequestToDraft"); + const [injected] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(injected.body).toContain(CHECKLIST_START); }); test("gui in the title without a screenshot fails and drafts", async () => { @@ -1145,6 +1601,7 @@ describe("GitHub Actions hardening", () => { "- Ran bun test tests/ci-workflows.test.ts", ].join("\n"), }, + authorPermission: "write", }); expect(methodsOf(result)).toEqual(readsAllowedBase()); @@ -1168,6 +1625,7 @@ describe("GitHub Actions hardening", () => { "- Ran bun test tests/ci-workflows.test.ts", ].join("\n"), }, + authorPermission: "write", }); expect(methodsOf(result)).toEqual(readsAllowedBase()); @@ -1200,6 +1658,7 @@ describe("GitHub Actions hardening", () => { test("guidance in the title does not demand a screenshot", async () => { const result = await run({ pr: { base: { ref: "dev" }, title: "Add contributor guidance docs" }, + authorPermission: "write", }); expect(methodsOf(result)).toEqual(readsAllowedBase()); @@ -1220,7 +1679,12 @@ describe("GitHub Actions hardening", () => { test("clears prior bot state when every gate passes again", async () => { const result = await run({ - pr: { base: { ref: "dev" }, draft: true }, + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, comments: [botComment({ version: 1, active: true, @@ -1234,11 +1698,21 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "graphql", "issues.updateComment", + "issues.createComment", ])); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(false); const [cleared] = callsTo(result, "issues.updateComment") as [{ body: string }]; expect(cleared.body).toContain('"active":false'); expect(cleared.body).toContain("PR quality gates passed"); + + // Checklist completion also lifts the draft and pings the maintainers. + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("**4/4** boxes ticked"); + expect(readinessBody).toContain("Maintainers notified: @lidge-jun @Ingwannu @Wibias"); + expect(readinessBody).toContain('"maintainersPinged":true'); + // The ping list is read through the recorded fs stub, and the change-log + // duplicate of @Wibias is not re-added. + expect(result.fsReads.some(read => read.endsWith("MAINTAINERS.md"))).toBe(true); }); test("every base outside the allow-list is still blocked", async () => { @@ -1251,12 +1725,15 @@ describe("GitHub Actions hardening", () => { const result = await run({ pr: { base: { ref }, title: "Add a thing", draft: false } }); expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.update", + "issues.createComment", "issues.createComment", "pulls.update", "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", + "issues.updateComment", ])); expect(lastEnforcerCommentBody(result)).toContain(`\`${ref}\``); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); @@ -1269,7 +1746,12 @@ describe("GitHub Actions hardening", () => { // does not fire, they are left with a permanently renamed, drafted PR // and no state to explain it. const result = await run({ - pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Port the runtime entry" }, + pr: { + base: { ref: "dev" }, + draft: true, + title: "[WRONG BRANCH] Port the runtime entry", + body: readinessChecklistBody(4), + }, comments: [botComment({ version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true })], }); @@ -1277,6 +1759,7 @@ describe("GitHub Actions hardening", () => { "pulls.update", "graphql", "issues.updateComment", + "issues.createComment", ])); expect(callsTo(result, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Port the runtime entry" }, @@ -1288,6 +1771,31 @@ describe("GitHub Actions hardening", () => { expect(cleared.body).toContain("now targets `dev`"); }); + test("a PR retargeted to dev with an open checklist stays a draft", async () => { + // Retargeting clears the branch failure, but the readiness checklist is + // still open, so the PR must NOT be marked ready. The enforcer message is + // updated to say the branch is now correct and the checklist is pending. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + title: "[WRONG BRANCH] Port the runtime entry", + }, + comments: [botComment({ version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true })], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "pulls.update", + "pulls.update", + "issues.updateComment", + "issues.createComment", + ])); + expect(callsTo(result, "graphql")).toEqual([]); + expect(lastEnforcerCommentBody(result)).toContain("now targets `dev`"); + expect(lastEnforcerCommentBody(result)).toContain("review readiness checklist is complete"); + expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(false); + }); + test("a PR moved from dev back to main is enforced again from a cleared state", async () => { // The other half of the round trip. After a restoration the marker is // inactive, so a move back out has to build fresh state rather than @@ -1298,14 +1806,23 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.update", + "issues.createComment", "issues.updateComment", "pulls.update", "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", + "issues.updateComment", ])); expect(callsTo(result, "pulls.update")).toEqual([ + { + owner: "lidge-jun", + repo: "opencodex", + pull_number: 42, + body: expect.stringContaining(CHECKLIST_START), + }, { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "[WRONG BRANCH] Port the runtime entry" }, ]); expect(lastEnforcerCommentBody(result)).toContain('"active":true'); @@ -1336,28 +1853,39 @@ describe("GitHub Actions hardening", () => { // checkpoint before convertToDraft so a successful convert followed by a // failed comment still restores later. expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.update", + "issues.createComment", "issues.createComment", "pulls.update", "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", + "issues.updateComment", ])); // The title update carries the title and nothing else. `base`, `state` // and `body` are all accepted by this endpoint; an audit round added // `base: "main"` here and no static assertion caught it. expect(callsTo(result, "pulls.update")).toEqual([ + { + owner: "lidge-jun", + repo: "opencodex", + pull_number: 42, + body: expect.stringContaining(CHECKLIST_START), + }, { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "[WRONG BRANCH] Add a thing" }, ]); // The first comment create addresses this PR, by its own number. - const [created] = callsTo(result, "issues.createComment") as [{ issue_number: number; body: string }]; + const createdComments = callsTo(result, "issues.createComment") as [{ issue_number: number; body: string }]; + const created = createdComments.find(call => call.body.includes(MARKER))!; expect(created.issue_number).toBe(42); expect(created.body).toContain(MARKER); const commentBody = lastEnforcerCommentBody(result); expect(commentBody).toContain("@contributor"); expect(commentBody).toContain('"autoDraftedByBot":true'); + expect(commentBody).toContain("Review readiness checklist"); // The only GraphQL mutation is the draft conversion — not a retarget. const [draft] = callsTo(result, "graphql") as [{ query: string; variables: unknown }]; @@ -1381,6 +1909,7 @@ describe("GitHub Actions hardening", () => { title: "Stacked child", draft: false, }, + authorPermission: "write", openPulls: [ { number: 41, @@ -1431,6 +1960,7 @@ describe("GitHub Actions hardening", () => { }, ], ], + authorPermission: "write", }); const listPages = callsTo(result, "pulls.list").map( @@ -1458,14 +1988,23 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.update", + "issues.createComment", "issues.createComment", "pulls.update", "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", + "issues.updateComment", ])); expect(callsTo(result, "pulls.update")).toEqual([ + { + owner: "lidge-jun", + repo: "opencodex", + pull_number: 42, + body: expect.stringContaining(CHECKLIST_START), + }, { owner: "lidge-jun", repo: "opencodex", @@ -1485,9 +2024,11 @@ describe("GitHub Actions hardening", () => { // then title prefix, then final explanation. State records that the bot // did not draft — which stops restore from marking it ready. expect(methodsOf(wrong)).toEqual(readsWrongBase([ + "pulls.update", "issues.createComment", "pulls.update", "issues.updateComment", + "issues.createComment", ])); expect(lastEnforcerCommentBody(wrong)).toContain('"autoDraftedByBot":false'); expect(wrong.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); @@ -1498,19 +2039,33 @@ describe("GitHub Actions hardening", () => { comments: [botComment({ version: 1, active: true, autoDraftedByBot: false, titlePrefixedByBot: true })], }); - // The prefix comes off; the draft stays. No GraphQL at all. + // The prefix comes off; the draft stays — the checklist is still open and + // the bot never drafted this PR. No GraphQL at all. expect(methodsOf(restored)).toEqual(readsAllowedBase([ + "pulls.update", "pulls.update", "issues.updateComment", + "issues.createComment", ])); expect(callsTo(restored, "pulls.update")).toEqual([ + { + owner: "lidge-jun", + repo: "opencodex", + pull_number: 42, + body: expect.stringContaining(CHECKLIST_START), + }, { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing" }, ]); }); test("a corrected PR gets its title and ready state back", async () => { const result = await run({ - pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, + pr: { + base: { ref: "dev" }, + draft: true, + title: "[WRONG BRANCH] Add a thing", + body: readinessChecklistBody(4), + }, comments: [botComment({ version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true })], }); @@ -1518,6 +2073,7 @@ describe("GitHub Actions hardening", () => { "pulls.update", "graphql", "issues.updateComment", + "issues.createComment", ])); expect(callsTo(result, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing" }, @@ -1531,6 +2087,7 @@ describe("GitHub Actions hardening", () => { expect(update.comment_id).toBe(7); expect(update.body).toContain('"active":false'); expect(update.body).toContain("PR quality gates passed"); + expect(update.body).toContain("review readiness checklist is complete"); }); test("only this workflow's own prefix is removed, not a contributor's edits", async () => { @@ -1540,6 +2097,12 @@ describe("GitHub Actions hardening", () => { }); expect(callsTo(result, "pulls.update")).toEqual([ + { + owner: "lidge-jun", + repo: "opencodex", + pull_number: 42, + body: expect.stringContaining(CHECKLIST_START), + }, { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing (v2)" }, ]); }); @@ -1552,8 +2115,10 @@ describe("GitHub Actions hardening", () => { // The comment is refreshed (pending + final); title and draft are already right. expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.update", "issues.updateComment", "issues.updateComment", + "issues.createComment", ])); }); @@ -1571,14 +2136,23 @@ describe("GitHub Actions hardening", () => { // `github.request("POST /repos/attacker/other/issues", …)` off precisely // this path because it was the one scenario asserting loosely. expect(methodsOf(wentWrong)).toEqual(readsWrongBase([ + "pulls.update", + "issues.createComment", "issues.createComment", "pulls.update", "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", + "issues.updateComment", ])); expect(callsTo(wentWrong, "pulls.update")).toEqual([ + { + owner: "lidge-jun", + repo: "opencodex", + pull_number: 42, + body: expect.stringContaining(CHECKLIST_START), + }, { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "[WRONG BRANCH] Add a thing" }, ]); @@ -1586,6 +2160,7 @@ describe("GitHub Actions hardening", () => { const wasFixed = await run({ pr: { base: { ref: "dev" } }, eventPayload: { base: { ref: "main" } }, + authorPermission: "write", }); expect(methodsOf(wasFixed)).toEqual(readsAllowedBase()); }); @@ -1629,7 +2204,12 @@ describe("GitHub Actions hardening", () => { })); const result = await run({ - pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, + pr: { + base: { ref: "dev" }, + draft: true, + title: "[WRONG BRANCH] Add a thing", + body: readinessChecklistBody(4), + }, commentPages: [ filler, [botComment({ version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true })], @@ -1637,13 +2217,18 @@ describe("GitHub Actions hardening", () => { }); // Found it: the prefix comes off, the PR is marked ready, and the - // existing comment is edited rather than duplicated. + // existing enforcer comment is edited rather than duplicated. The one + // created comment is the readiness checklist message, which did not + // exist on the busy PR yet. expect(methodsOf(result)).toEqual(readsAllowedBasePaged([ "pulls.update", "graphql", "issues.updateComment", + "issues.createComment", ])); - expect(callsTo(result, "issues.createComment")).toEqual([]); + const [created] = callsTo(result, "issues.createComment") as [{ body: string }]; + expect(created.body).toContain(READINESS_MARKER); + expect(created.body).not.toContain(MARKER); }); test("a bot comment with unreadable state is treated as no state, not as a reason to stop", async () => { @@ -1663,12 +2248,15 @@ describe("GitHub Actions hardening", () => { // Enforcement still happens, and the unreadable comment is repaired in // place rather than duplicated. expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.update", + "issues.createComment", "issues.updateComment", "pulls.update", "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", + "issues.updateComment", ])); expect(result.warnings.join(" ")).toContain("Could not parse stored workflow state"); }); @@ -1692,17 +2280,24 @@ describe("GitHub Actions hardening", () => { comments: [botComment(noRecordedChanges)], }); expect(methodsOf(stillWrong)).toEqual(readsWrongBase([ + "pulls.update", "issues.updateComment", "issues.updateComment", + "issues.createComment", ])); - // Corrected: nothing to undo, but the state must still be cleared or the - // next wrong-target event resumes from a stale record. + // Corrected branch, open checklist: nothing to undo, the enforcer state + // is cleared (the checklist message now owns the draft), and the next + // wrong-target event cannot resume from a stale record. const corrected = await run({ pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, comments: [botComment(noRecordedChanges)], }); - expect(methodsOf(corrected)).toEqual(readsAllowedBase(["issues.updateComment"])); + expect(methodsOf(corrected)).toEqual(readsAllowedBase([ + "pulls.update", + "issues.updateComment", + "issues.createComment", + ])); const [cleared] = callsTo(corrected, "issues.updateComment") as [{ body: string }]; expect(cleared.body).toContain('"active":false'); expect(cleared.body).toContain("PR quality gates passed"); @@ -1719,13 +2314,19 @@ describe("GitHub Actions hardening", () => { comments: [botComment({ version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true })], }); - // Nothing to un-draft, the prefix comes off, and the state is cleared. + // The prefix comes off, the state is cleared, and the open checklist + // re-drafts the PR the author undrafted by hand. expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", + "pulls.update", + "issues.createComment", + "issues.updateComment", + "graphql", "issues.updateComment", ])); const [cleared] = callsTo(result, "issues.updateComment") as [{ body: string }]; expect(cleared.body).toContain('"active":false'); + expect(cleared.body).toContain("review readiness checklist is complete"); }); test("a title the author already fixed by hand is not sliced a second time", async () => { @@ -1733,7 +2334,12 @@ describe("GitHub Actions hardening", () => { // the prefix — the author removed it themselves. Slicing anyway would eat // the first 15 characters of their title. const result = await run({ - pr: { base: { ref: "dev" }, draft: true, title: "Add a thing" }, + pr: { + base: { ref: "dev" }, + draft: true, + title: "Add a thing", + body: readinessChecklistBody(4), + }, comments: [botComment({ version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true })], }); @@ -1741,6 +2347,7 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "graphql", "issues.updateComment", + "issues.createComment", ])); }); @@ -1787,14 +2394,21 @@ describe("GitHub Actions hardening", () => { // nobody honours — the prefix stays on forever. Round ten bumped it to 2 // and every test passed, because nothing asserted the value. const wrong = await run({ pr: { base: { ref: "main" }, draft: false } }); - const [posted] = callsTo(wrong, "issues.createComment") as [{ body: string }]; + const postedComments = callsTo(wrong, "issues.createComment") as [{ body: string }]; + const posted = postedComments.find(call => call.body.includes(MARKER))!; expect(posted.body).toContain('"version":1'); const cleared = await run({ - pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, + pr: { + base: { ref: "dev" }, + draft: true, + title: "[WRONG BRANCH] Add a thing", + body: readinessChecklistBody(4), + }, comments: [botComment({ version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true })], }); - const [done] = callsTo(cleared, "issues.updateComment") as [{ body: string }]; + const done = (callsTo(cleared, "issues.updateComment") as [{ body: string }]) + .find(call => call.body.includes(MARKER))!; expect(done.body).toContain('"version":1'); }); @@ -1815,13 +2429,19 @@ describe("GitHub Actions hardening", () => { // changes are undone, and the marker is rewritten at the version this // workflow writes. const restored = await run({ - pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, + pr: { + base: { ref: "dev" }, + draft: true, + title: "[WRONG BRANCH] Add a thing", + body: readinessChecklistBody(4), + }, comments: [botComment(active)], }); expect(methodsOf(restored)).toEqual(readsAllowedBase([ "pulls.update", "graphql", "issues.updateComment", + "issues.createComment", ])); expect(callsTo(restored, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing" }, @@ -1838,12 +2458,15 @@ describe("GitHub Actions hardening", () => { comments: [botComment(active)], }); expect(methodsOf(wrong)).toEqual(readsWrongBase([ + "pulls.update", + "issues.createComment", "issues.updateComment", "pulls.update", "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", + "issues.updateComment", ])); expect(lastEnforcerCommentBody(wrong)).toContain(`"version":${version}`); expect(lastEnforcerCommentBody(wrong)).toContain('"active":true'); @@ -1865,13 +2488,19 @@ describe("GitHub Actions hardening", () => { // contributor-reachable. It is reachable across a migration, which is // exactly when the prefix must still come off. const loose = await run({ - pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, + pr: { + base: { ref: "dev" }, + draft: true, + title: "[WRONG BRANCH] Add a thing", + body: readinessChecklistBody(4), + }, comments: [botComment({ version: 1, active: "true", autoDraftedByBot: 1, titlePrefixedByBot: "yes" })], }); expect(methodsOf(loose)).toEqual(readsAllowedBase([ "pulls.update", "graphql", "issues.updateComment", + "issues.createComment", ])); expect(callsTo(loose, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing" }, @@ -1880,10 +2509,19 @@ describe("GitHub Actions hardening", () => { // And the falsy side is symmetric: `null` and `0` skip their own // restoration without stopping the run or the clearing write. const falsy = await run({ - pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, + pr: { + base: { ref: "dev" }, + draft: true, + title: "[WRONG BRANCH] Add a thing", + body: readinessChecklistBody(4), + }, comments: [botComment({ version: 1, active: true, autoDraftedByBot: null, titlePrefixedByBot: 0 })], }); - expect(methodsOf(falsy)).toEqual(readsAllowedBase(["issues.updateComment"])); + expect(methodsOf(falsy)).toEqual(readsAllowedBase([ + "graphql", + "issues.updateComment", + "issues.createComment", + ])); const [cleared] = callsTo(falsy, "issues.updateComment") as [{ body: string }]; expect(cleared.body).toContain('"active":false'); }); @@ -1893,18 +2531,30 @@ describe("GitHub Actions hardening", () => { // checkpointed before convertToDraft so a successful convert followed by a // failed comment still restores later. const result = await run({ pr: { base: { ref: "main" }, draft: false } }); + // The exact call order pins the checkpoint discipline: checklist message, + // enforcer ownership claim, title, draft claim, conversion, final. + expect(methodsOf(result)).toEqual(readsWrongBase(CONTRIBUTOR_WRONG_BASE_TAIL)); + + const creates = callsTo(result, "issues.createComment") as Array<{ body: string }>; + expect( + creates.some(call => + call.body.includes(MARKER) && call.body.includes("Recording ownership state"), + ), + ).toBe(true); + + const updates = callsTo(result, "issues.updateComment") as Array<{ body: string }>; + const draftClaim = updates.find(call => call.body.includes("Draft conversion pending")); + const finalUpdate = updates.filter(call => call.body.includes(MARKER)).at(-1); + expect(draftClaim).toBeDefined(); + expect(finalUpdate).toBeDefined(); + expect(finalUpdate!.body).toContain('"autoDraftedByBot":true'); const methods = methodsOf(result); - const pending = methods.indexOf("issues.createComment"); - const title = methods.indexOf("pulls.update"); - const draftClaim = methods.indexOf("issues.updateComment"); - const draft = methods.indexOf("graphql"); - const finalUpdate = methods.lastIndexOf("issues.updateComment"); - expect(pending).toBeGreaterThan(-1); - expect(pending).toBeLessThan(title); - expect(title).toBeLessThan(draftClaim); - expect(draftClaim).toBeLessThan(draft); - expect(draft).toBeLessThan(finalUpdate); - expect(lastEnforcerCommentBody(result)).toContain('"autoDraftedByBot":true'); + const draftIndex = methods.indexOf("graphql"); + const updateMethodIndices = methods + .map((method, index) => (method === "issues.updateComment" ? index : -1)) + .filter(index => index >= 0); + expect(updateMethodIndices[updates.indexOf(draftClaim!)]!).toBeLessThan(draftIndex); + expect(updateMethodIndices[updates.indexOf(finalUpdate!)]!).toBeGreaterThan(draftIndex); }); test("a title that is exactly the prefix is still enforced", async () => { @@ -1918,13 +2568,23 @@ describe("GitHub Actions hardening", () => { }); // Already prefixed, so no title write — but pending/draft/final still run. - expect(callsTo(result, "pulls.update")).toEqual([]); + expect(callsTo(result, "pulls.update")).toEqual([ + { + owner: "lidge-jun", + repo: "opencodex", + pull_number: 42, + body: expect.stringContaining(CHECKLIST_START), + }, + ]); expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.update", + "issues.createComment", "issues.createComment", "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", + "issues.updateComment", ])); expect(lastEnforcerCommentBody(result)).toContain('"titlePrefixedByBot":false'); expect(lastEnforcerCommentBody(result)).toContain('"autoDraftedByBot":true'); @@ -1939,12 +2599,20 @@ describe("GitHub Actions hardening", () => { }); expect(callsTo(result, "pulls.update")).toEqual([ + { + owner: "lidge-jun", + repo: "opencodex", + pull_number: 42, + body: expect.stringContaining(CHECKLIST_START), + }, { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "[WRONG BRANCH] " }, ]); expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.update", "issues.createComment", "pulls.update", "issues.updateComment", + "issues.createComment", ])); }); @@ -1962,10 +2630,19 @@ describe("GitHub Actions hardening", () => { // reads, finds no prior state, and records that it changed nothing. // Asserting only the absent write would let an early return keyed on the // doubled prefix pass, since that skips the write too. - expect(callsTo(result, "pulls.update")).toEqual([]); + expect(callsTo(result, "pulls.update")).toEqual([ + { + owner: "lidge-jun", + repo: "opencodex", + pull_number: 42, + body: expect.stringContaining(CHECKLIST_START), + }, + ]); expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.update", "issues.createComment", "issues.updateComment", + "issues.createComment", ])); expect(lastEnforcerCommentBody(result)).toContain('"titlePrefixedByBot":false'); expect(lastEnforcerCommentBody(result)).toContain('"active":true'); @@ -1983,14 +2660,24 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.update", + "issues.createComment", "issues.createComment", "issues.updateComment", "graphql", "issues.updateComment", "issues.updateComment", + "issues.updateComment", ])); // Already prefixed by the `startsWith` test, so no third prefix is added. - expect(callsTo(result, "pulls.update")).toEqual([]); + expect(callsTo(result, "pulls.update")).toEqual([ + { + owner: "lidge-jun", + repo: "opencodex", + pull_number: 42, + body: expect.stringContaining(CHECKLIST_START), + }, + ]); expect(lastEnforcerCommentBody(result)).toContain('"active":true'); expect(lastEnforcerCommentBody(result)).toContain('"autoDraftedByBot":true'); }); @@ -2012,7 +2699,12 @@ describe("GitHub Actions hardening", () => { body: [MARKER, ``].join("\n"), }; const result = await run({ - pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, + pr: { + base: { ref: "dev" }, + draft: true, + title: "[WRONG BRANCH] Add a thing", + body: readinessChecklistBody(4), + }, comments: [first, second], }); @@ -2022,6 +2714,7 @@ describe("GitHub Actions hardening", () => { "pulls.update", "graphql", "issues.updateComment", + "issues.createComment", ])); // And the first comment is the one rewritten, not the second. const [updated] = callsTo(result, "issues.updateComment") as [{ comment_id: number }]; @@ -2108,7 +2801,10 @@ describe("GitHub Actions hardening", () => { // The mechanism the three round-eight mutations shared: a truthiness or // `typeof` check that answers one way on the runner and the other way // here. Assert the answers match production for every injected name. - const result = await run({ pr: { base: { ref: "dev" } } }); + const result = await run({ + pr: { base: { ref: "dev" } }, + authorPermission: "write", + }); const probe = await runProbe(` const seen = {}; for (const [name, value] of Object.entries({ @@ -2185,15 +2881,21 @@ describe("GitHub Actions hardening", () => { failStatus: status, }); expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.update", + "issues.createComment", "issues.createComment", "pulls.update", "issues.updateComment", "graphql", "issues.updateComment", + "issues.updateComment", ])); const commentBody = lastEnforcerCommentBody(result); expect(commentBody).toContain('"autoDraftedByBot":false'); expect(commentBody).toContain("Automatic draft conversion failed"); + expect(lastReadinessCommentBody(result)).toContain( + "Automatic draft conversion failed", + ); expect(result.warnings.some((w) => w.includes("Could not convert pull request to draft"))).toBe(true); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); } @@ -2226,7 +2928,12 @@ describe("GitHub Actions hardening", () => { test("a failed ready-for-review conversion keeps ownership active for retry", async () => { const { script } = await readEnforcePrTarget(); const result = await runEnforcePrTarget(script, { - pr: { base: { ref: "dev" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, + pr: { + base: { ref: "dev" }, + draft: true, + title: "[WRONG BRANCH] Add a thing", + body: readinessChecklistBody(4), + }, comments: [ { id: 7, @@ -2261,8 +2968,8 @@ describe("GitHub Actions hardening", () => { // that the `[WRONG BRANCH] ` prefix is never removed. expect(script).toMatch(/state\.autoDraftedByBot\s*=\s*true/); expect(script).toMatch(/state\.titlePrefixedByBot\s*=\s*true/); - expect(script).toMatch(/storedState\.autoDraftedByBot/); - expect(script).toMatch(/storedState\.titlePrefixedByBot/); + expect(script).toMatch(/storedState\?\.autoDraftedByBot/); + expect(script).toMatch(/storedState\?\.titlePrefixedByBot/); expect(script).toMatch(/await\s+convertToDraft\(\)/); expect(script).toMatch(/await\s+markReadyForReview\(\)/); expect(script).toMatch(/core\.setFailed\(/); @@ -2290,9 +2997,22 @@ describe("GitHub Actions hardening", () => { // a draft forever, and every assertion above still passed because both // helpers and both state fields were still textually present. Presence of a // call proves nothing about whether it can be reached. - expect(script).toMatch(/\n\s*if \(!storedState\?\.active\) \{\n/); + expect(script).toMatch(/\n\s*if \(!storedState\?\.active && !checklistRequired\) \{\n/); expect(script).toMatch(/\n\s*if \(failures\.length > 0\) \{\n/); + // The readiness gate: contributor drafts are owned by the checklist in the + // PR body, and the maintainer ping is recorded in the checklist message + // state so it happens once. + expect(script).toMatch(/const mustDraft =/); + expect(script).toMatch(/extractReviewReadiness\(pr\.body\)/); + expect(script).toMatch(/appendReviewReadinessSection\(pr\.body/); + expect(script).toMatch(/checklistComplete = readiness\.present && readiness\.complete/); + expect(script).toMatch(/Maintainers notified:/); + expect(script).toMatch(/maintainersPinged\s*=\s*true/); + expect(script).toMatch(/READINESS_MARKER = ""/); + expect(script).toMatch(/readMaintainerLogins\(\)/); + expect(script).toMatch(/fs\.readFileSync/); + // Pending ownership is written before mutations; convertToDraft runs next; // a later upsertComment records autoDraftedByBot only after success (#631). const branchStart = script.indexOf("if (failures.length > 0) {"); diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index 99197e7ec1..45f3f07808 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -22,6 +22,13 @@ export type RecordedCall = { method: string; args: unknown }; export type HarnessResult = { calls: RecordedCall[]; + /** + * Paths the script read through its `node:fs` stub. Kept separate from + * `calls` so exact method-sequence assertions stay stable while the fs + * capability stays recorded (round: the harness must not hand a write-capable + * process module to a script that holds a write token). + */ + fsReads: string[]; logs: string[]; warnings: string[]; /** @@ -85,6 +92,11 @@ export type RunOptions = { failStatus?: number; /** Collaborator permission returned by `getCollaboratorPermissionLevel`. */ authorPermission?: string; + /** + * Fixture content for `MAINTAINERS.md`, so readiness-ping scenarios do not + * depend on the live repository file. Defaults to reading the real file. + */ + maintainersFile?: string; /** When true, permission lookup rejects like a transient API failure. */ failPermissionLookup?: boolean; /** Overrides for `compareCommitsWithBasehead` keyed by `basehead`. */ @@ -411,6 +423,7 @@ export async function runEnforcePrTarget( options: RunOptions, ): Promise { const calls: RecordedCall[] = []; + const fsReads: string[] = []; const logs: string[] = []; const warnings: string[] = []; const outputs: { name: string; value: unknown }[] = []; @@ -526,7 +539,7 @@ export async function runEnforcePrTarget( const nodeRequire = createRequire(path.join(process.cwd(), "package.json")); const scriptsRoot = path.resolve(process.cwd(), ".github", "scripts"); /** Bare modules the workflow script may load (see enforce-pr-target.yml). */ - const ALLOWED_MODULES = new Set(["path", "node:path"]); + const ALLOWED_MODULES = new Set(["path", "node:path", "node:fs"]); function scopedRequire(id: string) { calls.push({ method: "require", args: [id] }); @@ -535,6 +548,26 @@ export async function runEnforcePrTarget( if (!ALLOWED_MODULES.has(id)) { throw new Error(`the script must not require ${id}`); } + if (id === "node:fs") { + // The script may read exactly one file: the trusted default-branch + // MAINTAINERS.md. Everything else about `fs` (writes, directory + // listing, arbitrary reads) is a capability the harness must not hand + // over, and the read itself has to be recorded like every other call. + const nodeFs = nodeRequire("node:fs"); + return { + readFileSync: (pathLike: unknown) => { + const resolved = path.resolve(String(pathLike)); + fsReads.push(resolved); + if (resolved !== path.resolve(process.cwd(), "MAINTAINERS.md")) { + throw new Error(`the script must not read ${pathLike}`); + } + return ( + options.maintainersFile ?? + nodeFs.readFileSync(resolved, "utf8") + ); + }, + }; + } return nodeRequire(id); } const resolved = path.isAbsolute(id) ? path.resolve(id) : path.resolve(process.cwd(), id); @@ -864,7 +897,14 @@ export async function runEnforcePrTarget( await callback(); } - return { calls, logs, warnings, returnValue, coreSurface: Object.keys(core).sort() }; + return { + calls, + fsReads, + logs, + warnings, + returnValue, + coreSurface: Object.keys(core).sort(), + }; } /** Just the method names, in order — the usual thing to assert on. */