fix: open the modern listen stream whenever the filter is non-empty (#1920)

[cliffhall]

Closes #1920

The gap

On the modern (2026-07-28) era every server→client notification rides the subscriptions/listen stream. The Inspector only ever opened that stream from a resource-subscribe click:

// Nothing subscribed → keep the stream closed.
if (this.subscribedResources.size === 0) { … return; }

…while the filter it builds already modelled the list-change opt-ins (toolsListChanged, etc.). So a tools-only server that correctly advertises tools.listChanged had no path to an open stream — notifications/tools/list_changed was unreachable from the UI, because the Resources screen (the only affordance that opened it) isn't there when the server has no resources.

The fix

Option 1 from the issue thread: the trigger now matches the filter it builds. The stream opens — and stays open, and reconnects — whenever buildSubscriptionFilter() returns anything: subscribed URIs or an enabled list-change opt-in the server advertises. That is the same rule the SDK's own ClientOptions.listChanged auto-open uses (non-empty config ∩ capability intersection), applied to the hand-managed stream the Inspector keeps for its re-list/reconnect semantics.

• wantsModernStream() replaces subscribedResources.size === 0 at every "should the stream be open" site: the refresh bail, the reconnect decision after a drop, the reconnect timer's bail, the give-up check, and the post-failure reconcile.
• connect() opens the stream once the notification handlers are registered. A failure there does not fail the connect — the handshake succeeded and every request-scoped feature works without the stream — it is handed to the existing reconnect machinery, which retries with backoff and settles on ended past the cap.
• buildSubscriptionFilter() now omits resourceSubscriptions when nothing is subscribed, rather than sending [].

What active still means

ResourceSubscriptionStreamState.active deliberately keeps its narrower meaning — at least one URI is subscribed — via a separate modernStreamActive(). It drives the Subscriptions section's badge, which has nothing to report for a stream open purely for list-change notifications. Keeping the two predicates apart also preserves the invariant the rest of inspectorClient.ts is written against: an empty subscribed set is never announced alongside an active stream.

Tests

New modern era: opening the stream for listChanged alone (#1920) block in inspectorClient-subscriptions-era.test.ts, against a real tools-only server over a real transport:

• opens the stream on connect against a tools-only server (filter carries toolsListChanged, no resourceSubscriptions; the Subscriptions state stays inactive)
• leaves it closed when the opt-in is disabled in the Inspector's config (config ∩ capability)
• opens nothing on the legacy era
• reconnects a dropped listChanged-only stream — the case that used to give up because the subscribed set was empty
• connects anyway when the connect-time listen fails, and retries

Plus a new keeps the stream open past the last subscription when a listChanged opt-in remains, the counterpart to the existing "closes the stream when the last subscription is removed".

Four existing lifecycle tests needed an empty filter to keep testing what they were written to test. The SDK server advertises listChanged for every list it registers, so that can't come from the server config — those tests now connect with every Inspector-side opt-in off (NO_LIST_CHANGED), which is the pre-#1920 world they assume.

No UI change

Nothing visible changed: the Subscriptions badge is gated on the filtered subscription count, so a listChanged-only stream shows no chrome (and active stays false for it). The only file touched under clients/web/src is a doc comment on the subscriptionStreamState prop. Hence no screenshots.

Gate

npm run ci — validate, verify:build-gate, smoke, and Storybook all pass; core/mcp/inspectorClient.ts covers 96.7 / 91.9 / 95.4 / 97.1 (stmts/branch/funcs/lines).

One caveat, reproduced on v2/main unchanged: the coverage step exits 1 locally on two pre-existing Connection closed unhandled rejections from inspectorClient.test.ts (disconnect() during the progress/timeout tests). Identical exit code and identical failing tests with this branch stashed, so it is not from this change. All 4887 tests pass.

[Copilot]

Pull request overview

Updates modern subscription streams to open for any non-empty notification filter, including list-change-only servers.

Changes:

• Opens and reconnects modern listen streams based on the effective filter.
• Keeps resource subscription UI state separate from list-change-only streams.
• Adds integration coverage for tools-only servers and stream lifecycle behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
core/mcp/types.ts	Clarifies stream-state semantics.
core/mcp/inspectorClient.ts	Implements filter-driven stream lifecycle.
clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts	Tests list-change-only streams and reconnection.
clients/web/src/components/groups/ResourceControls/ResourceControls.tsx	Clarifies UI prop documentation.

Suppressed comments (1)

clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts:372

• This second double cast also needs an inline safety justification. The surrounding comment explains why the method is shadowed, but not why mirroring this private shape is type-safe.

        connected as unknown as {

---

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

[cliffhall]

Round 1 review response (eda8ec0)

Mirroring the inline replies here, since they go hidden once the fix is pushed.

1. openModernListenStreamOnConnect reconciled unconditionally — real, fixed. It now captures modernListenGeneration before the refresh and reconciles only when it advanced exactly once, the same ownership test subscribeToResource/unsubscribeFromResource make. The method comment says why this site is exposed: connect is dispatched before the stream opens, so a listener that subscribes to a resource can start and acknowledge a newer refresh while this one still awaits its listen() — reconciling anyway would arm a reconnect that tears down a healthy stream.

The failure test's stub now bumps the generation synchronously before rejecting (mirroring the real method, which claims its generation before it can fail), so it still reaches the reconcile. New companion test leaves a superseded connect-time failure to the refresh that owns the stream bumps twice and asserts no reconnect is armed.

2. Unjustified double casts in the test — fixed by removing the duplication rather than annotating it twice. The file already had a StreamInternals interface + internals() helper for this; it is hoisted to the outer scope, gained refreshModernSubscription, and both new sites go through it. One such cast in the file now (plus the pre-existing as unknown as McpSubscription for the poisoned-close stub), carrying the justification: InspectorClient declares every member in the shape, so it mirrors rather than reinterprets; the cast only reaches past private, which no single as can do; and these branches (a remote close, a failing listen()) are unreachable from any public API against a healthy server.

npm run validate clean; core/mcp/inspectorClient.ts at 96.75 / 91.95 / 95.38 / 97.14, 4888 tests passing.

[Copilot]

Pull request overview

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

[cliffhall]

Round 2 review response (d08a530)

Stream opened too late to close the notification gap — correct, and fixed. This was a real gap rather than a nicety: the managed list states start their initial refresh() from the connect event, so tools/list could be sent ahead of subscriptions/listen, and a list the server changed in that window would notify nobody — the UI stays stale with nothing to signal it.

The handler registration, the initial logging/setLevel, and the non-fatal stream open now all run before dispatchTypedEvent("connect"). No consumer can act on the connection until the notification channel is established (or its retry armed). Cost: one listen round-trip added to a modern connect, which is the right trade for closing a silent-staleness window.

New test has the stream up before the connect event fires asserts it from the consumer side (reads modernSubscription from inside a connect listener).

Full local gate green: validate, smoke, Storybook (466), and 4889 unit+integration tests; core/mcp/inspectorClient.ts at 96.75 / 91.95 / 95.38 / 97.14. The two Connection closed unhandled rejections that make the coverage step exit 1 remain the pre-existing ones from inspectorClient.test.ts, reproduced identically with this branch stashed.

[Copilot]

Pull request overview

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

Suppressed comments (3)

core/mcp/inspectorClient.ts:4997

• connect is now dispatched only after this method returns (line 2076), so the stated race through a connect listener cannot occur. Keep the generation guard, but describe an actual superseding path such as code reacting to the lifecycle events emitted before this call or another concurrent public call; otherwise this comment contradicts the ordering this PR intentionally establishes.

   * Gated on the same generation test as `subscribeToResource` — see the long
   * comment there. This call is *not* the only refresh that can be in flight:
   * the `connect` event has already been dispatched, so a listener subscribing
   * to a resource can start and acknowledge a newer refresh while this one is
   * still awaiting its `listen()`. Reconciling unconditionally would then arm a
   * reconnect that tears down a stream that is up and healthy.

core/mcp/inspectorClient.ts:5172

• The JSDoc above still defines these branches as “nothing subscribed” versus “URIs live” and says the timer bails when the set empties. This predicate now intentionally stays true with an empty URI set when a list-change opt-in remains, so document the empty/non-empty filter instead; the current description is false for the new listChanged-only path.

  private reconcileModernStreamStateAfterFailedRefresh(): void {
    if (!this.wantsModernStream()) {

clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts:438

• This scenario says the connect event fires before the stream open, but the production code now deliberately dispatches it afterward. Update the explanation to the lifecycle events that are actually emitted earlier; the current comment directly contradicts the preceding ordering test.

    it("leaves a superseded connect-time failure to the refresh that owns the stream", async () => {
      // `connect` is dispatched before the stream is opened, so a listener can
      // start and acknowledge a newer refresh while this one is still awaiting
      // its `listen()`. Reconciling anyway would arm a reconnect against a
      // healthy stream and tear it down — the same ownership test the
      // subscribe/unsubscribe paths make.

[cliffhall]

Round 3 review response (b2dfd40)

No new code findings. The three suppressed comments were all correct — the round-2 reorder invalidated comments written in round 1 — and are fixed. Comments only, no behavior change.

1. openModernListenStreamOnConnect's generation-guard rationale contradicted the new ordering. Right: connect is now dispatched after this call, so a list-state consumer can no longer be the superseding party. The guard still earns its place for paths that survive the reorder, and the comment now names them — statusChange has already fired, and any concurrent call on the instance qualifies: a subscribeToResource from a caller restoring subscriptions, or a disconnect(), whose resetSubscriptionStream bumps the generation too (reconciling there would arm a reconnect for a session that is already gone).

2. reconcileModernStreamStateAfterFailedRefresh's JSDoc still said "nothing subscribed" vs "URIs live". Those are the empty vs non-empty filter now, and the listChanged-only path is exactly where the two diverge. Rewritten, including the "timer bails on an emptied set" line further down.

3. Same stale rationale on the superseded-failure test. Updated to the paths that can actually supersede it, and to say what the double-bump stub stands in for.

npm run validate clean.

[Copilot]

Pull request overview

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

Suppressed comments (1)

core/mcp/inspectorClient.ts:2076

• The new awaited listen open creates a window where disconnect() (including one still awaiting client.close()) or a transport close can overtake this connect, but this line still emits connect unconditionally afterward. That re-triggers every managed list refresh against a session being torn down or already dead. Only announce the connection if it is still connected and no explicit disconnect owns teardown.

      this.dispatchTypedEvent("connect");

[cliffhall]

Round 4 review response (2a4e93c)

No new inline comments; the one suppressed comment was right and is fixed.

The connect dispatch was unconditional after a widened await window. Every await before the announcement is a window for a disconnect() — or a transport onclose/onerror — to overtake the connect, and moving the listen open ahead of the dispatch made that window a round-trip longer. Announcing anyway restarts every managed list refresh against a session being torn down or already dead.

The dispatch is now gated on status === "connected" && !this.disconnecting. The second half matters because an explicit teardown claims ownership (disconnecting = true) before it awaits client.close(), so the status still reads whatever it was until that block finishes — checking status alone would miss exactly the teardown that is in flight.

New test does not announce the connection when a disconnect overtakes the stream open holds the stream open, disconnects inside the window, then releases, and asserts no connect event fired. (It gates on the stub actually being reached — the stub only runs after the handshake, so disconnecting before that would hang the connect rather than race it.)

npm run validate clean; 4890 unit+integration tests pass; core/mcp/inspectorClient.ts at 96.75 / 91.99 / 95.38 / 97.14.

[Copilot]

Pull request overview

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