fix: keep a malformed annotation from costing the payload - #264
Closed
galatanovidiu wants to merge 3 commits into
Closed
fix: keep a malformed annotation from costing the payload#264galatanovidiu wants to merge 3 commits into
galatanovidiu wants to merge 3 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This was referenced Jul 28, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #264 +/- ##
============================================
+ Coverage 88.17% 88.31% +0.13%
- Complexity 1259 1294 +35
============================================
Files 54 54
Lines 4120 4236 +116
============================================
+ Hits 3633 3741 +108
- Misses 487 495 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
galatanovidiu
force-pushed
the
split/3-annotations-and-degradation
branch
from
July 28, 2026 18:34
8f075c7 to
7392507
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens MCP DTO emission against malformed annotations, size, and prompt message blocks so a bad hint/block degrades locally instead of causing tool/resource registration failures or losing an entire prompt response. It also clarifies in the docs where annotations belong per component/content type (tool descriptor vs resource descriptor vs content blocks) and documents MCP Apps / embedded-resource shapes.
Changes:
- Normalize/validate content-block annotations via a shared helper (
build_content_annotations()), dropping out-of-spec annotations and coercing unambiguous scalar types. - Coerce
Resource::sizeand promptresource_link.sizeto int (and drop non-numeric values), and attach siblingannotationsforresource/imagetool results to the emitted content block. - Degrade prompt message blocks rejected by schema DTOs into text blocks containing JSON, keeping the rest of the prompt intact; expand tests and docs accordingly.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| includes/Handlers/HandlerHelperTrait.php | Adds shared content-annotations mapping/validation helper used by handlers. |
| includes/Handlers/Tools/ToolsHandler.php | Applies content-annotation normalization to embedded resource + image tool results; adds warning for image results missing results. |
| includes/Handlers/Prompts/PromptsHandler.php | Normalizes prompt content blocks (annotations, embedded resource contents, resource_link.size) and degrades schema-rejected blocks to text. |
| includes/Domain/Tools/McpTool.php | Routes tool descriptor annotations through mapper/coercion to avoid DTO failures/empty-annotations emission. |
| includes/Domain/Resources/McpResource.php | Coerces size to int and maps/validates resource annotations before DTO creation. |
| includes/Domain/Prompts/RegisterAbilityAsMcpPrompt.php | Updates inline documentation to remove unsupported prompt descriptor annotations. |
| tests/phpunit/Unit/Tools/McpToolTest.php | Adds coverage for coercion/dropping of tool-hint annotations and ensures non-annotation type errors still surface. |
| tests/phpunit/Unit/Resources/McpResourceTest.php | Adds coverage for resource annotation coercion/validation and size coercion/dropping behavior. |
| tests/phpunit/Unit/Handlers/ToolsHandlerCallTest.php | Adds coverage for tool-result annotation behavior on embedded resources/images and the image-missing-results warning path. |
| tests/phpunit/Unit/Handlers/PromptsHandlerTest.php | Adds coverage for prompt message annotation normalization and per-message degradation behavior. |
| tests/phpunit/Fixtures/DummyAbility.php | Adds a fixture tool that returns an image-typed payload without results for warning/fallback testing. |
| docs/README.md | Adds docs nav link for MCP Apps section. |
| docs/guides/creating-abilities.md | Documents where annotations live per component/content type; adds MCP Apps and resource/image payload guidance. |
| docs/getting-started/basic-examples.md | Updates examples to reflect annotation placement/vocabulary and resource/prompt meta layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+332
to
347
| $is_image_result = isset( $result['type'] ) && 'image' === $result['type']; | ||
|
|
||
| // The image bytes are read from `results`. Without that key there is nothing to | ||
| // encode, so the result falls through to the generic path and reaches the client | ||
| // as a text block; say so rather than letting the `type` marker go unanswered. | ||
| if ( $is_image_result && ! isset( $result['results'] ) ) { | ||
| $this->mcp->get_error_handler()->log( | ||
| 'Tool result marked type "image" has no "results" key, returning it as tool data', | ||
| array( 'tool_name' => $tool_name ), | ||
| 'warning' | ||
| ); | ||
| } | ||
|
|
||
| if ( $is_image_result && isset( $result['results'] ) ) { | ||
| $image_data = base64_encode( $result['results'] ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode | ||
| $mime_type = $result['mimeType'] ?? self::DEFAULT_IMAGE_MIME_TYPE; |
Comment on lines
+73
to
+81
| * Anything a conforming client would reject is dropped here, because annotations are | ||
| * validated as part of the block that carries them, so a bad annotation costs the | ||
| * whole block rather than only itself. Two shapes have to be kept off the wire: a | ||
| * value outside what the schema allows, and an annotations object with nothing left | ||
| * in it, which PHP serializes as a JSON array where MCP declares an object. | ||
| * | ||
| * Dropping is logged rather than raised. Annotations are a rendering hint, and | ||
| * failing the whole response over one would be a worse outcome for a payload that is | ||
| * otherwise valid. |
galatanovidiu
force-pushed
the
split/3-annotations-and-degradation
branch
2 times, most recently
from
July 28, 2026 19:14
51fb5b4 to
1282505
Compare
galatanovidiu
force-pushed
the
split/3-annotations-and-degradation
branch
from
August 5, 2026 12:21
78e6bc5 to
803e6f7
Compare
pull Bot
pushed a commit
to jonathanbossenger/mcp-adapter
that referenced
this pull request
Aug 5, 2026
## What? See WordPress#245. Emit `mimeType` as declared. MCP places no format constraint on `mimeType` on any object that carries one, so presence and type are the only checks that apply. - remove `validate_mime_type()`, `validate_image_mime_type()`, `validate_audio_mime_type()`, `validate_icon_mime_type()` and the icon MIME allow-list, with their call sites - keep a `mimeType` when it is a non-empty string and emit the value as written - `image` and `audio` content blocks still require a `mimeType`, which the schema marks required there First of three stacked PRs splitting WordPress#260, which carries the same change as one branch. Review order is WordPress#262 → WordPress#263 → WordPress#264. To exercise all three together, test WordPress#260. ## Why? The adapter validated `mimeType` against an RFC 2045 pattern that rejects parameters, and icons against a fixed allow-list. Neither constraint comes from MCP, which types `mimeType` as a plain string everywhere it appears. The pattern drops any media type carrying a parameter. `text/html;profile=mcp-app` is the media type an MCP Apps UI template declares, so a UI resource never reached the `resources/list` descriptor with the type that identifies it. ## How? The four validators are removed rather than relaxed: with the format constraint gone, each reduces to `is_string()`, which the call sites already do. Icon `mimeType` keeps its type check and loses the allow-list. The spec states which types a *client* must support; it does not restrict what a server may declare, and an icon a given client cannot render is that client's decision to make. `McpResource` and `RegisterAbilityAsMcpResource` keep the `'' !== $mime_type` guard, so an empty or whitespace-only value is still omitted rather than emitted as an empty string. ### Behaviour changes - a `mimeType` carrying RFC 2045 parameters, such as `text/html;profile=mcp-app`, reaches the `resources/list` descriptor instead of being dropped - any `mimeType` string now survives to the wire as written, on every object that carries one - an icon declaring a MIME type outside the previous allow-list is kept rather than skipped - prompt message `image` and `audio` blocks no longer require the type to start with `image/` or `audio/`; a `mimeType` is still required - four `public static` methods are removed from `McpValidator` ### Use of AI Tools AI assistance: Yes Tool(s): Claude Code, Codex CLI Model(s): Claude Opus 5, Claude Fable 5, GPT-5.6 Sol Used for: Implementation and review. Every change was verified manually by me. ## Testing Instructions 1. Register a resource ability with `meta.mcp.mimeType` set to `text/html;profile=mcp-app` and confirm `resources/list` carries that exact string on the descriptor, unaltered. 2. Register a component with an icon whose `mimeType` is `image/avif` and confirm the icon is kept rather than skipped. 3. Return a prompt message `image` block with `mimeType` set to `application/octet-stream` and confirm the message renders rather than failing validation. Remove `mimeType` entirely and confirm it still fails. 4. Run `composer test`, `composer lint` and `composer phpstan`. ## Changelog Entry > Fixed - Emit `mimeType` as declared, so a media type carrying parameters such as `text/html;profile=mcp-app` reaches the client unaltered.
galatanovidiu
added a commit
that referenced
this pull request
Aug 12, 2026
## What? Closes #245. Carry `_meta` through to the DTOs the adapter emits, and normalize it at every site that builds one, so only a value a client can accept reaches the wire. - copy `_meta` from a handler's content item onto resource contents, embedded resources, content blocks and prompt messages - route every `_meta` through `McpValidator::normalize_meta()`, which yields a value only when it serializes as a JSON object - add a trailing, optional `resource_meta` argument to `ContentBlockHelper::embedded_text_resource()` and `embedded_blob_resource()`, which sets the nested resource contents' `_meta` while the existing argument sets the content block's - recognize a blob-only item as resource contents in `resources/read` - log a `_meta` the handlers drop, naming the object it sat on Second of three stacked PRs splitting #260, which carries the same change as one branch. **Based on #262**, so review that one first; the diff shown here is against it. Followed by #264. To exercise all three together, test #260. ## Why? `_meta` is the spec's channel for metadata that travels with a resource but is not its body. The adapter copied `uri`, `text`/`blob` and `mimeType` out of a handler's content item and left `_meta` behind, so MCP App UI resources (`ui://` with `text/html;profile=mcp-app`) reached the client with their HTML but none of the `_meta.ui` config the server attached. Fixing that alone would still emit metadata a client rejects. PHP represents a JSON object and a JSON array with one type, so an array check admits a list, and a list reaches the wire as a JSON array where MCP declares an object. A client validates `_meta` as part of the response that carries it, so a malformed value costs the whole payload rather than only itself. ## How? **Shape rule.** `McpValidator::normalize_meta()` returns a value only when it serializes as a JSON object; a non-array, an empty array or a list yields null. It returns null rather than raising, because `_meta` travels alongside a payload and a malformed one is not a reason to withhold the payload itself. **Embedded resources.** A tool may write the resource nested, under a `resource` key, or flat. The nested form addresses both `_meta` levels — outer for the block, inner for the contents. Strip `type` from the flat form and what remains is a `ResourceContents` literal, so its `_meta` describes the contents, which is what the same literal already means to `ResourcesHandler::create_content_dto()`. A caller who needs block-level `_meta` writes the nested form, which exists to express that distinction. **Resource contents detection.** `convert_contents_to_dtos()` distinguishes a single payload from a list of content items by looking in the first item for `uri`, `text` or `blob`. Binary contents carry no text and take the resource's own URI when they name none, so `blob` alone identifies one. **Where the generic path stops.** Only the two branches that read a `type` key — `resource` (inside the URI guard) and `image` — treat a sibling `_meta` as the content block's. The generic path returns the result verbatim as `structuredContent` and JSON-encodes it into a text block, so each key is already tool data; reading `_meta` off it would give one key two meanings. **Logging.** The handler sites route `_meta` through `HandlerHelperTrait::normalize_content_meta()`, which logs a warning naming the object the metadata sat on. It takes the raw value, because `normalize_meta()` answers null both for an absent `_meta` and for an unemittable one. A conforming client strips metadata it does not recognize and reports nothing, so this log is the only place the mistake surfaces. `ContentBlockHelper` is static and the domain factories build from config, so neither holds an error handler, and adding one would change signatures public since 0.5.0 — those sites normalize silently. **Documentation.** The user-facing guide for the shapes introduced here — structured resource contents, binary resources, and the MCP Apps route — lands in #264, because the same sections describe annotation and degradation behaviour that only exists after that PR. ### Behaviour changes - a `_meta` that cannot serialize as a JSON object is omitted rather than emitted as a JSON array; this reaches `ContentBlockHelper`, public API as of 0.5.0, though signatures are unchanged and only input that could never have serialized correctly is affected - a `resources/read` handler whose first item carries a `blob` and no `uri` or `text` returns one `BlobResourceContents` per item, rather than a single text block holding the list JSON-encoded - dropped `_meta` is logged with the object it sat on and the tool, prompt or URI it came from; the emitted payload is unchanged by the logging - the new `resource_meta` argument is trailing and optional, so existing calls are unaffected ### Use of AI Tools AI assistance: Yes Tool(s): Claude Code, Codex CLI Model(s): Claude Opus 5, Claude Fable 5, GPT-5.6 Sol Used for: Implementation and review. Every change was verified manually by me. ## Testing Instructions ### Resources 1. Register a resource whose handler returns a content item with `_meta`: ```php 'handler' => fn () => [[ 'uri' => 'ui://example/app', 'mimeType' => 'text/html;profile=mcp-app', 'text' => '<!doctype html>...', '_meta' => [ 'ui' => [ 'prefersBorder' => true ] ], ]], ``` Call `resources/read` and confirm `result.contents[0]._meta` is present. 2. Change that `_meta` to a list (`[ 'a', 'b' ]`) and confirm the key is absent from the response rather than emitted as a JSON array, and that a warning naming the resource is logged. 3. Return a handler payload whose first item carries only `blob` and `mimeType`, with no `uri` or `text`, and confirm `result.contents` holds one `BlobResourceContents` per item rather than a single text block with the list JSON-encoded. ### Tools 4. Register an ability with `meta.mcp._meta` set to an object and confirm it appears on that tool in `tools/list`. Set it to a list and confirm the key is omitted rather than emitted as a JSON array. 5. Return the **nested** embedded-resource shape from a tool and confirm each `_meta` lands on its own level — outer on the content block, inner on the resource contents: ```php [ 'type' => 'resource', 'resource' => [ 'uri' => 'ui://x', 'text' => '...', '_meta' => [ 'contents' => true ] ], '_meta' => [ 'block' => true ], ] ``` 6. Return the **flat** shape (`type`, `uri`, `mimeType`, `text`, `_meta`) and confirm its `_meta` lands on the resource contents, not the block. 7. Return a `type: "image"` result with a sibling `_meta` and confirm it reaches the image content block. The image bytes go in `results` as raw binary, which the handler base64-encodes. ### Prompts 8. Return a prompt message content block carrying a `_meta` object and confirm it survives to `prompts/get`. Change it to a list and confirm the key is omitted and a warning naming the prompt is logged. ### Gate 9. Run `composer test`, `composer lint` and `composer phpstan`. ## Changelog Entry > Fixed - Preserve `_meta` on resource contents, embedded resources, content blocks and prompt messages, and omit a `_meta` that would not serialize as a JSON object. --------- Co-authored-by: Grzegorz Ziolkowski <grzegorz@gziolo.pl>
The schema DTOs assert strict PHP types — `Annotations::priority` a float, `Resource::size` and `ResourceLink::size` an int, each `ToolAnnotations` hint a bool — and WordPress returns stored scalars as strings, so a priority read from post meta arrives as `"0.5"` and a hint as `"1"`. A value the DTO refuses throws, which costs a tool or resource its registration and a prompt its whole response. Route every `annotations` value through `McpAnnotationMapper::map()`, which keeps the fields the target type models and coerces them to the types it declares. Coercion covers unambiguous intent (a number written as a string, a boolean as `"1"`, `"true"`, `"0"`, `"false"`); anything else is dropped, and a field the DTO cannot accept still returns a `WP_Error` naming it. Mapped annotations run through `McpValidator::get_annotation_validation_errors()` and are dropped when it reports errors, since a well-typed but out-of-spec value is rejected by a conforming client along with the object carrying it. Vocabulary the target type does not model is dropped rather than left behind, because an annotations object with nothing in it serializes as `[]` where MCP declares an object. A tool result's `annotations` key carries content annotations — `audience`, `priority`, `lastModified` — not the `ToolAnnotations` vocabulary that describes a tool on its descriptor. `annotations` on a `type: "resource"` tool result now reaches the content block, and an `image` result carries the `annotations` written beside it. Only the two branches that read a `type` key treat sibling `annotations` as the content block's; the generic path returns the result verbatim as `structuredContent`, so each key there is already tool data. `PromptsHandler::normalize_content_block()` applies the same rules to each block, casts a `resource_link` `size` to int, and checks embedded contents through `is_valid_resource_contents()`. A block the DTOs refuse becomes a text block carrying that block's JSON with a logged warning, leaving the other messages untouched — the substitution the handler already applies to an unrecognized content type and an invalid role. Log a tool result marked `type: "image"` that carries no `results`, which reaches the client as tool data. Document where annotations go per component type, the two embedded-resource shapes, the MCP Apps route, and the prompt message block types.
`ToolsHandler` reads image bytes from a tool result's `results` key and base64-encodes them, and `base64_encode()` accepts only a string. Guard the image branch on `is_string()` as well as presence, so a missing key, a null, or any other type leaves nothing to encode and falls through to the generic path, reaching the client as tool data. The warning names the type found under `results`. `HandlerHelperTrait::build_content_annotations()` drops annotations on two paths: a mapped value the schema rejects, and an object whose every field belongs to a vocabulary the content block does not model. Log the second as well as the first, guarded on a non-empty input, so annotations written in the tool-hint vocabulary are accounted for while an already-empty object stays quiet.
galatanovidiu
force-pushed
the
split/3-annotations-and-degradation
branch
from
August 12, 2026 07:24
803e6f7 to
1f0a6b8
Compare
Validate optional annotations and size hints before DTO construction, and degrade malformed prompt or resource content locally. Normalize ability-backed resource permission input through core so schema defaults are applied consistently.
Contributor
Author
|
I’m closing this PR. After reviewing the problem again, most of the changes handle malformed annotations, sizes, or content returned by ability providers. These values are outside the documented contracts, and I don’t think the Adapter should coerce or recover from them. Providers should return valid data. The one reported Adapter bug included here is #261, the no-argument input used by ability-backed resources. That is a separate and much smaller fix. If we continue with it, it should have its own focused PR. #245 is already fixed by #263. This PR adds too much policy and complexity for problems we have not seen in real use, so I don’t think it should merge. |
Member
This PR was complex, so I agree it's a fair choice to focus on the targeted fix 👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
See #245. Closes #261.
Keep malformed optional annotations, size hints, and individual content blocks from costing the payload that carries them. This PR also normalizes the no-argument input used by ability-backed resources and documents where each annotation vocabulary belongs.
type: "resource"andtype: "image"tool results to their content blocksResourceandresource_linksizes only when they represent an exact, non-negative integerThis is the third of three PRs split from #260.
Why?
The schema DTOs enforce strict PHP types, while ability metadata commonly supplies stored scalars as strings. An optional rendering hint such as
priority: "0.5"should not prevent a tool or resource from registering, and one malformed prompt block should not fail the complete prompt response.Annotation vocabularies also differ by location. Tool descriptors use tool hints, while resources and content blocks use
audience,priority, and, from MCP 2025-06-18,lastModified. Passing fields to the wrong DTO can otherwise produce an empty annotations value or an exception.Ability-backed resources have a separate input boundary:
resources/readsupplies a URI, not ability arguments. An ability that also declares an object input schema still needs a schema-compatible no-argument value for execution and permission checks.How?
McpAnnotationMapperperforms unambiguous scalar coercion and vocabulary mapping. The mapped annotations then pass throughMcpValidator::get_annotation_validation_errors(); unusable annotations are logged and omitted without dropping their parent payload. PrebuiltAnnotationsDTOs follow the same validation path. Untyped tool results continue to treatannotationsand_metaas ordinary tool data.McpValidator::normalize_size()centralizes size handling. It accepts non-negative integers, finite integral floats, and digit-only decimal strings withinPHP_INT_MAX. Fractions, exponent notation, negative values, overflow, non-finite floats, and unrelated types are omitted rather than truncated or allowed to raise warnings. Zero remains valid.Prompt normalization reuses
McpResourceValidatorfor embedded resource contents. Invalid embedded resources and blocks rejected by the schema become text blocks containing their JSON representation, so other prompt messages remain available. Non-string roles and non-scalar shorthand content also reach this per-message fallback instead of failing the response early.For ability-backed resources,
AbilityArgumentNormalizerstill chooses the pre-Core no-argument value. The direct permission path additionally callsWP_Ability::normalize_input()beforecheck_permissions(), allowing Core to apply top-level schema defaults without changing the normalizer's shared contract or double-normalizing execution.The guide now keeps one annotation location table and focused examples, and identifies the minimum protocol versions for newer annotation fields and content types.
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code, Codex CLI
Model(s): Claude Opus 5, Claude Fable 5, GPT-5.6 Sol
Used for: Implementation, test design, documentation, and code review. Final verification is listed below.
Testing Instructions
npm run test:php— 1,087 tests, 4,079 assertions, 1 skippednpm run lint:php— passednpm run lint:php:stan— passedgit diff --check— passedRegression coverage includes annotation vocabulary and DTO validation, exact size normalization, invalid embedded-resource URIs, malformed prompt roles and content, per-message degradation, and ability schema defaults during typed permission callbacks.
Changelog Entry