Harden pin resolution for safety-critical correctness

Code review fixes for the #432 loop-pin-labels feature:

- Fix loop rendering to use port indices instead of pin numbers
  (pre-existing bug: non-sequential pins produced wrong diagram)
- Add duplicate label check to the ambiguity branch in resolve_pin()
- Prevent self-referencing loops (pin looped to itself)
- Fix activate_pin() type annotation to accept Optional[Side]
- Deduplicate pin resolution: Harness.connect() now delegates to
  Connector.resolve_pin() instead of reimplementing the logic
- Add 21-test suite covering all resolution paths and error modes

src/wireviz/DataClasses.py

@@ -213,6 +213,11 @@ class Connector:
                 resolved.append(pin)
                 # Make sure loop connected pins are not hidden.
                 self.activate_pin(pin, None)
+            if resolved[0] == resolved[1]:
+                raise Exception(
+                    f'Loop in connector "{self.name}" connects pin '
+                    f'"{resolved[0]}" to itself.'
+                )
             self.loops[i] = resolved
 
         for i, item in enumerate(self.additional_components):
@@ -229,7 +234,13 @@ class Connector:
         in_labels = pin in self.pinlabels if self.pinlabels else False
 
         if in_pins and in_labels:
-            # present in both lists — check for ambiguity
+            # present in both lists — check for duplicate labels first
+            if self.pinlabels.count(pin) > 1:
+                raise Exception(
+                    f'Pin label "{pin}" in connector "{self.name}" '
+                    f"is defined more than once in pinlabels."
+                )
+            # then check for positional ambiguity
             if self.pins.index(pin) != self.pinlabels.index(pin):
                 raise Exception(
                     f'"{pin}" in connector "{self.name}" exists in both '
@@ -252,7 +263,7 @@ class Connector:
             f'Unknown pin "{pin}" for connector "{self.name}"!'
         )
 
-    def activate_pin(self, pin: Pin, side: Side) -> None:
+    def activate_pin(self, pin: Pin, side: Optional[Side]) -> None:
         self.visible_pins[pin] = True
         if side == Side.LEFT:
             self.ports_left = True

src/wireviz/Harness.py

@@ -105,28 +105,17 @@ class Harness:
         to_name: str,
         to_pin: (int, str),
     ) -> None:
-        # check from and to connectors
-        for name, pin in zip([from_name, to_name], [from_pin, to_pin]):
+        # resolve pin labels to pin numbers via Connector.resolve_pin()
+        for name, pin, is_from in [
+            (from_name, from_pin, True),
+            (to_name, to_pin, False),
+        ]:
             if name is not None and name in self.connectors:
-                connector = self.connectors[name]
-                # check if provided name is ambiguous
-                if pin in connector.pins and pin in connector.pinlabels:
-                    if connector.pins.index(pin) != connector.pinlabels.index(pin):
-                        raise Exception(
-                            f"{name}:{pin} is defined both in pinlabels and pins, for different pins."
-                        )
-                    # TODO: Maybe issue a warning if present in both lists but referencing the same pin?
-                if pin in connector.pinlabels:
-                    if connector.pinlabels.count(pin) > 1:
-                        raise Exception(f"{name}:{pin} is defined more than once.")
-                    index = connector.pinlabels.index(pin)
-                    pin = connector.pins[index]  # map pin name to pin number
-                    if name == from_name:
-                        from_pin = pin
-                    if name == to_name:
-                        to_pin = pin
-                if not pin in connector.pins:
-                    raise Exception(f"{name}:{pin} not found.")
+                resolved = self.connectors[name].resolve_pin(pin)
+                if is_from:
+                    from_pin = resolved
+                else:
+                    to_pin = resolved
 
         # check via cable
         if via_name in self.cables:
@@ -280,9 +269,14 @@ class Harness:
                 else:
                     raise Exception("No side for loops")
                 for loop in connector.loops:
+                    # Convert pin numbers to 1-based port indices.
+                    # Ports are named p{index+1} in the connector table,
+                    # not p{pin_number} — these differ for non-sequential pins.
+                    idx0 = connector.pins.index(loop[0]) + 1
+                    idx1 = connector.pins.index(loop[1]) + 1
                     dot.edge(
-                        f"{connector.name}:p{loop[0]}{loop_side}:{loop_dir}",
-                        f"{connector.name}:p{loop[1]}{loop_side}:{loop_dir}",
+                        f"{connector.name}:p{idx0}{loop_side}:{loop_dir}",
+                        f"{connector.name}:p{idx1}{loop_side}:{loop_dir}",
                         label=" ",  # Work-around to avoid over-sized loops.
                     )
 

tests/test_resolve_pin.py

@@ -0,0 +1,297 @@
+# -*- coding: utf-8 -*-
+
+"""Tests for Connector.resolve_pin() and loop pin resolution.
+
+Covers the fix for https://github.com/wireviz/WireViz/issues/432
+and related safety-critical pin resolution correctness.
+"""
+
+import pytest
+
+from wireviz.DataClasses import Connector
+
+
+def make_connector(pins=None, pinlabels=None, loops=None, **kwargs):
+    """Helper to build a Connector with minimal required fields."""
+    args = {"name": "X1"}
+    if pins is not None:
+        args["pins"] = pins
+    if pinlabels is not None:
+        args["pinlabels"] = pinlabels
+    if loops is not None:
+        args["loops"] = loops
+    args.update(kwargs)
+    return Connector(**args)
+
+
+# --- resolve_pin() happy paths ---
+
+
+class TestResolvePinHappyPaths:
+    def test_pin_number_passthrough(self):
+        """Pin number that exists in self.pins returns unchanged."""
+        c = make_connector(pins=[1, 2, 3])
+        assert c.resolve_pin(2) == 2
+
+    def test_label_resolves_to_pin_number(self):
+        """Pin label resolves to the corresponding pin number."""
+        c = make_connector(pins=[1, 2, 3], pinlabels=["VCC", "GND", "SIG"])
+        assert c.resolve_pin("GND") == 2
+
+    def test_label_with_non_sequential_pins(self):
+        """Labels work correctly even when pin numbers are non-sequential."""
+        c = make_connector(pins=[10, 20, 30], pinlabels=["A", "B", "C"])
+        assert c.resolve_pin("B") == 20
+
+    def test_value_in_both_lists_same_position(self):
+        """Value that exists in both pins and pinlabels at the same index."""
+        c = make_connector(pins=["A", "B", "C"], pinlabels=["A", "X", "Y"])
+        assert c.resolve_pin("A") == "A"
+
+    def test_empty_pinlabels_falls_through(self):
+        """When pinlabels is empty, pin numbers still resolve."""
+        c = make_connector(pins=[1, 2, 3], pinlabels=[])
+        assert c.resolve_pin(3) == 3
+
+
+# --- resolve_pin() error paths ---
+
+
+class TestResolvePinErrors:
+    def test_unknown_pin_raises(self):
+        """Resolving a pin that doesn't exist in either list raises."""
+        c = make_connector(pins=[1, 2, 3], pinlabels=["A", "B", "C"])
+        with pytest.raises(Exception, match="Unknown pin"):
+            c.resolve_pin("NONEXISTENT")
+
+    def test_unknown_number_raises(self):
+        """Resolving a pin number not in self.pins raises."""
+        c = make_connector(pins=[1, 2, 3])
+        with pytest.raises(Exception, match="Unknown pin"):
+            c.resolve_pin(99)
+
+    def test_ambiguous_pin_different_positions(self):
+        """Value in both pins and pinlabels at different positions raises."""
+        c = make_connector(pins=["A", "B", "C"], pinlabels=["X", "A", "Y"])
+        with pytest.raises(Exception, match="exists in both"):
+            c.resolve_pin("A")
+
+    def test_duplicate_label_only_in_labels(self):
+        """Duplicate label in pinlabels (label-only path) raises."""
+        c = make_connector(pins=[1, 2, 3], pinlabels=["A", "B", "A"])
+        with pytest.raises(Exception, match="defined more than once"):
+            c.resolve_pin("A")
+
+    def test_duplicate_label_in_both_lists(self):
+        """Duplicate label detected even when value also exists in pins (C-2 fix)."""
+        c = make_connector(pins=["A", "B", "C"], pinlabels=["A", "X", "A"])
+        with pytest.raises(Exception, match="defined more than once"):
+            c.resolve_pin("A")
+
+
+# --- Loop resolution ---
+
+
+class TestLoopResolution:
+    def test_loop_with_labels(self):
+        """Loops specified with pin labels resolve to pin numbers."""
+        c = make_connector(
+            pins=[1, 2, 3, 4],
+            pinlabels=["VCC", "GND", "TX", "RX"],
+            loops=[["VCC", "GND"]],
+        )
+        assert c.loops == [[1, 2]]
+
+    def test_loop_with_mixed_number_and_label(self):
+        """Loop with one pin number and one label resolves correctly."""
+        c = make_connector(
+            pins=[1, 2, 3, 4],
+            pinlabels=["VCC", "GND", "TX", "RX"],
+            loops=[[1, "GND"]],
+        )
+        assert c.loops == [[1, 2]]
+
+    def test_loop_with_non_sequential_pins(self):
+        """Loops resolve correctly with non-sequential pin numbering."""
+        c = make_connector(
+            pins=[10, 20, 30, 40],
+            pinlabels=["VCC", "GND", "TX", "RX"],
+            loops=[["VCC", "GND"]],
+        )
+        assert c.loops == [[10, 20]]
+
+    def test_loop_self_reference_raises(self):
+        """A loop from a pin to itself raises (I-2 fix)."""
+        with pytest.raises(Exception, match="to itself"):
+            make_connector(
+                pins=[1, 2, 3],
+                pinlabels=["A", "B", "C"],
+                loops=[["A", "A"]],
+            )
+
+    def test_loop_self_reference_via_label_and_number(self):
+        """Self-loop detected even when specified as label + number."""
+        with pytest.raises(Exception, match="to itself"):
+            make_connector(
+                pins=[1, 2, 3],
+                pinlabels=["A", "B", "C"],
+                loops=[[2, "B"]],
+            )
+
+    def test_loop_activates_pins(self):
+        """Loop-connected pins are marked visible (for hide_disconnected_pins)."""
+        c = make_connector(
+            pins=[1, 2, 3, 4],
+            pinlabels=["VCC", "GND", "TX", "RX"],
+            loops=[["TX", "RX"]],
+        )
+        assert c.visible_pins.get(3) is True
+        assert c.visible_pins.get(4) is True
+        # Unconnected pins should not be in visible_pins
+        assert 1 not in c.visible_pins
+
+    def test_multiple_loops(self):
+        """Multiple loops on the same connector all resolve correctly."""
+        c = make_connector(
+            pins=[1, 2, 3, 4, 5, 6],
+            pinlabels=["A", "B", "C", "D", "E", "F"],
+            loops=[["A", "B"], ["E", "F"]],
+        )
+        assert c.loops == [[1, 2], [5, 6]]
+
+
+# --- Loop rendering: non-sequential pins (C-1 fix) ---
+
+
+class TestLoopRendering:
+    """Verify that the GraphViz output uses port indices, not pin numbers.
+
+    This is the critical C-1 fix: Harness.py must convert resolved pin
+    numbers to 1-based position indices for GraphViz port references.
+    """
+
+    def _render_gv(self, yaml_str):
+        """Parse a YAML string through WireViz and return the GV source."""
+        import yaml
+
+        from wireviz.DataClasses import Metadata, Options, Tweak
+        from wireviz.Harness import Harness
+
+        data = yaml.safe_load(yaml_str)
+        harness = Harness(
+            metadata=Metadata(data.get("metadata", {})),
+            options=Options(**data.get("options", {})),
+            tweak=Tweak(**data.get("tweak", {})),
+        )
+        for name, conn_data in data.get("connectors", {}).items():
+            harness.add_connector(name, **conn_data)
+        for name, cable_data in data.get("cables", {}).items():
+            harness.add_cable(name, **cable_data)
+
+        graph = harness.create_graph()
+        return graph.source
+
+    def test_non_sequential_pins_use_indices(self):
+        """C-1 regression test: loops must use port indices, not pin numbers.
+
+        With pins: [10, 20, 30, 40], a loop between pins 10 and 20
+        must produce port references p1 and p2 (1-based indices),
+        NOT p10 and p20 (pin numbers).
+        """
+        yaml_str = """
+connectors:
+  X1:
+    pins: [10, 20, 30, 40]
+    pinlabels: [VCC, GND, TX, RX]
+    loops:
+      - [VCC, GND]
+cables:
+  W1:
+    wirecount: 2
+    colors: [BK, RD]
+connections:
+  -
+    - X1: [TX, RX]
+    - W1: [1-2]
+"""
+        gv = self._render_gv(yaml_str)
+        # The loop should reference p1 and p2 (positions), not p10 and p20.
+        # Look specifically for the loop edge pattern with port names.
+        import re
+
+        loop_edges = re.findall(r"X1:p(\d+)\w:\w -- X1:p(\d+)\w:\w", gv)
+        assert len(loop_edges) == 1, f"Expected 1 loop edge, found {len(loop_edges)}"
+        idx_a, idx_b = loop_edges[0]
+        # Pin 10 is at index 1, pin 20 is at index 2
+        assert idx_a == "1" and idx_b == "2", (
+            f"Loop ports should be p1/p2 (indices), got p{idx_a}/p{idx_b}"
+        )
+
+    def test_sequential_pins_still_work(self):
+        """Sequential pins (the common case) continue to work correctly."""
+        yaml_str = """
+connectors:
+  X1:
+    pinlabels: [VCC, GND, TX, RX]
+    loops:
+      - [VCC, GND]
+cables:
+  W1:
+    wirecount: 2
+    colors: [BK, RD]
+connections:
+  -
+    - X1: [TX, RX]
+    - W1: [1-2]
+"""
+        gv = self._render_gv(yaml_str)
+        # With sequential pins [1,2,3,4], p1/p2 are both the index and number
+        assert ":p1" in gv
+        assert ":p2" in gv
+
+
+# --- Harness.connect() delegation (I-1 fix) ---
+
+
+class TestHarnessConnectDelegation:
+    """Verify Harness.connect() now delegates to Connector.resolve_pin()."""
+
+    def test_connect_resolves_labels(self):
+        """Wire connections using pin labels resolve correctly."""
+        from wireviz.DataClasses import Cable, Metadata, Options, Tweak
+        from wireviz.Harness import Harness
+
+        harness = Harness(
+            metadata=Metadata({}),
+            options=Options(),
+            tweak=Tweak(),
+        )
+        harness.add_connector(
+            "X1", pins=[1, 2, 3], pinlabels=["VCC", "GND", "SIG"]
+        )
+        harness.add_connector("X2", pins=[1, 2, 3])
+        harness.add_cable("W1", wirecount=1, colors=["BK"])
+
+        # Connect using a label on the from side
+        harness.connect("X1", "SIG", "W1", 1, "X2", 1)
+
+        # The connection should store the resolved pin number (3), not the label
+        conn = harness.cables["W1"].connections[0]
+        assert conn.from_pin == 3
+
+    def test_connect_rejects_unknown_pin(self):
+        """Wire connections with unknown pins raise via resolve_pin()."""
+        from wireviz.DataClasses import Metadata, Options, Tweak
+        from wireviz.Harness import Harness
+
+        harness = Harness(
+            metadata=Metadata({}),
+            options=Options(),
+            tweak=Tweak(),
+        )
+        harness.add_connector("X1", pins=[1, 2, 3])
+        harness.add_connector("X2", pins=[1, 2, 3])
+        harness.add_cable("W1", wirecount=1, colors=["BK"])
+
+        with pytest.raises(Exception, match="Unknown pin"):
+            harness.connect("X1", 99, "W1", 1, "X2", 1)