Python: [BREAKING] Require committed finish reason for tool calls - #8305
Python: [BREAKING] Require committed finish reason for tool calls#8305Eduard van Valkenburg (eavanvalkenburg) wants to merge 16 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Require terminal completion evidence before exposing tool_calls as the finalized finish reason in OpenAI Responses streams and non-streaming responses. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The security-sensitive authorization boundary spans core replay semantics and multiple provider protocols, warranting final human review despite extensive tests.
Pull request overview
Establishes finish_reason="tool_calls" as the authorization boundary for executing local Python function calls.
Changes:
- Enforces committed-call checks and excludes uncommitted turns from replay.
- Resolves provider-specific terminal evidence across OpenAI, Anthropic, Gemini, and AG-UI.
- Adds regression coverage, specification updates, and a proposed ADR.
File summaries
| File | Description |
|---|---|
python/packages/openai/tests/openai/test_openai_chat_client.py |
Tests OpenAI commitment evidence and execution behavior. |
python/packages/openai/agent_framework_openai/_chat_client.py |
Resolves Responses API call commitment. |
python/packages/ollama/tests/test_ollama_chat_client.py |
Verifies pre-terminal calls remain unauthorized. |
python/packages/gemini/tests/test_gemini_client.py |
Tests Gemini terminal-reason handling. |
python/packages/gemini/agent_framework_gemini/_chat_client.py |
Authorizes calls only on normal terminal candidates. |
python/packages/core/tests/workflow/test_agent_executor_tool_calls.py |
Updates workflow test responses with authorization. |
python/packages/core/tests/core/test_observability.py |
Updates observable tool-call fixtures. |
python/packages/core/tests/core/test_middleware_with_chat.py |
Updates middleware tool-call fixture. |
python/packages/core/tests/core/test_harness_agent.py |
Updates streamed harness fixture. |
python/packages/core/tests/core/test_function_invocation_logic.py |
Covers authorization and replay exclusion. |
python/packages/core/tests/core/test_clients.py |
Corrects client fixture finish reason. |
python/packages/core/tests/core/test_agents.py |
Updates agent test clients and fixtures. |
python/packages/core/tests/core/conftest.py |
Auto-finalizes committed fixture calls. |
python/packages/core/agent_framework/_types.py |
Documents the stronger finish-reason contract. |
python/packages/core/agent_framework/_tools.py |
Enforces authorization and marks uncommitted turns. |
python/packages/core/agent_framework/_sessions.py |
Restricts follow-ups to authorized calls. |
python/packages/core/agent_framework/_clients.py |
Filters marked turns before model input. |
python/packages/anthropic/tests/test_anthropic_client.py |
Tests Anthropic block and message completion. |
python/packages/anthropic/agent_framework_anthropic/_chat_client.py |
Resolves commitment at message_stop. |
python/packages/ag-ui/tests/ag_ui/test_ag_ui_client.py |
Tests AG-UI terminal commitment and failures. |
python/packages/ag-ui/tests/ag_ui/conftest.py |
Updates AG-UI test client finalization. |
python/packages/ag-ui/agent_framework_ag_ui/_client.py |
Tracks completed client-side AG-UI calls. |
docs/specs/004-python-function-calling-loop.md |
Defines authorization and regression requirements. |
docs/decisions/0041-authorize-function-calls-with-finish-reason.md |
Records the proposed architectural decision. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (16 commit(s)): 6eff2ba2f117, 2c3b994ff6f9, 118cc1f6d223, 481d5bb82952, 1b15c4011893, 62c2ff84488b, 519b2114568d, 4507afd289e6, 32e6be0b94e9, 95bdd7359a0a, f583414c69d7, 2e4b7eb6ee38, 25de4b6ad0eb, f2ab412cef7b, cbdf396163c0, dde9ffb5f873
Model: gpt-5.6-sol-fast
Overview
The PR establishes a clear fail-closed authorization boundary and adds strong streaming/non-streaming coverage across core and the affected providers. Provider terminal-event checks and whole-turn replay markers prevent the ordinary incomplete-response paths from invoking local tools. Two residual gaps remain: the core verdict can arrive after durable continuation state has already committed, and AG-UI's ID-only completion tracking can authorize an unfinished local call after an ID collision.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 2 files. Details are attached to the affected lines below.
Affected areas: python/packages/ag-ui/agent_framework_ag_ui/_client.py, python/packages/core/agent_framework/_tools.py
| # 1. Extract only actionable, unanswered calls from this model turn. | ||
| tools = _extract_tools(options) | ||
| function_calls = _extract_function_calls(response) | ||
| if function_calls and response.finish_reason != "tool_calls": |
There was a problem hiding this comment.
This verdict is applied only after super_get_response has returned, but per-service-call history middleware persists the response before control reaches this point, and continuation state is advanced before this call. A serializing HistoryProvider therefore stores an interrupted call-bearing turn without the exclusion marker, while a service-managed conversation can retain it behind the newly saved session ID; the next run then replays or resumes from the turn this change intends to exclude. Please establish the uncommitted verdict before persistence and continuation state are committed, or explicitly suppress those commits when the final reason is not tool_calls.
| if event_tool_name in client_tool_set: | ||
| open_client_tool_call_ids.add(event_tool_call_id) | ||
| elif event_type == "TOOL_CALL_END": | ||
| open_client_tool_call_ids.discard(event_tool_call_id) |
There was a problem hiding this comment.
This clears an open local call solely by ID, even when the ending event belongs to a duplicate or server-tool start that reused that ID. Its end can empty the set, so RUN_FINISHED authorizes the still-unfinished local call; a parameterless or defaulted local tool can then execute prematurely. Please track starts and ends by local-call occurrence/type (or reject duplicate and empty IDs) and commit only after every local occurrence has ended.
Motivation & Context
Function-call content can arrive before a provider has authoritatively completed and committed the model turn. Treating complete-looking fragments as executable requests can cause local approvals, middleware, or tool bodies to run after an interrupted, incomplete, malformed, or failed response.
This change makes the existing
finish_reason="tool_calls"value the provider-neutral authorization boundary for local function invocation while preserving incremental streaming updates.Description & Review Guide
finish_reason="tool_calls"before core approves or executes actionable local calls.finish_reason="tool_calls".Related Issue
N/A
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.