fix(harnesses): fail closed for TOOL_CALL when native policy hook can't reach the server (#579)

* fix(harnesses): fail closed for TOOL_CALL when native policy hook can't reach the server

Fixes #536.

For native harnesses (claude-native, codex-native) the PreToolUse/
PostToolUse hook subprocess is the entire policy-governance layer: it
gates Bash/Write/Edit, the native Skill tool, and connector-native
mcp__* tools by POSTing to /v1/sessions/{id}/policies/evaluate. Every
error/edge path returned exit 0 with no stdout — "no opinion" — so any
condition that prevented a well-formed verdict (server unreachable,
non-2xx, empty body, malformed JSON) silently disabled all DENY/ASK
enforcement. A transient AP outage turned a blocked tool into an
allowed one, with only a stderr line. P0 bypass.

Make the hooks' default phase-aware, mirroring the runner-side fix in
PR #163. Once a session is known to be governed (active session id +
configured ap_server_url) and the evaluate round-trip cannot yield a
usable verdict, a PreToolUse (PHASE_TOOL_CALL) call fails CLOSED with a
deny — the authoritative, only-enforcement-point gate — while
UserPromptSubmit (advisory request gate) and PostToolUse (the tool
already ran) keep failing OPEN. Pre-evaluation short-circuits that mean
the session simply isn't governed (no session, no ap_server_url,
unparseable payload, relay-gated mcp__omnigent__* tools) still emit "no
opinion" so non-Omnigent sessions are never blocked.

The client timeout is intentionally left unchanged: the long timeout
backs the server-side ASK long-poll, and shortening it would break ASK
and reintroduce a fail-open. A hung server still blocks (the safe
direction) rather than failing open.

Changes:
- native_policy_hook.py: new shared fail_closed_hook_output() helper.
- claude_native_hook.py / codex_native_hook.py: the HTTP-error,
  empty-body, and malformed-response branches now fail closed for the
  tool-call gate instead of returning no opinion.
- Tests: unit coverage for the helper plus integration tests asserting
  PreToolUse denies across connect-error/non-2xx/empty/malformed while
  PostToolUse and UserPromptSubmit stay fail-open.

* test/fix(harnesses): address Polly review — clearer non-2xx log, shared test helper, unknown-event guard

Non-blocking follow-ups from the Polly AI review on PR #579:

- Log non-2xx responses distinctly from connection errors. Both native
  hooks now catch httpx.HTTPStatusError before the broad httpx.HTTPError
  branch and log the status code, so a real AP outage (e.g. 503) is
  distinguishable from an unreachable server in production diagnostics.
  Behavior is unchanged — both still fail closed for the tool-call gate.
- Deduplicate the failing-client test stub into
  tests/native_hook_helpers.make_failing_client, imported by both the
  claude- and codex-native hook test modules, so the four failure modes
  can't drift.
- Add an explicit unknown-event test for fail_closed_hook_output
  ("SomeNewEvent" -> None) documenting the fail-open-for-unknowns contract.

omnigent/claude_native_hook.py

@@ -29,6 +29,7 @@ from omnigent.claude_native_bridge import (
 from omnigent.entities.session_resources import terminal_resource_id
 from omnigent.native_policy_hook import (
     evaluation_response_to_hook_output,
+    fail_closed_hook_output,
     hook_payload_to_evaluation_request,
 )
 
@@ -743,9 +744,20 @@ def _main_evaluate_policy(argv: list[str]) -> int:
     ``additionalContext`` (Claude sees the warning but the tool result
     is already committed — PostToolUse hooks are observational).
 
-    On transport failures the hook returns exit 0 with no output,
-    which Claude Code treats as "no opinion". Fail-open design
-    ensures a network blip doesn't block every tool call.
+    Failure handling is phase-aware (mirroring the runner-side default
+    from PR #163). Once the session is known to be governed (an active
+    session id and a configured ``ap_server_url``) and the round-trip to
+    ``/policies/evaluate`` cannot yield a usable verdict — the server is
+    unreachable, returns non-2xx, or returns an empty / malformed body —
+    a ``PreToolUse`` (``PHASE_TOOL_CALL``) call fails CLOSED with a
+    ``deny`` (this hook is the sole enforcement point for native tools, so
+    a transient outage must not silently let a gated call through), while
+    ``UserPromptSubmit`` and ``PostToolUse`` fail OPEN. Pre-evaluation
+    conditions that mean the session simply is not governed — no active
+    session, no ``ap_server_url``, an unparseable hook payload, or an
+    ``mcp__omnigent__*`` tool already gated on the relay path — still
+    return exit 0 with no output ("no opinion") so non-Omnigent tool
+    calls are never blocked.
 
     :param argv: CLI argv after the ``evaluate-policy`` subcommand,
         e.g. ``["--bridge-dir", "/tmp/x"]``.
@@ -800,6 +812,15 @@ def _main_evaluate_policy(argv: list[str]) -> int:
     if status_model:
         context["model"] = status_model
 
+    # The session is governed (active id + ap_server_url) and we have a
+    # policy-relevant event: from here a failure to obtain a usable verdict
+    # fails CLOSED for the tool-call gate (see ``fail_closed_hook_output``).
+    def _fail_closed() -> int:
+        out = fail_closed_hook_output(hook_event)
+        if out is not None:
+            sys.stdout.write(json.dumps(out))
+        return 0
+
     url = f"{ap_server_url.rstrip('/')}/v1/sessions/{url_component(session_id)}/policies/evaluate"
     try:
         with httpx.Client(
@@ -807,16 +828,24 @@ def _main_evaluate_policy(argv: list[str]) -> int:
         ) as client:
             resp = client.post(url, json=eval_request)
             resp.raise_for_status()
+    except httpx.HTTPStatusError as exc:
+        print(
+            f"omnigent evaluate-policy hook: Omnigent returned {exc.response.status_code}",
+            file=sys.stderr,
+        )
+        return _fail_closed()
     except httpx.HTTPError as exc:
         print(f"omnigent evaluate-policy hook: Omnigent request failed: {exc}", file=sys.stderr)
-        return 0
+        return _fail_closed()
     if not resp.content:
-        return 0
+        print("omnigent evaluate-policy hook: empty Omnigent response", file=sys.stderr)
+        return _fail_closed()
 
     try:
         eval_response = resp.json()
     except json.JSONDecodeError:
-        return 0
+        print("omnigent evaluate-policy hook: malformed Omnigent response", file=sys.stderr)
+        return _fail_closed()
 
     hook_output = evaluation_response_to_hook_output(hook_event, eval_response)
     if hook_output is not None:

omnigent/codex_native_hook.py

@@ -26,6 +26,7 @@ from omnigent.codex_native_bridge import (
 )
 from omnigent.native_policy_hook import (
     evaluation_response_to_hook_output,
+    fail_closed_hook_output,
     hook_payload_to_evaluation_request,
 )
 
@@ -74,13 +75,22 @@ def _main_evaluate_policy(argv: list[str]) -> int:
     ``decision: "block"`` for UserPromptSubmit — the request-phase gate
     for native sessions, which drops the prompt before the model runs).
 
-    On any transport or lookup failure the hook returns exit 0 with no
-    output, which Codex treats as "no opinion". This is the deliberate
-    fail-open behavior shared with the Claude-native hook: a network
-    blip must not block every tool call. The complementary fail-loud
-    guard — asserting the hook is actually registered and trusted — lives
-    at session startup in :mod:`omnigent.codex_native_app_server`, not
-    here, because a silently-skipped hook cannot report its own absence.
+    Failure handling is phase-aware (mirroring the runner-side default
+    from PR #163), shared with the Claude-native hook. Once the session is
+    known to be governed (an active session id and a configured
+    ``ap_server_url``) and the round-trip to ``/policies/evaluate`` cannot
+    yield a usable verdict — server unreachable, non-2xx, or an empty /
+    malformed body — a ``PreToolUse`` (``PHASE_TOOL_CALL``) call fails
+    CLOSED with a ``deny`` (this hook is the sole enforcement point for
+    native tools), while ``UserPromptSubmit`` and ``PostToolUse`` fail
+    OPEN. Conditions that mean the session simply is not governed — no
+    bridge state, no ``ap_server_url``, an unparseable payload, or an
+    ``mcp__omnigent__*`` tool already gated on the relay path — still
+    return exit 0 with no output ("no opinion") so non-Omnigent tool calls
+    are never blocked. The complementary fail-loud guard — asserting the
+    hook is actually registered and trusted — lives at session startup in
+    :mod:`omnigent.codex_native_app_server`, not here, because a
+    silently-skipped hook cannot report its own absence.
 
     :param argv: CLI argv after the ``evaluate-policy`` subcommand,
         e.g. ``["--bridge-dir", "/tmp/x"]``.
@@ -139,6 +149,15 @@ def _main_evaluate_policy(argv: list[str]) -> int:
     if model:
         context["model"] = model
 
+    # The session is governed (bridge state + ap_server_url) and we have a
+    # policy-relevant event: from here a failure to obtain a usable verdict
+    # fails CLOSED for the tool-call gate (see ``fail_closed_hook_output``).
+    def _fail_closed() -> int:
+        out = fail_closed_hook_output(hook_event)
+        if out is not None:
+            sys.stdout.write(json.dumps(out))
+        return 0
+
     session_component = urllib.parse.quote(session_id, safe="")
     url = f"{ap_server_url.rstrip('/')}/v1/sessions/{session_component}/policies/evaluate"
     try:
@@ -147,19 +166,30 @@ def _main_evaluate_policy(argv: list[str]) -> int:
         ) as client:
             resp = client.post(url, json=eval_request)
             resp.raise_for_status()
+    except httpx.HTTPStatusError as exc:
+        print(
+            f"omnigent codex evaluate-policy hook: Omnigent returned {exc.response.status_code}",
+            file=sys.stderr,
+        )
+        return _fail_closed()
     except httpx.HTTPError as exc:
         print(
             f"omnigent codex evaluate-policy hook: Omnigent request failed: {exc}",
             file=sys.stderr,
         )
-        return 0
+        return _fail_closed()
     if not resp.content:
-        return 0
+        print("omnigent codex evaluate-policy hook: empty Omnigent response", file=sys.stderr)
+        return _fail_closed()
 
     try:
         eval_response = resp.json()
     except json.JSONDecodeError:
-        return 0
+        print(
+            "omnigent codex evaluate-policy hook: malformed Omnigent response",
+            file=sys.stderr,
+        )
+        return _fail_closed()
 
     hook_output = evaluation_response_to_hook_output(hook_event, eval_response)
     if hook_output is not None:

omnigent/native_policy_hook.py

@@ -34,6 +34,14 @@ _POST_TOOL_USE = "PostToolUse"
 # direct-terminal prompts). It can block the prompt before the model runs.
 _USER_PROMPT_SUBMIT = "UserPromptSubmit"
 
+# Reason surfaced when a tool call is denied because its policy verdict
+# could not be obtained (server unreachable / non-2xx / empty or malformed
+# body). Mirrors the runner-side fail-closed default in
+# ``omnigent.runner.app._evaluate_policy_via_omnigent`` (PR #163).
+_EVAL_UNAVAILABLE_REASON = (
+    "Omnigent policy evaluation unavailable; failing closed for this tool call."
+)
+
 
 def hook_payload_to_evaluation_request(
     hook_event: str,
@@ -218,3 +226,46 @@ def evaluation_response_to_hook_output(
         return None
 
     return None
+
+
+def fail_closed_hook_output(hook_event: str) -> dict[str, object] | None:
+    """
+    Build the fail-closed hook output for an unobtainable policy verdict.
+
+    Called by the per-harness hooks when the ``/policies/evaluate``
+    round-trip cannot produce a usable verdict for an *already-governed*
+    session — the server is unreachable, returns a non-2xx status, or
+    returns an empty / malformed body. Without this the hooks emitted "no
+    opinion" on those paths, silently letting the gated tool run: for
+    native harnesses this hook is the sole enforcement point (it gates
+    Bash / Write / Edit / the native Skill tool / connector-native
+    ``mcp__*`` tools), so a transient outage disabled all DENY/ASK
+    enforcement.
+
+    The default is phase-aware, matching
+    :data:`omnigent.policies.types.FAIL_CLOSED_PHASES` (the runner-side
+    precedent from PR #163) — but expressed in hook-event terms so the
+    lightweight hook subprocess need not import the policy package:
+
+    - ``PreToolUse`` (``PHASE_TOOL_CALL``) fails CLOSED → ``deny``. This is
+      the authoritative pre-execution gate; an unevaluable policy must not
+      let the call through.
+    - ``UserPromptSubmit`` (``PHASE_REQUEST``) and ``PostToolUse``
+      (``PHASE_TOOL_RESULT``) fail OPEN → ``None``. The request gate is
+      advisory (the tool-call gate still catches dangerous actions) and by
+      the result phase the tool has already executed, so denying would only
+      block an already-incurred side effect.
+
+    :param hook_event: Hook event name, e.g. ``"PreToolUse"``.
+    :returns: A ``permissionDecision: "deny"`` hook output for
+        ``PreToolUse``; ``None`` for every other event (fail open).
+    """
+    if hook_event == _PRE_TOOL_USE:
+        return {
+            "hookSpecificOutput": {
+                "hookEventName": _PRE_TOOL_USE,
+                "permissionDecision": "deny",
+                "permissionDecisionReason": _EVAL_UNAVAILABLE_REASON,
+            },
+        }
+    return None

tests/native_hook_helpers.py

@@ -0,0 +1,44 @@
+"""Shared test helpers for the native-harness policy hooks.
+
+Used by both ``tests/test_claude_native_hook.py`` and
+``tests/test_codex_native_hook.py`` so the fail-closed failure-mode stub
+lives in one place and can't drift if new modes are added.
+"""
+
+from __future__ import annotations
+
+import httpx
+
+
+def make_failing_client(mode: str) -> type:
+    """
+    Build an ``httpx.Client`` stub that fails the policy POST a given way.
+
+    :param mode: One of ``"connect_error"`` (POST raises), ``"non_2xx"``
+        (503 → ``raise_for_status``), ``"empty_body"`` (200, no content),
+        or ``"malformed_json"`` (200, non-JSON body).
+    :returns: A class usable as a drop-in for :class:`httpx.Client`.
+    """
+
+    class _FailingHttpxClient:
+        def __init__(self, *, headers: dict[str, str], timeout: object) -> None:
+            del headers, timeout
+
+        def __enter__(self) -> _FailingHttpxClient:
+            return self
+
+        def __exit__(self, *args: object) -> None:
+            del args
+
+        def post(self, url: str, *, json: dict[str, object]) -> httpx.Response:
+            del json
+            req = httpx.Request("POST", url)
+            if mode == "connect_error":
+                raise httpx.ConnectError("AP unreachable", request=req)
+            if mode == "non_2xx":
+                return httpx.Response(503, text="upstream down", request=req)
+            if mode == "empty_body":
+                return httpx.Response(200, content=b"", request=req)
+            return httpx.Response(200, text="not json at all", request=req)
+
+    return _FailingHttpxClient

tests/test_claude_native_hook.py

@@ -19,6 +19,7 @@ from omnigent.claude_native_bridge import (
     record_hook_event,
     write_active_session_id,
 )
+from tests.native_hook_helpers import make_failing_client
 
 
 @pytest.fixture(autouse=True)
@@ -1709,3 +1710,79 @@ def test_ask_user_question_hook_returns_deny_without_updated_input(
     assert "updatedInput" not in hs, (
         "updatedInput must not appear on a deny response — there are no answers to inject"
     )
+
+
+@pytest.mark.parametrize("mode", ["connect_error", "non_2xx", "empty_body", "malformed_json"])
+def test_evaluate_policy_pre_tool_use_fails_closed_when_verdict_unavailable(
+    tmp_path: Path,
+    monkeypatch: pytest.MonkeyPatch,
+    capsys: pytest.CaptureFixture[str],
+    mode: str,
+) -> None:
+    """
+    A governed PreToolUse call denies when no usable verdict is returned.
+
+    For native harnesses this hook is the sole TOOL_CALL enforcement point,
+    so a server outage / non-2xx / empty / malformed response must fail
+    CLOSED (deny) instead of "no opinion" — the bypass reported in #536.
+    """
+    monkeypatch.setattr("omnigent.claude_native_bridge._TRUSTED_PARENT", tmp_path)
+    monkeypatch.setattr("omnigent.claude_native_bridge._BRIDGE_ROOT", tmp_path / "root")
+    monkeypatch.setattr(claude_native_hook.httpx, "Client", make_failing_client(mode))
+    bridge_dir = prepare_bridge_dir("conv_abc", bridge_id="bridge_shared", workspace=tmp_path)
+    write_active_session_id(bridge_dir, "conv_active")
+    build_hook_settings(bridge_dir, ap_server_url="http://127.0.0.1:8787")
+    payload = {
+        "hook_event_name": "PreToolUse",
+        "tool_name": "Bash",
+        "tool_input": {"command": "rm -rf /"},
+    }
+    monkeypatch.setattr(sys, "stdin", io.StringIO(json.dumps(payload)))
+
+    exit_code = claude_native_hook.main(["evaluate-policy", "--bridge-dir", str(bridge_dir)])
+
+    captured = capsys.readouterr()
+    assert exit_code == 0
+    result = json.loads(captured.out)
+    assert result["hookSpecificOutput"]["permissionDecision"] == "deny", result
+    assert result["hookSpecificOutput"]["permissionDecisionReason"]
+
+
+@pytest.mark.parametrize(
+    "payload",
+    [
+        {
+            "hook_event_name": "PostToolUse",
+            "tool_name": "Bash",
+            "tool_input": {"command": "ls"},
+            "tool_output": "ok",
+        },
+        {"hook_event_name": "UserPromptSubmit", "prompt": "hello"},
+    ],
+)
+def test_evaluate_policy_non_tool_call_phases_fail_open_on_error(
+    tmp_path: Path,
+    monkeypatch: pytest.MonkeyPatch,
+    capsys: pytest.CaptureFixture[str],
+    payload: dict[str, object],
+) -> None:
+    """
+    Off the tool-call gate, an unobtainable verdict stays fail-open.
+
+    PostToolUse runs after the tool executed and the request gate is
+    advisory, so neither denies on a transport error — mirroring the
+    runner-side ``FAIL_CLOSED_PHASES`` (PR #163).
+    """
+    monkeypatch.setattr("omnigent.claude_native_bridge._TRUSTED_PARENT", tmp_path)
+    monkeypatch.setattr("omnigent.claude_native_bridge._BRIDGE_ROOT", tmp_path / "root")
+    monkeypatch.setattr(claude_native_hook.httpx, "Client", make_failing_client("connect_error"))
+    bridge_dir = prepare_bridge_dir("conv_abc", bridge_id="bridge_shared", workspace=tmp_path)
+    write_active_session_id(bridge_dir, "conv_active")
+    build_hook_settings(bridge_dir, ap_server_url="http://127.0.0.1:8787")
+    monkeypatch.setattr(sys, "stdin", io.StringIO(json.dumps(payload)))
+
+    exit_code = claude_native_hook.main(["evaluate-policy", "--bridge-dir", str(bridge_dir)])
+
+    captured = capsys.readouterr()
+    assert exit_code == 0
+    assert captured.out == ""

tests/test_codex_native_hook.py

@@ -18,6 +18,7 @@ from omnigent.codex_native_bridge import (
     write_bridge_state,
     write_policy_hook_config,
 )
+from tests.native_hook_helpers import make_failing_client
 
 
 class _DenyHttpxClient:
@@ -390,3 +391,72 @@ def test_missing_policy_config_is_fail_open(
     captured = capsys.readouterr()
     assert exit_code == 0
     assert captured.out == ""
+
+
+@pytest.mark.parametrize("mode", ["connect_error", "non_2xx", "empty_body", "malformed_json"])
+def test_pre_tool_use_fails_closed_when_verdict_unavailable(
+    bridge_dir: Path,
+    monkeypatch: pytest.MonkeyPatch,
+    capsys: pytest.CaptureFixture[str],
+    mode: str,
+) -> None:
+    """
+    A governed PreToolUse call denies when no usable verdict is returned.
+
+    For native harnesses this hook is the sole TOOL_CALL enforcement point,
+    so a server outage / non-2xx / empty / malformed response must fail
+    CLOSED (deny) instead of "no opinion" — the bypass reported in #536.
+    """
+    write_policy_hook_config(bridge_dir, ap_server_url="http://127.0.0.1:8787", ap_auth_headers={})
+    monkeypatch.setattr(codex_native_hook.httpx, "Client", make_failing_client(mode))
+
+    exit_code = _run_hook(
+        bridge_dir,
+        {
+            "hook_event_name": "PreToolUse",
+            "tool_name": "Bash",
+            "tool_input": {"command": "rm -rf /"},
+        },
+        monkeypatch,
+    )
+
+    captured = capsys.readouterr()
+    assert exit_code == 0
+    result = json.loads(captured.out)
+    assert result["hookSpecificOutput"]["permissionDecision"] == "deny", result
+    assert result["hookSpecificOutput"]["permissionDecisionReason"]
+
+
+@pytest.mark.parametrize(
+    "payload",
+    [
+        {
+            "hook_event_name": "PostToolUse",
+            "tool_name": "Bash",
+            "tool_input": {"command": "ls"},
+            "tool_output": "ok",
+        },
+        {"hook_event_name": "UserPromptSubmit", "prompt": "hello"},
+    ],
+)
+def test_non_tool_call_phases_fail_open_on_error(
+    bridge_dir: Path,
+    monkeypatch: pytest.MonkeyPatch,
+    capsys: pytest.CaptureFixture[str],
+    payload: dict[str, object],
+) -> None:
+    """
+    Off the tool-call gate, an unobtainable verdict stays fail-open.
+
+    PostToolUse runs after the tool executed and the request gate is
+    advisory, so neither denies on a transport error — mirroring the
+    runner-side ``FAIL_CLOSED_PHASES`` (PR #163).
+    """
+    write_policy_hook_config(bridge_dir, ap_server_url="http://127.0.0.1:8787", ap_auth_headers={})
+    monkeypatch.setattr(codex_native_hook.httpx, "Client", make_failing_client("connect_error"))
+
+    exit_code = _run_hook(bridge_dir, payload, monkeypatch)
+
+    captured = capsys.readouterr()
+    assert exit_code == 0
+    assert captured.out == ""

tests/test_native_policy_hook.py

@@ -6,6 +6,7 @@ import pytest
 
 from omnigent.native_policy_hook import (
     evaluation_response_to_hook_output,
+    fail_closed_hook_output,
     hook_payload_to_evaluation_request,
 )
 
@@ -310,3 +311,46 @@ def test_user_prompt_submit_non_blocking_actions_return_none(action: str) -> Non
     """
     output = evaluation_response_to_hook_output("UserPromptSubmit", {"result": action})
     assert output is None
+
+
+def test_fail_closed_pre_tool_use_denies() -> None:
+    """
+    An unobtainable verdict on PreToolUse fails CLOSED with ``deny``.
+
+    PreToolUse is the authoritative pre-execution gate for native tools —
+    the sole enforcement point for connector-native ``mcp__*`` tools and
+    native Bash/Write/Edit — so a verdict that cannot be fetched must deny
+    rather than silently let the call through (issue #536).
+    """
+    output = fail_closed_hook_output("PreToolUse")
+    assert output is not None
+    hook_specific = output["hookSpecificOutput"]
+    assert hook_specific["hookEventName"] == "PreToolUse"
+    assert hook_specific["permissionDecision"] == "deny"
+    # A deny is inert without a reason on the consuming harnesses, so one
+    # must always be present.
+    assert hook_specific["permissionDecisionReason"]
+
+
+@pytest.mark.parametrize("hook_event", ["UserPromptSubmit", "PostToolUse"])
+def test_fail_closed_non_tool_call_phases_fail_open(hook_event: str) -> None:
+    """
+    Off the tool-call gate, an unobtainable verdict fails OPEN (``None``).
+
+    The request gate is advisory (the tool-call gate still catches
+    dangerous actions) and PostToolUse runs after the tool has executed, so
+    denying there only blocks an already-incurred side effect. This mirrors
+    the runner-side ``FAIL_CLOSED_PHASES`` (PR #163).
+    """
+    assert fail_closed_hook_output(hook_event) is None
+
+
+def test_fail_closed_unknown_event_fails_open() -> None:
+    """
+    An unrecognized hook event fails OPEN (``None``), not closed.
+
+    Only the exact ``PreToolUse`` event denies; any novel event name added
+    by a future harness must fall through to "no opinion" rather than
+    accidentally blocking — the conservative default for an unknown gate.
+    """
+    assert fail_closed_hook_output("SomeNewEvent") is None