From 3c4fbf6c42d10cd94226884bffb1a4a2bc5aca22 Mon Sep 17 00:00:00 2001 From: Umut CAN Date: Wed, 16 Sep 2026 11:51:22 +0300 Subject: [PATCH] =?UTF-8?q?fix(task-16):=20a=20P&ID=20line=20ending=20on?= =?UTF-8?q?=20an=20unknown=5Fblock=20is=20dangling=20=E2=80=94=20a=200.3?= =?UTF-8?q?=20guess=20suppresses=20no=20finding=20either?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 715c8ef applied the evidence rule in one direction only: an `unknown_block` (any INSERT a plain LINE happened to touch, filed at 0.3) could not *create* a pid_dangling_line, but it could still *suppress* one. build_graph attaches a line end that lands on such a box and invents port `p1` at the line's own end, so `graph["dangling"]` is empty and `_dangling`, which only read that list, called the line connected. A process line from P-101's discharge to the sheet frame (a CTO title block is a block reference) passed drawing_critique and the drawing_finalize gate; the same line stopping 2 mm short of the frame was an error. Spec §11.1 says the end resolves to nothing in both cases — a frame, a logo or a note symbol is nothing the drawing vouches for. `_dangling` now reads two sources with one rule: the graph's own dangling rows (filtered to evidenced edges, as before) plus every end of an evidenced edge whose ref points at an `unknown_block`, at that inferred port's x/y. The `nearest` hint is computed the way the graph computes it for a free end (nearest port within 10·tolerance, same space), over recognised nodes' ports only, so the invented `p1` can never be the suggested fix; `detail["touches"]` names the block the end sits on. The other direction is unchanged: a note leader whose only anchor is an unknown_block is not evidenced and stays out. Tests: the reviewer's repro (landing on the frame → error; graph attaches it, dangling=[]), the stop-short control, the finalize gate raising on it, and the nearest-port hint naming a catalogue valve's port 1 mm away rather than the frame's inferred port. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FKFStjEFzpGyLq5bMpWxTd --- engineering/pid/critique.py | 81 ++++++++++++++++++++++++++----- tests/test_pid_critique.py | 96 +++++++++++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 12 deletions(-) diff --git a/engineering/pid/critique.py b/engineering/pid/critique.py index 424d6ff..1b2a0b7 100644 --- a/engineering/pid/critique.py +++ b/engineering/pid/critique.py @@ -8,16 +8,19 @@ scored here. The checks score evidence, never a guess. ``build_graph(include_foreign=True)`` files *any* INSERT as an ``unknown_block`` at confidence 0.3 the moment a plain LINE's end lands on its bounding box (spec §9.2 step 2) — a bolt with a note -leader qualifies — and gives every foreign block's inferred port a placeholder -``kind: "process"``. Neither is a measurement (spec §9.4: read the confidence -before trusting a foreign graph), so no focus rests on them: an -``unknown_block`` is not P&ID content, is never tagged or scored, and an edge -anchored on nothing better is never called dangling; an inferred port is never -called incompatible. +leader qualifies, and so does the sheet frame — and gives every foreign block's +inferred port a placeholder ``kind: "process"``. Neither is a measurement (spec +§9.4: read the confidence before trusting a foreign graph), so no focus rests +on them in *either* direction: an ``unknown_block`` is not P&ID content, is +never tagged or scored, and an edge anchored on nothing better is never called +dangling — but it is nothing the drawing vouches for either, so a P&ID line by +evidence whose end resolves to one *is* dangling (spec §11.1: the end resolves +to nothing); an inferred port is never called incompatible. """ from __future__ import annotations +import math from typing import TYPE_CHECKING from engineering.plan_spec import Issue @@ -72,12 +75,63 @@ def _evidenced_edges(graph: dict) -> set[str]: return out -def _dangling(graph: dict) -> list[Issue]: - evidenced = _evidenced_edges(graph) - out = [] - for d in graph["dangling"]: - if d["edge"] not in evidenced: +def _nearest_recognised_port(graph: dict, nodes: dict, space, x: float, y: float) -> dict | None: + """The same hint the graph gives a free end (spec §9.2 step 5: the nearest + port within ``10·tolerance``), over ports the drawing vouches for. A + bubble's radial port is matched on its circle, not its centre, so it is + left out exactly as the graph's grid leaves it out.""" + reach = 10.0 * graph["tolerance"] + best = None + for node in nodes.values(): + if not _recognised(node) or node["space"] != space: continue + for pname, port in node["ports"].items(): + if port.get("radius", 0.0) > 0: + continue + d = math.hypot(port["x"] - x, port["y"] - y) + if d <= reach and (best is None or d < best["distance"]): + best = {"node": node["id"], "port": pname, "distance": round(d, 6)} + return best + + +def _dangling_ends(graph: dict) -> list[dict]: + """Free ends of evidenced edges, in the graph's ``dangling`` row shape. + + Two sources, one rule (spec §11.1: the end resolves to nothing). The graph's + own ``dangling`` rows are ends it could attach to nothing at all. An end the + graph attached to an ``unknown_block`` is the other half: that node is a 0.3 + touch guess with a port invented at the line's own end, so a sheet frame, + a logo or a note symbol "connects" whatever line happens to stop on its box. + A 0.3 guess cannot create a finding, and it cannot suppress one either. + """ + evidenced = _evidenced_edges(graph) + nodes = {n["id"]: n for n in graph["nodes"]} + rows = [d for d in graph["dangling"] if d["edge"] in evidenced] + for edge in graph["edges"]: + if edge["id"] not in evidenced: + continue + for end in ("from", "to"): + ref = edge[end] + if not ref or "node" not in ref or _recognised(nodes[ref["node"]]): + continue + port = nodes[ref["node"]]["ports"][ref["port"]] + x, y = port["x"], port["y"] + rows.append( + { + "edge": edge["id"], + "end": end, + "x": x, + "y": y, + "nearest": _nearest_recognised_port(graph, nodes, edge["space"], x, y), + "touches": ref["node"], + } + ) + return rows + + +def _dangling(graph: dict) -> list[Issue]: + out = [] + for d in _dangling_ends(graph): nearest = d.get("nearest") if nearest: hint = ( @@ -87,6 +141,9 @@ def _dangling(graph: dict) -> list[Issue]: ) else: hint = "no port within reach; connect this end with pid_line_draw or delete the line" + detail = {"end": d["end"], "x": d["x"], "y": d["y"], "nearest": nearest, "hint": hint} + if d.get("touches"): + detail["touches"] = d["touches"] out.append( Issue( "error", @@ -94,7 +151,7 @@ def _dangling(graph: dict) -> list[Issue]: f"Line {d['edge']} {d['end']} end at ({d['x']:.2f}, {d['y']:.2f}) " "connects to nothing.", [d["edge"]], - {"end": d["end"], "x": d["x"], "y": d["y"], "nearest": nearest, "hint": hint}, + detail, ) ) return out diff --git a/tests/test_pid_critique.py b/tests/test_pid_critique.py index 84e9650..94e4218 100644 --- a/tests/test_pid_critique.py +++ b/tests/test_pid_critique.py @@ -232,6 +232,102 @@ async def test_a_note_leader_off_an_unknown_block_on_a_pid_is_not_dangling(backe assert len(issues) == 1 and issues[0].detail["x"] == pytest.approx(220.0) +async def _frame_and_pump(backend): + """A sheet frame as a block reference (a CTO title block is one) and a + catalogue pump: the shape that files the frame as an ``unknown_block`` at + 0.3 the moment a P&ID line stops on its border.""" + await backend.drawing_apply_iso_layers("pid") + await backend.block_define( + "FRAME_A3", + [{"type": "polyline", "points": [[0, 0], [420, 0], [420, 297], [0, 297]], "closed": True}], + ) + frame = await backend.block_insert("FRAME_A3", 0, 0, layer="TITLEBLOCK") + pump = await place_symbol(backend, "centrifugal_pump", 100, 100, tag="P-101") + return frame.handle, pump + + +async def test_a_pid_line_ending_on_a_sheet_frame_insert_is_dangling(backend): + """Spec §11.1: an edge end that resolves to nothing is dangling. The graph + attaches the process line to the frame's box and invents port ``p1`` at + the line's own end, so its ``dangling`` list is empty — but a 0.3 touch + guess is nothing the drawing vouches for, and it can no more suppress a + finding than create one. Landing on the frame and stopping 2 mm short of + it must be the same error.""" + frame, pump = await _frame_and_pump(backend) + line = await draw_line( + backend, {"handle": pump["handle"], "port": "discharge"}, {"x": 420, "y": 106} + ) + graph = await build_graph(backend, include_foreign=True) + node = next(n for n in graph["nodes"] if n["id"] == frame) + assert node["kind"] == "unknown_block" and node["ports"]["p1"]["inferred"] is True + edge = next(e for e in graph["edges"] if e["id"] == line["handle"]) + assert edge["to"] == {"node": frame, "port": "p1"} and graph["dangling"] == [] + + issues = await backend.drawing_critique(list(PID_FOCUSES)) + assert [(i.severity, i.focus, i.handles, i.detail["end"]) for i in issues] == [ + ("error", "pid_dangling_line", [line["handle"]], "to") + ] + assert issues[0].detail["x"] == pytest.approx(420.0) + assert issues[0].detail["y"] == pytest.approx(106.0) + assert issues[0].detail["touches"] == frame + assert issues[0].detail["nearest"] is None and "hint" in issues[0].detail + + +async def test_stopping_short_of_the_frame_is_the_same_finding(backend): + _frame, pump = await _frame_and_pump(backend) + line = await draw_line( + backend, {"handle": pump["handle"], "port": "discharge"}, {"x": 418, "y": 106} + ) + issues = await backend.drawing_critique(["pid_dangling_line"]) + assert [(i.severity, i.handles, i.detail["end"]) for i in issues] == [ + ("error", [line["handle"]], "to") + ] + + +async def test_finalize_gate_fails_a_pid_line_that_ends_on_the_frame(backend, tmp_path): + """The gate runs ``focus=None``; a process line "connected" to the title + block must not pass it.""" + import server + + class _Ctx: + def __init__(self, backend): + self.lifespan_context = {"backend": backend} + + async def warning(self, message): + pass + + from fastmcp.exceptions import ToolError + + _frame, pump = await _frame_and_pump(backend) + await draw_line(backend, {"handle": pump["handle"], "port": "discharge"}, {"x": 420, "y": 106}) + with pytest.raises(ToolError, match="pid_dangling_line"): + await server.drawing_finalize(save_path=str(tmp_path / "pid.dxf"), ctx=_Ctx(backend)) + + +async def test_unknown_block_end_hint_names_the_nearest_recognised_port(backend): + """The hint a free end gets from the graph (nearest port within + 10·tolerance) is computed the same way for an end the graph hung on an + ``unknown_block``: over ports the drawing vouches for, never the invented + ``p1``.""" + frame, pump = await _frame_and_pump(backend) + cv = await place_symbol(backend, "globe", 418, 106, tag="FCV-101") + port = cv["ports"]["in"] + await backend.entity_move(frame, port["x"] - 420, 0) + out = pump["ports"]["discharge"] + line = await backend.entity_create_line( + out["x"], out["y"], port["x"], port["y"] + 1.0, layer="PROCESS-PIPING-MAIN" + ) + graph = await build_graph(backend, include_foreign=True) + edge = next(e for e in graph["edges"] if e["id"] == line.handle) + assert edge["to"]["node"] == frame, "the end sits on the frame's border, 1 mm off the port" + issues = await backend.drawing_critique(["pid_dangling_line"]) + assert [i.handles for i in issues] == [[line.handle]] + nearest = issues[0].detail["nearest"] + assert nearest["node"] == cv["handle"] and nearest["port"] == "in" + assert nearest["distance"] == pytest.approx(1.0) + assert f"'{cv['handle']}'" in issues[0].detail["hint"] + + async def test_readme_gear_sheet_is_untouched_by_pid_focuses(backend): """The spec's own negative: all six return [] on the README gear sheet.""" import importlib.util