fix(core): fail closed on non-bool needs_approval predicate results

[xixi-box]

Summary

This pull request fixes #4845 by making a callable needs_approval predicate fail closed when its resolved return value is not a bool.

A predicate that falls off the end of a branch returns None, and evaluate_needs_approval_setting coerced the result with bool(), so bool(None) became False and the guarded tool executed without any approval request. Every other unanswerable approval question in the feature already fails closed: a predicate that raises is caught in tool planning, and uninspectable arguments short-circuit to "requires approval" (the fix for #3863 in #3867). A non-bool result is the same kind of non-answer, so the shared helper now treats any resolved non-bool result (for example None, "", or 0) as requiring approval.

The change is a 5-line rule in the single shared helper (src/agents/util/_approvals.py), so it applies uniformly to Runner function tools, shell tools, apply-patch tools, custom tools, and Realtime sessions. MCP callable require_approval policies are routed through the same helper in src/agents/mcp/server.py, so a policy returning a non-bool there also fails closed. Behavior for predicates that return actual booleans is unchanged, and truthy non-bool returns keep the released outcome of requesting approval; only falsy non-bool results flip from silent execution to an approval interruption. strict continues to govern only invalid setting types, so the strict=False Realtime path gets the same fail-closed rule.

A separately timed docs-only PR will extend the existing fail-closed sentence in docs/human_in_the_loop.md to cover non-bool results, since that wording describes behavior not yet in a published release.

Test plan

• Added regression tests in tests/test_hitl_error_scenarios.py: parametrized async predicate returning None, "", 0, and a truthy string (the truthy case pins the released compat behavior), plus a sync-predicate None case, all asserting an approval interruption with the tool not executed.
• Added a Realtime regression test in tests/realtime/test_session.py asserting a None predicate result produces RealtimeToolApprovalRequired and does not invoke the tool.
• Added MCP regression tests in tests/mcp/test_mcp_util.py: sync and async require_approval policies returning None require approval (the policy still runs and receives the tool), and a sync policy returning False keeps the released run behavior.
• Verified the issue reproducer: with the fix, the returns None row changes from ran=YES -> no approval requested to asked for approval, while all three control rows are unchanged.
• Focused approval suites pass on both Runner and Realtime paths (including all pre-existing bool-predicate tests).

Issue number

Fixes #4845

Checks

• I've added new tests, if relevant
• I've run .agents/skills/code-change-verification/scripts/run.sh
• I've confirmed all verification steps pass
• If using Codex, I've run /review before submitting this PR

[xixi-box]

Quick context for reviewers, since #4846 and #4847 also propose fixes for #4845 and I'd like to make comparison easy:

Scope and precedent. This follows the fail-closed precedent the maintainers set for the sibling issue #3863 (fixed in #3867 by treating uninspectable arguments as unanswered), which is also the semantics I proposed in the #4845 thread when I took the issue. A non-bool result is the same kind of non-answer, so the two cases now behave consistently.

One shared helper, every surface. The rule lives in evaluate_needs_approval_setting (src/agents/util/_approvals.py), the single source of truth for callable needs_approval evaluation, so the fix covers Runner function tools, shell, apply-patch, custom tools, and Realtime sessions at once — no per-surface call-site changes needed. Realtime keeps its strict=False semantics untouched: strict continues to govern only invalid setting types, while non-bool results now fail closed uniformly.

Compatibility. Predicates returning actual booleans keep exact v0.22.0 behavior, and truthy non-bool returns (e.g. a reason string meaning "yes") keep requesting approval — only the falsy half, the dangerous one, flips from silent execution to an approval interruption. The public bool | Callable[..., MaybeAwaitable[bool]] contract is unchanged.

Tests. Regression tests cover both Runner (tests/test_hitl_error_scenarios.py, parametrized None/""/0/truthy-string plus a sync-predicate case) and Realtime (tests/realtime/test_session.py, asserting RealtimeToolApprovalRequired with the tool not invoked); the truthy-string case pins the released compat behavior. The issue's own reproducer now prints asked for approval on the returns None row with all three control rows unchanged.

Verification. Beyond the focused suites, this went through two independent clean reviews on the full diff (approval-gate security and compatibility/parity specialties, with sibling scans over every evaluate_needs_approval_setting consumer) and the repository format/lint/typecheck/test stack; type-check and test failures observed locally were confirmed identical on a clean main checkout, i.e. pre-existing environment issues rather than anything introduced here.

[sylvesterkaczmarek]

Hi, this fixes the shared function/Realtime evaluator, but MCP approval policies still use _get_needs_approval_for_tool() in src/agents/mcp/server.py, which ends in return bool(result). A policy returning None there can still become False and run without approval. Could MCP route through evaluate_needs_approval_setting() too, with an MCP regression?

[xixi-box]

Good catch — that site had the same defect and is now fixed in 69ce2ba.

_get_needs_approval_for_tool no longer applies its own bool() coercion: the bridged _needs_approval closure now routes the callable policy result through evaluate_needs_approval_setting(), so a policy that returns None (or any other non-bool) requires approval instead of running the guarded MCP tool. Sync and async policies are both covered because the shared evaluator resolves awaitables; strict is irrelevant on this path since the setting here is always callable.

Regression coverage added in tests/mcp/test_mcp_util.py:

• sync and async require_approval policies returning None now require approval (and the policy still runs and receives the tool, preserving the bridging contract),
• a sync policy returning False keeps the released run behavior (bool results untouched),
• the pre-existing bool-policy tests continue to pass (145 tests across tests/mcp/test_mcp_util.py and tests/mcp/test_mcp_approval.py).

Thanks for flagging the gap.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 69ce2ba54a

ℹ️ 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".

[chatgpt-codex-connector]

Preserve the parametrized sync-policy selector

When the async_policy=False case runs, the nested async def async_policy has already rebound the parameter name, so this condition is always truthy and both parametrized cases install the async policy. The newly changed sync non-bool MCP path therefore remains untested despite the sync case label; rename the boolean selector (for example, to is_async) so the false case selects sync_policy.

AGENTS.md reference: AGENTS.md:L156-L156

Useful? React with 👍 / 👎.

[xixi-box]

Fixed in 2c9a625 — thanks.

The local async def async_policy was shadowing the async_policy boolean parameter, so async_policy if async_policy else sync_policy always selected the async policy and the sync parametrized case was silently testing the async path. The selector is renamed to is_async, so the sync case now exercises the non-bool sync policy path through the shared evaluator and the async case the awaitable one; both pass.

[xixi-box]

@codex review

The two follow-up commits since your last review pass may be worth another look: 69ce2ba routes the MCP callable require_approval policy through evaluate_needs_approval_setting() (fixing the _get_needs_approval_for_tool gap flagged by @sylvesterkaczmarek) with new MCP regression tests, and 2c9a625 renames the parametrized selector to is_async to fix the shadowing you reported.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 2c9a6251ba

ℹ️ 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".