Edward Langley
051f54111b
Both found by ChatGPT Codex on PR #133, both invisible on macOS, both arriving as exit 2 with no output -- jq's answer to an invalid --argjson, so a shell fault at the top of the script wears jq's exit code. `local n="$1" name="$2" f="$DIR/$n.$name.json"` creates every name before running the assignments on bash 5, so $n is unbound in the third clause and set -u kills the shell. The dead command substitution then hands --argjson an empty string. Works on macOS bash 3.2: the suite passed 19/19 locally and 2/19 in CI on the same commit. Each local now gets its own statement. `gh api --paginate` emits one JSON array PER PAGE, so a comment thread past the page size is several concatenated documents that --argjson rejects the same way. Both fetch paths now flatten through one pages_to_array (`jq -s 'add // []'`), which is a no-op on a single page. The fixture path shares that helper, which is the only reason a fixture can test it: a fixture holding a single array would exercise a shape the live fetch never produces. Three cases added, and removing the helper from the fixture path fails exactly those three, the wait case with the live exit-2 signature. Suite is 22 cases. No PR in this repo has yet had enough inline comments to reach a second page, so nothing but the test will see that one before a user does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
131 lines
6.8 KiB
Markdown
131 lines
6.8 KiB
Markdown
# Reading a PR's reviews
|
|
|
|
Twenty PRs merged in one campaign with their reviews unread. CI was green and
|
|
an adversarial agent had been dispatched, which felt like the whole gate. Two
|
|
ChatGPT Codex reviews were found afterwards on merged PRs; both were correct
|
|
and one was a live P2. Three more were found on the next three PRs — one per
|
|
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
|
|
not show it.
|
|
|
|
It posts under the repository **owner's** account — the same account this
|
|
repo's own agents post under — so **authorship does not distinguish it**. The
|
|
tell is placement: a Codex review *ends* with
|
|
`_Automated review by ChatGPT Codex (…)_`, where an agent's comment *opens*
|
|
with `_Posted by Claude (…)_`.
|
|
|
|
It may target an **earlier head**. `pr-reviews` says which, because that
|
|
changes what you do: a finding against an old commit may already be fixed or
|
|
may still be live, and only checking tells you which. Both cases have occurred
|
|
— one fixed by a later round, one live at the head after three more commits.
|
|
|
|
## Absence is not approval
|
|
|
|
Reviews arrive as the owner's quota allows. Observed latency is ten to fifteen
|
|
minutes after a push. `--wait` exists so that a merge four minutes after a
|
|
push does not race the review that was about to arrive; when it times out,
|
|
that is a decision to merge unreviewed rather than evidence of a clean bill.
|
|
|
|
## Answer them on the thread
|
|
|
|
Including after the PR has merged. That is where the next reader looks, and a
|
|
finding that was declined or already-fixed is worth as much as one that was
|
|
taken — the reasoning is the artefact, not the verdict.
|