feat(agui): convert reasoning messages - #3239
akkupratap323 wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
This PR closes a real gap: AguiMessageConverter used to drop ThinkingBlock content entirely, so non-streaming AG-UI consumers never saw reasoning. Adding a reasoning role plus reasoningMessage(...) / toAguiMessages(...) and wiring toAguiMessageList through flatMap is the right shape, the mixed-turn split is tested in both directions, and the appendContent extraction removes the repeated length() > 0 boilerplate cleanly. Thanks for the thorough test coverage on a first contribution!
No blocking issues — I left the findings as inline comments, mostly about round-trip fidelity and about keeping the new non-streaming behaviour consistent with the existing streaming path:
- [Warning] the streaming path gates reasoning behind
AguiAdapterConfig.isEnableReasoning()(ThinkingBlockEventConvertertoo), while the converter path now always emits it — worth aligning or documenting. - [Warning]
msg.getId() + "-reasoning"is neither collision-proof nor reversible;toMsgcannot restore the original id. - [Warning]
ThinkingBlock.getMetadata()(METADATA_REASONING_DETAILS, OpenRouter/Gemini encrypted reasoning) is dropped on both directions. - [Warning]
toMsgon a reasoning message withMessageContent.Blocksthrows the generic "only supported for AG-UI user messages" error. - [Info]
toAguiMessageListbecomes 1:N — public API behaviour change worth a changelog note;toAguiMessage(singular) still loses reasoning for mixed turns. - [Info]
reasoningcomes from the AG-UI Reasoning draft; linking the spec in the javadoc/description helps consumers who validate against the stable role enum.
Verdict: COMMENT — the design is sound and the concerns above are non-blocking polish. Looks ready for maintainer attention once CI is green.
Automated review by github-manager-bot
| * @param msg The AgentScope message to convert | ||
| * @return The converted AG-UI messages | ||
| */ | ||
| public List<AguiMessage> toAguiMessages(Msg msg) { |
There was a problem hiding this comment.
Nice to see reasoning finally reaching non-streaming consumers. One asymmetry worth noting though: the streaming path in AguiAgentAdapter only emits REASONING_* events when AguiAdapterConfig.isEnableReasoning() is true, while toAguiMessages/toAguiMessage emit a reasoning message unconditionally. A deployment that turned reasoning off for streaming will now still get thinking content through the non-streaming conversion path. Would it make sense to gate this behind the same flag (e.g. an enableReasoning option on the converter, defaulting to the current behaviour) so both paths agree?
|
|
||
| return List.of( | ||
| AguiMessage.reasoningMessage( | ||
| msg.getId() + "-reasoning", reasoningContent.toString()), |
There was a problem hiding this comment.
Two related concerns with the derived ID:
msg.getId() + "-reasoning"can collide with an ID that already exists in the conversation (IDs are caller-supplied), andAguiMessageonly enforces non-null, not uniqueness.- It is not reversible: feeding the pair back through
toMsgproduces an assistantMsgwith id<id>-reasoninginstead of the original<id>, so aMsg -> AguiMessage -> Msground-trip no longer preserves identity.
Could the suffix be made collision-resistant (hash/UUID or a reserved separator) and/or the origin id carried in message metadata so the reverse conversion can restore it? At minimum a javadoc note on the ID contract would help downstream consumers.
| content.append(tb.getText()); | ||
| appendContent(content, tb.getText()); | ||
| } else if (block instanceof ThinkingBlock tb) { | ||
| appendContent(reasoningContent, tb.getThinking()); |
There was a problem hiding this comment.
ThinkingBlock also carries getMetadata() (METADATA_REASONING_DETAILS, used by providers such as Gemini/OpenRouter to keep signed reasoning details across turns). Here only getThinking() is propagated, and addThinkingBlock on the reverse path never restores metadata, so a Msg -> AguiMessage -> Msg round-trip silently drops it. If reasoning details are needed to continue a conversation with those providers, that can degrade into provider errors or lost tool-call continuity. Either propagate the metadata (e.g. as an AG-UI message name/metadata field) or document explicitly that AG-UI reasoning messages are lossy by design.
| MessageContent content = aguiMessage.getContent(); | ||
| if (content instanceof MessageContent.Text text) { | ||
| addTextBlock(blocks, text.value(), aguiMessage); | ||
| if (aguiMessage.isReasoningMessage()) { |
There was a problem hiding this comment.
The reverse conversion only handles MessageContent.Text. A reasoning message deserialized with structured MessageContent.Blocks falls into the blocksContent branch below, where !isUserMessage() throws IllegalArgumentException("Structured content blocks are only supported for AG-UI user messages"). That is technically consistent with assistant/system/tool messages, but since reasoning is a brand-new role, it would be friendlier to either convert the text parts into ThinkingBlocks or raise a role-specific error message. A test for this input shape would pin the intended behaviour.
| public List<AguiMessage> toAguiMessageList(List<Msg> msgs) { | ||
| return msgs.stream().map(this::toAguiMessage).collect(Collectors.toList()); | ||
| return msgs.stream() | ||
| .flatMap(msg -> toAguiMessages(msg).stream()) |
There was a problem hiding this comment.
toAguiMessageList changes from a 1:1 map to a 1:N flatMap, which is an observable behaviour change on a public API of this extension (callers that zipped the result back against the input list by index would break). Grep shows no in-repo production caller today, so this is mainly about the changelog: worth calling out in the release notes / class javadoc that the output size is no longer equal to the input size. Also, toAguiMessage (singular) still drops the reasoning part for mixed turns, so callers who use it directly keep the old data loss - consider deprecating it in favour of toAguiMessages.
| * @param content The reasoning content as plain text | ||
| * @return A new reasoning message | ||
| */ | ||
| public static AguiMessage reasoningMessage(String id, String content) { |
There was a problem hiding this comment.
Minor: reasoning is not part of the stable AG-UI Role enum, it comes from the Reasoning draft (the streaming adapter references the same draft). Downstream consumers that validate roles against the stable enum will reject these messages. Could the PR description or the javadoc link the exact draft spec being followed, so implementers know this is draft-gated and may change?
CI noteThe only failing required check is
Current Automated note by github-manager-bot |
AgentScope-Java Version
2.0.4-SNAPSHOT
Description
AguiMessageConvertercurrently dropsThinkingBlockcontent when converting AgentScope messages to AG-UI messages. This leaves non-streaming consumers without the reasoning message defined by the AG-UI protocol.This change:
reasoningrole and a reasoning-message factory toAguiMessage;ThinkingBlockinstances;toAguiMessage;toAguiMessagesand updates list conversion so mixed reasoning/assistant turns retain both parts with distinct IDs; andCloses #2859.
Validation
mvn -pl agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui spotless:applymvn -pl agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui -am -Dtest=AguiMessageConverterTest,AguiModelTest -Dsurefire.failIfNoSpecifiedTests=false test(98 tests, 0 failures)Checklist
mvn spotless:apply