feat(spec): split a value the way clap splits one by jdx · Pull Request #961 · jdx/usage

jdx · 2026-08-17T04:36:45Z

Stacked on #941. Third item on the "changes what a CLI does" list in PLAN.md.

flag "--tags <tag>" var=#true delimiter=","

#[usage(long, delimiter = ',')]
tags: Vec<String>,

--tags a,b,c as three values. The spec could already record a clap default split by a delimiter — lib/src/spec/arg.rs has done that since the --fs-events fix — and had no way to say the same thing about a value someone types. An adopter using value_delimiter was losing behaviour rather than spelling.

Split before anything judges the result

This is the part that decides whether the feature is worth having. choices is asked about each value rather than about the word that carried them, and var_min/var_max count the values the user meant rather than the words they typed:

--env dev,prod   # two values, both on the choices list — accepted
--env dev,nope   # reports `nope`, not `dev,nope`
--env a,b,c      # three values against var_max=2 — refused

Judging first would reject dev,prod against a list that both halves are on, which is what the first draft of this did.

A delimiter needs somewhere to put what it splits

So it goes with var. On a single-value flag everything after the first separator would be dropped silently — refused where it is written instead, in all three places a spec can declare one (root arg, cmd block arg, and flag), and a compile error in the derive, where the field has to be a Vec. A non-ASCII delimiter is refused too, since values are split by byte.

Where the split runs

The derive splits on the cold path, first in check and after the environment has filled what argv left out — TAGS=a,b is one word too. The binder is untouched: it collects words and knows nothing about what a value is, which is what keeps the hot path free of all of this. A split costs one allocation per value, only for a field that asked for one.

The bridge carries it

Arg::get_value_delimiter is public — the same getter the default splitting already used. Only where several values can land, because clap refuses value_delimiter with num_args(1) itself, and a spec recording one on a single-value argument would be a spec this crate then declines to parse.

cargo test --all --all-features, mise run lint, mise run render and mise run gen-shadow are clean.

🤖 Generated with Claude Code

Note

Medium Risk
Touches core argv binding and both derive and spec parsers, so miscounting or split order could change CLI behavior; mitigated by extensive tests and alignment with clap semantics.

Overview
Adds delimiter (spec KDL, #[usage(delimiter = ',')], metadata) so one argv word can represent several values, matching clap’s value_delimiter.

Parsing behavior: Values are split on the delimiter before choices and var_min/var_max run, so --env dev,prod is judged per value and limits count intended values, not raw words. The derive path splits in generated check after env fallback; usage-lib does the same for spec-driven parse; the argv binder stays word-oriented but uses values_in and can return VarTooMany when a delimited word exceeds var_max in one step.

Validation & bridge: Delimiter requires a Vec field (compile-time in derive) and var in the spec; only ASCII separators are stored in binding tables. Clap import sets var when a delimiter is present and round-trips ASCII delimiters in KDL/docs/shadow codegen.

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

coderabbitai · 2026-08-17T04:36:55Z

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: 49f61e3f-025f-4f9d-96f2-735b001886fb

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-17T04:40:35Z

Greptile Summary

Adds delimiter-aware value splitting across the KDL specification, clap bridge, interpreted parser, compiled argv parser, and derive-generated parser.

Validates delimiters and requires storage capable of holding multiple values.
Splits values before choices and cardinality checks.
Carries delimiter metadata through generated KDL and parser tables.
Adds conformance and parser coverage for split values and bounded variadics.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains within the eligible follow-up scope.

No blocking failure remains.

Important Files Changed

Filename	Overview
argv/src/lib.rs	Adds delimiter metadata and counts split values while binding bounded variadic flags and positionals.
derive/src/codegen.rs	Emits delimiter metadata and splits collected values before generated post-binding validation.
derive/src/model.rs	Parses and validates the delimiter attribute, restricting it to ASCII delimiters on collection fields.
lib/src/parse.rs	Splits positional and flag values before choice validation and variadic-bound accounting.
lib/src/spec/arg.rs	Adds delimiter parsing, serialization, validation, and clap metadata conversion for value arguments.
lib/src/spec/flag.rs	Validates delimiter use on value-bearing variadic flags and carries clap delimiter metadata into specifications.
argv/src/spec.rs	Adds delimiter fields to cold metadata and includes them in generated KDL.
conformance/tests/post_binding.rs	Exercises delimiter splitting, choices, positional values, emitted specifications, and variadic bounds.

_{Reviews (18): Last reviewed commit: "fix(conformance): carry the delimiter th..." | Re-trigger Greptile}

github-actions · 2026-08-17T04:48:39Z

Instruction counts

Nothing was compared, and so nothing was gated. No series appears on both sides: either the base has no measurements recorded, or the two were measured on different runner classes, which are deliberately not comparable — counts shift between machine types by more than a real regression does.

New, nothing to compare against: markdown on bamboo-v2-ubuntu24.04-x64-30vcpu-24gb-rust1.97.1, startup on bamboo-v2-ubuntu24.04-x64-30vcpu-24gb-rust1.97.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.}

Shadow comparison

Parsing mise use -g node@20 against a shadow of mise's committed spec.
Reported, not gated: the shadow grows as the derive learns to express more, so
what to watch is the ratio rather than either column.

framework	instructions, cold parse	vs usage
usage	4203	—
argh	6292	1.5x
clap	5895248	1402x
bpaf	21917778	5214x

                                              min       p01       p10    median
usage-rs: argv -> struct                      194       201       204       211  ns
argh: argv -> struct                          280       284       287       292  ns
clap: build tree + parse -> struct         485256    486874    491357    505659  ns
bpaf: build parser + parse -> struct      1567730   1567730   1585358   1603538  ns

usage: argv -> struct                             208 ns      0.21 µs
clap: build tree + parse -> struct             508707 ns    508.71 µs
clap: parse -> struct, tree reused              22795 ns     22.80 µs
clap: build tree only                          316326 ns    316.33 µs

_{abc6187ab19f vs c5d9ad605c95 · measured on the runner, not pushed to the history.}

jdx · 2026-08-17T04:51:25Z

Both findings were right, and they were the same mistake in two places: treating a word that carries several values as one value.

Positional choices ran before the split (@cursor Bugbot, High) — the flag path split first and the positional path did not, so src:docs was rejected against a list both halves are on, and a bad half was reported as the whole word. It splits first now, and the collection below reuses what the split produced instead of doing it again.

The bridge dropped a delimiter clap would have used (High) — I had gated it on Append/Count or num_args > 1, reasoning that clap refuses a delimiter with num_args(1). It doesn't: clap_builder's parser.rs reaches for arg.get_value_delimiter() before it looks at anything else, so ArgAction::Set with value_delimiter(',') is one word becoming several — and that's the common spelling. The generated spec was leaving a CLI whose defaults split and whose typed values didn't, which is the exact drift this PR set out to close.

A delimiter is now taken as the statement that several values can land, so it brings var with it rather than waiting for one. That also keeps the emitted spec parseable under this PR's own rule that a delimiter needs somewhere to put what it splits — a_single_valued_clap_arg_keeps_its_delimiter round-trips it to make sure.

This comment was generated by Claude Code.

jdx · 2026-08-17T18:29:45Z

Pushed a fix for @cursor Bugbot's var_max finding. Both halves were real, and the second one changed where the fix belongs.

var_max is enforced by binding for these shapes, not after it: a variadic stops on its bound and the next field takes the rest, so before delimiters the bound could be reached but never passed. A delimiter breaks the assumption underneath that — one word stops being one value, so --include a,b,c is three on the single word the collection was entitled to take, and stopping is no longer the same as staying within the bound.

So instead of re-adding a post-binding count, the parser tables gain the delimiter, beside the var_max they already carry and for the reason var_max is documented as living there: it decides where a word lands. Binding counts the values a word holds rather than the word, and since a word cannot be split between two owners, passing the bound is an error where reaching it is merely a stopping place. Positionals and variadic flags are fixed together.

usage-lib was already counting values rather than words, so its collection loop stopped in the right place but never reported the overshoot. It now does, at the end of an occurrence's run.

What I got wrong first

My initial attempt counted the accumulated list on both sides. The corpus vector a-bound-counts-one-occurrence caught it: the bound is on what one occurrence takes, not on the list the occurrences build up, so --include a,b --include c,d is two twice and not four. Reading it as a total would make the same declaration mean fewer values per occurrence the more often the flag is given. Both fixes are per-occurrence now, and that vector still passes.

Tests

a_bound_counts_the_values_a_word_carried (conformance) — the flag and the positional
a_split_bound_still_counts_one_occurrence_at_a_time (conformance), a_split_bound_counts_one_occurrence_at_a_time (usage-lib) — the per-occurrence rule on split words
corpus a-bound-counts-values-not-words — pins it for any implementation, including the Go one

Stubbing values_in back to counting words fails the conformance test and the corpus vector; stubbing the usage-lib report fails its test.

Cost and API

The shadow benchmark is bit-identical at 63,822 instructions and startup is unchanged — mise declares no delimiters, so values_in short-circuits and the new field is never read. One thing for your call: Flag and Arg each gain a public field, so a caller constructing them literally rather than through ..Flag::BOOL / ..Arg::REQUIRED would need updating. The conformance harness was the only such caller in-tree.

cargo test --all --all-features, mise run lint, mise run render and mise run gen-shadow are clean, with no regenerated output.

This comment was generated by Claude Code.

jdx · 2026-08-17T18:57:52Z

CI was red on test:go, and it was my corpus vector that did it.

The Go parser has no delimiter support at all — not a partial implementation, no mention of it anywhere in go/. So a-bound-counts-values-not-words failed there for want of the feature rather than for want of this rule, and notYet is not the escape hatch: the harness asserts skipped == 0 precisely so the corpus stays answered in full, and relaxing that to land a Rust fix would trade away a real invariant of the port.

So the vector is gone, and corpus/03-positionals.json is byte-identical to before this PR touched it. The rule stays held across the derive and usage-lib by a_bound_counts_the_values_a_word_carried, which is the agreement this change is actually about; the corpus vector belongs with whichever change teaches Go to split. That reasoning is in the commit message rather than only here, so the next person to add delimiter vectors finds it.

My mistake in process, not just in content: I ran cargo test --all --all-features and reported it clean, which it was — but the Go suite runs under mise run test:go, so "clean" was narrower than it sounded. I have run mise run ci end to end on the amended commit and it exits 0, Go included.

This comment was generated by Claude Code.

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 27f0cdd. Configure here.}

jdx · 2026-08-17T19:37:14Z

Rebased onto the updated #941, and through it onto main.

conformance/src/tables.rs arrived on main and took over the table building this branch had been editing in argv.rs, constructing each struct field by field — so the delimiter added here left four initializers short. Both models want it, and differently: the binding tables take the byte that binding counts values by, and the metadata takes the char the spec declared.

Worth noting the short-flag assertion a few lines above in that same file, which arrived independently: it refuses a non-ASCII short flag because "the line arrives as UTF-8, where such a character is two bytes or more, and whatever single byte a cast produced would match nothing anybody could type." That is the same reasoning as the delimiter fix in the previous commit, reached separately — good corroboration that ASCII is the right boundary here rather than a convenience.

mise run ci passes end to end, Go included.

This comment was generated by Claude Code.

`--tags a,b,c` as three values. The spec could record a clap *default* split by a delimiter — `lib/src/spec/arg.rs` has done that since the `--fs-events` fix — and had no way to say the same thing about a value someone types, so an adopter using `value_delimiter` lost behaviour rather than spelling. flag "--tags <tag>" var=#true delimiter="," Split during the parse, before anything judges what came out, which is the part that matters: `choices` is asked about each value rather than about the word that carried them, and `var_min`/`var_max` count the values the user meant rather than the words they typed. Judging first would reject `--env dev,prod` against a list that both halves are on. A delimiter needs somewhere to put what it splits, so it goes with `var`. On a single-value flag everything after the first separator would be dropped silently — refused where it is written instead, in all three places a spec can declare one, and a compile error in the derive, where the field has to be a `Vec`. The derive splits on the cold path, first in `check` and after the environment has filled what argv left out, since `TAGS=a,b` is one word too. The binder is untouched: it collects words and knows nothing about what a value *is*, which is what keeps the hot path out of this. The bridge carries it — `Arg::get_value_delimiter` is public, and it is the same getter the default splitting already used. Only where several values can land, because clap refuses `value_delimiter` with `num_args(1)` itself and a spec recording one there would be a spec this crate then declines to parse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…miter Two from review, both of them the same mistake in two places: treating a word that carries several values as one value. **A positional judged the whole word.** The flag path split first and the positional path did not, so `src:docs` was rejected against a list that both halves are on, and a bad half was reported as the whole word. Split first now, as the flag path does, and the collection below reuses what the split produced rather than doing it again. **The bridge dropped a delimiter clap would have used.** It was carried only for `Append`/`Count` or a `num_args` above one, on the reasoning that clap refuses a delimiter with `num_args(1)`. clap does no such thing: its parser reaches for `arg.get_value_delimiter()` before it looks at anything else, so `ArgAction::Set` with `value_delimiter(',')` is one word becoming several — and that is the common spelling. The generated spec left a CLI whose defaults split and whose typed values did not. A delimiter *is* the statement that several values can land, so it brings `var` with it rather than waiting for one, which also keeps the emitted spec parseable by the rule that a delimiter needs somewhere to put what it splits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`var_max` is enforced by binding: a variadic stops on its bound and the next field takes the rest, so before delimiters a collection could reach the bound but never pass it. A delimiter breaks that assumption — one word is no longer one value, and `--include a,b,c` is already three on the single word the collection was entitled to take. Binding counted words, so the bound was silently exceeded, for positionals and for variadic flags alike. The parser tables gain the delimiter, beside the `var_max` they already carry and for the same stated reason: it decides *where* a word lands. Binding counts the values a word holds rather than the word, and a word cannot be split between two owners, so passing the bound is an error where reaching it is just a stopping place. usage-lib already counted values but only ever stopped, so it gets the same report at the end of an occurrence's run. Both keep the rule the corpus documents: the bound is on what one occurrence takes, not on the list the occurrences build up, so `--include a,b --include c,d` is two twice and not four. No corpus vector for this yet, deliberately. The corpus is what every implementation answers, and the Go parser has no delimiter support at all — a vector here would fail it for want of the feature rather than for want of this rule. It belongs with whichever change teaches Go to split. The rule is held across the derive and usage-lib by the conformance test in the meantime, which is the agreement this change is about. The derive path is unchanged in cost — the shadow benchmark is bit-identical at 63,822 instructions, mise declaring no delimiters for `values_in` to count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The KDL side accepted any single Unicode scalar while everything below it splits by byte, so the two disagreed in both directions. A separator above U+00FF was dropped on the way into the binding tables and the CLI quietly stopped splitting. Worse, one between U+0080 and U+00FF *fits* in a byte as a scalar while taking two in UTF-8, so matching that byte found the continuation bytes inside unrelated characters: with `delimiter="§"`, binding split `aЧbЧc` into three values and refused it against `var_max=2`, while usage-lib accepted it. Rejecting a command line nobody wrote a separator into is the worse half of the bug, and it arrived with the tables learning the delimiter at all. So the spec enforces what the derive already enforced where it reads the same property, in the same words: one byte, use an ASCII separator. Both conversions into the tables now filter on `is_ascii` rather than on fitting in a `u8`, which is the distinction that was wrong. The clap bridge records only an ASCII delimiter too. clap splits by character and may well have a wider one; the spec keeps `var`, so the values still arrive, and drops only its account of how they were separated — a spec that recorded it could not be written back out, since `to_kdl` would emit what parsing now refuses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`conformance/src/tables.rs` arrived on main while this branch was open, taking over the table building this branch had been editing in `argv.rs` and constructing each struct field by field — so the `delimiter` added here left four initializers short. Both models want it, and they want it differently: the binding tables take the byte binding counts values by, filtered on `is_ascii` rather than on fitting in a `u8`, and the metadata takes the `char` the spec declared. The short-flag assertion a few lines above already says why that distinction matters — a character that is several bytes in UTF-8 has no single byte that matches anything anyone could type. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>