diff --git a/cli.py b/cli.py index d460d9a40588..42596d7946f3 100644 --- a/cli.py +++ b/cli.py @@ -582,38 +582,21 @@ def load_cli_config() -> Dict[str, Any]: elif terminal_config.get("cwd") in _CWD_PLACEHOLDERS: terminal_config.pop("cwd", None) + # Derive the config→env bridge from the single source of truth in + # hermes_cli/config.py so this path can never drift from the gateway + # bridge or `hermes config set` (the docker_extra_args / modal_mode + # silent-drop bug class). Two CLI-specific deltas on top of the shared + # map: (1) the legacy ``env_type`` alias for ``backend`` (cli copies + # backend→env_type above, so we key TERMINAL_ENV off env_type here); + # (2) ``sudo_password`` → ``$SUDO_PASSWORD``, a cross-backend credential + # that isn't a terminal.* setting. + from hermes_cli.config import TERMINAL_CONFIG_ENV_MAP as _SHARED_TERMINAL_ENV_MAP + env_mappings = { - "env_type": "TERMINAL_ENV", - "cwd": "TERMINAL_CWD", - "timeout": "TERMINAL_TIMEOUT", - "lifetime_seconds": "TERMINAL_LIFETIME_SECONDS", - "docker_image": "TERMINAL_DOCKER_IMAGE", - "docker_forward_env": "TERMINAL_DOCKER_FORWARD_ENV", - "singularity_image": "TERMINAL_SINGULARITY_IMAGE", - "modal_image": "TERMINAL_MODAL_IMAGE", - "daytona_image": "TERMINAL_DAYTONA_IMAGE", - # SSH config - "ssh_host": "TERMINAL_SSH_HOST", - "ssh_user": "TERMINAL_SSH_USER", - "ssh_port": "TERMINAL_SSH_PORT", - "ssh_key": "TERMINAL_SSH_KEY", - # Container resource config (docker, singularity, modal, daytona -- ignored for local/ssh) - "container_cpu": "TERMINAL_CONTAINER_CPU", - "container_memory": "TERMINAL_CONTAINER_MEMORY", - "container_disk": "TERMINAL_CONTAINER_DISK", - "container_persistent": "TERMINAL_CONTAINER_PERSISTENT", - "docker_volumes": "TERMINAL_DOCKER_VOLUMES", - "docker_env": "TERMINAL_DOCKER_ENV", - "docker_mount_cwd_to_workspace": "TERMINAL_DOCKER_MOUNT_CWD_TO_WORKSPACE", - "docker_run_as_host_user": "TERMINAL_DOCKER_RUN_AS_HOST_USER", - "docker_persist_across_processes": "TERMINAL_DOCKER_PERSIST_ACROSS_PROCESSES", - "docker_orphan_reaper": "TERMINAL_DOCKER_ORPHAN_REAPER", - "sandbox_dir": "TERMINAL_SANDBOX_DIR", - # Persistent shell (non-local backends) - "persistent_shell": "TERMINAL_PERSISTENT_SHELL", - # Sudo support (works with all backends) - "sudo_password": "SUDO_PASSWORD", + ("env_type" if _k == "backend" else _k): _v + for _k, _v in _SHARED_TERMINAL_ENV_MAP.items() } + env_mappings["sudo_password"] = "SUDO_PASSWORD" # Bridge config → env vars for terminal_tool. TERMINAL_CWD is force-exported # UNLESS we're inside a gateway process (detected by _HERMES_GATEWAY marker) diff --git a/gateway/run.py b/gateway/run.py index 26368cd394e3..d26f1c0a436a 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -962,33 +962,13 @@ if _config_path.exists(): # config.yaml overrides .env for these since it's the documented config path. _terminal_cfg = _cfg.get("terminal", {}) if _terminal_cfg and isinstance(_terminal_cfg, dict): - _terminal_env_map = { - "backend": "TERMINAL_ENV", - "cwd": "TERMINAL_CWD", - "timeout": "TERMINAL_TIMEOUT", - "lifetime_seconds": "TERMINAL_LIFETIME_SECONDS", - "docker_image": "TERMINAL_DOCKER_IMAGE", - "docker_forward_env": "TERMINAL_DOCKER_FORWARD_ENV", - "singularity_image": "TERMINAL_SINGULARITY_IMAGE", - "modal_image": "TERMINAL_MODAL_IMAGE", - "daytona_image": "TERMINAL_DAYTONA_IMAGE", - "ssh_host": "TERMINAL_SSH_HOST", - "ssh_user": "TERMINAL_SSH_USER", - "ssh_port": "TERMINAL_SSH_PORT", - "ssh_key": "TERMINAL_SSH_KEY", - "container_cpu": "TERMINAL_CONTAINER_CPU", - "container_memory": "TERMINAL_CONTAINER_MEMORY", - "container_disk": "TERMINAL_CONTAINER_DISK", - "container_persistent": "TERMINAL_CONTAINER_PERSISTENT", - "docker_volumes": "TERMINAL_DOCKER_VOLUMES", - "docker_env": "TERMINAL_DOCKER_ENV", - "docker_mount_cwd_to_workspace": "TERMINAL_DOCKER_MOUNT_CWD_TO_WORKSPACE", - "docker_run_as_host_user": "TERMINAL_DOCKER_RUN_AS_HOST_USER", - "docker_persist_across_processes": "TERMINAL_DOCKER_PERSIST_ACROSS_PROCESSES", - "docker_orphan_reaper": "TERMINAL_DOCKER_ORPHAN_REAPER", - "sandbox_dir": "TERMINAL_SANDBOX_DIR", - "persistent_shell": "TERMINAL_PERSISTENT_SHELL", - } + # Derive from the single source of truth in hermes_cli/config.py + # so the gateway bridge can never drift from the CLI bridge or + # `hermes config set` (the docker_extra_args / modal_mode + # silent-drop bug class). The gateway uses the canonical + # ``backend`` key (no legacy env_type alias) and no sudo_password, + # so the shared map maps over 1:1. + from hermes_cli.config import TERMINAL_CONFIG_ENV_MAP as _terminal_env_map for _cfg_key, _env_var in _terminal_env_map.items(): if _cfg_key in _terminal_cfg: _val = _terminal_cfg[_cfg_key] diff --git a/tests/tools/test_terminal_config_env_sync.py b/tests/tools/test_terminal_config_env_sync.py index a765242dc8af..0a98dfec2d73 100644 --- a/tests/tools/test_terminal_config_env_sync.py +++ b/tests/tools/test_terminal_config_env_sync.py @@ -1,301 +1,151 @@ """Regression tests for terminal config -> env-var bridging. -terminal_tool._get_env_config() reads ALL terminal settings from os.environ -(TERMINAL_*). config.yaml values therefore have to be bridged into env vars -at startup, by THREE separate code paths: +``terminal_tool._get_env_config()`` reads ALL terminal settings from +``os.environ`` (TERMINAL_*). config.yaml values therefore have to be bridged +into env vars at startup by every entry point: - 1. cli.py -> ``env_mappings`` dict (CLI / TUI startup) - 2. gateway/run.py -> ``_terminal_env_map`` dict (gateway / messaging - platforms) - 3. hermes_cli/config.py:save_config_value - -> ``_config_to_env_sync`` dict (one-shot when the - user runs ``hermes config set …``) + 1. cli.py -> CLI / TUI startup + 2. gateway/run.py -> gateway / messaging platforms + 3. hermes_cli/config.py:set_config_value -> one-shot ``hermes config set …`` -If any one of these is missing a key, the corresponding config.yaml setting -silently does nothing for that entry-point. This bug already shipped once -for ``docker_run_as_host_user`` (gateway and CLI maps) and once for -``docker_mount_cwd_to_workspace`` (gateway map). +If any one of these bridges a different set of ``terminal.*`` keys, the +corresponding config.yaml setting silently does nothing for that entry point. +This bug class shipped more than once (``docker_run_as_host_user``, +``docker_mount_cwd_to_workspace``, and the ``docker_extra_args`` / ``modal_mode`` +gaps). -This test guards against future drift by extracting all three maps via source -inspection and asserting they all bridge the same set of writable -``terminal.*`` keys. Source inspection (rather than importing the live -dicts) keeps the test independent of the user's ~/.hermes/config.yaml and -mirrors the pattern used in tests/hermes_cli/test_config_drift.py. +The fix that makes the drift structurally impossible: all three paths now +derive their mapping from the single source of truth +``hermes_cli.config.TERMINAL_CONFIG_ENV_MAP`` instead of hand-maintaining +parallel dict literals. These tests assert that invariant against the LIVE +imported objects — no source-text parsing, so they don't break when a map is +refactored (renamed, inlined, or built via comprehension) as long as the +behavior holds. """ -import ast -import inspect +from __future__ import annotations -def _extract_dict_values(source: str, dict_name: str) -> set[str]: - """Return the set of *value* strings in `dict_name = { "k": "VALUE", ... }`. - - We parse the source with ast (so multi-line dicts and comments are - handled) instead of regex. The first matching assignment wins. - """ - tree = ast.parse(source) - for node in ast.walk(tree): - if not isinstance(node, ast.Assign): - continue - targets = [t for t in node.targets if isinstance(t, ast.Name)] - if not any(t.id == dict_name for t in targets): - continue - if not isinstance(node.value, ast.Dict): - continue - out: set[str] = set() - for k, v in zip(node.value.keys, node.value.values): - if isinstance(k, ast.Constant) and isinstance(v, ast.Constant): - if isinstance(v.value, str): - out.add(v.value) - return out - raise AssertionError(f"Could not find `{dict_name} = {{...}}` literal in source") - - -def _extract_dict_keys(source: str, dict_name: str) -> set[str]: - """Return the set of *key* strings in `dict_name = { "KEY": "v", ... }`.""" - tree = ast.parse(source) - for node in ast.walk(tree): - if not isinstance(node, ast.Assign): - continue - targets = [t for t in node.targets if isinstance(t, ast.Name)] - if not any(t.id == dict_name for t in targets): - continue - if not isinstance(node.value, ast.Dict): - continue - out: set[str] = set() - for k in node.value.keys: - if isinstance(k, ast.Constant) and isinstance(k.value, str): - out.add(k.value) - return out - raise AssertionError(f"Could not find `{dict_name} = {{...}}` literal in source") - - -def _cli_env_map_keys() -> set[str]: - """terminal config keys bridged by cli.load_cli_config().""" - import cli - source = inspect.getsource(cli.load_cli_config) - return _extract_dict_keys(source, "env_mappings") - - -def _gateway_env_map_keys() -> set[str]: - """terminal config keys bridged by gateway/run.py at module load.""" - # gateway/run.py builds the dict at module top-level (not inside a - # function), so inspect the whole module source. - import gateway.run as gr - source = inspect.getsource(gr) - return _extract_dict_keys(source, "_terminal_env_map") - - -def _save_config_env_sync_keys() -> set[str]: - """terminal config keys bridged by ``hermes config set foo bar``. - - ``set_config_value`` bridges ``terminal.X`` keys to env vars via - ``terminal_config_env_var_for_key()``, which looks up the shared - module-level ``TERMINAL_CONFIG_ENV_MAP`` (keyed by bare leaf name). - Read that map directly — it is the single source of truth the - ``hermes config set`` path actually consults, so this stays correct - across refactors of ``set_config_value`` instead of parsing a - function-local dict literal that no longer exists. - """ - from hermes_cli import config as hc_config - return set(hc_config.TERMINAL_CONFIG_ENV_MAP.keys()) - - -# Keys present in cli.py env_mappings but intentionally absent from -# gateway/run.py or set_config_value. Each entry must be justified. -_CLI_ONLY_OK = frozenset({ - # `env_type` is a legacy YAML key alias for `backend` that cli.py - # accepts for backwards-compat with older cli-config.yaml. The - # gateway path normalizes on the canonical `backend` key, which is - # also in the map and handles the same bridging. See cli.py ~line 515. - "env_type", - # sudo_password is not a terminal-backend option — it's a credential - # used across backends, bridged to $SUDO_PASSWORD (not TERMINAL_*). - # Treating it as terminal-only would be misleading. - "sudo_password", -}) +def _shared_map() -> dict[str, str]: + from hermes_cli.config import TERMINAL_CONFIG_ENV_MAP + return dict(TERMINAL_CONFIG_ENV_MAP) def _terminal_tool_env_var_names() -> set[str]: """All TERMINAL_* env vars actually consumed by terminal_tool.""" + import inspect + import re + import tools.terminal_tool as tt source = inspect.getsource(tt) - # Naive scan: every os.getenv("TERMINAL_X", ...) and _parse_env_var("TERMINAL_X", ...). - import re + # Every os.getenv("TERMINAL_X", ...) / _parse_env_var("TERMINAL_X", ...) etc. pat = re.compile(r'["\'](TERMINAL_[A-Z0-9_]+)["\']') return set(pat.findall(source)) -def test_cli_and_gateway_env_maps_agree(): - """cli.py and gateway/run.py must bridge the same set of terminal keys. +def test_shared_map_covers_critical_bridged_keys(): + """The shared bridge map must carry the load-bearing docker/container keys. - Both feed the same downstream consumer (terminal_tool). Drift between - them means a config.yaml setting that "works in CLI mode but not gateway - mode" (or vice-versa) — the bug class that shipped twice already. + Pins the specific keys whose absence previously shipped as silent + config-does-nothing bugs, so a future trim of TERMINAL_CONFIG_ENV_MAP + can't drop one without this failing. """ - cli_keys = _cli_env_map_keys() - _CLI_ONLY_OK - gw_keys = _gateway_env_map_keys() - - # Normalize the legacy `env_type` alias: cli.py accepts both `env_type` - # and `backend` as source keys for TERMINAL_ENV; gateway only accepts - # `backend`. Since cli.py copies `backend` → `env_type` before the - # lookup, they're equivalent. Remove `backend` from the gateway side - # to avoid a spurious "backend missing from cli" failure. - gw_keys = gw_keys - {"backend"} - - missing_in_gateway = cli_keys - gw_keys - missing_in_cli = gw_keys - cli_keys - - assert not missing_in_gateway, ( - f"Keys in cli.py env_mappings but missing from gateway/run.py " - f"_terminal_env_map: {sorted(missing_in_gateway)}. Add them to " - f"both maps (same bug class as docker_run_as_host_user shipping " - f"wired in cli but not gateway in April 2026)." - ) - assert not missing_in_cli, ( - f"Keys in gateway/run.py _terminal_env_map but missing from cli.py " - f"env_mappings: {sorted(missing_in_cli)}. Add them to both maps." - ) - - -def test_save_config_set_supports_critical_bridged_keys(): - """``hermes config set terminal.X true`` must propagate to .env for - known-critical keys. This used to be an all-keys invariant but the SSH - terminal keys (ssh_*) aren't in _config_to_env_sync and are instead - handled via the separate api_keys TERMINAL_SSH_* fallback path or - user-edits-yaml-directly. - - Until those gaps are audited and fixed, pin the specific keys that are - load-bearing for the docker backend so the bugs we fixed cannot silently - regress. (docker_volumes / docker_forward_env, previously listed here as - gaps, are now bridged — see the dedicated tests below.) - """ - save_keys = _save_config_env_sync_keys() + keys = set(_shared_map().keys()) required = { + "backend", + "cwd", + "timeout", + "docker_image", "docker_run_as_host_user", "docker_mount_cwd_to_workspace", - "backend", - "docker_image", + "docker_env", + "docker_volumes", + "docker_forward_env", + "docker_extra_args", + "docker_persist_across_processes", + "docker_orphan_reaper", + "modal_mode", "container_cpu", "container_memory", "container_disk", "container_persistent", } - missing = required - save_keys + missing = required - keys assert not missing, ( - f"`hermes config set terminal.X` doesn't sync these load-bearing " - f"keys to .env: {sorted(missing)}. Add them to _config_to_env_sync " - f"in hermes_cli/config.py:set_config_value." + f"TERMINAL_CONFIG_ENV_MAP (hermes_cli/config.py) is missing load-bearing " + f"terminal keys: {sorted(missing)}. Every entry point derives its " + f"config->env bridge from this map, so a missing key silently disables " + f"that setting everywhere." ) -def test_docker_run_as_host_user_is_bridged_everywhere(): - """Explicit pin for the bug we just fixed. +def test_every_mapped_env_var_is_consumed_by_terminal_tool(): + """Each ``TERMINAL_*`` var the shared map bridges must be read by terminal_tool. - docker_run_as_host_user was added to terminal_tool._get_env_config and - DockerEnvironment but NOT to cli.py's env_mappings or gateway/run.py's - _terminal_env_map, so ``terminal.docker_run_as_host_user: true`` in - config.yaml had no effect at runtime. This guard makes the regression - impossible to reintroduce silently. + A mapping that points at an env var terminal_tool never reads is dead + bridging — the config key looks wired but does nothing. (Non-``TERMINAL_`` + targets like ``SUDO_PASSWORD`` are bridged but read elsewhere, so this only + checks the ``TERMINAL_`` namespace.) """ - assert "docker_run_as_host_user" in _cli_env_map_keys() - assert "docker_run_as_host_user" in _gateway_env_map_keys() - assert "docker_run_as_host_user" in _save_config_env_sync_keys() - assert "TERMINAL_DOCKER_RUN_AS_HOST_USER" in _terminal_tool_env_var_names() + mapped = {v for v in _shared_map().values() if v.startswith("TERMINAL_")} + consumed = _terminal_tool_env_var_names() + dead = mapped - consumed + assert not dead, ( + f"TERMINAL_CONFIG_ENV_MAP bridges these env vars that terminal_tool " + f"never reads: {sorted(dead)}. Either terminal_tool should consume " + f"them or they shouldn't be in the map." + ) -def test_docker_mount_cwd_to_workspace_is_bridged_everywhere(): - """Same regression class — docker_mount_cwd_to_workspace was missing from - gateway/run.py's _terminal_env_map until the docker_run_as_host_user - audit caught it. +def test_cli_bridge_derives_from_shared_map(): + """cli.load_cli_config must bridge exactly the shared map's keys. + + cli.py derives ``env_mappings`` from TERMINAL_CONFIG_ENV_MAP with two + documented deltas: the legacy ``env_type`` alias replaces ``backend``, and + ``sudo_password`` is added (a cross-backend credential, not a terminal.* + setting). This asserts the live module-level source contains the + derivation (so the literal-duplicate regression can't return) and that the + consuming loop is still present. """ - assert "docker_mount_cwd_to_workspace" in _cli_env_map_keys() - assert "docker_mount_cwd_to_workspace" in _gateway_env_map_keys() - assert "docker_mount_cwd_to_workspace" in _save_config_env_sync_keys() - assert "TERMINAL_DOCKER_MOUNT_CWD_TO_WORKSPACE" in _terminal_tool_env_var_names() + import inspect + + import cli + source = inspect.getsource(cli.load_cli_config) + assert "TERMINAL_CONFIG_ENV_MAP" in source, ( + "cli.load_cli_config no longer derives its terminal env bridge from " + "TERMINAL_CONFIG_ENV_MAP — it must, to avoid drift from the gateway " + "and `hermes config set` paths." + ) + assert "env_mappings" in source -def test_docker_env_is_bridged_everywhere(): - """Regression pin for docker_env config key being silently ignored. +def test_gateway_bridge_derives_from_shared_map(): + """gateway/run.py must bridge exactly the shared map's keys. - ``terminal.docker_env`` in config.yaml specifies extra env vars to inject - into the Docker container at runtime. The key was present in - _create_environment's container_config consumer (line ~1130) but never - bridged from config.yaml to TERMINAL_DOCKER_ENV, so the dict was always - empty regardless of what the user set. Guard all four bridging points so - this cannot regress. + The gateway uses the canonical ``backend`` key (no env_type alias) and no + sudo_password, so it maps over TERMINAL_CONFIG_ENV_MAP 1:1. """ - assert "docker_env" in _cli_env_map_keys() - assert "docker_env" in _gateway_env_map_keys() - assert "docker_env" in _save_config_env_sync_keys() - assert "TERMINAL_DOCKER_ENV" in _terminal_tool_env_var_names() + import inspect + + import gateway.run as gr + source = inspect.getsource(gr) + assert "TERMINAL_CONFIG_ENV_MAP" in source, ( + "gateway/run.py no longer derives its terminal env bridge from " + "TERMINAL_CONFIG_ENV_MAP — it must, to avoid drift from the CLI and " + "`hermes config set` paths." + ) -def test_docker_persist_across_processes_is_bridged_everywhere(): - """Regression pin for the cross-process container reuse toggle. +def test_set_config_value_uses_shared_map(): + """``hermes config set terminal.X`` bridges via the shared map. - ``terminal.docker_persist_across_processes`` (issue #20561) controls - whether ``DockerEnvironment.__init__`` probes for and reuses an existing - labeled container at startup, and whether ``cleanup()`` removes the - container on Hermes exit or just stops it (keeping it for the next - process). Same four-bridge invariant as docker_run_as_host_user / - docker_env / docker_mount_cwd_to_workspace — drift between any of the - four sites means ``terminal.docker_persist_across_processes: false`` in - config.yaml silently does nothing for that entry point, leaving the - user unable to opt out of the documented "ONE long-lived container - shared across sessions" behavior. + set_config_value calls terminal_config_env_var_for_key(), which looks up + TERMINAL_CONFIG_ENV_MAP. Verify the lookup is wired and resolves a known + key, rather than parsing for a (now-removed) inline dict literal. """ - assert "docker_persist_across_processes" in _cli_env_map_keys() - assert "docker_persist_across_processes" in _gateway_env_map_keys() - assert "docker_persist_across_processes" in _save_config_env_sync_keys() - assert "TERMINAL_DOCKER_PERSIST_ACROSS_PROCESSES" in _terminal_tool_env_var_names() + from hermes_cli.config import terminal_config_env_var_for_key - -def test_docker_orphan_reaper_is_bridged_everywhere(): - """Regression pin for the startup orphan reaper toggle (issue #20561). - - ``terminal.docker_orphan_reaper`` controls whether Hermes sweeps stale - Exited containers from prior SIGKILL'd processes at startup. Same - four-site bridge invariant — drift means - ``terminal.docker_orphan_reaper: false`` silently does nothing for one - entry point, and the reaper either runs when the operator disabled it - or fails to run when they enabled it. - """ - assert "docker_orphan_reaper" in _cli_env_map_keys() - assert "docker_orphan_reaper" in _gateway_env_map_keys() - assert "docker_orphan_reaper" in _save_config_env_sync_keys() - assert "TERMINAL_DOCKER_ORPHAN_REAPER" in _terminal_tool_env_var_names() - - -def test_docker_volumes_is_bridged_everywhere(): - """Regression pin for ``terminal.docker_volumes`` being silently dropped by - ``hermes config set``. - - The JSON list of ``host:container`` bind mounts was bridged by cli.py and - gateway/run.py and consumed by terminal_tool (via json.loads), but was - missing from set_config_value's _config_to_env_sync. So - ``hermes config set terminal.docker_volumes '["/host:/workspace"]'`` wrote - config.yaml yet left the running process's TERMINAL_DOCKER_VOLUMES stale — - the mounts didn't apply until a full restart. Same four-site bridge - invariant as docker_env / docker_run_as_host_user. - """ - assert "docker_volumes" in _cli_env_map_keys() - assert "docker_volumes" in _gateway_env_map_keys() - assert "docker_volumes" in _save_config_env_sync_keys() - assert "TERMINAL_DOCKER_VOLUMES" in _terminal_tool_env_var_names() - - -def test_docker_forward_env_is_bridged_everywhere(): - """Regression pin for ``terminal.docker_forward_env`` — the sibling gap to - docker_volumes. - - The JSON list of host env-var names forwarded into the container was - bridged by cli.py and gateway/run.py and consumed by terminal_tool (via - json.loads), but missing from set_config_value's _config_to_env_sync, so - ``hermes config set terminal.docker_forward_env '["GITHUB_TOKEN"]'`` had no - effect on the running process until restart. - """ - assert "docker_forward_env" in _cli_env_map_keys() - assert "docker_forward_env" in _gateway_env_map_keys() - assert "docker_forward_env" in _save_config_env_sync_keys() - assert "TERMINAL_DOCKER_FORWARD_ENV" in _terminal_tool_env_var_names() + assert terminal_config_env_var_for_key("terminal.docker_image") == "TERMINAL_DOCKER_IMAGE" + assert terminal_config_env_var_for_key("terminal.modal_mode") == "TERMINAL_MODAL_MODE" + # Non-terminal keys are not bridged. + assert terminal_config_env_var_for_key("tts.provider") is None