diff --git a/server/router/mcp/adapter.go b/server/router/mcp/adapter.go index 070c3cb64..9437179e4 100644 --- a/server/router/mcp/adapter.go +++ b/server/router/mcp/adapter.go @@ -102,11 +102,36 @@ func substitutePathParameters(operation *openAPIOperation, arguments map[string] if !ok || value == nil || valueToString(value) == "" { return "", errors.Errorf(`missing required path parameter "%s"`, parameter.Name) } - path = strings.ReplaceAll(path, "{"+parameter.Name+"}", url.PathEscape(valueToString(value))) + id := trimResourceNamePrefix(operation.Path, parameter.Name, valueToString(value)) + path = strings.ReplaceAll(path, "{"+parameter.Name+"}", url.PathEscape(id)) } return path, nil } +// trimResourceNamePrefix accepts canonical resource names for path parameters. +// The API returns names like "memos/abc123", but the REST paths take the bare +// ID ("/api/v1/memos/{memo}"), so clients that round-trip a returned name +// would otherwise request "/api/v1/memos/memos/abc123" and get a 404. When the +// placeholder directly follows its collection segment and the value carries +// that collection prefix, strip the prefix; bare IDs pass through unchanged. +func trimResourceNamePrefix(path, parameterName, value string) string { + placeholder := "/{" + parameterName + "}" + index := strings.Index(path, placeholder) + if index < 0 { + return value + } + head := path[:index] + collection := head[strings.LastIndex(head, "/")+1:] + if collection == "" { + return value + } + id, ok := strings.CutPrefix(value, collection+"/") + if !ok || id == "" || strings.Contains(id, "/") { + return value + } + return id +} + func valueToString(value any) string { switch typed := value.(type) { case string: diff --git a/server/router/mcp/adapter_test.go b/server/router/mcp/adapter_test.go index 584955533..64b13674c 100644 --- a/server/router/mcp/adapter_test.go +++ b/server/router/mcp/adapter_test.go @@ -6,6 +6,7 @@ import ( "errors" "io" "net/http" + "strings" "testing" "github.com/labstack/echo/v5" @@ -47,11 +48,10 @@ func TestNewStructuredToolResultUsesObjectStructuredContent(t *testing.T) { func TestNewToolErrorResult(t *testing.T) { result := newToolErrorResult("resource not found") require.True(t, result.IsError) - require.Equal(t, map[string]any{ - "error": map[string]any{ - "message": "resource not found", - }, - }, result.StructuredContent) + // Error results carry no structuredContent: tools declare an outputSchema + // for their success payload, and strict clients validate structuredContent + // against it — an error object would fail validation and mask the message. + require.Nil(t, result.StructuredContent) require.NotEmpty(t, result.Content) text, ok := result.Content[0].(*sdkmcp.TextContent) require.True(t, ok) @@ -118,6 +118,35 @@ func TestBuildAPIRequestMapsPathQueryAndBody(t *testing.T) { require.JSONEq(t, `{"memo":{"name":"memos/abc123","content":"updated"}}`, string(body)) } +func TestBuildAPIRequestAcceptsResourceNamesForPathParameters(t *testing.T) { + tests := []struct { + name string + path string + value string + wantPath string + }{ + {name: "canonical memo name", path: "/api/v1/memos/{memo}", value: "memos/abc123", wantPath: "/api/v1/memos/abc123"}, + {name: "bare memo id", path: "/api/v1/memos/{memo}", value: "abc123", wantPath: "/api/v1/memos/abc123"}, + {name: "canonical name on nested route", path: "/api/v1/memos/{memo}/comments", value: "memos/abc123", wantPath: "/api/v1/memos/abc123/comments"}, + {name: "canonical attachment name", path: "/api/v1/attachments/{attachment}", value: "attachments/att42", wantPath: "/api/v1/attachments/att42"}, + {name: "foreign prefix left untouched", path: "/api/v1/memos/{memo}", value: "attachments/att42", wantPath: "/api/v1/memos/attachments%2Fatt42"}, + {name: "multi-segment value left untouched", path: "/api/v1/memos/{memo}", value: "memos/abc/extra", wantPath: "/api/v1/memos/memos%2Fabc%2Fextra"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + parameterName := test.path[strings.Index(test.path, "{")+1 : strings.Index(test.path, "}")] + operation := &openAPIOperation{ + Method: "GET", + Path: test.path, + Parameters: []openAPIParameter{{Name: parameterName, In: "path", Required: true, Schema: jsonSchema{"type": "string"}}}, + } + req, err := buildAPIRequest(context.Background(), operation, map[string]any{parameterName: test.value}, "") + require.NoError(t, err) + require.Equal(t, test.wantPath, req.URL.EscapedPath()) + }) + } +} + func TestBuildAPIRequestRequiresPathParameters(t *testing.T) { operation := &openAPIOperation{ Method: "GET", @@ -201,11 +230,7 @@ func TestExecuteOperationConvertsAPIErrorsToToolErrors(t *testing.T) { result, err := adapter.execute(context.Background(), operation, map[string]any{"memo": "missing"}, "") require.NoError(t, err) require.True(t, result.IsError) - require.Equal(t, map[string]any{ - "error": map[string]any{ - "message": "404 Not Found: missing memo", - }, - }, result.StructuredContent) + require.Nil(t, result.StructuredContent) text, ok := result.Content[0].(*sdkmcp.TextContent) require.True(t, ok) require.Contains(t, text.Text, "404") diff --git a/server/router/mcp/result.go b/server/router/mcp/result.go index 2f92ed8b4..4941a0064 100644 --- a/server/router/mcp/result.go +++ b/server/router/mcp/result.go @@ -35,18 +35,17 @@ func newStructuredToolResult(value any) (*sdkmcp.CallToolResult, error) { }, nil } +// newToolErrorResult reports a tool execution error through IsError and text +// content only. Error results must not carry structuredContent: every tool +// declares an outputSchema describing its success payload, and spec-strict +// clients validate structuredContent against that schema — an {"error": ...} +// object fails validation and masks the real error message. func newToolErrorResult(message string) *sdkmcp.CallToolResult { - structured := map[string]any{ - "error": map[string]any{ - "message": message, - }, - } return &sdkmcp.CallToolResult{ Content: []sdkmcp.Content{ &sdkmcp.TextContent{Text: message}, }, - StructuredContent: structured, - IsError: true, + IsError: true, } } diff --git a/server/router/mcp/service_test.go b/server/router/mcp/service_test.go index ebfb3ab3d..54586f8c2 100644 --- a/server/router/mcp/service_test.go +++ b/server/router/mcp/service_test.go @@ -237,11 +237,17 @@ func TestMCPToolCallRejectsInvalidArguments(t *testing.T) { result, ok := response["result"].(map[string]any) require.True(t, ok) require.Equal(t, true, result["isError"]) - structured, ok := result["structuredContent"].(map[string]any) + // Error results carry no structuredContent — it would fail + // validation against the tool's declared outputSchema in strict + // clients. The message travels in the text content instead. + _, hasStructured := result["structuredContent"] + require.False(t, hasStructured) + content, ok := result["content"].([]any) require.True(t, ok) - errorObject, ok := structured["error"].(map[string]any) + require.NotEmpty(t, content) + textBlock, ok := content[0].(map[string]any) require.True(t, ok) - require.Contains(t, errorObject["message"], test.wantError) + require.Contains(t, textBlock["text"], test.wantError) }) } require.Zero(t, routeHits)