fix(parse): enforce double_dash="required" for positional args

[JamBalaya56562]

Fixes the root cause behind jdx/mise#5759.

Problem

The parser never looked at SpecDoubleDashChoices::Required. Seeing a -- only set enable_flags = false; nothing consulted double_dash when assigning positionals. So for

arg "[file]" double_dash="required"

both of these succeeded:

mise run echo test        # should be a parse error
mise run echo -- test     # correct

The reverse half was missing too: an arg requiring -- that sits behind a greedy variadic was never reachable, even with a separator. In examples/mise.usage.kdl that is [-- TASK_ARGS_LAST]..., [-- ARGS_LAST]... and exec's [-- COMMAND]... — all declared, none ever filled.

Fix

double_dash="required" is generated from clap's Arg::last(true) (lib/src/spec/arg.rs, is_last_set() → Required), so both halves of that semantic are now implemented:

• Rejection — a word offered to such an arg before an explicit -- is reported as ArgRequiresDoubleDash and not assigned. A variadic arg reports this once rather than once per word, and an arg that is both required and double_dash="required" no longer also collects MissingArg. The check runs ahead of validate_choices so a discarded word is not additionally reported as an invalid choice.
• Routing — an explicit -- moves the positional cursor onto the arg that requires it, past earlier args including a greedy variadic.

Routing can leave an earlier arg empty, so "next arg" and "how many args are filled" no longer coincide. The cursor became an explicit index, and the two loops that used skip(out.args.len()) to find unfilled args — the MissingArg check and the default/env application in Parser::parse — now ask out.args by key instead of counting. Without that, a skipped arg would silently lose its default and its missing-arg diagnostic.

Interactions, both covered by tests:

• A -- that double_dash="preserve" keeps as a value stays a value. It does not count as a separator and does not unlock a required arg — one token cannot be both.
• restart_token resets the separator, so each invocation after it needs its own --.

Specs with no double_dash="required" arg are bit-for-bit unaffected: the cursor jump looks for such an arg and finds none.

Completions

ParseOutput gains next_arg (where the cursor stopped) and double_dash_seen. complete-word's local next_arg_for_completion re-derived the next arg from args and knew nothing about --, so with routing in place it would have disagreed with the parser; it now reads the parser's own cursor. While an arg is still locked behind a separator, the completion offers -- itself rather than values the parser would reject.

Versioning — lint:semver is red on purpose

This is a breaking change and cargo semver-checks check-release -p usage-lib says so. Two source-level breaks, both intentional:

• UsageErr gains an ArgRequiresDoubleDash variant (enum_variant_added)
• ParseOutput gains next_arg and double_dash_seen (constructible_struct_adds_field); the only construction site is inside parse.rs

I briefly cleared the check by bumping the workspace to 5.0.0 in this PR, then backed that out — it would have caused an unplanned release. tasks/release-plz opens with

if ! echo "$released_versions" | grep -q "^v$cur_version$"; then
  cargo publish -p usage-lib
  ...
  git tag "v$cur_version"

which is the publish half of the release-PR flow: release bumps the version, it merges to main, main is briefly a version with no tag, and this branch publishes it. A hand-written bump in a feature PR is indistinguishable from that, so merging one would publish to crates.io on the merge push — with no CHANGELOG entry, no release PR, and outside the 7-day cadence guard in auto-merge-release.yml.

So the commit is marked fix(parse)!: with a BREAKING CHANGE: footer instead. With breaking_always_bump_major = true in cliff.toml, git-cliff computes the major bump at release time and the version lands through the normal release PR. lint:semver stays red until then, which seemed like the honest state — it is reporting a real breaking change rather than a mistake.

Happy to add #[non_exhaustive] to ParseOutput in the same commit if you would rather close that off now, or to reshape the marker if you want the release to go differently.

Behavior changes

Measured against examples/mise.usage.kdl:

Spec site	Preceding arg	Change
tasks add <TASK> + [-- RUN]...	<TASK> (non-var)	mise tasks add t echo hi is now an error — clap already rejects it, so this closes a divergence
root [-- TASK_ARGS_LAST]...	[TASK_ARGS]... (greedy)	post--- values move from TASK_ARGS to TASK_ARGS_LAST
tasks run [-- ARGS_LAST]...	[ARGS]... (greedy)	post--- values move from ARGS to ARGS_LAST
exec [-- COMMAND]...	[TOOL@VERSION]... (greedy)	post--- values move from TOOL@VERSION to COMMAND

The last three are the args finally doing what they were declared to do, but they change which usage_* variable a consumer reads. @jdx — worth confirming this is what mise expects before release, since mise is the main consumer of these specs. Spec authors who want the old permissiveness can drop back to double_dash="optional" (the default).

double_dash="automatic" remains unimplemented in the parser; this PR does not change that, and docs/spec/reference/arg.md now says so explicitly.

Tests

33 new tests, no existing test modified:

• lib/tests/parse.rs — 17 end-to-end cases over the three spellings (double_dash="required", arg "[-- x]", arg "<-- x>"), covering rejection, routing past a greedy variadic, single-error guarantees, choices non-interference, and optional staying untouched.
• lib/src/parse.rs — 10 unit tests asserting ParseOutput internals: error counts and kinds, the gap left by routing (defaults and MissingArg still correct), restart_token in both directions, preserve non-interference, and parse_partial staying Ok where parse errors.
• cli/tests/complete_word.rs — 6 tests on two new choices-only fixtures, examples/double-dash.usage.kdl and examples/double-dash-default-subcommand.usage.kdl. Choices rather than a complete runner, so they resolve in-process and run on every platform.

cargo test --all --all-features is green on Linux across the workspace. Three failures on my machine are a local gap rather than this change: test_usage_double_slash_execution{,_old} and test_typescript_sdk_typechecks need node, which is not installed there.

---

This pull request was generated by Claude Code.

Summary by CodeRabbit

• New Features

  • Added support for arguments available only after an explicit -- separator, including shorthand syntax.
  • Values after -- are routed correctly with variadic arguments, restart tokens, and default subcommands.
  • Added clear errors when a required separator is missing.

• Bug Fixes

  • Improved completion to suggest --, suppress flags afterward, and support dash-prefixed positional values.

• Documentation

  • Expanded guidance and examples covering required separators and parsing behavior.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro Plus

Run ID: d0bd73c6-b53d-462e-8313-68f04994a521

📥 Commits

Reviewing files that changed from the base of the PR and between ce82199 and 6f2edb1.

📒 Files selected for processing (9)

• cli/src/cli/complete_word.rs
• cli/tests/complete_word.rs
• docs/spec/reference/arg.md
• examples/double-dash-default-subcommand.usage.kdl
• examples/double-dash.usage.kdl
• lib/src/error.rs
• lib/src/lib.rs
• lib/src/parse.rs
• lib/tests/parse.rs

---

📝 Walkthrough

Walkthrough

The parser now routes required separator arguments after --, reports separator violations, and exposes completion state. CLI completion suppresses flags after --. Tests, examples, and documentation cover routing, variadic arguments, restart tokens, default subcommands, and file completion.

Changes

Required separator argument support

Layer / File(s)	Summary
Parser state and public contracts lib/src/parse.rs, lib/src/error.rs, lib/src/lib.rs	ParseOutput exposes the next positional argument and separator state. Parsing emits ArgRequiresDoubleDash and the crate re-exports SpecDoubleDashChoices.
Separator routing and validation lib/src/parse.rs	The parser tracks positional gaps, routes values after --, handles variadic and preserved separators, resets state after restart tokens, and suppresses duplicate errors.
Parser regression coverage lib/tests/parse.rs, lib/src/parse.rs	Tests cover required, optional, and variadic arguments, positional gaps, restart behavior, preserved separators, error handling, and unchanged optional behavior.
Completion and usage integration cli/src/cli/complete_word.rs, cli/tests/complete_word.rs, examples/*.usage.kdl, docs/spec/reference/arg.md	Completion offers required separators, suppresses flags after --, and completes positional values including dash-prefixed paths. Fixtures and documentation describe the parsing rules.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• jdx/usage#738: Both changes modify parser and completion behavior in parse.rs and complete_word.rs.
• jdx/usage#753: Both changes modify positional argument selection and variadic completion logic in complete_word.rs.

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant Parser
  participant ParseOutput
  participant PositionalCompletion
  CLI->>Parser: parse partial command line
  Parser->>ParseOutput: record next_arg and double_dash_seen
  ParseOutput-->>CLI: return completion state
  CLI->>PositionalCompletion: complete current positional token
  PositionalCompletion-->>CLI: offer `--` or positional values

Loading

Poem

A rabbit typed -- in the snow,
Then flags stopped where values go.
Dash-marked paths joined the line,
Restarted routes reset just fine.
Parser and completer shared the trail. 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main parser change: enforcing required double-dash separators for positional arguments.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Greptile Summary

This PR enforces double_dash="required" during parsing and aligns runtime completions with the parser’s separator-aware positional cursor.

• Rejects positional values supplied before the required --.
• Routes post-separator values past preceding positional arguments, including greedy variadics.
• Exposes parser cursor and separator state to dynamic completion.
• Adds parser, completion, fixture, and documentation coverage for separator behavior.

Confidence Score: 5/5

The PR appears safe to merge, with no eligible blocking failure remaining.

No blocking failure remains.

Important Files Changed

Filename	Overview
lib/src/parse.rs	Introduces explicit positional cursor and separator state, enforces required separators, and updates default and missing-argument handling for cursor gaps.
cli/src/cli/complete_word.rs	Uses parser-owned cursor and separator state to select positional candidates and suppress flag completion after a consumed separator.
lib/src/error.rs	Adds the dedicated error variant used when a positional value is supplied before its required separator.
lib/src/lib.rs	Re-exports the double-dash choice enum for the CLI completion implementation.
lib/tests/parse.rs	Adds end-to-end coverage for rejection, separator routing, shorthand syntax, choices, and unchanged optional behavior.
cli/tests/complete_word.rs	Adds runtime completion coverage for separator suggestions, restart tokens, default subcommands, variadics, and dash-prefixed values.
docs/spec/reference/arg.md	Documents enforced required-separator semantics, shorthand forms, routing behavior, and interactions with preserve and restart modes.

_{Reviews (3): Last reviewed commit: "fix(parse)!: enforce double_dash="requir..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

lib/src/parse.rs (1)

749-757: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider documenting the no-rewind case for an earlier required arg.

The search finds the first unfilled double_dash="required" argument anywhere in out.cmd.args, but the jump applies only when target > next_arg_idx. If such an argument is declared before the current cursor and the cursor already moved past it, the -- does not route to it. The argument is then reported as MissingArg.

This shape is unusual, because double_dash="required" mirrors clap Arg::last(true), which is the final positional. The behavior is safe, but the reason for the > guard is not stated in the comment. Add a sentence that records this decision.

🤖 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 `@lib/src/parse.rs` around lines 749 - 757, Add a concise comment beside the
`target > next_arg_idx` guard explaining that required double-dash arguments
declared before the current cursor are intentionally not revisited, preserving
forward-only routing and allowing them to remain reported as `MissingArg`.

🤖 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 `@cli/src/cli/complete_word.rs`:
- Around line 146-158: Centralize positional completion in a helper that applies
the `parsed.next_arg` and `parsed.double_dash_seen` rules, then use it from the
flag branch, `after_restart_token` path, and default-subcommand fallback instead
of calling `complete_arg` directly. When `double_dash_seen` is true, do not
classify dash-prefixed tokens as flags; when the next argument requires a
separator and it has not been seen, offer only `--` for an empty token and
suppress value/file completion. Add regressions covering dash-prefixed values
after `--`, `restart_token`, and `default_subcommand`.

---

Nitpick comments:
In `@lib/src/parse.rs`:
- Around line 749-757: Add a concise comment beside the `target > next_arg_idx`
guard explaining that required double-dash arguments declared before the current
cursor are intentionally not revisited, preserving forward-only routing and
allowing them to remain reported as `MissingArg`.

🪄 Autofix (Beta)

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: e7144fcd-3818-4ed2-9de2-0952e680467a

📥 Commits

Reviewing files that changed from the base of the PR and between 4b2fde7 and bf9bb63.

⛔ Files ignored due to path filters (1)

• mise.lock is excluded by !**/*.lock

📒 Files selected for processing (8)

• cli/src/cli/complete_word.rs
• cli/tests/complete_word.rs
• docs/spec/reference/arg.md
• examples/double-dash.usage.kdl
• lib/src/error.rs
• lib/src/lib.rs
• lib/src/parse.rs
• lib/tests/parse.rs

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@cli/src/cli/complete_word.rs`:
- Around line 178-186: Preserve and propagate the constrained result returned by
complete_positional in this branch instead of discarding it. Update the
surrounding completion flow so a constrained default-subcommand argument that
matches no choices after -- remains constrained, preventing the fallback at the
later choices handling from offering file paths.
- Around line 122-124: Update the positional-token completion logic near
complete_path so dash-prefixed tokens after parsed.double_dash_seen still use
file completion when no explicit choices are configured. Keep flag completion
disabled after -- while removing the unconditional ctoken.starts_with('-')
exclusion from the positional file fallback.

🪄 Autofix (Beta)

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: 6829058e-75bf-4b83-b091-c19a9cbcf3ab

📥 Commits

Reviewing files that changed from the base of the PR and between bf9bb63 and f4f00c7.

📒 Files selected for processing (9)

• cli/src/cli/complete_word.rs
• cli/tests/complete_word.rs
• docs/spec/reference/arg.md
• examples/double-dash-default-subcommand.usage.kdl
• examples/double-dash.usage.kdl
• lib/src/error.rs
• lib/src/lib.rs
• lib/src/parse.rs
• lib/tests/parse.rs

🚧 Files skipped from review as they are similar to previous changes (5)

• lib/src/lib.rs
• lib/tests/parse.rs
• lib/src/error.rs
• docs/spec/reference/arg.md
• lib/src/parse.rs

[github-actions]

This PR currently has failing checks. If this continues for 7 days, it will be closed automatically.

This is warning day 1 of 7.

Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it.

This comment was generated by an automated workflow.

[JamBalaya56562]

CI is in. Everything passes except lint:semver.

223 checks: 221 pass, 2 fail, 0 warn, 30 skip

--- failure constructible_struct_adds_field ---
  field ParseOutput.next_arg          lib/src/parse.rs:261
  field ParseOutput.double_dash_seen  lib/src/parse.rs:267

--- failure enum_variant_added ---
  variant UsageErr:ArgRequiresDoubleDash  lib/src/error.rs:30

Summary  semver requires new major version: 2 major and 0 minor checks failed

Those are exactly the two breaks declared in the commit's BREAKING CHANGE: footer, so the gate is reporting the change rather than a mistake. The rest of the job is green — build, test, render and its no-diff assertion, lint:actionlint, lint:fmt, lint:prettier, and lint:clippy (now with --all-targets from #770). coverage passes too.

Why each break is needed

UsageErr::ArgRequiresDoubleDash — the diagnostic for "this arg only accepts values after --". UsageErr has no free-form message variant, and the nearest existing ones would either misreport an optional arg as missing (MissingArg) or render as Invalid usage config (InvalidInput).

ParseOutput::next_arg / double_dash_seen — once -- routes values past a greedy variadic onto the arg that required it, complete-word's local next_arg_for_completion (first unfilled arg, knows nothing about --) stops agreeing with the parser. Exposing the parser's own cursor is what keeps the two from drifting.

Why I am not bumping the version here

I did try that, then backed it out. tasks/release-plz opens with

if ! echo "$released_versions" | grep -q "^v$cur_version$"; then
  cargo publish -p usage-lib
  ...
  git tag "v$cur_version"

which is the publish half of the release-PR flow — release bumps the version, it merges to main, main is briefly a version with no tag, and this runs. A hand-written bump in a feature PR is indistinguishable from that, so merging one would publish to crates.io on the merge push: no CHANGELOG entry, no release PR, and outside the 7-day guard in auto-merge-release.yml.

Marking the commit fix(parse)!: instead lets breaking_always_bump_major in cliff.toml compute the major at release time, through the normal flow. The cost is that lint:semver stays red until then.

Two things for your call

1. The behavior table in the description. Three specs in examples/mise.usage.kdl change which usage_* variable a consumer reads. Since mise is the main consumer of these specs, worth confirming that is what you expect before this ships.

2. Whether to add #[non_exhaustive] to ParseOutput and UsageErr in the same commit. On its own that is also a major change, so it never pays for itself — but this release is already spending one, which makes now the cheap moment. Say the word and I will fold it in.

---

This comment was generated by Claude Code.

[JamBalaya56562]

Heads-up on the version, and this one is my mistake.

The squash commit came out as

fix(parse): enforce double_dash="required" for positional args (#762)

The ! and the BREAKING CHANGE: footer I put in the branch commit did not survive, because squash takes the PR title and I never updated the title to match. #743 is the counter-example: feat(clap_usage)!: expose spec(), publish against usage-lib 4 carried through, because its title had the !.

So git cliff --bumped-version will read this as a plain fix: and compute v4.1.1, which would ship two breaking API changes as a patch:

• UsageErr gained the ArgRequiresDoubleDash variant (enum_variant_added)
• ParseOutput gained next_arg and double_dash_seen (constructible_struct_adds_field)

lint:semver should go red on the release PR — it compares against the last published version, so 4.1.0 → 4.1.1 still trips both — and auto-merge-release.yml merges with --auto, so it ought to stall there. But that is a backstop rather than a plan.

Simplest fix is to set the version to 5.0.0 on the release PR when it appears. Happy to open a follow-up PR if you would rather have it handled here — just say which.

---

This comment was generated by Claude Code.