feat: CLI quick wins — help, version, colors, exit codes#68
Merged
Conversation
…mary - --help / -h: structured usage with sections (USAGE, OPTIONS, EXIT CODES) - --version / -V: prints version from Cargo.toml - -c shorthand for --config - --check alias for --validate (matches common CLI conventions) - Colored output: green for success, red for errors, yellow for warnings, dim for secondary info. Respects NO_COLOR env var and non-TTY stderr. - Consistent exit codes: 0=success, 1=config error, 2=runtime error - Config errors caught and printed cleanly (no Rust panic traces) - Pipeline build errors caught at startup with colored context - Startup summary shows pipeline topology: inputs -> SQL -> outputs - Structured code: commands split into cmd_config/cmd_blackhole/cmd_generate_json Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughAdded Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/logfwd/Cargo.toml (1)
10-22: 🛠️ Refactor suggestion | 🟠 MajorAdd the
netfeature to Tokio.
crates/logfwd/src/main.rscallstokio::runtime::Builder::enable_io()(line 418), which requires thenetfeature. Currently, this crate only declaresrt-multi-threadandtime, relying on transitive feature unification to compile.♻️ Proposed fix
-tokio = { version = "1", features = ["rt-multi-thread", "time"] } +tokio = { version = "1", features = ["rt-multi-thread", "time", "net"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/logfwd/Cargo.toml` around lines 10 - 22, The Tokio dependency in Cargo.toml is missing the "net" feature required by tokio::runtime::Builder::enable_io() (used in crates/logfwd/src/main.rs), so update the tokio entry in Cargo.toml to include "net" alongside "rt-multi-thread" and "time" (i.e., add "net" to the features array) to ensure compile-time availability without relying on transitive feature unification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/logfwd/src/main.rs`:
- Around line 153-160: The validate-only path currently returns immediately
after Config::load, skipping pipeline-level validation; instead, iterate over
config.pipelines and call Pipeline::from_config (or the existing pipeline
construction/validation function) for each pipeline to surface SQL/input/output
wiring errors, collect any errors and return a non-zero Result on failure, and
only print the "config ok" message after all Pipeline::from_config validations
succeed; update the validate_only branch in main/run to perform that loop and
error handling.
- Around line 141-143: The current extraction of validate_only and dry_run using
args.iter().any lets unknown tokens silently pass; change to explicitly parse
the tail slice (e.g., let extra = &args[3..]) and iterate matching allowed flags
only, setting validate_only and dry_run when you see "--validate"|"--check" and
"--dry-run" respectively, and return/exit with EXIT_CONFIG (or call the same
error path used elsewhere) if any unknown token is encountered; update
references to args, config_path, validate_only, dry_run to use this parsed
result so typos like "--dry-ru" produce an error instead of a real run.
- Around line 260-274: The sibling threads never see shutdown flipped and their
JoinHandle results are ignored; change the logic around pipelines/main_pipeline
so that after main_pipe.run(&shutdown) returns (on both Ok and Err) you set the
shared shutdown flag to true (e.g., shutdown.store(true, Ordering::SeqCst) or
shutdown.set(true) depending on the shutdown type) and then join all worker
threads, collecting each thread::JoinHandle result and the inner io::Result from
pipeline.run(&sd) and propagate any errors (map panics/join errors and
io::Result::Err into a returned Err) so non-main pipeline failures affect the
process exit code.
- Around line 242-248: DiagnosticsServer currently spawns the thread in
DiagnosticsServer::start() but defers the bind (which may panic) to the
background, so change DiagnosticsServer::start() to perform the bind
synchronously and return Result<JoinHandle<()>, E> (or similar) instead of
unconditionally spawning; then in main.rs replace Some(server.start()) with
matching on server.start(): on Ok(handle) keep Some(handle) and print the
diagnostics URL, on Err(e) log the bind error and abort/startup failure. Update
the DiagnosticsServer::start signature and callers (including the place that
previously expected a JoinHandle) to handle the Result so bind failures are
surfaced before readiness is advertised.
- Around line 200-203: The parse failure for args[2] should trigger the
configuration exit path instead of being wrapped as an io::Error and bubbling to
main (which causes EXIT_RUNTIME); change the `.parse().map_err(|e|
io::Error::other(...))?` handling for num_lines so that on parse error you
return or exit with the configuration error/exit code (use the existing
EXIT_CONFIG constant or the program's Config/Argument error variant) rather than
creating an io::Error — locate the parsing code that sets `num_lines` (the
`.parse()` call for args[2]) and adjust it to either call
`process::exit(EXIT_CONFIG)` on parse failure or return the appropriate
config-error result from main so the program returns the documented
config/argument exit code before calling `generate_json_log_file`.
---
Outside diff comments:
In `@crates/logfwd/Cargo.toml`:
- Around line 10-22: The Tokio dependency in Cargo.toml is missing the "net"
feature required by tokio::runtime::Builder::enable_io() (used in
crates/logfwd/src/main.rs), so update the tokio entry in Cargo.toml to include
"net" alongside "rt-multi-thread" and "time" (i.e., add "net" to the features
array) to ensure compile-time availability without relying on transitive feature
unification.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: bf5372f3-0a75-49a7-8354-088ac312be4b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/logfwd/Cargo.tomlcrates/logfwd/src/main.rs
Comment on lines
+242
to
+248
| let _diag_handle = if let Some(ref addr) = config.server.diagnostics { | ||
| let mut server = DiagnosticsServer::new(addr); | ||
| for p in &pipelines { | ||
| server.add_pipeline(Arc::clone(p.metrics())); | ||
| } | ||
| eprintln!(" {}diagnostics{}: http://{addr}", dim(), reset()); | ||
| Some(server.start()) |
There was a problem hiding this comment.
Surface diagnostics bind failures before advertising readiness.
DiagnosticsServer::start() only spawns the thread; the actual bind still happens later in crates/logfwd-core/src/diagnostics.rs, where startup uses expect(...). If the port is busy, this path logs diagnostics: and keeps running while the background thread panics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd/src/main.rs` around lines 242 - 248, DiagnosticsServer
currently spawns the thread in DiagnosticsServer::start() but defers the bind
(which may panic) to the background, so change DiagnosticsServer::start() to
perform the bind synchronously and return Result<JoinHandle<()>, E> (or similar)
instead of unconditionally spawning; then in main.rs replace
Some(server.start()) with matching on server.start(): on Ok(handle) keep
Some(handle) and print the diagnostics URL, on Err(e) log the bind error and
abort/startup failure. Update the DiagnosticsServer::start signature and callers
(including the place that previously expected a JoinHandle) to handle the Result
so bind failures are surfaced before readiness is advertised.
Comment on lines
+260
to
+274
| let mut handles = Vec::new(); | ||
| let main_pipeline = pipelines.pop(); | ||
|
|
||
| for mut pipeline in pipelines { | ||
| let sd = shutdown.clone(); | ||
| handles.push(std::thread::spawn(move || pipeline.run(&sd))); | ||
| } | ||
|
|
||
| if let Some(mut main_pipe) = main_pipeline { | ||
| main_pipe.run(&shutdown)?; | ||
| } | ||
|
|
||
| for h in handles { | ||
| let _ = h.join(); | ||
| } |
There was a problem hiding this comment.
Propagate shutdown and worker errors across pipelines.
shutdown is never flipped to true, so when the main pipeline returns normally the sibling threads keep running and join() can block forever. The worker JoinHandle<io::Result<()>> results are also discarded, so non-main pipeline failures never affect the process exit code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/logfwd/src/main.rs` around lines 260 - 274, The sibling threads never
see shutdown flipped and their JoinHandle results are ignored; change the logic
around pipelines/main_pipeline so that after main_pipe.run(&shutdown) returns
(on both Ok and Err) you set the shared shutdown flag to true (e.g.,
shutdown.store(true, Ordering::SeqCst) or shutdown.set(true) depending on the
shutdown type) and then join all worker threads, collecting each
thread::JoinHandle result and the inner io::Result from pipeline.run(&sd) and
propagate any errors (map panics/join errors and io::Result::Err into a returned
Err) so non-main pipeline failures affect the process exit code.
New `format: console` option for stdout output. Renders logs as:
10:30:00.000Z INFO request handled GET /api/v1/users/10000 duration_ms=1 service=myapp
10:30:00.001Z DEBUG request handled GET /api/v1/orders/10007 duration_ms=14 ...
10:30:00.002Z WARN request handled GET /api/v2/products/10014 duration_ms=27 ...
10:30:00.003Z ERROR request handled GET /health/10021 duration_ms=40 ...
- Timestamp shown as time-only (strips date prefix)
- Level colored: green=INFO, yellow=WARN, red=ERROR, dim=DEBUG
- Message displayed prominently
- Remaining fields as dim key=value pairs
- _raw field excluded (redundant with parsed fields)
- Respects NO_COLOR and non-TTY stdout
Config:
output:
type: stdout
format: console
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…es, exit codes - Reject unknown flags after --config (typos like --dry-ru now error) - --validate and --check now build pipelines via Pipeline::from_config to catch SQL/wiring errors (not just YAML parsing) - --generate-json parse error exits with EXIT_CONFIG (1) not EXIT_RUNTIME (2) - Removed unused dry_run parameter from run_pipelines Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 29, 2026
This was referenced Mar 30, 2026
This was referenced Apr 6, 2026
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.
Summary
Polish the logfwd CLI with the basics every good CLI tool has.
New flags:
--help/-h— structured usage with USAGE, OPTIONS, EXIT CODES sections--version/-V— prints version from Cargo.toml-cshorthand for--config--checkalias for--validateColored output (ANSI, no dependencies):
config ok,ready,logfwd running)error: config I/O error: ...)warning: meter provider shutdown)NO_COLORenv var and non-TTY stderrConsistent exit codes:
Startup summary — shows pipeline topology on boot:
Better error handling:
--generate-jsonvalidates num_lines parseTest plan
cargo clippy -p logfwd -- -D warningscleancargo fmt --checkcleanlogfwd --helpshows usage, exits 0logfwd --versionshowslogfwd 0.1.0, exits 0logfwd --config bad.yamlprints error, exits 1logfwd --config good.yaml --checkprintsconfig ok, exits 0logfwd --config good.yaml --dry-runshows summary +dry run ok, exits 0logfwd --bogusprints error + help hint, exits 1🤖 Generated with Claude Code