mcp: avoid panicking when a resource handler returns nil (#1176)
### What `readResource` dereferenced the handler's result before the `if res == nil` check that was meant to guard it. A `ResourceHandler` returning `(nil, nil)` therefore panicked instead of returning an error: - modern clients: `handleMultiRoundTripResult` calls `res.setResultType` on the nil result, and - older clients: `res.setDefaultCacheableValues()` runs on the nil result. Either way the `if res == nil` guard a few lines below was dead code. ### Why it matters Request handlers run in their own goroutine and nothing recovers, so the panic does not just fail the one request. It terminates the whole server process and every concurrent session. The sibling handlers already avoid this: `callTool` and `getPrompt` both guard their result with `if err == nil && res != nil` before touching it, so a `(nil, nil)` return flows up and becomes a normal error response. ### Change Move the nil check in `readResource` ahead of `handleMultiRoundTripResult` and `setDefaultCacheableValues`, so a nil result returns the existing descriptive error (`read handler returned nil information`) instead of panicking. Added a regression test that drives a resource handler returning `(nil, nil)` through the server and asserts a clean error.
This commit is contained in:
parent
64e454e35c
commit
0eb1600eef
2 changed files with 30 additions and 1 deletions
|
|
@ -127,6 +127,32 @@ func TestResourceNotFoundErrorCode(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
// TestReadResourceNilResult verifies that a resource handler returning
|
||||
// (nil, nil) yields a clean error to the client instead of a nil pointer
|
||||
// panic that crashes the server. The sibling handlers callTool and getPrompt
|
||||
// already tolerate a (nil, nil) return; readResource must do the same.
|
||||
func TestReadResourceNilResult(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
|
||||
cs, _, cleanup := basicConnection(t, func(s *Server) {
|
||||
s.AddResource(
|
||||
&Resource{URI: "file:///nil.txt", Name: "nil", MIMEType: "text/plain"},
|
||||
func(ctx context.Context, req *ReadResourceRequest) (*ReadResourceResult, error) {
|
||||
return nil, nil
|
||||
},
|
||||
)
|
||||
})
|
||||
defer cleanup()
|
||||
|
||||
_, err := cs.ReadResource(ctx, &ReadResourceParams{URI: "file:///nil.txt"})
|
||||
if err == nil {
|
||||
t.Fatal("got nil error, want non-nil error for nil resource result")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "nil information") {
|
||||
t.Errorf("got error %q, want it to mention 'nil information'", err.Error())
|
||||
}
|
||||
}
|
||||
|
||||
// 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.
|
||||
|
|
|
|||
|
|
@ -1035,6 +1035,9 @@ func (s *Server) readResource(ctx context.Context, req *ReadResourceRequest) (*R
|
|||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if res == nil {
|
||||
return nil, fmt.Errorf("reading resource %s: read handler returned nil information", uri)
|
||||
}
|
||||
if err := handleMultiRoundTripResult(req.Session, s.opts.Logger, res); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
|
@ -1042,7 +1045,7 @@ func (s *Server) readResource(ctx context.Context, req *ReadResourceRequest) (*R
|
|||
if res.resultType == resultTypeInputRequired {
|
||||
return res, nil
|
||||
}
|
||||
if res == nil || res.Contents == nil {
|
||||
if res.Contents == nil {
|
||||
return nil, fmt.Errorf("reading resource %s: read handler returned nil information", uri)
|
||||
}
|
||||
// As a convenience, populate some fields.
|
||||
|
|
|
|||
Loading…
Reference in a new issue