fix(scripts): pr-reviews reads all three review surfaces (improvise-xgra)
It printed review BODIES only and was blind to inline review comments -- the surface a human reviewer is most likely to use. On PR #122 the user reviewed with the body "Generally good, added comments inline." and five substantive rulings underneath it; pr-reviews printed the one-liner, and an agent reported the PR as awaiting review while the answers sat on it. pulls/{n}/comments and issues/{n}/comments join pulls/{n}/reviews, all three interleaved in time order, each inline comment carrying its path and line (falling back to original_line, which is what an outdated comment has left). The exit contract is unchanged: 0 means something exists against the current head, 1 means the wait elapsed, 4 is usage. An inline comment at the head now satisfies --wait -- it carries a commit_id and is exactly the case that failed. An issue-level comment does not: it names no commit and so makes no claim about which tree was read. The usage's exit-code warning is widened from "do not pipe into head/tail" to the real rule: nothing may follow the script in the same `;` list either, because a trailing `echo "exit=$?"` makes the status the echo's. scripts/pr-reviews-test.sh is the check_the_checker, fixture-driven and needing no network, wired into CI's kb job beside pr-watch-test.sh and release-preflight-test.sh. Five faults were injected and watched to fail; context/test-evidence/tooling.md records which case caught each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
Executable
+215
@@ -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 <<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
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# 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 ]
|
||||
Reference in New Issue
Block a user