fix(integrations): stop bm subprocesses inheriting Hermes's Python env - #1179
Conversation
|
|
|
thanks for the PR @Palo-Alto-AI-Research-Lab |
|
thanks — and one thing cleared on our side since you wrote: the CLA is signed, so whenever you get to the review, the two scope calls in the description are one-word answers, and i'll push either the same day:
no urgency on either; the fix stands as it is. the diagnosis in #1093 was @tellus1019's and already complete — this is that, implemented, with a repro that runs in CI. |
phernandez
left a comment
phernandez
left a comment
There was a problem hiding this comment.
Reviewed at 33c2011e; no findings.
I also applied the PR cleanly to current main (eedce9bb) and verified:
just package-check-hermes: 267 passed, 12 skipped- focused child-environment regression tests: 10 passed
- Ruff check/format, Hermes manifest validation, and
git diff --check: clean
For the two scope questions:
- Leave
LD_LIBRARY_PATH/DYLD_LIBRARY_PATHalone. They can carry legitimate native-library configuration and are not implicated in this failure. - Keep
__PYVENV_LAUNCHER__in the strip list. It is parent-interpreter launcher state and should not steer the independentbmchild.
The boundary here looks right: isolate the child from Hermes's Python installation while preserving unrelated environment such as PATH, HOME, proxies, and credentials.
Thanks for the careful diagnosis, complete spawn-site sweep, and strong regression coverage.
Hermes runs on its own interpreter and exports PYTHONPATH; bm is a separate
uv-managed tool on Python 3.12+. Every process the plugin spawned inherited
those variables, so the child resolved imports against Hermes's site-packages
and died during interpreter startup:
ModuleNotFoundError: No module named 'pydantic_core._pydantic_core'
That is a compiled-extension mismatch between two Python versions, not a
missing dependency, so it does not respond to reinstalling anything.
Adds _clean_child_env() and applies it at all three spawn sites:
- _BmMcpActor.__init__ -> StdioServerParameters(env=...) for 'bm mcp'
- _install_bm_via_uv -> 'uv tool install basic-memory'
- _ensure_local_project -> 'bm project add'
The last two passed no env= at all, so they inherited implicitly and are easy
to miss when patching only the actor.
PATH, HOME, proxies and credentials are preserved; only the four variables
that name a specific interpreter are dropped.
Tests spawn real subprocesses with a deliberately contaminated PYTHONPATH, as
requested in the issue: a mock cannot reproduce this because the failure
happens in interpreter startup rather than in our code. One test asserts the
contamination reaches an inherited child, so a no-op sanitizer cannot pass.
Fixes basicmachines-co#1093
Signed-off-by: Palo-Alto-AI-Research-Lab <Palo-Alto-AI-Research-Lab@users.noreply.github.com>
33c2011 to
1b80ee3
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
3 participants
Fixes #1093.
What
The Hermes plugin spawns three external processes, and every one of them inherited the parent's Python environment. Hermes ships its own interpreter and exports
PYTHONPATH;bmis a separate uv-managed tool on 3.12+. The child therefore resolved imports against Hermes's site-packages and failed during interpreter startup withModuleNotFoundError: No module named 'pydantic_core._pydantic_core'— a compiled-extension mismatch between two Python versions, not a missing dependency, which is why reinstalling doesn't help._clean_child_env()drops the four variables that name a specific interpreter (PYTHONPATH,PYTHONHOME,VIRTUAL_ENV,__PYVENV_LAUNCHER__) and keeps everything else —PATH,HOME, proxies, credentials — because the child still needs them.Applied at all three sites:
_BmMcpActor.__init__→StdioServerParameters(env=…)bm mcpos.environ.copy()_install_bm_via_uvuv tool install basic-memoryenv=— inherited implicitly_ensure_local_projectbm project addenv=— inherited implicitlyThe bottom two are the easy ones to miss: with no
env=argument there is nothing to grep for, anduv tool installis what builds the environment the MCP server later runs in.Testing
tests/test_child_env.py, 10 tests. The four covering the spawn sites were confirmed to fail before the fix and pass after — a sanitizer that did nothing would not go green.The issue asked for an integration test with a contaminated
PYTHONPATHand noted that unit mocks can't reproduce a native-extension ABI failure. That's right, but the failure doesn't need a second interpreter to reproduce deterministically — a directory onPYTHONPATHshadowing a real module does it on one Python:A child spawned with the inherited environment imports the shadow (
ImportError); a child spawned through_clean_child_env()cannot see it at all (ModuleNotFoundError). Both directions are asserted, so the test proves the contamination is real before it proves it is gone. No ABI mismatch to stage in CI and no second interpreter needed.Local runs:
267 passedon Python 3.11, 3.12 and 3.14;just manifest-checkclean;ruff checkandruff format --checkclean.Two scope decisions, in case you'd rather they went the other way
LD_LIBRARY_PATH/DYLD_LIBRARY_PATHare deliberately not stripped. They can break a child the same way, but they also carry legitimate local configuration and were not implicated here. Say the word and I'll add them.__PYVENV_LAUNCHER__is included defensively. The macOS framework launcher exports it to steer a child at a specific interpreter, but I did not reproduce a failure caused by it, unlike the other three. Happy to drop it if you'd rather keep the list to what's proven.I also swept
integrations/openclawfor the same pattern — it has no subprocess spawns, so nothing to fix there.Credit to @tellus1019: the diagnosis in the issue was already correct and complete, including all three scopes. This is that diagnosis implemented, plus a repro that runs in CI.
Disclosure: written by an AI agent acting on behalf of the account holder, as part of Palo Alto AI Research Lab. Everything reported above was run, not assumed; the one untested item is flagged as such.