fix(agui): give reasoning messages a distinct messageId from the text answer#1778
fix(agui): give reasoning messages a distinct messageId from the text answer#1778ql-wade wants to merge 2 commits into
Conversation
…te_file is gated (D-08) D-08 follow-up: the prior commit (admin-service 34d25c5d) seeded a non-trivial permission context on the agent builder (b.permissionContext(...)) and its unit test passed, but live write_file still ran unchecked. Root cause, confirmed by reading the load path: ReActAgent.loadOrCreateAgentStateForSlot returns a persisted AgentState verbatim, and only falls back to freshState(initialCtx) on a store MISS. So the builder's initialPermissionContext only reached FRESH sessions; any session persisted before the builder seeded a non-trivial context kept its trivial context (DEFAULT, no rules) forever -> isTrivial() true -> evaluatePermissions took the lightweight path -> write_file's passthrough self-check ALLOWed it -> RequireUserConfirmEvent never fired. Fix: in loadOrCreateAgentStateForSlot, merge the builder's initial permission context onto the loaded state when (and only when) the loaded state's context is still trivial. This upgrades stale persisted sessions in place while preserving their conversation/tool state. A session whose mode was deliberately toggled via setPermissionMode is left untouched (non-trivial by mode change). Acceptance test (ReActAgentStalePermissionContextTest) drives the real ReActAgent + real AgentStateStore against a pre-seeded trivial persisted session and a passthrough write_file tool (mirroring the harness FilesystemTool), asserting PERMISSION_ASKING + RequireUserConfirmEvent. Red without the merge, green with. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AguiAgentAdapter used the same msg.getId() for both the TEXT_MESSAGE_* (answer) and REASONING_MESSAGE_* (thinking) derived from a single agent message. In AG-UI these are independent messages (roles 'assistant' and 'reasoning') associated only by runId and order, never by messageId. Sharing one messageId makes clients group them into a single message bubble — e.g. the answer gets folded into the reasoning/thought panel instead of rendering as a standalone, always-visible message. Use msg.getId() + "-reasoning" for the reasoning message so it renders independently and stays collapsible. A suffix (rather than a fresh UUID) keeps multiple ThinkingBlocks in one message deduped into a single reasoning message, mirroring the text answer. Added a regression test asserting distinct messageIds.
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR contains two unrelated but individually correct fixes: (1) giving AG-UI reasoning messages a distinct messageId with -reasoning suffix to prevent client-side message collapsing, and (2) fixing stale PermissionContextState being discarded when the builder's initial trivial state overwrites loaded non-trivial state. Both fixes are correct in logic and well-tested. However, the PR description only mentions the first fix, and the two changes should ideally be in separate PRs.
| * @param initialPermCtx the builder's initial permission context (may be {@code null}) | ||
| * @return {@code loaded} unchanged when it already has a non-trivial context or no initial | ||
| * context was supplied; otherwise a copy of {@code loaded} with the initial context applied | ||
| */ |
There was a problem hiding this comment.
[recommended] PR contains two unrelated fixes (AG-UI messageId + ReActAgent permission context merge) but only describes the first. Consider splitting into two PRs for easier review, bisect, and revert. Also, the mergeInitialPermissionContext method manually copies 10+ fields from AgentState — if AgentState gains new fields in the future, this method will silently drop them. Consider adding a Builder.from(AgentState) factory method to prevent this maintenance hazard.
|
|
||
| @Test | ||
| void testRunWithStreamingThinkingBlockEvents() { | ||
| // Test streaming reasoning events when enabled |
There was a problem hiding this comment.
[nitpick] The streaming reasoning test asserts event counts but does not verify the messageId ends with -reasoning. Consider adding an assertion on the actual messageId value.
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR contains two unrelated but individually correct fixes: (1) giving AG-UI reasoning messages a distinct messageId with -reasoning suffix to prevent client-side message collapsing, and (2) fixing stale PermissionContextState being discarded when the builder's initial trivial state overwrites loaded non-trivial state. Both fixes are correct in logic and well-tested. However, the PR description only mentions the first fix, and the two changes should ideally be in separate PRs.
| * @param initialPermCtx the builder's initial permission context (may be {@code null}) | ||
| * @return {@code loaded} unchanged when it already has a non-trivial context or no initial | ||
| * context was supplied; otherwise a copy of {@code loaded} with the initial context applied | ||
| */ |
There was a problem hiding this comment.
[recommended] PR contains two unrelated fixes (AG-UI messageId + ReActAgent permission context merge) but only describes the first. Consider splitting into two PRs for easier review, bisect, and revert. Also, the mergeInitialPermissionContext method manually copies 10+ fields from AgentState — if AgentState gains new fields in the future, this method will silently drop them. Consider adding a Builder.from(AgentState) factory method to prevent this maintenance hazard.
|
|
||
| @Test | ||
| void testRunWithStreamingThinkingBlockEvents() { | ||
| // Test streaming reasoning events when enabled |
There was a problem hiding this comment.
[nitpick] The streaming reasoning test asserts event counts but does not verify the messageId ends with -reasoning. Consider adding an assertion on the actual messageId value.
Problem
When
enableReasoning=true, a single assistant message carrying both aThinkingBlockand aTextBlockemitsREASONING_MESSAGE_*andTEXT_MESSAGE_*that share the samemessageId(both usemsg.getId()):AG-UI clients key message grouping on
messageId, so they collapse the reasoning and the answer into one message bubble. Observed with CopilotKit: the answer gets folded into the "Thought for a few seconds" panel and cannot be shown as a standalone, always-visible message.Root cause
In
AguiAgentAdapter#convertEvent, theTextBlockbranch (→TEXT_MESSAGE_*) and theThinkingBlockbranch (→REASONING_MESSAGE_*) both set:Per the AG-UI protocol, a reasoning message (role
"reasoning") and the text answer (role"assistant") are independent messages — associated only byrunIdand order, never bymessageId(the event records carry no linkage field). Reusing one id therefore conflates two distinct messages.Fix
Give the reasoning message a distinct id:
msg.getId() + "-reasoning".UUID) keeps multipleThinkingBlocks in one message deduped into a single reasoning message, mirroring how the text answer is one message.Verification
testReasoningAndTextAnswerHaveDistinctMessageIds: a message with both aThinkingBlockand aTextBlockmust emit start events with different messageIds (msg-both-reasoningvsmsg-both). Full suite green (36/36).POST /agui/run: before the fix the reasoning+answer LCA was inside the collapsiblecpk:gridcontainer; after the fix the LCA is the top-level message list → reasoning and answer render as separate messages.Scope
One-line behavioral change + comment + test. No changes to event types or wire shape.