Pre-push review: Transit PR #245 — MCP maintenance toggle refresh

Needs fixes

The implementation correctly advertises tools.listChanged, frames the notification as SSE, preserves origin validation, and keeps live tools/list behavior. However, the new long-lived transport has two correctness/lifecycle defects that should be fixed before pushing.

Review scope

git diff origin/main...HEAD

11 files, 378 additions, 15 deletions; four commits.

Validation

✓ make test-quick

✓ make lint — 0 violations

✓ git diff --check

Specification

MCP 2025-03-26 Streamable HTTP, tools list-change capability, duplicate-display-ID Decision 9, and the T-2169 bugfix report were reviewed.

Must-fix findings

HighOne toggle is broadcast across all SSE streams

Location: Transit/Transit/MCP/MCPToolListChangeBroadcaster.swift:25-28 and Transit/TransitTests/MCPToolListChangeNotificationTests.swift:36-54

notifyToolsListChanged() yields the same notification to every registered continuation, and the regression explicitly expects two streams to receive the same event. MCP 2025-03-26 allows one client to hold multiple SSE streams simultaneously but states that the server must send each JSON-RPC message on only one connected stream and must not broadcast it across multiple streams.

Because Transit does not establish session IDs or otherwise associate GET streams with logical clients, it cannot distinguish “two clients with one stream each” from “one client with two streams.” The current fan-out therefore codifies a protocol violation for a valid client topology.

Required direction: introduce client/session correlation and route one notification per client to one selected stream, or choose and document another protocol-compliant transport design. Add coverage for one logical client opening multiple streams.

HighAn idle disconnected GET stream can remain registered until the next toggle

Location: Transit/Transit/MCP/MCPServer+ToolListNotifications.swift:25-29 and Transit/Transit/MCP/MCPToolListChangeBroadcaster.swift:16-21

The response body waits only on the settings-owned AsyncStream. If the TCP peer disconnects while no notification is being written, Hummingbird's connection task remains suspended in that loop; there is no write to surface the closed channel and no explicit broadcaster cancellation. The continuation—and its one-item buffer—therefore remains in the settings-scoped dictionary until a later toggle causes writer.write to fail and releases the stream.

Repeated connect/disconnect cycles between maintenance toggles can accumulate continuations and per-toggle fan-out. A live SSE request also makes graceful stop/restart rely on the configured five-second shutdown escalation rather than ending promptly.

Required direction: tie registration lifetime to channel/request cancellation and server shutdown, explicitly finish/remove streams, and add a real channel-level disconnect plus stop/restart regression. The current in-process writer test ends only after an event and cannot exercise idle peer closure.

Additional findings

Medium`Accept` negotiation uses substring matching

Location: Transit/Transit/MCP/MCPServer+ToolListNotifications.swift:12-15

.contains("text/event-stream") accepts values such as text/event-stream;q=0 and application/x-text/event-stream. Parse the media ranges and honor quality values, or at minimum compare comma-delimited media types exactly. Add negative route tests for these cases.

LowThe notification test duplicates the shared HTTP harness

Location: Transit/TransitTests/MCPToolListChangeNotificationTests.swift:99-120

The responder, request, EmbeddedChannel, and context setup overlap MCPHTTPTestHelpers.swift. Extending the shared helper for custom headers/streaming responses would reduce divergent lifecycle behavior in tests.

What was checked

Area	Result
Code reuse	One low-severity duplicated test harness; no duplicated production utility found.
Code quality	Typed capability and notification payloads are clear; transport ownership is under-specified.
Efficiency/lifecycle	One-event buffering is bounded per stream, but stream count and disconnected registrations are not bounded.
Spec/docs/tests	Docs explain intended behavior, but the multi-stream test contradicts MCP's multiple-connections rule and disconnect/shutdown behavior is untested.

Commits reviewed

742dc23 — T-2169: Add MCP tool-list refresh regression
d037e5f — T-2169: Notify MCP clients when tools change
462b28f — T-2169: Strengthen MCP notification coverage
4d1e8e9 — T-2169: Align notification regression report

Generated from the fetched origin/main...HEAD merge-base diff. No production code, commits, pushes, or merges were performed.