fix(terminal): correct off-by-one in _hermes_repo_root path resolution
_hermes_repo_root used parents[1] which resolves to tools/ instead of the repository root. The file lives at tools/environments/local.py so it needs parents[2] to reach the actual repo root that Electron injects into PYTHONPATH. The test test_repo_root_stripped reused the module constant under test as its input. This made it pass regardless of what the constant pointed at. The test now computes the real repo root independently from the source file location. It fails with the old parents[1] code and passes with the fix. Reported by spfcraze in PR #78917 review.
This commit is contained in:
@@ -10,6 +10,7 @@ See: https://github.com/NousResearch/hermes-agent/issues/1264
|
||||
|
||||
import os
|
||||
import threading
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
@@ -532,16 +533,31 @@ class TestPythonpathSelectiveStrip:
|
||||
assert "/home/user/my-lib" in entries
|
||||
|
||||
def test_repo_root_stripped(self):
|
||||
"""The Hermes repo root entry is stripped from PYTHONPATH."""
|
||||
from tools.environments.local import _strip_mismatched_site_packages, _hermes_repo_root
|
||||
repo_root = str(_hermes_repo_root)
|
||||
"""The Hermes repo root entry is stripped from PYTHONPATH.
|
||||
|
||||
Electron prepends the *actual* repository root (the directory
|
||||
containing ``tools/``, ``hermes_cli/``, etc.) to PYTHONPATH so the
|
||||
backend can ``import tools``. This test independently computes that
|
||||
real repo root from the source-file location - three levels up from
|
||||
``tools/environments/local.py`` - rather than reusing the module
|
||||
constant under test. That way an off-by-one in ``_hermes_repo_root``
|
||||
(e.g. ``parents[1]`` resolving to ``tools/``) would cause this test
|
||||
to fail instead of silently passing.
|
||||
"""
|
||||
from tools.environments.local import _strip_mismatched_site_packages
|
||||
|
||||
# Independently compute the real repo root: local.py lives at
|
||||
# tools/environments/local.py, so the repo root is parents[2].
|
||||
local_file = Path(__import__("tools.environments.local", fromlist=["__file__"]).__file__).resolve()
|
||||
real_repo_root = str(local_file.parents[2])
|
||||
|
||||
env = {
|
||||
"PYTHONPATH": os.pathsep.join([repo_root, "/home/user/my-lib"]),
|
||||
"PYTHONPATH": os.pathsep.join([real_repo_root, "/home/user/my-lib"]),
|
||||
}
|
||||
_strip_mismatched_site_packages(env)
|
||||
pp = env.get("PYTHONPATH", "")
|
||||
entries = pp.split(os.pathsep) if pp else []
|
||||
assert repo_root not in entries
|
||||
assert real_repo_root not in entries
|
||||
assert "/home/user/my-lib" in entries
|
||||
|
||||
|
||||
|
||||
@@ -1364,12 +1364,13 @@ def _is_path_under(child: Path, parent: Path) -> bool:
|
||||
#: the interpreter is actually inside a venv (``sys.prefix != sys.base_prefix``).
|
||||
_hermes_venv_root: Path = Path(sys.prefix)
|
||||
|
||||
#: The Hermes repository root - two levels up from this file
|
||||
#: (``tools/environments/local.py`` -> ``tools/`` -> repo root). This is the
|
||||
#: directory the Electron app prepends to PYTHONPATH so the backend can do
|
||||
#: ``import tools``, ``import hermes_cli``, etc. Subprocesses that are NOT
|
||||
#: the Hermes backend don't need it and it can shadow local packages.
|
||||
_hermes_repo_root: Path = Path(__file__).resolve().parents[1]
|
||||
#: The Hermes repository root - three levels up from this file
|
||||
#: (``tools/environments/local.py`` -> ``tools/environments`` -> ``tools``
|
||||
#: -> repo root). This is the directory the Electron app prepends to
|
||||
#: PYTHONPATH so the backend can do ``import tools``, ``import hermes_cli``,
|
||||
#: etc. Subprocesses that are NOT the Hermes backend don't need it and it
|
||||
#: can shadow local packages.
|
||||
_hermes_repo_root: Path = Path(__file__).resolve().parents[2]
|
||||
|
||||
#: Whether the current interpreter is running inside a venv. On Python 3.3+
|
||||
#: ``sys.base_prefix != sys.prefix`` indicates a venv (or virtualenv).
|
||||
|
||||
Reference in New Issue
Block a user