fix(cli)!: refuse the renamed flags with a migration instead of ignoring them - #2915
Conversation
…bout them The `--client-*` / `--server-*` / `--client-quic-*` spellings parse into hidden fields that `Config::resolved()` folds into the canonical ones. Every consumer had to remember to call it, and the binaries read the raw fields instead: `moq` decided whether to dial from `moq.client.url`, so `--client-connect` warned about the rename and then dialed nothing, without erroring or exiting. Same in moq-boy and moq-bench. `quic::Config::resolve()`, which every backend reads its knobs through, skipped the fold too, so `--client-quic-gso=false` reached the iroh bind as "GSO on". Fold once, at each binary's parse boundary, so nothing downstream can read an unfolded value: - moq-cli folds inside `Invocation::try_parse_from`; moq-boy, moq-bench, and moq-relay fold right after `Log::init`. - `quic::Config::resolve()` folds before filling defaults, which closes the one path that bypassed a binary's fold. - The folds are now pure: each config exposes `deprecations()` returning the warning lines, so a caller can fold while parsing (before there is a subscriber to warn to) and report once logging is up. That also stops `listen::Config::has_explicit_bind()` re-warning on every call. moq-cli's tests now parse through `Invocation` rather than `Cli`, so they exercise the fold the process runs; two cluster-lan tests were passing only because `server_config()` folded on its way to the assertion. Also fixes `moq-cli --features transcode`, which stopped compiling when `client::Config::connect` became `connect::Config::url` and which no default feature build covers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing them
The `--client-*` / `--server-*` / `--client-quic-*` spellings parsed into hidden
fields that `Config::resolved()` folded into the canonical ones. Every consumer
had to remember to call it and the binaries didn't: `moq` decided whether to dial
from `moq.client.url`, so `--client-connect` warned about the rename and then
dialed nothing, without erroring or exiting. Same in moq-boy and moq-bench.
`quic::Config::resolve()`, which every backend reads its knobs through, skipped
the fold too, so `--client-quic-gso=false` reached the iroh bind as "GSO on".
A warning that scrolls past is what made this bite: every existing script kept
running, kept printing a line that reads as advisory, and stopped working. So
rather than fix the fold, drop it. A renamed setting now stops the process and
prints what to write instead:
error: these settings were renamed and are no longer applied; update them and try again:
--client-connect / MOQ_CLIENT_CONNECT -> --connect / MOQ_CONNECT
--client-quic-gso / MOQ_CLIENT_QUIC_GSO -> --quic-gso / MOQ_QUIC_GSO (now applies to dialed and accepted connections alike)
The old spellings stay parsable, with their original env vars, purely so the
error can name their replacement; clap would otherwise report an unexpected
argument, and an env-configured deployment would slip past the check entirely.
`moq_tokio::Deprecated` collects them, each binary checks it before reading
anything else, and `Client::new` / `Server::new` check again so a library caller
can't reach an endpoint that ignored half its config. The replacements that don't
mean the same thing say so: `--client-reconnect` inverts, and the per-role QUIC
knobs now apply in both directions.
This deletes the fold and every accessor that existed to read through it
(`resolved()` on each section, `quic::Config::or`, the `effective_*` pile in
tls), so the backends read plain fields.
Scope: only the settings that had bespoke fold code. Plain `#[serde(alias)]`
renames (`[client]`/`[server]` tables, `connect`/`listen`/`failover_delay` keys)
are exact and silent, cost one attribute, and keep working.
Also fixes `moq-cli --features transcode`, which stopped compiling when
`client::Config::connect` became `connect::Config::url`. Nightly's `--all-features`
gate covers that crate but runs on `main` only, so a dev-only break is invisible
to it.
Fixes #2913
Fixes #2917
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kixelated
changed the title
![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: 51a44cbdc2
ℹ️ 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 roots = self.root.clone(); | ||
| let system_roots = self.system_roots; |
There was a problem hiding this comment.
When a consumer flattens the public tls::Connect type and calls its public build() method directly, legacy inputs such as --client-tls-root, --tls-fingerprint, or MOQ_CLIENT_TLS_CERT now remain in the private legacy fields while these lines build the policy exclusively from canonical fields. The Client::new check does not protect this supported direct-builder path, and Connect::deprecated() is private, so callers cannot perform the new check themselves. For example, an old custom-root flag silently falls back to system trust instead of being honored or rejected; reject deprecated values inside build() or expose a check that direct callers can invoke. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L111-L113
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 539e8db, plus a second instance of the same class in 454d946.
You were right that Client::new doesn't protect the direct-builder path, and that a private deprecated() left callers with no way to check. Both Connect::deprecated() and Listen::deprecated() are now public, and the builders refuse rather than relying on the caller:
Connect::verification()— every trust-policy path runs through it, sobuild()and the quiche backend are both covered.Listen::load_roots(),client_verifier(),server_config().
The accept side turned out to be the worse half, and reachable in-repo: moq-cli's export hls flattens tls::Listen, and its listener decides whether to serve TLS at all from the canonical cert/generate fields. So --tls-cert took the "no TLS" branch and served plaintext HTTP without ever reaching the refusing builder, mTLS roots included. The parse-time check now folds in every stage's own args, so that fails before anything binds:
$ moq --connect http://relay/anon --broadcast room export hls --tls-cert /tmp/c.pem --server-tls-root /tmp/ca.pem
error: these settings were renamed and are no longer applied; update them and try again:
--tls-cert / MOQ_SERVER_TLS_CERT -> --listen-tls-cert / MOQ_LISTEN_TLS_CERT
--server-tls-root / MOQ_SERVER_TLS_ROOT -> --listen-tls-root / MOQ_LISTEN_TLS_ROOT
Regression tests: the_client_builder_refuses_a_released_spelling, the_server_builders_refuse_a_released_spelling, a_stage_local_released_spelling_is_refused.
(Written by Opus 5)
`tls::Connect` and `tls::Listen` derive `Args`, so a consumer can flatten either into a parser of its own and call `build()` / `load_roots()` / `server_config()` without ever going through `connect::Config` or `Client::new`. moq-cli's `export hls` does exactly that. On that path the released spellings sat unread in the private field and the builders read the canonical ones, so the values were silently dropped: `--client-tls-root` reads as "no custom roots" and falls back to the system store, and a dropped `--server-tls-root` takes the mTLS client CAs with it, leaving the listener accepting unauthenticated peers. Both widen trust rather than merely losing a setting. Refuse in `Connect::verification` (which every trust-policy path runs through) and in each `Listen` builder, and make both `deprecated()` methods public so a direct consumer can check up front and print the migration itself. Reported by the Codex connector on #2915. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… check Two paths accepted a released spelling and ignored it, which is the failure this PR exists to remove. `export hls` flattens `tls::Listen` into its own stage args, which the parse-time check never saw. Worse, the listener decides whether to serve TLS at all from the canonical `cert`/`generate` fields, so a released `--tls-cert` took the "no TLS" branch and served plaintext HTTP without ever reaching the builder that refuses, with the mTLS roots gone too. The check now folds in every stage's own args, so a future stage that flattens a moq-tokio config has a place to hook into. `iroh::EndpointConfig::bind` is public and takes a `quic::Config` this crate did not necessarily check, then reads it through `resolve()`, which sees only the canonical fields. A released `--client-quic-gso=false` arrived as "GSO on" and bound an endpoint the caller had asked not to have; the canonical spelling errors with GsoUnsupported. Both reported by Codex reviewing #2915. 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: 454d9463fb
ℹ️ 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".
| config = toml::from_str(&std::fs::read_to_string(file)?)?; | ||
| config.update_from(&args); | ||
| } | ||
| config.check_deprecated()?; |
There was a problem hiding this comment.
When users follow the override example in rs/moq-bench/config/hd.toml:7, its --client-connect argument now reaches this check and is rejected, so the documented command no longer starts the benchmark. Update that sample to use --connect, as required for renamed CLI examples. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L197-L197
Useful? React with 👍 / 👎.
The one refusal without coverage, noted in review. It precedes the key file and the socket, so the test also pins that ordering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Fixes #2913. Fixes #2917.
Supersedes the first version of this PR, which fixed the fold so the old spellings worked again. Per review, the shim goes instead: a renamed setting now stops the process and prints the migration.
Root cause
The released
--client-*/--server-*/--client-quic-*spellings parse into hidden fields, andConfig::resolved()folded them into the canonical ones. Nothing forced a consumer to call it, so the binaries read the raw fields:moq-cli'sspawn_moqdecides whether to dial frommoq.client.url. With--client-connect,urlwas stillNone, so the process warned about the rename and then dialed nothing. It did not error and did not exit.moq-boyandmoq-benchhad the same read.quic::Config::resolve(), which produces theResolvedevery backend reads its knobs through, filled defaults straight fromself.iroh::EndpointConfig::bindis handed the freshly parsed config, so--client-quic-gso=falsearrived as "GSO on".Both confirmed on the merge base against a local relay.
Fix
A warning is only ever an addition to honoring the old spelling, never a substitute: it reads as advisory, so the deployment keeps running and stops working. Each setting is now either supported or refused.
--client-connect", which tells nobody what to write; and without the hidden env var, an env-configured deployment slips past the check into exactly the silent case this is fixing.moq_tokio::Deprecatedcollects the findings and renders the migration. Every entry point that reads a canonical field checks it first: each binary at its parse boundary (moq-cliinInvocation::try_parse_fromas a clap error, exit 2;moq-relay/moq-bench/moq-boyvia anyhow, exit 1), plusClient::new,Server::new,iroh::EndpointConfig::bind, and the publictls::Connect/tls::Listenbuilders, so a library caller can't reach one that ignored half its config either.--client-reconnectinverts into--connect-once, and the per-role QUIC knobs now bound both directions rather than one.resolved()on each section,quic::Config::or, and theeffective_*pile intls. Backends read plain fields.Scope
Only settings that needed bespoke fold code. Plain
#[serde(alias)]renames ([client]/[server]tables, theconnect/listen/failover_delay/disable_verifykeys) are exact, silent, and cost one attribute, so they keep working — that is the "support both" half of the rule. The refused set is the flags, their env vars, the[client.quic]/[server.quic]tables, and the TOMLreconnectkey.Unrelated deprecations elsewhere (
cluster.connectbare hosts,mesh = "<url>") are untouched: those warn and apply, so they aren't the failure mode here.Breaking
A deployment still on a released
--client-*/--server-*spelling, or itsMOQ_CLIENT_*/MOQ_SERVER_*variable, now fails to start instead of running with the setting dropped. That is the intent. Worth a release-note line, and worth knowing that an unrelatedMOQ_CLIENT_*left in an operator's environment will now stop every moq binary rather than being ignored.Review round
Codex found two more instances of the same accepted-and-ignored class, both fixed here. Both were security-relevant, because a dropped TLS setting doesn't just vanish — it falls back to something weaker:
tls::Connectandtls::ListenderiveArgs, so a consumer can flatten either into its own parser and callbuild()/load_roots()/server_config()without going throughClient::new. A released--client-tls-rootthen read as "no custom roots" and fell back to the system store; a dropped--server-tls-roottook the mTLS client CAs with it. Bothdeprecated()methods are now public and the builders refuse.moq-cli'sexport hlsflattenstls::Listen, and the parse-time check only covered the globals. Worse, that listener decides whether to serve TLS at all from the canonicalcert/generatefields, so--tls-certtook the "no TLS" branch and served plaintext HTTP without ever reaching the refusing builder. The check now folds in every stage's own args.Codex's re-review verdict was MERGE; its one non-blocking note (no iroh-bind regression test) is closed by
bind_refuses_a_released_quic_spelling.Tests
Each section asserts that a released spelling leaves the canonical field unset and reports the right migration line, so neither half can rot alone:
moq-tokio:connect,listen,quic,tls(both halves),tcp,unix,websocket. Plus the backstops:building_a_client_refuses_a_released_spelling,the_client_builder_refuses_a_released_spelling,the_server_builders_refuse_a_released_spelling,bind_refuses_a_released_quic_spelling, andthe_migration_names_both_spellingsfor the env-var half.every_replacement_is_a_real_flagchecks the QUIC migration targets against the parser, since those replacement names are generated from the flag rather than written out.moq-cli:released_spellings_are_refused_with_a_migrationover six flags at once,a_stage_local_released_spelling_is_refused, andone_released_spelling_is_enough_to_refuse.moq-relay:released_listen_spellings_refuse_to_boot,released_per_role_quic_tables_refuse_to_boot.moq-bench,moq-boy:the_released_connect_spelling_is_refused. moq-boy's existing test passed only because it calledresolved()on its way to the assertion while the binary dialed nothing; it now goes through the same checkmainruns.moq-relay/tests/released_cli.rs(the frozen released surface) still passes unchanged: every old flag and env var must keep parsing, which is what the migration message depends on.Docs
The root
CLAUDE.mdDeprecation section said to keep the deprecated path working but invisible, which the warn-then-ignore case slipped through. It now states the rule this PR follows: supported or refused, never warned-and-dropped, and refusing means keeping the spelling parsable so the error can name its replacement.rs/CLAUDE.mdpoints atDeprecated.No user-facing doc changes: deprecated flags were never documented, and no demo config or justfile uses one.
#2917 (transcode)
moq-cli --features transcodereadsmoq.client.connect, which becameconnect::Config::url. Fixed here since it is the same rename reaching the same config. On the open question there: nightly'sjust rs featuresdoes run--all-featuresover the whole workspace, so it covers the crate — but nightly only runs onmain, and this break isdev-only, so it was never going to see it.Ran
just checkandjust test(1854 passed), pluscargo check -p moq-cli --features transcode,play,capture.(Written by Opus 5)