test: check that every advised rocm command parses; fix dashboard labels that don't

[rominf]

Fixes #534.

Remediation lines, help examples and docs tell users which rocm commands to run, but tests pin only their wording at most. A renamed flag or a removed subcommand therefore leaves users with advice that fails.

This adds a test that collects every rocm/rocmd command the project advises, from:

• the --help of every visible command
• production string literals: labelled next step:/Try:/apply with: lines, the rocm fix recipe commands and dashboard labels
• README.md, docs/, skills/, and the demo tapes

It fills in placeholders and routes each command through the same entry points rocm uses: the natural-language router, then the real clap definition. It fails when:

• clap rejects an advised command
• a single word falls through to the planner, meaning the advised subcommand does not exist
• a line meant to be run as written omits a required argument (inline prose, or text marked with …, may leave values out)

Each source must still yield a known command and a minimum count, so a broken extractor cannot pass silently. The templated retry advice for services stop/restart is checked through the real message function.

It found four broken commands, fixed here:

• The dashboard's ROCm tab showed three:

  • Runs: rocm install --channel … --format … (missing sdk)
  • Runs: rocm update --check (no such flag)
  • Runs: rocm doctor (not a subcommand; from a shell it reaches the planner and does nothing)

  They now name what each screen actually runs, and a new unit test, rocm_runs_labels_name_what_each_manager_spawns, ties each label to the argv that screen's manager builds.

• README's rocm storage [report] [--json] implied rocm storage --json, which is rejected. It is now rocm storage [report [--json]].

No Gherkin scenario. The dashboard change corrects display-only text in the ROCm tab's detail pane; what each manager spawns is unchanged. The new unit test derives each expected label from the argv the manager builds, so it proves the label true rather than pinning a string. No existing dash scenario asserts the Runs: text (dash-01 only checks that the ROCm actions are listed).

Verification. Every guard was mutation-checked. The test goes red for each of these:

• reverting each fix
• a bad flag injected into help text, a Rust literal, a doc, or a tape
• making rocm examine require an argument (19 findings)
• disabling each source extractor
• changing a dashboard label to rocm update --apply

The new tests run in about 1 s and are pure: no environment, filesystem or process side effects. rocmd is checked with --help appended, which stops at the parser.

Local gates:

• cargo fmt --all --check: 0
• both clippy invocations: 0
• cargo xtask manifest --check: 0
• cargo test -p rocm --all-targets: 975 passed. Two tests unrelated to this change failed once under the parallel run and pass in isolation.
• rocm-dash-tui ui:: tests: 0

docs/testing.md describes the check and when an exclusion is appropriate.

[juhovainio]

I read through this PR in full (the new advised_commands.rs scanner, the three dashboard label fixes, the README tweak, and the new argv-tying unit test) against AGENTS.md/CONTRIBUTING.md and didn't find anything I'd block on.

What I checked specifically:

• The three claimed CLI bugs are real and the fixes are correct: rocm install --channel/--format needs the nested sdk subcommand to be valid, rocm update --check isn't a flag (bare rocm update already does the check), and rocm doctor isn't a real subcommand (only a natural-language-planner alias) — confirmed directly against apps/rocm/src/main.rs's Command enum rather than taking the PR body's word for it.
• The new scanner's extraction logic (backtick-span parity, literal-invocation detection skipping comments/#[cfg(test)], synopsis expansion for [...]/a|b/ellipsis, placeholder substitution) looks sound on a manual trace, and it's routed through the real clap parsing entry points rather than a reimplementation.
• The NOT_INVOCATIONS exclusion list entries all look like genuinely non-advice text (comments, unrelated prose), not a cover for a real gap — and there's a dedicated test (exclusions_still_match_something) guarding against an exclusion going stale.
• The new rocm_runs_labels_name_what_each_manager_spawns test genuinely ties the dashboard labels to the real argv-producing code (EXAMINE_ARGS, UpdateAction::Check.args(), build_args()) rather than just restating the string.
• Scope is tight (one logical change: advised-command drift), no internal-content leaks, test-only module is properly #[cfg(test)]-gated.

This looks safe to approve after final validation (CI green, a maintainer's own pass) — this is a diff-only read on my end, not a substitute for that.

[juhovainio]

Reviewed the full diff (the new advised_commands.rs scanner, the three dashboard label fixes, the README tweak, and the new argv-tying unit test) against AGENTS.md/CONTRIBUTING.md. No blocking issues.

What I checked:

• The three claimed CLI bugs are real and the fixes are correct: rocm install --channel/--format needs the nested sdk subcommand, rocm update --check isn't a flag (bare rocm update already does the check), and rocm doctor isn't a real subcommand (only a natural-language-planner alias) — confirmed directly against apps/rocm/src/main.rs's Command enum rather than trusting the PR body.
• The scanner's extraction logic (backtick-span parity, literal-invocation detection skipping comments/#[cfg(test)], synopsis expansion, placeholder substitution) looks sound and is routed through the real clap parsing entry points, not a reimplementation.
• NOT_INVOCATIONS entries all look like genuinely non-advice text, and there's a dedicated test guarding against an exclusion going stale.
• The new rocm_runs_labels_name_what_each_manager_spawns test genuinely ties the dashboard labels to the real argv-producing code, not just a restated string.
• Scope is tight, no internal-content leaks, test-only module is properly #[cfg(test)]-gated.

Looks good to merge pending CI.

[rominf]

The merge-queue run failed on every_advised_command_parses. It flagged rocm diagnose --model {}: {} in crates/rocm-core/src/model_readiness.rs, which arrived on main with #407 after this branch was approved. That line is the heading of the model-readiness report (<command>: <verdict>), not advice to run anything, so it's now in NOT_INVOCATIONS with that reason (ca0e0ada), next to the other status headings. I rebased onto current main (5c4f401d) first, since the exclusion has to match text that exists on the branch, and confirmed it's the only new hit. The CodeQL failure in that run was a side effect: its upload failed because the queue branch had already been deleted on ejection.

Verified locally: advised_commands (7), xtask (255), e2e-cucumber lib and feature_naming, cargo check --test e2e, and clippy -D warnings for both --all-targets and --test e2e. The push moves the head past the existing approval, so this needs a re-approval before it can go back into the queue.

[siloteemu]

🔴 Automated review · pr-review-watcher · ca0e0ad

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 — no blocking findings

Full review of the whole change.
Closes #534 completely: the three wrong dashboard Runs: labels and the README rocm storage synopsis are fixed, and the new check is what found them.

Blocking

None

Non-blocking

• The checker lets some commands through that clap would reject, and the docs say it checks "every" command — apps/rocm/src/advised_commands.rs:462-473, apps/rocm/src/advised_commands.rs:992-995, apps/rocm/src/advised_commands.rs:1036, docs/testing.md:118-142
  To test this I added a fenced block to a scratch doc under docs/ and ran every_advised_command_parses. Only the bare rocm frobnicate line was reported. Three lines that would fail for a user passed:

  • rocm update && rocm frobnicate --x: command_part cuts the line at &&, so only the first command is checked. The same happens at ;, | and ||.
  • rocm install sdk --channel release --bogus-flag: command_part also cuts at a double space, so the bad flag is never seen.
  • rocm frobnicate <TEXT>: the checker's own placeholder fills <TEXT> with "start a local model". That value contains whitespace, so is_deliberate_natural_language accepts it as a deliberate natural-language request. A removed subcommand with a required text-like argument is therefore never caught. Advice like this only exists today as rocm --yes <request>, so this is latent, not a current miss.

  docs/testing.md says the test "checks every command that the CLI or its docs tell a user to run" and that "a command that clap rejects" fails it. The cases above show both statements are wider than what the code does. The same section says every_source_is_scanned "requires each source to still yield a known command". In the code (advised_commands.rs:1117-1152), only the help and Rust sources are checked for a named command. Markdown and tapes are checked by count alone.
  Confidence 100 (reproduced by running the test) · logic · Fix: the code should govern, because it is what actually runs. Three options:

  • Check every &&/;-separated rocm segment.
  • Stop cutting at a double space for anything except help EXAMPLES rows.
  • Only accept a request as deliberate natural language when the multi-word value came from the advice text, not from a filled-in placeholder.

  Otherwise, list these limits in docs/testing.md the way that file already lists the blind spots of the environment-test scan, and correct the every_source_is_scanned sentence.

Decisions for the author

• A text scanner over source code, versus structured advice — tradeoff
  advised_commands.rs finds advice by reading Rust and Markdown as text. It works line by line, matches labels like next step:, and skips #[cfg(test)] items by counting braces. That needs no change to production code and covers every surface today, including help and tapes.
  The cost is that each heuristic has edges that pass silently:

  • A rocm … line inside a raw or multi-line Rust string is never extracted.
  • A backtick span that wraps across lines in rendered help is dropped.
  • A { or } inside a string within a #[cfg(test)] item could make the skip run to the end of the file.

  The other choice is to keep advice in typed constants that tests can list, which is more reliable but means changing every call site. The minimum-count floors are a reasonable backstop. Confirm the text approach is deliberate.

• The Install label test checks only a prefix and the flag names — tradeoff
  rocm_runs_labels_name_what_each_manager_spawns checks that the label starts with rocm install sdk and contains --channel and --format. A label with literal values, extra flags or reordered flags would still pass. The flag list is written out by hand rather than taken from build_args(). Exact matching is impossible because the label shows … for values the user picks, so a partial check is defensible. Confirm it is enough.

Positive signals

• The Update and Examine labels are checked against what the screen actually runs (UpdateAction::Check.args() and the new EXAMINE_ARGS, which run_examine now uses), so the test proves the label is true rather than pinning a string. I confirmed each of the three label fixes is caught on its own: putting each old label back one at a time made the test fail at rocm.rs:207, :212 and :225.
• rocmd_verdict appends --help, so Cli::try_parse_from stops at the parser before the tokio runtime is built. rocmd advice is checked against the real definition without running anything.
• Each NOT_INVOCATIONS entry is keyed by file plus exact text and carries a reason. exclusions_still_match_something keeps the list from going stale.

Deployment notes

None

What this covered

I read every file in the change in full: all 1,354 lines of advised_commands.rs, the four dash-tui files, and the README and docs/testing.md hunks. I also read the routing in apps/rocm/src/main.rs that the checker reuses (run, parse_freeform_invocation, should_treat_as_freeform, command_invocation_error, cli_command), the clap definitions for storage, install sdk, update and examine, and rocmd::run_from_args. Range: 2d45c61…ca0e0ada.

Tests I ran:

• cargo test -p rocm --bin rocm advised_commands: 7 passed, 1 ignored.
• The three-way revert of the labels described above.
• The false-pass probe described in the finding, using a scratch doc that I deleted afterwards.

The four code passes ran as separate read-only workers. History, agent-instruction adherence and code-comment compliance ran as one combined worker. I did not run separate Step 7 workers for the design questions. I settled those myself from the worker reports and my own runs, so that step was not independent. The prior-changes pass did not run, because earlier reviews of these files are not readable from here.

CI at review time: E2E tests (MI300X) and E2E tests (MI350P) were still queued and had not reported, so nothing here rests on them. Skill checks (skillscope) had not reported at handoff and has since reported as skipped. Every other check passed.

The PR text explains why there is no Gherkin scenario (the dashboard fix changes display text only, and dash-01 does not assert the Runs: text), as AGENTS.md §3 requires. The leak scan of the diff found nothing beyond the license headers and sign-off identities.

This review was not reconciled against the PR discussion.

[rominf]

Addressing the automated review on ca0e0ada. Rebased onto f9a80117 (now 9ad9f3d5). All three false passes reproduced and are fixed in b24da211 and 0fa6ba49, each with a fixture test that fails when its fix is reverted:

1. Shell lists. Advice is now split at &&, ||, ; and a standalone |, but not inside quotes, <…>, […] or {…}, and stop|restart stays intact. Every part that runs rocm/rocmd is checked, and parts for other tools are skipped. rocm update && rocm frobnicate --x is now a finding.
2. Double space. It is no longer a general cut. Only the description column of EXAMPLES rows needs it, so it now applies only to an indented row: the rendered help and the after_help literal those rows are written in. rocm install sdk --channel release --bogus-flag is now a finding. README's aligned synopsis rows are also checked past their padding now, and all of them parse.
3. Placeholder-filled requests. Each argv word now remembers the advice word it came from. A natural-language verdict counts as deliberate only if the advice itself is multi-word (a quoted request or <natural language request>), so rocm frobnicate <TEXT> is now a finding.

8ee20a46 makes docs/testing.md state exactly what is scanned and checked. every_source_is_scanned now also requires Markdown and tapes to yield a named command, not just a count.

The rebase surfaced a semantic conflict with main. Main's out-of-memory recipes (#251) advise rocm serve <model> <case-appropriate options above>. The checker filled that placeholder with an invented value, which clap rejects. 9ad9f3d5 treats a placeholder that names options as standing for no arguments, so rocm serve <model> is still checked rather than excluded; disabling the rule fails both its unit test and the full-tree check. The same commit lists two prose notes from #384 that open with rocm serve/rocm chat as not advice.

On the two decisions:

(a) Text scanner vs typed advice constants: deliberate. A scanner needs no production change and reaches help output, tapes and docs, which typed constants could not. The per-source named-command checks and count floors guard against an extractor going blind. The edges you listed are now in a "Known gaps" list in docs/testing.md:

• raw or multi-line Rust strings
• backtick spans wrapped across lines in a Rust string
• {/} inside strings in a #[cfg(test)] item throwing off the skip

(b) Install label: tightened in c79e9f7e. The expected label is now built from build_args() on the default form: the subcommand, then every value flag with its value shown as …. Mode switches such as --dry-run and the approval flag are left out, since they depend on what the user picks. The comparison is exact, so an added, dropped or invented flag fails it.

Re-ran locally on the rebased head, all clean:

• rocm --bin rocm (1024 passed), xtask (271), e2e-cucumber lib (136), feature_naming and the --test e2e check
• both clippy invocations and fmt