moq-cli: unified endpoint grammar (binary renamed to moq)#1985
moq-cli: unified endpoint grammar (binary renamed to moq)#1985kixelated wants to merge 10 commits into
moq)#1985Conversation
Replace the per-purpose subcommands (publish/subscribe/serve/accept/hls) with a unified endpoint model: `moq <import|export> <MoQ side> <endpoint>`. Everything wires onto one shared Origin. - MoQ side: --client-connect (dial a relay) and/or --server-bind (host). - Endpoints (one per invocation): stdin/stdout, hls, rtmp, srt, rtc. - Bidirectional gateways take --connect (dial) or --listen (bind); the parent verb fixes push vs pull. Listeners are directional (an import listener rejects plays; an export listener rejects publishes). - rtmp/srt --connect are stubbed pending the dial-out library (#1982). Ripple: doc/bin/cli.md rewrite, demo justfile, smoke test, scattered docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Replace the rtmp/srt `--connect` stubs with the real dial-out from #1982: moq_rtmp::Client (connect/publish/pull) and moq_srt::dial (publish/pull), with URL parsing for rtmp://host[:1935]/<app>/<key> and srt://host:port. - Rename the shipped binary from `moq-cli` to `moq` via a [[bin]] override. The package stays `moq-cli`. Update the release plumbing accordingly: the CLI workflow, nix mainProgram, Docker entrypoint, Homebrew formula, the package-binary/package-windows scripts (new --bin flag), and smoke. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MoQ names each broadcast by the connection path plus any explicit `--broadcast`, so the name is optional: an unset `--broadcast` uses the root broadcast at the connection path instead of erroring. Point endpoints (stdin/stdout, HLS, `--connect`) no longer require a name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
WalkthroughThis PR renames the shipped CLI binary to ChangesRelated Issues: None provided Sequence Diagram(s)sequenceDiagram
participant CLI as moq
participant Origin
participant Relay
participant Endpoint
CLI->>Origin: import/export routing
Origin->>Relay: client/server relay bridging
Endpoint->>Origin: ingest or serve broadcasts
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
Dockerfile (1)
31-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded binary-name mapping duplicates the
[[bin]]override knowledge.This special-cases
moq-cli→moqin the Dockerfile, separate from the[[bin]]target already defined inrs/moq-cli/Cargo.toml. If another crate's binary is ever renamed, this conditional must be remembered and updated here too, or the entrypoint silently breaks. Since/output/result/binalready contains the single built binary, deriving the name from that directory avoids the duplicated/hardcoded knowledge.♻️ Proposed fix: derive binary name from build output
-# Create entry.sh script that knows which binary to run. The binary usually -# matches the package name; `moq-cli` is the exception, shipping as `moq`. -RUN binary="${package}"; \ - if [ "${package}" = "moq-cli" ]; then binary="moq"; fi; \ - printf '#!/bin/sh\nexec /bin/%s "$@"\n' "${binary}" > /output/entry.sh && \ +# Create entry.sh script that knows which binary to run, derived from the +# single binary produced under result/bin (handles crates whose [[bin]] name +# differs from the package name, e.g. `moq-cli` shipping as `moq`). +RUN binary="$(ls /output/result/bin)"; \ + printf '#!/bin/sh\nexec /bin/%s "$@"\n' "${binary}" > /output/entry.sh && \ chmod +x /output/entry.sh🤖 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 `@Dockerfile` around lines 31 - 36, The entrypoint generation in the Dockerfile is duplicating binary-name knowledge by special-casing package names like moq-cli to moq. Update the entry.sh creation logic so it derives the executable name from the built output in /output/result/bin instead of hardcoding a package-to-binary mapping. Use the existing Dockerfile RUN block that writes /output/entry.sh, and make it pick the single produced binary dynamically so future [[bin]] renames in Cargo.toml do not require manual Dockerfile edits.rs/moq-cli/src/main.rs (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required
//!block to this entrypoint.This file is
rs/**/main.rs, but it now starts directly with module declarations. As per coding guidelines,rs/**/main.rs: "Add a module-level doc block to every entrypoint file: use a//!block at the top of each Rust module root."🤖 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-cli/src/main.rs` around lines 1 - 9, The entrypoint main module is missing the required module-level documentation block, so add a top-of-file `//!` doc comment to this `main.rs` before the existing `mod` declarations. Keep the change focused on the entrypoint module root and ensure the `//!` block is the first item in the file.Source: Coding guidelines
🤖 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/bin/cli.md`:
- Around line 8-10: The page still uses the old executable name `moq-cli` in
runnable examples and prose, but the shipped binary is `moq`. Update the command
examples and any surrounding usage text in `doc/bin/cli.md` to consistently
reference `moq`, especially in sections that show invocations or copy-pastable
commands, so the documented commands match the actual binary name.
In `@doc/bin/index.md`:
- Line 29: The example command still uses the deprecated moq-cli executable, so
update the documentation snippet to use moq instead. Replace the old binary name
in the import example so it matches the installed command and continues to work
after install; the relevant symbol to locate is the moq-cli invocation in the
example command under the import usage section.
In `@doc/concept/layer/hang.md`:
- Line 55: The example command still references the old moq-cli executable, so
update the snippet to use the shipped moq binary instead. Adjust the explicit
hangz opt-in example in the surrounding documentation text so the CLI command
matches the renamed executable, while keeping the Rust CatalogFormat::HangZ and
`@moq/watch` references unchanged.
In `@doc/concept/standard/interop.md`:
- Around line 30-36: The command examples are stale because they still reference
the old executable name; update the two snippets in the interop docs to use the
shipped binary name `moq` instead of `moq-cli`. Make the change in both the
import and subscribe examples so the copy-paste commands match the current CLI
entrypoint.
In `@doc/lib/rs/crate/hang.md`:
- Around line 137-141: Both pipeline examples still reference the old moq-cli
binary, so update the command text in the hang.md examples to use the renamed
moq executable instead. Locate the two import pipeline snippets under the
publish examples and replace moq-cli with moq consistently in both commands so
the docs match the installed binary name.
In `@doc/lib/rs/crate/moq-mux.md`:
- Around line 90-94: The CLI examples are still using the old executable name,
so update the documented import commands to use the renamed `moq` binary instead
of `moq-cli` in both example blocks. Make the change in the markdown snippet
that shows the stdin ts import flow and the FFmpeg pipe example so copied
commands match the shipped `moq` command.
In `@doc/lib/rs/index.md`:
- Around line 127-131: The example commands still use the old moq-cli executable
even though the shipped binary is now moq. Update both import/pipeline examples
in the documentation snippet to reference moq instead of moq-cli so the commands
match the current binary name.
In `@rs/moq-cli/README.md`:
- Around line 22-55: The README examples still invoke the old `moq-cli`
executable even though the shipped binary is `moq`, so update the command
snippets under the publish/subscribe and self-host sections to use `moq`
consistently. Keep the install example as-is if needed, and make sure the
runnable examples match the renamed CLI entrypoint so copy/paste works.
In `@rs/moq-cli/src/args.rs`:
- Around line 76-79: The `Args::dir` flag is currently accepted even when
`--server-bind` is not set, but `run_import` and `run_export` only use it from
the server path, so add validation in the CLI argument handling to reject
`--dir` unless `server_bind` is present. Update the parsing/validation logic
around `Args` so client-only invocations fail with a clear error, and keep the
existing `run_import`/`run_export` behavior unchanged except for relying on the
validated `moq.server.bind` branch.
- Around line 225-227: The SRT `prefix` argument is currently accepted on all
modes, but it is only used by the listen paths, so `srt --connect` can accept a
no-op flag. Update the argument handling in `args.rs` so `prefix` is gated
behind listen mode, using the relevant SRT argument/type definitions (for
example the struct that holds the SRT mode options and any validation or clap
attributes around `prefix`). Ensure connect mode rejects or ignores `--prefix`
appropriately, while listen mode continues to expose and use it.
In `@rs/moq-cli/src/rtc.rs`:
- Around line 79-86: The CORS setup in serve currently allows Any origin,
method, and header on the WHIP/WHEP router, which makes the listener broadly
cross-site callable. Update the CORS configuration in serve to use a safer
default such as no CORS by default or a narrow allowlist, and only permit the
specific origins, methods, and headers required by the WHIP/WHEP flow. Keep the
change localized to the router.layer(CorsLayer::new()) setup in serve so the
listener remains usable without exposing unauthenticated browser access.
In `@rs/moq-cli/src/rtmp.rs`:
- Around line 115-129: Update parse_url in rtmp.rs to validate the RTMP URL
contract before any DNS lookup or dialing: reject any scheme other than rtmp and
require both an app and non-empty key from the path. Keep the existing host
resolution logic, but add early anyhow::ensure checks near parse_url for
url.scheme(), app, and key so inputs like non-rtmp URLs or rtmp://host/app fail
fast with a clear error matching the documented rtmp://host[:1935]/<app>/<key>
format.
In `@rs/moq-cli/src/srt.rs`:
- Around line 113-130: The parse_url helper in srt.rs accepts any Url without
checking its scheme, so non-SRT endpoints can slip through and fail later. Add
an upfront validation in parse_url to require the SRT scheme before resolving
the host or extracting the resource, and return an error for anything else so
callers of parse_url and the downstream dial path reject invalid URLs
immediately.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 31-36: The entrypoint generation in the Dockerfile is duplicating
binary-name knowledge by special-casing package names like moq-cli to moq.
Update the entry.sh creation logic so it derives the executable name from the
built output in /output/result/bin instead of hardcoding a package-to-binary
mapping. Use the existing Dockerfile RUN block that writes /output/entry.sh, and
make it pick the single produced binary dynamically so future [[bin]] renames in
Cargo.toml do not require manual Dockerfile edits.
In `@rs/moq-cli/src/main.rs`:
- Around line 1-9: The entrypoint main module is missing the required
module-level documentation block, so add a top-of-file `//!` doc comment to this
`main.rs` before the existing `mod` declarations. Keep the change focused on the
entrypoint module root and ensure the `//!` block is the first item in the file.
🪄 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: dec2eeaf-6191-4d8a-97b4-9a4bf7255f9d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.github/homebrew/Formula/moq-cli.rb.tmpl.github/workflows/moq-cli.ymlCargo.tomlDockerfiledemo/pub/justfiledoc/bin/cli.mddoc/bin/index.mddoc/bin/relay/auth.mddoc/concept/layer/hang.mddoc/concept/standard/interop.mddoc/lib/rs/crate/hang.mddoc/lib/rs/crate/moq-mux.mddoc/lib/rs/index.mdnix/overlay.nixrs/moq-cli/Cargo.tomlrs/moq-cli/README.mdrs/moq-cli/src/args.rsrs/moq-cli/src/client.rsrs/moq-cli/src/hls.rsrs/moq-cli/src/main.rsrs/moq-cli/src/moq.rsrs/moq-cli/src/publish.rsrs/moq-cli/src/rtc.rsrs/moq-cli/src/rtmp.rsrs/moq-cli/src/server.rsrs/moq-cli/src/srt.rsrs/moq-cli/src/web.rsrs/moq-rtmp/README.mdrs/moq-rtmp/src/lib.rsrs/moq-srt/README.mdrs/moq-srt/src/lib.rsrs/moq-srt/src/ts.rsrs/scripts/package-binary.shrs/scripts/package-windows.shtest/smoke/smoke.sh
💤 Files with no reviewable changes (2)
- rs/moq-cli/src/server.rs
- rs/moq-cli/src/client.rs
- Move the MoQ side (`--client-connect` / `--server-bind` / `--broadcast`) before the `import`/`export` verb, at the top level. - Make the container format the endpoint subcommand (`import ts`, `export fmp4`) instead of a `stdin`/`stdout` subcommand carrying a format. - Move `--max-latency` / `--fragment-duration` / `--catalog` to the `export` level (shared by all stdout formats). - Drop the redundant `MoqSide` accessor methods (fields are public). - Move the per-protocol arg structs into their modules: `rtmp::Args`, `srt::Args`, `rtc::Args`, `hls::Args`. - Docs: put the broadcast in the tcp:// URL path in the auth example, and reflow every invocation to the new shape. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-cli/src/args.rs (1)
64-70: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftScope endpoint-specific flags so they cannot become no-ops.
--broadcastis global even for HLS export/listen endpoints that do not consumename, and the export tuning flags are only used whenstdout_format()is selected. Move these flags into the endpoint variants that use them, or add post-parse validation that rejects incompatible combinations. This is required by the PR objective that no endpoint flag is silently ignored.Also applies to: 147-159
🤖 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-cli/src/args.rs` around lines 64 - 70, The endpoint-specific CLI flags are currently defined globally, so options like broadcast and the export tuning flags can be accepted even when the selected endpoint does not use them. Update the argument handling in args.rs by either moving these fields into the endpoint variants that actually consume them or adding post-parse validation in the relevant parsing/dispatch logic to reject incompatible combinations. Focus on the broadcast field and the stdout_format()-gated export flags so no endpoint flag can be silently ignored.
🤖 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-cli/src/args.rs`:
- Around line 41-42: The public command fields in the args types are missing
rustdoc, so add doc comments for each exported field named direction, source,
and sink in the relevant structs in args.rs. Update the struct definitions that
own these fields (including the command/args types referenced by Direction) so
every public member has a clear documentation comment matching the project’s
public API guideline.
---
Outside diff comments:
In `@rs/moq-cli/src/args.rs`:
- Around line 64-70: The endpoint-specific CLI flags are currently defined
globally, so options like broadcast and the export tuning flags can be accepted
even when the selected endpoint does not use them. Update the argument handling
in args.rs by either moving these fields into the endpoint variants that
actually consume them or adding post-parse validation in the relevant
parsing/dispatch logic to reject incompatible combinations. Focus on the
broadcast field and the stdout_format()-gated export flags so no endpoint flag
can be silently ignored.
🪄 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: 23fb71d0-c373-4ef8-80eb-ecc21180f58e
📒 Files selected for processing (18)
demo/pub/justfiledoc/bin/cli.mddoc/bin/index.mddoc/bin/relay/auth.mddoc/concept/standard/interop.mddoc/lib/rs/crate/hang.mddoc/lib/rs/crate/moq-mux.mddoc/lib/rs/index.mdrs/moq-cli/README.mdrs/moq-cli/src/args.rsrs/moq-cli/src/hls.rsrs/moq-cli/src/main.rsrs/moq-cli/src/publish.rsrs/moq-cli/src/rtc.rsrs/moq-cli/src/rtmp.rsrs/moq-cli/src/srt.rsrs/moq-cli/src/subscribe.rstest/smoke/smoke.sh
✅ Files skipped from review due to trivial changes (6)
- doc/bin/index.md
- doc/lib/rs/crate/moq-mux.md
- doc/bin/relay/auth.md
- rs/moq-cli/README.md
- doc/lib/rs/index.md
- doc/bin/cli.md
🚧 Files skipped from review as they are similar to previous changes (7)
- doc/lib/rs/crate/hang.md
- rs/moq-cli/src/rtmp.rs
- doc/concept/standard/interop.md
- test/smoke/smoke.sh
- rs/moq-cli/src/rtc.rs
- demo/pub/justfile
- rs/moq-cli/src/main.rs
- Docs: runnable examples now invoke `moq` (the shipped binary), not the `moq-cli` package name. Package/install/image/title references keep `moq-cli`. - Reject `--dir` unless `--server-bind` is set (was silently ignored). - Reject `--prefix` together with a gateway `--connect` (listen-only flag); use conflicts_with since the requires target sits in a mutually-exclusive group. Applies to rtmp and srt. - Validate the URL scheme up front in rtmp/srt `parse_url`, and require both app and stream key for rtmp. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Document the exported direction / source / sink / log fields per the project's "document every exported symbol" rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
moq)moq)
--fragment-duration only applies to the fragmented stdout containers (fmp4/mkv), so it moves off the shared `export` level onto those two subcommands. --max-latency stays at `export` (shared by all stdout formats); its doc is corrected to note the gateways have their own latency controls. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Clarifies that it selects the catalog *format* to read for track discovery (hang/hangz/msf), not a catalog track name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Add the required //! entrypoint doc block to main.rs. - Dockerfile: derive the exec'd binary from the single file in result/bin instead of hardcoding the moq-cli -> moq mapping, so future [[bin]] renames need no Dockerfile edit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Redesigns the entire
moq-clisurface into a unified endpoint model, closing #1983.moq-clibecomes a media router that wires one endpoint onto a sharedOrigin, replacing the per-purpose subcommands (publish/subscribe/serve/accept/hls) and folding in the RTMP/SRT/WebRTC gateways under one grammar. Builds on #1975 (library-only gateway crates) and #1982 (RTMP/SRT dial-out).The grammar
--client-connect <url>dials a relay. The URL path is the relay auth path (e.g./anon);?jwt=supplies a token;--broadcastnames the broadcast (optional).--server-bind <addr>hosts MoQ sessions (--tls-*).importroutes media INTO MoQ (a source fills the Origin);exportroutes it OUT (a sink drains it). The verb fixes direction.avc3/fmp4/ts/flvfrom stdin on import;fmp4/mkv/ts/flvto stdout on export), or a gateway (hls,rtmp,srt,rtc). For the bidirectional gateways,--connectdials and--listenbinds; the parent verb decides push vs pull. Exactly one endpoint per invocation.Command mapping (old → new)
publish --url R --broadcast N tsmoq --client-connect R --broadcast N import tssubscribe --url R --broadcast N --format tsmoq --client-connect R --broadcast N export tsserve --broadcast N tsmoq --server-bind [::]:4443 --tls-generate localhost --broadcast N import tshls --url R import --playlist Pmoq --client-connect R --broadcast N import hls Phls --url R export --listen Lmoq --client-connect R export hls --listen Lmoq --client-connect R import rtmp --listen [::]:1935moq --client-connect R --broadcast N export rtmp --connect rtmp://…Key points for reviewers
--listenunderimportaccepts pushes only (rejects plays); underexportit serves plays only (rejects publishes). Enforced in the CLI's accept loops."". MoQ names each broadcast by the connection path plus any explicit--broadcast, so an unset name is the root broadcast at the connection path. The connection URL path is the auth path (AuthParams::from_path), passed through unchanged.moq_rtmp::Client/moq_srt::dialfrom moq-rtmp/moq-srt: add client (dial-out) role #1982.rtmp::Args,srt::Args,rtc::Args,hls::Args); the format is the endpoint subcommand itself (nostdin/stdoutwrapper); the subscribe knobs (--max-latency/--fragment-duration/--catalog) sit onexport.--connectXOR--listen;--dirrequires--server-bind; gateway--prefixconflicts with--connect; rtmp/srt--connectURLs are scheme-checked.Binary renamed:
moq-cli→moqThe package stays
moq-cli, but the shipped binary is nowmoq(a[[bin]]override). Chased through every place that assumedbin name == crate name:moq-cli.yml,nix/overlay.nix(mainProgram),Dockerfileentrypoint, the Homebrew formula template, andpackage-binary.sh/package-windows.sh(new--binflag). The.deb/.rpm(nfpm) already produced/usr/bin/moq.Maintainer note: the external winget manifest's
RelativeFilePath(inmicrosoft/winget-pkgs) currently points atmoq-cli.exe; the firstwingetcreateupdate after this ships needsmoq.exe(the release zip now containsmoq.exe).The ripple
Breaking change to every documented
moq-cliinvocation. Updateddoc/bin/cli.md(full rewrite + gateway sections),demo/pub/justfile,test/smoke/smoke.sh,rs/moq-cli/README.md, and the scattered examples acrossdoc/. Runnable examples now invokemoq; package/install/image references keepmoq-cli.Test plan
cargo build -p moq-cli→target/debug/moq; clippy +cargo fmt/taplovia nix--helprenders per-endpoint; arg groups enforce one MoQ side + one endpoint +--connectXOR--listen--broadcastworksCloses #1983.
🤖 Generated with Claude Code
(Written by Claude Opus 4.8)