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 5620b735fd..b2e1968742 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,6 +46,34 @@ 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; 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. `jq -s` slurps the input into an array; use `add` to flatten before aggregating: + +```bash +# 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 '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 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. 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"