From aac74be2f11f0fc80a1988bcaacabf40b28a1295 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 2 Aug 2026 18:51:19 -0700 Subject: [PATCH] fix(approval): classify CLI/TUI approval timeouts separately from explicit denials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an approval prompt expired without a response, every CLI-side path collapsed the timeout into the same 'deny' choice as an explicit user refusal, so the agent was told the user denied the action when the user simply never answered. The gateway wait already distinguished the two ('timed out without user response... Silence is not consent.'); this brings the CLI/TUI/ACP surfaces to parity. - prompt_dangerous_approval(): input()-path expiry now returns a distinct 'timeout' choice (still fail-closed). - cli.py _approval_callback + hermes_cli/callbacks.py approval_callback: deadline expiry returns 'timeout' instead of 'deny'. - check_all_command_guards / _run_approval_gate CLI tails: 'timeout' maps to outcome='timeout' with a 'timed out without user response... Silence is not consent.' BLOCKED message (matching the gateway wording); explicit deny keeps outcome='denied' and gains user_consent=False for shape parity. - computer_use: 'timeout' verdict threads through the CLI adapter and yields a 'prompt timed out — the user did not respond' error instead of 'denied by user'. - ACP permissions bridge: FutureTimeout returns 'timeout' (other failures still 'deny'); elicitation maps 'timeout' to 'cancel' like the gateway's unresolved outcome; codex wire mapping documents deny/timeout→decline. - write_approval already treats unknown choices as 'stage, not drop', so a timeout now stages the memory write instead of silently refusing it. Every timeout path remains fail-closed — the action never runs; only the classification reported to the agent changes. --- acp_adapter/permissions.py | 11 +- agent/transports/codex_app_server_session.py | 2 + cli.py | 6 +- hermes_cli/callbacks.py | 2 +- tests/acp/test_permissions.py | 6 +- tests/tools/test_approval.py | 106 +++++++++++++++++++ tools/approval.py | 50 ++++++++- tools/computer_use/tool.py | 8 ++ 8 files changed, 183 insertions(+), 8 deletions(-) diff --git a/acp_adapter/permissions.py b/acp_adapter/permissions.py index 5f29a96725..b10b2a169e 100644 --- a/acp_adapter/permissions.py +++ b/acp_adapter/permissions.py @@ -158,9 +158,16 @@ def make_approval_callback( try: response = future.result(timeout=timeout) - except (FutureTimeout, Exception) as exc: + except FutureTimeout: future.cancel() - logger.warning("Permission request timed out or failed: %s", exc) + logger.warning("Permission request timed out after %ss", timeout) + # Distinct from an explicit deny: the client never answered. + # tools.approval callers report this as "timed out without user + # response" instead of a user denial. + return "timeout" + except Exception as exc: + future.cancel() + logger.warning("Permission request failed: %s", exc) return "deny" if response is None: diff --git a/agent/transports/codex_app_server_session.py b/agent/transports/codex_app_server_session.py index e2ace753bc..384a7de8ab 100644 --- a/agent/transports/codex_app_server_session.py +++ b/agent/transports/codex_app_server_session.py @@ -1259,6 +1259,8 @@ def _approval_choice_to_codex_decision(choice: str) -> str: return "accept" if choice in {"session", "always"}: return "acceptForSession" + # "deny" and "timeout" both map to decline — codex has no wire value for + # "prompt expired"; the Hermes-side messaging already distinguishes them. return "decline" diff --git a/cli.py b/cli.py index 95ce2b18f8..55b94c146a 100644 --- a/cli.py +++ b/cli.py @@ -13229,7 +13229,10 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): self._approval_deadline = 0 self._paint_now() _cprint(f"\n{_DIM} ⏱ Timeout — denying command{_RST}") - return "deny" + self._persist_prompt_summary( + "⚠", "Approval", command, "timed out (no response)", + ) + return "timeout" def _approval_choices(self, command: str, *, allow_permanent: bool = True, smart_denied: bool = False) -> list[str]: @@ -13261,6 +13264,7 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): "session": "approve_session", "always": "always_approve", "deny": "deny", + "timeout": "timeout", }.get(verdict, "deny") def _handle_approval_selection(self) -> None: diff --git a/hermes_cli/callbacks.py b/hermes_cli/callbacks.py index 69f4b6d405..aad0542d28 100644 --- a/hermes_cli/callbacks.py +++ b/hermes_cli/callbacks.py @@ -250,4 +250,4 @@ def approval_callback(cli, command: str, description: str) -> str: if hasattr(cli, "_app") and cli._app: cli._app.invalidate() cprint(f"\n{_DIM} ⏱ Timeout — denying command{_RST}") - return "deny" + return "timeout" diff --git a/tests/acp/test_permissions.py b/tests/acp/test_permissions.py index 649a388f01..3d8576d3e2 100644 --- a/tests/acp/test_permissions.py +++ b/tests/acp/test_permissions.py @@ -119,7 +119,7 @@ class TestApprovalBridge: assert result == "always" - def test_timeout_returns_deny_and_cancels_future(self): + def test_timeout_returns_timeout_and_cancels_future(self): loop = MagicMock(spec=asyncio.AbstractEventLoop) request_permission = AsyncMock(name="request_permission") future = MagicMock(spec=Future) @@ -138,7 +138,9 @@ class TestApprovalBridge: scheduled["coro"].close() - assert result == "deny" + # A no-response expiry is classified as "timeout" (still blocked, + # fail-closed) so the agent isn't told the user explicitly refused. + assert result == "timeout" assert scheduled["loop"] is loop assert future.cancel.call_count == 1 diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index 61525aec0d..9c4b0cbc68 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1397,3 +1397,109 @@ class TestApprovalPromptRedaction: # The script's credential must not appear in the user-facing message. assert "sk-proj-abc123xyz4567890abcdef" not in result["message"] assert "sk-proj-abc123xyz4567890abcdef" not in result["command"] + + +class TestCliApprovalTimeoutClassifiedSeparately: + """CLI-path parity for the timeout-vs-deny distinction. + + The gateway wait already reported "timed out without user response"; + the CLI/TUI callback path collapsed a prompt timeout into "deny", so + the agent was told the user *refused* when the user simply never + answered. The prompt now returns a distinct "timeout" choice and both + guard tails classify it with outcome="timeout" + a "Silence is not + consent." message. + """ + + def _interactive_env(self): + return mock_patch.dict( + "os.environ", + {"HERMES_INTERACTIVE": "1"}, + clear=False, + ) + + def test_prompt_returns_timeout_when_input_never_arrives(self): + """The raw input() path returns 'timeout', not 'deny', on expiry.""" + import builtins + from unittest.mock import patch as _patch + + def _hang(_prompt=""): + time.sleep(10) + return "" + + with _patch.object(builtins, "input", _hang): + result = prompt_dangerous_approval( + "rm -rf /var/data", "recursive delete", + timeout_seconds=0.05, + ) + assert result == "timeout" + + def test_guard_classifies_callback_timeout_as_timeout(self, monkeypatch): + """check_all_command_guards: a 'timeout' choice from the CLI callback + yields outcome='timeout' and a no-response message, not 'denied by + user'.""" + from unittest.mock import patch as _patch + from tools import approval as mod + + mod._session_approved.clear() + mod._permanent_approved.clear() + + cfg = {"approvals": {"mode": "manual"}} + with self._interactive_env(): + with _patch("hermes_cli.config.load_config_readonly", return_value=cfg): + result = mod.check_all_command_guards( + "rm -rf /var/data", "local", + approval_callback=lambda *a, **kw: "timeout", + ) + + assert result["approved"] is False + assert result.get("outcome") == "timeout" + assert result.get("user_consent") is False + msg = result["message"] + assert "timed out without user response" in msg + assert "Silence is not consent" in msg + assert "denied" not in msg.lower() + + def test_guard_still_classifies_explicit_deny_as_denied(self): + """Explicit CLI deny keeps outcome='denied' and the denial wording.""" + from unittest.mock import patch as _patch + from tools import approval as mod + + mod._session_approved.clear() + mod._permanent_approved.clear() + + cfg = {"approvals": {"mode": "manual"}} + with self._interactive_env(): + with _patch("hermes_cli.config.load_config_readonly", return_value=cfg): + result = mod.check_all_command_guards( + "rm -rf /var/data", "local", + approval_callback=lambda *a, **kw: "deny", + ) + + assert result["approved"] is False + assert result.get("outcome") == "denied" + assert "denied" in result["message"].lower() + assert "Silence is not consent" not in result["message"] + + def test_run_approval_gate_cli_timeout_is_not_a_denial(self): + """The shared plugin-escalation gate (_run_approval_gate) also + distinguishes a prompt timeout from an explicit deny on the CLI + path.""" + from unittest.mock import patch as _patch + from tools import approval as mod + + mod._session_approved.clear() + mod._permanent_approved.clear() + + cfg = {"approvals": {"mode": "manual"}} + with self._interactive_env(): + with _patch("hermes_cli.config.load_config_readonly", return_value=cfg): + result = mod.request_tool_approval( + "write_file", "plugin flagged this write", + approval_callback=lambda *a, **kw: "timeout", + ) + + assert result["approved"] is False + assert result.get("outcome") == "timeout" + assert result.get("user_consent") is False + assert "timed out without user response" in result["message"] + assert "Silence is not consent" in result["message"] diff --git a/tools/approval.py b/tools/approval.py index f0771b8727..48edfa2f3a 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -2523,7 +2523,10 @@ def prompt_dangerous_approval(command: str, description: str, smart_denied=False) -> str. Legacy callback signatures remain supported when ``smart_denied`` is false. - Returns: 'once', 'session', 'always', or 'deny' + Returns: 'once', 'session', 'always', 'deny', or 'timeout'. + 'timeout' means the prompt expired without a user response — the + action must still be blocked (fail-closed), but callers should + report it as "no response" rather than an explicit user denial. """ if timeout_seconds is None: timeout_seconds = _get_approval_timeout() @@ -2612,7 +2615,10 @@ def prompt_dangerous_approval(command: str, description: str, if thread.is_alive(): print("\n" + t("approval.timeout")) - return "deny" + # Distinct from an explicit deny: the user never answered. + # Callers still block (fail-closed) but tell the agent the + # prompt timed out instead of claiming the user refused. + return "timeout" choice = result["choice"] if smart_denied: @@ -3113,6 +3119,21 @@ def _run_approval_gate( choice = prompt_dangerous_approval(display_target, description, approval_callback=approval_callback) + if choice == "timeout": + return { + "approved": False, + "message": ( + f"BLOCKED: Action timed out without user response. The user " + f"has NOT consented to this action. Do NOT retry it, do NOT " + f"rephrase it, and do NOT attempt the same outcome via a " + f"different path. Silence is not consent." + ), + "pattern_key": pattern_key, + "description": description, + "outcome": "timeout", + "user_consent": False, + } + if choice == "deny": return { "approved": False, @@ -3123,6 +3144,8 @@ def _run_approval_gate( ), "pattern_key": pattern_key, "description": description, + "outcome": "denied", + "user_consent": False, } if choice == "session": @@ -3887,6 +3910,25 @@ def check_all_command_guards(command: str, env_type: str, choice=choice, ) + if choice == "timeout": + breaker_addendum = _denial_breaker_addendum(session_key) + return { + "approved": False, + "message": ( + "BLOCKED: Command timed out without user response. The user " + "has NOT consented to this action. Do NOT retry this " + "command, do NOT rephrase it, and do NOT attempt the same " + "outcome via a different command. Stop the current workflow " + "and wait for the user to respond before taking any further " + "destructive or irreversible action. Silence is not " + f"consent.{breaker_addendum}" + ), + "pattern_key": primary_key, + "description": combined_desc, + "outcome": "timeout", + "user_consent": False, + } + if choice == "deny": breaker_addendum = _denial_breaker_addendum(session_key) return { @@ -4244,6 +4286,10 @@ def request_elicitation_consent( if choice in ("once", "session", "always"): return "accept" + if choice == "timeout": + # Prompt expired without a user response — mirror the gateway's + # unresolved outcome ("cancel") rather than an explicit decline. + return "cancel" return "decline" diff --git a/tools/computer_use/tool.py b/tools/computer_use/tool.py index dbe6fc1558..1422ff67e4 100644 --- a/tools/computer_use/tool.py +++ b/tools/computer_use/tool.py @@ -550,6 +550,14 @@ def _request_approval(action: str, args: Dict[str, Any], if verdict == "always_approve": _session_auto_approve[session_id] = True return None + if verdict == "timeout": + return json.dumps({ + "error": ( + "approval prompt timed out — the user did not respond. " + "Silence is not consent; do not retry without the user." + ), + "action": action, + }) return json.dumps({"error": "denied by user", "action": action})