feat(derive): say that a command cannot be run alone, which it knew and did not write - #937
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (4)
Included review availability: Your plan includes up to 4 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe derive code now computes required-subcommand metadata for root and nested commands. ChangesSubcommand requirement metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The change adds a public Sequence Diagram(s)sequenceDiagram
participant CLI as CLI declaration
participant DeriveCodegen
participant CommandMeta
participant KDLSerializer
CLI->>DeriveCodegen: define subcommand fields
DeriveCodegen->>CommandMeta: emit subcommand_required
CommandMeta->>KDLSerializer: provide command metadata
KDLSerializer-->>CLI: serialize required-subcommand flag
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 aligns derived command metadata with parser behavior by emitting
Confidence Score: 5/5The PR appears safe to merge. The previously reported flattened-subcommand mismatch is prevented by a compile-time assertion on each flattened group, and no blocking failure remains. Important Files Changed
Reviews (5): Last reviewed commit: "feat(derive): say that a command cannot ..." | 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. Shadow comparisonParsing
|
ae69396 to
eb78f7b
Compare
eb78f7b to
a2b02d4
Compare
a2b02d4 to
4332d6f
Compare
|
Addressed the P1, and it was real — worse than reported, in fact. A The spec says Refused at compile time, the way It points at the parent's derive, which is where the flatten is written. The legitimate shape is untouched — There is no compile-fail harness in this repo, so the refusal is verified by hand and documented at the top of This comment was generated by Claude Code. |
…nd did not write A bare `T` subcommand field requires a subcommand and an `Option<T>` does not. The parser has always refused the invocation accordingly — that half was never missing — but `Spec::to_kdl` wrote nothing, so the emitted KDL described `usage generate` as a command a user could type on its own. That reaches past help text. Docs, manpages, completions and the SDK generators all read the emitted spec rather than the Rust type, so every one of them was offering a command that cannot run. Cold metadata, not a parse table: `subcommand_required` is not how a word binds, and the hot path never reads it. Written only where there is something to require, since `subcommand_required` on a command with no subcommands is a spec the linter reports and nothing could satisfy. Found by converting usage-cli itself to the derive and diffing the spec it prints against the one the clap bridge printed. `nesting`'s snapshot picks the change up on a fixture that was already shaped this way, which is the check that this is emission rather than a new rule. Not fixed here, and recorded in PLAN.md beside the root mount it resembles: the spec accepts the property only inside a `cmd` block, so a root that cannot run alone still has no way to say so. The clap bridge could not say it either, so nothing regressed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4332d6f to
8ca7f41
Compare
…a tree (jdx#939) Stacked on jdx#937 (and so on jdx#923) — review those first; this PR's diff is the last commit. The second of the two gaps jdx#936 found by converting usage-cli itself to the derive. usage-lib resolves this by walking outward from the command that ran and falling back to the spec's own setting — `effective_unknown_flags` in `lib/src/parse.rs`. usage-argv held the effective value per command instead, on the theory that whoever built the tables would resolve it. A derive cannot. It expands one struct at a time and cannot see the command above, so `#[usage(unknown_flags = "error")]` reached the root alone — and on an `Args` it was parsed and then **ignored**, a declaration that compiled and did nothing. A CLI could not make a subcommand strict, or a single command lenient, whatever it wrote. ```rust #[usage(bin = "usage", unknown_flags = "error")] // now reaches the whole tree struct Cli { … } #[usage(unknown_flags = "value")] // now means something on an Args struct Bash { … } // `usage bash x.sh --script-flag` ``` Without it, `usage lint --nope f.kdl` offers `--nope` to the positional, so the *file* becomes `--nope` and the real file is the unexpected word — where clap named the flag. ## Shape `Command::unknown_flags` is an `Option` now: `None` means inherit. The parser carries the effective value down as it descends rather than reading it off the command in scope — one branch on descent, and a field read instead of a pointer chase on the two sites that ask. The KDL writer follows: a command that states nothing writes nothing, one that restates what it inherited writes nothing, and one that differs says so. Same output as before for every spec that already round-tripped. **The corpus's table builder stops resolving inheritance on the way in**, which is the part worth noticing: `conformance/src/argv.rs` had its own correct implementation of the rule, and that is exactly why no vector ever caught the parser not having one. One implementation now, in the parser, where the reference has it. ## Cost **160 instructions per parse — 72,272 against 72,112** at mise's scale, measured twice against the parent branch on one machine. cachegrind is deterministic; both figures repeated. ## Verification - `conformance/tests/unknown_flags_inherit.rs` — five tests: a subcommand inherits the root's answer, it reaches two levels down, a command that declares its own keeps it, the emitted spec says what the parser does, and **usage-lib reads the emitted spec the same way**, which is the claim that matters. - `a_subcommand_writes_unknown_flags_only_where_it_differs` gains a command that declares nothing at all, which is the case the `Option` adds. - Workspace suite green, clippy clean, corpus green. _This PR was generated by Claude Code._ <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes core argv binding for unrecognized flags across the whole CLI tree; behavior fixes are intentional but affect every parse path that relied on the old pre-resolved tables. > > **Overview** > **`unknown_flags` now inherits down the command tree** instead of being baked into each command table. `Command::unknown_flags` is `Option<UnknownFlags>` (`None` = inherit); the parser keeps an effective mode on descent and only updates it when a subcommand sets one. > > The derive emits `None` for commands that do not declare the attribute (fixing root-only `error` and ignored `#[usage(unknown_flags)]` on `Args`). KDL emission and the conformance table builder stop flattening inheritance on build—only the parser applies the rule, matching usage-lib’s outward walk. > > New `conformance/tests/unknown_flags_inherit.rs` covers inheritance depth, per-command `value`, spec round-trip, and usage-lib parity on emitted KDL. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 2ff6307. 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 Opus 5 <noreply@anthropic.com>
…#965) Stacked on #963, the `usage-rs` facade. `usage` is now its own first adopter. Its command structs, root, and command enums use the `usage-rs` facade instead of Clap, and `--usage-spec` prints `Cli::to_kdl()` from the same tables that parsed the command line. ## What goes away - `clap`, `clap_usage`, and the `clap-sort` dev dependency - `tests/clap_sort.rs`; declaration order is held by the spec - the duplicated command-effect tables; effects now live on the commands they describe - the empty `Sponsors` struct; unit subcommands are supported directly - runtime checks for relationships the spec can express ## Migration shape The declarations remain close to their former Clap layout: - inferred shorts use `#[usage(short, long)]` - command aliases live on their command structs, e.g. `#[usage(alias = "c", alias_hidden("complete", "completions"))]` - positive dependencies use `requires`, including `--cache-key` → `--usage-cmd` and `--out-dir` → `--multi` - aliases written on enum variants still merge with struct aliases for compatibility - deliberately formatted docs keep `#[usage(verbatim_doc_comment)]` - path hints keep `value_hint = usage_rs::ValueHint::FilePath` or `DirPath` from usage’s own runtime type - subcommand payloads stay unboxed as they were under Clap; `Box<T>` remains an optional size optimization The four shell commands are separate derived structs flattening a shared group. A single destination struct cannot identify which enum variant selected it, so a macro keeps their common declaration in one place. ## Parity gaps closed by the stack - required subcommands survive KDL generation (#937) - strict unknown-flag behavior is inherited by subcommands (#939) - command-owned visible and hidden aliases are supported (#946) - duplicate non-repeatable flags are rejected (#945) - a missing required subcommand prints the available command choices (#947) - verbatim doc-comment layout is preserved on commands, fields, and variants (#949) - file and directory value hints reach native and emitted completions (#951) The emitted spec still intentionally gains metadata the Clap bridge dropped, including `JDX_USAGE_BIN` on `--usage-bin`, preserved multiline long help, the manpage description, and the correct binary name `usage`. ## Verification - `mise run render` - `cargo test --all --all-features` - `cargo clippy --all --all-features --all-targets -- -D warnings` - direct binary checks for no-argument help, duplicate flags, command aliases, and positive requirements _This PR was generated by Codex._ <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes how the primary `usage` CLI is parsed and documented (including `--completions` UX), but behavior is covered by updated integration tests and is an intentional migration to the shipped parser. > > **Overview** > The **`usage` binary dogfoods `usage-rs`**: command structs use `#[derive(usage_rs::Cli)]` / `#[usage(...)]` instead of Clap, and **`--usage-spec` emits `Cli::to_kdl()`** from the same parse tables that handle argv. **Clap, `clap_usage`, and `clap-sort` are removed**, along with the post-hoc **`command_effects` patch table**—`effect`, `requires`, `overrides`, and similar metadata now live on the declarations. > > **Root CLI shape is corrected in spec and docs**: **`--completions <SHELL>` is a long flag** (not a positional), with an **explicit multi-line `usage` synopsis** for `usage <COMMAND>` / `--completions` / `--usage-spec`. Shell runner commands (`bash`, `fish`, `zsh`, `powershell`, `exec`) use **`unknown_flags = "value"`** so script arguments can include unknown flags. > > **Help and manpages** gain support for an optional root **`usage` string** on `Spec` (derive + KDL emission); root help/man synopsis prefer that over generated lines. Generated assets (`usage.usage.kdl`, `usage.1`, Fig spec, CLI reference markdown, snapshots) are refreshed to match. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 3aba1aa. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added `--completions <SHELL>` for selecting a completion shell. * Improved command validation, aliases, defaults, environment settings, and file-output completion hints. * Shell commands now forward unrecognized arguments as script values. * Added clearer help text and descriptions for generation commands. * **Documentation** * Updated CLI references, manpages, usage specifications, and shell documentation to reflect revised options and behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Replaces #936 after flipping the facade and CLI layers. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Stacked on #923 — review that first; this PR's diff is the last commit. One of the two gaps #936 found by converting usage-cli itself to the derive; the other one gets its own PR.
A bare
Tsubcommand field requires a subcommand and anOption<T>does not. The parser has always refused the invocation accordingly — that half was never missing — butSpec::to_kdlwrote nothing, so the emitted KDL describedusage generateas a command a user could type on its own.That reaches past help text: docs, manpages, completions and the SDK generators all read the emitted spec rather than the Rust type, so every one of them was offering a command that cannot run.
Shape
Cold metadata rather than a parse table —
subcommand_requiredis not how a word binds, and the hot path never reads it.CommandMetagains abool, the derive fills it from the field's type, and the writer emits it only where there is something to require: the property on a command with no subcommands is a spec the linter reports and nothing could satisfy, so both conditions are held.gen-shadowalready readsubcommand_requiredfrom a KDL and choseTagainstOption<T>accordingly, so mise's shadow round-trips it now instead of losing it on the way back out.Verification
conformance/tests/subcommand_required.rs— four tests: a strict command says it, a loose one stays quiet and reads back asfalse, a leaf never claims it, and the parser and the spec agree about which invocations exist.nesting's snapshot picks the change up on a fixture that was already shaped this way, which is the check that this is emission rather than a new rule.Not fixed here
The spec accepts
subcommand_requiredonly inside acmdblock, so a root that cannot run alone still has no way to say so — the same restriction as the root mount, recorded next to it in PLAN.md. The clap bridge could not say it either, so nothing regressed.This PR was generated by Claude Code.
Note
Low Risk
Cold-path metadata and KDL emission only; parsing behavior was already correct, with added compile-time validation for an invalid flatten shape.
Overview
Emits
subcommand_requiredin specs so docs, completions, and generators match parser behavior: a non-optional#[usage(subcommand)]field sets cold metadata and writessubcommand_required=#trueonly when the command actually has subcommands; optional fields stay omitted and read back as false.Compile-time guard via
flatten_checks: a#[usage(flatten)]type that still declares subcommands fails const evaluation, because flatten would leave the parent unable to express required subcommands while the nestedbuildstill demands one.Tests and docs: new
conformance/tests/subcommand_required.rs, updated nesting snapshot (settingsgains the flag), and PLAN.md marks the derive/spec gap done while noting root-levelsubcommand_requiredremains unsupported in KDL.Reviewed by Cursor Bugbot for commit 8ca7f41. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation