Bug: when an approved tool re-raises CallDeferred or ApprovalRequired during execution, the call is silently lost. The continue here skips adding it to executed, but remaining = requests.remaining(results) at line 757 also excludes it because the call's ID is in results.approvals. So the call ends up in neither executed nor remaining.

The comment says "it goes back to unresolved" but the code doesn't do that. The resolution loop in _agent_graph.py won't see the call again because it iterates over deferred_tool_requests (the remaining from this method).

To fix, track re-raised call IDs and exclude them from resolved_ids when computing remaining, or build remaining by subtracting only the IDs that were actually executed or denied (not just those the handler claimed to handle).

build_results doesn't reference self at all — it doesn't validate that the provided IDs match this request's calls/approvals, nor does it pre-populate from the request. As-is it's just DeferredToolResults(approvals=approvals or {}, ...) and could be a @staticmethod or omitted entirely.

If the intent is a convenience builder, it would be more useful if it actually used self — e.g. to auto-approve all calls: requests.build_results(approve_all=True). Otherwise this adds surface area to the public API without adding value over constructing DeferredToolResults directly.

The pyright: ignore[reportReturnType] here is because the return type is Self | None but the method constructs a plain DeferredToolRequests(...) which isn't Self if this class is subclassed. Since DeferredToolRequests isn't designed for subclassing (it's kw_only=True dataclass), the simpler fix is to use DeferredToolRequests | None as the return type instead of Self | None, avoiding the type suppression.

5 is a magic number — extract it to a module-level constant (e.g. _MAX_DEFERRED_RESOLUTION_ROUNDS = 5).

Also, when the limit is hit (the for/else path at line 1575), the remaining deferred calls are silently treated as unresolved. This could mask infinite-loop bugs during development. Consider logging a warning when the max is reached so users can diagnose unexpected deferred-tool-as-output behavior.

Naming nit: merge as an in-place mutating method is surprising on a dataclass. The standard library convention is update for in-place dict-like mutation (à la dict.update()). If this stays in-place, renaming to update would be more Pythonic.

📝 Info: Handler-resolved batch path skips usage limit checks for tool execution

In _agent_graph.py:1597-1606, handler-resolved calls go through _call_tools for execution without a preceding usage-limit check (the check at line 1488 only covers calls_to_run). Additionally, _call_tool calls execute_tool_call without passing usage, so executed tools don't increment usage.tool_calls. However, this is consistent with the existing deferred-resume path (via UserPromptNode._handle_deferred_tool_results), which also goes through _call_tools → _call_tool without usage counting. The handler-resolved path is not a regression — it follows the same pattern as pre-existing deferred tool execution.

Was this helpful? React with 👍 or 👎 to provide feedback.

🚩 Missing FunctionToolCallEvent for non-ToolApproved handler-resolved calls in batch path

In the batch handler path (_agent_graph.py:1573-1606), FunctionToolCallEvents are only emitted for ToolApproved calls that reach the validation step (line 1590). For non-ToolApproved handler results (ToolDenied, ModelRetry, RetryPromptPart, ToolReturn), no FunctionToolCallEvent is emitted before the FunctionToolResultEvent produced by _call_tools. This contrasts with the UserPromptNode resume path (line 1484-1486) where non-ToolApproved deferred results get FunctionToolCallEvent(call) without args_valid. This is a minor inconsistency in event emission that could affect streaming consumers that expect paired call/result events, but is unlikely to cause practical issues since these calls were already deferred (and got their initial call event during the first processing pass).

Was this helpful? React with 👍 or 👎 to provide feedback.

Let's allow the handler to return None as well if it doesn't want to handle at all.

Also when the model calls a tool that was specifically registered as unapproved/external. We should link to the deferred tools / approval / external tools docs here.

This is an instance method because in the future I expect to add a context: object field to Request that is meant to be reproduced on Results verbatim, but I agree with an earlier auto-review comment that this method doesn't serve much purpose right now.

So let's make it serve some purpose and raise an error if a result is provided for a tool call ID that wasn't in the appropriate "requests list". We may have a check already somewhere that you can't provide ToolApproved for an external call that wants a result. I don't know if we similarly block (or should block) providing a tool result directly for a tool that was looking for approval. But either way some validation here would be useful.

An optional approve_all=True is also interesting. If that's provided along with approvals, it would apply to the rest?

I see too much overlap here with these from tools.py:

DeferredToolApprovalResult: TypeAlias = Annotated[ToolApproved | ToolDenied, Discriminator('kind')]
"""Result for a tool call that required human-in-the-loop approval."""
DeferredToolCallResult: TypeAlias = Annotated[
    Annotated[ToolReturn, Tag('tool-return')]
    | Annotated[ModelRetry, Tag('model-retry')]
    | Annotated[RetryPromptPart, Tag('retry-prompt')],
    Discriminator(_deferred_tool_call_result_discriminator),
]
"""Result for a tool call that required external execution."""
DeferredToolResult = DeferredToolApprovalResult | DeferredToolCallResult
"""Result for a tool call that required approval or external execution."""

I don't think we need both? We should be able to use those?

I suppose the case where we set this to False is if we want to call the handler in a batch? Do we do that currently in the agent graph?

Do we lose outcome=denied on the ToolReturnPart here, somehow? should we raise the exception?...

I don't love the complicated list of tuples this method returns...

Do these 2 method calls need to be separate? Would a single helper method make sense?

Could we stop overriding this method now?

We need docs where we currently introduce deferred/external/approved tools and how to work with DeferredToolRequests! This new approach is way simpler than what we had before. The old "stop the world/agent run" method still has its uses though, like with UI adapters where you need to return the pending tool call to the frontend, so that it can then start a new run that sends back the result/approval.

I'd prefer to present this as the primary method of dealing with deferred tools, with both methods getting their own section but the handler method being introduced first.

It feels like we're missing setting ToolReturnPart.outcome='denied' here?

There is some apparent duplication between here and to_tool_call_results. Inevitable?

Above it says somewhere that the handler should provide a result for "each pending call", but that's too strict, because we do allow partial resolution, and having multiple capabilities each handle a subset of deferred tool calls.

By the way, if we haven't yet done that, we should also add docs for the new handle_... "hook" method on capabilities.md. And likely also hooks.md, making sure that it can be used by the Hooks capability, like most of the other methods on AbstractCapability are.

So "bubble up as output" is not completely accurate -- because they can also be handled y anotehr handler.

It's also worth explicitly specifying that there is a hook method on the capability class (by linking to the capabilities doc for example), if you want to use HITL/external function calls in a custom capability.

Having this as a public class makes me want to also handle it in places like we handle ModelRetry, as a way for a tool call function to explicitly raise/wrap an error as "this means you weren't allowed to do this" (in surfacing to a user or o11y backend), even if our native tool approval wasn't used. That's similar to the ToolFailed exception being introduced in #2586 which is worth reading. But I don't want to implement the full scope of that right now (unless you think it's really easy as we're working on this code anyway -- to allow tool functions anywhere to raise ToolFailed('...', reason='denied') which is registered as outcome='denied'? But I'd prefer not to bring that in scope, as that'd imply documenting it etc...).

Does this need to be public? Is there another convention we could find for the way a method indicates that it was a tool-denial, e.g. returning ToolDenied | Any and telling people to isinstance check it? We already pass A | B | Any around in a few places. Discuss with me please.

🚩 handle_call return type change is a public API contract change

The return type of ToolManager.handle_call changed from Any to Any | ToolDenied. While ToolManager is not a heavily used public API (primarily internal), it IS exposed on RunContext.tool_manager and the PR's own tests show users calling ctx.tool_manager.handle_call() from within tool functions. Existing callers that don't check for ToolDenied will silently treat the denial message string as a successful tool result. The docstring at tool_manager.py:634-639 documents this requirement clearly, and the only internal caller (result.py:209-216) is correctly handled with a cast and comment explaining output tools can't be deferred. This is a valid design trade-off (returning ToolDenied vs raising an exception), but worth noting in changelog/migration docs.

(Refers to lines 612-657)

Was this helpful? React with 👍 or 👎 to provide feedback.

Would this read nicer as on.deferred_tool_calls?

Would it make sense to list ToolReurn[Any] in these types of cases as well? And I prefer to end with Any and start specific, here and elsewhere.