Replace FileResource.is_binary with an encoding field#3171
Conversation
FileResource guessed is_binary from mime_type with a text/* prefix check, and because False doubled as the "not given" sentinel an explicit is_binary=False could never take effect, so application/json files were always served as base64 blobs. Text reads also used Path.read_text() with no encoding, i.e. the platform locale. The field is now encoding: str | None. A string decodes the file and serves it as text; None reads bytes and serves a blob. When omitted the default comes from mime_type: the declared charset if present, else utf-8 for textual types (text/*, +json/+xml suffixes, application/json, xml, javascript), else None. Resource models now reject unknown keyword arguments, so passing the removed is_binary= fails at construction instead of being ignored.
📚 Documentation preview
|
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Beyond the inline doc nit, a few other candidates were examined and ruled out: an invalid charset= in mime_type (e.g. file(1)-style charset=binary) fails at read time inside the existing Error reading file ValueError path rather than silently misbehaving; extra="forbid" doesn't break any in-repo construction site (the only dynamic one, ResourceTemplate.create_resource, passes only declared fields); and the data-aware default_factory signature needs pydantic ≥ 2.10, which the pydantic>=2.12.0 floor satisfies.
Extended reasoning...
A single nit-level documentation bug was found and is posted inline. This note only records the additional candidates examined and refuted this run — the charset-as-codec concern (fails loudly via the existing read-error wrapping), extra="forbid" compatibility with internal Resource construction paths, and the pydantic version floor for the data-aware default_factory. It is informational for later review passes, not a correctness guarantee. The PR itself is a deliberate breaking API change (is_binary → encoding, extra="forbid" on Resource), documented in docs/migration.md per repo policy, so it should still get a human look.
The set of application/* types treated as text is now just json and xml, the two ubiquitous un-suffixed textual types the structural rules (text/*, +json/+xml) can't reach. application/javascript and application/ecmascript are gone: RFC 9239 made text/javascript canonical, which the text/* rule already covers. The inferred text encoding is now utf-8-sig, so a byte-order mark is dropped rather than surfacing as a leading U+FEFF that JSON parsers reject. A declared charset= is still used as-is. The encoding is validated with codecs.lookup at construction, so a typo fails at registration instead of on the first read. The extra="forbid" change gets its own migration section, since it applies to every Resource class, not just FileResource.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Beyond the inline finding, this run also examined whether a charset= parameter overriding a binary mime essence (e.g. image/png; charset=utf-8) could break real-world configurations — ruled out: a charset parameter on a binary type is user-declared and honoring it is the intended override path. Also checked that the encoding default_factory's data["mime_type"] lookup is safe — mime_type is defined on the base class so it is always validated (including its default) before encoding. The earlier application/ecmascript doc/code mismatch was resolved by shrinking the textual mime set in 769fd84.
Extended reasoning...
Bugs were found this run (one nit: the encoding validator accepts non-text transform codecs like base64/hex that then fail on first read), so the inline comment carries the finding and this note only records what else was examined. The refuted charset-over-binary-essence candidate and the default_factory ordering check are worth recording so a later pass does not re-explore them. The prior run's ecmascript inline nit was addressed by commit 769fd84, which removed application/javascript and application/ecmascript from _TEXTUAL_APPLICATION_TYPES and aligned migration.md with the code.
Validate encoding by decoding a probe byte instead of codecs.lookup, so registered-but-not-text codecs (base64_codec, rot13, hex) are rejected at construction alongside unknown names, rather than passing validation and failing on read. Scope the migration guide's "flips the other way" example to a newly textual type (application/xml) - text/* files were never blobs in v1 - and call out separately that text/* files previously decoded with the platform locale rather than UTF-8.
There was a problem hiding this comment.
No bugs found this pass — the earlier findings (non-text transform codecs passing validation, the application/ecmascript doc mismatch) are resolved by the probe-byte validator and the shrunken textual set. Deferring only because this is a deliberate public-API redesign — removing FileResource.is_binary and putting extra="forbid" on the Resource base, which now affects every subclass including user-defined ones — so a maintainer should make the final call on the design.
What was reviewed:
- The default-encoding matrix (charset param > utf-8-sig for textual types > None) against the docstring, migration guide, and tests — all three agree after
0c7c333. - The charset parsing for quoted values and multi-parameter mime types, and a candidate concern that
charset=on a binary mime type flips it to text — that only happens when the server author declares the charset themselves, andencoding=Noneoverrides it. - The
b'x'.decode(encoding)probe: rejects unknown names and transform codecs via LookupError, tolerates multi-byte codecs via the UnicodeError escape — covered by the utf-16 and base64_codec tests. - The
default_factory(data)mechanism: pydantic floor is >=2.12 (form added in 2.10) andmime_typeis a base-class field, so it is validated beforeencoding.
Extended reasoning...
Overview
The PR replaces FileResource.is_binary: bool with encoding: str | None in src/mcp/server/mcpserver/resources/types.py, adds extra="forbid" to the Resource base model in base.py, rewrites the file-resource test module into flat test_* functions, and documents the break in docs/migration.md. The old field had a real bug (False doubled as the not-given sentinel, so application/json could never be served as text) and text reads used the platform locale; the new design fixes both.
Security risks
None identified. The change narrows behavior: unknown kwargs and bad codec names now fail at construction, the locale is never consulted, and text/blob selection is driven by an explicit field. The mime-type parsing is a simple split used only to pick a default encoding; a bad declared charset fails at construction rather than at read.
Level of scrutiny
This is a breaking change to a public server-API class on the v2 branch, where breaking changes are expected and must be documented — which this PR does. The logic itself is small and well-tested (default matrix, declared charset, BOM handling, explicit override, rejection of the removed kwarg and of non-text codecs). The reason not to shadow-approve is not correctness but design ownership: removing a public field and flipping the base model to extra="forbid" (which now rejects unknown kwargs on all seven resource classes and any user subclass) is exactly the kind of intentional API decision the repo guidelines say must be deliberate, so a maintainer should confirm it.
Other factors
Two earlier review rounds (mine and cubic's) raised four issues — the ecmascript doc/code mismatch, a doc misstatement about prior text/* behavior, and construction-time validation missing non-text transform codecs — and all were addressed in 769fd84 and 0c7c333 with matching tests. Finder agents this run raised two further candidates (unconditional charset honoring; quoting-unaware split(';')) and verifiers refuted both. I could not execute the test suite in this environment (uv version mismatch), so my verification of the validator and default_factory behavior is by reading, plus the pydantic changelog for the default_factory(data) form against the >=2.12 floor.
FileResourceguessedis_binaryfrommime_typewith atext/*prefix check, and becauseFalsedoubled as the "not given" sentinel an explicitis_binary=Falsecould never take effect, somime_type="application/json"was always served as a base64 blob. Text reads also calledPath.read_text()with no encoding, i.e. the platform locale (cp1252 on Windows).Motivation and Context
Rather than adding a
Nonesentinel on top of the boolean, this replaces it withencoding: str | None, the single field the read path actually consults, mirroring how the sibling SDKs let the return type (string vs bytes) carry the text-vs-blob decision:Nonereads bytes and serves a blobmime_type: the declaredcharset=if there is one, else"utf-8"for textual types (text/*,application/json/xml/javascript, any+json/+xmlsuffix), elseNoneSo JSON and XML files come back as text with no configuration, the locale is never consulted, and the ambiguous state is gone rather than special-cased.
Resourcemodels now reject unknown keyword arguments (extra="forbid"), so passing the removedis_binary=fails at construction instead of being silently swallowed and blowing up later at read time.How Has This Been Tested?
Unit tests cover the new default matrix (textual/binary mime types, declared charset, explicit override, rejection of
is_binary). Also drove it end-to-end through an in-memoryClientreadingFileResources off anMCPServer:Breaking Changes
Yes:
FileResource.is_binaryis removed in favour ofencoding, andResourceclasses now reject unknown kwargs. Documented indocs/migration.mdwith before/after examples.Types of changes
Checklist
AI Disclaimer