fix(dotnet): map reasoning and skip activity in AsChatMessages - #2295
Open
1aifanatic wants to merge 1 commit into
Open
fix(dotnet): map reasoning and skip activity in AsChatMessages#22951aifanatic wants to merge 1 commit into
1aifanatic wants to merge 1 commit into
Conversation
AGUI.Abstractions declares seven roles in AGUIRoles and deserializes all seven in AGUIMessageJsonConverter, but AsChatMessages called MapChatRole unconditionally and MapChatRole handled only five. A single reasoning message anywhere in RunAgentInput.Messages threw "Unknown chat role: reasoning", surfacing as an HTTP 500. The spec has clients send reasoning messages back to the agent on subsequent turns, so a conforming client that echoes them fails on turn 2 of every thread where the model reasoned — and keeps failing, because the message is now part of the client's authoritative history. - reasoning maps to an assistant ChatMessage carrying TextReasoningContent, MEAI's model for the same concept. encryptedValue becomes ProtectedData, the opaque provider blob MEAI round-trips untouched, preserving encrypted chain-of-thought continuity across turns (store:false / ZDR). - activity is dropped. It is frontend-only and never forwarded to the agent, so a client that sends one anyway should be ignored, not 500. Dropped before the parallel-tool-call bookkeeping so it stays fully transparent and cannot split a run mid-coalesce. - MapChatRole maps reasoning to Assistant so the public helper no longer throws for a role the SDK itself defines. Activity has no ChatRole equivalent and AsChatMessages no longer routes it there; that is now documented on the method. The now-unreachable AGUIReasoningMessage arm in the plain-text switch is removed, since reasoning returns before it. Adds a role-coverage theory asserting that everything AGUIMessageJsonConverter can deserialize, AsChatMessages can consume without throwing, so the three lists cannot drift apart again. Fixes ag-ui-protocol#2290
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2290
Problem
AGUI.Abstractionsis internally inconsistent about message roles across three lists:AGUIRolesactivityandreasoningAGUIMessageJsonConverterMapChatRoleInvalidOperationExceptionon the other twoAsChatMessagescallsMapChatRoleunconditionally before any per-message-type branching, so one reasoning message anywhere inRunAgentInput.Messagesfails the whole run — an HTTP 500 from a hosted agent endpoint.This isn't an edge case. The spec says reasoning messages "are meant to be sent back to the agent for further processing on subsequent turns", so a conforming client that echoes them gets a 500 on turn 2 of every thread in which the model reasoned — and every turn after, because the offending message is now part of the client's authoritative history. The thread is permanently unusable.
Fix
reasoning→ an assistantChatMessagecarryingTextReasoningContent, which is howMicrosoft.Extensions.AImodels the same concept.encryptedValuemaps toProtectedData— MEAI's documented home for "an opaque blob of data... that should be roundtripped back to the provider... often encrypted". That is what keeps encrypted chain-of-thought continuous across turns, which is the entire point of the field (and required forstore:false/ ZDR).activity→ dropped. It's frontend-only and "never forwarded to the agent", so a client that sends one anyway should be ignored rather than 500. It's dropped before the parallel-tool-call bookkeeping so the message is fully transparent — an interleaved activity must not split a tool-call run thatAsChatMessagesis mid-coalesce (there's a test for exactly that).MapChatRolenow mapsreasoning→ChatRole.Assistant, so the public helper no longer throws for a role the SDK itself defines.activitygenuinely has noChatRoleequivalent, andAsChatMessagesno longer routes it there; I documented that on the method rather than inventing a bogus mapping. SinceAGUIMessageJsonConverteralready rejects unknown role discriminators with aJsonException, the remainingthrowis now unreachable from the deserialization path — it only guards programmatically-constructed messages.Also removed the
AGUIReasoningMessage reasoning => reasoning.Contentarm from the plain-text switch: reasoning now returns before it, so it was dead code that read as if reasoning were already handled. (That phantom handler is arguably how this drifted in the first place.)Tests
13 new tests in
AGUIChatMessageExtensionsTest, including the exact turn-2RunAgentInputpayload from the issue.The issue asked for a guard so the three lists can't drift again — that's
AsChatMessages_EveryDeserializableRole_DoesNotThrow, a[Theory]over all seven roles asserting that anythingAGUIMessageJsonConverterdeserializes,AsChatMessagesconsumes without throwing. 8 of the 13 fail onmain.Full .NET suite (net10.0), all green with this change:
AGUI.Abstractions.UnitTestsAGUI.Server.UnitTestsAGUI.Client.UnitTestsAGUI.Hosting.AspNetCore.IntegrationTestsAGUI.Protobuf.UnitTestsAGUI.Formatting.UnitTestsAGUI.Abstractionsbuilds clean (0 warnings,TreatWarningsAsErrors) across all five TFMs:net10.0,net9.0,net8.0,netstandard2.0,net472. No public API surface added, so noPublicAPI.Unshipped.txtchange.Scope note
The outbound direction is still asymmetric:
AsAGUIMessagesnever produces anAGUIReasoningMessage, so aTextReasoningContentcoming back from a provider maps to a plain assistant message. That's pre-existing and outside this issue, but this change makes it more visible — happy to send a follow-up if you'd like the reverse mapping too.