ci: harden PR readiness gate — consolidated comment, draft-review trigger, G.UI waiver

.github/scripts/enforce-pr-target.test.cjs

@@ -48,6 +48,78 @@ describe("enforce-pr-target workflow", () => {
     assert.match(workflow, /synchronize/);
   });
 
+  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", () => {
+    // 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/);
+  });
+
+  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 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/);
+  });
+
+  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", () => {
+    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.

.github/scripts/pr-quality-messages.cjs

@@ -5,11 +5,17 @@ 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. */
+/**
+ * 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 = "<!-- pr-quality-readiness -->";
+/** Marks the bot's consolidated PR gate message. */
+const GATE_MARKER = "<!-- opencodex-pr-gate -->";
 
 function inlineCode(value) {
   const text = String(value);
@@ -29,32 +35,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),
-    "",
-    "## Review readiness checklist",
-    "",
-    readiness.present
-      ? "This PR is kept in **draft** until every requirement below is fulfilled. The tickable checklist has been added to your PR description — tick all four boxes there."
-      : "The review readiness checklist is not required for this author.",
+    GATE_MARKER,
+    gateStateMarker(state),
     "",
-    ...(readiness.present ? readinessChecklistLines(readiness) : []),
+    `## ${statusEmoji} ${status}`,
+    statusReason ? `- ${statusReason}` : "",
     "",
-    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 +209,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 +253,14 @@ function buildStaleNotice({ completionHeadSha, liveHeadSha, eventAction }) {
 
 module.exports = {
   READINESS_MARKER,
+  GATE_MARKER,
   inlineCode,
   readinessChecklistLines,
-  buildReadinessCommentBody,
+  buildGateCommentBody,
   descriptionFailureLines,
   buildFailureSections,
   failureSummary,
   buildStaleNotice,
-  buildClaimCheckNotice
+  buildClaimCheckNotice,
+  buildFindingsClaimNotice
 };

.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('<!-- pr-quality-readiness-state:{"version":2'));
+  it("carries the marker, serialized state, status, mirror, and tick count", () => {
+    const state = { version: 1, maintainersPinged: false };
+    const body = buildGateCommentBody(state, {
+      status: "DRAFT",
+      statusReason: "review readiness checklist open (1/4 boxes ticked).",
+      actions: ["Tick all four boxes in the PR description once you're done."],
+      readiness,
+      checklistRequired: true,
+      notices: ["extra line"]
+    }).join("\n");
+    assert.ok(body.startsWith(GATE_MARKER));
+    assert.ok(body.includes('<!-- opencodex-pr-gate-state:{"version":1'));
+    assert.ok(body.includes("## ⏳ DRAFT"));
+    assert.ok(body.includes("## What to do"));
+    assert.ok(body.includes("Tick all four boxes"));
+    assert.ok(body.includes("## Review readiness checklist"));
     assert.ok(body.includes("**1/4** boxes ticked."));
     assert.ok(body.includes("extra line"));
-    assert.ok(body.includes("tick all four boxes there."));
   });
 
-  it("states the checklist is not required without claiming the PR is ready", () => {
-    const body = buildReadinessCommentBody(
-      { version: 2 },
-      { present: false, complete: false, checked: 0, total: 0, items: [] },
-      ["⚠️ **Wrong target branch**"],
+  it("renders a ready status without a checklist when not required", () => {
+    const body = buildGateCommentBody(
+      { version: 1 },
+      {
+        status: "READY",
+        statusReason: "this PR is ready for review.",
+        actions: [],
+        readiness: { present: false, complete: false, checked: 0, total: 0, items: [] },
+        checklistRequired: false,
+        notices: ["⚠️ **Wrong target branch**"]
+      },
     ).join("\n");
-    assert.ok(body.includes("not required for this author."));
-    assert.ok(!body.includes("This PR is ready for review"));
-    assert.ok(body.includes("⚠️ **Wrong target branch**"));
+    assert.ok(body.includes("## ✅ READY"));
+    assert.ok(!body.includes("## Review readiness checklist"));
     assert.ok(!body.includes("boxes ticked"));
+    assert.ok(body.includes("⚠️ **Wrong target branch**"));
+  });
+
+  it("does not claim ready when the PR is kept in draft", () => {
+    const body = buildGateCommentBody(
+      { version: 1 },
+      {
+        status: "DRAFT",
+        statusReason: "PR is kept in draft.",
+        actions: [],
+        readiness,
+        checklistRequired: true,
+        notices: []
+      },
+    ).join("\n");
+    assert.ok(!body.includes("READY"));
   });
 });
 
@@ -202,3 +234,23 @@ describe("buildClaimCheckNotice", () => {
     ]);
   });
 });
+
+describe("buildFindingsClaimNotice", () => {
+  it("names each bot with unresolved threads and the untick", () => {
+    const notice = buildFindingsClaimNotice({
+      "chatgpt-codex-connector[bot]": 2,
+      "coderabbitai[bot]": 1,
+    });
+    assert.match(notice[0], /Codex has 2 unresolved findings/);
+    assert.match(notice[0], /\*\*Codex\/CodeRabbit findings\*\* box has been unticked/);
+    assert.match(notice[1], /CodeRabbit has 1 unresolved finding/);
+    assert.match(notice[2], /Resolve every open review conversation/);
+  });
+
+  it("handles a single bot with one thread", () => {
+    const notice = buildFindingsClaimNotice({ "coderabbitai[bot]": 1 });
+    assert.equal(notice.length, 2);
+    assert.match(notice[0], /CodeRabbit has 1 unresolved finding/);
+    assert.match(notice[1], /Resolve every open review conversation/);
+  });
+});