relayburn-analyze: port hotspots aggregator (#274)#291
Merged
willwashburn merged 2 commits intomainfrom May 5, 2026
Merged
Conversation
Ports `packages/analyze/src/hotspots.ts` into the Rust analyze crate.
`hotspots`:
- Types: `ToolAttribution`, `SessionTotals`, `HotspotsResult`,
`HotspotsOptions`, `AttributionMethod` (`Sized | EvenSplit`,
serializes kebab-case), `FileAggregation`, `BashAggregation`,
`BashVerbAggregation`, `SubagentAggregation`.
- `attribute_hotspots` composes the per-tool attribution loop:
initial cost charged at the *paying* turn's rate using its
`(input + cacheCreate)` mix, sibling group capped at the actual
new-content tokens; persistence allocated proportionally by size
against `cacheRead` with the per-result eviction signal; even-split
fallback when no per-result sizes are available; user-turn block
sizes preferred over content-sidecar estimates.
- `aggregate_by_file` / `aggregate_by_bash` / `aggregate_by_bash_verb`
/ `aggregate_by_subagent` mirror the four CLI rollups; bash-verb
examples sort by `(cost desc, command asc)` and the verb table by
`(cost desc, verb asc)` so output is deterministic across runs.
- Session grand totals route through `cost_for_turn` so source-aware
reasoning semantics (Codex `included_in_output`) flow through and
don't double-bill.
Conformance gate: every test from `hotspots.test.ts` is mirrored as a
Rust test (sized 8k-Read persistence within ±10%, file rank, Bash
argsHash collapse, bash-verb drill-down, subagent rollup, even-split
fallback, user-turn vs sidecar precedence, sibling initial / persistence
caps, paying-turn-rate cross-model, Codex reasoning regression, totals
identity); per-row USD totals stay within 1e-9 of TS.
Drive-by: re-export `UserTurnBlockKind` from `relayburn-reader` so
downstream crates can match on the block kind without reaching into the
private `types` module.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Rust ChangesHotspots Attribution and Aggregation Engine
Sequence DiagramsequenceDiagram
actor Client
participant API as attribute_hotspots
participant SessionMgr as Session Grouping
participant Engine as Attribution Engine
participant Cache as Cache/Persistence Allocator
participant Agg as Aggregators
Client->>API: calls attribute_hotspots(turns[], HotspotsOptions)
API->>SessionMgr: group turns by session_id\nstable-sort by turn_index
SessionMgr->>Engine: session turns, pricing, content maps
loop per session
Engine->>Engine: choose AttributionMethod (Sized/EvenSplit)
loop per turn
Engine->>Engine: emit initial costs for prior-turn tool calls\n(use paying turn model rates)
Engine->>Cache: update riding/pending cached results
Cache->>Engine: allocate persistence/cacheRead costs\n(proportional by size, apply caps/eviction)
end
Engine->>API: session ToolAttribution[] + SessionTotals
end
API->>Agg: pass ToolAttribution[] to aggregators
Agg->>Agg: aggregate_by_file(), aggregate_by_bash(),\naggregate_by_bash_verb(), aggregate_by_subagent()
API->>Client: return HotspotsResult (attributions, session_totals, grand totals)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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: 3
🤖 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 12: Summary: The release note about `relayburn-analyze` was added to the
root CHANGELOG under `[Unreleased]` but should live in the package-scoped
changelog. Fix: remove the `relayburn-analyze` entry (the paragraph beginning
"`relayburn-analyze` (Rust): port the `hotspots` aggregator...") from the root
CHANGELOG.md `[Unreleased]` block and add the same entry to the
relayburn-analyze package's CHANGELOG entry under its own `[Unreleased]` section
so package-scoped changes stay in that package's changelog; keep the exact
public surface names (`attribute_hotspots`, `aggregate_by_file`,
`aggregate_by_bash`, `aggregate_by_bash_verb`, `aggregate_by_subagent`,
`AttributionMethod`, `BashAggregation`, `BashVerbAggregation`,
`FileAggregation`, `HotspotsOptions`, `HotspotsResult`, `SessionTotals`,
`SubagentAggregation`, `ToolAttribution`) and the PR reference `(`#274`)`
unchanged.
In `@crates/relayburn-analyze/src/hotspots.rs`:
- Around line 347-373: The code computes still_cached from riding_active but
never writes it back, so items that should be evicted keep being treated as
active; also eviction must occur even when turn_rate is None. Move the
computation of still_cached out of the rate-only block (or compute it
unconditionally when have_any_sizes && !riding_active.is_empty() &&
turn.usage.cache_read > 0), use it for allocation only if turn_rate/ rate is
Some (preserving the existing cost updates using rate.cache_read, PER_MILLION,
attributions, etc.), and then assign riding_active = still_cached (or replace
riding_active's contents with still_cached) so the filtered set persists across
turns; ensure symbols referenced are still_cached, riding_active,
turn_rate/rate, attributions, PER_MILLION.
- Around line 268-320: The code flips to AttributionMethod::Sized when any size
exists (have_any_sizes) causing unsized tool_results to get zero tokens; change
the decision to use Sized only when every emitted tool result has a size:
compute the total number of emitted tool results (e.g. from turns iteration or
the existing attributions/tool_uses collection) and set method =
AttributionMethod::Sized only if size_by_tool_use_id.len() ==
total_tool_results, otherwise use AttributionMethod::EvenSplit; update any logic
that currently branches on have_any_sizes (and variables like pending_initial,
riding_active, and attributions[].result_tokens) to instead branch on the new
all_sized predicate so unsized results fall back to the even-split 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: f0173b83-5823-43bc-9fc9-889d8d831366
📒 Files selected for processing (4)
CHANGELOG.mdcrates/relayburn-analyze/src/hotspots.rscrates/relayburn-analyze/src/lib.rscrates/relayburn-reader/src/lib.rs
| - `relayburn-analyze` (Rust): port the ghost-surface detector — `ghost_surface` and `ghost_surface_inputs` modules with Claude / Codex / OpenCode adapters, slash-command miners, the per-source-scoped orchestrator, and the `WasteFinding` envelope adapter. Findings sort deterministically by `(cost desc, sizeTokens desc, path)` and dedup against the OpenCode catalog-bloat detector via `countedByCatalogBloat`. (#273) | ||
| - `relayburn-analyze` (Rust): port the `compare` aggregator — `build_compare_table` for the in-memory `(model, activity)` rollup with per-cell turn / edit / one-shot / priced / cost / cache-hit / median-retries metrics, plus `compare_from_archive` sourced from the SQLite ledger via `Ledger::query_turns`. Public surface: `CompareCell`, `CompareTable`, `CompareTotals`, `CompareOptions`, `CompareCategory`, `DEFAULT_MIN_SAMPLE`, `compare_from_archive`, `CompareFromArchiveResult`. (#269) | ||
| - `relayburn-analyze` (Rust): port `subagent_tree` and `claude_md` modules. `build_subagent_tree` / `aggregate_subagent_type_stats` walk per-session subagent invocations (relationship-row substrate with legacy `subagent` fallback) and roll up self/cumulative cost. `parse_claude_md` / `attribute_claude_md` / `build_trim_recommendations` / `render_unified_diff_for_recommendation` produce CLAUDE.md section attribution and trim diffs whose unified-diff format stays byte-aligned with the TS implementation. (#272) | ||
| - `relayburn-analyze` (Rust): port the `hotspots` aggregator — `attribute_hotspots` composes the per-tool sized / even-split attribution loop (paying-turn rate, sibling-cap on initial cost, proportional cacheRead allocation on persistence, source-aware reasoning via `cost_for_turn`) with the `aggregate_by_file` / `aggregate_by_bash` / `aggregate_by_bash_verb` / `aggregate_by_subagent` rollups. Public surface mirrors `@relayburn/analyze`: `attribute_hotspots`, `aggregate_by_file`, `aggregate_by_bash`, `aggregate_by_bash_verb`, `aggregate_by_subagent`, `AttributionMethod`, `BashAggregation`, `BashVerbAggregation`, `FileAggregation`, `HotspotsOptions`, `HotspotsResult`, `SessionTotals`, `SubagentAggregation`, `ToolAttribution`. Per-row USD totals match the TS implementation within 1e-9. (#274) |
There was a problem hiding this comment.
Move this note out of the root changelog.
This change is package-scoped in the provided diff, so keeping it under the root [Unreleased] block breaks the repo’s changelog split.
As per coding guidelines, CHANGELOG.md: Update the root CHANGELOG.md [Unreleased] block only when work spans multiple packages or warrants a top-level summary; single-package work belongs only in that package's CHANGELOG
🧰 Tools
🪛 LanguageTool
[grammar] ~12-~12: Use a hyphen to join words.
Context: ...ttribute_hotspots` composes the per-tool sized / even-split attribution loop (pay...
(QB_NEW_EN_HYPHEN)
🤖 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 `@CHANGELOG.md` at line 12, Summary: The release note about `relayburn-analyze`
was added to the root CHANGELOG under `[Unreleased]` but should live in the
package-scoped changelog. Fix: remove the `relayburn-analyze` entry (the
paragraph beginning "`relayburn-analyze` (Rust): port the `hotspots`
aggregator...") from the root CHANGELOG.md `[Unreleased]` block and add the same
entry to the relayburn-analyze package's CHANGELOG entry under its own
`[Unreleased]` section so package-scoped changes stay in that package's
changelog; keep the exact public surface names (`attribute_hotspots`,
`aggregate_by_file`, `aggregate_by_bash`, `aggregate_by_bash_verb`,
`aggregate_by_subagent`, `AttributionMethod`, `BashAggregation`,
`BashVerbAggregation`, `FileAggregation`, `HotspotsOptions`, `HotspotsResult`,
`SessionTotals`, `SubagentAggregation`, `ToolAttribution`) and the PR reference
`(`#274`)` unchanged.
Member
Author
There was a problem hiding this comment.
Skipping — keeping this consistent with the four sibling relayburn-analyze (Rust) port entries already in [Unreleased] from #269 / #271 / #272 / #273. There is no crates/relayburn-analyze/CHANGELOG.md yet, so the root file is the only place these Rust port milestones currently land. Migrating Rust crate entries to per-crate CHANGELOGs is a separate cleanup that should sweep all five entries together, not fork this one.
There was a problem hiding this comment.
@willwashburn — that context makes sense. Since no crates/relayburn-analyze/CHANGELOG.md exists yet and the four sibling port entries (#269, #271, #272, #273) all live in the root [Unreleased] block, keeping this entry consistent there is the right call. A future cleanup that creates the per-crate changelog and moves all five entries together is the appropriate scope for that change. I'll note this for future reviews.
✏️ Learnings added
Learnt from: willwashburn
Repo: AgentWorkforce/burn PR: 291
File: CHANGELOG.md:12-12
Timestamp: 2026-05-05T13:52:26.990Z
Learning: In the AgentWorkforce/burn repository, there is currently no `crates/relayburn-analyze/CHANGELOG.md`. All Rust crate port milestones for `relayburn-analyze` are intentionally recorded in the root `CHANGELOG.md` under `[Unreleased]` (e.g. `#269`, `#271`, `#272`, `#273`, `#274`). Migrating these entries to a per-crate changelog is a deferred cleanup that should sweep all entries together rather than be done per-PR. Do not flag root-level `relayburn-analyze` changelog entries as misplaced until the per-crate changelog exists.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+268
to
+320
| let have_any_sizes = !size_by_tool_use_id.is_empty(); | ||
| let method = if have_any_sizes { | ||
| AttributionMethod::Sized | ||
| } else { | ||
| AttributionMethod::EvenSplit | ||
| }; | ||
|
|
||
| let mut attributions: Vec<ToolAttribution> = Vec::new(); | ||
| // Indices into `attributions` for tool_uses emitted on the prior turn | ||
| // that haven't been charged initial cost yet. They pay at the next | ||
| // iteration using the *paying* turn's model rate and (input + cacheCreate) | ||
| // mix. | ||
| let mut pending_initial: Vec<usize> = Vec::new(); | ||
| // Indices for results whose initial cost has already been paid; eligible | ||
| // to ride along (persistence) on subsequent turns until the cacheRead | ||
| // eviction signal drops them. | ||
| let mut riding_active: Vec<usize> = Vec::new(); | ||
|
|
||
| for turn in turns { | ||
| let turn_rate = lookup_model_rate(&turn.model, pricing); | ||
|
|
||
| // 1) Initial cost: this turn pays for tool_results emitted on the | ||
| // previous turn. Use THIS turn's rate and (input/cacheCreate) mix | ||
| // — not the emit turn's. | ||
| if !pending_initial.is_empty() { | ||
| if let Some(rate) = turn_rate { | ||
| let new_content = (turn.usage.input | ||
| + turn.usage.cache_create_5m | ||
| + turn.usage.cache_create_1h) as f64; | ||
| if new_content > 0.0 { | ||
| let input_share = turn.usage.input as f64 / new_content; | ||
| let create_share = 1.0 - input_share; | ||
| let per_token_price = input_share * rate.input + create_share * rate.cache_write; | ||
| if have_any_sizes { | ||
| let sibling_total: f64 = pending_initial | ||
| .iter() | ||
| .map(|&i| attributions[i].result_tokens as f64) | ||
| .sum(); | ||
| if sibling_total > 0.0 { | ||
| // Cap at what turn N+1 actually paid for new | ||
| // content — otherwise multiple tool_results | ||
| // entering on the same turn could over-attribute | ||
| // past the actual paid total. | ||
| let cap = sibling_total.min(new_content); | ||
| for &i in &pending_initial { | ||
| let result_tokens_f = attributions[i].result_tokens as f64; | ||
| let tokens = (result_tokens_f / sibling_total) * cap; | ||
| let cost = (tokens / PER_MILLION) * per_token_price; | ||
| attributions[i].initial_cost = cost; | ||
| attributions[i].initial_tokens = tokens; | ||
| attributions[i].total_cost += cost; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t enter sized mode unless every emitted tool result has a size.
Lines 268-320 switch the whole session to Sized as soon as any size is present. In a mixed session, any unsized tool_call keeps result_tokens == 0, so it gets neither proportional initial cost nor persistence cost and never reaches the even-split fallback. That under-attributes totals and breaks the stated TS parity.
Suggested fix
- let have_any_sizes = !size_by_tool_use_id.is_empty();
- let method = if have_any_sizes {
+ let tool_call_ids: Vec<&str> = turns
+ .iter()
+ .flat_map(|turn| turn.tool_calls.iter().map(|tc| tc.id.as_str()))
+ .collect();
+ let have_all_sizes = !tool_call_ids.is_empty()
+ && tool_call_ids
+ .iter()
+ .all(|id| size_by_tool_use_id.contains_key(*id));
+ let method = if have_all_sizes {
AttributionMethod::Sized
} else {
AttributionMethod::EvenSplit
};
@@
- if have_any_sizes {
+ if have_all_sizes {📝 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 have_any_sizes = !size_by_tool_use_id.is_empty(); | |
| let method = if have_any_sizes { | |
| AttributionMethod::Sized | |
| } else { | |
| AttributionMethod::EvenSplit | |
| }; | |
| let mut attributions: Vec<ToolAttribution> = Vec::new(); | |
| // Indices into `attributions` for tool_uses emitted on the prior turn | |
| // that haven't been charged initial cost yet. They pay at the next | |
| // iteration using the *paying* turn's model rate and (input + cacheCreate) | |
| // mix. | |
| let mut pending_initial: Vec<usize> = Vec::new(); | |
| // Indices for results whose initial cost has already been paid; eligible | |
| // to ride along (persistence) on subsequent turns until the cacheRead | |
| // eviction signal drops them. | |
| let mut riding_active: Vec<usize> = Vec::new(); | |
| for turn in turns { | |
| let turn_rate = lookup_model_rate(&turn.model, pricing); | |
| // 1) Initial cost: this turn pays for tool_results emitted on the | |
| // previous turn. Use THIS turn's rate and (input/cacheCreate) mix | |
| // — not the emit turn's. | |
| if !pending_initial.is_empty() { | |
| if let Some(rate) = turn_rate { | |
| let new_content = (turn.usage.input | |
| + turn.usage.cache_create_5m | |
| + turn.usage.cache_create_1h) as f64; | |
| if new_content > 0.0 { | |
| let input_share = turn.usage.input as f64 / new_content; | |
| let create_share = 1.0 - input_share; | |
| let per_token_price = input_share * rate.input + create_share * rate.cache_write; | |
| if have_any_sizes { | |
| let sibling_total: f64 = pending_initial | |
| .iter() | |
| .map(|&i| attributions[i].result_tokens as f64) | |
| .sum(); | |
| if sibling_total > 0.0 { | |
| // Cap at what turn N+1 actually paid for new | |
| // content — otherwise multiple tool_results | |
| // entering on the same turn could over-attribute | |
| // past the actual paid total. | |
| let cap = sibling_total.min(new_content); | |
| for &i in &pending_initial { | |
| let result_tokens_f = attributions[i].result_tokens as f64; | |
| let tokens = (result_tokens_f / sibling_total) * cap; | |
| let cost = (tokens / PER_MILLION) * per_token_price; | |
| attributions[i].initial_cost = cost; | |
| attributions[i].initial_tokens = tokens; | |
| attributions[i].total_cost += cost; | |
| } | |
| } | |
| let tool_call_ids: Vec<&str> = turns | |
| .iter() | |
| .flat_map(|turn| turn.tool_calls.iter().map(|tc| tc.id.as_str())) | |
| .collect(); | |
| let have_all_sizes = !tool_call_ids.is_empty() | |
| && tool_call_ids | |
| .iter() | |
| .all(|id| size_by_tool_use_id.contains_key(*id)); | |
| let method = if have_all_sizes { | |
| AttributionMethod::Sized | |
| } else { | |
| AttributionMethod::EvenSplit | |
| }; | |
| let mut attributions: Vec<ToolAttribution> = Vec::new(); | |
| // Indices into `attributions` for tool_uses emitted on the prior turn | |
| // that haven't been charged initial cost yet. They pay at the next | |
| // iteration using the *paying* turn's model rate and (input + cacheCreate) | |
| // mix. | |
| let mut pending_initial: Vec<usize> = Vec::new(); | |
| // Indices for results whose initial cost has already been paid; eligible | |
| // to ride along (persistence) on subsequent turns until the cacheRead | |
| // eviction signal drops them. | |
| let mut riding_active: Vec<usize> = Vec::new(); | |
| for turn in turns { | |
| let turn_rate = lookup_model_rate(&turn.model, pricing); | |
| // 1) Initial cost: this turn pays for tool_results emitted on the | |
| // previous turn. Use THIS turn's rate and (input/cacheCreate) mix | |
| // — not the emit turn's. | |
| if !pending_initial.is_empty() { | |
| if let Some(rate) = turn_rate { | |
| let new_content = (turn.usage.input | |
| turn.usage.cache_create_5m | |
| turn.usage.cache_create_1h) as f64; | |
| if new_content > 0.0 { | |
| let input_share = turn.usage.input as f64 / new_content; | |
| let create_share = 1.0 - input_share; | |
| let per_token_price = input_share * rate.input + create_share * rate.cache_write; | |
| if have_all_sizes { | |
| let sibling_total: f64 = pending_initial | |
| .iter() | |
| .map(|&i| attributions[i].result_tokens as f64) | |
| .sum(); | |
| if sibling_total > 0.0 { | |
| // Cap at what turn N+1 actually paid for new | |
| // content — otherwise multiple tool_results | |
| // entering on the same turn could over-attribute | |
| // past the actual paid total. | |
| let cap = sibling_total.min(new_content); | |
| for &i in &pending_initial { | |
| let result_tokens_f = attributions[i].result_tokens as f64; | |
| let tokens = (result_tokens_f / sibling_total) * cap; | |
| let cost = (tokens / PER_MILLION) * per_token_price; | |
| attributions[i].initial_cost = cost; | |
| attributions[i].initial_tokens = tokens; | |
| attributions[i].total_cost += cost; | |
| } | |
| } |
🤖 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-analyze/src/hotspots.rs` around lines 268 - 320, The code
flips to AttributionMethod::Sized when any size exists (have_any_sizes) causing
unsized tool_results to get zero tokens; change the decision to use Sized only
when every emitted tool result has a size: compute the total number of emitted
tool results (e.g. from turns iteration or the existing attributions/tool_uses
collection) and set method = AttributionMethod::Sized only if
size_by_tool_use_id.len() == total_tool_results, otherwise use
AttributionMethod::EvenSplit; update any logic that currently branches on
have_any_sizes (and variables like pending_initial, riding_active, and
attributions[].result_tokens) to instead branch on the new all_sized predicate
so unsized results fall back to the even-split behavior.
Member
Author
There was a problem hiding this comment.
Skipping — this mirrors packages/analyze/src/hotspots.ts:166 exactly:
const haveAnySizes = sizeByToolUseId.size > 0;
const method: AttributionMethod = haveAnySizes ? "sized" : "even-split";The PR-stated conformance goal is bit-for-bit parity with the TS implementation within the 1e-9 USD precision contract from #244. Switching the gate to "every emitted tool result has a size" would diverge from TS on any session where some tool_use_ids are missing from both the user-turn tool_result blocks and the content sidecar — fixtures that currently land in Sized mode (with unsized rows contributing resultTokens = 0) would silently drop into EvenSplit, changing every per-row total.
If the underlying semantics are wrong, that needs a coordinated TS + Rust change with an updated test fixture, not a unilateral deviation in the port PR.
There was a problem hiding this comment.
@willwashburn, that's a fair and clear correction — the have_any_sizes gate is an intentional, faithful mirror of the TS haveAnySizes predicate at hotspots.ts:166, and diverging from it here would break the 1e-9 conformance contract this port is gated on. I agree the right venue for any semantic change would be a coordinated TS + Rust update with updated fixtures, not this port PR. Withdrawing the suggestion.
✏️ Learnings added
Learnt from: willwashburn
Repo: AgentWorkforce/burn PR: 291
File: crates/relayburn-analyze/src/hotspots.rs:330-382
Timestamp: 2026-05-05T13:52:31.031Z
Learning: In `crates/relayburn-analyze/src/hotspots.rs`, the `have_any_sizes` predicate (used to select `AttributionMethod::Sized` vs `AttributionMethod::EvenSplit`) intentionally mirrors the TypeScript source at `packages/analyze/src/hotspots.ts:166` (`haveAnySizes = sizeByToolUseId.size > 0`). The conformance contract for all `relayburn-analyze` Rust ports is bit-for-bit parity with the TS implementation within 1e-9 USD precision (established in `#244`). Do NOT suggest changing this gate to "all sizes present" in a port PR — any semantic change must be a coordinated TS + Rust update with updated test fixtures.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+347
to
+373
| if have_any_sizes && !riding_active.is_empty() && turn.usage.cache_read > 0 { | ||
| if let Some(rate) = turn_rate { | ||
| let still_cached: Vec<usize> = riding_active | ||
| .iter() | ||
| .copied() | ||
| .filter(|&i| { | ||
| let rt = attributions[i].result_tokens; | ||
| rt > 0 && turn.usage.cache_read >= rt | ||
| }) | ||
| .collect(); | ||
| if !still_cached.is_empty() { | ||
| let active_total: f64 = still_cached | ||
| .iter() | ||
| .map(|&i| attributions[i].result_tokens as f64) | ||
| .sum(); | ||
| let allocatable = (turn.usage.cache_read as f64).min(active_total); | ||
| for &i in &still_cached { | ||
| let rt = attributions[i].result_tokens as f64; | ||
| let tokens = (rt / active_total) * allocatable; | ||
| let cost = (tokens / PER_MILLION) * rate.cache_read; | ||
| attributions[i].persistence_tokens += tokens; | ||
| attributions[i].persistence_cost += cost; | ||
| attributions[i].total_cost += cost; | ||
| attributions[i].riding_turns += 1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Persist evictions back into riding_active.
Lines 349-356 compute still_cached, but that filtered set is never written back to riding_active. A tool result that drops out on one turn can start accruing persistence again on a later turn with a larger cache_read, and turns with no pricing rate never evict anything because the filter sits inside if let Some(rate). That over-attributes persistence after eviction.
Suggested fix
- if have_any_sizes && !riding_active.is_empty() && turn.usage.cache_read > 0 {
- if let Some(rate) = turn_rate {
- let still_cached: Vec<usize> = riding_active
- .iter()
- .copied()
- .filter(|&i| {
- let rt = attributions[i].result_tokens;
- rt > 0 && turn.usage.cache_read >= rt
- })
- .collect();
- if !still_cached.is_empty() {
+ if have_all_sizes && !riding_active.is_empty() {
+ riding_active.retain(|&i| {
+ let rt = attributions[i].result_tokens;
+ rt > 0 && turn.usage.cache_read >= rt
+ });
+ if turn.usage.cache_read > 0 {
+ if let Some(rate) = turn_rate {
+ if !riding_active.is_empty() {
let active_total: f64 = still_cached
.iter()
.map(|&i| attributions[i].result_tokens as f64)
.sum();
let allocatable = (turn.usage.cache_read as f64).min(active_total);
- for &i in &still_cached {
+ for &i in &riding_active {
let rt = attributions[i].result_tokens as f64;
let tokens = (rt / active_total) * allocatable;
let cost = (tokens / PER_MILLION) * rate.cache_read;
attributions[i].persistence_tokens += tokens;
attributions[i].persistence_cost += cost;
attributions[i].total_cost += cost;
attributions[i].riding_turns += 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-analyze/src/hotspots.rs` around lines 347 - 373, The code
computes still_cached from riding_active but never writes it back, so items that
should be evicted keep being treated as active; also eviction must occur even
when turn_rate is None. Move the computation of still_cached out of the
rate-only block (or compute it unconditionally when have_any_sizes &&
!riding_active.is_empty() && turn.usage.cache_read > 0), use it for allocation
only if turn_rate/ rate is Some (preserving the existing cost updates using
rate.cache_read, PER_MILLION, attributions, etc.), and then assign riding_active
= still_cached (or replace riding_active's contents with still_cached) so the
filtered set persists across turns; ensure symbols referenced are still_cached,
riding_active, turn_rate/rate, attributions, PER_MILLION.
Member
Author
There was a problem hiding this comment.
Skipping — same parity argument. packages/analyze/src/hotspots.ts:232-251 is structurally identical:
if (haveAnySizes && ridingActive.length > 0 && turnRate && turn.usage.cacheRead > 0) {
const stillCached: ToolAttribution[] = [];
for (const a of ridingActive) {
if (a.resultTokens > 0 && turn.usage.cacheRead >= a.resultTokens) {
stillCached.push(a);
}
}
if (stillCached.length > 0) {
// ... allocate over stillCached only
}
}TS computes stillCached per-turn from the persistent ridingActive list and never writes back. The semantics are intentionally "snapshot per-turn": a result that doesn't fit this turn's cacheRead doesn't accrue persistence this turn, but is re-evaluated against next turn's cacheRead. The suggested riding_active.retain(...) flips this to permanent eviction, which would change cost across any session where cache_read dips on one turn and rebounds on a later one.
Same with the if let Some(rate) placement — TS skips the entire persistence block when turnRate is null (line 232 short-circuits on turnRate), so the Rust port's if let Some(rate) = turn_rate is the same behavior, not a divergence.
If the TS behavior is wrong it should be fixed in both ports together with new tests; the Rust port can't unilaterally diverge here without breaking the 1e-9 parity contract this PR gates against.
There was a problem hiding this comment.
@willwashburn, you're right — I missed that the TS implementation uses the same snapshot-per-turn pattern. The riding_active.retain(...) suggestion would have introduced permanent eviction semantics, diverging from the intentional behavior in both implementations and breaking the 1e-9 parity contract. I'll withdraw the comment.
(ʕ •ᴥ•ʔ)?
✏️ Learnings added
Learnt from: willwashburn
Repo: AgentWorkforce/burn PR: 291
File: crates/relayburn-analyze/src/hotspots.rs:409-435
Timestamp: 2026-05-05T13:52:47.604Z
Learning: In `crates/relayburn-analyze/src/hotspots.rs`, the `riding_active` list intentionally uses snapshot-per-turn eviction semantics that mirror `packages/analyze/src/hotspots.ts`. `still_cached` (Rust) / `stillCached` (TS) is computed per-turn from `riding_active` but is never written back — a result that doesn't fit one turn's `cache_read` is re-evaluated against the next turn's `cache_read` rather than being permanently evicted. The `if let Some(rate) = turn_rate` guard also correctly mirrors the TS `turnRate &&` short-circuit, skipping the entire persistence block when no rate is available. Any change to this eviction model must be made in both the Rust and TS implementations together, with new tests, to preserve the 1e-9 parity contract.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
CodeRabbit's docstring-coverage check on #291 came back at 61% (threshold 80%). Adds rustdoc to the public surface in `hotspots.rs`: the four output struct types (`ToolAttribution`, `SessionTotals`, `HotspotsResult`, the four `*Aggregation` rollups), the two-variant `AttributionMethod` enum, and the five public functions (`attribute_hotspots` plus the four `aggregate_by_*` helpers). No behavior change — comments only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 6, 2026
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.
Closes #274.
Summary
Ports
packages/analyze/src/hotspots.tsinto the Rust analyze crate. Composes the per-tool sized / even-split attribution loop with the four CLI rollups (file, bash, bash-verb, subagent) soburn hotspotscan read its full surface fromrelayburn-analyzeonce the CLI port lands.(input + cacheCreate)mix, caps sibling groups at the turn's actual new-content tokens, and allocates persistence proportionally by size againstcacheReadwith the per-result eviction signal. Even-split fallback when no per-result sizes are available.tool_resultblockapproxTokenswin over content-sidecarestimateTokens(UTF-16 code units / 4, matching TSstring.length / 4).Read | Edit | Write | NotebookEdit, sort by cost desc), bash (argsHashcollapse), bash-verb (parser callback, examples sortcost desc, command asc, verbs sortcost desc, verb asc), subagent (Agent | Taskfilter, key onsubagent_type).cost_for_turnso Codexincluded_in_outputreasoning semantics flow through — no double-billing on Codex sessions.Drive-by: re-exports
UserTurnBlockKindfromrelayburn-readerso analyze can match on it without reaching into the privatetypesmodule.Conformance
Every test from
packages/analyze/src/hotspots.test.tsis mirrored as a Rust test:argsHashcollapseavgPersistenceTurns)newContentcacheReadincluded_in_outputreasoning regressionattributedTotal + unattributedTotal == grandTotalidentityPer-row USD totals stay within the 1e-9 USD precision contract called out in #244.
Test plan
cargo test -p relayburn-analyze— 231 tests passing (14 new hotspots tests)cargo build --workspacecleancargo clippy -p relayburn-analyze --tests— no new warnings on hotspots production code (only the pre-existingtoo_many_argumentsshape on test helpers, consistent with the rest of the crate)🤖 Generated with Claude Code