fix(understand): drawing_diff minors from the review — direction, repeated tags, bounded output

The review of Task 9 left four minor findings. Three change what a user reads:

- A LINE drawn back to front and moved was reported as a geometry change.
  The signature already ignores a LINE's direction, so its movement now
  does too: the reversed order is tried before the entity is called
  reshaped.
- An INSERT carrying one attribute tag twice was read as a dict. An edit
  of the first value could disappear, and since the pair then had nothing
  to report, it counted as unchanged. A repeated tag is now compared as
  the list of its values; a tag carried once keeps plain from / to.
- cap_report capped the three lists but not the clusters. A widespread
  edit of a large drawing could return an unbounded cluster list. It is
  capped like the others, with truncated["clusters"]; the markup still
  clouds every cluster.

The fourth bounds work: when the whole drawing moves, every entity reaches
the position stage, and a generous move_tol gave each one hundreds of
candidates, which is n x m memory. Only the 32 nearest per entity are kept
now. A 200-line test shows each line still pairs with its own moved copy.

The first three tests failed against the previous module (the attribute
test with no changed row at all). The whole diff suite passes: 38 tests.

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

engineering/understand/diff.py

@@ -66,6 +66,7 @@ padding are this module's declared defaults, not values from a standard.
 from __future__ import annotations
 
 import hashlib
+import heapq
 import math
 import os
 from collections import defaultdict
@@ -99,6 +100,10 @@ MOVE_FRACTION = 0.02
 ROBUST_QUANTILE = 0.01
 #: A cluster's box is padded by this share of ``move_tol``.
 PAD_FRACTION = 0.25
+#: The position stage keeps at most this many nearest old candidates per new
+#: entity, so a dense drawing that moved as a whole (every entity left for this
+#: stage, hundreds of neighbours within ``move_tol``) cannot build n x m pairs.
+NEAREST_PER_ENTITY = 32
 DEFAULT_LIMIT = 200
 CATEGORIES = ("changed", "added", "removed")
 
@@ -239,12 +244,14 @@ def _nearest_pairs(
             if b is None:
                 continue
             cx, cy = math.floor(b[0] / move_tol), math.floor(b[1] / move_tol)
+            near: list[tuple[float, str, str, int, int]] = []
             for dx in (-1, 0, 1):
                 for dy in (-1, 0, 1):
                     for i, a in grid.get((cx + dx, cy + dy), ()):
                         distance = math.dist(a, b)
                         if distance <= move_tol:
-                            candidates.append((distance, olds[i].handle, news[j].handle, i, j))
+                            near.append((distance, olds[i].handle, news[j].handle, i, j))
+            candidates.extend(heapq.nsmallest(NEAREST_PER_ENTITY, near))
     candidates.sort()
     used_old: set[int] = set()
     used_new: set[int] = set()
@@ -296,10 +303,18 @@ def _movement(o: EntityRecord, n: EntityRecord, tol: float) -> dict:
         return {"geometry": {"from": [list(p) for p in before], "to": [list(p) for p in after]}}
     dx, dy = b[0] - a[0], b[1] - a[1]
     half = tol / 2.0
-    translated = len(before) == len(after) and all(
-        abs(p[0] + dx - q[0]) <= half and abs(p[1] + dy - q[1]) <= half
-        for p, q in zip(before, after, strict=True)
-    )
+
+    def translates(start: list, end: list) -> bool:
+        return len(start) == len(end) and all(
+            abs(p[0] + dx - q[0]) <= half and abs(p[1] + dy - q[1]) <= half
+            for p, q in zip(start, end, strict=True)
+        )
+
+    translated = translates(before, after)
+    if not translated and o.type == "LINE" and len(after) == 2:
+        # the signature ignores a LINE's direction, so its movement must too:
+        # the same line drawn the other way round and moved is a move
+        translated = translates(before, after[::-1])
     if not translated:
         return {"geometry": {"from": [list(p) for p in before], "to": [list(p) for p in after]}}
     if abs(dx) > half or abs(dy) > half:
@@ -308,12 +323,33 @@ def _movement(o: EntityRecord, n: EntityRecord, tol: float) -> dict:
 
 
 def _attrib_changes(o: EntityRecord, n: EntityRecord) -> list[dict]:
-    before, after = dict(o.attribs), dict(n.attribs)
-    return [
-        {"tag": tag, "from": before.get(tag), "to": after.get(tag)}
-        for tag in sorted(set(before) | set(after))
-        if before.get(tag) != after.get(tag)
-    ]
+    """One row per attribute tag whose values differ. A tag an INSERT carries
+    twice is compared as the list of its values, so one change cannot hide
+    behind the other; a tag carried once keeps plain ``from`` / ``to``."""
+
+    def by_tag(attribs) -> dict[str, list]:
+        values: dict[str, list] = defaultdict(list)
+        for tag, value in attribs:
+            values[tag].append(value)
+        return values
+
+    before, after = by_tag(o.attribs), by_tag(n.attribs)
+    rows = []
+    for tag in sorted(set(before) | set(after)):
+        old_values, new_values = before.get(tag, []), after.get(tag, [])
+        if old_values == new_values:
+            continue
+        if len(old_values) <= 1 and len(new_values) <= 1:
+            rows.append(
+                {
+                    "tag": tag,
+                    "from": old_values[0] if old_values else None,
+                    "to": new_values[0] if new_values else None,
+                }
+            )
+        else:
+            rows.append({"tag": tag, "from": old_values, "to": new_values})
+    return rows
 
 
 def _changes(o: EntityRecord, n: EntityRecord, tol: float) -> dict:
@@ -554,13 +590,16 @@ def diff_snapshots(
 
 
 def cap_report(result: dict, limit: int = DEFAULT_LIMIT) -> dict:
-    """``result`` with each list of ``CATEGORIES`` cut to ``limit`` rows and
-    ``truncated`` saying how many rows each cut dropped."""
+    """``result`` with each list of ``CATEGORIES`` and the ``clusters`` cut to
+    ``limit`` rows and ``truncated`` saying how many rows each cut dropped.
+
+    The clusters are capped in the report only: a markup still clouds every one.
+    """
     if int(limit) < 1:
         raise ValueError(f"limit must be at least 1; got {limit!r}")
     capped = dict(result)
     capped["truncated"] = {}
-    for category in CATEGORIES:
+    for category in (*CATEGORIES, "clusters"):
         rows = list(result.get(category) or [])
         capped[category] = rows[: int(limit)]
         capped["truncated"][category] = max(0, len(rows) - int(limit))

tests/test_understand_diff.py

@@ -294,10 +294,48 @@ def test_cap_report_cuts_every_list_and_says_how_much():
     new = snap(*STILL, *(line(f"N{i}", 0, 1000 * (i + 1), 100, 1000 * (i + 1)) for i in range(5)))
     capped = cap_report(diff_snapshots(old, new, move_tol=10.0), limit=2)
     assert len(capped["added"]) == 2
-    assert capped["truncated"] == {"changed": 0, "added": 3, "removed": 0}
+    # five lines 1000 apart are five clusters: the list is capped like the others
+    assert len(capped["clusters"]) == 2
+    assert capped["truncated"] == {"changed": 0, "added": 3, "removed": 0, "clusters": 3}
     assert capped["summary"]["added"] == 5
 
 
+def test_a_line_drawn_back_to_front_and_moved_is_a_move():
+    # the signature ignores a LINE's direction; so does its movement
+    old = snap(*STILL, line("D", 0, 200, 100, 200))
+    new = snap(*STILL, line("D", 150, 250, 50, 250))  # reversed, then moved by (50, 50)
+    (row,) = diff_snapshots(old, new, move_tol=100.0)["changed"]
+    assert row["changes"] == {"moved_by": [pytest.approx(50.0), pytest.approx(50.0)]}
+
+
+def test_an_attribute_tag_carried_twice_cannot_hide_a_change():
+    # as a dict, both revisions read NOTE = "B" and the edit of the first NOTE vanished
+    before = (("NOTE", "A"), ("NOTE", "B"))
+    after = (("NOTE", "X"), ("NOTE", "B"))
+    old = snap(*STILL, rec("I", "INSERT", "TAGS", ((500, 500),), block="TB", attribs=before))
+    new = snap(*STILL, rec("I", "INSERT", "TAGS", ((500, 500),), block="TB", attribs=after))
+    (row,) = diff_snapshots(old, new)["changed"]
+    assert row["changes"] == {"attribs": [{"tag": "NOTE", "from": ["A", "B"], "to": ["X", "B"]}]}
+
+
+def test_a_dense_drawing_moved_as_a_whole_pairs_each_entity_with_its_own_copy():
+    # 200 lines 10 apart on one layer, all moved (3, 0) under new handles: no exact
+    # match, so no handle stage, and with move_tol 500 each line has about a hundred
+    # candidates. The position stage keeps only the nearest few per entity, and each
+    # line must still pair with its own moved copy.
+    old = snap(*(line(f"O{i}", 0, 10 * i, 5, 10 * i) for i in range(200)))
+    new = snap(*(line(f"N{i}", 3, 10 * i, 8, 10 * i) for i in range(200)))
+    result = diff_snapshots(old, new, move_tol=500.0)
+    assert result["handles_stable"] is False
+    assert (result["summary"]["changed"], result["summary"]["added"]) == (200, 0)
+    assert result["summary"]["removed"] == 0
+    assert all(
+        row["changes"] == {"moved_by": [pytest.approx(3.0), pytest.approx(0.0)]}
+        and row["old_handle"][1:] == row["new_handle"][1:]
+        for row in result["changed"]
+    )
+
+
 # -- block definitions -----------------------------------------------------------------
 
 

tests/test_understand_diff_tool.py

@@ -46,7 +46,7 @@ async def test_the_current_document_against_its_older_revision(client, tmp_path)
         pair["insert"],
     }
     assert result["blocks"]["compared"] is True and result["blocks"]["changed"] == []
-    assert result["truncated"] == {"changed": 0, "added": 0, "removed": 0}
+    assert result["truncated"] == {"changed": 0, "added": 0, "removed": 0, "clusters": 0}
     assert await _total(client) == before, "a diff without markup writes nothing"