Browse Source

Add contract tests for the MCP wire contract

Pin the parts of tools/list and tools/call that strict pydantic-based
clients validate:

- TestWireSchemasAreObjects: every declared inputSchema/outputSchema has
  wire type "object" (the rule Hermes tripped over).
- TestToolArgumentContract: tool names, argument names (camelCase such
  as isPublished), and the required/optional split are a frozen API.
- TestStructuredContentMatchesDeclaredSchema: every tool that declares
  an outputSchema delivers structuredContent that validates against it;
  an empty page list is {"pages": []}, never a bare null.
- TestNullPageHasNoStructuredContent: a missing page is text "null"
  with no structuredContent, preserving the original server's semantic.

All schema assertions run against re-marshaled wire JSON, matching what
strict clients validate. The fake wiki gains an emptyList switch.
Paul Klumpp 6 ngày trước cách đây
mục cha
commit
ebd76a9c2c
2 tập tin đã thay đổi với 279 bổ sung và 38 xóa
  1. 271 0
      internal/server/contract_test.go
  2. 8 38
      internal/server/tools_test.go

+ 271 - 0
internal/server/contract_test.go

@@ -0,0 +1,271 @@
+// Contract tests: these pin the MCP wire contract of this server — the parts
+// of a tools/list and tools/call response that strict clients (e.g.
+// pydantic-based MCP SDKs) validate on every connection.
+//
+// Background: get_all_pages once returned a bare []PageListItem. The SDK then
+// inferred outputSchema.type ["null","array"], which violates the MCP spec
+// (type must be the string "object") and made strict clients reject the whole
+// tools/list response with a ValidationError — the server connected but
+// silently offered no tools. These tests keep that class of bug out.
+//
+// All schema assertions run against the wire format (re-marshaled JSON), not
+// against Go structs, because the wire format is what strict clients see.
+package server
+
+import (
+	"context"
+	"encoding/json"
+	"slices"
+	"strings"
+	"testing"
+
+	"github.com/google/jsonschema-go/jsonschema"
+	"github.com/modelcontextprotocol/go-sdk/mcp"
+)
+
+// wireType returns the wire `type` value of a JSON schema as raw JSON, so the
+// string "object" and the invalid-ish array ["null","array"] are
+// distinguishable. Returns nil if the schema has no type.
+func wireType(t *testing.T, schema any) json.RawMessage {
+	t.Helper()
+	if schema == nil {
+		return nil
+	}
+	raw, err := json.Marshal(schema)
+	if err != nil {
+		t.Fatalf("marshal schema: %v", err)
+	}
+	var out struct {
+		Type json.RawMessage `json:"type"`
+	}
+	if err := json.Unmarshal(raw, &out); err != nil {
+		t.Fatalf("unmarshal schema: %v", err)
+	}
+	return out.Type
+}
+
+func listedTools(t *testing.T, cs *mcp.ClientSession) []*mcp.Tool {
+	t.Helper()
+	res, err := cs.ListTools(context.Background(), nil)
+	if err != nil {
+		t.Fatalf("ListTools() error = %v", err)
+	}
+	if len(res.Tools) == 0 {
+		t.Fatal("ListTools() returned no tools")
+	}
+	return res.Tools
+}
+
+func requireObjectSchemas(t *testing.T, tool *mcp.Tool) {
+	t.Helper()
+	if tool.InputSchema == nil {
+		t.Errorf("tool %q has no inputSchema", tool.Name)
+	} else if got := wireType(t, tool.InputSchema); string(got) != `"object"` {
+		t.Errorf("tool %q inputSchema.type = %s, want \"object\"", tool.Name, got)
+	}
+	if tool.OutputSchema != nil {
+		if got := wireType(t, tool.OutputSchema); string(got) != `"object"` {
+			t.Errorf("tool %q outputSchema.type = %s, want \"object\"", tool.Name, got)
+		}
+	}
+}
+
+// TestWireSchemasAreObjects pins the rule that broke Hermes: every declared
+// schema must have type "object" on the wire. inputSchema must always be an
+// object; outputSchema must be an object whenever it is declared at all
+// (absent outputSchema is optional per the MCP spec). Uses an edit-enabled
+// session so all six tools are covered.
+func TestWireSchemasAreObjects(t *testing.T) {
+	cs, _ := newTestSession(t, true)
+
+	for _, tool := range listedTools(t, cs) {
+		t.Run(tool.Name, func(t *testing.T) {
+			requireObjectSchemas(t, tool)
+		})
+	}
+}
+
+// toolArgContract pins the public tool surface: names and argument names are a
+// stable API (camelCase included), and required/optional must not drift.
+var toolArgContract = map[string]struct {
+	required []string
+	optional []string
+}{
+	"search_pages":     {required: []string{"query"}, optional: []string{"locale", "path"}},
+	"get_page_by_id":   {required: []string{"id"}},
+	"get_page_by_path": {required: []string{"locale", "path"}},
+	"get_all_pages":    {optional: []string{"limit", "locale", "tags"}},
+	"create_page": {
+		required: []string{"content", "description", "path", "title"},
+		optional: []string{"editor", "isPrivate", "isPublished", "locale", "tags"},
+	},
+	"edit_page": {
+		required: []string{"content", "description", "id", "path", "title"},
+		optional: []string{"editor", "isPrivate", "isPublished", "locale", "tags"},
+	},
+}
+
+func sortedClone(values []string) []string {
+	out := slices.Clone(values)
+	slices.Sort(out)
+	return out
+}
+
+// TestToolArgumentContract pins tool names and argument names (including
+// camelCase such as isPublished) plus the required/optional split of every
+// tool. A mismatch means the public API changed and README.md,
+// AGENTS.md, and every client integration need a conscious update.
+func TestToolArgumentContract(t *testing.T) {
+	cs, _ := newTestSession(t, true)
+
+	tools := listedTools(t, cs)
+	seen := map[string]bool{}
+	for _, tool := range tools {
+		seen[tool.Name] = true
+		want, ok := toolArgContract[tool.Name]
+		if !ok {
+			t.Errorf("unexpected tool %q: not in the argument contract; update toolArgContract", tool.Name)
+			continue
+		}
+		raw, err := json.Marshal(tool.InputSchema)
+		if err != nil {
+			t.Fatalf("marshal inputSchema of %q: %v", tool.Name, err)
+		}
+		var schema struct {
+			Properties map[string]json.RawMessage `json:"properties"`
+			Required   []string                   `json:"required"`
+		}
+		if err := json.Unmarshal(raw, &schema); err != nil {
+			t.Fatalf("unmarshal inputSchema of %q: %v", tool.Name, err)
+		}
+
+		var optional []string
+		for name := range schema.Properties {
+			if !slices.Contains(schema.Required, name) {
+				optional = append(optional, name)
+			}
+		}
+		if got, want := sortedClone(schema.Required), sortedClone(want.required); !slices.Equal(got, want) {
+			t.Errorf("tool %q required args = %v, want %v", tool.Name, got, want)
+		}
+		if got, want := sortedClone(optional), sortedClone(want.optional); !slices.Equal(got, want) {
+			t.Errorf("tool %q optional args = %v, want %v", tool.Name, got, want)
+		}
+	}
+	for name := range toolArgContract {
+		if !seen[name] {
+			t.Errorf("tool %q is in the contract but not listed by the server", name)
+		}
+	}
+}
+
+// validateStructuredContent re-marshals the declared output schema, resolves
+// it, and validates a real structuredContent against it — the same
+// self-consistency check a strict client performs.
+func validateStructuredContent(t *testing.T, tool *mcp.Tool, structured any) {
+	t.Helper()
+	raw, err := json.Marshal(tool.OutputSchema)
+	if err != nil {
+		t.Fatalf("marshal outputSchema of %q: %v", tool.Name, err)
+	}
+	var schema jsonschema.Schema
+	if err := json.Unmarshal(raw, &schema); err != nil {
+		t.Fatalf("unmarshal outputSchema of %q: %v", tool.Name, err)
+	}
+	resolved, err := schema.Resolve(nil)
+	if err != nil {
+		t.Fatalf("resolve outputSchema of %q: %v", tool.Name, err)
+	}
+	if err := resolved.Validate(structured); err != nil {
+		t.Errorf("structuredContent of %q does not match its declared outputSchema: %v", tool.Name, err)
+	}
+}
+
+func findTool(t *testing.T, tools []*mcp.Tool, name string) *mcp.Tool {
+	t.Helper()
+	for _, tool := range tools {
+		if tool.Name == name {
+			return tool
+		}
+	}
+	t.Fatalf("tool %q not listed", name)
+	return nil
+}
+
+// TestStructuredContentMatchesDeclaredSchema calls every tool that declares an
+// outputSchema and validates the returned structuredContent against that
+// schema. What we announce in tools/list must be what we deliver in
+// tools/call.
+func TestStructuredContentMatchesDeclaredSchema(t *testing.T) {
+	cs, fake := newTestSession(t, true)
+	tools := listedTools(t, cs)
+
+	cases := []struct {
+		name string
+		args map[string]any
+	}{
+		{name: "search_pages", args: map[string]any{"query": "mcp"}},
+		{name: "get_all_pages"},
+		{name: "create_page", args: map[string]any{
+			"title": "New", "content": "body", "description": "desc", "path": "/new",
+		}},
+		{name: "edit_page", args: map[string]any{
+			"id": 1, "title": "Edit", "content": "body", "description": "desc", "path": "/edit",
+		}},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			tool := findTool(t, tools, tc.name)
+			if tool.OutputSchema == nil {
+				t.Fatalf("tool %q declares no outputSchema", tc.name)
+			}
+			res := callTool(t, cs, tc.name, tc.args)
+			if res.IsError {
+				t.Fatalf("IsError = true: %s", toolText(t, res))
+			}
+			if res.StructuredContent == nil {
+				t.Fatalf("structuredContent = nil, want a value matching the declared outputSchema")
+			}
+			validateStructuredContent(t, tool, res.StructuredContent)
+		})
+	}
+
+	// get_all_pages with an empty result must deliver {"pages": []}, never a
+	// bare null: a nil Go slice would serialize as JSON null and violate the
+	// required "pages" property of the declared schema.
+	fake.emptyList = true
+	t.Run("get_all_pages empty", func(t *testing.T) {
+		tool := findTool(t, tools, "get_all_pages")
+		res := callTool(t, cs, "get_all_pages", nil)
+		if res.IsError {
+			t.Fatalf("IsError = true: %s", toolText(t, res))
+		}
+		raw, err := json.Marshal(res.StructuredContent)
+		if err != nil {
+			t.Fatalf("marshal structuredContent: %v", err)
+		}
+		if got, want := string(raw), `{"pages":[]}`; got != want {
+			t.Errorf("structuredContent = %s, want %s", got, want)
+		}
+		validateStructuredContent(t, tool, res.StructuredContent)
+	})
+}
+
+// TestNullPageHasNoStructuredContent pins the semantic contract for a missing
+// page: the text content is exactly "null" and no structuredContent is sent.
+// A typed nil page coerced into structured content would surface as {} and
+// read like an existing empty page.
+func TestNullPageHasNoStructuredContent(t *testing.T) {
+	cs, _ := newTestSession(t, false)
+
+	res := callTool(t, cs, "get_page_by_id", map[string]any{"id": 404})
+	if res.IsError {
+		t.Fatalf("IsError = true: %s", toolText(t, res))
+	}
+	if res.StructuredContent != nil {
+		t.Errorf("structuredContent = %v, want none for a missing page", res.StructuredContent)
+	}
+	if got := strings.TrimSpace(toolText(t, res)); got != "null" {
+		t.Errorf("text = %q, want \"null\"", got)
+	}
+}

+ 8 - 38
internal/server/tools_test.go

@@ -23,9 +23,10 @@ type capturedRequest struct {
 }
 
 type fakeWiki struct {
-	t        *testing.T
-	mu       sync.Mutex
-	requests []capturedRequest
+	t         *testing.T
+	mu        sync.Mutex
+	requests  []capturedRequest
+	emptyList bool // GetAllPages returns an empty list
 }
 
 func (f *fakeWiki) handle(w http.ResponseWriter, r *http.Request) {
@@ -70,6 +71,10 @@ func (f *fakeWiki) handle(w http.ResponseWriter, r *http.Request) {
 			"tags":[],"content":"body","createdAt":"c","updatedAt":"u","locale":%q}}}}`, path, locale)
 
 	case strings.Contains(req.Query, "query GetAllPages"):
+		if f.emptyList {
+			fmt.Fprint(w, `{"data":{"pages":{"list":[]}}}`)
+			return
+		}
 		fmt.Fprint(w, `{"data":{"pages":{"list":[
 			{"id":1,"path":"/a","locale":"de","title":"A","description":"a",
 			 "createdAt":"c","updatedAt":"u","tags":["go"]}
@@ -185,41 +190,6 @@ func TestToolListWithoutEdit(t *testing.T) {
 	}
 }
 
-// TestListToolsOutputSchemasAreObjects guards against the MCP spec violation
-// that made strict clients (for example pydantic-based ones) reject the whole
-// tools/list response: a tool whose outputSchema.type is not the string
-// "object" is invalid whenever an output schema is declared. The check runs
-// against the wire format, which is what strict clients validate.
-func TestListToolsOutputSchemasAreObjects(t *testing.T) {
-	cs, _ := newTestSession(t, true)
-
-	res, err := cs.ListTools(context.Background(), nil)
-	if err != nil {
-		t.Fatalf("ListTools() error = %v", err)
-	}
-	if len(res.Tools) == 0 {
-		t.Fatal("no tools listed")
-	}
-	for _, tool := range res.Tools {
-		if tool.OutputSchema == nil {
-			continue // optional per the MCP spec
-		}
-		raw, err := json.Marshal(tool.OutputSchema)
-		if err != nil {
-			t.Fatalf("marshal outputSchema of %q: %v", tool.Name, err)
-		}
-		var schema struct {
-			Type json.RawMessage `json:"type"`
-		}
-		if err := json.Unmarshal(raw, &schema); err != nil {
-			t.Fatalf("unmarshal outputSchema of %q: %v", tool.Name, err)
-		}
-		if string(schema.Type) != `"object"` {
-			t.Errorf("tool %q outputSchema.type = %s, want \"object\"", tool.Name, schema.Type)
-		}
-	}
-}
-
 func TestToolListWithEdit(t *testing.T) {
 	cs, _ := newTestSession(t, true)