Skip to content

relayburn-cli: clap scaffold + render helpers (#248 part a)#309

Merged
willwashburn merged 5 commits intomainfrom
rust-port/cli-scaffold
May 6, 2026
Merged

relayburn-cli: clap scaffold + render helpers (#248 part a)#309
willwashburn merged 5 commits intomainfrom
rust-port/cli-scaffold

Conversation

@willwashburn
Copy link
Copy Markdown
Member

Summary

  • Replaces the eprintln!/exit-1 stub in crates/relayburn-cli/src/main.rs with a clap v4 derive root, eight subcommand stubs (summary, hotspots, overhead, compare, run, state, ingest, mcp-server), and shared render::{table,json,error} helpers — the structure the eight Wave 2 fan-out PRs plug into.
  • Global flags mirror the TS CLI's: --json, --ledger-path <PATH>, --no-color. Per-command flag wiring is deliberately deferred so the fan-out PRs can land in parallel without touching cli.rs.
  • Each stub today exits 1 with a not yet implemented message; with --json it emits a {"error": …} envelope on stdout. Wave 2 PRs replace each stub with a thin presenter over relayburn-sdk.

Parent issue: #248. Unblocks the eight Wave 2 fan-out PRs (summary, hotspots, overhead, compare, run, state, ingest, mcp-server).

Coordination notes:

Test plan

  • cargo build -p relayburn-cli produces a burn binary.
  • cargo test -p relayburn-cli passes (7 unit + 6 integration).
  • cargo test --workspace still passes (623 tests across 7 binaries).
  • cargo run -p relayburn-cli -- --help lists all eight subcommands and the three global flags.
  • target/release/burn --help cold-start is ~10 ms on M-series silicon (no model loads at startup).
  • burn <subcommand> --help exits 0 with non-empty stdout for every stub.
  • Bare burn <subcommand> exits 1 with the not yet implemented tripwire message; Wave 2 PRs flip this assertion as they wire up real presenters.
  • burn --json summary emits a {"error": "burn summary: not yet implemented"} envelope on stdout (verifies the JSON-mode error path).

🤖 Generated with Claude Code

Replaces the eprintln!/exit-1 stub in `crates/relayburn-cli/src/main.rs`
with a proper clap v4 derive root, eight subcommand stubs, and shared
rendering helpers under `render::{table,json,error}`. Each stub today
exits 1 with a "not yet implemented" message (or a `{"error": …}`
envelope under `--json`); the eight Wave 2 fan-out PRs replace the
stubs with thin presenters over `relayburn-sdk`.

Global flags mirror the TS CLI's: `--json`, `--ledger-path <PATH>`,
`--no-color`. Per-command flag wiring is deliberately deferred to the
fan-out PRs so they can land in parallel without touching `cli.rs`.

Smoke test under `tests/smoke.rs` drives the binary end-to-end via
`assert_cmd`:

  - `burn --help` exits 0 and lists every subcommand
  - `burn <sub> --help` exits 0 with non-empty stdout for every stub
  - bare `burn <sub>` exits 1 with "not yet implemented" on stderr
  - `--json burn <sub>` emits a JSON error envelope on stdout
  - `burn --version` exits 0
  - unknown subcommands exit non-zero

`cargo build -p relayburn-cli` produces a `burn` binary and
`target/release/burn --help` runs in ~10 ms on M-series silicon
(no model loads at startup).

Unblocks the eight Wave 2 fan-out PRs (`summary`, `hotspots`,
`overhead`, `compare`, `run`, `state`, `ingest`, `mcp-server`).
Parent issue: #248. Coordination notes (#248-b owns
`crates/relayburn-cli/src/harnesses/`; #248-c owns the golden-output
snapshots) deliberately untouched.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 6, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cf533010-20c1-4b1b-acb3-24dbfab66e4e

📥 Commits

Reviewing files that changed from the base of the PR and between c0c4d52 and b187b6f.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • CHANGELOG.md

📝 Walkthrough

Walkthrough

Adds a Clap v4-based CLI crate relayburn-cli with global flags and eight stub subcommands, shared render helpers (error/json/table), smoke tests, and Cargo manifest updates; updates CHANGELOG with ports and new public surfaces in relayburn-analyze and relayburn-ingest.

Changes

CLI & Render Scaffold

Layer / File(s) Summary
Dependencies & Configuration
crates/relayburn-cli/Cargo.toml
Adds runtime and dev dependencies (clap, comfy-table, serde, serde_json, anyhow, thiserror, assert_cmd, predicates) and explanatory comments.
Core CLI Structure
crates/relayburn-cli/src/cli.rs, crates/relayburn-cli/src/main.rs
Introduces GlobalArgs, Args, Command enum, Clap parsing via Args::parse(), and dispatch(args) to route to subcommand handlers.
Command Module Wiring
crates/relayburn-cli/src/commands/mod.rs
Re-exports eight subcommand modules and implements shared not_yet_implemented helper that delegates to render error reporting.
Command Stubs
crates/relayburn-cli/src/commands/{summary,hotspots,overhead,compare,run,state,ingest,mcp_server}.rs
Adds per-command stub modules; each exposes pub fn run(globals: &GlobalArgs) -> i32 and calls the shared not-yet-implemented path.
Rendering Utilities
crates/relayburn-cli/src/render/{mod.rs,error.rs,json.rs,table.rs}
Adds rendering subsystems: error mapping with EXIT_* codes and JSON envelope output, JSON helpers (render_json, render_json_compact), and table rendering/printing honoring --no-color, with unit tests.
CLI Tests
crates/relayburn-cli/tests/smoke.rs
Adds smoke tests verifying top-level help, per-subcommand help, stub exit codes/messages (including JSON envelope), --version, and unknown-command behavior.

Changelog / Ports

Layer / File(s) Summary
Changelog Entries
CHANGELOG.md
Adds Unreleased entries documenting the new relayburn-cli scaffold and multiple ports/exports in relayburn-analyze and relayburn-ingest (detectors, aggregators, primitives, ledger integration).

Sequence Diagram(s)

sequenceDiagram
  participant User as "User (CLI)"
  participant Parser as "Args::parse (clap)"
  participant Dispatcher as "dispatch"
  participant Command as "subcommand::run"
  participant Render as "render::{error,json,table}"

  User->>Parser: invoke `burn` with args
  Parser->>Dispatcher: parsed Args
  Dispatcher->>Command: call corresponding run(globals)
  Command->>Render: request output (error/json/table)
  Render-->>User: write human or JSON envelope / table to stdout/stderr
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

  • AgentWorkforce/burn#287: Related to the relayburn-analyze ghost-surface detector and public API ports referenced in the changelog.

Poem

🐰 A stubby CLI hops into sight,
Flags crisp and subcommands polite,
JSON envelopes and tables to show,
Tests that nudge each tiny flow,
Soon the presenters will bound in light.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding clap scaffolding and render helpers to the relayburn-cli, which is the primary accomplishment of this PR.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, detailing the replacement of the stub with clap scaffolding, subcommand stubs, render helpers, global flags, and test results.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rust-port/cli-scaffold

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 735ac72e3e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// single-byte chars, and multi-byte UTF-8 sequences never
// contain 0x1b except in their leading position (which we
// handled above).
out.push(bytes[i] as char);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve UTF-8 while stripping ANSI escapes

When --no-color is enabled, render_table calls strip_ansi, but this function rebuilds the output byte-by-byte with out.push(bytes[i] as char). That corrupts any non-ASCII UTF-8 content into mojibake, including the table borders themselves because this renderer uses UTF8_FULL (box-drawing characters). As soon as Wave 2 commands print tables with --no-color, users will get garbled output instead of clean uncolored text.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3a20c5b — went with option (c): replaced the hand-rolled strip_ansi with comfy_table::Table::force_no_tty(), which suppresses cell styling at the source so there's no post-hoc string surgery to get wrong. Added a regression test (no_color_preserves_non_ascii_cell_contents) that round-trips Japanese (日本語) + an emoji and asserts both the non-ASCII cell content and the UTF8_FULL box-drawing characters survive intact.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
crates/relayburn-cli/tests/smoke.rs (1)

98-112: ⚡ Quick win

Harden JSON-mode contract by asserting stderr is empty.

Line 95–97 says --json should move error reporting to stdout. The test currently checks stdout content but does not enforce that nothing is emitted to stderr.

Proposed test tightening
     let output = burn()
         .args(["--json", "summary"])
         .assert()
         .code(1)
+        .stderr(predicate::str::is_empty())
         .get_output()
         .clone();
🤖 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 `@crates/relayburn-cli/tests/smoke.rs` around lines 98 - 112, The test checks
JSON-mode output (using burn().args(["--json", "summary"]) and the captured
`output`/`stdout`) but doesn't assert that nothing was written to `stderr`;
update the test to assert `output.stderr` is empty (e.g., ensure `output.stderr`
is zero-length or that converting it to a UTF-8 string yields an empty string)
after the existing stdout assertions so the JSON-mode contract (errors only on
stdout) is enforced.
🤖 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 `@CHANGELOG.md`:
- Line 7: Remove the CLI-only entry from the root CHANGELOG.md and add it to the
relayburn-cli package changelog instead: delete the bullet describing
`relayburn-cli` (the clap v4 root flags `--json`/`--ledger-path`/`--no-color`,
the eight stub subcommands `summary, hotspots, overhead, compare, run, state,
ingest, mcp-server`, and the `render::{table,json,error}` helpers) from the root
`[Unreleased]` section, and append or create an equivalent entry under the
relayburn-cli package CHANGELOG.md so the single-package work is only recorded
in that package’s changelog.

In `@crates/relayburn-cli/src/commands/mod.rs`:
- Around line 1-3: Update the module doc string in the commands module to avoid
claiming all stubs print to stderr: change the sentence that currently reads
"prints `not yet implemented` to stderr" to clarify that stubs print that
message to stderr by default but when the CLI is run with --json errors are
emitted as a JSON envelope to stdout; edit the top-level doc comment in the
module (the module-level comment in crates/relayburn-cli/src/commands/mod.rs) to
reflect this JSON-mode output path so Wave 2 implementers are not misled.

In `@crates/relayburn-cli/src/render/json.rs`:
- Around line 53-56: The test render_json_accepts_arbitrary_serialize_input
currently discards the io::Result returned by render_json and
render_json_compact so failures are ignored; change the test to assert success
by unwrapping or using expect/is_ok on those results (e.g., call
render_json(...).unwrap() or assert!(render_json(...).is_ok()) and likewise for
render_json_compact) to ensure the test fails on Err and include a short message
in expect if desired.

In `@crates/relayburn-cli/src/render/table.rs`:
- Around line 70-103: strip_ansi currently casts individual bytes to char which
mangles multi-byte UTF-8; change the logic to track chunk_start (byte index of
the next non-escape segment) and whenever you detect an ESC sequence, push the
valid UTF-8 substring input[chunk_start..i] into out (rather than pushing bytes
one-by-one), advance i past the entire escape and set chunk_start = i, and after
the loop push the final substring input[chunk_start..bytes.len()] so all
multi-byte characters remain valid UTF-8; update the function strip_ansi to use
these string slices instead of bytes[i] as char.
- Around line 61-65: Change print_table to return io::Result<()> instead of
using println!: make its signature pub fn print_table(globals: &GlobalArgs,
headers: &[&str], rows: &[Vec<String>]) -> std::io::Result<()>; call
render_table(globals, headers, rows) to get the rendered string and write it to
stdout using std::io::stdout().write_all(rendered.as_bytes()) (or use
write!(stdout_lock, "{}\n", rendered)) and propagate the Result, so broken-pipe
(EPIPE) is returned to the caller rather than panicking.

---

Nitpick comments:
In `@crates/relayburn-cli/tests/smoke.rs`:
- Around line 98-112: The test checks JSON-mode output (using
burn().args(["--json", "summary"]) and the captured `output`/`stdout`) but
doesn't assert that nothing was written to `stderr`; update the test to assert
`output.stderr` is empty (e.g., ensure `output.stderr` is zero-length or that
converting it to a UTF-8 string yields an empty string) after the existing
stdout assertions so the JSON-mode contract (errors only on stdout) is enforced.
🪄 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 Plus

Run ID: 362acd04-06a9-4f66-8fd5-5ae68690f2a7

📥 Commits

Reviewing files that changed from the base of the PR and between 09cc4e0 and 735ac72.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (18)
  • CHANGELOG.md
  • crates/relayburn-cli/Cargo.toml
  • crates/relayburn-cli/src/cli.rs
  • crates/relayburn-cli/src/commands/compare.rs
  • crates/relayburn-cli/src/commands/hotspots.rs
  • crates/relayburn-cli/src/commands/ingest.rs
  • crates/relayburn-cli/src/commands/mcp_server.rs
  • crates/relayburn-cli/src/commands/mod.rs
  • crates/relayburn-cli/src/commands/overhead.rs
  • crates/relayburn-cli/src/commands/run.rs
  • crates/relayburn-cli/src/commands/state.rs
  • crates/relayburn-cli/src/commands/summary.rs
  • crates/relayburn-cli/src/main.rs
  • crates/relayburn-cli/src/render/error.rs
  • crates/relayburn-cli/src/render/json.rs
  • crates/relayburn-cli/src/render/mod.rs
  • crates/relayburn-cli/src/render/table.rs
  • crates/relayburn-cli/tests/smoke.rs

Comment thread CHANGELOG.md

## [Unreleased]

- `relayburn-cli` (Rust): scaffold the clap v4 derive root with global `--json` / `--ledger-path` / `--no-color` flags, eight stub subcommands (`summary`, `hotspots`, `overhead`, `compare`, `run`, `state`, `ingest`, `mcp-server`), and shared `render::{table,json,error}` helpers. Stubs exit `1` with a `not yet implemented` message (or a `{"error": …}` envelope under `--json`); Wave 2 fan-out PRs replace each stub with a thin presenter over `relayburn-sdk`. (#248 part a)
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Move this CLI-only note out of root CHANGELOG.md.

This entry is scoped to relayburn-cli; it should live in the package changelog rather than root [Unreleased].

As per coding guidelines, "single-package work belongs only in that package's CHANGELOG."

🤖 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 `@CHANGELOG.md` at line 7, Remove the CLI-only entry from the root CHANGELOG.md
and add it to the relayburn-cli package changelog instead: delete the bullet
describing `relayburn-cli` (the clap v4 root flags
`--json`/`--ledger-path`/`--no-color`, the eight stub subcommands `summary,
hotspots, overhead, compare, run, state, ingest, mcp-server`, and the
`render::{table,json,error}` helpers) from the root `[Unreleased]` section, and
append or create an equivalent entry under the relayburn-cli package
CHANGELOG.md so the single-package work is only recorded in that package’s
changelog.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushing back on this one — keeping the entry in root CHANGELOG.md.

The root file is currently the cross-package narrative for the Rust port (epic #240) progress. Per-port-milestone entries land there until the 2.0 cutover so the high-level Wave 1 / Wave 2 progression is readable in one place.

Per-package CHANGELOG flow described in CLAUDE.md (packages/*/CHANGELOG.md curated under [Unreleased], promoted at publish time, git-log fallback) only applies to the eight npm packages under packages/. Rust crates under crates/ aren't on that flow yet — there's no crates/relayburn-cli/CHANGELOG.md and no publish workflow that would promote it. Until the cutover, the root file is where Rust-port milestones go.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willwashburn, that's a clear and well-reasoned explanation — thanks for the context. I misapplied the packages/*/CHANGELOG.md convention to the Rust crates, which are on a different flow. I'll stand down on this one and note it for future reviews.


✏️ Learnings added
Learnt from: willwashburn
Repo: AgentWorkforce/burn PR: 309
File: CHANGELOG.md:7-7
Timestamp: 2026-05-06T12:35:11.052Z
Learning: In the AgentWorkforce/burn repository, the root `CHANGELOG.md` is intentionally used as the cross-package narrative for Rust port (epic `#240`) milestones. Per-port-milestone entries for Rust crates under `crates/` land in the root `[Unreleased]` section until the 2.0 cutover. The per-package CHANGELOG flow (curated `[Unreleased]`, promoted at publish time) described in CLAUDE.md applies only to the eight npm packages under `packages/`, not to Rust crates under `crates/`. There is no `crates/relayburn-cli/CHANGELOG.md` or Rust publish workflow yet. Do not flag Rust-crate entries in root CHANGELOG.md as misplaced.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment thread crates/relayburn-cli/src/commands/mod.rs Outdated
Comment thread crates/relayburn-cli/src/render/json.rs
Comment thread crates/relayburn-cli/src/render/table.rs
Comment thread crates/relayburn-cli/src/render/table.rs Outdated
…xes)

The previous `--no-color` path post-processed `comfy-table`'s output with
a hand-rolled `strip_ansi` that walked bytes and pushed each as a `char`,
which silently corrupted any multi-byte UTF-8 — including the table's
own UTF8_FULL box-drawing characters and any non-ASCII cell content —
into mojibake. As soon as a Wave 2 command rendered a table with
`--no-color`, the output would have been broken.

Switch to comfy-table's built-in `Table::force_no_tty()`, which
disables cell styling at the source. This kills the bug entirely
(option c from the review): there's no post-hoc string surgery to get
wrong, and codepoints flow through untouched.

Add a regression test that round-trips Japanese text and an emoji
through `--no-color` rendering and asserts both the cell content and
the box-drawing borders survive intact, plus that no escape bytes leak
through.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
crates/relayburn-cli/src/render/table.rs (1)

63-65: ⚠️ Potential issue | 🟠 Major

Return io::Result<()> from print_table to avoid broken-pipe panic paths.

Line 65 uses println!, which can panic on EPIPE (... | head) instead of letting callers handle the write failure cleanly.

Proposed fix
+use std::io::{self, Write};
+
 /// Convenience: render and write to stdout with a trailing newline.
-pub fn print_table(globals: &GlobalArgs, headers: &[&str], rows: &[Vec<String>]) {
+pub fn print_table(
+    globals: &GlobalArgs,
+    headers: &[&str],
+    rows: &[Vec<String>],
+) -> io::Result<()> {
     let rendered = render_table(globals, headers, rows);
-    println!("{rendered}");
+    let stdout = io::stdout();
+    let mut handle = stdout.lock();
+    handle.write_all(rendered.as_bytes())?;
+    handle.write_all(b"\n")
 }
#!/bin/bash
set -euo pipefail

# Verify current implementation still uses println! and unit return.
ast-grep --pattern $'pub fn print_table($_, $_, $_) { $$$ println!($_); $$$ }'

# Show callsites that will need to handle Result if signature changes.
rg -nP --type rust '\bprint_table\s*\(' crates/relayburn-cli/src -C2
🤖 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 `@crates/relayburn-cli/src/render/table.rs` around lines 63 - 65, Change
print_table to return std::io::Result<()> instead of unit and propagate I/O
errors instead of panicking; replace the println! call with writing to stdout
using a Write handle (e.g. let mut out = std::io::stdout(); writeln!(out, "{}",
rendered)?;) and return Ok(()). Update the function signature pub fn
print_table(globals: &GlobalArgs, headers: &[&str], rows: &[Vec<String>]) ->
std::io::Result<()> and ensure callers of print_table handle the Result.
🧹 Nitpick comments (1)
crates/relayburn-cli/src/render/table.rs (1)

156-174: ⚡ Quick win

Test name and assertions don’t match the behavior being documented.

The test is named as if ANSI is stripped from pre-styled cells, but it only passes "plain" (no ANSI), so it doesn’t validate that contract either way.

Proposed fix
-    fn no_color_strips_pre_styled_cell_ansi() {
+    fn no_color_passes_through_pre_styled_cell_ansi_verbatim() {
@@
         let rendered = render_table(
             &no_color_globals(),
             &["k"],
-            &[vec!["plain".into()]],
+            &[vec!["\u{1b}[31mred\u{1b}[0m".into()]],
         );
-        assert!(!rendered.contains('\u{1b}'));
+        assert!(
+            rendered.contains('\u{1b}'),
+            "pre-styled cell content should be passed through verbatim"
+        );
     }
🤖 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 `@crates/relayburn-cli/src/render/table.rs` around lines 156 - 174, The test
no_color_strips_pre_styled_cell_ansi currently documents stripping but passes
only "plain", so either rename it to reflect pass-through behavior or change the
input/assertion to actually test stripping; prefer keeping current contract
(renderer doesn't add color) — update the test function name
(no_color_passes_through_pre_styled_cell or
no_color_does_not_add_color_to_pre_styled_cells) and change the test to pass an
ANSI-containing cell (e.g. "\u{1b}[31mred\u{1b}[0m") via
render_table(no_color_globals(), &["k"],
&[vec!["\u{1b}[31mred\u{1b}[0m".into()]]), then assert that the rendered output
contains the same escape sequence (or at minimum contains '\u{1b}') to reflect
pass-through; keep references to render_table and no_color_globals to locate the
code.
🤖 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.

Duplicate comments:
In `@crates/relayburn-cli/src/render/table.rs`:
- Around line 63-65: Change print_table to return std::io::Result<()> instead of
unit and propagate I/O errors instead of panicking; replace the println! call
with writing to stdout using a Write handle (e.g. let mut out =
std::io::stdout(); writeln!(out, "{}", rendered)?;) and return Ok(()). Update
the function signature pub fn print_table(globals: &GlobalArgs, headers:
&[&str], rows: &[Vec<String>]) -> std::io::Result<()> and ensure callers of
print_table handle the Result.

---

Nitpick comments:
In `@crates/relayburn-cli/src/render/table.rs`:
- Around line 156-174: The test no_color_strips_pre_styled_cell_ansi currently
documents stripping but passes only "plain", so either rename it to reflect
pass-through behavior or change the input/assertion to actually test stripping;
prefer keeping current contract (renderer doesn't add color) — update the test
function name (no_color_passes_through_pre_styled_cell or
no_color_does_not_add_color_to_pre_styled_cells) and change the test to pass an
ANSI-containing cell (e.g. "\u{1b}[31mred\u{1b}[0m") via
render_table(no_color_globals(), &["k"],
&[vec!["\u{1b}[31mred\u{1b}[0m".into()]]), then assert that the rendered output
contains the same escape sequence (or at minimum contains '\u{1b}') to reflect
pass-through; keep references to render_table and no_color_globals to locate the
code.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 00213bf6-f96f-48dd-8909-89d3471f7434

📥 Commits

Reviewing files that changed from the base of the PR and between 735ac72 and 3a20c5b.

📒 Files selected for processing (1)
  • crates/relayburn-cli/src/render/table.rs

…son smoke (review fixes round 2)

- render::table::print_table now returns io::Result<()> so EPIPE
  (`burn summary | head`) bubbles to the dispatcher instead of panicking
  inside println!. Mirrors render_json's shape; happy-path unit test pins
  Ok(()).
- render::json smoke test now asserts is_ok() rather than discarding the
  Result, so a future regression that returns Err without panicking will
  surface.
- commands::mod doc reflects both output paths: stderr in human mode and
  stdout JSON envelope in --json mode, matching report_unimplemented.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/relayburn-cli/src/render/table.rs (1)

185-205: 💤 Low value

Test name is slightly misleading—consider renaming for clarity.

The test is named no_color_strips_pre_styled_cell_ansi, but the comment correctly documents that pre-styled cells are passed through verbatim (not stripped). The test also doesn't actually include pre-styled input—it uses a plain string.

A more accurate name would be something like no_color_does_not_add_ansi or no_color_renderer_emits_no_escapes, which matches what the test actually verifies.

Not blocking—the inline documentation is clear about the actual contract.

🤖 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 `@crates/relayburn-cli/src/render/table.rs` around lines 185 - 205, Rename the
test function no_color_strips_pre_styled_cell_ansi to a name that matches the
asserted behavior (e.g., no_color_does_not_add_ansi or
no_color_renderer_emits_no_escapes) and update any references; this test is
exercising render_table with no_color_globals and asserting the rendered output
contains no escape bytes, so ensure the new name reflects that the renderer does
not emit ANSI rather than stripping pre-styled cell content, and update the doc
comment to match the renamed test.
🤖 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 `@crates/relayburn-cli/src/render/table.rs`:
- Around line 185-205: Rename the test function
no_color_strips_pre_styled_cell_ansi to a name that matches the asserted
behavior (e.g., no_color_does_not_add_ansi or
no_color_renderer_emits_no_escapes) and update any references; this test is
exercising render_table with no_color_globals and asserting the rendered output
contains no escape bytes, so ensure the new name reflects that the renderer does
not emit ANSI rather than stripping pre-styled cell content, and update the doc
comment to match the renamed test.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 207f9896-2bb1-4334-a7a3-b12a13faa384

📥 Commits

Reviewing files that changed from the base of the PR and between 3a20c5b and 17cdbdb.

📒 Files selected for processing (3)
  • crates/relayburn-cli/src/commands/mod.rs
  • crates/relayburn-cli/src/render/json.rs
  • crates/relayburn-cli/src/render/table.rs

…go.toml, Cargo.lock conflicts)

# Conflicts:
#	CHANGELOG.md
#	Cargo.lock
#	crates/relayburn-cli/Cargo.toml
@willwashburn willwashburn merged commit ac1b5d0 into main May 6, 2026
3 checks passed
@willwashburn willwashburn deleted the rust-port/cli-scaffold branch May 6, 2026 13:49
willwashburn added a commit that referenced this pull request May 6, 2026
…cts after #309 landed)

# Conflicts:
#	CHANGELOG.md
#	Cargo.lock
#	crates/relayburn-cli/Cargo.toml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant