Skip to content

fix(argv): inherit unknown_flags, which reached one command out of a tree - #939

Merged
jdx merged 1 commit into
claude/derive-subcommand-requiredfrom
claude/derive-unknown-flags-inherit
Aug 17, 2026
Merged

fix(argv): inherit unknown_flags, which reached one command out of a tree#939
jdx merged 1 commit into
claude/derive-subcommand-requiredfrom
claude/derive-unknown-flags-inherit

Conversation

@jdx

@jdx jdx commented Aug 17, 2026

Copy link
Copy Markdown
Owner

Stacked on #937 (and so on #923) — review those first; this PR's diff is the last commit. The second of the two gaps #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.

#[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.


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.

Reviewed by Cursor Bugbot for commit 2ff6307. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown

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: 9babe7c7-c348-4993-bf21-3f9ce6eb4aba

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

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR moves unknown_flags inheritance into the compiled argv parser so root policies flow through command trees while explicit child policies override them.

  • Represents an unstated command policy as None and carries the effective policy during parser descent.
  • Updates derive output and the conformance adapter to preserve unresolved inheritance.
  • Updates KDL emission to serialize policies only where their effective value changes.
  • Adds nested inheritance, override, and usage-lib parity coverage.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
argv/src/lib.rs Changes command policy storage to an optional declaration and consistently updates the effective parser policy during monotonic subcommand descent.
argv/src/spec.rs Preserves effective unknown-flag semantics in generated KDL while omitting redundant inherited declarations.
derive/src/codegen.rs Emits explicit optional policies for both root and Args-derived command tables, allowing silent child commands to inherit.
conformance/src/argv.rs Stops pre-resolving descendant policies so the conformance adapter exercises parser-owned inheritance.
conformance/tests/unknown_flags_inherit.rs Covers root inheritance, deep descent, child overrides, emitted-spec shape, and usage-lib parity.

Reviews (5): Last reviewed commit: "fix(argv): inherit `unknown_flags`, whic..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Instruction counts

benchmark trend instructions Δ wall (min) Δ
markdown ▁████████ 180,301,068 → 180,365,943 +0.04% 17.50 → 16.53ms -5.57%
startup ▁▆▂▂▅█▆██ 1,226,319 → 1,226,125 -0.02% 1.00 → 0.96ms -3.79%

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 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.

usage clap ratio
instructions, cold parse 60604 5893640 97x
usage: argv -> struct                            1030 ns      1.03 µs
clap: build tree + parse -> struct             496703 ns    496.70 µs
clap: parse -> struct, tree reused              23070 ns     23.07 µs
clap: build tree only                          312822 ns    312.82 µs

2ff6307581d6 vs 02e459bc675b · measured on the runner, not pushed to the history.

@jdx
jdx force-pushed the claude/derive-unknown-flags-inherit branch 2 times, most recently from 86026ea to 0c6a382 Compare August 17, 2026 01:21
@jdx
jdx force-pushed the claude/derive-unknown-flags-inherit branch from 0c6a382 to 3313d52 Compare August 17, 2026 01:56
…a tree

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.

So `Command::unknown_flags` is an `Option` now: `None` means inherit, and the
parser carries the effective value down as it descends rather than reading it
off the command in scope. That is one branch on descent and a field read instead
of a pointer chase on the two sites that ask.

The corpus's table builder stops resolving inheritance on the way in, which is
the part worth noticing: it 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.

Costs **160 instructions per parse — 72,272 against 72,112** at mise's scale,
measured twice against the parent branch on one machine.

Found by converting usage-cli itself to the derive, where every command wants
the strict reading and the five that hand a command line to somebody else's
script want the lenient one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jdx
jdx force-pushed the claude/derive-unknown-flags-inherit branch from 3313d52 to 2ff6307 Compare August 17, 2026 02:01
@jdx
jdx merged commit 14bb879 into main Aug 17, 2026
11 checks passed
@jdx
jdx deleted the claude/derive-unknown-flags-inherit branch August 17, 2026 04:26
pull Bot pushed a commit to rozsazoltan-forks/usage-rust-cli that referenced this pull request Aug 17, 2026
## Summary

- accept `alias` and `alias_hidden(...)` on derived `Args` command
structs
- merge struct aliases with any enum-variant aliases for backwards
compatibility
- reject duplicate command names and aliases at compile time
- document the migration-friendly placement and cover it with
conformance tests

This lets Clap-shaped command structs keep their command metadata when
moving to `usage_derive`, so callers no longer need to relocate aliases
into the parent enum.

## Stack

Targets jdx#939 (`claude/derive-unknown-flags-inherit`). jdx#936 should be
retargeted onto the final PR in this stack.

## Checks

- `cargo test -p usage-conformance --test subcommands`
- `cargo test -p usage-derive`
- `cargo test -p usage-argv`
- full stacked `cargo test --all --all-features`
- full stacked Clippy with warnings denied

_This PR was generated by Codex._

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Changes derive codegen and static command tables used by every CLI;
mistakes could break routing, help, or specs, though conformance tests
cover the new alias paths.
> 
> **Overview**
> **Command aliases can live on derived `Args` structs** (clap-style
migration), not only on `Subcommands` variants. Visible and hidden
aliases on the struct and on the mounting variant are **joined at
compile time** via new `concat_aliases`, and emitted into both parse
tables and metadata.
> 
> **Root `Cli` still cannot declare aliases** — those attributes are
rejected there as misplaced. **Duplicate aliases on one command** fail
at derive time; **`assert_unique_subcommand_names`** panics at compile
time if any two subcommands share a name or alias after merging struct +
variant tables.
> 
> Conformance tests move install/remove aliases onto the `Args` types
and assert spec + parsing behavior (including hidden vs visible
aliases).
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
6a4511b. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
jdx added a commit that referenced this pull request Aug 17, 2026
…#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant