diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index 51f800390..d48068c56 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -2801,33 +2801,26 @@ func TestNewServerWithToolsAdminProfile(t *testing.T) { } } +func compareMCPToolInventory(expected, actual map[string]*server.ServerTool) error { + if len(expected) != len(actual) { + return fmt.Errorf("tool count = %d, want %d", len(actual), len(expected)) + } + for name := range expected { + if actual[name] == nil { + return fmt.Errorf("missing tool %q", name) + } + } + return nil +} + func TestNewServerWithToolsNilRegistersAll(t *testing.T) { s := newMCPTestStore(t) - srv := NewServerWithTools(s, nil) if srv == nil { t.Fatal("expected MCP server instance") } - - tools := srv.ListTools() - - allTools := []string{ - "mem_save", "mem_search", "mem_context", "mem_session_summary", - "mem_session_start", "mem_session_end", "mem_get_observation", - "mem_suggest_topic_key", "mem_capture_passive", "mem_save_prompt", - "mem_update", "mem_delete", "mem_stats", "mem_timeline", "mem_merge_projects", - "mem_current_project", "mem_judge", "mem_compare", "mem_doctor", "mem_review", - "mem_pin", "mem_unpin", - } - - for _, name := range allTools { - if tools[name] == nil { - t.Errorf("nil allowlist: expected tool %q to be registered", name) - } - } - - if len(tools) != len(allTools) { - t.Errorf("expected %d tools with nil allowlist, got %d", len(allTools), len(tools)) + if err := compareMCPToolInventory(NewServer(s).ListTools(), srv.ListTools()); err != nil { + t.Fatalf("nil allowlist inventory: %v", err) } } @@ -2915,34 +2908,7 @@ func TestMemDoctorUnknownProjectReturnsStructuredError(t *testing.T) { } } -func TestNewServerBackwardsCompatible(t *testing.T) { - s := newMCPTestStore(t) - - // NewServer (no tools filter) should register all tools - srv := NewServer(s) - tools := srv.ListTools() - - // 18 agent + 4 admin = 22 total. - if len(tools) != 22 { - t.Errorf("NewServer should register all 22 tools, got %d", len(tools)) - } -} - func TestProfileConsistency(t *testing.T) { - // Verify that agent + admin = all 22 tools - combined := make(map[string]bool) - for tool := range ProfileAgent { - combined[tool] = true - } - for tool := range ProfileAdmin { - combined[tool] = true - } - - // 18 agent + 4 admin = 22 total. - if len(combined) != 22 { - t.Errorf("agent + admin should cover all 22 tools, got %d", len(combined)) - } - // Verify no overlap between profiles for tool := range ProfileAgent { if ProfileAdmin[tool] { @@ -3267,10 +3233,8 @@ func TestNewServerWithConfig(t *testing.T) { if srv == nil { t.Fatal("expected MCP server instance") } - tools := srv.ListTools() - // Should have all 22 tools (18 agent + 4 admin). - if len(tools) != 22 { - t.Errorf("NewServerWithConfig should register all 22 tools, got %d", len(tools)) + if err := compareMCPToolInventory(NewServer(s).ListTools(), srv.ListTools()); err != nil { + t.Fatalf("default config inventory: %v", err) } } diff --git a/internal/mcp/tool_contract_test.go b/internal/mcp/tool_contract_test.go index e568cd8e6..0f5511938 100644 --- a/internal/mcp/tool_contract_test.go +++ b/internal/mcp/tool_contract_test.go @@ -58,6 +58,7 @@ func TestNormalizeMCPToolContract(t *testing.T) { {"supported recursion", `{"type":"object","title":"ignored","properties":{"a/b":{"type":"array","items":{"type":["integer","string"],"description":"ignored"}}},"required":["a/b","a/b"],"additionalProperties":false}`, ""}, {"missing type", `{"properties":{}}`, "/input"}, {"unknown keyword", `{"type":"string","pattern":"x"}`, "/input/pattern"}, + {"unknown live-only addition", `{"type":"object","properties":{"new":{"type":"string","pattern":"x"}}}`, "/input/properties/new/pattern"}, {"malformed properties", `{"type":"object","properties":[]}`, "/input/properties"}, {"invalid required", `{"type":"object","properties":{},"required":["missing"]}`, "/input/required"}, {"tuple items", `{"type":"array","items":[{"type":"string"}]}`, "/input/items"}, @@ -351,15 +352,53 @@ func TestReadMCPToolContractFixture(t *testing.T) { } } -func TestExactMCPToolContract(t *testing.T) { - base := map[string]mcpToolSchema{"x": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"p": {Types: []string{"array"}, Items: &mcpToolSchema{Types: []string{"string"}, Enum: []string{`"a"`}}}, "q": {Types: []string{"number"}}}, Required: []string{"p"}, Additional: false}} - for index, changed := range []map[string]mcpToolSchema{ - base, {}, {"x": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"p": {Types: []string{"array"}, Items: &mcpToolSchema{Types: []string{"string"}, Enum: []string{`"b"`}}}, "q": {Types: []string{"integer"}}}, Required: []string{"p", "q"}, Additional: false}}, {"x": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"p": {Types: []string{"array"}}, "q": {Types: []string{"number"}}, "new": {Types: []string{"string"}}}, Required: []string{"p"}, Additional: false}}, - } { - if err := exactMCPToolContract(base, changed); index == 0 && err != nil || index > 0 && err == nil { - t.Fatal("exact equality expectation failed") +func TestCompareMCPToolContract(t *testing.T) { + base := map[string]mcpToolSchema{"x": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"optional": {Types: []string{"integer"}}, "required": {Types: []string{"array"}, Items: &mcpToolSchema{Types: []string{"string"}, Enum: []string{`"a"`}}}}, Required: []string{"required"}, Additional: false}} + cases := []struct { + name string + live map[string]mcpToolSchema + want string + }{ + {"compatible widening", map[string]mcpToolSchema{"x": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"optional": {Types: []string{"number", "boolean"}}, "required": {Types: []string{"array"}, Items: &mcpToolSchema{Types: []string{"string"}, Enum: []string{`"a"`, `"b"`}}}, "new": {Types: []string{"boolean"}}}, Required: []string{"required"}, Additional: true}, "new-tool": {Types: []string{"string"}}}, ""}, + {"missing tool", map[string]mcpToolSchema{}, "missing-tool"}, + {"missing property", map[string]mcpToolSchema{"x": {Types: []string{"object"}}}, "missing-property"}, + {"requiredness narrowed", map[string]mcpToolSchema{"x": {Types: []string{"object"}, Properties: base["x"].Properties, Required: []string{"optional", "required"}, Additional: false}}, "requiredness-narrowed"}, + {"new required property", map[string]mcpToolSchema{"x": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"optional": {Types: []string{"integer"}}, "required": base["x"].Properties["required"], "new": {Types: []string{"string"}}}, Required: []string{"required", "new"}, Additional: false}}, "new-required-property"}, + {"type narrowed", map[string]mcpToolSchema{"x": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"optional": {Types: []string{"integer"}}, "required": {Types: []string{"array"}, Items: &mcpToolSchema{Types: []string{"number"}}}}, Required: []string{"required"}, Additional: false}}, "type-narrowed"}, + {"enum narrowed", map[string]mcpToolSchema{"x": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"optional": {Types: []string{"integer"}}, "required": {Types: []string{"array"}, Items: &mcpToolSchema{Types: []string{"string"}, Enum: []string{`"b"`}}}}, Required: []string{"required"}, Additional: false}}, "enum-narrowed"}, + {"unproven optional addition", map[string]mcpToolSchema{"x": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"optional": {Types: []string{"integer"}}, "required": base["x"].Properties["required"], "new": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"nested": {Types: []string{"string"}}}}}, Required: []string{"required"}, Additional: false}}, "unproven-optional-addition"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := compareMCPToolContract(base, tc.live) + if tc.want == "" && err != nil || tc.want != "" && (err == nil || !strings.Contains(err.Error(), tc.want)) { + t.Fatalf("error = %v, want %q", err, tc.want) + } + }) + } + additionalBase := map[string]mcpToolSchema{"x": base["x"]} + additionalBase["x"] = mcpToolSchema{Types: []string{"object"}, Properties: base["x"].Properties, Required: []string{"required"}, Additional: true} + if err := compareMCPToolContract(additionalBase, base); err == nil || !strings.Contains(err.Error(), "additional-properties-narrowed") { + t.Fatalf("additionalProperties narrowing error = %v", err) + } +} + +func TestCompareMCPToolContractDiagnostics(t *testing.T) { + base := map[string]mcpToolSchema{"a/b": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"x~y": {Types: []string{"string"}}}, Additional: true}, "z": {Types: []string{"string"}}} + live := map[string]mcpToolSchema{"a/b": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"x~y": {Types: []string{"number"}}}, Additional: false}} + err := compareMCPToolContract(base, live) + if err == nil { + t.Fatal("expected multiple drifts") + } + got := err.Error() + for _, want := range []string{"/tools/a~1b: additional-properties-narrowed: v1=", "/tools/a~1b/properties/x~0y: type-narrowed: v1=", "/tools/z: missing-tool: v1="} { + if !strings.Contains(got, want) { + t.Fatalf("diagnostic %q missing from %s", want, got) } } + if strings.Index(got, "/tools/a~1b: additional-properties-narrowed") > strings.Index(got, "/tools/a~1b/properties/x~0y: type-narrowed") || strings.Index(got, "/tools/a~1b/properties/x~0y: type-narrowed") > strings.Index(got, "/tools/z: missing-tool") { + t.Fatalf("diagnostics are not sorted: %s", got) + } } func TestMCPToolContractV1(t *testing.T) { @@ -375,17 +414,40 @@ func TestMCPToolContractV1(t *testing.T) { if err != nil { t.Fatal(err) } - if err := exactMCPToolContract(fixture, live); err != nil { + if err := verifyMCPToolContract(fixture, live, before); err != nil { t.Fatal(err) } - if formatted := formatMCPToolContract(live); string(before) != formatted { - t.Fatal("fixture is not byte-canonical live formatter output") - } if after, err := os.ReadFile("testdata/tool-contract-v1.json"); err != nil || string(before) != string(after) { t.Fatalf("fixture changed: %v", err) } } +func TestVerifyMCPToolContract(t *testing.T) { + fixture := map[string]mcpToolSchema{"base": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"kept": {Types: []string{"string"}}}, Additional: true}} + canonical := []byte(formatMCPToolContract(fixture)) + live := map[string]mcpToolSchema{"base": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"kept": {Types: []string{"string"}}, "added": {Types: []string{"boolean"}}}, Additional: true}, "new": {Types: []string{"string"}}} + if err := verifyMCPToolContract(fixture, live, canonical); err != nil { + t.Fatalf("compatible additions: %v", err) + } + incompatible := map[string]mcpToolSchema{"base": {Types: []string{"object"}, Properties: map[string]mcpToolSchema{"kept": {Types: []string{"number"}}}, Additional: true}} + if err := verifyMCPToolContract(fixture, incompatible, canonical); err == nil || !strings.Contains(err.Error(), "type-narrowed") { + t.Fatalf("incompatible live change: %v", err) + } + if err := verifyMCPToolContract(fixture, fixture, []byte("not canonical")); err == nil || !strings.Contains(err.Error(), "fixture is not byte-canonical fixture formatter output") { + t.Fatalf("noncanonical fixture: %v", err) + } +} + +func verifyMCPToolContract(fixture, live map[string]mcpToolSchema, fixtureBytes []byte) error { + if err := compareMCPToolContract(fixture, live); err != nil { + return err + } + if string(fixtureBytes) != formatMCPToolContract(fixture) { + return fmt.Errorf("fixture is not byte-canonical fixture formatter output") + } + return nil +} + func formatMCPToolContract(tools map[string]mcpToolSchema) string { var b strings.Builder b.WriteString("{\n \"version\": \"engram.mcp-tool-contract/v1\",\n \"tools\": ") @@ -509,10 +571,111 @@ func readMCPToolContractFixture(raw []byte) (map[string]mcpToolSchema, error) { return out, nil } -func exactMCPToolContract(expected, live map[string]mcpToolSchema) error { - want, got := formatMCPToolContract(expected), formatMCPToolContract(live) - if want == got { +type mcpToolContractDrift struct{ path, code, v1, live string } + +func compareMCPToolContract(v1, live map[string]mcpToolSchema) error { + var drifts []mcpToolContractDrift + for _, name := range sortedKeys(v1) { + path := jsonPointer("/tools", name) + observed, ok := live[name] + if !ok { + drifts = append(drifts, mcpToolContractDrift{path, "missing-tool", schemaFact(v1[name]), ""}) + continue + } + compareMCPToolSchema(&drifts, path, v1[name], observed) + } + if len(drifts) == 0 { return nil } - return fmt.Errorf("exact MCP tool contract drift\nexpected:\n%s\nlive:\n%s", want, got) + sort.Slice(drifts, func(i, j int) bool { + return drifts[i].path < drifts[j].path || drifts[i].path == drifts[j].path && drifts[i].code < drifts[j].code + }) + var lines []string + for _, drift := range drifts { + lines = append(lines, fmt.Sprintf("%s: %s: v1=%s live=%s", drift.path, drift.code, drift.v1, drift.live)) + } + return fmt.Errorf("MCP tool contract drift\n%s", strings.Join(lines, "\n")) +} + +func compareMCPToolSchema(drifts *[]mcpToolContractDrift, path string, v1, live mcpToolSchema) { + drift := func(code string) { + *drifts = append(*drifts, mcpToolContractDrift{path, code, schemaFact(v1), schemaFact(live)}) + } + for _, typ := range v1.Types { + if !slices.Contains(live.Types, typ) && !(typ == "integer" && slices.Contains(live.Types, "number")) { + drift("type-narrowed") + break + } + } + if hasExtraType(v1.Types, live.Types) && (!simplePrimitive(v1) || !simplePrimitive(live)) { + drift("unproven-type-drift") + } + if v1.Additional && !live.Additional { + drift("additional-properties-narrowed") + } + if len(v1.Enum) == 0 && len(live.Enum) > 0 { + drift("enum-added") + } else if len(v1.Enum) > 0 && len(live.Enum) > 0 { + for _, value := range v1.Enum { + if !slices.Contains(live.Enum, value) { + drift("enum-narrowed") + break + } + } + } + if v1.Items == nil && live.Items != nil { + drift("shape-drift") + } else if v1.Items != nil && live.Items != nil { + compareMCPToolSchema(drifts, jsonPointer(path, "items"), *v1.Items, *live.Items) + } + for _, name := range sortedKeys(v1.Properties) { + child, ok := live.Properties[name] + if !ok { + *drifts = append(*drifts, mcpToolContractDrift{jsonPointer(jsonPointer(path, "properties"), name), "missing-property", schemaFact(v1.Properties[name]), ""}) + continue + } + compareMCPToolSchema(drifts, jsonPointer(jsonPointer(path, "properties"), name), v1.Properties[name], child) + } + for _, name := range live.Required { + if !slices.Contains(v1.Required, name) { + if _, ok := v1.Properties[name]; ok { + driftAt(drifts, jsonPointer(jsonPointer(path, "properties"), name), "requiredness-narrowed", v1.Properties[name], live.Properties[name]) + } else { + driftAt(drifts, jsonPointer(jsonPointer(path, "properties"), name), "new-required-property", mcpToolSchema{}, live.Properties[name]) + } + } + } + for _, name := range sortedKeys(live.Properties) { + if _, exists := v1.Properties[name]; !exists && !slices.Contains(live.Required, name) && !simplePrimitive(live.Properties[name]) { + driftAt(drifts, jsonPointer(jsonPointer(path, "properties"), name), "unproven-optional-addition", mcpToolSchema{}, live.Properties[name]) + } + } +} + +func hasExtraType(v1, live []string) bool { + for _, typ := range live { + if !slices.Contains(v1, typ) && !(typ == "number" && slices.Contains(v1, "integer")) { + return true + } + } + return false +} +func simplePrimitive(schema mcpToolSchema) bool { + if len(schema.Types) == 0 || len(schema.Properties) > 0 || schema.Items != nil || len(schema.Enum) > 0 { + return false + } + for _, typ := range schema.Types { + if !slices.Contains([]string{"string", "number", "integer", "boolean"}, typ) { + return false + } + } + return true +} +func driftAt(drifts *[]mcpToolContractDrift, path, code string, v1, live mcpToolSchema) { + *drifts = append(*drifts, mcpToolContractDrift{path, code, schemaFact(v1), schemaFact(live)}) +} +func schemaFact(schema mcpToolSchema) string { + var b strings.Builder + formatMCPToolSchema(&b, schema, 0) + return b.String() } diff --git a/internal/mcp/tool_contract_update_test.go b/internal/mcp/tool_contract_update_test.go new file mode 100644 index 000000000..e287efe68 --- /dev/null +++ b/internal/mcp/tool_contract_update_test.go @@ -0,0 +1,92 @@ +//go:build mcp_contract_update + +package mcp + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestWriteMCPToolContractFixture(t *testing.T) { + live, err := observeMCPToolContract(NewServer(newMCPTestStore(t)).ListTools()) + if err != nil { + t.Fatal(err) + } + if err := writeMCPToolContractFixture(os.Getenv("ENGRAM_MCP_CONTRACT_WRITE"), filepath.Join("testdata", "tool-contract-v1.json"), live, os.Getenv("CI") != "" || os.Getenv("GITHUB_ACTIONS") != ""); err != nil { + t.Fatal(err) + } +} + +func TestMCPToolContractWriterRefusals(t *testing.T) { + fixture := []byte(formatMCPToolContract(map[string]mcpToolSchema{"x": {Types: []string{"number"}}})) + for _, tc := range []struct { + name, mode, want string + ci bool + live map[string]mcpToolSchema + }{ + {"CI", "promote-v1", "refuses CI", true, map[string]mcpToolSchema{"x": {Types: []string{"number"}}}}, + {"invalid mode", "invalid", "must be", false, map[string]mcpToolSchema{"x": {Types: []string{"number"}}}}, + {"initial overwrite", "initial-v1", "refuses to overwrite", false, map[string]mcpToolSchema{"x": {Types: []string{"number"}}}}, + {"incompatible promotion", "promote-v1", "incompatible", false, map[string]mcpToolSchema{"x": {Types: []string{"integer"}}}}, + } { + t.Run(tc.name, func(t *testing.T) { + target := filepath.Join(t.TempDir(), "fixture.json") + if err := os.WriteFile(target, fixture, 0o600); err != nil { + t.Fatal(err) + } + if err := writeMCPToolContractFixture(tc.mode, target, tc.live, tc.ci); err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("error = %v, want %q", err, tc.want) + } + }) + } +} + +func writeMCPToolContractFixture(mode, target string, live map[string]mcpToolSchema, ci bool) error { + if ci { + return fmt.Errorf("fixture writer refuses CI") + } + current, statErr := os.ReadFile(target) + switch mode { + case "initial-v1": + if statErr == nil { + return fmt.Errorf("initial-v1 refuses to overwrite %s", target) + } + if !os.IsNotExist(statErr) { + return statErr + } + case "promote-v1": + if statErr != nil { + return fmt.Errorf("promote-v1 requires an existing fixture: %w", statErr) + } + v1, err := readMCPToolContractFixture(current) + if err != nil { + return err + } + if err := compareMCPToolContract(v1, live); err != nil { + return fmt.Errorf("promote-v1 refuses incompatible contract: %w", err) + } + default: + return fmt.Errorf("ENGRAM_MCP_CONTRACT_WRITE must be initial-v1 or promote-v1") + } + formatted := []byte(formatMCPToolContract(live)) + if _, err := readMCPToolContractFixture(formatted); err != nil { + return fmt.Errorf("validate formatted fixture: %w", err) + } + temp, err := os.CreateTemp(filepath.Dir(target), ".tool-contract-*") + if err != nil { + return err + } + tempName := temp.Name() + defer os.Remove(tempName) + if _, err := temp.Write(formatted); err != nil { + temp.Close() + return err + } + if err := temp.Close(); err != nil { + return err + } + return os.Rename(tempName, target) +}