From 2094265047efa307cef07495c4a4a19f659aeecb Mon Sep 17 00:00:00 2001 From: emozilla Date: Wed, 16 Sep 2026 23:13:20 -0400 Subject: [PATCH] fix(desktop): preserve source freshness and console diagnostics Remove metadata-only source caching so mapped edits and transient read failures cannot leave stale rebuild decisions. Keep directory pruning, lazy npm lookup, renderer manifests and the COM scheduler query. Separate stdout and stderr logging, decode bounded logical lines before redaction, and route desktop records to the GUI log. Add real-child, mapped-write and temporary-read-lock regressions. Verified by independent review, targeted Windows/Linux suites, renderer tests and packaged Windows manual checks. Existing Windows logging failures reproduce on the parent; no end-to-end startup percentage is claimed for this corrected version. --- hermes_cli/desktop_console.py | 45 +++++-- hermes_cli/desktop_source_cache.py | 111 ------------------ hermes_cli/main_desktop.py | 4 +- hermes_logging.py | 2 +- tests/hermes_cli/test_desktop_console.py | 60 ++++++++++ tests/hermes_cli/test_desktop_startup_cost.py | 51 ++++++-- 6 files changed, 134 insertions(+), 139 deletions(-) delete mode 100644 hermes_cli/desktop_source_cache.py diff --git a/hermes_cli/desktop_console.py b/hermes_cli/desktop_console.py index 5af13ea526..b311b4863e 100644 --- a/hermes_cli/desktop_console.py +++ b/hermes_cli/desktop_console.py @@ -4,6 +4,7 @@ import logging import os import sys import threading +import time from contextlib import contextmanager logger = logging.getLogger("hermes_cli.desktop") @@ -31,19 +32,39 @@ def desktop_console_output(*, source_mode: bool): yield {} return - import subprocess - - read_fd, write_fd = os.pipe() - - def drain(): + def drain(read_fd, level): with os.fdopen(read_fd, "rb") as stream: - while data := stream.readline(8192): - logger.info("[desktop] %s", data.decode("utf-8", errors="replace").rstrip()) + # Decode/redact complete records, not arbitrary pipe-read fragments. + # Discard an oversized line in full: even its prefix may be a secret. + limit = 128 * 1024 + while data := stream.readline(limit + 1): + if len(data) > limit: + while data and not data.endswith(b"\n"): + data = stream.readline(8192) + logger.log(level, "[desktop] [oversized line omitted]") + else: + logger.log(level, "[desktop] %s", data.decode("utf-8", errors="replace").rstrip()) - reader = threading.Thread(target=drain, name="desktop-console", daemon=True) - reader.start() + streams = {} + readers = [] try: - yield {"stdout": write_fd, "stderr": subprocess.STDOUT} + # Keep diagnostics visible even when ordinary INFO output is disabled. + for name, level in (("stdout", logging.INFO), ("stderr", logging.ERROR)): + read_fd, write_fd = os.pipe() + streams[name] = write_fd + reader = threading.Thread( + target=drain, args=(read_fd, level), name=f"desktop-console-{name}", daemon=True, + ) + try: + reader.start() + except BaseException: + os.close(read_fd) + raise + readers.append(reader) + yield streams finally: - os.close(write_fd) - reader.join(timeout=1) + for write_fd in streams.values(): + os.close(write_fd) + deadline = time.monotonic() + 1 + for reader in readers: + reader.join(timeout=max(0, deadline - time.monotonic())) diff --git a/hermes_cli/desktop_source_cache.py b/hermes_cli/desktop_source_cache.py deleted file mode 100644 index 0413487f68..0000000000 --- a/hermes_cli/desktop_source_cache.py +++ /dev/null @@ -1,111 +0,0 @@ -"""Reuse the desktop content hash while all source metadata is unchanged. - -This is a build-freshness cache, not an integrity/security check. Normal launches -must not read thousands of source/test files (and trigger on-access AV scans). -Changed metadata always falls back to the existing content hash, so checkout -mtime churn alone still does not force a rebuild. -""" - -import ctypes -import contextlib -import hashlib -import json -import os -import tempfile -from functools import lru_cache -from pathlib import Path -from typing import Callable - - -@lru_cache(maxsize=1) -def _windows_file_api(): - from ctypes import wintypes - - class FileBasicInfo(ctypes.Structure): - _fields_ = [(name, ctypes.c_longlong) for name in ( - "creation", "access", "write", "change" - )] + [("attributes", wintypes.DWORD)] - - kernel = ctypes.WinDLL("kernel32", use_last_error=True) - kernel.CreateFileW.argtypes = [wintypes.LPCWSTR, wintypes.DWORD, wintypes.DWORD, - ctypes.c_void_p, wintypes.DWORD, wintypes.DWORD, wintypes.HANDLE] - kernel.CreateFileW.restype = wintypes.HANDLE - kernel.GetFileInformationByHandleEx.argtypes = [wintypes.HANDLE, ctypes.c_int, - ctypes.c_void_p, wintypes.DWORD] - kernel.GetFileInformationByHandleEx.restype = wintypes.BOOL - kernel.CloseHandle.argtypes = [wintypes.HANDLE] - kernel.CloseHandle.restype = wintypes.BOOL - return kernel, FileBasicInfo - - -def _change_time(path: Path, st: os.stat_result) -> int: - if os.name != "nt": - return st.st_ctime_ns - # Windows stat().st_ctime is CREATION time on supported Python versions. - # FILE_BASIC_INFO.ChangeTime also catches same-size edits with restored mtime. - # Request metadata only: never FILE_READ_DATA or a read of source contents. - kernel, info_type = _windows_file_api() - handle = kernel.CreateFileW(str(path), 0x80, 7, None, 3, 0x02000000, None) - if handle == ctypes.c_void_p(-1).value: - raise ctypes.WinError(ctypes.get_last_error()) - try: - info = info_type() - if not kernel.GetFileInformationByHandleEx(handle, 0, ctypes.byref(info), ctypes.sizeof(info)): - raise ctypes.WinError(ctypes.get_last_error()) - return info.change - finally: - kernel.CloseHandle(handle) - - -def _source_metadata(project_root: Path, tree_dir: Path) -> str: - from hermes_cli.main_web_build import _source_tree_files - - digest = hashlib.sha256() - for path in _source_tree_files(project_root, tree_dir): - st = path.stat() - record = (str(path.relative_to(project_root)), st.st_dev, st.st_ino, - st.st_size, st.st_mtime_ns, _change_time(path, st)) - digest.update(json.dumps(record, ensure_ascii=True).encode()) - digest.update(b"\0") - return digest.hexdigest() - - -def cached_desktop_source_hash(project_root: Path, compute: Callable[[], str]) -> str: - from hermes_constants import get_hermes_home - - tree_dir = project_root / "apps" / "desktop" - cache_file = get_hermes_home() / "desktop-source-cache.json" - scope = str(project_root.resolve()) - try: - before = _source_metadata(project_root, tree_dir) - except OSError: - return compute() # Metadata unavailable: keep the full content check. - try: - cache = json.loads(cache_file.read_text(encoding="utf-8")) - if (isinstance(cache, dict) and cache.get("version") == 1 - and cache.get("root") == scope and cache.get("metadata") == before - and isinstance(cache.get("contentHash"), str) and len(cache["contentHash"]) == 64): - return cache["contentHash"] - except (OSError, ValueError): - pass - - content_hash = compute() - temporary = None - try: - # Do not cache a digest if an editor/update changed inputs while we read. - if before != _source_metadata(project_root, tree_dir): - return content_hash - cache_file.parent.mkdir(parents=True, exist_ok=True) - with tempfile.NamedTemporaryFile(mode="w", encoding="utf-8", dir=cache_file.parent, - prefix=".desktop-source-", delete=False) as stream: - temporary = Path(stream.name) - json.dump({"version": 1, "root": scope, "metadata": before, - "contentHash": content_hash}, stream) - temporary.replace(cache_file) - except OSError: - pass # Read-only cache locations must not prevent building/launching. - finally: - if temporary is not None: - with contextlib.suppress(OSError): - temporary.unlink(missing_ok=True) - return content_hash diff --git a/hermes_cli/main_desktop.py b/hermes_cli/main_desktop.py index 670e24cc70..8c3a25b89b 100644 --- a/hermes_cli/main_desktop.py +++ b/hermes_cli/main_desktop.py @@ -37,9 +37,7 @@ def _desktop_dist_exists(desktop_dir: Path) -> bool: def _compute_desktop_content_hash(project_root: Path) -> str: """SHA-256 of ``apps/desktop/`` (minus .gitignore matches) plus root workspace config.""" - from hermes_cli.desktop_source_cache import cached_desktop_source_hash - return cached_desktop_source_hash( - project_root, lambda: _hash_source_tree(project_root, project_root / "apps" / "desktop")) + return _hash_source_tree(project_root, project_root / "apps" / "desktop") def _desktop_stamp_path() -> Path: diff --git a/hermes_logging.py b/hermes_logging.py index a2f786f9bb..9142759181 100644 --- a/hermes_logging.py +++ b/hermes_logging.py @@ -165,7 +165,7 @@ COMPONENT_PREFIXES = { "tools": ("tools",), "cli": ("hermes_cli", "cli"), "cron": ("cron",), - "gui": ("hermes_cli.web_server", "hermes_cli.pty_bridge", "tui_gateway", "uvicorn"), + "gui": ("hermes_cli.web_server", "hermes_cli.pty_bridge", "hermes_cli.desktop", "tui_gateway", "uvicorn"), } diff --git a/tests/hermes_cli/test_desktop_console.py b/tests/hermes_cli/test_desktop_console.py index 0bfa99c1a0..e3d48794df 100644 --- a/tests/hermes_cli/test_desktop_console.py +++ b/tests/hermes_cli/test_desktop_console.py @@ -30,6 +30,66 @@ def test_packaged_console_output_drains_both_streams(caplog, monkeypatch): assert any("diagnostic" in message for message in messages) +@pytest.mark.windows_only +@pytest.mark.parametrize("level", [logging.WARNING, logging.ERROR]) +def test_stderr_survives_logging_threshold(caplog, level): + caplog.set_level(level, logger="hermes_cli.desktop") + with desktop_console_output(source_mode=False) as streams: + result = subprocess.run( + [sys.executable, "-c", "import sys; print('ordinary-output'); " + "sys.stderr.write('fatal-diagnostic\\n'); sys.exit(7)"], + timeout=15, check=False, **streams, + ) + assert result.returncode == 7 + messages = [record.getMessage() for record in caplog.records] + assert any("fatal-diagnostic" in message for message in messages) + assert not any("ordinary-output" in message for message in messages) + + +@pytest.mark.windows_only +@pytest.mark.parametrize("stream", ["stdout", "stderr"]) +def test_complete_records_reach_redacted_logs(tmp_path, monkeypatch, caplog, stream): + import hermes_logging + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + hermes_logging._reset_queued_handlers() + caplog.set_level(logging.INFO) + caplog.set_level(logging.INFO, logger="hermes_cli.desktop") + secret = "synthetic-boundary-credential" + payload = ( + b" " * (8192 - len(b"API_KEY=")) + b"API_KEY=" + secret.encode() + b"\n" + + b"u" * 8191 + "\u20ac\n".encode() + + b"oversized-start API_KEY=" + secret.encode() + b"z" * 262144 + + b"oversized-end\nrecovered\ntail " + "\u20ac".encode() + ) + payload_path = tmp_path / "child-output.bin" + payload_path.write_bytes(payload) + try: + log_dir = hermes_logging.setup_logging(hermes_home=tmp_path, log_level="INFO", mode="gui") + with desktop_console_output(source_mode=False) as streams: + result = subprocess.run( + [sys.executable, "-c", "import pathlib, sys; " + "data = pathlib.Path(sys.argv[1]).read_bytes(); " + "out = getattr(sys, sys.argv[2]).buffer; " + "[(out.write(data[i:i+997]), out.flush()) for i in range(0, len(data), 997)]", + str(payload_path), stream], + timeout=15, check=False, **streams, + ) + assert result.returncode == 0 + hermes_logging.flush_log_queue() + text = (log_dir / "agent.log").read_text(encoding="utf-8") + assert secret not in text + assert "u" * 8191 + "\u20ac" in text + assert "\ufffd" not in text + assert "oversized-start" not in text and "oversized-end" not in text + assert "recovered" in text and "tail \u20ac" in text + assert "omitted" in text.lower() + gui_text = (log_dir / "gui.log").read_text(encoding="utf-8") + assert "recovered" in gui_text and secret not in gui_text + finally: + hermes_logging._reset_queued_handlers() + + def test_source_launch_keeps_interactive_streams(): with desktop_console_output(source_mode=True) as streams: assert streams == {} diff --git a/tests/hermes_cli/test_desktop_startup_cost.py b/tests/hermes_cli/test_desktop_startup_cost.py index 5f038e83b3..8a27553b50 100644 --- a/tests/hermes_cli/test_desktop_startup_cost.py +++ b/tests/hermes_cli/test_desktop_startup_cost.py @@ -1,7 +1,6 @@ """Startup regressions exercised against real source trees on the current host.""" import argparse -import builtins import os import subprocess from pathlib import Path @@ -25,28 +24,21 @@ def _tree(tmp_path): return root, app, source -def test_unchanged_desktop_hash_does_not_reopen_sources(tmp_path): - root, app, source = _tree(tmp_path) +def test_desktop_hash_prunes_ignored_build_directories(tmp_path): + root, app, _ = _tree(tmp_path) ignored = app / "dist" ignored.mkdir() (ignored / "bundle.js").write_text("built output") expected = desktop._compute_desktop_content_hash(root) - original_open = builtins.open original_scandir = os.scandir - reads = [] - - def track(file, *args, **kwargs): - if isinstance(file, (str, os.PathLike)) and Path(file) == source: - reads.append(file) - return original_open(file, *args, **kwargs) def scan(directory): assert Path(directory) != ignored, "ignored build output must be pruned before traversal" return original_scandir(directory) - with patch("builtins.open", side_effect=track), patch("os.scandir", side_effect=scan): + (ignored / "bundle.js").write_text("different build output") + with patch("os.scandir", side_effect=scan): assert desktop._compute_desktop_content_hash(root) == expected - assert reads == [], "launch should use source metadata, not read every source file again" def test_desktop_hash_invalidates_for_edits_even_with_restored_mtime(tmp_path): @@ -67,6 +59,41 @@ def test_desktop_hash_invalidates_for_edits_even_with_restored_mtime(tmp_path): assert desktop._compute_desktop_content_hash(root) != original +@pytest.mark.windows_only +def test_desktop_hash_detects_memory_mapped_edits(tmp_path): + import mmap + + root, _, source = _tree(tmp_path) + original = desktop._compute_desktop_content_hash(root) + with source.open("r+b") as stream: + with mmap.mmap(stream.fileno(), 0, access=mmap.ACCESS_WRITE) as view: + view[-1:] = b"2" + view.flush() + assert source.read_bytes().endswith(b"2") + assert desktop._compute_desktop_content_hash(root) != original + + +@pytest.mark.windows_only +def test_desktop_hash_recovers_after_a_temporary_read_lock(tmp_path): + import msvcrt + + root, _, source = _tree(tmp_path) + source.write_bytes(b"") + original = desktop._compute_desktop_content_hash(root) + payload = b"export const x = 2" + source.write_bytes(payload) + with source.open("r+b") as locked: + msvcrt.locking(locked.fileno(), msvcrt.LK_NBLCK, len(payload)) + try: + with pytest.raises(OSError): + source.read_bytes() + desktop._compute_desktop_content_hash(root) + finally: + locked.seek(0) + msvcrt.locking(locked.fileno(), msvcrt.LK_UNLCK, len(payload)) + assert desktop._compute_desktop_content_hash(root) != original + + def test_current_packaged_launch_does_not_require_npm(tmp_path, monkeypatch): root, app, _ = _tree(tmp_path) monkeypatch.setattr(cli_main, "PROJECT_ROOT", root)