fix(parse): keep every -- after the first - #809
Conversation
Only the first is a separator. A later one is an ordinary value, because flag parsing has already stopped and there is nothing left for it to do — so a command line forwarded through a wrapper came out altered. The test that asserted the old behavior was documenting a bug rather than a decision. #229 reported exactly this ("I would expect double dashes after the first one to be passed through"), and the fix for it added `preserve` as an opt-in without changing the default underneath. `preserve` stays meaningful: it is about the *first* separator, which it keeps as a value rather than consuming. Every parser worth comparing against keeps the later ones — POSIX getopt, Python argparse, clap in both its positional modes, commander, and yargs all read `-- a -- b` as three values. The corpus now includes the command line from the report, verbatim. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Warning Review limit reached
Next review available in:16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe parser now treats only the first ChangesDouble-dash parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
Instruction countsNothing 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: 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.
|
Greptile SummaryThe PR corrects argument parsing so only the first
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix(argv): count allocations per thread,..." | Re-trigger Greptile |
The zero-allocation test failed under coverage on this branch while passing everywhere else, which is the signature of a race rather than a regression: the counter was a global flag, so the harness's own thread — waiting, printing, collecting results — had its allocations attributed to the parse. Instrumentation changed the timing enough to make it land inside an armed region. Arming is now per thread, so only the thread being measured can contribute and neither the harness nor a sibling test can. The flag is a `const`-initialized thread-local, since reading it inside a global allocator must not allocate, and it is read with `try_with` because the local is gone during thread teardown and an allocation then must not panic. This was latent on main rather than introduced here. 15 runs under `-C instrument-coverage` now pass where one in a few used to fail. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
`main` is red because of a tripwire I wrote. #806 and #809 each added an ordinary corpus vector; each was green alone, because each saw only its own increment of a hardcoded count, and together they made 65 where the constant said 64. Counting was the mistake, not the number. A count asserts something nobody can check by reading it, and it collides whenever two changes touch the data it counts. What the assertion was actually for — noticing when a vector changes sides — is better served by a snapshot of which vectors are exempt and why: it fails as a reviewable diff naming the vector that moved, adding an ordinary vector does not touch it at all, and the list documents the post-binding boundary that the number obscured. The file lost two magic numbers, not one. `error_expectations_are_reachable` asserted that at least six error classes were exercised, which the per-vector checks already cover. Also drops `no_vector_has_an_unloadable_spec`, which duplicated `specs_are_valid` in reference.rs, and `out_of_scope_vectors_say_why`, whose reasons are now visible in the snapshot. Corpus well-formedness is checked in one place; this file is about the parser. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
You were right to be suspicious. It is a bad test, and the archaeology says so.
Where it came from
#229 is a user reporting this as a bug, titled "arg removes all double dashes from the arguments, even after the first one":
The fix, #417, added
double_dash="preserve"as an opt-in and left the default underneath untouched — then addedtest_double_dashes_without_preserveasserting it. So the test documents the reported bug as if it were a decision. Nobody chose it.preserveis still meaningful, and the survey clarifies what it actually is: it keeps the first separator, which is clap'strailing_var_arg+allow_hyphen_valuesshape. That is a different question from what happens to later ones.Survey
I ran these rather than recall them —
-- a -- bagainst one variadic positional:getopt(3)(viagnu_getopt)a -- bargparse,nargs='*'a -- bclap,num_args(0..)a -- bclap,trailing_var_arga -- bcommander,[args...]a -- byargs, defaulta -- ba bUnanimous, including the one mise is built on. argparse also reproduces the exact report:
-- hello -- goodbye --→hello -- goodbye --.The change
Gate the separator branch on flags still being enabled, so a
--that arrives after flag parsing has stopped is a value like any other. The old test now asserts the corrected result, with a comment saying whatpreserveis actually for, and the corpus carries the reporter's command line verbatim.The corpus flagged the stale divergence label the moment the parser changed, which is the third time that workflow has paid for itself.
AI-assisted — Tool: Claude Code; model: anthropic/claude-fable-5; version: unavailable.
Note
Cursor Bugbot is generating a summary for commit 86348d6. Configure here.
Summary by CodeRabbit
--separators during command-line parsing.--tokens as positional values when collecting variadic arguments.