From c6ca11618a87c6b12e9a4025d339eb905a03ac8c Mon Sep 17 00:00:00 2001 From: ethernet Date: Mon, 11 May 2026 17:04:34 -0400 Subject: [PATCH] refactor(tui): simplify TUI build logic, remove stale staleness checks The old mtime-tracking staleness machinery (_tui_build_needed, _hermes_ink_bundle_stale, _find_bundled_tui) tried to avoid rebuilding by comparing source timestamps to dist/entry.js. This was fragile and added ~100 lines of code. Replace with three clear paths: 1. HERMES_TUI_DIR set (prebuilt/nix): just node dist/entry.js, no build 2. --dev mode: tsx src/entry.tsx, no build, hot reload 3. Normal: always npm run build (esbuild is ~1s, correctness > caching) Also error when HERMES_TUI_DIR is set with --dev (footgun: prebuilt bundle has no source code to hot-reload). --- hermes_cli/main.py | 136 ++++++----------------- tests/hermes_cli/test_tui_npm_install.py | 35 ------ 2 files changed, 33 insertions(+), 138 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 15246a88ab5..009af00b6ee 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -967,66 +967,6 @@ def _tui_need_npm_install(root: Path) -> bool: return False -def _find_bundled_tui(tui_dir: Path) -> Optional[Path]: - """Directory whose dist/entry.js we should run: HERMES_TUI_DIR first, else repo ui-tui.""" - env = os.environ.get("HERMES_TUI_DIR") - if env: - p = Path(env) - if (p / "dist" / "entry.js").exists() and not _tui_need_npm_install(p): - return p - if (tui_dir / "dist" / "entry.js").exists() and not _tui_need_npm_install(tui_dir): - return tui_dir - return None - - -def _tui_build_needed(tui_dir: Path) -> bool: - # esbuild bundles @hermes/ink + source directly into dist/entry.js; - # the old ink-bundle.js check only fires when entry.js hasn't been - # produced yet (a dev checkout that hasn't been built at all). Once - # entry.js exists, the mtime walk below covers all source trees. - entry = tui_dir / "dist" / "entry.js" - if not entry.exists(): - # Nothing built yet — signal that a build is needed. - return True - dist_m = entry.stat().st_mtime - skip = frozenset({"node_modules", "dist"}) - for dirpath, dirnames, filenames in os.walk(tui_dir, topdown=True): - dirnames[:] = [d for d in dirnames if d not in skip] - for fn in filenames: - if fn.endswith((".ts", ".tsx")): - if os.path.getmtime(os.path.join(dirpath, fn)) > dist_m: - return True - for meta in ( - "package.json", - "package-lock.json", - "tsconfig.json", - "tsconfig.build.json", - ): - mp = tui_dir / meta - if mp.exists() and mp.stat().st_mtime > dist_m: - return True - return False - - -def _hermes_ink_bundle_stale(tui_dir: Path) -> bool: - ink_root = tui_dir / "packages" / "hermes-ink" - bundle = ink_root / "dist" / "ink-bundle.js" - if not bundle.exists(): - return True - bm = bundle.stat().st_mtime - skip = frozenset({"node_modules", "dist"}) - for dirpath, dirnames, filenames in os.walk(ink_root, topdown=True): - dirnames[:] = [d for d in dirnames if d not in skip] - for fn in filenames: - if fn.endswith((".ts", ".tsx")): - if os.path.getmtime(os.path.join(dirpath, fn)) > bm: - return True - mp = ink_root / "package.json" - if mp.exists() and mp.stat().st_mtime > bm: - return True - return False - - def _ensure_tui_node() -> None: """Make sure `node` + `npm` are on PATH for the TUI. @@ -1085,7 +1025,7 @@ def _ensure_tui_node() -> None: def _make_tui_argv(tui_dir: Path, tui_dev: bool) -> tuple[list[str], Path]: - """TUI: --dev → tsx src; else node dist (HERMES_TUI_DIR or ui-tui, build when stale).""" + """TUI: --dev → tsx src; else node dist (HERMES_TUI_DIR prebuilt or esbuild).""" _ensure_tui_node() def _node_bin(bin: str) -> str: @@ -1099,23 +1039,31 @@ def _make_tui_argv(tui_dir: Path, tui_dev: bool) -> tuple[list[str], Path]: sys.exit(1) return path - # pre-built dist + node_modules (nix / full HERMES_TUI_DIR) skips npm. + # Footgun: --dev against a prebuilt bundle that has no source/node_modules. + ext_dir = os.environ.get("HERMES_TUI_DIR") + if tui_dev and ext_dir: + print( + f"Error: --dev is incompatible with HERMES_TUI_DIR={ext_dir}\n" + f"The prebuilt TUI has no source code to hot-reload.\n" + f"Unset HERMES_TUI_DIR (e.g. `unset HERMES_TUI_DIR`) to use --dev from a checkout.", + file=sys.stderr, + ) + sys.exit(1) + + # 1. Prebuilt bundle (nix / packaged release): just run it. if not tui_dev: - ext_dir = os.environ.get("HERMES_TUI_DIR") if ext_dir: p = Path(ext_dir) - if (p / "dist" / "entry.js").exists() and not _tui_need_npm_install(p): + if (p / "dist" / "entry.js").is_file(): node = _node_bin("node") return [node, str(p / "dist" / "entry.js")], p - npm = _node_bin("npm") + # 2. Normal flow: npm install if needed, always esbuild, then node dist/entry.js. + # --dev flow: npm install if needed, then tsx src/entry.tsx (no build). if _tui_need_npm_install(tui_dir): + npm = _node_bin("npm") if not os.environ.get("HERMES_QUIET"): print("Installing TUI dependencies…") - # Capture stdout as well as stderr — some npm errors (notably EACCES on a - # root-owned node_modules in containers) are emitted on stdout, and a - # bare "npm install failed." with no preview defeats debugging. We keep - # the failure-only print path so a successful install stays silent. result = subprocess.run( [npm, "install", "--silent", "--no-fund", "--no-audit", "--progress=false"], cwd=str(tui_dir), @@ -1133,47 +1081,30 @@ def _make_tui_argv(tui_dir: Path, tui_dev: bool) -> tuple[list[str], Path]: sys.exit(1) if tui_dev: - if _hermes_ink_bundle_stale(tui_dir): - result = subprocess.run( - [npm, "run", "build", "--prefix", "packages/hermes-ink"], - cwd=str(tui_dir), - capture_output=True, - text=True, - ) - if result.returncode != 0: - combined = f"{result.stdout or ''}{result.stderr or ''}".strip() - preview = "\n".join(combined.splitlines()[-30:]) - print("@hermes/ink build failed.") - if preview: - print(preview) - sys.exit(1) tsx = tui_dir / "node_modules" / ".bin" / "tsx" if tsx.exists(): return [str(tsx), "src/entry.tsx"], tui_dir + npm = _node_bin("npm") return [npm, "start"], tui_dir - if _tui_build_needed(tui_dir): - result = subprocess.run( - [npm, "run", "build"], - cwd=str(tui_dir), - capture_output=True, - text=True, - ) - if result.returncode != 0: - combined = f"{result.stdout or ''}{result.stderr or ''}".strip() - preview = "\n".join(combined.splitlines()[-30:]) - print("TUI build failed.") - if preview: - print(preview) - sys.exit(1) - - root = _find_bundled_tui(tui_dir) - if not root: - print("TUI build did not produce dist/entry.js") + # Always rebuild — esbuild is fast and this avoids staleness-edge-case bugs. + npm = _node_bin("npm") + result = subprocess.run( + [npm, "run", "build"], + cwd=str(tui_dir), + capture_output=True, + text=True, + ) + if result.returncode != 0: + combined = f"{result.stdout or ''}{result.stderr or ''}".strip() + preview = "\n".join(combined.splitlines()[-30:]) + print("TUI build failed.") + if preview: + print(preview) sys.exit(1) node = _node_bin("node") - return [node, str(root / "dist" / "entry.js")], root + return [node, str(tui_dir / "dist" / "entry.js")], tui_dir def _normalize_tui_toolsets(toolsets: object) -> list[str]: @@ -5503,7 +5434,6 @@ def _gateway_prompt(prompt_text: str, default: str = "", timeout: float = 300.0) def _web_ui_build_needed(web_dir: Path) -> bool: """Return True if the web UI dist is missing or stale. - Mirrors the staleness logic used by ``_tui_build_needed()`` for the TUI. The Vite build outputs to ``hermes_cli/web_dist/`` (per vite.config.ts outDir: "../hermes_cli/web_dist"), NOT to ``web/dist/``. Uses the Vite manifest as the sentinel because it is written last and therefore has the diff --git a/tests/hermes_cli/test_tui_npm_install.py b/tests/hermes_cli/test_tui_npm_install.py index f17ed5a0744..efad281565b 100644 --- a/tests/hermes_cli/test_tui_npm_install.py +++ b/tests/hermes_cli/test_tui_npm_install.py @@ -25,12 +25,6 @@ def _touch_tui_entry(root: Path) -> None: entry.write_text("console.log('tui')") -def _touch_ink_bundle(root: Path) -> None: - bundle = root / "packages" / "hermes-ink" / "dist" / "ink-bundle.js" - bundle.parent.mkdir(parents=True, exist_ok=True) - bundle.write_text("export {}") - - def test_need_install_when_ink_missing(tmp_path: Path, main_mod) -> None: (tmp_path / "package-lock.json").write_text("{}") assert main_mod._tui_need_npm_install(tmp_path) is True @@ -126,32 +120,3 @@ def test_no_install_prebuilt_bundle_mode(tmp_path: Path, main_mod) -> None: """dist/entry.js present and no package-lock.json → prebuilt bundle, skip npm install.""" _touch_tui_entry(tmp_path) assert main_mod._tui_need_npm_install(tmp_path) is False - - -def test_build_needed_when_source_newer_than_entry(tmp_path: Path, main_mod) -> None: - _touch_tui_entry(tmp_path) - _touch_ink(tmp_path) - src = tmp_path / "src" / "entry.tsx" - src.parent.mkdir(parents=True, exist_ok=True) - src.write_text("console.log('newer')") - os.utime(src, (200, 200)) - os.utime(tmp_path / "dist" / "entry.js", (100, 100)) - - assert main_mod._tui_need_npm_install(tmp_path) is False - assert main_mod._tui_build_needed(tmp_path) is True - - -def test_build_not_needed_when_entry_exists_and_sources_unchanged(tmp_path: Path, main_mod) -> None: - _touch_tui_entry(tmp_path) - _touch_ink(tmp_path) - - assert main_mod._tui_need_npm_install(tmp_path) is False - assert main_mod._tui_build_needed(tmp_path) is False - - -def test_build_not_needed_when_entry_and_ink_bundle_present(tmp_path: Path, main_mod) -> None: - _touch_tui_entry(tmp_path) - _touch_ink(tmp_path) - _touch_ink_bundle(tmp_path) - - assert main_mod._tui_build_needed(tmp_path) is False