Architecture refactor + breaking public-API tightening (→ 4.0.0)#488
Merged
Conversation
classifier.rs (1915 lines) mixed three distinct concerns. Extract two into a classifier/ submodule directory, leaving the activity classifier + rule tables in the parent: - classifier/bash_parse.rs (516) — the bash-command parser: parse_bash_command and its helpers, the binary lookup tables, and ENV_ASSIGN_RE. - classifier/slash_triads.rs (327) — slash-triad + task-notification row detection: detect_slash_triads, is_task_notification, and row helpers. - classifier.rs (1094) — activity classifier, rule tables, shared types, tests. Pure code-movement: all moved item bodies are byte-identical, and the item set is unchanged (138 items before and after). build_re becomes pub(super); the public surface is preserved via re-exports so crate::reader::classifier::<name> paths and reader.rs re-exports keep resolving. Verified: 138/138 item-set parity, byte-identity on representative functions, warning-free build, clippy clean, 832 SDK tests pass, live summary/hotspots OK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
resolve_token_counter and join_nonempty were byte-identical copies in both the codex and opencode readers. Move them to reader/user_turn.rs (pub(crate)): resolve_token_counter sits beside the UserTurnTokenizer/HeuristicCounter types it maps between, and join_nonempty serves combined user-turn text assembly — both readers' only callers. Per util.rs's convention, reader-only helpers stay in the reader rather than the cross-module util module. The other reader helpers the survey flagged (to_usage, merge_usage_coverage, build_*_fidelity, pick_target, measure_*_tool_output) are genuinely per-harness (different tool-name conventions and coverage/usage types) and are deliberately left as-is — unifying them would be a leaky abstraction. Pure move: bodies byte-identical, all reader tests pass (codex 47, opencode 55, user_turn 28), clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
parse_codex_buffer was an 885-line streaming state machine threading ~25 mutable locals plus a parallel committed_* shadow set through one giant payload-type match. Decompose it without changing behavior: - CodexParseState owns the 25 working fields; per-event handler methods (handle_session_meta / turn_context / compacted / event_msg / response_item) hold each arm's logic. The driver loop is now a clean 5-arm dispatch (170 lines, was 885). - CommittedSnapshot captures the transaction (commit-on-boundary) mechanism as an explicit type. snapshot() is invoked only from handle_event_msg's task_complete arm — the sole commit site — and the result is still assembled entirely from committed state (finalized_count / user_turns_count / end_offset filters), so trailing partial lines are still discarded. Pure behavior-preserving extract: handler bodies are byte-identical to the original arms modulo var->self.var, continue->return (loop body ends at the match, so equivalent), and the commit-block extraction. Verified: 47 codex tests + full SDK suite pass, clippy clean, warning-free build, and an independent golden diff (summary/sessions/hotspots byte-identical over 60 sessions / 350 real codex turns), corroborated by a line-by-line adversarial equivalence review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
query_verbs/summary.rs was the largest SDK file (2056 lines), mixing the public report API with its private computation engine. Split into a summary/ module: - summary/mod.rs (970) — the public surface: option/report types and impls, the LedgerHandle dispatchers (summary / summary_report / summary_timeseries), the free-function verbs, and the public fidelity-to-value helpers. - summary/compute.rs (1125) — the private compute engine: query building, filtering, tag/model aggregation, by-tool cost attribution, and relationship matching. Re-exported via `pub(crate) use compute::*` so existing crate::query_verbs::summary::<name> paths (incl. tests.rs) keep resolving. Pure code-movement: 101/101 item-set parity (none dropped or duplicated), moved bodies byte-identical, 832 SDK tests pass, clippy clean, live summary OK. query_verbs/mod.rs: pruned imports the engine took with it (use-block only). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
commands/summary.rs was the largest CLI file (1515 lines), bundling three presenter concerns. Split into a summary/ module: - summary/mod.rs (611) — orchestration: SummaryArgs, run, run_inner (dispatch + flag-exclusivity validation), arg parsing, ingest, and the test block. - summary/json.rs (170) — JSON serialization (emit_json, grouped_json_value, the *_to_json helpers). - summary/human.rs (778) — human/table rendering (emit_human, all render_*, format_*, coverage cells). Public surface (SummaryArgs, run) unchanged at crate::commands::summary::*. Pure code-movement: 47/47 item-set parity, moved bodies byte-identical, all CLI tests pass, clippy clean, live summary/--by-tool OK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirror the commands/summary split so the two largest CLI commands share one structure. commands/hotspots.rs (1236 lines) → hotspots/ module: - hotspots/mod.rs (294) — orchestration: HotspotsArgs, RankBy, run, run_inner, pattern-selection, and the test block. - hotspots/json.rs (280) — JSON serialization (the *_to_json helpers). - hotspots/human.rs (687) — human/table rendering (emit_human, findings/section tables, *_row, sort_*, format_bytes); internal helpers kept private. Public surface (HotspotsArgs, RankBy, run) unchanged. Pure code-movement: 43/43 item-set parity, moved bodies byte-identical, CLI tests pass, clippy clean, live hotspots OK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
run_incremental was a 531-line streaming parser threading ~25 mutable locals (plus evidence) through one match-on-line_type loop — the Claude analogue of the codex god-function. Decompose it without changing behavior: - ClaudeParseState owns the working fields + evidence; a constructor reproduces the setup (incl. the start_offset>0 prescan wiring and the resume_marker_offset sentinel derivation) verbatim. Per-line handlers handle_assistant / handle_user / handle_system hold each arm's logic. The driver is now setup + early return + a 3-way dispatch loop + the unchanged post-loop assembly (332 lines, was 531). Pure behavior-preserving extract: handler bodies are byte-identical to the original arms modulo var->self.var and parameter threading; the loop preamble and entire post-loop are byte-identical. The early-return path builds evidence inline (a clone became a move — new_evidence is a pure constructor, so unobservable). Verified: 103 claude tests + full SDK suite pass, clippy clean, warning-free build, an independent golden diff (summary/by-tool/sessions/hotspots byte-identical over 58 sessions / 3567 real Claude turns), and a line-by-line adversarial equivalence review covering the resume/early-return paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…l.rs claude.rs was the largest SDK source file (2134 lines). Extract the incremental-parse engine — ClaudeParseState + its handlers, run_incremental, prescan_nodes, record_root_incremental, PrescanOutput — into a new claude/incremental.rs submodule, leaving the root as the public API + parse-primitive helpers. claude.rs drops to 1345; incremental.rs is 844. Pure code-movement: the moved block is byte-identical (only run_incremental gained pub(super)); root helpers the engine uses were widened to pub(in crate::reader::claude) (tightest scope, warning-free). Gives the reader trio the same shape (each reader is a directory; the engine lives in its own submodule). 103 claude tests pass, full SDK green, clippy clean, live OK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirror the claude split: extract the codex incremental-parse engine — Pending<T>, CodexParseState + its handlers, CommittedSnapshot, parse_codex_buffer — out of the oversized codex.rs (1801) into codex/incremental.rs, leaving the root as the public API + parse-primitive helpers. codex.rs drops to 831; incremental.rs is 1014. Pure code-movement: the moved block is byte-identical (only parse_codex_buffer gained pub(super)); the engine's root dependencies were widened to pub(in crate::reader::codex) and now-unused root imports pruned. All three readers now share one shape — each is a directory with its engine in incremental.rs. 47 codex tests pass, full SDK green, clippy clean, live OK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BREAKING: the published relayburn-sdk surface re-exported ~44 low-level analyze-layer items (detector/aggregator fns like detect_patterns, attribute_hotspots, compute_quality, the aggregate_by_* family; helper types like PricingTable, ComputeQualityOptions, OverheadFile, ProviderRule, SessionTotals) that no embedder used — the verb layer (LedgerHandle methods / summary_report / hotspots / compare) is the intended API. Flip them to pub(crate) and drop them from lib.rs, removing the confusing HotspotsOptions-vs-HotspotsOptions collision aliases (AnalyzeHotspotsOptions/ AnalyzeHotspotsResult) the leak forced. The verb layer reaches analyze via `use crate::analyze::`, not the lib re-export, so intra-crate plumbing is unaffected. Items that appear in a kept-public signature (ModelCost/ReasoningMode, OneShotMetrics/SessionOutcome, SpanEvent/ SpanStatus, …) stayed public — the build is the ground truth. Also pruned a few now-unreachable dead helpers. 44 public names removed from lib.rs. Full workspace (SDK + CLI + node bindings) builds clean, all tests pass, clippy clean. CLI/MCP/@relayburn/sdk unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…analyze compare layer
The CLI compare command reimplemented the SDK verb inline (a CLAUDE.md layering
violation: presenters should wrap verbs). Rewire the non-bucket path onto
LedgerHandle::compare(): the three renderers (json/csv/tty) now consume the
verb's CompareResult/CompareCellResult instead of building the low-level
analyze CompareTable, and analyzed-turns/fidelity come from the verb. The bucket
path already used the verb (compare_timeseries) and is untouched.
With the CLI off the low-level layer, de-publicize analyze::compare
(CompareOptions/CompareTable/CompareCell/CompareCategory/CompareTotals/
build_compare_table) plus the now-internal load_pricing/provider_for/
has_minimum_fidelity/ProviderFilter; delete the dead compare_from_archive path.
BREAKING (numeric): the verb's round_digits used (n*scale).round()/scale, which
diverged from the {:.N}/toFixed rounding the renderers (and the rest of the
codebase) use. Switched to format!-based rounding so the verb's pre-rounded
cells match the display contract; tie values shift by one in the last digit.
Verified behaviorally: burn compare json/csv/human/fidelity output is
byte-identical over a mixed 4-model / 3998-turn corpus; the bucketed JSON is
equivalent modulo the rounding fix and a pre-existing HashMap key-ordering
non-determinism in the fidelity block. Full workspace (SDK + CLI + node) builds
clean, all tests pass, clippy clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FidelitySummary stored by_class / by_granularity / missing_coverage as HashMap, which serde serializes in randomized per-process order — so the `fidelity` block of `summary`/`compare` JSON changed run-to-run (breaking diffing, caching, and snapshot tests; surfaced while golden-verifying the compare rewire). Switch the three maps to BTreeMap and derive Ord on FidelityClass/UsageGranularity so keys emit in a stable order (fidelity-class declaration order; missing-coverage alphabetical). Output is now reproducible. Map .insert/.get call sites are unchanged (BTreeMap shares the API). Full workspace builds clean, all tests pass, clippy clean; bucketed compare JSON is now byte-stable across runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Holistic-review follow-up to the public-API tightening: - TurnProvider was still publicly re-exported but orphaned — it's only produced by provider_for/provider_for_with_rules (both pub(crate)) and appears in no public signature or field, so a caller could name it but never obtain one. Flip the struct to pub(crate) and drop the dead re-export. Completes the analyze-internals de-publicization. - CostBreakdown::model's doc-comment intra-linked to `sum_costs`, which became pub(crate) in the same change set (rustdoc warned about a public->private doc link). Demote it to a plain code span. Full workspace builds warning-free, all tests pass, clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ckage changelogs The compare canonical-rounding and fidelity-summary key-order changes flow through to the published npm packages, but only the root CHANGELOG had entries. Add concise [Unreleased] notes to @relayburn/sdk, @relayburn/mcp, and relayburn (the CLI wrapper) describing the practical output effect. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR moves command business logic from the CLI layer into SDK query verbs, adds Rust implementations of ChangesSDK pipeline ownership & CLI presenter refactor
Reader parser modularization
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 100, 200, 0.5)
Note over CLI,SDK: burn summary / burn hotspots flow (new)
end
participant CLI as CLI run_inner
participant Handle as LedgerHandle (SDK)
participant Compute as query_verbs/summary/compute.rs
participant Analyze as analyze layer (pub(crate))
participant Ledger as Ledger/SQL
CLI->>Handle: summary_report(SummaryReportOptions)
Handle->>Compute: build_summary_report_query(opts)
Handle->>Ledger: query_enriched_turns(query)
Ledger-->>Handle: Vec<EnrichedTurn>
Handle->>Compute: filter_summary_enriched_turns(turns, agent, provider)
Handle->>Analyze: summarize_fidelity, compute_quality, summarize_replacement_savings
Analyze-->>Handle: FidelitySummary (BTreeMap, stable order), QualityResult
Handle->>Compute: compute_summary_by_tool_report / aggregate_by_model
Compute->>Ledger: load user turns for by-tool attribution
Compute-->>Handle: SummaryByToolReport
Handle-->>CLI: SummaryReport variant
CLI->>CLI: emit_json or emit_human (thin presenter)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
crates/relayburn-sdk/src/analyze/provider.rs (1)
27-27: 🧹 Nitpick | 🔵 TrivialNarrow
provider_forandprovider_for_model_with_rulestopub(crate)to match documented intent.
TurnProvideris crate-private, and lib.rs explicitly documents thatprovider_for/provider_for_model_with_rulesshould bepub(crate)internals, not publicly re-exported. However, these functions are still declaredpub fnin provider.rs. Narrow them to align with this design decision.♻️ Proposed visibility cleanup
-pub fn provider_for(turn: &TurnRecord) -> TurnProvider { +pub(crate) fn provider_for(turn: &TurnRecord) -> TurnProvider { provider_for_with_rules(turn, default_rules()) } @@ -pub fn provider_for_model_with_rules( +pub(crate) fn provider_for_model_with_rules( model: &str, source: Option<SourceKind>, rules: &[ProviderRule], ) -> TurnProvider {Also applies to: 139-151
🤖 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/analyze/provider.rs` at line 27, The functions `provider_for` and `provider_for_model_with_rules` in provider.rs are currently declared as `pub fn` but should be narrowed to `pub(crate) fn` to align with the crate-private design of `TurnProvider` and the documented intent in lib.rs. Change the visibility modifier from `pub` to `pub(crate)` for both the `provider_for` function and the `provider_for_model_with_rules` function to ensure they are not publicly re-exported.
🤖 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: The BREAKING entry at line 7 in CHANGELOG.md is too verbose and
implementation-focused with excessive details about specific function and type
names that were removed. Shorten this entry to a single concise, impact-first
sentence that clearly states: the Rust SDK no longer re-exports low-level
analyze-layer internals, and users should migrate to using the verb layer
(LedgerHandle methods/summary_report/hotspots/compare). Remove the detailed
lists of removed functions and types, and eliminate the implementation
backstory, keeping only what directly impacts users and how they should adapt
their code.
In `@crates/relayburn-cli/src/commands/hotspots/mod.rs`:
- Around line 81-82: Update the documentation comment at lines 81-82 in the
hotspots module to replace the deprecated term "waste-pattern detectors" with
the new "hotspots" terminology. Ensure the help text refers to hotspots
detectors instead of waste-pattern detectors to maintain consistency with the
current API naming conventions.
In `@crates/relayburn-cli/src/commands/summary/json.rs`:
- Around line 63-73: The json! macro on line 70 is treating the unquoted
identifier label_key as a literal field name instead of using the variable
value. The variable label_key (defined from report.group_by.wire_str() at line
56) should be used as a dynamic key in the JSON object. Fix this by wrapping the
label_key variable in parentheses in the json! macro so it evaluates the
variable and uses its string value as the actual field name, rather than
creating a literal "label_key" field. This will ensure non-tag grouped reports
emit the correct dynamic keys like "model" or "provider" instead of a hardcoded
"label_key" field.
In `@crates/relayburn-sdk/src/query_verbs/summary/compute.rs`:
- Around line 154-161: The resolve_summary_agent_session_tree function is using
Query::default() which loses source filtering context when querying
relationships, allowing unrelated sessions to be included. Modify the function
signature to accept a query parameter (likely a Query reference), replace the
Query::default() call in ledger.query_relationships() with this parameter, and
then update the two call sites in
crates/relayburn-sdk/src/query_verbs/summary/mod.rs (at the original lines 592
and 656) to pass the source-scoped query variable (q) instead of letting the
function create a default query.
In `@crates/relayburn-sdk/src/reader/classifier/bash_parse.rs`:
- Around line 116-122: The env command handler in the block where binary equals
"env" loses quoted argument boundaries when calling env_args.join(" "), causing
arguments like "npm test" to be reparsed as separate tokens. Instead of joining
the env_args vector into a string and recursively calling
parse_bash_command_inner with a reparsed string, either refactor
parse_bash_command_inner to accept the token vector directly (avoiding
reparsing), or re-quote tokens in env_args that contain whitespace before
joining them to preserve the token boundaries during reparsing.
In `@crates/relayburn-sdk/src/reader/claude/incremental.rs`:
- Around line 268-278: The prescan_nodes function call rebuilds nodes_by_uuid
but fails to carry forward the slash-triad state needed for resumed parses,
causing assistant messages with parent chains pointing to pre-offset slash
triads to miss the Skill override. Modify prescan_nodes to return or populate a
compact slash-triad state containing only the triad row and skill-UUID
information (not full user row data via obj.clone()). After prescan_nodes
completes when start_offset is greater than zero, capture this compact triad
state and feed it into the detect_slash_triads function before processing the
current pass, ensuring the slash-triad detector has the complete context from
both the prescan phase and the current incremental parse. Apply this fix in all
locations where prescan_nodes is called and detect_slash_triads is subsequently
invoked, including the sections around lines 385-391 and 673-686.
---
Nitpick comments:
In `@crates/relayburn-sdk/src/analyze/provider.rs`:
- Line 27: The functions `provider_for` and `provider_for_model_with_rules` in
provider.rs are currently declared as `pub fn` but should be narrowed to
`pub(crate) fn` to align with the crate-private design of `TurnProvider` and the
documented intent in lib.rs. Change the visibility modifier from `pub` to
`pub(crate)` for both the `provider_for` function and the
`provider_for_model_with_rules` function to ensure they are not publicly
re-exported.
🪄 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: c2f9b466-80a1-43e1-b68f-1b9615a09cd5
📒 Files selected for processing (47)
CHANGELOG.mdcrates/relayburn-cli/src/commands/compare.rscrates/relayburn-cli/src/commands/hotspots/human.rscrates/relayburn-cli/src/commands/hotspots/json.rscrates/relayburn-cli/src/commands/hotspots/mod.rscrates/relayburn-cli/src/commands/summary.rscrates/relayburn-cli/src/commands/summary/human.rscrates/relayburn-cli/src/commands/summary/json.rscrates/relayburn-cli/src/commands/summary/mod.rscrates/relayburn-sdk/src/analyze.rscrates/relayburn-sdk/src/analyze/claude_md.rscrates/relayburn-sdk/src/analyze/compare.rscrates/relayburn-sdk/src/analyze/context_delta.rscrates/relayburn-sdk/src/analyze/cost.rscrates/relayburn-sdk/src/analyze/fidelity.rscrates/relayburn-sdk/src/analyze/findings.rscrates/relayburn-sdk/src/analyze/flow_graph.rscrates/relayburn-sdk/src/analyze/hotspots.rscrates/relayburn-sdk/src/analyze/overhead.rscrates/relayburn-sdk/src/analyze/patterns.rscrates/relayburn-sdk/src/analyze/pricing.rscrates/relayburn-sdk/src/analyze/provider.rscrates/relayburn-sdk/src/analyze/provider_reattribution.rscrates/relayburn-sdk/src/analyze/quality.rscrates/relayburn-sdk/src/analyze/replacement_savings.rscrates/relayburn-sdk/src/analyze/subagent_tree.rscrates/relayburn-sdk/src/analyze/tool_call_patterns.rscrates/relayburn-sdk/src/analyze/tool_output_bloat.rscrates/relayburn-sdk/src/lib.rscrates/relayburn-sdk/src/query_verbs/compare.rscrates/relayburn-sdk/src/query_verbs/mod.rscrates/relayburn-sdk/src/query_verbs/summary.rscrates/relayburn-sdk/src/query_verbs/summary/compute.rscrates/relayburn-sdk/src/query_verbs/summary/mod.rscrates/relayburn-sdk/src/reader/classifier.rscrates/relayburn-sdk/src/reader/classifier/bash_parse.rscrates/relayburn-sdk/src/reader/classifier/slash_triads.rscrates/relayburn-sdk/src/reader/claude.rscrates/relayburn-sdk/src/reader/claude/incremental.rscrates/relayburn-sdk/src/reader/codex.rscrates/relayburn-sdk/src/reader/codex/incremental.rscrates/relayburn-sdk/src/reader/opencode.rscrates/relayburn-sdk/src/reader/types.rscrates/relayburn-sdk/src/reader/user_turn.rspackages/mcp/CHANGELOG.mdpackages/relayburn/CHANGELOG.mdpackages/sdk-node/CHANGELOG.md
💤 Files with no reviewable changes (2)
- crates/relayburn-cli/src/commands/summary.rs
- crates/relayburn-sdk/src/query_verbs/summary.rs
Comment on lines
+63
to
+73
| let mut row = if report.group_by == SummaryGroupBy::Tag { | ||
| json!({ | ||
| "tag": report.tag_key.as_deref().unwrap_or(""), | ||
| "value": report.tag_values.get(idx).cloned().flatten(), | ||
| }) | ||
| } else { | ||
| json!({ | ||
| label_key: r.label, | ||
| }) | ||
| }; | ||
| let obj = row.as_object_mut().unwrap(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n crates/relayburn-cli/src/commands/summary/json.rs | head -100Repository: AgentWorkforce/burn
Length of output: 3949
🏁 Script executed:
# Verify the behavior of json!() macro with variable keys by checking how label_key is used
cat -n crates/relayburn-cli/src/commands/summary/json.rs | sed -n '50,90p'Repository: AgentWorkforce/burn
Length of output: 1728
🏁 Script executed:
# Check if there are any references to old terminology (waste, diagnose, context) in this file
rg -i "waste|diagnose|context" crates/relayburn-cli/src/commands/summary/json.rsRepository: AgentWorkforce/burn
Length of output: 45
🏁 Script executed:
# Verify what wire_str() returns by checking the SummaryGroupBy enum
rg -A 15 "enum SummaryGroupBy" --type rustRepository: AgentWorkforce/burn
Length of output: 1216
🏁 Script executed:
# Check the serde_json::json! macro documentation behavior with dynamic keys
# Let's look for similar usage patterns in the codebase to see how dynamic keys are handled elsewhere
rg -B 2 -A 2 "Map::new()" crates/relayburn-cli/src/commands/summary/json.rsRepository: AgentWorkforce/burn
Length of output: 163
🏁 Script executed:
# Let's verify the complete context around this function to understand the full scope
cat -n crates/relayburn-cli/src/commands/summary/json.rs | sed -n '51,130p'Repository: AgentWorkforce/burn
Length of output: 3407
Use a true dynamic key for grouped row labels.
Line 70 currently emits a literal "label_key" field, not "model"/"provider", breaking the JSON schema for non-tag grouped reports. The variable label_key (defined line 56 as report.group_by.wire_str()) contains the intended key string but the json! macro treats unquoted identifiers as literal keys.
Suggested fix
.map(|(idx, r)| {
let mut row = if report.group_by == SummaryGroupBy::Tag {
json!({
"tag": report.tag_key.as_deref().unwrap_or(""),
"value": report.tag_values.get(idx).cloned().flatten(),
})
} else {
- json!({
- label_key: r.label,
- })
+ let mut obj = Map::new();
+ obj.insert(label_key.to_string(), json!(r.label));
+ Value::Object(obj)
};
let obj = row.as_object_mut().unwrap();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut row = if report.group_by == SummaryGroupBy::Tag { | |
| json!({ | |
| "tag": report.tag_key.as_deref().unwrap_or(""), | |
| "value": report.tag_values.get(idx).cloned().flatten(), | |
| }) | |
| } else { | |
| json!({ | |
| label_key: r.label, | |
| }) | |
| }; | |
| let obj = row.as_object_mut().unwrap(); | |
| let mut row = if report.group_by == SummaryGroupBy::Tag { | |
| json!({ | |
| "tag": report.tag_key.as_deref().unwrap_or(""), | |
| "value": report.tag_values.get(idx).cloned().flatten(), | |
| }) | |
| } else { | |
| let mut obj = Map::new(); | |
| obj.insert(label_key.to_string(), json!(r.label)); | |
| Value::Object(obj) | |
| }; | |
| let obj = row.as_object_mut().unwrap(); |
🤖 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/json.rs` around lines 63 - 73, The
json! macro on line 70 is treating the unquoted identifier label_key as a
literal field name instead of using the variable value. The variable label_key
(defined from report.group_by.wire_str() at line 56) should be used as a dynamic
key in the JSON object. Fix this by wrapping the label_key variable in
parentheses in the json! macro so it evaluates the variable and uses its string
value as the actual field name, rather than creating a literal "label_key"
field. This will ensure non-tag grouped reports emit the correct dynamic keys
like "model" or "provider" instead of a hardcoded "label_key" field.
Comment on lines
+154
to
+161
| pub(crate) fn resolve_summary_agent_session_tree( | ||
| ledger: &crate::ledger::Ledger, | ||
| agent_id: &str, | ||
| ) -> Result<HashSet<String>> { | ||
| Ok(collect_summary_agent_session_tree( | ||
| &ledger.query_relationships(&Query::default())?, | ||
| agent_id, | ||
| )) |
There was a problem hiding this comment.
Keep agent-session expansion source-scoped.
Line 159 uses Query::default(), which drops source filtering and can pull relationship rows from other sources, admitting unrelated sessions into agent-filtered summaries.
Suggested fix
pub(crate) fn resolve_summary_agent_session_tree(
ledger: &crate::ledger::Ledger,
+ q: &Query,
agent_id: &str,
) -> Result<HashSet<String>> {
Ok(collect_summary_agent_session_tree(
- &ledger.query_relationships(&Query::default())?,
+ &ledger.query_relationships(&Query {
+ source: q.source,
+ ..Default::default()
+ })?,
agent_id,
))
}Also update callers in crates/relayburn-sdk/src/query_verbs/summary/mod.rs (Line 592 and Line 656) to pass &q.
🤖 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/summary/compute.rs` around lines 154 -
161, The resolve_summary_agent_session_tree function is using Query::default()
which loses source filtering context when querying relationships, allowing
unrelated sessions to be included. Modify the function signature to accept a
query parameter (likely a Query reference), replace the Query::default() call in
ledger.query_relationships() with this parameter, and then update the two call
sites in crates/relayburn-sdk/src/query_verbs/summary/mod.rs (at the original
lines 592 and 656) to pass the source-scoped query variable (q) instead of
letting the function create a default query.
Comment on lines
+116
to
+122
| if binary == "env" { | ||
| let env_args = env_command_args(&tokens, i + 1); | ||
| if env_args.is_empty() { | ||
| return None; | ||
| } | ||
| return parse_bash_command_inner(&env_args.join(" "), depth + 1); | ||
| } |
There was a problem hiding this comment.
Preserve token boundaries when unwrapping env.
env_args.join(" ") loses quoted arguments, so env bash -lc "npm test" is reparsed as bash -lc npm test and normalizes to npm instead of npm test. Recurse over the remaining token vector directly, or re-quote tokens containing whitespace before reparsing.
🤖 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/reader/classifier/bash_parse.rs` around lines 116 -
122, The env command handler in the block where binary equals "env" loses quoted
argument boundaries when calling env_args.join(" "), causing arguments like "npm
test" to be reparsed as separate tokens. Instead of joining the env_args vector
into a string and recursively calling parse_bash_command_inner with a reparsed
string, either refactor parse_bash_command_inner to accept the token vector
directly (avoiding reparsing), or re-quote tokens in env_args that contain
whitespace before joining them to preserve the token boundaries during
reparsing.
Comment on lines
+268
to
+278
| if start_offset > 0 { | ||
| let pre = prescan_nodes( | ||
| path, | ||
| start_offset, | ||
| &mut nodes_by_uuid, | ||
| &mut evidence, | ||
| &mut tool_result_counters, | ||
| )?; | ||
| last_assistant_message_id = pre.last_assistant_message_id; | ||
| next_event_index = pre.next_event_index; | ||
| } |
There was a problem hiding this comment.
Carry compact slash-triad state through the resume prescan.
On resumed parses, prescan_nodes rebuilds nodes_by_uuid but does not seed the slash-triad input, so an assistant emitted after start_offset whose parent chain points to a pre-offset slash triad will not get the Skill override. Also, obj.clone() stores full user rows despite the detector needing only UUID/parent/purpose markers. Carry a compact triad row/skill-UUID state from prescan and append compact current-pass rows before detect_slash_triads.
Also applies to: 385-391, 673-686
🤖 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/reader/claude/incremental.rs` around lines 268 -
278, The prescan_nodes function call rebuilds nodes_by_uuid but fails to carry
forward the slash-triad state needed for resumed parses, causing assistant
messages with parent chains pointing to pre-offset slash triads to miss the
Skill override. Modify prescan_nodes to return or populate a compact slash-triad
state containing only the triad row and skill-UUID information (not full user
row data via obj.clone()). After prescan_nodes completes when start_offset is
greater than zero, capture this compact triad state and feed it into the
detect_slash_triads function before processing the current pass, ensuring the
slash-triad detector has the complete context from both the prescan phase and
the current incremental parse. Apply this fix in all locations where
prescan_nodes is called and detect_slash_triads is subsequently invoked,
including the sections around lines 385-391 and 673-686.
… changelog CodeRabbit review follow-up (the two valid nits; the other findings were a verified false positive plus two pre-existing items left for a separate PR): - `burn hotspots --patterns` help said "waste-pattern detectors", reintroducing the deprecated `waste` term; rename to "hotspot-pattern detectors" per the repo's hotspots terminology rule. - Trim the verbose BREAKING SDK changelog entry to an impact-first summary (keeps a couple representative removed types + the verb-layer migration path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
14 commits: behavior-preserving structural refactors plus a breaking tightening of the published SDK surface. Breaking → major version (4.0.0).
Behavior-preserving (verified equivalent)
parse_codex_buffer(885→170 lines) and clauderun_incremental(531→332) into*ParseStatestructs + per-event handlers. Each verified with an independent golden diff over thousands of real sessions (codex: 350 turns; claude: 3567 turns, 4 output dimensions, byte-identical) plus a line-by-line adversarial equivalence review.classifier.rs→ rule-tables /bash_parse/slash_triads; the two largest CLI commands (summary,hotspots) →args/json/humansubmodules.user_turn.rs(left the per-harness ones — unifying them would be leaky).query_verbs/summary.rs(2056) along an API/engine boundary; moved the claude & codex parse engines into<reader>/incremental.rsso all three readers share one shape.All moves verified by item-set parity + byte-identity spot-checks + full workspace tests.
Breaking changes (the 4.0.0 driver)
relayburn-sdk: stopped re-exporting ~50 low-levelanalyze-layer internals (detectors/aggregators + helper types likePricingTable,CompareTable,CompareCell). The verb layer (LedgerHandlemethods /summary_report/hotspots/compare) is the sole intended public surface. TheAnalyzeHotspotsOptions/AnalyzeCompareOptionscollision aliases are gone. Only the Rust SDK API breaks — the in-repo embedders (CLI, node bindings) never used these.comparerewired onto the verb (fixed the one CLAUDE.md layering violation), enabling the compare-layer de-publicization. Output byte-identical over a 4-model / 3998-turn corpus.@relayburn/sdkoutput, not their APIs): compare costs now use canonical decimal rounding (shift by 1 in the last digit at ties);fidelityJSON keys now emit in a stable, deterministic order (fixes a pre-existingHashMapnon-determinism bug surfaced during review).Verification
clippy --workspace+fmtclean.Release notes
[Unreleased]entries are ready in the rootCHANGELOG.mdand all three package changelogs (@relayburn/sdk,@relayburn/mcp,relayburn). README and CLAUDE.md were checked and remain accurate. Publish via the Publish Packages workflow withversion: major(lockstep → 4.0.0).🤖 Generated with Claude Code