Decline a node grounding the graph already carries (#361)

[realmarcin]

Closes #361.

ground_causal_nodes.py wrote any mapped CURIE into any ungrounded node — including one another node in the same graph already carried, which is exactly the shape audit-graphs reports as DUPLICATE_GROUNDING. The writer manufactured findings the auditor then reported against us.

The concrete regression

#352 removed GO:0004096 from the catalase node and GO:0009039 from urease, on the argument that a protein is not its activity — the graphs already say so with catalase -enables-> catalase_function, and the function node carries the term. But mappings/node_grounding.tsv still has:

115: catalase   GENE_OR_PROTEIN   GO:0004096   catalase activity   skos:closeMatch   GO   high
116: urease     GENE_OR_PROTEIN   GO:0009039   urease activity     skos:closeMatch   GO   high

Measured against #360's corpus, before this change:

files modifiable:    2
nodes grounded:      2
by target CURIE:
  GO:0004096   1
  GO:0009039   1

Those 2 are exactly the 2 nodes #360 ungrounded. The next --apply put both duplicates straight back.

Deleting the mapping rows would over-correct

#352's own reasoning says the protein → GO-activity shorthand is fine wherever the graph does not also model the function as its own node, and 72 GENE_OR_PROTEIN nodes rely on it. The row is not the defect; writing it into a graph that already says the same thing is. So the guard belongs in the writer.

The guard

ground_nodes_in_doc now tracks the CURIEs present in each graph — seeded from existing groundings and updated as it writes, so two ungrounded nodes mapping to one CURIE cannot both take it — and declines a candidate that collides.

Declined nodes stay ungrounded, so they are counted into residual as well: that report describes what the corpus actually looks like, and an ungrounded node belongs there however it got that way. The summary prints what was withheld and why, rather than silently skipping:

declined (already in graph): 2
  withheld because another node in the same graph already carries the CURIE,
  which audit-graphs would report as DUPLICATE_GROUNDING (#361):
  catalase (GENE_OR_PROTEIN) -> GO:0004096  ×1  [data/traits/physiology/catalase_activity.yaml]

Scoped per graph, not per record, because DUPLICATE_GROUNDING is a within-graph defect — the same CURIE in a different graph of the same record is not a duplicate and is still written. There's a test pinning that, because getting it wrong would suppress legitimate groundings.

Verification

• Against Burn down the 11 disposition-typing findings — none of the eight were retypes (#352) #360's branch state: grounded 2 → 0, declined 2.
• On main: corpus unaffected — 0 modifiable, reports/node_grounding_residual.tsv byte-identical.
• 4 new tests, including the 72-node shorthand case (still grounds) and the per-graph scoping case.

ground_causal_predicates.py has no analogous gap: nothing flags a duplicate predicate_id and nothing should, since many edges legitimately share one. Checked rather than assumed.

Ordering: independent of #360 — this branches off main and touches no trait data. But the regression it prevents only becomes reachable once #360 merges, so this should land at or before that point.

just qc green · 523 tests pass (4 new) · ruff clean · history record included per #325.

[culturebot-reviewer]

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

---

Review

The guard itself is right. taken is seeded from existing groundings and updated as it writes, scoped per graph — that matches scripts/audit_causal_graphs.py:248-256 exactly (same per-graph scope, same .strip() normalization), so it declines precisely the shape the auditor reports and nothing else. The per-graph test pins the one way this could over-fire.

Verified against the corpus: data/traits/physiology/catalase_activity.yaml:47 and :51 both still carry GO:0004096 on main, so nothing is currently ungrounded for the guard to decline — the "0 modifiable, residual byte-identical" claim holds. No CURIEs are introduced by the diff; the ones in the tests (GO:0004096 catalase activity, GO:0009039 urease activity, CHEBI:16240, CHEBI:15379, GO:0015979) all match their labels. History record present. I could not run the test suite or the script here — permissions — so the 523-tests figure is unverified on my side.

One real problem.

🟡 Putting declined nodes in residual breaks that TSV's contract for its two consumers

reports/node_grounding_residual.tsv has meant labels with no mapping entry, and two things read it that way:

• scripts/match_uniprot_to_proteins.py:126 — load_target_labels(), docstring "labels with no grounding" — takes every GENE_OR_PROTEIN row and, under --apply, appends a UniProtKB: row to mappings/node_grounding.tsv (:316-325) with no check against rows already there.
• .claude/commands/ground-or-propose-metpo.md:35 — hands the same rows to an agent told to ground them or draft a METPO proposal.

After #360 lands, catalase and urease get declined every run and land in that TSV as GENE_OR_PROTEIN targets. catalase is a tier-1 exact match in UniProt and is not in SKIP_LABELS, so the next match_uniprot_to_proteins.py --apply appends catalase GENE_OR_PROTEIN UniProtKB:… alongside the existing catalase … GO:0004096 row — and load_mapping() (scripts/ground_causal_nodes.py:92-96) then raises mapping conflict for ('catalase', 'GENE_OR_PROTEIN'), taking out just ground-nodes and the report-freshness check in just qc. The METPO path is the softer version of the same failure: an agent asked to ground a label that already has a curated mapping.

The hazard isn't new — _record_residual(residual + grounded_keys) at :266 already leaks mapped labels into residual when a file fails validation — but that's an error path. This makes it steady state for labels that will be declined on every run, forever.

"An ungrounded node belongs in residual however it got that way" is defensible; it just needs the consumers to be able to tell the two cases apart. Either shape works:

• add a reason column (unmapped / declined_duplicate) and filter on it in load_target_labels(); or
• leave the TSV alone and keep declined nodes out of residual, reporting them only through the declined counter you already added.

Fix this →

Also worth flagging for merge order: this PR alone leaves reports/node_grounding_residual.tsv byte-identical, but this plus #360 does not — whoever lands the second of the two needs to regenerate it or just qc goes STALE.

Everything else — the per-graph scoping, the write-time taken update, the declined summary, the docstring's account of why the mapping rows stay — reads correctly to me.

---

Requested changes on the PR.
· branch fix/361-ground-nodes-duplicate-guard

[culturebot-reviewer]

The guard itself is right — per-graph scope and .strip() normalization match audit_causal_graphs.py:248-256 exactly, and main is genuinely unaffected (catalase_activity.yaml:47,51 still both carry GO:0004096).

One 🟡: counting declined nodes into residual changes the meaning of reports/node_grounding_residual.tsv, which two consumers read as "labels with no mapping entry". match_uniprot_to_proteins.py:126 feeds those rows into an --apply that appends to mappings/node_grounding.tsv with no existing-row check, so once #360 lands, catalase gets a second, conflicting UniProtKB row and load_mapping() raises a mapping conflict — taking out just ground-nodes and the freshness check. .claude/commands/ground-or-propose-metpo.md:35 reads the same TSV the same way.

Either add a reason column (unmapped / declined_duplicate) and filter on it in load_target_labels(), or keep declined nodes out of residual and report them only via the declined counter.

Details in the comment on the PR.

[culturebot-reviewer]

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

---

Review

ebfbed22 takes the second of the two options and it's the right one. residual[key] += 1 is gone from the decline branch, so residual is now populated only by the else — labels with no mapping entry — which is exactly what load_target_labels() (scripts/match_uniprot_to_proteins.py:126) and .claude/commands/ground-or-propose-metpo.md:35 assume. The new test_declined_nodes_stay_out_of_residual pins it with a third genuinely-unmapped node so the assertion can't pass vacuously. The reasoning in the docstring (scripts/ground_causal_nodes.py:119-130) is the stronger version of what I argued: a declined node isn't awaiting a grounding, so proposing a UniProt accession for it would be wrong even with no downstream breakage.

Two things I checked that follow from the change:

• The merge-order warning I gave is now moot. On main, catalase_activity.yaml:43-52 has the protein and catalase_function in the same graph, both on GO:0004096. After Burn down the 11 disposition-typing findings — none of the eight were retypes (#352) #360 ungrounds the protein, the function node still holds the CURIE, so the protein is declined — and declined no longer means residual. reports/node_grounding_residual.tsv is byte-identical under Decline a node grounding the graph already carries (#361) #362 alone and under Burn down the 11 disposition-typing findings — none of the eight were retypes (#352) #360+Decline a node grounding the graph already carries (#361) #362. Nothing to regenerate.
• The validation-failure path stays consistent. _record_residual(residual + grounded_keys) at :274 can't pick up a declined node, since it lands in neither counter. Correct — a rejected file doesn't make a withheld grounding into a wanted one.

The guard still matches audit_causal_graphs.py:248-256 exactly: per-graph, .strip()-normalized, node-type-agnostic. No CURIEs introduced.

One thing left over.

🟡 The history record now describes the behavior this PR reversed

history/infrastructure/ground-nodes-duplicate-guard/2026-08-08T043349Z-claude-code-d0a8f5.yaml still carries, in details:

Declined nodes stay ungrounded so they are counted into residual as well

That is the first commit's behavior, and ebfbed22 is the commit that removed it. The record is the only committed artifact explaining why the code is shaped this way, and it currently tells a future reader the opposite — which is the specific way this goes wrong: someone reads it, notices the code disagrees, and "restores" the residual counting, putting catalase back in the UniProt matcher's queue and re-arming the mapping conflict break. Same paragraph's 523 tests pass (4 new) is also stale against ebfbed22's own 524 tests - 5 new.

Rewrite that sentence to say declined nodes are reported through the declined counter and deliberately kept out of residual, with the work-queue-not-census reason — the docstring already has the wording. The other single-event records under history/infrastructure/ suggest amending this record rather than adding a second one, but either shape records the truth.

Worth doing the same to the PR body's "Deleting the mapping rows would over-correct" section, which still has the old declined nodes … counted into residual rationale — not blocking, but it's what gets read at merge.

Fix this →

Not verified on my side: the 524 tests pass / ruff / just qc claims — I don't have permission to run uv run here.

Everything else reads correctly. Once the history record matches what shipped, this is good to go.

---

· branch fix/361-ground-nodes-duplicate-guard

[realmarcin]

Good catch — fixed in ebfbed22, taking your second option (keep declined nodes out of residual).

I verified the chain rather than taking it on trust: load_target_labels() filters the residual TSV to node_type == "GENE_OR_PROTEIN", match_uniprot_to_proteins.py:316 opens MAPPING_TSV in "a" mode with no existing-row check, and load_mapping() raises mapping conflict for one (label, node_type) with two CURIEs. So catalase would have gained a UniProtKB row beside its GO:0004096 one and taken out just ground-nodes — and audit-derived-reports with it, since that recipe runs the same generator.

I went with option 2 over the reason column because the semantics settle it independently of the breakage: a declined node is not awaiting a grounding — it has one, deliberately withheld. Proposing a UniProt accession for it would be wrong even if nothing downstream broke. A reason column would have kept it in a work queue it does not belong in and pushed the filtering onto every consumer, including the two you named and any future one that forgets.

It also has a property I like better than my original: the residual TSV is now byte-identical to before in every case, so no consumer is perturbed at all. Confirmed against #360's corpus state — catalase and urease are declined and absent from the GENE_OR_PROTEIN rows; only oxyr, a genuinely unmapped protein, remains a target for the matcher.

Added a test pinning the distinction with your reasoning as its docstring, so the next person who thinks "an ungrounded node belongs in the ungrounded report" finds out why it doesn't before shipping it.

524 tests pass, just qc green.

[culturebot-reviewer]

ebfbed22 resolves the residual-TSV issue correctly — declined nodes are out of residual, so load_target_labels() and the METPO command see only genuinely unmapped labels, and the TSV stays byte-identical under #362 alone and under #360+#362 (so the merge-order STALE warning is moot).

One 🟡 left: history/infrastructure/ground-nodes-duplicate-guard/2026-08-08T043349Z-claude-code-d0a8f5.yaml still says "Declined nodes stay ungrounded so they are counted into residual as well" — the behavior this PR's second commit removed — and its 523/4-new test tally is stale. That record is the only committed explanation of why the code is shaped this way, and it currently says the opposite; someone acting on it would re-arm the mapping-conflict break. Amend the details field to match the docstring.

Details in the comment on the PR.

[culturebot-reviewer]

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

---

Review

886b20bf fixes the record. The details field now says what ships — declined nodes are reported only through the declined counter and deliberately kept out of residual — with the work-queue-not-census reason and the load_target_labels → conflicting-UniProtKB-row → load_mapping raises chain spelled out. The stale tally is corrected to 524 tests pass (5 new), which matches the diff: git diff origin/main...HEAD -- tests/test_ground_causal_nodes.py adds exactly 5 def test_ (17 total in the file). Nothing in the record contradicts scripts/ground_causal_nodes.py:119-130 anymore.

Re-checked the code, since the record is only worth what it describes:

• The guard is unchanged and still matches audit_causal_graphs.py:248-256 — per-graph taken, .strip()-normalized, updated at write time (:154-179).
• residual[key] += 1 appears only in the else at :182, so a declined node lands in neither residual nor grounded_keys, and _record_residual(residual + grounded_keys) at :274 can't resurrect it.
• The 5 tests cover the four ways this could go wrong: the The corpus types 'capacity to X' two ways: TRAIT in some records, CAPACITY/STATE in others #352 shape, the residual exclusion (with a third genuinely-unmapped node so the assertion isn't vacuous), the write-time taken update, per-graph scoping, and the 72-node shorthand still grounding.
• CURIEs in the tests all match their labels — GO:0004096 catalase activity, CHEBI:16240 hydrogen peroxide, CHEBI:15379 molecular oxygen, GO:0015979 photosynthesis. No trait data touched.

No 🔴 or 🟡. One optional note:

🔵 "byte-identical to before in every case" is slightly stronger than true

The record's sentence holds for the corpus and for every normal run, but not literally every case: on the validation-failure path, a node that is now declined would pre-PR have been grounded and then re-classified into residual via grounded_keys, so pre- and post-PR residual TSVs differ for a file that both has a declined-shape node and fails validation. Zero such files exist, the new behavior is the correct one, and this changes nothing about the code — it's just that this record is now the artifact people will trust, so "byte-identical for the corpus, in every non-error path" would age better. Not worth another commit on its own.

Not verified on my side: the 524 tests pass / ruff / just qc claims — uv run needs approval I don't have here. The tally is consistent with the diff, but I did not execute the suite.

Approving.
· branch fix/361-ground-nodes-duplicate-guard

[culturebot-reviewer]

The history record now matches what shipped. details states declined nodes are reported only via the declined counter and kept out of residual, with the work-queue reason and the load_target_labels -> conflicting UniProtKB row -> load_mapping raises chain; tally corrected to 524 (5 new), which matches the diff exactly (5 new def test_, 17 in the file).

Re-verified the code: guard unchanged and still matching audit_causal_graphs.py:248-256 (per-graph, .strip()-normalized, taken updated at write time); residual[key] += 1 only in the else branch, so _record_residual(residual + grounded_keys) cannot resurrect a declined node; test CURIEs all match their labels; no trait data touched.

One 🔵 non-blocking note in the PR comment about the record's 'byte-identical in every case' phrasing being slightly stronger than true on the validation-failure path. Nothing to change in the code.