diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 66764c96..321ada37 100644 --- a/.github/workflows/ci.yml +++ b/.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. diff --git a/context/code/pr-review.md b/context/code/pr-review.md index d91e2180..4a410beb 100644 --- a/context/code/pr-review.md +++ b/context/code/pr-review.md @@ -9,6 +9,61 @@ PR, every one a P1. `scripts/pr-reviews ` prints them. `--wait ` 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. + +## `$?` 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. + ## Why a review is easy to miss here It arrives as a **review**, not a comment, so reading the comment thread does diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 223bdddd..efa2c4af 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -219,3 +219,50 @@ 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. diff --git a/scripts/pr-reviews b/scripts/pr-reviews index 02558fc9..4e99aa4e 100755 --- a/scripts/pr-reviews +++ b/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. -# Exists because twenty PRs merged with their reviews unread; see -# context/code/pr-review.md. +# Prints everything a reviewer may have written on a PR -- review bodies, +# INLINE review comments and issue-level comments -- and optionally waits for +# one against the current head. See context/code/pr-review.md. set -euo pipefail usage() { cat >&2 <<'EOF' usage: pr-reviews [--wait ] [--interval ] - --wait wait up to this long for a review whose commit is the - current head (default 0 = print what exists and exit) + --wait wait up to this long for a review or an inline review + comment whose commit is the current head + (default 0 = print what exists and exit) --interval seconds between polls (default 30) -exit 0 a review against the current head exists (or --wait was 0) -exit 1 --wait elapsed with no review against the current head +exit 0 something against the current head exists (or --wait was 0) +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 -script's, and a --wait that timed out reads as success. That trap has cost -this repo four separate diagnoses. Redirect to a file and read it instead. +THIS SCRIPT'S `$?` MUST BE THE ONE YOU READ. Nothing may follow it in the same +pipeline OR the same `;` list: `pr-reviews 1 --wait 60 | tail` gives you tail's +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 -than the count: a finding against an old commit may be fixed or still live, and -only reading it tells you which. +Three surfaces, because a reviewer writes on whichever one is nearest: + [REVIEW] the review's own body (pulls/{n}/reviews) + [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,82 @@ 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" +} + +fixture_file() { + local n="$1" name="$2" f="$PR_REVIEWS_FIXTURE_DIR/$n.$name.json" + [ -f "$f" ] || f="$PR_REVIEWS_FIXTURE_DIR/last.$name.json" + [ -f "$f" ] && cat "$f" || echo 'null' +} + +fetch() { + if [ -n "${PR_REVIEWS_FIXTURE_DIR:-}" ]; then + local n; n=$(fixture_poll) + jq -n \ + --argjson pr "$(fixture_file "$n" pr)" \ + --argjson inline "$(fixture_file "$n" inline)" \ + --argjson issue "$(fixture_file "$n" issue)" \ + '{head: ($pr.headRefOid // ""), reviews: ($pr.reviews // []), + inline: ($inline // []), issue: ($issue // [])}' + return + fi + local repo pr inline issue + repo=$(gh_ repo view --json nameWithOwner -q .nameWithOwner) + pr=$(gh_ pr view "$PR" --json headRefOid,reviews) + inline=$(gh_ api "repos/$repo/pulls/$PR/comments" --paginate) + issue=$(gh_ api "repos/$repo/issues/$PR/comments" --paginate) + 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" ' - if (.reviews | length) == 0 then " (no reviews)" - else .reviews[] | - " [\(.state)] \(.submittedAt) commit=\(.commit.oid[0:8])" + - (if .commit.oid == $head then " <- CURRENT HEAD" else " <- an EARLIER head; verify whether it still applies" end) + - "\n" + (.body | split("\n") | map(" " + .) | join("\n")) - end' + printf '%s' "$json" | jq -r --arg head "$head" "$normalize"' + | if length == 0 then " (nothing written on any of the three surfaces)" + else .[] | + " [\(.kind)\(if .state == "" then "" else " " + .state end)] \(.at) @\(.who)" + + (if .at_line == "" then "" else " " + .at_line end) + + (if .commit == "" then " <- not attached to a commit" + 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)" \ - 'any(.reviews[]?; .commit.oid == $h)' >/dev/null 2>&1 + printf '%s' "$1" | jq -e --arg h "$(printf '%s' "$1" | jq -r .head)" \ + 'any((.reviews[]?.commit.oid), (.inline[]?.commit_id); . == $h)' >/dev/null 2>&1 } json=$(fetch) @@ -80,12 +153,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 diff --git a/scripts/pr-reviews-test.sh b/scripts/pr-reviews-test.sh new file mode 100755 index 00000000..f6e1516f --- /dev/null +++ b/scripts/pr-reviews-test.sh @@ -0,0 +1,215 @@ +#!/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 < "$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 < "$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 ]