moq-token-cli: treat - as stdin/stdout, not a literal file path#1490
Conversation
Previously, `moq-token-cli generate --out -` silently created a file literally named `-`, which made `generate --out - | wrangler secret put` pipe 0 bytes to wrangler (and then wrangler happily stored an empty secret). - `--out` / `--public` write to stdout when set to `-` - `--key` for `sign` reads from stdin when set to `-` - error if both keys would land on stdout (`--out -` + `--public -`, or `--public -` with `--out` unset) - error on `verify --key -` since stdin is reserved for the token Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (1)
WalkthroughThe CLI now treats 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
rs/moq-token-cli/src/main.rs (3)
173-177: ⚡ Quick winAdd error context for clearer diagnostics.
While the stdin guard (lines 173-175) is correct, the subsequent operations on lines 176-177 would benefit from error context to make failures more actionable.
As per coding guidelines, prefer
anyhow::Contextfor error conversion.📝 Suggested improvement
if is_dash(&key) { anyhow::bail!("--key cannot read from stdin; stdin is reserved for the token"); } - let key = moq_token::Key::from_file(key)?; - let token = io::read_to_string(io::stdin())?.trim().to_string(); + let key = moq_token::Key::from_file(key).context("failed to read verification key")?; + let token = io::read_to_string(io::stdin()).context("failed to read token from stdin")?.trim().to_string(); let payload = key.decode(&token)?;🤖 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-cli/src/main.rs` around lines 173 - 177, The stdin guard is fine but failures when reading the key file and reading stdin lack context; update the calls that can fail—moq_token::Key::from_file(key) and io::read_to_string(io::stdin())—to use anyhow::Context (e.g., .with_context(...)) so errors include actionable messages (mention which key path failed and that reading stdin for the token failed), and keep the is_dash(&key) check as-is.
87-94: ⚡ Quick winAdd error context for clearer diagnostics.
Adding
.context()to the error paths would make failures more actionable. For example, line 89 could indicate serialization failure, and line 92 could show which file path failed.As per coding guidelines, prefer
anyhow::Contextfor error conversion.📝 Suggested improvement
fn write_key(key: &moq_token::Key, path: &std::path::Path) -> anyhow::Result<()> { if is_dash(path) { - println!("{}", key.to_str()?); + println!("{}", key.to_str().context("failed to serialize key")?); Ok(()) } else { - Ok(key.to_file(path)?) + key.to_file(path).context("failed to write key to file") } }🤖 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-cli/src/main.rs` around lines 87 - 94, The write_key function should attach context to errors using anyhow::Context: wrap key.to_str()? with .context("failed to serialize key to string") (used when is_dash(path) is true) and wrap key.to_file(path)? with .with_context(|| format!("failed to write key to file: {}", path.display())) so failures from key.to_str and key.to_file (and the provided path) produce actionable diagnostics; keep existing is_dash(path) logic and return types unchanged.
96-103: ⚡ Quick winAdd error context for clearer diagnostics.
Each error path should include context about what operation failed. This helps users diagnose issues when reading keys from stdin or files.
As per coding guidelines, prefer
anyhow::Contextfor error conversion.📝 Suggested improvement
fn read_key(path: &std::path::Path) -> anyhow::Result<moq_token::Key> { if is_dash(path) { - let contents = io::read_to_string(io::stdin())?; - Ok(moq_token::Key::from_str(contents.trim())?) + let contents = io::read_to_string(io::stdin()).context("failed to read key from stdin")?; + moq_token::Key::from_str(contents.trim()).context("failed to parse key from stdin") } else { - Ok(moq_token::Key::from_file(path)?) + moq_token::Key::from_file(path).context("failed to read key from file") } }🤖 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-cli/src/main.rs` around lines 96 - 103, The read_key function needs richer error context: wrap the io::read_to_string(io::stdin()) call with anyhow::Context to indicate failure reading from stdin (include that it was stdin), and wrap the moq_token::Key::from_str(contents.trim()) conversion with Context to indicate parsing the key from stdin failed; likewise, for the file branch wrap moq_token::Key::from_file(path) with Context to indicate failure reading/parsing the key file and include the path in the message. Update read_key (which uses is_dash, io::read_to_string, moq_token::Key::from_str, and moq_token::Key::from_file) to use anyhow::Context on each fallible call so errors surface clear diagnostic text.
🤖 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.
Nitpick comments:
In `@rs/moq-token-cli/src/main.rs`:
- Around line 173-177: The stdin guard is fine but failures when reading the key
file and reading stdin lack context; update the calls that can
fail—moq_token::Key::from_file(key) and io::read_to_string(io::stdin())—to use
anyhow::Context (e.g., .with_context(...)) so errors include actionable messages
(mention which key path failed and that reading stdin for the token failed), and
keep the is_dash(&key) check as-is.
- Around line 87-94: The write_key function should attach context to errors
using anyhow::Context: wrap key.to_str()? with .context("failed to serialize key
to string") (used when is_dash(path) is true) and wrap key.to_file(path)? with
.with_context(|| format!("failed to write key to file: {}", path.display())) so
failures from key.to_str and key.to_file (and the provided path) produce
actionable diagnostics; keep existing is_dash(path) logic and return types
unchanged.
- Around line 96-103: The read_key function needs richer error context: wrap the
io::read_to_string(io::stdin()) call with anyhow::Context to indicate failure
reading from stdin (include that it was stdin), and wrap the
moq_token::Key::from_str(contents.trim()) conversion with Context to indicate
parsing the key from stdin failed; likewise, for the file branch wrap
moq_token::Key::from_file(path) with Context to indicate failure reading/parsing
the key file and include the path in the message. Update read_key (which uses
is_dash, io::read_to_string, moq_token::Key::from_str, and
moq_token::Key::from_file) to use anyhow::Context on each fallible call so
errors surface clear diagnostic text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60e03129-e130-4090-b45e-9703333bdee6
📒 Files selected for processing (1)
rs/moq-token-cli/src/main.rs
CodeRabbit nitpick: bare IO/JSON errors from from_file/to_file leave the user guessing which file failed. Wrap with anyhow context that includes the path, and route verify through read_key so it inherits the same. Context on to_str() (infallible-in-practice) and on stdin reads (self-describing) intentionally skipped — would be noise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed in 13bb504:
Skipped intentionally:
(Written by Claude) |
Previously `verify` always pulled the token from stdin, which forced the key onto a file (`--key -` was rejected). With `--in <PATH>` (default `-`), the token source is explicit and `--key -` works as long as the token isn't also on stdin. Existing `verify --key <file> < token` invocations still work because `--in` defaults to `-`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (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-token-cli/src/main.rs`:
- Around line 77-83: Update the doc comment for the key argument to clarify that
`--key -` (stdin) is only supported when the token comes from a file (i.e.,
`--in` is not `-`). Edit the doc string above the key field (the `key: PathBuf`
arg) to something like: "Path to the key file. Use `-` for stdin, but only when
`--in` is not `-` (token must come from a file)." This makes the behavior
consistent with the guard around token handling in the verify flow that checks
the token source.
🪄 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: ef9b64f0-328a-4939-a36f-135b2cb1a8b9
📒 Files selected for processing (1)
rs/moq-token-cli/src/main.rs
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
moq-token-cli generate --out -silently created a file literally named-instead of writing to stdout, which made the obvious pipeline footgun-prone:This PR makes
-mean stdin/stdout consistently and guards the cases where that would collide with another stream.Changes
generate:--out -and--public -now write to stdout (existing default-no---outstdout behavior is unchanged).sign:--key -now reads the JWK from stdin, sogenerate --out - | sign --key -works.generateerrors if both keys would land on stdout (--out -with--public -, or--public -with--outunset and no--out-dir).verify --key -errors with a clear message since stdin is already reserved for the token.--out-dir/--public-dirflags are unaffected (a directory of stdout doesn't make sense).Test plan
cargo build -p moq-token-clicleancargo clippy -p moq-token-cli --all-targets -- -D warningscleangenerate --out -prints to stdoutgenerate --public - --out file.jwkprints the public key, writes the private to filegenerate --public -(no--out) errors instead of jamming both keys to stdoutgenerate --out - | sign --key - --root test --subscribe ""produces a valid tokenverify --key -errors with the reserved-stdin messagegenerate --out f.jwk→sign --key f.jwk→verify --key f.jwk) unchanged🤖 Generated with Claude Code