From bc75768afe1f3a991fc5e1d0e8919ae8f3a10a34 Mon Sep 17 00:00:00 2001 From: KaifAhmad1 Date: Sat, 1 Aug 2026 11:46:13 +0530 Subject: [PATCH] Fix two review findings in SKOS cycle validation - validate_skos_hierarchy() re-walked every existing hierarchy edge in the graph on each write, so one pre-existing cycle anywhere would block all unrelated future SKOS writes. It now only traverses concepts touched by the edges actually being written, while still checking those against existing edges for cross-boundary cycles. - In /api/ontology/load, `except HTTPException: raise` sat after a broader `except Exception` clause that already matched HTTPException, so a 422 raised after a successful OntologyIngestor parse was silently swallowed and retried via the fallback RDF parser instead of reaching the caller. Reordered the except clauses. Co-authored-by: mikemikimike <13286568797@163.com> --- CHANGELOG.md | 2 + semantica/context/context_graph.py | 4 +- semantica/explorer/routes/ontology.py | 10 ++- semantica/explorer/session.py | 2 +- semantica/utils/skos.py | 76 +++++++++++++++-------- tests/context/test_context.py | 13 ++++ tests/explorer/test_ontology_subissue3.py | 37 +++++++++++ tests/utils/test_skos.py | 64 +++++++++++++++++++ 8 files changed, 176 insertions(+), 32 deletions(-) create mode 100644 tests/utils/test_skos.py diff --git a/CHANGELOG.md b/CHANGELOG.md index bc00cef6..b9649c8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added cycle detection (`validate_skos_hierarchy`) for `skos:broader` and `skos:narrower` relationships in `ContextGraph.add_edge()` and `ContextGraph.add_edges()`, preventing direct 2-node cycles, self-loops, and multi-hop hierarchy cycles - Added `GraphSession.add_nodes_and_edges()` to validate SKOS hierarchy edges upfront under lock before node insertion, preventing partial-write leaks where nodes remain after a cyclic edge is rejected - Updated vocabulary, ontology (`/api/ontology/load`, `/api/ontology/create`), and JSON/CSV import routes to use `add_nodes_and_edges()` and return HTTP 422 with actionable error messages when a cycle is detected + - Follow-up fix by @KaifAhmad1: `validate_skos_hierarchy()` previously re-walked *every* SKOS hierarchy edge already in the graph on each write, so one pre-existing cycle anywhere (e.g. legacy data) blocked all unrelated future writes; it now only traverses concepts touched by the edges being written, while still checking against existing edges for cycles that span old and new data + - Follow-up fix by @KaifAhmad1: in `/api/ontology/load`, `except HTTPException: raise` was unreachable because a broader `except Exception` clause above it already matched `HTTPException`, so a 422 raised after a successful `OntologyIngestor` parse was silently swallowed and reprocessed via the fallback RDF parser; reordered the clauses so the deliberate 422 always propagates - **`AgnoDecisionKit`/`AgnoKGToolkit` silently swallowed Agno tool registration failures** (#780, #818) by @Sameer6305 and @KaifAhmad1 - Removed the `try/except: pass` wrapped around `self.register(fn)` in both toolkits' `__init__`; when Agno is installed, a registration failure now propagates immediately instead of leaving the toolkit half-registered with no signal to the caller diff --git a/semantica/context/context_graph.py b/semantica/context/context_graph.py index f5ef322e..ebfc0464 100644 --- a/semantica/context/context_graph.py +++ b/semantica/context/context_graph.py @@ -597,7 +597,7 @@ class ContextGraph: existing_edges = [ edge for edge in self.find_edges() if is_skos_hierarchy_edge(edge) ] - validate_skos_hierarchy([*existing_edges, *hierarchy_edges]) + validate_skos_hierarchy(hierarchy_edges, existing_edges) for raw_edge in edges: if not isinstance(raw_edge, dict): continue @@ -962,7 +962,7 @@ class ContextGraph: existing_edges = [ edge for edge in self.find_edges() if is_skos_hierarchy_edge(edge) ] - validate_skos_hierarchy([*existing_edges, candidate]) + validate_skos_hierarchy([candidate], existing_edges) return self._add_internal_edge( ContextEdge( edge_id=edge_id, diff --git a/semantica/explorer/routes/ontology.py b/semantica/explorer/routes/ontology.py index 2fac4c15..a9cce469 100644 --- a/semantica/explorer/routes/ontology.py +++ b/semantica/explorer/routes/ontology.py @@ -1339,15 +1339,19 @@ async def load_ontology( except OSError as cleanup_exc: logger.debug("Failed to remove temporary ontology file: %s", cleanup_exc) + except HTTPException: + # Re-raise HTTPExceptions we deliberately raised above (e.g. the 422 from + # SKOS cycle validation) instead of letting the broad `except Exception` + # below mask them as an ingestor failure and silently retry via the + # fallback parser. + raise except Exception as ingest_exc: logger.warning(f"OntologyIngestor failed, falling back to basic parsing: {ingest_exc}") - + # Fallback to basic parsing nodes, edges, metadata = await asyncio.to_thread( _parse_rdf_sync, content_str.encode("utf-8"), fmt ) - except HTTPException: - raise except Exception as exc: raise HTTPException(status_code=422, detail=f"Could not parse ontology: {exc}") from exc diff --git a/semantica/explorer/session.py b/semantica/explorer/session.py index 369d326f..1d0a60f9 100644 --- a/semantica/explorer/session.py +++ b/semantica/explorer/session.py @@ -754,7 +754,7 @@ class GraphSession: existing_edges = [ edge for edge in self.graph.find_edges() if is_skos_hierarchy_edge(edge) ] - validate_skos_hierarchy([*existing_edges, *hierarchy_edges]) + validate_skos_hierarchy(hierarchy_edges, existing_edges) def add_nodes_and_edges( self, diff --git a/semantica/utils/skos.py b/semantica/utils/skos.py index 8c24bba5..dd105bca 100644 --- a/semantica/utils/skos.py +++ b/semantica/utils/skos.py @@ -17,41 +17,65 @@ def is_skos_hierarchy_edge(edge: Mapping[str, object]) -> bool: ) -def validate_skos_hierarchy(edges: Iterable[Mapping[str, object]]) -> None: - """Raise ``ValueError`` when SKOS hierarchy edges contain a cycle. +def _child_parent(edge: Mapping[str, object]) -> tuple[str, str] | None: + """Normalize a SKOS hierarchy edge to a ``(child, parent)`` pair, or ``None``.""" + if not isinstance(edge, Mapping) or not is_skos_hierarchy_edge(edge): + return None + edge_type = next( + ( + edge.get(key) + for key in ("type", "edge_type", "relationship", "predicate", "relation") + if edge.get(key) in _HIERARCHY_EDGE_TYPES + ), + None, + ) + if edge_type not in _HIERARCHY_EDGE_TYPES: + return None + + raw_source = edge.get("source", edge.get("source_id")) + raw_target = edge.get("target", edge.get("target_id")) + if raw_source is None or raw_target is None: + return None + source = str(raw_source).strip() + target = str(raw_target).strip() + if not source or not target: + return None + + return (source, target) if edge_type == "skos:broader" else (target, source) + + +def validate_skos_hierarchy( + new_edges: Iterable[Mapping[str, object]], + existing_edges: Iterable[Mapping[str, object]] = (), +) -> None: + """Raise ``ValueError`` when adding ``new_edges`` would introduce a cycle. ``skos:broader`` points from a concept to its parent while ``skos:narrower`` expresses the same relationship in the opposite direction. Both forms are normalized to child-to-parent adjacency before cycle detection. + + ``existing_edges`` supplies the SKOS hierarchy edges already persisted in + the graph so that cycles spanning old and new edges are still caught. + Only the concepts touched by ``new_edges`` are checked, though: a cycle + that already exists entirely within ``existing_edges`` must not block an + unrelated write elsewhere in the graph. """ - parents: dict[str, set[str]] = defaultdict(set) - for edge in edges: - if not isinstance(edge, Mapping) or not is_skos_hierarchy_edge(edge): - continue - edge_type = next( - ( - edge.get(key) - for key in ("type", "edge_type", "relationship", "predicate", "relation") - if edge.get(key) in _HIERARCHY_EDGE_TYPES - ), - None, - ) - if edge_type not in _HIERARCHY_EDGE_TYPES: - continue + for edge in existing_edges: + pair = _child_parent(edge) + if pair is not None: + parents[pair[0]].add(pair[1]) - raw_source = edge.get("source", edge.get("source_id")) - raw_target = edge.get("target", edge.get("target_id")) - if raw_source is None or raw_target is None: + touched: set[str] = set() + for edge in new_edges: + pair = _child_parent(edge) + if pair is None: continue - source = str(raw_source).strip() - target = str(raw_target).strip() - if not source or not target: - continue - - child, parent = ((source, target) if edge_type == "skos:broader" else (target, source)) + child, parent = pair parents[child].add(parent) + touched.add(child) + touched.add(parent) visiting: set[str] = set() visited: set[str] = set() @@ -68,5 +92,5 @@ def validate_skos_hierarchy(edges: Iterable[Mapping[str, object]]) -> None: visiting.remove(concept) visited.add(concept) - for concept in parents: + for concept in touched: visit(concept) diff --git a/tests/context/test_context.py b/tests/context/test_context.py index 15ec1b49..73edaa4e 100644 --- a/tests/context/test_context.py +++ b/tests/context/test_context.py @@ -223,6 +223,19 @@ class TestContextModule(unittest.TestCase): self.assertEqual(len(graph.edges), 0) + def test_context_graph_preexisting_unrelated_cycle_does_not_block_new_write(self): + """A cycle already persisted elsewhere in the graph (e.g. legacy data + written before cycle detection existed) must not poison unrelated + SKOS hierarchy writes for concepts it doesn't touch.""" + from semantica.context.context_graph import ContextEdge + + graph = ContextGraph() + graph._add_internal_edge(ContextEdge(source_id="X", target_id="Y", edge_type="skos:broader")) + graph._add_internal_edge(ContextEdge(source_id="Y", target_id="X", edge_type="skos:broader")) + + self.assertTrue(graph.add_edge("C", "D", "skos:broader")) + self.assertEqual(len(graph.edges), 3) + # --- AgentContext Tests --- @patch('semantica.context.agent_memory.AgentMemory._generate_embedding') def test_agent_context_end_to_end(self, mock_gen_embedding): diff --git a/tests/explorer/test_ontology_subissue3.py b/tests/explorer/test_ontology_subissue3.py index 40cb6bbe..a106d6d7 100644 --- a/tests/explorer/test_ontology_subissue3.py +++ b/tests/explorer/test_ontology_subissue3.py @@ -658,3 +658,40 @@ ex:B a skos:Concept ; skos:prefLabel "Beta" ; skos:inScheme ex:S ; skos:broader assert "cycle" in response.json()["detail"].lower() +def test_ontology_load_does_not_swallow_422_from_ingestor_success_path(client): + """A ValueError raised by add_nodes_and_edges() after OntologyIngestor + succeeds must surface as its own 422, not be masked by the broad + `except Exception` fallback-to-basic-parsing handler and silently + retried under a different parser.""" + from semantica.ingest.ontology_ingestor import OntologyData + + fake_data = OntologyData( + data={ + "uri": "http://example.org/onto-fake", + "name": "Fake Ontology", + "classes": [{"uri": "http://example.org/onto-fake#A", "name": "A"}], + "properties": [], + }, + source_path="fake.ttl", + format="turtle", + ) + + with patch( + "semantica.ingest.ontology_ingestor.OntologyIngestor.ingest_ontology", + return_value=fake_data, + ), patch( + "semantica.explorer.session.GraphSession.add_nodes_and_edges", + side_effect=ValueError("SKOS hierarchy contains a cycle involving 'A'."), + ), patch( + "semantica.explorer.routes.ontology._parse_rdf_sync" + ) as fallback_parse: + response = client.post( + "/api/ontology/load", + json={"content": "@prefix ex: . ex:A a ex:Thing .", "format": "turtle"}, + ) + + assert response.status_code == 422 + assert "cycle" in response.json()["detail"].lower() + fallback_parse.assert_not_called() + + diff --git a/tests/utils/test_skos.py b/tests/utils/test_skos.py new file mode 100644 index 00000000..ed28a6f0 --- /dev/null +++ b/tests/utils/test_skos.py @@ -0,0 +1,64 @@ +import unittest + +from semantica.utils.skos import is_skos_hierarchy_edge, validate_skos_hierarchy + + +class TestIsSkosHierarchyEdge(unittest.TestCase): + + def test_recognizes_broader_and_narrower(self): + self.assertTrue(is_skos_hierarchy_edge({"source": "A", "target": "B", "type": "skos:broader"})) + self.assertTrue(is_skos_hierarchy_edge({"source": "A", "target": "B", "type": "skos:narrower"})) + + def test_ignores_other_edge_types(self): + self.assertFalse(is_skos_hierarchy_edge({"source": "A", "target": "B", "type": "rdfs:subClassOf"})) + + def test_ignores_non_mapping(self): + self.assertFalse(is_skos_hierarchy_edge("not-a-dict")) + + +class TestValidateSkosHierarchy(unittest.TestCase): + + def test_accepts_acyclic_chain(self): + validate_skos_hierarchy([ + {"source": "A", "target": "B", "type": "skos:broader"}, + {"source": "B", "target": "C", "type": "skos:broader"}, + ]) + + def test_rejects_self_loop(self): + with self.assertRaisesRegex(ValueError, "cycle"): + validate_skos_hierarchy([{"source": "A", "target": "A", "type": "skos:broader"}]) + + def test_rejects_direct_two_node_cycle(self): + with self.assertRaisesRegex(ValueError, "cycle"): + validate_skos_hierarchy([ + {"source": "A", "target": "B", "type": "skos:broader"}, + {"source": "B", "target": "A", "type": "skos:broader"}, + ]) + + def test_rejects_cycle_spanning_existing_and_new_edges(self): + existing = [ + {"source": "A", "target": "B", "type": "skos:broader"}, + {"source": "B", "target": "C", "type": "skos:broader"}, + ] + with self.assertRaisesRegex(ValueError, "cycle"): + validate_skos_hierarchy([{"source": "C", "target": "A", "type": "skos:broader"}], existing) + + def test_preexisting_unrelated_cycle_does_not_block_new_write(self): + """A cycle already persisted elsewhere in the graph (e.g. legacy data + written before cycle detection existed) must not poison unrelated + writes for concepts it doesn't touch.""" + existing = [ + {"source": "X", "target": "Y", "type": "skos:broader"}, + {"source": "Y", "target": "X", "type": "skos:broader"}, + ] + validate_skos_hierarchy([{"source": "C", "target": "D", "type": "skos:broader"}], existing) + + def test_none_and_blank_endpoints_are_ignored(self): + validate_skos_hierarchy([ + {"source": None, "target": "B", "type": "skos:broader"}, + {"source": " ", "target": "B", "type": "skos:broader"}, + ]) + + +if __name__ == "__main__": + unittest.main()