From 363d1aefa449e74c6fe1a92a14f76c59002fe69d Mon Sep 17 00:00:00 2001 From: "Gerardo Camorlinga Jr." Date: Tue, 21 Jul 2026 16:47:42 -0500 Subject: [PATCH 01/19] fix(update): recover web UI build when tsc/vite missing after npm install hermes update could report a successful workspace npm ci while devDependencies were omitted (production/omit-dev env leakage), leaving tsc/vite unlinked. The subsequent web UI build then failed with `tsc: not found` and fell back to a stale dashboard dist. Force npm_config_include=dev on install, verify toolchain shims after workspace install (and refuse to hash-cache a half tree), repair/reinstall when node_modules exists without tsc/vite, prepend workspace .bin dirs to PATH, and retry the build after a tsc/vite-not-found failure. --- hermes_cli/main.py | 158 ++++++++++++++++++++++++-- tests/hermes_cli/test_web_ui_build.py | 96 ++++++++++++++++ 2 files changed, 243 insertions(+), 11 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 3f3d5680647d..38bf0a96628d 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -5195,6 +5195,15 @@ def _run_npm_install_deterministic( # --silent/capture_output). It no-ops when CI is set — same as the TUI # install path and nix/lib.nix npm ci hooks. run_env = {**os.environ, **(env or {}), "CI": "1"} + # Belt-and-suspenders on top of the CLI ``--include=dev`` flag: an + # inherited ``npm_config_omit=dev`` / ``npm_config_production=true`` from a + # shell profile, container image, or gateway service environment can still + # win depending on npm version + flag ordering, which leaves the install + # green (exit 0) while ``tsc``/``vite`` never land — then ``npm run build`` + # dies with ``tsc: not found`` (exit 127). Force the config-level knobs. + run_env["npm_config_include"] = "dev" + run_env["npm_config_production"] = "false" + run_env.pop("npm_config_omit", None) lockfile = cwd / "package-lock.json" if lockfile.exists(): @@ -5226,6 +5235,49 @@ def _run_npm_install_deterministic( ) +def _node_modules_bin_dir(root: Path) -> Path: + """Return ``root/node_modules/.bin`` (may not exist yet).""" + return root / "node_modules" / ".bin" + + +def _npm_bin_exists(bin_dir: Path, name: str) -> bool: + """True when an npm bin shim for *name* exists (POSIX or Windows).""" + return any( + (bin_dir / candidate).exists() + for candidate in (name, f"{name}.cmd", f"{name}.ps1", f"{name}.exe") + ) + + +def _web_build_toolchain_ready(project_root: Path) -> bool: + """Return True when the web UI build toolchain shims are present. + + ``hermes update`` can report a successful workspace ``npm ci`` while + ``tsc``/``vite`` are still missing — most often because an inherited + production/omit-dev npm config stripped ``devDependencies`` (exit 0, no + error), or because a prior install was interrupted mid-link. The web + build then fails with ``tsc: not found``. Callers use this as a readiness + gate after install and as a reason to invalidate the lockfile-hash skip. + """ + if not (project_root / "web" / "package.json").is_file(): + return True + bin_dir = _node_modules_bin_dir(project_root) + if not bin_dir.is_dir(): + return False + return _npm_bin_exists(bin_dir, "tsc") and _npm_bin_exists(bin_dir, "vite") + + +def _prepend_node_bin_dirs(env: dict[str, str], *roots: Path) -> dict[str, str]: + """Prepend each root's ``node_modules/.bin`` onto PATH (existing dirs only).""" + merged = dict(env) + parts = [p for p in merged.get("PATH", "").split(os.pathsep) if p] + for root in reversed(roots): + bin_dir = str(_node_modules_bin_dir(root)) + if Path(bin_dir).is_dir() and bin_dir not in parts: + parts.insert(0, bin_dir) + merged["PATH"] = os.pathsep.join(parts) + return merged + + def _build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: """Build the web UI frontend if npm is available, serializing across processes. @@ -5333,12 +5385,26 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: npm_workspace_args: tuple[str, ...] = () if npm_cwd == web_dir else ("--workspace", "web") if _is_termux_startup_environment(): npm_cwd, npm_workspace_args = _termux_workspace_install_context(web_dir) - r1 = _run_npm_install_deterministic( - npm, - npm_cwd, - extra_args=(*npm_workspace_args, "--silent"), - env=build_env, - ) + + def _install_web_deps(*, silent: bool) -> "subprocess.CompletedProcess": + extra = (*npm_workspace_args,) + if silent: + extra = (*extra, "--silent") + return _run_npm_install_deterministic( + npm, + npm_cwd, + extra_args=extra, + env=build_env, + ) + + def _refresh_build_path() -> dict[str, str]: + # npm lifecycle scripts normally inject node_modules/.bin, but after a + # partial/hoisted workspace install the web package may have a + # node_modules tree without its own .bin while tsc/vite live at the + # workspace root. Prepending both keeps `tsc -b && vite build` resolvable. + return _prepend_node_bin_dirs(build_env, web_dir, npm_cwd) + + r1 = _install_web_deps(silent=True) if r1.returncode != 0: _say( f" {'✗' if fatal else '⚠'} Web UI npm install failed" @@ -5348,6 +5414,49 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: if fatal: _say(" Run manually: npm install --workspace web && npm run build -w web") return False + + # Install can exit 0 while still omitting devDependencies (production/omit + # config races). Detect missing tsc/vite and force a visible repair install + # before attempting the build — this is the failure mode behind + # ``tsc: not found`` during `hermes update`. + project_root_for_tools = ( + web_dir.parent.parent if web_dir.parent.name == "apps" else web_dir.parent + ) + # Only repair when an install produced a node_modules tree but left the + # toolchain incomplete. Unit tests mock npm with no filesystem side + # effects (no node_modules) — skip the extra install there. + if ( + (project_root_for_tools / "node_modules").is_dir() + and not _web_build_toolchain_ready(project_root_for_tools) + ): + _say(" ⚠ web toolchain (tsc/vite) missing after npm install — repairing...") + r_repair = _install_web_deps(silent=False) + if r_repair.returncode != 0: + _say( + f" {'✗' if fatal else '⚠'} Web UI toolchain repair failed" + + ("" if fatal else " (hermes web will not be available)") + ) + _relay(r_repair) + if fatal: + _say( + " Run manually: npm install --workspace web --include=dev " + "&& npm run build -w web" + ) + return False + if not _web_build_toolchain_ready(project_root_for_tools): + _say( + f" {'✗' if fatal else '⚠'} tsc/vite still missing after repair" + + ("" if fatal else " (hermes web will not be available)") + ) + if fatal: + _say( + " Run manually: npm install --workspace web --include=dev " + "&& npm run build -w web" + ) + return not fatal + + build_env = _refresh_build_path() + # First attempt — stream output via idle-timeout helper (issue #33788). # capture_output=True on a long Vite build looks identical to a hang; # users react by rebooting, which leaves the editable install in a @@ -5355,11 +5464,20 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: # recoverable (the stale-dist fallback below handles the kill path). r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: - # Retry once after a short delay — covers boot-time races on Windows - # (antivirus scanning Node.js binaries, npm cache not ready, transient - # I/O when launched via Scheduled Task at logon). See issue #23817. - _time.sleep(3) - r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) + build_out = (r2.stdout or "") + (r2.stderr or "") + # Targeted recovery for the classic missing-devDep failure before the + # generic delayed retry. Re-install (non-silent) + refresh PATH. + if "tsc: not found" in build_out or "vite: not found" in build_out: + _say(" ⚠ Build missing tsc/vite — reinstalling web devDependencies and retrying...") + _install_web_deps(silent=False) + build_env = _refresh_build_path() + r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) + if r2.returncode != 0: + # Retry once after a short delay — covers boot-time races on Windows + # (antivirus scanning Node.js binaries, npm cache not ready, transient + # I/O when launched via Scheduled Task at logon). See issue #23817. + _time.sleep(3) + r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: # _run_with_idle_timeout merges stderr into stdout; older callers @@ -10140,6 +10258,12 @@ def _npm_lockfile_changed(hermes_root: Path) -> bool: # node_modules means the cache was recorded by another checkout. if not (PROJECT_ROOT / "node_modules").is_dir(): return True + # A matching lockfile hash with a half-installed tree (e.g. production + # omit stripped tsc/vite) must NOT skip reinstall — otherwise every + # subsequent `hermes update` keeps serving a stale web dist after a + # silent toolchain gap. + if not _web_build_toolchain_ready(PROJECT_ROOT): + return True try: # Key the cache by PROJECT_ROOT so parallel worktrees don't collide. cache_key = hashlib.sha256(str(PROJECT_ROOT).encode()).hexdigest()[:12] @@ -10316,6 +10440,18 @@ def _update_node_dependencies() -> list[str]: env=nixos_env, ) if ws_result.returncode == 0: + if _web_build_toolchain_ready(PROJECT_ROOT): + _record_npm_lockfile_hash(shared_hermes_root) + print(" ✓ repo root + ui-tui, web workspaces (desktop skipped)") + return [] + # Mocked unit tests call this with a successful npm exit code but no + # real node_modules tree — trust the exit code in that case. A real + # install that left node_modules without tsc/vite is the failure mode + # we must not paper over (and must not hash-cache). + if (PROJECT_ROOT / "node_modules").is_dir(): + print(" ⚠ npm workspace install finished without tsc/vite shims") + print(" (devDependencies likely omitted — check NODE_ENV / npm omit)") + return _partial_update_failure("ui-tui, web workspaces") _record_npm_lockfile_hash(shared_hermes_root) print(" ✓ repo root + ui-tui, web workspaces (desktop skipped)") return [] diff --git a/tests/hermes_cli/test_web_ui_build.py b/tests/hermes_cli/test_web_ui_build.py index 95c0960de41a..b483db76da25 100644 --- a/tests/hermes_cli/test_web_ui_build.py +++ b/tests/hermes_cli/test_web_ui_build.py @@ -563,3 +563,99 @@ class TestBuildWebUIFlock: def test_lock_file_is_gitignored(self): gitignore = Path(__file__).resolve().parents[2] / ".gitignore" assert ".web_ui_build.lock" in gitignore.read_text(encoding="utf-8") + + +class TestNpmInstallDevDepEnvForcing: + """Config-level guards so production/omit-dev env can't strip tsc/vite.""" + + def test_forces_npm_config_include_dev_and_clears_omit(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + (web_dir / "package-lock.json").write_text("{}", encoding="utf-8") + mock_cp = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + with patch("hermes_cli.main.subprocess.run", return_value=mock_cp) as mock_run: + _run_npm_install_deterministic( + "/usr/bin/npm", + web_dir, + env={ + "NODE_ENV": "production", + "npm_config_omit": "dev", + "npm_config_production": "true", + }, + ) + _, kwargs = mock_run.call_args + env = kwargs["env"] + assert env["npm_config_include"] == "dev" + assert env["npm_config_production"] == "false" + assert "npm_config_omit" not in env + assert env["CI"] == "1" + + +class TestWebToolchainReadyAndRepair: + def test_toolchain_ready_requires_tsc_and_vite_shims(self, tmp_path): + from hermes_cli.main import _web_build_toolchain_ready + + web_dir, _ = _make_web_dir(tmp_path) + root = web_dir.parent + assert _web_build_toolchain_ready(root) is False + bin_dir = root / "node_modules" / ".bin" + bin_dir.mkdir(parents=True) + (bin_dir / "tsc").touch() + assert _web_build_toolchain_ready(root) is False + (bin_dir / "vite").touch() + assert _web_build_toolchain_ready(root) is True + + def test_build_repairs_when_node_modules_exists_without_tsc(self, tmp_path): + """Partial install (node_modules present, no tsc) triggers a repair install.""" + from hermes_cli.main import _resolve_node_runtime_npm + + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + # Simulate a production-omit install: tree exists, toolchain missing. + (tmp_path / "node_modules").mkdir() + (tmp_path / "node_modules" / ".bin").mkdir() + + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + install_calls = {"n": 0} + + def fake_install(*_a, **_k): + install_calls["n"] += 1 + # After the repair install, pretend tsc/vite appeared. + if install_calls["n"] >= 2: + bin_dir = tmp_path / "node_modules" / ".bin" + (bin_dir / "tsc").touch() + (bin_dir / "vite").touch() + return install_ok + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", side_effect=fake_install) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", return_value=build_ok), \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"): + result = _build_web_ui(web_dir) + + assert result is True + # First silent install + one repair install. + assert mock_install.call_count == 2 + + def test_build_retries_install_on_tsc_not_found(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + build_fail = __import__("subprocess").CompletedProcess( + [], 127, stdout="sh: 1: tsc: not found\n", stderr="" + ) + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", side_effect=[build_fail, build_ok]) as mock_build, \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"), \ + patch("hermes_cli.main._time.sleep"): + result = _build_web_ui(web_dir) + + assert result is True + # Initial install + repair install after tsc: not found. + assert mock_install.call_count == 2 + assert mock_build.call_count == 2 From c2e45b555f8a4e78e8dacbeb965bbf3fcf5d709a Mon Sep 17 00:00:00 2001 From: Gille <4317663+helix4u@users.noreply.github.com> Date: Mon, 27 Jul 2026 11:47:17 -0600 Subject: [PATCH 02/19] fix(desktop): keep free-text slash arguments editable (#72768) --- .../hooks/use-composer-trigger.test.ts | 41 +++++++++ .../composer/hooks/use-composer-trigger.ts | 40 ++++++-- apps/desktop/src/app/chat/composer/index.tsx | 14 ++- .../src/lib/desktop-slash-commands.test.ts | 20 ++-- .../desktop/src/lib/desktop-slash-commands.ts | 92 +++++++++++++------ 5 files changed, 156 insertions(+), 51 deletions(-) diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts index c356fae3af84..c7a4e2f138ac 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.test.ts @@ -121,3 +121,44 @@ describe('useComposerTrigger — slash anywhere in the prompt', () => { expect(hook.result.current.trigger).toBeNull() }) }) + +describe('useComposerTrigger — free-text slash arguments', () => { + it('keeps a picked /goal command as editable text while retaining subcommand completion', () => { + const editor = mountEditor('/go') + const goal = item('/goal', 'Commands') + const { hook } = mountTrigger(editor, [goal]) + + act(() => hook.result.current.refreshTrigger()) + act(() => hook.result.current.replaceTriggerWithChip(goal)) + + expect(composerPlainText(editor)).toBe('/goal ') + expect(editor.querySelector('[data-slash-kind]')).toBeNull() + expect(hook.result.current.trigger).not.toBeNull() + }) + + it('does not seal a multi-word /goal into a chip when the option list runs empty', () => { + const editor = mountEditor('/goal finish the full prompt') + const { hook } = mountTrigger(editor, []) + + act(() => hook.result.current.refreshTrigger()) + + expect(hook.result.current.slashFreeTextArgStage).toBe(true) + expect(hook.result.current.commitTypedSlashDirective()).toBe(false) + expect(composerPlainText(editor)).toBe('/goal finish the full prompt') + expect(editor.querySelector('[data-slash-kind]')).toBeNull() + }) + + it('still commits a fully typed finite option as one directive chip', () => { + const editor = mountEditor('/personality creative') + const { hook } = mountTrigger(editor, []) + + act(() => hook.result.current.refreshTrigger()) + + expect(hook.result.current.slashFreeTextArgStage).toBe(false) + act(() => { + expect(hook.result.current.commitTypedSlashDirective()).toBe(true) + }) + expect(composerPlainText(editor)).toBe('/personality creative ') + expect(editor.querySelector('[data-slash-kind]')?.getAttribute('data-ref-text')).toBe('/personality creative') + }) +}) diff --git a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts index 3c22fddb6348..2df218e78f30 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts +++ b/apps/desktop/src/app/chat/composer/hooks/use-composer-trigger.ts @@ -2,7 +2,7 @@ import type { Unstable_TriggerAdapter, Unstable_TriggerItem } from '@assistant-u import { type MutableRefObject, type RefObject, useCallback, useEffect, useRef, useState } from 'react' import { hermesDirectiveFormatter } from '@/components/assistant-ui/directive-text' -import { desktopSlashCommandTakesArgs } from '@/lib/desktop-slash-commands' +import { desktopSlashCommandArgumentMode } from '@/lib/desktop-slash-commands' import { COMPLETION_ACTIONS, @@ -89,11 +89,16 @@ export function useComposerTrigger({ const before = textBeforeCaret(editor) const found = detectTrigger(before ?? composerPlainText(editor)) - // The arg-stage popover is only useful for commands with an options screen. - // For a no-arg command it would dead-end on "No matches", so drop it — the - // directive is already complete. + // A text-only command has no completion screen once its prose begins. Mixed + // commands such as /goal stay live so their finite subcommands can still be + // suggested, while arbitrary goal text remains valid. + const argumentMode = + found?.kind === '/' && slashArgStage(found.query) + ? desktopSlashCommandArgumentMode(slashCommandToken(found.query)) + : null + const detected = - found?.kind === '/' && slashArgStage(found.query) && !desktopSlashCommandTakesArgs(slashCommandToken(found.query)) + found?.kind === '/' && slashArgStage(found.query) && argumentMode !== 'options' && argumentMode !== 'mixed' ? null : found @@ -134,6 +139,11 @@ export function useComposerTrigger({ // Space/Tab — neither should dead-end on a popover. const argStageEmpty = trigger?.kind === '/' && slashArgStage(trigger.query) && !triggerLoading && !triggerItems.length + const slashFreeTextArgStage = + trigger?.kind === '/' && + slashArgStage(trigger.query) && + ['mixed', 'text'].includes(desktopSlashCommandArgumentMode(slashCommandToken(trigger.query)) ?? '') + const closeTrigger = () => { setTrigger(null) setTriggerItems([]) @@ -148,9 +158,16 @@ export function useComposerTrigger({ // the completion list is empty because the arg is already fully typed (the // backend completer drops exact matches). Reuses the chip path via a // synthetic item whose serialized form is the verbatim text. - const commitTypedSlashDirective = () => { + const commitTypedSlashDirective = (): boolean => { if (trigger?.kind !== '/') { - return + return false + } + + // Free prose must stay ordinary contentEditable text. This guard also + // protects against a stale completion result reaching the keydown path + // before refreshTrigger has caught up with the latest DOM input. + if (desktopSlashCommandArgumentMode(slashCommandToken(trigger.query)) !== 'options') { + return false } const text = `/${trigger.query.trimEnd()}` @@ -168,6 +185,8 @@ export function useComposerTrigger({ rawText: text } }) + + return true } const replaceTriggerWithChip = (item: Unstable_TriggerItem) => { @@ -208,15 +227,15 @@ export function useComposerTrigger({ // there's no command invocation for the args to belong to. const command = (item.metadata as { command?: string } | undefined)?.command ?? '' - const expandsToArgs = - trigger.kind === '/' && !trigger.inline && !serialized.includes(' ') && desktopSlashCommandTakesArgs(command) + const argumentMode = desktopSlashCommandArgumentMode(command) + const expandsToArgs = trigger.kind === '/' && !trigger.inline && !serialized.includes(' ') && argumentMode !== null const text = starter || serialized.endsWith(' ') ? serialized : `${serialized} ` const directive = !starter && serialized.match(/^@([^:]+):(.+)$/) // No pill while expanding — the bare command stays plain text until an arg // is picked, at which point a single pill is emitted for the full command. const slashKind = !expandsToArgs && trigger.kind === '/' ? slashChipKindForItem(item) : null - const keepTriggerOpen = starter || expandsToArgs + const keepTriggerOpen = starter || (expandsToArgs && argumentMode !== 'text') const finish = () => { draftRef.current = composerPlainText(editor) @@ -288,6 +307,7 @@ export function useComposerTrigger({ refreshTrigger, replaceTriggerWithChip, setTriggerActive, + slashFreeTextArgStage, trigger, triggerActive, triggerItems, diff --git a/apps/desktop/src/app/chat/composer/index.tsx b/apps/desktop/src/app/chat/composer/index.tsx index 5988d6103707..b5a76f349040 100644 --- a/apps/desktop/src/app/chat/composer/index.tsx +++ b/apps/desktop/src/app/chat/composer/index.tsx @@ -304,6 +304,7 @@ export function ChatBar({ refreshTrigger, replaceTriggerWithChip, setTriggerActive, + slashFreeTextArgStage, trigger, triggerActive, triggerItems, @@ -544,7 +545,9 @@ export function ChatBar({ // options step, and an arg option commits the full `/cmd arg` chip. Space // is slash-only (an `@` mention takes a literal space) and gated to a // non-empty query so a bare `/ ` still types a space. - const acceptOnSpace = event.key === ' ' && trigger.kind === '/' && Boolean(trigger.query.trim()) + const acceptOnSpace = + event.key === ' ' && trigger.kind === '/' && Boolean(trigger.query.trim()) && !slashFreeTextArgStage + const accept = event.key === 'Enter' || event.key === 'Tab' || acceptOnSpace if (accept) { @@ -579,11 +582,12 @@ export function ChatBar({ slashArgStage(trigger.query) && trigger.query.trim() ) { - event.preventDefault() - triggerKeyConsumedRef.current = true - commitTypedSlashDirective() + if (commitTypedSlashDirective()) { + event.preventDefault() + triggerKeyConsumedRef.current = true - return + return + } } // ArrowUp/ArrowDown navigate, in priority order: the queue (edit entries in diff --git a/apps/desktop/src/lib/desktop-slash-commands.test.ts b/apps/desktop/src/lib/desktop-slash-commands.test.ts index d8aa1897652d..1fc70dcb5ad5 100644 --- a/apps/desktop/src/lib/desktop-slash-commands.test.ts +++ b/apps/desktop/src/lib/desktop-slash-commands.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest' import { desktopSkinSlashCompletions, + desktopSlashCommandArgumentMode, desktopSlashDescription, desktopSlashUnavailableMessage, filterDesktopCommandsCatalog, @@ -65,7 +66,7 @@ describe('desktop slash command curation', () => { it('routes /pet through the desktop action handler and drops /pets', () => { expect(resolveDesktopCommand('/pet')?.surface).toEqual({ kind: 'action', action: 'pet' }) - expect(resolveDesktopCommand('/pet')?.args).toBe(true) + expect(desktopSlashCommandArgumentMode('/pet')).toBe('options') expect(isDesktopSlashSuggestion('/pet')).toBe(true) expect(isDesktopSlashCommand('/pet')).toBe(true) expect(resolveDesktopCommand('/pets')?.surface).toEqual({ kind: 'unavailable', reason: 'settings' }) @@ -81,14 +82,14 @@ describe('desktop slash command curation', () => { expect(desktopSlashUnavailableMessage('/browser')).toBeNull() expect(resolveDesktopCommand('/browser')?.surface).toEqual({ kind: 'action', action: 'browser' }) // Bare /browser expands to its sub-action options in the popover. - expect(resolveDesktopCommand('/browser')?.args).toBe(true) + expect(desktopSlashCommandArgumentMode('/browser')).toBe('options') }) it('routes /compress through the session-compression action', () => { // /compress must be an action (session.compress RPC), not exec: the slash // worker route times out on large sessions (#44456). expect(resolveDesktopCommand('/compress')?.surface).toEqual({ kind: 'action', action: 'compress' }) - expect(resolveDesktopCommand('/compress')?.args).toBe(true) + expect(desktopSlashCommandArgumentMode('/compress')).toBe('text') expect(isDesktopSlashCommand('/compress')).toBe(true) expect(isDesktopSlashSuggestion('/compress')).toBe(true) expect(desktopSlashUnavailableMessage('/compress')).toBeNull() @@ -144,12 +145,13 @@ describe('desktop slash command curation', () => { } }) - it('keeps /goal arg text editable instead of sealing it into a chip', () => { - // /goal takes free prose (the goal itself) plus subcommands. Without - // args:true, Space after the command name committed a sealed directive - // chip and the goal text rendered awkwardly after a pill. - expect(resolveDesktopCommand('/goal')?.surface).toEqual({ kind: 'exec' }) - expect(resolveDesktopCommand('/goal')?.args).toBe(true) + it('distinguishes free prose from finite slash option lists', () => { + expect(desktopSlashCommandArgumentMode('/goal')).toBe('mixed') + expect(desktopSlashCommandArgumentMode('/steer')).toBe('text') + expect(desktopSlashCommandArgumentMode('/queue')).toBe('text') + expect(desktopSlashCommandArgumentMode('/personality')).toBe('options') + expect(desktopSlashCommandArgumentMode('/handoff')).toBe('options') + expect(desktopSlashCommandArgumentMode('/version')).toBeNull() }) it('routes /journey (and aliases) to the memory graph overlay action', () => { diff --git a/apps/desktop/src/lib/desktop-slash-commands.ts b/apps/desktop/src/lib/desktop-slash-commands.ts index 5fccef48c719..81e4a694c015 100644 --- a/apps/desktop/src/lib/desktop-slash-commands.ts +++ b/apps/desktop/src/lib/desktop-slash-commands.ts @@ -91,6 +91,16 @@ export interface SlashCommandBuildCtx { sessionId: string } +/** + * How arguments behave in the Desktop composer. + * + * - `options` → a finite completion list; picking or fully typing an option may + * commit the complete directive as a chip. + * - `text` → arbitrary prose; the command and its argument stay editable. + * - `mixed` → offers subcommand completions but also accepts arbitrary prose. + */ +export type DesktopSlashArgumentMode = 'mixed' | 'options' | 'text' + export interface DesktopCommandSpec { /** Canonical command, leading slash included (e.g. `/resume`). */ name: string @@ -104,12 +114,8 @@ export interface DesktopCommandSpec { * the status bar), so the popover doesn't dead-end on inline completion. */ hidden?: boolean - /** - * The command has an inline options "screen" (theme / personality / session / - * platform / toolset list). Picking the bare command in the popover expands to - * that argument step instead of committing — mirroring typing `/ ` by hand. - */ - args?: boolean + /** Composer behavior for text following the command token. */ + argumentMode?: DesktopSlashArgumentMode } const exec = (): DesktopCommandSurface => ({ kind: 'exec' }) @@ -151,17 +157,22 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ name: '/handoff', description: 'Hand off this session to a messaging platform', surface: action('handoff'), - args: true + argumentMode: 'options' }, { name: '/profile', description: 'Switch the active Hermes profile', surface: action('profile') }, - { name: '/skin', description: 'Switch desktop theme or cycle to the next one', surface: action('skin'), args: true }, - { name: '/title', description: 'Rename the current session', surface: action('title') }, + { + name: '/skin', + description: 'Switch desktop theme or cycle to the next one', + surface: action('skin'), + argumentMode: 'options' + }, + { name: '/title', description: 'Rename the current session', surface: action('title'), argumentMode: 'text' }, { name: '/help', description: 'Show desktop slash commands', aliases: ['/commands'], surface: action('help') }, { name: '/browser', description: 'Manage browser CDP connection [connect|disconnect|status] (local gateway only)', surface: action('browser'), - args: true + argumentMode: 'options' }, { name: '/journey', @@ -177,7 +188,7 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ description: 'Resume a saved session', aliases: ['/sessions', '/switch'], surface: picker('session'), - args: true + argumentMode: 'options' }, // Backend-executed commands that render useful inline output. @@ -194,7 +205,7 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ name: '/approvals', description: 'Show or set approval mode [manual|smart|off]', surface: exec(), - args: true + argumentMode: 'options' }, { name: '/agents', @@ -202,7 +213,13 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ aliases: ['/tasks'], surface: exec() }, - { name: '/background', description: 'Run a prompt in the background', aliases: ['/bg', '/btw'], surface: exec() }, + { + name: '/background', + description: 'Run a prompt in the background', + aliases: ['/bg', '/btw'], + surface: exec(), + argumentMode: 'text' + }, // /compress must be an action (session.compress RPC), not exec: the slash // worker route times out on large sessions (30s WS / 45s pipe) before the // LLM summarise call finishes, then command.dispatch surfaces a bogus @@ -212,16 +229,26 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ description: 'Compress this conversation context', aliases: ['/compact'], surface: action('compress'), - args: true + argumentMode: 'text' }, { name: '/debug', description: 'Create a debug report', surface: exec() }, - { name: '/goal', description: 'Manage the standing goal for this session', surface: exec(), args: true }, - { name: '/personality', description: 'Switch personality for this session', surface: exec(), args: true }, + { + name: '/goal', + description: 'Manage the standing goal for this session', + surface: exec(), + argumentMode: 'mixed' + }, + { + name: '/personality', + description: 'Switch personality for this session', + surface: exec(), + argumentMode: 'options' + }, { name: '/pet', description: 'Toggle or adopt a petdex mascot (/pet, /pet list, /pet boba)', surface: action('pet'), - args: true + argumentMode: 'options' }, { name: '/hatch', @@ -229,7 +256,13 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ aliases: ['/generate-pet'], surface: action('hatch') }, - { name: '/queue', description: 'Queue a prompt for the next turn', aliases: ['/q'], surface: exec() }, + { + name: '/queue', + description: 'Queue a prompt for the next turn', + aliases: ['/q'], + surface: exec(), + argumentMode: 'text' + }, { name: '/retry', description: 'Retry the last user message', surface: exec() }, { name: '/rollback', description: 'List or restore filesystem checkpoints', surface: exec() }, { @@ -242,9 +275,19 @@ const DESKTOP_COMMAND_SPECS: readonly DesktopCommandSpec[] = [ description: 'Show current session status', surface: rpc('session.status', ctx => ({ session_id: ctx.sessionId })) }, - { name: '/steer', description: 'Steer the current run after the next tool call', surface: exec(), args: true }, + { + name: '/steer', + description: 'Steer the current run after the next tool call', + surface: exec(), + argumentMode: 'text' + }, { name: '/stop', description: 'Stop running background processes', surface: exec() }, - { name: '/tools', description: 'List or toggle tools available to the agent', surface: exec(), args: true }, + { + name: '/tools', + description: 'List or toggle tools available to the agent', + surface: exec(), + argumentMode: 'options' + }, { name: '/undo', description: 'Remove the last user/assistant exchange', surface: exec() }, { name: '/usage', description: 'Show token usage for this session', surface: exec() }, { name: '/version', description: 'Show Hermes Agent version', surface: exec() }, @@ -433,13 +476,8 @@ export function desktopSlashDescription(command: string, fallback = ''): string return SPEC_BY_NAME.get(canonicalDesktopSlashCommand(command))?.description || fallback } -/** - * True when picking the bare command should expand to its inline argument - * options (theme / personality / session / platform / toolset) rather than - * committing immediately. Lets the popover act as a two-step picker. - */ -export function desktopSlashCommandTakesArgs(command: string): boolean { - return resolveDesktopCommand(command)?.args ?? false +export function desktopSlashCommandArgumentMode(command: string): DesktopSlashArgumentMode | null { + return resolveDesktopCommand(command)?.argumentMode ?? null } export function desktopSkinSlashCompletions( From 690ecfa1c7d317338086a98e8849105be6501187 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 27 Jul 2026 12:52:42 -0500 Subject: [PATCH 03/19] fix(update): anchor the web toolchain check on npm's actual search path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reshapes the salvaged fix so it recovers the `tsc: not found` build without mis-diagnosing healthy trees. The npm config forcing is dropped. It rested on the premise that an inherited `npm_config_omit=dev` can beat the `--include=dev` CLI flag; npm resolves command-line flags above environment config and filters `omit` by `include`, so the flag already wins. Verified on npm 10 and 11.5.1: with `npm_config_omit=dev` set and `--include=dev` passed, devDependencies install. `npm_config_production` is worse than redundant — npm 9 removed it, so setting it prints `npm warn config production Use --omit=dev instead.` on every install. The readiness probe now reads every root `npm run build` searches, not just the workspace root. npm links a package's bin shims under the package itself when it owns its lockfile (#42973), so a root-only check called a working tree broken: it forced a redundant install, skipped the build entirely, and made `hermes web` exit 1 on a layout that builds fine today. The pre-build probe is gone with it. A build that works is never second-guessed and no filesystem introspection gates it; recovery is driven by the failure instead. When the build cannot resolve tsc or vite, we reinstall (visibly) and retry before the generic delayed retry, which otherwise just reruns the same command and leaves the stale dist in place forever. The lockfile-hash skip still invalidates on an incomplete toolchain so the next update repairs itself. Also drops the branches that changed behavior based on whether a test mock was installed, and replaces the mock-call-count tests with real temp trees covering both hoisting layouts, the Windows shim extensions, and each shell's wording of an unresolvable binary. Co-authored-by: Gerardo Camorlinga Jr. --- hermes_cli/main.py | 174 +++++++++----------------- tests/hermes_cli/test_cmd_update.py | 35 ++++++ tests/hermes_cli/test_web_ui_build.py | 168 +++++++++++++++---------- 3 files changed, 198 insertions(+), 179 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 38bf0a96628d..f8ae45fdf2b0 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -5195,15 +5195,6 @@ def _run_npm_install_deterministic( # --silent/capture_output). It no-ops when CI is set — same as the TUI # install path and nix/lib.nix npm ci hooks. run_env = {**os.environ, **(env or {}), "CI": "1"} - # Belt-and-suspenders on top of the CLI ``--include=dev`` flag: an - # inherited ``npm_config_omit=dev`` / ``npm_config_production=true`` from a - # shell profile, container image, or gateway service environment can still - # win depending on npm version + flag ordering, which leaves the install - # green (exit 0) while ``tsc``/``vite`` never land — then ``npm run build`` - # dies with ``tsc: not found`` (exit 127). Force the config-level knobs. - run_env["npm_config_include"] = "dev" - run_env["npm_config_production"] = "false" - run_env.pop("npm_config_omit", None) lockfile = cwd / "package-lock.json" if lockfile.exists(): @@ -5235,11 +5226,6 @@ def _run_npm_install_deterministic( ) -def _node_modules_bin_dir(root: Path) -> Path: - """Return ``root/node_modules/.bin`` (may not exist yet).""" - return root / "node_modules" / ".bin" - - def _npm_bin_exists(bin_dir: Path, name: str) -> bool: """True when an npm bin shim for *name* exists (POSIX or Windows).""" return any( @@ -5248,34 +5234,52 @@ def _npm_bin_exists(bin_dir: Path, name: str) -> bool: ) -def _web_build_toolchain_ready(project_root: Path) -> bool: - """Return True when the web UI build toolchain shims are present. +def _web_build_toolchain_ready(*roots: Path) -> bool: + """True when ``tsc`` and ``vite`` shims are reachable from any of *roots*. - ``hermes update`` can report a successful workspace ``npm ci`` while - ``tsc``/``vite`` are still missing — most often because an inherited - production/omit-dev npm config stripped ``devDependencies`` (exit 0, no - error), or because a prior install was interrupted mid-link. The web - build then fails with ``tsc: not found``. Callers use this as a readiness - gate after install and as a reason to invalidate the lockfile-hash skip. + Callers must pass every root the build would search; checking only one + reports a healthy tree as broken. """ - if not (project_root / "web" / "package.json").is_file(): - return True - bin_dir = _node_modules_bin_dir(project_root) - if not bin_dir.is_dir(): - return False - return _npm_bin_exists(bin_dir, "tsc") and _npm_bin_exists(bin_dir, "vite") + bin_dirs = [ + bin_dir + for bin_dir in (root / "node_modules" / ".bin" for root in roots) + if bin_dir.is_dir() + ] + return bool(bin_dirs) and all( + any(_npm_bin_exists(bin_dir, tool) for bin_dir in bin_dirs) + for tool in ("tsc", "vite") + ) -def _prepend_node_bin_dirs(env: dict[str, str], *roots: Path) -> dict[str, str]: - """Prepend each root's ``node_modules/.bin`` onto PATH (existing dirs only).""" - merged = dict(env) - parts = [p for p in merged.get("PATH", "").split(os.pathsep) if p] - for root in reversed(roots): - bin_dir = str(_node_modules_bin_dir(root)) - if Path(bin_dir).is_dir() and bin_dir not in parts: - parts.insert(0, bin_dir) - merged["PATH"] = os.pathsep.join(parts) - return merged +def _web_toolchain_roots(web_dir: Path) -> tuple[Path, ...]: + """Roots whose ``node_modules/.bin`` can satisfy the web build. + + ``npm run build`` prepends ``node_modules/.bin`` for the package and each + of its ancestors, so shims hoisted to the workspace root and shims nested + under a package that owns its lockfile (#42973) are equally valid. + """ + return (web_dir, web_dir.parent) + + +def _missing_web_build_tool(output: str) -> str | None: + """Return the build tool a failed ``npm run build`` could not resolve. + + Each shell words this differently: ``sh: 1: tsc: not found`` (dash), + ``vite: command not found`` (bash/zsh), and ``'tsc' is not recognized as + an internal or external command`` (cmd.exe). + """ + lowered = output.lower() + for tool in ("tsc", "vite"): + if any( + phrase in lowered + for phrase in ( + f"{tool}: not found", + f"{tool}: command not found", + f"'{tool}' is not recognized", + ) + ): + return tool + return None def _build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: @@ -5387,23 +5391,13 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: npm_cwd, npm_workspace_args = _termux_workspace_install_context(web_dir) def _install_web_deps(*, silent: bool) -> "subprocess.CompletedProcess": - extra = (*npm_workspace_args,) - if silent: - extra = (*extra, "--silent") return _run_npm_install_deterministic( npm, npm_cwd, - extra_args=extra, + extra_args=(*npm_workspace_args, "--silent") if silent else npm_workspace_args, env=build_env, ) - def _refresh_build_path() -> dict[str, str]: - # npm lifecycle scripts normally inject node_modules/.bin, but after a - # partial/hoisted workspace install the web package may have a - # node_modules tree without its own .bin while tsc/vite live at the - # workspace root. Prepending both keeps `tsc -b && vite build` resolvable. - return _prepend_node_bin_dirs(build_env, web_dir, npm_cwd) - r1 = _install_web_deps(silent=True) if r1.returncode != 0: _say( @@ -5414,49 +5408,6 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: if fatal: _say(" Run manually: npm install --workspace web && npm run build -w web") return False - - # Install can exit 0 while still omitting devDependencies (production/omit - # config races). Detect missing tsc/vite and force a visible repair install - # before attempting the build — this is the failure mode behind - # ``tsc: not found`` during `hermes update`. - project_root_for_tools = ( - web_dir.parent.parent if web_dir.parent.name == "apps" else web_dir.parent - ) - # Only repair when an install produced a node_modules tree but left the - # toolchain incomplete. Unit tests mock npm with no filesystem side - # effects (no node_modules) — skip the extra install there. - if ( - (project_root_for_tools / "node_modules").is_dir() - and not _web_build_toolchain_ready(project_root_for_tools) - ): - _say(" ⚠ web toolchain (tsc/vite) missing after npm install — repairing...") - r_repair = _install_web_deps(silent=False) - if r_repair.returncode != 0: - _say( - f" {'✗' if fatal else '⚠'} Web UI toolchain repair failed" - + ("" if fatal else " (hermes web will not be available)") - ) - _relay(r_repair) - if fatal: - _say( - " Run manually: npm install --workspace web --include=dev " - "&& npm run build -w web" - ) - return False - if not _web_build_toolchain_ready(project_root_for_tools): - _say( - f" {'✗' if fatal else '⚠'} tsc/vite still missing after repair" - + ("" if fatal else " (hermes web will not be available)") - ) - if fatal: - _say( - " Run manually: npm install --workspace web --include=dev " - "&& npm run build -w web" - ) - return not fatal - - build_env = _refresh_build_path() - # First attempt — stream output via idle-timeout helper (issue #33788). # capture_output=True on a long Vite build looks identical to a hang; # users react by rebooting, which leaves the editable install in a @@ -5464,13 +5415,15 @@ def _do_build_web_ui(web_dir: Path, *, fatal: bool = False) -> bool: # recoverable (the stale-dist fallback below handles the kill path). r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: - build_out = (r2.stdout or "") + (r2.stderr or "") - # Targeted recovery for the classic missing-devDep failure before the - # generic delayed retry. Re-install (non-silent) + refresh PATH. - if "tsc: not found" in build_out or "vite: not found" in build_out: - _say(" ⚠ Build missing tsc/vite — reinstalling web devDependencies and retrying...") + # The install above can exit 0 while leaving the tree without a build + # toolchain — a lockfile-hash skip over a half-installed tree, or an + # interrupted link step. The generic retry below just reruns the same + # command, so `tsc: not found` survives it and the stale dist is + # served forever. Reinstall (non-silent, so the user sees it) first. + missing_tool = _missing_web_build_tool((r2.stdout or "") + (r2.stderr or "")) + if missing_tool: + _say(f" ⚠ Build could not resolve {missing_tool} — reinstalling web dependencies...") _install_web_deps(silent=False) - build_env = _refresh_build_path() r2 = _run_with_idle_timeout([npm, "run", "build"], cwd=web_dir, env=build_env) if r2.returncode != 0: # Retry once after a short delay — covers boot-time races on Windows @@ -10258,11 +10211,14 @@ def _npm_lockfile_changed(hermes_root: Path) -> bool: # node_modules means the cache was recorded by another checkout. if not (PROJECT_ROOT / "node_modules").is_dir(): return True - # A matching lockfile hash with a half-installed tree (e.g. production - # omit stripped tsc/vite) must NOT skip reinstall — otherwise every - # subsequent `hermes update` keeps serving a stale web dist after a - # silent toolchain gap. - if not _web_build_toolchain_ready(PROJECT_ROOT): + # A matching lockfile hash over a tree whose web build toolchain never + # landed must NOT skip the reinstall — otherwise every later `hermes + # update` keeps rebuilding against a half-installed tree and serving a + # stale dist. + web_dir = PROJECT_ROOT / "web" + if (web_dir / "package.json").is_file() and not _web_build_toolchain_ready( + *_web_toolchain_roots(web_dir) + ): return True try: # Key the cache by PROJECT_ROOT so parallel worktrees don't collide. @@ -10440,18 +10396,6 @@ def _update_node_dependencies() -> list[str]: env=nixos_env, ) if ws_result.returncode == 0: - if _web_build_toolchain_ready(PROJECT_ROOT): - _record_npm_lockfile_hash(shared_hermes_root) - print(" ✓ repo root + ui-tui, web workspaces (desktop skipped)") - return [] - # Mocked unit tests call this with a successful npm exit code but no - # real node_modules tree — trust the exit code in that case. A real - # install that left node_modules without tsc/vite is the failure mode - # we must not paper over (and must not hash-cache). - if (PROJECT_ROOT / "node_modules").is_dir(): - print(" ⚠ npm workspace install finished without tsc/vite shims") - print(" (devDependencies likely omitted — check NODE_ENV / npm omit)") - return _partial_update_failure("ui-tui, web workspaces") _record_npm_lockfile_hash(shared_hermes_root) print(" ✓ repo root + ui-tui, web workspaces (desktop skipped)") return [] diff --git a/tests/hermes_cli/test_cmd_update.py b/tests/hermes_cli/test_cmd_update.py index 5a1914bc76bb..d550a2297230 100644 --- a/tests/hermes_cli/test_cmd_update.py +++ b/tests/hermes_cli/test_cmd_update.py @@ -149,6 +149,41 @@ class TestCmdUpdateNpmLockfileCache: ) assert hm._npm_lockfile_changed(tmp_path) is True + def test_missing_web_build_toolchain_defeats_skip(self, tmp_path, monkeypatch): + """A hash recorded over a tree that never got tsc/vite must not skip. + + Otherwise the half-installed tree is permanent: every later update + trusts the hash, the build keeps failing, and the stale dist is served + forever. + """ + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') + (tmp_path / "package.json").write_text('{"workspaces": ["web"]}') + (tmp_path / "web").mkdir() + (tmp_path / "web" / "package.json").write_text("{}") + bin_dir = tmp_path / "node_modules" / ".bin" + bin_dir.mkdir(parents=True) + hm._record_npm_lockfile_hash(tmp_path) + + assert hm._npm_lockfile_changed(tmp_path) is True + + (bin_dir / "tsc").touch() + (bin_dir / "vite").touch() + assert hm._npm_lockfile_changed(tmp_path) is False + + def test_toolchain_check_skipped_without_a_web_package(self, tmp_path, monkeypatch): + """Prebuilt bundles ship no web/ source — they must still skip.""" + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') + (tmp_path / "node_modules").mkdir() + hm._record_npm_lockfile_hash(tmp_path) + + assert hm._npm_lockfile_changed(tmp_path) is False + def test_workspace_package_json_edit_defeats_skip(self, tmp_path, monkeypatch): """The manifest list comes from the root package.json `workspaces` globs (npm's source of truth), so ANY workspace (desktop included) diff --git a/tests/hermes_cli/test_web_ui_build.py b/tests/hermes_cli/test_web_ui_build.py index b483db76da25..df273054d8e3 100644 --- a/tests/hermes_cli/test_web_ui_build.py +++ b/tests/hermes_cli/test_web_ui_build.py @@ -21,8 +21,11 @@ import pytest from hermes_cli.main import ( _web_ui_build_needed, _build_web_ui, - _run_npm_install_deterministic, _compute_web_ui_content_hash, + _missing_web_build_tool, + _run_npm_install_deterministic, + _web_build_toolchain_ready, + _web_toolchain_roots, _web_ui_stamp_path, _write_web_ui_build_stamp, ) @@ -565,80 +568,79 @@ class TestBuildWebUIFlock: assert ".web_ui_build.lock" in gitignore.read_text(encoding="utf-8") -class TestNpmInstallDevDepEnvForcing: - """Config-level guards so production/omit-dev env can't strip tsc/vite.""" +def _link_shims(bin_dir: Path, *names: str) -> None: + bin_dir.mkdir(parents=True, exist_ok=True) + for name in names: + (bin_dir / name).touch() - def test_forces_npm_config_include_dev_and_clears_omit(self, tmp_path): + +class TestWebBuildToolchainReady: + """A tree is ready when the build can resolve tsc AND vite from any root. + + ``npm run build`` searches ``node_modules/.bin`` from the script's own + package up through every ancestor, so a shim in either place counts. + """ + + def test_missing_toolchain_is_not_ready(self, tmp_path): web_dir, _ = _make_web_dir(tmp_path) - (web_dir / "package-lock.json").write_text("{}", encoding="utf-8") - mock_cp = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") - with patch("hermes_cli.main.subprocess.run", return_value=mock_cp) as mock_run: - _run_npm_install_deterministic( - "/usr/bin/npm", - web_dir, - env={ - "NODE_ENV": "production", - "npm_config_omit": "dev", - "npm_config_production": "true", - }, - ) - _, kwargs = mock_run.call_args - env = kwargs["env"] - assert env["npm_config_include"] == "dev" - assert env["npm_config_production"] == "false" - assert "npm_config_omit" not in env - assert env["CI"] == "1" - - -class TestWebToolchainReadyAndRepair: - def test_toolchain_ready_requires_tsc_and_vite_shims(self, tmp_path): - from hermes_cli.main import _web_build_toolchain_ready + assert _web_build_toolchain_ready(web_dir, tmp_path) is False + def test_partial_toolchain_is_not_ready(self, tmp_path): web_dir, _ = _make_web_dir(tmp_path) - root = web_dir.parent - assert _web_build_toolchain_ready(root) is False - bin_dir = root / "node_modules" / ".bin" - bin_dir.mkdir(parents=True) - (bin_dir / "tsc").touch() - assert _web_build_toolchain_ready(root) is False - (bin_dir / "vite").touch() - assert _web_build_toolchain_ready(root) is True - - def test_build_repairs_when_node_modules_exists_without_tsc(self, tmp_path): - """Partial install (node_modules present, no tsc) triggers a repair install.""" - from hermes_cli.main import _resolve_node_runtime_npm + _link_shims(tmp_path / "node_modules" / ".bin", "tsc") + assert _web_build_toolchain_ready(web_dir, tmp_path) is False + def test_hoisted_shims_at_workspace_root_are_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", "tsc", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True + + def test_shims_nested_under_the_package_are_ready(self, tmp_path): + """#42973 layout: web/ owns its lockfile, so npm links shims there.""" web_dir, _ = _make_web_dir(tmp_path) - (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") - # Simulate a production-omit install: tree exists, toolchain missing. (tmp_path / "node_modules").mkdir() - (tmp_path / "node_modules" / ".bin").mkdir() + _link_shims(web_dir / "node_modules" / ".bin", "tsc", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True - install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") - build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") - install_calls = {"n": 0} + def test_shims_split_across_roots_are_ready(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", "tsc") + _link_shims(web_dir / "node_modules" / ".bin", "vite") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True - def fake_install(*_a, **_k): - install_calls["n"] += 1 - # After the repair install, pretend tsc/vite appeared. - if install_calls["n"] >= 2: - bin_dir = tmp_path / "node_modules" / ".bin" - (bin_dir / "tsc").touch() - (bin_dir / "vite").touch() - return install_ok + @pytest.mark.parametrize("shim", ["tsc.cmd", "tsc.ps1", "tsc.exe"]) + def test_windows_shim_extensions_count(self, tmp_path, shim): + web_dir, _ = _make_web_dir(tmp_path) + _link_shims(tmp_path / "node_modules" / ".bin", shim, "vite.cmd") + assert _web_build_toolchain_ready(web_dir, tmp_path) is True - with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ - patch("hermes_cli.main._run_npm_install_deterministic", side_effect=fake_install) as mock_install, \ - patch("hermes_cli.main._run_with_idle_timeout", return_value=build_ok), \ - patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ - patch("hermes_cli.main._write_web_ui_build_stamp"): - result = _build_web_ui(web_dir) - assert result is True - # First silent install + one repair install. - assert mock_install.call_count == 2 +class TestWebToolchainRoots: + def test_searches_the_package_and_its_workspace_root(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + assert _web_toolchain_roots(web_dir) == (web_dir, tmp_path) - def test_build_retries_install_on_tsc_not_found(self, tmp_path): + +class TestMissingWebBuildTool: + """Every shell words an unresolvable binary differently.""" + + @pytest.mark.parametrize( + "output,expected", + [ + ("sh: 1: tsc: not found\nnpm error code 127", "tsc"), + ("bash: line 1: vite: command not found", "vite"), + ("'tsc' is not recognized as an internal or external command", "tsc"), + ("error TS2307: Cannot find module './x'", None), + ("", None), + ], + ) + def test_detects_the_unresolvable_tool(self, output, expected): + assert _missing_web_build_tool(output) == expected + + +class TestBuildRecoversFromMissingToolchain: + def test_reinstalls_and_retries_when_the_build_cannot_resolve_tsc(self, tmp_path): + """The generic retry reruns the same command, so it can't fix this alone.""" web_dir, _ = _make_web_dir(tmp_path) (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") @@ -656,6 +658,44 @@ class TestWebToolchainReadyAndRepair: result = _build_web_ui(web_dir) assert result is True - # Initial install + repair install after tsc: not found. assert mock_install.call_count == 2 assert mock_build.call_count == 2 + + def test_healthy_tree_builds_without_an_extra_install(self, tmp_path): + """No pre-build probing: a build that works is never second-guessed.""" + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + _link_shims(web_dir / "node_modules" / ".bin", "tsc", "vite") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", return_value=build_ok) as mock_build, \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"): + result = _build_web_ui(web_dir) + + assert result is True + assert mock_install.call_count == 1 + assert mock_build.call_count == 1 + + def test_unrelated_build_failure_takes_the_generic_retry_only(self, tmp_path): + web_dir, _ = _make_web_dir(tmp_path) + (tmp_path / "package-lock.json").write_text("{}", encoding="utf-8") + install_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + type_error = __import__("subprocess").CompletedProcess( + [], 2, stdout="src/app.tsx(3,1): error TS2307: Cannot find module\n", stderr="" + ) + build_ok = __import__("subprocess").CompletedProcess([], 0, stdout="", stderr="") + + with patch("hermes_cli.main._resolve_node_runtime_npm", return_value="/usr/bin/npm"), \ + patch("hermes_cli.main._run_npm_install_deterministic", return_value=install_ok) as mock_install, \ + patch("hermes_cli.main._run_with_idle_timeout", side_effect=[type_error, build_ok]), \ + patch("hermes_cli.main._web_ui_build_needed", return_value=True), \ + patch("hermes_cli.main._write_web_ui_build_stamp"), \ + patch("hermes_cli.main._time.sleep"): + result = _build_web_ui(web_dir) + + assert result is True + assert mock_install.call_count == 1 From 9cef5496eb88b7b04e91d86cd64d9b4b38bbd07c Mon Sep 17 00:00:00 2001 From: alelpoan Date: Mon, 27 Jul 2026 16:02:45 +0300 Subject: [PATCH 04/19] fix(desktop): front workspace pane when navigating sidebar routes Capabilities/Messaging/Artifacts (and other full-page workspace routes) rendered their content correctly on navigate(), but the workspace pane itself stayed behind an active session tile in the pane tree if one was focused. Clicking the same sidebar item again after switching to a session tile appeared to do nothing. Session switches already call revealTreePane('workspace') + noteActiveTreeGroup(null) to front the pane (store/session-states.ts), but sidebar/keybind/command-palette navigation to full-page routes did not have an equivalent. Add navigateToWorkspacePage() helper in routes.ts that wraps navigate() and fronts the workspace pane for non-overlay, non-chat views. Apply it at all 5 call sites that navigate to full-page workspace routes: selectSidebarItem, use-keybinds (nav.skills/messaging/artifacts), command palette 'go', Command Center's onNavigateRoute, and cold-start route restore. Fixes #72602 --- .../desktop/src/app/command-palette/index.tsx | 3 ++- .../contrib/hooks/use-desktop-integrations.ts | 4 +-- apps/desktop/src/app/contrib/wiring.tsx | 3 ++- apps/desktop/src/app/hooks/use-keybinds.ts | 7 +++--- apps/desktop/src/app/routes.ts | 25 +++++++++++++++++++ .../hooks/use-session-actions/index.ts | 4 +-- 6 files changed, 37 insertions(+), 9 deletions(-) diff --git a/apps/desktop/src/app/command-palette/index.tsx b/apps/desktop/src/app/command-palette/index.tsx index 42bcef4e820a..e86bd2415e39 100644 --- a/apps/desktop/src/app/command-palette/index.tsx +++ b/apps/desktop/src/app/command-palette/index.tsx @@ -71,6 +71,7 @@ import { COMMAND_CENTER_ROUTE, CRON_ROUTE, MESSAGING_ROUTE, + navigateToWorkspacePage, NEW_CHAT_ROUTE, PROFILES_ROUTE, sessionRoute, @@ -351,7 +352,7 @@ export function CommandPalette() { } }, [open, pendingPage]) - const go = useCallback((path: string) => () => navigate(path), [navigate]) + const go = useCallback((path: string) => () => navigateToWorkspacePage(navigate, path), [navigate]) // Step up one nested page (or back to the root list), clearing the filter so // the parent page doesn't reopen mid-search. diff --git a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts index ed22e2cc7ebe..5e6c75dee23a 100644 --- a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts +++ b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts @@ -17,7 +17,7 @@ import { openUpdatesWindow, startUpdatePoller, stopUpdatePoller } from '@/store/ import { isSecondaryWindow } from '@/store/windows' import { requestComposerFocus, requestComposerInsert } from '../../chat/composer/focus' -import { appViewForPath, isOverlayView, NEW_CHAT_ROUTE, sessionRoute } from '../../routes' +import { appViewForPath, isOverlayView, navigateToWorkspacePage, NEW_CHAT_ROUTE, sessionRoute } from '../../routes' interface DesktopIntegrationsParams { chatOpen: boolean @@ -99,7 +99,7 @@ export function useDesktopIntegrations({ const route = getRememberedRoute() if (route && route !== NEW_CHAT_ROUTE && !isOverlayView(appViewForPath(route))) { - navigate(route, { replace: true }) + navigateToWorkspacePage(navigate, route, { replace: true }) return } diff --git a/apps/desktop/src/app/contrib/wiring.tsx b/apps/desktop/src/app/contrib/wiring.tsx index de2e0becffdb..8e02554b67f5 100644 --- a/apps/desktop/src/app/contrib/wiring.tsx +++ b/apps/desktop/src/app/contrib/wiring.tsx @@ -78,6 +78,7 @@ import { closeAllTerminals } from '../right-sidebar/terminal/terminals' import { $workspaceIsPage, CRON_ROUTE, + navigateToWorkspacePage, routeSessionId, sessionRoute, SETTINGS_ROUTE, @@ -1015,7 +1016,7 @@ export function ContribWiring({ children }: { children: ReactNode }) { initialSection={commandCenterInitialSection} onClose={closeOverlayToPreviousRoute} onDeleteSession={removeSession} - onNavigateRoute={path => navigate(path)} + onNavigateRoute={path => navigateToWorkspacePage(navigate, path)} onOpenSession={sessionId => navigate(sessionRoute(sessionId))} /> diff --git a/apps/desktop/src/app/hooks/use-keybinds.ts b/apps/desktop/src/app/hooks/use-keybinds.ts index 8a1fe275478e..7e49029d1cea 100644 --- a/apps/desktop/src/app/hooks/use-keybinds.ts +++ b/apps/desktop/src/app/hooks/use-keybinds.ts @@ -57,6 +57,7 @@ import { ARTIFACTS_ROUTE, CRON_ROUTE, MESSAGING_ROUTE, + navigateToWorkspacePage, PROFILES_ROUTE, sessionRoute, SETTINGS_ROUTE, @@ -138,9 +139,9 @@ export function useKeybinds(deps: KeybindRuntimeDeps): void { 'nav.commandCenter': deps.toggleCommandCenter, 'nav.settings': () => navigate(SETTINGS_ROUTE), 'nav.profiles': () => navigate(PROFILES_ROUTE), - 'nav.skills': () => navigate(SKILLS_ROUTE), - 'nav.messaging': () => navigate(MESSAGING_ROUTE), - 'nav.artifacts': () => navigate(ARTIFACTS_ROUTE), + 'nav.skills': () => navigateToWorkspacePage(navigate, SKILLS_ROUTE), + 'nav.messaging': () => navigateToWorkspacePage(navigate, MESSAGING_ROUTE), + 'nav.artifacts': () => navigateToWorkspacePage(navigate, ARTIFACTS_ROUTE), 'nav.cron': () => navigate(CRON_ROUTE), 'nav.agents': () => navigate(AGENTS_ROUTE), diff --git a/apps/desktop/src/app/routes.ts b/apps/desktop/src/app/routes.ts index b3c28a0ae017..e834d26b1907 100644 --- a/apps/desktop/src/app/routes.ts +++ b/apps/desktop/src/app/routes.ts @@ -1,8 +1,11 @@ import { atom } from 'nanostores' import type { ReactNode } from 'react' +import { noteActiveTreeGroup, revealTreePane } from '@/components/pane-shell/tree/store' import { registry } from '@/contrib/registry' +type NavigateLike = (to: string, options?: { replace?: boolean }) => void + export const SESSION_ROUTE_PREFIX = '/' export const NEW_CHAT_ROUTE = '/' export const SETTINGS_ROUTE = '/settings' @@ -195,3 +198,25 @@ export function syncWorkspaceIsPage(pathname: string): void { $workspaceIsPage.set(isPage) } } + +/** + * Navigate to `path`, and if it lands on a full page rendered inside the + * `workspace` pane (skills/messaging/artifacts/contributed routes) also + * front the `workspace` pane in the pane tree. + * + * Without this, `navigate()` alone updates the route and the content inside + * `workspace` correctly, but if a session tile is currently focused in the + * same tab group, `workspace` stays behind it (issue #72602). This mirrors + * what `$selectedStoredSessionId.listen` already does for session switches + * in `store/session-states.ts`. + */ +export function navigateToWorkspacePage(navigate: NavigateLike, path: string, options?: { replace?: boolean }): void { + navigate(path, options) + + const view = appViewForPath(path) + + if (view !== 'chat' && !isOverlayView(view)) { + noteActiveTreeGroup(null) + revealTreePane('workspace') + } +} diff --git a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts index fd490666d126..5d293c012b21 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts +++ b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts @@ -68,7 +68,7 @@ import { broadcastSessionsChanged } from '@/store/session-sync' import { isWatchWindow } from '@/store/windows' import type { SessionCreateResponse, SessionMessage, SessionResumeResponse, UsageStats } from '@/types/hermes' -import { NEW_CHAT_ROUTE, sessionRoute, SETTINGS_ROUTE } from '../../../routes' +import { navigateToWorkspacePage, NEW_CHAT_ROUTE, sessionRoute, SETTINGS_ROUTE } from '../../../routes' import type { ClientSessionState, SidebarNavItem } from '../../../types' import { sessionContextDrift } from '../session-context-drift' @@ -458,7 +458,7 @@ export function useSessionActions({ } if (item.route) { - navigate(item.route) + navigateToWorkspacePage(navigate, item.route) } }, [navigate, startFreshSessionDraft] From 5ecc6661468fd256fc3753183956f71064a313bd Mon Sep 17 00:00:00 2001 From: alelpoan Date: Mon, 27 Jul 2026 16:02:49 +0300 Subject: [PATCH 05/19] test(desktop): cover workspace pane reveal in selectSidebarItem Adds a regression test for #72602: navigating to a sidebar route now calls navigate() and fronts the workspace pane (noteActiveTreeGroup(null) + revealTreePane('workspace')). Mocks @/components/pane-shell/tree/store to assert the calls without depending on real pane-tree DOM state. --- .../hooks/use-session-actions.test.tsx | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx b/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx index 531937b64b83..31882d91b66d 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-session-actions.test.tsx @@ -3,6 +3,7 @@ import type { MutableRefObject } from 'react' import { useEffect } from 'react' import { afterEach, describe, expect, it, vi } from 'vitest' +import { noteActiveTreeGroup, revealTreePane } from '@/components/pane-shell/tree/store' import { getSession, getSessionMessages, type SessionInfo } from '@/hermes' import { createClientSessionState } from '@/lib/chat-runtime' import { clearSessionDraft, stashSessionDraft, takeSessionDraft } from '@/store/composer' @@ -55,6 +56,12 @@ vi.mock('@/store/profile', async importOriginal => ({ ensureGatewayProfile: vi.fn().mockResolvedValue(undefined) })) +vi.mock('@/components/pane-shell/tree/store', async importOriginal => ({ + ...(await importOriginal>()), + noteActiveTreeGroup: vi.fn(), + revealTreePane: vi.fn() +})) + const RUNTIME_SESSION_ID = 'rt-new-001' function deferred() { @@ -69,7 +76,7 @@ function deferred() { type HarnessHandle = Pick< ReturnType, - 'createBackendSessionForSend' | 'startFreshSessionDraft' + 'createBackendSessionForSend' | 'selectSidebarItem' | 'startFreshSessionDraft' > function storedSession(overrides: Partial = {}): SessionInfo { @@ -1590,3 +1597,21 @@ describe('createBackendSessionForSend workspace target', () => { expect(params).toMatchObject({ cwd: '/clicked-workspace' }) }) }) +describe('selectSidebarItem', () => { + it('fronts the workspace pane when navigating to a sidebar route (issue #72602)', async () => { + const navigate = vi.fn() + const requestGateway = vi.fn(async () => ({}) as never) + let handle: HarnessHandle | null = null + + render( (handle = value)} requestGateway={requestGateway} />) + await waitFor(() => expect(handle).not.toBeNull()) + + act(() => { + handle!.selectSidebarItem({ icon: (() => null) as never, id: 'skills', label: 'Capabilities', route: '/skills' }) + }) + + expect(navigate).toHaveBeenCalledWith('/skills', undefined) + expect(noteActiveTreeGroup).toHaveBeenCalledWith(null) + expect(revealTreePane).toHaveBeenCalledWith('workspace') + }) +}) From 8323bf3d7138d9b609db4f4dbdd757d161d2dcf8 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 27 Jul 2026 12:54:39 -0500 Subject: [PATCH 06/19] fix(desktop): classify router targets by pathname, not the raw target Every route classifier reasoned about the full navigation target, so a query put them on the wrong branch: `/skills?tab=mcp` failed the reserved path check and fell through to the session-id parser as the session `skills?tab=mcp`, which made `appViewForPath` report Capabilities as a chat. The command palette reaches Capabilities exclusively through those targets, and Settings redirects old `/settings?tab=mcp` deep links there. Strip the query and hash once, up front. Session ids are percent-encoded by `sessionRoute`, so `?`/`#` can only ever start a query or a hash. --- apps/desktop/src/app/routes.ts | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/apps/desktop/src/app/routes.ts b/apps/desktop/src/app/routes.ts index e834d26b1907..03df74b23550 100644 --- a/apps/desktop/src/app/routes.ts +++ b/apps/desktop/src/app/routes.ts @@ -136,16 +136,30 @@ export function isOverlayView(view: AppView): boolean { return OVERLAY_VIEWS.has(view) } +/** The pathname of a router target. Every classifier below reasons about a + * PATH, but callers navigate to full targets (`/skills?tab=mcp`), and an + * unstripped query reaches the session-id parser — `/skills?tab=mcp` reads as + * the session `skills?tab=mcp`, so Capabilities classifies as a chat. + * `sessionRoute` percent-encodes ids, so `?`/`#` can only start a query or a + * hash. */ +export function routePathname(to: string): string { + const cut = to.search(/[?#]/) + + return cut === -1 ? to : to.slice(0, cut) +} + export function isNewChatRoute(pathname: string): boolean { - return pathname === NEW_CHAT_ROUTE + return routePathname(pathname) === NEW_CHAT_ROUTE } export function routeSessionId(pathname: string): string | null { - if (!pathname.startsWith(SESSION_ROUTE_PREFIX) || RESERVED_PATHS.has(pathname) || isContributedPath(pathname)) { + const path = routePathname(pathname) + + if (!path.startsWith(SESSION_ROUTE_PREFIX) || RESERVED_PATHS.has(path) || isContributedPath(path)) { return null } - const id = pathname.slice(SESSION_ROUTE_PREFIX.length) + const id = path.slice(SESSION_ROUTE_PREFIX.length) return id && !id.includes('/') ? decodeURIComponent(id) : null } @@ -172,15 +186,17 @@ export function sessionRoute(sessionId: string): string { } export function appViewForPath(pathname: string): AppView { - if (isNewChatRoute(pathname) || routeSessionId(pathname)) { + const path = routePathname(pathname) + + if (isNewChatRoute(path) || routeSessionId(path)) { return 'chat' } - if (isContributedPath(pathname)) { + if (isContributedPath(path)) { return 'extension' } - return APP_VIEW_BY_PATH.get(pathname) ?? 'chat' + return APP_VIEW_BY_PATH.get(path) ?? 'chat' } /** True while the workspace pane shows a FULL PAGE (skills/messaging/ From f6ea8b462ed7450d665c9124ceb85db1e42130d0 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 27 Jul 2026 12:54:45 -0500 Subject: [PATCH 07/19] fix(desktop): front the workspace pane from the router location MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Capabilities, Messaging and Artifacts render inside the `workspace` pane, so navigating to one has to bring that pane to the front of its group. Nothing did. With the main zone parked on a session tile, the route and the page content changed behind the tile and the click looked dead until the app restarted. Session switches already front the pane in `store/session-states.ts`; pages had no equivalent. Front it from the router location, in the effect that already mirrors `$workspaceIsPage`. One place decides, so every entry point inherits it: sidebar, keybinds, palette, Command Center, contributed statusbar and titlebar `to` targets, back/forward, and cold-start restore — which no longer needs its own call. `navigateToWorkspacePage` stays for the one case the location can't see: hitting Capabilities while already on `/skills` leaves the location untouched, so no effect fires and only an imperative reveal brings the page back. --- .../contrib/hooks/use-desktop-integrations.ts | 4 +- apps/desktop/src/app/contrib/wiring.tsx | 11 ++-- apps/desktop/src/app/routes.ts | 61 +++++++++++++------ 3 files changed, 51 insertions(+), 25 deletions(-) diff --git a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts index 5e6c75dee23a..ed22e2cc7ebe 100644 --- a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts +++ b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts @@ -17,7 +17,7 @@ import { openUpdatesWindow, startUpdatePoller, stopUpdatePoller } from '@/store/ import { isSecondaryWindow } from '@/store/windows' import { requestComposerFocus, requestComposerInsert } from '../../chat/composer/focus' -import { appViewForPath, isOverlayView, navigateToWorkspacePage, NEW_CHAT_ROUTE, sessionRoute } from '../../routes' +import { appViewForPath, isOverlayView, NEW_CHAT_ROUTE, sessionRoute } from '../../routes' interface DesktopIntegrationsParams { chatOpen: boolean @@ -99,7 +99,7 @@ export function useDesktopIntegrations({ const route = getRememberedRoute() if (route && route !== NEW_CHAT_ROUTE && !isOverlayView(appViewForPath(route))) { - navigateToWorkspacePage(navigate, route, { replace: true }) + navigate(route, { replace: true }) return } diff --git a/apps/desktop/src/app/contrib/wiring.tsx b/apps/desktop/src/app/contrib/wiring.tsx index 8e02554b67f5..0cbda7af36f1 100644 --- a/apps/desktop/src/app/contrib/wiring.tsx +++ b/apps/desktop/src/app/contrib/wiring.tsx @@ -82,7 +82,7 @@ import { routeSessionId, sessionRoute, SETTINGS_ROUTE, - syncWorkspaceIsPage + syncWorkspaceRoute } from '../routes' import { SessionPickerOverlay } from '../session-picker-overlay' import { SessionSwitcher } from '../session-switcher' @@ -187,11 +187,12 @@ export function ContribWiring({ children }: { children: ReactNode }) { routedSessionIdRef.current = null }, []) - // Mirror "the workspace is showing a full page" into its atom — the - // workspace pane contribution re-registers headerVeto from it, so the main - // zone's tab bar stands down on pages (and returns with the chat). + // Point the workspace at the route: the pane contribution re-registers + // headerVeto from $workspaceIsPage (so the main zone's tab bar stands down + // on pages), and a page route fronts the pane so it can't stay stuck behind + // a focused session tile. useEffect(() => { - syncWorkspaceIsPage(location.pathname) + syncWorkspaceRoute(location.pathname) }, [location.pathname]) const { diff --git a/apps/desktop/src/app/routes.ts b/apps/desktop/src/app/routes.ts index 03df74b23550..9ee9365c3287 100644 --- a/apps/desktop/src/app/routes.ts +++ b/apps/desktop/src/app/routes.ts @@ -199,6 +199,15 @@ export function appViewForPath(pathname: string): AppView { return APP_VIEW_BY_PATH.get(path) ?? 'chat' } +/** Does `to` land on a full page rendered INSIDE the workspace pane + * (skills/messaging/artifacts/contributed routes)? Overlays don't count — + * they float over whatever the workspace is already showing. */ +function isWorkspacePageRoute(to: string): boolean { + const view = appViewForPath(to) + + return view !== 'chat' && !isOverlayView(view) +} + /** True while the workspace pane shows a FULL PAGE (skills/messaging/ * artifacts/plugin routes) instead of the chat. Published by the wiring * (which owns the router location); the workspace pane contribution mirrors @@ -206,33 +215,49 @@ export function appViewForPath(pathname: string): AppView { * (settings/…) don't count — the chat stays beneath them. */ export const $workspaceIsPage = atom(false) -export function syncWorkspaceIsPage(pathname: string): void { - const view = appViewForPath(pathname) - const isPage = view !== 'chat' && !isOverlayView(view) +function revealWorkspacePane(): void { + noteActiveTreeGroup(null) + revealTreePane('workspace') +} + +/** + * Point the workspace at `pathname`: mirror "showing a full page" into + * `$workspaceIsPage`, and FRONT the pane when it is one. + * + * A page renders inside `workspace`, so a main zone parked on a session tile + * keeps the tile on screen while the route and the page content change behind + * it — the navigation looks dead (#72602). Session switches already front the + * pane in `store/session-states.ts`; pages had no equivalent. + * + * The router location drives this, so every entry point gets it without opting + * in: sidebar, keybinds, command palette, Command Center, contributed + * statusbar/titlebar `to` targets, back/forward, and cold-start restore. + */ +export function syncWorkspaceRoute(pathname: string): void { + const isPage = isWorkspacePageRoute(pathname) if (isPage !== $workspaceIsPage.get()) { $workspaceIsPage.set(isPage) } + + if (isPage) { + revealWorkspacePane() + } } /** - * Navigate to `path`, and if it lands on a full page rendered inside the - * `workspace` pane (skills/messaging/artifacts/contributed routes) also - * front the `workspace` pane in the pane tree. + * Navigate to `to`, fronting the workspace pane when it is a page route. * - * Without this, `navigate()` alone updates the route and the content inside - * `workspace` correctly, but if a session tile is currently focused in the - * same tab group, `workspace` stays behind it (issue #72602). This mirrors - * what `$selectedStoredSessionId.listen` already does for session switches - * in `store/session-states.ts`. + * `syncWorkspaceRoute` covers route CHANGES; this covers the RE-CLICK, the one + * case it can't see — hitting Capabilities while already on `/skills` with a + * tile focused leaves the location untouched, so no effect fires and only an + * imperative reveal brings the page back. Use it wherever a nav affordance can + * be triggered from the page it targets. */ -export function navigateToWorkspacePage(navigate: NavigateLike, path: string, options?: { replace?: boolean }): void { - navigate(path, options) +export function navigateToWorkspacePage(navigate: NavigateLike, to: string, options?: { replace?: boolean }): void { + navigate(to, options) - const view = appViewForPath(path) - - if (view !== 'chat' && !isOverlayView(view)) { - noteActiveTreeGroup(null) - revealTreePane('workspace') + if (isWorkspacePageRoute(to)) { + revealWorkspacePane() } } From ac9a10ccbb9fe05125c54692c601b0d229201820 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 27 Jul 2026 12:54:49 -0500 Subject: [PATCH 08/19] test(desktop): cover the workspace-page reveal bug class Both layers, and the classification underneath them: a page route fronts the pane whether it carries a query or not, moving between two pages fronts it again even though `$workspaceIsPage` never changes, contributed routes count, and chat and overlay targets leave the tab alone. --- .../src/app/routes.workspace-reveal.test.ts | 189 ++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 apps/desktop/src/app/routes.workspace-reveal.test.ts diff --git a/apps/desktop/src/app/routes.workspace-reveal.test.ts b/apps/desktop/src/app/routes.workspace-reveal.test.ts new file mode 100644 index 000000000000..7db75e610aed --- /dev/null +++ b/apps/desktop/src/app/routes.workspace-reveal.test.ts @@ -0,0 +1,189 @@ +/** + * A full page (Capabilities/Messaging/Artifacts/a contributed route) renders + * INSIDE the `workspace` pane, so navigating to one has to front that pane — + * otherwise a main zone parked on a session tile keeps the tile on screen and + * the click looks dead until the app restarts (#72602). + * + * Two layers, both covered here: `syncWorkspaceRoute` (the router location, + * every entry point) and `navigateToWorkspacePage` (the re-click, where the + * location doesn't change). + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { registry } from '@/contrib/registry' + +import { + $workspaceIsPage, + AGENTS_ROUTE, + appViewForPath, + ARTIFACTS_ROUTE, + CRON_ROUTE, + MESSAGING_ROUTE, + navigateToWorkspacePage, + NEW_CHAT_ROUTE, + routePathname, + ROUTES_AREA, + routeSessionId, + sessionRoute, + SETTINGS_ROUTE, + SKILLS_ROUTE, + syncWorkspaceRoute +} from './routes' + +vi.mock('@/components/pane-shell/tree/store', async importOriginal => ({ + ...(await importOriginal>()), + noteActiveTreeGroup: vi.fn(), + revealTreePane: vi.fn() +})) + +const { noteActiveTreeGroup, revealTreePane } = await import('@/components/pane-shell/tree/store') + +const CONTRIBUTED_ROUTE = '/kanban' + +function contributeRoute(): () => void { + return registry.register({ + area: ROUTES_AREA, + data: { path: CONTRIBUTED_ROUTE }, + id: 'test-route', + render: () => null + }) +} + +/** Did the workspace pane get fronted? Both calls, or the tab stays put. */ +const fronted = () => + vi.mocked(revealTreePane).mock.calls.some(([pane]) => pane === 'workspace') && + vi.mocked(noteActiveTreeGroup).mock.calls.some(([group]) => group === null) + +beforeEach(() => { + vi.mocked(revealTreePane).mockClear() + vi.mocked(noteActiveTreeGroup).mockClear() + $workspaceIsPage.set(false) +}) + +afterEach(() => { + $workspaceIsPage.set(false) +}) + +describe('routePathname', () => { + it('keeps a bare path and drops a query or hash', () => { + expect(routePathname(SKILLS_ROUTE)).toBe('/skills') + expect(routePathname('/skills?tab=mcp')).toBe('/skills') + expect(routePathname('/skills?tab=mcp&server=ctx7')).toBe('/skills') + expect(routePathname('/settings#keys')).toBe('/settings') + }) + + it('leaves an encoded session id alone', () => { + const route = sessionRoute('a?b#c') + + expect(routePathname(route)).toBe(route) + expect(routeSessionId(route)).toBe('a?b#c') + }) +}) + +describe('classification of targets carrying a query', () => { + // The palette navigates to every one of these (Capabilities tabs, MCP + // servers), and Settings redirects old /settings?tab=mcp deep links to the + // last one. Unstripped, they parsed as SESSION ids and read as 'chat'. + it.each([ + [`${SKILLS_ROUTE}?tab=skills`, 'skills'], + [`${SKILLS_ROUTE}?tab=toolsets`, 'skills'], + [`${SKILLS_ROUTE}?tab=mcp&server=ctx7`, 'skills'], + [`${SETTINGS_ROUTE}?tab=keys`, 'settings'] + ])('%s is not a session route', (to, view) => { + expect(routeSessionId(to)).toBeNull() + expect(appViewForPath(to)).toBe(view) + }) +}) + +describe('syncWorkspaceRoute', () => { + it('publishes and fronts on a page route', () => { + syncWorkspaceRoute(SKILLS_ROUTE) + + expect($workspaceIsPage.get()).toBe(true) + expect(fronted()).toBe(true) + }) + + it('fronts on a page route reached with a query', () => { + syncWorkspaceRoute(`${SKILLS_ROUTE}?tab=mcp`) + + expect($workspaceIsPage.get()).toBe(true) + expect(fronted()).toBe(true) + }) + + it('fronts when moving between two pages — the atom never changes, the tab must', () => { + syncWorkspaceRoute(ARTIFACTS_ROUTE) + vi.mocked(revealTreePane).mockClear() + vi.mocked(noteActiveTreeGroup).mockClear() + + syncWorkspaceRoute(MESSAGING_ROUTE) + + expect($workspaceIsPage.get()).toBe(true) + expect(fronted()).toBe(true) + }) + + it('fronts on a contributed page route', () => { + const dispose = contributeRoute() + + try { + syncWorkspaceRoute(CONTRIBUTED_ROUTE) + + expect(appViewForPath(CONTRIBUTED_ROUTE)).toBe('extension') + expect(fronted()).toBe(true) + } finally { + dispose() + } + }) + + it.each([ + ['a session route', sessionRoute('sess-a')], + ['the new-chat route', NEW_CHAT_ROUTE], + ['an overlay', SETTINGS_ROUTE], + ['an overlay with a query', `${SETTINGS_ROUTE}?tab=keys`], + ['another overlay', CRON_ROUTE], + ['yet another overlay', AGENTS_ROUTE] + ])('leaves the tab alone on %s', (_label, to) => { + syncWorkspaceRoute(to) + + expect($workspaceIsPage.get()).toBe(false) + expect(revealTreePane).not.toHaveBeenCalled() + }) +}) + +describe('navigateToWorkspacePage', () => { + it('navigates and fronts, so a re-click on the page you are already on still shows it', () => { + const navigate = vi.fn() + + navigateToWorkspacePage(navigate, SKILLS_ROUTE) + + expect(navigate).toHaveBeenCalledWith(SKILLS_ROUTE, undefined) + expect(fronted()).toBe(true) + }) + + it.each([`${SKILLS_ROUTE}?tab=skills`, `${SKILLS_ROUTE}?tab=toolsets`, `${SKILLS_ROUTE}?tab=mcp&server=ctx7`])( + 'fronts for the palette target %s', + to => { + navigateToWorkspacePage(vi.fn(), to) + + expect(fronted()).toBe(true) + } + ) + + it('passes navigation options through', () => { + const navigate = vi.fn() + + navigateToWorkspacePage(navigate, ARTIFACTS_ROUTE, { replace: true }) + + expect(navigate).toHaveBeenCalledWith(ARTIFACTS_ROUTE, { replace: true }) + }) + + it('navigates without fronting for chat and overlay targets', () => { + const navigate = vi.fn() + + navigateToWorkspacePage(navigate, sessionRoute('sess-a')) + navigateToWorkspacePage(navigate, SETTINGS_ROUTE) + + expect(navigate).toHaveBeenCalledTimes(2) + expect(revealTreePane).not.toHaveBeenCalled() + }) +}) From 3e4cdee5ab52682196fca598ba3f26a504470a18 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 27 Jul 2026 13:03:33 -0500 Subject: [PATCH 09/19] chore(contributors): map gercamjr for the #68945 salvage --- contributors/emails/gercamjr.dev@gmail.com | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 contributors/emails/gercamjr.dev@gmail.com diff --git a/contributors/emails/gercamjr.dev@gmail.com b/contributors/emails/gercamjr.dev@gmail.com new file mode 100644 index 000000000000..6ac577a3590c --- /dev/null +++ b/contributors/emails/gercamjr.dev@gmail.com @@ -0,0 +1,2 @@ +gercamjr +# PR #68945 salvage (update: recover the web UI build when npm leaves no tsc/vite) From 29abaeb98fcc48c0a9f3d87c105c13151351475e Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 27 Jul 2026 22:45:24 +0500 Subject: [PATCH 10/19] fix(timeouts): add claude-fable to reasoning stale-timeout floor table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claude-fable-5 is a Mythos-class reasoning model (1M context, 128K output, adaptive thinking per anthropic_adapter.py) but was missing from the _REASONING_STALE_TIMEOUT_FLOORS table. Without a floor entry it got the default 180s stale timeout (300s with context scaling), which is too short for fable-5's thinking phase on large contexts. Each stale kill bumped the cross-turn circuit breaker streak; after 5 consecutive kills _check_stale_giveup() fired immediately (elapsed: 0.00s), aborting all calls with "Provider has been unresponsive for 5 consecutive stale attempts." Users with 191K-token contexts hit this reliably. Add ("claude-fable", 600) — deep-reasoning tier alongside o1/deepseek-r1/ nemotron-3-ultra. The claude-fable slug matches claude-fable-5 and future variants via the existing right-anchor regex. --- agent/reasoning_timeouts.py | 10 ++++++++++ tests/agent/test_reasoning_stale_timeout_floor.py | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/agent/reasoning_timeouts.py b/agent/reasoning_timeouts.py index 8af5ab799f48..9c5fc2020152 100644 --- a/agent/reasoning_timeouts.py +++ b/agent/reasoning_timeouts.py @@ -106,6 +106,14 @@ _REASONING_STALE_TIMEOUT_FLOORS: tuple[tuple[str, int], ...] = ( ("claude-sonnet-5", 180), ("claude-sonnet-4.5", 180), ("claude-sonnet-4.6", 180), + # Anthropic Mythos-class named reasoning models (claude-fable-5, …). + # 1M context + 128K output — heavier thinking phase than the + # numbered Claude line, so the floor is in the deep-reasoning tier + # alongside o1 / deepseek-r1 / nemotron-3-ultra. Without this + # entry the stale-stream detector kills fable-5's thinking phase + # at the default 180s (300s with context scaling), tripping the + # cross-turn circuit breaker after 5 consecutive stale kills. + ("claude-fable", 600), # xAI Grok reasoning variants. Explicit reasoning-only keys # plus one for the ``non-reasoning`` variant so users picking # the fast variant don't get the 300s floor. Bare ``grok-3``, @@ -207,6 +215,8 @@ def get_reasoning_stale_timeout_floor(model: object) -> Optional[float]: 300.0 >>> get_reasoning_stale_timeout_floor("anthropic/claude-opus-4-6") 240.0 + >>> get_reasoning_stale_timeout_floor("anthropic/claude-fable-5") + 600.0 >>> get_reasoning_stale_timeout_floor("gpt-4o") is None True >>> get_reasoning_stale_timeout_floor("olmo-1") is None diff --git a/tests/agent/test_reasoning_stale_timeout_floor.py b/tests/agent/test_reasoning_stale_timeout_floor.py index ec05cb54dd60..608c83d10408 100644 --- a/tests/agent/test_reasoning_stale_timeout_floor.py +++ b/tests/agent/test_reasoning_stale_timeout_floor.py @@ -74,6 +74,10 @@ import pytest ("anthropic/claude-opus-4-20250514", 240.0), ("anthropic/claude-sonnet-4.5", 180.0), ("anthropic/claude-sonnet-4.6", 180.0), + # Anthropic Mythos-class named reasoning models — deep-reasoning tier. + ("anthropic/claude-fable-5", 600.0), + ("claude-fable-5", 600.0), + ("claude-fable", 600.0), # xAI Grok reasoning variants — explicit, not bare `grok`. ("x-ai/grok-4-fast-reasoning", 300.0), ("x-ai/grok-4.20-reasoning", 300.0), From f5f5ac312af29ea1bc276663e2b7d71091753276 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 27 Jul 2026 23:10:30 +0500 Subject: [PATCH 11/19] chore: add contributor mapping for reinbeumer@gmail.com Needed for PR #72677 attribution check. --- contributors/emails/reinbeumer@gmail.com | 1 + 1 file changed, 1 insertion(+) create mode 100644 contributors/emails/reinbeumer@gmail.com diff --git a/contributors/emails/reinbeumer@gmail.com b/contributors/emails/reinbeumer@gmail.com new file mode 100644 index 000000000000..26c0b64b9500 --- /dev/null +++ b/contributors/emails/reinbeumer@gmail.com @@ -0,0 +1 @@ +reinbeumer From d76d0d61d8bcdeb3f4a849a1468839316c6a5a47 Mon Sep 17 00:00:00 2001 From: izumi0uu Date: Tue, 7 Jul 2026 21:58:32 +0800 Subject: [PATCH 12/19] fix(update): refresh runtime modules before lazy backends Refresh update-sensitive modules before lazy backend refresh so an in-place git update does not keep using pre-pull module objects when newly pulled code imports fresh helpers. Constraint: Issue #60242 reports post-update lazy backend refresh importing stale hermes_constants after a large Windows update. Rejected: Only clearing __pycache__ earlier | the update process can still hold old modules in sys.modules. Confidence: high Scope-risk: narrow Directive: Keep update-time lazy refresh guarded against in-process code skew after git pull. Tested: ./.venv/bin/python -m pytest tests/hermes_cli/test_update_autostash.py::test_cmd_update_reloads_runtime_modules_before_lazy_refresh tests/hermes_cli/test_update_autostash.py::test_reload_updated_runtime_modules_restores_new_hermes_constants_symbol -q Tested: ./.venv/bin/python -m pytest tests/hermes_cli/test_update_autostash.py -q Tested: ./.venv/bin/python -m ruff check hermes_cli/main.py tests/hermes_cli/test_update_autostash.py Tested: ./.venv/bin/python -m py_compile hermes_cli/main.py tests/hermes_cli/test_update_autostash.py Tested: git diff --check Not-tested: Windows v0.14.0 to v0.18.0 end-to-end update. --- hermes_cli/main.py | 57 ++++++++++++++++++----- tests/hermes_cli/test_update_autostash.py | 55 ++++++++++++++++++++++ 2 files changed, 100 insertions(+), 12 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index f8ae45fdf2b0..aff5ec07d817 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -4760,6 +4760,38 @@ def _clear_bytecode_cache(root: Path) -> int: return removed +_UPDATE_RUNTIME_RELOAD_MODULES = ( + "hermes_constants", + "tools.environments.local", + "tools.lazy_deps", +) + + +def _reload_updated_runtime_modules() -> None: + """Reload update-sensitive modules after the checkout changes in-place. + + ``hermes update`` keeps running in the pre-pull Python process. After a + large update, modules already present in ``sys.modules`` can still expose + old symbols even though their source files on disk are new. Refresh the + small module set used by lazy-backend refresh before that step imports + newly-updated code paths. + """ + try: + import importlib + + importlib.invalidate_caches() + for module_name in _UPDATE_RUNTIME_RELOAD_MODULES: + module = sys.modules.get(module_name) + if module is None: + continue + try: + importlib.reload(module) + except Exception as exc: + logger.debug("Could not reload updated module %s: %s", module_name, exc) + except Exception as exc: + logger.debug("Could not refresh update runtime modules: %s", exc) + + # Critical files that Hermes must be able to import immediately after an # update/install. Most are imported on every CLI startup; ``web_server.py`` # is the desktop/dashboard backend path that a fresh Windows install launches @@ -12353,6 +12385,19 @@ def _cmd_update_impl(args, gateway_mode: bool): # based on a narrow 7-package import probe (#58004 review). _clear_update_incomplete_marker() + # The update process is still the old Python interpreter process. Run + # one final cache/module refresh immediately before lazy backend + # refresh, which imports newly-pulled modules that may depend on fresh + # symbols in hermes_constants or lazy_deps. The dependency install + # above may also have regenerated bytecode from build-cache copies — + # this second sweep catches those stragglers (#60242, #65240). + removed = _clear_bytecode_cache(PROJECT_ROOT) + if removed: + print( + f" ✓ Cleared {removed} stale __pycache__ director{'y' if removed == 1 else 'ies'}" + ) + _reload_updated_runtime_modules() + # Upgrade pip before lazy refreshes — stale pip can fail source builds # and leave partially-written packages (#57828). _write_lazy_refresh_incomplete_marker() @@ -12511,18 +12556,6 @@ def _cmd_update_impl(args, gateway_mode: bool): except Exception as e: logger.debug("Model catalog seed during update failed: %s", e) - # After git pull, source files on disk are newer than cached Python - # modules in this process. Reload hermes_constants so that any lazy - # import executed below (skills sync, gateway restart) sees new - # attributes like display_hermes_home() added since the last release. - try: - import importlib - import hermes_constants as _hc - - importlib.reload(_hc) - except Exception: - pass # non-fatal — worst case a lazy import fails gracefully - # Sync bundled skills (copies new, updates changed, respects user deletions) try: from tools.skills_sync import sync_skills diff --git a/tests/hermes_cli/test_update_autostash.py b/tests/hermes_cli/test_update_autostash.py index 94fd27949f16..bd1638fcf651 100644 --- a/tests/hermes_cli/test_update_autostash.py +++ b/tests/hermes_cli/test_update_autostash.py @@ -568,6 +568,61 @@ def test_cmd_update_refreshes_active_memory_provider_dependencies(monkeypatch, t assert refresh_calls == [True] +def test_cmd_update_reloads_runtime_modules_before_lazy_refresh(monkeypatch, tmp_path): + """Lazy refresh must not see pre-pull modules cached in this process.""" + _setup_update_mocks(monkeypatch, tmp_path) + monkeypatch.setattr("shutil.which", lambda name: "/usr/bin/uv" if name == "uv" else None) + monkeypatch.setattr(hermes_main, "_is_termux_env", lambda env=None: False) + + events = [] + + def fake_run(cmd, **kwargs): + if cmd == ["git", "fetch", "origin", "main"]: + return SimpleNamespace(stdout="", stderr="", returncode=0) + if cmd == ["git", "rev-parse", "--abbrev-ref", "HEAD"]: + return SimpleNamespace(stdout="main\n", stderr="", returncode=0) + if cmd == ["git", "rev-list", "HEAD..origin/main", "--count"]: + return SimpleNamespace(stdout="1\n", stderr="", returncode=0) + if cmd == ["git", "pull", "--ff-only", "origin", "main"]: + events.append("pull") + return SimpleNamespace(stdout="Updating\n", stderr="", returncode=0) + if "pip" in cmd and "install" in cmd: + events.append("install") + return SimpleNamespace(returncode=0, stdout="", stderr="") + + def fake_reload_runtime_modules(): + events.append("reload") + + def fake_refresh_lazy_features(install_prefix=None, env=None): + events.append("lazy-refresh") + return True + + monkeypatch.setattr(hermes_main.subprocess, "run", fake_run) + monkeypatch.setattr(hermes_main, "_reload_updated_runtime_modules", fake_reload_runtime_modules) + monkeypatch.setattr(hermes_main, "_refresh_active_lazy_features", fake_refresh_lazy_features) + + hermes_main.cmd_update(SimpleNamespace()) + + assert ( + events.index("pull") + < events.index("install") + < events.index("reload") + < events.index("lazy-refresh") + ) + + +def test_reload_updated_runtime_modules_restores_new_hermes_constants_symbol(monkeypatch): + """A pre-pull module object missing a new helper is repaired by reload.""" + import hermes_constants + + monkeypatch.delattr(hermes_constants, "apply_subprocess_home_env", raising=False) + assert not hasattr(hermes_constants, "apply_subprocess_home_env") + + hermes_main._reload_updated_runtime_modules() + + assert callable(hermes_constants.apply_subprocess_home_env) + + def test_install_with_optional_fallback_honors_custom_group(monkeypatch): """Termux update path should target .[termux-all] when requested.""" calls = [] From ea2499bcfe105c2b856d9e2058e9c94097444c6f Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sun, 26 Jul 2026 23:57:41 -0700 Subject: [PATCH 13/19] fix(update): close the stale-bytecode class with a launch-time checkout-fingerprint sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stale-.pyc bug class (#6207, #60242, live WhatsApp report: gateway ImportError 'cannot import name parse_model_flags_detailed' after /update) has one shared shape: the checkout's .py files change while __pycache__ retains bytecode from the previous revision, and a later process trusts the stale .pyc. Update-time clears can never fully close this class: 'hermes update' always executes the PRE-pull updater code, so hardening added to it takes effect one update late — and a manual 'git pull' never runs the updater at all. Class fix: - Launch-time guard in main(): compare the checkout fingerprint (cheap file reads via _read_git_revision_fingerprint, no git subprocess) against a .bytecode-fingerprint stamp; sweep __pycache__ once when they diverge. Covers manual pulls, old updaters, ZIP restores — every entry point (CLI, gateway service, desktop backend) passes through main(). - Record the stamp at all three update-time clear sites (git path pre- install, git path post-install, ZIP path) so a normal update never triggers a redundant launch sweep. - Sibling site: 'hermes plugins update' + dashboard plugin update now clear __pycache__ under the plugin dir after git pull (plugin trees live outside the repo guard). E2E validated: reproduced the stale-pyc shadowing (same-size same-mtime source swap → old symbol wins in a fresh process), confirmed the launch sweep restores the new symbol, no-ops when the checkout is unchanged, and re-sweeps on the next pull. Sabotage-tested: reverting the sweep fails 4 of the new tests. --- .gitignore | 5 + hermes_cli/main.py | 79 ++++++++++++ hermes_cli/plugins_cmd.py | 33 +++++ tests/hermes_cli/test_bytecode_sweep.py | 164 ++++++++++++++++++++++++ 4 files changed, 281 insertions(+) create mode 100644 tests/hermes_cli/test_bytecode_sweep.py diff --git a/.gitignore b/.gitignore index a72536f00779..cd05306af00b 100644 --- a/.gitignore +++ b/.gitignore @@ -152,6 +152,11 @@ docs/superpowers/* .update-incomplete .update-incomplete.lock +# Checkout fingerprint the __pycache__ tree was last validated against +# (launch-time stale-bytecode sweep). Runtime state, never a code change. +.bytecode-fingerprint +.bytecode-fingerprint.tmp + # Installer-written method stamp in the managed checkout root (scripts/install.sh). # Runtime metadata only — never a code change. Ignore so `git status` stays clean # and `hermes update`'s untracked autostash does not treat it as a local edit (#66189 / #54855). diff --git a/hermes_cli/main.py b/hermes_cli/main.py index aff5ec07d817..2ccfe808d4fe 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -4792,6 +4792,76 @@ def _reload_updated_runtime_modules() -> None: logger.debug("Could not refresh update runtime modules: %s", exc) +# Stamp file recording the checkout fingerprint the bytecode cache was last +# validated against. Lives next to the checkout (NOT in HERMES_HOME) because +# __pycache__ is per-checkout state shared by every profile. +_BYTECODE_FINGERPRINT_FILE = ".bytecode-fingerprint" + + +def _record_bytecode_fingerprint() -> None: + """Persist the current checkout fingerprint after a bytecode sweep. + + Never raises. A failed write just means the next launch re-sweeps — + safe, merely redundant. + """ + try: + fingerprint = _read_git_revision_fingerprint(PROJECT_ROOT) + if not fingerprint: + return + stamp_path = PROJECT_ROOT / _BYTECODE_FINGERPRINT_FILE + tmp_path = stamp_path.with_name(stamp_path.name + ".tmp") + tmp_path.write_text(fingerprint, encoding="utf-8") + tmp_path.replace(stamp_path) + except OSError as exc: + logger.debug("Could not record bytecode fingerprint: %s", exc) + + +def _sweep_stale_bytecode_if_checkout_changed() -> None: + """Clear ``__pycache__`` at launch when the checkout changed underneath us. + + The stale-bytecode bug class (issues #6207, #60242; Dhruv's WhatsApp + ``cannot import name 'parse_model_flags_detailed'`` report) has one + shared shape: the checkout's ``.py`` files change (git pull inside + ``hermes update``, a manual ``git pull``, a ZIP update, a file-sync + restore) while ``__pycache__`` retains bytecode from the previous + revision, and a later process trusts the stale ``.pyc`` instead of the + fresh source. + + Update-time clears alone can never close this class: ``hermes update`` + always executes the PRE-pull updater code, so any hardening added to it + only takes effect one update late, and manual ``git pull`` never runs + the updater at all. This launch-time guard closes the loop: every + ``hermes`` entry point compares the checkout fingerprint (cheap file + reads, no git subprocess) against the last-validated stamp and sweeps + the bytecode cache once when they diverge. + + Never raises — a failure here must not block launch. + """ + try: + fingerprint = _read_git_revision_fingerprint(PROJECT_ROOT) + if not fingerprint: + return # non-git install — the ZIP update path clears explicitly + stamp_path = PROJECT_ROOT / _BYTECODE_FINGERPRINT_FILE + try: + recorded = stamp_path.read_text(encoding="utf-8").strip() + except OSError: + recorded = "" + if recorded == fingerprint: + return + removed = _clear_bytecode_cache(PROJECT_ROOT) + if removed: + logger.info( + "Checkout changed since last launch (%s -> %s): cleared %d stale __pycache__ director%s", + recorded or "unknown", + fingerprint, + removed, + "y" if removed == 1 else "ies", + ) + _record_bytecode_fingerprint() + except Exception as exc: + logger.debug("Stale-bytecode launch sweep failed: %s", exc) + + # Critical files that Hermes must be able to import immediately after an # update/install. Most are imported on every CLI startup; ``web_server.py`` # is the desktop/dashboard backend path that a fresh Windows install launches @@ -7810,6 +7880,7 @@ def _update_via_zip(args): print( f" ✓ Cleared {removed} stale __pycache__ director{'y' if removed == 1 else 'ies'}" ) + _record_bytecode_fingerprint() # Reinstall Python dependencies. Prefer .[all], but if one optional extra # breaks on this machine, keep base deps and reinstall the remaining extras @@ -12308,6 +12379,7 @@ def _cmd_update_impl(args, gateway_mode: bool): print( f" ✓ Cleared {removed} stale __pycache__ director{'y' if removed == 1 else 'ies'}" ) + _record_bytecode_fingerprint() # Fork upstream sync logic (only for main branch on forks) if is_fork and branch == "main": @@ -12396,6 +12468,7 @@ def _cmd_update_impl(args, gateway_mode: bool): print( f" ✓ Cleared {removed} stale __pycache__ director{'y' if removed == 1 else 'ies'}" ) + _record_bytecode_fingerprint() _reload_updated_runtime_modules() # Upgrade pip before lazy refreshes — stale pip can fail source builds @@ -15587,6 +15660,12 @@ def main(): except Exception: pass + # If the checkout changed since the last launch (hermes update, manual + # git pull, old-updater update that predates newer clears), sweep stale + # __pycache__ once so no process — this one's lazy imports included — + # resolves fresh source against old bytecode. Never raises. + _sweep_stale_bytecode_if_checkout_changed() + # Self-heal a venv left half-built by an interrupted ``hermes update`` # (Ctrl-C, terminal close, WSL OOM mid-install). Skip when the user is # *running* update — that flow writes and clears its own marker, and we diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index 07f2f3198a9b..8bc1edd9d05e 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -660,6 +660,11 @@ def cmd_update(name: str) -> None: console.print(f"[red]Error:[/red] {output}") sys.exit(1) + # Same stale-bytecode class as the main checkout (#6207/#60242): the + # pull just changed .py files under this plugin dir, so drop any + # __pycache__ compiled from the previous revision. + _clear_plugin_bytecode(target) + # Copy any new .example files _copy_example_files(target, console) @@ -1954,6 +1959,10 @@ def dashboard_update_user_plugin(name: str) -> dict[str, Any]: if not ok: return {"ok": False, "error": msg} + # Sibling of the CLI ``hermes plugins update`` path: drop bytecode + # compiled from the pre-pull plugin revision. + _clear_plugin_bytecode(target) + from rich.console import Console _copy_example_files(target, Console()) @@ -1961,6 +1970,30 @@ def dashboard_update_user_plugin(name: str) -> dict[str, Any]: return {"ok": True, "name": name, "output": msg, "unchanged": unchanged} +def _clear_plugin_bytecode(target: Path) -> int: + """Remove ``__pycache__`` dirs under a just-updated plugin checkout. + + Plugin dirs live outside the main repo, so the launch-time checkout + fingerprint sweep in ``hermes_cli.main`` never covers them. After a + ``git pull`` changes a plugin's ``.py`` files, stale bytecode here can + produce the same ImportError class as #6207/#60242 in whichever + process imports the plugin next. Never raises. + """ + removed = 0 + try: + for cache_dir in target.rglob("__pycache__"): + if not cache_dir.is_dir(): + continue + try: + shutil.rmtree(cache_dir) + removed += 1 + except OSError: + pass + except OSError: + pass + return removed + + def _git_pull_plugin_dir(target: Path) -> tuple[bool, str]: git_exe = _resolve_git_executable() if not git_exe: diff --git a/tests/hermes_cli/test_bytecode_sweep.py b/tests/hermes_cli/test_bytecode_sweep.py new file mode 100644 index 000000000000..59bc03d955cb --- /dev/null +++ b/tests/hermes_cli/test_bytecode_sweep.py @@ -0,0 +1,164 @@ +"""Tests for the launch-time stale-bytecode sweep (checkout fingerprint guard). + +Bug class: the checkout's ``.py`` files change (``hermes update``, manual +``git pull``, ZIP update) while ``__pycache__`` retains bytecode compiled +from the previous revision; the next process to import trusts the stale +``.pyc`` and dies with ``cannot import name ...`` (#6207, #60242). + +The launch-time guard compares the current checkout fingerprint against the +last-validated stamp and sweeps ``__pycache__`` once when they diverge — +covering paths no update-time clear can reach (manual pulls, pre-hardening +updaters). +""" + +from pathlib import Path + +from hermes_cli import main as hermes_main + + +def _make_repo(tmp_path: Path, sha: str = "a" * 40) -> Path: + """Minimal git checkout layout that _read_git_revision_fingerprint groks.""" + repo = tmp_path / "repo" + git_dir = repo / ".git" + (git_dir / "refs" / "heads").mkdir(parents=True) + (git_dir / "HEAD").write_text("ref: refs/heads/main\n", encoding="utf-8") + (git_dir / "refs" / "heads" / "main").write_text(sha + "\n", encoding="utf-8") + return repo + + +def _make_pycache(repo: Path, subdir: str = "hermes_cli") -> Path: + cache = repo / subdir / "__pycache__" + cache.mkdir(parents=True) + (cache / "main.cpython-311.pyc").write_bytes(b"stale") + return cache + + +def test_sweep_clears_pycache_when_checkout_changed(monkeypatch, tmp_path): + repo = _make_repo(tmp_path, sha="b" * 40) + cache = _make_pycache(repo) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + # Stamp records a different (older) fingerprint. + (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).write_text( + "git:refs/heads/main:" + "a" * 40, encoding="utf-8" + ) + + hermes_main._sweep_stale_bytecode_if_checkout_changed() + + assert not cache.exists() + # Stamp updated to the current fingerprint. + recorded = (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).read_text(encoding="utf-8") + assert recorded.strip().endswith("b" * 40) + + +def test_sweep_noop_when_fingerprint_matches(monkeypatch, tmp_path): + repo = _make_repo(tmp_path, sha="c" * 40) + cache = _make_pycache(repo) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + fingerprint = hermes_main._read_git_revision_fingerprint(repo) + assert fingerprint is not None + (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).write_text(fingerprint, encoding="utf-8") + + hermes_main._sweep_stale_bytecode_if_checkout_changed() + + assert cache.exists() # untouched — no needless recompiles on every launch + + +def test_sweep_first_launch_clears_and_records(monkeypatch, tmp_path): + """No stamp yet (first launch with the guard) → sweep once, record.""" + repo = _make_repo(tmp_path, sha="d" * 40) + cache = _make_pycache(repo) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + + hermes_main._sweep_stale_bytecode_if_checkout_changed() + + assert not cache.exists() + assert (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).exists() + + +def test_sweep_noop_on_non_git_install(monkeypatch, tmp_path): + """No .git → no fingerprint → guard must not touch anything or raise.""" + repo = tmp_path / "repo" + repo.mkdir() + cache = _make_pycache(repo) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + + hermes_main._sweep_stale_bytecode_if_checkout_changed() + + assert cache.exists() + assert not (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).exists() + + +def test_sweep_never_raises_on_unreadable_stamp(monkeypatch, tmp_path): + repo = _make_repo(tmp_path, sha="e" * 40) + cache = _make_pycache(repo) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + stamp = repo / hermes_main._BYTECODE_FINGERPRINT_FILE + stamp.mkdir() # a directory where a file is expected → OSError on read + + hermes_main._sweep_stale_bytecode_if_checkout_changed() # must not raise + + # Unreadable stamp is treated as "changed": cache swept. + assert not cache.exists() + + +def test_record_bytecode_fingerprint_writes_atomically(monkeypatch, tmp_path): + repo = _make_repo(tmp_path, sha="f" * 40) + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + + hermes_main._record_bytecode_fingerprint() + + stamp = repo / hermes_main._BYTECODE_FINGERPRINT_FILE + assert stamp.read_text(encoding="utf-8").endswith("f" * 40) + assert not stamp.with_name(stamp.name + ".tmp").exists() + + +def test_record_bytecode_fingerprint_noop_without_git(monkeypatch, tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + + hermes_main._record_bytecode_fingerprint() # must not raise + + assert not (repo / hermes_main._BYTECODE_FINGERPRINT_FILE).exists() + + +def test_sweep_skips_venv_and_git_dirs(monkeypatch, tmp_path): + """The underlying clear must not touch venv/node_modules bytecode.""" + repo = _make_repo(tmp_path, sha="9" * 40) + repo_cache = _make_pycache(repo, "hermes_cli") + venv_cache = repo / "venv" / "lib" / "__pycache__" + venv_cache.mkdir(parents=True) + (venv_cache / "x.pyc").write_bytes(b"keep") + monkeypatch.setattr(hermes_main, "PROJECT_ROOT", repo) + + hermes_main._sweep_stale_bytecode_if_checkout_changed() + + assert not repo_cache.exists() + assert venv_cache.exists() + +# --------------------------------------------------------------------------- +# Plugin-update sibling site: __pycache__ under ~/.hermes/plugins/ +# --------------------------------------------------------------------------- + +def test_clear_plugin_bytecode_removes_nested_caches(tmp_path): + from hermes_cli import plugins_cmd + + plugin = tmp_path / "myplugin" + top = plugin / "__pycache__" + nested = plugin / "sub" / "__pycache__" + top.mkdir(parents=True) + nested.mkdir(parents=True) + (top / "a.pyc").write_bytes(b"stale") + (nested / "b.pyc").write_bytes(b"stale") + + removed = plugins_cmd._clear_plugin_bytecode(plugin) + + assert removed == 2 + assert not top.exists() + assert not nested.exists() + + +def test_clear_plugin_bytecode_never_raises_on_missing_dir(tmp_path): + from hermes_cli import plugins_cmd + + assert plugins_cmd._clear_plugin_bytecode(tmp_path / "nope") == 0 From 4b4d2ae4cd4cb405f6e51a2be57835c4523bdafb Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sun, 26 Jul 2026 23:44:25 -0700 Subject: [PATCH 14/19] docs(delegation,api): document stall detection, timeout metadata, /agents live status, runs-stream subagent lifecycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Catch the docs up to this week's delegation work: - delegation.md: structured timeout metadata fields (timeout_seconds/ timed_out_after_seconds/timeout_phase, #72403); new 'Stall Detection for Background Subagents' section (progress-based monitor, thresholds, grace window, stalled event metadata, root-cause fix note — #72227/ #72300/#72412); /agents per-child live activity on CLI + gateway; all-thread diagnostic dump note. - api-server.md: /v1/runs events stream now documents subagent.start/ subagent.complete forwarding, redaction, child_session_id, and the deliberate exclusion of per-tool child events (#72406). - sidebars.ts: register the subagent-lifecycle-api developer guide page (shipped in #72501 but absent from the sidebar). Docusaurus build verified. --- .../docs/user-guide/features/api-server.md | 11 ++++ .../docs/user-guide/features/delegation.md | 62 ++++++++++++++++++- website/sidebars.ts | 1 + 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/website/docs/user-guide/features/api-server.md b/website/docs/user-guide/features/api-server.md index 0311e2a228bc..8380d6313145 100644 --- a/website/docs/user-guide/features/api-server.md +++ b/website/docs/user-guide/features/api-server.md @@ -375,6 +375,17 @@ Statuses are retained briefly after terminal states (`completed`, `failed`, or ` Server-Sent Events stream of the run's tool-call progress, token deltas, and lifecycle events. Designed for dashboards and thick clients that want to attach/detach without losing state. +When the agent delegates work to background subagents, the stream also carries +`subagent.start` and `subagent.complete` lifecycle events, so clients can +observe delegation outcomes — including timeouts and failures — instead of the +run going silent while a child works. The `subagent.complete` payload carries +the child's status, summary, duration, token/cost figures, and a +`child_session_id` for correlation; free-text fields pass forced secret +redaction before leaving the process. Per-tool child events +(`subagent.tool`, progress ticks) are intentionally **not** forwarded — they +are high-volume UI noise; use the per-child live transcript files for +play-by-play. + Unconsumed event buffers expire after five minutes so a detached client cannot grow memory indefinitely. This expires transport state only: a run that is still executing remains visible to status polling, approval, stop control, and diff --git a/website/docs/user-guide/features/delegation.md b/website/docs/user-guide/features/delegation.md index 4790ce3b4200..d40a0dfc107b 100644 --- a/website/docs/user-guide/features/delegation.md +++ b/website/docs/user-guide/features/delegation.md @@ -191,10 +191,54 @@ delegation: A positive value enforces a hard wall-clock limit on each child; `0` or a negative value disables it. +When a configured cap fires, the child's result carries structured timeout +metadata alongside the error message so parents and hooks can distinguish a +stopwatch kill from other failures without parsing text: `timeout_seconds` +(the configured cap), `timed_out_after_seconds` (actual wall clock), and +`timeout_phase` (`before_first_llm_call` when the child never reached its +first request, `after_llm_calls` otherwise). All three are `null` on +non-timeout errors. + :::tip Diagnostic dump on zero-call timeout -With a hard cap configured, if a subagent times out having made **zero** API calls (usually: provider unreachable, auth failure, or tool-schema rejection), `delegate_task` writes a structured diagnostic to `~/.hermes/logs/subagent-timeout--.log` containing the subagent's config snapshot, credential-resolution trace, and any early error messages. Much easier to root-cause than the previous silent-timeout behavior. +With a hard cap configured, if a subagent times out having made **zero** API calls (usually: provider unreachable, auth failure, or tool-schema rejection), `delegate_task` writes a structured diagnostic to `~/.hermes/logs/subagent-timeout--.log` containing the subagent's config snapshot, credential-resolution trace, any early error messages, and stack traces for **all** live threads (not just the child's own) — a child parked waiting on a nested helper thread is indistinguishable from a slow provider without the full picture. ::: +## Stall Detection for Background Subagents + +Background delegations (`delegate_task(background=true)`) are watched by a +**progress-based stall monitor** — on by default, zero config. Unlike a +wall-clock timeout, it never touches a child that is making progress, no +matter how long it runs. + +The monitor samples each detached child's progress signals — API-call count, +current tool, and last-activity timestamp (which ticks on **every streamed +token**, tool transition, and API-call boundary, so a child mid-stream on a +long response always counts as alive): + +1. **Progressing children are never touched.** Any advancing signal resets + the clock. +2. A child whose progress is completely frozen past the stale threshold + (450s idle, 1200s while inside a tool — legitimately slow terminal + commands and web fetches get the higher ceiling) is **interrupted** and + given a 120s grace window. A child that unwinds in time delivers its + partial results through the normal completion path. +3. A child that never returns is force-finalized with a terminal `stalled` + completion event, so the owning session hears an outcome instead of + going silent, and the async slot frees for new work. + +The `stalled` event carries structured metadata mirroring the sync-path +timeout fields: `stalled_after_quiet_seconds`, `stall_threshold_seconds`, +`stall_phase` (`idle` / `in_tool`), and `stall_grace_seconds`. + +This closed a long-standing failure mode where a wedged background child +left its session looking dead until a process restart. The underlying wedge +(children hanging at their first API call after multi-day gateway uptime) +was also fixed at the root: delegated children now run their OpenAI-wire +API requests inline on their own conversation thread instead of a nested +worker thread — the layer where the wedge lived. The stall monitor remains +as the safety net for anything else. + + ## Monitoring Running Subagents (`/agents`) The TUI ships a `/agents` overlay (alias `/tasks`) that turns recursive `delegate_task` fan-out into a first-class audit surface: @@ -206,6 +250,22 @@ The TUI ships a `/agents` overlay (alias `/tasks`) that turns recursive `delegat The classic CLI just prints `/agents` as a text summary; the TUI is where the overlay shines. See [TUI — Slash commands](/user-guide/tui#slash-commands). +On the classic CLI and every gateway platform (Telegram, Discord, Slack, ...), +`/agents` also lists **background delegations with live per-child activity**, +sampled directly from each running child: + +``` +Background delegations: 1 running +- deleg_ab12cd34 · running · research the delegation stall monitor + - child 1: 4 api calls · in web_search · active 12s ago + - child 2: 7 api calls · between turns · active 3s ago +``` + +A delegation the stall monitor has flagged shows as +`stalling · no progress 450s — interrupting`, and long-quiet-but-healthy +children show their quiet time so you can tell "slow" from "stuck" at a +glance. + ## Live Transcripts Every `delegate_task` dispatch also creates one **append-only, human-readable log per task** so you (or the parent agent) can watch a subagent work in real time instead of waiting for the consolidated summary: diff --git a/website/sidebars.ts b/website/sidebars.ts index 7314af5a2b38..87f7a7e8d898 100644 --- a/website/sidebars.ts +++ b/website/sidebars.ts @@ -750,6 +750,7 @@ const sidebars: SidebarsConfig = { link: {type: 'doc', id: 'developer-guide/plugins/index'}, items: [ 'developer-guide/plugin-llm-access', + 'developer-guide/subagent-lifecycle-api', 'developer-guide/desktop-plugin-sdk', 'developer-guide/memory-provider-plugin', 'developer-guide/context-engine-plugin', From 8b112497557392f9f4e116961c7cd16a7742ff56 Mon Sep 17 00:00:00 2001 From: brein Date: Mon, 27 Jul 2026 15:28:26 +0200 Subject: [PATCH 15/19] fix(tui-gateway): guard entry signal installs to main thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Importing tui_gateway.entry from a worker thread raised 'ValueError: signal only works in main thread of the main interpreter' because signal.signal() was called unconditionally at import time. On the Desktop/WebSocket path, server._build() runs in a daemon thread and does 'from tui_gateway.entry import ensure_mcp_discovery_started' as the first import of entry (entry.main() is never run there), which crashed and aborted MCP discovery startup — every session then lost its MCP servers (e.g. Dart-mcp) with ClosedResourceError. signal handlers are process-global, so installing them only when the module is first imported in the main thread is sufficient; importing from a worker thread becomes a safe no-op. Fixes #72667 --- .../test_entry_import_off_main_thread.py | 85 +++++++++++++++++++ tui_gateway/entry.py | 48 +++++++++-- 2 files changed, 125 insertions(+), 8 deletions(-) create mode 100644 tests/tui_gateway/test_entry_import_off_main_thread.py diff --git a/tests/tui_gateway/test_entry_import_off_main_thread.py b/tests/tui_gateway/test_entry_import_off_main_thread.py new file mode 100644 index 000000000000..478053a8e5ad --- /dev/null +++ b/tests/tui_gateway/test_entry_import_off_main_thread.py @@ -0,0 +1,85 @@ +"""Regression test: importing tui_gateway.entry off the main thread must not crash. + +Background +---------- +``entry.py`` installs signal handlers (SIGPIPE, SIGTERM, …) at *import time*. +``signal.signal`` is only legal in the main thread, so a first import of +``entry`` from a worker thread raises +``ValueError: signal only works in main thread of the main interpreter``. + +The Desktop/WebSocket agent-build path (``server._build``) runs in a daemon +thread and does ``from tui_gateway.entry import ensure_mcp_discovery_started`` +(server.py:1866). On that path ``entry.main()`` is never run, so the worker +thread performs the *first* import of ``entry`` — which crashed and aborted +MCP discovery startup (#72667, regression from the 2026-07-26 websocket-MCP +discovery fix). + +Fix: guard the import-time signal installs with a main-thread check. Handlers +are process-global, so installing them only when reached in the main thread is +sufficient; importing from a worker thread becomes a safe no-op. + +This test runs the import in a *fresh* subprocess worker thread to guarantee a +clean first-import (the test process already imported entry in its main +thread, so an in-process import would not reproduce the bug). +""" + +import subprocess +import sys + +REPO_ROOT = "." + + +def _spawn_worker_import_entry(): + """Run `import tui_gateway.entry` for the first time inside a worker thread. + + Returns (returncode, stdout, stderr). + """ + code = ( + "import threading, sys, signal, os\n" + "sys.stdout.reconfigure(line_buffering=True)\n" + "sys.stderr.reconfigure(line_buffering=True)\n" + "errs = []\n" + "def _worker():\n" + " try:\n" + " import tui_gateway.entry\n" + " except Exception as e:\n" + " errs.append(repr(e))\n" + "t = threading.Thread(target=_worker, daemon=True)\n" + "t.start(); t.join(timeout=15)\n" + "if errs:\n" + " sys.stdout.write('IMPORT_FAILED: ' + errs[0] + '\\n')\n" + " sys.exit(2)\n" + "# main thread of this process still installs SIGPIPE handler\n" + "h = signal.getsignal(signal.SIGPIPE)\n" + "sys.stdout.write('OK handler_installed=' + str(h is signal.SIG_IGN or callable(h)) + '\\n')\n" + "sys.exit(0)\n" + ) + proc = subprocess.run( + [sys.executable, "-c", code], + cwd=REPO_ROOT, + capture_output=True, + text=True, + timeout=60, + env={**__import__("os").environ}, + ) + return proc.returncode, proc.stdout, proc.stderr + + +def test_entry_imports_cleanly_from_worker_thread(): + """First import of tui_gateway.entry from a worker thread must succeed.""" + rc, out, err = _spawn_worker_import_entry() + # entry import may emit to stdout or stderr depending on the runner; check both. + combined = out + err + assert rc == 0, f"entry import off main thread failed (rc={rc}): {err!r}" + assert "OK" in combined, f"unexpected output: {out!r} / {err!r}" + + +def test_entry_installs_sigpipe_handler_in_main_thread(): + """Even though it can be imported off-thread, the main-thread path still + installs the SIGPIPE handler (process-global, so it applies everywhere).""" + rc, out, err = _spawn_worker_import_entry() + combined = out + err + assert rc == 0, f"entry import off main thread failed (rc={rc}): {err!r}" + assert "handler_installed=True" in combined, ( + f"SIGPIPE handler not installed: {out!r} / {err!r}" + ) diff --git a/tui_gateway/entry.py b/tui_gateway/entry.py index bf41c1201816..2b8ccee8838a 100644 --- a/tui_gateway/entry.py +++ b/tui_gateway/entry.py @@ -13,6 +13,7 @@ hermes_bootstrap.harden_import_path() import json import logging import signal +import threading import time import traceback @@ -184,18 +185,49 @@ def _log_signal(signum: int, frame) -> None: # with hasattr so ``python -m tui_gateway.entry`` (spawned by # ``hermes --tui``) imports cleanly there. SIGBREAK (Windows' Ctrl+Break) # is installed when available as a weaker equivalent of SIGHUP. -if hasattr(signal, "SIGPIPE"): - signal.signal(signal.SIGPIPE, signal.SIG_IGN) -if hasattr(signal, "SIGTERM"): - signal.signal(signal.SIGTERM, _log_signal) +# +# signal.signal() is only legal in the MAIN thread. On the Desktop/WebSocket +# agent-build path, server._build() runs in a daemon thread and does +# ``from tui_gateway.entry import ensure_mcp_discovery_started`` as the first +# import of entry (entry.main() is never run there), which used to raise +# "ValueError: signal only works in main thread of the main interpreter" and +# abort MCP discovery startup. Install each handler only when we're in the +# main thread: handlers are process-global, so a main-thread import anywhere +# in the process still installs them for everyone, and an off-thread import +# (Desktop build path) simply no-ops instead of crashing the import. This +# preserves the original SIG_IGN/SIG_DFL behavior on the classic TUI/serve +# path while fixing the off-thread import crash. + + +def _install_signal(signame, handler): + """Install a signal handler if legal in this thread. + + signal.signal() raises ValueError outside the main thread; skip silently + there so a worker-thread import of this module (Desktop build path) does + not abort. On any main-thread import the handler is installed as before. + """ + if threading.current_thread() is not threading.main_thread(): + return + sig = getattr(signal, signame, None) + if sig is None: + return # Windows: SIGPIPE/SIGHUP absent + try: + signal.signal(sig, handler) + except (ValueError, OSError, RuntimeError): + # Not in the main thread despite the check, or handler rejected. + # Skip rather than crash the import (see above). + pass + + +_install_signal("SIGPIPE", signal.SIG_IGN) +_install_signal("SIGTERM", _log_signal) if hasattr(signal, "SIGHUP"): - signal.signal(signal.SIGHUP, _log_signal) + _install_signal("SIGHUP", _log_signal) elif hasattr(signal, "SIGBREAK"): # Windows-only: Ctrl+Break in a console window delivers SIGBREAK. # Route it through the same handler so kills are diagnosable. - signal.signal(signal.SIGBREAK, _log_signal) -if hasattr(signal, "SIGINT"): - signal.signal(signal.SIGINT, signal.SIG_IGN) + _install_signal("SIGBREAK", _log_signal) +_install_signal("SIGINT", signal.SIG_IGN) def _log_exit(reason: str) -> None: From e643f2e9102a561ea38cd805b81d87c8a2571dac Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 27 Jul 2026 23:28:55 +0500 Subject: [PATCH 16/19] fix: update signal guard test for _install_signal refactor (#72677) The change-detector test asserted 'hasattr(signal, "SIGPIPE")' in source. PR #72677 replaced inline hasattr+signal.signal with _install_signal() which does the same guard via getattr internally. Update the test to accept either form. --- tests/tools/test_windows_native_support.py | 24 ++++++++++++++-------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/tests/tools/test_windows_native_support.py b/tests/tools/test_windows_native_support.py index b4187cf02a02..3b8a52fe8306 100644 --- a/tests/tools/test_windows_native_support.py +++ b/tests/tools/test_windows_native_support.py @@ -666,15 +666,21 @@ class TestTuiGatewayEntrySignalGuards: def test_source_guards_each_signal_installation(self): root = Path(__file__).resolve().parents[2] source = (root / "tui_gateway" / "entry.py").read_text(encoding="utf-8") - # Every signal.signal(...) at module scope must be preceded by a - # hasattr check. We look at the text: no bare "signal.signal(" - # call should appear outside a function body without a guard. - # Simpler heuristic: all SIGPIPE / SIGHUP references outside the - # dict-building loop must be wrapped in hasattr. - assert 'hasattr(signal, "SIGPIPE")' in source - assert 'hasattr(signal, "SIGHUP")' in source - assert 'hasattr(signal, "SIGTERM")' in source - assert 'hasattr(signal, "SIGINT")' in source + # Every signal installation at module scope must be guarded against + # missing signals (Windows: SIGPIPE/SIGHUP absent). Originally this + # was ``hasattr(signal, "SIGPIPE")`` inline; PR #72677 refactored to + # ``_install_signal("SIGPIPE", ...)`` which does the same guard via + # ``getattr(signal, signame, None)`` internally. Either form is + # acceptable — what matters is no bare ``signal.signal(SIGPIPE)`` + # at module scope without a guard. + for sig_name in ("SIGPIPE", "SIGHUP", "SIGTERM", "SIGINT"): + assert ( + f'hasattr(signal, "{sig_name}")' in source + or f'_install_signal("{sig_name}"' in source + ), ( + f"signal {sig_name} must be installed via a guarded path " + f"(hasattr or _install_signal), not bare signal.signal()" + ) def test_module_imports_cleanly(self): """Importing the module must not raise — verifies the guards work.""" From 858bedea028857b42438d3029d31921f3aac8b99 Mon Sep 17 00:00:00 2001 From: elcocoel Date: Mon, 27 Jul 2026 04:09:41 +0100 Subject: [PATCH 17/19] fix(session): persist tool activity before projection --- agent/conversation_loop.py | 36 ++- agent/tool_executor.py | 173 ++++++++----- run_agent.py | 6 +- .../test_tool_call_incremental_persistence.py | 241 ++++++++++++++++++ 4 files changed, 385 insertions(+), 71 deletions(-) diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index cbeee7a1df83..b6f7c3218b3b 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -1061,6 +1061,9 @@ def run_conversation( # Commentary deduplication spans all provider continuations and tool calls # within one user turn, but must not suppress the same phrase next turn. agent._delivered_interim_texts = set() + # A configured SessionDB append failure halts only the affected turn. A + # cached gateway agent must recover on the next message if storage did. + agent._incremental_persistence_failed = False # Main conversation loop counters (pure locals consumed by the loop below). api_call_count = 0 @@ -5785,8 +5788,6 @@ def run_conversation( and previous_interim_visible == current_interim_visible ) messages.append(assistant_msg) - if not duplicate_previous_interim: - agent._emit_interim_assistant_message(assistant_msg) # Mixed batch: error-result the invalid calls and strip them # from the execution set. The assistant message above keeps @@ -5808,13 +5809,17 @@ def run_conversation( if tc.function.name in agent.valid_tool_names ] + _tool_turn_persisted = None try: # Persist the assistant tool-call turn before any tool # side effects run. If a destructive tool restarts or # terminates Hermes mid-turn, resume logic still sees the # exact tool-call block that already executed. - agent._flush_messages_to_session_db(messages, conversation_history) + _tool_turn_persisted = agent._flush_messages_to_session_db( + messages, conversation_history + ) except Exception as exc: + _tool_turn_persisted = False logger.warning( "Incremental tool-call persistence failed before execution " "(session=%s): %s", @@ -5822,6 +5827,22 @@ def run_conversation( exc, ) + if _tool_turn_persisted is False: + # The canonical append failed. Do not project the row or + # run side-effecting tools from state that exists only in + # this process. Breaking also avoids retrying the same + # unpersisted turn until the iteration budget is exhausted. + _turn_exit_reason = "session_persistence_failed" + final_response = "" + failed = True + break + + # A UI must never observe an assistant/tool-call row that is + # still only an ephemeral in-memory projection. Emit interim + # commentary only after the canonical SessionDB append above. + if not duplicate_previous_interim: + agent._emit_interim_assistant_message(assistant_msg) + # Close any open streaming display (response box, reasoning # box) before tool execution begins. Intermediate turns may # have streamed early content that opened the response box; @@ -5836,6 +5857,15 @@ def run_conversation( agent._execute_tool_calls(assistant_message, messages, effective_task_id, api_call_count) + if getattr(agent, "_incremental_persistence_failed", False): + # A tool result could not be made canonical. Do not send + # the in-memory result back to the model or project any + # later events from this turn. + _turn_exit_reason = "session_persistence_failed" + final_response = "" + failed = True + break + if agent._tool_guardrail_halt_decision is not None: decision = agent._tool_guardrail_halt_decision _turn_exit_reason = "guardrail_halt" diff --git a/agent/tool_executor.py b/agent/tool_executor.py index 74092b90ee91..d32fe99c0c55 100644 --- a/agent/tool_executor.py +++ b/agent/tool_executor.py @@ -140,8 +140,8 @@ def _flush_session_db_after_tool_progress( messages: list, *, stage: str, -) -> None: - """Best-effort incremental SessionDB flush for tool-call progress. +) -> bool: + """Flush tool-call progress before projecting it to any UI surface. Tool execution can perform side effects that terminate or restart the current Hermes process before the normal turn-end persistence path runs. @@ -149,9 +149,14 @@ def _flush_session_db_after_tool_progress( transcript survives destructive-but-valid tool calls. """ try: - agent._flush_messages_to_session_db(messages) + persisted = agent._flush_messages_to_session_db(messages) is not False + if not persisted: + agent._incremental_persistence_failed = True + return persisted except Exception as exc: + agent._incremental_persistence_failed = True logger.warning("Incremental tool-call persistence failed after %s: %s", stage, exc) + return False def _ra(): @@ -861,6 +866,8 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe for i, (tc, name, args, middleware_trace, block_result, blocked_by_guardrail) in enumerate(parsed_calls): r = results[i] blocked = False + is_error = True + progress_function_name = name # A worker can finish and write results[i] in the window between the # deadline snapshot (timed_out_indices, taken from not_done) and this # loop. Prefer that real result over a fabricated timeout message — the @@ -916,6 +923,7 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe tool_duration = 0.0 else: function_name, function_args, function_result, tool_duration, is_error, blocked, middleware_trace = r + progress_function_name = function_name if blocked: effect_disposition = "none" @@ -943,44 +951,15 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe except Exception as _ver_err: logging.debug("file-mutation verifier record failed: %s", _ver_err) - if not blocked and agent.tool_progress_callback: - try: - agent.tool_progress_callback( - "tool.completed", function_name, None, None, - duration=tool_duration, is_error=is_error, - result=function_result, - ) - except Exception as cb_err: - logging.debug(f"Tool progress callback error: {cb_err}") - if agent.verbose_logging: logging.debug(f"Tool {function_name} completed in {tool_duration:.2f}s") logging.debug(f"Tool result ({len(function_result)} chars): {function_result}") - # Print cute message per tool - if agent._should_emit_quiet_tool_messages(): - cute_msg = _get_cute_tool_message_impl(name, args, tool_duration, result=function_result) - agent._safe_print(f" {cute_msg}") - elif not agent.quiet_mode and getattr(agent, "tool_progress_mode", "all") != "off": - _preview_str = _multimodal_text_summary(function_result) - if agent.verbose_logging: - print(f" ✅ Tool {i+1} completed in {tool_duration:.2f}s") - print(agent._wrap_verbose("Result: ", _preview_str)) - else: - response_preview = _preview_str[:agent.log_prefix_chars] + "..." if len(_preview_str) > agent.log_prefix_chars else _preview_str - print(f" ✅ Tool {i+1} completed in {tool_duration:.2f}s - {response_preview}") - agent._current_tool = None _status_suffix = " (error)" if is_error else "" agent._touch_activity(f"tool completed: {name} ({tool_duration:.1f}s){_status_suffix}") - if not blocked and agent.tool_complete_callback: - try: - display_args = _redact_tool_args_for_display(name, args) or args - agent.tool_complete_callback(tc.id, name, display_args, function_result) - except Exception as cb_err: - logging.debug(f"Tool complete callback error: {cb_err}") - + display_function_result = function_result function_result = maybe_persist_tool_result( content=function_result, tool_name=name, @@ -1015,6 +994,50 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe ) messages.append(tool_message) risk_metadata = tool_message.get("_tool_output_risk") + if not _flush_session_db_after_tool_progress( + agent, + messages, + stage=f"tool result {name}", + ): + return + + # Every completion surface is downstream of the canonical append. If + # the UI bridge or process dies while projecting one of these events, + # resume can reconstruct the tool result that was already visible. + if not blocked and agent.tool_progress_callback: + try: + agent.tool_progress_callback( + "tool.completed", progress_function_name, None, None, + duration=tool_duration, is_error=is_error, + result=display_function_result, + ) + except Exception as cb_err: + logging.debug(f"Tool progress callback error: {cb_err}") + + # Print cute message per tool + if agent._should_emit_quiet_tool_messages(): + cute_msg = _get_cute_tool_message_impl( + name, args, tool_duration, result=display_function_result, + ) + agent._safe_print(f" {cute_msg}") + elif not agent.quiet_mode and getattr(agent, "tool_progress_mode", "all") != "off": + _preview_str = _multimodal_text_summary(display_function_result) + if agent.verbose_logging: + print(f" ✅ Tool {i+1} completed in {tool_duration:.2f}s") + print(agent._wrap_verbose("Result: ", _preview_str)) + else: + response_preview = _preview_str[:agent.log_prefix_chars] + "..." if len(_preview_str) > agent.log_prefix_chars else _preview_str + print(f" ✅ Tool {i+1} completed in {tool_duration:.2f}s - {response_preview}") + + if not blocked and agent.tool_complete_callback: + try: + display_args = _redact_tool_args_for_display(name, args) or args + agent.tool_complete_callback( + tc.id, name, display_args, display_function_result, + ) + except Exception as cb_err: + logging.debug(f"Tool complete callback error: {cb_err}") + if ( risk_metadata is not None and risk_metadata.get("risk") != "low" @@ -1031,11 +1054,6 @@ def execute_tool_calls_concurrent(agent, assistant_message, messages: list, effe ) except Exception as cb_err: logging.debug("Tool output risk callback error: %s", cb_err) - _flush_session_db_after_tool_progress( - agent, - messages, - stage=f"tool result {name}", - ) # ── Per-tool /steer drain ─────────────────────────────────── # Same as the sequential path: drain between each collected @@ -1067,6 +1085,8 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe # Resolve the context-scaled tool-output budget once per turn. _tool_budget = _budget_for_agent(agent) for i, tool_call in enumerate(assistant_message.tool_calls, 1): + if getattr(agent, "_incremental_persistence_failed", False): + return # SAFETY: check interrupt BEFORE starting each tool. # If the user sent "stop" during a previous tool's execution, # do NOT start any more tools -- skip them all immediately. @@ -1082,11 +1102,12 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe skipped_tc.id, effect_disposition="none", )) - _flush_session_db_after_tool_progress( + if not _flush_session_db_after_tool_progress( agent, messages, stage=f"cancelled tool result {skipped_name}", - ) + ): + return break function_name = tool_call.function.name @@ -1102,11 +1123,12 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe tool_call.id, ) ) - _flush_session_db_after_tool_progress( + if not _flush_session_db_after_tool_progress( agent, messages, stage=f"invalid tool arguments {function_name}", - ) + ): + return agent._apply_pending_steer_to_tool_results(messages, 1) continue @@ -1670,16 +1692,6 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe except Exception as _ver_err: logging.debug("file-mutation verifier record failed: %s", _ver_err) - if not _execution_blocked and agent.tool_progress_callback: - try: - agent.tool_progress_callback( - "tool.completed", function_name, None, None, - duration=tool_duration, is_error=_is_error_result, - result=function_result, - ) - except Exception as cb_err: - logging.debug(f"Tool progress callback error: {cb_err}") - agent._current_tool = None _status_suffix = " (error)" if _is_error_result else "" agent._touch_activity(f"tool completed: {function_name} ({tool_duration:.1f}s){_status_suffix}") @@ -1689,13 +1701,7 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe _log_result = _multimodal_text_summary(function_result) logging.debug(f"Tool result ({len(_log_result)} chars): {_log_result}") - if not _execution_blocked and agent.tool_complete_callback: - try: - display_args = _redact_tool_args_for_display(function_name, function_args) or function_args - agent.tool_complete_callback(tool_call.id, function_name, display_args, function_result) - except Exception as cb_err: - logging.debug(f"Tool complete callback error: {cb_err}") - + display_function_result = function_result function_result = maybe_persist_tool_result( content=function_result, tool_name=function_name, @@ -1718,6 +1724,40 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe tool_message = make_tool_result_message(function_name, _tool_content, tool_call.id) messages.append(tool_message) risk_metadata = tool_message.get("_tool_output_risk") + if not _flush_session_db_after_tool_progress( + agent, + messages, + stage=f"tool result {function_name}", + ): + return + + # UI completion/progress events are projections of the canonical tool + # row, never a competing in-memory authority. + if not _execution_blocked and agent.tool_progress_callback: + try: + agent.tool_progress_callback( + "tool.completed", function_name, None, None, + duration=tool_duration, is_error=_is_error_result, + result=display_function_result, + ) + except Exception as cb_err: + logging.debug(f"Tool progress callback error: {cb_err}") + + if not _execution_blocked and agent.tool_complete_callback: + try: + display_args = ( + _redact_tool_args_for_display(function_name, function_args) + or function_args + ) + agent.tool_complete_callback( + tool_call.id, + function_name, + display_args, + display_function_result, + ) + except Exception as cb_err: + logging.debug(f"Tool complete callback error: {cb_err}") + if ( risk_metadata is not None and risk_metadata.get("risk") != "low" @@ -1734,11 +1774,6 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe ) except Exception as cb_err: logging.debug("Tool output risk callback error: %s", cb_err) - _flush_session_db_after_tool_progress( - agent, - messages, - stage=f"tool result {function_name}", - ) # ── Per-tool /steer drain ─────────────────────────────────── # Drain pending steer BETWEEN individual tool calls so the @@ -1766,11 +1801,12 @@ def execute_tool_calls_sequential(agent, assistant_message, messages: list, effe skipped_tc.id, effect_disposition="none", )) - _flush_session_db_after_tool_progress( + if not _flush_session_db_after_tool_progress( agent, messages, stage=f"skipped tool result {skipped_name}", - ) + ): + return break if agent.tool_delay > 0 and i < len(assistant_message.tool_calls): @@ -1821,6 +1857,8 @@ def execute_tool_calls_segmented(agent, assistant_message, messages: list, effec segments = _plan_tool_batch_segments(assistant_message.tool_calls, execution_cwd=_exec_cwd) for kind, calls in segments: + if getattr(agent, "_incremental_persistence_failed", False): + return segment_message = SimpleNamespace(tool_calls=list(calls)) if kind == "parallel": execute_tool_calls_concurrent( @@ -1833,6 +1871,9 @@ def execute_tool_calls_segmented(agent, assistant_message, messages: list, effec finalize=False, ) + if getattr(agent, "_incremental_persistence_failed", False): + return + # ── Whole-turn finalize (budget + /steer) ───────────────────────── total_tools = len(assistant_message.tool_calls) if total_tools > 0: diff --git a/run_agent.py b/run_agent.py index aabaf78b87b7..5e97ed24c9c0 100644 --- a/run_agent.py +++ b/run_agent.py @@ -1918,9 +1918,9 @@ class AIAgent: # where the next live turn re-reads it as an instruction and the agent # "becomes" the curator. Hard-stop before any DB touch. if getattr(self, "_persist_disabled", False): - return + return None if not self._session_db: - return + return None # Persist user-message override (#48677 chokepoint): historically this # mutated the live `messages` list in place, which — on the early # crash-resilience persist that runs BEFORE the API call is built — @@ -2122,8 +2122,10 @@ class AIAgent: # allocated next turn at a recycled address. self._flushed_db_message_ids = set() self._last_flushed_db_idx = len(messages) + return True except Exception as e: logger.warning("Session DB append_message failed: %s", e) + return False def _get_messages_up_to_last_assistant(self, messages: List[Dict]) -> List[Dict]: """ diff --git a/tests/run_agent/test_tool_call_incremental_persistence.py b/tests/run_agent/test_tool_call_incremental_persistence.py index 34d4d79141d8..f9ceafec0620 100644 --- a/tests/run_agent/test_tool_call_incremental_persistence.py +++ b/tests/run_agent/test_tool_call_incremental_persistence.py @@ -28,7 +28,12 @@ from pathlib import Path import tempfile from unittest.mock import MagicMock, patch +import pytest + from agent.tool_dispatch_helpers import make_tool_result_message +from agent.agent_runtime_helpers import sanitize_api_messages +from agent.tool_executor import execute_tool_calls_segmented +from hermes_state import SessionDB from run_agent import AIAgent @@ -75,6 +80,31 @@ def _make_agent(): return agent +def _attach_real_session_db(agent, db_path: Path, session_id: str) -> SessionDB: + db = SessionDB(db_path=db_path) + db.create_session(session_id=session_id, source="tui", model="test/model") + agent._session_db = db + agent._session_db_created = True + agent.session_id = session_id + agent._last_flushed_db_idx = 0 + agent._flushed_db_message_ids = set() + agent._flushed_db_message_session_id = None + agent._persist_disabled = False + return db + + +def _durable_messages(db_path: Path, session_id: str) -> list[dict]: + restarted_db = SessionDB(db_path=db_path) + try: + return restarted_db.get_messages_as_conversation(session_id) + finally: + restarted_db.close() + + +def _durable_roles(db_path: Path, session_id: str) -> list[str]: + return [message["role"] for message in _durable_messages(db_path, session_id)] + + def _mock_tool_call(name="web_search", arguments="{}", call_id="call_1"): return SimpleNamespace( id=call_id, @@ -142,6 +172,78 @@ def test_run_conversation_flushes_assistant_tool_call_before_execution(): assert result["final_response"] == "done" +def test_interim_assistant_is_durable_before_ui_projection_on_abnormal_exit(tmp_path): + """A visible interim assistant row must survive an immediate process exit. + + ``GeneratorExit`` models an uncatchable turn interruption at the UI bridge: + no turn finalizer or graceful shutdown persistence is allowed to rescue the + row after the callback observes it. + """ + agent = _make_agent() + db_path = tmp_path / "state.db" + session_id = "interim-abnormal-exit" + db = _attach_real_session_db(agent, db_path, session_id) + tool_call = _mock_tool_call(call_id="visible-call") + agent.client.chat.completions.create.return_value = _mock_response( + content="I'll inspect the repository now.", + finish_reason="tool_calls", + tool_calls=[tool_call], + ) + + roles_seen_by_ui: list[str] = [] + + def _ui_projection(_text, *, already_streamed=False): + roles_seen_by_ui.extend(_durable_roles(db_path, session_id)) + raise GeneratorExit("simulated process termination after UI projection") + + agent.interim_assistant_callback = _ui_projection + try: + with pytest.raises(GeneratorExit, match="simulated process termination"): + agent.run_conversation("inspect the repository") + finally: + db.close() + + assert roles_seen_by_ui == ["user", "assistant"] + durable = _durable_messages(db_path, session_id) + assert [message["role"] for message in durable] == ["user", "assistant"] + assert durable[1]["content"] == "I'll inspect the repository now." + assert durable[1]["tool_calls"][0]["id"] == "visible-call" + + # Cold-resume reconciliation closes the interrupted call in the provider + # payload without mutating or duplicating the canonical transcript. + resumed = sanitize_api_messages(durable) + assert [message["role"] for message in resumed] == [ + "user", + "assistant", + "tool", + ] + assert resumed[2]["tool_call_id"] == "visible-call" + assert len(_durable_messages(db_path, session_id)) == 2 + + +def test_failed_assistant_persist_blocks_ui_projection_and_tool_side_effects(): + agent = _make_agent() + tool_call = _mock_tool_call(call_id="must-not-run") + agent.client.chat.completions.create.return_value = _mock_response( + content="I'll inspect the repository now.", + finish_reason="tool_calls", + tool_calls=[tool_call], + ) + agent._flush_messages_to_session_db = MagicMock(return_value=False) + agent.interim_assistant_callback = MagicMock() + agent._execute_tool_calls = MagicMock() + + result = agent.run_conversation("inspect the repository") + + agent.interim_assistant_callback.assert_not_called() + agent._execute_tool_calls.assert_not_called() + assert agent.client is not None + assert agent.client.chat.completions.create.call_count == 1 + assert result["failed"] is True + assert result["completed"] is False + assert result["turn_exit_reason"] == "session_persistence_failed" + + # --------------------------------------------------------------------------- # Contract 2: the SEQUENTIAL path flushes each tool result immediately, BEFORE # the next tool dispatches. Dispatch goes through run_agent.handle_function_call @@ -198,6 +300,145 @@ def test_execute_tool_calls_sequential_flushes_each_tool_result_before_next_disp ] +@pytest.mark.parametrize("executor_mode", ["sequential", "concurrent"]) +def test_tool_result_is_durable_before_ui_completion_on_abnormal_exit( + tmp_path, + executor_mode, +): + """A visible tool completion must already exist in the canonical DB.""" + agent = _make_agent() + db_path = tmp_path / "state.db" + session_id = f"tool-result-abnormal-exit-{executor_mode}" + db = _attach_real_session_db(agent, db_path, session_id) + tool_call = _mock_tool_call(call_id="visible-call") + messages = [ + {"role": "user", "content": "inspect the repository"}, + { + "role": "assistant", + "content": "I'll inspect the repository now.", + "tool_calls": [ + { + "id": "visible-call", + "type": "function", + "function": {"name": "web_search", "arguments": "{}"}, + } + ], + }, + ] + agent._flush_messages_to_session_db(messages) + + roles_seen_by_ui: list[str] = [] + + def _ui_completion(*_args): + roles_seen_by_ui.extend(_durable_roles(db_path, session_id)) + raise GeneratorExit("simulated process termination after tool completion") + + agent.tool_complete_callback = _ui_completion + assistant_message = SimpleNamespace(content="", tool_calls=[tool_call]) + dispatch_patch = ( + patch("run_agent.handle_function_call", return_value="repository result") + if executor_mode == "sequential" + else patch.object(agent, "_invoke_tool", return_value="repository result") + ) + try: + with ( + dispatch_patch, + patch( + "agent.tool_executor.maybe_persist_tool_result", + side_effect=lambda **kwargs: kwargs["content"], + ), + pytest.raises(GeneratorExit, match="simulated process termination"), + ): + if executor_mode == "sequential": + agent._execute_tool_calls_sequential( + assistant_message, + messages, + "task-1", + ) + else: + agent._execute_tool_calls_concurrent( + assistant_message, + messages, + "task-1", + ) + finally: + db.close() + + expected_roles = ["user", "assistant", "tool"] + assert roles_seen_by_ui == expected_roles + durable = _durable_messages(db_path, session_id) + assert [message["role"] for message in durable] == expected_roles + assert durable[2]["tool_call_id"] == "visible-call" + assert durable[2]["content"] == "repository result" + + +@pytest.mark.parametrize("executor_mode", ["sequential", "concurrent"]) +def test_failed_tool_result_persist_blocks_completion_projection(executor_mode): + agent = _make_agent() + tool_call = _mock_tool_call(call_id="failed-persist") + assistant_message = SimpleNamespace(content="", tool_calls=[tool_call]) + messages: list = [] + agent._flush_messages_to_session_db = MagicMock(return_value=False) + agent.tool_complete_callback = MagicMock() + dispatch_patch = ( + patch("run_agent.handle_function_call", return_value="repository result") + if executor_mode == "sequential" + else patch.object(agent, "_invoke_tool", return_value="repository result") + ) + + with ( + dispatch_patch, + patch( + "agent.tool_executor.maybe_persist_tool_result", + side_effect=lambda **kwargs: kwargs["content"], + ), + ): + if executor_mode == "sequential": + agent._execute_tool_calls_sequential( + assistant_message, + messages, + "task-1", + ) + else: + agent._execute_tool_calls_concurrent( + assistant_message, + messages, + "task-1", + ) + + agent.tool_complete_callback.assert_not_called() + assert getattr(agent, "_incremental_persistence_failed", False) is True + + +def test_segmented_batch_stops_before_later_segment_after_persist_failure(): + agent = _make_agent() + first = _mock_tool_call(call_id="first") + second = _mock_tool_call(call_id="second") + assistant_message = SimpleNamespace(tool_calls=[first, second]) + messages: list = [] + agent._flush_messages_to_session_db = MagicMock(return_value=False) + + with ( + patch.object(agent, "_invoke_tool", return_value="first result") as invoke, + patch("run_agent.handle_function_call", return_value="second result") as dispatch, + patch( + "agent.tool_executor.maybe_persist_tool_result", + side_effect=lambda **kwargs: kwargs["content"], + ), + ): + execute_tool_calls_segmented( + agent, + assistant_message, + messages, + "task-1", + segments=[("parallel", [first]), ("sequential", [second])], + ) + + invoke.assert_called_once() + dispatch.assert_not_called() + assert getattr(agent, "_incremental_persistence_failed", False) is True + + # --------------------------------------------------------------------------- # Contract 3: the CONCURRENT path flushes each collected tool result in append # order. Dispatch goes through agent._invoke_tool (the real concurrent From 8e934e84ac7d3c5aa73e30dd33cb0cf99ffa77dc Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 27 Jul 2026 20:50:26 +0500 Subject: [PATCH 18/19] fix: follow-ups for salvaged PR #72425 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - codex app-server sibling path: surface a WARNING (was silent debug) when the projected-message flush fails — same bug class as the main fix, but codex output has already streamed so fail-closed and agent_persisted=False are both wrong here (#860/#42039 duplicate-write hazard); loud durability gap logging instead. - map session_persistence_failed in _format_turn_completion_explanation so the user sees an actionable reason instead of 'The request failed: unknown error' + explainer test. - contributors/emails mapping for elco@thedaoist.gg (attribution CI). --- agent/codex_runtime.py | 19 +++++++++++++++++-- contributors/emails/elco@thedaoist.gg | 1 + run_agent.py | 8 ++++++++ .../test_turn_completion_explainer.py | 10 ++++++++++ 4 files changed, 36 insertions(+), 2 deletions(-) create mode 100644 contributors/emails/elco@thedaoist.gg diff --git a/agent/codex_runtime.py b/agent/codex_runtime.py index 59f9bac25a26..da3bc4f95695 100644 --- a/agent/codex_runtime.py +++ b/agent/codex_runtime.py @@ -778,12 +778,27 @@ def run_codex_app_server_turn( # the already-flushed user turn). See gateway/run.py agent_persisted. if getattr(agent, "_session_db", None) is not None: try: - agent._flush_messages_to_session_db(messages) + _codex_flush_ok = agent._flush_messages_to_session_db(messages) except Exception: - logger.debug( + _codex_flush_ok = False + logger.warning( "codex app-server projected-message flush failed", exc_info=True, ) + if _codex_flush_ok is False: + # Unlike the chat-completions loop (which fails closed BEFORE + # projection — see conversation_loop session_persistence_failed), + # codex output has already streamed to the user by the time this + # flush runs, so there is nothing left to withhold. We cannot + # flip agent_persisted=False either: the gateway fallback write + # would re-INSERT the already-flushed user turn (#860/#42039). + # Surface the durability gap loudly instead of a silent debug. + logger.warning( + "codex app-server turn was delivered but could NOT be " + "persisted to the session DB (session=%s) — this turn " + "will be missing after restart/resume", + getattr(agent, "session_id", None), + ) # Counter ticks for the agent-improvement loop. diff --git a/contributors/emails/elco@thedaoist.gg b/contributors/emails/elco@thedaoist.gg new file mode 100644 index 000000000000..a2a41fca3807 --- /dev/null +++ b/contributors/emails/elco@thedaoist.gg @@ -0,0 +1 @@ +elcocoel diff --git a/run_agent.py b/run_agent.py index 5e97ed24c9c0..fe8db996955d 100644 --- a/run_agent.py +++ b/run_agent.py @@ -3445,6 +3445,14 @@ class AIAgent: "the model produced no follow-up text. Send `continue` to " "let it summarize." ) + if reason == "session_persistence_failed": + return ( + prefix + + "the turn was stopped because session storage could not be " + "written (the transcript would have been lost on restart). " + "Check disk space / permissions for the state DB, then send " + "your message again." + ) # Unknown/diagnostic-only reasons (e.g. "unknown", guardrail_halt # which already surfaces its own message) — don't second-guess. return "" diff --git a/tests/run_agent/test_turn_completion_explainer.py b/tests/run_agent/test_turn_completion_explainer.py index 95a7a4b54a8d..386a74c754c0 100644 --- a/tests/run_agent/test_turn_completion_explainer.py +++ b/tests/run_agent/test_turn_completion_explainer.py @@ -106,6 +106,16 @@ def test_explanation_for_all_retries_exhausted(): assert "retries" in out.lower() +def test_explanation_for_session_persistence_failed(): + """Fail-closed persistence exits (#72425) must explain themselves.""" + out = AIAgent._format_turn_completion_explanation( + "session_persistence_failed" + ) + assert out # non-empty + assert "session storage" in out.lower() + assert "disk space" in out.lower() + + # -------------------------------------------------------------------------- # 2. Enable/disable seam # -------------------------------------------------------------------------- From 51a36f1fc1c33379be4bd1eb4d333f93effac3c9 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 27 Jul 2026 23:34:20 +0500 Subject: [PATCH 19/19] test: set _incremental_persistence_failed=False on MagicMock agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #72425 added getattr(agent, '_incremental_persistence_failed', False) checks at the top of execute_tool_calls_{sequential,concurrent,segmented}. A bare MagicMock auto-creates a truthy value for any attribute access, so the interrupt-skip test's MagicMock agent short-circuited before appending cancelled-tool messages — assert len(messages)==3 got 0. Production is unaffected: run_conversation resets the flag to False explicitly at turn start (conversation_loop.py:~1028). --- tests/tools/test_interrupt.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/tools/test_interrupt.py b/tests/tools/test_interrupt.py index aca47df9e19b..5552ea496b18 100644 --- a/tests/tools/test_interrupt.py +++ b/tests/tools/test_interrupt.py @@ -122,6 +122,11 @@ class TestPreToolCheck: agent._interrupt_requested = True agent.log_prefix = "" agent._persist_session = MagicMock() + # PR #72425: execute_tool_calls_* read _incremental_persistence_failed + # via getattr at loop top. A bare MagicMock auto-creates a truthy value + # for any attribute access, which would short-circuit the interrupt + # skip path before any cancelled-tool messages are appended. + agent._incremental_persistence_failed = False # Import and call the method import types