fix(approval): fail closed when no approval responder can exist (no_responder)

[a1398394385]

Fixes #87488

Summary

The three no-notify_cb fallback branches in tools/approval.py — the dangerous-command gate, the aggregated command guard, and the execute_code guard — used to call submit_pending() and return status=pending_approval. That promise was unkeepable: approval._pending has no consumer anywhere in the codebase (the TUI replay path reads _gateway_queues, a different store; gateway /approve resolves only the gateway runner's own queue). A headless process escalating a dangerous operation got back "Asking the user for approval" from a user that mathematically cannot answer. The task parked forever, and in the kanban case the per-minute heartbeat kept the wedge below the stale-claim reclaim — the worker incident on #87183, root-caused in #87488.

This PR makes those branches fail closed immediately via a shared _no_responder_block_result() helper: approved=False, outcome=no_responder, user_consent=False, and the standard BLOCKED message shape ("do NOT retry / do NOT rephrase / do NOT attempt the same outcome via a different path") so the LLM treats it as terminal and the run fails cleanly instead of idling. No _pending entries are written on these paths. Full root cause, forensics, and a deterministic API-level repro are in #87488.

Why deny immediately rather than after approvals.timeout: a queued entry with no consumer is unresolvable either way; the countdown only buys an occupied worker slot. The notify_cb path's timeout semantics are untouched.

Behavior changes

Context	Before	After
headless + HERMES_EXEC_ASK=1, dangerous command (all three guards)	hang forever (pending nobody can resolve)	immediate deny, outcome=no_responder
headless + ask-leak, interactive CLI, the two command guards	local panel (since e37a0321e / #86043)	unchanged
headless + ask-leak, interactive CLI, execute_code	hang forever	immediate deny — see edge note below
gateway session with notify_cb	deny after approvals.timeout	unchanged
request_tool_approval plugin escalation, headless	fail-closed (fail_closed_when_no_human)	unchanged
headless without ask-mode, dangerous command	fail-open auto-approve	unchanged — out of scope, see #76428

Edge note on the third row: check_execute_code_guard has no CLI fall-through today (that is #86270, still open), so an interactive-CLI execute_code ask-leak previously hit the same unresolvable pending. It now denies immediately. Strictly better than hanging; once #86270 lands, that case paints the local panel instead and this branch becomes headless-only.

Combination effects with sibling PRs

These four open PRs touch the same region; none of them subsumes this fix (per-PR analysis in the dedup comment on #87488):

• fix(kanban): drop approval context env vars from spawned workers #63189 (kanban workers stop inheriting approval env markers) closes the most common entry path into this bug. It composes cleanly with this PR, but note the ordering effect: fix(kanban): drop approval context env vars from spawned workers #63189 moves workers out of the ask-leak row and into the fail-open row of the matrix above (headless, no ask-mode → auto-approve) unless approvals.noninteractive_mode: deny is also set. Entry-point scrubbing alone just relocates workers between the two broken rows; the durable fix for the remaining row is feat(approval): add approvals.noninteractive_mode for headless contexts #76428. Full safety for kanban = fix(kanban): drop approval context env vars from spawned workers #63189 + this PR + feat(approval): add approvals.noninteractive_mode for headless contexts #76428 (deny).
• feat(approval): add approvals.noninteractive_mode for headless contexts #76428 (approvals.noninteractive_mode) is the complementary knob for the fail-open sibling branch and is deliberately not attempted here. This PR adds no config surface; the knob discussion stays in feat(approval): add approvals.noninteractive_mode for headless contexts #76428.
• Harden gateway command approvals with durable atomic approval store #41427 (SQLite durable approval store) rewrites the adjacent submit_pending / _await_gateway_decision region of tools/approval.py, so expect textual conflicts — whichever lands second rebases. Semantically they are compatible: the durable store wires persistence inside _await_gateway_decision (the notify_cb path), which this PR does not touch.
• fix(approval): route check_execute_code_guard through the CLI fall-through too #86270 (CLI fall-through for the execute_code guard) converts this PR's interactive-CLI edge case from immediate-deny to local-panel. Minor textual overlap in check_execute_code_guard; the no_responder branch remains correct and headless-only once it lands.

Residual: submit_pending() is now dead code

After this PR, submit_pending() has zero call sites. _pending survives only as: the module-level dict declaration, the pop in clear_session(), and the not-growth assertions in the new tests. It was kept deliberately: removing it means touching clear_session() and module state for no behavioral gain, and #41427 rewrites this exact region — a follow-up deleting it should be sequenced against whichever of #41427 / this PR lands second. Happy to do that cleanup as a separate tiny PR if maintainers prefer.

Tests

The pending fallback was encoded as expected behavior in five files; those contracts are flipped to the terminal block, plus new headless regressions:

• tests/tools/test_cli_approval_exec_ask_leak.py — flipped test_pending_approval_still_used_without_cli_callback → test_headless_ask_mode_without_cli_callback_fails_closed; new test_dangerous_command_headless_without_cli_callback_fails_closed (both assert approval._pending stays empty)
• tests/gateway/test_approve_deny_commands.py — TestFallbackNoCallback asserts outcome=no_responder, no pending_approval status, no approval_pending flag
• tests/tools/test_approval.py — TestApprovalPromptRedaction keeps its redaction assertions on the terminal block shape
• tests/tools/test_execute_code_approval_cluster.py — three flips, including the smart-DENY-no-notifier cases now failing closed and the two smart-deny payload tests asserting no _pending record
• tests/tools/test_modal_sandbox_fixes.py — two Docker host-bind approval tests flipped

Verification (baseline c6dfdcbf8):

pytest tests/tools/test_cli_approval_exec_ask_leak.py tests/gateway/test_approve_deny_commands.py \
       tests/tools/test_execute_code_approval_cluster.py tests/tools/test_modal_sandbox_fixes.py \
       tests/tools/test_approval.py -q
→ 167 passed, 2 failed
ruff check tools/approval.py → clean

The 2 failures reproduce identically on clean origin/main with the same command (git stash → rerun → same 2): one is ordering pollution (test_smart_approval_does_not_allowlist_the_pattern_for_session passes in isolation), one was introduced upstream by c6dfdcbf8 (test_nonrecursive_verification_artifact_cleanup_is_not_dangerous). Zero new failures from this change.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(approval): fail closed when no approval responder can exist (no_responder)

• This is a real behavior change for headless API-server flows: an unanswerable request previously queued as pending_approval (later resolvable via /approve) and now terminates immediately with outcome=no_responder. Please confirm every consumer that branches on status == "pending_approval" / approval_pending was updated (run_agent.py, model_tools.py, gateway session handling, terminal_tool, TUI) — the diff updates the tests, but a stray consumer would now see neither flag and could mis-handle the result.
• check_execute_code_guard no longer includes the code in the blocked message (the old payload rendered a **Code:** block). Since this is a terminal "do not retry" instruction, keeping a redacted excerpt would help the model know exactly which action was refused — command carries it, but the message body itself is now generic.
• Unifying both old fallbacks (approval_required in _run_approval_gate, pending_approval in the guards) into _no_responder_block_result is a welcome cleanup. smart_denied_for_owner still decorates the result — good.
• Minor: the new result drops pattern_keys (the old pending payload in check_all_command_guards included all_keys). If any consumer keyed off pattern_keys for display/allowlist logic, it needs the field preserved.

[a1398394385]

Thanks for looking at this. I went through each point; a couple of notes below, no code changes needed.

On the consumer sweep (point 1): I re-reviewed the consuming code after reading your comment. The only production branch on status == "pending_approval" is terminal_tool.py:2940 (line references below are against this PR's base commit, c6dfdcb, since main moves), and its else-branch handles the new shape by rendering the blocked message, which is the intended path. run_agent / model_tools / gateway don't branch on the flag, and the TUI replay reads get_pending_gateway_approval(), which is backed by _gateway_queues, not the _pending dict this PR stops writing to.

One small thing on the "/approve could later resolve it" part, since it took me a while to untangle too: /approve (gateway/slash_commands.py:5639) resolves runner._pending_approvals, the gateway runner's own dict fed by the notify_cb path. It never read approval._pending. That store has no readers at all — one write site and a pop in clear_session(), nothing else. That's actually how the bug lived as long as it did; details in #87488 if useful.

On the dropped **Code:** block (point 2): I left it out on purpose. The other deny paths in the same guard don't embed the code either, and this branch only fires when there's no human around to read an embed anyway. The redacted command is still in the result dict for programmatic use. Happy to add an excerpt if a maintainer prefers it.

On pattern_keys (point 4): double-checked against that same base commit — the old returned result never carried it. pattern_keys only existed inside the pending_data payload that went into the unread store, so there's no before/after delta there.

(Glad the unification reads well — point 3.)