fix(cli): replace stdin auto-detect with explicit --stdin flag#1407
Conversation
readStdin() blocks forever on pipes that never close (test harnesses, subprocess spawners), hanging method run and workflow run in non-TTY contexts. Replace auto-detection with an explicit --stdin flag that callers must pass to opt into reading piped input, matching the jq -n pattern of explicit stdin control. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
CLI UX Review
Blocking
None.
Suggestions
-
Error message still says "piped stdin" instead of
--stdin— bothmodel_method_run.ts:207andworkflow_run.ts:183throw"Cannot combine piped stdin with --input-file."The--input-filehelp text was correctly updated to reference--stdin, but the runtime error message wasn't. Suggest:"Cannot combine --stdin with --input-file."for consistency. -
Silent no-op when
--stdinis passed in a TTY — if a user runsswamp model method run my-server deploy --stdininteractively (forgetting to pipe),readStdin()returns null and the command silently proceeds without stdin input. A brief warning (--stdin specified but stdin is a terminal; ignoring) would be friendlier. Low priority since the behavior is still correct.
Verdict
PASS — the --stdin flag is clearly documented in help text, examples, and the flag reference table. Flag naming and description are consistent with the rest of the CLI. The breaking change from auto-detection to explicit opt-in is well-motivated and properly reflected across all user-facing surfaces.
There was a problem hiding this comment.
Code Review
Clean, well-scoped fix. The core change — gating readStdin() behind an explicit --stdin flag instead of auto-detecting piped stdin — directly addresses the blocking-forever problem in non-TTY contexts. The jq -n pattern is a good design choice. Documentation updates across skills, design docs, and command examples are thorough and consistent.
No CLAUDE.md violations, no libswamp import boundary violations, no security concerns. Domain logic is untouched — this is purely a CLI/infrastructure change.
Blocking Issues
None.
Suggestions
-
Silent no-op when
--stdin+ TTY: If a user passes--stdinbut forgets to pipe data (stdin is a TTY),readStdin()returnsnulland the command silently proceeds without stdin input. Now that the user has explicitly requested stdin reading, this is a confusing UX — consider throwing aUserError("--stdin was passed but stdin is a terminal — did you forget to pipe input?")instead of silently ignoring it. This could be done either inreadStdin()or at the call sites. -
Error message wording: Both
model_method_run.ts:207andworkflow_run.ts:183still say"Cannot combine piped stdin with --input-file."— now that the mechanism is an explicit flag,"Cannot combine --stdin with --input-file."would be clearer and consistent with the new UX. -
No test for the
--stdingating logic: The existing 52input_parser_test.tstests cover parsing, but there's no test verifying thatreadStdin()is only called when--stdinis passed (vs. being called unconditionally as before). A unit test for the conditional would prevent regressions if someone later removes the flag.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
-
Silent breakage for existing piped-stdin scripts —
src/cli/commands/model_method_run.ts:202,src/cli/commands/workflow_run.ts:178After this PR, existing scripts like
echo '{"env":"prod"}' | swamp model method run my-server deploywill silently ignore the piped data — no warning, no error. The command runs with empty inputs and may succeed with defaults or fail with a confusing "missing input" error far downstream.Breaking example: A CI pipeline pipes JSON to
method run. After upgrading swamp, the pipeline silently deploys with wrong/empty inputs.Suggested fix: When
!options.stdin && !Deno.stdin.isTerminal(), emit a one-line warning:"Stdin data available but --stdin not passed — ignoring piped input. Pass --stdin to read it."This gives migrating users a clear signal. The warning could be removed in a future major version.
Low
-
--stdinon a TTY is a silent no-op —src/infrastructure/io/stdin_reader.ts:28If a user runs
swamp model method run foo bar --stdininteractively (no pipe),readStdin()returnsnullbecauseisTerminal()is true. The explicit--stdinflag is silently ignored. Theoretical only — unlikely to cause real confusion, but a debug-level log line ("--stdin passed but stdin is a TTY, ignoring") would be cheap insurance. -
Other
readStdin()callers still auto-detect —model_edit.ts:93,workflow_edit.ts:98,vault_put.ts:181Three other commands still call
readStdin()unconditionally (without a--stdingate). If the blocking-pipe issue from the PR description also affects these commands in test harnesses or subprocess spawners, they remain vulnerable. Likely out of scope for this PR, but worth tracking.
Verdict
PASS — The core fix is correct and well-scoped. The --stdin flag cleanly solves the blocking-pipe problem for method run and workflow run. All docs, examples, and skill files are consistently updated. The only concern is the silent migration path (Medium #1), which is a UX improvement suggestion, not a correctness bug.
Summary
readStdin()blocks forever on pipes that never close (test harnesses, subprocess spawners like cli-testing-library), hangingmethod runandworkflow runin non-TTY contexts--stdinflag — callers must opt in to read piped input, matching thejq -npattern of explicit stdin control--stdinflagTest plan
echo '{"env":"prod"}' | swamp model method run my-server deploy --stdinreads and uses piped inputswamp model method run my-server deploywithout--stdindoes not block on stdinswamp workflow run my-wf --stdin < inputs.jsonreads workflow inputsswamp workflow run my-wfwithout--stdindoes not block on stdindeno run test src/cli/input_parser_test.tspasses (52 tests)🤖 Generated with Claude Code