Repository navigation
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>
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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit 17b756b.
No actionable findings. The narrow backport keeps unknown-parameter skipping limited to drafts 14/15, consumes both KVP representations before decoding later known fields, and applies FORWARD validation only to drafts 15–17. Keeping this separate from the broader codec refactor is appropriate; no alternative implementation is needed.
Review limits: static GitHub review only; tests were not independently run. Check, WASM, and Platform workflows remain in progress. The independent chatgpt-codex-connector review is also running.
(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: 17b756b657
ℹ️ 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".
Co-Authored-By: GPT-6 Astra <noreply@openai.com>
Co-Authored-By: GPT-6 Astra <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a59a4d1e02
ℹ️ 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".
# Conflicts: # js/net/src/ietf/ietf.test.ts
Drafts 14 and 15 have a receiver ignore unrecognized request parameters, repeats included. Rust already skips them; JS still stored any unrecognized id and rejected a repeat as a duplicate. Skip ids JS does not define on the legacy drafts, and cover repeated unknown ids in both languages. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit e003464 against the current release base, accounting for intervening base merges.
[P2] The unknown-duplicate fix remains incomplete for draft 14 at js/net/src/ietf/parameters.ts:652–655. messageParamKind(0x21n) returns filter, so this guard treats LOCATION_FILTER as known even though line 679 explicitly identifies it as undefined in draft 14. A parameter block [2, 0x21, 0, 0x21, 0] falls into params.bytes and throws on its second entry at lines 690–692; draft-14 SUBSCRIBE calls this decoder even though its filter is an inline field. Rust's Parameters::skip accepts that block. Make recognition draft-specific before duplicate checks and add this regression. This is a remaining case of the existing independent finding, referenced here rather than duplicated inline; its 0x3e/0x3d examples are fixed.
The Rust NEW_GROUP_REQUEST finding is also fixed. The narrow release backport remains the right direction; completing the recognition gate is preferable to importing the broader codec refactor.
Verification limits: static GitHub code and regression review only; the byte-path above was traced, not executed. Tests and CI were not independently verified.
(Written by OpenAI)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e003464460
ℹ️ 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".
| (version === Version.DRAFT_14 || version === Version.DRAFT_15) && | ||
| (MSG_PARAM_AFTER_DRAFT15.includes(id) || messageParamKind(id) === undefined) |
There was a problem hiding this comment.
When draft 14 is negotiated, this condition skips only definitions added after draft 15, so IDs first defined in draft 15, including 0x20, are treated as recognized. A draft-14 peer may legally repeat an unrecognized parameter, but two 0x20 KVPs in a SUBSCRIBE parameter block reach the duplicate check below and close an otherwise valid session. Fresh evidence beyond the resolved draft-15 finding is that 0x20 is present in draft 15's parameter registry but absent from draft 14's registry; recognition therefore needs a separate draft-14 gate.
AGENTS.md reference: AGENTS.md:L77-L79
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Problem
Release rejects unknown request KVPs in drafts 14/15 and legal FORWARD on legacy namespace subscriptions. The first backport still parsed fields according to later drafts before deciding whether the negotiated draft recognized them. That rejected valid draft-15 NEW_GROUP_REQUEST and attempted to interpret future or inapplicable filter values.
Approach
Consume unknown legacy parameters using their even-varint/odd-bytes framing, preserving following known fields and bounds checks. Add private declaration gates before semantic parsing in Rust. Apply them to SUBSCRIBE, FETCH and PUBLISH, and recognize NEW_GROUP_REQUEST in draft 15, as specified in section 9.2.1.12.
Synchronize JavaScript's legacy behavior: ignore future definitions, validate a legacy filter only when its consuming message needs it, and validate FORWARD's boolean domain on draft-15/16 namespace subscriptions. Draft 14 still ignores FORWARD there; draft 17 already validates its boolean encoding.
Impact
Alternatives
Adapt the relevant declaration-gate approach from merged #5028 without importing main's codec API refactor. Parsing every declared field first is insufficient because unknown legacy values must be skipped using the negotiated draft's framing.
Validation
Fail-before regressions reproduce Rust SUBSCRIBE/PUBLISH rejection, JavaScript rejection of future FILL_PARAMETERS, and JavaScript acceptance of invalid FORWARD=2 in drafts 15/16. The final JavaScript codec suite passes 128 tests, including a control that a malformed filter still fails when SUBSCRIBE consumes it. The final Rust suite passes 5,074 tests, including the corrected raw-wire PUBLISH regression, plus 387 and 132 transport feature tests. Full release-scoped Nix
just 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. Host configuration, package releases and unrelated main changes are outside this backport.
(Written by GPT-6 Astra)