fix(cli): read stdin on demand (CLI-2223) - #6450
Conversation
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@d30986a6bbb87c0ce24cc64da52e1f1bb68ef84fPreview package for commit |
There was a problem hiding this comment.
Superseded by a newer AI review
🤖 AI Review
Eight deduplicated findings remain from 7 Claude and 2 Codex findings. Six are confirmed, including the bare-CR framing mismatch that bypasses the line-size guard, permanent collapse after a read error, missing production-boundary coverage, an uncancellable fd-0 read after timeout, concurrent-reader races, and the understated size bound. One timeout/data-loss race remains uncertain because it depends on Effect/Bun interruption timing not established by the checked-out code. One documentation finding is refuted because the comment already acknowledges one chunk of read-ahead.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | apps/cli/src/shared/runtime/stdin.layer.ts:52 |
correctness |
codex | Bare carriage returns reset the pending-line byte counter even though Effect's line splitter does not delimit bare CR, allowing CR-containing input to bypass the 64 KiB guard and grow as one unbounded line. |
| 🟠 MAJOR | apps/cli/src/shared/runtime/stdin.layer.ts:72 |
error-handling |
claude | Any typed stdin read failure permanently collapses this reader to None, so a transient failure can silently make every subsequent prompt use its default. |
| 🟡 MINOR | apps/cli/src/shared/runtime/stdin.layer.ts:78 |
correctness |
claude | A timeout racing with completion of a pull may consume and discard an entire returned chunk before it is stored in the shared lines buffer. |
| 🟡 MINOR | apps/cli/src/shared/runtime/stdin.integration.test.ts:15 |
test-coverage |
claude+codex | No test verifies the production fd-0 adapter's central backpressure and lifecycle guarantees under a large or unbounded producer. |
| 🟡 MINOR | apps/cli/src/shared/runtime/stdin.layer.ts:26 |
resource-management |
claude | A timed-out prompt can leave an outstanding fd-0 read that cannot be cancelled or destroyed, allowing it to consume input intended for a later stdin consumer. |
| 🟡 MINOR | apps/cli/src/shared/runtime/stdin.layer.ts:77 |
concurrency |
claude | Concurrent readLine calls can race the shared mutable buffer and invoke the same stream pull concurrently, losing or reordering lines. |
| ⚪ NIT | apps/cli/src/shared/runtime/stdin.service.ts:31 |
documentation |
claude | The documented 64 KiB pending-line bound is lower than the implemented bound because enforcement occurs only before the next pull. |
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/shared/runtime/stdin.layer.ts:13(documentation): The comment incorrectly guarantees that all unread input remains available to a child inheriting fd 0, despite one chunk being buffered in-process.
Refuted: The comment explicitly acknowledges up to one chunk of read-ahead.The restnaturally refers to input beyond that chunk; it does not promise that the acknowledged read-ahead chunk or lines already pulled to satisfy a prompt remain visible to a child.
Stats
Claude findings: 7 · Codex findings: 2 · Confirmed: 6 · Refuted: 1 · Uncertain: 1
Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
There was a problem hiding this comment.
🤖 AI Review
Eight deduplicated findings were adjudicated from seven Claude findings and two Codex findings. Four are confirmed: the 64 KiB line limit can be bypassed by a terminating chunk, read failures are silently and permanently treated as EOF, the subprocess test violates the trusted test-category convention, and the read-ahead documentation is imprecise. Windows portability, timeout-arrival data loss, and flood-test flakiness remain uncertain. The process-exit test concern is refuted because production deliberately exits through ProcessControl and the test exits zero only on success.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/shared/runtime/stdin.layer.ts:16 |
portability |
claude | Replacing the platform Stdio abstraction with a raw node:fs stream over fd 0 may break console or piped prompts on published Windows builds, while the new adapter is tested only on Linux CI. |
| 🟡 MINOR | apps/cli/src/shared/runtime/stdin.layer.ts:74 |
error-handling |
claude | A typed stdin read failure is silently converted to None and leaves every subsequent prompt indistinguishable from EOF. |
| 🟡 MINOR | apps/cli/src/shared/runtime/stdin.layer.ts:52 |
correctness |
codex | The 64 KiB pending-line limit is bypassed when an over-limit line terminates in the next pulled chunk. |
| 🟡 MINOR | apps/cli/src/shared/runtime/stdin.layer.ts:120 |
concurrency |
claude | Timing out the shared pull may discard a chunk if input completes concurrently with interruption of that pull. |
| ⚪ NIT | apps/cli/src/shared/runtime/stdin.integration.test.ts:207 |
test-architecture |
claude+codex | The real subprocess/fd-boundary case is placed in the in-process integration suite instead of the separately controlled e2e suite. |
| ⚪ NIT | apps/cli/src/shared/runtime/stdin.integration.test.ts:134 |
test-flakiness |
claude | The flood test performs 10,000 sequential timed reads and assertions under the integration project's default test timeout, creating a potential loaded-runner flake. |
| ⚪ NIT | apps/cli/src/shared/runtime/stdin.service.ts:30 |
documentation |
claude | The readLine documentation overstates how much input remains in the pipe because the adapter may consume one read-ahead chunk beyond the requested lines. |
Findings outside the diff
- 🟡 MINOR
apps/cli/src/shared/runtime/stdin.layer.ts:120— Timing out the shared pull may discard a chunk if input completes concurrently with interruption of that pull.
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/shared/runtime/stdin.integration.test.ts:236(test-quality): Calling process.exit(0) prevents the subprocess test from detecting whether the fd-0 stream would keep a production CLI process alive and obscures program failures.
Refuted: Production also exits explicitly through ProcessControl at apps/cli/src/shared/cli/run.ts:926 and :930, so natural event-loop liveness is not production behavior this test needs to reproduce. Moreover, the process.exit(0) callback runs only on fulfillment, and the parent assertion at stdin.integration.test.ts:249 reports captured stderr on failure.
Stats
Claude findings: 7 · Codex findings: 2 · Confirmed: 4 · Refuted: 1 · Uncertain: 3
Models: claude-opus-5 + gpt-5.6-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
Coly010
left a comment
There was a problem hiding this comment.
Ran this through independent design (architect), correctness (engineer), and Go-parity review. Design and correctness both come back as a clean fix — this genuinely resolves #6287 (piped answers past line 1024 silently taking their default): stdin is now pulled on demand through a backpressured fd-0 stream instead of a 1024-line dropping queue, and collapsing the old TTY-vs-pipe branch into one Channel.toPull-serialized reader actually removes a latent race the old TTY path had (non-atomic Ref get/set outside any lock).
I also independently checked the Go-parity claims used to reject a couple of the AI-review threads above, since the fix for #6287 doesn't need Go's involvement to be justified on its own terms. Two of those don't hold up as literally stated — see the inline notes. Also reproduced one gap none of the automated passes caught (silent EAGAIN swallow). None of this blocks the fix; flagging for a pass before merge.
Also worth a look, non-blocking:
Stdin's four members (readLine,readPipedBytes,readPipedText,pipedBytesStream) each now open an independent handle on fd 0 (theevaluatethunk re-openscreateReadStreamon every pull ofstdin), instead of sharing one Readable the wayStdio.stdindid. Safe today only because no command calls two of them in the same invocation — worth a doc comment onStdinShapespelling that out so it doesn't regress silently later.- The non-TTY 100ms window is measurably stricter than before for a slow multi-prompt pipe: the old background pump kept buffering between prompts (so an answer arriving 150ms after the previous prompt was already queued by the time the next prompt asked), while this reader opens a fresh 100ms window from nothing at each prompt. Scripts that write all answers up front are unaffected, but this is a real narrowing worth an explicit accept on the record rather than silence.
- The fd-0 subprocess test (
stdinLayer over fd 0) landed in*.integration.test.ts; repo policy (and both AI-review passes) puts black-box subprocess tests in*.e2e.test.ts. Your rejection reasoning (it drives the layer directly rather than the CLI binary, and the e2e project's serialization would only weaken the pin) is reasonable — just flagging that the categorization is a judgment call, not obviously settled.
TL;DR
follow-up to #6290 Piped answers past the 1024th line were dropped.
stdin is now read one chunk at a time as prompts ask for it, so nothing is dropped and the rest stays in the pipe...
what was biting?
Bun reads a pipe as fast as it fills and cannot be paused, so #6290 drained it into a queue of 1024 lines to stop
yes | supabase db pushfrom eating memory.Anything past 1024 lines fell off the queue and those prompts took their default...
why this approach is better?
A file stream over fd 0 honours backpressure, so there is no queue and no cap.
Memory is bounded the same way Go's
bufio.Scannerdid it: a line over 64 KiB ends line reading and every prompt from then on takes its default...ref: