Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion .github/workflows/reusable-fix.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 28 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

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.
Comment thread
rh-hemartin marked this conversation as resolved.

**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.
Expand Down
6 changes: 3 additions & 3 deletions internal/scaffold/fullsend-repo/scripts/post-review.sh
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,10 @@ if [ "${POST_REVIEW_EXIT}" -eq 10 ]; then
REDISPATCH_MARKER="<!-- fullsend:stale-head-redispatch -->"
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"
Expand Down
Loading