fix(bootstrap): address Opus pre-merge review feedback (#1478)
Three changes from the pre-merge Opus review: **MUST-FIX** — XPC_SERVICE_NAME false-positive on macOS Terminal macOS launchd sets `XPC_SERVICE_NAME` in EVERY Terminal-spawned shell, not just real services. Typical noise values: `"0"` (truthy in Python!) and `"application.com.apple.Terminal.<UUID>"`. A bare `os.environ.get(name)` existence check would auto-promote interactive `./start.sh` runs to foreground mode on every Mac dev machine — silently breaking the most common installation path (no /health probe, no browser open, no log file, hanging shell). Fix: new `_is_real_supervisor_value()` helper that filters noise. For `XPC_SERVICE_NAME` specifically, reject `"0"` and any `"application.*"` prefix. Real launchd plists use reverse-DNS Label form (`com.<rdns>.<svc>`) which still triggers correctly. 7 new tests in `TestXPCServiceNameNoiseFilter`: - 4 noise values (`0`, Terminal.app, iTerm2, VSCode) → no detection - 3 real Label forms → correct detection - Mixed env with XPC noise + real INVOCATION_ID → falls through to systemd **SHOULD-FIX 1** — Test env leakage The original `clean_env` fixture stripped supervisor-detection env vars but not the resolved bootstrap vars (HERMES_WEBUI_HOST/PORT/AGENT_DIR) that `main()` mutates onto `os.environ`. After `test_foreground_exports_resolved_env_vars` ran, later tests would import bootstrap with polluted defaults (DEFAULT_HOST="0.0.0.0" instead of "127.0.0.1"). Existing assertions still passed (tautological vs DEFAULT_*), but it was a footgun for future tests. Fix: extend `clean_env` to also `delenv` the three resolved vars before each test. **SHOULD-FIX 2** — Pre-execv executability guard If `discover_launcher_python` returns a path that doesn't exist or isn't executable, `os.execv` raises OSError → wrapper catches → SystemExit(1) → supervisor restarts → loop forever. That's exactly the failure mode this PR is supposed to eliminate. Fix: `os.access(python_exe, os.X_OK)` check before execv. Converts infinite supervisor loop into a single visible RuntimeError. 1 new test in `TestForegroundExecutabilityGuard` pinning that the guard fires before execv when the python path is non-executable. **Docs** — supervisor.md updates - New section explaining the XPC_SERVICE_NAME noise filter and what values trigger / don't trigger detection - New section listing supervisors that are NOT auto-detected (runit, daemontools, PM2, Foreman/Honcho, custom shell-script supervisors) with explicit recommendation to set HERMES_WEBUI_FOREGROUND=1 Verification - 3820 tests pass (+9 from this commit's new tests vs the original PR push of 3811) - Filter manually verified end-to-end with the live os.environ: XPC=0 → None, XPC=application.* → None, XPC=com.example.foo → triggers - run-browser-tests.sh ALL CHECKS PASSED on the worktree Items deferred from the Opus review - #4 chdir target may not exist: REPO_ROOT comes from __file__.resolve() so it's stable; not a real concern in practice - #6 two startup messages in foreground mode: cosmetic, useful for diagnostics - #7 stricter explicit-only mode: leaves user the override of just not passing --foreground (current behavior) - #8 test stub return value: trivial, can fix later if regression surface - #9 argparse positional-after-option ordering: test reads fine These can be follow-up issues if anyone hits them.
This commit is contained in:
41
bootstrap.py
41
bootstrap.py
@@ -232,6 +232,14 @@ def parse_args() -> argparse.Namespace:
|
||||
# - XPC_SERVICE_NAME launchd (set to the Label of the running plist)
|
||||
# - SUPERVISOR_ENABLED supervisord
|
||||
# - HERMES_WEBUI_FOREGROUND explicit user opt-in (=1 / true / yes / on)
|
||||
#
|
||||
# Note on XPC_SERVICE_NAME: macOS launchd sets this in EVERY Terminal-launched
|
||||
# shell too — typical values include "0" (truthy in Python!) and
|
||||
# "application.com.apple.Terminal.<UUID>". A bare existence check would
|
||||
# false-positive on every Mac dev machine running ./start.sh interactively.
|
||||
# We narrow to launchd Label-style names (com.<reverse-dns>.<svc>) — those
|
||||
# are real services. Verified with `launchctl getenv XPC_SERVICE_NAME` and
|
||||
# Apple's documented launchd behavior.
|
||||
_SUPERVISOR_ENV_VARS = (
|
||||
"INVOCATION_ID",
|
||||
"JOURNAL_STREAM",
|
||||
@@ -241,6 +249,26 @@ _SUPERVISOR_ENV_VARS = (
|
||||
)
|
||||
|
||||
|
||||
def _is_real_supervisor_value(name: str, value: str) -> bool:
|
||||
"""Filter out known-noise env-var values that aren't actual supervisors.
|
||||
|
||||
Most env vars in _SUPERVISOR_ENV_VARS are only set by the supervisor we
|
||||
care about, so any non-empty value is meaningful. XPC_SERVICE_NAME is the
|
||||
exception: macOS launchd sets it in every Terminal-spawned shell with
|
||||
values like "0" or "application.com.apple.Terminal.<UUID>". A real
|
||||
launchd-managed service has a reverse-DNS Label like "com.example.foo".
|
||||
"""
|
||||
if not value:
|
||||
return False
|
||||
if name == "XPC_SERVICE_NAME":
|
||||
# Reject Apple's noise values; accept Label-style names.
|
||||
if value == "0":
|
||||
return False
|
||||
if value.startswith("application."):
|
||||
return False
|
||||
return True
|
||||
|
||||
|
||||
def _detect_supervisor() -> str | None:
|
||||
"""Return the name of the detected supervisor env var, or None.
|
||||
|
||||
@@ -251,7 +279,8 @@ def _detect_supervisor() -> str | None:
|
||||
if explicit in ("1", "true", "yes", "on"):
|
||||
return "HERMES_WEBUI_FOREGROUND"
|
||||
for name in _SUPERVISOR_ENV_VARS:
|
||||
if os.environ.get(name):
|
||||
value = os.environ.get(name, "")
|
||||
if _is_real_supervisor_value(name, value):
|
||||
return name
|
||||
return None
|
||||
|
||||
@@ -301,6 +330,16 @@ def main() -> int:
|
||||
raise RuntimeError(
|
||||
f"Could not chdir to {server_cwd!r} before exec: {exc}"
|
||||
) from exc
|
||||
# Defensive check: if python_exe is missing or non-executable, execv
|
||||
# raises OSError, the wrapper catches and SystemExit(1)s, and the
|
||||
# supervisor restarts — looping forever, exactly the failure mode this
|
||||
# PR is meant to eliminate. Convert to a single visible error.
|
||||
if not os.access(python_exe, os.X_OK):
|
||||
raise RuntimeError(
|
||||
f"Python interpreter at {python_exe!r} is not executable. "
|
||||
f"Set HERMES_WEBUI_PYTHON to a working interpreter or fix "
|
||||
f"the agent venv at {agent_dir}."
|
||||
)
|
||||
# os.execv replaces the current process image. Anything after this line
|
||||
# only runs if execv itself fails (it raises OSError on failure).
|
||||
os.execv(python_exe, [python_exe, server_path])
|
||||
|
||||
Reference in New Issue
Block a user