[jussielo-amd]

Summary

rocm uninstall started the client file logger before it built its plan, and the logger creates <data folder>/logs. On a machine ROCm CLI had never touched, rocm uninstall --dry-run therefore created the data folder and then reported it as the one item to remove, so a dry run was not free of side effects and an empty machine looked installed. It happened with the default folders and with ROCM_CLI_DATA_DIR set alike, and I reproduced it on current main before changing anything.

The uninstall command now skips the file logger (logging::is_enabled_for, consulted in run() before logging::init); every other command logs as before. A real uninstall --yes loses nothing by this: the log it would have written lives in the folder it then deletes, and nothing in the tests or docs depends on an uninstall log line. After the fix a clean home stays empty, the plan says "Nothing managed by rocm-cli would be removed.", and a real uninstall still works.

Fixes #791.

Scope

--help and config show also create the logs folder on a clean machine; that is the logger's designed first-run behavior and I left it alone. An uninstall reached from rocm chat or from a freeform request (rocm --yes "uninstall ...") runs under a logger that was already started, so those paths are unchanged. This does not touch the dry-run footer, the --force suggestion or the help examples from #788.

Tests

Scenario uninstall-06 (uninstall-dry-run-on-an-untouched-machine-creates-nothing) covers the behavior: it runs uninstall --keep-binaries --dry-run on a machine with none of the folders and asserts both that the plan says there is nothing to remove and that no folder exists afterwards. It is not OS-gated and runs on the mock lane on every PR; the When step sets both HOME and USERPROFILE, but I could not run it on Windows. A unit test, uninstall_is_the_only_invocation_that_skips_the_log_file, pins which invocations skip the logger. With the fix disabled the scenario goes red at "nothing would be removed", and a prefix-match mutation turns the unit test red.

README, docs/testing.md and docs/manual-testing.md now say that the dry run creates nothing; the README sentence is qualified with --keep-binaries, because without it a machine with release-installed binaries lists them.

Run locally: cargo xtask e2e -- -n "uninstall-" (6 of 6), cargo test -p rocm --bin rocm logging::, clippy with -D warnings for rocm and e2e-cucumber, and cargo fmt --check. Not run: the full workspace gate and Windows. CI is the authoritative check for the rest.

Draft PRs #569 and #299 also edit uninstall code: expect textual conflicts in main.rs, uninstall.feature and uninstall_steps.rs, and a scenario renumber if #569 merges first.

The logger gate reads the subcommand as argv[1], so a unit test now fails, naming the cause, if the top-level command ever gains an argument, a subcommand gains an uninstall alias or an abbreviated name reaches it. Known gap, out of scope here: maybe_migrate_legacy_dashboard_config() predates this change and still runs for uninstall --dry-run, so a dry run on a host with a legacy dashboard file can still write config.json; it is a different code path from the folders the #791 repro is about, and a follow-up can skip it behind the same gate with its own scenario.

[juhovainio]

Looks good. The fix is at the right layer: the logger no longer creates logs before the uninstall plan is built, so a dry run leaves the state folders alone. The new scenario (uninstall-06) asserts the "nothing to remove" output together with every state folder still being absent, which covers the #791 behavior, and the docs are updated to match.

Two non-blocking notes are left inline (the argv[1] coupling in the logger, and the legacy dashboard config migration that still runs on a dry run). Neither needs to hold this up.

This reflects a read of the diff. On CI, the Strix Halo nightly/release e2e failures are at the GPU model-serving step (Qwen3-0.6B not served within 90s), which looks unrelated to uninstall; worth confirming with a re-run.

[juhovainio]

Non-blocking: the skip decision relies on the subcommand being argv[1]. That holds today (no global flags, no uninstall alias), but a future global flag would silently re-enable the logger, and only the e2e scenario would catch it. A comment next to the clap definition, or deriving this from the parsed command, would make that coupling harder to miss.

[jussielo-amd]

Fixed in 990cc08. The coupling is now guarded by a unit test that fails, and names the cause, when the top-level command gains an argument (a global flag that could come before the subcommand), when any subcommand answers to uninstall as an alias, or when an abbreviated name such as uninstal reaches it. I checked that it goes red for a global flag and for an alias on uninstall. The Cli definition also carries a comment pointing at logging::is_enabled_for and the test. I kept the argv[1] check itself rather than deriving the decision from the parsed command, because the logger has to be started or skipped before parsing.

[juhovainio]

Non-blocking, related: maybe_migrate_legacy_dashboard_config() just below still runs for uninstall --dry-run and writes config.json when a legacy dashboard TOML exists, so a dry run can still have a side effect. This predates the PR and is outside the #791 repro, but it is worth a mention in the scope notes or a follow-up.

[jussielo-amd]

Agreed that this is a real remaining side effect, and I'm leaving it out of this change on purpose: maybe_migrate_legacy_dashboard_config() predates this PR, runs for every command, and writes config.json from a legacy dashboard file rather than creating the folders the #791 repro is about, so fixing it here would widen the change beyond the reported behaviour. I added it to the scope notes in the PR body, so it is on record as a known gap and not an oversight. The simplest fix is to skip the migration for uninstall --dry-run by the same argv gate the logger uses, with its own scenario.