Edward Langley
c6b53eaa77
On #210 the review and comment JSON outgrew ARG_MAX, and `jq -n --argjson ...` failed to exec with exit 126 before printing anything. A `combine` helper now pipes the three documents to `jq -s` through the printf builtin, on both the fixture path and the live path. The new suite cases put about 2 MB on each surface, so a fix that moves only one document off argv still fails. Both rivals fail 5 of 30: the pre-fix script, and a partial fix that keeps the issue comments on argv. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
336 lines
14 KiB
Bash
Executable File
336 lines
14 KiB
Bash
Executable File
#!/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)"
|
|
work="$(mktemp -d)"
|
|
trap 'rm -rf "$work"' EXIT
|
|
|
|
# Every run is bounded, so a loop that never stops FAILS (exit 124) rather than
|
|
# hanging the suite. The real sleep, resolved before any case stubs PATH; NixOS has no /bin/sleep.
|
|
BOUND=20
|
|
REAL_SLEEP="$(type -P sleep)"
|
|
bounded_reviews() {
|
|
"$here/pr-reviews" "$@" &
|
|
local pid=$! dog done rc
|
|
"$REAL_SLEEP" "$BOUND" &
|
|
dog=$!
|
|
wait -n -p done "$pid" "$dog"; rc=$?
|
|
if [ "$done" = "$pid" ]; then kill "$dog" 2>/dev/null; wait "$dog" 2>/dev/null; return "$rc"; fi
|
|
kill -TERM "$pid" 2>/dev/null; wait "$pid" 2>/dev/null
|
|
echo "pr-reviews-test: killed after ${BOUND}s; the --wait loop never stopped" >&2
|
|
return 124
|
|
}
|
|
REVIEWS=bounded_reviews
|
|
|
|
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
|
|
|
|
# The same fixture on a clock that ticks one second per read: a loaded machine,
|
|
# compressed. A second passing between two reads must not cost the only poll.
|
|
clock="$work/clock"
|
|
mkdir -p "$clock"
|
|
cat > "$clock/date" <<'EOF'
|
|
#!/usr/bin/env bash
|
|
[ "$*" = "+%s" ] || { echo "fake date: only +%s is faked, got: $*" >&2; exit 2; }
|
|
n=$(( $(cat "$CLOCK_TICKS" 2>/dev/null || echo 0) + 1 ))
|
|
echo "$n" > "$CLOCK_TICKS"
|
|
echo $(( 1000 + n ))
|
|
EOF
|
|
printf '#!/usr/bin/env bash\nexit 0\n' > "$clock/sleep"
|
|
chmod +x "$clock/date" "$clock/sleep"
|
|
rm -f "$d/.poll" "$work/ticks"
|
|
CLOCK_TICKS="$work/ticks" PATH="$clock:$PATH" PR_REVIEWS_FIXTURE_DIR="$d" \
|
|
"$REVIEWS" 1 --wait 1 --interval 1 > "$work/out" 2>&1
|
|
got=$?
|
|
[ "$got" = 0 ] && ok "a second passing before the first re-poll does not skip it" \
|
|
|| bad "a second passing before the first re-poll does not skip it" "exit $got, wanted 0"
|
|
|
|
# Every fixture above resolves by poll 2, so a loop that stops after one
|
|
# re-poll passed them all (improvise-7epn). This one resolves on poll 3.
|
|
d3="$work/arrives-third"
|
|
for n in 1 2; do
|
|
mk "$d3" "$n.pr.json" <<EOF
|
|
{"headRefOid":"$HEAD","reviews":[]}
|
|
EOF
|
|
mk "$d3" "$n.inline.json" <<'EOF'
|
|
[]
|
|
EOF
|
|
mk "$d3" "$n.issue.json" <<'EOF'
|
|
[]
|
|
EOF
|
|
done
|
|
cp -f "$d/last.pr.json" "$d/last.inline.json" "$d/last.issue.json" "$d3/"
|
|
rm -f "$d3/.poll" "$work/ticks"
|
|
CLOCK_TICKS="$work/ticks" PATH="$clock:$PATH" PR_REVIEWS_FIXTURE_DIR="$d3" \
|
|
"$REVIEWS" 1 --wait 10 --interval 1 > "$work/out" 2>&1
|
|
got=$?
|
|
polls=$(cat "$d3/.poll" 2>/dev/null || echo 0)
|
|
[ "$got" = 0 ] && [ "$polls" = 3 ] && ok "a review arriving on the third poll is seen" \
|
|
|| bad "a review arriving on the third poll is seen" "exit $got after $polls polls, wanted 0 after 3"
|
|
|
|
# The timeout path: nothing arrives, and on a clock that ticks once per read the
|
|
# count is exact: 1 poll, then one per tick until the deadline, so WAIT + 1.
|
|
WAIT=5
|
|
rm -f "$work/empty/.poll" "$work/ticks"
|
|
CLOCK_TICKS="$work/ticks" PATH="$clock:$PATH" PR_REVIEWS_FIXTURE_DIR="$work/empty" \
|
|
"$REVIEWS" 1 --wait "$WAIT" --interval 1 > "$work/out" 2>&1
|
|
got=$?
|
|
polls=$(cat "$work/empty/.poll" 2>/dev/null || echo 0)
|
|
[ "$got" = 1 ] && [ "$polls" = $((WAIT + 1)) ] && ok "--wait keeps polling until its deadline" \
|
|
|| bad "--wait keeps polling until its deadline" "exit $got after $polls polls, wanted 1 after $((WAIT + 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
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# A thread past ARG_MAX (improvise-5jxt). PR #210's comments outgrew the argv
|
|
# limit, and a fetch that hands the JSON to jq as an argument dies with exit
|
|
# 126 before printing anything. 2MB per surface clears every platform's limit,
|
|
# and EACH surface is large, so a fix that moves only one of them off argv fails.
|
|
d="$work/huge"
|
|
mkdir -p "$d"
|
|
big=$(head -c 2000000 /dev/zero | tr '\0' x)
|
|
{ printf '{"headRefOid":"%s","reviews":[{"state":"COMMENTED",' "$HEAD"
|
|
printf '"submittedAt":"2026-09-22T09:00:00Z","author":{"login":"owner"},'
|
|
printf '"commit":{"oid":"%s"},"body":"BIGREVIEW %s"}]}\n' "$OLD" "$big"
|
|
} > "$d/last.pr.json"
|
|
{ printf '[{"created_at":"2026-09-22T10:00:00Z","user":{"login":"owner"},"commit_id":"%s",' "$HEAD"
|
|
printf '"path":"big.rs","line":1,"body":"%s"},\n' "$big"
|
|
printf '{"created_at":"2026-09-22T10:00:01Z","user":{"login":"owner"},"commit_id":"%s",' "$HEAD"
|
|
printf '"path":"after.rs","line":9,"body":"AFTERTHEBIGONE"}]\n'
|
|
} > "$d/last.inline.json"
|
|
printf '[{"created_at":"2026-09-22T11:00:00Z","user":{"login":"owner"},"body":"BIGISSUE %s"}]\n' \
|
|
"$big" > "$d/last.issue.json"
|
|
unset big
|
|
run_case "a thread larger than ARG_MAX is still read" 0 "$d"
|
|
prints "a comment after a multi-MB one is printed" "$d" 'after\.rs:9 commit=aaaaaaaa <- CURRENT HEAD'
|
|
prints "a multi-MB review body is printed" "$d" 'BIGREVIEW x'
|
|
prints "a multi-MB issue comment is printed" "$d" 'BIGISSUE x'
|
|
run_case "a thread larger than ARG_MAX 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 ]
|