Skip to content

.Net: fix: fall back to ToString() when logging function results with unregistered types#13884

Merged
rogerbarreto merged 6 commits intomicrosoft:mainfrom
octo-patch:fix/issue-13681-logging-fallback-tostring
May 1, 2026
Merged

.Net: fix: fall back to ToString() when logging function results with unregistered types#13884
rogerbarreto merged 6 commits intomicrosoft:mainfrom
octo-patch:fix/issue-13681-logging-fallback-tostring

Conversation

@octo-patch
Copy link
Copy Markdown
Contributor

Fixes #13681

Problem

When a KernelFunction returns a value whose runtime type is not registered in AbstractionsJsonContext (e.g. Microsoft.Extensions.AI.TextContent returned by an MCP tool via .AsKernelFunction()), the function result logging in AOT/source-generation mode throws NotSupportedException during JSON serialization. The exception is caught but produces an unhelpful log entry:

Function SomePlugin-SomeTool result: Failed to log function result value
System.NotSupportedException: JsonTypeInfo metadata for type 'Microsoft.Extensions.AI.TextContent' was not provided by TypeInfoResolver of type 'Microsoft.SemanticKernel.AbstractionsJsonContext'.

Solution

Add a ToString() fallback in the NotSupportedException catch block of LogFunctionResultValueInternal. When JSON serialization fails because the runtime type is absent from the source-generated context, the logger now calls resultValue?.Value?.ToString() to produce useful output instead of the generic error message.

Testing

  • Added ItShouldFallBackToToStringWhenJsonSerializationIsNotSupported unit test that uses a restricted source-generated JsonSerializerContext (containing only object and IDictionary<string, object?>) to simulate the AOT scenario, verifying that ToString() output appears in the log instead of the failure message.
  • All existing ItShouldLogFunctionResultOfAnyType test cases are unchanged.

…n result logging (fixes microsoft#13681)

When a KernelFunction returns a type not registered in AbstractionsJsonContext
(e.g. Microsoft.Extensions.AI.TextContent from MCP tools), and JSON
serialization is attempted with AOT-safe JsonSerializerOptions, a
NotSupportedException is thrown and swallowed, causing the log message
to show "Failed to log function result value" instead of useful content.

This change adds a ToString() fallback in the NotSupportedException catch
block so that result values still produce meaningful log output even when
their runtime type is absent from the source-generated JSON context.
@octo-patch octo-patch requested a review from a team as a code owner April 18, 2026 02:05
@moonbox3 moonbox3 added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel kernel.core labels Apr 18, 2026
@github-actions github-actions Bot changed the title fix: fall back to ToString() when logging function results with unregistered types .Net: fix: fall back to ToString() when logging function results with unregistered types Apr 18, 2026
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Code Review

Reviewers: 4 | Confidence: 92%

✓ Correctness

The change adds a reasonable ToString() fallback when JSON serialization throws NotSupportedException for unregistered types (e.g., in AOT scenarios). The fallback chain is correct: try GetValue() → try JSON serialization → try ToString() → log error. The inner bare catch is consistent with the existing pattern at line 143. The test properly simulates the scenario with a source-generated restricted JsonSerializerContext. No correctness issues found.

✓ Security Reliability

This change adds a ToString() fallback in the NotSupportedException catch block of LogFunctionResultValueInternal, invoked when JSON serialization fails for unregistered types (e.g., in AOT scenarios). The change is well-bounded and safe: (1) the logging path is gated behind LogLevel.Trace which is disabled by default (line 108), (2) the ToString() output is passed as a structured log parameter via LogerMessage.Define, not string-interpolated, so there is no log injection risk, (3) null safety is properly handled via null-conditional operators and ?? string.Empty, (4) the bare inner catch block is consistent with the existing pattern on line 143 and the method already suppresses CA1031 (line 130), and (5) the outer NotSupportedException ex variable is correctly preserved and logged if the ToString() fallback itself fails. The test coverage validates the expected behavior. No security or reliability issues found.

✓ Test Coverage

The PR adds a ToString() fallback when JSON serialization throws NotSupportedException during function result logging. The new test covers the primary happy-path scenario well — it verifies the ToString() output is logged instead of the error message. However, the diff introduces a nested try/catch where the inner catch block (handling the case when ToString() itself throws) has no corresponding test. This is a minor coverage gap for a defensive code path.

✓ Design Approach

The change adds a ToString() fallback in LogFunctionResultValueInternal when JSON serialization throws NotSupportedException (e.g., in AOT scenarios where unregistered MEAITypes like TextContent are returned). The approach is sound for a trace-level logging path: JSON is still tried first (more informative for complex types), and only when that fails does ToString() serve as a last resort. The bare inner catch is consistent with the existing pattern on line 143. One design inconsistency worth noting: the symmetric LogFunctionArgumentsInternal (lines 65–90) has the identical NotSupportedException catch but still logs the opaque error string rather than falling back to ToString(). If the fallback is the right approach here, the arguments path has the same gap.

Suggestions

  • Add a test for the inner catch block (lines 170-173) where ToString() itself throws, verifying that the original 'Failed to log function result value' message with the NotSupportedException is still logged. For example, create a type whose ToString() throws and assert the error message and exception are passed to the logger.
  • The parallel LogFunctionArgumentsInternal method (lines 65–90) has the same NotSupportedException catch pattern but does not fall back to ToString(). If arguments can also contain unregistered types, that path will still log the unhelpful 'Failed to serialize arguments to Json' message. Consider applying the same fallback for consistency.

Automated review by octo-patch's agents

Copilot AI review requested due to automatic review settings April 30, 2026 10:59
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes AOT/source-generation logging failures when KernelFunction returns values whose runtime types aren’t present in the source-generated JSON context, by falling back to ToString() instead of emitting an unhelpful serialization error.

Changes:

  • Add a ToString() fallback path when JSON serialization throws NotSupportedException while logging function results.
  • Add a unit test that simulates a restricted source-generated JsonSerializerContext to validate the fallback behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
dotnet/src/SemanticKernel.Abstractions/Functions/KernelFunctionLogMessages.cs Adds ToString() fallback when AOT/source-gen JSON serialization can’t handle an unregistered runtime type.
dotnet/src/SemanticKernel.UnitTests/Functions/KernelFunctionLogMessagesTests.cs Adds a new test and a restricted source-generated JSON context to reproduce the AOT scenario.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread dotnet/src/SemanticKernel.UnitTests/Functions/KernelFunctionLogMessagesTests.cs Outdated
Comment thread dotnet/src/SemanticKernel.Abstractions/Functions/KernelFunctionLogMessages.cs Outdated
SergeyMenshykh and others added 2 commits April 30, 2026 17:13
…gMessagesTests.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…nLogMessages.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Mark KernelFunctionLogMessagesTests as partial so the nested
RestrictedJsonContext source generator can emit the required code.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@octo-patch
Copy link
Copy Markdown
Contributor Author

@SergeyMenshykh the partial-class feedback (discussion_r3167442374) has been addressed in c5ab40f — the outer KernelFunctionLogMessagesTests class was made partial so the source-generated additions for RestrictedJsonContext compile cleanly (SYSLIB1032). The other two Copilot suggestions (MEAItype → MEAI type, and the AggregateException fallback in the catch block) are also in via 5c6b4d7 and 07e344f respectively.

@rogerbarreto rogerbarreto added this pull request to the merge queue May 1, 2026
Merged via the queue into microsoft:main with commit 006a5d9 May 1, 2026
18 checks passed
@github-project-automation github-project-automation Bot moved this from Community PR to Done in Agent Framework May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kernel.core kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Bug: NotImplementedException on SK Logging

6 participants