fix(mcp): remove redundant structuredContent from error response#834
fix(mcp): remove redundant structuredContent from error response#834yamadashy merged 3 commits intoyamadashy:mainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoved structuredContent from error responses produced by buildMcpToolErrorResponse in src/mcp/tools/mcpToolRuntime.ts. Updated related tests to stop asserting structuredContent in error cases for MCP runtime and file system tools. No function signatures changed; error responses still return isError and text content. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Tool as Tool
participant Runtime as buildMcpToolErrorResponse
participant Client as MCP Client (SDK)
Tool->>Runtime: Error occurs
activate Runtime
Runtime-->>Tool: CallToolResult { isError: true, content: [text], (no structuredContent) }
deactivate Runtime
Tool->>Client: Return error CallToolResult
Client->>Client: Validate against output schema
Note right of Client: No structuredContent present, avoids additional properties error
Client-->>Tool: Accepts error result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR removes the redundant structuredContent property from the error response structure in the MCP tool runtime. The change addresses issue #833 by eliminating duplicate data that was being included in both the text content and structured content fields.
- Removes the
structuredContentproperty frombuildMcpToolErrorResponsefunction - Updates corresponding test cases to reflect the removal
- Maintains error information through the text content field while eliminating redundancy
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/mcp/tools/mcpToolRuntime.ts | Removes redundant structuredContent property from error response builder |
| tests/mcp/tools/mcpToolRuntime.test.ts | Updates test expectations to match removed property |
| tests/mcp/tools/fileSystemReadFileTool.test.ts | Updates test expectations for file system read operations |
| tests/mcp/tools/fileSystemReadDirectoryTool.test.ts | Updates test expectations for directory read operations |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @huy-trn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request streamlines error responses by removing a redundant field, structuredContent, from the buildMcpToolErrorResponse function. This change simplifies the error object structure and ensures that only necessary information is returned, improving clarity and potentially reducing payload size.
Highlights
- Error Response Structure: The buildMcpToolErrorResponse function no longer includes the structuredContent property in its returned object, as it was deemed redundant.
- Test Updates: Corresponding test files (fileSystemReadDirectoryTool.test.ts, fileSystemReadFileTool.test.ts, mcpToolRuntime.test.ts) have been updated to reflect this change by commenting out the structuredContent assertions in expected error responses.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request correctly removes the redundant structuredContent property from the error response object created by buildMcpToolErrorResponse. The change is simple, effective, and improves the API response consistency. The associated tests have been updated to reflect this change. My feedback focuses on improving the test code by removing commented-out code blocks, which will enhance readability and maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/mcp/tools/mcpToolRuntime.test.ts (1)
327-336: Inconsistent with PR objective: this error test still expects structuredContent
This contradicts the new contract (no structuredContent on errors) and will fail if the runtime omits it. Update the expectation.expect(response).toEqual({ isError: true, content: [ { type: 'text', text: 'null', }, ], - structuredContent: errorContent, }); + expect(response).not.toHaveProperty('structuredContent');
🧹 Nitpick comments (6)
tests/mcp/tools/mcpToolRuntime.test.ts (2)
291-301: Good: error response no longer includes structuredContent; add an explicit negative assertionThis aligns tests with the new error shape. For clarity and to guard regressions, assert the absence explicitly.
expect(response).toEqual({ isError: true, content: [ { type: 'text', text: JSON.stringify(errorContent, null, 2), }, ], - // structuredContent: errorContent, }); + expect(response).not.toHaveProperty('structuredContent');
311-321: Same here: add explicit assertion that structuredContent is absent on errorsPrevents accidental reintroduction.
expect(response).toEqual({ isError: true, content: [ { type: 'text', text: JSON.stringify(errorContent, null, 2), }, ], - // structuredContent: errorContent, }); + expect(response).not.toHaveProperty('structuredContent');tests/mcp/tools/fileSystemReadDirectoryTool.test.ts (2)
49-52: Drop commented code; assert absence of structuredContent explicitlyThe commented block is noise. Replace with a clear negative assertion.
], - // structuredContent: { - // errorMessage: `Error: Path must be absolute. Received: ${testPath}`, - // }, }); + expect(result).not.toHaveProperty('structuredContent');
70-73: Same cleanup and assertion for non-existent directory case], - // structuredContent: { - // errorMessage: `Error: Directory not found at path: ${testPath}`, - // }, }); + expect(result).not.toHaveProperty('structuredContent');tests/mcp/tools/fileSystemReadFileTool.test.ts (2)
49-52: Remove commented structuredContent and assert it’s absent], - // structuredContent: { - // errorMessage: `Error: Path must be absolute. Received: ${testPath}`, - // }, }); + expect(result).not.toHaveProperty('structuredContent');
70-73: Same for non-existent file case], - // structuredContent: { - // errorMessage: `Error: File not found at path: ${testPath}`, - // }, }); + expect(result).not.toHaveProperty('structuredContent');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mcp/tools/mcpToolRuntime.ts(0 hunks)tests/mcp/tools/fileSystemReadDirectoryTool.test.ts(2 hunks)tests/mcp/tools/fileSystemReadFileTool.test.ts(2 hunks)tests/mcp/tools/mcpToolRuntime.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/mcp/tools/mcpToolRuntime.ts
yamadashy
force-pushed
the
fix/mcp_error_output_schema
branch
from
6c596b4 to
b288701
Compare
Add comment explaining why structuredContent is omitted from error responses to prevent schema validation issues. Remove commented-out code from tests for better maintainability as suggested in PR review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #834 +/- ##
==========================================
- Coverage 87.29% 87.29% -0.01%
==========================================
Files 113 113
Lines 6661 6660 -1
Branches 1372 1372
==========================================
- Hits 5815 5814 -1
Misses 846 846 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@huy-trn I've added a code comment to make it clear why the parameter is omitted. Looking forward to your future contributions! |
This pull request makes a small change to the
buildMcpToolErrorResponsefunction inmcpToolRuntime.ts, removing the unnecessary inclusion of thestructuredContentproperty from the returned object.Fix #833
Checklist
npm run testnpm run lint