fix(serve): validate --device via clap possible values (exit 2)

[r0x0r]

Summary

rocm serve --device bogus previously rejected the invalid value via application
logic (exit code 1, message "unsupported device policy") instead of clap's
possible values validation (exit code 2). Every other enum-style argument in
the CLI — --engine, config set-*, engines install, etc. — uses clap
possible values, which produces exit code 2 and lists the valid choices.

This models --device as a clap ValueEnum (DevicePolicyArg) so invalid input
is rejected consistently and the valid choices are listed in the error.

Changes

• Add DevicePolicyArg and change serve --device from Option<String> to
  Option<DevicePolicyArg>. Only gpu_required and gpu_preferred are
  advertised; cpu_only is #[value(hide = true)] — still accepted so the
  intentional exit-1 rejection keeps working, but hidden from --help, shell
  completion, and the invalid-value list.
• The historical aliases (auto/gpu → gpu_required, cpu → cpu_only) stay
  accepted for backward compatibility but are hidden from the advertised list.
• The intentional cpu_only/cpu app-level rejection (exit 1 with rocm serve requires ROCm GPU execution; CPU mode is not a fallback path in rocm-cli) is
  preserved — only the generic "unsupported device policy" exit-1 path is
  replaced by clap's exit-2 usage error.
• Reworded the gpu_preferred help so it no longer implies a CPU-fallback path:
  it now reads "Accepted for compatibility; behaves identically to gpu_required
  (rocm serve has no CPU fallback path)" (AGENTS.md §6).
• Dropped cpu_only from the README rocm serve synopsis and removed the stale
  README line that described --gpu being ignored with --device cpu_only, so no
  documented surface advertises a value that --help deliberately hides.
• Reworked the device possible-values sync test to read clap's structural possible
  values instead of a hand-written doc string, mirroring the existing --engine
  test.

Behavior

Before:

$ rocm serve qwen --device bogus
Error: unsupported device policy: bogus      # exit 1

After:

$ rocm serve qwen --device bogus
error: invalid value 'bogus' for '--device <DEVICE>'
  [possible values: gpu_required, gpu_preferred]   # exit 2

--device cpu_only (and the cpu alias) still exit 1 with the GPU-required
message; they are simply no longer advertised as choices.

Tests

• serve_rejects_unknown_device_policy_as_usage_error — asserts invalid
  --device is a clap InvalidValue usage error (fails before the fix, passes
  after).
• serve_accepts_device_policy_values_and_aliases — asserts every value and
  legacy alias still parses to the expected variant.
• serve_device_help_lists_match_device_policy_names — reworked to verify clap's
  structural possible values (including hidden entries, via get_possible_values)
  stay in sync with the full DevicePolicy set.
• device_policy_arg_maps_through_parse_device_policy — pins the DevicePolicyArg
  → DevicePolicy mapping so a mis-wired variant is caught.

cargo test -p rocm --bin rocm, cargo clippy -p rocm --all-targets -- -D warnings, and cargo fmt all pass. No new e2e scenario is added: the change is a
CLI usage-validation/exit-code correction with no new runtime engine behavior, and
it is fully covered by the argument-parsing unit tests above.

[volen-silo]

Review — changes requested

Reviewed main...HEAD (1 commit, apps/rocm/src/main.rs, +80/-45). The core idea is right and the implementation is clean: DevicePolicyArg is a genuine improvement over the free-form Option<String>, the alias set is preserved faithfully (verified EnumValueParser matching is case-sensitive by default, same as the old exact-string match — no input that used to work is now rejected), and hidden aliases really are excluded from the advertised list. One blocking item, plus a few notes.

Blocking

1. The new per-variant doc comments are rendered as user-facing help text, and one of them is wrong.

Because --device is now a ValueEnum, clap promotes the variant doc comments into --help and shell completions. Confirmed against the built binary:

      --device <DEVICE>
          Device policy

          Possible values:
          - gpu_required:  Require a ROCm GPU; fail if none is usable (the default)
          - gpu_preferred: Prefer a ROCm GPU when one is available
          - cpu_only:      Run on CPU only (not a supported fallback path in rocm serve)

parse_device_policy (main.rs:16763-16771) maps gpu_required, gpu_preferred, auto and gpu all to DevicePolicy::GpuRequired. gpu_preferred does not "prefer" anything — it hard-requires a GPU and fails if none is usable, identically to gpu_required. Both engine normalizers do the same (engines/vllm/src/lib.rs:1154, engines/lemonade/src/lib.rs:3756); no layer implements prefer-with-CPU-fallback.

This text is new in this PR — before the change the doc comment was just /// Device policy [possible values: gpu_required, gpu_preferred, cpu_only]. with no descriptions. So the PR introduces user-facing documentation that contradicts the code, and specifically advertises what reads as a CPU-fallback path, which AGENTS.md §6 explicitly guards against ("preserve strict GPU-required behavior; do not introduce silent CPU fallback").

Suggested fix (one line): reword to state actual behavior, e.g.

/// Accepted for compatibility; behaves identically to `gpu_required`
/// (rocm serve has no CPU fallback path).
GpuPreferred,

Please re-read all three variant docs as user-facing help text, not internal comments — that is what they now are.

Non-blocking

2. cpu_only is advertised as a valid choice in the error message that corrects a typo. It is unconditionally rejected (main.rs:16766). The old error path had no possible-values list, so this is the one dimension where the change makes the advertised contract slightly less honest than before: a user who types --device bogus is now told [possible values: gpu_required, gpu_preferred, cpu_only], steering them at a value that can never succeed. #[value(hide = true)] on CpuOnly would keep it accepted (preserving the deliberate exit-1 message) while dropping it from help, completions and the suggestion list. Consistent with the alias-hiding already used on this enum, and it does not break serve_device_help_lists_match_device_policy_names (get_possible_values() still returns hidden entries). Your call — the parenthetical caveat in the description is a partial mitigation.

3. as_policy_str() is untested and is the one place a swap ships silently. It has exactly two references: its definition (main.rs:1004) and the single call site (main.rs:4878). Neither new test exercises it — they stop at DevicePolicyArg, never crossing into DevicePolicy. A mis-typed arm mapping CpuOnly to "gpu_required" would turn the intentional CPU rejection into a silent GPU-required serve and every test in the suite would still pass. Cheapest fix: extend serve_accepts_device_policy_values_and_aliases to feed the parsed variant through parse_device_policy and assert the resulting DevicePolicy (including that cpu_only still errors).

It is also the third hand-written copy of the same three strings, alongside device_policy_name() (main.rs:17110) and parse_device_policy's match arms. The new sync test guards the possible-values list against DevicePolicy, but nothing guards this mapping.

4. The scope is right, but the half-migration is undisclosed. engines/vllm/src/lib.rs:1164 and engines/lemonade/src/lib.rs:3853 carry the identical pre-fix pattern — free-form String --device-policy, manual match, bail! on a bad value, exit 1, no possible-values, no completions. These are not internal-only: docs/vllm.md:34 documents rocm-engine-vllm resolve-model ... --device-policy gpu_required as a directly-runnable command, so a typo there still hits exactly the bug this PR fixes for rocm serve. Migrating them is legitimately separable work per AGENTS.md §11 — just worth a line in the description or a tracked follow-up so this does not read as done everywhere.

5. While you're here: README.md's --gpu paragraph states "--gpu is ignored with --device cpu_only (the model runs on CPU)", which is false for the same reason as item 2. Pre-existing and outside the diff, but this PR is precisely about the honesty of the --device value list, so it is a natural companion fix.

6. e2e scenario justification. AGENTS.md §3 names exit codes as user-observable and says a unit test does not discharge the scenario requirement. I checked the actual norm rather than the letter: PR #303 is directly on point (a pure CLI exit-code correction, 3 → 0, justified in near-identical language, merged with no scenario), and tests/e2e-cucumber/expectations.toml has no row tracking this behavior, so the PR #296 xfail-flip route does not apply either. I am satisfied the omission is consistent with repo practice — but citing #303 in the description would preempt the next reviewer reading §3 literally. No existing scenario regresses or silently passes on a stale assumption (the serve-no-gpu-fails-fast family tests the app-level GPU refusal path with valid --device, untouched here).

Tradeoff worth noting

Routing DevicePolicyArg back through a string into parse_device_policy rather than a direct From impl looks like a smell but is defensible: parse_device_policy is the single choke point enforcing the cpu_only rejection across three callers (the CLI, the disk-persisted managed-service restart at main.rs:13940, and the hidden __engine-serve-http subcommand at main.rs:16689). A direct conversion would create a path that bypasses that guardrail. Worth confirming this was deliberate — if so, a one-line comment on as_policy_str saying so would lock the reasoning in.

Positive

• --device gains structural shell completion for the first time (it had none as a bare String). Worth a line in the description; it is a real user-facing win beyond the exit-code fix.
• Consolidating serve_arg_help + possible_values_listed_in_help into a single serve_possible_values(arg_id) shared by both sync tests is a genuine simplification, and dropping the rendered-help string parsing costs no coverage now that there is no hand-written list left to drift.
• serve_rejects_unknown_device_policy_as_usage_error genuinely fails on main (where --device had no value parser, so parse_serve returned Ok and .expect_err would panic) — a real regression test, per AGENTS.md §3.

Verification performed

cargo clippy -p rocm --bin rocm --all-targets -- -D warnings clean; cargo fmt --all -- --check clean; cargo test -p rocm --bin rocm device 6/6 and serve_engine 5/5 pass. Rendered rocm serve --help from the built binary to confirm item 1. Checked clap 4.6 source for EnumValueParser case sensitivity, PossibleValue::alias hiding, and Error::exit_code() returning 2 for InvalidValue — the exit-2 claim holds for both --device and --engine. Swept the repo for other --device/--device-policy consumers, scripts, feature files, expectations rows, completion snapshots and docs.

[r0x0r]

Thanks for the catch — pushed in 8b1b470.

• Misleading gpu_preferred help (blocking): the per-variant ValueEnum doc comments render in serve --help, completions, and the invalid-value suggestion list, so "Prefer a ROCm GPU when one is available" was actively wrong — parse_device_policy maps gpu_preferred straight to GpuRequired and there is no CPU fallback. It now reads: "Accepted for compatibility; behaves identically to gpu_required (rocm serve has no CPU fallback path)."
• cpu_only advertised as a choice: taken your suggestion and added #[value(hide = true)], so it no longer appears in --help, completions, or the invalid-value hint ([possible values: gpu_required, gpu_preferred]). It still parses so the deliberate exit-1 rejection message is preserved, and serve_device_help_lists_match_device_policy_names still passes (get_possible_values() returns hidden entries).
• as_policy_str was untested: added device_policy_arg_maps_through_parse_device_policy, which feeds each parsed variant back through parse_device_policy and asserts the resulting DevicePolicy — gpu_required/gpu_preferred → GpuRequired, cpu_only → rejected — so a mis-typed arm can't silently swap the policy while other tests stay green.

Verified locally: the 7 device/serve unit tests pass and serve --help / --device bogus render as described.

[volen-silo]

Follow-up review — resolved

Re-verified against the current head 8b1b470 (was 4bd5a84 at the time of the first review). All three items are genuinely fixed — checked against the rebuilt binary, not the claim.

Blocking

1. Misleading gpu_preferred help text — resolved. Rendered from the rebuilt binary:

      --device <DEVICE>
          Device policy

          Possible values:
          - gpu_required:  Require a ROCm GPU; fail if none is usable (the default)
          - gpu_preferred: Accepted for compatibility; behaves identically to `gpu_required` (rocm serve has no CPU fallback path)

No CPU-fallback claim survives in any of the variant docs. The AGENTS.md §6 concern is discharged.

Non-blocking, previously raised

2. cpu_only advertised — resolved, on all three surfaces:

• rocm serve qwen --device bogus → [possible values: gpu_required, gpu_preferred], exit 2
• rocm completions bash → compgen -W "gpu_required gpu_preferred"
• a near-miss (--device cpu_onl) no longer suggests it either

And the deliberate rejection is intact: --device cpu_only and the cpu alias both still exit 1 with rocm serve requires ROCm GPU execution; CPU mode is not a fallback path in rocm-cli.

3. as_policy_str untested — resolved. device_policy_arg_maps_through_parse_device_policy passes and genuinely bites: the CpuOnly => "gpu_required" swap I described flips the is_err() assertion. The as_policy_str doc comment now also records the choke-point reasoning, which closes the tradeoff item as well.

4, 5, 6 — not addressed, and that is fine. The engine-crate half-migration disclosure, the README --gpu/cpu_only line, and citing #303 for the e2e omission. All were non-blocking; none block merge.

Small ripples from this commit

None blocking, but hide = true left three consistency loose ends:

• The PR description is now stale. Its "After" block still shows [possible values: gpu_required, gpu_preferred, cpu_only], which is not what the binary prints any more, and it does not mention the hidden cpu_only, the reworded help, or the fourth test. It is the artifact maintainers read — worth a refresh before merge.
• README.md:298 still lists [--device gpu_required|gpu_preferred|cpu_only] in the serve synopsis, so the README now advertises a value that --help deliberately hides. Same family as item 5; those two README lines are now the last place cpu_only is presented as a real choice for rocm serve.
• crates/rocm-dash-tui/src/ui/serve_wizard.rs:45-50 hardcodes DEVICES = ["(engine default)", "gpu_required", "gpu_preferred", "cpu_only"]. It is not clap-derived, so it did not pick up the hide: a user can still select cpu_only in the TUI wizard and get a launch that always fails. Pre-existing and out of scope, just noting it is now the remaining surface offering the dead choice.

Minor: the comment at apps/rocm/src/main.rs:18110-18114 says the possible values "are advertised in --help and shell completion" and that "the sync tests below keep the advertised lists honest". For --device that is no longer quite true — cpu_only is deliberately unadvertised, and serve_device_help_lists_match_device_policy_names compares against the full DevicePolicy set including hidden entries. The test is still the right test; the sentence describing it drifted.

Gates

Run at 8b1b470, exit codes checked:

• cargo fmt --all -- --check — 0
• cargo clippy --workspace --all-targets -- -D warnings — 0
• cargo test --workspace --all-targets — 0, zero failures workspace-wide, including the four device/serve argument tests

CI is green apart from docs/readthedocs..., which is also failing on unrelated open PRs and is not caused by this change.

Verdict

LGTM. The blocking finding is resolved and nothing outstanding blocks merge. Refreshing the PR description is the only thing I would still do before landing.

[r0x0r]

Thanks for the thorough re-verification. The description refresh is done, and the three hide = true ripples are addressed:

• PR description: the "After" block now shows [possible values: gpu_required, gpu_preferred], and the body documents the hidden cpu_only (#[value(hide = true)], still accepted for the exit-1 rejection), the reworded gpu_preferred help, and the fourth test (device_policy_arg_maps_through_parse_device_policy).
• README: the same push drops cpu_only from the rocm serve synopsis (README.md:298) and removes the stale line describing --gpu being ignored with --device cpu_only, so no documented surface advertises a value that --help hides. That also closes the earlier non-blocking README item.
• main.rs:18110 comment: updated to note --device's cpu_only is deliberately unadvertised and that serve_device_help_lists_match_device_policy_names compares against the full DevicePolicy set (hidden entries via get_possible_values), so the sentence no longer overstates what is in --help/completion.

serve_wizard.rs:45-50's hardcoded DEVICES list is left as-is — as you noted, it's pre-existing and out of scope for this change.

Note on history: the branch's fix commit was signed-off but not cryptographically signed, which was failing the commit-signatures gate and blocking the push hook. I re-signed the range and force-pushed with lease; git range-diff confirms content is byte-identical (signatures only). verify-commits, clippy, fmt and test pass at the new head.