diff --git a/agentkit/apps/mcp_app/mcp_app.py b/agentkit/apps/mcp_app/mcp_app.py index f7273b94..8a34f037 100644 --- a/agentkit/apps/mcp_app/mcp_app.py +++ b/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: diff --git a/tests/apps/test_mcp_app.py b/tests/apps/test_mcp_app.py index 182b5b78..3b012065 100644 --- a/tests/apps/test_mcp_app.py +++ b/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 # ---------------------------------------------------------------------------