Implement Rust summary parity#367
Merged
willwashburn merged 2 commits intomainfrom May 7, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThis PR extends the Rust ChangesMulti-Mode Summary Expansion with Tool Cost Attribution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 1a7e0435fa
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/relayburn-cli/src/commands/summary.rs (1)
170-174:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
--no-archiveintoLedgerOpenOptions.
SummaryArgs::no_archiveis parsed, but this open path never reads it, soburn summary --no-archivestill uses the default archive behavior. That makes the documented escape hatch a no-op.🤖 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/summary.rs` around lines 170 - 174, SummaryArgs::no_archive is parsed but never applied when opening the ledger; modify the LedgerOpenOptions you build before calling Ledger::open so it reflects the parsed no_archive flag from SummaryArgs (e.g. after creating opts via LedgerOpenOptions::with_home(h) or LedgerOpenOptions::default(), set opts.no_archive = summary_args.no_archive or call the appropriate builder method like opts.set_no_archive(summary_args.no_archive)), then pass that opts into Ledger::open so --no-archive actually changes archive behavior.
🤖 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/summary.rs`:
- Around line 196-203: The cost-attribution logic is running against the
filtered turn stream causing mis-attribution when turns are skipped; fix by
performing tool-cost attribution on the original unfiltered enriched list and
only filter afterward (or ensure render_by_tool_mode/attribute_cost_to_tools
iterate the full enriched sequence). Specifically, keep the initial enriched =
handle.raw().query_turns(&q)? result intact, call
attribute_cost_to_tools(enriched, ...) / render_by_tool_mode(enriched, ...) on
that full list, then call filter_enriched_turns(...) and
turns_from_enriched(...) to produce the displayed turns so indices like list[i -
1] reference real session predecessors rather than the post-filtered sequence.
- Around line 1539-1549: The BFS loop only queries rows with session_id == id,
missing child edges stored under related_session_id/agent_id; change the lookup
to fetch both kinds of relationships (e.g., perform two queries via
ledger.query_relationships: one with Query { session_id: Some(id),
..query_base.clone() } and another with Query { related_session_id: Some(id),
..query_base.clone() } ), iterate both result sets (deduping using the existing
out/seen_ids logic), and then continue using relationship_connected_ids(&r) and
inserting with relationship_instance_key(&r) as before so subagent children are
discovered.
In `@crates/relayburn-sdk/src/ledger/content.rs`:
- Around line 130-134: The query function currently eagerly collects all content
bodies into memory via query_map(...).collect(), causing memory spikes; change
it to stream rows from stmt.query_map(...) and iterate over the returned
iterator, extracting each row's body, applying the Query filter and only
deserializing/pushing matching entries into the result Vec<ContentRecord>.
Update logic in the query function (referencing conn, query, stmt, rows, Query,
ContentRecord) to loop over the iterator, handle rusqlite::Result for each row,
parse JSON (e.g., serde_json::from_str) and construct ContentRecord only when
the row satisfies the Query, avoiding the intermediate Vec<String>.
---
Outside diff comments:
In `@crates/relayburn-cli/src/commands/summary.rs`:
- Around line 170-174: SummaryArgs::no_archive is parsed but never applied when
opening the ledger; modify the LedgerOpenOptions you build before calling
Ledger::open so it reflects the parsed no_archive flag from SummaryArgs (e.g.
after creating opts via LedgerOpenOptions::with_home(h) or
LedgerOpenOptions::default(), set opts.no_archive = summary_args.no_archive or
call the appropriate builder method like
opts.set_no_archive(summary_args.no_archive)), then pass that opts into
Ledger::open so --no-archive actually changes archive behavior.
🪄 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: ca8a8bad-6c98-4d9f-9e04-645f8d8ad617
📒 Files selected for processing (4)
crates/relayburn-cli/src/commands/summary.rscrates/relayburn-sdk/src/ledger.rscrates/relayburn-sdk/src/ledger/content.rscrates/relayburn-sdk/src/lib.rs
Member
Author
|
Addressed the outside-diff --no-archive comment in 2bccc01 as well: summary now installs the same RELAYBURN_ARCHIVE=0 guard pattern used by compare before opening/running the ledger path. Validation rerun: cargo test -p relayburn-cli; cargo test -p relayburn-sdk; BURN_GOLDEN=1 cargo test -p relayburn-cli --test golden -- --nocapture. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/relayburn-cli/src/commands/summary.rs (2)
854-872: 💤 Low valueConsider filtering user turns by session at the query level if supported.
load_user_turns_for_by_toolloads all user turns viaquery_user_turns(&Query::default())and filters in-memory by session ID. For large ledgers with many sessions, this could be inefficient. If the SDK'squery_user_turnssupportssession_idfiltering, consider passing the session IDs to reduce I/O.If the SDK doesn't support this filter, the current approach is acceptable.
🤖 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/summary.rs` around lines 854 - 872, The function load_user_turns_for_by_tool currently calls ledger.query_user_turns(&Query::default()) and then filters by session_id in-memory, which is inefficient for large ledgers; update the call to pass a Query that includes the session_ids (or a single session_id when iterating) if the SDK supports session_id filtering (e.g., construct a Query with session_id or a sessions list and call ledger.query_user_turns(&query)), falling back to the existing in-memory filter only if the SDK lacks that filter; keep references to load_user_turns_for_by_tool, ledger.query_user_turns, and Query::default when making the change.
453-454: 💤 Low value
std::env::set_var/remove_varwill requireunsafeblocks in Rust Edition 2024.These functions are unsafe in Edition 2024 and will require explicit
unsafeblocks to compile. Currently set at lines 24–25 (inArchiveOverride::activate) and lines 39–40 (inDropimpl) without unsafe wrapping. While the current Edition 2021 allows this, the pattern is unsound in multi-threaded contexts and will break on upgrade.Consider refactoring to read the config once at startup and pass it through the call chain, or use a thread-safe configuration struct instead of relying on process-wide environment mutation.
🤖 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/summary.rs` around lines 453 - 454, The code currently mutates the process environment with std::env::set_var/remove_var in ArchiveOverride::activate and its Drop impl (and toggles RELAYBURN_ARCHIVE), which will become unsafe in Rust 2024; instead refactor to stop mutating process-wide env: change ArchiveOverride::activate to accept a configuration flag (or a reference to a thread-safe config struct/OnceCell) passed in from program startup, store that flag on the ArchiveOverride instance, and remove any calls to std::env::set_var/remove_var from both ArchiveOverride::activate and the Drop impl; alternatively implement a thread-local or atomic/shared config that is initialized once at startup and read by the code paths that currently depend on RELAYBURN_ARCHIVE, ensuring no direct process-wide env mutation occurs.
🤖 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/summary.rs`:
- Around line 854-872: The function load_user_turns_for_by_tool currently calls
ledger.query_user_turns(&Query::default()) and then filters by session_id
in-memory, which is inefficient for large ledgers; update the call to pass a
Query that includes the session_ids (or a single session_id when iterating) if
the SDK supports session_id filtering (e.g., construct a Query with session_id
or a sessions list and call ledger.query_user_turns(&query)), falling back to
the existing in-memory filter only if the SDK lacks that filter; keep references
to load_user_turns_for_by_tool, ledger.query_user_turns, and Query::default when
making the change.
- Around line 453-454: The code currently mutates the process environment with
std::env::set_var/remove_var in ArchiveOverride::activate and its Drop impl (and
toggles RELAYBURN_ARCHIVE), which will become unsafe in Rust 2024; instead
refactor to stop mutating process-wide env: change ArchiveOverride::activate to
accept a configuration flag (or a reference to a thread-safe config
struct/OnceCell) passed in from program startup, store that flag on the
ArchiveOverride instance, and remove any calls to std::env::set_var/remove_var
from both ArchiveOverride::activate and the Drop impl; alternatively implement a
thread-local or atomic/shared config that is initialized once at startup and
read by the code paths that currently depend on RELAYBURN_ARCHIVE, ensuring no
direct process-wide env mutation occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 32556da9-65fb-433d-ab4e-84a6efe2b674
📒 Files selected for processing (2)
crates/relayburn-cli/src/commands/summary.rscrates/relayburn-sdk/src/ledger/content.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
burn summaryparity for workflow/agent/provider filters--by-tool,--by-subagent-type,--by-relationship,--subagent-tree, and--qualityTests
cargo test -p relayburn-clicargo test -p relayburn-sdkBURN_GOLDEN=1 cargo test -p relayburn-cli --test golden -- --nocapture