feat(lifecycle): add on_turn_start and on_turn_end hooks to RunHooksBase (#2671)#1
Open
adityasingh2400 wants to merge 4 commits into
Open
feat(lifecycle): add on_turn_start and on_turn_end hooks to RunHooksBase (#2671)#1adityasingh2400 wants to merge 4 commits into
adityasingh2400 wants to merge 4 commits into
Conversation
…ase (openai#2671) Both RunHooksBase and AgentHooksBase get two new hook methods: - on_turn_start(context, agent, turn_number): fires before each LLM call - on_turn_end(context, agent, turn_number): fires after all tool calls for the turn complete (i.e. just before the next-step decision) Turn numbers are 1-indexed and increment each time through the agent loop, regardless of handoffs. The hooks are called in both the sync and streaming code paths. Agent-level hooks on agent.hooks are also called, matching the existing on_tool_start/on_tool_end pattern. Closes openai#2671
📝 WalkthroughWalkthroughAdds a TurnControl type and on_turn_start/on_turn_end async hooks to RunHooksBase and AgentHooksBase; runner and agent turn hooks are invoked concurrently each agent turn, and a hook-returned "stop" halts the run before that turn's LLM call. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(135,206,235,0.5)
participant Runner
end
rect rgba(144,238,144,0.5)
participant Agent
end
rect rgba(255,228,181,0.5)
participant Hooks
end
rect rgba(221,160,221,0.5)
participant LLM/Tools
end
Runner->>Hooks: on_turn_start(context, agent, turn_number)
Runner->>Agent: agent.hooks.on_turn_start(context, agent, turn_number)
Note right of Runner: await asyncio.gather(...)
alt any hook returns "stop"
Hooks-->>Runner: "stop"
Agent-->>Runner: "stop"/None
Runner->>Runner: mark max-turns, emit QueueCompleteSentinel, raise MaxTurnsExceeded
Note right of Runner: halted — no LLM/Tools call for this turn
else all continue
Hooks-->>Runner: None/"continue"
Agent-->>Runner: None/"continue"
Runner->>LLM/Tools: execute turn (LLM/tool calls)
LLM/Tools-->>Runner: turn result
Runner->>Hooks: on_turn_end(context, agent, turn_number)
Runner->>Agent: agent.hooks.on_turn_end(context, agent, turn_number)
Note right of Runner: await asyncio.gather(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_turn_lifecycle_hooks.py`:
- Around line 115-119: The tests define async hook methods on_llm_start and
on_llm_end that append literal strings using unnecessary f-strings
(call_order.append(f"llm_start") and call_order.append(f"llm_end")), which
triggers Ruff F541; remove the f-string prefixes and use plain string literals
(call_order.append("llm_start") and call_order.append("llm_end")) in the
on_llm_start and on_llm_end implementations to satisfy the linter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bdc7731b-45ac-4b18-afdd-c275d8cb7287
📒 Files selected for processing (4)
src/agents/lifecycle.pysrc/agents/run.pysrc/agents/run_internal/run_loop.pytests/test_turn_lifecycle_hooks.py
- Remove unnecessary f-string prefixes from on_llm_start/on_llm_end (Ruff F541) - Add missing docstrings to class methods to improve docstring coverage - Add docstring to OrderTrackingHooks inner class
adityasingh2400
force-pushed
the
feat/turn-lifecycle-hooks-2671-v2
branch
from
ff99d90 to
05a4f15
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agents/run.py`:
- Around line 1107-1116: The on_turn_end hook is being emitted prematurely when
run_single_turn yields NextStepInterruption; update the logic around the await
asyncio.gather call that invokes hooks.on_turn_end and
current_agent.hooks.on_turn_end so it skips calling those callbacks if the turn
was interrupted (i.e., detected NextStepInterruption returned from
run_single_turn) and only invokes on_turn_end when the resumed path completes a
non-interrupted step; apply the same conditional guard/change to the analogous
block in run_internal.run_loop (the section around run_loop lines handling
on_turn_end) so sync and streaming behavior match, and add a regression test
exercising interrupt -> resume -> complete to assert on_turn_end is emitted
exactly once at final completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e8532f9-f6ab-4e6f-a350-24009b7e485a
📒 Files selected for processing (4)
src/agents/lifecycle.pysrc/agents/run.pysrc/agents/run_internal/run_loop.pytests/test_turn_lifecycle_hooks.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/agents/run_internal/run_loop.py
- src/agents/lifecycle.py
Comment on lines
+1107
to
+1116
| await asyncio.gather( | ||
| hooks.on_turn_end(context_wrapper, current_agent, current_turn), | ||
| ( | ||
| current_agent.hooks.on_turn_end( | ||
| context_wrapper, current_agent, current_turn | ||
| ) | ||
| if current_agent.hooks | ||
| else _coro.noop_coroutine() | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Don't emit on_turn_end before an interrupted turn actually finishes.
When run_single_turn(...) yields NextStepInterruption, Line 1107 still fires on_turn_end(...) even though the turn is only paused for approval/resume. The resumed path above (Lines 648-760) then completes that same turn without re-emitting the hook, so interrupted runs observe on_turn_end too early and never at the real completion point.
Please skip this callback for NextStepInterruption and emit it only once the resumed turn reaches a non-interrupted step. The same fix should be mirrored in src/agents/run_internal/run_loop.py:923-931 to keep sync and streaming behavior aligned. A regression test for the interrupt/resume flow would also help here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/run.py` around lines 1107 - 1116, The on_turn_end hook is being
emitted prematurely when run_single_turn yields NextStepInterruption; update the
logic around the await asyncio.gather call that invokes hooks.on_turn_end and
current_agent.hooks.on_turn_end so it skips calling those callbacks if the turn
was interrupted (i.e., detected NextStepInterruption returned from
run_single_turn) and only invokes on_turn_end when the resumed path completes a
non-interrupted step; apply the same conditional guard/change to the analogous
block in run_internal.run_loop (the section around run_loop lines handling
on_turn_end) so sync and streaming behavior match, and add a regression test
exercising interrupt -> resume -> complete to assert on_turn_end is emitted
exactly once at final completion.
Address seratch's review feedback on openai#2911: hooks that only observe cannot affect agent loop orchestration. This commit adds a TurnControl return type ('continue' | 'stop') so on_turn_start can now halt the run before the LLM is called for that turn. Changes: - lifecycle.py: on_turn_start now returns Union[TurnControl, None] (None and 'continue' are equivalent; 'stop' halts the loop) - run.py (non-streaming path): checks return value; raises MaxTurnsExceeded with descriptive message on 'stop' - run_internal/run_loop.py (streaming path): checks return value; signals QueueCompleteSentinel on 'stop' - __init__.py: exports TurnControl, RunHooksBase, AgentHooksBase - tests: 4 new test cases covering stop-on-turn-N, stop-on-turn-1, explicit 'continue', and agent-level stop The MaxTurnsExceeded raise on 'stop' keeps behaviour consistent with the existing max_turns limit: callers can catch and inspect .run_data if needed.
adityasingh2400
force-pushed
the
feat/turn-lifecycle-hooks-2671-v2
branch
from
05a4f15 to
f1cafcd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agents/lifecycle.py`:
- Around line 115-118: The docstring for on_turn_start incorrectly claims
raising StopAgentRun will gracefully halt a run, but runners only handle the
hook's return value; remove the exception-based stopping text and only document
returning "stop" (or None/"continue") as the supported mechanism in the
on_turn_start documentation, and ensure the symbol names mentioned are
on_turn_start and StopAgentRun so future implementers know the discrepancy if
runners are later updated.
In `@src/agents/run_internal/run_loop.py`:
- Around line 935-944: The resumed-interrupted-turn path (where
resolve_interrupted_turn(...) completes a previously interrupted turn) never
emits the on_turn_end hooks, so add the same hook invocation used at the bottom
of the loop to the resume path: after resolve_interrupted_turn(...) finishes,
await asyncio.gather(hooks.on_turn_end(context_wrapper, current_agent,
current_turn), (current_agent.hooks.on_turn_end(context_wrapper, current_agent,
current_turn) if current_agent.hooks else _coro.noop_coroutine())) so resumed
runs fire both the global hooks.on_turn_end and the agent-specific
current_agent.hooks.on_turn_end; locate the resume branch around
resolve_interrupted_turn and mirror the existing invocation around on_turn_end
in run_loop.py.
- Around line 823-844: The on_turn_start stop branch currently terminates the
stream directly; instead, mirror the existing max-turns flow by setting
streamed_result._max_turns_handled = True, updating streamed_result.current_turn
and run_state._current_turn/_current_step as you already do, then raise the same
MaxTurnsExceeded exception (the one used by the max-turns handler) so the outer
max-turns handling path processes the stop and produces the unified final output
(preserve the logger.debug and QueueCompleteSentinel enqueueing behavior before
raising so behavior matches the sync path).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68cf7fac-dc17-4621-858a-5666b3fcf559
📒 Files selected for processing (5)
src/agents/__init__.pysrc/agents/lifecycle.pysrc/agents/run.pysrc/agents/run_internal/run_loop.pytests/test_turn_lifecycle_hooks.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_turn_lifecycle_hooks.py
- src/agents/run.py
Comment on lines
+823
to
+844
| run_hook_control, agent_hook_control = await asyncio.gather( | ||
| hooks.on_turn_start(context_wrapper, current_agent, current_turn), | ||
| ( | ||
| current_agent.hooks.on_turn_start( | ||
| context_wrapper, current_agent, current_turn | ||
| ) | ||
| if current_agent.hooks | ||
| else _coro.noop_coroutine() | ||
| ), | ||
| ) | ||
| if run_hook_control == "stop" or agent_hook_control == "stop": | ||
| logger.debug( | ||
| "Turn %s: on_turn_start hook requested stop; halting run.", | ||
| current_turn, | ||
| ) | ||
| streamed_result._max_turns_handled = True | ||
| streamed_result.current_turn = current_turn | ||
| if run_state is not None: | ||
| run_state._current_turn = current_turn | ||
| run_state._current_step = None | ||
| streamed_result._event_queue.put_nowait(QueueCompleteSentinel()) | ||
| break |
There was a problem hiding this comment.
Route hook-requested stops through the existing max-turns path.
Lines 833-844 terminate the stream directly, but they skip the max_turns handler flow already implemented at Lines 746-821. That means a configured max_turns error handler cannot translate on_turn_start(...)->"stop" into a final output, and it also diverges from the sync path in src/agents/run.py:971-988, which raises MaxTurnsExceeded for the same condition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/run_internal/run_loop.py` around lines 823 - 844, The
on_turn_start stop branch currently terminates the stream directly; instead,
mirror the existing max-turns flow by setting streamed_result._max_turns_handled
= True, updating streamed_result.current_turn and
run_state._current_turn/_current_step as you already do, then raise the same
MaxTurnsExceeded exception (the one used by the max-turns handler) so the outer
max-turns handling path processes the stop and produces the unified final output
(preserve the logger.debug and QueueCompleteSentinel enqueueing behavior before
raising so behavior matches the sync path).
CodeRabbit flagged that on_turn_end was being emitted prematurely whenever a
turn ended in NextStepInterruption, and was never emitted at all when the
resumed path completed the previously interrupted turn.
This change:
* Wraps the on_turn_end gather in run.py and run_internal/run_loop.py with
a not-isinstance(..., NextStepInterruption) guard so an interrupted turn
no longer fires on_turn_end before the run returns.
* Adds an on_turn_end gather on the resume path (both sync and streaming)
after the NextStepInterruption check, so a successfully resumed turn
fires on_turn_end exactly once before handling RunAgain / FinalOutput /
Handoff.
* Updates the on_turn_start docstring to drop the StopAgentRun reference
(only the return-value mechanism is supported by the runners) and adds a
note to on_turn_end clarifying that interrupted turns defer the call
until they resume to a non-interrupted outcome.
* Adds two regression tests (sync + streaming) covering interrupt then
approve then resume, asserting on_turn_end fires exactly once per
completed turn and never on the interrupted attempt.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_turn_lifecycle_hooks.py (1)
255-340: Add one streamed stop-control regression.The new
"stop"behavior is only pinned onRunner.run(...)here, butRunner.run_streamed(...)goes through its own branch insrc/agents/run_internal/run_loop.py. Mirroring one of these stop tests through the streamed runner would protect the parity claim the same way the interrupt/resume tests already do.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_turn_lifecycle_hooks.py` around lines 255 - 340, Add a mirrored streamed test that verifies returning "stop" from RunHooks.on_turn_start halts the streamed loop: create a test like test_run_hook_stop_halts_loop but call Runner.run_streamed(...) (or the streamed API used in tests) with StopAfterTurnRunHooks(stop_after=1), a FakeModel with the same two-turn outputs, and assert that the streamed call raises agents.exceptions.MaxTurnsExceeded with "halted by on_turn_start hook"; also assert hooks.turn_starts == [1,2] and hooks.turn_ends == [1] to match the non-streamed behavior. Ensure the new test uses the same helper symbols (StopAfterTurnRunHooks, FakeModel, get_function_tool, get_function_tool_call, get_text_message) and mirrors the assertions from test_run_hook_stop_halts_loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agents/lifecycle.py`:
- Around line 17-18: The TurnControl docstring contains en dashes (–) that
trigger RUF001; open the TurnControl docstring and replace each en dash with a
plain hyphen '-' (e.g., in the `"continue" (default / None) – proceed...` and
`"stop" – abort...` phrases), saving the updated string in the TurnControl
definition so linting passes.
In `@src/agents/run.py`:
- Around line 996-1003: Stop requests from on_turn_start should be handled via
the existing max-turns handler flow instead of raising MaxTurnsExceeded
directly; modify the branch that checks run_hook_control/agent_hook_control so
it calls resolve_run_error_handler_result (passing the same MaxTurnsExceeded
instance or an indicator that this is a max-turns stop) and returns/uses its
result, rather than raising the exception, so callers with a configured
max-turns handler receive the handled final output path; reference the
run_hook_control/agent_hook_control check, logger.debug call, on_turn_start
hook, current_turn, MaxTurnsExceeded, and resolve_run_error_handler_result when
making the change.
---
Nitpick comments:
In `@tests/test_turn_lifecycle_hooks.py`:
- Around line 255-340: Add a mirrored streamed test that verifies returning
"stop" from RunHooks.on_turn_start halts the streamed loop: create a test like
test_run_hook_stop_halts_loop but call Runner.run_streamed(...) (or the streamed
API used in tests) with StopAfterTurnRunHooks(stop_after=1), a FakeModel with
the same two-turn outputs, and assert that the streamed call raises
agents.exceptions.MaxTurnsExceeded with "halted by on_turn_start hook"; also
assert hooks.turn_starts == [1,2] and hooks.turn_ends == [1] to match the
non-streamed behavior. Ensure the new test uses the same helper symbols
(StopAfterTurnRunHooks, FakeModel, get_function_tool, get_function_tool_call,
get_text_message) and mirrors the assertions from test_run_hook_stop_halts_loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ceee7f44-1ed8-4f72-9b47-71669a935467
📒 Files selected for processing (4)
src/agents/lifecycle.pysrc/agents/run.pysrc/agents/run_internal/run_loop.pytests/test_turn_lifecycle_hooks.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/agents/run_internal/run_loop.py
Comment on lines
+17
to
+18
| * ``"continue"`` (default / ``None``) – proceed with the turn as normal. | ||
| * ``"stop"`` – abort the run gracefully after this hook returns, exactly as if |
There was a problem hiding this comment.
Replace the en dashes in the TurnControl docstring.
Ruff flags the – characters on Lines 17-18 as ambiguous punctuation (RUF001), so this will fail in strict linting. Use plain - here instead.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 17-17: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
[warning] 18-18: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/lifecycle.py` around lines 17 - 18, The TurnControl docstring
contains en dashes (–) that trigger RUF001; open the TurnControl docstring and
replace each en dash with a plain hyphen '-' (e.g., in the `"continue" (default
/ None) – proceed...` and `"stop" – abort...` phrases), saving the updated
string in the TurnControl definition so linting passes.
Comment on lines
+996
to
+1003
| if run_hook_control == "stop" or agent_hook_control == "stop": | ||
| logger.debug( | ||
| "Turn %s: on_turn_start hook requested stop; halting run.", | ||
| current_turn, | ||
| ) | ||
| raise MaxTurnsExceeded( | ||
| f"Run halted by on_turn_start hook at turn {current_turn}" | ||
| ) |
There was a problem hiding this comment.
Route hook-triggered stops through the existing max-turns handler.
Line 1001 raises MaxTurnsExceeded directly, so a stop returned from on_turn_start skips the resolve_run_error_handler_result(...) flow used by the real max-turns branch below. That breaks the new “same as max_turns” contract for callers that have a max-turns handler configured, because they’ll get an exception here instead of the handled final output path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/run.py` around lines 996 - 1003, Stop requests from on_turn_start
should be handled via the existing max-turns handler flow instead of raising
MaxTurnsExceeded directly; modify the branch that checks
run_hook_control/agent_hook_control so it calls resolve_run_error_handler_result
(passing the same MaxTurnsExceeded instance or an indicator that this is a
max-turns stop) and returns/uses its result, rather than raising the exception,
so callers with a configured max-turns handler receive the handled final output
path; reference the run_hook_control/agent_hook_control check, logger.debug
call, on_turn_start hook, current_turn, MaxTurnsExceeded, and
resolve_run_error_handler_result when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Right now there's no way to react to the start or end of an individual agent turn — you can hook into LLM calls and tool calls, but not the turn boundary itself. This matters for things like compacting context between turns, injecting messages, logging per-turn metrics, or cancelling after a specific turn.
This PR adds
on_turn_startandon_turn_endto bothRunHooksBaseandAgentHooksBase.Closes openai#2671
What changed
on_turn_start(context, agent, turn_number)andon_turn_end(context, agent, turn_number)toRunHooksBaseandAgentHooksBaseinlifecycle.pyrun.py) and streaming (run_internal/run_loop.py) code pathsUsage example
Tests
Added
tests/test_turn_lifecycle_hooks.pycovering:on_turn_startfires before the LLM,on_turn_endfires after tool callsagent.hooks) also fireSummary by CodeRabbit
New Features
Behavior
Tests