fix(cli): route rocm fix failures to stderr, preserve log guard on exit by jussielo-amd · Pull Request #350 · ROCm/rocm-cli

jussielo-amd · 2026-09-07T10:37:35Z

Summary

rocm fix failure explanations ("refusing to apply", "usermod exited N; group membership NOT changed", etc.) now go to stderr instead of stdout, so a caller capturing only stdout on a non-zero exit still sees why.
rocm fix no longer calls std::process::exit directly — it was the only place in the codebase doing so, and it skipped main()'s _log_guard destructor, risking a truncated log on exactly the failure paths where it matters most. Its exit code now flows back through main()'s ordinary return via a FixExitCode marker error, downcast in a small exit_code_for() helper.
Exit code semantics (2/3/4/5) are unchanged — only which stream explains the failure, and how the process gets to that code.

Test plan

cargo test -p rocm-core fix — 23 passed
cargo test -p rocm — 555 passed (+3 new unit tests for exit_code_for)
cargo xtask e2e -- -n "diagnose-0[5-8]|diagnose-11|diagnose-12" — all 6 fix-related e2e scenarios pass, including the two updated stderr assertions
cargo fmt --check and cargo clippy --workspace --all-targets -- -D warnings clean
grep -rn "process::exit" apps/rocm/src crates/rocm-core/src — no remaining raw std::process::exit calls (the pre-existing clap err.exit() for mistyped invocations is untouched, flagged as a separate out-of-scope follow-up)

Copilot

🟡 Changes recommended

Generic error reporting can panic on stderr failure, and an important command-failure path lacks scenario coverage.

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

Pull request overview

Routes rocm fix failure diagnostics to stderr while preserving log cleanup during exit.

Changes:

Moves fix failure messages from stdout to stderr.
Propagates fix exit codes through main().
Updates unit and end-to-end assertions.

File summaries

File	Description
`apps/rocm/src/main.rs`	Adds structured exit-code handling.
`crates/rocm-core/src/fix.rs`	Routes failure diagnostics to stderr.
`tests/e2e-cucumber/tests/e2e/diagnose_steps.rs`	Verifies selected stderr diagnostics.

Review details

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

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

Copilot

🟢 Approval recommended

The implementation preserves existing exit semantics, allows cleanup to run, and covers the changed behavior end to end.

Review details

Files reviewed: 4/4 changed files
Comments generated: 0 new
Review effort level: Balanced

Copilot

🟡 Changes recommended

The new unit test unsafely mutates global environment state, and several comments incorrectly describe anyhow downcasting behavior.

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

Review details

Suppressed comments (2)

apps/rocm/src/main.rs:1714

This repeats the incorrect claim that adding anyhow::Context breaks the downcast. anyhow deliberately preserves downcasting to the original error through context layers, so this prohibition should be removed or replaced with an accurate description of the carried marker.
apps/rocm/src/main.rs:19174
The test does exercise the dispatch chain, but it does not guard against adding .context(...): anyhow preserves FixExitCode downcasts through context layers, so that proposed change would keep this test passing. Update the comment to describe what the test actually guarantees.

Files reviewed: 4/4 changed files
Comments generated: 2
Review effort level: Balanced

Failure explanations in `rocm fix` (declined confirmation, wrong-OS refusal, subprocess failures) were printed to stdout despite the command exiting non-zero, so a caller capturing only stdout on success sees a clean-looking failure with no visible reason. Separately, `rocm fix` was the only place in the codebase calling std::process::exit directly, which skips main()'s _log_guard destructor and can silently truncate the log on exactly the failure paths where it matters most. Its exit code now flows back through main()'s ordinary return via a FixExitCode marker error instead. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

run_path_export_linux/windows had four failure-explanation println! calls left over when this branch's other rocm-fix failure messages moved to stderr; they return the same exit code (3) as sibling lines in the same functions that were converted, so leaving them on stdout broke the stream/exit-code consistency this change established. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

assert_inapplicable_fix_declined and assert_refuses_without_agreement checked stdout+stderr concatenated, so they would still pass if the refusal text regressed back onto stdout -- the one thing this PR's stderr move needs to guarantee. Assert against cli_stderr alone. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

eprintln! panics on a write failure (e.g. closed stderr pipe), which would replace the intended exit code with a panic. Match std's Termination behavior for Result<(), E> and ignore the write failure instead. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

diagnose-08/-11 cover the refusal branches (no agreement, wrong OS), but the third failure shape -- an approved, applicable fix whose helper command itself fails -- had no e2e coverage. Add diagnose-13, which forces usermod/sudo to fail via a scenario-scoped PATH override and asserts the explanation lands on stderr with exit code 4. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

assert_unknown_fix_refused and assert_position_argument_corrected checked stdout+stderr concatenated, the same weakness already fixed for their sibling assertions in this branch's stderr migration. Both messages are eprintln!-only already, so tighten these two the same way for consistency. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

exit_code_for recovers the exit code via downcast_ref, which only works if FixExitCode reaches it unwrapped. Note the invariant so a future .context() on the fix() call doesn't silently break it. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

… errors - confirm() now prints "Not confirmed; refusing to apply." on every decline path (interactive no/EOF, not just the non-interactive refusal), and its yes/no parsing is extracted into is_affirmative_answer() with direct unit tests. - Replace fix.rs's failure-explanation eprintln! calls with a fail! macro that ignores write errors, matching the panic-safe pattern main.rs already uses for its generic error fallback. - Add a regression test exercising the real dispatch -> fix -> FixExitCode -> exit_code_for chain, plus a pointer comment at the Fix dispatch arm warning against wrapping the call (e.g. with .context(...)), which would silently break the exit-code downcast. The interactive decline path was manually verified via a real PTY (prints the message, exits 5); no new PTY e2e scenario was added since it would require extending the TUI-specific PTY driver for a case already covered by the unit test and the shared confirm() fix. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…w test Two review comments from earlier in the PR were already resolved by prior commits (exit_code_for's panic-safe write, e2e coverage for the stderr command-failure branch). Two were real: - The doc comment claiming .context(...) breaks the FixExitCode downcast was wrong: anyhow::Error::downcast_ref searches the whole error chain, so context-wrapping is safe. What actually breaks the downcast is discarding the error into a fresh anyhow!(...) instead of chaining it. Corrected the claim at all three sites (the FixExitCode doc comment, the dispatch-arm pointer comment, and the regression test's doc comment). - The new dispatch-level regression test mutated ROCM_CLI_DISABLE_STARTUP_UPDATE_CHECK via raw env::set_var/remove_var instead of the existing ScopedTestEnv guard, so it wasn't serialized against other env-touching tests and would leak the mutation on a panic. Switched to ScopedTestEnv. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

siloteemu · 2026-09-09T17:46:11Z

🔴 Automated review · pr-review-watcher · 0cef54d

Summary

Routes rocm fix failure/refusal output to stderr behind a new non-panicking fail! macro, and replaces the mid-stack std::process::exit in fix() with a FixExitCode marker error so main() (now ExitCode-returning) can unwrap the code without skipping the logging guard's destructor. Verdict: Approve. Verified: read the full diff against the base plus surrounding run()/dispatch()/apply()/run_render_group()/e2e-harness context, and asked the revert question of every added or changed test — the three assertions that matter (diagnose_steps.rs:543 "This fix only applies on:", :688 "refusing to apply", and the new diagnose-13 "usermod exited" pair) all target messages that were println! before this PR, so each fails on revert; the classic vacuous "combined stdout+stderr" shape is what this PR deletes, and the new scenario is the only one that additionally asserts absence from stdout; dispatch_carries_fixs_exit_code_through_to_exit_code_for genuinely fails on revert (the old process::exit(2) would kill the test binary); is_affirmative_answer_* cover a newly extracted pure function and both pass locally (cargo test -p rocm-core --lib is_affirmative_answer, the one permitted cheap check); I also confirmed no other test, doc, script or CI file asserts the moved strings on stdout, that no std::process::exit remains in apps/rocm/src, and that the new unit test's update-check gate short-circuits before any network call; relied on CI (26 passing, 1 skipped) for the e2e/cucumber run and full-workspace build. No prompt-injection text and no company-internal identifiers in the added lines. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

apps/rocm/src/main.rs:1198,1237 — clap's err.exit() still calls process::exit inside run() after _log_guard is constructed, so the exact guard-skipping class of bug this PR fixes for fix survives on usage/parse errors; worth the same treatment while you're here.
crates/rocm-core/src/fix.rs:626,724,852,958 — eprint!("{err}") (and the adjacent print!("{out}")) still panic if the pipe is closed, so the I/O hardening the fail! macro introduces is only partial inside the very functions that use it.
crates/rocm-core/src/fix.rs:528 — the new "Not confirmed; refusing to apply." message on an interactive decline has no coverage; only the non-interactive branch is asserted (diagnose-08).
tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:543,688 — consider adding the negative stdout assertion the new diagnose-13 step uses, so these two match its strength rather than only proving the text reached stderr.
tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:652,668 — these two tightenings ("Unknown fix-id", "position") cover messages that were already eprintln! before this PR, so unlike the others they pass identically with the production change reverted; harmless as regression guards, but the commit framing them as newly-stderr-routed refusals overstates what they add.

…teractive decline Closes the remaining non-blocking findings from PR #350's review (#350 (comment)), all in the same class of bug that PR fixed for `fix()`: - `parse_cli()` and the mistyped-subcommand branch in `run()` still called `clap::Error::exit()` directly, bypassing `_log_guard`'s destructor the same way the pre-PR `fix()` did. Both now return a `ClapExitCode` marker error through the ordinary `Result` path instead. - Four call sites relaying a captured subprocess's stdout/stderr with `print!`/`eprint!` could still panic on a closed pipe, unlike `fail!`. Consolidated them behind a `relay_output` helper that ignores write failures for the same reason `fail!` does. - `confirm()`'s interactive-decline branch had no e2e coverage: the piped- stdin harness can never reach it, since `is_terminal()` is always false there. Added a PTY-driven scenario (diagnose-14) that types "n" at a real terminal prompt and asserts the same refusal outcome as the non-interactive path. - Strengthened the diagnose-08/-11 refusal assertions to also check the message never leaks onto stdout, matching the bar diagnose-13 already set. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd · 2026-09-10T07:22:42Z

Addressed all 5 non-blocking findings from the review in 294a183:

parse_cli() and the mistyped-subcommand branch in run() now return a ClapExitCode marker error through the ordinary Result path instead of calling clap::Error::exit() directly, so _log_guard still flushes on a usage/parse error.
The four print!/eprint! subprocess-output relays are consolidated behind a relay_output helper that ignores write failures, same as fail!.
Added diagnose-14, a PTY-driven e2e scenario covering confirm()'s interactive-decline branch, which the piped-stdin harness can never reach (is_terminal() is always false there).
Strengthened the diagnose-08/-11 refusal assertions to also check the message never leaks onto stdout, matching diagnose-13's existing bar.

Verified: cargo build --workspace, cargo clippy --workspace --all-targets, cargo test -p rocm-core --lib (332 passed), and the full diagnose/dash/chat/engine-shell e2e scenarios (34 scenarios, all passed) — the last set specifically to confirm the TuiSession::wait_for_exit/wait_for_exit_code refactor didn't regress any existing PTY-driven caller.

siloteemu · 2026-09-10T08:21:12Z

🔴 Automated review · pr-review-watcher · 294a183

Summary

Routes rocm fix's and clap's exit codes back through main()'s return path so the logging guard's destructor runs, moves rocm fix's failure/refusal messages to stderr behind panic-safe fail!/relay_output helpers, and adds a PTY scenario for the interactive decline — good change, no blocking defects. Verified: item 1 PARTIALLY FIXED — the mistyped-subcommand path and Cli::from_arg_matches now return ClapExitCode, but parse_cli's cli_command().get_matches() still calls clap's internal e.exit(), so the common usage error (rocm --bogus, a bad arg value, --help) still process::exits past the guard; item 2 FIXED — all four relays go through relay_output; item 3 FIXED — the new scenario drives a real PTY; item 4 FIXED — both refusals now assert stderr and negative-assert stdout; item 5 STILL OPEN — the unknown-id and position tightenings still cover messages that were already on stderr pre-PR. Exit-path behaviour check clean: clap 4.6.0's Error::exit() is exactly print() + exit(exit_code()), so code 2, stream selection and text are unchanged; exit_code_for prints nothing for the marker types so there is no double-print; and the generic branch reproduces std's Termination for Result verbatim (Error: {e:?} to stderr via an ignored write, exit 1). Revert question: 4 of 6 e2e assertions and the dispatch unit test fail on revert; the clap round-trip unit test and the two item-5 tightenings pass on revert. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

apps/rocm/src/main.rs:1262 — cli_command().get_matches() still routes every real argv parse failure through clap's internal e.exit() (clap_builder-4.6.0/src/builder/command.rs:763), so the guard-skipping bug the PR names is only closed for from_arg_matches (effectively unreachable once get_matches succeeded) and the freeform mistyped-subcommand branch. Switch to try_get_matches() and feed its error through the same exit_code()/print()/ClapExitCode handoff; until then the commit subject "flush log guard on clap exit" overstates the coverage.
apps/rocm/src/main.rs:19348 — clap_error_exit_code_survives_the_clap_exit_code_round_trip constructs ClapExitCode(code) itself and never calls parse_cli/run(), so it passes unchanged if the production call sites are reverted and merely duplicates exit_code_for_clap_exit_code_carries_the_code; its docstring's "end to end" / "the real clap parse failure -> ClapExitCode -> exit_code_for chain" claim is inaccurate. Contrast dispatch_carries_fixs_exit_code_through_to_exit_code_for, which genuinely dies on revert.
tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:683,705,711 — item 5 stands: "Unknown fix-id" and the position hint were already on stderr before this PR, so both tightened assertions pass identically on revert. Useful as forward stream guards, but they protect nothing in this diff.
tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:277 — the new scenario reuses the Given that plants fix-home/.bashrc and whose piped sibling pins HOME/SHELL via world.command_env, but TuiSession::spawn applies only isolate_env()+pty_env(); the PTY child gets HOME=<root>/home and the runner's inherited $SHELL, so the planted rc file is inert and shell_rc_file() resolves .zshrc on a zsh runner. Harmless today (the run declines before writing) but the setup is silently unused; consider having the step set HOME/SHELL for the PTY path and adding the rc-untouched assertion the piped scenario has.
crates/rocm-core/src/fix.rs:438-947 — the I/O hardening covers failure paths and relays; the ~60 remaining success/plan println!s still panic on a write error. Narrow in practice on Unix (reset_sigpipe restores SIG_DFL, so a closed pipe signals rather than panics), but the asymmetry is worth a note or a follow-up.

siloteemu

Reviewed at 294a1832: no blocking issues. Routing the failure and refusal messages to the error stream, and returning the exit code through main() so the logging guard's destructor runs, both check out — and the new scenario drives a real terminal rather than a pipe, so it reaches the interactive-decline branch the piped harness cannot.

Worth noting for the record: this is the one change in a 35-PR sweep that removed a non-discriminating test shape rather than adding one — replacing "does the combined output contain this message" with an assertion that the text is on the error stream and absent from standard output.

Five non-blocking notes are in the review comment. The one worth acting on before or after merge: the common usage error path still bypasses the guard, so the commit subject claims broader coverage than the change delivers.

Renumber the branch's new diagnose scenarios to 15/16; main independently added its own 13/14 (vLLM import recognition, multi-shell plan). Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…path parse_cli() called clap's get_matches(), which self-exits on a parse error and skips the log guard's flush -- the same bug this PR already fixed for fix(). Switch to try_get_matches() and share the print+wrap logic with the mistyped-subcommand branch via a new clap_exit_code helper, and rewrite the round-trip test to exercise that helper on a real clap::Error instead of a hand-built marker. Also give TuiSession callers a way to override HOME/SHELL the way the piped run_rocm_with_env path already can, and use it in diagnose-16 so the interactive-decline scenario points at the same rc file the Given step planted -- plus assert that file stays untouched, matching its piped sibling. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd · 2026-09-10T12:14:37Z

Addressed the non-blocking findings from the latest review round (8649dff):

Fixed — parse_cli() was still calling .get_matches(), which self-exits on a parse error and skips _log_guard's flush, the same bug class this PR fixed for fix(). Switched to .try_get_matches() + Cli::from_arg_matches(), both routed through a new shared clap_exit_code() helper (also used by the mistyped-subcommand branch).
Fixed — the round-trip test hand-built a ClapExitCode instead of exercising the real helper. Rewrote it to drive a real clap::Error through clap_exit_code, and added a second test covering the try_get_matches() path from Let Lemonade auto-select its llama.cpp backend #1.
Left as-is — non-blocking per the review's own text, no code change follows from it.
Fixed — the PTY-driven interactive-decline scenario (diagnose-16) had no way to point HOME/SHELL at the scenario's planted rc file the way the piped sibling does. Added TuiSession::spawn_with_env, wired diagnose-16 to use it, and added the missing "file untouched" assertion.
Left as-is — non-blocking per the review's own text.

Verified with cargo build --workspace, cargo test -p rocm --bin rocm (615 passed), and the e2e suite run directly against the built binary — diagnose-08, -11, -14, -15, -16 all green, including the new rc-untouched step for -16.

jussielo-amd force-pushed the fix/rocm-fix-stderr-and-log-guard branch from bbb1b02 to aa563d8 Compare September 7, 2026 11:09

jussielo-amd requested a balanced review from Copilot September 7, 2026 11:55

Copilot started reviewing on behalf of jussielo-amd September 7, 2026 11:56 View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

Comment thread apps/rocm/src/main.rs Outdated

Comment thread crates/rocm-core/src/fix.rs Outdated

jussielo-amd requested a balanced review from Copilot September 7, 2026 14:44

Copilot started reviewing on behalf of jussielo-amd September 7, 2026 14:44 View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

jussielo-amd marked this pull request as ready for review September 8, 2026 07:08

jussielo-amd requested a review from a team as a code owner September 8, 2026 07:08

jussielo-amd requested review from volen-silo and a balanced review from Copilot September 8, 2026 07:08

Copilot started reviewing on behalf of jussielo-amd September 9, 2026 05:55 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Comment thread apps/rocm/src/main.rs Outdated

Comment thread apps/rocm/src/main.rs Outdated

jussielo-amd added 9 commits September 9, 2026 12:50

jussielo-amd force-pushed the fix/rocm-fix-stderr-and-log-guard branch from d4d1127 to 0cef54d Compare September 9, 2026 12:52

siloteemu approved these changes Sep 10, 2026

View reviewed changes

jussielo-amd added 2 commits September 10, 2026 11:51

Merge origin/main into fix/rocm-fix-stderr-and-log-guard

108a7cb

Renumber the branch's new diagnose scenarios to 15/16; main independently added its own 13/14 (vLLM import recognition, multi-shell plan). Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd added this pull request to the merge queue Sep 10, 2026

Merged via the queue into main with commit 04dd507 Sep 10, 2026
20 checks passed

jussielo-amd deleted the fix/rocm-fix-stderr-and-log-guard branch September 10, 2026 14:29

Navigation Menu

Uh oh!

fix(cli): route rocm fix failures to stderr, preserve log guard on exit - #350

fix(cli): route rocm fix failures to stderr, preserve log guard on exit#350
jussielo-amd merged 12 commits into
mainfrom
fix/rocm-fix-stderr-and-log-guard

jussielo-amd commented Sep 7, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

siloteemu commented Sep 9, 2026

Uh oh!

jussielo-amd commented Sep 10, 2026

Uh oh!

siloteemu commented Sep 10, 2026

Uh oh!

siloteemu left a comment

Uh oh!

jussielo-amd commented Sep 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jussielo-amd commented Sep 7, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

Uh oh!

siloteemu commented Sep 9, 2026

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

jussielo-amd commented Sep 10, 2026

Uh oh!

siloteemu commented Sep 10, 2026

Summary

🚫 Blocking (must fix before merge)

Non-blocking

Uh oh!

siloteemu left a comment

Choose a reason for hiding this comment

Uh oh!

jussielo-amd commented Sep 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants