docs(#2091): document gh api --paginate + jq per-page semantics - #2288
Conversation
|
🤖 Review · |
1b6087a to
63c9907
Compare
Site previewPreview: https://28db6bfa-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🤖 Finished Review · ✅ Success · Started 7:18 AM UTC · Completed 7:29 AM UTC |
ReviewFindingsMedium
Low
Info
Previous runReviewFindingsMedium
Low
Info
Previous runReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Info
Previous run (2)ReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| **Do not** use aggregating jq filters directly in `--jq` with `--paginate`: | ||
|
|
||
| ```bash | ||
| # WRONG — `length` runs per-page; produces one number per page, not a total |
There was a problem hiding this comment.
[low] code-example-style
Code comment capitalization is inconsistent: # WRONG uses all caps while # Correct uses title case.
Suggested fix: Change # Correct to # CORRECT to match the all-caps style, or use title case for both.
fullsend-ai-review
Bot
added
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review (4 agents: claude-coder, claude-researcher, gemini-code-review, cursor-code-review). Core guidance is technically accurate — two MEDIUM findings on explanation accuracy and completeness. See inline comments.
63c9907 to
3777cd7
Compare
|
🤖 Finished Review · ✅ Success · Started 6:41 AM UTC · Completed 6:53 AM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Multi-agent review (5 agents: 2x claude-coder, claude-researcher, gemini-code-review, cursor-code-review). Prior MEDIUM findings resolved. One new MEDIUM on error handling — see inline.
| | jq -s "add | [.[] | select(.body | contains(\"${REDISPATCH_MARKER}\")) | ||
| | select(.created_at > (now - 300 | strftime(\"%Y-%m-%dT%H:%M:%SZ\")))] | ||
| | length" 2>/dev/null || echo "0") | ||
| | length" || echo "0") |
There was a problem hiding this comment.
[MEDIUM] Fallback value corruption under pipefail
This script runs with set -euo pipefail. The || echo "0" fallback is inside the $(...) command substitution, so if gh api writes partial data then exits non-zero:
jqprocesses the partial data and writes its result (e.g.,0) to stdout- Pipeline exits non-zero due to
pipefail || echo "0"triggers, appending a second0to stdoutRECENT_REDISPATCHcaptures both lines:"0\n0"[ "${RECENT_REDISPATCH}" -gt 0 ]fails withinteger expression expected
The else branch runs (redispatch happens) — fail-open and functionally correct, but produces noisy stderr. The reusable-fix.yml version avoids this because its fallback reassigns the variable (FIX_COMMITS="${ITERATION_CAP:-5}").
Reproduced locally:
$ bash -c 'set -o pipefail; X=$( (echo "3"; exit 1) | cat || echo "0"); echo "X=[$X]"'
X=[3
0]Suggested fix: Move the fallback outside the command substitution and add add // [] to guard against null on empty input:
RECENT_REDISPATCH=$(gh api \
"repos/${REPO_FULL_NAME}/issues/${PR_NUMBER}/comments" \
--paginate 2>/dev/null \
| jq -s "add // [] | [.[] | select(.body | contains(\"${REDISPATCH_MARKER}\"))
| select(.created_at > (now - 300 | strftime(\"%Y-%m-%dT%H:%M:%SZ\")))]
| length") || RECENT_REDISPATCH=0Flagged by 1/5 review agents (claude-coder), verified by reproducing in bash.
3777cd7 to
4cd91a3
Compare
gh api --paginate applies the --jq expression independently to each page, not over the combined output. Aggregating filters like length, sort_by, and group_by silently produce per-page results, causing multi-line output that breaks bash integer comparisons. Add a Shell scripting section documenting: - The wrong pattern (--paginate --jq '... | length') - The correct pattern (pipe to jq -s for slurp-mode aggregation) - Review guidance to flag this as a medium-severity finding Signed-off-by: Hector Martinez <hemartin@redhat.com>
Fix misleading explanation of --paginate output behavior, add --slurp flag documentation, and prevent pipefail fallback corruption in the redispatch guard by moving the fallback outside the command substitution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
4cd91a3 to
2677c80
Compare
|
🤖 Finished Review · ✅ Success · Started 6:54 AM UTC · Completed 7:06 AM UTC |
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Retro · ✅ Success · Started 6:22 AM UTC · Completed 6:30 AM UTC |
Retro: PR #2288 — document
|
3 participants
Summary
Adds a Shell scripting section to AGENTS.md documenting the
gh api --paginate+ jq per-page behavior that caused the loop-guard bug in PR #1834.What's documented
gh api --paginateapplies--jqper page, not over combined outputlength,sort_by,group_by,add,min_by,max_by) silently break when used directly in--jqwith--paginatejq -sfor slurp-mode aggregation--paginate --jq '... | length'(or similar) as a medium-severity findingValidation
Per issue #2091: next code agent PRs using
gh api --paginatewith jq should usejq -sfor aggregation. Review agent should not flag the per-page pagination issue on new code.Closes #2091