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
6.8 KiB
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.