diff --git a/CHANGELOG.md b/CHANGELOG.md index c57a8f62..ca806667 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **MCP `handle_get_causal_chain` returned an empty-but-valid-looking response when both `CausalChainAnalyzer` and the graph fallback were unavailable** (#781, #817) by @Sameer6305 and @KaifAhmad1 + - Returns an explicit `{"error": "Causal chain analysis is not supported on this graph backend", "chain": []}` instead of `{"chain": [], "count": 0, "direction": ...}`, letting clients distinguish "unsupported" from a legitimately empty chain + - The fallback path now introspects `graph.get_causal_chain`'s signature to forward `direction`/`max_depth` (or a `depth` kwarg, or nothing, depending on what the backend accepts) instead of always calling with just `decision_id`, matching the primary analyzer path's behavior + - Hardened input handling: non-dict `args`, non-string `decision_id` (previously a latent `AttributeError` on `.strip()`), and `max_depth` clamped to `(0, 100]` with a safe default on invalid input + - Added `tests/test_mcp_decisions_causal_chain.py` (11 tests) covering the unsupported-backend, fallback-forwarding, and validation/exception paths across multiple backend signature shapes + - **Follow-up review fix**: the signature-detection try/except previously caught the *actual call*'s exceptions in the same block used for introspection failures, so a genuine bug inside a backend's `get_causal_chain` (raising an unrelated `TypeError`) was misread as a signature mismatch and the backend was invoked a second time with identical arguments before the real error surfaced. Signature introspection and the resulting call are now split into separate try/excepts so a successfully-introspected call is made exactly once; added `test_internal_typeerror_calls_backend_only_once` to lock this in + - **`ProvenanceManager` duplicated the same checksum/persist/exception-swallow block across 4 tracking methods** (#784, #815) by @Sameer6305 and @KaifAhmad1 - Consolidated the repeated `entry.checksum = compute_checksum(entry)` / `try: self.storage.store(entry) except Exception: pass` block used by `track_entity`, `track_relationship`, `track_chunk`, and `track_property_source` into a single `ProvenanceManager._save_entry()` helper, preserving the existing graceful-failure behavior and the batch `_conn`/re-raise semantics from #807 - Added 4 regression tests (`tests/provenance/test_manager.py`) covering storage-failure swallowing for each of the four tracking methods, none of which had coverage for this path before diff --git a/mcp/tools/decisions.py b/mcp/tools/decisions.py index d18117c5..a0643d12 100644 --- a/mcp/tools/decisions.py +++ b/mcp/tools/decisions.py @@ -115,9 +115,21 @@ def handle_get_causal_chain(args: dict) -> dict: except (ImportError, AttributeError): if hasattr(graph, "get_causal_chain"): import inspect + # Introspect the signature in its own try/except: only + # failure to introspect (ValueError/TypeError from + # inspect.signature itself, e.g. a C-extension callable) + # should fall through to the trial-and-error cascade below. + # A call made after a *successful* introspection must not be + # wrapped in that cascade's except block — otherwise a + # genuine bug inside get_causal_chain (raising an unrelated + # TypeError) gets misread as "wrong signature" and the + # backend is invoked a second time with identical arguments. try: - sig = inspect.signature(graph.get_causal_chain) - params = sig.parameters + params = inspect.signature(graph.get_causal_chain).parameters + except (ValueError, TypeError): + params = None + + if params is not None: has_var_kwargs = any( p.kind == inspect.Parameter.VAR_KEYWORD for p in params.values() @@ -137,7 +149,7 @@ def handle_get_causal_chain(args: dict) -> dict: ) else: chain = graph.get_causal_chain(decision_id) - except (ValueError, TypeError): + else: try: chain = graph.get_causal_chain( decision_id, diff --git a/tests/test_mcp_decisions_causal_chain.py b/tests/test_mcp_decisions_causal_chain.py index 841c00e2..d9f8219d 100644 --- a/tests/test_mcp_decisions_causal_chain.py +++ b/tests/test_mcp_decisions_causal_chain.py @@ -232,6 +232,38 @@ class TestMCPDecisionsCausalChain(unittest.TestCase): }, ) + @patch("mcp.tools.decisions.get_graph") + @patch("semantica.context.causal_analyzer.CausalChainAnalyzer") + def test_internal_typeerror_calls_backend_only_once(self, mock_analyzer_cls, mock_get_graph): + """ + Regression test: a signature that introspects successfully must be called + exactly once, even if the call itself raises TypeError for reasons unrelated + to the signature (e.g. a bug inside the backend). Previously this TypeError + was caught by the same except block used for introspection failures, causing + an identical retry call before the error was correctly surfaced. + """ + mock_analyzer_cls.side_effect = ImportError("mocked import error") + + class BadInternalGraphMock: + def __init__(self): + self.call_count = 0 + + def get_causal_chain(self, node_id, direction="downstream", max_depth=5): + self.call_count += 1 + raise TypeError("unsupported operand type(s) for +: 'int' and 'str'") + + graph_mock = BadInternalGraphMock() + mock_get_graph.return_value = graph_mock + response = handle_get_causal_chain({"decision_id": "dec_err"}) + self.assertEqual( + response, + { + "error": "unsupported operand type(s) for +: 'int' and 'str'", + "chain": [], + }, + ) + self.assertEqual(graph_mock.call_count, 1) + if __name__ == "__main__": unittest.main()