From 09151a67270a8fe08677374171ad7d044a6309a2 Mon Sep 17 00:00:00 2001 From: noQbot <299857951+noQbot@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:37:06 +0100 Subject: [PATCH] Mark failed PR tool calls as isError instead of successful text The prs MCP tools (list_prs, triage_prs, get_pr_impact) returned genuine failures (gh missing/unauth, unresolved PR) as success text. Raise a ToolError propagated as isError on both the 1.x and 2.x SDK paths, so a client sees the failure; an empty-but-successful result (0 PRs, PR with no changed files) is unaffected. Mirrors the #2725 MCP-error pattern. --- graphify/serve.py | 29 +++++++++++++++++++++++++---- tests/test_serve_http.py | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/graphify/serve.py b/graphify/serve.py index 58c0925b4..94c5d480a 100644 --- a/graphify/serve.py +++ b/graphify/serve.py @@ -22,6 +22,16 @@ except ImportError: _jieba = None +class ToolError(Exception): + """Raised by a tool handler to signal an error result. + + A normal string return is sent as an ordinary (successful) text result. A + ToolError is instead turned into a tool result with ``isError: true`` so a + client that only checks ``isError`` can tell a genuine failure — e.g. the + ``gh`` CLI missing or a PR that cannot be resolved — from success. + """ + + def _load_graph(graph_path: str) -> nx.Graph: try: resolved = Path(graph_path).resolve() @@ -1877,7 +1887,7 @@ def _build_server(graph_path: str): try: prs = fetch_prs(repo=repo, base=base) except RuntimeError as e: - return f"Error: {e}" + raise ToolError(f"Error: {e}") from e worktrees = fetch_worktrees() for pr in prs: pr.worktree_path = worktrees.get(pr.branch) @@ -1894,7 +1904,7 @@ def _build_server(graph_path: str): view_args += ["--repo", repo] pr_data = _gh(*view_args) if pr_data is None: - return f"PR #{number} not found or gh not authenticated." + raise ToolError(f"PR #{number} not found or gh not authenticated.") files = fetch_pr_files(number, repo) if not files: return f"PR #{number}: no changed files found (may require gh auth)." @@ -1921,7 +1931,7 @@ def _build_server(graph_path: str): try: prs = fetch_prs(repo=repo, base=base) except RuntimeError as e: - return f"Error: {e}" + raise ToolError(f"Error: {e}") from e worktrees = fetch_worktrees() for pr in prs: pr.worktree_path = worktrees.get(pr.branch) @@ -2049,6 +2059,11 @@ def _build_server(graph_path: str): try: _select_graph(project_path) # bind G/communities to the target graph return [types.TextContent(type="text", text=handler(arguments))] + except ToolError: + # A handler-signalled error: propagate so the result is marked + # isError:true (the mcp 1.x decorator wraps a raised exception into + # an error result; the 2.x path catches it in _on_call_tool). + raise except Exception as exc: return [types.TextContent(type="text", text=f"Error executing {name}: {exc}")] @@ -2068,7 +2083,13 @@ def _build_server(graph_path: str): return types.ListToolsResult(tools=await list_tools()) async def _on_call_tool(ctx, params) -> types.CallToolResult: - content = await call_tool(params.name, dict(params.arguments or {})) + try: + content = await call_tool(params.name, dict(params.arguments or {})) + except ToolError as exc: + return types.CallToolResult( + content=[types.TextContent(type="text", text=str(exc))], + isError=True, + ) return types.CallToolResult(content=content) async def _on_list_resources(ctx, params) -> types.ListResourcesResult: diff --git a/tests/test_serve_http.py b/tests/test_serve_http.py index 7893a0f4e..360b325b2 100644 --- a/tests/test_serve_http.py +++ b/tests/test_serve_http.py @@ -361,3 +361,39 @@ def test_cli_api_key_from_env(monkeypatch): monkeypatch.setattr(serve_mod, "serve_http", lambda gp, **k: captured.update(**k)) serve_mod._main(["g.json", "--transport", "http"]) assert captured["api_key"] == "from-env" + + +def test_pr_tool_failure_sets_iserror(tmp_path, monkeypatch): + """ADR-0001 finding 4: a PR tool that fails because gh is missing / not + authenticated must return isError:true, not a successful text result.""" + import graphify.prs as prs_mod + + def _boom(*a, **k): + raise RuntimeError("gh CLI not found or not authenticated. Run: gh auth login") + + monkeypatch.setattr(prs_mod, "fetch_prs", _boom) + + app = serve_mod._build_http_app(_graph_file(tmp_path), json_response=True) + with _client(app) as client: + headers = _init_session(client) + resp = client.post("/mcp", headers=headers, json={ + "jsonrpc": "2.0", "id": 2, "method": "tools/call", + "params": {"name": "list_prs", "arguments": {}}, + }) + assert resp.status_code == 200 + result = resp.json()["result"] + assert result.get("isError") is True, result + assert "gh" in result["content"][0]["text"].lower() + + +def test_normal_tool_result_is_not_iserror(tmp_path): + """A successful tool result must not be marked isError.""" + app = serve_mod._build_http_app(_graph_file(tmp_path), json_response=True) + with _client(app) as client: + headers = _init_session(client) + resp = client.post("/mcp", headers=headers, json={ + "jsonrpc": "2.0", "id": 2, "method": "tools/call", + "params": {"name": "graph_stats", "arguments": {}}, + }) + assert resp.status_code == 200 + assert not resp.json()["result"].get("isError")