From 31b82c07e55afb9c3c4d3b102d4325a853a94e69 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 24 Aug 2026 02:56:11 +0000 Subject: [PATCH 1/2] fix(derive): keep an invalid choice after an override displaces the flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--trace` declares `overrides --log-level`, and displacing a flag cleared `__given_*` along with the value it held — so the post-binding choice check, which sat inside that guard, never asked the `__invalid_choice_*` ledger and `mise --log-level=v --trace` was accepted. usage-lib and clap both refuse it. `overrides` settles which of a pair is in effect; it does not make a word that was never one of the choices into one. Only a later occurrence of the flag itself replaces the word it was handed, which is the ledger's own reset and is left alone. Found by the differential proptest over mise's spec, reported as `mise github token --log-level=v --include-global --trace`; the command path and the undeclared flag are incidental. Pinned as the corpus vector `overrides-do-not-erase-an-invalid-choice`, which usage-lib already answers, so the reference divergence list stays empty. Co-Authored-By: Claude Opus 5 --- benches/gate/tests/differential.rs | 66 +++++++++++++++++++++++++++++ conformance/tests/post_binding.rs | 43 +++++++++++++++++++ corpus/08-choices-and-required.json | 10 +++++ derive/src/codegen.rs | 24 +++++++---- docs/rust/validation.md | 5 ++- docs/spec/reference/flag.md | 6 +++ 6 files changed, 145 insertions(+), 9 deletions(-) diff --git a/benches/gate/tests/differential.rs b/benches/gate/tests/differential.rs index 08c642fae..f358f676a 100644 --- a/benches/gate/tests/differential.rs +++ b/benches/gate/tests/differential.rs @@ -25,6 +25,13 @@ //! surfaced. The third was real and is fixed: `subcommand_required` was in the spec and no //! parser read it (#992). //! +//! A later run found the second real one, and it pointed the other way: usage-argv accepted +//! `mise --log-level=v --trace` because `--trace` declares `overrides --log-level`, and +//! displacing a flag threw away the record of the word it had been given along with the +//! value. usage-lib and clap both refused it. Pinned as +//! `overrides-do-not-erase-an-invalid-choice` in the corpus and as +//! `a_flag_displaced_by_an_override_still_reports_its_invalid_choice` below. +//! //! Repeats are permissive in usage by default, including at the derive layer. A command that //! owns its entire grammar can opt into clap's rule with `args_override_self=false`: //! @@ -521,6 +528,9 @@ fn clap_shadow_keeps_flag_requirements_from_the_shared_spec() { #[test] fn an_override_does_not_erase_an_invalid_choice() { + // `--index` is not declared on `tool-alias set` at all, so it falls through to a + // positional: what this pins is that a later *token* does not wash out the bad word. + // The `overrides` half of the name is the test below, which this one did not reach. let outcome = run( &SPEC, &[ @@ -535,6 +545,62 @@ fn an_override_does_not_erase_an_invalid_choice() { assert_eq!(outcome.clap, Verdict::InvalidChoice); } +/// The proptest's finding, kept as a line rather than as a seed. +/// +/// `mise github token --log-level=v --include-global --trace` came out of the generator +/// reading `Accept` / refuse / `InvalidChoice`, which no arm of `explained` names. Shrunk by +/// hand to the two tokens that matter: `--include-global` is undeclared here and falls +/// through to `[HOST]`, and the command path is incidental — `--log-level` is global. +/// +/// `--trace` declares `overrides --log-level`, and usage-argv resolved that by clearing the +/// flag *and* the record of the word it had been given, so nothing was left to judge. The +/// pair is what `overrides` settles; whether `v` is one of the five levels is not, and the +/// other two parsers said so. Fixed in the derive's post-binding checks and pinned in the +/// corpus as `overrides-do-not-erase-an-invalid-choice`. +#[test] +fn a_flag_displaced_by_an_override_still_reports_its_invalid_choice() { + for line in [ + vec![ + "github".to_string(), + "token".to_string(), + "--log-level=v".to_string(), + "--include-global".to_string(), + "--trace".to_string(), + ], + // The same disagreement with the command path and the stray flag taken away. + vec!["--log-level=v".to_string(), "--trace".to_string()], + ] { + let outcome = run(&SPEC, &line); + assert_eq!( + outcome.argv, + Verdict::InvalidChoice, + "mise {}: the override settles the pair, not the word", + line.join(" ") + ); + assert!(!outcome.lib, "mise {}", line.join(" ")); + assert_eq!( + outcome.clap, + Verdict::InvalidChoice, + "mise {}", + line.join(" ") + ); + } + + // The half that was already right, kept beside it: `overrides` holds between the two + // flags whichever order they arrive in, so neither order may swallow the bad word. + let outcome = run(&SPEC, &["--trace".to_string(), "--log-level=v".to_string()]); + assert_eq!(outcome.argv, Verdict::InvalidChoice); + + // And a valid level is still displaced in silence — the fix does not turn `overrides` + // into a refusal. + let outcome = run( + &SPEC, + &["--log-level=info".to_string(), "--trace".to_string()], + ); + assert_eq!(outcome.argv, Verdict::Accept); + assert!(outcome.lib); +} + #[test] fn a_bare_word_diverges_because_of_the_mount_not_the_parser() { // The first reading of this was "usage-argv refuses a lone `-` that clap accepts", filed as diff --git a/conformance/tests/post_binding.rs b/conformance/tests/post_binding.rs index e5ffe8b0b..56f5d61b8 100644 --- a/conformance/tests/post_binding.rs +++ b/conformance/tests/post_binding.rs @@ -441,6 +441,12 @@ struct Ovr { /// Everything #[usage(long)] all: bool, + /// How much to log + #[usage(long, choices("debug", "info"), overrides = "--trace")] + level: Option, + /// Log everything + #[usage(long)] + trace: bool, } #[test] @@ -458,6 +464,43 @@ fn the_last_of_two_overriding_flags_wins() { assert!(!ovr.stdin, "displaced by the flag that came after it"); } +/// The corpus's `overrides-do-not-erase-an-invalid-choice`. +/// +/// Found by the differential proptest over mise's spec, where +/// `mise --log-level=v --trace` was accepted here and refused by usage-lib and by clap. +/// The pair is settled by `overrides`; whether `v` is a word the flag accepts is not. +#[test] +fn an_override_does_not_erase_an_invalid_choice() { + let a = argv(["--level", "v", "--trace"]); + assert!( + matches!( + Ovr::parse_from(&a), + Err(usage_argv::Error::InvalidChoice { name: "level", .. }) + ), + "the displaced flag's word was still never one of the choices" + ); + + // The other way round, where the flag carrying the bad word is the one that won. + let a = argv(["--trace", "--level", "v"]); + assert!(matches!( + Ovr::parse_from(&a), + Err(usage_argv::Error::InvalidChoice { name: "level", .. }) + )); + + // A word that *is* one of the choices is displaced in silence, as before. + let a = argv(["--level", "info", "--trace"]); + let ovr = Ovr::parse_from(&a).expect("should parse"); + assert!(ovr.trace); + assert_eq!(ovr.level, None, "displaced by the flag that came after it"); + + // And a later occurrence of the flag itself *is* a correction: only that replaces the + // word, which is the rule `the_last_scalar_choice_controls_validation` pins. + let a = argv(["--level", "v", "--level", "info", "--trace"]); + let ovr = Ovr::parse_from(&a).expect("the second occurrence replaced the bad word"); + assert!(ovr.trace); + assert_eq!(ovr.level, None); +} + #[test] fn a_later_override_clears_an_earlier_duplicate() { let a = argv(["--file", "a", "--file", "b", "--stdin"]); diff --git a/corpus/08-choices-and-required.json b/corpus/08-choices-and-required.json index a3daf7766..95045dbb7 100644 --- a/corpus/08-choices-and-required.json +++ b/corpus/08-choices-and-required.json @@ -150,6 +150,16 @@ }, "layer": "post-binding" }, + { + "id": "overrides-do-not-erase-an-invalid-choice", + "doc": "`--trace` came last, so `--log-level` lost and holds nothing by the end. That settles which of the two is in effect; it does not make `v` one of the choices. Only a later occurrence of `--log-level` itself can replace the word it was given — a different flag winning the pair is not a correction of what was typed.", + "spec": "name \"ex\"\nbin \"ex\"\nflag \"--log-level \" overrides=\"--trace\" {\n choices \"debug\" \"info\"\n}\nflag \"--trace\"\n", + "argv": ["--log-level", "v", "--trace"], + "expect": { + "error": "invalid_choice" + }, + "layer": "post-binding" + }, { "id": "conflicts-both-given", "doc": "Two flags declared to conflict cannot be given together. Unlike `overrides`, where the last one wins, a conflict is a mistake to report rather than an order to resolve.", diff --git a/derive/src/codegen.rs b/derive/src/codegen.rs index fc1423505..90aba94c5 100644 --- a/derive/src/codegen.rs +++ b/derive/src/codegen.rs @@ -9283,19 +9283,27 @@ fn post_binding(cli: &Cli) -> TokenStream { let standing_only = unless_standing_only(f); let given = format_ident!("__given_{}", ident); Some(quote! { + // Asked outside the `given` guard below, because a flag that lost an `overrides` + // is no longer given and its value has gone back to its default — so the loop has + // nothing left to look at, and without this the word would go unreported. + // `overrides` settles which of two flags is in effect; it does not make a word + // that was never one of the choices into one. usage-lib and clap both refuse + // `--log-level=v --trace` for that reason, and mise's spec is where the + // divergence showed up. A repeat of the *same* flag clears this ledger itself, + // so what reaches here is a word nothing later replaced. + if #active && partial.#invalid { + return ::std::result::Result::Err( + usage_argv::Error::InvalidChoice { + name: #name, + choices: #choices, + }, + ); + } // Choices validate lexical input. A typed computed default is already a value of // the field's type; its `Display` form exists only so the partial can carry it to // the final conversion and need not itself be one of the advertised CLI words. // Environment values mark the field given, so they still take this path. if #active && partial.#given #standing_only { - if partial.#invalid { - return ::std::result::Result::Err( - usage_argv::Error::InvalidChoice { - name: #name, - choices: #choices, - }, - ); - } for value in #values { // Compared as text, since a choice is a word. // diff --git a/docs/rust/validation.md b/docs/rust/validation.md index e0073a70a..d957704c5 100644 --- a/docs/rust/validation.md +++ b/docs/rust/validation.md @@ -39,7 +39,10 @@ max_retries: Option, `overrides` is the quiet sibling of `conflicts`: a later occurrence of one flag discards an earlier occurrence of the other instead of erroring — useful for `--json` / `--yaml` pairs where -the last one typed should win. +the last one typed should win. What it discards is the value, not the fact that a word was +typed: a value outside the loser's `choices` is still an `InvalidChoice`, since `overrides` +answers which flag is in effect and not whether the word was one that flag accepts. Only a +later occurrence of the same flag replaces its value. ## Groups diff --git a/docs/spec/reference/flag.md b/docs/spec/reference/flag.md index 97737ba66..eb0135944 100644 --- a/docs/spec/reference/flag.md +++ b/docs/spec/reference/flag.md @@ -157,6 +157,12 @@ mistake, and silently honouring one of them hides it. A conflict holds in either direction, so declaring it once is enough; it applies to flags that were actually given, not to defaults. +Losing an override unsets a flag; it does not unsay the word it was given. `--log-level=v +--trace` is still refused when `v` is outside `--log-level`'s +[`choices`](/spec/reference/arg) — which +of the two is in effect is what `overrides` settles, and only a later occurrence of +`--log-level` itself replaces the word it was handed. + For three or more flags that exclude each other, or for "one of these is required", a [`group`](/spec/reference/group) says in one node what `conflicts` says once per pair. From ca16d62d269886c6fdd5254448ce1a6a0699f8fa Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Mon, 24 Aug 2026 03:11:22 +0000 Subject: [PATCH 2/2] fix(go): judge a displaced flag's word against its choices MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit usage-go had the same gap the derives did, and the corpus vector caught it: a flag that lost an `overrides` was dropped before anything looked at it, so `--log-level=v --trace` parsed. Losing a pair unsets a flag; it does not unsay the word the flag was given. `argv.CheckDisplaced` is the half of `Check` that still applies — the choices, and nothing else. A loser stays out of the running for `required`, for `env` and `default`, and for the rules that read one entry to judge another. The conformance driver now walks every declared entry rather than only those with a final state, so a line with more than one fault reports the same failure whichever flag won the pair. Co-Authored-By: Claude Opus 5 --- docs/go/generated-code.md | 4 ++- docs/go/index.md | 4 ++- go/argv/post.go | 49 ++++++++++++++++++++++-------- go/argv/post_test.go | 25 +++++++++++++++ go/conformance/conformance_test.go | 37 ++++++++++++++++++---- 5 files changed, 98 insertions(+), 21 deletions(-) diff --git a/docs/go/generated-code.md b/docs/go/generated-code.md index 8dfa8606d..aba68200c 100644 --- a/docs/go/generated-code.md +++ b/docs/go/generated-code.md @@ -111,7 +111,9 @@ command's struct is filled with no caller involvement. Three things `Parse` deliberately does **not** do: -- **`overrides` is not applied.** If your spec uses it, call `argv.ApplyOverrides` yourself. +- **`overrides` is not applied.** If your spec uses it, call `argv.ApplyOverrides` yourself, + then `argv.CheckDisplaced` on each key it reports: losing a pair unsets a flag, it does not + unsay the word the flag was given. - **Help and version are not printed** — they come back as `*argv.Error` with `CodeHelp` / `CodeVersion` for you to render ([Help and errors](/go/help)). - **No chain comes back with an error.** The renderers want the command chain; recover it with diff --git a/docs/go/index.md b/docs/go/index.md index e80db38ff..7f13ca78d 100644 --- a/docs/go/index.md +++ b/docs/go/index.md @@ -148,7 +148,9 @@ Worth knowing before you commit: - **`overrides` is not enforced by generated `Parse`.** `conflicts`, `required_if`, and `required_unless` are; a spec relying on last-one-wins `overrides` semantics needs to call - `argv.ApplyOverrides` itself. + `argv.ApplyOverrides` itself, and `argv.CheckDisplaced` on each key it reports — a flag that + lost is out of the running for `required`, `env`, and `default`, but the word it was typed is + still judged against its `choices`. - **Fields are `string`, `bool`, `[]string`, or `int` (for counts).** A spec says what a value is called, never what type it is — convert with [`argv.Int`, `argv.Duration`, etc.](/go/binding#typed-values) - **`complete` run-scripts, `config` nodes, `group`, `value_hint`, and `mount` are not carried diff --git a/go/argv/post.go b/go/argv/post.go index 76605da94..5c8dde74a 100644 --- a/go/argv/post.go +++ b/go/argv/post.go @@ -311,6 +311,40 @@ func explicitRelationshipValues(m *Meta, values []string, source Source, negated return values } +// CheckDisplaced judges the words an entry was typed, and nothing else about it. +// +// It is the half of [Check] that still applies to a flag which lost an +// `overrides`. Such a flag is out of the running for `required`, for `env` and +// `default`, and for the rules that read one entry to judge another — but +// `overrides` settles which of a pair is *in effect*, not whether the word the +// loser was handed was ever one it accepts. `--log-level=v --trace` is refused +// for that reason, and usage-lib and the Rust derives agree; the corpus pins it +// as `overrides-do-not-erase-an-invalid-choice`. +// +// Pass the values the command line supplied, before any `env` or `default` +// fallback: a loser is not filled from either, so there is nothing else to judge. +func CheckDisplaced(m *Meta, values []string) *Error { + if m == nil { + return nil + } + accepted := m.AcceptedChoices + if len(accepted) == 0 { + accepted = m.Choices + } + if len(accepted) == 0 || m.AllowUnknownChoices { + return nil + } + // Every value, not just the first: a variadic can be given a good value + // and a bad one, and so can a repeatable flag across occurrences. + for _, v := range values { + if !containsChoice(accepted, v, m.IgnoreCase) { + return &Error{Code: CodeInvalidChoice, Name: m.Name, + Spelling: m.Spelling, Choices: m.Choices} + } + } + return nil +} + // Check applies the rules that judge what ended up bound. // // `values` is the result of [Fill], and `occurrences` is how many times a @@ -341,19 +375,8 @@ func Check(m *Meta, values []string, occurrences int) *Error { return &Error{Code: code, Name: m.Name, Spelling: m.Spelling} } - accepted := m.AcceptedChoices - if len(accepted) == 0 { - accepted = m.Choices - } - if len(accepted) > 0 && !m.AllowUnknownChoices { - // Every value, not just the first: a variadic can be given a good value - // and a bad one, and so can a repeatable flag across occurrences. - for _, v := range values { - if !containsChoice(accepted, v, m.IgnoreCase) { - return &Error{Code: CodeInvalidChoice, Name: m.Name, - Spelling: m.Spelling, Choices: m.Choices} - } - } + if err := CheckDisplaced(m, values); err != nil { + return err } if m.Validate != "" { diff --git a/go/argv/post_test.go b/go/argv/post_test.go index e05fb0a33..ea07ed452 100644 --- a/go/argv/post_test.go +++ b/go/argv/post_test.go @@ -174,6 +174,31 @@ func TestCheckRichChoices(t *testing.T) { } } +// The corpus's `overrides-do-not-erase-an-invalid-choice`. +// +// A flag that lost an `overrides` holds nothing, so every other rule in [Check] +// has nothing to judge — but `overrides` settles which of a pair is in effect, +// not whether the word the loser was handed was one it accepts. +func TestCheckDisplacedJudgesOnlyTheWords(t *testing.T) { + meta := &Meta{Name: "log-level", Spelling: "--log-level", + Choices: []string{"debug", "info"}, Required: true} + + if err := CheckDisplaced(meta, []string{"v"}); err == nil || err.Code != CodeInvalidChoice { + t.Fatalf("a word outside the choices survives the displacement: %+v", err) + } + if err := CheckDisplaced(meta, []string{"info"}); err != nil { + t.Fatalf("a word among the choices is displaced in silence: %v", err) + } + // The half that does *not* survive: a loser is not missing, it is out of the + // running. Reporting it would undo the last-one-wins the user asked for. + if err := CheckDisplaced(meta, nil); err != nil { + t.Fatalf("a loser that was never typed has nothing to answer for: %v", err) + } + if err := CheckDisplaced(nil, []string{"v"}); err != nil { + t.Fatalf("an unknown entry declares no choices: %v", err) + } +} + func TestCheckPortableValidation(t *testing.T) { meta := &Meta{ Name: "port", diff --git a/go/conformance/conformance_test.go b/go/conformance/conformance_test.go index ed1e07661..939495841 100644 --- a/go/conformance/conformance_test.go +++ b/go/conformance/conformance_test.go @@ -354,7 +354,14 @@ func run(s *spec.Spec, args []string, argv0 *string, env map[string]string) (*Pa for key, b := range got { order[key] = b.at } + // Kept even though the binding is dropped: a loser is not judged on what it + // *holds* — it holds nothing — but the words it was typed are still its own, + // and `choices` is asked about them below. + displaced := map[uint64][]string{} for key := range argv.ApplyOverrides(meta, order) { + if b := got[key]; b != nil { + displaced[key] = b.values + } delete(got, key) lost[key] = true } @@ -397,20 +404,29 @@ func run(s *spec.Spec, args []string, argv0 *string, env map[string]string) (*Pa return r } + // Every entry the command declares, in declaration order, losers included. + // `scope` is what has a final state to compare and fill; this is the order the + // per-entry pass reports failures in, and a loser has one question left to + // answer even though it has no state. + var ordered []uint64 + for _, cmd := range path { for _, f := range cmd.Flags { + ordered = append(ordered, f.Key) // A flag that lost an override is out of the running rather than - // merely absent: it is not filled from `env` or `default`, and it is - // not judged either. A `required` loser reported as missing would undo - // the last-one-wins the user asked for by typing the other flag, and - // usage-lib skips overridden flags in the requirement pass for exactly - // that reason. + // merely absent: it is not filled from `env` or `default`, and the + // rules that judge what it *holds* do not apply. A `required` loser + // reported as missing would undo the last-one-wins the user asked for + // by typing the other flag, and usage-lib skips overridden flags in the + // requirement pass for exactly that reason. Its `choices` are still + // asked about the words it was typed — see [argv.CheckDisplaced]. if lost[f.Key] { continue } fill(f.Key, f.TakesValue).flag = f } for _, a := range cmd.Args { + ordered = append(ordered, a.Key) fill(a.Key, true).arg = a } } @@ -432,7 +448,16 @@ func run(s *spec.Spec, args []string, argv0 *string, env map[string]string) (*Pa } // What one entry ended up with, judged on its own. - for _, key := range scope { + for _, key := range ordered { + if lost[key] { + // Nothing to end up with, so only the words it was typed are left to + // judge. In declaration order with the rest, so which failure a line + // with more than one fault reports does not depend on who won a pair. + if err := argv.CheckDisplaced(meta.Lookup(key), displaced[key]); err != nil { + return nil, err + } + continue + } r := final[key] occurrences := r.occurrences if r.arg != nil {