fix(mcp): review round 5 — thread the refresh outcome to every operator surface

The 'skipped'/None refresh sentinel added in round 4 was only half
threaded: consumers still misreported it. Unify all operator surfaces
on ONE source of truth — the per-server last_refresh_outcome ('ok' /
'skipped' / 'error:<Class>') — exposed via a new last_refresh_outcome()
accessor:

- _refresh_all returns None (not ([], [])) for a FAILURE too, so a
  failed refresh is never rendered as 'no changes' (the pre-#839 lie
  the sentinel exists to close); None is disambiguated skipped-vs-failed
  by the outcome. ([], []) now strictly means 'ran, no changes'.
- /mcp refresh renders skip ('skipped — retry scheduled') and failure
  ('refresh failed (error:X)') distinctly from 'no changes'.
- The node-internal refresh endpoint returns 202 'skipped' instead of a
  misleading 200 'ok' for a refresh that never ran (the busy-lock skip);
  it reads the outcome from the manager accessor because the public
  status projection deliberately whitelists last_refresh_outcome out.
- admin.js paints 'skipped' with a neutral info pill
  (.mcp-refresh-pill-skip), not the error-red any-non-'ok' used to get.
- _admit_list_changed rolls back BOTH the coalesce marker and the
  debounce stamp when scheduling raises, so a same-kind push in the
  window afterward isn't debounced against a refresh that never spawned
  (the pool path has no on_debounce_drop recovery).

Tests: endpoint 202-skip, CLI skip/failure render, _refresh_all
failure→None + outcome, spawn-failure stamp+marker rollback. Suite
9399 green.

NOTE filed #843: the admin refresh pill's data (last_refresh_at/outcome)
is stripped by BOTH status projections and never reaches admin.js — a
pre-existing latent bug (the pill has never rendered); the admin.js
color fix here is correct-when-reachable. Out of #839 scope (the read
projection strips it for a privacy reason that needs its own coarsening
decision).

Refs #839

CHANGELOG.md

@@ -202,7 +202,15 @@ Earlier stable lines (`stable/1.6`, `stable/1.5`) are frozen.
   stranding a live session and published catalog with the config already
   gone), and `reconcile_sync` retries a removal that timed out instead of
   marking it done — previously a DB-driven delete of a busy server could be a
-  silent, permanent no-op until process restart.
+  silent, permanent no-op until process restart. A refresh outcome now
+  threads consistently to every operator surface off one source of truth
+  (the per-server `last_refresh_outcome`): a busy-skip and a genuine failure
+  are each reported distinctly from a real "no changes" — `/mcp refresh`
+  prints "skipped" or "failed" rather than a false "no changes", the
+  node-internal refresh endpoint returns `202 skipped` instead of a
+  misleading `200 ok` for a refresh that never ran, and the admin console's
+  refresh pill paints a benign "skipped" in a neutral tint rather than
+  failure-red.
 
 - **OpenAI Responses streaming: truncated and refused responses no longer
   vanish.** A response that hit `max_output_tokens` terminates the stream

tests/test_mcp_admin_api.py

@@ -2578,6 +2578,35 @@ class TestInternalMcpRefreshOneEndpoint:
         assert "url" not in data["server"]
         assert data["server"]["circuit_open"] is True
 
+    def test_refresh_one_skipped_returns_202(self, node_app_factory) -> None:
+        # A busy-lock skip never ran the refresh — it must NOT be reported
+        # as 200 "ok" (the caller would believe the catalog is current).
+        # 202 Accepted + status "skipped": the health-tick retry will run it.
+        mgr = MagicMock()
+        mgr.refresh_sync.return_value = {"srv": None}
+        # The endpoint reads the outcome from the manager accessor, not the
+        # stripped status (the public projection whitelists it out).
+        mgr.last_refresh_outcome.return_value = "skipped"
+        mgr.get_server_status.return_value = {
+            "connected": True,
+            "tools": 3,
+            "resources": 0,
+            "prompts": 1,
+            "error": "",
+            "transport": "stdio",
+            "command": "secret",
+            "url": "",
+            "circuit_open": False,
+            "consecutive_failures": 0,
+        }
+        c = node_app_factory(mgr)
+        r = c.post("/v1/api/_internal/mcp-refresh/srv")
+        assert r.status_code == 202
+        data = r.json()
+        assert data["status"] == "skipped"
+        assert "command" not in data["server"]  # stripped
+        mgr.last_refresh_outcome.assert_called_with("srv")
+
     def test_refresh_one_invalid_name_returns_400(self, node_app_factory) -> None:
         # sec-4: name validation symmetric with console side.
         mgr = MagicMock()

tests/test_mcp_client.py

@@ -1499,6 +1499,36 @@ class TestSessionRefresh:
         session.ui.on_error.assert_called_once()
         assert "MCP refresh failed" in session.ui.on_error.call_args[0][0]
 
+    def test_mcp_refresh_renders_skip_distinctly(self, tmp_db):
+        # A None result (busy-skip) with a "skipped" outcome must NOT render
+        # as "no changes" — the operator would think the server is current.
+        mock_mcp = MagicMock()
+        mock_mcp.get_tools.return_value = [_fake_openai_tool()]
+        mock_mcp.server_names = ["srv"]
+        mock_mcp.refresh_sync.return_value = {"srv": None}
+        mock_mcp.last_refresh_outcome.return_value = "skipped"
+        session = self._make_session(mcp_client=mock_mcp)
+
+        session.handle_command("/mcp refresh")
+        rendered = session.ui.on_info.call_args[0][0]
+        assert "skipped" in rendered
+        assert "no changes" not in rendered
+
+    def test_mcp_refresh_renders_failure_distinctly(self, tmp_db):
+        # A None result whose outcome is an error must render as a failure,
+        # NOT as "no changes" (the pre-#839 lie the None sentinel closes).
+        mock_mcp = MagicMock()
+        mock_mcp.get_tools.return_value = [_fake_openai_tool()]
+        mock_mcp.server_names = ["srv"]
+        mock_mcp.refresh_sync.return_value = {"srv": None}
+        mock_mcp.last_refresh_outcome.return_value = "error:ConnectionError"
+        session = self._make_session(mcp_client=mock_mcp)
+
+        session.handle_command("/mcp refresh")
+        rendered = session.ui.on_info.call_args[0][0]
+        assert "failed" in rendered
+        assert "no changes" not in rendered
+
 
 # ---------------------------------------------------------------------------
 # MCP Resources
@@ -2593,12 +2623,17 @@ class TestCircuitBreaker:
             mock_session.list_prompts = AsyncMock(side_effect=dead)
             _seed_static_state(mgr, "test", session=mock_session)
 
-            await mgr._refresh_all("test")
+            results = await mgr._refresh_all("test")
 
             # Dead session evicted → next refresh tick / dispatch reconnects.
             assert mgr._static_servers["test"].session is None
             ts, outcome = mgr._last_refresh["test"]
             assert outcome == "error:ClosedResourceError"
+            # A FAILURE reports None, NOT ([], []) — rendering a failed
+            # refresh as "no changes" is the operator lie the sentinel
+            # closes; the outcome disambiguates None (error vs skipped).
+            assert results["test"] is None
+            assert mgr.last_refresh_outcome("test") == "error:ClosedResourceError"
 
         asyncio.run(_run())
 
@@ -3358,6 +3393,32 @@ class TestStaticNotificationRefresh:
             "a same-window different-kind notification must spawn its own refresh"
         )
 
+    def test_scheduling_failure_rolls_back_stamp_and_marker(self, running_loop_mgr) -> None:
+        """If _spawn_background raises (loop tearing down), BOTH the coalesce
+        marker and the debounce stamp we just wrote must be rolled back —
+        else a same-kind push landing in the window afterward is debounced
+        against a stamp for a refresh that never spawned, and (on the pool
+        path, no on_debounce_drop) never recovers."""
+        mgr, loop, _thread = running_loop_mgr
+        handler = mgr._make_static_notification_handler("srv")
+
+        def _boom(coro: Any, _label: str) -> None:
+            coro.close()  # avoid "coroutine was never awaited" — prod is loop-teardown
+            raise RuntimeError("loop closing")
+
+        async def _fire() -> None:
+            mgr._static_connect_lock_for("srv")
+            _seed_static_state(mgr, "srv", session=MagicMock())
+            note = mcp_types.ServerNotification(
+                mcp_types.ToolListChangedNotification(method="notifications/tools/list_changed")
+            )
+            with patch.object(mgr, "_spawn_background", side_effect=_boom):
+                await handler(note)  # must swallow the scheduling failure
+
+        _run_on_loop(loop, _fire())
+        assert ("srv", "tools") not in mgr._last_notification_refresh, "stamp must roll back"
+        assert ("srv", "tools") not in mgr._static_refresh_pending, "marker must roll back"
+
     def test_same_kind_debounce_drop_arms_retry(self, running_loop_mgr) -> None:
         """A SAME-kind push landing in the debounce window AFTER the
         previous runner completed (no runner queued) is genuinely lost to

turnstone/console/static/admin.js

@@ -4811,8 +4811,13 @@ function _renderMcpServers(items) {
         ageShort = Math.floor(ageSeconds / 3600) + "h";
       else ageShort = Math.floor(ageSeconds / 86400) + "d";
       const outcomeText = newestRefreshOutcome || "unknown";
-      const pillCls =
-        outcomeText === "ok" ? "mcp-refresh-pill-ok" : "mcp-refresh-pill-err";
+      // "skipped" is benign (lock busy, retry scheduled) — a neutral info
+      // pill, not the error-red any-non-ok used to paint; only genuine
+      // "error:*" outcomes are failures.
+      let pillCls;
+      if (outcomeText === "ok") pillCls = "mcp-refresh-pill-ok";
+      else if (outcomeText === "skipped") pillCls = "mcp-refresh-pill-skip";
+      else pillCls = "mcp-refresh-pill-err";
       const pillTitle =
         "Last refresh " +
         new Date(newestRefreshAt * 1000).toISOString() +

turnstone/console/static/style.css

@@ -2823,6 +2823,13 @@ h3.skill-spec-heading {
   background: color-mix(in srgb, var(--warn) 15%, transparent);
   color: var(--warn);
 }
+/* "skipped" is benign and transient (the server's lock was busy; a retry
+   is scheduled) — an info tint, NOT the warn red the error pill uses, so a
+   healthy busy server is not painted as failed. */
+.mcp-refresh-pill-skip {
+  background: color-mix(in srgb, var(--accent) 15%, transparent);
+  color: var(--accent);
+}
 
 .mcp-detail-modal::before {
   background: linear-gradient(

turnstone/core/mcp_client.py

@@ -1674,12 +1674,16 @@ class MCPClientManager:
                 f"{label_prefix} {kind} refresh for '{label_target}'",
             )
         except Exception as exc:
-            # Scheduling failed (loop shutting down) — release the
-            # coalesce marker or this key+kind never refreshes again.
-            # Structured fields only — ``exc_info=True`` would serialize
-            # the chained ``httpx.Request`` whose headers carry the
-            # bearer (configured for auth_type=static, minted for pool).
+            # Scheduling failed (loop shutting down) — release BOTH the
+            # coalesce marker and the debounce stamp we just wrote, or a
+            # same-kind push landing inside the window afterward is
+            # debounced against a stamp for a refresh that never spawned
+            # (the pool path has no on_debounce_drop recovery). Structured
+            # fields only — ``exc_info=True`` would serialize the chained
+            # ``httpx.Request`` whose headers carry the bearer (configured
+            # for auth_type=static, minted for pool).
             pending.discard(marker)
+            stamps.pop(marker, None)
             log.warning(
                 "list_changed refresh scheduling failed for %s exc=%s",
                 origin,
@@ -4292,6 +4296,19 @@ class MCPClientManager:
         self._set_error(name, f"Refresh failed: {type(exc).__name__}: {exc}")
         self._arm_refresh_retry(name)
 
+    def last_refresh_outcome(self, name: str) -> str | None:
+        """Authoritative last-refresh outcome for static server *name*.
+
+        One of ``"ok"``, ``"skipped"``, ``"error:<Class>"``, or ``None``
+        (no refresh since start). The single source of truth every
+        operator surface renders from — the ``refresh_sync`` diff tuple
+        says only WHAT changed, not whether the pass ran, was skipped, or
+        failed. Static-only (reads ``_last_refresh`` directly); no
+        oauth_user status routing.
+        """
+        row = self._last_refresh.get(name)
+        return row[1] if row is not None else None
+
     def _arm_refresh_retry(self, name: str) -> None:
         """Arm the health-tick refresh retry for *name* — the ONE gate copy.
 
@@ -4480,14 +4497,23 @@ class MCPClientManager:
         """Refresh tools, resources, and prompts for one or all servers.
 
         For disconnected servers (in config but not connected), attempts
-        reconnect.  Returns ``{server: (added, removed)}`` per server —
-        or ``None`` for a server whose refresh was SKIPPED (another
-        operation held its lock, or it was removed/evicted mid-pass).
-        ``None`` is a distinct shape on purpose: rendering a skip as
-        ``([], [])`` told the operator "refreshed, no changes" for a
-        server that was never refreshed at all. Skips on live servers
-        also stamp a ``skipped`` ``_last_refresh`` row so the admin pill
-        tells the same story.
+        reconnect. Per-server value:
+
+        * ``(added, removed)`` — the refresh RAN to completion; the tuple
+          is the tool diff (``([], [])`` = ran, no changes). Outcome
+          ``ok``.
+        * ``None`` — the refresh produced NO catalog, either SKIPPED
+          (another operation held the lock, or removed/evicted mid-pass;
+          outcome ``skipped``) or FAILED (it ran and errored; outcome
+          ``error:<Class>``). Consumers disambiguate via the
+          ``last_refresh_outcome`` status field.
+
+        ``None`` is a distinct shape on purpose: rendering either a skip
+        or a failure as ``([], [])`` told the operator "refreshed, no
+        changes" for a server that was never refreshed / whose refresh
+        failed. The ``_last_refresh`` status row carries the authoritative
+        outcome for every operator surface (CLI, HTTP endpoint, admin
+        pill) so they render consistently off ONE source of truth.
         """
         results: dict[str, tuple[list[str], list[str]] | None] = {}
         targets = [server_name] if server_name else list(self._server_configs.keys())
@@ -4564,7 +4590,13 @@ class MCPClientManager:
                 # abort the whole refresh pass. Redaction + retry-arm live
                 # in the shared helper.
                 self._record_refresh_failure(name, exc, context="Refresh")
-                results[name] = ([], [])
+                # None, NOT ``([], [])``: a failure produced no catalog, and
+                # ``([], [])`` is reserved for "ran, no changes" — rendering
+                # a failure as "no changes" is the operator lie the sentinel
+                # exists to prevent. Consumers disambiguate None (skipped vs
+                # failed) via ``last_refresh_outcome``, which the
+                # ``error:<Class>`` write at the end of this block sets.
+                results[name] = None
                 # A dead transport leaves a non-None but unusable session, so
                 # the reconnect branch at the top of this loop (gated on
                 # ``session is None``) would never fire and every later pass
@@ -4605,13 +4637,13 @@ class MCPClientManager:
     ) -> dict[str, tuple[list[str], list[str]] | None]:
         """Refresh tools synchronously (blocks the calling thread).
 
-        Returns ``{server: (added_names, removed_names)}`` per server, or
-        ``{server: None}`` when that server's refresh was SKIPPED (its
-        lock was held by a concurrent reconnect/refresh, or it was
-        removed mid-pass). Callers MUST distinguish ``None`` from
-        ``([], [])``: the latter is a refresh that ran and found no
-        changes; ``None`` is a refresh that never ran, deferred to the
-        health-tick retry.
+        Returns ``{server: (added_names, removed_names)}`` for a refresh
+        that RAN (``([], [])`` = no changes), or ``{server: None}`` when
+        it produced no catalog — SKIPPED (lock held by a concurrent
+        reconnect/refresh, or removed mid-pass) or FAILED. Callers MUST
+        NOT render ``None`` as "no changes"; consult
+        :meth:`last_refresh_outcome` (or the status ``last_refresh_outcome``
+        field) to distinguish ``skipped`` from ``error:<Class>``.
         """
         assert self._loop is not None
         future = asyncio.run_coroutine_threadsafe(self._refresh_all(server_name), self._loop)

turnstone/core/session.py

@@ -3059,12 +3059,17 @@ class ChatSession:
         lines: list[str] = []
         for srv, diff in sorted(results.items()):
             if diff is None:
-                # Skipped, NOT refreshed: another operation held the
-                # server's lock (a reconnect / a push refresh), or it was
-                # removed mid-pass. Reported distinctly from "no changes"
-                # so the operator isn't told a stale server is current;
-                # the health-tick retry runs the real refresh shortly.
-                lines.append(f"  {srv}: {dim('skipped (busy — retry scheduled)')}")
+                # No catalog produced — the refresh either was SKIPPED
+                # (lock held by a concurrent reconnect/push refresh, or
+                # removed mid-pass) or FAILED. Disambiguate via the
+                # authoritative outcome so the operator is never told a
+                # stale/broken server is current. Both are distinct from
+                # "no changes".
+                outcome = self._mcp_client.last_refresh_outcome(srv) or ""
+                if outcome.startswith("error"):
+                    lines.append(f"  {srv}: {RED}refresh failed{RESET} ({dim(outcome)})")
+                else:
+                    lines.append(f"  {srv}: {dim('skipped (busy — retry scheduled)')}")
                 continue
             added, removed = diff
             if added or removed:

turnstone/server.py

@@ -3459,9 +3459,19 @@ def internal_mcp_refresh_one(request: Request) -> JSONResponse:
         return JSONResponse({"status": "error", "error": "refresh failed"}, status_code=500)
 
     # _refresh_all swallows per-server errors into _last_error rather than
-    # raising, so a 200-OK from refresh_sync isn't enough — re-check status
-    # and surface 500 if the refresh actually failed for this server.
+    # raising, so a 200-OK from refresh_sync isn't enough — re-check the
+    # authoritative outcome. A refresh can (a) run and fail → 500, (b) be
+    # SKIPPED because the server's connect lock was busy (a reconnect / a
+    # push refresh already running) → 202 with status "skipped", the
+    # health-tick retry will run it, or (c) run cleanly → 200 ok. Reporting
+    # a skip as 200 "ok" told the caller the catalog is current when
+    # nothing ran. The outcome is read from the manager directly — the
+    # public status projection whitelists ``last_refresh_outcome`` out (it
+    # encodes the error class, which read-scope deliberately coarsens to
+    # ``has_error``), so it cannot be recovered from the stripped dict.
     status = _public_server_status(mcp_mgr, name)
+    if mcp_mgr.last_refresh_outcome(name) == "skipped":
+        return JSONResponse({"status": "skipped", "server": status}, status_code=202)
     if status.get("error"):
         log.warning(
             "internal_mcp_refresh_one: refresh reported error for %s: %s", name, status["error"]