feat: add hook data per-hook mutable state#222
Conversation
…(spec 4.1.5, 4.3.8) - Add hook_data_for(hook) to HookContext for per-hook isolated mutable state that persists across all hook stages (spec 4.1.5, 4.6.1) - Add spec tests verifying hook_data isolation between hook instances - Add spec tests verifying finally hook receives evaluation details on both success and error paths (spec 4.3.8) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jose Colella <jose.colella@gusto.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the OpenFeature SDK by implementing a crucial feature for hooks: the ability to maintain mutable state specific to each hook instance throughout its lifecycle. This change ensures that hooks can store and retrieve data across their various stages, improving their utility and adherence to the OpenFeature specification. Accompanying these functional changes are comprehensive tests that validate the correct isolation of this per-hook data and confirm the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism for hooks to maintain mutable state across their lifecycle stages, along with corresponding specification tests. The implementation in HookContext uses hook.object_id to scope data, which could lead to potential data leakage in long-running applications due to object_id reuse. I've suggested a more robust approach using the hook object itself as the key. The new tests are well-written and cover the intended functionality.
| # The same hash is returned across all hook stages (before, after, error, finally), | ||
| # allowing hooks to share state across their lifecycle (spec 4.1.5, 4.6.1). | ||
| def hook_data_for(hook) | ||
| @hook_data[hook.object_id] ||= {} |
There was a problem hiding this comment.
Using hook.object_id as a key in the hash can be problematic. While unlikely in short-lived processes, object_ids can be reused by the Ruby garbage collector for new objects after old ones are destroyed. This could lead to data leakage between different hook instances over the lifetime of a long-running application. A more robust approach is to use the hook object itself as the key. Ruby's Hash handles object keys correctly based on their identity, which is safer and more idiomatic.
@hook_data[hook] ||= {}
Summary
hook_data_for(hook)method toHookContextthat provides per-hook isolated mutable state persisting across all hook stages (before → after → error → finally)finallyhook receives evaluation details on both success and error paths (spec 4.3.8)Spec References
Part of the spec compliance roadmap: #220
Test plan
bundle exec rspec spec/specification/hooks_spec.rb— 22 examples, 0 failuresbundle exec standardrb— no offenses🤖 Jose's AI agent