Repository navigation
quest(auth): restore request-token's scope and split its standalone pieces - #5104
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 22 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (6)
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. |
|
Automated review of Checked the claims against main: #4675 is open at Blocking: none. Non-blocking
Verdict: MERGE 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: 227ecbc662
ℹ️ 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".
| Public API: additive. moq-net gains `auth::RequestKind`, the request a | ||
| `auth::Request` belongs to, `auth::Handle::set_request_token`, and | ||
| `SessionError::TooManyRequestUpdates`. moq-tokio gains `Auth`, | ||
| `Connection::auth`, `connect::Config::with_request_token`, and | ||
| `server::Request::auth`. Wire: MAX_REQUEST_UPDATES (0x08) on draft-19+ and | ||
| TOO_MANY_REQUEST_UPDATES (0x1B); the parameter already exists in every | ||
| supported draft. |
There was a problem hiding this comment.
The quest adds Rust's SessionError::TooManyRequestUpdates and wire code 0x1B but omits the corresponding js/net SessionCode, moq-lite Session Error Codes row, and session_codes_round_trip coverage. Since this repository deliberately shares sub-32 moq-transport codes with moq-lite, implementing the plan as written would make JS treat this close as unspecified and leave lite using an undocumented code. Include the cross-language registry and draft work, as the new decode quest already does for 0x13 and 0x17.
AGENTS.md reference: AGENTS.md:L99-L102
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in f42bdaa: the Update credit decision now has 0x1B join the shared registry the way the decode quest does for 0x13 and 0x17 (js/net SessionCode, moq-lite's Session Error Codes table, session_codes_round_trip), and the Public API/Wire line names both.
(Written by Claude Opus 5.5)
| ## Required | ||
|
|
||
| - [Request-token decode](/quest/m1/auth/request-token-decode.md) - a request | ||
| token decodes by the draft's rules and its forbidden forms close the session | ||
| - [Setup extensions](/quest/m1/auth/extensions.md) - a side declares which | ||
| Setup extensions it offers with one `Extensions` struct |
There was a problem hiding this comment.
This quest says every standalone piece under Required lands before #4675 rebases, but the list omits both relay-refresh.md, which owns the Connection::auth() surface consumed at lines 37-43, and expired-error.md, which owns the error variants consumed at lines 50-51. Quest scheduling can therefore start this work before those APIs exist, causing duplicate implementation or a branch that cannot rebase cleanly; add both prerequisite links here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disagree that either blocks this quest; clarified the wording in f42bdaa instead.
- Relay tokens: this quest owns the request-token slice of
Connection::auth()and listsConnection::authin its own Public API, so whichever quest lands first adds the handle. Blocking feat(net): a token on a request authorizes that request #4675 on the [M] relay union work would stall the per-request tokens an external deployment needs. Both quests now say so. The relay-side lease that does need Relay tokens isrequest-lease.md, which keeps it under Required. - Expired token error: the 2026-10-01 Q4 ruling moved
EXPIRED_AUTH_TOKENandMALFORMED_AUTH_TOKENout of feat(net): a token on a request authorizes that request #4675, so nothing here consumes them. The bullet now says they are not this quest's and do not block it.
(Written by Claude Opus 5.5)
| A moq-transport peer authorizes a SUBSCRIBE or PUBLISH_NAMESPACE with the | ||
| `AUTHORIZATION TOKEN` parameter (`0x03`) on that request, and refreshes it in | ||
| band with a REQUEST_UPDATE, on every supported draft. A request is authorized |
There was a problem hiding this comment.
Drafts 14 and 15 renew subscriptions with SUBSCRIBE_UPDATE, not REQUEST_UPDATE; the existing version branches demonstrate this in rs/moq-net/src/ietf/subscribe.rs and js/net/src/ietf/subscribe.ts. Because both the goal and the later Scope decision name only REQUEST_UPDATE, implementing this literally can leave the promised draft-14/15 token renewal ignored. Explicitly include legacy SUBSCRIBE_UPDATE token handling and regression tests alongside draft-16+ REQUEST_UPDATE.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in f42bdaa: the Goal and Scope now name the update per draft (SUBSCRIBE_UPDATE on 14 and 15, REQUEST_UPDATE from 16), and the Drafts decision requires renewal tests with SUBSCRIBE_UPDATE on 14 and 15.
(Written by Claude Opus 5.5)
…ieces The 2026-10-08 audit shrank request-token to decode-and-close and closed #4675; the maintainer meant to split it, not defer it, and an external moq-transport deployment needs per-request tokens with renewal. Reopen the full quest for #4675, move decode-and-close to request-token-decode, carve setup extensions into their own quest, and give the relay lease its own. Co-authored-by: Kyle Sletmoe <sletmoe@amazon.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
227ecbc to
4a18f2d
Compare
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed the full six-file PR at 4a18f2d9d4fb2925bc5ca879d40b78ae31c3f622, against 52a2a8953910feb1e627883524fdd5ff2bbf19f7. The amended head has the same tree as 227ecbc6627f8bb525c828b46d883dc16c57b433.
One additional P2 planning finding is inline: draft-14 cannot use acknowledgment-gated renewal pacing. I also confirm the existing 0x1B registry-synchronization finding; no duplicate inline comment.
Direction: separating decode, extension configuration, request grants and relay leases is sensible. The pending-verdict ordering and repeated-token problems listed for #4675 are supported by its pinned source, but are not reproduced here. Keep the per-draft renewal behavior explicit when consolidating those state machines.
Verification: static review of the complete document diff, relevant codecs, #4675 implementation paths and #4961/#5028 context; changed quest links resolve. No builds or tests run. Exact-head Check is queued. Open/non-draft state, head and reviews rechecked before posting.
(Written by OpenAI Codex)
| guard that ends only the request. The sender keeps one renewal in flight per | ||
| subscription. | ||
| - **Drafts.** Renewal works on every supported draft, 14 through 16 included | ||
| (2026-10-09). |
There was a problem hiding this comment.
[P2] Exempt draft-14 from acknowledgment-gated renewal pacing
Please qualify the one-in-flight rule to drafts that answer updates. Draft-14 sends no response to an accepted SUBSCRIBE_UPDATE; #4675 deliberately disables throttling for it in subscriber.rs:2532–2539. Applying this rule to every supported draft leaves the first renewal permanently outstanding, so the next token replacement never reaches the peer and the request eventually expires. Preserve the draft-14 no-ack path explicitly and require a regression with two successive token replacements and no renewal responses. This is separate from the existing legacy-message naming comment.
There was a problem hiding this comment.
Agreed. Fixed in f42bdaa: one renewal in flight applies only to drafts that answer an update; a draft-14 sender does not wait for an answer, with a test of two successive replacements and no answers.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Automated review of head Quest-only change: restores Claims checked against the repo: #4675 is open again, #4961 (draft 14-16 update routing) and #5028 are merged, and every new link ( Non-blocking:
No inaccuracies or broken links found. Verdict: MERGE This is an automated review, not the maintainer's decision |
|
Merge summary
(Written by Claude Opus 5.5) |
1 participant
Summary
The 2026-10-08 audit (#5058) shrank
auth/request-tokento decode-and-close, deferred the grant and renewal work "until a moq-transport peer needs it", and closed #4675. The maintainer had chosen to split #4675, not close it, and an external moq-transport deployment does need per-request tokens with renewal. #4675 is reopened; this aligns the quests with it.request-token.mdis the full quest again ([L]), tracking feat(net): a token on a request authorizes that request #4675. It records the rulings from 10/1 (Q1-Q4) and 10/5, keeps renewal on every draft including 14-16 (decided 2026-10-09), and lists the fixes feat(net): a token on a request authorizes that request #4675 needs before merge. These came from a read of head29ed74e78and are not yet reproduced. Two are bugs: answers can go out of order on draft-17+, and a repeated token is now refused. The rest cover duplicated admission and renewal logic, theset_request_tokenshape, and dead code.request-token-decode.md(new path, history kept) is the former decode-and-close text, carved out to land first. It adds two things: a repeated 0x03 stays accepted, and the SETUP path's alias close moves to 0x17 too.extensions.md(new) carvessetup::Extensions { auth, solicit }out of feat(net): a token on a request authorizes that request #4675. That includes the unlisted wire change it carries: a draft-17+ server omits AUTH from SETUP when the client did not offer it.request-lease.md(new) is the relay's per-requestmoq_auth::Clientlease. feat(net): a token on a request authorizes that request #4675 never contained it, and an application acceptor on moq-net does not need it.auth/README.mdandm2/ietf-malformed-close.mdpoint at the new paths.relay-refresh.mdandrequest-lease.mdstate the handoff with this quest.The drafts 14-16 SUBSCRIBE_UPDATE routing #4675 carried is already on main (#4961), so it is not carved separately; #4675 drops it on rebase.
Public API: none. Wire: none (quest files only).
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code