Python: count async FunctionTool invocation exceptions - #8278
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Direct async calls remain untracked, and synchronous wrappers returning awaitables lack regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates FunctionTool to count exceptions raised while awaiting async results.
Changes:
- Enforces
max_invocation_exceptionsfor awaited failures. - Adds native async regression coverage.
File summaries
| File | Summary |
|---|---|
python/packages/core/agent_framework/_tools.py |
Adds await-boundary exception counting; direct async calls still bypass tracking, and the awaitable-wrapper branch lacks coverage. |
python/packages/core/tests/core/test_tools.py |
Adds native async limit coverage; missing a synchronous-wrapper awaitable case. |
Review details
Suppressed comments (2)
python/packages/core/agent_framework/_tools.py:637
- This only tracks failures reached through
invoke()'s_invoke_functionpath.FunctionToolis also directly callable (the existing async test awaits the result ofasync_test_tool(...)), and that path returns the coroutine from__call__without passing through this helper, so a failing async direct call still leavesinvocation_exception_countunchanged and never enforcesmax_invocation_exceptions. Please route direct async calls through the same tracked await path, or explicitly narrow/document this limit as applying only toinvoke()execution.
return await self._await_invocation_result(res) if inspect.isawaitable(res) else res
python/packages/core/tests/core/test_tools.py:354
- This regression test only decorates an
async def, so it exercises theinspect.iscoroutinefunctionbranch. The other supported case added here—a synchronous function returning an awaitable—runs through theasyncio.to_threadbranch and has no regression assertion, so that path could regress while this test remains green. Please add a second max-limit case for the wrapper and verify the second call leaves both counters unchanged.
@tool(name="failing_async_tool", max_invocation_exceptions=1)
async def failing_async_tool() -> str:
raise RuntimeError("boom")
with pytest.raises(RuntimeError, match="boom"):
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
Please use the defined PR template CoralGarden52 |
|
Hi Eduard van Valkenburg (@eavanvalkenburg) , apologies for missing the repository’s defined PR template in the initial PR description, and thank you for pointing this out and for reviewing the PR. I’ve now updated the PR description to use the defined PR template. |
Motivation & Context
FunctionTool.max_invocation_exceptionsis documented as the maximum number of exceptions allowed during tool invocations. Before this change, exceptions raised while awaiting an async result were not counted consistently.For native async tools,
FunctionTool.__call__returned a coroutine successfully and the exception was raised only when that coroutine was awaited. The same gap affected synchronous functions that return an awaitable. Direct async calls also bypassed the tracked await path.This allowed a failing async tool to continue running after its configured exception budget was exhausted. The separate
max_consecutive_errors_per_requestsetting is request-scoped and does not replace the per-tool lifetime limit.Description & Review Guide
_await_invocation_resulthelper.FunctionToolcalls through the same tracked await path.max_invocation_exceptionsis now enforced consistently across all supported async execution paths.ToolExceptionwithout invoking the underlying function again.max_consecutive_errors_per_requestbehavior is unchanged.__call__,invoke(), and theasyncio.to_threadpath.Reproduction before this change
Ran the same public tool paths against the upstream baseline
c37de519bwithmax_invocation_exceptions=1:The final tuple fields are
invocation_countandinvocation_exception_count. The async failures were raised, but the exception counter remained zero and the invocation count continued to increase.Verification after this change
Ran the identical reproduction against the current PR head
87afcbac8:This confirms that the first awaited failure is counted, the configured limit is enforced on the second call, and no additional underlying invocation is started.
Tests and checks
uv run pytest packages/core/tests -q -m 'not integration'— passed.uv run pytest packages/core/tests/core/test_tools.py::test_async_tool_exception_limit_counts_awaited_failures packages/core/tests/core/test_tools.py::test_direct_async_tool_exception_limit_counts_awaited_failures packages/core/tests/core/test_tools.py::test_sync_awaitable_tool_exception_limit_counts_awaited_failures packages/core/tests/core/test_tools.py -q -m 'not integration'— passed.uv run ruff format --check packages/core/agent_framework/_tools.py packages/core/tests/core/test_tools.py— 2 files already formatted.uv run ruff check packages/core/agent_framework/_tools.py packages/core/tests/core/test_tools.py— all checks passed.git diff --check— no whitespace errors.The implementation is limited to async invocation exception accounting and its regression coverage.
Related Issue
Fixes #8277
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix) — a workflow keeps the label and title prefix in sync automatically.