Repository navigation
fix: drop non-object structuredContent after a jmespath filter - #873
Conversation
MCP structuredContent must be a JSON object, and the SDK server checks every tools/call result against CallToolResultSchema. A filter such as items[].title, a missing path, or length(items) turned it into an array, null, or a number, which would fail the whole call with "expected record, received array". Keep structuredContent only when the filtered value is an object; the filtered JSON is still in the text content. No handler sets structuredContent today, so this guards a latent path. Refs #675
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description clearly explains the JMESPath structuredContent issue, the implemented fix, tests, and verification results. Title check The title clearly and concisely describes the main change: dropping non-object structuredContent after a JMESPath filter.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
zereight
left a comment
zereight
left a comment
There was a problem hiding this comment.
Reviewed at 4219d07. No P0/P1 findings. The guard only drops structuredContent when the filtered value is not a JSON object, the text block still carries the filtered JSON, and masking and compaction already handle a missing key. No current tool sets structuredContent, so there is no visible change today.
1 participant
Why
applyJmespathToToolResultfiltersstructuredContentwith the same expression as the text content and returns whatever JMESPath produces. A filter such asitems[].title,keys(@),length(items), or a missing path turns it into an array, a number, ornull. The MCP spec saysstructuredContentis a JSON object, and the SDK's server checks everytools/callresult againstCallToolResultSchema(@modelcontextprotocol/sdk1.31,Server.setRequestHandler). That check runs whether or not the tool has anoutputSchema. A non-object value therefore fails the whole call withInvalid tools/call result, the sameexpected record, received arrayerror that #675 reports, and the model gets no result to work with.No handler sets
structuredContenttoday, so this path is not reachable onmain. This is a guard for the first tool that does, not a fix for the report in #675.What changed
structuredContentis kept only when the result is a JSON object.nulldrops thestructuredContentkey. The filtered JSON is still in the text content block.structuredContentthroughitems[].title,missing, andlength(items), and checks each result with the SDK'sCallToolResultSchema. It fails onmainand passes here.Tradeoffs
Dropping keeps the text block and
structuredContentsaying the same thing whenever both are present. Wrapping the value as{ "result": [...] }would make up a shape that nooutputSchemadescribes. Keeping the unfiltered object would bring back fields the caller filtered out.structuredContentis optional when a tool has nooutputSchema, and no tool here declares one.Blast Radius
None for current tools, because none of them return
structuredContent. Results withoutstructuredContent, error results, and filters that return an object are unchanged.Verification
npm run build,npx tsc --noEmit,npm run lint(0 errors), andprettier --checkon the changed files passed.node --import tsx/esm --test test/utils/jmespath-tool-result.test.tspassed. Without the fix, the new test fails becauseitems[].titleleavesstructuredContentas["a","b"].npm run test:mockpassed locally (1780 pass, 0 fail) and in CI.Refs #675