mirror of
https://github.com/safishamsi/graphify.git
synced 2026-10-15 16:39:43 +00:00
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.
This commit is contained in:
1 parent
35c63493e5
commit
09151a6727
2 files changed
+61
-4
No files matched your search
+25
-4
@@ -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:
|
||||
|
||||
@@ -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")
|
||||
Reference in new issue
Block a user