[Mahdi-Hasan]

Description

MCPClient.executeToolCall only read result.content[0]. Text in later blocks was dropped, and a leading image block reached the model as raw base64 JSON. Fixes #1932.

Every block is now formatted, in order, and joined with newlines:

Block	Output
text	the text
image, audio	[image/png, 3 bytes] (MIME type and decoded size, no base64; a data: prefix and line breaks are ignored when measuring)
resource with text	[resource <uri>] then the text
resource with blob	[resource file:///b.bin, application/zip, 2 bytes]
resource_link	[resource link: file:///c.md]
anything else	JSON, as before

• Empty content still returns Tool executed successfully (no output).
• The completion log now includes blockCount.
• Works with fix(mcp): report an MCP isError result as a failed tool call #1926: the formatted text goes through its output(...) wrapper, so a failed call still returns {llmContent, isError: true}.
• Note: fix(mcp): keep every content block in MCP tool results #1948 also targets this issue. I claimed [Bug] MCP tool results keep only the first content block #1932 on the issue and addyCooks asked me to pick it up.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Changeset

• Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

• New features include passing tests in .spec.ts/tsx files

• All existing tests pass (pnpm test:all completes successfully)

• Tests cover both success and error scenarios

• 6 new tests in source/mcp/mcp-client.spec.ts: multiple text blocks, image and audio with later text, text and blob resources plus a resource link, data URL and wrapped base64 sizes, unknown blocks, empty content.

• The text-block and image tests fail on main with the exact output from the issue.

• mcp-client.spec.ts: 91 pass, 8 skipped (live tests). Types, biome and knip pass.

• Developed on Windows, where pnpm test:ava has unrelated failures on main (POSIX paths). CI on Linux should settle it.

Manual Testing

• Tested with Ollama
• Tested with OpenRouter
• Tested with OpenAI-compatible API
• Tested MCP integration (if applicable)

Checklist

• If this was for an open issue, I was assigned to it (claimed on the issue, assignment pending)
• Code follows project style guidelines
• Self-review completed
• Documentation updated (if needed)
• No breaking changes (or clearly documented)
• Appropriate logging added using structured logging (see CONTRIBUTING.md)

[github-actions]

nc-review: needs work — 1 blocking, 2 nits

@Mahdi-Hasan — there is a blocking item below.

The change replaces MCPClient.executeToolCall's result.content[0] read with a per-block formatter that joins every content block with newlines, summarising image/audio/blob-resource blocks as short placeholders instead of dumping base64, and it adds five colocated regression tests plus a correctly-named patch changeset. The implementation itself is sound and matches the issue's suggested fix; the blocker is not in the code but in the queue: PR #1948 is already open with the identical title, branch name and target issue, so two conflicting implementations of the same function are in flight. Resolve the duplicate (per the author's note, a maintainer asked them to pick it up, so closing #1948 is likely the intended resolution) before merging.

Possible duplicate of #1948 — worth checking before going further.

🔴 blocking · duplicate

PR #1948 (fix/issue-1932-bug-mcp-tool-results-keep-only-the-first, by ShryukGrandhi) is already open with the identical title — fix(mcp): keep every content block in MCP tool results — and the same target issue #1932. Both change the same function in the same file (executeToolCall in source/mcp/mcp-client.ts), so whichever lands second will conflict.

The author is explicit about this in the PR description ("Note: #1948 also targets this issue. I claimed #1932 on the issue and addyCooks asked me to pick it up"), which suggests the duplication is already known and sanctioned — so the resolution is likely a maintainer closing #1948 rather than a change to this diff. Please confirm that before merge so the queue does not end up with two competing implementations of the same formatter.

⚪ nit · correctness · source/mcp/mcp-client.ts:76

An embedded resource carrying text is rendered as the bare text, dropping its uri and mimeType:

if (typeof resource.text === 'string') {
    return resource.text;
}

When a tool returns two embedded resources from different files, the two texts are concatenated with no marker, so the model cannot tell which file each came from. Prefixing with the URI (the blob branch already emits it) — e.g. [resource file:///a.txt] hello — would keep the provenance without reintroducing binary payloads.

⚪ nit · correctness · source/mcp/mcp-client.ts:53

base64ByteLength derives the decoded size purely from the string length and a trailing-= check:

const padding = data.endsWith('==') ? 2 : data.endsWith('=') ? 1 : 0;
return Math.max(0, Math.floor((data.length * 3) / 4) - padding);

The MCP spec requires raw base64, so this is right for conforming servers, but servers that send a data URL (data:image/png;base64,...) or wrap the payload across lines get a wrong byte count in the placeholder (e.g. data:image/png;base64,AAA= reports 18 bytes instead of 2). The security property the fix is after still holds — the payload is summarised, never dumped — so this only affects the accuracy of the printed size. Stripping a data: prefix and whitespace before measuring would fix it.

---

_{🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional}

_{Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.}

[Mahdi-Hasan]

Addressed the nc-review nits in 29546c8:

• Embedded text resources now start with a [resource <uri>] line, so texts from different resources stay apart.
• base64ByteLength strips a data: prefix and whitespace before measuring. New test covers both.

The duplicate flag (#1948) is for a maintainer to decide. I claimed #1932 on the issue, as addyCooks asked.

[addyCooks]

Thanks @Mahdi-Hasan, the block formatting does what #1932 asks, and mcp-client.spec.ts passes on this branch (91 tests). The branch now conflicts with main in source/mcp/mcp-client.ts, because #1926 (the isError flag) and #1651 changed the same function. Could you merge main and:

• keep your formatToolResultContent(result.content), and return it through #1926's output(...) wrapper so a failed call still returns {llmContent, isError: true}
• update the test "a non-text first part keeps its JSON text and is still flagged": with your change its image block now becomes [image/png, 1 bytes] instead of JSON, which is the intended behaviour, so the expectation needs updating

Happy to re-review once it's updated.

[Mahdi-Hasan]

@addyCooks Thanks! Merged main in addd206:

• executeToolCall keeps formatToolResultContent(result.content) and returns it through fix(mcp): report an MCP isError result as a failed tool call #1926's output(...), so a failed call still returns {llmContent, isError: true}.
• The "non-text first part" case now expects [image/png, 1 bytes]. I also added a failed call with a text block plus an image, which keeps both blocks and stays flagged.
• Type check and Biome pass. mcp-client.spec.ts passes (92), and all MCP specs pass (125). Without the output(...) wrapper, the isError test fails.

I also removed the "Generated with Claude Code" line from this description, as on #1961.

[addyCooks]

Thanks @Mahdi-Hasan, that covers it. I checked this merged with current main: the type check and Biome pass, the MCP, tool-manager and tool-catalog specs pass (207), and the new tests fail without the change. A failed call with several blocks still returns {llmContent, isError: true} through #1926's output(...).

Approving 🎉