feat(hooks): unify hook system — HookAwareTurn + pre_turn/post_turn rename + v0.5.0 breaking changes - #125
Merged
Merged
Conversation
OpenSpec change proposing unified hook architecture that fires all 4 hook types (pre_turn, post_turn, pre_tool_use, post_tool_use) in Turn.execute() via HookAwareTurn mixin, replacing fragmented firing in _run_stream_once() and RunHandle.start(). Key decisions: - D1: ALL hooks fire in Turn.execute() (convergence point for standalone + SessionPool) - D2: ACP tool hooks advisory (ToolCallStart/Complete), permission hooks blocking - D3: Keep Hook types, retire AgentHooks.as_capability() adapter - D4: HookAwareTurn mixin handles ALL 4 hook types - D5: Rename pre_run/post_run → pre_turn/post_turn (deprecated aliases, remove v0.5.0) - D6: Three-tier test strategy (core/smoke/integration) - D7: Dead code cleanup (stripping hack, delegate methods, broken tests) Known gap: ACP standalone mode uses inline _stream_events() that bypasses ACPTurn.execute() — hooks retained in _run_stream_once() until ACPAgentAPI adapter is built (future work, tasks.md section 11). Implements #124 (sub-issue of #123 audit). Verified by Oracle (2 rounds) + Momus (1 round).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request proposes a unified hook system across native and ACP agents, renaming run-level hooks to turn-level hooks and centralizing their execution within Turn.execute(). The review feedback highlights several critical design flaws that must be addressed: a potential TypeError due to passing duration_ms to run_post_turn_hooks without updating its signature, a double-firing risk for ACP tool hooks requiring permission, potential coupling issues in the HookAwareTurn mixin's access to the run context, and a bug in the hooks_fired guard that would break multi-turn executions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
… multi-turn
Fixes 4 issues identified by gemini-code-assist review:
1. duration_ms for post_turn: Add duration_ms: float = 0.0 to
run_post_turn_hooks() signature (task 2.7, D5 affected names)
2. ACP pre_tool_use double-firing: Add tool-call-ID-scoped guard
f'pre_tool_use:{tool_call_id}' to prevent double-firing between
request_permission() (blocking) and ToolCallStart (advisory).
New scenario in acp-server spec.
3. HookAwareTurn context access: Use class variable annotations
(_run_ctx: AgentRunContext | None = None) instead of properties.
Properties prevent subclass __init__ from setting via simple
assignment. Mock turns inherit None default (guard skipped).
4. hooks_fired breaks multi-turn: Clear hooks_fired at start of each
turn (RunHandle.start() loop for Path A, _run_stream_once() for
Path B). Also fix guard direction: in Path B, old path fires
FIRST and adds to hooks_fired; Turn.execute() checks and skips.
The original design had the direction backwards.
Oracle-evaluated solutions for all 4 comments.
…kEvent, AgentHooks, NativeAgentHookManager, and HooksConfig refs: openspec/changes/unify-hook-system
… and hooks_fired guard - HooksConfig: AliasChoices for pre_run/post_run backward compat with DeprecationWarning - AgentRunContext: hooks_fired set field for double-fire prevention - RunHandle.start() and _run_stream_once(): clear hooks_fired at turn start - HookAwareTurn mixin: abstract properties + 4 hook-firing methods with guards refs: openspec/changes/unify-hook-system
…rd old firing path - NativeTurn: inherits HookAwareTurn, fires pre_turn/post_turn in execute(), deny handling - ACPTurn: inherits HookAwareTurn, fires pre_turn/post_turn in execute(), advisory tool hooks - base_agent.py: old hook firing guarded with hooks_fired set to prevent double-firing - create_turn() methods pass hooks= parameter to turn constructors refs: openspec/changes/unify-hook-system
- 5 test files, 39 tests (4 skipped for ACP SessionPool) - Unit tests for HookAwareTurn mixin in isolation - Integration tests for NativeTurn + ACPTurn with hooks - E2E regression tests for SessionPool hook firing - 16-cell smoke test matrix (4 hook types × 4 modes) - Fix: ACPTurn post_turn hooks now fire on pre_turn deny (moved deny check inside try/finally block)
… aliases - as_capability() in AgentHooks and NativeAgentHookManager emits DeprecationWarning pointing to HookAwareTurn as replacement - HooksConfig field docstrings updated with deprecation notices for pre_run/post_run YAML aliases - (Todos 8-9)
- Moved _ToolInterceptCapability to standalone tool_intercept.py module - Removed stripping hack from as_capability() - Removed unused delegate methods (run_pre_turn_hooks, run_post_turn_hooks, run_pre_run_hooks, run_post_run_hooks deprecated aliases) - Simplified as_capability() to return ToolInterceptCapability directly - Kept: __init__, has_hooks, agent_hooks, run_pre_tool_hooks, run_post_tool_hooks (still needed by ToolInterceptCapability) - (Todo 10)
…, and old hook firing path (Todo 12) - Remove as_capability() from AgentHooks and NativeAgentHookManager - Remove _wrap_* helpers from agent_hooks.py - Remove run_pre_run_hooks()/run_post_run_hooks() deprecated aliases - Remove pre_run/post_run AliasChoices and model_validator from HooksConfig - Guard old hook firing in base_agent.py with AGENT_TYPE != 'native' - Native agents now fire hooks exclusively via HookAwareTurn - ACP standalone path preserved (still uses old hook firing) - Delete test_hooks_capability.py (19 tests for removed as_capability()) - Update tests to use ToolInterceptCapability directly
… .openspec.yaml status (Todo 13)
Collaborator
Author
|
/gemini review |
Leoyzen
changed the title
There was a problem hiding this comment.
Code Review
This pull request unifies the hook system by renaming pre_run/post_run to pre_turn/post_turn and introducing a shared HookAwareTurn mixin for both native and ACP agents to execute hooks from a single choke point. Feedback on the changes suggests removing committed workspace files under .omo/, using an explicit is not None check for tool_call_id in _fire_pre_tool_hooks, and passing the actual elapsed turn duration in milliseconds to _fire_post_turn_hooks instead of hardcoding it to 0.0.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ation_ms to post_turn hooks (PR #125 review)
Leoyzen
added a commit
that referenced
this pull request
Jul 8, 2026
Leoyzen
added a commit
that referenced
this pull request
Jul 8, 2026
…ation_ms to post_turn hooks (PR #125 review)
Leoyzen
added a commit
that referenced
this pull request
Jul 8, 2026
…_tool_execute The unified hook system refactor (fa326ff, PR #125) introduced ToolInterceptCapability.wrap_tool_execute() with a broad except Exception that swallowed all exceptions including control-flow ones. This caused the agent to continue executing after user cancel, broke deferred execution, approval flows, and LLM retry signals. Re-raised exceptions (mirrors pydantic-ai's own contract in _run_execute_hooks, tool_manager.py:325-328): Pydantic-AI control-flow: - CallDeferred — deferred execution signal from elicitation/MCP tools - ApprovalRequired — human-in-the-loop approval signal - ToolRetryError — what ModelRetry becomes after _raw_execute converts it (the actual retry signal in normal flow) - ModelRetry — safety net for wrap_validation_errors=False edge case AgentPool control-flow: - RunAbortedError — user cancel/timeout, propagates to NativeTurn.execute() - ToolSkippedError — pre-tool hook deny, propagates as skip signal Not included (verified by Oracle analysis): - ChainAbortedError — never raised anywhere in codebase - JobError (base class) — too broad, catches unrelated JobRegistrationError Adds 6 regression tests verifying each exception type is re-raised.
Leoyzen
added a commit
that referenced
this pull request
Jul 8, 2026
…_tool_execute (#129) The unified hook system refactor (fa326ff, PR #125) introduced ToolInterceptCapability.wrap_tool_execute() with a broad except Exception that swallowed all exceptions including control-flow ones. This caused the agent to continue executing after user cancel, broke deferred execution, approval flows, and LLM retry signals. Re-raised exceptions (mirrors pydantic-ai's own contract in _run_execute_hooks, tool_manager.py:325-328): Pydantic-AI control-flow: - CallDeferred — deferred execution signal from elicitation/MCP tools - ApprovalRequired — human-in-the-loop approval signal - ToolRetryError — what ModelRetry becomes after _raw_execute converts it (the actual retry signal in normal flow) - ModelRetry — safety net for wrap_validation_errors=False edge case AgentPool control-flow: - RunAbortedError — user cancel/timeout, propagates to NativeTurn.execute() - ToolSkippedError — pre-tool hook deny, propagates as skip signal Not included (verified by Oracle analysis): - ChainAbortedError — never raised anywhere in codebase - JobError (base class) — too broad, catches unrelated JobRegistrationError Adds 6 regression tests verifying each exception type is re-raised.
6 tasks
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
2 participants
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
Unifies the hook system so all 4 hook types (
pre_turn,post_turn,pre_tool_use,post_tool_use) fire reliably fromTurn.execute()via a newHookAwareTurnmixin — the original bug was hooks not firing in SessionPool mode.Key changes
HookAwareTurn mixin (
orchestrator/turn.py) — single choke point for hook execution inTurn.execute(), inherited by bothNativeTurnandACPTurnRename
pre_run/post_run→pre_turn/post_turnthroughout the codebase (HookEvent, AgentHooks, HooksConfig, all callers)v0.5.0 breaking changes — removed:
AgentHooks.as_capability()and_wrap_*helpersNativeAgentHookManager.as_capability()pre_run/post_runYAML config aliasesrun_pre_run_hooks()/run_post_run_hooks()deprecated methodsbase_agent.pyfor native agents (ACP standalone preserved)NativeAgentHookManager slimmed 726→187 LOC (74% reduction).
_ToolInterceptCapabilityextracted totool_intercept.py.39 new tests — 14 unit (HookAwareTurn mixin), 16-cell smoke matrix (4 hook types × 4 modes), 9 integration/E2E (NativeTurn, ACPTurn, SessionPool)
Migration docs in AGENTS.md — YAML rename guide,
as_capability()removal, v0.5.0 breaking changes tableImplementation status
openspec/changes/archive/2026-07-08-unify-hook-system/Test plan
uv run ruff check src/— cleanuv run mypy src/— clean (600 files)uv run pytest tests/hooks/ tests/agents/native_agent/test_native_turn_hooks.py tests/agents/acp_agent/test_acp_turn_hooks.py tests/orchestrator/test_session_pool_hooks.py -x— 56 passed, 4 skipped (ACP SessionPool requires real subprocess)Migration guide
See AGENTS.md "Hooks & Events System" section for:
pre_run:→pre_turn:,post_run:→post_turn:as_capability()removed — hooks now fire via HookAwareTurn automatically