fix(bin): keep fleet-sized snapshot documents out of jq's argv - #2777
Open
x45dev wants to merge 2 commits into
Open
fix(bin): keep fleet-sized snapshot documents out of jq's argv#2777x45dev wants to merge 2 commits into
x45dev wants to merge 2 commits into
Conversation
The fleet snapshot passed whole JSON documents to jq on the command line with --argjson. execve caps a single argument string far below the total ARG_MAX (MAX_ARG_STRLEN is 128 KiB on Linux), so once a home's backlog document crossed that ceiling every snapshot aborted with "Argument list too long" before jq ran. It is a hard size ceiling, not a jq bug, and it only worsens as a home grows. Every document that grows with the fleet - the backlog, the task inventory, a home summary, the aggregated secondmate records and their accumulator - now reaches jq through a pipe via `--slurpfile <name> <(printf '%s' "$doc")`, read back as `($<name>[0]) as $<binding>` so each filter body is unchanged and every caller's output contract stays byte-identical. Nothing is staged on disk, so no signal trap is needed and the bounded, routinely killed cross-home reads still die promptly leaving nothing behind. An empty document keeps the hard failure --argjson raised, since through a pipe it would otherwise bind null and silently reshape the output. --argjson stays where the value is bounded by something other than fleet size: a flag, a count, a single id, one task's own state or paths, or a surface an FM_SNAPSHOT_* cap already truncates. Tests size their fixture from the ceiling the host actually enforces and assert the document really cleared it, so the regression is neither slow here nor vacuous elsewhere; both the canonical and home-summary modes are covered, plus the "stages nothing on disk" contract.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (2): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Fix bin/fm-bearings-snapshot.sh failing outright with "jq: Argument list too long" / "fm-fleet-snapshot: main inventory summary failed".
Root cause: bin/fm-fleet-snapshot.sh's main_inventory_json() passed whole JSON documents to jq on the command line via --argjson backlog "$1" and --argjson tasks "$2". With 67 backlog records the serialized document exceeds the kernel's argv limit, so execve refuses before jq ever runs. It is a hard size ceiling, not a jq bug, and it only worsens as the backlog grows.
Required fix: stop passing JSON documents through argv. Feed them to jq through files or stdin - --slurpfile/--rawfile against a temp file, or piping one document in and reading the other with --slurpfile, whichever reads more naturally in the surrounding code. Keep the function's signature and its output contract byte-identical so every caller is unaffected.
Required audit: audit the rest of the file in the same change. main_inventory_json is where it surfaced, but any other --argjson carrying a document that grows with the fleet has the same latent ceiling. Fix every instance found; leave --argjson alone where the value is genuinely small and bounded (a flag, a count, a single id).
Required test: ship a test that fails without this change - construct a backlog document large enough to exceed getconf ARG_MAX on the host, run the snapshot, and assert it produces valid output rather than erroring. Follow whatever test convention this repo already uses; do not invent a new harness.
Required verification: run bin/fm-bearings-snapshot.sh against the real home and confirm it returns a complete snapshot.
Decisions and tradeoffs made while doing the work, which a reviewer reading only the diff would not know:
--slurpfile <name> <(printf '%s' "$doc"), read back as($<name>[0]) as $<binding>) over staging a temp file. Deliberate: nothing is staged on disk, so no signal trap is needed, and the bounded, routinely killed cross-home reads elsewhere in this script still die promptly and leave nothing behind. The binding form was chosen specifically so every jq filter body stays identical to the --argjson form it replaced, keeping the output contract byte-identical.Constraints: this is firstmate's own shared tracked material, so firstmate-coding-guidelines applies - bin/*.sh must pass shellcheck and bin/fm-lint.sh, tests stay colocated in tests/ extending the existing tests/fm-fleet-snapshot-view.test.sh rather than adding a runner, one sentence per line in tracked markdown, plain dash never an em dash, and no agent co-author on the commit.
Verified before this run: the new test fails without the fix with the exact reported error ("jq: Argument list too long", 258 records over this host's 131072-byte ceiling); the full tests/fm-fleet-snapshot-view.test.sh suite is 17/17 green with it; bin/fm-lint.sh passes (ShellCheck 0.11.0, actionlint 1.7.12); and bin/fm-bearings-snapshot.sh against the real home returns rc=0 with a complete snapshot (64 backlog records, 8 tasks, 2 secondmates, main_inventory.valid true).
Known and deliberately out of scope: tests/fm-bearings-snapshot.test.sh fails on this machine under load, with its bounded cross-home reads reporting "structured home snapshot timed out" at a load average around 19. That failure reproduces at HEAD without this change and the failing test moves between runs, so it is a pre-existing load-sensitive flake, not a regression from this work, and it is deliberately not fixed here.
What Changed
bin/fm-fleet-snapshot.sh, replaced--argjsonwith--slurpfile <name> <(printf '%s' "$doc")(bound back via($<name>[0]) as $<binding>) for every JSON document whose size grows with the fleet, so large documents reachjqthrough a pipe instead of argv and stop tripping the kernel's per-argument execve limit:main_inventory_json,secondmate_home_summary_json,parent_evidence_reconciliation_json,secondmate_current_json(including its per-record accumulator, previously rebuilt through argv on every loop iteration),secondmate_landed_from_current_json, and the top-level snapshot assembly.--argjsonin place for values bounded independently of fleet size (flags, counts, a single id, one task's own state, an already-truncatedFM_SNAPSHOT_*surface).require_json_docsguard so an empty document still hard-fails as it did under--argjson, instead of silently slurping to[]/nulland reshaping output.tests/fm-fleet-snapshot-view.test.shcoverage: a regression test sized from the host's actualARG_MAXceiling for both--jsonand--secondmate-home-summarymodes, plus a test asserting the snapshot stages nothing on disk under a privateTMPDIR.Risk Assessment
✅ Low: The change mechanically replaces --argjson with guarded --slurpfile/process-substitution at every site that carries a fleet-sized document (verified via grep that all --slurpfile call sites have a matching require_json_docs guard, and that all remaining --argjson uses carry genuinely bounded scalars/single-record values, matching the stated audit scope), preserves each jq filter body unchanged apart from the added binding preamble, adds a require_json_docs guard with correct empty-vs-null semantics, and ships a regression test that sizes its fixture from the host's actual argv ceiling and asserts both output validity and that the fixture genuinely exceeds the ceiling; no risky, ambiguous, or unverified behavior was found.
Testing
All 17 tests in tests/fm-fleet-snapshot-view.test.sh pass against the fix, the two new regression tests were confirmed to fail with the exact pre-fix error against the base commit's script, and manual end-to-end runs of both bin/fm-fleet-snapshot.sh --json and its bin/fm-bearings-snapshot.sh wrapper against a 300-record synthetic backlog completed successfully (rc=0, valid output) - the author's real firstmate home referenced in the intent's verification note is not reachable from this isolated test worktree, so a synthetic large-backlog home was substituted to demonstrate the same end-to-end path.
Evidence: Pre-fix regression test output (confirms failure without the change)
Evidence: Manual fm-bearings-snapshot.sh run against a small synthetic home
Evidence: Manual fm-bearings-snapshot.sh run against a 300-record oversize backlog
Evidence: fm-fleet-snapshot.sh --json output over a 300-record oversize backlog
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-fleet-snapshot-view.test.shon the fix (db3ab48) - all 17 tests pass, including test_backlog_larger_than_one_argv_string (258 records over a 131072-byte ceiling) and test_snapshot_stages_nothing_on_diskSame two new tests run in isolation against the pre-fix bin/fm-fleet-snapshot.sh (base dc0172c) via a scratch copy - fails with the exact reported error: 'jq: Argument list too long' / 'fm-fleet-snapshot: main inventory summary failed'Manual end-to-end run:FM_HOME=<synthetic-home> bin/fm-fleet-snapshot.sh --jsonagainst a 300-record queued backlog - rc=0, valid JSON, backlog.records length 300, main_inventory.valid trueManual end-to-end run:FM_HOME=<synthetic-home> bin/fm-bearings-snapshot.shagainst the same 300-record backlog - rc=0, complete TOON snapshot with 20-row gates table shown (bounded by default fields)Manual smoke run:FM_HOME=<small-synthetic-home> bin/fm-bearings-snapshot.shagainst a 3-record backlog (queued/done mix) - rc=0, complete snapshot✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.