Server handlers implementation for auto-completion #679
Conversation
WalkthroughAdds typed completion support: new completion RPC/method and types with custom JSON unmarshalling, completion provider interfaces and defaults, server options and handler for completion requests, completion-specific hooks, example utilities and tests, and README documentation (duplicated blocks present). No public API removals. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
ezraisw
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
mcp/types_test.go (1)
384-407: Consider adding a test case forCompleteContextparsing.The current tests validate
RefandArgumentparsing but don't cover theContextfield (which containsArguments map[string]string). Adding a test case with"context": {"arguments": {"key": "value"}}would improve coverage.🔎 Suggested additional test case
{ name: "PromptReference with context", inputJSON: `{ "ref": { "type": "ref/prompt", "name": "test-prompt" }, "argument": { "name": "test-arg", "value": "test-value" }, "context": { "arguments": { "previous_arg": "previous_value" } } }`, expectedErr: "", expected: CompleteParams{ Ref: PromptReference{ Type: "ref/prompt", Name: "test-prompt", }, Argument: CompleteArgument{ Name: "test-arg", Value: "test-value", }, Context: CompleteContext{ Arguments: map[string]string{ "previous_arg": "previous_value", }, }, }, },server/server_test.go (1)
2882-3100: Comprehensive test coverage for completion functionality.The tests cover key scenarios:
- Capability disabled →
METHOD_NOT_FOUND- Invalid reference type →
INVALID_REQUESTwith descriptive error- Default providers returning empty completions
- Custom providers with context propagation verification
Consider adding a test case for provider error handling (when a custom provider returns an error) to verify the
INTERNAL_ERRORresponse path.🔎 Optional: Add error handling test case
t.Run("Provider returns error", func(t *testing.T) { message := `{ "jsonrpc": "2.0", "id": 1, "method": "completion/complete", "params": { "ref": { "type": "ref/prompt", "name": "code_review" }, "argument": { "name": "language", "value": "py" } } }` server := NewMCPServer("test-server", "1.0.0", WithCompletions(), WithPromptCompletionProvider( promptCompletionProviderFunc(func( ctx context.Context, promptName string, argument mcp.CompleteArgument, context mcp.CompleteContext, ) (*mcp.Completion, error) { return nil, fmt.Errorf("provider error") }), ), ) response := server.HandleMessage(context.Background(), []byte(message)) errorResp, ok := response.(mcp.JSONRPCError) assert.True(t, ok) assert.Equal(t, mcp.INTERNAL_ERROR, errorResp.Error.Code) })server/completion.go (2)
9-17: Parameter namecontextshadows the importedcontextpackage.The parameter name
context mcp.CompleteContextshadows thecontextpackage imported on line 4. While this works because thecontextpackage is only used forcontext.Context(the first parameter), it can cause confusion and potential issues if the method body needs to reference thecontextpackage directly.🔎 Rename parameter to avoid shadowing
type PromptCompletionProvider interface { // CompletePromptArgument provides completions for a prompt argument - CompletePromptArgument(ctx context.Context, promptName string, argument mcp.CompleteArgument, context mcp.CompleteContext) (*mcp.Completion, error) + CompletePromptArgument(ctx context.Context, promptName string, argument mcp.CompleteArgument, argContext mcp.CompleteContext) (*mcp.Completion, error) } type ResourceCompletionProvider interface { // CompleteResourceArgument provides completions for a resource template argument - CompleteResourceArgument(ctx context.Context, uri string, argument mcp.CompleteArgument, context mcp.CompleteContext) (*mcp.Completion, error) + CompleteResourceArgument(ctx context.Context, uri string, argument mcp.CompleteArgument, argContext mcp.CompleteContext) (*mcp.Completion, error) }
19-20: GoDoc comment is misleading.The comment says "DefaultCompletionProvider" but the struct is named
DefaultPromptCompletionProvider. As per coding guidelines, GoDoc comments should start with the identifier name.🔎 Fix GoDoc comments
-// DefaultCompletionProvider returns no completions (fallback) +// DefaultPromptCompletionProvider returns no completions for prompt arguments (fallback). type DefaultPromptCompletionProvider struct{}And for line 28:
-// DefaultResourceCompletionProvider returns no completions (fallback) +// DefaultResourceCompletionProvider returns no completions for resource arguments (fallback). type DefaultResourceCompletionProvider struct{}server/server.go (1)
241-253: Consider nil-guard for provider options.These options accept any
PromptCompletionProviderorResourceCompletionProvider, includingnil. If a caller passesnil,handleCompletewill panic when calling methods on the nil provider. Consider either:
- Documenting that
nilis not allowed, or- Adding a nil-guard that falls back to the default provider.
🔎 Optional: Add nil-guard
// WithPromptCompletionProvider sets a custom prompt completion provider func WithPromptCompletionProvider(provider PromptCompletionProvider) ServerOption { return func(s *MCPServer) { + if provider == nil { + return + } s.promptCompletionProvider = provider } } // WithResourceCompletionProvider sets a custom resource completion provider func WithResourceCompletionProvider(provider ResourceCompletionProvider) ServerOption { return func(s *MCPServer) { + if provider == nil { + return + } s.resourceCompletionProvider = provider } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
server/internal/gen/data.gois excluded by!**/gen/**
📒 Files selected for processing (10)
README.mdexamples/everything/id.goexamples/everything/main.gomcp/types.gomcp/types_test.goserver/completion.goserver/hooks.goserver/request_handler.goserver/server.goserver/server_test.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/everything/id.gomcp/types_test.goserver/server.goserver/hooks.goserver/completion.goexamples/everything/main.goserver/server_test.gomcp/types.goserver/request_handler.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/types_test.goserver/server_test.go
🧠 Learnings (6)
📚 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:
mcp/types_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:
mcp/types_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.goexamples/everything/main.goserver/server_test.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:
examples/everything/main.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:
examples/everything/main.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:
examples/everything/main.goserver/server_test.go
🧬 Code graph analysis (6)
mcp/types_test.go (1)
mcp/types.go (4)
CompleteParams(1166-1170)PromptReference(1250-1254)CompleteArgument(1218-1223)ResourceReference(1243-1247)
server/hooks.go (1)
mcp/types.go (3)
CompleteRequest(1159-1163)CompleteResult(1212-1215)MethodCompletionComplete(106-106)
server/completion.go (1)
mcp/types.go (3)
CompleteArgument(1218-1223)CompleteContext(1226-1228)Completion(1231-1240)
examples/everything/main.go (2)
server/server.go (3)
WithCompletions(396-400)WithPromptCompletionProvider(242-246)WithResourceCompletionProvider(249-253)mcp/types.go (3)
CompleteArgument(1218-1223)CompleteContext(1226-1228)Completion(1231-1240)
server/server_test.go (5)
mcp/types.go (6)
CompleteArgument(1218-1223)CompleteContext(1226-1228)Completion(1231-1240)JSONRPCError(380-384)JSONRPCResponse(373-377)Result(281-285)server/server.go (4)
NewMCPServer(403-439)WithCompletions(396-400)WithPromptCompletionProvider(242-246)WithResourceCompletionProvider(249-253)mcp/utils.go (1)
NewJSONRPCErrorDetails(138-144)client/transport/interface.go (1)
JSONRPCResponse(69-74)testdata/mockstdio_server.go (1)
JSONRPCResponse(21-26)
server/request_handler.go (3)
mcp/types.go (5)
MethodCompletionComplete(106-106)CompleteRequest(1159-1163)CompleteResult(1212-1215)METHOD_NOT_FOUND(408-408)INVALID_REQUEST(405-405)server/errors.go (1)
ErrUnsupported(10-10)server/server.go (1)
UnparsableMessageError(103-107)
🔇 Additional comments (16)
README.md (1)
778-888: Documentation for auto-completions looks good.The new section clearly explains:
- How to enable completions via
server.WithCompletions()- Provider interface implementations for prompts and resources
- Usage of
CompleteContextfor multi-argument completions- Response constraints (max 100 items, Total, HasMore)
The code examples align well with the interfaces defined in
server/completion.goand types inmcp/types.go.examples/everything/main.go (3)
73-75: Completion providers properly wired into server configuration.The example correctly demonstrates enabling completions and registering both prompt and resource completion providers, aligning with the new server options introduced in
server/server.go.
519-539: Prompt completion provider implementation looks correct.The
fallthroughon line 533 is intentional—when the prompt isCOMPLEXbut the argument is not"style", it falls through to return an empty completion. This is a valid pattern for the example.
544-580: Resource completion provider demonstrates paginated completions well.The implementation correctly handles:
- Empty input → suggestions 0-9 with
Total=maxIdandHasMore=true- Non-numeric input → empty suggestions (graceful fallback)
- Out-of-range input → empty suggestions
- Valid input → next-digit suggestions with proper
TotalandHasMoreThis provides a good reference implementation for paginated completion scenarios.
server/request_handler.go (1)
436-461: Completion request handling follows established patterns.The new
MethodCompletionCompletecase correctly:
- Checks capability support (
s.capabilities.completions == nil)- Unmarshals into
mcp.CompleteRequest- Injects HTTP headers
- Invokes pre/post hooks (
beforeComplete,afterComplete)- Delegates to
handleComplete- Returns appropriate JSON-RPC responses
This is generated code (per the header comment), and the implementation is consistent with other method handlers in the file.
mcp/types_test.go (1)
377-457: Test coverage for CompleteParams unmarshalling is solid.The table-driven test correctly validates:
PromptReferenceparsing withref/prompttypeResourceReferenceparsing withref/resourcetype- Error handling for unknown reference types
examples/everything/id.go (3)
5-15: Power function implementation is correct.The
powfunction uses exponentiation by squaring, which is an efficient O(log p) algorithm. The bitwise operations correctly handle the algorithm.
25-33: ID suggestion logic is correct for the intended use case.The
getIdSuggestionsfunction generates suggestions by appending digits 0-9 to the input number. The caller inresourceCompletionProvidervalidates the input withisIdInRangebefore calling this function, so out-of-range inputs are handled properly.
27-33: The code is compatible with the project. Line 29 usesfor i := range 10, a Go 1.22+ feature, and the range-over-int feature was added to Go 1.22. The project's go.mod specifiesgo 1.23.0, which exceeds this requirement, so there are no version compatibility concerns.server/hooks.go (2)
106-107: Completion hook types follow established patterns.The
OnBeforeCompleteFuncandOnAfterCompleteFunctype definitions are consistent with other hook types in the file, using*mcp.CompleteRequestand*mcp.CompleteResultas expected from the types defined inmcp/types.go.
666-692: Hook registration and invocation methods are correctly implemented.The methods follow the same pattern as other hook implementations:
AddBeforeComplete/AddAfterCompleteappend to the respective hook slicesbeforeCompletecallsbeforeAnyfirst, then invokes specific hooksafterCompletecallsonSuccessfirst, then invokes specific hooksThe nil checks are consistently placed after the generic hook calls, matching the pattern established by other methods like
beforeInitialize.server/server_test.go (1)
2870-2880: Well-structured adapter types for testing.These function type adapters follow the idiomatic Go pattern (similar to
http.HandlerFunc) for bridging function signatures to interface implementations, enabling concise inline provider definitions in tests.server/server.go (1)
178-179: Provider fields added without mutex protection.The
promptCompletionProviderandresourceCompletionProviderfields are accessed inhandleCompletewithout mutex protection. While providers are typically set during initialization viaServerOption, if they were to be modified after server creation (e.g., via a futureSetPromptCompletionProvidermethod), this could cause a data race.The current design is safe if providers are only set at construction time. Consider documenting this constraint or adding a dedicated mutex if runtime modification is expected in the future.
mcp/types.go (3)
1172-1209: Well-implemented polymorphic JSON unmarshalling.The
UnmarshalJSONcorrectly:
- Uses the
Aliaspattern to avoid infinite recursion- Defers
Refparsing usingjson.RawMessage- Uses a type peek to determine the concrete type
- Returns a descriptive error for unknown reference types
This enables servers to correctly route completion requests based on the discriminator field.
1217-1254: Well-structured completion types with proper documentation.The types are well-designed:
CompleteArgumentholds the name and value of the argument for completion matching.CompleteContextprovides already-resolved arguments via a map.Completioncorrectly usesomitemptyfor optionalTotalandHasMorefields, and documents the 100-item limit.ResourceReferenceandPromptReferencecorrectly require theTypefield (noomitempty) for JSON discrimination.
103-107: The spec URL is correct. Version 2025-11-25 is the latest MCP specification, released on November 25, 2025 (approximately three weeks ago), so the reference date is accurate and up-to-date.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/server.go (1)
241-253: Consider adding nil guards in the provider options.The options follow the established pattern and have proper GoDoc comments. However, if a user explicitly passes
nil, it would overwrite the default provider and cause a panic whenhandleCompleteinvokes the provider methods.This is a programming error by the user, but adding nil guards would be more defensive.
🔎 Optional nil guard
// WithPromptCompletionProvider sets a custom prompt completion provider func WithPromptCompletionProvider(provider PromptCompletionProvider) ServerOption { return func(s *MCPServer) { + if provider == nil { + return + } s.promptCompletionProvider = provider } } // WithResourceCompletionProvider sets a custom resource completion provider func WithResourceCompletionProvider(provider ResourceCompletionProvider) ServerOption { return func(s *MCPServer) { + if provider == nil { + return + } s.resourceCompletionProvider = provider } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/server.go
🧰 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:
server/server.go
🧠 Learnings (1)
📚 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.go
🧬 Code graph analysis (1)
server/server.go (3)
server/completion.go (4)
PromptCompletionProvider(9-12)ResourceCompletionProvider(14-17)DefaultPromptCompletionProvider(20-20)DefaultResourceCompletionProvider(29-29)mcp/utils.go (1)
ToBoolPtr(959-961)mcp/types.go (6)
CompleteRequest(1159-1163)CompleteResult(1212-1215)Completion(1231-1240)Params(212-212)PromptReference(1250-1254)ResourceReference(1243-1247)
🔇 Additional comments (7)
server/server.go (7)
178-179: LGTM! Completion provider fields are well-structured.The new fields follow Go naming conventions and are appropriately unexported. They're initialized with default implementations in
NewMCPServer, ensuring they're never nil during normal operation.
204-204: LGTM! Completions capability field follows established pattern.The
completions *boolfield is consistent with other capability fields likelogging,sampling, andelicitation, using a pointer to distinguish between unset and explicitly disabled.
395-400: LGTM! Completions option follows established patterns.The
WithCompletions()option is consistent with other capability-enabling options likeWithLogging()andWithElicitation(), usingmcp.ToBoolPtr(true)appropriately.
419-420: LGTM! Default provider initialization ensures robustness.Initializing the providers with default implementations ensures they're never nil, preventing potential panics and providing sensible no-op behavior out of the box.
422-430: LGTM! Explicit nil initialization clarifies default state.Explicitly setting all capabilities to
nilmakes the default (disabled) state clear and self-documenting.
802-804: LGTM! Completions capability advertisement follows the pattern.The capability check and advertisement follows the same pattern as other capabilities, using
&struct{}{}as a presence marker in the initialize response.
1563-1608: LGTM! The handleComplete function is well-implemented and addresses the previous nil-dereference concern.The function follows Go conventions and coding guidelines:
- Context as first parameter ✓
- Proper error handling with appropriate error codes ✓
- Thread-safe (read-only access to providers) ✓
The nil check at lines 1600-1603 correctly addresses the concern from the previous review about potential nil pointer dereference. If a provider returns
(nil, nil), the code now safely creates an emptyCompletioninstead of panicking.The switch statement properly routes to the appropriate provider based on reference type, with a sensible fallback for unknown types.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/server.go
🧰 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:
server/server.go
🧠 Learnings (1)
📚 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.go
🧬 Code graph analysis (1)
server/server.go (4)
server/completion.go (4)
PromptCompletionProvider(9-12)ResourceCompletionProvider(14-17)DefaultPromptCompletionProvider(20-20)DefaultResourceCompletionProvider(29-29)examples/custom_context/main.go (1)
MCPServer(100-102)mcp/utils.go (1)
ToBoolPtr(959-961)mcp/types.go (8)
CompleteRequest(1159-1163)CompleteResult(1212-1215)Completion(1231-1240)Params(212-212)PromptReference(1250-1254)ResourceReference(1243-1247)INVALID_PARAMS(411-411)INTERNAL_ERROR(414-414)
🔇 Additional comments (7)
server/server.go (7)
178-179: LGTM: Provider fields follow established patterns.The completion provider fields are consistent with the server's configuration pattern where options are applied during construction.
204-204: LGTM: Completions capability flag is consistent.The
completions *boolfield follows the same pattern as other capability flags likelogging,sampling, etc.
241-253: LGTM: Provider ServerOption functions follow conventions.The
WithPromptCompletionProviderandWithResourceCompletionProviderfunctions follow the establishedServerOptionpattern and have proper GoDoc comments.
395-400: LGTM: Completions capability enabler is consistent.The
WithCompletionsfunction follows the same pattern as other capability enablers likeWithLoggingandWithElicitation.
419-420: LGTM: Default providers ensure safe nil-free operation.Initializing both completion providers with default implementations is good defensive programming, ensuring the providers are never nil even when not explicitly configured.
802-804: LGTM: Completions capability advertisement is consistent.The capability advertisement follows the same pattern as other empty capabilities like logging and elicitation.
1600-1602: Past review issue resolved: nil completion now handled safely.The nil check for
completionat lines 1600-1602 resolves the previous review concern about potential nil pointer dereference. When the provider returns(nil, nil), the code now safely returns an emptyCompleteResultinstead of panicking.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/server.go (1)
1600-1602: Nil completion handling is defensive but treats nil as valid.The nil check prevents the panic that was previously flagged. However, a provider returning
(nil, nil)is likely a bug. The current code treats it as "no completions available" by returning an empty result. Consider whether this should log a warning or return an internal error to make provider bugs more visible during development.Based on learnings: The
capabilitiesfield is a struct value, not a pointer, so providers (also initialized inNewMCPServer) similarly should never be nil. However, custom providers might still have bugs.💡 Optional: Add comment explaining defensive nil handling
if err != nil { return nil, &requestError{ id: id, code: mcp.INTERNAL_ERROR, err: err, } } + // Defensive nil check: default providers always return non-nil completions, + // but custom providers might erroneously return nil. Treat as empty result. if completion == nil { return &mcp.CompleteResult{}, nil }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/server.go
🧰 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:
server/server.go
🧠 Learnings (1)
📚 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.go
🔇 Additional comments (4)
server/server.go (4)
178-179: LGTM: Completion provider fields are correctly initialized.The provider fields are properly initialized with default implementations in
NewMCPServer, ensuring they're never nil. Since these are set during construction and only read during request handling, no synchronization is required.Also applies to: 419-420
241-253: LGTM: ServerOption functions follow established patterns.The completion provider options and capability enabler follow the same pattern as existing server options, with proper GoDoc comments and consistent naming.
Also applies to: 395-400
204-204: LGTM: Completions capability integration is consistent.The completions capability field and advertisement logic follow the same pattern as other simple capabilities (logging, sampling, roots, etc.).
Also applies to: 802-804
1563-1607: handleComplete implementation looks solid.The handler correctly:
- Routes completion requests to the appropriate provider based on reference type
- Returns
INVALID_REQUESTfor unknown reference types (addressing previous feedback)- Handles provider errors with
INTERNAL_ERROR- Guards against nil completion results to prevent panics
The implementation follows Go conventions and integrates well with the existing request handling pattern.
…REQUEST to match marshal error behavior
2 participants
Description
Implements the specification for auto-completion, allowing servers to provide contextual suggestions for prompt and resource template arguments.
PromptCompletionProviderandResourceCompletionProviderinterfaces inserver/completion.gocompletion/completerequest handler in the serverWithCompletions(),WithPromptCompletionProvider(), andWithResourceCompletionProvider()server optionsCompleteParams.ArgumentintoCompleteArgumentCompleteResult.CompletionintoCompletionUnmarshalJSONforCompleteParamsto handlePromptReferenceandResourceReferencepolymorphismexamples/everythingFixes #657
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Very small unlikely breaking change
Due to some nested structs being extracted into its own named type, there is a very small possibility that it will break compilation on a certain syntax:
This should be a rare occurrence as it is an unusual way to assign a nested struct value.
Invalid ref type response
Due to the polymorphism logic contained in the
CompleteParams.UnmarshalJSON, the server will respond withINVALID_REQUESTinstead ofINVALID_PARAMSwhen theref.typeis not "ref/prompt" or "ref/resource".Summary by CodeRabbit
New Features
Documentation
Examples
Tests
✏️ Tip: You can customize this high-level summary in your review settings.