From 476688b554ea91fdacdc89e28144cd04727b03f4 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 08:14:53 +0200 Subject: [PATCH 1/9] feat(ci): reset PR readiness checklist when new commits land after completion --- .github/scripts/pr-quality.cjs | 25 +++ .github/scripts/pr-quality.test.cjs | 53 ++++++ .github/workflows/enforce-pr-target.yml | 69 ++++++- AGENTS.md | 5 +- MAINTAINERS.md | 4 + .../content/docs/contributing/pr-quality.md | 9 +- tests/ci-workflows.test.ts | 174 +++++++++++++++++- 7 files changed, 325 insertions(+), 14 deletions(-) diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index 79768fbc9b..be4282bf26 100644 --- a/.github/scripts/pr-quality.cjs +++ b/.github/scripts/pr-quality.cjs @@ -320,6 +320,30 @@ 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`. + */ +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(); + const reset = + body.slice(0, start) + section + body.slice(end + REVIEW_READINESS_END.length); + return reset.replace(/\n{3,}/g, "\n\n").trimEnd(); +} + function collectPrQualityFailures({ baseRef, allowedBases, @@ -389,6 +413,7 @@ module.exports = { extractReviewReadiness, appendReviewReadinessSection, stripReviewReadinessSection, + resetReviewReadinessSection, collectPrQualityFailures, hasEscapedNewlines, stripPrTemplateBoilerplate, diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index 4645cea988..23c06e5cb7 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -14,6 +14,7 @@ const { extractReviewReadiness, appendReviewReadinessSection, stripReviewReadinessSection, + resetReviewReadinessSection, collectPrQualityFailures, } = require("./pr-quality.cjs"); @@ -399,6 +400,58 @@ 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("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("assessPrDescription with the readiness section", () => { diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 856f55f9e1..ea3522387a 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -52,6 +52,7 @@ jobs: extractReviewReadiness, appendReviewReadinessSection, stripReviewReadinessSection, + resetReviewReadinessSection, REVIEW_READINESS_ITEMS } = require( path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), @@ -67,7 +68,10 @@ jobs: const READINESS_MARKER = ""; const READINESS_STATE_PATTERN = //; - const READINESS_STATE_VERSION = 1; + // 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; const MAINTAINERS_FILE = "MAINTAINERS.md"; const { owner, repo } = context.repo; @@ -177,7 +181,8 @@ jobs: return { version: READINESS_STATE_VERSION, autoDraftedByBot: false, - maintainersPinged: false + maintainersPinged: false, + completedAtHeadSha: null }; } @@ -580,7 +585,48 @@ 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. + // If new commits land after completion, that attestation no longer + // covers the code under review: reset the boxes and the + // notification state, re-draft, and tell the author to re-test and + // re-tick against the latest code. State written before this + // feature existed has no recorded head, so the binding only starts + // at the next completion (the completion path records the SHA). + const completionHeadSha = + storedReadinessState?.completedAtHeadSha ?? null; + const headDrifted = + checklistRequired && + checklistComplete && + completionHeadSha !== null && + completionHeadSha !== pr.head.sha; + + let readinessStateOverride = null; + let headDriftNotice = []; + if (headDrifted) { + const resetBody = resetReviewReadinessSection(pr.body ?? ""); + if (resetBody !== pr.body) { + await github.rest.pulls.update({ + owner, + repo, + pull_number, + body: resetBody + }); + } + readiness = extractReviewReadiness(resetBody); + checklistComplete = readiness.present && readiness.complete; + readinessStateOverride = { + version: READINESS_STATE_VERSION, + autoDraftedByBot: false, + maintainersPinged: false, + completedAtHeadSha: null + }; + headDriftNotice = [ + `New commits were pushed after the checklist was completed on ${inlineCode(String(completionHeadSha).slice(0, 7))}; the current head is ${inlineCode(pr.head.sha.slice(0, 7))}.`, + "The checklist has been reset: re-test against the latest code and tick all four boxes again." + ]; + } // A contributor PR stays a draft while the checklist is open, even // when every quality gate already passes. @@ -589,9 +635,11 @@ jobs: if (mustDraft) { let draftConverted = false; - const readinessState = storedReadinessState - ? { ...storedReadinessState } - : defaultReadinessState(); + const readinessState = + readinessStateOverride ?? + (storedReadinessState + ? { ...storedReadinessState } + : defaultReadinessState()); const state = storedState?.active ? { ...storedState } : { @@ -640,6 +688,7 @@ jobs: readinessState, readiness, [ + ...headDriftNotice, "This PR stays in draft until every box above is ticked." ] ); @@ -758,6 +807,7 @@ jobs: readinessState, readiness, [ + ...headDriftNotice, checklistComplete ? "✅ **All four boxes are ticked.** This PR still stays in draft until the issues above are resolved." : pr.draft || draftConverted @@ -811,6 +861,7 @@ jobs: readinessState, readiness, [ + ...headDriftNotice, 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 +981,17 @@ 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; + 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..5d19edd89c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -188,7 +188,10 @@ 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. 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..5155249bd0 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -31,6 +31,10 @@ 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. 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..0914578a66 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -46,9 +46,12 @@ 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. - **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..426eed2394 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1007,11 +1007,13 @@ 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. + // Six `pulls.update` sites: the maintainer checklist retirement, the + // checklist injection, and the checklist head-drift reset (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"], ["owner", "pull_number", "repo", "title"], @@ -1294,6 +1296,170 @@ 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([ + "graphql", + "issues.updateComment", + ])); + const readinessBody = lastReadinessCommentBody(result); + // The completion is bound to the exact head that was reviewed. + expect(readinessBody).toContain( + '"completedAtHeadSha":"3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"', + ); + 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.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(callsTo(result, "graphql").join("")).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([ + "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.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("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 From bc9147fd98d868e2ceea5cdb5b2fab0b49be757f Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 08:39:47 +0200 Subject: [PATCH 2/9] fix(ci): bind checklist completion to the attested head and recover partial resets --- .github/scripts/pr-quality.cjs | 11 ++- .github/scripts/pr-quality.test.cjs | 18 ++++ .github/workflows/enforce-pr-target.yml | 89 ++++++++++++------ tests/ci-workflows.test.ts | 119 ++++++++++++++++++++++++ 4 files changed, 207 insertions(+), 30 deletions(-) diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index be4282bf26..7e4dbdbe53 100644 --- a/.github/scripts/pr-quality.cjs +++ b/.github/scripts/pr-quality.cjs @@ -339,9 +339,14 @@ function resetReviewReadinessSection(body) { return body; } const section = buildReviewReadinessSection(); - const reset = - body.slice(0, start) + section + body.slice(end + REVIEW_READINESS_END.length); - return reset.replace(/\n{3,}/g, "\n\n").trimEnd(); + // 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({ diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index 23c06e5cb7..a880eccced 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -433,6 +433,24 @@ describe("review readiness checklist", () => { 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] "), diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index ea3522387a..642373dd10 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -588,44 +588,69 @@ jobs: let checklistComplete = readiness.present && readiness.complete; // A completed checklist is an attestation about a specific head. - // If new commits land after completion, that attestation no longer - // covers the code under review: reset the boxes and the - // notification state, re-draft, and tell the author to re-test and - // re-tick against the latest code. State written before this - // feature existed has no recorded head, so the binding only starts - // at the next completion (the completion path records the SHA). + // 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. Either way 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. State + // written before this feature existed has no recorded head, so a + // synchronize event whose head matches the live head binds + // forward at the next completion instead of retroactively + // drafting already-ready PRs. + const eventHeadSha = + context.payload.pull_request?.head?.sha ?? pr.head.sha; const completionHeadSha = storedReadinessState?.completedAtHeadSha ?? null; + const completionRecordedForLiveHead = + completionHeadSha !== null && completionHeadSha === pr.head.sha; + const ticksPredateLiveHead = + completionHeadSha === null && eventHeadSha !== pr.head.sha; const headDrifted = checklistRequired && - checklistComplete && - completionHeadSha !== null && - completionHeadSha !== pr.head.sha; + readiness.present && + ((completionHeadSha !== null && !completionRecordedForLiveHead) || + ticksPredateLiveHead); let readinessStateOverride = null; let headDriftNotice = []; if (headDrifted) { - const resetBody = resetReviewReadinessSection(pr.body ?? ""); - if (resetBody !== pr.body) { - await github.rest.pulls.update({ - owner, - repo, - pull_number, - body: resetBody - }); - } - readiness = extractReviewReadiness(resetBody); - checklistComplete = readiness.present && readiness.complete; - readinessStateOverride = { - version: READINESS_STATE_VERSION, - autoDraftedByBot: false, - maintainersPinged: false, - completedAtHeadSha: null - }; + // 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 = [ - `New commits were pushed after the checklist was completed on ${inlineCode(String(completionHeadSha).slice(0, 7))}; the current head is ${inlineCode(pr.head.sha.slice(0, 7))}.`, + completionHeadSha !== null + ? `New commits were pushed after the checklist was completed on ${inlineCode(String(completionHeadSha).slice(0, 7))}; the current head is ${inlineCode(freshPr.head.sha.slice(0, 7))}.` + : `The checklist was ticked before the current head ${inlineCode(freshPr.head.sha.slice(0, 7))} was pushed.`, "The checklist has been reset: re-test against the latest code and tick all four boxes again." ]; + 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; } // A contributor PR stays a draft while the checklist is open, even @@ -640,6 +665,15 @@ jobs: (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 } : { @@ -985,6 +1019,7 @@ jobs: // `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, diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 426eed2394..c69dc65fb3 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1324,6 +1324,8 @@ describe("GitHub Actions hardening", () => { 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'); @@ -1352,6 +1354,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "pulls.get", "pulls.update", "issues.updateComment", "graphql", @@ -1437,6 +1440,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsWrongBase([ + "pulls.get", "pulls.update", "issues.updateComment", "issues.createComment", @@ -1460,6 +1464,121 @@ describe("GitHub Actions hardening", () => { ); }); + 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(callsTo(result, "graphql").join("")).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 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("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 From 387da10055a3776912130d7c8a43b03d9bf488e5 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 08:52:14 +0200 Subject: [PATCH 3/9] fix(ci): only reject completions when the checklist is actually ticked --- .github/workflows/enforce-pr-target.yml | 4 ++- tests/ci-workflows.test.ts | 42 +++++++++++++++++++++---- 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 642373dd10..bb555225c8 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -607,7 +607,9 @@ jobs: const completionRecordedForLiveHead = completionHeadSha !== null && completionHeadSha === pr.head.sha; const ticksPredateLiveHead = - completionHeadSha === null && eventHeadSha !== pr.head.sha; + completionHeadSha === null && + checklistComplete && + eventHeadSha !== pr.head.sha; const headDrifted = checklistRequired && readiness.present && diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index c69dc65fb3..7656659430 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1369,9 +1369,7 @@ describe("GitHub Actions hardening", () => { const drafts = callsTo(result, "graphql") as [{ query: string }]; expect(drafts).toHaveLength(1); expect(drafts[0]!.query).toContain("convertPullRequestToDraft"); - expect(callsTo(result, "graphql").join("")).not.toContain( - "markPullRequestReadyForReview", - ); + expect(drafts[0]!.query).not.toContain("markPullRequestReadyForReview"); const readinessBody = lastReadinessCommentBody(result); expect(readinessBody).toContain("**0/4** boxes ticked"); @@ -1492,9 +1490,7 @@ describe("GitHub Actions hardening", () => { const drafts = callsTo(result, "graphql") as [{ query: string }]; expect(drafts).toHaveLength(1); expect(drafts[0]!.query).toContain("convertPullRequestToDraft"); - expect(callsTo(result, "graphql").join("")).not.toContain( - "markPullRequestReadyForReview", - ); + 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.", @@ -1579,6 +1575,40 @@ describe("GitHub Actions hardening", () => { 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 From ccc109886db9d47b637c6a362e8319ce545978d7 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 09:11:32 +0200 Subject: [PATCH 4/9] refactor(ci): split enforce-target inline script into focused modules --- .../scripts/enforce-pr-target-maintainers.cjs | 31 ++ .../enforce-pr-target-maintainers.test.cjs | 51 +++ .../scripts/enforce-pr-target-messages.cjs | 173 +++++++++ .../enforce-pr-target-messages.test.cjs | 159 ++++++++ .github/scripts/enforce-pr-target-state.cjs | 148 ++++++++ .../scripts/enforce-pr-target-state.test.cjs | 177 +++++++++ .github/workflows/enforce-pr-target.yml | 357 ++++-------------- tests/ci-workflows.test.ts | 20 +- 8 files changed, 832 insertions(+), 284 deletions(-) create mode 100644 .github/scripts/enforce-pr-target-maintainers.cjs create mode 100644 .github/scripts/enforce-pr-target-maintainers.test.cjs create mode 100644 .github/scripts/enforce-pr-target-messages.cjs create mode 100644 .github/scripts/enforce-pr-target-messages.test.cjs create mode 100644 .github/scripts/enforce-pr-target-state.cjs create mode 100644 .github/scripts/enforce-pr-target-state.test.cjs diff --git a/.github/scripts/enforce-pr-target-maintainers.cjs b/.github/scripts/enforce-pr-target-maintainers.cjs new file mode 100644 index 0000000000..6abf051a0b --- /dev/null +++ b/.github/scripts/enforce-pr-target-maintainers.cjs @@ -0,0 +1,31 @@ +"use strict"; + +/** + * Maintainers from `MAINTAINERS.md` text. Only the current-maintainers table + * is authoritative; the change log below it can mention retired accounts. + */ +function parseMaintainerLogins(text) { + 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)]; +} + +module.exports = { + parseMaintainerLogins +}; diff --git a/.github/scripts/enforce-pr-target-maintainers.test.cjs b/.github/scripts/enforce-pr-target-maintainers.test.cjs new file mode 100644 index 0000000000..26426df4a0 --- /dev/null +++ b/.github/scripts/enforce-pr-target-maintainers.test.cjs @@ -0,0 +1,51 @@ +"use strict"; + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { + parseMaintainerLogins +} = require("./enforce-pr-target-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 the whole text when the section heading is missing", () => { + const text = "- [@only](https://github.com/only) is listed."; + assert.deepEqual(parseMaintainerLogins(text), ["only"]); + }); + + 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/enforce-pr-target-messages.cjs b/.github/scripts/enforce-pr-target-messages.cjs new file mode 100644 index 0000000000..00aa82c3e3 --- /dev/null +++ b/.github/scripts/enforce-pr-target-messages.cjs @@ -0,0 +1,173 @@ +"use strict"; + +const { + REVIEW_READINESS_ITEMS +} = require("./pr-quality.cjs"); +const { + readinessStateMarker +} = require("./enforce-pr-target-state.cjs"); + +/** Marks the bot's review-readiness checklist message. */ +const READINESS_MARKER = ""; + +function inlineCode(value) { + return `\`${String(value).replaceAll("`", "\\`")}\``; +} + +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." + : "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 + ]; +} + +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 reset notice shown when a completion no longer covers the live head. */ +function buildStaleNotice({ completionHeadSha, liveHeadSha }) { + return [ + completionHeadSha !== null + ? `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))}.` + : `The checklist was ticked before the current head ${inlineCode(liveHeadSha.slice(0, 7))} was pushed.`, + "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 +}; diff --git a/.github/scripts/enforce-pr-target-messages.test.cjs b/.github/scripts/enforce-pr-target-messages.test.cjs new file mode 100644 index 0000000000..db2361b2d9 --- /dev/null +++ b/.github/scripts/enforce-pr-target-messages.test.cjs @@ -0,0 +1,159 @@ +"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 +} = require("./enforce-pr-target-messages.cjs"); + +const PR = { + base: { ref: "main" }, + user: { login: "contributor" } +}; +const ALLOWED_BASES = ["dev"]; +const DEFAULT_BASE = "dev"; + +describe("inlineCode", () => { + it("wraps values in backticks and escapes embedded backticks", () => { + assert.equal(inlineCode("dev"), "`dev`"); + 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; + +/** 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. + */ +function completionIsStale({ + checklistRequired, + checklistComplete, + readinessPresent, + completionHeadSha, + eventHeadSha, + liveHeadSha +}) { + const completionRecordedForLiveHead = + completionHeadSha !== null && completionHeadSha === liveHeadSha; + const ticksPredateLiveHead = + completionHeadSha === null && + checklistComplete && + eventHeadSha !== liveHeadSha; + + return ( + checklistRequired && + readinessPresent && + ((completionHeadSha !== null && !completionRecordedForLiveHead) || + ticksPredateLiveHead) + ); +} + +module.exports = { + STATE_PATTERN, + READINESS_STATE_PATTERN, + READINESS_STATE_VERSION, + parseState, + stateMarker, + parseReadinessState, + readinessStateMarker, + clearedEnforcerState, + defaultEnforcerState, + defaultReadinessState, + completionIsStale +}; diff --git a/.github/scripts/enforce-pr-target-state.test.cjs b/.github/scripts/enforce-pr-target-state.test.cjs new file mode 100644 index 0000000000..dab994a16e --- /dev/null +++ b/.github/scripts/enforce-pr-target-state.test.cjs @@ -0,0 +1,177 @@ +"use strict"; + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { + parseState, + stateMarker, + parseReadinessState, + readinessStateMarker, + clearedEnforcerState, + defaultEnforcerState, + defaultReadinessState, + completionIsStale, + READINESS_STATE_VERSION +} = require("./enforce-pr-target-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 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, + ); + }); +}); diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index bb555225c8..82a0fe08f0 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -52,26 +52,58 @@ jobs: extractReviewReadiness, appendReviewReadinessSection, stripReviewReadinessSection, - resetReviewReadinessSection, - REVIEW_READINESS_ITEMS + resetReviewReadinessSection } = require( path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), ); + const { + parseState, + stateMarker, + parseReadinessState, + clearedEnforcerState, + defaultEnforcerState, + defaultReadinessState, + completionIsStale, + READINESS_STATE_VERSION + } = require( + path.join( + process.cwd(), + ".github", + "scripts", + "enforce-pr-target-state.cjs" + ), + ); + const { + READINESS_MARKER, + inlineCode, + buildReadinessCommentBody, + buildFailureSections, + failureSummary, + buildStaleNotice + } = require( + path.join( + process.cwd(), + ".github", + "scripts", + "enforce-pr-target-messages.cjs" + ), + ); + const { + parseMaintainerLogins + } = require( + path.join( + process.cwd(), + ".github", + "scripts", + "enforce-pr-target-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 = - //; - // 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; const MAINTAINERS_FILE = "MAINTAINERS.md"; const { owner, repo } = context.repo; @@ -108,84 +140,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, - completedAtHeadSha: null - }; - } - /** * Maintainers from `MAINTAINERS.md` on the trusted default branch * (checked out sparse by the step above). The file is the canonical @@ -197,27 +155,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}` @@ -227,35 +165,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({ @@ -277,10 +188,6 @@ jobs: readinessCommentId = created.data.id; } - function inlineCode(value) { - return `\`${String(value).replaceAll("`", "\\`")}\``; - } - async function upsertComment(body) { if (botCommentId) { await github.rest.issues.updateComment({ @@ -346,108 +253,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; @@ -587,34 +396,22 @@ jobs: } let checklistComplete = readiness.present && readiness.complete; - // 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. Either way the gate resets the + // 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. State - // written before this feature existed has no recorded head, so a - // synchronize event whose head matches the live head binds - // forward at the next completion instead of retroactively - // drafting already-ready PRs. + // 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 completionRecordedForLiveHead = - completionHeadSha !== null && completionHeadSha === pr.head.sha; - const ticksPredateLiveHead = - completionHeadSha === null && - checklistComplete && - eventHeadSha !== pr.head.sha; - const headDrifted = - checklistRequired && - readiness.present && - ((completionHeadSha !== null && !completionRecordedForLiveHead) || - ticksPredateLiveHead); + const headDrifted = completionIsStale({ + checklistRequired, + checklistComplete, + readinessPresent: readiness.present, + completionHeadSha, + eventHeadSha, + liveHeadSha: pr.head.sha + }); let readinessStateOverride = null; let headDriftNotice = []; @@ -630,12 +427,10 @@ jobs: freshPr.body ?? "" ); readinessStateOverride = defaultReadinessState(); - headDriftNotice = [ - completionHeadSha !== null - ? `New commits were pushed after the checklist was completed on ${inlineCode(String(completionHeadSha).slice(0, 7))}; the current head is ${inlineCode(freshPr.head.sha.slice(0, 7))}.` - : `The checklist was ticked before the current head ${inlineCode(freshPr.head.sha.slice(0, 7))} was pushed.`, - "The checklist has been reset: re-test against the latest code and tick all four boxes again." - ]; + headDriftNotice = buildStaleNotice({ + completionHeadSha, + liveHeadSha: freshPr.head.sha + }); if (freshReadiness.present && freshReadiness.complete) { const resetBody = resetReviewReadinessSection( freshPr.body ?? "" @@ -678,15 +473,7 @@ jobs: } 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" ); @@ -742,7 +529,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( @@ -853,7 +644,9 @@ jobs: ); } - core.setFailed(`PR quality gate failed: ${failureSummary(failures)}`); + core.setFailed( + `PR quality gate failed: ${failureSummary(failures, { pr })}` + ); return; } diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 7656659430..b6cd180f18 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[] { @@ -3355,10 +3362,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/enforce-pr-target-messages.cjs", + ); + expect(messagesModule).toMatch( + /READINESS_MARKER = ""/, + ); + expect(script).toMatch(/enforce-pr-target-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) {"); From dfaa8fd7f38ad00b8343eaeec7965827de8fe2d6 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 09:54:32 +0200 Subject: [PATCH 5/9] refactor(ci): name PR-quality modules by responsibility Drop the enforce-pr-target-* prefix from the split helpers so they match the existing pr-quality*.cjs naming and describe what they own. --- ...enforce-pr-target-maintainers.cjs => pr-maintainers.cjs} | 0 ...-target-maintainers.test.cjs => pr-maintainers.test.cjs} | 2 +- ...force-pr-target-messages.cjs => pr-quality-messages.cjs} | 2 +- ...arget-messages.test.cjs => pr-quality-messages.test.cjs} | 2 +- .../{enforce-pr-target-state.cjs => pr-quality-state.cjs} | 0 ...e-pr-target-state.test.cjs => pr-quality-state.test.cjs} | 2 +- .github/workflows/enforce-pr-target.yml | 6 +++--- tests/ci-workflows.test.ts | 4 ++-- 8 files changed, 9 insertions(+), 9 deletions(-) rename .github/scripts/{enforce-pr-target-maintainers.cjs => pr-maintainers.cjs} (100%) rename .github/scripts/{enforce-pr-target-maintainers.test.cjs => pr-maintainers.test.cjs} (96%) rename .github/scripts/{enforce-pr-target-messages.cjs => pr-quality-messages.cjs} (99%) rename .github/scripts/{enforce-pr-target-messages.test.cjs => pr-quality-messages.test.cjs} (99%) rename .github/scripts/{enforce-pr-target-state.cjs => pr-quality-state.cjs} (100%) rename .github/scripts/{enforce-pr-target-state.test.cjs => pr-quality-state.test.cjs} (99%) diff --git a/.github/scripts/enforce-pr-target-maintainers.cjs b/.github/scripts/pr-maintainers.cjs similarity index 100% rename from .github/scripts/enforce-pr-target-maintainers.cjs rename to .github/scripts/pr-maintainers.cjs diff --git a/.github/scripts/enforce-pr-target-maintainers.test.cjs b/.github/scripts/pr-maintainers.test.cjs similarity index 96% rename from .github/scripts/enforce-pr-target-maintainers.test.cjs rename to .github/scripts/pr-maintainers.test.cjs index 26426df4a0..c5b32b6478 100644 --- a/.github/scripts/enforce-pr-target-maintainers.test.cjs +++ b/.github/scripts/pr-maintainers.test.cjs @@ -4,7 +4,7 @@ const { describe, it } = require("node:test"); const assert = require("node:assert/strict"); const { parseMaintainerLogins -} = require("./enforce-pr-target-maintainers.cjs"); +} = require("./pr-maintainers.cjs"); const FIXTURE = [ "## Current maintainers", diff --git a/.github/scripts/enforce-pr-target-messages.cjs b/.github/scripts/pr-quality-messages.cjs similarity index 99% rename from .github/scripts/enforce-pr-target-messages.cjs rename to .github/scripts/pr-quality-messages.cjs index 00aa82c3e3..9f29e592b5 100644 --- a/.github/scripts/enforce-pr-target-messages.cjs +++ b/.github/scripts/pr-quality-messages.cjs @@ -5,7 +5,7 @@ const { } = require("./pr-quality.cjs"); const { readinessStateMarker -} = require("./enforce-pr-target-state.cjs"); +} = require("./pr-quality-state.cjs"); /** Marks the bot's review-readiness checklist message. */ const READINESS_MARKER = ""; diff --git a/.github/scripts/enforce-pr-target-messages.test.cjs b/.github/scripts/pr-quality-messages.test.cjs similarity index 99% rename from .github/scripts/enforce-pr-target-messages.test.cjs rename to .github/scripts/pr-quality-messages.test.cjs index db2361b2d9..b9193d9949 100644 --- a/.github/scripts/enforce-pr-target-messages.test.cjs +++ b/.github/scripts/pr-quality-messages.test.cjs @@ -14,7 +14,7 @@ const { buildFailureSections, failureSummary, buildStaleNotice -} = require("./enforce-pr-target-messages.cjs"); +} = require("./pr-quality-messages.cjs"); const PR = { base: { ref: "main" }, diff --git a/.github/scripts/enforce-pr-target-state.cjs b/.github/scripts/pr-quality-state.cjs similarity index 100% rename from .github/scripts/enforce-pr-target-state.cjs rename to .github/scripts/pr-quality-state.cjs diff --git a/.github/scripts/enforce-pr-target-state.test.cjs b/.github/scripts/pr-quality-state.test.cjs similarity index 99% rename from .github/scripts/enforce-pr-target-state.test.cjs rename to .github/scripts/pr-quality-state.test.cjs index dab994a16e..dd7f10a6f5 100644 --- a/.github/scripts/enforce-pr-target-state.test.cjs +++ b/.github/scripts/pr-quality-state.test.cjs @@ -12,7 +12,7 @@ const { defaultReadinessState, completionIsStale, READINESS_STATE_VERSION -} = require("./enforce-pr-target-state.cjs"); +} = require("./pr-quality-state.cjs"); describe("enforcer state markers", () => { it("parses a valid enforcer state marker", () => { diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 82a0fe08f0..457bf2a905 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -70,7 +70,7 @@ jobs: process.cwd(), ".github", "scripts", - "enforce-pr-target-state.cjs" + "pr-quality-state.cjs" ), ); const { @@ -85,7 +85,7 @@ jobs: process.cwd(), ".github", "scripts", - "enforce-pr-target-messages.cjs" + "pr-quality-messages.cjs" ), ); const { @@ -95,7 +95,7 @@ jobs: process.cwd(), ".github", "scripts", - "enforce-pr-target-maintainers.cjs" + "pr-maintainers.cjs" ), ); diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index b6cd180f18..c92ba64256 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -3368,12 +3368,12 @@ describe("GitHub Actions hardening", () => { // 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/enforce-pr-target-messages.cjs", + ".github/scripts/pr-quality-messages.cjs", ); expect(messagesModule).toMatch( /READINESS_MARKER = ""/, ); - expect(script).toMatch(/enforce-pr-target-messages\.cjs/); + 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). From bbb20a272cf88d174539ccacaf0b527e1c60f41f Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 10:01:05 +0200 Subject: [PATCH 6/9] fix(ci): harden readiness split helpers and synchronize provenance Fail closed when MAINTAINERS.md lacks the current-maintainers section, render code spans with a safe delimiter, stop claiming readiness from checklist absence, and reject unrecorded complete checklists on synchronize so a push cannot inherit a still-queued attestation. --- .github/scripts/pr-maintainers.cjs | 17 +++++---- .github/scripts/pr-maintainers.test.cjs | 4 +-- .github/scripts/pr-quality-messages.cjs | 24 +++++++++---- .github/scripts/pr-quality-messages.test.cjs | 26 +++++++++++--- .github/scripts/pr-quality-state.cjs | 19 +++++++++-- .github/scripts/pr-quality-state.test.cjs | 26 ++++++++++++++ .github/workflows/enforce-pr-target.yml | 6 ++-- tests/ci-workflows.test.ts | 36 ++++++++++++++++++++ tests/helpers/enforce-pr-target-harness.ts | 8 ++++- 9 files changed, 140 insertions(+), 26 deletions(-) diff --git a/.github/scripts/pr-maintainers.cjs b/.github/scripts/pr-maintainers.cjs index 6abf051a0b..b53ef450be 100644 --- a/.github/scripts/pr-maintainers.cjs +++ b/.github/scripts/pr-maintainers.cjs @@ -3,20 +3,23 @@ /** * 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 sectionStart = text.indexOf("## Current maintainers"); + if (sectionStart === -1) { + return []; + } + const nextHeading = text.indexOf( "\n## ", sectionStart + "## Current maintainers".length ); - const section = - sectionStart === -1 - ? text - : text.slice( - sectionStart, - nextHeading === -1 ? text.length : nextHeading - ); + const section = text.slice( + sectionStart, + nextHeading === -1 ? text.length : nextHeading + ); const logins = [ ...section.matchAll( /\[\@([A-Za-z0-9_-]+)\]\(https:\/\/github\.com\/[^)]*\)/g diff --git a/.github/scripts/pr-maintainers.test.cjs b/.github/scripts/pr-maintainers.test.cjs index c5b32b6478..08584257a6 100644 --- a/.github/scripts/pr-maintainers.test.cjs +++ b/.github/scripts/pr-maintainers.test.cjs @@ -30,9 +30,9 @@ describe("parseMaintainerLogins", () => { ]); }); - it("returns the whole text when the section heading is missing", () => { + it("returns an empty list when the section heading is missing", () => { const text = "- [@only](https://github.com/only) is listed."; - assert.deepEqual(parseMaintainerLogins(text), ["only"]); + assert.deepEqual(parseMaintainerLogins(text), []); }); it("handles empty and duplicate-free output", () => { diff --git a/.github/scripts/pr-quality-messages.cjs b/.github/scripts/pr-quality-messages.cjs index 9f29e592b5..ea4d887ffc 100644 --- a/.github/scripts/pr-quality-messages.cjs +++ b/.github/scripts/pr-quality-messages.cjs @@ -11,7 +11,13 @@ const { const READINESS_MARKER = ""; function inlineCode(value) { - return `\`${String(value).replaceAll("`", "\\`")}\``; + 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) { @@ -36,7 +42,7 @@ function buildReadinessCommentBody(state, readiness, extra) { "", 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.", + : "The review readiness checklist is not required for this author.", "", ...(readiness.present ? readinessChecklistLines(readiness) : []), "", @@ -152,11 +158,17 @@ function failureSummary(failures, { pr }) { } /** The reset notice shown when a completion no longer covers the live head. */ -function buildStaleNotice({ completionHeadSha, liveHeadSha }) { +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 [ - completionHeadSha !== null - ? `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))}.` - : `The checklist was ticked before the current head ${inlineCode(liveHeadSha.slice(0, 7))} was pushed.`, + lead, "The checklist has been reset: re-test against the latest code and tick all four boxes again." ]; } diff --git a/.github/scripts/pr-quality-messages.test.cjs b/.github/scripts/pr-quality-messages.test.cjs index b9193d9949..1ad425243b 100644 --- a/.github/scripts/pr-quality-messages.test.cjs +++ b/.github/scripts/pr-quality-messages.test.cjs @@ -24,9 +24,10 @@ const ALLOWED_BASES = ["dev"]; const DEFAULT_BASE = "dev"; describe("inlineCode", () => { - it("wraps values in backticks and escapes embedded backticks", () => { + 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("a``b"), "```a``b```"); assert.equal(inlineCode(42), "`42`"); }); }); @@ -62,13 +63,15 @@ describe("buildReadinessCommentBody", () => { assert.ok(body.includes("tick all four boxes there.")); }); - it("renders the ready-for-review variant when the checklist is absent", () => { + it("states the checklist is not required without claiming the PR is ready", () => { const body = buildReadinessCommentBody( { version: 2 }, { present: false, complete: false, checked: 0, total: 0, items: [] }, - [], + ["⚠️ **Wrong target branch**"], ).join("\n"); assert.ok(body.includes("not required for this author.")); + assert.ok(!body.includes("This PR is ready for review")); + assert.ok(body.includes("⚠️ **Wrong target branch**")); assert.ok(!body.includes("boxes ticked")); }); }); @@ -147,7 +150,20 @@ describe("buildStaleNotice", () => { completionHeadSha: null, liveHeadSha: "2222222222222222222222222222222222222222" }); - assert.match(notice[0], /ticked before the current head `2222222` was pushed/); + assert.ok(notice[0].includes("ticked before the current head `2222222` was pushed")); + }); + + it("covers an unrecorded complete checklist on synchronize", () => { + const notice = buildStaleNotice({ + completionHeadSha: null, + liveHeadSha: "2222222222222222222222222222222222222222", + eventAction: "synchronize" + }); + assert.match( + notice[0], + /synchronize event with no recorded completion head/, + ); + assert.ok(notice[0].includes("current head is `2222222`")); }); it("matches the injected section text it resets", () => { diff --git a/.github/scripts/pr-quality-state.cjs b/.github/scripts/pr-quality-state.cjs index e9a0a7ccaf..68c1f718c6 100644 --- a/.github/scripts/pr-quality-state.cjs +++ b/.github/scripts/pr-quality-state.cjs @@ -108,7 +108,9 @@ function defaultReadinessState() { * 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. + * 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). */ function completionIsStale({ checklistRequired, @@ -116,20 +118,31 @@ function completionIsStale({ readinessPresent, completionHeadSha, eventHeadSha, - liveHeadSha + 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) + ticksPredateLiveHead || + unrecordedCompleteOnSynchronize) ); } diff --git a/.github/scripts/pr-quality-state.test.cjs b/.github/scripts/pr-quality-state.test.cjs index dd7f10a6f5..c70bb3f9b1 100644 --- a/.github/scripts/pr-quality-state.test.cjs +++ b/.github/scripts/pr-quality-state.test.cjs @@ -151,6 +151,32 @@ describe("completionIsStale", () => { 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( diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 457bf2a905..0451099698 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -410,7 +410,8 @@ jobs: readinessPresent: readiness.present, completionHeadSha, eventHeadSha, - liveHeadSha: pr.head.sha + liveHeadSha: pr.head.sha, + eventAction: context.payload.action }); let readinessStateOverride = null; @@ -429,7 +430,8 @@ jobs: readinessStateOverride = defaultReadinessState(); headDriftNotice = buildStaleNotice({ completionHeadSha, - liveHeadSha: freshPr.head.sha + liveHeadSha: freshPr.head.sha, + eventAction: context.payload.action }); if (freshReadiness.present && freshReadiness.complete) { const resetBody = resetReviewReadinessSection( diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index c92ba64256..37b842b61f 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1509,6 +1509,42 @@ describe("GitHub Actions hardening", () => { 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 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 diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index 45f3f07808..9229856cff 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 @@ -722,7 +728,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: { From c9c63094bb3b195f174bb9169822e9a79a852576 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:25:46 +0200 Subject: [PATCH 7/9] fix(ci): match exact H2 heading in maintainer parser --- .github/scripts/pr-maintainers.cjs | 5 +++-- .github/scripts/pr-maintainers.test.cjs | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/.github/scripts/pr-maintainers.cjs b/.github/scripts/pr-maintainers.cjs index b53ef450be..d96cdc6810 100644 --- a/.github/scripts/pr-maintainers.cjs +++ b/.github/scripts/pr-maintainers.cjs @@ -7,11 +7,12 @@ * so the recipient list is empty rather than scanning the whole file. */ function parseMaintainerLogins(text) { - const sectionStart = text.indexOf("## Current maintainers"); - if (sectionStart === -1) { + 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 diff --git a/.github/scripts/pr-maintainers.test.cjs b/.github/scripts/pr-maintainers.test.cjs index 08584257a6..81082d54fa 100644 --- a/.github/scripts/pr-maintainers.test.cjs +++ b/.github/scripts/pr-maintainers.test.cjs @@ -34,6 +34,20 @@ describe("parseMaintainerLogins", () => { 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("handles empty and duplicate-free output", () => { assert.deepEqual(parseMaintainerLogins(""), []); From cc48263c5148f058b389c0fec8b1d58e0689877c Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:32:18 +0200 Subject: [PATCH 8/9] test(ci): cover valid CRLF maintainers heading --- .github/scripts/pr-maintainers.test.cjs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/scripts/pr-maintainers.test.cjs b/.github/scripts/pr-maintainers.test.cjs index 81082d54fa..20da1896ac 100644 --- a/.github/scripts/pr-maintainers.test.cjs +++ b/.github/scripts/pr-maintainers.test.cjs @@ -48,6 +48,16 @@ describe("parseMaintainerLogins", () => { 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(""), []); From b1a59df61214548fd8aaaa4c28c7cf3be8166bf6 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:53:46 +0200 Subject: [PATCH 9/9] feat(ci): verify checklist claims on completion before lifting the draft The gate now checks two of the four readiness boxes itself instead of trusting the author's ticks: the head's `ci` check must be green, and the branch must be on the latest dev or at most 10 commits behind it. A disproved claim unchecks the matching box, clears the completion attestation and maintainer ping, and keeps the PR a draft with an explanatory notice. Unknown CI/compare state fails closed. --- .github/scripts/pr-quality-messages.cjs | 26 +- .github/scripts/pr-quality-messages.test.cjs | 31 ++- .github/scripts/pr-quality-state.cjs | 31 +++ .github/scripts/pr-quality-state.test.cjs | 60 +++++ .github/scripts/pr-quality.cjs | 56 ++++ .github/scripts/pr-quality.test.cjs | 66 +++++ .github/workflows/enforce-pr-target.yml | 102 +++++++- AGENTS.md | 4 + MAINTAINERS.md | 4 + .../content/docs/contributing/pr-quality.md | 4 + tests/ci-workflows.test.ts | 241 +++++++++++++++++- tests/helpers/enforce-pr-target-harness.ts | 18 ++ 12 files changed, 630 insertions(+), 13 deletions(-) diff --git a/.github/scripts/pr-quality-messages.cjs b/.github/scripts/pr-quality-messages.cjs index ea4d887ffc..e42d31c374 100644 --- a/.github/scripts/pr-quality-messages.cjs +++ b/.github/scripts/pr-quality-messages.cjs @@ -4,7 +4,8 @@ const { REVIEW_READINESS_ITEMS } = require("./pr-quality.cjs"); const { - readinessStateMarker + readinessStateMarker, + READINESS_LATEST_DEV_BEHIND_MAX } = require("./pr-quality-state.cjs"); /** Marks the bot's review-readiness checklist message. */ @@ -157,6 +158,26 @@ function failureSummary(failures, { pr }) { .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; @@ -181,5 +202,6 @@ module.exports = { descriptionFailureLines, buildFailureSections, failureSummary, - buildStaleNotice + buildStaleNotice, + buildClaimCheckNotice }; diff --git a/.github/scripts/pr-quality-messages.test.cjs b/.github/scripts/pr-quality-messages.test.cjs index 1ad425243b..893f9173ec 100644 --- a/.github/scripts/pr-quality-messages.test.cjs +++ b/.github/scripts/pr-quality-messages.test.cjs @@ -13,7 +13,8 @@ const { descriptionFailureLines, buildFailureSections, failureSummary, - buildStaleNotice + buildStaleNotice, + buildClaimCheckNotice } = require("./pr-quality-messages.cjs"); const PR = { @@ -173,3 +174,31 @@ describe("buildStaleNotice", () => { assert.match(section, /\[ \] All CI tests are green on my local testing\./); }); }); + +describe("buildClaimCheckNotice", () => { + it("names each violated claim and the reset action", () => { + const notice = buildClaimCheckNotice( + ["ci_green", "latest_dev"], + "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b", + ); + assert.match(notice[0], /CI is not green on the current head `3f1c0de`/); + assert.match(notice[0], /\*\*CI green\*\* box has been unticked/); + assert.match(notice[1], /more than 10 commits behind `dev`/); + assert.match(notice[1], /\*\*latest dev\*\* box has been unticked/); + assert.match(notice[2], /reset: re-test against the latest code/); + }); + + it("handles a single violation", () => { + const notice = buildClaimCheckNotice(["ci_green"], "a".repeat(40)); + assert.equal(notice.length, 2); + assert.match(notice[0], /CI is not green/); + assert.match(notice[1], /has been reset/); + }); + + it("returns only the reset line for an empty violation list", () => { + const notice = buildClaimCheckNotice([], "a".repeat(40)); + assert.deepEqual(notice, [ + "The checklist has been reset: re-test against the latest code and tick the boxes again.", + ]); + }); +}); diff --git a/.github/scripts/pr-quality-state.cjs b/.github/scripts/pr-quality-state.cjs index 68c1f718c6..f8a9072269 100644 --- a/.github/scripts/pr-quality-state.cjs +++ b/.github/scripts/pr-quality-state.cjs @@ -14,6 +14,10 @@ const READINESS_STATE_PATTERN = */ 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); @@ -112,6 +116,31 @@ function defaultReadinessState() { * 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, @@ -147,6 +176,8 @@ function completionIsStale({ } module.exports = { + READINESS_LATEST_DEV_BEHIND_MAX, + readinessClaimViolations, STATE_PATTERN, READINESS_STATE_PATTERN, READINESS_STATE_VERSION, diff --git a/.github/scripts/pr-quality-state.test.cjs b/.github/scripts/pr-quality-state.test.cjs index c70bb3f9b1..e3b63fd5c2 100644 --- a/.github/scripts/pr-quality-state.test.cjs +++ b/.github/scripts/pr-quality-state.test.cjs @@ -11,6 +11,8 @@ const { defaultEnforcerState, defaultReadinessState, completionIsStale, + readinessClaimViolations, + READINESS_LATEST_DEV_BEHIND_MAX, READINESS_STATE_VERSION } = require("./pr-quality-state.cjs"); @@ -201,3 +203,61 @@ describe("completionIsStale", () => { ); }); }); + +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 7e4dbdbe53..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. @@ -327,6 +337,50 @@ function stripReviewReadinessSection(body) { * 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); @@ -418,6 +472,8 @@ module.exports = { extractReviewReadiness, appendReviewReadinessSection, stripReviewReadinessSection, + REVIEW_READINESS_CLAIM_INDEX, + uncheckReviewReadinessBoxes, resetReviewReadinessSection, collectPrQualityFailures, hasEscapedNewlines, diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index a880eccced..6bf40964eb 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -14,6 +14,8 @@ const { extractReviewReadiness, appendReviewReadinessSection, stripReviewReadinessSection, + uncheckReviewReadinessBoxes, + REVIEW_READINESS_CLAIM_INDEX, resetReviewReadinessSection, collectPrQualityFailures, } = require("./pr-quality.cjs"); @@ -472,6 +474,70 @@ describe("review readiness checklist", () => { }); }); +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", () => { const SUBSTANTIAL = [ "## Summary", diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 0451099698..94023e7d0a 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -52,6 +52,8 @@ jobs: extractReviewReadiness, appendReviewReadinessSection, stripReviewReadinessSection, + uncheckReviewReadinessBoxes, + REVIEW_READINESS_CLAIM_INDEX, resetReviewReadinessSection } = require( path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), @@ -64,6 +66,7 @@ jobs: defaultEnforcerState, defaultReadinessState, completionIsStale, + readinessClaimViolations, READINESS_STATE_VERSION } = require( path.join( @@ -79,7 +82,8 @@ jobs: buildReadinessCommentBody, buildFailureSections, failureSummary, - buildStaleNotice + buildStaleNotice, + buildClaimCheckNotice } = require( path.join( process.cwd(), @@ -416,6 +420,7 @@ jobs: 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. @@ -452,11 +457,100 @@ jobs: 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 = @@ -513,7 +607,7 @@ jobs: readinessState, readiness, [ - ...headDriftNotice, + ...revalidationNotice, "This PR stays in draft until every box above is ticked." ] ); @@ -636,7 +730,7 @@ jobs: readinessState, readiness, [ - ...headDriftNotice, + ...revalidationNotice, checklistComplete ? "✅ **All four boxes are ticked.** This PR still stays in draft until the issues above are resolved." : pr.draft || draftConverted @@ -692,7 +786,7 @@ jobs: readinessState, readiness, [ - ...headDriftNotice, + ...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." diff --git a/AGENTS.md b/AGENTS.md index 5d19edd89c..060fa06b9a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -192,6 +192,10 @@ 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 5155249bd0..81e57197bc 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -35,6 +35,10 @@ see [The retired `dev2-go` line](#the-retired-dev2-go-line). 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 0914578a66..1bc5ac3958 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -52,6 +52,10 @@ tells you exactly what to change: 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 37b842b61f..2ca6f48f88 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1014,15 +1014,16 @@ describe("GitHub Actions hardening", () => { return found; } - // Six `pulls.update` sites: the maintainer checklist retirement, the - // checklist injection, and the checklist head-drift reset (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"], ["owner", "pull_number", "repo", "title"], ["owner", "pull_number", "repo", "title"], @@ -1054,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", @@ -1291,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", ])); @@ -1323,6 +1327,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "issues.updateComment", ])); @@ -1411,6 +1416,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "issues.updateComment", ])); expect(callsTo(result, "pulls.update")).toEqual([]); @@ -1545,6 +1551,219 @@ describe("GitHub Actions hardening", () => { 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 @@ -1825,6 +2044,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "issues.updateComment", "issues.createComment", @@ -2085,6 +2305,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "graphql", "issues.updateComment", "issues.createComment", @@ -2145,6 +2366,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "pulls.update", "graphql", "issues.updateComment", @@ -2459,6 +2681,7 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", "pulls.update", "graphql", "issues.updateComment", @@ -2610,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", @@ -2734,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", @@ -2827,6 +3052,7 @@ describe("GitHub Actions hardening", () => { comments: [botComment(active)], }); expect(methodsOf(restored)).toEqual(readsAllowedBase([ + "checks.listForRef", "pulls.update", "graphql", "issues.updateComment", @@ -2886,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", @@ -2907,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", @@ -3100,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", diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index 9229856cff..4527404c44 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -115,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 }>; }; /** @@ -136,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", @@ -613,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, {