refactor: a private name must have a reader

changelog.d/3583-private-name-needs-a-reader.md

@@ -0,0 +1,22 @@
+### Changed: a private name must have a reader
+
+Two private module-level names were defined and never read anywhere in the
+tree. `_VALID_MODES` in `strands_robots.robot` named the three mode spellings
+while every site that decides or reports a mode - `_auto_detect_mode`'s
+membership test, its warning and the factory's `ValueError` - spelled them
+inline, so the constant documented an owner that did not exist.
+`_PEERS_VERSION` in `strands_robots.mesh.session` was a registry version
+counter incremented under `_PEERS_LOCK` at every insert, eviction, prune and
+clear, and consulted by nothing: four write sites and three `global`
+declarations maintaining a change signal for a cache that was never built.
+
+Both are removed. `tests/test_no_private_name_is_defined_and_never_read.py`
+pins the invariant for module-level private names and private methods across
+`strands_robots/`, the module-scope half of what
+`test_no_private_state_is_written_and_never_read.py` holds for instance
+attributes. A reader is a load of the identifier, an attribute or import of
+it, a string literal containing it, or any mention under `tests/`,
+`tests_integ/`, `examples/` or `scripts/` - an example reaches a backend's
+private helper through `getattr`, and it ships in the same tree. The grader
+excludes its own file from that read, so naming an offender in its docstring
+does not clear it.

strands_robots/mesh/session.py

@@ -603,13 +603,11 @@ def peer_is_physical(peer: Mapping[str, Any] | None) -> tuple[bool, str]:
 
 
 _PEERS: dict[str, PeerInfo] = {}
-_PEERS_VERSION: int = 0
 _PEERS_LOCK = threading.Lock()
 
 
 def update_peer(peer_id: str, peer_type: str, hostname: str, caps: dict[str, Any]) -> bool:
     """Insert or update a peer.  Returns ``True`` when the peer is new."""
-    global _PEERS_VERSION  # noqa: PLW0603 - module-level singleton by design
     with _PEERS_LOCK:
         is_new = peer_id not in _PEERS
         # When a NEW peer would push us over the cap, evict the oldest
@@ -621,7 +619,6 @@ def update_peer(peer_id: str, peer_type: str, hostname: str, caps: dict[str, Any
             while len(_PEERS) >= cap and _PEERS:
                 oldest_id = min(_PEERS, key=lambda pid: _PEERS[pid].last_seen_mono)
                 del _PEERS[oldest_id]
-                _PEERS_VERSION += 1
                 logger.warning(
                     "Mesh: peer registry at cap (%d); evicted oldest peer %s",
                     cap,
@@ -634,8 +631,6 @@ def update_peer(peer_id: str, peer_type: str, hostname: str, caps: dict[str, Any
             last_seen_mono=time.monotonic(),
             caps=caps,
         )
-        if is_new:
-            _PEERS_VERSION += 1
         return is_new
 
 
@@ -663,7 +658,6 @@ def prune_peers(timeout: float = PEER_TIMEOUT) -> list[str]:
     Returns:
         List of pruned peer IDs (may be empty).
     """
-    global _PEERS_VERSION  # noqa: PLW0603
     # Argument order is load-bearing: max() keeps its FIRST operand when a
     # comparison answers False, so with the timeout first a nan that somehow
     # reached this line degrades to the timeout instead of to never-pruned.
@@ -676,7 +670,6 @@ def prune_peers(timeout: float = PEER_TIMEOUT) -> list[str]:
         stale = [pid for pid, p in _PEERS.items() if now - p.last_seen_mono > cutoff]
         for pid in stale:
             del _PEERS[pid]
-            _PEERS_VERSION += 1
             pruned.append(pid)
     for pid in pruned:
         # Under retention this fires at retention expiry, potentially long
@@ -734,10 +727,8 @@ def peer_count() -> int:
 
 def clear_peers() -> None:
     """Remove **all** peers.  Intended for tests only."""
-    global _PEERS_VERSION  # noqa: PLW0603
     with _PEERS_LOCK:
         _PEERS.clear()
-        _PEERS_VERSION += 1
 
 
 # Session lifecycle

strands_robots/robot.py

@@ -71,8 +71,6 @@
 
 logger = logging.getLogger(__name__)
 
-_VALID_MODES = ("sim", "real", "auto")
-
 
 def _normalize_mode(mode: Any) -> str:
     """Lowercase + strip a mode value if it's a string. Pass non-str through unchanged

tests/test_no_private_name_is_defined_and_never_read.py

@@ -0,0 +1,188 @@
+"""A private module-level name or method must have a reader.
+
+This is the module-scope half of the invariant
+:mod:`tests.test_no_private_state_is_written_and_never_read` holds for instance
+attributes. A ``_name`` bound at module scope - a constant, a function, a
+class - or a ``_method`` defined in a class body, that nothing in the tree
+refers to, is dead code by the project's own rule ("if it's not called and not
+part of base class, delete it"), and it is dead in the direction that costs a
+reader: a private constant spelling a vocabulary reads as the place that
+vocabulary is owned, so the reader goes looking for the call sites that
+consult it and finds that every consumer spells the literal instead.
+
+Two instances on the tree this arrived in, out of 714 private methods and
+every module-level private name in the package. ``_VALID_MODES`` in
+:mod:`strands_robots.robot` named the three mode spellings while the three
+sites that decide or report a mode (``_auto_detect_mode``'s membership test,
+its warning and the factory's ``ValueError``) each spelled them inline, so the
+constant documented an owner that did not exist. ``_PEERS_VERSION`` in
+:mod:`strands_robots.mesh.session` was the other kind - a registry version
+counter incremented under the lock at every insert, eviction, prune and
+clear, and consulted by nothing, so four write sites and three ``global``
+declarations maintained a change signal no cache ever read.
+
+A reader is any of
+
+* a load of the identifier anywhere in the package - a ``Name`` in ``Load``
+  context, an ``Attribute`` reached through ``self`` or a module alias, or an
+  ``import`` of the name from another module;
+* a string literal containing the identifier anywhere in the package, which
+  covers the ``getattr(module, "_name")`` shape and a ``__all__``-style
+  ledger that names the symbol;
+* any mention of the identifier under ``tests/``, ``tests_integ/``,
+  ``examples/`` or ``scripts/`` - an example that reaches a backend's private
+  helper through ``getattr(sim, "_name", None)`` is a reader the package
+  cannot see, and it ships in the same tree. This file's own text is left out
+  of that read, so naming an offender here does not clear it.
+
+Dunder names are out of scope (``__all__``, ``__getattr__`` and friends are
+protocol, not state), and so is the bare ``_`` throwaway. A name that is
+defined in several modules counts as read if any of them reads it, the same
+simplification the instance-state grader makes for a mixin's state.
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+from collections import defaultdict
+from pathlib import Path
+
+REPO_ROOT = Path(__file__).resolve().parent.parent
+PACKAGE = REPO_ROOT / "strands_robots"
+MENTION_TREES = (REPO_ROOT / "tests", REPO_ROOT / "tests_integ", REPO_ROOT / "examples", REPO_ROOT / "scripts")
+
+# Private names that are defined and never read, and correctly so. One reason
+# per entry; an entry without a reason that survives review is a bug in review.
+DEFINED_WITHOUT_A_READER_BY_DESIGN: dict[str, str] = {}
+
+_IDENTIFIER = re.compile(r"[A-Za-z_][A-Za-z0-9_]*")
+
+
+def _is_private(name: str) -> bool:
+    return name.startswith("_") and not name.startswith("__") and name != "_"
+
+
+def _definitions(tree: ast.Module) -> list[tuple[str, int]]:
+    """Private names bound at module scope, plus private methods in class bodies.
+
+    Descends through module-level ``if`` / ``try`` / ``with`` blocks, which is
+    where ``TYPE_CHECKING`` mirrors and optional-import fallbacks bind names,
+    and stops at a function body: a name bound there is a local, not a symbol.
+    """
+    found: list[tuple[str, int]] = []
+
+    def visit(statements: list[ast.stmt], *, in_class: bool) -> None:
+        for node in statements:
+            if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
+                if _is_private(node.name):
+                    found.append((node.name, node.lineno))
+            elif isinstance(node, ast.ClassDef):
+                if not in_class and _is_private(node.name):
+                    found.append((node.name, node.lineno))
+                visit(node.body, in_class=True)
+            elif in_class:
+                continue
+            elif isinstance(node, ast.Assign):
+                for target in node.targets:
+                    for name_node in ast.walk(target):
+                        if isinstance(name_node, ast.Name) and _is_private(name_node.id):
+                            found.append((name_node.id, node.lineno))
+            elif isinstance(node, (ast.AnnAssign, ast.AugAssign)):
+                if isinstance(node.target, ast.Name) and _is_private(node.target.id):
+                    found.append((node.target.id, node.lineno))
+            elif isinstance(node, (ast.If, ast.Try, ast.With, ast.For, ast.While)):
+                for field in ("body", "orelse", "finalbody"):
+                    visit(getattr(node, field, []), in_class=False)
+                for handler in getattr(node, "handlers", []):
+                    visit(handler.body, in_class=False)
+
+    visit(tree.body, in_class=False)
+    return found
+
+
+def _readers(tree: ast.Module) -> set[str]:
+    """Every identifier the module refers to other than by defining it."""
+    names: set[str] = set()
+    for node in ast.walk(tree):
+        if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Load):
+            names.add(node.id)
+        elif isinstance(node, ast.Attribute):
+            names.add(node.attr)
+        elif isinstance(node, ast.alias):
+            names.add(node.name.rsplit(".", 1)[-1])
+            if node.asname:
+                names.add(node.asname)
+        elif isinstance(node, ast.Constant) and isinstance(node.value, str):
+            names.update(_IDENTIFIER.findall(node.value))
+    return names
+
+
+def _scan() -> tuple[dict[str, list[str]], int, int]:
+    """Return (unread private name -> definition sites, definitions seen, files parsed)."""
+    files = sorted(PACKAGE.rglob("*.py"))
+    definitions: dict[str, list[str]] = defaultdict(list)
+    read: set[str] = set()
+    for path in files:
+        tree = ast.parse(path.read_text(encoding="utf-8"))
+        for name, lineno in _definitions(tree):
+            definitions[name].append(f"{path.relative_to(REPO_ROOT)}:{lineno}")
+        read |= _readers(tree)
+
+    own = Path(__file__).resolve()
+    mention_text = "\n".join(
+        path.read_text(encoding="utf-8")
+        for tree in MENTION_TREES
+        if tree.is_dir()
+        for path in tree.rglob("*.py")
+        if path.resolve() != own
+    )
+    read |= set(_IDENTIFIER.findall(mention_text))
+
+    unread = {
+        name: sites
+        for name, sites in definitions.items()
+        if name not in read and name not in DEFINED_WITHOUT_A_READER_BY_DESIGN
+    }
+    return unread, sum(len(sites) for sites in definitions.values()), len(files)
+
+
+def test_every_private_name_defined_in_the_package_has_a_reader() -> None:
+    unread, _, _ = _scan()
+    assert not unread, "Private names defined and never read:\n" + "\n".join(
+        f"  {name}: {', '.join(sites)}" for name, sites in sorted(unread.items())
+    )
+
+
+def test_the_scan_reaches_the_package() -> None:
+    """A scan that walked nothing would pass the check above vacuously."""
+    _, definitions_seen, file_count = _scan()
+    assert file_count > 200, f"only {file_count} package files parsed"
+    assert definitions_seen > 1000, f"only {definitions_seen} private definitions seen"
+
+
+def test_every_exemption_states_a_reason() -> None:
+    for name, reason in DEFINED_WITHOUT_A_READER_BY_DESIGN.items():
+        assert len(reason.split()) >= 5, f"{name} needs a reason, not a label: {reason!r}"
+
+
+def test_a_module_constant_nothing_reads_is_reported() -> None:
+    """The grader's own verdict on the shape it was written for, without the tree."""
+    source = (
+        "_VOCABULARY = ('sim', 'real')\n"
+        "_USED = 3\n"
+        "def _helper():\n"
+        "    return _USED\n"
+        "class _Owner:\n"
+        "    def _unused_method(self):\n"
+        "        pass\n"
+        "    def _called(self):\n"
+        "        return self._called\n"
+        "def public():\n"
+        "    return _helper(), _Owner()\n"
+    )
+    tree = ast.parse(source)
+    defined = {name for name, _ in _definitions(tree)}
+    unread = defined - _readers(tree)
+    assert defined == {"_VOCABULARY", "_USED", "_helper", "_Owner", "_unused_method", "_called"}
+    assert unread == {"_VOCABULARY", "_unused_method"}