From 2cad5da0abfa58ee4d60cfa82d085f3cdaec5699 Mon Sep 17 00:00:00 2001 From: tegwick Date: Sun, 7 Jun 2026 19:30:58 +0200 Subject: [PATCH] fix: harden MCP write tool errors --- mcp_server/TOOLS.md | 36 +++ mcp_server/server.py | 124 +++++++-- tests/test_mcp_write_tools.py | 254 ++++++++++++++++++ ...ATE-WP-0059-mcp-write-layer-reliability.md | 31 ++- 4 files changed, 424 insertions(+), 21 deletions(-) create mode 100644 tests/test_mcp_write_tools.py diff --git a/mcp_server/TOOLS.md b/mcp_server/TOOLS.md index dd9d8eb..cc970cc 100644 --- a/mcp_server/TOOLS.md +++ b/mcp_server/TOOLS.md @@ -20,6 +20,42 @@ Do not use them as a substitute for formal work definition inside the domain rep --- +## MCP/REST Parity and Failure Handling + +The MCP server is a thin stateless HTTP client over the FastAPI service. On +successful writes, MCP tools return the same JSON object shape as the REST +endpoint they wrap: + +| MCP tool | REST endpoint | +|---|---| +| `create_workstream(...)` | `POST /workstreams/` | +| `create_task(...)` | `POST /tasks/` | +| `update_task_status(...)` | `PATCH /tasks/{task_id}` | +| `record_decision(...)` | `POST /decisions/` | +| `add_progress_event(...)` | `POST /progress/` | + +For write tools that emit automatic progress events, the progress event is only +sent after the primary REST write returns a valid object. If the API is +unreachable, returns an HTTP error, or returns a malformed object, the MCP tool +returns a JSON error payload instead: + +```json +{ + "error": "API 404: ...", + "tool": "update_task_status", + "response": {"error": "API 404: ..."} +} +``` + +That error is intentional and actionable: do not treat it as success. Fall back +to the corresponding REST `curl` call from the repo instructions, then record a +normal progress event once the REST write succeeds. If the primary write +succeeds but its automatic progress event fails, the tool returns an error with +the successful `write_result` included so the caller can avoid duplicating the +entity while recording the missing progress event. + +--- + ## Query Tools (read-only, use freely) | Tool | Key Args | When to use | diff --git a/mcp_server/server.py b/mcp_server/server.py index 51d941a..edfbae2 100644 --- a/mcp_server/server.py +++ b/mcp_server/server.py @@ -92,6 +92,54 @@ def _delete(path: str) -> None: return {"error": f"Request failed: {e}"} +def _mcp_error(tool_name: str, message: str, response: Any | None = None) -> dict[str, Any]: + payload: dict[str, Any] = {"error": message, "tool": tool_name} + if response is not None: + payload["response"] = response + return payload + + +def _response_error( + tool_name: str, + response: Any, + required_fields: tuple[str, ...] = (), +) -> dict[str, Any] | None: + """Return an MCP-visible error payload for failed or malformed API results.""" + if isinstance(response, dict) and isinstance(response.get("error"), str): + return _mcp_error(tool_name, response["error"], response) + if not isinstance(response, dict): + return _mcp_error(tool_name, "API returned a non-object response", response) + + missing = [field for field in required_fields if response.get(field) is None] + if missing: + return _mcp_error( + tool_name, + f"API response missing required field(s): {', '.join(missing)}", + response, + ) + return None + + +def _emit_progress_event( + tool_name: str, + write_result: dict[str, Any], + body: dict[str, Any], +) -> dict[str, Any] | None: + progress = _post("/progress", body) + error = _response_error(f"{tool_name}.progress_event", progress, ("id",)) + if error: + return _mcp_error( + tool_name, + "Primary write succeeded, but automatic progress_event failed", + {"write_result": write_result, "progress_error": error}, + ) + return None + + +def _json_result(result: Any) -> str: + return json.dumps(result, indent=2) + + # --------------------------------------------------------------------------- # Resources # --------------------------------------------------------------------------- @@ -449,7 +497,10 @@ def create_workstream( "planning_priority": planning_priority, "planning_order": planning_order, }) - _post("/progress", { + if error := _response_error("create_workstream", ws, ("id",)): + return _json_result(error) + + progress_error = _emit_progress_event("create_workstream", ws, { "topic_id": topic_id, "workstream_id": ws["id"], "event_type": "workstream_created", @@ -457,7 +508,9 @@ def create_workstream( "author": "custodian", "detail": {"owner": owner, "slug": slug}, }) - return json.dumps(ws, indent=2) + if progress_error: + return _json_result(progress_error) + return _json_result(ws) @mcp.tool() @@ -487,7 +540,10 @@ def create_task( "assignee": assignee, "due_date": due_date, }) - _post("/progress", { + if error := _response_error("create_task", task, ("id",)): + return _json_result(error) + + progress_error = _emit_progress_event("create_task", task, { "workstream_id": workstream_id, "task_id": task["id"], "event_type": "task_created", @@ -495,7 +551,9 @@ def create_task( "author": "custodian", "detail": {"priority": priority, "assignee": assignee}, }) - return json.dumps(task, indent=2) + if progress_error: + return _json_result(progress_error) + return _json_result(task) @mcp.tool() @@ -558,7 +616,10 @@ def update_task_status( body["token_note"] = note task = _patch(f"/tasks/{task_id}", body) - _post("/progress", { + if error := _response_error("update_task_status", task, ("id", "title")): + return _json_result(error) + + progress_error = _emit_progress_event("update_task_status", task, { "task_id": task_id, "workstream_id": task.get("workstream_id"), "event_type": "task_status_changed", @@ -566,8 +627,10 @@ def update_task_status( "author": "custodian", "detail": {"blocking_reason": blocking_reason}, }) + if progress_error: + return _json_result(progress_error) - return json.dumps(task, indent=2) + return _json_result(task) @mcp.tool() @@ -585,7 +648,10 @@ def flag_for_human(task_id: str, note: str) -> str: "needs_human": True, "intervention_note": note, }) - _post("/progress", { + if error := _response_error("flag_for_human", task, ("id", "title")): + return _json_result(error) + + progress_error = _emit_progress_event("flag_for_human", task, { "task_id": task_id, "workstream_id": task.get("workstream_id"), "event_type": "task_flagged_human", @@ -593,7 +659,9 @@ def flag_for_human(task_id: str, note: str) -> str: "author": "custodian", "detail": {"intervention_note": note}, }) - return json.dumps(task, indent=2) + if progress_error: + return _json_result(progress_error) + return _json_result(task) @mcp.tool() @@ -609,14 +677,19 @@ def clear_human_flag(task_id: str) -> str: task = _patch(f"/tasks/{task_id}", { "needs_human": False, }) - _post("/progress", { + if error := _response_error("clear_human_flag", task, ("id", "title")): + return _json_result(error) + + progress_error = _emit_progress_event("clear_human_flag", task, { "task_id": task_id, "workstream_id": task.get("workstream_id"), "event_type": "task_flag_cleared", "summary": f"Human-intervention flag cleared: {task['title']}", "author": "custodian", }) - return json.dumps(task, indent=2) + if progress_error: + return _json_result(progress_error) + return _json_result(task) @mcp.tool() @@ -671,7 +744,10 @@ def record_decision( "decided_by": decided_by, "deadline": deadline, }) - _post("/progress", { + if error := _response_error("record_decision", decision, ("id",)): + return _json_result(error) + + progress_error = _emit_progress_event("record_decision", decision, { "topic_id": topic_id, "workstream_id": workstream_id, "decision_id": decision["id"], @@ -680,7 +756,9 @@ def record_decision( "author": "custodian", "detail": {"status": decision.get("status"), "escalation_note": decision.get("escalation_note")}, }) - return json.dumps(decision, indent=2) + if progress_error: + return _json_result(progress_error) + return _json_result(decision) @mcp.tool() @@ -703,7 +781,10 @@ def resolve_decision( "decided_by": decided_by, "decided_at": datetime.now(tz=timezone.utc).isoformat(), }) - _post("/progress", { + if error := _response_error("resolve_decision", decision, ("id", "title")): + return _json_result(error) + + progress_error = _emit_progress_event("resolve_decision", decision, { "topic_id": decision.get("topic_id"), "workstream_id": decision.get("workstream_id"), "decision_id": decision_id, @@ -712,7 +793,9 @@ def resolve_decision( "author": "custodian", "detail": {"rationale": rationale}, }) - return json.dumps(decision, indent=2) + if progress_error: + return _json_result(progress_error) + return _json_result(decision) @mcp.tool() @@ -748,7 +831,9 @@ def add_progress_event( "author": "custodian", "detail": detail, }) - return json.dumps(event, indent=2) + if error := _response_error("add_progress_event", event, ("id",)): + return _json_result(error) + return _json_result(event) @mcp.tool() @@ -760,14 +845,19 @@ def update_workstream_status(workstream_id: str, status: str) -> str: status: proposed | ready | active | blocked | backlog | finished | archived """ ws = _patch(f"/workstreams/{workstream_id}", {"status": status}) - _post("/progress", { + if error := _response_error("update_workstream_status", ws, ("id", "title")): + return _json_result(error) + + progress_error = _emit_progress_event("update_workstream_status", ws, { "workstream_id": workstream_id, "topic_id": ws.get("topic_id"), "event_type": "workstream_status_changed", "summary": f"Workstream status → {status}: {ws['title']}", "author": "custodian", }) - return json.dumps(ws, indent=2) + if progress_error: + return _json_result(progress_error) + return _json_result(ws) @mcp.tool() diff --git a/tests/test_mcp_write_tools.py b/tests/test_mcp_write_tools.py new file mode 100644 index 0000000..0eda9aa --- /dev/null +++ b/tests/test_mcp_write_tools.py @@ -0,0 +1,254 @@ +""" +Regression tests for State Hub MCP write tools. + +These call the registered FastMCP tools in-process and monkeypatch only the +thin HTTP helpers. That exercises FastMCP argument validation and the MCP +wrapper logic without depending on a running State Hub API. +""" +from __future__ import annotations + +import json +from typing import Any + +import mcp_server.server as server + + +async def _call_tool(name: str, arguments: dict[str, Any]) -> dict[str, Any]: + result = await server.mcp.call_tool(name, arguments) + return json.loads(result.content[0].text) + + +class TestMCPWriteTools: + async def test_create_workstream_returns_rest_shape_and_emits_progress(self, monkeypatch): + calls: list[tuple[str, dict[str, Any]]] = [] + + def fake_post(path: str, body: dict[str, Any]) -> dict[str, Any]: + calls.append((path, body)) + if path == "/workstreams": + return { + "id": "ws-1", + "topic_id": body["topic_id"], + "title": body["title"], + "slug": body["slug"], + "status": body["status"], + } + if path == "/progress": + return {"id": "event-1", **body} + raise AssertionError(f"unexpected POST {path}") + + monkeypatch.setattr(server, "_post", fake_post) + + body = await _call_tool( + "create_workstream", + {"topic_id": "topic-1", "title": "MCP Reliable Write"}, + ) + + assert body == { + "id": "ws-1", + "topic_id": "topic-1", + "title": "MCP Reliable Write", + "slug": "mcp-reliable-write", + "status": "active", + } + assert [path for path, _ in calls] == ["/workstreams", "/progress"] + assert calls[1][1]["workstream_id"] == "ws-1" + assert calls[1][1]["event_type"] == "workstream_created" + + async def test_create_task_returns_rest_shape_and_emits_progress(self, monkeypatch): + calls: list[tuple[str, dict[str, Any]]] = [] + + def fake_post(path: str, body: dict[str, Any]) -> dict[str, Any]: + calls.append((path, body)) + if path == "/tasks": + return { + "id": "task-1", + "workstream_id": body["workstream_id"], + "title": body["title"], + "priority": body["priority"], + "status": "todo", + } + if path == "/progress": + return {"id": "event-1", **body} + raise AssertionError(f"unexpected POST {path}") + + monkeypatch.setattr(server, "_post", fake_post) + + body = await _call_tool( + "create_task", + {"workstream_id": "ws-1", "title": "MCP task", "priority": "high"}, + ) + + assert body == { + "id": "task-1", + "workstream_id": "ws-1", + "title": "MCP task", + "priority": "high", + "status": "todo", + } + assert [path for path, _ in calls] == ["/tasks", "/progress"] + assert calls[1][1]["task_id"] == "task-1" + assert calls[1][1]["event_type"] == "task_created" + + async def test_update_task_status_api_error_is_clear_and_skips_progress(self, monkeypatch): + post_calls: list[tuple[str, dict[str, Any]]] = [] + + def fake_patch(path: str, body: dict[str, Any]) -> dict[str, Any]: + assert path == "/tasks/task-1" + assert body["status"] == "done" + return {"error": "API 404: task not found"} + + monkeypatch.setattr(server, "_patch", fake_patch) + monkeypatch.setattr(server, "_post", lambda path, body: post_calls.append((path, body))) + + body = await _call_tool( + "update_task_status", + {"task_id": "task-1", "status": "done"}, + ) + + assert body["tool"] == "update_task_status" + assert body["error"] == "API 404: task not found" + assert post_calls == [] + + async def test_update_task_status_returns_rest_shape_and_emits_progress(self, monkeypatch): + post_calls: list[tuple[str, dict[str, Any]]] = [] + + def fake_patch(path: str, body: dict[str, Any]) -> dict[str, Any]: + assert path == "/tasks/task-1" + assert body["status"] == "done" + return { + "id": "task-1", + "workstream_id": "ws-1", + "title": "Finish MCP reliability", + "status": "done", + } + + def fake_post(path: str, body: dict[str, Any]) -> dict[str, Any]: + post_calls.append((path, body)) + return {"id": "event-1", **body} + + monkeypatch.setattr(server, "_patch", fake_patch) + monkeypatch.setattr(server, "_post", fake_post) + + body = await _call_tool( + "update_task_status", + {"task_id": "task-1", "status": "done"}, + ) + + assert body == { + "id": "task-1", + "workstream_id": "ws-1", + "title": "Finish MCP reliability", + "status": "done", + } + assert [path for path, _ in post_calls] == ["/progress"] + assert post_calls[0][1]["task_id"] == "task-1" + assert post_calls[0][1]["event_type"] == "task_status_changed" + + async def test_add_progress_event_accepts_json_string_detail(self, monkeypatch): + calls: list[tuple[str, dict[str, Any]]] = [] + + def fake_post(path: str, body: dict[str, Any]) -> dict[str, Any]: + calls.append((path, body)) + return {"id": "event-1", **body} + + monkeypatch.setattr(server, "_post", fake_post) + + body = await _call_tool( + "add_progress_event", + { + "summary": "MCP progress", + "event_type": "note", + "workstream_id": "ws-1", + "detail": '{"files_changed": 3}', + }, + ) + + assert body["id"] == "event-1" + assert body["detail"] == {"files_changed": 3} + assert calls == [ + ( + "/progress", + { + "topic_id": None, + "workstream_id": "ws-1", + "task_id": None, + "event_type": "note", + "summary": "MCP progress", + "author": "custodian", + "detail": {"files_changed": 3}, + }, + ) + ] + + async def test_record_decision_returns_rest_shape_and_emits_progress(self, monkeypatch): + calls: list[tuple[str, dict[str, Any]]] = [] + + def fake_post(path: str, body: dict[str, Any]) -> dict[str, Any]: + calls.append((path, body)) + if path == "/decisions": + return { + "id": "decision-1", + "title": body["title"], + "decision_type": body["decision_type"], + "topic_id": body["topic_id"], + "workstream_id": body["workstream_id"], + "status": "open", + "escalation_note": None, + } + if path == "/progress": + return {"id": "event-1", **body} + raise AssertionError(f"unexpected POST {path}") + + monkeypatch.setattr(server, "_post", fake_post) + + body = await _call_tool( + "record_decision", + { + "title": "Keep MCP stateless", + "decision_type": "made", + "topic_id": "topic-1", + "workstream_id": "ws-1", + }, + ) + + assert body["id"] == "decision-1" + assert body["title"] == "Keep MCP stateless" + assert [path for path, _ in calls] == ["/decisions", "/progress"] + assert calls[1][1]["decision_id"] == "decision-1" + assert calls[1][1]["event_type"] == "decision_recorded" + + async def test_create_workstream_api_error_skips_progress(self, monkeypatch): + calls: list[tuple[str, dict[str, Any]]] = [] + + def fake_post(path: str, body: dict[str, Any]) -> dict[str, Any]: + calls.append((path, body)) + return {"error": "API 422: invalid topic"} + + monkeypatch.setattr(server, "_post", fake_post) + + body = await _call_tool( + "create_workstream", + {"topic_id": "bad-topic", "title": "No progress on failure"}, + ) + + assert body["tool"] == "create_workstream" + assert body["error"] == "API 422: invalid topic" + assert [path for path, _ in calls] == ["/workstreams"] + + async def test_record_decision_missing_id_is_clear_and_skips_progress(self, monkeypatch): + calls: list[tuple[str, dict[str, Any]]] = [] + + def fake_post(path: str, body: dict[str, Any]) -> dict[str, Any]: + calls.append((path, body)) + return {"title": body["title"], "status": "open"} + + monkeypatch.setattr(server, "_post", fake_post) + + body = await _call_tool( + "record_decision", + {"title": "Malformed API response", "topic_id": "topic-1"}, + ) + + assert body["tool"] == "record_decision" + assert body["error"] == "API response missing required field(s): id" + assert [path for path, _ in calls] == ["/decisions"] diff --git a/workplans/STATE-WP-0059-mcp-write-layer-reliability.md b/workplans/STATE-WP-0059-mcp-write-layer-reliability.md index 196d1ec..aa02db6 100644 --- a/workplans/STATE-WP-0059-mcp-write-layer-reliability.md +++ b/workplans/STATE-WP-0059-mcp-write-layer-reliability.md @@ -4,7 +4,7 @@ type: workplan title: "State Hub MCP write-layer reliability" domain: custodian repo: state-hub -status: ready +status: finished owner: codex topic_slug: custodian created: "2026-06-07" @@ -67,7 +67,7 @@ record status/progress, leaving file↔hub drift unreconciled. ```task id: STATE-WP-0059-T01 -status: todo +status: done priority: high state_hub_task_id: "7fc04b00-2493-4310-ae76-101660621da6" ``` @@ -90,7 +90,7 @@ tool wrapper, not only the FastAPI endpoint, and records: ```task id: STATE-WP-0059-T02 -status: todo +status: done priority: high state_hub_task_id: "2636e1d5-142e-463f-875e-4dc1edf12853" ``` @@ -117,7 +117,7 @@ Implementation notes: ```task id: STATE-WP-0059-T03 -status: todo +status: done priority: medium state_hub_task_id: "af137d6b-07ca-4110-abb0-45a96b84b7a3" ``` @@ -143,3 +143,26 @@ After workplan updates, run from `~/state-hub`: ```bash make fix-consistency REPO=state-hub ``` + +## Verification Notes + +Completed 2026-06-07: + +- Added shared MCP response/error helpers in `mcp_server/server.py` so API error + payloads and malformed REST responses return clear MCP-visible errors instead + of triggering response-field KeyErrors or false success. +- Kept successful MCP write responses in the same JSON shape as their REST + equivalents, while preserving automatic progress events only after successful + primary writes. +- Added `tests/test_mcp_write_tools.py`, which invokes the registered FastMCP + tools in-process and covers success/error behavior for `create_workstream`, + `create_task`, `update_task_status`, `add_progress_event`, and + `record_decision`. +- Documented MCP/REST write parity and fallback expectations in + `mcp_server/TOOLS.md`. + +Verification: + +- `.venv/bin/python -m pytest tests/test_mcp_write_tools.py -q` -> 8 passed +- `.venv/bin/python -m pytest tests/test_mcp_write_tools.py tests/test_mcp_smoke.py -q` -> 21 passed +- `git diff --check` -> clean