fix(mcp): keep original exception and record error span when wrapped tool/agent fails

agentkit/apps/mcp_app/mcp_app.py

@@ -43,6 +43,7 @@ async def async_wrapper(*args, **kwargs) -> Any:
                 # with tracer.start_as_current_span("tool") as span:
                 with telemetry.tracer.start_as_current_span(name="tool") as span:
                     exception = None
+                    result = None
                     try:
                         result = await func(*args, **kwargs)
 
@@ -71,6 +72,7 @@ def sync_wrapper(*args, **kwargs) -> Any:
                 # with tracer.start_as_current_span("tool") as span:
                 with telemetry.tracer.start_as_current_span(name="tool") as span:
                     exception = None
+                    result = None
                     try:
                         result = func(*args, **kwargs)
                     except Exception as e:
@@ -99,6 +101,7 @@ def agent_as_a_tool(self, func: Callable) -> Callable:
             async def async_wrapper(*args, **kwargs) -> Any:
                 with telemetry.tracer.start_as_current_span(name="tool") as span:
                     exception = None
+                    result = None
                     try:
                         result = await func(*args, **kwargs)
                     except Exception as e:
@@ -123,6 +126,7 @@ async def async_wrapper(*args, **kwargs) -> Any:
             def sync_wrapper(*args, **kwargs) -> Any:
                 with telemetry.tracer.start_as_current_span(name="tool") as span:
                     exception = None
+                    result = None
                     try:
                         result = func(*args, **kwargs)
                     except Exception as e:

tests/apps/test_mcp_app.py

@@ -256,7 +256,7 @@ async def my_async_tool(x):
     assert call["args"] == (5,)
 
 
-def test_invoking_sync_tool_wrapper_raises_unbound_local_error_when_func_fails(
+def test_invoking_sync_tool_wrapper_propagates_original_error_and_records_span(
     app, fake_telemetry
 ):
     boom = ValueError("kaboom")
@@ -267,19 +267,20 @@ def failing_tool():
     app.tool(failing_tool)
     wrapper = app._mcp_server.registered[0]
 
-    # NOTE: latent bug -- when the wrapped func raises, ``result`` is never
-    # assigned, so the ``finally`` block's ``telemetry.trace_tool(..., func_result=result, ...)``
-    # dereferences an unbound local. That UnboundLocalError is raised out of the
-    # ``finally`` and MASKS the original ValueError, so trace_tool is never
-    # invoked. We pin the actual current behavior: an UnboundLocalError, not the
-    # original ValueError, escapes and no telemetry call is recorded.
-    with pytest.raises(UnboundLocalError):
+    # The original exception must propagate; the finally-block telemetry call
+    # must not raise UnboundLocalError and mask it. trace_tool records the
+    # exception on the span with no successful result.
+    with pytest.raises(ValueError, match="kaboom"):
         wrapper()
 
-    assert fake_telemetry.calls == []
+    assert len(fake_telemetry.calls) == 1
+    call = fake_telemetry.calls[0]
+    assert call["operation_type"] == "mcp_tool"
+    assert call["exception"] is boom
+    assert call["func_result"] is None
 
 
-def test_invoking_async_tool_wrapper_raises_unbound_local_error_when_func_fails(
+def test_invoking_async_tool_wrapper_propagates_original_error_and_records_span(
     app, fake_telemetry
 ):
     boom = ValueError("kaboom")
@@ -290,13 +291,17 @@ async def failing_tool():
     app.tool(failing_tool)
     wrapper = app._mcp_server.registered[0]
 
-    # NOTE: latent bug -- see sync counterpart. The async wrapper's ``finally``
-    # references the unbound ``result``, raising UnboundLocalError that masks the
-    # original ValueError; trace_tool is never reached.
-    with pytest.raises(UnboundLocalError):
+    # The original exception must propagate; the finally-block telemetry call
+    # must not raise UnboundLocalError and mask it. trace_tool records the
+    # exception on the span with no successful result.
+    with pytest.raises(ValueError, match="kaboom"):
         asyncio.run(wrapper())
 
-    assert fake_telemetry.calls == []
+    assert len(fake_telemetry.calls) == 1
+    call = fake_telemetry.calls[0]
+    assert call["operation_type"] == "mcp_tool"
+    assert call["exception"] is boom
+    assert call["func_result"] is None
 
 
 # ---------------------------------------------------------------------------
@@ -380,7 +385,31 @@ async def my_agent(a, b):
     assert call["args"] == (2, 3)
 
 
-def test_invoking_async_agent_tool_wrapper_raises_unbound_local_error_when_func_fails(
+def test_invoking_sync_agent_tool_wrapper_propagates_original_error_and_records_span(
+    app, fake_telemetry
+):
+    boom = RuntimeError("agent boom")
+
+    def failing_agent():
+        raise boom
+
+    app.agent_as_a_tool(failing_agent)
+    wrapper = app._mcp_server.registered[0]
+
+    # The original exception must propagate; the finally-block telemetry call
+    # must not raise UnboundLocalError and mask it. trace_tool records the
+    # exception on the span with no successful result.
+    with pytest.raises(RuntimeError, match="agent boom"):
+        wrapper()
+
+    assert len(fake_telemetry.calls) == 1
+    call = fake_telemetry.calls[0]
+    assert call["operation_type"] == "agent_mcp_tool"
+    assert call["exception"] is boom
+    assert call["func_result"] is None
+
+
+def test_invoking_async_agent_tool_wrapper_propagates_original_error_and_records_span(
     app, fake_telemetry
 ):
     boom = RuntimeError("agent boom")
@@ -391,13 +420,17 @@ async def failing_agent():
     app.agent_as_a_tool(failing_agent)
     wrapper = app._mcp_server.registered[0]
 
-    # NOTE: latent bug -- same unbound-``result`` defect as the tool() wrappers.
-    # The ``finally`` block raises UnboundLocalError, masking the original
-    # RuntimeError, and trace_tool is never called.
-    with pytest.raises(UnboundLocalError):
+    # The original exception must propagate; the finally-block telemetry call
+    # must not raise UnboundLocalError and mask it. trace_tool records the
+    # exception on the span with no successful result.
+    with pytest.raises(RuntimeError, match="agent boom"):
         asyncio.run(wrapper())
 
-    assert fake_telemetry.calls == []
+    assert len(fake_telemetry.calls) == 1
+    call = fake_telemetry.calls[0]
+    assert call["operation_type"] == "agent_mcp_tool"
+    assert call["exception"] is boom
+    assert call["func_result"] is None
 
 
 # ---------------------------------------------------------------------------