merge: track-a task 2

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FKFStjEFzpGyLq5bMpWxTd

backends/block_specs.py

@@ -0,0 +1,213 @@
+# backends/block_specs.py
+"""Typed primitive specs for ``block_define``, validated before any write.
+
+Both engines consume the same normalised specs, so a malformed request is
+refused identically on COM and ezdxf and nothing is written on either. Every
+error names the offending entry (``entities[i]`` / ``attdefs[i]``) and key.
+"""
+
+from __future__ import annotations
+
+import math
+import re
+
+from security import illegal_symbol_name_chars
+
+ALLOWED_TYPES = ("line", "circle", "arc", "polyline", "text", "solid")
+TEXT_ALIGNMENTS = ("left", "center", "right", "middle_center")
+#: Checked with ``fullmatch`` — ``$`` alone would accept a trailing newline, which
+#: ezdxf then strips on write, turning two "distinct" tags into one duplicate.
+ATTDEF_TAG_RE = re.compile(r"^[A-Z][A-Z0-9_]{0,30}$")
+
+_REQUIRED = {
+    "line": ("x1", "y1", "x2", "y2"),
+    "circle": ("cx", "cy", "r"),
+    "arc": ("cx", "cy", "r", "start_deg", "end_deg"),
+    "polyline": ("points",),
+    "text": ("text", "x", "y", "height"),
+    "solid": ("points",),
+}
+_POSITIVE = {"r", "height"}
+
+
+def _scalar(value, where: str, *, positive: bool = False) -> float:
+    if isinstance(value, bool) or not isinstance(value, (int, float)):
+        raise TypeError(f"{where} must be a number, got {value!r}")
+    number = float(value)
+    if not math.isfinite(number):
+        raise TypeError(f"{where} must be finite")
+    if positive and number <= 0:
+        raise TypeError(f"{where} must be > 0, got {number}")
+    return number
+
+
+def _number(spec: dict, key: str, where: str) -> float:
+    return _scalar(spec.get(key), f"{where}: {key!r}", positive=key in _POSITIVE)
+
+
+def _points(
+    spec: dict, where: str, *, minimum: int, maximum: int | None = None
+) -> list[tuple[float, float]]:
+    raw = spec.get("points")
+    if not isinstance(raw, (list, tuple)) or len(raw) < minimum:
+        raise TypeError(f"{where}: 'points' must be a list of at least {minimum} [x, y] pairs")
+    if maximum is not None and len(raw) > maximum:
+        raise TypeError(f"{where}: 'points' must have at most {maximum} pairs")
+    out: list[tuple[float, float]] = []
+    for i, pt in enumerate(raw):
+        if not isinstance(pt, (list, tuple)) or len(pt) != 2:
+            raise TypeError(f"{where}: points[{i}] must be an [x, y] pair")
+        out.append(
+            (
+                _scalar(pt[0], f"{where}: points[{i}].x"),
+                _scalar(pt[1], f"{where}: points[{i}].y"),
+            )
+        )
+    return out
+
+
+def _align(spec: dict, where: str) -> str:
+    align = spec.get("align", "left")
+    if align not in TEXT_ALIGNMENTS:
+        raise TypeError(f"{where}: 'align' must be one of {TEXT_ALIGNMENTS}, got {align!r}")
+    return align
+
+
+def _layer(spec: dict, where: str) -> str:
+    layer = spec.get("layer", "0")
+    if not isinstance(layer, str) or not layer.strip():
+        raise TypeError(f"{where}: 'layer' must be a non-empty string")
+    layer = layer.strip()
+    # Gate the DXF name rule here rather than letting ezdxf raise DXFValueError
+    # on `add_*(dxfattribs={"layer": ...})` mid-loop, after the block exists and
+    # earlier primitives are already written (and, with overwrite, the old
+    # definition is already gone).
+    offenders = illegal_symbol_name_chars(layer)
+    if offenders:
+        shown = ", ".join(repr(c) for c in offenders)
+        raise TypeError(f"{where}: 'layer' is not a legal DXF layer name (contains {shown})")
+    return layer
+
+
+def _one_line(value, where: str) -> str:
+    """A single-line DXF string (TEXT content, ATTDEF prompt/default).
+
+    ezdxf's DXFAttr fixer (``fix_one_line_text``) silently strips line breaks
+    and a trailing ``^`` on write, logged at DEBUG only, so the declared value
+    and the value read back would differ; COM writes the raw string, so the two
+    engines would also diverge. Refuse it here, before any write, using ezdxf's
+    own ``is_valid_one_line_text`` rule plus the other C0 controls DXF text
+    cannot carry.
+    """
+    if not isinstance(value, str):
+        raise TypeError(f"{where} must be a string")
+    controls = sorted({c for c in value if ord(c) < 32})
+    if controls:
+        shown = ", ".join(repr(c) for c in controls)
+        raise TypeError(
+            f"{where} must be a single line without control characters (contains {shown})"
+        )
+    if value.endswith("^"):
+        raise TypeError(f"{where} must not end with '^' (DXF strips a trailing caret)")
+    return value
+
+
+def validate_entity_specs(entities) -> list[dict]:
+    """Normalise ``block_define`` primitive specs; refuse the whole list on the first defect."""
+    if not isinstance(entities, (list, tuple)) or not entities:
+        raise ValueError(
+            "block_define: 'entities' is empty — a block that draws nothing is refused"
+        )
+    out: list[dict] = []
+    for i, spec in enumerate(entities):
+        where = f"entities[{i}]"
+        if not isinstance(spec, dict):
+            raise TypeError(f"{where}: must be an object")
+        kind = spec.get("type")
+        if kind not in ALLOWED_TYPES:
+            raise TypeError(f"{where}: 'type' must be one of {ALLOWED_TYPES}, got {kind!r}")
+        for key in _REQUIRED[kind]:
+            if key not in spec:
+                raise TypeError(f"{where}: {kind} requires {key!r}")
+        norm: dict = {"type": kind, "layer": _layer(spec, where)}
+        if kind == "line":
+            for key in ("x1", "y1", "x2", "y2"):
+                norm[key] = _number(spec, key, where)
+        elif kind == "circle":
+            for key in ("cx", "cy", "r"):
+                norm[key] = _number(spec, key, where)
+        elif kind == "arc":
+            for key in ("cx", "cy", "r", "start_deg", "end_deg"):
+                norm[key] = _number(spec, key, where)
+        elif kind == "polyline":
+            pts = _points(spec, where, minimum=2)
+            closed = spec.get("closed", False)
+            if not isinstance(closed, bool):
+                raise TypeError(f"{where}: 'closed' must be a boolean")
+            bulges = spec.get("bulges")
+            if bulges is not None:
+                if not isinstance(bulges, (list, tuple)) or len(bulges) != len(pts):
+                    raise TypeError(f"{where}: 'bulges' must list one value per point")
+                bulges = [_scalar(b, f"{where}: bulges[{j}]") for j, b in enumerate(bulges)]
+            norm.update(points=pts, closed=closed, bulges=list(bulges) if bulges else None)
+        elif kind == "text":
+            norm.update(
+                text=_one_line(spec.get("text"), f"{where}: 'text'"),
+                x=_number(spec, "x", where),
+                y=_number(spec, "y", where),
+                height=_number(spec, "height", where),
+                rotation_deg=_scalar(spec.get("rotation_deg", 0.0), f"{where}: 'rotation_deg'"),
+                align=_align(spec, where),
+            )
+        elif kind == "solid":
+            norm["points"] = _points(spec, where, minimum=3, maximum=4)
+        out.append(norm)
+    return out
+
+
+def validate_attdef_specs(attdefs) -> list[dict]:
+    """Normalise ATTDEF specs: tag pattern, uniqueness, numeric fields, flags."""
+    if attdefs is None:
+        return []
+    if not isinstance(attdefs, (list, tuple)):
+        raise TypeError("block_define: 'attdefs' must be a list")
+    out: list[dict] = []
+    seen: set[str] = set()
+    for i, spec in enumerate(attdefs):
+        where = f"attdefs[{i}]"
+        if not isinstance(spec, dict):
+            raise TypeError(f"{where}: must be an object")
+        tag = spec.get("tag")
+        if not isinstance(tag, str) or not ATTDEF_TAG_RE.fullmatch(tag):
+            raise TypeError(f"{where}: 'tag' must match {ATTDEF_TAG_RE.pattern}, got {tag!r}")
+        if tag in seen:
+            raise TypeError(f"{where}: duplicate tag {tag!r}")
+        seen.add(tag)
+        prompt = _one_line(spec.get("prompt", tag), f"{where}: 'prompt'")
+        default = _one_line(spec.get("default", ""), f"{where}: 'default'")
+        invisible = spec.get("invisible", False)
+        if not isinstance(invisible, bool):
+            raise TypeError(f"{where}: 'invisible' must be a boolean")
+        out.append(
+            {
+                "tag": tag,
+                "prompt": prompt,
+                "default": default,
+                "x": _number(spec, "x", where),
+                "y": _number(spec, "y", where),
+                "height": _number(spec, "height", where),
+                "rotation_deg": _scalar(spec.get("rotation_deg", 0.0), f"{where}: 'rotation_deg'"),
+                "align": _align(spec, where),
+                "invisible": invisible,
+            }
+        )
+    return out
+
+
+def solid_vertices(points: list[tuple[float, float]]) -> list[tuple[float, float]]:
+    """DXF SOLID draws vtx0→vtx1→vtx3→vtx2, so a quad given in polygon order
+    (a, b, c, d) is stored as (a, b, d, c). Triangles are stored as given."""
+    pts = [(float(x), float(y)) for x, y in points]
+    if len(pts) == 4:
+        return [pts[0], pts[1], pts[3], pts[2]]
+    return pts

security.py

@@ -384,6 +384,17 @@ _FORBIDDEN_SYMBOL_CHARS = set('<>/\\":;?*|,=`') | set("\n\r\t")
 _MAX_SYMBOL_NAME = 255
 
 
+def illegal_symbol_name_chars(name: str) -> list[str]:
+    """The characters in `name` that DXF forbids in a symbol-table name, sorted.
+
+    The one rule behind every symbol-name gate in this repo: `sanitize_symbol_name`
+    refuses on it for macro interpolation, and pre-write validators (block
+    primitive specs, for one) refuse on it so ezdxf's own `DXFValueError` never
+    fires mid-write after earlier entities have already landed.
+    """
+    return sorted({c for c in name if c in _FORBIDDEN_SYMBOL_CHARS or ord(c) < 32})
+
+
 def sanitize_symbol_name(name: str, *, kind: str = "symbol") -> str:
     """Return `name` if it is a legal DXF symbol-table name, else refuse.
 
@@ -397,7 +408,7 @@ def sanitize_symbol_name(name: str, *, kind: str = "symbol") -> str:
         raise ToolError(f"{kind} name rejected: empty.")
     if len(candidate) > _MAX_SYMBOL_NAME:
         raise ToolError(f"{kind} name rejected: longer than {_MAX_SYMBOL_NAME} characters.")
-    offenders = sorted({c for c in candidate if c in _FORBIDDEN_SYMBOL_CHARS or ord(c) < 32})
+    offenders = illegal_symbol_name_chars(candidate)
     if offenders:
         shown = ", ".join(repr(c) for c in offenders)
         raise ToolError(

tests/test_block_specs.py

@@ -0,0 +1,212 @@
+# tests/test_block_specs.py
+"""block_define request validation: refuse the whole request before any write."""
+
+from __future__ import annotations
+
+import pytest
+
+from backends.block_specs import (
+    ALLOWED_TYPES,
+    solid_vertices,
+    validate_attdef_specs,
+    validate_entity_specs,
+)
+
+
+def test_allowed_types_are_the_documented_six():
+    assert ALLOWED_TYPES == ("line", "circle", "arc", "polyline", "text", "solid")
+
+
+def test_empty_entities_is_refused():
+    with pytest.raises(ValueError, match="empty"):
+        validate_entity_specs([])
+
+
+def test_each_type_normalises_to_floats_and_layer_zero():
+    out = validate_entity_specs(
+        [
+            {"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0},
+            {"type": "circle", "cx": 0, "cy": 0, "r": 2},
+            {"type": "arc", "cx": 0, "cy": 0, "r": 2, "start_deg": 0, "end_deg": 180},
+            {"type": "polyline", "points": [[0, 0], [1, 0], [1, 1]], "closed": True},
+            {"type": "text", "text": "M", "x": 0, "y": 0, "height": 2},
+            {"type": "solid", "points": [[0, 0], [1, 0], [0, 1]]},
+        ]
+    )
+    assert [e["type"] for e in out] == list(ALLOWED_TYPES)
+    assert out[0] == {"type": "line", "layer": "0", "x1": 0.0, "y1": 0.0, "x2": 1.0, "y2": 0.0}
+    assert out[3]["points"] == [(0.0, 0.0), (1.0, 0.0), (1.0, 1.0)]
+    assert out[3]["closed"] is True and out[3]["bulges"] is None
+    assert out[4]["align"] == "left" and out[4]["rotation_deg"] == 0.0
+    assert out[5]["points"] == [(0.0, 0.0), (1.0, 0.0), (0.0, 1.0)]
+
+
+@pytest.mark.parametrize(
+    "bad, fragment",
+    [
+        ({"type": "square"}, "type"),
+        ({"type": "line", "x1": 0, "y1": 0, "x2": 1}, "y2"),
+        ({"type": "circle", "cx": 0, "cy": 0, "r": 0}, "r"),
+        ({"type": "circle", "cx": 0, "cy": 0, "r": float("nan")}, "finite"),
+        ({"type": "text", "text": "M", "x": 0, "y": 0, "height": -1}, "height"),
+        ({"type": "text", "text": 5, "x": 0, "y": 0, "height": 1}, "text"),
+        ({"type": "text", "text": "A\nB", "x": 0, "y": 0, "height": 1}, "'text'"),
+        ({"type": "text", "text": "A\rB", "x": 0, "y": 0, "height": 1}, "'text'"),
+        ({"type": "text", "text": "A\x01B", "x": 0, "y": 0, "height": 1}, "'text'"),
+        ({"type": "text", "text": "10^", "x": 0, "y": 0, "height": 1}, "'text'"),
+        ({"type": "text", "text": "M", "x": 0, "y": 0, "height": 1, "align": "top"}, "align"),
+        ({"type": "polyline", "points": [[0, 0]]}, "points"),
+        ({"type": "polyline", "points": [[0, 0], [1]]}, "points[1]"),
+        ({"type": "polyline", "points": [[0, 0], [1, 1]], "bulges": [0.5]}, "bulges"),
+        ({"type": "polyline", "points": [[0, 0], [1, 1]], "closed": "yes"}, "closed"),
+        ({"type": "solid", "points": [[0, 0], [1, 0]]}, "points"),
+        ({"type": "solid", "points": [[0, 0], [1, 0], [1, 1], [0, 1], [2, 2]]}, "at most 4"),
+        ({"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": ""}, "layer"),
+        ({"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": "BAD<LAYER"}, "layer"),
+        ({"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": "A;B"}, "layer"),
+        ({"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": "A\x01B"}, "layer"),
+        ({"type": "line", "x1": True, "y1": 0, "x2": 1, "y2": 0}, "x1"),
+    ],
+)
+def test_bad_entity_names_the_index_and_key(bad, fragment):
+    good = {"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0}
+    with pytest.raises(TypeError, match=r"entities\[1\]") as exc:
+        validate_entity_specs([good, bad])
+    assert fragment in str(exc.value)
+
+
+def test_illegal_layer_name_is_refused_before_any_write():
+    """ezdxf would raise DXFValueError on this layer mid-loop, after earlier
+    primitives (and, with overwrite, the old definition) were already written.
+    The gate has to be ours, typed, and indexed."""
+    with pytest.raises(TypeError, match=r"entities\[0\].*'layer'") as exc:
+        validate_entity_specs(
+            [{"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": "BAD<LAYER"}]
+        )
+    assert "'<'" in str(exc.value)
+    # Legal names, including the ones with spaces and dots, still pass.
+    out = validate_entity_specs(
+        [{"type": "line", "x1": 0, "y1": 0, "x2": 1, "y2": 0, "layer": " PID.Valves-1 "}]
+    )
+    assert out[0]["layer"] == "PID.Valves-1"
+
+
+def test_attdefs_normalise_and_default():
+    out = validate_attdef_specs([{"tag": "TAG", "x": 0, "y": 3, "height": 2.5}])
+    assert out == [
+        {
+            "tag": "TAG",
+            "prompt": "TAG",
+            "default": "",
+            "x": 0.0,
+            "y": 3.0,
+            "height": 2.5,
+            "rotation_deg": 0.0,
+            "align": "left",
+            "invisible": False,
+        }
+    ]
+
+
+@pytest.mark.parametrize(
+    "bad, fragment",
+    [
+        ({"tag": "tag", "x": 0, "y": 0, "height": 1}, "tag"),
+        ({"tag": "T-1", "x": 0, "y": 0, "height": 1}, "tag"),
+        ({"tag": "TAG\n", "x": 0, "y": 0, "height": 1}, "tag"),
+        ({"tag": "TAG", "x": 0, "y": 0, "height": 0}, "height"),
+        ({"tag": "TAG", "x": 0, "y": 0, "height": 1, "invisible": 1}, "invisible"),
+        ({"tag": "TAG", "x": 0, "y": 0, "height": 1, "default": 3}, "default"),
+        ({"tag": "TAG", "x": 0, "y": 0, "height": 1, "prompt": 3}, "prompt"),
+        ({"tag": "TAG", "x": 0, "y": 0, "height": 1, "prompt": "P\nQ"}, "'prompt'"),
+        ({"tag": "TAG", "x": 0, "y": 0, "height": 1, "default": "L-1\r\n"}, "'default'"),
+        ({"tag": "TAG", "x": 0, "y": 0, "height": 1, "default": "L-1^"}, "'default'"),
+        ({"tag": "TAG", "x": 0, "y": 0, "height": 1, "default": "L\x1f1"}, "'default'"),
+    ],
+)
+def test_bad_attdef_names_the_index_and_key(bad, fragment):
+    with pytest.raises(TypeError, match=r"attdefs\[0\]") as exc:
+        validate_attdef_specs([bad])
+    assert fragment in str(exc.value)
+
+
+def test_duplicate_attdef_tags_are_refused():
+    with pytest.raises(TypeError, match="duplicate"):
+        validate_attdef_specs(
+            [
+                {"tag": "TAG", "x": 0, "y": 0, "height": 1},
+                {"tag": "TAG", "x": 0, "y": 5, "height": 1},
+            ]
+        )
+
+
+def test_trailing_newline_cannot_smuggle_a_duplicate_tag():
+    """`$` matches before a trailing newline; ezdxf strips it on write, so
+    'TAG\n' + 'TAG' would land as two ATTDEFs with the same tag and the
+    attribute readers (dict keyed by tag) would silently drop one."""
+    with pytest.raises(TypeError, match=r"attdefs\[0\].*'tag'"):
+        validate_attdef_specs(
+            [
+                {"tag": "TAG\n", "x": 0, "y": 0, "height": 1},
+                {"tag": "TAG", "x": 0, "y": 0, "height": 1},
+            ]
+        )
+
+
+@pytest.mark.parametrize("value", ["A\nB", "A\rB", "10^", "A\tB", "L-1\r\n"])
+def test_one_line_strings_are_refused_before_ezdxf_would_rewrite_them(value):
+    """ezdxf's `fix_one_line_text` strips line breaks and a trailing caret on
+    `add_text` / `add_attdef` (DEBUG log only), so 'A\\nB' lands as 'AB' and the
+    ATTRIB value the P&ID reader gets back differs from the declared one; COM
+    writes the raw string, so the engines would diverge too. The gate is ours,
+    typed, and indexed — the same hole the tag check closed, on its siblings."""
+    from ezdxf.lldxf.validator import is_valid_one_line_text
+
+    # Either ezdxf's own rule rejects it or it is a C0 control DXF text cannot carry.
+    assert not is_valid_one_line_text(value) or any(ord(c) < 32 for c in value)
+    with pytest.raises(TypeError, match=r"entities\[0\]: 'text'"):
+        validate_entity_specs([{"type": "text", "text": value, "x": 0, "y": 0, "height": 2}])
+    with pytest.raises(TypeError, match=r"attdefs\[0\]: 'prompt'"):
+        validate_attdef_specs([{"tag": "TAG", "x": 0, "y": 0, "height": 2, "prompt": value}])
+    with pytest.raises(TypeError, match=r"attdefs\[0\]: 'default'"):
+        validate_attdef_specs([{"tag": "TAG", "x": 0, "y": 0, "height": 2, "default": value}])
+
+
+@pytest.mark.parametrize("value", ["10^2", "Ø20 H7", "%%C20", "L-101", ""])
+def test_one_line_strings_that_ezdxf_keeps_verbatim_pass(value):
+    """A caret inside the text (`10^2`), unicode and `%%C` survive both engines."""
+    from ezdxf.lldxf.validator import is_valid_one_line_text
+
+    assert is_valid_one_line_text(value)
+    out = validate_entity_specs([{"type": "text", "text": value, "x": 0, "y": 0, "height": 2}])
+    assert out[0]["text"] == value
+    att = validate_attdef_specs(
+        [{"tag": "TAG", "x": 0, "y": 0, "height": 2, "prompt": value, "default": value}]
+    )
+    assert att[0]["prompt"] == value and att[0]["default"] == value
+
+
+def test_one_line_gate_writes_through_ezdxf_unchanged():
+    """Round-trip proof: what the gate accepts, ezdxf stores verbatim (no
+    DXFAttr fixer rewrite), so declared == read back."""
+    import ezdxf
+
+    doc = ezdxf.new()
+    blk = doc.blocks.new("T")
+    [spec] = validate_entity_specs(
+        [{"type": "text", "text": "10^2 Ø", "x": 0, "y": 0, "height": 2}]
+    )
+    [att] = validate_attdef_specs(
+        [{"tag": "TAG", "x": 0, "y": 0, "height": 2, "prompt": "P Q", "default": "L-101"}]
+    )
+    t = blk.add_text(spec["text"])
+    a = blk.add_attdef(att["tag"], text=att["default"], dxfattribs={"prompt": att["prompt"]})
+    assert t.dxf.text == "10^2 Ø"
+    assert a.dxf.text == "L-101" and a.dxf.prompt == "P Q"
+
+
+def test_solid_vertices_use_dxf_bowtie_order():
+    quad = [(0.0, 0.0), (1.0, 0.0), (1.0, 1.0), (0.0, 1.0)]
+    assert solid_vertices(quad) == [(0.0, 0.0), (1.0, 0.0), (0.0, 1.0), (1.0, 1.0)]
+    tri = [(0.0, 0.0), (1.0, 0.0), (0.0, 1.0)]
+    assert solid_vertices(tri) == tri