From 39f1b618fcfc40efeb0aba43702d5f2593171ba0 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:58:22 -0700 Subject: [PATCH] refactor(web_server): compact lifecycle/files/mcp helper docstrings and call layouts --- hermes_cli/web_server_files.py | 51 ++---- hermes_cli/web_server_lifecycle.py | 242 ++++++++--------------------- hermes_cli/web_server_mcp.py | 94 +++-------- 3 files changed, 104 insertions(+), 283 deletions(-) diff --git a/hermes_cli/web_server_files.py b/hermes_cli/web_server_files.py index b2610fafd3..dbb4807d85 100644 --- a/hermes_cli/web_server_files.py +++ b/hermes_cli/web_server_files.py @@ -1,8 +1,7 @@ -"""Managed-files policy for the dashboard file browser: root resolution, path canonicalisation/containment, entry metadata. +"""Managed-files policy for the dashboard file browser: root resolution, path containment, entry metadata. -Split out of ``hermes_cli.web_server``; every externally used name is re-imported -there, so ``web_server.`` keeps resolving (and monkeypatching) as before. -Helpers that tests patch on ``web_server`` are reached lazily through it. +Split out of ``hermes_cli.web_server``, which re-imports every externally used +name so ``web_server.`` keeps resolving (and monkeypatching) as before. """ import mimetypes @@ -93,19 +92,12 @@ def _default_hermes_root_is_opt_data() -> bool: def _dashboard_local_update_managed_externally() -> bool: - """Return true when the dashboard should not offer ``hermes update``. + """True when the dashboard should not offer ``hermes update``. - Containerized dashboards are updated by the outer launcher/image, not by an - in-browser local update action. Keep this dashboard capability separate - from install-method detection: manual git/pip installs inside containers can - still behave like their actual install method in the CLI. - - However, when the install method is ``git`` (a bind-mounted checkout inside - a container — e.g. the hermes-webui image sharing the Hermes source tree), - the dashboard's ``hermes update`` button is the correct update path and - should not be suppressed. Other containerized install methods remain - externally managed unless their apply path is proven safe inside the - running container filesystem. + Containerized dashboards are updated by the outer launcher/image — except a + ``git`` install (bind-mounted checkout, e.g. the hermes-webui image), where + the update button is the correct path. pip stays blocked in containers: its + apply path mutates the running container filesystem. """ from hermes_cli.web_server import PROJECT_ROOT, detect_install_method if _default_hermes_root_is_opt_data(): @@ -117,14 +109,8 @@ def _dashboard_local_update_managed_externally() -> bool: return False except Exception: return False - # We are inside a container, but the install may still be self-managed. - # If the install method is git, the dashboard update button works against - # the mounted checkout and should be offered. Keep pip blocked inside - # containers: its apply path mutates the running container filesystem and - # is not the bind-mounted checkout case this gate is meant to recover. try: - method = detect_install_method(PROJECT_ROOT) - if method == "git": + if detect_install_method(PROJECT_ROOT) == "git": return False except Exception: pass @@ -137,11 +123,9 @@ def _managed_files_policy(request: Request, *, create_root: bool = True) -> Mana root = _ensure_managed_root(raw_forced_root) if create_root else _canonical_path(Path(raw_forced_root)) return ManagedFilesPolicy(default_path=root, locked_root=root, can_change_path=False) - # Remote/OAuth access does not imply a hosted container. Users can expose a - # local dashboard through the auth gate (for example a macOS launchd install) - # and still expect the Files page to browse their local home directory. Lock - # to /opt/data only when the installation's Hermes root is actually /opt/data - # (the container/hosted layout) or when HERMES_DASHBOARD_FILES_ROOT is set. + # Remote/OAuth access does not imply a hosted container (a gated macOS launchd + # install still browses its home). Lock to /opt/data only when the Hermes + # root actually IS /opt/data or HERMES_DASHBOARD_FILES_ROOT is set. if _default_hermes_root_is_opt_data(): root = _ensure_managed_root(_HOSTED_MANAGED_FILES_ROOT) if create_root else _HOSTED_MANAGED_FILES_ROOT return ManagedFilesPolicy(default_path=root, locked_root=root, can_change_path=False) @@ -151,10 +135,7 @@ def _managed_files_policy(request: Request, *, create_root: bool = True) -> Mana def _resolve_managed_path( - raw_path: str | None, - request: Request, - *, - for_write: bool = False, + raw_path: str | None, request: Request, *, for_write: bool = False ) -> tuple[ManagedFilesPolicy, Path, str]: policy = _managed_files_policy(request) text = _path_text(raw_path) @@ -190,11 +171,7 @@ def _resolve_managed_path( def _managed_response_meta(policy: ManagedFilesPolicy) -> Dict[str, Any]: locked_root = str(policy.locked_root) if policy.locked_root is not None else None - return { - "root": locked_root, - "locked_root": locked_root, - "can_change_path": policy.can_change_path, - } + return {"root": locked_root, "locked_root": locked_root, "can_change_path": policy.can_change_path} def _managed_file_entry(policy: ManagedFilesPolicy, target: Path) -> Dict[str, Any]: diff --git a/hermes_cli/web_server_lifecycle.py b/hermes_cli/web_server_lifecycle.py index 5aef159867..a91c313c96 100644 --- a/hermes_cli/web_server_lifecycle.py +++ b/hermes_cli/web_server_lifecycle.py @@ -1,8 +1,7 @@ -"""Serve-process lifecycle helpers: parent start markers and death watchdog, port-conflict preflight, ready-file/sentinel announcement, browser auto-open, forwarded-IP resolution. +"""Serve-process lifecycle: parent death watchdog, port-conflict preflight, READY announcement, browser open, trusted proxies. -Split out of ``hermes_cli.web_server``; every externally used name is re-imported -there, so ``web_server.`` keeps resolving (and monkeypatching) as before. -Helpers that tests patch on ``web_server`` are reached lazily through it. +Split out of ``hermes_cli.web_server``, which re-imports every externally used +name so ``web_server.`` keeps resolving (and monkeypatching) as before. """ import logging @@ -27,8 +26,8 @@ _log = logging.getLogger("hermes_cli.web_server") def _process_start_marker(pid: int) -> str: """Return a cross-runtime marker for the current incarnation of ``pid``. - ``ProcessLookupError`` means the process is absent. Other failures are left - distinct so callers can fail safe rather than killing a healthy backend. + ``ProcessLookupError`` means the process is absent; other failures stay + distinct so callers fail safe rather than killing a healthy backend. """ if sys.platform == "linux": try: @@ -36,8 +35,8 @@ def _process_start_marker(pid: int) -> str: except FileNotFoundError as exc: raise ProcessLookupError(pid) from exc - # The command in field 2 may contain spaces or parentheses. Splitting - # after its final ')' leaves field 3 at index zero and field 22 at 19. + # Field 2 (comm) may contain spaces/parens; split after its final ')' + # so field 3 is index 0 and field 22 (starttime) is index 19. fields = stat_line.rsplit(")", 1)[1].strip().split() if len(fields) < 20 or not fields[19].isdigit(): raise OSError(f"invalid /proc stat data for PID {pid}") @@ -51,13 +50,7 @@ def _process_start_marker(pid: int) -> str: kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) kernel32.OpenProcess.argtypes = [wintypes.DWORD, wintypes.BOOL, wintypes.DWORD] kernel32.OpenProcess.restype = wintypes.HANDLE - kernel32.GetProcessTimes.argtypes = [ - wintypes.HANDLE, - ctypes.POINTER(wintypes.FILETIME), - ctypes.POINTER(wintypes.FILETIME), - ctypes.POINTER(wintypes.FILETIME), - ctypes.POINTER(wintypes.FILETIME), - ] + kernel32.GetProcessTimes.argtypes = [wintypes.HANDLE] + [ctypes.POINTER(wintypes.FILETIME)] * 4 kernel32.GetProcessTimes.restype = wintypes.BOOL kernel32.CloseHandle.argtypes = [wintypes.HANDLE] kernel32.CloseHandle.restype = wintypes.BOOL @@ -68,32 +61,19 @@ def _process_start_marker(pid: int) -> str: raise ProcessLookupError(pid) raise OSError(error, f"OpenProcess failed for PID {pid}") - creation = wintypes.FILETIME() - exit_time = wintypes.FILETIME() - kernel = wintypes.FILETIME() - user = wintypes.FILETIME() + creation, exit_time, kernel, user = (wintypes.FILETIME() for _ in range(4)) try: if not kernel32.GetProcessTimes( - handle, - ctypes.byref(creation), - ctypes.byref(exit_time), - ctypes.byref(kernel), - ctypes.byref(user), + handle, ctypes.byref(creation), ctypes.byref(exit_time), ctypes.byref(kernel), ctypes.byref(user) ): - error = ctypes.get_last_error() - raise OSError(error, f"GetProcessTimes failed for PID {pid}") + raise OSError(ctypes.get_last_error(), f"GetProcessTimes failed for PID {pid}") finally: kernel32.CloseHandle(handle) filetime = (creation.dwHighDateTime << 32) | creation.dwLowDateTime return f"win:{filetime + 504911232000000000}" - result = subprocess.run( - ["ps", "-p", str(pid), "-o", "lstart="], - capture_output=True, - text=True, - check=False, - ) + result = subprocess.run(["ps", "-p", str(pid), "-o", "lstart="], capture_output=True, text=True, check=False) marker = result.stdout.strip() if result.returncode == 0 and marker: return f"ps:{marker}" @@ -112,12 +92,11 @@ def _valid_parent_start_marker(marker: str) -> bool: def _parent_start_markers_match(actual: str, expected: str) -> bool: - """Compare parent markers across Desktop protocol generations. + """Compare parent markers across Desktop generations. - Older Windows Desktop builds send .NET ticks (``win:``). New builds use - Electron's native process creation time in Unix milliseconds (``winms:``) - so startup does not need to launch PowerShell. The backend still reads the - exact FILETIME and normalizes it only when the expected marker is ``winms``. + Old Windows Desktop sends .NET ticks (``win:``); new builds send Electron's + creation time in Unix ms (``winms:``) to avoid launching PowerShell. The + backend reads the exact FILETIME and normalizes only for ``winms``. """ if actual == expected: return True @@ -138,32 +117,18 @@ def _parent_start_markers_match(actual: str, expected: str) -> bool: def _warm_gateway_module() -> None: """Pre-import heavy modules so the event loop is not stalled on first use. - On a cold Windows install, importing these module chains triggers .pyc - compilation and Defender real-time scans that can stall the event loop - for 15-30s. The original fix (pre-#60800) only warmed - ``hermes_cli.gateway``. But the first WS connection and its initial - RPC burst (``setup.status``, ``setup.runtime_check``, - ``gateway.ready``→``resolve_skin``) pull in several *other* heavy - chains that were still imported on the loop thread, contributing to - the ~14s cold-start stall (#60800). Warm them all here so the cost - is paid in a worker thread while the server socket is already open. + Cold Windows installs pay .pyc compilation + Defender scans (15-30s) on + these chains; the first WS RPC burst (setup.status, setup.runtime_check, + gateway.ready→resolve_skin, model.options) pulled them in on the loop + thread (#60800). Warm them all off-loop while the socket is already open. """ for mod in ( "hermes_cli.gateway", - # setup.status / setup.runtime_check resolve provider auth state, - # which imports copilot_auth (→ subprocess module) and scans - # credential files. First import is noticeably slow on Windows. - "hermes_cli.auth", + "hermes_cli.auth", # provider auth state → copilot_auth → subprocess "hermes_cli.copilot_auth", "hermes_cli.runtime_provider", - # resolve_skin() reads config + initialises the skin engine. - # Even though handle_ws now calls it via asyncio.to_thread - # (see tui_gateway/ws.py), warming it here avoids the first-call - # import cost inside that thread. - "hermes_cli.skin_engine", - # model.options / picker context — parses provider catalogs and - # the models.dev cache on first use. - "hermes_cli.inventory", + "hermes_cli.skin_engine", # resolve_skin() config + skin engine init + "hermes_cli.inventory", # provider catalogs + models.dev cache "hermes_cli.model_switch", ): try: @@ -184,12 +149,9 @@ def _resolve_restart_drain_timeout() -> float: def _eager_reconcile_own_session_db() -> None: """One writable open of this process's own state.db at startup. - ``SessionDB.__init__`` runs ``_init_schema`` → ``_reconcile_columns``, - bringing a store left behind by `hermes update` current before the - dashboard's first session-list poll, with the open-time lock patience - (jittered retries) absorbing transient contention. Never raises: a - store this cannot fix is still served through the read-probe heal in - :func:`_open_session_db_at_path`, which retries on every poll. + ``SessionDB.__init__`` runs ``_init_schema`` → ``_reconcile_columns`` with + open-time lock patience. Never raises: an unfixable store still gets the + per-poll read-probe heal in :func:`_open_session_db_at_path`. """ try: from hermes_state import SessionDB, _default_db_path @@ -203,25 +165,17 @@ def _eager_reconcile_own_session_db() -> None: def _read_bound_port(server: "uvicorn.Server", fallback: int) -> int: - """Read the OS-assigned port from a live uvicorn server socket. - - After ``server.startup()`` the socket is bound. Returns the actual - port so ephemeral (port-0) discovery works without a pre-bind TOCTOU. - Falls back to *fallback* if the socket list is empty (shouldn't happen - but guards against uvicorn internals changing). - """ + """Read the OS-assigned port from the live uvicorn socket (ephemeral port-0 discovery).""" if server.servers and server.servers[0].sockets: return server.servers[0].sockets[0].getsockname()[1] return fallback def _write_dashboard_ready_file(actual_port: int) -> None: - """Optionally publish the dashboard port through an atomic ready file. + """Publish the port through an atomic ready file when ``HERMES_DESKTOP_READY_FILE`` is set. - Windows Desktop can launch dashboard backends with ``pythonw.exe`` to avoid - console flashes. That path cannot rely on stdout for the port announcement, - so Electron passes ``HERMES_DESKTOP_READY_FILE`` and waits for this JSON. - Normal CLI/dashboard launches still use the stdout READY line below. + Windows Desktop launches via ``pythonw.exe`` (no console flash) cannot use + stdout for the port announcement, so Electron waits for this JSON instead. """ target = os.environ.get("HERMES_DESKTOP_READY_FILE") if not target: @@ -233,12 +187,7 @@ def _write_dashboard_ready_file(actual_port: int) -> None: path.parent.mkdir(parents=True, exist_ok=True) payload = json.dumps({"port": int(actual_port)}, separators=(",", ":")) with tempfile.NamedTemporaryFile( - "w", - encoding="utf-8", - dir=str(path.parent), - prefix=f"{path.name}.", - suffix=".tmp", - delete=False, + "w", encoding="utf-8", dir=str(path.parent), prefix=f"{path.name}.", suffix=".tmp", delete=False ) as fh: fh.write(payload) fh.flush() @@ -254,26 +203,18 @@ def _write_dashboard_ready_file(actual_port: int) -> None: _log.warning("Failed to write dashboard ready file %r: %s", target, exc) -def _maybe_open_browser( - host: str, actual_port: int, open_browser: bool, initial_profile: str -) -> None: +def _maybe_open_browser(host: str, actual_port: int, open_browser: bool, initial_profile: str) -> None: """Open the dashboard URL in the user's browser if appropriate. - Skips on headless Linux (no ``DISPLAY`` / ``WAYLAND_DISPLAY``) to avoid - TUI browsers (links, lynx) that would SIGHUP the server process. - Maps ``0.0.0.0`` / ``::`` binds to ``127.0.0.1`` so the browser opens - a reachable URL. + Skips headless Linux (no DISPLAY/WAYLAND_DISPLAY) so a TUI browser can't + SIGHUP the server; maps ``0.0.0.0``/``::`` binds to ``127.0.0.1``. """ if not open_browser: return import webbrowser - _has_display = ( - sys.platform != "linux" - or bool(os.environ.get("DISPLAY")) - or bool(os.environ.get("WAYLAND_DISPLAY")) - ) + _has_display = sys.platform != "linux" or bool(os.environ.get("DISPLAY") or os.environ.get("WAYLAND_DISPLAY")) if not _has_display: _log.debug( "Skipping browser-open: no DISPLAY or WAYLAND_DISPLAY detected " @@ -306,23 +247,15 @@ def _is_serve_orphaned( ) -> bool: """True when the exact Desktop process that owns this backend is gone. - ``HERMES_PARENT_PID`` is the Electron Desktop PID, not necessarily this - Python process's immediate PPID. On Windows the venv ``hermes.exe`` launcher - introduces one or more shim processes, so comparing ``os.getppid()`` to the - Electron PID incorrectly treats a healthy backend as orphaned and exits 0. - - New Desktop versions also provide the owner's process-start marker. This - prevents a recycled PID from keeping an orphan alive. Older versions remain - compatible through the PID-only probe. Any inconclusive probe failure is - fail-safe: keep serving rather than killing a backend whose owner could not - be conclusively shown to be dead. + ``HERMES_PARENT_PID`` is the Electron PID, not necessarily our PPID (the + Windows ``hermes.exe`` launcher adds shims), so never compare getppid(). + The start marker (newer Desktops) defeats PID recycling; PID-only probing + stays for older ones. Any inconclusive failure keeps serving (fail-safe). """ try: if expected_start_marker is not None: probe = process_start_marker or _process_start_marker - return not _parent_start_markers_match( - probe(int(desktop_pid)), expected_start_marker - ) + return not _parent_start_markers_match(probe(int(desktop_pid)), expected_start_marker) if pid_exists is None: from gateway.status import _pid_exists @@ -338,11 +271,9 @@ def _is_serve_orphaned( def _start_parent_death_watchdog() -> None: """Exit when the exact desktop parent that spawned this backend dies. - The desktop passes its PID and, in newer versions, its process-start marker - plus a per-spawn nonce. The marker distinguishes a live owner from PID reuse; - the nonce makes partial/mixed-version identity plumbing fail safe. Legacy - Desktop versions that provide only ``HERMES_PARENT_PID`` retain PID-only - tracking. + Desktop passes its PID and, in newer versions, a start marker (defeats PID + reuse) plus a per-spawn nonce (makes mixed-version plumbing fail safe: + marker without nonce or vice versa disables the watchdog). """ raw_pid = os.environ.get("HERMES_PARENT_PID") start_marker = os.environ.get("HERMES_PARENT_START_MARKER") @@ -356,14 +287,9 @@ def _start_parent_death_watchdog() -> None: return has_marker = start_marker is not None - has_nonce = nonce is not None - if has_marker != has_nonce: + if has_marker != (nonce is not None): return - if has_marker and ( - not _valid_parent_start_marker(start_marker or "") - or not nonce - or nonce != nonce.strip() - ): + if has_marker and (not _valid_parent_start_marker(start_marker or "") or not nonce or nonce != nonce.strip()): return try: @@ -379,22 +305,12 @@ def _start_parent_death_watchdog() -> None: threading.Thread(target=_loop, daemon=True, name="serve-parent-watchdog").start() -# ── Port-conflict sentinel (#93608) ───────────────────────────────────────── -# When the requested port is already bound, uvicorn's ``bind_socket()`` -# catches the OSError itself and does ``logger.error(exc); sys.exit(1)`` — a -# bare ERROR line plus the same exit 1 as any real backend crash. The desktop -# spawn (and any script wrapping ``hermes serve``) cannot tell "port occupied" -# from "backend broken". So we probe the exact bind before handing the socket -# to uvicorn and, on conflict, emit ONE machine-readable stdout sentinel plus -# a human hint, then exit with a distinct code. -# -# 75 == BSD ``EX_TEMPFAIL`` (sysexits.h) — the codebase's existing convention -# for "transient environmental condition, not a code failure" (see -# gateway/restart.py and kanban_db.py's quota-wall sentinel). +# Port-conflict sentinel (#93608): uvicorn's bind_socket() turns EADDRINUSE into +# a bare ERROR + exit 1, indistinguishable from a crash for the Desktop spawn. +# We probe the exact bind first and emit ONE machine-readable stdout line plus a +# distinct exit code. 75 == BSD EX_TEMPFAIL, the codebase's "transient +# environmental condition" convention (gateway/restart.py, kanban_db.py). PORT_IN_USE_EXIT_CODE = 75 - -# One line, stable format, parsed by machines — mirrors the shape of the -# HERMES_BACKEND_READY sentinel (which is NOT changed by any of this). _PORT_IN_USE_SENTINEL = "BACKEND_PORT_IN_USE port={port}" @@ -402,19 +318,16 @@ def _is_addr_in_use_error(exc: OSError) -> bool: """True when ``exc`` is the platform's address-in-use bind failure.""" import errno - codes = {errno.EADDRINUSE, 98, 48, 10048} # POSIX, Linux, macOS, WinSock - if exc.errno in codes: - return True - return getattr(exc, "winerror", None) == 10048 # WSAEADDRINUSE + # POSIX, Linux, macOS, WinSock; WSAEADDRINUSE also surfaces as winerror. + return exc.errno in {errno.EADDRINUSE, 98, 48, 10048} or getattr(exc, "winerror", None) == 10048 def _port_bind_conflict(host: str, port: int) -> bool: """Probe whether binding ``host:port`` would fail with EADDRINUSE. - ``port == 0`` (ephemeral) can never conflict — the kernel picks a free - port — so the probe is skipped and ``--port 0`` behaves exactly as - before. Any probe error other than address-in-use returns ``False`` so - uvicorn surfaces it with its normal diagnostics (bad host, EACCES, …). + ``port == 0`` (ephemeral) never conflicts, so it is skipped. Any probe + error other than address-in-use returns ``False`` so uvicorn surfaces it + with its normal diagnostics (bad host, EACCES, …). """ if not port: return False @@ -426,21 +339,15 @@ def _port_bind_conflict(host: str, port: int) -> bool: except OSError: return False try: - import sys as _sys_mod - _exclusive = getattr(_socket, "SO_EXCLUSIVEADDRUSE", None) - if _sys_mod.platform == "win32" and _exclusive is not None: - # Windows: SO_REUSEADDR means "bind over anyone" — a probe (or - # uvicorn bind) with it SUCCEEDS on top of a live LISTEN socket, - # so it can never detect a conflict. SO_EXCLUSIVEADDRUSE makes - # the probe fail with WSAEADDRINUSE exactly when another socket - # holds the port (the reporter's 10048 shape in #93608). + if sys.platform == "win32" and _exclusive is not None: + # Windows SO_REUSEADDR binds over a live LISTEN socket and can never + # detect a conflict; SO_EXCLUSIVEADDRUSE fails with 10048 exactly + # when another socket holds the port (#93608). probe.setsockopt(_socket.SOL_SOCKET, _exclusive, 1) else: - # POSIX: match uvicorn's bind flags (uvicorn/config.py - # bind_socket) so the probe conflicts exactly when uvicorn's own - # bind would: SO_REUSEADDR lets TIME_WAIT remnants pass while a - # live LISTEN socket still fails. + # Match uvicorn's own bind flags so the probe conflicts exactly when + # its bind would: TIME_WAIT remnants pass, a live LISTEN fails. probe.setsockopt(_socket.SOL_SOCKET, _socket.SO_REUSEADDR, 1) probe.bind((host, port)) except OSError as exc: @@ -455,20 +362,11 @@ def _port_bind_conflict(host: str, port: int) -> bool: def _write_machine_sentinel_line(line: str) -> None: """Write a machine-parsed sentinel line to the REAL stdout (fd 1). - The serve startup path imports ``tui_gateway.server`` (flush-on-SIGTERM - handlers, #94724) which redirects ``sys.stdout`` to ``sys.stderr`` at - import time to keep stray prints off the JSON-RPC protocol stream. Any - machine-readable sentinel printed after that import via ``print()`` lands - on stderr — invisible to consumers that parse the child's stdout pipe - (the Desktop spawn, scripts). fd 1 is untouched by the Python-level - redirect, so write there. - - Best-effort by design: if fd 1 is unwritable (closed; invalid under - pythonw.exe), fall back to ``print()`` for human visibility only — the - redirected stream can't reach stdout-parsing consumers, and pythonw - Desktop spawns rely on ``_write_dashboard_ready_file()`` (the - HERMES_DESKTOP_READY_FILE channel) for port discovery instead. Never - raises: a sentinel-delivery failure must not kill a healthy serve. + ``tui_gateway.server`` redirects ``sys.stdout`` to stderr at import (#94724), + so a ``print()`` sentinel never reaches the Desktop's stdout pipe; fd 1 is + untouched. If fd 1 is unwritable (pythonw.exe) fall back to ``print()`` for + humans only — pythonw spawns discover the port via the ready file. Never + raises. """ try: os.write(1, (line + "\n").encode()) @@ -496,12 +394,10 @@ _DEFAULT_DASHBOARD_FORWARDED_ALLOW_IPS = ("127.0.0.1", "::1") def _dashboard_forwarded_allow_ips(dashboard_config: dict[str, Any]) -> list[str]: - """Return the bounded proxy addresses uvicorn may trust. + """Return the bounded proxy addresses uvicorn may trust: loopback plus valid config entries. - Uvicorn's default trusts loopback. Preserve that behavior and extend it - only with explicit IP addresses or CIDR networks from config. Invalid or - unbounded entries fail closed instead of turning arbitrary client-supplied - forwarding headers into request metadata. + Invalid or unbounded (/0, '*') ``dashboard.trusted_proxies`` entries fail + closed so client-supplied forwarding headers never become request metadata. """ configured = dashboard_config.get("trusted_proxies", []) if configured in (None, ""): diff --git a/hermes_cli/web_server_mcp.py b/hermes_cli/web_server_mcp.py index 47002fef0a..5308a35e1a 100644 --- a/hermes_cli/web_server_mcp.py +++ b/hermes_cli/web_server_mcp.py @@ -1,8 +1,9 @@ -"""MCP server dashboard helpers: create-payload normalisation, env redaction/summary, and the dashboard-driven MCP OAuth worker. +"""MCP dashboard helpers: create-payload normalisation, env redaction/summary, dashboard-driven MCP OAuth worker. -Split out of ``hermes_cli.web_server``; every externally used name is re-imported -there, so ``web_server.`` keeps resolving (and monkeypatching) as before. -Helpers that tests patch on ``web_server`` are reached lazily through it. +Split out of ``hermes_cli.web_server``, which re-imports every externally used +name so ``web_server.`` keeps resolving (and monkeypatching) as before. +Wraps the same config layer the CLI uses (hermes_cli.mcp_config); stdio ``env`` +secrets are redacted on read. """ import threading @@ -15,38 +16,14 @@ from hermes_cli.config import redact_key from hermes_cli.web_models import MCPServerCreate -# --------------------------------------------------------------------------- -# Automation Blueprints — parameterized automation blueprints. The dashboard renders the -# slot schema as a form; submitting instantiates a real cron job via the same -# create_job path. See cron/blueprint_catalog.py for the single source of truth. -# --------------------------------------------------------------------------- - - -# --------------------------------------------------------------------------- -# MCP server endpoints — list / add / remove / test. -# -# Wraps the same config data layer the CLI uses (hermes_cli.mcp_config), so -# servers managed here show up under `hermes mcp list` and vice versa. Secrets -# in stdio `env` blocks are redacted on read; the agent picks them up from -# config.yaml at session start exactly as with CLI-added servers. -# --------------------------------------------------------------------------- - - -def _normalize_mcp_server_create( - body: MCPServerCreate, -) -> tuple[str, Dict[str, Any], Optional[str]]: +def _normalize_mcp_server_create(body: MCPServerCreate) -> tuple[str, Dict[str, Any], Optional[str]]: """Validate a Dashboard MCP create request and build its safe config. - The returned config never contains the submitted Bearer token. Callers - persist the token with the shared Bearer helper only after they enter the - intended profile scope. Keeping this conversion shared makes the - standalone MCP page and the Profile Builder enforce the same - transport/auth contract. + The returned config never contains the Bearer token; callers persist it via + the shared Bearer helper once inside the intended profile scope. Shared by + the MCP page and the Profile Builder so both enforce one transport/auth contract. """ - from hermes_cli.mcp_config import ( - _bearer_auth_headers, - _strip_bearer_prefix, - ) + from hermes_cli.mcp_config import _bearer_auth_headers, _strip_bearer_prefix from hermes_cli.mcp_security import validate_mcp_server_entry name = (body.name or "").strip() @@ -56,11 +33,7 @@ def _normalize_mcp_server_create( url = (body.url or "").strip() command = (body.command or "").strip() auth = (body.auth or "none").strip().lower() - bearer_token = ( - body.bearer_token.get_secret_value() - if body.bearer_token is not None - else None - ) + bearer_token = body.bearer_token.get_secret_value() if body.bearer_token is not None else None if bool(url) == bool(command): raise ValueError("Provide exactly one of URL (HTTP/SSE) or command (stdio)") @@ -72,9 +45,7 @@ def _normalize_mcp_server_create( if body.args: raise ValueError("Arguments are only supported for stdio MCP servers") if body.env: - raise ValueError( - "Environment variables are only supported for stdio MCP servers" - ) + raise ValueError("Environment variables are only supported for stdio MCP servers") if auth == "header": normalized = _strip_bearer_prefix(bearer_token) if bearer_token else "" if not normalized or normalized.lower() == "bearer": @@ -88,9 +59,7 @@ def _normalize_mcp_server_create( server_config["auth"] = "oauth" else: if auth != "none" or body.bearer_token is not None: - raise ValueError( - "HTTP authentication is not supported for stdio MCP servers" - ) + raise ValueError("HTTP authentication is not supported for stdio MCP servers") server_config["command"] = command if body.args: server_config["args"] = list(body.args) @@ -118,9 +87,7 @@ def _mcp_server_summary(name: str, cfg: Dict[str, Any]) -> Dict[str, Any]: transport = "http" if cfg.get("url") else ("stdio" if cfg.get("command") else "unknown") auth = cfg.get("auth") headers = cfg.get("headers") or {} - if not auth and isinstance(headers, dict) and any( - str(key).lower() == "authorization" for key in headers - ): + if not auth and isinstance(headers, dict) and any(str(key).lower() == "authorization" for key in headers): auth = "header" return { "name": name, @@ -149,17 +116,9 @@ def _mcp_oauth_transaction(flow) -> threading.Lock: def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: """Run the normal MCP probe with dashboard redirect/callback handlers.""" - from hermes_cli.mcp_config import ( - _oauth_tokens_present, - _probe_single_server, - _save_mcp_server, - ) + from hermes_cli.mcp_config import _oauth_tokens_present, _probe_single_server, _save_mcp_server try: - from agent.secret_scope import ( - build_profile_secret_scope, - reset_secret_scope, - set_secret_scope, - ) + from agent.secret_scope import build_profile_secret_scope, reset_secret_scope, set_secret_scope from hermes_constants import reset_hermes_home_override, set_hermes_home_override from tools.mcp_dashboard_oauth import dashboard_oauth_flow from tools.mcp_oauth import HermesTokenStorage, force_interactive_oauth @@ -175,10 +134,7 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: backup = storage.snapshot() previous_entry = None try: - previous_entry = manager.remove( - flow.server_name, - hermes_home=flow.hermes_home, - ) + previous_entry = manager.remove(flow.server_name, hermes_home=flow.hermes_home) tools = _probe_single_server( flow.server_name, cfg, @@ -198,28 +154,20 @@ def _run_dashboard_mcp_oauth(flow, cfg: dict) -> None: reconnect_mcp_server(flow.server_name) except Exception: storage.restore(backup, only_if_absent=True) - manager.restore_entry( - flow.server_name, - previous_entry, - hermes_home=flow.hermes_home, - ) + manager.restore_entry(flow.server_name, previous_entry, hermes_home=flow.hermes_home) raise finally: reset_secret_scope(secret_token) reset_hermes_home_override(home_token) except Exception as exc: msg = str(exc) - # Providers that gate RFC 7591 registration to pre-approved clients - # (Figma's MCP catalog, etc.) 403 the register call before any - # authorization URL exists — surface what's actually happening - # instead of a bare "403 Forbidden". + # Providers gating RFC 7591 registration to pre-approved clients 403 the + # register call before any auth URL exists; say so, not "403 Forbidden". try: from tools.mcp_oauth import humanize_oauth_registration_error humanized = humanize_oauth_registration_error( - flow.server_name, - exc, - server_url=cfg.get("url") if isinstance(cfg, dict) else None, + flow.server_name, exc, server_url=cfg.get("url") if isinstance(cfg, dict) else None ) if humanized: msg = humanized