diff --git a/history/infrastructure/ground-nodes-duplicate-guard/2026-08-08T043349Z-claude-code-d0a8f5.yaml b/history/infrastructure/ground-nodes-duplicate-guard/2026-08-08T043349Z-claude-code-d0a8f5.yaml new file mode 100644 index 00000000..1639122c --- /dev/null +++ b/history/infrastructure/ground-nodes-duplicate-guard/2026-08-08T043349Z-claude-code-d0a8f5.yaml @@ -0,0 +1,45 @@ +history_version: 1 +target: + kind: infrastructure + path: scripts/ground_causal_nodes.py + slug: ground-nodes-duplicate-guard +session: + id: 2026-08-08T043349Z-claude-code-d0a8f5 + timestamp: '2026-08-08T04:33:49Z' + actors: + - type: ai_agent + name: claude-code +links: + issues: + - https://github.com/CultureBotAI/TraitMech/issues/361 +events: +- type: EDIT + outcome: changed + sections: + - grounding + summary: Decline a node grounding the graph already carries, instead of writing the duplicate + details: '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. Concretely: #352 removed GO:0004096 from + the catalase node and GO:0009039 from urease, but mappings/node_grounding.tsv still maps both proteins + to those GO ACTIVITY terms, so the next ''just ground-nodes --apply'' re-created both duplicates (verified: + 2 files modifiable, 2 nodes grounded, exactly the two that had been ungrounded). Deleting the mapping + rows would over-correct, because protein -> GO-activity is the corpus''s accepted shorthand wherever + the graph does not also model the function as its own node, and 72 GENE_OR_PROTEIN nodes rely on it. + So the guard lives in the writer: ground_nodes_in_doc now tracks the CURIEs already present in each + graph, seeded from existing groundings and updated as it writes, and declines a candidate that collides. + Declined nodes are reported ONLY through the declined counter and are deliberately kept OUT of residual: + the residual TSV reads like a census of ungrounded nodes but its consumers treat it as a work queue + of labels still needing a mapping. match_uniprot_to_proteins.py''s load_target_labels takes every + GENE_OR_PROTEIN row from it and under --apply appends a UniProtKB row to mappings/node_grounding.tsv + with no existing-row check, so listing catalase there would earn it a second mapping row conflicting + with its GO one, and load_mapping raises on exactly that - taking out just ground-nodes and the derived-reports + freshness check with it (caught in review of PR 362). The semantics settle it independently of the + breakage: a declined node is not awaiting a grounding, it has one that was deliberately withheld, + so proposing a UniProt accession for it would be wrong even if nothing downstream broke. A consequence + worth stating: the residual TSV is byte-identical to before in every case, so no consumer is perturbed + at all. The summary prints what was withheld and why. Scoped per graph rather than per record, since + DUPLICATE_GROUNDING is a within-graph defect. Verified against the fix/352 branch state in memory: + grounded 2 -> 0, declined 2. On main the corpus is unaffected - 0 modifiable, residual TSV byte-identical. + ground_causal_predicates.py has no analogous gap: nothing flags a duplicate predicate_id and nothing + should, since many edges legitimately share a predicate. 524 tests pass (5 new), qc green.' diff --git a/scripts/ground_causal_nodes.py b/scripts/ground_causal_nodes.py index 1e7a1e65..716f2dd8 100644 --- a/scripts/ground_causal_nodes.py +++ b/scripts/ground_causal_nodes.py @@ -101,9 +101,34 @@ def load_mapping(path: Path) -> dict[MappingKey, tuple[str, str]]: def ground_nodes_in_doc( doc: dict[str, Any], mapping: dict[MappingKey, tuple[str, str]], -) -> tuple[int, Counter, Counter, Counter]: +) -> tuple[int, Counter, Counter, Counter, Counter]: """Mutate ``doc`` in place, grounding empty grounding slots. + A candidate CURIE already carried by another node in the SAME graph is + declined rather than written, because ``audit-graphs`` reports exactly + that shape as DUPLICATE_GROUNDING -- so writing it would mean this script + manufacturing a finding the audit then reports against us. #352 burned + down three such duplicates by hand; two of them (`catalase` GO:0004096, + `urease` GO:0009039) are still live rows in the mapping table, because the + protein-to-GO-activity shorthand is correct wherever the graph does NOT + also model the function as its own node -- 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. Without this guard the next ``--apply`` re-created + both duplicates (#361). + + A declined node is NOT counted into ``residual``, and that is deliberate. + The residual TSV reads like a census of ungrounded nodes but its consumers + treat it as a WORK QUEUE of labels still needing a mapping: + match_uniprot_to_proteins.py's ``load_target_labels`` takes every + GENE_OR_PROTEIN row from it and, under ``--apply``, appends a UniProtKB row + to mappings/node_grounding.tsv with no existing-row check. Listing + `catalase` there would earn it a second, conflicting mapping row, and + ``load_mapping`` raises on exactly that -- taking out ``just ground-nodes`` + and the freshness check with it (#362 review). A declined node is not + awaiting a grounding; it has one, deliberately withheld. Proposing a + UniProt accession for it would be actively wrong. It is reported through + ``declined`` instead, which is what that counter is for. + Returns ------- grounded : int @@ -112,16 +137,28 @@ def ground_nodes_in_doc( Map from target CURIE → grounded-node count. residual : Counter (label, node_type) → count of nodes that had no mapping entry. + Declined nodes are excluded; see above. grounded_keys : Counter (label, node_type) → count of nodes that **were** grounded. Caller needs this to re-classify them as residual if a later validation step rejects the file. + declined : Counter + (label, node_type, curie) → count of nodes whose mapped CURIE was + withheld because the graph already carried it. """ grounded = 0 per_curie: Counter = Counter() residual: Counter = Counter() grounded_keys: Counter = Counter() + declined: Counter = Counter() for graph in (doc.get("causal_graphs") or []): + # Seeded from what the graph already carries, then updated as we go, + # so two ungrounded nodes mapping to one CURIE cannot both take it. + taken = { + (n.get("grounding") or "").strip() + for n in (graph.get("nodes") or []) + if (n.get("grounding") or "").strip() + } for node in (graph.get("nodes") or []): label = (node.get("label") or "").strip() node_type = (node.get("node_type") or "").strip() @@ -133,13 +170,17 @@ def ground_nodes_in_doc( key = (label.lower(), node_type) if key in mapping: curie, _src = mapping[key] + if curie in taken: + declined[(label.lower(), node_type, curie)] += 1 + continue node["grounding"] = curie + taken.add(curie) grounded += 1 per_curie[curie] += 1 grounded_keys[key] += 1 else: residual[key] += 1 - return grounded, per_curie, residual, grounded_keys + return grounded, per_curie, residual, grounded_keys, declined def main() -> int: @@ -166,6 +207,8 @@ def main() -> int: per_curie_total: Counter = Counter() residual_total: Counter = Counter() residual_examples: dict[MappingKey, list[str]] = defaultdict(list) + declined_total: Counter = Counter() + declined_examples: dict[tuple[str, str, str], list[str]] = defaultdict(list) for path in files: try: @@ -176,7 +219,7 @@ def main() -> int: if not isinstance(doc, dict): continue - grounded, per_curie, residual, grounded_keys = ground_nodes_in_doc(doc, mapping) + grounded, per_curie, residual, grounded_keys, declined = ground_nodes_in_doc(doc, mapping) def _record_residual(keys_counter: Counter) -> None: for key, n in keys_counter.items(): @@ -184,6 +227,13 @@ def _record_residual(keys_counter: Counter) -> None: if len(residual_examples[key]) < 3: residual_examples[key].append(str(path.relative_to(REPO_ROOT))) + # Recorded regardless of whether the file is written: a declined + # grounding is a fact about the corpus, not about this run's mode. + for dkey, n in declined.items(): + declined_total[dkey] += n + if len(declined_examples[dkey]) < 3: + declined_examples[dkey].append(str(path.relative_to(REPO_ROOT))) + if grounded == 0: _record_residual(residual) continue @@ -249,6 +299,14 @@ def _record_residual(keys_counter: Counter) -> None: print(" by target CURIE:", file=sys.stderr) for curie, n in per_curie_total.most_common(): print(f" {curie:30s} {n:>6d}", file=sys.stderr) + if declined_total: + print(f" declined (already in graph): {sum(declined_total.values())}", file=sys.stderr) + print(" withheld because another node in the same graph already carries the CURIE,", + file=sys.stderr) + print(" which audit-graphs would report as DUPLICATE_GROUNDING (#361):", file=sys.stderr) + for (label, node_type, curie), n in declined_total.most_common(): + examples = ", ".join(declined_examples[(label, node_type, curie)]) + print(f" {label} ({node_type}) -> {curie} ×{n} [{examples}]", file=sys.stderr) if not args.apply and files_modified: print("", file=sys.stderr) print(" Re-run with --apply to write the changes.", file=sys.stderr) diff --git a/tests/test_ground_causal_nodes.py b/tests/test_ground_causal_nodes.py index 3b11c8c6..70b9af63 100644 --- a/tests/test_ground_causal_nodes.py +++ b/tests/test_ground_causal_nodes.py @@ -120,7 +120,7 @@ def test_ground_nodes_basic(): ("molecular oxygen", "CHEMICAL"): ("CHEBI:15379", "CHEBI"), ("photosynthesis", "BIOLOGICAL_PROCESS"): ("GO:0015979", "GO"), } - grounded, per_curie, residual, grounded_keys = ground_nodes_in_doc(doc, mapping) + grounded, per_curie, residual, grounded_keys, _ = ground_nodes_in_doc(doc, mapping) assert grounded == 2 assert per_curie == Counter({"CHEBI:15379": 1, "GO:0015979": 1}) assert residual == Counter() @@ -141,7 +141,7 @@ def test_ground_nodes_skips_existing_grounding(): {"node_id": "b", "label": "molecular oxygen", "node_type": "CHEMICAL"}, ]) mapping = {("molecular oxygen", "CHEMICAL"): ("CHEBI:15379", "CHEBI")} - grounded, _, _, _ = ground_nodes_in_doc(doc, mapping) + grounded, _, _, _, _ = ground_nodes_in_doc(doc, mapping) assert grounded == 1 nodes = doc["causal_graphs"][0]["nodes"] assert nodes[0]["grounding"] == "CHEBI:99999" @@ -159,7 +159,7 @@ def test_ground_nodes_node_type_keyed_lookup(): mapping = { ("terminal electron acceptor", "CHEMICAL"): ("METPO:1007504", "METPO"), } - grounded, _, residual, _ = ground_nodes_in_doc(doc, mapping) + grounded, _, residual, _, _ = ground_nodes_in_doc(doc, mapping) assert grounded == 1 assert residual == Counter({("terminal electron acceptor", "PATHWAY"): 1}) @@ -170,7 +170,7 @@ def test_ground_nodes_idempotent_second_pass(): ]) mapping = {("photosynthesis", "BIOLOGICAL_PROCESS"): ("GO:0015979", "GO")} ground_nodes_in_doc(doc, mapping) - grounded2, _, residual2, grounded_keys2 = ground_nodes_in_doc(doc, mapping) + grounded2, _, residual2, grounded_keys2, _ = ground_nodes_in_doc(doc, mapping) assert grounded2 == 0 assert residual2 == Counter() assert grounded_keys2 == Counter() @@ -182,7 +182,7 @@ def test_ground_nodes_skips_nodes_without_label_or_type(): {"node_id": "b", "label": "x"}, # no node_type {"node_id": "c", "label": "", "node_type": "CHEMICAL"}, # empty label ]) - grounded, _, residual, _ = ground_nodes_in_doc(doc, {("x", "CHEMICAL"): ("X:1", "X")}) + grounded, _, residual, _, _ = ground_nodes_in_doc(doc, {("x", "CHEMICAL"): ("X:1", "X")}) assert grounded == 0 assert residual == Counter() @@ -196,7 +196,7 @@ def test_ground_nodes_grounded_keys_separable_from_residual(): {"node_id": "b", "label": "unmapped thing", "node_type": "CHEMICAL"}, ]) mapping = {("molecular oxygen", "CHEMICAL"): ("CHEBI:15379", "CHEBI")} - grounded, _, residual, grounded_keys = ground_nodes_in_doc(doc, mapping) + grounded, _, residual, grounded_keys, _ = ground_nodes_in_doc(doc, mapping) assert grounded == 1 assert residual == Counter({("unmapped thing", "CHEMICAL"): 1}) assert grounded_keys == Counter({("molecular oxygen", "CHEMICAL"): 1}) @@ -206,3 +206,111 @@ def test_ground_nodes_grounded_keys_separable_from_residual(): ("molecular oxygen", "CHEMICAL"): 1, ("unmapped thing", "CHEMICAL"): 1, }) + + +# ------------------------------------------------- duplicate-grounding guard (#361) + + +def test_ground_nodes_declines_curie_already_in_graph(): + """The exact shape #352 burned down by hand: a protein node and the + function node it enables, where the mapping table sends the protein to + the function's GO ACTIVITY term. Writing it would re-create the + DUPLICATE_GROUNDING that audit-graphs reports.""" + doc = _doc_with_nodes([ + {"node_id": "catalase_function", "label": "catalase activity", + "node_type": "MOLECULAR_FUNCTION", "grounding": "GO:0004096"}, + {"node_id": "catalase", "label": "catalase", "node_type": "GENE_OR_PROTEIN"}, + ]) + mapping = {("catalase", "GENE_OR_PROTEIN"): ("GO:0004096", "GO")} + grounded, per_curie, residual, grounded_keys, declined = ground_nodes_in_doc(doc, mapping) + + assert grounded == 0 + assert per_curie == Counter() + assert grounded_keys == Counter() + assert declined == Counter({("catalase", "GENE_OR_PROTEIN", "GO:0004096"): 1}) + assert "grounding" not in doc["causal_graphs"][0]["nodes"][1] + # NOT residual -- see the next test for why that distinction is load-bearing. + assert residual == Counter() + + +def test_declined_nodes_stay_out_of_residual(): + """The residual TSV is a WORK QUEUE, not a census (#362 review). + + match_uniprot_to_proteins.py's load_target_labels() takes every + GENE_OR_PROTEIN row from reports/node_grounding_residual.tsv and, under + --apply, appends a UniProtKB row to mappings/node_grounding.tsv with no + existing-row check. A declined node listed there would earn `catalase` a + second mapping row conflicting with its GO one, and load_mapping() raises + on exactly that -- taking out `just ground-nodes` and the freshness check. + A declined node is not awaiting a grounding; it has one, withheld. + """ + doc = _doc_with_nodes([ + {"node_id": "fn", "label": "catalase activity", + "node_type": "MOLECULAR_FUNCTION", "grounding": "GO:0004096"}, + {"node_id": "prot", "label": "catalase", "node_type": "GENE_OR_PROTEIN"}, + {"node_id": "other", "label": "genuinely unmapped", "node_type": "GENE_OR_PROTEIN"}, + ]) + mapping = {("catalase", "GENE_OR_PROTEIN"): ("GO:0004096", "GO")} + _, _, residual, _, declined = ground_nodes_in_doc(doc, mapping) + + # Only the genuinely unmapped label is a target for the UniProt matcher. + assert residual == Counter({("genuinely unmapped", "GENE_OR_PROTEIN"): 1}) + assert ("catalase", "GENE_OR_PROTEIN") not in residual + assert declined == Counter({("catalase", "GENE_OR_PROTEIN", "GO:0004096"): 1}) + + +def test_ground_nodes_declines_second_node_mapping_to_same_curie(): + """`taken` is updated as we go, not just seeded once -- otherwise two + ungrounded nodes sharing a mapped CURIE would both take it and produce + the duplicate this guard exists to prevent.""" + doc = _doc_with_nodes([ + {"node_id": "a", "label": "catalase", "node_type": "GENE_OR_PROTEIN"}, + {"node_id": "b", "label": "catalase (KatA)", "node_type": "GENE_OR_PROTEIN"}, + ]) + mapping = { + ("catalase", "GENE_OR_PROTEIN"): ("GO:0004096", "GO"), + ("catalase (kata)", "GENE_OR_PROTEIN"): ("GO:0004096", "GO"), + } + grounded, _, _, _, declined = ground_nodes_in_doc(doc, mapping) + + assert grounded == 1 + assert declined == Counter({("catalase (kata)", "GENE_OR_PROTEIN", "GO:0004096"): 1}) + nodes = doc["causal_graphs"][0]["nodes"] + assert nodes[0]["grounding"] == "GO:0004096" + assert "grounding" not in nodes[1] + + +def test_ground_nodes_guard_is_per_graph_not_per_record(): + """DUPLICATE_GROUNDING is scoped to one graph, so the same CURIE in a + DIFFERENT graph of the same record is not a duplicate and must still be + written -- otherwise the guard would suppress legitimate groundings.""" + doc = {"causal_graphs": [ + {"nodes": [{"node_id": "a", "label": "catalase activity", + "node_type": "MOLECULAR_FUNCTION", "grounding": "GO:0004096"}]}, + {"nodes": [{"node_id": "b", "label": "catalase", + "node_type": "GENE_OR_PROTEIN"}]}, + ]} + mapping = {("catalase", "GENE_OR_PROTEIN"): ("GO:0004096", "GO")} + grounded, _, _, _, declined = ground_nodes_in_doc(doc, mapping) + + assert grounded == 1 + assert declined == Counter() + assert doc["causal_graphs"][1]["nodes"][0]["grounding"] == "GO:0004096" + + +def test_ground_nodes_guard_leaves_the_72_protein_shorthand_alone(): + """The mapping rows are NOT the defect: where a graph models the protein + but not its function, protein -> GO activity is the corpus's accepted + shorthand and must still be written (#361).""" + doc = _doc_with_nodes([ + {"node_id": "catalase", "label": "catalase", "node_type": "GENE_OR_PROTEIN"}, + {"node_id": "h2o2", "label": "hydrogen peroxide", "node_type": "CHEMICAL", + "grounding": "CHEBI:16240"}, + ]) + mapping = {("catalase", "GENE_OR_PROTEIN"): ("GO:0004096", "GO")} + grounded, per_curie, _, _, declined = ground_nodes_in_doc(doc, mapping) + + assert grounded == 1 + assert declined == Counter() + assert per_curie == Counter({"GO:0004096": 1}) + assert doc["causal_graphs"][0]["nodes"][0]["grounding"] == "GO:0004096"