fix(agui): reconcile replayed tool results with session state - #3046
fix(agui): reconcile replayed tool results with session state#3046guslegend0510 wants to merge 12 commits into
Conversation
Preserve pending results across client assistant turns and deduplicate consumed results before recovery or context insertion. Keep unknown IDs, duplicates, partial results with text, and permission confirmation strict. Fixes agentscope-ai#3043
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Reconciles replayed AG-UI tool results with server-side session state: the AG-UI processor now keeps the full client transcript for ReAct-backed agents (RuntimeContext.REPLAYED_INPUT), and ReActAgent selects the follow-up messages after authoritative state is loaded, while consumed results are stripped before recovery/context insertion. Good, well-scoped fix for a genuinely subtle protocol bug, and 22 regression cases plus EN/ZH docs is a strong evidence base. I have two robustness questions about transcript shapes that the current selection window may not cover, plus API-surface nits. No blocking correctness issue found in the paths I could trace.
CLA: no license/cla status context was found on this PR's head commit (unknown), so this review is posted as a comment rather than an approval — approvals are reserved for PRs where CLA state is verifiable.
Findings:
- [Warning]
ReActAgent.java:2077— selection only rescans indices<= lastAssistant; a pending result appearing after the last assistant message falls into the "replay the old user prompt" fallback. - [Warning]
ReActAgent.java:761— unchecked(CallExecution) callScopecast in the newprepareInputMessagesoverride. - [Info]
RuntimeContext.java:43— new cross-module flag modelled as a raw string attribute. - [Info]
AguiRequestProcessor.java:166—addResumeInterruptshoisting changes input for the non-ReAct + server-memory branch.
Automated review by github-manager-bot
|
|
||
| @Override | ||
| protected List<Msg> prepareInputMessages(List<Msg> msgs, Object callScope) { | ||
| CallExecution scope = (CallExecution) callScope; |
There was a problem hiding this comment.
prepareInputMessages does an unchecked cast (CallExecution) callScope and then calls selectReplayedMessages. Two things worth hardening: (1) if callScope is ever null or a different type (a subclass of ReActAgent overriding beforeAgentExecution, or a future hook that passes a different scope) this becomes a raw ClassCastException/NPE inside the call path instead of a clear error; (2) the base-class contract for prepareInputMessages is not documented here. An instanceof check that returns msgs unchanged on mismatch (or making the hook signature generic/typed) would keep the replay optimisation strictly best-effort and non-breaking for subclasses.
| break; | ||
| } | ||
| } | ||
| if (lastAssistant < 0) { |
There was a problem hiding this comment.
selected.addAll(msgs.subList(lastAssistant + 1, msgs.size())) keeps everything after the last assistant message, and in the selected.isEmpty() fallback the single last USER message is re-appended. Please confirm the ordering guarantees for the case where a tool result for a pending call appears after the last assistant message in the client transcript — with the fallback path, a mid-transcript pending result would be dropped and only the old user prompt replayed, which is exactly the failure mode the PR sets out to fix. If that transcript shape is possible with the AG-UI client, a scan over the whole transcript (not only indices <= lastAssistant) would be more robust; if it is guaranteed impossible, it would help to state that as an assumption in the javadoc of RuntimeContext.REPLAYED_INPUT.
| * incremental input or stateless, client-owned history. | ||
| */ | ||
| public static final String REPLAYED_INPUT = "agentscope_replayed_input"; | ||
|
|
There was a problem hiding this comment.
Nit on the public API surface: this introduces a new cross-module contract (agentscope-extensions-agui writes the flag, agentscope-core reads it) as a raw string-keyed boolean attribute. Since it is a first-class behavioural switch rather than user data, consider a typed accessor on RuntimeContext (e.g. isReplayedInput() / builder().replayedInput(true)) so the flag cannot be misspelled or set to a non-Boolean value, and so it shows up in the typed-attribute API rather than in the free-form get(String) bag.
| } | ||
|
|
||
| try { | ||
| RuntimeContext effectiveRuntimeContext = |
There was a problem hiding this comment.
Moving addResumeInterrupts(input, runtimeContext) above the memory/extractLatestUserMessage branch changes which input the resume interrupts are computed from (full transcript instead of the extracted last user message, for the non-ReAct-with-memory path it is now built before truncation). It looks intentional and the tests cover the ReAct case; could you add (or point to) an assertion for the non-ReAct + server-memory path so the ordering change stays pinned? That is the only branch where the pre-existing behaviour could silently drift.
3 participants
AgentScope-Java Version
2.0.3-SNAPSHOT
Description
Fixes #3043
Stateful AG-UI clients can replay tool results across multiple local assistant turns. Previously, transcript extraction could discard a result the server still needed, while forwarding already-consumed results that caused subsequent requests to fail.
This change:
Orphan tool results are now rejected even when no calls are pending. Initial client-owned history can still include results paired with preceding assistant tool calls.
Validation:
Checklist