From 0aec86f5418606e93b5796cb43cc5324b61b26c4 Mon Sep 17 00:00:00 2001 From: Hector Martinez Date: Mon, 15 Jun 2026 09:09:26 +0200 Subject: [PATCH 1/2] docs(#2091): document gh api --paginate + jq per-page semantics 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 --- AGENTS.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 5620b735fd..367ed3c511 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,6 +46,30 @@ The e2e tests require GitHub credentials. There are three ways to provide them: If only `E2E_GITHUB_USERNAME` and a password source are available, `make e2e-test` will automatically generate a session file before running tests. See `make help` for all available targets. +## Shell scripting + +### `gh api --paginate` and jq + +`gh api --paginate` applies the `--jq` expression **independently to each page** of results, not to the combined output. This is a documented `gh` CLI behavior and a common source of bugs. + +**Do not** use aggregating jq filters directly in `--jq` with `--paginate`: + +```bash +# WRONG — `length` runs per-page; multi-line output breaks integer comparisons +count=$(gh api --paginate /repos/{owner}/{repo}/issues/comments --jq '.[].id | length') +``` + +**Do** collect all pages first, then pipe to a separate `jq -s` (slurp) call: + +```bash +# Correct — slurp (-s) combines all pages into one array before aggregating +count=$(gh api --paginate /repos/{owner}/{repo}/issues/comments | jq -s 'length') +``` + +This applies to any aggregating filter: `length`, `sort_by`, `group_by`, `add`, `min_by`, `max_by`, etc. If the filter only selects or transforms individual items (e.g., `.[] | .id`), per-page application is fine — but pipe the result through a final `jq -s` step before any cross-page aggregation. + +**When reviewing shell scripts:** Flag `--paginate --jq '... | length'` (or any other aggregating filter in `--jq`) as a medium-severity finding. The fix is always to move the aggregation to a separate `| jq -s '...'` pipe. + ## Forge abstraction All git forge operations (GitHub API calls, PR comments, issue creation, workflow dispatch, etc.) **must** go through the `forge.Client` interface defined in `internal/forge/forge.go`. This is a fundamental architectural rule — the codebase supports multiple forges (GitHub, GitLab, Forgejo) and direct coupling to any single forge breaks the abstraction. From 2677c80985ce4aa79f2a1aaa3b3cdaf06d900207 Mon Sep 17 00:00:00 2001 From: Hector Martinez Date: Mon, 15 Jun 2026 09:12:08 +0200 Subject: [PATCH 2/2] fix(#2091): correct jq -s aggregation example in paginate guidance 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 Signed-off-by: Hector Martinez --- .github/workflows/reusable-fix.yml | 3 ++- AGENTS.md | 16 ++++++++++------ .../fullsend-repo/scripts/post-review.sh | 6 +++--- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/.github/workflows/reusable-fix.yml b/.github/workflows/reusable-fix.yml index a42f9e378a..1f75a6c543 100644 --- a/.github/workflows/reusable-fix.yml +++ b/.github/workflows/reusable-fix.yml @@ -255,7 +255,8 @@ jobs: # agent execution, allowing at most +1 overshoot. The concurrency # group's cancel-in-progress mostly prevents this. FIX_COMMITS="$(gh api "repos/${SOURCE_REPO}/pulls/${PR_NUM}/commits" \ - --paginate --jq '[.[] | select(.commit.author.name == "fullsend-fix")] | length' 2>/dev/null)" \ + --paginate 2>/dev/null \ + | jq -s 'add | [.[] | select(.commit.author.name == "fullsend-fix")] | length')" \ || { echo "::warning::Could not count prior fix commits — defaulting to cap"; FIX_COMMITS="${ITERATION_CAP:-5}"; } ITERATION=$(( FIX_COMMITS + 1 )) echo "Fix iteration: ${ITERATION} (${FIX_COMMITS} previous fix commits)" >&2 diff --git a/AGENTS.md b/AGENTS.md index 367ed3c511..b2e1968742 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,20 +55,24 @@ If only `E2E_GITHUB_USERNAME` and a password source are available, `make e2e-tes **Do not** use aggregating jq filters directly in `--jq` with `--paginate`: ```bash -# WRONG — `length` runs per-page; multi-line output breaks integer comparisons -count=$(gh api --paginate /repos/{owner}/{repo}/issues/comments --jq '.[].id | length') +# WRONG — `length` runs per-page; produces one number per page, not a total +count=$(gh api --paginate /repos/{owner}/{repo}/issues/comments --jq 'length') ``` -**Do** collect all pages first, then pipe to a separate `jq -s` (slurp) call: +**Do** collect all pages first, then pipe to a separate `jq -s` (slurp) call. `jq -s` slurps the input into an array; use `add` to flatten before aggregating: ```bash -# Correct — slurp (-s) combines all pages into one array before aggregating -count=$(gh api --paginate /repos/{owner}/{repo}/issues/comments | jq -s 'length') +# CORRECT — slurp all pages, flatten with add, then aggregate +count=$(gh api --paginate /repos/{owner}/{repo}/issues/comments | jq -s 'add | length') ``` +Without `--jq`, `gh api --paginate` merges all page arrays into a single flat JSON array before writing to stdout. `jq -s` then wraps that into an array-of-one; `add` unwraps it back to the flat array, and the aggregating filter runs once over all items. This pattern is defensive — it works correctly whether the upstream emits one merged array or (as when `--jq` is present) one array per page. + This applies to any aggregating filter: `length`, `sort_by`, `group_by`, `add`, `min_by`, `max_by`, etc. If the filter only selects or transforms individual items (e.g., `.[] | .id`), per-page application is fine — but pipe the result through a final `jq -s` step before any cross-page aggregation. -**When reviewing shell scripts:** Flag `--paginate --jq '... | length'` (or any other aggregating filter in `--jq`) as a medium-severity finding. The fix is always to move the aggregation to a separate `| jq -s '...'` pipe. +**When reviewing shell scripts:** Flag `--paginate --jq '... | length'` (or any other aggregating filter in `--jq`) as a medium-severity finding. The fix is always to move the aggregation to a separate `| jq -s 'add | ...'` pipe. + +**Alternative — `--slurp` flag:** When no inline `--jq` transform is needed, `gh api --paginate --slurp` combines pages into a single array directly. However, `--slurp` is mutually exclusive with `--jq` (errors with `"the --slurp option is not supported with --jq or --template"`), so the `| jq -s 'add | ...'` pipe pattern is required whenever you also need per-item filtering. ## Forge abstraction diff --git a/internal/scaffold/fullsend-repo/scripts/post-review.sh b/internal/scaffold/fullsend-repo/scripts/post-review.sh index ee196d4461..8e1725cefc 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-review.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review.sh @@ -159,10 +159,10 @@ if [ "${POST_REVIEW_EXIT}" -eq 10 ]; then REDISPATCH_MARKER="" RECENT_REDISPATCH=$(gh api \ "repos/${REPO_FULL_NAME}/issues/${PR_NUMBER}/comments" \ - --paginate --jq \ - "[.[] | select(.body | contains(\"${REDISPATCH_MARKER}\")) + --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" 2>/dev/null || echo "0") + | length") || RECENT_REDISPATCH=0 if [ "${RECENT_REDISPATCH}" -gt 0 ]; then echo "Recent stale-head re-dispatch already exists — skipping"