fix(gateway): fail fast on undeliverable approval notifications

[konsisumer]

What does this PR do?

Stops the 5-minute hang when a dangerous command needs approval but the platform adapter can't deliver the prompt to the user.

_approval_notify_sync (in gateway/run.py) used to swallow SendResult(success=False) and exceptions raised by adapter.send(). The notify_cb returned normally, the approval entry stayed queued, and the agent thread blocked on entry.event.wait(timeout=gateway_timeout) for the full 5 minutes — the user never saw the prompt, and there was no path to resolve it via /approve or /deny.

The fallback send() result is now checked: if scheduling yields no future, send() raises, or SendResult.success is False, we log and re-raise. The existing safety net in tools.approval.check_all_command_guards already catches exceptions from notify_cb, drains the entry from _gateway_queues, and returns a BLOCKED: Failed to send approval request to user result — we just have to reach it. Concretely fixes the /background flow on the API Server adapter (APIServerAdapter.send always returns SendResult(success=False, ...)), but applies to any adapter where push delivery is unavailable.

Related Issue

Fixes #19731

Related: #6059 — implements the dashboard SSE approval stream for the API Server (the long-term push path). This PR is complementary: even once #6059 lands, _approval_notify_sync should fail fast on SendResult(success=False) rather than block on entry.event.wait(), so any future adapter without a push channel surfaces a clear BLOCKED instead of a hang.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• gateway/run.py: in the approval text-send fallback, raise when safe_schedule_threadsafe returns no future, when send() raises, or when the returned SendResult.success is false — so the approval guard fails fast with BLOCKED instead of waiting out gateway_timeout.
• tests/tools/test_approval_plugin_hooks.py: adds test_notify_cb_failure_blocks_fast_without_waiting_for_timeout, which registers a notify_cb mimicking the API Server failure and asserts the guard returns BLOCKED well under the configured gateway_timeout.

How to Test

1. pytest tests/tools/test_approval_plugin_hooks.py -q (the new regression test plus the existing suite).
2. pytest tests/tools/test_approval_plugin_hooks.py tests/gateway/test_approve_deny_commands.py tests/gateway/test_background_command.py tests/gateway/test_session_boundary_security_state.py -q (related approval and background-command suites).
3. Manual repro of the original hang requires an API Server gateway session and isn't reproducible in local pytest, so the regression test simulates the failure pattern at the notify_cb seam.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS (darwin-arm64)

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — or N/A
• I've updated cli-config.yaml.example if I added/changed config keys — or N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Addressing maintainer feedback

• @austinpickett — reformatted this PR description to follow PULL_REQUEST_TEMPLATE.md as requested.
• feat(api_server): wire up dangerous command approval for /v1/runs SSE stream #6059 (raised by @alt-glitch) is OPEN and implements the SSE approval stream for the API Server. This PR is intentionally complementary defense-in-depth, not a duplicate — see the Related Issue section above.
• Note on the pytest tests/ -q checkbox: the PR's own suite (tests/tools/test_approval_plugin_hooks.py) passes locally (4 passed). The branch was rebased onto current main; the earlier CI test failures were on files this PR does not touch (e.g. test_registry_manifest, test_google_chat, test_teams, model-list tests) caused by a stale base, which the rebase resolves.

[konsisumer]

Rebased onto origin/main. Resolved a conflict in tests/tools/test_approval_plugin_hooks.py: #22098 removed test_pre_and_post_fire_on_gateway_surface and test_timeout_reports_timeout_choice from main as stale, so the rebased file now keeps only this PR's new regression test (test_notify_cb_failure_blocks_fast_without_waiting_for_timeout). The gateway/run.py change is unchanged. Local pytest tests/tools/test_approval_plugin_hooks.py is green (4 passed).

Addressing maintainer feedback

• Related: feat(api_server): wire up dangerous command approval for /v1/runs SSE stream #6059 — @alt-glitch noted that feat(api_server): wire up dangerous command approval for /v1/runs SSE stream #6059 implements the dashboard SSE approval stream for the API Server, which is the long-term fix for the underlying gap. This PR is complementary defense-in-depth: even once feat(api_server): wire up dangerous command approval for /v1/runs SSE stream #6059 lands, _approval_notify_sync should still fail fast on SendResult(success=False) rather than block on entry.event.wait() for the full gateway_timeout, so that any future adapter without a push channel surfaces a clear BLOCKED instead of a 5-minute hang.

[konsisumer]

Rebased onto origin/main and reconciled the gateway/run.py conflict: upstream switched the approval text-send to safe_schedule_threadsafe(...), so the PR's _send_result.success fail-fast check is now applied to the future returned by that helper (with a None guard for the unavailable-loop case). Local approval-plugin-hooks tests pass and ruff is clean on the touched files.

[austinpickett]

Please use PULL_REQUEST_TEMPLATE.md

[Copilot]

Pull request overview

Fixes a 5-minute hang in the gateway when a dangerous command requires approval but the platform adapter (e.g., APIServerAdapter) cannot push the prompt. Previously _approval_notify_sync silently discarded SendResult(success=False), leaving the agent thread blocked on event.wait() with no way to resolve. The change makes the notify callback raise when delivery fails, so the existing handler in tools/approval.py drains the queue and returns BLOCKED immediately.

Changes:

• In gateway/run.py, _approval_notify_sync now raises when safe_schedule_threadsafe yields no future, when send() raises, or when the returned SendResult.success is false.
• Adds regression test test_notify_cb_failure_blocks_fast_without_waiting_for_timeout that verifies the guard returns BLOCKED synchronously and drains _gateway_queues.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
gateway/run.py	Propagate adapter send failures from _approval_notify_sync so the approval path fails fast instead of hanging.
tests/tools/test_approval_plugin_hooks.py	New regression test asserting BLOCKED result and queue drain when notify_cb raises.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[konsisumer]

Closing — this PR removes the silent-swallow path in _approval_notify_sync but tests for that behavior still exist in files outside this PR's scope. A maintainer would need to either restore the removed code or remove the obsolete tests; the bot can't do either within scope. If the removal is still wanted, please reopen with the test cleanup attached.