diff --git a/CHANGELOG.md b/CHANGELOG.md index bcace0fb..ae6ccf95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`ContextGraph.add_edge` had no dedupe — identical edges were stored repeatedly under one shared edge ID, and re-ingest doubled the edge set** (#926, closes #922) by @pravit-amp + - `_add_internal_edge` appended to `self.edges`, `edge_type_index`, and `_adjacency` unconditionally, with no check for an edge already present. Edge identity is content-derived (`_resolve_edge_identity` builds `edge_id` from `source_id`/`target_id`/`edge_type`/`weight`/`metadata`/`valid_from`/`valid_until`), so two identical `add_edge` calls produced two edge objects sharing one `edge_id` — the graph already considered them the same edge, it just kept both copies. `self.nodes` already deduped by ID; edges did not, so `stats()["edge_count"]` inflated, `density()` could exceed its mathematical maximum of `1.0`, and a refresh/restore job calling `build_from_entities_and_relationships()` (or reloading a saved graph) doubled the edge set on every cycle + - Added an `edge_id -> ContextEdge` index (`_edge_index`), mirroring how `self.nodes` dedupes by node ID. `_add_internal_edge` now returns `False` when the `edge_id` already exists, checked before touching `edges`/`edge_type_index`/`_adjacency` and before firing the mutation callback, so a repeat `add_edge` is a silent no-op with no phantom `ADD_EDGE` audit event + - Genuinely parallel edges are unaffected: differing type/weight/metadata/validity still produce distinct content-derived `edge_id`s, so multigraph semantics are preserved + - Both state-reset paths (`load_from_file()` and `clear()`) also clear `_edge_index` + - New tests: repeat `add_edge` is a no-op, parallel edges with distinct attributes are preserved, re-ingest via `build_from_entities_and_relationships()` stays at one edge, and `clear()` resets the dedupe index + - `pytest tests/context/test_context.py`: 31 passed + - **`POST /api/enrich/extract` returned 503 on every request; the whole `/api/decisions*` family returned 500 as soon as a decision existed** (#886, closes #883, closes #884, closes #889) by @joseedson18jc, reviewed by @Sameer6305 - `semantica/explorer/routes/enrich.py` imported `extract_entities`/`extract_relations` from `semantica.semantic_extract.methods`, names that module never defined (only per-strategy variants like `extract_entities_ml` exist) — the `except ImportError` handler reported this as `"semantic_extract module not available"`, masking a wiring bug as a missing dependency. The route now calls `NamedEntityRecognizer`/`RelationExtractor` directly and forwards extracted entities into relation extraction instead of re-deriving them - `ContextGraph.record_decision()` stores `timestamp` as `datetime.now().timestamp()` (a float), while `DecisionResponse.timestamp` was typed `Optional[str]`; passing the value through unconverted failed pydantic validation on every decision route (`/api/decisions`, `/{id}`, `/{id}/chain`, `/{id}/precedents`, `/{id}/compliance`). Added a `field_validator(mode="before")` on `DecisionResponse` normalizing float/int/datetime inputs to ISO-8601 diff --git a/semantica/context/context_graph.py b/semantica/context/context_graph.py index df7c71be..4cc66e70 100644 --- a/semantica/context/context_graph.py +++ b/semantica/context/context_graph.py @@ -464,6 +464,7 @@ def __init__(self, config: Optional[Dict[str, Any]] = None, **kwargs): self.nodes: Dict[str, ContextNode] = {} self.edges: List[ContextEdge] = [] + self._edge_index: Dict[str, ContextEdge] = {} self._adjacency: Dict[str, List[ContextEdge]] = defaultdict(list) @@ -1120,6 +1121,7 @@ def load_from_file(self, path: str) -> None: # Clear existing self.nodes.clear() self.edges.clear() + self._edge_index.clear() self._adjacency.clear() self.node_type_index.clear() self.edge_type_index.clear() @@ -1523,6 +1525,7 @@ def clear(self) -> None: with self._lock: self.nodes.clear() self.edges.clear() + self._edge_index.clear() self._adjacency.clear() self.node_type_index.clear() self.edge_type_index.clear() @@ -1595,6 +1598,11 @@ def _add_internal_edge(self, edge: ContextEdge) -> bool: self.logger.warning("Skipping internal edge with invalid endpoints: %r", edge) return False with self._lock: + # Edge identity is content-derived, so an existing edge_id means this + # exact edge is already stored; re-adding it is a no-op (issue #922). + if edge.edge_id in self._edge_index: + return False + # Ensure nodes exist if edge.source_id not in self.nodes: self._add_internal_node( @@ -1605,6 +1613,7 @@ def _add_internal_edge(self, edge: ContextEdge) -> bool: ContextNode(edge.target_id, "entity", edge.target_id) ) + self._edge_index[edge.edge_id] = edge self.edges.append(edge) self.edge_type_index[edge.edge_type].append(edge) self._adjacency[edge.source_id].append(edge) diff --git a/tests/context/test_context.py b/tests/context/test_context.py index dd9a706f..45000912 100644 --- a/tests/context/test_context.py +++ b/tests/context/test_context.py @@ -109,6 +109,61 @@ def test_context_graph_operations(self): self.assertEqual(neighbors[0]["id"], "n2") self.assertEqual(neighbors[0]["relationship"], "knows") + def test_add_edge_is_idempotent(self): + graph = ContextGraph() + graph.add_node("a", "t") + graph.add_node("b", "t") + + self.assertTrue(graph.add_edge("a", "b", "rel")) + self.assertFalse(graph.add_edge("a", "b", "rel")) + self.assertFalse(graph.add_edge("a", "b", "rel")) + + self.assertEqual(len(graph.edges), 1) + self.assertEqual(len(graph.edge_type_index["rel"]), 1) + self.assertEqual(len(graph._adjacency["a"]), 1) + self.assertEqual(graph.stats()["edge_count"], 1) + self.assertLessEqual(graph.density(), 1.0) + + def test_parallel_edges_with_distinct_attributes_are_kept(self): + graph = ContextGraph() + graph.add_node("a", "t") + graph.add_node("b", "t") + + graph.add_edge("a", "b", "rel", confidence=0.9) + graph.add_edge("a", "b", "rel", confidence=0.5) + graph.add_edge("a", "b", "other") + + self.assertEqual(len(graph.edges), 3) + self.assertEqual(len({e.edge_id for e in graph.edges}), 3) + + def test_reingest_does_not_duplicate_edges(self): + graph = ContextGraph() + entities = [ + {"id": "alice", "type": "person"}, + {"id": "acme", "type": "org"}, + ] + relationships = [ + {"source_id": "alice", "target_id": "acme", "type": "works_at"} + ] + + for _ in range(3): + graph.build_from_entities_and_relationships(entities, relationships) + + self.assertEqual(len(graph.edges), 1) + + def test_clear_resets_edge_dedupe_index(self): + graph = ContextGraph() + graph.add_node("a", "t") + graph.add_node("b", "t") + graph.add_edge("a", "b", "rel") + + graph.clear() + + graph.add_node("a", "t") + graph.add_node("b", "t") + self.assertTrue(graph.add_edge("a", "b", "rel")) + self.assertEqual(len(graph.edges), 1) + def test_get_nodes_by_label_returns_metadata_copy(self): graph = ContextGraph() graph.add_node("n1", "person", "Alice", role="engineer")