Skip to content

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

Merged
realmarcin merged 7 commits into
mainfrom
fix-697-700-696-evidence-integrity
Aug 30, 2026
Merged

fix: stop the cache-name loop, share the NCBITaxon adapter, write the protocol (#697, #700, #696, #704, #705)#702
realmarcin merged 7 commits into
mainfrom
fix-697-700-696-evidence-integrity

Conversation

@realmarcin

@realmarcin realmarcin commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Closes #696, #697, #700, #704, #705.

#697 — the loop that costs money

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 here builds doi_… from the
doi: citation, so the file it just wrote is unreachable — and per
src/communitymech/paths.py an unreachable cache is not a skip, it sends the
fetcher back to the network. Miss, fetch, write an unreadable name, miss
again. 133 files accumulated that way before #690.

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. scripts/normalize_cache_names.py now runs
after it. Option 1 from #697; option 3 (accept both casings on read) stays
rejected — it makes local and CI answers differ, which is #694's failure.

Details that matter:

  • Renaming goes via a temporary nameDOI_x.mddoi_x.md is a no-op
    where case is ignored.
  • Both casings existing as distinct files is a conflict, not a rename:
    reported and left alone, because choosing between two fetches of the same
    reference is not a script's call.
  • Only the prefix is normalised. 10.1134/S0026261716060059 is a real DOI;
    lowercasing the whole filename would break resolution rather than fix it.
  • The canonical prefix map lives in communitymech.paths, imported by both the
    renamer and the checker. A second copy is how 133 references_cache files are named DOI_* while every citation says doi:, so 23% of the cache is invisible on Linux #690's two halves got out of
    step.

The guard caught a recipe I missed. I wired validate-references and
validate-references-all; the test immediately failed on repair-references,
which fetches on a miss exactly as validate does — its --dry-run is about not
editing the record, not about not writing a cache.

It also pins the subtler half: the validator's exit code must survive the
rename.
A recipe ending with a successful rename reports success whatever the
validation said, turning an evidence gate into a no-op. Verified both ways (clean
record → 0, planted fabrication → 1) and mutation-checked by removing the step.

#700 — the audit, and its honest result

Searching for the shape (a substring deciding a skip) across tests/,
scripts/ and src/ found three sites. Only one was the defect, and it was
already fixed:

site verdict
test_record_roots_are_shared's _SHARED_MARKERS the instance — fixed in #689
test_snippet_rendering_artefacts:103 the check's own logic; reports only snippets that do not match
test_gtdb_coherence_validator:644 a parse-saving shortcut that decided coverage by substring — removed

That last one would have silently skipped a curation_note written with any
other spacing. 328 files parse in under a second, which is not worth a blind
spot.

any(marker in body) in test_cache_is_source_text_not_notes is left alone: the
full-text markers are written into the cache by the fetchers, so presence is the
definition rather than an exemption — and it already recognises both forms
deliberately, which is the trap that twice destroyed real full text.

So the pattern was not widespread. The rule is the deliverable.

#696 — the protocol

CLAUDE.md gains "Proving a gate can fail": the mutation ritual has three
ways to lie and two produce a false green — a test certified as able to fail
when it cannot, which is precisely what the ritual exists to prevent. All three
were hit in one session: an unapplied mutation (twice), stale bytecode, and a
mutation this filesystem cannot observe. Plus the fourth point, about damage
rather than lying: back up by copy, because git checkout -- discards unrelated
uncommitted work in the same file.

The #700 rule lands beside it, since both are about evidence that looks like
evidence and is not: a substring may decide what a guard looks at, never that
something is fine.

#704 — one adapter, so availability is one question

ncbi_domain and shared_taxon_ids each built their own OAK adapter from
sqlite:obo:ncbitaxon, with the same except Exception: return None. The two
copies always agreed, so nothing ever looked wrong. What the duplication broke
was a measurement.

When s3.amazonaws.com/bbop-sqlite/ncbitaxon.db.gz began answering 403
verified from a developer machine on both HEAD and a ranged GET, so it is
upstream, not a runner — I gated the tests that fail when
ncbi_domain._adapter() returns None and shipped. CI failed on thirteen more,
because a probe pointed at one copy is blind to every caller of the other.

communitymech.ontology_adapters is now the one construction site and
ncbitaxon_available() the one question. Both validators alias _adapter to
it, which keeps monkeypatch.setattr(module, "_adapter", …) working for the
tests whose subject is the absent case.

Re-measured properly: the whole suite run against an unreachable ontology,
not one module. tests/test_ncbitaxon_adapter_is_shared.py holds it —
identity (is, not equivalence), one selector literal under src/, and a
subprocess run of the gated modules made hermetic by an empty PYSTOW_HOME
plus an unroutable proxy, so it needs no network and cannot pull 13 GB the day
the 403 lifts. GATED_MODULES is pinned rather than discovered by scanning for
the fixture name — a scan takes its coverage from the thing it checks — and it
caught two modules I had missed on its first run.

The skip says SKIPPED, NOT PASSED, matching what shared_taxon_ids already
prints to stderr. #708 tracks that two gates are consequently off until the
upstream artefact returns.

The review found a false green in this PR's own mutation check (#709)

test_the_check_can_actually_fail copied a module to tmp_path, removed a
fixture argument, and asserted a red. It got one — from fixture 'requires_ncbi_adapter' not found, because pytest loads a conftest from the
test file's own directory and the copy had none. An unmutated copy fails
identically (31 passed, 6 errors), so the test passed whatever the mutation
did.

That is #696's failure mode, committed one commit after writing #696's
protocol, inside the file whose subject is the same discipline. No run shows
it; green is what it produces either way. Fixed in fefa833: the conftest is
copied alongside, and the test grew a control arm — the unmutated copy must
be green through the identical harness before the mutated arm's red means
anything.

CLAUDE.md gains that as rule 5. Rules 1–3 interrogate the mutated run and
rule 4 the restore; none catches a red with a second sufficient cause.

#705 — an interrupted rename stranded the fetch

Also from reviewing this PR's diff. rename goes via a temporary name, and
Path.rename clobbers its destination silently — that destination being a
cache file an earlier run died holding. Worse, doi_x.md.casetmp is a name no
reader resolves and canonical_cache_name returns None for, so a later pass
stepped over it: the reference reads as a MISS, and a miss goes back to the
network. #697's loop, reached from the other side.

rename now refuses when a leftover exists; recover() finishes the
interrupted rename; main sweeps orphans first. Mutation-checked by removing
the guard from the executed source and asserting the leftover is clobbered.

Verification

just lint                     clean (black, ruff, mypy)
pytest tests/                 2856 passed, 17 skipped
pytest, ontology unreachable  2828 passed, 42 skipped   <- the CI condition
CI validate-strict            green: 2771 passed, 100 skipped

The two suite runs differ only in skips. The outage run is not a guess at
CI's environment: PYSTOW_HOME points at an empty directory and an unroutable
proxy blocks the re-download, which reproduces the exact Downloading ncbitaxon.db.gz: 0.00B symptom seen on main.

CI is green because the ontology is unavailable and the checks skipped
loudly — not because a cache warmed. From the run log:

Downloading ncbitaxon.db.gz: 0.00B [00:00, ?B/s]
[taxon-ids] NCBITaxon is unavailable, so the shared-id check (#292) was skipped, not passed.
[gtdb-domain] NCBITaxon is unavailable, so the prokaryote-only check (#365) was skipped, not passed.

One test skips on macOS and says so rather than passing vacuously — the conflict
case needs two files a case-insensitive filesystem cannot hold.

Known unrelated failure: test_no_snippet_stops_mid_word fails locally on
data/isolates/Methylobacterium_REE_Ewaste_Platform.yaml, driven by an untracked
local cache; it skips on a clean checkout.

Filed from the review

issue in this PR?
#705 interrupted rename strands a fetch fixed — new code, belongs here
#709 mutation check was a false green fixed — new code, belongs here
#706 normaliser's exit code discarded, so a conflict cannot fail anything filed — policy call; validate-references is not a CI gate
#707 OAK cache key never rotates, pinning one ontology snapshot filed — pre-existing pattern; caching strategy is its own change
#708 the #292/#365 gates are off while bbop-sqlite 403s filed — tracking, so turning them back on is an action someone takes

realmarcin and others added 3 commits August 28, 2026 19:14
…#697)

`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 here builds `doi_...` from the
`doi:` citation, so the file it just wrote is unreachable -- and per
src/communitymech/paths.py an unreachable cache is not a skip, it sends the
fetcher back to the network. The loop never converges: miss, fetch, write an
unreadable name, miss again. 133 files accumulated that way before #690.

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, so `scripts/normalize_cache_names.py` runs
after it. Option 1 from #697; option 3 (accept both casings on read) stays
rejected -- it makes the local and CI answers differ, which is #694's failure.

Renaming goes via a temporary name because `DOI_x.md` -> `doi_x.md` is a no-op
where case is ignored. Where BOTH casings exist as distinct files -- possible on
Linux -- that is a conflict, not a rename: reported and left alone, because
choosing between two fetches of the same reference is not a script's call.

Only the PREFIX is normalised. A DOI suffix is case-significant --
10.1134/S0026261716060059 is a real DOI -- so lowercasing the whole filename
would break resolution rather than fix it.

The canonical prefix map lives in communitymech.paths, imported by both the
renamer and the checker. A second copy is how #690's two halves got out of step:
the repo-side writer was fixed and the upstream one, which nobody had listed,
went on producing DOI_*.

The guard found a recipe I had missed. I wired `validate-references` and
`validate-references-all`; the test immediately failed on `repair-references`,
which fetches on a miss exactly as validate does -- its --dry-run is about not
editing the RECORD, not about not writing a cache. Wired too.

It also pins the subtler half: the validator's exit code must survive the
rename. A recipe ending with a successful rename reports success whatever the
validation said, turning an evidence gate into a no-op. Verified both ways --
clean record exits 0, planted-fabrication record exits 1 -- and mutation-checked
by removing the step from one recipe.

The conflict test skips on macOS, which it says out loud rather than passing
vacuously: a case-insensitive filesystem cannot hold the two files the test
needs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…le (#696, #700)

#696 -- the ritual this repository defends its gate tests with is written
nowhere, and it has three ways to lie. Two produce a FALSE GREEN: a test
certified as able to fail when it cannot, which is exactly what the ritual
exists to prevent. All three were hit in one session:

  * the mutation never applied -- a tuple entry Black had collapsed onto one
    line, and `parents[2]` in paths.py, which appears twice;
  * stale bytecode served a module that was not on disk (fixed mechanically in
    #693, but invisible to someone reading a test);
  * the mutation was real and undetectable here -- kb/taxa -> kb/TAXA changes
    behaviour on Linux and nothing on macOS.

The protocol adds the fourth point that is not about lying but about damage:
back up by copy, because `git checkout --` discards unrelated uncommitted work
in the same file.

#700 -- the audit, and the honest result. Searching for the shape (a substring
deciding a SKIP) across tests/, scripts/ and src/ found three sites, and only
one was the defect:

  * test_record_roots_are_shared's `_SHARED_MARKERS` -- the instance, already
    fixed in #689;
  * test_snippet_rendering_artefacts:103 -- `if flat in text: continue` is the
    check's own logic, reporting only snippets that do NOT match;
  * test_gtdb_coherence_validator:644 -- `if "curation_note:" not in text:
    continue`, a parse-saving shortcut that decided coverage by substring. A
    note written with any other spacing would have been skipped silently. The
    shortcut is removed: 328 files parse in under a second, which is not worth
    a blind spot.

`any(marker in body)` in test_cache_is_source_text_not_notes is left alone. The
full-text markers are literally written into the cache by the fetchers, so
presence is the definition rather than an exemption -- and it already recognises
both forms deliberately, which is the trap that twice destroyed real full text.

So the pattern was not widespread. The rule is the deliverable: a substring may
decide what a guard LOOKS AT, never that something is FINE.

2848 passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#697)

Adversarial review of this branch, and the finding is a comment of mine that was
not true.

CANONICAL_CACHE_PREFIXES said it was "derived from the citations rather than
chosen" and then listed seven prefixes. That is true of `doi` and `PMID` -- 1096
and 4716 citations respectively -- and FALSE of the other five. Nothing cites
`pmc:`, `europepmc:`, `epmc:`, `openalex:` or `semanticscholar:`; nothing in this
repository writes or resolves those caches. So their "canonical" casing was a
guess dressed as a rule, and the normaliser would have renamed 39 files on the
strength of it.

They move to UNRESOLVED_CACHE_PREFIXES: recorded as considered rather than
overlooked, neither renamed nor flagged. `canonical_cache_name` returns None for
them, so the blast radius of the renamer is now exactly the two prefixes a reader
can be wrong about.

The checker also kept its OWN seven-entry list. That is the duplication #690 is
a case study in -- two copies of "which casing is right", fixed in one place and
not the other. It now imports both sets and calls `canonical_cache_name`, so the
renamer and the checker cannot disagree.

Verified by planting files rather than by reading: a `DOI_*` file fails the
checker, the normaliser renames it, the checker passes; a `zenodo_*` file fails
the unknown-prefix guard. 2848 passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI went red on this PR, and the cause is not this PR: validate-strict is
failing on MAIN too, at 22673ba, which had passed as a PR hours earlier. The
runner log says why --

    Downloading ncbitaxon.db.gz: 0.00B [00:00, ?B/s]

so the OAK adapter is None, every domain lookup answers None, and 12 assertions
about lookup results fail for a reason unrelated to any change. That is how a
real failure gets waved through as "the flaky one".

It can happen because validate-strict has NO actions/cache step -- 0, against
label-correspondence's 1 -- so the lane that runs the whole suite re-downloads
the database every time with nothing to fall back on. Both suite-running lanes
now cache ~/.data/oaklib the way label-correspondence has since it was written.

The cache is not the whole fix: a cold cache or an upstream outage still leaves
the adapter absent. So the assertions that need a lookup now skip with a stated
reason. Which 12 was MEASURED, not guessed -- an autouse fixture forcing
_adapter to None, then reading off the failures.

Three deliberate exclusions, each a case where skipping would remove the point:

  * test_an_unavailable_adapter_degrades_rather_than_guesses and
    test_an_unavailable_ontology_is_reported_not_silently_passed assert the
    ABSENT case. They must run precisely when the adapter is gone.
  * the False half of test_scope_is_decided_only_when_it_can_be. `False` must
    hold with or without a lookup -- that one-directional property is what every
    caller relies on -- so it stays running and only the True cases request the
    fixture.

The fixture is function-scoped, not session-scoped: _adapter is already
lru_cached so re-asking is free, and a session fixture would freeze the answer
at whatever the first test happened to see.

Verified by forcing the adapter to None: 12 skipped, 95 passed. The two failures
that remain under that probe are artefacts of the probe itself -- the test does
`real_adapter.cache_clear()` and the probe replaced _adapter with a plain lambda
that has no such attribute, which the test's own comment anticipates. On CI the
function is the real lru_cached one that merely returns None, so both pass.

2848 passed locally.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
realmarcin and others added 2 commits August 29, 2026 22:56
`ncbi_domain` and `shared_taxon_ids` each built their own OAK adapter from
`sqlite:obo:ncbitaxon`, with the same `except Exception: return None`. The
duplication never changed behaviour -- the two copies always agreed -- so it
read as harmless. What it broke was a measurement.

When `s3.amazonaws.com/bbop-sqlite/ncbitaxon.db.gz` began answering 403
(verified from a developer machine: upstream, not a runner), I gated the tests
that fail when `ncbi_domain._adapter()` returns None and shipped. CI failed on
a further thirteen, because a probe pointed at one copy cannot see any caller
of the other.

`communitymech.ontology_adapters` is now the one construction site, and
`ncbitaxon_available()` the one question. Both validators alias `_adapter` to
it, which keeps `monkeypatch.setattr(module, "_adapter", ...)` working for the
tests whose subject IS the absent case.

Re-measured properly this time: the whole suite run against an unreachable
ontology, not one module. 13 more tests depend on a real lookup -- 12 in
`test_shared_taxon_ids.py`, one in `test_prokaryotic_lineage.py` -- and each
now takes `requires_ncbi_adapter`. The skip says SKIPPED, NOT PASSED, which is
the wording `shared_taxon_ids` already prints to stderr for this situation:
a green run that had no ontology must never read as a clean KB.

`tests/test_ncbitaxon_adapter_is_shared.py` holds it:

* the two validators resolve `_adapter` to the same object -- identity, not
  equivalence, since two functions that merely behave alike is precisely the
  state that hid this;
* the selector literal appears once under `src/`. No exemption list: the ENVO
  and ChEBI adapters are outside the guard by what they are, not by being
  excused for containing a token (#700);
* the gated modules skip rather than fail with NCBITaxon unreachable, checked
  in a subprocess made hermetic by an empty `PYSTOW_HOME` plus an unroutable
  proxy -- so it neither needs the network nor downloads 13 GB the day the 403
  is lifted. It refuses to run rather than pass if the simulation did not take.

`GATED_MODULES` is pinned rather than discovered by scanning for the fixture
name: a scan would take its coverage from the thing it checks, so deleting the
last fixture use would drop a module out of the set and go green. It caught two
modules I had missed on its first run.

Mutations verified with the count printed before each run, and green on the
very next run after each restore.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…704)

`test_the_check_can_actually_fail` ran a mutated copy of
`test_prokaryotic_lineage.py` from `tmp_path` and asserted it went red. It did
go red -- but not because of the mutation. pytest loads a conftest from the
test file's own directory, and `tmp_path` had none, so every test requesting
`requires_ncbi_adapter` errored with "fixture not found". An UNMUTATED copy
fails the same way, verified: 31 passed, 6 errors.

So the test passed whatever the mutation did. That is the #696 failure mode --
a test certified as able to fail when it cannot -- committed inside the change
that documents it, one commit after writing the protocol down. Found by
reviewing the diff rather than by any run, because there is no run that shows
it: green is what it produces either way.

Two changes. `tests/conftest.py` is now copied next to the module copy, which
makes the fixture resolve. And the test grew a control arm: the unmutated copy
must be GREEN through the identical harness before the mutated arm's red is
attributed to anything. One arm proves nothing on its own.

Both arms mutation-checked, with the mutation's presence printed before each
run:

* replace the mutation with a no-op -> RED (the assertion discriminates);
* delete the conftest copy -> SKIPPED, naming a fixture error as a likely
  cause. Not a pass: a broken harness must refuse to certify rather than
  certify wrongly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`normalize_cache_names.rename` goes through a temporary name, because
`DOI_x.md` -> `doi_x.md` is a no-op where the filesystem ignores case. Two
holes in those three lines, both found reviewing this PR's own diff.

**A leftover `.casetmp` was silently overwritten.** `Path.rename` clobbers its
destination without a word, and that destination is a cache file -- an earlier
run died holding it. `rename` now refuses and says so.

**A crash between the two renames stranded the fetch.** `doi_x.md.casetmp` is
a name no reader resolves, and `canonical_cache_name` returns None for it, so a
later pass of this very script stepped over it. The reference then reads as a
MISS, and a miss sends the fetcher back to the network -- the loop #697 exists
to close, reached from the other side. `recover()` finishes the rename, and
`main` sweeps orphans before doing anything else, since a leftover also blocks
the rename that would have produced it.

Both mutation-checked. `test_the_check_can_actually_fail` mutates the guard out
of the script's source and asserts the leftover IS clobbered, so the test
cannot pass with the guard gone; separately, stubbing `recover` to a no-op reds
the recovery test.

Also adds rule 5 to "Proving a gate can fail": run a control arm. Rules 1-3
interrogate the mutated run and rule 4 the restore; none of them catches a red
with a second sufficient cause, which is exactly what #709 was. Written from
that failure rather than in the abstract.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@realmarcin realmarcin changed the title fix: stop the cache-name loop, audit the substring guards, write the protocol (#697, #700, #696) fix: stop the cache-name loop, share the NCBITaxon adapter, write the protocol (#697, #700, #696, #704, #705) Aug 30, 2026
@realmarcin
realmarcin merged commit 851b39a into main Aug 30, 2026
5 checks passed
@realmarcin
realmarcin deleted the fix-697-700-696-evidence-integrity branch August 30, 2026 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The mutation-testing protocol is undocumented, and three of its failure modes produce false greens

1 participant