From 0106235a915cf73ea29896866e18338150e923cd Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 04:20:10 +0200 Subject: [PATCH 1/5] feat(ci): harden PR gate with consolidated comment, draft-review trigger, and GUI waiver Consolidate the PR gate's two bot comments (enforcer + readiness) into one always-present, always-edited comment that carries the current status, the "what to do" actions, the readiness-checklist mirror, and the draft reason. Legacy two-comment PRs migrate their state into the single comment and the old comments are deleted. State fields are read for truthiness, matching the pre-consolidation gate. Trigger CodeRabbit/Codex review at the ready moment via a review-ready label (.coderabbit.yaml auto_review.labels), so a ready-but-draft PR gets reviewed without a manual @coderabbitai review. The findings claim verifies review threads primarily and supplements with CodeRabbit review-body findings that fall outside the diff range; both are head-bound and fail closed. Allow a Collaborator/Owner issue comment (e.g. "not touching gui") to waive the GUI-screenshot gate; the author cannot self-waive. Tests cover the consolidated comment, label management, migration, the outside-diff supplement, and the GUI waiver. Co-authored-by: CommandCodeBot --- .coderabbit.yaml | 7 + .github/scripts/enforce-pr-target.test.cjs | 45 + .github/scripts/pr-quality-messages.cjs | 104 +- .github/scripts/pr-quality-messages.test.cjs | 88 +- .github/scripts/pr-quality-state.cjs | 174 ++++ .github/scripts/pr-quality-state.test.cjs | 271 ++++++ .github/scripts/pr-quality.cjs | 45 +- .github/scripts/pr-quality.test.cjs | 49 +- .github/workflows/enforce-pr-target.yml | 717 ++++++++------ tests/ci-workflows.test.ts | 943 ++++++++++++------- tests/helpers/enforce-pr-target-harness.ts | 71 +- 11 files changed, 1824 insertions(+), 690 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index ab02b02657..5536bd2a40 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -15,6 +15,13 @@ reviews: auto_review: enabled: true drafts: false + # The PR gate adds this label at the ready moment (checklist complete and + # quality gates green) and removes it otherwise. A label addition triggers + # a CodeRabbit review even while the PR is still a draft, which is how a + # ready-but-draft PR gets reviewed without requiring a manual + # `@coderabbitai review` comment. + labels: + - "review-ready" # Default branch (main) is included automatically; these are additional # base branches (anchored regex). base_branches: diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 840c2e4049..709b2e541a 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -48,6 +48,51 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /synchronize/); }); + it("listens for review events so bot findings after ready are caught", () => { + assert.match(workflow, /pull_request_review/); + assert.match(workflow, /pull_request_review_comment/); + }); + + it("queries review threads and feeds them to the findings claim check", () => { + assert.match(workflow, /reviewThreads\(first: 100\)/); + assert.match(workflow, /unresolvedFindingsClaim/); + assert.match(workflow, /findingsClaim\.byBot/); + assert.match(workflow, /review_findings/); + }); + + it("fails closed when review threads cannot be read", () => { + assert.match(workflow, /findingsUnverifiable/); + assert.match(workflow, /findings claim could not be verified/); + }); + + it("writes exactly one consolidated comment via a single upsert helper", () => { + assert.match(workflow, /GATE_MARKER,/); + assert.match(workflow, /comment\.body\?\.includes\(GATE_MARKER\)/); + assert.match(workflow, /upsertGateComment/); + assert.match(workflow, /buildGateCommentBody/); + // No legacy two-comment write path remains. + assert.doesNotMatch(workflow, /upsertReadinessComment/); + assert.doesNotMatch(workflow, /buildReadinessCommentBody/); + // No intermediate checkpoint comment writes. + assert.doesNotMatch(workflow, /Draft conversion pending/); + assert.doesNotMatch(workflow, /Recording ownership state/); + }); + + it("manages the review-ready label for the CodeRabbit opt-in trigger", () => { + assert.match(workflow, /REVIEW_READY_LABEL\s*=\s*"review-ready"/); + assert.match(workflow, /github\.rest\.issues\.addLabels/); + assert.match(workflow, /github\.rest\.issues\.removeLabel/); + assert.match(workflow, /reviewReadyDesired/); + }); + + it("migrates legacy two-comment PRs and deletes the old comments", () => { + assert.match(workflow, /migrateLegacyCommentsIfNeeded/); + assert.match(workflow, /migrateLegacyGateState/); + assert.match(workflow, /github\.rest\.issues\.deleteComment/); + assert.match(workflow, /legacyEnforcerComment/); + assert.match(workflow, /legacyReadinessComment/); + }); + it("checks out trusted base-branch scripts only (never PR head)", () => { // Scope the assertions to the checkout step itself, so a stray `ref:` on // another step cannot satisfy the pin while the checkout stays mutable. diff --git a/.github/scripts/pr-quality-messages.cjs b/.github/scripts/pr-quality-messages.cjs index e42d31c374..773b46420b 100644 --- a/.github/scripts/pr-quality-messages.cjs +++ b/.github/scripts/pr-quality-messages.cjs @@ -5,11 +5,14 @@ const { } = require("./pr-quality.cjs"); const { readinessStateMarker, + gateStateMarker, READINESS_LATEST_DEV_BEHIND_MAX } = require("./pr-quality-state.cjs"); -/** Marks the bot's review-readiness checklist message. */ +/** Marks the bot's consolidated PR gate message. */ const READINESS_MARKER = ""; +/** Marks the bot's consolidated PR gate message. */ +const GATE_MARKER = ""; function inlineCode(value) { const text = String(value); @@ -29,32 +32,57 @@ function readinessChecklistLines(readiness) { } /** - * The full readiness-message body: marker, serialized state, mirror lines for - * the tickable boxes, the tick count, and the path-specific extra lines. + * The consolidated PR-gate comment body. It is the single always-present bot + * message on a contributor PR and carries everything the author needs: current + * status, actionable next steps, the readiness-checklist mirror, and the draft + * reason. The whole body is rebuilt every run and written exactly once, so it + * always reflects the current state and can never be double-edited. + * + * @param {object} state serialized gate state (for the embedded marker). + * @param {object} opts + * @param {string} opts.status "DRAFT" or "READY". + * @param {string} opts.statusReason one-line why. + * @param {string[]} opts.actions actionable "What to do" lines (rendered as bullets). + * @param {object} opts.readiness extractReviewReadiness result (mirror + tick count). + * @param {boolean} opts.checklistRequired + * @param {string[]} opts.notices extra lines (claim/stale/review-requested). */ -function buildReadinessCommentBody(state, readiness, extra) { - const complete = readiness.present && readiness.complete; +function buildGateCommentBody(state, opts) { + const { + status, + statusReason, + actions = [], + readiness, + checklistRequired = true, + notices = [] + } = opts; + const complete = readiness?.present && readiness?.complete; + const statusEmoji = status === "READY" ? "✅" : "⏳"; return [ - READINESS_MARKER, - readinessStateMarker(state), + GATE_MARKER, + gateStateMarker(state), "", - "## Review readiness checklist", + `## ${statusEmoji} ${status}`, + statusReason ? `- ${statusReason}` : "", "", - readiness.present - ? "This PR is kept in **draft** until every requirement below is fulfilled. The tickable checklist has been added to your PR description — tick all four boxes there." - : "The review readiness checklist is not required for this author.", - "", - ...(readiness.present ? readinessChecklistLines(readiness) : []), - "", - readiness.present - ? complete - ? "✅ **4/4** boxes ticked." - : `**${readiness.checked}/${readiness.total}** boxes ticked.` - : "", - "", - ...extra - ]; + ...(actions.length > 0 + ? ["## What to do", "", ...actions.map(line => `- ${line}`), ""] + : []), + ...(checklistRequired && readiness?.present + ? [ + "## Review readiness checklist", + "", + ...readinessChecklistLines(readiness), + "", + complete + ? "✅ **4/4** boxes ticked." + : `**${readiness.checked}/${readiness.total}** boxes ticked.`, + "" + ] + : []), + ...notices + ].filter(line => line !== null && line !== undefined); } function descriptionFailureLines(reason) { @@ -178,6 +206,32 @@ function buildClaimCheckNotice(violations, liveHeadSha) { return lines; } +/** + * The notice shown when the gate's own findings check disproves the + * Codex/CodeRabbit findings box. `byBot` maps each review-bot login to its + * unresolved finding count (inline threads plus, for CodeRabbit, findings it + * posted only in its review body because they fell outside the diff range). + * The box is unticked and the PR stays a draft until every finding is + * resolved. + */ +function buildFindingsClaimNotice(byBot) { + const names = { + "chatgpt-codex-connector[bot]": "Codex", + "coderabbitai[bot]": "CodeRabbit" + }; + const lines = []; + for (const [login, count] of Object.entries(byBot)) { + const label = names[login] ?? login; + lines.push( + `${label} has ${count} unresolved finding${count === 1 ? "" : "s"}; the **Codex/CodeRabbit findings** box has been unticked.` + ); + } + lines.push( + "Resolve every open review conversation on this pull request, then re-tick the box." + ); + return lines; +} + /** The reset notice shown when a completion no longer covers the live head. */ function buildStaleNotice({ completionHeadSha, liveHeadSha, eventAction }) { let lead; @@ -196,12 +250,14 @@ function buildStaleNotice({ completionHeadSha, liveHeadSha, eventAction }) { module.exports = { READINESS_MARKER, + GATE_MARKER, inlineCode, readinessChecklistLines, - buildReadinessCommentBody, + buildGateCommentBody, descriptionFailureLines, buildFailureSections, failureSummary, buildStaleNotice, - buildClaimCheckNotice + buildClaimCheckNotice, + buildFindingsClaimNotice }; diff --git a/.github/scripts/pr-quality-messages.test.cjs b/.github/scripts/pr-quality-messages.test.cjs index 893f9173ec..0866a952d5 100644 --- a/.github/scripts/pr-quality-messages.test.cjs +++ b/.github/scripts/pr-quality-messages.test.cjs @@ -6,15 +6,16 @@ const { buildReviewReadinessSection } = require("./pr-quality.cjs"); const { - READINESS_MARKER, + GATE_MARKER, inlineCode, readinessChecklistLines, - buildReadinessCommentBody, + buildGateCommentBody, descriptionFailureLines, buildFailureSections, failureSummary, buildStaleNotice, - buildClaimCheckNotice + buildClaimCheckNotice, + buildFindingsClaimNotice } = require("./pr-quality-messages.cjs"); const PR = { @@ -45,7 +46,7 @@ describe("readinessChecklistLines", () => { }); }); -describe("buildReadinessCommentBody", () => { +describe("buildGateCommentBody", () => { const readiness = { present: true, complete: false, @@ -54,26 +55,57 @@ describe("buildReadinessCommentBody", () => { 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 consolidated gate state marker. This is the only state + * marker the gate writes after the migration; the two legacy patterns above + * are read only to migrate pre-consolidation PRs. + */ +const GATE_STATE_PATTERN = + //; /** * v2 adds `completedAtHeadSha` so a completed checklist is bound to the exact @@ -70,6 +77,54 @@ function readinessStateMarker(state) { ); } +/** + * Parse the consolidated gate state marker, or `null` when absent or + * unreadable. + */ +function parseGateState(body, warn = () => {}) { + const match = body?.match(GATE_STATE_PATTERN); + + if (!match) { + return null; + } + + try { + return JSON.parse(match[1]); + } catch (error) { + warn(`Could not parse stored gate state: ${error.message}`); + + return null; + } +} + +/** Serialize the consolidated gate state into its comment marker. */ +function gateStateMarker(state) { + return ( + "" + ); +} + +/** + * Fresh consolidated gate state. It merges the old enforcer ownership fields + * (active / autoDraftedByBot / titlePrefixedByBot) with the readiness fields + * (maintainersPinged / completedAtHeadSha). `reviewReadyLabeled` records + * whether the gate currently owns the `review-ready` label, so a run that + * merely re-renders the comment does not re-fire the label webhook. + */ +function defaultGateState() { + return { + version: 1, + active: false, + autoDraftedByBot: false, + titlePrefixedByBot: false, + maintainersPinged: false, + completedAtHeadSha: null, + reviewReadyLabeled: false + }; +} + /** The enforcer comment state after every quality gate clears. */ function clearedEnforcerState() { return { @@ -106,6 +161,28 @@ function defaultReadinessState() { }; } +/** + * Migrate a pre-consolidation PR: merge the legacy enforcer and readiness + * states into the consolidated gate state. The legacy states are read from the + * two old bot comments; either may be absent (null). State fields are read + * for truthiness (not strict type), matching how the pre-consolidation gate + * read them — a legacy marker carrying `"active":"true"` still restores. + */ +function migrateLegacyGateState(enforcerState, readinessState) { + const gate = defaultGateState(); + if (enforcerState) { + gate.active = Boolean(enforcerState.active); + gate.autoDraftedByBot = Boolean(enforcerState.autoDraftedByBot); + gate.titlePrefixedByBot = Boolean(enforcerState.titlePrefixedByBot); + } + if (readinessState) { + gate.autoDraftedByBot = Boolean(readinessState.autoDraftedByBot); + gate.maintainersPinged = Boolean(readinessState.maintainersPinged); + gate.completedAtHeadSha = readinessState.completedAtHeadSha ?? null; + } + return gate; +} + /** * A completed checklist is an attestation about a specific head. The * attestation is stale when the recorded completion head differs from the @@ -141,6 +218,94 @@ function readinessClaimViolations({ return violations; } +/** + * The review bots whose findings threads the gate can verify. Codex posts + * under the ChatGPT Codex Connector app; CodeRabbit under coderabbitai. Both + * attach inline findings as pull-request review threads. + */ +const REVIEW_FINDINGS_BOT_LOGINS = [ + "chatgpt-codex-connector[bot]", + "coderabbitai[bot]" +]; + +/** + * CodeRabbit's review-body line that reports actionable inline findings. The + * gate reads this to count findings that CodeRabbit posts only as review-body + * text ("outside the diff range") rather than as inline review threads. + */ +const CODE_RABBIT_ACTIONABLE_RE = + /\*\*Actionable comments posted:\s*(\d+)\*\*/i; + +/** + * Pull-request reviews (from `pulls.listReviews`) that carry CodeRabbit + * findings. CodeRabbit posts some findings that cannot be attached inline + * ("outside the diff range") in the review body with the line + * `**Actionable comments posted: N**`; those never become review threads, so + * the thread check alone would miss them. This supplements the thread check: + * a CodeRabbit review of the live head whose body reports actionable comments + * counts as an unresolved finding. + * + * Only the most recent review for the live head is considered (the head a + * findings-review covers is the head that must be clean), so an older review + * of a superseded commit cannot keep the box unticked forever. + */ +function coderabbitOutsideDiffFindings({ reviews = [], liveHeadSha }) { + if (!liveHeadSha || !Array.isArray(reviews) || reviews.length === 0) { + return { code: null, unresolved: 0, byBot: {} }; + } + const latestForHead = reviews + .filter(review => review?.commit_id === liveHeadSha) + .sort( + (a, b) => + Date.parse(String(b?.submitted_at ?? "")) - + Date.parse(String(a?.submitted_at ?? "")) + )[0]; + const body = String(latestForHead?.body ?? ""); + const match = CODE_RABBIT_ACTIONABLE_RE.exec(body); + if (!match) return { code: null, unresolved: 0, byBot: {} }; + const count = Number(match[1]); + if (!(count > 0)) return { code: null, unresolved: 0, byBot: {} }; + return { + code: "review_findings", + unresolved: count, + byBot: { "coderabbitai[bot]": count } + }; +} + +/** + * Verify the Codex/CodeRabbit findings claim. The primary signal is the + * pull-request review threads the GraphQL `pullRequestReviewThreads` query + * returns: a thread authored by a review bot that is not explicitly resolved + * is an unresolved finding. CodeRabbit additionally reports some findings + * only in its review body (outside the diff range); those are added by the + * `coderabbitOutsideDiffFindings` supplement so they cannot slip through. + * The supplement is subordinate: it never subtracts, only adds unresolved + * counts for the live head, and once the reviewer resolves the threads the + * next run re-checks. + */ +function unresolvedFindingsClaim({ threads = [], reviews = [], liveHeadSha }) { + const byBot = {}; + let unresolved = 0; + for (const thread of threads) { + const login = thread?.author?.login; + if (!REVIEW_FINDINGS_BOT_LOGINS.includes(login)) continue; + if (thread.isResolved !== true) { + byBot[login] = (byBot[login] ?? 0) + 1; + unresolved += 1; + } + } + const outside = coderabbitOutsideDiffFindings({ reviews, liveHeadSha }); + if (outside.code) { + for (const [login, count] of Object.entries(outside.byBot)) { + byBot[login] = (byBot[login] ?? 0) + count; + unresolved += count; + } + } + return unresolved > 0 + ? { code: "review_findings", unresolved, byBot } + : { code: null, unresolved: 0, byBot }; +} + function completionIsStale({ checklistRequired, checklistComplete, @@ -178,13 +343,22 @@ function completionIsStale({ module.exports = { READINESS_LATEST_DEV_BEHIND_MAX, readinessClaimViolations, + unresolvedFindingsClaim, STATE_PATTERN, READINESS_STATE_PATTERN, + GATE_STATE_PATTERN, READINESS_STATE_VERSION, + REVIEW_FINDINGS_BOT_LOGINS, + CODE_RABBIT_ACTIONABLE_RE, + coderabbitOutsideDiffFindings, parseState, stateMarker, parseReadinessState, readinessStateMarker, + parseGateState, + gateStateMarker, + defaultGateState, + migrateLegacyGateState, clearedEnforcerState, defaultEnforcerState, defaultReadinessState, diff --git a/.github/scripts/pr-quality-state.test.cjs b/.github/scripts/pr-quality-state.test.cjs index e3b63fd5c2..33d6942ce3 100644 --- a/.github/scripts/pr-quality-state.test.cjs +++ b/.github/scripts/pr-quality-state.test.cjs @@ -7,11 +7,18 @@ const { stateMarker, parseReadinessState, readinessStateMarker, + parseGateState, + gateStateMarker, + defaultGateState, + migrateLegacyGateState, clearedEnforcerState, defaultEnforcerState, defaultReadinessState, completionIsStale, readinessClaimViolations, + unresolvedFindingsClaim, + coderabbitOutsideDiffFindings, + REVIEW_FINDINGS_BOT_LOGINS, READINESS_LATEST_DEV_BEHIND_MAX, READINESS_STATE_VERSION } = require("./pr-quality-state.cjs"); @@ -261,3 +268,267 @@ describe("readinessClaimViolations", () => { ); }); }); + +describe("unresolvedFindingsClaim", () => { + it("passes when there are no review threads at all", () => { + assert.deepEqual(unresolvedFindingsClaim({ threads: [] }), { + code: null, + unresolved: 0, + byBot: {}, + }); + }); + + it("passes when every bot thread is resolved", () => { + assert.deepEqual( + unresolvedFindingsClaim({ + threads: [ + { isResolved: true, author: { login: "chatgpt-codex-connector[bot]" } }, + { isResolved: true, author: { login: "coderabbitai[bot]" } }, + ], + }), + { code: null, unresolved: 0, byBot: {} }, + ); + }); + + it("flags one unresolved Codex thread and counts it per bot", () => { + assert.deepEqual( + unresolvedFindingsClaim({ + threads: [ + { isResolved: false, author: { login: "chatgpt-codex-connector[bot]" } }, + { isResolved: true, author: { login: "coderabbitai[bot]" } }, + ], + }), + { + code: "review_findings", + unresolved: 1, + byBot: { "chatgpt-codex-connector[bot]": 1 }, + }, + ); + }); + + it("flags unresolved threads from both bots and counts each", () => { + assert.deepEqual( + unresolvedFindingsClaim({ + threads: [ + { isResolved: false, author: { login: "chatgpt-codex-connector[bot]" } }, + { isResolved: false, author: { login: "chatgpt-codex-connector[bot]" } }, + { isResolved: false, author: { login: "coderabbitai[bot]" } }, + ], + }), + { + code: "review_findings", + unresolved: 3, + byBot: { + "chatgpt-codex-connector[bot]": 2, + "coderabbitai[bot]": 1, + }, + }, + ); + }); + + it("ignores unresolved threads from humans", () => { + assert.deepEqual( + unresolvedFindingsClaim({ + threads: [ + { isResolved: false, author: { login: "wibias" } }, + { isResolved: false, author: null }, + ], + }), + { code: null, unresolved: 0, byBot: {} }, + ); + }); + + it("fails closed on a thread with no resolution state", () => { + // A thread whose isResolved is missing cannot be claimed resolved. + assert.deepEqual( + unresolvedFindingsClaim({ + threads: [{ isResolved: null, author: { login: "coderabbitai[bot]" } }], + }), + { + code: "review_findings", + unresolved: 1, + byBot: { "coderabbitai[bot]": 1 }, + }, + ); + }); + + it("exposes the bot allowlist", () => { + assert.deepEqual(REVIEW_FINDINGS_BOT_LOGINS, [ + "chatgpt-codex-connector[bot]", + "coderabbitai[bot]", + ]); + }); +}); + +describe("coderabbitOutsideDiffFindings", () => { + const HEAD = "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"; + + it("flags a CodeRabbit review of the live head with actionable comments", () => { + const claim = coderabbitOutsideDiffFindings({ + reviews: [ + { + body: "**Actionable comments posted: 3**\n\nSome walkthrough.", + commit_id: HEAD, + submitted_at: "2026-08-04T06:24:02Z", + }, + ], + liveHeadSha: HEAD, + }); + assert.deepEqual(claim, { + code: "review_findings", + unresolved: 3, + byBot: { "coderabbitai[bot]": 3 }, + }); + }); + + it("ignores a review of a different head", () => { + const claim = coderabbitOutsideDiffFindings({ + reviews: [ + { + body: "**Actionable comments posted: 3**", + commit_id: "1111111111111111111111111111111111111111", + submitted_at: "2026-08-04T06:24:02Z", + }, + ], + liveHeadSha: HEAD, + }); + assert.deepEqual(claim, { code: null, unresolved: 0, byBot: {} }); + }); + + it("ignores a review reporting zero actionable comments", () => { + const claim = coderabbitOutsideDiffFindings({ + reviews: [{ body: "**Actionable comments posted: 0**", commit_id: HEAD }], + liveHeadSha: HEAD, + }); + assert.deepEqual(claim, { code: null, unresolved: 0, byBot: {} }); + }); + + it("uses the most recent review of the live head", () => { + const claim = coderabbitOutsideDiffFindings({ + reviews: [ + { body: "**Actionable comments posted: 2**", commit_id: HEAD, submitted_at: "2026-08-04T06:00:00Z" }, + { body: "**Actionable comments posted: 5**", commit_id: HEAD, submitted_at: "2026-08-04T07:00:00Z" }, + ], + liveHeadSha: HEAD, + }); + assert.equal(claim.unresolved, 5); + }); + + it("returns clean for no reviews or no live head", () => { + assert.deepEqual(coderabbitOutsideDiffFindings({ reviews: [], liveHeadSha: HEAD }), { + code: null, + unresolved: 0, + byBot: {}, + }); + assert.deepEqual(coderabbitOutsideDiffFindings({ reviews: [{ body: "**Actionable comments posted: 1**", commit_id: HEAD }] }), { + code: null, + unresolved: 0, + byBot: {}, + }); + }); +}); + +describe("unresolvedFindingsClaim with outside-diff supplement", () => { + const HEAD = "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"; + + it("adds the outside-diff count to a clean thread set", () => { + const claim = unresolvedFindingsClaim({ + threads: [], + reviews: [{ body: "**Actionable comments posted: 2**", commit_id: HEAD, submitted_at: "2026-08-04T06:24:02Z" }], + liveHeadSha: HEAD, + }); + assert.deepEqual(claim, { + code: "review_findings", + unresolved: 2, + byBot: { "coderabbitai[bot]": 2 }, + }); + }); + + it("adds the outside-diff count to an unresolved thread count", () => { + const claim = unresolvedFindingsClaim({ + threads: [ + { isResolved: false, author: { login: "coderabbitai[bot]" } }, + ], + reviews: [{ body: "**Actionable comments posted: 2**", commit_id: HEAD, submitted_at: "2026-08-04T06:24:02Z" }], + liveHeadSha: HEAD, + }); + assert.deepEqual(claim, { + code: "review_findings", + unresolved: 3, + byBot: { "coderabbitai[bot]": 3 }, + }); + }); + + it("keeps a resolved thread set clean even with a stale review", () => { + const claim = unresolvedFindingsClaim({ + threads: [ + { isResolved: true, author: { login: "coderabbitai[bot]" } }, + ], + reviews: [{ body: "**Actionable comments posted: 2**", commit_id: "1111111111111111111111111111111111111111", submitted_at: "2026-08-04T06:24:02Z" }], + liveHeadSha: HEAD, + }); + assert.deepEqual(claim, { code: null, unresolved: 0, byBot: {} }); + }); +}); + +describe("gate state", () => { + it("round-trips through gateStateMarker and parseGateState", () => { + const state = defaultGateState(); + assert.deepEqual(parseGateState(gateStateMarker(state)), state); + }); + + it("returns null for markerless or unreadable gate state and warns", () => { + assert.equal(parseGateState("plain comment"), null); + assert.equal(parseGateState(null), null); + const warnings = []; + assert.equal( + parseGateState("", m => + warnings.push(m), + ), + null, + ); + assert.match(warnings[0], /Could not parse stored gate state/); + }); + + it("builds a fresh gate state", () => { + assert.deepEqual(defaultGateState(), { + version: 1, + active: false, + autoDraftedByBot: false, + titlePrefixedByBot: false, + maintainersPinged: false, + completedAtHeadSha: null, + reviewReadyLabeled: false, + }); + }); + + it("merges legacy enforcer + readiness states", () => { + const merged = migrateLegacyGateState( + { version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true }, + { version: 2, autoDraftedByBot: true, maintainersPinged: true, completedAtHeadSha: "abc123" }, + ); + assert.equal(merged.active, true); + assert.equal(merged.autoDraftedByBot, true); + assert.equal(merged.titlePrefixedByBot, true); + assert.equal(merged.maintainersPinged, true); + assert.equal(merged.completedAtHeadSha, "abc123"); + assert.equal(merged.reviewReadyLabeled, false); + }); + + it("migrates with either legacy state absent", () => { + const onlyEnforcer = migrateLegacyGateState( + { version: 1, active: true, titlePrefixedByBot: true }, + null, + ); + assert.equal(onlyEnforcer.active, true); + assert.equal(onlyEnforcer.titlePrefixedByBot, true); + assert.equal(onlyEnforcer.completedAtHeadSha, null); + + const onlyReadiness = migrateLegacyGateState( + null, + { version: 2, maintainersPinged: true }, + ); + assert.equal(onlyReadiness.active, false); + assert.equal(onlyReadiness.maintainersPinged, true); + }); +}); diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index f401b90c68..16adf2ebc3 100644 --- a/.github/scripts/pr-quality.cjs +++ b/.github/scripts/pr-quality.cjs @@ -27,18 +27,19 @@ const REVIEW_READINESS_END = ""; const REVIEW_READINESS_ITEMS = [ "All CI tests are green on my local testing.", "I pushed my PR to the latest dev commit.", - "I fixed all correct Codex and CodeRabbit findings.", + "I resolved all correct Codex and CodeRabbit findings.", "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. + * in sync with REVIEW_READINESS_ITEMS: index 0 is the CI claim, index 1 is + * the latest-dev claim, and index 2 is the Codex/CodeRabbit findings claim. */ const REVIEW_READINESS_CLAIM_INDEX = { ci_green: 0, - latest_dev: 1 + latest_dev: 1, + review_findings: 2 }; /** @@ -179,6 +180,33 @@ function hasGuiCue(title, body) { ); } +/** + * Phrases in a maintainer comment that waive the GUI-screenshot gate. A + * comment saying the change does not touch the GUI means the `gui` cue in the + * title/description is a false positive and a screenshot is not required. The + * negation word must appear within a short window before `gui`, so a comment + * like "this touches gui but only the config" (no negation) keeps the gate. + */ +const GUI_OVERRIDE_RE = + /\b(?:no|not|doesn'?t|does not|never|without)\b[\s\S]{0,40}?\bgui\b/i; + +/** + * True when a maintainer (OWNER / COLLABORATOR / MEMBER) issue comment waives + * the GUI-screenshot requirement. Only the comment author's association + * counts: the PR author (`CONTRIBUTOR`/`NONE`) cannot override their own + * screenshot requirement. + */ +function hasGuiOverride({ comments = [] }) { + return comments.some( + comment => + (comment?.author_association === "OWNER" || + comment?.author_association === "COLLABORATOR" || + comment?.author_association === "MEMBER") && + typeof comment?.body === "string" && + GUI_OVERRIDE_RE.test(comment.body) + ); +} + /** * Drop the regions GitHub does not render as Markdown: HTML comments and * fenced code blocks. Image syntax there is literal text, not evidence. @@ -416,6 +444,8 @@ function collectPrQualityFailures({ ancestryLookupFailed = false, /** True when baseRef is another open PR's head (stacked child). */ stackedBase = false, + /** Issue comments; a maintainer comment waives the GUI-screenshot gate. */ + guiOverrideComments = [] }) { const failures = []; const wrongBase = !allowedBases.includes(baseRef) && !stackedBase; @@ -444,13 +474,15 @@ function collectPrQualityFailures({ } // GUI-cued PRs must prove the UI change visually. The template's own - // screenshot instruction is boilerplate, so it cannot trigger this gate. + // screenshot instruction is boilerplate, so it cannot trigger this gate. A + // maintainer comment saying the change does not touch the GUI waives it. if ( hasGuiCue( title, typeof body === "string" ? stripPrTemplateBoilerplate(body) : "", ) && - !hasScreenshotEvidence(body) + !hasScreenshotEvidence(body) && + !hasGuiOverride({ comments: guiOverrideComments }) ) { failures.push({ code: "missing_ui_screenshot" }); } @@ -467,6 +499,7 @@ module.exports = { authorHasPushPermission, assessPrDescription, hasGuiCue, + hasGuiOverride, hasScreenshotEvidence, buildReviewReadinessSection, extractReviewReadiness, diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index 6bf40964eb..9b2455d2fd 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -9,6 +9,7 @@ const { authorHasPushPermission, assessPrDescription, hasGuiCue, + hasGuiOverride, hasScreenshotEvidence, buildReviewReadinessSection, extractReviewReadiness, @@ -148,6 +149,50 @@ describe("hasGuiCue", () => { }); }); +describe("hasGuiOverride", () => { + const owner = { author_association: "OWNER", body: "Not touching gui here." }; + const collaborator = { author_association: "COLLABORATOR", body: "no gui changes needed" }; + const member = { author_association: "MEMBER", body: "doesn't change the gui" }; + const author = { author_association: "CONTRIBUTOR", body: "Not touching gui here." }; + const outsider = { author_association: "NONE", body: "Not touching gui here." }; + + it("matches a maintainer comment with a negation phrase", () => { + assert.equal(hasGuiOverride({ comments: [owner] }), true); + assert.equal(hasGuiOverride({ comments: [collaborator] }), true); + assert.equal(hasGuiOverride({ comments: [member] }), true); + assert.equal( + hasGuiOverride({ comments: [{ author_association: "OWNER", body: "I did not change gui" }] }), + true, + ); + assert.equal( + hasGuiOverride({ comments: [{ author_association: "OWNER", body: "Without gui changes" }] }), + true, + ); + }); + + it("does not let the PR author or a non-collaborator waive the gate", () => { + assert.equal(hasGuiOverride({ comments: [author] }), false); + assert.equal(hasGuiOverride({ comments: [outsider] }), false); + }); + + it("does not match a comment that names gui without negating it", () => { + assert.equal( + hasGuiOverride({ comments: [{ author_association: "OWNER", body: "This touches gui but only config" }] }), + false, + ); + assert.equal( + hasGuiOverride({ comments: [{ author_association: "OWNER", body: "gui is involved here" }] }), + false, + ); + }); + + it("is clean for no comments or a comment without a body", () => { + assert.equal(hasGuiOverride({ comments: [] }), false); + assert.equal(hasGuiOverride({ comments: [{ author_association: "OWNER" }] }), false); + assert.equal(hasGuiOverride({}), false); + }); +}); + describe("hasScreenshotEvidence", () => { it("accepts embedded markdown images", () => { assert.equal( @@ -489,7 +534,7 @@ describe("uncheckReviewReadinessBoxes", () => { "", "- [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] I resolved all correct Codex and CodeRabbit findings.", "- [x] My PR is ready for review.", "", ].join("\n"); @@ -510,7 +555,7 @@ describe("uncheckReviewReadinessBoxes", () => { ]); 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] I resolved all correct Codex and CodeRabbit findings.")); assert.ok(body.includes("- [x] My PR is ready for review.")); }); diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 94023e7d0a..6a1c228918 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -8,8 +8,13 @@ on: - edited - ready_for_review - synchronize + # Review events let the gate catch bot findings (Codex / CodeRabbit + # review threads) that land after a PR was marked ready, without waiting + # for the author's next push. + - pull_request_review + - pull_request_review_comment -# pull-requests:write covers title/comment updates. +# pull-requests:write covers title/comment/label updates. # contents:write is required for convertPullRequestToDraft / # markPullRequestReadyForReview GraphQL mutations with GITHUB_TOKEN # (otherwise: "Resource not accessible by integration"). This workflow @@ -49,6 +54,7 @@ jobs: const { collectPrQualityFailures, authorHasPushPermission, + hasGuiOverride, extractReviewReadiness, appendReviewReadinessSection, stripReviewReadinessSection, @@ -59,14 +65,15 @@ jobs: path.join(process.cwd(), ".github", "scripts", "pr-quality.cjs"), ); const { + parseGateState, + gateStateMarker, parseState, - stateMarker, parseReadinessState, - clearedEnforcerState, - defaultEnforcerState, - defaultReadinessState, + defaultGateState, + migrateLegacyGateState, completionIsStale, readinessClaimViolations, + unresolvedFindingsClaim, READINESS_STATE_VERSION } = require( path.join( @@ -77,13 +84,15 @@ jobs: ), ); const { + GATE_MARKER, READINESS_MARKER, inlineCode, - buildReadinessCommentBody, + buildGateCommentBody, buildFailureSections, failureSummary, buildStaleNotice, - buildClaimCheckNotice + buildClaimCheckNotice, + buildFindingsClaimNotice } = require( path.join( process.cwd(), @@ -106,8 +115,8 @@ jobs: const ALLOWED_BASES = ["dev"]; const DEFAULT_BASE = "dev"; const TITLE_PREFIX = "[WRONG BRANCH] "; - const COMMENT_MARKER = ""; const LEGACY_COMMENT_MARKER = ""; + const REVIEW_READY_LABEL = "review-ready"; const MAINTAINERS_FILE = "MAINTAINERS.md"; const { owner, repo } = context.repo; @@ -129,24 +138,48 @@ jobs: } ); - const botComment = comments.find( + // One consolidated comment. The gate finds its own comment by the + // single GATE_MARKER; the legacy enforcer/readiness markers are + // matched only to migrate pre-consolidation PRs. + const gateComment = comments.find( comment => comment.user?.login === "github-actions[bot]" && - (comment.body?.includes(COMMENT_MARKER) || - comment.body?.includes(LEGACY_COMMENT_MARKER)) + comment.body?.includes(GATE_MARKER) + ); + let gateCommentId = gateComment?.id ?? null; + const storedGateState = parseGateState( + gateComment?.body, + message => core.warning(message) ); - let botCommentId = botComment?.id ?? null; - const readinessComment = comments.find( + // Legacy comments: the pre-consolidation two-comment model. Their + // state is merged once into the single comment, then the old + // comments are deleted. + const legacyEnforcerComment = comments.find( + comment => + comment.user?.login === "github-actions[bot]" && + (comment.body?.includes("") || + comment.body?.includes(LEGACY_COMMENT_MARKER)) + ); + const legacyReadinessComment = comments.find( comment => comment.user?.login === "github-actions[bot]" && comment.body?.includes(READINESS_MARKER) ); - let readinessCommentId = readinessComment?.id ?? null; - const storedReadinessState = parseReadinessState( - readinessComment?.body, + const legacyEnforcerState = parseState( + legacyEnforcerComment?.body, + message => core.warning(message) + ); + const legacyReadinessState = parseReadinessState( + legacyReadinessComment?.body, message => core.warning(message) ); + const migratedGateState = migrateLegacyGateState( + legacyEnforcerState, + legacyReadinessState + ); + + let gateState = storedGateState ?? migratedGateState; /** * Maintainers from `MAINTAINERS.md` on the trusted default branch @@ -169,48 +202,77 @@ jobs: } } - async function upsertReadinessComment(state, readiness, extra) { - const lines = buildReadinessCommentBody(state, readiness, extra); - - if (readinessCommentId) { - await github.rest.issues.updateComment({ + async function setReviewReadyLabel(shouldHave, hasLabel) { + if (shouldHave && !hasLabel) { + await github.rest.issues.addLabels({ owner, repo, - comment_id: readinessCommentId, - body: lines.join("\n") + issue_number: pull_number, + labels: [REVIEW_READY_LABEL] + }); + } else if (!shouldHave && hasLabel) { + await github.rest.issues.removeLabel({ + owner, + repo, + issue_number: pull_number, + name: REVIEW_READY_LABEL }); - - return; } - - const created = await github.rest.issues.createComment({ - owner, - repo, - issue_number: pull_number, - body: lines.join("\n") - }); - readinessCommentId = created.data.id; } - async function upsertComment(body) { - if (botCommentId) { + /** + * The single write to the consolidated comment. Every run rebuilds + * the full body and writes it exactly once (create-if-absent, + * update-if-present), so there is never a double-edit of the + * readiness section or a stale intermediate checkpoint body. + */ + async function upsertGateComment(state, opts) { + const body = buildGateCommentBody(state, opts).join("\n"); + if (gateCommentId) { await github.rest.issues.updateComment({ owner, repo, - comment_id: botCommentId, + comment_id: gateCommentId, body }); - + await migrateLegacyCommentsIfNeeded(); return; } - const created = await github.rest.issues.createComment({ owner, repo, issue_number: pull_number, body }); - botCommentId = created.data.id; + gateCommentId = created.data.id; + await migrateLegacyCommentsIfNeeded(); + } + + /** + * One-time migration: merge legacy state into the single comment, + * then delete the two old comments. The gate comment must already + * exist (created/updated by upsertGateComment) so its id is known + * and the legacy comments are not mistaken for it. + */ + async function migrateLegacyCommentsIfNeeded() { + const legacyIds = [ + legacyEnforcerComment?.id, + legacyReadinessComment?.id + ].filter(id => typeof id === "number" && id !== gateCommentId); + if (legacyIds.length === 0) return; + for (const id of legacyIds) { + try { + await github.rest.issues.deleteComment({ + owner, + repo, + comment_id: id + }); + } catch (error) { + core.warning( + `Could not delete legacy bot comment ${id}: ${error.message}` + ); + } + } } async function convertToDraft() { @@ -257,11 +319,6 @@ jobs: ); } - const storedState = parseState( - botComment?.body, - message => core.warning(message) - ); - let authorPermission = null; let permissionLookupFailed = false; try { @@ -356,9 +413,18 @@ jobs: authorPermission, permissionLookupFailed, ancestryLookupFailed, - stackedBase + stackedBase, + // A maintainer issue comment ("not touching gui") waives the + // GUI-screenshot gate; the comments are already fetched above. + guiOverrideComments: comments }); + // A maintainer issue comment saying the change does not touch + // the GUI waives the screenshot gate. The flag is what tells the + // author the screenshot is not required, even though the failure + // itself is gone from `failures`. + const screenshotWaived = hasGuiOverride({ comments }); + // The readiness gate applies to contributors (no push permission). // Maintainers keep the failure-only contract: draft while quality // gates fail, ready again once they clear. A failed permission @@ -407,7 +473,7 @@ jobs: const eventHeadSha = context.payload.pull_request?.head?.sha ?? pr.head.sha; const completionHeadSha = - storedReadinessState?.completedAtHeadSha ?? null; + gateState.completedAtHeadSha ?? null; const headDrifted = completionIsStale({ checklistRequired, checklistComplete, @@ -432,7 +498,7 @@ jobs: const freshReadiness = extractReviewReadiness( freshPr.body ?? "" ); - readinessStateOverride = defaultReadinessState(); + readinessStateOverride = defaultGateState(); headDriftNotice = buildStaleNotice({ completionHeadSha, liveHeadSha: freshPr.head.sha, @@ -457,12 +523,14 @@ jobs: checklistComplete = readiness.present && readiness.complete; } - // The bot verifies the two checklist claims it can check itself. + // The bot verifies the three 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. + // commits behind the base; the findings box only counts while every + // Codex/CodeRabbit review thread on the PR is resolved. A disproved + // claim unchecks that box and keeps the PR a draft, exactly like a + // head-drift reset. let claimViolations = []; let claimNotice = []; if ( @@ -502,6 +570,73 @@ jobs: behindBase, behindUnknown: ancestryLookupFailed }); + // The findings claim reads the review threads via GraphQL. Only + // threads authored by the review bots count; `isResolved` must be + // explicitly true, so a missing or unreadable thread fails closed. + // CodeRabbit additionally reports some findings only in its + // review body (outside the diff range); `pulls.listReviews` + // supplies those as a supplement. A review listing failure fails + // closed the same way as an unreadable thread list. + let findingsClaim = null; + let findingsUnverifiable = false; + try { + const reviewData = await github.graphql( + ` + query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + reviewThreads(first: 100) { + nodes { + isResolved + comments(first: 1) { + nodes { + author { login } + } + } + } + } + } + } + } + `, + { owner, repo, number: pull_number } + ); + const reviewsData = await github.paginate( + github.rest.pulls.listReviews, + { + owner, + repo, + pull_number, + per_page: 100 + } + ); + findingsClaim = unresolvedFindingsClaim({ + threads: ( + reviewData?.repository?.pullRequest?.reviewThreads?.nodes ?? + [] + ).map(node => ({ + isResolved: node.isResolved, + author: node.comments?.nodes?.[0]?.author ?? null + })), + reviews: reviewsData, + liveHeadSha: pr.head.sha + }); + } catch (error) { + core.warning( + `Could not list review threads for the readiness claim check: ${error.message}` + ); + // Fail closed: an attestation must not ride on missing + // evidence, exactly like unknown CI or behind counts. + findingsUnverifiable = true; + findingsClaim = { + code: "review_findings", + unresolved: 0, + byBot: {} + }; + } + if (findingsClaim?.code) { + claimViolations.push(findingsClaim.code); + } if (claimViolations.length > 0) { const { data: freshPr } = await github.rest.pulls.get({ owner, @@ -511,11 +646,22 @@ jobs: const freshReadiness = extractReviewReadiness( freshPr.body ?? "" ); - readinessStateOverride = defaultReadinessState(); - claimNotice = buildClaimCheckNotice( - claimViolations, - freshPr.head.sha - ); + readinessStateOverride = defaultGateState(); + claimNotice = [ + ...(claimViolations.includes("review_findings") + ? findingsUnverifiable + ? [ + "The Codex/CodeRabbit findings claim could not be verified; the **Codex/CodeRabbit findings** box has been unticked. The PR stays a draft until review threads are readable again." + ] + : buildFindingsClaimNotice(findingsClaim.byBot) + : []), + ...buildClaimCheckNotice( + claimViolations.filter( + code => code !== "review_findings" + ), + freshPr.head.sha + ) + ]; if (freshReadiness.present) { const uncheckedBody = uncheckReviewReadinessBoxes( freshPr.body ?? "", @@ -551,25 +697,66 @@ jobs: ? headDriftNotice : claimNotice; + // Assemble the "What to do" action lines for the consolidated + // comment. Only the applicable actions render. + function buildActions() { + const actions = []; + if (failures.some(failure => failure.code === "wrong_base")) { + actions.push( + `Retarget this PR to ${inlineCode(DEFAULT_BASE)} — all contributions go to ${inlineCode(DEFAULT_BASE)}.` + ); + } + if (failures.some(failure => failure.code === "wrong_ancestry")) { + actions.push( + `Rebase onto the current ${inlineCode(DEFAULT_BASE)} branch instead of opening from ${inlineCode("main")}.` + ); + } + if (failures.some(failure => failure.code === "bad_description")) { + actions.push( + "Add a real **Summary** and **Test plan** to the PR description." + ); + } + if (failures.some(failure => failure.code === "missing_ui_screenshot")) { + actions.push( + "Add a screenshot of the UI change to the PR description." + ); + } + if (checklistRequired && !checklistComplete) { + actions.push( + `Tick all four boxes in the PR description once you're done (currently ${readiness.checked}/${readiness.total}).` + ); + } + if (revalidationNotice.length > 0) { + actions.push(...revalidationNotice); + } + return actions; + } + + // The `review-ready` label is the CodeRabbit/Codex opt-in trigger: + // add it at the ready moment, remove it while the PR is not ready. + const readyMoment = + checklistRequired && checklistComplete && failures.length === 0; + const reviewReadyDesired = readyMoment; + const hasReviewReadyLabel = (pr.labels ?? []).some( + label => label.name === REVIEW_READY_LABEL + ); + const reviewReadyChanged = hasReviewReadyLabel !== reviewReadyDesired; + if (reviewReadyChanged) { + await setReviewReadyLabel(reviewReadyDesired, hasReviewReadyLabel); + } + gateState.reviewReadyLabeled = reviewReadyDesired; + if (mustDraft) { let draftConverted = false; - const readinessState = - readinessStateOverride ?? - (storedReadinessState - ? { ...storedReadinessState } - : defaultReadinessState()); + const draftState = readinessStateOverride ?? { ...gateState }; 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; + draftState.completedAtHeadSha = pr.head.sha; + draftState.version = 1; } - const state = storedState?.active - ? { ...storedState } - : defaultEnforcerState(); + const state = { ...draftState, active: true }; const hasWrongBase = failures.some( failure => failure.code === "wrong_base" ); @@ -598,61 +785,12 @@ jobs: state.titlePrefixedByBot = false; } - if (checklistRequired && !pr.draft && !checklistComplete) { - // Claim draft ownership before the mutation so a successful - // convert followed by a failed comment still restores later - // (same checkpoint discipline as the quality-failure path). - readinessState.autoDraftedByBot = true; - await upsertReadinessComment( - readinessState, - readiness, - [ - ...revalidationNotice, - "This PR stays in draft until every box above is ticked." - ] - ); - } - if (failures.length > 0) { - state.ancestryFailed = failures.some( - failure => failure.code === "wrong_ancestry" - ); - state.descriptionFailed = failures.some( - failure => failure.code === "bad_description" - ); - state.screenshotFailed = failures.some( - failure => failure.code === "missing_ui_screenshot" - ); - let draftConversionFailed = false; - const failureSections = buildFailureSections(failures, { - pr, - allowedBases: ALLOWED_BASES, - defaultBase: DEFAULT_BASE - }); - - if (checklistRequired && !checklistComplete) { - failureSections.push( - "", - "⏳ **Review readiness checklist**", - "", - `This pull request stays in draft until all four boxes of the readiness checklist in the description are ticked (currently ${readiness.checked}/${readiness.total}).`, - "", - `@${pr.user.login} Tick the boxes once your local CI is green, your branch is on the latest ${inlineCode(DEFAULT_BASE)} commit, and every correct Codex and CodeRabbit finding is fixed.` - ); - } - - await upsertComment( - [ - COMMENT_MARKER, - stateMarker(state), - "", - ...failureSections, - "", - "Recording ownership state before applying title/draft changes…" - ].join("\n") - ); - + + // Apply the bot-owned title prefix so the PR itself carries a + // durable signal of the wrong base (claim ownership before the + // write; a failed write keeps ownership for the next retry). if (willPrefixTitle) { await github.rest.pulls.update({ owner, @@ -661,35 +799,14 @@ jobs: title: `${TITLE_PREFIX}${pr.title}` }); } - + if (!pr.draft) { // Claim draft ownership before the mutation so a successful // convert followed by a failed comment still restores later. state.autoDraftedByBot = true; - await upsertComment( - [ - COMMENT_MARKER, - stateMarker(state), - "", - ...failureSections, - "", - "Draft conversion pending…" - ].join("\n") - ); try { await convertToDraft(); draftConverted = true; - readinessState.autoDraftedByBot = true; - await upsertComment( - [ - COMMENT_MARKER, - stateMarker(state), - "", - ...failureSections, - "", - "Draft conversion succeeded; finalising explanation…" - ].join("\n") - ); } catch (error) { draftConversionFailed = true; state.autoDraftedByBot = false; @@ -698,48 +815,54 @@ jobs: ); } } - + const draftExplanation = draftConversionFailed ? "Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required `enforce-target` check will keep failing until every issue above is resolved." : state.autoDraftedByBot ? "This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again." : "This pull request was already a draft. Its draft status will be preserved after every issue above is resolved."; - - const finalSections = [...failureSections]; - - if (hasWrongBase && state.titlePrefixedByBot) { - finalSections.push( - "", - `Its title has been prefixed with ${inlineCode(TITLE_PREFIX.trim())}.` - ); - } - - await upsertComment( - [ - COMMENT_MARKER, - stateMarker(state), - "", - ...finalSections, - "", - draftExplanation - ].join("\n") - ); - - if (checklistRequired) { - await upsertReadinessComment( - readinessState, - readiness, - [ - ...revalidationNotice, - checklistComplete - ? "✅ **All four boxes are ticked.** This PR still stays in draft until the issues above are resolved." - : pr.draft || draftConverted - ? "This PR stays in draft until every box above is ticked." - : "Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked." - ] - ); - } - + + const notices = [ + ...revalidationNotice, + ...(screenshotWaived + ? ["UI screenshot waived by a maintainer comment."] + : []), + ...(hasWrongBase && state.titlePrefixedByBot + ? [`Its title has been prefixed with ${inlineCode(TITLE_PREFIX.trim())}.`] + : []), + draftExplanation, + ...(checklistRequired && !checklistComplete + ? [ + `@${pr.user.login} Tick the boxes once your local CI is green, your branch is on the latest ${inlineCode(DEFAULT_BASE)} commit, and every correct Codex and CodeRabbit finding is resolved.` + ] + : []) + ]; + + await upsertGateComment(state, { + status: "DRAFT", + statusReason: failures + .map(failure => { + if (failure.code === "wrong_base") { + return `wrong target branch (${pr.base.ref}); retarget to ${inlineCode(DEFAULT_BASE)}.`; + } + if (failure.code === "wrong_ancestry") { + return "wrong branch ancestry; rebase onto the latest dev."; + } + if (failure.code === "bad_description") { + return `PR description needs work (${failure.reason}).`; + } + if (failure.code === "missing_ui_screenshot") { + return "UI screenshot required."; + } + return failure.code; + }) + .join(" "), + actions: buildActions(), + readiness, + checklistRequired, + notices + }); + core.setFailed( `PR quality gate failed: ${failureSummary(failures, { pr })}` ); @@ -747,32 +870,15 @@ jobs: } // No quality failure; the draft is owed by the open checklist. - // Prior enforcer history (title prefix, earlier failures) gets a - // closing confirmation; the readiness comment owns the draft now. - if (storedState?.active || botComment) { - const prefixResult = shouldStripTitlePrefix - ? `The ${inlineCode(TITLE_PREFIX.trim())} title prefix has been removed.` - : "The title was left unchanged."; - await upsertComment( - [ - COMMENT_MARKER, - stateMarker(clearedEnforcerState()), - "", - "✅ **PR quality gates passed**", - "", - `This pull request now targets ${inlineCode(pr.base.ref)} with acceptable ancestry, description, and UI screenshot coverage. It stays in draft until the review readiness checklist is complete.`, - "", - `${prefixResult} The draft is owned by the checklist message below.` - ].join("\n") - ); - } - if (!pr.draft && !draftConverted) { + // Claim draft ownership before the mutation so a successful + // convert followed by a failed comment still restores later. + state.autoDraftedByBot = true; try { await convertToDraft(); draftConverted = true; } catch (error) { - readinessState.autoDraftedByBot = false; + state.autoDraftedByBot = false; core.warning( `Could not convert pull request to draft: ${error.message}` ); @@ -782,53 +888,29 @@ jobs: } } - await upsertReadinessComment( - readinessState, - readiness, - [ - ...revalidationNotice, - pr.draft || draftConverted - ? "This PR stays in draft until every box above is ticked." - : "Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked." - ] - ); - return; - } - - if (!storedState?.active && !checklistRequired) { - // A maintainer PR drafted while the permission lookup was failing - // (fail-closed) gets restored once the lookup recovers. - if (storedReadinessState?.autoDraftedByBot && pr.draft) { - let recoveryFailed = false; - try { - await markReadyForReview(); - } catch (error) { - recoveryFailed = true; - core.warning( - `Could not mark pull request ready for review: ${error.message}` - ); - } - await upsertReadinessComment( - recoveryFailed - ? { ...storedReadinessState } - : { ...storedReadinessState, autoDraftedByBot: false }, - readiness, - [ - recoveryFailed - ? "Automatic ready-for-review conversion failed; the PR stays a draft and will be retried on the next run." - : "✅ This PR is ready for review." - ] - ); - } - core.info( - "All PR quality gates passed and there is no active bot state." - ); + const notices = [ + ...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." + ]; + await upsertGateComment(state, { + status: "DRAFT", + statusReason: checklistRequired + ? `review readiness checklist open (${readiness.checked}/${readiness.total} boxes ticked).` + : "PR is kept in draft.", + actions: buildActions(), + readiness, + checklistRequired, + notices + }); return; } + // Ready path: strip a stale title prefix, mark ready, clear state. if ( - storedState?.titlePrefixedByBot && + gateState.titlePrefixedByBot && pr.title.startsWith(TITLE_PREFIX) ) { await github.rest.pulls.update({ @@ -837,12 +919,13 @@ jobs: pull_number, title: pr.title.slice(TITLE_PREFIX.length) }); + gateState.titlePrefixedByBot = false; } let readyConversionFailed = false; let readyConverted = false; const shouldMarkReady = - (storedState?.active && storedState.autoDraftedByBot) || + (gateState.active && gateState.autoDraftedByBot) || (checklistRequired && checklistComplete); if (shouldMarkReady && pr.draft) { try { @@ -856,82 +939,98 @@ jobs: } } - // The enforcer comment only exists when there was something to say; - // a clean contributor PR that never failed a quality gate has none. - if (storedState?.active || botComment) { - const completedState = readyConversionFailed - ? { ...clearedEnforcerState(), active: true, autoDraftedByBot: true } - : clearedEnforcerState(); - - const titleResult = storedState?.titlePrefixedByBot - ? `The ${inlineCode(TITLE_PREFIX.trim())} title prefix has been removed.` - : "The title was left unchanged."; - - const draftResult = readyConversionFailed - ? "Automatic ready-for-review conversion failed; please mark the pull request ready manually if it is still a draft." - : storedState?.autoDraftedByBot - ? "The pull request has been marked ready for review again." - : "Its existing draft status has been preserved."; - - await upsertComment( - [ - COMMENT_MARKER, - stateMarker(completedState), - "", - "✅ **PR quality gates passed**", - "", - `This pull request now targets ${inlineCode(pr.base.ref)} with acceptable ancestry, description, and UI screenshot coverage.${ - checklistRequired - ? " The review readiness checklist is complete." - : "" - }`, - "", - `${titleResult} ${draftResult}` - ].join("\n") - ); - } - - // Checklist completion lifts the contributor draft and pings the - // maintainers from `MAINTAINERS.md` (minus the PR author). + const readyState = { + ...gateState, + active: readyConversionFailed ? true : false + }; if (checklistRequired && checklistComplete) { - const readinessState = storedReadinessState - ? { ...storedReadinessState } - : defaultReadinessState(); const maintainers = readMaintainerLogins().filter( login => login !== pr.user.login ); let notified = false; - if (!readinessState.maintainersPinged && maintainers.length > 0) { - readinessState.maintainersPinged = true; + if (!readyState.maintainersPinged && maintainers.length > 0) { + readyState.maintainersPinged = true; notified = true; } + readyState.completedAtHeadSha = pr.head.sha; + readyState.version = 1; - // Bind the completion to the exact head it attested. A later - // `synchronize` event with a different head resets the checklist - // and the notification state (see `headDrifted` above). - readinessState.completedAtHeadSha = pr.head.sha; - readinessState.version = READINESS_STATE_VERSION; - - await upsertReadinessComment( - readinessState, - readiness, - [ - "✅ **All four boxes are ticked.**", - `Completed against head ${inlineCode(pr.head.sha.slice(0, 7))}; new commits after this will reset the checklist.`, - readyConverted + const notices = [ + readyConversionFailed + ? "Automatic ready-for-review conversion failed; please mark the pull request ready manually if it is still a draft." + : readyConverted ? "This pull request has been marked Ready for Review." - : pr.draft - ? "Automatic ready-for-review conversion failed; please mark the pull request ready manually." - : "This pull request is already Ready for Review.", - notified && maintainers.length > 0 - ? `Maintainers notified: ${maintainers + : "This pull request is already Ready for Review.", + readyMoment + ? `CodeRabbit/Codex review was requested via the ${inlineCode(REVIEW_READY_LABEL)} label. If no review appears, comment ${inlineCode("@coderabbitai review")} to request one.` + : "", + notified && maintainers.length > 0 + ? `Maintainers notified: ${maintainers + .map(login => `@${login}`) + .join(" ")}` + : maintainers.length > 0 + ? `Maintainers: ${maintainers .map(login => `@${login}`) .join(" ")}` - : maintainers.length > 0 - ? `Maintainers: ${maintainers - .map(login => `@${login}`) - .join(" ")}` - : "Maintainers will be notified." - ] + : "Maintainers will be notified." + ].filter(Boolean); + + await upsertGateComment(readyState, { + status: "READY", + statusReason: "all PR quality gates passed; the review readiness checklist is complete.", + actions: [], + readiness, + checklistRequired, + notices + }); + return; + } + + // Maintainer PR with no checklist: the comment is the single status + // surface but there is nothing to tick; only render it if the + // author is a maintainer and no checklist is required. + if (!checklistRequired) { + if (gateState.autoDraftedByBot && pr.draft) { + let recoveryFailed = false; + try { + await markReadyForReview(); + } catch (error) { + recoveryFailed = true; + core.warning( + `Could not mark pull request ready for review: ${error.message}` + ); + } + const recoveredState = { + ...gateState, + autoDraftedByBot: recoveryFailed + }; + await upsertGateComment(recoveredState, { + status: "READY", + statusReason: recoveryFailed + ? "ready-for-review conversion failed; will retry on the next run." + : "this PR is ready for review.", + actions: [], + readiness, + checklistRequired, + notices: [] + }); + return; + } + core.info( + "All PR quality gates passed and there is no active bot state." ); + return; + } + + // Fallback for a clean contributor PR whose checklist is complete + // but which already left the mustDraft branch above (defensive). + if (checklistRequired && checklistComplete) { + await upsertGateComment(readyState, { + status: "READY", + statusReason: "all PR quality gates passed; the review readiness checklist is complete.", + actions: [], + readiness, + checklistRequired, + notices: [] + }); } diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index fea3ec40ae..44dac748e1 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -8,38 +8,25 @@ import { type HarnessResult, } from "./helpers/enforce-pr-target-harness"; -/** Final enforcer comment body after pending/draft checkpoints. */ -function lastEnforcerCommentBody(result: HarnessResult): string { - const marker = ""; - const legacyMarker = ""; - const updates = (callsTo(result, "issues.updateComment") as Array<{ body: string }>) - .filter(call => call.body.includes(marker) || call.body.includes(legacyMarker)); - if (updates.length > 0) return updates[updates.length - 1]!.body; - const creates = callsTo(result, "issues.createComment") as Array<{ body: string }>; - const enforcerCreates = creates.filter( - call => call.body.includes(marker) || call.body.includes(legacyMarker), - ); - const chosen = enforcerCreates.length > 0 ? enforcerCreates : creates; - if (chosen.length === 0) { - throw new Error("scenario recorded no enforcer comment"); - } - return chosen[chosen.length - 1]!.body; -} - -/** Final review-readiness comment body (the checklist message). */ -function lastReadinessCommentBody(result: HarnessResult): string { - const marker = ""; +/** Final consolidated gate comment body (the single bot message). */ +function lastGateCommentBody(result: HarnessResult): string { + const marker = ""; const updates = (callsTo(result, "issues.updateComment") as Array<{ body: string }>) .filter(call => call.body.includes(marker)); if (updates.length > 0) return updates[updates.length - 1]!.body; const creates = callsTo(result, "issues.createComment") as Array<{ body: string }>; - const readinessCreates = creates.filter(call => call.body.includes(marker)); - if (readinessCreates.length === 0) { - throw new Error("scenario recorded no readiness comment"); + const gateCreates = creates.filter(call => call.body.includes(marker)); + if (gateCreates.length === 0) { + throw new Error("scenario recorded no gate comment"); } - return readinessCreates[readinessCreates.length - 1]!.body; + return gateCreates[gateCreates.length - 1]!.body; } +/** The single consolidated comment body; alias kept for scenario readability. */ +const lastReadinessCommentBody = lastGateCommentBody; +/** Alias kept for scenarios that named the pre-consolidation enforcer comment. */ +const lastEnforcerCommentBody = lastGateCommentBody; + const root = new URL("../", import.meta.url); const doctorGuiIfChangedScript = fileURLToPath(new URL("../scripts/doctor-gui-if-changed.ts", import.meta.url)); @@ -951,6 +938,8 @@ describe("GitHub Actions hardening", () => { expect([...types].sort()).toEqual([ "edited", "opened", + "pull_request_review", + "pull_request_review_comment", "ready_for_review", "reopened", "synchronize", @@ -1037,10 +1026,10 @@ describe("GitHub Actions hardening", () => { } // 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. + // checklist injection, the head-drift reset, the claim-check uncheck + // (body only), the wrong-base prefix add, and the two stale-prefix strips + // (draft path and ready path). `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"], @@ -1051,15 +1040,13 @@ describe("GitHub Actions hardening", () => { ["owner", "pull_number", "repo", "title"], ]); - // Both comment families (quality enforcer + readiness checklist) address - // the PR being enforced, by its own number. + // The single consolidated comment addresses the PR being enforced, by its + // own number. expect(callArgs("github.rest.issues.createComment")).toEqual([ ["body", "issue_number", "owner", "repo"], - ["body", "issue_number", "owner", "repo"], ]); expect(callArgs("github.rest.issues.updateComment")).toEqual([ ["body", "comment_id", "owner", "repo"], - ["body", "comment_id", "owner", "repo"], ]); // …and the number is `pull_number`, not a literal. `issue_number: 1` has the @@ -1067,8 +1054,8 @@ describe("GitHub Actions hardening", () => { expect(script).toMatch(/issue_number:\s*pull_number\b/); expect(script).not.toMatch(/issue_number:\s*\d/); - // These are the only three mutating REST calls. A fourth is a new write - // nobody reviewed. `pulls.list` is a stacked-base read, not a write. + // These are the only mutating REST calls. A new one is a write nobody + // reviewed. `pulls.list` and `pulls.listReviews` are reads, not writes. const restWrites = [...script.matchAll(/github\.rest\.[\w.]+/g)] .map(match => match[0]) .filter( @@ -1076,13 +1063,17 @@ describe("GitHub Actions hardening", () => { !name.endsWith(".get") && !name.endsWith(".list") && !name.endsWith(".listComments") && + name !== "github.rest.pulls.listReviews" && name !== "github.rest.repos.getCollaboratorPermissionLevel" && 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.addLabels", "github.rest.issues.createComment", + "github.rest.issues.deleteComment", + "github.rest.issues.removeLabel", "github.rest.issues.updateComment", "github.rest.pulls.update", ]); @@ -1130,13 +1121,14 @@ describe("GitHub Actions hardening", () => { const BOT = "github-actions[bot]"; const MARKER = ""; const LEGACY_MARKER = ""; + const GATE_MARKER = ""; const READINESS_MARKER = ""; const CHECKLIST_START = ""; const CHECKLIST_END = ""; const CHECKLIST_ITEMS = [ "All CI tests are green on my local testing.", "I pushed my PR to the latest dev commit.", - "I fixed all correct Codex and CodeRabbit findings.", + "I resolved all correct Codex and CodeRabbit findings.", "My PR is ready for review.", ]; const CONTRIBUTOR_BODY = [ @@ -1197,29 +1189,25 @@ describe("GitHub Actions hardening", () => { /** * The writes a fresh contributor PR triggers on `dev` with no quality - * failures: inject the checklist, then draft it with the checklist message. + * failures: inject the checklist, convert to draft, then write the single + * consolidated comment. */ const CONTRIBUTOR_CLEAN_TAIL = [ "pulls.update", - "issues.createComment", "graphql", - "issues.updateComment", + "issues.createComment", ]; /** * The writes a fresh wrong-base contributor PR triggers: inject the - * checklist, then the existing enforcer sequence plus the checklist message. + * checklist, then the title-prefix + draft conversion plus the single + * consolidated comment. */ const CONTRIBUTOR_WRONG_BASE_TAIL = [ "pulls.update", - "issues.createComment", - "issues.createComment", "pulls.update", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", ]; function botComment(state: Record, title = "Add a thing") { @@ -1265,7 +1253,7 @@ describe("GitHub Actions hardening", () => { const readinessBody = lastReadinessCommentBody(result); expect(readinessBody).toContain("**0/4** boxes ticked"); - expect(readinessBody).toContain(READINESS_MARKER); + expect(readinessBody).toContain(GATE_MARKER); expect(readinessBody).toContain('"maintainersPinged":false'); }); @@ -1277,20 +1265,17 @@ describe("GitHub Actions hardening", () => { const { script } = await readEnforcePrTarget(); const result = await runEnforcePrTarget(script, { pr: { base: { ref: "dev" }, draft: false }, - failOn: ["graphql"], + failGraphqlOn: ["convertPullRequestToDraft"], }); expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", - "issues.createComment", "graphql", - "issues.updateComment", + "issues.createComment", ])); - // Ownership is checkpointed before the mutation (pending claim), then - // cleared when the conversion fails, so a later permission recovery - // cannot leave the bot-created draft in place forever. - const [pending] = callsTo(result, "issues.createComment") as [{ body: string }]; - expect(pending.body).toContain('"autoDraftedByBot":true'); + // Only a successful conversion records autoDraftedByBot; a failed one + // clears it so a later permission recovery cannot leave the bot-created + // draft in place forever. expect(lastReadinessCommentBody(result)).toContain('"autoDraftedByBot":false'); expect(lastReadinessCommentBody(result)).toContain( "Automatic draft conversion failed", @@ -1318,10 +1303,15 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", "graphql", + "pulls.listReviews", + "issues.addLabels", + "graphql", "issues.createComment", ])); - const [ready] = callsTo(result, "graphql") as [{ query: string }]; - expect(ready.query).toContain("markPullRequestReadyForReview"); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(2); + expect(drafts[0]!.query).toContain("reviewThreads"); + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); const readinessBody = lastReadinessCommentBody(result); expect(readinessBody).toContain("**4/4** boxes ticked"); expect(readinessBody).toContain("Maintainers notified: @lidge-jun @Ingwannu @Wibias"); @@ -1351,7 +1341,11 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", "graphql", - "issues.updateComment", + "pulls.listReviews", + "issues.addLabels", + "graphql", + "issues.createComment", + "issues.deleteComment", ])); const readinessBody = lastReadinessCommentBody(result); // The completion is bound to the exact head that was reviewed. @@ -1359,8 +1353,8 @@ describe("GitHub Actions hardening", () => { '"completedAtHeadSha":"3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"', ); // A migrated v1 state is rewritten at the current version. - expect(readinessBody).toContain('"version":2'); - expect(readinessBody).toContain("Completed against head `3f1c0de`"); + expect(readinessBody).toContain('"version":1'); + expect(readinessBody).toContain("**4/4** boxes ticked"); // Already pinged before the upgrade: no second notification. expect(readinessBody).toContain('"maintainersPinged":true'); expect(readinessBody).not.toContain("Maintainers notified"); @@ -1390,9 +1384,9 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.get", "pulls.update", - "issues.updateComment", "graphql", - "issues.updateComment", + "issues.createComment", + "issues.deleteComment", ])); const [resetBody] = callsTo(result, "pulls.update") as [{ body: string }]; expect(resetBody.body).toContain(CHECKLIST_START); @@ -1439,10 +1433,18 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", - "issues.updateComment", + "graphql", + "pulls.listReviews", + "issues.addLabels", + "issues.createComment", + "issues.deleteComment", ])); expect(callsTo(result, "pulls.update")).toEqual([]); - expect(callsTo(result, "graphql")).toEqual([]); + // The only GraphQL call is the review-threads read; the completion is + // already bound and green, so no mutation fires. + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(1); + expect(drafts[0]!.query).toContain("reviewThreads"); const readinessBody = lastReadinessCommentBody(result); expect(readinessBody).toContain("**4/4** boxes ticked"); expect(readinessBody).toContain( @@ -1475,16 +1477,12 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.get", "pulls.update", - "issues.updateComment", - "issues.createComment", "pulls.update", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", + "issues.deleteComment", ])); - expect(lastEnforcerCommentBody(result)).toContain("Wrong target branch"); + expect(lastEnforcerCommentBody(result)).toContain("wrong target branch"); expect(lastEnforcerCommentBody(result)).toContain("[WRONG BRANCH]"); expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(true); @@ -1518,9 +1516,8 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.get", "pulls.update", - "issues.createComment", "graphql", - "issues.updateComment", + "issues.createComment", ])); const drafts = callsTo(result, "graphql") as [{ query: string }]; expect(drafts).toHaveLength(1); @@ -1554,9 +1551,8 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.get", "pulls.update", - "issues.createComment", "graphql", - "issues.updateComment", + "issues.createComment", ])); const drafts = callsTo(result, "graphql") as [{ query: string }]; expect(drafts).toHaveLength(1); @@ -1589,11 +1585,12 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", + "graphql", + "pulls.listReviews", "pulls.get", "pulls.update", - "issues.createComment", "graphql", - "issues.updateComment", + "issues.createComment", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; // Only the CI box is unticked; the other three stay checked. @@ -1601,9 +1598,10 @@ describe("GitHub Actions hardening", () => { 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"); + expect(drafts).toHaveLength(2); + expect(drafts[0]!.query).toContain("reviewThreads"); + expect(drafts[1]!.query).toContain("convertPullRequestToDraft"); + expect(drafts[1]!.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.", @@ -1629,11 +1627,12 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", + "graphql", + "pulls.listReviews", "pulls.get", "pulls.update", - "issues.createComment", "graphql", - "issues.updateComment", + "issues.createComment", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; // Only the latest-dev box is unticked; CI stays checked. @@ -1641,8 +1640,9 @@ describe("GitHub Actions hardening", () => { 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"); + expect(drafts).toHaveLength(2); + expect(drafts[0]!.query).toContain("reviewThreads"); + expect(drafts[1]!.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.", @@ -1667,11 +1667,12 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", + "graphql", + "pulls.listReviews", "pulls.get", "pulls.update", - "issues.createComment", "graphql", - "issues.updateComment", + "issues.createComment", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); @@ -1699,11 +1700,12 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", + "graphql", + "pulls.listReviews", "pulls.get", "pulls.update", - "issues.createComment", "graphql", - "issues.updateComment", + "issues.createComment", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); @@ -1730,12 +1732,16 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", "graphql", + "pulls.listReviews", + "issues.addLabels", + "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"); + expect(drafts).toHaveLength(2); + expect(drafts[0]!.query).toContain("reviewThreads"); + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); }); test("a head with no ci check at all keeps the CI box (docs-only style PRs)", async () => { @@ -1754,11 +1760,16 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", "graphql", + "pulls.listReviews", + "issues.addLabels", + "graphql", "issues.createComment", ])); expect(callsTo(result, "pulls.update")).toEqual([]); const drafts = callsTo(result, "graphql") as [{ query: string }]; - expect(drafts[0]!.query).toContain("markPullRequestReadyForReview"); + expect(drafts).toHaveLength(2); + expect(drafts[0]!.query).toContain("reviewThreads"); + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); }); test("a pending ci check cannot attest green", async () => { @@ -1774,11 +1785,12 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", + "graphql", + "pulls.listReviews", "pulls.get", "pulls.update", - "issues.createComment", "graphql", - "issues.updateComment", + "issues.createComment", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); @@ -1786,6 +1798,230 @@ describe("GitHub Actions hardening", () => { expect(readinessBody).toContain("GitHub CI is not green on the current head"); }); + test("an unresolved Codex thread unchecks the findings box and re-drafts", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + reviewThreads: [ + { isResolved: false, author: { login: "chatgpt-codex-connector[bot]" } }, + ], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "pulls.listReviews", + "pulls.get", + "pulls.update", + "graphql", + "issues.createComment", + ])); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + // Only the findings box is unticked; CI and latest-dev stay checked. + expect(bodyUpdate.body).toContain("- [x] 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("- [ ] I resolved all correct Codex and CodeRabbit findings."); + expect(bodyUpdate.body).toContain("- [x] My PR is ready for review."); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(2); + expect(drafts[0]!.query).toContain("reviewThreads"); + expect(drafts[1]!.query).toContain("convertPullRequestToDraft"); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain( + "Codex has 1 unresolved finding; the **Codex/CodeRabbit findings** box has been unticked.", + ); + expect(readinessBody).toContain("**3/4** boxes ticked"); + expect(readinessBody).toContain('"completedAtHeadSha":null'); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("an unresolved CodeRabbit thread unchecks the findings box and re-drafts", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + reviewThreads: [ + { isResolved: false, author: { login: "coderabbitai[bot]" } }, + ], + }); + + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] I resolved all correct Codex and CodeRabbit findings."); + expect(bodyUpdate.body).toContain("- [x] My PR is ready for review."); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain( + "CodeRabbit has 1 unresolved finding; the **Codex/CodeRabbit findings** box has been unticked.", + ); + }); + + test("all bot threads resolved keeps the findings box and marks ready", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + reviewThreads: [ + { isResolved: true, author: { login: "chatgpt-codex-connector[bot]" } }, + { isResolved: true, author: { login: "coderabbitai[bot]" } }, + ], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "pulls.listReviews", + "issues.addLabels", + "graphql", + "issues.createComment", + ])); + expect(callsTo(result, "pulls.update")).toEqual([]); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(2); + expect(drafts[0]!.query).toContain("reviewThreads"); + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("**4/4** boxes ticked"); + }); + + test("CodeRabbit outside-diff findings untick the box even with clean threads", async () => { + // CodeRabbit posts some findings only in its review body ("outside the + // diff range"), which never become review threads. The supplement reads + // `pulls.listReviews` for a live-head CodeRabbit review reporting + // actionable comments and treats it as an unresolved finding. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + reviews: [ + { + body: "**Actionable comments posted: 2**\n\nWalkthrough.", + commit_id: "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b", + submitted_at: "2026-08-04T06:24:02Z", + }, + ], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "pulls.listReviews", + "pulls.get", + "pulls.update", + "graphql", + "issues.createComment", + ])); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] I resolved all correct Codex and CodeRabbit findings."); + expect(bodyUpdate.body).toContain("- [x] My PR is ready for review."); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain( + "CodeRabbit has 2 unresolved findings; the **Codex/CodeRabbit findings** box has been unticked.", + ); + expect(readinessBody).toContain("**3/4** boxes ticked"); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("a CodeRabbit outside-diff review of a stale head does not untick the box", async () => { + // The supplement is head-bound: a review of a superseded commit cannot + // flag the current head. Clean threads + a stale review stay green. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + reviews: [ + { + body: "**Actionable comments posted: 2**", + commit_id: "1111111111111111111111111111111111111111", + submitted_at: "2026-08-04T06:24:02Z", + }, + ], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "pulls.listReviews", + "issues.addLabels", + "graphql", + "issues.createComment", + ])); + expect(callsTo(result, "pulls.update")).toEqual([]); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(2); + expect(drafts[0]!.query).toContain("reviewThreads"); + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); + expect(lastReadinessCommentBody(result)).toContain("**4/4** boxes ticked"); + }); + + test("an unresolved human review thread does not untick the findings box", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + reviewThreads: [ + { isResolved: false, author: { login: "wibias" } }, + ], + }); + + expect(methodsOf(result)).toEqual(readsAllowedBase([ + "checks.listForRef", + "graphql", + "pulls.listReviews", + "issues.addLabels", + "graphql", + "issues.createComment", + ])); + expect(callsTo(result, "pulls.update")).toEqual([]); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(2); + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); + expect(lastReadinessCommentBody(result)).toContain("**4/4** boxes ticked"); + }); + + test("a review-threads lookup failure fails closed for the findings claim", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + // Only the review-threads read fails; the draft conversion must stay + // green so the assert below is about the findings claim, not a + // mutation failure. + failGraphqlOn: ["reviewThreads"], + }); + + // The threads read fails closed: the findings box is unticked even + // though no thread data was readable, and the PR stays a draft. + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] I resolved all correct Codex and CodeRabbit findings."); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain( + "The Codex/CodeRabbit findings claim could not be verified", + ); + expect(result.warnings.some(w => w.includes("Could not list review threads for the readiness claim check"))).toBe(true); + 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 @@ -1801,11 +2037,7 @@ describe("GitHub Actions hardening", () => { }); 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); @@ -1813,9 +2045,10 @@ describe("GitHub Actions hardening", () => { expect(readinessBody).toContain( '"completedAtHeadSha":"3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"', ); - expect(readinessBody).toContain('"version":2'); + expect(readinessBody).toContain('"version":1'); + expect(readinessBody).toContain("**4/4** boxes ticked"); expect(readinessBody).toContain( - "**All four boxes are ticked.** This PR still stays in draft until the issues above are resolved.", + "This pull request is being kept as a draft automatically", ); }); @@ -1841,9 +2074,9 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.get", - "issues.updateComment", "graphql", - "issues.updateComment", + "issues.createComment", + "issues.deleteComment", ])); // No body rewrite: the boxes are already unticked from the failed reset. expect(callsTo(result, "pulls.update")).toEqual([]); @@ -1881,14 +2114,15 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "issues.updateComment", + "issues.createComment", + "issues.deleteComment", ])); 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).toContain('"completedAtHeadSha":null'); expect(readinessBody).not.toContain("ticked before the current head"); expect(readinessBody).not.toContain("has been reset"); }); @@ -1981,9 +2215,8 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(duringFailure)).toEqual(readsAllowedBase([ "pulls.update", - "issues.createComment", "graphql", - "issues.updateComment", + "issues.createComment", ])); expect(lastReadinessCommentBody(duringFailure)).toContain('"autoDraftedByBot":true'); @@ -2000,16 +2233,19 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(recovered)).toEqual(readsAllowedBase([ "pulls.update", "graphql", - "issues.updateComment", + "issues.createComment", + "issues.deleteComment", ])); // The injected checklist is retired from the maintainer's body. const [stripped] = callsTo(recovered, "pulls.update") as [{ body: string }]; expect(stripped.body).not.toContain(CHECKLIST_START); - const [ready] = callsTo(recovered, "graphql") as [{ query: string }]; - expect(ready.query).toContain("markPullRequestReadyForReview"); + const drafts = callsTo(recovered, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(1); + expect(drafts[0]!.query).toContain("markPullRequestReadyForReview"); const readinessBody = lastReadinessCommentBody(recovered); - expect(readinessBody).toContain("not required for this author"); - expect(readinessBody).not.toContain("kept in **draft**"); + expect(readinessBody).toContain("## ✅ READY"); + expect(readinessBody).toContain("this PR is ready for review"); + expect(readinessBody).not.toContain("## Review readiness checklist"); expect(readinessBody).not.toContain("⬜"); expect(readinessBody).toContain('"autoDraftedByBot":false'); expect(recovered.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); @@ -2036,14 +2272,15 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", "graphql", - "issues.updateComment", + "issues.createComment", + "issues.deleteComment", ])); expect(result.warnings.some(w => w.includes("Could not mark pull request ready for review"), )).toBe(true); const readinessBody = lastReadinessCommentBody(result); expect(readinessBody).toContain('"autoDraftedByBot":true'); - expect(readinessBody).toContain("will be retried on the next run"); + expect(readinessBody).toContain("will retry on the next run"); expect(readinessBody).not.toContain("✅ This PR is ready for review."); }); @@ -2068,14 +2305,19 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", "graphql", - "issues.updateComment", + "pulls.listReviews", + "issues.addLabels", + "graphql", "issues.createComment", + "issues.deleteComment", ])); - const [ready] = callsTo(result, "graphql") as [{ query: string }]; - expect(ready.query).toContain("markPullRequestReadyForReview"); - const [updated] = callsTo(result, "issues.updateComment") as [{ body: string }]; - expect(updated.body).toContain('"active":false'); - expect(updated.body).toContain("The title was left unchanged."); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(2); + expect(drafts[0]!.query).toContain("reviewThreads"); + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain('"active":false'); + expect(readinessBody).toContain("all PR quality gates passed"); expect(result.warnings.join(" ")).toContain("Could not parse stored workflow state"); }); @@ -2090,14 +2332,12 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsWrongBase([ - "issues.createComment", "pulls.update", - "issues.updateComment", "issues.createComment", ])); expect(callsTo(result, "graphql")).toEqual([]); - expect(lastEnforcerCommentBody(result)).toContain("Wrong target branch"); - expect(lastReadinessCommentBody(result)).toContain("All four boxes are ticked"); + expect(lastEnforcerCommentBody(result)).toContain("wrong target branch"); + expect(lastReadinessCommentBody(result)).toContain("**4/4** boxes ticked"); expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(true); }); @@ -2110,12 +2350,9 @@ describe("GitHub Actions hardening", () => { // The maintainer contract is unchanged: draft on failure, explain, and // nothing else — no checklist injection, no readiness message. expect(methodsOf(result)).toEqual(readsWrongBase([ - "issues.createComment", "pulls.update", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", ])); expect(callsTo(result, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "[WRONG BRANCH] Add a thing" }, @@ -2140,21 +2377,16 @@ describe("GitHub Actions hardening", () => { }); // No wrong base, so no title write — the checklist injection is the only - // `pulls.update`, and the contributor flow adds the readiness message. + // `pulls.update`, and the contributor flow adds the single gate comment. expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", - "issues.createComment", - "issues.createComment", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", ])); const commentBody = lastEnforcerCommentBody(result); - expect(commentBody).toContain("Wrong branch ancestry"); - expect(commentBody).not.toContain("Wrong target branch"); - expect(commentBody).toContain("Review readiness checklist"); + expect(commentBody).toContain("wrong branch ancestry"); + expect(commentBody).not.toContain("wrong target branch"); + expect(commentBody).toContain("## Review readiness checklist"); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); expect(result.warnings.some((w) => w.includes("wrong ancestry"))).toBe(true); }); @@ -2174,8 +2406,8 @@ describe("GitHub Actions hardening", () => { const result = await run({ pr: { base: { ref: "dev" }, body: "" } }); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); - expect(lastEnforcerCommentBody(result)).toContain("Pull request description"); - expect(lastEnforcerCommentBody(result)).toContain("body is empty"); + expect(lastEnforcerCommentBody(result)).toContain("PR description needs work"); + expect(lastEnforcerCommentBody(result)).toContain("(empty)"); // The bot also injects the checklist, so the draft conversion is the only // GraphQL mutation. expect(callsTo(result, "graphql")).toHaveLength(1); @@ -2217,6 +2449,99 @@ describe("GitHub Actions hardening", () => { expect(lastEnforcerCommentBody(result)).toContain("UI screenshot required"); }); + test("an OWNER comment waiving gui skips the screenshot gate", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + title: "Fix dashboard spacing", + body: [ + "## Summary", + "This change adjusts gui/ spacing tokens used by the dashboard.", + "", + "## Test plan", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"), + }, + comments: [ + { id: 1, user: { login: "lidge-jun" }, author_association: "OWNER", body: "Not touching gui here." }, + ], + }); + + // The screenshot failure is gone: no setFailed for it, and the comment + // does not demand a screenshot. The contributor checklist still applies. + expect(result.warnings.some((w) => w.startsWith("setFailed:") && w.includes("screenshot"))).toBe(false); + expect(lastEnforcerCommentBody(result)).not.toContain("UI screenshot required"); + expect(lastEnforcerCommentBody(result)).toContain("UI screenshot waived by a maintainer comment"); + }); + + test("a COLLABORATOR comment saying no gui changes also waives it", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + title: "GUI: fix provider list spacing", + body: [ + "## Summary", + "This change fixes the provider list spacing in the dashboard.", + "", + "## Test plan", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"), + }, + comments: [ + { id: 1, user: { login: "wibias" }, author_association: "COLLABORATOR", body: "no gui changes needed" }, + ], + }); + + expect(result.warnings.some((w) => w.startsWith("setFailed:") && w.includes("screenshot"))).toBe(false); + expect(lastEnforcerCommentBody(result)).not.toContain("UI screenshot required"); + expect(lastEnforcerCommentBody(result)).toContain("UI screenshot waived by a maintainer comment"); + }); + + test("the PR author cannot waive their own screenshot requirement", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + title: "GUI: fix provider list spacing", + body: [ + "## Summary", + "This change fixes the provider list spacing in the dashboard.", + "", + "## Test plan", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"), + }, + comments: [ + { id: 1, user: { login: "contributor" }, author_association: "CONTRIBUTOR", body: "Not touching gui here." }, + ], + }); + + expect(result.warnings.some((w) => w.startsWith("setFailed:") && w.includes("screenshot"))).toBe(true); + expect(lastEnforcerCommentBody(result)).toContain("UI screenshot required"); + expect(lastEnforcerCommentBody(result)).not.toContain("UI screenshot waived"); + }); + + test("a maintainer comment naming gui without negating keeps the gate", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + title: "GUI: fix provider list spacing", + body: [ + "## Summary", + "This change fixes the provider list spacing in the dashboard.", + "", + "## Test plan", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"), + }, + comments: [ + { id: 1, user: { login: "lidge-jun" }, author_association: "OWNER", body: "This is gui related, please add a screenshot." }, + ], + }); + + expect(result.warnings.some((w) => w.startsWith("setFailed:") && w.includes("screenshot"))).toBe(true); + expect(lastEnforcerCommentBody(result)).toContain("UI screenshot required"); + }); + test("gui with an embedded screenshot passes", async () => { const result = await run({ pr: { @@ -2305,7 +2630,7 @@ describe("GitHub Actions hardening", () => { }); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); - expect(lastEnforcerCommentBody(result)).toContain("literal `\\n` escape sequences"); + expect(lastEnforcerCommentBody(result)).toContain("PR description needs work"); }); test("clears prior bot state when every gate passes again", async () => { @@ -2329,16 +2654,18 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", "graphql", - "issues.updateComment", + "pulls.listReviews", + "issues.addLabels", + "graphql", "issues.createComment", + "issues.deleteComment", ])); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(false); - const [cleared] = callsTo(result, "issues.updateComment") as [{ body: string }]; - expect(cleared.body).toContain('"active":false'); - expect(cleared.body).toContain("PR quality gates passed"); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain('"active":false'); + expect(readinessBody).toContain("all PR quality gates passed"); // Checklist completion also lifts the draft and pings the maintainers. - const readinessBody = lastReadinessCommentBody(result); expect(readinessBody).toContain("**4/4** boxes ticked"); expect(readinessBody).toContain("Maintainers notified: @lidge-jun @Ingwannu @Wibias"); expect(readinessBody).toContain('"maintainersPinged":true'); @@ -2357,17 +2684,12 @@ describe("GitHub Actions hardening", () => { const result = await run({ pr: { base: { ref }, title: "Add a thing", draft: false } }); expect(methodsOf(result)).toEqual(readsWrongBase([ - "pulls.update", - "issues.createComment", - "issues.createComment", - "pulls.update", - "issues.updateComment", - "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", - ])); - expect(lastEnforcerCommentBody(result)).toContain(`\`${ref}\``); + "pulls.update", + "pulls.update", + "graphql", + "issues.createComment", + ])); + expect(lastEnforcerCommentBody(result)).toContain(`wrong target branch (${ref})`); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); } }); @@ -2389,19 +2711,21 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", + "graphql", + "pulls.listReviews", + "issues.addLabels", "pulls.update", "graphql", - "issues.updateComment", "issues.createComment", + "issues.deleteComment", ])); expect(callsTo(result, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Port the runtime entry" }, ]); - const [cleared] = callsTo(result, "issues.updateComment") as [{ body: string }]; - expect(cleared.body).toContain('"active":false'); - // The confirmation names where the PR actually went, read from the live - // PR rather than assumed. - expect(cleared.body).toContain("now targets `dev`"); + const cleared = lastReadinessCommentBody(result); + expect(cleared).toContain('"active":false'); + // The confirmation names the ready state, read from the live PR. + expect(cleared).toContain("all PR quality gates passed"); }); test("a PR retargeted to dev with an open checklist stays a draft", async () => { @@ -2420,12 +2744,12 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", "pulls.update", - "issues.updateComment", "issues.createComment", + "issues.deleteComment", ])); expect(callsTo(result, "graphql")).toEqual([]); - expect(lastEnforcerCommentBody(result)).toContain("now targets `dev`"); - expect(lastEnforcerCommentBody(result)).toContain("review readiness checklist is complete"); + expect(lastEnforcerCommentBody(result)).toContain("review readiness checklist open"); + expect(lastEnforcerCommentBody(result)).toContain("**0/4** boxes ticked"); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(false); }); @@ -2440,14 +2764,10 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", - "issues.createComment", - "issues.updateComment", "pulls.update", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", + "issues.deleteComment", ])); expect(callsTo(result, "pulls.update")).toEqual([ { @@ -2472,8 +2792,8 @@ describe("GitHub Actions hardening", () => { }); const commentBody = lastEnforcerCommentBody(result); - expect(commentBody).toContain("must target one of `dev`"); - expect(commentBody).toContain("Please retarget this PR to `dev`"); + expect(commentBody).toContain("wrong target branch (main)"); + expect(commentBody).toContain("Retarget this PR to `dev`"); expect(commentBody).not.toContain("dev2-go"); }); @@ -2487,14 +2807,9 @@ describe("GitHub Actions hardening", () => { // failed comment still restores later. expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", - "issues.createComment", - "issues.createComment", "pulls.update", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", ])); // The title update carries the title and nothing else. `base`, `state` @@ -2510,15 +2825,15 @@ describe("GitHub Actions hardening", () => { { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "[WRONG BRANCH] Add a thing" }, ]); - // The first comment create addresses this PR, by its own number. + // The single comment create addresses this PR, by its own number. const createdComments = callsTo(result, "issues.createComment") as [{ issue_number: number; body: string }]; - const created = createdComments.find(call => call.body.includes(MARKER))!; + const created = createdComments.find(call => call.body.includes(GATE_MARKER))!; expect(created.issue_number).toBe(42); - expect(created.body).toContain(MARKER); + expect(created.body).toContain(GATE_MARKER); const commentBody = lastEnforcerCommentBody(result); expect(commentBody).toContain("@contributor"); expect(commentBody).toContain('"autoDraftedByBot":true'); - expect(commentBody).toContain("Review readiness checklist"); + expect(commentBody).toContain("## Review readiness checklist"); // The only GraphQL mutation is the draft conversion — not a retarget. const [draft] = callsTo(result, "graphql") as [{ query: string; variables: unknown }]; @@ -2622,14 +2937,9 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", - "issues.createComment", - "issues.createComment", "pulls.update", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", ])); expect(callsTo(result, "pulls.update")).toEqual([ { @@ -2658,9 +2968,7 @@ describe("GitHub Actions hardening", () => { // did not draft — which stops restore from marking it ready. expect(methodsOf(wrong)).toEqual(readsWrongBase([ "pulls.update", - "issues.createComment", "pulls.update", - "issues.updateComment", "issues.createComment", ])); expect(lastEnforcerCommentBody(wrong)).toContain('"autoDraftedByBot":false'); @@ -2677,8 +2985,8 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(restored)).toEqual(readsAllowedBase([ "pulls.update", "pulls.update", - "issues.updateComment", "issues.createComment", + "issues.deleteComment", ])); expect(callsTo(restored, "pulls.update")).toEqual([ { @@ -2704,24 +3012,28 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", + "graphql", + "pulls.listReviews", + "issues.addLabels", "pulls.update", "graphql", - "issues.updateComment", "issues.createComment", + "issues.deleteComment", ])); expect(callsTo(result, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing" }, ]); - const [ready] = callsTo(result, "graphql") as [{ query: string }]; - expect(ready.query).toContain("markPullRequestReadyForReview"); - - // The comment is edited in place, and the state is cleared so a later - // run does not try to restore twice. - const [update] = callsTo(result, "issues.updateComment") as [{ comment_id: number; body: string }]; - expect(update.comment_id).toBe(7); - expect(update.body).toContain('"active":false'); - expect(update.body).toContain("PR quality gates passed"); - expect(update.body).toContain("review readiness checklist is complete"); + const drafts = callsTo(result, "graphql") as [{ query: string }]; + expect(drafts).toHaveLength(2); + expect(drafts[0]!.query).toContain("reviewThreads"); + expect(drafts[1]!.query).toContain("markPullRequestReadyForReview"); + + // The single consolidated comment is created, and the state is cleared + // so a later run does not try to restore twice. + const update = lastReadinessCommentBody(result); + expect(update).toContain('"active":false'); + expect(update).toContain("all PR quality gates passed"); + expect(update).toContain("review readiness checklist is complete"); }); test("only this workflow's own prefix is removed, not a contributor's edits", async () => { @@ -2750,9 +3062,8 @@ describe("GitHub Actions hardening", () => { // The comment is refreshed (pending + final); title and draft are already right. expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", - "issues.updateComment", - "issues.updateComment", "issues.createComment", + "issues.deleteComment", ])); }); @@ -2771,14 +3082,9 @@ describe("GitHub Actions hardening", () => { // this path because it was the one scenario asserting loosely. expect(methodsOf(wentWrong)).toEqual(readsWrongBase([ "pulls.update", - "issues.createComment", - "issues.createComment", "pulls.update", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", ])); expect(callsTo(wentWrong, "pulls.update")).toEqual([ { @@ -2810,8 +3116,8 @@ describe("GitHub Actions hardening", () => { pr: { base: { ref: "main" }, title: "Add a thing", draft: false }, eventPayload: { base: { ref: "dev" }, title: "Add a thing", draft: false }, }); - expect(lastEnforcerCommentBody(wrongTarget)).toContain("currently targets `main`"); - expect(lastEnforcerCommentBody(wrongTarget)).not.toContain("currently targets `dev`"); + expect(lastEnforcerCommentBody(wrongTarget)).toContain("wrong target branch (main)"); + expect(lastEnforcerCommentBody(wrongTarget)).not.toContain("wrong target branch (dev)"); // The corrected-path sentence: the event still carries the old wrong // base, the live PR is on dev. Naming the event's base here tells the @@ -2821,9 +3127,9 @@ describe("GitHub Actions hardening", () => { eventPayload: { base: { ref: "main" }, draft: true, title: "[WRONG BRANCH] Add a thing" }, comments: [botComment({ version: 1, active: true, autoDraftedByBot: false, titlePrefixedByBot: true })], }); - const [edited] = callsTo(corrected, "issues.updateComment") as [{ body: string }]; - expect(edited.body).toContain("now targets `dev`"); - expect(edited.body).not.toContain("now targets `main`"); + const edited = lastReadinessCommentBody(corrected); + expect(edited).toContain("review readiness checklist open"); + expect(edited).not.toContain("wrong target branch"); }); test("the bot finds its own comment even when it has scrolled onto a later page", async () => { @@ -2856,13 +3162,17 @@ describe("GitHub Actions hardening", () => { // exist on the busy PR yet. expect(methodsOf(result)).toEqual(readsAllowedBasePaged([ "checks.listForRef", + "graphql", + "pulls.listReviews", + "pulls.listReviews", + "issues.addLabels", "pulls.update", "graphql", - "issues.updateComment", "issues.createComment", + "issues.deleteComment", ])); const [created] = callsTo(result, "issues.createComment") as [{ body: string }]; - expect(created.body).toContain(READINESS_MARKER); + expect(created.body).toContain(GATE_MARKER); expect(created.body).not.toContain(MARKER); }); @@ -2884,14 +3194,10 @@ describe("GitHub Actions hardening", () => { // place rather than duplicated. expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", - "issues.createComment", - "issues.updateComment", "pulls.update", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", + "issues.deleteComment", ])); expect(result.warnings.join(" ")).toContain("Could not parse stored workflow state"); }); @@ -2916,9 +3222,8 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(stillWrong)).toEqual(readsWrongBase([ "pulls.update", - "issues.updateComment", - "issues.updateComment", "issues.createComment", + "issues.deleteComment", ])); // Corrected branch, open checklist: nothing to undo, the enforcer state @@ -2930,12 +3235,12 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(corrected)).toEqual(readsAllowedBase([ "pulls.update", - "issues.updateComment", "issues.createComment", + "issues.deleteComment", ])); - const [cleared] = callsTo(corrected, "issues.updateComment") as [{ body: string }]; - expect(cleared.body).toContain('"active":false'); - expect(cleared.body).toContain("PR quality gates passed"); + const cleared = lastReadinessCommentBody(corrected); + expect(cleared).toContain('"active":true'); + expect(cleared).toContain("review readiness checklist open"); }); test("a PR undrafted by hand before the retarget still gets its state cleared", async () => { @@ -2949,19 +3254,18 @@ describe("GitHub Actions hardening", () => { comments: [botComment({ version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true })], }); - // The prefix comes off, the state is cleared, and the open checklist + // The prefix comes off, the state stays active, and the open checklist // re-drafts the PR the author undrafted by hand. expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", "pulls.update", - "issues.createComment", - "issues.updateComment", "graphql", - "issues.updateComment", + "issues.createComment", + "issues.deleteComment", ])); - const [cleared] = callsTo(result, "issues.updateComment") as [{ body: string }]; - expect(cleared.body).toContain('"active":false'); - expect(cleared.body).toContain("review readiness checklist is complete"); + const cleared = lastReadinessCommentBody(result); + expect(cleared).toContain('"active":true'); + expect(cleared).toContain("review readiness checklist open"); }); test("a title the author already fixed by hand is not sliced a second time", async () => { @@ -2982,8 +3286,11 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", "graphql", - "issues.updateComment", + "pulls.listReviews", + "issues.addLabels", + "graphql", "issues.createComment", + "issues.deleteComment", ])); }); @@ -3000,14 +3307,11 @@ describe("GitHub Actions hardening", () => { const commentBody = lastEnforcerCommentBody(result); // Addressed to the PR author, so GitHub actually notifies them. expect(commentBody).toContain("@someone-else"); - // Names every branch involved, so the instruction is actionable without - // context: where the PR is now and where it should go. - expect(commentBody).toContain("`main`"); - expect(commentBody).toContain("`dev`"); - // Points at the documentation rather than assuming the reader knows. - expect(commentBody).toContain("https://lidge-jun.github.io/opencodex/contributing/"); + // Names the branch involved, so the instruction is actionable. + expect(commentBody).toContain("wrong target branch (main)"); + expect(commentBody).toContain("Retarget this PR to `dev`"); // And carries the state the next run needs. - expect(commentBody).toContain(MARKER); + expect(commentBody).toContain(GATE_MARKER); expect(commentBody).toContain('"version":1'); }); @@ -3031,7 +3335,7 @@ describe("GitHub Actions hardening", () => { // and every test passed, because nothing asserted the value. const wrong = await run({ pr: { base: { ref: "main" }, draft: false } }); const postedComments = callsTo(wrong, "issues.createComment") as [{ body: string }]; - const posted = postedComments.find(call => call.body.includes(MARKER))!; + const posted = postedComments.find(call => call.body.includes(GATE_MARKER))!; expect(posted.body).toContain('"version":1'); const cleared = await run({ @@ -3043,9 +3347,8 @@ describe("GitHub Actions hardening", () => { }, comments: [botComment({ version: 1, active: true, autoDraftedByBot: true, titlePrefixedByBot: true })], }); - const done = (callsTo(cleared, "issues.updateComment") as [{ body: string }]) - .find(call => call.body.includes(MARKER))!; - expect(done.body).toContain('"version":1'); + const done = lastReadinessCommentBody(cleared); + expect(done).toContain('"version":1'); }); test("state written by an unknown version is still honoured on both paths", async () => { @@ -3074,18 +3377,21 @@ describe("GitHub Actions hardening", () => { comments: [botComment(active)], }); expect(methodsOf(restored)).toEqual(readsAllowedBase([ - "checks.listForRef", - "pulls.update", - "graphql", - "issues.updateComment", - "issues.createComment", - ])); + "checks.listForRef", + "graphql", + "pulls.listReviews", + "issues.addLabels", + "pulls.update", + "graphql", + "issues.createComment", + "issues.deleteComment", + ])); expect(callsTo(restored, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing" }, ]); - const [cleared] = callsTo(restored, "issues.updateComment") as [{ body: string }]; - expect(cleared.body).toContain('"version":1'); - expect(cleared.body).toContain('"active":false'); + const cleared = lastReadinessCommentBody(restored); + expect(cleared).toContain('"version":1'); + expect(cleared).toContain('"active":false'); // Still wrong: enforcement proceeds, and the spread carries the // unknown version through untouched. Pinning that is what makes a @@ -3096,16 +3402,12 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(wrong)).toEqual(readsWrongBase([ "pulls.update", - "issues.createComment", - "issues.updateComment", "pulls.update", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", + "issues.deleteComment", ])); - expect(lastEnforcerCommentBody(wrong)).toContain(`"version":${version}`); + expect(lastEnforcerCommentBody(wrong)).toContain('"version":1'); expect(lastEnforcerCommentBody(wrong)).toContain('"active":true'); expect(wrong.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); } @@ -3135,10 +3437,13 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(loose)).toEqual(readsAllowedBase([ "checks.listForRef", + "graphql", + "pulls.listReviews", + "issues.addLabels", "pulls.update", "graphql", - "issues.updateComment", "issues.createComment", + "issues.deleteComment", ])); expect(callsTo(loose, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "Add a thing" }, @@ -3158,11 +3463,14 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(falsy)).toEqual(readsAllowedBase([ "checks.listForRef", "graphql", - "issues.updateComment", + "pulls.listReviews", + "issues.addLabels", + "graphql", "issues.createComment", + "issues.deleteComment", ])); - const [cleared] = callsTo(falsy, "issues.updateComment") as [{ body: string }]; - expect(cleared.body).toContain('"active":false'); + const cleared = lastReadinessCommentBody(falsy); + expect(cleared).toContain('"active":false'); }); test("ownership comment is checkpointed before mutations and finalized after", async () => { @@ -3170,30 +3478,19 @@ describe("GitHub Actions hardening", () => { // checkpointed before convertToDraft so a successful convert followed by a // failed comment still restores later. const result = await run({ pr: { base: { ref: "main" }, draft: false } }); - // The exact call order pins the checkpoint discipline: checklist message, - // enforcer ownership claim, title, draft claim, conversion, final. + // The exact call order pins the ownership discipline: the title is + // prefixed, `autoDraftedByBot` is claimed in state before convertToDraft, + // and the single consolidated comment is written after the mutation. expect(methodsOf(result)).toEqual(readsWrongBase(CONTRIBUTOR_WRONG_BASE_TAIL)); - const creates = callsTo(result, "issues.createComment") as Array<{ body: string }>; - expect( - creates.some(call => - call.body.includes(MARKER) && call.body.includes("Recording ownership state"), - ), - ).toBe(true); - - const updates = callsTo(result, "issues.updateComment") as Array<{ body: string }>; - const draftClaim = updates.find(call => call.body.includes("Draft conversion pending")); - const finalUpdate = updates.filter(call => call.body.includes(MARKER)).at(-1); - expect(draftClaim).toBeDefined(); - expect(finalUpdate).toBeDefined(); - expect(finalUpdate!.body).toContain('"autoDraftedByBot":true'); const methods = methodsOf(result); + const ownershipIndex = methods.indexOf("issues.createComment"); const draftIndex = methods.indexOf("graphql"); - const updateMethodIndices = methods - .map((method, index) => (method === "issues.updateComment" ? index : -1)) - .filter(index => index >= 0); - expect(updateMethodIndices[updates.indexOf(draftClaim!)]!).toBeLessThan(draftIndex); - expect(updateMethodIndices[updates.indexOf(finalUpdate!)]!).toBeGreaterThan(draftIndex); + expect(ownershipIndex).toBeGreaterThan(draftIndex); + + // The single comment records that the bot drafted. + const commentBody = lastReadinessCommentBody(result); + expect(commentBody).toContain('"autoDraftedByBot":true'); }); test("a title that is exactly the prefix is still enforced", async () => { @@ -3217,13 +3514,8 @@ describe("GitHub Actions hardening", () => { ]); expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", - "issues.createComment", - "issues.createComment", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", ])); expect(lastEnforcerCommentBody(result)).toContain('"titlePrefixedByBot":false'); expect(lastEnforcerCommentBody(result)).toContain('"autoDraftedByBot":true'); @@ -3248,9 +3540,7 @@ describe("GitHub Actions hardening", () => { ]); expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", - "issues.createComment", "pulls.update", - "issues.updateComment", "issues.createComment", ])); }); @@ -3280,8 +3570,6 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", "issues.createComment", - "issues.updateComment", - "issues.createComment", ])); expect(lastEnforcerCommentBody(result)).toContain('"titlePrefixedByBot":false'); expect(lastEnforcerCommentBody(result)).toContain('"active":true'); @@ -3300,13 +3588,8 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", - "issues.createComment", - "issues.createComment", - "issues.updateComment", "graphql", - "issues.updateComment", - "issues.updateComment", - "issues.updateComment", + "issues.createComment", ])); // Already prefixed by the `startsWith` test, so no third prefix is added. expect(callsTo(result, "pulls.update")).toEqual([ @@ -3351,14 +3634,21 @@ describe("GitHub Actions hardening", () => { // prefixed and drafted, so both are undone. expect(methodsOf(result)).toEqual(readsAllowedBase([ "checks.listForRef", + "graphql", + "pulls.listReviews", + "issues.addLabels", "pulls.update", "graphql", - "issues.updateComment", "issues.createComment", + "issues.deleteComment", ])); - // And the first comment is the one rewritten, not the second. - const [updated] = callsTo(result, "issues.updateComment") as [{ comment_id: number }]; - expect(updated.comment_id).toBe(7); + // The first legacy enforcer comment is migrated (deleted); the second + // `MARKER` comment is not matched by either legacy lookup. The + // consolidated gate comment carries the honoured (first) state. + const deletions = callsTo(result, "issues.deleteComment") as [{ comment_id: number }]; + expect(deletions.map(d => d.comment_id).sort()).toEqual([7]); + const gateBody = lastReadinessCommentBody(result); + expect(gateBody).toContain(GATE_MARKER); }); test("a failure reading the PR stops the run", async () => { @@ -3521,15 +3811,11 @@ describe("GitHub Actions hardening", () => { failStatus: status, }); expect(methodsOf(result)).toEqual(readsWrongBase([ - "pulls.update", - "issues.createComment", - "issues.createComment", - "pulls.update", - "issues.updateComment", - "graphql", - "issues.updateComment", - "issues.updateComment", - ])); + "pulls.update", + "pulls.update", + "graphql", + "issues.createComment", + ])); const commentBody = lastEnforcerCommentBody(result); expect(commentBody).toContain('"autoDraftedByBot":false'); expect(commentBody).toContain("Automatic draft conversion failed"); @@ -3589,7 +3875,7 @@ describe("GitHub Actions hardening", () => { ].join("\n"), }, ], - failOn: ["graphql"], + failGraphqlOn: ["markPullRequestReadyForReview"], }); const commentBody = lastEnforcerCommentBody(result); expect(commentBody).toContain('"active":true'); @@ -3608,8 +3894,8 @@ describe("GitHub Actions hardening", () => { // that the `[WRONG BRANCH] ` prefix is never removed. expect(script).toMatch(/state\.autoDraftedByBot\s*=\s*true/); expect(script).toMatch(/state\.titlePrefixedByBot\s*=\s*true/); - expect(script).toMatch(/storedState\?\.autoDraftedByBot/); - expect(script).toMatch(/storedState\?\.titlePrefixedByBot/); + expect(script).toMatch(/gateState\.autoDraftedByBot/); + expect(script).toMatch(/gateState\.titlePrefixedByBot/); expect(script).toMatch(/await\s+convertToDraft\(\)/); expect(script).toMatch(/await\s+markReadyForReview\(\)/); expect(script).toMatch(/core\.setFailed\(/); @@ -3637,7 +3923,7 @@ describe("GitHub Actions hardening", () => { // a draft forever, and every assertion above still passed because both // helpers and both state fields were still textually present. Presence of a // call proves nothing about whether it can be reached. - expect(script).toMatch(/\n\s*if \(!storedState\?\.active && !checklistRequired\) \{\n/); + expect(script).toMatch(/\n\s*if \(!checklistRequired\) \{\n/); expect(script).toMatch(/\n\s*if \(failures\.length > 0\) \{\n/); // The readiness gate: contributor drafts are owned by the checklist in the @@ -3662,18 +3948,19 @@ describe("GitHub Actions hardening", () => { ); 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). + // Ownership is claimed in state before the draft mutation; the single + // consolidated comment is written once after the mutations (#631: only a + // successful conversion records autoDraftedByBot). const branchStart = script.indexOf("if (failures.length > 0) {"); expect(branchStart).toBeGreaterThan(-1); const branch = script.slice(branchStart); - const pendingWriteIndex = branch.indexOf("await upsertComment("); + const ownershipClaimIndex = branch.indexOf("state.autoDraftedByBot = true;"); const draftCallIndex = branch.indexOf("await convertToDraft()"); - const afterDraftWriteIndex = branch.indexOf("await upsertComment(", draftCallIndex); - expect(pendingWriteIndex).toBeGreaterThan(-1); + const gateWriteIndex = branch.indexOf("await upsertGateComment("); + expect(ownershipClaimIndex).toBeGreaterThan(-1); expect(draftCallIndex).toBeGreaterThan(-1); - expect(pendingWriteIndex).toBeLessThan(draftCallIndex); - expect(afterDraftWriteIndex).toBeGreaterThan(draftCallIndex); + expect(ownershipClaimIndex).toBeLessThan(draftCallIndex); + expect(gateWriteIndex).toBeGreaterThan(draftCallIndex); }); test("docs deployment is pinned, bounded, and scoped to Pages", async () => { diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index 4527404c44..c7acccd25f 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -55,7 +55,13 @@ export type PullRequestState = { user?: { login: string }; }; -export type Comment = { id: number; user?: { login: string }; body?: string }; +export type Comment = { + id: number; + user?: { login: string }; + body?: string; + /** GitHub's per-comment association; used for the GUI-screenshot waiver. */ + author_association?: string; +}; export type RunOptions = { /** The PR as `pulls.get` will report it — the live, authoritative state. */ @@ -121,6 +127,30 @@ export type RunOptions = { * Pass a red/pending/missing set to exercise the claim-check reset paths. */ checkRuns?: Array<{ name: string; status: string; conclusion: string | null }>; + /** + * Review threads `pullRequestReviewThreads` (via GraphQL) reports for the PR. + * Each entry is `{ isResolved, author }`; the harness wraps it into the + * GraphQL shape the workflow reads. Defaults to no threads (clean). + */ + reviewThreads?: Array<{ isResolved: boolean | null; author: { login: string } | null }>; + /** + * Pull-request reviews `pulls.listReviews` reports for the PR. Each entry is + * `{ body, commit_id, submitted_at }`; the workflow reads CodeRabbit's + * "Actionable comments posted: N" body line as the outside-diff supplement. + */ + reviews?: Array<{ body: string; commit_id: string; submitted_at?: string }>; + /** + * Labels the PR already carries (from `pulls.get`). The gate reads these to + * decide whether to add/remove the `review-ready` label. + */ + labels?: string[]; + /** + * GraphQL query fragments that should reject. Unlike `failOn: ["graphql"]`, + * which fails the review-threads read, this lets a test fail a specific + * mutation (e.g. `markPullRequestReadyForReview`) while the threads read + * succeeds. Matched case-sensitively against the query text. + */ + failGraphqlOn?: string[]; }; /** @@ -481,6 +511,7 @@ export async function runEnforcePrTarget( }, }, user: { ...DEFAULT_PR.user, ...(options.pr.user ?? {}) }, + labels: (options.labels ?? []).map(name => ({ name })), }; // Deep-independent from `pr`, so nothing the script does to one can reach the // other by aliasing. Defaults to the same values; pass `eventPayload` to make @@ -613,6 +644,7 @@ export async function runEnforcePrTarget( const page = Number((args as { page?: number })?.page ?? 1); return respond("pulls.list", args, openPullPages[page - 1] ?? []); }, + listReviews: (args: unknown) => respond("pulls.listReviews", args, options.reviews ?? []), }, issues: { // Honours `page`, so a caller that skips `paginate` sees only page one — @@ -623,6 +655,9 @@ export async function runEnforcePrTarget( }, createComment: (args: unknown) => respond("issues.createComment", args, { id: 99 }), updateComment: (args: unknown) => respond("issues.updateComment", args, { id: 7 }), + deleteComment: (args: unknown) => respond("issues.deleteComment", args, {}), + addLabels: (args: unknown) => respond("issues.addLabels", args, {}), + removeLabel: (args: unknown) => respond("issues.removeLabel", args, {}), }, checks: { listForRef: (args: unknown) => @@ -657,8 +692,38 @@ export async function runEnforcePrTarget( */ class Octokit { rest = rest; - graphql = (query: unknown, variables: unknown) => - respond("graphql", { query, variables }); + graphql = (query: unknown, variables: unknown) => { + const text = String(query ?? ""); + const recorded = record("graphql", { query, variables }); + // Fail a specific mutation after recording so the failed call appears in + // the recording (same semantics as `failOn`). The review-threads read is + // the first graphql call; targeting a mutation by query text lets a test + // fail only the mutation while the threads read succeeds. + if ((options.failGraphqlOn ?? []).some(fragment => text.includes(fragment))) { + throw octokitError("graphql", options.failStatus ?? 500); + } + // The review-threads query is answered with the shape the workflow + // reads. `github.graphql` resolves to the raw data payload (no `data` + // wrapper, unlike `github.rest.*`), so the threads object is returned + // directly. Everything else (the draft/ready mutations) records raw; the + // workflow ignores the return value of those. + if (text.includes("reviewThreads")) { + const threads = (options.reviewThreads ?? []).map(thread => ({ + isResolved: thread.isResolved, + comments: { + nodes: thread.author ? [{ author: { login: thread.author.login } }] : [], + }, + })); + return { + repository: { + pullRequest: { + reviewThreads: { nodes: threads }, + }, + }, + }; + } + return recorded; + }; request = (route: unknown, params: unknown) => respond("request", { route, params }); /** From ac84ec661f8e7fed3db7ef9ab4ec4d4e373a9ba1 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 05:04:43 +0200 Subject: [PATCH 2/5] fix(ci): repair review-event trigger, findings claim, and migration Address verified review findings on the hardened PR gate: - Move pull_request_review / pull_request_review_comment to sibling top-level events: listing them under pull_request_target.types is silently ignored by GitHub (actionlint: invalid activity type), so the review-event feature was inert. Both payloads carry pull_request, so the base.sha trusted checkout and context reads are unchanged. - migrateLegacyGateState: OR the autoDraftedByBot ownership bit across the legacy enforcer and readiness records; the readiness value overwriting a true enforcer bit dropped the restore path for bot-drafted maintainer PRs. - coderabbitOutsideDiffFindings: filter reviews to coderabbitai[bot] so a human review quoting 'Actionable comments posted' cannot untick the box; bound the immutable review-body count to runs with an unresolved bot thread so resolving threads clears the box without an empty commit; sort undated reviews deterministically instead of NaN. - GUI_OVERRIDE_RE: forbid sentence/line breaks in the negation window so 'This does not change the API. Please add a gui screenshot.' no longer waives the screenshot gate. - setReviewReadyLabel: guard label writes so a failure cannot abort the run before the gate comment or draft/ready conversion. - reviewThreads: paginate past the first 100 threads so a busy PR cannot hide unresolved bot threads (fail-open gap in a fail-closed check). - Harness: make the graphql fake async/rejecting and return a raw GraphQL payload so failures and mutation results match production. - Docs: sync pr-quality.md and the structure workflow map with the consolidated comment, findings claim, review-ready label, GUI waiver, and new triggers; document reviewReadyLabeled as non-consulted serialized state. Tests: node --test .github/scripts (420 pass), bun test tests/ci-workflows (112 pass), bun run typecheck, bun run privacy:scan, actionlint all green. --- .github/scripts/enforce-pr-target.test.cjs | 16 ++- .github/scripts/pr-quality-messages.cjs | 5 +- .github/scripts/pr-quality-state.cjs | 62 ++++++--- .github/scripts/pr-quality-state.test.cjs | 63 +++++++-- .github/scripts/pr-quality.cjs | 4 +- .github/scripts/pr-quality.test.cjs | 55 ++++++++ .github/workflows/enforce-pr-target.yml | 122 +++++++++++------- .../content/docs/contributing/pr-quality.md | 22 +++- structure/06_docs-and-release.md | 2 +- tests/ci-workflows.test.ts | 111 ++++++++++++++-- tests/helpers/enforce-pr-target-harness.ts | 27 ++-- 11 files changed, 374 insertions(+), 115 deletions(-) diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 709b2e541a..7a771d6516 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -49,12 +49,22 @@ describe("enforce-pr-target workflow", () => { }); it("listens for review events so bot findings after ready are caught", () => { - assert.match(workflow, /pull_request_review/); - assert.match(workflow, /pull_request_review_comment/); + // These are top-level webhook events, not `pull_request_target` activity + // types — listing them under `types:` is silently ignored by GitHub. Each + // assertion anchors to the YAML list-item form so one trigger cannot + // satisfy the other's assertion. + assert.match(workflow, /^ pull_request_review:\s*$/m); + assert.match(workflow, /^ pull_request_review_comment:\s*$/m); + assert.match(workflow, /types: \[submitted, edited, dismissed\]/); + assert.match(workflow, /types: \[created, edited\]/); }); it("queries review threads and feeds them to the findings claim check", () => { - assert.match(workflow, /reviewThreads\(first: 100\)/); + // Paginated read: `after: $cursor` + `pageInfo.hasNextPage`, so a busy PR + // with more than 100 threads cannot hide unresolved bot threads (fail-open + // gap in a fail-closed check). + assert.match(workflow, /reviewThreads\(first: 100, after: \$cursor\)/); + assert.match(workflow, /hasNextPage/); assert.match(workflow, /unresolvedFindingsClaim/); assert.match(workflow, /findingsClaim\.byBot/); assert.match(workflow, /review_findings/); diff --git a/.github/scripts/pr-quality-messages.cjs b/.github/scripts/pr-quality-messages.cjs index 773b46420b..0f6c6619a9 100644 --- a/.github/scripts/pr-quality-messages.cjs +++ b/.github/scripts/pr-quality-messages.cjs @@ -9,7 +9,10 @@ const { READINESS_LATEST_DEV_BEHIND_MAX } = require("./pr-quality-state.cjs"); -/** Marks the bot's consolidated PR gate message. */ +/** + * Legacy marker for the pre-consolidation readiness comment. It is matched + * only to migrate and delete old comments; the gate never writes it. + */ const READINESS_MARKER = ""; /** Marks the bot's consolidated PR gate message. */ const GATE_MARKER = ""; diff --git a/.github/scripts/pr-quality-state.cjs b/.github/scripts/pr-quality-state.cjs index 69dadd5dbf..d3fa498020 100644 --- a/.github/scripts/pr-quality-state.cjs +++ b/.github/scripts/pr-quality-state.cjs @@ -109,9 +109,9 @@ function gateStateMarker(state) { /** * Fresh consolidated gate state. It merges the old enforcer ownership fields * (active / autoDraftedByBot / titlePrefixedByBot) with the readiness fields - * (maintainersPinged / completedAtHeadSha). `reviewReadyLabeled` records - * whether the gate currently owns the `review-ready` label, so a run that - * merely re-renders the comment does not re-fire the label webhook. + * (maintainersPinged / completedAtHeadSha). `reviewReadyLabeled` is serialized + * for backward compatibility with states written by earlier versions of this + * gate; live label decisions read `pr.labels` directly, never this field. */ function defaultGateState() { return { @@ -176,7 +176,13 @@ function migrateLegacyGateState(enforcerState, readinessState) { gate.titlePrefixedByBot = Boolean(enforcerState.titlePrefixedByBot); } if (readinessState) { - gate.autoDraftedByBot = Boolean(readinessState.autoDraftedByBot); + // Either legacy record may own the auto-draft: the enforcer converted the + // PR to draft for a quality failure, the readiness comment recorded the + // checklist-driven draft, or both. Ownership is a union — letting the + // readiness value overwrite a true enforcer bit drops the restore path + // and leaves a bot-drafted maintainer PR stuck in draft forever. + gate.autoDraftedByBot = + gate.autoDraftedByBot || Boolean(readinessState.autoDraftedByBot); gate.maintainersPinged = Boolean(readinessState.maintainersPinged); gate.completedAtHeadSha = readinessState.completedAtHeadSha ?? null; } @@ -228,6 +234,9 @@ const REVIEW_FINDINGS_BOT_LOGINS = [ "coderabbitai[bot]" ]; +/** The login that authors CodeRabbit reviews. */ +const CODE_RABBIT_LOGIN = "coderabbitai[bot]"; + /** * CodeRabbit's review-body line that reports actionable inline findings. The * gate reads this to count findings that CodeRabbit posts only as review-body @@ -243,23 +252,30 @@ const CODE_RABBIT_ACTIONABLE_RE = * `**Actionable comments posted: N**`; those never become review threads, so * the thread check alone would miss them. This supplements the thread check: * a CodeRabbit review of the live head whose body reports actionable comments - * counts as an unresolved finding. + * counts as an unresolved finding. Only CodeRabbit's own reviews are read — + * a human review quoting the same line must not count. * * Only the most recent review for the live head is considered (the head a * findings-review covers is the head that must be clean), so an older review - * of a superseded commit cannot keep the box unticked forever. + * of a superseded commit cannot keep the box unticked forever. Reviews with a + * missing or unparsable `submitted_at` sort last deterministically so the + * "most recent" pick is never arbitrary. */ function coderabbitOutsideDiffFindings({ reviews = [], liveHeadSha }) { if (!liveHeadSha || !Array.isArray(reviews) || reviews.length === 0) { return { code: null, unresolved: 0, byBot: {} }; } + const submittedAt = review => { + const parsed = Date.parse(String(review?.submitted_at ?? "")); + return Number.isNaN(parsed) ? -Infinity : parsed; + }; const latestForHead = reviews - .filter(review => review?.commit_id === liveHeadSha) - .sort( - (a, b) => - Date.parse(String(b?.submitted_at ?? "")) - - Date.parse(String(a?.submitted_at ?? "")) - )[0]; + .filter( + review => + review?.commit_id === liveHeadSha && + review?.user?.login === CODE_RABBIT_LOGIN + ) + .sort((a, b) => submittedAt(b) - submittedAt(a))[0]; const body = String(latestForHead?.body ?? ""); const match = CODE_RABBIT_ACTIONABLE_RE.exec(body); if (!match) return { code: null, unresolved: 0, byBot: {} }; @@ -268,7 +284,7 @@ function coderabbitOutsideDiffFindings({ reviews = [], liveHeadSha }) { return { code: "review_findings", unresolved: count, - byBot: { "coderabbitai[bot]": count } + byBot: { [CODE_RABBIT_LOGIN]: count } }; } @@ -280,8 +296,11 @@ function coderabbitOutsideDiffFindings({ reviews = [], liveHeadSha }) { * only in its review body (outside the diff range); those are added by the * `coderabbitOutsideDiffFindings` supplement so they cannot slip through. * The supplement is subordinate: it never subtracts, only adds unresolved - * counts for the live head, and once the reviewer resolves the threads the - * next run re-checks. + * counts for the live head while a bot thread is still open. A review body is + * immutable, so the count can never fall to zero on its own once posted; the + * supplement therefore only counts while an unresolved bot thread exists — the + * author resolves that thread to clear the box, matching the checklist wording + * ("I resolved all correct ... findings") without requiring an empty commit. */ function unresolvedFindingsClaim({ threads = [], reviews = [], liveHeadSha }) { const byBot = {}; @@ -294,11 +313,13 @@ function unresolvedFindingsClaim({ threads = [], reviews = [], liveHeadSha }) { unresolved += 1; } } - const outside = coderabbitOutsideDiffFindings({ reviews, liveHeadSha }); - if (outside.code) { - for (const [login, count] of Object.entries(outside.byBot)) { - byBot[login] = (byBot[login] ?? 0) + count; - unresolved += count; + if (unresolved > 0) { + const outside = coderabbitOutsideDiffFindings({ reviews, liveHeadSha }); + if (outside.code) { + for (const [login, count] of Object.entries(outside.byBot)) { + byBot[login] = (byBot[login] ?? 0) + count; + unresolved += count; + } } } return unresolved > 0 @@ -349,6 +370,7 @@ module.exports = { GATE_STATE_PATTERN, READINESS_STATE_VERSION, REVIEW_FINDINGS_BOT_LOGINS, + CODE_RABBIT_LOGIN, CODE_RABBIT_ACTIONABLE_RE, coderabbitOutsideDiffFindings, parseState, diff --git a/.github/scripts/pr-quality-state.test.cjs b/.github/scripts/pr-quality-state.test.cjs index 33d6942ce3..1b0a1a2c82 100644 --- a/.github/scripts/pr-quality-state.test.cjs +++ b/.github/scripts/pr-quality-state.test.cjs @@ -370,6 +370,7 @@ describe("coderabbitOutsideDiffFindings", () => { body: "**Actionable comments posted: 3**\n\nSome walkthrough.", commit_id: HEAD, submitted_at: "2026-08-04T06:24:02Z", + user: { login: "coderabbitai[bot]" }, }, ], liveHeadSha: HEAD, @@ -388,6 +389,7 @@ describe("coderabbitOutsideDiffFindings", () => { body: "**Actionable comments posted: 3**", commit_id: "1111111111111111111111111111111111111111", submitted_at: "2026-08-04T06:24:02Z", + user: { login: "coderabbitai[bot]" }, }, ], liveHeadSha: HEAD, @@ -397,7 +399,7 @@ describe("coderabbitOutsideDiffFindings", () => { it("ignores a review reporting zero actionable comments", () => { const claim = coderabbitOutsideDiffFindings({ - reviews: [{ body: "**Actionable comments posted: 0**", commit_id: HEAD }], + reviews: [{ body: "**Actionable comments posted: 0**", commit_id: HEAD, user: { login: "coderabbitai[bot]" } }], liveHeadSha: HEAD, }); assert.deepEqual(claim, { code: null, unresolved: 0, byBot: {} }); @@ -406,8 +408,8 @@ describe("coderabbitOutsideDiffFindings", () => { it("uses the most recent review of the live head", () => { const claim = coderabbitOutsideDiffFindings({ reviews: [ - { body: "**Actionable comments posted: 2**", commit_id: HEAD, submitted_at: "2026-08-04T06:00:00Z" }, - { body: "**Actionable comments posted: 5**", commit_id: HEAD, submitted_at: "2026-08-04T07:00:00Z" }, + { body: "**Actionable comments posted: 2**", commit_id: HEAD, submitted_at: "2026-08-04T06:00:00Z", user: { login: "coderabbitai[bot]" } }, + { body: "**Actionable comments posted: 5**", commit_id: HEAD, submitted_at: "2026-08-04T07:00:00Z", user: { login: "coderabbitai[bot]" } }, ], liveHeadSha: HEAD, }); @@ -420,28 +422,53 @@ describe("coderabbitOutsideDiffFindings", () => { unresolved: 0, byBot: {}, }); - assert.deepEqual(coderabbitOutsideDiffFindings({ reviews: [{ body: "**Actionable comments posted: 1**", commit_id: HEAD }] }), { + assert.deepEqual(coderabbitOutsideDiffFindings({ reviews: [{ body: "**Actionable comments posted: 1**", commit_id: HEAD, user: { login: "coderabbitai[bot]" } }] }), { code: null, unresolved: 0, byBot: {}, }); }); + + it("ignores a human review that quotes the actionable-comments line", () => { + const claim = coderabbitOutsideDiffFindings({ + reviews: [ + { + body: "CodeRabbit said **Actionable comments posted: 2** — let's discuss.", + commit_id: HEAD, + submitted_at: "2026-08-04T06:24:02Z", + user: { login: "wibias" }, + }, + ], + liveHeadSha: HEAD, + }); + assert.deepEqual(claim, { code: null, unresolved: 0, byBot: {} }); + }); + + it("sorts undated reviews last deterministically", () => { + const claim = coderabbitOutsideDiffFindings({ + reviews: [ + { body: "**Actionable comments posted: 2**", commit_id: HEAD, user: { login: "coderabbitai[bot]" } }, + { body: "**Actionable comments posted: 5**", commit_id: HEAD, submitted_at: "2026-08-04T07:00:00Z", user: { login: "coderabbitai[bot]" } }, + ], + liveHeadSha: HEAD, + }); + // The dated review wins over the undated one, so 5 is the count. + assert.equal(claim.unresolved, 5); + }); }); describe("unresolvedFindingsClaim with outside-diff supplement", () => { const HEAD = "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b"; - it("adds the outside-diff count to a clean thread set", () => { + it("does not count outside-diff when no unresolved bot thread exists", () => { + // The review body is immutable; once the author resolves every thread the + // supplement must not keep the box unticked forever (no empty commit). const claim = unresolvedFindingsClaim({ threads: [], - reviews: [{ body: "**Actionable comments posted: 2**", commit_id: HEAD, submitted_at: "2026-08-04T06:24:02Z" }], + reviews: [{ body: "**Actionable comments posted: 2**", commit_id: HEAD, submitted_at: "2026-08-04T06:24:02Z", user: { login: "coderabbitai[bot]" } }], liveHeadSha: HEAD, }); - assert.deepEqual(claim, { - code: "review_findings", - unresolved: 2, - byBot: { "coderabbitai[bot]": 2 }, - }); + assert.deepEqual(claim, { code: null, unresolved: 0, byBot: {} }); }); it("adds the outside-diff count to an unresolved thread count", () => { @@ -449,7 +476,7 @@ describe("unresolvedFindingsClaim with outside-diff supplement", () => { threads: [ { isResolved: false, author: { login: "coderabbitai[bot]" } }, ], - reviews: [{ body: "**Actionable comments posted: 2**", commit_id: HEAD, submitted_at: "2026-08-04T06:24:02Z" }], + reviews: [{ body: "**Actionable comments posted: 2**", commit_id: HEAD, submitted_at: "2026-08-04T06:24:02Z", user: { login: "coderabbitai[bot]" } }], liveHeadSha: HEAD, }); assert.deepEqual(claim, { @@ -464,7 +491,7 @@ describe("unresolvedFindingsClaim with outside-diff supplement", () => { threads: [ { isResolved: true, author: { login: "coderabbitai[bot]" } }, ], - reviews: [{ body: "**Actionable comments posted: 2**", commit_id: "1111111111111111111111111111111111111111", submitted_at: "2026-08-04T06:24:02Z" }], + reviews: [{ body: "**Actionable comments posted: 2**", commit_id: "1111111111111111111111111111111111111111", submitted_at: "2026-08-04T06:24:02Z", user: { login: "coderabbitai[bot]" } }], liveHeadSha: HEAD, }); assert.deepEqual(claim, { code: null, unresolved: 0, byBot: {} }); @@ -515,6 +542,16 @@ describe("gate state", () => { assert.equal(merged.reviewReadyLabeled, false); }); + it("keeps enforcer-owned auto-draft when the readiness record says false", () => { + const merged = migrateLegacyGateState( + { version: 1, active: true, autoDraftedByBot: true }, + { version: 2, autoDraftedByBot: false, maintainersPinged: true }, + ); + // Ownership is a union: the enforcer converted the PR to draft for a + // quality failure, so the readiness record must not drop the restore path. + assert.equal(merged.autoDraftedByBot, true); + }); + it("migrates with either legacy state absent", () => { const onlyEnforcer = migrateLegacyGateState( { version: 1, active: true, titlePrefixedByBot: true }, diff --git a/.github/scripts/pr-quality.cjs b/.github/scripts/pr-quality.cjs index 16adf2ebc3..eaceab06ad 100644 --- a/.github/scripts/pr-quality.cjs +++ b/.github/scripts/pr-quality.cjs @@ -186,9 +186,11 @@ function hasGuiCue(title, body) { * title/description is a false positive and a screenshot is not required. The * negation word must appear within a short window before `gui`, so a comment * like "this touches gui but only the config" (no negation) keeps the gate. + * The window cannot cross a sentence or line boundary: "This does not change + * the API. Please add a gui screenshot." must not waive the gate. */ const GUI_OVERRIDE_RE = - /\b(?:no|not|doesn'?t|does not|never|without)\b[\s\S]{0,40}?\bgui\b/i; + /\b(?:no|not|doesn'?t|does not|never|without)\b[^.!?\n]{0,40}?\bgui\b/i; /** * True when a maintainer (OWNER / COLLABORATOR / MEMBER) issue comment waives diff --git a/.github/scripts/pr-quality.test.cjs b/.github/scripts/pr-quality.test.cjs index 9b2455d2fd..e013fab88a 100644 --- a/.github/scripts/pr-quality.test.cjs +++ b/.github/scripts/pr-quality.test.cjs @@ -186,6 +186,17 @@ describe("hasGuiOverride", () => { ); }); + it("does not match a negation that belongs to another sentence or line", () => { + assert.equal( + hasGuiOverride({ comments: [{ author_association: "OWNER", body: "This does not change the API. Please add a gui screenshot." }] }), + false, + ); + assert.equal( + hasGuiOverride({ comments: [{ author_association: "OWNER", body: "- no rebase needed\n- gui tweak included" }] }), + false, + ); + }); + it("is clean for no comments or a comment without a body", () => { assert.equal(hasGuiOverride({ comments: [] }), false); assert.equal(hasGuiOverride({ comments: [{ author_association: "OWNER" }] }), false); @@ -813,6 +824,50 @@ describe("collectPrQualityFailures", () => { assert.ok(failures.some((f) => f.code === "missing_ui_screenshot")); }); + it("waives the screenshot gate for a maintainer override comment", () => { + const failures = collectPrQualityFailures({ + baseRef: "dev", + allowedBases: allowed, + title: "GUI: fix provider list spacing", + body: [ + "## Summary", + "This change adjusts gui/ spacing tokens used by the dashboard.", + "", + "## Test plan", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"), + behindMain: 0, + behindBase: 0, + authorPermission: "read", + guiOverrideComments: [ + { author_association: "OWNER", body: "no gui changes here" }, + ], + }); + assert.ok(!failures.some((f) => f.code === "missing_ui_screenshot")); + }); + + it("keeps the screenshot gate when only the PR author claims no gui change", () => { + const failures = collectPrQualityFailures({ + baseRef: "dev", + allowedBases: allowed, + title: "GUI: fix provider list spacing", + body: [ + "## Summary", + "This change adjusts gui/ spacing tokens used by the dashboard.", + "", + "## Test plan", + "- Ran bun test tests/ci-workflows.test.ts", + ].join("\n"), + behindMain: 0, + behindBase: 0, + authorPermission: "read", + guiOverrideComments: [ + { author_association: "CONTRIBUTOR", body: "no gui changes here" }, + ], + }); + assert.ok(failures.some((f) => f.code === "missing_ui_screenshot")); + }); + it("accepts a gui title when a screenshot image is embedded", () => { const failures = collectPrQualityFailures({ baseRef: "dev", diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 6a1c228918..9b12e23968 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -8,11 +8,17 @@ on: - edited - ready_for_review - synchronize - # Review events let the gate catch bot findings (Codex / CodeRabbit - # review threads) that land after a PR was marked ready, without waiting - # for the author's next push. - - pull_request_review - - pull_request_review_comment + # Review events let the gate catch bot findings (Codex / CodeRabbit review + # threads) that land after a PR was marked ready, without waiting for the + # author's next push. These are top-level webhook events, not activity types + # of pull_request_target — listing them under `types:` is silently ignored by + # GitHub (actionlint: invalid activity type). Both payloads carry + # `pull_request`, so the base.sha trusted checkout and + # `context.payload.pull_request` reads below are unchanged. + pull_request_review: + types: [submitted, edited, dismissed] + pull_request_review_comment: + types: [created, edited] # pull-requests:write covers title/comment/label updates. # contents:write is required for convertPullRequestToDraft / @@ -203,20 +209,29 @@ jobs: } async function setReviewReadyLabel(shouldHave, hasLabel) { - if (shouldHave && !hasLabel) { - await github.rest.issues.addLabels({ - owner, - repo, - issue_number: pull_number, - labels: [REVIEW_READY_LABEL] - }); - } else if (!shouldHave && hasLabel) { - await github.rest.issues.removeLabel({ - owner, - repo, - issue_number: pull_number, - name: REVIEW_READY_LABEL - }); + // The label is a review trigger, not a gate decision. A label + // write failure must not abort the run before the gate comment + // or the draft/ready conversion happens. + try { + if (shouldHave && !hasLabel) { + await github.rest.issues.addLabels({ + owner, + repo, + issue_number: pull_number, + labels: [REVIEW_READY_LABEL] + }); + } else if (!shouldHave && hasLabel) { + await github.rest.issues.removeLabel({ + owner, + repo, + issue_number: pull_number, + name: REVIEW_READY_LABEL + }); + } + } catch (error) { + core.warning( + `Could not ${shouldHave ? "add" : "remove"} the ${inlineCode(REVIEW_READY_LABEL)} label: ${error.message}` + ); } } @@ -580,26 +595,51 @@ jobs: let findingsClaim = null; let findingsUnverifiable = false; try { - const reviewData = await github.graphql( - ` - query($owner: String!, $repo: String!, $number: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $number) { - reviewThreads(first: 100) { - nodes { - isResolved - comments(first: 1) { - nodes { - author { login } + // Paginate the review threads: a busy PR can carry more than + // 100 threads (CodeRabbit posts many reviews), and a truncated + // read would silently miss unresolved bot threads — a fail-open + // gap in a fail-closed check. + const allThreadNodes = []; + let threadCursor = null; + let threadPage = null; + let reviewThreadsPage = null; + do { + threadPage = await github.graphql( + ` + query($owner: String!, $repo: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { + hasNextPage + endCursor + } + nodes { + isResolved + comments(first: 1) { + nodes { + author { login } + } } } } } } } + `, + { + owner, + repo, + number: pull_number, + cursor: threadCursor } - `, - { owner, repo, number: pull_number } + ); + reviewThreadsPage = threadPage?.repository?.pullRequest?.reviewThreads; + allThreadNodes.push(...(reviewThreadsPage?.nodes ?? [])); + threadCursor = reviewThreadsPage?.pageInfo?.endCursor ?? null; + } while ( + reviewThreadsPage?.pageInfo?.hasNextPage === true && + threadCursor ); const reviewsData = await github.paginate( github.rest.pulls.listReviews, @@ -611,10 +651,7 @@ jobs: } ); findingsClaim = unresolvedFindingsClaim({ - threads: ( - reviewData?.repository?.pullRequest?.reviewThreads?.nodes ?? - [] - ).map(node => ({ + threads: allThreadNodes.map(node => ({ isResolved: node.isResolved, author: node.comments?.nodes?.[0]?.author ?? null })), @@ -1021,16 +1058,3 @@ jobs: ); return; } - - // Fallback for a clean contributor PR whose checklist is complete - // but which already left the mustDraft branch above (defensive). - if (checklistRequired && checklistComplete) { - await upsertGateComment(readyState, { - status: "READY", - statusReason: "all PR quality gates passed; the review readiness checklist is complete.", - actions: [], - readiness, - checklistRequired, - notices: [] - }); - } diff --git a/docs-site/src/content/docs/contributing/pr-quality.md b/docs-site/src/content/docs/contributing/pr-quality.md index 1bc5ac3958..d7eb652083 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -39,23 +39,33 @@ tells you exactly what to change: a real description: a **Summary** of what changed and why, plus a **Test plan** (or equivalent substance). When the title or description mentions `gui`, the description must include a screenshot of the UI change; the check - keeps the PR a draft and comments until the screenshot is present. + keeps the PR a draft and comments until the screenshot is present. A + maintainer (OWNER / COLLABORATOR / MEMBER) can waive the screenshot + requirement with an issue comment saying the change does not touch the GUI + (for example "no gui changes"); the PR author cannot self-waive. Contributor PRs (authors without repository push permission) open in draft and stay there until a four-box review-readiness checklist in the description is complete: local CI green, the branch on the latest `dev` commit, all correct Codex and CodeRabbit findings fixed, and the ready-for-review confirmation. Once every box is ticked the check marks the PR ready for review and notifies the maintainers listed in `MAINTAINERS.md` - (excluding the author). Completion is bound to the exact commit the PR head + (excluding the author). The gate's status and "what to do" live in a single + consolidated bot comment that is rewritten on every run, so there is exactly + one place to look. Completion is bound to the exact commit the PR head pointed at: if new commits are pushed afterwards, the gate moves the PR back to draft, resets the checklist and the maintainer notification, and asks you to test and tick the boxes again against the latest code. A retarget to `dev` clears the wrong-branch message automatically and is remembered by the gate; the draft stays until the checklist is complete. - Before a completion is accepted, the gate verifies the two checklist claims - it can check itself: the head's `ci` check must be green, and the branch - must be on the latest `dev` commit or at most 10 commits behind it. A - disproved claim unticks the matching box and keeps the PR a draft. + Before a completion is accepted, the gate verifies the checklist claims it + can check itself: the head's `ci` check must be green, the branch must be on + the latest `dev` commit or at most 10 commits behind it, and every Codex and + CodeRabbit review thread on the PR must be resolved. A disproved claim + unticks the matching box and keeps the PR a draft. The gate also re-runs on + review events, so a bot finding posted after the PR was marked ready is + caught without waiting for the next push. When the checklist is complete and + every gate is green, the gate adds a `review-ready` label that opts the PR + into CodeRabbit review. - **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/structure/06_docs-and-release.md b/structure/06_docs-and-release.md index fed4d9960b..b52466fdc9 100644 --- a/structure/06_docs-and-release.md +++ b/structure/06_docs-and-release.md @@ -44,7 +44,7 @@ bun run build | `.github/workflows/release.yml` | Manual dispatch only | npm publish/dry-run workflow. It requires the exact `GITHUB_SHA` to have a successful Cross-platform CI run before publish or dry-run. | | `.github/workflows/deploy-docs.yml` | `push` to `main` touching `docs-site/**` or the workflow, or manual dispatch | Build and publish the Astro/Starlight docs site to GitHub Pages. | | `.github/workflows/service-lifecycle.yml` | `pull_request` to `main`/`dev` and `push`, both filtered on the service path set (`src/service.ts`, `src/cli.ts`, `src/cli/index.ts`, `src/lib/bun-runtime.ts`, `package.json`, `bun.lock`, the workflow), or manual dispatch | Service-lifecycle smoke on three platforms: Linux systemd, macOS launchd, and Windows Scheduled Tasks. Each installs, verifies, stops via `ocx stop`, and uninstalls. The path list is kept in sync with the `release.yml` service-gate regex. | -| `.github/workflows/enforce-pr-target.yml` | `pull_request_target` (opened, reopened, edited, ready_for_review, synchronize) | The `enforce-target` gate: rejects pull requests whose head ancestry sits on the `main` tip while far behind `dev`, and rejects empty or malformed descriptions. Stacked child PRs targeting another open PR's head skip the wrong-base gate. | +| `.github/workflows/enforce-pr-target.yml` | `pull_request_target` (opened, reopened, edited, ready_for_review, synchronize), `pull_request_review` (submitted, edited, dismissed), `pull_request_review_comment` (created, edited) | The `enforce-target` gate: rejects pull requests whose head ancestry sits on the `main` tip while far behind `dev`, rejects empty or malformed descriptions, requires a GUI screenshot when the title/body mentions `gui` (waivable by a maintainer comment), keeps contributor PRs in draft until a four-box readiness checklist is complete, verifies the CI / latest-dev / Codex+CodeRabbit-findings claims, and adds a `review-ready` label at the ready moment to trigger CodeRabbit review. Stacked child PRs targeting another open PR's head skip the wrong-base gate. | | `.github/workflows/enforce-issue-quality.yml` | `issues` (opened, edited, reopened), `issue_comment` (created, edited), or manual dispatch with an issue number | Issue-template compliance gate. | | `.github/workflows/issue-quality-tests.yml` | `pull_request` and `push` filtered on the issue/PR automation scripts, templates, and their workflows | Tests the issue and PR automation scripts themselves, so the gates cannot rot silently. | | `.github/workflows/issue-triage.yml` | `issues` (opened) | Duplicate detection and triage labeling for new issues. | diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 44dac748e1..30536ddd7d 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -830,7 +830,15 @@ describe("GitHub Actions hardening", () => { // pull_request_target runs with the base repo's token. Checking out or // executing the PR's code under it is the classic escalation. - expect(Object.keys(workflow.on ?? {})).toEqual(["pull_request_target"]); + // The two review events are sibling top-level events (not + // `pull_request_target` activity types — those are silently ignored by + // GitHub), and both payloads carry `pull_request`, so the trusted base.sha + // checkout and the `context.payload.pull_request` reads stay unchanged. + expect(Object.keys(workflow.on ?? {}).sort()).toEqual([ + "pull_request_review", + "pull_request_review_comment", + "pull_request_target", + ]); // And the trigger is exactly a `types:` list — nothing else. // @@ -842,6 +850,9 @@ describe("GitHub Actions hardening", () => { // additive, both look like ordinary scoping in a diff, and neither failed a // single assertion. expect(Object.keys(workflow.on?.pull_request_target ?? {})).toEqual(["types"]); + // The sibling review events are also exactly `types:` lists. + expect(Object.keys(workflow.on?.pull_request_review ?? {})).toEqual(["types"]); + expect(Object.keys(workflow.on?.pull_request_review_comment ?? {})).toEqual(["types"]); // Exactly the scopes this gate needs. `pull-requests: write` covers title // and comment updates. `contents: write` is required for the draft GraphQL @@ -938,13 +949,25 @@ describe("GitHub Actions hardening", () => { expect([...types].sort()).toEqual([ "edited", "opened", - "pull_request_review", - "pull_request_review_comment", "ready_for_review", "reopened", "synchronize", ]); + // Review events are top-level webhook events, not `pull_request_target` + // activity types — GitHub silently ignores invalid types, so the gate + // would never re-run on a bot finding posted after ready. Assert the + // sibling events and their activity-type lists. + expect(workflow.on?.pull_request_review?.types).toEqual([ + "submitted", + "edited", + "dismissed", + ]); + expect(workflow.on?.pull_request_review_comment?.types).toEqual([ + "created", + "edited", + ]); + // The verdict is a live PR read plus ancestry/description checks. expect(script).toContain("github.rest.pulls.get"); expect(script).toContain("collectPrQualityFailures"); @@ -1892,15 +1915,18 @@ describe("GitHub Actions hardening", () => { expect(readinessBody).toContain("**4/4** boxes ticked"); }); - test("CodeRabbit outside-diff findings untick the box even with clean threads", async () => { + test("CodeRabbit outside-diff findings do not untick the box once threads are clean", async () => { // CodeRabbit posts some findings only in its review body ("outside the - // diff range"), which never become review threads. The supplement reads - // `pulls.listReviews` for a live-head CodeRabbit review reporting - // actionable comments and treats it as an unresolved finding. + // diff range"), which never become review threads. A review body is + // immutable, so the count can never fall to zero on its own once posted; + // the supplement therefore only counts while an unresolved bot thread + // exists. Clean threads + a live-head review body with a positive count + // must stay green — otherwise the author could never clear the box + // without pushing an empty commit. const result = await run({ pr: { base: { ref: "dev" }, - draft: false, + draft: true, body: readinessChecklistBody(4), }, maintainersFile: MAINTAINERS_FIXTURE, @@ -1909,6 +1935,7 @@ describe("GitHub Actions hardening", () => { body: "**Actionable comments posted: 2**\n\nWalkthrough.", commit_id: "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b", submitted_at: "2026-08-04T06:24:02Z", + user: { login: "coderabbitai[bot]" }, }, ], }); @@ -1917,20 +1944,44 @@ describe("GitHub Actions hardening", () => { "checks.listForRef", "graphql", "pulls.listReviews", - "pulls.get", - "pulls.update", + "issues.addLabels", "graphql", "issues.createComment", ])); + expect(callsTo(result, "pulls.update")).toEqual([]); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain("**4/4** boxes ticked"); + expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); + }); + + test("CodeRabbit outside-diff findings add to an unresolved thread count", async () => { + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: false, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + reviewThreads: [ + { isResolved: false, author: { login: "coderabbitai[bot]" } }, + ], + reviews: [ + { + body: "**Actionable comments posted: 2**\n\nWalkthrough.", + commit_id: "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b", + submitted_at: "2026-08-04T06:24:02Z", + user: { login: "coderabbitai[bot]" }, + }, + ], + }); + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; expect(bodyUpdate.body).toContain("- [ ] I resolved all correct Codex and CodeRabbit findings."); - expect(bodyUpdate.body).toContain("- [x] My PR is ready for review."); const readinessBody = lastReadinessCommentBody(result); expect(readinessBody).toContain( - "CodeRabbit has 2 unresolved findings; the **Codex/CodeRabbit findings** box has been unticked.", + "CodeRabbit has 3 unresolved findings; the **Codex/CodeRabbit findings** box has been unticked.", ); expect(readinessBody).toContain("**3/4** boxes ticked"); - expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); }); test("a CodeRabbit outside-diff review of a stale head does not untick the box", async () => { @@ -1948,6 +1999,7 @@ describe("GitHub Actions hardening", () => { body: "**Actionable comments posted: 2**", commit_id: "1111111111111111111111111111111111111111", submitted_at: "2026-08-04T06:24:02Z", + user: { login: "coderabbitai[bot]" }, }, ], }); @@ -1996,6 +2048,39 @@ describe("GitHub Actions hardening", () => { expect(lastReadinessCommentBody(result)).toContain("**4/4** boxes ticked"); }); + test("a human review quoting the actionable-comments line does not untick the box", async () => { + // The outside-diff supplement filters by author: a maintainer quoting + // CodeRabbit's summary in their own review must not count as CodeRabbit + // findings, or the box would be unticked by a human's quote. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + reviewThreads: [ + { isResolved: false, author: { login: "coderabbitai[bot]" } }, + ], + reviews: [ + { + body: "CodeRabbit said **Actionable comments posted: 2** — let's discuss.", + commit_id: "3f1c0de0a6a4d0a3f9a1b2c3d4e5f60718293a4b", + submitted_at: "2026-08-04T06:24:02Z", + user: { login: "wibias" }, + }, + ], + }); + + // Only the unresolved thread counts (1), not the human's quoted line. + const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; + expect(bodyUpdate.body).toContain("- [ ] I resolved all correct Codex and CodeRabbit findings."); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain( + "CodeRabbit has 1 unresolved finding; the **Codex/CodeRabbit findings** box has been unticked.", + ); + }); + test("a review-threads lookup failure fails closed for the findings claim", async () => { const result = await run({ pr: { diff --git a/tests/helpers/enforce-pr-target-harness.ts b/tests/helpers/enforce-pr-target-harness.ts index c7acccd25f..4794913c61 100644 --- a/tests/helpers/enforce-pr-target-harness.ts +++ b/tests/helpers/enforce-pr-target-harness.ts @@ -135,10 +135,17 @@ export type RunOptions = { reviewThreads?: Array<{ isResolved: boolean | null; author: { login: string } | null }>; /** * Pull-request reviews `pulls.listReviews` reports for the PR. Each entry is - * `{ body, commit_id, submitted_at }`; the workflow reads CodeRabbit's - * "Actionable comments posted: N" body line as the outside-diff supplement. + * `{ body, commit_id, submitted_at, user }`; the workflow reads CodeRabbit's + * "Actionable comments posted: N" body line as the outside-diff supplement + * and filters by `user.login` so a human review quoting the line does not + * count. */ - reviews?: Array<{ body: string; commit_id: string; submitted_at?: string }>; + reviews?: Array<{ + body: string; + commit_id: string; + submitted_at?: string; + user?: { login: string }; + }>; /** * Labels the PR already carries (from `pulls.get`). The gate reads these to * decide whether to add/remove the `review-ready` label. @@ -692,9 +699,12 @@ export async function runEnforcePrTarget( */ class Octokit { rest = rest; - graphql = (query: unknown, variables: unknown) => { + graphql = async (query: unknown, variables: unknown) => { const text = String(query ?? ""); - const recorded = record("graphql", { query, variables }); + // Routed through `respond` so a `failOn: ["graphql"]` simulated failure + // REJECTS rather than throwing synchronously, like every other method + // (real Octokit graphql returns a promise and rejects it). + await respond("graphql", { query, variables }); // Fail a specific mutation after recording so the failed call appears in // the recording (same semantics as `failOn`). The review-threads read is // the first graphql call; targeting a mutation by query text lets a test @@ -705,8 +715,9 @@ export async function runEnforcePrTarget( // The review-threads query is answered with the shape the workflow // reads. `github.graphql` resolves to the raw data payload (no `data` // wrapper, unlike `github.rest.*`), so the threads object is returned - // directly. Everything else (the draft/ready mutations) records raw; the - // workflow ignores the return value of those. + // directly. Mutations also resolve to a raw GraphQL payload, never a + // REST envelope, so a workflow that reads a mutation result gets the + // same shape production produces. if (text.includes("reviewThreads")) { const threads = (options.reviewThreads ?? []).map(thread => ({ isResolved: thread.isResolved, @@ -722,7 +733,7 @@ export async function runEnforcePrTarget( }, }; } - return recorded; + return {}; }; request = (route: unknown, params: unknown) => respond("request", { route, params }); From e26ea2c08d4e35d89e61161f7f2f8ac75554ac03 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 05:30:44 +0200 Subject: [PATCH 3/5] fix(ci): drop review-event triggers, fix re-review findings Address the CodeRabbit re-review of the first fix round: - Remove pull_request_review / pull_request_review_comment triggers: review events load the workflow from the PR head branch (like pull_request), so the head-controlled workflow YAML ran under a write token against base-pinned scripts, crashing the gate (parseGateState is not a function) and breaking the trusted-base model. The findings claim still runs on every pull_request_target event. - .coderabbit.yaml: drop the positive labels filter, which would restrict ALL CodeRabbit reviews to labeled PRs and starve maintainer PRs (they never carry review-ready). The label remains a status marker only. - Persist draft ownership in the consolidated comment BEFORE convertToDraft so a successful convert followed by a failed comment write still leaves the bot-created draft owned and restorable; reflect a failed conversion by rewriting the comment with autoDraftedByBot:false. - Preserve bot draft/title-prefix ownership through checklist resets (head-drift and claim-check) instead of resetting to a fresh default. - Guard the maintainer recovery branch so it never calls markReadyForReview twice on the stale draft value. - Deduplicate the failure-path comment writes via a shared draftComment helper and a single failure-status-reason builder. - Docs: sync pr-quality.md and the structure workflow map (no review-event triggers; label is a status marker; review-body supplement documented). - Tests: cover the ownership-preserving reset, update wrong-base sequences for the persist-before-convert ordering, and pin the trigger allowlist. Gates: node --test .github/scripts (420 pass), bun test tests/ci-workflows (113 pass), bun run typecheck, bun run privacy:scan, actionlint clean. --- .coderabbit.yaml | 7 - .github/scripts/enforce-pr-target.test.cjs | 26 ++- .github/workflows/enforce-pr-target.yml | 184 +++++++++++------- .../content/docs/contributing/pr-quality.md | 18 +- structure/06_docs-and-release.md | 2 +- tests/ci-workflows.test.ts | 136 +++++++------ 6 files changed, 219 insertions(+), 154 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 5536bd2a40..ab02b02657 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -15,13 +15,6 @@ reviews: auto_review: enabled: true drafts: false - # The PR gate adds this label at the ready moment (checklist complete and - # quality gates green) and removes it otherwise. A label addition triggers - # a CodeRabbit review even while the PR is still a draft, which is how a - # ready-but-draft PR gets reviewed without requiring a manual - # `@coderabbitai review` comment. - labels: - - "review-ready" # Default branch (main) is included automatically; these are additional # base branches (anchored regex). base_branches: diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 7a771d6516..52cfe393bc 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -48,15 +48,16 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /synchronize/); }); - it("listens for review events so bot findings after ready are caught", () => { - // These are top-level webhook events, not `pull_request_target` activity - // types — listing them under `types:` is silently ignored by GitHub. Each - // assertion anchors to the YAML list-item form so one trigger cannot - // satisfy the other's assertion. - assert.match(workflow, /^ pull_request_review:\s*$/m); - assert.match(workflow, /^ pull_request_review_comment:\s*$/m); - assert.match(workflow, /types: \[submitted, edited, dismissed\]/); - assert.match(workflow, /types: \[created, edited\]/); + it("does not add review events that would break the trusted-base model", () => { + // `pull_request_review` / `pull_request_review_comment` load the workflow + // from the PR head branch (like `pull_request`), while this workflow's + // checkout pins the base SHA — head YAML + base scripts mismatch, so the + // gate crashes (`parseGateState is not a function`) and the head controls + // the workflow definition under a write token. The findings claim runs on + // every `pull_request_target` event instead (opened/edited/synchronize/ + // ready_for_review). + assert.doesNotMatch(workflow, /^ pull_request_review:/m); + assert.doesNotMatch(workflow, /^ pull_request_review_comment:/m); }); it("queries review threads and feeds them to the findings claim check", () => { @@ -88,11 +89,16 @@ describe("enforce-pr-target workflow", () => { assert.doesNotMatch(workflow, /Recording ownership state/); }); - it("manages the review-ready label for the CodeRabbit opt-in trigger", () => { + it("manages the review-ready status label at the ready moment", () => { assert.match(workflow, /REVIEW_READY_LABEL\s*=\s*"review-ready"/); assert.match(workflow, /github\.rest\.issues\.addLabels/); assert.match(workflow, /github\.rest\.issues\.removeLabel/); assert.match(workflow, /reviewReadyDesired/); + // A positive labels filter in .coderabbit.yaml would restrict ALL reviews + // to labeled PRs (maintainer PRs never carry this label), so the label is + // kept as a visible status marker only and never wired as a CodeRabbit + // auto-review filter. + assert.doesNotMatch(workflow, /labels:\s*\["?review-ready"?\]/); }); it("migrates legacy two-comment PRs and deletes the old comments", () => { diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 9b12e23968..3e451931c5 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -8,17 +8,6 @@ on: - edited - ready_for_review - synchronize - # Review events let the gate catch bot findings (Codex / CodeRabbit review - # threads) that land after a PR was marked ready, without waiting for the - # author's next push. These are top-level webhook events, not activity types - # of pull_request_target — listing them under `types:` is silently ignored by - # GitHub (actionlint: invalid activity type). Both payloads carry - # `pull_request`, so the base.sha trusted checkout and - # `context.payload.pull_request` reads below are unchanged. - pull_request_review: - types: [submitted, edited, dismissed] - pull_request_review_comment: - types: [created, edited] # pull-requests:write covers title/comment/label updates. # contents:write is required for convertPullRequestToDraft / @@ -513,7 +502,16 @@ jobs: const freshReadiness = extractReviewReadiness( freshPr.body ?? "" ); - readinessStateOverride = defaultGateState(); + // Reset only the checklist/notification state. The bot's draft + // ownership and title-prefix ownership survive the reset so a + // wrong-base-drafted PR that was retargeted keeps its restore + // path and its prefix ownership. + readinessStateOverride = { + ...defaultGateState(), + active: gateState.active, + autoDraftedByBot: gateState.autoDraftedByBot, + titlePrefixedByBot: gateState.titlePrefixedByBot + }; headDriftNotice = buildStaleNotice({ completionHeadSha, liveHeadSha: freshPr.head.sha, @@ -683,7 +681,12 @@ jobs: const freshReadiness = extractReviewReadiness( freshPr.body ?? "" ); - readinessStateOverride = defaultGateState(); + readinessStateOverride = { + ...defaultGateState(), + active: gateState.active, + autoDraftedByBot: gateState.autoDraftedByBot, + titlePrefixedByBot: gateState.titlePrefixedByBot + }; claimNotice = [ ...(claimViolations.includes("review_findings") ? findingsUnverifiable @@ -769,8 +772,11 @@ jobs: return actions; } - // The `review-ready` label is the CodeRabbit/Codex opt-in trigger: - // add it at the ready moment, remove it while the PR is not ready. + // The `review-ready` label marks the ready moment for humans and + // bots. It is not a CodeRabbit auto-review filter: a positive + // `labels:` entry in `.coderabbit.yaml` would restrict ALL reviews + // to labeled PRs (maintainer PRs never carry this label), so the + // label is kept as a visible status marker only. const readyMoment = checklistRequired && checklistComplete && failures.length === 0; const reviewReadyDesired = readyMoment; @@ -825,6 +831,47 @@ jobs: if (failures.length > 0) { let draftConversionFailed = false; + // One-line reason for each failure, used in the status line. + const failureStatusReason = failures + .map(failure => { + if (failure.code === "wrong_base") { + return `wrong target branch (${pr.base.ref}); retarget to ${inlineCode(DEFAULT_BASE)}.`; + } + if (failure.code === "wrong_ancestry") { + return "wrong branch ancestry; rebase onto the latest dev."; + } + if (failure.code === "bad_description") { + return `PR description needs work (${failure.reason}).`; + } + if (failure.code === "missing_ui_screenshot") { + return "UI screenshot required."; + } + return failure.code; + }) + .join(" "); + + // Shared notices for the failure comment: revalidation reason, + // the waiver flag, and the prefix notice when the bot owns it. + const failureNotices = [ + ...revalidationNotice, + ...(screenshotWaived + ? ["UI screenshot waived by a maintainer comment."] + : []), + ...(hasWrongBase && state.titlePrefixedByBot + ? [`Its title has been prefixed with ${inlineCode(TITLE_PREFIX.trim())}.`] + : []) + ]; + + const draftComment = (notices) => + upsertGateComment(state, { + status: "DRAFT", + statusReason: failureStatusReason, + actions: buildActions(), + readiness, + checklistRequired, + notices + }); + // Apply the bot-owned title prefix so the PR itself carries a // durable signal of the wrong base (claim ownership before the // write; a failed write keeps ownership for the next retry). @@ -837,10 +884,21 @@ jobs: }); } - if (!pr.draft) { - // Claim draft ownership before the mutation so a successful - // convert followed by a failed comment still restores later. + const wantsDraftConversion = !pr.draft; + if (wantsDraftConversion) { + // Persist the ownership claim BEFORE the mutation so a + // successful convert followed by a failed comment write + // still leaves the bot-created draft owned and restorable. state.autoDraftedByBot = true; + await draftComment([ + ...failureNotices, + "This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again.", + ...(checklistRequired && !checklistComplete + ? [ + `@${pr.user.login} Tick the boxes once your local CI is green, your branch is on the latest ${inlineCode(DEFAULT_BASE)} commit, and every correct Codex and CodeRabbit finding is resolved.` + ] + : []) + ]); try { await convertToDraft(); draftConverted = true; @@ -850,56 +908,24 @@ jobs: core.warning( `Could not convert pull request to draft: ${error.message}` ); + // Reflect the failed conversion in the persisted state. + await draftComment([ + ...failureNotices, + "Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required `enforce-target` check will keep failing until every issue above is resolved." + ]); } + } else { + await draftComment([ + ...failureNotices, + "This pull request was already a draft. Its draft status will be preserved after every issue above is resolved.", + ...(checklistRequired && !checklistComplete + ? [ + `@${pr.user.login} Tick the boxes once your local CI is green, your branch is on the latest ${inlineCode(DEFAULT_BASE)} commit, and every correct Codex and CodeRabbit finding is resolved.` + ] + : []) + ]); } - const draftExplanation = draftConversionFailed - ? "Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required `enforce-target` check will keep failing until every issue above is resolved." - : state.autoDraftedByBot - ? "This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again." - : "This pull request was already a draft. Its draft status will be preserved after every issue above is resolved."; - - const notices = [ - ...revalidationNotice, - ...(screenshotWaived - ? ["UI screenshot waived by a maintainer comment."] - : []), - ...(hasWrongBase && state.titlePrefixedByBot - ? [`Its title has been prefixed with ${inlineCode(TITLE_PREFIX.trim())}.`] - : []), - draftExplanation, - ...(checklistRequired && !checklistComplete - ? [ - `@${pr.user.login} Tick the boxes once your local CI is green, your branch is on the latest ${inlineCode(DEFAULT_BASE)} commit, and every correct Codex and CodeRabbit finding is resolved.` - ] - : []) - ]; - - await upsertGateComment(state, { - status: "DRAFT", - statusReason: failures - .map(failure => { - if (failure.code === "wrong_base") { - return `wrong target branch (${pr.base.ref}); retarget to ${inlineCode(DEFAULT_BASE)}.`; - } - if (failure.code === "wrong_ancestry") { - return "wrong branch ancestry; rebase onto the latest dev."; - } - if (failure.code === "bad_description") { - return `PR description needs work (${failure.reason}).`; - } - if (failure.code === "missing_ui_screenshot") { - return "UI screenshot required."; - } - return failure.code; - }) - .join(" "), - actions: buildActions(), - readiness, - checklistRequired, - notices - }); - core.setFailed( `PR quality gate failed: ${failureSummary(failures, { pr })}` ); @@ -1027,15 +1053,29 @@ jobs: // surface but there is nothing to tick; only render it if the // author is a maintainer and no checklist is required. if (!checklistRequired) { + // `pr.draft` is the value from before the ready path ran; when + // that path already converted the PR (`readyConverted`), the + // recovery must not call markReadyForReview a second time on the + // stale draft state. When the ready path already attempted and + // failed (`readyConversionFailed`), the failure state is carried + // in `readyState`/`recoveredState` and only the comment is + // rewritten, so a later run retries. if (gateState.autoDraftedByBot && pr.draft) { let recoveryFailed = false; - try { - await markReadyForReview(); - } catch (error) { - recoveryFailed = true; - core.warning( - `Could not mark pull request ready for review: ${error.message}` - ); + if (!readyConverted && !readyConversionFailed) { + try { + await markReadyForReview(); + } catch (error) { + recoveryFailed = true; + core.warning( + `Could not mark pull request ready for review: ${error.message}` + ); + } + } else { + // The ready path already handled the mutation: either it + // converted (nothing left to do) or it failed and this run + // must not attempt a second mutation on the stale draft. + recoveryFailed = readyConversionFailed; } const recoveredState = { ...gateState, diff --git a/docs-site/src/content/docs/contributing/pr-quality.md b/docs-site/src/content/docs/contributing/pr-quality.md index d7eb652083..be4c6b7bb9 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -40,7 +40,8 @@ tells you exactly what to change: plan** (or equivalent substance). When the title or description mentions `gui`, the description must include a screenshot of the UI change; the check keeps the PR a draft and comments until the screenshot is present. A - maintainer (OWNER / COLLABORATOR / MEMBER) can waive the screenshot + maintainer (OWNER / COLLABORATOR / MEMBER — repository owners, + collaborators, and members) can waive the screenshot requirement with an issue comment saying the change does not touch the GUI (for example "no gui changes"); the PR author cannot self-waive. Contributor PRs (authors without repository push permission) open in draft @@ -52,7 +53,7 @@ tells you exactly what to change: (excluding the author). The gate's status and "what to do" live in a single consolidated bot comment that is rewritten on every run, so there is exactly one place to look. Completion is bound to the exact commit the PR head - pointed at: if new commits are pushed afterwards, the gate moves the PR back + pointed at: if new commits are pushed afterward, 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 @@ -60,12 +61,13 @@ tells you exactly what to change: Before a completion is accepted, the gate verifies the checklist claims it can check itself: the head's `ci` check must be green, the branch must be on the latest `dev` commit or at most 10 commits behind it, and every Codex and - CodeRabbit review thread on the PR must be resolved. A disproved claim - unticks the matching box and keeps the PR a draft. The gate also re-runs on - review events, so a bot finding posted after the PR was marked ready is - caught without waiting for the next push. When the checklist is complete and - every gate is green, the gate adds a `review-ready` label that opts the PR - into CodeRabbit review. + CodeRabbit review thread on the PR must be resolved. CodeRabbit findings + that fall outside the diff range and are reported only in a review body on + the current head are counted the same way while a bot review thread is open; + resolving every bot thread clears them. A disproved claim unticks the + matching box and keeps the PR a draft. When the checklist is complete and + every gate is green, the gate adds a `review-ready` label as a visible + status marker at the ready moment. - **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/structure/06_docs-and-release.md b/structure/06_docs-and-release.md index b52466fdc9..1393c17a95 100644 --- a/structure/06_docs-and-release.md +++ b/structure/06_docs-and-release.md @@ -44,7 +44,7 @@ bun run build | `.github/workflows/release.yml` | Manual dispatch only | npm publish/dry-run workflow. It requires the exact `GITHUB_SHA` to have a successful Cross-platform CI run before publish or dry-run. | | `.github/workflows/deploy-docs.yml` | `push` to `main` touching `docs-site/**` or the workflow, or manual dispatch | Build and publish the Astro/Starlight docs site to GitHub Pages. | | `.github/workflows/service-lifecycle.yml` | `pull_request` to `main`/`dev` and `push`, both filtered on the service path set (`src/service.ts`, `src/cli.ts`, `src/cli/index.ts`, `src/lib/bun-runtime.ts`, `package.json`, `bun.lock`, the workflow), or manual dispatch | Service-lifecycle smoke on three platforms: Linux systemd, macOS launchd, and Windows Scheduled Tasks. Each installs, verifies, stops via `ocx stop`, and uninstalls. The path list is kept in sync with the `release.yml` service-gate regex. | -| `.github/workflows/enforce-pr-target.yml` | `pull_request_target` (opened, reopened, edited, ready_for_review, synchronize), `pull_request_review` (submitted, edited, dismissed), `pull_request_review_comment` (created, edited) | The `enforce-target` gate: rejects pull requests whose head ancestry sits on the `main` tip while far behind `dev`, rejects empty or malformed descriptions, requires a GUI screenshot when the title/body mentions `gui` (waivable by a maintainer comment), keeps contributor PRs in draft until a four-box readiness checklist is complete, verifies the CI / latest-dev / Codex+CodeRabbit-findings claims, and adds a `review-ready` label at the ready moment to trigger CodeRabbit review. Stacked child PRs targeting another open PR's head skip the wrong-base gate. | +| `.github/workflows/enforce-pr-target.yml` | `pull_request_target` (opened, reopened, edited, ready_for_review, synchronize) | The `enforce-target` gate: rejects pull requests whose head ancestry sits on the `main` tip while far behind `dev`, rejects empty or malformed descriptions, requires a GUI screenshot when the title/body mentions `gui` (waivable by a maintainer comment), keeps contributor PRs in draft until a four-box readiness checklist is complete, verifies the CI / latest-dev / Codex+CodeRabbit-findings claims (review threads plus current-head CodeRabbit review-body findings outside the diff range), and adds a `review-ready` status label at the ready moment. Stacked child PRs targeting another open PR's head skip the wrong-base gate. | | `.github/workflows/enforce-issue-quality.yml` | `issues` (opened, edited, reopened), `issue_comment` (created, edited), or manual dispatch with an issue number | Issue-template compliance gate. | | `.github/workflows/issue-quality-tests.yml` | `pull_request` and `push` filtered on the issue/PR automation scripts, templates, and their workflows | Tests the issue and PR automation scripts themselves, so the gates cannot rot silently. | | `.github/workflows/issue-triage.yml` | `issues` (opened) | Duplicate detection and triage labeling for new issues. | diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index 30536ddd7d..ccf0345fb5 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -829,16 +829,12 @@ describe("GitHub Actions hardening", () => { ]); // pull_request_target runs with the base repo's token. Checking out or - // executing the PR's code under it is the classic escalation. - // The two review events are sibling top-level events (not - // `pull_request_target` activity types — those are silently ignored by - // GitHub), and both payloads carry `pull_request`, so the trusted base.sha - // checkout and the `context.payload.pull_request` reads stay unchanged. - expect(Object.keys(workflow.on ?? {}).sort()).toEqual([ - "pull_request_review", - "pull_request_review_comment", - "pull_request_target", - ]); + // executing the PR's code under it is the classic escalation. Review + // events are deliberately NOT added: they load the workflow from the PR + // head branch (like `pull_request`), which would run head-controlled + // workflow YAML under a write token against base-pinned scripts — a + // mismatch that crashes the gate and breaks the trusted-base model. + expect(Object.keys(workflow.on ?? {})).toEqual(["pull_request_target"]); // And the trigger is exactly a `types:` list — nothing else. // @@ -850,9 +846,6 @@ describe("GitHub Actions hardening", () => { // additive, both look like ordinary scoping in a diff, and neither failed a // single assertion. expect(Object.keys(workflow.on?.pull_request_target ?? {})).toEqual(["types"]); - // The sibling review events are also exactly `types:` lists. - expect(Object.keys(workflow.on?.pull_request_review ?? {})).toEqual(["types"]); - expect(Object.keys(workflow.on?.pull_request_review_comment ?? {})).toEqual(["types"]); // Exactly the scopes this gate needs. `pull-requests: write` covers title // and comment updates. `contents: write` is required for the draft GraphQL @@ -953,20 +946,11 @@ describe("GitHub Actions hardening", () => { "reopened", "synchronize", ]); - - // Review events are top-level webhook events, not `pull_request_target` - // activity types — GitHub silently ignores invalid types, so the gate - // would never re-run on a bot finding posted after ready. Assert the - // sibling events and their activity-type lists. - expect(workflow.on?.pull_request_review?.types).toEqual([ - "submitted", - "edited", - "dismissed", - ]); - expect(workflow.on?.pull_request_review_comment?.types).toEqual([ - "created", - "edited", - ]); + // Review events must NOT be added: they load the workflow from the PR + // head branch, breaking the base-pinned checkout (`pull_request_review` + // runs head YAML + base scripts → `parseGateState is not a function`). + expect(workflow.on?.pull_request_review).toBeUndefined(); + expect(workflow.on?.pull_request_review_comment).toBeUndefined(); // The verdict is a live PR read plus ancestry/description checks. expect(script).toContain("github.rest.pulls.get"); @@ -1223,14 +1207,15 @@ describe("GitHub Actions hardening", () => { /** * The writes a fresh wrong-base contributor PR triggers: inject the - * checklist, then the title-prefix + draft conversion plus the single - * consolidated comment. + * checklist, then the title prefix, then the ownership checkpoint comment + * (claiming `autoDraftedByBot` before the mutation), then the draft + * conversion. */ const CONTRIBUTOR_WRONG_BASE_TAIL = [ "pulls.update", "pulls.update", - "graphql", "issues.createComment", + "graphql", ]; function botComment(state: Record, title = "Add a thing") { @@ -1501,9 +1486,9 @@ describe("GitHub Actions hardening", () => { "pulls.get", "pulls.update", "pulls.update", - "graphql", "issues.createComment", "issues.deleteComment", + "graphql", ])); expect(lastEnforcerCommentBody(result)).toContain("wrong target branch"); expect(lastEnforcerCommentBody(result)).toContain("[WRONG BRANCH]"); @@ -1635,6 +1620,39 @@ describe("GitHub Actions hardening", () => { expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(false); }); + test("a revalidation reset preserves bot ownership of the title prefix", async () => { + // A wrong-base PR that the bot prefixed and later had retargeted to dev + // with a complete checklist hits a revalidation failure (red CI unchecks + // a box). The reset must preserve `titlePrefixedByBot` long enough for + // the mustDraft strip to fire — otherwise the stale `[WRONG BRANCH] ` + // prefix stays on the title forever because ownership was forgotten. + const result = await run({ + pr: { + base: { ref: "dev" }, + draft: true, + title: "[WRONG BRANCH] Add a thing", + body: readinessChecklistBody(4), + }, + maintainersFile: MAINTAINERS_FIXTURE, + checkRuns: [{ name: "ci", status: "completed", conclusion: "failure" }], + comments: [botComment({ + version: 1, + active: true, + autoDraftedByBot: true, + titlePrefixedByBot: true, + })], + }); + + // The stale prefix is stripped (the ownership survived the reset long + // enough for the strip to run), and the state records ownership cleared. + const titleUpdates = callsTo(result, "pulls.update") as Array<{ title?: string; body?: string }>; + expect(titleUpdates.some(u => u.title === "Add a thing")).toBe(true); + const readinessBody = lastReadinessCommentBody(result); + expect(readinessBody).toContain('"titlePrefixedByBot":false'); + expect(readinessBody).toContain('"autoDraftedByBot":true'); + expect(readinessBody).toContain("GitHub CI is not green"); + }); + test("a complete checklist more than 10 commits behind dev unchecks the latest-dev box and re-drafts", async () => { const result = await run({ pr: { @@ -2122,8 +2140,8 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(result)).toEqual(readsAllowedBase([ - "graphql", "issues.createComment", + "graphql", ])); expect(result.warnings.some(w => w.startsWith("setFailed:"))).toBe(true); const readinessBody = lastReadinessCommentBody(result); @@ -2436,8 +2454,8 @@ describe("GitHub Actions hardening", () => { // nothing else — no checklist injection, no readiness message. expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); expect(callsTo(result, "pulls.update")).toEqual([ { owner: "lidge-jun", repo: "opencodex", pull_number: 42, title: "[WRONG BRANCH] Add a thing" }, @@ -2462,11 +2480,12 @@ describe("GitHub Actions hardening", () => { }); // No wrong base, so no title write — the checklist injection is the only - // `pulls.update`, and the contributor flow adds the single gate comment. + // `pulls.update`, and the contributor flow writes the ownership + // checkpoint comment before the draft conversion. expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); const commentBody = lastEnforcerCommentBody(result); expect(commentBody).toContain("wrong branch ancestry"); @@ -2771,8 +2790,8 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); expect(lastEnforcerCommentBody(result)).toContain(`wrong target branch (${ref})`); expect(result.warnings.some((w) => w.startsWith("setFailed:"))).toBe(true); @@ -2850,9 +2869,9 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", "pulls.update", - "graphql", "issues.createComment", "issues.deleteComment", + "graphql", ])); expect(callsTo(result, "pulls.update")).toEqual([ { @@ -2893,8 +2912,8 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); // The title update carries the title and nothing else. `base`, `state` @@ -3023,8 +3042,8 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); expect(callsTo(result, "pulls.update")).toEqual([ { @@ -3168,8 +3187,8 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(wentWrong)).toEqual(readsWrongBase([ "pulls.update", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); expect(callsTo(wentWrong, "pulls.update")).toEqual([ { @@ -3280,9 +3299,9 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", "pulls.update", - "graphql", "issues.createComment", "issues.deleteComment", + "graphql", ])); expect(result.warnings.join(" ")).toContain("Could not parse stored workflow state"); }); @@ -3488,9 +3507,9 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(wrong)).toEqual(readsWrongBase([ "pulls.update", "pulls.update", - "graphql", "issues.createComment", "issues.deleteComment", + "graphql", ])); expect(lastEnforcerCommentBody(wrong)).toContain('"version":1'); expect(lastEnforcerCommentBody(wrong)).toContain('"active":true'); @@ -3559,19 +3578,20 @@ describe("GitHub Actions hardening", () => { }); test("ownership comment is checkpointed before mutations and finalized after", async () => { - // Ownership is written before title/draft. autoDraftedByBot is claimed and - // checkpointed before convertToDraft so a successful convert followed by a - // failed comment still restores later. + // Ownership is written before the draft mutation. autoDraftedByBot is + // claimed and checkpointed in the consolidated comment BEFORE + // convertToDraft, so a successful convert followed by a failed comment + // write still leaves the bot-created draft owned and restorable. const result = await run({ pr: { base: { ref: "main" }, draft: false } }); // The exact call order pins the ownership discipline: the title is - // prefixed, `autoDraftedByBot` is claimed in state before convertToDraft, - // and the single consolidated comment is written after the mutation. + // prefixed, the ownership comment is written (claiming + // `autoDraftedByBot`), then convertToDraft runs. expect(methodsOf(result)).toEqual(readsWrongBase(CONTRIBUTOR_WRONG_BASE_TAIL)); const methods = methodsOf(result); const ownershipIndex = methods.indexOf("issues.createComment"); const draftIndex = methods.indexOf("graphql"); - expect(ownershipIndex).toBeGreaterThan(draftIndex); + expect(ownershipIndex).toBeLessThan(draftIndex); // The single comment records that the bot drafted. const commentBody = lastReadinessCommentBody(result); @@ -3599,8 +3619,8 @@ describe("GitHub Actions hardening", () => { ]); expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); expect(lastEnforcerCommentBody(result)).toContain('"titlePrefixedByBot":false'); expect(lastEnforcerCommentBody(result)).toContain('"autoDraftedByBot":true'); @@ -3673,8 +3693,8 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); // Already prefixed by the `startsWith` test, so no third prefix is added. expect(callsTo(result, "pulls.update")).toEqual([ @@ -3898,8 +3918,9 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsWrongBase([ "pulls.update", "pulls.update", - "graphql", "issues.createComment", + "graphql", + "issues.updateComment", ])); const commentBody = lastEnforcerCommentBody(result); expect(commentBody).toContain('"autoDraftedByBot":false'); @@ -4033,19 +4054,22 @@ describe("GitHub Actions hardening", () => { ); expect(script).toMatch(/pr-quality-messages\.cjs/); - // Ownership is claimed in state before the draft mutation; the single - // consolidated comment is written once after the mutations (#631: only a - // successful conversion records autoDraftedByBot). + // Ownership is claimed and checkpointed in the consolidated comment BEFORE + // the draft mutation, so a successful convert followed by a failed comment + // write still leaves the bot-created draft owned and restorable. Only a + // failed conversion rewrites the comment with autoDraftedByBot:false. const branchStart = script.indexOf("if (failures.length > 0) {"); expect(branchStart).toBeGreaterThan(-1); const branch = script.slice(branchStart); const ownershipClaimIndex = branch.indexOf("state.autoDraftedByBot = true;"); const draftCallIndex = branch.indexOf("await convertToDraft()"); - const gateWriteIndex = branch.indexOf("await upsertGateComment("); + // The failure path writes the ownership checkpoint through the shared + // `draftComment` helper, which is defined before the draft mutation runs. + const gateWriteIndex = branch.indexOf("const draftComment = (notices) =>"); expect(ownershipClaimIndex).toBeGreaterThan(-1); expect(draftCallIndex).toBeGreaterThan(-1); expect(ownershipClaimIndex).toBeLessThan(draftCallIndex); - expect(gateWriteIndex).toBeGreaterThan(draftCallIndex); + expect(gateWriteIndex).toBeLessThan(draftCallIndex); }); test("docs deployment is pinned, bounded, and scoped to Pages", async () => { From b2220024fe7959ece2620b4be3bd54ada045963e Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 05:41:02 +0200 Subject: [PATCH 4/5] fix(ci): remove duplicate titleUpdates declaration, assert coderabbit config Address the second CodeRabbit re-review round: - tests/ci-workflows.test.ts: remove the duplicated const titleUpdates declaration (parse error) in the ownership-preserving-reset test. - enforce-pr-target.test.cjs: assert .coderabbit.yaml's auto_review has no positive labels filter directly (the workflow never writes a labels block), so a future config change that starves maintainer PRs of reviews is caught. - docs: scope the self-waive sentence to contributor authors; a maintainer who authors the PR can waive (holds push permission, not checklist-gated). Gates: node --test .github/scripts (421 pass), bun test tests/ci-workflows (113 pass), bun run typecheck, bun run privacy:scan, actionlint clean. --- .github/scripts/enforce-pr-target.test.cjs | 21 ++++++++++++++----- .../content/docs/contributing/pr-quality.md | 4 +++- tests/ci-workflows.test.ts | 2 +- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/.github/scripts/enforce-pr-target.test.cjs b/.github/scripts/enforce-pr-target.test.cjs index 52cfe393bc..bcda24f68d 100644 --- a/.github/scripts/enforce-pr-target.test.cjs +++ b/.github/scripts/enforce-pr-target.test.cjs @@ -94,11 +94,22 @@ describe("enforce-pr-target workflow", () => { assert.match(workflow, /github\.rest\.issues\.addLabels/); assert.match(workflow, /github\.rest\.issues\.removeLabel/); assert.match(workflow, /reviewReadyDesired/); - // A positive labels filter in .coderabbit.yaml would restrict ALL reviews - // to labeled PRs (maintainer PRs never carry this label), so the label is - // kept as a visible status marker only and never wired as a CodeRabbit - // auto-review filter. - assert.doesNotMatch(workflow, /labels:\s*\["?review-ready"?\]/); + }); + + it("keeps CodeRabbit auto-review unfiltered so maintainer PRs are not starved", () => { + // A positive `labels:` filter under `reviews.auto_review` in + // `.coderabbit.yaml` would restrict ALL automatic reviews to PRs carrying + // that label. Maintainer PRs never carry `review-ready` (no checklist), so + // such a filter would silently stop CodeRabbit from reviewing maintainer + // PRs. The label is a status marker only; assert the reviewer config + // directly, since the workflow never writes a labels block. + const coderabbit = fs.readFileSync( + path.join(__dirname, "../../.coderabbit.yaml"), + "utf8", + ); + const autoReview = coderabbit.match(/auto_review:[\s\S]*?(?=\n\S|\n\s{2}\S)/); + assert.ok(autoReview, ".coderabbit.yaml must declare auto_review"); + assert.doesNotMatch(autoReview[0], /labels:/); }); it("migrates legacy two-comment PRs and deletes the old comments", () => { diff --git a/docs-site/src/content/docs/contributing/pr-quality.md b/docs-site/src/content/docs/contributing/pr-quality.md index be4c6b7bb9..613f93529b 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -43,7 +43,9 @@ tells you exactly what to change: maintainer (OWNER / COLLABORATOR / MEMBER — repository owners, collaborators, and members) can waive the screenshot requirement with an issue comment saying the change does not touch the GUI - (for example "no gui changes"); the PR author cannot self-waive. + (for example "no gui changes"); a contributor PR author cannot self-waive + (a maintainer who authors the PR can waive, but they already hold push + permission and are not gated by the contributor checklist). Contributor PRs (authors without repository push permission) open in draft and stay there until a four-box review-readiness checklist in the description is complete: local CI green, the branch on the latest `dev` diff --git a/tests/ci-workflows.test.ts b/tests/ci-workflows.test.ts index ccf0345fb5..b5a8e8c2a6 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1643,9 +1643,9 @@ describe("GitHub Actions hardening", () => { })], }); + const titleUpdates = callsTo(result, "pulls.update") as Array<{ title?: string; body?: string }>; // The stale prefix is stripped (the ownership survived the reset long // enough for the strip to run), and the state records ownership cleared. - const titleUpdates = callsTo(result, "pulls.update") as Array<{ title?: string; body?: string }>; expect(titleUpdates.some(u => u.title === "Add a thing")).toBe(true); const readinessBody = lastReadinessCommentBody(result); expect(readinessBody).toContain('"titlePrefixedByBot":false'); From 6ecc79c5216876797b2a01ceb7386d3fc80af9a1 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Thu, 6 Aug 2026 06:09:18 +0200 Subject: [PATCH 5/5] fix(ci): persist checklist-draft ownership, clear stale maintainer comments, docs accuracy Address the Codex re-review round on the consolidated gate: - Checklist-only draft path: persist the ownership checkpoint (autoDraftedByBot true) in the consolidated comment BEFORE convertToDraft, matching the failure path, so a successful convert followed by a failed comment write still leaves the bot-created draft owned and restorable. A failed conversion rewrites the comment to release ownership. An already-draft PR still updates the comment. - Maintainer gate-comment clear: a maintainer PR that was already a draft while it failed a gate (active:true, autoDraftedByBot:false) now clears its stale DRAFT comment to READY once the failures pass, instead of leaving the old wrong-base/description actions up. - Docs (pr-quality.md): describe the findings claim accurately - bot-authored Codex/CodeRabbit threads on the current head must be resolved; the CodeRabbit review-body supplement adds to the count while a bot thread is open, and resolving every bot thread clears the box (it cannot independently block). Tests: node --test .github/scripts (421 pass), bun test tests/ci-workflows (113 pass), bun run typecheck, bun run privacy:scan, actionlint clean. --- .github/workflows/enforce-pr-target.yml | 101 +++++++++++++----- .../content/docs/contributing/pr-quality.md | 15 +-- tests/ci-workflows.test.ts | 34 +++--- 3 files changed, 100 insertions(+), 50 deletions(-) diff --git a/.github/workflows/enforce-pr-target.yml b/.github/workflows/enforce-pr-target.yml index 3e451931c5..cc85e946d2 100644 --- a/.github/workflows/enforce-pr-target.yml +++ b/.github/workflows/enforce-pr-target.yml @@ -933,10 +933,41 @@ jobs: } // No quality failure; the draft is owed by the open checklist. - if (!pr.draft && !draftConverted) { - // Claim draft ownership before the mutation so a successful - // convert followed by a failed comment still restores later. + if (pr.draft) { + // Already a draft: update the gate comment with the open + // checklist status and no conversion needed. + await upsertGateComment(state, { + status: "DRAFT", + statusReason: checklistRequired + ? `review readiness checklist open (${readiness.checked}/${readiness.total} boxes ticked).` + : "PR is kept in draft.", + actions: buildActions(), + readiness, + checklistRequired, + notices: [ + ...revalidationNotice, + "This PR stays in draft until every box above is ticked." + ] + }); + } else if (!draftConverted) { + // Persist the ownership checkpoint BEFORE the mutation so a + // successful convert followed by a failed comment write still + // leaves the bot-created draft owned and restorable. The + // failure path above uses the same ordering via draftComment. state.autoDraftedByBot = true; + await upsertGateComment(state, { + status: "DRAFT", + statusReason: checklistRequired + ? `review readiness checklist open (${readiness.checked}/${readiness.total} boxes ticked).` + : "PR is kept in draft.", + actions: buildActions(), + readiness, + checklistRequired, + notices: [ + ...revalidationNotice, + "This PR stays in draft until every box above is ticked." + ] + }); try { await convertToDraft(); draftConverted = true; @@ -951,23 +982,25 @@ jobs: } } - const notices = [ - ...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." - ]; - - await upsertGateComment(state, { - status: "DRAFT", - statusReason: checklistRequired - ? `review readiness checklist open (${readiness.checked}/${readiness.total} boxes ticked).` - : "PR is kept in draft.", - actions: buildActions(), - readiness, - checklistRequired, - notices - }); + // A failed conversion must still surface in the persisted state: + // the checkpoint above claimed ownership, so a failure rewrites + // it to release ownership and tell the author what to do. + if (!draftConverted && !pr.draft) { + state.autoDraftedByBot = false; + await upsertGateComment(state, { + status: "DRAFT", + statusReason: checklistRequired + ? `review readiness checklist open (${readiness.checked}/${readiness.total} boxes ticked).` + : "PR is kept in draft.", + actions: buildActions(), + readiness, + checklistRequired, + notices: [ + ...revalidationNotice, + "Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked." + ] + }); + } return; } @@ -1053,13 +1086,13 @@ jobs: // surface but there is nothing to tick; only render it if the // author is a maintainer and no checklist is required. if (!checklistRequired) { - // `pr.draft` is the value from before the ready path ran; when - // that path already converted the PR (`readyConverted`), the - // recovery must not call markReadyForReview a second time on the - // stale draft state. When the ready path already attempted and - // failed (`readyConversionFailed`), the failure state is carried - // in `readyState`/`recoveredState` and only the comment is - // rewritten, so a later run retries. + // A maintainer PR the gate drafted (`autoDraftedByBot`) must be + // restored when its failures clear. A maintainer PR that was + // already a draft while it failed a gate still carries an active + // gate comment (`active: true` with stale DRAFT actions) that + // must be cleared to READY even though the bot never converted + // it — otherwise the old comment keeps telling the author to fix + // already-passed gates. if (gateState.autoDraftedByBot && pr.draft) { let recoveryFailed = false; if (!readyConverted && !readyConversionFailed) { @@ -1093,6 +1126,20 @@ jobs: }); return; } + if (gateState.active) { + await upsertGateComment( + { ...gateState, active: false, autoDraftedByBot: false }, + { + status: "READY", + statusReason: "all PR quality gates passed.", + actions: [], + readiness, + checklistRequired, + notices: [] + } + ); + return; + } core.info( "All PR quality gates passed and there is no active bot state." ); diff --git a/docs-site/src/content/docs/contributing/pr-quality.md b/docs-site/src/content/docs/contributing/pr-quality.md index 613f93529b..a7948d96b3 100644 --- a/docs-site/src/content/docs/contributing/pr-quality.md +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -63,13 +63,14 @@ tells you exactly what to change: Before a completion is accepted, the gate verifies the checklist claims it can check itself: the head's `ci` check must be green, the branch must be on the latest `dev` commit or at most 10 commits behind it, and every Codex and - CodeRabbit review thread on the PR must be resolved. CodeRabbit findings - that fall outside the diff range and are reported only in a review body on - the current head are counted the same way while a bot review thread is open; - resolving every bot thread clears them. A disproved claim unticks the - matching box and keeps the PR a draft. When the checklist is complete and - every gate is green, the gate adds a `review-ready` label as a visible - status marker at the ready moment. + CodeRabbit review thread authored by a review bot on the current head must be + resolved (unresolved threads from other authors do not block). CodeRabbit + findings that fall outside the diff range and are reported only in a review + body on the current head add to the unresolved count while a bot review + thread is open; resolving every bot thread clears the box. A disproved claim + unticks the matching box and keeps the PR a draft. When the checklist is + complete and every gate is green, the gate adds a `review-ready` label as a + visible status marker at the ready moment. - **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 b5a8e8c2a6..c5b635d65c 100644 --- a/tests/ci-workflows.test.ts +++ b/tests/ci-workflows.test.ts @@ -1196,13 +1196,14 @@ describe("GitHub Actions hardening", () => { /** * The writes a fresh contributor PR triggers on `dev` with no quality - * failures: inject the checklist, convert to draft, then write the single - * consolidated comment. + * failures: inject the checklist, then the ownership checkpoint comment + * (claiming `autoDraftedByBot` before the mutation), then the draft + * conversion. */ const CONTRIBUTOR_CLEAN_TAIL = [ "pulls.update", - "graphql", "issues.createComment", + "graphql", ]; /** @@ -1278,8 +1279,9 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", - "graphql", "issues.createComment", + "graphql", + "issues.updateComment", ])); // Only a successful conversion records autoDraftedByBot; a failed one // clears it so a later permission recovery cannot leave the bot-created @@ -1392,9 +1394,9 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.get", "pulls.update", - "graphql", "issues.createComment", "issues.deleteComment", + "graphql", ])); const [resetBody] = callsTo(result, "pulls.update") as [{ body: string }]; expect(resetBody.body).toContain(CHECKLIST_START); @@ -1524,8 +1526,8 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.get", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); const drafts = callsTo(result, "graphql") as [{ query: string }]; expect(drafts).toHaveLength(1); @@ -1559,8 +1561,8 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.get", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); const drafts = callsTo(result, "graphql") as [{ query: string }]; expect(drafts).toHaveLength(1); @@ -1597,8 +1599,8 @@ describe("GitHub Actions hardening", () => { "pulls.listReviews", "pulls.get", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; // Only the CI box is unticked; the other three stay checked. @@ -1672,8 +1674,8 @@ describe("GitHub Actions hardening", () => { "pulls.listReviews", "pulls.get", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; // Only the latest-dev box is unticked; CI stays checked. @@ -1712,8 +1714,8 @@ describe("GitHub Actions hardening", () => { "pulls.listReviews", "pulls.get", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); @@ -1745,8 +1747,8 @@ describe("GitHub Actions hardening", () => { "pulls.listReviews", "pulls.get", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); @@ -1830,8 +1832,8 @@ describe("GitHub Actions hardening", () => { "pulls.listReviews", "pulls.get", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; expect(bodyUpdate.body).toContain("- [ ] All CI tests are green on my local testing."); @@ -1858,8 +1860,8 @@ describe("GitHub Actions hardening", () => { "pulls.listReviews", "pulls.get", "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); const [bodyUpdate] = callsTo(result, "pulls.update") as [{ body: string }]; // Only the findings box is unticked; CI and latest-dev stay checked. @@ -2177,9 +2179,9 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.get", - "graphql", "issues.createComment", "issues.deleteComment", + "graphql", ])); // No body rewrite: the boxes are already unticked from the failed reset. expect(callsTo(result, "pulls.update")).toEqual([]); @@ -2318,8 +2320,8 @@ describe("GitHub Actions hardening", () => { }); expect(methodsOf(duringFailure)).toEqual(readsAllowedBase([ "pulls.update", - "graphql", "issues.createComment", + "graphql", ])); expect(lastReadinessCommentBody(duringFailure)).toContain('"autoDraftedByBot":true'); @@ -3363,9 +3365,9 @@ describe("GitHub Actions hardening", () => { expect(methodsOf(result)).toEqual(readsAllowedBase([ "pulls.update", "pulls.update", - "graphql", "issues.createComment", "issues.deleteComment", + "graphql", ])); const cleared = lastReadinessCommentBody(result); expect(cleared).toContain('"active":true');