diff --git a/agent/transports/codex_app_server_session.py b/agent/transports/codex_app_server_session.py index 7954ecbf4d5..e2ace753bc8 100644 --- a/agent/transports/codex_app_server_session.py +++ b/agent/transports/codex_app_server_session.py @@ -1054,6 +1054,18 @@ class CodexAppServerSession: ) def _decide_exec_approval(self, params: dict) -> str: + """Decide a Codex exec approval request. + + This is protocol-level routing only — it carries NO Hermes + approval-mode/timeout logic. The Hermes-side resolution happens + upstream: ``agent/codex_runtime.py`` derives + ``auto_approve_exec`` from the canonical + ``tools.approval.is_approval_bypass_active()`` (which reads + ``approvals.mode`` via ``tools.approval._get_approval_mode``), + and ``self._approval_callback`` itself runs the shared approval + gate (mode + ``approvals.timeout``) in ``tools/approval.py``. + Keep it that way — do not re-read approval config here. + """ if self._routing.auto_approve_exec: return "accept" command = params.get("command") or "" @@ -1077,6 +1089,12 @@ class CodexAppServerSession: return "decline" # fail-closed when no callback wired def _decide_apply_patch_approval(self, params: dict) -> str: + """Decide a Codex apply_patch approval request. + + Protocol-level routing only; Hermes approval-mode/timeout + resolution is delegated to ``tools/approval.py`` upstream — see + the docstring on ``_decide_exec_approval``. + """ if self._routing.auto_approve_apply_patch: return "accept" if self._approval_callback is not None: @@ -1231,6 +1249,11 @@ def _approval_choice_to_codex_decision(choice: str) -> str: Codex expects 'accept', 'acceptForSession', 'decline', or 'cancel' (verified against codex-rs/app-server-protocol/src/protocol/v2/item.rs on codex 0.130.0). + + This mapping is Codex-protocol-semantic and intentionally lives here, + NOT in tools/approval.py: the Hermes approval mode/timeout resolution + and the choice itself come from the shared core (tools/approval.py); + only the wire-value translation is local. """ if choice in {"once",}: return "accept" diff --git a/tests/tools/test_approval_mode_parity.py b/tests/tools/test_approval_mode_parity.py new file mode 100644 index 00000000000..41545761ad0 --- /dev/null +++ b/tests/tools/test_approval_mode_parity.py @@ -0,0 +1,156 @@ +"""Cross-surface approval mode/timeout parity invariant. + +The approval mode (``approvals.mode``) and timeout (``approvals.timeout``) +must resolve identically on every surface that consults them: + + - the canonical core: ``tools.approval._get_approval_mode`` / + ``tools.approval._get_approval_timeout`` + - the TUI gateway: ``tui_gateway.server._load_approval_mode`` (delegates + to the core as of the decision-core migration) + - the codex app-server surface: ``agent/codex_runtime.py`` feeds + ``auto_approve_*`` from ``tools.approval.is_approval_bypass_active()``, + which itself reads the core resolver — so parity there reduces to + ``is_approval_bypass_active() == (mode == "off")`` when no yolo + source is active. + +Historic drift class: tui_gateway re-read config raw and normalized +locally (see commits f9cd577915, 1e652cca7a, bd246db10d — repeated parity +re-alignments). This test pins the invariant so drift regressions fail +loudly instead of silently disagreeing per surface. + +There is no per-platform ``approvals.mode`` override in the config schema; +mode/timeout are global, so the synthetic configs below cover global-set, +unset (defaults), and malformed values. +""" + +from __future__ import annotations + +import importlib +from unittest.mock import MagicMock, patch + +import pytest + + +@pytest.fixture() +def hermes_home(tmp_path, monkeypatch): + home = tmp_path / ".hermes" + home.mkdir() + monkeypatch.setenv("HERMES_HOME", str(home)) + return home + + +@pytest.fixture() +def tui_server(): + with patch.dict( + "sys.modules", + { + "hermes_cli.env_loader": MagicMock(), + "hermes_cli.banner": MagicMock(), + }, + ): + yield importlib.import_module("tui_gateway.server") + + +def _write_config(home, yaml_text: str | None) -> None: + cfg = home / "config.yaml" + if yaml_text is None: + if cfg.exists(): + cfg.unlink() + else: + cfg.write_text(yaml_text, encoding="utf-8") + + +# (config yaml, expected mode, expected timeout) +CASES = [ + pytest.param(None, "smart", 300, id="unset-defaults"), + pytest.param( + "approvals:\n mode: manual\n", "manual", 300, id="global-manual" + ), + pytest.param( + "approvals:\n mode: smart\n timeout: 120\n", + "smart", + 120, + id="global-smart-timeout", + ), + pytest.param( + # YAML 1.1 parses bare OFF as boolean False; the normalizer maps + # False -> "off". Both surfaces must agree on that quirk. + "approvals:\n mode: OFF\n timeout: 45\n", + "off", + 45, + id="yaml-bool-off", + ), + pytest.param( + "approvals:\n mode: bogus-value\n timeout: not-a-number\n", + "manual", + 300, + id="malformed-values", + ), + pytest.param( + "approvals:\n mode: ' Smart '\n", "smart", 300, id="whitespace-case" + ), +] + + +def _approval_module(): + """Resolve tools.approval via sys.modules, not the package attribute. + + The ``tui_server`` fixture's ``patch.dict("sys.modules", ...)`` purges + modules imported during its block at teardown; ``from tools import + approval`` can then hand back a stale attribute cached on the ``tools`` + package while the server re-imports a fresh module object. Going + through ``importlib.import_module`` keeps the test and the server on + the same sys.modules entry. + """ + return importlib.import_module("tools.approval") + + +@pytest.mark.parametrize("yaml_text,expected_mode,expected_timeout", CASES) +def test_mode_and_timeout_parity_across_surfaces( + hermes_home, tui_server, yaml_text, expected_mode, expected_timeout +): + approval_mod = _approval_module() + + _write_config(hermes_home, yaml_text) + + core_mode = approval_mod._get_approval_mode() + core_timeout = approval_mod._get_approval_timeout() + tui_mode = tui_server._load_approval_mode() + + # Canonical resolver matches expectations. + assert core_mode == expected_mode + assert core_timeout == expected_timeout + + # TUI surface returns the identical mode (delegation invariant). + assert tui_mode == core_mode + + # Codex surface: auto-approve routing is derived from + # is_approval_bypass_active(), which must equal (mode == "off") + # whenever no yolo source is active in this process. + if not approval_mod._YOLO_MODE_FROZEN: + with patch.object( + approval_mod, "is_current_session_yolo_enabled", return_value=False + ): + assert approval_mod.is_approval_bypass_active() == ( + core_mode == "off" + ) + + +def test_tui_loader_delegates_to_core(hermes_home, tui_server): + """The TUI must not re-resolve mode itself — it delegates to the core. + + Pin the delegation seam directly: patching the core resolver changes + what the TUI reports, proving there is no independent config read left. + """ + approval_mod = _approval_module() + + with patch.object(approval_mod, "_get_approval_mode", return_value="smart"): + assert tui_server._load_approval_mode() == "smart" + with patch.object(approval_mod, "_get_approval_mode", return_value="off"): + assert tui_server._load_approval_mode() == "off" + # Defensive clamp: an out-of-vocabulary value from the core is coerced + # to manual rather than leaking an unknown mode to the TUI client. + with patch.object( + approval_mod, "_get_approval_mode", return_value="weird" + ): + assert tui_server._load_approval_mode() == "manual" diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 306afd51896..814fff82445 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -3575,14 +3575,23 @@ _APPROVAL_MODES = frozenset({"manual", "smart", "off"}) def _load_approval_mode() -> str: - from hermes_cli.config import DEFAULT_CONFIG, _deep_merge - from tools.approval import _normalize_approval_mode + """Resolve the effective ``approvals.mode`` for the TUI surface. - raw_cfg = _load_cfg() - cfg = _deep_merge(DEFAULT_CONFIG, raw_cfg if isinstance(raw_cfg, dict) else {}) - approvals = cfg.get("approvals") - raw = approvals.get("mode") if isinstance(approvals, dict) else None - mode = _normalize_approval_mode(raw) + Delegates to the canonical resolver in ``tools.approval`` + (``_get_approval_mode``) so mode resolution cannot drift per surface — + the same normalization, defaults, and config precedence the approval + gate itself uses (see ``tools/approval.py``). + + Previously this re-read the config raw via ``_load_cfg`` + + ``_deep_merge(DEFAULT_CONFIG, ...)`` and normalized locally, which + could disagree with the gate's own view of the mode (e.g. the + canonical ``hermes_cli.config.load_config`` path applies managed-scope + overlays and ``${VAR}`` env expansion that the TUI's raw YAML read did + not fully mirror). + """ + from tools.approval import _get_approval_mode + + mode = _get_approval_mode() return mode if mode in _APPROVAL_MODES else "manual"