Audit the isolates the validators already cover (#350, #521)

.github/workflows/network-quality.yml

@@ -28,6 +28,9 @@ on:
   pull_request:
     paths:
       - 'kb/communities/*.yaml'
+      # Isolates are audited too, since #350. Without this an isolate
+      # could gain a dangling interaction and no run would look at it.
+      - 'data/isolates/*.yaml'
       - 'src/communitymech/network/**'
       - 'src/communitymech/schema/**'
       # Self-triggering, so a change to this workflow is exercised by the

scripts/validate_strict.py

@@ -40,6 +40,7 @@
 from linkml.validator.plugins import JsonschemaValidationPlugin
 from linkml.validator.report import Severity
 
+from communitymech.paths import default_record_roots
 from communitymech.validators.gtdb_coherence import validate_gtdb_coherence
 from communitymech.validators.gtdb_lineage_tree import check_lineage_shape
 from communitymech.validators.prokaryotic_lineage import check_record as check_prokaryotic_lineage
@@ -51,7 +52,9 @@
 # `data/isolates` uses the same taxon_term shape and was outside this gate
 # while the comment below claimed "the same gate as everything else"
 # (#390 review). It has been outside a gate once before (#310).
-DEFAULT_ROOTS = [_REPO_ROOT / "kb" / "communities", _REPO_ROOT / "data" / "isolates"]
+# Sourced rather than restated: the audit needs the same list, and when each
+# kept its own copy they drifted (#350).
+DEFAULT_ROOTS = default_record_roots()
 TARGET_CLASS = "MicrobialCommunity"
 
 

src/communitymech/cli.py

@@ -44,8 +44,12 @@ def cli():
 @click.option(
     "--communities-dir",
     type=click.Path(exists=True, file_okay=False, dir_okay=True, path_type=Path),
-    default="kb/communities",
-    help="Directory containing community YAML files",
+    # No default: falling through to None lets the auditor use
+    # `default_record_roots()`, which covers `data/isolates` too. A literal
+    # "kb/communities" here silently overrode that — the audit still reported
+    # 312 records after the auditor itself had been fixed (#350).
+    default=None,
+    help="Directory of records to audit (default: every record directory)",
 )
 @click.option(
     "--check-only",

src/communitymech/network/auditor.py

@@ -11,12 +11,13 @@
 import json
 import sys
 from collections import defaultdict
+from collections.abc import Iterable
 from enum import Enum
 from pathlib import Path
 
 import yaml
 
-from communitymech.paths import REPO_ROOT
+from communitymech.paths import REPO_ROOT, default_record_roots
 
 
 class IssueType(str, Enum):
@@ -115,8 +116,32 @@ def issue_severity(issue: dict) -> str:
 class NetworkIntegrityAuditor:
     """Audit community YAML files for network data integrity issues."""
 
-    def __init__(self, communities_dir: Path = Path("kb/communities")):
-        self.communities_dir = Path(communities_dir)
+    def __init__(self, communities_dir: Path | Iterable[Path] | None = None):
+        """
+        Args:
+            communities_dir: One directory, or several. Defaults to every
+                id-bearing record directory, sourced from
+                ``scripts/validate_strict.DEFAULT_ROOTS`` rather than restated,
+                so the set of records that are *audited* cannot drift from the
+                set that is *validated*. It had: `data/isolates/**` was added to
+                this workflow's triggers and to the validators, while the audit
+                stayed `kb/communities`-only, so editing an isolate re-ran a
+                suite that never looked at its interactions (#350).
+
+                A single Path is still accepted, because callers and tests pass
+                one directory and `NetworkIntegrityAuditor(tmp_path)` should
+                keep meaning what it always did.
+        """
+        if communities_dir is None:
+            roots = list(default_record_roots())
+        elif isinstance(communities_dir, (str, Path)):
+            roots = [Path(communities_dir)]
+        else:
+            roots = [Path(directory) for directory in communities_dir]
+        self.record_dirs = roots
+        # Retained: callers and the report header read `.communities_dir`, and
+        # the first of the roots is the one they mean by it.
+        self.communities_dir = roots[0] if roots else Path("kb/communities")
         self.issues: dict[str, list[dict]] = defaultdict(list)
 
     def audit_all(self, check_only: bool = False, quiet: bool = False) -> dict[str, list[dict]]:
@@ -135,7 +160,9 @@ def audit_all(self, check_only: bool = False, quiet: bool = False) -> dict[str,
         Returns:
             Dictionary mapping community names to their issues
         """
-        yaml_files = sorted(self.communities_dir.glob("*.yaml"))
+        yaml_files = sorted(
+            path for directory in self.record_dirs for path in directory.glob("*.yaml")
+        )
         verbose = not (check_only or quiet)
 
         if verbose:

src/communitymech/network/batch_reporter.py

@@ -24,7 +24,10 @@ def __init__(
         self,
         llm_client: AnthropicClient | None = None,
         validator: SuggestionValidator | None = None,
-        communities_dir: Path = Path("kb/communities"),
+        # None, not a literal: passing "kb/communities" through explicitly
+        # overrides the auditor's own default, so the repair path could not
+        # see records the audit reports on (#521).
+        communities_dir: Path | None = None,
         parallel: bool = True,
         max_workers: int = 4,
     ):
@@ -40,8 +43,11 @@ def __init__(
         """
         self.llm_client = llm_client or AnthropicClient()
         self.validator = validator or SuggestionValidator()
-        self.communities_dir = Path(communities_dir)
         self.auditor = NetworkIntegrityAuditor(communities_dir=communities_dir)
+        # Take it back off the auditor rather than recomputing, so the reporter
+        # and the audit it reports on cannot disagree about which records exist.
+        self.record_dirs = self.auditor.record_dirs
+        self.communities_dir = self.auditor.communities_dir
         self.parallel = parallel
         self.max_workers = max_workers
 
@@ -75,7 +81,9 @@ def generate_report(
         }
 
         # Get all community files
-        yaml_files = sorted(self.communities_dir.glob("*.yaml"))
+        yaml_files = sorted(
+            path for directory in self.record_dirs for path in directory.glob("*.yaml")
+        )
 
         if max_communities:
             yaml_files = yaml_files[:max_communities]

src/communitymech/paths.py

@@ -35,6 +35,24 @@
 REPORTS = REPO_ROOT / "reports"
 DOCS = REPO_ROOT / "docs"
 KB_COMMUNITIES = REPO_ROOT / "kb" / "communities"
+DATA_ISOLATES = REPO_ROOT / "data" / "isolates"
+
+
+def default_record_roots() -> list[Path]:
+    """Every directory holding `MicrobialCommunity` records, in one place.
+
+    The validators, the term checks and the network audit each need this list,
+    and each used to carry its own copy. They drifted: `data/isolates/**` was
+    added to `validate_strict`, `validate-all`, `validate-terms-all` and
+    `validate-references-all`, but the network auditor kept its own
+    `Path("kb/communities")` default and never saw an isolate's interactions
+    (#350).
+
+    Defining it once does not by itself keep a *new* directory in step — that is
+    what `tests/test_record_roots_are_shared.py` is for — but it removes the
+    copies that made the drift invisible.
+    """
+    return [KB_COMMUNITIES, DATA_ISOLATES]
 
 
 def looks_like_a_checkout() -> bool:

tests/test_record_roots_are_shared.py

@@ -0,0 +1,192 @@
+"""Every gate looks at the same record directories (#350).
+
+`data/isolates/**` was added to the network-quality workflow's trigger paths,
+and separately to `validate_strict`, `validate-all`, `validate-terms-all` and
+`validate-references-all`. The network auditor was not: it kept
+`communities_dir: Path = Path("kb/communities")`. So editing an isolate re-ran a
+suite whose only isolate coverage was schema validation, and the interactions in
+`data/isolates` — 12 of them across 3 records — were never audited at all.
+
+Nothing was wrong with any of them, which is the point. The gap was invisible
+because it produced no findings either way, and would have stayed invisible
+until an isolate gained a dangling reference.
+
+`default_record_roots()` is now the single source, in `communitymech.paths`, and
+this file checks the places that must agree with it. Two of those checks are
+about *shape* rather than content, because the failure mode was never a wrong
+list — it was a second list nobody remembered existed:
+
+* the CLI's `--communities-dir` must default to `None`. It defaulted to the
+  string `"kb/communities"`, which silently overrode the auditor's own default:
+  after the auditor was fixed, `audit-network` still reported 312 records. The
+  fix looked wired and was not, and only running it and reading the count found
+  that.
+* the workflow's trigger paths must mention every root. A gate that covers a
+  directory but never fires on changes to it is covered only by accident.
+"""
+
+from __future__ import annotations
+
+import ast
+import pathlib
+
+import pytest
+import yaml
+
+from communitymech.paths import default_record_roots
+
+REPO = pathlib.Path(__file__).parent.parent
+WORKFLOW = REPO / ".github/workflows/network-quality.yml"
+
+
+@pytest.fixture(scope="module")
+def roots() -> list[pathlib.Path]:
+    return default_record_roots()
+
+
+def test_the_roots_exist_and_hold_records(roots):
+    """Guard: a root that does not exist makes every sweep below vacuous."""
+    assert len(roots) >= 2
+    for root in roots:
+        assert root.is_dir(), f"{root} is not a directory"
+        assert list(root.glob("*.yaml")), f"{root} holds no records"
+
+
+def test_the_auditor_defaults_to_all_of_them(roots):
+    """The defect itself: the audit saw one root while the validators saw two."""
+    from communitymech.network.auditor import NetworkIntegrityAuditor
+
+    assert NetworkIntegrityAuditor().record_dirs == roots
+
+
+def test_a_single_directory_is_still_accepted():
+    """`NetworkIntegrityAuditor(tmp_path)` must keep meaning what it did.
+
+    Widening the default would be a poor trade if it broke every caller and
+    test that audits one directory deliberately.
+    """
+    from communitymech.network.auditor import NetworkIntegrityAuditor
+
+    only = REPO / "kb/communities"
+    assert NetworkIntegrityAuditor(only).record_dirs == [only]
+    assert NetworkIntegrityAuditor([only]).record_dirs == [only]
+
+
+def test_validate_strict_uses_the_shared_list():
+    """Parsed, not imported: importing the script pulls in linkml and forks.
+
+    Asserted positively — that `DEFAULT_ROOTS` is assigned from a call to
+    `default_record_roots` — rather than by checking the old literals are
+    absent, which would also pass on a file that stopped defining roots.
+    """
+    tree = ast.parse((REPO / "scripts/validate_strict.py").read_text(encoding="utf-8"))
+    assignments = [
+        node
+        for node in ast.walk(tree)
+        if isinstance(node, ast.Assign)
+        and any(isinstance(t, ast.Name) and t.id == "DEFAULT_ROOTS" for t in node.targets)
+    ]
+    assert len(assignments) == 1, "DEFAULT_ROOTS is gone or defined more than once"
+    value = assignments[0].value
+    assert isinstance(value, ast.Call), "DEFAULT_ROOTS is a literal again, not the shared list"
+    assert getattr(value.func, "id", None) == "default_record_roots"
+
+
+def test_the_cli_does_not_override_the_default():
+    """A literal default here silently defeats the auditor's own (#350).
+
+    This is the check that would have caught the half-fix: the auditor was
+    already correct while `audit-network` still reported 312 records, because
+    click passed `"kb/communities"` explicitly on every invocation.
+    """
+    tree = ast.parse((REPO / "src/communitymech/cli.py").read_text(encoding="utf-8"))
+    # Scoped to `audit_network`'s own decorators. A module-wide sweep also flags
+    # `generate-umap`, which takes the same option name but is a visualisation
+    # rather than a gate — whether isolates belong in the UMAP is a separate
+    # question (#519), not something to settle by widening this test.
+    command = next(
+        node
+        for node in ast.walk(tree)
+        if isinstance(node, ast.FunctionDef) and node.name == "audit_network"
+    )
+    offenders = []
+    for node in command.decorator_list:
+        if not isinstance(node, ast.Call):
+            continue
+        if getattr(node.func, "attr", None) != "option":
+            continue
+        names = [a.value for a in node.args if isinstance(a, ast.Constant)]
+        if "--communities-dir" not in names:
+            continue
+        for keyword in node.keywords:
+            if keyword.arg == "default" and not (
+                isinstance(keyword.value, ast.Constant) and keyword.value.value is None
+            ):
+                offenders.append(ast.unparse(keyword.value))
+    assert offenders == [], (
+        "`--communities-dir` has a non-None default "
+        f"({offenders}), which overrides `default_record_roots()` and drops "
+        "`data/isolates` from the audit without changing anything visible (#350)"
+    )
+
+
+def test_the_workflow_fires_on_every_root(roots):
+    """A gate that covers a directory but never triggers on it is luck."""
+    workflow = yaml.safe_load(WORKFLOW.read_text(encoding="utf-8"))
+    # `on` is parsed as the boolean True by YAML 1.1, which is why this reads
+    # the key rather than the attribute.
+    triggers = workflow.get("on") or workflow.get(True)
+    patterns = triggers["pull_request"]["paths"]
+    missing = [
+        str(root.relative_to(REPO))
+        for root in roots
+        if not any(str(root.relative_to(REPO)) in pattern for pattern in patterns)
+    ]
+    assert missing == [], (
+        "the network-quality workflow audits these directories but does not "
+        f"trigger on changes to them: {missing}. Add them to `paths:` (#350)."
+    )
+
+
+def test_no_constructor_hardcodes_the_old_root():
+    """The click default was one of two identical sites (#521).
+
+    `BatchReporter.__init__` took `communities_dir: Path = Path("kb/communities")`
+    and passed it to the auditor explicitly, so widening the auditor's default
+    did nothing for it — the same half-fix, in the tool that repairs exactly
+    what the audit reports. `cli.py` builds it as bare `BatchReporter()` in four
+    places, so the literal is what ran every time.
+
+    This walks the whole package rather than naming the two known offenders,
+    because the point is the *shape*: any default that hardcodes a record
+    directory reintroduces the drift `default_record_roots()` exists to remove.
+    `BrowserExporter` is exempt — it is an export feeding the browser UI, and
+    whether isolates belong in a visualisation is #519, not a coverage gap.
+    """
+    # Visualisation and export paths, not gates. Whether isolates belong in a
+    # UMAP or the browser UI is a modelling question (#519), and answering it by
+    # widening a coverage test would decide it silently.
+    exempt = {"browser_export.py", "render.py", "umap_generator.py"}
+    offenders = []
+    for path in sorted((REPO / "src/communitymech").rglob("*.py")):
+        if path.name in exempt:
+            continue
+        tree = ast.parse(path.read_text(encoding="utf-8"))
+        for node in ast.walk(tree):
+            if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
+                continue
+            arguments = node.args
+            names = [a.arg for a in arguments.posonlyargs + arguments.args + arguments.kwonlyargs]
+            defaults = list(arguments.defaults) + list(arguments.kw_defaults)
+            aligned = names[len(names) - len(defaults) :]
+            for name, default in zip(aligned, defaults, strict=True):
+                if name != "communities_dir" or default is None:
+                    continue
+                rendered = ast.unparse(default)
+                if "kb/communities" in rendered or "data/isolates" in rendered:
+                    offenders.append(f"{path.name}:{node.lineno} {name}={rendered}")
+    assert offenders == [], (
+        "these hardcode a record directory as a default, which overrides "
+        "`default_record_roots()` at the call site and silently narrows what "
+        "gets audited (#350, #521):\n" + "\n".join(offenders)
+    )