feat(agent): add add_hook convenience method for hook callback registration#1706
feat(agent): add add_hook convenience method for hook callback registration#1706
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Assessment: Comment This PR adds a useful convenience method for hook registration that improves the developer experience. The implementation is functional and well-documented. Review Categories
The feature addresses a real need per issue #1686 and the type inference capability is a nice touch. Consider the API consistency feedback before merging. |
0198c55 to
5b8a264
Compare
|
/strands review the current pull request and add the @ overload changes proposed in the review |
Implementation of Review FeedbackI've implemented the Changes Made1.
2.
Verification
Diff# src/strands/hooks/registry.py
+from typing import (..., overload, ...)
+ @overload
+ def add_callback(self, callback: HookCallback[TEvent]) -> None: ...
+
+ @overload
+ def add_callback(self, event_type: type[TEvent], callback: HookCallback[TEvent]) -> None: ...
# src/strands/agent/agent.py
+from typing import (..., overload, ...)
+ @overload
+ def add_hook(self, callback: HookCallback[TEvent]) -> None: ...
+
+ @overload
+ def add_hook(self, callback: HookCallback[TEvent], event_type: type[TEvent]) -> None: ...Note: I don't have push access to apply these changes directly. The PR author will need to apply them. |
|
/strands review |
|
Assessment: Approve The Review Status
The API design with type inference is a nice convenience feature. The |
f552d46 to
0842479
Compare
|
Assessment: Approve ✅ All review feedback has been addressed in the latest commit. The PR is ready to merge. Final Review Summary
The implementation is clean, well-tested, and provides a nice convenience API for hook registration with type inference support. |
|
Assessment: Approve ✅ The latest commit significantly simplifies the implementation by removing the complex union type from Review Summary
Note: PR includes unrelated MCP version bump (1.11.0 → 1.23.0) in pyproject.toml. Consider splitting if this causes issues. The implementation is now much cleaner and addresses the method overloading concerns raised by other reviewers. |
Motivation
Currently, registering a hook requires accessing the internal hooks registry:
This exposes implementation details and is less discoverable for users. Plugin authors and users who want to add hooks directly need a cleaner, more intuitive API.
Resolves #1686
Public API Changes
The Agent class now has an
add_hookmethod that provides a cleaner API for registering hook callbacks:The
add_callbackmethod onHookRegistryalso supports type inference:When
event_typeis not provided, it's inferred from the callback's first parameter type hint. AValueErroris raised with a helpful message if inference fails.Naming Justification
I went back and forth between
add_hookandadd_callback. I decided to go withadd_hook, as I think its the most representative name (callback is generic, buthookspecifically means running code at a specific lifecycle event).As for helping users who may not be familiar with hooks, I added a link to our docs to help them understand this concept.