fix(tui): reload.mcp lock scope + coalesced refresh + macOS polarity flip (Bugbot)

Three real findings from the review of the reload.mcp pooling + OSC-10 work:

- Lock released too early: the leader now holds _mcp_reload_lock across
  shutdown+discover AND its own agent refresh — releasing after discover let
  a second reload tear the registry down while the first was still reading it
  to rebuild the session's tool snapshot.
- Coalesced reload skipped the agent refresh: a follower returned
  "reloaded" without rebuilding ITS OWN session's snapshot, so a coalesced
  session kept stale tools. Followers now wait, then refresh their agent
  against the freshly-built registry (under the lock, skipping the redundant
  shutdown/discover). Shared _finish_reload tail for the `always` opt-out.
- macOS AppleInterfaceStyle fallback never set `resolved`, so a late OSC-10
  foreground reply could re-flip the committed inference (visible churn). It
  now marks resolved; a real OSC-11 background measurement still corrects it
  (that listener intentionally doesn't gate on resolved — measurement beats
  inference).

Bugbot's other four findings were diff-truncation false positives (color.ts,
themeBoot.ts, and the mcp_rev/light_colors/dark_colors types all exist;
typecheck + build green). 384 tui_gateway + TS suites pass.
This commit is contained in:
Brooklyn Nicholson 2026-07-21 13:49:54 -05:00
parent 11f2e54f0c
commit fdac23b641
2 changed files with 53 additions and 31 deletions

View file

@ -12881,6 +12881,24 @@ def _(rid, params: dict) -> dict:
_mcp_reload_lock = threading.Lock()
def _finish_reload(rid, params: dict, *, coalesced: bool) -> dict:
"""Shared tail for both reload paths: honor ``always`` (persist the
confirm opt-out) and return the ok payload."""
if bool(params.get("always", False)):
try:
from cli import save_config_value as _save_cfg
_save_cfg("approvals.mcp_reload_confirm", False)
except Exception as _exc:
logger.warning("Failed to persist mcp_reload_confirm=false: %s", _exc)
payload = {"status": "reloaded"}
if coalesced:
payload["coalesced"] = True
return _ok(rid, payload)
@method("reload.mcp")
def _(rid, params: dict) -> dict:
session = _sessions.get(params.get("session_id", ""))
@ -12935,26 +12953,16 @@ def _(rid, params: dict) -> dict:
from tools.mcp_tool import shutdown_mcp_servers, discover_mcp_tools
if not _mcp_reload_lock.acquire(blocking=False):
# A reload is already in flight; wait for it to finish and reuse
# its result instead of tearing the freshly-built registry down.
with _mcp_reload_lock:
pass
return _ok(rid, {"status": "reloaded", "coalesced": True})
try:
shutdown_mcp_servers()
discover_mcp_tools()
finally:
_mcp_reload_lock.release()
if session:
def _refresh_session_agent() -> None:
"""Rebuild THIS session's cached tool snapshot from the live
registry and push session.info. The agent snapshots tools once at
build and never re-reads the registry, so an explicit rebuild is
required (mirrors gateway/run.py::_execute_mcp_reload). Runs under
_mcp_reload_lock so the registry it reads can't be torn down by a
concurrent reload mid-refresh."""
if not session:
return
agent = session["agent"]
# Rebuild the cached agent's tool snapshot so the current session
# picks up added/removed MCP tools without `/new` (which discards
# history). The agent snapshots tools once at build and never
# re-reads the registry, so an explicit rebuild is required here.
# The user already consented to the prompt-cache invalidation via
# the confirm gate above. Mirrors gateway/run.py::_execute_mcp_reload.
try:
from tools.mcp_tool import refresh_agent_mcp_tools
@ -12970,22 +12978,30 @@ def _(rid, params: dict) -> dict:
"Failed to refresh cached agent tools after /reload-mcp: %s",
_exc,
)
_emit(
"session.info",
params.get("session_id", ""),
_session_info(agent, session),
)
_emit("session.info", params.get("session_id", ""), _session_info(agent, session))
# Honor `always=true` by persisting the opt-out to config.
if bool(params.get("always", False)):
# Serialize reloads. The LEADER (won the non-blocking acquire) holds
# the lock across shutdown+discover AND its own agent refresh —
# releasing after discover would let a second reload tear the registry
# down while this one is still reading it to rebuild the snapshot. A
# FOLLOWER (lock busy) waits, then — still holding the lock — refreshes
# ITS OWN session against the freshly-built registry (skipping the
# redundant shutdown/discover), so a coalesced session still gets an
# updated tool snapshot rather than silently keeping stale tools.
if _mcp_reload_lock.acquire(blocking=False):
try:
from cli import save_config_value as _save_cfg
shutdown_mcp_servers()
discover_mcp_tools()
_refresh_session_agent()
finally:
_mcp_reload_lock.release()
_save_cfg("approvals.mcp_reload_confirm", False)
except Exception as _exc:
logger.warning("Failed to persist mcp_reload_confirm=false: %s", _exc)
return _finish_reload(rid, params, coalesced=False)
return _ok(rid, {"status": "reloaded"})
with _mcp_reload_lock:
_refresh_session_agent()
return _finish_reload(rid, params, coalesced=True)
except Exception as e:
return _err(rid, 5015, str(e))

View file

@ -250,6 +250,12 @@ export function syncThemeToTerminalBackground(): void {
// every later signal (config pin, real OSC answer) still outranks it.
const dark = !error && stdout.trim() === 'Dark'
// Mark resolved so a LATE OSC-10 foreground reply (also an inference)
// can't re-flip this committed guess after the fact — visible churn.
// A real OSC-11 background answer still corrects it: that listener
// intentionally doesn't gate on `resolved` (a measurement outranks an
// inference).
resolved = true
process.env.HERMES_TUI_BACKGROUND = dark ? '#1e1e1e' : '#ffffff'
reapplyTheme()
})