fix: bind tool approvals to concrete invocations - #4257
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20e8ed990c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if existing_pending is not None and pending_identity is None: | ||
| return status |
There was a problem hiding this comment.
When resuming a hosted MCP interruption whose legacy provider_data lacks name, server_label, or arguments—the shape covered by test_hosted_mcp_approval_matches_unknown_tool_key—tool_invocation_identity() returns None, but this branch still returns the stored per-call decision. A custom provider can therefore reuse that approved ID for an arbitrary current MCP request and receive an approval response without a new user decision, bypassing the invocation-binding protection. Treat an unbindable pending approval as requiring reapproval instead of honoring it.
AGENTS.md reference: AGENTS.md:L68-L68
Useful? React with 👍 / 👎.
| if completed: | ||
| return False |
There was a problem hiding this comment.
When a completed function call is replayed with the same ID and semantically identical JSON arguments but different formatting or key order, _normalize_arguments() gives it the same fingerprint and this branch suppresses execution. However, _dedupe_tool_call_items() compares the raw argument string, so it retains the replayed ToolCallItem while no corresponding output is appended. The next model request can therefore contain an unmatched function call and fail in adapters that enforce call/output pairing; dedupe the public call item with the same canonical identity or reattach the committed output.
AGENTS.md reference: AGENTS.md:L125-L125
Useful? React with 👍 / 👎.
| binding = record.invocations.get(call_id) | ||
| if binding is None or binding.invocation_type != invocation_type: | ||
| continue |
There was a problem hiding this comment.
When an ID previously bound to an approved function call is reused as a shell, custom, apply-patch, or MCP call, this condition ignores the existing binding solely because its invocation_type differs. The new call therefore passes validation, and if that tool category does not independently require approval its side effect executes despite violating the newly documented rule that an approved ID cannot be reused for changed tool identity. Treat a binding with the same call ID but a different invocation type as a mismatch and raise before planning execution.
AGENTS.md reference: AGENTS.md:L127-L127
Useful? React with 👍 / 👎.
| shell_calls=approved_shell_calls, | ||
| apply_patch_calls=approved_apply_patch_calls, | ||
| ) | ||
| _validate_unresolved_function_calls(context_wrapper, missing_function_tools) |
There was a problem hiding this comment.
On an interrupted resume that contains a changed missing function call under an approved ID plus a sibling with a dynamic needs_approval predicate, validation is deferred until here, after _select_function_tool_runs_for_resume() and the shell, apply-patch, and custom _collect_runs_by_approval() passes have already invoked user callbacks. The eventual ModelBehaviorError therefore does not occur before all user-code side effects as intended; validate missing_function_tools before running any approval predicates.
AGENTS.md reference: AGENTS.md:L128-L128
Useful? React with 👍 / 👎.
1 participant
This pull request adds defense-in-depth hardening for per-call tool approvals. Providers are expected to assign a unique call ID to each tool invocation, so the affected path is not reachable under normal provider behavior; it requires a custom provider or adapter to reuse an approved ID for different tool identity or arguments. To make the SDK robust even against that invalid reuse, each per-call approval or rejection is now bound to a canonical invocation fingerprint, changed content is rejected before side effects, and exact replays are suppressed after their output is committed. The binding and completion state are serialized in RunState schema 1.14 while older snapshots remain readable; sticky decisions continue to apply only to the same tool.