fix(cli): accept negative float flag values in space form (EAI-8243)

[r0x0r]

Summary

rocm serve --temperature -1 (space-separated form) was rejected by clap with the
confusing error unexpected argument '-1', because clap parsed the leading-dash
token as a flag rather than a value. Only the equals form (--temperature=-1)
reached the value validator that reports the valid range.

This is the classic negative-number-as-flag gotcha. It is a tokenizer-level
ambiguity that fires before any value parser runs, so it is not float-specific —
it affected every numeric value flag: --temperature and --top-p (float) and
--max-tokens (positive integer) on both chat and serve.

Fix

Enable clap's allow_negative_numbers on the affected numeric flags so the space
form reaches the value parser. Both forms now validate identically and surface a
clear range/parse message instead of an unexpected-argument error.

allow_negative_numbers is preferred over allow_hyphen_values here: it only
accepts leading-dash tokens that parse as numbers, so it does not swallow the next
flag when a value is missing (the tradeoff called out for --gpu-memory-utilization).

Testing

• Gherkin scenario (AGENTS.md §3): @id:serve-negative-temperature-rejected
  in tests/e2e-cucumber/features/model_serving.feature covers the user-observable
  stderr change end-to-end. It is ungated — the value parser runs inside argument
  parsing before engine selection or GPU pre-flight, so it needs no GPU and no
  engine and gates every PR (mirrors the ungated @id:fix-position-argument-rejected
  precedent). The scenario is now numbered (Scenario: 15 - ...) for consistency
  with the rest of the suite.
• Extended the space-form regression tests (serve/chat) to cover
  --temperature, --top-p, and --max-tokens in both space and equals
  forms, asserting they reach ValueValidation (the value parser) rather than
  being rejected as an unexpected argument.
• cargo test -p rocm --bin rocm (touched sampling tests pass); cargo clippy -p rocm --all-targets -- -D warnings; cargo fmt -p rocm.

Deliberate scope-out (follow-up)

rocm fix --device-index -1 has the same gotcha with its own dedicated
--device-index must be >= 0 message (crates/rocm-core/src/fix.rs), but it is
not the only remaining instance. The same negative-as-flag rejection still
fires on a wider class of single-value numeric flags across the tree — for
example rocm serve --port -1, rocm bench load --isl -1 (and
--osl/--requests/--batch-sizes/--keep), rocm diagnose --top -5, and
rocm comfyui logs --lines -5 — roughly eight flags, not one. These are left to
a follow-up to keep this PR one logical change. The durable fix for the recurring
class is a whole-tree guard test walking Cli::command() to assert every
single-value numeric arg sets allow_negative_numbers/allow_hyphen_values; it
belongs with that follow-up, which is "fix the class" rather than "fix
--device-index".

[volen-silo]

Code review — changes requested

The fix itself is correct and well-chosen. I verified the mechanism against the pinned clap (4.6.1) and against the built binary on this branch: rocm serve foo --temperature -1 now reports invalid value '-1' for '--temperature <TEMPERATURE>': temperature must be a finite value >= 0.0, and pre-fix the same input returns ErrorKind::UnknownArgument, so the new tests genuinely discriminate the bug rather than tautologically passing. Preferring allow_negative_numbers over allow_hyphen_values is the right call and the PR body explains why — confirmed in clap source that it only fires when the whole token after - parses as a number, so --temperature --top-p 0.5 still errors with "a value is required" instead of swallowing the next flag.

Two things to resolve before merge.

1. Missing Gherkin scenario (AGENTS.md §3)

AGENTS.md §3 states that if a change alters what a user of the CLI can observe — command output, exit codes — the behavior "must be covered by a Gherkin scenario in tests/e2e-cucumber/features/", that "a unit test asserting the internal helper does NOT discharge this", and that the carve-out is for "purely internal changes (refactors, CI plumbing, docs)".

This change alters CLI stderr for a command users type directly, so the carve-out does not apply, and the PR body's justification ("Behavior is CLI argument parsing only... The new unit tests assert the user-observable error kind") is the exact substitution the rule forbids. No scenario in tests/e2e-cucumber/features/ mentions temperature, top-p, or max-tokens today, so there is no existing @id: to name either.

There is a direct precedent for this class in the repo — @id:fix-position-argument-rejected (tests/e2e-cucumber/features/diagnose.feature:60) is a confusing-argument-error-becomes-clear fix that got an ungated scenario.

Adding one here is cheap and needs no gating tags: the value parser runs inside try_parse_from before model resolution or any GPU pre-flight, verified by running the binary with a bogus model name and no --engine. Then serving is refused before any engine starts already exists (tests/e2e-cucumber/tests/e2e/serving_steps.rs:784) and is reusable verbatim; only one new When and one new Then are needed (~16 lines, mirroring user_serves_gpu_required at serving_steps.rs:709 and assert_no_gpu_message at serving_steps.rs:797). No expectations.toml row — that file is for known-bug xfails only. Something like:

  @id:serve-negative-temperature-rejected
  Scenario: Serving with a temperature below zero is refused with a clear reason
    When the user serves a model with a negative sampling temperature
    Then serving is refused before any engine starts
    And the CLI explains that temperature cannot be negative

2. --max-tokens has the identical bug, on the same two commands

apps/rocm/src/main.rs:269 (chat) and :413 (serve) declare --max-tokens with value_parser = parse_positive_u32 and no allow_negative_numbers. Verified on the binary built from this branch:

$ rocm serve foo --max-tokens -1
error: unexpected argument '-1' found

...while --max-tokens=-1 reaches the parser and reports max-tokens must be greater than 0 (main.rs:8237). That is exactly the asymmetry this PR fixes, three lines away in the same struct, with a dedicated message that stays unreachable in the space form.

The PR body's claim ("all float flags that accept a value") is literally accurate, but the underlying gotcha is not float-specific — it is a tokenizer-level ambiguity that fires before any value parser runs, so the argument that "u32 has no legitimate negative value" does not make the user-facing symptom any less confusing. Please either include --max-tokens (plus a test case) or state the scope-out explicitly in the PR body so it is a deliberate decision rather than an oversight.

Design note — this gotcha has now recurred three times

git log -S allow_hyphen_values / -S allow_negative_numbers shows two prior independent fixes for the same clap gotcha in this file (32acc52d, 7cdc30c1) before this one, and two instances are still open (--max-tokens, and --device-index on fix). Per-flag annotation has not prevented recurrence.

A whole-tree guard test would close the class durably, and the repo already has the idiom: cli_command_definition_is_valid (main.rs:17694) and hidden_subcommands_excluded_from_completions_tree already walk Cli::command(). Confirmed the needed getters are public in clap_builder 4.6.0 — Arg::is_allow_negative_numbers_set (arg.rs:4462), is_allow_hyphen_values_set (arg.rs:4457), get_value_parser (arg.rs:4488) — so a ~30-line recursive test asserting "every single-value arg with a numeric value parser sets allow_negative_numbers or allow_hyphen_values" is writable with public API only. Worth considering here or as a follow-up; not blocking this PR.

Non-blocking

• Comment overclaims slightly. --temperature -0.5abc still yields error: unexpected argument '-0' found, because clap only treats a token as a negative number when the entire remainder parses as one. The comment's "instead of clap rejecting -1 as an unexpected argument" is fine for the literal case it names, but the general framing does not hold for malformed negative-looking input.
• Comment coverage. The 3-line explanation sits above --temperature in both structs (main.rs:260, :404) but never above --top-p (:266, :410), which gets the same treatment for the same reason. A reader landing on --top-p sees the attribute with no rationale. Either repeat it or write it once covering both flags — the verbatim duplication across the two structs is also a small maintenance smell.
• Test comment asymmetry. serve_negative_sampling_space_form_reaches_range_validator carries the explanation; the adjacent, materially identical chat_... test has none.
• Equals-form cases in the new tests are not regression guards. --temperature=-1 / --top-p=-0.5 already passed before the fix (clap resolves --opt=value through parse_opt_value unconditionally once the arg matches), and serve_rejects_out_of_range_sampling (main.rs:18239) already exercises the equals path. Keeping them as an explicit "both forms behave identically" assertion is defensible — just noting they add no new discriminating coverage.
• Adjacent, out of scope: rocm fix --device-index -1 has the same gotcha, and crates/rocm-core/src/fix.rs:852 has a dedicated --device-index must be >= 0 message that the space form can never reach. Worth a follow-up.

Positive signals

• Choosing allow_negative_numbers over the broader allow_hyphen_values, and calling out in the PR body why (it will not swallow the next flag when a value is missing) — that tradeoff is the non-obvious part and it is documented in the right place.
• The regression tests fail before the fix and pass after, verified rather than assumed.
• All CI checks are green on the current head.

[r0x0r]

Thanks — thorough review. Addressed in 862b0e0.

1. Gherkin scenario (blocking, §3): added @id:serve-negative-temperature-rejected to model_serving.feature, ungated per your @id:fix-position-argument-rejected precedent — the value parser runs inside try_parse_from before engine selection or GPU pre-flight, so it needs no GPU/engine and gates every PR. It reuses serving is refused before any engine starts verbatim and adds one When (mirrors user_serves_gpu_required) and one Then (mirrors assert_no_gpu_message) asserting the temperature must be a finite value >= 0.0 message. e2e-cucumber compiles with the new steps.

2. --max-tokens (blocking): included it. Added allow_negative_numbers to --max-tokens on both chat and serve, and extended both space-form regression tests with --max-tokens -1 / --max-tokens=-1 (they now reach parse_positive_u32 → ValueValidation instead of UnknownArgument). PR body updated to say the fix covers all numeric value flags, not just floats.

Non-blocking, addressed:

• Comment coverage / overclaim: reworked the shared rationale comment to cover all three numeric flags in one place (above --temperature in both structs), and it now states clap only treats a -… token as a number when the whole remainder parses as one — so a missing value (--temperature --top-p 0.5) or a malformed token (--temperature -0.5abc) still errors at parse time. That removes the earlier overclaim.
• Test comment asymmetry: the chat_… test now carries a symmetric comment pointing back to the serve_… one.
• Equals-form cases: kept as an explicit "both forms behave identically" assertion, as you noted they're defensible even though they aren't new discriminating coverage.

Deliberate follow-up (not in this PR): rocm fix --device-index -1 shares the gotcha with its own --device-index must be >= 0 message. I scoped it out in the PR body to keep this one logical change. The whole-tree Cli::command() guard test is the right durable fix for the recurring class, but it would necessarily fail until --device-index is also fixed, so it belongs with that follow-up rather than here.

[volen-silo]

Follow-up review — both blocking findings resolved

Re-verified against the current head 862b0e03 (changed from a5003c0c, which is what the original review covered). Checked out that commit locally and ran the gates — cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace --all-targets all exit 0, no failures.

1. Missing Gherkin scenario — resolved

Not taken on trust: I ran it. @id:serve-negative-temperature-rejected executes ungated on a no-GPU WSL host and passes all three steps, with the harness reporting 0 xfail, 0 XPASS, 0 unexpected failure(s) — so the "gates every PR" claim holds, and no expectations.toml row is needed. It also genuinely discriminates the bug: pre-fix the CLI emits unexpected argument '-1' found, so serving is refused before any engine starts would still pass on the non-zero exit but the CLI explains that temperature cannot be negative would fail. The asserted literal matches the parser's message exactly (main.rs:8222). Step definitions follow the surrounding style — serve_output reuse, assertion-message format, the lowercase-then-match idiom that assert_no_gpu_message already uses.

2. --max-tokens — resolved

allow_negative_numbers is now on --max-tokens in both structs (main.rs:272, :419). Verified on a binary built from this head:

$ rocm serve foo --max-tokens -1
error: invalid value '-1' for '--max-tokens <MAX_TOKENS>': `-1` is not a valid positive integer
$ rocm chat --prompt hi --max-tokens -1
error: invalid value '-1' for '--max-tokens <MAX_TOKENS>': `-1` is not a valid positive integer

I also went looking for a regression and did not find one. allow_negative_numbers is evaluated per-Arg in clap 4.6, not command-wide, so it cannot leak into the model positional or interact with allow_hyphen_values on --gpu-memory-utilization in the same struct. Confirmed on the binary: serve -1 … still rejects the positional, --max-tokens 100 with trailing flags parses fine, and serve foo --max-tokens (missing value) still reports a value is required rather than swallowing the next token. The new space-form test case is the discriminating one (pre-fix it returns UnknownArgument, not ValueValidation); the equals-form case is the "both forms behave identically" assertion, as agreed.

Non-blocking items from the original review

• Comment overclaim — resolved, and the new wording is accurate. Checked both halves against clap 4.6 and the binary: --temperature --top-p 0.5 gives a value is required for '--temperature' (a long flag is always a new arg regardless of the setting), and --temperature -0.5abc gives unexpected argument '-0' found. Both "error at parse time" as the comment now says.
• Comment coverage — resolved. One block covering all three flags. Minor and not worth changing: the block sits above --temperature while --max-tokens is six lines further down past --top-p's own doc comment.
• Test comment asymmetry — resolved.
• Equals-form cases — your call, and it's the right one. Keeping them as an explicit both-forms assertion is fine; no change wanted.
• --device-index scope-out — accepted. Stating it in the PR body is what I asked for, and the guard test belongs with it rather than here.

One correction to the scope-out (not blocking, doesn't change the merge decision)

The PR body reads as though --device-index is the one remaining instance. It isn't — the class is wider than either of us said. On the binary built from this head:

$ rocm serve foo --port -1        → error: unexpected argument '-1' found
$ rocm bench load --isl -1        → error: unexpected argument '-1' found
$ rocm diagnose --top -5          → error: unexpected argument '-5' found
$ rocm comfyui logs --lines -5    → error: unexpected argument '-5' found

--osl, --requests, --batch-sizes and --keep look the same by inspection. That matters mainly for the follow-up's sizing: the whole-tree Cli::command() guard would fail on roughly eight flags, not one, so the follow-up is "fix the class" rather than "fix --device-index". Worth reflecting in the body so the next person doesn't scope it as a one-liner.

One new nit introduced by 862b0e03

model_serving.feature:143 is the only unnumbered scenario title in the suite. Every other scenario across all the feature files — including @id:fix-position-argument-rejected, the precedent this PR cites — is Scenario: <N> - <Title>. Worth a one-word fix (Scenario: 15 - Serving with a temperature below zero…) to keep the file consistent.

Verdict

LGTM. Both blocking findings are genuinely resolved, verified rather than accepted. The scenario-number nit and the PR-body scope-out correction are worth doing but neither blocks merge.

Note on CI: E2E tests (GPU) and the Read the Docs build are failing here, but they fail identically on main's latest run and on every other currently open PR, so they are pre-existing and unrelated to this change.

[r0x0r]

Thanks — both follow-ups are pushed.

• Scenario number: model_serving.feature:143 is now Scenario: 15 - Serving with a temperature below zero is refused with a clear reason (15 was the next free number in the suite), so it matches the Scenario: <N> - <Title> convention every other scenario uses.
• Scope-out correction: the PR body no longer implies --device-index is the last instance. It now names the wider class you found — --port, --isl/--osl/--requests/--batch-sizes/--keep, --top, --lines, plus --device-index (~eight flags) — and frames the follow-up (the whole-tree Cli::command() guard test) as "fix the class" rather than a one-liner.

Note on history: the branch's earlier commit was signed-off but not cryptographically signed, so it 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 the content is byte-identical (signatures only). verify-commits, clippy, fmt and test all pass at the new head.