feat(argv): add a zero-allocation argv parser - #798
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds the Changesusage-argv parser
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Corpus as conformance corpus
participant Runner as conformance::argv::run
participant Parser
Corpus->>Runner: provide Vector
Runner->>Parser: construct parser tables and parse argv
Parser-->>Runner: emit Event or Error
Runner-->>Corpus: return Outcome
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Instruction counts
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.
|
Contributor
Greptile SummaryThe PR adds the zero-allocation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; both previously reported short-bundle defects are fixed in the current code. Important Files Changed
Reviews (3): Last reviewed commit: "chore(argv): release usage-argv on the s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
usage-argv implements the binding half of the argv grammar: which token becomes which flag or argument, when a word routes to a subcommand, and what is an error. It builds no command tree, allocates nothing on any outcome, and reads argv once. The tables are borrowed slices so a derive can emit them as static data, and parsing yields events rather than a map — generated code can assign an event straight into a struct field, which is both faster and the reason no allocation is needed. Values come back as bytes borrowed from argv, so a non-UTF-8 command line still parses and only the values actually inspected can fail to convert. Scope is binding only. required, choices, env, defaults, var_min/var_max and overrides are decided after the last token and need a value's type, so they belong to the layer that owns the target struct. Keeping them out is what makes the loop small. Verified two ways: 61 of the corpus's 83 vectors run through this parser (the other 22 are post-binding, and that count is asserted), and a counting global allocator proves both successful and failing parses allocate zero times. All 15 vectors where usage-lib diverges from the grammar are among the ones this parser gets right. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The corpus gained a vector for `double_dash="automatic"`, which is a binding rule — it decides whether a later token is a flag or a value — so it belongs in this parser rather than in the layer above. Once an argument in that mode takes a value, flag interpretation stops as though the caller had typed the separator. usage-argv now answers 62 of the corpus's 86 vectors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both review bots caught the same pair of bugs, independently. `-fz` emitted the `-f` event and only then discovered that `z` matches nothing, so a rejected command line left an earlier flag applied. Events go out one at a time, so the fix is to walk the whole token first and refuse it before binding anything; the scan stops at the first value-taking letter, since everything after that is its value. The error also carried an empty slice: `self.bundle` was cleared before the token was read out of it. The whole token as typed is now kept for the length of the bundle, so the error names `-fz` — which is also the unit in which it is rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Publishes usage-argv as an ordinary member of the workspace at the shared version rather than holding it at 0.0.0 outside the release cycle. The release script needed four adjustments to make that correct rather than half-true: publish the crate (and catch it up the way clap_usage is), include argv/ when computing the version bump and the changelog so an argv-only change can cut a release, stage its bumped manifest — the commit listed only root, cli, and lib, so the bump would have been dropped — and exclude usage-conformance from the bump, since the harness is never published and does not need a version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
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 180aec7. Configure here.
| // the bare name. | ||
| negate: f.negate.as_ref().map(|n| leak(n.trim_start_matches('-'))), | ||
| takes_value: f.arg.is_some(), | ||
| var: f.var || f.arg.as_ref().is_some_and(|a| a.var), |
There was a problem hiding this comment.
Repeatable flag triggers greedy collection
Medium Severity
When building Flag tables, var is set from both spec var=#true and a variadic flag argument (...), but the parser treats Flag.var as greedy multi-value collection for a single flag occurrence. A repeatable flag like --include <pattern> with var=#true should take one value per occurrence; tokens after the first value should go to positionals, not keep filling the same flag.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 180aec7. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
argv/src/lib.rs (1)
624-627: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit coverage for variadic flags and for
Error::TooDeep.No test in this module declares a flag with
var: true, so thecollectingpath instep(Lines 344-356) and the two assignment sites (Lines 421 and 492) are unit-tested only through the conformance crate. No test reachesMAX_DEPTHeither, soError::TooDeepis unexercised. Both paths carry the divergence flagged at Lines 421-423.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@argv/src/lib.rs` around lines 624 - 627, Add unit tests in the existing tests module covering flags configured with var: true, exercising the collecting path in step and both assignment sites, including the expected variadic parsing behavior. Add a separate test that constructs nesting beyond MAX_DEPTH and asserts Error::TooDeep, preserving existing test conventions and validating the divergence-prone paths directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@argv/src/lib.rs`:
- Around line 421-423: Update the long-flag handling around the collecting
assignment so collecting is set only when the flag’s takes_value property is
true, matching the short_flag path. Ensure var-only flags that do not take
values leave collecting unset and do not consume following words or emit a
value.
- Around line 307-310: Correct the state exposed by Args::double_dash_seen so it
reports true only when the user explicitly supplied a -- separator, rather than
when word applies DoubleDash::Automatic and sets self.double_dash. Track
explicit consumption separately from automatic flag interpretation, and have the
accessor return that explicit-separator state while preserving existing parsing
behavior.
- Around line 1069-1079: Update non_utf8_values_still_parse to construct a
command-line argument containing actual invalid UTF-8 bytes as a value, rather
than using the valid "--force" flag. Assert that parse accepts and binds this
non-UTF-8 value, while retaining the as_str rejection assertion for invalid
bytes.
In `@conformance/src/argv.rs`:
- Around line 57-59: Update the out_of_scope check around out_of_scope so
post-binding declarations are evaluated only after token routing identifies the
selected command path, including applicable inherited flags. Avoid scanning
declarations from unselected subcommands, while preserving the existing
Outcome::OutOfScope behavior for declarations that apply to the selected path.
In `@conformance/tests/argv.rs`:
- Around line 28-53: Update the corpus loop in conformance/tests/argv.rs around
corpus() and Outcome::OutOfScope to count out-of-scope results and directly
assert that the count is 24, while preserving the existing in-scope and failure
assertions. In docs/spec/argv.md lines 248-253, keep the documented
excluded-vector count aligned with this 24-vector assertion.
---
Nitpick comments:
In `@argv/src/lib.rs`:
- Around line 624-627: Add unit tests in the existing tests module covering
flags configured with var: true, exercising the collecting path in step and both
assignment sites, including the expected variadic parsing behavior. Add a
separate test that constructs nesting beyond MAX_DEPTH and asserts
Error::TooDeep, preserving existing test conventions and validating the
divergence-prone paths directly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 83d2c706-4154-40d9-9ee6-26544fe0b6c2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlargv/Cargo.tomlargv/src/lib.rsargv/tests/no_alloc.rsconformance/Cargo.tomlconformance/src/argv.rsconformance/src/lib.rsconformance/tests/argv.rsdocs/spec/argv.mdtasks/release-plz
Comment on lines
+307
to
+310
| /// Whether a `--` has been consumed as a separator. | ||
| pub fn double_dash_seen(&self) -> bool { | ||
| self.double_dash | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
double_dash_seen reports true when no -- was given.
word sets self.double_dash for DoubleDash::Automatic (Line 540). The accessor then returns true although the user typed no separator, which contradicts the documented meaning. A caller that reproduces the command line, or that reports how a value was accepted, gets the wrong answer.
Track the two states apart, or document the accessor as "flag interpretation has stopped".
♻️ Proposed direction
- /// Whether a `--` has been consumed as a separator.
+ /// Whether flag interpretation has stopped, either because a `--` was
+ /// consumed or because a `double_dash = "automatic"` argument took a value.
pub fn double_dash_seen(&self) -> bool {
self.double_dash
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Whether a `--` has been consumed as a separator. | |
| pub fn double_dash_seen(&self) -> bool { | |
| self.double_dash | |
| } | |
| /// Whether flag interpretation has stopped, either because a `--` was | |
| /// consumed or because a `double_dash = "automatic"` argument took a value. | |
| pub fn double_dash_seen(&self) -> bool { | |
| self.double_dash | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@argv/src/lib.rs` around lines 307 - 310, Correct the state exposed by
Args::double_dash_seen so it reports true only when the user explicitly supplied
a -- separator, rather than when word applies DoubleDash::Automatic and sets
self.double_dash. Track explicit consumption separately from automatic flag
interpretation, and have the accessor return that explicit-separator state while
preserving existing parsing behavior.
Comment on lines
+421
to
+423
| if flag.var { | ||
| self.collecting = Some(flag); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set collecting only when the flag takes a value.
long_flag sets collecting for any var flag, but short_flag (Line 492) sets it only inside the takes_value branch. A flag declared with var: true and takes_value: false therefore binds differently through its long form than through its short form: the long form swallows the following words and emits Event::Flag { value: Some(..) } for a flag that declares no value.
Align the two paths.
🐛 Proposed fix
- if flag.var {
+ if flag.takes_value && flag.var {
self.collecting = Some(flag);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if flag.var { | |
| self.collecting = Some(flag); | |
| } | |
| if flag.takes_value && flag.var { | |
| self.collecting = Some(flag); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@argv/src/lib.rs` around lines 421 - 423, Update the long-flag handling around
the collecting assignment so collecting is set only when the flag’s takes_value
property is true, matching the short_flag path. Ensure var-only flags that do
not take values leave collecting unset and do not consume following words or
emit a value.
Comment on lines
+1069
to
+1079
| #[test] | ||
| fn non_utf8_values_still_parse() { | ||
| // A value that is not valid UTF-8 binds; only converting it fails, and | ||
| // only if a caller asks. | ||
| let raw = OsStr::new("--force"); | ||
| let a = [raw]; | ||
| assert!(parse(&ROOT, &a).is_ok()); | ||
|
|
||
| assert!(as_str(b"ok").is_ok()); | ||
| assert!(as_str(&[0xff, 0xfe]).is_err()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The test does not use a non-UTF-8 command line.
raw is "--force", which is valid UTF-8 and binds as a flag, not as a value. The assertion at Line 1075 passes regardless of the property the test names. Feed real non-UTF-8 bytes and assert that the value binds while as_str rejects it.
💚 Proposed fix
#[test]
fn non_utf8_values_still_parse() {
// A value that is not valid UTF-8 binds; only converting it fails, and
// only if a caller asks.
- let raw = OsStr::new("--force");
- let a = [raw];
- assert!(parse(&ROOT, &a).is_ok());
+ #[cfg(unix)]
+ {
+ use std::os::unix::ffi::OsStrExt;
+ let raw = OsStr::from_bytes(&[b'a', 0xff, b'z']);
+ let a = [raw];
+ let events = parse(&ROOT, &a).unwrap();
+ let Event::Arg { value, .. } = events[0] else {
+ panic!("expected an arg");
+ };
+ assert_eq!(value, &[b'a', 0xff, b'z'][..]);
+ assert!(as_str(value).is_err());
+ }
assert!(as_str(b"ok").is_ok());
assert!(as_str(&[0xff, 0xfe]).is_err());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[test] | |
| fn non_utf8_values_still_parse() { | |
| // A value that is not valid UTF-8 binds; only converting it fails, and | |
| // only if a caller asks. | |
| let raw = OsStr::new("--force"); | |
| let a = [raw]; | |
| assert!(parse(&ROOT, &a).is_ok()); | |
| assert!(as_str(b"ok").is_ok()); | |
| assert!(as_str(&[0xff, 0xfe]).is_err()); | |
| } | |
| #[test] | |
| fn non_utf8_values_still_parse() { | |
| // A value that is not valid UTF-8 binds; only converting it fails, and | |
| // only if a caller asks. | |
| #[cfg(unix)] | |
| { | |
| use std::os::unix::ffi::OsStrExt; | |
| let raw = OsStr::from_bytes(&[b'a', 0xff, b'z']); | |
| let a = [raw]; | |
| let events = parse(&ROOT, &a).unwrap(); | |
| let Event::Arg { value, .. } = events[0] else { | |
| panic!("expected an arg"); | |
| }; | |
| assert_eq!(value, &[b'a', 0xff, b'z'][..]); | |
| assert!(as_str(value).is_err()); | |
| } | |
| assert!(as_str(b"ok").is_ok()); | |
| assert!(as_str(&[0xff, 0xfe]).is_err()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@argv/src/lib.rs` around lines 1069 - 1079, Update non_utf8_values_still_parse
to construct a command-line argument containing actual invalid UTF-8 bytes as a
value, rather than using the valid "--force" flag. Assert that parse accepts and
binds this non-UTF-8 value, while retaining the as_str rejection assertion for
invalid bytes.
Comment on lines
+57
to
+59
| if let Some(reason) = out_of_scope(&spec, &vector.expect) { | ||
| return Outcome::OutOfScope(reason); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Limit out-of-scope checks to the selected command path.
out_of_scope runs before token routing. declares_post_binding then scans every subcommand. A default, choice, or requirement in an unselected subcommand causes this runner to skip a vector that usage-argv can bind.
Evaluate post-binding declarations only for the selected command path and applicable inherited flags. This prevents valid binding vectors from being excluded and hiding parser regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@conformance/src/argv.rs` around lines 57 - 59, Update the out_of_scope check
around out_of_scope so post-binding declarations are evaluated only after token
routing identifies the selected command path, including applicable inherited
flags. Avoid scanning declarations from unselected subcommands, while preserving
the existing Outcome::OutOfScope behavior for declarations that apply to the
selected path.
Comment on lines
+28
to
+53
| for vector in corpus() { | ||
| let outcome = run(&vector); | ||
| if let Outcome::OutOfScope(_) = outcome { | ||
| continue; | ||
| } | ||
| in_scope += 1; | ||
| if !outcome.matches(&vector.expect) { | ||
| failures.push(format!( | ||
| "{}: {}\n expected: {:?}\n got: {outcome:?}", | ||
| vector.id, vector.doc, vector.expect | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| assert!( | ||
| failures.is_empty(), | ||
| "{} binding vector(s) failed:\n - {}", | ||
| failures.len(), | ||
| failures.join("\n - ") | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| in_scope, IN_SCOPE, | ||
| "the number of vectors usage-argv answers changed; if that was the point \ | ||
| of your change, update IN_SCOPE" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the out-of-scope count directly.
The test only asserts that 62 vectors are in scope. If the corpus adds an out-of-scope vector, this assertion can still pass. This does not enforce the documented 24-vector exclusion set.
conformance/tests/argv.rs#L28-L53: countOutcome::OutOfScoperesults and assert that the count is 24.docs/spec/argv.md#L248-L253: keep the documented count aligned with the direct test assertion.
Proposed test change
const IN_SCOPE: usize = 62;
+const OUT_OF_SCOPE: usize = 24;
let mut failures = Vec::new();
let mut in_scope = 0;
+ let mut out_of_scope = 0;
- if let Outcome::OutOfScope(_) = outcome {
+ if let Outcome::OutOfScope(_) = outcome {
+ out_of_scope += 1;
continue;
}
+ assert_eq!(out_of_scope, OUT_OF_SCOPE);
assert_eq!(in_scope, IN_SCOPE);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for vector in corpus() { | |
| let outcome = run(&vector); | |
| if let Outcome::OutOfScope(_) = outcome { | |
| continue; | |
| } | |
| in_scope += 1; | |
| if !outcome.matches(&vector.expect) { | |
| failures.push(format!( | |
| "{}: {}\n expected: {:?}\n got: {outcome:?}", | |
| vector.id, vector.doc, vector.expect | |
| )); | |
| } | |
| } | |
| assert!( | |
| failures.is_empty(), | |
| "{} binding vector(s) failed:\n - {}", | |
| failures.len(), | |
| failures.join("\n - ") | |
| ); | |
| assert_eq!( | |
| in_scope, IN_SCOPE, | |
| "the number of vectors usage-argv answers changed; if that was the point \ | |
| of your change, update IN_SCOPE" | |
| ); | |
| let mut failures = Vec::new(); | |
| let mut in_scope = 0; | |
| let mut out_of_scope = 0; | |
| for vector in corpus() { | |
| let outcome = run(&vector); | |
| if let Outcome::OutOfScope(_) = outcome { | |
| out_of_scope += 1; | |
| continue; | |
| } | |
| in_scope += 1; | |
| if !outcome.matches(&vector.expect) { | |
| failures.push(format!( | |
| "{}: {}\n expected: {:?}\n got: {outcome:?}", | |
| vector.id, vector.doc, vector.expect | |
| )); | |
| } | |
| } | |
| assert!( | |
| failures.is_empty(), | |
| "{} binding vector(s) failed:\n - {}", | |
| failures.len(), | |
| failures.join("\n - ") | |
| ); | |
| assert_eq!(out_of_scope, OUT_OF_SCOPE); | |
| assert_eq!( | |
| in_scope, IN_SCOPE, | |
| "the number of vectors usage-argv answers changed; if that was the point \ | |
| of your change, update IN_SCOPE" | |
| ); |
📍 Affects 2 files
conformance/tests/argv.rs#L28-L53(this comment)docs/spec/argv.md#L248-L253
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@conformance/tests/argv.rs` around lines 28 - 53, Update the corpus loop in
conformance/tests/argv.rs around corpus() and Outcome::OutOfScope to count
out-of-scope results and directly assert that the count is 24, while preserving
the existing in-scope and failure assertions. In docs/spec/argv.md lines
248-253, keep the documented excluded-vector count aligned with this 24-vector
assertion.
jdx
added a commit
that referenced
this pull request
Aug 11, 2026
Two review findings from #798. A flag declared `var=#true` with a single-value argument is repeatable — one value per occurrence — while a flag with a variadic argument (`<pattern>...`) is greedy. The conformance harness set the parser's flag from either, so `--include a b` gave a merely repeatable flag both values and silently stole the positional that `b` should have filled. The grammar already drew this distinction; nothing tested it, so there is now a vector that does, and usage-lib agrees with it. The field invited the mistake by sharing a name with the spec's flag-level `var`, which means something else, so it is now `variadic` and says what it is not. Renaming it is free today because no release has published the crate yet. `double_dash_seen()` also reported true when no separator had been typed: automatic mode stops flag interpretation by setting the same flag the accessor reads. Those are now two pieces of state, since a caller asking the question wants to know what the user wrote. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jdx
added a commit
that referenced
this pull request
Aug 11, 2026
Two findings from the review of #798, both real. ## A repeatable flag was greedy The spec has two similar-looking declarations that mean different things: - `flag "--include <pattern>" var=#true` — **repeatable**: one value per occurrence - `flag "--include <pattern>..."` — **variadic**: one occurrence takes several values The conformance harness set the parser's greedy flag from either, so `--include a b` gave a merely repeatable flag both values — and silently stole the positional that `b` should have filled. The grammar already drew the distinction; nothing tested it. There is now a vector that does, and usage-lib agrees with it, so this was ours alone. The field name invited the mistake — `Flag.var` sat next to the spec's flag-level `var`, which means the other thing — so it is now `Flag.variadic` with a doc comment saying what it is *not*. Free to rename today, since no release has published the crate. ## `double_dash_seen()` could lie `automatic` mode stops flag interpretation by setting the same field the accessor reads, so it reported a separator that the user never typed. Now two pieces of state: `flags_stopped` for the parser, `separator_seen` for the question callers actually ask. usage-lib draws the same line for `preserve`, where a `--` is kept as a value rather than consumed. Three unit tests cover the pair, including the counterpart case — a non-variadic flag must leave the next word alone. 87 vectors, 63 answered by usage-argv, 16 recorded usage-lib divergences. *AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable.* <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes argv binding semantics for specs that relied on the old greedy `var` mapping; API rename on unreleased `Flag` field and behavior change for `double_dash_seen()` in edge cases. > > **Overview** > Fixes **usage-argv** conflating spec **repeatable** flags (`var=#true`, one value per occurrence) with **variadic** flag arguments (`<pattern>...`, greedy until a flag-like token). The conformance bridge no longer sets parser greed from flag-level `var`; only a variadic argument enables value collection. **`Flag.var` is renamed to `variadic`** with docs clarifying it is not flag-level `var`. > > **`double_dash_seen()`** is corrected by splitting parser state: `flags_stopped` (flag interpretation off, including `automatic` args) vs `separator_seen` (a real `--` was consumed). Required-arg checks use `separator_seen` so `preserve` / `automatic` do not lie to callers. > > Adds corpus vector **`long-repeatable-flag-is-not-greedy`**, unit tests, and doc/corpus count updates (63 in-scope vectors). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 8ef1ff2. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Repeatable flags now consume exactly one value per occurrence, leaving subsequent values available to positional arguments. * Variadic flags correctly consume multiple values from a single occurrence. * Improved handling and reporting of explicit `--` separators, including positional arguments and subcommands. * **Documentation** * Clarified flag value consumption rules and updated command-line conformance statistics. * Added coverage for repeatable, variadic, and separator-handling scenarios. * **Refactor** * Renamed the public flag property from `var` to `variadic` for clearer behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
jdx
added a commit
that referenced
this pull request
Aug 11, 2026
`PLAN.md` — the plan for this work written down end to end, including the parts that do not exist yet. Three PRs in (#797, #798, #799), the plan lived in PR descriptions and in my head. That is fine for one PR and not for a dozen, especially for the config layer, where the shape is worth arguing about *before* it gets built. Checkboxes rather than prose, so the file doubles as status: **an unchecked box means the thing does not exist.** Ticking them as things land keeps it honest, and makes it obvious when a branch of the plan has stalled. ## What it covers - **Why** — mise's measured numbers, and the fact that mise already hand-maintains two argv scanners to avoid building its clap tree. That workaround existing is the argument for the project. - **How it is arranged** — the four rules that hold it together: code authors and the spec defines; usage-lib is the reference implementation; the hot path stays small; end users never need a second binary. - **Milestones** — what is done, the derive work next, the table stakes after it (help, self-contained completions, docs, diagnostics). - **The gate** — the perf targets, measured with `tak` against a shadow CLI generated from mise's own committed spec. Explicitly: if the targets miss by a wide margin, write that down and stop. Nothing touches mise before this. - **Known usage-lib divergences** — as a to-do list, since each is a small change to `lib/src/parse.rs` and the corpus already knows how to verify a fix. - **Config** — the v2 design, from reading all four CLIs. ## The config section is the part worth reviewing mise, hk, pitchfork, and fnox have each independently built the same settings model — a TOML registry, `build.rs` codegen, a typed `Settings` plus a meta map, project-over-global-over-defaults layering — and agree on ~80% of the vocabulary. The differences are mostly *drift* rather than intent: - Every one hand-writes the CLI-to-settings binding, and every one has a hole in it: hk declares `sources.cli` entries nothing reads, pitchfork's `--help` documents a CLI layer it does not have (copied by hand into its committed spec), and fnox resolves `age_key_file` through a hardcoded five-way chain because its settings and config files are separate systems. - Only hk can say where a value came from, and it needed a second parallel merge to do it. - Docs/schema generation is three separate reimplementations, and fnox has none. The proposal is to declare props in code, lower them into the spec's `config { prop ... }` block — which **exists today and no CLI emits or consumes** — and generate the CLI binding instead of hand-writing it. That block needs extending first (`deprecated`, `enum`, `optional`, `aliases`, `merge`, scope, per-source lists), which is spec-first per the canonicality rule. Three open questions are listed rather than decided, including whether config belongs in this repo at all. *AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable.* <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Documentation-only addition with no runtime, build, or API changes. > > **Overview** > Introduces **`PLAN.md`** as the canonical, in-repo plan for the compiled argv parser work and the later shared config layer—replacing plan text that lived only in PR descriptions. > > The doc uses **unchecked checkboxes as status** (unchecked = not built yet) and covers motivation (mise/clap cost), architecture (spec → usage-derive / usage-argv / usage-lib), milestones (done vs derive vs gate vs adoption), perf gate targets, corpus gaps, known **usage-lib** divergences as a fix list, and a **config** design sketch (unify mise/hk/pitchfork/fnox settings) with open questions—not implementation. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 96b60f9. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #797 — review that one first; this PR's diff against
mainwill include it until it merges.What this is
argv/— a new crate,usage-argv, implementing the binding half of the grammar from #797. No command tree, no allocation, one pass overargv.Not published (name is reserved on crates.io at 0.0.0), and deliberately outside the shared-version release cycle, the way
clap_usagealready is.Two design decisions worth a look
Events, not a map. Parsing yields
Event::{Command, Flag, Arg}rather than returning a structure. A map would have to allocate and then be read back out; an event can be assigned straight into a struct field by generated code. Same reasoning as serde deserializing into your type instead of into aValue.Values are
&[u8]. Borrowed fromargv, converted by the caller withas_str. Slicing anOsStrinto&strpieces needs either an allocation orunsafe, and this crate forbidsunsafe. The upside is that a non-UTF-8 command line still parses — flags match, subcommands route — and only the values actually looked at can fail to convert, which is where that failure belongs.Tables are borrowed slices (
&'a [&'a Flag<'a>]) so a derive can emit the whole tree asstaticdata. Tables andargvcarry separate lifetimes; a single lifetime compiled but forcedargvto be'static, which a doctest caught.Scope
Binding only.
required,choices,envfallback, defaults,var_min/var_max, andoverridesall happen after the last token is read and need to know a value's type, so they belong to the layer that owns the target struct. Keeping them out is what keeps this loop small.How it's verified
The corpus.
conformance/gained a second runner, so the same vectors now exercise both parsers. usage-argv answers 61 of 83; the remaining 22 are post-binding and report why they're exempt, with the count asserted so the exempt set can't quietly grow. A separate test asserts that all 15 vectors where usage-lib diverges from the grammar are ones this parser gets right — that was the point of writing the grammar down.Since nothing emits tables yet, the harness builds them from a
Specand leaks them. A test process building a few small tables is the one place where leaking is the simplest correct answer; generated code has no such problem.Zero allocation.
argv/tests/no_alloc.rsarms a counting global allocator around parses of ten realistic command lines and four failing ones, and asserts zero. It also asserts the counter observes a deliberate allocation, so the test can't pass vacuously.One wrinkle worth knowing if you write similar tests: the file holds exactly one
#[test], because the counter is global and a sibling test building aVecon another thread showed up as four phantom allocations here.One grammar change
Implementing this surfaced a rule the doc didn't state: a flag with a variadic argument (
--include <pattern>...) consumes following tokens until one is flag-like. I've added it todocs/spec/argv.md, including the note that it is greedy and will eat positionals — inherent to the feature, ended explicitly with--.Next
A derive that emits these tables, and then a benchmark against a clap-shaped equivalent at mise's scale, which is the gate for whether this is worth continuing.
AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable.
Note
Medium Risk
New public parser surface and grammar-aligned behavior that differs from usage-lib on recorded divergences; release script changes affect publishing, but no changes to existing usage-lib parse paths in this diff.
Overview
Introduces
usage-argv, a dependency-free crate that implements the argv binding half of the usage grammar in one pass with no heap allocation. Parsing is driven by staticCommand/Flag/Argtables (intended for future derive output) and streamsEventvalues with&[u8]payloads instead of building a map.Conformance gains a second runner (
conformance/src/argv.rs+conformance/tests/argv.rs) that builds leaked tables from corpus specs and asserts 62 in-scope binding vectors pass, including cases where usage-lib diverges; post-binding features (required,choices,env, defaults, etc.) are explicitly out of scope with a fixed count.argv/tests/no_alloc.rsenforces zero allocations during parse via a counting global allocator.docs/spec/argv.mddocuments greedy variadic flag value consumption and notes dual-parser corpus testing.tasks/release-plzpublishesusage-argvwith the shared version and includesargv/**in changelog path filters.Reviewed by Cursor Bugbot for commit 180aec7. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
--handling.Documentation
Tests