fix(docker): keep the venv on PATH in login shells - #72197
Closed
ugoenyioha wants to merge 1 commit into
Closed
Conversation
`ENV PATH` puts /opt/hermes/bin and the venv ahead of the system paths, and
ordinary processes inherit it. A LOGIN shell does not: it rebuilds PATH from
/etc/profile and drops all of it.
That path is exercised in normal operation. The terminal tool starts its
in-container sessions with a login shell —
tools/environments/docker.py::_run_bash() runs `docker exec … bash -l -c`
whenever login=True, which is how init_session snapshots the environment.
Measured in a container built from this image:
bash -c → python3 -> /opt/hermes/.venv/bin/python3
bash -lc → python3 -> /usr/bin/python3
so those sessions get the system interpreter, which has none of Hermes'
dependencies. The resulting failures read as missing packages rather than as a
PATH problem, which makes them slow to diagnose.
Drop a /etc/profile.d/hermes-venv.sh that re-exports the same prefix. The
ordering deliberately mirrors the ENV above with /opt/hermes/bin first: that is
the privilege-drop shim, and it must keep winning PATH resolution in login
shells too. Prepending only the venv would silently invert that precedence.
Split out of NousResearch#33179 as a focused docker-lane change, per review. The
S6_KEEP_ENV hunk from that PR is dropped -- it is a no-op now that NousResearch#32412
landed (verified separately on NousResearch#33148) -- and the Mattermost reply-mode feature
it also carried is NousResearch#33149.
Collaborator
19 tasks
Author
|
Closing as a duplicate — #64849 predates this and is strictly more complete: a versioned I should have run the duplicate search in CONTRIBUTING's "Before You Start" section before writing this. I've moved the useful part — a before/after No action needed here; #64849 is the one to land. |
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
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.
Summary
Split out of #33179 as the focused docker-lane change @benbarclay asked for. One
RUNline; the other two concerns from that PR are addressed separately (see the bottom).ENV PATHputs/opt/hermes/binand the venv ahead of the system paths, and ordinary processes inherit it. A login shell does not — it rebuildsPATHfrom/etc/profileand drops all of it.Answering the question from #33179
Terminal-tool sessions.
tools/environments/docker.py::_run_bash()— docstring "Spawn a bash process inside the Docker container" — does:init_sessionis the caller that passeslogin=True, so environment snapshotting runs underbash -linside the container. Your read ofmain-wrapper.shand the s6runscripts was right — none of those are login shells. This is a different entry point.Measured, not assumed
In a container built from this image, with no env overrides:
The login shell loses the venv entirely and
python3falls back to the system interpreter, which has none of Hermes' dependencies. The failures that follow read as missing packages rather than as aPATHproblem, which is what makes this slow to diagnose rather than merely broken.One deviation from #33179
That PR's line prepended only
/opt/hermes/.venv/bin:/opt/data/.local/bin, omitting/opt/hermes/bin. TheENV PATHcomment right above explains why that comes first — it is the privilege-drop shim, and it must win resolution. Omitting it would leave login shells with the opposite precedence to every other process. This version mirrors theENVordering exactly.Scope
ENV S6_KEEP_ENV=1from fix(docker): inject venv PATH into /etc/profile.d for login shells #33179 is dropped, as requested. It is a no-op now that fix(docker): propagate env through s6 to cont-init and main CMD #32412 has landed — I verified that separately and posted the measurement on fix(docker): set S6_KEEP_ENV=1 to preserve container environment #33148: on a current image the pod environment survives the s6 scrub without it.reply_mode = "auto"feature from fix(docker): inject venv PATH into /etc/profile.d for login shells #33179 is feat(mattermost): add 'auto' reply mode — flat DMs, threaded channels #33149, reworked against the current centralized resolver.