seb/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #31760 from NousResearch/hermes/hermes-bf5898da

Browse Source 
feat(docker)!: s6-overlay container supervision (salvage of #30136)
This commit is contained in:
Ben Barclay
2026-05-25 12:57:51 +10:00
committed by GitHub
parent 46f8948bad da8b2e95fd
commit 7e165e843d
47 changed files with 6004 additions and 231 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/tools/test_docker_environment.py
+17 -15
View File
@@ -385,18 +385,19 @@ def test_normalize_env_dict_rejects_complex_values():
assert result == {"GOOD": "string"}
def test_security_args_include_setuid_setgid_for_gosu_drop(monkeypatch):
def test_security_args_include_setuid_setgid_for_privdrop(monkeypatch):
"""The default (run_as_host_user=False) invocation must include SETUID and
SETGID caps so the image entrypoint can drop from root to the non-root
`hermes` user via gosu.
SETGID caps so the image's init can drop from root to a non-root user
(e.g. via ``s6-setuidgid`` in the bundled Hermes image, or ``gosu``/``su``
in user-provided images).
Without these caps gosu exits with
``error: failed switching to 'hermes': operation not permitted``
and the container exits immediately (exit 1) before running any work.
Without these caps the privilege-drop helper fails with
``operation not permitted`` and the container exits immediately (exit 1)
before running any work.
`no-new-privileges` is kept, so gosu still cannot escalate back to root
after the drop — the drop is a one-way transition performed before the
`no_new_privs` bit is enforced on the exec boundary.
``no-new-privileges`` is kept, so the dropped process still cannot
escalate back to root after the drop — the drop is a one-way transition
performed before the ``no_new_privs`` bit is enforced on the exec boundary.
"""
monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
calls = _mock_subprocess_run(monkeypatch)
@@ -412,8 +413,8 @@ def test_security_args_include_setuid_setgid_for_gosu_drop(monkeypatch):
for i, flag in enumerate(run_args[:-1])
if flag == "--cap-add"
}
assert "SETUID" in added, "SETUID cap missing — gosu drop in entrypoint will fail"
assert "SETGID" in added, "SETGID cap missing — gosu drop in entrypoint will fail"
assert "SETUID" in added, "SETUID cap missing — image privilege-drop will fail"
assert "SETGID" in added, "SETGID cap missing — image privilege-drop will fail"
# ── run_as_host_user tests ────────────────────────────────────────
@@ -441,8 +442,9 @@ def test_run_as_host_user_passes_uid_gid(monkeypatch):
def test_run_as_host_user_drops_setuid_setgid_caps(monkeypatch):
"""When --user is passed, the container never needs gosu, so SETUID/SETGID
caps are omitted for a tighter security posture."""
"""When --user is passed, the container already starts unprivileged and
never needs a privilege drop, so SETUID/SETGID caps are omitted for a
tighter security posture."""
monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker")
monkeypatch.setattr(docker_env.os, "getuid", lambda: 1000, raising=False)
monkeypatch.setattr(docker_env.os, "getgid", lambda: 1000, raising=False)
@@ -459,10 +461,10 @@ def test_run_as_host_user_drops_setuid_setgid_caps(monkeypatch):
if flag == "--cap-add"
}
assert "SETUID" not in added, (
"SETUID cap should be dropped when running as host user — no gosu drop is needed"
"SETUID cap should be dropped when running as host user — no privilege drop is needed"
)
assert "SETGID" not in added, (
"SETGID cap should be dropped when running as host user — no gosu drop is needed"
"SETGID cap should be dropped when running as host user — no privilege drop is needed"
)
# Core non-privilege-drop caps must still be there (pip/npm/apt need them).
assert "DAC_OVERRIDE" in added
tests/tools/test_dockerfile_pid1_reaping.py
+49 -14
View File
@@ -5,11 +5,17 @@ they deliberately avoid snapshotting specific package versions, line numbers,
or exact flag choices. What they DO assert is that the Dockerfile maintains
the properties required for correct production behaviour:
- A PID-1 init (tini) is installed and wraps the entrypoint, so that orphaned
- A PID-1 init is installed and wraps the entrypoint, so that orphaned
subprocesses (MCP stdio servers, git, bun, browser daemons) get reaped
instead of accumulating as zombies (#15012).
- Signal forwarding runs through the init so ``docker stop`` triggers
hermes's own graceful-shutdown path.
The init can be any reaper-capable PID-1: the historical lineage was
``tini``; the current image uses s6-overlay's ``/init`` (which execs
``s6-svscan`` as PID 1, with the same SIGCHLD-reaping property). The
checks below accept either family — the contract is behavioural, not
nominal.
"""
from __future__ import annotations
@@ -24,6 +30,21 @@ DOCKERFILE = REPO_ROOT / "Dockerfile"
DOCKERIGNORE = REPO_ROOT / ".dockerignore"
# Init-process families this repo accepts as PID 1. ``tini`` /
# ``dumb-init`` / ``catatonit`` are classic minimal reapers; s6-overlay
# ships ``/init`` which execs ``s6-svscan`` as PID 1 (same reaper
# contract, plus supervision of declared services). Either family
# satisfies the zombie-reaping invariant — see issue #15012.
_KNOWN_INIT_TOKENS: tuple[str, ...] = (
"tini",
"dumb-init",
"catatonit",
"s6-overlay",
"s6-svscan",
"/init",
)
@pytest.fixture(scope="module")
def dockerfile_text() -> str:
if not DOCKERFILE.exists():
@@ -57,8 +78,17 @@ def _run_steps(dockerfile_text: str) -> list[str]:
]
def _instruction_text(dockerfile_text: str) -> str:
"""Join every non-comment Dockerfile instruction into one searchable
string. Crucially excludes comments — otherwise the historical
explanation of "we used to use tini" would silently satisfy a
substring check long after tini was removed from the build.
"""
return "\n".join(_dockerfile_instructions(dockerfile_text))
def test_dockerfile_installs_an_init_for_zombie_reaping(dockerfile_text):
"""Some init (tini, dumb-init, catatonit) must be installed.
"""Some init (tini, dumb-init, catatonit, s6-overlay) must be installed.
Without a PID-1 init that handles SIGCHLD, hermes accumulates zombie
processes from MCP stdio subprocesses, git operations, browser
@@ -67,12 +97,17 @@ def test_dockerfile_installs_an_init_for_zombie_reaping(dockerfile_text):
"""
# Accept any of the common reapers. The contract is behavioural:
# something must be installed that reaps orphans.
known_inits = ("tini", "dumb-init", "catatonit")
installed = any(name in dockerfile_text for name in known_inits)
#
# Scan instructions only (no comments) so a stale historical mention
# in a comment can't masquerade as a current install. Without this,
# removing tini from the actual build but leaving the word in a
# comment would silently keep the test green.
instructions = _instruction_text(dockerfile_text)
installed = any(name in instructions for name in _KNOWN_INIT_TOKENS)
assert installed, (
"No PID-1 init detected in Dockerfile (looked for: "
f"{', '.join(known_inits)}). Without an init process to reap "
"orphaned subprocesses, hermes accumulates zombies in Docker "
"No PID-1 init detected in Dockerfile instructions (looked for: "
f"{', '.join(_KNOWN_INIT_TOKENS)}). Without an init process to "
"reap orphaned subprocesses, hermes accumulates zombies in Docker "
"deployments. See issue #15012."
)
@@ -80,8 +115,8 @@ def test_dockerfile_installs_an_init_for_zombie_reaping(dockerfile_text):
def test_dockerfile_entrypoint_routes_through_the_init(dockerfile_text):
"""The ENTRYPOINT must invoke the init, not the entrypoint script directly.
Installing tini is only half the fix — the container must actually run
with tini as PID 1. If the ENTRYPOINT executes the shell script
Installing the init is only half the fix — the container must actually
run with it as PID 1. If the ENTRYPOINT executes the shell script
directly, the shell becomes PID 1 and will ``exec`` into hermes,
which then runs as PID 1 without any zombie reaping.
"""
@@ -96,12 +131,12 @@ def test_dockerfile_entrypoint_routes_through_the_init(dockerfile_text):
assert entrypoint_line is not None, "Dockerfile is missing an ENTRYPOINT directive"
known_inits = ("tini", "dumb-init", "catatonit")
routes_through_init = any(name in entrypoint_line for name in known_inits)
routes_through_init = any(name in entrypoint_line for name in _KNOWN_INIT_TOKENS)
assert routes_through_init, (
f"ENTRYPOINT does not route through an init: {entrypoint_line!r}. "
"If tini is only installed but not wired into ENTRYPOINT, hermes "
"still runs as PID 1 and zombies will accumulate (#15012)."
f"ENTRYPOINT does not route through a PID-1 init: {entrypoint_line!r}. "
f"Expected one of {_KNOWN_INIT_TOKENS}. If the init is installed but "
"not wired into ENTRYPOINT, hermes still runs as PID 1 and zombies "
"will accumulate (#15012)."
)