refactor(interpreter): model stdin as a single shared StdinStream#285
refactor(interpreter): model stdin as a single shared StdinStream#285a0preetham wants to merge 1 commit into
Conversation
|
@mysorepreetham is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
@cramforce Claude seems to write loops and the pipes within a loop do not work currently. |
|
Please run changeset, sign the commit, and revisit test coverage |
059330d to
36d29c0
Compare
|
Done! ✅ Changeset — .changeset/while-loop-stdin-cursor.md with patch bump |
36d29c0 to
638d663
Compare
|
Updated with the requested changes. Here's a summary of what's in the latest push: Changeset — Additional fixes surfaced by test coverage:
Test coverage (25 tests across 2 files, all passing):
Coverage for changed files (unit suite, WASM tests excluded):
The gaps in |
|
Follow-up push: extended stdin-redirect support to all remaining compound commands and fixed a subtle What changedNew: stdin redirect support for All five now process # Now works correctly in all compound commands
if true; then
IFS= read -r a
IFS= read -r b
echo "$a $b"
fi < /file # "line1 line2"
for ((i=0; i<3; i++)); do
IFS= read -r line
echo "$i: $line"
done < /file # "0: x\n1: y\n2: z"
case 1 in
1) IFS= read -r a; IFS= read -r b; echo "$a $b";;
esac < /file # "hello world"Bug fix:
Coverage (8141 tests, 377 files)
Key files:
18 new tests added covering: |
|
Coverage before/after comparison (unit suite, WASM/security tests excluded): Key files
Overall suite (tests only, not line counts changed)
|
|
Updated coverage — added Key files — before → after
Branch coverage on Overall
|
|
Final coverage update — added
|
| Stage | Stmts | Branches | Functions |
|---|---|---|---|
| Before this PR | 83.8% | 70.0% | — |
| After StdinCursor refactor | 76.1% | 63.1% | — |
+ if/for/until/case fix + </<</<<< tests |
82.75% | 74.76% | — |
+ <<- strip-tabs + file-not-found tests |
90.18% | 78.5% | 100% |
Overall (8163 tests, 377 files)
| Metric | Value |
|---|---|
| Statements | 61.38% (26372/42963) |
| Branches | 56.88% (17460/30695) |
| Functions | 64.34% (2462/3826) |
| Lines | 61.98% (25311/40835) |
Remaining uncovered branches in control-flow.ts are pre-existing edge cases unrelated to this PR: failglob mode in for loops, break/continue inside a while condition expression, and the loopError="error" path in nested loop error handling.
Replace the groupStdin?: string snapshot (and its per-call-site fallback rules) with one representation of stdin everywhere: a StdinStream object that works like a bash file descriptor — content bytes plus a read offset, shared by reference. Reading is consuming, so a command that drains stdin advances the offset for every other holder of the stream, including across subshell and pipeline-stage boundaries. The bug class "read stdin but forgot to advance it" is impossible by construction. Interpreter changes: - src/stdin-stream.ts: StdinStream with readAll() (consume), peek() (forward without consuming), advance(n) (partial consumers). - IOState.stdin: StdinStream replaces groupStdin; install/restore is scoped via withStdin() in stdin-redirect.ts. - stdin-redirect.ts: single resolver for <, <<, <<-, <<<, <& shared by simple commands, all compound commands, and function-def redirects. This extends stdin redirect support to if/for/until/case and fixes the heredoc byte-encoding and mojibake-prone file reads that the while/group copies of this logic had diverged on. - Pipeline stages install a fresh stream per stage (first stage inherits, matching bash fd inheritance); the "clear groupStdin and never restore it" bug goes away, as does the empty-pipe-output fallback to the enclosing scope's stdin. - read/mapfile consume via peek()+advance()/readAll() on the shared stream; eval/subshell/group/user-script stdin plumbing deleted (they inherit the stream by reference). - Function definition redirects now apply per call and win over piped stdin; a missing redirect file errors without running the body (both match real bash). CommandContext (breaking, TypeScript): ctx.stdin is the stream; ~35 commands now call ctx.stdin.readAll(), forwarders (timeout/time/env/ bash) use ctx.stdin.peek(). diff reads stdin once so `diff - -` compares stdin against itself. StdinStream is exported from the package root. Tests: 40 ported compound-redirect tests pass unchanged; new StdinStream unit tests; 15 new comparison tests recorded against real bash covering consumption in loop bodies, subshell offset sharing, pipeline isolation, and function redirect semantics. Full suite green except pre-existing python3/WASM failures caused by git-LFS pointers not being materialized in this checkout (identical on main). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ba58f83 to
f51269c
Compare
|
@vercel run a review |
Fixes stdin position tracking in loops and compound commands:
groupStdinwas a string snapshot. This fix makes stdin stream-like: one shared object that tracks the offset already read, so any consumer advances the position for everyone.What
Replaces the interpreter's
IOState.groupStdin?: stringsnapshot with a singleStdinStreamobject — the only representation of stdin anywhere in the interpreter. It works like a bash file descriptor: content bytes plus a read offset, shared by reference. Reading is consuming: a command that drains stdin advances the offset for every other holder of the stream, including across subshell and pipeline-stage boundaries.Why
The old string-snapshot approach had a recurring bug class: commands inside a
while readloop body (pipelines,cat,grep,tr, …) consumed the loop's stdin without advancing the shared position, so the next iteration re-read the same input. The fallback rules (stdin || groupStdin) were duplicated acrossbuiltin-dispatch,read,mapfile, and the pipeline glue, andpipeline-execution.tsclearedgroupStdinwithout restoring it.Design
src/stdin-stream.ts—StdinStreamwithreadAll()(consume),peek()(forward without consuming; used bytimeout/time/env/bash),advance(n)(partial consumers:read,mapfile).src/interpreter/stdin-redirect.ts— one resolver for<,<<,<<-,<<<,<&, shared by simple commands, all compound commands (if/for/while/until/case/subshell/group), and function-def redirects; pluswithStdin()for scoped install/restore. This replaces four diverged copies of the redirect-processing loop and fixes the heredoc UTF-8 byte-encoding those copies were missing.CommandContext.stdinis now aStdinStream(breaking, TypeScript): commands callctx.stdin.readAll();StdinStreamis exported from the package root. A changeset (minor) documents the migration.Behavior fixes verified against real bash: subshells share the stdin offset with the parent; function definition redirects apply per call and win over piped stdin; a missing redirect file on a function errors without running the body;
diff - -compares stdin against itself;cat - -reads stdin once.Test coverage
src/interpreter/while-loop-stdin.test.ts— 40 tests covering stdin redirects (<,<<,<<-,<<<, file-not-found) on all six compound commands, pipelines inside loop bodies, andreadposition tracking. Carried over from the previous design and passing unchanged — the rewrite preserves all of its behavior.src/comparison-tests/stdin-consumption.comparison.test.ts— 15 new comparison tests with fixtures recorded from real bash: consumption bycat/grep/trin loop and group bodies, subshell offset sharing, pipeline-stage isolation, piped-loop position keeping, function redirect-vs-pipe precedence, missing-file redirect errors,cat - -,diff - -, empty-pipe no-fallback, nested loop redirects, andevalconsuming group stdin.src/stdin-stream.test.ts— 12 unit tests for the stream primitive itself (consume/peek/advance/exhaustion/shared-reference semantics).pnpm typecheck, lint, knip, spec tests (3825 passed), comparison tests (553 passed), and unit tests (13k+) all green. The only failures in a full run are pre-existing python3/WASM tests caused by unmaterialized git-LFS blobs in the local checkout (identical onmain).🤖 Generated with Claude Code