relayburn-cli: burn overhead + overhead trim presenters (#248 D2)#312
relayburn-cli: burn overhead + overhead trim presenters (#248 D2)#312willwashburn merged 3 commits intomainfrom
Conversation
Wire `burn overhead` and `burn overhead trim` as thin presenters over `relayburn_sdk::overhead` / `::overhead_trim`. Output (human + `--json`) is byte-equivalent with the TS CLI; the four overhead golden invocations flip to `enabled: true`. To make the JSON shape match TS, widen `OverheadAttributionDetail` to include `totalTokens` + `sessionCosts` (in TS field order) and let `OverheadSectionCost.section` carry the full `MarkdownSection` rather than a slim projection. The CLI's `--json` writer post-processes integer-valued `f64`s back to bare integers (`0` not `0.0`) so the golden snapshots stay byte-identical to `JSON.stringify` output. Smoke test: drop `overhead` from the not-yet-implemented gate.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR fully implements the Rust CLI ChangesOverhead CLI Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/relayburn-sdk/src/query_verbs.rs (1)
426-446:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis mutates the published SDK surface in a breaking way.
OverheadSectionCost.sectionchanges type andOverheadAttributionDetailgains new required fields, so downstream Rust callers that construct or destructure these public structs will stop compiling. Ifrelayburn-sdkis being released independently, this needs a semver-major release or a compat layer/new v2 types instead of rewriting the existing ones in place.🤖 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-sdk/src/query_verbs.rs` around lines 426 - 446, The change breaks the public SDK surface by altering OverheadSectionCost.section's type and adding required fields to OverheadAttributionDetail; either revert these in-place changes or introduce a backward-compatible alternative: keep the existing OverheadSectionCost and OverheadAttributionDetail as-is, and add new structs (e.g., OverheadSectionCostV2 and OverheadAttributionDetailV2) with the new MarkdownSection type and extra fields, update internal code to use the V2 types where needed, and leave the original types and serde signatures untouched (or mark new fields as Option<T> with #[serde(default)] if you must mutate the existing types). Ensure public symbols OverheadSectionCost, OverheadAttributionDetail, OverheadSectionCostV2, and OverheadAttributionDetailV2 are defined so downstream callers keep compiling or can opt into v2.
🧹 Nitpick comments (1)
crates/relayburn-cli/tests/smoke.rs (1)
37-42: ⚡ Quick winAdd a smoke assertion for
burn overhead trim --help.Once
overheadmoves intoIMPLEMENTED_SUBCOMMANDS, the suite no longer touches the nestedtrimhelp path. A dedicated assertion would cover one of this PR's unchecked acceptance items and catch clap wiring regressions early.Suggested test shape
+#[test] +fn overhead_trim_help_exits_zero_with_non_empty_stdout() { + let output = burn() + .args(["overhead", "trim", "--help"]) + .assert() + .success() + .get_output() + .clone(); + let stdout = String::from_utf8(output.stdout).expect("help should be valid UTF-8"); + assert!(!stdout.is_empty(), "`overhead trim --help` should emit non-empty stdout"); +}🤖 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 37 - 42, Add a dedicated smoke assertion that runs the CLI with the args ["overhead","trim","--help"] and asserts it exits successfully and prints usage text; update the test suite in crates/relayburn-cli/tests/smoke.rs (near IMPLEMENTED_SUBCOMMANDS and related helpers like each_stub_exits_one_with_not_yet_implemented_message) by adding a new test function (e.g., smoke_overhead_trim_help) that spawns the binary or invokes the test runner helper, checks exit code == 0 and that stdout/stderr contains the expected help/usage fragment for the trim subcommand to catch clap wiring regressions early.
🤖 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 `@crates/relayburn-cli/src/commands/overhead.rs`:
- Around line 127-135: The JSON output currently may reorder object keys; update
the workspace dependency for serde_json to enable the preserve_order feature by
changing serde_json = "1" to serde_json = { version = "1", features =
["preserve_order"] } in Cargo.toml so that functions like
render_json_ts_compatible and coerce_integer_floats emit fields in declaration
order for byte-equivalent snapshots with the TypeScript CLI.
- Around line 143-156: The function coerce_integer_floats currently calls
Number::as_f64() on every serde_json::Number which forces integer numbers
through an f64 and can cause precision loss; change the logic to first check
n.is_f64() and only perform the as_f64()/fractional tests when the Number is
actually stored as a float, leaving integer-backed Numbers untouched; update the
branches around Number::as_f64() in coerce_integer_floats so integers are
skipped and only float-backed values are coerced to integer
Number::from(u64/i64) when appropriate.
---
Outside diff comments:
In `@crates/relayburn-sdk/src/query_verbs.rs`:
- Around line 426-446: The change breaks the public SDK surface by altering
OverheadSectionCost.section's type and adding required fields to
OverheadAttributionDetail; either revert these in-place changes or introduce a
backward-compatible alternative: keep the existing OverheadSectionCost and
OverheadAttributionDetail as-is, and add new structs (e.g.,
OverheadSectionCostV2 and OverheadAttributionDetailV2) with the new
MarkdownSection type and extra fields, update internal code to use the V2 types
where needed, and leave the original types and serde signatures untouched (or
mark new fields as Option<T> with #[serde(default)] if you must mutate the
existing types). Ensure public symbols OverheadSectionCost,
OverheadAttributionDetail, OverheadSectionCostV2, and
OverheadAttributionDetailV2 are defined so downstream callers keep compiling or
can opt into v2.
---
Nitpick comments:
In `@crates/relayburn-cli/tests/smoke.rs`:
- Around line 37-42: Add a dedicated smoke assertion that runs the CLI with the
args ["overhead","trim","--help"] and asserts it exits successfully and prints
usage text; update the test suite in crates/relayburn-cli/tests/smoke.rs (near
IMPLEMENTED_SUBCOMMANDS and related helpers like
each_stub_exits_one_with_not_yet_implemented_message) by adding a new test
function (e.g., smoke_overhead_trim_help) that spawns the binary or invokes the
test runner helper, checks exit code == 0 and that stdout/stderr contains the
expected help/usage fragment for the trim subcommand to catch clap wiring
regressions early.
🪄 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: 48ca14bd-08b0-4c64-828d-a58609df65ce
📒 Files selected for processing (9)
CHANGELOG.mdcrates/relayburn-cli/src/cli.rscrates/relayburn-cli/src/commands/overhead.rscrates/relayburn-cli/src/main.rscrates/relayburn-cli/tests/smoke.rscrates/relayburn-sdk/src/lib.rscrates/relayburn-sdk/src/query_verbs.rstests/fixtures/cli-golden/invocations.jsontests/fixtures/cli-golden/ledger/.gitignore
Three non-architectural fixes from PR #312 review: 1. Add `is_f64()` guard in `coerce_integer_floats` so exact-integer JSON Numbers above 2^53 round-trip losslessly. Previously every Number went through `as_f64()` even when it was already an integer, and `9_007_199_254_740_993` (2^53 + 1) would silently truncate to `9_007_199_254_740_992`. Adds four unit tests covering the guarded path, the lossless large-u64 case, the whole-valued-float coercion, and the recurse-into-array-and-object arms. 2. Pin `features = ["preserve_order"]` on `serde_json` in relayburn-cli's Cargo.toml. Cargo's feature unification means the CLI inherits this transitively from the SDK today, but pinning it explicitly is defense-in-depth so a future SDK refactor can't silently regress the byte-equivalent JSON snapshots. 3. Add `overhead trim --help` smoke test. Now that `overhead` is in `IMPLEMENTED_SUBCOMMANDS`, the parent help-coverage test no longer touches the nested `trim` action; cover it explicitly so a regression in the nested-action help wiring doesn't slip past CI.
|
Re: CodeRabbit's outside-diff flag on Keeping the widening — it's deliberate, not an oversight. Design rationale (confirmed with the project lead):
We'll lock the contract at the 1.0 cutover. Until then, additive widening is the path. |
…NTED_SUBCOMMANDS conv.
Folds D1+D2+D3 (PRs #315/#312/#314) into the D4 state branch. Resolutions: - CHANGELOG.md: union all four bullets, alphabetized. - crates/relayburn-cli/src/cli.rs: keep all five typed Command variants (Summary/Hotspots/Overhead/Compare/State); merge ValueEnum import; keep D4's state-related types (StateArgs et al.) plus D2/D3's OverheadArgs / CompareArgs / OverheadKind. - crates/relayburn-cli/src/main.rs: auto-merged — five typed dispatch arms. - crates/relayburn-cli/tests/smoke.rs: drop REAL_COMMANDS allow-list and early-continue; remove "state" from UNIMPLEMENTED_SUBCOMMANDS, leaving ["run", "ingest", "mcp-server"]. - tests/fixtures/cli-golden/ledger/.gitignore: superset of patterns. - Cargo.lock: regenerated from origin/main. - tests/fixtures/cli-golden/snapshots/state-status*.stdout.txt: keep D4's 2.0 SQLite layout (deliberate divergence) but refresh row counts to reflect the bootstrapped fixture (golden.rs on main now replays ledger.jsonl into burn.sqlite before running, so the fixture has 18 rows instead of 0).
Summary
burn overheadandburn overhead trimas thin presenters overrelayburn_sdk::overhead/::overhead_trim, matching the TS CLI flag set + output.Notes
relayburn_sdk::OverheadAttributionDetailto carrytotalTokens+sessionCostsand re-ordered the field set to match the TS shape;OverheadSectionCost.sectionnow carries the fullMarkdownSectionrather than a slim projection. Both moves were needed to make the SDK's JSON serialization byte-equivalent with the TS sibling.--jsonwriter post-processes integer-valuedf64s back to bare integers (0not0.0) so the golden snapshots matchJSON.stringify.Test plan
🤖 Generated with Claude Code