diff --git a/.github/workflows/network-quality.yml b/.github/workflows/network-quality.yml index af6d2ee8..12b45fc3 100644 --- a/.github/workflows/network-quality.yml +++ b/.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 diff --git a/scripts/validate_strict.py b/scripts/validate_strict.py index c524674a..9255cda7 100644 --- a/scripts/validate_strict.py +++ b/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" diff --git a/src/communitymech/cli.py b/src/communitymech/cli.py index 131a983a..78bb9ef7 100644 --- a/src/communitymech/cli.py +++ b/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", diff --git a/src/communitymech/network/auditor.py b/src/communitymech/network/auditor.py index afdf8b9c..251a0b35 100644 --- a/src/communitymech/network/auditor.py +++ b/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: diff --git a/src/communitymech/network/batch_reporter.py b/src/communitymech/network/batch_reporter.py index 2b9c20e4..70768ce7 100644 --- a/src/communitymech/network/batch_reporter.py +++ b/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] diff --git a/src/communitymech/paths.py b/src/communitymech/paths.py index aa844e18..f405975a 100644 --- a/src/communitymech/paths.py +++ b/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: diff --git a/tests/test_record_roots_are_shared.py b/tests/test_record_roots_are_shared.py new file mode 100644 index 00000000..3ec78d2a --- /dev/null +++ b/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) + )