Repository navigation
fix(net): backport repeated unknown SETUP option handling - #5222
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 69ab205
No actionable correctness findings in the full three-file diff. The scoped SETUP backport looks appropriate: both implementations allow unknown repeats while preserving duplicate rejection for recognized options and consuming each repeated value. The regressions cover unknown odd/even kinds and GREASE across drafts 14–22, plus known duplicates and truncated unknown values on drafts 17–22.
Public API and wire encoding are unchanged; decoder acceptance is broadened as intended. Keeping the unrelated GROUP_ORDER work out of this release backport is the simpler approach.
Verification limits: static GitHub review of the diff and surrounding decoder/setup code only; I did not run tests. Check, Test, WASM, macOS, and Windows CI were still running when checked.
(Written by OpenAI)
![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: 69ab205e58
ℹ️ 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".
| hash_map::Entry::Vacant(entry) => entry.insert(u64::decode(&mut r, version)?), | ||
| }; | ||
| // Unknown SETUP options may repeat, but their values must still decode. | ||
| if !matches!(kind, ParameterVarInt::Unknown(_)) && vars.contains_key(&kind) { |
There was a problem hiding this comment.
For draft 17 and later, MaxRequestId is no longer a recognized SETUP option, as documented on its enum variant, but this check treats every named variant as known regardless of version. A peer sending option 2 twice in a draft 17-22 SETUP is therefore still rejected with Duplicate instead of having the now-unknown option ignored; make knownness version-dependent and mirror that logic in JS's KNOWN_SETUP_OPTIONS.
AGENTS.md reference: rs/moq-net/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Co-Authored-By: GPT-6 Astra <noreply@openai.com>
Co-Authored-By: GPT-6 Astra <noreply@openai.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: ed7441b
No new actionable findings. The retired MAX_REQUEST_ID issue raised in the independent Codex review is fixed in both implementations: option 2 may repeat only in the draft-17+ branch, while drafts 14–16 retain duplicate rejection. The new raw-wire tests cover both sides of that boundary across all supported drafts.
The narrow, version-specific correction is appropriate for this release backport; no broader codec refactor is needed. Public API and wire encoding remain unchanged.
Verification limits: static review of the meaningful delta and surrounding decoders, accounting for the release merge. I did not run tests; current-head CI is still in progress.
(Written by OpenAI)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed7441b71e
ℹ️ 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 !matches!(kind, ParameterVarInt::Unknown(_)) && vars.contains_key(&kind) { | ||
| return Err(DecodeError::Duplicate); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier MAX_REQUEST_ID report is cluster::supported in rs/moq-net/src/ietf/cluster.rs:51-55, which explicitly excludes drafts 14-16. On those drafts, repeated numeric HOP_ID or RELAY_COST options are therefore unknown and must be ignored, but converting them to named enum variants here causes the second occurrence to return Duplicate; the equivalent KNOWN_SETUP_OPTIONS check in JS has the same problem. Make recognizedness depend on both the option and negotiated draft.
AGENTS.md reference: rs/moq-net/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e7c2651: one draft-aware predicate (Rust ParameterVarInt::defined, JS definedSetupOption) now treats HOP_ID/RELAY_COST as unknown on drafts 14-16 and MAX_REQUEST_ID as unknown from 17. SOLICIT and HIDDEN ride every draft, so they stay unique everywhere. A table test covers every named option on drafts 14-22 in both languages.
(Written by Claude Opus 5.5)
HOP_ID and RELAY_COST are only negotiated on draft-17+, so on drafts 14-16 they are unknown options and may repeat, just like MAX_REQUEST_ID from draft-17 on. Replace the per-branch special cases with one draft-aware predicate in Rust and JS, and cover every named option on every draft. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: e7c2651
No new actionable correctness findings in the one-commit delta since ed7441b. The independent legacy-cluster finding is fixed: HOP_ID and RELAY_COST may repeat on drafts 14–16, matching cluster negotiation, while remaining unique from draft 17. The prior MAX_REQUEST_ID fix is preserved. SOLICIT and HIDDEN remain recognized on every supported draft, consistent with their setup consumers.
Centralizing draft-aware knownness is simpler than accumulating decoder-branch exceptions and fits this release backport. The table regressions cover all named varint options, IMPLEMENTATION, and unknown/GREASE IDs across drafts 14–22; they do not cover every named byte option as the description currently claims.
Verification limits: static GitHub review of the delta, decoder context, and extension version gates. I did not run tests or independently verify the reported validation results.
(Written by OpenAI)
|
Merge summary for e7c2651:
(Written by Claude Opus 5.5) |
1 participant
Problem
Release rejects repeated unknown SETUP options that peers are allowed to send. Knownness also depends on the draft: retired
MAX_REQUEST_IDis unknown from draft 17, and the MoQ ClusterHOP_IDandRELAY_COSToptions are unknown before draft 17 (cluster::supported).Approach
Backport the SETUP decoder portion of #4927 in Rust and JavaScript. Keep duplicate rejection for recognized options, but allow unknown options to repeat while decoding and size-checking their values. One draft-aware predicate (Rust
ParameterVarInt::defined, JSdefinedSetupOption) decides knownness:MAX_REQUEST_IDis unique only in drafts 14-16,HOP_ID/RELAY_COSTonly from draft 17, and the other named options on every draft.Impact
Alternatives
Use the narrow decoder backport rather than importing main's codec refactor or unrelated GROUP_ORDER tightening. No compatibility shim is needed.
Validation
A table test in each language repeats every named varint option,
IMPLEMENTATION, and GREASE/unknown ids on drafts 14-22 and assertsDuplicateexactly where the draft defines the option. ForcingHOP_ID/RELAY_COSTback to always-known fails it on drafts 14-16 in both languages. The final Rust suite passes 5,073 tests, plus 387 and 132 transport feature tests. Full release-scoped Nixjust checkandjust test interop --allpass.Interop negotiates Lite; explicit draft-specific raw-wire regressions establish the IETF behavior.
Follow-ups
Main quest cleanup remains gated in #5231 until all six backports land. Main's newer decoder still treats every named option as known on every draft (including
MAX_REQUEST_IDand the cluster options), so it needs the same draft-aware predicate in its own PR; it is not imported into this release backport.(Written by GPT-6 Astra, updated by Claude Opus 5.5)