diff --git a/.github/scripts/pr-maintainers.cjs b/.github/scripts/pr-maintainers.cjs new file mode 100644 index 0000000000..d96cdc6810 --- /dev/null +++ b/.github/scripts/pr-maintainers.cjs @@ -0,0 +1,35 @@ +"use strict"; + +/** + * Maintainers from `MAINTAINERS.md` text. Only the current-maintainers table + * is authoritative; the change log below it can mention retired accounts. + * Missing the section heading means we cannot identify current maintainers, + * so the recipient list is empty rather than scanning the whole file. + */ +function parseMaintainerLogins(text) { + const heading = /^## Current maintainers[ \t]*\r?$/m.exec(text ?? ""); + if (heading === null) { + return []; + } + + const sectionStart = heading.index; + const nextHeading = text.indexOf( + "\n## ", + sectionStart + "## Current maintainers".length + ); + const section = text.slice( + sectionStart, + nextHeading === -1 ? text.length : nextHeading + ); + const logins = [ + ...section.matchAll( + /\[\@([A-Za-z0-9_-]+)\]\(https:\/\/github\.com\/[^)]*\)/g + ) + ].map(match => match[1]); + + return [...new Set(logins)]; +} + +module.exports = { + parseMaintainerLogins +}; diff --git a/.github/scripts/pr-maintainers.test.cjs b/.github/scripts/pr-maintainers.test.cjs new file mode 100644 index 0000000000..20da1896ac --- /dev/null +++ b/.github/scripts/pr-maintainers.test.cjs @@ -0,0 +1,75 @@ +"use strict"; + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { + parseMaintainerLogins +} = require("./pr-maintainers.cjs"); + +const 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.", + "- [@retired](https://github.com/retired) stepped down.", +].join("\n"); + +describe("parseMaintainerLogins", () => { + it("reads the current-maintainers table and excludes the change log", () => { + assert.deepEqual(parseMaintainerLogins(FIXTURE), [ + "lidge-jun", + "Ingwannu", + "Wibias", + ]); + }); + + it("returns an empty list when the section heading is missing", () => { + const text = "- [@only](https://github.com/only) is listed."; + assert.deepEqual(parseMaintainerLogins(text), []); + }); + it("does not match a ### subsection or prose mentioning the heading", () => { + const subsection = [ + "### Current maintainers", + "| [@subsection](https://github.com/subsection) | x |", + ].join("\n"); + assert.deepEqual(parseMaintainerLogins(subsection), []); + + const prose = [ + "See the ## Current maintainers section below.", + "| [@prose](https://github.com/prose) | x |", + ].join("\n"); + assert.deepEqual(parseMaintainerLogins(prose), []); + }); + + it("accepts a valid CRLF heading with trailing whitespace and a following H2", () => { + const crlf = [ + "## Current maintainers \t", + "| [@crlf](https://github.com/crlf) | x |", + "## Changelog", + "| [@retired](https://github.com/retired) | y |", + ].join("\r\n"); + assert.deepEqual(parseMaintainerLogins(crlf), ["crlf"]); + }); + + + it("handles empty and duplicate-free output", () => { + assert.deepEqual(parseMaintainerLogins(""), []); + assert.deepEqual( + parseMaintainerLogins( + [ + "## Current maintainers", + "| [@dup](https://github.com/dup) | x |", + "| [@dup](https://github.com/dup) | y |", + ].join("\n"), + ), + ["dup"], + ); + }); +}); diff --git a/.github/scripts/pr-quality-messages.cjs b/.github/scripts/pr-quality-messages.cjs new file mode 100644 index 0000000000..e42d31c374 --- /dev/null +++ b/.github/scripts/pr-quality-messages.cjs @@ -0,0 +1,207 @@ +"use strict"; + +const { + REVIEW_READINESS_ITEMS +} = require("./pr-quality.cjs"); +const { + readinessStateMarker, + READINESS_LATEST_DEV_BEHIND_MAX +} = require("./pr-quality-state.cjs"); + +/** Marks the bot's review-readiness checklist message. */ +const READINESS_MARKER = ""; + +function inlineCode(value) { + const text = String(value); + const longestBacktickRun = Math.max( + 0, + ...(text.match(/`+/g) ?? []).map(run => run.length) + ); + const delimiter = "`".repeat(longestBacktickRun + 1); + return `${delimiter}${text}${delimiter}`; +} + +function readinessChecklistLines(readiness) { + return REVIEW_READINESS_ITEMS.map( + (item, index) => + `- ${readiness.items?.[index]?.checked ? "✅" : "⬜"} ${item}` + ); +} + +/** + * The full readiness-message body: marker, serialized state, mirror lines for + * the tickable boxes, the tick count, and the path-specific extra lines. + */ +function buildReadinessCommentBody(state, readiness, extra) { + const complete = readiness.present && readiness.complete; + + return [ + READINESS_MARKER, + readinessStateMarker(state), + "", + "## Review readiness checklist", + "", + readiness.present + ? "This PR is kept in **draft** until every requirement below is fulfilled. The tickable checklist has been added to your PR description — tick all four boxes there." + : "The review readiness checklist is not required for this author.", + "", + ...(readiness.present ? readinessChecklistLines(readiness) : []), + "", + readiness.present + ? complete + ? "✅ **4/4** boxes ticked." + : `**${readiness.checked}/${readiness.total}** boxes ticked.` + : "", + "", + ...extra + ]; +} + +function descriptionFailureLines(reason) { + switch (reason) { + case "empty": + return [ + "The pull request body is empty after stripping HTML comments.", + "", + "Include a real description: a **Summary** of what changed and why, plus a **Test plan** (or equivalent substance)." + ]; + case "placeholder": + return [ + "The pull request body contains only placeholder text (for example `N/A`, `TODO`, or `No response`).", + "", + "Replace placeholders with a **Summary** and **Test plan**, or another description with at least two substantive sections or paragraphs." + ]; + case "escaped_newlines": + return [ + "The pull request body uses literal `\\n` escape sequences instead of real line breaks.", + "", + "Fix the formatting so the body uses normal markdown line breaks, then add a **Summary** and **Test plan**." + ]; + case "thin": + default: + return [ + "The pull request description is too thin to review.", + "", + "Add a **Summary** and **Test plan** (two sections with at least 40 characters each), or an unstructured body of at least 120 characters with two paragraphs or bullet groups." + ]; + } +} + +function buildFailureSections(failures, { pr, allowedBases, defaultBase }) { + const sections = []; + + if (failures.some(failure => failure.code === "wrong_base")) { + sections.push( + "⚠️ **Wrong target branch**", + "", + `This pull request currently targets ${inlineCode(pr.base.ref)}, but pull requests must target one of ${allowedBases.map(inlineCode).join(" or ")}.`, + "", + `@${pr.user.login} Please retarget this PR to ${inlineCode(defaultBase)}. All contributions go to ${inlineCode(defaultBase)}; \`main\` receives only release promotions. See our [Contributing guide](https://lidge-jun.github.io/opencodex/contributing/) for details. Thanks! 🙏` + ); + } + + if (failures.some(failure => failure.code === "wrong_ancestry")) { + sections.push( + "⚠️ **Wrong branch ancestry**", + "", + `This pull request targets ${inlineCode(pr.base.ref)}, but its head appears to sit on the current ${inlineCode("main")} tip while being far behind ${inlineCode(pr.base.ref)}.`, + "", + `@${pr.user.login} Rebase onto the current ${inlineCode(pr.base.ref)} branch instead of opening from ${inlineCode("main")}. That keeps already-released commits out of the integration branch.` + ); + } + + const badDescription = failures.find( + failure => failure.code === "bad_description" + ); + if (badDescription) { + sections.push( + "⚠️ **Pull request description**", + "", + ...descriptionFailureLines(badDescription.reason) + ); + } + + if ( + failures.some( + failure => failure.code === "missing_ui_screenshot" + ) + ) { + sections.push( + "⚠️ **UI screenshot required**", + "", + `This pull request mentions ${inlineCode("gui")} in its title or description, so it is treated as a GUI change.`, + "", + `@${pr.user.login} Please add a screenshot of the UI change to the description — drag and drop the image into the description editor, or paste a markdown image such as ${inlineCode("![Screenshot](https://example.com/after.png)")}. The check re-runs automatically once the description is edited.` + ); + } + + return sections; +} + +function failureSummary(failures, { pr }) { + return failures + .map(failure => { + if (failure.code === "wrong_base") { + return `wrong base (${pr.base.ref})`; + } + if (failure.code === "wrong_ancestry") { + return "wrong ancestry"; + } + if (failure.code === "bad_description") { + return `bad description (${failure.reason})`; + } + if (failure.code === "missing_ui_screenshot") { + return "missing UI screenshot"; + } + return failure.code; + }) + .join("; "); +} + +/** The notice shown when the gate's own claim check disproves a ticked box. */ +function buildClaimCheckNotice(violations, liveHeadSha) { + const lines = []; + for (const code of violations) { + if (code === "ci_green") { + lines.push( + `GitHub CI is not green on the current head ${inlineCode(liveHeadSha.slice(0, 7))}; the **CI green** box has been unticked.` + ); + } else if (code === "latest_dev") { + lines.push( + `The PR is more than ${READINESS_LATEST_DEV_BEHIND_MAX} commits behind ${inlineCode("dev")}; the **latest dev** box has been unticked.` + ); + } + } + lines.push( + "The checklist has been reset: re-test against the latest code and tick the boxes again." + ); + return lines; +} + +/** The reset notice shown when a completion no longer covers the live head. */ +function buildStaleNotice({ completionHeadSha, liveHeadSha, eventAction }) { + let lead; + if (completionHeadSha !== null) { + lead = `New commits were pushed after the checklist was completed on ${inlineCode(String(completionHeadSha).slice(0, 7))}; the current head is ${inlineCode(liveHeadSha.slice(0, 7))}.`; + } else if (eventAction === "synchronize") { + lead = `A complete checklist was found on a synchronize event with no recorded completion head; the current head is ${inlineCode(liveHeadSha.slice(0, 7))}.`; + } else { + lead = `The checklist was ticked before the current head ${inlineCode(liveHeadSha.slice(0, 7))} was pushed.`; + } + return [ + lead, + "The checklist has been reset: re-test against the latest code and tick all four boxes again." + ]; +} + +module.exports = { + READINESS_MARKER, + inlineCode, + readinessChecklistLines, + buildReadinessCommentBody, + descriptionFailureLines, + buildFailureSections, + failureSummary, + buildStaleNotice, + buildClaimCheckNotice +}; diff --git a/.github/scripts/pr-quality-messages.test.cjs b/.github/scripts/pr-quality-messages.test.cjs new file mode 100644 index 0000000000..893f9173ec --- /dev/null +++ b/.github/scripts/pr-quality-messages.test.cjs @@ -0,0 +1,204 @@ +"use strict"; + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { + buildReviewReadinessSection +} = require("./pr-quality.cjs"); +const { + READINESS_MARKER, + inlineCode, + readinessChecklistLines, + buildReadinessCommentBody, + descriptionFailureLines, + buildFailureSections, + failureSummary, + buildStaleNotice, + buildClaimCheckNotice +} = require("./pr-quality-messages.cjs"); + +const PR = { + base: { ref: "main" }, + user: { login: "contributor" } +}; +const ALLOWED_BASES = ["dev"]; +const DEFAULT_BASE = "dev"; + +describe("inlineCode", () => { + it("wraps values with a delimiter longer than any backtick run", () => { + assert.equal(inlineCode("dev"), "`dev`"); + assert.equal(inlineCode("a`b"), "``a`b``"); + assert.equal(inlineCode("a``b"), "```a``b```"); + assert.equal(inlineCode(42), "`42`"); + }); +}); + +describe("readinessChecklistLines", () => { + it("mirrors per-item checked state", () => { + const readiness = { + items: [{ checked: true }, { checked: false }, { checked: true }, { checked: false }] + }; + const lines = readinessChecklistLines(readiness); + assert.equal(lines.length, 4); + assert.match(lines[0], /^\- ✅ /); + assert.match(lines[1], /^\- ⬜ /); + }); +}); + +describe("buildReadinessCommentBody", () => { + const readiness = { + present: true, + complete: false, + checked: 1, + total: 4, + items: [{ checked: true }, { checked: false }, { checked: false }, { checked: false }] + }; + + it("carries the marker, serialized state, mirror, and tick count", () => { + const state = { version: 2, maintainersPinged: false }; + const body = buildReadinessCommentBody(state, readiness, ["extra line"]).join("\n"); + assert.ok(body.startsWith(READINESS_MARKER)); + assert.ok(body.includes('/; +/** Regex that finds the readiness state marker inside a bot comment body. */ +const READINESS_STATE_PATTERN = + //; + +/** + * v2 adds `completedAtHeadSha` so a completed checklist is bound to the exact + * head it attested. v1 states (no field) are read the same way: the binding + * only starts on the next completion. + */ +const READINESS_STATE_VERSION = 2; + +/** A completed checklist may attest "on the latest dev" while the head is up to + * this many commits behind the base. Beyond it the box no longer holds. */ +const READINESS_LATEST_DEV_BEHIND_MAX = 10; + +/** Parse the enforcer state marker, or `null` when absent or unreadable. */ +function parseState(body, warn = () => {}) { + const match = body?.match(STATE_PATTERN); + + if (!match) { + return null; + } + + try { + return JSON.parse(match[1]); + } catch (error) { + warn(`Could not parse stored workflow state: ${error.message}`); + + return null; + } +} + +/** Serialize the enforcer state into its comment marker. */ +function stateMarker(state) { + return ( + "" + ); +} + +/** Parse the readiness state marker, or `null` when absent or unreadable. */ +function parseReadinessState(body, warn = () => {}) { + const match = body?.match(READINESS_STATE_PATTERN); + + if (!match) { + return null; + } + + try { + return JSON.parse(match[1]); + } catch (error) { + warn(`Could not parse stored readiness state: ${error.message}`); + + return null; + } +} + +/** Serialize the readiness state into its comment marker. */ +function readinessStateMarker(state) { + return ( + "" + ); +} + +/** The enforcer comment state after every quality gate clears. */ +function clearedEnforcerState() { + return { + version: 1, + active: false, + autoDraftedByBot: false, + titlePrefixedByBot: false, + ancestryFailed: false, + descriptionFailed: false, + screenshotFailed: false + }; +} + +/** Fresh enforcer state for a run that must draft the PR. */ +function defaultEnforcerState() { + return { + version: 1, + active: true, + 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, + completedAtHeadSha: null + }; +} + +/** + * A completed checklist is an attestation about a specific head. The + * attestation is stale when the recorded completion head differs from the + * live head (new commits landed after the last completion) or when the boxes + * were ticked in an event that saw an older head than the live one — a push + * raced the `edited` job, so no completion head was recorded yet but the + * ticks predate the code under review — or when a synchronize event sees a + * complete checklist with no recorded head at all (the completion job may + * still be queued for an older head). + */ + +/** + * Bot-side verification of the two checklist claims the gate can check itself. + * The CI box only holds when the head's `ci` check is green, and the + * latest-dev box only holds while the head is at most + * READINESS_LATEST_DEV_BEHIND_MAX commits behind the base. Unknown state + * (compare or checks lookup failed) fails closed: an unverifiable claim is a + * violation, because an attestation must not ride on missing evidence. + */ +function readinessClaimViolations({ + ciGreen, + behindBase, + behindUnknown = false, + behindMax = READINESS_LATEST_DEV_BEHIND_MAX +}) { + const violations = []; + if (!ciGreen) { + violations.push("ci_green"); + } + if (behindUnknown || behindBase > behindMax) { + violations.push("latest_dev"); + } + return violations; +} + +function completionIsStale({ + checklistRequired, + checklistComplete, + readinessPresent, + completionHeadSha, + eventHeadSha, + liveHeadSha, + eventAction +}) { + const completionRecordedForLiveHead = + completionHeadSha !== null && completionHeadSha === liveHeadSha; + // A push raced the edited job: the event still carries the older head the + // boxes were ticked against. + const ticksPredateLiveHead = + completionHeadSha === null && + checklistComplete && + eventHeadSha !== liveHeadSha; + // A complete checklist with no recorded head on synchronize has no + // provenance for which head was attested. The edited job may still be + // queued for an older head; do not let this push inherit that attestation. + const unrecordedCompleteOnSynchronize = + completionHeadSha === null && + checklistComplete && + eventAction === "synchronize"; + + return ( + checklistRequired && + readinessPresent && + ((completionHeadSha !== null && !completionRecordedForLiveHead) || + ticksPredateLiveHead || + unrecordedCompleteOnSynchronize) + ); +} + +module.exports = { + READINESS_LATEST_DEV_BEHIND_MAX, + readinessClaimViolations, + STATE_PATTERN, + READINESS_STATE_PATTERN, + READINESS_STATE_VERSION, + parseState, + stateMarker, + parseReadinessState, + readinessStateMarker, + clearedEnforcerState, + defaultEnforcerState, + defaultReadinessState, + completionIsStale +}; diff --git a/.github/scripts/pr-quality-state.test.cjs b/.github/scripts/pr-quality-state.test.cjs new file mode 100644 index 0000000000..e3b63fd5c2 --- /dev/null +++ b/.github/scripts/pr-quality-state.test.cjs @@ -0,0 +1,263 @@ +"use strict"; + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { + parseState, + stateMarker, + parseReadinessState, + readinessStateMarker, + clearedEnforcerState, + defaultEnforcerState, + defaultReadinessState, + completionIsStale, + readinessClaimViolations, + READINESS_LATEST_DEV_BEHIND_MAX, + READINESS_STATE_VERSION +} = require("./pr-quality-state.cjs"); + +describe("enforcer state markers", () => { + it("parses a valid enforcer state marker", () => { + const state = { version: 1, active: true, autoDraftedByBot: true }; + assert.deepEqual( + parseState(``), + state, + ); + assert.deepEqual( + parseState( + ``, + ), + state, + ); + }); + + it("returns null for markerless or unreadable state and warns", () => { + assert.equal(parseState("plain comment"), null); + assert.equal(parseState(null), null); + const warnings = []; + assert.equal( + parseState("", message => + warnings.push(message), + ), + null, + ); + assert.match(warnings[0], /Could not parse stored workflow state/); + }); + + it("round-trips through stateMarker", () => { + const state = { version: 1, active: false }; + assert.deepEqual(parseState(stateMarker(state)), state); + }); + + it("parses and serializes readiness state with warnings on failure", () => { + const state = { version: 2, maintainersPinged: true }; + assert.deepEqual( + parseReadinessState( + ``, + ), + state, + ); + assert.deepEqual(parseReadinessState(readinessStateMarker(state)), state); + const warnings = []; + assert.equal( + parseReadinessState("", m => + warnings.push(m), + ), + null, + ); + assert.match(warnings[0], /Could not parse stored readiness state/); + }); +}); + +describe("state defaults", () => { + it("builds the cleared enforcer state", () => { + assert.deepEqual(clearedEnforcerState(), { + version: 1, + active: false, + autoDraftedByBot: false, + titlePrefixedByBot: false, + ancestryFailed: false, + descriptionFailed: false, + screenshotFailed: false + }); + }); + + it("builds the fresh active enforcer state", () => { + const state = defaultEnforcerState(); + assert.equal(state.active, true); + assert.equal(state.version, 1); + }); + + it("builds the fresh readiness state at the current version", () => { + assert.deepEqual(defaultReadinessState(), { + version: READINESS_STATE_VERSION, + autoDraftedByBot: false, + maintainersPinged: false, + completedAtHeadSha: null + }); + }); +}); + +describe("completionIsStale", () => { + const base = { + checklistRequired: true, + readinessPresent: true, + liveHeadSha: "2222222222222222222222222222222222222222" + }; + + it("is not stale when the recorded completion head matches the live head", () => { + assert.equal( + completionIsStale({ + ...base, + checklistComplete: true, + completionHeadSha: base.liveHeadSha, + eventHeadSha: base.liveHeadSha + }), + false, + ); + }); + + it("is stale when the recorded head differs from the live head, even with an open checklist", () => { + // Open checklist + mismatched recorded head is the partial-reset window. + assert.equal( + completionIsStale({ + ...base, + checklistComplete: false, + completionHeadSha: "1111111111111111111111111111111111111111", + eventHeadSha: base.liveHeadSha + }), + true, + ); + }); + + it("is stale when ticks predate the live head on a first completion", () => { + assert.equal( + completionIsStale({ + ...base, + checklistComplete: true, + completionHeadSha: null, + eventHeadSha: "1111111111111111111111111111111111111111" + }), + true, + ); + }); + + it("is not stale when ticks predate the live head but nothing is ticked", () => { + assert.equal( + completionIsStale({ + ...base, + checklistComplete: false, + completionHeadSha: null, + eventHeadSha: "1111111111111111111111111111111111111111" + }), + false, + ); + }); + it("is stale when a complete checklist has no recorded head on synchronize", () => { + assert.equal( + completionIsStale({ + ...base, + checklistComplete: true, + completionHeadSha: null, + eventHeadSha: base.liveHeadSha, + eventAction: "synchronize" + }), + true, + ); + }); + + it("is not stale for an unrecorded complete checklist on a non-synchronize event", () => { + assert.equal( + completionIsStale({ + ...base, + checklistComplete: true, + completionHeadSha: null, + eventHeadSha: base.liveHeadSha, + eventAction: "edited" + }), + false, + ); + }); + + + it("is not stale for maintainers or absent checklists", () => { + assert.equal( + completionIsStale({ + ...base, + checklistRequired: false, + checklistComplete: true, + completionHeadSha: "1111111111111111111111111111111111111111", + eventHeadSha: base.liveHeadSha + }), + false, + ); + assert.equal( + completionIsStale({ + ...base, + readinessPresent: false, + checklistComplete: true, + completionHeadSha: "1111111111111111111111111111111111111111", + eventHeadSha: base.liveHeadSha + }), + false, + ); + }); +}); + +describe("readinessClaimViolations", () => { + it("passes when CI is green and the head is current", () => { + assert.deepEqual( + readinessClaimViolations({ ciGreen: true, behindBase: 0 }), + [], + ); + assert.deepEqual( + readinessClaimViolations({ ciGreen: true, behindBase: 10 }), + [], + ); + }); + + it("flags red CI", () => { + assert.deepEqual( + readinessClaimViolations({ ciGreen: false, behindBase: 0 }), + ["ci_green"], + ); + }); + + it("flags a head more than the threshold behind the base", () => { + assert.deepEqual( + readinessClaimViolations({ + ciGreen: true, + behindBase: READINESS_LATEST_DEV_BEHIND_MAX + 1, + }), + ["latest_dev"], + ); + }); + + it("flags both when both claims fail", () => { + assert.deepEqual( + readinessClaimViolations({ + ciGreen: false, + behindBase: READINESS_LATEST_DEV_BEHIND_MAX + 20, + }), + ["ci_green", "latest_dev"], + ); + }); + + it("fails closed when the behind count is unknown", () => { + assert.deepEqual( + readinessClaimViolations({ + ciGreen: true, + behindBase: 0, + behindUnknown: true, + }), + ["latest_dev"], + ); + }); + + it("honours a custom threshold", () => { + assert.deepEqual( + readinessClaimViolations({ ciGreen: true, behindBase: 5, behindMax: 4 }), + ["latest_dev"], + ); + }); +}); diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index 79768fbc9b..f401b90c68 100644 --- a/.github/scripts/pr-quality.cjs +++ b/.github/scripts/pr-quality.cjs @@ -31,6 +31,16 @@ const REVIEW_READINESS_ITEMS = [ "My PR is ready for review.", ]; +/** + * Which checklist box each bot-verifiable claim maps to. The order must stay + * in sync with REVIEW_READINESS_ITEMS: index 0 is the CI claim and index 1 is + * the latest-dev claim. + */ +const REVIEW_READINESS_CLAIM_INDEX = { + ci_green: 0, + latest_dev: 1 +}; + /** * Exact instruction / checklist lines from `.github/PULL_REQUEST_TEMPLATE.md`. * Untouched templates must not count as substance. @@ -320,6 +330,79 @@ function stripReviewReadinessSection(body) { return stripped.replace(/\n{3,}/g, "\n\n").trimEnd(); } +/** + * Replace the bot-managed readiness section with a fresh unticked copy. + * Used when new commits land after the checklist was completed: the old + * attestation covered a different head, so every box resets and the author + * must re-tick against the latest code. Malformed marker sets (duplicates, + * extra pairs) stay untouched, matching `stripReviewReadinessSection`. + */ + +/** + * Untick only the given 0-based checklist boxes inside the bot-managed + * section, leaving every other box and the surrounding body byte-for-byte + * unchanged. Used when the gate's own claim check disproves a ticked box + * (CI not green, head too far behind dev): the false claim is removed while + * the still-true boxes survive. Malformed marker sets stay untouched. + */ +function uncheckReviewReadinessBoxes(body, indexes) { + 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; + if ( + body.split(REVIEW_READINESS_START).length - 1 !== 1 || + body.split(REVIEW_READINESS_END).length - 1 !== 1 + ) { + return body; + } + const wanted = new Set(indexes); + let boxIndex = 0; + const section = body.slice( + start + REVIEW_READINESS_START.length, + end + ); + const updatedSection = section.replace( + /^([ \t]*[-*]\s+)\[([ xX])\](?=\s)/gm, + (match, lead, mark) => { + const current = boxIndex; + boxIndex += 1; + if (wanted.has(current) && mark !== " ") { + return lead + "[ ]"; + } + return match; + } + ); + if (updatedSection === section) return body; + return ( + body.slice(0, start + REVIEW_READINESS_START.length) + + updatedSection + + body.slice(end) + ); +} + +function resetReviewReadinessSection(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; + if ( + body.split(REVIEW_READINESS_START).length - 1 !== 1 || + body.split(REVIEW_READINESS_END).length - 1 !== 1 + ) { + return body; + } + const section = buildReviewReadinessSection(); + // Splice only the bounded section: the author's surrounding content — + // including deliberate blank lines and trailing markdown — stays byte for + // byte identical to what they wrote. + return ( + body.slice(0, start) + + section + + body.slice(end + REVIEW_READINESS_END.length) + ); +} + function collectPrQualityFailures({ baseRef, allowedBases, @@ -389,6 +472,9 @@ module.exports = { extractReviewReadiness, appendReviewReadinessSection, stripReviewReadinessSection, + REVIEW_READINESS_CLAIM_INDEX, + uncheckReviewReadinessBoxes, + resetReviewReadinessSection, collectPrQualityFailures, hasEscapedNewlines, stripPrTemplateBoilerplate, diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index 4645cea988..6bf40964eb 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -14,6 +14,9 @@ const { extractReviewReadiness, appendReviewReadinessSection, stripReviewReadinessSection, + uncheckReviewReadinessBoxes, + REVIEW_READINESS_CLAIM_INDEX, + resetReviewReadinessSection, collectPrQualityFailures, } = require("./pr-quality.cjs"); @@ -399,6 +402,140 @@ describe("review readiness checklist", () => { assert.equal(stripReviewReadinessSection("plain body"), "plain body"); assert.equal(stripReviewReadinessSection(null), null); }); + + it("resets every checked box to unticked and keeps the surrounding body", () => { + const body = [ + "## Summary", + "Author content.", + "", + SECTION.replaceAll("- [ ] ", "- [x] "), + "", + "## Test plan", + "- Ran the suite.", + ].join("\n"); + const reset = resetReviewReadinessSection(body); + const extracted = extractReviewReadiness(reset); + assert.equal(extracted.present, true); + assert.equal(extracted.complete, false); + assert.equal(extracted.checked, 0); + assert.equal(extracted.total, 4); + assert.equal((reset.match(/\[x\]/g) || []).length, 0); + assert.ok(reset.includes("Author content.")); + assert.ok(reset.includes("## Test plan")); + }); + + it("resets a partially ticked section as well", () => { + const partial = SECTION.replace( + "- [ ] My PR is ready for review.", + "- [x] My PR is ready for review.", + ); + const reset = resetReviewReadinessSection(partial); + const extracted = extractReviewReadiness(reset); + assert.equal(extracted.checked, 0); + assert.equal(extracted.complete, false); + }); + + it("preserves the surrounding author formatting exactly", () => { + const body = [ + "## Summary", + "Author content.", + "", + "", + SECTION.replaceAll("- [ ] ", "- [x] "), + "", + "", + "Trailing note with blank lines above.", + "", + ].join("\n"); + const reset = resetReviewReadinessSection(body); + // Only the bounded section changed; deliberate blank lines and trailing + // markdown survive byte for byte (no `\n{3,}` collapse, no trimEnd). + assert.equal(reset, body.replaceAll("- [x] ", "- [ ] ")); + }); + + it("is idempotent on an already-unticked section", () => { + const once = resetReviewReadinessSection( + SECTION.replaceAll("- [ ] ", "- [x] "), + ); + assert.equal(resetReviewReadinessSection(once), once); + assert.equal(extractReviewReadiness(once).checked, 0); + }); + + it("leaves markerless and malformed bodies alone", () => { + assert.equal(resetReviewReadinessSection("plain body"), "plain body"); + assert.equal(resetReviewReadinessSection(null), null); + const duplicate = SECTION + SECTION; + assert.equal(resetReviewReadinessSection(duplicate), duplicate); + const inverted = + "\n" + + "\n" + + "- [x] orphan box"; + assert.equal(resetReviewReadinessSection(inverted), inverted); + }); +}); + +describe("uncheckReviewReadinessBoxes", () => { + const checkedBody = [ + "## Summary", + "", + "Substantive summary text for the author's own description.", + "", + "## Test plan", + "", + "- [x] Run the suite", + "", + "", + "## Review readiness checklist", + "", + "- [x] All CI tests are green on my local testing.", + "- [x] I pushed my PR to the latest dev commit.", + "- [x] I fixed all correct Codex and CodeRabbit findings.", + "- [x] My PR is ready for review.", + "", + ].join("\n"); + + it("unchecks only the requested boxes", () => { + const body = uncheckReviewReadinessBoxes(checkedBody, [ + REVIEW_READINESS_CLAIM_INDEX.ci_green, + ]); + assert.ok(body.includes("- [ ] All CI tests are green on my local testing.")); + assert.ok(body.includes("- [x] I pushed my PR to the latest dev commit.")); + assert.ok(body.includes("- [x] My PR is ready for review.")); + }); + + it("can uncheck several boxes at once", () => { + const body = uncheckReviewReadinessBoxes(checkedBody, [ + REVIEW_READINESS_CLAIM_INDEX.ci_green, + REVIEW_READINESS_CLAIM_INDEX.latest_dev, + ]); + assert.ok(body.includes("- [ ] All CI tests are green on my local testing.")); + assert.ok(body.includes("- [ ] I pushed my PR to the latest dev commit.")); + assert.ok(body.includes("- [x] I fixed all correct Codex and CodeRabbit findings.")); + assert.ok(body.includes("- [x] My PR is ready for review.")); + }); + + it("preserves the surrounding author content exactly", () => { + const body = uncheckReviewReadinessBoxes(checkedBody, [0]); + assert.ok(body.startsWith("## Summary\n")); + assert.ok(body.includes("- [x] Run the suite\n")); + assert.ok(body.endsWith("")); + // The three untouched checklist boxes keep their ticks; only the CI box + // flipped. The author's own box in the Test plan is untouched too. + const checklist = body.split("")[1]; + assert.equal((checklist.match(/- \[x\]/g) || []).length, 3); + }); + + it("is idempotent on an already-unchecked box", () => { + const once = uncheckReviewReadinessBoxes(checkedBody, [0]); + const twice = uncheckReviewReadinessBoxes(once, [0]); + assert.equal(twice, once); + }); + + it("leaves markerless and malformed bodies alone", () => { + assert.equal(uncheckReviewReadinessBoxes("plain body", [0]), "plain body"); + const malformed = checkedBody + "\n"; + assert.equal(uncheckReviewReadinessBoxes(malformed, [0]), malformed); + }); }); describe("assessPrDescription with the readiness section", () => { diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 856f55f9e1..94023e7d0a 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -52,22 +52,62 @@ jobs: extractReviewReadiness, appendReviewReadinessSection, stripReviewReadinessSection, - REVIEW_READINESS_ITEMS + uncheckReviewReadinessBoxes, + REVIEW_READINESS_CLAIM_INDEX, + resetReviewReadinessSection } = require( path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), ); + const { + parseState, + stateMarker, + parseReadinessState, + clearedEnforcerState, + defaultEnforcerState, + defaultReadinessState, + completionIsStale, + readinessClaimViolations, + READINESS_STATE_VERSION + } = require( + path.join( + process.cwd(), + ".github", + "scripts", + "pr-quality-state.cjs" + ), + ); + const { + READINESS_MARKER, + inlineCode, + buildReadinessCommentBody, + buildFailureSections, + failureSummary, + buildStaleNotice, + buildClaimCheckNotice + } = require( + path.join( + process.cwd(), + ".github", + "scripts", + "pr-quality-messages.cjs" + ), + ); + const { + parseMaintainerLogins + } = require( + path.join( + process.cwd(), + ".github", + "scripts", + "pr-maintainers.cjs" + ), + ); const ALLOWED_BASES = ["dev"]; const DEFAULT_BASE = "dev"; const TITLE_PREFIX = "[WRONG BRANCH] "; const COMMENT_MARKER = ""; 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; @@ -104,83 +144,10 @@ jobs: ); let readinessCommentId = readinessComment?.id ?? null; const storedReadinessState = parseReadinessState( - readinessComment?.body + readinessComment?.body, + message => core.warning(message) ); - function parseState(body) { - const match = body?.match(STATE_PATTERN); - - if (!match) { - return null; - } - - try { - return JSON.parse(match[1]); - } catch (error) { - core.warning( - `Could not parse stored workflow state: ${error.message}` - ); - - return null; - } - } - - function stateMarker(state) { - return ( - "" - ); - } - - function parseReadinessState(body) { - const match = body?.match(READINESS_STATE_PATTERN); - - if (!match) { - return null; - } - - try { - return JSON.parse(match[1]); - } catch (error) { - core.warning( - `Could not parse stored readiness state: ${error.message}` - ); - - return null; - } - } - - function readinessStateMarker(state) { - return ( - "" - ); - } - - /** The enforcer comment state after every quality gate clears. */ - function clearedEnforcerState() { - return { - version: 1, - active: false, - autoDraftedByBot: false, - titlePrefixedByBot: false, - ancestryFailed: false, - descriptionFailed: false, - screenshotFailed: false - }; - } - - /** Fresh checklist-message state for a contributor PR. */ - function defaultReadinessState() { - return { - version: READINESS_STATE_VERSION, - autoDraftedByBot: false, - maintainersPinged: false - }; - } - /** * Maintainers from `MAINTAINERS.md` on the trusted default branch * (checked out sparse by the step above). The file is the canonical @@ -192,27 +159,7 @@ 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 = [ - ...section.matchAll( - /\[\@([A-Za-z0-9_-]+)\]\(https:\/\/github\.com\/[^)]*\)/g - ) - ].map(match => match[1]); - - return [...new Set(logins)]; + return parseMaintainerLogins(text); } catch (error) { core.warning( `Could not read ${MAINTAINERS_FILE}: ${error.message}` @@ -222,35 +169,8 @@ jobs: } } - function readinessChecklistLines(readiness) { - return REVIEW_READINESS_ITEMS.map( - (item, index) => - `- ${readiness.items?.[index]?.checked ? "✅" : "⬜"} ${item}` - ); - } - async function upsertReadinessComment(state, readiness, extra) { - const complete = readiness.present && readiness.complete; - const lines = [ - READINESS_MARKER, - readinessStateMarker(state), - "", - "## Review readiness checklist", - "", - readiness.present - ? "This PR is kept in **draft** until every requirement below is fulfilled. The tickable checklist has been added to your PR description — tick all four boxes there." - : "This PR is ready for review; the review readiness checklist is not required for this author.", - "", - ...(readiness.present ? readinessChecklistLines(readiness) : []), - "", - readiness.present - ? complete - ? "✅ **4/4** boxes ticked." - : `**${readiness.checked}/${readiness.total}** boxes ticked.` - : "", - "", - ...extra - ]; + const lines = buildReadinessCommentBody(state, readiness, extra); if (readinessCommentId) { await github.rest.issues.updateComment({ @@ -272,10 +192,6 @@ jobs: readinessCommentId = created.data.id; } - function inlineCode(value) { - return `\`${String(value).replaceAll("`", "\\`")}\``; - } - async function upsertComment(body) { if (botCommentId) { await github.rest.issues.updateComment({ @@ -341,108 +257,10 @@ jobs: ); } - function descriptionFailureLines(reason) { - switch (reason) { - case "empty": - return [ - "The pull request body is empty after stripping HTML comments.", - "", - "Include a real description: a **Summary** of what changed and why, plus a **Test plan** (or equivalent substance)." - ]; - case "placeholder": - return [ - "The pull request body contains only placeholder text (for example `N/A`, `TODO`, or `No response`).", - "", - "Replace placeholders with a **Summary** and **Test plan**, or another description with at least two substantive sections or paragraphs." - ]; - case "escaped_newlines": - return [ - "The pull request body uses literal `\\n` escape sequences instead of real line breaks.", - "", - "Fix the formatting so the body uses normal markdown line breaks, then add a **Summary** and **Test plan**." - ]; - case "thin": - default: - return [ - "The pull request description is too thin to review.", - "", - "Add a **Summary** and **Test plan** (two sections with at least 40 characters each), or an unstructured body of at least 120 characters with two paragraphs or bullet groups." - ]; - } - } - - function buildFailureSections(failures) { - const sections = []; - - if (failures.some(failure => failure.code === "wrong_base")) { - sections.push( - "⚠️ **Wrong target branch**", - "", - `This pull request currently targets ${inlineCode(pr.base.ref)}, but pull requests must target one of ${ALLOWED_BASES.map(inlineCode).join(" or ")}.`, - "", - `@${pr.user.login} Please retarget this PR to ${inlineCode(DEFAULT_BASE)}. All contributions go to ${inlineCode(DEFAULT_BASE)}; \`main\` receives only release promotions. See our [Contributing guide](https://lidge-jun.github.io/opencodex/contributing/) for details. Thanks! 🙏` - ); - } - - if (failures.some(failure => failure.code === "wrong_ancestry")) { - sections.push( - "⚠️ **Wrong branch ancestry**", - "", - `This pull request targets ${inlineCode(pr.base.ref)}, but its head appears to sit on the current ${inlineCode("main")} tip while being far behind ${inlineCode(pr.base.ref)}.`, - "", - `@${pr.user.login} Rebase onto the current ${inlineCode(pr.base.ref)} branch instead of opening from ${inlineCode("main")}. That keeps already-released commits out of the integration branch.` - ); - } - - const badDescription = failures.find( - failure => failure.code === "bad_description" - ); - if (badDescription) { - sections.push( - "⚠️ **Pull request description**", - "", - ...descriptionFailureLines(badDescription.reason) - ); - } - - if ( - failures.some( - failure => failure.code === "missing_ui_screenshot" - ) - ) { - sections.push( - "⚠️ **UI screenshot required**", - "", - `This pull request mentions ${inlineCode("gui")} in its title or description, so it is treated as a GUI change.`, - "", - `@${pr.user.login} Please add a screenshot of the UI change to the description — drag and drop the image into the description editor, or paste a markdown image such as ${inlineCode("![Screenshot](https://example.com/after.png)")}. The check re-runs automatically once the description is edited.` - ); - } - - return sections; - } - - function failureSummary(failures) { - return failures - .map(failure => { - if (failure.code === "wrong_base") { - return `wrong base (${pr.base.ref})`; - } - if (failure.code === "wrong_ancestry") { - return "wrong ancestry"; - } - if (failure.code === "bad_description") { - return `bad description (${failure.reason})`; - } - if (failure.code === "missing_ui_screenshot") { - return "missing UI screenshot"; - } - return failure.code; - }) - .join("; "); - } - - const storedState = parseState(botComment?.body); + const storedState = parseState( + botComment?.body, + message => core.warning(message) + ); let authorPermission = null; let permissionLookupFailed = false; @@ -580,29 +398,178 @@ jobs: }); readiness = extractReviewReadiness(injectedBody); } - const checklistComplete = readiness.present && readiness.complete; + let checklistComplete = readiness.present && readiness.complete; + + // A completed checklist is an attestation about a specific head + // (see `completionIsStale`). When it is stale the gate resets the + // boxes and the notification state, re-drafts, and tells the + // author to re-test and re-tick against the latest code. + const eventHeadSha = + context.payload.pull_request?.head?.sha ?? pr.head.sha; + const completionHeadSha = + storedReadinessState?.completedAtHeadSha ?? null; + const headDrifted = completionIsStale({ + checklistRequired, + checklistComplete, + readinessPresent: readiness.present, + completionHeadSha, + eventHeadSha, + liveHeadSha: pr.head.sha, + eventAction: context.payload.action + }); + + let readinessStateOverride = null; + let headDriftNotice = []; + let revalidationNotice = []; + if (headDrifted) { + // Re-fetch the PR so an author edit that landed while this job + // was reading cannot be clobbered by the reset. + const { data: freshPr } = await github.rest.pulls.get({ + owner, + repo, + pull_number + }); + const freshReadiness = extractReviewReadiness( + freshPr.body ?? "" + ); + readinessStateOverride = defaultReadinessState(); + headDriftNotice = buildStaleNotice({ + completionHeadSha, + liveHeadSha: freshPr.head.sha, + eventAction: context.payload.action + }); + if (freshReadiness.present && freshReadiness.complete) { + const resetBody = resetReviewReadinessSection( + freshPr.body ?? "" + ); + if (resetBody !== freshPr.body) { + await github.rest.pulls.update({ + owner, + repo, + pull_number, + body: resetBody + }); + } + readiness = extractReviewReadiness(resetBody); + } else { + readiness = freshReadiness; + } + checklistComplete = readiness.present && readiness.complete; + } + + // The bot verifies the two checklist claims it can check itself. + // The CI box only counts when the head's `ci` check (the repo's + // documented "CI passed" signal) is green; the latest-dev box only + // counts while the head is at most READINESS_LATEST_DEV_BEHIND_MAX + // commits behind the base. A disproved claim unchecks that box and + // keeps the PR a draft, exactly like a head-drift reset. + let claimViolations = []; + let claimNotice = []; + if ( + checklistRequired && + checklistComplete && + !headDrifted && + failures.length === 0 + ) { + let ciGreen = true; + try { + const { data: checksData } = + await github.rest.checks.listForRef({ + owner, + repo, + ref: pr.head.sha, + per_page: 100 + }); + const ciCheck = (checksData.check_runs ?? []).find( + check => check.name === "ci" + ); + // No `ci` check means no CI run exists for this head (for + // example a docs-only change): there is nothing to contradict + // the author's claim. A real `ci` check must be completed + // successfully. + ciGreen = + ciCheck === undefined || + (ciCheck.status === "completed" && + ciCheck.conclusion === "success"); + } catch (error) { + core.warning( + `Could not list checks for the readiness claim check: ${error.message}` + ); + ciGreen = false; + } + claimViolations = readinessClaimViolations({ + ciGreen, + behindBase, + behindUnknown: ancestryLookupFailed + }); + if (claimViolations.length > 0) { + const { data: freshPr } = await github.rest.pulls.get({ + owner, + repo, + pull_number + }); + const freshReadiness = extractReviewReadiness( + freshPr.body ?? "" + ); + readinessStateOverride = defaultReadinessState(); + claimNotice = buildClaimCheckNotice( + claimViolations, + freshPr.head.sha + ); + if (freshReadiness.present) { + const uncheckedBody = uncheckReviewReadinessBoxes( + freshPr.body ?? "", + claimViolations.map( + code => REVIEW_READINESS_CLAIM_INDEX[code] + ) + ); + if (uncheckedBody !== freshPr.body) { + await github.rest.pulls.update({ + owner, + repo, + pull_number, + body: uncheckedBody + }); + } + readiness = extractReviewReadiness(uncheckedBody); + } else { + readiness = freshReadiness; + } + 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); + // Which reset notice (head drift vs claim check) accompanies the + // draft path; only one can be active because the claim check is + // skipped when the head drifted. + revalidationNotice = headDrifted + ? headDriftNotice + : claimNotice; + if (mustDraft) { let draftConverted = false; - const readinessState = storedReadinessState - ? { ...storedReadinessState } - : defaultReadinessState(); + const readinessState = + readinessStateOverride ?? + (storedReadinessState + ? { ...storedReadinessState } + : defaultReadinessState()); + if (checklistRequired && checklistComplete) { + // The attestation covers this head even while another quality + // gate keeps the draft: bind it now, because the failure path + // below returns before the completion block that records it. + // A later push then still resets the checklist instead of + // sliding the completion forward onto un-attested code. + readinessState.completedAtHeadSha = pr.head.sha; + readinessState.version = READINESS_STATE_VERSION; + } const state = storedState?.active ? { ...storedState } - : { - version: 1, - active: true, - autoDraftedByBot: false, - titlePrefixedByBot: false, - ancestryFailed: false, - descriptionFailed: false, - screenshotFailed: false - }; + : defaultEnforcerState(); const hasWrongBase = failures.some( failure => failure.code === "wrong_base" ); @@ -640,6 +607,7 @@ jobs: readinessState, readiness, [ + ...revalidationNotice, "This PR stays in draft until every box above is ticked." ] ); @@ -657,7 +625,11 @@ jobs: ); let draftConversionFailed = false; - const failureSections = buildFailureSections(failures); + const failureSections = buildFailureSections(failures, { + pr, + allowedBases: ALLOWED_BASES, + defaultBase: DEFAULT_BASE + }); if (checklistRequired && !checklistComplete) { failureSections.push( @@ -758,6 +730,7 @@ jobs: readinessState, readiness, [ + ...revalidationNotice, checklistComplete ? "✅ **All four boxes are ticked.** This PR still stays in draft until the issues above are resolved." : pr.draft || draftConverted @@ -767,7 +740,9 @@ jobs: ); } - core.setFailed(`PR quality gate failed: ${failureSummary(failures)}`); + core.setFailed( + `PR quality gate failed: ${failureSummary(failures, { pr })}` + ); return; } @@ -811,6 +786,7 @@ jobs: readinessState, readiness, [ + ...revalidationNotice, 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." @@ -930,11 +906,18 @@ jobs: notified = true; } + // Bind the completion to the exact head it attested. A later + // `synchronize` event with a different head resets the checklist + // and the notification state (see `headDrifted` above). + readinessState.completedAtHeadSha = pr.head.sha; + readinessState.version = READINESS_STATE_VERSION; + await upsertReadinessComment( readinessState, readiness, [ "✅ **All four boxes are ticked.**", + `Completed against head ${inlineCode(pr.head.sha.slice(0, 7))}; new commits after this will reset the checklist.`, readyConverted ? "This pull request has been marked Ready for Review." : pr.draft diff --git a/AGENTS.md b/AGENTS.md index 544bdc3f5a..060fa06b9a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -188,7 +188,14 @@ stay there until a four-box review-readiness checklist in the description is complete: local CI green, branch on the latest `dev` commit, all correct Codex and CodeRabbit findings fixed, and the ready-for-review confirmation. When all four boxes are ticked the gate marks the PR ready and notifies the maintainers -listed in `MAINTAINERS.md` (excluding the author). +listed in `MAINTAINERS.md` (excluding the author). Completion is bound to the +exact commit the PR head pointed at: if new commits are pushed afterwards, the +gate moves the PR back to draft, resets the checklist and the notification, +and asks the author to test and tick the boxes again against the latest code. +Before a completion is accepted, the gate verifies the two checklist claims it +can check itself: the head's `ci` check must be green, and the branch must be +on the latest `dev` commit or at most 10 commits behind it. A disproved claim +unticks the matching box and keeps the PR a draft. 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 c8a94010c6..81e57197bc 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -31,6 +31,14 @@ see [The retired `dev2-go` line](#the-retired-dev2-go-line). all correct Codex and CodeRabbit findings fixed, and the ready-for-review confirmation. When all four boxes are ticked the gate marks the PR ready and notifies the maintainers listed in `MAINTAINERS.md` (excluding the author). + Completion is bound to the exact commit the PR head pointed at: if new + commits are pushed afterwards, the gate moves the PR back to draft, resets + the checklist and the notification, and asks the author to test and tick the + boxes again against the latest code. + Before a completion is accepted, the gate verifies the two checklist claims + it can check itself: the head's `ci` check must be green, and the branch + must be on the latest `dev` commit or at most 10 commits behind it. A + disproved claim unticks the matching box and keeps the PR a draft. 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 326431a1d9..1bc5ac3958 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -46,9 +46,16 @@ tells you exactly what to change: commit, all correct Codex and CodeRabbit findings fixed, and the ready-for-review confirmation. Once every box is ticked the check marks the PR ready for review and notifies the maintainers listed in `MAINTAINERS.md` - (excluding the author). A retarget to `dev` clears the wrong-branch message - automatically and is remembered by the gate; the draft stays until the - checklist is complete. + (excluding the author). Completion is bound to the exact commit the PR head + pointed at: if new commits are pushed afterwards, the gate moves the PR back + to draft, resets the checklist and the maintainer notification, and asks you + to test and tick the boxes again against the latest code. A retarget to + `dev` clears the wrong-branch message automatically and is remembered by the + gate; the draft stays until the checklist is complete. + Before a completion is accepted, the gate verifies the two checklist claims + it can check itself: the head's `ci` check must be green, and the branch + must be on the latest `dev` commit or at most 10 commits behind it. A + disproved claim unticks the matching box and keeps the PR a draft. - **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 abe220531b..2ca6f48f88 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -694,7 +694,14 @@ describe("GitHub Actions hardening", () => { return { workflow, jobs, steps: steps!, allSteps, script }; } - const SCRIPT_LOAD = ["require", "require", "require"] as const; + const SCRIPT_LOAD = [ + "require", + "require", + "require", + "require", + "require", + "require", + ] as const; /** Reads every allowed-base PR performs before any enforcement writes. */ function readsAllowedBase(tail: string[] = []): string[] { @@ -1007,11 +1014,14 @@ describe("GitHub Actions hardening", () => { return found; } - // 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. + // Seven `pulls.update` sites: the maintainer checklist retirement, the + // checklist injection, the head-drift reset, and the claim-check uncheck + // (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"], ["body", "owner", "pull_number", "repo"], ["body", "owner", "pull_number", "repo"], ["owner", "pull_number", "repo", "title"], @@ -1045,7 +1055,9 @@ describe("GitHub Actions hardening", () => { !name.endsWith(".list") && !name.endsWith(".listComments") && name !== "github.rest.repos.getCollaboratorPermissionLevel" && - name !== "github.rest.repos.compareCommitsWithBasehead", + name !== "github.rest.repos.compareCommitsWithBasehead" && + // The claim check reads check-runs; it must never count as a write. + name !== "github.rest.checks.listForRef", ); expect([...new Set(restWrites)].sort()).toEqual([ "github.rest.issues.createComment", @@ -1282,6 +1294,7 @@ describe("GitHub Actions hardening", () => { // No prior enforcer history: the checklist completion alone lifts the // draft and notifies the maintainers from MAINTAINERS.md. expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "issues.createComment", ])); @@ -1294,6 +1307,570 @@ describe("GitHub Actions hardening", () => { expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); }); + test("completing the checklist records the head it was completed on", async () => { + // A pre-binding v1 state has no recorded SHA. Completion on the current + // head binds forward instead of resetting, so a checklist that was + // completed before this feature exists does not draft every already-ready + // PR on the first run after the upgrade. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + comments: [readinessComment({ + version: 1, + autoDraftedByBot: true, + maintainersPinged: true, + })], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "issues.updateComment", + ])); + const readinessBody = lastReadinessCommentBody(result); + // The completion is bound to the exact head that was reviewed. + expect(readinessBody).toContain( + '"completedAtHeadSha":"3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"', + ); + // A migrated v1 state is rewritten at the current version. + expect(readinessBody).toContain('"version":2'); + expect(readinessBody).toContain("Completed against head `3f1c0de`"); + // Already pinged before the upgrade: no second notification. + expect(readinessBody).toContain('"maintainersPinged":true'); + expect(readinessBody).not.toContain("Maintainers notified"); + }); + + test("new commits after checklist completion re-draft, reset the checklist, and clear the notification state", async () => { + // The reviewer's gap: a completed checklist is an attestation about a + // specific head. When new commits land, the attestation no longer covers + // the code under review, so the gate resets the boxes and the maintainer + // ping, converts the PR back to a draft, and tells the author to + // re-test and re-tick on the latest code. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + comments: [readinessComment({ + version: 2, + autoDraftedByBot: false, + maintainersPinged: true, + completedAtHeadSha: "1111111111111111111111111111111111111111", + })], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "pulls.get", + "pulls.update", + "issues.updateComment", + "graphql", + "issues.updateComment", + ])); + const [resetBody] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(resetBody.body).toContain(CHECKLIST_START); + expect(resetBody.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(resetBody.body).toContain("- [ ] My PR is ready for review."); + expect(resetBody.body).not.toContain("- [x]"); + + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(1); + expect(drafts[0]!.query).toContain("convertPullRequestToDraft"); + expect(drafts[0]!.query).not.toContain("markPullRequestReadyForReview"); + + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("**0/4** boxes ticked"); + expect(readinessBody).toContain('"completedAtHeadSha":null'); + expect(readinessBody).toContain('"maintainersPinged":false'); + expect(readinessBody).toContain( + "New commits were pushed after the checklist was completed on `1111111`", + ); + expect(readinessBody).toContain( + "The checklist has been reset: re-test against the latest code and tick all four boxes again.", + ); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("a checklist completed on the current head is not reset on a rerun", async () => { + // Same head, same boxes, already notified: the rerun is a no-op apart + // from refreshing the readiness message. No re-draft, no body rewrite, + // no second maintainer ping. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + comments: [readinessComment({ + version: 2, + autoDraftedByBot: false, + maintainersPinged: true, + completedAtHeadSha: "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b", + })], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "issues.updateComment", + ])); + expect(callsTo(result, "pulls.update")).toEqual([]); + expect(callsTo(result, "graphql")).toEqual([]); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("**4/4** boxes ticked"); + expect(readinessBody).toContain( + '"completedAtHeadSha":"3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"', + ); + expect(readinessBody).toContain('"maintainersPinged":true'); + expect(readinessBody).not.toContain("Maintainers notified"); + }); + + test("new commits after completion still enforce quality failures", async () => { + // The head-drift reset folds into the existing failure path instead of + // short-circuiting it: a PR that drifted onto a wrong base is drafted, + // the checklist resets, AND the wrong-base gate still fails closed with + // its title prefix and explanation. + const result = await run({ + pr: { + base: { ref: "main" }, + draft: false, + title: "Add a thing", + body: readinessChecklistBody(4), + }, + comments: [readinessComment({ + version: 2, + autoDraftedByBot: false, + maintainersPinged: true, + completedAtHeadSha: "1111111111111111111111111111111111111111", + })], + }); + + expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.get", + "pulls.update", + "issues.updateComment", + "issues.createComment", + "pulls.update", + "issues.updateComment", + "graphql", + "issues.updateComment", + "issues.updateComment", + "issues.updateComment", + ])); + expect(lastEnforcerCommentBody(result)).toContain("Wrong target branch"); + expect(lastEnforcerCommentBody(result)).toContain("[WRONG BRANCH]"); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(true); + + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("**0/4** boxes ticked"); + expect(readinessBody).toContain('"maintainersPinged":false'); + expect(readinessBody).toContain('"completedAtHeadSha":null'); + expect(readinessBody).toContain( + "New commits were pushed after the checklist was completed", + ); + }); + + test("a completion whose ticks predate the live head is rejected and reset", async () => { + // A push raced the `edited` job: the event saw the older head the boxes + // were ticked against, but the live head is newer. Binding the + // completion to the live head would attest code the author never ticked + // against, so the gate rejects the completion, resets the boxes, and + // re-drafts instead of sliding the attestation forward. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + eventPayload: { + head: { sha: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" }, + }, + maintainersFile: MAINTAINERS_FIXTURE, + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "pulls.get", + "pulls.update", + "issues.createComment", + "graphql", + "issues.updateComment", + ])); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(1); + expect(drafts[0]!.query).toContain("convertPullRequestToDraft"); + expect(drafts[0]!.query).not.toContain("markPullRequestReadyForReview"); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain( + "The checklist was ticked before the current head `3f1c0de` was pushed.", + ); + expect(readinessBody).toContain("**0/4** boxes ticked"); + expect(readinessBody).toContain('"completedAtHeadSha":null'); + expect(readinessBody).toContain('"maintainersPinged":false'); + expect(readinessBody).not.toContain("Maintainers notified"); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("a synchronize event does not inherit an unrecorded complete checklist", async () => { + // The boxes were ticked on head A, but the edited job has not yet + // persisted completedAtHeadSha. A synchronize for head B must not + // mark B ready with A's attestation; it must reset and re-draft. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + eventAction: "synchronize", + maintainersFile: MAINTAINERS_FIXTURE, + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "pulls.get", + "pulls.update", + "issues.createComment", + "graphql", + "issues.updateComment", + ])); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(1); + expect(drafts[0]!.query).toContain("convertPullRequestToDraft"); + expect(drafts[0]!.query).not.toContain("markPullRequestReadyForReview"); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain( + "A complete checklist was found on a synchronize event with no recorded completion head", + ); + expect(readinessBody).toContain("**0/4** boxes ticked"); + expect(readinessBody).toContain('"completedAtHeadSha":null'); + expect(readinessBody).toContain('"maintainersPinged":false'); + expect(readinessBody).not.toContain("Maintainers notified"); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("a complete checklist with red CI unchecks the CI box and re-drafts", async () => { + // The author ticked every box, but the head's `ci` check is red. The + // gate checks the CI claim itself and unticked the CI box instead of + // letting a false attestation lift the draft. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [{ name: "ci", status: "completed", conclusion: "failure" }], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "pulls.get", + "pulls.update", + "issues.createComment", + "graphql", + "issues.updateComment", + ])); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + // Only the CI box is unticked; the other three stay checked. + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(bodyUpdate.body).toContain("- [x] I pushed my PR to the latest dev commit."); + expect(bodyUpdate.body).toContain("- [x] My PR is ready for review."); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(1); + expect(drafts[0]!.query).toContain("convertPullRequestToDraft"); + expect(drafts[0]!.query).not.toContain("markPullRequestReadyForReview"); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain( + "GitHub CI is not green on the current head `3f1c0de`; the **CI green** box has been unticked.", + ); + expect(readinessBody).toContain("**3/4** boxes ticked"); + expect(readinessBody).toContain('"completedAtHeadSha":null'); + expect(readinessBody).toContain('"maintainersPinged":false'); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("a complete checklist more than 10 commits behind dev unchecks the latest-dev box and re-drafts", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + compareByBasehead: { + "dev...3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b": { ahead_by: 0, behind_by: 11 }, + }, + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "pulls.get", + "pulls.update", + "issues.createComment", + "graphql", + "issues.updateComment", + ])); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + // Only the latest-dev box is unticked; CI stays checked. + expect(bodyUpdate.body).toContain("- [x] All CI tests are green on my local testing."); + expect(bodyUpdate.body).toContain("- [ ] I pushed my PR to the latest dev commit."); + expect(bodyUpdate.body).toContain("- [x] My PR is ready for review."); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(1); + expect(drafts[0]!.query).toContain("convertPullRequestToDraft"); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain( + "The PR is more than 10 commits behind `dev`; the **latest dev** box has been unticked.", + ); + expect(readinessBody).toContain("**3/4** boxes ticked"); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("a complete checklist with red CI and a stale dev base unchecks both boxes", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [{ name: "ci", status: "completed", conclusion: "failure" }], + compareByBasehead: { + "dev...3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b": { ahead_by: 0, behind_by: 42 }, + }, + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "pulls.get", + "pulls.update", + "issues.createComment", + "graphql", + "issues.updateComment", + ])); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(bodyUpdate.body).toContain("- [ ] I pushed my PR to the latest dev commit."); + expect(bodyUpdate.body).toContain("- [x] My PR is ready for review."); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("GitHub CI is not green on the current head"); + expect(readinessBody).toContain("more than 10 commits behind `dev`"); + expect(readinessBody).toContain("**2/4** boxes ticked"); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("a checks lookup failure fails closed for the CI claim", async () => { + // Cannot verify CI: the claim is unverifiable, so the box is unticked + // and the PR stays a draft rather than riding on missing evidence. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + failOn: ["checks.listForRef"], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "pulls.get", + "pulls.update", + "issues.createComment", + "graphql", + "issues.updateComment", + ])); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + expect(bodyUpdate.body).toContain("- [x] I pushed my PR to the latest dev commit."); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("GitHub CI is not green on the current head"); + expect(result.warnings.some(w => w.includes("Could not list checks for the readiness claim check"))).toBe(true); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("a head exactly 10 commits behind dev keeps the latest-dev box", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + compareByBasehead: { + "dev...3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b": { ahead_by: 0, behind_by: 10 }, + }, + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "issues.createComment", + ])); + expect(callsTo(result, "pulls.update")).toEqual([]); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(1); + expect(drafts[0]!.query).toContain("markPullRequestReadyForReview"); + }); + + test("a head with no ci check at all keeps the CI box (docs-only style PRs)", async () => { + // No CI run exists for this head: there is nothing to contradict the + // author's claim, so the CI box survives. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "issues.createComment", + ])); + expect(callsTo(result, "pulls.update")).toEqual([]); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts[0]!.query).toContain("markPullRequestReadyForReview"); + }); + + test("a pending ci check cannot attest green", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [{ name: "ci", status: "in_progress", conclusion: null }], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "pulls.get", + "pulls.update", + "issues.createComment", + "graphql", + "issues.updateComment", + ])); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("GitHub CI is not green on the current head"); + }); + + test("a completion recorded while quality gates fail still binds the head", async () => { + // The mustDraft failure path returns before the completion block, so + // without an explicit record the checklist would stay unbound while a + // quality gate is red — the author could push un-attested code and have + // the newest head bound to the old attestation once the gate clears. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + title: "GUI: fix provider list spacing", + body: readinessChecklistBody(4), + }, + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "issues.createComment", + "issues.updateComment", + "graphql", + "issues.updateComment", + "issues.updateComment", + "issues.createComment", + ])); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(true); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain( + '"completedAtHeadSha":"3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"', + ); + expect(readinessBody).toContain('"version":2'); + expect(readinessBody).toContain( + "**All four boxes are ticked.** This PR still stays in draft until the issues above are resolved.", + ); + }); + + test("a stale recorded head with an already-open checklist still recovers the reset state", async () => { + // Partial-reset window: the body update succeeded but the readiness + // comment failed, leaving unticked boxes with the old completion head + // and ping flag. The stale-record detection must not depend on the + // boxes being ticked, or the next completion would be reset one extra + // cycle and the ping would silently survive. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(0), + }, + comments: [readinessComment({ + version: 2, + autoDraftedByBot: false, + maintainersPinged: true, + completedAtHeadSha: "1111111111111111111111111111111111111111", + })], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "pulls.get", + "issues.updateComment", + "graphql", + "issues.updateComment", + ])); + // No body rewrite: the boxes are already unticked from the failed reset. + expect(callsTo(result, "pulls.update")).toEqual([]); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(1); + expect(drafts[0]!.query).toContain("convertPullRequestToDraft"); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain('"completedAtHeadSha":null'); + expect(readinessBody).toContain('"maintainersPinged":false'); + expect(readinessBody).toContain( + "New commits were pushed after the checklist was completed on `1111111`", + ); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("a stale event on a never-completed checklist does not wipe bot state or post a reset notice", async () => { + // `ticksPredateLiveHead` must only fire for an actual completion. A + // stale event on an open checklist has nothing to reset: posting the + // notice would be noise, and replacing the stored state would drop the + // bot's draft-ownership record (`autoDraftedByBot`). + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(1), + }, + eventPayload: { + head: { sha: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" }, + }, + comments: [readinessComment({ + version: 2, + autoDraftedByBot: true, + maintainersPinged: false, + })], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "issues.updateComment", + ])); + expect(callsTo(result, "pulls.update")).toEqual([]); + expect(callsTo(result, "graphql")).toEqual([]); + const readinessBody = lastReadinessCommentBody(result); + // Ownership is preserved and no reset was performed or announced. + expect(readinessBody).toContain('"autoDraftedByBot":true'); + expect(readinessBody).not.toContain('"completedAtHeadSha"'); + expect(readinessBody).not.toContain("ticked before the current head"); + expect(readinessBody).not.toContain("has been reset"); + }); + 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 @@ -1467,6 +2044,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "issues.updateComment", "issues.createComment", @@ -1727,6 +2305,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "issues.updateComment", "issues.createComment", @@ -1787,6 +2366,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "pulls.update", "graphql", "issues.updateComment", @@ -2101,6 +2681,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "pulls.update", "graphql", "issues.updateComment", @@ -2252,6 +2833,7 @@ describe("GitHub Actions hardening", () => { // created comment is the readiness checklist message, which did not // exist on the busy PR yet. expect(methodsOf(result)).toEqual(readsAllowedBasePaged([ + "checks.listForRef", "pulls.update", "graphql", "issues.updateComment", @@ -2376,6 +2958,7 @@ describe("GitHub Actions hardening", () => { expect(callsTo(result, "pulls.update")).toEqual([]); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "issues.updateComment", "issues.createComment", @@ -2469,6 +3052,7 @@ describe("GitHub Actions hardening", () => { comments: [botComment(active)], }); expect(methodsOf(restored)).toEqual(readsAllowedBase([ + "checks.listForRef", "pulls.update", "graphql", "issues.updateComment", @@ -2528,6 +3112,7 @@ describe("GitHub Actions hardening", () => { comments: [botComment({ version: 1, active: "true", autoDraftedByBot: 1, titlePrefixedByBot: "yes" })], }); expect(methodsOf(loose)).toEqual(readsAllowedBase([ + "checks.listForRef", "pulls.update", "graphql", "issues.updateComment", @@ -2549,6 +3134,7 @@ describe("GitHub Actions hardening", () => { comments: [botComment({ version: 1, active: true, autoDraftedByBot: null, titlePrefixedByBot: 0 })], }); expect(methodsOf(falsy)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "issues.updateComment", "issues.createComment", @@ -2742,6 +3328,7 @@ describe("GitHub Actions hardening", () => { // The first comment's state is the one honoured: it says the bot // prefixed and drafted, so both are undone. expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "pulls.update", "graphql", "issues.updateComment", @@ -3040,10 +3627,19 @@ describe("GitHub Actions hardening", () => { 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/); + // The readiness marker and the state serializers live in the shared + // modules the script loads; the script itself must import and use them. + const messagesModule = await readText( + ".github/scripts/pr-quality-messages.cjs", + ); + expect(messagesModule).toMatch( + /READINESS_MARKER = ""/, + ); + expect(script).toMatch(/pr-quality-messages\.cjs/); + // 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 45f3f07808..4527404c44 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -70,6 +70,12 @@ export type RunOptions = { * still passed because the two were aliases. They are independent here. */ eventPayload?: PullRequestState; + /** + * Webhook `action` delivered on the event (opened/edited/synchronize/...). + * Defaults to `"opened"`. Pass `"synchronize"` to exercise push-path + * completion provenance rules. + */ + eventAction?: string; /** * Comments as `listComments` returns them, PAGE BY PAGE. Pass more than one * page to prove the script paginates: an audit round replaced `paginate` with @@ -109,6 +115,12 @@ export type RunOptions = { openPulls?: unknown[]; /** Page-keyed open PR fixtures for `pulls.list` (1-based via array index). */ openPullPages?: unknown[][]; + /** + * Check-runs `checks.listForRef` reports for the head. Defaults to a green + * `ci` check so completed-checklist scenarios pass the claim check. + * Pass a red/pending/missing set to exercise the claim-check reset paths. + */ + checkRuns?: Array<{ name: string; status: string; conclusion: string | null }>; }; /** @@ -130,6 +142,11 @@ const DEFAULT_BODY = [ "- [x] Confirm enforce-pr-target behaviour locally", ].join("\n"); +/** The repo's documented "CI passed" check, green by default. */ +const DEFAULT_GREEN_CHECKS = [ + { name: "ci", status: "completed", conclusion: "success" }, +]; + const DEFAULT_PR = { number: 42, node_id: "PR_kwDOnode42", @@ -607,6 +624,13 @@ export async function runEnforcePrTarget( createComment: (args: unknown) => respond("issues.createComment", args, { id: 99 }), updateComment: (args: unknown) => respond("issues.updateComment", args, { id: 7 }), }, + checks: { + listForRef: (args: unknown) => + respond("checks.listForRef", args, { + total_count: (options.checkRuns ?? DEFAULT_GREEN_CHECKS).length, + check_runs: options.checkRuns ?? DEFAULT_GREEN_CHECKS, + }), + }, repos: { getCollaboratorPermissionLevel: (args: unknown) => respond("repos.getCollaboratorPermissionLevel", args, { @@ -722,7 +746,7 @@ export async function runEnforcePrTarget( * runner and absent here is another `if (payload.x) return;`. */ payload = { - action: "opened", + action: options.eventAction ?? "opened", number: eventPr.number, pull_request: eventPr, repository: {