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..d1545a6 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,27 @@ 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 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) + ) + "\n" + + def _robot_diff(left: Path, right: Path, out: Path, robot: str = "robot") -> None: subprocess.run( [ @@ -57,6 +79,67 @@ 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. +_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 _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 @@ -80,13 +163,20 @@ def extract( tdp = Path(td) 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) - _robot_diff(left, right, diff_md, robot=robot) + # 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_with_import_fallback(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..5ff2a77 --- /dev/null +++ b/tests/test_stage1_extract.py @@ -0,0 +1,221 @@ +"""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, + _robot_diff_with_import_fallback, + _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_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 +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", + ]