#643

feat(interpreter): stream first-stage for-loop output

Draftasafyishwants to merge intofrom

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

Stream completed statements from a first-stage for loop with static words and no outer redirections. Reuse the existing loop executor and pipeline, preserving control-flow status, diagnostics and isolated shell state.

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

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

vercel-security-reviewer[bot]12d ago
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/12
Changeset included✅ (.changeset/streaming-pipelines.md)

Review panel: 🟡 medium highest severity

just-bash maintainer code review: 🟡 medium

Streaming works on success, but failure propagation corrupts partial non-ASCII output.

  • packages/just-bash/src/interpreter/streaming-pipeline.ts:319 — Fatal pipeline errors lose the accumulator's `stdoutKind: "bytes"`: `prependTo` copies Latin-1-shaped bytes into `ControlFlowError.stdout`, which is returned as text. Thus partial non-ASCII output emitted before a streaming stage fails is mojibake (and its accounting uses the wrong UTF-8 length). Preserve/decode the byte kind when propagating failures.

General code review: 🟡 medium

Streaming overrides cannot safely delegate piped input to non-streaming original commands.

  • packages/just-bash/src/interpreter/builtin-dispatch.ts:947 — A streaming custom override inherits `stdio` plus a `stdin` getter that always throws, and `origCommand` invokes the bundled command with that same context. Delegating to a non-streaming original that reads stdin (for example, a streaming `sort` override calling `ctx.origCommand(args)` in `seq | sort`) therefore fails instead of consuming the pipe; the original command needs a buffered adapter context.

Adversarial security: 🟢 low

No actionable adversarial security findings in the complete base-to-head diff.

Adversarial security (second opinion): 🟡 medium

No backdoor, exfiltration, or sandbox-escape path found; the streaming pipeline is well gated (bails on redirections, builtins, functions, fds, groupStdin, extraArgs) and resource-bounded, but concurrent stages leak DefenseInDepthBox trusted scope to sibling-stage builtin/interpreter code, which should be fixed or consciously accepted before merge.

  • packages/just-bash/src/interpreter/streaming-pipeline.ts:180 — Concurrent streaming stages share one DefenseInDepthBox executionId, and `isTrustedContext()` consults the static per-executionId `trustedExecutionDepth` counter. The PR closes this for command bodies by wrapping untrusted `cmd.execute` in `runUntrustedAsync` (builtin-dispatch.ts:1031), but builtins dispatched via `dispatchBuiltin` and interpreter-level work (expansion, redirection, loop body execution in a `For` stage) are not wrapped. So while a trusted streaming stage (`defineCommand` defaults to `trusted: true`) is in flight, all sibling-stage builtin/interpreter work sees `shouldBlock() === false` and runs with global-access blocking and audit logging disabled for that window — a defense-in-depth regression that did not exist before intra-exec concurrency was introduced.

Standard Bash and host portability: 🟡 medium

Streaming cat breaks ordered operand processing and early pipeline termination for mixed file/stdin inputs.

  • packages/just-bash/src/commands/cat/cat.ts:124 — Streaming `cat` eagerly drains stdin whenever any `-` operand exists, before processing earlier file operands. Thus `seq 1000000000 | cat /file - | head -n1` can hit limits or hang instead of immediately outputting `/file` as standard `cat` does. Consume stdin only when its `-` operand is reached.

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

asafyish11d ago

Confirmed all three loop-specific findings with failing regressions and fixed them in 0be6ff4f:

  • An 80-statement loop now completes with default defense enabled; its periodic yield uses the existing captured timer.
  • set -e; for i in a; do exit 7; done | cat; echo survived prints survived, matching native Bash. false is covered too. Streaming stages convert exit/errexit to stage results using the same approach as buffered pipelines.
  • for i in a; do echo err >&2; echo out; done | head -n 0 retains err on stderr. The loop keeps already-produced diagnostics when returning broken-pipe status.

The shared trusted-scope overlap was also reproduced and fixed by 691c1faa (the existing runUntrustedAsync wrapper at command dispatch). Details and the reusable foundation commit are posted on #453.

150 focused pipeline/control-flow tests pass, along with type checking, formatting and banned-pattern checks. Three loop regressions added; no new pipeline implementation.

asafyish10d ago

Verified the control-flow/broken-pipe finding and fixed it in bab73de1. for i in a; do { echo err >&2; echo out; break; }; done | head -n 0 previously dropped err\n; the same happened with continue and exit 7. The existing diagnostic regression now covers those cases. The fix catches the final emit's broken pipe and lets already-populated control-flow errors propagate to the existing stage handler.

The macOS seq claim is incorrect for the supported host used to verify this: the system-provided /usr/bin/seq 1 3 runs successfully on macOS 15.7.3. These seq-based comparison cases also come from the shared #453 foundation; normal comparison runs use recorded fixtures. This finding does not justify a fixture rewrite.

Also carried the shared result-getter protection from #642 (679e6098 here). The mixed-file/stdin cat issue reproduces in #453 and remains a separate dependency concern.

154 focused loop, control-flow, and pipeline tests pass, along with typecheck and banned-pattern checks.

asafyish9d ago

Confirmed and fixed the UTF-8 failure-output and mixed cat operand findings in the shared foundation. The failure regression uses a preceding text statement: printf ö; seq 1 | fail, where fail writes é\n then throws a limit error, now retains öé\n rather than mojibake.

For original-command delegation, a wrapper that only calls ctx.origCommand(args) should use the default buffered command contract. Verified a sort override in seq 3 | sort -r returns 3\n2\n1\n. Explicitly opting into streaming: true promises stdio consumption; delegating unchanged to a buffered original violates that contract and raises the existing explicit stdin error. I am not adding an implicit buffering adapter to this loop PR.

For the broader trusted-depth observation: interpreter/builtin code is repository-owned, and host hooks are trusted under THREAT_MODEL.md §1.1. Untrusted command bodies and result normalization are explicitly scoped with runUntrustedAsync; the newly verified frozen-result getter hole is fixed and tested. I did not reproduce an additional attacker-controlled callback bypass in the remaining interpreter work, so I am not expanding the trust model based only on the shared counter observation.

Rebased onto current main, preserving loop status handling; scoped pipeline/control-flow tests, typecheck, formatting, knip and banned-pattern checks pass.

This pull request is a draft

Merging is unavailable until it is marked ready for review.