mirror of
https://github.com/semantica-agi/semantica.git
synced 2026-08-29 04:26:20 +00:00
fix(mcp): call backend get_causal_chain only once on internal TypeError
Signature introspection and the resulting call were sharing one try/except, so a genuine bug inside a backend's get_causal_chain (raising an unrelated TypeError) was misread as a signature mismatch, causing an identical retry call before the real error surfaced. Split introspection from the call so a successfully-introspected call happens exactly once; the trial-and-error cascade now only runs when inspect.signature itself fails. Also adds the CHANGELOG entry for #781/#817, which was missing.
This commit is contained in:
@@ -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
|
||||
|
||||
+15
-3
@@ -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,
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user