From a09839ce723eb6e7957b7d8e601cb9799512fb24 Mon Sep 17 00:00:00 2001 From: Tom Softreck Date: Fri, 4 Sep 2026 00:10:26 +0200 Subject: [PATCH] fix(api): apply only the fields a ticket PATCH actually sent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FastAPI parses `{"inputs": {"uri_processes": [...]}}` into a complete TicketInputs, filling every omitted field with its model default, and the store applies sections with a shallow dict update. One partial write therefore erased everything the caller did not mention. Observed on a running deployment, 2026-09-03. 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 reset it to None. The same shape erased execution.last_error and left scheduler occurrences with no started_at, so a watchdog failed handlers that had in fact run cleanly. Pydantic records which fields were present at each nesting level, so merge the sent keys of executor, execution, inputs and outputs onto what is stored. An explicitly sent null still clears its field; only omission is now inert. Six tests pin the behaviour, including the two exact production shapes. Both fail without the merge; the four that assert unchanged semantics — overwrite, explicit null, untouched sections, a patch naming no section — pass either way and stand as regression guards. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015QJeEkvVSGaDJyaxu4aoRL --- planfile/api/server.py | 34 ++++++++- tests/test_partial_ticket_update.py | 106 ++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 tests/test_partial_ticket_update.py diff --git a/planfile/api/server.py b/planfile/api/server.py index ea5b42d..adb27b1 100644 --- a/planfile/api/server.py +++ b/planfile/api/server.py @@ -974,6 +974,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() @@ -989,7 +1021,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