diff --git a/tests/tools/test_approval_cron_block.py b/tests/tools/test_approval_cron_block.py new file mode 100644 index 000000000..152d9243a --- /dev/null +++ b/tests/tools/test_approval_cron_block.py @@ -0,0 +1,78 @@ +"""Tests for approval cron-context blocking (#1542, #1554). + +Verifies that _is_unattended_context() and _cron_blocked_result() correctly +convert pending_approval into non-retryable blocked results in cron/subagent +contexts, preventing the retry spirals that cause 44% of terminal failures. + +#1554: _is_unattended_context() now requires a *positive* signal +(HERMES_CRON_SESSION or HERMES_SUBAGENT) instead of defaulting to True +when nobody is "home" — the catch-all broke CI test environments. +""" + +from __future__ import annotations + +from unittest.mock import patch + +import pytest + +from tools.approval import _cron_blocked_result, _is_unattended_context + + +class TestIsUnattendedContext: + """_is_unattended_context() detects cron/subagent via positive signal.""" + + def test_cron_session_is_unattended(self): + """HERMES_CRON_SESSION=1 → always unattended.""" + with patch( + "tools.approval.env_var_enabled", + side_effect=lambda v: v == "HERMES_CRON_SESSION", + ): + assert _is_unattended_context() is True + + def test_subagent_is_unattended(self): + """HERMES_SUBAGENT=1 → unattended (#1554 positive signal).""" + with patch( + "tools.approval.env_var_enabled", + side_effect=lambda v: v == "HERMES_SUBAGENT", + ): + assert _is_unattended_context() is True + + def test_interactive_cli_not_unattended(self): + """Interactive CLI with no cron → not unattended.""" + with patch("tools.approval.env_var_enabled", return_value=False): + assert _is_unattended_context() is False + + def test_gateway_not_unattended(self): + """Gateway session → not unattended.""" + with patch("tools.approval.env_var_enabled", return_value=False): + assert _is_unattended_context() is False + + def test_no_context_not_unattended(self): + """No positive signal → NOT unattended (#1554 fix). + + Previously defaulted to True (catch-all "nobody's home"), which broke + CI test environments. Now requires HERMES_CRON_SESSION or + HERMES_SUBAGENT. + """ + with patch("tools.approval.env_var_enabled", return_value=False): + assert _is_unattended_context() is False + + +class TestCronBlockedResult: + """_cron_blocked_result() builds a non-retryable blocked response.""" + + def test_returns_blocked_status(self): + result = _cron_blocked_result("test desc", "rm -rf /") + assert result["status"] == "blocked" + assert result["approved"] is False + assert result["approval_pending"] is False + + def test_message_says_do_not_retry(self): + result = _cron_blocked_result("desc", "cmd") + assert "Do NOT retry" in result["message"] + + def test_preserves_command_and_desc(self): + result = _cron_blocked_result("my desc", "my cmd", pattern_key="pk") + assert result["command"] == "my cmd" + assert result["description"] == "my desc" + assert result["pattern_key"] == "pk" diff --git a/tools/approval.py b/tools/approval.py index 7a03fd66d..db3790d3f 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -244,6 +244,45 @@ def _is_gateway_approval_context() -> bool: return True return bool(_get_session_platform()) + +def _is_unattended_context() -> bool: + """True when no human or gateway is available to approve commands (#1542). + + In cron/subagent contexts, returning ``pending_approval`` causes the agent + to retry the blocked command 3-4x (44% of terminal failures). Callers + should return a non-retryable ``blocked`` status instead. + + Requires a **positive signal** (``HERMES_CRON_SESSION`` or + ``HERMES_SUBAGENT``) rather than defaulting to True when nobody is + "home" — the catch-all default was too aggressive and broke CI test + environments that are non-interactive, non-gateway, and non-cron + (#1554). + """ + if env_var_enabled("HERMES_CRON_SESSION"): + return True + if env_var_enabled("HERMES_SUBAGENT"): + return True + return False + + +def _cron_blocked_result(desc, command, pattern_key=""): + """Build a non-retryable ``blocked`` result for cron/subagent (#1542).""" + return { + "approved": False, + "status": "blocked", + "approval_pending": False, + "command": command, + "description": desc, + "pattern_key": pattern_key, + "message": ( + f"BLOCKED: {desc} — no user or gateway present to approve in this " + f"context. Do NOT retry; find an alternative approach using " + f"file/search tools instead of terminal, or set " + f"approvals.cron_mode: approve in config.yaml." + ), + } + + # Sensitive write targets that should trigger approval even when referenced # via shell expansions like $HOME or $HERMES_HOME, or by the resolved absolute # active profile home path such as /home/hermes/.hermes/config.yaml. The @@ -3536,6 +3575,10 @@ def check_all_command_guards(command: str, env_type: str, # Return approval_required for backward compat. Redact secrets in the # user-facing copy — the raw `command` is preserved for execution and # the allowlist keys off pattern_key, so redaction is display-only. + # + # #1542: In unattended contexts, return non-retryable blocked. + if _is_unattended_context(): + return _cron_blocked_result(combined_desc, command, pattern_key=primary_key) from agent.redact import redact_sensitive_text _disp_command = redact_sensitive_text(command) _disp_combined_desc = redact_sensitive_text(combined_desc) @@ -3762,6 +3805,10 @@ def check_execute_code_guard(code: str, env_type: str, if notify_cb is None: # No gateway callback registered (e.g. ask-mode without a notifier): # surface a pending approval for backward compatibility. + # + # #1542: In unattended contexts, return non-retryable blocked. + if _is_unattended_context(): + return _cron_blocked_result(display_description, display_command, pattern_key=pattern_key) pending_data = { "command": display_command, "pattern_key": pattern_key,