-
Notifications
You must be signed in to change notification settings - Fork 611
Expose input messages to BeforeInvocationEvent #1007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add incoming messages to BeforeInvocationEvent so this hook can be used for guardrails aiming to check (and perhaps redact) user inputs before kicking off the event loop.
23f6080 to
3d72200
Compare
mkmeral
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, and I'm sorry for the delayed response.
The feature and PR makes a lot of sense, I just had 2 small comments.
| self.hooks.invoke_callbacks(BeforeInvocationEvent(agent=self)) | ||
| if not self.messages and not prompt: | ||
| raise ValueError("No conversation history or prompt provided") | ||
| temp_messages: Messages = self.messages + self._convert_prompt_to_messages(prompt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just send the new messages? I think that would allow devs to differentiate between new messages (what we pass), and agent.messages (what already exists in history).
| with self.tracer.tracer.start_as_current_span( | ||
| "execute_structured_output", kind=trace_api.SpanKind.CLIENT | ||
| "execute_structured_output", | ||
| attributes={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to update the span?
|
Apologies for the delay on this issue, but we have made some significant changes since this was submitted. We need to rebase this pr in order to fit with the current state of the codebase. |
|
Hey @athewsey! 👋 I noticed this PR has merge conflicts. The Since this is a valuable feature (tagged as The feature makes a lot of sense for implementing early guardrails on user input before it gets added to memory. I saw @mkmeral's feedback from Jan 8th - looks like the PR just needs:
Would love to see this land! 🚀 Comment from strands-coder autonomous agent 🦆 |
cagataycali
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @athewsey! 👏 This PR addresses a real need for input-side guardrails - being able to inspect and potentially redact user input before it enters the agent flow.
🔍 Review Summary
What I Like
- Clean implementation that passes
messagestoBeforeInvocationEvent - The structured_output code reorganization is sensible - moving validation before the tracing span
- Comprehensive test updates
- Good backward compatibility considerations
Implementation Notes
Breaking Change Consideration
This is technically a breaking change since BeforeInvocationEvent now requires the messages parameter. Any existing code that manually creates BeforeInvocationEvent(agent=agent) will fail.
Consider making messages optional with a default:
@dataclass
class BeforeInvocationEvent(HookEvent):
"""Event emitted at the start of agent operations."""
messages: Messages = field(default_factory=list) # Optional with default empty listOr update the docstring to clearly indicate this is now required.
Span Attributes
I notice you moved the span attributes from set_attributes() to the start_as_current_span() call. This is cleaner, but just confirming this produces identical OTEL output?
Documentation
As you mentioned in the PR description, docs update needed. The use case for input guardrails is compelling - would be great to see an example in the hooks documentation.
Suggested Enhancement (Optional)
For guardrail implementations, it might be helpful to also include a stop_invocation() method or flag on the event:
@dataclass
class BeforeInvocationEvent(HookEvent):
messages: Messages
_abort_invocation: bool = field(default=False, init=False)
def abort(self) -> None:
self._abort_invocation = TrueThis would let hooks cleanly abort without raising exceptions. But this could be a follow-up!
Tests
✅ Tests look comprehensive - good coverage of both regular agent calls and structured_output flows.
Verdict
LGTM with minor consideration for the breaking change aspect. The core implementation is solid.
🦆
Review by Strands-Coder
|
Closing as this is already implemented: #1474 |
Description
Add the incoming
messagestoBeforeInvocationEvent, so that this hook can be used for implementing guardrails that check (and perhaps redact) user inputs before kicking off the event loop or raisingMessageAddedEvents.Related Issues
#1006
Documentation PR
None yet - Should be some small updates required if we go ahead. I can work on this if reviewers are OK with the change in principle?
Type of Change
New feature
Testing
Updated existing unit tests to validate the messages are passed through correctly (both in structured output and normal agent invocation).
hatch 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.