Skip to content

Re-ground the last substrate-to-product edges, and harden the gate to zero (#327, closes #301) - #332

Merged
realmarcin merged 2 commits into
mainfrom
fix/327-derives-into
Aug 6, 2026
Merged

Re-ground the last substrate-to-product edges, and harden the gate to zero (#327, closes #301)#332
realmarcin merged 2 commits into
mainfrom
fix/327-derives-into

Conversation

@realmarcin

Copy link
Copy Markdown
Contributor

Retires the MICROBE_DOMAIN_ON_NONORGANISM class entirely: 1 → 0, and converts the
check from a ratchet into a hard gate.

No minting needed

RO:0001001 (derives into, biolink:derives_into) holds between an old and a new
material entity where the new "begins to exist when the old ceases to exist, and inherits
the significant portion of the matter of the old"
— substrate-to-product chemistry. That
is exactly what these edges assert, so #327 resolves with an upstream term and no new
METPO placeholder
:

edge was now
cellulose --is hydrolyzed to--> cellobiose METPO:2000013 RO:0001001
thiosulfate --derives into--> sulfate METPO:2007800 RO:0001001
thiosulfate --derives into--> sulfide METPO:2007800 RO:0001001

The first is the defect #327 was filed for — METPO:2000013 reads agent→substrate, so
grounding a substrate→product edge to it asserted cellulose hydrolyses cellobiose. Its
label is kept, since it names the mechanism the general relation doesn't. The other two had
a CHEMICAL subject on produces, and a chemical doesn't bring another into existence the
way an agent does.

METPO:2007405 (oxidized to, v2) was considered and rejected twice over: it fits only
the oxidised half, so it would split a matched pair from one disproportionation reaction,
and it's an unminted placeholder where RO:0001001 is real.

One edge was examined and deliberately left: acetyl_phosphate --produces--> ATP fails
derives into's own test — ATP's matter comes mostly from ADP, only the phosphoryl group
transfers. Filed as #331 rather than forced into a relation that doesn't fit.

The gate is now hard

audit_predicate_domains shipped as a ratchet over 530 findings (#314) because nothing
was fixable without decisions that hadn't been made. Those landed — v8 for #302/#303
(#320, #323), v9 for #301 (#326, #328, #329), and this PR — so:

  • just audit-predicate-domains passes --fail-on any;
  • conf/predicate_domain_audit_baseline.tsv is deleted, not kept empty, because an
    empty baseline invites a --write-baseline run to refill it;
  • the recipe comment says plainly not to reintroduce one.

The ratchet machinery in the script is deliberately kept — it's what makes a future
violation class landable without blocking unrelated work, exactly as this one was.

What was checked

  • Canaried the hard gate: injecting one microbe-domain edge exits 1, with no
    baseline to forgive it. File restored.
  • RO:0001001's label verified via OAK as "derives into" before writing the mapping rows,
    since label-correspondence checks (id, label) pairs against RO.
  • just qc green, just validate-strict 477 / 0 errors, uv run pytest 420 passed,
    ruff clean.
  • Curation events added to both records, with curation_history verified to sort in order.

Closes

#327, and #301 — whose 366 edges are now fully retired.

🤖 Generated with Claude Code

…327)

RO:0001001 (derives into) holds between an old and a new material entity
where the new begins to exist as the old ceases and inherits the
significant portion of its matter -- substrate-to-product chemistry.
biolink maps it as biolink:derives_into. That is exactly what these edges
say, so #327 resolves with an upstream term and no minting at all:

  cellulose --is hydrolyzed to--> cellobiose   METPO:2000013 -> RO:0001001
  thiosulfate --derives into--> sulfate        METPO:2007800 -> RO:0001001
  thiosulfate --derives into--> sulfide        METPO:2007800 -> RO:0001001

The first was the defect #327 was filed for: METPO:2000013 reads
agent-to-substrate, so grounding a substrate-to-product edge to it
asserted that cellulose hydrolyses cellobiose. Its label is kept, since
it names the mechanism the general relation does not. The other two had a
CHEMICAL subject on `produces`, and a chemical does not bring another
into existence the way an agent does.

METPO:2007405 (oxidized to, proposals v2) was considered for the sulfate
half and rejected twice over: it fits only the oxidised half, so it would
split a matched pair from one disproportionation reaction, and it is an
unminted placeholder where RO:0001001 is real.

acetyl_phosphate --produces--> ATP was examined and deliberately NOT
moved: ATP's matter comes mostly from ADP, only the phosphoryl group
transfers, so it fails RO:0001001's "inherits the significant portion of
the matter" test. Filed separately rather than forced.

MICROBE_DOMAIN_ON_NONORGANISM: 1 -> 0. #301's 366 edges are fully retired.

So the check is now a HARD GATE. It shipped as a ratchet over 530 findings
(#314) because nothing was fixable without the decisions that have since
landed. `just audit-predicate-domains` now passes --fail-on any, and
conf/predicate_domain_audit_baseline.tsv is deleted rather than kept
empty -- an empty baseline invites a --write-baseline run to refill it.
Canaried: injecting one microbe-domain edge exits 1 with nothing to
forgive it.

The ratchet machinery in the script is deliberately kept, since it is what
makes a future violation class landable the same way this one was.

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

culturebot-reviewer Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude finished @realmarcin's task in 5m 28s —— View job


Review posted — changes requested

  • Read repo conventions (README, docs/)
  • Review the diff (YAML edits, mappings, justfile, audit script)
  • Verify RO:0001001 resolves and its label matches
  • Check the hard-gate change and baseline deletion
  • Run validation / audit — not possible here, see note below
  • Post review + verdict (--request-changes)

The ontology call is right: RO:0001001 is biolink:derives_into's exact_mapping in the vendored model (data/raw/biolink-model.yaml:5126-5138) and its definition matches the text quoted in the mapping notes, so the substrate→product grounding is sound with no minting. The METPO:2007405 rejection holds, as does the deferral of acetyl_phosphate --produces--> ATP to #331.

Three defects:

Plus one 🔵 (--write-baseline can still recreate the deleted baseline, and a non-recipe invocation defaults to --fail-on new and would load it) and one advisory (no history/records/ entry, though only three exist repo-wide).

Note: I could not run just qc, the audits, or pytest — Bash approvals in this environment are limited to a read-only set, so uv run … and just … were denied. The green results in the description are taken at face value; everything above comes from reading the diff, the mapping table, the grounding script's gate semantics, and the vendored biolink model. Add those to --allowedTools if you want the suites actually executed on review.
· branch fix/327-derives-into

@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 ontology call is right. RO:0001001 is biolink:derives_into's exact_mapping in the vendored model (data/raw/biolink-model.yaml:5126-5138), and its definition there matches the text the mapping notes quote, so the substrate→product reading is sound and no minting is needed. The METPO:2007405 rejection is correct, and so is the alternative I checked for you: biolink's more specific has metabolite (is_a: derives into) is exact-mapped only to CHEBI:25212, which its own comment flags as a role rather than a predicate. Deferring acetyl_phosphate --produces--> ATP to #331 rather than forcing it is the right call for the same reason.

Three things to fix.

🟡 justfile:105 names the wrong CURIE. The recipe comment says the last edge was "re-grounded to RO:0002234 in #327". RO:0002234 is has output; the edge went to RO:0001001. The script docstring (scripts/audit_predicate_domains.py:35) has it right, so this is just the justfile copy — but it is the curator-facing one, and a wrong CURIE in the comment documenting the grounding decision is exactly the thing this repo is careful about.

🟡 The is hydrolyzed to mapping row covers a second edge that was not grounded. data/traits/metabolism/starch_degradation.yaml:81 is starch --is hydrolyzed to--> maltooligosaccharides, CHEMICAL→CHEMICAL, no predicate_id. It matches the new row's gate, so ground_causal_predicates.py now considers it mapped — which is why its row disappeared from reports/predicate_grounding_residual.tsv in this diff. But the YAML edge is still ungrounded. Net effect: the edge is in neither the grounded set nor the residual backlog, and just ground-predicates --apply would now produce an uncommitted diff that nothing gates on. Either ground it here (with a curation event, as the other two got) or leave it visible.

🟡 docs/CURATION_PLAYBOOK.md is stale in two places this PR makes false. Line 139 still reads "#301 tracks the 366 edges that still carry them" — that count is now 0 and #301 closes with this PR. Line 124 says "Nothing in just qc catches it", which stopped being true when the audit joined qc and is now emphatically untrue under --fail-on any. The sibling enables paragraph was updated the same way when #302/#303 hit zero ("The count is now 0, and the audit ... fails any new one"), so the convention is already established here.

🔵 The comment says not to reintroduce a baseline, but just audit-predicate-domains --write-baseline still writes conf/predicate_domain_audit_baseline.tsv, and any invocation that does not go through the recipe defaults to --fail-on new and would load it. Keeping the machinery is the right decision; if you want the norm enforced rather than documented, that is the seam.

Advisory, not a defect: neither trait got a record under history/records/ — though only three exist repo-wide, so the practice is clearly still partial.

I could not run just qc, the audits, or pytest in this environment (Bash approvals are restricted to a read-only set), so the green results in the description are taken at face value; the findings above come from reading the diff, the mapping table, the grounding script's gate semantics, and the vendored biolink model.

…or (PR #332 review)

Three review findings, all valid.

The new `is hydrolyzed to` mapping row also matched
starch_degradation.yaml's `starch --is hydrolyzed to-->
maltooligosaccharides`, a CHEMICAL->CHEMICAL edge with no predicate_id.
Adding the row dropped it from the residual report because the grounding
tool now considered it mapped, but nothing had grounded it -- so it sat
in neither the grounded set nor the backlog. Grounded to RO:0001001; it
is the same substrate-to-product shape as the cellulose edge.

justfile:105 named RO:0002234 (has output) as what the last edge was
re-grounded to. It was RO:0001001 (derives into). The script had it
right, the recipe comment did not.

docs/CURATION_PLAYBOOK.md said "Nothing in `just qc` catches it" and
"#301 tracks the 366 edges that still carry them". Both were true when
written and are now false: the audit is a qc member running --fail-on
any, and the count is 0. Rewritten to describe a mistake the tooling
prevents rather than a backlog to work around, mirroring what #323 did
for the enables paragraph.

Also acted on the 🔵, which was a real hole rather than a nit:
--write-baseline can still recreate the deleted baseline, and a
non-recipe invocation defaulted to --fail-on new, so it would have loaded
that file and passed. Deleting the baseline while leaving the default
permissive only hardened the path that goes through `just`. The default
is now `any`; the ratchet is still available via an explicit
--fail-on new for reintroducing this check over a NEW violation class.

That flip broke test_write_baseline_freezes_then_passes, which had
encoded the old contract -- the test was right to fail. Updated to assert
both halves of the new one, plus a new test_default_fail_on_is_any
pinning that a stray baseline cannot weaken a default run, so the
hardening cannot be undone by an argparse edit.

Canaried both paths: injecting one microbe-domain edge exits 1 via the
recipe AND via a direct no-flag invocation.

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

Copy link
Copy Markdown
Contributor Author

All three 🟡 fixed, and the 🔵 turned out to be a real hole rather than a nit — so I acted on it too.

🟡 The second is hydrolyzed to edge

Caught cleanly. Adding the mapping row made starch_degradation.yaml's starch --is hydrolyzed to--> maltooligosaccharides look mapped, so the grounding tool dropped it from the residual — but nothing had actually grounded it, leaving it in neither the grounded set nor the backlog. Grounded to RO:0001001; it's the same substrate→product shape as the cellulose edge, so it belonged in this PR rather than a follow-up.

🟡 justfile:105 wrong CURIE

Right — RO:0002234 is has output; the edge went to RO:0001001. The script had it right and the recipe comment didn't. Fixed.

🟡 Stale playbook

Both sites were true when written and are now false. Rewritten to say the audit does catch it and hard-fails in qc, and that #301's 366 edges are migrated and its count is 0 — mirroring what #323 did for the enables paragraph.

🔵 The back door — fixed, not just noted

You're right that --write-baseline can recreate the deleted file, and I'd left the default at --fail-on new, so a direct invocation would have loaded it and passed. Deleting the baseline while leaving the default permissive only hardened the path through just.

Default is now any. The ratchet remains available via an explicit --fail-on new, which is the right shape: reintroducing this check over a new violation class is a deliberate act, not a default.

That flip broke test_write_baseline_freezes_then_passes — correctly, since it encoded the old contract. Updated to assert both halves of the new one, and added test_default_fail_on_is_any pinning that a stray baseline can't weaken a default run, so this can't be undone by an argparse edit later.

Re-verified

Canaried both paths: an injected microbe-domain edge exits 1 via the recipe and via a direct no-flag invocation. just qc green, validate-strict 477 / 0, 421 tests pass (one new), ruff clean.

On the tooling note

Understood that uv/just were denied in your environment — worth flagging that the counts above are from my runs, and the CI checks on this PR are the independent signal.

@culturebot-reviewer

culturebot-reviewer Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude finished @realmarcin's task in 3m 1s —— View job


Review posted — approved

  • Read the new commit 3e4757b2
  • Verify the second is hydrolyzed to edge is grounded
  • Verify justfile:105 CURIE fix
  • Verify playbook rewrite
  • Verify the --fail-on any default + tests
  • Check CI on this head
  • Post verdict (--approve)

All three 🟡 are fixed, and the 🔵 was closed rather than documented away.

  • Second is hydrolyzed to edgedata/traits/metabolism/starch_degradation.yaml:82 now carries predicate_id: RO:0001001 with a GROUND_CAUSAL_PREDICATES event, and its residual row is gone for the right reason this time. Both nodes are CHEMICAL, so the edge sits inside the mapping row's own CHEMICAL|CHEMICAL gate rather than being let in by a wildcard. Grepping the corpus, is hydrolyzed to and derives into occur on exactly 4 edges and all 4 are grounded — no third edge slipped through the same seam.
  • justfile:105 now names RO:0001001 (derives into).
  • Playbook no longer claims just qc misses this, and the 366 causal-graph edges assert microbe-domain METPO predicates on non-organism subjects #301 paragraph reads as a closed migration.
  • The default flip to --fail-on any is the right shape, and the recipe still composes: {{args}} is appended after --fail-on any, so just audit-predicate-domains --fail-on new still reaches the ratchet when someone deliberately wants it. test_default_fail_on_is_any pins the half that mattered — a stray baseline on disk cannot weaken a no-flag run.

The label side is confirmed independently of the description: label-correspondence is green on this head, and that is the gate that checks (RO:0001001, "derives into") against RO. qc (which now contains the hard-gated audit), validate-strict, pytest and curation-history are green too — so unlike last round the suites are verified, just by CI rather than by me.

🔵, not blocking: the is hydrolyzed to row's note (mappings/predicate_grounding.tsv:110) still describes one edge — "Substrate-to-product hydrolysis: cellulose -> cellobiose" and "This is the last of the 366 #301 edges." The row now grounds two, and the starch one was never among the 366 (it had no predicate_id at all). Not false, but it's the curator-facing documentation for a gate that admits more than it says.

Advisory as before: no history/records/ entry for either trait.
· branch fix/327-derives-into

@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.

All three 🟡 are fixed, and the 🔵 was closed properly rather than documented away.

Re-verified against the diff:

  • starch --is hydrolyzed to--> maltooligosaccharides (data/traits/metabolism/starch_degradation.yaml:82) now carries predicate_id: RO:0001001, with a GROUND_CAUSAL_PREDICATES curation event and the residual row correctly gone. Both nodes are CHEMICAL, so the edge sits inside the mapping row's own CHEMICAL|CHEMICAL gate rather than being admitted by a wildcard. Grepping the corpus, is hydrolyzed to and derives into occur on exactly 4 edges total and all 4 are grounded — no third edge slipped through the same seam.
  • justfile:105 now names RO:0001001 (derives into).
  • docs/CURATION_PLAYBOOK.md no longer claims just qc misses this, and the #301 paragraph reads as a closed migration.
  • The default flip to --fail-on any is the right shape, and the recipe still composes: {{args}} is appended after --fail-on any, so just audit-predicate-domains --fail-on new still reaches the ratchet when someone deliberately wants it. test_default_fail_on_is_any pins the half that mattered — a stray baseline on disk cannot weaken a no-flag run.

The label side checks out independently: label-correspondence is green on this head, which is the gate that verifies (RO:0001001, "derives into") against RO, so the two new mapping rows are confirmed by CI and not only by the description. qc — which now contains the hard-gated audit — validate-strict, pytest and curation-history are all green as well.

One 🔵, nothing blocking: the is hydrolyzed to row's note (mappings/predicate_grounding.tsv:110) still describes a single edge — "Substrate-to-product hydrolysis: cellulose -> cellobiose" and "This is the last of the 366 #301 edges." The row now grounds two edges, and the starch one was never among the 366 (it had no predicate_id at all). Not false, but the note is the curator-facing documentation for a gate that admits more than it says.

Advisory as before: no history/records/ entry for either trait.

Approving.

@realmarcin
realmarcin merged commit 6e5e992 into main Aug 6, 2026
8 checks passed
@realmarcin
realmarcin deleted the fix/327-derives-into branch August 6, 2026 06:29
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