fix(scripts): pre-merge gate reads issue comments, not just review comments (#1549)

* fix(scripts): pre-merge gate reads issue comments, not just review comments

Codex posts clean verdicts ("Didn't find any major issues") as ISSUE
comments; pulls/{n}/comments returns only inline review comments, so
clean-verdict PRs were blocked forever despite an explicit sign-off,
contradicting the script's own documented contract (reviews, comments,
check runs). Verified against PR #1543 and #1545: BLOCKED -> OK.

* test(scripts): teach the pre-merge gh stub the issue-comments endpoint

The fake gh fell through to 'unexpected gh invocation' on the new
issues/{n}/comments lookup, failing all allow-merge scenarios (codex review
on #1549). Adds the endpoint plus two scenarios: a codex clean-verdict
issue comment satisfies the gate (the exact case the fix exists for), and
an unreadable issue-comments feed blocks.

* fix(scripts): require fresh clean-verdict codex issue comments

Issue comments are PR-wide, not head-scoped: a clean Codex verdict
posted before the latest push counted as reviewer activity for the new
head. The gate now keeps only issue comments created after the head
commit and, for Codex, requires the body to be a clean verdict —
findings arrive as review comments, so only the sign-off is an issue
comment. Adds stale and non-verdict scenarios to the gh stub.

* fix(scripts): pin codex issue-comment verdicts to the head SHA

Timestamp comparison against the head commit's committer date cannot
prove a verdict reviewed this SHA (committer dates survive cherry-picks
and rebases) and the unconditional commits API call blocked the gate on
transient failures even when no issue comments existed. Codex embeds
'Reviewed commit: <short sha>' in its clean verdicts, so the gate now
counts an issue comment only when it is a codex clean verdict whose
embedded SHA is a prefix of the current head — no timestamps, no extra
API call, and non-codex issue comments never count.

* fix(scripts): extract the reviewed-commit SHA instead of substring search

Searching for the head's first ten characters anywhere in the body
could accept a verdict whose prose merely mentions the SHA and rejects
verdicts where codex embeds a shorter short SHA. The gate now extracts
the hex run following the 'Reviewed commit' label (>= 7 chars, git's
short-SHA floor, markdown decoration tolerated) and requires it to be
a prefix of the current head. Stub head is now a realistic 40-char SHA;
adds short-SHA acceptance and SHA-in-prose rejection scenarios.

* fix(scripts): skip unpinned codex verdicts instead of aborting the gate

Under set -euo pipefail, the SHA-extraction grep pipeline exits nonzero
when a clean verdict has no 'Reviewed commit' label, killing the whole
gate. Tolerate the no-match case with || true so legacy unpinned
verdicts are simply never counted while reviews and check runs still
satisfy the reviewer requirement.

Author: joshuaswarren

scripts/pre-merge-check.sh

@@ -108,6 +108,16 @@ if ! COMMENTS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" --paginate --
   exit 1
 fi
 
+# `pulls/{n}/comments` returns only inline REVIEW comments. When Codex finds
+# no issues it posts its verdict ("Didn't find any major issues") as an ISSUE
+# comment, which lives on `issues/{n}/comments` — without this, clean-verdict
+# PRs are blocked forever even though the reviewer explicitly signed off.
+# Body newlines/tabs are flattened so each comment stays one TSV row.
+if ! ISSUE_COMMENTS_RAW=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate --jq '.[] | [.user.login, (.body // "" | gsub("[\r\n\t]"; " "))] | @tsv' 2>/dev/null); then
+  echo "[pre-merge] BLOCKED: Failed to read PR issue comments from GitHub."
+  exit 1
+fi
+
 # Some bots (Cursor Bugbot, Kilo Code Review) post as check runs via GitHub
 # Apps rather than as PR comments. A completed check run counts as reviewer
 # activity. The app.slug field maps to reviewer aliases (e.g. "cursor" matches
@@ -126,7 +136,30 @@ if [[ -n "$HEAD_SHA" ]]; then
   fi
 fi
 
-ALL_REVIEWERS=$(printf '%s\n%s\n' "$REVIEWS" "$COMMENTS" | sort -u)
+# Issue comments are PR-wide, not head-scoped, and this gate reads them for
+# exactly one reason: Codex posts its clean verdict ("Didn't find any major
+# issues … **Reviewed commit:** \`<short sha>\`") as an issue comment. Count
+# ONLY those, and only when the SHA embedded after the "Reviewed commit"
+# label is a prefix (>= 7 hex chars, git's short-SHA floor) of the CURRENT
+# head. Timestamps cannot prove a verdict reviewed this SHA (committer dates
+# survive cherry-picks and rebases); the extracted SHA pin can. Every other
+# issue comment is conversation, never reviewer activity.
+ISSUE_COMMENTERS=""
+while IFS=$'\t' read -r ic_login ic_body; do
+  [[ "$ic_login" == "chatgpt-codex-connector[bot]" ]] || continue
+  grep -qiE "find any major issues|no major issues" <<< "$ic_body" || continue
+  # Extract the hex run following the "Reviewed commit" label, tolerating
+  # markdown decoration (bold markers, backticks, colon) between the two.
+  # `|| true` keeps errexit/pipefail from aborting the gate on comments with
+  # no label (legacy unpinned verdicts) — those are simply never counted.
+  ic_sha=$(grep -oiE "reviewed commit[^0-9a-fA-F]*[0-9a-fA-F]{7,40}" <<< "$ic_body" \
+    | grep -oE "[0-9a-fA-F]{7,40}" | head -n 1 | tr '[:upper:]' '[:lower:]' || true)
+  if [[ -n "$ic_sha" && -n "$HEAD_SHA" && "$HEAD_SHA" == "$ic_sha"* ]]; then
+    ISSUE_COMMENTERS+="${ic_login}"$'\n'
+  fi
+done <<< "$ISSUE_COMMENTS_RAW"
+
+ALL_REVIEWERS=$(printf '%s\n%s\n%s\n' "$REVIEWS" "$COMMENTS" "$ISSUE_COMMENTERS" | sort -u)
 
 has_reviewer_check_run() {
   local reviewer="$1"

tests/pre-merge-check.test.mjs

@@ -42,7 +42,7 @@ if [[ "$1" == "api" && "$2" == "graphql" ]]; then
     malformed_page:1)
       printf '5\\t0\\n'
       ;;
-    cursor_check_ok:1|unrelated_cursor_check:1|all_required_check_runs_ok:1)
+    cursor_check_ok:1|unrelated_cursor_check:1|all_required_check_runs_ok:1|codex_issue_comment_ok:1|codex_issue_comment_short_sha:1|codex_issue_comment_stale:1|codex_issue_comment_not_verdict:1|codex_verdict_sha_in_prose:1|codex_verdict_unpinned:1|generic_issue_comment_ignored:1|issue_comments_fail:1)
       printf '3\\t0\\tfalse\\t\\n'
       ;;
     repeated_cursor:1)
@@ -68,6 +68,10 @@ if [[ "$1" == "api" && "$2" == "repos/example/repo/pulls/7/reviews" ]]; then
     printf 'chatgpt-codex-connector[bot]\\n'
     exit 0
   fi
+  if [[ "$GH_STUB_SCENARIO" == codex_issue_comment_* || "$GH_STUB_SCENARIO" == "codex_verdict_sha_in_prose" || "$GH_STUB_SCENARIO" == "generic_issue_comment_ignored" ]]; then
+    printf 'cursor[bot]\\n'
+    exit 0
+  fi
   if [[ "$GH_STUB_SCENARIO" == "all_required_check_runs_ok" ]]; then
     exit 0
   fi
@@ -79,16 +83,52 @@ if [[ "$1" == "api" && "$2" == "repos/example/repo/pulls/7/comments" ]]; then
   exit 0
 fi
 
+if [[ "$1" == "api" && "$2" == "repos/example/repo/issues/7/comments" ]]; then
+  if [[ "$GH_STUB_SCENARIO" == "issue_comments_fail" ]]; then
+    echo "issue comments unavailable" >&2
+    exit 3
+  fi
+  if [[ "$GH_STUB_SCENARIO" == "codex_issue_comment_ok" ]]; then
+    printf "chatgpt-codex-connector[bot]\\tCodex Review: Didn't find any major issues. **Reviewed commit:** deadbeef12\\n"
+    exit 0
+  fi
+  if [[ "$GH_STUB_SCENARIO" == "codex_issue_comment_short_sha" ]]; then
+    printf "chatgpt-codex-connector[bot]\\tCodex Review: Didn't find any major issues. Reviewed commit: deadbee\\n"
+    exit 0
+  fi
+  if [[ "$GH_STUB_SCENARIO" == "codex_issue_comment_stale" ]]; then
+    printf "chatgpt-codex-connector[bot]\\tCodex Review: Didn't find any major issues. **Reviewed commit:** cafebabe12\\n"
+    exit 0
+  fi
+  if [[ "$GH_STUB_SCENARIO" == "codex_issue_comment_not_verdict" ]]; then
+    printf "chatgpt-codex-connector[bot]\\tFound 2 major issues in the diff. **Reviewed commit:** deadbeef12\\n"
+    exit 0
+  fi
+  if [[ "$GH_STUB_SCENARIO" == "codex_verdict_sha_in_prose" ]]; then
+    printf "chatgpt-codex-connector[bot]\\tCodex Review: Didn't find any major issues. Compare with deadbeef12 later. **Reviewed commit:** cafebabe12\\n"
+    exit 0
+  fi
+  if [[ "$GH_STUB_SCENARIO" == "codex_verdict_unpinned" ]]; then
+    printf "chatgpt-codex-connector[bot]\\tCodex Review: Didn't find any major issues. Hooray!\\n"
+    exit 0
+  fi
+  if [[ "$GH_STUB_SCENARIO" == "generic_issue_comment_ignored" ]]; then
+    printf "someuser\\tLooks good to me! Reviewed commit: deadbeef12\\n"
+    exit 0
+  fi
+  exit 0
+fi
+
 if [[ "$1" == "pr" && "$2" == "view" ]]; then
   if [[ "$*" != *"--repo example/repo"* ]]; then
     echo "gh pr view must pass --repo" >&2
     exit 5
   fi
-  printf 'deadbeef\\n'
+  printf 'deadbeef1234567890abcdef1234567890abcdef\\n'
   exit 0
 fi
 
-if [[ "$1" == "api" && "$2" == "repos/example/repo/commits/deadbeef/check-runs" ]]; then
+if [[ "$1" == "api" && "$2" == "repos/example/repo/commits/deadbeef1234567890abcdef1234567890abcdef/check-runs" ]]; then
   if [[ "$GH_STUB_SCENARIO" == "cursor_check_ok" ]]; then
     printf 'cursor\\tCursor Bugbot\\n'
     exit 0
@@ -194,6 +234,91 @@ test("pre-merge check fails closed when GitHub pagination does not advance", asy
   });
 });
 
+test("pre-merge check accepts a codex clean-verdict ISSUE comment as reviewer activity", async () => {
+  // Codex posts "no major issues" verdicts as issue comments, not reviews or
+  // review comments — the endpoint gap this PR fixes.
+  await withGhStub("codex_issue_comment_ok", async (env, countPath) => {
+    const result = runPreMergeCheck(env);
+
+    assert.equal(result.status, 0, result.stderr);
+    assert.match(result.stdout, /OK: All reviewers posted/);
+    assert.equal((await readFile(countPath, "utf8")).trim(), "1");
+  });
+});
+
+test("pre-merge check accepts codex verdicts with a short (7-char) reviewed-commit SHA", async () => {
+  // Codex controls its own short-SHA length; the gate must accept any git
+  // short SHA (>= 7 hex chars) that is a prefix of the current head.
+  await withGhStub("codex_issue_comment_short_sha", async (env) => {
+    const result = runPreMergeCheck(env);
+
+    assert.equal(result.status, 0, result.stderr);
+    assert.match(result.stdout, /OK: All reviewers posted/);
+  });
+});
+
+test("pre-merge check anchors the SHA pin to the Reviewed commit label, not prose", async () => {
+  // The head SHA appearing elsewhere in the body must not satisfy the pin
+  // when the verdict's own "Reviewed commit" points at a different SHA.
+  await withGhStub("codex_verdict_sha_in_prose", async (env) => {
+    const result = runPreMergeCheck(env);
+
+    assert.equal(result.status, 1);
+    assert.match(result.stdout, /Missing reviews from: chatgpt-codex-connector\[bot\]/);
+  });
+});
+
+test("pre-merge check ignores unpinned codex verdicts without aborting the gate", async () => {
+  // A legacy clean verdict with no "Reviewed commit" label must be skipped —
+  // not crash the errexit/pipefail script — so reviewers satisfied via
+  // reviews/check runs still pass.
+  await withGhStub("codex_verdict_unpinned", async (env) => {
+    const result = runPreMergeCheck(env);
+
+    assert.equal(result.status, 0, result.stderr);
+    assert.match(result.stdout, /OK: All reviewers posted/);
+  });
+});
+
+test("pre-merge check rejects codex verdicts pinned to a different commit", async () => {
+  // A clean verdict earned on a previous head must not carry over to new
+  // commits — the comment feed is PR-wide, not SHA-scoped. The verdict's
+  // embedded "Reviewed commit" SHA must be a prefix of the current head.
+  await withGhStub("codex_issue_comment_stale", async (env) => {
+    const result = runPreMergeCheck(env);
+
+    assert.equal(result.status, 1);
+    assert.match(result.stdout, /Missing reviews from: chatgpt-codex-connector\[bot\]/);
+  });
+});
+
+test("pre-merge check ignores codex issue comments that are not clean verdicts", async () => {
+  await withGhStub("codex_issue_comment_not_verdict", async (env) => {
+    const result = runPreMergeCheck(env);
+
+    assert.equal(result.status, 1);
+    assert.match(result.stdout, /Missing reviews from: chatgpt-codex-connector\[bot\]/);
+  });
+});
+
+test("pre-merge check never counts non-codex issue comments as reviewer activity", async () => {
+  await withGhStub("generic_issue_comment_ignored", async (env) => {
+    const result = runPreMergeCheck(env);
+
+    assert.equal(result.status, 1);
+    assert.match(result.stdout, /Missing reviews from: chatgpt-codex-connector\[bot\]/);
+  });
+});
+
+test("pre-merge check blocks when issue comments cannot be read", async () => {
+  await withGhStub("issue_comments_fail", async (env) => {
+    const result = runPreMergeCheck(env);
+
+    assert.equal(result.status, 1);
+    assert.match(result.stdout + result.stderr, /Failed to read PR issue comments/);
+  });
+});
+
 test("pre-merge check accepts the Cursor Bugbot check run as reviewer activity", async () => {
   await withGhStub("cursor_check_ok", async (env, countPath) => {
     const result = runPreMergeCheck(env);