diff --git a/planfile/api/server.py b/planfile/api/server.py index 3144c45..97564c6 100644 --- a/planfile/api/server.py +++ b/planfile/api/server.py @@ -986,6 +986,38 @@ def public_forensic_log_days(): } +#: Sub-models a PATCH may address. A request carries each of them as a complete +#: object, so they are merged rather than written through. +_MERGEABLE_TICKET_SECTIONS = ("executor", "execution", "inputs", "outputs") + + +def _merged_ticket_updates(body: "TicketUpdate", current) -> dict: + """Apply only the fields a PATCH actually sent. + + FastAPI parses `{"inputs": {"uri_processes": [...]}}` into a complete + `TicketInputs`, filling every omitted field with its model default. Writing + that straight through replaces the stored section, so one partial update + silently erases everything the caller did not mention — `inputs.api_body`, + `execution.started_at`, `execution.last_error` and the rest. + + Pydantic records which fields were present at each level, so merge the sent + keys onto what is stored. An explicitly sent `null` still clears its field; + only omission is now inert. + """ + updates = {k: v for k, v in body.model_dump().items() if v is not None} + for section in _MERGEABLE_TICKET_SECTIONS: + if section not in body.model_fields_set: + continue + sent = getattr(body, section) + if sent is None: + continue + supplied = sent.model_dump(mode="json", exclude_unset=True) + stored = getattr(current, section, None) + base = stored.model_dump(mode="json", exclude_none=True) if stored is not None else {} + updates[section] = {**base, **supplied} + return updates + + @app.patch("/tickets/{ticket_id}", tags=["tickets"]) async def update_ticket(ticket_id: str, body: TicketUpdate): pf = get_planfile() @@ -1001,7 +1033,7 @@ async def update_ticket(ticket_id: str, body: TicketUpdate): body.inputs if body.inputs is not None else current.inputs, require_for_legacy=False, ) - updates = {k: v for k, v in body.model_dump().items() if v is not None} + updates = _merged_ticket_updates(body, current) if body.status is not None and str(body.status) != str(current.status.value): updates["actor"] = body.actor or "unknown:api" updates["reason"] = body.reason or f"status_transition:{current.status.value}->{body.status}" diff --git a/tests/test_partial_ticket_update.py b/tests/test_partial_ticket_update.py new file mode 100644 index 0000000..e6802d5 --- /dev/null +++ b/tests/test_partial_ticket_update.py @@ -0,0 +1,106 @@ +"""A PATCH must change only the fields it sends. + +Observed 2026-09-03 on a running deployment: twelve supervisor plans sat in +`waiting_input` for up to 152 hours with an automatic recovery that could never +see them. The recovery keys on `inputs.api_body`; the producer wrote it, and a +later unrelated PATCH — one that only meant to set `uri_processes` — erased it, +because FastAPI had filled every omitted field of `TicketInputs` with its model +default before the store wrote the section through. + +The same shape erased `execution.last_error` and `execution.started_at` +elsewhere. These tests pin the section-merge behaviour that stops it. +""" + +from __future__ import annotations + +import pytest +from fastapi.testclient import TestClient + + +@pytest.fixture() +def client(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + from planfile.api import server + + server.get_planfile.cache_clear() if hasattr(server.get_planfile, "cache_clear") else None + return TestClient(server.app) + + +def create(client, **overrides): + payload = {"name": "Supervisor plan: verify the seam", "priority": "high", **overrides} + response = client.post("/tickets", json=payload) + assert response.status_code in (200, 201), response.text + return response.json()["id"] + + +def get_inputs(client, ticket_id): + response = client.get(f"/tickets/{ticket_id}") + assert response.status_code == 200, response.text + return response.json().get("inputs") or {} + + +def get_execution(client, ticket_id): + response = client.get(f"/tickets/{ticket_id}") + assert response.status_code == 200, response.text + return response.json().get("execution") or {} + + +def patch(client, ticket_id, body): + response = client.patch( + f"/tickets/{ticket_id}", + json={"actor": "test:agent", "reason": "covering the partial-update seam", **body}, + ) + assert response.status_code == 200, response.text + return response.json() + + +def test_a_partial_inputs_patch_keeps_the_fields_it_did_not_send(client): + envelope = {"schema": "subactor.supervisor-ticket-input/v1", "apply": False, "goal": "ship it"} + ticket_id = create(client, inputs={"api_body": envelope, "prompt": "original prompt"}) + assert get_inputs(client, ticket_id)["api_body"] == envelope + + # The exact shape that erased it: a write that only means to set processes. + patch(client, ticket_id, {"inputs": {"uri_processes": []}}) + + stored = get_inputs(client, ticket_id) + assert stored["api_body"] == envelope, "an unmentioned section field must survive" + assert stored["prompt"] == "original prompt" + + +def test_a_partial_execution_patch_keeps_the_failure_reason(client): + ticket_id = create(client) + patch(client, ticket_id, {"execution": {"state": "running", "last_error": "bridge_422:rejected"}}) + assert get_execution(client, ticket_id)["last_error"] == "bridge_422:rejected" + + # A heartbeat-shaped write that names only the queue. + patch(client, ticket_id, {"execution": {"queue": "project-operator-bot"}}) + + execution = get_execution(client, ticket_id) + assert execution["last_error"] == "bridge_422:rejected", "a heartbeat must not erase the reason" + assert execution["queue"] == "project-operator-bot" + + +def test_a_sent_field_still_overwrites(client): + ticket_id = create(client, inputs={"prompt": "first"}) + patch(client, ticket_id, {"inputs": {"prompt": "second"}}) + assert get_inputs(client, ticket_id)["prompt"] == "second" + + +def test_an_explicit_null_still_clears_a_field(client): + ticket_id = create(client, inputs={"prompt": "first"}) + patch(client, ticket_id, {"inputs": {"prompt": None}}) + # Omission is inert; sending null is still how a caller clears a field. + assert "prompt" not in get_inputs(client, ticket_id) + + +def test_untouched_sections_are_left_alone(client): + ticket_id = create(client, inputs={"prompt": "keep me"}) + patch(client, ticket_id, {"execution": {"queue": "somewhere"}}) + assert get_inputs(client, ticket_id)["prompt"] == "keep me" + + +def test_a_patch_that_sends_no_section_changes_nothing_in_them(client): + envelope = {"schema": "subactor.supervisor-ticket-input/v1", "apply": False} + ticket_id = create(client, inputs={"api_body": envelope}) + patch(client, ticket_id, {"priority": "low"}) + assert get_inputs(client, ticket_id)["api_body"] == envelope