fix: three gates that could not see what they checked (#686, #689, #690) - #692
Merged
Conversation
references_cache/ carried two conventions for one prefix -- 133 files named
DOI_* and 79 named doi_* -- while every reference in the corpus writes `doi:`
in lowercase (1096 occurrences, zero uppercase). The stem convention shared by
this repo's scripts and by the upstream validator,
reference.replace(":", "_").replace("/", "_")
produces `doi_...`, so the DOI_* half was findable only on a filesystem that
ignores case. macOS ignores case; Linux does not.
Not a tidiness problem. linkml_reference_validator's fetcher builds the same
stem and calls .exists() (etl/reference_fetcher.py:204-225), so on Linux those
were cache MISSES -- and per src/communitymech/paths.py a miss is not a skip,
it sends the fetcher to the network. 117 of 514 distinct references (23%) were
in that state.
All 133 renamed to lowercase. `git mv DOI_x.md doi_x.md` is a no-op under
core.ignorecase=true, so each went via a temporary name; verified afterwards
that 0 tracked DOI_* remain and 212 doi_* exist (79 + 133), with no .casetmp
left behind and no stem colliding in both cases.
tests/test_reference_cache_names_are_case_exact.py stops it returning. It uses
os.listdir rather than Path.is_file(), because is_file() on a case-insensitive
filesystem answers this exact question with a "yes" Linux would not give -- the
check has to be one that can fail on the machine running it. Mutation-checked
by renaming one cache back to DOI_*, with the rename confirmed applied before
trusting the red.
One untracked working-tree cache was renamed the same way rather than left to
fail the new check locally; its content is untouched.
Out of scope, recorded rather than changed: 39 cache files carry prefixes no
reference cites (pmc_, europepmc_, epmc_, openalex_, semanticscholar_), and 13
references have no cache at all. Neither is a case defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`conf/id_label_targets.yaml` has a `kgx_nodes` target globbing
output/kgx/nodes.tsv. output/kgx/ is gitignored with zero tracked files, and
no job built the export before `just validate-products` ran -- so on CI the
glob matched nothing, the target skipped, and the gate printed "All id-label
pairs correspond" having never read the export. Reproduced by moving the local
artifact aside: exit 0, `- kgx_nodes: no files match`, green.
Three real MISMATCHes were sitting behind that skip:
CHEBI:49591 label='europium(3+) cation' canonical='europium(3+)'
CHEBI:49650 label='holmium(3+) cation' canonical='holmium(3+)'
CHEBI:49746 label='lutetium(3+) cation' canonical='lutetium(3+)'
They are the same `+ cation` typo already waived once for CHEBI:16793
("mercury(2+) cation"), in a hand-written metal->CHEBI map. The waiver called
it an exporter-side design choice; it was not, it was a typo, and it had been
covering for three more of its own kind. All four labels are corrected at the
source and the waiver is DELETED rather than multiplied -- mercury now passes
as OK_CANONICAL, which also confirms the canonical form the waiver asserted.
Exceptions 13 -> 12, errors 0.
The workflow now runs `just kgx-export` before both `report-label-drift` and
`validate-products` -- the report too, since it is the triage artifact people
read when the gate fails and would otherwise under-report exactly then. The
target becomes `required: true`, so an export that stops being produced is a
MISSING_GLOB error instead of a silent skip.
tests/test_generated_gate_inputs_are_built_in_ci.py holds both halves
together: build-before-validate AND required. Either alone restores the hole --
a required target with no build fails every run, a build step with an optional
target goes quiet the day the build breaks. Mutation-checked three ways
(drop `required`, move the build after the gate, delete the build step), each
confirmed applied before trusting the red.
`output/kgx/**` is removed from the paths filter: a gitignored directory can
never put a file in a pull request, so it advertised a trigger that could not
fire. test_ci_triggers_cover_what_the_gates_read.py was requiring exactly that
-- it treated every read root as needing a trigger, which is right for
committed data and meaningless for generated data, and is what produced the
dead entry. Generated roots are now exempted there and carry the
build-before-read obligation in the new module instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_no_constructor_hardcodes_the_old_root scans src/communitymech/**. Many of this repository's gates ARE tests, so a test globbing kb/communities alone was invisible to it -- which is how the #529 discriminator swept one root for months and why the shared-roots guard did not catch it. Only modules that SWEEP the directory are classified. A test naming one record as a fixture is not making a scope decision, and asking it to justify itself would train people to add entries without thinking. 14 modules sweep; the detection is pinned by a >= 10 guard so a broken scanner cannot quietly make the check vacuous. The split is evidence, not taste. data/isolates holds 4 records with 66 snippets, 3 ecological_interactions, 3 gtdb_classification blocks, 0 cultivation_setup and 0 go_terms, and none of the 4 is rendered into docs/ (324 pages for 324 kb/communities records). _COMMUNITY_ONLY (3): writers_leave_a_trace is scoped by its own argument; docs_do_not_contradict_the_kb and network_palette follow the rendered pages, which isolates do not have. _OWED_BOTH_ROOTS (11): each with the measured consequence -- 66 unchecked snippets for the two snippet gates, 3 unchecked interactions, 3 unchecked groundings, and two that miss nothing today only because isolates carry no cultivation_setup or go_terms yet, and would not notice the first that does. test_network_auditor passes communities_dir= explicitly, overriding the auditor's shared default -- the exact shape #350 fixed in the auditor and left standing in its test. Mutation-checked: removing an entry fails test_every_sweeping_test_declares_its_scope, and a planted nonexistent module fails both the rot check and the bound. Each mutation was confirmed applied before the red was trusted. Emptying _OWED_BOTH_ROOTS is follow-up work; this commit makes the 12th addition a choice someone defends rather than a drift nobody sees. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…#686) Adversarial review of this PR found the rename was only half the fix, plus a trap in the test that proved it. 1. scripts/cache_fulltext.py::_doi_cache_path PREFERRED `DOI_` and fell back to `doi_` last, so a DOI with no cache yet was CREATED uppercase — unreachable on a case-sensitive filesystem the moment anything read it back. That is how 133 of them accumulated, and renaming them without this would have refilled the set on the next fetch. Canonical is now lowercase; uppercase is still READ so an old local file is found rather than silently re-fetched, but it is never the name a new file gets. 2. The test that pins this initially loaded the script with importlib.spec_from_file_location. `scripts/` is not a package, so that writes scripts/__pycache__ — and Python validates that cache on (mtime, size). Flipping `doi_` to `DOI_` changes neither, so a mutation and its restore within the same second are indistinguishable to the loader. It served a STALE module and produced a FALSE RED; the same mechanism would just as happily have produced a false green. The test now compiles the source text on every run and creates no __pycache__ at all. Re-verified: the mutation fails, and the restore returns to green immediately rather than one run later. The legacy-read assertion was also platform-dependent — it asserted path equality, which holds on Linux and not on macOS, where the lowercase candidate already `.exists()` for a file written uppercase. It now asserts the returned path exists and holds the right content, which is the claim that matters and is true on both. 3. test_generated_gate_inputs_are_built_in_ci named one workflow, so a SECOND workflow running `just validate-products` without building output/kgx would reopen #686 with every existing assertion still green. It now checks every workflow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI failed test_no_snippet_stops_mid_word on this branch, and the cause is this
branch: renaming the DOI caches let the check reach a source it had never been
able to read on Linux.
`_cached` tried two glob patterns, `{key}*` and `{key.upper()}*`. The second was
meant to catch the uppercase-prefix caches and never could -- it upper-cases the
WHOLE stem, so "doi_10.1128_aem..." becomes "DOI_10.1128_AEM...", matching no
real filename. So on Linux the reference resolved to nothing and the record was
skipped; on macOS the filesystem found DOI_*.md and the check ran. Six findings
in Tinto_River_Iron_Cycling_Community.yaml sat on the wrong side of that split.
They are cache artefacts, not truncated quotes. The abstract's italic taxon
names lost their surrounding spaces when fetched, so
"...organisms related to Leptospirillum spp., Acidithiobacillus..."
is cached as "...related toLeptospirillumspp.,Acidithiobacillus...". The snippet
ends at "related to", a real word boundary in the article; only the cache
disagrees. That is exactly what _CACHE_RUN_TOGETHER is for, and the entry says
why rather than just listing the triple. Mutation-checked: removing it flags all
six again.
The dead `key.upper()` fallback is deleted rather than fixed. Prefixes are
canonically lowercase since #690, and a pattern that cannot match is worse than
no pattern, because it reads as coverage.
I earlier told the user this test's failure was entirely their untracked WIP.
That was half right: the Methylobacterium finding is (its cache is untracked, so
CI cannot see it), and the Tinto findings were not -- they are real, mine to
surface, and were hidden by the case bug.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 28, 2026
Closed
realmarcin
added a commit
that referenced
this pull request
Aug 28, 2026
…694) (#698) Two parts, and the second turned up something the closure of #690 got wrong. PART 1 -- the broken-fallback audit. Every `.upper()` in tests/, scripts/ and src/ was checked: none participates in a filename or glob. The hits are report text, env-var and CURIE-prefix comparisons, and enum lookups. Every cache resolver now builds the canonical stem and looks it up with an exact `is_file()`; the one lenient case (evidence_snippet_audit.py:174) does a case-insensitive substring scan over the directory listing, which is loose but cannot silently miss. So the broken fallback was a single instance, already removed in #692. Recorded rather than assumed. PART 2 -- the italic artefact, chased to its actual cause. Three hypotheses died on the way, each tested rather than asserted: * "BeautifulSoup get_text() with no separator ate the spaces" -- reproduced on synthetic JATS and it did NOT; the spaces survive. * "Crossref stores it run-together" -- fetched the live record: the raw abstract has newlines and indentation between `related to` and `<jats:italic>Leptospirillum</jats:italic>`. * "the cache is what the tool produces today" -- re-fetched with the current dependency into a scratch directory: 24 newlines, joins 10 -> 4, words separated. The committed cache came from an older version. So the remedy for one cache is a re-fetch, not a waiver. Done for doi_10.1128_aem.69.8.4853-4865.2003; validate-references on the citing record is clean (exit 0, 0 errors), and the Tinto entry is REMOVED from _CACHE_RUN_TOGETHER rather than left standing. The other five degraded caches were re-fetched too and are NOT changed: three returned empty content and two returned the same single-line text. Their degradation is in Crossref's stored abstract, so there is nothing here to fix. The parvus waiver stays for the same reason -- PMID:38516398 is not in that set. WHAT THE RE-FETCH REVEALED, and it means #690 is not finished: the upstream fetcher wrote its output as `DOI_*.md`. It normalises a reference id to `DOI:` and builds the cache path from that, so every cache miss `just validate-references` fills recreates the split that accumulated 133 files. I closed #690 saying the writer was fixed; scripts/cache_fulltext.py was, and this one is not ours. Filed separately with this evidence. The guard is here, and it fires earlier than the resolve check: no cache filename may use a prefix casing no citation resolves to. The resolve check only notices once a record cites the new file; this notices as soon as the file exists. It caught seven such files in the working tree -- created by that fetcher -- which were renamed, content verified unchanged by checksum. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
realmarcin
added a commit
that referenced
this pull request
Aug 28, 2026
…689) (#699) * test: empty _OWED_BOTH_ROOTS -- all eleven gates now sweep both roots (#689) The guard added in #692 recorded eleven test modules that swept kb/communities where data/isolates holds the same content. This converts all eleven and empties the list. Checked BEFORE converting, not hoped for afterwards: the truncation gate examined 66 isolate snippets and flagged 0, no isolate interaction participant sits outside its taxonomy, and the auditor reports 0 issues of any kind across the 4 records. So this adds coverage rather than a defect backlog -- the value is that a defect arriving in an isolate tomorrow is visible to eleven gates that could not have seen it. `communitymech.paths` gains `record_files()` and `record_path()`. `default_record_roots()` answers "which directories" and almost every caller then wrote the same glob line; eleven skipped that step entirely and globbed one root. One function, so a module cannot get half the corpus by writing slightly less code than the module beside it. Two of the eleven needed more than a directory. `test_ncbi_domain_scope` and `test_gtdb_near_tie_marker` iterate `document["taxonomy"]`, and a CommonTaxon in kb/taxa has no such key -- the trap `taxon_blocks.iter_taxon_descriptors` was written for in #656. They now use `taxon_descriptor_roots()` with that walker, and reach 16 isolate and 2 kb/taxa descriptors that were previously invisible; gtdb_classification blocks seen: 752 -> 755. `test_network_auditor` was the sharpest. It passed `communities_dir=` explicitly, silently replacing the auditor's own `default_record_roots()` default -- the shape #350 fixed IN the auditor and left standing in its test. The override is gone, so the default is under test as much as the corpus is, and an assertion now requires an isolate to be among the audited records. `test_snippet_rendering_artefacts` monkeypatched the COMMUNITIES constant to drive its mutation check over one record. Sweeping both roots makes the walk a call rather than a directory, so the seam moves to a `_record_files()` indirection; the mutation check still fires. The scanner guard is recalibrated against `_COMMUNITY_ONLY` instead of a written count. The population legitimately FELL from 14 to 3, so a fixed threshold would have had to be lowered -- and a threshold lowered to match reality measures nothing. What must stay true is that the scanner still sees the modules classified as single-root. Verified: 2831 passed, 16 skipped. Conversion proven non-vacuous by collection -- the 4 isolate records now appear as parametrised cases in test_snippet_truncation, which collected only kb/communities before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: the scanner was passing incomplete conversions (#689) Adversarial review of the previous commit. Four findings, all in my own work. 1. THE GUARD WAS WEAKER THAN IT LOOKED. `_sweeping_test_modules()` skipped any module mentioning `record_files`/`default_record_roots`, treating the presence of a NAME as proof of its USE. test_no_vacuous_go_annotations imported record_files, never called it -- its sweep read `(corpus or COMMUNITIES).glob(...)`, which the conversion's pattern did not match -- and ruff then deleted the now-unused import, leaving a module that looked converted, was not, and passed this scan twice over. Detection is now structural: a module that still globs a constant bound to kb/communities is flagged whatever else it says. A genuinely converted module cannot trip it, because it no longer has such a glob. 2. That stricter scan immediately surfaced two more, both previously hidden: test_gtdb_status_writer swept one root for curated GTDB pins, and test_ncbi_domain_scope had a SECOND sweep at line 134 that I missed while converting line 82 -- a module I had already claimed to convert. 3. test_interaction_participants_outside_taxonomy swept both roots but then constructed `NetworkIntegrityAuditor(COMMUNITIES)`, handing the auditor one root -- the same override I had just removed from test_network_auditor, one file away. Three modules had dead COMMUNITIES constants left behind. 4. The stricter scan then produced a FALSE positive: test_isolates_are_covered_by_id_checks was flagged for a docstring that DISCUSSES reverting a loop to `Path("kb/communities").glob("*.yaml")` -- it describes the defect it exists to prevent. The scan now strips docstrings and comments first; a guard that cannot tell code from commentary teaches people to reword their explanations. Reindenting the status-writer sweep also moved its `assert curation_note` outside the `if block.get("curated")`, so it briefly asserted a note on every descriptor and reported a curated pin in a record containing zero occurrences of `curated:`. Chased to the edit rather than "fixed" in the corpus, and mutation-checked afterwards: blanking the note in the pinned record fails the test, restoring it passes. 2833 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three gates that could not see what they claimed to check. Each commit is
independent and separately mutation-checked.
#690 — 133 caches named so only macOS could find them
references_cache/carried two conventions for one prefix — 133DOI_*and 79doi_*— while every reference writes lowercasedoi:(1096 occurrences, zerouppercase). The stem convention shared by this repo's scripts and by the
upstream validator produces
doi_…, so theDOI_*half was findable only on acase-insensitive filesystem. macOS is; Linux is not.
Not tidiness:
linkml_reference_validator's fetcher builds the same stem andcalls
.exists()(etl/reference_fetcher.py:204-225), so on Linux those werecache misses — and per
src/communitymech/paths.pya miss sends the fetcherto the network. 117 of 514 distinct references (23%) were in that state.
All 133 renamed.
git mv DOI_x.md doi_x.mdis a no-op undercore.ignorecase=true, so each went via a temporary name; verified afterwardsthat 0 tracked
DOI_*remain, 212doi_*exist (79 + 133), no.casetmpwasleft behind, and no stem collides in both cases.
tests/test_reference_cache_names_are_case_exact.pystops it returning. It usesos.listdirrather thanPath.is_file(), becauseis_file()on acase-insensitive filesystem answers this exact question with a "yes" Linux would
not give — the check has to be able to fail on the machine running it.
#686 — the KGX gate ran on nothing, hiding three real mismatches
kgx_nodesglobsoutput/kgx/nodes.tsv;output/kgx/is gitignored with zerotracked files, and no job built the export before
just validate-products. Sothe glob matched nothing, the target skipped, and the gate printed "All id↔label
pairs correspond" having read nothing.
Three genuine MISMATCHes were behind that skip:
They are the same
+ cationtypo already waived once forCHEBI:16793("mercury(2+) cation") in a hand-written metal→CHEBI map. The waiver called it
an exporter-side design choice. It was a typo, and it had been covering for
three more of its own kind. All four are fixed at the source and the waiver is
deleted rather than multiplied — mercury now passes as
OK_CANONICAL, whichalso confirms the canonical form the waiver asserted. Exceptions 13 → 12,
errors 0.
The workflow now runs
just kgx-exportbefore bothreport-label-driftandvalidate-products, and the target isrequired: trueso a missing artifact isa
MISSING_GLOBerror rather than a skip.tests/test_generated_gate_inputs_are_built_in_ci.pyholds both halvestogether — either alone restores the hole.
output/kgx/**leaves thepaths:filter: a gitignored directory can never puta file in a PR, so it advertised a trigger that could not fire.
test_ci_triggers_cover_what_the_gates_read.pywas requiring that — right forcommitted data, meaningless for generated data — so generated roots are exempted
there and carry the build-before-read obligation in the new module instead.
#689 — a test's record-root scope is now a decision
test_no_constructor_hardcodes_the_old_rootscanssrc/communitymech/**, butmany gates here are tests, so one globbing
kb/communitiesalone wasinvisible to it. That is how the #529 discriminator swept one root for months.
Only modules that sweep are classified — naming one record as a fixture is not
a scope decision. 14 sweep. The split is measured, not assumed:
data/isolatesholds 4 records with 66 snippets, 3
ecological_interactions, 3gtdb_classificationblocks, 0cultivation_setup, 0go_terms, and none isrendered into
docs/(324 pages for 324 community records)._COMMUNITY_ONLY(3) — scoped by argument or by following the rendered pages._OWED_BOTH_ROOTS(11) — each with what is being missed. Two miss nothingtoday only because isolates carry no
cultivation_setuporgo_termsyet, andwould not notice the first that does.
test_network_auditorpassescommunities_dir=explicitly, overriding the auditor's shared default — theshape data/isolates/** now triggers validate-strict, but that job does not validate isolates #350 fixed in the auditor and left in its test.
Emptying
_OWED_BOTH_ROOTSis follow-up; this makes the 12th addition a choicesomeone defends.
Verification
Every new assertion mutation-checked, each mutation confirmed applied before
a red was trusted: rename a cache back to
DOI_*; droprequired: true; movethe build after the gate; delete the build step; remove an
_OWEDentry; planta nonexistent module.
Known unrelated failure:
test_no_snippet_stops_mid_wordfails locally ondata/isolates/Methylobacterium_REE_Ewaste_Platform.yaml, driven by anuntracked local cache. Untouched here; skips on a clean checkout.
Adversarial review (commits 4–5)
The rename was only half the fix
scripts/cache_fulltext.py::_doi_cache_pathpreferredDOI_and fell backto
doi_last, so a DOI with no cache yet was created uppercase —unreachable the moment anything read it back. That is how the 133 accumulated,
and renaming them without this would have refilled the set on the next fetch.
Canonical is now lowercase; uppercase is still read, so an old local file is
found rather than silently re-fetched, but it is never the name a new file gets.
A false red, from stale bytecode — #693
The test pinning that fix first loaded the script with
spec_from_file_location.scripts/is not a package, so that writesscripts/__pycache__, which Python validates on (mtime, size) — anddoi_→DOI_changes neither. A mutation and its restore within the same secondwere indistinguishable to the loader, which served a stale module and
produced a failure with correct source on disk.
The direction that bit was harmless. The same mechanism produces the opposite: a
mutation check going green from the stale mutated module, certifying a test
that cannot fail. The test now compiles the source text on every run and creates
no
__pycache__. Re-verified: the mutation fails, and the restore returns togreen immediately rather than one run later. Filed as #693 — 36 test
modules use that loading pattern.
The legacy-read assertion was also platform-dependent (path equality holds on
Linux, not on macOS where the lowercase candidate already
.exists()). It nowasserts the returned path exists and holds the right content.
The rename exposed a gate CI could never run — #694
CI failed
test_no_snippet_stops_mid_wordon this branch, caused by thisbranch.
_cachedglobbed{key}*then{key.upper()}*; the second could nevermatch, because it upper-cases the whole stem (
doi_10.1128_aem…→DOI_10.1128_AEM…) when only the prefix was ever uppercase. So on Linux thereference resolved to nothing and the record was skipped, while on macOS the
filesystem found it and the check ran — the gate reported on a different
corpus depending on where it ran.
Six findings in
Tinto_River_Iron_Cycling_Community.yamlsat on the wrong sideof that split. They are cache artefacts, not bad quotes: the abstract's italic
taxon names lost their surrounding spaces when fetched, so
"...related to Leptospirillum spp., Acidithiobacillus..."is cached as"...related toLeptospirillumspp.,Acidithiobacillus...". The snippet ends at a real wordboundary in the article; only the cache disagrees. Waived in
_CACHE_RUN_TOGETHERwith the reasoning recorded, mutation-checked (removingthe entry flags all six again). The dead
key.upper()pattern is deleted ratherthan repaired.
Also extended:
test_generated_gate_inputs_are_built_in_cinamed one workflow,so a second workflow running the gate without building would reopen #686 with
every assertion still green. It now checks every workflow.
Final CI
all SUCCESS.
label-correspondenceis the meaningful one: it is the first run inwhich that job actually built and read the KGX export.