fix(derive): let a `--`-only argument follow a variadic by jdx · Pull Request #823 · jdx/usage

jdx · 2026-08-12T02:20:00Z

Stacked on #821.

The derive refused any argument after a variadic, on the grounds that a variadic takes every remaining word. True except in the one case that matters: an argument only fillable after a --, since the -- is what stops the variadic.

mise declares exactly that on run, exec and git:

arg "[ARGS]…"      help="Arguments to pass to the tasks. Use \":::\" to separate tasks" var=#true
arg "[-- ARGS_LAST]…" var=#true hide=#true

So the check was refusing a shape three of mise's commands depend on. Both parsers were already right — usage-argv binds it, usage-lib binds it — only the derive's validation disagreed.

How it was found

By generating a shadow CLI from mise's committed 5,592-line spec and trying to compile it. Four commands failed, and the parser was not at fault. That's what the shadow exercise is for, and it's the reason to do it before touching mise rather than after.

Two corpus vectors pin the behaviour, with and without the separator, and a derive test checks the emitted spec still says which argument is which — otherwise docs and completions would describe a different CLI from the one that runs.

AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.

Note

Medium Risk
Touches derive validation and key generation for all derived CLIs; behavior is corrective but key values change and incorrect validation could accept unreachable argument shapes.

Overview
Allows a double_dash = "required" argument to follow a variadic in derive validation. The old rule rejected any argument after a variadic; the -- that such an argument requires is exactly what stops the first variadic, which mise needs on run/exec/git. Anything after a separator-consuming variadic is still rejected.

Folds module_path!() into generated keys via a new usage_argv::key_base, so byte-identical declarations in different modules no longer collide. That collision used to fail Spec::to_kdl's uniqueness assert on otherwise valid CLIs. Codegen now emits named key consts (needed for match patterns) instead of inlining the hash.

Adds corpus vectors and derive/identity tests for both behaviors, plus a note that var_max is a post-binding check and does not hand leftover words to the next argument.

^{Reviewed by Cursor Bugbot for commit dd0436c. Bugbot is set up for automated code reviews on this repo. Configure here.}

The check refused any argument after a variadic, on the grounds that the variadic takes every remaining word. That is true except of the one case that matters: an argument only fillable after a `--`, because the `--` is what stops the variadic. mise declares exactly that on `run`, `exec` and `git` — `[ARGS]…` for the words before the separator and `[-- ARGS_LAST]…` for the ones after — so the check was refusing a shape three of its commands rely on. usage-argv already bound it correctly and so does usage-lib; only the derive's own validation disagreed. Found by generating a shadow CLI from mise's committed spec, which is what that exercise is for: four commands failed to compile and the parser was not the one at fault. Two corpus vectors pin the behaviour, with and without the separator.

coderabbitai · 2026-08-12T02:20:14Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: f0a1539e-d25a-49a9-8fe3-c357a95060c8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

greptile-apps · 2026-08-12T02:22:20Z

Greptile Summary

The PR aligns derive validation with usage-argv’s separator and variadic semantics while preventing generated-key collisions across modules.

Allows one double_dash = "required" argument after an ordinary variadic.
Rejects arguments after a separator-consuming variadic and documents post-binding var_max behavior.
Mixes module_path!() into generated key bases and adds collision coverage.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
derive/src/model.rs	Validation now permits the reachable pre-/post-separator variadic layout while rejecting arguments after a separator-consuming variadic.
derive/src/codegen.rs	Generated keys now use module-aware const key bases consistently across command, flag, and argument tables and match arms.
argv/src/lib.rs	Adds a const module-and-declaration hash function used to namespace generated keys.
conformance/tests/derive.rs	Covers parsing and emitted-spec behavior for variadics split by a required separator.
conformance/tests/identity.rs	Verifies identical declarations in separate modules receive noncolliding keys and bind independently.
corpus/06-double-dash.json	Adds parser vectors for variadics before and after a separator.
corpus/08-choices-and-required.json	Establishes that usage-argv treats positional `var_max` as post-binding validation rather than a cursor limit.
docs/spec/argv.md	Documents greedy variadic binding and post-binding minimum and maximum checks.

_{Reviews (5): Last reviewed commit: "test(derive): make the identity test pro..." | Re-trigger Greptile}

github-actions · 2026-08-12T02:28:22Z

Instruction counts

benchmark	trend	instructions	Δ	wall (min)	Δ
markdown	`▁▁▁▁▁▁▁█████████`	148,242,482 → 148,229,539	-0.01%	13.93 → 13.74ms	-1.33%
startup	`▄▄▄▄▄▄▄█████▁▁▁▅`	1,199,527 → 1,201,969	+0.20%	0.95 → 0.94ms	-0.96%

No instruction-count regression above 1%.

_{Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run.}

_{Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes.}

_{dd0436c3af65 vs 7047ae1232b2 · measured on the runner, not pushed to the history.}

An argument may follow a variadic when it requires a `--`, because the separator is what stops the variadic. Review pointed out that the exemption was written as a property of each argument rather than of the command line: two variadics behind a `--` both passed the check, and the first would take every word after the only separator there is, leaving the second unreachable — the very thing the rule exists to refuse. So a variadic that takes what follows the separator now ends the list: nothing may come after it, variadic or not.

jdx · 2026-08-12T10:31:48Z

Valid, and the framing was the useful part: I had written the exemption as a property of each argument when it is a property of the command line. Two variadics behind a -- both passed, and the first would take every word after the only separator there is — exactly what the rule exists to refuse.

So a variadic that takes what follows the separator now ends the list: nothing may come after it, variadic or not. Two unit tests, one for a second ---required variadic and one for a plain argument trailing it.

AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.

Review asked whether a `--`-required variadic with a finite `var_max` frees the argument after it: the parser would advance once the bound was reached, so the layout would be legal and the new check wrong to refuse it. Measured instead of argued. usage-argv gives the variadic both words and leaves the next argument empty; **usage-lib stops at `var_max` and fills the next argument**. The two have disagreed all along and nothing recorded it. The grammar sides with usage-argv — a variadic "collects every word still available", and `var_min`/`var_max` are checks on what it got — so the check stays, and the grammar now says so in as many words. The divergence is a corpus vector rather than a silent difference, with a note that clap behaves the way usage-lib does, which makes this a question about the grammar rather than a bug on either side.

…ffer Review called this out on the nesting PR and it reproduces: two commands declared byte-for-byte alike in different modules — an `add::Op` and a `remove::Op` — share a key, and `Spec::to_kdl` asserts that no two things in a CLI do. A correct CLI failed the assertion, in the debug build tests run in. The old comment argued the residue was harmless because a key only chooses which `match` arm to jump to and each arm checks the event came from its own table. That much is true, and it is why nothing ever misbound; what it missed is the safety net above it, which called a legitimate CLI a collision. A macro cannot see a module path, but the generated code can: the declaration's hash is now mixed with `module_path!()` in a `const fn`, so the two halves of a key are decided in the two places that can each see half. Keys become named `const` items because a key is a `match` pattern as well as a table field, and a pattern cannot be an expression — `BASE | KIND_FLAG | 0` would parse as an or-pattern. A test declares the two identical commands and checks both that the spec has no duplicate keys and that each command still binds its own flag.

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 80daef3. Configure here.}

The two commands it declares were not identical: their doc comments differed, and the fingerprint hashes the whole item, attributes included. So their keys already diverged and the test passed whether or not the module was folded in — it asserted the fix while demonstrating nothing. They are byte-for-byte identical now, and I checked the test fails without the fix: stub the module out of `key_base` and it reports two things sharing a key, which is what it is supposed to notice.

jdx · 2026-08-12T11:07:09Z

The var_max finding is re-posted from the earlier commit; it's answered in the previous round rather than outstanding. Short version, since it turned out to be more interesting than a yes/no: I measured it, and usage-argv and usage-lib disagree — usage-argv gives the variadic every remaining word, usage-lib stops it at var_max and fills the next argument. The grammar sides with usage-argv ("collects every word still available"), so the check stays and docs/spec/argv.md now says a bound is a check rather than a limit. The divergence is recorded as a-bound-does-not-stop-a-variadic in the corpus, with a note that clap behaves the way usage-lib does — so which one is right is a grammar question for @jdx, not a bug on either side.

The identity-test finding was right and worth more than its severity. add::Op and remove::Op were not byte-identical — their doc comments differed, and the fingerprint hashes the whole item — so their keys already diverged and the test passed with or without the fix. It asserted the fix while demonstrating nothing. They're identical now, and I checked it fails when the module is stubbed out of key_base: "two things in this CLI share a key", which is exactly what it should notice.

AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.

) Stacked on #823. This is the first half of the gate — the thing the whole experiment is supposed to be judged by. `xtask gen-shadow <spec.kdl> <dir>` reads a usage spec and writes a crate of derived types declaring the same CLI. **mise's committed 5,592-line spec compiles**: 211 commands, 711 flags, 128 arguments, four levels deep, in 2.6 seconds. ## What it drops, and says so ``` dropped, because the derive cannot express it yet: `double_dash = "automatic"` on an argument: 3 a default on a flag that collects values: 1 a flag's second long or short form: 13 ``` Seventeen out of roughly a thousand. The generator names each class and counts it, because a silent cap reads as "all of it was expressible". ## A first measurement The gate parses one command line and exits, beside a null binary that does everything *except* parse. Subtracting the second from the first is what turns a process measurement into a parser measurement — about two thirds of a small binary's instructions are the dynamic loader and libc starting up, and neither parser is responsible for those. | invocation | instructions to parse | | --- | --- | | `mise` | 99,281 | | `mise use -g node@20` | 104,720 | | `mise settings set experimental true` | 104,650 | | `mise -vv ls --installed` | 105,754 | | `mise tasks run build -- --verbose` | 104,650 | Near constant regardless of depth, which is what static tables should look like: there is no tree to build. **The clap side is the next PR** — until it lands, this number stands on its own rather than as a ratio, and the ~3.1M figure from the original analysis is not something I have reproduced here. ## Two things worth review - **The generated crate is excluded from the workspace.** A shadow of mise trips `large_enum_variant` on the commands with thirty flags, which the real mise answers by boxing its variants — something the derive cannot express yet, so it's on the roadmap rather than papered over with an `allow`. The gate depends on the shadow by path so it is still built, and the smoke tests live in the gate crate, where they get linted like anything else. - **The shadow is checked in, not built by a `build.rs`.** The compile-time comparison wants a fixed input, and a generated file in the tree is a diff a reviewer can read when the derive's vocabulary changes. CI runs `mise run gen-shadow` and fails on a diff, the same way it does for `render`. Along the way, a note on fidelity: mise's spec declares **no positionals on the top-level `run`**, and that is deliberate — `src/cli/usage.rs` clears them (`run.args = vec![]`) and adds a `mount` of `mise tasks --usage` plus `restart_token = ":::"`, so the project's real tasks are what complete after `run` rather than a static `[TASK]`. The shadow can carry neither the mount nor the restart token yet, so its `run` is slightly cheaper to parse than real mise's would be. The smoke tests use `tasks run` for the separator shape and the root's own `[TASK]` for a bare task. (I first suspected the clap bridge was dropping them, since `Run(Box<run::Run>)` is the only boxed variant of the commands involved and the same struct registered unboxed under `tasks run` kept all three positionals. I tested that against clap 4 directly: boxing loses nothing, in clap or in the bridge. Recording it here because it is a tempting wrong answer.) *AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.*  --- > [!NOTE] > **Medium Risk** > Large generated shadow and new workspace/CI wiring for the gate; not security-critical, but the shadow exercises the derive at full mise scale and CI will fail on generator drift. > > **Overview** > Adds the first half of the **performance gate**: generate a compileable shadow of a real CLI from its usage spec, then measure parsing at mise's full scale. > > **`xtask gen-shadow`** turns any `.usage.kdl` into a crate of derived types. mise's checked-in 5,592-line spec compiles (211 commands, 711 flags, 128 args). Features the derive cannot express yet are counted and reported rather than silently dropped. The generated `shadow-mise` crate is committed and **excluded from the workspace** (it trips `large_enum_variant`); the gate depends on it by path. > > The new **`gate`** crate provides one-shot parse binaries (`parse-usage` vs a null `parse-none` baseline) plus smoke tests for real mise invocations — including the `[ARGS]…` / `[-- ARGS_LAST]…` separator shape and nested/global-flag cases. > > CI now runs `mise r gen-shadow` and fails if the checked-in shadow drifts, same as `render`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 4470be0. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>

Stacked on #825. Settles the question left open in #823, the way you called it. ## What was open `var_max` had two coherent readings, and the two implementations had each quietly picked one. For `arg "[a]" var=#true var_max=1` then `arg "[b]"`, given `ex x y`: | | result | | --- | --- | | usage-argv (before) | `a = ["x","y"]`, then `var_too_many` | | usage-lib | `a = ["x"]`, `b = "y"` | | clap (`num_args = 0..=1`) | same as usage-lib | ## The decision **A limit, not a check.** clap behaves that way, every spec in the fleet is generated from a clap command, and it is the only reading under which `[a]… [b]` can be filled at all. So `var_max` moves into the hot `Arg`/`Flag` tables — the ones kept deliberately free of anything but what binding needs — as `Option<u32>`, and the parser counts what a variadic has taken. `var_min` stays a post-binding check, since no single word tells you a variadic will end up short. `var_too_many` accordingly stops being reachable for a bounded variadic and now describes only a repeatable flag's occurrences. **The cost: 55 instructions per parse, 0.1%.** Wall clock unchanged at 2.0µs. That was the thing worth checking, since the objection to this reading was that it puts a validation concept in the binder. ## What follows from it - **The derive's rule relaxes.** An argument after an *unbounded* variadic is still refused; after a bounded one it is allowed, and a bounded variadic behind a `--` no longer spends the separator. The error message now names both things that stop a variadic, so it points at a fix rather than just a refusal. - **Four corpus vectors**, at the **binding** layer where the question now lives: the bound hands over, the bound with nothing after it is an `unexpected_arg` rather than a silent drop, unbounded still takes everything, and the flag form. - **One divergence recorded rather than fixed**: for a flag whose single occurrence collects, usage-lib does not collect at all — it takes one value, so the second word falls through to the positional. That predates this change (`long-variadic-flag-arg` documents the same gap) and bounding what usage-argv collects is consistent with it collecting in the first place. `docs/spec/argv.md` states the rule, and PLAN.md records the decision with its reasoning so it does not get re-litigated. *AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.*  --- > [!NOTE] > **Medium Risk** > Changes core argv binding semantics and parser hot-path behavior; mistakes could mis-route tokens or break CLIs relying on the old post-binding `var_max` for variadics, though coverage is broad. > > **Overview** > Settles **`var_max` as a binding limit**, not a post-parse count check, so bounded variadics stop collecting and following positionals can be filled (clap `num_args` / usage-lib behavior). > > **usage-argv** adds `var_max: Option<u32>` on `Flag` and `Arg`, tracks per-occurrence counts (`collected` / `arg_taken`), resets counts when advancing positionals or jumping past `--`, and stops variadic flag collection when the bound is hit. > > **usage-derive** emits `var_max` into hot tables for variadic args/flags only; compile-time rules allow an argument after a **bounded** variadic (or after `--`); post-binding `var_too_many` applies only to **repeatable** flags counting occurrences. > > **Conformance bridge**, corpus vectors, spec docs, and **PLAN.md** are updated (including revised bench numbers); bounded variadic-flag behavior vs usage-lib remains a recorded divergence. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 285503b. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>   ## Summary by CodeRabbit * **New Features** * Added maximum value limits for variadic flags and positional arguments. * Bounded variadic arguments can pass remaining values to subsequent arguments. * Added conflict detection for incompatible flags. * Documented `var_max` behavior and clarified validation rules. * **Bug Fixes** * Corrected variadic value collection, overflow handling, inline values, and parser state resets. * Preserved minimum-value validation after parsing. * Improved handling around separators and repeatable flag occurrences. * **Tests** * Expanded coverage for bounded and unbounded variadics, positional fallthrough, overflow, and double-dash scenarios.

greptile-apps Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread derive/src/model.rs

jdx mentioned this pull request Aug 12, 2026

feat(bench): generate a shadow CLI from a spec, and compile mise's #824

Merged

greptile-apps Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread derive/src/model.rs

jdx added 2 commits August 12, 2026 10:38

cursor Bot reviewed Aug 12, 2026

View reviewed changes

Comment thread conformance/tests/identity.rs

This was referenced Aug 12, 2026

feat(derive): nest commands to any depth #818

Merged

feat(argv): a bound stops a variadic #826

Merged

jdx merged commit 22cd8cb into main Aug 12, 2026
9 checks passed

jdx deleted the agent/derive-double-dash branch August 12, 2026 13:57

mise-en-dev mentioned this pull request Aug 12, 2026

chore: release v6.0.0 #795

Merged

BrewTestBot mentioned this pull request Aug 22, 2026

usage 6.0.0 Homebrew/homebrew-core#300125

Merged

Navigation Menu

Uh oh!

fix(derive): let a `--`-only argument follow a variadic - #823

fix(derive): let a `--`-only argument follow a variadic#823
jdx merged 5 commits into
agent/derive-overridesfrom
agent/derive-double-dash

jdx commented Aug 12, 2026 •

edited by cursor Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading

Review skipped

Uh oh!

greptile-apps Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

Uh oh!

github-actions Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

jdx commented Aug 12, 2026

Uh oh!

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

jdx commented Aug 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

jdx commented Aug 12, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How it was found

Uh oh!

coderabbitai Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

greptile-apps Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

Uh oh!

github-actions Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instruction counts

Uh oh!

jdx commented Aug 12, 2026

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jdx commented Aug 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jdx commented Aug 12, 2026 •

edited by cursor Bot

Loading

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading

greptile-apps Bot commented Aug 12, 2026 •

edited

Loading

github-actions Bot commented Aug 12, 2026 •

edited

Loading