Skip to content

feat(derive): let parse() answer a failure the way a program does - #910

Merged
jdx merged 2 commits into
agent/versionfrom
agent/parse-diagnostics
Aug 16, 2026
Merged

feat(derive): let parse() answer a failure the way a program does#910
jdx merged 2 commits into
agent/versionfrom
agent/parse-diagnostics

Conversation

@jdx

@jdx jdx commented Aug 16, 2026

Copy link
Copy Markdown
Owner

The second regression from the communique port. parse() rendered a failure with {:?}:

UnknownFlag { token: [45, 45, 110, 111, 112, 101] }

while the clap-shaped rendering sat in this crate unused. communique's main.rs hand-rolled 34 lines to reach it, which is not something an adopter should have to work out.

parse() is the entry point that is the process — it already printed a help page and exited. It now prints the message to stderr and exits 2, clap's status, so a script checking for it keeps working. parse_from still hands the error back, for a library embedding a CLI that wants to decide.

Why the renderer is reached through a function rather than a #[cfg]

Not stylistic. Whether the good rendering exists is a feature of usage-argv in the adopter's graph, and a cfg written into generated code is evaluated in the adopter's crate, where the feature is not theirs to see — which is exactly how a metadata field got silently dropped earlier in this project. So the cfg lives beside the thing it gates: render_failure is the clap-shaped message with diagnostics on, and the Debug form without it, which is what a parser-only build asked for.

What it buys the adopter

With this and #909, communique's port matches clap on --version, -V, every error message tested, and every exit code:

########## --version
  clap : communique 1.3.1 [exit 0]
  usage: communique 1.3.1 [exit 0]
########## badcmd
  clap : error: unrecognized subcommand 'badcmd' [exit 2]
  usage: error: unrecognized subcommand 'badcmd' [exit 2]
########## --fore
  clap : error: unexpected argument '--fore' found [exit 2]
  usage: error: unexpected argument '--fore' found [exit 2]

and main.rs is 34 lines shorter than the port needed before it.

Breaking: parse() returns Self rather than Result<Self, String>. Nothing is released yet.

Verification

Mutation: making render_failure fall back to the Debug form with diagnostics on fails the new test. Both feature configurations — spec alone and spec,diagnostics — build clean.

AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.


Note

High Risk
Breaking change to generated parse()’s return type and all failure/help/version exit paths; every derive adopter’s main must migrate from Result handling to the new process semantics.

Overview
Cli::parse() is now a process-style entry point: it returns Self (breaking change from Result<Self, String>), prints help/version to stdout and exits 0, and on real parse failures prints a user-facing message to stderr and exits 2 (clap’s code). parse_from still returns errors for embedders that want control.

Adds usage_argv::render_failure in the argv crate with #[cfg] beside the gated renderer: with diagnostics, output matches clap-shaped diagnostics; without it, a minimal Debug-style line—so generated code does not need feature cfgs in the adopter crate.

Conformance adds strip_ansi and a test that render_failure wording matches clap for an unknown flag (e.g. --nope).

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

The second thing porting communique turned up. `parse()` rendered a failure with `{:?}` —
`UnknownFlag { token: [45, 45, 110, 111, 112, 101] }` — while the clap-shaped rendering
sat in this crate unused. So communique's `main.rs` hand-rolled thirty-four lines to
reach it, which is not a thing an adopter should have to work out.

`parse()` is the entry point that *is* the process: it already printed a help page and
exited. It now prints the message to stderr and exits 2, which is clap's status, so a
script that checks for it keeps working. `parse_from` still hands the error back, for a
library embedding a CLI that wants to decide.

The renderer is reached through `render_failure` in usage-argv rather than by generating
a `#[cfg]`. That is not a stylistic choice: whether the good rendering exists is a
feature of *usage-argv in the adopter's graph*, and a `cfg` written into generated code
is evaluated in the adopter's crate, where the feature is not theirs to see — which is
exactly how a metadata field once got silently dropped. Without `diagnostics` the same
function gives the Debug form, which is what a parser-only build asked for.

With this and the commit below it, communique's port matches clap on `--version`, `-V`,
every error message tested, and every exit code — and `main.rs` is thirty-four lines
shorter than the port needed yesterday.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 16, 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: ea955af8-5ebd-4a53-8c0d-71b7fc22ba98

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 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR changes generated Cli::parse() into a process-level entry point that renders failures to stderr and exits with status 2, while preserving parse_from() for callers that need error control.

  • Adds feature-appropriate failure rendering to usage-argv.
  • Updates generated parsing behavior and public derive documentation.
  • Adds conformance coverage for diagnostic wording.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
argv/src/lib.rs Adds diagnostics-aware and spec-only implementations of the failure-rendering boundary used by generated CLIs.
derive/src/codegen.rs Changes generated parse() to return the parsed value directly and handle help, version, and parse failures as process outcomes.
derive/src/lib.rs Documents the new distinction between process-oriented parse() and embeddable parse_from().
conformance/tests/version.rs Adds coverage that verifies human-readable failure wording independently of terminal color escapes.

Reviews (2): Last reviewed commit: "test(argv): read the message, not the es..." | Re-trigger Greptile

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 91c27e4. Configure here.

Comment thread conformance/tests/version.rs
@github-actions

Copy link
Copy Markdown
Contributor

Instruction counts

benchmark trend instructions Δ wall (min) Δ
markdown ▁▁▁▂▂▇███ 177,573,547 → 177,450,658 -0.07% 17.36 → 15.88ms -8.52%
startup ▇▁▁▇▁▂▂▂█ 1,221,836 → 1,221,966 +0.01% 1.05 → 0.97ms -7.05%

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 72152 5893576 81x
usage: argv -> struct                            1247 ns      1.25 µs
clap: build tree + parse -> struct             500749 ns    500.75 µs
clap: parse -> struct, tree reused              23344 ns     23.34 µs
clap: build tree only                          320721 ns    320.72 µs

91c27e44c37d vs 6bdf2219e320 · measured on the runner, not pushed to the history.

`render_failure` styles for the terminal it finds itself in, so under `CLICOLOR_FORCE=1`
— or any TTY — the assertion read

    \x1b[1m\x1b[31merror:\x1b[0m unexpected argument '\x1b[33m--nope\x1b[0m' found

and failed on a message that was perfectly correct. A test whose result depends on the
ambient terminal is a flake waiting for the machine that has one.

Stripped before reading. What this test is about is the wording; the colouring has tests
of its own. Checked green under a plain run, `CLICOLOR_FORCE=1` and `NO_COLOR=1`.

Found by Cursor Bugbot.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

jdx commented Aug 16, 2026

Copy link
Copy Markdown
Owner Author

Real, and a good catch — fixed in the head commit.

Reproduced before fixing: CLICOLOR_FORCE=1 cargo test --test version failed with

\x1b[1m\x1b[31merror:\x1b[0m unexpected argument '\x1b[33m--nope\x1b[0m' found

a message that is perfectly correct, failing a starts_with that could not see past the escapes. A test whose result depends on the ambient terminal is a flake waiting for the machine that has one — CI happens not to be a TTY today, which is the only reason it was green.

The message is stripped before it is read. What the test is about is the wording; the colouring has tests of its own. Green under a plain run, CLICOLOR_FORCE=1, and NO_COLOR=1.

AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.

@jdx
jdx merged commit 9fa599e into main Aug 16, 2026
7 of 8 checks passed
@jdx
jdx deleted the agent/parse-diagnostics branch August 16, 2026 22:08
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