Implement Elicitation URL mode for MCP spec 2025-11-25 #666
Conversation
WalkthroughAdds URL-mode elicitation to MCP: new mode constants, typed ElicitationCapability, extended ElicitationParams (meta, mode, optional requestedSchema, elicitationId, url), URLElicitationRequiredError with JSON-RPC mapping, completion notification, client/server helpers and validation, tests, and example URL-flow tooling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp/errors.go (1)
81-109: Add case forURL_ELICITATION_REQUIREDinAsError()to reconstructURLElicitationRequiredError.When the client receives a JSON-RPC error with code
-32042,AsError()falls through to the default case and returns a generic error, losing theelicitationsdata. Add a case that reconstructsURLElicitationRequiredErrorfrome.Dataso clients can access elicitation details through the standard error conversion path (lines 81–109).
🧹 Nitpick comments (7)
server/elicitation.go (1)
35-66: URL-mode elicitation helper is well-shaped; capability checks remain caller responsibility
RequestURLElicitationcorrectly constructs URL-modeElicitationParams(Mode, Message, ElicitationID, URL) and routes through the existingSessionWithElicitationpath. It doesn’t itself validate that the client advertised URL-mode support or that the URL uses HTTPS; those checks are reasonable to leave to higher-level call sites that have access to negotiated capabilities and deployment policy.mcp/errors.go (1)
35-42: Consider addingIs()method forerrors.Iscompatibility.
UnsupportedProtocolVersionErrorimplementsIs()(lines 66-70) for better error handling witherrors.Is. For consistency and as per coding guidelines,URLElicitationRequiredErrorcould benefit from the same pattern.+// Is implements the errors.Is interface for better error handling +func (e URLElicitationRequiredError) Is(target error) bool { + _, ok := target.(URLElicitationRequiredError) + return ok +}server/elicitation_test.go (2)
46-51: Potential goroutine leak: unbuffered notification channel discarded.When
notifyChanis nil, a new buffered channel is created but never stored or returned to the caller for draining. If notifications are sent to this channel and it fills up, senders could block. Consider documenting this as intentional for tests where notifications aren't expected, or use a larger buffer.
239-241: Redundant assertion:result.Actionis a string type, never nil.
ElicitationResponseActionis astringtype alias. Theassert.NotNilcheck is redundant sinceresult.Actionis already validated byassert.Equal(t, tt.expectedType, result.Action)on line 237.if tt.expectedType == mcp.ElicitationResponseActionAccept { - assert.NotNil(t, result.Action) + assert.NotNil(t, result.Content) }Or remove this block entirely if content validation isn't needed.
examples/elicitation/main.go (2)
271-287: Misleading flow: completion notification sent before browser authentication completes.In a real URL elicitation flow,
SendElicitationCompleteshould be called after the user completes authentication in the browser (e.g., via webhook or polling). Here, it's called immediately after the user consents to open the URL, before they've actually authenticated. This is fine for a demo, but consider adding a comment clarifying this is simulated.if result.Action == mcp.ElicitationResponseActionAccept { // User consented to open the URL // They will complete the flow in their browser // Server will store credentials when user submits the form - // Simulate sending completion notification + // NOTE: In production, this notification would be sent after + // the server receives the authentication callback from the browser. + // Here we simulate immediate completion for demonstration purposes. if err := mcpServer.SendElicitationComplete(ctx, session, elicitationID); err != nil {
303-329: Dead code: success path is unreachable with hardcodedisAuthorized := false.The code at lines 324-328 is never executed. While this is likely intentional for demonstration, consider using a TODO comment or restructuring to make the intent clearer.
func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - // Check if identified - isAuthorized := false + // TODO: In production, check actual authorization state + // For demo purposes, we always trigger elicitation + isAuthorized := false // Always false to demonstrate error flow if !isAuthorized {mcp/types.go (1)
1311-1320: Consider extracting method string to a constant.The method
"notifications/elicitation/complete"is hardcoded. For consistency with other methods defined as constants (lines 17-82), consider adding a constant likeMethodNotificationElicitationComplete.+// MethodNotificationElicitationComplete notifies when a URL mode elicitation completes. +MethodNotificationElicitationComplete MCPMethod = "notifications/elicitation/complete"Then use it in the constructor:
func NewElicitationCompleteNotification(elicitationID string) *ElicitationCompleteNotification { return &ElicitationCompleteNotification{ Notification: Notification{ - Method: "notifications/elicitation/complete", + Method: string(MethodNotificationElicitationComplete), },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
client/client.go(1 hunks)client/elicitation.go(1 hunks)client/inprocess_elicitation_test.go(1 hunks)examples/elicitation/main.go(4 hunks)mcp/consts.go(1 hunks)mcp/elicitation_test.go(1 hunks)mcp/errors.go(1 hunks)mcp/errors_test.go(1 hunks)mcp/types.go(4 hunks)server/elicitation.go(1 hunks)server/elicitation_test.go(8 hunks)server/server.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
mcp/errors_test.gomcp/consts.goserver/server.gomcp/elicitation_test.goclient/inprocess_elicitation_test.goserver/elicitation.gomcp/errors.goserver/elicitation_test.goclient/client.goexamples/elicitation/main.gomcp/types.goclient/elicitation.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
mcp/errors_test.gomcp/elicitation_test.goclient/inprocess_elicitation_test.goserver/elicitation_test.go
🧠 Learnings (10)
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
mcp/errors_test.gomcp/elicitation_test.goserver/elicitation_test.go
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
server/server.gomcp/elicitation_test.goclient/inprocess_elicitation_test.goclient/client.goexamples/elicitation/main.gomcp/types.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
mcp/elicitation_test.goserver/elicitation_test.go
📚 Learning: 2025-03-04T07:00:57.111Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
mcp/elicitation_test.goexamples/elicitation/main.go
📚 Learning: 2025-08-24T00:52:47.610Z
Learnt from: semistrict
Repo: mark3labs/mcp-go PR: 495
File: server/elicitation_test.go:229-231
Timestamp: 2025-08-24T00:52:47.610Z
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.
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*.go : Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Write table-driven tests using a tests := []struct{ name, ... } pattern
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*.go : Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
Applied to files:
examples/elicitation/main.go
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
examples/elicitation/main.go
📚 Learning: 2025-03-04T06:59:43.882Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
examples/elicitation/main.gomcp/types.go
🧬 Code graph analysis (9)
mcp/errors_test.go (2)
mcp/errors.go (2)
URLElicitationRequiredError(36-38)URL_ELICITATION_REQUIRED(32-32)mcp/types.go (2)
ElicitationParams(888-906)JSONRPCError(356-360)
server/server.go (1)
mcp/types.go (1)
ElicitationCapability(1296-1299)
mcp/elicitation_test.go (2)
mcp/types.go (2)
ElicitationParams(888-906)ElicitationCapability(1296-1299)mcp/consts.go (2)
ElicitationModeForm(10-10)ElicitationModeURL(11-11)
client/inprocess_elicitation_test.go (1)
mcp/types.go (1)
ElicitationCapability(1296-1299)
server/elicitation.go (2)
mcp/types.go (8)
ElicitationResult(909-912)ElicitationParams(888-906)ElicitationRequest(882-885)Request(179-182)MethodElicitationCreate(60-60)Params(188-188)JSONRPCNotification(343-346)Notification(190-193)mcp/consts.go (1)
ElicitationModeURL(11-11)
mcp/errors.go (2)
mcp/types.go (3)
ElicitationParams(888-906)JSONRPCError(356-360)JSONRPCErrorDetails(364-373)client/transport/error.go (1)
Error(6-8)
server/elicitation_test.go (4)
mcp/types.go (7)
ElicitationResult(909-912)ElicitationRequest(882-885)JSONRPCNotification(343-346)ElicitationResponseActionAccept(928-928)Content(1006-1008)ElicitationResponse(915-921)Params(188-188)server/elicitation.go (2)
ErrNoActiveSession(12-12)ErrElicitationNotSupported(14-14)server/server.go (2)
NewMCPServer(345-373)WithElicitation(324-328)mcp/consts.go (1)
ElicitationModeURL(11-11)
client/client.go (1)
mcp/types.go (1)
ElicitationCapability(1296-1299)
examples/elicitation/main.go (5)
server/session.go (1)
ClientSessionFromContext(112-117)mcp/types.go (3)
ElicitationResponseActionAccept(928-928)Content(1006-1008)ElicitationParams(888-906)mcp/utils.go (1)
NewTextContent(225-230)mcp/errors.go (1)
URLElicitationRequiredError(36-38)mcp/consts.go (1)
ElicitationModeURL(11-11)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (12)
server/server.go (1)
703-705: Typed ElicitationCapability wiring in initialize looks correctUsing
&mcp.ElicitationCapability{}here matches the new public type and keeps the JSON payload backward-compatible ({}today, with room to setForm/URLflags later if desired). No issues from a server capability perspective.mcp/consts.go (1)
3-11: Elicitation mode constants are well-definedThe new
ElicitationModeForm/ElicitationModeURLconstants are appropriately named and align with their usage inElicitationParamsand tests.client/inprocess_elicitation_test.go (1)
126-128: Test capability setup matches new ElicitationCapability typeSwitching to
&mcp.ElicitationCapability{}keeps this in-process test consistent with the updated capabilities schema and client initialization.mcp/elicitation_test.go (1)
1-94: Elicitation params & capability serialization tests are solidThe table-driven coverage for form vs URL modes and the capability JSON shapes (empty/form/url) is clear and uses testify as per test guidelines. This gives good confidence in wire-format stability of the new types.
mcp/errors_test.go (1)
172-203: URLElicitationRequiredError behavior test is appropriateThe test thoroughly checks the error string, JSON-RPC code/message, and the typed
elicitationspayload inData, covering the main semantics of the new error type.client/client.go (1)
200-203: Client now advertises elicitation via concrete ElicitationCapabilityUsing
&mcp.ElicitationCapability{}when anElicitationHandleris set keeps capability negotiation consistent with the new type and mirrors the server side. Note this still intentionally overwrites any pre-setCapabilities.Elicitation, matching the existing pattern for sampling/roots.server/elicitation.go (1)
20-33: Explicit ErrElicitationNotSupported return improves error signalingReturning
ErrElicitationNotSupportedwhen the session doesn’t implementSessionWithElicitationmakes the behavior explicit and lets callers distinguish “no active session” from “session doesn’t support elicitation”.server/elicitation_test.go (1)
246-281: LGTM with minor suggestion: Consider splitting into separate test cases.The test correctly validates
RequestURLElicitationandSendElicitationComplete. For better isolation and readability per coding guidelines (table-driven tests pattern), consider splitting into separate test functions or using subtests witht.Run().examples/elicitation/main.go (1)
240-295: Good example demonstrating URL elicitation flow.The
auth_via_urltool effectively demonstrates the URL elicitation pattern: generating a unique ID, constructing a tracking URL, requesting elicitation, and handling the response. The error handling forSendElicitationComplete(log and continue) is appropriate for this use case.mcp/types.go (3)
1295-1299: LGTM: Capability struct follows established patterns.The
ElicitationCapabilitystruct using*struct{}pointers forFormandURLis consistent with other capability definitions (e.g.,Sampling *struct{}at line 491). This allows proper distinction between unset and explicitly enabled modes.
888-906: LGTM: ElicitationParams properly extended for URL mode.The struct extension is well-designed:
- Mode defaults to "form" for backward compatibility
- Fields are grouped by mode with clear comments
- All optional fields use
omitemptyappropriately- Meta field added for protocol extensibility
493-493:and
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
mcp/errors.go (2)
31-32: Use aconstfor the error code and avoid mixing it into the sentinel errorvarblock
URL_ELICITATION_REQUIREDrepresents a fixed JSON-RPC error code, so defining it as aconst(and keeping thevarblock scoped to sentinelerrorvalues) would be more idiomatic and prevent accidental mutation.For example:
const URL_ELICITATION_REQUIRED = -32042You can place this near the other JSON-RPC error code constants for consistency.
35-55: URLElicitationRequiredError struct and JSON-RPC mapping look correct and spec-alignedThe exported type, GoDoc, and JSON tag on
Elicitationsare consistent with the rest of the package, andError()provides a clear, deterministic message.JSONRPCError()now correctly setsJSONRPC: JSONRPC_VERSIONand wraps the elicitation payload underdata.elicitations, matching the intended JSON-RPC integration for this new error code.If, in the future, you want client-side callers of
JSONRPCErrorDetails.AsError()to be able to detect this condition viaerrors.As, consider adding a dedicated branch forURL_ELICITATION_REQUIREDthat reconstructs aURLElicitationRequiredErrorfromData, but that’s optional and can be deferred.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mcp/errors.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
mcp/errors.go
🧬 Code graph analysis (1)
mcp/errors.go (2)
mcp/types.go (4)
ElicitationParams(888-906)JSONRPCError(356-360)JSONRPC_VERSION(122-122)JSONRPCErrorDetails(364-373)client/transport/error.go (1)
Error(6-8)
⏰ 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). (1)
- GitHub Check: test
…rror`, implement its `Is` method, and add a corresponding test.
…th increased capacity and correct test assertion for accepted response content.
…tionCompleteNotification`.
… nil session check
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
mcp/errors.go (1)
105-122: Consider renaming shadowed error variable for clarity.The
errvariable inside theifblocks (lines 110, 114) shadows the outererrdeclared at line 88. While this is scoped correctly and works, it could be confusing during maintenance.Consider renaming to avoid shadowing:
case URL_ELICITATION_REQUIRED: // Attempt to reconstruct URLElicitationRequiredError from Data if e.Data != nil { // Round-trip through JSON to parse into struct // This handles both map[string]any (from unmarshal) and other forms - if dataBytes, err := json.Marshal(e.Data); err == nil { + if dataBytes, marshalErr := json.Marshal(e.Data); marshalErr == nil { var data struct { Elicitations []ElicitationParams `json:"elicitations"` } - if err := json.Unmarshal(dataBytes, &data); err == nil { + if unmarshalErr := json.Unmarshal(dataBytes, &data); unmarshalErr == nil { return URLElicitationRequiredError{ Elicitations: data.Elicitations, } } } } // Fallback if data is missing or invalid return URLElicitationRequiredError{}server/elicitation_test.go (2)
46-51: Consider documenting the buffer size choice.The notification channel buffer size was changed from 1 to 100. While 100 is reasonable for test scenarios, a brief comment explaining why this size was chosen would aid future maintenance.
func (m *mockElicitationSession) NotificationChannel() chan<- mcp.JSONRPCNotification { if m.notifyChan == nil { + // Buffer of 100 to avoid blocking during tests with multiple notifications m.notifyChan = make(chan mcp.JSONRPCNotification, 100) } return m.notifyChan }
275-280: Consider validating elicitationId in notification params.The test validates the notification method but doesn't verify that the
elicitationIdis correctly included in the notification params. This would ensure complete end-to-end validation.select { case notif := <-notifyChan: assert.Equal(t, "notifications/elicitation/complete", notif.Method) + // Validate elicitationId is included in params + elicitationID, ok := notif.Params.AdditionalFields["elicitationId"] + assert.True(t, ok, "expected elicitationId in notification params") + assert.Equal(t, "id-123", elicitationID) case <-time.After(100 * time.Millisecond): t.Error("Expected notification not received") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/elicitation/main.go(4 hunks)mcp/errors.go(4 hunks)mcp/errors_test.go(1 hunks)mcp/types.go(6 hunks)server/elicitation.go(1 hunks)server/elicitation_test.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mcp/errors_test.go
- server/elicitation.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
examples/elicitation/main.goserver/elicitation_test.gomcp/errors.gomcp/types.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/elicitation_test.go
🧠 Learnings (13)
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
examples/elicitation/main.gomcp/errors.gomcp/types.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*.go : Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
Applied to files:
examples/elicitation/main.go
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
examples/elicitation/main.go
📚 Learning: 2025-03-04T06:59:43.882Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
examples/elicitation/main.gomcp/errors.gomcp/types.go
📚 Learning: 2025-03-04T07:00:57.111Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
examples/elicitation/main.gomcp/errors.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
server/elicitation_test.gomcp/errors.go
📚 Learning: 2025-08-24T00:52:47.610Z
Learnt from: semistrict
Repo: mark3labs/mcp-go PR: 495
File: server/elicitation_test.go:229-231
Timestamp: 2025-08-24T00:52:47.610Z
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.
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*.go : Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Write table-driven tests using a tests := []struct{ name, ... } pattern
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-08-08T15:37:18.458Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 0
File: :0-0
Timestamp: 2025-08-08T15:37:18.458Z
Learning: In mark3labs/mcp-go stdio transport, using bufio.Scanner without calling Scanner.Buffer imposes a 64 KiB per-line limit; to support very long lines safely, set a larger max via Scanner.Buffer or use a streaming Reader/json.Decoder approach.
Applied to files:
mcp/errors.go
📚 Learning: 2025-06-20T20:39:51.870Z
Learnt from: lariel-fernandes
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
mcp/errors.go
📚 Learning: 2025-06-26T09:38:18.629Z
Learnt from: davidleitw
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
mcp/errors.go
🧬 Code graph analysis (1)
mcp/errors.go (1)
mcp/types.go (5)
ElicitationParams(894-912)JSONRPCError(359-363)JSONRPC_VERSION(125-125)JSONRPCErrorDetails(367-376)URL_ELICITATION_REQUIRED(405-405)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (11)
examples/elicitation/main.go (3)
240-298: LGTM! Well-documented URL elicitation demo.The
auth_via_urltool implementation correctly demonstrates the URL elicitation flow:
- Generates a unique elicitation ID using UUID
- Constructs a tracking URL with the elicitation ID
- Calls
RequestURLElicitationwith proper parameters- Handles both accept and decline responses
- Includes helpful comments explaining that in production,
SendElicitationCompletewould be called after receiving the authentication callbackThe immediate call to
SendElicitationCompleteis appropriately documented as a simulation for demonstration purposes.
300-334: LGTM! Clear demonstration of error-triggered elicitation.The
protected_actiontool correctly demonstrates returningURLElicitationRequiredErrorto signal that authorization is required. TheElicitationParamsis properly populated with all URL-mode fields (Mode,ElicitationID,URL,Message).The TODO comment at line 307 appropriately documents that the
isAuthorized := falseis for demonstration purposes.
3-14: Import ordering follows guidelines.Standard library imports are listed first, followed by third-party packages (
github.com/google/uuid), then local packages (github.com/mark3labs/mcp-go/*). This follows the Go import conventions enforced by goimports.mcp/errors.go (2)
34-54: LGTM! URLElicitationRequiredError properly implemented.The error type correctly:
- Implements
Error()with a descriptive message including elicitation count- Sets
JSONRPC: JSONRPC_VERSIONinJSONRPCError()(addressing the previous review feedback)- Uses the correct error code
URL_ELICITATION_REQUIRED(-32042)- Includes the elicitations payload in the error data
66-70: LGTM! Is() method follows established pattern.The
Is()implementation enableserrors.Is()matching forURLElicitationRequiredError, following the same pattern asUnsupportedProtocolVersionError.mcp/types.go (4)
61-63: LGTM! Method constant for elicitation complete notification.The new
MethodNotificationElicitationCompleteconstant follows the established naming pattern for MCP methods and correctly defines the notification method name.
893-912: LGTM! ElicitationParams extended for URL mode support.The struct now properly supports both elicitation modes:
Modedefaults to "form" when omitted (backward compatible)- Form-mode field
RequestedSchemamade optional for URL mode- URL-mode fields
ElicitationIDandURLadded with appropriateomitemptytags- Added
Metafield for optional metadataThe structure maintains backward compatibility with existing form-mode usage.
1301-1320: LGTM! ElicitationCapability and notification helper well implemented.
ElicitationCapabilitycorrectly uses the pointer-to-empty-struct pattern (*struct{}) for presence signaling, consistent with other capability structs in this file (e.g.,Sampling,Logging).
NewElicitationCompleteNotificationproperly constructs aJSONRPCNotificationusingAdditionalFieldsinNotificationParamsto carry theelicitationId, which aligns with howNotificationParamshandles extra fields via its custom JSON marshaling.
499-499: Verify the backward compatibility interpretation for emptyElicitationCapability.The
Elicitationfield was changed from*struct{}to*ElicitationCapability. The implementation creates emptyElicitationCapability{}structs (with bothFormandURLnil), but there is no visible logic that interprets nil fields as "form-only support." Per PR objectives, this should maintain backward compatibility, but the current approach is ambiguous—an empty struct with nil fields doesn't explicitly declare which modes are supported. Confirm whether nilForm/URLfields should be interpreted as form-only, or if the struct should explicitly setForm = &struct{}{}to properly indicate form mode support.Also applies to: 531-531
server/elicitation_test.go (2)
246-281: LGTM! Comprehensive URL elicitation test.The test properly validates:
RequestURLElicitationpopulatesMode,ElicitationID, andURLin the request paramsSendElicitationCompletedelivers the notification through the session's channel- The notification method matches
"notifications/elicitation/complete"The use of
selectwith a 100ms timeout is appropriate for async notification verification in test environments.
82-83: LGTM! Test assertions follow testify guidelines.Tests correctly use
requirefor fatal assertions (e.g., error checks) andassertfor value comparisons. This follows the coding guidelines for test files. The table-driven test pattern inTestRequestElicitationis well-structured.Also applies to: 106-107, 146-152
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mcp/errors.go(4 hunks)mcp/types.go(6 hunks)server/elicitation_test.go(8 hunks)server/server.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mcp/errors.go
- server/server.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
mcp/types.goserver/elicitation_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/elicitation_test.go
🧠 Learnings (8)
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
mcp/types.go
📚 Learning: 2025-03-04T06:59:43.882Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
mcp/types.go
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
mcp/types.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-08-24T00:52:47.610Z
Learnt from: semistrict
Repo: mark3labs/mcp-go PR: 495
File: server/elicitation_test.go:229-231
Timestamp: 2025-08-24T00:52:47.610Z
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.
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*.go : Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Write table-driven tests using a tests := []struct{ name, ... } pattern
Applied to files:
server/elicitation_test.go
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (9)
mcp/types.go (5)
61-63: LGTM!The new method constant follows the established pattern for notification methods and includes appropriate documentation.
424-425: LGTM!The new error code follows MCP conventions and is properly documented.
519-519: LGTM! Breaking change is intentional.The change from
*struct{}to*ElicitationCapabilityenables proper capability negotiation for form and URL modes, aligning with the MCP spec. This follows the pattern established by other structured capabilities likeTasksCapability.Also applies to: 553-553
1483-1487: LGTM!The
ElicitationCapabilitystruct follows established patterns for capability types. Using optional pointer fields enables clear declaration of form and/or URL support while maintaining backward compatibility.
1489-1502: LGTM!The notification constructor follows Go conventions and correctly uses
AdditionalFieldsto include the elicitation ID in the notification payload.server/elicitation_test.go (4)
7-7: LGTM!The time import is necessary for timeout handling in the new URL elicitation test.
35-40: Good test mock improvements.The additions enable better test verification:
lastRequestfield allows asserting on exact parameters passed- Buffered notification channel (capacity 100) prevents blocking
- Lazy initialization is efficient
These changes support the new URL elicitation test requirements.
Also applies to: 46-52, 61-61
83-84: LGTM!The migration to testify assertions follows coding guidelines and improves test clarity. The use of
requirefor fatal conditions andassertfor multiple checks is appropriate.Based on coding guidelines.
Also applies to: 107-109, 147-154, 231-242
212-212: Good consistency improvement.Using the defined
mockBasicSessiontype improves maintainability and keeps test setup consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/elicitation_test.go (1)
247-286: Test scope doesn't match the test name.
TestRequestURLElicitationactually tests bothRequestURLElicitation(lines 260-265) andSendElicitationComplete(lines 273-285). The test name should reflect the full scope, or the test should be split into two separate focused tests.server/elicitation.go (1)
74-75: Remove duplicate comment.Lines 74 and 75 contain identical comments. Remove one of them.
🔎 Proposed fix
-// SendElicitationComplete sends a notification that a URL mode elicitation has completed // SendElicitationComplete sends a notification that a URL mode elicitation has completed func (s *MCPServer) SendElicitationComplete(
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/client.go(2 hunks)mcp/elicitation_validation_test.go(1 hunks)mcp/types.go(6 hunks)server/elicitation.go(1 hunks)server/elicitation_test.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/client.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/elicitation_test.goserver/elicitation.gomcp/elicitation_validation_test.gomcp/types.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/elicitation_test.gomcp/elicitation_validation_test.go
🧠 Learnings (10)
📚 Learning: 2025-04-28T00:14:49.263Z
Learnt from: robert-jackson-glean
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
server/elicitation_test.gomcp/elicitation_validation_test.go
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
server/elicitation_test.gomcp/elicitation_validation_test.go
📚 Learning: 2025-08-24T00:52:47.610Z
Learnt from: semistrict
Repo: mark3labs/mcp-go PR: 495
File: server/elicitation_test.go:229-231
Timestamp: 2025-08-24T00:52:47.610Z
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.
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*.go : Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Write table-driven tests using a tests := []struct{ name, ... } pattern
Applied to files:
server/elicitation_test.gomcp/elicitation_validation_test.go
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
server/elicitation.gomcp/types.go
📚 Learning: 2025-06-20T20:39:51.870Z
Learnt from: lariel-fernandes
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
mcp/elicitation_validation_test.go
📚 Learning: 2025-03-04T06:59:43.882Z
Learnt from: xinwo
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
mcp/types.go
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
mcp/types.go
🧬 Code graph analysis (2)
server/elicitation.go (4)
mcp/types.go (7)
Params(211-211)ElicitationResult(965-968)ElicitationParams(918-936)ElicitationRequest(912-915)Request(202-205)MethodElicitationCreate(60-60)NewElicitationCompleteNotification(1516-1528)server/server.go (1)
MCPServer(154-183)server/session.go (2)
ClientSession(12-21)SessionWithElicitation(79-83)mcp/consts.go (1)
ElicitationModeURL(11-11)
mcp/elicitation_validation_test.go (2)
mcp/types.go (1)
ElicitationParams(918-936)mcp/consts.go (2)
ElicitationModeForm(10-10)ElicitationModeURL(11-11)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (5)
server/elicitation.go (1)
28-30: LGTM!The validation call ensures ElicitationParams are properly validated before being sent to the client, addressing the previous review feedback about mode-dependent field validation.
mcp/types.go (4)
61-63: LGTM!The new notification method constant follows MCP naming conventions and is properly documented.
919-936: LGTM!The ElicitationParams structure properly supports both form and URL modes with clear field separation and appropriate JSON tags. The mode discriminator pattern follows MCP conventions.
938-962: LGTM!The Validate method correctly implements mode-specific validation, treating empty mode as form mode and enforcing required fields for each mode. This addresses the validation requirements from previous review feedback.
1509-1528: LGTM!The ElicitationCapability type follows the MCP capability pattern (using
*struct{}for presence indicators), and the NewElicitationCompleteNotification helper correctly constructs the notification with proper structure.
…peration and improve `ElicitationParams.Validate` error assertions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/elicitation_test.go (1)
247-286: Consider splitting or renaming test for clarity.
TestRequestURLElicitationtests bothRequestURLElicitation(lines 260-265) andSendElicitationComplete(lines 267-285). Consider either:
- Splitting into two separate test functions for better failure isolation, or
- Renaming to
TestRequestURLElicitation_WithNotificationCompleteto reflect the full scope🔎 Option 1: Split into two tests
func TestRequestURLElicitation(t *testing.T) { s := NewMCPServer("test", "1.0", WithElicitation()) mockSession := &mockElicitationSession{ sessionID: "test-url-1", result: &mcp.ElicitationResult{ ElicitationResponse: mcp.ElicitationResponse{ Action: mcp.ElicitationResponseActionAccept, }, }, } ctx := context.Background() _, err := s.RequestURLElicitation(ctx, mockSession, "id-123", "https://example.com/auth", "Please auth") require.NoError(t, err) assert.Equal(t, mcp.ElicitationModeURL, mockSession.lastRequest.Params.Mode) assert.Equal(t, "id-123", mockSession.lastRequest.Params.ElicitationID) assert.Equal(t, "https://example.com/auth", mockSession.lastRequest.Params.URL) +} + +func TestSendElicitationComplete_WithNotification(t *testing.T) { + s := NewMCPServer("test", "1.0", WithElicitation()) notifyChan := make(chan mcp.JSONRPCNotification, 1) - mockSessionWithChan := &mockElicitationSession{ - sessionID: "test-url-2", + mockSession := &mockElicitationSession{ + sessionID: "test-session", notifyChan: notifyChan, } - err = s.SendElicitationComplete(ctx, mockSessionWithChan, "id-123") + err := s.SendElicitationComplete(context.Background(), mockSession, "id-123") require.NoError(t, err) select { case notif := <-notifyChan: assert.Equal(t, "notifications/elicitation/complete", notif.Method) - // Validate elicitationId is included in params elicitationID, ok := notif.Params.AdditionalFields["elicitationId"] assert.True(t, ok, "expected elicitationId in notification params") assert.Equal(t, "id-123", elicitationID) case <-time.After(100 * time.Millisecond): t.Error("Expected notification not received") } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mcp/elicitation_validation_test.go(1 hunks)server/elicitation_test.go(8 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/elicitation_test.gomcp/elicitation_validation_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/elicitation_test.gomcp/elicitation_validation_test.go
🧠 Learnings (8)
📚 Learning: 2025-04-28T00:14:49.263Z
Learnt from: robert-jackson-glean
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Testing: use testify/assert and testify/require
Applied to files:
server/elicitation_test.gomcp/elicitation_validation_test.go
📚 Learning: 2025-04-21T21:26:32.945Z
Learnt from: octo
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
server/elicitation_test.gomcp/elicitation_validation_test.go
📚 Learning: 2025-08-24T00:52:47.610Z
Learnt from: semistrict
Repo: mark3labs/mcp-go PR: 495
File: server/elicitation_test.go:229-231
Timestamp: 2025-08-24T00:52:47.610Z
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.
Applied to files:
server/elicitation_test.gomcp/elicitation_validation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*.go : Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Applied to files:
server/elicitation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Applies to **/*_test.go : Write table-driven tests using a tests := []struct{ name, ... } pattern
Applied to files:
server/elicitation_test.gomcp/elicitation_validation_test.go
📚 Learning: 2025-10-13T09:35:20.180Z
Learnt from: CR
Repo: mark3labs/mcp-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-13T09:35:20.180Z
Learning: Use the provided commands for testing, coverage, linting, and code generation (go test ./... -race, coverage command with exclusions, golangci-lint run, go generate ./...)
Applied to files:
mcp/elicitation_validation_test.go
📚 Learning: 2025-06-20T20:39:51.870Z
Learnt from: lariel-fernandes
Repo: mark3labs/mcp-go PR: 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.
Applied to files:
mcp/elicitation_validation_test.go
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (5)
mcp/elicitation_validation_test.go (1)
10-89: LGTM! Comprehensive validation test coverage.The test follows all coding guidelines: table-driven pattern, testify assertions, and comprehensive coverage of both Form and URL mode validation scenarios. The previous review comment regarding testify usage has been addressed.
server/elicitation_test.go (4)
34-66: LGTM! Mock session improvements enable better test verification.The addition of
lastRequestandnotifyChanfields with lazy channel initialization provides better test observability. The buffered channel (capacity 100) prevents blocking during tests with multiple notifications.
68-245: LGTM! Test updates follow coding guidelines.Existing tests properly updated to use testify assertions (
require.Error,require.NoError,assert.Equal) and follow error handling best practices witherrors.Is.
288-311: LGTM! Independent notification scenario well-tested.This test verifies
SendElicitationCompletecan send notifications independently without prior request state, addressing the previous review comment about duplicate tests. The test correctly validates the notification structure and elicitationId propagation.
247-286: Add security constraint tests for URL elicitation.The PR implements server URL elicitation but the test at lines 247-286 only validates happy-path functionality. Per MCP specification requirements, servers must securely associate elicitation state with individual users (not session IDs alone), and user identification must be derived from MCP authorization credentials. Add tests covering:
- URL format validation (parsing, scheme verification)
- State binding to user identity rather than session alone
- Rejection of HTTP URLs (HTTPS enforcement per security best practices)
- Prevention of sensitive data in URL parameters
2 participants
Description
Implemented full support for URL-based elicitation (out-of-band authentication/interaction) as defined in the MCP specification.
Key changes:
mcppackage: AddedElicitationModeURLconstant,URLElicitationRequiredErrortype, and updatedElicitationParams/ElicitationCapabilityto support URL mode.serverpackage: AddedRequestURLElicitationandSendElicitationCompletehelper methods for easier implementation of out-of-band flows.examples: Added a comprehensive example inexamples/elicitation/main.godemonstrating:auth_via_url).protected_actionreturningURLElicitationRequiredError).client: Updated client capability negotiation to support URL mode.mcp/elicitation_test.go,mcp/errors_test.go, andserver/elicitation_test.goto use thetestifypackage for more robust assertions.Fixes #655
Type of Change
Checklist
MCP Spec Compliance
Summary by CodeRabbit
New Features
Bug Fixes / Behavioral
Tests
✏️ Tip: You can customize this high-level summary in your review settings.