feat(cli)!: migrate command-line parsing to usage-rs - #3030
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7b4dad4c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut config = Config::parse_from(&argv) | ||
| .map_err(|err| anyhow::anyhow!(usage::render_failure(Config::spec(), &argv, &err)))?; |
There was a problem hiding this comment.
When moq-relay is invoked with --help or --version, Usage returns its Help or Version control-flow error here, but this converts every parser result into anyhow::Error; main then returns it and exits as a failure instead of printing the requested output and exiting 0. The identical conversion in rs/moq-bench/src/config.rs has the same regression. Handle these Usage variants as successful exits, as the custom moq-cli parser does, before wrapping genuine parse failures.
Useful? React with 👍 / 👎.
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
I think Usage is a positive direction for these binaries, but I found three regressions that should be fixed before landing, in addition to the existing moq-relay / moq-bench help-exit finding.
-
[P1]
moqprints no help or version output. Inrs/moq-cli/src/args.rs::parse_error,Help,HelpAll, andVersionare classified as successful control flow, but their message is still built withusage::render_failure. Usage 6.3 explicitly renders those variants as an empty string because callers must handle them first.ParseError::exittherefore exits 0 after printing nothing formoq --help, stage help, andmoq --version. These variants need their actual help page or version response rendered before conversion, with subprocess tests pinning stdout and status. -
[P2] Completion endpoints are bypassed by the custom entry points.
#[usage(completion)]installs the__complete_word__interception in the generatedCli::parse()method.Invocation::parsecallstry_parse_frominstead, while relay and bench call their ownparse_and_mergepaths, so all three feed completion requests into the ordinary grammar and reject them. Callcompletion_requestbefore custom parsing, or expose an upstream process-entry helper that custom loaders can reuse. Formoq, the completion route also needs to account for the repeated stage grammar after--, since the root Usage spec models only the first stage. -
[P2] The old command aliases become visible. Clap's
#[command(alias = "publish")]andsubscribealiases were hidden. For Usage subcommands,aliasis advertised in help and completions;alias_hiddenis the hidden spelling. The migrated attributes onCommand::{Import, Export}therefore reintroduce the old names into the published surface. Usealias_hiddento preserve the existing canonicalimport/exportinterface.
Two design concerns are worth resolving while this is still a spike:
- Relay and bench now implement layer provenance indirectly through serialize, TOML overlay, deserialize,
update_from, alias normalization, and manualhas_long_flagexceptions. Usage's settings/config layer records explicit CLI presence directly and is a better owner forCLI > TOML > env > defaults; otherwise each new default-true boolean needs another parser-specific exception. - The accepted MoQ versions moved from
Version::names()to four literalchoices(...)lists across connect/listen and legacy/current flags. A new protocol version can now parse viaFromStrbut remain unavailable through these CLIs until every copy is updated. Please restore a single source of truth, or at least add a spec regression test comparing every version flag's choices withVersion::names().
(Written by GPT-5)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70e6e8365a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // against a grammar that also carries the process-wide flags a stage refuses. | ||
| // Narrowing that needs the request's own line and cursor rewritten to the last | ||
| // stage, which is not done here. | ||
| if let Some(reply) = Cli::completion_request(args.get(1..).unwrap_or_default()) { |
There was a problem hiding this comment.
When completion is requested with the cursor after a -- stage separator, this always invokes completion against the root Cli grammar. As the preceding comment acknowledges, that grammar includes process-wide flags which are rejected in later stages and does not model the active repeated stage, so shell completion can suggest invalid global flags or omit the applicable endpoint options. Rewrite the completion request to the active chunk and complete it against Stage when the cursor is in a later stage.
Useful? React with 👍 / 👎.
70e6e83 to
2e70524
Compare
… the cursor is in Both findings from the Codex review on #3030. moq-relay and moq-bench turned a help or version request into an `anyhow` error, which renders as an empty string and exits non-zero: `moq-relay --help` printed nothing and failed. moq-cli had the same bug and its own fix, so the rendering now lives in `moq_tokio::cli::answer` and all three share it. A real failure still comes back as an error, so a caller parsing synthetic args keeps its Result. Fixing that exposed what the missing output had hidden: Usage names a spec after the type that declares it unless told otherwise, where clap used the binary name. Both binaries called themselves `config` in every usage line, version string, and completion. They now declare their own names, with a test. Completion answered a later `--` stage against the root grammar, which carries the process-wide flags a stage refuses. Both request shapes normalize to a word list first -- elvish sends one, everything else sends a line and cursor that Usage's own splitter turns into the same thing -- so the active chunk is found without taking apart shell input by hand. A cursor sitting on the separator itself stays with the root, since that is typing `--` rather than being past it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 171405d557
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(file) = config.file.clone() { | ||
| config = toml::from_str(&std::fs::read_to_string(file)?)?; | ||
| config.update_from(&args); | ||
| let mut merged = toml::Value::try_from(&config)?; |
There was a problem hiding this comment.
When a relay uses a positional TOML file together with an environment-backed field marked #[serde(skip)], such as MOQ_AUTH_PUBLIC_SUBSCRIBE or deprecated variables like MOQ_CLIENT_CONNECT, this serialization drops the value before merged.try_into() reconstructs the config. Because update_from reapplies only explicit argv, the current auth setting is ignored and deprecated settings evade resolve() instead of being refused, allowing the relay to start with unintended defaults. Preserve these fields outside the serde round trip or otherwise retain the environment layer.
AGENTS.md reference: AGENTS.md:L121-L123
Useful? React with 👍 / 👎.
| #[derive(usage::Cli, Clone)] | ||
| #[usage(unknown_flags = "error", args_override_self = false)] | ||
| #[usage(completion)] |
There was a problem hiding this comment.
Usage derives the command name from the Rust type unless name or bin is specified, so moq-boy --help now renders usage for config; the same omission makes moq-bench-host --help and --version identify the binary as args, and affects the clock example similarly. Add explicit binary names to these root parsers so their public help, version output, and completion specifications refer to the invoked tools.
Useful? React with 👍 / 👎.
Usage takes a spec's program name from the type that declares it unless told otherwise, where clap used the binary name. moq-boy and the clock example rendered `Usage: config`, and moq-bench-host identified itself as `args`, in help, `--version`, and the completions their specs describe. Found by the Codex review on #3030, after the same defect surfaced in moq-relay and moq-bench once their help stopped printing an empty string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ironment `update_from` re-parses the CLI over the merged config so explicit flags win, and fills a declared default or an environment value only where the standing value is still empty. Emptiness is a property of the type: a bare `bool` reads empty when it is `false`, so TOML saying `false` is refilled from the environment and the file loses. `stats.enabled` and `cluster.lan.enabled` were `Option<bool>` before the Usage migration materialized them; they are again, with the default resolved in code. Every other bare boolean under a Usage derive was checked: the two remaining are a resolved-output struct and moq-bench-host, neither of which merges a file. Scalars are safe, because a plain value always reads as present. `Vec<T>` has the same hazard -- an empty list reads absent -- for about fifteen env-bound list fields. That predates this migration and is unchanged here, so it is tracked in #3051 along with the question of whether the environment should outrank the file at all, which is what most tools do and what this repo does not. rs/CLAUDE.md claimed every shape but `bool` was safe, which is wrong for `Vec`, and doc/bin/relay/config.md presented the precedence as a decision when it is an artifact of parse-then-overlay. Both now say what is true. Found by the Codex review on #3030. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73993f4f58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if answer.is_question() { | ||
| answer.exit(); | ||
| } | ||
| anyhow::bail!("{}", answer.message()); |
There was a problem hiding this comment.
For an invalid moq-relay invocation, this converts Answer::Failure into an anyhow::Error; because main returns that error, Rust terminates with status 1 rather than the parser's documented status 2. The same path exists in rs/moq-bench/src/config.rs, so scripts can no longer distinguish malformed command lines from runtime failures. Keep synthetic parsing fallible for tests, but have the process-facing load() path print and exit through Answer::exit() for parser failures.
Useful? React with 👍 / 👎.
… the cursor is in Both findings from the Codex review on #3030. moq-relay and moq-bench turned a help or version request into an `anyhow` error, which renders as an empty string and exits non-zero: `moq-relay --help` printed nothing and failed. moq-cli had the same bug and its own fix, so the rendering now lives in `moq_tokio::cli::answer` and all three share it. A real failure still comes back as an error, so a caller parsing synthetic args keeps its Result. Fixing that exposed what the missing output had hidden: Usage names a spec after the type that declares it unless told otherwise, where clap used the binary name. Both binaries called themselves `config` in every usage line, version string, and completion. They now declare their own names, with a test. Completion answered a later `--` stage against the root grammar, which carries the process-wide flags a stage refuses. Both request shapes normalize to a word list first -- elvish sends one, everything else sends a line and cursor that Usage's own splitter turns into the same thing -- so the active chunk is found without taking apart shell input by hand. A cursor sitting on the separator itself stays with the root, since that is typing `--` rather than being past it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Usage takes a spec's program name from the type that declares it unless told otherwise, where clap used the binary name. moq-boy and the clock example rendered `Usage: config`, and moq-bench-host identified itself as `args`, in help, `--version`, and the completions their specs describe. Found by the Codex review on #3030, after the same defect surfaced in moq-relay and moq-bench once their help stopped printing an empty string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ironment `update_from` re-parses the CLI over the merged config so explicit flags win, and fills a declared default or an environment value only where the standing value is still empty. Emptiness is a property of the type: a bare `bool` reads empty when it is `false`, so TOML saying `false` is refilled from the environment and the file loses. `stats.enabled` and `cluster.lan.enabled` were `Option<bool>` before the Usage migration materialized them; they are again, with the default resolved in code. Every other bare boolean under a Usage derive was checked: the two remaining are a resolved-output struct and moq-bench-host, neither of which merges a file. Scalars are safe, because a plain value always reads as present. `Vec<T>` has the same hazard -- an empty list reads absent -- for about fifteen env-bound list fields. That predates this migration and is unchanged here, so it is tracked in #3051 along with the question of whether the environment should outrank the file at all, which is what most tools do and what this repo does not. rs/CLAUDE.md claimed every shape but `bool` was safe, which is wrong for `Vec`, and doc/bin/relay/config.md presented the precedence as a decision when it is an artifact of parse-then-overlay. Both now say what is true. Found by the Codex review on #3030. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cluster.lan` is only a field under `cluster-lan`, so the new regression test broke a build with that feature off. The stats half still runs there. Three doc comments also claimed every non-boolean plain value reads as present. An empty `Vec<T>` does not, which is the defect #3051 tracks. Found by the Codex review on #3030. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
73993f4 to
ba5e2db
Compare
Co-Authored-By: GPT-5 <noreply@openai.com>
…-true bools TOML-overridable Six fixes on top of the Usage migration, all found by reading Usage's own semantics against what the migrated attributes assume. Help and version printed nothing. `usage::render_failure` renders `Error::Help`, `HelpAll`, and `Version` as an empty string, because they are questions rather than failures and a caller is expected to take them first; the generated `parse()` does exactly that. moq-cli parses each `--`-separated stage itself, so it never reaches that code and exited 0 having printed nothing for `moq --help`, stage help, and `moq --version`. `parse_error` now renders the page through `usage::help::page` and builds the version line from the spec. `MissingArgsHelp` keeps clap's contract of the short page on stderr with status 2. Completion endpoints were unreachable. `#[usage(completion)]` installs the `__complete_word__` interception in the generated `parse()`, which none of the three binaries use: moq-cli splits on `--` first, and relay and bench load through `parse_and_merge`. Every completion request therefore reached the ordinary grammar and was refused. Each entry point now calls `completion_request` before parsing. A cursor inside a later moq-cli stage still completes against the root grammar, which also offers the process-wide flags a stage refuses; narrowing that needs the request's own line and cursor rewritten to the last stage, and is noted where it would go. A default-true `bool` was refilled over its TOML value. `update_from` fills a declared default only where the standing value is still empty, and a bare `bool` reading `false` is what Usage counts as empty, so `default = "true"` won every merge. That was patched by scanning argv for the three flags it affects, which left the next such field unguarded. `runtime.pin`, `web.ws`, and `connect.websocket.enabled` are `Option<bool>` again with the default resolved in code, and both copies of `has_long_flag` are gone. Every other shape was already safe: a plain value always reads as present, so the rest of the migration's move off `Option` stands. `web.ws` was a bare `bool` on dev too, so this fixes a latent clobber there rather than only restoring parity. The old `publish` / `subscribe` spellings became visible. Usage advertises an `alias` in help and completions where clap hid it; `alias_hidden` is the quiet one. They parse as before and no longer appear on the published surface. The accepted MoQ versions are four `choices(...)` literals, so a new draft can parse through `FromStr` while staying unreachable from the command line. A test walks the spec and holds every `*-version` flag to `Version::names()` as a set. rs/CLAUDE.md carried the new merge rule without its one exception, and still described a hidden flag as a plain `alias`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… the cursor is in Both findings from the Codex review on #3030. moq-relay and moq-bench turned a help or version request into an `anyhow` error, which renders as an empty string and exits non-zero: `moq-relay --help` printed nothing and failed. moq-cli had the same bug and its own fix, so the rendering now lives in `moq_tokio::cli::answer` and all three share it. A real failure still comes back as an error, so a caller parsing synthetic args keeps its Result. Fixing that exposed what the missing output had hidden: Usage names a spec after the type that declares it unless told otherwise, where clap used the binary name. Both binaries called themselves `config` in every usage line, version string, and completion. They now declare their own names, with a test. Completion answered a later `--` stage against the root grammar, which carries the process-wide flags a stage refuses. Both request shapes normalize to a word list first -- elvish sends one, everything else sends a line and cursor that Usage's own splitter turns into the same thing -- so the active chunk is found without taking apart shell input by hand. A cursor sitting on the separator itself stays with the root, since that is typing `--` rather than being past it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Usage takes a spec's program name from the type that declares it unless told otherwise, where clap used the binary name. moq-boy and the clock example rendered `Usage: config`, and moq-bench-host identified itself as `args`, in help, `--version`, and the completions their specs describe. Found by the Codex review on #3030, after the same defect surfaced in moq-relay and moq-bench once their help stopped printing an empty string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ironment `update_from` re-parses the CLI over the merged config so explicit flags win, and fills a declared default or an environment value only where the standing value is still empty. Emptiness is a property of the type: a bare `bool` reads empty when it is `false`, so TOML saying `false` is refilled from the environment and the file loses. `stats.enabled` and `cluster.lan.enabled` were `Option<bool>` before the Usage migration materialized them; they are again, with the default resolved in code. Every other bare boolean under a Usage derive was checked: the two remaining are a resolved-output struct and moq-bench-host, neither of which merges a file. Scalars are safe, because a plain value always reads as present. `Vec<T>` has the same hazard -- an empty list reads absent -- for about fifteen env-bound list fields. That predates this migration and is unchanged here, so it is tracked in #3051 along with the question of whether the environment should outrank the file at all, which is what most tools do and what this repo does not. rs/CLAUDE.md claimed every shape but `bool` was safe, which is wrong for `Vec`, and doc/bin/relay/config.md presented the precedence as a decision when it is an artifact of parse-then-overlay. Both now say what is true. Found by the Codex review on #3030. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cluster.lan` is only a field under `cluster-lan`, so the new regression test broke a build with that feature off. The stats half still runs there. Three doc comments also claimed every non-boolean plain value reads as present. An empty `Vec<T>` does not, which is the defect #3051 tracks. Found by the Codex review on #3030. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dev moved the sink's config into `connect_config` / `quic_config` while this
branch made `quic::Config::{idle_timeout,keep_alive}` concrete. The properties
stay optional, so assigning one through is no longer a no-op when it is unset:
it would override the default with whatever `Option` resolved to. Assign only
what the caller actually set.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba5e2dba78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /// Public UDP address(es) advertised as ICE host candidates (repeatable). | ||
| #[arg(long, requires = "rtc-listen")] | ||
| #[usage(long, requires = "rtc-listen")] |
There was a problem hiding this comment.
--listen
When an RTC listener supplies --public-addr, this constraint still references Clap's internal rtc-listen ID instead of the Usage flag spelling. Unlike the corrected udp_bind constraint immediately above, it therefore cannot recognize the actual --listen argument as satisfying the requirement, so valid listeners that advertise a public ICE address are rejected. Change this target to --listen as well.
Useful? React with 👍 / 👎.
ba5e2db to
6b19d24
Compare
1 participant
Summary
usage-rs, including native completion endpoints, strict unknown and duplicate argument handling, and typed adapters for custom values.moq-cli's repeated stage syntax while delegating each stage's parsing, validation, help, specification, and completions to Usage.Option, backed by a reusable human-readablemoq_tokio::Durationtype.Public API changes
moq_tokio::connect::Config::{race,resolution_delay,timeout}now use concretemoq_tokio::Durationvalues instead of optional standard durations.moq_tokio::connection::Backoff::{initial,multiplier,max,timeout}andGoawayConfig::{redirect,handover}now expose concrete defaulted values.moq_tokio::quic::Config::{idle_timeout,keep_alive},unix::Config::allow, andwebsocket::Config::{enabled,delay}now expose concrete defaulted values.moq_tokio::Durationprovides human-readable parsing, display, serde, conversion, and comparison support.moq-ffi,libmoq, andmoq-gstonly adapt their internal construction to these changes. Their published ABI and wrapper surfaces are unchanged, so bindings and binding documentation were not regenerated.Wire behavior changes
Documentation
moq-cliexamples remain valid because canonical command and flag spellings are unchanged.Test plan
MOQ_STRICT=1 nix develop --command just fixMOQ_STRICT=1 nix develop --command just checkMOQ_STRICT=1 nix develop --command just test(3,472 Rust tests passed, 2 skipped; 52 Python tests passed)git diff --check(Written by GPT-5)