Callable `needs_approval` fails open when the predicate returns a non-bool (e.g. an unhandled branch returning `None`)

[mahirhir]

Please read this first

• Have you read the docs? Yes, the human-in-the-loop / tool approval page.
• Have you searched for related issues? Yes. The closest is Callable needs_approval fails open when tool arguments are invalid JSON #3863, "Callable needs_approval fails open when tool arguments are invalid JSON", which is closed and fixed. This is the remaining sibling on the same function: there the predicate was fed {} and answered False; here the predicate answers nothing at all.

Describe the bug

When needs_approval is a callable, a return value that is not a bool is coerced with bool(), so a predicate that falls off the end of a branch returns None, bool(None) is False, and the guarded tool runs with no approval requested.

src/agents/util/_approvals.py:

if callable(needs_approval_setting):
    maybe_result = needs_approval_setting(*args)
    if inspect.isawaitable(maybe_result):
        maybe_result = await maybe_result
    return bool(maybe_result)

The declared contract is a bool (CustomToolApprovalFunction = Callable[..., MaybeAwaitable[bool]], and the shell/apply_patch variants likewise), so None is out of contract. The point is what happens to an out-of-contract answer on a security gate: everywhere else in this feature an unanswerable approval question fails closed, and here it fails open.

Measured on main at 89c02c82, one script, five rows, four of them controls:

needs_approval policy -> did the guarded tool run without approval?
  ran=no   returns True   (control: must ask)             -> asked for approval
  ran=YES  returns False  (control: must run)             -> no approval requested
  ran=no   raises         (unanswerable: fails closed)    -> UserError: Error running tool wire_money: policy backend unreac
  ran=no   unparsable args (unanswerable: fails closed)   -> asked for approval
  ran=YES  returns None   (unhandled branch)              -> no approval requested

Rows one and two show the harness can tell an approval request from an execution. Rows three and four are the two unanswerable cases that already fail closed: a predicate that raises is caught in tool_planning (except Exception: needs_approval = True), and unparsable arguments return True in function_needs_approval (the fix from #3863). Row five is the same kind of non-answer taking the other branch.

The predicate style in the docs is the one this bites. Both documented examples are single-expression returns, but the moment a policy grows a branch:

async def needs_review(_ctx, params, _call_id) -> bool:
    if params.get("amount", 0) > 1000:
        return True
    # every other amount falls through -> None -> tool runs unapproved

The annotation says -> bool, so a type checker flags this one; a policy that returns inside if/elif chains without a final else, or that returns the result of a lookup like POLICY.get(tool_name), is not flagged and behaves the same way.

Debug information

• Agents SDK version: 0.22.0 (main at 89c02c82)
• Python version: 3.14.4
• Operating system: Windows
• Model and model provider: none, reproduced with agents.testing.ScriptedModel
• Does the issue reproduce with the latest Agents SDK release? Reproduced on main; the coercion line is unchanged in 0.22.0.
• Does the issue occur consistently or intermittently? Consistently.

Repro steps

import asyncio

from openai.types.responses import (
    ResponseFunctionToolCall,
    ResponseOutputMessage,
    ResponseOutputText,
)

from agents import Agent, Runner, function_tool
from agents.testing import ScriptedModel

EXECUTED: list[str] = []


def tool_call(arguments: str = '{"amount": 100}') -> ResponseFunctionToolCall:
    return ResponseFunctionToolCall(
        id="fc_1", call_id="call_1", type="function_call", name="wire_money", arguments=arguments
    )


def final_message() -> ResponseOutputMessage:
    return ResponseOutputMessage(
        id="msg_1",
        type="message",
        role="assistant",
        content=[ResponseOutputText(text="done", type="output_text", annotations=[], logprobs=[])],
        status="completed",
    )


def build(policy, arguments: str):
    @function_tool(needs_approval=policy)
    def wire_money(amount: int) -> str:
        EXECUTED.append(f"wired {amount}")
        return "wired"

    return Agent(
        name="banker",
        model=ScriptedModel(steps=[[tool_call(arguments)], [final_message()]]),
        tools=[wire_money],
    )


async def probe(label: str, policy, arguments: str = '{"amount": 100}') -> None:
    EXECUTED.clear()
    result = await Runner.run(build(policy, arguments), "send it")
    asked = bool(getattr(result, "interruptions", []))
    print(f'  ran={"YES" if EXECUTED else "no "}  {label:44} -> {"asked for approval" if asked else "no approval requested"}')


def always_true(ctx, args, call_id):
    return True


def always_false(ctx, args, call_id):
    return False


def forgot_a_branch(ctx, args, call_id):
    if args.get("amount", 0) > 1000:
        return True
    # no return for anything else -> None


async def main() -> None:
    await probe("returns True  (control: must ask)", always_true)
    await probe("returns False (control: must run)", always_false)
    await probe("unparsable args (control: fails closed)", always_false, "not json")
    await probe("returns None  (unhandled branch)", forgot_a_branch)


asyncio.run(main())

Expected behavior

An approval predicate that does not return a bool has not answered the question, so it should be treated the way the other unanswerable cases already are: require approval, or raise.

I have not opened a PR because the choice is yours and the helper already carries the dial for it. evaluate_needs_approval_setting raises UserError for a non-bool setting under strict=True; applying the same rule to a non-bool result would be consistent, but it turns a currently-silent truthy return (a policy returning a non-empty reason string to mean yes) into an error. Failing closed on a non-bool result instead keeps those callers working and only changes the falsy ones, which are the dangerous half. Happy to send whichever you prefer, with tests.

[xixi-box]

I'd like to work on this.

I'll keep the fix focused on callable needs_approval result handling so that non-boolean return values (such as None) cannot silently become a False approval decision.

My plan is to preserve the existing behavior for valid True/False results, cover both sync and async predicates, and add regression tests for None and other non-boolean return values.

For the exact behavior on invalid return types, I'm inclined to fail closed rather than allow the tool to execute, but I'm happy to follow the maintainers' preferred compatibility semantics.

[mahirhir]

Both PRs land the same repair on _approvals.py, and there is a second site that only one of them covers.

src/agents/mcp/server.py, _get_needs_approval_for_tool, on main today:

async def _needs_approval(run_context, _args, _call_id) -> bool:
    result = policy(run_context, agent, tool)
    if inspect.isawaitable(result):
        result = await result
    return bool(result)          # line 849

Same shape, same annotation, same consequence: an MCP tool policy that falls off a branch returns None, bool(None) is False, and the guarded tool runs with no approval requested.

#4847 routes that closure through evaluate_needs_approval_setting, so both sites are covered by one predicate. #4846 touches src/agents/util/_approvals.py and one test file, so mcp/server.py:849 still fails open after it merges. That is a difference in coverage rather than in quality; the _approvals.py change is equivalent in both.

Two smaller things in the same function, offered as observations rather than as part of this report:

return bool(policy.get(tool.name, False)) on the dict branch. The default is False, so a tool absent from the mapping needs no approval. That is a defensible design choice and it is not the same defect, but it does mean the dict form answers "unknown tool" with "allow" while the callable form will answer it with "ask" after either of these merges. The two policy shapes disagree about the unanswered case.

if agent is None: return True a few lines above already fails closed for a missing agent, which is the same instinct these PRs are applying to a missing answer. Worth citing in whichever PR lands, since it shows the file's own convention rather than importing one.

I filed the original report and have no stake in which PR is taken. Measured on main at 89c02c82; the four control values in the issue body still reproduce.

[xixi-box]

@mahirhir (the reporter) and maintainers — a status check, since four pull requests now implement the same fix for this issue (#4846, #4847, #4848, #4849), and it would help to have a maintainer direction on which one to move forward with.

All four converge on the same semantics in evaluate_needs_approval_setting: a resolved non-bool predicate result now requires approval instead of being coerced with bool(). That direction follows the precedent the maintainers set in #3867 (failing closed when a callable approval predicate cannot answer the question), which is also the behavior I proposed when claiming this issue.

For #4848 (mine) specifically:

• Helper fix plus MCP coverage: the callable require_approval bridge routes through the same shared evaluator, with sync/async MCP regression tests and a bool-result control case.
• A Realtime regression test: a non-bool predicate result produces RealtimeToolApprovalRequired and the tool is not invoked, so the strict=False path stays uniform.
• A truthy-non-bool case pinning the released v0.22.0 behavior (approval requested), so only the falsy half changes.
• The diff went through two independent clean reviews (approval-gate security and compatibility/parity specialties, including sibling scans over every evaluate_needs_approval_setting consumer) plus the repository format/lint/typecheck/test stack; the type-check and test failures observed on the local Windows environment were confirmed identical on a clean main checkout.

Core semantics are identical across the four PRs, so the decision comes down to surface coverage. Happy to rebase in whichever direction maintainers prefer; until then I'll hold off on further changes to avoid churn.

[mahirhir]

Since there are now four PRs on this, here is a runnable reproduction on the released package that any of them can be checked against. It is offline: no API key, no network, and the model implements the library's own public Model interface rather than patching anything inside agents, because a demo that monkeypatches the library proves nothing about the library.

pip install openai-agents && python demo.py

Output on openai-agents 0.22.0:

controls -- these fix the range of the reading below
  needs_approval -> True                  held: True   tool ran: False
  needs_approval -> False                 held: False  tool ran: True

trial
  needs_approval -> None                  held: False  tool ran: True

discrimination -- which accidental values fail open?
  needs_approval -> '' (empty str)        held: False  tool ran: True
  needs_approval -> [] (empty list)       held: False  tool ran: True
  needs_approval -> 0 (int)               held: False  tool ran: True
  needs_approval -> 'no' (truthy str)     held: True   tool ran: False

The controls run first and gate the trial: if True had not held the call, or False had not run it, the script prints UNTESTABLE and exits 2 rather than reporting the trial, because a reading with no established range is not evidence. Exit 1 means reproduced, 0 means not reproduced on that version.

It measures a side effect, not a return value. The guarded tool appends to a module-level list, since what an approval gate protects is a real write.

The discrimination is the useful part for choosing between the PRs

bool(maybe_result) does not just mishandle None. It makes the falsiness of the accidental value decide the direction:

returned by the predicate	approval held	tool ran	direction
None	no	yes	fails open
''	no	yes	fails open
[]	no	yes	fails open
0	no	yes	fails open
'no'	yes	no	fails closed

'no' is the one that surprised me. A policy that returns a reason string means "deny", and it currently gets the safe answer by accident. A policy that returns an empty reason string when there is no objection means "allow" and gets it by accident too, but there the accident is the unsafe direction.

So the dangerous set is not None. It is every falsy value a predicate can return by accident, and '' is the realistic one, because "return the reason, empty if none" is a normal way to write a policy function.

That gives a concrete acceptance test for whichever PR is merged: all five rows above must hold the call for approval, and True/False must still behave. A fix that special-cases None passes the trial in this issue's title and still ships '' and 0 failing open.

Second site

src/agents/mcp/server.py, _get_needs_approval_for_tool, has the same return bool(result) shape. I have not exercised it here because it needs a live MCP server, so I am reporting it as read rather than as measured. Of the four PRs, whichever lands should cover it or say why not, since a repair confined to _approvals.py leaves that path on the old semantics.

I am not opening a fifth PR. If a maintainer would like this reproduction added as a regression test to whichever PR is chosen, it is here to take.

demo.py

"""Runnable reproduction: a needs_approval predicate that returns None runs the tool.

Offline. No API key and no network. The model here implements the library's own public
Model interface rather than patching anything inside the library, because a demo that
monkeypatches the library proves nothing about the library.

    pip install openai-agents && python demo_openai_agents.py

Exit code 1 means the defect reproduced, 0 means it did not on this version, and 2 means
the controls did not behave so the trial has no established range and must not be counted.
"""

import asyncio
import sys
from collections.abc import AsyncIterator
from typing import Any

from agents import Agent, Runner, function_tool, set_tracing_disabled
from agents.items import ModelResponse
from agents.models.interface import Model
from agents.usage import Usage
from openai.types.responses import ResponseFunctionToolCall

set_tracing_disabled(True)

EXECUTED: list[str] = []
DECISION: list[Any] = [False]


class StubModel(Model):
    """One tool call on the first turn, then nothing, so a run always terminates."""

    async def get_response(self, system_instructions, input, model_settings, tools,
                           output_schema, handoffs, tracing, *, previous_response_id=None,
                           conversation_id=None, prompt=None, **kwargs) -> ModelResponse:
        asked = isinstance(input, list) and any(
            (i.get("type") if isinstance(i, dict) else getattr(i, "type", None)) == "function_call"
            for i in input
        )
        out = [] if asked else [
            ResponseFunctionToolCall(
                type="function_call", name="delete_everything", arguments="{}",
                call_id="call_1", id="fc_1",
            )
        ]
        return ModelResponse(output=out, usage=Usage(), response_id=None)

    def stream_response(self, *a, **k) -> AsyncIterator[Any]:
        raise NotImplementedError("this demo never streams")


def predicate(*_args: Any) -> Any:
    # A real predicate falls off the end of an if-chain and returns None. That is the
    # whole defect: the missing branch is not a syntax error and not a runtime type
    # error, so nothing anywhere reports it.
    return DECISION[0]


@function_tool(needs_approval=predicate)
def delete_everything() -> str:
    # The evidence is that this line ran, not what the run returned. An approval gate
    # protects a real write, and a return value can be discarded while a write cannot.
    EXECUTED.append("delete_everything")
    return "deleted"


async def trial(label: str, decision: Any) -> object:
    EXECUTED.clear()
    DECISION[0] = decision
    agent = Agent(name="demo", tools=[delete_everything], model=StubModel())
    try:
        result = await Runner.run(agent, "proceed", max_turns=4)
        held = bool(getattr(result, "interruptions", []))
        ran = bool(EXECUTED)
    except Exception as exc:
        print(f"  needs_approval -> {label:<20}  raised {type(exc).__name__}")
        return "raised"
    print(f"  needs_approval -> {label:<20}  held: {str(held):<5}  tool ran: {ran}")
    return ran


async def main() -> int:
    from importlib.metadata import version

    print(f"openai-agents {version('openai-agents')}")
    print()

    print("controls -- these fix the range of the reading below")
    held = await trial("True", True)
    ran = await trial("False", False)

    if held is not False or ran is not True:
        print()
        print("UNTESTABLE: the controls did not behave, so the trial below means nothing.")
        return 2

    print()
    print("trial")
    unanswered = await trial("None", None)

    print()
    print("discrimination -- which accidental values fail open?")
    for label, value in (("'' (empty str)", ""), ("[] (empty list)", []),
                         ("0 (int)", 0), ("'no' (truthy str)", "no")):
        await trial(label, value)

    print()
    if unanswered is not True:
        print("Not reproduced on this version.")
        return 0

    print("DEFECT REPRODUCED: an unhandled predicate branch ran the guarded tool.")
    print("  agents/util/_approvals.py, evaluate_needs_approval_setting, ends with")
    print("  `return bool(maybe_result)`. bool(None) is False, and False on this setting")
    print("  means 'no approval needed', so the missing answer becomes a decision to run.")
    print("  bool() also decides the direction for every other accidental value: falsy")
    print("  ones fail open, truthy ones fail closed. The dangerous set is every falsy")
    print("  value a predicate can return by accident, not None alone.")
    print("  agents/mcp/server.py, _get_needs_approval_for_tool, has the same shape and")
    print("  is not exercised here because it needs a live MCP server.")
    return 1


if __name__ == "__main__":
    sys.exit(asyncio.run(main()))

[xixi-box]

Thanks @mahirhir — the offline reproduction on the released package is a clean way to demonstrate the falsy-non-bool set (not just None). For the record, #4848's parametrized regression tests already cover the same discrimination values your script prints (None, "", 0, and a truthy string pinning the released approval-requested behavior), so the PR can absorb this script as an additional regression test if it's the one chosen. Either way, thanks for the runnable oracle.

[mahirhir]

Thanks @xixi-box. I checked the four PRs against the second site rather than taking it on trust, since that was the one thing my script could not exercise. gh api repos/openai/openai-agents-python/pulls/<n>/files, just now:

PR	util/_approvals.py	mcp/server.py	size
#4846	yes	no	+84/-1
#4847	yes	yes	+59/-5
#4848	yes	yes	+205/-5
#4849	yes	no	+32/-1

So the choice splits cleanly on coverage, not on style: #4846 and #4849 leave _get_needs_approval_for_tool on return bool(result), which is the same defect on a path a repair confined to _approvals.py does not reach. #4847 and #4848 close both.

Between those two, #4848 is the wider one (it also adds a realtime case), #4847 the smaller. That is a maintainer's call about review surface, and both satisfy the acceptance rows.

Correcting myself on one point: I said the MCP site "needs a live MCP server". #4848's tests/mcp/test_mcp_util.py reaches it with a fake server and asserts on function_tool.needs_approval(...) directly, so it was testable and my script simply did not find the seam. The reproduction I posted is narrower than it should have been.

All four currently read mergeable_state: blocked, which as far as I can tell is fork CI awaiting maintainer approval rather than anything in the diffs.

[xixi-box]

Thanks for verifying all four against the second site, and for the correction — it keeps the record accurate for whoever picks this up.

One size note for completeness: the runtime delta is the same few lines in #4848 and #4847 (the same non-bool rule in _approvals.py, plus routing the MCP bridge through the shared evaluator); the +205/-5 versus +59/-5 difference is test volume — Realtime and MCP regression cases plus the truthy-compat pin — not extra runtime machinery. Agree the pick between them is the maintainer's review-surface call; holding here either way.

[mahirhir]

Checked the size claim rather than taking it. pulls/<n>/files, additions split by directory:

PR	src/ additions	tests/ additions
#4846	7	77
#4847	3	56
#4848	9	196
#4849	3	29

So the +205 vs +59 gap is 140 test lines and 6 source lines, and the 6 are a two-line comment plus the same two statements spread over more lines. That matches what you said. All four also use isinstance(maybe_result, bool) rather than special-casing None, so all four hold the five acceptance rows, not just the None one.

A third site in the same helper that none of the four reaches

evaluate_needs_approval_setting has a strict parameter. When strict=False and the setting is neither a bool nor a callable, it returns default, which is False. One caller passes strict=False: src/agents/realtime/session.py, _function_needs_approval, line 662 on main at 89c02c82.

That is the same unanswered question one level up from this issue. Here it is the setting itself that is out of contract rather than a predicate's return value. FunctionTool.needs_approval is annotated bool | Callable[...], so a string or an int has answered nothing.

Measured on released openai-agents 0.22.0 by driving RealtimeSession._handle_tool_call with a stub RealtimeModel. The evidence is the tool's on_invoke_tool appending to a list, and the first two rows are controls:

  needs_approval=True       tool ran: False
  needs_approval=False      tool ran: True
  needs_approval="true"     tool ran: True
  needs_approval=""         tool ran: True
  needs_approval=None       tool ran: True
  needs_approval=0          tool ran: True
  needs_approval=1          tool ran: True

The same settings through the evaluator with the default strict=True, which is what the run path uses:

  needs_approval="true"   run path: UserError   realtime: False
  needs_approval=""       run path: UserError   realtime: False
  needs_approval=None     run path: UserError   realtime: False
  needs_approval=0        run path: UserError   realtime: False
  needs_approval=1        run path: UserError   realtime: False

"true" and 1 are the ones I would not have predicted. Both read as "yes, require approval" to whoever wrote them, both arrive from a config file or a JSON field as easily as a bool does, and on the realtime surface both execute the tool while the run path refuses to start. Note the direction is worse than the bool() line this issue is about: bool() at least sends truthy values to the safe side, and default=False sends every out-of-contract value to the unsafe one.

I am not asking for this in any of the four PRs. It is a different branch of the same function, and keeping realtime from raising may well be deliberate. Recording it because after whichever one merges, _approvals.py still has one path where an unanswered approval setting resolves to "no".

[xixi-box]

Thanks for auditing the size claim — good to have the split on the record.

On the strict=False branch: agreed it is the same unanswered-question shape one level up, and agreed it does not belong in any of the four PRs — #4848's scope contract deliberately kept strict semantics untouched, and whether realtime should fail closed (require approval) or raise like the run path on an out-of-contract setting is a design call for maintainers to make after one of these merges. Worth keeping on the issue record either way; the falsy-by-default direction makes it more than a style question.

[seratch]

Thanks for investigating. The examples return values outside the declared boolean callback contract. They establish the current truthiness behavior, but do not establish an SDK-owned untyped input boundary or a bypass with a supported callback result. Please keep approval decisions explicitly boolean in application code. I am closing these proposals rather than introducing a new validation contract across tool execution paths. A concrete supported-path bypass would warrant reopening the issue.