feat: add pre/post call hooks to ToolSimulator (#167)#168
feat: add pre/post call hooks to ToolSimulator (#167)#168kaghatim wants to merge 1 commit intostrands-agents:mainfrom
Conversation
Add optional pre_call_hook and post_call_hook parameters to ToolSimulator for fault injection and response modification during tool simulation. - pre_call_hook: intercepts before LLM call, can short-circuit with a custom response (e.g., rate limit errors, timeouts) - post_call_hook: modifies LLM-generated response before caching (e.g., inject metadata, corrupt fields for robustness testing) Hooks receive typed dataclass events (PreCallHookEvent, PostCallHookEvent) following the Strands SDK hook pattern for extensibility. Closes strands-agents#167
cfdcc34 to
2314f7f
Compare
|
|
||
| def _call_tool(self, registered_tool: RegisteredTool, parameters_string: str, state_key: str) -> dict[str, Any]: | ||
| """Simulate a tool invocation and return the response.""" | ||
| """Simulate a tool invocation and return the response. |
There was a problem hiding this comment.
This implementation reminds me of decorator pattern, which also applies changes pre and post a decorated function. Wondering if it might be better to refactor this with decorator pattern?
def _apply_pre_call_hook(self, func: Callable) -> Callable:
"""Internal decorator that applies pre_call_hook logic."""
if self._pre_call_hook is None:
return func
@wraps(func)
def wrapper(registered_tool: RegisteredTool, parameters_string: str, state_key: str) -> dict[str, Any]:
parameters = json.loads(parameters_string)
current_state = self.state_registry.get_state(state_key)
# Invoke pre-call hook
event = PreCallHookEvent(
tool_name=registered_tool.name,
parameters=parameters,
state_key=state_key,
previous_calls=current_state.get("previous_calls", []),
)
hook_response = self._pre_call_hook(event)
# Short-circuit if hook returns a response
if hook_response is not None:
if not isinstance(hook_response, dict):
raise TypeError(f"pre_call_hook must return a dict or None, got {type(hook_response).__name__}")
self.state_registry.cache_tool_call(
registered_tool.name, state_key, hook_response, parameters=parameters
)
return hook_response
# Otherwise, proceed with normal execution
return func(registered_tool, parameters_string, state_key)
return wrapper
def _apply_post_call_hook(self, func: Callable) -> Callable:
"""Internal decorator that applies post_call_hook logic."""
if self._post_call_hook is None:
return func
@wraps(func)
def wrapper(registered_tool: RegisteredTool, parameters_string: str, state_key: str) -> dict[str, Any]:
# Execute the function to get response
response_data = func(registered_tool, parameters_string, state_key)
# Apply post-call hook to modify response
parameters = json.loads(parameters_string)
event = PostCallHookEvent(
tool_name=registered_tool.name,
parameters=parameters,
state_key=state_key,
response=response_data,
)
modified_response = self._post_call_hook(event)
if not isinstance(modified_response, dict):
raise TypeError(f"post_call_hook must return a dict, got {type(modified_response).__name__}")
return modified_response
return wrapper
......
def _create_tool_wrapper
......
def wrapper(*args, **kwargs):
# Apply decorators in reverse order (post first, then pre)
# This ensures execution order is: pre -> core logic -> post
call_func = self._call_tool
call_func = self._apply_post_call_hook(call_func)
call_func = self._apply_pre_call_hook(call_func)
......
return call_func
I think the pros are a) self._call_tool is not touched, b) more modularized, c) it seems to me that the pre/post hooks need access to many members of TS. Instead of relying on Event classes we might rather give them full access to self. Though I also worry this seems an overkill instead of just modifying _call_tool. @poshinchen thoughts?
There was a problem hiding this comment.
Yeah the reason I went with hooks over subclassing was specifically because _call_tool is private. I didn't want to couple to its internals or commit its signature as public API. The event classes keep the contract narrow so _call_tool can be refactored freely. But I'm open to a different approach if you have a preference. I would be happy to rework it.
Edit: Good point on the decorator pattern. I think the inline approach is simpler here, it avoids re-parsing parameters_string across decorators and keeps the flow in one place. Happy to wait for @poshinchen's take.
| tool_name: Name of the tool that was called. | ||
| parameters: Parsed parameters for the tool call. | ||
| state_key: Key for the state (tool_name or share_state_id). | ||
| response: The LLM-generated response dict, which the hook may modify. |
There was a problem hiding this comment.
Also just some food for thoughts:
- What if we only add 1 new
class CallHookEvent, just fetch response after it's cached intoprevios_calls[-1], and update it if post hook is applied? - What if we just throw the entire TS self to the hooks?
My worry is these event types are ad-hoc and bonded to TS changes. Say if in the future we want access to more TS internal members and states, one has to touch these types again.
There was a problem hiding this comment.
On one class with previous_calls[-1]: right now the post-hook runs before caching so it returns a modified response and that's what gets stored. Caching first and having the hook mutate previous_calls[-1] in place would work, but it makes the hook a side-effect function instead of a simple transform. I went with the return-a-value pattern to keep hooks easier to test and reason about.
On passing self: I kept the events narrow to avoid coupling hooks to TS internals, but I hear the concern about the types growing over time. Happy to rework if that's the preference.
|
Thanks for creating this PR @kaghatim! Have you considered taking the Plugin approach? I think that would be cleaner. You would pass in the plugin here: https://github.com/kaghatim/evals/blob/2314f7f66d6b33c0bb72b8301b65296a310a07fd/src/strands_evals/simulation/tool_simulator.py#L243-L250 and you would do: simulator = ToolSimulator(
model=model,
plugin=my_hook_plugin, <=== new
)See https://strandsagents.com/docs/user-guide/concepts/plugins/ |
Description
Add optional
pre_call_hookandpost_call_hookparameters toToolSimulator.__init__for fault injection and response modification during tool simulation.pre_call_hook: invoked before the LLM generates a response. Receives aPreCallHookEventdataclass. Returns a dict to short-circuit the LLM call, orNoneto proceed normally. Short-circuited responses are cached in the state registry.post_call_hook: invoked after the LLM generates a response but before caching. Receives aPostCallHookEventdataclass. Returns a (possibly modified) response dict.Hook events follow the Strands SDK pattern (
@dataclassevents, before/after separation) and live intypes/simulation/hook_events.py.Both hooks are optional and default to
None, making this fully backward compatible.Related Issues
Closes #167
Documentation PR
N/A - No new docs needed for this change. Hook usage is documented in the
__init__docstring.Type of Change
New feature
Testing
hatch test tests/strands_evals/simulation/test_tool_simulator.pyhatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.