From 8b6f6166ccc18bfdb14421cb23667385f10ee865 Mon Sep 17 00:00:00 2001 From: "marcin p. joachimiak" <4625870+realmarcin@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:29:10 -0700 Subject: [PATCH 1/2] Audit the isolates the validators already cover (#350) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most of #350 had been fixed since it was filed: validate_strict's roots, validate-all, validate-terms-all and validate-references-all all cover data/isolates today, and the four term-validator failures it measured no longer reproduce. What remained is the network audit, which kept its own `Path("kb/communities")` default and so never looked at the 12 interactions in data/isolates. They are clean, and that is the point — the gap produced no findings either way and would have stayed invisible until an isolate gained a dangling reference. The workflow did not even trigger on data/isolates, so both halves were missing: a gate that did not fire, in front of a job that would not have covered it. `default_record_roots()` in communitymech.paths is now the one list; the auditor and validate_strict source it rather than restating it. A single directory is still accepted, since callers and tests pass one deliberately. The canary earned its keep. After the auditor was fixed the audit still reported 312 records, because click passed "kb/communities" explicitly on every invocation — the fix looked wired and was not, and only running it and reading the count showed that. `test_the_cli_does_not_override_the_default` exists for exactly that. It is scoped to audit_network's decorators: generate-umap takes the same option name, but whether isolates belong in a visualisation is a modelling question, filed as #519 rather than settled by widening a test. Mutation-checked: restoring the click default reddens the CLI test, removing the trigger path reddens the workflow test, and injecting a bogus participant into an isolate is now reported by the audit. Co-Authored-By: Claude Opus 5 --- .github/workflows/network-quality.yml | 3 + scripts/validate_strict.py | 5 +- src/communitymech/cli.py | 8 +- src/communitymech/network/auditor.py | 35 +++++- src/communitymech/paths.py | 18 ++++ tests/test_record_roots_are_shared.py | 148 ++++++++++++++++++++++++++ 6 files changed, 210 insertions(+), 7 deletions(-) create mode 100644 tests/test_record_roots_are_shared.py 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/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..e7319e1e --- /dev/null +++ b/tests/test_record_roots_are_shared.py @@ -0,0 +1,148 @@ +"""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)." + ) From 8f387b96bed7daa438ce14c808f291fa287fb6a4 Mon Sep 17 00:00:00 2001 From: "marcin p. joachimiak" <4625870+realmarcin@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:40:15 -0700 Subject: [PATCH 2/2] Fix the second copy of the same override (#521) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of this PR found BatchReporter doing exactly what the click option did: `communities_dir: Path = Path("kb/communities")`, passed to the auditor explicitly, so widening the auditor's default did nothing for it. cli.py builds it as bare `BatchReporter()` in four places, so the literal is what ran every time — meaning the LLM repair path could not see records the audit now reports on. Two halves of one workflow disagreeing about which records exist. It now takes the roots back off the auditor rather than recomputing them, so they cannot disagree by construction. The test that caught it is written against the shape rather than the two known names: any `communities_dir` default that hardcodes a record directory fails. A check naming only the click option is how the second site survived in the first place. Visualisation paths are exempt by name with the reason attached — whether isolates belong in a UMAP or the browser UI is #519, and widening a coverage test would answer it silently. Mutation-checked: restoring the literal reddens it. Co-Authored-By: Claude Opus 5 --- src/communitymech/network/batch_reporter.py | 14 +++++-- tests/test_record_roots_are_shared.py | 44 +++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) 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/tests/test_record_roots_are_shared.py b/tests/test_record_roots_are_shared.py index e7319e1e..3ec78d2a 100644 --- a/tests/test_record_roots_are_shared.py +++ b/tests/test_record_roots_are_shared.py @@ -146,3 +146,47 @@ def test_the_workflow_fires_on_every_root(roots): "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) + )