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.
fix(harnesses): fail closed for TOOL_CALL when native policy hook can't reach the server
2026-06-18 04:58:14 +00:00 · 2026-06-18 00:37:52 +00:00
7 changed files with 361 additions and 16 deletions
@@ -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:
@@ -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:
@@ -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
@@ -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
@@ -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 == ""
@@ -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 == ""
@@ -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