diff --git a/internal/mcp/client.go b/internal/mcp/client.go index 665e0dfa2..064e7f213 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -26,6 +26,10 @@ type RemoteTool struct { type Content struct { Type string `json:"type"` Text string `json:"text,omitempty"` + // MimeType names what a non-text block holds. Decoded but not yet forwarded: + // it is what lets a dropped block be described to the model instead of + // vanishing (#823). Servers that omit it still decode fine. + MimeType string `json:"mimeType,omitempty"` } type CallToolResult struct { @@ -495,3 +499,52 @@ func TextContent(content []Content) string { } return strings.TrimSpace(strings.Join(parts, "\n")) } + +// DroppedContentSummary describes the blocks TextContent discards, e.g. +// "1 image/png block" or "2 resource blocks, 1 audio/wav block". It returns "" +// when a result is entirely text, so a caller adds nothing to the ordinary case. +// +// This exists because dropping silently is the worst available behaviour. A +// screenshot server returns a valid image, TextContent keeps nothing, and the +// call is reported as "(empty MCP tool result)" — so the model concludes the +// tool produced nothing and usually retries, burning another call on the same +// empty answer. Naming what came back costs nothing and ends that loop even +// though the payload still cannot be forwarded. +// +// Counts are grouped by mime type and ordered by first appearance, so the same +// result always produces the same sentence. +func DroppedContentSummary(content []Content) string { + labels := make([]string, 0, len(content)) + counts := make(map[string]int, len(content)) + for _, item := range content { + if item.Type == "text" { + continue + } + // Prefer the mime type: "image/png" tells the reader more than "image". + // A server may omit it, so fall back to the block type rather than + // printing an empty label. + label := strings.TrimSpace(item.MimeType) + if label == "" { + label = strings.TrimSpace(item.Type) + } + if label == "" { + label = "unknown" + } + if _, seen := counts[label]; !seen { + labels = append(labels, label) + } + counts[label]++ + } + if len(labels) == 0 { + return "" + } + parts := make([]string, 0, len(labels)) + for _, label := range labels { + part := fmt.Sprintf("%d %s block", counts[label], label) + if counts[label] != 1 { + part += "s" + } + parts = append(parts, part) + } + return strings.Join(parts, ", ") +} diff --git a/internal/mcp/non_text_content_test.go b/internal/mcp/non_text_content_test.go new file mode 100644 index 000000000..a1082962f --- /dev/null +++ b/internal/mcp/non_text_content_test.go @@ -0,0 +1,158 @@ +package mcp + +import ( + "context" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/tools" +) + +// A server that returns only an image currently reports "(empty MCP tool +// result)": TextContent keeps text blocks and drops the rest, so a successful +// call looks like it produced nothing. The model then usually retries, which is +// the worst outcome, and the user is never told an image existed (#823). +// +// Carrying the payload is a separate change. Naming what was dropped is what +// stops the retry loop, and it has to be true of the DELIVERED result, so this +// drives registryTool.Run rather than the helper alone. +func TestAnImageOnlyResultSaysWhatItReturned(t *testing.T) { + tool := registryTool{ + client: &nonTextClient{content: []Content{ + {Type: "image", MimeType: "image/png"}, + }}, + server: Server{Name: "shots"}, + remote: RemoteTool{Name: "screenshot"}, + } + + result := tool.Run(context.Background(), map[string]any{}) + + if strings.Contains(result.Output, "(empty MCP tool result)") { + t.Fatalf("an image-only result still reports empty, so the model will retry:\n%s", result.Output) + } + if !strings.Contains(result.Output, "image/png") { + t.Errorf("the output does not name what the server returned:\n%s", result.Output) + } + // Naming the block is only half of it. Without the guidance the model still + // retries, which is the expensive symptom, so the wording is pinned too. + // + // "cannot recover this payload" and not "will return the same thing": every + // retry is a fresh call and the server may answer differently. What cannot + // change is that Zero has nowhere to put a non-text block. + if !strings.Contains(result.Output, "Retrying cannot recover this payload.") { + t.Errorf("the output does not tell the model retrying is pointless:\n%s", result.Output) + } + if strings.Contains(result.Output, "will return the same thing") { + t.Errorf("the output promises an identical response, which a fresh call cannot guarantee:\n%s", result.Output) + } + if result.Status != tools.StatusOK { + t.Errorf("status = %v, want OK: the call succeeded, we just cannot forward the payload", result.Status) + } +} + +// The quieter half of the same bug: when a result carries text AND an image, +// the text arrives and the image vanishes with no mention at all. +func TestTextAlongsideAnImageStillReportsTheImage(t *testing.T) { + tool := registryTool{ + client: &nonTextClient{content: []Content{ + {Type: "text", Text: "captured the page"}, + {Type: "image", MimeType: "image/png"}, + }}, + server: Server{Name: "shots"}, + remote: RemoteTool{Name: "screenshot"}, + } + + result := tool.Run(context.Background(), map[string]any{}) + + if !strings.Contains(result.Output, "captured the page") { + t.Errorf("the text block was lost:\n%s", result.Output) + } + if !strings.Contains(result.Output, "image/png") { + t.Errorf("the dropped image was not mentioned:\n%s", result.Output) + } +} + +// A text-only result must be byte-for-byte what it was before: this change adds +// a line only when something was actually dropped. +func TestATextOnlyResultIsUnchanged(t *testing.T) { + tool := registryTool{ + client: &nonTextClient{content: []Content{{Type: "text", Text: "plain answer"}}}, + server: Server{Name: "shots"}, + remote: RemoteTool{Name: "lookup"}, + } + + if got := tool.Run(context.Background(), map[string]any{}).Output; got != "plain answer" { + t.Fatalf("output = %q, want exactly %q", got, "plain answer") + } +} + +func TestDroppedContentSummaryNamesTheBlocks(t *testing.T) { + tests := []struct { + name string + content []Content + want string + }{ + { + name: "nothing dropped", + content: []Content{{Type: "text", Text: "hi"}}, + want: "", + }, + { + name: "no content at all", + content: nil, + want: "", + }, + { + name: "one image with a mime type", + content: []Content{{Type: "image", MimeType: "image/png"}}, + want: "1 image/png block", + }, + { + // A server may omit mimeType. Fall back to the block type rather than + // inventing one or printing an empty pair of slashes. + name: "one image without a mime type", + content: []Content{{Type: "image"}}, + want: "1 image block", + }, + { + name: "several of the same kind are counted, not repeated", + content: []Content{ + {Type: "image", MimeType: "image/png"}, + {Type: "image", MimeType: "image/png"}, + }, + want: "2 image/png blocks", + }, + { + // Order follows first appearance so the message is stable to read and + // to assert on. + name: "mixed kinds", + content: []Content{ + {Type: "text", Text: "ignored here"}, + {Type: "resource"}, + {Type: "audio", MimeType: "audio/wav"}, + {Type: "resource"}, + }, + want: "2 resource blocks, 1 audio/wav block", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := DroppedContentSummary(test.content); got != test.want { + t.Fatalf("DroppedContentSummary() = %q, want %q", got, test.want) + } + }) + } +} + +type nonTextClient struct { + content []Content +} + +func (client *nonTextClient) ListTools(context.Context) ([]RemoteTool, error) { return nil, nil } + +func (client *nonTextClient) CallTool(context.Context, string, map[string]any) (CallToolResult, error) { + return CallToolResult{Content: client.content}, nil +} + +func (client *nonTextClient) Close() error { return nil } diff --git a/internal/mcp/registry.go b/internal/mcp/registry.go index 8c9dac7bd..bb44900b7 100644 --- a/internal/mcp/registry.go +++ b/internal/mcp/registry.go @@ -325,6 +325,24 @@ func (tool registryTool) Run(ctx context.Context, args map[string]any) tools.Res status = tools.StatusError } output := TextContent(result.Content) + // Say what was thrown away. Without this an image-only result reads as + // "(empty MCP tool result)", the model concludes the call produced nothing + // and retries, and the user never learns an image came back (#823). The note + // is appended only when something was actually dropped, so a text-only + // result is byte-for-byte what it was before. + // + // It says retrying cannot RECOVER the payload rather than that a retry + // returns the same thing. Each retry is a fresh call, so the server may well + // answer differently; what cannot change is that Zero still has nowhere to + // put a non-text block. Claiming the response would be identical would be a + // promise this code is in no position to make. + if dropped := DroppedContentSummary(result.Content); dropped != "" { + note := "[zero] this server also returned " + dropped + ", which Zero cannot forward yet. Retrying cannot recover this payload." + if output == "" { + note = "[zero] this server returned " + dropped + ", which Zero cannot forward yet. Retrying cannot recover this payload." + } + output = strings.TrimSpace(output + "\n\n" + note) + } if output == "" { output = "(empty MCP tool result)" }