From b11b5ece2e68e4e5456fe1e3f364ff7564bdcb8b Mon Sep 17 00:00:00 2001
From: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com>
Date: Tue, 21 Jul 2026 19:01:27 -0500
Subject: [PATCH 1/8] =?UTF-8?q?fix(tui):=20revision-aware=20reload.mcp=20?=
 =?UTF-8?q?=E2=80=94=20an=20ack=20now=20means=20the=20revision=20was=20LOA?=
 =?UTF-8?q?DED?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Review on #20379, finding 1 (High). Two ways an MCP config revision could
be silently acknowledged without ever being applied:

Client: the poll advanced its accepted mcp_rev BEFORE calling reload.mcp,
and quietRpc collapses failures to null — a reload that failed against a
temporarily broken server left the revision recorded as applied, and no
subsequent poll retried until an unrelated MCP edit. The handshake is now
syncMcpReload(): send the observed rev with the request, advance `accepted`
only when the server answers status=reloaded (to the server's loaded_rev,
falling back to the requested rev on older gateways), and re-compare on
EVERY poll tick — decoupled from mtime — so a transient failure heals on
the next tick. An in-flight guard stops the 5s poll from stacking requests
behind a slow reload.

Server: generation-only coalescing let a follower triggered by revision B
ack against revision A's registry when the config changed under a slow
leader. The leader now re-hashes the MCP-relevant config after discovery
and repeats until stable (bounded), records _mcp_reload_loaded_rev, and a
follower coalesces only when the revision it was asked to load matches —
otherwise it re-runs the full reload itself. Responses carry loaded_rev.

Deterministic tests for the exact failure sequences: failed reload → no
ack, no generation advance; A-then-B overlap → follower re-runs; matching
rev → coalesces; failed leader → follower re-runs; legacy no-rev callers
keep generation-only coalescing (thread ordering via an instrumented lock,
no sleeps). Client: 6 vitest cases on the ack/retry/in-flight contract.
---
 tests/tui_gateway/test_mcp_reload_rev.py   | 236 +++++++++++++++++++++
 tui_gateway/server.py                      | 104 ++++++---
 ui-tui/src/__tests__/useConfigSync.test.ts |  98 ++++++++-
 ui-tui/src/app/useConfigSync.ts            |  83 ++++++--
 ui-tui/src/gatewayTypes.ts                 |   4 +
 5 files changed, 478 insertions(+), 47 deletions(-)
 create mode 100644 tests/tui_gateway/test_mcp_reload_rev.py

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/src/__tests__/useConfigSync.test.ts b/ui-tui/src/__tests__/useConfigSync.test.ts
index e760e8d748a1..4338541c6235 100644
--- a/ui-tui/src/__tests__/useConfigSync.test.ts
+++ b/ui-tui/src/__tests__/useConfigSync.test.ts
@@ -7,7 +7,9 @@ import {
   normalizeBusyInputMode,
   normalizeIndicatorStyle,
   normalizeMouseTracking,
-  normalizeStatusBar
+  normalizeStatusBar,
+  syncMcpReload,
+  type McpRevState
 } from '../app/useConfigSync.js'
 
 describe('applyDisplay', () => {
@@ -372,6 +374,100 @@ 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/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 <T extends Record<string, any> = Record<string, any>>(
   }
 }
 
+// ── 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<void> => {
+  if (!nextMcpRev || nextMcpRev === state.accepted || state.inFlight) {
+    return
+  }
+
+  state.inFlight = true
+
+  try {
+    const r = await quietRpc<ReloadMcpResponse>(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<McpRevState>({ 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<ReloadMcpResponse>(gw, 'reload.mcp', { session_id: sid, confirm: true }).then(
             r => r && turnController.pushActivity('MCP reloaded after config change')
           )
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 {

From e6cc5612e6e5afa6f373515364ee5ec11561248b Mon Sep 17 00:00:00 2001
From: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com>
Date: Tue, 21 Jul 2026 19:06:25 -0500
Subject: [PATCH 2/8] =?UTF-8?q?fix(ui-tui):=20boot=20theme=20cache=20gets?=
 =?UTF-8?q?=20provenance=20=E2=80=94=20a=20stale=20hint=20can't=20pin=20th?=
 =?UTF-8?q?e=20terminal?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Review on #20379, finding 2 (High). The boot cache seeded the previous
session's background into HERMES_TUI_BACKGROUND, the same slot a CURRENT
OSC-11 answer occupies — so a cache written on a light terminal pinned a
now-pure-black terminal to light forever: the new OSC-11 #000000 answer is
distrusted by design, the pure-white OSC-10 foreground is distrusted too,
and the macOS appearance fallback refuses to run while the slot is set.

Seeding now records provenance (themeBoot.seedBootEnvironment, extracted
and testable against a passed env). When the current terminal answers the
background probe with the untrusted fingerprint, the gateway handler calls
invalidateBootBackground(): the slot clears ONLY while it still holds the
seeded value (a trusted answer that overwrote it is authoritative), OSC-10
gets first claim in the same startup batch, and a short settle pass re-
derives from the live fallback chain if nothing answered.

The cache is also pin-coherent now: commitTheme persists the config mode
pin (display.tui_theme) alongside the resolved theme + physical background.
Previously "/theme light" on a dark terminal cached a light theme next to a
dark background — the next launch painted light, flipped dark when the skin
resolved against the seeded background, then flipped light again on config
hydration: the exact multi-stage flash the cache exists to eliminate. The
seeded pin counts as config-owned (bootSeededPin), so a later 'auto' can
still clear it instead of mistaking it for a user shell export.

Tests cover the review's sequences: stale-light cache vs current dark
terminal (invalidate → fallback chain), stale-dark vs ambiguous light,
pinned light on a dark physical background across restart (and the
inverse), trusted-overwrite protection, and the explicit-signal guards.
---
 ui-tui/src/__tests__/themeBoot.test.ts      | 130 ++++++++++++++++++++
 ui-tui/src/app/createGatewayEventHandler.ts |  33 ++++-
 ui-tui/src/lib/themeBoot.ts                 | 119 +++++++++++++++---
 3 files changed, 259 insertions(+), 23 deletions(-)
 create mode 100644 ui-tui/src/__tests__/themeBoot.test.ts

diff --git a/ui-tui/src/__tests__/themeBoot.test.ts b/ui-tui/src/__tests__/themeBoot.test.ts
new file mode 100644
index 000000000000..92bb500f0a4f
--- /dev/null
+++ b/ui-tui/src/__tests__/themeBoot.test.ts
@@ -0,0 +1,130 @@
+import { describe, expect, it } from 'vitest'
+
+import { invalidateBootBackground, seedBootEnvironment, type BootTheme } 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> = {}): 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/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/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

From c25e08aa2b483195583b4ebf2abe4d4722ab5447 Mon Sep 17 00:00:00 2001
From: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com>
Date: Tue, 21 Jul 2026 19:10:16 -0500
Subject: [PATCH 3/8] fix(ui-tui): stacked dialog gets input before the grid
 under it
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Review on #20379, finding 3 (Medium). /grid-test's `d` opens a dialog on
top without clearing the grid, but the grid's input branch ran FIRST — so
Esc/q/Enter mutated the hidden grid (close/unzoom/promote) instead of
closing the visible dialog, contradicting its "Esc/q/Enter close" hint.

Input routing now follows visual stacking: the dialog/grid dispatch is
extracted into handleStackedModalInput() with the dialog branch first, the
hook consumes through it, and tests drive the real dispatch against the
overlay store — each advertised close key closes only the dialog (grid
byte-identical), grid keys don't leak through while the dialog is up, and
the same keys route to the grid again after it closes.
---
 ui-tui/src/__tests__/themeBoot.test.ts        |   2 +-
 ui-tui/src/__tests__/useConfigSync.test.ts    |   6 +-
 ui-tui/src/__tests__/useInputHandlers.test.ts |  97 +++++-
 ui-tui/src/app/useInputHandlers.ts            | 292 +++++++++---------
 4 files changed, 243 insertions(+), 154 deletions(-)

diff --git a/ui-tui/src/__tests__/themeBoot.test.ts b/ui-tui/src/__tests__/themeBoot.test.ts
index 92bb500f0a4f..29f406520656 100644
--- a/ui-tui/src/__tests__/themeBoot.test.ts
+++ b/ui-tui/src/__tests__/themeBoot.test.ts
@@ -1,6 +1,6 @@
 import { describe, expect, it } from 'vitest'
 
-import { invalidateBootBackground, seedBootEnvironment, type BootTheme } from '../lib/themeBoot.js'
+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
diff --git a/ui-tui/src/__tests__/useConfigSync.test.ts b/ui-tui/src/__tests__/useConfigSync.test.ts
index 4338541c6235..13bd5f4924f6 100644
--- a/ui-tui/src/__tests__/useConfigSync.test.ts
+++ b/ui-tui/src/__tests__/useConfigSync.test.ts
@@ -4,12 +4,12 @@ import { $uiState, resetUiState } from '../app/uiStore.js'
 import {
   applyDisplay,
   hydrateFullConfig,
+  type McpRevState,
   normalizeBusyInputMode,
   normalizeIndicatorStyle,
   normalizeMouseTracking,
   normalizeStatusBar,
-  syncMcpReload,
-  type McpRevState
+  syncMcpReload
 } from '../app/useConfigSync.js'
 
 describe('applyDisplay', () => {
@@ -449,11 +449,13 @@ describe('syncMcpReload (revision-aware ack)', () => {
 
   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)
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/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<OverlayState, 'dialog' | 'gridTest'>,
+  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
       }
 

From 9fb2a63db8dd49f62a589a34a7c5894977927265 Mon Sep 17 00:00:00 2001
From: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com>
Date: Tue, 21 Jul 2026 19:16:08 -0500
Subject: [PATCH 4/8] =?UTF-8?q?fix(ui-tui):=20make=20`npm=20run=20visual`?=
 =?UTF-8?q?=20portable=20off=20POSIX=20=E2=80=94=20zero=20new=20deps?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Review on #20379, finding 4 (Medium). Three portability defects in the
visual verification harness:

- `FORCE_COLOR=3 COLORTERM=truecolor tsx ...` POSIX env assignment does not
  work under the Windows npm command shell. The script is now a plain Node
  launcher (scripts/visual/run.mjs) that sets the env itself and spawns tsx
  via require.resolve('tsx/cli') — no cross-env, no shell syntax.
- Hardcoded /tmp/tui-visual.{html,png} resolve to a drive-root path like
  C:\tmp on native Windows (and fail when that directory doesn't exist).
  Both scripts now derive the output directory from a shared paths.mjs
  helper: os.tmpdir()/hermes-tui-visual (created recursively;
  HERMES_TUI_VISUAL_DIR overrides for CI or side-by-side runs).
- electron was undeclared by ui-tui and only worked via hoisting luck. The
  launcher now resolves it EXPLICITLY from the install tree the desktop
  workspace already provides (require('electron') in plain Node returns the
  binary path), with an ELECTRON_BIN override and a clear error pointing at
  the repo-root install when it's absent — instead of declaring a second
  ~100MB dependency on a TUI workspace for a dev-only harness.

Also fixes the trailing-whitespace line in render.tsx that `git diff
--check` flags. Verified end-to-end: render writes the HTML scene sheet
and the electron shot step produces the screenshot from the tmpdir path;
the missing-electron error path prints the guidance message.
---
 ui-tui/package.json              |  2 +-
 ui-tui/scripts/visual/paths.mjs  | 11 ++++++++
 ui-tui/scripts/visual/render.tsx | 22 +++++++++++----
 ui-tui/scripts/visual/run.mjs    | 47 ++++++++++++++++++++++++++++++++
 ui-tui/scripts/visual/shot.mjs   | 14 +++++++---
 5 files changed, 85 insertions(+), 11 deletions(-)
 create mode 100644 ui-tui/scripts/visual/paths.mjs
 create mode 100644 ui-tui/scripts/visual/run.mjs

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 <tmpdir>/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<never>(() => {})
- 
+
 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 += '</div></body>'
-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()
 })

From cd24efec615cc0124e8be310679a6cc6cbc81780 Mon Sep 17 00:00:00 2001
From: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com>
Date: Tue, 21 Jul 2026 19:18:06 -0500
Subject: [PATCH 5/8] perf(ui-tui): shimmer loaders share one clock and stop
 after a bounded period
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Review on #20379, finding 5 (Perf). Every ShimmerRows mounted its own 90 ms
setInterval — the session panel can show lazy skills AND lazy tools at
once, and a lazy watch session stays lazy indefinitely, so an otherwise-
idle TUI ran ~22 React state updates per second forever.

All shimmer compositions now subscribe to a single module-level clock: one
interval regardless of how many skeletons are on screen, updates delivered
in one timer callback so React batches them into a single render pass, and
the interval is torn down with the last subscriber. Each mount's animation
is also bounded (SHIMMER_ANIMATE_MS, 30 s): after the budget the skeleton
freezes in place — it still reads as "loading" — and stops costing renders
entirely.

Tests: fake-timer coverage that N subscribers share one timer in lockstep,
the interval stops with the last unsubscribe, and a late subscriber
restarts the clock cleanly.
---
 ui-tui/src/__tests__/loaders.test.ts | 65 +++++++++++++++++++++++-
 ui-tui/src/components/loaders.tsx    | 74 +++++++++++++++++++++++++---
 2 files changed, 130 insertions(+), 9 deletions(-)

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/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
 }

From 7d757cf7e20444fb1e6483d2ced27744fc824304 Mon Sep 17 00:00:00 2001
From: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com>
Date: Tue, 21 Jul 2026 19:20:52 -0500
Subject: [PATCH 6/8] fmt(js): prettier pass on the new review tests

---
 ui-tui/src/__tests__/themeBoot.test.ts     | 5 +----
 ui-tui/src/__tests__/useConfigSync.test.ts | 3 ++-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/ui-tui/src/__tests__/themeBoot.test.ts b/ui-tui/src/__tests__/themeBoot.test.ts
index 29f406520656..0c2932872571 100644
--- a/ui-tui/src/__tests__/themeBoot.test.ts
+++ b/ui-tui/src/__tests__/themeBoot.test.ts
@@ -24,10 +24,7 @@ describe('seedBootEnvironment', () => {
   })
 
   it('never seeds over explicit user signals', () => {
-    for (const preset of [
-      { HERMES_TUI_THEME: 'dark' },
-      { HERMES_TUI_LIGHT: '1' }
-    ] as NodeJS.ProcessEnv[]) {
+    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)
 
diff --git a/ui-tui/src/__tests__/useConfigSync.test.ts b/ui-tui/src/__tests__/useConfigSync.test.ts
index 13bd5f4924f6..9191b26d70ba 100644
--- a/ui-tui/src/__tests__/useConfigSync.test.ts
+++ b/ui-tui/src/__tests__/useConfigSync.test.ts
@@ -380,7 +380,8 @@ describe('applyDisplay → voice.record_key (#18994)', () => {
 // (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 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 })
 

From 502939e19e3802ebd25cfbe390f2c825f10c0b14 Mon Sep 17 00:00:00 2001
From: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com>
Date: Tue, 21 Jul 2026 19:27:35 -0500
Subject: [PATCH 7/8] chore: gitignore node_modules symlinks, not just
 directories

Worktrees symlink node_modules to the main checkout; the dir-only
node_modules/ pattern doesn't match symlinks, so one slipped into a
commit and broke npm ci on CI (ENOTDIR). Dropping the trailing slash
matches both.
---
 .gitignore | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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

From 5c714be5fe781127c2f97d1575f0154f6b9c52a2 Mon Sep 17 00:00:00 2001
From: Brooklyn Nicholson <brooklyn.bb.nicholson@gmail.com>
Date: Tue, 21 Jul 2026 20:40:29 -0500
Subject: [PATCH 8/8] ci: retrigger (transient setup-uv manifest fetch flake)