diff --git a/src/gen_mcp_server/client.py b/src/gen_mcp_server/client.py index 1724e84..8de6b83 100644 --- a/src/gen_mcp_server/client.py +++ b/src/gen_mcp_server/client.py @@ -38,6 +38,31 @@ class GenApiError(Exception): """Raised on a non-2xx GEN API response. Message mirrors the TS server.""" +class GenConfirmationRequired(Exception): + """Rails returned 428 for a destructive Vidsheet operation (GEN-4797). + + Rails gates PAT-authenticated destructive Vidsheet deletes behind a + two-phase confirm: the first DELETE returns 428 with an authoritative + ``would_destroy`` preview and a short-lived, single-use ``confirm_token`` + bound to (operation, target-state fingerprint, acting user + agent). + + MCP callers are interactive humans holding a PAT, so they are *in* the + gated class by design. This exception carries the preview back to the + caller instead of collapsing it into an opaque ``GenApiError``, so the + model can show what would be destroyed and re-call with ``confirm_token``. + + We deliberately do NOT auto-confirm: Rails is the authority for target + drift, expiry and one-shot consumption, and an MCP tool must never + manufacture a user's approval. + """ + + def __init__(self, would_destroy: Any, confirm_token: str, body: Any) -> None: + self.would_destroy = would_destroy + self.confirm_token = confirm_token + self.body = body + super().__init__("confirmation_required") + + def _pat_from_http_request() -> str | None: """In hosted (HTTP) mode, read the caller's PAT from the request headers. @@ -98,6 +123,18 @@ async def _call( data: Any = json.loads(text) except json.JSONDecodeError: data = {"raw": text} + if res.status_code == httpx.codes.PRECONDITION_REQUIRED: + # GEN-4808: Rails' two-phase destructive confirm. Surface the preview + # + token rather than the generic error, which is unactionable and made + # every gated MCP delete look like an opaque server failure. + token = data.get("confirm_token") if isinstance(data, dict) else None + if isinstance(token, str) and token: + raise GenConfirmationRequired( + would_destroy=data.get("would_destroy") if isinstance(data, dict) else None, + confirm_token=token, + body=data, + ) + # A 428 without a usable token is a real error — fail closed. if not res.is_success: # Same shape as TS: "API error : " raise GenApiError(f"API error {res.status_code}: {json.dumps(data)}") @@ -161,3 +198,33 @@ async def integration_api_call( def json_result(data: Any) -> str: """FastMCP returns the string as the tool's text content (mirrors jsonResult).""" return json.dumps(data, indent=2) + + +async def gated_delete(path: str, *, confirm_token: str | None = None) -> str: + """DELETE a destructive Vidsheet resource through Rails' two-phase gate. + + GEN-4808. Without ``confirm_token`` Rails answers 428 with the + authoritative ``would_destroy`` preview; we return that preview so the + caller can show the user exactly what would be deleted and then re-call + the same tool with the returned ``confirm_token``. + + The token is minted and validated by Rails and is single-use, expiring, + and bound to the target's state fingerprint — so a stale preview fails + closed rather than deleting something the user never saw. + """ + try: + data = await api_call("DELETE", path) + except GenConfirmationRequired as gate: + return json_result( + { + "status": "confirmation_required", + "would_destroy": gate.would_destroy, + "confirm_token": gate.confirm_token, + "next_step": ( + "Show the user what would_destroy lists. If they confirm, call this " + "same tool again with confirm_token set to the value above. The token " + "is single-use and expires; do not reuse or cache it." + ), + } + ) + return json_result(data) diff --git a/src/gen_mcp_server/server.py b/src/gen_mcp_server/server.py index e413836..81cbfbc 100644 --- a/src/gen_mcp_server/server.py +++ b/src/gen_mcp_server/server.py @@ -9,7 +9,7 @@ from fastmcp import FastMCP from pydantic import Field -from .client import api_call, agent_api_call, agent_core_api_call, form_call, integration_api_call, json_result +from .client import api_call, agent_api_call, agent_core_api_call, form_call, gated_delete, integration_api_call, json_result from .generation_types import resolve_generation_type from .reference import API_REFERENCE from .vidsheet_contract import enum_values, model_facing_fields, require_enum_value @@ -825,14 +825,17 @@ async def gen_reorder_columns( ) return json_result(data) -@mcp.tool(name="gen_delete_column", description="Step 4 (Edit & Generate): Delete a column from a vidsheet. Only ingredient-role columns can be deleted.") +@mcp.tool(name="gen_delete_column", description="Step 4 (Edit & Generate): Delete a column from a vidsheet. Only ingredient-role columns can be deleted. Destructive: the first call returns a would_destroy preview plus a confirm_token; show the preview to the user, then call again with confirm_token to actually delete.") async def gen_delete_column( engine_id: Annotated[str, Field(description="The engine ID")], column_id: Annotated[str, Field(description="The column ID to delete")], agent_id: Annotated[str, Field(description="The agent ID that owns the engine")], + confirm_token: Annotated[str | None, Field(description="Single-use token from this tool's previous confirmation_required response. Omit on the first call.")] = None, ) -> str: - data = await api_call("DELETE", f"/vidsheet/{engine_id}/columns/{column_id}?agent_id={agent_id}") - return json_result(data) + path = f"/vidsheet/{engine_id}/columns/{column_id}?agent_id={agent_id}" + if confirm_token: + path = f"{path}&confirm_token={confirm_token}" + return await gated_delete(path) @mcp.tool(name="gen_list_rows", description="Step 4 (Edit & Generate): List all rows in a vidsheet. A row is one piece of content; cells across its columns are its ingredients and generated outputs.") async def gen_list_rows( @@ -944,15 +947,18 @@ async def gen_reorder_layers( ) return json_result(data) -@mcp.tool(name="gen_delete_layer", description="Step 4 (Edit & Generate): Delete a layer from a cell.") +@mcp.tool(name="gen_delete_layer", description="Step 4 (Edit & Generate): Delete a layer from a cell. Destructive: the first call returns a would_destroy preview plus a confirm_token; show the preview to the user, then call again with confirm_token to actually delete.") async def gen_delete_layer( engine_id: Annotated[str, Field(description="The engine ID")], cell_id: Annotated[str, Field(description="The cell ID")], layer_id: Annotated[str, Field(description="The layer ID to delete")], agent_id: Annotated[str, Field(description="The agent ID that owns the engine")], + confirm_token: Annotated[str | None, Field(description="Single-use token from this tool's previous confirmation_required response. Omit on the first call.")] = None, ) -> str: - data = await api_call("DELETE", f"/vidsheet/{engine_id}/cells/{cell_id}/layers/{layer_id}?agent_id={agent_id}") - return json_result(data) + path = f"/vidsheet/{engine_id}/cells/{cell_id}/layers/{layer_id}?agent_id={agent_id}" + if confirm_token: + path = f"{path}&confirm_token={confirm_token}" + return await gated_delete(path) @mcp.tool(name="gen_list_variables", description="Step 4 (Edit & Generate): Get global variables for a vidsheet. Variables are key-value pairs used for template substitution in prompts and content (e.g. {{brand_name}}).") async def gen_list_variables( @@ -1872,14 +1878,17 @@ async def gen_update_variable( data = await api_call("PATCH", f"/vidsheet/{engine_id}/global_variables/{variable_id}?agent_id={agent_id}", body) return json_result(data) -@mcp.tool(name="gen_delete_variable", description="Step 4 (Edit & Generate): Delete a global variable from a vidsheet.") +@mcp.tool(name="gen_delete_variable", description="Step 4 (Edit & Generate): Delete a global variable from a vidsheet. Destructive: the first call returns a would_destroy preview plus a confirm_token; show the preview to the user, then call again with confirm_token to actually delete.") async def gen_delete_variable( engine_id: Annotated[str, Field(description="The vidsheet/engine ID")], variable_id: Annotated[str, Field(description="The variable ID to delete")], agent_id: Annotated[str, Field(description="The agent ID that owns the engine")], + confirm_token: Annotated[str | None, Field(description="Single-use token from this tool's previous confirmation_required response. Omit on the first call.")] = None, ) -> str: - data = await api_call("DELETE", f"/vidsheet/{engine_id}/global_variables/{variable_id}?agent_id={agent_id}") - return json_result(data) + path = f"/vidsheet/{engine_id}/global_variables/{variable_id}?agent_id={agent_id}" + if confirm_token: + path = f"{path}&confirm_token={confirm_token}" + return await gated_delete(path) # ─── Vidsheet undo / redo (change-set history) ─────────────────────────────── # Backs the undo/redo surface: every grouped write to a vidsheet is diff --git a/tests/test_destructive_confirm.py b/tests/test_destructive_confirm.py new file mode 100644 index 0000000..e922ee8 --- /dev/null +++ b/tests/test_destructive_confirm.py @@ -0,0 +1,102 @@ +"""GEN-4808: MCP delete tools must satisfy Rails' two-phase destructive gate. + +Rails (GEN-4797) gates PAT-authenticated destructive Vidsheet deletes: the +first DELETE returns 428 with an authoritative ``would_destroy`` preview and a +single-use ``confirm_token``. mcp.gen.pro sends the caller's PAT, so it is in +the gated class by design — but before this fix no delete tool accepted a +token and ``client._call`` collapsed the 428 into an opaque ``GenApiError``. +Every MCP column/layer/variable delete against a populated target therefore +failed in production (fail-closed: nothing was destroyed, but nothing worked). + +These tests fail on a tree without the fix: +- no GenConfirmationRequired / gated_delete export +- the 428 body is swallowed, so would_destroy + confirm_token never reach the caller +- the delete tools expose no confirm_token parameter +""" +from __future__ import annotations + +import asyncio +import json + +import httpx +import pytest + +from gen_mcp_server import client as client_mod +from gen_mcp_server.client import ( + GenApiError, + GenConfirmationRequired, + gated_delete, +) + +PREVIEW = { + "would_destroy": {"video_layers": [{"id": 42, "name": "Old Logo"}]}, + "confirm_token": "tok_abc123", +} + + +def _stub_call(monkeypatch, *, status: int, body: dict) -> list[str]: + """Replace api_call with one that mimics client._call's status handling.""" + seen: list[str] = [] + + async def fake_api_call(method: str, path: str, body_arg=None): + seen.append(path) + if status == httpx.codes.PRECONDITION_REQUIRED: + token = body.get("confirm_token") + if isinstance(token, str) and token: + raise GenConfirmationRequired( + would_destroy=body.get("would_destroy"), + confirm_token=token, + body=body, + ) + raise GenApiError(f"API error {status}: {json.dumps(body)}") + return body + + monkeypatch.setattr(client_mod, "api_call", fake_api_call) + return seen + + +def test_gated_delete_surfaces_preview_instead_of_opaque_error(monkeypatch): + """A 428 must return the preview + token, not raise GenApiError.""" + _stub_call(monkeypatch, status=httpx.codes.PRECONDITION_REQUIRED, body=PREVIEW) + + result = json.loads(asyncio.run(gated_delete("/vidsheet/1/columns/2?agent_id=a"))) + + assert result["status"] == "confirmation_required" + assert result["confirm_token"] == "tok_abc123" + assert result["would_destroy"] == PREVIEW["would_destroy"] + # The model must be told what to do next, or it will retry blindly. + assert "confirm_token" in result["next_step"] + + +def test_gated_delete_passes_through_a_successful_delete(monkeypatch): + """With a valid token Rails 2xxes; the tool returns the real body.""" + _stub_call(monkeypatch, status=200, body={"deleted": True}) + + result = json.loads( + asyncio.run(gated_delete("/vidsheet/1/columns/2?agent_id=a&confirm_token=tok_abc123")) + ) + + assert result == {"deleted": True} + + +def test_428_without_a_token_fails_closed(monkeypatch): + """A malformed preview must NOT be treated as approval.""" + _stub_call( + monkeypatch, + status=httpx.codes.PRECONDITION_REQUIRED, + body={"would_destroy": {"video_layers": []}}, # no confirm_token + ) + + with pytest.raises(GenApiError): + asyncio.run(gated_delete("/vidsheet/1/columns/2?agent_id=a")) + + +def test_delete_tools_accept_confirm_token(): + """All three gated delete tools must expose the token parameter.""" + import gen_mcp_server.server as server + + tools = {t.name: t for t in asyncio.run(server.mcp.list_tools())} + for name in ("gen_delete_column", "gen_delete_layer", "gen_delete_variable"): + assert name in tools, f"{name} missing from the tool registry" + params = (getattr(tools[name], "parameters", None) or {}).get("properties", {}) + assert "confirm_token" in params, f"{name} cannot satisfy the Rails 428 gate"