diff --git a/docs/specs/004-python-function-calling-loop.md b/docs/specs/004-python-function-calling-loop.md index be0c4d96ae2..ebffabcc050 100644 --- a/docs/specs/004-python-function-calling-loop.md +++ b/docs/specs/004-python-function-calling-loop.md @@ -386,6 +386,9 @@ that manually replay messages own the equivalent rule: do not resend an approval while the original field remains directly available to trusted local code. Remote protocol serializers use the marker only for status and use the channel-visible `result` or `items` for output text. `include_detailed_errors=False` keeps the channel-visible result generic; enabling it explicitly may place diagnostic text in the result for that configured channel. +- An ordinary exception raised by a custom `FunctionTool.result_parser` follows the same terminal tool-error + contract, including `include_detailed_errors`; the raw return value does not replace the failed custom conversion. + Default parsing retains its string fallback, and explicit skip-parsing behavior is unchanged. - Parallel calls retain model order in the returned transcript. - `call_id` remains the provider/service correlation id; a locally actionable `function_call` also carries a stable Agent Framework occurrence identity in `Content.id`. @@ -623,6 +626,7 @@ that manually replay messages own the equivalent rule: do not resend an approval | Rejected execution | Rejection is a normal terminal result, not an exception to the caller. | `test_unapproved_tool_execution_raises_exception` | | Approved tool exception | Generic and detailed error modes preserve one result and one execution. | `test_approved_function_call_with_error_without_detailed_errors`, `test_approved_function_call_with_error_with_detailed_errors` | | Tool exception diagnostics | Internal diagnostics remain available to trusted local code, serialization preserves only a fixed failure marker, and explicit detailed-error configuration affects only the channel-visible result. | `packages/core/tests/core/test_types.py::test_function_result_exception_is_internal_by_default`, `packages/core/tests/core/test_function_invocation_logic.py::test_function_invocation_config_include_detailed_errors_false`, `test_function_invocation_config_include_detailed_errors_true`, `test_streaming_function_invocation_config_include_detailed_errors_false`, `test_streaming_function_invocation_config_include_detailed_errors_true` | +| Custom result-parser exception | Failed custom conversion produces one tool-error result rather than the raw return value, in both response modes and with observability enabled or disabled; detailed-error configuration is preserved. | `packages/core/tests/core/test_function_invocation_logic.py::test_function_invocation_result_parser_failure`, `packages/core/tests/core/test_tools.py::test_invoke_result_parser_exception_propagates` | | Approved validation error | Validation failure returns one result without invoking the function body. | `test_approved_function_call_with_validation_error` | | Pre-validation middleware repair | Schema-compatible calls retain normalized middleware arguments without duplicate validation. Prepared-value reuse does not require validator outputs to be copyable, and unchanged NaNs remain stable. Identity-only opaque values may be reused for ordinary execution, but security authority rejects them before downstream middleware or the tool body because in-place mutation cannot be detected safely. When provisional validation fails, function middleware observes raw parsed arguments, may repair them before final validation, and the body receives normalized values; short-circuiting skips final validation and execution. Repair after security middleware fails closed using recursive type-aware, float-bit-exact comparison, including invalid and short-circuited mutations. Security inspects exact normalized values, and validation errors after hidden-value resolution do not disclose resolved values or mapping keys, including validator `TypeError` paths. | `test_function_middleware_keeps_normalized_arguments_for_valid_calls`, `test_prepared_arguments_support_noncopyable_validator_output`, `test_nan_prepared_and_approval_snapshots_are_stable`, `test_function_middleware_repairs_raw_arguments_before_validation`, `test_function_middleware_can_short_circuit_before_argument_validation`, `test_invalid_arguments_produced_by_middleware_keep_argument_error_contract`, `packages/core/tests/test_security.py::TestVariableArgumentPolicy::test_security_rejects_opaque_mutable_validator_output`, `test_argument_mutation_after_security_middleware_fails_closed`, `test_security_snapshot_accepts_unchanged_nan`, `test_argument_mutation_after_security_short_circuit_fails_closed`, `test_security_policy_observes_custom_validator_transform_once`, `test_hidden_argument_validation_error_does_not_disclose_resolved_value`, `test_hidden_mapping_key_is_not_disclosed_by_validation_error`, `test_hidden_value_is_not_disclosed_by_validator_type_error`, `test_hidden_argument_can_be_normalized_after_security_check` | | Approved middleware repair | Approval binds to the normalized middleware-entry representation, so ordinary Pydantic coercion still completes in one approval round. A changed approval-bound call executes zero times under the old grant, returns a persisted occurrence-bound replacement request in both response modes, and executes once only after the replacement is approved. Recursive type-aware, float-bit-exact comparison treats booleans and numbers, and positive and negative zero, as distinct while keeping unchanged NaNs stable. Opaque mutable normalized values fail closed before approval authority is established. The same replacement rule applies when middleware short-circuits instead of calling the tool. Security expansion preserves approval-visible placeholders. | `test_approved_coercing_arguments_execute_without_replacement`, `test_approved_argument_repair_requires_replacement_approval`, `test_approved_argument_repair_short_circuit_requires_replacement_approval`, `test_approval_snapshot_distinguishes_exact_values`, `test_approval_rejects_opaque_mutable_validator_output`, `packages/core/tests/test_security.py::TestVariableArgumentPolicy::test_hidden_argument_resolution_does_not_require_reapproval` | diff --git a/python/packages/core/agent_framework/_tools.py b/python/packages/core/agent_framework/_tools.py index 64f8d0f9557..5f7cc56ccc5 100644 --- a/python/packages/core/agent_framework/_tools.py +++ b/python/packages/core/agent_framework/_tools.py @@ -42,6 +42,7 @@ ) from uuid import uuid4 +from opentelemetry import trace from opentelemetry.metrics import Histogram, NoOpHistogram from pydantic import BaseModel, Field, ValidationError, create_model @@ -531,10 +532,12 @@ def __init__( Pydantic model when runtime validation matters. Treat dictionary schemas as declarations for trusted settings and non-sensitive functions only; never rely on them as an authorization or security boundary. - result_parser: An optional callable with signature ``Callable[[Any], str]`` that + result_parser: An optional callable with signature ``Callable[[Any], str | list[Content]]`` that overrides the default result parsing behavior. When provided, this callable - is used to convert the raw function return value to a string instead of the - built-in :meth:`parse_result` logic. Pass the :data:`SKIP_PARSING` sentinel + converts the raw function return value to a string or content items instead of the + built-in :meth:`parse_result` logic. Exceptions raised by a custom parser propagate + from :meth:`invoke`; the raw result is not used as a fallback. Handle recoverable + conversion errors inside the parser if a fallback is needed. Pass the :data:`SKIP_PARSING` sentinel instead of a callable to opt out of parsing entirely; in that case :meth:`invoke` returns the wrapped function's raw return value. Depending on your function, it may be easiest to just do the serialization directly @@ -903,6 +906,11 @@ async def invoke( configured on the tool. Every result — text, rich media, or serialized objects — is represented uniformly as Content items. + Exceptions raised by a custom result parser propagate to the caller without + falling back to the raw result. During automatic tool invocation, these + exceptions follow the existing tool-error handling, including the + ``include_detailed_errors`` setting. Default parsing retains its string fallback. + Parsing can be skipped in two ways: configure the tool with ``result_parser=SKIP_PARSING`` to always skip parsing, or pass ``skip_parsing=True`` per call. Either way the wrapped function's raw value @@ -937,7 +945,6 @@ async def invoke( configured_parser = self.result_parser skip_parsing = skip_parsing or configured_parser is SKIP_PARSING - parser = configured_parser if callable(configured_parser) else FunctionTool.parse_result parameter_names = set(self.parameters().get("properties", {}).keys()) direct_argument_kwargs = ( @@ -1001,9 +1008,19 @@ async def invoke( and configured_parser is None ): parsed = result + elif callable(configured_parser): + try: + parsed = configured_parser(result) + except Exception as exception: + self.invocation_exception_count += 1 + if OBSERVABILITY_SETTINGS.SENSITIVE_DATA_ENABLED: + logger.error(f"Function {self.name}: result parser failed. Error: {exception}") + else: + logger.error(f"Function {self.name}: result parser failed.") + raise else: try: - parsed = parser(result) + parsed = FunctionTool.parse_result(result) except Exception: logger.warning(f"Function {self.name}: result parser failed, falling back to str().") parsed = [Content.from_text(str(result))] @@ -1076,9 +1093,23 @@ async def invoke( and configured_parser is None ): parsed = result + elif callable(configured_parser): + try: + parsed = configured_parser(result) + except Exception as exception: + self.invocation_exception_count += 1 + attributes[OtelAttr.ERROR_TYPE] = type(exception).__name__ + if OBSERVABILITY_SETTINGS.SENSITIVE_DATA_ENABLED: + capture_exception(span=span, exception=exception, timestamp=time_ns()) + logger.error(f"Function {self.name}: result parser failed. Error: {exception}") + else: + span.set_attribute(OtelAttr.ERROR_TYPE, type(exception).__name__) + span.set_status(status=trace.StatusCode.ERROR) + logger.error(f"Function {self.name}: result parser failed.") + raise else: try: - parsed = parser(result) + parsed = FunctionTool.parse_result(result) except Exception: logger.warning(f"Function {self.name}: result parser failed, falling back to str().") parsed = [Content.from_text(str(result))] @@ -1519,11 +1550,13 @@ def tool( max_invocation_exceptions: The maximum number of exceptions allowed during invocations. If None, there is no limit, should be at least 1. additional_properties: Additional properties to set on the function. - result_parser: An optional callable with signature ``Callable[[Any], str]`` that + result_parser: An optional callable with signature ``Callable[[Any], str | list[Content]]`` that overrides the default result parsing. When provided, this callable converts the - raw function return value to a string instead of using the built-in - :meth:`FunctionTool.parse_result`. Depending on your function, it may be - easiest to just do the serialization directly in the function body rather + raw function return value to a string or content items instead of using the built-in + :meth:`FunctionTool.parse_result`. Exceptions raised by a custom parser propagate + from :meth:`FunctionTool.invoke`; the raw result is not used as a fallback. + Handle recoverable conversion errors inside the parser if a fallback is needed. + Depending on your function, it may be easiest to do the serialization directly in the function body rather than providing a custom ``result_parser``. Note: @@ -1736,12 +1769,21 @@ def _function_execution_error_result( config: FunctionInvocationConfiguration, context: FunctionInvocationContext | None = None, ) -> Content: - logger.warning( - "Function '%s' raised an exception; returning an error result to the model. " - "Set include_detailed_errors=True for the full detail. Exception: %r", - tool_name, - exception, - ) + from .observability import OBSERVABILITY_SETTINGS + + if OBSERVABILITY_SETTINGS.SENSITIVE_DATA_ENABLED: + logger.warning( + "Function '%s' raised an exception; returning an error result to the model. " + "Set include_detailed_errors=True for the full detail. Exception: %r", + tool_name, + exception, + ) + else: + logger.warning( + "Function '%s' raised an exception; returning an error result to the model. " + "Set include_detailed_errors=True for the full detail.", + tool_name, + ) message = "Error: Function failed." if config.get("include_detailed_errors", False): message = f"{message} Exception: {exception}" diff --git a/python/packages/core/tests/core/test_function_invocation_logic.py b/python/packages/core/tests/core/test_function_invocation_logic.py index f09dab17485..31f18e19eef 100644 --- a/python/packages/core/tests/core/test_function_invocation_logic.py +++ b/python/packages/core/tests/core/test_function_invocation_logic.py @@ -8,6 +8,7 @@ import warnings from collections.abc import AsyncIterable, Awaitable, Callable, Sequence from typing import Any, Literal +from unittest.mock import Mock import pytest from pydantic import BaseModel, field_validator @@ -3350,6 +3351,97 @@ def hidden_func(arg1: str) -> str: assert len(function_calls) >= 1 +@pytest.mark.parametrize( + ("enable_instrumentation", "enable_sensitive_data"), + [(False, False), (True, False), (True, True)], + indirect=True, +) +@pytest.mark.usefixtures("span_exporter") +@pytest.mark.parametrize("stream", [False, True]) +@pytest.mark.parametrize("include_detailed_errors", [False, True]) +async def test_function_invocation_result_parser_failure( + chat_client_base: SupportsChatGetResponse, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, + stream: bool, + include_detailed_errors: bool, + enable_sensitive_data: bool, +) -> None: + parser = Mock(side_effect=ValueError(f"Unsupported result: {_PRIVATE_ERROR_DETAIL}")) + + @tool(result_parser=parser) + def make_result() -> dict[str, str]: + return {"value": "unparsed-value"} + + call = Content.from_function_call(call_id="1", name="make_result", arguments="{}") + monkeypatch.setattr( + chat_client_base, + "run_responses", + [ + ChatResponse(messages=Message(role="assistant", contents=[call])), + ChatResponse(messages=Message(role="assistant", contents=["done"])), + ], + ) + monkeypatch.setattr( + chat_client_base, + "streaming_responses", + [ + [ChatResponseUpdate(role="assistant", contents=[call])], + [ChatResponseUpdate(role="assistant", contents=[Content.from_text("done")])], + ], + ) + monkeypatch.setitem( + chat_client_base.function_invocation_configuration, # type: ignore[attr-defined] # ty: ignore[unresolved-attribute] + "include_detailed_errors", + include_detailed_errors, + ) + provider = Mock(wraps=chat_client_base._inner_get_response) # type: ignore[attr-defined] # ty: ignore[unresolved-attribute] + monkeypatch.setattr(chat_client_base, "_inner_get_response", provider) + messages = [Message(role="user", contents=["hello"])] + options: ChatOptions = {"tools": [make_result]} + + if stream: + response_stream = chat_client_base.get_response(messages, options=options, stream=True) + updates = [update async for update in response_stream] + assert "unparsed-value" not in json.dumps([update.to_dict() for update in updates]) + response = await response_stream.get_final_response() + else: + response = await chat_client_base.get_response(messages, options=options) + + results = [content for msg in response.messages for content in msg.contents if content.type == "function_result"] + assert len(results) == 1 + assert results[0].call_id == "1" + assert results[0].exception is not None + assert "Unsupported result" in results[0].exception + expected_result = "Error: Function failed." + if include_detailed_errors: + expected_result += f" Exception: Unsupported result: {_PRIVATE_ERROR_DETAIL}" + assert results[0].result == expected_result + serialized_response = json.dumps(response.to_dict()) + assert (_PRIVATE_ERROR_DETAIL in serialized_response) is include_detailed_errors + assert "unparsed-value" not in json.dumps(response.to_dict()) + assert response.messages[-1].text == "done" + assert provider.call_count == 2 + provider_messages = provider.call_args.kwargs["messages"] + provider_results = [ + content for msg in provider_messages for content in msg.contents if content.type == "function_result" + ] + assert len(provider_results) == 1 + assert provider_results[0].result == results[0].result + assert "unparsed-value" not in json.dumps([msg.to_dict() for msg in provider_messages]) + parser.assert_called_once_with({"value": "unparsed-value"}) + assert make_result.invocation_count == 1 + execution_warnings = [ + record.getMessage() + for record in caplog.records + if record.name == "agent_framework" + and record.levelno == logging.WARNING + and "raised an exception; returning an error result" in record.getMessage() + ] + assert len(execution_warnings) == 1 + assert (_PRIVATE_ERROR_DETAIL in execution_warnings[0]) is enable_sensitive_data + + async def test_function_invocation_config_include_detailed_errors_false(chat_client_base: SupportsChatGetResponse): """Test that include_detailed_errors=False returns generic error messages.""" diff --git a/python/packages/core/tests/core/test_tools.py b/python/packages/core/tests/core/test_tools.py index e23d1db751d..89aa73486cc 100644 --- a/python/packages/core/tests/core/test_tools.py +++ b/python/packages/core/tests/core/test_tools.py @@ -1,5 +1,6 @@ # Copyright (c) Microsoft. All rights reserved. import asyncio +import logging import threading from typing import Annotated, Any, Literal, get_args, get_origin from unittest.mock import Mock @@ -800,7 +801,9 @@ def pydantic_test_tool(x: int, y: int) -> int: assert span.attributes[OtelAttr.TOOL_ARGUMENTS] == '{"x": 5, "y": 10}' # type: ignore[index] # pyrefly: ignore[unsupported-operation] # ty: ignore[not-subscriptable] -async def test_tool_invoke_telemetry_with_exception(span_exporter: InMemorySpanExporter): +async def test_tool_invoke_telemetry_with_exception( + span_exporter: InMemorySpanExporter, caplog: pytest.LogCaptureFixture +): """Test the tool invoke method with telemetry when an exception occurs.""" @tool( @@ -817,6 +820,8 @@ def exception_test_tool(x: int, y: int) -> int: # Call invoke and expect exception with pytest.raises(ValueError, match="Test exception for telemetry"): await exception_test_tool.invoke(x=1, y=2, tool_call_id="exception_call") + assert "Function failed. Error: Test exception for telemetry" in caplog.text + assert "result parser failed" not in caplog.text spans = span_exporter.get_finished_spans() assert len(spans) == 1 span = spans[0] @@ -1525,6 +1530,111 @@ def needs_event_loop() -> str: assert tool_thread_ids == [event_loop_thread_id] +@pytest.mark.parametrize( + ("enable_instrumentation", "enable_sensitive_data"), + [(False, False), (True, False), (True, True)], + indirect=True, +) +async def test_invoke_result_parser_exception_propagates( + span_exporter: InMemorySpanExporter, + enable_instrumentation: bool, + enable_sensitive_data: bool, + caplog: pytest.LogCaptureFixture, +) -> None: + from agent_framework.exceptions import ToolException + + error = ValueError("Unsupported result") + parser = Mock(side_effect=error) + raw_result = {"value": "unparsed-value"} + + @tool(result_parser=parser, max_invocation_exceptions=1) + def make_result() -> dict[str, str]: + return raw_result + + histogram = Mock() + make_result._invocation_duration_histogram = histogram + with caplog.at_level(logging.DEBUG, logger="agent_framework"), pytest.raises(ValueError) as exc_info: + await make_result.invoke() + + assert exc_info.value is error + parser.assert_called_once_with(raw_result) + assert make_result.invocation_count == 1 + assert make_result.invocation_exception_count == 1 + with pytest.raises(ToolException, match="maximum exception limit"): + await make_result.invoke() + parser.assert_called_once_with(raw_result) + assert make_result.invocation_count == 1 + assert make_result.invocation_exception_count == 1 + parser_errors = [ + record.getMessage() + for record in caplog.records + if record.name == "agent_framework" + and record.levelno == logging.ERROR + and "result parser failed" in record.getMessage() + ] + expected_parser_error = "Function make_result: result parser failed." + if enable_sensitive_data: + expected_parser_error += " Error: Unsupported result" + assert parser_errors == [expected_parser_error] + assert "succeeded" not in caplog.text + assert "unparsed-value" not in caplog.text + spans = span_exporter.get_finished_spans() + if enable_instrumentation: + assert len(spans) == 2 + assert spans[0].status.status_code == trace.StatusCode.ERROR + assert spans[0].attributes is not None + assert spans[0].attributes[OtelAttr.ERROR_TYPE] == "ValueError" + assert OtelAttr.TOOL_RESULT not in spans[0].attributes + assert bool(spans[0].events) is enable_sensitive_data + assert ("Unsupported result" in (spans[0].status.description or "")) is enable_sensitive_data + assert spans[1].attributes is not None + assert spans[1].attributes[OtelAttr.ERROR_TYPE] == "ToolException" + assert histogram.record.call_count == 2 + assert histogram.record.call_args_list[0].kwargs["attributes"][OtelAttr.ERROR_TYPE] == "ValueError" + else: + assert not spans + histogram.record.assert_not_called() + + +@pytest.mark.parametrize("enable_instrumentation", [False, True], indirect=True) +@pytest.mark.parametrize("return_content", [False, True]) +async def test_invoke_result_parser_success(span_exporter: InMemorySpanExporter, return_content: bool) -> None: + parsed = [Content.from_text("parsed-value")] if return_content else "parsed-value" + parser = Mock(return_value=parsed) + + @tool(result_parser=parser) + def make_result() -> dict[str, str]: + return {"value": "unparsed-value"} + + result = await make_result.invoke() + + assert len(result) == 1 + assert result[0].text == "parsed-value" + parser.assert_called_once_with({"value": "unparsed-value"}) + if return_content: + assert result is parsed + for span in span_exporter.get_finished_spans(): + assert span.attributes is not None + assert span.attributes[OtelAttr.TOOL_RESULT] == "parsed-value" + + +@pytest.mark.parametrize("enable_instrumentation", [False, True], indirect=True) +@pytest.mark.usefixtures("span_exporter") +async def test_invoke_default_result_parser_failure_keeps_fallback(monkeypatch: pytest.MonkeyPatch) -> None: + @tool + def make_result() -> dict[str, int]: + return {"value": 1} + + monkeypatch.setattr(FunctionTool, "parse_result", Mock(side_effect=ValueError("Cannot convert result"))) + + result = await make_result.invoke() + + assert len(result) == 1 + assert result[0].text == "{'value': 1}" + + +@pytest.mark.parametrize("enable_instrumentation", [False, True], indirect=True) +@pytest.mark.usefixtures("span_exporter") async def test_invoke_skip_parsing_bypasses_configured_result_parser() -> None: """The tool's own result_parser is bypassed when skip_parsing=True is requested.""" parser_calls: list[Any] = [] @@ -1548,6 +1658,8 @@ def make_dict() -> dict[str, int]: assert parsed[0].text == "PARSED" +@pytest.mark.parametrize("enable_instrumentation", [False, True], indirect=True) +@pytest.mark.usefixtures("span_exporter") async def test_constructor_skip_parsing_sentinel_returns_raw_by_default() -> None: """Constructing a tool with result_parser=SKIP_PARSING makes invoke return the raw value."""