Repository navigation
fix: bound the stdin read so an open pipe cannot hang the CLI (COR-13656) - #18
Closed
Bradenream wants to merge 1 commit into
Closed
Bradenream wants to merge 1 commit into
Bradenream wants to merge 1 commit into
Conversation
Every command that accepts a request body reads stdin to EOF. When stdin
is a pipe that is open but never sends data and never closes, io.ReadAll
blocks forever and the CLI hangs with no output. That is 76 of 138
commands, and it is the worst failure available to this CLI: a coding
agent or CI job stalls with nothing to act on. It also masquerades as
success, since a harness that times out the process reports empty output
rather than a hang.
Two shapes hang, and both are fixed:
sleep 30 | vf agent update ... never writes, never closes
( echo '{}'; sleep 30 ) | vf ... writes, then holds the write end
The read is now bounded at 10s and fails loudly, naming the fix, instead
of stalling. The logic lives in a new non-generated file; the edit to
generated metadata.go is two one-line call swaps.
Deciding whether to read is still done by file type, never by polling
stdin for readiness. A readiness check was the first design and it was
wrong: it cannot tell 'silent forever' from 'slow by ten milliseconds'
without waiting, which is the thing it exists to avoid. Measured against
the real binary, a zero-timeout poll discarded the body from curl, node
and python3 pipelines on every run, silently — including the pipeline
this repo's own README documents. Waiting and failing loudly is slower
in the pathological case and correct everywhere else.
The new tests drive real shell pipelines rather than execa's input:
option. That distinction is the point — input: writes from an
already-warm parent at spawn time, so it survives designs that real
pipelines do not. The rejected poll passed the entire existing suite
while breaking every cold producer.
There was a problem hiding this comment.
🟡 Changes recommended
The timeout error text and associated test assertion are currently inaccurate because the timeout can also occur when stdin has data but hasn’t reached EOF by the deadline, which can mislead users.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses a critical CLI reliability issue where commands that accept a request body can hang indefinitely when stdin is a non-TTY pipe that never reaches EOF, by introducing a bounded stdin read and adding integration tests that exercise real shell pipelines.
Changes:
- Introduces
internal/flagutil/readStdinBoundedwith a 10s timeout and a teaching error message when stdin can’t be read to completion. - Updates generated request-building helpers to use the bounded stdin reader instead of
io.ReadAll. - Adds a new Vitest suite that validates stdin behavior using “cold” producers via
sh -cpipelines (node/python/echo/slow producer + silent pipe cases).
File summaries
| File | Description |
|---|---|
| test/stdin-handling.test.ts | New integration tests that drive real shell pipelines to validate stdin-read correctness and bounded failure behavior. |
| internal/flagutil/stdin.go | New bounded stdin reader with timeout semantics and user-facing guidance on failure. |
| internal/flagutil/metadata.go | Switches stdin reads in request builders from io.ReadAll to the bounded reader. |
Review details
Files not reviewed (1)
- internal/flagutil/metadata.go: Generated file
Suppressed comments (1)
internal/flagutil/stdin.go:75
- The timeout error text claims "stdin ... sent no data", but readStdinBounded times out whenever io.ReadAll hasn’t reached EOF within StdinReadTimeout (including when the producer already wrote some or all of the body but keeps the pipe open). This can mislead users and makes the message incorrect for the write-then-hold-open case.
return nil, fmt.Errorf(
"%w after %s: stdin is open but sent no data and never closed.\n"+
"If you did not mean to pipe a request body, redirect it: vf <command> </dev/null\n"+
"If you did, make sure the producer closes its write end when it is finished",
ErrStdinTimeout, StdinReadTimeout,
- Files reviewed: 2/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+39
to
+41
| // ErrStdinTimeout is returned when stdin stayed open past StdinReadTimeout | ||
| // without delivering data or closing. | ||
| var ErrStdinTimeout = errors.New("timed out reading stdin") |
Comment on lines
+86
to
+90
| expect(result.timedOut, 'the CLI hung instead of bounding the read').toBe(false); | ||
| expect(result.exitCode, 'giving up must be an error, not a silent empty body').not.toBe(0); | ||
| expect(result.stderr).toContain('stdin is open but sent no data'); | ||
| expect(result.stderr).toContain('</dev/null'); | ||
| }); |
DecathectZero
approved these changes
Aug 31, 2026
effervescentia
approved these changes
Aug 31, 2026
Contributor
Author
Merge activity
|
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
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.
Part of COR-13656. First of four PRs from the CLI ease-of-use work.
The bug
Every command that accepts a request body reads stdin to EOF. If stdin is a pipe that is open but never sends data and never closes,
io.ReadAllblocks forever. 76 of 138 commands are affected.This is the worst failure this CLI can produce. A hang gives a coding agent or CI job nothing to act on, and it masquerades as success: a harness that times the process out reports empty output, which is how this originally surfaced during validation as a mysterious "exit 0, zero bytes" that we could not explain.
A second shape hangs too, and is arguably more common in the wild — a producer that writes and then keeps the write end open (a CI wrapper, an agent harness, a subshell inheriting the shell's stdout):
The fix
The read is bounded at 10s and fails loudly rather than proceeding with an empty body:
Whether to read is still decided by file type, never by polling for readiness.
A readiness poll was the first design, and it was wrong
Worth recording, because it looked obviously correct. Adversarial review built that patch and measured it against the real binary.
syscall.Selectwith a zero timeout fires ~1ms after exec; real producers write later — node 10-14ms, python3 8-13ms. Measured body loss over 15 runs each:It silently broke the
curl -s … | vf <command>example this repo's own README documents at line 486, and the discarded producer gotEPIPE— an unhandled crash in Node — so the pipeline failed at both ends with errors pointing nowhere near the CLI.No timeout can both admit a cold Node producer and bound a silent pipe, because those are the same observation. Readiness cannot be the decision. Waiting and failing loudly is slower in the pathological case and correct in every other one.
On the tests
The new tests drive real shell pipelines, not execa's
input:option, and that distinction is the point.input:writes from an already-warm parent at spawn time — the fastest possible producer — so it survives designs that real pipelines do not. The rejected poll passed all ~30 existing stdin tests while breaking every cold producer. A suite that cannot fail is not coverage.Added: node / python3 / echo / slow-producer pipelines asserting the body arrives, the node case repeated 20× so a race fails reliably rather than flaking, the open-silent-pipe case asserting a bounded exit with a teaching message, write-then-hold-open asserting the body survives, and the existing
input:shape kept as a backward-compatibility guard.Verification
go build ./...,go vet ./internal/...cleanFootprint
Logic lives in a new non-generated file (
internal/flagutil/stdin.go), following theinternal/cli/docs.gopattern. The edit to generatedmetadata.gois two one-line call swaps plus an import removal — deliberately minimal, since that file is Speakeasy-tracked and every line there is a future merge conflict.