diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index 3500ede939..1b2f395c74 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1016,14 +1016,17 @@ class TestWebhookApprovalExclusion: class TestUnattendedPlatformExecAskLeak: """A gateway process sets HERMES_EXEC_ASK=1 for EVERY session it serves, so an api_server/webhook turn reads as "ask" in ``_presence()`` even though the platform was - already excluded from gateway approval contexts. Without a registered notify callback - (non-streaming /v1/chat/completions, webhook, msgraph_webhook) the ask branch parks the - approval as ``pending_approval`` that no client on that endpoint can ever answer, and - ``approvals.unattended_mode`` never runs — the unanswerable stall of #100532. + already excluded from gateway approval contexts. Without a surface that can answer the + card — non-streaming /v1/chat/completions registers no notify callback, and + webhook/msgraph_webhook register one on every turn yet render no approval surface and + have no reader — the ask branch blocks for the full approval timeout with no human who + can resolve it, and ``approvals.unattended_mode`` never runs (#100532). - Fix: ``_presence()`` drops ``is_ask`` for unattended-platform sessions without a notify - callback, so the unattended branch resolves instantly from ``approvals.unattended_mode``. - ``/v1/runs`` and streaming chat completions register a callback and keep the ask path. + Fix: ``_presence()`` drops ``is_ask`` for unattended-platform sessions that no client can + answer — no notify callback on api_server, or a notifier-blind platform (webhook, + msgraph_webhook) where the TurnRunner lane registers one unconditionally — so the + unattended branch resolves instantly from ``approvals.unattended_mode``. ``/v1/runs`` and + streaming chat completions register a callback and keep the ask path. """ def _isolate(self, monkeypatch, *, platform="api_server"): @@ -1100,6 +1103,48 @@ class TestUnattendedPlatformExecAskLeak: _, _is_cli, _is_gateway, is_ask = approval_mod._presence() assert is_ask is True + def test_webhook_registered_notifier_still_resolves_unattended(self, monkeypatch): + """webhook/msgraph_webhook residual of #100532: the generic TurnRunner lane registers a + notify callback for *every* inbound turn (``_run_conversation_with_approval`` registers + unconditionally), yet the WebhookAdapter renders no ``send_exec_approval``/``/approve`` + surface and the inbound lane is ``POST -> 202`` with no reader — so a registered + notifier there does not mean anyone can answer. The ask leak must resolve from + ``approvals.unattended_mode`` instead of blocking the full approval timeout.""" + import tools.approval as approval_mod + from tools.approval import check_all_command_guards + + self._isolate(monkeypatch, platform="webhook") + monkeypatch.setattr(approval_mod, "_smart_verdict", + lambda *a, **k: pytest.fail("guardian ran after unattended resolution")) + + def _no_human(data): + pytest.fail("approval card delivered to a platform with no approval transport") + + approval_mod.register_gateway_notify("test-unattended-ask-leak", _no_human) + try: + result = check_all_command_guards("pkill -9 -f soffice", "local") + finally: + approval_mod.unregister_gateway_notify("test-unattended-ask-leak") + + assert result["approved"] is False + assert result.get("status") != "pending_approval" + assert result.get("approval_pending") is not True + assert "unattended platform" in result["message"] + assert "approvals.unattended_mode" in result["message"] + + def test_msgraph_webhook_registered_notifier_drops_ask(self, monkeypatch): + """Same shape at the ``_presence()`` level for msgraph_webhook: the TurnRunner-registered + notifier must not keep the ask branch alive on a notifier-blind platform.""" + import tools.approval as approval_mod + + self._isolate(monkeypatch, platform="msgraph_webhook") + approval_mod.register_gateway_notify("test-unattended-ask-leak", lambda data: None) + try: + _, _is_cli, _is_gateway, is_ask = approval_mod._presence() + assert is_ask is False + finally: + approval_mod.unregister_gateway_notify("test-unattended-ask-leak") + class TestNormalizationBypass: """Obfuscation techniques must not bypass dangerous command detection.""" diff --git a/tools/approval.py b/tools/approval.py index 8df609aa3a..f3400a8a9a 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -914,23 +914,30 @@ def _presence(approval_callback=None) -> tuple: HERMES_INTERACTIVE=1 for sudo prompts, and a gateway sets HERMES_EXEC_ASK=1 at startup and passes its environ to every external cron worker (#110932) — in neither can a human answer the card, so the gate must resolve from ``approvals._mode`` instead of parking on a - pending approval. Unattended *platforms* keep ``is_ask`` only while the session has a - registered approval notifier: ``/v1/runs`` and streaming chat completions bridge one - (``approval.request`` → ``POST /v1/runs/{id}/approval``). Without a notifier no client on - the platform can ever answer, so ``is_ask`` drops too and the unattended branch resolves - from ``approvals.unattended_mode`` instead of parking an unanswerable card (#100532).""" + pending approval. Unattended *platforms* keep ``is_ask`` only while a client on the session + could actually answer: ``/v1/runs`` and streaming chat completions bridge an approval + notifier (``approval.request`` → ``POST /v1/runs/{id}/approval``), so on api_server a + registered notifier is the discriminator. webhook/msgraph_webhook turns run through the + generic TurnRunner lane, which registers a notifier for *every* turn — but those adapters + render no approval surface and the inbound lane has no reader, so there ``is_ask`` drops + regardless and the unattended branch resolves from ``approvals.unattended_mode`` instead of + parking an unanswerable card (#100532).""" approval_callback = _resolve_cli_approval_callback(approval_callback) is_cli, is_gateway = _is_interactive_cli(), _is_gateway_approval_context() is_ask = env_var_enabled("HERMES_EXEC_ASK") if _is_single_query_approval_context() or _is_cron_approval_context(): is_cli = is_gateway = is_ask = False elif is_ask and _is_unattended_platform_approval_context() \ - and _gateway_notify_cb(get_current_session_key()) is None: + and (_gateway_notify_cb(get_current_session_key()) is None + or _get_session_platform() in approval_context._NOTIFIER_BLIND_APPROVAL_PLATFORMS): # start_gateway() sets HERMES_EXEC_ASK process-wide, so an api_server/webhook turn # inside that gateway still reads as "ask" here even though _is_gateway_approval_context() # already excluded the platform. /v1/runs and streaming chat register their notify # callback before the turn runs; a session with none has no surface that can resolve an - # approval — resolve from approvals.unattended_mode, never a pending no one can answer. + # approval. webhook/msgraph_webhook are notifier-blind: the generic TurnRunner lane + # registers one for every turn while the adapter has no ``send_exec_approval`` and the + # POST -> 202 lane has no reader, so a registered callback there answers nothing either + # — resolve from approvals.unattended_mode, never a pending no one can answer. is_ask = False return approval_callback, is_cli, is_gateway, is_ask diff --git a/tools/approval_context.py b/tools/approval_context.py index a2b60094f0..b28b820c9a 100644 --- a/tools/approval_context.py +++ b/tools/approval_context.py @@ -146,6 +146,15 @@ def _is_unattended_platform_approval_context() -> bool: return _get_session_platform() in _UNATTENDED_APPROVAL_PLATFORMS +# Platforms where a *registered* gateway notify callback still does not mean a human can answer: +# the generic TurnRunner lane registers one for every inbound turn +# (``_run_conversation_with_approval`` registers unconditionally, no platform branch), while the +# adapter renders no ``send_exec_approval``/``/approve`` surface and the inbound lane is a +# fire-and-forget ``POST -> 202`` with no reader. Notifier presence is only a meaningful +# "someone can answer" discriminator on api_server, whose turn paths choose whether to register one. +_NOTIFIER_BLIND_APPROVAL_PLATFORMS = frozenset({"webhook", "msgraph_webhook"}) + + def _is_single_query_approval_context() -> bool: """True for a single-query (-q) session: ``hermes chat -q`` exports ``HERMES_INTERACTIVE=1`` (so sudo password prompts work) but nobody is waiting