Set isError on MCP tool-level failures - #2725
Conversation
A bad project_path, a corrupt project graph, or an unknown tool name all returned isError unset, indistinguishable from success to any caller that branches on the flag rather than reading the text (health checks, CI gates, rollout scripts). call_tool now returns a full CallToolResult with isError set on failure; both the mcp 1.x decorator and the mcp 2.x on_call_tool callback forward it as-is. Fixes Graphify-Labs#2714.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
This PR changes the MCP call_tool handler in graphify/serve.py to return a types.CallToolResult object instead of a bare list[TextContent], setting isError=True on the failure paths (unknown tool, exceptions, graph selection errors) and leaving it unset on success. It updates both the mcp 1.x decorator registration and the mcp 2.x on_call_tool callback to forward this result directly rather than re-wrapping the content. The test file adds a _call_tool_result helper that returns the full result object and four new tests asserting the isError flag behavior across bad project paths, corrupt graphs, successful calls, and unknown tool names.
Worth a look
- call_tool binds shared G/communities per-request without locking; concurrent tool calls race on global graph selection —
graphify/serve.py:1988· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 460 functions depend on the 161 functions this change touches.
Health — this change adds coupling hotspots:
- new:
test_corrupt_project_graph_sets_is_error()— 0 callers, 7 callees
Verification — 460 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 310 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify \_build\_server.
The verifier did not have enough to check \_build\_server, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly ImportError — names the real obstacle, not a sampling gap)
· 1 grounded finding(s) anchored inline below.
|
@safishamsi this PR's CI run is waiting on approval (first-time contributor gate) — could you approve the workflow run when you get a chance? Happy to address any review feedback in the meantime. |
A bad project_path, a corrupt project graph, or an unknown tool name all returned isError unset, indistinguishable from success to any caller that branches on the flag rather than reading the text (health checks, CI gates, rollout scripts). call_tool now returns a full CallToolResult with isError set on failure; both the mcp 1.x decorator and the mcp 2.x on_call_tool callback forward it as-is.
Fixes #2714.