From e2bbd8002de4f44a75e6bf77a022521987f11633 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Wed, 28 May 2025 04:02:00 +0000 Subject: [PATCH] internal/mcp: add JSON conformance tests, and fix some bugs Add the beginnings of some testdata-driven conformance tests that check JSON-level compatibility of SDK messages, and fix some bugs in our JSON message handling: - "params" can be missing - "_meta" should not be null - lists of tools/prompts/resources should not be null In order to implement this, we need to be able to know when the server is done processing messages. In general, this is a hard problem; for gopls we do this with a complicated system of progress reporting and accounting. However, the new synctest package makes this trivial, as we can call synctest.Wait to detect when the server is idle. Therefore, this test requires 1.25 (it uses synctest.Test, added in 1.25, and so can't run at 1.24 with GOEXPERIMENT=synctest). Change-Id: I97fa28b56868340ece266a58e1eb3c3bf8cfe0e9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/676538 LUCI-TryBot-Result: Go LUCI Reviewed-by: Jonathan Amsterdam Auto-Submit: Robert Findley --- internal/jsonrpc2/messages.go | 13 + mcp/conformance_test.go | 280 ++++++++++++++++++ mcp/mcp_test.go | 111 ++++--- mcp/server.go | 7 + mcp/shared.go | 8 +- mcp/testdata/conformance/server/prompts.txtar | 74 +++++ mcp/testdata/conformance/server/tools.txtar | 93 ++++++ 7 files changed, 535 insertions(+), 51 deletions(-) create mode 100644 mcp/conformance_test.go create mode 100644 mcp/testdata/conformance/server/prompts.txtar create mode 100644 mcp/testdata/conformance/server/tools.txtar diff --git a/internal/jsonrpc2/messages.go b/internal/jsonrpc2/messages.go index e10abfa..5db0dbb 100644 --- a/internal/jsonrpc2/messages.go +++ b/internal/jsonrpc2/messages.go @@ -146,6 +146,19 @@ func EncodeMessage(msg Message) ([]byte, error) { return data, nil } +// EncodeIndent is like EncodeMessage, but honors indents. +// TODO(rfindley): refactor so that this concern is handled independently. +// Perhaps we should pass in a json.Encoder? +func EncodeIndent(msg Message, prefix, indent string) ([]byte, error) { + wire := wireCombined{VersionTag: wireVersion} + msg.marshal(&wire) + data, err := json.MarshalIndent(&wire, prefix, indent) + if err != nil { + return data, fmt.Errorf("marshaling jsonrpc message: %w", err) + } + return data, nil +} + func DecodeMessage(data []byte) (Message, error) { msg := wireCombined{} if err := json.Unmarshal(data, &msg); err != nil { diff --git a/mcp/conformance_test.go b/mcp/conformance_test.go new file mode 100644 index 0000000..b2e4146 --- /dev/null +++ b/mcp/conformance_test.go @@ -0,0 +1,280 @@ +// Copyright 2025 The Go MCP SDK Authors. All rights reserved. +// Use of this source code is governed by an MIT-style +// license that can be found in the LICENSE file. + +//go:build go1.25 + +package mcp + +import ( + "bytes" + "encoding/json" + "errors" + "flag" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "testing/synctest" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/modelcontextprotocol/go-sdk/internal/jsonrpc2" + "golang.org/x/tools/txtar" +) + +var update = flag.Bool("update", false, "if set, update conformance test data") + +// A conformance test checks JSON-level conformance of a test server or client. +// This allows us to confirm that we can handle the input or output of other +// SDKs, even if they behave differently at the JSON level (for example, have +// different behavior with respect to optional fields). +// +// The client and server fields hold an encoded sequence of JSON-RPC messages. +// +// For server tests, the client messages are a sequence of messages to be sent +// from the (synthetic) client and the server messages are the expected +// messages to be received from the real server. +// +// For client tests, it's the other way around: server messages are synthetic, +// and client messages are expected from the real client. +// +// Conformance tests are loaded from txtar-encoded testdata files. Run the test +// with -update to have the test runner update the expected output, which may +// be client or server depending on the perspective of the test. +type conformanceTest struct { + name string // test name + path string // path to test file + archive *txtar.Archive // raw archive, for updating + tools, prompts, resources []string // named features to include + client []jsonrpc2.Message // client messages + server []jsonrpc2.Message // server messages +} + +// TODO(rfindley): add client conformance tests. + +func TestServerConformance(t *testing.T) { + var tests []*conformanceTest + dir := filepath.Join("testdata", "conformance", "server") + if err := filepath.WalkDir(dir, func(path string, _ fs.DirEntry, err error) error { + if err != nil { + return err + } + if strings.HasSuffix(path, ".txtar") { + test, err := loadConformanceTest(dir, path) + if err != nil { + return fmt.Errorf("%s: %v", path, err) + } + tests = append(tests, test) + } + return nil + }); err != nil { + t.Fatal(err) + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // We use synctest here because in general, there is no way to know when the + // server is done processing any notifications. As long as our server doesn't + // do background work, synctest provides an easy way for us to detect when the + // server is done processing. + // + // By comparison, gopls has a complicated framework based on progress + // reporting and careful accounting to detect when all 'expected' work + // on the server is complete. + synctest.Test(t, func(t *testing.T) { + runServerTest(t, test) + }) + }) + } +} + +// runServerTest runs the server conformance test. +// It must be executed in a synctest bubble. +func runServerTest(t *testing.T, test *conformanceTest) { + ctx := t.Context() + // Construct the server based on features listed in the test. + s := NewServer("testServer", "v1.0.0", nil) + add(tools, s.AddTools, test.tools...) + add(prompts, s.AddPrompts, test.prompts...) + add(resources, s.AddResources, test.resources...) + + // Connect the server, and connect the client stream, + // but don't connect an actual client. + cTransport, sTransport := NewInMemoryTransports() + ss, err := s.Connect(ctx, sTransport) + if err != nil { + t.Fatal(err) + } + cStream, err := cTransport.Connect(ctx) + if err != nil { + t.Fatal(err) + } + + // Collect server messages asynchronously. + var wg sync.WaitGroup + var ( + serverMessages []jsonrpc2.Message + serverErr error // abnormal failure of the server stream + ) + wg.Add(1) + go func() { + defer wg.Done() + for { + msg, _, err := cStream.Read(ctx) + if err != nil { + // TODO(rfindley): we don't document (or want to document) that the in + // memory transports use a net.Pipe. How can users detect this failure? + // Should we promote it to EOF? + if !errors.Is(err, io.ErrClosedPipe) { + serverErr = err + } + break + } + serverMessages = append(serverMessages, msg) + } + }() + + // Write client messages to the stream. + for _, msg := range test.client { + if _, err := cStream.Write(ctx, msg); err != nil { + t.Fatalf("Write failed: %v", err) + } + } + + // Before closing the stream, wait for all messages to be processed. + synctest.Wait() + if err := cStream.Close(); err != nil { + t.Fatalf("Stream.Close failed: %v", err) + } + + ss.Wait() + wg.Wait() + if serverErr != nil { + t.Fatalf("reading server messages failed: %v", serverErr) + } + + // Handle server output. If -update is set, write the 'server' file. + // Otherwise, compare with expected. + if *update { + arch := &txtar.Archive{ + Comment: test.archive.Comment, + } + var buf bytes.Buffer + for _, msg := range serverMessages { + data, err := jsonrpc2.EncodeIndent(msg, "", "\t") + if err != nil { + t.Fatalf("jsonrpc2.EncodeIndent failed: %v", err) + } + buf.Write(data) + buf.WriteByte('\n') + } + serverFile := txtar.File{Name: "server", Data: buf.Bytes()} + seenServer := false // replace or append the 'server' file + for _, f := range test.archive.Files { + if f.Name == "server" { + seenServer = true + arch.Files = append(arch.Files, serverFile) + } else { + arch.Files = append(arch.Files, f) + } + } + if !seenServer { + arch.Files = append(arch.Files, serverFile) + } + if err := os.WriteFile(test.path, txtar.Format(arch), 0666); err != nil { + t.Fatalf("os.WriteFile(%q) failed: %v", test.path, err) + } + } else { + // jsonrpc2.Messages are not comparable, so we instead compare lines of JSON. + transform := cmpopts.AcyclicTransformer("toJSON", func(msg jsonrpc2.Message) []string { + encoded, err := jsonrpc2.EncodeIndent(msg, "", "\t") + if err != nil { + t.Fatal(err) + } + return strings.Split(string(encoded), "\n") + }) + if diff := cmp.Diff(test.server, serverMessages, transform); diff != "" { + t.Errorf("Mismatching server messages (-want +got):\n%s", diff) + } + } +} + +// loadConformanceTest loads one conformance test from the given path contained +// in the root dir. +func loadConformanceTest(dir, path string) (*conformanceTest, error) { + content, err := os.ReadFile(path) + if err != nil { + return nil, err + } + test := &conformanceTest{ + name: strings.TrimPrefix(path, dir+string(filepath.Separator)), + path: path, + archive: txtar.Parse(content), + } + if len(test.archive.Files) == 0 { + return nil, fmt.Errorf("txtar archive %q has no '-- filename --' sections", path) + } + + // decodeMessages loads JSON-RPC messages from the archive file. + decodeMessages := func(data []byte) ([]jsonrpc2.Message, error) { + dec := json.NewDecoder(bytes.NewReader(data)) + var res []jsonrpc2.Message + for dec.More() { + var raw json.RawMessage + if err := dec.Decode(&raw); err != nil { + return nil, err + } + m, err := jsonrpc2.DecodeMessage(raw) + if err != nil { + return nil, err + } + res = append(res, m) + } + return res, nil + } + // loadFeatures loads lists of named features from the archive file. + loadFeatures := func(data []byte) []string { + var feats []string + for line := range strings.SplitSeq(string(data), "\n") { + if f := strings.TrimSpace(line); f != "" { + feats = append(feats, f) + } + } + return feats + } + + seen := make(map[string]bool) // catch accidentally duplicate files + for _, f := range test.archive.Files { + if seen[f.Name] { + return nil, fmt.Errorf("duplicate file name %q", f.Name) + } + seen[f.Name] = true + switch f.Name { + case "tools": + test.tools = loadFeatures(f.Data) + case "prompts": + test.prompts = loadFeatures(f.Data) + case "resource": + test.resources = loadFeatures(f.Data) + case "client": + test.client, err = decodeMessages(f.Data) + if err != nil { + return nil, fmt.Errorf("txtar archive %q contains bad -- client -- section: %v", path, err) + } + case "server": + test.server, err = decodeMessages(f.Data) + if err != nil { + return nil, fmt.Errorf("txtar archive %q contains bad -- server -- section: %v", path, err) + } + default: + return nil, fmt.Errorf("txtar archive %q contains unexpected file %q", path, f.Name) + } + } + + return test, nil +} diff --git a/mcp/mcp_test.go b/mcp/mcp_test.go index 5549d42..52e5f18 100644 --- a/mcp/mcp_test.go +++ b/mcp/mcp_test.go @@ -58,34 +58,10 @@ func TestEndToEnd(t *testing.T) { InitializedHandler: func(context.Context, *ServerSession, *InitializedParams) { notificationChans["initialized"] <- 0 }, RootsListChangedHandler: func(context.Context, *ServerSession, *RootsListChangedParams) { notificationChans["roots"] <- 0 }, } - s := NewServer("testServer", "v1.0.0", sopts) - - // The 'greet' tool says hi. - s.AddTools(NewTool("greet", "say hi", sayHi)) - - // The 'fail' tool returns this error. - failure := errors.New("mcp failure") - s.AddTools( - NewTool("fail", "just fail", func(context.Context, *ServerSession, struct{}) ([]*Content, error) { - return nil, failure - }), - ) - - s.AddPrompts( - NewPrompt("code_review", "do a code review", - func(_ context.Context, _ *ServerSession, params struct{ Code string }, _ *GetPromptParams) (*GetPromptResult, error) { - return &GetPromptResult{ - Description: "Code review prompt", - Messages: []*PromptMessage{ - {Role: "user", Content: NewTextContent("Please review the following code: " + params.Code)}, - }, - }, nil - }), - NewPrompt("fail", "", func(_ context.Context, _ *ServerSession, args struct{}, _ *GetPromptParams) (*GetPromptResult, error) { - return nil, failure - }), - ) + add(tools, s.AddTools, "greet", "fail") + add(prompts, s.AddPrompts, "code_review", "fail") + add(resources, s.AddResources, "info.txt", "fail.txt") // Connect the server. ss, err := s.Connect(ctx, st) @@ -167,8 +143,8 @@ func TestEndToEnd(t *testing.T) { t.Errorf("prompts/get 'code_review' mismatch (-want +got):\n%s", diff) } - if _, err := cs.GetPrompt(ctx, &GetPromptParams{Name: "fail"}); err == nil || !strings.Contains(err.Error(), failure.Error()) { - t.Errorf("fail returned unexpected error: got %v, want containing %v", err, failure) + if _, err := cs.GetPrompt(ctx, &GetPromptParams{Name: "fail"}); err == nil || !strings.Contains(err.Error(), errTestFailure.Error()) { + t.Errorf("fail returned unexpected error: got %v, want containing %v", err, errTestFailure) } s.AddPrompts(&ServerPrompt{Prompt: &Prompt{Name: "T"}}) @@ -227,7 +203,7 @@ func TestEndToEnd(t *testing.T) { } wantFail := &CallToolResult{ IsError: true, - Content: []*Content{{Type: "text", Text: failure.Error()}}, + Content: []*Content{{Type: "text", Text: errTestFailure.Error()}}, } if diff := cmp.Diff(wantFail, gotFail); diff != "" { t.Errorf("tools/call 'fail' mismatch (-want +got):\n%s", diff) @@ -243,27 +219,12 @@ func TestEndToEnd(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("TODO: fix for Windows") } - resource1 := &Resource{ - Name: "public", - MIMEType: "text/plain", - URI: "file:///info.txt", - } - resource2 := &Resource{ - Name: "public", // names are not unique IDs - MIMEType: "text/plain", - URI: "file:///nonexistent.txt", - } - - readHandler := s.FileResourceHandler("testdata/files") - s.AddResources( - &ServerResource{resource1, readHandler}, - &ServerResource{resource2, readHandler}) - + wantResources := []*Resource{resource2, resource1} lrres, err := cs.ListResources(ctx, nil) if err != nil { t.Fatal(err) } - if diff := cmp.Diff([]*Resource{resource1, resource2}, lrres.Resources); diff != "" { + if diff := cmp.Diff(wantResources, lrres.Resources); diff != "" { t.Errorf("resources/list mismatch (-want, +got):\n%s", diff) } @@ -272,7 +233,7 @@ func TestEndToEnd(t *testing.T) { mimeType string // "": not found; "text/plain": resource; "text/template": template }{ {"file:///info.txt", "text/plain"}, - {"file:///nonexistent.txt", ""}, + {"file:///fail.txt", ""}, // TODO(jba): add resource template cases when we implement them } { rres, err := cs.ReadResource(ctx, &ReadResourceParams{URI: tt.uri}) @@ -416,6 +377,60 @@ func TestEndToEnd(t *testing.T) { } } +// Registry of values to be referenced in tests. +var ( + errTestFailure = errors.New("mcp failure") + + tools = map[string]*ServerTool{ + "greet": NewTool("greet", "say hi", sayHi), + "fail": NewTool("fail", "just fail", func(context.Context, *ServerSession, struct{}) ([]*Content, error) { + return nil, errTestFailure + }), + } + + prompts = map[string]*ServerPrompt{ + "code_review": NewPrompt("code_review", "do a code review", + func(_ context.Context, _ *ServerSession, params struct{ Code string }, _ *GetPromptParams) (*GetPromptResult, error) { + return &GetPromptResult{ + Description: "Code review prompt", + Messages: []*PromptMessage{ + {Role: "user", Content: NewTextContent("Please review the following code: " + params.Code)}, + }, + }, nil + }), + "fail": NewPrompt("fail", "", func(_ context.Context, _ *ServerSession, args struct{}, _ *GetPromptParams) (*GetPromptResult, error) { + return nil, errTestFailure + }), + } + + resource1 = &Resource{ + Name: "public", + MIMEType: "text/plain", + URI: "file:///info.txt", + } + resource2 = &Resource{ + Name: "public", // names are not unique IDs + MIMEType: "text/plain", + URI: "file:///fail.txt", + } + readHandler = fileResourceHandler("testdata/files") + resources = map[string]*ServerResource{ + "info.txt": {resource1, readHandler}, + "fail.txt": {resource2, readHandler}, + } +) + +// Add calls the given function to add the named features. +func add[T any](m map[string]T, add func(...T), names ...string) { + for _, name := range names { + feat, ok := m[name] + if !ok { + panic("missing feature " + name) + } + add(feat) + } +} + // errorCode returns the code associated with err. // If err is nil, it returns 0. // If there is no code, it returns -1. diff --git a/mcp/server.go b/mcp/server.go index f5cde12..6d33234 100644 --- a/mcp/server.go +++ b/mcp/server.go @@ -189,6 +189,7 @@ func (s *Server) listPrompts(_ context.Context, _ *ServerSession, params *ListPr } res := new(ListPromptsResult) res.NextCursor = nextCursor + res.Prompts = []*Prompt{} // avoid JSON null for _, p := range prompts { res.Prompts = append(res.Prompts, p.Prompt) } @@ -219,6 +220,7 @@ func (s *Server) listTools(_ context.Context, _ *ServerSession, params *ListTool } res := new(ListToolsResult) res.NextCursor = nextCursor + res.Tools = []*Tool{} // avoid JSON null for _, t := range tools { res.Tools = append(res.Tools, t.Tool) } @@ -248,6 +250,7 @@ func (s *Server) listResources(_ context.Context, _ *ServerSession, params *List } res := new(ListResourcesResult) res.NextCursor = nextCursor + res.Resources = []*Resource{} // avoid JSON null for _, r := range resources { res.Resources = append(res.Resources, r.Resource) } @@ -296,6 +299,10 @@ func (s *Server) readResource(ctx context.Context, ss *ServerSession, params *Re // are always caught. Go 1.24 and above also protects against symlink-based attacks, // where symlinks under dir lead out of the tree. func (s *Server) FileResourceHandler(dir string) ResourceHandler { + return fileResourceHandler(dir) +} + +func fileResourceHandler(dir string) ResourceHandler { // Convert dir to an absolute path. dirFilepath, err := filepath.Abs(dir) if err != nil { diff --git a/mcp/shared.go b/mcp/shared.go index fc7ab96..d33ec6d 100644 --- a/mcp/shared.go +++ b/mcp/shared.go @@ -105,8 +105,10 @@ func newMethodInfo[S ClientSession | ServerSession, P Params, R Result](d typedM return methodInfo[S]{ unmarshalParams: func(m json.RawMessage) (Params, error) { var p P - if err := json.Unmarshal(m, &p); err != nil { - return nil, fmt.Errorf("unmarshaling %q into a %T: %w", m, p, err) + if m != nil { + if err := json.Unmarshal(m, &p); err != nil { + return nil, fmt.Errorf("unmarshaling %q into a %T: %w", m, p, err) + } } return p, nil }, @@ -200,7 +202,7 @@ func (m Meta) MarshalJSON() ([]byte, error) { // If ProgressToken is nil, accept Data["progressToken"]. We can't call marshalStructWithMap // in that case because it will complain about duplicate fields. (We'd have to // make it much smarter to avoid that; not worth it.) - if m.ProgressToken == nil { + if m.ProgressToken == nil && len(m.Data) > 0 { return json.Marshal(m.Data) } return marshalStructWithMap((*metaSansMethods)(&m), "Data") diff --git a/mcp/testdata/conformance/server/prompts.txtar b/mcp/testdata/conformance/server/prompts.txtar new file mode 100644 index 0000000..0ef0cdc --- /dev/null +++ b/mcp/testdata/conformance/server/prompts.txtar @@ -0,0 +1,74 @@ +Check behavior of a server with just prompts. + +Fixed bugs: +- empty tools lists should not be returned as 'null' + +-- prompts -- +code_review + +-- client -- +{ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": { "name": "ExampleClient", "version": "1.0.0" } + } +} +{ "jsonrpc": "2.0", "id": 2, "method": "tools/list" } +{ "jsonrpc": "2.0", "id": 4, "method": "prompts/list" } +-- server -- +{ + "jsonrpc": "2.0", + "id": 1, + "result": { + "_meta": {}, + "capabilities": { + "completions": {}, + "logging": {}, + "prompts": { + "listChanged": true + }, + "resources": { + "listChanged": true + }, + "tools": { + "listChanged": true + } + }, + "protocolVersion": "2024-11-05", + "serverInfo": { + "name": "testServer", + "version": "v1.0.0" + } + } +} +{ + "jsonrpc": "2.0", + "id": 2, + "result": { + "_meta": {}, + "tools": [] + } +} +{ + "jsonrpc": "2.0", + "id": 4, + "result": { + "_meta": {}, + "prompts": [ + { + "arguments": [ + { + "name": "Code", + "required": true + } + ], + "description": "do a code review", + "name": "code_review" + } + ] + } +} diff --git a/mcp/testdata/conformance/server/tools.txtar b/mcp/testdata/conformance/server/tools.txtar new file mode 100644 index 0000000..039e08d --- /dev/null +++ b/mcp/testdata/conformance/server/tools.txtar @@ -0,0 +1,93 @@ +Check behavior of a server with just tools. + +Fixed bugs: +- "tools/list" can have missing params +- "_meta" should not be nil +- empty resource or prompts should not be returned as 'null' + +-- tools -- +greet + +-- client -- +{ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": { "name": "ExampleClient", "version": "1.0.0" } + } +} +{ "jsonrpc": "2.0", "id": 2, "method": "tools/list" } +{ "jsonrpc": "2.0", "id": 3, "method": "resources/list" } +{ "jsonrpc": "2.0", "id": 4, "method": "prompts/list" } +-- server -- +{ + "jsonrpc": "2.0", + "id": 1, + "result": { + "_meta": {}, + "capabilities": { + "completions": {}, + "logging": {}, + "prompts": { + "listChanged": true + }, + "resources": { + "listChanged": true + }, + "tools": { + "listChanged": true + } + }, + "protocolVersion": "2024-11-05", + "serverInfo": { + "name": "testServer", + "version": "v1.0.0" + } + } +} +{ + "jsonrpc": "2.0", + "id": 2, + "result": { + "_meta": {}, + "tools": [ + { + "description": "say hi", + "inputSchema": { + "type": "object", + "required": [ + "Name" + ], + "properties": { + "Name": { + "type": "string" + } + }, + "additionalProperties": { + "not": {} + } + }, + "name": "greet" + } + ] + } +} +{ + "jsonrpc": "2.0", + "id": 3, + "result": { + "_meta": {}, + "resources": [] + } +} +{ + "jsonrpc": "2.0", + "id": 4, + "result": { + "_meta": {}, + "prompts": [] + } +}