Hook system: tool interception and session lifecycle#128
Conversation
Add hooks module in reloaded-code-core with game-style tool hooks, HookSet/HookSetBuilder container, and session lifecycle events. - ToolHook trait with ToolOriginal trampoline for chain dispatch - HookSet wraps tool hooks + session start/end/compact events - HookSetBuilder for convenient construction - AgentRuntimeBuilder::hooks() to attach HookSet to runtime - Empty fast path: dispatch_tool calls real tool directly when no hooks - SessionContext, EndReason, event callback type aliases - Full docs (hooks.md) with examples and architecture diagram Also fix clippy::unwrap_used in permissions test helper.
|
Warning Review limit reached
More reviews will be available in 39 minutes and 4 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 Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR introduces a complete hooks system for intercepting tool calls and observing session lifecycle events in the ReloadedCode agent runtime. The implementation spans core type definitions and abstractions, a registry container with dispatch methods, a fluent builder API, integration into AgentRuntime, and comprehensive documentation. Developers can now register tool hooks to inspect, modify, or block tool calls, and attach session lifecycle callbacks for start, end, and compact events. The feature is wired into AgentRuntimeBuilder for straightforward configuration and exposes a clean public API through the reloaded-code-core crate. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🧹 Nitpick comments (2)
src/reloaded-code-core/src/permissions.rs (1)
460-465: ⚡ Quick winPreserve the underlying error in the panic message.
The closure discards the
ExpandErrorvia|_|, losing details about which environment variable lookup failed or why pattern expansion failed. This makes debugging test failures harder.♻️ Proposed fix to preserve error context
- ruleset.push(Rule::new(*perm, *pat, *action).unwrap_or_else(|_| { + ruleset.push(Rule::new(*perm, *pat, *action).unwrap_or_else(|e| { panic!( - "failed to create Rule for permission {:?}, pattern {:?}, action {:?}", - perm, pat, action + "failed to create Rule for permission {:?}, pattern {:?}, action {:?}: {}", + perm, pat, action, e ) }));🤖 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/reloaded-code-core/src/permissions.rs` around lines 460 - 465, The panic currently discards the ExpandError by using `|_|` in the `unwrap_or_else` inside the `ruleset.push(Rule::new(*perm, *pat, *action).unwrap_or_else(|_| { ... }))` call; change the closure to capture the error (e.g. `|err|`) and include that error in the panic message (for example using `{:?}`) so the underlying error from `Rule::new`/pattern expansion is preserved in the log.docs/src/hooks.md (1)
82-84: ⚡ Quick winHarden the
.envguard example for Windows paths.Line 83 only checks
/-style separators, so a copied implementation can miss\.envon Windows paths.Proposed doc snippet adjustment
- if let Some(path) = req.args.get("path").and_then(|v| v.as_str()) { - if path.starts_with(".env") || path.contains("/.env") { + if let Some(path) = req.args.get("path").and_then(|v| v.as_str()) { + let normalized = path.replace('\\', "/"); + if normalized == ".env" + || normalized.starts_with(".env.") + || normalized.contains("/.env") + { return Ok(ToolOutput::new( "Blocked: .env files contain secrets" )); } }🤖 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 `@docs/src/hooks.md` around lines 82 - 84, The current guard around req.args.get("path") only checks Unix-style separators via path.starts_with(".env") and path.contains("/.env"), which misses Windows paths; update the condition to also detect backslash-separated entries (e.g., path.contains("\\.env")) or, better, normalize/parse the path (using Path or splitting on both '/' and '\\') to reliably detect a component named ".env" before returning the ToolOutput::new; adjust the check around req.args.get("path") accordingly so Windows paths are hardened as well.
🤖 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 `@docs/src/hooks.md`:
- Around line 186-200: Remove or use the unused Markdown link reference
definitions causing MD053: either delete the unused reference lines for
`ToolHook`, `ToolOriginal`, `ToolHookFuture`, `ToolExecutor`, `ToolCallContext`,
`ToolRequest`, `ToolOutput`, `SessionContext`, `EndReason`, `SessionStartFn`,
`SessionEndFn`, `SessionCompactFn`, `HookSet`, `HookSetBuilder`, and
`AgentRuntime` from the reference block, or add inline links that reference
these identifiers in the hooks.md content so they are consumed; update the
references consistently (keep only those actually linked from the document) and
ensure no orphaned `[Identifier]: https://...` definitions remain.
In `@src/reloaded-code-agents/src/runtime/builder.rs`:
- Around line 314-318: The test builder_hooks_sets_hook_set currently uses an
empty HookSet so it can't detect if AgentRuntimeBuilder::hooks(...) is ignored;
change the test to create a non-empty HookSet (e.g., add at least one hook when
building via HookSet::builder()) and then after let runtime =
AgentRuntimeBuilder::new().hooks(custom_hooks).build()? assert that
runtime.hooks() contains that hook (or is not empty) to verify hooks are
propagated through the build process.
In `@src/reloaded-code-core/src/hooks/builder.rs`:
- Around line 12-14: The current fixed-capacity ArrayVec fields session_start,
session_end, session_compact can panic when push is called beyond INLINE_CAP;
change these fields to TinyVec<[SessionStartFn; INLINE_CAP]> (and analogous
types for SessionEndFn and SessionCompactFn) and remove the Option wrapper so
you store handlers directly; update the registration methods on_session_start,
on_session_end, on_session_compact to push directly into the TinyVec (and handle
result/overflow if needed), update the HookSet types in
src/reloaded-code-core/src/hooks/hook_set.rs to match the new TinyVec+non-Option
types, and simplify the dispatch logic to iterate over the vectors directly (no
flatten/Option handling).
---
Nitpick comments:
In `@docs/src/hooks.md`:
- Around line 82-84: The current guard around req.args.get("path") only checks
Unix-style separators via path.starts_with(".env") and path.contains("/.env"),
which misses Windows paths; update the condition to also detect
backslash-separated entries (e.g., path.contains("\\.env")) or, better,
normalize/parse the path (using Path or splitting on both '/' and '\\') to
reliably detect a component named ".env" before returning the ToolOutput::new;
adjust the check around req.args.get("path") accordingly so Windows paths are
hardened as well.
In `@src/reloaded-code-core/src/permissions.rs`:
- Around line 460-465: The panic currently discards the ExpandError by using
`|_|` in the `unwrap_or_else` inside the `ruleset.push(Rule::new(*perm, *pat,
*action).unwrap_or_else(|_| { ... }))` call; change the closure to capture the
error (e.g. `|err|`) and include that error in the panic message (for example
using `{:?}`) so the underlying error from `Rule::new`/pattern expansion is
preserved in the log.
🪄 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
Run ID: 3805243f-c23c-455d-a6e5-b8c03d13e92a
📒 Files selected for processing (13)
docs/mkdocs.ymldocs/src/architecture.mddocs/src/hooks.mddocs/src/index.mdsrc/reloaded-code-agents/src/runtime/builder.rssrc/reloaded-code-agents/src/runtime/state.rssrc/reloaded-code-core/src/hooks/builder.rssrc/reloaded-code-core/src/hooks/hook_set.rssrc/reloaded-code-core/src/hooks/mod.rssrc/reloaded-code-core/src/hooks/session.rssrc/reloaded-code-core/src/hooks/tool_hook.rssrc/reloaded-code-core/src/lib.rssrc/reloaded-code-core/src/permissions.rs
Remove 7 unused `[Identifier]:` definitions (ToolExecutor, SessionContext, EndReason, SessionStartFn, SessionEndFn, SessionCompactFn, AgentRuntime) that had no corresponding `[Identifier]` reference in the document body. Fixes MD053 (link-reference definitions should be used).
… on overflow ArrayVec::push panics when exceeding INLINE_CAP (3). TinyVec auto-spills to heap, making overflow safe while preserving inline storage for the common case.
1 participant
What
New hooks module in
reloaded-code-coreGame-style tool hooks + session lifecycle events.
Attached to
AgentRuntimeBuilder.Types
ToolHooktrait - intercepts tool calls, may callToolOriginalto continue, skip to blockHookSet/HookSetBuilder- container for tool hooks + session start/end/compact eventsToolCallContext,ToolRequest,ToolExecutor,HookFuture- supporting typesSessionContext,EndReason- lifecycle event typesAgentRuntimeBuilder::hooks(set)- attach to runtime, zero-cost when emptyDocs
New
hooks.mdwith design rationale, mermaid diagram, example code(HomeExpander, EnvFileGuard). Updated architecture docs, index, nav.
Status
Still work in progress.