BAI-344 propagate MCP tool isError as ToolException in call_tool - #63
Conversation
CallToolTool converted a failed inner tool call (result.isError=True, missing/invalid arguments, tool-not-found, transport errors) into ordinary text content instead of an error-flagged ToolMessage. FastMCP already reports these as isError=True per SEP-1303, but this wrapper discarded that signal, so a model reading the response saw what looked like a normal success and kept retrying blindly instead of adjusting its arguments (observed: propose_skill called 36 times in a row with no arguments). Raise ToolException on all three failure paths and set handle_tool_error=True so LangChain marks the resulting ToolMessage status="error", which is the signal calling models/harnesses actually react to.
![gecko-security[bot]](https://avatars.githubusercontent.com/in/1047747?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gecko PR Review
No vulnerabilities found, LGTM.
Summary
This PR refactors languagemodelcommon/tools/mcp/call_tool_tool.py to use ToolException for error signaling instead of returning error tuples, enabling LangChain's BaseTool to properly mark failed tool calls with status="error" in resulting ToolMessage objects. The change sets handle_tool_error: bool = True and raises ToolException on three failure paths: missing tools, MCP server errors (result.isError), and unexpected exceptions. This ensures calling models can distinguish between successful and failed MCP tool invocations.
Important Files Changed
2 important files
| File Path | Why it matters |
|---|---|
languagemodelcommon/tools/mcp/call_tool_tool.py |
Core change: replaces error-tuple return pattern with ToolException raises on missing tools, MCP errors, and unexpected exceptions, coupled with handle_tool_error: bool = True to enable proper error status in LangChain message contracts. |
tests/mcp/test_call_tool_tool.py |
Test file must be updated to verify that ToolException is now raised instead of tuples returned, and that error paths propagate correctly to callers. |
Sequence Diagram
sequenceDiagram
participant Model
participant CallToolTool
participant ToolCatalog
participant MCPServer
Model->>CallToolTool: invoke(name, args)
CallToolTool->>ToolCatalog: get_tool(name)
alt Tool not found
ToolCatalog-->>CallToolTool: None
CallToolTool->>CallToolTool: raise ToolException
CallToolTool-->>Model: ToolMessage(status="error")
else Tool found
ToolCatalog-->>CallToolTool: entry
CallToolTool->>MCPServer: call_tool()
alt MCP call succeeds but result.isError=true
MCPServer-->>CallToolTool: CallToolResult(isError=true)
CallToolTool->>CallToolTool: raise ToolException
CallToolTool-->>Model: ToolMessage(status="error")
else MCP call succeeds normally
MCPServer-->>CallToolTool: CallToolResult(isError=false)
CallToolTool-->>Model: ToolMessage(status="success")
end
end
Edit Gecko PR Settings | Reviewed commit: 8c2c15b
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the ToolException + handle_tool_error mechanism is correct and verified against the LangChain source (BaseTool.arun catches ToolException, sets status="error" via _handle_tool_error when handle_tool_error=True), and the AuthorizationNeededException re-raise path is preserved. Only issue found is a stale comment (flagged inline), which doesn't affect behavior.
Extended reasoning...
Overview
This PR changes error signaling in CallToolTool._arun (languagemodelcommon/tools/mcp/call_tool_tool.py) from returning (text, {"is_error": True}) tuples to raising ToolException on all three failure paths (tool not found, inner result.isError, transport exception), paired with handle_tool_error: bool = True. Tests in tests/mcp/test_call_tool_tool.py were updated accordingly, including a new end-to-end test that exercises the real BaseTool.arun to confirm ToolMessage.status == "error".
Security risks
None. This is purely an internal error-signaling mechanism between a wrapper tool and the calling LLM/agent harness — no new external inputs, no auth/crypto/tenant-boundary code touched. The existing AuthorizationNeededException re-raise (used for login-link flows) is preserved unchanged and still takes precedence over the new ToolException handling.
Level of scrutiny
I verified the core claim against the actual langchain_core source (found a local extracted copy): BaseTool.arun catches ToolException, and when handle_tool_error=True it calls _handle_tool_error and sets status = "error" on the resulting ToolMessage — exactly as the PR description claims. This is a small, well-scoped bug fix (root-causing a real production retry-loop incident per the PR description) with direct test coverage of both the unit-level (_arun raises) and integration-level (arun produces the right ToolMessage status) behavior. Low risk given the scope and quality of tests.
Other factors
The one substantive issue found — a stale comment in tool_event_handlers.py (and a matching test docstring) that still describes the old artifact-based signaling this PR removes — is a documentation-only nit with no runtime impact (the consuming is_error check is an OR of both signals, so it still evaluates correctly either way). It's already captured as an inline comment. No outstanding unaddressed reviewer comments were found in the timeline.
| ) | ||
| text = _call_tool_result_to_text(result) | ||
|
|
||
| # Surface the *inner* tool's own error status (result.isError), not just | ||
| # transport-level failures — MCP servers report rejected/invalid calls | ||
| # this way rather than raising, so this is the only place that signal | ||
| # exists. Raising here (with handle_tool_error=True) is what makes the | ||
| # resulting ToolMessage carry status="error" instead of looking like an | ||
| # ordinary successful call to the model. | ||
| if result.isError: | ||
| raise ToolException(text) | ||
|
|
||
| app_embed = await self.mcp_tool_provider.fetch_mcp_app_embed( | ||
| tool=entry.tool, | ||
| tool_name=name, |
There was a problem hiding this comment.
🟡 This PR changes CallToolTool's error signaling from artifact={"is_error": True} to raising ToolException, but languagemodelcommon/converters/tool_event_handlers.py:184-189 still has a comment (and tests/converters/test_tool_event_handlers.py:161-163 a matching docstring) claiming CallToolTool reports failures via the artifact — both now describe a contract this PR removed. Runtime behavior is unaffected since the dual-signal check (status=='error' OR artifact.get('is_error')) still passes via the status branch, but the comment/docstring should be updated since it explicitly cites CallToolTool as the rationale.
Extended reasoning...
What's stale
languagemodelcommon/converters/tool_event_handlers.py:184-186 contains a comment explaining the dual-signal is_error check:
tool_message.statusonly flips to"error"for an uncaught exception; meta-tools likecall_toolcatch their own failures and report them via the artifact instead (seeCallToolTool).
tests/converters/test_tool_event_handlers.py:161-163 has a matching docstring making the same claim, and constructs a ToolMessage with artifact={"is_error": True} to simulate what it says is CallToolTool's output.
Why it's now wrong
This PR (languagemodelcommon/tools/mcp/call_tool_tool.py:114-128) inverts exactly the contract these two comments describe. CallToolTool._arun no longer returns artifact={"is_error": True} on failure — that code path was deleted. Instead, on result.isError, tool-not-found, or a transport exception, it now raises ToolException, and handle_tool_error: bool = True makes BaseTool.arun catch that exception and set ToolMessage.status = "error". So both clauses of the comment are now false: call_tool's status does flip to "error" (via a handled, not uncaught, exception), and it no longer reports through the artifact at all.
Why nothing breaks today
tool_event_handlers.py implements the is_error check as status == "error" OR artifact.get("is_error") — an OR of two signals. Before this PR, call_tool failures satisfied it via the artifact branch; after this PR, they satisfy it via the status branch instead. The check still evaluates to True for a failed call_tool invocation either way, so the runtime logic that consumes this signal is unaffected. Neither tool_event_handlers.py nor its test file is touched by this PR — it only modified call_tool_tool.py and tests/mcp/test_call_tool_tool.py.
Concrete walkthrough
- Before this PR:
call_toolcalls a rejecting MCP tool →_arunreturns("Tool call failed: ...", {"is_error": True})→BaseToolbuilds aToolMessagewithstatus="success"(no exception raised) andartifact={"is_error": True}→tool_event_handlers.py's checkstatus=="error" or artifact.get("is_error")evaluatesFalse or True = True. Matches the comment's description exactly. - After this PR: same call →
_arunraisesToolException("Tool call failed: ...")→ becausehandle_tool_error=True,BaseTool.aruncatches it and builds aToolMessagewithstatus="error"and nois_errorartifact key → the same check evaluatesTrue or False = True. Same net result, but now reached via the other branch — the one the comment says never happens forcall_tool.
Fix
Update the comment in tool_event_handlers.py:184-186 to drop the now-incorrect claim that call_tool reports via the artifact (or simply state that both signals are checked because different tools/versions may use either), and update the docstring/fixture in test_tool_event_handlers.py:161-163 to stop citing CallToolTool as the source of the artifact={"is_error": True} pattern — the artifact branch may still be exercised by other meta-tools, but not by CallToolTool anymore.
This is a documentation-only staleness issue with no behavioral impact, so it should not block merge, but it's worth a quick follow-up since this very PR is what changed the contract being described.
2 participants
Summary
CallToolTool._arunconverted a failed inner MCP tool call (result.isError=True, missing/invalid arguments, tool-not-found, or a transport exception) into ordinary text content instead of an error-flaggedToolMessage. FastMCP already reports validation failures asisError=True(confirmed by tracingmcp/server/lowlevel/server.py's_make_error_result), consistent with SEP-1303 ("Input Validation Errors as Tool Execution Errors") — but this wrapper discarded that signal before it reached the calling model.propose_skill(baileyai-skills-service) 36 times in a row viacall_toolwith theargumentspayload omitted entirely. The model never received a real failure signal, so it kept retrying the same malformed call instead of adjusting its arguments.ToolExceptionon all three failure paths in_arun(tool not found, innerresult.isError, transport exception) and sethandle_tool_error: bool = TrueonCallToolTool. LangChain'sBaseTool.handle_tool_errordefaults toFalse(a raisedToolExceptionjust re-raises); setting itTrueis what makes the caught exception become aToolMessagewithstatus="error"instead of looking like an ordinary successful call.call_tool, not just one server's tools.Related
propose_skill/propose_resource/propose_script) by giving required fields empty-string defaults and validating manually, so FastMCP's own schema-validation error is bypassed in favor of the tool's own actionable rejection message. That fix stays — it improves message wording — but this PR is the root-cause fix for the actual retry-loop incident, since it fixes error visibility rather than error wording.Test plan
tests/mcp/test_call_tool_tool.py: existing tests now assertToolExceptionis raised (not a(text, artifact)tuple) for the not-found/transport-error/inner-error-result paths. Addedtest_inner_tool_error_result_sets_tool_message_status_error, an end-to-end test going through the realBaseTool.arunto confirm the resultingToolMessage.status == "error".uv run pytest tests/mcp tests/converters— 305 passeduv run ruff check/ruff format --check/mypy— clean🤖 Generated with Claude Code