docs: copyedit README, CONTRIBUTING, and vLLM; add uninstall and verify guidance

[pmoutsias-amd]

Summary

This PR has sixteen content commits plus two merges of main. Four add content (1, 5, 6, and 9), two add or place cross-reference links (7 and 8), three are copyedit commits (2 to 4), two correct commit 9 (10 and 11), one corrects commits 5 and 6 and the diagnose text (12), one addresses the first automated review (13), one rewords the default-version text (14), one addresses later review comments (15), and one corrects stale install, serve, and cleanup guidance (16). Commits 2 to 4 add no new facts; commits 1, 5, 6, 9, 10, 11, 12, 13, 14, 15, and 16 do, and are described separately below so they can be reviewed apart from the wording changes. The merges of main resolve one conflict in the README.md uninstall section; see the last section.

1. Call out supported ROCm versions (7.14, 10.0, 10.1)

• States that ROCm CLI supports ROCm 7.14, 10.0, and 10.1, and that rocm install sdk currently installs ROCm 7.14 when --version is omitted. The statement appears in the ROCm installation intro and in Getting started (commits 13 and 14 reword it from "default 7.14").
• Explains that ROCm 10.0 or newer needs a --version pin, and may need the exact GPU arch as a raw gfx code through --family, with an example (--version 10.1.0 --family gfx1200). Commit 16 softens this: --family is needed only when the CLI can't detect the GPU arch, and the error names the command to re-run.
• Moves the "ROCm 10 and newer" section from the end of "ROCm installation" to just after "Install location". The new intro sentence points readers to that section, so it now sits next to the other install sdk subsections (--devel, approval prompt, install location) and ahead of the unrelated "Driver installation" and "Updates" sections. This is the only block move in the PR, and it belongs with the new pointer sentence: if the pointer goes, the move should go with it.
• Updates the getting-started.md include anchor to match the reworded README sentence.

2. Copyedit README, CONTRIBUTING, and vLLM guide

• Copyedit-only pass on README.md, CONTRIBUTING.md, and docs/vllm.md, which are single-sourced into the docs site.
• Style and consistency changes: removed em dashes and spaced double hyphens, "instead of" and "through" in place of "rather than" and "via", replaced and/or and word slashes, spelled out TUI on first use in each standalone doc, American spellings, split long paragraphs.
• Doc links in README.md and CONTRIBUTING.md now use absolute GitHub URLs so they resolve on the docs site.
• "WSL" is now "WSL2" in the README heading and live-mode note, matching the platform table.
• No new facts, no restructuring, no docs-site wiring changes.

3. Tighten README flag lists and add lead-ins

• Keeps the rocm diagnose flag list tight (one paragraph per bullet) by moving the long --report and --send detail below the list.
• Adds a colon lead-in to the diagnose, fix, dash, and bench load lists.

4. Rejoin prune lock sentence and complete acceptance-script list

• Rejoins "The same lock runs in the other direction..." to the preceding services prune paragraph, so its reference to the wait is not left dangling.
• Moves the two behaviors that sat outside the vLLM acceptance-script list ("rejects external vLLM command overrides", "does not allow CPU fallback") into the list.

5. Add install verification and rocm uninstall guidance (new content)

• Adds a "Verify the installation" section after the install commands: run rocm version, which prints the CLI version (release tag or branch and commit hash), the ROCm SDK this machine would use, and the GPU driver version, or not detected for the last two when none is found.
• Documents rocm uninstall under "Logs and cleanup": what --dry-run, --yes, and each --keep-* flag do (commit 13 adds the default-layout caveat for the --keep-* flags), and a new --force-dev-binaries entry in the usage block.
• Notes what uninstall warns about before it removes anything: shared caches it leaves alone, managed service records (their processes are not stopped), and remote sessions (run rocm remote stop <session> first).
• Statements were checked against the CLI source in apps/rocm/src/main.rs (version(), build_uninstall_plan(), and the Uninstall flag definitions), not copied from the earlier draft; commit 13 corrects one flag claim that this check missed, which said rocm version showed only the CLI version and that uninstall warned about services "still running".
• Needs a content reviewer to confirm. No site wiring: the new verify section is picked up by the existing include on the installation page.

6. Clarify verify and uninstall guidance (new content)

• Verify section: says what a successful rocm version run looks like, that not detected for the SDK and driver is expected before ROCm or a driver is installed, and what to check if rocm is not found.
• Uninstall: names the four categories it removes (binaries, configuration, data, caches) and lists the managed ROCm SDK installs under --keep-data, since they live in the data directory.
• Uninstall warnings: split into a list (managed service records, remote sessions) and a sentence naming the shared caches it leaves in place (the uv package cache and the Hugging Face model cache; commit 12 refines this). Checked against build_uninstall_plan() and shared_cache_candidates().

7. Link install pages to uninstall guidance

• Adds one sentence in the README "ROCm installation" section and one on the site install page (docs/rocm-docs/install/installation.md) pointing to "Logs and cleanup", so readers can find how to remove ROCm CLI. The site-page sentence is first placed directly under Build from source; commit 8 moves it.

8. Give the install page uninstall pointer its own heading

• On the site install page, moves the uninstall pointer under a new ## Uninstall ROCm CLI heading so it doesn't read as part of Build from source, and keeps the existing Contributing sentence in its original place. This and commit 7 are the only changes in this PR to a site wrapper; neither adds include anchors or alters existing ones.

9. Fix command synopses and document rocm model, rocm chat --provider, and automation modes (new content)

• Synopses: adds --send (requires --report) to rocm diagnose and the positional [QUERY ...] to rocm logs.
• Adds a short "Curated models" section for rocm model [--verbose] (alias rocm models), which the "Will a model run here?" text already referred to, and splits that text's undetermined conditions into a list.
• Chat: replaces the anthropic|openai|... placeholder with local|openai|anthropic, and documents --provider setup (rocm config enable-provider and set-provider-key) and --tools.
• Automations: describes observe, propose, and contained. In contained mode only server-recover acts on its own (restarts a failed managed service); the others stay review-gated or record only, per crates/rocm-core and apps/rocmd/src/watchers.rs. Per-watcher default modes are left out to avoid a table that can drift.
• All checked against apps/rocm/src/main.rs.

10. Clarify that rocm chat --provider applies only to one-prompt use

• In an interactive terminal, rocm chat without --prompt opens the dashboard chat, where you switch providers with /provider <name>. With --prompt or piped input, it sends one prompt and prints the reply.
• Notes that --provider is ignored by the dashboard chat, and that local is the default.
• Checked against the Command::Chat handler in apps/rocm/src/main.rs.

11. Correct contained-mode, chat flag, and model verbose descriptions

• Chat: extends the note to --model and --tools, which the dashboard chat also ignores, and states it once for all three flags.
• Automations: corrects the contained description, which overstated that every watcher applies changes on its own. propose now says it applies to watchers that can make a change.
• rocm model --verbose: describes what it prints (every recipe, including hidden ones, with aliases, data type, minimum GPU memory, and engines) instead of repeating the flag's help text.
• Checked against the Command::Chat handler in apps/rocm/src/main.rs, and against the watcher code in crates/rocm-core and apps/rocmd/src/watchers.rs.

12. Clarify uninstall cache and data scope, add APU case to diagnose

• Uninstall: separates the managed uv cache (under the data directory, controlled by --keep-data) from the shared one (UV_CACHE_DIR or ~/.cache/uv, left in place and reported). Lists downloaded models under --keep-data, since they live in the data directory and uninstall removes them.
• Diagnose: the undetermined conditions now include the APU case, where the CLI can't yet read the memory pool the engine uses. Checked against UndeterminedReason in model_readiness.rs.
• Audit of the rest of the new text against source found no other discrepancies.

13. Address automated review: uninstall default layout, ROCm versions, synopses

• Uninstall: on the default layout the config and data directories are both ~/.rocm and the cache is ~/.rocm/cache, so a single --keep-* flag doesn't protect its files. The intro now says each flag "skips the removal of one category", and a new paragraph says to pass --keep-config and --keep-data together and to run --dry-run first. docs/engine-plugins.md gets the same caveat. The earlier "checked against build_uninstall_plan()" claim had verified each flag in isolation, not the overlapping default layout. The behavior itself is tracked in uninstall: --keep-* flags are ignored when config, data, and cache directories overlap (default layout) #626.
• ROCm versions: reworded as a support statement ("supports ROCm 7.14, 10.0, and 10.1") instead of a hardcoded default. Source has no allow-list or default version. Commit 14 replaces the "latest stable release, currently 7.14" wording, which was wrong because 10.1 is the latest stable release.
• Disk space: the "never removed" claim is narrowed to rocm storage, because rocm uninstall does remove downloaded models along with the data directory (see the Storage section for the --keep-* caveat).
• Synopses: rocm diagnose [--symptom TEXT] [--top N] [--json] [--distro [NAME] | --report [--send]] and rocm storage [report [--json]]. The undetermined list now points to rocm model --verbose.
• getting-started.md: resyncs the hand-copied sentence with the README.

14. Avoid calling 7.14 the latest stable release

• ROCm 10.1 is the latest stable release, so the README no longer describes the unpinned result as the "latest stable release". It now says rocm install sdk "currently installs ROCm 7.14" without --version, and that 10.0 or 10.1 need --version and --family.

15. Address review comments on getting started, uninstall, and vLLM

• Getting started: "Configure ROCm and serve a model" now starts with rocm examine and the active_runtime_status: ready check, and runs rocm install sdk only to install a runtime or get a different version or configuration.
• Installation page: the uninstall pointer now shows a rocm uninstall example (with --dry-run) and points to "Logs and cleanup".
• vLLM guide: reworded the torch-release sentence and split a long sentence.
• Re-edits the getting-started.md include anchor to match the README.
• Not in this PR: the WSL page wiring (tracked for a future PR) and the --keep-* help-text mismatch (tracked in uninstall: --keep-* flags are ignored when config, data, and cache directories overlap (default layout) #626).

16. Correct stale install, serve, and cleanup guidance

• README: the rocm: command not found advice now covers Windows as well as Linux (~/.local/bin, %USERPROFILE%\.local\bin, or ROCM_CLI_INSTALL_DIR), checked against install.sh and install.ps1.
• README synopses: rocm fix gains --json (valid only without a fix id), rocm engines install gains --yes, and rocm serve gains --require-api-key, --api-key, --gpu-memory-utilization, and --tool-call-parser, with short explanations. Prefer ROCM_SERVE_API_KEY over --api-key.
• docs/vllm.md: the pin-table sentence is superseded in commit 17.
• --help text in apps/rocm/src/main.rs: rocm chat and rocm diagnose --send now describe current behavior.
• The only Rust change is --help doc-comment text. It was not compiled or run (no Rust toolchain on the authoring machine), so CI is the first check.

17. Align vLLM, uninstall, and help wording with the code

• docs/vllm.md: the pin-table sentence now reads "For every ROCm SDK version outside a discovery major (today, everything but 10.x)". Any version without a matching row falls back to the table's default row, so "7.x" was too narrow and contradicted the later "Every other ROCm SDK version" line.
• MANIFEST.md: the uv-cache note no longer says --keep-data alone keeps it. On the default layout, keeping it takes --keep-data and --keep-config together.
• --send help in apps/rocm/src/main.rs: "read a form" is now "read and send a mail".
• Reverts the apps/rocm/src/storage.rs wording change and its loosened test, so this PR stays docs-only. The rocm storage output wording ("never removed by ROCm CLI" is not accurate, because rocm uninstall removes downloaded models with the data directory) is not changed here and needs a separate change with a full-string test.

Merges of main

• The merge of main conflicted in the README "Logs and cleanup" section. main had added two paragraphs after the rocm uninstall synopsis (symlinked config, data, and cache directories on Linux; the error when uninstall cannot check whether a directory exists). Both are now carried into the four-category uninstall section.
• Also corrects two claims the reviews flagged: the Storage section now says keeping downloaded models on a default installation takes both --keep-data and --keep-config, and --force-dev-binaries is described as removing the ROCm CLI binaries in that build directory.

Dependencies on code changes

• Uninstall removing the running rocm.exe on Windows and undoing the installer's PATH change is a CLI fix in fix(uninstall): undo the installer's PATH change and remove the running exe #636, so this PR doesn't document a manual workaround. The "removes its binaries" wording is accurate once fix(uninstall): undo the installer's PATH change and remove the running exe #636 lands.
• The ROCm 10.0 --version resolution (torchaudio pinned to the requested build) is fixed in fix(therock): pin torchaudio to the requested ROCm 10 build #630, so the README 10.0 wording is correct once fix(therock): pin torchaudio to the requested ROCm 10 build #630 lands.

Test plan

• All 14 {include} directives in docs/rocm-docs/ still find their :start-after: and :end-before: strings, in order, against the edited files.
• Code fences and tables are balanced, intra-doc anchors resolve, and no flags, numbers, or versions were added or removed by the copyedit commits (commits 2 to 4).
• All commits are signed and carry a DCO Signed-off-by trailer.
• New technical claims in commits 5, 6, 9, 10, 11, and 16 were checked by reading the CLI source. The binary was not built or run.
• The docs example rocm install sdk --version 10.1.0 --family gfx1200 --dry-run resolves on WSL2 (run 2026-10-09): ROCm 10.1.0, torch 2.14.0, torchvision 0.28.0, and torchaudio 2.11.0.3, all +rocm10.1.0, with no error. A review dry-run the same day showed an unpinned install picking 7.14.1. The dry-run on WSL2 did not use a gfx1200 GPU. A review dry-run of --version 10.0.0 --family gfx1200 failed on a torchaudio mismatch (it selected the 10.1.0 build); fix(therock): pin torchaudio to the requested ROCm 10 build #630 fixes that resolution.
• Content reviewer confirms three claims: rocm uninstall warns about remote sessions, contained mode acts on its own only for server-recover, and the dashboard chat ignores --model and --tools.
• docs-build passes in CI.
• Spot-check the rendered getting-started, installation, and vLLM pages.

[rominf]

🔴 Automated review · pr-review-watcher · cf49378

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

Review — needs work

Full review of the whole change.
No ticket named. The description lists twelve commits and covers what the diff does. One of its claims is wrong: it says the uninstall text was "checked against build_uninstall_plan()", and the first blocking finding below shows it was not.

This is the first automated round on this PR, so every finding below is new.

Blocking

• (new) On a default install, the new rocm uninstall text promises that each --keep-* flag keeps its category. It does not. — README.md:924-934; same claim at docs/engine-plugins.md:98; root cause apps/rocm/src/main.rs:21012 (build_uninstall_plan) with crates/rocm-core/src/runtime.rs:215-227

  • By default the config dir and the data dir are both ~/.rocm, and the cache dir is ~/.rocm/cache. The plan deletes each category's directory unless that category's flag is passed.
  • I ran rocm uninstall --dry-run --keep-binaries with a throwaway HOME:
    • --keep-data still plans config: ~/.rocm, which wipes the models, managed SDK installs and uv-cache the README says it keeps.
    • --keep-config still plans data: ~/.rocm, which contains the config.
    • --keep-cache still plans config: ~/.rocm, which contains cache/.
  • "Each --keep-* flag leaves one category in place" is therefore false on the default layout. A user who trusts --keep-data loses every managed runtime and downloaded model.
  • The code decides what actually happens, so the text is the side that is wrong today. The bug underneath is in the code.
  • Confidence 100 · logic · Fix: either make the plan honour a kept category when directories overlap (skip deleting a directory that contains or equals a kept one), or document that on the default layout these flags only work in combination. The code fix is the better one.

• (new) The new ROCm version claims state a moving value as fixed and give a list of installable versions that is wrong today. — README.md:228, README.md:395 (README.md:228 is also included into docs/rocm-docs/getting-started.md)

  • The source has no allow-list of versions. Without --version, therock.rs installs whatever the release index currently lists as latest.
  • I ran dry-runs against the live public index:
    • install sdk --family gfx120X-all --version 7.13.0 --dry-run resolves 7.13.0, which "It can install ROCm 7.14, 10.0, or 10.1" excludes.
    • --version 10.0.0 --family gfx1200 --dry-run fails ("selected torchaudio 2.11.0.3+rocm10.1.0, which does not share ROCm build 10.0.0").
    • The default today picks 7.14.1.
  • "By default … installs ROCm 7.14" becomes wrong at the next release, and install sdk --help says nothing about a default version.
  • Confidence 85 (run-confirmed, but it depends on what the index serves today) · logic · Fix: say "the latest stable release (currently 7.14)". If 7.14, 10.0 and 10.1 are a support statement, call them "supported" rather than "can install", and recheck that 10.0 actually works before listing it.

Non-blocking

• (new) The README now contradicts itself about downloaded models. — README.md:557-562 against README.md:931-932

  • The new uninstall text says --keep-data keeps downloaded models, which is correct: they sit under <data>/models, and uninstall deletes data_dir.
  • The reworded Disk space paragraph, and rocm storage's "Shared with other tools (never removed by ROCm CLI)" heading (apps/rocm/src/storage.rs:508-514), still say they are never removed.
  • The code decides this, so the Disk space sentence and the storage label are the ones to fix. That claim was there before this PR.
  • Confidence 88 · mechanical · Fix: limit "never removed" to the uv and Hugging Face caches, or to rocm storage's commands.

• (new) Two --help texts now contradict the README this PR corrected (AGENTS.md §5: check the same claim on every surface). — apps/rocm/src/main.rs:299-302, apps/rocm/src/main.rs:185-195

  • rocm chat --help still says it "Reads the prompt from --prompt or, if omitted, from standard input". The README now correctly says an interactive run without --prompt opens the dashboard chat (main.rs:2226).
  • rocm diagnose --help describes --send as a "prefilled issue form" that "reaches the tracker". The README correctly describes a prefilled mail (report_delivery.rs).
  • Both were stale before this PR.
  • Confidence 85 · mechanical · Fix: update both doc comments to match the README.

• (new) The pointer from diagnose --model's undetermined list to rocm model is incomplete. — README.md:594-595

  • The new Curated models section (README.md:607-610) says the default list hides some recipes, and only --verbose shows them.
  • assess_model_readiness matches against every recipe, hidden ones included.
  • Confidence 75 · mechanical · Fix: point at rocm model --verbose.

• (new) Synopsis errors that predate this PR survive a commit titled "fix command synopses". All confirmed against the built binary's --help or by a run.

  • README.md:533, rocm storage [report] [--json]: rocm storage --json fails with "unexpected argument '--json'". It should read rocm storage [report [--json]].
  • README.md:298: the edited [--distro [NAME]] [--report [--send]] implies the two can be combined, but the CLI rejects --report with --distro.
  • README.md:617-626: the rocm serve synopsis omits --gpu-memory-utilization, --tool-call-parser, --api-key and --require-api-key.
  • README.md:568: the rocm engines install synopsis omits --yes.
  • Confidence 100 · mechanical · Fix: correct the synopses, or narrow what the commit claims to fix.

• (new) The hand-copied sentence on the docs site was not updated with its README original. — docs/rocm-docs/getting-started.md:35-37

  • The README now reads "GGUF versus safetensors rule, because which…", but the copy still says "GGUF-vs-safetensors rule, since which…".
  • The include anchor is unaffected, so this is wording drift only.
  • Confidence 85 · mechanical · Fix: copy the new wording over.

Decisions for the author

• Absolute blob/main links instead of relative ones — tradeoff
  • In favour: CONTRIBUTING.md and the README "More docs" list are pulled into the Sphinx site, where relative docs/… links would break. The files already use this pattern.
  • Against: those links are no longer checked by the offline docs-links (lychee) job. On a versioned docs site they always point at main, not the release being read.

Positive signals

• Every {include} slice in docs/rocm-docs/ still resolves to a unique, intended span. The only changes against the base are the intended content. A local sphinx-build -W passed, and CI's Sphinx and lychee jobs are green.
• The new undetermined bullets, including the APU case, match UndeterminedReason::UnifiedMemoryUnreadable in model_readiness.rs exactly.
• The rocm chat text now describes the code's real split between the interactive dashboard chat and the one-prompt form (main.rs:2226).
• The docs/vllm.md restructure keeps every qualifier and condition. Each technical claim in it was confirmed against engines/vllm/src/install.rs, and the (0, 1] range check was confirmed by a run.

Deployment notes

None

What this covered

• Read: every changed file in full, against 2c37f5bc…cf49378a (base merge-base 7f72556d → head), using the PR's three-dot diff. That is README.md, CONTRIBUTING.md, docs/vllm.md, docs/rocm-docs/getting-started.md and docs/rocm-docs/install/installation.md.
• Also read: every docs/rocm-docs include wrapper, and the CLI source behind each new claim (main.rs, therock.rs, storage.rs, model_readiness.rs, runtime.rs, apps/rocmd/src/watchers.rs, engines/vllm/src/install.rs).
• Ran: the CLI built from this head, for --help and dry-runs, with throwaway HOME dirs. CI status was read for head cf49378: all checks green, GPU lanes skipped as docs-only. The --keep-data dry-run in the first blocking finding was re-run independently before filing.
• Fan-out: five independent workers. Three covered disjoint README ranges plus include integrity, one covered CONTRIBUTING.md and docs/vllm.md, and one ran agent-instruction adherence, history, code comments, prior-PR review comments (#473, #396, #311, #579) and commit quality.
• Did not run: the version findings depend on the live package index on 2026-10-09.
• Not reconciled against any discussion on this PR.

[randyh62]

Maybe this could simply have a rocm uninstall command example, and then point to Logs and Cleanup?

[randyh62]

This starts with installing a rocm sdk, but shouldn't it start by checking if the needed sdk is available using rocm examine? Then if you need a different version of configuration you would run rocm install sdk?

[randyh62]

Suggested change

	first, because the new install takes over as the active default; see
	Running _rocm install sdk_ when a managed runtime is already active prompts for confirmation first, because the new install takes over as the active default; see

[randyh62]

Suggested change

	moved to the SDK's *build* of the torch *release* that the engine pins: the release

[randyh62]

Is it most SDK versions or 7.14?

[randyh62]

Maybe break this sentence into two or three sentences?

[rominf]

🔴 Automated review · pr-review-watcher · 5ed97f6

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

Review — needs work

Full review of the whole change.
No ticket named. The description refers to #626, which tracks the --keep-* default-layout behaviour; it is not a closing claim.

This is the second automated round. Findings are marked new (first raised this round) or standing (carried from the round at cf49378).

Blocking

• (standing, part still holds) The README still tells users ROCm 10.0 installs with --version and --family, and that path fails today — README.md:237-240 (included into docs/rocm-docs/getting-started.md), README.md:405-407

  • This round's blind review did not re-find it. It was settled separately by putting the objection to the reviewer after its report was returned, so the verdict below is anchored, not blind.
  • Answered since cf49378: the list is now worded as a support statement ("supports ROCm 7.14, 10.0, and 10.1"), and the default reads "currently installs ROCm 7.14".
  • Still holds: both paragraphs say 10.0 is installed by adding --version and --family. The PR's own test plan records that --version 10.0.0 --family gfx1200 --dry-run failed on a torchaudio mismatch (it selected the +rocm10.1.0 build). The code refuses mixed builds at that point (apps/rocm/src/therock.rs:3496), and nothing in this diff changes resolution. A user who follows the 10.0 instructions gets an error.
  • Not re-run this round: no binary built at this head was run against the live index, so whether the index has since gained a matching 10.0 torchaudio is unconfirmed.
  • Fix: drop 10.0 from the "install with --version and --family" sentences until the 10.0 path resolves, or confirm on a current dry run that it now does and say so in the test plan.

• (new) The new uninstall docs say rocm uninstall removes ROCm CLI's binaries, but on Windows it leaves the running rocm.exe in place. On every platform it also leaves the PATH change the installer made — docs/rocm-docs/install/installation.md:34-43, README.md:937-940
  On Windows, build_uninstall_plan skips the running executable: apps/rocm/src/main.rs:17853-17858 ("skipping running executable on Windows: …"). Nothing in the uninstall path reverses the PATH edits made by install.sh (shell profile line) or install.ps1 (user PATH). The installation page says "To remove ROCm CLI and what it manages: rocm uninstall", and the README lists "its binaries" among the four categories removed. Windows is a supported platform, so a Windows user who follows the page still has rocm.exe and a dangling PATH entry, and nothing in the docs tells them. The code is what governs here; the docs describe it incompletely.
  Confidence 85 · logic · Fix: add a sentence covering both. On Windows, delete rocm.exe yourself after uninstall finishes. On any platform, remove the PATH line or entry that the installer added.

• (new) The PR description misstates the change's scope — PR #616 description
  It says "This PR has fourteen commits" and describes items 1–14. The branch has 16 commits. The final one (5ed97f6) adds content the description never mentions:

  • "Configure ROCm" now starts with rocm examine and the active_runtime_status: ready check.
  • installation.md gains the rocm uninstall / --dry-run example.
  • docs/vllm.md:66 gains a new factual claim, "For ROCm SDK 7.x versions, including 7.14".

  Readers rely on the description's split between "adds facts" and "copyedit only" to decide what to check, and this content falls outside both lists. With squash merges the description does not land in history, but it is the map used for this review round.
  Confidence 100 · mechanical · Fix: update the description to cover the last commit and the merge (narrowing is not an option, because the work is already in the diff).

On the merge gate: this change request is held on the standing 10.0 item above. The two new items are listed as the review rated them, but neither alone would have been filed as a change request: rocm uninstall itself prints the skipped Windows executable under "Left alone", and the description does not land in the squash commit.

Non-blocking

• (new) --family is presented as required for ROCm 10.x, but the code only needs an exact arch, and host detection supplies one — README.md:238-240, README.md:405-407
resolve_family returns raw_arch: Some(..) from detect_host_gfx_target() when no --family is passed (apps/rocm/src/therock.rs:3390-3399). select_source_layout bails only when raw_arch.is_none() (therock.rs:314). The advice works but overstates the requirement. This repeats the existing "two things together" wording at README.md:467 in the more prominent getting-started path.
  Confidence 85 · logic · Fix: "add --version; pass --family <gfx code> only when rocm examine shows no detected_gfx_target, or when you target a different GPU."

• (new) docs/vllm.md:66 "including 7.14" suggests a 7.14-specific pin. There is none: 7.14 falls through to the 7.2.3 default row — docs/vllm.md:66-67
VLLM_ROCM_BUILD_TABLE has a single row, 7.2.3 → vllm 0.26.0 / rocm723 (engines/vllm/src/install.rs:36-40). Every version with no row and no discovery match gets that row (install.rs:1184-1207). The old wording, "most ROCm SDK versions", matched the code better.
  Confidence 85 · logic · Fix: restore "most", or state that 7.14 uses the default 0.26.0+rocm723 pin.

• (new) The storage section now says rocm uninstall removes "downloaded models". The CLI's own printed note for that row still says "ROCm CLI never removes them", and the README calls the row "shared with other tools" — README.md:570-575, README.md:944-945, apps/rocm/src/storage.rs:508-515
uninstall removes data_dir wholesale (main.rs:17867-17880), so the README's uninstall statement is correct. The printed string is false, and AGENTS.md §5 asks that the same claim agree across README and printed output. Also, the only thing rocm-cli writes under <data>/models is artifact marker JSON (crates/rocm-core/src/lib.rs:3198-3207), while model weights live in the Hugging Face cache, which uninstall leaves alone. So "--keep-data keeps downloaded models" promises more than that directory holds. The printed string predates this PR; the PR documents around it.
  Confidence 70 · logic · Fix: describe the row as what it actually holds, and correct the storage.rs note in this or a follow-up change.

• (new) The rocm chat --help text still describes only a one-shot, stdin-reading command. The README now correctly documents the interactive dash routing — apps/rocm/src/main.rs:312-316, apps/rocm/src/main.rs:328, against README.md:839-854
  The handler opens the dash chat when the terminal is interactive and --prompt is absent (main.rs:2240-2260). AGENTS.md §5 asks that README and --help claims be kept in sync. The help text drift predates this PR.
  Confidence 85 · mechanical · Fix: update the Chat doc comment and the --prompt help.

• (new) Commit messages that will land in the squash body contain review-process notes — commits 5ed97f6, 633629d, 59b3d9f
5ed97f6 has the subject "address Randy's review comments…", and its body ends with a first-person note to a reviewer: "Review comment #1 (the WSL page) is not in this commit. I removed the --keep-* caveat and the inlined qwen copy on purpose, so the message doesn't mention them." That note refers to changes that are not in the diff. The merge commit's body describes conflict resolution. Squash merges build the commit body from these messages.
  Confidence 100 · mechanical · Fix: reword 5ed97f6, or write the squash message by hand at merge time.

Decisions for the author

• The hardcoded "supports ROCm 7.14, 10.0, and 10.1" and "currently installs ROCm 7.14" (README.md:237-238, README.md:405-406) — tradeoff
  Readers get a concrete answer. Against that: the code has no allow-list or default version. Unpinned selection is "highest stable with a matching torch stack" (therock.rs select_rocm_version / channel_rocm_candidates), so the sentence goes stale silently. It is stated twice, and it does not cover --channel nightly or --format tarball. By the description's own account, the 7.14 figure comes from a dry run, not from source. Confirm it with the CLI owners, or qualify it ("on the default release channel, as of this release").
• Absolute github.com/ROCm/rocm-cli/blob/main/... links in README and CONTRIBUTING — tradeoff
  AGENTS.md sanctions them for files the docs site includes, and they resolve on the site. However, they always point at main rather than the revision being read, and the offline lychee check no longer validates them.
• One PR combines a copyedit pass with substantive new content across 16 commits — non-blocking-improvement
  AGENTS.md §11 asks for one logical change per PR. The description tries to separate the two kinds of change, but the content commits would be easier to review and revert on their own.

Positive signals

• The new "Verify the installation" section matches version() exactly, including the not detected lines for the SDK and driver.
• The --keep-* paragraph warns that on the default layout a single flag protects nothing, which matches build_uninstall_plan. docs/engine-plugins.md carries the same caveat.
• The comment in getting-started.md says that the hand-copied sentence and the reused :start-after: anchor must be edited together. The new anchors (Running \rocm install sdk` when a, newer requirements.`) are unique in README.

Deployment notes

None

What this covered

I read every changed file completely: README.md, CONTRIBUTING.md, docs/vllm.md, docs/engine-plugins.md, docs/rocm-docs/getting-started.md and docs/rocm-docs/install/installation.md. I also checked the behavioural claims against the implementing code (clap definitions, therock.rs, uninstall plan, storage.rs, watchers.rs, the vLLM install table) and against the docs-site include directives. The diff reviewed was prw-base...5ed97f69.

Ran: lychee --offline on the changed Markdown (0 errors; the absolute links are excluded from that check). The CI rollup for this head is all green or skipped, including the Sphinx docs build, lychee, the commit-signature and DCO check, and the e2e tests.

Not run, or not run independently:

• The prior-changes pass did not run, because review comments on earlier PRs touching these files were not read.
• The design-review step (Step 7) ran in the driver context, not as independent workers.
• No CLI binary was built or run. Claims were verified by reading the code.
• sphinx-build was not run locally; I relied on the CI result.
• The 7.14 default could not be confirmed from source, because it depends on the package index.

The four code and source-pass workers were independent, run on a cheaper model tier. This report was not reconciled against the PR discussion.

Standing objections from cf49378, settled after the review by putting each to the reviewer (anchored evidence, not blind findings):

• --keep-* on the default layout: cleared. README and docs/engine-plugins.md now say a single flag does not protect its files on the default layout and give the combination that does; that advice matches build_uninstall_plan (traced, not run). The behaviour itself is unchanged and tracked in #626. The --keep-* --help texts (apps/rocm/src/main.rs:619-628), which this PR does not touch, still describe each flag as self-sufficient.
• ROCm versions: partly answered; the 10.0 part still holds (above).

[rominf]

On the uninstall finding in my review above (rocm.exe left on Windows, PATH change left everywhere): this is a CLI bug, not a docs gap, so please don't add the manual workaround the review suggested. #636 fixes it in code:

• rocm uninstall now removes the running rocm.exe on Windows.
• It undoes the PATH change the installer recorded making, that is, the marked shell-profile block or the Windows user PATH entry.

Once #636 lands, the "removes its binaries" wording here is accurate. Two cases where the PATH change is intentionally kept, in case you want to mention them:

• when other programs share the install folder (often ~/.local/bin), and the review names them;
• with --keep-binaries.

[siloteemu]

🔴 Automated review · pr-review-watcher · 5ed97f6

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

Review — needs work

Full review of the whole change (prw-base...5ed97f69, 15 non-merge commits, 6 documentation files).
The description says the PR is "fourteen commits" and covers ROCm version support, install verification, uninstall guidance, command synopses and chat/automation text. No ticket is named.

Blocking

• [new] The branch conflicts with the base in README.md's "Logs and cleanup" section. README.md:930-960
  A merge-tree of prw-base and HEAD reports CONFLICT (content): Merge conflict in README.md, and GitHub reports mergeable: CONFLICTING, mergeStateStatus: DIRTY. A change that landed on the base after this branch last merged it added two paragraphs right after the rocm uninstall synopsis. One covers symlinked config, data and cache directories on Linux. The other covers the error when uninstall cannot check whether a directory exists. This PR rewrites exactly that spot. Whoever resolves the conflict must carry those paragraphs into the new four-category structure, or that fix's documentation is lost.
  Confidence 100 · logic · Fix: merge the base again and fold the two paragraphs into the new uninstall section.

• [new] The storage section says --keep-data alone keeps downloaded models, which the PR's own uninstall caveat says is false. README.md:573-575
  New text: "rocm uninstall removes downloaded models along with the data directory unless you pass --keep-data". The PR's uninstall section (README.md:949-953) and its docs/engine-plugins.md:98-101 edit say that on the default layout config and data are both ~/.rocm, so one flag does not protect its files and both flags are needed.
  The code governs here, and it agrees with the caveat. build_uninstall_plan (apps/rocm/src/main.rs:17868-17882) queues the config, data and cache directories independently. --keep-data alone still queues config_dir, which is the same directory as data_dir on the default layout, and models live under it. I traced this and did not run it.
  Confidence 85 · logic · Fix: add "and --keep-config on the default layout", or link to the caveat instead of restating a shorter rule.

• [new] The description does not account for the last commit, and its commit count is wrong. PR description, Summary; docs/rocm-docs/install/installation.md:34-43, README.md:215-256, docs/rocm-docs/getting-started.md, docs/vllm.md:66-67
  The description says "fourteen commits" and describes commits 1 to 14. The branch has 15 non-merge commits. The last, 5ed97f6 ("address Randy's review comments on getting started, uninstall, and vLLM"), is not in the description. It changes things the description does not mention:

  • the opening of "Configure ROCm and serve a model", which now starts with rocm examine keyed on active_runtime_status: ready;
  • a new rocm uninstall example and a --dry-run note on the install page;
  • the "7.x, including 7.14" and "torch release" wording in docs/vllm.md;
  • a re-edit of the getting-started.md include anchor.
    Unmentioned changes are unreviewed changes, so this mismatch blocks.
    Confidence 85 · mechanical · Fix: add a section for that commit and correct the count. If the PR is squash-merged, the squash message needs the same content.

Non-blocking

• [new] The ROCm version statement is hardcoded and not derived from code. README.md:237-238 and README.md:405-407, repeated in the rendered getting-started page.
  "ROCm CLI supports ROCm 7.14, 10.0, and 10.1. Without --version, rocm install sdk currently installs ROCm 7.14."

  • With no --version, the code picks the newest build on the selected channel's index (apps/rocm/src/therock.rs:3421-3427). Nothing enforces "supports 7.14, 10.0 and 10.1": the version check rejects only empty input (therock.rs:138-151).
  • "currently installs 7.14" ignores --channel nightly, which is in the synopsis directly above it.
  • The PR description itself says the 7.14 default "comes from an automated review's dry run, not from source" and needs confirmation. The doc rules list version numbers that will change as a maintenance liability, and this one is stated in two places.
    Confidence 75 · logic · Fix: say once that the default is the newest build on the selected channel and point to rocm install sdk --dry-run, which prints selected_rocm_version:. If the three versions are a policy statement, label them as the validated set.

• [new] --family is presented as required for ROCm 10, but the code detects the arch from the host. README.md:238-240
resolve_family fills raw_arch from detect_host_gfx_target() when --family is omitted (apps/rocm/src/therock.rs:3394-3401). The refusal at therock.rs:314 fires only when raw_arch is None. The steps given work, but they overstate the requirement.
  Confidence 75 · logic · Fix: "add --version; if the host GPU is not detected, also pass --family <exact gfx arch>".

• [new] --force-dev-binaries is described as removing "the debug and release binaries beside it". README.md:955-957
is_dev_binary_layout matches a parent directory named debug or release under target (apps/rocm/src/main.rs:18103-18118). The plan then removes only the binaries in the directory the running rocm sits in (collect_installed_binary_candidates, main.rs:18068-18090). target/debug is untouched when you run from target/release.
  Confidence 75 · logic · Fix: "remove the ROCm CLI binaries in that build directory".

• [new] "removes … its binaries" is not fully true on Windows. README.md:937-942
  On Windows the running executable is skipped (apps/rocm/src/main.rs:17853-17859) and listed under "Left alone". The new section never says so.
  Confidence 75 · logic · Fix: add a one-line Windows note.

• [new] The "Verify the installation" failure advice is POSIX-only. README.md:110-125, included in docs/rocm-docs/install/installation.md
  "If the shell reports rocm: command not found, confirm that ~/.local/bin is on your PATH" is in a bash block. On Windows the installer uses %USERPROFILE%\.local\bin (install.ps1:568-574) and PowerShell words the error differently. Both installers also honour ROCM_CLI_INSTALL_DIR, which the note ignores.
  Confidence 75 · mechanical · Fix: add the Windows form and mention ROCM_CLI_INSTALL_DIR.

• [new] The new "rocm fix takes these flags:" lead-in introduces an incomplete list. README.md:306, README.md:382-388
rocm fix --json exists (apps/rocm/src/main.rs:237-239) but appears in neither the synopsis nor the list.
  Confidence 70 · mechanical · Fix: add [--json] and a bullet.

• [new] Commit 5ed97f6's message carries a meta note and a reviewer's first name. git log body of 5ed97f6
  The body says "Review comment #1 (the WSL page) is not in this commit. I removed the --keep-* caveat and the inlined qwen copy on purpose…". That describes edits the commit does not contain and reads as a note about the review exchange. It would reach main's history in a squash. The subject also names a reviewer by first name. Neither is an internal system, so this is noise rather than a rule violation.
  Confidence 70 · mechanical · Fix: drop the sentence and the name when writing the squash message.

Decisions for the author

• Hardcoded version numbers in user docs — tradeoff
  Naming 7.14, 10.0 and 10.1 helps a reader who wants to know what works today. It also needs a manual edit whenever the index moves, and it sits in two places. A pointer to rocm install sdk --dry-run stays correct on its own but tells the reader less. Confirm which you want.

• Absolute github.com/.../blob/main/... links in README.md and CONTRIBUTING.md — tradeoff
  AGENTS.md section 5 exempts both files from the relative-link rule because they are included into a site built with -W, so this complies, and every target exists at HEAD. The cost is that on a fork or a release branch the links point at upstream main, and the offline link checkers do not verify them. This matches the pre-existing style of the other absolute links in CONTRIBUTING.md.

Positive signals

• The {include} contract held. All 14 :start-after:/:end-before: anchors in docs/rocm-docs/ still resolve once, in order, and the hand-copied sentence in getting-started.md is resynced with its HTML comment updated to describe the new "end of the sentence" mechanism.
• The docs/engine-plugins.md uninstall guidance now says --keep-data --keep-config and explains why. Under the old --keep-data-only advice ~/.rocm was deleted anyway. It is consistent with the README caveat and matches build_uninstall_plan.
• The Chat section now documents the real interactive versus one-prompt split and which flags each form ignores, matching the handler at apps/rocm/src/main.rs:2229-2290.
• README.md:607-608 points readers to rocm model --verbose. The default view hides recipes that diagnose --model still matches.
• The new rocm diagnose synopsis [--distro [NAME] | --report [--send]] matches the clap conflicts_with/requires constraints.

Deployment notes

None. The PR is documentation-only and alters no behaviour. AGENTS.md section 3's regression-test and Gherkin requirements therefore do not apply, but the PR text should say that.

What this covered

Every changed file was read completely at HEAD (CONTRIBUTING.md, README.md, docs/engine-plugins.md, docs/rocm-docs/getting-started.md, docs/rocm-docs/install/installation.md, docs/vllm.md), and the diff against prw-base. Claims were checked against apps/rocm, apps/rocmd, crates/rocm-core, engines/vllm, install.sh, install.ps1 and the local prw-base history. Five read-only workers covered the file groups plus the source-reading passes. The conflict, the --keep-data/models contradiction, the --force-dev-binaries scope and the PR description's commit count were re-verified directly. The other code-backed claims in the findings above are the workers' reads, each cited to a line.

Checks at this head: the build, test, clippy, docs-build (-W), link check, commit-signature and prek jobs passed. The E2E lanes (MI300X, Strix Halo Ubuntu, Windows, WSL2), both consolidated reports, skill checks, the Linux PR binaries job and the containerised remote-control job were SKIPPED rather than passed. For a documentation-only change that is expected, but a skipped check is not a pass.

Not run: no build or test run, because the change is Markdown-only and no finding needed one. The mutation check on tests does not apply because the PR adds none. The history and prior-changes passes ran. The conflict finding came from the history pass.

Not reconciled against any discussion here. The PR's comments and earlier reviews were deliberately not read.

[rominf]

🔴 Automated review · pr-review-watcher · aeed0c8

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

Review — needs work

Full review of the whole change.
No ticket is named as the change's goal. The description names #626, #630 and #636 as follow-up or dependency issues. All three are open, and #630 and #636 are unmerged PRs.

This is the third automated round. The review point from the previous round (5ed97f6) is no longer in the branch, so this round reviewed the whole change again. Findings are marked new (first raised this round) or standing (raised in an earlier round and still holding).

Blocking

• (standing in part, re-found this round) The README states as current behaviour two things that are only true once the open PRs #630 and #636 merge, and this PR is not a draft — README.md:239-243, README.md:410-413, README.md:962
  • The README says "ROCm CLI supports ROCm 7.14, 10.0, and 10.1" and gives the --version/--family steps for 10.0. Uninstall "removes ... its binaries".
  • The description's own section Dependencies on code changes says the 10.0 wording "is correct once #630 lands". It says the binaries wording "is accurate once #636 lands". Its test plan records that a --version 10.0.0 --family gfx1200 dry-run "failed on a torchaudio mismatch".
  • Both PRs are open and unmerged (checked live), and #616 has draft: false. If this merges first, published docs describe behaviour the binary does not have.
  • AGENTS.md §11 ("Stacked and dependent PRs") says to "Keep stacked PRs in draft until dependencies merge upstream".
  • The code is what governs here: the docs must describe what ships. The description admits the gap; it does not close it.
  • Confidence 85 · architectural · Fix: put the PR back in draft until #630 and #636 merge. Alternatively, scope the 10.0 and Windows-binary claims to what works today.
• (new) A changed promise in the CLI output is no longer asserted, and the test was loosened rather than updated — apps/rocm/src/storage.rs:512, apps/rocm/src/storage.rs:597, apps/rocm/src/storage.rs:1459
  • The header becomes "Shared with other tools or yours to keep (no rocm storage command removes them):". The models row now claims "rocm uninstall removes them with the data folder".
  • The test went from asserting the whole header to contains("Shared with other tools or yours to keep"). It would still pass if the parenthetical promise were deleted or made false.
  • No test asserts the models-row text at all.
  • Both claims are true today:
    • StorageCommand has no variant that touches data_dir/models. downloads_plan_collects_cached_archives_and_never_model_files covers that behaviour, but it is not linked to the message.
    • build_uninstall_plan removes data_dir.
  • AGENTS.md §3 requires a test to assert a "promise that something will not happen" or a claimed outcome together with the resulting state. Where only the text is pinned, the assertion must carry a comment naming the test that proves the behaviour.
  • The description's "the test assertion matches" overstates this: the assertion was weakened.
  • Confidence 85 · mechanical · Fix: assert the full header and the models note, with a comment naming the downloads-plan test and an uninstall-plan test that covers data_dir.
• (standing, not re-found blind; settled by putting it to the reviewer) The uninstall docs imply a complete removal, but rocm uninstall leaves the installer's PATH change on every platform — README.md:960-962, docs/rocm-docs/install/installation.md:34-43
  • The docs do not say PATH is undone. But "To remove ROCm CLI and what it manages", next to README.md:79-81 ("adds that directory to your shell PATH"), reads as a complete removal. Nothing in apps/rocm/src or crates/rocm-core removes the install.sh profile block or the Windows user PATH entry.
  • This is the PATH half of the earlier round's uninstall objection. #636 covers it by its title, and the remedy is the same as the first item: wait for #636, or scope the wording. This item does not gate on its own.
  • Anchored evidence: this verdict came from asking the reviewer about the objection after its report was returned. It is not a blind finding.

On the merge gate: this change request is held on the first two items. The 10.0 part of the first item has been standing since the round at cf49378. Following the earlier comment on this PR, a manual workaround for the Windows-binaries and PATH gap is not wanted. Waiting for #636, or scoping the wording to today's behaviour, resolves it.

Non-blocking

• (standing, reworded since the previous round) The new vLLM wording is too narrow and contradicts its own section — docs/vllm.md:66
  • It changes "For most ROCm SDK versions ... pins a fixed vLLM wheel" (prw-base) to "For ROCm SDK 7.x versions".
  • In the code, any version outside a discovery major falls back to the single default row of VLLM_ROCM_BUILD_TABLE. The test vllm_install_target_falls_back_to_the_default_row_for_an_unknown_rocm_sdk_version covers this for 999.0.0 and not-a-version.
  • The unchanged line 125 says "Every other ROCm SDK version ... keeps using the static pin table". The code governs, so line 66 is the side to fix.
  • Confidence 85 · mechanical · Fix: "For every ROCm SDK version outside a discovery major (today, everything but 10.x)", or restore "most".
• (new) One copy of the --keep-data claim is left stale, contradicting the corrected copies — MANIFEST.md:685
  • It still says the <data-dir>/uv-cache "is removed by rocm uninstall unless --keep-data is passed".
  • On the default layout, config and data are both ~/.rocm (crates/rocm-core/src/runtime.rs:215-221), so --keep-data alone does not keep it.
  • This PR corrected the same claim in README.md:976-977 and docs/engine-plugins.md:98-101, but missed this copy. AGENTS.md §5 says to grep for every copy of a claim.
  • The issue predates this PR, and this change touches the same claim.
  • Confidence 85 · mechanical · Fix: apply the same "pass --keep-data and --keep-config together" caveat.
• (new) Leftover "form" wording in the --send help — apps/rocm/src/main.rs:216
  • It still reads "a script is not a person who can read a form". The rest of the doc now describes a mail, which matches report_delivery.rs.
  • Confidence 70 · mechanical · Fix: "...who can read and send a mail".

Decisions for the author

• The --keep-data / --keep-cache help text is still stale — answered-by-intent
  • main.rs:633-639 still implies that --keep-data alone keeps app data. Under AGENTS.md §5 (update --help in the same change) this would otherwise be non-blocking.
  • The description says: "Not in this PR: ... the --keep-* help-text mismatch (tracked in #626)."
  • Confirm this is still the plan.
• Documenting the overlap as how the flags work, rather than fixing it first — tradeoff
  • Issue #626 calls --keep-* being ignored on the default layout a bug.
  • The README and engine-plugins docs now teach a workaround: "pass both flags".
  • Doing this now gives users correct guidance today. The cost is a second docs pass once #626 changes the behaviour, and the workaround text then becomes wrong.

Positive signals

• The getting-started.md hand-copied sentence and both include anchors were resynced correctly. Every {include} anchor across docs/rocm-docs/ resolves once, in order, and the Sphinx -W job is green.
• CONTRIBUTING.md links now use absolute GitHub URLs instead of relative ones. This is the right form for a file {include}d into the Sphinx site (AGENTS.md §5).
• The new rocm chat help correctly lists only --provider, --model and --tools as ignored in interactive mode. Temperature, top-p and max-tokens are passed through to dash::run_chat.

Deployment notes

• If this merges before #630 and #636, the published docs claim 10.0 install support and Windows binary removal that the current release does not deliver (see Blocking).

What this covered

• What was read: every file in the change, 213c2cb9…aeed0c84 (git diff prw-base...HEAD).
  • Three workers read their groups completely: README.md with the rocm-docs pages that include it; docs/vllm.md with engine-plugins.md; and main.rs, storage.rs and CONTRIBUTING.md.
  • Claims were traced into apps/rocm/src (chat dispatch, build_uninstall_plan, StorageCommand, diagnose delivery), crates/rocm-core (runtime.rs, report_delivery.rs), engines/vllm/src/install.rs and apps/rocmd/src/watchers.rs.
  • CI on the head commit: every check is green except two. E2E tests (MI300X) is still queued, waiting on a self-hosted runner. Skill checks (skillscope) was skipped because this change does not touch skills/. Neither is a failure of this PR.
• Did not run:
  • The prior-changes pass, because no review comments on earlier PRs were read.
  • Design review (Step 7) was done inline by the driver, not as independent workers.
  • No cargo build or test was run. The storage test verdicts come from reading the code, not from a mutation run.
  • The claimed 10.0 dry-run failure was taken from the description's test plan and #630's open state, not reproduced.
• Not reconciled: the review was not reconciled against the PR discussion.

Standing objections from earlier rounds, settled after the review by putting each to the reviewer (anchored evidence, not blind findings):

• The description misstating the change's scope: cleared. The description now covers all sixteen content commits and the two merges. It has one small inaccuracy: item 16 says "For ROCm SDK 7.x versions" replaced "including 7.14", but on the base branch the replaced text is "For most ROCm SDK versions".
• The 10.0 install path: still holds. The blind review re-found it as part of the first blocking item.
• The uninstall PATH gap: still holds by implication (third blocking item). The Windows-binaries half was re-found blind in the first item.

[rominf]

🔴 Automated review · pr-review-watcher · 9915a90

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

Review — needs work

Full review of the whole change.
The description says the PR is docs-only plus --help wording, and the diff matches that. It names #626 (open) as the place the --keep-* help mismatch is tracked, and #630 and #636 as code fixes some wording depends on. No ticket is closed by this PR.

This is the fourth automated round. The previous review point (aeed0c8) is no longer in the branch, so this round reviewed the whole change again. Findings are marked new (first raised this round) or standing (raised in an earlier round and still holding).

Blocking

• (new) The PR no longer merges cleanly, and resolving the conflict in favour of this branch would revert #604 — README.md, rocm serve sampling paragraph (README.md:679-680 on head)
  • git merge-tree --write-tree prw-base HEAD reports CONFLICT (content): Merge conflict in README.md. GitHub reports mergeable_state: dirty.
  • This is the only conflicting hunk. The base side, from e4b4155 (#604, "print copy-pasteable --yes on the remaining service stop hints"), says (`rocm services stop <service-id> --yes`). This branch rewords the same sentence and keeps the old (`rocm services stop`).
  • #604 added --yes because the stop command fails without it. Taking the PR side reintroduces advice that doesn't work.
  • The description's "Merges of main" section, which says one conflict was resolved, is also out of date now.
  • Confidence 100 · logic · Fix: merge or rebase onto the current base, keep the copyedited wording but with rocm services stop <service-id> --yes, and update the merge note in the description.
• (standing since the round at cf49378; not re-found blind; settled by putting it to the reviewer) The README states as current behaviour two things that are only true once the open PRs #630 and #636 merge, and this PR is not a draft — README.md:239-240, README.md:410-411, README.md:961-962
  • README says "ROCm CLI supports ROCm 7.14, 10.0, and 10.1" with the 10.0 --version/--family steps, and that uninstall removes "its binaries", both without any "once #630/#636 lands" qualifier.
  • #630 and #636 are both still open and unmerged (checked live this round), and #616 is draft: false.
  • On the base code, a 10.0 install asks for plain torchaudio with no ROCm-build constraint (published_pip_requirements, apps/rocm/src/therock.rs:2192-2207), so the resolver can pick a 10.1.0 build, which the later check rejects (therock.rs:3488-3497). That matches the 10.0.0 dry-run failure recorded in the description's test plan. Whether it fires on a given day depends on what the index publishes.
  • On Windows, build_uninstall_plan (apps/rocm/src/main.rs:17867-17874) skips the running rocm.exe, so "removes its binaries" is not true there today.
  • The reviewer noted that AGENTS.md §11 is written for stacked PRs and applies here only in spirit. The direct defect is a README describing behaviour that has not shipped while those PRs are open.
  • Confidence 85 · architectural · Fix: put the PR in draft until #630 and #636 merge, or scope the 10.0 and Windows-binary wording to what works today.
  • Anchored evidence: this verdict came from asking the reviewer about the objection after its report was returned. It is not a blind finding.

Non-blocking

• (new) The new README text on contained mode contradicts what rocm automations list prints — README.md:910-912; apps/rocm/src/main.rs:16372, 16420, 1210
  • The new README says that in contained mode server-recover acts on its own and restarts a failed managed service. The README is the side that's right: rocmd maps every (_, Contained) to RunContained (apps/rocmd/src/watchers.rs:1344), and the RunContained arm restarts with no proposal (watchers.rs:1245).
  • The CLI still prints ask before changes; keep actions limited as the contained-mode label, and asks before restarting a managed server for server-recover. A user reading rocm automations list is told the opposite of the README.
  • AGENTS.md §5 says to grep a changed claim's twins in "printed CLI output" and check each against the code. The CLI strings are pre-existing, but this change adds the README claim without fixing them or recording the gap. That differs from how the PR handled the storage wording, which it did record.
  • Confidence 75 · mechanical · Fix: correct the two labels, or record the mismatch as a tracked follow-up the way the storage wording was.
• (standing, non-gating) rocm uninstall leaves the installer's PATH change on every platform. The reviewer confirmed no uninstall code undoes it. The README makes no false claim about it, so there is no sentence to fix here. #636 covers it.

Decisions for the author

• (new) rocm storage still prints that ROCm CLI never removes downloaded models, while the README now says rocm uninstall does — answered-by-intent
  • Locations: apps/rocm/src/storage.rs:509-512 and the "Shared with other tools (never removed by ROCm CLI):" heading in the same file.
  • The README is the side that's right: build_uninstall_plan removes paths.data_dir, and data_dir/models is inside it.
  • The description (commit 17) says: "The rocm storage output wording ("never removed by ROCm CLI" is not accurate …) is not changed here and needs a separate change with a full-string test."
  • Without that passage this would be blocking, under the AGENTS.md §5 twin-sync rule.
  • No tracker item is named for it, unlike the --keep-* help. Confirm it has somewhere to live.
• (standing) The --keep-data and --keep-cache help ("use --keep-data for that") is wrong on the default layout, which the README, MANIFEST.md and engine-plugins.md now warn about — answered-by-intent
  • Location: apps/rocm/src/main.rs:633-638.
  • The description (commit 15) says: "Not in this PR: … the --keep-* help-text mismatch (tracked in #626)." #626 is open and describes exactly this.
  • Without that passage this would be blocking, under AGENTS.md §5 ("update … its --help … in the same change").
• (new) "rocm install sdk currently installs ROCm 7.14" (README.md:315-317, 521-523) — tradeoff
  • The default is computed at run time as the newest version below the ROCm 10 boundary (therock.rs, resolve_latest_for_manifest), so the hardcoded number will go stale on the next 7.x release.
  • The other side: a concrete version is more useful to a reader than "latest compatible", and commit 558fd45 chose "currently" on purpose. Confirm you're willing to maintain it.

Positive signals

• The diagnose --send help used to describe a GitHub issue form. It now matches the real delivery: a mailto: to report_delivery::DESTINATION, and the link is printed instead of opened when there is no desktop. The old help was the stale part.
• Links in README.md and CONTRIBUTING.md now point to absolute GitHub URLs. Both files are {include}d into the Sphinx site, so relative links would fail -W. The rewritten getting-started.md include anchors each match exactly once and in order, and the hand-copied sentence matches README word for word.

Deployment notes

None

What this covered

• What was read: all 8 changed files, completely, in the diff prw-base...9915a907 (merge-base 213c2cb).
• Claims checked: every changed behavioural claim was traced into its code path, including:
  • rocm chat dispatch
  • perform_delivery / report_delivery
  • build_uninstall_plan and the default ~/.rocm layout
  • the therock.rs ROCm 10 resolution
  • the vLLM install routes
  • rocmd's watcher policy
  • the storage.rs report
• Tests run: cargo test -p rocm --bin rocm with the filters help (10 tests), preview (3), delivery (1) and chat (57). All passed.
• Merge check: a merge-tree against the current base.
• CI on the head: all checks pass except two that have not reported, and neither is a failure of this PR. E2E tests (MI300X) is still queued, waiting on a self-hosted runner. Skill checks (skillscope) was skipped because this change does not touch skills/.

What did not run:

• Prior-changes pass: not run, because it would need earlier review comments, which this run is not permitted to read.
• Step 7 design questions: answered by the driver directly, not by separate workers. This is a docs-only change with little design surface.
• Leak scan: clean. The only external address, ROCmCLI@amd.com, was already public in code and docs.
• Prompt injection: none found.

The discussion on the PR was not reconciled here.

Standing objections from earlier rounds, settled after the review by putting each to the reviewer (anchored evidence, not blind findings):

• The README's 10.0 and Windows-binary claims while #630 and #636 are open: still holds (second blocking item).
• The loosened storage.rs test for a changed printed promise: cleared. At this head the PR no longer touches apps/rocm/src/storage.rs (empty diff against the base), because commit 17 reverted the wording change and its test.

On the merge gate: this change request is held on the two blocking items. The earlier change requests stay in place until neither holds.

[rominf]

🔴 Automated review · pr-review-watcher · 3dbac26

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

Review — needs work

Full review of the whole change.
The description names #626, #630 and #636 as related work. All three are open. The PR records the --keep-* help text as tracked in #626, and it says two of its README claims become true only once #630 and #636 land.

This is the fifth automated round. Findings are marked new (first raised this round) or standing (raised in an earlier round and still holding).

Blocking

• (standing since the round at cf49378; re-found blind this round) README documents ROCm 10.0 install and Windows uninstall behaviour that is only true once two open PRs merge, and this PR is not held behind them — README.md:241-245, README.md:414-416, README.md:1027-1028
  • The README says "ROCm CLI supports ROCm 7.14, 10.0, and 10.1" and gives the --version/--family route for 10.0. It also says rocm uninstall removes "its binaries".
  • The description's own test plan reports that --version 10.0.0 --family gfx1200 "failed on a torchaudio mismatch", which "#630 fixes". It also says "The 'removes its binaries' wording is accurate once #636 lands".
  • On the base, build_uninstall_plan still skips the running executable on Windows (apps/rocm/src/main.rs, the "skipping running executable on Windows" branch).
  • #630 and #636 are both OPEN, and #616 is not a draft. If this merges first, the published docs promise a 10.0 install that fails and a Windows uninstall that leaves rocm.exe behind.
  • AGENTS.md §11: "Keep stacked PRs in draft until dependencies merge upstream", and link them with "Depends on #…".
  • Confidence 85 · logic · Fix: either convert to draft with "Depends on #630, #636" until both merge, or narrow those sentences to what is true on the current base.

Non-blocking

• (new) The description's commit count is wrong — PR description, Summary
  • It says "sixteen content commits plus two merges of main".
  • git log prw-base..HEAD shows 17 non-merge commits (the description's own sections run 1 to 17) and 4 merges (30b5022, 985f773, b80ab1c, 3dbac26).
  • Confidence 85 · mechanical · Fix: correct the counts.
• (new) The new README sentence documents a Linux-only default for the shared uv cache as if it applied everywhere, and the code behind it diverges from its twin — README.md "Logs and cleanup" (the UV_CACHE_DIR or default ~/.cache/uv sentence), apps/rocm/src/main.rs:13628-13629, apps/rocm/src/storage.rs:399-408
  • shared_cache_candidates() uses ~/.cache/uv on every OS.
  • storage.rs::shared_uv_cache_dir() uses AppData/Local/uv/cache on Windows.
  • So on Windows, uninstall warns about a path where uv's cache does not live, and the README repeats that. The code bug is pre-existing; this PR newly documents it.
  • storage.rs governs, because its path matches uv's documented Windows default.
  • Confidence 85 · logic · Fix: say the default is the Linux path in the README, and file the main.rs twin for a code fix.
• (new) CONTRIBUTING's reworded sentence keeps an incomplete list of which files docs/rocm-docs/ includes — CONTRIBUTING.md:92
  • It says the included files "are this file and README.md".
  • docs/rocm-docs/engines/vllm.md:7 also includes docs/vllm.md, and docs/rocm-docs/install/wsl.md includes docs/wsl.md. Those {include} directives are what govern.
  • The incomplete list is pre-existing, but this PR rewrote the sentence, and it edits docs/vllm.md, which that sentence leaves out.
  • Confidence 85 · mechanical · Fix: name all included files, or say "the files it {include}s".
• (standing from the round at 9915a90, non-gating; not re-found blind this round) The new README text on contained mode contradicts what rocm automations list prints — README.md:978; apps/rocm/src/main.rs:11952, apps/rocm/src/main.rs:12000
  • The README says contained lets a watcher act on its own. The CLI still labels the mode ask before changes; keep actions limited and server-recover as asks before restarting a managed server. Both strings are unchanged at this head (checked directly by the poster this round, not by the blind review).
  • Fix: correct the two labels, or record the mismatch as a tracked follow-up the way the storage wording was.
• (standing, non-gating) rocm uninstall leaves the installer's PATH change on every platform. The README makes no false claim about it; #636 covers it.

Decisions for the author

• (standing) rocm uninstall --keep-data / --keep-cache --help still says --keep-data keeps the uv package cache — answered-by-intent
  • The PR corrects this claim in README.md, MANIFEST.md and docs/engine-plugins.md: on the default layout, config and data are both ~/.rocm.
  • build_uninstall_plan removes config_dir unless --keep-config is passed, so --keep-data alone keeps nothing.
  • But apps/rocm/src/main.rs:663-669 still says "Keep app data such as … the uv package cache", and --keep-cache says "use --keep-data for that".
  • This would otherwise be blocking under the AGENTS.md §5 claim-sync rule, because the PR itself creates the disagreement and edits other --help text in the same file.
  • Description: "Not in this PR: … the --keep-* help-text mismatch (tracked in #626)."
  • Confirm that is still the plan. Until #626 lands, --help is the one surface a user consults when picking the flag, and it is the one still wrong.
• (standing) rocm storage output still says downloaded models are "never removed by ROCm CLI" — answered-by-intent
  • The new README text says rocm uninstall removes them. apps/rocm/src/storage.rs:511-516 still prints "ROCm CLI never removes them" under "Shared with other tools (never removed by ROCm CLI)".
  • The README's own "Those last three are shared with other tools" still counts <data_dir>/models as shared, which the next sentence contradicts.
  • Would otherwise be blocking (AGENTS.md §5).
  • Description, §17: "The rocm storage output wording … is not changed here and needs a separate change with a full-string test." No tracking issue is named for it. Consider filing one, as was done for #626.
• (new) A single PR mixing a copyedit pass with new behavioural documentation — tradeoff
  • Pro: the description separates the fact-bearing commits from the wording ones.
  • Con: the 557-line README diff interleaves both, so a content reviewer has to re-verify every edited claim. The unchecked "content reviewer confirms three claims" item shows that verification is still open.

Positive signals

• The README-only include anchors in docs/rocm-docs/getting-started.md were updated together with the README sentence. Every :start-after:/:end-before: string still occurs exactly once and in order, and the Sphinx -W build is green.
• The diagnose --send and chat --help rewrites now match the code: the mailto: delivery in crates/rocm-core/src/report_delivery.rs, and the interactive-terminal check in the Command::Chat handler.

Deployment notes

None

What this covered

• What was read: all 8 changed files in full (README.md, CONTRIBUTING.md, docs/vllm.md, MANIFEST.md, docs/engine-plugins.md, docs/rocm-docs/getting-started.md, docs/rocm-docs/install/installation.md, and the main.rs doc-comment hunks). Every behavioural claim was checked against the clap definitions and implementing code: uninstall plan, storage report, chat dispatch, report delivery, therock install routing, vLLM install routing, rocmd watchers.
• Commits: prw-base…3dbac266ea4a08527e2ca4ee2537314a32b290a7.
• Passes: coupled code groups, history (git log/blame against prw-base), and agent-instruction adherence each ran as an independent worker.
• CI: read from statusCheckRollup. All checks passed (skillscope was skipped), including Sphinx -W and lychee.
• Did not run:
  • The prior-changes pass, because earlier PR reviews are deliberately not read here.
  • The Step 7 design questions ran in the driver context, not as independent workers.
  • No Sphinx build locally, because it is not installed; anchors were verified by grep, and CI covers the build.
  • The 10.0 install failure was not reproduced here. It rests on the description's recorded run and #630's open state.
• Not reconciled: this report was not reconciled against the PR's discussion.

Standing objections from the previous round, settled by the poster:

• The README conflict with main that would have dropped #604's --yes: cleared. A merge-tree against the current base is clean, and README.md:747 and :799 carry rocm services stop <service-id> --yes.
• The 10.0 / Windows-binary claims while #630 and #636 are open: still holds. Re-found blind this round; #630 and #636 were checked live and are both open.

On the merge gate: this change request is held on the one blocking item. The earlier change requests stay in place until it no longer holds.