refactor(net)!: name the hop identifier Hop, and fix three API shapes the review found - #3252
Merged
Merged
Conversation
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. |
![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: 14ecd8ec5c
ℹ️ 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".
| // Hop identity and the `Consume` conversion trait aren't part of a role | ||
| // module; keep them flat at the crate root. | ||
| pub use origin_impl::{Consume, InvalidHop, InvalidOrigin, Origin, OriginList}; | ||
| pub use origin_impl::{Consume, Hop, Hops, InvalidHop}; |
There was a problem hiding this comment.
This commit is based directly on main, but replacing the exported Origin/OriginList types with Hop/Hops removes names from published moq-net 0.2.15 without compatibility aliases; the changed moq-tokio constructors and peer methods similarly break existing callers of version 0.19.11. These are not 0.0.x packages, so this change must target dev rather than making the next main release stop downstream code from compiling. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L217-L219
Useful? React with 👍 / 👎.
| { | ||
| "node": "https://relay-b.example/", | ||
| "origin_id": "200", | ||
| "hop_id": "200", |
There was a problem hiding this comment.
hop_id field
The updated /nodes example promises hop_id, but Node still derives Serialize with a field named origin_id (rs/moq-relay/src/nodes.rs), and its JSON regression test still expects "origin_id". Consequently every real response continues returning the old key, so clients written against this documentation receive no hop_id; rename the field or add an explicit serde rename and update the test. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L203-L203
Useful? React with 👍 / 👎.
Comment on lines
+30
to
+31
| /// Its own id lives in [`Info::id`]. The wire calls the SETUP parameter carrying it | ||
| /// `Origin`, which is why the spec and this type disagree on the name. |
There was a problem hiding this comment.
Origin wire-name claim
This commit renames moq-lite SETUP parameter 0x5 and the matching draft entry to Hop, so this new public API documentation now states the opposite of the implemented specification by claiming the wire still calls it Origin. The same stale claim was added to js/net/src/hop.ts and rs/moq-relay/src/nodes.rs; update all three so published docs describe the current wire name. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L112-L117
Useful? React with 👍 / 👎.
kixelated
added a commit
that referenced
this pull request
Aug 31, 2026
The previous commit renamed `origin_id` to `hop_id` in `doc/bin/relay/http.md` and in the field's doc comment, but not in the `Serialize` field itself, so `GET /nodes` kept emitting `origin_id` against documentation promising `hop_id`. A dashboard updated to the documented contract would have read the Hop ID as absent. The snapshot test compares the whole serialized object for equality, which does reject a renamed key -- but it was carrying the old spelling too, so it agreed with the code and stayed green. Nothing ties that object to the HTTP docs, which is the gap that let the two drift; the test is the contract for code-vs-test, not code-vs-docs. Renames the field, its builder, the local it is computed into, and the `Announced.origins` map (keyed by hop id) with its two use sites. `.internal/origins` is untouched: that is the discovery namespace on the wire, not a hop. Found by Codex in adversarial review of #3252. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Combining two optional subscription bounds comes in two families that disagree on what `None` means, and the names did not say which was which. `min_some` treats it as the neutral element (the other side wins), for intersecting two ranges that each restrict independently. `min_floored` and `max_unbounded` treat it as absorbing, for aggregating across subscribers, where one subscriber asking for everything makes the aggregate everything. Picking the wrong family silently narrows or widens what the publisher sends. `resume.rs` carried a private `max_unbounded_start` that was the `_some` behavior under a `_unbounded` name: the exact inversion the reader would assume. It is `max_some`, the mirror of `min_some`, so it moves next to it in `subscription.rs` and the segment-intersection rationale stays at its call site in `slice`. No public surface: all four are `pub(super)` or private. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`moq_net::track::Subscriber::ordered` consumes the subscriber, so mixing arrival-order and sequence-order reads is a compile error in Rust. A foreign binding cannot express a consuming conversion, so `MoqTrackConsumer` commits on the first group read instead and refused the other cursor afterwards with `MoqError::Unsupported`. That is the same error a caller gets when a feature genuinely is not available (datagrams over IETF moq-transport, a codec this build lacks), so a binding could not tell "MoQ cannot do this here" from "you held it wrong" -- and the two want opposite responses: one is a capability check, the other is a bug in the caller. `Unsupported` now means only the former, and `AlreadyCommitted` names the latter, pointing at the fix (subscribe again for a second cursor). The three `Cursor` arms that are not a committed cursor stop hiding behind a catch-all too. `Uncommitted` and `Converting` are only reachable if a conversion unwound mid-swap, leaving the handle with no subscriber to read: that is a poisoned handle, so it reports `Closed` rather than being folded in with caller misuse. `ErrAlreadyCommitted` joins the Go wrapper's sentinel list; the generated `go/ffi` bindings pick the variant up on their next build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`moq_net` had two public things called Origin. `origin::Producer` / `origin::Consumer`
are a routing table: the broadcasts a peer can serve. `Origin` was something else
entirely, the 62-bit id one relay stamps into a broadcast's hop chain so a receiver can
spot its own id and reject a loop. Same word, unrelated concepts, both exported from the
crate root, and the docs had already started conflating them: `doc/lib/rs/index.md` told
you to "wire an Origin into the client" while linking `struct.Origin.html`, which is the
id and not something you can wire anywhere.
The chain was already half-renamed. `InvalidHop`, `MAX_HOPS`, `Route::hops`, and
`with_hop` all said hop; only the type they are about did not. So `Origin` is `Hop`,
`OriginList` is `Hops`, and the accessors follow: `Client::with_peer_origin` and
`Request::{with_peer_origin, peer_origin}` become `with_peer_hop` / `peer_hop` in both
`moq-net` and `moq-tokio`, and the internal `self_origin` / `session_origin` /
`assigned_origin` fields become `self_hop` / `session_hop` / `assigned_hop`.
`InvalidOrigin` folds into `InvalidHop::Range`. Two error types for one domain bought
nothing: `Hop::new` rejects an out-of-range id and `Hops::push` rejects a chain that
loops or overflows, and callers of either want the same "this hop is not usable"
answer. `InvalidHop` was already `#[non_exhaustive]`, so a third variant costs its
matchers nothing.
`origin::Dynamic::info` returned the id while every other `info()` in the crate returns
an `Info`; it is `hop()` now. `lite::setup::poll_origin` is `poll_hop`.
js/net's `hop.ts` gets the same treatment (`Origin` -> `Hop`, `OriginSchema` ->
`HopSchema`, `UNKNOWN_ORIGIN` -> `UNKNOWN_HOP`, `randomOrigin` -> `randomHop`), where
the collision was worse: `index.ts` re-exports the routing table as `Origin`, so the
package had a `hop.ts` whose main export was named after a different module.
The wire is untouched, so no draft changes. moq-lite's SETUP parameter is still called
`Origin` and `moq-relay`'s admin JSON still reports `origin_id`; both now say so where
the names disagree, including the bike-shed table in `doc/concept/layer/moq-lite.md`.
Nothing crosses the FFI as a type either -- `MoqRoute.hops` is raw `u64` -- so the
generated bindings are unaffected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Client::new(connect::Config, quic::Config)` and `Server::new(listen::Config,
quic::Config)` took two positional configs, so the next knob either becomes a third
parameter or forces a breaking change on every caller. The reason there were two is that
`quic::Config` is shared: one binary flattens the dial half, the accept half, and one
set of QUIC settings that applies to both directions, so neither endpoint config can own
the other.
`client::Config { connect, quic }` and `server::Config { listen, quic }` group them at
the constructor without touching how a binary parses them. Both are `#[non_exhaustive]`
with `Default` and `with_*` builders, so a future knob is a field, and `mod client` and
`mod server` become public to carry them (the flat `Client` / `Server` re-exports stay).
The ergonomic path is unchanged: `connect::Config::init(quic)` and
`listen::Config::init(quic)` are still one call and still the way nearly every caller
builds one, now delegating through the new struct. Adding a field there needs no change
at those call sites at all, which is the point.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last places still calling a Hop ID an origin were the ones users actually read: the wire parameter, the relay's `/nodes` JSON, and `moq --origin`. Keeping them was defensible while the spec disagreed with the code, but the spec is ours, so the disagreement is worth removing rather than documenting. moq-lite SETUP parameter 0x5 is `Hop`, not `Origin`. Its value was already spelled `Hop ID` in the same table, so the parameter was the only thing in the section named after the other concept. The ID and the encoding are untouched: this is an editorial rename within moq-lite-06, which is unpublished (`Lite06Wip`), so the changelog entry that added it is corrected in place rather than gaining a "renamed from" note nobody should need. The Routing and Security sections that referred to "the origin the subscriber declared" now say Hop ID, which is what they always meant. The code follows: `lite::Setup::origin` is `hop`, `PARAM_ORIGIN` is `PARAM_HOP`, and `lite::AnnounceOk::origin` is `hop` (the draft has called that field `Hop ID` all along). Same in js/net, plus the three internal holders of an endpoint's own id (`Connection`, `Publisher`, `Subscriber`). `moq-relay`'s `GET /nodes` reports `hop_id` instead of `origin_id`. It is an unstable debug endpoint whose own docs say not to build on it, so it changes outright rather than carrying both spellings. `moq --origin` is `--hop` (`MOQ_ORIGIN` -> `MOQ_HOP`). That one is a released spelling, so it takes the path the `--client-*` renames took: the old flag stays in the parser hidden, and a process that passes it is refused with the migration rather than started on a setting it would not honor. `Deprecated`'s recorders become public so `moq-cli` can add its own rename to the same message; a config-flattening binary that has to refuse twice makes the caller fix one rename, rerun, and hit the next. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit renamed `origin_id` to `hop_id` in `doc/bin/relay/http.md` and in the field's doc comment, but not in the `Serialize` field itself, so `GET /nodes` kept emitting `origin_id` against documentation promising `hop_id`. A dashboard updated to the documented contract would have read the Hop ID as absent. The snapshot test compares the whole serialized object for equality, which does reject a renamed key -- but it was carrying the old spelling too, so it agreed with the code and stayed green. Nothing ties that object to the HTTP docs, which is the gap that let the two drift; the test is the contract for code-vs-test, not code-vs-docs. Renames the field, its builder, the local it is computed into, and the `Announced.origins` map (keyed by hop id) with its two use sites. `.internal/origins` is untouched: that is the discovery namespace on the wire, not a hop. Found by Codex in adversarial review of #3252. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Hop`'s doc explained that the wire called the same thing `Origin` and that the spec and the type therefore disagreed. The next commit renamed the SETUP parameter to `Hop`, so the caveat documents a discrepancy that no longer exists and points a reader at the wrong name in the draft. Same text in js/net's mirror. Found by Codex in review of #3252. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kixelated
force-pushed
the
claude/api-changes-dev-main-953ba8
branch
from
August 31, 2026 23:09
6f1caa8 to
555f27e
Compare
Three more comments still told a reader the wire parameter is `Origin`, which the rename in this branch made false: the lite publisher's announce filter and its `ExcludeHop` state, and the relay's `/nodes` handler. The earlier pass missed them because it grepped the phrasing it had written rather than the concept, and these are worded differently. `ietf::cluster::Peer::origin` goes with them. Its own doc says it holds what the peer put in RELAY_HOPS, which is a Hop ID, so the field was the last place in the IETF path still calling one an origin. Crate-internal (`mod ietf` is private), so no API change. Found by Codex reviewing #3252; the sweep this time is on the concept, not the wording. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rename left roughly fifty comments across moq-net, moq-tokio, moq-relay, moq-cli, moq-ffi and js/net still calling a Hop ID an "origin id", which is the half-renamed state this branch set out to remove: a reader who follows the prose looks for a type that no longer exists. The sweep is on "origin id" / "origin identity" and the few spellings around the reserved 0, all of which name the identifier and never the routing table. Comments that really do mean `origin::Producer` are untouched, including publisher.rs's excluded-origin handle (a `OnceLock<origin::Consumer>`), the empty-origin defaulting in lite::session, and every "standalone broadcast with no origin". Comment-only: no code, no behavior. The three preceding passes each missed sites because they grepped the phrasing already written rather than the concept. This one enumerates every comment mentioning an origin across the touched crates and judges each. Found by Codex reviewing #3252. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kixelated
added a commit
that referenced
this pull request
Sep 1, 2026
Carries dev's Hop rename (#3252) through the prefix-route refactor. Conflicts were resolved by keeping this branch's route model and re-applying the rename: Origin -> Hop, OriginList -> Hops, InvalidOrigin -> InvalidHop::Range, with_peer_origin -> with_peer_hop, cluster Peer.origin -> Peer.hop, origin::Dynamic::info() -> hop(), the --origin flag and SETUP parameter terminology, and moq-tokio's server::Config. Also lands the review fixes: - announce cursors track delivered prefixes, so a retraction arriving after an undelivered metadata update still reaches the consumer instead of cancelling with it (regression test included) - sync_cursor filters to the most specific covering prefix like best_server, so a scoped cursor advertises the metadata of the route a request resolves - new origin::Consumer::routed_broadcast composes routed + request_broadcast with the coverage-churn retry every caller needs; moq-ffi, libmoq, moq-cli transcode, moq-rtmp, moq-gst, and moq-relay web now share it instead of hand-rolling (or missing) the loop - moq-stats keeps a node's live reader across metadata-only route updates and re-resolves when a subscription ends, so an invisible identical-route failover no longer drops the node from the aggregate permanently - js announced() clamps a broader covering route to the root suffix, matching request() resolution and the Rust prefix intersection (test included) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Started as a review of the API delta between
devandmain, and turned into fixing what the review found. The delta itself is healthy: 464 public signatures removed against 1243 added, and every removal has a named replacement. No lost capability.What it did surface was a naming collision and three smaller shape problems.
Originmeant two unrelated thingsmoq_netexported both from the crate root.origin::Producer/origin::Consumerare a routing table: the broadcasts a peer can serve.Originwas the 62-bit id one relay stamps into a broadcast's hop chain so a receiver can spot its own id and reject a loop.The docs had already lost the thread.
doc/lib/rs/index.mdtold you to "wire anOrigininto the client" while linkingstruct.Origin.html— the id, which isn't something you can wire anywhere.The chain was also already half-renamed:
InvalidHop,MAX_HOPS,Route::hops, andwith_hopall said hop; only the type they were about didn't. So:Origin→Hop,OriginList→HopsClient::with_peer_originandRequest::{with_peer_origin, peer_origin}→with_peer_hop/peer_hop, in bothmoq-netandmoq-tokioInvalidOriginfolds intoInvalidHop::Range— two error types for one domain bought nothing, andInvalidHopwas already#[non_exhaustive]origin::Dynamic::inforeturned the id while every otherinfo()in the crate returns anInfo; it'shop()nowhop.tsgets the same treatment, where the collision was worse:index.tsre-exports the routing table asOrigin, so the package had ahop.tswhose main export was named after a different moduleThen, in a second pass, the three places users actually read:
Hop, notOrigin. Its value was already spelledHop IDin the same table. The ID and encoding are untouched — this is editorial within moq-lite-06, which is unpublished (Lite06Wip), so the changelog entry that added it is corrected in place rather than gaining a "renamed from" note nobody should need.AnnounceOkfollowed too; the draft has called that fieldHop IDall along and only the structs lagged.moq-relay'sGET /nodesreportshop_id. Straight rename, no shim: the endpoint's own docs say don't build on it.moq --originis--hop(MOQ_ORIGIN→MOQ_HOP). The only released spelling of the three, so it takes the path the--client-*renames took: old flag stays hidden in the parser, and a process that passes it is refused with the migration rather than started on a setting it would not honor. That neededDeprecated's recorders to go public — they werepub(crate), so an external binary could consume aDeprecatedbut never contribute to one, andmoq-cliflattening these configs is exactly the case that has to.Three smaller shapes
MoqError::Unsupportedwas overloaded.track::Subscriber::orderedconsumes the subscriber, so mixing arrival-order and sequence-order reads is a compile error in Rust. A foreign binding can't express that, soMoqTrackConsumercommits on first read and refused the other cursor withUnsupported— the same error a caller gets when a feature genuinely isn't available. A binding couldn't tell "MoQ can't do this here" from "you held it wrong", and the two want opposite responses.Unsupportednow means only the former;AlreadyCommittednames the latter and points at the fix.Client::new/Server::newtook two positional configs, so the next knob is either a third parameter or a breaking change.client::Config { connect, quic }andserver::Config { listen, quic }group them without touching how a binary parses them —quic::Configis shared between both directions, so neither endpoint config can own the other.connect::Config::init(quic)is unchanged and still the way nearly every caller builds one.resume.rshad a privatemax_unbounded_startthat was_somebehavior under a_unboundedname — the exact inversion a reader would assume. Combining optional bounds comes in two families that disagree on whatNonemeans, and picking the wrong one silently narrows or widens what the publisher sends. Nowmax_some, next tomin_some, with the two families spelled out. Internal only.Not changed
cluster.md's "origin identity" and the cluster draft'sRELAY_HOPSare about the content's origin publisher, a genuinely different concept.Role::from_originsis about the publish/subscribe origin handles.origin::Producerstill has a private field namedinfoholding aHop; untangling it means touching a dozen sites whereinfois also a local for the realInfo, and the public surface reads correctly now.Verification
nix developwas deadlocked on a shared eval-cache SQLite lock held by another worktree, sojust check/just testdid not run end to end. Equivalent tools ran directly against the same nix-storerust 1.95.0:-D warnings, 20 packages — cleancargo nexteston moq-net/moq-tokio/moq-cli/moq-relay — 1695 passedcargo fmt,biome check— cleantsc --noEmit+bun teston@moq/net— 604 pass, 0 failkramdown-rfc+xml2rfc --preptoolondraft-lcurley-moq-lite— validUncovered versus a real
just check: the moq-doc build, the wasm and feature-gated selections, publint, remark, taplo. CI is the gate on those.Cross-package sync
js/netanddoc/move withmoq-net; the draft moves with the wire. Nothing crosses the FFI as a type (MoqRoute.hopsis rawu64), so no bindings regenerate — onlygo/wrappergains theErrAlreadyCommittedsentinel, and the generatedgo/ffipicks the variant up on its next build. Interop (just test smoke-full) not run locally.🤖 Generated with Claude Code
(Written by Claude Opus 5)