fix(context): clarify get_node_property not-found contract (#877) (#882)

* fix(context): clarify get_node_property not-found contract (#877)

Add default= param to get_node_property and get_node_attributes so
callers can distinguish node-missing from property-missing using a
sentinel. Fix add_node_attribute calling mutation_callback outside
the lock. Tests added for all cases.

* fix(context): address Qodo review findings (#877)

* fix(context): wrap add_node_attribute mutation_callback in try/except (#877)

The PR claimed to move the callback back inside `with self._lock`, but
the diff only dropped a stray blank line -- the call stayed outside the
lock, unchanged. That's actually correct: self._lock is an RLock, and
_add_internal_node/_add_internal_edge deliberately release the lock
before invoking the callback too, so a slow/misbehaving callback never
holds up other threads. The real gap was that, unlike those two
siblings, this call site didn't catch exceptions from the callback.
Wrapped it the same way, with a regression test.

---------

Co-authored-by: Mohd Kaif <98801504+KaifAhmad1@users.noreply.github.com>
Co-authored-by: KaifAhmad1 <kaifahmad087@gmail.com>

CHANGELOG.md

@@ -41,6 +41,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **`ContextGraph.get_node_property`/`get_node_attributes` "not found" contract clarified; `add_node_attribute` mutation-callback exception safety fixed** (#882, closes #877) by @ZohaibHassan16
+  - `get_node_property` returned `None` for both "node missing" and "property missing" with no way to distinguish them, and `get_node_attributes` returned `{}` for a missing node while its siblings disagreed on the not-found signal (`get_node_property`/`find_node` → `None`, `get_edge_data` → `{}`). Both now accept a `default=` parameter matching `dict.get()`'s convention, defaulting to their historical return values (`None` and `{}` respectively) for backward compatibility. Callers that need to disambiguate "node missing" from "value legitimately absent" can pass a private sentinel as `default`
+  - Added Google-style docstrings to `get_node_property`, `get_node_attributes`, `get_edge_data`, and `find_node` documenting each method's not-found contract, addressing #877's "sibling not-found contract undocumented" gap
+  - **Corrected during review**: the PR as submitted claimed to fix `add_node_attribute` firing its `mutation_callback` "outside `with self._lock`, without holding the lock," but the diff only removed a stray blank line — the callback call remained outside the lock, unchanged. Further investigation found this was not actually a bug: `self._lock` is a `threading.RLock`, and the same release-the-lock-before-invoking-the-callback pattern is used deliberately in `_add_internal_node`/`_add_internal_edge` elsewhere in this class, avoiding holding the lock for the duration of an arbitrary user-supplied callback. The real inconsistency was that, unlike those two siblings, `add_node_attribute`'s callback call wasn't wrapped in `try/except` — a raising callback propagated uncaught here but was caught and logged there. Now wrapped the same way (`except Exception as e: self.logger.warning(...)`)
+  - 13 tests covering happy path, missing node, missing property, sentinel disambiguation, falsy-zero, callback firing/non-firing, and (added during review) a raising callback no longer propagating out of `add_node_attribute`
+  - `pytest tests/context/test_context.py -q`: 27 passed
+
 - **Three `tests/normalize/` tests failed for reasons unrelated to the normalize implementations: a missing optional-dependency skip guard, an incomplete chardet allowlist, and a UTC/local timezone mismatch** (#881, closes #860) by @aoright
   - `test_detect_language`/`test_detect_with_confidence` in `tests/normalize/test_language_detector.py` asserted on real `langdetect` output with no skip guard, even though `langdetect` is an optional dependency absent from `pyproject.toml` that `LanguageDetector` already degrades gracefully without (`LANGDETECT_AVAILABLE = False`, falls back to `default_language`) — any environment without it failed both tests unconditionally, including a fresh CI run without optional extras installed. Both are now gated with `@unittest.skipUnless(LANGDETECT_AVAILABLE, ...)`
   - `test_detect_encoding` in `tests/normalize/test_encoding_handler.py` asserted `chardet.detect()`'s result against a 3-name allowlist (`iso-8859-1`/`windows-1252`/`latin-1`); on a short Latin-1 sample, chardet is free to return other compatible single-byte codepages (e.g. `windows-1253`), which fails the allowlist and then cascades into `test_convert_to_utf8` decoding the bytes as Greek instead of the original text. The test now uses a longer, unambiguous Latin-1 corpus and asserts that the detected encoding round-trip-decodes the original text instead of matching a fixed name list; `test_convert_to_utf8` now passes `source_encoding="latin-1"` explicitly rather than relying on chardet's heuristic auto-detection

semantica/context/context_graph.py

@@ -410,6 +410,9 @@ class ContextEdge:
         return d
 
 
+_ATTRS_MISSING = object()
+
+
 class ContextGraph:
     """
     Easy-to-Use Context Graph with All Advanced Features.
@@ -708,18 +711,71 @@ class ContextGraph:
                     })
         return result
 
-    def get_node_property(self, node_id: str, property_name: str) -> Any:
-        with self._lock:
-            node = self.nodes.get(node_id)
-            if not node:
-                return None
-            return node.properties.get(property_name)
+    def get_node_property(
+        self,
+        node_id: str,
+        property_name: str,
+        default: Any = None,
+    ) -> Any:
+        """Return the value of *property_name* on *node_id*.
 
-    def get_node_attributes(self, node_id: str) -> Dict[str, Any]:
+        Returns *default* when the node does not exist or when the property is
+        not set on the node.  Both failure modes return the same *default*, so
+        a sentinel can identify *any not-found result* as distinct from a
+        property whose value is legitimately ``None``::
+
+            _MISSING = object()
+            val = graph.get_node_property(node_id, "score", default=_MISSING)
+            if val is _MISSING:
+                ...  # node absent or property not set
+
+        To distinguish a missing node from a missing property specifically,
+        call ``find_node()`` first to check node existence.
+
+        Args:
+            node_id: ID of the node to look up.
+            property_name: Name of the property to retrieve.
+            default: Value returned when the node or property is absent.
+                Defaults to ``None`` (backward-compatible).
+
+        Returns:
+            The property value, or *default* if not found.
+        """
         with self._lock:
             node = self.nodes.get(node_id)
-            if not node:
-                return {}
+            if node is None:
+                return default
+            return node.properties.get(property_name, default)
+
+    def get_node_attributes(
+        self,
+        node_id: str,
+        default: Any = _ATTRS_MISSING,
+    ) -> Any:
+        """Return a copy of all properties on *node_id*.
+
+        Returns *default* when the node does not exist.  The historical
+        default is ``{}`` (an empty dict), preserved for backward
+        compatibility.  Pass a private sentinel as *default* to detect a
+        missing node unambiguously::
+
+            _MISSING = object()
+            attrs = graph.get_node_attributes(node_id, default=_MISSING)
+            if attrs is _MISSING:
+                ...  # node does not exist
+
+        Args:
+            node_id: ID of the node to look up.
+            default: Value returned when the node is absent.
+                Defaults to ``{}`` (backward-compatible).
+
+        Returns:
+            A shallow copy of the node's properties dict, or *default*.
+        """
+        with self._lock:
+            node = self.nodes.get(node_id)
+            if node is None:
+                return {} if default is _ATTRS_MISSING else default
             return node.properties.copy()
 
     def add_node_attribute(self, node_id: str, attributes: Dict[str, Any]) -> None:
@@ -730,13 +786,28 @@ class ContextGraph:
             node.properties.update(attributes)
             node.metadata.update(attributes)
 
-
         if getattr(self, "mutation_callback", None) and not getattr(
             self, "_suspend_mutation_callback", False
         ):
-            self.mutation_callback("UPDATE_NODE", node_id, node.to_dict())
+            try:
+                self.mutation_callback("UPDATE_NODE", node_id, node.to_dict())
+            except Exception as e:
+                self.logger.warning(f"Audit trail callback failed for node {node_id}: {e}")
 
     def get_edge_data(self, source_id: str, target_id: str) -> Dict[str, Any]:
+        """Return metadata for the edge between *source_id* and *target_id*.
+
+        Returns an empty dict ``{}`` when no edge exists between the two nodes
+        or when either node is absent.
+
+        Args:
+            source_id: ID of the source node.
+            target_id: ID of the target node.
+
+        Returns:
+            A dict containing edge metadata (``id``, ``familyId``, ``type``,
+            ``weight``, plus any custom metadata), or ``{}`` if not found.
+        """
         with self._lock:
             for edge in self._adjacency.get(source_id, []):
                 if edge.target_id == target_id:
@@ -1080,7 +1151,17 @@ class ContextGraph:
         self.logger.info(f"Loaded context graph from {path}")
 
     def find_node(self, node_id: str) -> Optional[Dict[str, Any]]:
-        """Find a node by ID."""
+        """Return a dict representation of the node identified by *node_id*.
+
+        Returns ``None`` when the node does not exist.
+
+        Args:
+            node_id: ID of the node to look up.
+
+        Returns:
+            A dict with keys ``id``, ``type``, ``content``, and ``metadata``,
+            or ``None`` if the node is not found.
+        """
         with self._lock:
             node = self.nodes.get(node_id)
             if node:

tests/context/test_context.py

@@ -252,5 +252,87 @@ class TestContextModule(unittest.TestCase):
         self.assertIsNotNone(ctx._memory)
         self.assertEqual(len(ctx._memory.short_term_memory), 1)
 
+class TestContextGraphNodePropertyContract(unittest.TestCase):
+
+    _MISSING = object()
+
+    def _graph_with_node(self):
+        graph = ContextGraph()
+        graph.add_node("n1", "person", "Alice", role="engineer", score=0)
+        return graph
+
+    def test_get_node_property_existing_node_existing_prop(self):
+        graph = self._graph_with_node()
+        self.assertEqual(graph.get_node_property("n1", "role"), "engineer")
+
+    def test_get_node_property_existing_node_missing_prop(self):
+        graph = self._graph_with_node()
+        self.assertIsNone(graph.get_node_property("n1", "nonexistent"))
+
+    def test_get_node_property_missing_node_returns_default_none(self):
+        graph = self._graph_with_node()
+        self.assertIsNone(graph.get_node_property("ghost", "role"))
+
+    def test_get_node_property_returns_default_on_missing_node(self):
+        graph = self._graph_with_node()
+        result = graph.get_node_property("ghost", "role", default=self._MISSING)
+        self.assertIs(result, self._MISSING)
+
+    def test_get_node_property_returns_default_on_missing_prop(self):
+        graph = self._graph_with_node()
+        result = graph.get_node_property("n1", "nonexistent", default=self._MISSING)
+        self.assertIs(result, self._MISSING)
+
+    def test_get_node_property_explicit_default_returned_for_absent_node(self):
+        graph = self._graph_with_node()
+        self.assertEqual(graph.get_node_property("ghost", "role", default="fallback"), "fallback")
+
+    def test_get_node_property_prop_value_of_zero_not_swallowed(self):
+        graph = self._graph_with_node()
+        self.assertEqual(graph.get_node_property("n1", "score"), 0)
+
+    def test_get_node_attributes_existing_node_returns_copy(self):
+        graph = self._graph_with_node()
+        attrs = graph.get_node_attributes("n1")
+        self.assertIsInstance(attrs, dict)
+        self.assertEqual(attrs.get("role"), "engineer")
+
+    def test_get_node_attributes_missing_node_returns_empty_dict_by_default(self):
+        graph = self._graph_with_node()
+        self.assertEqual(graph.get_node_attributes("ghost"), {})
+
+    def test_get_node_attributes_missing_node_explicit_default(self):
+        graph = self._graph_with_node()
+        result = graph.get_node_attributes("ghost", default={})
+        self.assertEqual(result, {})
+
+    def test_add_node_attribute_mutation_callback_fires_on_update(self):
+        graph = self._graph_with_node()
+        fired = []
+        graph.mutation_callback = lambda op, nid, data: fired.append((op, nid))
+        graph.add_node_attribute("n1", {"extra": "value"})
+        self.assertEqual(len(fired), 1)
+        self.assertEqual(fired[0], ("UPDATE_NODE", "n1"))
+
+    def test_add_node_attribute_missing_node_no_callback(self):
+        graph = self._graph_with_node()
+        fired = []
+        graph.mutation_callback = lambda op, nid, data: fired.append((op, nid))
+        graph.add_node_attribute("ghost", {"extra": "value"})
+        self.assertEqual(len(fired), 0)
+
+    def test_add_node_attribute_raising_callback_does_not_propagate(self):
+        graph = self._graph_with_node()
+
+        def _boom(op, nid, data):
+            raise RuntimeError("audit sink unavailable")
+
+        graph.mutation_callback = _boom
+        # Should not raise, matching _add_internal_node/_add_internal_edge,
+        # which already catch and log mutation_callback exceptions.
+        graph.add_node_attribute("n1", {"extra": "value"})
+        self.assertEqual(graph.get_node_property("n1", "extra"), "value")
+
+
 if __name__ == '__main__':
     unittest.main()