Improve Rust SDK query ergonomics#322
Merged
willwashburn merged 5 commits intomainfrom May 7, 2026
Merged
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.
📝 WalkthroughWalkthroughAdds a new synchronous ChangesCompare verb, ingest scoping, query enrichment, and Node bindings
Sequence Diagram(s)sequenceDiagram
participant Client
participant CompareVerb as compare()
participant Query as Query Builder
participant Ledger as Ledger Reader
participant Enrichment as Enrichment Filter
participant Result as Result Shaper
Client->>CompareVerb: compare(models, provider?, workflow?, min_fidelity?, min_sample?)
CompareVerb->>CompareVerb: validate ≥2 models
CompareVerb->>Query: build_query(session?, provider?, enrichment?)
Query->>Ledger: query_turns(q)
Ledger->>Enrichment: evaluate turn enrichment
Enrichment-->>Ledger: filtered turns
Ledger-->>CompareVerb: EnrichedTurn[]
CompareVerb->>CompareVerb: filter by fidelity >= min_fidelity and model/provider
CompareVerb->>Result: build comparison table, round values, compute excluded breakdown
Result-->>CompareVerb: CompareResult
CompareVerb-->>Client: serialized CompareResult (BigInt-promoted for u64 fields)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 343ada00b7
ℹ️ 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".
# Conflicts: # CHANGELOG.md # crates/relayburn-cli/src/commands/overhead.rs # crates/relayburn-cli/src/main.rs # crates/relayburn-cli/tests/smoke.rs # crates/relayburn-sdk/src/ingest_verb.rs # crates/relayburn-sdk/src/lib.rs # crates/relayburn-sdk/src/query_verbs.rs # crates/relayburn-sdk/tests/integration.rs # tests/fixtures/cli-golden/ledger/.gitignore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/relayburn-sdk/src/query_verbs.rs`:
- Around line 1370-1398: The code computes fidelity_summary and other metadata
from the full turns set before applying the models subset passed into
build_compare_table, causing top-level counts to mismatch cells/totals; modify
the flow so that the turns vector is filtered by the requested models
(opts.models) before calling summarize_fidelity_from_iter and before applying
min_fidelity filtering and build_compare_table. In practice, apply the models
filter to turns (using the same model-identification logic used by
build_compare_table/AnalyzeCompareOptions) immediately after the provider filter
step so that summarize_fidelity_from_iter, has_minimum_fidelity checks, and the
call to build_compare_table all operate on the same filtered turns set, then
pass that filtered turns into shape_compare_result.
🪄 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: af6ca046-f2f5-4a36-802a-500d0919118f
📒 Files selected for processing (4)
crates/relayburn-cli/src/commands/compare.rscrates/relayburn-sdk-node/src/lib.rscrates/relayburn-sdk/src/lib.rscrates/relayburn-sdk/src/query_verbs.rs
✅ Files skipped from review due to trivial changes (1)
- crates/relayburn-cli/src/commands/compare.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/relayburn-sdk/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/relayburn-cli/src/harnesses/pending_stamp.rs`:
- Around line 203-206: The PendingStampWriteOptions currently hard-codes
ledger_home: None which forces default-home behavior and can break resolution
when ingest uses a non-default ledger; update the creation of
PendingStampWriteOptions to propagate the explicit ledger_home used by the
harness/ingest instead of None (use the ledger_home field from the harness or
the context—e.g., the same ledger_home value passed to or stored on self.harness
or available on ctx) so that write and read directories remain consistent for
PendingStamp resolution.
🪄 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: 7413ac3f-9d74-4e50-acec-7593ebc1ea33
📒 Files selected for processing (3)
crates/relayburn-cli/src/harnesses/pending_stamp.rscrates/relayburn-sdk/src/ingest/pending_stamps.rscrates/relayburn-sdk/src/query_verbs.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/relayburn-cli/src/harnesses/claude.rs (1)
148-151: 💤 Low valueConsider using explicit
ledger_homefor consistency with codex/opencode.The claude adapter relies on
LedgerOpenOptions::default()which picks up theRELAYBURN_HOMEenv var set by the driver. While this works, codex/opencode adapters use the explicitctx.ledger_homefield. For consistency, consider threadingctx.ledger_homethrough here as well.This isn't a bug (env var fallback works correctly), but explicit propagation would make the data flow more visible.
🤖 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 148 - 151, The code opens the ledger with Ledger::open(LedgerOpenOptions::default()) which relies on RELAYBURN_HOME env var; instead propagate ctx.ledger_home explicitly by constructing LedgerOpenOptions with ledger_home set from ctx.ledger_home and pass that to Ledger::open (retain subsequent calls to handle.raw_mut() and ingest_claude_session). Update the call site where LedgerOpenOptions::default() is used so Ledger::open receives an options struct created from ctx.ledger_home to match codex/opencode patterns.crates/relayburn-cli/src/harnesses/pending_stamp.rs (1)
378-389: 💤 Low valueConsider using
tempfilecrate for automatic cleanup.The
tempdirhelper creates unique directories but relies on OS temp cleanup. For test isolation and avoiding accumulation of test artifacts, consider using thetempfilecrate which provides automatic cleanup on drop.That said, this is a minor improvement and the current approach works correctly for the test's purpose.
🤖 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/pending_stamp.rs` around lines 378 - 389, Replace the manual tempdir helper with tempfile's TempDir: change the tempdir function to return tempfile::TempDir and create the directory via tempfile::Builder::new().prefix(&format!("burn-pending-stamp-{label}-{}", std::process::id())).tempdir()? (or use .tempdir_in(std::env::temp_dir()) if you want it under the OS temp dir); add tempfile to Cargo.toml, and update any callers of tempdir (in this file) to either hold the TempDir so it isn't dropped prematurely or use tempdir.path() while keeping the TempDir alive for the lifetime of the test/harness. Ensure function name tempdir and its call sites are updated to the new return type (tempfile::TempDir) so automatic cleanup occurs on drop.
🤖 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/harnesses/claude.rs`:
- Around line 148-151: The code opens the ledger with
Ledger::open(LedgerOpenOptions::default()) which relies on RELAYBURN_HOME env
var; instead propagate ctx.ledger_home explicitly by constructing
LedgerOpenOptions with ledger_home set from ctx.ledger_home and pass that to
Ledger::open (retain subsequent calls to handle.raw_mut() and
ingest_claude_session). Update the call site where LedgerOpenOptions::default()
is used so Ledger::open receives an options struct created from ctx.ledger_home
to match codex/opencode patterns.
In `@crates/relayburn-cli/src/harnesses/pending_stamp.rs`:
- Around line 378-389: Replace the manual tempdir helper with tempfile's
TempDir: change the tempdir function to return tempfile::TempDir and create the
directory via
tempfile::Builder::new().prefix(&format!("burn-pending-stamp-{label}-{}",
std::process::id())).tempdir()? (or use .tempdir_in(std::env::temp_dir()) if you
want it under the OS temp dir); add tempfile to Cargo.toml, and update any
callers of tempdir (in this file) to either hold the TempDir so it isn't dropped
prematurely or use tempdir.path() while keeping the TempDir alive for the
lifetime of the test/harness. Ensure function name tempdir and its call sites
are updated to the new return type (tempfile::TempDir) so automatic cleanup
occurs on drop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bec65118-3d12-4ee7-9e46-8a5d210e46e9
📒 Files selected for processing (7)
crates/relayburn-cli/src/commands/run.rscrates/relayburn-cli/src/harnesses/claude.rscrates/relayburn-cli/src/harnesses/codex.rscrates/relayburn-cli/src/harnesses/mod.rscrates/relayburn-cli/src/harnesses/opencode.rscrates/relayburn-cli/src/harnesses/pending_stamp.rscrates/relayburn-cli/src/harnesses/registry.rs
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
compareverb, typed result shape, NAPI binding, and coverage in the ten-verb integration testQueryinstead of post-hoc scansledger_homethrough ingest config and pending-stamp sidecars, and wire ghost-surface findings into Rust hotspotsTests
cargo test --workspacecargo clippy --workspace --all-targets(passes with existing repo warnings)