fix(tui): revision-aware reload.mcp — an ack now means the revision was LOADED

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

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

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