fix!: correct catalog, timeline, token, and teardown contracts found in API review - #2439
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, your pull request is larger than the review limit of 150000 diff characters
…stale `Key` exposed `pub` fields while caching the derived `EncodingKey` and `DecodingKey` in a `OnceLock`. Signing or verifying once populated that cache; mutating `algorithm` or `key` afterwards left it in place, so the next call wrote the new `alg` into the JWT header while signing with the old key material. `scope` was documented as an immutable authorization ceiling but was a `pub` field anyone could overwrite, bypassing the validation in `with_scope`. Split the serialized JWK representation (`Jwk`, all `pub` fields) from an immutable runtime `Key` with private fields, accessors, and validating constructors (`TryFrom<Jwk>`, `from_str`, `generate`, consuming `with_scope` / `with_operations`). Nothing can be reassigned after construction, so the cache can never disagree with the metadata it was derived from. Also fixes a latent serde bug found on the way: a bare `Jwk::deserialize` resolves to serde's `remote = "Self"` inherent method rather than the trait impl, silently skipping the `kty` -> `oct` backfill that keeps previously issued tokens valid. No wire, JWK, or JWT change. `js/token` needs no matching change: it re-imports the key on every sign and verify, so it has no cached-material equivalent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…review
A review of the accumulated pre-release API surface turned up several contracts
worth fixing before they are published, plus one spec conformance bug.
Container forward compatibility (the conformance bug). draft-lcurley-moq-hang
says a consumer MUST ignore a rendition whose container `kind` it does not
recognize. Neither implementation did: `hang::catalog::Container` had no
fallback, so an unknown `kind` failed to deserialize the rendition and took the
whole `Catalog` with it, and `js/hang` threw for the same reason. A publisher
shipping a future container on one rendition would have blacked out every other
rendition for all existing clients. Adds a `Container::Unknown` variant that
preserves the original `kind` and fields verbatim so a relay or transcoder
round-trips it, with the matching passthrough in `js/hang`. A malformed *known*
kind still hard-errors rather than degrading to `Unknown`. The "ignore the
rendition" policy lands in `moq_mux::select` and the watch decoders, and
`moq-ffi` filters such renditions out so the language bindings never see one.
The draft already specified this and needs no change.
Catalog publishing is explicit. `moq_json::snapshot::Guard` and
`moq_mux::catalog::Guard` published from `Drop` and discarded the result, so a
serialization failure in a `CatalogExt` extension, or a write to a closed track,
left the catalog absent or stale while the caller saw success. Adds
`commit(self) -> Result<()>`; `Drop` still publishes for the one-liner form but
now warns instead of swallowing. Also removes an `expect` reachable from that
`Drop`, where a panic during unwind would have aborted the process.
Timeline errors are real. `catalog::Producer::timeline` panicked via `expect`
when its track name collided, through an ordinary public API and unreachable
from `media_producer`, which could not propagate it; both now return `Result`.
The consumer silently rewrote malformed input, turning an invalid timescale into
milliseconds and an out-of-range PTS into zero, which sends seeking and
live-edge logic to the wrong place; both are decode errors now. Drops the unused
`RecordExt` parameter from the publishing side, where recording is crate private
and always supplied the default.
Teardown consumes the handle. `group::Producer::{finish,abort}` and
`track::Producer::abort` took `&mut self`, leaving a usable-but-closed handle.
`abort` now consumes `self` across moq-net and the wrapper types that own a
producer. `finish` deliberately keeps `&mut self`: it declares completion and a
later failure must still be reportable through `abort`, and on a track it only
declares a final sequence while lower-numbered groups may still be written.
Three `Drop` impls became RAII guards, since a type with `Drop` cannot have its
fields moved out.
Corrects the `moq_json::stream` module docs, which promised late joiners the
retained suffix of the log. Past moq-net's 32 MiB group budget they get
`Error::Lagged` and nothing, and a compressed suffix would be undecodable
anyway because its DEFLATE window depends on the evicted prefix. The behavior is
unchanged and intended; only the documentation was wrong.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
force-pushed
the
claude/api-review-pr-2395-14e353
branch
from
July 21, 2026 21:03
15f29a6 to
bedf2ce
Compare
`timeline::Producer::finish` and `catalog::Producer::finish` take `&mut self`, so these tests never needed to finish through a throwaway clone. Bind the producer mutably and finish it directly. Leaves the three `broadcast.clone().finish()` sites in moq-rtmp and moq-srt alone. `broadcast::Producer::finish` consumes `self` for a reason: it only sets `closing`, and the broadcast ends when the last producer handle drops, so consuming the handle is the mechanism rather than a signature preference. Relaxing it to `&mut self` marks intent without ever ending the broadcast, which hangs the moq-hls renditions cursor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`finish` took `self` and only set `closing`, which gates serving new tracks. What consumers actually wait on is the separate `alive` channel, and that closed only once every producer handle dropped. So finishing did not end the broadcast; dropping the last handle did, and consuming `self` was standing in for that. The signature made that unusable for anything holding a producer in a struct, and three gateways worked around it with `self.broadcast.clone().finish()`, which is strictly worse: it sets the flag and drops a throwaway clone while the real handle lives on, so the broadcast keeps lingering. `finish` now takes `&mut self` and closes `alive`, so it ends the broadcast outright whether or not other clones are alive. Existing tracks stay readable, per the no-cascading-close rule. This matches `track::Producer::finish` and `group::Producer::finish`: finishing declares the end, and must not depend on the caller also surrendering the handle. The crate-private `abort(&self)` keeps the old flag-only behavior. It is used by sessions tearing down announced broadcasts, where the broadcast may still linger for a reconnect, so it is a genuinely different operation rather than a `&self` copy of `finish`. Drops the three `clone().finish()` workarounds in moq-rtmp and moq-srt. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe change preserves unrecognized catalog containers through JSON parsing and republishing while marking them unsupported for playback and export. Catalog publishing, timeline creation, timestamp decoding, and codec importer construction now propagate errors. Abort APIs across importers, producers, bridges, and publishers consume owned values, with catalog rendition cleanup moved into RAII helpers. Token keys now separate editable JWK data from validated crypto-ready keys with accessor-based usage. Tests and documentation cover these behaviors. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rs/moq-token/src/key.rs (1)
144-169: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Jwkshould be#[non_exhaustive]with a constructor.
Jwkis a public struct with all-pubfields and noDefault/constructor. Since its doc comment explicitly positions it as the type users build via struct literal before converting intoKey, adding a new field later (e.g. another JWK parameter) would be a breaking change for any external struct-literal construction.As per coding guidelines, "Public Rust config structs with `pub` fields must use `#[non_exhaustive]` and provide `Default` or a constructor."♻️ Proposed direction
+#[non_exhaustive] #[derive(Clone, Serialize, Deserialize)] #[serde(remote = "Self")] pub struct Jwk { ... } + +impl Jwk { + pub fn new(algorithm: Algorithm, key: KeyType) -> Self { + Self { + algorithm, + operations: [KeyOperation::Sign, KeyOperation::Verify].into(), + key, + kid: None, + scope: None, + } + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-token/src/key.rs` around lines 144 - 169, Update the public Jwk struct to use #[non_exhaustive] and provide a construction API, such as a constructor or Default implementation, that initializes its existing fields. Preserve the current serialization attributes and field behavior, while ensuring users no longer need external struct literals to create Jwk values.Source: Coding guidelines
js/hang/src/catalog/container.ts (1)
3-45: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMalformed known containers silently become "Unknown" instead of erroring.
z.union([discriminatedUnion, UnknownContainerSchema])tries members in order. A payload like{kind:"cmaf"}(missinginit) fails the discriminated union but is happily accepted byUnknownContainerSchema(only requireskind: string).containerSupported()then returnstruefor it (since"cmaf"is inKNOWN_KINDS), andisCmafContainer()also returnstrue, so downstream decoders (e.g.js/watch/src/audio/decoder.ts,js/watch/src/video/decoder.ts) treat it as CMAF and read.init, which isundefined— crashingbase64ToBytes(undefined).The Rust reference implementation explicitly guards against this: it inspects
kindfirst and only routes recognized kinds through strict decoding, so a malformed known container is a hard error rather than degrading toUnknown, with a dedicated regression test.Make
UnknownContainerSchemareject known kinds so a malformed known-kind payload fails the whole union instead of falling through.🐛 Proposed fix: reject known kinds in the unknown-container fallback
export const UnknownContainerSchema = z.looseObject({ - kind: z.string(), + kind: z.string().refine((kind) => !KNOWN_KINDS.includes(kind), { + error: "a known container kind must match its schema", + }), });Based on learnings, Rust diffs use
moq_native::tls::Error-style patterns for non-exhaustive enums, but that's not directly applicable here; the relevant guidance is the explicit design intent inrs/hang/src/catalog/container.rs: Only route to the derived enum for kinds we know, so a malformed known container is still a hard error instead of silently becoming Unknown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/hang/src/catalog/container.ts` around lines 3 - 45, Update UnknownContainerSchema so it rejects every value in KNOWN_KINDS while still accepting unrecognized container kinds for verbatim preservation. Keep ContainerSchema’s strict known-kind validation and unknown-kind fallback intact, ensuring malformed legacy, cmaf, or loc payloads fail the union instead of being treated as Unknown.
🧹 Nitpick comments (1)
js/hang/src/catalog/container.ts (1)
29-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
.default()instead ofz._defaultz._defaultis an internal Zod helper, soz.union([...]).default({ kind: "legacy" })keeps this on the documented API surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/hang/src/catalog/container.ts` around lines 29 - 45, Replace the internal z._default wrapper in ContainerSchema with the documented .default() method on the union, preserving the existing container variants and the { kind: "legacy" } fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/lib/rs/env/native.md`:
- Around line 191-192: Update the container documentation around the “Anything
else decodes as Container::Unknown” statement to explicitly document loc as a
supported container mapped to Container::Loc. Keep the existing Unknown behavior
and guidance for unrecognized containers unchanged.
In `@rs/moq-json/Cargo.toml`:
- Line 24: Add tracing to the root workspace.dependencies and update this
crate’s tracing dependency declaration to use the shared workspace version via
workspace = true.
In `@rs/moq-mux/src/container/mkv/import.rs`:
- Line 298: In handle_tracks, construct the media producer via media_producer
before inserting or publishing the rendition configuration, so producer creation
failure cannot leave a stale catalog entry for a dropped track. Preserve
existing error handling for failed track processing, and add a regression test
covering media_producer failure that verifies no rendition/catalog entry is
published.
In `@rs/moq-mux/src/container/ts/import.rs`:
- Around line 1943-1946: Correct the comment above the track-name lookup to
state that Import::finish retains the stream entries and VerbatimEntry cleanup
occurs only when import is dropped. Keep the existing name lookup and finish
behavior unchanged.
- Line 664: Update the track-import flow around catalog.media_producer and the
mpegts.tracks insertion so a media producer construction error removes the newly
inserted catalog entry before propagating the error. Preserve the successful
return path and add a regression test that forces media producer creation to
fail and verifies the advertised track is rolled back.
In `@rs/moq-mux/src/error.rs`:
- Around line 121-135: Add the #[non_exhaustive] attribute to the public Error
enum declaration in rs/moq-mux/src/error.rs, placing it directly above the enum.
Leave the existing variants and their behavior unchanged.
In `@rs/moq-rtc/src/codec/vp8.rs`:
- Around line 34-40: Update the announcement flows in
rs/moq-rtc/src/codec/vp8.rs lines 34-40 and rs/moq-rtc/src/codec/vp9.rs lines
33-39 to mutate through named catalog guards, call guard.commit()? immediately
after inserting the rendition, and set announced only after a successful commit;
add failed-commit regression tests for both VP8 and VP9, preserving Ok(())
without marking the track announced when the catalog is closed.
---
Outside diff comments:
In `@js/hang/src/catalog/container.ts`:
- Around line 3-45: Update UnknownContainerSchema so it rejects every value in
KNOWN_KINDS while still accepting unrecognized container kinds for verbatim
preservation. Keep ContainerSchema’s strict known-kind validation and
unknown-kind fallback intact, ensuring malformed legacy, cmaf, or loc payloads
fail the union instead of being treated as Unknown.
In `@rs/moq-token/src/key.rs`:
- Around line 144-169: Update the public Jwk struct to use #[non_exhaustive] and
provide a construction API, such as a constructor or Default implementation,
that initializes its existing fields. Preserve the current serialization
attributes and field behavior, while ensuring users no longer need external
struct literals to create Jwk values.
---
Nitpick comments:
In `@js/hang/src/catalog/container.ts`:
- Around line 29-45: Replace the internal z._default wrapper in ContainerSchema
with the documented .default() method on the union, preserving the existing
container variants and the { kind: "legacy" } fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1229134b-a7e7-46b2-877d-73e873c35afe
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (72)
doc/concept/layer/hang.mddoc/lib/rs/env/native.mdjs/hang/src/catalog/container.test.tsjs/hang/src/catalog/container.tsjs/watch/src/audio/decoder.tsjs/watch/src/video/decoder.tsrs/hang/src/catalog/container.rsrs/hang/src/catalog/root.rsrs/libmoq/src/publish.rsrs/moq-audio/src/encode/producer.rsrs/moq-cli/src/publish.rsrs/moq-ffi/src/media.rsrs/moq-ffi/src/producer.rsrs/moq-gst/src/sink/pad.rsrs/moq-hls/src/export/mod.rsrs/moq-hls/src/export/rendition.rsrs/moq-hls/src/import.rsrs/moq-json/Cargo.tomlrs/moq-json/src/snapshot.rsrs/moq-json/src/stream.rsrs/moq-mux/src/catalog/hang/container.rsrs/moq-mux/src/catalog/hang/mod.rsrs/moq-mux/src/catalog/producer.rsrs/moq-mux/src/catalog/tracks.rsrs/moq-mux/src/codec/aac/import.rsrs/moq-mux/src/codec/av1/import.rsrs/moq-mux/src/codec/flac/import.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/codec/h265/import.rsrs/moq-mux/src/codec/legacy.rsrs/moq-mux/src/codec/mp3.rsrs/moq-mux/src/codec/opus/import.rsrs/moq-mux/src/codec/video.rsrs/moq-mux/src/codec/vp8/import.rsrs/moq-mux/src/codec/vp9/import.rsrs/moq-mux/src/container/consumer.rsrs/moq-mux/src/container/flv/export.rsrs/moq-mux/src/container/flv/import.rsrs/moq-mux/src/container/flv/import_test.rsrs/moq-mux/src/container/fmp4/export.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/fmp4/muxer.rsrs/moq-mux/src/container/mkv/export.rsrs/moq-mux/src/container/mkv/import.rsrs/moq-mux/src/container/producer.rsrs/moq-mux/src/container/ts/export.rsrs/moq-mux/src/container/ts/import.rsrs/moq-mux/src/error.rsrs/moq-mux/src/import/container.rsrs/moq-mux/src/import/track.rsrs/moq-mux/src/select.rsrs/moq-mux/src/timeline.rsrs/moq-net/src/model/broadcast.rsrs/moq-net/src/model/group.rsrs/moq-net/src/model/origin.rsrs/moq-net/src/model/track.rsrs/moq-rtc/src/codec/av1.rsrs/moq-rtc/src/codec/h264.rsrs/moq-rtc/src/codec/h265.rsrs/moq-rtc/src/codec/mod.rsrs/moq-rtc/src/codec/opus.rsrs/moq-rtc/src/codec/vp8.rsrs/moq-rtc/src/codec/vp9.rsrs/moq-rtc/src/session.rsrs/moq-rtmp/src/dial.rsrs/moq-rtmp/src/server.rsrs/moq-srt/src/ts.rsrs/moq-token/src/generate.rsrs/moq-token/src/key.rsrs/moq-token/src/set.rsrs/moq-transcode/src/rung.rsrs/moq-video/src/encode/producer.rs
💤 Files with no reviewable changes (1)
- rs/moq-mux/src/container/flv/import_test.rs
| serde = { workspace = true } | ||
| serde_json = "1" | ||
| thiserror = "2" | ||
| tracing = "0.1" |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '/^\[workspace.dependencies\]/,/^\[/p' Cargo.toml
rg -n '^\s*tracing\s*=' --glob 'Cargo.toml'Repository: moq-dev/moq
Length of output: 2448
Use the shared tracing dependency Add tracing to the root [workspace.dependencies] and reference it here with tracing = { workspace = true }.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rs/moq-json/Cargo.toml` at line 24, Add tracing to the root
workspace.dependencies and update this crate’s tracing dependency declaration to
use the shared workspace version via workspace = true.
Source: Coding guidelines
Collaborator
Author
There was a problem hiding this comment.
Not applied here: tracing is not in [workspace.dependencies], and all 10 crates that use it declare tracing = "0.1" literally, so moq-json matches the existing convention. Hoisting it to the workspace is worth doing, but it touches every one of those crates and is unrelated to this PR. Leaving this thread open for @kixelated to decide.
🤖 Addressed by Claude Code
(written by Opus 4.8)
…cked Making `catalog::Producer::media_producer` return `Result` earlier in this branch turned three previously-safe orderings into stranded-rendition bugs. Found by CodeRabbit on the PR. `mkv` and `ts` import both published a rendition into the catalog and only then built its media producer. Now that the producer can fail (it mints the rendition's `<name>.timeline.z` track, which can collide) the catalog is left advertising a rendition nothing serves. `ts` is the worse of the two: its `VerbatimEntry` guard, which removes the entry on drop, is only constructed once the function returns successfully, so nothing cleans up. Both now build the producer first and publish only once it exists. The moq-rtc VP8 and VP9 bridges mutated the catalog through a temporary `lock()` guard, whose drop publishes and merely warns on failure, and then latched `announced = true` unconditionally. A closed catalog therefore returned `Ok(())` while leaving the media track advertised nowhere, and `announced` latches so it would never retry. They now commit explicitly and only latch after the commit succeeds, which is what `Guard::commit` was added for. Regression test on the mkv path: it squats the timeline track name so the producer fails, and asserts the rendition is absent. Verified to fail against the previous ordering. Also documents `loc` in the native container list, which listed only legacy and cmaf and so implied `loc` decodes as `Unknown`, and corrects a test comment that claimed `Import::finish` drops the importer (it borrows). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`ContainerSchema` was `z.union([discriminatedUnion, UnknownContainerSchema])`,
and zod tries union members in order. A payload like `{"kind":"cmaf"}` with no
`init` failed the strict CMAF arm and was then happily accepted by the
passthrough arm, which only required `kind: string`.
That is worse than being ignored. `containerSupported` and `isCmafContainer`
both key on the kind string, so the rendition still reported as decodable CMAF,
and the watch decoders went on to call `base64ToBytes(container.init)` on an
undefined init segment.
The passthrough arm now rejects recognized kinds, so a known kind can only ever
parse through its own strict schema and a malformed one fails the whole union.
That restores parity with the Rust side, which inspects `kind` first and only
routes recognized kinds through strict decoding. Regression test verified to
fail against the previous schema.
Also makes `moq_token::Jwk` `#[non_exhaustive]` with a `Jwk::new` constructor.
Its own docs invite building it by struct literal, so a future JWK parameter
would otherwise be a breaking change for external callers. `generate` now builds
through the constructor.
Both found by CodeRabbit on the PR.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two ergonomic regressions from the earlier API fixes, both avoidable without
giving up the invariant each one was protecting.
`moq_token::Key` now wraps its `Jwk` and derefs to it, so `key.algorithm`,
`key.kid`, `key.scope`, `key.operations` and `key.key` read exactly as they did
before this branch. There is deliberately no `DerefMut`: handing out `&mut Jwk`
would let a caller change the algorithm or key material behind the cached crypto
material, which is the whole point of the split. Only construction and mutation
break now, which is the part that has to. This follows the existing idiom in
moq-net, where `group::Producer` derefs to `group::Info`. Drops the five
accessors added earlier, since they duplicate the fields.
`hang::catalog::Container` in JS was widened to `kind: string` by the passthrough
arm, which broke `kind === "cmaf"` narrowing and forced an `isCmafContainer`
guard. The passthrough now maps to a literal tag, `{ kind: "unknown", raw }`, so
`Container` is a proper discriminated union again and tolerating a future
container costs no type safety. `raw` keeps the original object, including its
real `kind`. The type guard is gone; the watch decoders narrow directly.
`containerSupported` is now just `kind !== "unknown"`.
Nothing on the JS publish path parses and republishes a catalog, so the decode
side transform has no wire cost. If that changes, unknown containers need to
serialize from `raw` rather than the tagged shape.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`Key` derefs to its `Jwk`, so the JWK field names are now user-facing, and `key.key` stuttered. The type it holds is not a "type" either: it carries the actual EC/RSA/OCT/OKP parameters, private ones included. Renames `KeyType` to `KeyMaterial` and the field to `material`, so it reads `key.material`. Adds `Jwk::validate(self) -> Result<Key>` as the named counterpart to `Key::try_from`, so the conversion is discoverable from the `Jwk` docs instead of only through a trait impl. `TryFrom` delegates to it. Keeps the `Jwk` and `Key` names. `Jwk` is precise and matches RFC 7517; the friendlier alternatives are either vaguer or, in the case of `KeyInfo`, actively misleading, since unlike the `Info` types in moq-net this one carries private key material and must not read as harmless metadata. No wire change: `material` is `#[serde(flatten)]` and the other fields are serde-renamed, so the JWK JSON is untouched. The JS interop tests, which parse @moq/token-generated keys and verify JS-signed tokens, still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Tagging the passthrough arm `{ kind: "unknown", raw }` made the skip warning print
the literal "unknown" instead of the kind the publisher actually named, which was
the one fact the warning existed to convey. Read it back out of `raw`.
Also corrects three docs the rename and the tagging left behind: the
`UnknownContainerSchema` rationale still claimed the parsed value round-trips
directly (it round-trips through `raw`), `rs/CLAUDE.md` still named `KeyType`, and
the `Jwk` docs pointed only at `Key::try_from` rather than the named
`Jwk::validate` added alongside it.
Found by a review pass over the commits CodeRabbit had not seen.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`Jwk::validate` collided with `Scope::validate` and `Claims::validate`, which check and return nothing rather than converting. It called `scope.validate()?` two lines into its own body, so one verb meant two things in adjacent lines. Renames it to `Jwk::import` and adds the matching `Key::export`, borrowing the verb WebCrypto uses for this exact operation (`importKey`) and that `js/token` already uses on the other side of this crate pair (`importJoseKey`). The pair also gives the reverse direction a discoverable name instead of only an anonymous `From<&Key> for Jwk` impl, which now delegates to `export` just as `TryFrom` delegates to `import`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
enabled auto-merge (squash)
July 22, 2026 16:31
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-mux/src/container/mkv/import_test.rs`:
- Around line 400-405: Update the test around Import::decode to capture and
assert the expected decoding error caused by the injected collision before
inspecting catalog.snapshot().video.renditions. Do not discard the decode
result; preserve the existing empty-renditions assertion after verifying
decoding fails.
In `@rs/moq-net/src/model/broadcast.rs`:
- Around line 651-652: Update the public documentation for Dynamic::closed to
accurately describe the current closure condition, including that
Producer::finish can close the broadcast alongside every producer dropping. Keep
the documented readiness cause as Error::Dropped and align the parent method’s
wording with this behavior.
- Around line 448-465: Change Producer::finish to consume self rather than
borrow it mutably, preserving its terminal behavior of marking the broadcast
closing and signaling alive. Update affected callers to surrender the producer
handle, and keep any nonterminal lifecycle operation separate from finish.
In `@rs/moq-token/src/key.rs`:
- Around line 146-147: Update the documentation for Jwk::import and the
corresponding Key::export text to state that import validates and constructs a
Key, while permitted operations are determined by the JWK’s key_ops. Remove the
claim that every imported Key can sign and verify, preserving accurate wording
for verify-only public JWKs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a3c5ac8-97d5-4691-9a0b-2f3401e0d32e
📒 Files selected for processing (25)
doc/lib/rs/env/native.mdjs/hang/src/catalog/container.test.tsjs/hang/src/catalog/container.tsjs/watch/src/audio/decoder.tsjs/watch/src/video/decoder.tsrs/CLAUDE.mdrs/libmoq/src/publish.rsrs/moq-hls/src/server/mod.rsrs/moq-mux/src/container/mkv/import.rsrs/moq-mux/src/container/mkv/import_test.rsrs/moq-mux/src/container/ts/import.rsrs/moq-native/tests/broadcast.rsrs/moq-net/src/model/broadcast.rsrs/moq-net/src/model/origin.rsrs/moq-relay/src/cluster.rsrs/moq-rtc/src/codec/vp8.rsrs/moq-rtc/src/codec/vp9.rsrs/moq-rtc/src/server/mod.rsrs/moq-rtmp/src/dial.rsrs/moq-rtmp/src/server.rsrs/moq-srt/src/ts.rsrs/moq-stats/src/produce.rsrs/moq-token/src/generate.rsrs/moq-token/src/key.rsrs/moq-token/src/set.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- js/hang/src/catalog/container.test.ts
- doc/lib/rs/env/native.md
- rs/moq-rtmp/src/dial.rs
- rs/moq-mux/src/container/mkv/import.rs
- rs/moq-rtmp/src/server.rs
- rs/libmoq/src/publish.rs
- rs/moq-srt/src/ts.rs
- rs/moq-token/src/set.rs
- rs/moq-rtc/src/codec/vp8.rs
- rs/moq-rtc/src/codec/vp9.rs
- rs/moq-mux/src/container/ts/import.rs
kixelated
disabled auto-merge
July 22, 2026 16:46
The test squats a timeline track name so building the media producer fails, then asserts no rendition was published. If the fixture ever stopped reaching track import, that assertion would pass for the wrong reason. Adds a control that runs the same fixture without the collision and requires it to publish exactly one rendition, so the failure path is provably the thing being measured. Asserting on `decode`'s result instead, as the review suggested, would not work: the importer logs and skips a track it cannot build rather than failing the whole decode, so `decode` returns `Ok`. Verified by trying it. Also corrects two docs. Both `closed()` methods on the broadcast still said closure happens when every producer drops, which stopped being the whole story when `finish` began closing the broadcast itself. And `Jwk::import` claimed to produce a key that "can sign and verify", when what the key may do is whatever `key_ops` allows: a verify-only public JWK imports fine and simply cannot sign. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
enabled auto-merge (squash)
July 22, 2026 16:52
This was referenced Jul 22, 2026
Closed
Merged
Merged
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.
Fixes contracts found while reviewing the accumulated public API for the release in #2395. Targets
main: these APIs have not been published yet, so reshaping them does not break any released contract. #2395 itself is unchanged and can rebase onto this.Two of these are real bugs in behavior, not just API shape: a spec conformance violation that would black out sibling renditions, and a broadcast that never actually ended.
Container forward compatibility (spec conformance)
drafts/draft-lcurley-moq-hang.mdalready says:Neither implementation did.
hang::catalog::Containerhad no fallback, so an unrecognizedkindfailed to deserialize that rendition and took the entireCatalogwith it;js/hangthrew for the same reason. A publisher shipping a future container on one rendition would have blacked out every other rendition for every existing client.Adds
Container::Unknown, preserving the originalkindand fields verbatim so a relay or transcoder round-trips it. A malformed known kind (cmafwith noinit) still hard-errors, so a publisher bug stays loud. The "ignore the rendition" policy lives inmoq_mux::selectand the watch decoders rather than being smeared across every muxer, andmoq-ffifilters such renditions out so the language bindings never see one they cannot decode.On the JS side this was worse than a parse failure: the passthrough arm accepted
{"kind":"cmaf"}with noinit,isCmafContainerstill reported true, and the watch decoders calledbase64ToBytes(undefined). Fixed, and the passthrough now maps to a literal tag{ kind: "unknown", raw }soContainerstays a discriminated union andkind === "cmaf"narrows natively.The draft is already correct, so
drafts/is unchanged. Deliberately not#[non_exhaustive]: that is cross-crate only, so it would strip exhaustiveness checking from the eight moq-mux dispatch sites, which is exactly where the compiler should force a decision, and it does nothing about the JSON-level problem that was the actual bug.Token key encapsulation
moq_token::Keyexposedpubfields while caching the derivedEncodingKey/DecodingKeyin aOnceLock. Sign once to populate the cache, mutatealgorithm, sign again: the JWT header carries the newalgwhile the signature comes from the old key material.scopewas documented as an immutable authorization ceiling but was apubfield that bypassedwith_scopevalidation.Splits the serialized JWK (
Jwk) from an immutable runtimeKey.Keyderefs to itsJwk(with noDerefMut), so reads likekey.algorithmare unchanged and only construction and mutation break. Also renamesKeyTypetoKeyMaterialand its field tomaterial, since it carries the actual EC/RSA/OCT/OKP parameters andkey.keystuttered once it became user-facing.No wire, JWK, or JWT change. Also fixes a latent serde bug: a bare
Jwk::deserializeresolves to serde'sremote = "Self"inherent method and silently skipped thekty->octbackfill that keeps previously-issued tokens valid.js/tokenneeds no matching change; it re-imports the key on every sign and verify, so it has no cached-material equivalent.Catalog publishing and timeline errors
moq_json::snapshot::Guardandmoq_mux::catalog::Guardpublished fromDropand discarded the result, so a failed write, or aCatalogExtthat fails to serialize, left the catalog absent or stale while the caller saw success. Addscommit(self) -> Result<()>;Dropstill publishes soproducer.lock().video = Some(..)keeps working, but warns rather than swallowing. Also removes anexpectreachable from thatDrop, where a panic during unwind would have aborted the process.catalog::Producer::timelinepanicked viaexpecton a track-name collision through ordinary public API, andmedia_producercould not propagate it; both returnResultnow. The timeline consumer silently rewrote malformed input (invalid timescale to milliseconds, out-of-range PTS to zero), which misdirects seeking and live-edge logic; both are decode errors now. Drops the unreachableRecordExtparameter from the publishing side, where recording is crate-private and always supplied the default.Making
media_producerfallible turned two previously-safe orderings into stranded-rendition bugs in the mkv and ts importers, which now build the producer before publishing the rendition. Regression-tested on the mkv path.Teardown
abortconsumesselfacross moq-net and the wrapper types, so use-after-abort does not compile.finishdeliberately keeps&mut self: it declares completion and a later failure must still be reportable throughabort, and on a track it only declares a final sequence while lower-numbered groups may still be written.broadcast::Producer::finishwas not ending broadcasts. It set aclosingflag that only gates serving new tracks; what consumers wait on is thealivechannel, which closed only when the last producer handle dropped. Takingselfwas standing in for that drop. Three gateways worked around the signature withself.broadcast.clone().finish(), which sets the flag and drops a throwaway clone while the real handle lives on, so those broadcasts lingered instead of ending.finishnow takes&mut selfand closesalivedirectly. This is a live bug in released code that this PR fixes as a side effect, so it may deserve a release note.Documentation correction
moq_json::stream's module docs promised late joiners "whatever frames the relay still retains". Past moq-net's 32 MiB group budget they getError::Laggedand nothing at all, and a compressed suffix would be undecodable anyway because its DEFLATE window depends on the evicted prefix. Behavior is unchanged and intended; only the docs were wrong.Notes for reviewers
Cloneover shared state, so.clone().abort()remains possible. Consumingselfcatches the accidental single-owner mistake rather than making misuse impossible.Dropimpls became RAII guards, because a type withDropcannot have its fields moved out.fmp4/mkv/flvImport::abortnow unregisters renditions from the catalog before tearing down tracks rather than after. Same end state, different publish ordering.tracinginto[workspace.dependencies]. It is a real inconsistency with the repo's own convention, but it spans the ten crates that declare it literally and does not belong in this PR.Verification
cargo check --workspace --all-targets,cargo test --workspace(59 binaries, 0 failures),cargo clippy --workspace --all-targets,RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps,just js check, andbun testinjs/hang. Cross-package sync walked:js/hang,js/watch,doc/concept/layer/hang.md,doc/lib/rs/env/native.mdupdated;drafts/andjs/tokenverified as needing no change.(written by Opus 4.8)