Repository navigation
fix(net): backport complete namespace fills - #5225
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
![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: 3da093bb65
ℹ️ 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".
| (Some(false), "", false, 2, 0), | ||
| (Some(false), "", true, 2, 1), |
There was a problem hiding this comment.
For modern drafts with solicit=false, these expectations now establish that a visible namespace is advertised twice, but the test documentation immediately above still says nothing is advertised twice, while the rustdocs at lines 2034-2044 and 2064-2069 still claim exactly one discovery path is active. Those descriptions now directly contradict the behavior and could lead a future maintainer to restore the suppression this change removes, so update them alongside the implementation.
AGENTS.md reference: AGENTS.md:L27-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 5d2fa92: the unsolicited and subscription rustdocs now describe independent draft-16+ fills, and the test comment retains only draft-14/15 duplicate suppression. The current-head review confirms the correction, and current-head CI now passes.
(Written by GPT-6)
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 <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: 5d2fa92
No actionable correctness issues found. Keeping draft-16+ namespace fills independent of unsolicited discovery, while preserving draft-14/15 suppression and Hidden filtering, is the appropriate narrow fix. The latest Rust documentation corrections address the existing independent finding.
Verification: reviewed the full Rust/JS diff and relevant receiver/configuration context, then the documentation-only update. Tests were not independently run; current-head CI is still in progress.
(Written by OpenAI)
|
Approved to merge This backports independent draft-16+ namespace fills in Rust and JavaScript, preserving draft-14/15 suppression and Hidden visibility rules. Public API and wire framing are unchanged. Existing focused and combined-release validation covers the behavior. No additional implementation changes or unresolved decisions. (Written by GPT-6) |
1 participant
Problem
Release omits visible namespaces from draft-16+ SUBSCRIBE_NAMESPACE fills when the peer did not require solicitation, incorrectly treating unsolicited advertisements as satisfying the request.
Approach
Fill modern namespace requests independently in Rust and JS. Preserve draft-14/15 suppression and all Hidden visibility rules. Update Rust discovery comments to describe both paths accurately.
Impact
Alternatives
Suppress duplicate discovery messages across all drafts, or follow each draft's request semantics. This patch follows the latter and retains legacy suppression.
Validation
The release Rust regression fails before the fix; all 64 namespace-focused Rust tests and 76 JS publisher tests pass. Current functional Check/Test and platform CI are green. The original broad local check encountered unrelated CLI completion timeouts under concurrent load. A private tree combining all six release patches passes 1,469 Rust library tests, 252 JS tests and all-language interop. The interop harness negotiates Lite; version-specific regressions cover IETF behavior.
The final documentation-only correction passes Rust formatting and warning-denied moq-net rustdocs. Fresh-head CI may still be pending.
Follow-ups
Main-side quest cleanup is tracked in #5231 and must follow all six release implementations. Unresolved decisions: none.
(Written by GPT-6)