fix: harden MCP write tool errors

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 |

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()

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"]

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