Skip to content

Improve CLI help - #48

Merged
r0x0r merged 4 commits into
mainfrom
cli-help
Jun 26, 2026
Merged

Improve CLI help #48
r0x0r merged 4 commits into
mainfrom
cli-help

Conversation

@r0x0r

@r0x0r r0x0r commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

This pull request significantly improves the usability and user guidance of the rocm CLI by enhancing command documentation, providing better help messages, and introducing intelligent suggestions for mistyped commands. The changes focus on making the CLI more approachable for new users and easier to navigate, especially when encountering errors or seeking help.

Enhanced CLI documentation and help:

  • Expanded the main CLI help and individual command help texts, including detailed long_about and after_help sections with usage examples for commands like chat, update, model, serve, logs, install sdk, and engines install. This provides users with clear instructions and practical command usage patterns. [1] [2] [3] [4] [5] [6] [7]

  • Added descriptive comments to enum variants such as InstallFormat, Provider, WatcherModeArg, and TelemetryModeArg to clarify their intent and possible values.

Improved error handling and user guidance:

  • Implemented logic to detect unrecognized single-word commands, providing a helpful error message that suggests the closest known command (using Levenshtein distance) and directs users to available help resources. This includes a new unknown_command_hint function and supporting logic.

  • Updated the fallback notes for unmatched freeform requests to direct users to rocm --help and suggest including an action word in their request.

Testing and reliability:

  • Added new tests to verify that the unknown command hinting logic works as expected, including suggestions for close typos and correct handling of natural language requests.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the rocm CLI’s usability by expanding help/documentation text across commands and by adding a “did you mean” hint path for mistyped single-word freeform invocations.

Changes:

  • Expanded top-level and per-command help text (long_about, after_help) with clearer guidance and examples.
  • Added unknown-command hinting for single-token freeform requests, including Levenshtein-based suggestions.
  • Added unit tests validating the unknown-command hint behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread apps/rocm/src/main.rs Outdated
r0x0r added a commit that referenced this pull request Jun 24, 2026
The .chars().all(...) predicate already rejects whitespace, so the
preceding str::contains(char::is_whitespace) check was redundant.
Addresses review feedback on PR #48.

Signed-off-by: Roman <roman.sirokov@amd.com>

@rominf rominf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't yet check all the commands in the examples. I recommend you verify they all operational.

Comment thread apps/rocm/src/main.rs Outdated
Comment thread apps/rocm/src/main.rs Outdated
r0x0r added a commit that referenced this pull request Jun 24, 2026
Expand top-level and per-command help (long_about/after_help) with
clearer guidance and verified examples for examine, install, model,
serve, services, chat, update, logs, and engines.

Route botched command invocations to clap's built-in suggestions engine
(the "suggestions" feature) so a mistyped subcommand -- whether it
stands alone (`instal`) or is followed by flags (`doctorgdfg --help`) --
surfaces a friendly "did you mean"/usage error and exits, instead of a
hand-rolled Levenshtein matcher or a raw planner request plan. Genuine
natural-language requests still flow to the planner.

Addresses review feedback on PR #48:
- replace the custom unknown-command hint with clap suggestions
- reference `rocm examine` instead of the removed `rocm doctor`

Signed-off-by: Roman <roman.sirokov@amd.com>
@r0x0r

r0x0r commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

@rominf Verified using an agent that all commands in the example are operational

@rominf rominf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: typo-routing has a gap, and one help string names a non-existent command

CI is green and the leak scan is clean. Two concrete issues, plus a refactor that fixes the bigger one and removes a fragility in the same move.

1. Mistyped subcommand + argument bypasses clap suggestions (logic bug)

rocm automatios list falls through to the natural-language planner instead of surfacing clap's "did you mean automations":

$ rocm automatios list
request plan
  ...
  note: No ROCm action matched this request.
  note: Run `rocm --help` to see available commands, or rephrase ...

Root cause in command_invocation_error (apps/rocm/src/main.rs:855):

let has_flag = request_args.iter().any(|arg| arg.starts_with('-'));
if request_args.len() > 1 && !has_flag {
    return None;   // <- "automatios list" exits here, never reaching clap
}

A near-miss subcommand followed by a normal argument (no flag) is treated as prose. It should still get a suggestion. instal works only because it is a single token; instal list would fail the same way.

2. Daemon help references a command that does not exist

apps/rocm/src/main.rs:347 says the daemon is "normally started on demand by rocm automations enable and rocm managed serve". There is no rocm managed serve command — the managed path is the --managed flag on serve (serve --managed, cf. main.rs:3760 and main.rs:10810). Should read rocm serve --managed.

3. Suggested refactor (fixes #1 and drops a fragility)

The lone-token branch keys off err.to_string().contains("similar") (main.rs:864), matching clap's human-readable wording. clap exposes this structurally via err.get(ContextKind::SuggestedSubcommand). Switching to it lets the helper collapse to a single rule: surface clap's error when it carries a suggestion, or when a flag is present (e.g. doctorgdfg --help); otherwise fall through to the planner. That removes both the len() > 1 && !has_flag early-return (cause of #1) and the string match, and keeps the existing tests green — instal still suggests, prose like please install still routes to the planner.

@rominf

rominf commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

One more, on commit hygiene: both commits on this branch are unsigned (git log --format=%G?N), whereas every commit on main is signed (E). If signed commits are expected here, this needs the two commits re-signed, which means a history rewrite + --force-with-lease rather than a follow-up commit. Not CI-enforced now, I will create a PR for this soon.

r0x0r added a commit that referenced this pull request Jun 25, 2026
…help

Address review feedback (PR #48):

- command_invocation_error now surfaces clap's error whenever clap
  carries a subcommand suggestion (via ContextKind::SuggestedSubcommand)
  or the request contains a flag, instead of bailing out for any
  multi-word request. This fixes `rocm automatios list` (and
  `instal list`), which previously fell through to the natural-language
  planner, and drops the fragile `err.to_string().contains("similar")`
  match.

- Daemon help referenced a non-existent `rocm managed serve`; corrected
  to `rocm serve --managed`.

Signed-off-by: Roman <roman.sirokov@amd.com>
@r0x0r

r0x0r commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the detailed review — all three addressed in 6fac2ab.

1 & 3 (typo-routing gap + refactor). Adopted the suggested refactor. command_invocation_error now surfaces clap's error whenever clap carries a subcommand suggestion (read structurally via err.get(ContextKind::SuggestedSubcommand)) or the request contains a flag; otherwise it falls through to the planner. This drops both the len() > 1 && !has_flag early-return and the fragile to_string().contains("similar") match. rocm automatios list now suggests automations, and instal list suggests install:

$ rocm automatios list
error: unrecognized subcommand 'automatios'

  tip: some similar subcommands exist: 'status', 'automations'

Prose (please install a model) and lone non-command words still route to the planner. Added regression tests for the trailing-arg case.

2 (daemon help). Fixed rocm managed serverocm serve --managed.

cargo clippy --workspace is clean and cargo test -p rocm is green (764 tests).

On commit signing: good flag — happy to align if signed commits are expected here. As you noted it needs a history rewrite to re-sign the existing commits rather than a follow-up, so I'll leave the existing history in place pending the separate PR you mentioned; let me know if you'd prefer I rewrite + --force-with-lease instead.

r0x0r added a commit that referenced this pull request Jun 25, 2026
Expand top-level and per-command help (long_about/after_help) with
clearer guidance and verified examples for examine, install, model,
serve, services, chat, update, logs, and engines.

Route botched command invocations to clap's built-in suggestions engine
(the "suggestions" feature) so a mistyped subcommand -- whether it
stands alone (`instal`) or is followed by flags (`doctorgdfg --help`) --
surfaces a friendly "did you mean"/usage error and exits, instead of a
hand-rolled Levenshtein matcher or a raw planner request plan. Genuine
natural-language requests still flow to the planner.

Addresses review feedback on PR #48:
- replace the custom unknown-command hint with clap suggestions
- reference `rocm examine` instead of the removed `rocm doctor`

Signed-off-by: Roman <roman.sirokov@amd.com>
r0x0r added a commit that referenced this pull request Jun 25, 2026
…help

Address review feedback (PR #48):

- command_invocation_error now surfaces clap's error whenever clap
  carries a subcommand suggestion (via ContextKind::SuggestedSubcommand)
  or the request contains a flag, instead of bailing out for any
  multi-word request. This fixes `rocm automatios list` (and
  `instal list`), which previously fell through to the natural-language
  planner, and drops the fragile `err.to_string().contains("similar")`
  match.

- Daemon help referenced a non-existent `rocm managed serve`; corrected
  to `rocm serve --managed`.

Signed-off-by: Roman <roman.sirokov@amd.com>
@michaelroy-amd

Copy link
Copy Markdown
Member

Review — approve with nits. Help/doc-only + clap 4.5→4.6 (suggestions); low risk and a real UX improvement.

  • [MEDIUM] --engine and --device are Option<String> (free-form), but the help now hand-writes [possible values: lemonade, pytorch, llama.cpp, vllm, sglang] and [possible values: gpu_required, gpu_preferred, cpu_only]. That text will drift from the actual accepted values over time. Either make them ValueEnum (clap then renders + validates the list for free) or add a test asserting the help list matches the real accept-set.
  • [LOW] Branch is BEHIND main — rebase needed; currently CHANGES_REQUESTED.

The per-variant and ValueEnum doc comments and the EXAMPLES after-help are a nice touch.

r0x0r added 4 commits June 26, 2026 07:17
Expand top-level and per-command help (long_about/after_help) with
clearer guidance and verified examples for examine, install, model,
serve, services, chat, update, logs, and engines.

Route botched command invocations to clap's built-in suggestions engine
(the "suggestions" feature) so a mistyped subcommand -- whether it
stands alone (`instal`) or is followed by flags (`doctorgdfg --help`) --
surfaces a friendly "did you mean"/usage error and exits, instead of a
hand-rolled Levenshtein matcher or a raw planner request plan. Genuine
natural-language requests still flow to the planner.

Addresses review feedback on PR #48:
- replace the custom unknown-command hint with clap suggestions
- reference `rocm examine` instead of the removed `rocm doctor`

Signed-off-by: Roman <roman.sirokov@amd.com>
Signed-off-by: Roman <roman.sirokov@amd.com>
…help

Address review feedback (PR #48):

- command_invocation_error now surfaces clap's error whenever clap
  carries a subcommand suggestion (via ContextKind::SuggestedSubcommand)
  or the request contains a flag, instead of bailing out for any
  multi-word request. This fixes `rocm automatios list` (and
  `instal list`), which previously fell through to the natural-language
  planner, and drops the fragile `err.to_string().contains("similar")`
  match.

- Daemon help referenced a non-existent `rocm managed serve`; corrected
  to `rocm serve --managed`.

Signed-off-by: Roman <roman.sirokov@amd.com>
Address review feedback (PR #48): the hand-written `[possible values: ...]`
help on `serve --engine`/`--device` could drift from the real accept-set.
Keep both flags free-form (`--engine` is unconstrained, `--device` accepts
aliases like `auto`/`gpu` and intentionally rejects `cpu_only` per the
no-CPU-fallback policy), so a ValueEnum would change accepted input. Instead
add tests asserting the rendered help lists stay in sync with their sources of
truth (`builtin_engine_inventory()` and `DevicePolicy` names), and fix the
existing drift where the engine list omitted `atom`.

Signed-off-by: Roman <roman.sirokov@amd.com>
@r0x0r

r0x0r commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks @michaelroy-amd — both addressed in 21f8873.

[MEDIUM] --engine/--device possible-values drift. Went with the test option rather than ValueEnum, because making these enums would change accepted input:

  • --engine is intentionally free-form: select_serve_engine passes any value through (recipe/config can also supply engines), so a ValueEnum would start rejecting inputs that work today.
  • --device accepts aliases beyond the displayed canonical names — parse_device_policy maps auto/gpugpu_required, and cpu/cpu_only is deliberately rejected with the "CPU is not a fallback" message (strict-GPU policy). A ValueEnum would drop those aliases and that targeted error.

So instead there are now two sync tests that render the actual --help and assert the advertised lists match their source of truth:

  • serve_engine_help_lists_match_engine_inventorybuiltin_engine_inventory()
  • serve_device_help_lists_match_device_policy_names → the DevicePolicy variant names

Writing the test surfaced an existing drift: the engine list was missing atom (present in the inventory), now added — so serve --help lists lemonade, pytorch, llama.cpp, vllm, sglang, atom. These tests fail if either list drifts again.

[LOW] Branch behind main. Rebased onto current main; PR is now MERGEABLE. Commits are SSH-signed and DCO-signed.

cargo clippy --workspace clean; cargo test -p rocm green (788 tests).

@r0x0r

r0x0r commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator Author

@rominf follow-up on the signing note — I went ahead with the rewrite rather than waiting for a separate PR.

The branch history is now rewritten: all four commits are SSH-signed and carry DCO Signed-off-by trailers (git log --format='%G?'G on each), and the branch is rebased onto current main.

Full-transparency caveat: GitHub still shows them Unverified (reason: unknown_key) right now, because I signed with a locally-generated SSH key and that public key isn't registered to the account yet. They will flip to Verified as soon as the key is added under the account's SSH signing keys. If you'd rather these be signed by a specific canonical key/identity as the trust anchor, tell me which one and I'll re-sign + --force-with-lease to that instead — I didn't want to assume an ad-hoc key should be the trust root.

@r0x0r
r0x0r added this pull request to the merge queue Jun 26, 2026
Merged via the queue into main with commit 8720f43 Jun 26, 2026
7 checks passed
@r0x0r
r0x0r deleted the cli-help branch June 26, 2026 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants