Skip to content

fix(mcp): surface tool execution errors from MCP results - #295

Open
pei711 wants to merge 3 commits into
Eigenwise:mainfrom
pei711:fix/mcp-tool-error-results
Open

pei711 wants to merge 3 commits into
Eigenwise:mainfrom
pei711:fix/mcp-tool-error-results

Conversation

@pei711

@pei711 pei711 commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

MCP tool failures marked with isError: true must not be returned as successful typed or generic outputs. At the same time, raw application dictionaries may legitimately contain an isError key and must remain direct output data.

User Impact

Generated tools now raise through the existing execution-error path for MCP error results, preserve text diagnostics from text blocks, and safely ignore non-text blocks even when they contain a text: null field.

Why This Change Was Made

The result parser now distinguishes MCP result envelopes from raw dictionaries and extracts messages only from type="text" blocks whose text is a string. The change remains confined to tool-result handling and its regression tests.

Evidence

  • uv run pytest --cov=atomic_agents atomic-agents: 375 passed, 3 skipped (live MiniMax integration); package coverage 93%, mcp_factory.py 90%.
  • Regression coverage includes typed and generic raw dictionaries with {"isError": true, "name": "test_value"}, SDK and dictionary error envelopes, empty error content, and mixed text/image content where image text is null.
  • uv run black --check on both touched files passed. Flake8 with the repository's default 150-character limit passed; with the pre-commit 127-character override, seven existing long lines in mcp_factory.py are also reported on origin/main. No such lines are added to this PR's net diff, and Flake8 reports no other issue in the touched files.
  • Radon cyclomatic complexity: _mcp_tool_error_message 8 (B); _mcp_text_block_text 5 (A); new regression tests 6 (B) and 3 (A).
  • git diff --check passed.

Protocol reference: https://modelcontextprotocol.io/specification/2025-06-18/server/tools#error-handling

@Eigenwise Eigenwise left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two regressions need fixing before merge:

  • mcp_factory.py:243-249 treats every dictionary's isError field as an MCP envelope flag. A supported raw output such as {"isError": true, "name": "test_value"} succeeds on main but now raises, for both typed and generic outputs. Preserve the direct-dictionary output path and add that regression.
  • mcp_factory.py:250-254 reads text from every content kind. Valid mixed content with a text error and an image carrying text=None raises a join TypeError and loses the server diagnostic. Extract actual text blocks and keep the no-text fallback.

Both were reproduced offline against this exact head and compared with main. Please also remove unrelated string-wrapping changes from the MCP resource/prompt code and keep the fix surgical. The new tests should cover these failure inputs.

@pei711

pei711 commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both requested regressions in e63563d: raw dictionaries with an isError application field are preserved for typed and generic outputs, and error text is extracted only from text blocks so a mixed image block with text: null cannot mask the server diagnostic. I also removed the unrelated string-wrapping changes from the PR's net diff. Updated the Evidence section with full-suite results, coverage, and per-function complexity. The pre-commit 127-character Flake8 override still reports seven unchanged lines already present on main; there are no other Flake8 findings in the touched files.

@Eigenwise Eigenwise left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historical review of e63563de.

Rechecked e63563de. Both original cases pass now: raw {isError: true, name: 'test_value'} and real SDK text mixed with ImageContent(text=None). The unrelated formatting churn is gone too.

One raw-data collision remains at mcp_factory.py:46-47: {isError: true, name: 'test_value', content: 'article'} gets treated as an error envelope and raises the fallback RuntimeError. On d2b61b90, the matching typed schema gets the whole dictionary and generic output gets article. A string content field doesn't make this an MCP result envelope. Eight mocked typed/generic, sync/async and owned/borrowed-session comparisons reproduce it.

On this historical head, the production error helper had native Radon CC8 and the modified async tool method CC30. The earlier score-only refactor request is withdrawn. Keep the meaningful assertions.

The exact module has 129 passing tests. Text-only diagnostics, fallback, private structured/meta/binary/resource exclusion and session ownership passed the additional probes. The SDK structuredContent=None JSON-text failure also exists on the base, so that's a separate fix rather than a regression introduced here.

Superseded by the source-only review of cc68db60: the raw content: article collision is repaired, and the 13 changed production functions measure complexity 2 through 5. That later review identifies a different typed raw-string exception-cause regression. The 129 passing tests above belong only to e63563de.

@pei711

pei711 commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the remaining raw-dictionary collision and split result decoding/error handling into focused helpers. A dictionary containing isError: true and a string content is now preserved as application data for typed output, while generic output retains the existing content extraction behavior. Added typed/generic regressions, including mixed text and non-text error blocks.

Validation: uv run pytest --cov=atomic_agents atomic-agents (381 passed, 3 skipped), Black passed. The pre-commit Flake8 hook reports seven unchanged overlong lines already present on origin/main. Updated commit: cc68db6. Please re-review.

@Eigenwise Eigenwise left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source-only recheck of cc68db60: the raw {isError: true, name: 'test_value', content: 'article'} collision is repaired, and the valid mixed text/image diagnostic path still looks correct. Native Radon 6.0.1 measures the 13 changed production bodies at complexity 2 through 5, compared with 8/30 on the earlier head. I haven't executed this head's tests or measured its coverage.

One introduced compatibility regression remains at mcp_factory.py:136-152: a typed raw string such as just a string, not structured reaches dictionary .get, so the wrapped failure's cause becomes AttributeError. The existing contract deliberately raises ValueError with the unparseable-result message. Preserve that non-mapping failure path and add an assertion for the exception cause, rather than only the outer RuntimeError. This finding is traced from source, not runtime-reproduced on this head.

Please also test malformed result-envelope handling: {isError: true, content: [{type: 'text', text: 'Search unavailable'}], structuredContent: 'PRIVATE'} fails whole-envelope validation at :35-43 and falls back to application-data handling. The source then permits generic success or logs the private structured scalar during typed validation. This is an unexecuted concern; the regression should preserve legitimate raw dictionaries while rejecting a recognizable malformed error envelope without exposing its private fields.

Current-head coverage cannot be accepted from test_force_mark_unreachable_lines_for_coverage at test_mcp_factory.py:1610-1624, which still compiles pass under the production filename. Replace that artificial line-touching with actual mocked calls to the changed code. We've already removed it on the maintenance branch; preserve that removal when resolving integration overlap. No numerical inflation amount is claimed.

Keep the useful assertions. This refactor also changes decoding behavior and includes the separate SDK structuredContent=None fallback, so integration needs those behavior changes kept explicit.

This branch has not been deployed

No deployments
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.

2 participants