Skip to content

Enrich server implementation#171

Open
kubinio123 wants to merge 21 commits into
masterfrom
152-enrich-server-implementation
Open

Enrich server implementation#171
kubinio123 wants to merge 21 commits into
masterfrom
152-enrich-server-implementation

Conversation

@kubinio123

@kubinio123 kubinio123 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

DONE:

  • misc(client): renaming of the client components to don't clash with server ones
  • misc(client): removal of cancelation option, perhaps a future followup but major SDK like java don't support it too
  • misc: proper usage of mdoc:compile-only in docs
  • server enrichment, namely
    • prompts and resources support
    • streaming server context with option to push logs and progress notification to the clients
    • fluent builder for server definition
    • distinction between http and stdio server transport and corresponding implementations with generic tests
    • ZIO streaming server integration
    • cover for several more conformance specs

@kubinio123 kubinio123 linked an issue Jun 16, 2026 that may be closed by this pull request
@kubinio123 kubinio123 changed the title [WIP] Enrich server implementation Enrich server implementation Jun 26, 2026
@kubinio123 kubinio123 marked this pull request as ready for review June 26, 2026 07:52
@kubinio123 kubinio123 requested a review from adamw June 26, 2026 07:52
@adamw

adamw commented Jun 26, 2026

Copy link
Copy Markdown
Member

Review

Solid, well-structured work — the new builder API and transport hierarchy are clean and the conformance/test coverage is broad. The bulk of the diff is mechanical (the *TransportClient*Transport/Server*Transport rename sweep + regenerated docs); the substance is the server enrichment (prompts, resources, completion, logging, subscriptions, streaming server context, HTTP+stdio + ZIO streaming transports), which is where I focused. A few findings, most-severe first:

1. originCheck defaults to localhostOnly + enabled — silently 403s every remote deployment

server/src/main/scala/chimp/server/McpServer.scala (McpServer case class)

McpServer now defaults originCheck = OriginCheck.localhostOnly (enabled), and ServerHttpTransport.serve rejects any request whose Host header resolves to anything outside {localhost, 127.0.0.1, [::1], ::1} with 403 Forbidden before the handler runs. This default is applied to the existing McpServer(...).endpoint(...) path used in every example/README.

Failure scenario: deploy the adder example so it's reachable at mcp.example.com:8080; a client POSTs with Host: mcp.example.com403 Forbidden, the tool is never invoked, with no obvious cause. Previously there was no origin check at all, so this is a surprising, undocumented behavior change. docs/server/transport.md doesn't mention OriginCheck or how to relax it (.withOriginCheck(OriginCheck.disabled)). Worth reconsidering the default (e.g. validate Origin only, per the MCP DNS-rebinding guidance) and/or documenting it prominently.

2. Resource-not-found returns InvalidParams (-32602) instead of ResourceNotFound (-32002)

server/src/main/scala/chimp/server/McpHandler.scala (handleResourcesRead / resourceReadResponse)

Unknown/failed resource reads are reported with JSONRPCErrorCodes.InvalidParams. The MCP spec defines -32002 "Resource not found" for this case, and JSONRPCErrorCodes currently has no such constant. This is likely why sep-2164-resource-not-found remains in conformance-baseline.yml as a known failure — so the gap may be acknowledged, but the error code is the thing to fix.

3. ZioServerHttpTransportforkDaemon outlives client disconnect (fiber + unbounded queue leak)

server-streaming/server-zio/src/main/scala/chimp/server/zio/ZioServerHttpTransport.scala (eventStream)

handle(sink) is run via .forkDaemon, decoupled from the returned ZStream's scope. If the SSE consumer terminates early (client disconnects mid-stream), the daemon keeps running a long streaming tool and offering notifications to a Queue.unbounded that nobody drains.

Failure scenario: a streamingServerLogic tool that reports progress over 30s; client disconnects at 1s → fiber + queue stay alive until the logic finishes naturally. Consider scoping the fiber to the stream (e.g. ZStream.scoped / fromQueueWithShutdown) so it's interrupted on consumer teardown.

Minor (not blocking)

  • ServerStreamingHttpTransport.serve discards McpResponse.statusCode, always returning 200 OK with an SSE body — even for parse errors and notifications (which the non-streaming path answers with 202). Harmless for the chimp client, but a slight protocol deviation.
  • OriginCheck.localhostHosts includes bare "::1", but hostName("::1") returns "" (the takeWhile(_ != ':') stops immediately), so that set entry is unreachable. Dead, not wrong.

🤖 Generated with Claude Code

@adamw

adamw commented Jun 26, 2026

Copy link
Copy Markdown
Member

There's suspiciously little changes in the README ;) Do I read the description of the PR correctly that this implements the server stdio transport?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enrich server implementation

2 participants