From 30f886c240cde1152d526dff6eea4c61dfd62c67 Mon Sep 17 00:00:00 2001 From: Umut CAN Date: Thu, 24 Sep 2026 12:12:40 +0300 Subject: [PATCH] fix(task-9): no phantom changes from the tol grid, no handle stage without evidence, a truncated DXF refused Three blocking review findings on drawing_diff. 1. An unedited entity reported as changed with changes == {} and clouded. The signature snaps values to the tol grid, so a value 1 ulp across a half-grid point lands in another bucket. AutoCAD writes 16 significant digits (0.14 + 0.005 = 0.14500000000000002 comes back as 0.145), so a round trip split the pair, which was matched later and counted as changed. A matched pair is now compared by difference (more than half a tol, around the circle for angles) instead of by grid bucket, and a pair with nothing to report counts as unchanged, with no row and no cluster. Measured live on AutoCAD 2026 (Documents.Open read-only, select all, Export DXF, Close(False)): before, summary changed=1, a row with changes {} and one cluster; after, unchanged=21, changed=0, no cluster. Over 12000 computed coordinates (a*0.01 + {0.005, 0.015, 0.105, 0.305}) 795 signatures flip; before, 795 were changed, and after, 0. 2. With zero exact matches, handles were declared stable, so strangers from two fresh files that share handles were paired and given wrong moves. The handle stage now needs at least one exact match. The report adds handle_evidence {exact_matches, kept_handle}. 3. A truncated DXF raised StopIteration from ezdxf's tag reader. asyncio.to_thread cannot hand that exception to an awaiting caller, so drawing_diff hung instead of refusing. read_revision now turns every parse or walk failure (not MemoryError) into ValueError "not a readable DXF (...)". tol is validated first, so its refusal keeps its own name. Tests: grid splits by handle and by position, radius/height/rotation/bulge split at tol=1, a real edit next to a split, no-evidence handles, a truncated file directly and over the MCP wire with a 30 s timeout. The six new tests fail against the previous module. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01FKFStjEFzpGyLq5bMpWxTd --- engineering/understand/diff.py | 83 +++++++++++++++++++------ tests/test_understand_diff.py | 98 ++++++++++++++++++++++++++++++ tests/test_understand_diff_tool.py | 14 +++++ 3 files changed, 175 insertions(+), 20 deletions(-) diff --git a/engineering/understand/diff.py b/engineering/understand/diff.py index 67fca14..da8bef7 100644 --- a/engineering/understand/diff.py +++ b/engineering/understand/diff.py @@ -19,8 +19,12 @@ moment it is matched: 2. **Handle.** A record left over on both sides with the same handle and the same type is the same entity, edited. A save that renumbers handles would make this stage pair strangers, so it runs only while handles look stable: - when at least half of the exact matches kept their handle. The report says - which way it went (``handles_stable``). + when there is at least one exact match and at least half of the exact + matches kept their handle. With no exact match there is no evidence either + way - two fresh files hand out handles from the same seed, and a drawing + moved as a whole matches nothing exactly - so the stage does not run. The + report says which way it went (``handles_stable``) and on what evidence + (``handle_evidence``: the exact matches and how many kept their handle). 3. **Position.** Within one (type, layer, space), a leftover new record is paired with the nearest leftover old record whose anchor (the mean of its points, else the centre of its box) lies within ``move_tol``; the closest @@ -29,8 +33,15 @@ moment it is matched: anchor of both revisions - so one stray entity kilometres away cannot inflate it. -What is left is added (new side) or removed (old side). A matched pair that -is not identical is *changed*, and the row says what changed: ``moved_by`` +What is left is added (new side) or removed (old side). A matched pair is +compared field by field, each number within half a ``tol`` of its old value +counting as equal - the same test a move uses. A pair with nothing to report +is *unchanged*: the signature snaps every value to the ``tol`` grid, so a +value one ulp either side of a half-grid point (AutoCAD writes 0.145 for the +0.14500000000000002 it was given) lands in another bucket, misses the exact +stage and is paired later, yet nothing was edited. Such a pair counts under +``unchanged`` and gets no row and no cloud. Any other matched pair is +*changed*, and the row says what changed: ``moved_by`` (dx, dy) when the geometry is the old geometry translated, else ``geometry`` from -> to; ``text``, ``attribs`` (by tag), ``radius``, ``angles``, ``height``, ``rotation``, ``scale``, ``block``, ``layer``, ``space``, @@ -48,8 +59,8 @@ reference, so a redrawn definition changes every drawn copy while every INSERT record stays identical. The 2 % move tolerance, the 1st/99th percentiles, the half-of-the-matches -handle rule and the cluster padding are this module's declared defaults, not -values from a standard. +(and at least one) handle rule, the half-a-``tol`` equality and the cluster +padding are this module's declared defaults, not values from a standard. """ from __future__ import annotations @@ -248,11 +259,21 @@ def _nearest_pairs( def _number_changed(a, b, tol: float, *, modulo: float | None = None) -> bool: + """More than half a ``tol`` apart (around the circle when ``modulo``). + + A difference, never two grid buckets: ``_q`` puts 0.14500000000000002 + and 0.145 in different buckets at ``tol=0.01``. + """ if a is None or b is None: return (a is None) != (b is None) + a, b = float(a), float(b) + if not (math.isfinite(a) and math.isfinite(b)): + return repr(a) != repr(b) + difference = abs(a - b) if modulo is not None: - a, b = a % modulo, b % modulo - return _q(a, tol) != _q(b, tol) + difference %= modulo + difference = min(difference, modulo - difference) + return difference > tol / 2.0 def _movement(o: EntityRecord, n: EntityRecord, tol: float) -> dict: @@ -302,7 +323,9 @@ def _changes(o: EntityRecord, n: EntityRecord, tol: float) -> dict: if o.space != n.space: out["space"] = {"from": o.space, "to": n.space} out.update(_movement(o, n, tol)) - if tuple(_q(b, tol) for b in o.bulges) != tuple(_q(b, tol) for b in n.bulges): + if len(o.bulges) != len(n.bulges) or any( + _number_changed(a, b, tol) for a, b in zip(o.bulges, n.bulges, strict=False) + ): out["bulges"] = {"from": list(o.bulges), "to": list(n.bulges)} if bool(o.closed) != bool(n.closed): out["closed"] = {"from": bool(o.closed), "to": bool(n.closed)} @@ -441,7 +464,9 @@ def diff_snapshots( exact += 1 kept_handle += int(olds[i].handle == rec.handle) left_old = [i for i in range(len(olds)) if i not in used_old] - handles_stable = exact == 0 or 2 * kept_handle >= exact + # No exact match is no evidence: strangers from two fresh files share + # handles, and so do the entities of a drawing that moved as a whole. + handles_stable = exact > 0 and 2 * kept_handle >= exact pairs: list[tuple[int, int, str]] = [] # 2. handle @@ -468,10 +493,16 @@ def diff_snapshots( changed: list[dict] = [] items: list[tuple[str, Box, int]] = [] - for key, (i, j, how) in enumerate( - sorted(pairs, key=lambda p: (olds[p[0]].handle, news[p[1]].handle)) - ): + same = 0 + for i, j, how in sorted(pairs, key=lambda p: (olds[p[0]].handle, news[p[1]].handle)): o, n = olds[i], news[j] + changes = _changes(o, n, tol) + if not changes: + # the signature grid split an unedited pair (a value 1 ulp across a + # half-grid point): unchanged, no row and no cloud + same += 1 + continue + key = len(changed) at = anchor(n) changed.append( { @@ -482,7 +513,7 @@ def diff_snapshots( "space": n.space, "matched_by": how, "at": None if at is None else [at[0], at[1]], - "changes": _changes(o, n, tol), + "changes": changes, } ) for rec in (o, n): @@ -504,10 +535,11 @@ def diff_snapshots( "tol": tol, "move_tol": move_tol, "handles_stable": handles_stable, + "handle_evidence": {"exact_matches": exact, "kept_handle": kept_handle}, "summary": { "old_entities": len(olds), "new_entities": len(news), - "unchanged": exact, + "unchanged": exact + same, "changed": len(changed), "added": len(added), "removed": len(removed), @@ -623,11 +655,16 @@ def read_revision(path: str, *, tol: float = DEFAULT_TOL) -> tuple[Snapshot, dic """One read of a DXF revision: its records and its block signatures. Refused before reading: a file that cannot be stat'ed and one over - ``MAX_DXF_BYTES`` (the size and the variable named); a file ezdxf cannot - parse is refused with ezdxf's reason. + ``MAX_DXF_BYTES`` (the size and the variable named). A file ezdxf cannot + parse or walk is refused as ``not a readable DXF`` with the reason - + whatever ezdxf raised: a truncated file (an interrupted save, a partial + copy) raises ``StopIteration`` from inside its tag reader, which + ``asyncio.to_thread`` cannot hand back to an awaiting caller, so letting it + through would hang ``drawing_diff`` instead of refusing. """ import ezdxf + tol = _positive("tol", tol) limit = int(config.settings.max_dxf_bytes) try: size = os.path.getsize(path) @@ -640,6 +677,12 @@ def read_revision(path: str, *, tol: float = DEFAULT_TOL) -> tuple[Snapshot, dic ) try: doc = ezdxf.readfile(path) - except (OSError, ezdxf.DXFError) as exc: - raise ValueError(f"{path}: not a readable DXF ({exc})") from exc - return records_from_doc(doc, source=str(path)), block_signatures(doc, tol=tol) + return records_from_doc(doc, source=str(path)), block_signatures(doc, tol=tol) + except MemoryError: + raise + except Exception as exc: # noqa: BLE001 - every parse failure is one refusal + if isinstance(exc, StopIteration): + reason = "the file ends early - an interrupted save or a partial copy?" + else: + reason = str(exc) or type(exc).__name__ + raise ValueError(f"{path}: not a readable DXF ({reason})") from exc diff --git a/tests/test_understand_diff.py b/tests/test_understand_diff.py index 26d185c..cfb0d1d 100644 --- a/tests/test_understand_diff.py +++ b/tests/test_understand_diff.py @@ -146,6 +146,29 @@ def test_renumbered_handles_never_pair_strangers_that_share_a_handle(): assert [r["at"] for r in result["added"]] == [[5000.0, 5000.0]] +def test_no_exact_match_is_no_evidence_that_handles_are_stable(): + # two fresh files hand out handles from one seed; every circle moved +1000 in x + # and the new file added them in reverse order, so equal handles are strangers + old = snap( + *(rec(f"{0x20 + i:X}", "CIRCLE", "EQ", ((0, 200 * i),), radius=10.0) for i in range(8)) + ) + new = snap( + *( + rec(f"{0x20 + k:X}", "CIRCLE", "EQ", ((1000, 200 * i),), radius=10.0) + for k, i in enumerate(reversed(range(8))) + ) + ) + result = diff_snapshots(old, new) + assert result["handles_stable"] is False + assert result["handle_evidence"] == {"exact_matches": 0, "kept_handle": 0} + assert result["changed"] == [] + assert (result["summary"]["added"], result["summary"]["removed"]) == (8, 8) + # given a move tolerance that reaches, the position stage finds the true move + moved = diff_snapshots(old, new, move_tol=1001.0) + assert {row["matched_by"] for row in moved["changed"]} == {"position"} + assert {tuple(row["changes"]["moved_by"]) for row in moved["changed"]} == {(1000.0, 0.0)} + + def test_beyond_move_tol_a_line_is_removed_and_another_added(): old = snap(*STILL, line("D", 0, 200, 100, 200)) new = snap(*STILL, line("E", 0, 700, 100, 700)) @@ -154,6 +177,65 @@ def test_beyond_move_tol_a_line_is_removed_and_another_added(): assert result["changed"] == [] +# -- a value split by the signature grid ---------------------------------------------- + + +def test_a_value_one_ulp_across_a_half_grid_point_is_unchanged(): + # AutoCAD writes 16 significant digits: 0.14 + 0.005 = 0.14500000000000002 comes + # back as 0.145, and at tol=0.01 the two snap to buckets 15 and 14 + assert 0.14 + 0.005 != 0.145 + edge = 0.14 + 0.005 + assert signature(line("L", edge, 300, 50, 300)) != signature(line("L", 0.145, 300, 50, 300)) + old = snap(*STILL, line("L", edge, 300, 50, 300)) + for new in ( + snap(*STILL, line("L", 0.145, 300, 50, 300)), # paired by handle + snap(*RENUMBERED, line("1L", 0.145, 300, 50, 300)), # paired by position + ): + result = diff_snapshots(old, new, move_tol=10.0) + assert result["summary"]["unchanged"] == 4 + assert (result["summary"]["changed"], result["summary"]["added"]) == (0, 0) + assert result["summary"]["removed"] == 0 + assert result["changed"] == [] and result["clusters"] == [] + assert result["by_layer"] == {} and result["by_type"] == {} + + +def test_numbers_one_ulp_across_a_grid_boundary_are_not_changes(): + # at tol=1, 12.500000000000002 and 12.5 snap to 13 and 12 + edge = 12.500000000000002 + old = snap( + *STILL, + rec("R", "CIRCLE", "PIPE", ((edge, 0),), radius=edge), + rec("T", "TEXT", "NOTES", ((10, 10),), text="X", height=edge, rotation=edge), + rec("P", "LWPOLYLINE", "PIPE", ((0, 0), (10, 0)), bulges=(edge, 0.0)), + ) + new = snap( + *STILL, + rec("R", "CIRCLE", "PIPE", ((12.5, 0),), radius=12.5), + rec("T", "TEXT", "NOTES", ((10, 10),), text="X", height=12.5, rotation=12.5), + rec("P", "LWPOLYLINE", "PIPE", ((0, 0), (10, 0)), bulges=(12.5, 0.0)), + ) + result = diff_snapshots(old, new, tol=1.0) + assert result["summary"]["unchanged"] == 6 + assert result["changed"] == [] and result["clusters"] == [] + + +def test_a_real_edit_next_to_a_grid_split_is_still_reported(): + old = snap( + *STILL, + line("L", 0.14 + 0.005, 300, 50, 300), + rec("R", "CIRCLE", "PIPE", ((0, 0),), radius=10.0), + ) + new = snap( + *STILL, line("L", 0.145, 300, 50, 300), rec("R", "CIRCLE", "PIPE", ((0, 0),), radius=10.2) + ) + result = diff_snapshots(old, new) + (row,) = result["changed"] + assert row["old_handle"] == "R" + assert row["changes"] == {"radius": {"from": 10.0, "to": 10.2}} + assert result["summary"]["unchanged"] == 4 + assert [c["count"] for c in result["clusters"]] == [1] + + # -- the move tolerance ---------------------------------------------------------------- @@ -260,6 +342,22 @@ def test_read_revision_refuses_a_file_that_is_not_a_dxf(tmp_path): read_revision(str(path)) +def test_read_revision_refuses_a_truncated_dxf(tmp_path): + # an interrupted save: ezdxf's tag reader runs out and raises StopIteration, + # which must surface as the refusal, not escape (it would hang asyncio.to_thread) + path = tmp_path / "half.dxf" + path.write_bytes(b" 0\nSECTION\n 2\nHEADER\n 9\n$ACADVER\n 1\nAC1015\n") + with pytest.raises(ValueError, match=r"half\.dxf: not a readable DXF \(the file ends early"): + read_revision(str(path)) + + +def test_read_revision_refuses_a_non_positive_tol_by_name(tmp_path): + path = tmp_path / "empty.dxf" + ezdxf.new().saveas(path) + with pytest.raises(ValueError, match=r"^tol must be a positive number"): + read_revision(str(path), tol=0) + + # -- the known edits on the synthetic plant -------------------------------------------- diff --git a/tests/test_understand_diff_tool.py b/tests/test_understand_diff_tool.py index 2f718c9..e7c5eda 100644 --- a/tests/test_understand_diff_tool.py +++ b/tests/test_understand_diff_tool.py @@ -125,6 +125,20 @@ async def test_a_revision_that_is_neither_dxf_nor_dwg_is_refused_by_parameter(cl assert "old_path:" in text and "reads a .dxf or a .dwg" in text +async def test_a_truncated_revision_is_refused_not_hung(client, tmp_path): + import asyncio + + half = tmp_path / "half.dxf" + half.write_bytes(b" 0\nSECTION\n 2\nHEADER\n 9\n$ACADVER\n 1\nAC1015\n") + result = await asyncio.wait_for( + client.call_tool("drawing_diff", {"old_path": str(half)}, raise_on_error=False), + timeout=30, + ) + assert result.is_error is True + text = result.content[0].text + assert "old_path:" in text and "not a readable DXF" in text + + async def test_the_diff_is_advertised_as_non_destructive(client): tools = {tool.name: tool for tool in await client.list_tools()} assert tools["drawing_diff"].annotations.destructiveHint is False