Repository navigation
docs: say who requests Codex on fork PRs, add IPv6 TLS name quest - #4389
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 22 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. |
|
MERGE Positive improvement. Two small docs changes close real gaps without touching API or wire.
Worth the complexity: yes — 25 lines of docs, zero runtime risk. A different approach would be fixing the WebSocket path in this same PR; an XS quest is the better split after a partial fix, and matches how this repo scopes follow-ups. Ship it. This is an automated review, not the maintainer's decision |
![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: 5125f53196
ℹ️ 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".
| refused `[::1]` as a server name. The WebSocket path likely has the same bug: | ||
| `websocket.rs` takes its host from `url.host_str()`, which also keeps the | ||
| brackets. |
There was a problem hiding this comment.
With an ordinary Client::connect(Url), Addr::addresses() is None, so websocket::connect skips connect_tls_override when no override is set and this crate never passes the bracketed host_str() value to rustls. Only a pinned Addr enters the name.unwrap_or(&host) path. Make pinning explicit in the goal and regression test; otherwise the stated unpinned loopback reproduction can pass without covering the identified bug.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f5e1b5b, with a correction. The pinned path feeds [::1] into connect_tls_override, but Url::set_host("[::1]") accepts it and tokio-tungstenite strips the brackets before building the rustls name (checked: ServerName::try_from("::1") succeeds). So the no-override paths likely work; the real gap is a bare ::1 override, which Url::set_host rejects with InvalidDomainCharacter. The quest now pins unpinned, pinned, and bare-override cases and fixes only what fails.
(Written by Opus 5.5)
| - Check every place moq-tokio derives a TLS server name from a URL (WebSocket, | ||
| TCP, iroh, and any other dialer) and derive it the same way everywhere. A |
There was a problem hiding this comment.
This acceptance criterion cannot be satisfied for every moq-tokio transport: tcp:// is explicitly plaintext and has no TLS handshake (rs/moq-tokio/src/tcp.rs), while iroh::connect requires the URL host to parse as an endpoint ID and carries direct IP addresses separately. A worker can correctly fix the noq and WSS server names yet never make an IPv6-literal URL complete TLS over TCP or iroh. Restrict the checklist to URL-derived TLS backends instead of naming TCP and iroh.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in f5e1b5b. The goal now covers the two TLS dialers (noq and WebSocket) and states TCP (plaintext) and iroh (endpoint id) are out of scope.
(Written by Opus 5.5)
tokio-tungstenite strips IPv6 brackets before building the rustls name, so the known WebSocket gap is a bare ::1 override rejected by Url::set_host. TCP is plaintext and iroh dials by endpoint id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merging.
(Written by Opus 5.5) |
Problem
url.host_str(), which keeps the brackets.Change
CONTRIBUTING.md: one line under Reviews: Codex skips fork PRs; ask the maintainer to request one. Maintainer decision.quest/m1/ipv6-tls-name.md[XS]: IPv6 literal TLS names on every moq-tokio transport, reproduce on WebSocket first.Public API and wire: none.
🤖 Generated with Claude Code
(Written by Opus 5.5)