Python: bind tool-approval responses to surfaced approval requests (#7383) - #7581
Conversation
Mirrors the .NET behavior from microsoft#7111 for the Python core, closing the gap tracked in microsoft#7383: an inbound function_approval_response previously executed whatever function_call it carried, so an edited or replayed response could execute a call that was never surfaced for approval. - surfaced approval requests are recorded in the session tool-approval state bag when the batch pauses (same gating as the existing already-approved-siblings mechanism) - on resume, each decision is bound back to its recorded request: the recorded function call is executed, a differing embedded call is logged and ignored - records are one-time-use: consumed by the first decision, approved or rejected - sessionless flows are unchanged (nothing recorded, responses pass through as before) Tests: two new cases in test_harness_tool_approval.py (edited response executes the surfaced call; record consumed on decision). Full runs of test_harness_tool_approval.py, test_function_invocation_logic.py, test_sessions.py and test_security.py: 475 passed, 4 pre-existing environment failures (aiohttp/mcp deps) that also fail on clean main. Assisted-by: Claude (Anthropic)
|
Tony Dzi (Anton Dziatkovskii) (@tonydzi) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull request overview
Binds Python tool-approval decisions to session-recorded approval requests to prevent edited calls and replay.
Changes:
- Records surfaced approval requests in session state.
- Rebinds approved responses and consumes stored requests.
- Adds binding and consumption tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
python/packages/core/agent_framework/_tools.py |
Implements approval recording, binding, and consumption. |
python/packages/core/tests/core/test_harness_tool_approval.py |
Tests edited responses and record consumption. |
Suppressed comments (4)
python/packages/core/agent_framework/_tools.py:2159
- A session with no
surfaced_approval_requestsmap is currently treated like a sessionless invocation. After the final record is removed below—or when resuming state serialized before this change—a stale/fabricated approved response is therefore executed unchanged, defeating one-time consumption. Onlystate is Noneshould retain passthrough behavior; a session-backed response without a recorded unresolved request must be ignored or rejected, with all supported local approval paths recorded first.
state = _get_tool_approval_state(invocation_session)
raw_surfaced = state.get(_SURFACED_APPROVAL_REQUESTS_KEY) if state is not None else None
if state is None or not isinstance(raw_surfaced, dict):
return [response for response in approval_responses if response.approved]
python/packages/core/agent_framework/_tools.py:2172
- An approved response whose ID is absent from the record is still appended and executed from its caller-supplied function call. This permits an unsurfaced approval to execute whenever the session happens to contain records for other requests. Unknown or malformed records must be ignored/rejected rather than passed through.
if recorded is None or recorded.type != "function_call":
responses_to_execute.append(response)
continue
python/packages/core/agent_framework/_tools.py:2185
- Reconstructing the response drops its annotations, additional properties, and raw representation. Function middleware receives this response through
context.metadata["approval_response"], so session-backed approvals now lose caller metadata that previously passed through. Preserve the original decision content and replace only its untrustedfunction_call.
responses_to_execute.append(
Content.from_function_approval_response(
id=response.id, # type: ignore[arg-type]
function_call=recorded,
approved=True,
)
python/packages/core/agent_framework/_tools.py:2831
- Only the list sent to the executor is rebound;
_replace_approval_contents_with_resultsstill receives the original edited responses. If an embeddedcall_idwas changed, execution produces a result under the recorded call ID, but normalization looks it up under the edited ID and emits no terminal result, leaving the approval wrapper in the transcript. Bind the pending response map itself (including rejected decisions), then derive approved executions from that canonical map.
responses_to_execute = _bind_approval_responses_to_surfaced_requests(
invocation_session,
list(pending_approval_responses.values()),
)
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| def _bind_approval_responses_to_surfaced_requests( | ||
| invocation_session: AgentSession | None, | ||
| approval_responses: Sequence[Content], | ||
| ) -> list[Content]: |
|
Good, this closes the specific gap cleanly -- rebind-to-recorded-call is the right primitive regardless of how the state-restore question eventually gets answered. On the state-restore known edge: agreed on the layering (staleness-invalidation belongs to the state layer, not this binding) -- but worth naming the concrete residual explicitly, since "that's a property of the state layer" can read as "not a real problem" if it's left implicit. As written, a |
|
Real, well-tested implementation of the shape from #7383 -- the record/rebind/consume discipline holds up exactly as discussed, and the one-time-use consumption (approved OR rejected) closes the replay gap cleanly. On the open question (newest-record-wins vs first-occurrence): newest wins is the right call, and here's the concrete reasoning from our own analogous case. Our /review verdicts bind to an Nice, concrete edge case caught in the tests (edited call args never reaching the tool) -- that's the actual security property, not just the happy path. |
Summary
Closes the gap tracked in #7383 by mirroring the .NET behavior from #7111 in the Python core: a
function_approval_responseis treated as a decision token bound to the approval request that was actually surfaced, not as a work order carrying its own function call.Opened as a draft deliberately: #7383 is assigned to Eduard van Valkenburg (@eavanvalkenburg), and this is the working draft I offered on #7345 — please treat it as raw material, not a claim on the issue. Close it freely in favor of your own implementation, or tell me what to change and I'll iterate.
What changes
surfaced_approval_requests), with the same session gating as the existing already-approved-siblings mechanism._resolve_approval_responsesbinds each inbound decision back to its recorded request: the recorded function call is what executes. An embedded call that differs from the recorded one is logged (logger.warning) and ignored, so an edited or replayed response cannot execute a call that was never surfaced.Known edges (reviewer input welcome)
call_id), so if the same call id is surfaced again in a later turn the newer record wins. That matches the correlate-by-latest intuition but differs from_collect_unanswered_approval_requests, which keeps the first occurrence — happy to align either way.Testing
test_harness_tool_approval.py: an approval response carrying an edited call executes the surfaced call (the edited arguments never reach the tool), and the record is consumed on first decision.test_harness_tool_approval.py(27 passed), plustest_function_invocation_logic.py,test_sessions.py,test_security.py: 475 passed, 3 skipped, 4 failed — the 4 failures are environment-dependent (aiohttp / MCP extras) and fail identically on cleanmainin the same environment.Disclosure
Architecture and validation are mine; the code and text were drafted in pair with Claude (commit carries
Assisted-by:).