Skip to content

fix(cli): replace stdin auto-detect with explicit --stdin flag#1407

Merged
stack72 merged 1 commit into
mainfrom
fix/stdin-explicit-flag
May 19, 2026
Merged

fix(cli): replace stdin auto-detect with explicit --stdin flag#1407
stack72 merged 1 commit into
mainfrom
fix/stdin-explicit-flag

Conversation

@stack72
Copy link
Copy Markdown
Contributor

@stack72 stack72 commented May 19, 2026

Summary

  • readStdin() blocks forever on pipes that never close (test harnesses, subprocess spawners like cli-testing-library), hanging method run and workflow run in non-TTY contexts
  • Replaces auto-detection with an explicit --stdin flag — callers must opt in to read piped input, matching the jq -n pattern of explicit stdin control
  • Updates all skill docs, design docs, and command examples to reflect the new --stdin flag

Test plan

  • echo '{"env":"prod"}' | swamp model method run my-server deploy --stdin reads and uses piped input
  • swamp model method run my-server deploy without --stdin does not block on stdin
  • swamp workflow run my-wf --stdin < inputs.json reads workflow inputs
  • swamp workflow run my-wf without --stdin does not block on stdin
  • Existing deno run test src/cli/input_parser_test.ts passes (52 tests)
  • UAT tests that previously hung now complete

🤖 Generated with Claude Code

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>
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLI UX Review

Blocking

None.

Suggestions

  1. Error message still says "piped stdin" instead of --stdin — both model_method_run.ts:207 and workflow_run.ts:183 throw "Cannot combine piped stdin with --input-file." The --input-file help text was correctly updated to reference --stdin, but the runtime error message wasn't. Suggest: "Cannot combine --stdin with --input-file." for consistency.

  2. Silent no-op when --stdin is passed in a TTY — if a user runs swamp model method run my-server deploy --stdin interactively (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.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Silent no-op when --stdin + TTY: If a user passes --stdin but forgets to pipe data (stdin is a TTY), readStdin() returns null and the command silently proceeds without stdin input. Now that the user has explicitly requested stdin reading, this is a confusing UX — consider throwing a UserError("--stdin was passed but stdin is a terminal — did you forget to pipe input?") instead of silently ignoring it. This could be done either in readStdin() or at the call sites.

  2. Error message wording: Both model_method_run.ts:207 and workflow_run.ts:183 still 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.

  3. No test for the --stdin gating logic: The existing 52 input_parser_test.ts tests cover parsing, but there's no test verifying that readStdin() is only called when --stdin is passed (vs. being called unconditionally as before). A unit test for the conditional would prevent regressions if someone later removes the flag.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial Review

Critical / High

None.

Medium

  1. Silent breakage for existing piped-stdin scriptssrc/cli/commands/model_method_run.ts:202, src/cli/commands/workflow_run.ts:178

    After this PR, existing scripts like echo '{"env":"prod"}' | swamp model method run my-server deploy will 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

  1. --stdin on a TTY is a silent no-opsrc/infrastructure/io/stdin_reader.ts:28

    If a user runs swamp model method run foo bar --stdin interactively (no pipe), readStdin() returns null because isTerminal() is true. The explicit --stdin flag 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.

  2. Other readStdin() callers still auto-detectmodel_edit.ts:93, workflow_edit.ts:98, vault_put.ts:181

    Three other commands still call readStdin() unconditionally (without a --stdin gate). 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.

@stack72 stack72 merged commit 81252c6 into main May 19, 2026
11 checks passed
@stack72 stack72 deleted the fix/stdin-explicit-flag branch May 19, 2026 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant