fix(docker): preserve custom PATH entries in subprocess environment (#70905) - #70923
fix(docker): preserve custom PATH entries in subprocess environment (#70905)#70923webtecnica wants to merge 3 commits into
Conversation
|
The current branch includes two unrelated commits before the PATH fix: |
…D exhaustion (NousResearch#69567) The cron execution ledger was leaking SQLite connections because `sqlite3.Connection` as a context manager only commits/rolls back -- it does NOT close the connection. Without explicit `conn.close()`, every ledger call (create, mark_running, finish, recover, list, latest) left an open connection and its WAL/SHM file descriptors. Created a `_transaction()` context manager that wraps lock acquisition, connection open, schema initialization, transaction commit/rollback, and deterministic connection close in a `finally` block. Schema init runs inside the `try` too so PRAGMA/DDL failures after a successful connect still close the connection. Key design: - `_connect()` now only opens the connection (no schema) - `_initialize_schema(conn)` handles PRAGMAs, DDL, and WAL setup - `_transaction()` combines lock + connect + init + commit/rollback + close - `apply_wal_with_fallback` is preserved (unlike PR NousResearch#69594 which replaced it with raw PRAGMA, losing NFS/SMB fallback) All 7 call sites migrated from `with _lock, _connect() as conn:` to `with _transaction() as conn:`. Added regression test `test_every_ledger_call_closes_sqlite_connection` that repeatedly calls all ledger functions and asserts the /proc/self/fd count for executions.db doesn't grow.
…ousResearch#70905) Ensure that custom PATH entries set via Dockerfile ENV, .env files, or config.yaml terminal.env / docker_env are preserved in the agent's subprocess execution context. Root cause ---------- Two issues combined to silently drop custom PATH entries: 1. _make_run_env (local.py): the filtering loop that strips Hermes provider credentials could, in edge cases with a future-expanded blocklist, leave the run_env dict without a PATH key. Even when PATH was present, there was no fallback chain if it was empty. 2. DockerEnvironment._build_init_env_args (docker.py): the list of -e env-var flags passed to docker exec during init_session only included docker_env overrides and forwarded/passthrough vars. PATH from the container's default environment (Dockerfile ENV) was NOT explicitly forwarded. If the login shell's profile scripts (/etc/profile, ~/.bashrc) truncated or reset PATH, the init_session snapshot would capture a narrower PATH than the container's default, silently losing custom entries on every subsequent execute() call. Fix --- 1. _make_run_env: Add a PATH propagation guard that restores PATH from os.environ if the filtering loop drops it, and falls back to os.environ when the run_env PATH is empty. 2. _build_init_env_args: Explicitly include PATH from the host os.environ when it's not already overridden by docker_env, so the init_session snapshot always captures the full PATH. 3. Add comprehensive test suite (tests/tools/test_path_propagation.py): 12 tests covering custom PATH preservation, _SANE_PATH fallback, terminal.env override, multiple custom entries, empty PATH recovery, and full execute() integration.
6338165 to
227c5c7
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for investigating the PATH-loss report. The Docker snapshot path is the right area to examine, but the current implementation does not preserve the container-defined value.
Problems
- In this diff,
tools/environments/docker.py:1518readsos.environ["PATH"]from the Docker host. Main injects those arguments intodocker execattools/environments/docker.py:1511-1523, while the container image defines its own runtime PATH atDockerfile:415. The host PATH cannot carry a container-only Dockerfile entry such as/potato. tests/tools/test_path_propagation.py:15-22exercises onlyLocalEnvironment; it has no Docker backend or derived-image reproduction. This does not validate the reported snapshot failure.- As noted in the existing PR discussion, the reviewed diff still contains unrelated cron changes in
cron/executions.pyandcron/scheduler.py.
Suggested changes
- Preserve the target container's pre-login PATH, then apply an explicit
docker_envPATH override with defined precedence rather than forwarding the host PATH. - Add a Docker integration regression with a derived image whose
ENV PATHincludes/potatoand verify its binary through the session snapshot path.
This is an automated hermes-sweeper review.
| # Prefer the docker_env override when present; otherwise fall back to | ||
| # the host os.environ, which inside a Docker container carries the | ||
| # Dockerfile ENV PATH. | ||
| if "PATH" not in exec_env: |
There was a problem hiding this comment.
os.environ here belongs to the Docker host, not the target container. The reported /potato entry is introduced by the target image's Dockerfile, so forwarding this value cannot preserve that container-only PATH and may replace it during the login-shell snapshot. Capture the container's pre-login PATH instead, then apply any explicit docker_env override.
Graph note (no action implied — a maintainer has already reviewed this thread). Our triage graph places this PR in a complex with 1 related issue ( Full neighbourhood: https://hermes-triage.gottz.de/?node=70923 This note exists so the relationship stays discoverable from the thread itself. |
Fix: Preserve custom PATH entries in agent's subprocess execution context
Closes #70905
Problem
Custom PATH entries set via Dockerfile
ENV,.envfiles, orconfig.yaml(terminal.env/docker_env) are silently dropped, causingcommand not found(exit 127) when the agent runs installed binaries.Root cause
Two issues combined:
_make_run_env(local.py): The filtering loop that strips Hermes provider credentials from the subprocess env had no fallback chain for PATH. If a future expansion of the blocklist accidentally filtered PATH, or the run_env PATH was empty, the custom entries would be lost.DockerEnvironment._build_init_env_args(docker.py): The-eenv-var flags passed todocker execduringinit_sessiononly includeddocker_envoverrides and forwarded/passthrough vars. PATH from the container's default environment (DockerfileENV) was NOT explicitly forwarded. When the login shell's profile scripts (/etc/profile,~/.bashrc) truncated or reset PATH, theinit_sessionsnapshot would capture a narrower PATH than the container's default, silently losing custom entries on every subsequentexecute()call.Fix
_make_run_env: Add a PATH propagation guard that restores PATH fromos.environif the filtering loop drops it, and falls back toos.environwhen the run_env PATH is empty._build_init_env_args: Explicitly include PATH from the hostos.environwhen it's not already overridden bydocker_env, so theinit_sessionsnapshot always captures the full PATH.New test suite (
tests/tools/test_path_propagation.py): 12 tests covering custom PATH preservation,_SANE_PATHfallback,terminal.envoverride, multiple custom entries, empty PATH recovery, and fullexecute()integration.Testing
pytest tests/tools/test_path_propagation.py— 12/12 passedpytest tests/tools/test_local_env_blocklist.py— all existing tests passpytest tests/tools/test_env_passthrough.py— all existing tests passpytest tests/tools/test_hermes_subprocess_env.py— all existing tests pass