feat: add interactivity, wire hooks and tool health into bare.rs - #32
Conversation
|
Warning Review limit reached
More reviews will be available in 44 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds interactivity-aware hooks and per-tool health tracking to BareLoop (feature-gated): introduces Interactivity and HookAction helpers, extends HookExecutor, wires optional hook_executor and health_registry into BareLoop, and integrates hooks at compaction, tool dispatch, and session lifecycle points with health recording. ChangesHook-based lifecycle interception and tool health tracking
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
🤖 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 `@src/engine/bare.rs`:
- Around line 1154-1160: PreCompactContext and PostCompactContext are being
populated with placeholder zeros (tokens_before, tokens_after, duration_ms);
update the compaction code that builds these contexts (the PreCompactContext and
PostCompactContext construction sites) to supply real values: compute
tokens_before and tokens_after using the same tokenizer/counting utility used
elsewhere in this module (or expose an Option to represent unknown), and compute
duration_ms by capturing Instant::now() before compaction and subtracting it
after to produce a millisecond duration; ensure you set
PostCompactContext.tokens_after and PostCompactContext.duration_ms (and remove
literal 0s) so hook consumers receive real metrics.
- Around line 1918-1933: SessionEndContext is being filled with zeroed totals
and SessionEndReason is inferred via fragile substring checks; instead thread a
structured end-metadata value from the run/abort paths into this code and use it
to populate SessionEndContext fields (total_turns, total_tokens, duration_secs)
and reason. Update the exit paths that currently call into the block (where
run/abort produce errors) to produce and pass a typed EndMetadata/SessionEndInfo
(including reason as an enum, counts, and duration), replace the
error.is_some_and substring logic with a direct match on that enum
(SessionEndReason) and set ctx from the provided metadata. Adjust any
callers/signatures that construct the SessionEndContext so they accept the
structured metadata rather than inferring from error text.
🪄 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: 17d257ae-2a23-4bcb-a431-6e6538f46342
📒 Files selected for processing (3)
src/engine/bare.rssrc/hooks.rssrc/hooks/executor.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/engine/bare.rs (2)
1168-1196:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
abort_sessiondiscards accumulated token counts.When the session ends due to cancellation or max-turns exceeded, the
budgetcontaining accumulated tokens from previous turns is available at the call sites (lines 911–923) but not passed to this function. Hook consumers receivingSessionEndContextwill seetotal_tokens: 0even if turns were completed.Consider threading
&SessionBudgetthroughabort_sessionto provide accurate token totals.🔧 Proposed fix
fn abort_session( &self, turn_count: usize, session_duration: Duration, reason: AbortReason, + budget: &SessionBudget, ) -> Result<SessionResult, AgentError> { let reason_str = match &reason { AbortReason::Cancelled => "Cancelled", AbortReason::MaxTurnsExceeded => "Max turns exceeded", }; self.emit_session_stop(turn_count, session_duration, false, reason_str); let end_reason = match &reason { AbortReason::Cancelled => EndReason::Cancelled, AbortReason::MaxTurnsExceeded => EndReason::MaxTurns, }; self.notify_session_end(&SessionEndInfo { success: false, reason: end_reason, total_turns: turn_count, - total_tokens: 0, + total_tokens: budget.input_tokens.saturating_add(budget.output_tokens), duration_secs: session_duration.as_secs(), });Update call sites:
if self.is_cancelled() { return self.abort_session( budget.turn_count, start.elapsed(), AbortReason::Cancelled, + &budget, ); } if budget.turn_count >= max_turns { return self.abort_session( budget.turn_count, start.elapsed(), AbortReason::MaxTurnsExceeded, + &budget, ); }🤖 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 `@src/engine/bare.rs` around lines 1168 - 1196, The abort_session implementation drops accumulated token counts by always setting total_tokens: 0; change abort_session to accept a &SessionBudget (or &SessionBudget) parameter, update its signature and all callers (the places that call abort_session from the session loop) to pass the current SessionBudget, and use the budget's total token value when constructing SessionEndInfo.total_tokens (and any other places that report token usage, e.g., emit_session_stop if it should reflect tokens). Ensure the AbortReason handling and returned AgentError behavior remain identical while plumbing SessionBudget through.
2268-2269:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
durationcfg attribute does not account forhooksfeature usage.
notify_post_tool_use_hooks(line 1707) readstool_result.durationwhen thehooksfeature is enabled. The current attribute expects dead code only whentool_healthis disabled, but ifhooksis enabled (withouttool_health), the field is used and theexpect(dead_code)lint will be unsatisfied.🔧 Proposed fix
- #[cfg_attr(not(feature = "tool_health"), expect(dead_code))] + #[cfg_attr(not(any(feature = "tool_health", feature = "hooks")), expect(dead_code))] duration: Duration,🤖 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 `@src/engine/bare.rs` around lines 2268 - 2269, The duration field is currently annotated with #[cfg_attr(not(feature = "tool_health"), expect(dead_code))] but notify_post_tool_use_hooks (which reads tool_result.duration) is enabled when the hooks feature is on, so the dead_code expectation must only apply when neither tool_health nor hooks is enabled; update the cfg_attr on the duration field to require both not(feature = "tool_health") and not(feature = "hooks") before applying expect(dead_code) so tool_result.duration is considered used when hooks is enabled (refer to the duration field and notify_post_tool_use_hooks/tool_result.duration to locate the change).
🤖 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.
Outside diff comments:
In `@src/engine/bare.rs`:
- Around line 1168-1196: The abort_session implementation drops accumulated
token counts by always setting total_tokens: 0; change abort_session to accept a
&SessionBudget (or &SessionBudget) parameter, update its signature and all
callers (the places that call abort_session from the session loop) to pass the
current SessionBudget, and use the budget's total token value when constructing
SessionEndInfo.total_tokens (and any other places that report token usage, e.g.,
emit_session_stop if it should reflect tokens). Ensure the AbortReason handling
and returned AgentError behavior remain identical while plumbing SessionBudget
through.
- Around line 2268-2269: The duration field is currently annotated with
#[cfg_attr(not(feature = "tool_health"), expect(dead_code))] but
notify_post_tool_use_hooks (which reads tool_result.duration) is enabled when
the hooks feature is on, so the dead_code expectation must only apply when
neither tool_health nor hooks is enabled; update the cfg_attr on the duration
field to require both not(feature = "tool_health") and not(feature = "hooks")
before applying expect(dead_code) so tool_result.duration is considered used
when hooks is enabled (refer to the duration field and
notify_post_tool_use_hooks/tool_result.duration to locate the change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3305e3ac-3068-48e6-b555-e692d4f4f9c4
📒 Files selected for processing (1)
src/engine/bare.rs
feat: add interactivity, wire hooks and tool health into bare.rs
1 participant
No description provided.