diff --git a/tests/tools/test_code_execution_modes.py b/tests/tools/test_code_execution_modes.py index f114c1ab8c..168bd8dcc7 100644 --- a/tests/tools/test_code_execution_modes.py +++ b/tests/tools/test_code_execution_modes.py @@ -39,6 +39,7 @@ from tools.code_execution_tool import ( _get_execution_mode, _is_usable_python, _python_environment_prefix, + _python_prefix_cache, _resolve_child_cwd, _resolve_child_python, _uses_hermes_python_environment, @@ -385,10 +386,10 @@ class TestPythonEnvironmentPrefix(unittest.TestCase): """Unit tests for the helper that queries sys.prefix of an interpreter.""" def setUp(self): - _python_environment_prefix.cache_clear() + _python_prefix_cache.clear() def tearDown(self): - _python_environment_prefix.cache_clear() + _python_prefix_cache.clear() def test_returns_realpath_of_current_interpreter_prefix(self): """Happy path: sys.executable reports its own prefix.""" @@ -431,20 +432,49 @@ class TestPythonEnvironmentPrefix(unittest.TestCase): _python_environment_prefix("/cached/python") self.assertEqual(mock_run.call_count, 1) + def test_failure_is_not_cached(self): + """A transient probe failure must not stick — the next call retries. + + A sticky cached failure would silently drop the hermes root from + every subsequent execute_code call in the process. + """ + with patch("subprocess.run", + side_effect=subprocess.TimeoutExpired(cmd=[], timeout=5)) as mock_run: + self.assertEqual(_python_environment_prefix("/flaky/python"), "") + self.assertEqual(mock_run.call_count, 1) + with patch("subprocess.run") as mock_run: + mock_run.return_value = unittest.mock.MagicMock( + returncode=0, stdout="/recovered/prefix\n" + ) + result = _python_environment_prefix("/flaky/python") + self.assertEqual(mock_run.call_count, 1, "probe must be retried after a failure") + self.assertEqual(result, os.path.realpath("/recovered/prefix")) + class TestUsesHermesPythonEnvironment(unittest.TestCase): """Unit tests for _uses_hermes_python_environment.""" def setUp(self): - _python_environment_prefix.cache_clear() + _python_prefix_cache.clear() def tearDown(self): - _python_environment_prefix.cache_clear() + _python_prefix_cache.clear() def test_true_for_current_interpreter(self): """sys.executable always belongs to the current environment.""" self.assertTrue(_uses_hermes_python_environment(sys.executable)) + def test_true_for_current_interpreter_without_probe(self): + """sys.executable short-circuits — no subprocess probe on the default path. + + Guards the strict-mode invariant: a flaky probe (timeout under load) + must never drop the hermes root for the interpreter Hermes itself runs. + """ + with patch("subprocess.run", + side_effect=subprocess.TimeoutExpired(cmd=[], timeout=5)) as mock_run: + self.assertTrue(_uses_hermes_python_environment(sys.executable)) + mock_run.assert_not_called() + def test_false_for_different_prefix(self): """An interpreter reporting a different prefix is external.""" with patch("tools.code_execution_tool._python_environment_prefix", @@ -476,13 +506,15 @@ class TestPythonPathComposition(unittest.TestCase): cover the detection logic end-to-end. """ - def _capture_pythonpath(self, same_env: bool) -> str: - """Return the PYTHONPATH that execute_code would pass to the child.""" + def _capture_pythonpath(self, same_env: bool) -> tuple: + """Return (PYTHONPATH, staging_dir) that execute_code passes to the child.""" captured = {} def _fake_popen(cmd, **kwargs): env = kwargs.get("env") or {} captured["PYTHONPATH"] = env.get("PYTHONPATH", "") + # cmd is [python, /script.py] + captured["staging_dir"] = os.path.dirname(cmd[1]) mock_proc = unittest.mock.MagicMock() mock_proc.stdout.read.return_value = b"" mock_proc.stderr.read.return_value = b"" @@ -496,41 +528,41 @@ class TestPythonPathComposition(unittest.TestCase): patch("tools.code_execution_tool._uses_hermes_python_environment", return_value=same_env), \ patch("subprocess.Popen", side_effect=_fake_popen): - try: - execute_code(code="pass", task_id="test-pp", enabled_tools=[]) - except Exception: - pass + execute_code(code="pass", task_id="test-pp", enabled_tools=[]) - return captured.get("PYTHONPATH", "") + # If execute_code never reached Popen, the capture is empty and any + # "X not in PYTHONPATH" assertion downstream would pass vacuously. + self.assertIn("PYTHONPATH", captured, + "execute_code never spawned the child process") + return captured["PYTHONPATH"], captured["staging_dir"] def _hermes_root(self) -> str: - tools_dir = os.path.dirname(os.path.abspath( - __import__("tools.code_execution_tool", fromlist=["__file__"]).__file__ - )) + import tools.code_execution_tool as _cet + tools_dir = os.path.dirname(os.path.abspath(_cet.__file__)) return os.path.dirname(tools_dir) def test_hermes_root_included_when_same_env(self): """When interpreter is in the Hermes env, hermes root is in PYTHONPATH.""" - pythonpath = self._capture_pythonpath(same_env=True) + pythonpath, _ = self._capture_pythonpath(same_env=True) parts = pythonpath.split(os.pathsep) self.assertIn(self._hermes_root(), parts, "hermes root must be in PYTHONPATH for same-env interpreters") def test_hermes_root_excluded_when_external_env(self): """When interpreter is external, hermes root must NOT be in PYTHONPATH.""" - pythonpath = self._capture_pythonpath(same_env=False) + pythonpath, _ = self._capture_pythonpath(same_env=False) parts = pythonpath.split(os.pathsep) self.assertNotIn(self._hermes_root(), parts, "hermes root must not leak into an external interpreter's PYTHONPATH") - def test_staging_dir_always_present(self): + def test_staging_dir_always_first(self): """The staging tmpdir must always be the first PYTHONPATH entry.""" for same_env in (True, False): with self.subTest(same_env=same_env): - pythonpath = self._capture_pythonpath(same_env=same_env) + pythonpath, staging_dir = self._capture_pythonpath(same_env=same_env) parts = pythonpath.split(os.pathsep) - self.assertTrue(parts and parts[0], - "PYTHONPATH must start with the staging tmpdir") + self.assertEqual(parts[0], staging_dir, + "PYTHONPATH must start with the staging tmpdir") if __name__ == "__main__": diff --git a/tools/code_execution_tool.py b/tools/code_execution_tool.py index 6342338f9f..16a4ce1ac0 100644 --- a/tools/code_execution_tool.py +++ b/tools/code_execution_tool.py @@ -1489,6 +1489,14 @@ def execute_code( _pp_parts = [tmpdir] if _uses_hermes_python_environment(_child_python): _pp_parts.append(_hermes_root) + else: + # Import behavior changes silently otherwise — surface it so + # "import hermes_constants suddenly fails" reports are diagnosable. + logger.info( + "execute_code: child interpreter %s is outside the Hermes " + "environment; hermes root omitted from PYTHONPATH", + _child_python, + ) if _existing_pp: _pp_parts.append(_existing_pp) child_env["PYTHONPATH"] = os.pathsep.join(_pp_parts) @@ -1839,47 +1847,73 @@ def _is_usable_python(python_path: str) -> bool: Requires Python 3.8+ (f-strings and stdlib modules the RPC stubs need). Cached so we don't fork a subprocess on every execute_code call. """ + result = _probe_python( + python_path, + "import sys; sys.exit(0 if sys.version_info >= (3, 8) else 1)", + ) + return result is not None and result.returncode == 0 + + +def _probe_python(python_path: str, code: str, *, text: bool = False): + """Run ``python_path -c code`` with the standard interpreter-probe guards. + + Returns the ``CompletedProcess``, or ``None`` when the interpreter is + missing, can't be spawned, or hangs past the 5s timeout. + """ try: from agent.delegation_context import delegated_child_subprocess_env - result = subprocess.run( - [python_path, "-c", - "import sys; sys.exit(0 if sys.version_info >= (3, 8) else 1)"], + return subprocess.run( + [python_path, "-c", code], timeout=5, capture_output=True, + text=text, creationflags=subprocess.CREATE_NO_WINDOW if _IS_WINDOWS else 0, stdin=subprocess.DEVNULL, env=delegated_child_subprocess_env(), ) - return result.returncode == 0 except (OSError, subprocess.TimeoutExpired, subprocess.SubprocessError): - return False + return None + + +_python_prefix_cache: dict = {} -@functools.lru_cache(maxsize=32) def _python_environment_prefix(python_path: str) -> str: - """Return the resolved ``sys.prefix`` reported by *python_path*, if any.""" - try: - from agent.delegation_context import delegated_child_subprocess_env + """Return the resolved ``sys.prefix`` reported by *python_path*, if any. - result = subprocess.run( - [python_path, "-c", "import sys; print(sys.prefix)"], - timeout=5, - capture_output=True, - text=True, - creationflags=subprocess.CREATE_NO_WINDOW if _IS_WINDOWS else 0, - stdin=subprocess.DEVNULL, - env=delegated_child_subprocess_env(), - ) - if result.returncode == 0 and result.stdout.strip(): - return os.path.realpath(result.stdout.strip()) - except (OSError, subprocess.TimeoutExpired, subprocess.SubprocessError): - pass + Successful probes are cached per interpreter path. Failures are NOT + cached: a transient probe failure (fork pressure, 5s timeout on a loaded + host) must not stick for the process lifetime — a sticky empty result + would silently drop the hermes root from every subsequent execute_code + call's PYTHONPATH. + """ + cached = _python_prefix_cache.get(python_path) + if cached is not None: + return cached + result = _probe_python(python_path, "import sys; print(sys.prefix)", text=True) + if result is not None and result.returncode == 0 and result.stdout.strip(): + prefix = os.path.realpath(result.stdout.strip()) + if len(_python_prefix_cache) < 32: + _python_prefix_cache[python_path] = prefix + return prefix return "" def _uses_hermes_python_environment(python_path: str) -> bool: - """Whether *python_path* belongs to Hermes's active Python environment.""" + """Whether *python_path* belongs to Hermes's active Python environment. + + Short-circuits when *python_path* IS the running interpreter (by path or + realpath) — no subprocess probe on the default strict-mode path, and no + way for a flaky probe of ``sys.executable`` itself to break the invariant + that repo-root modules are importable in strict mode. The realpath leg + also covers venvs whose bin/python resolves to the same binary (e.g. + ``uv run`` setting VIRTUAL_ENV without changing sys.prefix). + """ + if python_path == sys.executable or ( + os.path.realpath(python_path) == os.path.realpath(sys.executable) + ): + return True return _python_environment_prefix(python_path) == os.path.realpath(sys.prefix)