ROB-3756 - Extract text from MCP resource content blocks in tool results - #1961
Conversation
_invoke_async only collected TextContent blocks, dropping any EmbeddedResource the server returned. The github MCP server's get_file_contents returns the file body inside a ResourceContents (EmbeddedResource), so Holmes saw only the "successfully downloaded text file (SHA: ...)" preamble and the LLM never received the file content. Same problem for files >= 1MB, which come back as a ResourceLink, and for any other MCP server using the resource pattern. Now extract: - TextResourceContents.text directly - BlobResourceContents.blob (base64) decoded when mimeType is text-like - ResourceLink uri/name as a hint Signed-off-by: Claude <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
WalkthroughUpdate MCP tool result extraction to support content block types Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
naomi-robusta
changed the title
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📂 Previous Runs📜 #3 · Run @ __96b0fe8__ (#25056178169) — Apr 28, 13:47 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 96b0fe8 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 74 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #2 · Run @ __dd6e4c6__ (#25053290326) — Apr 28, 12:57 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit dd6e4c6 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 74 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #1 · Run @ __6448abe__ (#25051867509) — Apr 28, 12:17 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 6448abe on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 74 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
✅ Results of HolmesGPT evalsAutomatically triggered by commit 5b87478 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 74 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals (applies to both automatic runs and
Examples: 🏷️ Valid tags
🤖 Valid models
Commands: CLI: |
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:ce1f3796
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:ce1f3796 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:ce1f3796
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:ce1f3796
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:ce1f3796
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:ce1f3796 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:ce1f3796
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:ce1f3796Patch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:ce1f3796 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:ce1f3796Robusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:ce1f3796 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:ce1f3796 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_mcp_toolset.py (1)
1232-1232: Movebase64imports to module scopeThese function-local imports should be hoisted to the top-level import block.
♻️ Suggested cleanup
import asyncio +import base64 import copy import logging @@ - import base64 as _b64 - file_body = '{"hello": "world"}' - encoded = _b64.b64encode(file_body.encode("utf-8")).decode("ascii") + encoded = base64.b64encode(file_body.encode("utf-8")).decode("ascii") @@ - import base64 as _b64 - - encoded = _b64.b64encode(b"\x89PNG\r\n\x1a\n").decode("ascii") + encoded = base64.b64encode(b"\x89PNG\r\n\x1a\n").decode("ascii")As per coding guidelines "ALWAYS place Python imports at the top of the file, not inside functions or methods".
Also applies to: 1256-1256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_toolset.py` at line 1232, Hoist the function-local "import base64 as _b64" statements into the module-level import block (keeping the alias _b64) and remove the local imports inside the test functions; ensure the top-level imports appear with the other imports and that any references to _b64 in the functions still work without local imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/toolsets/mcp/toolset_mcp.py`:
- Around line 471-475: The code currently fully decodes text-like base64 blobs
(base64.b64decode(blob).decode("utf-8", errors="replace")) and can return
arbitrarily large strings; add a decode-size guard (e.g., MAX_DECODE_BYTES) and
only decode and return up to that limit, include a clear truncation indicator
and the original size/uri/mime in the returned string, and for blobs larger than
the limit return a short summary like the existing "[binary resource ...]"
message with base64_size and a note that the decoded text was truncated; update
the handling around the base64.b64decode(...).decode(...) call and the fallback
return that uses uri, mime, and len(blob) to reflect truncation.
- Around line 476-482: The code returns raw resource_link URIs which can expose
sensitive query params; update the resource_link branch (the block_type ==
"resource_link" code that uses variables uri, name, title, label) to redact
query strings and fragments before returning the URI — e.g., parse the uri and
drop any query and fragment components (or split at '?'/'#') so only the
scheme/host/path remain, then construct the return string using that sanitized
URI and the existing label logic.
---
Nitpick comments:
In `@tests/test_mcp_toolset.py`:
- Line 1232: Hoist the function-local "import base64 as _b64" statements into
the module-level import block (keeping the alias _b64) and remove the local
imports inside the test functions; ensure the top-level imports appear with the
other imports and that any references to _b64 in the functions still work
without local imports.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 319e96eb-5072-4e5a-989d-78d9ca75ea7f
📒 Files selected for processing (2)
holmes/plugins/toolsets/mcp/toolset_mcp.pytests/test_mcp_toolset.py
🔬 CLI Performance Benchmark🔴 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
CLAUDE.md requires imports at the top of the file. Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_mcp_toolset.py`:
- Around line 1251-1294: Update the two tests to assert the exact placeholder
formats rather than loose substring checks: in
test_invoke_async_keeps_binary_blob_as_placeholder, replace the two loose
asserts against result.data with a single assertion that result.data equals (or
contains) the full binary placeholder emitted by the extractor (include the
exact placeholder token + the MIME and the resource URI as produced by the code
that handles BlobResourceContents/EmbeddedResource); in
test_invoke_async_surfaces_resource_link, replace the loose URI and message
checks with an assertion that result.data contains the exact ResourceLink
placeholder format (include the URI and the filename/name field from
ResourceLink and the surrounding structured wrapper the extractor emits). Locate
these tests by name (_run_invoke_with_content, BlobResourceContents,
ResourceLink, EmbeddedResource, StructuredToolResultStatus) and assert the full
expected placeholder strings instead of partial substrings.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fd20d43-2452-423e-a5d3-458f17c52321
📒 Files selected for processing (1)
tests/test_mcp_toolset.py
Substring checks in the binary-blob and resource_link tests would pass even if the wrapper format degraded: the resource_link test was asserting "too large to display" — text emitted by the upstream TextContent block, not the resource_link extractor — so a broken [resource_link <name>: <uri>] wrapper would not have been caught. Tighten both to full-string equality. Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_mcp_toolset.py (1)
2-2: Use a more descriptivebase64import alias.
_b64is terse for a module alias in test code;base64(orb64) would be clearer and still concise.As per coding guidelines "Use semantic, descriptive names for variables, functions, and components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_toolset.py` at line 2, The import alias `_b64` in tests/test_mcp_toolset.py is non-descriptive; replace the import "import base64 as _b64" with a clearer alias such as "import base64" or "import base64 as b64" and then update every usage of `_b64` in the file to the new name (e.g., base64.b64encode/base64.b64decode or b64.b64encode/b64.b64decode) so all references match the updated import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_mcp_toolset.py`:
- Line 2: The import alias `_b64` in tests/test_mcp_toolset.py is
non-descriptive; replace the import "import base64 as _b64" with a clearer alias
such as "import base64" or "import base64 as b64" and then update every usage of
`_b64` in the file to the new name (e.g., base64.b64encode/base64.b64decode or
b64.b64encode/b64.b64decode) so all references match the updated import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c82d630-ccca-4e97-b52b-e9f20657d251
📒 Files selected for processing (1)
tests/test_mcp_toolset.py
|
us-central1-docker.pkg.dev/genuine-flight-317411/devel/holmes:github-mcp-get-file-content-fix |
3 participants
Summary
This PR fixes a bug where MCP tool results containing file contents in
EmbeddedResourceblocks were silently dropped, preventing the LLM from accessing the actual data. The fix extracts text from all MCP content block types and properly handles different resource formats.Key Changes
Added
_extract_text_from_content_block()method toRemoteMCPToolthat handles extraction from:TextContent: Direct text passthroughEmbeddedResourcewithTextResourceContents: Extracts the text fieldEmbeddedResourcewithBlobResourceContents: Base64-decodes when mimeType indicates text (text/*, application/json, application/xml, etc.), otherwise returns a placeholder with resource metadataResourceLink: Surfaces the URI as a hint for large files (>1MB)Updated
_invoke_async()method to use the new extraction logic instead of only filtering forTextContentblocksAdded comprehensive test coverage with four new test cases:
test_invoke_async_extracts_text_resource_contents: Verifies text extraction fromTextResourceContentstest_invoke_async_decodes_text_blob_resource: Verifies base64 decoding of text-like blobstest_invoke_async_keeps_binary_blob_as_placeholder: Verifies binary blobs emit a placeholder instead of failingtest_invoke_async_surfaces_resource_link: VerifiesResourceLinkURIs are surfacedImplementation Details
getattr) to safely handle different content block types without strict type checkingtext/prefix, specific types likeapplication/json, and+json/+xmlsuffixeserrors='replace'to gracefully handle malformed UTF-8[binary resource uri=... mimeType=... base64_size=...]to provide context without attempting text conversion[resource_link label: uri]or[resource_link: uri]depending on availability of name/titleThis resolves the issue where tools like GitHub's
get_file_contentswould appear to succeed but deliver no usable content to the LLM.https://claude.ai/code/session_01M3RAEYpyaVQuRQifYUhwEf
Summary by CodeRabbit
New Features
Bug Fixes
Tests