feat(hooks): accept callable hook callbacks in Agent constructor #1992
feat(hooks): accept callable hook callbacks in Agent constructor #1992agent-of-mkmeral wants to merge 4 commits intostrands-agents:mainfrom
Conversation
|
/strands review |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@agent-of-mkmeral did you not run |
|
Assessment: ✅ Approve This PR addresses a legitimate usability gap by enabling the Review Summary
Well done on the clear PR description with before/after examples! |
|
Fixed! Ran Commit: Issues found & fixed
Verification
|
There was a problem hiding this comment.
Can this be added to the existing agent test file?
Also this is a lot of testing code for a simple addition. Can we trim this down?
There was a problem hiding this comment.
Done! Deleted test_agent_hooks_callable.py entirely and moved 5 trimmed-down essential tests into the existing test_agent_hooks.py:
test_hooks_param_accepts_callable— plain callable via hooks paramtest_hooks_param_accepts_mixed_list— HookProvider + callable mixtest_hooks_param_accepts_tuple— Sequence support (new)test_hooks_param_invalid_hook_raises_error— error pathtest_hooks_param_callable_invoked_during_lifecycle— fires during agent call
Removed the other 7 tests that were redundant (None/empty list already work by default, async/lambda/multiple were over-testing).
12 tests → 5 tests. All 29 tests in the file pass ✅
| assert events_received[0].agent is agent | ||
|
|
||
| def test_hooks_param_accepts_hook_provider(self): | ||
| """Verify that HookProvider still works as before (backward compatibility).""" |
There was a problem hiding this comment.
This is duplicated, lets remove it
There was a problem hiding this comment.
Removed! This was a duplicate of test_agent__init__hooks which already verifies HookProvider works via the hooks= param. The whole file is deleted now — tests consolidated into test_agent_hooks.py.
|
@agent-of-mkmeral check the latest review, update as needed |
|
Addressed all 3 review threads from @Unshure! Commit: Changes
Verification
|
|
/strands review |
|
Assessment: ✅ Approve Re-reviewed after updates. All reviewer feedback has been addressed appropriately. Changes Since Last Review
Final State
Ready to merge. |
…s parameter
Previously, the hooks parameter in Agent.__init__ only accepted
HookProvider instances. This change allows passing plain callable
hook callbacks (functions with typed event parameters) directly,
matching the flexibility of Agent.add_hook().
The hooks param now accepts a mixed list of HookProvider instances
and/or callable hook callbacks:
def on_start(event: BeforeInvocationEvent) -> None:
print('Starting!')
agent = Agent(hooks=[on_start, MyHookProvider()])
This provides a more convenient API for simple hook use cases where
creating a full HookProvider class is unnecessary.
Changes:
- Updated hooks param type: list[HookProvider | HookCallback] | None
- Added isinstance check to dispatch HookProviders vs callables
- Added ValueError for invalid hook types
- Added comprehensive tests (12 test cases)
- Remove unused variable assignments (F841 lint errors) - Apply auto-formatting from hatch run format
Review feedback from @Unshure: 1. hooks param now accepts Sequence (list, tuple) instead of just list - Changed type hint to Sequence[HookProvider | HookCallback] - Added Sequence import from collections.abc 2. Moved tests to existing test_agent_hooks.py and trimmed down - Removed separate test_agent_hooks_callable.py file - Kept 5 essential tests: callable, mixed list, tuple, invalid, lifecycle - Removed duplicate HookProvider test (already covered by test_agent__init__hooks) - Removed redundant tests (None, empty list, async, lambda, multiple) 3. Removed duplicated test_hooks_param_accepts_hook_provider - This was already covered by existing test_agent__init__hooks
Per @mkmeral's feedback: keep hooks as list[...] for devx simplicity. If users want explicit tuple/Sequence control, they can use add_hook(). Changes: - Revert Sequence -> list in hooks type hint - Revert docstring to not mention tuple support - Remove Sequence import (no longer needed) - Remove test_hooks_param_accepts_tuple test
00e9b85 to
3127130
Compare
|
Rebased onto latest The Post-rebase verification
CI should go green now. Ready for merge when you're happy with it @mkmeral. |
Motivation
Currently, the
hooksparameter inAgent.__init__only acceptsHookProviderinstances:However,
Agent.add_hook()(added in #1706) already supports plain callables:This inconsistency is confusing. If you can register a callable via
add_hook(), you should also be able to pass it via the constructor.Changes
src/strands/agent/agent.py:hooksparam type fromlist[HookProvider]tolist[HookProvider | HookCallback]HookProvider→add_hook(), callable →add_callback(None, hook)ValueErrorfor invalid hook types with clear error messageAfter This Change
Callables must have typed event parameters (same requirement as
agent.add_hook()). Lambdas without type hints will raiseValueErrorwith a helpful message.Tests
Added 12 new test cases in
tests/strands/agent/test_agent_hooks_callable.py:All 24 existing hook tests + 61 hook registry tests continue to pass.
Related
add_hookconvenience method)cc @mkmeral