diff --git a/internal/mcp/client.go b/internal/mcp/client.go index 826c3f1..c7033d4 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -257,7 +257,16 @@ func (c *Client) call(ctx context.Context, method string, params any, out any) e if resp.Error != nil { return fmt.Errorf("mcp: %s: %s: %w", c.name, method, resp.Error) } - if out == nil || len(resp.Result) == 0 { + // A frame carrying our id and neither result nor error is not an answer. + // The HTTP transport already refuses one; the stdio transport does not, and + // without this check the refusal depended on which door the server was + // behind. Letting it through is the one failure that lies: tools/call + // returns an empty string and a nil error, so the act is recorded as done + // and the tool never ran. + if len(resp.Result) == 0 { + return fmt.Errorf("mcp: %s: %s: response carries neither result nor error", c.name, method) + } + if out == nil { return nil } if err := json.Unmarshal(resp.Result, out); err != nil { diff --git a/internal/mcp/manager.go b/internal/mcp/manager.go index 9c8c433..4250a63 100644 --- a/internal/mcp/manager.go +++ b/internal/mcp/manager.go @@ -525,10 +525,16 @@ func (m *Manager) Resources(ctx context.Context) []Resource { // ReadResource reads one resource from one server. func (m *Manager) ReadResource(ctx context.Context, server, uri string) (string, error) { - cl, cfg, _, _, _ := m.lookup(server, "") - if cl == nil { + cl, cfg, _, configured, _ := m.lookup(server, "") + if !configured { return "", fmt.Errorf("%w: %s", ErrNoServer, server) } + // A configured server that is merely down is not an unknown server. Call + // already keeps the two apart; reporting ErrNoServer here tells a caller + // the resource can never exist, when the truth is "not right now". + if cl == nil { + return "", fmt.Errorf("%w: %s", ErrNotConnected, server) + } cctx, cancel := context.WithTimeout(ctx, cfg.Timeout) defer cancel() return cl.ReadResource(cctx, uri) diff --git a/internal/mcp/mcp_test.go b/internal/mcp/mcp_test.go index 9c8c966..a1f2fbc 100644 --- a/internal/mcp/mcp_test.go +++ b/internal/mcp/mcp_test.go @@ -641,3 +641,59 @@ func TestReconnectBackoffGrows(t *testing.T) { t.Fatalf("first retry = %v, want %v", c.backoff(), DefaultReconnectEvery) } } + +// emptyFrameTransport answers the handshake normally and then replies to every +// later call with a well-formed frame carrying our id and nothing else — no +// result, no error. That is the answer a partially-implemented server gives, +// and it is the one that lies: without a check it reads as success. +type emptyFrameTransport struct{ handshaken bool } + +func (t *emptyFrameTransport) Call(ctx context.Context, req *rpcRequest) (*rpcResponse, error) { + id := req.ID + if !t.handshaken { + t.handshaken = true + raw, _ := json.Marshal(map[string]any{ + "protocolVersion": ProtocolVersion, + "serverInfo": map[string]any{"name": "empty", "version": "0"}, + }) + return &rpcResponse{JSONRPC: "2.0", ID: &id, Result: raw}, nil + } + return &rpcResponse{JSONRPC: "2.0", ID: &id}, nil +} + +func (t *emptyFrameTransport) Notify(context.Context, string, any) error { return nil } +func (t *emptyFrameTransport) Close() error { return nil } + +func TestResultlessResponseIsNotSuccess(t *testing.T) { + c := newClient("empty", &emptyFrameTransport{}) + if err := c.Initialize(context.Background()); err != nil { + t.Fatalf("initialize: %v", err) + } + out, err := c.CallTool(context.Background(), "break_thing", map[string]any{"q": "x"}) + if err == nil { + t.Fatalf("a frame with neither result nor error must not read as success (got %q)", out) + } + if out != "" { + t.Fatalf("out = %q", out) + } + if _, err := c.ListTools(context.Background()); err == nil { + t.Fatal("tools/list with no result must be an error, not an empty catalogue") + } +} + +func TestReadResourceOnDownServerIsNotConnected(t *testing.T) { + // A url server with no poster factory: configured, validated, never dialed. + m, err := NewManager(nil, []ServerConfig{{ + Name: "down", URL: "http://example.test/mcp", Enabled: true, + }}) + if err != nil { + t.Fatal(err) + } + m.Connect(context.Background()) + if _, err := m.ReadResource(context.Background(), "down", "note://one"); !errors.Is(err, ErrNotConnected) { + t.Fatalf("err = %v, want ErrNotConnected", err) + } + if _, err := m.ReadResource(context.Background(), "nosuch", "note://one"); !errors.Is(err, ErrNoServer) { + t.Fatalf("err = %v, want ErrNoServer", err) + } +}