pr-reviews: hand jq its JSON on stdin, not argv (improvise-5jxt)
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
This commit is contained in:
Edward Langley
@@ -1731,3 +1731,44 @@ already requires the dev shell for `tpl`, and the dev shell's `stat` is GNU.
|
||||
From `ci.yml`, the toolchain version probes ("the dev shell supplies the pinned
|
||||
web toolchain", "Playwright matches the dev shell's browsers") and every
|
||||
`shell: bash` override, so every step runs in `nix develop .#ci`.
|
||||
|
||||
### A thread past ARG_MAX is read from stdin — `scripts/pr-reviews-test.sh` (improvise-5jxt)
|
||||
|
||||
**What broke.** `fetch` handed the review, inline-comment and issue-comment
|
||||
JSON to one `jq -n --argjson …` call as arguments. Once #210's comments
|
||||
outgrew the kernel's argument limit, `execve` refused to start jq:
|
||||
`jq: Argument list too long`, exit 126, with nothing printed. That happened
|
||||
with or without `--wait`. The script is how agents read reviews, so an ADR
|
||||
steward was blocked on it.
|
||||
|
||||
**Fix.** A `combine` helper sends the three documents to `jq -s` on stdin
|
||||
through `printf`. `printf` is a bash builtin, so no `execve` sees the data.
|
||||
Both paths use `combine`: the fixture path and the live `gh` path. The other
|
||||
jq calls in `pr-reviews` already read `$json` from a pipe. `--arg head` carries
|
||||
only a 40-character SHA.
|
||||
|
||||
**The fixture.** Each of the three surfaces carries about 2 MB: a review body,
|
||||
an inline comment followed by a small one after it, and an issue comment. 2 MB
|
||||
exceeds both macOS's total `ARG_MAX` (1 MB) and Linux's per-argument
|
||||
`MAX_ARG_STRLEN` (128 KB). Every surface is large, so a fix that moves only one
|
||||
document off argv still fails. The file is built with `head -c`, `tr` and
|
||||
`printf`, so it depends on no tool version and no path.
|
||||
|
||||
**Red first.** Before the fix, "a thread larger than ARG_MAX is still read" and
|
||||
"… still satisfies --wait" exited 126, as #210 did live.
|
||||
|
||||
**Rivals** (each 25 passed, 5 failed; the five are the new cases):
|
||||
- **The pre-fix script**, restored from `gh/main`: exit 126 on both run cases,
|
||||
and all three print assertions miss.
|
||||
- **A partial fix** that pipes the review and inline documents but still
|
||||
passes the issue comments with `--argjson issue "$3"`: also exit 126. So the
|
||||
large issue fixture is load-bearing, not decoration.
|
||||
|
||||
**Live.** `./scripts/pr-reviews 210` exits 0 with its output sent to a file.
|
||||
The output includes the user's inline answers on
|
||||
`context/adr/007-stable-identities.md` from 2026-09-24T19:50:22Z onward
|
||||
("full unicode", "Just a plane number.", …).
|
||||
|
||||
**Scope sweep.** `scripts/pr-watch` passes its JSON through `printf '%s' | jq`,
|
||||
never as an argument. `grep -rn 'argjson\|--arg '` over `scripts/`,
|
||||
`.agents/` and `.claude/skills/` finds no other data argument.
|
||||
|
||||
+12
-8
@@ -87,28 +87,32 @@ fixture_file() {
|
||||
}
|
||||
|
||||
# `gh api --paginate` emits one JSON array PER PAGE, so a comment thread past
|
||||
# the page size arrives as several concatenated documents that --argjson
|
||||
# rejects. Both paths flatten through here, so a fixture can carry two pages.
|
||||
# the page size arrives as several concatenated documents that combine
|
||||
# would misnumber. Both paths flatten through here, so a fixture can carry two pages.
|
||||
pages_to_array() { jq -s 'add // []'; }
|
||||
|
||||
# The three documents reach jq on STDIN, never as --argjson: a thread past
|
||||
# ARG_MAX is exit 126 before jq runs (improvise-5jxt). printf is a builtin.
|
||||
combine() {
|
||||
printf '%s\n%s\n%s\n' "$1" "$2" "$3" | jq -s \
|
||||
'{head: (.[0].headRefOid // ""), reviews: (.[0].reviews // []),
|
||||
inline: .[1], issue: .[2]}'
|
||||
}
|
||||
|
||||
fetch() {
|
||||
if [ -n "${PR_REVIEWS_FIXTURE_DIR:-}" ]; then
|
||||
local n; n=$(fixture_poll)
|
||||
local pr; pr=$(fixture_file "$n" pr)
|
||||
local inline; inline=$(fixture_file "$n" inline | pages_to_array)
|
||||
local issue; issue=$(fixture_file "$n" issue | pages_to_array)
|
||||
jq -n --argjson pr "$pr" --argjson inline "$inline" --argjson issue "$issue" \
|
||||
'{head: ($pr.headRefOid // ""), reviews: ($pr.reviews // []),
|
||||
inline: $inline, issue: $issue}'
|
||||
combine "$pr" "$inline" "$issue"
|
||||
return
|
||||
fi
|
||||
local repo; repo=$(gh_ repo view --json nameWithOwner -q .nameWithOwner)
|
||||
local pr; pr=$(gh_ pr view "$PR" --json headRefOid,reviews)
|
||||
local inline; inline=$(gh_ api "repos/$repo/pulls/$PR/comments" --paginate | pages_to_array)
|
||||
local issue; issue=$(gh_ api "repos/$repo/issues/$PR/comments" --paginate | pages_to_array)
|
||||
jq -n --argjson pr "$pr" --argjson inline "$inline" --argjson issue "$issue" \
|
||||
'{head: ($pr.headRefOid // ""), reviews: ($pr.reviews // []),
|
||||
inline: $inline, issue: $issue}'
|
||||
combine "$pr" "$inline" "$issue"
|
||||
}
|
||||
|
||||
# `line` is null once the line has moved under the comment; `original_line` is
|
||||
|
||||
@@ -291,6 +291,32 @@ 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"
|
||||
|
||||
Reference in New Issue
Block a user