feat(spec): add conflicts to flags - #819
Conversation
The spec could say `overrides`, `required_if` and `required_unless`, but not that two flags must not be given together — while mise declares forty `conflicts_with` relationships in clap, every one of which the `From<&clap::Command>` bridge was silently dropping. Anything the derive is going to express has to lower into the spec first, so the spec gains it here. `conflicts` takes one target as a property or several as a child node, mirroring `overrides`. The distinction between the two is the point: `overrides` resolves a collision by letting the last flag win, `conflicts` reports it, for combinations that have no sensible meaning at all. The clap bridge reads `Command::get_arg_conflicts_with`, since clap keeps conflicts on the command rather than on the argument — the conversion is the only place both are in view. clap exposes only the direction the conflict was declared in, which is enough: the check looks at every flag that was given, so either order is refused. Four corpus vectors and a new `conflicting_flags` error code go with it.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 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.
|
Greptile SummaryThe PR adds mutually exclusive flag metadata to the usage specification and preserves clap conflict declarations during conversion.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (4): Last reviewed commit: "fix(spec): name a short-only flag in a c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A flag backed by an env var was only counted as given when it was the flag named by someone else conflicts, not when it was the one doing the naming, so the same pair was a conflict or not depending on which one happened to be typed. clap treats an environment variable as a value source rather than a weaker kind of default and reports the conflict in all three combinations — verified against clap 4 directly, including env-on-both-sides. Two corpus vectors pin it.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a259a46. Configure here.
Every flag in the generated JSON grew a `"conflicts": []`, which is what the
render check caught. The neighbouring lists all skip when empty; this one now
does too.
The render also picks up the first flag the bridge fix rescues: usage`s own
`--multi` declares `conflicts_with("--out-file")` in clap, and its committed spec
had never said so.
A conflict pointing at a flag with no long form was dropped: `get_long()` gives nothing for `-q`, and the spec was left accepting a combination clap rejects — the same class of loss this change set out to fix. Selectors take `-q` as readily as `--long`, so the short form is used when it is the only name a flag has. A conflict pointing at a positional is still dropped, and now deliberately: the spec cannot name one in a conflict, and writing `--<name>` would be a selector matching nothing, which reads as a relationship that holds.
|
Both findings were real; the short-only one was the more serious. Short-only conflicts dropped — fixed. A conflict pointing at a positional is still dropped, and now deliberately, with a comment saying so: the spec has no way to name a positional in a conflict, and writing Empty AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
…er (#820) Stacked on #819, which taught the spec `conflicts`. This teaches the derive to declare it, along with the other two relationships that need only *whether* a flag was given: `required_if` and `required_unless`. ```rust #[derive(usage::Cli)] struct Rel { /// Read from a file #[usage(long, required_unless("--url", "--stdin"))] file: Option<String>, /// Read from standard input #[usage(short = 's', long, conflicts("--file", "--url"))] stdin: bool, /// Where to write #[usage(long, required_if = "--stdin")] out: Option<String>, } ``` Each takes a flag the way the spec names one — `"--long"` or `"-s"` — with one as a value and several as a list. They reach `FlagMeta`, so the emitted KDL says what the struct says and `usage g markdown` and the completions can mention them. ## Worth a look - **A selector naming no flag is a compile error**, and so is one naming its own field. This is the concrete advantage of declaring relationships in code rather than in KDL: a typo'd selector in a hand-written spec is a relationship that quietly does not hold, and nothing ever tells you. Unit-tested through `Cli::from_input`, including resolution by long, short, and negation spelling. - **Env counts on both sides.** The checks read `__given_*` after the environment fallback, so a value from the environment is as good as a typed one — matching clap (verified directly, including env-on-both-sides) and #819's usage-lib behaviour. An asymmetric rule would make the same pair a conflict or not depending on which side happened to be typed. - **Conflicts are reported before required-ness**, matching usage-lib: when a conflict has also left something unfilled, "you gave two flags that cannot go together" beats "give me one more flag". There's a test for that ordering. - **`overrides` is deliberately not here.** It is the one relationship that asks which flag came *last* rather than whether both arrived, and nothing records arrival order yet — so it gets its own PR rather than a half-answer in this one. - The derive docs had a duplicated "Typed values" bullet and still claimed `env` was unread and required-ness unreported, both of which stopped being true a few PRs ago. Corrected. *AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.* <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Adds new post-binding constraint enforcement that changes which command lines parse successfully, but the checks are well-tested and do not touch auth or data handling. > > **Overview** > Teaches the derive to declare **flag relationships** that only need whether a flag was given: `conflicts`, `required_if`, and `required_unless`. > > ```rust > #[usage(long, conflicts("--file", "--url"))] > stdin: bool, > #[usage(long, required_if = "--stdin")] > out: Option<String>, > ``` > > Selectors use the spec's spelling (`"--long"` / `"-s"`), resolve at compile time (unknown or self-referential names are errors), and flow into `FlagMeta` so emitted KDL, docs, and completions see them. Post-binding checks enforce them after env fallback — env counts on both sides — and report conflicts before missing-required errors. `overrides` is deliberately deferred; it needs arrival order. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit b3d833d. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 participant
The spec can say
overrides,required_ifandrequired_unless, but not that two flags must not be given together. mise declares fortyconflicts_withrelationships in clap, and theFrom<&clap::Command>bridge was dropping every one — so a spec generated from mise's CLI silently lost them.Per the canonicality law (code authors, the spec defines), the spec gains the property before the derive can carry it.
What's here
SpecFlag.conflicts, parsed as a property for one target (conflicts="--stdin") or a child node for several, and serialized back the same way — mirroringoverrides.Command::get_arg_conflicts_with. clap keeps conflicts on the command rather than on the argument, so the conversion incmd.rsis the only place both are in view.conflicting_flagserror code.overridesvsconflictsis the distinction worth keeping straight, and it's now documented:overridesresolves a collision by letting the last flag win (--color/--no-color, where a later flag is a correction);conflictsreports it, for combinations with no sensible meaning, where silently honouring one hides the mistake.Notes for review
UsageErr::InvalidFlag { reason: "conflicts with …" }is reused rather than adding a variant:UsageErrisn't#[non_exhaustive], so a new variant would be a breaking change for a diagnostic that reads fine as an invalid-flag reason.env, consistent with howrequired_unlesstreats a selector), never on defaults.AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.
Note
Medium Risk
Touches core argv post-binding validation and the clap→spec bridge, so incorrect conflict handling could accept or reject flag combinations wrongly. Changes are additive and covered by corpus/unit tests.
Overview
Adds
conflictson flags so two flags can be declared mutually exclusive and rejected when both are present — unlikeoverrides, which lets the last one win.The clap bridge now carries
conflicts_withinto the spec (it was previously dropped because clap stores conflicts on the command). The parser enforces conflicts after binding, in either order, including values supplied via env.Also wires
conflicts=--out-fileonto the CLI's--multimarkdown flag, and documents the newconflicting_flagserror in the corpus and spec docs.Reviewed by Cursor Bugbot for commit 5de5a53. Bugbot is set up for automated code reviews on this repo. Configure here.