fix(argv): stop a repeatable flag from eating a positional - #799
Conversation
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>
7a27844 to
8ef1ff2
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe parser now distinguishes repeatable single-value flags from variadic flags. It separately tracks automatic flag stopping and explicit ChangesArgument parsing semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Parser
participant Flag
participant PositionalArgument
Parser->>Flag: inspect variadic setting
Flag-->>Parser: collect one or multiple values
Parser->>PositionalArgument: assign remaining word
Parser->>Parser: record explicit -- separator
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThe PR corrects argv binding by distinguishing repeatable flags from flags with variadic arguments and separately tracking stopped flag parsing from an explicitly consumed
Confidence Score: 5/5The PR appears safe to merge, with the parser-state split and repeatable-flag correction consistently implemented and covered. The changed parser paths use variadicity only for greedy value collection, preserve repeated-value accumulation in the conformance layer, and distinguish actual separators from automatic flag stopping without leaving inconsistent state reads. Important Files Changed
Reviews (1): Last reviewed commit: "fix(argv): stop a repeatable flag from e..." | Re-trigger Greptile |
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.
|
`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>
Five review findings, all real. The first is the distinction #799 fixed in the conformance harness, recurring here: `var` was setting the parser's greedy collection, so `--include a b` gave the flag both values and stole the positional that `b` should have filled. The two are now spelled the way a spec spells them — `var` for a flag that may be repeated, `variadic` for one occurrence that keeps taking values — and a `Vec` flag is repeatable without having to say so. The original test missed this by always writing `--include` twice. A negation is another long form, so it now collides like one; two flags could previously answer to the same token with only the first reachable. Generic parameters were silently dropped from the generated impl, and are now refused with a reason. A non-ASCII `short` was truncated to one byte and could never be matched, so it is refused too. And a `u8` count given 256 occurrences panicked in debug and wrapped to zero in release; it saturates, with the accumulator typed as the field rather than inferred. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Five review findings, all real. The first is the distinction #799 fixed in the conformance harness, recurring here: `var` was setting the parser's greedy collection, so `--include a b` gave the flag both values and stole the positional that `b` should have filled. The two are now spelled the way a spec spells them — `var` for a flag that may be repeated, `variadic` for one occurrence that keeps taking values — and a `Vec` flag is repeatable without having to say so. The original test missed this by always writing `--include` twice. A negation is another long form, so it now collides like one; two flags could previously answer to the same token with only the first reachable. Generic parameters were silently dropped from the generated impl, and are now refused with a reason. A non-ASCII `short` was truncated to one byte and could never be matched, so it is refused too. And a `u8` count given 256 occurrences panicked in debug and wrapped to zero in release; it saturates, with the accumulator typed as the field rather than inferred. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Five review findings, all real. The first is the distinction #799 fixed in the conformance harness, recurring here: `var` was setting the parser's greedy collection, so `--include a b` gave the flag both values and stole the positional that `b` should have filled. The two are now spelled the way a spec spells them — `var` for a flag that may be repeated, `variadic` for one occurrence that keeps taking values — and a `Vec` flag is repeatable without having to say so. The original test missed this by always writing `--include` twice. A negation is another long form, so it now collides like one; two flags could previously answer to the same token with only the first reachable. Generic parameters were silently dropped from the generated impl, and are now refused with a reason. A non-ASCII `short` was truncated to one byte and could never be matched, so it is refused too. And a `u8` count given 256 occurrences panicked in debug and wrapped to zero in release; it saturates, with the accumulator typed as the field rather than inferred. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 occurrenceflag "--include <pattern>..."— variadic: one occurrence takes several valuesThe conformance harness set the parser's greedy flag from either, so
--include a bgave a merely repeatable flag both values — and silently stole the positional thatbshould 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.varsat next to the spec's flag-levelvar, which means the other thing — so it is nowFlag.variadicwith a doc comment saying what it is not. Free to rename today, since no release has published the crate.double_dash_seen()could lieautomaticmode 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_stoppedfor the parser,separator_seenfor the question callers actually ask. usage-lib draws the same line forpreserve, 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.
Note
Medium Risk
Changes argv binding semantics for specs that relied on the old greedy
varmapping; API rename on unreleasedFlagfield and behavior change fordouble_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-levelvar; only a variadic argument enables value collection.Flag.varis renamed tovariadicwith docs clarifying it is not flag-levelvar.double_dash_seen()is corrected by splitting parser state:flags_stopped(flag interpretation off, includingautomaticargs) vsseparator_seen(a real--was consumed). Required-arg checks useseparator_seensopreserve/automaticdo not lie to callers.Adds corpus vector
long-repeatable-flag-is-not-greedy, unit tests, and doc/corpus count updates (63 in-scope vectors).Reviewed by Cursor Bugbot for commit 8ef1ff2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
--separators, including positional arguments and subcommands.Documentation
Refactor
vartovariadicfor clearer behavior.