From 8721432f7d4b3baa1a75434f5c077d4649ec3274 Mon Sep 17 00:00:00 2001 From: ugur Date: Tue, 29 Sep 2026 11:25:41 +0100 Subject: [PATCH 1/2] Support non-CL edit files: OBO input + ontology-neutral wording Found while integrating this package into uberon (obophenotype/uberon issue #3778, PR #3779). Two real, reproducible gaps, both fixed here: 1. stage1.extract() silently dropped all definition refs for OBO edit files. definition_refs() only understands OWL functional-syntax `AnnotationAssertion(...)` lines, but extract() fed it the raw right-side edit-file text unconverted -- fine for CL's cl-edit.owl (already functional syntax) but a no-op for an OBO file like uberon-edit.obo. Fixed by converting the right file to functional syntax (robot convert -f ofn) before calling definition_refs(), with the obo:/oboInOwl: prefixes it expects since robot doesn't add those by default. 2. `robot diff` failed outright against a real uberon-edit.obo, because it tries to resolve every owl:imports declaration and one of uberon's import PURLs currently 404s. Stage 1 only ever needs the edit file's own asserted axioms, never the merged import closure, so _strip_imports() now drops `import:` (OBO) and `Import(...)` (OWL functional syntax) lines before anything reaches robot. This also makes every review run faster and independent of import-graph network calls, for any ontology. 3. agent_instructions.md instructed the verification agent to treat "the subject" as "the cell type" in several places that shape actual assertion wording (core vs. background classification, and the instruction to preserve "the cell-type subject"). Generalized to "the term"/"the term's subject", plus a note up top that CL_... ids and "cell type" elsewhere in the doc are illustrative examples, not a CL-only contract. Left the `cell_id` field name and CL_... example ids elsewhere as-is: they're internal bookkeeping/examples that don't leak into assertion content, and renaming them would mean a coordinated schema change across every consumer (CL and uberon's clara-review.yml both key off `cell_id` in produced verdicts.json), which isn't needed to fix the actual problem. Added tests/test_stage1_extract.py covering _strip_imports(), _robot_convert_to_ofn(), and an end-to-end extract() run against a throwaway git repo with an OBO edit file carrying a deliberately broken import -- reproducing both original failures and proving the fix. Full suite (44 tests) passes. No consumer-visible schema changes: changes.json's shape and existing CL fixtures/tests are unaffected. Co-Authored-By: Claude Sonnet 5 --- clara_workflow/agent_instructions.md | 11 +- clara_workflow/stage1/extract.py | 57 +++++++++- tests/test_stage1_extract.py | 161 +++++++++++++++++++++++++++ 3 files changed, 223 insertions(+), 6 deletions(-) create mode 100644 tests/test_stage1_extract.py diff --git a/clara_workflow/agent_instructions.md b/clara_workflow/agent_instructions.md index b2357f3..c859719 100644 --- a/clara_workflow/agent_instructions.md +++ b/clara_workflow/agent_instructions.md @@ -3,6 +3,11 @@ You are verifying factual claims implied by routed CLARA targets against their cited references. +Ontology-agnostic: this runs against any OBO Foundry ontology's edit file, not +just CL. `CL_4033094`-style ids and "cell type" below are illustrative only — +substitute the term's own prefix and kind of entity (e.g. an anatomical +structure in UBERON) throughout. + Work one term at a time, but process **all routed targets for that term** together. @@ -107,9 +112,9 @@ For `ntr` targets: `definition_changes`). 2. Decompose its `value` into atomic assertions. 3. Tag each assertion as: - - `core` — the subject is the cell type itself + - `core` — the subject is the term itself - `background` — the subject is a molecule, gene, process, or external - concept rather than the cell type + concept rather than the term itself 4. Also take each `added` change from `relationship_changes` and convert it into one atomic `core` assertion. @@ -238,7 +243,7 @@ For every decomposable `textual_changes` entry: - One fact per assertion. - Split conjunctions ("secretes X, Y, and Z" → three assertions). - Split clauses that bundle identity + location + function. -- Preserve the cell-type subject in every `core` assertion so it stands alone. +- Preserve the term's subject in every `core` assertion so it stands alone. - Strip hedges ("crucial for", "key") but keep the factual core. - Do not invent claims the text does not make. - Use only the routed textual change's `value` as the prose source of truth. diff --git a/clara_workflow/stage1/extract.py b/clara_workflow/stage1/extract.py index c53157d..e0eb572 100644 --- a/clara_workflow/stage1/extract.py +++ b/clara_workflow/stage1/extract.py @@ -18,6 +18,7 @@ import dataclasses import json import os +import re import shutil import subprocess import sys @@ -43,6 +44,24 @@ def _git_show(repo: Path, ref: str, path: str, out: Path) -> None: ) +# Matches an OBO `import:` line, or an OWL functional-syntax `Import()` +# line (the two serializations edit files are known to use). +_IMPORT_LINE_RE = re.compile(r"^(import:\s|Import\()") + + +def _strip_imports(text: str) -> str: + """Drop owl:imports declarations before handing text to ROBOT. + + Stage 1 only ever needs the edit file's own asserted axioms, never the + merged import closure, and resolving imports pulls in large remote + ontologies on every review run — or fails outright if a PURL has moved. + (Confirmed against a live uberon-edit.obo import that currently 404s.) + """ + return "\n".join( + line for line in text.splitlines() if not _IMPORT_LINE_RE.match(line) + ) + "\n" + + def _robot_diff(left: Path, right: Path, out: Path, robot: str = "robot") -> None: subprocess.run( [ @@ -57,6 +76,30 @@ def _robot_diff(left: Path, right: Path, out: Path, robot: str = "robot") -> Non ) +# `definition_refs()` scans for the compact `obo:`/`oboInOwl:` curie form (it +# has to — that's what real edit files use), so a conversion must declare the +# same prefixes ROBOT doesn't add by default. +_OFN_PREFIXES = ( + "obo: http://purl.obolibrary.org/obo/", + "oboInOwl: http://www.geneontology.org/formats/oboInOwl#", +) + + +def _robot_convert_to_ofn(input_path: Path, output_path: Path, robot: str = "robot") -> None: + """Convert `input_path` to OWL functional syntax, in the curie form + `definition_refs()` expects, regardless of the input's own format. + + This is what lets `definition_refs()` work against an OBO edit file: it + only understands functional-syntax `AnnotationAssertion(...)` lines, so + non-OWL edit files (e.g. `uberon-edit.obo`) need converting first. + """ + cmd = [robot, "convert", "-i", str(input_path), "-f", "ofn"] + for prefix in _OFN_PREFIXES: + cmd += ["--add-prefix", prefix] + cmd += ["-o", str(output_path)] + subprocess.run(cmd, check=True) + + def _change_to_dict(c: Change) -> dict: d = dataclasses.asdict(c) d.pop("raw", None) # drop the debug field from serialised output @@ -78,15 +121,23 @@ def extract( """ with tempfile.TemporaryDirectory() as td: tdp = Path(td) + left_raw = tdp / "left.raw" + right_raw = tdp / "right.raw" left = tdp / "left.owl" right = tdp / "right.owl" + right_ofn = tdp / "right.ofn" diff_md = tdp / "diff.md" - _git_show(repo, left_ref, edit_file, left) - _git_show(repo, right_ref, edit_file, right) + _git_show(repo, left_ref, edit_file, left_raw) + _git_show(repo, right_ref, edit_file, right_raw) + left.write_text(_strip_imports(left_raw.read_text())) + right.write_text(_strip_imports(right_raw.read_text())) _robot_diff(left, right, diff_md, robot=robot) + # Read definition refs off a functional-syntax conversion rather than + # the right file directly, so this also works for non-OWL edit files. + _robot_convert_to_ofn(right, right_ofn, robot=robot) return ( parse_diff_markdown(diff_md.read_text()), - definition_refs(right.read_text()), + definition_refs(right_ofn.read_text()), ) diff --git a/tests/test_stage1_extract.py b/tests/test_stage1_extract.py new file mode 100644 index 0000000..86f7265 --- /dev/null +++ b/tests/test_stage1_extract.py @@ -0,0 +1,161 @@ +"""Tests for the stage-1 extractor's ROBOT-facing plumbing. + +Unlike test_stage1_parse.py (pure functions over cached fixtures), these +exercise the parts of extract.py that actually shell out to `robot`, so they +skip outright if `robot` isn't on PATH. +""" + +from __future__ import annotations + +import shutil +import subprocess +from pathlib import Path + +import pytest + +from clara_workflow.stage1.extract import ( + _robot_convert_to_ofn, + _strip_imports, + extract, +) +from clara_workflow.stage1.parse import definition_refs + +ROBOT = shutil.which("robot") +requires_robot = pytest.mark.skipif(ROBOT is None, reason="robot not on PATH") + + +# --- _strip_imports ---------------------------------------------------- + +def test_strip_imports_drops_obo_import_lines(): + text = ( + "format-version: 1.2\n" + "import: http://purl.obolibrary.org/obo/uberon/components/foo.owl\n" + "ontology: uberon/test\n" + ) + stripped = _strip_imports(text) + assert "import:" not in stripped + assert "ontology: uberon/test" in stripped + + +def test_strip_imports_drops_owl_functional_import_lines(): + text = ( + "Prefix(obo:=)\n" + "Ontology(\n" + "Import()\n" + "Declaration(Class(obo:CL_0000001))\n" + ")\n" + ) + stripped = _strip_imports(text) + assert "Import(" not in stripped + assert "Declaration(Class(obo:CL_0000001))" in stripped + + +def test_strip_imports_leaves_unrelated_lines_untouched(): + text = "def: \"An import-related structure.\" [GOC:test]\n" + assert _strip_imports(text) == text + + +# --- _robot_convert_to_ofn ---------------------------------------------- + +_OBO_TERM = """format-version: 1.2 +ontology: uberon/test + +[Term] +id: UBERON:0004177 +name: hemopoietic organ +def: "Organ that produces blood cells." [GOC:Obol, DOI:10.9999/test.uberon.4177] +""" + + +@requires_robot +def test_robot_convert_to_ofn_preserves_dbxrefs_from_obo(tmp_path: Path): + """OBO input, once converted, must still yield refs via definition_refs(). + + This is the crux of the OBO-support fix: definition_refs() only + understands functional-syntax AnnotationAssertion(...) lines, so an OBO + edit file needs converting first, with the obo:/oboInOwl: curie prefixes + it expects. + """ + src = tmp_path / "right.obo" + src.write_text(_OBO_TERM) + out = tmp_path / "right.ofn" + + _robot_convert_to_ofn(src, out, robot=ROBOT) + + refs = definition_refs(out.read_text()) + assert refs["UBERON:0004177"] == ["DOI:10.9999/test.uberon.4177", "GOC:Obol"] + + +# --- extract() end to end ------------------------------------------------- + +def _init_repo(tmp_path: Path) -> Path: + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q"], cwd=repo, check=True) + subprocess.run(["git", "config", "user.email", "test@example.org"], cwd=repo, check=True) + subprocess.run(["git", "config", "user.name", "Test"], cwd=repo, check=True) + return repo + + +@requires_robot +def test_extract_end_to_end_against_obo_edit_file_with_broken_import(tmp_path: Path): + """Regression test for two real-world failures found reviewing uberon: + + 1. `robot diff` fails outright on an edit file with an unresolvable + `import:` PURL, unless imports are stripped first. + 2. Without converting to functional syntax first, `definition_refs()` + silently returns nothing for an OBO edit file. + """ + repo = _init_repo(tmp_path) + edit_file = repo / "uberon-edit.obo" + + base = """format-version: 1.2 +ontology: uberon/test +import: http://purl.obolibrary.org/obo/uberon/components/does-not-exist.owl + +[Term] +id: UBERON:0004177 +name: hemopoietic organ +def: "Organ that is part of the hematopoietic system." [GOC:Obol] +""" + edit_file.write_text(base) + subprocess.run(["git", "add", "uberon-edit.obo"], cwd=repo, check=True) + subprocess.run(["git", "commit", "-q", "-m", "base"], cwd=repo, check=True) + base_sha = subprocess.run( + ["git", "rev-parse", "HEAD"], cwd=repo, check=True, capture_output=True, text=True + ).stdout.strip() + + revised = base.replace( + 'def: "Organ that is part of the hematopoietic system." [GOC:Obol]', + 'def: "Organ that produces, stores, or regulates the maturation of ' + 'blood cells, and is part of the hematopoietic system." ' + '[GOC:Obol, DOI:10.9999/test.uberon.4177]', + ) + edit_file.write_text(revised) + subprocess.run(["git", "add", "uberon-edit.obo"], cwd=repo, check=True) + subprocess.run(["git", "commit", "-q", "-m", "revise def"], cwd=repo, check=True) + head_sha = subprocess.run( + ["git", "rev-parse", "HEAD"], cwd=repo, check=True, capture_output=True, text=True + ).stdout.strip() + + changes, head_definition_refs = extract( + repo=repo, + left_ref=base_sha, + right_ref=head_sha, + edit_file="uberon-edit.obo", + robot=ROBOT, + ) + + added_defs = [c for c in changes if c.kind == "text_def" and c.side == "added"] + assert len(added_defs) == 1 + assert added_defs[0].term_id == "UBERON:0004177" + assert "DOI:10.9999/test.uberon.4177" in added_defs[0].refs + + # This term's definition wasn't left untouched, but the fallback path + # (used when a *different* axiom needs justifying from an existing, + # unchanged definition) must be populated too, proving OBO input reaches + # definition_refs() correctly rather than silently returning {}. + assert head_definition_refs.get("UBERON:0004177") == [ + "DOI:10.9999/test.uberon.4177", + "GOC:Obol", + ] From 9f0abfa36a7114156e8efff310d2b7d7a85ce36b Mon Sep 17 00:00:00 2001 From: ugur Date: Tue, 29 Sep 2026 14:09:48 +0100 Subject: [PATCH 2/2] Prefer full import resolution over stripped-imports fallback The previous commit always stripped owl:imports before running robot diff/convert. Verified against a real cell-ontology PR (3717) that this is a real regression, not just a style choice: stripping loses label resolution for any entity defined only in an import (e.g. BFO's `part of`, or an UBERON class referenced from cl-edit.owl), degrading axiom text the verification agent reads directly, e.g. "part of some mouth mucosa" -> "BFO_0000050 some UBERON_0003729" Fixed by trying the full import graph first in both _robot_diff_with_import_fallback() and _robot_convert_to_ofn_with_import_fallback(), only falling back to import-stripped copies if that attempt fails outright (a moved PURL, uberon's current situation -- being fixed separately on that side, but this fallback stays as a safety net for whenever an import PURL moves again, on any ontology). Verified against real data both ways: - cell-ontology PR 3717 (d7ff69b9..51ef2001): output now byte-identical to pre-fix behavior (predicate_label, value_label, Manchester-syntax axiom text, and ref sets all match) -- confirms no regression for CL, where imports currently resolve fine. - The existing broken-import end-to-end test (OBO edit file with a dangling import: PURL) still passes via the fallback path. Added two hermetic tests (mocked subprocess, no network) asserting the fallback only strips after a real failure, and never strips when the first attempt succeeds -- so this ordering can't silently regress back to "always strip". Full suite: 46 passed. Co-Authored-By: Claude Sonnet 5 --- clara_workflow/stage1/extract.py | 67 +++++++++++++++++++++++++------- tests/test_stage1_extract.py | 60 ++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 14 deletions(-) diff --git a/clara_workflow/stage1/extract.py b/clara_workflow/stage1/extract.py index e0eb572..d1545a6 100644 --- a/clara_workflow/stage1/extract.py +++ b/clara_workflow/stage1/extract.py @@ -50,12 +50,15 @@ def _git_show(repo: Path, ref: str, path: str, out: Path) -> None: def _strip_imports(text: str) -> str: - """Drop owl:imports declarations before handing text to ROBOT. - - Stage 1 only ever needs the edit file's own asserted axioms, never the - merged import closure, and resolving imports pulls in large remote - ontologies on every review run — or fails outright if a PURL has moved. - (Confirmed against a live uberon-edit.obo import that currently 404s.) + """Drop owl:imports declarations from edit-file text. + + Used only as a fallback when ROBOT fails to resolve the real import + graph (e.g. a moved PURL) — not the default path. Stripping unconditionally + would also strip label resolution for entities defined only in an import + (e.g. BFO's `part of`, or an UBERON class referenced from cl-edit.owl), + degrading axiom text like `part of some mouth mucosa` down to + `BFO_0000050 some UBERON_0003729`. That's a real quality loss for the + downstream verification agent, so this is a last resort, not a default. """ return "\n".join( line for line in text.splitlines() if not _IMPORT_LINE_RE.match(line) @@ -76,6 +79,26 @@ def _robot_diff(left: Path, right: Path, out: Path, robot: str = "robot") -> Non ) +def _robot_diff_with_import_fallback( + left: Path, right: Path, out: Path, robot: str = "robot" +) -> None: + """Diff with the edit files as-is first (full import graph -> real + labels); only fall back to import-stripped copies if that fails (e.g. a + moved PURL). Keeps today's label quality for every repo whose imports + still resolve, while not hard-failing for one that doesn't. + """ + try: + _robot_diff(left, right, out, robot=robot) + return + except subprocess.CalledProcessError: + pass + stripped_left = left.with_name(left.name + ".noimports") + stripped_right = right.with_name(right.name + ".noimports") + stripped_left.write_text(_strip_imports(left.read_text())) + stripped_right.write_text(_strip_imports(right.read_text())) + _robot_diff(stripped_left, stripped_right, out, robot=robot) + + # `definition_refs()` scans for the compact `obo:`/`oboInOwl:` curie form (it # has to — that's what real edit files use), so a conversion must declare the # same prefixes ROBOT doesn't add by default. @@ -100,6 +123,23 @@ def _robot_convert_to_ofn(input_path: Path, output_path: Path, robot: str = "rob subprocess.run(cmd, check=True) +def _robot_convert_to_ofn_with_import_fallback( + input_path: Path, output_path: Path, robot: str = "robot" +) -> None: + """Same fallback strategy as `_robot_diff_with_import_fallback`: definitions' + own dbxrefs never depend on an import, so stripping only kicks in when the + full-import conversion fails outright. + """ + try: + _robot_convert_to_ofn(input_path, output_path, robot=robot) + return + except subprocess.CalledProcessError: + pass + stripped = input_path.with_name(input_path.name + ".noimports") + stripped.write_text(_strip_imports(input_path.read_text())) + _robot_convert_to_ofn(stripped, output_path, robot=robot) + + def _change_to_dict(c: Change) -> dict: d = dataclasses.asdict(c) d.pop("raw", None) # drop the debug field from serialised output @@ -121,20 +161,19 @@ def extract( """ with tempfile.TemporaryDirectory() as td: tdp = Path(td) - left_raw = tdp / "left.raw" - right_raw = tdp / "right.raw" left = tdp / "left.owl" right = tdp / "right.owl" right_ofn = tdp / "right.ofn" diff_md = tdp / "diff.md" - _git_show(repo, left_ref, edit_file, left_raw) - _git_show(repo, right_ref, edit_file, right_raw) - left.write_text(_strip_imports(left_raw.read_text())) - right.write_text(_strip_imports(right_raw.read_text())) - _robot_diff(left, right, diff_md, robot=robot) + _git_show(repo, left_ref, edit_file, left) + _git_show(repo, right_ref, edit_file, right) + # Full import graph first (real labels on cross-ontology references); + # only degrade to import-stripped copies if that fails outright (e.g. + # a moved PURL) -- see _robot_diff_with_import_fallback. + _robot_diff_with_import_fallback(left, right, diff_md, robot=robot) # Read definition refs off a functional-syntax conversion rather than # the right file directly, so this also works for non-OWL edit files. - _robot_convert_to_ofn(right, right_ofn, robot=robot) + _robot_convert_to_ofn_with_import_fallback(right, right_ofn, robot=robot) return ( parse_diff_markdown(diff_md.read_text()), definition_refs(right_ofn.read_text()), diff --git a/tests/test_stage1_extract.py b/tests/test_stage1_extract.py index 86f7265..5ff2a77 100644 --- a/tests/test_stage1_extract.py +++ b/tests/test_stage1_extract.py @@ -15,6 +15,7 @@ from clara_workflow.stage1.extract import ( _robot_convert_to_ofn, + _robot_diff_with_import_fallback, _strip_imports, extract, ) @@ -55,6 +56,65 @@ def test_strip_imports_leaves_unrelated_lines_untouched(): assert _strip_imports(text) == text +# --- _robot_diff_with_import_fallback ------------------------------------- +# +# Hermetic (mocked subprocess) regression tests for the fallback ordering +# itself. Stripping imports loses label resolution for any entity defined +# only in an import (e.g. BFO's `part of`, or an UBERON class referenced from +# cl-edit.owl) -- confirmed by comparing real output on a live CL PR before +# and after this fix. So stripping must be a last resort, never the default: +# these tests fail loudly if that ordering regresses back to "always strip". + +def test_prefers_full_imports_when_diff_succeeds(tmp_path, monkeypatch): + calls: list[list[str]] = [] + + def fake_run(cmd, check): + calls.append(cmd) + Path(cmd[cmd.index("--output") + 1]).write_text("ok") + + monkeypatch.setattr(subprocess, "run", fake_run) + + left = tmp_path / "left.owl" + right = tmp_path / "right.owl" + left.write_text("import: http://example.org/foo.owl\nfoo\n") + right.write_text("import: http://example.org/foo.owl\nbar\n") + out = tmp_path / "diff.md" + + _robot_diff_with_import_fallback(left, right, out, robot="robot") + + assert len(calls) == 1, "must not strip imports when the first attempt succeeds" + assert str(left) in calls[0] and str(right) in calls[0] + assert not (tmp_path / "left.owl.noimports").exists() + assert out.read_text() == "ok" + + +def test_falls_back_to_stripped_copies_only_after_a_failure(tmp_path, monkeypatch): + calls: list[list[str]] = [] + + def fake_run(cmd, check): + calls.append(cmd) + if len(calls) == 1: + raise subprocess.CalledProcessError(1, cmd) + Path(cmd[cmd.index("--output") + 1]).write_text("ok") + + monkeypatch.setattr(subprocess, "run", fake_run) + + left = tmp_path / "left.owl" + right = tmp_path / "right.owl" + left.write_text("import: http://example.org/broken.owl\nfoo\n") + right.write_text("import: http://example.org/broken.owl\nbar\n") + out = tmp_path / "diff.md" + + _robot_diff_with_import_fallback(left, right, out, robot="robot") + + assert len(calls) == 2 + # First attempt used the original, import-carrying files. + assert str(left) in calls[0] and str(right) in calls[0] + # Retry used stripped copies, not the originals. + assert str(left) not in calls[1] and str(right) not in calls[1] + assert out.read_text() == "ok" + + # --- _robot_convert_to_ofn ---------------------------------------------- _OBO_TERM = """format-version: 1.2