Merge pull request #133 from fiddlerwoaroof/fix/pr-reviews-inline

fix(scripts): pr-reviews reads all three review surfaces

.github/workflows/ci.yml

@@ -273,6 +273,10 @@ jobs:
       # a tag is already public and immutable, so its refusals are checked here.
       - name: The release preflight refuses what it claims to
         run: ./scripts/release-preflight-test.sh
+      # pr-reviews is what stands between a merge and an unread review, and it
+      # was blind to inline comments for as long as it existed (improvise-xgra).
+      - name: The PR review reader refuses what it claims to
+        run: ./scripts/pr-reviews-test.sh
 
   # ---------------------------------------------------------------------------
   # Lints the one crate the `test` job cannot reach.

context/code/pr-review.md

@@ -9,6 +9,97 @@ PR, every one a P1.
 `scripts/pr-reviews <n>` prints them. `--wait <sec>` blocks until one lands
 against the current head.
 
+## Three surfaces, because a reviewer writes on whichever one is nearest
+
+GitHub has three places to put a review, and they are three different API
+endpoints:
+
+| printed as | endpoint | what it is |
+|---|---|---|
+| `[REVIEW]` | `pulls/{n}/reviews` | the review's own body |
+| `[INLINE]` | `pulls/{n}/comments` | a comment on a line of the diff |
+| `[COMMENT]` | `issues/{n}/comments` | a comment on the PR thread |
+
+**`pr-reviews` read only the first for as long as it existed** (improvise-xgra),
+and the surface it could not see is the one a human is most likely to use. On
+PR #122 the user reviewed with the body *"Generally good, added comments
+inline."* and put five substantive rulings underneath it as inline comments.
+`pr-reviews` printed the one-line body faithfully and nothing else, and an
+agent reported the PR as awaiting review while the answers sat on it. The user
+had to say so.
+
+`scripts/pr-watch` does not cover the gap: that one watches **checks**. Nothing
+in `scripts/` surfaced the surface a human writes on.
+
+The three are **interleaved in time order**, not printed in three blocks,
+because a review body and its inline comments are one act of reading and a
+block-per-endpoint layout hides which body the rulings belong under.
+
+## What satisfies `--wait`
+
+An **inline comment at the current head does**, and that is the whole point of
+the fix rather than an incidental widening. `--wait`'s question is *has anyone
+read this tree yet*, and a `pulls/{n}/comments` entry carries a `commit_id`: it
+is a review comment, written by a reviewer, about a specific commit. The case
+that failed on #122 is exactly one where the substance was inline — treating it
+as not-a-review would leave `--wait` blind to the reviews it most needs to
+catch. Note that a single "Add single comment" on GitHub creates a review whose
+**body is empty**, so under the old rule that reviewer's exit code was right
+and their words were invisible; the surfaces and the wait have to move together.
+
+An **issue-level comment does not**, because it carries no commit. It makes no
+claim about which tree was read, and "I am rebasing this shortly" satisfying a
+wait for a review would be the widening that looks like the fix.
+
+A comment on an **earlier** head does not satisfy the wait either — same rule
+the review bodies already had, and the same reason: it may be fixed or still
+live, and only reading it tells you which. An outdated comment has `line: null`
+and only `original_line` left, which is what is printed.
+
+**A `commit_id` is per-comment, not per-review, and they differ inside one
+submission.** Measured on PR #122: one review submitted between 19:48 and 19:53
+carries five inline comments, of which three name `246e8f4a` and two name
+`6a4c7dcd`, and the review body names `246e8f4a` — GitHub pins each comment to
+the file version it was written against, so a comment on an outdated diff
+position keeps the older commit while its siblings move on. Two consequences.
+The per-entry `<- an EARLIER head` flag is honest but can make a *current*
+review look partly stale, so read the timestamps together before discounting
+one. And a head comparison against the review **body** is not even a proxy for
+its own comments: on that PR the body read as an earlier head while two of its
+rulings sat on the current one.
+
+## `$?` must be this script's own
+
+The usage text used to warn against piping into `head`/`tail`. That is narrower
+than the real rule: **nothing may follow the script in the same `;` list
+either**, because a trailing `echo "exit=$?"` makes the status the echo's. An
+agent nearly reported a review that did not exist that way. Redirect to a file
+and read the file in a later command.
+
+## Two things the shell and `gh` do that this script got wrong first
+
+Both were found by ChatGPT Codex on PR #133, and both are invisible on macOS.
+
+**A multi-assignment `local` is not left-to-right on bash 5.** Written
+`local n="$1" name="$2" f="$DIR/$n.$name.json"`, bash creates every name
+*before* running the assignments, so `$n` is unbound in the third one and
+`set -u` kills the shell. It works on macOS's bash 3.2, so the whole suite
+passed locally and 17 of 19 cases failed in CI. **The signature to recognise is
+`exit 2` with no output at all**: the dead command substitution handed
+`jq --argjson` an empty string, and jq answers an invalid `--argjson` with a
+usage error, which is 2. So a shell bug at the top of the script arrives
+wearing jq's exit code. Give each `local` its own statement.
+
+**`gh api --paginate` emits one JSON array per page**, not one array. A comment
+thread longer than the page size therefore arrives as several concatenated
+documents, and `--argjson` rejects it with the same exit 2. `jq -s 'add // []'`
+flattens the pages and is a no-op on a single page. The fixture path runs
+through the **same** helper, which is the only reason a fixture can test it at
+all: a fixture that could hold just one array would be exercising a shape the
+live fetch never produces. This was unreachable on every PR in this repo so far
+— #122 has five inline comments — which is exactly why it needed a test rather
+than a look.
+
 ## Why a review is easy to miss here
 
 It arrives as a **review**, not a comment, so reading the comment thread does

context/test-evidence/tooling.md

@@ -219,3 +219,72 @@ The live tree was itself the first injection: before the three merges the check
 exited 1 naming all three pairs with both line numbers; after them it exits 0.
 The clean arms (two headings naming two tests, and prose repeated) are what
 stop an always-refuse from satisfying the rest.
+
+### The PR review reader's own suite — `scripts/pr-reviews-test.sh`
+
+improvise-xgra. Like `kb-test.sh` and `pr-watch-test.sh`, its cases are already
+injected faults and none of their names is a Rust `fn` the evidence checker
+could resolve, so the case list is the evidence. What is recorded here is the
+five faults it was watched to fail against, and which case caught each — a
+suite that only asked "did it print something" passes all five.
+
+Fault 1, **the inline surface dropped entirely**, which is the live bug as it
+shipped: `inline: []` in both fetch paths. Eight cases fail, including both
+`--wait` arms. This is the one that matters, because the old script's behaviour
+*was* this fault and it exited 0 on PR #122's review body while printing none
+of the five rulings underneath it.
+
+Fault 2, **an inline comment counted rather than compared**:
+`((.inline|length) > 0)` in place of the `commit_id == head` test. One case
+fails, `an inline comment at an EARLIER head does not satisfy --wait`, and that
+is the pair the two inline fixtures exist to discriminate — the at-head case
+passes under this fault, so on its own it proves nothing.
+
+Fault 3, **the time sort dropped** (`| .` for `| sort_by(.at)`). Only
+`the three surfaces interleave in time order` fails: it reads the three bodies
+back as `SECOND THIRD FIRST`, the per-endpoint concatenation order. Every
+content assertion still passes, because each body is still printed.
+
+Fault 4, **no `original_line` fallback**. Only the outdated-comment case fails.
+`line` is null exactly when the line has moved under the comment, so a renderer
+reading `.line` alone prints `draw.rs:null` on precisely the comments whose
+location is hardest to recover by hand.
+
+Fault 5, **`--wait` widened to any issue-level comment**. Only
+`an issue-level comment does not satisfy --wait` fails. That is the fault a
+well-meaning repair would introduce — "wait for anyone to say anything" — and
+the fixture that separates it is a PR thread comment with no `commit_id` at
+all.
+
+Two cases are there for the contract rather than the bug:
+`a review body at the current head still satisfies --wait` and
+`--wait 0 over an empty PR is success`, because other briefs depend on exit 0
+and exit 1 meaning what they did before. `no PR number is exit 4` pins the
+third code, which is distinguishable from 1 only if nothing collapses them.
+
+`the usage warns about a trailing $? reader` asserts a sentence rather than a
+behaviour, which is unusual and deliberate: the `; echo "exit=$?"` trap costs a
+wrong diagnosis every time it is hit, the warning is the only guard against it,
+and a header is exactly the thing a later edit trims without noticing.
+
+Three more cases were added after ChatGPT Codex reviewed PR #133, and the pair
+of bugs it found is the reason the suite has to run in CI rather than only on
+the author's machine. Both produce **exit 2 with no output whatsoever**, which
+is jq refusing an invalid `--argjson` — so a shell fault at the top of the
+script arrives wearing jq's exit code, and 17 of 19 cases failed identically
+with nothing to read.
+
+The first, a multi-assignment `local` whose third clause reads its first, is
+fine on macOS's bash 3.2 and fatal on bash 5 under `set -u`. **The suite passed
+19/19 locally and 2/19 in CI on the same commit**, which is the case for wiring
+it into the `kb` job in the same change rather than afterwards.
+
+The second is `gh api --paginate` emitting one array per page. Fault 6 removes
+`| pages_to_array` from the fixture path and exactly the three new cases fail —
+`a comment on the first page survives pagination`, `…the second page…`, and
+`a paginated thread still satisfies --wait` (exit 2, the live signature). The
+fixture carries two concatenated arrays and both paths flatten through the same
+helper; a fixture holding one array could not have caught this, because it
+would exercise a shape the live fetch never produces. No PR in this repo has
+yet had enough inline comments to reach a second page, so the test is the only
+thing that will ever see it before a user does.

scripts/pr-reviews

@@ -1,28 +1,43 @@
 #!/usr/bin/env bash
-# Prints a PR's reviews, and optionally waits for one against the current head.
+# Prints everything a reviewer may have written on a PR -- review bodies,
-# Exists because twenty PRs merged with their reviews unread; see
+# INLINE review comments and issue-level comments -- and optionally waits for
-# context/code/pr-review.md.
+# one against the current head. See context/code/pr-review.md.
 set -euo pipefail
 
 usage() {
   cat >&2 <<'EOF'
 usage: pr-reviews <pr-number> [--wait <sec>] [--interval <sec>]
 
-  --wait <sec>      wait up to this long for a review whose commit is the
+  --wait <sec>      wait up to this long for a review or an inline review
-                    current head (default 0 = print what exists and exit)
+                    comment whose commit is the current head
+                    (default 0 = print what exists and exit)
   --interval <sec>  seconds between polls (default 30)
 
-exit 0  a review against the current head exists (or --wait was 0)
+exit 0  something against the current head exists (or --wait was 0)
-exit 1  --wait elapsed with no review against the current head
+exit 1  --wait elapsed with nothing against the current head
 exit 4  usage or environment error
 
-DO NOT PIPE THIS INTO head/tail. `$?` is then the pager's status, not this
+THIS SCRIPT'S `$?` MUST BE THE ONE YOU READ. Nothing may follow it in the same
-script's, and a --wait that timed out reads as success. That trap has cost
+pipeline OR the same `;` list: `pr-reviews 1 --wait 60 | tail` gives you tail's
-this repo four separate diagnoses. Redirect to a file and read it instead.
+status, and so does `pr-reviews 1 --wait 60; echo "exit=$?"` -- the echo runs
+first and $? is then the echo's. Either way a --wait that TIMED OUT reads as
+success. That trap has cost this repo five separate diagnoses, the most recent
+an agent that nearly reported a review which did not exist. Redirect to a file,
+then read the file in a later command.
 
-A review may target an EARLIER head, which is why the head is compared rather
+Three surfaces, because a reviewer writes on whichever one is nearest:
-than the count: a finding against an old commit may be fixed or still live, and
+  [REVIEW]  the review's own body           (pulls/{n}/reviews)
-only reading it tells you which.
+  [INLINE]  a comment on a line of the diff (pulls/{n}/comments)
+  [COMMENT] a comment on the PR thread      (issues/{n}/comments)
+They are interleaved in time order. INLINE is the one a human is most likely to
+use and the one this script was blind to until improvise-xgra: PR #122 was
+reviewed with the body "Generally good, added comments inline." and five
+substantive rulings underneath it, of which this script printed none.
+
+A review or comment may target an EARLIER head, which is why the head is
+compared rather than the count: a finding against an old commit may be fixed or
+still live, and only reading it tells you which. An issue-level [COMMENT] is
+attached to no commit at all, so it can never satisfy --wait.
 EOF
   exit 4
 }
@@ -48,24 +63,91 @@ gh_() {
   fi
 }
 
-fetch() { gh_ pr view "$PR" --json headRefOid,reviews; }
+# One poll reads all three surfaces, so the counter is bumped once per poll and
+# not once per file. It lives on disk because fetch is called as "$(fetch)" and
+# a command substitution is a SUBSHELL -- a variable incremented in it is lost,
+# which is how pr-watch's fixtures could never reach their second poll.
+fixture_poll() {
+  local c="$PR_REVIEWS_FIXTURE_DIR/.poll"
+  local n=$(( $(cat "$c" 2>/dev/null || echo 0) + 1 ))
+  echo "$n" > "$c"
+  echo "$n"
+}
+
+# Each `local` gets its own statement. Bash 5 creates every name in a
+# multi-assignment `local` before running the assignments, so a later one
+# reading an earlier one is unbound under `set -u` -- see context/code/pr-review.md.
+fixture_file() {
+  local n="$1"
+  local name="$2"
+  local f="$PR_REVIEWS_FIXTURE_DIR/$n.$name.json"
+  [ -f "$f" ] || f="$PR_REVIEWS_FIXTURE_DIR/last.$name.json"
+  [ -f "$f" ] && cat "$f" || echo 'null'
+}
+
+# `gh api --paginate` emits one JSON array PER PAGE, so a comment thread past
+# the page size arrives as several concatenated documents that --argjson
+# rejects. Both paths flatten through here, so a fixture can carry two pages.
+pages_to_array() { jq -s 'add // []'; }
+
+fetch() {
+  if [ -n "${PR_REVIEWS_FIXTURE_DIR:-}" ]; then
+    local n; n=$(fixture_poll)
+    local pr; pr=$(fixture_file "$n" pr)
+    local inline; inline=$(fixture_file "$n" inline | pages_to_array)
+    local issue; issue=$(fixture_file "$n" issue | pages_to_array)
+    jq -n --argjson pr "$pr" --argjson inline "$inline" --argjson issue "$issue" \
+      '{head: ($pr.headRefOid // ""), reviews: ($pr.reviews // []),
+        inline: $inline, issue: $issue}'
+    return
+  fi
+  local repo; repo=$(gh_ repo view --json nameWithOwner -q .nameWithOwner)
+  local pr; pr=$(gh_ pr view "$PR" --json headRefOid,reviews)
+  local inline; inline=$(gh_ api "repos/$repo/pulls/$PR/comments" --paginate | pages_to_array)
+  local issue; issue=$(gh_ api "repos/$repo/issues/$PR/comments" --paginate | pages_to_array)
+  jq -n --argjson pr "$pr" --argjson inline "$inline" --argjson issue "$issue" \
+    '{head: ($pr.headRefOid // ""), reviews: ($pr.reviews // []),
+      inline: $inline, issue: $issue}'
+}
+
+# `line` is null once the line has moved under the comment; `original_line` is
+# where it was written, and is all an outdated comment has left.
+normalize='
+  [ (.reviews[]? | {at: .submittedAt, kind: "REVIEW", state: (.state // ""),
+                    who: (.author.login // "?"), commit: (.commit.oid // ""),
+                    at_line: "", body: (.body // "")}),
+    (.inline[]?  | {at: .created_at, kind: "INLINE", state: "",
+                    who: (.user.login // "?"), commit: (.commit_id // ""),
+                    at_line: "\(.path // "?"):\(.line // .original_line // "?")",
+                    body: (.body // "")}),
+    (.issue[]?   | {at: .created_at, kind: "COMMENT", state: "",
+                    who: (.user.login // "?"), commit: "",
+                    at_line: "", body: (.body // "")}) ]
+  | sort_by(.at)'
 
 report() {
   local json="$1" head
-  head=$(printf '%s' "$json" | jq -r '.headRefOid')
+  head=$(printf '%s' "$json" | jq -r '.head')
   printf 'pr-reviews: #%s head=%s\n' "$PR" "${head:0:8}"
-  printf '%s' "$json" | jq -r --arg head "$head" '
+  printf '%s' "$json" | jq -r --arg head "$head" "$normalize"'
-    if (.reviews | length) == 0 then "  (no reviews)"
+    | if length == 0 then "  (nothing written on any of the three surfaces)"
-    else .reviews[] |
+      else .[] |
-      "  [\(.state)] \(.submittedAt) commit=\(.commit.oid[0:8])" +
+        "  [\(.kind)\(if .state == "" then "" else " " + .state end)] \(.at) @\(.who)" +
-      (if .commit.oid == $head then " <- CURRENT HEAD" else " <- an EARLIER head; verify whether it still applies" end) +
+        (if .at_line == "" then "" else " " + .at_line end) +
-      "\n" + (.body | split("\n") | map("    " + .) | join("\n"))
+        (if .commit == "" then " <- not attached to a commit"
-    end'
+         elif .commit == $head then " commit=\(.commit[0:8]) <- CURRENT HEAD"
+         else " commit=\(.commit[0:8]) <- an EARLIER head; verify whether it still applies" end) +
+        "\n" + (.body | split("\n") | map("    " + .) | join("\n"))
+      end'
 }
 
+# An inline comment at the head counts, and that is the whole point of the fix:
+# the case that failed was a reviewer whose substance was entirely inline. An
+# issue-level comment carries no commit and so cannot count -- it makes no claim
+# about which tree was read.
 has_head_review() {
-  printf '%s' "$1" | jq -e --arg h "$(printf '%s' "$1" | jq -r .headRefOid)" \
+  printf '%s' "$1" | jq -e --arg h "$(printf '%s' "$1" | jq -r .head)" \
-    'any(.reviews[]?; .commit.oid == $h)' >/dev/null 2>&1
+    'any((.reviews[]?.commit.oid), (.inline[]?.commit_id); . == $h)' >/dev/null 2>&1
 }
 
 json=$(fetch)
@@ -80,12 +162,12 @@ while [ "$(date +%s)" -lt "$deadline" ]; do
   sleep "$INTERVAL"
   json=$(fetch)
   if has_head_review "$json"; then
-    printf '\npr-reviews: a review against the current head arrived.\n\n'
+    printf '\npr-reviews: a review or inline comment against the current head arrived.\n\n'
     report "$json"
     exit 0
   fi
 done
 
-printf '\npr-reviews: waited %ss; no review against the current head.\n' "$WAIT"
+printf '\npr-reviews: waited %ss; nothing against the current head.\n' "$WAIT"
 printf 'Reviews arrive as the owner quota allows, so absence is not approval.\n'
 exit 1

scripts/pr-reviews-test.sh

@@ -0,0 +1,239 @@
+#!/usr/bin/env bash
+# check_the_checker for scripts/pr-reviews. Fixture-driven, no network.
+# The case that matters is improvise-xgra: a reviewer whose substance is
+# entirely inline. See context/test-evidence/tooling.md.
+set -uo pipefail
+
+here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+REVIEWS="$here/pr-reviews"
+work="$(mktemp -d)"
+trap 'rm -rf "$work"' EXIT
+
+pass=0; fail=0
+ok()  { printf '  ok   %s\n' "$1"; pass=$((pass+1)); }
+bad() { printf '  FAIL %s (%s)\n' "$1" "$2"; fail=$((fail+1)); }
+
+# $1 name, $2 expected exit, $3 fixture dir, rest: extra args. Output goes to
+# $work/out so a case may assert on what was PRINTED as well as on $?; nothing
+# follows the script in its own pipeline, which is the rule its header states.
+run_case() {
+  local name="$1" want="$2" dir="$3"; shift 3
+  rm -f "$dir/.poll"
+  PR_REVIEWS_FIXTURE_DIR="$dir" "$REVIEWS" 1 "$@" > "$work/out" 2>&1
+  local got=$?
+  [ "$got" = "$want" ] && ok "$name" || bad "$name" "exit $got, wanted $want"
+}
+
+# $1 name, $2 fixture dir, $3 grep -E pattern the output must contain
+prints() {
+  local name="$1" dir="$2" pat="$3"
+  rm -f "$dir/.poll"
+  PR_REVIEWS_FIXTURE_DIR="$dir" "$REVIEWS" 1 > "$work/out" 2>&1
+  grep -Eq "$pat" "$work/out" && ok "$name" || bad "$name" "no /$pat/ in output"
+}
+
+mk() { mkdir -p "$1"; cat > "$1/$2"; }
+
+HEAD=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+OLD=bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
+
+# ---------------------------------------------------------------------------
+# The bug this script exists for. The review BODY is a one-liner at an older
+# head; every ruling is an inline comment at the current head. A pr-reviews
+# that reads only .reviews prints the one-liner, reports exit 1, and the
+# rulings are invisible -- which is what happened on PR #122.
+d="$work/inline-at-head"
+mk "$d" last.pr.json <<EOF
+{"headRefOid":"$HEAD","reviews":[
+ {"state":"COMMENTED","submittedAt":"2026-09-22T10:00:00Z","author":{"login":"owner"},
+  "commit":{"oid":"$OLD"},"body":"Generally good, added comments inline."}]}
+EOF
+mk "$d" last.inline.json <<EOF
+[{"created_at":"2026-09-22T10:00:05Z","user":{"login":"owner"},"commit_id":"$HEAD",
+  "path":"crates/improvise-core/src/model/types.rs","line":412,
+  "body":"Use Aggregation::Exact here, not the none_cats list."}]
+EOF
+mk "$d" last.issue.json <<'EOF'
+[]
+EOF
+run_case "an inline comment at the current head satisfies --wait" 0 "$d" --wait 1 --interval 1
+prints "an inline comment's body is printed" "$d" 'Aggregation::Exact'
+prints "an inline comment names its file and line" "$d" 'types\.rs:412'
+prints "an inline comment at the head is marked CURRENT HEAD" "$d" 'types\.rs:412 commit=aaaaaaaa <- CURRENT HEAD'
+prints "a review body at an earlier head still says so" "$d" 'commit=bbbbbbbb <- an EARLIER head'
+
+# ---------------------------------------------------------------------------
+# The mirror-image fault: an inline comment about a tree that no longer
+# exists must NOT satisfy the wait. A version that counts inline comments
+# rather than comparing their commit passes the case above and this one
+# wrongly, so the pair is what discriminates.
+d="$work/inline-at-old"
+mk "$d" last.pr.json <<EOF
+{"headRefOid":"$HEAD","reviews":[]}
+EOF
+mk "$d" last.inline.json <<EOF
+[{"created_at":"2026-09-22T09:00:00Z","user":{"login":"owner"},"commit_id":"$OLD",
+  "path":"src/draw.rs","line":10,"body":"stale finding"}]
+EOF
+mk "$d" last.issue.json <<'EOF'
+[]
+EOF
+run_case "an inline comment at an EARLIER head does not satisfy --wait" 1 "$d" --wait 1 --interval 1
+prints "an earlier-head inline comment is still printed, flagged" "$d" \
+  'draw\.rs:10 commit=bbbbbbbb <- an EARLIER head'
+
+# ---------------------------------------------------------------------------
+# The contract that must not change: a review body at the head still counts.
+d="$work/review-at-head"
+mk "$d" last.pr.json <<EOF
+{"headRefOid":"$HEAD","reviews":[
+ {"state":"APPROVED","submittedAt":"2026-09-22T11:00:00Z","author":{"login":"owner"},
+  "commit":{"oid":"$HEAD"},"body":"LGTM"}]}
+EOF
+mk "$d" last.inline.json <<'EOF'
+[]
+EOF
+mk "$d" last.issue.json <<'EOF'
+[]
+EOF
+run_case "a review body at the current head still satisfies --wait" 0 "$d" --wait 1 --interval 1
+prints "a review prints its state" "$d" '\[REVIEW APPROVED\]'
+
+# ---------------------------------------------------------------------------
+# An issue-level comment is attached to no commit, so it makes no claim about
+# which tree was read and cannot satisfy the wait. Widening --wait to "any
+# text at all" would pass the first case and fail this one.
+d="$work/issue-only"
+mk "$d" last.pr.json <<EOF
+{"headRefOid":"$HEAD","reviews":[]}
+EOF
+mk "$d" last.inline.json <<'EOF'
+[]
+EOF
+mk "$d" last.issue.json <<'EOF'
+[{"created_at":"2026-09-22T12:00:00Z","user":{"login":"owner"},
+  "body":"rebasing this shortly"}]
+EOF
+run_case "an issue-level comment does not satisfy --wait" 1 "$d" --wait 1 --interval 1
+prints "an issue-level comment is still printed" "$d" '\[COMMENT\].*<- not attached to a commit'
+
+# ---------------------------------------------------------------------------
+# Time order across the three surfaces. A concatenation that prints each
+# surface in its own block passes every case above and fails this one.
+d="$work/interleaved"
+mk "$d" last.pr.json <<EOF
+{"headRefOid":"$HEAD","reviews":[
+ {"state":"COMMENTED","submittedAt":"2026-09-22T10:00:00Z","author":{"login":"owner"},
+  "commit":{"oid":"$HEAD"},"body":"SECOND"}]}
+EOF
+mk "$d" last.inline.json <<EOF
+[{"created_at":"2026-09-22T11:00:00Z","user":{"login":"owner"},"commit_id":"$HEAD",
+  "path":"a.rs","line":1,"body":"THIRD"}]
+EOF
+mk "$d" last.issue.json <<'EOF'
+[{"created_at":"2026-09-22T09:00:00Z","user":{"login":"owner"},"body":"FIRST"}]
+EOF
+rm -f "$d/.poll"
+PR_REVIEWS_FIXTURE_DIR="$d" "$REVIEWS" 1 > "$work/out" 2>&1
+order=$(grep -oE 'FIRST|SECOND|THIRD' "$work/out" | tr '\n' ' ')
+[ "$order" = "FIRST SECOND THIRD " ] \
+  && ok "the three surfaces interleave in time order" \
+  || bad "the three surfaces interleave in time order" "got: $order"
+
+# ---------------------------------------------------------------------------
+# An outdated comment has `line: null` and only `original_line` left. A
+# renderer that reads `.line` alone prints "a.rs:null" and loses the location.
+d="$work/outdated-line"
+mk "$d" last.pr.json <<EOF
+{"headRefOid":"$HEAD","reviews":[]}
+EOF
+mk "$d" last.inline.json <<EOF
+[{"created_at":"2026-09-22T10:00:00Z","user":{"login":"owner"},"commit_id":"$HEAD",
+  "path":"src/draw.rs","line":null,"original_line":77,"body":"moved since"}]
+EOF
+mk "$d" last.issue.json <<'EOF'
+[]
+EOF
+prints "an outdated inline comment falls back to original_line" "$d" 'draw\.rs:77'
+
+# ---------------------------------------------------------------------------
+# Nothing on any surface, and the two shapes of that.
+d="$work/empty"
+mk "$d" last.pr.json <<EOF
+{"headRefOid":"$HEAD","reviews":[]}
+EOF
+mk "$d" last.inline.json <<'EOF'
+[]
+EOF
+mk "$d" last.issue.json <<'EOF'
+[]
+EOF
+run_case "--wait 0 over an empty PR is success" 0 "$d"
+run_case "--wait over an empty PR times out" 1 "$d" --wait 1 --interval 1
+prints "an empty PR says so" "$d" 'nothing written on any of the three surfaces'
+
+# ---------------------------------------------------------------------------
+# An inline comment that arrives on a later poll. A script that fetched once
+# and then only re-read the first response would sit here until the deadline.
+d="$work/arrives-later"
+mk "$d" 1.pr.json <<EOF
+{"headRefOid":"$HEAD","reviews":[]}
+EOF
+mk "$d" 1.inline.json <<'EOF'
+[]
+EOF
+mk "$d" 1.issue.json <<'EOF'
+[]
+EOF
+mk "$d" last.pr.json <<EOF
+{"headRefOid":"$HEAD","reviews":[]}
+EOF
+mk "$d" last.inline.json <<EOF
+[{"created_at":"2026-09-22T10:00:00Z","user":{"login":"owner"},"commit_id":"$HEAD",
+  "path":"a.rs","line":3,"body":"arrived on the second poll"}]
+EOF
+mk "$d" last.issue.json <<'EOF'
+[]
+EOF
+run_case "an inline comment arriving mid-wait is seen" 0 "$d" --wait 1 --interval 1
+
+# ---------------------------------------------------------------------------
+# `gh api --paginate` emits one array PER PAGE, so a thread past the page size
+# arrives as several concatenated documents. Passing that straight to
+# --argjson is a jq usage error -- exit 2, no output, every case dead. The
+# fixture carries two concatenated arrays because both paths flatten through
+# the same pages_to_array; a fixture that could only hold one would be testing
+# a code path the live fetch does not take.
+d="$work/paginated"
+mk "$d" last.pr.json <<EOF
+{"headRefOid":"$HEAD","reviews":[]}
+EOF
+mk "$d" last.inline.json <<EOF
+[{"created_at":"2026-09-22T10:00:00Z","user":{"login":"owner"},"commit_id":"$HEAD",
+  "path":"page1.rs","line":1,"body":"FIRSTPAGE"}]
+[{"created_at":"2026-09-22T10:00:01Z","user":{"login":"owner"},"commit_id":"$HEAD",
+  "path":"page2.rs","line":2,"body":"SECONDPAGE"}]
+EOF
+mk "$d" last.issue.json <<'EOF'
+[]
+EOF
+prints "a comment on the first page survives pagination" "$d" 'page1\.rs:1'
+prints "a comment on the second page survives pagination" "$d" 'page2\.rs:2'
+run_case "a paginated thread still satisfies --wait" 0 "$d" --wait 1 --interval 1
+
+# ---------------------------------------------------------------------------
+# Usage is exit 4 and must stay there: briefs distinguish it from 1.
+rm -f "$work/empty/.poll"
+PR_REVIEWS_FIXTURE_DIR="$work/empty" "$REVIEWS" > "$work/out" 2>&1
+got=$?
+[ "$got" = 4 ] && ok "no PR number is exit 4" || bad "no PR number is exit 4" "exit $got"
+
+# The header must keep saying that nothing may follow this script, because the
+# `; echo` half of that trap is the one an agent hit this week.
+"$REVIEWS" --help > "$work/out" 2>&1
+grep -q 'echo "exit=\$?"' "$work/out" \
+  && ok "the usage warns about a trailing \$? reader" \
+  || bad "the usage warns about a trailing \$? reader" "not in --help"
+
+printf '\npr-reviews-test: %d passed, %d failed\n' "$pass" "$fail"
+[ "$fail" -eq 0 ]