diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 34319d67..c7c92df3 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -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. diff --git a/scripts/pr-reviews b/scripts/pr-reviews index 55de7185..298a7687 100755 --- a/scripts/pr-reviews +++ b/scripts/pr-reviews @@ -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 diff --git a/scripts/pr-reviews-test.sh b/scripts/pr-reviews-test.sh index 5050958d..a5294069 100755 --- a/scripts/pr-reviews-test.sh +++ b/scripts/pr-reviews-test.sh @@ -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"