Repository navigation
Conversation
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
f0f2f08 to
76413d6
Compare
|
CLA Not Signed The Contributor License Agreement (CLA) check is currently pending on this PR ( @dargoner please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/agentscope-ai/agentscope-java. Once signed, the Automated check by github-manager-bot |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
The TextOutputDispositionEvent layer is a well-motivated addition: keeping TextDeltaEvent intact and adding an opt-in INTERMEDIATE/TERMINAL classification (with per-source + taskId isolation and per-subscription state) is the right shape, and the Jackson subtype registration plus the dedicated tests are appreciated. The blocking concerns are the lifecycle of the buffered top-level AgentEndEvent, the fail-open success inference, and the unbounded tracker state.
Verdict: comment-only for this round — two critical correctness items (inline) plus a scope request. Not approving.
Findings
- [Critical]
AgentEventStreams.java:66—IllegalStateExceptionon a late event aborts the wrapped stream viaconcatMap, turning an ordering anomaly into a user-visible failure of an opt-in, purely additive signal. - [Critical]
AgentEventStreams.java:127— buffering the top-levelAgentEndEventuntilcomplete()(line 144) means the end event is dropped when the source errors and never arrives if the stream does not terminate; emitting[terminal, end]inline (as the child branch already does) removes both hazards. - [Warning]
AgentEventStreams.java:176—outcome == nullcounts as success, so any end withoutMETADATA_INVOCATION_OUTCOME(custom/forwarded subagents, replayed state) can yield aTERMINAL. - [Warning]
ReplyLifecycleTracker.java:140—computeIfAbsentplus no eviction on child end growsstateswithout bound, and each entry pinslastResult. - [Warning]
AgentEvent.java:40— a new@JsonSubTypesentry is a wire change; consumers on an older core fail on unknown type id, which conflicts with the "ignore it safely" compatibility claim. - [Info]
FinalAnswerFilterMiddleware.java:73— shared tracker now records unusedlastResultstate and widens reply-id recognition; needs its own guard-rail test.
Scope — please split this PR
This branch carries 187 commits, 249 files and ~34.4k additions, and includes substantial work unrelated to text-output disposition: skill/evolution/* (new SkillArtifactMaterializer, CanonicalSkillHasher), OtelTracingMiddleware changes, AguiResumeCoordinator rework, ReActAgent per-session-state and structured-output tests, sandbox/McpJsonDefaults fixes, and more. A reviewer cannot meaningfully approve a feature hidden inside that volume, and any regression cannot be attributed or reverted cleanly. Could you rebase this onto current main and keep only the disposition events, wrapper, tracker, the AG-UI/agent-protocol wiring, and their tests? The remaining changes belong in their own PRs.
mergeable_state is currently blocked, and the license/cla status is pending — the CLA reminder is posted separately. Once the CLA is signed and the scope is reduced, I am happy to re-review the trimmed diff.
Automated review by github-manager-bot
(cherry picked from commit 0db3ed7)
(cherry picked from commit 72e56f5)
(cherry picked from commit d09f6b5)
(cherry picked from commit 3ba05d8)
(cherry picked from commit c788fe9)
(cherry picked from commit 0fb5206)
(cherry picked from commit 6e3b5a1)
(cherry picked from commit 61f58ae)
(cherry picked from commit 1c8149e)
(cherry picked from commit 3ab4abe)
(cherry picked from commit 50ed765)
(cherry picked from commit 22a30bf)
按来源和任务关闭最后模型回复，并区分子代理成功、失败与取消结束。 补齐协议、AG-UI、前端和数据面的回归验证与最终修复报告。 (cherry picked from commit 80c3595)
透传子智能体生命周期、思考、文本处置、工具参数结果及任务标识。 修正 acting 中间件事件发布位置，避免事件遗漏或重复。 (cherry picked from commit 6af8e3b)
补齐本地子智能体的 AgentResultEvent 转发，避免只收到结束事件而缺少权威结果。 AG-UI 仅按当前运行实际生成的分段消息清理临时文本，避免误删合法历史消息。 Web 预览总线不再重放订阅前事件，并串行化并发发送以避免丢帧。 (cherry picked from commit 76413d6)
上游 agentscope-ai#3010 之后文本消息 ID 为 replyId-blockId（如 reply-1-text-1），本分支的 AguiStreamContext 仍把传入值当 replyId 再拼 :text:N，导致处置事件 messageIds 与流式消息 ID 不一致。 改为直接复用官方 messageId，并从段 ID 反推所属 replyId 维持处置分组；保留按本轮实际生成段判定快照排除的语义，同步更新测试期望与 README 说明。 (cherry picked from commit 42ad838)
42ad838 to
39ecc5f
Compare
维护者评审提出的两个 critical： 1. 迟到事件原先直接抛 IllegalStateException，经 concatMap 会转成 onError， 把一个可选的附加信号升级成用户可见的流失败。现改为记录告警后原样透传； 同一来源再次出现 MODEL_CALL_START 时视为新一次调用并恢复标注。 2. 顶层 AgentEndEvent 原先缓存到流结束后的 complete() 才发出，源出错时 end 事件丢失，流不终止时永不发出。现改为内联发出 [terminal, end]，并删除 pendingTopLevelEnds 与 complete()。 同时处理评审中的其余意见： - 子代理结束必须显式声明 OUTCOME_SUCCESS 才产生 TERMINAL，未知或异常 outcome 不再 fail-open。 - 每个来源在 AgentEndEvent 时清理 tracker 状态，并把结果关联移出 tracker， 避免 states 无界增长并 pin 住 AgentResultEvent。 - markDispositionEmitted 不再重建已清理的来源状态。 - 补充 AgentEndEvent 的 outcome 契约与新增事件的线格式兼容说明。 新增回归测试：结束先于源错误/源完成发出、迟到事件透传、子代理异常结束不产生 TERMINAL、tracker 状态清理、中间轮文本不泄漏进最终答案。
|
Thanks for the detailed review — all findings are addressed, and the scope split is done. Scope: the branch is rebased onto current Critical 1 — late events ( Critical 2 — buffered top-level end ( Warning — fail-open success ( Warning — unbounded tracker state ( Warning — wire format ( Info — middleware guard rail ( Info — tracker visibility and outcome contract: Verification: |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the trimmed branch (9b76667c..ffa69712). The scope reduction I asked for landed — 249 files / ~34.4k additions is now 40 files / +3225, the unrelated control-plane and skill-evolution work is gone, and both blocking items from the last round are genuinely fixed: late events after AgentEndEvent are now forwarded with a warning instead of failing the wrapped stream, and the top-level end event is emitted inline instead of being buffered until complete(). Opt-in AguiAdapterConfig.textOutputDispositionEnabled plus the README that spells out "terminal does not mean final answer" is the right shape. CI is green (build on ubuntu/windows, Check License, Check Module Sync) and license/cla is now passing.
Verdict: comment-only again — no blocking item left, but three correctness risks I would like resolved before this merges (inline).
Findings
- [Warning]
AgentEventStreams.java:155—endedSourcesis still append-only for child sources, so the unbounded-state problem is only half fixed. - [Warning]
AguiStreamContext.java:468— replyId is recovered by regex over the AG-UI message id; mis-parsing silently attaches dispositions to the wrong reply. - [Warning]
AguiStreamContext.java:274—MESSAGES_SNAPSHOTcan drop the submitted user turn whenAgentState.getContext()does not echo it, and consumers are told to replace streamed text with that snapshot. - [Warning]
ReActAgent.java:2787— publication moved fromactingStream(...)to the middleware-wrapped stream; a middleware that short-circuitsonActingno longer publishes core tool events. - [Info]
AguiStreamContext.java:236—getTextMessageIdsreturns different key spaces depending on the config flag. - [Info]
AgentEventStreams.java:219— the derived disposition inherits the trigger metadata map, includingMETADATA_INVOCATION_OUTCOME.
What I verified this round
forwardsLateEventsWithoutFailingOrClassifyingThemexists inAgentEventStreamsTest, so the fail-open behaviour has a regression test.tracker.clearSource(sourceKey)on child end and the explicitOUTCOME_SUCCESSrequirement for child terminals both close earlier warnings.SubagentEventConverternow stampstaskIdon forwarded events and carriesreplyId/blockIdthrough, which keeps multi-task subagent streams separable downstream.
Nice work on the rework — the diff is now reviewable, and the remaining items are narrow.
Automated review by github-manager-bot
维护者第二轮复审提出的四项风险： 1. 子来源墓碑无界：endedSources 里的子来源键按次生成（task_<uuid>），只增不减 等价于让一个订阅保留它见过的所有子调用。现在子来源在结束事件发出后立即回收， 顶层键保留到下一次模型调用重新打开，与 clearSource 的生命周期保持一致。 2. 派生处置事件继承了触发的整体 metadata：从 AgentEndEvent 派生的 TERMINAL 会带上 METADATA_INVOCATION_OUTCOME，消费者据此判断会与权威结果矛盾。现在只复制 taskId 这一关联键。 3. AG-UI 的 replyId 靠消息 id 正则反推：任何以 -text/-thinking/-reasoning 结尾的合法 id 都会被归到别的 replyId，非文本转换器产生的 id 还会落到 messageId==replyId 的 伪桶。现在文本转换器显式传入 replyId，正则只作兼容回退。 4. MESSAGES_SNAPSHOT 会丢掉本轮用户消息：仅当 agent 状态回显该消息时才保留 run 输入，而消费者按快照替换流式文本，状态未回显时该轮次在 UI 上消失。现在 run 输入 始终保留。 同时 getTextMessageIds 不再随 textOutputDispositionEnabled 返回不同键空间， 并删除只写不读的 activeTextMessageIdsByReply 与 currentTextReplyId。 新增回归测试：子来源回收与顶层墓碑释放、处置事件只带关联键、中间轮文本不泄漏、 拒绝工具的 DENIED 结果在 acting 中间件下仍发布、询问工具的停止路径仍发布、 agent 状态不回显用户轮次时快照仍保留该轮次。
e367ee6 to
c9503ab
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review on the synced head d83765e5 in response to your @mention — this closes my last open thread, so approving.
- Coverage gap I asked about is gone. CI now runs on the merge itself:
build (ubuntu-latest),build (windows-latest),Check License,Check Module Syncandcodecov/patchare all green ond83765e5, so the combined content ofReActAgent.javais compiled and tested rather than only the pre-merge branch state. - The conflict resolution kept main's side intact. In
ReActAgent.javathe#3104auto-deny lifecycle is all still present on your head —gate.autoDeniedIds(),writeAutoDeniedResults(...),deniedToolResultEvents(...)and thePERMISSION_DENIED_BY_RULESpath (10autoDeniedreferences, same asmain@70686e4a) — and your reorder that publishes events once after theonActingmiddleware chain is preserved. No leftover conflict markers anywhere in the file, and the only-lines versus main are the two statements your commit deliberately moved, not upstream code silently reverted. - CLA now reports
successon this head, which was the second thing holding the previous pass back. mergeable: MERGEABLE,mergeStateStatus: CLEAN.
Thanks for the careful rebase and for reporting exactly which files were in conflict — that made the resolution straightforward to verify.
Automated review by github-manager-bot
SummaryRe-review after the rebase onto What I verified in the CI on Thanks for the rebase and the conflict notes — resolving Automated review by github-manager-bot — re-review in response to the @mention request on #3013. |
Review findings (line-anchored)Posting these as a comment because this account already has an unresolved pending review draft on the branch ( [WARNING]
Could you confirm the intended threading contract? If a tracker is strictly one-per-subscription, a line in the class javadoc saying so is worth having for the next reader (and the [INFO] This Automated review by github-manager-bot — re-review of #3013 after the rebase to |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Superseded: this was an unsubmitted draft left over from an earlier review pass. The current re-review of head 816027a0 follows as a separate review.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review on head 816027a0, in response to the re-review requests on this thread. The delta since the last pass (492488d4) is upstream sync only — three merge: 同步上游 main commits, no new author commits — so this pass verified the sync instead of re-reading the feature:
- Merge resolution is correct. Upstream state is intact in the regions that conflicted:
assembleModelConfigstill threadsflatFailoverListenerintoModelConfig(#3145/#3149), pending-tool-id handling still resolves throughMessageUtils.pendingToolUseIds(state.contextMutable()),persistAgentStateCasstill uses thesaveIfVersion(...)unconditional-write form (#3220/#3236), and thetotalCacheCreation/totalReasoning/totalToolUsePromptusage accumulators (#3215) survived. This PR's owndoOnNext(this::publishEvent)reorder is unchanged, and no conflict markers are left anywhere in the tree. - Both conditions from the previous round are met.
license/clareports success on this head, and the one thread that was still open here asked for exactly what you did: sync main and let CI run once more on the merge. On816027a0build (ubuntu-latest),build (windows-latest),Check License,Check Module Syncandcodecov/patchare all green, 23 of 24 review threads are resolved, andmergeableistrue/clean.
Approving. One non-blocking observation for whoever touches this next: AguiStreamContext.TEXT_SEGMENT_ID (^(.+)-(?:text|thinking|reasoning)(?:-\d+)?$) recovers the reply id with a greedy group, so an id that itself ends in one of the reserved suffixes resolves to the longest prefix ("a-text-text" → reply id "a-text"). That is inherent to the reserved-suffix scheme rather than a defect — worth one regression test asserting the intended behaviour, since replyId is caller-supplied in the AgentResultEvent path.
Merging stays a maintainer action — this bot never merges.
Automated review by github-manager-bot
| actingCore) | ||
| .apply(new ActingInput(toolCalls)); | ||
| return stream.doOnNext( | ||
| return stream.doOnNext(this::publishEvent) |
There was a problem hiding this comment.
Merge-sync verification (not a new finding): this is the region where #3145/#3149 and the CAS work in #3236 landed upstream after the previous review head. Re-checked on 816027a0: the upstream state is intact — assembleModelConfig still threads b.flatFailoverListener into ModelConfig, the pending-tool-id helpers still resolve through MessageUtils.pendingToolUseIds(state.contextMutable()), persistAgentStateCas still uses the saveIfVersion(...) unconditional-write form, and the new usage accumulators (totalCacheCreation / totalReasoning / totalToolUsePrompt) are present. This PR's own doOnNext(this::publishEvent) reorder survived the sync unchanged, so nothing upstream is silently reverted here. Reopening the previous round's [Info] thread condition (sync main and let CI run on the merge) is satisfied: build (ubuntu-latest), build (windows-latest), Check License, Check Module Sync and codecov/patch are all green on this head.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Adds a TEXT_OUTPUT_DISPOSITION lifecycle signal so streaming consumers can tell an intermediate reply from the terminal one for an invocation, backed by a new shared ReplyLifecycleTracker, and refactors FinalAnswerFilterMiddleware plus the AGUI stream context / sub-agent converters onto it. The extraction is a real improvement — the per-round replyId/toolCallSeen duplication in the filter is gone, the new class is documented as internal-only, the persistence caveat for the new event type is spelled out in its javadoc, and AgentEventStreams keeps its per-subscription state bounded by evicting on AGENT_END/AGENT_RESULT. Two items below; the first one is carried over from the previous round and still stands.
Findings
- [Warning]
ReplyLifecycleTracker.java:87-92— the unsynchronizedLinkedHashMap(LRU eviction viaremoveEldestEntry, and access-ordered mutation on plainget()) is still shared across packages whileobserve()mutates it. Either confine one tracker to one subscriber (say so in the javadoc) or make the map thread-safe with an explicit size guard. - [Info]
FinalAnswerFilterMiddleware.java:73—observe()now records aReplyStateper childsource/taskIdthat this middleware never reads, so a high-fan-out round allocates up toMAX_TRACKED_CHILD_SOURCESentries purely as overhead beforetracker.clear()at line 113 resets everything.
Notes
No new commit-level regression found relative to 492488d4; tests cover the tracker's blank-delta, tool-call-before-text, disposition and clearSource paths, which is the right surface. Two process notes: this PR still has a dangling empty pending review draft on the branch (flagged last round) which blocks inline comments for this account — a maintainer with repo rights can dismiss it; and the PR is CONFLICTING/DIRTY against main, which the rebase reminder on the thread already covers.
Automated review by github-manager-bot
|
|
||
| private final Map<SourceKey, ReplyState> childStates = | ||
| new LinkedHashMap<>() { | ||
| @Override | ||
| protected boolean removeEldestEntry(Map.Entry<SourceKey, ReplyState> eldest) { | ||
| return size() > MAX_TRACKED_CHILD_SOURCES; |
There was a problem hiding this comment.
This is a re-report of the previous round's finding: childStates is still a plain LinkedHashMap (with removeEldestEntry eviction), mutated from observe()/clearSource()/markDispositionEmitted() and read from snapshot(), with no synchronization, while the class javadoc still says it is public so several components can share one instance. A sharing contract needs either a lock (or ConcurrentHashMap + explicit size guard — note LinkedHashMap is also access-ordered here via the single-arg override, so even a get() structurally mutates it), or an explicit "must be confined to one subscriber/thread" statement in the javadoc plus an assertion. If the new tests only ever drive one thread, the concurrent case stays untested.
| if (event instanceof ModelCallStartEvent start) { | ||
| replyId = start.getReplyId(); | ||
| toolCallSeen = false; | ||
| Observation observation = tracker.observe(event); |
There was a problem hiding this comment.
tracker.observe(event) now runs for every event the filter sees, so childStates accumulates a ReplyState for each child source/taskId pair flowing through the stream even though RoundState only ever consumes the top-level reply (tracker.clear() at line 113 is the only reset). Functionally neutral today, but for long-lived streams with high sub-agent fan-out it means an LRU of up to MAX_TRACKED_CHILD_SOURCES (4096) is allocated per round purely as overhead. Either skip observation for non-top-level sources here, or drop childStates for this call site by using a top-level-only tracker.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed on 85b6520d (32 commits, 42 files, +3038/-253). The design holds up and the earlier round's issues stay fixed: the disposition layer is genuinely additive (Flux.defer per subscription, bounded per-source bookkeeping, AgentEndEvent forwarded as soon as it is observed, ordering anomalies logged and forwarded rather than failing the stream), the top-level terminal correctly requires an authoritative AgentResultEvent, and FinalAnswerFilterMiddleware losing its private replyId/toolCallSeen duplication to the shared tracker is the right consolidation. The new event type's serialization caveat is stated plainly in its own javadoc, which is more than most PRs do. CI is green (Check License, Check Module Sync, build (ubuntu-latest), build (windows-latest), codecov/patch) and the PR is mergeable.
Two things are worth settling before merge, both about reach beyond the local happy path rather than about the core algorithm. The detail=full rule now admits AGENT_RESULT passthrough for every existing subagent consumer, which is a wire change bundled into an opt-in feature and not gated by textOutputDispositionEnabled; and the "a subagent end must report OUTCOME_SUCCESS" rule has no counterpart on the remote decode path, where RUN_FINISHED arrives with no metadata at all, so remote children silently never classify. My note on the child-source cap is only about making its failure mode observable.
Findings
- [Warning]
RemoteEventCodec.java:258—isAuthoritativeFullEventalso matchesAGENT_RESULT, sodetail=full(the default for a streaming parent) now carries passthrough DTOs thatAGENT_EVENT -> VERBOSEused to gate; the change is not behind the opt-in flag. - [Warning]
AgentEventStreams.java:186— the successful-child-end requirement has no remote counterpart:RemoteEventCodecline 174 decodesRUN_FINISHEDinto anAgentEndEventwith noMETADATA_INVOCATION_OUTCOME, so a remote child's final reply is never classified and nothing says why. - [Info]
ReplyLifecycleTracker.java:89— the 4096-child eviction is silent, and after evictionFinalAnswerFilterMiddlewarestops honouring its own "final answer only" contract for that source rather than degrading visibly.
Verified good
withTextOutputDispositionnever fails or reorders the source stream:endedSourcesgating forwards post-end events unchanged (reopening only on a newModelCallStartEvent), and per-source state is reclaimed atAGENT_END, so a long fan-out subscription does not accumulate sources.- The compatibility posture is honest. Registering
TEXT_OUTPUT_DISPOSITIONadditively inAgentEvent's@JsonSubTypesmeans an older reader cannot resolve it, andTextOutputDispositionEvent's javadoc says so and tells producers to strip it before persistence — the right way to hand a derived event to a wire. - The
FinalAnswerFilterMiddlewarerefactor is behaviour-preserving on the paths I could compare line by line:currentReplyEvent()matches the oldisCurrentReply,clear()resets the same fields, andobserve()'s mutations do not change what the filter branches on. TextOutputDispositionEventcarries only the correlation keys (source,METADATA_TASK_ID) rather than the trigger's metadata, which is the correct call — the trigger'sinvocationOutcomedescribes the invocation, not the classification.AgentEndEvent's new metadata constants are documented from the consumer's side (what a producer must set to keep replies classifiable), andAgentSpawnToolnow tags success / error / cancelled ends distinctly withtagForwardedEventso theendEmittedguard cannot leak a duplicate on the cancel race.
Automated review by github-manager-bot
| RemoteStreamDetail level = RemoteStreamDetail.parse(detail); | ||
| return level.includes(RemoteStreamDetail.VERBOSE) | ||
| || (level.includes(RemoteStreamDetail.FULL) | ||
| && isAuthoritativeFullEvent(event.getEventType())); |
There was a problem hiding this comment.
isAuthoritativeFullEvent also matches AGENT_RESULT, so detail=full now admits RemoteEventType.AGENT_EVENT passthrough DTOs where the previous rule was AGENT_EVENT -> level.includes(VERBOSE). full is the default for a streaming parent, which means this widens the wire for every existing consumer of a running subagent, not only for callers that opt into disposition: a parent that filtered AGENT_EVENT out at full and treated its absence as "the interesting events are the typed ones" now receives authoritative result DTOs it never asked for, and the change is not gated by AguiAdapterConfig.textOutputDispositionEnabled.
The disposition half really is additive — an old client drops the unknown subtype in deserializeEvent and then through the AGENT_EVENT -> Optional.empty() fallback, which is graceful and matches the compat note you added to TextOutputDispositionEvent. The result half is a separate, unrelated behaviour change riding along with it. Please gate AGENT_RESULT-at-full behind the same opt-in flag, or split it out and state it in the release notes as its own wire change, so consumers do not have to bisect a feature PR to find why their stream grew.
| event)); | ||
| tracker.markDispositionEmitted(sourceKey); | ||
| } | ||
| } else if (isSuccessfulChildEnd(event)) { |
There was a problem hiding this comment.
This branch requires a subagent end to carry METADATA_INVOCATION_OUTCOME equal to success before the reply closes, which is the right conservatism for locally synthesized ends — but RemoteEventCodec line 174 decodes the wire's RUN_FINISHED into new AgentEndEvent(null) with no metadata, and withTypedFields gives it nothing either. So a remote child whose server predates this PR (or any path where the end arrives as a typed RUN_FINISHED rather than a JSON payload) produces a child reply that is never classified: no terminal disposition, silently, with no log line to explain the gap.
Over the wire RUN_FINISHED is the success signal, so the two halves of one protocol now disagree about what that end means. If the intent is "remote children classify only once both sides are new", say so here and in the AgentEndEvent#METADATA_INVOCATION_OUTCOME javadoc, which currently tells producers only about the harness spawn tool. Otherwise tag that arm OUTCOME_SUCCESS — and its error/cancel counterparts with their own outcomes — so local and remote children behave the same way from the wrapper's point of view.
| private ReplyState topLevelState; | ||
|
|
||
| private final Map<SourceKey, ReplyState> childStates = | ||
| new LinkedHashMap<>() { |
There was a problem hiding this comment.
The 4096-child cap with removeEldestEntry is the right instinct for a long-lived subscription fanning out to many subagents, and it supersedes nothing here — my earlier note on this field was about its unsynchronized mutation, which is still open and posted separately on the annotator. This one is about what eviction does. It is silent, and the consequence is asymmetric: once a source's ReplyState is dropped, that source's later text events arrive with state == null, so currentReplyEvent() is false and the filter treats them as foreign. In FinalAnswerFilterMiddleware the visible effect is the worst available one — text that used to be buffered-suppressed starts passing through, so a stream that crossed the cap quietly stops honouring its "final answer only" contract instead of failing in a detectable way.
Worth either counting evictions (a debug log with the tracked count, or a package-visible counter the tests can assert on) or keying child state on the reply rather than the source, so an evicted entry can be re-created without changing classification semantics. And since MAX_TRACKED_CHILD_SOURCES is what makes eviction reachable at all, put the cap and its consequence in the class javadoc: the type already tells internal callers it may change without notice, but not that it silently forgets sources past N.
d76bd31 to
f453a60
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the new pushes since the previous approval. The core lifecycle/correlation work looks solid, but the opt-in AG-UI disposition layer is not yet wired end-to-end and the snapshot conversion breaks non-user multimodal round-trips — two blocking gaps before this can land.
Findings
See inline comments (2 warnings).
Automated review by github-manager-bot, re-review at f453a60
| * | ||
| * @return true to derive text output disposition events | ||
| */ | ||
| public boolean isTextOutputDispositionEnabled() { |
There was a problem hiding this comment.
This flag is not wired into the adapter flow: AguiAgentAdapter still converts the original stream, the registry never registers TextOutputDispositionConverter, and no caller invokes AguiStreamContext.observe. As a result, enabling the option emits neither the disposition CUSTOM events nor MESSAGES_SNAPSHOT (the snapshot guard always sees finalResult == null). Conditionally wrap the agent stream, register the converter, capture result events, and add enabled/disabled end-to-end adapter tests.
| .collect(Collectors.joining("\n")); | ||
| return text.isEmpty() ? null : new MessageContent.Text(text); | ||
| } | ||
| return new MessageContent.Blocks(contentParts); |
There was a problem hiding this comment.
This now emits MessageContent.Blocks for image/audio/video regardless of the message role, but toMsg rejects structured blocks unless aguiMessage.isUserMessage(). A snapshot containing an assistant media response or a media tool result cannot be sent back on the next run and throws IllegalArgumentException. Either support these role/content combinations on input or avoid emitting unsupported blocks, with assistant and tool-message round-trip tests.
修复 AG-UI disposition 配置未接入的问题，补充 assistant/tool 媒体消息转换，并让最终答案过滤器跳过子源状态跟踪。
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of 07ae18d3 after the two blocking gaps from the 10:46Z round. Both are closed in the right place: AguiAgentAdapter now applies AgentEventStreams.withTextOutputDisposition on the ReAct and harness branches behind textOutputDispositionEnabled, the registry registers TextOutputDispositionConverter only when the flag is on (so the default wire format is unchanged), context.observe(event) moved into the registry so child events reach the snapshot state, and AguiMessageConverter no longer rejects structured blocks for non-user roles — tool messages now round-trip through a ToolResultBlock, which is what the media round-trip needed. FinalAnswerFilterMiddleware also stops child ModelCallStart/End events from polluting the parent reply tracker. The new tests cover enabled/disabled on the adapter, the child bypass, and both assistant and tool round-trips.
Two things to fix before this is ready, both in AguiMessageConverter.toMsg: the new blocks path builds the ToolResultBlock through a constructor that defaults the state to RUNNING while the sibling text path sets SUCCESS, and the removed guard now lets a tool message without a toolCallId through as a TOOL-role message carrying no ToolResultBlock. Neither is architectural — they are the two edges of the new branch.
CI on this head is still running (build (ubuntu-latest) and build (windows-latest) pending), so I am leaving this as a comment. CLA is signed.
Automated review by github-manager-bot
| } | ||
| if (aguiMessage.isToolMessage() && aguiMessage.getToolCallId() != null) { | ||
| blocks.add( | ||
| new ToolResultBlock(aguiMessage.getToolCallId(), null, structuredBlocks)); |
There was a problem hiding this comment.
[Warning] This path builds the tool result through the 3-arg constructor, which delegates to this(id, name, output, null, null) — and ToolResultBlock's creator normalizes a null state to ToolResultState.RUNNING (ToolResultBlock.java:65). The sibling plain-text tool path directly below builds the same block through the builder with .state(ToolResultState.SUCCESS) (AguiMessageConverter.java:316-325). So a tool result that comes back with media blocks is recorded as RUNNING while a text-only result is SUCCESS, and determineToolResultState special-cases RUNNING (ReActAgent.java:3504-3506), so the two shapes take different paths through history rendering, persistence and any resume check.
Please build it the same way as addTextBlock:
blocks.add(
ToolResultBlock.builder()
.id(aguiMessage.getToolCallId())
.output(structuredBlocks)
.state(ToolResultState.SUCCESS)
.build());and extend testConvertToolBlocksContentPreservesToolResultId with an assertion on getState() — it currently checks the id and the output types only, which is exactly how this stays invisible.
| blocks.add( | ||
| new ToolResultBlock(aguiMessage.getToolCallId(), null, structuredBlocks)); | ||
| } else { | ||
| blocks.addAll(structuredBlocks); |
There was a problem hiding this comment.
[Warning] Removing the guard leaves a hole for role: "tool" with a null toolCallId: the condition above is isToolMessage() && getToolCallId() != null, so such a message falls through to this else and produces a TOOL-role Msg whose content is raw Text/Image/Audio blocks with no ToolResultBlock at all — the shape the comment on the plain-text branch says must never happen ("Tool results must always carry a ToolResultBlock, even when the frontend returned empty content", AguiMessageConverter.java:317-319"). Downstream correlation matches on the result id, so a client typo here yields an unresolvable pending tool call rather than a clear error, and it also weakens the invariant the previous IllegalArgumentException` was protecting for every non-user role.
Suggest keeping an explicit rejection when a tool message has no id, e.g.
} else if (aguiMessage.isToolMessage()) {
throw new IllegalArgumentException(
"AG-UI tool messages must carry toolCallId to wrap their content blocks");
} else {
blocks.addAll(structuredBlocks);
}and stating in the class javadoc which roles may carry structured blocks now that non-user roles are accepted.
| error -> Flux.concat(finishPendingEvents(context), Flux.error(error))); | ||
| } | ||
|
|
||
| private Flux<AgentEvent> withTextOutputDisposition(Flux<AgentEvent> events) { |
There was a problem hiding this comment.
[Info] The operator is applied on the ReAct branch and the harness branch, but not on the 1.x fallback below (agent.stream(...)), so textOutputDispositionEnabled(true) silently does nothing for a plain Agent. That is probably the intent, but the flag is on AguiAdapterConfig and reads as a global switch.
A sentence in the config javadoc, or a log.debug when the flag is set and the fallback branch is taken, would save someone a debugging session. Related: the 3-arg AgentEventConverterRegistry constructor still delegates with false, so a caller who builds the registry directly and applies the operator themselves gets disposition events absorbed by CustomAgentEventConverter instead of the typed converter — worth one line in that overload's javadoc.
媒体工具结果显式标记为成功，缺少 toolCallId 的工具消息直接拒绝，并补充兼容配置说明和回归测试。
|
机器人本轮复审意见已处理并推送到 PR 分支。
提交： |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the new commit 47e20b02. Keeping the tool-result toolCallId intact is the right call, but as written this turns a payload the previous tests explicitly guaranteed would still convert into a hard IllegalArgumentException on the inbound AG-UI boundary — the opposite of the opt-in posture the same commit documents. Two of the three new branches are also untested. Commenting rather than approving until that compatibility question is settled.
Findings
- [Critical]
AguiMessageConverter.java:101— a previously tolerated payload now throws at the request boundary. - [Warning]
AguiMessageConverter.java:119— every inbound tool result is stampedSUCCESS, including failures reported by the frontend. - [Warning]
AguiMessageConverterTest.java:479— the new assertion does not coverisBlank()or the no-content + missing-id path.
Suggestions
If strict rejection is intended, gate it behind the same kind of opt-in flag as AguiAdapterConfig#isTextOutputDispositionEnabled introduced in this PR, or map it to a client-visible validation error instead of letting an unchecked exception escape toMsg, and record it in the release notes. Please also add cases for toolCallId == "" / " " and for a tool message with no content and a missing id, and confirm ToolResultState resolves in the converter (no import hunk was added in the diff).
Automated review by github-manager-bot
|
|
||
| if (aguiMessage.isToolMessage() | ||
| && (aguiMessage.getToolCallId() == null || aguiMessage.getToolCallId().isBlank())) { | ||
| throw new IllegalArgumentException( |
There was a problem hiding this comment.
This turns a previously tolerated payload into a hard failure: the deleted test asserted a tool message with null toolCallId "should still convert properly" (to a text block). Existing AG-UI clients now break, and because toMsg sits on the inbound request boundary the IllegalArgumentException will likely surface as a 500 and abort the whole stream rather than skip one message. Please either (a) gate strictness behind a config flag defaulting to the old lenient behavior, mirroring the opt-in posture documented in AguiAdapterConfig#isTextOutputDispositionEnabled in this same commit, or (b) log a warning and fall back to the unwrapped blocks. If strict rejection is intended, map it to a client-side validation error (400-class) and call out the break in the release notes.
| ToolResultBlock.builder() | ||
| .id(aguiMessage.getToolCallId()) | ||
| .output(structuredBlocks) | ||
| .state(ToolResultState.SUCCESS) |
There was a problem hiding this comment.
Hardcoding ToolResultState.SUCCESS mislabels outcomes: an AG-UI tool message carrying a failed tool result is now converted into a successful one, and that state is what the model sees downstream. The prior 3-arg ToolResultBlock constructor left this unset. Either leave state null unless AguiMessage exposes an explicit status/error field to map from, or add that field and derive the state. Also confirm the new state does not leak into the outbound payload via toAguiMessage, since the round-trip test at line 582 would otherwise change the wire format for existing consumers.
| @Test | ||
| void testConvertToolMessageWithNullToolCallId() { | ||
| // Tool message without toolCallId - should still convert properly | ||
| void testConvertToolMessageWithNullToolCallIdIsRejected() { |
There was a problem hiding this comment.
This rename plus the single added state assertion does not cover the converter change. Missing: (1) the isBlank() half of the new guard - add cases for toolCallId "" and " "; (2) the second changed condition at AguiMessageConverter line 124, i.e. a tool message with no content and a missing toolCallId, which now throws where it previously fell through to addTextBlock; (3) a positive case proving the MessageContent.Text path still yields a ToolResultBlock with state SUCCESS, since the added assertion only exercises the blocks path. Without these the guard's blank handling and its interaction with the no-content branch are unverified.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review on head 47e20b02 (the branch moved by 07ae18d3 + 47e20b02 since my last pass; the rest is upstream main merges).
Great progress — most of the open threads are genuinely fixed, with tests:
isTextOutputDispositionEnabled()is no longer a dead flag: the wrapper is applied on the ReAct (AguiAgentAdapter.java:208) and harness (:220) branches andTextOutputDispositionConverteris registered (AgentEventConverterRegistry.java:100-102), with an enabled/disabled end-to-end test.- The blocks path no longer builds a
RUNNINGtool result (AguiMessageConverter.java:116-120). - The missing-
toolCallIdhole is closed with an explicit guard (:99-103) — exactly the shape requested. - The user-only restriction on structured content is gone, so media round-trips for assistant/tool messages work.
- Child events now bypass the parent reply state machine (
FinalAnswerFilterMiddleware.java:73-75), which removes the unboundedchildStatesgrowth at that call site. - The tracker-confinement answer (javadoc) is acceptable: I verified both construction sites are inside
Flux.defer, so a tracker is per-subscription andAguiStreamContext.observe()has no concurrent caller at this head. - The 1.x-fallback and 3-arg-registry caveats are documented.
What still blocks an approval (4 warning-level items, inline comments attached):
- Inbound compatibility of the strict guard.
toMsgList()is called atAguiAgentAdapter.java:152-154, i.e. before thetryat:164and outsideonErrorResumeat:182, so theINVALID_INPUT_ERRORmapping at:429-431never sees thisIllegalArgumentException. Arole:"tool"message with a missing/blanktoolCallId— previously converted and tolerated — now aborts the run with a bare error signal instead of aRUN_ERRORevent. Either move the conversion inside the guarded block (so it becomes a properRUN_ERROR), or gate/warn-and-fallback as the thread suggested. ToolResultState.SUCCESSis still hardcoded (AguiMessageConverter.java:119and:334) whileAguiMessagehas no status field, so a failed frontend tool result is reported to the model as a success. If AG-UI cannot express the failure, please say so in the javadoc and/or expose a default-state knob rather than silently mislabeling.isAuthoritativeFullEventstill widens the wire for every streaming parent, not only for consumers of this opt-in feature (RemoteEventCodec.java:261-264).- Remote child ends stay silently unclassified:
RUN_FINISHEDdecodes to anAgentEndEventwithout an invocation outcome (RemoteEventCodec.java:183), soisSuccessfulChildEnd(AgentEventStreams.java:219-222) never closes the reply for pre-PR peers — at minimum log it.
Plus three test gaps (blank-toolCallId arm of the new guard, the bypass regression test that does not discriminate the fix, and the harness branch coverage) — see the inline comments.
None of this is architectural; items 1 and 2 are the ones I would like to see settled before merge. CLA is signed, all checks are green, and the PR is mergeable/clean. Ping @oss-maintainer for another pass after the push.
Automated review by github-manager-bot
| ToolResultBlock.builder() | ||
| .id(aguiMessage.getToolCallId()) | ||
| .output(structuredBlocks) | ||
| .state(ToolResultState.SUCCESS) |
There was a problem hiding this comment.
Still hardcoded ToolResultState.SUCCESS. 47e20b02 unified the blocks path onto the builder with .state(SUCCESS) (here) and the plain-text path keeps it at :334, so an AG-UI tool message that represents a failed frontend tool result is converted into a successful one, and that state is what ReActAgent.determineToolResultState and the model see downstream. AguiMessage still exposes no status/error field to derive the state from (AguiMessage.java:44-48: id, role, content, toolCalls, toolCallId only), so "add that field and derive the state" remains the open half of the thread. (Sub-point verified: the state does not leak into the outbound wire — toAguiMessage at :152-171 reads only id + output.)
|
|
||
| if (aguiMessage.isToolMessage() | ||
| && (aguiMessage.getToolCallId() == null || aguiMessage.getToolCallId().isBlank())) { | ||
| throw new IllegalArgumentException( |
There was a problem hiding this comment.
The strict guard is kept and widened by the delta (null or blank toolCallId) with none of the compat mitigations from the thread: no config gate, no warn-and-fallback, no 400-class mapping, no release-note call-out. Traced at head: toMsgList runs at AguiAgentAdapter.java:152-154, before the try/catch at :164-170 and outside the onErrorResume at :182-189, so the IllegalArgumentException propagates as a bare stream error — the explicit IllegalArgumentException -> INVALID_INPUT_ERROR mapping at AguiAgentAdapter.java:429-431 is unreachable for inbound conversion failures and the adapter itself emits no RUN_ERROR AG-UI event (the Spring MVC starter does catch onError and send a RunError frame, AguiMvcController.java:185-190, but direct run() consumers get a raw error). One malformed message still aborts the whole run rather than skipping a message, and 47e20b02 newly rejects blank (""/whitespace) ids that previously converted. Either gate the strictness behind config (mirroring isTextOutputDispositionEnabled's opt-in posture), or surface it as a RUN_ERROR/validation error at this boundary and call out the break in the release notes.
|
|
||
| private static boolean isAuthoritativeFullEvent(String eventType) { | ||
| return AgentEventType.TEXT_OUTPUT_DISPOSITION.name().equals(eventType) | ||
| || AgentEventType.AGENT_RESULT.name().equals(eventType); |
There was a problem hiding this comment.
Unchanged by the delta: isAuthoritativeFullEvent still matches AGENT_RESULT (:261-264), so detail=full — the default for a streaming parent — now forwards AGENT_EVENT passthrough result DTOs to every existing consumer of a running subagent, not only callers that opted into textOutputDispositionEnabled. The disposition half of the change is additive and graceful for old clients, but the result half remains an ungated wire widening riding along with this feature PR. Please gate AGENT_RESULT-at-full behind the same opt-in flag, or split it out and state it in the release notes as its own wire change.
| event)); | ||
| tracker.markDispositionEmitted(sourceKey); | ||
| } | ||
| } else if (isSuccessfulChildEnd(event)) { |
There was a problem hiding this comment.
Unchanged by the delta: a child terminal still requires METADATA_INVOCATION_OUTCOME == success (isSuccessfulChildEnd, :219-222), while RemoteEventCodec.java:183 decodes the wire's RUN_FINISHED into new AgentEndEvent(null) with no metadata — so a remote child whose server predates this PR (or any path where the end arrives as a typed RUN_FINISHED rather than a JSON payload) produces a child reply that is never terminally classified, silently, with no log line. Over the wire RUN_FINISHED is the success signal, so the two halves of one protocol still disagree. Either tag that arm OUTCOME_SUCCESS (and its error/cancel counterparts with their own outcomes) so local and remote children behave the same, or document the "remote children classify only once both sides are new" intent here and in the AgentEndEvent#METADATA_INVOCATION_OUTCOME javadoc.
| new LinkedHashMap<>() { | ||
| @Override | ||
| protected boolean removeEldestEntry(Map.Entry<SourceKey, ReplyState> eldest) { | ||
| return size() > MAX_TRACKED_CHILD_SOURCES; |
There was a problem hiding this comment.
Partially addressed by the delta. The new top-level bypass (FinalAnswerFilterMiddleware.java:73-75) means child sources never enter childStates on that call site, so the worst manifestation from this thread — eviction making the filter quietly stop honouring its "final answer only" contract — can no longer happen there. Residual: AgentEventStreams.DispositionAnnotator (AgentEventStreams.java:69,103) still observes child sources and still evicts silently past 4096 (:90-96) — an evicted source's later text arrives with state == null and is silently unclassified. There is still no eviction counter/debug log and still no mention of the cap or its consequence in the class javadoc (only trackedSourceCount()'s javadoc references it).
| @Test | ||
| void testConvertToolMessageWithNullToolCallId() { | ||
| // Tool message without toolCallId - should still convert properly | ||
| void testConvertToolMessageWithNullToolCallIdIsRejected() { |
There was a problem hiding this comment.
Coverage is still incomplete for the guard this delta widened. Missing: (1) the isBlank() half — only the null case is asserted here; add cases for toolCallId "" and " ", which 47e20b02 newly rejects; (2) a tool message with no content and a missing/blank toolCallId, which now throws at the top guard (AguiMessageConverter.java:99-103) where the pre-delta code fell through to addTextBlock — that interaction is unverified. The Text-path SUCCESS positive case the thread asked for does exist at AguiMessageConverterTest.java:201-238 (testConvertToolMessageToMsg and the empty/null-content variants), and the blocks-path state assertion was added at :579.
| } | ||
|
|
||
| @Test | ||
| void childSourceEventsBypassParentReplyTracker() { |
There was a problem hiding this comment.
New test added by 07ae18d3 does not actually discriminate the fix. Replaying these events through the pre-delta code produces the same output: pre-fix, childText was buffered and then flushed by the child's own ModelCallEndEvent (finalEvents = [childText, childEnd]), and the parent's "final answer" delta still passed through once clear() wiped the top-level state — so both assertions (contains(childText), "final answer" present) hold either way. To pin the bypass, add a case where a child ToolCallStartEvent or ModelCallEndEvent arrives while the parent has unflushed buffered text: pre-fix that hits bufferedTextEvents.clear()/tracker.clear() and drops the parent's final answer; post-fix it must survive. Also worth one taskId-only (blank source) child event — SourceKey.isTopLevel() (ReplyLifecycleTracker.java:71-73) classifies it as non-top-level, but no test exercises that arm; the new test only covers source+taskId.
| Objects.requireNonNull( | ||
| invokeHarnessStreamEvents(agent, msgs, runtimeContext), | ||
| "agent stream is null"); | ||
| events = withTextOutputDisposition(events); |
There was a problem hiding this comment.
withTextOutputDisposition is now applied on the ReAct branch (:208) and this harness branch (:220), but the new end-to-end test (AguiAgentAdapterV2Test.java:102-141) drives only a mocked ReActAgent. The reflection-based harness path (invokeHarnessStreamEvents, :261-286) has no enabled/disabled coverage, so the flag's effect on the harness stream — the branch that produced the IllegalStateException mapping at :282-285 — is unverified. The 1.x fallback's no-op behaviour is now documented (AguiAdapterConfig.java:124-126), which matches the intent stated in the thread.
恢复缺少工具调用标识消息的兼容转换，支持可选工具结果状态映射。补齐远程结束事件成功标记、子任务状态淘汰提示以及 AG-UI、Harness 和父子事件回归测试。
|
针对最新一轮复审（提交 300a09e）已完成以下调整：
验证结果：
关于 RemoteEventCodec 的 detail=full：AGENT_RESULT 透传是该远程协议当前 FULL 级别的既有约定，VERBOSE 仍提供完整事件流；本次没有把它静默改成仅新开关可见，以免破坏已有远程消费者。 |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the new push (47e20b02) on top of the commits already reviewed. It does three things: media/structured tool results now carry an explicit ToolResultState.SUCCESS, AG-UI role:"tool" messages without a usable toolCallId are rejected up front, and two javadoc notes document the text-output-disposition compatibility surface. The state fix is a genuine correctness win — the previous code path defaulted to RUNNING, so a media tool result never resolved the pending call — and the added assertions in AguiMessageConverterTest cover it. The validation, however, is thrown from a point that no error mapping in this module can see, so a malformed client message now kills the SSE stream instead of producing a protocol error.
Findings
- [Warning]
AguiMessageConverter.java:99— the newIllegalArgumentExceptionis raised insideAguiAgentAdapter.run'sFlux.defersupplier, before thetry/catch → errorEvents(...)and the.onErrorResume(errorEvents)are assembled;AguiRequestProcessor'scatch (Throwable) → processorErrorEvents(...)and the WebFlux handler'sonErrorResume(handleParseError)are both assembly-time only. Clients get a bare stream error instead ofRUN_ERROR(code=INVALID_INPUT_ERROR), whichmapErrorCodealready knows how to derive fromIllegalArgumentException. - [Warning]
AguiMessageConverter.java:102—toMsgList(List<AguiMessage>)maps the full transcript, so one bad tool message aborts the whole run; previously the message degraded to plain text. This is a client-visible behaviour break and is not yet covered by any upgrade note. - [Info]
AguiMessageConverter.java:119— verified the state fix is real:new ToolResultBlock(id, null, output)routed tothis(id, name, output, null, null), and the canonical constructor coercesnull → ToolResultState.RUNNING(ToolResultBlock.java:65), so media results entered the ReAct loop as still-pending. The text path (addTextBlock) already setSUCCESS, so the two paths are now consistent.
Suggestions
- Route the validation through the existing error-event path, e.g. move
toMsgList(...)into thetryblock inAguiAgentAdapter.run(or wrap the conversion in its ownFlux.defer(...).onErrorResume(e -> errorEvents(threadId, runId, input, e, true))), and add an adapter-level test that subscribes with atoolCallId-less tool message and asserts aRunErrorevent — today's tests only assert thattoMsgthrows, so the transport contract stays unverified. - Add a whitespace-only
toolCallIdcase (theisBlank()half of the guard is untested) and a round-trip assertion for the empty-content tool path soSUCCESSis pinned there too. - Consider tolerating the id-less tool message for history entries and enforcing it only for the message that resolves the pending call; the file's own comments already treat id rewriting by proxies/older clients as expected, so a strict rejection on history can break otherwise valid resumes. If fail-fast is the deliberate choice, please note it in the AG-UI migration docs.
Automated review by github-manager-bot
Additional notes (not anchored to a changed line)
- [WARNING]
agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui/src/main/java/io/agentscope/core/agui/converter/AguiMessageConverter.java:99— [Warning] The newIllegalArgumentExceptionis thrown from a place that no error handler covers.
AguiAgentAdapter.run calls messageConverter.toMsgList(...) inside its Flux.defer supplier (around lines 151-153 at this head), i.e. before the try { streamWithRuntimeContext(...) } catch (Throwable) { return errorEvents(...) } block and before the .onErrorResume(errorEvents) on the concat are even assembled. Flux.defer turns the supplier throw into an onError signal, and AguiRequestProcessor's catch (Throwable) -> processorErrorEvents(...) only guards synchronous assembly, as does AguiWebFluxHandler.processInput (onErrorResume(this::handleParseError) is attached to the Mono that builds the response, not to the SSE body).
Net effect for a client that omits toolCallId: the SSE stream dies with a raw error and no RUN_ERROR frame, instead of the INVALID_INPUT_ERROR code that mapErrorCode already derives from IllegalArgumentException. Since this is exactly the fail-fast path the commit intends, please make the failure protocol-visible: move the toMsgList call into the existing try block in AguiAgentAdapter.run (or wrap it in a Flux.defer(...) + onErrorResume(errorEvents(...))) so an invalid tool message produces RUN_STARTED + RUN_ERROR(code=INVALID_INPUT_ERROR).
Existing tests only assert toMsg throws, so the broken transport contract is invisible to CI. An adapter-level test that subscribes to run(...) with a role:"tool" message lacking toolCallId and asserts a RunError event would pin it. (line outside diff)
| @@ -101,14 +102,21 @@ public Msg toMsg(AguiMessage aguiMessage) { | |||
| if (content instanceof MessageContent.Text text) { | |||
There was a problem hiding this comment.
[Warning] toMsgList(List<AguiMessage>) converts the whole transcript, so a single malformed tool message now aborts the entire run. Previously such a message degraded to plain text and the run proceeded.
That is the right strictness for resume-driven tool results, but it is a behaviour break for older clients and proxies that rewrite or drop ids (the same file already documents id rewriting as a real occurrence in contentMatchesAnchor-style comments and in AgentResolver history handling). Two options:
- Keep fail-fast but surface it as a protocol error (see the inline note at line 99) and mention it in the AG-UI upgrade notes, so operators can tell a protocol violation from an agent crash; or
- tolerate it for history messages (a completed tool call already persisted is harmless as text) and enforce the id only for the message that resolves the currently pending tool call.
Either is fine; what matters is documenting which one is chosen, because today the run simply dies.
| } else { | ||
| blocks.addAll(structuredBlocks); | ||
| } | ||
| } else if (aguiMessage.isToolMessage()) { |
There was a problem hiding this comment.
[Info] Verified this is a genuine bug fix, not cosmetic: the previous new ToolResultBlock(id, null, structuredBlocks) call resolved to the 3-arg constructor, which routes through this(id, name, output, null, null) and then state != null ? state : ToolResultState.RUNNING (agentscope-core/.../ToolResultBlock.java:65). So every media/structured tool result entered the ReAct loop as RUNNING, i.e. the call stayed pending downstream. Explicit SUCCESS here matches what addTextBlock already did for the text path, so the two paths are now consistent.
Nit: addTextBlock (line 326) still guards getToolCallId() != null only, while toMsg now rejects blank ids as well. Harmless given the up-front validation, but making the guard symmetric (isBlank) avoids a future caller re-introducing the blank case.
# Conflicts: # agentscope-core/src/main/java/io/agentscope/core/middleware/FinalAnswerFilterMiddleware.java
将输入消息转换纳入统一异常保护，确保非法输入通过 AG-UI RUN_ERROR 返回。对缺少或空白工具调用标识的历史消息保留普通工具内容，避免构造不可解析的工具结果，并补充适配器和转换器回归测试。
|
已同步
验证结果：相关 AG-UI 测试 |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the delta since 300a09eb. Both open items from the previous round are now closed the way that round asked for, and I verified them rather than taking the commit message at face value:
- Error path —
messageConverter.toMsgList(...)moved inside the guardedtryinAguiAgentAdapter.run, so a malformed AG-UI payload now producesRUN_STARTED+RUN_ERROR(code=INVALID_INPUT_ERROR)instead of killing the SSE stream with a raw error. The newinputConversionFailureBecomesRunErrorEventtest pins that transport contract. - Blank
toolCallIdtolerance — the up-frontIllegalArgumentExceptionis replaced by a null-or-blank guard in both the structured-blocks branch andaddTextBlock, so id-less tool history degrades to ordinary TOOL content instead of fabricating a blank-idToolResultBlock.collectToolResultIdsalready filtered blank ids, so pending-call resolution is unaffected, and both new branches (including the previously untestedisBlank()half) have converter tests.
The upstream sync (dd975e09a) touched no agui module files and removed none of the disposition/tracker lines — the merge is sound. The core disposition/lifecycle code is unchanged in this delta, and I found no correctness, compatibility, ordering or state-leak regression in it. CI is green (ubuntu, windows, license, module-sync, codecov); mergeStateStatus=BLOCKED here is branch protection, not a failing check.
One non-blocking note inline: the blank-id tolerance is currently pinned only at converter level — an adapter-level test would guard the end-to-end contract against future re-tightening.
Thanks for the thorough follow-up and for the clear responses on the review threads — this is a well-iterated contribution.
Automated review by github-manager-bot
| class RuntimeContextAndLifecycleTests { | ||
|
|
||
| @Test | ||
| void inputConversionFailureBecomesRunErrorEvent() { |
There was a problem hiding this comment.
The tolerated blank/null toolCallId path is only pinned at the converter level (AguiMessageConverterTest.testConvertToolMessageWithBlankToolCallIdRemainsBackwardCompatible); the new adapter test exercises the error route with an unsupported DocumentInputContent instead. An adapter-level test subscribing to run(...) with a role:"tool" message lacking toolCallId would pin the end-to-end contract that such history now converts to plain TOOL content and the run proceeds, guarding against a future re-tightening of the guard.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the delta since 492488d4c — the two post-review fixes (47e20b02c, 300a09eb6), the upstream sync merge (dd975e09a) and 3fbd63555. The sync merge is sound: it does not drop the disposition/tracker changes. Both items from the previous round are genuinely closed — messageConverter.toMsgList(...) now sits inside the guarded assembly block, and the blank-toolCallId up-front IllegalArgumentException is replaced by a null-or-blank guard on both branches, each with a converter test.
Findings
- [Warning]
AguiAgentAdapter.java:162— the guarded block is still incomplete:buildRuntimeContext(input, runtimeContext)runs before thetry, and for a client tool payload missingname/descriptionit reachesToolSchema'sObjects.requireNonNull, so the stream dies with a rawonErrorinstead ofRUN_STARTED+RUN_ERROR— the exact contract this commit set out to close. - [Warning]
AguiMessageConverter.java:366— an unrecognized non-blankstatussilently becomesToolResultState.SUCCESS, so a client-reported failure enters the agent loop as a success; the fallback branch is also untested. - [Info]
ReplyLifecycleTracker.java:45— the new javadoc says "least recently used child is evicted", butchildStatesis an insertion-orderedLinkedHashMapwithout the access-order flag, so eviction is FIFO by creation.
Suggestions
Findings 1 and 2 are small and contained: move the buildRuntimeContext(...) call inside the existing try (all of its consumers already are), and either log or map unknown status values to ERROR instead of defaulting to SUCCESS. For 3, either one word of javadoc (oldest-created) or new LinkedHashMap<>(16, 0.75f, true) makes the doc and the behaviour agree.
Verdict
The disposition semantics this PR introduces look correct, and the core TextOutputDisposition / ReplyLifecycleTracker logic is unchanged in this delta, so I am leaving this as a comment rather than a change request. CLA is signed. Please close the buildRuntimeContext gap before merge — it is the one finding that can still take down an SSE session.
Automated review by github-manager-bot
| AgentStream agentStream; | ||
| try { | ||
| // Convert input inside the guarded assembly block so malformed AG-UI | ||
| // payloads use the same RUN_ERROR protocol as stream setup failures. |
There was a problem hiding this comment.
The guarded block is incomplete: buildRuntimeContext(input, runtimeContext) (lines 148-149) still runs outside the try and invokes buildToolRequestConfig -> AguiToolConverter.toToolSchemaList -> ToolSchema, whose constructor does requireNonNull(name)/requireNonNull(description). A client tool payload missing name or description throws NPE at subscribe time, so the Flux errors with a raw onError and the client never gets RUN_STARTED/RUN_ERROR - contradicting this comment. Suggested fix: move the buildRuntimeContext call inside the guarded block; all its consumers are already there.
| return state; | ||
| } | ||
| } | ||
| return ToolResultState.SUCCESS; |
There was a problem hiding this comment.
Unrecognized non-blank status values (e.g. "failed", "timeout", "cancelled") are silently coerced to ToolResultState.SUCCESS, so a client-reported tool failure enters the agent loop as a success and the model may trust bad output. This fallback branch is also untested - only "error" is covered. Suggested fix: log a warning when falling back (ideally map unknown values to ERROR), and add tests for the unknown-status and getValue()-matching branches.
| * io.agentscope.core.middleware}) can share one set of reply/source correlation rules. It is not part | ||
| * of the supported API surface and may change without notice; do not use it outside this project. | ||
| * Instances are scoped to one subscription and must be accessed serially by that stream; the | ||
| * tracker is intentionally not synchronized. When the child-source cap is reached, the least |
There was a problem hiding this comment.
The new javadoc claims "the least recently used child is evicted", but childStates is an insertion-ordered LinkedHashMap and storeState only inserts new children, so eviction is FIFO by creation time: a still-streaming child created first is evicted first while newer idle children survive (the removed removeEldestEntry had the same FIFO behavior, but this doc claim is new). Suggested fix: correct the javadoc to say oldest-created, or use new LinkedHashMap<>(16, 0.75f, true) for true LRU.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the 492488d4 → 3fbd6355 delta you pushed after my last pass — the four commits address the compatibility points I raised: input conversion now runs inside the guarded assembly block (malformed AG-UI payloads surface as RUN_ERROR / INVALID_INPUT_ERROR instead of escaping the stream), blank or missing toolCallId no longer throws, RUN_FINISHED carries an explicit success outcome for remote events, and license/cla is now success. Tests cover each of those paths, including the blank-id regression cases.
Two spots still lose information rather than preserving it — the unrecognised-status fallback and the blank-id structured-content path — so I am leaving these as inline comments rather than converting to an approval. Both are small; reply or push a follow-up and I will re-review the new head.
- [Warning]
AguiMessageConverter.java:366— unknown / misspelledstatussilently becomesSUCCESS;"running"is accepted on a terminal result. - [Warning]
AguiMessageConverter.java:117— blanktoolCallIddrops theToolResultBlockand the status with it. - [Info]
ReplyLifecycleTracker.java:221— the eviction javadoc says "least recently used"; the map is insertion-ordered.
CI: mergeable: clean; checks passing on 3fbd6355 apart from the usual codecov/patch noise seen across other PRs in this window.
Automated review by github-manager-bot
| return state; | ||
| } | ||
| } | ||
| return ToolResultState.SUCCESS; |
There was a problem hiding this comment.
[Warning] Unrecognised non-blank status values fall through to ToolResultState.SUCCESS. A client that sends "failed", "cancelled" or a typo is recorded as a successful tool result, so the failure signal is silently inverted — exactly the class of bug this PR is trying to remove. Consider keeping SUCCESS as the default only for omitted/blank values and mapping an unparseable value to ERROR (or at least log.warn with the raw value) so a bad status is visible instead of optimistic.
The same applies to "running": it is accepted verbatim, so a terminal tool message can end up with RUNNING state, which downstream consumers of ToolResultBlock do not expect for a completed result. Worth either rejecting it here or documenting that a tool message may legitimately carry RUNNING.
| .state(resolveToolResultState(aguiMessage)) | ||
| .build()); | ||
| } else { | ||
| // Keep legacy history messages with missing/blank ids as ordinary TOOL content; |
There was a problem hiding this comment.
[Warning] Backward compatibility is restored for history messages without a toolCallId, but the status extension is dropped along with the ToolResultBlock on this path: a tool message with a blank id and status=error degrades into plain content blocks and the error outcome disappears. Suggest either (a) synthesising an id (e.g. "unknown-" + message id) so the result still round-trips as a ToolResultBlock, or (b) annotating the converted Msg with the lost status in metadata, so the outcome is recoverable. A short test for the blank id + non-success status combination would pin whichever behaviour you choose.
| topLevelState = state; | ||
| } else { | ||
| childStates.put(sourceKey, state); | ||
| if (childStates.size() > MAX_TRACKED_CHILD_SOURCES) { |
There was a problem hiding this comment.
[Info] Eviction itself is unchanged from the previous removeEldestEntry override, and the new debug log is a genuine improvement for diagnosing unclassified events. One nit on the javadoc added above: this LinkedHashMap is insertion-ordered (no accessOrder = true), and put on an existing key does not reorder it, so the entry that leaves is the eldest inserted source, not the least recently used one. "least recently used" will read as a promise to future maintainers — "least recently created" (or passing 16, 0.75f, true) would be accurate.
This branch has not been deployed
3 participants
AgentScope-Java Version
2.0.3-SNAPSHOT
Description
TextDeltaEventpreserves provider-level token streaming, but it does not tell a consumer whether a completed text segment belongs to an intermediate model turn that will continue with tools or to the last user-visible reply. Today, consumers have to infer that intent from neighboring tool and lifecycle events. That inference is easy to implement differently across AG-UI, Agent Protocol, Web UI, and custom consumers, especially when subagents or concurrent tasks are involved.This PR adds an opt-in semantic layer without changing the existing token stream.
Behavior comparison
The examples below show a tool-calling turn: the model first emits progress text, invokes a tool, and then emits the final answer.
Before: feature disabled (default)
The original
streamEvents()sequence is unchanged:"Checking..."and"Final answer"both arrive as ordinary text events. Nothing in the text event itself says that the first segment is intermediate or that the second is the final user-visible reply candidate.For AG-UI, the corresponding wire sequence remains the legacy one:
There is no semantic text classification event and no authoritative final-message snapshot.
After: semantic classification enabled
Consumers can opt in without replacing the underlying event stream:
All original events are preserved; the wrapper only inserts
TextOutputDispositionEventat the completed text lifecycle boundary:INTERMEDIATE: the referenced text belongs to a non-terminal model turn and can be presented as progress or commentary.TERMINAL: the last user-visible reply candidate has completed its streaming lifecycle.TERMINALis still not the authoritative answer.AgentResultEventremains the authoritative invocation result and can reconcile the preview with the final message, structured output, or an empty result. The closing order for a normally completed invocation is:AG-UI enables the same behavior with:
When enabled, the original AG-UI text stream remains intact and the adapter adds a semantic custom event:
For
MODEL_STOP,STRUCTURED_OUTPUT, andMAX_ITERATIONS, the authoritativeAgentResultEventproducesMESSAGES_SNAPSHOTimmediately beforeRUN_FINISHED. Consumers can therefore useINTERMEDIATEto style progress text, useTERMINALto mark the last provisional reply candidate, and then replace provisional streamed text with the authoritative transcript fromMESSAGES_SNAPSHOT.The managed Web flow applies the same model: text deltas are non-persistent preview updates,
INTERMEDIATEcan downgrade a preview to commentary,TERMINALcloses the preview lifecycle, andAgentEndEventcommits the message derived from the buffered authoritative result. Empty authoritative results can remove stale preview text, and late subscribers do not receive obsolete preview frames.Correlation and isolation
replyIdassociates a disposition with the text lifecycle it classifies.sourceandmetadata.taskIdisolate top-level, subagent, and concurrent invocations, including concurrent calls from the same source.Compatibility
ReActAgent#streamEvents()keeps its existing default event sequence.AgentEventJSON gains the newTEXT_OUTPUT_DISPOSITIONsubtype. Consumers that deserialize raw event JSON should use a version that knows this subtype; code that only consumes the existing typed events can ignore the additional event.MESSAGES_SNAPSHOTpreserves the submitted user turn, does not let run input override authoritative state, and merges the final result last.Verification
Verified on
7041ec45:mvn -pl agentscope-core -Dtest=ReplyLifecycleTrackerTest,AgentEventStreamsTest test: 33 tests, 0 failures.mvn -pl agentscope-extensions/agentscope-extensions-protocol/agentscope-extensions-agui -Dtest=AguiRequestProcessorTest,AguiAgentAdapterV2Test,SubagentEventConverterTest test: 142 tests, 0 failures.spotless:check;git diff --checkis clean.Covered regressions include top-level terminal retention under a subagent fan-out larger than the tracker cap, case-insensitive user-role snapshot preservation, late child events after source cleanup, tracker capacity and source reclamation, and AG-UI snapshot precedence with colliding or client-only run-input messages.
Related to #2872.
Related to #2975.
Checklist
mvn spotless:checkmvn test)