fix: re-raise control-flow exceptions in ToolInterceptCapability.wrap_tool_execute - #129
Conversation
There was a problem hiding this comment.
Code Review
This pull request ensures that control-flow exceptions (RunAbortedError, ToolSkippedError, and ModelRetry) raised during tool execution are re-raised and propagated rather than being swallowed as generic tool errors. Unit tests are added to verify this behavior. The reviewer suggests also propagating other critical control-flow exceptions, such as CallDeferred, ApprovalRequired, and ChainAbortedError, to prevent breaking deferred execution and approval flows.
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.
…_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.
184b9c2 to
fc0090c
Compare
Problem
The unified hook system refactor (fa326ff, PR #125) introduced
ToolInterceptCapability.wrap_tool_execute()with a broadexcept Exceptionthat swallowed all exceptions including control-flow ones:RunAbortedError— raised when user cancels elicitation/question (e.g.question_for_usercancel). Should propagate toNativeTurn.execute()'sexcept RunAbortedErrorhandler to abort the run. Instead it was converted to aToolReturnerror response, causing the agent to continue executing after cancel.ModelRetry— pydantic-ai's retry signal for fixable tool errors. Should propagate to the pydantic-ai framework. Instead the LLM received it as a tool success with error text — semantically different behavior.ToolSkippedError— raised when a pre-tool hook denies execution. Should propagate as a skip signal. Instead converted to a tool error.Evidence
From
~/Library/Logs/agentpool/acp.log:After the warning, the agent continued:
CallToolsNode → ModelRequestNode(LLM got the error and generated another request).Fix
Add
except (RunAbortedError, ToolSkippedError, ModelRetry): raisebefore the genericexcept Exceptionblock inwrap_tool_execute().Tests
3 new regression tests in
tests/agents/native_agent/test_tool_intercept_capability.py:test_wrap_tool_execute_reraises_run_aborted_errorRunAbortedErroris re-raised, not swallowedtest_wrap_tool_execute_reraises_model_retryModelRetryis re-raised, not swallowedtest_wrap_tool_execute_reraises_tool_skipped_errorToolSkippedErroris re-raised, not swallowedExisting
test_wrap_tool_execute_catches_exception(verifying normalValueErroris still caught) continues to pass. All 16 tests in the file pass, plus 11 question abort regression tests.Affected Exception Sources
RunAbortedErrorquestion_for_user.py:226,tool_impls/question/tool.py:103,agents/context.py:436ModelRetryToolSkippedErrormcp_server/tool_bridge.py:524