Migrate 164 enables→TRAIT edges onto the v8 predicates (#302, #303) - #323
Conversation
RO:0002327 (enables) has range 'biological process or activity', and a TRAIT node is a disposition, so all 164 of these edges entailed trait ⊑ BiologicalProcessOrActivity. They now carry the predicates proposed in proposals/metpo_traitmech_v8 (merged in #320): confers METPO:2007700 146 edges, direction unchanged has electron donor METPO:2007701 13 edges, direction reversed has electron acceptor METPO:2007702 5 edges, direction reversed The 18 electron edges are reversed back to <trait> -> <chemical>, which restores the donor/acceptor role PR #300 had to collapse onto enables (#303). The organism-subject problem that forced that collapse does not arise here: these predicates take a causal-node domain, not METPO:2000001's microbe domain (#301). The three mapping rows are GATED rather than the '*'/'*' used by earlier placeholder rows: confers is object_types=TRAIT with the 9 observed subject types, and the electron pair is TRAIT -> CHEMICAL. That enforces the "a chemical species" clause the declared OWL range deliberately does not, and is why oxygen_preference's MOLECULAR_FUNCTION node ("O2 as terminal electron acceptor", the USE of O2 rather than O2) grounds to confers instead. Applied by scripts/migrate_enables_trait_edges.py, which edits the raw lines rather than round-tripping through write_validated_trait: that helper is not byte-identical on hand-edited YAML (#322) and would have buried 164 real changes in reflow churn across 128 files. Baseline deltas, both intended and both verified: - predicate_domain_audit: 530 -> 366. ENABLES_RANGE_ON_TRAIT is fully burned down, 164 -> 0. The remaining 366 are the untouched #301 microbe-domain class. - evidence_snippet: 6 rows re-keyed, count unchanged at 2737. Reversing an edge changes its baseline key (subject->object), so 6 pre-existing findings re-key; all 6 old keys are present in the previous baseline. No new snippet defects. validate-strict: 477 files, 0 errors. pages/ regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 6m 53s —— View job Review
The mechanical part of this is sound. I verified independently:
Two things I'd want changed. 🟡 That section still reads:
and then shows a worked 🟡 - subject: molecular_oxygen # node_type: CHEMICAL, line 48
predicate: confers
object: manganese_oxidation_trait
description: O2 serves as the direct oxidant enabling multicopper-oxidase-mediated
Mn(II) oxidation.The published rule is "a
🔵 The proposal deferred the 🔵 Advisory only: no record under 🔵 Note on verification: Requesting changes on the two 🟡 items. |
![culturebot-reviewer[bot]](https://avatars.githubusercontent.com/u/233210009?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mechanically sound — 146/13/5 partition verified independently, all 18 reversed edges land on nodes actually typed CHEMICAL, the ENABLES_RANGE_ON_TRAIT ratchet is genuinely at 0, and reachability is undirected so the reversals cannot island a node.
Two 🟡 items:
-
docs/CURATION_PLAYBOOK.md:147-169still says the #302 decision "is not settled", cites the now-zero 164-edge count, and gives interimenables→process guidance with a worked example. That is the repo's convention doc; after this merges it actively misdirects curators away from the predicates this PR establishes. -
data/traits/metabolism/manganese_oxidation.yaml:86-89—molecular_oxygen(CHEMICAL) --confers-->manganese_oxidation_trait, description "O2 serves as the direct oxidant". Under the published rule (a CHEMICAL subject whose label or edge description names an electron donor/acceptor role) this ishas electron acceptor, reversed.ACCEPTOR_REmatches wording, not role, so it fell to the catch-all. The other three CHEMICAL→confers subjects (ectoine, surfactant, membrane_unsaturated_fatty_acids) are correctly not electron-role edges, so this is the single miss.confersis weaker rather than false — flagged 🟡, not 🔴 — but the weakening is exactly what #303 is about. Fixing it moves the partition to 145/13/6 and needs the merged v8 proposal appendix updated; recording the judgment call there instead is an acceptable alternative, but leaving it silent is not.
Plus three 🔵s in the PR comment (deferred oxygen_preference retyping, missing history/ record, mid-loop abort in the migration script).
Note: uv/python were not runnable in my sandbox, so validate-strict / qc / pytest were not independently re-run.
Two review findings, both valid. manganese_oxidation described O2 as the "terminal oxidant", which names the terminal-electron-acceptor role, but ACCEPTOR_RE matched only `electron acceptor|terminal electron` so the edge fell to the `confers` catch-all. That is not false, just strictly weaker — precisely the loss #303 exists to stop. Widened the rule to `oxidant` (and `reductant`, for symmetry on the donor side) and reversed that edge onto METPO:2007702. Verified against the pre-migration corpus that the widened rule reclassifies exactly one edge and no others, so the partition moves 146/13/5 -> 145/13/6 and nothing else shifts. proposals/metpo_traitmech_v8 counts, appendix and observations cells updated to match. docs/CURATION_PLAYBOOK.md still told curators the #302 decision "is not settled", cited the now-zero 164-edge count, and gave a worked `enables` example — the opposite of what this PR establishes, in the one place the repo documents the convention. Rewritten to state the settled convention with a table of the three predicates, the trait -> chemical direction of the electron pair, the node-type gating, and the placeholder-id caveat. Also closed the loop the proposal had deferred TO this PR: the oxygen_preference MOLECULAR_FUNCTION node keeps its type and grounds to `confers`, and the mapping gate (object_types=CHEMICAL) stops it drifting onto the electron pair later. Recorded in the proposal's classification notes rather than left dangling. Two advisory findings filed rather than fixed: #324 (the script's mid-loop abort can leave a repo-wide partial migration) and #325 (no history/records entry for a 128-file change, and nothing enforces one). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 4m 54s —— View job Review
Both 🟡 items from the previous round are properly resolved, and I re-verified the fixes rather than taking the commit message for it:
I also checked the "widened rule reclassifies exactly one edge" claim independently, since a regex widening is the kind of change that quietly catches more than intended. It holds: 🟡 The count correction stopped at the tables — five prose sites still say 146/13/5, and one of them contradicts the corrected total in the same document.
Line 187 is the sharpest: it sits in the "what was verified" list and asserts a partition that line 254 and line 244's Blast radius is limited — 🔵 The PR description's own table still reads 146 / 13 / 5. Worth syncing before merge since it's what lands in the squash commit message. 🔵 The previous 🔵s are all handled: the Note on verification: Requesting changes on the one 🟡. The data half of this is sound; it's the prose counts that are now internally inconsistent. |
|
Both 🟡 findings were valid and are fixed; both 🔵s are addressed or filed. 🟡 The O2 acceptor edge — fixedYou're right, and the diagnosis was exact. Widened to I checked the widened rule against the pre-migration corpus rather than assuming: it reclassifies exactly one edge — this one — and leaves the other 163 unchanged. So the partition moves 146/13/5 → 145/13/6 and nothing else shifts, which matches your reading. Proposal counts, the appendix, and both 🟡 The playbook — fixedFully rewritten. It now states the settled convention with a predicate table, spells out that the electron pair runs trait → chemical and why, documents the node-type gating (including that a 🔵 The
|
There was a problem hiding this comment.
Both 🟡 items from the last round are properly resolved, and I re-verified rather than trusting the commit message: the playbook now states the settled convention (and its gating claim matches mappings/predicate_grounding.tsv:102-104), the manganese_oxidation edge is reversed onto METPO:2007702 with a CHEMICAL object, the corpus partition counts 145/13/6 = 164, and ENABLES_RANGE_ON_TRAIT is 0 in both baseline and report. The regex widening is correctly scoped — classify() guards on CHEMICAL subjects and targets_in() only sees TRAIT objects, and the one other CHEMICAL-subject edge mentioning 'reductant' (nitrogen_fixing_symbiosis.yaml:156) points at a non-TRAIT node, so 'exactly one edge reclassified' holds.
One 🟡 remains: the count correction was applied to the tables, appendix, subject-type breakdown and ROBOT template, but not to prose. Five sites still say 146/13/5 — proposal.md:187 ('Edge partition (146 + 13 + 5 = 164)'), :288 ('Repoint the 146 confers edges … reverse the 18 electron edges', now 145 and 19), :99 ('The 18 electron edges', now 19), :93 ('114 of these 146 subjects'), and mappings/predicate_grounding.tsv:102 ('across the 146 migrated edges'). Line 187 contradicts the corrected 'Grand total: 145 + 13 + 6 = 164' about 60 lines below it, so the proposal now asserts two different partitions; line 288 is the adoption plan someone follows literally when METPO mints the ids. The submitted ROBOT TSV is correct, so blast radius is limited to the human-facing proposal — but it's a round-trip with the METPO maintainers if it ships. Note the subject_types pipe-list on that mapping row does NOT need changing; CHEMICAL still has 3 edges so all nine types remain observed.
🔵 The PR description's table still reads 146/13/5, which is what lands in the squash message.
Note: python3/uv are not executable in my sandbox, so validate-strict / qc / pytest were not independently re-run; counts, node types, gating rows and baseline totals were checked directly against the files.
The previous commit corrected the partition tables, the appendix, the grand total and both ROBOT traits_addressed cells, but not the prose around them — so proposal.md asserted two different partitions about 60 lines apart. Fixed: proposal.md:93 114 of these 146 subjects -> 145 proposal.md:99 The 18 electron edges -> 19 (13 + 6) proposal.md:187 Edge partition (146 + 13 + 5) -> (145 + 13 + 6) proposal.md:288 Repoint the 146 ... reverse 18 -> 145 ... 19 predicate_grounding.tsv:102 across the 146 migrated edges -> 145 Line 288 mattered most: it is the adoption plan someone follows literally when METPO mints the real ids, and it was wrong on both the repoint count and the reversal count. The subject_types pipe-list on the confers mapping row is deliberately unchanged: CHEMICAL still has 3 edges, so all nine node types remain observed. Line 254 keeps "146/13/5 to 145/13/6" because that sentence records the correction itself. Every remaining count claim cross-checked against the corpus: confers 145, donor 13, acceptor 6, electron 19, total 164. PR description synced too, since it becomes the squash commit message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 4m 1s —— View job Review
The five prose sites are all corrected, and I re-counted rather than trusting the commit message:
One new 🟡, introduced by the hand-fix in 🟡 The event reads:
But this file migrated two edges, not one. Pre-PR ( The correct form is the one the script produced for the other mixed file —
I checked every Not data corruption — the edges themselves are right and the acceptor reversal is correct — but curation history is the audit trail this repo leans on, and a 128-file migration where one record disclaims half its own change is exactly the thing that misleads later. 🔵 Note on verification: Requesting changes on the one 🟡. The prose-count sweep itself is complete and correct — this is a separate regression from the earlier hand-fix. |
There was a problem hiding this comment.
The five prose count sites are all corrected and I re-counted independently: METPO:2007700 = 145 across 114 files, METPO:2007701 = 13 across 13, METPO:2007702 = 6 across 4, total 164. That matches the appendix table, the grand total, and all three traits_addressed cells in the ROBOT template including the CHEMICAL 4 -> 3 fix. The only remaining 146 in proposal.md is line 254, which records the correction itself and is correctly left alone. ENABLES_RANGE_ON_TRAIT is 0 in both the baseline and the report. The rewritten playbook section checks out against the mapping file.
One new 🟡, introduced by the hand-fix in abb038b rather than by the count sweep:
data/traits/metabolism/manganese_oxidation.yaml:164 — the MIGRATE_ENABLES_TRAIT_EDGES event now reads 'Migrated 1 causal edge(s) ... (1 to has electron acceptor)', but this file migrated two edges. On origin/main both line 76 (multicopper_oxidase --enables--> manganese_oxidation_trait) and line 95 (the O2 edge) carried RO:0002327, and both were rewritten — the file today has METPO:2007700 on the first and METPO:2007702 on the second. The original event in 8d75418 said 'Migrated 2 ... (2 to confers)' and had the count right; rewriting it in place for the O2 reclassification dropped the confers edge from the record entirely, so nothing in the file now documents that migration. The correct form is the one the script emits for the other mixed file, dissimilatory_manganese_reduction.yaml:159: 'Migrated 2 causal edge(s) ... (1 to has electron acceptor, 1 to confers)'. I checked every MIGRATE_ENABLES_TRAIT_EDGES event in the corpus and this is the only one that disagrees with its own file — consistent with it being the only record hand-edited rather than regenerated. Re-running the widened script over origin/main would also emit 2, so the committed record is not what the script reproduces.
Not corrupting — the edges are right and the acceptor reversal is correct — but curation history is the audit trail here, and one record disclaiming half its own change is what misleads later.
🔵 proposal.md:93 — the denominator fix is right but the numerator is stale from #320: '114 of these 145 subjects are proteins, processes, or environmental conditions'. By the appendix table, BIOLOGICAL_PROCESS + GENE_OR_PROTEIN + ENVIRONMENTAL_FACTOR = 81+32+12 = 125 (113 without env). 114 is the record count, which is what the ROBOT traits_addressed cell correctly says. Optional, but you are already in the file.
Note: just/uv and script execution are unavailable in my sandbox, so validate-strict / qc / pytest were not re-run. All counts above were derived from the files directly against this branch and origin/main.
…review) Hand-editing that file's curation event to describe the O2 reversal overwrote the half the script had written: the record claimed "Migrated 1 causal edge(s) ... (1 to has electron acceptor)" while the file actually migrated two — multicopper_oxidase --confers--> the trait as well. A 128-file migration where one record disclaims half its own change is exactly what misleads a later reader of the audit trail. Rewritten to the form the script emits for the other mixed file (dissimilatory_manganese_reduction), so the record now matches both the file and what a re-run would reproduce, while keeping the explanation of why the O2 edge was reversed. Audited every MIGRATE_ENABLES_TRAIT_EDGES event in the corpus against its own file rather than fixing only the reported one: 0 remaining disagreements across all 128. Also fixed the pre-existing numerator at proposal.md:93 (from #320): "114 of these 145 subjects" was the trait-RECORD count leaking into a sentence about subjects. By the appendix it is 125 (81 BIOLOGICAL_PROCESS + 32 GENE_OR_PROTEIN + 12 ENVIRONMENTAL_FACTOR), now stated with the breakdown so it cannot drift from the table again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Valid catch, and worse than reported — fixed, plus a corpus-wide audit so it can't be hiding elsewhere. 🟡 The manganese_oxidation curation eventYou're right. Hand-editing that event to describe the O2 reversal overwrote the half the script had written: it claimed 1 edge / Rewritten to exactly the form the script emits for the other mixed file ( I then audited every 🔵 The 114/145 numerator — fixedGood spot, and you're right it predates this PR. 114 is the trait-record count (correct in the ROBOT Re-verified
|
|
Claude finished @realmarcin's task in 3m 51s —— View job Review
The 🟡 is resolved. I re-derived everything rather than reading the commit message. The event now matches its file. The corpus-wide audit claim holds. I parsed all 128 Counts are internally consistent now. One thing I went looking for because the reversals could have hidden it, and it's clean: reversing 🔵 The two open advisories are both filed rather than fixed, which is the right call for a PR this size: #324 (the mid-loop Approving. The migration, the reversals, the gating, the ratchet burn-down, and the audit trail all line up. Note on verification: |
There was a problem hiding this comment.
Verified the fix and re-derived every count independently. The manganese_oxidation curation event now reads 'Migrated 2 causal edge(s) ... (1 to has electron acceptor, 1 to confers)', matching both its file and the form the script emits for the other mixed record. I audited all 128 MIGRATE_ENABLES_TRAIT_EDGES events against their own files rather than taking the claim: every per-file claimed total matches its actual METPO:20077xx count, and the per-predicate claim labels sum to 145 confers / 13 donor / 6 acceptor = 164, which matches the corpus (13 donor across 13 files, 6 acceptor across 4 files). The 114 -> 125 numerator fix at proposal.md:93 is right and now carries its own derivation (81+32+12), and the only surviving '146' in the proposal is line 254, the sentence recording the correction. ENABLES_RANGE_ON_TRAIT is absent from both the baseline and the report. The corpus diff is exactly symmetric: 164 removals of predicate/predicate_id pairs, 145+13+6 additions, and 76 subject/object lines = 19 reversals, no stray edits.
One thing I checked because the reversals could have hidden it: reversing photoorganoheterotrophic_trait -> organic_compound collides with the pre-existing 'uses carbon source' edge between the same nodes, since the evidence-snippet baseline key omits the predicate. That is handled correctly -- the baseline stores a count per key (audit_evidence_snippets.py:409), so the duplicate key is counted, not masked, and the row total is unchanged at 2737.
No blocking issues. Remaining items are the already-filed #324 (mid-loop abort) and #325 (no history/records entry for the 128-file change), both advisory.
Note: uv/python execution is unavailable in my sandbox, so validate-strict / qc / pytest were not independently re-run; all counts above came from the files directly.
…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>
… zero (#327, closes #301) (#332) * Re-ground the last 3 substrate-to-product edges, and harden the gate (#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> * Ground the second is-hydrolyzed-to edge, and close the gate's back door (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> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* Say what write_validated_trait actually does to formatting (#322) The comment claimed that re-running the helper over an existing file "produces a byte-identical diff". Measured against the corpus: it holds for 127 of 477 files. The other 350 are reformatted, because safe_dump re-wraps long strings at its own width and drops hand-written quoting. The claim is not harmless. It is what a bulk-script author reads before deciding how to rewrite N files, and believing it means burying the real change in reflow churn across every long string in every file touched -- the difference between a reviewable migration and an unreviewable one. Both the #323 and #328 migrations edit raw lines instead, and this issue exists because the claim was checked rather than trusted. Corrected to state the real behaviour with the measured number, to say plainly that it matters for BULK use and not for a single record the helper already owns, and to note that making the claim true would mean normalising all 477 files once behind a round-trip test -- a large one-time reformat, which is the part of #322 still open. Three tests pin the two causes (re-wrapping, dropped quoting) and the case where the round trip DOES hold, so the claim cannot quietly return. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Bind the round-trip tests to the helper's real options (PR #343 review) The finding is exact and it is this PR's own thesis one level up. The tests asserted on yaml.safe_dump against a re-declared DUMP_OPTS dict, with nothing tying it to the helper. Adding width=4096 to the real options would have stopped the re-wrapping, falsified both the comment's stated cause and its 350/127 counts, and left every test passing -- because they were testing PyYAML's defaults, not write_validated_trait. Fixed structurally rather than by adding another assertion: the options are now EMIT_OPTS at module scope in write_validated.py and imported by the tests, so there is no copy to drift. The helper composes them with its yaml_kwargs exactly as before. Rewritten to exercise the real thing: - a hand-edited corpus file written through write_validated_trait to a tmp_path comes back with different bytes - the helper's OWN output round-trips byte-identically, which is the boundary that makes it safe for one record and unsafe for a bulk pass - the documented 127/350 split is asserted against the live corpus, so the numbers in the comment cannot go stale as a snapshot -- which is how the original claim came to mislead in the first place. It will fail loudly when #322's normalisation lands, which is the right moment to update them. Canaried by injecting width=4096 into the REAL options: the split test fails. The first test still passes there, because quoting differences alone keep some files reformatted -- worth knowing that the split test is the one carrying that guarantee. The corpus walk is lru_cached; three tests need it and each pass parses all 477 records (20s -> 7s). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Bind the split to the emit path, and stop the docstring over-claiming (PR #343 round 2) The 🟡 is a fair hit: that test's docstring said "change them so the reformatting stops and this fails", and I had ALREADY measured that it does not -- injecting width=4096 leaves it passing, because dropped quoting alone still reformats some files. I reported that honestly in the PR comment and then left the docstring asserting the opposite, which is worse than not having measured it. Corrected to say what is true: this one demonstrates the behaviour end to end, and the corpus-split test is what carries the guarantee. The 🔵 was the same class as the original finding, one step smaller: _split() rebuilt `yaml.safe_dump(doc, **EMIT_OPTS)` itself, so it bound to the options but not to how they are COMPOSED with yaml_kwargs. A change to the composition would have gone unnoticed. Extracted emit_trait_yaml() as the single emit path, used by write_validated_trait and imported by the tests, so there is nothing left to re-implement. Canaried the composition specifically this time, not just the options dict: injecting width into the composition expression fails the split test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
… (#344) * Normalise the corpus so the round-trip claim becomes enforceable (#322) write_validated_trait's comment claimed a byte-identical round trip. #343 corrected the claim to match reality -- it held for 127 of 477 records. This makes the claim TRUE instead, which is the half #322 left open. 350 records are rewritten through the helper's own emission path. No data changes; only formatting. safe_dump re-wraps long strings at its own width and drops hand-written quoting, so the diff is entirely those two things. Verified three independent ways rather than trusted: - The normaliser is TWO-PASS (#324's lesson): it computes and semantically checks every file before writing any, and aborts on the first mismatch rather than leaving the corpus half-normalised. 0 problems across 477. - Every rewritten file was re-parsed and compared to its pre-write document; a formatting pass that changed data would have failed there. - Independently, every file's parsed document was compared against `git show HEAD:` afterwards. 477 compared, 0 semantic differences. The strongest evidence is what did NOT change: pages/, reports/ and conf/ are byte-identical, because every generator and audit parses the YAML. Identical data in, identical artifacts out. The tests flip from documenting the gap to enforcing its absence. What asserted a 127/350 split now asserts that NO record fails to round trip, and the end-to-end test asserts equality rather than difference. Canaried by re-quoting a single scalar by hand: the suite fails. The payoff is that bulk scripts can use this helper again. #323, #328 and #341 all hand-rolled raw-line editors specifically to avoid the reflow churn, and that workaround is no longer necessary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Re-trigger CI The initial push produced no workflow runs at all despite the diff matching several paths: filters (data/traits/**, src/traitmech/**.py, tests/**.py). Close/reopen did not trigger them either. Empty commit to fire a synchronize event. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Re-trigger CI after the Actions outage This PR was opened at 19:24Z on 2026-08-06, during a critical GitHub Actions incident (stspg.io/rcz3fcm83sff) in which webhook triggers were throttled to ~15% and many push and pull_request events never created workflow runs. It received none, so it has sat CLEAN-looking and entirely unverified -- which is the state #346's detector now reports on every merge to main, and it flagged this PR within a minute of landing. The incident is resolved. Empty commit to fire a synchronize event now that they are being delivered again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Pin the corpus total, and fix prose the flip contradicted (PR #344 review) Both 🟡 are fair and the first is a regression I introduced when flipping the test. _split() skips anything that will not parse. The old assertion was `(len(same), len(changed)) == (127, 350)`, which pinned the total at 477, so a skipped record failed it. `assert not changed` does not -- a record edited into invalid YAML would drop silently out of the guard and the test would pass. Now asserts len(same) == len(TRAITS) as well. Canaried by appending invalid YAML to a record: it fails, and names the count. The second is the stale-prose-beside-updated-code mistake I have now made several times in this session. The module docstring still opened with the claim being "false for most of the corpus" three lines above saying it is now true, and test_the_helpers_own_output_round_trips still described the helper as "unsafe for a bulk rewrite" -- the exact opposite of what this PR establishes and of write_validated.py's own comment. Rewritten: the docstring is now past-tense about the pre-#322 state, and that test is described as what it actually asserts, the emitter's idempotence, which is a different property from the corpus being in that form. The 🔵 about a `str` representer emitting block scalars for the ~50 curation-history prose blocks is a good idea and is filed as #347 rather than taken here: it changes the emitted format, so it would mean re-normalising all 477 records inside a PR already under review, and the round-trip test would then enforce whichever form is chosen -- a one-way decision worth making deliberately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…350 round 3) The 🟡 is a good catch and neatly framed: the failure message told contributors to set source=local, which the tests added LAST round prove no longer exempts anything. It also could not be caught by qc, being prose in a passing gate. Rewritten to name what actually works -- repoint to a real slot, or MINT it as #342 did -- and to say plainly that source=local does not exempt and the escape is ALLOWED_UNBACKED, keyed to the CURIE. The gate read only the mapping table. That is not the only route into the corpus: ground_causal_predicates writes from the table, but a curator can type a predicate_id straight into a record, and #342's whole point is that the CURIE in the RECORD is what a reader believes. It now checks both, reporting an example file per offending CURIE so it can be found. Canaried by typing biolink:not_a_slot into a record: exit 1, CURIE and file named. The first attempt at that canary was a no-op -- it targeted RO:0002327 in crispr_cas_system.yaml, which #323 had already migrated away -- and reported a false pass until I checked the substitution actually happened. Also fixed the 9-vs-8 record count in both the v9 row and the mapping note, and guarded relative_to() against a traits dir outside the repo, which the new tests hit immediately (the same guard audit_causal_graphs already carries). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Stop the encodes row claiming provenance it does not have (#342) mappings/predicate_grounding.tsv grounded `encodes` to biolink:encodes with source=biolink and a note claiming an "exact label match against biolink slot". The pinned data/raw/biolink-model.yaml (4.4.0) has no such slot -- the gene-to-product slot is `has gene product` -- so the CURIE resolved to nothing upstream. Swept all 13 biolink: CURIEs used in the mapping table against the model. biolink:encodes is the ONLY one with no backing slot; the other twelve resolve. That answers the question the issue asked and bounds the problem to one row. The row keeps the coinage, because a gene CLUSTER to protein COMPLEX edge does not fit `has gene product`'s range of gene product mixin -- repointing would trade a missing term for a wrong one. What changes is the claim: source=local rather than biolink, and predicate_id skos:closeMatch rather than exactMatch, since there is no upstream term to be exactly equal to. `local` joins METPO/biolink/RO/RDFS in the column's vocabulary. The real defect was not the coinage but that nothing noticed. The signal had been sitting in reports/biolink_coverage.tsv all along as the only applied biolink: CURIE with both backing columns empty -- a report nobody consults is not a check. audit-biolink-curies is that check, and is in qc. Deliberately narrow: it does NOT require the corpus LABEL to match a slot name. Most labels are synonyms -- generates, yields, forms and six others all ground to biolink:produces -- so requiring that would flag correct rows. It requires only that the CURIE names a real slot, and exempts rows declaring source=local, which keeps a coinage legitimate while making it say so. Canaried by setting the row back to source=biolink: the gate fails and names it. Six tests, including that a synonym label passes and that a local row is exempt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Mint encodes rather than disclaim it in a table nobody reads (PR #350 review) The 🟡 is right and it changes the fix rather than refining it. source=local lived in one column of one TSV, ground_causal_predicates discards it outright, and the unqualified `biolink:encodes` was what got written into the corpus -- so a reader of a trait record saw an apparently-upstream term and the disclaimer was nowhere near them. Annotating provenance at a layer the data does not carry is not fixing the claim. Minted METPO:2007813 `encodes` into the v9 cohort instead, and migrated all 10 edges across 8 records. That removes the false claim from the DATA rather than describing it elsewhere, and puts the term in the same pending-upstream state as the other 13 v9 predicates instead of a category of its own. RO:0002205 (has gene product) was considered and rejected on the same grounds the coinage existed for: it relates a GENE to a gene product, while every corpus edge relates a gene CLUSTER or OPERON to a protein COMPLEX or a biosynthetic PROCESS. Recorded as skos:closeMatch at medium confidence -- the granularity differs on both sides, and saying `high` would overstate it. audit-biolink-curies stays. It now passes with no exemptions at all, which is a better resting state than passing because one row opted out, and it still catches the next unbacked CURIE. Worth recording: the #322 round-trip guard caught this commit. Inserting curation events as hand-built YAML left 8 records non-canonical, the guard failed, and re-emitting them through the helper fixed it -- which is exactly the workflow #322 was for, on the first bulk edit after it landed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Key the escape to CURIEs, and gate the report that knew (PR #350 round 2) Both 🔵s are the PR's own thesis pointed at the PR. The exemption was `source == "local"`, a free-text cell. A future unbacked CURIE could have been silenced by typing five characters into a TSV -- the same shape of failure this gate exists to catch, one level up, and exactly what the reviewer said. Now an explicit ALLOWED_UNBACKED set keyed to the CURIE, following conf/id_label_targets.yaml's `exceptions:` convention. It is EMPTY, because #342's only coinage was minted as METPO:2007813 rather than exempted, so adding an entry is a code change someone reviews rather than a cell edit. Two tests: source=local alone no longer exempts, and an explicitly allowed CURIE does. reports/biolink_coverage.tsv is now in audit-derived-reports. It had drifted -- regenerating it moved 549 lines unrelated to this change (produces METPO:2000202 -> METPO:2007800, defines residual -> applied) -- and it is the report that recorded biolink:encodes as unbacked for however long that was true. A stale report nobody reads is even less of a check than a current one, which is the argument this PR is built on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Check the corpus too, and stop the message contradicting the code (PR #350 round 3) The 🟡 is a good catch and neatly framed: the failure message told contributors to set source=local, which the tests added LAST round prove no longer exempts anything. It also could not be caught by qc, being prose in a passing gate. Rewritten to name what actually works -- repoint to a real slot, or MINT it as #342 did -- and to say plainly that source=local does not exempt and the escape is ALLOWED_UNBACKED, keyed to the CURIE. The gate read only the mapping table. That is not the only route into the corpus: ground_causal_predicates writes from the table, but a curator can type a predicate_id straight into a record, and #342's whole point is that the CURIE in the RECORD is what a reader believes. It now checks both, reporting an example file per offending CURIE so it can be found. Canaried by typing biolink:not_a_slot into a record: exit 1, CURIE and file named. The first attempt at that canary was a no-op -- it targeted RO:0002327 in crispr_cas_system.yaml, which #323 had already migrated away -- and reported a false pass until I checked the substitution actually happened. Also fixed the 9-vs-8 record count in both the v9 row and the mapping note, and guarded relative_to() against a traits dir outside the repo, which the new tests hit immediately (the same guard audit_causal_graphs already carries). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Parse the corpus, and fix the comment I missed a third time (PR #350 round 4) The 🟡 is the same defect a third time in this PR: I corrected the remedy in the script's failure message last round and left it in the justfile recipe comment. As the reviewer notes, qc cannot catch it -- prose in a passing recipe, which is the class this PR was opened over. Corrected, and while there, the comment now also states that the gate checks the corpus, which it had not said either. The corpus sweep regexed YAML in a module that already imports yaml. Now parses causal_graphs[].edges[].predicate_id properly. That is not just tidiness: a text scan matches a CURIE quoted inside curation_history prose, and several records now contain exactly that BECAUSE they describe this issue -- methanogenesis.yaml and intracellular_inclusion.yaml both mention biolink:encodes in their history. A regex would have reported those as live groundings the moment the phrasing shifted. A test pins it. Switching to parsing broke two of my own tests, correctly: their fixtures used a flat `edges:` list rather than the real causal_graphs[].edges[] shape, so they would have passed through the parsing version unseen and proved nothing. Fixtures rebuilt in the real structure. Re-canaried after the change: a CURIE typed into a record still exits 1 and names the file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
The corpus half of the #302/#303 fix. #320 proposed the predicates; this adopts them.
What changed
RO:0002327(enables) has range 'biological process or activity'; aTRAITnodeis a disposition, so all 164 of these edges entailed
trait ⊑ BiologicalProcessOrActivity. They now carry themetpo_traitmech_v8predicates:
METPO:2007700confersMETPO:2007701has electron donorMETPO:2007702has electron acceptorThe reversal restores the donor/acceptor role PR #300 had to collapse onto
enables(#303). The organism-subject problem that forced that collapse doesn't arise here —
these predicates take a causal-node domain, not
METPO:2000001's microbe domain (#301).The mapping rows are gated, unlike the
*/*used by earlier placeholder rows:confersisobject_types=TRAITwith the nine observed subject types, and the electronpair is
TRAIT → CHEMICAL. That enforces the "a chemical species" clause the declaredOWL range deliberately doesn't — and is exactly why
oxygen_preference'sMOLECULAR_FUNCTIONnode ("O2 as terminal electron acceptor" — the use of O2, not O2)grounds to
confersinstead.The proof it landed
ENABLES_RANGE_ON_TRAITis fully burned down, 164 → 0, in the ratcheted audit addedby #314. The predicate-domain baseline drops 530 → 366; the remaining 366 are the
untouched #301 microbe-domain class.
What was checked
just validate-strict— 477 files, 0 errors.just qc— green, every ratchet atblocking: 0.uv run pytest— 420 passed. Ruff clean.dissimilatory_iron_reduction(3 acceptor reversals — the hardest case) and once on
biofilm_formation(one of the5 records with a
discussions:block aftercuration_history, where a naiveappend-at-EOF would land in the wrong place). Both diffs were inspected line by line
before applying to the other 126.
2737. Reversing an edge changes its baseline key (
subject->object), so 6pre-existing findings re-key; all 6 old keys are present in the previous baseline, so
these are not new defects. A 7th diff line is byte-identical content that merely moved
position in the sort.
On the migration script
scripts/migrate_enables_trait_edges.pyedits the raw lines rather thanround-tripping through
write_validated_trait. That helper's docstring claims abyte-identical round-trip, but it isn't for hand-edited YAML — it re-wraps long strings
and drops hand-written quoting (filed as #322). Over 128 files that would have buried
164 real changes in reflow churn, which is the unreviewable outcome #301 warns about.
The parsed document is used only to decide; only the
subject/object/predicate/predicate_idlines that change are touched.The script refuses to write a partial migration if the text matches fewer edges than the
parse found, so a silent under-application isn't possible.
Scope
Closes #302 and #303. Does not touch #301's 366 microbe-domain edges — those need
their own per-predicate-family decisions, and the ratchet keeps them visible and
non-growing meanwhile. The
METPO:2007700–2007702ids remain placeholders untilupstream mints them; the round-trip plan is in
proposals/metpo_traitmech_v8/proposal.md.🤖 Generated with Claude Code