Skip to content

feat: implement issue #2072 — bot-comment retry scan fails with "jq: Argument list too long" on PRs with many comments — undispositioned bot comments are never retried #2073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
don-petry merged 4 commits into from
Oct 9, 2026
Merged

feat: implement issue #2072 — bot-comment retry scan fails with "jq: Argument list too long" on PRs with many comments — undispositioned bot comments are never retried #2073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
42f2e0f
feat: implement issue #2072 — bot-comment retry scan fails with "jq: …
don-petry Oct 7, 2026
ebef04c
Merge branch 'main' into dev-lead/issue-2072-20261007-1738
don-petry Oct 8, 2026
de1ad23
Merge branch 'main' into dev-lead/issue-2072-20261007-1738
don-petry Oct 8, 2026
480b103
fix(dev-lead): reject a non-advancing comment cursor and always clean…
claude Oct 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 53 additions & 14 deletions scripts/lib/bot-comment-retry.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,10 +202,35 @@ bcr_retry_decisions() {

# bcr_fetch_pr_comments <repo> <pr_number>
# Echo every PR issue-comment node (paginated GraphQL) as one JSON array with
# the fields bcr_retry_decisions reads. Returns 1 (echoing nothing) on any API or
# parse failure — a partial list could hide a disposition, so callers fail closed.
# the fields bcr_retry_decisions reads. Returns 1 (echoing nothing on stdout, one
# `[warn]` line naming the failure on stderr) on any API or parse failure — a
# partial list could hide a disposition, so callers fail closed.
# Pages are merged from a temp file, never passed to jq as arguments: a PR's
# accumulated comment JSON outgrows the exec argument limit (#2072, PR #1953).
bcr_fetch_pr_comments() {
local repo="$1" pr="$2"
local pages_file rc
pages_file=$(mktemp "${TMPDIR:-/tmp}/bcr-comments.XXXXXX") || {
echo " [warn] bot-comment retry: could not create a temp file for PR $2 comments" >&2
return 1
}
# A subshell with its own EXIT trap removes the temp file (it holds fetched
# comment bodies) even on interruption, without touching the caller's traps;
# `|| rc=$?` keeps errexit from skipping the status capture.
rc=0
(
trap 'rm -f "$pages_file"' EXIT
_bcr_fetch_pr_comments_into "$1" "$2" "$pages_file"
) || rc=$?
rm -f "$pages_file"
return "$rc"
}

# _bcr_fetch_pr_comments_into <repo> <pr_number> <pages_file>
# bcr_fetch_pr_comments' body: appends each page's nodes array to <pages_file>
# (one per line) and echoes their concatenation once every page has been read.
_bcr_fetch_pr_comments_into() {
local repo="$1" pr="$2" pages_file="$3"
local warn=" [warn] bot-comment retry: PR ${pr} comments in ${repo}:"
# shellcheck disable=SC2016 # $owner/$repo/$pr/$cursor are GraphQL variables
local query='query($owner:String!,$repo:String!,$pr:Int!,$cursor:String){
repository(owner:$owner,name:$repo){
Expand All @@ -217,27 +242,41 @@ bcr_fetch_pr_comments() {
}
}
}'
local all='[]' cursor="" has_next="true" page nodes
local cursor="" prev_cursor="" has_next="true" page
local cursor_args=()
while [ "$has_next" = "true" ]; do
page=$(gh api graphql -f query="$query" -F owner="${repo%%/*}" -F repo="${repo##*/}" \
-F pr="$pr" "${cursor_args[@]}" 2>/dev/null) || return 1
-F pr="$pr" "${cursor_args[@]}" 2>/dev/null) || {
echo "${warn} API request failed" >&2; return 1; }
# A partial response (GraphQL errors alongside data) or an unreadable page
# cursor is a partial snapshot — it could hide a disposition. Fail closed.
printf '%s' "$page" | jq -e '(.errors // []) | length == 0' >/dev/null 2>&1 || return 1
nodes=$(printf '%s' "$page" | jq -ce '.data.repository.pullRequest.comments.nodes | arrays' 2>/dev/null) \
|| return 1
all=$(jq -cn --argjson a "$all" --argjson b "$nodes" '$a + $b') || return 1
printf '%s' "$page" | jq -e 'type == "object"' >/dev/null 2>&1 || {
echo "${warn} unparseable API response" >&2; return 1; }
printf '%s' "$page" | jq -e '(.errors // []) | length == 0' >/dev/null 2>&1 || {
echo "${warn} response carried GraphQL errors" >&2; return 1; }
printf '%s' "$page" | jq -ce '.data.repository.pullRequest.comments.nodes | arrays' \
>> "$pages_file" 2>/dev/null || {
Comment thread
don-petry marked this conversation as resolved.
echo "${warn} unparseable comment page (no nodes array)" >&2; return 1; }
has_next=$(printf '%s' "$page" | jq -r '.data.repository.pullRequest.comments.pageInfo.hasNextPage
| if type == "boolean" then tostring else "invalid" end' 2>/dev/null) || return 1
case "$has_next" in true|false) ;; *) return 1 ;; esac
cursor=$(printf '%s' "$page" | jq -r '.data.repository.pullRequest.comments.pageInfo.endCursor // ""')
| if type == "boolean" then tostring else "invalid" end' 2>/dev/null) || has_next="invalid"
cursor=$(printf '%s' "$page" | jq -r '.data.repository.pullRequest.comments.pageInfo.endCursor // ""' 2>/dev/null) \
|| cursor=""
Comment thread
don-petry marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
case "$has_next" in true|false) ;; *)
echo "${warn} invalid pagination info (hasNextPage)" >&2; return 1 ;;
esac
if [ "$has_next" = "true" ] && [ -z "$cursor" ]; then
return 1
echo "${warn} invalid pagination info (no endCursor)" >&2; return 1
fi
# A cursor that did not advance would re-fetch the same page forever.
if [ "$has_next" = "true" ] && [ "$cursor" = "$prev_cursor" ]; then
echo "${warn} invalid pagination info (endCursor did not advance)" >&2; return 1
fi
prev_cursor="$cursor"
cursor_args=(-f "cursor=${cursor}")
done
printf '%s\n' "$all"
# Concatenate the pages from the file — never via jq arguments (#2072).
jq -cs 'add // []' "$pages_file" 2>/dev/null || {
echo "${warn} could not merge comment pages" >&2; return 1; }
}

# bcr_retry_marker <comment_id> <version> <attempt> <now_iso>
Expand Down
100 changes: 95 additions & 5 deletions tests/dev-lead/unit/test_bot_comment_retry.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
# • the sweep wiring in dev-lead-retry.sh (exactly one dispatch, dedup marker,
# payload carries the comment node id — never its body).

bats_require_minimum_version 1.5.0

SCRIPT_DIR="$(cd "$(dirname "$BATS_TEST_FILENAME")/../../.." && pwd)"
LIB="$SCRIPT_DIR/scripts/lib/bot-comment-retry.sh"
RETRY_SCRIPT="$SCRIPT_DIR/scripts/dev-lead-retry.sh"
Expand Down Expand Up @@ -304,21 +306,109 @@ $(_ours '<!-- dev-lead-bot-comment-retry id=IC_cr version=2026-10-01T23:05:43Z a

@test "bcr_fetch_pr_comments: a GraphQL response carrying errors fails closed" {
gh() { printf '%s' '{"errors":[{"message":"rate limited"}],"data":{"repository":{"pullRequest":{"comments":{"pageInfo":{"hasNextPage":false,"endCursor":null},"nodes":[]}}}}}'; }
run bcr_fetch_pr_comments "petry-projects/.github-private" 2009
[ "$status" -ne 0 ]
run --separate-stderr bcr_fetch_pr_comments "petry-projects/.github-private" 2009
[ "$status" -eq 1 ]
[ -z "$output" ]
[[ "$stderr" == *"GraphQL errors"* ]]
}

@test "bcr_fetch_pr_comments: hasNextPage true without a cursor fails closed" {
gh() { printf '%s' '{"data":{"repository":{"pullRequest":{"comments":{"pageInfo":{"hasNextPage":true,"endCursor":null},"nodes":[]}}}}}'; }
run bcr_fetch_pr_comments "petry-projects/.github-private" 2009
[ "$status" -ne 0 ]
run --separate-stderr bcr_fetch_pr_comments "petry-projects/.github-private" 2009
[ "$status" -eq 1 ]
[ -z "$output" ]
[[ "$stderr" == *"pagination"* ]]
}

@test "bcr_fetch_pr_comments: a non-boolean hasNextPage fails closed" {
gh() { printf '%s' '{"data":{"repository":{"pullRequest":{"comments":{"pageInfo":{"endCursor":null},"nodes":[]}}}}}'; }
run --separate-stderr bcr_fetch_pr_comments "petry-projects/.github-private" 2009
[ "$status" -eq 1 ]
[ -z "$output" ]
[[ "$stderr" == *"pagination"* ]]
}

# #2072: a failed API call and an unparseable page each fail closed with their
# own reason, so a fetch failure is distinguishable from a parse failure.
@test "bcr_fetch_pr_comments: a failed API call fails closed with a fetch reason" {
gh() { return 1; }
run --separate-stderr bcr_fetch_pr_comments "petry-projects/.github-private" 2009
[ "$status" -eq 1 ]
[ -z "$output" ]
[[ "$stderr" == *"API request failed"* ]]
}

@test "bcr_fetch_pr_comments: an unparseable page fails closed with a parse reason" {
gh() { printf '%s' '<html>502 Bad Gateway</html>'; }
run --separate-stderr bcr_fetch_pr_comments "petry-projects/.github-private" 2009
[ "$status" -eq 1 ]
[ -z "$output" ]
[[ "$stderr" == *"unparseable"* ]]
[[ "$stderr" != *"API request failed"* ]]
}

# A page that reports more pages but repeats the cursor it was fetched with
# would loop on the same page forever; it fails closed instead.
@test "bcr_fetch_pr_comments: a repeated non-empty endCursor fails closed" {
gh() { printf '%s' '{"data":{"repository":{"pullRequest":{"comments":{"pageInfo":{"hasNextPage":true,"endCursor":"same"},"nodes":[]}}}}}'; }
run --separate-stderr bcr_fetch_pr_comments "petry-projects/.github-private" 2009
[ "$status" -eq 1 ]
[ -z "$output" ]
[[ "$stderr" == *"did not advance"* ]]
}

# The temp file holds fetched comment bodies; it is removed on success and failure.
@test "bcr_fetch_pr_comments: leaves no temp file behind" {
export TMPDIR="$BATS_TEST_TMPDIR/tmp"
mkdir -p "$TMPDIR"
gh() { printf '%s' '{"data":{"repository":{"pullRequest":{"comments":{"pageInfo":{"hasNextPage":false,"endCursor":null},"nodes":[]}}}}}'; }
run bcr_fetch_pr_comments "petry-projects/.github-private" 2009
[ "$status" -ne 0 ]
[ "$status" -eq 0 ]
gh() { return 1; }
run bcr_fetch_pr_comments "petry-projects/.github-private" 2009
[ "$status" -eq 1 ]
[ -z "$(ls -A "$TMPDIR")" ]
}

# #2072: PR #1953's comment history grew past the per-argument size limit and the
# scan died with "jq: Argument list too long" because pages were merged through
# `jq --argjson`. Thousands of comments and several MB of body text — far beyond
# ARG_MAX — must be read in full.
@test "bcr_fetch_pr_comments: reads thousands of comments and MBs of text (well over ARG_MAX)" {
local pages_dir="$BATS_TEST_TMPDIR/pages" pages=30 per_page=100 i
mkdir -p "$pages_dir"
for ((i = 1; i <= pages; i++)); do
jq -nc --argjson p "$i" --argjson n "$per_page" --argjson last "$pages" '
("x" * 2048) as $pad
| {data: {repository: {pullRequest: {comments: {
pageInfo: {hasNextPage: ($p < $last),
endCursor: (if $p < $last then "c\($p + 1)" else null end)},
nodes: [range($n) as $k
| {id: "IC_\($p)_\($k)", author: {login: "coderabbitai", __typename: "Bot"},
authorAssociation: "NONE", body: "comment \($p)/\($k) \($pad)",
createdAt: "2026-10-01T23:05:43Z", lastEditedAt: null,
isMinimized: false, minimizedReason: null}]}}}}}' > "$pages_dir/c$i.json"
done
export PAGES_DIR="$pages_dir"
gh() {
local a cursor="c1"
for a in "$@"; do
case "$a" in cursor=*) cursor="${a#cursor=}" ;; esac
done
cat "$PAGES_DIR/$cursor.json"
}
local out_file="$BATS_TEST_TMPDIR/out.json" err_file="$BATS_TEST_TMPDIR/err.txt"
bcr_fetch_pr_comments "petry-projects/.github-private" 1953 > "$out_file" 2> "$err_file"
[ ! -s "$err_file" ]
# Well over ARG_MAX, so it could never have been passed as one argument.
[ "$(wc -c < "$out_file")" -gt "$(getconf ARG_MAX)" ]
[ "$(jq 'length' "$out_file")" -eq $((pages * per_page)) ]
[ "$(jq -r 'first.id' "$out_file")" = "IC_1_0" ]
[ "$(jq -r 'last.id' "$out_file")" = "IC_${pages}_$((per_page - 1))" ]
# The full set still drives the pure decision without hitting the size limit.
run bcr_retry_decisions "$(cat "$out_file")" "$TRUSTED" "$INFO" "$NOW_EPOCH" "$AUTOMATION"
[ "$status" -eq 0 ]
[ "$(jq 'length' <<< "$output")" -eq $((pages * per_page)) ]
}

# ── dev-lead-retry.sh sweep wiring ───────────────────────────────────────────
Expand Down
Loading