fix(mcp): name the non-text blocks a tool result drops

internal/mcp/client.go

@@ -26,6 +26,10 @@
 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 {
@@ -74,7 +78,7 @@
 	initializeTimeout = 30 * time.Second
 )
 
 func Connect(ctx context.Context, server Server) (ToolClient, error) {
 	return ConnectWithOptions(ctx, server, ConnectOptions{})
 }
 
@@ -495,3 +499,52 @@
 	}
 	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, ", ")
+}

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 }

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)"
 	}