fix(hermes): follow wrapper-script exec target when locating Hermes Python - #620
Merged
AxDSan merged 5 commits intoAug 9, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe installer now resolves Hermes symlinks and supported shell wrappers before selecting Python. It validates targets, bounds resolution depth, detects loops, and uses the resolved executable directory for Python discovery. Tests cover valid and invalid launcher layouts. ChangesHermes executable resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant HermesLauncher
participant HermesExecutable
participant HermesPython
Installer->>HermesLauncher: Resolve symlink or wrapper exec target
HermesLauncher->>HermesExecutable: Validate target
HermesExecutable-->>Installer: Return resolved executable
Installer->>HermesPython: Search beside resolved executable
HermesPython-->>Installer: Return virtual-environment interpreter
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integrations/hermes/src/mnemosyne_hermes/install.py`:
- Around line 511-512: Update the symlink branch in the launcher-resolution
helper to catch OSError and RuntimeError from path.resolve(), and return None
when resolution fails or the target is not an executable file. Ensure broken or
looping symlinks cannot proceed to _find_hermes_python() or select a stale
sibling Python, while preserving fail-soft automatic installation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1dd5aea4-b466-45a5-a514-2034987179c1
📒 Files selected for processing (2)
integrations/hermes/src/mnemosyne_hermes/install.pyintegrations/hermes/tests/test_install_hermes.py
Collaborator
|
@Awshesh12 Please implement the Coderabbit Finding - Handle invalid symlink targets as unresolved launchers. |
Awshesh12
pushed a commit
to Awshesh12/mnemosyne
that referenced
this pull request
Aug 4, 2026
Address review feedback on mnemosyne-oss#620: - Catch OSError/RuntimeError from Path.resolve() in the symlink branch. - Return None when the resolved target is missing or non-executable. - Apply the same executable and resolve-exception checks to wrapper-script exec targets. - Add regression tests for broken symlinks, non-executable targets, and symlink loops.
Contributor
Author
|
Implemented the CodeRabbit finding in 8e46c5c:
The relevant pytest cases pass. The remaining failures in the full suite are the same environment-level venv/ensurepip issues present before this change. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
integrations/hermes/src/mnemosyne_hermes/install.py (2)
558-570: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve direct package-entrypoint discovery.
If
shutil.which("hermes")returns a regular executable that is not anexecwrapper,_resolve_hermes_bin()returnsNone._find_hermes_python()then skips that executable's sibling Python and can silently omit Hermes dependency bootstrap. Return a validated direct executable when no wrapper target is present.As per path instructions, “The Hermes integration is installed through pipx/package entry points; no symlink or directory-copy setup should be required.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integrations/hermes/src/mnemosyne_hermes/install.py` around lines 558 - 570, The _find_hermes_python() flow must fall back to the resolved direct Hermes executable when _resolve_hermes_bin() returns None for a regular package entry point. Validate that executable and return it, while preserving the existing sibling-python discovery for wrapper or symlink targets and avoiding any symlink or directory-copy setup.Source: Path instructions
558-570: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRequire an executable Python candidate.
candidate.is_file()accepts regular files without execute permission._find_hermes_python()can return that path, and installers later pass it to subprocess invocation. Accept the candidate only when it is a file/runnable, such as by requiringos.access(candidate, os.X_OK), and continue trying the next path otherwise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integrations/hermes/src/mnemosyne_hermes/install.py` around lines 558 - 570, Update the Python candidate selection in _find_hermes_python around the resolved launcher path to require both a regular file and executable permission before returning it, using os.access with os.X_OK or the existing equivalent. If a candidate is not runnable, continue checking the next name instead of returning it.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integrations/hermes/src/mnemosyne_hermes/install.py`:
- Around line 514-518: Update the resolver function containing the
resolved.is_file() check and its OSError/RuntimeError handler to emit
debug-level logs before each return None for resolution exceptions and invalid
or non-executable targets. Include the launcher and a specific failure reason in
each log, while preserving the existing fail-soft None returns.
- Around line 511-518: Update the PATH-entry resolution used by
_find_hermes_python() so a symlink pointing to an executable shell wrapper is
parsed for its exec target instead of being returned immediately. Continue
following symlink and launcher layers until the underlying executable is found,
while tracking visited paths or enforcing a maximum resolution depth to prevent
loops.
- Around line 533-538: Update the executable target resolution around the
match-group handling so explicit relative paths are joined to the wrapper’s
path.parent before expansion and validation, while bare command names continue
through PATH lookup. Preserve the existing executable check and resolve error
handling in the surrounding target-resolution logic.
- Around line 533-538: Move the Path construction and expanduser call in the
wrapper shim discovery flow into the existing try block so unresolved home
configuration errors are caught. Ensure failures from expanduser, symlink
checks, or target.resolve() return None instead of propagating, while preserving
successful executable-target resolution.
In `@integrations/hermes/tests/test_install_hermes.py`:
- Around line 74-75: Isolate the fallback behavior tested around
_find_hermes_python by patching sys.prefix, VIRTUAL_ENV, and the known
Hermes-root interpreter sources so an invalid _resolve_hermes_bin result cannot
discover another Python executable. Keep the assertions verifying that
_resolve_hermes_bin and _find_hermes_python return None.
---
Outside diff comments:
In `@integrations/hermes/src/mnemosyne_hermes/install.py`:
- Around line 558-570: The _find_hermes_python() flow must fall back to the
resolved direct Hermes executable when _resolve_hermes_bin() returns None for a
regular package entry point. Validate that executable and return it, while
preserving the existing sibling-python discovery for wrapper or symlink targets
and avoiding any symlink or directory-copy setup.
- Around line 558-570: Update the Python candidate selection in
_find_hermes_python around the resolved launcher path to require both a regular
file and executable permission before returning it, using os.access with os.X_OK
or the existing equivalent. If a candidate is not runnable, continue checking
the next name instead of returning it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b258e509-b572-488b-82d3-170936f6f7fc
📒 Files selected for processing (2)
integrations/hermes/src/mnemosyne_hermes/install.pyintegrations/hermes/tests/test_install_hermes.py
Awshesh12
pushed a commit
to Awshesh12/mnemosyne
that referenced
this pull request
Aug 4, 2026
…r handling Address follow-up review feedback on mnemosyne-oss#620: - Refactor _resolve_hermes_bin() into a loop that follows symlink + wrapper layers with a visited-path set and a max depth guard against loops. - Return a validated direct executable when the launcher is a plain package entry point (not symlink or wrapper), preserving pipx/pip discovery. - Resolve relative exec targets against the wrapper directory and bare exec targets via PATH lookup. - Catch OSError/RuntimeError from resolve/expanduser and return None on broken/looping symlinks, non-executable targets, or invalid wrapper targets. - Add debug logging with the launcher path and failure reason. - Require os.access(..., X_OK) before accepting sibling python candidates. - Cap wrapper-script read size at 4 KB to avoid reading large binaries. - Add regression tests for direct executables, symlink-to-wrapper chains, relative/bare exec targets, symlink loops, wrapper loops, broken links, and non-executable targets; isolate fallback interpreter sources.
Contributor
Author
|
Pushed 679c481 to address the latest CodeRabbit findings:
The targeted pytest cases pass. The remaining failures in the full file are the same local venv/ensurepip environment issues unrelated to this change. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integrations/hermes/src/mnemosyne_hermes/install.py`:
- Around line 543-551: The target resolution logic must distinguish explicit
paths from bare commands: in install.py lines 543-551, use wrapper.parent only
when raw_target includes a directory component, return None when that explicit
path is missing, and call shutil.which() only for bare command names while
preserving provider discovery and pipx support. In test_install_hermes.py lines
155-177, add an executable shim/hermes decoy beside hermes-launcher and retain
the assertion that resolution selects real_bin/hermes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69c3bb97-fe5e-49ee-9dcc-943decf8e1e9
📒 Files selected for processing (2)
integrations/hermes/src/mnemosyne_hermes/install.pyintegrations/hermes/tests/test_install_hermes.py
Awshesh12
pushed a commit
to Awshesh12/mnemosyne
that referenced
this pull request
Aug 4, 2026
Address the latest CodeRabbit finding on mnemosyne-oss#620: - In _resolve_exec_target(), treat absolute paths and explicit relative paths (containing a directory component) as file paths: join relative ones to the wrapper directory and return None immediately if the path does not exist. - Use shutil.which() only for bare command names, preventing a missing relative path from accidentally resolving to an unrelated binary on PATH. - Strengthen the bare-command regression test with an executable shim/hermes decoy next to the wrapper, asserting that resolution still picks the real_bin/hermes from PATH rather than the adjacent decoy.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integrations/hermes/src/mnemosyne_hermes/install.py`:
- Around line 543-550: Update the target-resolution logic to classify explicit
relative paths using the unnormalized raw target string, preserving ./ and ../
components before Path normalization. Ensure ./hermes-real resolves relative to
wrapper.parent rather than through shutil.which, and add a regression covering a
current-directory decoy executable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: baa05e90-e51f-4075-9569-fbbbb19246a8
📒 Files selected for processing (2)
integrations/hermes/src/mnemosyne_hermes/install.pyintegrations/hermes/tests/test_install_hermes.py
This was referenced Aug 4, 2026
dplush
requested changes
Aug 8, 2026
dplush
left a comment
dplush
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the broader launcher coverage. I re-ran the resolver against adversarial wrapper forms and found two blocking cases where it can select the wrong Python interpreter.
-
./targets lose their explicit-path meaning afterPathnormalization. Forexec ./hermes-real "$@", the code later treatshermes-realas a bare command and runswhichfrom the process CWD. A CWD decoy is then selected instead ofwrapper.parent / "hermes-real". Please preserve the raw target when deciding whether it is an explicit path, and add a regression with a CWD decoy. -
Valid shell forms with environment setup are unsafe today:
exec env FOO=bar /real/hermes "$@"
FOO=bar exec /real/hermes "$@"
The first resolves /usr/bin/env as the launcher; the second is not recognized and can fall back to the wrapper itself. Both can make _find_hermes_python() select a system or decoy adjacent Python. Please either support a deliberately bounded, tested tokenizer for these forms or reject them as unresolved. Do not fall back to a direct launcher in these cases. Add adversarial tests with decoy Python siblings.
The symlink-loop and executable-target hardening looks good. After these fixes, please rebase onto current main, rerun CI, and request a fresh review.
Awshesh12
force-pushed
the
fix/hermes-install-wrapper-script-python-618
branch
from
August 9, 2026 06:51
d07a549 to
4747b66
Compare
Awshesh12
pushed a commit
to Awshesh12/mnemosyne
that referenced
this pull request
Aug 9, 2026
Address review feedback on mnemosyne-oss#620: - Catch OSError/RuntimeError from Path.resolve() in the symlink branch. - Return None when the resolved target is missing or non-executable. - Apply the same executable and resolve-exception checks to wrapper-script exec targets. - Add regression tests for broken symlinks, non-executable targets, and symlink loops.
Awshesh12
pushed a commit
to Awshesh12/mnemosyne
that referenced
this pull request
Aug 9, 2026
…r handling Address follow-up review feedback on mnemosyne-oss#620: - Refactor _resolve_hermes_bin() into a loop that follows symlink + wrapper layers with a visited-path set and a max depth guard against loops. - Return a validated direct executable when the launcher is a plain package entry point (not symlink or wrapper), preserving pipx/pip discovery. - Resolve relative exec targets against the wrapper directory and bare exec targets via PATH lookup. - Catch OSError/RuntimeError from resolve/expanduser and return None on broken/looping symlinks, non-executable targets, or invalid wrapper targets. - Add debug logging with the launcher path and failure reason. - Require os.access(..., X_OK) before accepting sibling python candidates. - Cap wrapper-script read size at 4 KB to avoid reading large binaries. - Add regression tests for direct executables, symlink-to-wrapper chains, relative/bare exec targets, symlink loops, wrapper loops, broken links, and non-executable targets; isolate fallback interpreter sources.
Awshesh12
pushed a commit
to Awshesh12/mnemosyne
that referenced
this pull request
Aug 9, 2026
Address the latest CodeRabbit finding on mnemosyne-oss#620: - In _resolve_exec_target(), treat absolute paths and explicit relative paths (containing a directory component) as file paths: join relative ones to the wrapper directory and return None immediately if the path does not exist. - Use shutil.which() only for bare command names, preventing a missing relative path from accidentally resolving to an unrelated binary on PATH. - Strengthen the bare-command regression test with an executable shim/hermes decoy next to the wrapper, asserting that resolution still picks the real_bin/hermes from PATH rather than the adjacent decoy.
Awshesh12
force-pushed
the
fix/hermes-install-wrapper-script-python-618
branch
from
August 9, 2026 06:54
4747b66 to
d07a549
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integrations/hermes/src/mnemosyne_hermes/install.py (1)
1777-1779: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the mismatch message and the empty
The condition compares interpreter paths, not versions, so the text "Python version MISMATCH" can be wrong when both interpreters share a version. Line 1779 also prints an empty line when
_vercontains no space, and otherwise prints a bare version number instead of a command. State the path difference and remove the non-actionable line.🐛 Proposed fix
if state.mode == "symlink" and hermes_python.resolve() != Path(sys.executable).resolve(): - print(" ⚠ Python version MISMATCH! Install and Hermes use different Python versions.") - print(f" → Run: {_ver.split()[1]}" if " " in _ver else "") + print(" ⚠ Interpreter MISMATCH! The installer and Hermes use different Python interpreters.") + print(f" → Run: uv pip install --python {hermes_python} -U 'mnemosyne-hermes[all]'")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integrations/hermes/src/mnemosyne_hermes/install.py` around lines 1777 - 1779, Update the symlink mismatch handling around state.mode and hermes_python.resolve() to describe an interpreter path mismatch rather than a Python version mismatch, and remove the conditional print that emits either a bare version or an empty line. Retain only actionable mismatch guidance, using the compared interpreter paths where appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integrations/hermes/src/mnemosyne_hermes/install.py`:
- Around line 592-603: The exec option scanner in _resolve_hermes_bin must treat
“--” as the end of options: advance past it by one token and stop scanning,
rather than consuming the following executable. In
integrations/hermes/src/mnemosyne_hermes/install.py lines 592-603, update the
scanner accordingly; in integrations/hermes/tests/test_install_hermes.py lines
234-248, add coverage for a wrapper using exec -- "<real_hermes>" "$@" and
assert that _resolve_hermes_bin returns the real executable.
- Around line 539-544: Update _skip_env_command to classify -C and --chdir as
unsupported argument-taking env options alongside -u, --unset, -S, and
--split-string, returning None instead of advancing to treat their directory
argument as the wrapped command.
In `@integrations/hermes/tests/test_install_hermes.py`:
- Around line 42-72: Update
test_find_hermes_python_follows_wrapper_script_to_real_venv to call
_isolate_hermes_python_sources(tmp_path, monkeypatch) and _skip_on_windows()
before exercising _find_hermes_python(), matching the shared setup used by the
other resolution tests.
- Around line 26-31: Update _isolate_hermes_python_sources to isolate every
fallback source used by _find_hermes_python(), including redirecting Path.home()
and patching or otherwise skipping the absolute Hermes roots (/opt, /usr/local,
and /usr/lib). Preserve separate coverage for those absolute roots so normal
tests cannot discover host installations while root-probing behavior remains
tested.
---
Outside diff comments:
In `@integrations/hermes/src/mnemosyne_hermes/install.py`:
- Around line 1777-1779: Update the symlink mismatch handling around state.mode
and hermes_python.resolve() to describe an interpreter path mismatch rather than
a Python version mismatch, and remove the conditional print that emits either a
bare version or an empty line. Retain only actionable mismatch guidance, using
the compared interpreter paths where appropriate.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9a0d06a2-d98a-471d-8ac8-6caee39487fb
📒 Files selected for processing (2)
integrations/hermes/src/mnemosyne_hermes/install.pyintegrations/hermes/tests/test_install_hermes.py
Comment on lines
+592
to
+603
| # Skip exec options (e.g. -a name, -l). Only skip simple no-arg options; | ||
| # -a takes an argument and -- is a stopper. | ||
| while idx < len(tokens) and tokens[idx].startswith("-"): | ||
| if tokens[idx] in ("-a", "--"): | ||
| idx += 2 | ||
| else: | ||
| idx += 1 | ||
| if idx > len(tokens): | ||
| return UNSUPPORTED_WRAPPER_TARGET | ||
|
|
||
| if idx >= len(tokens): | ||
| return UNSUPPORTED_WRAPPER_TARGET |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
exec -- is parsed as an option that takes an argument. The exec option scanner groups -- with -a and advances the index by two, so the token after -- is skipped and a valid wrapper is rejected. No regression test covers this form.
integrations/hermes/src/mnemosyne_hermes/install.py#L592-L603: treat--as end-of-options; advance the index by one and stop the option scan.integrations/hermes/tests/test_install_hermes.py#L234-L248: add a test with a wrapper containingexec -- "<real_hermes>" "$@"and assert that_resolve_hermes_binreturns the real executable.
📍 Affects 2 files
integrations/hermes/src/mnemosyne_hermes/install.py#L592-L603(this comment)integrations/hermes/tests/test_install_hermes.py#L234-L248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integrations/hermes/src/mnemosyne_hermes/install.py` around lines 592 - 603,
The exec option scanner in _resolve_hermes_bin must treat “--” as the end of
options: advance past it by one token and stop scanning, rather than consuming
the following executable. In integrations/hermes/src/mnemosyne_hermes/install.py
lines 592-603, update the scanner accordingly; in
integrations/hermes/tests/test_install_hermes.py lines 234-248, add coverage for
a wrapper using exec -- "<real_hermes>" "$@" and assert that _resolve_hermes_bin
returns the real executable.
Comment on lines
+26
to
+31
| def _isolate_hermes_python_sources(tmp_path, monkeypatch): | ||
| """Patch fallback interpreter sources so tests only see the mocked hermes launcher.""" | ||
| monkeypatch.setattr(install_mod, "hermes_home", lambda: tmp_path / "no_hermes_home") | ||
| monkeypatch.setattr(install_mod.sys, "prefix", install_mod.sys.base_prefix) | ||
| monkeypatch.delenv("VIRTUAL_ENV", raising=False) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Isolate the remaining fallback interpreter sources.
The helper patches hermes_home, sys.prefix, and VIRTUAL_ENV. Step 2 of _find_hermes_python() also probes Path.home() / "hermes-agent", /opt/hermes/hermes-agent, /usr/local/lib/hermes-agent, and /usr/lib/hermes-agent. On a developer or CI machine that has a real Hermes install, the tests that assert _find_hermes_python() is None (Lines 199, 218, 379) fail. Redirect Path.home() and skip or patch the absolute roots.
♻️ Proposed change
def _isolate_hermes_python_sources(tmp_path, monkeypatch):
"""Patch fallback interpreter sources so tests only see the mocked hermes launcher."""
monkeypatch.setattr(install_mod, "hermes_home", lambda: tmp_path / "no_hermes_home")
+ monkeypatch.setattr(install_mod.Path, "home", classmethod(lambda cls: tmp_path / "no_home"))
monkeypatch.setattr(install_mod.sys, "prefix", install_mod.sys.base_prefix)
monkeypatch.delenv("VIRTUAL_ENV", raising=False)The absolute roots still need coverage. Do you want me to add a fixture that patches the root list in _find_hermes_python()?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integrations/hermes/tests/test_install_hermes.py` around lines 26 - 31,
Update _isolate_hermes_python_sources to isolate every fallback source used by
_find_hermes_python(), including redirecting Path.home() and patching or
otherwise skipping the absolute Hermes roots (/opt, /usr/local, and /usr/lib).
Preserve separate coverage for those absolute roots so normal tests cannot
discover host installations while root-probing behavior remains tested.
Comment on lines
+42
to
+72
| def test_find_hermes_python_follows_wrapper_script_to_real_venv(tmp_path, monkeypatch): | ||
| """A PATH shim that execs the real Hermes binary must not pick a stray python beside the shim.""" | ||
| # Real Hermes venv layout. | ||
| real_venv = tmp_path / "hermes-agent" / "venv" | ||
| real_bin = real_venv / "bin" | ||
| real_bin.mkdir(parents=True) | ||
| real_hermes = real_bin / "hermes" | ||
| real_hermes.write_text("#!/bin/sh\n", encoding="utf-8") | ||
| real_hermes.chmod(0o755) | ||
| real_python = real_bin / "python" | ||
| real_python.write_text("#!/bin/sh\n", encoding="utf-8") | ||
| real_python.chmod(0o755) | ||
|
|
||
| # PATH shim in ~/.local/bin that execs the real Hermes binary. | ||
| shim_dir = tmp_path / ".local" / "bin" | ||
| shim_dir.mkdir(parents=True) | ||
| shim = shim_dir / "hermes" | ||
| shim.write_text( | ||
| f'#!/usr/bin/env bash\nexec "{real_hermes}" "$@"\n', | ||
| encoding="utf-8", | ||
| ) | ||
| shim.chmod(0o755) | ||
|
|
||
| # A decoy python next to the shim: this is the bug case. | ||
| decoy_python = shim_dir / "python" | ||
| decoy_python.write_text("#!/bin/sh\n", encoding="utf-8") | ||
| decoy_python.chmod(0o755) | ||
|
|
||
| monkeypatch.setattr(install_mod.shutil, "which", lambda _bin: str(shim)) | ||
| found = install_mod._find_hermes_python() | ||
| assert found == real_python |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Apply the shared test setup to this test.
This test calls _find_hermes_python() but does not call _isolate_hermes_python_sources(tmp_path, monkeypatch) or _skip_on_windows(). Every other resolution test calls both. The test relies on chmod(0o755) and os.access(..., os.X_OK), which do not behave the same on Windows. Add both calls for consistency.
♻️ Proposed change
def test_find_hermes_python_follows_wrapper_script_to_real_venv(tmp_path, monkeypatch):
"""A PATH shim that execs the real Hermes binary must not pick a stray python beside the shim."""
+ _skip_on_windows()
+
# Real Hermes venv layout.
@@
+ _isolate_hermes_python_sources(tmp_path, monkeypatch)
monkeypatch.setattr(install_mod.shutil, "which", lambda _bin: str(shim))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_find_hermes_python_follows_wrapper_script_to_real_venv(tmp_path, monkeypatch): | |
| """A PATH shim that execs the real Hermes binary must not pick a stray python beside the shim.""" | |
| # Real Hermes venv layout. | |
| real_venv = tmp_path / "hermes-agent" / "venv" | |
| real_bin = real_venv / "bin" | |
| real_bin.mkdir(parents=True) | |
| real_hermes = real_bin / "hermes" | |
| real_hermes.write_text("#!/bin/sh\n", encoding="utf-8") | |
| real_hermes.chmod(0o755) | |
| real_python = real_bin / "python" | |
| real_python.write_text("#!/bin/sh\n", encoding="utf-8") | |
| real_python.chmod(0o755) | |
| # PATH shim in ~/.local/bin that execs the real Hermes binary. | |
| shim_dir = tmp_path / ".local" / "bin" | |
| shim_dir.mkdir(parents=True) | |
| shim = shim_dir / "hermes" | |
| shim.write_text( | |
| f'#!/usr/bin/env bash\nexec "{real_hermes}" "$@"\n', | |
| encoding="utf-8", | |
| ) | |
| shim.chmod(0o755) | |
| # A decoy python next to the shim: this is the bug case. | |
| decoy_python = shim_dir / "python" | |
| decoy_python.write_text("#!/bin/sh\n", encoding="utf-8") | |
| decoy_python.chmod(0o755) | |
| monkeypatch.setattr(install_mod.shutil, "which", lambda _bin: str(shim)) | |
| found = install_mod._find_hermes_python() | |
| assert found == real_python | |
| def test_find_hermes_python_follows_wrapper_script_to_real_venv(tmp_path, monkeypatch): | |
| """A PATH shim that execs the real Hermes binary must not pick a stray python beside the shim.""" | |
| _skip_on_windows() | |
| # Real Hermes venv layout. | |
| real_venv = tmp_path / "hermes-agent" / "venv" | |
| real_bin = real_venv / "bin" | |
| real_bin.mkdir(parents=True) | |
| real_hermes = real_bin / "hermes" | |
| real_hermes.write_text("#!/bin/sh\n", encoding="utf-8") | |
| real_hermes.chmod(0o755) | |
| real_python = real_bin / "python" | |
| real_python.write_text("#!/bin/sh\n", encoding="utf-8") | |
| real_python.chmod(0o755) | |
| # PATH shim in ~/.local/bin that execs the real Hermes binary. | |
| shim_dir = tmp_path / ".local" / "bin" | |
| shim_dir.mkdir(parents=True) | |
| shim = shim_dir / "hermes" | |
| shim.write_text( | |
| f'#!/usr/bin/env bash\nexec "{real_hermes}" "$@"\n', | |
| encoding="utf-8", | |
| ) | |
| shim.chmod(0o755) | |
| # A decoy python next to the shim: this is the bug case. | |
| decoy_python = shim_dir / "python" | |
| decoy_python.write_text("#!/bin/sh\n", encoding="utf-8") | |
| decoy_python.chmod(0o755) | |
| _isolate_hermes_python_sources(tmp_path, monkeypatch) | |
| monkeypatch.setattr(install_mod.shutil, "which", lambda _bin: str(shim)) | |
| found = install_mod._find_hermes_python() | |
| assert found == real_python |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integrations/hermes/tests/test_install_hermes.py` around lines 42 - 72,
Update test_find_hermes_python_follows_wrapper_script_to_real_venv to call
_isolate_hermes_python_sources(tmp_path, monkeypatch) and _skip_on_windows()
before exercising _find_hermes_python(), matching the shared setup used by the
other resolution tests.
…ython _find_hermes_python() only resolved symlinks, so a PATH shim that execs a real Hermes binary (instead of symlinking to it) caused the installer to pick a stray python next to the shim rather than the venv Python. Add _resolve_hermes_bin() which follows the exec target of wrapper scripts and only falls back to the original symlink behavior otherwise. This keeps the PATH probe working for pipx/pip-installed launchers while avoiding the wrong interpreter reported in mnemosyne-oss#618. Closes mnemosyne-oss#618
Address review feedback on mnemosyne-oss#620: - Catch OSError/RuntimeError from Path.resolve() in the symlink branch. - Return None when the resolved target is missing or non-executable. - Apply the same executable and resolve-exception checks to wrapper-script exec targets. - Add regression tests for broken symlinks, non-executable targets, and symlink loops.
…r handling Address follow-up review feedback on mnemosyne-oss#620: - Refactor _resolve_hermes_bin() into a loop that follows symlink + wrapper layers with a visited-path set and a max depth guard against loops. - Return a validated direct executable when the launcher is a plain package entry point (not symlink or wrapper), preserving pipx/pip discovery. - Resolve relative exec targets against the wrapper directory and bare exec targets via PATH lookup. - Catch OSError/RuntimeError from resolve/expanduser and return None on broken/looping symlinks, non-executable targets, or invalid wrapper targets. - Add debug logging with the launcher path and failure reason. - Require os.access(..., X_OK) before accepting sibling python candidates. - Cap wrapper-script read size at 4 KB to avoid reading large binaries. - Add regression tests for direct executables, symlink-to-wrapper chains, relative/bare exec targets, symlink loops, wrapper loops, broken links, and non-executable targets; isolate fallback interpreter sources.
Address the latest CodeRabbit finding on mnemosyne-oss#620: - In _resolve_exec_target(), treat absolute paths and explicit relative paths (containing a directory component) as file paths: join relative ones to the wrapper directory and return None immediately if the path does not exist. - Use shutil.which() only for bare command names, preventing a missing relative path from accidentally resolving to an unrelated binary on PATH. - Strengthen the bare-command regression test with an executable shim/hermes decoy next to the wrapper, asserting that resolution still picks the real_bin/hermes from PATH rather than the adjacent decoy.
- Classify the raw exec target before Path drops a leading "./", so an explicit relative launcher (./hermes-real) resolves beside the wrapper instead of a same-named file in the process working directory. - Replace the exec regex with a bounded tokenizer that follows leading VAR=val assignments and `exec env [VAR=val ...]` handoffs, and rejects forms it cannot parse unambiguously instead of falling back to the wrapper (which could select a system or decoy interpreter). - Add adversarial regression tests: CWD decoy, env prefix, pre-exec assignment, and a rejected arg-taking env option.
Awshesh12
force-pushed
the
fix/hermes-install-wrapper-script-python-618
branch
from
August 9, 2026 07:59
d07a549 to
59fa0c1
Compare
Contributor
Author
|
Thanks for the adversarial pass — both were real gaps. 1. 2. Rebased onto current |
tonythomson
added a commit
to tonythomson/mnemosyne
that referenced
this pull request
Aug 9, 2026
mnemosyne-oss#620 taught _find_hermes_python() to follow a wrapper launcher through its exec target, which fixed the reported mnemosyne-oss#618 case. Two paths still returned the wrong interpreter. A launcher that is neither a symlink nor an exec wrapper -- a script that runs the real binary as a subprocess, or a compiled shim with no exec line to read -- resolves to itself, so bin_dir stays the shim directory and its sibling python is returned. In the reported layout that sibling is ~/.local/bin/python, a Homebrew symlink, and mnemosyne-hermes[all] is installed into it while the run reports success. The known-root branches returned candidate.resolve(). A venv's bin/python is a symlink to its base interpreter, so resolving it discards the venv: bootstrap targets the base install and Hermes' environment is left untouched. Branch 1 already documented this hazard while branches 2-4 did the opposite. A candidate is now returned only when pyvenv.cfg proves its directory is a real virtualenv, applied to the launcher sibling and the install roots alike, and no branch resolves the interpreter symlink. An unvalidated sibling is discarded rather than kept as a fallback: the only layout it uniquely covers is a non-venv system install, which is where bootstrapping does the most damage. Validation can now yield nothing, so the caller changes with it, in the same commit because neither half stands alone: - --python is authoritative and reaches symlink-mode discovery and --dry-run, where it previously only selected wrapper-install site-packages. - A symlink install with no validated interpreter stops and names --python instead of proceeding. --no-bootstrap continues with a warning: it already forbids touching Hermes' venv, so there is no wrong-interpreter install to prevent, and failing there would preempt the guard that refuses to replace an existing wrapper install. - The installer-vs-Hermes comparison uses the paths as selected. Resolving both sides reported a venv and its base interpreter as the same runtime and skipped the bootstrap check entirely. The fixtures in test_install_hermes.py model pip/pipx venvs that omitted pyvenv.cfg because nothing required it; they now write the marker a real venv always has, leaving each test's subject unchanged. The stubs in test_install_status.py accept the new keyword. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq
tonythomson
added a commit
to tonythomson/mnemosyne
that referenced
this pull request
Aug 10, 2026
mnemosyne-oss#620 taught _find_hermes_python() to follow a wrapper launcher through its exec target, which fixed the reported mnemosyne-oss#618 case. Two paths still returned the wrong interpreter. A launcher that is neither a symlink nor an exec wrapper -- a script that runs the real binary as a subprocess, or a compiled shim with no exec line to read -- resolves to itself, so bin_dir stays the shim directory and its sibling python is returned. In the reported layout that sibling is ~/.local/bin/python, a Homebrew symlink, and mnemosyne-hermes[all] is installed into it while the run reports success. The known-root branches returned candidate.resolve(). A venv's bin/python is a symlink to its base interpreter, so resolving it discards the venv: bootstrap targets the base install and Hermes' environment is left untouched. Branch 1 already documented this hazard while branches 2-4 did the opposite. A candidate is now returned only when pyvenv.cfg proves its directory is a real virtualenv, applied to the launcher sibling and the install roots alike, and no branch resolves the interpreter symlink. An unvalidated sibling is discarded rather than kept as a fallback: the only layout it uniquely covers is a non-venv system install, which is where bootstrapping does the most damage. Validation can now yield nothing, so the caller changes with it, in the same commit because neither half stands alone: - --python is authoritative and reaches symlink-mode discovery and --dry-run, where it previously only selected wrapper-install site-packages. - A symlink install with no validated interpreter stops and names --python instead of proceeding. --no-bootstrap continues with a warning: it already forbids touching Hermes' venv, so there is no wrong-interpreter install to prevent, and failing there would preempt the guard that refuses to replace an existing wrapper install. - The installer-vs-Hermes comparison uses the paths as selected. Resolving both sides reported a venv and its base interpreter as the same runtime and skipped the bootstrap check entirely. The fixtures in test_install_hermes.py model pip/pipx venvs that omitted pyvenv.cfg because nothing required it; they now write the marker a real venv always has, leaving each test's subject unchanged. The stubs in test_install_status.py accept the new keyword. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq
dplush
pushed a commit
that referenced
this pull request
Aug 10, 2026
…follow-up to #620) (#623) * fix(hermes): validate a discovered interpreter before returning it #620 taught _find_hermes_python() to follow a wrapper launcher through its exec target, which fixed the reported #618 case. Two paths still returned the wrong interpreter. A launcher that is neither a symlink nor an exec wrapper -- a script that runs the real binary as a subprocess, or a compiled shim with no exec line to read -- resolves to itself, so bin_dir stays the shim directory and its sibling python is returned. In the reported layout that sibling is ~/.local/bin/python, a Homebrew symlink, and mnemosyne-hermes[all] is installed into it while the run reports success. The known-root branches returned candidate.resolve(). A venv's bin/python is a symlink to its base interpreter, so resolving it discards the venv: bootstrap targets the base install and Hermes' environment is left untouched. Branch 1 already documented this hazard while branches 2-4 did the opposite. A candidate is now returned only when pyvenv.cfg proves its directory is a real virtualenv, applied to the launcher sibling and the install roots alike, and no branch resolves the interpreter symlink. An unvalidated sibling is discarded rather than kept as a fallback: the only layout it uniquely covers is a non-venv system install, which is where bootstrapping does the most damage. Validation can now yield nothing, so the caller changes with it, in the same commit because neither half stands alone: - --python is authoritative and reaches symlink-mode discovery and --dry-run, where it previously only selected wrapper-install site-packages. - A symlink install with no validated interpreter stops and names --python instead of proceeding. --no-bootstrap continues with a warning: it already forbids touching Hermes' venv, so there is no wrong-interpreter install to prevent, and failing there would preempt the guard that refuses to replace an existing wrapper install. - The installer-vs-Hermes comparison uses the paths as selected. Resolving both sides reported a venv and its base interpreter as the same runtime and skipped the bootstrap check entirely. The fixtures in test_install_hermes.py model pip/pipx venvs that omitted pyvenv.cfg because nothing required it; they now write the marker a real venv always has, leaving each test's subject unchanged. The stubs in test_install_status.py accept the new keyword. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq * docs(hermes): document how the installer selects Hermes' interpreter Covers the discovery order, the pyvenv.cfg requirement, the fail-closed default path, and --no-bootstrap as an explicitly unvalidated install that needs `hermes memory status` to confirm. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq * docs(hermes): bound the claim about wrapper-launcher resolution The section said discovery follows a shell-wrapper launcher, with no mention that it reads a capped prefix, follows a capped number of hops, and understands a fixed set of exec forms. An unsupported layout falls through to the install roots instead, and needs --python when nothing is found there. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq * fix(hermes): require the known-root interpreter to be executable Aligns the known-root branch with the validated-runtime contract the launcher branch already applies. Everything downstream runs the candidate as `<python> -m pip install ...`, so a bin/python that exists inside a valid venv but cannot be executed is not a runtime and must not be returned. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq * fix(hermes): hold every implicit candidate to one validation predicate The sys.prefix and VIRTUAL_ENV branches still returned any existing bin/python. VIRTUAL_ENV is an ordinary environment variable, not an assertion that a venv is live, so a stale or hand-set VIRTUAL_ENV=/usr named /usr/bin/python and would have handed bootstrap the system interpreter. sys.prefix != sys.base_prefix describes the running interpreter and says nothing about the bin/python being asked for, which can be absent or non-executable in a partially built environment. _is_validated_venv_python() is now the single predicate for the launcher, the known install roots, sys.prefix and VIRTUAL_ENV: an existing file, executable, inside a real virtualenv. The four branches can no longer drift apart. Only --python bypasses it, deliberately, since an explicitly named interpreter is reported against rather than silently swapped. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq * fix(hermes): stop the wrapper dry run from reporting a bootstrap run_install() does not look for an interpreter in wrapper mode, let alone bootstrap one, but the dry run printed `Will bootstrap:` whenever it held an interpreter. Threading --python into discovery gave the wrapper dry run a truthy value, so `install --dry-run --mode wrapper --python <valid>` described work that would never run. The line is now gated on symlink mode. The interpreter itself is still reported, through the existing `Wrapper Python:` line. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq * fix(hermes): reject an empty --python instead of falling through `if explicit_python:` treated an empty string like "not supplied", so `--python ""` dropped into implicit discovery and could answer with a different interpreter than the one the user asked for. That is the silent substitution branch 0 exists to prevent. None is now the only "not supplied" signal. An empty or whitespace-only value raises, which main() reports as `error: ...` with exit 1, so the run stops rather than installing somewhere the user did not name. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq * fix(hermes): keep an explicit --python path exactly as given The blank-value check stripped the value it then returned. A POSIX filename may end in whitespace, so stripping named a path that does not exist and sent the install elsewhere -- the same silent substitution the explicit branch exists to prevent. strip() is now used only to decide whether anything was named; Path() gets the original string. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq * fix(hermes): fail closed when a launcher cannot be classified _wrapper_exec_target() returned (False, None) -- "read it, it is a binary" -- for three cases where it had not established that. (False, None) licenses the caller to trust the interpreter beside the launcher, so each one could hand bootstrap an unrelated venv: - A shebang script larger than the read bound. The parser read exactly _MAX_WRAPPER_READ_BYTES, so a handoff past that was invisible and the script looked like a direct executable. - An unreadable launcher. OSError said nothing about what the file is. - An exec line that will not tokenize. The line was skipped, and the file then reached the "no handoff found" return. The read is now binary and takes _MAX_WRAPPER_READ_BYTES + 1, so the overflow is detectable. A non-shebang file is still a direct launcher, which keeps the pipx layout the launcher branch exists to serve; every case where the classification is unknown returns (True, None) instead. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq * docs: limit the validation claim to implicitly discovered interpreters The entry said every returned candidate requires pyvenv.cfg. An explicit non-empty --python deliberately bypasses the predicate, and the entry now says so, alongside the executable requirement and the branches it covers. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq * fix(hermes): fail closed on every unparsed exec handoff, not just the read Only a leading `exec ...` was recognised, so any other shape fell through to the "no handoff found" return, which licenses the caller to trust the launcher's sibling interpreter. Now covered: if true; then exec /opt/hermes/bin/hermes "$@"; fi [ -x /opt/hermes ] && exec /opt/hermes/bin/hermes "$@" cd /tmp; exec /opt/hermes/bin/hermes "$@" run() { exec /opt/hermes/bin/hermes "$@"; } sh -c "exec /opt/hermes/bin/hermes" _mentions_exec() matches a token whose first word is `exec`, which catches the bare token and the nested-quoted case that arrives as one token. It is deliberately not a substring search: a Python console script calling os.execv() or the exec builtin is a direct launcher, and failing closed on it would break the pipx layout the launcher branch exists to serve. Claude-Session: https://claude.ai/code/session_01BAsEiEXsPWUpsfDgDDjAFq
13 tasks
This was referenced Aug 13, 2026
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.
Closes #618
When
hermeson PATH is a shell wrapper thatexecs the real Hermes binary (instead of being a symlink to it),_find_hermes_python()resolved the wrapper's own directory and picked a straypythonnext to the shim, causing the installer to bootstrap into the wrong environment.Changes:
_resolve_hermes_bin()which follows theexectarget of wrapper scripts and falls back to symlink resolution otherwise._find_hermes_python()now uses the resolved real Hermes binary to locate the venvpythonsibling.pythonbeside it.This keeps the PATH probe working for pipx/pip-installed launchers while avoiding the wrong interpreter reported in #618.
Summary
PATH-basedexectargets.Architecture impact