Skip to content

relayburn-cli: burn run driver + Claude adapter (#248 D5)#318

Merged
willwashburn merged 4 commits intomainfrom
rust-port/cli-run-claude
May 7, 2026
Merged

relayburn-cli: burn run driver + Claude adapter (#248 D5)#318
willwashburn merged 4 commits intomainfrom
rust-port/cli-run-claude

Conversation

@willwashburn
Copy link
Copy Markdown
Member

Summary

Wires burn run <harness> as a real driver over the harness substrate from #248 part b, plus the Claude adapter as the canonical eager unit-struct registration in EAGER_ADAPTERS.

  • Driver lives in crates/relayburn-cli/src/commands/run.rs. Lifecycle: plan -> before_spawn -> (start_watcher) -> spawn -> wait -> watcher.stop -> after_exit -> emit ingest summary line. Spawn uses std::process::Command with inherited stdio (avoids needing tokio's process feature); adapter calls run on a current-thread tokio runtime, mirroring the D1 summary presenter pattern. The user-visible exit code is the child's (transparent driver).
  • Watcher slot is plumbed: start_watcher returns Option<WatcherController> and reports flow into a shared IngestReport accumulator. Claude returns None; codex/opencode ([Rust port] relayburn-cli: clap CLI + mcp-server subcommand (closes #210) #248 D6) hook in via pending_stamp::adapter_static.
  • Claude adapter lives in crates/relayburn-cli/src/harnesses/claude.rs as a zero-sized ClaudeAdapter + static CLAUDE_ADAPTER. plan mints a v4 UUID and injects it via --session-id plus RELAYBURN_SESSION_ID; before_spawn writes a session-targeted Stamp via the SDK ledger; after_exit runs relayburn_sdk::ingest_claude_session (per-session fast-path).
  • Registry: EAGER_ADAPTERS now holds "claude" => &CLAUDE_ADAPTER; the EXPECTED_HARNESS_NAMES snapshot in the deterministic-ordering test is bumped to &["claude"].
  • CLI surface: Command::Run becomes Run(RunArgs) carrying optional harness, repeatable --tag k=v, and a trailing_var_arg passthrough vec. --ledger-path is honored by setting RELAYBURN_HOME for the rest of the process so adapter-internal Ledger::open calls and the spawned child both see it.
  • SDK surfacing: relayburn_sdk::lib.rs re-exports the existing ingest_claude_session so the adapter doesn't reach into private modules. No new SDK code; just a re-export.
  • Smoke: "run" removed from UNIMPLEMENTED_SUBCOMMANDS; the JSON-envelope tripwire moves to "ingest". Two new smoke cases pin burn run (no positional → help + exit 2) and burn run <unknown> (typed error + exit 2 + stderr lists claude).
  • Changelog: one impact-first bullet under root [Unreleased].

Lifecycle (driver perspective)

lookup(name)            -> &'static dyn HarnessAdapter | unknown -> exit 2
build PlanCtx           -> cwd + passthrough + merged --tag/RELAYBURN_* + spawn_start_ts
adapter.plan(ctx)       -> SpawnPlan { binary, args, env_overrides, session_id }
adapter.before_spawn    -> stamp now (claude) | drop pending manifest (codex/opencode)
adapter.start_watcher   -> Option<WatcherController> (claude: None)
spawn child (inherit)   -> wait for exit
watcher.stop / tick     -> drain final state
adapter.after_exit      -> IngestReport (folded into running totals)
emit `[burn] <name> ingest: N session(s) (+M turns)`
exit child.code

Test plan

  • cargo build --workspace clean from worktree
  • cargo test --workspace green (10 cli smoke tests, 56 cli unit tests including the new claude adapter + run driver tests, 610 sdk tests)
  • BURN_GOLDEN=1 cargo test --test golden -p relayburn-cli green — run-help snapshot stays at enabled: false (no new golden enabled this PR)
  • Manual: burn run --help lists usage
  • Manual: burn run (no positional) prints help + lists claude + exits 2
  • Manual: burn run unknown-harness exits 2 with burn: unknown harness "unknown-harness". Known: claude on stderr
  • Manual: burn run claude -- --version runs end-to-end against a local claude binary (mints session-id, stamps, spawns, exits 0, runs after_exit and emits [burn] claude ingest: 0 sessions (+0 turns) because claude --version writes no session file)

Files

  • crates/relayburn-cli/src/harnesses/claude.rs — new
  • crates/relayburn-cli/src/harnesses/mod.rspub mod claude;
  • crates/relayburn-cli/src/harnesses/registry.rsEAGER_ADAPTERS now holds claude; test fixture EXPECTED_HARNESS_NAMES updated
  • crates/relayburn-cli/src/cli.rsRun(RunArgs) + RunArgs struct
  • crates/relayburn-cli/src/commands/run.rs — full driver, replacing the not-yet-implemented stub
  • crates/relayburn-cli/src/main.rs — dispatch arm passes args
  • crates/relayburn-cli/tests/smoke.rsrun removed from unimplemented list; new tripwires for help-on-no-positional and unknown-harness paths; JSON envelope tripwire moved to ingest
  • crates/relayburn-sdk/src/lib.rs — re-export ingest_claude_session (existing public function from crate::ingest; no new code)
  • CHANGELOG.md — root [Unreleased] bullet

Wire `burn run <harness>` as a real driver over the harness substrate
(#248 part b). Lifecycle: `plan -> before_spawn -> spawn (inherited
stdio) -> after_exit`, with an optional `start_watcher` slot that
codex/opencode (#248 D6) will populate. Reports from the watcher and
`after_exit` fold into a single `[burn] <name> ingest: N session(s)
(+M turns)` line on stderr; the user-visible exit code is the child's,
matching the TS sibling.

Claude adapter lands as `CLAUDE_ADAPTER` in `EAGER_ADAPTERS` (eager
unit-struct tier — value is a const expression, no `Box::leak` needed).
`plan` mints a v4 UUID and injects it via `--session-id` plus
`RELAYBURN_SESSION_ID` so transitive `burn ...` invocations inherit the
id; `before_spawn` writes a session-targeted stamp via the SDK ledger;
`after_exit` runs the per-session fast-path
(`relayburn_sdk::ingest_claude_session`).

`relayburn-sdk` re-exports `ingest_claude_session` so the adapter
doesn't have to reach into private ingest modules.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 6, 2026

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: f758b40d-7b89-4884-9b59-6e30717a0012

📥 Commits

Reviewing files that changed from the base of the PR and between 28e6902 and 10f4cd8.

📒 Files selected for processing (6)
  • CHANGELOG.md
  • crates/relayburn-cli/Cargo.toml
  • crates/relayburn-cli/src/cli.rs
  • crates/relayburn-cli/src/commands/mod.rs
  • crates/relayburn-cli/src/main.rs
  • crates/relayburn-cli/tests/smoke.rs
✅ Files skipped from review due to trivial changes (1)
  • CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/relayburn-cli/src/cli.rs
  • crates/relayburn-cli/Cargo.toml

📝 Walkthrough

Walkthrough

Implements a typed burn run <harness> CLI driver, the harness substrate and registry wiring, a concrete claude adapter, time utilities, tests, and Cargo/tokio feature updates; run lifecycle includes plan → before_spawn → optional watcher → spawned child → after_exit with aggregated ingest summary.

Changes

burn run Command + Claude Adapter

Layer / File(s) Summary
CLI Arguments
crates/relayburn-cli/src/cli.rs
Command::Run now carries RunArgs (harness: Option<String>, repeatable --tag k=v, passthrough: Vec<String>).
Dispatch Update
crates/relayburn-cli/src/main.rs
Match arm updated to Command::Run(args) and forwards args to commands::run::run.
Run Command Core
crates/relayburn-cli/src/commands/run.rs
Adds run(globals, args) and async drive: adapter lookup, help/unknown handling, --tag parsing, set RELAYBURN_HOME, build PlanCtx, call adapter planbefore_spawn → optional start_watcher → spawn child with merged env and inherited stdio → await exit → stop watcher → call after_exit; prints single stderr ingest summary; returns child exit code (127 on spawn failure). Includes unit tests for tag parsing and spawn env re-export.
Harness Substrate Types & Trait
crates/relayburn-cli/src/harnesses/mod.rs
Adds PlanCtx, SpawnPlan (binary/args/env_overrides/session_id), HarnessAdapter trait with lifecycle hooks and defaults, WatcherController wrapper, tests for trait-object dispatch; re-exports list_harness_names/lookup.
Claude Adapter
crates/relayburn-cli/src/harnesses/claude.rs
Adds ClaudeAdapter and CLAUDE_ADAPTER singleton. plan mints UUID-like session id, injects --session-id and RELAYBURN_SESSION_ID, forwards passthrough args; before_spawn writes a session stamp with iso_now and enrichment; after_exit calls ingest_claude_session. Includes session-id, iso_now, and wiring tests.
Registry Wiring
crates/relayburn-cli/src/harnesses/registry.rs
Registers "claude" => &claude::CLAUDE_ADAPTER in compile-time EAGER_ADAPTERS phf_map and updates deterministic ordering test to include "claude".
Time Utilities
crates/relayburn-cli/src/util/time.rs, crates/relayburn-cli/src/util/mod.rs, crates/relayburn-cli/src/lib.rs
Adds util::time with iso_now, iso_from_system_time, civil_from_days (Howard Hinnant algorithm) and tests; exposes pub mod util; from crate lib.
Tests / Smoke
crates/relayburn-cli/tests/smoke.rs
Removes run from UNIMPLEMENTED stub list; adds tests asserting burn run prints help/known harnesses and exits 2 when harness missing or unknown (includes claude).
Cargo / SDK Reformat / Changelog
crates/relayburn-cli/Cargo.toml, crates/relayburn-sdk/src/lib.rs, CHANGELOG.md
Adds tokio feature process; reformats SDK re-exports; inserts CHANGELOG Unreleased bullet documenting relayburn-cli run/claude wiring.

Sequence Diagram

sequenceDiagram
    actor CLI as CLI Invoker
    participant Dispatch as Dispatcher
    participant Adapter as HarnessAdapter
    participant Child as Child Process
    participant Ledger as Ledger/Ingest

    CLI->>Dispatch: burn run claude [--tag k=v] [-- args...]
    Dispatch->>Adapter: lookup("claude")
    Adapter-->>Dispatch: &CLAUDE_ADAPTER
    Dispatch->>Adapter: plan(PlanCtx)
    Adapter->>Adapter: mint session_id
    Adapter-->>Dispatch: SpawnPlan{binary,args,env_overrides,session_id}
    Dispatch->>Adapter: before_spawn(ctx, plan)
    Adapter->>Ledger: write_session_stamp(iso_now, enrichment)
    Dispatch->>Child: spawn(process with merged env + inherited stdio)
    Child-->>Dispatch: exit(code)
    Dispatch->>Adapter: after_exit(ctx, plan)
    Adapter->>Ledger: ingest_claude_session(session_id, cwd)
    Dispatch->>CLI: print "[burn] claude ingest: <summary>"
    Dispatch->>CLI: return exit code
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related issues

Possibly related PRs

Poem

🐰 I minted a session id with a hop and a bound,

stamped it with timestamps and tags all around,
passed args through the burrow, env keys in my sack,
one burn run later — the rabbit brings data back!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main changes: implementing a real 'burn run' driver and adding a Claude adapter, with the PR identifier.
Description check ✅ Passed The description comprehensively details the driver implementation, Claude adapter, CLI changes, registry updates, and test coverage—all of which align directly with the changeset.
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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rust-port/cli-run-claude

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

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 (4)
crates/relayburn-cli/src/harnesses/claude.rs (2)

48-53: 💤 Low value

Consider using a more portable home directory lookup.

std::env::var_os("HOME") works on Unix but returns None on Windows. If cross-platform support is ever needed, this would silently fall back to ".".

🔧 Suggested fix using dirs crate (if cross-platform matters)
 fn claude_projects_root() -> PathBuf {
-    let home = std::env::var_os("HOME")
-        .map(PathBuf::from)
-        .unwrap_or_else(|| PathBuf::from("."));
+    let home = dirs::home_dir().unwrap_or_else(|| PathBuf::from("."));
     home.join(".claude").join("projects")
 }
🤖 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/harnesses/claude.rs` around lines 48 - 53, The
claude_projects_root function uses std::env::var_os("HOME") which is not
portable to Windows; replace that logic with a cross-platform home lookup (e.g.,
dirs::home_dir() or dirs_next::home_dir()) and fall back to PathBuf::from(".")
if None; update Cargo.toml to add the chosen crate (dirs or dirs-next) and
adjust claude_projects_root to call that crate's home_dir(), map to a PathBuf,
then join(".claude").join("projects") as before (refer to function
claude_projects_root).

106-142: 💤 Low value

Duplicate iso_now / civil_from_days implementation.

This code is duplicated in crates/relayburn-cli/src/commands/run.rs (lines 282-307). The comment at line 281 in run.rs acknowledges this: "Same shape as the claude adapter's local iso_now; keep them in sync."

Consider extracting to a shared utility module to avoid drift. However, since the duplication is documented and the scope is small, this is low priority.

🤖 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/harnesses/claude.rs` around lines 106 - 142, Both
files duplicate the same ISO timestamp logic (iso_now and civil_from_days);
extract these two functions into a new shared util (e.g., a module like
time_utils) and replace the local implementations in claude.rs and run.rs with
calls to that shared iso_now; ensure the new module is public or re-exported so
callers can use time_utils::iso_now(), move the civil_from_days helper into the
same module (keeping signatures unchanged), update imports in both files, and
run cargo build/test to verify nothing broke.
crates/relayburn-cli/src/commands/run.rs (2)

147-158: 💤 Low value

Mutex .lock().unwrap() could panic on poisoned lock.

If the watcher callback panics while holding the lock, subsequent lock() calls will panic. This is unlikely in practice since the callback body is simple arithmetic, but a poisoned lock would crash the driver.

The TS sibling doesn't have this concern since JS is single-threaded. Given the callback simplicity and the low likelihood of panic, this is acceptable for now.

🤖 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/commands/run.rs` around lines 147 - 158, The closure
assigned to on_report uses totals_for_sink.lock() which currently
unwraps/assumes an unpoisoned Mutex and could panic if the lock is poisoned;
update the lock handling inside the on_report closure (referring to
totals_for_sink and the ReportSink closure) to recover from a poisoned lock by
using the PoisonError::into_inner() (e.g., call lock().unwrap_or_else(|e|
e.into_inner()) or match on lock() and use e.into_inner()), then perform the
arithmetic on the recovered guard so the driver won't panic on a poisoned Mutex
while still updating the IngestReport totals.

200-200: 💤 Low value

Signal-terminated child returns exit code 0.

When the child is killed by a signal (e.g., SIGKILL, SIGTERM), exit_status.code() returns None and the fallback is 0. This might mask abnormal termination.

Consider using the signal number as an exit code (128 + signal is a common convention):

🔧 Suggested alternative for signal handling
-    let code = exit_status.code().unwrap_or(0);
+    #[cfg(unix)]
+    let code = exit_status.code().unwrap_or_else(|| {
+        use std::os::unix::process::ExitStatusExt;
+        exit_status.signal().map(|s| 128 + s).unwrap_or(1)
+    });
+    #[cfg(not(unix))]
+    let code = exit_status.code().unwrap_or(1);
🤖 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/commands/run.rs` at line 200, The current line let
code = exit_status.code().unwrap_or(0); masks signal-terminated children by
returning 0; change it to inspect the platform-specific signal number when
code() is None (use std::os::unix::process::ExitStatusExt::signal() on Unix) and
map that to a conventional exit value (e.g., 128 + signal); specifically replace
the unwrap_or(0) behavior for exit_status (the exit_status variable/ExitStatus
value in run.rs, where code is computed) with logic that returns
exit_status.code() when Some, otherwise maps exit_status.signal() to 128 +
signal and falls back to 1 if neither is available.
🤖 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/commands/run.rs`:
- Around line 147-158: The closure assigned to on_report uses
totals_for_sink.lock() which currently unwraps/assumes an unpoisoned Mutex and
could panic if the lock is poisoned; update the lock handling inside the
on_report closure (referring to totals_for_sink and the ReportSink closure) to
recover from a poisoned lock by using the PoisonError::into_inner() (e.g., call
lock().unwrap_or_else(|e| e.into_inner()) or match on lock() and use
e.into_inner()), then perform the arithmetic on the recovered guard so the
driver won't panic on a poisoned Mutex while still updating the IngestReport
totals.
- Line 200: The current line let code = exit_status.code().unwrap_or(0); masks
signal-terminated children by returning 0; change it to inspect the
platform-specific signal number when code() is None (use
std::os::unix::process::ExitStatusExt::signal() on Unix) and map that to a
conventional exit value (e.g., 128 + signal); specifically replace the
unwrap_or(0) behavior for exit_status (the exit_status variable/ExitStatus value
in run.rs, where code is computed) with logic that returns exit_status.code()
when Some, otherwise maps exit_status.signal() to 128 + signal and falls back to
1 if neither is available.

In `@crates/relayburn-cli/src/harnesses/claude.rs`:
- Around line 48-53: The claude_projects_root function uses
std::env::var_os("HOME") which is not portable to Windows; replace that logic
with a cross-platform home lookup (e.g., dirs::home_dir() or
dirs_next::home_dir()) and fall back to PathBuf::from(".") if None; update
Cargo.toml to add the chosen crate (dirs or dirs-next) and adjust
claude_projects_root to call that crate's home_dir(), map to a PathBuf, then
join(".claude").join("projects") as before (refer to function
claude_projects_root).
- Around line 106-142: Both files duplicate the same ISO timestamp logic
(iso_now and civil_from_days); extract these two functions into a new shared
util (e.g., a module like time_utils) and replace the local implementations in
claude.rs and run.rs with calls to that shared iso_now; ensure the new module is
public or re-exported so callers can use time_utils::iso_now(), move the
civil_from_days helper into the same module (keeping signatures unchanged),
update imports in both files, and run cargo build/test to verify nothing broke.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: aeb1b65a-da11-420f-931c-9cb198cf80f0

📥 Commits

Reviewing files that changed from the base of the PR and between 067c096 and c82ff04.

📒 Files selected for processing (9)
  • CHANGELOG.md
  • crates/relayburn-cli/src/cli.rs
  • crates/relayburn-cli/src/commands/run.rs
  • crates/relayburn-cli/src/harnesses/claude.rs
  • crates/relayburn-cli/src/harnesses/mod.rs
  • crates/relayburn-cli/src/harnesses/registry.rs
  • crates/relayburn-cli/src/main.rs
  • crates/relayburn-cli/tests/smoke.rs
  • crates/relayburn-sdk/src/lib.rs

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: c82ff04670

ℹ️ 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".

Comment on lines +193 to +198
Err(err) => {
eprintln!("[burn] failed to spawn {}: {err}", plan.binary);
// Match the TS sibling: 127 for spawn failure (POSIX
// "command not found"-ish).
return Ok(127);
}
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 Run cleanup path even when harness spawn fails

The Err branch from cmd.status() returns 127 immediately, which skips both watcher.stop() and adapter.after_exit(). This means before_spawn side effects have already happened (stamps / pending manifests), but they are never reconciled when the harness binary is missing or not executable. The TypeScript sibling still runs finalization after a spawn error, so this early return causes stale ingest state and drops the expected final ingest summary in exactly the failure mode where users most need cleanup.

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 28e6902. Reworked the spawn block to capture the outcome into a local SpawnOutcome enum (Exited(ExitStatus) / SpawnFailed) and run watcher.stop(), adapter.after_exit(), and the [burn] <name> ingest: ... summary line unconditionally before mapping to the process exit code at the very end. Spawn failure no longer skips finalization, so any before_spawn side effects (claude's eager stamp / codex+opencode's pending-stamp manifests via the watcher's first tick) are still reconciled. after_exit errors are now folded into the summary as a non-fatal stderr line for the same reason. Manual smoke (PATH=/usr/bin:/bin burn run claude -- --version) confirms the missing-binary path emits [burn] failed to spawn …, then [burn] no session file found … (after_exit ran), then the [burn] claude ingest: 0 sessions (+0 turns) summary line, then exits 127.

w.tick().await;
}

let exit_status = match cmd.status() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid blocking watcher execution with synchronous wait

The driver uses a current-thread Tokio runtime but then blocks it with std::process::Command::status(). Because this synchronous wait does not yield, any watcher task started by start_watcher cannot make periodic progress while the child process is running, so "live" ingest effectively collapses to post-exit ingest only. This breaks the intended watch-loop behavior for adapters that rely on in-flight draining.

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 28e6902. Switched the spawn from std::process::Command::status() to tokio::process::Command::status().await so the current-thread runtime can yield between tokio primitives while the child is alive. Watcher tasks (the SDK's WatchController::tick / stop) can now make periodic progress instead of being starved until exit. Added the process feature to the relayburn-cli tokio dep — none of the rest of the workspace needs it, so the change is local. Pairs naturally with the P1 cleanup refactor (sibling comment 3198179089): the captured SpawnOutcome enum is the natural container for the Result<ExitStatus, io::Error> the async status() returns, so both fixes landed in the same edit.

Resolves D5 (claude adapter) vs D6 (codex adapter) conflicts:
- harnesses/registry.rs: union EAGER_ADAPTERS (claude) + RUNTIME_ADAPTERS
  (codex); EXPECTED_HARNESS_NAMES = ["claude", "codex"].
- harnesses/mod.rs: keep both 'pub mod claude;' and 'pub mod codex;'.
- CHANGELOG.md: keep both Unreleased bullets.
- relayburn-sdk/src/lib.rs: union ingest_claude_session +
  ingest_codex_sessions / ingest_opencode_sessions re-exports.
- Cargo.lock regenerated from origin/main.
…l extract)

Three fixes on top of the merge with origin/main:

P1 (Codex 3198179089) — burn run driver skipped finalization on spawn
failure. before_spawn may have written a session stamp / pending-stamp
manifest, and the watcher's first tick may have accumulated reports;
returning early on Err(spawn) left both unreconciled and dropped the
expected `[burn] <name> ingest: ...` summary line. Refactored the spawn
block to capture the outcome into a local SpawnOutcome enum, then run
watcher.stop() / adapter.after_exit() / summary emission unconditionally
before mapping to the exit code (127 on spawn failure, child code
otherwise).

P2 (Codex 3198179092) — driver ran on a current-thread Tokio runtime but
blocked it with std::process::Command::status(), starving any watcher
ticks scheduled on the same runtime. Switched to
tokio::process::Command::status().await so the runtime can yield while
the child is alive. Added the `process` feature to the relayburn-cli
tokio dep (the rest of the workspace doesn't need it).

CodeRabbit nitpick #2 — duplicate iso_now / civil_from_days across
harnesses/claude.rs and commands/run.rs. Extracted to a new shared
crate-internal module crates/relayburn-cli/src/util/time.rs (iso_now,
iso_from_system_time, civil_from_days). Both call sites now import from
relayburn_cli::util::time so the keep-in-sync comment in run.rs is no
longer load-bearing.

Pushed back on two CodeRabbit nitpicks (no code change):
- #1 (claude.rs HOME portability via `dirs` crate): MVP target is
  macOS + Linux; existing fallback to "." is acceptable. Adding a dep
  for non-MVP platforms is unnecessary.
- #3 (run.rs Mutex.lock().unwrap() poison panic): CodeRabbit's own
  assessment was "acceptable for now"; no change.

Verified with cargo build --workspace, cargo test --workspace (all
green, 21 cli tests + 610 sdk tests), BURN_GOLDEN=1 cargo test --test
golden -p relayburn-cli, and a manual spawn-failure smoke
(`PATH=/usr/bin:/bin burn run claude -- --version` exits 127 with the
ingest summary line emitted post-cleanup as expected).
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 (2)
crates/relayburn-cli/src/commands/run.rs (1)

101-103: 💤 Low value

std::env::set_var can be problematic in concurrent contexts.

While this call occurs before the Tokio runtime starts (and is therefore safe here), set_var is being deprecated as safe in future Rust editions due to thread-safety concerns. If this codebase later moves to Rust 2024 edition, this will require unsafe.

For now this is fine, but consider a comment noting the ordering dependency (must run before multi-threading).

🤖 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/commands/run.rs` around lines 101 - 103, This code
calls std::env::set_var("RELAYBURN_HOME", p) when globals.ledger_path is
present; add a concise comment above this block (referencing
globals.ledger_path, std::env::set_var, and the RELAYBURN_HOME env var) stating
that this mutation of the global process environment must occur before the
Tokio/multithreaded runtime is started due to thread-safety concerns and future
Rust edition changes, so it relies on ordering and should not be moved after
runtime initialization (or else converted to a thread-safe alternative); keep
the comment short and specific about the ordering dependency.
crates/relayburn-cli/src/harnesses/claude.rs (1)

49-54: 💤 Low value

Unix-only HOME fallback may behave unexpectedly on Windows.

HOME is not typically set on Windows; you'd need USERPROFILE or similar. The fallback to "." means the session root silently becomes ./claude/projects on Windows, which is almost certainly wrong.

If Windows support is out of scope, consider documenting this or adding a compile-time #[cfg(unix)] guard. Otherwise, consider a cross-platform home-directory lookup.

🤖 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/harnesses/claude.rs` around lines 49 - 54, The
claude_projects_root function currently uses the Unix-only HOME env var and
falls back to ".", which is wrong on Windows; update claude_projects_root to use
a cross-platform home directory lookup (e.g., check USERPROFILE on Windows or
use a crate like dirs/dirs-next to get the user's home_dir) instead of
defaulting to "."; ensure the function returns
home.join(".claude").join("projects") when a real home is found and decide
whether to error or document/guard with #[cfg(unix)] if Windows support is
intentionally out of scope so the behavior is explicit.
🤖 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/commands/run.rs`:
- Around line 101-103: This code calls std::env::set_var("RELAYBURN_HOME", p)
when globals.ledger_path is present; add a concise comment above this block
(referencing globals.ledger_path, std::env::set_var, and the RELAYBURN_HOME env
var) stating that this mutation of the global process environment must occur
before the Tokio/multithreaded runtime is started due to thread-safety concerns
and future Rust edition changes, so it relies on ordering and should not be
moved after runtime initialization (or else converted to a thread-safe
alternative); keep the comment short and specific about the ordering dependency.

In `@crates/relayburn-cli/src/harnesses/claude.rs`:
- Around line 49-54: The claude_projects_root function currently uses the
Unix-only HOME env var and falls back to ".", which is wrong on Windows; update
claude_projects_root to use a cross-platform home directory lookup (e.g., check
USERPROFILE on Windows or use a crate like dirs/dirs-next to get the user's
home_dir) instead of defaulting to "."; ensure the function returns
home.join(".claude").join("projects") when a real home is found and decide
whether to error or document/guard with #[cfg(unix)] if Windows support is
intentionally out of scope so the behavior is explicit.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0aa004d1-8145-407f-96e7-5c973454c4f9

📥 Commits

Reviewing files that changed from the base of the PR and between c82ff04 and 28e6902.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (10)
  • CHANGELOG.md
  • crates/relayburn-cli/Cargo.toml
  • crates/relayburn-cli/src/commands/run.rs
  • crates/relayburn-cli/src/harnesses/claude.rs
  • crates/relayburn-cli/src/harnesses/mod.rs
  • crates/relayburn-cli/src/harnesses/registry.rs
  • crates/relayburn-cli/src/lib.rs
  • crates/relayburn-cli/src/util/mod.rs
  • crates/relayburn-cli/src/util/time.rs
  • crates/relayburn-sdk/src/lib.rs
✅ Files skipped from review due to trivial changes (6)
  • crates/relayburn-cli/src/lib.rs
  • crates/relayburn-cli/src/util/mod.rs
  • crates/relayburn-cli/Cargo.toml
  • crates/relayburn-sdk/src/lib.rs
  • crates/relayburn-cli/src/util/time.rs
  • crates/relayburn-cli/src/harnesses/registry.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • CHANGELOG.md
  • crates/relayburn-cli/src/harnesses/mod.rs

Absorbs #319 (Wave 2 D8 — burn ingest + burn mcp-server) on top of D5's
prior merge with origin/main.

Resolutions:

- crates/relayburn-cli/src/cli.rs — auto-merged: keep all eight typed
  Command variants (D5's Run + D8's Ingest/McpServer + the four already
  typed by D1-D4 + State from D4).
- crates/relayburn-cli/src/main.rs — auto-merged: dispatch arms for all
  eight subcommands route to typed handlers; no more not_yet_implemented
  fallbacks anywhere.
- crates/relayburn-cli/Cargo.toml — union the tokio feature flags from
  both branches: D5 needed `process` (tokio::process::Command::status
  for the run driver), D8 needed `signal` (SIGINT/SIGTERM trap in
  --watch). Final feature set: ["sync", "rt", "process", "signal"].
- crates/relayburn-cli/tests/smoke.rs — every Wave 2 subcommand is now
  wired, so UNIMPLEMENTED_SUBCOMMANDS becomes [] and the
  each_stub_exits_one_with_not_yet_implemented_message iteration helper
  becomes a no-op pass over an empty slice (constant kept so a future
  scaffold has somewhere to land). Also pivoted
  json_mode_emits_error_envelope_on_unimplemented to use `compare`'s
  no-models error path (still routes through report_error so the JSON
  envelope contract is exercised); renamed it
  json_mode_emits_error_envelope_on_argument_failure.
- crates/relayburn-cli/src/commands/mod.rs — `not_yet_implemented` is no
  longer called by any subcommand; added `#[allow(dead_code)]` so the
  placeholder helper survives without warning.
- CHANGELOG.md — additive: keep both D5 and D8 bullets under
  [Unreleased].
- Cargo.lock — refreshed via `git checkout origin/main -- Cargo.lock &&
  cargo build --workspace`. Tokio's `signal` feature pulled in
  signal-hook-registry / mio extensions cleanly.

Verified: cargo build --workspace clean; cargo test --workspace green
(610 SDK + 9 sdk-node + 10 cli smoke + 5 cli golden); BURN_GOLDEN=1
cargo test --test golden -p relayburn-cli passes byte-for-byte.
@willwashburn willwashburn merged commit 810fa96 into main May 7, 2026
8 checks passed
@willwashburn willwashburn deleted the rust-port/cli-run-claude branch May 7, 2026 00:54
willwashburn added a commit that referenced this pull request May 7, 2026
Resolve registry.rs conflicts by unioning all three Wave 2 adapters:
- EAGER_ADAPTERS keeps claude (from #318 D5)
- RUNTIME_ADAPTERS keeps codex (#317 D6) + opencode (this branch, D7)
- RUNTIME_ADAPTER_NAMES = ["codex", "opencode"] (alphabetical)
- EXPECTED_HARNESS_NAMES = ["claude", "codex", "opencode"]

CHANGELOG.md additive: keep opencode bullet alongside D5/D6/D8 bullets.
mod.rs auto-merged with all three pub mod statements.
Cargo.lock taken from origin/main and confirmed unchanged after build.
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