#642

perf(jq): read bounded file batches and emit per-input results

Draftasafyishwants to merge intofrom

Closes https://github.com/vercel-labs/just-bash/issues/634

Read jq input files in bounded batches and emit each evaluated input before reading later batches. Slurp still collects its input; parsing and evaluation are unchanged. File byte accounting is tracked separately in #650.

Draft while #453 remains unmerged. This branch is rebased onto current main with the shared prerequisite fixes. Focused follow-up diff. The full GitHub diff includes the prerequisite; it should be rebased again once that prerequisite lands.

Validation: focused behavior tests, TypeScript, scoped Biome, knip and banned-pattern checks pass. No manual workflow rerun or review-bot request was made.

Activity

15
vercel[bot]12d ago

@asafyish is attempting to deploy a commit to the Vercel Labs Team on Vercel.

A member of the Team first needs to authorize it.

auto-maintain[bot]12d ago

🤖 auto-maintain review

Automated, advisory triage for @asafyish's PR. Facts below are read from the GitHub API.

CheckResult
Author's merged PRs (this repo)1
Account established✅ (age 5392d · 13 followers · 7 public repos)
Commits signed/verified⚠️ 0/13
Changeset included✅ (.changeset/streaming-pipelines.md)

Review panel: 🟡 medium highest severity

just-bash maintainer code review: 🟢 low

No actionable defects found in the complete base-to-head diff.

General code review: 🟢 low

No actionable correctness issues found in the complete base-to-head diff.

Adversarial security: 🟢 low

No actionable adversarial security issues found in the complete diff.

Adversarial security (second opinion): 🟢 low

Reviewed the full streaming-pipeline and jq batching diff from an adversarial security angle: the eligibility gate keeps attacker-reachable constructs (redirections, assignments, builtins, aliases, dynamic words, inherited stdin/FDs) on the buffered executor, stages still go through normal command dispatch so no resolution or expansion check is skipped, stdio is revocable and defense-context-wrapped like fs/env, output accounting nets to a single charge with forged-accounting credit capped, and the switch to runUntrustedAsync tightens rather than loosens the trust scope under concurrency. No dependency, install-script, CI, or network changes. No actionable security findings.

Standard Bash and host portability: 🟡 medium

The streaming path breaks standard pipeline behavior for common `cat` invocations involving options or mixed file/stdin operands.

  • packages/just-bash/src/commands/cat/cat.ts:124 — Streaming `cat` eagerly drains all stdin before processing operands whenever any transform or file operand is present. Thus `seq 1000000000 | cat file - | head -n1` waits for or exhausts the producer instead of emitting `file` first and stopping it as ordinary Bash does. Consume stdin at its operand position and stream transformations incrementally.

Posted by auto-maintain. This automated code review is advisory; a human maintainer makes the call.

asafyish11d ago

Verified and fixed the local correctness cases in 569f488c:

  • File operands are evaluated/emitted before a later - operand reads stdin. Whole-input stdin decoding remains as documented. The regression exercises downstream closure while an earlier file is being emitted.
  • jq . /first /missing | cat, with /first containing 1, preserves 1\n before the missing-file diagnostic/status 2. The successful prefix is yielded before reporting the batch error.
  • jq -r .a /in on {"a":""} emits \n both standalone and in a pipeline. Native jq agrees; the buffered empty-output check was wrong.

The proposed rawfile/slurpfile fix assumes incorrect native behavior. jq 1.8.2 opens a literal file named - for these options. Given that file containing 1\n2\n and piped stdin 99\n:

jq -cn --rawfile v - '$v'    # "1\n2\n"
jq -cn --slurpfile v - '$v'  # [1,2]

Without a file named -, jq reports a file-open error. The small fix is allowStdinMarker: false on the existing binding-file reader, rather than adding stdin-backed bindings. Both modes are covered.

The byte-accounting observation is real but inherited from the shared reader. I ran jq empty /f0 ... /f49 on 50 files of roughly 10 KB each with maxInputBytes: 64 and maxLiveBytes: 4096: both unchanged #453 (262a17b2) and this branch succeed. The batch refactor does not establish a byte bound, and its description only promises a 50-file bound. Correcting shared file-input accounting needs separate scope; I have flagged it rather than folding that change into this PR.

Also carried the shared trust-scope fix from #453 in 7bc7c9db. Three focused tests added; 388 jq/pipeline tests pass, plus type checking, formatting and banned-pattern checks.

asafyish11d ago

The confirmed file-input/live-memory budget gap is now addressed separately in #650, based directly on main. It covers ordinary jq file operands and --rawfile/--slurpfile without bringing streaming changes into that fix. The PR links back here and to #634; three focused regressions and all 826 relevant tests pass locally.

auto-maintain[bot]11d ago

⚠️ auto-maintain: review severity raised to 🔴 high

A new push changed this PR and the review now contains a higher-severity finding. See the updated review comment above.

asafyish10d ago

Verified the latest findings against this PR's previous head.

  • The deferred result-getter finding is real. Fixed in 6fcc99cb by keeping result accounting and copying inside the existing trusted/untrusted callback. The existing overlapping-stage regression now also exercises a returned getter; that case failed before this fix.
  • The misplaced jq streaming flag is real. Fixed in 1619d621 by moving the property onto jqCommand. The existing early-stop test now covers direct registration as well as the normal registry; direct registration previously read all 1,000 files.
  • I could not reproduce the output-accounting claim, including its suggested late-error case. jq . /first /missing | cat, with /first containing 1, produces 2 stdout bytes and 40 diagnostic bytes. A 42-byte output budget permits the result; 41 bytes fails with status 126. accountResult only credits explicit internalOutputAccounting; the shared checkpoint caps existing credit and does not create credit from sibling output. jq returns no such marker. I have left that accounting design unchanged.

The mixed cat prefix - issue does reproduce, but originates in the shared #453 foundation. It remains separate from jq's operand handling, which already emits earlier files before reading stdin.

The jq and streaming-pipeline checks, the accounting reproduction, typecheck, and changed-file formatting pass. No additional review was requested.

asafyish9d ago

Confirmed and fixed the plain mixed-operand cat case in the shared foundation: seq 1000000000 | cat /prefix - | head -n 1 outputs the file first and cancels the producer. Cat transformation options still use the existing buffered transformation path; a new incremental formatter is outside this jq batching fix. The shared trust-accounting, UTF-8 failure-output and bounded pipe-read fixes are included. Rebased onto current main; the jq batch change remains separate from #650’s input-budget fix.

vercel-security-reviewer[bot]9d ago

This pull request is a draft

Merging is unavailable until it is marked ready for review.