feat: implement MCP elicitation support (#413) #495
Conversation
WalkthroughAdds end-to-end elicitation support: new MCP types/constants, server and client capability flags and handler interfaces, transport and session plumbing (stdio, in-process), client request routing, tests, and an example demonstrating structured elicitation flows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
server/session.go (1)
51-56: Interface addition LGTM, but clarify blocking behaviour.
RequestElicitationis (implicitly) expected to block until the client responds.
Please document this expectation in the interface comment (or make it explicit in the method name) so implementers know whether they must spawn a goroutine / select onctx.Done()internally.-// RequestElicitation sends an elicitation request to the client and waits for response +// RequestElicitation sends an elicitation request to the client and MUST block +// until either a response is received or ctx is cancelled. +// Implementations should respect ctx cancellation to avoid server goroutine leaks.server/elicitation_test.go (1)
59-82: Use consistent server configuration pattern.There's an inconsistency in how elicitation capability is enabled. Line 61 directly sets
server.capabilities.elicitation = mcp.ToBoolPtr(true), while other tests use theWithElicitation()option (line 85). Consider using the option pattern consistently for better maintainability.func TestMCPServer_RequestElicitation_NoSession(t *testing.T) { - server := NewMCPServer("test", "1.0.0") - server.capabilities.elicitation = mcp.ToBoolPtr(true) + server := NewMCPServer("test", "1.0.0", WithElicitation())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
client/client.go(5 hunks)client/elicitation.go(1 hunks)client/elicitation_test.go(1 hunks)client/inprocess_elicitation_test.go(1 hunks)client/transport/inprocess.go(3 hunks)client/transport/sse_test.go(2 hunks)client/transport/streamable_http_test.go(2 hunks)examples/elicitation/main.go(1 hunks)mcp/tools.go(3 hunks)mcp/types.go(4 hunks)mcptest/mcptest.go(1 hunks)server/elicitation.go(1 hunks)server/elicitation_test.go(1 hunks)server/inprocess_session.go(3 hunks)server/server.go(3 hunks)server/session.go(1 hunks)server/stdio.go(6 hunks)server/streamable_http_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
mcptest/mcptest.go (5)
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: octo
PR: mark3labs/mcp-go#149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.263Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
server/server.go (3)
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
client/transport/sse_test.go (1)
Learnt from: leavez
PR: mark3labs/mcp-go#114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
server/streamable_http_test.go (5)
Learnt from: octo
PR: mark3labs/mcp-go#149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
server/elicitation.go (1)
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
mcp/tools.go (8)
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in mark3labs/mcp-go handles both InputSchema and RawInputSchema formats. When unmarshaling JSON, it first tries to parse into a structured ToolInputSchema format, and if that fails, it falls back to using the raw schema format, providing symmetry with the MarshalJSON method.
Learnt from: octo
PR: mark3labs/mcp-go#149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: davidleitw
PR: mark3labs/mcp-go#451
File: mcp/tools.go:1192-1217
Timestamp: 2025-06-26T09:38:18.629Z
Learning: In mcp-go project, the maintainer prefers keeping builder pattern APIs simple without excessive validation for edge cases. The WithOutput* functions are designed to assume correct usage rather than defensive programming, following the principle of API simplicity over comprehensive validation.
Learnt from: lariel-fernandes
PR: mark3labs/mcp-go#428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
server/elicitation_test.go (2)
Learnt from: octo
PR: mark3labs/mcp-go#149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
examples/elicitation/main.go (4)
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
client/inprocess_elicitation_test.go (2)
Learnt from: octo
PR: mark3labs/mcp-go#149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
mcp/types.go (4)
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
client/elicitation_test.go (1)
Learnt from: octo
PR: mark3labs/mcp-go#149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
client/client.go (1)
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
server/stdio.go (1)
Learnt from: xinwo
PR: mark3labs/mcp-go#35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
🧬 Code Graph Analysis (9)
server/server.go (1)
mcp/utils.go (1)
ToBoolPtr(817-819)
server/session.go (1)
mcp/types.go (2)
ElicitationRequest(805-808)ElicitationResult(819-826)
client/elicitation.go (1)
mcp/types.go (2)
ElicitationRequest(805-808)ElicitationResult(819-826)
server/elicitation.go (3)
server/server.go (1)
MCPServer(138-163)mcp/types.go (2)
ElicitationRequest(805-808)ElicitationResult(819-826)server/session.go (2)
ClientSessionFromContext(78-83)SessionWithElicitation(52-56)
mcp/tools.go (1)
mcp/types.go (2)
Meta(123-135)Content(917-919)
examples/elicitation/main.go (5)
server/server.go (4)
MCPServer(138-163)ToolHandlerFunc(41-41)NewMCPServer(300-326)WithElicitation(286-290)mcp/tools.go (7)
CallToolRequest(48-52)CallToolResult(38-45)NewTool(638-660)WithDescription(681-685)WithString(916-934)Required(748-752)Description(740-744)mcp/types.go (7)
ElicitationRequest(805-808)Params(167-167)ElicitationParams(811-816)ElicitationResponseTypeAccept(842-842)Content(917-919)ElicitationResponseTypeDecline(844-844)ElicitationResponseTypeCancel(846-846)mcp/utils.go (1)
NewTextContent(199-204)server/stdio.go (1)
NewStdioServer(271-280)
server/inprocess_session.go (4)
mcp/types.go (3)
ElicitationRequest(805-808)ElicitationResult(819-826)JSONRPCNotification(323-326)client/elicitation.go (1)
ElicitationHandler(11-19)server/session.go (4)
ClientSession(11-20)SessionWithLogging(23-29)SessionWithClientInfo(43-49)SessionWithElicitation(52-56)server/sampling.go (1)
SessionWithSampling(39-42)
client/client.go (4)
client/elicitation.go (1)
ElicitationHandler(11-19)server/inprocess_session.go (1)
ElicitationHandler(19-21)mcp/types.go (9)
JSONRPCRequest(315-320)JSONRPCResponse(329-333)MethodElicitationCreate(60-60)ElicitationParams(811-816)Params(167-167)ElicitationRequest(805-808)Request(158-161)JSONRPC_VERSION(113-113)Result(236-240)client/transport/interface.go (2)
JSONRPCRequest(50-55)JSONRPCResponse(57-66)
server/stdio.go (3)
mcp/types.go (7)
ElicitationResult(819-826)ElicitationRequest(805-808)Params(167-167)ElicitationParams(811-816)JSONRPC_VERSION(113-113)MethodElicitationCreate(60-60)Result(236-240)server/session.go (2)
ClientSession(11-20)SessionWithElicitation(52-56)server/sampling.go (1)
SessionWithSampling(39-42)
🔇 Additional comments (49)
mcptest/mcptest.go (1)
145-145: Cosmetic change approvedThe removal of the superfluous blank line is a harmless style cleanup and has no functional impact.
client/transport/sse_test.go (1)
408-459: Whitespace-only change – no action needed.
No functional impact observed.client/transport/streamable_http_test.go (1)
418-451: Whitespace-only change – no action needed.
The surrounding logic remains untouched.server/streamable_http_test.go (1)
846-897: Cosmetic re-indentation – looks fine.
Behaviour of the header-passthrough test is unchanged.mcp/tools.go (1)
470-488: Whitespace-only cleanup – no functional changes.Also applies to: 498-529
server/server.go (3)
178-178: LGTM! Elicitation capability field follows established pattern.The addition of the
elicitation *boolfield to theserverCapabilitiesstruct correctly follows the same pattern as the existingloggingcapability field, using a pointer to bool for optional capability configuration.
285-290: LGTM! Capability option function follows established pattern.The
WithElicitation()function correctly implements the elicitation capability option following the exact same pattern asWithLogging(), including proper use ofmcp.ToBoolPtr(true)to set the capability flag.
578-580: LGTM! Capability advertisement follows established pattern.The elicitation capability advertisement in
handleInitializecorrectly follows the same pattern as the logging capability, with proper nil and boolean checks before setting the capability to an empty struct.server/elicitation.go (1)
13-25: LGTM! Method implementation follows established session patterns.The
RequestElicitationmethod correctly implements the server-side elicitation request forwarding with proper session validation, interface type assertion, and error handling. The implementation follows the same pattern as other session-dependent server methods.client/elicitation.go (1)
11-19: LGTM! Well-designed interface with comprehensive documentation.The
ElicitationHandlerinterface is excellently designed with a clean single-method contract, proper type usage from the MCP protocol, and comprehensive documentation that clearly guides implementers on expected behavior including user interaction patterns.client/transport/inprocess.go (3)
16-16: LGTM! Field addition follows established pattern.The
elicitationHandlerfield correctly follows the same pattern as the existingsamplingHandlerfield, using the appropriateserver.ElicitationHandlerinterface type for in-process transport.
32-36: LGTM! Option function follows established pattern.The
WithElicitationHandleroption function correctly follows the same pattern asWithSamplingHandler, maintaining consistency in the transport configuration API.
59-60: LGTM! Session creation logic properly handles both handler types.The updated session creation logic correctly uses OR condition to create sessions when either handler is present and properly passes both handlers to
NewInProcessSessionWithHandlers, maintaining backward compatibility while enabling elicitation support.server/inprocess_session.go (5)
18-21: LGTM! Interface definition follows established patterns.The
ElicitationHandlerinterface correctly defines the server-side contract for elicitation handling with proper method signature, context support, and appropriate MCP protocol types.
30-30: LGTM! Field addition follows established pattern.The
elicitationHandlerfield correctly follows the same pattern as the existingsamplingHandlerfield, maintaining consistency in the struct design.
42-49: LGTM! Constructor follows established patterns.The
NewInProcessSessionWithHandlersconstructor correctly follows the same initialization pattern as the existing constructor while properly setting both handler fields, maintaining consistency and enabling dual handler support.
105-115: LGTM! Method implementation mirrors established patterns.The
RequestElicitationmethod correctly follows the exact same pattern asRequestSamplingwith proper read locking, nil checking, and handler delegation, ensuring thread safety and consistent error handling.
128-128: LGTM! Interface compliance assertion ensures correctness.The addition of
SessionWithElicitationto the interface compliance assertions provides important compile-time verification thatInProcessSessioncorrectly implements the elicitation interface, following the established pattern of other interface assertions.server/elicitation_test.go (4)
12-57: Well-structured mock implementations for testing.The mock session types correctly implement the expected interfaces and provide good test coverage infrastructure. The
mockBasicSessionproperly represents sessions without elicitation support, whilemockElicitationSessionextends it with configurable elicitation behavior.
84-112: Good test coverage for unsupported session scenario.This test correctly validates the error handling when a session doesn't support elicitation. The setup and assertions are appropriate.
114-172: Comprehensive test for successful elicitation flow.This test effectively validates the happy path scenario with proper response validation and type checking. The mock setup and assertions are thorough.
174-263: fakeSession type validatedThe table-driven tests reference
fakeSession, which is defined inserver/server_test.go, so there’s no missing type or compilation issue. Everything else inTestRequestElicitationis well-structured and provides comprehensive coverage. Approving these changes.examples/elicitation/main.go (2)
1-13: Clean package setup with appropriate imports.The import statements are well-organized and all dependencies are necessary for the example functionality.
99-99: Appropriate use of atomic counter for request tracking.The atomic counter effectively demonstrates thread-safe state management across tool requests.
client/client.go (4)
26-26: Consistent implementation following established patterns.The new
elicitationHandlerfield andWithElicitationHandleroption function correctly follow the same patterns as the existing sampling functionality. The documentation is clear and implementation is straightforward.Also applies to: 46-52
166-169: Proper capability declaration following existing pattern.The elicitation capability declaration correctly mirrors the sampling capability pattern, ensuring consistency in the client initialization flow.
443-443: Clean integration with existing request routing.The elicitation request routing follows the established pattern and integrates seamlessly with the existing request handling infrastructure.
Also applies to: 448-449
503-549: Well-implemented transport handler following established patterns.The
handleElicitationRequestTransportmethod correctly mirrors the structure and patterns of the existinghandleSamplingRequestTransportmethod. The parameter handling, error checking, and response construction all follow the established conventions.client/elicitation_test.go (4)
13-24: Well-designed mock for flexible testing scenarios.The
mockElicitationHandlerprovides good flexibility for testing different response scenarios and error conditions.
26-123: Comprehensive test coverage with good scenario validation.The table-driven test effectively covers all important scenarios including error conditions and different response types. The assertion logic is thorough and appropriate.
125-135: Simple and focused option test.The test correctly verifies that the
WithElicitationHandleroption properly sets the handler on the client.
192-225: Complete and appropriate mock transport implementation.The mock transport provides all necessary interface methods with configurable behavior, making it suitable for testing various scenarios.
client/inprocess_elicitation_test.go (3)
12-33: Effective mock handler for integration testing.The mock implementation provides appropriate tracking capabilities and simulates user acceptance, which is perfect for testing the end-to-end elicitation flow.
185-197: Useful helper function for in-process elicitation testing.The helper function correctly sets up both server and client sides of elicitation handling for in-process testing scenarios.
199-206: Simple and effective adapter pattern implementation.The wrapper correctly bridges the client and server elicitation handler interfaces, enabling seamless in-process testing.
mcp/types.go (8)
58-60: LGTM! Method constant follows established patterns.The new elicitation method constant is well-documented and follows the existing naming convention for MCP methods.
455-456: Client capability addition looks good.The elicitation capability follows the same pattern as other client capabilities like sampling.
485-486: Server capability addition is consistent.The elicitation capability matches the client-side declaration and follows established patterns.
803-808: Request type structure is well-designed.The
ElicitationRequestfollows the established pattern for MCP request types, properly embedding the baseRequesttype.
811-816: Parameters are appropriately typed.The use of
anyforRequestedSchemais correct for JSON Schema flexibility, consistent with other schema fields in the codebase.
819-826: Result type is well-documented.The
ElicitationResultstructure properly embeds the baseResulttype and clearly documents all possible response scenarios.
829-835: Response structure handles all cases correctly.The optional
Valuefield withomitemptytag is appropriate since it's only populated for accepted responses.
838-847: Enum values cover all response scenarios.The three response types (accept, decline, cancel) appropriately handle all user interaction outcomes.
server/stdio.go (6)
55-64: Session struct changes follow existing patterns.The
pendingElicitationsmap mirrors the design ofpendingRequestsfor sampling, maintaining consistency in the codebase.
73-77: Response type matches established patterns.The
elicitationResponsestruct correctly mirrors thesamplingResponsepattern with appropriate types.
185-246: RequestElicitation implementation is solid.The method correctly implements the request-response pattern with proper cleanup, error handling, and context support, mirroring the sampling implementation.
256-260: Interface implementation check added correctly.The compile-time assertion for
SessionWithElicitationensures the session properly implements the interface.
263-267: Session instance initialization is complete.The
pendingElicitationsmap is properly initialized alongside existing maps.
423-426: Response routing added correctly.The elicitation response check follows the established pattern and is properly integrated into the message processing flow.
semistrict
force-pushed
the
feat/elicitation
branch
from
e215c98 to
c0b1fd7
Compare
|
Any idea when this might be getting in? It would be great to have elicitation. |
|
hey when we are planning to merge this ? |
|
@dugenkui03 or @ezynda3 can you review when you have a chance? Thanks! |
|
This has a bunch of merge conflicts that need to be addressed |
|
it looks like @JBUinfo resolved the conflicts and this is now mergable. |
* Add ElicitationRequest, ElicitationResult, and related types to mcp/types.go * Implement server-side RequestElicitation method with session support * Add client-side ElicitationHandler interface and request handling * Implement elicitation in stdio and in-process transports * Add comprehensive tests following sampling patterns * Create elicitation example demonstrating usage patterns * Use 'Elicitation' prefix for type names to maintain clarity
semistrict
force-pushed
the
feat/elicitation
branch
from
c0b1fd7 to
b7a18c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/elicitation.go (2)
10-12: Clarify doc comment with explicit error contracts.Documenting the concrete errors helps users handle them correctly (especially if switched to sentinels).
-// RequestElicitation sends an elicitation request to the client. -// The client must have declared elicitation capability during initialization. -// The session must implement SessionWithElicitation to support this operation. +// RequestElicitation sends an elicitation request to the client. +// Preconditions: client declares elicitation capability during initialization. +// Returns: +// - ErrNoActiveSession if no active session is associated with ctx. +// - ErrElicitationNotSupported if the session doesn't implement SessionWithElicitation.
13-13: Optional: keep parameter passing consistent with adjacent APIs.If other request-style entrypoints in this package accept pointers (to avoid copying larger request structs and to align signatures), consider
request *mcp.ElicitationRequest. If peers use values, keep as-is for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
client/client.go(5 hunks)client/elicitation.go(1 hunks)client/elicitation_test.go(1 hunks)client/inprocess_elicitation_test.go(1 hunks)client/transport/inprocess.go(3 hunks)examples/elicitation/main.go(1 hunks)mcp/types.go(4 hunks)server/elicitation.go(1 hunks)server/elicitation_test.go(1 hunks)server/inprocess_session.go(4 hunks)server/server.go(3 hunks)server/session.go(1 hunks)server/stdio.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- server/session.go
- server/elicitation_test.go
- client/elicitation.go
- server/server.go
- mcp/types.go
- client/transport/inprocess.go
- client/inprocess_elicitation_test.go
- server/inprocess_session.go
- client/client.go
- client/elicitation_test.go
- examples/elicitation/main.go
- server/stdio.go
🧰 Additional context used
🧬 Code graph analysis (1)
server/elicitation.go (3)
server/server.go (1)
MCPServer(144-169)mcp/types.go (2)
ElicitationRequest(829-832)ElicitationResult(843-850)server/session.go (2)
ClientSessionFromContext(82-87)SessionWithElicitation(56-60)
🔇 Additional comments (1)
server/elicitation.go (1)
13-22: Control flow and delegation look good.Fetching the session from context, early-returning on absence, and delegating via SessionWithElicitation preserves transport-agnostic behavior and cleanly separates concerns.
server/elicitation.go
Outdated
| import ( | ||
| "context" | ||
| "fmt" | ||
|
|
||
| "github.com/mark3labs/mcp-go/mcp" | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use sentinel errors (errors.New) instead of raw strings; simplify imports.
Returning typed, package-level errors enables callers/tests to use errors.Is for reliable matching and avoids brittle string checks. Also, no formatting is needed—prefer errors.New over fmt.Errorf here.
If you adopt the sentinels, run this to find any string-based assertions that need updating:
Proposed diff:
import (
"context"
- "fmt"
+ "errors"
"github.com/mark3labs/mcp-go/mcp"
)
+// Sentinel errors to allow callers to detect conditions via errors.Is.
+var (
+ ErrNoActiveSession = errors.New("no active session")
+ ErrElicitationNotSupported = errors.New("session does not support elicitation")
+)
@@
session := ClientSessionFromContext(ctx)
if session == nil {
- return nil, fmt.Errorf("no active session")
+ return nil, ErrNoActiveSession
}
@@
- return nil, fmt.Errorf("session does not support elicitation")
+ return nil, ErrElicitationNotSupportedAlso applies to: 16-17, 24-24
🏁 Script executed:
#!/bin/bash
# Find places asserting on error strings that should switch to errors.Is
rg -n -C2 --type=go '"no active session"|"session does not support elicitation"'Length of output: 2426
Use sentinel errors and update tests accordingly
Defining package-level sentinel errors and replacing raw fmt.Errorf calls will enable callers and tests to use errors.Is for reliable matching. After making these changes, you’ll need to update all test assertions that compare error strings to use errors.Is(err, <SentinelError>) instead.
Bullet-traced refactor plan:
-
server/elicitation.go
• Removefmtimport, adderrors.
• Declare at top of file:var ( ErrNoActiveSession = errors.New("no active session") ErrElicitationNotSupported = errors.New("session does not support elicitation") )
• Replace:
- return nil, fmt.Errorf("no active session") + return nil, ErrNoActiveSession … - return nil, fmt.Errorf("session does not support elicitation") + return nil, ErrElicitationNotSupported
-
server/sampling.go
• Removefmtimport, adderrors.
• Declare near top:var ErrNoActiveSession = errors.New("no active session")
• Replace:
- return nil, fmt.Errorf("no active session") + return nil, ErrNoActiveSession
-
Tests in server/elicitation_test.go, server/sampling_test.go, server/server_test.go
• Replace string comparisons such as:expectedError := "no active session" if err.Error() != expectedError { … }
with
if !errors.Is(err, ErrNoActiveSession) { t.Errorf("expected ErrNoActiveSession, got %v", err) }
• And similarly for
ErrElicitationNotSupported.
To locate any remaining raw-string assertions, run:
rg -n -C2 --type=go '"no active session"|"session does not support elicitation"'This refactor is mandatory to keep error matching robust and maintainable.
🤖 Prompt for AI Agents
In server/elicitation.go around lines 3-8, replace the current use of raw
fmt.Errorf error strings by removing the fmt import and adding the errors
package, declare package-level sentinel errors (ErrNoActiveSession and
ErrElicitationNotSupported) at the top of the file, and replace all
fmt.Errorf(...) occurrences that produce those exact messages with the
corresponding sentinel error variables; do the same in server/sampling.go
(remove fmt, add errors, declare ErrNoActiveSession and replace raw error
strings with the sentinel); then update tests in server/elicitation_test.go,
server/sampling_test.go, and server/server_test.go to stop comparing error
strings and instead assert with errors.Is(err, ErrNoActiveSession) or
errors.Is(err, ErrElicitationNotSupported) as appropriate; finally run a search
(rg -n ...) to find any remaining raw-string comparisons and convert them to
errors.Is checks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
client/inprocess_elicitation_test.go (9)
23-33: Align mock response with requested schema and preferanyoverinterface{}The elicitation response returns keys (
response,accepted) that don’t match the schema you request later (confirm,details). It’s harmless given server-side schema validation isn’t implemented yet, but keeping them consistent prevents confusion and future-proof tests if validation is added. Also, prefermap[string]anyfor consistency with Go 1.18+.Apply this diff:
- return &mcp.ElicitationResult{ - Response: mcp.ElicitationResponse{ - Type: mcp.ElicitationResponseTypeAccept, - Value: map[string]interface{}{ - "response": "User provided data", - "accepted": true, - }, - }, - }, nil + return &mcp.ElicitationResult{ + Response: mcp.ElicitationResponse{ + Type: mcp.ElicitationResponseTypeAccept, + Value: map[string]any{ + "confirm": true, + "details": "User provided data", + }, + }, + }, nil
50-69: Usemap[string]anyconsistently for JSON-shaped mapsMinor style/nit: replace
map[string]interface{}withmap[string]anythroughout the schema literal.- RequestedSchema: map[string]interface{}{ + RequestedSchema: map[string]any{ "type": "object", - "properties": map[string]interface{}{ - "confirm": map[string]interface{}{ + "properties": map[string]any{ + "confirm": map[string]any{ "type": "boolean", "description": "Confirm the action", }, - "details": map[string]interface{}{ + "details": map[string]any{ "type": "string", "description": "Additional details", }, }, "required": []string{"confirm"}, },
85-104: Handle unknown response types and mark cancel as error (optional)Adding a default branch guards against unexpected values. Also consider surfacing cancellation as an error at the tool layer, which aligns with “proper error handling for cancelled elicitations” in the linked issue.
- var responseText string + var ( + responseText string + isError bool + ) switch result.Response.Type { case mcp.ElicitationResponseTypeAccept: responseText = "User accepted and provided data" case mcp.ElicitationResponseTypeDecline: responseText = "User declined to provide information" case mcp.ElicitationResponseTypeCancel: responseText = "User cancelled the request" + isError = true + default: + responseText = "Unknown elicitation response type" + isError = true } return &mcp.CallToolResult{ Content: []mcp.Content{ mcp.TextContent{ Type: "text", Text: responseText, }, }, - }, nil + IsError: isError, + }, nilIf your intended semantics are “decline is non-error, cancel is error,” this change reflects that. If you prefer both decline and cancel to be errors, set
isError = truein the decline branch as well.
3-10: Add timeouts to prevent hanging testsWrap Start/Initialize/CallTool calls with timeouts and import
time. This makes the test more robust against transport/server deadlocks.import ( "context" "testing" + "time"- // Start the client - if err := client.Start(context.Background()); err != nil { + // Start the client (with timeout) + startCtx, startCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer startCancel() + if err := client.Start(startCtx); err != nil { t.Fatalf("Failed to start client: %v", err) }- // Initialize the client - _, err = client.Initialize(context.Background(), mcp.InitializeRequest{ + // Initialize the client (with timeout) + initCtx, initCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer initCancel() + _, err = client.Initialize(initCtx, mcp.InitializeRequest{ Params: mcp.InitializeParams{ ProtocolVersion: mcp.LATEST_PROTOCOL_VERSION, ClientInfo: mcp.Implementation{ Name: "test-client", Version: "1.0.0", }, Capabilities: mcp.ClientCapabilities{ Elicitation: &struct{}{}, }, }, })- // Call the tool that triggers elicitation - result, err := client.CallTool(context.Background(), mcp.CallToolRequest{ + // Call the tool that triggers elicitation (with timeout) + callCtx, callCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer callCancel() + result, err := client.CallTool(callCtx, mcp.CallToolRequest{ Params: mcp.CallToolParams{ Name: "test_elicitation", Arguments: map[string]any{ "action": "test-action", }, }, })Also applies to: 116-120, 121-133, 138-147
152-155: Assert non-error result for the accept pathSince the mock returns “accept,” ensure the tool result isn’t flagged as an error.
// Verify the result if len(result.Content) == 0 { t.Fatal("Expected content in result") } + if result.IsError { + t.Fatalf("Unexpected error result: %+v", result) + }
171-174: Add a minimal schema assertion for stronger end-to-end coverageVerifying the schema shape reached the handler catches wiring mistakes and future regressions.
if mockHandler.LastRequest.Params.Message != "Need additional information for test-action" { t.Errorf("Unexpected elicitation message: %s", mockHandler.LastRequest.Params.Message) } + // Verify schema was passed through + if schema, ok := mockHandler.LastRequest.Params.RequestedSchema.(map[string]any); ok { + if _, hasProps := schema["properties"]; !hasProps { + t.Error("requestedSchema missing 'properties'") + } + } else { + t.Fatal("requestedSchema is not a JSON object") + }
184-186: Redundant assignment to client.elicitationHandler in in-process setup (confirm intent)Because the in-process transport is wired with
server.ElicitationHandlervia the wrapper, the explicitclient.elicitationHandler = handlermay be unnecessary for this test. If it isn’t required for capability negotiation elsewhere, consider removing it.client := NewClient(inProcessTransport) - client.elicitationHandler = handler
176-188: Optional: simplify helper signature by dropping the unused error returnThe helper can’t fail as written; simplifying the signature reduces noise.
Change the helper:
-func NewInProcessClientWithElicitationHandler(server *server.MCPServer, handler ElicitationHandler) (*Client, error) { +func NewInProcessClientWithElicitationHandler(server *server.MCPServer, handler ElicitationHandler) *Client { @@ - return client, nil + return client }And the call site:
- client, err := NewInProcessClientWithElicitationHandler(mcpServer, mockHandler) - if err != nil { - t.Fatalf("Failed to create client: %v", err) - } + client := NewInProcessClientWithElicitationHandler(mcpServer, mockHandler)Also applies to: 109-113
35-104: Broaden coverage: add decline and cancel scenariosThe test exercises only the accept path. Consider adding table-driven subtests to validate decline and cancel flows (and the
IsErrorsemantics you choose). I can draft a follow-up test if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
client/inprocess_elicitation_test.go(1 hunks)client/transport/sse.go(2 hunks)client/transport/streamable_http.go(4 hunks)client/transport/streamable_http_sampling_test.go(15 hunks)examples/elicitation/main.go(1 hunks)examples/sampling_client/main.go(1 hunks)examples/sampling_http_client/main.go(5 hunks)examples/sampling_http_server/main.go(1 hunks)server/sampling.go(1 hunks)server/sampling_test.go(2 hunks)server/stdio.go(6 hunks)server/streamable_http.go(5 hunks)server/streamable_http_sampling_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (10)
- server/sampling.go
- client/transport/streamable_http.go
- examples/sampling_client/main.go
- examples/sampling_http_server/main.go
- server/streamable_http_sampling_test.go
- examples/sampling_http_client/main.go
- client/transport/streamable_http_sampling_test.go
- server/sampling_test.go
- client/transport/sse.go
- server/streamable_http.go
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/elicitation/main.go
- server/stdio.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/inprocess_elicitation_test.go (7)
mcp/types.go (15)
ElicitationRequest(829-832)ElicitationResult(843-850)ElicitationResponse(853-859)ElicitationResponseTypeAccept(866-866)Params(181-181)ElicitationParams(835-840)Content(941-943)TextContent(947-954)TextContent(956-956)ElicitationResponseTypeDecline(868-868)ElicitationResponseTypeCancel(870-870)InitializeRequest(418-422)InitializeParams(424-430)Implementation(506-509)ClientCapabilities(459-471)server/server.go (3)
NewMCPServer(307-333)WithElicitation(293-297)MCPServer(144-169)mcp/tools.go (3)
CallToolRequest(54-58)CallToolResult(40-51)CallToolParams(60-64)client/elicitation.go (1)
ElicitationHandler(11-19)server/inprocess_session.go (1)
ElicitationHandler(19-21)client/client.go (3)
Client(17-29)WithElicitationHandler(50-54)NewClient(71-81)client/transport/inprocess.go (2)
NewInProcessTransportWithOptions(44-55)WithElicitationHandler(32-36)
🔇 Additional comments (1)
client/inprocess_elicitation_test.go (1)
39-45: Tool registration pattern matches current server API — looks goodYou’ve updated the tool registration to use
mcp.NewToolwith field options. This aligns with the current MCP server API patterns and resolves the earlier feedback about outdated registration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/inprocess_elicitation_test.go (1)
39-44: Tool registration now matches current server API.Using mcp.NewTool and option builders is consistent with the repo’s pattern. Thanks for addressing the earlier feedback.
🧹 Nitpick comments (15)
server/elicitation_test.go (6)
60-61: Use public option WithElicitation and consistent testify assertions
- Prefer the public option WithElicitation over mutating unexported fields.
- Use require/assert to fail fast and avoid brittle string equality checks.
- server := NewMCPServer("test", "1.0.0") - server.capabilities.elicitation = mcp.ToBoolPtr(true) + server := NewMCPServer("test", "1.0.0", WithElicitation())- if err == nil { - t.Error("expected error when no session available") - } - - expectedError := "no active session" - if err.Error() != expectedError { - t.Errorf("expected error %q, got %q", expectedError, err.Error()) - } + require.Error(t, err) + assert.Contains(t, err.Error(), "no active session")Also applies to: 74-81
104-111: Standardize error assertions with require/assertAlign with the table-driven style and avoid brittle string equality.
- if err == nil { - t.Error("expected error when session doesn't support elicitation") - } - - expectedError := "session does not support elicitation" - if err.Error() != expectedError { - t.Errorf("expected error %q, got %q", expectedError, err.Error()) - } + require.Error(t, err) + assert.Contains(t, err.Error(), "session does not support elicitation")
150-172: Tighten success-path assertionsUse require/assert consistently, and verify all expected fields (both projectName and framework). This fails fast and covers more behavior.
- if err != nil { - t.Errorf("unexpected error: %v", err) - } - - if result == nil { - t.Error("expected result, got nil") - return - } - - if result.Response.Type != mcp.ElicitationResponseTypeAccept { - t.Errorf("expected response type %q, got %q", mcp.ElicitationResponseTypeAccept, result.Response.Type) - } - - value, ok := result.Response.Value.(map[string]any) - if !ok { - t.Error("expected value to be a map") - return - } - - if value["projectName"] != "my-project" { - t.Errorf("expected projectName %q, got %q", "my-project", value["projectName"]) - } + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, mcp.ElicitationResponseTypeAccept, result.Response.Type) + value, ok := result.Response.Value.(map[string]any) + require.True(t, ok, "expected value to be a map[string]any") + assert.Equal(t, "my-project", value["projectName"]) + assert.Equal(t, "react", value["framework"])
258-260: Assert response value type for accept cases in table-driven testStrengthen the check to ensure the accepted response has the expected structure.
- if tt.expectedType == mcp.ElicitationResponseTypeAccept { - assert.NotNil(t, result.Response.Value) - } + if tt.expectedType == mcp.ElicitationResponseTypeAccept { + require.NotNil(t, result.Response.Value) + require.IsType(t, map[string]any{}, result.Response.Value) + }
59-63: Optional: Run tests in parallel to speed up CISubtests are isolated; parallelizing them can reduce overall test time.
func TestMCPServer_RequestElicitation_NoSession(t *testing.T) { - server := NewMCPServer("test", "1.0.0", WithElicitation()) + t.Parallel() + server := NewMCPServer("test", "1.0.0", WithElicitation())func TestMCPServer_RequestElicitation_SessionDoesNotSupportElicitation(t *testing.T) { - server := NewMCPServer("test", "1.0.0", WithElicitation()) + t.Parallel() + server := NewMCPServer("test", "1.0.0", WithElicitation())func TestMCPServer_RequestElicitation_Success(t *testing.T) { - server := NewMCPServer("test", "1.0.0", WithElicitation()) + t.Parallel() + server := NewMCPServer("test", "1.0.0", WithElicitation())for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() server := NewMCPServer("test", "1.0", WithElicitation()) ctx := server.WithContext(context.Background(), tt.session)Also applies to: 84-86, 114-116, 241-245
3-10: Add coverage for error and cancellation pathsIssue #413 calls out proper handling of cancelled elicitations. Add cases where the session returns context.Canceled and an arbitrary error to ensure propagation and messaging.
Add imports:
import ( "context" + "errors" "testing"Add table cases:
{ name: "session does not support elicitation", session: &mockBasicSession{sessionID: "test-3"}, request: mcp.ElicitationRequest{ Params: mcp.ElicitationParams{ Message: "Need info", RequestedSchema: map[string]any{"type": "object"}, }, }, expectedError: "session does not support elicitation", }, + { + name: "elicitation canceled", + session: &mockElicitationSession{ + sessionID: "test-4", + err: context.Canceled, + }, + request: mcp.ElicitationRequest{ + Params: mcp.ElicitationParams{ + Message: "Please confirm", + RequestedSchema: map[string]any{"type": "object"}, + }, + }, + expectedError: "canceled", + }, + { + name: "elicitation request error propagated", + session: &mockElicitationSession{ + sessionID: "test-5", + err: errors.New("backend failure"), + }, + request: mcp.ElicitationRequest{ + Params: mcp.ElicitationParams{ + Message: "Provide details", + RequestedSchema: map[string]any{"type": "object"}, + }, + }, + expectedError: "backend failure", + },If server maps cancellations to a custom error, adjust expectedError accordingly.
Also applies to: 182-239
client/inprocess_elicitation_test.go (9)
12-17: Make MockElicitationHandler concurrency-safe (future-proofing).If this handler ever gets reused in parallel tests, reads/writes to CallCount and LastRequest will race. Consider adding a small mutex.
Example change:
type MockElicitationHandler struct { - // Track calls for verification - CallCount int - LastRequest mcp.ElicitationRequest + // Track calls for verification + mu sync.Mutex + CallCount int + LastRequest mcp.ElicitationRequest }and in Elicit:
- h.CallCount++ - h.LastRequest = request + h.mu.Lock() + h.CallCount++ + h.LastRequest = request + h.mu.Unlock()You’ll also need to import sync.
50-69: Tighten the requested schema for clearer contracts.Add additionalProperties: false to prevent unexpected fields, and (optionally) a title/description for readability in UIs/logs.
RequestedSchema: map[string]any{ - "type": "object", + "type": "object", + "title": "TestElicitationResponse", + "description": "Response for test_elicitation tool", "properties": map[string]any{ "confirm": map[string]any{ "type": "boolean", "description": "Confirm the action", }, "details": map[string]any{ "type": "string", "description": "Additional details", }, }, "required": []string{"confirm"}, + "additionalProperties": false, },
85-104: Handle unknown response types explicitly.Future enum extensions could lead to empty responseText. Add a default case and mark the result as error to surface issues quickly.
var responseText string + isError := false switch result.Response.Type { case mcp.ElicitationResponseTypeAccept: responseText = "User accepted and provided data" case mcp.ElicitationResponseTypeDecline: responseText = "User declined to provide information" case mcp.ElicitationResponseTypeCancel: responseText = "User cancelled the request" + default: + responseText = "Unknown elicitation response type" + isError = true } return &mcp.CallToolResult{ Content: []mcp.Content{ mcp.TextContent{ Type: "text", Text: responseText, }, }, - }, nil + IsError: isError, + }, nil
3-10: Use a test-scoped timeout context to avoid hangs.If wiring regresses, Start/Initialize/CallTool could block. A short timeout keeps CI green with clear failures.
import ( "context" + "time" "testing" "github.com/mark3labs/mcp-go/client/transport" "github.com/mark3labs/mcp-go/mcp" "github.com/mark3labs/mcp-go/server" )
113-116: Apply the timeout context to Start.- // Start the client - if err := client.Start(context.Background()); err != nil { + // Start the client with a timeout to prevent hangs + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := client.Start(ctx); err != nil { t.Fatalf("Failed to start client: %v", err) }
118-133: Apply the timeout context to Initialize.- _, err := client.Initialize(context.Background(), mcp.InitializeRequest{ + _, err := client.Initialize(ctx, mcp.InitializeRequest{ Params: mcp.InitializeParams{ ProtocolVersion: mcp.LATEST_PROTOCOL_VERSION, ClientInfo: mcp.Implementation{ Name: "test-client", Version: "1.0.0", }, Capabilities: mcp.ClientCapabilities{ Elicitation: &struct{}{}, }, }, })
135-147: Apply the timeout context to CallTool.- result, err := client.CallTool(context.Background(), mcp.CallToolRequest{ + result, err := client.CallTool(ctx, mcp.CallToolRequest{ Params: mcp.CallToolParams{ Name: "test_elicitation", Arguments: map[string]any{ "action": "test-action", }, }, })
173-176: Strengthen assertions: verify requested schema shape.Validate that the handler received the expected schema, not just the message. This prevents silent drift in schema wiring.
- if mockHandler.LastRequest.Params.Message != "Need additional information for test-action" { - t.Errorf("Unexpected elicitation message: %s", mockHandler.LastRequest.Params.Message) - } + if mockHandler.LastRequest.Params.Message != "Need additional information for test-action" { + t.Errorf("Unexpected elicitation message: %s", mockHandler.LastRequest.Params.Message) + } + // Verify schema basics + schema, ok := mockHandler.LastRequest.Params.RequestedSchema.(map[string]any) + if !ok { + t.Fatal("Expected requestedSchema to be a map[string]any") + } + props, ok := schema["properties"].(map[string]any) + if !ok { + t.Fatal("Expected properties to be a map[string]any") + } + confirmProp, ok := props["confirm"].(map[string]any) + if !ok || confirmProp["type"] != "boolean" { + t.Errorf("Expected 'confirm' boolean property in schema, got: %#v", confirmProp) + }
178-189: Set the client’s elicitation handler and validate inputs.
- Defensive nil checks make failures immediate and obvious in tests.
- Also pass the handler via WithElicitationHandler so the client advertises capabilities automatically if the Initialize helper reads it.
func NewInProcessClientWithElicitationHandler(server *server.MCPServer, handler ElicitationHandler) *Client { + if server == nil { + panic("NewInProcessClientWithElicitationHandler: server is nil") + } + if handler == nil { + panic("NewInProcessClientWithElicitationHandler: elicitation handler is nil") + } // Create a wrapper that implements server.ElicitationHandler serverHandler := &inProcessElicitationHandlerWrapper{handler: handler} inProcessTransport := transport.NewInProcessTransportWithOptions(server, transport.WithElicitationHandler(serverHandler)) - client := NewClient(inProcessTransport) + client := NewClient(inProcessTransport, WithElicitationHandler(handler)) return client }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
client/elicitation_test.go(1 hunks)client/inprocess_elicitation_test.go(1 hunks)examples/elicitation/main.go(1 hunks)server/elicitation_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/elicitation_test.go
- examples/elicitation/main.go
🧰 Additional context used
🧬 Code graph analysis (2)
client/inprocess_elicitation_test.go (6)
mcp/types.go (9)
ElicitationRequest(829-832)ElicitationResult(843-850)ElicitationResponse(853-859)ElicitationResponseTypeAccept(866-866)Params(181-181)ElicitationParams(835-840)ElicitationResponseTypeDecline(868-868)ElicitationResponseTypeCancel(870-870)ClientCapabilities(459-471)server/server.go (3)
NewMCPServer(307-333)WithElicitation(293-297)MCPServer(144-169)client/elicitation.go (1)
ElicitationHandler(11-19)server/inprocess_session.go (1)
ElicitationHandler(19-21)client/client.go (2)
Client(17-29)WithElicitationHandler(50-54)client/transport/inprocess.go (2)
NewInProcessTransportWithOptions(44-55)WithElicitationHandler(32-36)
server/elicitation_test.go (4)
mcp/types.go (9)
JSONRPCNotification(337-340)ElicitationResult(843-850)ElicitationRequest(829-832)Params(181-181)ElicitationParams(835-840)ElicitationResponse(853-859)ElicitationResponseTypeAccept(866-866)ElicitationResponseType(862-862)ElicitationResponseTypeDecline(868-868)server/server.go (2)
NewMCPServer(307-333)WithElicitation(293-297)mcp/utils.go (1)
ToBoolPtr(861-863)server/session.go (1)
ClientSession(11-20)
🔇 Additional comments (6)
server/elicitation_test.go (1)
12-30: Mocks look solid and minimalmockBasicSession and mockElicitationSession correctly satisfy the expected interfaces for these tests, with small, focused behavior. Good separation of concerns.
Also applies to: 31-57
client/inprocess_elicitation_test.go (5)
1-199: Solid end-to-end in-process elicitation test.Good coverage of the accept path, correct use of the new tool builder API, and clean wiring of the in-process transport elicitation handler. Comments below are mostly optional robustness and test-hardening suggestions.
19-33: LGTM: well-scoped mock handler behavior.Returning a deterministic accept response with representative data keeps the test focused and readable.
71-83: Error path is user-friendly and correctly flagged.Returning a textual error content with IsError: true gives the caller actionable context without losing the tool result shape.
191-198: LGTM: thin wrapper keeps responsibilities clear.Minimal indirection and no surprises; good boundary between client and server handler interfaces.
159-166: Assertion to mcp.TextContent is safe – no pointer variants foundI verified that
TextContentis defined as a concretestructand throughout the repository everymcp.Contententry is instantiated with a value literal (mcp.TextContent{…}), never as a pointer (*mcp.TextContent) or vianew(mcp.TextContent)or&mcp.TextContent(no matches across all Go files). Therefore the assertiontextContent, ok := result.Content[0].(mcp.TextContent)will not mistakenly miss a pointer type, and no type switch is needed. No changes required here.
| name: "session does not support elicitation", | ||
| session: &fakeSession{sessionID: "test-3"}, | ||
| request: mcp.ElicitationRequest{ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Undefined type fakeSession breaks compilation
The table-driven test references fakeSession, which isn’t defined anywhere in this file. This will fail to compile. Reuse the existing mockBasicSession (which intentionally lacks elicitation support) or define fakeSession.
Apply this minimal fix:
- session: &fakeSession{sessionID: "test-3"},
+ session: &mockBasicSession{sessionID: "test-3"},Alternatively, if you want a distinct type:
// Place near other mocks
type fakeSession struct{ sessionID string }
func (m *fakeSession) SessionID() string { return m.sessionID }
func (m *fakeSession) NotificationChannel() chan<- mcp.JSONRPCNotification { return make(chan mcp.JSONRPCNotification, 1) }
func (m *fakeSession) Initialize() {}
func (m *fakeSession) Initialized() bool { return true }🤖 Prompt for AI Agents
In server/elicitation_test.go around lines 229-231, the test references an
undefined type fakeSession which breaks compilation; either replace fakeSession
with the existing mockBasicSession (which lacks elicitation support) in that
table entry, or add a minimal fakeSession mock next to the other test mocks in
this file implementing SessionID(), NotificationChannel(), Initialize(), and
Initialized() as described in the review comment so the test compiles and
behaves the same.
There was a problem hiding this comment.
hmm I think you are mistaken, all tests pass
There was a problem hiding this comment.
🧩 Analysis chain
@semistrict You're absolutely right, and I apologize for the confusion! Let me check the complete file to see what I missed.
🏁 Script executed:
#!/bin/bash
# Let's examine the complete server/elicitation_test.go file to see if fakeSession is defined
cat server/elicitation_test.go
Length of output: 6801
@semistrict I'm puzzled by this! Looking at the complete file, I can see that fakeSession is indeed referenced in the table-driven test on line 229, but I don't see its definition anywhere in the file. Let me check if it's defined elsewhere in the server package.
🏁 Script executed:
#!/bin/bash
# Search for fakeSession definition in the server package
rg -n "type.*fakeSession" server/
echo "---"
# Also search for any other references to fakeSession
rg -n "fakeSession" server/
Length of output: 2821
@semistrict You're absolutely correct, and I apologize for my mistake! I found the answer:
fakeSession is defined in server/server_test.go (line 1802) and since both files are in the same server package, elicitation_test.go can access it. This is a common Go pattern where test utilities are shared across test files within the same package.
The fakeSession type implements ClientSession but not SessionWithElicitation, making it perfect for testing the "session does not support elicitation" scenario in the table-driven test.
Thank you for pushing back on my analysis - you were right that the tests pass, and I was working with incomplete information by not considering the broader package context. This is a good reminder to look at the full package structure when analyzing Go code!
✏️ Learnings added
Learnt from: semistrict
PR: mark3labs/mcp-go#495
File: server/elicitation_test.go:229-231
Timestamp: 2025-08-24T00:52:47.590Z
Learning: In Go, test files within the same package can access types and functions defined in other test files in that package. When analyzing test code that references undefined types, check other test files in the same package for shared test utilities and mock types.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Description
This PR implements MCP elicitation support, allowing servers to request additional information from clients during interactions. The implementation follows the same patterns as the existing sampling feature, providing bidirectional communication capabilities for stdio and in-process transports.
Fixes #413
Type of Change
Checklist
MCP Spec Compliance
-- we do not implement response schema validation in the server
Additional Information
Implementation Details
ElicitationRequest,ElicitationResult,ElicitationParams,ElicitationResponse, andElicitationResponseTypeWithElicitation()option andRequestElicitation()methodElicitationHandlerinterface andWithElicitationHandler()optionDesign Decisions
Summary by CodeRabbit