diff --git a/engineering/pid/spec.py b/engineering/pid/spec.py index 1f5d01b..db45ff9 100644 --- a/engineering/pid/spec.py +++ b/engineering/pid/spec.py @@ -3,6 +3,7 @@ from __future__ import annotations import copy +import math from typing import TYPE_CHECKING from .critique import PID_FOCUSES, issues_for @@ -135,9 +136,24 @@ _LINE_KEYS = { def _num(value, where: str) -> float: + """A finite number, or a ``ValueError`` naming the path. + + ``float("nan")`` used to pass and be placed as an INSERT at ``[nan, 0]``; + the transaction committed and ``build_graph`` then crashed on the drawing + with no path in its message. Only in-process callers can deliver a + non-finite (the MCP pipeline coerces them first), but ``run_spec`` is a + public interface and its docstring promises nothing malformed reaches + the drawing. + """ if isinstance(value, bool) or not isinstance(value, (int, float)): raise ValueError(f"{where} must be a number") - return float(value) + try: + number = float(value) + except OverflowError as exc: # a 400-digit int is a number but not a float + raise ValueError(f"{where} must be a finite number") from exc + if not math.isfinite(number): + raise ValueError(f"{where} must be a finite number") + return number def _ref(value, where: str, ids: set[str]) -> tuple[str, str | None]: @@ -422,7 +438,11 @@ async def run_spec(backend: AutoCADBackend, spec: dict, dry_run: bool = False) - except ValueError as exc: raise ValueError(f"{where}: {exc}") from exc crossings_total += drawn["crossings"] - except Exception: + except BaseException: + # ``Exception`` alone let a client cancellation (``CancelledError`` is + # a ``BaseException``) leave the placed symbols in the drawing inside + # an open transaction — on COM with the undo mark still open, so every + # later ``pid_from_spec`` was refused until a manual rollback. await backend.transaction_rollback() raise await backend.transaction_commit() diff --git a/server.py b/server.py index b0fe247..cec1907 100644 --- a/server.py +++ b/server.py @@ -6837,11 +6837,13 @@ async def pid_from_spec( ) -> dict: """Equipment, valves, instruments and connectors placed, then every line drawn port-to-port, inside one transaction: any bad item rolls the whole - sheet back and the error names it (`lines[2].to`). The response carries the - graph read back from the drawing and the P&ID critique issues, so the - caller sees dangling ends or duplicate tags in the same round trip. - `dry_run` returns the planned vertices and crossings without touching the - drawing. + sheet back and the error names it (`lines[2].to`); so does an interruption + (a client cancellation mid-run leaves no half-drawn sheet and no open + transaction). A non-finite coordinate, rotation, scale or stub is refused + by path before anything is placed. The response carries the graph read + back from the drawing and the P&ID critique issues, so the caller sees + dangling ends or duplicate tags in the same round trip. `dry_run` returns + the planned vertices and crossings without touching the drawing. """ from engineering.pid.spec import run_spec diff --git a/tests/test_pid_from_spec.py b/tests/test_pid_from_spec.py index d0bc871..934dea4 100644 --- a/tests/test_pid_from_spec.py +++ b/tests/test_pid_from_spec.py @@ -85,3 +85,45 @@ async def test_finalize_scores_the_example_at_ninety_or_better(client, tmp_path) ).structured_content assert final["score"]["score"] >= 90, final["score"] assert not [i for i in final.get("critique", []) if str(i.get("focus", "")).startswith("pid_")] + + +# ── Track A hardening (track E wave 0, Task 5) ─────────────────────────────── + + +@pytest.mark.parametrize( + "mutate, fragment", + [ + (lambda s: s["equipment"][0].update({"x": float("nan")}), "equipment[0].x"), + (lambda s: s["valves"][0].update({"y": float("inf")}), "valves[0].y"), + (lambda s: s["instruments"][0].update({"rotation": 10**400}), "instruments[0].rotation"), + (lambda s: s["lines"][0].update({"stub": float("-inf")}), "lines[0].stub"), + ], +) +async def test_non_finite_numbers_are_refused_by_path_before_any_write(backend, mutate, fragment): + before = await backend.entity_count() + spec = copy.deepcopy(EXAMPLE_SPEC) + mutate(spec) + with pytest.raises(ValueError, match="finite") as exc: + await run_spec(backend, spec) + assert fragment in str(exc.value) + assert await backend.entity_count() == before + assert (await backend.system_status())["transaction_depth"] == 0 + + +async def test_a_cancellation_mid_run_rolls_the_sheet_back(backend, monkeypatch): + """``CancelledError`` is a ``BaseException``; ``except Exception`` let the + placed symbols stay inside an open transaction.""" + import asyncio + + from engineering.pid import spec as spec_module + + before = await backend.entity_count() + + async def cancelled(*args, **kwargs): + raise asyncio.CancelledError() + + monkeypatch.setattr(spec_module, "draw_line", cancelled) + with pytest.raises(asyncio.CancelledError): + await run_spec(backend, EXAMPLE_SPEC) + assert await backend.entity_count() == before, "the placed symbols were rolled back" + assert (await backend.system_status())["transaction_depth"] == 0, "no open transaction"