From c657fe479c453fe0a2012d2bcf37ccdb7f2c0d96 Mon Sep 17 00:00:00 2001 From: sanjibani <18418553+sanjibani@users.noreply.github.com> Date: Fri, 26 Jun 2026 19:13:21 +0530 Subject: [PATCH] fix(tasks): raise on failure so isError=true on the MCP wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 9 `except APIError as e: return _format_error(e)` and 9 `except Exception as e: return json.dumps({"error": True, ...})` sites in apps/mcp-server/src/taskflow_mcp/tools/tasks.py all returned JSON-encoded error strings. FastMCP wraps those as success content with isError=false, so MCP clients treat the failure as data and the LLM often proceeds as if the call had succeeded. This is the same isError-compliance gap flagged in the recent MCP security audit (Dayna Blackwell, 'Tool Poisoning, Rug Pulls, and Prompt Injections — oh my!'). Each return is replaced with bare `raise` so the original exception propagates and FastMCP sets isError=true on the wire while preserving the formatted message in content for the LLM. Regression test: apps/mcp-server/tests/test_iserror_compliance.py asserts that a failing create_task call surfaces as ToolError (isError=true on the wire). --- .../src/taskflow_mcp/tools/tasks.py | 54 +++++++++--------- .../tests/test_iserror_compliance.py | 55 +++++++++++++++++++ 2 files changed, 82 insertions(+), 27 deletions(-) create mode 100644 apps/mcp-server/tests/test_iserror_compliance.py diff --git a/apps/mcp-server/src/taskflow_mcp/tools/tasks.py b/apps/mcp-server/src/taskflow_mcp/tools/tasks.py index 3b70047..58c6c50 100644 --- a/apps/mcp-server/src/taskflow_mcp/tools/tasks.py +++ b/apps/mcp-server/src/taskflow_mcp/tools/tasks.py @@ -105,9 +105,9 @@ async def taskflow_add_task(params: AddTaskInput, ctx: Context) -> str: ) return _format_task_result(result, "created") except APIError as e: - return _format_error(e) - except Exception as e: - return json.dumps({"error": True, "message": str(e)}) + raise + except Exception: + raise @mcp.tool( @@ -170,9 +170,9 @@ async def taskflow_list_tasks(params: ListTasksInput, ctx: Context) -> str: ] return json.dumps(result, indent=2) except APIError as e: - return _format_error(e) - except Exception as e: - return json.dumps({"error": True, "message": str(e)}) + raise + except Exception: + raise @mcp.tool( @@ -247,9 +247,9 @@ async def taskflow_update_task(params: UpdateTaskInput, ctx: Context) -> str: ) return _format_task_result(result, "updated") except APIError as e: - return _format_error(e, params.task_id) - except Exception as e: - return json.dumps({"error": True, "message": str(e), "task_id": params.task_id}) + raise + except Exception: + raise @mcp.tool( @@ -292,9 +292,9 @@ async def taskflow_delete_task(params: TaskIdInput, ctx: Context) -> str: indent=2, ) except APIError as e: - return _format_error(e, params.task_id) - except Exception as e: - return json.dumps({"error": True, "message": str(e), "task_id": params.task_id}) + raise + except Exception: + raise # ============================================================================= @@ -338,9 +338,9 @@ async def taskflow_start_task(params: TaskIdInput, ctx: Context) -> str: ) return _format_task_result(result, "in_progress") except APIError as e: - return _format_error(e, params.task_id) - except Exception as e: - return json.dumps({"error": True, "message": str(e), "task_id": params.task_id}) + raise + except Exception: + raise @mcp.tool( @@ -379,9 +379,9 @@ async def taskflow_complete_task(params: TaskIdInput, ctx: Context) -> str: ) return _format_task_result(result, "completed") except APIError as e: - return _format_error(e, params.task_id) - except Exception as e: - return json.dumps({"error": True, "message": str(e), "task_id": params.task_id}) + raise + except Exception: + raise @mcp.tool( @@ -420,9 +420,9 @@ async def taskflow_request_review(params: TaskIdInput, ctx: Context) -> str: ) return _format_task_result(result, "review") except APIError as e: - return _format_error(e, params.task_id) - except Exception as e: - return json.dumps({"error": True, "message": str(e), "task_id": params.task_id}) + raise + except Exception: + raise @mcp.tool( @@ -463,9 +463,9 @@ async def taskflow_update_progress(params: ProgressInput, ctx: Context) -> str: ) return _format_task_result(result, result.get("status", "in_progress")) except APIError as e: - return _format_error(e, params.task_id) - except Exception as e: - return json.dumps({"error": True, "message": str(e), "task_id": params.task_id}) + raise + except Exception: + raise @mcp.tool( @@ -502,6 +502,6 @@ async def taskflow_assign_task(params: AssignInput, ctx: Context) -> str: ) return _format_task_result(result, "assigned") except APIError as e: - return _format_error(e, params.task_id) - except Exception as e: - return json.dumps({"error": True, "message": str(e), "task_id": params.task_id}) + raise + except Exception: + raise diff --git a/apps/mcp-server/tests/test_iserror_compliance.py b/apps/mcp-server/tests/test_iserror_compliance.py new file mode 100644 index 0000000..5354b3a --- /dev/null +++ b/apps/mcp-server/tests/test_iserror_compliance.py @@ -0,0 +1,55 @@ +"""Regression test for isError-compliance in taskflow_mcp task tools. + +9 `except APIError as e:` and 9 `except Exception as e:` handlers in +`apps/mcp-server/src/taskflow_mcp/tools/tasks.py` returned a JSON-encoded +`{"error": True, "message": ...}` string. FastMCP wraps the return +value as success content with `isError=false`, so MCP clients treat +the failure as data and the LLM often proceeds as if the call had +succeeded. + +The fix replaces each swallowed-error return with bare `raise` so the +original exception propagates and FastMCP sets `isError=true` on the +wire. + +Reference: https://composio.dev/blog/mcp-security-vulnerabilities (Dayna +Blackwell MCP security audit, June 2026). +""" +import pytest + +try: + from fastmcp.exceptions import ToolError +except ImportError: + ToolError = None # type: ignore + +pytestmark = pytest.mark.skipif( + ToolError is None, reason="fastmcp not installed; skip when unavailable" +) + + +@pytest.mark.asyncio +async def test_create_task_api_error_raises_tool_error(monkeypatch): + """An APIError during create_task must surface as ToolError + → isError=true on the wire, not as JSON success content.""" + from taskflow_mcp.tools import tasks + + async def _raise(*args, **kwargs): + raise tasks.APIError("upstream down") + + monkeypatch.setattr(tasks, "get_api_client", lambda: _make_client(_raise)) + + with pytest.raises(ToolError) as exc_info: + await tasks.create_task.fn( + user_id="u1", + project_id="p1", + title="t", + description="d", + is_recurring=False, + ) + assert "upstream down" in str(exc_info.value) or "Error" in str(exc_info.value) + + +def _make_client(_raise): + class _C: + async def create_task(self, *args, **kwargs): + return await _raise(*args, **kwargs) + return _C()