ZIO client streaming transport and generic tests - #148

PR #148 Review Summary — ZIO client streaming transport and generic tests

  Critical issues (fix before merge)

  1. Wrong-id dispatch in ZioStreamingHttpTransport.sendRequest — ZioStreamingHttpTransport.scala:72-78 completes the pending entry keyed by request.id instead of the response body's id. JSON-RPC permits interleaved responses; dispatch should mirror the SSE path
  (line 160) and key on the message's own id.
  2. O(n²) SSE body collection — ZioStreamingHttpTransport.scala:114 uses runFold("")(_ + _). Replace with StringBuilder or chunk-collect-then-concat.
  3. O(n²) SSE buffer growth — ZioStreamingHttpTransport.scala:144-152 uses List :+ line per character, never explicitly resetting. Use ListBuffer/Chunk.
  4. Unbounded ZIO stdin queue — ZioStreamingStdioTransport.scala:78 Queue.unbounded. Use Queue.bounded for backpressure.
  5. Stated PR goal undercut by sync side — sync HttpTransport and StdioTransport still keep field-level AtomicReference/ConcurrentHashMap/vars. PR claims the atomic-reference pattern was removed; only the ZIO path actually does so.
  6. SynchronousQueue + timed offer in SyncPendingRequests.scala:24,34 — silent message drop if waiter hasn't reached take within 1s/100ms. Use LinkedBlockingQueue size 1 or CompletableFuture.

  Important issues

  7. No reconnect on GET SSE stream — ZioStreamingHttpTransport.runGetStream (line 170-188) exits silently on disconnect; pending requests leak forever.
  8. ZioStreamingStdioTransport.close doesn't call pending.closeAll — awaiters can hang.
  9. No per-request timeout / pending map bound in ZioPendingRequests; lock-free Ref[Map] also has CAS contention vs sync ConcurrentHashMap.
  10. Architectural smell: StreamingHttpTransport/StreamingStdioTransport abstract classes are empty — pure scaffolding; reuse happens via static HttpTransport helpers instead. Delete them or pull real shared logic up.
  11. client-zio depends on full client module — drags sync stdio + Identity code into ZIO classpath. Consider extracting client-core.

  Documentation / readability

  12. Naming: McpClient (request/response-only) vs BidirectionalMcpClient is misleading — readers will assume McpClient is canonical. Same for Transport/BidirectionalTransport. Add scaladoc minimum.
  13. No scaladoc on any new public trait/factory; Transport.send's Option[JSONRPCMessage] return is non-obvious. CLAUDE.md mandates this.
  14. PendingRequests lives in chimp.client.internal but is consumed by client-zio — either rename internal or move to a public SPI package.
  15. Magic -32000 JSON-RPC error code repeated in 3+ places — extract a constant.
  16. Sync vs Zio PendingRequests.complete return semantics differ — document or align.

  Test gaps

  17. Streaming-specific scenarios not exercised — no test for long-lived response stream, reconnection, or transport-only server push beyond the resource-update notification.
  18. No error/cancellation integration tests — JSON-RPC error responses, mid-request disconnect, transport reconnect.
  19. ZioPendingRequests has no unit coverage.
  20. Redundant tests: McpClientTests:18-22 (two near-duplicate initialize assertions), :58-64 resource-reference call duplicates other callTool tests, BidirectionalMcpClientTests:42-64 triples up isError assertions that aren't the focus.
  21. Flaky polling: waitUntil uses Thread.sleep in F[Unit] (blocks ZIO fibers); also asserts isDefined after timeout instead of failing with a clear message.
  22. MCPEverythingContainer doesn't pin @modelcontextprotocol/server-everything version and relies on default port 3001; CI fragility.

  Refactoring opportunities

  23. Three duplicated dispatch routings (Response/Error → pending; else → handler) across StdioTransport, ZioStreamingStdioTransport, ZioStreamingHttpTransport. Hoist into PendingRequests or a MessageRouter[F].
  24. ZioStreamingHttpTransport.sendRequest/sendNonRequest share ~20 lines of skeleton; extract withResolvedOutcome helper.
  25. SSE parsing duplicated between sync HttpTransport.extractSingleSseData and ZioStreamingHttpTransport.parseSseEvents — share a tiny SseParsing object.

  Positives

  - The capability-by-type split (McpClient/BidirectionalMcpClient, Transport/BidirectionalTransport) is the right abstraction — encoding capability in the type, not runtime errors.
  - Effectful constructor cleanly eliminates the AtomicReference[Option[InitializeResult]] hack on the ZIO side; serverCapabilities/serverInfo are now plain vals.
  - Generic test traits (McpClientTests, BidirectionalMcpClientTests, *IntegrationSpec) are excellent — per-backend leaf specs are 10-20 lines of pure wiring.
  - HttpTransport.resolveResponse / HttpOutcome cleanly separates HTTP-status interpretation from monad-specific I/O.
  - CI tag scheme (-l Integration) correctly excludes integration tests from sbt test.

  Verdict

  Approve with the critical fixes (#1-#6) addressed before merge. The architecture is sound; the bugs are concentrated in the new streaming HTTP transport.

  Remaining issues from original review

  #10 — Empty abstract classes StreamingHttpTransport/StreamingStdioTransport: Still just extends BidirectionalTransport[F] with constructor params. Reasonable as extension points for future effect-library backends (cats-effect, etc.). Low priority.

  #11 — client-zio depends on full client module: Still dependsOn(client). Pragmatic while there's only one streaming backend. Low priority.

  #13 — No scaladoc on public traits: Transport.send returning Option[JSONRPCMessage] (None for notifications, Some for request-response) remains undocumented. McpClient, BidirectionalMcpClient, and Transport/BidirectionalTransport have no doc comments. Per your
  CLAUDE.md rules these warrant at least a short comment since the semantics aren't obvious from the signatures alone.

  #19 — No unit tests for ZioPendingRequests: Still covered only via integration tests.

  #23 — Duplicated routeMessage/dispatch: ZioStreamingHttpTransport.routeMessage and ZioStreamingStdioTransport.dispatch are identical 3-line methods. Minor.

  New issues

  N1 — waitUntil silently succeeds on exhaustion. ToFuture.waitUntil returns monad.unit(()) when attempts <= 0, making the test rely on the subsequent assertion to catch it. If a test's yield block doesn't re-check the condition, the test passes silently. This works
  today because all callers do re-assert, but it's a footgun. Consider failing when attempts are exhausted.

  N2 — send completing cancelled request on ensuring. In ZioStreamingHttpTransport.send, the request path does .ensuring(pending.complete(request.id, cancelled(request.id)).orDie). If the request succeeds normally (response already routed via routeMessage), ensuring
  still calls complete with a "cancelled" error — but complete is a no-op if the id was already removed from the map, so this is safe. Just worth noting the intent: it's a cleanup-only guard, not a bug.

  N3 — createBidirectional installs onIncoming before initialize, creating a race window. McpClientImpl.createBidirectional calls transport.onIncoming(handler) first, then initialize. If the server sends a notification between onIncoming and initialize completion, the
  handler would try to dispatch it while the client is still being constructed. In practice the server shouldn't send anything before receiving initialized, so this is theoretical. The non-bidirectional create doesn't install any incoming handler at all, which is
  correct.

  N4 — StdioTransport.close doesn't call pending.closeAll. The old drainPending() call in close() was removed. Now closeAll only happens in the readLoop finally block. If the process exits cleanly and readLoop terminates normally, pending requests get drained. But if
  close() is called while the reader thread is blocked on readLine() and the process hasn't terminated yet, there's a window where pending requests aren't notified. The reader thread eventually terminates after the process is killed (2s + 2s timeout), but callers
  blocked on await() hang during that window. Adding pending.closeAll back in close() before killing the process would be safer.