From 71aaa26bb516e2556955394dfe35ae5b7eb8b4b2 Mon Sep 17 00:00:00 2001 From: "marcin p. joachimiak" <4625870+realmarcin@users.noreply.github.com> Date: Thu, 6 Aug 2026 07:49:57 -0700 Subject: [PATCH 1/4] Make #369's one-off drift audit a standing invariant #369 audited all 366 grounded ids against the tool and found two blocks that disagreed - one resolving to a different rank, one to nothing. Re-auditing now, both are gone: the Chryseobacterium species/genus disagreement no longer occurs, and the Chlorobium block that grounds to nothing carries curated: true, which is a curator saying so rather than an unnoticed gap. So there is nothing to fix in the KB. What that audit was really testing is an invariant worth keeping: a stored grounding either matches what the tool produces today, or is marked curated: true. There is no third category - a block in it would be a claim nobody is accountable for, which is the state #294's status enum and #384's pin exist to prevent. Measured: 378 distinct grounded taxa, 11 blocks disagreeing with the tool, all 11 curated - nine demotion or nomenclature pins (#445, #451), two the Allobosea rename the crosswalk predates (#365). Three tests: the invariant, a guard on the fixture so an empty read cannot make it vacuous, and that every pin carries its reason. Verified the invariant fails when a pin loses its curated flag. Needs the crosswalk and so skips where that is absent, CI included - the same limitation as its neighbours. It earns its place anyway: the audit it replaces was run by hand once, and the KB has changed under it twice since. Co-Authored-By: Claude Fable 5 --- ...st_gtdb_uncurated_blocks_match_the_tool.py | 141 ++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 tests/test_gtdb_uncurated_blocks_match_the_tool.py diff --git a/tests/test_gtdb_uncurated_blocks_match_the_tool.py b/tests/test_gtdb_uncurated_blocks_match_the_tool.py new file mode 100644 index 00000000..04ebae3f --- /dev/null +++ b/tests/test_gtdb_uncurated_blocks_match_the_tool.py @@ -0,0 +1,141 @@ +"""Any grounding the tool would not produce must say a curator chose it (#369). + +#369 audited all 366 grounded ids against the tool and found two blocks that +disagreed with it — one resolving to a *different rank*, one to nothing at all — +and called them drift between the release a block was written from and the +current `NCBI2GTDB`. Re-auditing now, both are gone: the *Chryseobacterium* +species/genus disagreement no longer occurs, and the *Chlorobium* block that +grounds to nothing carries `curated: true`, which is a curator saying so rather +than an unnoticed gap. + +What that audit was really testing is an invariant worth keeping, so this makes +it standing rather than a one-off: **a stored grounding either matches what the +tool produces today, or is marked `curated: true`.** There is no third category. +A block in it would be a claim nobody is accountable for — the tool did not make +it and no curator signed it — and that is the state #294's status enum and +#384's pin exist to prevent. + +Measured when written: 378 distinct grounded taxa, 11 blocks disagreeing with +the tool, **all 11 curated**. Nine are demotion or nomenclature pins (#445, +#451), two are the *Allobosea* rename the crosswalk predates (#365). + +This needs the kg-microbe crosswalk and so skips where that is absent, CI +included — the same limitation as its neighbours. It earns its place anyway: the +audit it replaces was run by hand once, and the KB has changed under it twice +since. +""" + +from __future__ import annotations + +import importlib.util +import pathlib + +import pytest +import yaml + +REPO = pathlib.Path(__file__).parent.parent +RECORD_DIRS = ("kb/communities", "data/isolates") + + +@pytest.fixture(scope="module") +def gtdb(): + spec = importlib.util.spec_from_file_location("gtdb_ground", REPO / "scripts/gtdb_ground.py") + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture(scope="module") +def mapping(gtdb): + try: + path = gtdb.resolve_kg_microbe_dir(None) / "data/raw/NCBI2GTDB.tsv.gz" + except SystemExit as exc: + pytest.skip(f"kg-microbe mapping unavailable: {str(exc).splitlines()[0]}") + if not path.exists(): + pytest.skip(f"kg-microbe NCBI2GTDB mapping not available at {path}") + return path + + +def _grounded_taxa(): + """(curie, label) -> [(record, stored gtdb_id, curated)] for every block.""" + taxa: dict[tuple[str, str], list[tuple[str, str, bool]]] = {} + for directory in RECORD_DIRS: + for path in sorted((REPO / directory).glob("*.yaml")): + document = yaml.safe_load(path.read_text()) or {} + for entry in document.get("taxonomy") or []: + block = (entry or {}).get("taxon_term") or {} + grounding = block.get("gtdb_classification") + term = block.get("term") + if not isinstance(grounding, dict) or not isinstance(term, dict): + continue + if not term.get("id"): + continue + taxa.setdefault((term["id"], term.get("label") or ""), []).append( + (path.name, grounding.get("gtdb_id"), grounding.get("curated") is True) + ) + return taxa + + +@pytest.fixture(scope="module") +def audit(gtdb, mapping): + """Every stored grounding beside what the tool produces for it now.""" + taxa = _grounded_taxa() + want_ids, want_species, want_higher = set(), set(), set() + for curie, label in taxa: + clean = gtdb._clean_label(label) + if gtdb._is_species(clean): + want_ids.add(curie.split(":")[1]) + want_species |= set(gtdb.lookup_keys(label)) + elif clean: + want_higher |= set(gtdb.lookup_keys(label)) + by_id, by_name, by_higher = gtdb.collect_rows(mapping, want_ids, want_species, want_higher) + + rows = [] + for (curie, label), uses in sorted(taxa.items()): + found = gtdb.resolve_target(curie.split(":")[1], label, by_id, by_name, by_higher) + now = None if found is None else found.get("gtdb_id") + for record, stored, curated in uses: + rows.append((record, label, stored, now, curated)) + return rows + + +def test_the_audit_actually_read_the_kb(audit): + """Guard the fixture, so an empty read cannot make the next test vacuous.""" + assert len(audit) > 700, f"expected the KB's ~727 grounded blocks, read {len(audit)}" + assert any(stored != now for _, _, stored, now, _ in audit), ( + "no block disagrees with the tool at all, which has not been true since " + "#365 — the audit is probably not resolving anything" + ) + + +def test_a_block_the_tool_would_not_produce_is_curated(audit): + """The invariant: tool-derived, or curator-signed. Never neither.""" + unexplained = [ + f"{record}: {label!r} stores {stored}, tool says {now}" + for record, label, stored, now, curated in audit + if stored != now and not curated + ] + assert unexplained == [], ( + "these groundings match neither the tool nor a curator's decision — either " + "re-run `gtdb_ground.py --refresh --apply`, or pin them with `curated: true` " + "and a `curation_note` saying why (#369):\n" + "\n".join(unexplained) + ) + + +def test_every_curated_block_says_why(audit): + """A pin is only accountable if it carries its reason.""" + taxa = _grounded_taxa() + curated_records = {record for _, uses in taxa.items() for record, _, curated in uses if curated} + + missing = [] + for directory in RECORD_DIRS: + for path in sorted((REPO / directory).glob("*.yaml")): + if path.name not in curated_records: + continue + document = yaml.safe_load(path.read_text()) or {} + for entry in document.get("taxonomy") or []: + block = (entry or {}).get("taxon_term") or {} + grounding = block.get("gtdb_classification") or {} + if grounding.get("curated") is True and not grounding.get("curation_note"): + missing.append(f"{path.name}: {block.get('preferred_term')}") + assert missing == [], "curated without a note:\n" + "\n".join(missing) From 9cb6a4ff294d33a535a1ffbce94b93d019c6bd94 Mon Sep 17 00:00:00 2001 From: "marcin p. joachimiak" <4625870+realmarcin@users.noreply.github.com> Date: Thu, 6 Aug 2026 11:21:53 -0700 Subject: [PATCH 2/4] Address the #456 review: stop a pure-YAML check skipping in CI test_every_curated_block_says_why took the crosswalk-backed `audit` fixture and never used it - it calls _grounded_taxa() and re-reads the files itself. Its assertion is pure YAML, so the vestigial argument was the only thing making it skip where kg-microbe is absent, CI included. It is the one check here that can actually gate, and now does: verified it passes with KG_MICROBE_DIR pointed at nothing. Dropping the fixture also removed the file filter built from _grounded_taxa(), which required term.id while the inner loop did not - so a curated block with no term.id, in a file with no other curated block, would have escaped the note check. Zero such blocks today; the filter was only a parse-time optimisation and it was the thing introducing the gap. The failure message told the reader to run `--refresh --apply`, which is a guaranteed no-op when the tool reports None because it resolved the taxon as ambiguous - apply_to_community skips those. It now names --withdraw-ambiguous and the pin instead. Also record what the audit deliberately does not replicate: apply_to_community skips blocks in the legacy CURATED_GROUNDINGS list even without the flag, and per #384 the flag is primary - so a grounding protected only by the list is exactly what this test should complain about. The review reproduced the audit independently with its own walk and got the same numbers: 727 blocks, 378 distinct taxa, 11 disagreements, all curated, all with a note. It also confirmed both vacuity traps close - a resolver returning always None fails the invariant, always-the-stored-value fails the guard. Co-Authored-By: Claude Fable 5 --- ...st_gtdb_uncurated_blocks_match_the_tool.py | 35 ++++++++++++------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/tests/test_gtdb_uncurated_blocks_match_the_tool.py b/tests/test_gtdb_uncurated_blocks_match_the_tool.py index 04ebae3f..03f37043 100644 --- a/tests/test_gtdb_uncurated_blocks_match_the_tool.py +++ b/tests/test_gtdb_uncurated_blocks_match_the_tool.py @@ -19,10 +19,11 @@ the tool, **all 11 curated**. Nine are demotion or nomenclature pins (#445, #451), two are the *Allobosea* rename the crosswalk predates (#365). -This needs the kg-microbe crosswalk and so skips where that is absent, CI -included — the same limitation as its neighbours. It earns its place anyway: the -audit it replaces was run by hand once, and the KB has changed under it twice -since. +The invariant itself needs the kg-microbe crosswalk, so it skips where that is +absent — CI included, the same limitation as its neighbours. It earns its place +anyway: the audit it replaces was run by hand once, and the KB has changed under +it twice since. The third test here, that every pin carries its reason, reads +only YAML and therefore *does* gate in CI. """ from __future__ import annotations @@ -90,6 +91,10 @@ def audit(gtdb, mapping): want_higher |= set(gtdb.lookup_keys(label)) by_id, by_name, by_higher = gtdb.collect_rows(mapping, want_ids, want_species, want_higher) + # Deliberately not replicating `apply_to_community`'s two other skips: the + # legacy `CURATED_GROUNDINGS` list, and non-`NCBITaxon:` ids. Per #384 the + # block flag is primary and the list a fallback, so a grounding protected + # only by the list is exactly what this test should complain about. rows = [] for (curie, label), uses in sorted(taxa.items()): found = gtdb.resolve_target(curie.split(":")[1], label, by_id, by_name, by_higher) @@ -118,24 +123,30 @@ def test_a_block_the_tool_would_not_produce_is_curated(audit): assert unexplained == [], ( "these groundings match neither the tool nor a curator's decision — either " "re-run `gtdb_ground.py --refresh --apply`, or pin them with `curated: true` " - "and a `curation_note` saying why (#369):\n" + "\n".join(unexplained) + "and a `curation_note` saying why (#369). Where the tool now reports None " + "because it resolves the taxon as *ambiguous*, `--refresh --apply` will not " + "touch the block at all — that case needs `--withdraw-ambiguous` or a " + "pin:\n" + "\n".join(unexplained) ) -def test_every_curated_block_says_why(audit): - """A pin is only accountable if it carries its reason.""" - taxa = _grounded_taxa() - curated_records = {record for _, uses in taxa.items() for record, _, curated in uses if curated} +def test_every_curated_block_says_why(): + """A pin is only accountable if it carries its reason. + Deliberately takes no fixture. This is the one check here that reads only + YAML, so it is the one that can gate in CI — and an earlier version took the + crosswalk-backed `audit` fixture without using it, which skipped it there + for nothing. + """ missing = [] for directory in RECORD_DIRS: for path in sorted((REPO / directory).glob("*.yaml")): - if path.name not in curated_records: - continue document = yaml.safe_load(path.read_text()) or {} for entry in document.get("taxonomy") or []: block = (entry or {}).get("taxon_term") or {} - grounding = block.get("gtdb_classification") or {} + grounding = block.get("gtdb_classification") + if not isinstance(grounding, dict): + continue if grounding.get("curated") is True and not grounding.get("curation_note"): missing.append(f"{path.name}: {block.get('preferred_term')}") assert missing == [], "curated without a note:\n" + "\n".join(missing) From a52cad57fb384da9cd068812b003b0ff9fbb5b55 Mon Sep 17 00:00:00 2001 From: "marcin p. joachimiak" <4625870+realmarcin@users.noreply.github.com> Date: Thu, 6 Aug 2026 11:51:18 -0700 Subject: [PATCH 3/4] Re-trigger CI on the review-fix commit The push of 9cb6a4f produced no workflow run - the diff is test-only and tests/**/*.py is in the trigger paths, so it should have fired. Closing and reopening the PR did not start one either. An empty commit to get CI onto the final tree state rather than merging on a green run for the previous commit. Co-Authored-By: Claude Fable 5 From e3250c9272376a389a5bc2149915ea96ce2e0ca6 Mon Sep 17 00:00:00 2001 From: "marcin p. joachimiak" <4625870+realmarcin@users.noreply.github.com> Date: Thu, 6 Aug 2026 15:32:59 -0700 Subject: [PATCH 4/4] Re-trigger CI after the GitHub Actions outage Pushes during the outage (Actions was in major_outage from ~14:50 to ~22:00 UTC) produced no workflow run. Empty commit to queue one now that events are being accepted again. Co-Authored-By: Claude Fable 5