From 08923a5fbadc50490c518ae28e08e805577b17fe Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:15:55 +0200 Subject: [PATCH 1/8] feat(ci): draft contributor PRs until review-readiness checklist is complete --- .github/scripts/pr-quality.cjs | 85 +++ .github/scripts/pr-quality.test.cjs | 101 ++++ .github/workflows/enforce-pr-target.yml | 469 ++++++++++++--- AGENTS.md | 5 + MAINTAINERS.md | 6 + .../content/docs/contributing/pr-quality.md | 8 + tests/ci-workflows.test.ts | 564 ++++++++++++++++-- tests/helpers/enforce-pr-target-harness.ts | 2 +- 8 files changed, 1107 insertions(+), 133 deletions(-) diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index 4131d303b9..366767fd16 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. @@ -191,6 +207,69 @@ function hasScreenshotEvidence(body) { return hasRenderableReferenceImage(visible); } +/** + * The tickable checklist section injected into the PR description. It lives in + * the body (the author can tick it) and is bounded by HTML markers so the gate + * can find exactly this section and ignore any other task list in the body. + */ +function buildReviewReadinessSection() { + const items = REVIEW_READINESS_ITEMS.flatMap((item, index) => + index === REVIEW_READINESS_ITEMS.length - 1 + ? ["", `- [ ] ${item}`] + : [`- [ ] ${item}`], + ); + return [ + REVIEW_READINESS_START, + "## Review readiness checklist", + "", + "This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:", + "", + ...items, + REVIEW_READINESS_END, + ].join("\n"); +} + +/** + * Read the checklist section the bot manages. `present` means the marker pair + * exists; `complete` means the section contains exactly the four boxes and all + * of them are checked. Anything else (missing markers, fewer or extra boxes, + * unchecked boxes) keeps the gate closed. The author can reword an item, but + * the box count and the checked state are the contract. + */ +function extractReviewReadiness(body) { + if (typeof body !== "string") { + return { present: false, complete: false, checked: 0, total: 0 }; + } + const start = body.indexOf(REVIEW_READINESS_START); + const end = body.indexOf(REVIEW_READINESS_END); + if (start === -1 || end === -1 || end <= start) { + return { present: false, complete: false, checked: 0, total: 0 }; + } + const section = body.slice(start + REVIEW_READINESS_START.length, end); + const boxes = [...section.matchAll(/^\s*[-*]\s+\[([ xX])\]\s+/gm)]; + const checked = boxes.filter((match) => match[1] !== " ").length; + const total = boxes.length; + return { + present: true, + complete: + total === REVIEW_READINESS_ITEMS.length && checked === total, + checked, + total, + }; +} + +/** + * Append the checklist section to a PR body. Idempotent: a body that already + * carries the marker pair is returned unchanged, so a re-run can never stack a + * second checklist (or feed the `edited` event endless body churn). + */ +function appendReviewReadinessSection(body) { + if (extractReviewReadiness(body).present) return body; + const section = buildReviewReadinessSection(); + if (typeof body !== "string" || !body.trim()) return `${section}\n`; + return `${body.trimEnd()}\n\n${section}\n`; +} + function collectPrQualityFailures({ baseRef, allowedBases, @@ -248,11 +327,17 @@ function collectPrQualityFailures({ module.exports = { ANCESTRY_BEHIND_THRESHOLD, ANCESTRY_AHEAD_MAIN_MAX, + REVIEW_READINESS_ITEMS, + REVIEW_READINESS_START, + REVIEW_READINESS_END, isWrongAncestry, authorHasPushPermission, assessPrDescription, hasGuiCue, hasScreenshotEvidence, + buildReviewReadinessSection, + extractReviewReadiness, + appendReviewReadinessSection, collectPrQualityFailures, hasEscapedNewlines, stripPrTemplateBoilerplate, diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index 53586e1e62..2eea6b1e20 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -4,11 +4,15 @@ const { describe, it } = require("node:test"); const assert = require("node:assert/strict"); const { ANCESTRY_BEHIND_THRESHOLD, + REVIEW_READINESS_ITEMS, isWrongAncestry, authorHasPushPermission, assessPrDescription, hasGuiCue, hasScreenshotEvidence, + buildReviewReadinessSection, + extractReviewReadiness, + appendReviewReadinessSection, collectPrQualityFailures, } = require("./pr-quality.cjs"); @@ -215,6 +219,103 @@ describe("hasScreenshotEvidence", () => { }); }); +describe("review readiness checklist", () => { + const SECTION = buildReviewReadinessSection(); + + it("builds exactly the four required boxes inside the markers", () => { + assert.ok(SECTION.includes("")); + 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, + }); + assert.deepEqual(extractReviewReadiness(null), { + present: false, + complete: false, + checked: 0, + total: 0, + }); + }); + + it("counts checked boxes and requires all four for completion", () => { + const body = [ + "## Summary", + "Change.", + SECTION.replaceAll("- [ ] ", "- [x] "), + ].join("\n\n"); + assert.deepEqual(extractReviewReadiness(body), { + present: true, + complete: true, + checked: 4, + total: 4, + }); + + const partial = body.replace("- [x] My PR is ready for review.", "- [ ] My PR is ready for review."); + assert.deepEqual(extractReviewReadiness(partial), { + present: true, + complete: false, + checked: 3, + total: 4, + }); + }); + + it("treats a reworded but complete section as complete", () => { + const reworded = SECTION + .replace("All CI tests are green on my local testing.", "Local suite green.") + .replaceAll("- [ ] ", "- [x] "); + const result = extractReviewReadiness(reworded); + assert.equal(result.present, true); + assert.equal(result.complete, true); + assert.equal(result.checked, 4); + }); + + it("stays incomplete for fewer or extra boxes inside the markers", () => { + const fewer = SECTION.replace("- [ ] My PR is ready for review.", ""); + assert.equal(extractReviewReadiness(fewer).complete, false); + assert.equal(extractReviewReadiness(fewer).total, 3); + + const extra = SECTION.replace( + "", + "- [x] An extra box.\n", + ); + const result = extractReviewReadiness(extra); + assert.equal(result.complete, false); + assert.equal(result.total, 5); + }); + + it("appends once and is idempotent", () => { + const first = appendReviewReadinessSection("## Summary\n\nBody."); + assert.equal(extractReviewReadiness(first).present, true); + assert.equal(extractReviewReadiness(first).total, 4); + const second = appendReviewReadinessSection(first); + assert.equal(second, first); + assert.equal((second.match(/pr-quality-readiness-checklist:start/g) || []).length, 1); + }); + + it("appends cleanly to an empty body", () => { + const body = appendReviewReadinessSection(""); + assert.equal(extractReviewReadiness(body).present, true); + assert.ok(body.startsWith("")); + }); +}); + describe("collectPrQualityFailures", () => { const allowed = ["dev"]; diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index e7c475606a..7ee640d959 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -31,14 +31,23 @@ jobs: with: ref: ${{ github.event.repository.default_branch }} 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, + REVIEW_READINESS_ITEMS + } = require( path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), ); @@ -49,6 +58,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 +91,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 +127,105 @@ 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 ( + "" + ); + } + + /** + * 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" + ); + const logins = [ + ...text.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) => + `- ${index < readiness.checked ? "✅" : "⬜"} ${item}` + ); + } + + async function upsertReadinessComment(state, readiness, extra) { + const complete = readiness.present && readiness.complete; + const lines = [ + READINESS_MARKER, + readinessStateMarker(state), + "", + "## Review readiness checklist", + "", + "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.", + "", + ...readinessChecklistLines(readiness), + "", + 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 +495,45 @@ 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; + + // 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. + let readiness = extractReviewReadiness(pr.body); + 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 } + : { + version: READINESS_STATE_VERSION, + autoDraftedByBot: false, + maintainersPinged: false + }; const state = storedState?.active ? { ...storedState } : { @@ -389,17 +545,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,9 +561,57 @@ jobs: state.titlePrefixedByBot = true; } + // 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, + pull_number, + title: pr.title.slice(TITLE_PREFIX.length) + }); + state.titlePrefixedByBot = false; + } + + if (checklistRequired) { + const conversionPending = + !pr.draft && !checklistComplete; + if (conversionPending) { + await upsertReadinessComment( + readinessState, + readiness, + [ + "This PR stays in draft until every box above is ticked." + ] + ); + } + } + + 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, @@ -432,24 +630,6 @@ jobs: pull_number, title: `${TITLE_PREFIX}${pr.title}` }); - } else if (shouldStripTitlePrefix) { - await github.rest.pulls.update({ - owner, - repo, - pull_number, - title: pr.title.slice(TITLE_PREFIX.length) - }); - state.titlePrefixedByBot = false; - await upsertComment( - [ - COMMENT_MARKER, - stateMarker(state), - "", - ...failureSections, - "", - "Stale title prefix removed; continuing…" - ].join("\n") - ); } if (!pr.draft) { @@ -468,6 +648,8 @@ jobs: ); try { await convertToDraft(); + draftConverted = true; + readinessState.autoDraftedByBot = true; await upsertComment( [ COMMENT_MARKER, @@ -513,11 +695,78 @@ jobs: ].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; + } + + // 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({ + version: 1, + active: false, + autoDraftedByBot: false, + titlePrefixedByBot: false, + ancestryFailed: false, + descriptionFailed: false, + screenshotFailed: false + }), + "", + "✅ **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") + ); + } + + if (!pr.draft && !draftConverted) { + try { + await convertToDraft(); + draftConverted = true; + readinessState.autoDraftedByBot = true; + } catch (error) { + core.warning( + `Could not convert pull request to draft: ${error.message}` + ); + } + } + + await upsertReadinessComment( + readinessState, + readiness, + [ + 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." + ] + ); + return; } - if (!storedState?.active) { + if (!storedState?.active && !checklistRequired) { core.info( "All PR quality gates passed and there is no active bot state." ); @@ -526,7 +775,7 @@ jobs: } if ( - storedState.titlePrefixedByBot && + storedState?.titlePrefixedByBot && pr.title.startsWith(TITLE_PREFIX) ) { await github.rest.pulls.update({ @@ -538,12 +787,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 +803,95 @@ 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 + ? { + 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.${ + 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 } + : { + version: READINESS_STATE_VERSION, + autoDraftedByBot: false, + maintainersPinged: false + }; + let notified = false; + if (!readinessState.maintainersPinged) { + readinessState.maintainersPinged = true; + notified = true; + } + const maintainers = readMaintainerLogins().filter( + login => login !== pr.user.login + ); + + 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..5bb20b8b72 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -178,6 +178,11 @@ 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. 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..b90709790b 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. 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..6e212253c9 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -40,6 +40,14 @@ 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. 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..7351778dc8 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -10,10 +10,29 @@ 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), + ); + return (enforcerCreates.length > 0 ? enforcerCreates : creates)[ + enforcerCreates.length > 0 ? enforcerCreates.length - 1 : creates.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)); + return readinessCreates[readinessCreates.length - 1]!.body; } const root = new URL("../", import.meta.url); @@ -639,7 +658,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[] { @@ -818,7 +837,9 @@ describe("GitHub Actions hardening", () => { expect(checkout.with).toEqual({ ref: "${{ github.event.repository.default_branch }}", "persist-credentials": false, - "sparse-checkout": ".github/scripts", + // MAINTAINERS.md rides along so the completion ping reads the canonical + // maintainer list from the trusted default branch. + "sparse-checkout": ".github/scripts\nMAINTAINERS.md\n", }); expect(Object.keys(scriptStep).sort()).toEqual(["name", "uses", "with"]); @@ -946,21 +967,26 @@ 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. + // Four `pulls.update` sites: the checklist injection (body only), 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"], ["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 +1055,79 @@ 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"); + + /** 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 +1142,111 @@ 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("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), + }, + }); + + // 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("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 +1260,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 +1297,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 +1353,7 @@ describe("GitHub Actions hardening", () => { "- Ran bun test tests/ci-workflows.test.ts", ].join("\n"), }, + authorPermission: "write", }); expect(methodsOf(result)).toEqual(readsAllowedBase()); @@ -1168,6 +1377,7 @@ describe("GitHub Actions hardening", () => { "- Ran bun test tests/ci-workflows.test.ts", ].join("\n"), }, + authorPermission: "write", }); expect(methodsOf(result)).toEqual(readsAllowedBase()); @@ -1200,6 +1410,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 +1431,11 @@ 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), + }, comments: [botComment({ version: 1, active: true, @@ -1234,11 +1449,18 @@ 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'); }); test("every base outside the allow-list is still blocked", async () => { @@ -1251,12 +1473,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 +1494,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 +1507,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 +1519,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 +1554,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 +1601,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 +1657,7 @@ describe("GitHub Actions hardening", () => { title: "Stacked child", draft: false, }, + authorPermission: "write", openPulls: [ { number: 41, @@ -1431,6 +1708,7 @@ describe("GitHub Actions hardening", () => { }, ], ], + authorPermission: "write", }); const listPages = callsTo(result, "pulls.list").map( @@ -1458,14 +1736,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 +1772,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 +1787,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 +1821,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 +1835,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 +1845,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 +1863,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 +1884,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 +1908,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 +1952,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 +1965,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 +1996,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 +2028,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 +2062,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 +2082,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 +2095,7 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "graphql", "issues.updateComment", + "issues.createComment", ])); }); @@ -1787,14 +2142,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 +2177,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 +2206,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 +2236,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 +2257,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 +2279,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 +2316,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 +2347,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 +2378,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 +2408,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 +2447,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 +2462,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 +2549,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 +2629,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 +2676,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, @@ -2290,9 +2745,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..292d0f1525 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -526,7 +526,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] }); From bc66a878b0a8e5d4c9016ab88312178323b5635d Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:19:32 +0200 Subject: [PATCH 2/8] fix(ci): fail closed when checklist draft conversion fails; sync workflow assertions --- .github/scripts/enforce-pr-target.test.cjs | 7 +++--- .github/workflows/enforce-pr-target.yml | 3 +++ tests/ci-workflows.test.ts | 28 ++++++++++++++++++++++ 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index c09f633265..34f0d1c839 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -51,7 +51,8 @@ describe("enforce-pr-target workflow", () => { it("checks out trusted default-branch scripts only (never PR head)", () => { assert.match(workflow, /actions\/checkout@[0-9a-f]{40}/); assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.repository\.default_branch\s*\}\}/); - assert.match(workflow, /sparse-checkout:\s*\.github\/scripts/); + // The readiness ping reads MAINTAINERS.md from the same trusted checkout. + assert.match(workflow, /sparse-checkout:\s*\|\s*\n\s*\.github\/scripts\n\s*MAINTAINERS\.md/); assert.match(workflow, /persist-credentials:\s*false/); assert.doesNotMatch(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.head/); }); @@ -75,9 +76,9 @@ describe("enforce-pr-target workflow", () => { it("strips stale WRONG BRANCH prefix on failure when base is corrected", () => { const failureBlock = workflow.match( - /if \(failures\.length > 0\) \{([\s\S]*?)core\.setFailed\(/, + /if \(mustDraft\) \{([\s\S]*?)core\.setFailed\(/, ); - assert.ok(failureBlock, "workflow must have a failure path"); + assert.ok(failureBlock, "workflow must have a draft path"); const failurePath = failureBlock[1]; assert.match(failurePath, /shouldStripTitlePrefix/); assert.match(failurePath, /!hasWrongBase/); diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 7ee640d959..8d287ae155 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -751,6 +751,9 @@ jobs: 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." + ); } } diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 7351778dc8..a38f45d674 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1176,6 +1176,34 @@ describe("GitHub Actions hardening", () => { 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", + ])); + 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: { From acb93454b6222dfe78bddd232db76defafdeda3c Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:21:15 +0200 Subject: [PATCH 3/8] refactor(ci): flatten checklist condition and dedupe enforcer state defaults --- .github/workflows/enforce-pr-target.yml | 284 +++++++++++------------- 1 file changed, 135 insertions(+), 149 deletions(-) diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 8d287ae155..afdd99e7a3 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -153,6 +153,28 @@ jobs: ); } + /** 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 @@ -529,11 +551,7 @@ jobs: let draftConverted = false; const readinessState = storedReadinessState ? { ...storedReadinessState } - : { - version: READINESS_STATE_VERSION, - autoDraftedByBot: false, - maintainersPinged: false - }; + : defaultReadinessState(); const state = storedState?.active ? { ...storedState } : { @@ -573,69 +591,41 @@ jobs: state.titlePrefixedByBot = false; } - if (checklistRequired) { - const conversionPending = - !pr.draft && !checklistComplete; - if (conversionPending) { - await upsertReadinessComment( - readinessState, - readiness, - [ - "This PR stays in draft until every box above is ticked." - ] - ); - } + if (checklistRequired && !pr.draft && !checklistComplete) { + await upsertReadinessComment( + readinessState, + readiness, + [ + "This PR stays in draft until every box above is ticked." + ] + ); } 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.` + state.ancestryFailed = failures.some( + failure => failure.code === "wrong_ancestry" ); - } - - 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}` - }); - } - - if (!pr.draft) { - // Claim draft ownership before the mutation so a successful - // convert followed by a failed comment still restores later. - state.autoDraftedByBot = true; + 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, @@ -643,13 +633,23 @@ jobs: "", ...failureSections, "", - "Draft conversion pending…" + "Recording ownership state before applying title/draft changes…" ].join("\n") ); - try { - await convertToDraft(); - draftConverted = true; - readinessState.autoDraftedByBot = true; + + 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, @@ -657,60 +657,74 @@ 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())}.` ); } - } - - 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, + + await upsertComment( [ - 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." - ] + COMMENT_MARKER, + stateMarker(state), + "", + ...finalSections, + "", + draftExplanation + ].join("\n") ); - } - - core.setFailed(`PR quality gate failed: ${failureSummary(failures)}`); - return; + + 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; } // No quality failure; the draft is owed by the open checklist. @@ -723,15 +737,7 @@ jobs: await upsertComment( [ COMMENT_MARKER, - stateMarker({ - version: 1, - active: false, - autoDraftedByBot: false, - titlePrefixedByBot: false, - ancestryFailed: false, - descriptionFailed: false, - screenshotFailed: false - }), + stateMarker(clearedEnforcerState()), "", "✅ **PR quality gates passed**", "", @@ -810,24 +816,8 @@ jobs: // a clean contributor PR that never failed a quality gate has none. if (storedState?.active || botComment) { 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 - }; + ? { ...clearedEnforcerState(), active: true, autoDraftedByBot: true } + : clearedEnforcerState(); const titleResult = storedState.titlePrefixedByBot ? `The ${inlineCode(TITLE_PREFIX.trim())} title prefix has been removed.` @@ -862,11 +852,7 @@ jobs: if (checklistRequired && checklistComplete) { const readinessState = storedReadinessState ? { ...storedReadinessState } - : { - version: READINESS_STATE_VERSION, - autoDraftedByBot: false, - maintainersPinged: false - }; + : defaultReadinessState(); let notified = false; if (!readinessState.maintainersPinged) { readinessState.maintainersPinged = true; From 40b99b73e753c2ac916b1062663c2e9f69a556ea Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 00:36:30 +0200 Subject: [PATCH 4/8] fix(ci): address Codex/CodeRabbit review findings on the readiness gate --- .github/scripts/pr-quality.cjs | 23 +++- .github/scripts/pr-quality.test.cjs | 55 +++++++++ .github/workflows/enforce-pr-target.yml | 62 ++++++++-- AGENTS.md | 3 +- MAINTAINERS.md | 2 +- .../content/docs/contributing/pr-quality.md | 7 +- tests/ci-workflows.test.ts | 111 +++++++++++++++++- tests/helpers/enforce-pr-target-harness.ts | 42 ++++++- 8 files changed, 278 insertions(+), 27 deletions(-) diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index 366767fd16..fc6461cbab 100644 --- a/.github/scripts/pr-quality.cjs +++ b/.github/scripts/pr-quality.cjs @@ -238,23 +238,40 @@ function buildReviewReadinessSection() { */ function extractReviewReadiness(body) { if (typeof body !== "string") { - return { present: false, complete: false, checked: 0, total: 0 }; + return { + present: false, + complete: false, + checked: 0, + total: 0, + items: [], + }; } const start = body.indexOf(REVIEW_READINESS_START); const end = body.indexOf(REVIEW_READINESS_END); + // Any marker presence counts as present: an author-edited section that is + // inverted or partial must never trigger another append, or every `edited` + // event would stack a second checklist (and a second body write). if (start === -1 || end === -1 || end <= start) { - return { present: false, complete: false, checked: 0, total: 0 }; + 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 checked = boxes.filter((match) => match[1] !== " ").length; 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, }; } diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index 2eea6b1e20..1e22baec87 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -245,12 +245,14 @@ describe("review readiness checklist", () => { complete: false, checked: 0, total: 0, + items: [], }); assert.deepEqual(extractReviewReadiness(null), { present: false, complete: false, checked: 0, total: 0, + items: [], }); }); @@ -265,6 +267,12 @@ describe("review readiness checklist", () => { 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."); @@ -273,9 +281,30 @@ describe("review readiness checklist", () => { 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.") @@ -290,6 +319,7 @@ describe("review readiness checklist", () => { 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( "", @@ -298,6 +328,31 @@ describe("review readiness checklist", () => { 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; + assert.equal(extractReviewReadiness(duplicate).present, true); + assert.equal(appendReviewReadinessSection(duplicate), duplicate); }); it("appends once and is idempotent", () => { diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index afdd99e7a3..9473396521 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -186,8 +186,22 @@ jobs: 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 = [ - ...text.matchAll( + ...section.matchAll( /\[\@([A-Za-z0-9_-]+)\]\(https:\/\/github\.com\/[^)]*\)/g ) ].map(match => match[1]); @@ -205,7 +219,7 @@ jobs: function readinessChecklistLines(readiness) { return REVIEW_READINESS_ITEMS.map( (item, index) => - `- ${index < readiness.checked ? "✅" : "⬜"} ${item}` + `- ${readiness.items?.[index]?.checked ? "✅" : "⬜"} ${item}` ); } @@ -217,13 +231,17 @@ jobs: "", "## Review readiness checklist", "", - "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.", + 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.", "", - ...readinessChecklistLines(readiness), + ...(readiness.present ? readinessChecklistLines(readiness) : []), "", - complete - ? "✅ **4/4** boxes ticked." - : `**${readiness.checked}/${readiness.total}** boxes ticked.`, + readiness.present + ? complete + ? "✅ **4/4** boxes ticked." + : `**${readiness.checked}/${readiness.total}** boxes ticked.` + : "", "", ...extra ]; @@ -776,6 +794,24 @@ jobs: } 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) { + try { + await markReadyForReview(); + } catch (error) { + core.warning( + `Could not mark pull request ready for review: ${error.message}` + ); + } + await upsertReadinessComment( + { ...storedReadinessState, autoDraftedByBot: false }, + readiness, + [ + "✅ This PR is ready for review." + ] + ); + } core.info( "All PR quality gates passed and there is no active bot state." ); @@ -819,13 +855,13 @@ jobs: ? { ...clearedEnforcerState(), active: true, autoDraftedByBot: true } : clearedEnforcerState(); - const titleResult = storedState.titlePrefixedByBot + 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 + : storedState?.autoDraftedByBot ? "The pull request has been marked ready for review again." : "Its existing draft status has been preserved."; @@ -853,14 +889,14 @@ jobs: const readinessState = storedReadinessState ? { ...storedReadinessState } : defaultReadinessState(); + const maintainers = readMaintainerLogins().filter( + login => login !== pr.user.login + ); let notified = false; - if (!readinessState.maintainersPinged) { + if (!readinessState.maintainersPinged && maintainers.length > 0) { readinessState.maintainersPinged = true; notified = true; } - const maintainers = readMaintainerLogins().filter( - login => login !== pr.user.login - ); await upsertReadinessComment( readinessState, diff --git a/AGENTS.md b/AGENTS.md index 5bb20b8b72..d7c0c2cf86 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -182,7 +182,8 @@ 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. +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 b90709790b..c8a94010c6 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -30,7 +30,7 @@ see [The retired `dev2-go` line](#the-retired-dev2-go-line). 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. + 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 6e212253c9..ae6a27cd44 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -45,9 +45,10 @@ tells you exactly what to change: 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. A retarget to `dev` clears - the wrong-branch message automatically and is remembered by the gate; the - draft stays until the checklist is complete. + 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 a38f45d674..542cdf2834 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -19,9 +19,11 @@ function lastEnforcerCommentBody(result: HarnessResult): string { const enforcerCreates = creates.filter( call => call.body.includes(marker) || call.body.includes(legacyMarker), ); - return (enforcerCreates.length > 0 ? enforcerCreates : creates)[ - enforcerCreates.length > 0 ? enforcerCreates.length - 1 : creates.length - 1 - ]!.body; + 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). */ @@ -32,6 +34,9 @@ function lastReadinessCommentBody(result: HarnessResult): string { 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; } @@ -1073,6 +1078,24 @@ describe("GitHub Actions hardening", () => { "", "- 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 { @@ -1211,6 +1234,7 @@ describe("GitHub Actions hardening", () => { draft: true, body: readinessChecklistBody(4), }, + maintainersFile: MAINTAINERS_FIXTURE, }); // No prior enforcer history: the checklist completion alone lifts the @@ -1228,6 +1252,79 @@ describe("GitHub Actions hardening", () => { expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); }); + 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 }, + authorPermission: "write", + maintainersFile: MAINTAINERS_FIXTURE, + comments: [readinessComment({ + version: 1, + autoDraftedByBot: true, + maintainersPinged: false, + })], + }); + expect(methodsOf(recovered)).toEqual(readsAllowedBase([ + "graphql", + "issues.updateComment", + ])); + const [ready] = callsTo(recovered, "graphql") as [{ query: string }]; + expect(ready.query).toContain("markPullRequestReadyForReview"); + expect(lastReadinessCommentBody(recovered)).toContain( + "not required for this author", + ); + expect(lastReadinessCommentBody(recovered)).toContain('"autoDraftedByBot":false'); + expect(recovered.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + 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: { @@ -1464,6 +1561,7 @@ describe("GitHub Actions hardening", () => { draft: true, body: readinessChecklistBody(4), }, + maintainersFile: MAINTAINERS_FIXTURE, comments: [botComment({ version: 1, active: true, @@ -1489,6 +1587,9 @@ describe("GitHub Actions hardening", () => { 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 () => { @@ -2744,8 +2845,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\(/); diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index 292d0f1525..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 }[] = []; @@ -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. */ From a62bc19f46b306a740852006a46410db1b92bf9b Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 02:30:22 +0200 Subject: [PATCH 5/8] fix(ci): address second Codex review round on the readiness gate --- .github/scripts/enforce-pr-target.test.cjs | 4 +- .github/scripts/pr-quality.cjs | 23 +++++++- .github/scripts/pr-quality.test.cjs | 50 ++++++++++++++++ .github/workflows/enforce-pr-target.yml | 41 +++++++++++-- tests/ci-workflows.test.ts | 69 ++++++++++++++++++---- 5 files changed, 166 insertions(+), 21 deletions(-) diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 34f0d1c839..5cc20b721b 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -48,9 +48,9 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /synchronize/); }); - it("checks out trusted default-branch scripts only (never PR head)", () => { + it("checks out trusted base-branch scripts only (never PR head)", () => { assert.match(workflow, /actions\/checkout@[0-9a-f]{40}/); - assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.repository\.default_branch\s*\}\}/); + assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.ref\s*\}\}/); // The readiness ping reads MAINTAINERS.md from the same trusted checkout. assert.match(workflow, /sparse-checkout:\s*\|\s*\n\s*\.github\/scripts\n\s*MAINTAINERS\.md/); assert.match(workflow, /persist-credentials:\s*false/); diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index fc6461cbab..19af720289 100644 --- a/.github/scripts/pr-quality.cjs +++ b/.github/scripts/pr-quality.cjs @@ -125,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(); @@ -287,6 +288,21 @@ function appendReviewReadinessSection(body) { return `${body.trimEnd()}\n\n${section}\n`; } +/** + * Remove the bot-managed readiness section from a body. Used so the bot's own + * checklist never counts as author-written description substance, and so a + * confirmed maintainer's body can retire the injected section. + */ +function stripReviewReadinessSection(body) { + if (typeof body !== "string") return body; + const start = body.indexOf(REVIEW_READINESS_START); + const end = body.indexOf(REVIEW_READINESS_END); + if (start === -1 || end === -1 || end <= start) return body; + const stripped = + body.slice(0, start) + body.slice(end + REVIEW_READINESS_END.length); + return stripped.replace(/\n{3,}/g, "\n\n").trimEnd(); +} + function collectPrQualityFailures({ baseRef, allowedBases, @@ -355,6 +371,7 @@ module.exports = { buildReviewReadinessSection, extractReviewReadiness, appendReviewReadinessSection, + stripReviewReadinessSection, collectPrQualityFailures, hasEscapedNewlines, stripPrTemplateBoilerplate, diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index 1e22baec87..2c10400d36 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -13,6 +13,7 @@ const { buildReviewReadinessSection, extractReviewReadiness, appendReviewReadinessSection, + stripReviewReadinessSection, collectPrQualityFailures, } = require("./pr-quality.cjs"); @@ -369,6 +370,55 @@ describe("review readiness checklist", () => { 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", () => { diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 9473396521..f3a77433ea 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -29,7 +29,11 @@ jobs: - name: Checkout trusted PR-quality scripts uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: - ref: ${{ github.event.repository.default_branch }} + # The PR's base branch, 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`. + ref: ${{ github.event.pull_request.base.ref }} persist-credentials: false sparse-checkout: | .github/scripts @@ -46,6 +50,7 @@ jobs: authorHasPushPermission, extractReviewReadiness, appendReviewReadinessSection, + stripReviewReadinessSection, REVIEW_READINESS_ITEMS } = require( path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), @@ -543,11 +548,27 @@ jobs: !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. - let readiness = extractReviewReadiness(pr.body); if (checklistRequired && !readiness.present) { const injectedBody = appendReviewReadinessSection(pr.body ?? ""); await github.rest.pulls.update({ @@ -610,6 +631,10 @@ jobs: } 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, @@ -770,8 +795,8 @@ jobs: try { await convertToDraft(); draftConverted = true; - readinessState.autoDraftedByBot = true; } catch (error) { + readinessState.autoDraftedByBot = false; core.warning( `Could not convert pull request to draft: ${error.message}` ); @@ -797,18 +822,24 @@ jobs: // 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( - { ...storedReadinessState, autoDraftedByBot: false }, + recoveryFailed + ? { ...storedReadinessState } + : { ...storedReadinessState, autoDraftedByBot: false }, readiness, [ - "✅ This PR is ready for review." + 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." ] ); } diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 542cdf2834..6a5d36a5d0 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -840,10 +840,13 @@ describe("GitHub Actions hardening", () => { "sparse-checkout", ]); expect(checkout.with).toEqual({ - ref: "${{ github.event.repository.default_branch }}", + // The base branch, 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. + ref: "${{ github.event.pull_request.base.ref }}", "persist-credentials": false, // MAINTAINERS.md rides along so the completion ping reads the canonical - // maintainer list from the trusted default branch. + // maintainer list from the same trusted base revision as the scripts. "sparse-checkout": ".github/scripts\nMAINTAINERS.md\n", }); @@ -972,11 +975,12 @@ describe("GitHub Actions hardening", () => { return found; } - // Four `pulls.update` sites: the checklist injection (body only), 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. + // 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"], @@ -1216,6 +1220,12 @@ describe("GitHub Actions hardening", () => { "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", ); @@ -1272,7 +1282,7 @@ describe("GitHub Actions hardening", () => { expect(lastReadinessCommentBody(duringFailure)).toContain('"autoDraftedByBot":true'); const recovered = await runEnforcePrTarget(script, { - pr: { base: { ref: "dev" }, draft: true }, + pr: { base: { ref: "dev" }, draft: true, body: readinessChecklistBody(0) }, authorPermission: "write", maintainersFile: MAINTAINERS_FIXTURE, comments: [readinessComment({ @@ -1282,18 +1292,55 @@ describe("GitHub Actions hardening", () => { })], }); 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"); - expect(lastReadinessCommentBody(recovered)).toContain( - "not required for this author", - ); - expect(lastReadinessCommentBody(recovered)).toContain('"autoDraftedByBot":false'); + 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` From a924576346f55937e5a8684ee9f86994b3126e14 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 02:38:04 +0200 Subject: [PATCH 6/8] fix(ci): pin script checkout to event base SHA; treat duplicate checklist markers as malformed --- .github/scripts/enforce-pr-target.test.cjs | 2 +- .github/scripts/pr-quality.cjs | 21 +++++++++++++++++++-- .github/scripts/pr-quality.test.cjs | 8 +++++++- .github/workflows/enforce-pr-target.yml | 7 ++++--- tests/ci-workflows.test.ts | 9 +++++---- 5 files changed, 36 insertions(+), 11 deletions(-) diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 5cc20b721b..f11f094268 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -50,7 +50,7 @@ describe("enforce-pr-target workflow", () => { it("checks out trusted base-branch scripts only (never PR head)", () => { assert.match(workflow, /actions\/checkout@[0-9a-f]{40}/); - assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.ref\s*\}\}/); + assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}/); // The readiness ping reads MAINTAINERS.md from the same trusted checkout. assert.match(workflow, /sparse-checkout:\s*\|\s*\n\s*\.github\/scripts\n\s*MAINTAINERS\.md/); assert.match(workflow, /persist-credentials:\s*false/); diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index 19af720289..79768fbc9b 100644 --- a/.github/scripts/pr-quality.cjs +++ b/.github/scripts/pr-quality.cjs @@ -249,10 +249,19 @@ function extractReviewReadiness(body) { } 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). - if (start === -1 || end === -1 || end <= start) { + // 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, @@ -298,6 +307,14 @@ function stripReviewReadinessSection(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(); diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index 2c10400d36..4645cea988 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -352,8 +352,14 @@ describe("review readiness checklist", () => { assert.equal(appendReviewReadinessSection(orphanEnd), orphanEnd); const duplicate = SECTION + SECTION; - assert.equal(extractReviewReadiness(duplicate).present, true); + 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", () => { diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index f3a77433ea..856f55f9e1 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -29,11 +29,12 @@ jobs: - name: Checkout trusted PR-quality scripts uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: - # The PR's base branch, not the repository default: pull_request_target + # 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`. - ref: ${{ github.event.pull_request.base.ref }} + # 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 diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 6a5d36a5d0..6538f2de16 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -840,10 +840,11 @@ describe("GitHub Actions hardening", () => { "sparse-checkout", ]); expect(checkout.with).toEqual({ - // The base branch, 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. - ref: "${{ github.event.pull_request.base.ref }}", + // 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, // MAINTAINERS.md rides along so the completion ping reads the canonical // maintainer list from the same trusted base revision as the scripts. From 1fc7ab94b703f0aea4a63376e55f7379487d41fc Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 02:41:57 +0200 Subject: [PATCH 7/8] test(ci): scope the base-SHA checkout pin to the checkout step --- .github/scripts/enforce-pr-target.test.cjs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index f11f094268..840c2e4049 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -49,11 +49,16 @@ describe("enforce-pr-target workflow", () => { }); it("checks out trusted base-branch scripts only (never PR head)", () => { - assert.match(workflow, /actions\/checkout@[0-9a-f]{40}/); - assert.match(workflow, /ref:\s*\$\{\{\s*github\.event\.pull_request\.base\.sha\s*\}\}/); + // 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(workflow, /sparse-checkout:\s*\|\s*\n\s*\.github\/scripts\n\s*MAINTAINERS\.md/); - assert.match(workflow, /persist-credentials:\s*false/); + 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/); }); From 8daef74fdb1cb6a18de0184486ec9ef447eaad5a Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 4 Aug 2026 09:57:59 +0900 Subject: [PATCH 8/8] test(ci): pin the two-run checklist laundering path at the workflow layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `assessPrDescription` already refuses the injected section, and the unit tests cover that directly. What was not pinned is the sequence that exploits it, which no single-function test can express: the attack needs two runs and a body the bot itself wrote in between. Run one opens a contributor PR with an empty description; the gate fails `bad_description` and injects the checklist. Run two ticks the four boxes the bot just added. If the injected section ever counts as substance again, the description gate passes on text the author never wrote and the PR is marked ready for review — an empty PR, reviewable, with the maintainers pinged. The test takes the injected body from the recorded `pulls.update` call rather than a hand-built fixture, so it exercises the text the gate actually writes, and asserts `graphql` is never reached — that is where `markPullRequestReadyForReview` goes out. The screenshot axis gets the same treatment: the injected section adds renderable structure but no image, so a gui-cued PR with a complete checklist and no screenshot must still fail and stay drafted. Driven red to prove it is not vacuous: reverting the `stripReviewReadinessSection` call in `assessPrDescription` fails the laundering test and nothing else. Verified: bun test tests/ci-workflows.test.ts 83 pass; node --test on .github/scripts 58 pass; typecheck and privacy:scan pass. --- tests/ci-workflows.test.ts | 75 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 6538f2de16..e35788cb10 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1263,6 +1263,81 @@ describe("GitHub Actions hardening", () => { 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