From 37e4cd5faa89e793520198cee04a2c1f4a41b0e3 Mon Sep 17 00:00:00 2001
From: teknium1 <127238744+teknium1@users.noreply.github.com>
Date: Mon, 14 Sep 2026 21:04:14 -0700
Subject: [PATCH] fix(updater): a probe that cannot be spawned stays advisory
 instead of failing the update

bounded_probe_run collapses spawn failure and timeout into one None, so a venv
interpreter that exists but cannot be executed (PermissionError, ENOEXEC, fork
failure) was reported as "timed out before reporting import health" -- a bogus
warning on the git path and sys.exit(1) on the ZIP path -- and the
`except OSError: return {}` branch beneath it was dead. Let the Popen error
propagate (opt-in flag, default unchanged for the other probes) so "we could not
run our own probe" says nothing about the checkout and no longer blocks the
update, while a spawned child that hangs is still a verdict.

Restores the non-fatal test the earlier rewrite dropped, now driving the real
bounded_probe_run/Popen: red on the previous head, green here.

Review finding: spawn failure of the import probe reported as a fatal timeout.
---
 hermes_cli/_subprocess_compat.py             |  7 ++++++-
 hermes_cli/update_cmd_deps.py                |  7 ++++---
 tests/hermes_cli/test_update_import_guard.py | 12 ++++++++++++
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/hermes_cli/_subprocess_compat.py b/hermes_cli/_subprocess_compat.py
index 97d94f6ec7..07370c302b 100644
--- a/hermes_cli/_subprocess_compat.py
+++ b/hermes_cli/_subprocess_compat.py
@@ -498,11 +498,14 @@ def _legacy_kill_process_tree(proc: "subprocess.Popen") -> None:
 def bounded_probe_run(
     argv: Sequence[str], *, timeout: float, errors: str = "replace",
     env: "Mapping[str, str] | None" = None, cwd: "str | os.PathLike[str] | None" = None,
+    raise_on_spawn_failure: bool = False,
 ) -> "subprocess.CompletedProcess[str] | None":
     """Deadlock-safe ``subprocess.run(argv, capture_output=True, timeout=…)`` for fail-open probes.
 
     Returns a ``CompletedProcess`` when the child finished within *timeout* (any exit code), or
-    ``None`` on spawn failure or timeout.
+    ``None`` on spawn failure or timeout. With ``raise_on_spawn_failure=True`` the ``Popen``
+    exception propagates instead, so callers that treat a *timeout* as a verdict can still tell
+    "our own probe never started" apart from "the child hung".
 
     Why not ``subprocess.run``: on Windows, ``run()``'s post-timeout cleanup calls an *unbounded*
     ``communicate()`` after killing the direct child. Killing it can leave a descendant (``git.exe`` under a
@@ -518,6 +521,8 @@ def bounded_probe_run(
             text=True, encoding="utf-8", errors=errors,
             env=dict(env) if env is not None else None, cwd=cwd, **_popen_kwargs)
     except Exception:
+        if raise_on_spawn_failure:
+            raise
         return None
     try:
         stdout, stderr = proc.communicate(timeout=timeout)
diff --git a/hermes_cli/update_cmd_deps.py b/hermes_cli/update_cmd_deps.py
index 5b2cd19d0c..b0c0c97d14 100644
--- a/hermes_cli/update_cmd_deps.py
+++ b/hermes_cli/update_cmd_deps.py
@@ -89,11 +89,12 @@ def _critical_module_import_failures(
             if venv_python.exists():
                 interpreter = str(venv_python)
         result = bounded_probe_run(
-            [interpreter, "-c", probe], timeout=120, cwd=str(root),
+            [interpreter, "-c", probe], timeout=120, cwd=str(root), raise_on_spawn_failure=True,
         )
     except (OSError, subprocess.SubprocessError):
-        # Keep this guard advisory: a broken probe runner cannot make an otherwise
-        # successful update fail.
+        # Keep this guard advisory: a probe we could not even spawn (unreadable venv
+        # interpreter, fork failure) says nothing about the checkout, so it must not
+        # fail an otherwise successful update. A spawned child that hangs does.
         return {}
     if result is None:
         return _probe_failure("TimeoutExpired", "timed out before reporting import health")
diff --git a/tests/hermes_cli/test_update_import_guard.py b/tests/hermes_cli/test_update_import_guard.py
index 7262c279e5..235a1f1e36 100644
--- a/tests/hermes_cli/test_update_import_guard.py
+++ b/tests/hermes_cli/test_update_import_guard.py
@@ -16,6 +16,7 @@ and the syntax guard reported the update as successful.
 
 from __future__ import annotations
 
+import sys
 from pathlib import Path
 
 import pytest
@@ -249,6 +250,17 @@ def test_import_guard_reports_bounded_probe_failure(monkeypatch, tmp_path):
     )
 
 
+@pytest.mark.skipif(sys.platform == "win32", reason="POSIX exec-bit PermissionError")
+def test_import_guard_is_non_fatal_when_probe_cannot_run(tmp_path):
+    """A venv interpreter that exists but cannot be executed must not read as a hung probe:
+    spawn failure stays advisory (real ``bounded_probe_run``, real ``Popen``)."""
+    venv_python = tmp_path / "venv" / "bin" / "python"
+    venv_python.parent.mkdir(parents=True)
+    venv_python.write_text("#!/bin/sh\nexit 0\n")
+    venv_python.chmod(0o644)  # present, not executable -> Popen raises PermissionError
+    assert update_cmd._validate_critical_modules_import(tmp_path) == (True, None, None)
+
+
 # ---------------------------------------------------------------------------
 # partial_update_hint
 # ---------------------------------------------------------------------------