Docker s6 - #30136
Closed
benbarclay wants to merge 36 commits into
Closed
Conversation
alt-glitch
added
type/feature
Collaborator
|
@daimon-nous bossman can u look thru this PR, set up the environment. test the docker e2e using test profiles and whatnot. chart out the testing across profile+config, as well as different gateways. oh and see if s6 can also help with env var/secret isolation (like if say, gh auth token needs to be isolated, unreadable -- maybe look into the gh container broker stuff) |
Replace tini with s6-overlay as PID 1 in the Hermes Docker image so that main hermes, the dashboard, and dynamically-created per-profile gateways all run as supervised services. Includes container-boot reconciliation (Task 4.0) so per-profile gateways survive docker restart. Plan history: - v1: 2026-05-07 — original design (subagent gateways scope) - v2: 2026-05-18 — re-validated, scope narrowed to per-profile gateways, WindowsServiceManager added to protocol - v3: 2026-05-21 — re-validated in docker_s6 worktree, install-method stamp preservation noted in Task 2.3, Task 4.0 added for container restart survival 12.5 engineering days estimated across 7 phases.
Task 0.1 of the s6-overlay supervision plan. Establishes the test infrastructure for tests/docker/: skip-on-missing-Docker collection hook, session-scoped image-build fixture (overridable via the HERMES_TEST_IMAGE env var for faster local iteration), and a container_name fixture that ensures cleanup on test exit. Refs: docs/plans/2026-05-07-s6-overlay-dynamic-subagent-gateways.md
Tasks 0.2-0.6 of the s6-overlay supervision plan. Locks the user-visible behavior we must preserve through the Phase 2 init- system swap: - test_main_invocation.py (Task 0.2): docker run <image> with no args, chat subcommand passthrough, bare executable passthrough, bash pattern, exit-code propagation - test_tui_passthrough.py (Task 0.3): TTY allocation via docker -t using the host's script(1) for a PTY - test_dashboard.py (Task 0.4): HERMES_DASHBOARD=1 opt-in, HERMES_DASHBOARD_PORT override - test_profile_gateway.py (Task 0.5): per-profile gateway start/stop and profile-delete-stops-gateway. Both marked xfail(strict=True) because the current tini image refuses gateway lifecycle commands inside the container; Phase 4 Task 4.3 flips them to passing. - test_zombie_reaping.py (Task 0.6): PID 1 reaps orphaned zombies. tini does this today; s6-overlay's /init must continue to. Refs: docs/plans/2026-05-07-s6-overlay-dynamic-subagent-gateways.md
The agent-test suite default is 30s; docker test_no_args (the dashboard spin-up, the container restart) routinely take 60-90s. Without this they intermittently fail in CI with TimeoutError.
Two pre-existing baseline issues found while running the Phase 0 harness against the tini image that need fixing before later phases can use the harness as a behavior-parity oracle: 1. The autouse `_enforce_test_timeout` fixture in tests/conftest.py hard-coded a 30s SIGALRM, which preempted any `pytest.mark.timeout` marker (already honored by pytest-timeout). Honor the marker if present; fall back to 30s otherwise. Docker harness tests carry a 180s marker applied at collection time in tests/docker/conftest.py. 2. test_dashboard_port_override polled via `ss -tlnp` / `netstat -tln` — neither is installed in the Hermes image, so the probe trivially failed even when the dashboard was bound. The dashboard also takes 8-15s to bind on cold image; the 5s sleep was insufficient. Replace with a poll loop reading /proc/net/tcp directly (port 9120 = 0x23A0, state 0A = LISTEN). Bump probe deadline to 60s and switch test_dashboard_opt_in_starts to a similar poll for pgrep so we don't regress to the same race. Result: 11 passed, 2 xfailed (Phase 4 target) on tini image. Harness now ready to serve as Phase 2's behavior-parity oracle.
Phase 0.5 of the s6-overlay supervision plan. Catches Dockerfile and shell-script regressions that the behavioral docker-publish smoke test can't surface — unquoted variable expansions, silently-failing RUN commands, missing apt-get clean, etc. Both lint clean against the current (tini) Dockerfile + entrypoint.sh at the configured thresholds (hadolint: warning, shellcheck: error). Each ignore in .hadolint.yaml carries a one-line justification; the shellcheck severity floor is documented in the workflow file. Refs: docs/plans/2026-05-07-s6-overlay-dynamic-subagent-gateways.md
Phase 1 of the s6-overlay supervision plan. Pure-refactor addition: introduces the abstract interface (with runtime_checkable Protocol), detect_service_manager(), validate_profile_name(), and thin SystemdServiceManager / LaunchdServiceManager / WindowsServiceManager wrappers around the existing systemd_* / launchd_* / gateway_windows.* module-level functions. No host call site was modified — host code continues to use the existing functions directly; the protocol is for new backend-agnostic code (Phase 4 profile create/delete hooks and the Phase 4 s6 dispatch path in 'hermes gateway start/stop/restart'). WindowsServiceManager.install() forwards the v3 kwargs (start_now, start_on_login, elevated_handoff) added in PRs #28169-adjacent so non-Windows callers — there aren't any today — can opt in. The s6 backend lands in Phase 3; until then get_service_manager() raises a clear error if invoked on a host that detects as 's6'.
BREAKING CHANGE: the container ENTRYPOINT is now /init (s6-overlay)
instead of /usr/bin/tini. Main hermes runs as the container CMD with
TTY inherited (preserving --tui), dashboard runs as a supervised s6-rc
service (HERMES_DASHBOARD=1 starts it; crashes auto-restart), and the
ground is laid for per-profile gateway supervision (Phase 3+4).
All five pre-s6 docker run invocation patterns continue to work
identically — verified by the Phase 0 docker harness:
docker run <image> → `hermes` with no args
docker run <image> chat -q "..." → `hermes chat -q ...` passthrough
docker run <image> sleep infinity → `sleep infinity` direct
docker run <image> bash → interactive bash
docker run -it <image> --tui → interactive Ink TUI
Phase 2 harness result: 12 passed, 2 xfailed (Phase 4 target). Hadolint
+ shellcheck pass cleanly.
Architecture pivot from plan v3 (documented in main-hermes/run header):
the plan called for main hermes to be an s6-supervised service, but
two real s6-overlay v3 mechanics blocked that — cont-init.d scripts
receive no arguments (CMD args are not visible to stage2-hook), and
`/run/s6/basedir/bin/halt` after writing the exit code did not
propagate the desired exit code (container exits 143). We use the
s6-overlay-native CMD pattern instead: main-wrapper.sh is the
container's main program (ENTRYPOINT prepends it so leading-dash
args like --version aren't intercepted by /init), exec's the final
program with stdin/stdout/stderr inherited, and the program's exit
code becomes the container exit code. main-hermes is now a no-op
`sleep infinity` slot kept for future supervised-gateway-container
modes. This trades "supervised restart of main hermes" for arg-
parity with the pre-s6 contract — main hermes was already unsupervised
under tini, so we lose nothing functional. Dashboard supervision is
the only new guarantee added by this phase.
Files added:
docker/main-wrapper.sh # arg routing + s6-setuidgid drop
docker/stage2-hook.sh # gosu-equivalent + chown + seed
docker/s6-rc.d/main-hermes/{type,run,dependencies.d/base}
docker/s6-rc.d/dashboard/{type,run,dependencies.d/base}
docker/s6-rc.d/user/contents.d/{main-hermes,dashboard}
Files changed:
Dockerfile: tini → s6-overlay install + ENTRYPOINT flip + service wiring
docker/entrypoint.sh: thin shim to stage2-hook.sh for back-compat
tests/docker/test_dashboard.py: add test_dashboard_restarts_after_crash
Refs: docs/plans/2026-05-07-s6-overlay-dynamic-subagent-gateways.md
…vision
Phase 3 of the s6-overlay supervision plan. Implements the runtime-
registration surface from D4 — only the s6 backend supports
register_profile_gateway / unregister_profile_gateway /
list_profile_gateways; host backends continue to raise
NotImplementedError. No caller yet (Phase 4 wires in the profile
create/delete hooks).
Key implementation notes:
- Service directory shape: /run/service/gateway-<profile>/{type,run,log/run}.
Atomic register: write to gateway-<profile>.tmp, fsync via
os.rename. Cleanup on rescan failure.
- Run script uses #!/command/with-contenv sh so HERMES_HOME and any
extra_env arrive at exec time. The hermes -p <profile> gateway
start --foreground --port <port> command is wrapped in
s6-setuidgid hermes for the per-service privilege drop (OQ2-A).
- Log script (OQ8-C): persists via s6-log to
${HERMES_HOME}/logs/gateways/<profile>/. CRITICAL — HERMES_HOME is
a runtime env-var expansion in the rendered script, NOT a Python
f-string substitution. Negative-asserted in
test_s6_register_creates_service_dir_and_triggers_scan so
regressions are caught.
- PATH gotcha: /command/ is only on PATH for processes spawned by
the supervision tree (services, cont-init.d). `docker exec` and
profile-create hooks don't get it. S6ServiceManager calls all
s6-* binaries via absolute path through the new _S6_BIN_DIR
constant so callers don't have to fix up env vars.
- validate_profile_name rejects path-traversal, leading-dash (s6
would parse as a flag), uppercase, whitespace, and names >251
chars (s6-svscan default name_max).
Test coverage:
- 13 new unit tests in tests/hermes_cli/test_service_manager.py
(kind detection, run-script content, env quoting, register
rollback on rescan failure, unregister idempotence, list filter,
lifecycle dispatch, svstat parsing). Total: 36 passing.
- 2 new in-container integration tests in
tests/docker/test_s6_profile_gateway_integration.py validating
end-to-end registration against a real s6 supervision tree.
Docker harness: 14 passed, 2 xfailed (Phase 4 target unchanged).
Refs: docs/plans/2026-05-07-s6-overlay-dynamic-subagent-gateways.md
…iation
Phase 4 of the s6-overlay supervision plan. Activates the Phase 3
S6ServiceManager by hooking it into the profile lifecycle and the
`hermes gateway start/stop/restart` dispatcher, and adds a cont-
init.d-time reconciliation pass that survives `docker restart`.
Task 4.0 — container-boot reconciliation:
/run/service/ is tmpfs, so every `docker restart` wipes every
per-profile gateway slot. /etc/cont-init.d/02-reconcile-profiles
invokes hermes_cli.container_boot.reconcile_profile_gateways() on
every boot, which walks $HERMES_HOME/profiles/<name>/, reads each
gateway_state.json, recreates the s6 service slot, and auto-starts
only those whose last state was 'running'. Other states
(stopped, starting, startup_failed, missing) register the slot
in the down state — avoiding crash-loops across restarts for a
gateway that was broken last boot. Per-profile outcome is recorded
to $HERMES_HOME/logs/container-boot.log.
Implementation: hermes_cli/container_boot.py + 12 unit tests.
Profile-marker is SOUL.md, not config.yaml, because `hermes profile
create` only seeds SOUL.md by default (config.yaml comes from
`hermes setup`).
Task 4.1 / 4.2 — profile create/delete hooks:
hermes_cli/profiles.py::create_profile now calls
_maybe_register_gateway_service(<canon>) at the end, which routes
through ServiceManager.register_profile_gateway when running on s6
and no-ops on host backends. delete_profile mirrors with
_maybe_unregister_gateway_service. _allocate_gateway_port produces
a deterministic SHA-256-derived port in [9200, 9800).
Task 4.3 — gateway dispatch + remove rejection arms:
_dispatch_via_service_manager_if_s6(action) intercepts
start/stop/restart at the top of each subcommand and routes them
through S6ServiceManager.{start,stop,restart}. The pre-Phase-4
`elif is_container():` rejection arms are kept as fallback for
pre-s6 containers / unsupported runtimes, but only ever fire when
detect_service_manager() != 's6'. install/uninstall under s6
print informational guidance pointing users at profile create/delete.
Removed the two xfail(strict=True) markers from
tests/docker/test_profile_gateway.py — both tests now pass strictly.
Task 4.4 — status reporting:
get_gateway_runtime_snapshot() reports
Manager: 's6 (container supervisor)' inside an s6 container instead
of 'docker (foreground)'.
Plan-vs-reality drift fixed in this commit:
- Plan's S6ServiceManager._render_run_script used
`gateway start --foreground --port {port}` — invented args; the
real CLI is `gateway run`. Switched accordingly. port arg
retained for API parity but now documented as 'currently ignored'.
- Plan's reconciler keyed on config.yaml; switched to SOUL.md
(config.yaml is created by hermes setup, not by hermes profile
create, so the original gate caught nothing).
- The plan's _dispatch helper used _profile_arg() which returns
'--profile <name>' (i.e. with the flag prefix). Switched to
_profile_suffix() which returns the bare name.
- Architecture B's docker exec doesn't get /command on PATH or
the venv on PATH; Dockerfile's runtime PATH now includes
/opt/hermes/.venv/bin so 'docker exec <c> hermes ...' works
without sourcing the venv.
- stage2-hook now chowns $HERMES_HOME/profiles to hermes on every
boot, not just on the UID-remap path. Without this, files created
by docker-exec-as-root accumulate and the next reconciler run
fails with PermissionError reading SOUL.md.
Test harness:
19 passed, 0 xfailed (the two pre-Phase-4 xfail targets flip to
passing). 78 unit tests across service_manager + container_boot +
profiles_s6_hooks + gateway_s6_dispatch. Hadolint + shellcheck
pass cleanly.
Refs: docs/plans/2026-05-07-s6-overlay-dynamic-subagent-gateways.md
… updates
Phase 5 of the s6-overlay supervision plan. Documentation + small
diagnostic cleanups; no behavior changes.
website/docs/user-guide/docker.md:
- Replace the old 'entrypoint script does the bootstrap' section
with the s6-overlay boot flow (cont-init.d/01-hermes-setup,
cont-init.d/02-reconcile-profiles, static main-hermes + dashboard
services, ENTRYPOINT-as-main-program pattern).
- Add a 'Per-profile gateway supervision' subsection covering the
new lifecycle commands, restart semantics, log persistence, and
'Manager: s6 (container supervisor)' status reporting.
- Add 'Breaking change vs. pre-s6 images' callout naming the
/init ENTRYPOINT and pointing affected wrappers at the pin
workaround.
website/docs/user-guide/profiles.md:
- Add a note under 'Persistent services' pointing container users
at the docker.md section explaining s6 supervision inside the
image. Host-side systemd/launchd documentation is unchanged.
skills/software-development/hermes-s6-container-supervision/SKILL.md:
- New maintainer skill covering the supervision-tree map, file
layout, the Architecture B rationale (cont-init.d args + halt
exit-code propagation), quick recipes, and the 8 pitfalls we hit
while implementing the plan (PATH-without-/command, root-owned
profile dirs, SOUL.md as marker, the '143' anti-pattern, etc.).
hermes_cli/doctor.py:
- _check_gateway_service_linger skips on s6 (the linger concept
doesn't apply inside the container).
- New _check_s6_supervision section reports main-hermes/dashboard
state and per-profile-gateway count (registered vs supervised
up), only inside the s6 container. Host doctor output unchanged.
- External Tools / Docker check no longer emits a 'docker not
found' warning inside the container; prints an explanatory
info line instead. Still respects an explicit TERMINAL_ENV=docker
(in case the user mounted /var/run/docker.sock).
hermes_cli/gateway.py:
- Document _container_systemd_operational more precisely: it's
NOT for our Hermes Docker image (s6-overlay handles that via
detect_service_manager() == 's6'). It still covers
systemd-nspawn / k8s-with-systemd-init cases, so leaving it in
place is correct; the docstring just makes that explicit.
Test harness (verification, no test changes in this commit):
19 passed, 0 xfailed. 66 service-manager / container-boot /
profiles-s6-hooks / gateway-s6-dispatch unit tests still green.
61 doctor tests still green. Hadolint + shellcheck clean.
Refs: docs/plans/2026-05-07-s6-overlay-dynamic-subagent-gateways.md
…ivilege drop The s6-overlay migration replaced every runtime use of gosu with s6-setuidgid (in stage2-hook.sh, main-wrapper.sh, per-service run scripts, and cont-init.d hooks), but the gosu binary itself was still being copied into the image from tianon/gosu, and several comments across the repo still pointed to it. Image changes: - Drop the FROM tianon/gosu:1.19-trixie AS gosu_source stage - Drop the COPY --from=gosu_source /gosu /usr/local/bin/ layer - Net: one fewer base-image pull, ~12-15 MB layer eliminated Documentation/comment refresh (no behavior change): - Dockerfile: update root-user rationale comment + cont-init.d comment - docker/main-wrapper.sh: drop "pre-s6 contract (gosu drop)" reference - docker-compose.yml: update UID/GID remap comment - .hadolint.yaml: update DL3002 ignore rationale - website/docs/user-guide/docker.md: privilege-drop helper is s6-setuidgid now - hermes_cli/config.py: docker_run_as_host_user docstring tools/environments/docker.py runs *arbitrary user images* via the terminal backend, not the bundled Hermes image. It still needs SETUID/ SETGID caps so user images that use gosu/su/s6-setuidgid all work. Renamed the cap-list constant _GOSU_CAP_ARGS → _PRIVDROP_CAP_ARGS and updated comments to list s6-setuidgid alongside the others as examples. The matching test (test_security_args_include_setuid_setgid_for_gosu_drop → test_security_args_include_setuid_setgid_for_privdrop) was renamed and its docstring updated; behavior is unchanged. Verification: - hadolint clean against .hadolint.yaml - shellcheck clean against all docker/ shell scripts - Image rebuilt successfully (sha 1a090924ccea) - Docker harness: 19 passed in 41.87s (every Phase 0 test + Phase 4 per-profile-gateway lifecycle + container-restart reconciliation) - tests/tools/test_docker_environment.py: 23 passed (rename did not break test discovery; pre-existing unrelated mock warning) The plan document (docs/plans/2026-05-07-s6-overlay-dynamic-subagent-gateways.md) intentionally retains its historical references to gosu — it describes the pre-s6 entrypoint as background for understanding the migration.
Contributor
🔎 Lint report:
|
| Rule | Count |
|---|---|
unused-type-ignore-comment |
3 |
First entries
tests/gateway/test_discord_roles_dm_scope.py:277: [unused-type-ignore-comment] unused-type-ignore-comment: Unused blanket `type: ignore` directive
tests/test_lint_config.py:28: [unused-type-ignore-comment] unused-type-ignore-comment: Unused blanket `type: ignore` directive
tests/plugins/test_google_meet_node.py:475: [unused-type-ignore-comment] unused-type-ignore-comment: Unused blanket `type: ignore` directive
✅ Fixed issues (1029):
| Rule | Count |
|---|---|
unresolved-import |
1029 |
First entries
tests/gateway/test_cancel_background_drain.py:14: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/fakes/fake_ha_server.py:25: [unresolved-import] unresolved-import: Cannot resolve imported module `aiohttp`
tests/tools/test_browser_content_none_guard.py:14: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_keep_typing_timeout.py:27: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_sse_agent_cancel.py:14: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_stuck_loop.py:12: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/skills/test_youtube_quiz.py:9: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/hermes_cli/test_startup_plugin_gating.py:29: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/run_agent/conftest.py:23: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_fast_command.py:9: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_stream_consumer.py:7: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/plugins/model_providers/test_deepseek_profile.py:15: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/tools/test_credential_files.py:8: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/acp/test_ping_suppression.py:128: [unresolved-import] unresolved-import: Cannot resolve imported module `acp`
tests/gateway/test_ntfy_plugin.py:21: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_telegram_reply_mode.py:12: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_webhook_adapter.py:27: [unresolved-import] unresolved-import: Cannot resolve imported module `aiohttp.test_utils`
tests/tools/test_tts_mistral.py:6: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/run_agent/test_860_dedup.py:17: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_auth_fallback.py:6: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_whatsapp_formatting.py:12: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_session_state_cleanup.py:22: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/hermes_cli/test_auth_ssl_macos.py:20: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_pending_drain_no_recursion.py:26: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/cli/test_cli_shift_enter_newline.py:9: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
... and 1004 more
Unchanged: 3833 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
Contributor
|
Thanks Ben — careful work and the commit messages are excellent. Three blockers, then a bigger question. Blockers (pre-merge):
Smaller issues:
Bigger question — scope: Before this lands I want to push back on the architecture-rewrite framing. 6811 LOC + a 3191-line plan doc + replacing PID 1 across the whole image is a lot of surface for what reads (today) as two concrete user-visible wins:
The first I want. It's small — could be a focused PR around the static The second I'd defer. "One container supervising N internal gateways" is a pattern we'd be inventing — the Docker-idiomatic shape is one process per container, multiple profiles via docker-compose. To support the in-container fan-out we'd carry forward, forever, the cont-init.d reconciler + tmpfs/persistent-volume bridge + stale-PID cleanup + ServiceManager protocol. The protocol itself is mostly speculative: the Systemd / Launchd / Windows wrappers are thin facades around existing module-level functions, and there's no second non-s6 backend on the horizon — it's an abstraction built for a swap that isn't coming. The plan title is "dynamic-subagent-gateways" — that's the consumer I'd want to see before merging the supervision infrastructure. Is there a concrete subagent-gateways design landing soon that needs this? If yes, push back and I'll re-read with that in mind. If it's "build the supervision layer now so the agent feature can land cleanly later," I'd rather wait for the agent feature and let it inform the supervision shape. What I'd want to merge from this PR right now:
What I'd defer:
Happy to be wrong about scope — if the per-profile-in-one-container model has a consumer I'm not seeing, lay it out and I'll re-read. |
Contributor
Review: PR #30136 — Docker s6-overlay multi-process containerStatus: DRAFT | Author: @benbarclay | +6794 / −215 across 40 files | 1 commit behind main, merges cleanly Replaces tini+gosu with s6-overlay as PID 1. Adds Verdict: Approve with suggestionsThis is excellent infrastructure work — the architecture is clean, the test coverage is thorough (~66 unit tests + ~19 Docker integration tests), no core agent files are touched, and it follows the "Extend, Don't Duplicate" principle correctly. The s6 migration is well-motivated (zombie reaping via #15012 was just the start — supervised restarts and per-profile gateways are the real payoff). Suggestions below are ordered by severity. 🔴 Must-fix before merge1. No checksum verification on s6-overlay downloads The three # Current (no integrity check):
ADD https://github.com/just-containers/s6-overlay/releases/download/v${S6_OVERLAY_VERSION}/s6-overlay-noarch.tar.xz /tmp/
# Suggested pattern:
ARG S6_NOARCH_SHA256=<sha256>
ADD https://github.com/just-containers/s6-overlay/releases/download/v${S6_OVERLAY_VERSION}/s6-overlay-noarch.tar.xz /tmp/
RUN echo "${S6_NOARCH_SHA256} /tmp/s6-overlay-noarch.tar.xz" | sha256sum -c -The old gosu source used 2. Stale dashboard supervision note in Says: "The dashboard side-process is not supervised — if it crashes, it stays down" But the dashboard IS now a supervised s6 🟡 Should-fix (important but non-blocking)3. Stale docker-compose.yml comment (lines 18-20) Still says: "If you override entrypoint, keep /opt/hermes/docker/entrypoint.sh in the command chain. It drops root to the hermes user before gateway files." The entrypoint is now 4. x86_64 hardcoded — ARM builds are broken The PR acknowledges this ( 5.
Half-implemented contracts confuse future readers. The collision analysis (birthday-problem rate of ~28% at 20 profiles) is moot if the number is never used. 6. Plan document is 3,191 lines
🟢 Observations (non-blocking, for awareness)7.
8. Lines 78-81, 86, 111-113 use 9. Append-only with no rotation (~80 bytes × profiles × restarts). Not urgent but will grow on long-lived containers. 10. Non-atomic service-dir population in Unlike 11. Reduced to a 10-line shim that forwards to 12. Docker integration tests use
Architecture summaryThe PR cleanly separates concerns:
No core agent files touched (✅). |
Contributor
E2E Test Results — Built and ran the imagesImages built
A/B parity ✅
s6 supervision verified ✅
Integration test suite: 19/19 passed ✅All tests in 🔴 BUG FOUND:
|
PR #30136 review surfaced two issues, both rooted in the same audit gap: docker integration tests were running as root, not the unprivileged `hermes` user (UID 10000) that the runtime actually uses via `s6-setuidgid hermes`. Anything that probed PID-1 state or wrote to the s6 control surface worked as root in the tests but was inert in production. Fixes: 1. `_s6_running()` previously called `Path("/proc/1/exe").resolve()`, which is root-only readable. For UID 10000 the symlink yields PermissionError, `resolve()` silently returns the unresolved path, and `exe.name == "exe"` — so detection always returned False, the service-manager runtime-registration path was inert, and every `hermes profile create` / `hermes -p X gateway start` silently skipped the s6 hook. Replace with `/proc/1/comm` (world-readable) + `/run/s6/basedir` (s6-overlay-specific) — both required, fail closed. 2. `02-reconcile-profiles` now also chowns `/run/service/.s6-svscan/` {control,lock} to hermes so `s6-svscanctl -a/-an` works without root. Previously the directory chown stopped at `/run/service` and the FIFO inside stayed root-owned, so `register_profile_gateway` from hermes failed at the rescan-trigger step with EACCES — the wrapper in profiles.py caught the exception and printed a swallowed warning, so profile creation appeared to succeed while the slot was rolled back. Audit changes to flush this class of bug next time: - Add `docker_exec` / `docker_exec_sh` helpers to `tests/docker/conftest.py` that default to `-u hermes`. The module docstring explains why and flags `user="root"` as opt-in only for tests that explicitly need root (none currently do). - Refactor every `docker exec` call in tests/docker/ through the new helpers (test_dashboard.py, test_zombie_reaping.py, test_profile_gateway.py, test_container_restart.py, test_s6_profile_gateway_integration.py). - Add 5 unit tests covering `_s6_running` under various probe states (both signals present; comm wrong; basedir missing; PermissionError on /proc/1/comm; missing /proc — non-Linux). The PermissionError test is the explicit regression guard for the original bug. Known follow-up: the per-service `supervise/control` FIFO inside each `/run/service/gateway-<profile>/supervise/` is created root-owned by s6-supervise (which runs as root because s6-svscan is PID 1). `s6-svc -u/-d/-t` from the hermes user will get EACCES on those. The audit under `-u hermes` will reveal this in lifecycle tests — surfacing the issue cleanly so it can be fixed in a focused follow-up (likely via a small SUID helper or a polling chown loop in cont-init.d). The detection + svscanctl fixes here are independent and complete on their own.
The Dockerfile only ADD'd `s6-overlay-x86_64.tar.xz`, so the `build-arm64` job in docker-publish.yml — which runs on `ubuntu-24.04-arm` and publishes by digest — produced an image whose `/init` couldn't exec on actual arm64 hosts. Apple Silicon and ARM server users were getting a broken container. Map BuildKit's `TARGETARCH` (`amd64` / `arm64`) to s6's kernel-arch naming (`x86_64` / `aarch64`) inside the RUN step and fetch the correct tarball via `curl` (`ADD`'s URL is evaluated at parse time, before TARGETARCH substitution, so dynamic arch selection requires RUN). The noarch + symlinks tarballs are architecture-independent and stay as ADDs. The audit case is now explicit: unsupported architectures fail loudly at build time rather than producing a silently-broken image.
PR #30136 review flagged the s6-overlay install as a supply-chain regression vs the gosu source it replaced — `tianon/gosu` was digest-pinned via `FROM ...@sha256:...`, but the three new ADD/curl downloads had no integrity check at all. Pin all three tarballs (noarch, symlinks-noarch, per-arch) to upstream-published SHA256s via ARGs. Verification happens via `sha256sum -c` against a single checksum file (avoids a piped-shell hadolint DL4006 warning under dash). To bump S6_OVERLAY_VERSION, fetch the four `.sha256` files from the new release and update the ARGs — documented inline. If upstream artifacts are tampered with mid-build, the build now fails loudly at the verification step instead of silently producing a tainted image.
PR #30136 review caught a silent regression: the smoke-test action overrode ENTRYPOINT to `/opt/hermes/docker/entrypoint.sh`, which the s6-overlay migration reduced to a shim that just `exec`s the stage2 hook. stage2-hook ignores its CMD args, prints "Setup complete", and exits 0 — so `hermes --help` and `hermes dashboard --help` never ran. The #9153 regression guard was a green-always no-op. Drop the override so the smoke test uses the image's real ENTRYPOINT chain (`/init` + `main-wrapper.sh`), which is the actual production startup path. `hermes --help` and `hermes dashboard --help` now run through the full supervision tree and exercise the real argv routing.
PR #30136 review caught that `hermes gateway stop --all` and `... restart --all` were broken under s6. The Phase 4 dispatcher was gated on `not stop_all` (and the symmetric restart_all), so `--all` fell through to `kill_gateway_processes(all_profiles=True)`. pkill SIGTERMed every gateway, s6-supervise observed the crashes, and restarted every gateway ~1s later — net effect: `--all` *kicked* gateways instead of *stopping* them. Add `_dispatch_all_via_service_manager_if_s6(action)` that iterates `mgr.list_profile_gateways()` and routes stop/restart through each service slot. s6's `want up`/`want down` flips correctly, so a stop persists. Partial failures are surfaced per-profile with a running success count; the host pkill path is only reached when s6 isn't in play. `start --all` isn't a CLI surface — the helper rejects it and returns False (host code path can take over).
… the PR PR #30136 review caught that website/docs/user-guide/docker.md still said "The dashboard side-process is **not supervised** — if it crashes, it stays down until the container restarts." That was true under tini but is the opposite of the s6 behavior this PR ships and `test_dashboard_restarts_after_crash` proves. Replace with a description of what users actually see now: automatic restart by s6-overlay, new PID after a short backoff, logs via `docker logs`. The standalone-container caveat carries forward unchanged.
PR #30136 review caught: `hermes gateway start` (no `-p`) inside the container resolves `_profile_suffix() == ""` → service name `gateway-default`, but no such slot was ever registered. The Phase 4 profile-create hook only fired on `hermes profile create <name>`, and the root profile (which lives at the top of $HERMES_HOME, not under `profiles/`) was never one of those. So bare `hermes gateway start` landed on `s6-svc -u /run/service/gateway-default` → uncaught `CalledProcessError` → traceback to the user. Changes: 1. `reconcile_profile_gateways` now always registers a `gateway-default` slot before iterating named profiles. Its prior state is read from `$HERMES_HOME/gateway_state.json` (sibling to the profile root, not under `profiles/`); stale runtime files there are swept the same way. Auto-up only if the prior state was `running` — same rule as named profiles. 2. `S6ServiceManager._render_run_script` special-cases `profile == "default"` to emit `hermes gateway run` with NO `-p` flag. Passing `-p default` would resolve to `$HERMES_HOME/profiles/default/` — a different profile that almost certainly doesn't exist. The empty profile-suffix convention is the dispatcher's contract and the run script has to match. 3. A user-created `profiles/default/` collides with the reserved root-profile slot; the reconciler now skips it with a warning rather than producing two registrations of the same service name. Action-list ordering is stable: `default` first, then named profiles in directory order. Boot-log readers can rely on this. Tests: 8 new dedicated default-slot tests plus updates to every existing test that asserted against the action list (via the new `_named_actions` helper that drops the always-present default entry).
Kailigithub
pushed a commit
to Kailigithub/hermes-agent
that referenced
this pull request
May 25, 2026
…ilures PR NousResearch#30136 review caught: `S6ServiceManager.start/stop/restart` called `subprocess.run(check=True)` on `s6-svc`, so any failure surfaced as a raw `CalledProcessError` traceback. The two cases operators actually hit are: 1. The service slot doesn't exist — most commonly because the user typed a profile name wrong (`hermes -p typo gateway start`). 2. s6-svc itself fails — most commonly EACCES on the supervise control FIFO when running unprivileged. Both deserve named errors with actionable messages, not stacktraces. Changes: * Add `S6Error` base + two concrete errors in `hermes_cli.service_manager`: - `GatewayNotRegisteredError(profile)` — carries the unprefixed profile name; message: `no such gateway 'typo': register it with `hermes profile create typo` first, or pass an existing profile name via `-p <name>``. - `S6CommandError(service, action, returncode, stderr)` — carries the s6-svc rc and stderr; message: `s6-svc start on 'gateway-coder' failed (rc=111): <stderr>`. * Factor lifecycle dispatch through `_run_svc(flag, label, name)`: pre-checks that the service directory exists (raises GatewayNotRegisteredError before invoking s6-svc), then runs s6-svc and translates any CalledProcessError into S6CommandError. * `_dispatch_via_service_manager_if_s6` in `hermes_cli.gateway` catches both errors and prints `✗ <message>` + `sys.exit(1)` instead of letting the exception bubble. The dispatch path that used to dump a traceback at the user now gives an actionable one-liner. Tests: 6 new tests for the error types and their CLI rendering; existing lifecycle test pre-seeds the slot directory before calling `mgr.start` etc.
Kailigithub
pushed a commit
to Kailigithub/hermes-agent
that referenced
this pull request
May 25, 2026
PR NousResearch#30136 review caught a false positive: when HERMES_DASHBOARD was unset, the dashboard run script did `exec sleep infinity`, so `s6-svstat /run/service/dashboard` reported the slot as 'up'. `hermes doctor` and any other s6-svstat-based health check saw the dashboard as supervised-running even though no dashboard process existed. Add cont-init.d/03-dashboard-toggle: writes a `down` marker file into `/run/service/dashboard/` when HERMES_DASHBOARD is falsy, removes any leftover marker when it's truthy. s6-supervise honors `down` by not starting the service, so s6-svstat reports 'down' — matching reality. The run script's HERMES_DASHBOARD case-statement stays in place as a belt-and-suspenders guard, so the two layers can never disagree. Two new integration tests lock the behavior: slot reports down when unset; slot reports up when set to 1.
Kailigithub
pushed a commit
to Kailigithub/hermes-agent
that referenced
this pull request
May 25, 2026
PR NousResearch#30136 review caught: docker-compose.yml still said "If you override entrypoint, keep /opt/hermes/docker/entrypoint.sh in the command chain." That was true under tini; under s6-overlay the entrypoint is /init plus main-wrapper.sh, and entrypoint.sh is now only a backward-compat shim. Replace with an accurate description: /init must remain first in the chain because it's PID 1 and runs the cont-init.d scripts (chown, profile reconcile, dashboard toggle) before any service starts.
Kailigithub
pushed a commit
to Kailigithub/hermes-agent
that referenced
this pull request
May 25, 2026
PR NousResearch#30136 review caught: `_allocate_gateway_port()` in profiles.py computed a SHA-256-derived port that was threaded through `register_profile_gateway(profile, port=N)` → `_render_run_script(profile, port, extra_env)` → and then **ignored**. The rendered run script picked the bind port from the profile's config.yaml (`[gateway] port = …`), never from the allocator. So the entire allocator + parameter chain was dead code. Remove: * `hermes_cli.profiles._allocate_gateway_port` (deterministic SHA-256 → [9200, 9800) — never used). * `port` kwarg from `ServiceManager.register_profile_gateway` (Protocol + Mixin + S6 implementation). * `port` positional arg from `_render_run_script(profile, port, extra_env)` — now `_render_run_script(profile, extra_env)`. * The pass-through call in `profiles._maybe_register_gateway_service`. config.yaml is now the single source of truth for gateway port selection — matches reality and reduces the API surface. Three explanatory comments in service_manager.py / profiles.py document the retirement so future readers don't reach for the allocator and find a ghost. Tests: drop the three `_allocate_gateway_port` tests; update fakes' signatures throughout test_service_manager.py and test_profiles_s6_hooks.py to match the new no-port API.
Kailigithub
pushed a commit
to Kailigithub/hermes-agent
that referenced
this pull request
May 25, 2026
PR NousResearch#30136 review caught: three `s6-setuidgid hermes sh -c "..."` invocations in stage2-hook.sh interpolated $HERMES_HOME into a nested shell context. Practically low-risk (a malicious HERMES_HOME already requires container-launch privileges) but the cleaner pattern is to invoke commands directly so the shell isn't a second interpreter. * `mkdir -p` of the data subdirs now runs directly via s6-setuidgid, one path per arg. * The .install_method stamp is written via `printf | tee` — also no shell wrapper. * The skills_sync invocation uses the venv's python by absolute path instead of sourcing activate inside a shell. skills_sync.py doesn't need anything from activate beyond sys.path, which the bin-stub python already provides. No behavior change. Just a smaller attack surface and a script that's easier to read.
Kailigithub
pushed a commit
to Kailigithub/hermes-agent
that referenced
this pull request
May 25, 2026
PR NousResearch#30136 review noted: container-boot.log was append-only with no rotation. On a long-lived container with frequent restarts and many profiles it would grow unboundedly (~80 B per profile per reconcile pass). Add a soft cap: when the file size hits 256 KiB (`_LOG_ROTATE_BYTES`, ≈3000 reconcile lines, ≈1 year of daily reboots × 5 profiles), the current file is renamed to `container-boot.log.1` (replacing any existing one) before new entries are appended. Worst case is two files at ~512 KiB — well within visibility limits for grep/cat. Rotation is intentionally simple (no logrotate or s6-log machinery for one append-only file). Failures during rotation are logged via the module logger and treated as non-fatal — we keep appending to the existing file rather than dropping the reconcile entry. Three new unit tests cover above-threshold rotation, below-threshold non-rotation, and overwrite of an existing .1 file.
Kailigithub
pushed a commit
to Kailigithub/hermes-agent
that referenced
this pull request
May 25, 2026
PR NousResearch#30136 review noted the asymmetry: `register_profile_gateway` used tmp_dir + rename to publish a new service slot atomically, but the boot-time reconciler wrote files into the slot directly. Same underlying concern (a concurrent s6-svscan rescan could observe a half-populated directory), different code path. Rewrite `container_boot._register_service` to mirror the manager: build everything in `<scandir>/gateway-<profile>.tmp/`, then `Path.replace` into place. If a previous interrupted run left a `.tmp` sibling, it's cleaned up before the new build starts. If the target already exists, it's removed before the rename so `Path.replace` doesn't error on a non-empty target (Linux `rename` overwrites empty targets only). Three new tests: atomic publication leaves no .tmp leftovers, overwriting an existing slot still leaves no .tmp leftovers, and a stale .tmp from an interrupted run is cleaned up automatically.
Kailigithub
pushed a commit
to Kailigithub/hermes-agent
that referenced
this pull request
May 25, 2026
PR NousResearch#30136 review item O5: docker/entrypoint.sh is now a thin shim that forwards to stage2-hook.sh — the real ENTRYPOINT is /init plus main-wrapper.sh. External scripts that hard-coded entrypoint.sh as the container's ENTRYPOINT will see the cont-init bootstrap happen but the CMD will not be exec'd (because stage2-hook only handles bootstrap; main-wrapper.sh handles the CMD passthrough). Add a stderr warning explaining the new contract and pointing callers at the migration path (drop the --entrypoint override). The shim itself stays in place for one release cycle so the deprecation isn't a hard break — anyone still invoking it sees the warning in their logs and has time to migrate.
Kailigithub
pushed a commit
to Kailigithub/hermes-agent
that referenced
this pull request
May 25, 2026
PR NousResearch#30136 review item O6: test_container_restart.py used fixed `time.sleep(8)` calls after `docker restart` to wait for the cont-init reconciler to finish. Fixed sleeps are slow when the event happens fast and false-fail when the event happens slow. Replace with two polling helpers: * `_wait_for_path(container, path, kind='f' | 'd', deadline_s=...)` — generic `test -f/-d` poller. Returns True on success, False on timeout; callers assert with a clear message. * `_wait_for_reconcile_log_mention(container, profile, ...)` — the reconciler's per-profile log line is the canonical signal that the cont-init reconcile has finished for that profile. Poll on it instead of a sleep that hopes 8 seconds is enough. The fixture-level setup wait is similarly migrated: it now polls for `profile=default` in the boot log (every container always gets a default-slot entry per item I1) and raises a clear timeout error from the fixture if the container never finishes cont-init — much better diagnostics than a mid-test KeyError. The remaining `time.sleep()` calls are all internal interval_s between probe attempts; no fixed wait points left.
Kailigithub
pushed a commit
to Kailigithub/hermes-agent
that referenced
this pull request
May 25, 2026
PR NousResearch#30136 review item O7: the plan doc was 3,191 lines — 5x the size of any other plan in docs/plans/ and the largest reference document in the repo. With the implementation shipped, most of that content is either: * The phase-by-phase TDD walkthrough (~2,800 lines): now canonical in the PR commit log (`git log a957ef0..a6f7171`). * The v2/v3 re-validation preambles: artifacts of the planning process, no longer load-bearing. * The full Open Questions deliberations with options A/B/C laid out: collapsed into the Decision Log. * The Rollout Plan and Estimated Timeline: history. Trim to ~430 lines covering what readers actually need going forward: the goal, architecture, scope, key design decisions (D1–D9), risk register (now including the three risks surfaced in PR review — `_s6_running` detection, svscanctl FIFO perms, supervise control FIFO perms), the decision log including the post-merge additions, and the verification checklist (now all boxes ticked). Header now reads 'Status: shipped' and points at the PR. The git history preserves the full v3 plan for anyone who needs it.
Kailigithub
pushed a commit
to Kailigithub/hermes-agent
that referenced
this pull request
May 25, 2026
…e_manager Follow-up to @benbarclay's Docker s6 PR (NousResearch#30136). The Phase 4 hooks `_maybe_register_gateway_service` and `_maybe_unregister_gateway_service` were already documented as "no-op on host", but they reached that no-op by: 1. importing `hermes_cli.service_manager` 2. calling `get_service_manager()` (which calls `detect_service_manager()`) 3. checking `mgr.supports_runtime_registration()` and returning False If anything in step 1 or 2 raised an unexpected exception (e.g. a host machine with a partial s6 install — `/proc/1/comm == s6-svscan` somehow, but `/run/s6/basedir` absent, or vice versa), the `except Exception` in the hook would print a confusing "⚠ Could not register s6 gateway service: ..." warning on a non-container machine that has never touched the container. Reorder so `detect_service_manager() != "s6"` is checked FIRST, and return silently for any detection failure. Host machines now: - never import the s6 backend - never call get_service_manager() - never print an s6-shaped warning under any failure mode E2E confirmed on host Linux (systemd): `_maybe_register_gateway_service(...)` produces empty stdout, detect_service_manager() returns "systemd". Existing tests updated to patch `detect_service_manager` for the s6 call-through cases (they previously relied on get_service_manager being the only gate, which is no longer true). Added one new test — `test_register_silent_when_detect_throws` — asserting that a broken detector cannot leak a warning to host users. cc @benbarclay — visible behavior change vs. your branch is one fewer code path on host. Test changes are minimal (one helper + `_patch_detect_s6` opt-in per s6 test). Happy to revert if you prefer the original shape.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Type of Change
How to Test
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A