Skip to content

Fix data-URL strings decoding as binary values - #295

Open
EhsanAzish80 wants to merge 2 commits into
modelcontextprotocol:mainfrom
EhsanAzish80:fix/preserve-data-url-json-strings
Open

EhsanAzish80 wants to merge 2 commits into
modelcontextprotocol:mainfrom
EhsanAzish80:fix/preserve-data-url-json-strings

Conversation

@EhsanAzish80

Copy link
Copy Markdown

Fixes #277. JSON strings that look like data URLs now stay strings, so tool text is preserved when it passes through Value. This is an independently tested alternative to #278.

Tests: 554 Swift tests passed locally. AI was used to write the tests. I ran them locally.

Copy link
Copy Markdown

This fixes the accidental data-URL coercion, but it also makes the public Value.data case non-round-trippable through its own Codable conformance.

Value.data still encodes as a plain data-URL string:

case let .data(mimeType, value):
    try container.encode(value.dataURLEncoded(mimeType: mimeType))

while this PR now decodes every JSON string as .string. So:

let original = Value.data(mimeType: "application/octet-stream", Data([0, 1, 2]))
let decoded = try JSONDecoder().decode(Value.self, from: JSONEncoder().encode(original))

changes type from .data to .string. The new test explicitly codifies that asymmetry.

If generic JSON strings must never be auto-promoted to binary (which makes sense), could Value.data use an explicit tagged representation for encoding/decoding, or otherwise be removed/documented as non-Codable-lossless? As it stands, persistence or forwarding of a Value.data through JSON silently loses the enum case.

AI-assisted review; checked the current PR head and Value Codable paths before posting.

@EhsanAzish80

Copy link
Copy Markdown
Author

I kept JSON strings as strings and documented that Value.data is not lossless through generic JSON decoding. The new test shows how to recover the bytes with Data.parseDataURL. All 554 Swift tests passed locally. AI helped write the test; I ran it. Could a maintainer review this choice and approve CI?

@varun06

varun06 commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Hi @EhsanAzish80, thanks for this PR! I've reviewed it and it looks good to merge.

Could you please rebase it onto the latest main? CI was broken on macOS runners for every PR (a toolchain/SDK mismatch, fixed in #306), so rebasing will pick up the fix and give us a green run. I'll review and merge once it's updated.

If you're no longer able to work on this, no problem: just let us know. If we don't hear back in about two weeks, we may carry the change forward in a new PR, with credit to you.

@EhsanAzish80
EhsanAzish80 force-pushed the fix/preserve-data-url-json-strings branch from b11b38a to f7c51f7 Compare October 22, 2026 15:28
@EhsanAzish80
EhsanAzish80 force-pushed the fix/preserve-data-url-json-strings branch from f7c51f7 to a925398 Compare October 22, 2026 16:49
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.

Value decoding changes data-URL-looking JSON strings into Value.data

3 participants