Merge pull request #68999 from NousResearch/bb/widget-grid-hardening

fix(ui-tui): widget-grid hardening — review fast-follow for #20379

.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

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

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:

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",

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')
+}

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)

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')])

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()
 })

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()
+  })
+})

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> = {}): 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')
+  })
+})

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

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)
+  })
+})

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
     }
 

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')
           )

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
       }
 

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
 }

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 {

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