Skip to content

Make #369's one-off drift audit a standing invariant - #456

Merged
realmarcin merged 4 commits into
mainfrom
gtdb-release-drift-369
Aug 7, 2026
Merged

Make #369's one-off drift audit a standing invariant#456
realmarcin merged 4 commits into
mainfrom
gtdb-release-drift-369

Conversation

@realmarcin

Copy link
Copy Markdown
Contributor

Closes #369.

Re-audit: both cases are gone

#369 audited all 366 grounded ids against the tool and found two disagreements. Re-running that audit today:

#369's case now
NCBITaxon:59732s__Chryseobacterium_indologenes, tool said g__Chryseobacterium no longer disagrees
NCBITaxon:340177g__Chlorobium, tool said nothing still disagrees, but carries curated: true — a curator saying so, not an unnoticed gap

So there is nothing to fix in the KB, and this PR changes none of it.

What the audit was really testing

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 — the tool didn't make it and no curator signed it — which is exactly the state #294's status enum and #384's pin exist to prevent.

Current measurement: 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).

Three tests

Verified by mutation: stripping curated: true from one pin makes the invariant fail.

Limitation, stated plainly

Needs the kg-microbe crosswalk, so it skips where that is absent — CI included, the same limitation as its neighbours (test_gtdb_support_counts and friends). It earns its place anyway: the audit it replaces was run by hand once, and the KB has changed under it twice since.

just qc green.

🤖 Generated with Claude Code

realmarcin and others added 2 commits August 6, 2026 07:49
#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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
@realmarcin

Copy link
Copy Markdown
Contributor Author

Review round 1 — verdict: ship, one fix applied

The reviewer reproduced the audit independently — its own positional walk mirroring apply_to_community, its own want-set construction, no reuse of my fixture — and got the same numbers: 727 blocks, 378 distinct taxa, 11 disagreements, all 11 curated, all 11 carrying a note. Zero uncurated mismatches, so the PR's central claim holds. Scope confirmed complete: all 727 blocks live under kb/communities; data/isolates and kb/taxa have none.

Fixed — the one thing worth fixing

test_every_curated_block_says_why took the crosswalk-backed audit fixture and never referenced it. Its assertion is pure YAML, so that vestigial argument was the only thing making the one check here that can gate in CI skip there. Now dropped — verified it passes with KG_MICROBE_DIR pointed at nothing.

That also removed the file filter derived 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 was itself the gap.

Two smaller ones: the failure message told the reader to run --refresh --apply, which is a guaranteed no-op when the tool returns None because it resolved the taxon as ambiguous (apply_to_community skips those) — it now names --withdraw-ambiguous. And the fixture now records what it deliberately does not replicate: apply_to_community also honours the legacy CURATED_GROUNDINGS list, but per #384 the block flag is primary, so a grounding protected only by the list is exactly what this test should complain about.

Verified clean by the review

Want-set construction matches main() exactly, including both lookup_keys spellings and the denominator/exclude_unnamed defaults; building the want-sets as a whole-KB superset is safe post-#366 because collect_rows's seen guard makes by_higher[k] invariant to what else was requested. The AMBIGUOUS mapping (now = found.get("gtdb_id") → None) is correct per the invariant and cannot false-pass, since no block lacks a gtdb_id. Both vacuity traps close: a resolver returning always-None fails the invariant (716 unexplained), always-the-stored-value fails the guard.

just qc green · 1567 passed, 16 skipped.

@realmarcin realmarcin closed this Aug 6, 2026
@realmarcin realmarcin reopened this Aug 6, 2026
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 <noreply@anthropic.com>
@realmarcin

Copy link
Copy Markdown
Contributor Author

Blocked: CI has not run on the final commit

validate-strict, lint and vendored-sync all passed on 71aaa26, the first commit. The review-fix commit 9cb6a4f produced no workflow run, and neither did closing/reopening the PR nor an empty commit (a52cad5). The diff is test-only and tests/**/*.py is in the trigger paths, so it should have fired.

It is not this PR: GitHub Actions has produced no run on this repository since 14:50 UTC, roughly four hours. All eight workflows report active. That looks like an outage or a spending/quota limit rather than anything in the diff.

Local state, on the exact tree (a52cad5)

Diff from the CI-green commit is one test file, +23/-12.

black --check src/ tests/ scripts/   168 files would be left unchanged
ruff check  src/ tests/ scripts/     All checks passed!
mypy src/                            Success: no issues found in 47 source files
pytest tests/                        1567 passed, 16 skipped, 7 deselected

Leaving this open rather than merging. Local gates being a superset of the CI jobs is an argument, not a substitute for the gate, and waiving it is not a call to make unattended. Ready to merge as soon as Actions returns and a run goes green.

@realmarcin

Copy link
Copy Markdown
Contributor Author

Cause identified: a GitHub Actions platform outage

Not this repo, not this PR. GitHub's own status API:

$ curl -s https://www.githubstatus.com/api/v2/components.json
  Webhooks: operational
  Actions:  major_outage

Repo and org Actions settings are both {"enabled": true, "allowed_actions": "all"}, so it is not a permissions or quota problem either. No workflow has run on this repository since 14:50 UTC (now past 20:00).

Nothing to do here but wait. This PR is ready: just qc green locally — lint, the full suite, and every offline validator, which between them are a superset of the lint and validate-strict jobs. It should merge on a green run once Actions returns.

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 <noreply@anthropic.com>
@realmarcin
realmarcin merged commit 04e842e into main Aug 7, 2026
3 checks passed
@realmarcin
realmarcin deleted the gtdb-release-drift-369 branch August 7, 2026 03:01
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.

Two stored GTDB blocks disagree with the current tool, from mapping-release drift rather than the #366 bug

1 participant