Repository navigation
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
ubyndr
marked this pull request as draft
September 29, 2026 13:21
Contributor
Author
|
Closing for now — rolling this back while we reconsider approach. Will reopen or redo if we pick this back up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while integrating this package into uberon (obophenotype/uberon#3778, obophenotype/uberon#3779).
definition_refs()silently returned nothing for OBO edit files (it only parses OWL functional-syntaxAnnotationAssertion(...)lines;extract()fed it raw OBO text unconverted). Fixed by converting the right-side file to functional syntax (robot convert -f ofn, withobo:/oboInOwl:prefixes) before parsing.robot difffailed outright against a real uberon edit file because one of itsimport:PURLs currently 404s (that PURL is being fixed separately on the uberon side). ROBOT resolves everyowl:importsby default, and a broken one aborts the whole run._robot_diff_with_import_fallback()/_robot_convert_to_ofn_with_import_fallback()now try the full import graph first, and only fall back to import-stripped copies if that fails. Full imports first matters: stripping loses label resolution for anything defined only in an import (e.g. BFO'spart of, or an UBERON class referenced fromcl-edit.owl), which degrades axiom text the verification agent reads directly (part of some mouth mucosa→BFO_0000050 some UBERON_0003729). Verified this isn't just theoretical by re-running a real CL PR (3717) both ways — full-import-first output is now byte-identical to pre-fix behavior (labels, axiom text, ref sets all match); only the import-stripped fallback path degrades labels, and that only triggers when resolution genuinely fails.agent_instructions.mdtold the agent to treat the subject as "the cell type" in a few spots that shape actual assertion wording. Generalized to "the term" / "the term's subject", with a note thatCL_...ids and "cell type" elsewhere are illustrative, not CL-only. Left thecell_idfield name and otherCL_...examples as-is — they're internal/illustrative and don't leak into assertion content, and renaming would be a breaking schema change for existing consumers.tests/test_stage1_extract.py(7 tests):_strip_imports(),_robot_convert_to_ofn(), two hermetic (mocked subprocess) tests asserting the fallback only strips after a real failure and never strips when the first attempt succeeds, and an end-to-endextract()run against a throwaway git repo with an OBO file carrying a deliberately broken import. Full suite: 46 passed. No changes to existing CL fixtures/tests, no consumer-visible schema changes.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com