fix(approvals): fail closed when a callable needs_approval returns a non-bool

[ayaangazali]

Summary

evaluate_needs_approval_setting coerces the predicate's answer with bool():

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)

A callable that falls through a branch returns None, bool(None) is False, and the guarded tool runs with no approval requested. The declared contract is Callable[..., MaybeAwaitable[bool]], so a non-bool answer does not mean "no approval needed", it means the predicate did not answer the question.

Measured on main at 89c02c82, asking the only question that matters, whether the guarded tool actually ran without approval:

needs_approval returns      asked_approval   tool_ran
True   (control)            True             no
False  (control)            False            YES
None   (unhandled branch)   False            YES     <- runs unapproved
""                          False            YES
0                           False            YES

After the change the two controls are unchanged and every non-bool answer requires approval instead.

This is the same shape as #3867, "fail closed on invalid callable approval arguments", where an approval question that cannot be answered results in return True rather than a permissive default. That fix handled the case where the predicate cannot be given valid arguments; this one handles the case where it is given valid arguments and still does not answer.

Behaviour change worth stating plainly: a predicate that returns a falsy non-bool such as 0 or "" and today means "do not ask" will now ask. That is deliberate, it is out of contract either way, and the safe direction for a security gate is to ask.

Test plan

tests/test_hitl_error_scenarios.py, next to the #3867 tests and following their shape.

• test_callable_function_approval_fails_closed_for_non_bool_verdict, parametrized over None, "", 0, [] and {}. Each asserts the run was interrupted for approval and that the tool's on_invoke_tool never recorded an invocation, so it asserts the outcome rather than an intermediate value.
• test_callable_function_approval_honors_real_bool_verdicts, parametrized over True and False, pinning that genuine answers keep their meaning: True interrupts and does not run, False runs.

Verified the first fails without the source change by reverting src/: all five parametrizations fail. The two bool controls pass either way, which is the expected split for a test that also pins existing behaviour.

.agents/skills/code-change-verification/scripts/run.sh passes end to end: format, lint, typecheck and the full suite.

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

---

@mahirhir wrote #4845 with the mechanism and the fail-closed argument already worked out, including the controls, so the analysis is his and I only implemented and re-measured it. Happy to step aside if he would rather carry it. The judgement I would most like checked is treating every non-bool the same way rather than special casing None: it matches the declared return type and avoids a second rule, but it does change behaviour for anyone returning 0 today. I'm a freshman in college, so if you would rather this raised a UserError like the invalid-setting branch just above it, say so and I will switch it.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.