refactor(moq-native): migrate from anyhow to thiserror#1651
Conversation
Replace anyhow with a single source-chained `Error` enum (thiserror), in line with the library-crate convention. `#[from]` covers the upstream transport/proto/IO error types so `?` keeps working, and descriptive `#[source]` variants preserve the old `.context()` messages. Public functions now return `moq_native::Result<T>` instead of `anyhow::Result<T>`. Propagation via `?` and the `anyhow::Context` trait still compile for callers, but returning a moq-native result directly into an `anyhow`-typed position (tail expression, `tokio::select!` arm) now needs a `?` or `.map_err(Into::into)`. cargo-semver-checks will flag this and bump the minor version. anyhow moves to dev-dependencies for the examples and tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a crate-level typed Error enum and Result alias in moq-native, replaces runtime anyhow usage with thiserror-based variants, and updates Cargo deps (thiserror at runtime, anyhow moved to dev-dependencies). TLS, backends (quinn/quiche/noq/iroh/websocket), client, server, reconnect, utilities, and examples/CLI/libmoq callers are migrated to return crate::Result with explicit map_err/From conversions; reconnect.closed()/status() callers are updated to propagate or convert errors with ?. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
`Error::Connect`/`Error::InitFailed` hold `Arc<anyhow::Error>`, but the moq-native calls they wrap now yield `moq_native::Error`. Add `.into()` to convert. libmoq is excluded from the workspace default-members (needs a C toolchain), so `just check` without `--workspace` missed it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split the monolithic `Error` into per-backend types so each backend fully
describes its own failures:
- `moq_native::tls::Error` for certificate/key loading (shared by the client
TLS config and the quinn/noq servers).
- `moq_native::{quinn,noq,quiche,iroh,websocket}::Error`, each self-contained.
The top-level `moq_native::Error` now aggregates them via `#[from]` and keeps
only cross-cutting variants (log, reconnect, backend selection, status code).
This also drops the quinn/noq `#[from]` collision workaround, since each
backend's proto error types now live in their own enum.
libmoq grows a `#[from] moq_native::Error` (Arc-wrapped to stay `Clone`), so
the `.init()` calls use `?` instead of stringifying through anyhow.
Examples and CLIs use `Ok(reconnect.closed().await?)` instead of
`.map_err(Into::into)` at the reconnect boundary.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…y-a366bd # Conflicts: # rs/moq-native/src/tls.rs
kixelated
left a comment
There was a problem hiding this comment.
IMO don't use Result if it means you have to fully qualify std::result::Result. I'd rather have the opposite; fully-qualify the type alias.
Wrap each per-backend payload (and the non-Clone leaf sources: io, log init/directive) in `Arc` inside the top-level `Error`, with hand-written `From` impls so `?` stays ergonomic. The backend error types themselves remain as-is. Lets `reconnect` keep the real terminal `Error` (instead of a stringified copy) and lets libmoq use a plain `#[from] moq_native::Error` rather than an Arc wrapper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-native/src/error.rs`:
- Around line 17-18: The Directive variant currently wraps
Arc<tracing_subscriber::filter::ParseError> but lacks the #[source] attribute,
so implementors of std::error::Error won't expose the underlying ParseError; add
the #[source] attribute to the Directive enum variant (the
Directive(Arc<tracing_subscriber::filter::ParseError>) entry in the error enum
in error.rs) so Error::source() returns the inner parse error and downstream
diagnostics include the original error details.
🪄 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: 7f174d4b-f407-4b01-a748-53b386adcd03
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
rs/hang/examples/subscribe.rsrs/hang/examples/video.rsrs/libmoq/src/api.rsrs/libmoq/src/error.rsrs/libmoq/src/session.rsrs/moq-boy/src/main.rsrs/moq-cli/src/client.rsrs/moq-cli/src/main.rsrs/moq-native/Cargo.tomlrs/moq-native/examples/clock.rsrs/moq-native/src/client.rsrs/moq-native/src/error.rsrs/moq-native/src/iroh.rsrs/moq-native/src/lib.rsrs/moq-native/src/log.rsrs/moq-native/src/noq.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rsrs/moq-native/src/reconnect.rsrs/moq-native/src/server.rsrs/moq-native/src/tls.rsrs/moq-native/src/util.rsrs/moq-native/src/websocket.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- rs/hang/examples/subscribe.rs
- rs/moq-native/src/log.rs
- rs/moq-native/src/websocket.rs
- rs/moq-native/src/client.rs
- rs/moq-native/src/server.rs
- rs/moq-native/src/tls.rs
- rs/moq-native/src/noq.rs
- rs/moq-native/src/iroh.rs
…sult Address review: expose `crate::tls::Result` and use it for the cert-loading APIs (build, load_roots, quiche cert helpers). Move `ClientTls::build`'s rustls::ClientConfig construction (and NoCertificateVerification) into `crate::tls::client_config`, and extract the duplicated PEM-file reading shared by the client, server load_roots, and ServeCerts into `crate::tls::read_certs`. `ClientTls::build` is now a thin delegate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e chain `Directive` uses a custom display string (not transparent), so without `#[source]` the wrapped `ParseError` was dropped from `Error::source()`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`ClientTls` now lives in `crate::tls` as `tls::Client`, with `build()` as an inherent method (the former `tls::client_config` free fn). `ClientConfig.tls` and moq-relay's auth config reference the new path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rver Mirrors the ClientTls -> tls::Client move: `ServerTlsConfig` now lives in `crate::tls` as `tls::Server` with `load_roots()` as an inherent method. `ServerConfig.tls` and moq-relay's doc comment reference the new path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
While the API is already breaking, fold in three module-scoping cleanups:
- websocket: `ClientWebSocket` -> `websocket::Config`, `WebSocketListener` ->
`websocket::Listener`; drop the `pub use websocket::*` glob. (Config rather
than `Client` because clap names a flattened arg-group after the struct, and
`tls::Client` + `websocket::Client` would collide inside ClientConfig.)
- iroh: drop the `Iroh` prefix — `IrohEndpoint`/`IrohEndpointConfig`/`IrohRequest`
become `iroh::{Endpoint, EndpointConfig, Request}`; drop the root re-export.
- `ServerTlsInfo` -> `tls::Info`, so all TLS types live in the tls module.
moq-relay, moq-cli, and tests updated to the new paths.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clap derives an ArgGroup named after each flattened struct, so two configs named `Client` (tls + websocket) collided inside ClientConfig. Set explicit `#[group(id = ...)]` on the flattened config structs (matching moq-relay's existing `*-config` convention), which decouples the clap group from the struct name and lets the WebSocket config keep the symmetric `websocket::Client` name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`tls-client` / `tls-server` / `websocket-client` instead of the inconsistent `*-config` ids, so each flattened config's clap group matches its `module::Type`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nd modules
Move `pub use web_transport_{quinn,noq,quiche}` off the crate root into the
matching `quinn`/`noq`/`quiche` modules, and add `web_transport_iroh` to the
`iroh` module for parity. So e.g. `moq_native::quinn::web_transport_quinn`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Migrates
rs/moq-nativefromanyhowtothiserror, following the library-crate convention in CLAUDE.md (libraries usethiserrorwith#[from]; binaries keepanyhow).Errors are split per concern so each type fully describes its own failures:
moq_native::tls::Error— certificate/key loading, shared by the client TLS config and the quinn/noq servers.moq_native::quinn::Error,noq::Error,quiche::Error,iroh::Error,websocket::Error— one self-contained error type per backend, each returning its ownResult.moq_native::Error— the top-level type, aggregating all of the above via#[from]and holding the cross-cutting variants (log init, reconnect, backend selection, status code).Each variant uses
#[from]for upstream errors so?keeps working, with descriptive#[source]variants preserving the previous.context()messages.anyhowmoves to[dev-dependencies](still used by examples and tests).Because each backend now owns its proto-level error types, the quinn/noq
ConnectionError#[from]collision (which needed acfgworkaround when both features were enabled) is gone.Downstream:
libmoqgains a#[from] moq_native::Error(Arc-wrapped to keepError: Clone), so the.init()calls use?instead of stringifying through anyhow.Ok(reconnect.closed().await?)at the reconnect boundary.Compatibility
Technically breaking: public functions return
moq_native's error types instead ofanyhow::Error.?propagation and theanyhow::Contexttrait (.context()on aResult) still compile for callers; only direct/typed returns into ananyhow::Errorslot, oranyhow::Error-inherent methods (.downcast(),.chain(), …), need changes.cargo-semver-checkswill detect the return-type change and bump the minor version (0.16.3 → 0.17.0).Cross-package sync
No paired packages apply: this is an internal error-handling refactor of one crate with no wire, public-API-shape, or catalog change. The crate still exposes
std::error::Errortypes thatanyhow-based consumers absorb via?.Test plan
just rs check --workspace(default features): clippy-D warnings, fmt, doc (-D warnings), cargo shear (no unused deps), cargo sort — cleancargo clippy --workspace --all-features --all-targets -- -D warnings(quinn + noq + quiche + iroh + jemalloc + websocket together) — cleancargo check --workspace --no-default-features— buildscargo test -p moq-native— 53 passed, 0 failedlibmoq/moq-relay/moq-cli/moq-boy/ examples build🤖 Generated with Claude Code
(Written by Claude)