fix: stop the cache-name loop, share the NCBITaxon adapter, write the protocol (#697, #700, #696, #704, #705)

.github/workflows/validate-strict.yaml

@@ -73,6 +73,21 @@ jobs:
         # are available for the test step below.
         run: uv sync --frozen --all-extras
 
+      # This lane runs the whole test suite, and parts of it consult NCBITaxon
+      # through OAK. Without a cache the runner re-downloads ncbitaxon.db.gz
+      # every time -- and when that download returned 0 bytes, main went red for
+      # a reason no PR had caused (#704). label-correspondence has cached
+      # ~/.data/oaklib since it was written; this lane never did.
+      #
+      # The cache is not the whole fix. A cold cache or an upstream outage still
+      # leaves the adapter absent, which is why the affected assertions now skip
+      # with a reason rather than failing. Cache first, skip as the backstop.
+      - name: Cache OAK ontologies
+        uses: actions/cache@v4
+        with:
+          path: ~/.data/oaklib
+          key: oaklib-${{ runner.os }}-v1
+
       - name: Run validate-strict (closed-schema LinkML validation)
         run: just validate-strict
 
@@ -119,5 +134,12 @@ jobs:
       - name: Install dependencies
         run: uv sync --frozen --all-extras
 
+      # Same reasoning as the lane above: this one runs the suite too (#704).
+      - name: Cache OAK ontologies
+        uses: actions/cache@v4
+        with:
+          path: ~/.data/oaklib
+          key: oaklib-${{ runner.os }}-v1
+
       - name: Run tests
         run: uv run pytest tests/ -q --no-cov

CLAUDE.md

@@ -221,6 +221,56 @@ accepted taxon, strain, metabolite, condition, causal direction, accession,
 citation, and snippet before curating it into YAML. Record LLM assistance in the
 new history entry.
 
+## Proving a gate can fail
+
+Gate tests here are defended by mutation testing -- break the thing, confirm the
+specific test goes red, restore. A dozen modules carry an explicit
+`test_the_check_can_actually_fail`. The ritual has three ways to lie, and two of
+them produce a **false green**: a test certified as able to fail when it cannot
+(#696).
+
+1. **Prove the mutation applied** before trusting a red. `grep -c` the new text,
+   or assert the replacement count. A red from an unapplied mutation is
+   impossible, so this only ever catches false greens -- which is the point.
+   Two real cases: a tuple entry Black had collapsed onto one line, and
+   `parents[2]` in `paths.py`, which appears twice.
+2. **Prove the restore took.** The suite must be green on the *very next* run.
+   Needing a second run is the stale-bytecode symptom fixed in #693; if it
+   happens again, something is serving a module that is not on disk.
+3. **Choose a mutation this machine can see.** `kb/taxa` -> `kb/TAXA` is a real
+   change on Linux and a no-op on macOS. Prefer changing a value the assertion
+   names over a path, a filename, or anything the filesystem may normalise.
+4. **Back up by copy, not by `git checkout --`.** Restoring with `git checkout`
+   discards unrelated uncommitted work in the same file. `cp` to a scratch path
+   and copy back.
+5. **Run a control arm.** Rules 1-3 all interrogate the *mutated* run, and
+   rule 4 the restore. None of them catches a red that had a second sufficient
+   cause. So
+   before attributing a red to the mutation, put an **unmutated** copy through
+   the identical harness and confirm it is GREEN.
+
+   `test_the_check_can_actually_fail` in `test_ncbitaxon_adapter_is_shared.py`
+   shipped without one (#709). It copied a test module to `tmp_path`, removed a
+   fixture argument, and asserted a red. It got a red -- from `fixture
+   'requires_ncbi_adapter' not found`, because pytest loads a conftest from the
+   test file's own directory and the copy had none. The unmutated copy failed
+   identically. The mutation applied, the restore took, the filesystem saw it,
+   and the check still could not fail.
+
+### A guard may narrow, never excuse
+
+Related failure, same shape (#700): **a substring may decide what a guard LOOKS
+AT; it must never decide that something is FINE.**
+
+`test_record_roots_are_shared.py` skipped any module whose text contained
+`record_files`, treating the presence of a name as proof of its use. A module
+imported it, never called it, and `ruff --fix` then deleted the unused import --
+leaving something that read as converted, was not, and passed the guard twice.
+
+Deciding membership by substring is fine when it widens the candidate set and a
+structural check follows. Exempting on presence is how a rename, an auto-fix, or
+an unused import turns a gate off with nobody noticing.
+
 ## Code changes
 
 - Keep reusable runtime code under `src/communitymech/`; keep one-off curation

justfile

@@ -141,7 +141,16 @@ cache-supplements *refs:
 # class separately as RENDERING; validator errors == RENDERING + MISMATCH there.
 # Do not "fix" a RENDERING hit by editing the snippet to match the cache.
 validate-references FILE:
+    #!/usr/bin/env bash
+    # The normalise step is not cosmetic. On a cache MISS the validator fetches,
+    # and it names what it fetches `DOI_*` -- a casing no reader here resolves
+    # to, so the next run misses again and re-fetches. Left alone that loop never
+    # converges (#697). The validator's exit code is preserved: normalising must
+    # not turn a failing validation green.
     uv run linkml-reference-validator validate data {{FILE}} -s src/communitymech/schema/communitymech.yaml --config conf/reference_validator.yaml
+    code=$?
+    PYTHONPATH=src uv run python scripts/normalize_cache_names.py
+    exit $code
 
 # Same, with the validator's "only abstract available" note corrected (#496) and
 # its silent stripping of [bracketed] text called out (#622). Prefer this when
@@ -166,6 +175,10 @@ validate-references-all:
         echo "\\nValidating references in $file..."
         uv run linkml-reference-validator validate data "$file" -s src/communitymech/schema/communitymech.yaml --config conf/reference_validator.yaml || rc=1
     done
+    # Once for the whole sweep rather than per file: any miss can write a
+    # mis-cased name, and renaming after each of 300 files costs more than it
+    # saves (#697).
+    PYTHONPATH=src uv run python scripts/normalize_cache_names.py
     exit $rc
 
 # Validate cross-repo IDs (CultureMech, MediaIngredientMech) in one community file.
@@ -320,7 +333,14 @@ validate-schema-terms:
 
 # Repair references with suggested fixes (dry-run)
 repair-references FILE:
+    #!/usr/bin/env bash
+    # `repair` fetches on a miss exactly as `validate` does, so it can leave a
+    # `DOI_*` name behind even in --dry-run: the dry run is about not editing the
+    # RECORD, not about not writing a cache (#697).
     uv run linkml-reference-validator repair data {{FILE}} -s src/communitymech/schema/communitymech.yaml --dry-run
+    code=$?
+    PYTHONPATH=src uv run python scripts/normalize_cache_names.py
+    exit $code
 
 # Run tests
 test:

scripts/normalize_cache_names.py

@@ -0,0 +1,145 @@
+"""Rename cache files the fetcher wrote with a prefix casing nothing resolves to.
+
+`linkml-reference-validator` normalises a reference id to `DOI:` and builds its
+cache path from that, so every cache miss it fills writes `DOI_*.md`
+(``etl/reference_fetcher.py:204-225``). Every reader in this repository builds
+`doi_...` from the `doi:` citation, so those files are unreachable on a
+case-sensitive filesystem — and per ``src/communitymech/paths.py`` an unreachable
+cache is not a skip, it sends the fetcher back to the network. 133 of them
+accumulated that way before #690, and #697 is the loop that refills the set.
+
+This runs after the validator rather than instead of it. The dependency is not
+ours to change; what is ours is that its output leaves the tree in a state the
+next run can use.
+
+**Renaming on a case-insensitive filesystem.** ``Path.rename`` from `DOI_x.md` to
+`doi_x.md` is a no-op on macOS — same file — so each rename goes via a temporary
+name. On Linux both forms can exist at once, which is a genuine conflict rather
+than a rename: those are reported and left alone, because picking a winner would
+silently discard one fetch.
+"""
+
+from __future__ import annotations
+
+import argparse
+import sys
+from pathlib import Path
+
+from communitymech.paths import REFERENCES_CACHE, canonical_cache_name
+
+# The name a rename passes through, so that `DOI_x.md` -> `doi_x.md` is two
+# real renames rather than one no-op on a filesystem that ignores case.
+TEMPORARY_SUFFIX = ".casetmp"
+
+
+def plan(cache_dir: Path) -> list[tuple[Path, Path]]:
+    """(current, wanted) for every file whose prefix casing is wrong."""
+    pairs = []
+    for path in sorted(cache_dir.iterdir()):
+        if not path.is_file():
+            continue
+        wanted = canonical_cache_name(path.name)
+        if wanted is not None:
+            pairs.append((path, path.with_name(wanted)))
+    return pairs
+
+
+def rename(current: Path, wanted: Path) -> str:
+    """Rename via a temporary name, so it works where case is ignored."""
+    if wanted.exists() and not _same_file(current, wanted):
+        return f"[conflict] {current.name}: {wanted.name} already exists and differs"
+    temporary = current.with_name(current.name + TEMPORARY_SUFFIX)
+    if temporary.exists():
+        # `Path.rename` would clobber it without a word, and a leftover
+        # temporary IS a cache file -- an earlier run died holding it. Refuse
+        # and let `recover` deal with it (#705).
+        return (
+            f"[conflict] {current.name}: {temporary.name} is left over from an "
+            f"interrupted run; it holds a fetch nothing else can reach"
+        )
+    current.rename(temporary)
+    temporary.rename(wanted)
+    return f"[renamed] {current.name} -> {wanted.name}"
+
+
+def orphans(cache_dir: Path) -> list[Path]:
+    """Temporaries left behind by a run that died between the two renames."""
+    return [
+        path
+        for path in sorted(cache_dir.iterdir())
+        if path.is_file() and path.name.endswith(TEMPORARY_SUFFIX)
+    ]
+
+
+def recover(path: Path) -> str:
+    """Finish an interrupted rename, rather than leaving the fetch unreachable.
+
+    A `.casetmp` file is invisible twice over: no reader resolves the name, and
+    `canonical_cache_name` returns None for it, so a later run of this script
+    passes straight over it. The reference then reads as a cache MISS, and a
+    miss sends the fetcher back to the network -- the loop #697 closes, reached
+    from the other side (#705).
+    """
+    stem = path.name[: -len(TEMPORARY_SUFFIX)]
+    wanted = path.with_name(canonical_cache_name(stem) or stem)
+    if wanted.exists() and not _same_file(path, wanted):
+        return f"[conflict] {path.name}: {wanted.name} already exists and differs"
+    path.rename(wanted)
+    return f"[recovered] {path.name} -> {wanted.name}"
+
+
+def _same_file(a: Path, b: Path) -> bool:
+    """True when the filesystem ignores case and both names are one file."""
+    try:
+        return a.stat().st_ino == b.stat().st_ino
+    except OSError:
+        return False
+
+
+def main(argv: list[str] | None = None) -> int:
+    parser = argparse.ArgumentParser(description=__doc__)
+    parser.add_argument("--cache-dir", type=Path, default=REFERENCES_CACHE)
+    parser.add_argument(
+        "--check",
+        action="store_true",
+        help="report without renaming, and exit 1 if anything needs it",
+    )
+    args = parser.parse_args(argv)
+
+    if not args.cache_dir.is_dir():
+        print(f"no cache directory at {args.cache_dir}", file=sys.stderr)
+        return 0  # nothing to normalise is not an error
+
+    conflicts = 0
+
+    # Before anything else: an interrupted earlier run leaves a `.casetmp`
+    # holding a real fetch that nothing can reach, and it also blocks the rename
+    # that would have produced it (#705).
+    for path in orphans(args.cache_dir):
+        if args.check:
+            print(f"[would recover] {path.name}")
+            conflicts += 1
+            continue
+        message = recover(path)
+        print(message)
+        conflicts += message.startswith("[conflict]")
+
+    pairs = plan(args.cache_dir)
+    for current, wanted in pairs:
+        if args.check:
+            print(f"[would rename] {current.name} -> {wanted.name}")
+            continue
+        message = rename(current, wanted)
+        print(message)
+        conflicts += message.startswith("[conflict]")
+
+    if args.check:
+        return 1 if (pairs or conflicts) else 0
+    # A conflict is left for a human: both casings exist as distinct files, and
+    # choosing between two fetches of the same reference is not this script's
+    # call.
+    return 1 if conflicts else 0
+
+
+if __name__ == "__main__":
+    raise SystemExit(main())

src/communitymech/ontology_adapters.py

@@ -0,0 +1,69 @@
+"""One construction site for the NCBITaxon adapter (#704).
+
+Two runtime modules needed the same OAK adapter and each built its own:
+`validators/ncbi_domain.py` for `domain_of`, `validators/shared_taxon_ids.py`
+for `rank_of` and `known_cores`. Same selector, same `try/except Exception:
+return None`, same `lru_cache(maxsize=1)` — one fact written down twice.
+
+**The cost was not duplication, it was measurement.** When
+`https://s3.amazonaws.com/bbop-sqlite/ncbitaxon.db.gz` began returning 403, the
+suite went red in a way that had nothing to do with the change under test. I
+gated the tests that fail when *one* of those two copies returns None, checked
+the count, and called it done. CI then failed on the other copy's dependants,
+because a probe pointed at `ncbi_domain._adapter` cannot see anything that asks
+`shared_taxon_ids._adapter`. Two copies meant a question about availability had
+two answers and no way to notice they had diverged.
+
+So the point of this module is less that the code is shared than that
+`ncbitaxon_available()` is a *single* question with a *single* answer, which
+`tests/conftest.py` and the guard test can both ask.
+
+**Not included here on purpose:** the ENVO and ChEBI adapters in
+`cross_repo_environment.py`. Those read a locally-cached sqlite by path and
+return None when the file is absent, which is a different contract — nothing is
+downloaded, so nothing can 403 — and folding them in would blur the one thing
+this module is for.
+"""
+
+from __future__ import annotations
+
+import functools
+from typing import Any
+
+# OAK's selector for the NCBITaxon SQLite build. Written once here so that a
+# grep for it has exactly one hit under `src/`, which is what
+# `tests/test_ncbitaxon_adapter_is_shared.py` asserts.
+NCBITAXON_SELECTOR = "sqlite:obo:ncbitaxon"
+
+
+@functools.lru_cache(maxsize=1)
+def ncbitaxon_adapter() -> Any | None:
+    """The NCBITaxon adapter, or None when it cannot be built.
+
+    Cached because building it opens a large SQLite database and a grounding run
+    asks about a few hundred taxa. Shared, so the two validators that need it
+    open that database once between them rather than once each.
+
+    None covers every reason the adapter is unavailable — oaklib not installed,
+    the download failing, a corrupt cache — because callers act on all of them
+    identically: they decline to judge. Distinguishing them here would invite a
+    caller to treat one of them as "fine".
+    """
+    try:
+        # oaklib ships no py.typed marker; same ignore as the call sites this
+        # replaced.
+        from oaklib import get_adapter  # type: ignore[import-untyped]
+
+        return get_adapter(NCBITAXON_SELECTOR)
+    except Exception:
+        return None
+
+
+def ncbitaxon_available() -> bool:
+    """Can a taxonomy lookup actually be made right now?
+
+    The one question `tests/conftest.py` asks before skipping a test whose
+    subject is the *result* of a lookup. Cheap to re-ask: the adapter behind it
+    is cached, so this is a dictionary hit after the first call.
+    """
+    return ncbitaxon_adapter() is not None