-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: Add extensible data payload to lifecycle hooks (#2180) #2205
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
base: main
Are you sure you want to change the base?
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/agents/lifecycle.py
Outdated
| async def on_agent_start( | ||
| self, | ||
| context: RunContextWrapper[TContext], | ||
| agent: TAgent, | ||
| data: dict[str, Any] | None = None, |
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.
Keep hook overrides compatible after adding data param
Adding the data argument to the base hook signatures here makes every existing override missing that parameter incompatible under strict mypy. Classes like RunHooksForTests.on_agent_start in tests/test_run_hooks.py and AgentHooksForTests in tests/test_agent_hooks.py still use the old signature, so make mypy now fails with “Signature of ... incompatible with supertype RunHooksBase/AgentHooksBase”, and the same breakage will hit external hook implementations. This contradicts the stated goal of keeping existing hooks working without changes.
Useful? React with 👍 / 👎.
|
Thanks for sharing this idea! However, this is not the thing I was thinking of. My plan is to have two patterns - 1. current set of arguments, 2. a single object (perhaps typed dict) containing all the arguments and bring no breaking changes. The second pattern is flexible enough for future enhancement and we don't need to worry about the order of the arguments. |
|
Hey @seratch. Thanks for the feedback! I'd like to clarify the exact pattern you're looking for and I have a few questions here:
I want to make sure I implement exactly what you have in mind before proceeding so let me know. |
|
and @seratch do let me know if I should either continue working on this or create a new PR with the implementation you've suggested with your plan. |
Summary
This PR adds an optional
data: dict[str, Any] | None = Noneparameter to lifecycle hooks (on_agent_start,on_tool_start,on_tool_end,on_start) so we can pass extra contextual information (liketurn_inputand tool arguments) without breaking existing hook implementations.The intent is to make it possible to extend hook inputs going forward without having to keep changing method signatures.
What I added:
dataparameter as the last argument to hook methods inRunHooksBaseandAgentHooksBaseturn_inputvia thedatadict inon_agent_start/on_startdatadict inon_tool_start/on_tool_endinspect.signature()sodatais only passed to hooks that explicitly accept it (this keeps backward compatibility intact)_call_hook_with_dataintolifecycle.pyto avoid a circular import betweenrun.pyand_run_impl.pyTest Plan
test_hooks_receive_turn_input_and_arguments()to verify that hooks receiveturn_inputand tool arguments through thedatapayloaddataparameter continue to work unchangedIssue
Closes #2180
Checks
make lintandmake format