Skip to content

fix(docker): inject venv PATH into /etc/profile.d for login shells - #33179

Closed
ugoenyioha wants to merge 4 commits into
NousResearch:mainfrom
ugoenyioha:fix/venv-path-login-shell
Closed

fix(docker): inject venv PATH into /etc/profile.d for login shells#33179
ugoenyioha wants to merge 4 commits into
NousResearch:mainfrom
ugoenyioha:fix/venv-path-login-shell

Conversation

@ugoenyioha

Copy link
Copy Markdown

Summary

The terminal tool's init_session() spawns bash -l -c (login shell) to capture an environment snapshot. Login shells source /etc/profile which resets PATH to the system default, dropping the venv bin directory. All subsequent terminal commands use this snapshot, so python3 resolves to /usr/bin/python3 (system Python — no pip, no google-api-python-client, no installed packages) instead of /opt/hermes/.venv/bin/python3.

Root Cause

The Dockerfile sets ENV PATH="/opt/hermes/.venv/bin:..." which works for:

  • The hermes process itself (non-login shell)
  • docker exec commands
  • cont-init.d scripts

But bash -l resets PATH from /etc/profile, ignoring the Docker ENV. The terminal tool's BaseEnvironment.init_session() (line 384 in tools/environments/base.py) uses bash -l to build the snapshot, so the venv PATH is lost.

On v2026.5.16 (tini), entrypoint.sh ran source .venv/bin/activate in the same process tree that became PID 1, so the activated PATH was inherited by all children including login shells. With s6-overlay, /init is a C binary that doesn't carry shell state.

Fix

Write /etc/profile.d/hermes-venv.sh in the Dockerfile to prepend the venv to PATH for login shells. This is the standard mechanism for injecting PATH entries into login sessions on Debian.

Verification

Tested on Talos Kubernetes:

  • Before fix: bash -l -c 'which python3'/usr/bin/python3, terminal tool's python3 calls fail with ModuleNotFoundError
  • After fix: bash -l -c 'which python3'/opt/hermes/.venv/bin/python3, email fetch via google-api-python-client works

claude added 4 commits May 26, 2026 23:44
Introduces MATTERMOST_REPLY_MODE=auto alongside the existing 'thread'
and 'off' modes. When set to 'auto', the adapter checks the channel
type before deciding whether to thread:

  - DMs (channel_type 'D'): flat replies (no root_id) — avoids the
    'Invalid RootId parameter' 400 error that occurs when the adapter
    sends root_id in DM channels where no thread root exists.
  - Channels / groups ('O', 'P', 'G'): threaded replies (root_id set
    to the user's original post) — keeps conversations organized in
    busy channels without cluttering the main timeline.

Implementation:
  - New _should_thread(chat_id, reply_to) async method replaces all
    inline 'if reply_to and self._reply_mode == "thread"' checks.
  - _channel_type_cache dict avoids repeated /channels/{id} API calls
    for the same channel within a session.
  - get_chat_info() populates the cache on first lookup.
  - All four send paths (send, _send_url_as_file, _send_local_file,
    send_multiple_images) updated to use _should_thread.

Existing 'thread' and 'off' modes are unchanged.
s6-overlay v3 strips the container's environment before execing the
main program (CMD). This causes all Kubernetes-injected env vars
(MATTERMOST_TOKEN, MATTERMOST_URL, HERMES_HOME, HOME, API keys)
to vanish by the time the hermes gateway process starts. The gateway
then fails to detect any messaging platforms because os.getenv()
returns empty for every credential.

v2026.5.16 used tini as PID 1 (no env stripping), so this was never
an issue. The migration to s6-overlay on main introduced the
regression.

Fix: ENV S6_KEEP_ENV=1 in the Dockerfile, per the s6-overlay docs.
This preserves the full container environment for the main program
and all cont-init.d scripts.
The terminal tool's init_session() spawns `bash -l -c` (login shell)
to capture an environment snapshot. Login shells source /etc/profile
which resets PATH to the system default, dropping the venv bin dir.
Subsequent terminal commands use the snapshot, so `python3` resolves
to /usr/bin/python3 (system Python, no pip, no google-api-python-client)
instead of /opt/hermes/.venv/bin/python3.

On v2026.5.16 (tini), entrypoint.sh ran `source .venv/bin/activate`
in the same process tree, so the activated PATH survived into every
child — including login shells. With s6-overlay, /init is a C binary
that doesn't carry shell state, and main-wrapper.sh only activates the
venv for the hermes process itself.

Fix: write /etc/profile.d/hermes-venv.sh so login shells prepend the
venv to PATH. Works alongside the existing ENV PATH for non-login
shells and docker exec.
@alt-glitch alt-glitch added type/bug Something isn't working area/docker Docker image, Compose, packaging tool/terminal Terminal execution and process management P2 Medium — degraded but workaround exists labels May 27, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

I found one issue worth noting before merge.

PR scope mismatch: The PR body describes only the Docker venv PATH fix (/etc/profile.d/hermes-venv.sh + S6_KEEP_ENV=1), but the diff also includes significant changes to plugins/platforms/mattermost/adapter.py:

  1. New _should_thread() method with "auto" reply mode logic
  2. New _channel_type_cache for channel type lookups
  3. _send_url_as_file, _send_local_file, and send methods modified to use _should_thread instead of the previous self._reply_mode == "thread" check

These Mattermost adapter changes are not mentioned anywhere in the PR body, summary, or test plan. The Dockerfile changes are clean, but the bundled Mattermost feature should either be documented in the PR body or split into a separate PR.

Additionally, the Mattermost changes here overlap with PR #33181 (same author) which adds _resolve_effective_thread_root to the same adapter. Both PRs modify the send() method's threading logic with different approaches — #33179 adds _should_thread() while #33181 adds _resolve_effective_thread_root(). These will likely conflict on merge.

@benbarclay

Copy link
Copy Markdown
Collaborator

Thanks for this @ugoenyioha — there are some good ideas here but the PR bundles three independent concerns that need to be split before any can land. Could you break this into separate PRs:

1. ENV S6_KEEP_ENV=1 (Dockerfile) — please drop this hunk. The env-propagation issue it addresses was fixed differently on main via #32412 (commit 628aaea): docker/main-wrapper.sh now uses #!/command/with-contenv sh, which sources /run/s6/container_environment/ per-script instead of globally preserving env. Same outcome, tighter blast radius. #33148 was closed for the same reason yesterday.

2. /etc/profile.d/hermes-venv.sh (Dockerfile) — happy to land this as a focused docker-lane PR, but I'd like to confirm the use case first. Can you describe the path that hits this? The container's CMD goes through docker/main-wrapper.sh which sources /opt/hermes/.venv/bin/activate directly, and the s6 service run scripts do the same — none of those are login shells, so they don't reset PATH from /etc/profile. What's hitting bash -l inside the container? (Terminal-tool sessions? docker exec -it … bash -l? Something else?) If there's a real path that gets a stripped PATH, the fix is reasonable; I just want to make sure the comment in the PR ("terminal tool's init_session") matches what actually happens.

3. Mattermost reply_mode = "auto" (plugins/platforms/mattermost/adapter.py) — this is a real UX feature (thread in channels, flat in DMs, with channel-type caching) but it's completely orthogonal to docker. Please open it as a separate PR so the platforms maintainer can review it on its own merits.

Closing this one to make the split explicit — once (2) and (3) land as separate PRs, (1) is a no-op (already fixed). Sorry to fragment the work — appreciate the contribution and want each piece to land cleanly.

@ugoenyioha

Copy link
Copy Markdown
Author

Split done, @benbarclay — thanks for the clear breakdown.

One correction to that PR while I'm here: its profile.d line omitted /opt/hermes/bin, which would have left login shells resolving the venv ahead of the privilege-drop shim — the opposite precedence to every other process. #72197 mirrors the ENV PATH ordering instead.

No need to reopen this one; happy for it to stay closed now the pieces are separate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docker Docker image, Compose, packaging P2 Medium — degraded but workaround exists tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants