Skip to content

test: align read_file image-content assertions with #488#493

Merged
wonderwhy-er merged 1 commit into
mainfrom
fix/read-file-image-content-test
Jun 5, 2026
Merged

test: align read_file image-content assertions with #488#493
wonderwhy-er merged 1 commit into
mainfrom
fix/read-file-image-content-test

Conversation

@wonderwhy-er

@wonderwhy-er wonderwhy-er commented Jun 5, 2026

Copy link
Copy Markdown
Owner

What

Fixes the only failing test in the suite (test/test-file-handlers.js, Test 9).

Why

#488 intentionally added the image bytes back into read_file's MCP content array so the host model can actually see images. The test was never updated and still asserted the pre-#488 contract:

assert.ok(!imageResult.content.some(item => item.type === 'image'),
  'Image result should avoid image content item for host compatibility');

That assertion is now inverted from shipped behaviour, so the suite went red (39/40). This is a stale test, not a regression.

Change

Test 9 now asserts the current contract for both raster (PNG) and SVG reads: the content array includes an image item carrying non-empty base64 data and the correct mimeType (image/png, image/svg+xml), alongside the existing structuredContent checks (which were already correct).

No production code changes — handler behaviour is unchanged.

Note / open question

.svg is routed through ImageFileHandler (isImage: true), so read_file on an SVG emits an MCP image content block with mimeType: image/svg+xml. Not every host renders SVG image blocks, and SVG is also readable as text. This PR matches the current handler behaviour; if SVG should fall through to the text branch instead, that's a handler-side decision and a separate change.

Test

node test/test-file-handlers.js  →  ✅ all 10 tests pass (exit 0)

Summary by CodeRabbit

  • Tests
    • Enhanced image preview metadata validation for PNG and SVG files to ensure proper MIME type information and encoded image data are captured correctly.

#488 added the image bytes back into read_file's MCP `content` array so
the host model can see images, but test-file-handlers.js was never
updated and still asserted the pre-#488 contract ("avoid image content
item for host compatibility"). That assertion is now inverted from the
shipped behaviour and was the only failing test in the suite.

Update Test 9 to assert the current contract for both raster (PNG) and
SVG reads: the `content` array includes an `image` item carrying base64
`data` and the correct `mimeType` (image/png, image/svg+xml), alongside
the existing structuredContent checks.

No production code change — handler behaviour is unchanged.
@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 96060531-2887-48c8-a101-51c4647954b8

📥 Commits

Reviewing files that changed from the base of the PR and between f53f916 and f8e827c.

📒 Files selected for processing (1)
  • test/test-file-handlers.js

📝 Walkthrough

Walkthrough

Test assertions for image file preview metadata are updated to expect image-typed content items with specific MIME types and base64-encoded data for both PNG and SVG files, reflecting a new image preview contract.

Changes

Image preview content assertions

Layer / File(s) Summary
PNG and SVG preview expectations
test/test-file-handlers.js
For PNG and SVG previews, test now locates an image item in content, verifies its mimeType (image/png or image/svg+xml), and confirms non-empty base64 data.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Possibly related PRs

Suggested labels

size:L

Poem

🐰 A test that once doubted the image so bright,
Now trusts the base64 with all of its might,
PNG and SVG, side by side,
Their MIME types verified with pride! 🖼️

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: updating test assertions to align with issue #488's image-content behavior changes in the read_file functionality.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/read-file-image-content-test

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@wonderwhy-er wonderwhy-er merged commit 7126346 into main Jun 5, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant