Repository navigation
fix(uninstall): correct the dry-run footer and stop answering --force with --force-dev-binaries - #916
jussielo-amd wants to merge 9 commits into
Conversation
…moval `rocm uninstall` started the client file logger before it planned, 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 thing 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. The `uninstall` command now skips the file logger. Other commands still log as before. Scenario uninstall-06 runs a 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; a unit test pins which invocations skip the logger. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
… with --force-dev-binaries A dry run ended with "Choose Review uninstall to approve removal.", a control the command line does not have, and it printed that even when there was nothing to remove. A person at a terminal now gets "run it again without --dry-run" when there is something to remove, and nothing when there is not. Output that is captured (the dashboard's /uninstall and the daemon's rocm_command tool run the dry run as a subprocess) names no next step, since the real step belongs to whatever drives it. The assistant's preview has no flag to offer, so it says removal needs approval. The three wordings come from one audience value, chosen from whether the command has a terminal, rather than from strings that can drift. A mistyped --force was answered with "a similar argument exists: --force-dev-binaries", which removes the binaries of a development build, and so were near misses of it. clap ranks every declared flag by name, so --force is now declared, hidden, and refused with a pointer to --yes before anything is planned; near misses land on it instead of on the destructive flag, and a typo of the whole flag is still met with that flag. rocm uninstall --help now ends with examples that start with a dry run. Scenarios uninstall-07 to uninstall-11 cover the footer at a terminal (run again as it says, answering the prompt), outside one, with an empty plan, the --force cases, and the help examples. Every uninstall step now refuses to run without --keep-binaries. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
The step that finds the `EXAMPLES:` block in `rocm uninstall --help` used `map(..).unwrap_or_else(..)`, which the workspace's pedantic lints reject, so the clippy job and both Windows build jobs failed to compile the e2e harness. `map_or_else` keeps the same panic message and behavior. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
There was a problem hiding this comment.
🟡 Changes recommended
The source-file citation at docs/testing.md:967 uses a backticked path instead of a relative Markdown link, preventing documentation link checks from detecting a moved or deleted test file.
1 open issue
What changed in this PR
Improves rocm uninstall guidance by tailoring dry-run output to its audience, redirecting mistaken --force usage toward --yes, and adding help examples. Builds on #907’s dry-run logging safeguard.
Changes:
- Adds audience-specific dry-run footers and omits next-step guidance for empty plans or captured output.
- Rejects
--forceand steers near-miss suggestions away from--force-dev-binaries. - Adds regression scenarios, unit tests, and documentation for uninstall behavior.
| File | Description |
|---|---|
| tests/e2e-cucumber/tests/e2e/uninstall_steps.rs | Adds terminal, footer, flag-refusal, and help checks. |
| tests/e2e-cucumber/features/uninstall.feature | Adds scenarios covering dry-run safety and guidance. |
| README.md | Explains dry-run output and confirmation options. |
| docs/testing.md | Documents regression coverage and execution lanes. |
| docs/manual-testing.md | Adds manual checks for footers, flags, and help. |
| apps/rocm/src/uninstall.rs | Selects the plan audience from terminal availability. |
| apps/rocm/src/uninstall_plan.rs | Implements and tests audience-specific footers. |
| apps/rocm/src/main.rs | Adds help examples, hidden --force rejection, and logging gating. |
| apps/rocm/src/logging.rs | Skips file logging for direct uninstall invocations. |
| apps/rocm/src/deletion_properties.rs | Updates rendering tests to specify captured output. |
Review effort: Balanced
| is not answered with `--force-dev-binaries` (`uninstall-10`); the help starts | ||
| its examples with a dry run (`uninstall-11`). The assistant's uninstall preview | ||
| ends differently, since it has no flag to offer: it says removal needs approval, | ||
| which `lifecycle_read_mutate_split_is_honest` in `apps/rocm/src/chat.rs` backs. |
There was a problem hiding this comment.
Fixed in f8e3150. The citation is now a relative Markdown link to the file, so the documentation link check covers it. It was also stale: the test it names moved from chat.rs to tool_policy.rs on main, so I corrected the file name there and in the two code comments in uninstall_plan.rs that pointed at it. I found no other source-file citation added by this PR, and the repository's link check (cargo xtask lychee) passes locally.
Signed-off-by: Jussi Elo <jussi.elo@amd.com> #907, which this branch was stacked on, has merged, and main has since moved the clap command tree and its tests from main.rs into cli.rs and refactored the uninstall confirmation. The resolution keeps main's layout and re-applies this branch's own change on top of it: the hidden `--force` field and the `after_help` examples of `Uninstall`, and the two unit tests that parse the command, now live in cli.rs; the dispatch arm and `reject_uninstall_force` stay in main.rs; and uninstall.rs imports `interactive_terminal` again, because the plan's audience is chosen from it and main's refactor had dropped the import. What remains of the branch against main is only the footer, `--force` and help-examples change.
…a link, from the file it now lives in The testing guide named the test `lifecycle_read_mutate_split_is_honest` with a bare backticked path, which no link check can see go stale, and the path was already stale: the test moved from chat.rs to tool_policy.rs, so the guide and the two code comments that point at it named the wrong file. The guide now cites tool_policy.rs as a relative Markdown link, which the documentation link check resolves, and the comments name the file the test is in. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
I went through the diff against #788 and the changes match the issue: the footer is correct for the CLI and absent on an empty plan, --force and its near misses no longer suggest --force-dev-binaries, and the help examples lead with the dry run. The docs and tests are updated alongside. I found nothing that breaks behavior, but there are a few things I'd tidy before merge, two as inline comments:
- The scenario pins clap's exact "unexpected argument" wording, so a clap upgrade could fail it with no behavior change.
reject_uninstall_forceis a new domain helper inmain.rs; it would sit more naturally next to the uninstall code.- The assistant footer "Removal needs your approval." is backed by a unit test in
tool_policy.rsrather than a scenario. That is acceptable given the cited test, just noting it.
Two questions for the author: the dashboard /uninstall used to show the old "Choose Review uninstall" line and now shows no footer, so does it supply its own next step? And the hidden dummy --force relies on clap not suggesting hidden arguments (the unit test guards this); is that coupling acceptable?
I didn't build or run this locally; CI was still pending on build-and-test, clippy, Windows and coverage when I looked.
| ); | ||
| assert!( | ||
| stderr.contains("unexpected argument '--forc' found"), | ||
| "expected the unknown-argument error on stderr:\n{stderr}" |
There was a problem hiding this comment.
This pins clap's exact wording for the unknown-argument error. Exit code 2 plus the absence of --force-dev-binaries already carry the point; consider matching only unexpected argument so a clap upgrade doesn't break the scenario.
There was a problem hiding this comment.
Done in cf80f3d: the step now matches only unexpected argument, so a clap upgrade that rewords the message no longer breaks the scenario. It still fails on exit code 2 and if --force-dev-binaries is suggested again. Turning the refusal off and bringing the destructive suggestion back both fail it.
| /// such as `--forc` gets that same suggestion. Declared, those guesses land here | ||
| /// instead, before anything is planned or removed. | ||
| fn reject_uninstall_force(force: bool) -> Result<()> { | ||
| if force { |
There was a problem hiding this comment.
This refusal helper adds domain logic to main.rs; it could live beside uninstall in uninstall.rs instead.
There was a problem hiding this comment.
Moved in e058730 to uninstall.rs as reject_force, called from the dispatch arm as uninstall::reject_force. It is in uninstall.rs rather than uninstall_plan.rs because the plan module holds the shared plan types that storage.rs reuses, and this is argument handling for the uninstall command alone. The architecture note names it in its new place.
…ss scenario The scenario asserted clap's exact wording for the unknown-argument error, including the argument it echoes. The exit code 2 and the absence of `--force-dev-binaries` already carry the point, so the step now matches only `unexpected argument`, and a clap upgrade that rewords the message no longer breaks the scenario while it still fails if the destructive flag is suggested again. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…e uninstall command `reject_uninstall_force` was domain logic in main.rs. It now lives in uninstall.rs, which owns `uninstall()`, as `reject_force`; the dispatch arm in main.rs calls it through the qualified `uninstall::reject_force` and the unit tests in cli.rs import it from there. uninstall.rs rather than uninstall_plan.rs, because the plan module holds the shared plan types and file-removal helpers that storage.rs reuses, while this is argument handling for the `uninstall` command alone. The architecture note for uninstall.rs names it. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
…uggestions change The hidden `--force` works because clap's candidates for a near miss are every declared long name, hidden or not, so `--forc` is steered to `--force` and its refusal instead of to `--force-dev-binaries`. If a clap upgrade stopped suggesting hidden arguments, `--forc` would be answered with the destructive flag again. The guard test already failed on the destructive suggestion; it now also requires the near misses to name `--force`, and the two checks together do not depend on clap's wording around the suggestion. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
|
Thanks for the review. The tidy-ups are in cf80f3d, e058730 and 370bba7; answers to the rest:
|
main moved (the chat.rs split and other refactors) and the architecture notes for the therock, tool_policy and update_cmd modules changed there while this branch edited the entry for uninstall.rs. The resolution keeps main's wording for every entry and carries over only this change's entry for uninstall.rs. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
Summary
A dry run of
rocm uninstallended with "Choose Review uninstall to approve removal.", a control the command line does not have, and it printed that line even when there was nothing to remove. A person at a terminal now gets "run it again without --dry-run" when there is something to remove, and nothing when there is not. Output that is captured (the dashboard's/uninstalland the daemon'srocm_commandtool run the dry run as a subprocess) names no next step, since the real step belongs to whatever drives it. The assistant's uninstall preview shares the renderer and has no flag to offer, so it says removal needs approval. The three wordings come from one audience value, chosen from whether the command has a terminal.A mistyped
--forcewas answered with "a similar argument exists:--force-dev-binaries", which removes the binaries of a development build, and so were near misses of it. clap ranks every declared flag by name, so--forceis now declared, hidden, and refused with a pointer to--yesbefore anything is planned. Near misses such as--forcland on it instead of the destructive flag, and a typo of the whole flag is still met with that flag.rocm uninstall --helpnow ends with examples that start with a dry run.Fixes #788.
Tests
Scenarios uninstall-07 to uninstall-11 cover the footer at a terminal (run again as it says, answering the prompt, and assert the folder is gone), outside a terminal, with an empty plan, the
--forcecases, and the help examples. The terminal scenarios use a pseudo-terminal and run on the Linux lane; the others are not OS-gated and run on the mock lane on every PR. Unit tests cover the footer wording per audience, and I checked that each new test and scenario goes red when the behavior it covers is broken.The scenario numbers overlap with draft PR #569, which also adds scenarios to
uninstall.feature, so one of the two will need renumbering when the other lands.Run locally:
cargo xtask e2e -- -n "uninstall-"(11 of 11), therocmunit tests, clippy with-D warningsforrocmande2e-cucumber, andcargo fmt --check. Not run: the full workspace gate, the pre-push hooks, Windows (the pseudo-terminal scenarios are Linux-only), and clippy forrocm-dash-tuiandrocmd, which this change does not edit. CI is the authoritative check for the rest.