fix(approval): honour tirith_fail_open=false on Tirith ImportError (#20733)

check_all_command_guards() swallowed ImportError from tools.tirith_security
with an unconditional pass, leaving tirith_result["action"] as "allow"
regardless of security.tirith_fail_open.  When an operator sets
tirith_fail_open: false they have explicitly opted into fail-closed
behaviour; a missing or broken Tirith module must not silently permit
command execution.

Inside the except ImportError handler, read the live security config.
When tirith_enabled is true and tirith_fail_open is false, synthesise a
"warn"-action Tirith result so the command flows through the normal
approval path (prompt the user, or block in cron/gateway contexts)
instead of bypassing it.  The default tirith_fail_open: true behaviour
is unchanged.

Adds three regression tests to tests/tools/test_approval.py:
- fail_open=true  + ImportError → silently allowed (no regression)
- fail_open=false + ImportError → approval callback invoked, command denied
- tirith_enabled=false           → always allowed regardless of fail_open

Fixes #20733

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

# Conflicts:
#	tests/tools/test_approval.py

tests/tools/test_approval.py

@@ -1826,3 +1826,98 @@ class TestApprovalTimeoutIsNotConsent:
         assert last_post.get("choice") == "timeout", (
             f"hook choice should be 'timeout' on no-response, got {last_post.get('choice')!r}"
         )
+
+
+class TestTirithImportErrorFailOpenPolicy:
+    """Regression guard for #20733.
+
+    When ``tools.tirith_security`` cannot be imported, ``check_all_command_guards``
+    must honour the ``security.tirith_fail_open`` config knob:
+
+    * ``tirith_fail_open: true``  (default) → allow, no approval prompt.
+    * ``tirith_fail_open: false`` → surface a Tirith-style warning through
+      the normal approval flow so the command is not silently permitted.
+    """
+
+    def _make_failing_import(self, real_import):
+        """Return a builtins.__import__ replacement that raises for tirith."""
+        def _fake(name, *args, **kwargs):
+            if name == "tools.tirith_security":
+                raise ImportError("simulated tirith import failure")
+            return real_import(name, *args, **kwargs)
+        return _fake
+
+    def test_fail_open_true_allows_silently_on_import_error(self):
+        """Default fail-open: ImportError is silently swallowed, command allowed."""
+        import builtins
+        from unittest.mock import patch as _patch
+        from tools.approval import check_all_command_guards
+
+        cfg = {
+            "approvals": {"mode": "manual"},
+            "security": {"tirith_enabled": True, "tirith_fail_open": True},
+        }
+        real_import = builtins.__import__
+        with _patch("builtins.__import__", side_effect=self._make_failing_import(real_import)):
+            with _patch("hermes_cli.config.load_config", return_value=cfg):
+                with _patch("tools.approval.detect_dangerous_command", return_value=(False, None, None)):
+                    with mock_patch.dict("os.environ", {"HERMES_INTERACTIVE": "1"}, clear=False):
+                        result = check_all_command_guards("echo hello", "local")
+
+        assert result.get("approved") is True
+
+    def test_fail_open_false_escalates_to_approval_on_import_error(self):
+        """Fail-closed: ImportError must NOT silently allow when tirith_fail_open=false."""
+        import builtins
+        from unittest.mock import patch as _patch
+        from tools.approval import check_all_command_guards
+
+        cfg = {
+            "approvals": {"mode": "manual"},
+            "security": {"tirith_enabled": True, "tirith_fail_open": False},
+        }
+        calls = []
+
+        def approval_callback(command, description, **kwargs):
+            calls.append({"command": command, "description": description})
+            return "deny"
+
+        real_import = builtins.__import__
+        with _patch("builtins.__import__", side_effect=self._make_failing_import(real_import)):
+            with _patch("hermes_cli.config.load_config", return_value=cfg):
+                with _patch("tools.approval.detect_dangerous_command", return_value=(False, None, None)):
+                    with mock_patch.dict("os.environ", {"HERMES_INTERACTIVE": "1"}, clear=False):
+                        result = check_all_command_guards(
+                            "echo hello",
+                            "local",
+                            approval_callback=approval_callback,
+                        )
+
+        # The user must have been consulted — the command should NOT be silently allowed.
+        assert result.get("approved") is not True or calls, (
+            "Command was silently allowed despite tirith_fail_open=false and Tirith import failure. "
+            "This is the bug described in issue #20733."
+        )
+        # Specifically: user denied via callback, so approved must be False.
+        assert result.get("approved") is False
+        assert calls, "Approval callback was never invoked — command slipped through silently"
+        assert "tirith" in calls[0]["description"].lower() or "unavailable" in calls[0]["description"].lower()
+
+    def test_tirith_disabled_skips_fail_open_check(self):
+        """When tirith_enabled=false, ImportError is irrelevant — allow without prompt."""
+        import builtins
+        from unittest.mock import patch as _patch
+        from tools.approval import check_all_command_guards
+
+        cfg = {
+            "approvals": {"mode": "manual"},
+            "security": {"tirith_enabled": False, "tirith_fail_open": False},
+        }
+        real_import = builtins.__import__
+        with _patch("builtins.__import__", side_effect=self._make_failing_import(real_import)):
+            with _patch("hermes_cli.config.load_config", return_value=cfg):
+                with _patch("tools.approval.detect_dangerous_command", return_value=(False, None, None)):
+                    with mock_patch.dict("os.environ", {"HERMES_INTERACTIVE": "1"}, clear=False):
+                        result = check_all_command_guards("echo hello", "local")
+
+        assert result.get("approved") is True

tools/approval.py

@@ -1600,7 +1600,40 @@ def check_all_command_guards(command: str, env_type: str,
         from tools.tirith_security import check_command_security
         tirith_result = check_command_security(command)
     except ImportError:
-        pass  # tirith module not installed — allow
+        # Tirith module not installed.  When tirith_fail_open is True (the
+        # default) we silently allow, matching the pre-existing behaviour.
+        # When tirith_fail_open is False the operator has explicitly opted into
+        # fail-closed; an import failure must not silently grant access, so we
+        # synthesize a warn result that will be surfaced to the user through the
+        # normal approval flow.  Fixes #20733.
+        _tirith_fail_open = True  # safe default if config is unreadable
+        try:
+            from hermes_cli.config import load_config as _load_cfg
+            _sec = (_load_cfg() or {}).get("security", {}) or {}
+            _tirith_enabled = _sec.get("tirith_enabled", True)
+            if _tirith_enabled:
+                _tirith_fail_open = _sec.get("tirith_fail_open", True)
+        except Exception:
+            pass
+        if not _tirith_fail_open:
+            tirith_result = {
+                "action": "warn",
+                "findings": [
+                    {
+                        "rule_id": "tirith-import-error",
+                        "severity": "HIGH",
+                        "title": "Tirith security module unavailable",
+                        "description": (
+                            "The Tirith security scanner could not be imported. "
+                            "Because security.tirith_fail_open is false, this "
+                            "command cannot be silently allowed. Approve only if "
+                            "you have verified the command is safe."
+                        ),
+                    }
+                ],
+                "summary": "Tirith unavailable (fail-closed)",
+            }
+        # else: tirith_fail_open is True — allow as before (tirith_result stays "allow")
 
     # Dangerous command check (detection only, no approval)
     is_dangerous, pattern_key, description = detect_dangerous_command(command)