Skip to content

Conversation

@ruskaruma
Copy link
Contributor

Summary

This PR adds an optional data: dict[str, Any] | None = None parameter to lifecycle hooks (on_agent_start, on_tool_start, on_tool_end, on_start) so we can pass extra contextual information (like turn_input and 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:

  • An optional data parameter as the last argument to hook methods in RunHooksBase and AgentHooksBase
  • Passing turn_input via the data dict in on_agent_start / on_start
  • Passing tool arguments via the data dict in on_tool_start / on_tool_end
  • A small helper using inspect.signature() so data is only passed to hooks that explicitly accept it (this keeps backward compatibility intact)
  • Moved _call_hook_with_data into lifecycle.py to avoid a circular import between run.py and _run_impl.py

Test Plan

  • I added a new test test_hooks_receive_turn_input_and_arguments() to verify that hooks receive turn_input and tool arguments through the data payload
  • I ran the full test suite locally: 1106 passed, 3 skipped (the skipped ones are expected Redis tests that require a real Redis server)
  • Verified backward compatibility: existing hooks that don't define a data parameter continue to work unchanged
  • All existing hook-related tests pass without modification

Issue

Closes #2180

Checks

  • I've added new tests (if relevant)
  • I've added/updated the relevant documentation
  • I've run make lint and make format
  • I've made sure tests pass

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a 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".

Comment on lines 38 to 42
async def on_agent_start(
self,
context: RunContextWrapper[TContext],
agent: TAgent,
data: dict[str, Any] | None = None,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

@seratch seratch marked this pull request as draft December 19, 2025 06:16
@seratch
Copy link
Member

seratch commented Dec 19, 2025

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.

@ruskaruma
Copy link
Contributor Author

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:

  1. Should I keep the current method signatures completely unchanged and add a new optional parameter (e.g., hook_data: Optional[TypedDict] = None)?

  2. What should the structure of the typed dict look like? For example:

    • For on_agent_start: {"turn_input": list[TResponseInputItem]}
    • For on_tool_start: {"arguments": str}
  3. Should hooks be able to accept either:

    • The current signature (backward compatible)
    • The new signature with the typed dict
    • Or both at the same time?
  4. Should I look at the JavaScript implementation (Tool calling with LiteLLM + claude thinking models fails #765) as a reference for the pattern?

I want to make sure I implement exactly what you have in mind before proceeding so let me know.

@ruskaruma
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add more arguments to agent/run hooks

2 participants