diff --git a/.gitignore b/.gitignore index d440702c2897..b5d594e19430 100644 --- a/.gitignore +++ b/.gitignore @@ -44,7 +44,10 @@ run_datagen_sonnet.sh source-data/* run_datagen_megascience_glm4-6.sh data/* -node_modules/ +# No trailing slash: also matches node_modules SYMLINKS (worktrees often +# symlink node_modules to the main checkout; the dir-only pattern let one +# slip into a commit and break `npm ci` on CI with ENOTDIR). +node_modules browser-use/ agent-browser/ # Private keys diff --git a/tests/tui_gateway/test_mcp_reload_rev.py b/tests/tui_gateway/test_mcp_reload_rev.py new file mode 100644 index 000000000000..866473d1a42e --- /dev/null +++ b/tests/tui_gateway/test_mcp_reload_rev.py @@ -0,0 +1,236 @@ +"""reload.mcp revision-aware coalescing (review on #20379, finding 1). + +The TUI's config poll sends the ``mcp_rev`` it observed with each reload +request. The server must guarantee that a success response means THAT +revision (or a newer one) was actually loaded: + +- The leader re-hashes the MCP-relevant config after discovery and repeats + until the hash is stable, so a config edit racing a slow reload can't be + silently skipped. +- A follower that waited behind a leader coalesces only when the leader's + loaded revision matches the follower's requested revision; otherwise it + re-runs the full reload itself. +- A failed reload returns a JSON-RPC error and does not advance the + generation, so a follower behind a failed leader re-runs too. + +Each test file runs in its own subprocess (run_tests.sh isolation), but the +fixtures still restore the module globals they touch. +""" + +from __future__ import annotations + +import threading + +import pytest + +import tools.mcp_tool as mcp_tool +import tui_gateway.server as srv + + +@pytest.fixture() +def reload_env(monkeypatch): + """Neutralize side effects and expose call-counting fakes.""" + calls = {"discover": 0, "shutdown": 0} + rev_box = {"rev": "rev-a"} + + monkeypatch.setattr(mcp_tool, "shutdown_mcp_servers", lambda: calls.__setitem__("shutdown", calls["shutdown"] + 1)) + monkeypatch.setattr(mcp_tool, "discover_mcp_tools", lambda: calls.__setitem__("discover", calls["discover"] + 1)) + monkeypatch.setattr(srv, "_compute_mcp_rev", lambda: rev_box["rev"]) + + saved = (srv._mcp_reload_gen, srv._mcp_reload_loaded_rev) + srv._mcp_reload_gen = 0 + srv._mcp_reload_loaded_rev = "" + yield calls, rev_box + srv._mcp_reload_gen, srv._mcp_reload_loaded_rev = saved + + +def _reload(rev: str | None = None, rid: int = 1) -> dict: + params: dict = {"session_id": "no-such-session", "confirm": True} + if rev is not None: + params["rev"] = rev + return srv._methods["reload.mcp"](rid, params) + + +def test_success_reports_loaded_rev(reload_env): + calls, rev_box = reload_env + rev_box["rev"] = "rev-a" + + envelope = _reload(rev="rev-a") + + assert envelope["result"]["status"] == "reloaded" + assert envelope["result"]["loaded_rev"] == "rev-a" + assert calls["discover"] == 1 + assert srv._mcp_reload_gen == 1 + + +def test_failed_reload_is_an_error_and_no_generation_advance(reload_env, monkeypatch): + """The exact client-facing contract: a failure must NOT look like an ack. + quietRpc on the TUI side collapses this error to null and keeps the + revision un-accepted, so the next poll retries.""" + calls, _ = reload_env + + def _boom(): + raise RuntimeError("flapping server") + + monkeypatch.setattr(mcp_tool, "discover_mcp_tools", _boom) + + envelope = _reload(rev="rev-b") + + assert "error" in envelope + assert srv._mcp_reload_gen == 0 + assert srv._mcp_reload_loaded_rev == "" + + +def test_leader_rehashes_until_stable_when_config_changes_mid_reload(reload_env, monkeypatch): + """Revision A starts a reload; the config changes to revision B while + discovery is connecting servers. The leader must not mark A complete — + it re-hashes after discovery and reloads again until stable, so the + reported loaded_rev is what discovery actually read.""" + calls, _ = reload_env + + # Hash sequence: before pass 1 → rev-a; after pass 1 → rev-b (config + # changed mid-discovery); after pass 2 → rev-b (stable). + hashes = iter(["rev-a", "rev-b", "rev-b"]) + monkeypatch.setattr(srv, "_compute_mcp_rev", lambda: next(hashes)) + + envelope = _reload(rev="rev-a") + + assert envelope["result"]["loaded_rev"] == "rev-b" + assert calls["discover"] == 2 # pass 1 read stale config, pass 2 converged + assert srv._mcp_reload_gen == 1 + + +class _WaiterLock: + """Lock wrapper that signals when a FOLLOWER enters the blocking ``with`` + path. The handler snapshots ``gen_before`` on the line before ``with``, + so once ``waiting`` fires the snapshot is already taken and it is safe to + let the leader complete — no sleep-based ordering.""" + + def __init__(self): + self._lock = threading.Lock() + self.waiting = threading.Event() + + def acquire(self, blocking: bool = True) -> bool: + return self._lock.acquire(blocking) + + def release(self) -> None: + self._lock.release() + + def locked(self) -> bool: + return self._lock.locked() + + def __enter__(self): + self.waiting.set() + self._lock.acquire() + return self + + def __exit__(self, *exc): + self._lock.release() + return False + + +def _run_leader_follower(reload_env, monkeypatch, follower_rev): + """Drive the A-then-B overlap deterministically: the leader blocks inside + discovery until the follower is queued on the lock, then completes.""" + calls, _rev_box = reload_env + + lock = _WaiterLock() + monkeypatch.setattr(srv, "_mcp_reload_lock", lock) + + leader_in_discovery = threading.Event() + release_leader = threading.Event() + + def _slow_discover(): + calls["discover"] += 1 + if calls["discover"] == 1: + leader_in_discovery.set() + assert release_leader.wait(timeout=10) + + monkeypatch.setattr(mcp_tool, "discover_mcp_tools", _slow_discover) + + results: dict = {} + + lt = threading.Thread(target=lambda: results.__setitem__("leader", _reload(rev="rev-a", rid=1)), daemon=True) + lt.start() + assert leader_in_discovery.wait(timeout=10) + + ft = threading.Thread(target=lambda: results.__setitem__("follower", _reload(rev=follower_rev, rid=2)), daemon=True) + ft.start() + # The follower has snapshotted gen_before once it blocks on the lock. + assert lock.waiting.wait(timeout=10) + release_leader.set() + + lt.join(timeout=10) + ft.join(timeout=10) + assert not lt.is_alive() and not ft.is_alive() + + return results, calls + + +def test_follower_with_matching_rev_coalesces(reload_env, monkeypatch): + results, calls = _run_leader_follower(reload_env, monkeypatch, follower_rev="rev-a") + + assert results["leader"]["result"]["status"] == "reloaded" + assert results["follower"]["result"]["status"] == "reloaded" + assert results["follower"]["result"].get("coalesced") is True + # Only the leader ran discovery. + assert calls["discover"] == 1 + + +def test_follower_with_newer_rev_reruns_full_reload(reload_env, monkeypatch): + """The race from the review: the leader loaded revision A, but this + follower was triggered by revision B. Coalescing would ack B against A's + registry — instead the follower must re-run the full reload.""" + results, calls = _run_leader_follower(reload_env, monkeypatch, follower_rev="rev-b") + + assert results["follower"]["result"]["status"] == "reloaded" + assert results["follower"]["result"].get("coalesced") is None + # Leader discovery + follower's own re-run. + assert calls["discover"] == 2 + + +def test_follower_behind_failed_leader_reruns(reload_env, monkeypatch): + """A failed leader never advances the generation — the follower re-runs + the full reload instead of acking over an empty/partial registry.""" + calls, _ = reload_env + + lock = _WaiterLock() + monkeypatch.setattr(srv, "_mcp_reload_lock", lock) + + leader_in_discovery = threading.Event() + release_leader = threading.Event() + + def _discover(): + calls["discover"] += 1 + if calls["discover"] == 1: + leader_in_discovery.set() + assert release_leader.wait(timeout=10) + raise RuntimeError("flapping server") + + monkeypatch.setattr(mcp_tool, "discover_mcp_tools", _discover) + + results: dict = {} + lt = threading.Thread(target=lambda: results.__setitem__("leader", _reload(rev="rev-a", rid=1)), daemon=True) + lt.start() + assert leader_in_discovery.wait(timeout=10) + + ft = threading.Thread(target=lambda: results.__setitem__("follower", _reload(rev="rev-a", rid=2)), daemon=True) + ft.start() + assert lock.waiting.wait(timeout=10) + release_leader.set() + + lt.join(timeout=10) + ft.join(timeout=10) + + assert "error" in results["leader"] + assert results["follower"]["result"]["status"] == "reloaded" + assert calls["discover"] == 2 + + +def test_legacy_request_without_rev_still_coalesces_on_generation(reload_env, monkeypatch): + """Manual /reload-mcp and older clients send no rev — generation-only + coalescing (the pre-existing contract) still applies.""" + results, calls = _run_leader_follower(reload_env, monkeypatch, follower_rev=None) # type: ignore[arg-type] + + assert results["follower"]["result"].get("coalesced") is True + assert calls["discover"] == 1 diff --git a/tui_gateway/server.py b/tui_gateway/server.py index facc6f878253..e2d31d154825 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -12689,23 +12689,7 @@ def _(rid, params: dict) -> dict: # config-change poller uses it to reload MCP servers only when their # config actually changed — a /skin or /statusbar write bumps mtime # but must not cost a multi-second MCP reconnect. - try: - cfg = _load_cfg() - # mcp_servers holds the server DEFINITIONS the classic CLI watches - # for auto-reload (cli.py::_check_config_mcp_changes) — omitting it - # meant editing a server bumped mtime but not mcp_rev, so the TUI - # skipped reload.mcp and new servers never connected until a manual - # /reload-mcp. `mcp` (settings) and `tools` (enable/disable) round - # out the MCP-relevant surface. - rev_src = json.dumps( - {"mcp": cfg.get("mcp"), "mcp_servers": cfg.get("mcp_servers"), "tools": cfg.get("tools")}, - sort_keys=True, - default=str, - ) - mcp_rev = hashlib.sha1(rev_src.encode()).hexdigest()[:12] - except Exception: - mcp_rev = "" - return _ok(rid, {"mtime": mtime, "mcp_rev": mcp_rev}) + return _ok(rid, {"mtime": mtime, "mcp_rev": _compute_mcp_rev()}) return _err(rid, 4002, f"unknown config key: {key}") @@ -12882,14 +12866,46 @@ def _(rid, params: dict) -> dict: # reload.mcp runs on the RPC pool (see _LONG_HANDLERS) so a slow/flapping MCP # server can't freeze the reader thread. Serialize reloads: overlapping # shutdown+discover pairs from stacked config-change polls would interleave -# and leave the registry half-built. Piggyback rather than queue — a reload -# that arrives while one is running would just redo identical work. +# and leave the registry half-built. _mcp_reload_lock = threading.Lock() # Bumped once per SUCCESSFUL shutdown+discover. A follower that waited on the # lock only skips the redundant reload if this advanced while it waited — i.e. # the leader actually completed. If the leader threw (flapping server), the # follower sees no advance and re-runs the full reload itself. _mcp_reload_gen = 0 +# The mcp_rev hash that the last successful reload actually LOADED (config +# re-hashed after discovery, so it reflects what discover_mcp_tools read — +# not what the caller hoped for). A follower coalesces only when the +# revision it was asked to load matches this; otherwise the config changed +# under the leader (rev A loaded, rev B requested) and the follower must +# re-run the full reload itself instead of acking B against A's registry. +_mcp_reload_loaded_rev = "" +# Bounded convergence for a config edit racing a slow reload: the leader +# re-hashes after discovery and repeats until the hash is stable. +_MCP_RELOAD_MAX_PASSES = 3 + + +def _compute_mcp_rev() -> str: + """Hash of the MCP-relevant config sections (server definitions, + settings, toolset enables). ``config.get mtime`` ships it to the TUI so + cosmetic writes don't trigger reloads; ``reload.mcp`` uses it for + revision-aware coalescing. Empty string = unknown (fail open).""" + try: + cfg = _load_cfg() + # mcp_servers holds the server DEFINITIONS the classic CLI watches + # for auto-reload (cli.py::_check_config_mcp_changes) — omitting it + # meant editing a server bumped mtime but not mcp_rev, so the TUI + # skipped reload.mcp and new servers never connected until a manual + # /reload-mcp. `mcp` (settings) and `tools` (enable/disable) round + # out the MCP-relevant surface. + rev_src = json.dumps( + {"mcp": cfg.get("mcp"), "mcp_servers": cfg.get("mcp_servers"), "tools": cfg.get("tools")}, + sort_keys=True, + default=str, + ) + return hashlib.sha1(rev_src.encode()).hexdigest()[:12] + except Exception: + return "" def _finish_reload(rid, params: dict, *, coalesced: bool) -> dict: @@ -12903,7 +12919,7 @@ def _finish_reload(rid, params: dict, *, coalesced: bool) -> dict: except Exception as _exc: logger.warning("Failed to persist mcp_reload_confirm=false: %s", _exc) - payload = {"status": "reloaded"} + payload = {"status": "reloaded", "loaded_rev": _mcp_reload_loaded_rev} if coalesced: payload["coalesced"] = True @@ -12991,28 +13007,49 @@ def _(rid, params: dict) -> dict: ) _emit("session.info", params.get("session_id", ""), _session_info(agent, session)) - global _mcp_reload_gen + global _mcp_reload_gen, _mcp_reload_loaded_rev + + # The revision the CALLER is asking to load (the mcp_rev its poll + # observed). Empty on legacy clients and manual /reload-mcp — those + # coalesce on generation alone, as before. + req_rev = str(params.get("rev") or "") def _do_full_reload() -> None: """shutdown+discover+refresh under the lock, then mark a completed generation. The lock spans the refresh too: releasing after discover would let a second reload tear the registry down while - this one is still reading it to rebuild the session snapshot.""" - global _mcp_reload_gen + this one is still reading it to rebuild the session snapshot. + + Config can change WHILE discover is connecting servers (a slow + reload racing a config edit): re-hash after discovery and repeat + until the hash is stable, so the generation we mark completed + always reflects the config that was actually loaded.""" + global _mcp_reload_gen, _mcp_reload_loaded_rev + + loaded = _compute_mcp_rev() + for _ in range(_MCP_RELOAD_MAX_PASSES): + shutdown_mcp_servers() + discover_mcp_tools() + after = _compute_mcp_rev() + if after == loaded: + break + loaded = after - shutdown_mcp_servers() - discover_mcp_tools() _refresh_session_agent() + _mcp_reload_loaded_rev = loaded _mcp_reload_gen += 1 # Serialize reloads. The LEADER (won the non-blocking acquire) runs the # full reload. A FOLLOWER (lock busy) snapshots the generation, waits, - # then — still holding the lock — checks whether a reload actually - # COMPLETED while it waited: if so it just refreshes its own agent - # against the fresh registry (coalesced); if the leader threw (flapping - # server, no generation advance) it re-runs the full reload itself, so - # a failed leader can never leave a follower reporting a bogus success - # over an empty/partial registry. + # then — still holding the lock — checks whether a reload that + # actually COMPLETED while it waited satisfies ITS request: the + # generation must have advanced (leader didn't throw) AND the loaded + # revision must match the one this follower was asked to apply. Both + # true → just refresh its own agent against the fresh registry + # (coalesced). Leader threw, or leader loaded an older revision than + # this request observed → re-run the full reload, so a failed or + # stale leader can never leave a follower acking a revision that was + # never loaded. if _mcp_reload_lock.acquire(blocking=False): try: _do_full_reload() @@ -13024,7 +13061,10 @@ def _(rid, params: dict) -> dict: gen_before = _mcp_reload_gen with _mcp_reload_lock: - if _mcp_reload_gen > gen_before: + leader_completed = _mcp_reload_gen > gen_before + rev_satisfied = not req_rev or req_rev == _mcp_reload_loaded_rev + + if leader_completed and rev_satisfied: _refresh_session_agent() coalesced = True else: diff --git a/ui-tui/package.json b/ui-tui/package.json index 105dba69f86b..3dbcb758d2cc 100644 --- a/ui-tui/package.json +++ b/ui-tui/package.json @@ -8,7 +8,7 @@ "start": "tsx src/entry.tsx", "build": "node scripts/build.mjs", "build:ink": "npm run build --prefix packages/hermes-ink", - "visual": "FORCE_COLOR=3 COLORTERM=truecolor tsx scripts/visual/render.tsx && electron scripts/visual/shot.mjs", + "visual": "node scripts/visual/run.mjs", "typecheck": "tsc --noEmit -p tsconfig.json", "lint": "eslint src/ packages/", "lint:fix": "eslint src/ packages/ --fix", diff --git a/ui-tui/scripts/visual/paths.mjs b/ui-tui/scripts/visual/paths.mjs new file mode 100644 index 000000000000..fe971b5646b9 --- /dev/null +++ b/ui-tui/scripts/visual/paths.mjs @@ -0,0 +1,11 @@ +// Shared output location for the visual harness. Hardcoded '/tmp/...' paths +// resolve to a drive-root like C:\tmp on native Windows (and fail when the +// directory doesn't exist) — os.tmpdir() is the platform-neutral answer. +// Both render.tsx and shot.mjs derive the same directory from here; +// HERMES_TUI_VISUAL_DIR overrides it for CI or side-by-side runs. +import { tmpdir } from 'os' +import { join } from 'path' + +export function visualOutDir() { + return process.env.HERMES_TUI_VISUAL_DIR || join(tmpdir(), 'hermes-tui-visual') +} diff --git a/ui-tui/scripts/visual/render.tsx b/ui-tui/scripts/visual/render.tsx index e89887f5771c..8ebf1af45af9 100644 --- a/ui-tui/scripts/visual/render.tsx +++ b/ui-tui/scripts/visual/render.tsx @@ -1,6 +1,6 @@ /* Visual self-verification tool: `npm run visual` renders real TUI surfaces - * across theme x background scenes to /tmp/tui-visual.html, then shot.mjs - * screenshots it to /tmp/tui-visual.png for eyeball + agent review. + * across theme x background scenes to /hermes-tui-visual/tui-visual.html, + * then shot.mjs screenshots it to tui-visual.png for eyeball + agent review. * * Original note: : render real TUI surfaces with ANSI colors intact, * convert to HTML on the actual background, and screenshot in a browser. */ @@ -9,9 +9,12 @@ process.env.COLORTERM = 'truecolor' import '../../src/lib/forceTruecolor.js' -import { writeFileSync } from 'fs' +import { mkdirSync, writeFileSync } from 'fs' +import { join } from 'path' import { PassThrough } from 'stream' +import { visualOutDir } from './paths.mjs' + import { Box, renderSync, Text } from '@hermes/ink' import React, { type ReactElement } from 'react' @@ -25,7 +28,7 @@ import type { SessionInfo } from '../../src/types.js' const noop = () => {} const pending = () => new Promise(() => {}) - + const fakeGateway = { gw: { notify: noop, off: noop, on: noop, request: pending }, rpc: pending } as any const SLATE = { @@ -304,6 +307,13 @@ for (const scene of scenes) { } page += '' -writeFileSync('/tmp/tui-visual.html', page) -console.log('wrote /tmp/tui-visual.html') + +const outDir = visualOutDir() + +mkdirSync(outDir, { recursive: true }) + +const outFile = join(outDir, 'tui-visual.html') + +writeFileSync(outFile, page) +console.log(`wrote ${outFile}`) process.exit(0) diff --git a/ui-tui/scripts/visual/run.mjs b/ui-tui/scripts/visual/run.mjs new file mode 100644 index 000000000000..939437a50127 --- /dev/null +++ b/ui-tui/scripts/visual/run.mjs @@ -0,0 +1,47 @@ +// Zero-dependency launcher for the visual harness (`npm run visual`). +// +// - Sets FORCE_COLOR/COLORTERM itself instead of POSIX `VAR=x cmd` shell +// assignments (which break under the Windows npm command shell) — no +// cross-env needed. +// - Resolves electron from the install tree (the desktop workspace already +// ships it; a root `npm install` hoists it) instead of declaring a second +// copy as a ui-tui dependency. ELECTRON_BIN overrides for exotic setups. +import { spawnSync } from 'child_process' +import { createRequire } from 'module' +import { dirname, join } from 'path' +import { fileURLToPath } from 'url' + +const here = dirname(fileURLToPath(import.meta.url)) +const require = createRequire(import.meta.url) + +const run = (bin, args, env = {}) => { + const { status } = spawnSync(bin, args, { env: { ...process.env, ...env }, stdio: 'inherit' }) + + if (status !== 0) { + process.exit(status ?? 1) + } +} + +// 1. Render the scene sheet (tsx is a ui-tui devDependency). +run(process.execPath, [require.resolve('tsx/cli'), join(here, 'render.tsx')], { + COLORTERM: 'truecolor', + FORCE_COLOR: '3' +}) + +// 2. Screenshot it with electron, borrowed from the workspace that owns it. +let electronBin = process.env.ELECTRON_BIN + +if (!electronBin) { + try { + // In plain Node, `require('electron')` evaluates to the binary path. + electronBin = require('electron') + } catch { + console.error( + 'electron is not installed in this tree — the visual harness borrows it from the\n' + + 'desktop workspace. Run `npm install` at the repo root, or point ELECTRON_BIN at a binary.' + ) + process.exit(1) + } +} + +run(electronBin, [join(here, 'shot.mjs')]) diff --git a/ui-tui/scripts/visual/shot.mjs b/ui-tui/scripts/visual/shot.mjs index 1815ad484f27..8907894910dc 100644 --- a/ui-tui/scripts/visual/shot.mjs +++ b/ui-tui/scripts/visual/shot.mjs @@ -1,6 +1,9 @@ -// Screenshot /tmp/tui-visual.html with the repo's Electron (offscreen). +// Screenshot the render.tsx output with the workspace's Electron (offscreen). import { app, BrowserWindow } from 'electron' import { writeFileSync } from 'fs' +import { join } from 'path' + +import { visualOutDir } from './paths.mjs' app.disableHardwareAcceleration() @@ -12,12 +15,15 @@ app.whenReady().then(async () => { width: 1500 }) - await win.loadFile('/tmp/tui-visual.html') + const outDir = visualOutDir() + + await win.loadFile(join(outDir, 'tui-visual.html')) await new Promise(r => setTimeout(r, 700)) const image = await win.webContents.capturePage() + const outFile = join(outDir, 'tui-visual.png') - writeFileSync('/tmp/tui-visual.png', image.toPNG()) - console.log('wrote /tmp/tui-visual.png') + writeFileSync(outFile, image.toPNG()) + console.log(`wrote ${outFile}`) app.quit() }) diff --git a/ui-tui/src/__tests__/loaders.test.ts b/ui-tui/src/__tests__/loaders.test.ts index cab44b8d2b57..3e1d007a2d82 100644 --- a/ui-tui/src/__tests__/loaders.test.ts +++ b/ui-tui/src/__tests__/loaders.test.ts @@ -1,8 +1,8 @@ -import { describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { renderToScreen } from '../../packages/hermes-ink/src/ink/render-to-screen.js' import { cellAtIndex } from '../../packages/hermes-ink/src/ink/screen.js' -import { ShimmerRows, shimmerSegments } from '../components/loaders.js' +import { ShimmerRows, shimmerSegments, subscribeShimmerClock } from '../components/loaders.js' describe('ShimmerRows leniency (agent-authored calls)', () => { it('accepts a bare row COUNT and derives widths — the generated-code shape', async () => { @@ -50,3 +50,64 @@ describe('shimmerSegments', () => { expect(pre + band + post).toBe(10) }) }) + +// Review on #20379 (finding 5): independent 90 ms intervals per shimmer +// composition meant an idle TUI with two lazy sections repainted ~22x/sec +// forever. All compositions now share ONE clock, and the interval exists +// only while subscribers do. +describe('subscribeShimmerClock', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('drives any number of subscribers from a single interval', () => { + const a: number[] = [] + const b: number[] = [] + + const timersBefore = vi.getTimerCount() + const unsubA = subscribeShimmerClock(p => a.push(p)) + const unsubB = subscribeShimmerClock(p => b.push(p)) + + // Two subscribers, ONE new timer. + expect(vi.getTimerCount()).toBe(timersBefore + 1) + + vi.advanceTimersByTime(300) + + // Same shared phases, in lockstep. + expect(a.length).toBeGreaterThan(0) + expect(a).toEqual(b) + + unsubA() + unsubB() + }) + + it('stops the interval with the last unsubscribe', () => { + const unsubA = subscribeShimmerClock(() => {}) + const unsubB = subscribeShimmerClock(() => {}) + const timersRunning = vi.getTimerCount() + + unsubA() + // Still one subscriber — clock keeps running. + expect(vi.getTimerCount()).toBe(timersRunning) + + unsubB() + expect(vi.getTimerCount()).toBe(timersRunning - 1) + }) + + it('a late subscriber restarts the clock cleanly', () => { + const unsubA = subscribeShimmerClock(() => {}) + + unsubA() + + const seen: number[] = [] + const unsubB = subscribeShimmerClock(p => seen.push(p)) + + vi.advanceTimersByTime(200) + expect(seen.length).toBeGreaterThan(0) + unsubB() + }) +}) diff --git a/ui-tui/src/__tests__/themeBoot.test.ts b/ui-tui/src/__tests__/themeBoot.test.ts new file mode 100644 index 000000000000..0c2932872571 --- /dev/null +++ b/ui-tui/src/__tests__/themeBoot.test.ts @@ -0,0 +1,127 @@ +import { describe, expect, it } from 'vitest' + +import { type BootTheme, invalidateBootBackground, seedBootEnvironment } from '../lib/themeBoot.js' +import { defaultTheme } from '../theme.js' + +// Review on #20379 (finding 2): the boot cache seeds the previous session's +// background into HERMES_TUI_BACKGROUND, which detectLightMode treats as a +// CURRENT signal. Without provenance, a stale light cache pins a now-dark +// terminal to light indefinitely (the current probe's pure-black answer is +// distrusted, the pure-white foreground is distrusted, and the macOS +// fallback refuses to run while the slot is occupied). These tests cover +// the seed/invalidate contract the gateway handler drives. + +const cache = (over: Partial = {}): BootTheme => ({ theme: defaultTheme, ...over }) + +describe('seedBootEnvironment', () => { + it('seeds the cached background when no explicit signal outranks it', () => { + const env: NodeJS.ProcessEnv = {} + + const seeded = seedBootEnvironment(cache({ background: '#ffffff' }), env) + + expect(env.HERMES_TUI_BACKGROUND).toBe('#ffffff') + expect(seeded).toEqual({ seededBackground: '#ffffff', seededPin: false }) + }) + + it('never seeds over explicit user signals', () => { + for (const preset of [{ HERMES_TUI_THEME: 'dark' }, { HERMES_TUI_LIGHT: '1' }] as NodeJS.ProcessEnv[]) { + const env = { ...preset } + const seeded = seedBootEnvironment(cache({ background: '#ffffff', mode: 'light' }), env) + + expect(env.HERMES_TUI_BACKGROUND).toBeUndefined() + expect(seeded).toEqual({ seededBackground: null, seededPin: false }) + } + + // A user-exported background keeps its value; only the pin may seed. + const env: NodeJS.ProcessEnv = { HERMES_TUI_BACKGROUND: '#123456' } + + expect(seedBootEnvironment(cache({ background: '#ffffff' }), env).seededBackground).toBeNull() + expect(env.HERMES_TUI_BACKGROUND).toBe('#123456') + }) + + it('never seeds the untrusted pure-black fingerprint', () => { + const env: NodeJS.ProcessEnv = {} + + const seeded = seedBootEnvironment(cache({ background: '#000000' }), env) + + expect(env.HERMES_TUI_BACKGROUND).toBeUndefined() + expect(seeded.seededBackground).toBeNull() + }) + + it('replays a cached config pin coherently with the physical background', () => { + // "/theme light" pinned while the physical terminal is dark: the cache + // stores BOTH — replaying only the background would resolve the first + // skin dark and recreate the light → dark → light flash. + const env: NodeJS.ProcessEnv = {} + + const seeded = seedBootEnvironment(cache({ background: '#1e1e1e', mode: 'light' }), env) + + expect(env.HERMES_TUI_THEME).toBe('light') + expect(env.HERMES_TUI_BACKGROUND).toBe('#1e1e1e') + expect(seeded).toEqual({ seededBackground: '#1e1e1e', seededPin: true }) + }) + + it('replays a pinned dark on a light physical background too', () => { + const env: NodeJS.ProcessEnv = {} + + const seeded = seedBootEnvironment(cache({ background: '#ffffff', mode: 'dark' }), env) + + expect(env.HERMES_TUI_THEME).toBe('dark') + expect(env.HERMES_TUI_BACKGROUND).toBe('#ffffff') + expect(seeded.seededPin).toBe(true) + }) + + it('is a no-op without a cache', () => { + const env: NodeJS.ProcessEnv = {} + + expect(seedBootEnvironment(null, env)).toEqual({ seededBackground: null, seededPin: false }) + expect(env).toEqual({}) + }) +}) + +describe('invalidateBootBackground', () => { + it('clears the slot while it still holds the seeded value (stale light cache, current dark terminal)', () => { + const env: NodeJS.ProcessEnv = {} + + seedBootEnvironment(cache({ background: '#ffffff' }), env) + + // Current terminal answers OSC-11 with distrusted #000000 → the handler + // invalidates: the slot must clear so foreground / COLORFGBG / macOS + // appearance / the default get their turn. + expect(invalidateBootBackground(env)).toBe(true) + expect(env.HERMES_TUI_BACKGROUND).toBeUndefined() + + // Idempotent: a second distrusted answer has nothing left to clear. + expect(invalidateBootBackground(env)).toBe(false) + }) + + it('clears a stale dark cache on a current ambiguous terminal the same way', () => { + const env: NodeJS.ProcessEnv = {} + + seedBootEnvironment(cache({ background: '#1e1e1e' }), env) + + expect(invalidateBootBackground(env)).toBe(true) + expect(env.HERMES_TUI_BACKGROUND).toBeUndefined() + }) + + it('leaves a trusted OSC answer that overwrote the seed alone', () => { + const env: NodeJS.ProcessEnv = {} + + seedBootEnvironment(cache({ background: '#ffffff' }), env) + + // A real OSC-11 measurement replaced the hint — it is authoritative. + env.HERMES_TUI_BACKGROUND = '#282828' + + expect(invalidateBootBackground(env)).toBe(false) + expect(env.HERMES_TUI_BACKGROUND).toBe('#282828') + }) + + it('is a no-op when nothing was seeded', () => { + const env: NodeJS.ProcessEnv = { HERMES_TUI_BACKGROUND: '#ffffff' } + + seedBootEnvironment(null, env) + + expect(invalidateBootBackground(env)).toBe(false) + expect(env.HERMES_TUI_BACKGROUND).toBe('#ffffff') + }) +}) diff --git a/ui-tui/src/__tests__/useConfigSync.test.ts b/ui-tui/src/__tests__/useConfigSync.test.ts index e760e8d748a1..9191b26d70ba 100644 --- a/ui-tui/src/__tests__/useConfigSync.test.ts +++ b/ui-tui/src/__tests__/useConfigSync.test.ts @@ -4,10 +4,12 @@ import { $uiState, resetUiState } from '../app/uiStore.js' import { applyDisplay, hydrateFullConfig, + type McpRevState, normalizeBusyInputMode, normalizeIndicatorStyle, normalizeMouseTracking, - normalizeStatusBar + normalizeStatusBar, + syncMcpReload } from '../app/useConfigSync.js' describe('applyDisplay', () => { @@ -372,6 +374,103 @@ describe('applyDisplay → voice.record_key (#18994)', () => { }) }) +// Review on #20379 (finding 1): an MCP config revision must never be acked +// before the server confirms it was LOADED. The old poll advanced its +// accepted revision first and fired reload.mcp second — a reload that failed +// (quietRpc → null) left the revision recorded as applied, and no subsequent +// poll retried it until an unrelated MCP edit. +describe('syncMcpReload (revision-aware ack)', () => { + const gwOk = (payload: unknown) => + ({ request: vi.fn(() => Promise.resolve(payload)), on: vi.fn(), off: vi.fn() }) as any + + const freshState = (accepted = 'rev-a'): McpRevState => ({ accepted, inFlight: false }) + + it('advances accepted only after the server confirms the reload', async () => { + const gw = gwOk({ status: 'reloaded', loaded_rev: 'rev-b' }) + const state = freshState() + const onReloaded = vi.fn() + + await syncMcpReload(gw, 's1', 'rev-b', state, onReloaded) + + expect(gw.request).toHaveBeenCalledWith('reload.mcp', { confirm: true, rev: 'rev-b', session_id: 's1' }) + expect(state.accepted).toBe('rev-b') + expect(onReloaded).toHaveBeenCalledTimes(1) + }) + + it('does NOT advance accepted when the reload RPC fails — next poll retries', async () => { + const gw = { request: vi.fn(() => Promise.reject(new Error('flapping server'))), on: vi.fn(), off: vi.fn() } as any + const state = freshState() + const onReloaded = vi.fn() + + await syncMcpReload(gw, 's1', 'rev-b', state, onReloaded) + + // The exact failure sequence from the review: reload fails, revision + // must remain un-acked so the next tick retries it. + expect(state.accepted).toBe('rev-a') + expect(state.inFlight).toBe(false) + expect(onReloaded).not.toHaveBeenCalled() + + // Next poll tick: the server recovered — the SAME revision goes through. + gw.request = vi.fn(() => Promise.resolve({ status: 'reloaded', loaded_rev: 'rev-b' })) + await syncMcpReload(gw, 's1', 'rev-b', state, onReloaded) + expect(state.accepted).toBe('rev-b') + expect(onReloaded).toHaveBeenCalledTimes(1) + }) + + it('does not advance on confirm_required (reload did not happen)', async () => { + const gw = gwOk({ message: 'confirm first', status: 'confirm_required' }) + const state = freshState() + + await syncMcpReload(gw, 's1', 'rev-b', state) + + expect(state.accepted).toBe('rev-a') + }) + + it('records the server-reported loaded_rev, not the requested rev', async () => { + // A config edit raced the reload: the server re-hashed after discovery + // and loaded rev-c. Recording rev-c (not rev-b) makes the next poll a + // no-op instead of an immediate redundant reload. + const gw = gwOk({ status: 'reloaded', loaded_rev: 'rev-c' }) + const state = freshState() + + await syncMcpReload(gw, 's1', 'rev-b', state) + + expect(state.accepted).toBe('rev-c') + }) + + it('is a no-op when the revision is already accepted or empty', async () => { + const gw = gwOk({ status: 'reloaded' }) + const state = freshState() + + await syncMcpReload(gw, 's1', 'rev-a', state) + await syncMcpReload(gw, 's1', '', state) + + expect(gw.request).not.toHaveBeenCalled() + }) + + it('does not stack requests while one is in flight', async () => { + let resolveFirst!: (v: unknown) => void + + const gw = { + request: vi.fn(() => new Promise(res => (resolveFirst = res))), + on: vi.fn(), + off: vi.fn() + } as any + + const state = freshState() + + const first = syncMcpReload(gw, 's1', 'rev-b', state) + + // Second tick while the first RPC is outstanding: swallowed. + await syncMcpReload(gw, 's1', 'rev-b', state) + expect(gw.request).toHaveBeenCalledTimes(1) + + resolveFirst({ status: 'reloaded', loaded_rev: 'rev-b' }) + await first + expect(state.accepted).toBe('rev-b') + }) +}) + // Round-12 Copilot review regression on #19835: the live mtime-reload // path was previously untested, so a regression in the polling/RPC // wiring to applyDisplay would only be visible at runtime. The fetch diff --git a/ui-tui/src/__tests__/useInputHandlers.test.ts b/ui-tui/src/__tests__/useInputHandlers.test.ts index ef9e676f73e1..a9c690cf0230 100644 --- a/ui-tui/src/__tests__/useInputHandlers.test.ts +++ b/ui-tui/src/__tests__/useInputHandlers.test.ts @@ -1,10 +1,12 @@ -import { describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { GridTestState } from '../app/interfaces.js' import { getOverlayState, patchOverlayState, resetOverlayState } from '../app/overlayStore.js' import { applyVoiceRecordResponse, dismissSensitivePrompt, handleIdleHotkeyExit, + handleStackedModalInput, shouldAllowIdleHotkeyExit, shouldFallThroughForScroll } from '../app/useInputHandlers.js' @@ -144,3 +146,96 @@ describe('dismissSensitivePrompt', () => { await pending }) }) + +// Review on #20379 (finding 3): a dialog stacked over /grid-test was +// visually modal but did not receive input — the grid branch ran first, so +// every advertised close key (Esc/q/Enter) mutated the hidden grid instead +// of closing the visible dialog. Input routing must follow visual stacking. +describe('handleStackedModalInput — dialog over grid-test', () => { + const baseModalKey = { + ctrl: false, + downArrow: false, + escape: false, + leftArrow: false, + return: false, + rightArrow: false, + upArrow: false + } + + const grid: GridTestState = { + activeCol: 1, + activeRow: 1, + areas: false, + cols: 4, + gap: null, + nested: false, + paddingX: null, + rows: 3, + streamFocus: 0, + streamMain: 0, + streams: false, + zoomed: false + } + + beforeEach(() => { + resetOverlayState() + patchOverlayState({ gridTest: { ...grid } }) + }) + + const openDialogViaD = () => { + expect(handleStackedModalInput(getOverlayState(), baseModalKey, 'd')).toBe(true) + expect(getOverlayState().dialog).not.toBeNull() + expect(getOverlayState().gridTest).not.toBeNull() + } + + it.each([ + ['Esc', { ...baseModalKey, escape: true }, ''], + ['q', baseModalKey, 'q'], + ['Enter', { ...baseModalKey, return: true }, ''], + ['Ctrl+C', { ...baseModalKey, ctrl: true }, 'c'] + ])('%s closes only the dialog, leaving the grid untouched', (_label, key, ch) => { + openDialogViaD() + + const gridBefore = getOverlayState().gridTest + + expect(handleStackedModalInput(getOverlayState(), key, ch)).toBe(true) + expect(getOverlayState().dialog).toBeNull() + // The grid must be byte-identical: not closed, not zoomed, not reset. + expect(getOverlayState().gridTest).toBe(gridBefore) + }) + + it('after the dialog closes, the same keys route to the grid again', () => { + openDialogViaD() + handleStackedModalInput(getOverlayState(), { ...baseModalKey, escape: true }, '') + expect(getOverlayState().dialog).toBeNull() + + // Esc now closes the grid — the dialog no longer shields it. + expect(handleStackedModalInput(getOverlayState(), { ...baseModalKey, escape: true }, '')).toBe(true) + expect(getOverlayState().gridTest).toBeNull() + }) + + it('the dialog swallows grid keys entirely while open (no leak-through)', () => { + openDialogViaD() + + const gridBefore = getOverlayState().gridTest + + // 'a' toggles areas mode when the grid has focus — it must not now. + expect(handleStackedModalInput(getOverlayState(), baseModalKey, 'a')).toBe(true) + expect(getOverlayState().gridTest).toBe(gridBefore) + expect(getOverlayState().dialog).not.toBeNull() + }) + + it('stacking works from streams mode too', () => { + patchOverlayState({ gridTest: { ...grid, streams: true } }) + openDialogViaD() + + expect(handleStackedModalInput(getOverlayState(), { ...baseModalKey, return: true }, '')).toBe(true) + expect(getOverlayState().dialog).toBeNull() + expect(getOverlayState().gridTest?.streams).toBe(true) + }) + + it('reports unconsumed when neither modal is up', () => { + resetOverlayState() + expect(handleStackedModalInput(getOverlayState(), baseModalKey, 'x')).toBe(false) + }) +}) diff --git a/ui-tui/src/app/createGatewayEventHandler.ts b/ui-tui/src/app/createGatewayEventHandler.ts index 320d7441064c..a4c86b601190 100644 --- a/ui-tui/src/app/createGatewayEventHandler.ts +++ b/ui-tui/src/app/createGatewayEventHandler.ts @@ -19,7 +19,7 @@ import { openExternalUrl } from '../lib/openExternalUrl.js' import { rpcErrorMessage } from '../lib/rpc.js' import { topLevelSubagents } from '../lib/subagentTree.js' import { formatAbandonedClarify, formatToolCall, stripAnsi } from '../lib/text.js' -import { writeBootTheme } from '../lib/themeBoot.js' +import { bootSeededPin, invalidateBootBackground, writeBootTheme } from '../lib/themeBoot.js' import { defaultThemeForCurrentBackground, detectLightMode, fromSkin, type Theme } from '../theme.js' import type { Msg, SubagentProgress, SubagentStatus } from '../types.js' @@ -83,7 +83,14 @@ const commitTheme = (theme: Theme) => { lastCommittedTheme = theme patchUiState({ theme }) - writeBootTheme(theme, process.env.HERMES_TUI_BACKGROUND) + // Persist the config pin alongside the resolved theme + physical + // background: a pinned session's resolved polarity intentionally + // disagrees with the background, and caching one without the other + // recreates the multi-stage flash on the next launch (light first frame → + // dark skin resolve against the cached background → light config pin). + const pin = configPinnedTheme ? process.env.HERMES_TUI_THEME : undefined + + writeBootTheme(theme, process.env.HERMES_TUI_BACKGROUND, pin === 'light' || pin === 'dark' ? pin : undefined) if (changed) { setTimeout(() => forceRedraw(process.stdout), 40).unref?.() @@ -130,8 +137,11 @@ export function reapplyTheme(): void { * terminal background unset. */ // True once CONFIG (via light/dark) owns the HERMES_TUI_THEME env pin, so an -// 'auto' hydrate knows not to clobber a user's shell-exported pin. -let configPinnedTheme = false +// 'auto' hydrate knows not to clobber a user's shell-exported pin. A pin the +// boot cache replayed counts as config-owned — it originated from +// display.tui_theme last session, and treating it as a shell export would +// make a stale cached pin unclearable by 'auto'. +let configPinnedTheme = bootSeededPin export function applyConfiguredTuiTheme(raw: unknown): void { const mode = String(raw ?? '') @@ -224,6 +234,21 @@ export function syncThemeToTerminalBackground(): void { // foreground below resolves the pole for transparent hosts, and a truly // pure-black terminal lands on dark either way. if (hex === '#000000') { + // The CURRENT terminal answered with an untrusted value — a background + // the boot cache seeded is from another era and must not keep + // outranking the live fallback chain (previous light session + new + // pure-black terminal stayed light forever: OSC-10 pure-white is also + // rejected, and the macOS fallback refuses to run while the slot is + // occupied). Clear the stale hint, give OSC-10 (same startup batch) + // first claim, then settle via env heuristics if nothing answered. + if (invalidateBootBackground()) { + setTimeout(() => { + if (!resolved) { + reapplyTheme() + } + }, 250).unref?.() + } + return } diff --git a/ui-tui/src/app/useConfigSync.ts b/ui-tui/src/app/useConfigSync.ts index f5f0a525f793..6a1e5032111c 100644 --- a/ui-tui/src/app/useConfigSync.ts +++ b/ui-tui/src/app/useConfigSync.ts @@ -129,6 +129,56 @@ const quietRpc = async = Record>( } } +// ── MCP revision handshake ─────────────────────────────────────────── +// +// The poll must not ack an MCP config revision until the server confirms it +// actually LOADED it. Advancing `accepted` before the reload succeeds loses +// revisions permanently: quietRpc collapses a failed reload to null, the +// next poll sees the same mcp_rev, and the new config never applies until +// an unrelated MCP edit. So `accepted` only moves on a confirmed reload — +// to the server's loaded_rev (what discovery actually read), falling back +// to the requested rev for older gateways. Retries are decoupled from +// mtime: every poll re-compares, so a transiently broken server heals on +// the next tick. + +export interface McpRevState { + /** Last revision the server CONFIRMED it loaded (or boot baseline). */ + accepted: string + /** A reload RPC is outstanding — don't stack another every poll tick. */ + inFlight: boolean +} + +export const syncMcpReload = async ( + gw: GatewayClient, + sid: string, + nextMcpRev: string, + state: McpRevState, + onReloaded?: () => void +): Promise => { + if (!nextMcpRev || nextMcpRev === state.accepted || state.inFlight) { + return + } + + state.inFlight = true + + try { + const r = await quietRpc(gw, 'reload.mcp', { + confirm: true, + rev: nextMcpRev, + session_id: sid + }) + + if (r?.status === 'reloaded') { + state.accepted = String(r.loaded_rev || nextMcpRev) + onReloaded?.() + } + // Failure (null) or confirm_required: leave `accepted` unchanged so the + // next poll tick retries the same revision. + } finally { + state.inFlight = false + } +} + const _voiceRecordKeyFromConfig = (cfg: ConfigFullResponse | null): ParsedVoiceRecordKey => { const raw = cfg?.config?.voice?.record_key @@ -245,7 +295,7 @@ export function useConfigSync({ sid }: UseConfigSyncOptions) { const mtimeRef = useRef(0) - const mcpRevRef = useRef('') + const mcpRevRef = useRef({ accepted: '', inFlight: false }) useEffect(() => { if (!sid) { @@ -261,9 +311,9 @@ export function useConfigSync({ mtimeRef.current = Number(r?.mtime ?? 0) // Seed the MCP revision baseline too: after a normal boot mtime is // already non-zero, so the poller's baseline branch never runs, and an - // unset mcpRevRef would make the FIRST cosmetic write (mtime bump, same + // unset baseline would make the FIRST cosmetic write (mtime bump, same // mcp_rev) look like an MCP change and fire a needless reload.mcp. - mcpRevRef.current = String(r?.mcp_rev ?? '') + mcpRevRef.current.accepted = String(r?.mcp_rev ?? '') }) void hydrateFullConfig(gw, setBellOnComplete, setVoiceRecordKey) }, [gw, setBellOnComplete, setVoiceEnabled, setVoiceRecordKey, sid]) @@ -281,28 +331,33 @@ export function useConfigSync({ if (!mtimeRef.current) { if (next) { mtimeRef.current = next - mcpRevRef.current = nextMcpRev + mcpRevRef.current.accepted = nextMcpRev } return } + // Reload MCP only when the MCP-relevant config actually changed. + // Cosmetic writes (/skin, /statusbar, /theme) bump mtime constantly; + // reconnecting every MCP server for those costs seconds and made + // skin switching feel glacial. The handshake runs on EVERY poll tick + // (not just mtime changes) so a failed reload retries until the + // server confirms the revision was loaded. + if (nextMcpRev) { + void syncMcpReload(gw, sid, nextMcpRev, mcpRevRef.current, () => + turnController.pushActivity('MCP reloaded after config change') + ) + } + if (!next || next === mtimeRef.current) { return } mtimeRef.current = next - // Reload MCP only when the MCP-relevant config actually changed. - // Cosmetic writes (/skin, /statusbar, /theme) bump mtime constantly; - // reconnecting every MCP server for those costs seconds and made - // skin switching feel glacial. Older gateways don't send mcp_rev — - // fall back to reload-on-any-change there. - const mcpChanged = !nextMcpRev || nextMcpRev !== mcpRevRef.current - - mcpRevRef.current = nextMcpRev - - if (mcpChanged) { + // Older gateways don't send mcp_rev — fall back to + // reload-on-any-change there (no ack tracking possible). + if (!nextMcpRev) { quietRpc(gw, 'reload.mcp', { session_id: sid, confirm: true }).then( r => r && turnController.pushActivity('MCP reloaded after config change') ) diff --git a/ui-tui/src/app/useInputHandlers.ts b/ui-tui/src/app/useInputHandlers.ts index 8bd5f2944a01..11f23fad1117 100644 --- a/ui-tui/src/app/useInputHandlers.ts +++ b/ui-tui/src/app/useInputHandlers.ts @@ -146,6 +146,145 @@ const keepGridCursorInBounds = (grid: GridTestState): GridTestState => ({ activeRow: clamp(grid.activeRow, 0, grid.rows - 1) }) +interface StackedModalKey { + ctrl: boolean + downArrow: boolean + escape: boolean + leftArrow: boolean + return: boolean + rightArrow: boolean + upArrow: boolean +} + +/** + * Input routing for the stacked demo modals (dialog over grid-test). + * Exported so tests can drive the real dispatch against the overlay store. + * + * ORDER IS THE CONTRACT: input routing follows visual stacking. /grid-test's + * `d` opens a dialog ON TOP of the grid without clearing it — the dialog + * branch must run first, or Esc/q/Enter mutate the hidden grid instead of + * closing the visible dialog, contradicting its "Esc/q/Enter close" hint + * (review on #20379, finding 3). + * + * Returns true when a modal consumed the key (callers stop routing). + */ +export function handleStackedModalInput( + overlay: Pick, + key: StackedModalKey, + ch: string +): boolean { + if (overlay.dialog) { + if (key.escape || isCtrl(key, ch, 'c') || ch === 'q' || key.return) { + patchOverlayState({ dialog: null }) + } + + return true + } + + if (!overlay.gridTest) { + return false + } + + const updateGrid = (fn: (grid: GridTestState) => GridTestState) => + patchOverlayState(prev => (prev.gridTest ? { ...prev, gridTest: keepGridCursorInBounds(fn(prev.gridTest)) } : prev)) + + const openDemoDialog = () => + patchOverlayState({ + dialog: { + body: ['Dialog overlaid on top of /grid-test.', '', 'Backdrop dims the grid behind.'].join('\n'), + hint: 'Esc/q/Enter close', + title: 'Overlay primitive', + zone: 'center' + } + }) + + const resetGrid = () => + updateGrid(grid => ({ + ...grid, + activeCol: 0, + activeRow: 0, + areas: false, + cols: 4, + gap: null, + nested: false, + paddingX: null, + rows: 3, + streamFocus: 0, + streamMain: 0, + streams: false, + zoomed: false + })) + + if (isCtrl(key, ch, 'c')) { + patchOverlayState({ gridTest: null }) + + return true + } + + // Streams mode swallows the grid-shape keys: focus cycles across the + // live panels and Enter promotes the focused one to the 2x2 slot. + if (overlay.gridTest.streams) { + if (key.escape || ch === 'q' || ch === 's') { + updateGrid(grid => ({ ...grid, streams: false })) + } else if (key.return) { + updateGrid(grid => ({ ...grid, streamMain: grid.streamFocus })) + } else if (ch === 'd') { + openDemoDialog() + } else if (ch === 'r') { + resetGrid() + } else if (key.leftArrow || key.upArrow || ch === 'h' || ch === 'k') { + updateGrid(grid => ({ + ...grid, + streamFocus: (grid.streamFocus + GRID_STREAM_COUNT - 1) % GRID_STREAM_COUNT + })) + } else if (key.rightArrow || key.downArrow || ch === 'l' || ch === 'j') { + updateGrid(grid => ({ ...grid, streamFocus: (grid.streamFocus + 1) % GRID_STREAM_COUNT })) + } + + return true + } + + if (overlay.gridTest.zoomed && (key.escape || ch === 'q')) { + updateGrid(grid => ({ ...grid, zoomed: false })) + } else if (key.escape || ch === 'q') { + patchOverlayState({ gridTest: null }) + } else if (key.return) { + updateGrid(grid => ({ ...grid, nested: true, zoomed: true })) + } else if (ch === 'n') { + updateGrid(grid => ({ ...grid, nested: !grid.nested })) + } else if (ch === 'a') { + updateGrid(grid => ({ ...grid, areas: !grid.areas, streams: false })) + } else if (ch === 's') { + updateGrid(grid => ({ ...grid, areas: false, streams: true })) + } else if (ch === 'g') { + updateGrid(grid => ({ ...grid, gap: cycleAutoNumber(grid.gap, 3) })) + } else if (ch === 'p') { + updateGrid(grid => ({ ...grid, paddingX: cycleAutoNumber(grid.paddingX, 2) })) + } else if (ch === 'd') { + openDemoDialog() + } else if (ch === 'r') { + resetGrid() + } else if (ch === '+' || ch === '=') { + updateGrid(grid => ({ ...grid, cols: clamp(grid.cols + 1, 1, GRID_TEST_MAX_SIZE) })) + } else if (ch === '-' || ch === '_') { + updateGrid(grid => ({ ...grid, cols: clamp(grid.cols - 1, 1, GRID_TEST_MAX_SIZE) })) + } else if (ch === ']') { + updateGrid(grid => ({ ...grid, rows: clamp(grid.rows + 1, 1, GRID_TEST_MAX_SIZE) })) + } else if (ch === '[') { + updateGrid(grid => ({ ...grid, rows: clamp(grid.rows - 1, 1, GRID_TEST_MAX_SIZE) })) + } else if (key.leftArrow || ch === 'h') { + updateGrid(grid => ({ ...grid, activeCol: clamp(grid.activeCol - 1, 0, grid.cols - 1) })) + } else if (key.rightArrow || ch === 'l') { + updateGrid(grid => ({ ...grid, activeCol: clamp(grid.activeCol + 1, 0, grid.cols - 1) })) + } else if (key.upArrow || ch === 'k') { + updateGrid(grid => ({ ...grid, activeRow: clamp(grid.activeRow - 1, 0, grid.rows - 1) })) + } else if (key.downArrow || ch === 'j') { + updateGrid(grid => ({ ...grid, activeRow: clamp(grid.activeRow + 1, 0, grid.rows - 1) })) + } + + return true +} + export function useInputHandlers(ctx: InputHandlerContext): InputHandlerResult { const { actions, composer, gateway, terminal, voice, wheelStep } = ctx const { actions: cActions, refs: cRefs, state: cState } = composer @@ -428,156 +567,9 @@ export function useInputHandlers(ctx: InputHandlerContext): InputHandlerResult { return } - if (overlay.gridTest) { - const updateGrid = (fn: (grid: GridTestState) => GridTestState) => - patchOverlayState(prev => - prev.gridTest ? { ...prev, gridTest: keepGridCursorInBounds(fn(prev.gridTest)) } : prev - ) - - const openDemoDialog = () => - patchOverlayState({ - dialog: { - body: ['Dialog overlaid on top of /grid-test.', '', 'Backdrop dims the grid behind.'].join('\n'), - hint: 'Esc/q/Enter close', - title: 'Overlay primitive', - zone: 'center' - } - }) - - const resetGrid = () => - updateGrid(grid => ({ - ...grid, - activeCol: 0, - activeRow: 0, - areas: false, - cols: 4, - gap: null, - nested: false, - paddingX: null, - rows: 3, - streamFocus: 0, - streamMain: 0, - streams: false, - zoomed: false - })) - - if (isCtrl(key, ch, 'c')) { - return patchOverlayState({ gridTest: null }) - } - - // Streams mode swallows the grid-shape keys: focus cycles across the - // live panels and Enter promotes the focused one to the 2x2 slot. - if (overlay.gridTest.streams) { - if (key.escape || ch === 'q' || ch === 's') { - return updateGrid(grid => ({ ...grid, streams: false })) - } - - if (key.return) { - return updateGrid(grid => ({ ...grid, streamMain: grid.streamFocus })) - } - - if (ch === 'd') { - return openDemoDialog() - } - - if (ch === 'r') { - return resetGrid() - } - - if (key.leftArrow || key.upArrow || ch === 'h' || ch === 'k') { - return updateGrid(grid => ({ - ...grid, - streamFocus: (grid.streamFocus + GRID_STREAM_COUNT - 1) % GRID_STREAM_COUNT - })) - } - - if (key.rightArrow || key.downArrow || ch === 'l' || ch === 'j') { - return updateGrid(grid => ({ ...grid, streamFocus: (grid.streamFocus + 1) % GRID_STREAM_COUNT })) - } - - return - } - - if (overlay.gridTest.zoomed && (key.escape || ch === 'q')) { - return updateGrid(grid => ({ ...grid, zoomed: false })) - } - - if (key.escape || ch === 'q') { - return patchOverlayState({ gridTest: null }) - } - - if (key.return) { - return updateGrid(grid => ({ ...grid, nested: true, zoomed: true })) - } - - if (ch === 'n') { - return updateGrid(grid => ({ ...grid, nested: !grid.nested })) - } - - if (ch === 'a') { - return updateGrid(grid => ({ ...grid, areas: !grid.areas, streams: false })) - } - - if (ch === 's') { - return updateGrid(grid => ({ ...grid, areas: false, streams: true })) - } - - if (ch === 'g') { - return updateGrid(grid => ({ ...grid, gap: cycleAutoNumber(grid.gap, 3) })) - } - - if (ch === 'p') { - return updateGrid(grid => ({ ...grid, paddingX: cycleAutoNumber(grid.paddingX, 2) })) - } - - if (ch === 'd') { - return openDemoDialog() - } - - if (ch === 'r') { - return resetGrid() - } - - if (ch === '+' || ch === '=') { - return updateGrid(grid => ({ ...grid, cols: clamp(grid.cols + 1, 1, GRID_TEST_MAX_SIZE) })) - } - - if (ch === '-' || ch === '_') { - return updateGrid(grid => ({ ...grid, cols: clamp(grid.cols - 1, 1, GRID_TEST_MAX_SIZE) })) - } - - if (ch === ']') { - return updateGrid(grid => ({ ...grid, rows: clamp(grid.rows + 1, 1, GRID_TEST_MAX_SIZE) })) - } - - if (ch === '[') { - return updateGrid(grid => ({ ...grid, rows: clamp(grid.rows - 1, 1, GRID_TEST_MAX_SIZE) })) - } - - if (key.leftArrow || ch === 'h') { - return updateGrid(grid => ({ ...grid, activeCol: clamp(grid.activeCol - 1, 0, grid.cols - 1) })) - } - - if (key.rightArrow || ch === 'l') { - return updateGrid(grid => ({ ...grid, activeCol: clamp(grid.activeCol + 1, 0, grid.cols - 1) })) - } - - if (key.upArrow || ch === 'k') { - return updateGrid(grid => ({ ...grid, activeRow: clamp(grid.activeRow - 1, 0, grid.rows - 1) })) - } - - if (key.downArrow || ch === 'j') { - return updateGrid(grid => ({ ...grid, activeRow: clamp(grid.activeRow + 1, 0, grid.rows - 1) })) - } - - return - } - - if (overlay.dialog) { - if (key.escape || isCtrl(key, ch, 'c') || ch === 'q' || key.return) { - return patchOverlayState({ dialog: null }) - } - + // Stacked demo modals (dialog over grid-test): shared routing where + // the topmost visual layer consumes input first. + if (handleStackedModalInput(overlay, key, ch)) { return } diff --git a/ui-tui/src/components/loaders.tsx b/ui-tui/src/components/loaders.tsx index ebe0bb8ce822..8ce6ed7f9591 100644 --- a/ui-tui/src/components/loaders.tsx +++ b/ui-tui/src/components/loaders.tsx @@ -50,17 +50,77 @@ export function Shimmer({ ) } -/** Self-ticking phase for shimmer compositions. */ -export function useShimmerPhase(tickMs = 90): number { - const [phase, setPhase] = useState(0) +// ── Shared shimmer clock ───────────────────────────────────────────── +// +// ONE interval drives every mounted shimmer composition (review on #20379, +// finding 5: the session panel could mount independent 90 ms intervals for +// lazy skills AND lazy tools — ~22 state updates/sec on an otherwise-idle +// TUI). Subscribers share the tick; the interval exists only while +// subscribers do, and all updates land in one timer callback so React +// batches them into a single render pass. + +const TICK_MS = 90 + +/** Animation budget per mount. A lazy watch session can stay lazy + * indefinitely — after the budget the skeleton freezes in place (still + * reads as "loading") instead of repainting forever. */ +export const SHIMMER_ANIMATE_MS = 30_000 + +const clockListeners = new Set<(phase: number) => void>() +let clockId: NodeJS.Timeout | null = null +let clockPhase = 0 + +/** Subscribe to the shared clock (exported for tests). Returns unsubscribe; + * the interval stops with the last subscriber. */ +export function subscribeShimmerClock(fn: (phase: number) => void): () => void { + clockListeners.add(fn) + + if (!clockId) { + clockId = setInterval(() => { + clockPhase += 1 + + for (const listener of clockListeners) { + listener(clockPhase) + } + }, TICK_MS) + + clockId.unref?.() + } + + return () => { + clockListeners.delete(fn) + + if (!clockListeners.size && clockId) { + clearInterval(clockId) + clockId = null + } + } +} + +/** Phase from the shared clock, bounded: stops advancing (and stops costing + * renders) after `animateMs`. */ +export function useShimmerPhase(animateMs = SHIMMER_ANIMATE_MS): number { + const [phase, setPhase] = useState(clockPhase) useEffect(() => { - const id = setInterval(() => setPhase(p => p + 1), tickMs) + const startedAt = Date.now() - id.unref?.() + let unsubscribe: (() => void) | null = subscribeShimmerClock(next => { + if (Date.now() - startedAt >= animateMs) { + unsubscribe?.() + unsubscribe = null - return () => clearInterval(id) - }, [tickMs]) + return + } + + setPhase(next) + }) + + return () => { + unsubscribe?.() + unsubscribe = null + } + }, [animateMs]) return phase } diff --git a/ui-tui/src/gatewayTypes.ts b/ui-tui/src/gatewayTypes.ts index a4bbe04a8971..649561728702 100644 --- a/ui-tui/src/gatewayTypes.ts +++ b/ui-tui/src/gatewayTypes.ts @@ -436,6 +436,10 @@ export interface ModelOptionsResponse { export interface ReloadMcpResponse { status?: string message?: string + /** The mcp_rev the server actually loaded (re-hashed after discovery). + * The client records THIS as its accepted revision, not the one it + * requested — a reload that raced a config edit reports the newer rev. */ + loaded_rev?: string } export interface ReloadEnvResponse { diff --git a/ui-tui/src/lib/themeBoot.ts b/ui-tui/src/lib/themeBoot.ts index 6062ac2c103d..76872f457c59 100644 --- a/ui-tui/src/lib/themeBoot.ts +++ b/ui-tui/src/lib/themeBoot.ts @@ -23,6 +23,12 @@ import type { Theme } from '../theme.js' interface BootThemeFile { /** The resolved background hex that detection settled on, if any. */ background?: string + /** The config mode pin (`display.tui_theme`) active when this cache was + * written. Without it a pinned theme caches incoherently — a light + * resolved theme next to the dark PHYSICAL background — and the next + * launch flashes light → dark (skin resolves against the seeded + * background) → light (config pin rehydrates). */ + mode?: 'dark' | 'light' /** The fully-resolved Theme (palette + brand) from the last session. */ theme?: Theme version: 1 @@ -55,8 +61,14 @@ const looksLikeTheme = (value: unknown): value is Theme => { ) } +export interface BootTheme { + background?: string + mode?: 'dark' | 'light' + theme: Theme +} + /** Read the cached boot theme. Null on first launch / damage / test runs. */ -export function readBootTheme(): { background?: string; theme: Theme } | null { +export function readBootTheme(): BootTheme | null { if (isTestRun()) { return null } @@ -68,7 +80,11 @@ export function readBootTheme(): { background?: string; theme: Theme } | null { return null } - return { background: typeof raw.background === 'string' ? raw.background : undefined, theme: raw.theme } + return { + background: typeof raw.background === 'string' ? raw.background : undefined, + mode: raw.mode === 'light' || raw.mode === 'dark' ? raw.mode : undefined, + theme: raw.theme + } } catch { return null } @@ -77,7 +93,7 @@ export function readBootTheme(): { background?: string; theme: Theme } | null { let writeTimer: NodeJS.Timeout | null = null /** Persist the resolved theme (debounced, atomic, fire-and-forget). */ -export function writeBootTheme(theme: Theme, background?: string): void { +export function writeBootTheme(theme: Theme, background?: string, mode?: 'dark' | 'light'): void { if (isTestRun()) { return } @@ -90,7 +106,7 @@ export function writeBootTheme(theme: Theme, background?: string): void { writeTimer = null try { - const payload: BootThemeFile = { background, theme, version: 1 } + const payload: BootThemeFile = { background, mode, theme, version: 1 } const path = bootFilePath() const tmp = `${path}.tmp` @@ -104,25 +120,90 @@ export function writeBootTheme(theme: Theme, background?: string): void { writeTimer.unref?.() } +// ── Boot-time seeding (with provenance) ────────────────────────────── +// +// The cache seeds env slots so the first skin resolution matches the last +// session — but a previous-session background must NOT occupy the same +// authoritative slot as a current OSC answer forever. Provenance is +// recorded so that once the CURRENT terminal answers with a distrusted +// value (pure-black OSC-11, unusable OSC-10), the stale hint can be +// invalidated and detection falls through to foreground / COLORFGBG / +// macOS appearance / the default — instead of a light cache pinning a now- +// dark terminal to light indefinitely (review on #20379, finding 2). + +export interface BootSeedResult { + /** The background hex this boot seeded into env, or null. */ + seededBackground: null | string + /** True when the cached config pin was seeded into HERMES_TUI_THEME. */ + seededPin: boolean +} + +// Provenance of the last seeding pass — read by invalidateBootBackground. +// Module-load seeds process.env; tests re-seed against a fake env. +let seeded: BootSeedResult = { seededBackground: null, seededPin: false } + +/** Seeding step (exported for tests — module-load runs it on process.env). + * Explicit user signals always outrank the cache. Records provenance so + * invalidateBootBackground can later demote the hint. */ +export function seedBootEnvironment(boot: BootTheme | null, env: NodeJS.ProcessEnv): BootSeedResult { + const result: BootSeedResult = { seededBackground: null, seededPin: false } + + seeded = result + + if (!boot || env.HERMES_TUI_THEME || env.HERMES_TUI_LIGHT) { + return result + } + + // Replay the config mode pin first: a pinned session caches a resolved + // theme whose polarity intentionally disagrees with the physical + // background, and without the pin the first skin resolution flips to the + // physical pole before config hydration flips it back (multi-stage flash). + if (boot.mode) { + env.HERMES_TUI_THEME = boot.mode + result.seededPin = true + } + + if ( + boot.background && + // Never seed the untrusted "unset default" fingerprint — a cache written + // before the distrust rule existed must not poison this session's + // detection (it would also suppress the macOS-appearance fallback). + boot.background.toLowerCase() !== '#000000' && + !env.HERMES_TUI_BACKGROUND + ) { + env.HERMES_TUI_BACKGROUND = boot.background + result.seededBackground = boot.background + } + + return result +} + /** - * Boot-time seeding, run once at module load (imported by uiStore before the - * first render): make the cached background visible to `detectLightMode` - * unless an explicit signal already outranks it. + * Invalidate the cache-seeded background: called when the CURRENT terminal + * answered the background probe with an untrusted value, meaning the seeded + * hint is from another era and must not outrank the live fallback chain. + * Only clears the slot while it still holds the seeded value — a trusted + * OSC answer or explicit export that overwrote it is left alone. + * Returns true when the slot was cleared. */ +export function invalidateBootBackground(env: NodeJS.ProcessEnv = process.env): boolean { + if (!seeded.seededBackground || env.HERMES_TUI_BACKGROUND !== seeded.seededBackground) { + return false + } + + delete env.HERMES_TUI_BACKGROUND + seeded.seededBackground = null + + return true +} + const boot = readBootTheme() -if ( - boot?.background && - // Never seed the untrusted "unset default" fingerprint — a cache written - // before the distrust rule existed must not poison this session's - // detection (it would also suppress the macOS-appearance fallback). - boot.background.toLowerCase() !== '#000000' && - !process.env.HERMES_TUI_BACKGROUND && - !process.env.HERMES_TUI_THEME && - !process.env.HERMES_TUI_LIGHT -) { - process.env.HERMES_TUI_BACKGROUND = boot.background -} +/** True when this boot replayed a cached config pin into HERMES_TUI_THEME. + * applyConfiguredTuiTheme treats it as config-owned so a later 'auto' can + * clear it — otherwise a stale cached pin masquerades as a user shell + * export and becomes unclearable. */ +export const bootSeededPin: boolean = seedBootEnvironment(boot, process.env).seededPin /** The cached theme for the first frame, or null on first launch. */ export const bootTheme: Theme | null = boot?.theme ?? null