Skip to content

Conversation

ezynda3
Copy link
Contributor

@ezynda3 ezynda3 commented Sep 27, 2025

Description

Fixes #530

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • MCP spec compatibility implementation
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring (no functional changes)
  • Performance improvement
  • Tests only (no functional changes)
  • Other (please describe):

Checklist

  • My code follows the code style of this project
  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the documentation accordingly

MCP Spec Compliance

  • This PR implements a feature defined in the MCP specification
  • Link to relevant spec section: Link text
  • Implementation follows the specification exactly

Additional Information

Summary by CodeRabbit

  • New Features

    • Added a public helper to reliably extract text from varied content formats; updated example to use it.
    • Reuse persistent HTTP sessions across requests to preserve state and improve stability.
  • Bug Fixes

    • Normalize and robustly parse message and sampling response content before processing.
    • Streaming transport now relies on the caller-provided context for retries, avoiding per-iteration cancellations.
  • Tests

    • Added end-to-end HTTP sampling tests with deterministic responses and a basic echo scenario.
  • Documentation

    • Added Sampling Support and human-in-the-loop guidance for clients and servers.

andig and others added 27 commits July 27, 2025 21:29
Implements sampling capability for HTTP transport, resolving issue #419.
Enables servers to send sampling requests to HTTP clients via SSE and
receive LLM-generated responses.

## Key Changes

### Core Implementation
- Add `BidirectionalInterface` support to `StreamableHTTP`
- Implement `SetRequestHandler` for server-to-client requests
- Enhance SSE parsing to handle requests alongside responses/notifications
- Add `handleIncomingRequest` and `sendResponseToServer` methods

### HTTP-Specific Features
- Leverage existing MCP headers (`Mcp-Session-Id`, `Mcp-Protocol-Version`)
- Bidirectional communication via HTTP POST for responses
- Proper JSON-RPC request/response handling over HTTP

### Error Handling
- Add specific JSON-RPC error codes for different failure scenarios:
  - `-32601` (Method not found) when no handler configured
  - `-32603` (Internal error) for sampling failures
  - `-32800` (Request cancelled/timeout) for context errors
- Enhanced error messages with sampling-specific context

### Testing & Examples
- Comprehensive test suite in `streamable_http_sampling_test.go`
- Complete working example in `examples/sampling_http_client/`
- Tests cover success flows, error scenarios, and interface compliance

## Technical Details

The implementation maintains full backward compatibility while adding
bidirectional communication support. Server requests are processed
asynchronously to avoid blocking the SSE stream reader.

HTTP transport now supports the complete sampling flow that was
previously only available in stdio and inprocess transports.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
This completes the server-side implementation of sampling support for
HTTP transport, addressing the remaining requirements from issue #419.

Changes:
- Enhanced streamableHttpSession to implement SessionWithSampling interface
- Added bidirectional SSE communication for server-to-client requests
- Implemented session registry for proper response correlation
- Added comprehensive error handling with JSON-RPC error codes
- Created extensive test suite covering all scenarios
- Added working example server with sampling tools

Key Features:
- Server can send sampling requests to HTTP clients via SSE
- Clients respond via HTTP POST with proper session correlation
- Queue overflow protection and timeout handling
- Compatible with existing HTTP transport architecture

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Replace flaky time.Sleep calls with proper synchronization using channels
and sync.WaitGroup to make tests deterministic and avoid race conditions.

Also improves error handling robustness in test servers with proper JSON
decoding error checks.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Make request vs response detection more robust by checking for presence
  of "method" field instead of relying on nil Result/Error fields
- Add nil pointer check in sendResponseToServer function to prevent panics

These changes improve reliability against malformed messages and edge cases.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
The comment incorrectly stated that responses are broadcast to all sessions,
but the implementation actually delivers responses to the specific session
identified by sessionID using the activeSessions registry.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Previously, EnableSampling() was a no-op that didn't actually enable the
sampling capability in the server's declared capabilities.

Changes:
- Add Sampling field to mcp.ServerCapabilities struct
- Add sampling field to internal serverCapabilities struct
- Update EnableSampling() to set the sampling capability flag
- Update handleInitialize() to include sampling in capability response
- Add test to verify sampling capability is properly declared

Now when EnableSampling() is called, the server will properly declare
sampling capability during initialization, allowing clients to know
that the server supports sending sampling requests.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Replace unsafe type assertion result.Content.(mcp.TextContent).Text
with safe type checking to handle cases where Content might not
be a TextContent struct.

Now gracefully handles different content types without panicking.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
The SamplingInterface test was missing the EnableSampling() call,
which is necessary to activate sampling features for proper testing.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Replace single error test with comprehensive table-driven tests
- Add test cases for invalid request IDs and malformed results
- Replace t.Fatalf with t.Errorf to follow project conventions
- Use proper session ID format for valid test scenarios

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Remove recursive call in RequestSampling that could cause stack overflow
- Remove problematic response re-queuing to global channel
- Update deliverSamplingResponse to route responses directly to
  dedicated request channels via samplingRequests map lookup
- This prevents ordering issues and ensures responses reach
  the correct waiting request

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Modified deliverSamplingResponse to return error instead of just logging
- Added proper error handling for disconnected sessions
- Improved error messages for debugging
- Updated test expectations to match new error behavior

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Add signal handling for SIGINT and SIGTERM
- Move defer statement after error checking
- Improve shutdown error handling

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Add timeout context for SSE response processing (30s default)
- Add timeout for individual connection attempts in listenForever (10s)
- Use context-aware sleep in retry logic
- Ensure async goroutines properly respect context cancellation

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Make error message more descriptive and actionable
- Provide clearer debugging information about why the channel is blocked

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Rename 'baseMessage' to 'jsonMessage' for more neutral naming
- Improves code readability and follows consistent naming conventions

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Add test verifying that concurrent sampling requests are handled correctly
when the second request completes faster than the first. The test ensures:
- Responses are correctly associated with their request IDs
- Server processes requests concurrently without blocking
- Completion order follows actual processing time, not submission order

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Create new context with 30-second timeout for request handling
to prevent long-running handlers from blocking indefinitely.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Replace all occurrences of interface{} with the modern Go any type alias
for improved readability and consistency with current Go best practices.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Create timeout context from parent context instead of context.Background()
to ensure request handlers respect parent context cancellation.

Addresses review comment about context handling in async goroutine.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
The samplingResponseChan field was declared but never used in the
streamableHttpSession struct. Remove it and update tests accordingly.

Addresses review comment about unused fields in session struct.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Add signal handling for SIGINT and SIGTERM to allow graceful shutdown
of the sampling HTTP client example. This prevents indefinite blocking
and provides better production-ready behavior.

Addresses review comment about adding graceful shutdown handling.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Removes unused sync.RWMutex field that was flagged by golangci-lint.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Contributor

coderabbitai bot commented Sep 27, 2025

Walkthrough

Adds Content normalization for sampling requests/responses, reworks streamable HTTP listen context and session reuse, implements E2E HTTP sampling tests, introduces mcp.GetTextFromContent, updates an example to use it, and adds/expands sampling documentation.

Changes

Cohort / File(s) Summary
Client: sampling Content normalization
client/client.go
After unmarshalling Params, normalize each message Content that is map[string]any by calling mcp.ParseContent and replace it; return an error with index on parse failure.
Client transport: streamable HTTP listen loop
client/transport/...streamable_http.go
listenForever now reuses the provided parent ctx for repeated GET/SSE connections instead of creating/canceling a per-iteration child context; SSE/error handling and retry behavior preserved.
Server: streamable HTTP sessions & sampling flow
server/streamable_http.go
Implement atomic get-or-create session reuse via activeSessions (LoadOrStore); conditionally register/unregister sessions; store sampling result for later unmarshalling.
Server: stdio sampling response parsing
server/stdio.go
After unmarshalling sampling result, detect Result.Content as map[string]any and call mcp.ParseContent to convert to proper Content type before returning.
Client session sampling parse
server/streamable_http.go (method: RequestSampling)
After unmarshalling sampling response, convert Content from map[string]any into proper Content via mcp.ParseContent.
E2E tests: HTTP sampling
e2e/sampling_http_test.go
Add full E2E tests: TestSamplingHTTPE2E, TestSamplingHTTPBasic, TestMain; add TestSamplingHandler, thread-safe response storage, port allocator, and orchestrated server/client sampling scenarios.
MCP utility: text extraction
mcp/utils.go
Add GetTextFromContent(content any) string to extract text from TextContent, map-based content, plain strings, or fallback formatting.
Examples: use new utility
examples/sampling_server/main.go
Replace local helper with mcp.GetTextFromContent(result.Content) and remove the now-unused getTextFromContent helper.
Docs: HTTP transport & sampling guidance
www/docs/pages/transports/http.mdx, www/docs/pages/clients/advanced-sampling.mdx, www/docs/pages/servers/advanced-sampling.mdx
Add Sampling Support section and human-in-the-loop / user-consent guidance, checklist, workflow examples, lifecycle notes, and limitations (one duplicated insertion noted).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

area: mcp spec, type: enhancement, status: needs submitter response

Suggested reviewers

  • pottekkat
  • rwjblue-glean
  • dugenkui03

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description contains only the template structure and a “Fixes #530” placeholder without any summary of the actual changes made, leaving the Description section effectively empty. The Type of Change section marks only two options without explaining the bug or MCP spec considerations. The Checklist and MCP Spec Compliance sections are entirely unchecked and the spec link remains a placeholder. No Additional Information is provided. This means the description does not adhere to the required template. Please update the pull request description to include a concise narrative of the HTTP sampling improvements and bug fixes, mark all applicable Type of Change options, complete the Checklist to confirm code style adherence, tests, and documentation updates, provide a valid link and details for MCP Spec Compliance, and add any relevant implementation notes in the Additional Information section.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title “HTTP Sampling Improvements” is concise and directly reflects the core focus of the changeset, which enhances HTTP-based sampling workflows across client, server, and documentation components. It cleanly summarizes the main intent without extraneous detail or vague wording and will be easily understood by teammates reviewing the commit history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch http-sampling-improvements

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

  • Public repositories are always opted into early access features.
  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
server/streamable_http.go (1)

639-655: Populate sampling response payload before delivery

When a client POSTs a sampling response, we enqueue a samplingResponseItem but never copy responseMessage.Result into it. Downstream RequestSampling then tries to unmarshal an empty slice, which matches the CI failure (failed to unmarshal sampling response: unexpected end of JSON input). Please persist the raw payload so the waiting goroutine can decode it.

-	} else if responseMessage.Result != nil {
-		// Parse result
+	} else if responseMessage.Result != nil {
+		// Preserve the raw result so RequestSampling can decode it later.
+		response.result = append(json.RawMessage(nil), responseMessage.Result...)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1737192 and 3356446.

📒 Files selected for processing (4)
  • client/client.go (1 hunks)
  • client/transport/streamable_http.go (1 hunks)
  • e2e/sampling_http_test.go (1 hunks)
  • server/streamable_http.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/client.go (1)
mcp/types.go (3)
  • Content (952-954)
  • TextContent (958-965)
  • TextContent (967-967)
e2e/sampling_http_test.go (8)
mcp/types.go (10)
  • CreateMessageRequest (895-898)
  • CreateMessageResult (915-922)
  • Content (952-954)
  • SamplingMessage (925-928)
  • CreateMessageParams (900-909)
  • InitializeRequest (433-437)
  • Params (180-180)
  • InitializeParams (439-445)
  • Implementation (521-524)
  • ClientCapabilities (474-486)
mcp/prompts.go (3)
  • Role (81-81)
  • RoleAssistant (85-85)
  • RoleUser (84-84)
mcp/tools.go (6)
  • Tool (557-574)
  • Description (870-874)
  • ToolInputSchema (627-627)
  • CallToolRequest (54-58)
  • CallToolResult (40-51)
  • CallToolParams (60-64)
server/server.go (1)
  • ServerFromContext (80-85)
server/session.go (1)
  • ClientSessionFromContext (82-87)
server/streamable_http.go (1)
  • NewStreamableHTTPServer (152-167)
client/transport/streamable_http.go (2)
  • NewStreamableHTTP (131-161)
  • WithContinuousListening (32-36)
client/client.go (2)
  • NewClient (70-80)
  • WithSamplingHandler (41-45)
🪛 ast-grep (0.39.5)
e2e/sampling_http_test.go

[warning] 83-83: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control

(avoid-bind-to-all-interfaces-go)

🪛 GitHub Actions: go
e2e/sampling_http_test.go

[error] 189-189: E2E Test: server sampling request failed due to failed to unmarshal sampling response: unexpected end of JSON input


[error] 306-306: Question tool returned error: failed to unmarshal sampling response: unexpected end of JSON input

@ezynda3 ezynda3 changed the title Merge branch 'main' into http-sampling-improvements and resolve conflicts HTTP Sampling Improvements Sep 29, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a7d4463 and 43c8148.

📒 Files selected for processing (8)
  • client/client.go (1 hunks)
  • client/transport/streamable_http.go (1 hunks)
  • mcp/utils.go (1 hunks)
  • server/stdio.go (1 hunks)
  • server/streamable_http.go (5 hunks)
  • www/docs/pages/clients/advanced-sampling.mdx (1 hunks)
  • www/docs/pages/servers/advanced-sampling.mdx (1 hunks)
  • www/docs/pages/transports/http.mdx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • www/docs/pages/clients/advanced-sampling.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
  • client/client.go
🧰 Additional context used
🧬 Code graph analysis (3)
server/stdio.go (2)
mcp/types.go (1)
  • Content (952-954)
mcp/utils.go (1)
  • ParseContent (569-633)
mcp/utils.go (1)
mcp/types.go (2)
  • TextContent (958-965)
  • TextContent (967-967)
server/streamable_http.go (2)
mcp/types.go (1)
  • Content (952-954)
mcp/utils.go (1)
  • ParseContent (569-633)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: test
  • GitHub Check: coverage
🔇 Additional comments (9)
client/transport/streamable_http.go (1)

597-604: LGTM! Context handling improved for persistent SSE connections.

The change correctly removes per-iteration context timeouts in favor of reusing the parent context for continuous listening. This is the right approach for long-lived SSE connections where:

  • Persistent connections should remain open indefinitely
  • Network-level timeouts and keep-alives handle connection health
  • Parent context cancellation provides clean shutdown

The extensive comments clearly explain the rationale, making the design intent explicit for future maintainers.

www/docs/pages/servers/advanced-sampling.mdx (1)

9-26: LGTM! Important security guidance for sampling.

The addition of the "User Consent Required" section properly emphasizes the MCP spec requirement for human-in-the-loop approval. The documentation:

  • Clearly sets expectations about user interaction (review, modification, rejection)
  • Provides actionable design recommendations (clear descriptions, graceful error handling, timeouts)
  • Aligns with the broader security theme in the HTTP transport documentation

This guidance will help developers implement sampling responsibly.

server/stdio.go (1)

609-620: LGTM! Content normalization properly implemented.

The addition of content parsing for sampling responses correctly handles the case where Result.Content arrives as map[string]any (from JSON unmarshalling) rather than a proper Content type. The implementation:

  • Uses defensive type checking with proper fallback behavior
  • Calls mcp.ParseContent to convert to the correct Content type (TextContent, ImageContent, etc.)
  • Handles parse errors gracefully by setting samplingResp.err
  • Preserves existing behavior when content is already properly typed

This aligns with the broader content normalization theme across the PR and ensures sampling responses are properly structured before being passed to handlers.

mcp/utils.go (1)

944-971: LGTM! Useful utility for text extraction.

The new GetTextFromContent function provides a convenient way to extract text from various content representations. The implementation:

  • Handles multiple content forms (TextContent struct, map[string]any from JSON, plain string)
  • Uses clear type switching with appropriate checks (e.g., verifying type="text" in map case)
  • Documents the lossy fallback behavior for non-text content
  • Appropriately directs users to ParseContent() for strict validation

This is a practical helper for scenarios like logging, display, and examples where you need text and can tolerate fallback behavior. The clear documentation about limitations prevents misuse.

www/docs/pages/transports/http.mdx (1)

692-845: LGTM! Comprehensive sampling documentation.

The new "Sampling Support" section provides thorough documentation for StreamableHTTP sampling capabilities. The content:

  • Properly emphasizes security requirements with a clear warning about human-in-the-loop approval
  • Clearly states technical prerequisites (WithContinuousListening() requirement)
  • Provides practical server-side implementation examples
  • Explains the persistent connection architecture clearly
  • Honestly documents limitations (need for continuous listening, reconnection concerns)
  • Includes a detailed approval flow example showing proper user consent patterns

This gives developers both the security context and practical guidance needed to implement sampling responsibly. The example approval flow is particularly valuable as a reference implementation.

server/streamable_http.go (4)

238-251: LGTM! Clean session reuse implementation.

The function correctly retrieves persistent sessions from activeSessions or creates ephemeral ones as needed. The type assertion is properly guarded, and the documentation clearly explains the distinction between persistent and ephemeral sessions.


327-328: LGTM! Proper integration of session reuse.

The change correctly integrates getOrCreateSession to reuse persistent sessions for POST requests, enabling bidirectional communication support. The updated comment accurately reflects this behavior.


657-658: LGTM! Deferred result parsing for proper type handling.

Storing the result as json.RawMessage is correct here, as it defers unmarshaling until the RequestSampling method where proper content type conversion occurs.


933-941: LGTM! Essential content normalization for HTTP transport.

The content parsing logic correctly handles the fact that JSON unmarshaling produces map[string]any for interface types. Using mcp.ParseContent converts this to the proper concrete Content type (TextContent, ImageContent, etc.), which is essential for type safety and proper downstream usage.

Copy link

Merging this branch changes the coverage (3 decrease, 1 increase)

Impacted Packages Coverage Δ 🤖
github.com/mark3labs/mcp-go/client 67.51% (+0.35%) 👍
github.com/mark3labs/mcp-go/client/transport 67.02% (-0.06%) 👎
github.com/mark3labs/mcp-go/e2e 0.00% (ø)
github.com/mark3labs/mcp-go/examples/sampling_server 0.00% (ø)
github.com/mark3labs/mcp-go/mcp 79.90% (-0.77%) 👎
github.com/mark3labs/mcp-go/server 70.57% (-0.45%) 👎

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/mark3labs/mcp-go/client/client.go 65.78% (+0.48%) 225 (+6) 148 (+5) 77 (+1) 👍
github.com/mark3labs/mcp-go/client/transport/streamable_http.go 72.94% (-0.18%) 303 (-2) 221 (-2) 82 👎
github.com/mark3labs/mcp-go/examples/sampling_server/main.go 0.00% (ø) 0 0 0
github.com/mark3labs/mcp-go/mcp/utils.go 85.71% (-2.73%) 259 (+8) 222 37 (+8) 👎
github.com/mark3labs/mcp-go/server/stdio.go 40.57% (-1.02%) 244 (+6) 99 145 (+6) 👎
github.com/mark3labs/mcp-go/server/streamable_http.go 57.96% (-0.46%) 383 (+15) 222 (+7) 161 (+8) 👎

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

  • github.com/mark3labs/mcp-go/e2e/sampling_http_test.go

@ezynda3 ezynda3 merged commit 9d1f447 into main Sep 30, 2025
5 checks passed
elewis787 pushed a commit to elewis787/mcp-go that referenced this pull request Oct 1, 2025
* feat: implement sampling support for Streamable HTTP transport

Implements sampling capability for HTTP transport, resolving issue mark3labs#419.
Enables servers to send sampling requests to HTTP clients via SSE and
receive LLM-generated responses.

## Key Changes

### Core Implementation
- Add `BidirectionalInterface` support to `StreamableHTTP`
- Implement `SetRequestHandler` for server-to-client requests
- Enhance SSE parsing to handle requests alongside responses/notifications
- Add `handleIncomingRequest` and `sendResponseToServer` methods

### HTTP-Specific Features
- Leverage existing MCP headers (`Mcp-Session-Id`, `Mcp-Protocol-Version`)
- Bidirectional communication via HTTP POST for responses
- Proper JSON-RPC request/response handling over HTTP

### Error Handling
- Add specific JSON-RPC error codes for different failure scenarios:
  - `-32601` (Method not found) when no handler configured
  - `-32603` (Internal error) for sampling failures
  - `-32800` (Request cancelled/timeout) for context errors
- Enhanced error messages with sampling-specific context

### Testing & Examples
- Comprehensive test suite in `streamable_http_sampling_test.go`
- Complete working example in `examples/sampling_http_client/`
- Tests cover success flows, error scenarios, and interface compliance

## Technical Details

The implementation maintains full backward compatibility while adding
bidirectional communication support. Server requests are processed
asynchronously to avoid blocking the SSE stream reader.

HTTP transport now supports the complete sampling flow that was
previously only available in stdio and inprocess transports.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* feat: implement server-side sampling support for HTTP transport

This completes the server-side implementation of sampling support for
HTTP transport, addressing the remaining requirements from issue mark3labs#419.

Changes:
- Enhanced streamableHttpSession to implement SessionWithSampling interface
- Added bidirectional SSE communication for server-to-client requests
- Implemented session registry for proper response correlation
- Added comprehensive error handling with JSON-RPC error codes
- Created extensive test suite covering all scenarios
- Added working example server with sampling tools

Key Features:
- Server can send sampling requests to HTTP clients via SSE
- Clients respond via HTTP POST with proper session correlation
- Queue overflow protection and timeout handling
- Compatible with existing HTTP transport architecture

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: replace time.Sleep with synchronization primitives in tests

Replace flaky time.Sleep calls with proper synchronization using channels
and sync.WaitGroup to make tests deterministic and avoid race conditions.

Also improves error handling robustness in test servers with proper JSON
decoding error checks.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: improve request detection logic and add nil pointer checks

- Make request vs response detection more robust by checking for presence
  of "method" field instead of relying on nil Result/Error fields
- Add nil pointer check in sendResponseToServer function to prevent panics

These changes improve reliability against malformed messages and edge cases.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: correct misleading comment about response delivery

The comment incorrectly stated that responses are broadcast to all sessions,
but the implementation actually delivers responses to the specific session
identified by sessionID using the activeSessions registry.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: implement EnableSampling() to properly declare sampling capability

Previously, EnableSampling() was a no-op that didn't actually enable the
sampling capability in the server's declared capabilities.

Changes:
- Add Sampling field to mcp.ServerCapabilities struct
- Add sampling field to internal serverCapabilities struct
- Update EnableSampling() to set the sampling capability flag
- Update handleInitialize() to include sampling in capability response
- Add test to verify sampling capability is properly declared

Now when EnableSampling() is called, the server will properly declare
sampling capability during initialization, allowing clients to know
that the server supports sending sampling requests.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: prevent panic from unsafe type assertion in example server

Replace unsafe type assertion result.Content.(mcp.TextContent).Text
with safe type checking to handle cases where Content might not
be a TextContent struct.

Now gracefully handles different content types without panicking.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: add missing EnableSampling() call in interface test

The SamplingInterface test was missing the EnableSampling() call,
which is necessary to activate sampling features for proper testing.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: expand error test coverage and avoid t.Fatalf

- Replace single error test with comprehensive table-driven tests
- Add test cases for invalid request IDs and malformed results
- Replace t.Fatalf with t.Errorf to follow project conventions
- Use proper session ID format for valid test scenarios

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: eliminate recursive response handling and improve routing

- Remove recursive call in RequestSampling that could cause stack overflow
- Remove problematic response re-queuing to global channel
- Update deliverSamplingResponse to route responses directly to
  dedicated request channels via samplingRequests map lookup
- This prevents ordering issues and ensures responses reach
  the correct waiting request

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: improve sampling response delivery robustness

- Modified deliverSamplingResponse to return error instead of just logging
- Added proper error handling for disconnected sessions
- Improved error messages for debugging
- Updated test expectations to match new error behavior

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: add graceful shutdown handling to sampling client

- Add signal handling for SIGINT and SIGTERM
- Move defer statement after error checking
- Improve shutdown error handling

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: improve context handling in streamable HTTP transport

- Add timeout context for SSE response processing (30s default)
- Add timeout for individual connection attempts in listenForever (10s)
- Use context-aware sleep in retry logic
- Ensure async goroutines properly respect context cancellation

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: improve error message for notification channel queue full condition

- Make error message more descriptive and actionable
- Provide clearer debugging information about why the channel is blocked

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: rename struct variable for clarity in message parsing

- Rename 'baseMessage' to 'jsonMessage' for more neutral naming
- Improves code readability and follows consistent naming conventions

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* test: add concurrent sampling requests test with response association

Add test verifying that concurrent sampling requests are handled correctly
when the second request completes faster than the first. The test ensures:
- Responses are correctly associated with their request IDs
- Server processes requests concurrently without blocking
- Completion order follows actual processing time, not submission order

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: improve context handling in async goroutine

Create new context with 30-second timeout for request handling
to prevent long-running handlers from blocking indefinitely.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: replace interface{} with any throughout codebase

Replace all occurrences of interface{} with the modern Go any type alias
for improved readability and consistency with current Go best practices.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: improve context handling in async goroutine for StreamableHTTP

Create timeout context from parent context instead of context.Background()
to ensure request handlers respect parent context cancellation.

Addresses review comment about context handling in async goroutine.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: remove unused samplingResponseChan field from session struct

The samplingResponseChan field was declared but never used in the
streamableHttpSession struct. Remove it and update tests accordingly.

Addresses review comment about unused fields in session struct.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* feat: add graceful shutdown handling to sampling HTTP client example

Add signal handling for SIGINT and SIGTERM to allow graceful shutdown
of the sampling HTTP client example. This prevents indefinite blocking
and provides better production-ready behavior.

Addresses review comment about adding graceful shutdown handling.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: remove unused mu field from streamableHttpSession

Removes unused sync.RWMutex field that was flagged by golangci-lint.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Add e2e test

* wip

* wip

* fixes

* fixes

* fix race condition

---------

Co-authored-by: andig <cpuidle@gmx.de>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: andig <cpuidle@gmail.com>
@coderabbitai coderabbitai bot mentioned this pull request Oct 1, 2025
4 tasks
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.

bug: Streamable HTTP Sampling doesnt work
2 participants