Skip to content

Detect the typing inconsistency instead of guessing at seven records (#352) - #353

Merged
realmarcin merged 3 commits into
mainfrom
fix/352-disposition-typing
Aug 7, 2026
Merged

Detect the typing inconsistency instead of guessing at seven records (#352)#353
realmarcin merged 3 commits into
mainfrom
fix/352-disposition-typing

Conversation

@realmarcin

Copy link
Copy Markdown
Contributor

#352 asks for a sweep. Doing it by hand first showed why a sweep is the wrong
deliverable.

Every candidate CAPACITY node turns out to duplicate its own record's trait
salt_tolerance beside METPO:1000625, low_ph_tolerance beside METPO:1000232,
growth_at_4c beside METPO:1000618. Retyping them to TRAIT without merging would
recreate precisely the ungrounded-duplicate-anchor problem #351's review caught. That's
seven modelling decisions, not a mechanical pass.

So: two checks, in the audit that already has the ratchet machinery

defect what it catches
DUPLICATE_GROUNDING two nodes in one graph sharing a grounding — the machine-readable signature of one concept modelled twice
DISPOSITION_MISTYPED a CAPACITY/STATE node whose own description reads as a disposition ("capacity to", "ability to", "tolerance of") — which is a TRAIT

Both found more than my hand sweep did

Which is the argument for building them rather than doing one pass:

  • DUPLICATE_GROUNDING found catalase/catalase_function sharing GO:0004096 and
    urease/urease_function sharing GO:0009039. Neither was known — both are a
    protein node and a function node modelling one thing. Only the ph_delta_high case from
    Retype six mis-typed dispositions, and route six real states (#334) #351 was on anyone's list.
  • DISPOSITION_MISTYPED found 8 where I'd found 6 — including oxygen_tolerance
    (my pattern wanted "capacity to"; the record says "Capacity of a cell to") and
    elongation_capacity in sphere_shaped.yaml.

Anchored on the description, not the label, on purpose: the label is usually just the
concept name (buoyancy) while the description is where the disposition shows.

What was checked

  • All 11 baselined — each needs a per-record decision, so this makes the class visible
    and non-growing rather than guessing.
  • Canaried: a new CAPACITY node described as a disposition exits 1.
  • Six tests, including that distinct groundings don't flag, that absent groundings don't
    collapse onto each other as '', and that a genuine state ("Transmembrane proton
    gradient…"
    ) is not flagged.
  • just qc green · 478 tests pass · ruff clean.

Addresses the detection half of #352; the retyping/merging burn-down stays open.

🤖 Generated with Claude Code

…352)

#352 asks for a sweep. Doing it by hand first showed why a sweep is the
wrong deliverable: each candidate CAPACITY node turned out to duplicate
its OWN record's trait -- salt_tolerance beside METPO:1000625,
low_ph_tolerance beside METPO:1000232, growth_at_4c beside METPO:1000618
-- so retyping them to TRAIT without merging would recreate exactly the
ungrounded-duplicate-anchor problem #351's review caught. That is seven
modelling decisions, not a mechanical pass.

So this adds the two checks that make the class visible and non-growing,
in the audit that already has the ratchet machinery:

  DUPLICATE_GROUNDING   two nodes in one graph with the same grounding --
                        the machine-readable signature of one concept
                        modelled twice
  DISPOSITION_MISTYPED  a CAPACITY or STATE node whose own DESCRIPTION
                        reads as a disposition ("capacity to", "ability
                        to", "tolerance of"), which is a TRAIT

Both found more than my hand sweep did, which is the argument for
building them rather than doing one pass:

- DUPLICATE_GROUNDING found catalase/catalase_function sharing GO:0004096
  and urease/urease_function sharing GO:0009039. Neither was known; both
  are a protein node and a function node modelling one thing. Only the
  ph_delta_high case from #351 was on anyone's list.
- DISPOSITION_MISTYPED found 8 where I had found 6, including
  oxygen_tolerance -- my pattern wanted "capacity to" and the record says
  "Capacity OF A CELL to" -- and elongation_capacity in sphere_shaped.

Anchored on the DESCRIPTION rather than the label on purpose: the label
is usually just the concept name ("buoyancy"), while the description is
where the disposition shows.

All 11 baselined, since each needs a per-record decision. Canaried: a new
CAPACITY node described as a disposition exits 1.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@culturebot-reviewer

culturebot-reviewer Bot commented Aug 7, 2026

Copy link
Copy Markdown

Claude finished @realmarcin's task in 4m 21s —— View job


Review: PR #353

Verified CI (qc, pytest, pr-sanity, vendored-sync all green), reproduced the 11 findings against the corpus by hand, and confirmed all 8 DISPOSITION_MISTYPED hits are true positives — every one has a genuine disposition in its own description. No data/traits/** change, so no history/ record is owed. Two issues in the detection code.


🟡 scripts/audit_causal_graphs.py:262 — the check reads the label, but says it didn't.

blob = f"{n.get('label') or ''} {n.get('description') or ''}"

The comment at line 149 says the regex is "deliberately anchored on the phrasing a curator writes, not on the label", the PR body repeats it, and the finding text hard-codes description reads as a disposition. The implementation scans both, concatenated. Two consequences:

  • A label-only match emits a finding that names the wrong field. For a class the PR says needs "a per-record decision," the curator opens the record, reads the description, and finds nothing there.
  • Concatenation matches across the field boundary. capacit(y|ies)\b[^.]{0,30}?\bto will fire on a label ending in …capacity followed by a description whose first to lands inside 30 chars — e.g. label energy charge capacity + description ATP pool relative to total adenylate yields capacity ATP pool relative to. That is a schema-correct reservoir-of-X CAPACITY (traitmech.yaml:520) reported as mistyped.

Neither behaviour is covered: test_a_capacity_described_as_a_disposition_is_flagged asserts "salt_tolerance" not in detail on a node whose id is cap, which is vacuous, and no test exercises a label-only match. Scan description alone as documented, or scan the two fields separately and name the matching field in the detail.

🟡 scripts/audit_causal_graphs.py:253-256DUPLICATE_GROUNDING doesn't ratchet on growth.

_key takes detail.split(" ")[0] as the discriminator, so the key here is grounding=GO:0004096. A third node joining that same grounding leaves the key unchanged and the ratchet silent.

That is the identical fail-open the comment 20 lines below at :275 documents and fixes for FRAGMENTED_GRAPH"Detail MUST lead with the component count… 3 components -> 4 keeps the node count, so real backsliding stayed suppressed." on 2 nodeson 3 nodes is the same shape. Leading with the count (count=3 grounding=GO:0004096 …) restores it; worth a test that 2→3 un-suppresses, mirroring test_ratchet_stays_quiet_when_only_a_node_was_added.


🔵 The heuristic's precision leans on an accident. data/traits/environment/ph_optimum.yaml:77"Capacity of cytoplasmic buffers (e.g. inorganic/organic phosphates) to absorb pH fluctuations" — is a legitimate reservoir CAPACITY and correctly escapes, but only because [^.] is stopped by the dots in "e.g." and the span exceeds 30 chars. Drop the parenthetical in an unrelated cleanup and it flags, which under the default --fail-on new fails CI with a wrong diagnosis.

🔵 QUALITY isn't scanned, and data/traits/physiology/carboxydotrophic.yaml:80-83 carries the same phrasing (oxygen_tolerance, QUALITY, "Ability of an enzyme to function in the presence of O2") while the same node_id typed CAPACITY in oxygen_preference.yaml is flagged. Excluding it looks right — it's a property of an enzyme, not an organism trait — but say so in the docstring.

🔵 Both new blocks land between the FRAGMENTED_GRAPH rationale comment (:239-243, ending "skips unreferenced nodes above") and the _components call at :270 that it explains. Reads as if that paragraph documents DUPLICATE_GROUNDING. Move them above it.


Note, not a defect: the baseline's -29 is mostly a full regeneration — 14 rows stale since #351/#334 (buoyancy, gas_vesicle*, the pressure_optimum island, near_neutral_cytoplasmic_ph) drop out and several UNREACHABLE_FROM_TRAIT details re-anchor (metal_tolerant_traitmetal_tolerant_trait/arsenic_tolerance). That tightens the ratchet rather than loosening it, and the new baseline is now exactly the report with zero ERROR rows. Worth a line in the PR body so -29 isn't read as relaxed suppression.

The framing is right — the two checks did find catalase/catalase_function and urease/urease_function, which a hand sweep would not have. Fix the label/description mismatch and the discriminator and this is good.
· fix/352-disposition-typing

@culturebot-reviewer culturebot-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues in the detection code; details in the review comment.

  • The DISPOSITION_MISTYPED check scans label+description concatenated (scripts/audit_causal_graphs.py:262), contradicting both the comment at :149 and the finding text 'description reads as a disposition', and creating cross-field phantom matches.
  • DUPLICATE_GROUNDING's detail leads with 'grounding=...', so _key's leading-token discriminator makes 2->3 nodes sharing a grounding invisible to the ratchet -- the same fail-open the FRAGMENTED_GRAPH comment at :275 documents and fixes.

All 8 DISPOSITION_MISTYPED hits verified as true positives, and the baseline regeneration tightens rather than relaxes the ratchet.

…#353 review)

Four findings, all correct.

The docstring said the check reads the description "rather than the
label", and the code read both. Now description-only, which is what the
rationale actually argues for -- a label is usually just the concept name.

DUPLICATE_GROUNDING did not ratchet on growth. _key takes the leading
whitespace-delimited token of detail as the baseline discriminator, and
the detail led with the CURIE, so a THIRD node joining the same grounding
kept the key stable and baselining two would have silently forgiven
three. Now leads with the node count, which is exactly the reasoning
FRAGMENTED_GRAPH records a few lines below and which I had read without
applying. A test pins that two and three nodes key differently.

The pattern's precision was accidental. ph_optimum.yaml's "Capacity of
cytoplasmic buffers (e.g. ...) to absorb pH fluctuations" is a genuine
reservoir CAPACITY and escaped only because the dots in "e.g." stopped
the character class -- a right answer for a reason an unrelated cleanup
would erase, and under --fail-on new that is CI failing with a wrong
diagnosis. The capacity/ability arms now require the capacity to be
ORGANISM-scoped: bare, or of a cell/organism/bacterium/microbe. Verified
it still excludes the reservoir case WITH THE PARENTHETICAL REMOVED, and
still catches all eight real ones.

Documented why QUALITY is out of scope: carboxydotrophic's
oxygen_tolerance is "Ability of an ENZYME to function in the presence of
O2", a property of a protein rather than something an organism can do,
while the same node_id typed CAPACITY in oxygen_preference IS
organism-scoped and is flagged. Widening would need a distinction this
heuristic does not make.

Both blocks moved above the FRAGMENTED_GRAPH rationale comment, which
they had been inserted into the middle of.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@realmarcin

Copy link
Copy Markdown
Contributor Author

All four correct. Two were me not applying reasoning that was already written a few lines away.

🟡 Reading the label while claiming not to

Fixed — description-only, which is what the rationale actually argues for.

🟡 DUPLICATE_GROUNDING not ratcheting on growth

This one stings: _key takes the leading token of detail, my detail led with the CURIE, so a third node joining the same grounding kept the key stable — baselining two would have silently forgiven three. That is precisely the reasoning FRAGMENTED_GRAPH records a few lines below, which I read while writing this and didn't apply.

Detail now leads with the node count. A test pins that two and three nodes key differently.

🔵 Precision leaning on an accident

The sharpest of the four. ph_optimum's "Capacity of cytoplasmic buffers (e.g. …) to absorb pH fluctuations" escaped only because the dots in "e.g." stopped the character class — a right answer for a reason an unrelated cleanup would erase, and under --fail-on new that's CI failing with a wrong diagnosis.

The capacity/ability arms now require the capacity to be organism-scoped (bare, or of a cell/organism/bacterium/microbe). Verified it still excludes the reservoir case with the parenthetical removed, and still catches all eight real ones:

False  Capacity of cytoplasmic buffers to absorb pH fluctuations   <- no parenthetical
True   Capacity to grow and survive under elevated salinity
True   Capacity of a cell to survive exposure to molecular oxygen
False  Transmembrane proton gradient generated by light-driven pumping

🔵 QUALITY not scanned

Documented, with your reasoning: carboxydotrophic's oxygen_tolerance is "Ability of an enzyme to function…" — a property of a protein, not something an organism can do — while the same node_id typed CAPACITY in oxygen_preference is organism-scoped and is flagged. Widening would need a distinction this heuristic doesn't make.

🔵 Block placement

Moved above the FRAGMENTED_GRAPH rationale, which I'd inserted them into the middle of.

just qc green · 481 tests pass · ruff clean · findings unchanged at 8 + 3.

@culturebot-reviewer

culturebot-reviewer Bot commented Aug 7, 2026

Copy link
Copy Markdown

Claude finished @realmarcin's task in 2m 58s —— View job


Review: PR #353 (re-review of 6b95a19)

  • Read the follow-up commit
  • Verify the two 🟡 fixes
  • Verify the two 🔵 responses
  • Re-check findings, baseline, and CI

Both 🟡 are fixed and pinned by tests. CI green (qc, pytest, pr-sanity, vendored-sync); findings unchanged at 8 + 3. One new issue, introduced by the second fix.


🟡 scripts/audit_causal_graphs.py:265 — leading with the count closed one fail-open and opened its mirror.

"detail": (f"nodes={len(ids)} share grounding={g}: " ...

_key now discriminates on nodes=2. That correctly re-keys 2→3, which was the ask. But unlike FRAGMENTED_GRAPH — where the count is unique, one component-count per graph — DUPLICATE_GROUNDING can raise several findings per graph, and every pair-shaped one keys identically. Two distinct duplicated groundings in one graph collapse onto a single baseline key, so baselining one silently suppresses the other.

That is verbatim the failure _key's own docstring at :331 warns about: "Falling back to '' here would collapse every DANGLING_EDGE in a graph onto one key, so baselining one would suppress the rest."

Not currently triggered — the three findings sit in three separate files — but it is reachable from today's baseline without any new file: a second duplicated grounding appearing in the already-baselined ph_delta_high.yaml keys nodes=2, matches, and never surfaces. Enzyme-pair records are exactly where this shows up; catalase/catalase_function and urease/urease_function are the same shape, and a record carrying both enzymes would report one and hide one.

Both properties are available at once — put the count and the CURIE in the leading whitespace-delimited token:

"detail": (f"nodes={len(ids)}:{g} share grounding: " ...

nodes=2:GO:0004096nodes=3:GO:0004096 (ratchets on growth) and ≠ nodes=2:GO:0009039 (distinct groundings stay distinct). test_a_third_node_joining_a_grounding_re_keys still passes; worth a sibling asserting two different groundings in one graph key differently. Fix this → (baseline + report need regenerating with it.)


The other three are properly resolved, and I traced them rather than taking the commit message for it:

  • Label/description:272 reads n.get('description') alone; the finding text is now true. test_the_label_is_not_scanned puts tolerance of salt in the label with an innocuous description and asserts nothing fires, which is the non-vacuous version of the test I called out.
  • Organism-scoping — the new capacit(y|ies)(\s+of\s+(a|an|the)?\s*(cell|organism|bacteri\w+|microbe)s?)?\s+to no longer depends on the dots in "e.g.". Traced by hand: Capacity of cytoplasmic buffers to absorb pH fluctuations fails the optional scope group, so \s+to has to follow capacity directly, hits of, and doesn't match — with the parenthetical gone, which was the whole point. Capacity to grow… and Capacity of a cell to… still match; the a/an ordering backtracks correctly on "of an organism".
  • QUALITY scope and block placement — both done; the FRAGMENTED_GRAPH rationale paragraph now sits with the _components call it explains.

🔵 The scope nouns are cell|organism|bacteri*|microbe. A curator writing "Capacity of the strain to…" or "Ability of the species to…" — both ordinary in this corpus — is now missed. Precision is the right thing to prioritise for a --fail-on new gate, so this is a fine trade, but strain|species|isolate|archae\w+ are cheap additions if you want them.


Fix the discriminator and this is ready. The two checks are earning their keep — catalase/catalase_function and urease/urease_function were not on anyone's list.
· fix/352-disposition-typing

@culturebot-reviewer culturebot-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both 🟡 from the last round are fixed and pinned by non-vacuous tests, and I traced the regex fix by hand rather than trusting the commit message — the reservoir case in ph_optimum.yaml now escapes for a principled reason (organism-scoping) rather than an accident of "e.g." punctuation, verified with the parenthetical removed.

One new 🟡, introduced by the DUPLICATE_GROUNDING fix: leading the detail with nodes=N closed the 2→3 fail-open but opened its mirror. Unlike FRAGMENTED_GRAPH, where the component count is unique per graph, DUPLICATE_GROUNDING can raise several findings in one graph — and every pair-shaped one now keys as nodes=2, so two distinct duplicated groundings in one graph collapse onto a single baseline key and baselining one silently suppresses the other. That is the exact failure _key's own docstring warns about for DANGLING_EDGE. Not triggered today (the three findings are in three files), but reachable from the current baseline without any new file.

Both properties are available at once: lead with a single token carrying the count and the CURIE — f"nodes={len(ids)}:{g} share grounding: ...". Details in the comment.

…und 2)

Leading the detail with the count closed one fail-open and opened its
mirror. _key takes the leading whitespace-delimited token, so with
`nodes=2` in front, two DIFFERENT groundings each duplicated across 2
nodes in the same graph collapse onto one key -- freezing one silently
forgives the other. The previous round had the same bug pointing the
other way. Only one part varying is not enough; both are now in the
token: `nodes=2;grounding=METPO:1000478`.

A test writes two 2-node groundings in one graph and asserts two distinct
keys. Confirmed it fails (1 == 2) against the count-only form and passes
here, so it pins the fix rather than restating it.

Widened the disposition scope nouns with strain|species|isolate|archae*,
which the review offered as cheap and are ordinary in this corpus.
Findings unchanged at 8 + 3.

Baseline and report regenerated: the discriminator changed, so the three
DUPLICATE_GROUNDING keys are rewritten. Same 1532 findings.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@realmarcin

Copy link
Copy Markdown
Contributor Author

Right, and it's the same bug pointing the other way — I closed one fail-open by opening its mirror.

_key takes the leading whitespace-delimited token. With nodes=2 in front, two different groundings each duplicated across 2 nodes in the same graph collapse onto one key, so freezing one silently forgives the other. One part varying was never enough; both are now in the token:

nodes=2;grounding=METPO:1000478 (a, b)

A test writes two 2-node groundings into one graph and asserts two distinct keys. It fails against the count-only form (assert 1 == 2) and passes here, so it pins the fix rather than restating it.

Worth recording how close this came to shipping green: when I checked the fix I hit a stale-.pyc trap. My count-only variant swapped ; for a space, so the file kept its exact byte length, and with a same-second mtime Python reused the cached bytecode — the test "passed" against code that wasn't running. Cleared the cache and re-ran before believing it.

🔵 Scope nouns

Taken — added strain|species|isolate|archae\w+:

True   Capacity of the strain to tolerate acid
True   Ability of the species to grow at 4C
True   Capacity of an isolate to survive
False  Capacity of cytoplasmic buffers to absorb pH fluctuations

Findings unchanged at 8 + 3. Baseline and report regenerated — the discriminator changed, so the three DUPLICATE_GROUNDING keys are rewritten; same 1532 total.

just qc green · 482 tests pass · ruff clean.

@culturebot-reviewer

culturebot-reviewer Bot commented Aug 7, 2026

Copy link
Copy Markdown

Claude finished @realmarcin's task in 4m 7s —— View job


Review: PR #353 (re-review of 8108d75)

  • Read the new commit
  • Verify the DUPLICATE_GROUNDING discriminator fix and its test
  • Verify the scope-noun addition against the corpus
  • Re-check findings, baseline/report sync, and CI

The round-2 🟡 is fixed, and the fix is the right shape. Approving.


Discriminator. nodes=2;grounding=METPO:1000478 (a, b)_key's detail.split(" ", 1)[0] now yields nodes=2;grounding=METPO:1000478, so both parts vary in the token. 2→3 re-keys; two different groundings in one graph stay distinct. test_two_groundings_of_equal_size_do_not_collide is non-vacuous — it asserts len(dupes) == 2 and len({_key(f)}) == 2, and the count-only form gives 1 != 2. It pins the fix rather than restating it.

Verified independently rather than from the commit message:

  • just audit-graphsfindings: 1532 (baselined: 1532, new: 0, blocking: 0), DISPOSITION_MISTYPED 8, DUPLICATE_GROUNDING 3. Zero ERROR rows, and the report blob is byte-identical to the baseline (2534801e), which is what "the report with zero ERRORs" should look like. qc's staleness check (justfile:561) is green, so the committed report is what the current code produces.
  • 1521 → 1532 is exactly +11, and the baseline's 1535 → 1532 is 14 stale rows out, 11 new in — the arithmetic the earlier note claimed, confirmed.
  • The scope-noun widening is a no-op on today's corpus (still 8), so it's defensive, not a behaviour change smuggled in with a fix.
  • No false negatives introduced by the tightening. I checked every corpus description matching the pattern and confirmed the near-misses are all correctly out of scope by node_type, not by luck: spore_germination_trait is already TRAIT, protein_stability is MOLECULAR_FUNCTION, pathogenicity is BIOLOGICAL_PROCESS, and the euryhaline/nacl_range/metal_tolerant "tolerance of/to" hits are TRAIT nodes or edge descriptions. The only CAPACITY in the near-miss set is ph_optimum.yaml:77, which escapes because cytoplasmic buffers fails the scope group — the principled reason, independent of the parenthetical.

No data/traits/** change, so no history/ record is owed.


🔵 species is the one scope noun with a collision in this corpus: it appears 56 times chemically (reactive oxygen species). Capacity of reactive oxygen species to… is safe — the intervening words break the group — but Capacity of the species to… would match on a CAPACITY node describing a molecule, which is the enzyme case the docstring deliberately excludes. Zero hits today, so nothing to change; worth knowing which of the four additions is the load-bearing one if a false positive ever shows up.

🔵 A partial fix now blocks: reducing a 3-node duplicate to 2 re-keys off nodes=3;… and reports as new under --fail-on new. That is the same trade FRAGMENTED_GRAPH already makes at :317 for 4→3 components, so it's consistent and I'd leave it — just expect the re-baseline.

🔵 The docstring at :47 still says "today's 1541 findings"; it's 1532 now. Stale before this PR, but this PR is what moved the number.


Three rounds, and each one closed a fail-open that the previous fix opened in mirror image — CURIE-first forgave a third node, count-first forgave a sibling grounding, and only carrying both in the token closes it. The checks are earning their keep: catalase/catalase_function and urease/urease_function were not on anyone's list.

CI green — qc, pytest, pr-sanity, vendored-sync.
· fix/352-disposition-typing

@culturebot-reviewer culturebot-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The round-2 discriminator issue is fixed and the fix is the right shape: nodes=2;grounding=METPO:1000478 puts both the count and the CURIE in _key's leading token, so 2->3 re-keys and two distinct groundings in one graph stay on distinct keys. test_two_groundings_of_equal_size_do_not_collide is non-vacuous — it asserts two findings AND two distinct keys, which the count-only form fails at 1 != 2.

Verified independently rather than from the commit message: just audit-graphs reports 1532 findings, 0 new, 0 blocking, 8 DISPOSITION_MISTYPED + 3 DUPLICATE_GROUNDING, zero ERROR rows; the report blob is byte-identical to the baseline and qc's staleness check is green, so the committed report is what the current code produces. 1521 -> 1532 is exactly +11.

The scope-noun widening is a no-op on today's corpus, so it is defensive rather than a behaviour change. I also checked every corpus description matching the pattern for false negatives from the tightening: the near-misses are out of scope by node_type, not by luck (spore_germination_trait is already TRAIT, protein_stability is MOLECULAR_FUNCTION, pathogenicity is BIOLOGICAL_PROCESS, the tolerance-of/to hits are TRAIT nodes or edge descriptions). The only CAPACITY among them, ph_optimum.yaml:77, escapes because 'cytoplasmic buffers' fails the scope group — the principled reason, not the parenthetical.

No data/traits/** change, so no history/ record is owed.

Three suggestions in the comment, none blocking: 'species' is used chemically 56 times in this corpus and is the one scope noun with a plausible collision; a partial 3->2 fix will now re-key and report as new (the same trade FRAGMENTED_GRAPH already makes); and the docstring's '1541 findings' is now 1532.

@realmarcin
realmarcin merged commit 52a6726 into main Aug 7, 2026
5 checks passed
@realmarcin
realmarcin deleted the fix/352-disposition-typing branch August 7, 2026 23:14
realmarcin added a commit that referenced this pull request Aug 7, 2026
* Check that every workflow which SHOULD have run, ran (#348)

audit-pr-checks can only see TOTAL silence, and the reason is structural:
claude-code-review.yml fires on pull_request with no paths: filter and
records a run even when its if: gates skip the job, so nearly every PR
here has at least one qualifying event. qc, pytest and validate-strict
could all be mute and it would stay green. This is the stronger property
#348 asked for -- each PR-triggered workflow checked by name.

THE REQUIRED SET IS DERIVED, NOT DECLARED: every workflow in
.github/workflows with a pull_request: trigger, read from the files. #252
rejected a hand-maintained list for audit-qc-paths because a declaration
drifts the moment someone adds a workflow, and the argument applies here
unchanged. A test pins that adding a workflow file grows the set with no
second edit.

A paths: FILTER IS NOT A MISSING RUN. Five of the eight PR-triggered
workflows are filtered, so the filters are evaluated against the PR's own
changed files and only an unfiltered-or-matching workflow is expected.
That evaluation is the hard part, and also the payoff: a paths:
regression -- the class #184, #200, #250 and #252 all belong to -- surfaces
here as "expected, did not run".

Two things I got wrong while writing it, both pinned by tests:

PyYAML resolves the unquoted key `on:` to the BOOLEAN True under YAML 1.1,
so doc["on"] is absent in every workflow in this repo. Reading only "on"
would have found zero required workflows and exited 0 -- a check that
passes because it looked at nothing, the exact vacuous green it exists to
catch. Invisible without a test, so there is one asserting the trap itself.

The matcher anchored only the end, so `data/traits/**` would have matched
`vendor/data/traits/x.yaml`. Now fullmatch. GitHub's `*` also does not
cross a slash where fnmatch's does, which is why this is hand-rolled
rather than fnmatch.

Filter syntax the matcher does not implement ([], !, +, ?) is reported as
UNSUPPORTED and never guessed at, since the value of the check rests on
"expected, did not run" meaning something. PRs past GitHub's 300-file
path-filter evaluation limit are skipped AND NAMED, per #346's rule that a
PR vanishing from both counts makes the output read as coverage it did not
have.

Wired into pr-checks-present.yaml with if: always(), so a total silence
does not suppress the partial-silence report -- they answer different
questions.

CANARY (live, against this repo, before wiring it into CI). 8 workflows
parsed; PR #353's 4 changed files predicted 5 expected workflows, all 5
matched against real API data, and curation-history, label-correspondence
and validate-strict were correctly filtered out. Negative control: deleting
qc's run from the fetched data reports exactly ['.github/workflows/qc.yaml'],
so the green was not vacuous. Not exercised: a PR with >300 files, and
unsupported filter syntax, both of which exist only in tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Fetch at the PR head, survive pagination, decline branch filters (#354 review)

All three correct, and the first two were reachable today.

PAGINATION. `gh api --paginate -q` applies the jq filter to EACH PAGE and
concatenates, so past GitHub's default per_page=30 json.loads raises
"Extra data" -- taking down collect() for every OTHER open PR, and making
the >300-file skip branch unreachable because the fetch died at 31.
Reproduced live against PR #311 (346 changed files):

  JSONDecodeError: Extra data: line 2 column 1 (char 7050)

gh 2.97 refuses --slurp together with --jq, so the fix is --slurp plus
extraction in Python. Same PR now returns 346 filenames.

HEAD vs MAIN. GitHub dispatches pull_request events from the workflow
files AT THE PR HEAD; this process has whatever it was checked out at,
which for pr-checks-present.yaml is main. They disagree exactly when a PR
touches .github/workflows -- and every filtered workflow here lists its
own file in its paths:, so a PR DELETING one matches the filter and main's
copy expects a run GitHub correctly never made. A false "expected, did not
run" is what makes this check stop meaning anything, so collect() now
fetches the head's copy per PR. A fetch failure is a REFUSAL: the PR is
skipped and named rather than judged against the wrong ref, because
falling back to main would silently reintroduce the bug.

Writing that test found a bug the review did not: `pr.get("workflows") or
workflows` falls back when the head list is EMPTY, so a PR deleting all
the workflows would have every one of them reported missing. Now keyed on
`in`, not truthiness.

BRANCH FILTERS. `branches:`/`branches-ignore:` restrict which base a PR
must target and this audit never reads the base, so unmodelled they would
make every PR against another base a false offender. Routed through the
existing UNSUPPORTED escape hatch. `types:` gets a sharper rule rather
than a blanket refusal: every PR head arrives via `opened` (the first) or
`synchronize` (any later push), so a types: containing both is
predictable and one that does not is declined -- which keeps
claude-code-review.yml's `types: [opened, synchronize]` in the set.

Also: an unparseable workflow is now NAMED rather than silently dropped
(it would shrink the required set invisibly), and _glob_to_regex is
lru_cached.

CANARY, live, after the rewrite. 2 open PRs, 8 workflows fetched AT EACH
HEAD, 5 expected and 5 matched on both. Pagination exercised for real on
PR #311's 346 files. Negative control: deleting pytest's run from the
fetched data reports exactly ['.github/workflows/pytest.yaml'].
Still not exercised live: the >300-file skip and unsupported syntax,
both tests-only.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
realmarcin added a commit that referenced this pull request Aug 8, 2026
… retypes (#352) (#360)

* Burn down the 11 disposition-typing findings, half of which were not retypes (#352)

#353 shipped the detection and baselined what it found. This is the
burn-down: DISPOSITION_MISTYPED 8 -> 0, DUPLICATE_GROUNDING 3 -> 0.

ONLY FOUR OF THE EIGHT WERE MISTYPES. #352 framed the fix as "sweep
CAPACITY nodes matching the disposition pattern and retype them", and for
four nodes that is exactly right - salt tolerance, salt-tolerance breadth,
oxygen tolerance and low-pH tolerance are dispositions, now TRAIT and
grounded METPO:1000622 / 1000622 / 1000609 / 1003008.

For the other four, retyping would have been wrong, and the thing that
exposes it is the grounding. Every TRAIT node in the corpus is grounded,
and the only correct grounding for each of those four is the term its OWN
record already carries. Grounding them that way trades a
DISPOSITION_MISTYPED for a DUPLICATE_GROUNDING and calls it progress. What
it actually means is the node RESTATES its anchor:

- ph_homeostasis_capacity "Capacity to balance and maintain cytoplasmic pH"
  against cytoplasmic_ph_homeostasis, IN THE SAME GRAPH, already
  BIOLOGICAL_PROCESS and grounded GO:0051453.
- elongation_capacity "Capacity of a cell to elongate into a rod via
  sidewall growth" against lateral_elongation "Sidewall growth mode that
  lengthens rods" - both already carried `reduced in -> sphere_shaped_trait`.
- loss_sporulation_capacity IS METPO:1000872, the record's own term.
  Collapsed to low_spo0a_activity -causes-> non_spore_forming_trait, which
  is the shape loss_sporulation_genes already uses in that graph.
- growth_at_4c IS METPO:1000618, and it is a leaf hanging off the node
  grounded to it.

That is #352's third bullet read strictly. It warns that an UNGROUNDED new
TRAIT node becomes a free anchor, which suggests requiring a grounding
fixes it. Requiring one does something better: it separates a real mistype
from a restatement.

CAPACITY IS NOT VESTIGIAL, which #352 left open. 24 nodes carry it; these 8
leave 16, and the survivors are a different sense - reducing_power (a pool
of reductants), cytoplasmic_buffering_capacity (a reservoir),
swimming_velocity (a rate), metabolic_versatility (a breadth). A buffer has
a capacity; so does a battery; neither is something an organism can DO.
Same two-senses shape `reduces` recorded in predicate_grounding.tsv, and it
is why #353's heuristic is organism-scoped rather than matching bare
"capacity to".

THE 3 DUPLICATE_GROUNDINGS. growth_external_ph_5_5_9 shared METPO:1000478
with ph_delta_high_trait, but the two say different things: an ABSOLUTE
external range (~5.5-9.0) versus a BREADTH (~5-9 pH UNITS), which is what a
pH delta is. Regrounded to METPO:1000332 (pH range). catalase and urease
shared their function node's GO term; GO:0004096 is catalase ACTIVITY, and
a protein is not its activity - the graphs already say so correctly with
catalase -enables-> catalase_function. Dropped from the proteins, kept on
the functions. Note this is NOT a corpus-wide sweep: 72 GENE_OR_PROTEIN
nodes are grounded to GO terms and that shorthand is fine where no separate
function node exists; only these two graphs model both.

NOT AN IMPROVEMENT IN CONNECTIVITY, and saying so before anyone asks.
UNREACHABLE_FROM_TRAIT fell 1303 -> 1296, but FRAGMENTED_GRAPH is flat at
218 and the islands are intact. Every one of the seven is explained by a
retype adding an anchor INSIDE an existing island - in oxygen_preference,
superoxide_dismutase and two others stopped being reported because
oxygen_tolerance, sitting in their island, became a TRAIT. Filed as #359:
the metric moves for two different reasons and only one is progress, and
this is the second time it has invited the wrong claim.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Regenerate biolink_coverage.tsv for the merged edges (#352)

Two consequences of the merges that I regenerated the report too early to
capture: `reduced in` drops 3 -> 2 residual because elongation_capacity's
duplicate edge to sphere_shaped_trait was deduplicated into
lateral_elongation's, and `has capability` disappears entirely because its
only corpus edge was psychrotolerant_trait -> growth_at_4c, the leaf that
restated its own parent.

Both are the migration working as intended, not collateral: an ungrounded
predicate with no remaining edges should stop appearing in a coverage
report.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Correct two of the four claimed retypes to merges after review (#352)

Review of the first burn-down pass (#360) found that 2 of the 4 nodes it
retyped were restatements, caught by the SAME test the other restatements
failed: the grounding chosen for each contradicted an edge or a definition
the graph already had.

salt_tolerance_breadth was retyped and grounded METPO:1000622 (halotolerant)
while keeping its `is a -> nacl_delta` edge. nacl_delta is METPO:1000335, a
delta (sub 1000532/1000534), whereas 1000622 is a halophily preference (sub
1000629) - so the graph asserted halotolerant sub NaCl-delta, a subsumption
METPO does not have. That is the absolute-vs-breadth distinction this
migration already insists on for pH, missed for salt. Merged into nacl_delta.

oxygen_tolerance was grounded METPO:1000609 (aerotolerant), which METPO
defines as "does NOT use O2 for growth but tolerates its presence" - false of
the obligate aerobes the node also covers - and which is itself sub
METPO:1000601, whose own definition is "oxygen requirements OR TOLERANCE for
growth". So it was a sixth child phenotype in a graph that wires the other
four in with `is a` and left this one unlinked. Merged into
oxygen_preference_trait.

The migration is now 2 retypes, 6 merges, 3 regroundings.

THE MERGE IS A REAL CONNECTIVITY GAIN AND THE RETYPE WAS NOT, which is the
distinction #359 is about. oxygen_preference FRAGMENTED_GRAPH goes
components=3 of 14 (sizes 8, 4, 2) -> components=2 of 13 (sizes 11, 2): the
ROS-defence island now attaches to the trait via detoxifying_enzyme_expression
and superoxide_dismutase, instead of merely sitting beside a new anchor. The
retype left that finding unchanged and the PR body said so. Note
UNREACHABLE_FROM_TRAIT is 1296 either way - the count alone cannot tell the
two apart, which is exactly #359's point.

Also fixes the audit trail. The first pass logged all 11 events as
RETYPE_CAUSAL_NODE, including the 7 merges and regroundings; an audit trail
that calls a merge a retype cannot answer the question it exists to answer.
The script now emits MERGE / DROP / REGROUND / UNGROUND_CAUSAL_NODE and
appends the curation_history entry itself via record_curation_event rather
than leaving it to be hand-written after the fact. Its timestamp is fixed
rather than wall-clock because pages/ derives "Corpus as of" from the latest
curation_history entry (#228).

just qc green (audit-graphs and audit-snippets both 0 new) - 519 tests pass.

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

* Merge the last two claimed retypes: none of the eight were retypes (#352)

Third review round on #360. Both remaining retypes are mis-grounded, so the
migration is now 0 retypes, 8 merges, 3 regroundings.

salt_tolerance was grounded METPO:1000622 (halotolerant) on the reasoning that
the record's own METPO:1000625 (slightly halophilic) makes the term "distinct".
Verified against data/raw/metpo.owl: 1000622 and 1000625 are DIRECT SIBLINGS
under 1000629, and 1000622 means "tolerates high salt but DOES NOT REQUIRE it
for growth" while 1000625 means the organism "REQUIRES low to moderate salt for
optimal growth". The node asserted of this record the negation of what the
record's own term says. Merged into slightly_halophilic_trait.

low_ph_tolerance was grounded METPO:1003008 (acidotolerant), defined as
tolerating acid "WHILE MAINTAINING OPTIMAL GROWTH NEAR NEUTRAL pH" — which
excludes the acidophiles this generic pH-delta record covers. A grounding
narrower than the node it labels is a false claim about every organism in the
excluded part. Merged into ph_delta_trait.

THE TEST THAT FAILED FOUR TIMES was "is this term distinct from the record's
own?". A sibling term is maximally distinct and still wrong. The right question
is whether the term is COMPATIBLE with the record and no NARROWER than the node.
Written into the playbook, because the wrong test is the intuitive one.

CORRECTING AN OVERCLAIM OF MY OWN in the same pass: I first wrote that merging
salt_tolerance attaches the ectoine island. Measurement says otherwise — the
node was already in the trait's component via osmoprotectant_transport ->
compatible_solutes -> osmotic_stress, so that merge changes no structure. It is
a correctness fix, and saying more would be this migration committing the exact
error it was written to catch.

MEASURED, NOT ASSERTED, across all eight:

  retyping   changed component structure in ZERO of the eight graphs
  merging    improves three — oxygen_preference 3 components -> 2,
             ph_delta 3 -> 2, ph_delta_low 5 -> 4
             and leaves five unchanged as pure deduplication

UNREACHABLE_FROM_TRAIT reads 1296 under either fix. That is #359's point, and
the playbook now points at reports/causal_graph_connectivity.tsv rather than the
finding counts.

Playbook also updated because its canonical "disposition" example WAS
salt_tolerance's own description, which this pass merges away.

RETYPE kept as an empty table rather than deleted: "we looked and there were
none" and "we never modelled retypes" are different claims, and only one is true.

519 tests pass - ruff clean.

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

* Correct two claims the data no longer supports (#360 review)

Both are claims, not data — but one of them sits in curation_history, whose
only job is to be true, in the pass that rewrote the audit trail so a merge
would stop being logged as a retype.

"filed as a proposal" was false. Nothing under proposals/ covers a generic
salt-tolerance disposition and this PR adds nothing there. The first half of
the sentence is right and stays: METPO genuinely has no term to reground to,
since halotolerant and acidotolerant are the only candidate labels and each
already anchors its own record. Now cites issue #364, which proposes a
tolerance axis distinct from the preference axis, and says plainly that
nothing under proposals/ exists yet.

The playbook's CAPACITY table listed two TRAIT examples that are both nodes
THIS PR DELETED. Last round caught the row using salt_tolerance's description;
the fix swapped in oxygen_tolerance's, which acd4a12 merged away one commit
earlier, alongside "Ability to grow at 4 C" which is growth_at_4c, dropped in
f60cb99. So the table told a curator "this sense becomes TRAIT" using nodes
that did not become TRAIT, thirteen lines above a sentence saying #352 retyped
nothing.

Rewritten so the column says what to DO rather than what to type: a
disposition reading makes a node a CANDIDATE, and the grounding step decides.
The examples are kept and labelled as what they are — all three merged or
dropped, none survived — which is a better lesson than a list of nodes that
did become traits, since there are none.

Also stopped citing reports/causal_graph_connectivity.tsv in the present
tense: it arrives with #363, not on this branch. The hand-measured numbers
stay, attributed as hand-measured.

519 tests pass - ruff clean.

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

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

1 participant