[nick-nlb]

Overview

In mcp/client.py, async_call_tool previously reported failures in two different ways: transport, HTTP, protocol, and handshake errors raised McpTransportError (aborting the turn), while tool-layer errors (isError: true or JSON-RPC INVALID_PARAMS, including unknown tool names) returned {"error": <message>}. Returning an error dictionary with the same type as a successful result required callers to check "error" in result on an untyped dictionary, which goes against CODING_GUIDELINES.md §3.2 and §8.

With this change, async_call_tool raises McpToolError on tool-layer failures instead of returning an error dictionary, and all MCP client exceptions inherit from a common McpClientError base class. workflows/mcp_loop._run_tool catches McpToolError and constructs the same tool-call record as before, so agent and UI behavior are unchanged. Defining a typed exception now allows upcoming callers in the agent loop rewrite to catch McpToolError directly rather than checking for an "error" key.

Related Issues

• Follows #529, #530, #535, and #541.

Changes Made

Changes:

• Typed MCP client exceptions (mcp/client.py): Recoverable tool-layer failures now raise McpToolError(tool, message) instead of returning {"error": <message>}. McpToolError and McpTransportError both inherit from McpClientError (named to avoid colliding with mcp.MCPError from the SDK).
• Tool-call record compatibility (workflows/mcp_loop.py): _run_tool catches McpToolError and produces the same error record (status: "error" and result: json.dumps({"error": message})) instead of checking "error" in result.
• JSON type aliases (json_value.py, narratives/AGENTS.md): Adds JsonValue and JsonObject recursive type aliases for unstructured JSON payloads, and updates async_call_tool to use JsonObject.
• Unit tests (mcp/client_test.py, workflows/mcp_loop_test.py): Updates the MCP client and tool-loop tests to assert McpToolError and McpClientError while verifying that the serialized tool-call error record and warning logs are unchanged.

Testing Done

• pnpm test:agent (uv run pytest): 865 passed.
• pnpm test:ui (vitest run): 26 files, 286 passed.
• pnpm lint:ui (tsc --noEmit and biome check): passed.
• pnpm lint:agent (ruff format --check, ruff check, and mypy --strict): passed across all 65 source files.
• pnpm lint:deploy: terraform fmt -check, pnpm run lint:shell, and pnpm run lint:docker passed (terraform validate fails with a pre-existing GCS backend configuration error on main; no deploy/ files were modified).

Checklist:

• Unit tests passed
• Integration tests passed
• Manual verification
• Any updated goldens or fixtures were reviewed and are intentional

Manual

Verified against public Data Commons (MCP_SERVER_URL=https://api.datacommons.org/mcp) with pnpm dev:agent and pnpm dev:ui:

• Successful turn: Queried "What is the population of France?" through POST /agent/chat/stream. Verified that tool_call, data_status, sources, chart_config, streamed answer text, follow_ups, and the signed complete terminal frame were emitted as expected.
• Tool-error path: Called _run_tool against the live MCP server with an unknown tool name (no_such_tool, triggering INVALID_PARAMS) and with an invalid date argument on get_observations (date: "not-a-date", triggering isError: true). Verified that both calls logged a warning and returned the exact same status: "error" record and {"error": ...} JSON result string as main.

Risk & Rollback

Agent and UI behavior were not changed: failed tool calls still produce a record with status: "error" and result: json.dumps({"error": message}). Byte-for-byte compatibility of the result string is covered by test_tool_status_follows_the_call_outcome in workflows/mcp_loop_test.py. Rollback is reverting the PR.

Follow-ups

• Replace the remaining dict[str, Any] signatures in mcp/client.py (async_get_tools, cached_tools, _tool_dict, _send_tool_call, _ToolCache.tools, and _REFRESH_TASKS) and in mcp/schema.py.
• Move tool-result payload decoding from data_utils.parse_tool_result into mcp/client.py and replace the dict[str, Any] tool-call record type as part of the agent loop rewrite.

Checklist

• I have read AGENTS.md and followed CODING_GUIDELINES.md, plus FRONTEND.md for UI changes.
• I have run the app's lint, test, and build commands, as documented in that application's guide.
• I have commented my code, particularly in hard-to-understand areas.
• My changes generate no new warnings.

---

Note: Only Maintainers can approve and merge PRs. Expected initial review time: 3 business days.

[gemini-code-assist]

Code Review

This pull request introduces a structured exception hierarchy for MCP client failures by adding a base McpClientError and a specific McpToolError for tool-layer failures. It refactors async_call_tool to raise McpToolError instead of returning an error dictionary, and updates the workflow loop and tests to handle this new exception-based error reporting. Additionally, it introduces JsonValue and JsonObject type aliases in a new json_value.py module to replace untyped dictionaries for JSON-like payloads. I have no feedback to provide as the changes are clean, well-tested, and align with the repository's style guidelines.

[juliawu]

Thanks for this! Overall, LGTM!

I think it's fine as is for now, but it looks like the types in json_value would only be used by the MCP client for now. Are you imaginging those types will eventually get shared across the stack? Otherwise, a two-line json_value.py top-level module feels a little premature. We should move it into mcp/ following the "narrowest scope that owns it" coding guideline if MCP will be the only consumer.

[nick-nlb]

Thanks for this! Overall, LGTM!

I think it's fine as is for now, but it looks like the types in json_value would only be used by the MCP client for now. Are you imaginging those types will eventually get shared across the stack? Otherwise, a two-line json_value.py top-level module feels a little premature. We should move it into mcp/ following the "narrowest scope that owns it" coding guideline if MCP will be the only consumer.

They are only used by the MCP server for now, but that will change in upcoming PRs (including the next one coming shortly to type the SSE frames).