mirror of
https://gitea.com/gitea/gitea-mcp.git
synced 2026-08-03 15:49:23 +02:00
fix: accept null tool arguments and bound HTTP resource use
Review follow-ups on the SDK migration. An "arguments": null is what clients send for parameterless tools like get_me, and what mcp-go accepted by returning a nil map. The new adapter rejected it with InvalidParams, which broke those calls outright. The /mcp endpoint took unlimited request bodies and never expired idle sessions, so a peer that goes away without DELETE kept its session for the process lifetime. Both are reachable before any token check, so neither can stay unbounded; the body cap sits above the SDK default to leave room for the base64 content create_or_update_file accepts. Required() smuggled a bool through the property schema map and deleted it again, colliding with the JSON Schema keyword of the same name. It now sets a field on Property, so an object property can carry its own required list. The tool contract fixture cost a manual regeneration step and four hand-maintained counts on every tool change, and a snapshot freezes defects rather than reporting them. Property assertions cover the same surface and reject a duplicate tool name, a readOnlyHint that disagrees with the register call, and a default that contradicts its own type or enum. Co-Authored-By: Claude (Opus 5) <noreply@anthropic.com>
This commit is contained in:
silverwind
+99
-136
@@ -1,179 +1,142 @@
|
||||
package operation
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/json"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"sort"
|
||||
"slices"
|
||||
"testing"
|
||||
|
||||
"github.com/modelcontextprotocol/go-sdk/mcp"
|
||||
)
|
||||
|
||||
const updateToolContractEnv = "UPDATE_TOOL_CONTRACT"
|
||||
|
||||
type toolContract struct {
|
||||
Scope string `json:"scope"`
|
||||
Access string `json:"access"`
|
||||
Name string `json:"name"`
|
||||
Description string `json:"description"`
|
||||
InputSchema any `json:"inputSchema"`
|
||||
Annotations contractAnnotations `json:"annotations"`
|
||||
}
|
||||
|
||||
type contractAnnotations struct {
|
||||
Title string `json:"title"`
|
||||
ReadOnlyHint bool `json:"readOnlyHint"`
|
||||
DestructiveHint bool `json:"destructiveHint"`
|
||||
IdempotentHint bool `json:"idempotentHint"`
|
||||
OpenWorldHint bool `json:"openWorldHint"`
|
||||
}
|
||||
|
||||
// TestToolContract checks the properties every exposed tool must hold, rather
|
||||
// than a snapshot of the current surface, so adding a tool needs no fixture
|
||||
// update and a malformed schema fails here instead of panicking in AddTool.
|
||||
func TestToolContract(t *testing.T) {
|
||||
const (
|
||||
wantDomains = 18
|
||||
wantTools = 54
|
||||
wantRead = 33
|
||||
wantWrite = 21
|
||||
)
|
||||
|
||||
contracts := make([]toolContract, 0, wantTools)
|
||||
seenScopes := make(map[string]struct{}, wantDomains)
|
||||
seenNames := make(map[string]struct{}, wantTools)
|
||||
readCount, writeCount := 0, 0
|
||||
|
||||
scopeByName := map[string]string{}
|
||||
seenScopes := map[string]struct{}{}
|
||||
for _, domain := range domainTools {
|
||||
scope := domain.Scope()
|
||||
if scope == "" {
|
||||
t.Fatal("registered tool domain has an empty scope")
|
||||
t.Error("domainTools contains a domain with an empty scope")
|
||||
}
|
||||
// Tools() filters one domain by exactly one scope name, so a shared
|
||||
// scope would make --scope select more than the caller asked for.
|
||||
if _, duplicate := seenScopes[scope]; duplicate {
|
||||
t.Fatalf("duplicate tool domain scope %q", scope)
|
||||
t.Errorf("domainTools contains a duplicate scope %q", scope)
|
||||
}
|
||||
seenScopes[scope] = struct{}{}
|
||||
|
||||
for _, registered := range domain.ReadTools() {
|
||||
contracts = append(contracts, decodeToolContract(t, scope, "read", registered.Tool))
|
||||
readCount++
|
||||
assertToolContract(t, scope, registered.Tool, true, scopeByName)
|
||||
}
|
||||
for _, registered := range domain.WriteTools() {
|
||||
contracts = append(contracts, decodeToolContract(t, scope, "write", registered.Tool))
|
||||
writeCount++
|
||||
assertToolContract(t, scope, registered.Tool, false, scopeByName)
|
||||
}
|
||||
}
|
||||
if len(scopeByName) == 0 {
|
||||
t.Fatal("no tools are registered")
|
||||
}
|
||||
}
|
||||
|
||||
if len(seenScopes) != wantDomains {
|
||||
t.Errorf("domain count = %d, want %d", len(seenScopes), wantDomains)
|
||||
}
|
||||
if len(contracts) != wantTools {
|
||||
t.Errorf("tool count = %d, want %d", len(contracts), wantTools)
|
||||
}
|
||||
if readCount != wantRead {
|
||||
t.Errorf("read tool count = %d, want %d", readCount, wantRead)
|
||||
}
|
||||
if writeCount != wantWrite {
|
||||
t.Errorf("write tool count = %d, want %d", writeCount, wantWrite)
|
||||
}
|
||||
func assertToolContract(t *testing.T, scope string, definition *mcp.Tool, readOnly bool, scopeByName map[string]string) {
|
||||
t.Helper()
|
||||
|
||||
for _, contract := range contracts {
|
||||
if _, duplicate := seenNames[contract.Name]; duplicate {
|
||||
t.Errorf("duplicate tool name %q", contract.Name)
|
||||
t.Run(definition.Name, func(t *testing.T) {
|
||||
if previous, duplicate := scopeByName[definition.Name]; duplicate {
|
||||
t.Errorf("tool name is already registered in scope %q; AddTool would silently replace it", previous)
|
||||
}
|
||||
seenNames[contract.Name] = struct{}{}
|
||||
}
|
||||
scopeByName[definition.Name] = scope
|
||||
|
||||
sort.Slice(contracts, func(i, j int) bool {
|
||||
if contracts[i].Scope != contracts[j].Scope {
|
||||
return contracts[i].Scope < contracts[j].Scope
|
||||
// Strict MCP clients reject a tools/list entry without a description.
|
||||
if definition.Description == "" {
|
||||
t.Error("tool has no description")
|
||||
}
|
||||
if contracts[i].Access != contracts[j].Access {
|
||||
return contracts[i].Access < contracts[j].Access
|
||||
|
||||
// A write tool registered as read stays exposed under --read-only.
|
||||
if definition.Annotations == nil || definition.Annotations.ReadOnlyHint != readOnly {
|
||||
t.Errorf("annotations = %+v, want readOnlyHint %v", definition.Annotations, readOnly)
|
||||
}
|
||||
|
||||
schema := decodeJSON(t, definition.InputSchema)
|
||||
if schema["type"] != "object" {
|
||||
t.Fatalf("input schema type = %v, want object", schema["type"])
|
||||
}
|
||||
properties, ok := schema["properties"].(map[string]any)
|
||||
if !ok {
|
||||
t.Fatalf("input schema properties = %T, want a JSON object", schema["properties"])
|
||||
}
|
||||
|
||||
for name, raw := range properties {
|
||||
property, ok := raw.(map[string]any)
|
||||
if !ok {
|
||||
t.Errorf("property %q = %T, want a JSON object", name, raw)
|
||||
continue
|
||||
}
|
||||
assertPropertyContract(t, name, property)
|
||||
}
|
||||
return contracts[i].Name < contracts[j].Name
|
||||
})
|
||||
|
||||
got, err := json.MarshalIndent(contracts, "", " ")
|
||||
if err != nil {
|
||||
t.Fatalf("marshal tool contract: %v", err)
|
||||
}
|
||||
got = append(got, '\n')
|
||||
|
||||
goldenPath := filepath.Join("testdata", "tools.golden.json")
|
||||
if os.Getenv(updateToolContractEnv) == "1" {
|
||||
if err := os.WriteFile(goldenPath, got, 0o644); err != nil {
|
||||
t.Fatalf("update tool contract: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
want, err := os.ReadFile(goldenPath)
|
||||
if err != nil {
|
||||
t.Fatalf("read tool contract: %v", err)
|
||||
}
|
||||
if !bytes.Equal(got, want) {
|
||||
t.Errorf("tool contract changed; inspect the semantic diff before running %s=1 go test -run '^TestToolContract$' ./operation/", updateToolContractEnv)
|
||||
}
|
||||
}
|
||||
|
||||
func decodeToolContract(t *testing.T, scope, access string, toolDefinition any) toolContract {
|
||||
func assertPropertyContract(t *testing.T, name string, property map[string]any) {
|
||||
t.Helper()
|
||||
|
||||
data, err := json.Marshal(toolDefinition)
|
||||
if err != nil {
|
||||
t.Fatalf("marshal %s tool in scope %q: %v", access, scope, err)
|
||||
}
|
||||
var definition map[string]any
|
||||
if err := json.Unmarshal(data, &definition); err != nil {
|
||||
t.Fatalf("decode %s tool in scope %q: %v", access, scope, err)
|
||||
}
|
||||
|
||||
name := requiredString(t, definition, "name", scope)
|
||||
description := requiredString(t, definition, "description", name)
|
||||
inputSchema, ok := definition["inputSchema"].(map[string]any)
|
||||
propertyType, ok := property["type"].(string)
|
||||
if !ok {
|
||||
t.Fatalf("tool %q has inputSchema of type %T, want JSON object", name, definition["inputSchema"])
|
||||
t.Errorf("property %q has no type", name)
|
||||
return
|
||||
}
|
||||
// An omitted required keyword and an empty array have the same JSON Schema meaning.
|
||||
if _, ok := inputSchema["required"]; !ok {
|
||||
inputSchema["required"] = []any{}
|
||||
}
|
||||
annotations, _ := definition["annotations"].(map[string]any)
|
||||
|
||||
// Normalize protocol defaults independently of SDK omitempty behavior.
|
||||
return toolContract{
|
||||
Scope: scope,
|
||||
Access: access,
|
||||
Name: name,
|
||||
Description: description,
|
||||
InputSchema: inputSchema,
|
||||
Annotations: contractAnnotations{
|
||||
Title: stringField(annotations, "title", ""),
|
||||
ReadOnlyHint: boolField(annotations, "readOnlyHint", false),
|
||||
DestructiveHint: boolField(annotations, "destructiveHint", true),
|
||||
IdempotentHint: boolField(annotations, "idempotentHint", false),
|
||||
OpenWorldHint: boolField(annotations, "openWorldHint", true),
|
||||
},
|
||||
enum, hasEnum := property["enum"].([]any)
|
||||
if _, declared := property["enum"]; declared && len(enum) == 0 {
|
||||
t.Errorf("property %q has an empty enum", name)
|
||||
}
|
||||
|
||||
defaultValue, hasDefault := property["default"]
|
||||
if !hasDefault {
|
||||
return
|
||||
}
|
||||
if !matchesJSONType(defaultValue, propertyType) {
|
||||
t.Errorf("property %q default %#v is not a %s", name, defaultValue, propertyType)
|
||||
}
|
||||
if hasEnum && !slices.Contains(enum, defaultValue) {
|
||||
t.Errorf("property %q default %#v is not one of its enum values %#v", name, defaultValue, enum)
|
||||
}
|
||||
}
|
||||
|
||||
func requiredString(t *testing.T, object map[string]any, key, owner string) string {
|
||||
func matchesJSONType(value any, propertyType string) bool {
|
||||
switch propertyType {
|
||||
case "string":
|
||||
_, ok := value.(string)
|
||||
return ok
|
||||
case "number":
|
||||
_, ok := value.(float64)
|
||||
return ok
|
||||
case "boolean":
|
||||
_, ok := value.(bool)
|
||||
return ok
|
||||
case "array":
|
||||
_, ok := value.([]any)
|
||||
return ok
|
||||
case "object":
|
||||
_, ok := value.(map[string]any)
|
||||
return ok
|
||||
default:
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
// decodeJSON round-trips through JSON so the assertions see what an MCP client
|
||||
// receives rather than the Go values behind it.
|
||||
func decodeJSON(t *testing.T, value any) map[string]any {
|
||||
t.Helper()
|
||||
|
||||
value, ok := object[key].(string)
|
||||
if !ok || value == "" {
|
||||
t.Fatalf("%s has missing or empty %q", owner, key)
|
||||
encoded, err := json.Marshal(value)
|
||||
if err != nil {
|
||||
t.Fatalf("marshal: %v", err)
|
||||
}
|
||||
return value
|
||||
}
|
||||
|
||||
func stringField(object map[string]any, key, fallback string) string {
|
||||
if value, ok := object[key].(string); ok {
|
||||
return value
|
||||
var decoded map[string]any
|
||||
if err := json.Unmarshal(encoded, &decoded); err != nil {
|
||||
t.Fatalf("decode: %v", err)
|
||||
}
|
||||
return fallback
|
||||
}
|
||||
|
||||
func boolField(object map[string]any, key string, fallback bool) bool {
|
||||
if value, ok := object[key].(bool); ok {
|
||||
return value
|
||||
}
|
||||
return fallback
|
||||
return decoded
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user