fix(mcp): name the non-text blocks a tool result drops - #874
fix(mcp): name the non-text blocks a tool result drops#874Vasanthdev2004 wants to merge 2 commits into
Conversation
TextContent keeps only blocks whose type is "text", so a server that returns an image, audio, or an embedded resource produced "(empty MCP tool result)". A screenshot or browser server would complete a call successfully, hand back a perfectly good image, and the model would be told nothing came back. It then usually retries, which spends another call to reach the same empty answer, and the user never learns an image existed at all. Carrying the payload is the larger half of #823 and is not this change. Dropping silently is the part that causes the retry loop, and it can be fixed on its own. Decode mimeType on Content, and add DroppedContentSummary, which describes what TextContent discarded ("1 image/png block", "2 resource blocks, 1 audio/wav block"), grouping by mime type and ordering by first appearance so the same result always reads the same way. Servers that omit mimeType fall back to the block type rather than printing an empty label. registryTool.Run appends that as a note, and only when something was actually dropped, so a text-only result is byte-for-byte unchanged. The note also says retrying will return the same thing, since telling the model what happened without telling it not to retry only fixes half the loop. Refs #823.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe MCP client now preserves optional MIME metadata and summarizes dropped non-text content. ChangesMCP dropped content reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant registryToolRun
participant Result
MCPClient->>registryToolRun: return MCP content blocks
registryToolRun->>registryToolRun: summarize unsupported non-text blocks
registryToolRun->>Result: append dropped-content note when applicable
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/mcp/non_text_content_test.go (1)
30-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the retry guidance.
The test verifies that the output is non-empty and names the MIME type. It does not verify the retry guidance. Add an assertion for the corrected message so this behavior cannot regress.
Proposed test
if !strings.Contains(result.Output, "image/png") { t.Errorf("the output does not name what the server returned:\n%s", result.Output) } + if !strings.Contains(result.Output, "Retrying cannot recover this payload.") { + t.Errorf("the output does not contain retry guidance:\n%s", result.Output) + }As per coding guidelines, add a regression test for behavior changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/mcp/non_text_content_test.go` around lines 30 - 35, Extend the assertions in the non-text content test to verify that result.Output contains the corrected retry guidance message, alongside the existing non-empty and MIME-type checks. Use the established expected wording from the implementation rather than adding a new message.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/mcp/registry.go`:
- Around line 333-338: The retry guidance assembled in DroppedContentSummary
handling within registryTool.Run must not claim retries return the same result;
replace that wording with “Retrying cannot recover this payload.” Update
internal/mcp/registry.go lines 333-338 and adjust the image-only result
assertion in internal/mcp/non_text_content_test.go lines 30-35 to verify the
corrected message.
---
Nitpick comments:
In `@internal/mcp/non_text_content_test.go`:
- Around line 30-35: Extend the assertions in the non-text content test to
verify that result.Output contains the corrected retry guidance message,
alongside the existing non-empty and MIME-type checks. Use the established
expected wording from the implementation rather than adding a new message.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97f552dc-1eef-46ae-b884-152993cde36f
📒 Files selected for processing (3)
internal/mcp/client.gointernal/mcp/non_text_content_test.gointernal/mcp/registry.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: Approve
Checked out and verified rather than read. The defect is real and the fix is the smallest thing that ends it.
The claim holds. TextContent keeps only text blocks, so an image-only result reached the model as (empty MCP tool result) — which reads as "the tool produced nothing", and a model that believes that retries. Naming the payload ends the loop even though the bytes still cannot be forwarded, and it says so ("Retrying will return the same thing"), which is the part that actually stops the retry.
Gut-the-fix: forcing DroppedContentSummary to return "" fails three tests — TestAnImageOnlyResultSaysWhatItReturned, TestTextAlongsideAnImageStillReportsTheImage, TestDroppedContentSummaryNamesTheBlocks. Load-bearing, and the wiring in registry.go is covered too, not just the helper.
Additivity: I verified a text-only result produces "", so the note is appended only when something was genuinely dropped and an all-text call is byte-for-byte what it was. nil content likewise, so (empty MCP tool result) still appears for a genuinely empty result rather than being replaced by a confusing note.
Edge cases I probed directly, all correct:
- a block with neither
mimeTypenortypeyields1 unknown block, not an empty label - grouping and pluralisation:
2 image/png blocks, 1 resource block - ordering is by first appearance, so the same result always produces the same sentence — worth having, since a map-ordered version would produce a different string each run and make the output non-deterministic for no reason
Two small things, neither blocking:
MimeTypeis decoded but never forwarded. That is stated in the comment, so it is deliberate — worth a follow-up issue reference so it does not become a field nobody remembers is unused.- The message says "Zero cannot forward yet". Now that #843 has landed an image channel on tool results, that sentence has a shelf life. Not this PR's job, but the two will need reconciling.
go test ./internal/mcp/ green on the branch.
The dropped-block note said "Retrying will return the same thing". registryTool.Run makes a fresh remote call on every retry, so the server may well answer differently; that is a promise this code is in no position to make. What cannot change is that Zero still has nowhere to put a non-text block, so the note now says retrying cannot RECOVER the payload. Same guidance, a claim that is actually true. The test pins both directions: the new wording must be present, and the absolute claim must not come back. Raised by CodeRabbit on #874.
|
Fixed in 55d7cc3, and the point was correct.
|
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-approving after 55d7cc37. My earlier approval was auto-dismissed by the push; the new commit only improves things.
The wording change is the right correction, and a subtle one. Retrying will return the same thing was a claim this code is in no position to make — each retry is a fresh call and the server may well answer differently. Retrying cannot recover this payload is true regardless of what the server does, because the thing that cannot change is that Zero has nowhere to put a non-text block. Same steer to the model, without the false promise.
Verified at 55d7cc37:
go test ./internal/mcp/green.- Mutation-checked the new assertions: restoring the old
will return the same thingwording inregistry.go:340,342failsTestAnImageOnlyResultSaysWhatItReturnedon both lines — the missingcannot recoverguidance and the reintroduced identical-response promise. The test pins the wording in both directions, so neither half can rot silently.
Nothing further from me.
3 participants
Part 1 of #823. It stops the silent drop. It does not carry the payload yet.
Why this half is worth landing alone
TextContentkeeps only blocks whose type istext, so a server returning an image, audio, or an embedded resource produced(empty MCP tool result). A screenshot or browser server completes the call, hands back a perfectly good image, and the model is told nothing came back. It then usually retries, spending another call to reach the same empty answer, and the user never learns an image existed.Naming the dropped blocks ends that loop without moving a single byte of payload.
The change
ContentdecodesmimeType. Additive andomitempty, so servers that never send it decode exactly as before.DroppedContentSummarydescribes whatTextContentdiscarded:1 image/png block,2 resource blocks, 1 audio/wav block. Grouped by mime type, ordered by first appearance, so the same result always reads the same way. A server that omitsmimeTypefalls back to the block type rather than printing an empty label.registryTool.Runappends that as a note, and only when something was actually dropped, so a text-only result is byte-for-byte unchanged.The note also says retrying will return the same thing. Telling the model what happened without telling it not to retry only fixes half the loop.
What is still open on #823
Carrying image blocks through. That got easier today: #843 landed
zeroruntime.ImageBlock,Message.Images,CloneImageBlocksandSeedMessagesWithImages, which is the plumbing the issue says to reuse. The MCP side now only has to fill it. I have left #823 open for that.Tests
non_text_content_test.godrivesregistryTool.Run, not the helper alone, because the property has to be true of the delivered result. Covers image-only, text-alongside-image, text-only unchanged, and the summary's own cases including a missing mime type and repeated blocks.Mutation-checked: disabling the note block fails
TestAnImageOnlyResultSaysWhatItReturnedandTestTextAlongsideAnImageStillReportsTheImage, whileTestATextOnlyResultIsUnchangedcorrectly stays green.go build ./...,GOOS=windows go build ./...,go vet,gofmt, and the fullinternal/mcpandinternal/toolssuites are clean.Summary by CodeRabbit
New Features
Bug Fixes