mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-01 01:51:44 +00:00
feat(docker): run container as host user to avoid root-owned bind mounts
Add opt-in terminal.docker_run_as_host_user config flag that passes --user $(id -u):$(id -g) to the Docker backend so files written into bind-mounted directories (/workspace, /root, docker_volumes entries) are owned by the host user instead of root. When enabled on POSIX platforms, also drops SETUID/SETGID caps since the container no longer needs gosu/su to switch users. Falls back cleanly on platforms without os.getuid (e.g. native Windows Docker) with a warning. Wired through all three config.yaml -> TERMINAL_* env-var bridges: - cli.py env_mappings (CLI + TUI startup) - gateway/run.py _terminal_env_map (gateway / messaging platforms) - hermes_cli/config.py _config_to_env_sync (`hermes config set`) Also fixes docker_mount_cwd_to_workspace silently failing in gateway mode -- it was missing from gateway/run.py's _terminal_env_map. Adds tests/tools/test_terminal_config_env_sync.py to guard against future drift between the three bridges (same bug class shipped twice in one month). Bundled Hermes image won't work with this flag since its entrypoint expects to start as root for the usermod/gosu hermes flow; works with the default nikolaik/python-nodejs image and plain Debian/Ubuntu.
This commit is contained in:
parent
1d4218be56
commit
5531c0df82
10 changed files with 412 additions and 15 deletions
210
tests/tools/test_terminal_config_env_sync.py
Normal file
210
tests/tools/test_terminal_config_env_sync.py
Normal file
|
|
@ -0,0 +1,210 @@
|
|||
"""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:
|
||||
|
||||
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 …``)
|
||||
|
||||
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).
|
||||
|
||||
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.
|
||||
"""
|
||||
|
||||
import ast
|
||||
import inspect
|
||||
|
||||
|
||||
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``."""
|
||||
from hermes_cli import config as hc_config
|
||||
source = inspect.getsource(hc_config.set_config_value)
|
||||
keys = _extract_dict_keys(source, "_config_to_env_sync")
|
||||
# set_config_value uses fully-qualified ``terminal.foo`` keys; strip the
|
||||
# prefix so we can compare against the other two maps which use bare
|
||||
# leaf keys.
|
||||
return {k.split(".", 1)[1] for k in keys if k.startswith("terminal.")}
|
||||
|
||||
|
||||
# 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 _terminal_tool_env_var_names() -> set[str]:
|
||||
"""All TERMINAL_* env vars actually consumed by terminal_tool."""
|
||||
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
|
||||
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.
|
||||
|
||||
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.
|
||||
"""
|
||||
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 several
|
||||
pre-existing terminal keys (ssh_*, docker_forward_env, docker_volumes)
|
||||
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's ownership flag so the bug we just
|
||||
fixed cannot silently regress.
|
||||
"""
|
||||
save_keys = _save_config_env_sync_keys()
|
||||
required = {
|
||||
"docker_run_as_host_user",
|
||||
"docker_mount_cwd_to_workspace",
|
||||
"backend",
|
||||
"docker_image",
|
||||
"container_cpu",
|
||||
"container_memory",
|
||||
"container_disk",
|
||||
"container_persistent",
|
||||
}
|
||||
missing = required - save_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."
|
||||
)
|
||||
|
||||
|
||||
def test_docker_run_as_host_user_is_bridged_everywhere():
|
||||
"""Explicit pin for the bug we just fixed.
|
||||
|
||||
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.
|
||||
"""
|
||||
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()
|
||||
|
||||
|
||||
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.
|
||||
"""
|
||||
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()
|
||||
Loading…
Add table
Add a link
Reference in a new issue