[codex] Route HLS CLI import through moq-hls#1939
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
WalkthroughThe PR adds 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-cli/src/publish.rs`:
- Around line 9-13: The top-level help text for the `publish` and `serve`
commands is still describing stdin-only input, which conflicts with the new
`Hls`/`playlist` path in `publish.rs`. Update the command descriptions in
`main.rs` where the CLI help is defined so they mention both stdin and the HLS
playlist input path, and make sure the `publish`/`serve` wording matches the new
`Hls { playlist }` option exposed by `moq-cli`.
🪄 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: d52af98a-9ffa-42fd-8e6e-ec21463a0ba5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.taplo.tomlCargo.tomldemo/pub/justfiledoc/bin/cli.mddoc/index.mddoc/lib/index.mddoc/lib/rs/crate/hang.mddoc/lib/rs/crate/index.mddoc/lib/rs/crate/moq-mux.mddoc/lib/rs/index.mdjs/CLAUDE.mdjs/watch/README.mdrs/CLAUDE.mdrs/hang/README.mdrs/moq-cli/Cargo.tomlrs/moq-cli/src/publish.rsrs/moq-mux/README.mdrs/moq-mux/src/container/hls/import.rs
💤 Files with no reviewable changes (1)
- rs/moq-mux/src/container/hls/import.rs
| /// HLS playlist import. | ||
| Hls { | ||
| /// Remote HLS playlist URL (http/https) or local file path. | ||
| #[arg(long)] | ||
| playlist: String, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the top-level publish/serve help text.
This adds a non-stdin input path, but rs/moq-cli/src/main.rs:33-84 still describes both commands as reading from stdin. That makes --help misleading for hls --playlist ....
🤖 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/publish.rs` around lines 9 - 13, The top-level help text for
the `publish` and `serve` commands is still describing stdin-only input, which
conflicts with the new `Hls`/`playlist` path in `publish.rs`. Update the command
descriptions in `main.rs` where the CLI help is defined so they mention both
stdin and the HLS playlist input path, and make sure the `publish`/`serve`
wording matches the new `Hls { playlist }` option exposed by `moq-cli`.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doc/bin/cli.md (1)
142-145: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNote that
hlsdoes not read stdin.This section says publish formats read from stdin unless noted, but
publish hls --playlist ...never consumes stdin. Add the same kind of exception note you already use forcaptureso the list matches the actual behavior inrs/moq-cli/src/publish.rs:125-148.Suggested change
-- `hls --playlist <url-or-path>` - HLS playlist ingest +- `hls --playlist <url-or-path>` - HLS playlist ingest (does not read stdin)🤖 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 `@doc/bin/cli.md` around lines 142 - 145, Update the publish formats list so it explicitly notes that hls does not read stdin, matching the actual behavior in publish.rs. Add the same style of exception note used for capture to the hls entry in cli.md, and keep the wording consistent with the existing format descriptions so readers can see which publish modes consume stdin and which do not.
🧹 Nitpick comments (1)
rs/moq-cli/src/main.rs (1)
131-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
HlsCommandprivate.This enum is only consumed inside
rs/moq-cli/src/main.rs, sopubneedlessly expands the binary crate's API surface. Making it private also avoids exposing a newly undocumented public symbol. As per coding guidelines, "Document every exported public API symbol" and "Before exposing a new public type ... prefer smaller, more future-proof API shapes."Suggested change
- pub enum HlsCommand { + enum HlsCommand {🤖 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 131 - 132, The HlsCommand enum is unnecessarily public and should be kept private since it is only used within main.rs. Remove the pub visibility from HlsCommand so the binary crate does not expose an undocumented API surface, and keep the change localized to the HlsCommand declaration and its internal uses in main.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 `@rs/moq-cli/src/main.rs`:
- Around line 136-137: The HLS listener binding currently relies on the raw
SocketAddr default, which can behave IPv6-only on Windows when set to [::]:8089.
Update the listener setup in main.rs to use the dual-stack helper from
moq_native::bind::tcp instead of direct TcpListener::bind for the HLS endpoint,
so the existing listen option in the CLI continues to work for both IPv4 and
IPv6.
In `@rs/moq-hls/Cargo.toml`:
- Around line 22-24: The dependency declaration for anyhow in this workspace
crate is hard-coded instead of using the shared workspace pin. Update the
Cargo.toml entry for anyhow under the [dependencies] section to reference the
workspace dependency source of truth with { workspace = true } rather than
specifying its own version/features, and keep the change aligned with the
crate’s existing dependency usage.
---
Outside diff comments:
In `@doc/bin/cli.md`:
- Around line 142-145: Update the publish formats list so it explicitly notes
that hls does not read stdin, matching the actual behavior in publish.rs. Add
the same style of exception note used for capture to the hls entry in cli.md,
and keep the wording consistent with the existing format descriptions so readers
can see which publish modes consume stdin and which do not.
---
Nitpick comments:
In `@rs/moq-cli/src/main.rs`:
- Around line 131-132: The HlsCommand enum is unnecessarily public and should be
kept private since it is only used within main.rs. Remove the pub visibility
from HlsCommand so the binary crate does not expose an undocumented API surface,
and keep the change localized to the HlsCommand declaration and its internal
uses in main.
🪄 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: 41191904-0054-46d5-91ca-77678819d77e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
doc/bin/cli.mdrs/moq-cli/Cargo.tomlrs/moq-cli/src/main.rsrs/moq-hls/Cargo.tomlrs/moq-hls/bin/moq-hls.rsrs/moq-rtmp/README.mdrs/moq-rtmp/bin/moq-rtmp.rsrs/moq-rtmp/src/lib.rsrs/moq-srt/README.mdrs/moq-srt/bin/moq-srt.rsrs/moq-srt/src/lib.rs
💤 Files with no reviewable changes (1)
- rs/moq-hls/bin/moq-hls.rs
✅ Files skipped from review due to trivial changes (6)
- rs/moq-srt/src/lib.rs
- rs/moq-cli/Cargo.toml
- rs/moq-srt/bin/moq-srt.rs
- rs/moq-rtmp/src/lib.rs
- rs/moq-rtmp/bin/moq-rtmp.rs
- rs/moq-srt/README.md
| #[arg(long, env = "MOQ_HLS_LISTEN", default_value = "[::]:8089")] | ||
| listen: SocketAddr, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bind the HLS listener with the dual-stack helper.
std::net::TcpListener::bind on [::]:8089 is IPv6-only on Windows, so the default HLS endpoint won’t accept IPv4 there. Use moq_native::bind::tcp here as well.
🤖 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 136 - 137, The HLS listener binding
currently relies on the raw SocketAddr default, which can behave IPv6-only on
Windows when set to [::]:8089. Update the listener setup in main.rs to use the
dual-stack helper from moq_native::bind::tcp instead of direct TcpListener::bind
for the HLS endpoint, so the existing listen option in the CLI continues to work
for both IPv4 and IPv6.
| [dependencies] | ||
| # Always required (import + export library). | ||
| anyhow = { version = "1", features = ["backtrace"] } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the workspace pin for anyhow.
Line 24 hard-codes anyhow in a workspace crate, which bypasses the repo-wide dependency source of truth. This should be consumed via { workspace = true } instead.
Suggested change
-anyhow = { version = "1", features = ["backtrace"] }
+anyhow = { workspace = true, features = ["backtrace"] }As per coding guidelines, "Workspace crates must add shared dependencies and version/path pins under [workspace.dependencies], and new crates should reference them with { workspace = true } instead of declaring their own versions."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [dependencies] | |
| # Always required (import + export library). | |
| anyhow = { version = "1", features = ["backtrace"] } | |
| [dependencies] | |
| # Always required (import + export library). | |
| anyhow = { workspace = true, features = ["backtrace"] } |
🤖 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-hls/Cargo.toml` around lines 22 - 24, The dependency declaration for
anyhow in this workspace crate is hard-coded instead of using the shared
workspace pin. Update the Cargo.toml entry for anyhow under the [dependencies]
section to reference the workspace dependency source of truth with { workspace =
true } rather than specifying its own version/features, and keep the change
aligned with the crate’s existing dependency usage.
Source: Coding guidelines
1 participant
Summary
moq-cli publish hls --playlist ...as a thin wrapper aroundmoq-hls::importmoq-cli hls importandmoq-cli hls exportso the old HLS binary's import/export functionality lives undermoq-climoq-hlsbinary target and trim binary-only optional dependencies from themoq-hlslibrary cratemoq-muxHLS importer source and docs references that made HLS look owned bymoq-muxmoq-cliHLS surfaceWhy
HLS now lives in the
moq-hlslibrary crate, butmoq-clishould be the user-facing binary for common protocol/container workflows. This keeps the HLS implementation reusable while avoiding a separate installed binary for HLS.Validation
direnv exec . just fixdirenv exec . cargo check -p moq-clidirenv exec . cargo check -p moq-hls --no-default-featuresdirenv exec . cargo run -q --bin moq-cli -- publish --helpdirenv exec . cargo run -q --bin moq-cli -- publish hls --helpdirenv exec . cargo run -q --bin moq-cli -- hls --helpdirenv exec . cargo run -q --bin moq-cli -- hls import --helpdirenv exec . cargo run -q --bin moq-cli -- hls export --helpgit diff --check(Written by Claude)