mcp: return input validation errors as tool results, not JSON-RPC errors (#863)
This implements [SEP-1303](https://github.com/modelcontextprotocol/modelcontextprotocol/issues/1303). Input validation errors (missing required fields, wrong types) during tool calls are now returned as `CallToolResult` with `IsError=true` instead of JSON-RPC `-32602` errors. This allows LLMs to see the validation error and self-correct, per the MCP spec. The existing pattern for handler-returned errors (lines 347-350 in server.go) already converts regular errors to tool results with `IsError=true`. This change applies the same pattern to argument schema validation and unmarshal errors, resolving the `TODO(#450)` in the code. Thanks to @iwyrkore for filing the issue and proposing the approach, and @findleyr for the cross-SDK analysis in the discussion. Fixes #450 Co-authored-by: Maciej Kisiel <mkisiel@google.com>
This commit is contained in:
parent
b71a8391a6
commit
74d2751d88
4 changed files with 73 additions and 23 deletions
|
|
@ -51,17 +51,9 @@ func TestServerErrors(t *testing.T) {
|
|||
executeCall func() error
|
||||
expectedCode int64
|
||||
}{
|
||||
{
|
||||
name: "missing required param",
|
||||
executeCall: func() error {
|
||||
_, err := cs.CallTool(ctx, &CallToolParams{
|
||||
Name: "validate",
|
||||
Arguments: map[string]any{}, // Missing required "name" field
|
||||
})
|
||||
return err
|
||||
},
|
||||
expectedCode: jsonrpc.CodeInvalidParams,
|
||||
},
|
||||
// Note: "missing required param" is tested separately below, because
|
||||
// input validation errors are returned as tool results with IsError=true
|
||||
// rather than JSON-RPC errors (see #450).
|
||||
{
|
||||
name: "unknown tool",
|
||||
executeCall: func() error {
|
||||
|
|
@ -119,6 +111,44 @@ func TestServerErrors(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
// TestInputValidationToolError validates that input validation errors (missing
|
||||
// required params, wrong types) are returned as tool results with IsError=true,
|
||||
// not as JSON-RPC errors. This allows LLMs to see the error and self-correct.
|
||||
// See #450.
|
||||
func TestInputValidationToolError(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
|
||||
type RequiredParams struct {
|
||||
Name string `json:"name" jsonschema:"the name is required"`
|
||||
}
|
||||
handler := func(ctx context.Context, req *CallToolRequest, args RequiredParams) (*CallToolResult, any, error) {
|
||||
return &CallToolResult{
|
||||
Content: []Content{&TextContent{Text: "success"}},
|
||||
}, nil, nil
|
||||
}
|
||||
|
||||
cs, _, cleanup := basicConnection(t, func(s *Server) {
|
||||
AddTool(s, &Tool{Name: "validate", Description: "validates params"}, handler)
|
||||
})
|
||||
defer cleanup()
|
||||
|
||||
// Call the tool with missing required "name" field.
|
||||
result, err := cs.CallTool(ctx, &CallToolParams{
|
||||
Name: "validate",
|
||||
Arguments: map[string]any{},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("CallTool returned error: %v; want tool result with IsError", err)
|
||||
}
|
||||
if !result.IsError {
|
||||
t.Fatal("got IsError=false, want IsError=true for missing required param")
|
||||
}
|
||||
text := result.Content[0].(*TextContent).Text
|
||||
if !strings.Contains(text, "name") {
|
||||
t.Errorf("error text %q does not mention missing field \"name\"", text)
|
||||
}
|
||||
}
|
||||
|
||||
// TestURLElicitationRequired validates that URL elicitation required errors
|
||||
// are properly created and handled by the client.
|
||||
func TestURLElicitationRequired(t *testing.T) {
|
||||
|
|
|
|||
|
|
@ -321,15 +321,18 @@ func toolForErr[In, Out any](t *Tool, h ToolHandlerFor[In, Out], cache *SchemaCa
|
|||
var err error
|
||||
input, err = applySchema(input, inputResolved)
|
||||
if err != nil {
|
||||
// TODO(#450): should this be considered a tool error? (and similar below)
|
||||
return nil, fmt.Errorf("%w: validating \"arguments\": %v", jsonrpc2.ErrInvalidParams, err)
|
||||
var errRes CallToolResult
|
||||
errRes.SetError(fmt.Errorf("validating \"arguments\": %v", err))
|
||||
return &errRes, nil
|
||||
}
|
||||
|
||||
// Unmarshal and validate args.
|
||||
var in In
|
||||
if input != nil {
|
||||
if err := internaljson.Unmarshal(input, &in); err != nil {
|
||||
return nil, fmt.Errorf("%w: %v", jsonrpc2.ErrInvalidParams, err)
|
||||
var errRes CallToolResult
|
||||
errRes.SetError(err)
|
||||
return &errRes, nil
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -676,12 +676,19 @@ func testToolForSchema[In, Out any](t *testing.T, tool *Tool, in string, out Out
|
|||
}
|
||||
result, err := goth(context.Background(), ctr)
|
||||
if wantErrContaining != "" {
|
||||
if err == nil {
|
||||
t.Errorf("got nil error, want error containing %q", wantErrContaining)
|
||||
} else {
|
||||
// Input validation errors are returned as tool results with IsError=true,
|
||||
// not as Go errors. Check both possibilities.
|
||||
if err != nil {
|
||||
if !strings.Contains(err.Error(), wantErrContaining) {
|
||||
t.Errorf("got error %q, want containing %q", err, wantErrContaining)
|
||||
}
|
||||
} else if result != nil && result.IsError {
|
||||
text := result.Content[0].(*TextContent).Text
|
||||
if !strings.Contains(text, wantErrContaining) {
|
||||
t.Errorf("got tool error %q, want containing %q", text, wantErrContaining)
|
||||
}
|
||||
} else {
|
||||
t.Errorf("got no error, want error containing %q", wantErrContaining)
|
||||
}
|
||||
} else if err != nil {
|
||||
t.Errorf("got error %v, want no error", err)
|
||||
|
|
|
|||
22
mcp/testdata/conformance/server/tools.txtar
vendored
22
mcp/testdata/conformance/server/tools.txtar
vendored
|
|
@ -222,9 +222,14 @@ inc
|
|||
{
|
||||
"jsonrpc": "2.0",
|
||||
"id": 8,
|
||||
"error": {
|
||||
"code": -32602,
|
||||
"message": "invalid params: validating \"arguments\": validating root: required: missing properties: [\"In\"]"
|
||||
"result": {
|
||||
"content": [
|
||||
{
|
||||
"type": "text",
|
||||
"text": "validating \"arguments\": validating root: required: missing properties: [\"In\"]"
|
||||
}
|
||||
],
|
||||
"isError": true
|
||||
}
|
||||
}
|
||||
{
|
||||
|
|
@ -245,9 +250,14 @@ inc
|
|||
{
|
||||
"jsonrpc": "2.0",
|
||||
"id": 10,
|
||||
"error": {
|
||||
"code": -32602,
|
||||
"message": "invalid params: validating \"arguments\": validating root: required: missing properties: [\"Name\"]"
|
||||
"result": {
|
||||
"content": [
|
||||
{
|
||||
"type": "text",
|
||||
"text": "validating \"arguments\": validating root: required: missing properties: [\"Name\"]"
|
||||
}
|
||||
],
|
||||
"isError": true
|
||||
}
|
||||
}
|
||||
{
|
||||
|
|
|
|||
Loading…
Reference in a new issue