fix(chat): read a one-shot prompt from piped stdin

[r0x0r]

Summary

rocm chat --help documents the echo "…" | rocm chat form and states the
prompt is read from stdin when --prompt is omitted, but stdin was never
consumed. The interactive branch requires a TTY, so piped input fell through to
the non-interactive None arm, which rendered the static status screen via
render_chat_text and ignored stdin entirely. Only --prompt ever sent a
request.

Fix

Resolve the prompt from --prompt or, when it is omitted and stdin is not a
terminal, from piped standard input, then route it through the same
render_chat_prompt_text send path as --prompt. A new read_piped_prompt
helper reads stdin to EOF only when it is not a TTY, returning None for an
interactive terminal or empty input so the no-argument behavior (status
screen / interactive dash) is unchanged.

Reproduce

Before this change:

$ printf 'Summarize: the sky is blue.\n' | rocm chat --provider local
Chat assistant

Assistant source: local model on this computer
...

The piped prompt was dropped and the status screen printed. After this change
the same command routes the stdin text through the --prompt send path
(identical output to rocm chat --provider local --prompt "Summarize: the sky is blue.").

Tests

• Unit test command_chat_reads_prompt_from_piped_stdin asserting the no-prompt
  path reads stdin via read_piped_prompt.
• Gherkin scenario @id:chat-cli-stdin-prompt (tests/e2e-cucumber/features/chat.feature)
  that pipes a prompt with no --prompt and asserts the assistant reply is
  produced. Runs on the no-GPU mock lane.

Verified locally: reproduced before the fix, confirmed fixed after; the new and
existing one-shot chat scenarios pass, chat unit tests pass, and
cargo clippy --workspace --all-targets -- -D warnings is clean.

Review follow-ups (b2a0770)

Non-blocking points from the automated review of 48f2718a:

• scripts/smoke_local.py — its run() inherited the script's own stdin, so the
  rocm chat --provider local step is only safe when the harness happens to
  leave fd 0 a TTY or empty. Confirmed on Linux that the built binary waits
  indefinitely with an idle pipe on fd 0 and exits immediately with /dev/null,
  so every smoke child now gets stdin=subprocess.DEVNULL (matching
  scripts/vllm_therock_gpu_test.py). python scripts/smoke_local.py passes.
• The comment above the prompt resolution wrongly claimed the status-screen
  fallback is only reached on an interactive TTY; interactive_terminal() also
  requires stdout to be a TTY, and an empty pipe lands there too. Corrected.
• Documented as deliberate: reading to EOF (the usual filter contract, same as
  read_provider_key_from_user), surfacing a read error rather than treating it
  as "no prompt", and dropping whitespace-only stdin even though --prompt " "
  is forwarded verbatim.
• command_chat_reads_prompt_from_piped_stdin now also pins that the read's
  result supplies the dispatched prompt and happens before the send decision,
  and its docstring no longer implies it covers the behavior — chat-09 does.

[Copilot]

🟡 Changes recommended

Piped prompts currently lose meaningful leading and trailing whitespace.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds piped-stdin support for one-shot rocm chat prompts.

Changes:

• Reads non-terminal stdin when --prompt is omitted.
• Routes piped prompts through the existing one-shot chat path.
• Adds unit and end-to-end regression coverage.

File summaries

File	Description
apps/rocm/src/main.rs	Implements piped prompt handling and a regression test.
tests/e2e-cucumber/features/chat.feature	Adds the stdin prompt scenario.
tests/e2e-cucumber/tests/e2e/chat_steps.rs	Implements the piped-input test step.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jussielo-amd]

Logic is correct: piped stdin now routes through the same render_chat_prompt_text path as --prompt, and the whitespace-preservation follow-up is well tested (asserts on the recorded request body, not just the canned reply, which actually proves fidelity).

Blocking: windows-build-and-test fails — feature_naming rejects the new scenario:

chat.feature: scenario 9 is named "chat-08 - ..." but must start with "chat-09 - "

Main gained a scenario after this branch diverged (#321), so chat-08 is already taken. Rebase onto main and rename the new scenario to chat-09.

Nit: blank piped stdin silently falls back to the status screen with no indication the prompt was empty. A stderr note (like the existing --provider note a few lines up) would remove the ambiguity.

(Strix Halo Ubuntu / MI300X / WSL2 red checks look like runner-availability flake, not caused by this change.)

[r0x0r]

Merged main into the branch and renumbered the new scenario.

main has since added chat-04 (@id:chat-assistant-is-told-which-machine-it-is-on), which shifted the rest of chat.feature up so its last scenario is now chat-08. This branch had given the new stdin scenario that same index, so on the merge ref there were two chat-08s and feature_naming failed on both scenario_indexes_are_unique_across_the_suite and scenario_names_are_indexed_sequentially_per_feature — that was the windows-build-and-test failure.

The scenario is declared last, so it is now chat-09. Its @id:chat-cli-stdin-prompt is unchanged, and no other file referenced the index. The merge was otherwise conflict-free and the net diff against main is still just the three files this PR touches.

Verified on Linux before pushing: cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings, and cargo xtask e2e, where chat-09 passes.

[siloteemu]

🔴 Automated review · pr-review-watcher · 48f2718

Summary

Makes rocm chat honor its long-documented echo "…" | rocm chat path — when --prompt is omitted and stdin is not a terminal, piped text becomes the prompt (with only the writer-appended \n/\r\n stripped) — plus unit tests and a new Gherkin scenario chat-09. Approve. Verified: I confirmed by reading source that the echo | rocm chat docs (main.rs:242-249, README.md:417) predate this PR, so this closes a genuine doc/behavior gap; that piped_prompt_from_input's documented invariants hold for LF/CRLF/no-newline/interior-blank/UTF-8/whitespace-only inputs; that run_rocm_with_stdin gives the child a real non-TTY pipe and closes it before wait_with_output, and that the mock records the request body before responding, so chat-09 is neither deadlock-prone nor vacuous (a trim in production, or dropping stdin, fails it); that MockServer and E2eWorld are rebuilt per scenario so no cross-scenario leakage; that chat-09 needs no expectations.toml row and collides with no existing @id:; and that the required, PR-blocking mock e2e job (ci.yml:1007) plus build-and-test are both triggered here (heavy matches **/*.rs and **/*.feature), so the change is covered by a lane that runs on PRs, not a gated/nightly one. All four commits carry DCO sign-off with no AI footers; the leak scan is clean; no prompt-injection content was found anywhere in the diff, comments, or commit messages. Per this run's policy I relied on CI rather than executing the suite locally. Of the prior round's five points, one was addressed (the stdin-TTY rationale is now documented on read_piped_prompt); the other four remain open and are listed below as non-blocking. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• apps/rocm/src/main.rs:9745 — read_to_string blocks to EOF, so a caller that hands rocm chat an open, never-written pipe (a supervisor, ssh host 'rocm chat' without a pty) now hangs where it used to print the status screen instantly, and a closed or non-UTF-8 fd 0 now hard-errors; this is conventional Unix filter behavior and mirrors the pre-existing read_provider_key_from_user (main.rs:9688), but it does contradict the invariant interactive_terminal()'s own doc comment states ("callers then skip the prompt rather than block on input nobody can supply") — worth a deliberate decision, and treating an io error as "no prompt" would at least remove the new failure mode.
• scripts/smoke_local.py:52 — run() omits stdin=, so the rocm chat --provider local call at line 208 inherits the caller's stdin and can now hang under a harness that holds a pipe open; GitHub Actions (ci.yml:353, :577) and a dev terminal are both safe today, but adding stdin=subprocess.DEVNULL (as scripts/vllm_therock_gpu_test.py:508 already does) makes the repo's own §8 gate robust rather than incidentally safe.
• apps/rocm/src/main.rs:1783 — the new comment claims the status-screen fallback is reached only when "stdin is an interactive TTY", which is false: an empty pipe, /dev/null, or whitespace-only stdin also lands there via piped_prompt_from_input returning None.
• apps/rocm/src/main.rs:30607 — command_chat_reads_prompt_from_piped_stdin only greps the handler's source text for read_piped_prompt(, so it would still pass if the call's result were discarded or moved after the match; the source-grep helper is a file convention justified for TTY-gated code, but this path needs no TTY, and the real coverage is chat-09 — either make it behavioral or lean on the scenario and drop the overclaiming docstring.
• Minor: piped_prompt_from_input returns None for whitespace-only input while --prompt " " is sent verbatim, a deliberate but undocumented exception to the PR's own "piped must equal --prompt" invariant; main.rs:9746's use std::io::{IsTerminal, Read} re-imports the file-scope Read (main.rs:62) instead of following the use std::io::IsTerminal as _; precedent at main.rs:6197; and c592acd's trim() that 6cebd69 immediately replaces would read cleaner squashed.

[r0x0r]

Thanks — worked the five non-blocking points. All five are addressed in b2a0770 (no behavior change to the fix itself).

1. read_to_string blocks to EOF; io error hard-errors — real, kept deliberately, now documented. Confirmed empirically on Linux with the built binary: rocm chat --provider local with an idle pipe on fd 0 is still running after 20s, and exits rc=0 instantly with /dev/null. That is the conventional filter contract and the same one read_provider_key_from_user already follows; a non-blocking timeout/poll would be a redesign of the read, not a nit. I did not take the "treat an io error as no prompt" suggestion: a closed or non-UTF-8 fd 0 means text was piped and could not be decoded, and folding that into a status screen plus exit 0 hides a dropped prompt — the same class of bug this PR fixes. Both decisions are now written on read_piped_prompt.

2. scripts/smoke_local.py inherits stdin — correct, fixed. Verified line for line: run() passed no stdin=, the chat step at :208 has no --prompt, and vllm_therock_gpu_test.py:508 is the in-repo precedent. Out of the touched file set, but the hazard is created by this PR and lands on the repo's own §8 gate, so it belongs here rather than in a follow-up. Every smoke child now gets stdin=subprocess.DEVNULL; no smoke command reads input. python scripts/smoke_local.py passes on Linux (smoke: ok).

3. The "stdin is an interactive TTY here" comment is false — correct, fixed. interactive_terminal() is stdin().is_terminal() && stdout().is_terminal() (crates/rocm-core/src/lib.rs:2141), so a TTY stdin with piped stdout falls through, and an empty pipe or < /dev/null reaches the same arm. The comment now names both ways the fallback is reached.

4. The source-grep test would pass if the result were discarded — correct, tightened rather than dropped (dropping it would lose the wiring guard). It now also pins that the read supplies the dispatched prompt and that it happens before the send decision, and the docstring says plainly that it is a structural guard whose behavioral counterpart is chat-09 (@id:chat-cli-stdin-prompt). Both new assertions were falsified: discarding the result (let _ = read_piped_prompt()?;) fails on "the piped read must supply the prompt that is dispatched" while the old assertion still passed — exactly the gap you flagged; moving the read into the None arm after the match fails on "stdin must be read before the send/status-screen decision". Restored, green.

5. Minor — first two done, third deferred. The whitespace-only/--prompt " " divergence is now documented on piped_prompt_from_input as intentional. use std::io::{IsTerminal, Read} is now use std::io::IsTerminal as _;, matching main.rs:6197 (Read is already in file scope at main.rs:62). Squashing c592acd into 6cebd69 is the one I am leaving: the branch is pushed and under review, so rewriting it would force-push out from under this review and invalidate the 48f2718a anchor — happy to do it at merge time if you squash-merge or ask for it.

Gates on b2a0770 (Linux): cargo test --workspace --all-targets green, cargo clippy --workspace --all-targets -- -D warnings clean, cargo fmt --all --check clean, python scripts/smoke_local.py green. No e2e files changed, so cargo xtask e2e was not re-run; chat-09 is unmodified and still the behavioral coverage.

[siloteemu]

🔴 Automated review · pr-review-watcher · b2a0770

This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Makes rocm chat actually read a one-shot prompt from piped stdin when --prompt is omitted, closing a gap where --help and the README already promised that behaviour but the code printed a status screen instead; adds a whitespace-preserving parse, unit tests, an e2e scenario, and /dev/null stdin for smoke children. No blocking findings. Verified: built apps/rocm and ran the three new unit tests (3 passed, 0 failed); independently confirmed std::io::Read is already in scope (apps/rocm/src/main.rs:62) so the new function compiles, that interactive_terminal() is stdin().is_terminal() && stdout().is_terminal() (crates/rocm-core/src/lib.rs:2141) so a piped stdin can never enter the interactive dash and the new inline comment's parenthetical is accurate, that the stdin contract was already documented pre-diff (apps/rocm/src/main.rs:243-249, README.md:421), that run_rocm_with_stdin drops the child's stdin handle before wait_with_output so EOF is guaranteed (tests/e2e-cucumber/tests/e2e.rs:707-715), that last_chat_request returns an owned Option<Value> with no borrow-of-temporary problem (tests/e2e-cucumber/src/mock_server.rs:603), that the new step strings match the feature file character-exact and expectations.toml needs no entry, and that every other in-repo spawner of rocm chat either passes --prompt, closes the pipe, or runs under a PTY. All three new tests fail if the production change is reverted (the e2e Then panics because the mock never receives a request; both piped_prompt_from_input tests reference a function that would not exist; the structural test's read_piped_prompt( and ordering assertions both break). Leak scan clean. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• scripts/smoke_local.py:59 — the "(same reason as vllm_therock_gpu_test.py)" citation is wrong: that script never runs rocm chat, and its single stdin=subprocess.DEVNULL (line 508) detaches a background serve-http Popen, a different failure mode; a reader who follows the pointer learns nothing and may believe a precedent exists, so drop the parenthetical (the inline reasoning above it already stands on its own).
• apps/rocm/src/main.rs:9752 — genuine user-visible behaviour change for out-of-repo callers: a non-interactive rocm chat with no --prompt whose stdin is an open-but-never-written pipe now blocks forever where it previously printed the status screen; worth one line in the PR body for packagers and script authors, since < /dev/null is the required fix on their side.
• tests/e2e-cucumber/tests/e2e.rs:643 — the generic run_rocm helper sets no stdin, so any future scenario invoking bare chat through it would inherit the test runner's stdin and hang; a default .stdin(Stdio::null()) there closes the trap permanently (not a defect in this PR — no current scenario hits it).
• apps/rocm/src/main.rs:9752 — read_piped_prompt's TTY-guard branch has no unit test; the /dev/null-yields-status-screen fallback is in fact covered by the smoke gate's rocm chat --provider local assertion (scripts/smoke_local.py:214-219), which the new stdin=subprocess.DEVNULL turns into a real guard — saying so in the PR text would stop the next reader concluding the fallback is untested.
• CI shows one genuine failure at this head. I cannot see which lane it is and must not guess one. Nothing in the diff explains it on inspection; the one thing I could not rule out (and am explicitly inferring, not confirming — running clippy was outside my permitted verification budget) is that [workspace.lints.clippy] nursery = "warn" plus CI's -D warnings may flag option_if_let_else on the match input.strip_suffix('\n') { Some(rest) => …, None => input } in piped_prompt_from_input. Worth checking cargo clippy -p rocm --all-targets -- -D warnings locally before assuming the red check is unrelated.

[r0x0r]

The MI300X E2E failure on b2a0770 was not a regression from this PR

The first E2E tests (MI300X) run on b2a0770 reported
FAIL: 'chat-end-to-end-local-model' was expected to pass on this host but FAILED — a regression.
I re-ran that job on the same commit and it is green:

attempt	result
1	95 scenarios (89 passed, 6 failed) — 5 xfail, 1 unexpected failure
2	95 scenarios (90 passed, 5 failed) — 5 xfail, 0 unexpected failure(s)

chat-07 and the new chat-09 are both fully green in attempt 2, and every other GPU lane
(MI350P, rad3 R9700, Strix Halo WSL2/Ubuntu/Windows) was green on attempt 1 already.

What actually failed

Not the stdin path. chat-09 - The chat CLI reads a one-shot prompt from stdin passed on
attempt 1 (all five steps, in 8 ms). The failing step was chat-07's precondition
And a model is served in the background (serving_steps.rs:653) — a step this PR does not
touch — and the cause was a GPU memory shortfall inside vLLM:

attempt 1: ValueError: Free memory on device cuda:0 (14.93/191.98 GiB) on startup is less than
           desired GPU memory utilization (0.92, 176.63 GiB).
attempt 2: ValueError: ... 3.02 GiB KV cache is needed, which is larger than the available
           KV cache memory (0.49 GiB).

Both serve attempts found the card already occupied, while the harness's own drain probe,
taken seconds earlier in ensure_serve_port_free(), had reported
device state: drained (195725 MiB free of 196592 MiB, floor 150000 MiB).

The shape of that is a known hazard in the serial GPU lane, and commands.jsonl from the run
shows it: chat-06 runs rocm serve Qwen/Qwen3.5-0.8B --engine vllm --managed and never
stops it, so chat-07 always has to reclaim the card from its predecessor.
kill_listeners_on_port() does that with fuser -k 11435/tcp, which reaches only the process
holding the socket — vLLM v1 splits APIServer and EngineCore into separate processes
(APIServer pid=41476, EngineCore pid=41719 in the archived log), and it is the EngineCore
that owns the ~177 GiB. So the port can close, amd-smi can read the device as drained, and a
still-live or supervisor-restarted engine can take the card back inside the ~25 s window before
the next engine probes it. That race is pre-existing and lives entirely in the GPU e2e harness;
#251 (EAI-8058) is the adjacent work.

Why this PR cannot reach chat-07

• GPU lanes run max_concurrent_scenarios(1) (tests/e2e-cucumber/tests/e2e.rs), and chat-09
  is appended after chat-07 in chat.feature — it cannot run beside it or shift its start.
• The diff to chat_steps.rs is purely additive (two consts, one When, one Then).
• run_rocm spawns via Command::output(), which sets the child's stdin to Stdio::null();
  run_rocm_with_stdin writes then drops ChildStdin, closing the pipe. No e2e child is ever
  handed an idle fd 0, so the new read_to_string cannot block in this suite.

No code change was made — there is nothing here for this PR to fix.

One thing worth a separate decision

The "an idle pipe blocks rocm chat" concern is real, just unrelated to the failure above. On
Linux, with this branch built:

• rocm chat < /dev/null → rc=0 immediately, status screen
• rocm chat < <idle fifo> → still running at 20 s (timeout returns 124)
• printf 'hi\n' | rocm chat → rc=0, prompt path

That is the documented filter contract (read_piped_prompt's doc comment spells it out, and
scripts/smoke_local.py was given /dev/null for exactly this reason), so it is deliberate
rather than a defect — but whether rocm chat should bound that wait, or treat a silent pipe
as "no prompt", is a product decision I have deliberately not made here.

[siloteemu]

🔴 Automated review · pr-review-watcher · b2a0770

This automation posts comments only. It never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

On the GPU e2e failure — I'm satisfied, and I want to be clear about why, because "I re-ran it and it went green" on its own is the weakest possible argument and usually a way of explaining away a real regression. What makes it convincing here is everything attached to the re-run: the failing step is a precondition of a different, pre-existing scenario that this PR doesn't touch; the new stdin scenario itself passed on the first attempt in 8 ms; the lanes run one scenario at a time, so an appended scenario can't run alongside the failing one or move its start; and the step-definition diff is purely additive. On top of that there's an actual named mechanism — the serving engine splitting into a front end and an engine-core process, with the reclaim killing only the socket holder while the core keeps the allocation, which explains exactly the contradiction of a drain probe reporting a clear device seconds before the next engine finds the card occupied. A mechanism plus an unrelated failing step is a real diagnosis, not a hand-wave. Agreed: not a regression from this PR, and the race belongs to the harness.

The idle-pipe behaviour is the more interesting point, and I'm glad you raised it unprompted. Hanging forever on a silent pipe is standard filter semantics and I don't think it's a defect in the change — /dev/null returns immediately, piped text returns immediately, and the helper being handed /dev/null is the right call. But a doc comment on a function is not an adequate answer for user-facing behaviour. Nobody who runs the command and watches it sit there for 20 seconds is reading the source. If the contract stays as-is, it should be visible where users actually look — the command's help text or the user docs — rather than only in the implementation.

Whether the command should bound the wait or treat silence as "no prompt" is a genuine product call, and I agree it shouldn't be made inside this PR. Leaving it open is the right choice; just don't let it evaporate — it wants a follow-up of its own. Neither point is blocking as far as I'm concerned. This looks good to me.