feat!: implement HookData - #140
Conversation
|
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 (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe SDK adds ChangesHook data support
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR adds per-hook state sharing across hook stages with documented examples and no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The implementation satisfies issue Full details: Docstring CoverageExplanation Docstring coverage is 61.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 2 files. (1 skipped: 1 unsupported.)
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
🧹 Nitpick comments (3)
src/api/client.rs (1)
332-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming the closures to avoid shadowing the runner methods.
before_hooksandafter_hooksare also method names onClient(lines 403 and 434). Line 352 readsself.before_hooks(before_hooks(), ...), where the two identifiers mean different things. The code compiles because method-call syntax and local bindings resolve separately. Names such ashooks_in_before_orderandhooks_in_after_orderremove the ambiguity.♻️ Proposed rename
- // INFO: (hook, its data) in before order: API(global), Client, Invocation, Provider - let before_hooks = || ordered_hooks.iter().copied().zip(hook_data.iter()); - - // INFO: Hooks called after the resolution are in reverse order - // Provider, Invocation, Client, API(global) - let after_hooks = || before_hooks().rev(); + // INFO: (hook, its data) in before order: API(global), Client, Invocation, Provider + let hooks_in_before_order = || ordered_hooks.iter().copied().zip(hook_data.iter()); + + // INFO: Hooks called after the resolution are in reverse order + // Provider, Invocation, Client, API(global) + let hooks_in_after_order = || hooks_in_before_order().rev();Update the four call sites at lines 352, 361, 364, 380, 384, 392, and 397 accordingly.
🤖 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/api/client.rs` around lines 332 - 349, Rename the local closures before_hooks and after_hooks to unambiguous names such as hooks_in_before_order and hooks_in_after_order, then update every closure invocation in the surrounding hook evaluation flow while leaving the Client::before_hooks and Client::after_hooks method calls unchanged.README.md (1)
244-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winState that hook data is recreated for each evaluation.
The prose describes isolation between hooks. It does not state isolation between evaluations. The client creates a new
HookDataper hook per evaluation, andsrc/hooks/mod.rsverifies this inhook_data_not_shared_between_evaluations. Readers who register one hook instance on a client need this fact.📝 Proposed wording
A hook can carry state across its own stages within a single evaluation via `context.data`. Each hook instance gets its own `HookData`, shared across that hook's `before`, `after`, `error`, and `finally` stages and isolated from every other hook. This is the recommended place to stash state such as a start timestamp or a telemetry span opened in `before` and consumed in `after`/`finally`. +A fresh `HookData` is created for every evaluation, so a hook instance reused across evaluations never +observes state from a previous evaluation.🤖 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 `@README.md` around lines 244 - 247, Update the hook state documentation around context.data to explicitly state that each hook instance receives a newly created HookData for every evaluation, while retaining the existing isolation across hook instances and sharing across that evaluation’s stages.src/hooks/data.rs (1)
140-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a test for the poison-recovery path.
The tests do not exercise the
PoisonError::into_innerbranch inlock. A panic inside awith_mutclosure poisons the mutex. A test proves the store stays usable afterwards.🧪 Proposed test
#[test] fn store_remains_usable_after_panic_in_with_mut() { let data = HookData::new(); data.set("key", 1_i64); let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { data.with_mut::<i64, _>("key", |_| panic!("boom")); })); assert!(result.is_err()); // The mutex is poisoned, but `lock` recovers the guard. assert_eq!(data.get::<i64>("key"), Some(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 `@src/hooks/data.rs` around lines 140 - 292, Add a test in the `tests` module covering mutex poison recovery: invoke `with_mut` inside `std::panic::catch_unwind` with an `AssertUnwindSafe` closure that panics, assert the panic is captured, then verify `get` still returns the stored value through the recovered lock.
🤖 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/hooks/mod.rs`:
- Around line 1191-1226: Update the isolation test around IsolationHook to track
each hook’s after invocation count, using the same guard pattern as
hook_data_not_shared_between_evaluations. Assert that both hooks ran before
checking violations.is_empty(), so the test cannot pass when hooks are never
invoked.
- Around line 100-112: Document the public HookContext struct-literal breaking
change near its definition, explicitly instructing downstream users to add the
new data field and initialize it with HookData::default().
---
Nitpick comments:
In `@README.md`:
- Around line 244-247: Update the hook state documentation around context.data
to explicitly state that each hook instance receives a newly created HookData
for every evaluation, while retaining the existing isolation across hook
instances and sharing across that evaluation’s stages.
In `@src/api/client.rs`:
- Around line 332-349: Rename the local closures before_hooks and after_hooks to
unambiguous names such as hooks_in_before_order and hooks_in_after_order, then
update every closure invocation in the surrounding hook evaluation flow while
leaving the Client::before_hooks and Client::after_hooks method calls unchanged.
In `@src/hooks/data.rs`:
- Around line 140-292: Add a test in the `tests` module covering mutex poison
recovery: invoke `with_mut` inside `std::panic::catch_unwind` with an
`AssertUnwindSafe` closure that panics, assert the panic is captured, then
verify `get` still returns the stored value through the recovered lock.
🪄 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: 99a03dba-52d0-45a3-883b-93fb2f9de8c9
📒 Files selected for processing (5)
README.mdexamples/hook_data.rssrc/api/client.rssrc/hooks/data.rssrc/hooks/mod.rs
Adds a per-hook, per-evaluation HookData store on HookContext, shared across that hook's before/after/error/finally stages and isolated from other hooks. The Hook trait signatures are unchanged, so existing hooks are unaffected. Includes README updates and panic-recovery test coverage. BREAKING CHANGE: `HookContext` gains a public `data` field. Code that constructs `HookContext` directly must add `data: HookData::default()` (HookContext does not implement Default, and HookData is not exposed as a public constant). Signed-off-by: Jose Bovet Derpich <jose.bovet@gmail.com>
1 participant
This PR
HookContext(context.data) that lets a hook carry state across its own stages. e.g. start a timer or open a telemetry span inbeforeand finish it inafter/finallyHookDatawithset/get::<T>/take::<T>/with_mut::<T,_>HookData, shared across that hook'sbefore→after/error→finallystages and isolated from every other hookRelated Issues
Closes #102
Notes
HookContextkeepsPartialEq/Debugvia manual impls that exclude the type-eraseddatafield, so no trait impls are lostFollow-up Tasks
How to test
cargo test --features test-util cargo run --example hook_dataAlso verified:
cargo clippy -- -D warningsandcargo fmt --checkpass.