fix: surface list-fetch failures instead of swallowing them

[cliffhall]

Closes #1953

A list load that failed reported nothing. ManagedListState fires the connect-time refresh with no caller to await it (void this.refresh()), so a rejection became an unhandled rejection — the header said Connected, the panel rendered an empty list indistinguishable from a server that legitimately has none, and the failed exchange appeared in the Protocol panel as a clean success. The only trace was a console error.

That is the bug misreported in #1893 as "replay validates more strictly than the initial call": the two are validated identically, but only the replay path surfaced the error (as a toast).

What changed

The panel says what happened. ManagedListState records the failure as observable state (getError() + an errorChange event) and still re-throws — the rejection is what App's auth-recovery wrapper keys off to detect a 401 and start a re-authorization, so swallowing it would have broken OAuth step-up. The callers with nobody to await them (connect-time load, list_changed auto-refresh) catch it explicitly now that the state holds it. The four useManaged* hooks expose it through a shared useManagedListError, and the Tools/Prompts/Resources sidebars render a new ListLoadError element with the reason and a Retry.

The Protocol entry stops lying. A response the client refused is attributed back to its entry — markResponseRejected → responseRejected → MessageEntry.clientError — which now renders an Error status and a "Rejected by the Inspector" alert in the expanded detail, instead of the green resultType badge alone (that badge was suppressing the status badge, which is why the failed call looked green).

The SDK gives no request id with a decode failure — SdkError carries the method and nothing else — so the id is recovered by correlation with the last response received for that method. That's exact rather than approximate: the SDK rejects synchronously while decoding, inside the transport's onmessage, and the caller's catch runs in the very next microtask; delivering another response for the same method in that window would take a macrotask (a socket read), which cannot interleave there. The reasoning is on the method.

No more floor-dropping in listAllTools. The excluded-tools walk stays non-fatal but is now logged instead of .catch(() => {}), so a failing walk doesn't leave the "Excluded (SEP-2243)" section silently empty.

Not changed, deliberately

The strictness itself is correct — the 2026-07-28 normative schema declares ttlMs: number and cacheScope: "public" | "private" as required on ListToolsResult and its siblings, so the SDK is conformant and nothing here loosens it. (The spec's caching page contradicts its own schema by telling clients to tolerate an absent ttlMs; that belongs upstream.)

Verification

Reproduced with the repo's modern test server behind a proxy that strips ttlMs/cacheScope from */list results, connected with protocolEra: "modern".

Before — "Connected", empty Tools panel, Protocol entry showing a clean COMPLETE round-trip:

After — the panel names the failure with the reason and a Retry; the Protocol entry carries an ERROR badge alongside the wire's COMPLETE, and expands to "Rejected by the Inspector":

Retry was exercised live: with the proxy switched back to pass-through, clicking Retry clears the alert and loads the tools.

Tests

• managedToolsState — 10 cases covering the error lifecycle: recorded and re-thrown, errorChange dispatch, non-Error rejection wrapping, cleared on success, cleared on disconnect, no re-dispatch for the same error, and the two unawaited paths (connect, auto-refresh) landing in state rather than rejecting unobserved.
• The other three managed states pin their listMethod string, which is what attributes a failure to the right entry.
• useManagedListError — 7 cases (seed, update, clear, null state, unsubscribe).
• ListLoadError — 6 cases; plus 4 stories.
• messageLogState — 5 cases for responseRejected, including the standalone-response and reused-id paths.
• inspectorClient-response-rejected.test.ts — a new integration file driving a real stdio connection so the id correlation runs against real SDK-assigned ids: latest-response attribution, no cross-method bleed, no-op for an unanswered method.
• ProtocolEntry — 4 cases for the rejected-response rendering.
• The three control panels — retry wiring and the default no-error state.

npm run ci passes: validate → coverage (per-file ≥90 held) → verify:build-gate → smoke (incl. smoke:web:app) → Storybook (110 files, 466 tests).

Note: npm run coverage exits 1 on this branch and on v2/main with 2 pre-existing unhandled CONNECTION_CLOSED rejections from inspectorClient.test.ts's disconnect-during-in-flight-progress tests. Verified identical on a stashed tree; unrelated to this change and worth its own issue.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt

[Copilot]

Pull request overview

This PR fixes a UX/correctness gap where connect-time (and other unawaited) list loads could fail silently: the sidebar looked like an empty-but-valid list, and the Protocol entry looked successful even when the SDK rejected the response payload.

Changes:

• Add observable “last list-load error” state to ManagedListState (plus a shared React hook) so list failures surface to the UI while still re-throwing for awaited callers.
• Introduce responseRejected plumbing to attribute SDK decode/validation rejections back onto the correct Protocol entry as a client-side error.
• Render list-load failures in the Tools/Prompts/Resources sidebars via a new ListLoadError element with a Retry action; log excluded-tools walk failures instead of dropping them.

Reviewed changes

Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
core/react/useManagedTools.ts	Exposes managed tools list-load error via useManagedListError.
core/react/useManagedResourceTemplates.ts	Exposes templates list-load error via useManagedListError.
core/react/useManagedResources.ts	Exposes resources list-load error via useManagedListError.
core/react/useManagedPrompts.ts	Exposes prompts list-load error via useManagedListError.
core/react/useManagedListError.ts	New shared hook subscribing to errorChange from managed list states.
core/mcp/types.ts	Adds MessageEntry.clientError to represent client-side rejection reasons.
core/mcp/state/messageLogState.ts	Listens for responseRejected to annotate the matching message entry with clientError.
core/mcp/state/managedToolsState.ts	Pins listMethod: "tools/list" and inherits ManagedListEventMap.
core/mcp/state/managedResourceTemplatesState.ts	Pins listMethod: "resources/templates/list" and inherits ManagedListEventMap.
core/mcp/state/managedResourcesState.ts	Pins listMethod: "resources/list" and inherits ManagedListEventMap.
core/mcp/state/managedPromptsState.ts	Pins listMethod: "prompts/list" and inherits ManagedListEventMap.
core/mcp/state/managedListState.ts	Adds error state + errorChange; catches unawaited refresh rejections; marks rejected Protocol responses.
core/mcp/inspectorClientProtocol.ts	Adds optional markResponseRejected() API to the protocol interface.
core/mcp/inspectorClientEventTarget.ts	Adds responseRejected event to propagate rejection reason + correlated id.
core/mcp/inspectorClient.ts	Tracks last-answered id per method and emits responseRejected; logs excluded-tools refresh failures.
core/mcp/tests/fakeInspectorClient.ts	Adds markResponseRejected spy for managed-state tests.
clients/web/src/test/integration/mcp/inspectorClient-response-rejected.test.ts	Integration coverage for id correlation and clientError attribution on real stdio connection.
clients/web/src/test/core/react/useManagedListError.test.tsx	Unit coverage for the shared error subscription hook.
clients/web/src/test/core/mcp/state/messageLogState.test.ts	Tests responseRejected annotates the right entry and ignores invalid targets.
clients/web/src/test/core/mcp/state/managedToolsState.test.ts	Tests error lifecycle: record + rethrow, dispatch, clear, connect-time + auto-refresh handling.
clients/web/src/test/core/mcp/state/managedResourceTemplatesState.test.ts	Pins method string + attribution behavior for templates.
clients/web/src/test/core/mcp/state/managedResourcesState.test.ts	Pins method string + attribution behavior for resources.
clients/web/src/test/core/mcp/state/managedPromptsState.test.ts	Pins method string + attribution behavior for prompts.
clients/web/src/components/views/InspectorView/InspectorView.tsx	Wires list-load error props through to screens.
clients/web/src/components/screens/ToolsScreen/ToolsScreen.tsx	Threads loadError to sidebar controls.
clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.tsx	Threads loadError to sidebar controls.
clients/web/src/components/screens/PromptsScreen/PromptsScreen.tsx	Threads loadError to sidebar controls.
clients/web/src/components/groups/ToolControls/ToolControls.tsx	Renders ListLoadError above the tools sidebar list.
clients/web/src/components/groups/ToolControls/ToolControls.test.tsx	Adds coverage for sidebar error rendering + Retry wiring.
clients/web/src/components/groups/ResourceControls/ResourceControls.tsx	Renders ListLoadError above the resources sidebar list.
clients/web/src/components/groups/ResourceControls/ResourceControls.test.tsx	Adds coverage for sidebar error rendering + Retry wiring.
clients/web/src/components/groups/PromptControls/PromptControls.tsx	Renders ListLoadError above the prompts sidebar list.
clients/web/src/components/groups/PromptControls/PromptControls.test.tsx	Adds coverage for sidebar error rendering + Retry wiring.
clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.tsx	Renders client-rejected results as Error and shows “Rejected by the Inspector” detail.
clients/web/src/components/groups/ProtocolEntry/ProtocolEntry.test.tsx	Adds coverage for rendering a client-rejected response.
clients/web/src/components/elements/ListLoadError/ListLoadError.tsx	New element: “couldn’t load” alert + capped diagnostic + Retry button.
clients/web/src/components/elements/ListLoadError/ListLoadError.test.tsx	Unit tests for ListLoadError.
clients/web/src/components/elements/ListLoadError/ListLoadError.stories.tsx	Storybook stories for common error scenarios.
clients/web/src/App.tsx	Plumbs error out of managed hooks into InspectorView.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.

Suppressed comments (1)

core/mcp/state/managedListState.ts:317

• ManagedListState.refresh() calls this.client?.markResponseRejected?.(...) for any failure from fetchItems() (transport failures, timeouts, server JSON-RPC errors, etc.), but markResponseRejected is documented/implemented as “a response the client rejected after the fact” (i.e., a wire response existed and was logged). For failures that occur without a corresponding response frame, this can misattribute the error to the last successful response for the same method and render a misleading “Rejected by the Inspector” Protocol entry.

Consider only calling markResponseRejected when the thrown error is specifically an SDK decode/validation rejection (and otherwise skipping it), or moving the rejection marking into the layer that actually observes the decode failure + the correlated id.

    } catch (err) {
      const error = err instanceof Error ? err : new Error(String(err));
      this.setError(error);
      // Attribute the failure to the response it came from, so the Protocol
      // entry stops rendering a rejected result as a clean success (#1953).
      // Must happen in this catch, while the correlation window the client
      // documents is still valid.
      this.client?.markResponseRejected?.(
        this.config.listMethod,
        error.message,
      );

[cliffhall]

Picking up the suppressed comment on managedListState.ts:317 — it shouldn't have been suppressed, it was the most serious finding on this PR. Fixed in fad8480.

It's right, and it's the same bug this PR exists to remove, just pointed the other way. The correlation recovers the id as "the last response received for this method", which is the failing exchange only when a response actually arrived and was refused while decoding. On a transport drop, a timeout, or an abort, no response frame arrives at all — so lastAnsweredRequestByMethod still points at an earlier, successful call, and we'd stamp "Rejected by the Inspector" onto an exchange that worked. My correlation comment argued the microtask window rules out interleaving, which is true, but it silently assumed a response had arrived at all. That assumption wasn't stated and wasn't enforced.

The call is now gated:

function isClientDecodeRejection(err: unknown): boolean {
  return (
    SdkError.isInstance(err) &&
    (err.code === SdkErrorCode.InvalidResult ||
      err.code === SdkErrorCode.UnsupportedResultType)
  );
}

Both codes mean "a result arrived and the client refused it" — a decision made about a frame in hand. I took the first of your two options; the second (moving the marking into the layer that observes the decode failure with the id) is the better design, but that layer is inside the SDK's _onresponse, which is why the correlation exists in the first place.

Two things worth noting:

A server-sent JSON-RPC error is excluded too, for a different reason than transport failures. There the id would be correct — the error frame is a real response and does update the correlation map — but the failure is the server's, and the entry already renders as an error from the error frame itself. Labeling it "Rejected by the Inspector" would misattribute the cause. Different reasoning, same exclusion, and the guard's doc comment separates the two so a future reader doesn't collapse them.

The failure is still recorded as list state regardless. Only the Protocol attribution is gated — a transport drop still shows "Couldn't load tools" with the reason and a Retry in the sidebar. There's an explicit test for that (still records every failure as state, attributed or not) so a future tightening of the guard can't silently take the panel error down with it.

Tests added: InvalidResult and UnsupportedResultType attribute; ConnectionClosed, RequestTimeout, and a plain non-SDK Error do not.

Also re-verified live against the repro (modern server behind a proxy stripping ttlMs/cacheScope) that the real decode rejection still reaches both surfaces after the gate — the risk with a change like this is over-tightening it into a no-op, so the positive path was worth re-checking rather than assuming.

npm run ci is green end to end: validate 0, 4860 tests passing, build-gate 0, smoke 0, Storybook 0. (coverage still exits 1 on the 2 pre-existing CONNECTION_CLOSED unhandled rejections that are also present on v2/main — unrelated to this PR.)

[Copilot]

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.

Suppressed comments (2)

core/react/useManagedListError.ts:57

• useManagedListError re-syncs local React state from the state prop inside a useEffect (setError(state.getError()) and setError(null)), which can briefly render the previous state's error after state changes (e.g. when switching servers in the TUI) until the effect runs. This pattern is also explicitly discouraged in this repo’s React guidelines (state derived from props shouldn’t be re-synced in an effect).
  core/mcp/inspectorClient.ts:367
• The new outboundRequestMethods / lastAnsweredRequestByMethod correlation maps are never cleared on disconnect or transport teardown. If a session ends with in-flight requests (timeouts, connection drops), outboundRequestMethods can retain abandoned ids and grow across reconnects; clearing both maps on disconnect/onclose would avoid a small memory leak and remove any possibility of stale correlation data persisting across sessions.

  // Correlation for `markResponseRejected` (#1953): the method of each
  // outbound request still awaiting a response, and — once one is answered —
  // the id of the most recently answered request per method. Entries are
  // dropped as responses arrive, so this holds at most one id per method
  // rather than growing with the session.
  private outboundRequestMethods = new Map<string | number, string>();
  private lastAnsweredRequestByMethod = new Map<string, string | number>();

[cliffhall]

Both suppressed comments this round were valid — surfacing them here since they don't appear inline. Fixed in a6f233e.

1. useManagedListError derived state from props in an effect

Correct, and it's a direct violation of this repo's own rule (AGENTS.md → "NEVER reset or re-sync local state from a prop inside a useEffect"). The symptom is exactly as described: after state changes, one frame renders the previous store's error before the effect corrects it.

The prescribed fix is useValueChange, but that hook lives in clients/web/src/hooks/ and this one is in core/react/, which the CLI and TUI also consume — so it isn't reachable. I used useSyncExternalStore instead, which is the better answer for this shape anyway: the snapshot is read during render, so a store swap lands in the same frame, and it also closes a second gap the old code had — an error recorded between render and the effect subscribing was silently missed.

The snapshot returns the stored Error instance (or null), never a fresh object, so it's referentially stable as the API requires.

Regression test asserts the swap with no waitFor and no act() flush:

rerender({ s: other });
expect(result.current).toBeNull();   // fails on the old pattern

That immediacy is the point — a waitFor here would have passed under the buggy version too, restating eventual consistency instead of testing the frame.

Note the four sibling hooks (useManagedTools and friends) still use the older useState + useEffect subscribe pattern and have the same latent issue for their lists. Out of scope here, but it's a real cleanup and I'd rather it be tracked than quietly noticed — I'll file it separately.

2. Correlation maps never cleared

Also correct. trackResponse removes an id when its response arrives, so the leak is specifically the requests that never get one — a timeout, a dropped connection — which accumulate across reconnects.

Cleared in resetSessionState() rather than in disconnect()/onclose. That's the convention this class already documents: one route out (onerror with no onclose) runs neither teardown path, so the file deliberately starts clean at connect() instead of relying on the way out. Clearing there covers both concerns you raised — the accumulation, and any possibility of cross-session correlation data (which MessageLogState would already have orphaned, since it drops its entries on disconnect).

---

npm run ci green: validate 0, coverage 315 files / 4861 tests, build-gate 0, smoke 0, Storybook 0.

Two known non-issues for the record: coverage exits 1 on 2 pre-existing CONNECTION_CLOSED unhandled rejections also present on v2/main; and one coverage run hit a 30s timeout in inspectorClient-tasks-era.test.ts that I confirmed is a flake (passes in isolation twice, and the full re-run is green).

[cliffhall]

Filed the sibling-hook cleanup as #1955 (the other core/react hooks carry the same re-sync-in-effect pattern for their lists). Kept out of this PR — the stores return defensive copies, so converting them to useSyncExternalStore needs a stable-snapshot change in each store, which is real work rather than a drive-by.

[Copilot]

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated no new comments.