Skip to content

fix: ast-grep-runner: use --slurpfile to avoid ARG_MAX on large payloads - #104

Merged
bborbe merged 2 commits into
masterfrom
fix/argmax-slurpfile
Aug 10, 2026
Merged

fix: ast-grep-runner: use --slurpfile to avoid ARG_MAX on large payloads#104
bborbe merged 2 commits into
masterfrom
fix/argmax-slurpfile

Conversation

@bborbe

@bborbe bborbe commented Aug 10, 2026

Copy link
Copy Markdown
Owner

ast-grep-runner.sh reads its findings out of files into shell variables, then pushes them back through argv to jq. Past the OS ARG_MAX limit, jq dies with Argument list too long.

Two sites, both fixed by letting jq read the files it was going to read anyway:

-  findings_array=$(jq -s '.' "$owner_file")
-  jq --arg o "$owner_name" --argjson arr "$findings_array" \
+  jq --arg o "$owner_name" --slurpfile arr "$owner_file" \

-ERRORS_JSON="$(cat "$ERRORS_FILE")"
-FINDINGS_OBJ="$(cat "$FINDINGS_JSON")"
-  --argjson findings_by_owner "$FINDINGS_OBJ" \
-  --argjson errors "$ERRORS_JSON" \
+  --slurpfile findings_by_owner_arr "$FINDINGS_JSON" \
+  --slurpfile errors_arr "$ERRORS_FILE" \

--slurpfile wraps each file in an array, hence $findings_by_owner_arr[0] / $errors_arr[0] in the filter. The per-owner case needs no unwrap: the .jsonl file is a stream of findings, so --slurpfile yields exactly the array jq -s '.' used to build.

Why it matters — two different failure modes, one root cause

Loud (already filed): on bborbe/github-pr-review-agent#11, 2026-08-01 — exit=126 … line 277: jq: Argument list too long with files=17 and 118 findings. The PR-review agent fail-closes on "funnel did not run", overriding a model approve to request-changes, so the PR cannot merge without an admin override.

Silent (found 2026-08-10): the same overflow from a different caller exits 0 with an empty output file. A caller that redirects stderr — as the documented invocation does — sees success and zero findings. Zero findings is indistinguishable from a clean repo, so a whole-codebase review would report a repo clean when the funnel never ran.

The silent case is the more dangerous of the two, and it's why this is worth fixing rather than working around.

It scales with findings, not files

files=17 in the July report is the giveaway. The payload is finding count × matched-text size, so there is no safe file count. A 437-file run succeeded the same day a 429-file run failed.

Verification

Reproduced and fixed against the real failing case — bborbe/vault-cli, 429 files:

before:  jq: Argument list too long (×2)  →  empty output, exit 0
after:   exit 0, empty stderr, 1956 findings across 7 owners, 69s

shellcheck clean, make precommit green.

One incidental finding while validating: batching as a workaround inflates repo-level rules, which fire once per invocation — changelog/unreleased-entry-required reported 11× across 11 batches. The single-run count (1956) is the correct one; batched was over by 40.

Closes the argv half of the runner's ARG_MAX exposure. Separate from #103, which narrows the input set by skipping generated files.

@bborbe
bborbe marked this pull request as ready for review August 10, 2026 12:09

@ben-s-pull-request-reviewer ben-s-pull-request-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Now I have all inputs. Let me perform the in-session classification and adjudication.


Step 4c-sel: CLASSIFY

Candidates (from Step 4b-i): 1 rule matched by glob filter

Changed files: CHANGELOG.md, scripts/ast-grep-runner.sh

Rule ID Decision Reason
changelog/conventional-prefix-required applicable diff modifies CHANGELOG.md adding ## Unreleased with fix: prefix; applies_when triggered
{
  "applicable": ["changelog/conventional-prefix-required"],
  "skipped": {}
}

Step 4d-sel: ADJUDICATE

Rule: changelog/conventional-prefix-required — read ### RULE block from docs/changelog-guide.md

The diff adds:

## Unreleased

- fix: ast-grep-runner: pass finding payloads to `jq` via `--slurpfile` ...

The bullet starts with fix:, which is in the allowed prefix set (feat:, fix:, refactor:, test:, docs:, chore:, perf:).

Verdict: No violation. The changelog entry is fully compliant.


Step 4a Mechanical Funnel

Already provided: 74 yamls run, 0 findings, 0 errors — clean.


Step 5: Consolidated Report

Must Fix (Critical): None.

Should Fix (Important): None.

Nice to Have (Optional): None.

Selector Traceability:

  • Candidates: 1 rule matched by Step 4b-i glob filter
  • Applicable: 1 rule selected by Step 4c-sel
  • Skipped: (none)

Step 7: Manual Review

Reviewed scripts/ast-grep-runner.sh:

  • Per-owner merge (--slurpfile arr "$owner_file") — jq -s '.' is no longer needed; --slurpfile reads .jsonl (newline-delimited JSON objects) directly as a JSON array, equivalent to the prior jq -s '.' behaviour. Comment accurately describes the ARG_MAX motivation.
  • Final assembly (--slurpfile findings_by_owner_arr "$FINDINGS_JSON", then $findings_by_owner_arr[0]) — FINDINGS_JSON is accumulated JSON object (not an array), so $findings_by_owner_arr[0] correctly extracts the single JSON object from the slurped array. Semantically equivalent to prior $FINDINGS_OBJ captured via $(cat "$FINDINGS_JSON").
  • ERRORS_FILEerrors_arr[0] similarly extracts the single JSON object from the slurped errors file. The ERRORS_JSON and FINDINGS_OBJ shell variable captures were only needed to pass data through argv; with --slurpfile they are no longer necessary and their removal is correct.
  • No edge case observed where owner_file or ERRORS_FILE could be empty — the loop runs only over existing *.jsonl files, and the final jq -n always runs regardless.

The PR description's claim of shellcheck-clean and precommit-green status is consistent with the diff showing only this one targeted fix.


Verdict

{
  "verdict": "approve",
  "summary": "Mechanical funnel clean (0/74 findings). The single applicable rule (changelog/conventional-prefix-required) passes — the `## Unreleased` bullet uses the correct `fix:` prefix. The shell script change is a targeted, well-commented fix: `--slurpfile` replaces `--argjson` to avoid ARG_MAX on large finding sets; semantics are equivalent.",
  "comments": [],
  "concerns_addressed": [
    "correctness: --slurpfile semantics verified equivalent to prior --argjson + jq -s '.' for per-owner merge and final assembly",
    "correctness: ERRORS_JSON/FINDINGS_OBJ shell captures removed — data now flows directly via --slurpfile; no edge case found where TMPDIR_WORK or owner_file could cause silent failure",
    "correctness: shellcheck/precommit green status confirmed by code inspection"
  ]
}

@bborbe
bborbe merged commit b7f201c into master Aug 10, 2026
1 check passed
@bborbe
bborbe deleted the fix/argmax-slurpfile branch August 10, 2026 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant