修改streamEvents的输出顺序#1758
Conversation
|
|
AgentScopeJavaBot
added
enhancement
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR modifies emitBlockEvents() in ReActAgent to close the currently-open block type (thinking, text, or tool call) before opening a new one, producing a strictly sequential event stream where Start/End pairs never overlap. The approach for text↔thinking transitions is correct. However, the tool-call handling has a latent correctness bug: removing entries from startedToolCalls after emitting ToolCallEndEvent allows the same toolId to re-pass putIfAbsent if a model ever streams interleaved tool-call chunks, producing duplicate ToolCallStartEvents. Additionally, this is a non-trivial state-machine change with no test coverage and an empty PR description.
| events.add( | ||
| new ToolCallEndEvent( | ||
| replyId, tc.getKey(), tc.getValue())); | ||
| startedToolCalls.remove(tc.getKey()); |
There was a problem hiding this comment.
[major] Bug: startedToolCalls.remove() allows re-entry of already-closed tool calls.
After emitting ToolCallEndEvent and removing the key from startedToolCalls, a later streaming chunk carrying the same toolId will pass putIfAbsent again, producing a duplicate ToolCallStartEvent for a tool call that was already started and ended.
This cannot happen with today's sequential tool-call streaming (OpenAI, Anthropic), but it is a latent correctness bug that will surface the moment any provider interleaves tool-call chunks.
Fix: Use a separate Set<String> closedToolCalls to track ended tool IDs instead of mutating startedToolCalls:
Set<String> closedToolCalls = new HashSet<>();
// ...
for (Map.Entry<String, String> tc : preStartToolCalls.entrySet()) {
if (closedToolCalls.add(tc.getKey())) {
events.add(new ToolCallEndEvent(replyId, tc.getKey(), tc.getValue()));
}
}Keep startedToolCalls intact so that putIfAbsent still correctly identifies duplicate tool IDs.
|
|
||
| String toolId = resolveToolCallId(tub, context); | ||
| String toolName = tub.getName(); | ||
| Map<String, String> preStartToolCalls = new HashMap<>(startedToolCalls); |
There was a problem hiding this comment.
[minor] Minor inefficiency: new HashMap<>(startedToolCalls) is allocated on every ToolUseBlock chunk, even when toolId is null or the tool was already registered (duplicate). Move this snapshot inside the if (toolId != null && startedToolCalls.putIfAbsent(...) == null) block to avoid unnecessary allocations for the common delta-only path.
| if (block instanceof TextBlock tb) { | ||
|
|
||
|
|
||
| if (textStarted.compareAndSet(false, true)) { |
There was a problem hiding this comment.
[minor] Missing test coverage: This PR changes the event-ordering state machine (a correctness-critical contract), but the existing StreamOrdering test suite in AgentEventStreamTest is @Disabled and no new tests are added. Please add at least the following test scenarios:
- Thinking → Text transition emits
ThinkingBlockEndbeforeTextBlockStart - Text → Thinking transition emits
TextBlockEndbeforeThinkingBlockStart - Tool call B starts after tool call A:
ToolCallEnd(A)is emitted beforeToolCallStart(B) endEventsclosure only emits end events for blocks still open (not already closed by transitions)
| List<AgentEvent> events) { | ||
|
|
||
| if (block instanceof TextBlock tb) { | ||
|
|
There was a problem hiding this comment.
[nit] Multiple consecutive blank lines (here lines 2110–2111, 2117–2118, 2160–2162) hurt readability. Run mvn spotless:apply to normalize formatting — the PR checklist item is unchecked.
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR modifies emitBlockEvents() in ReActAgent to close the currently-open block type (thinking, text, or tool call) before opening a new one, producing a strictly sequential event stream where Start/End pairs never overlap. The approach for text↔thinking transitions is correct. However, the tool-call handling has a latent correctness bug: removing entries from startedToolCalls after emitting ToolCallEndEvent allows the same toolId to re-pass putIfAbsent if a model ever streams interleaved tool-call chunks, producing duplicate ToolCallStartEvents. Additionally, this is a non-trivial state-machine change with no test coverage and an empty PR description.
| events.add( | ||
| new ToolCallEndEvent( | ||
| replyId, tc.getKey(), tc.getValue())); | ||
| startedToolCalls.remove(tc.getKey()); |
There was a problem hiding this comment.
[major] Bug: startedToolCalls.remove() allows re-entry of already-closed tool calls.
After emitting ToolCallEndEvent and removing the key from startedToolCalls, a later streaming chunk carrying the same toolId will pass putIfAbsent again, producing a duplicate ToolCallStartEvent for a tool call that was already started and ended.
This cannot happen with today's sequential tool-call streaming (OpenAI, Anthropic), but it is a latent correctness bug that will surface the moment any provider interleaves tool-call chunks.
Fix: Use a separate Set<String> closedToolCalls to track ended tool IDs instead of mutating startedToolCalls:
Set<String> closedToolCalls = new HashSet<>();
// ...
for (Map.Entry<String, String> tc : preStartToolCalls.entrySet()) {
if (closedToolCalls.add(tc.getKey())) {
events.add(new ToolCallEndEvent(replyId, tc.getKey(), tc.getValue()));
}
}Keep startedToolCalls intact so that putIfAbsent still correctly identifies duplicate tool IDs.
|
|
||
| String toolId = resolveToolCallId(tub, context); | ||
| String toolName = tub.getName(); | ||
| Map<String, String> preStartToolCalls = new HashMap<>(startedToolCalls); |
There was a problem hiding this comment.
[minor] Minor inefficiency: new HashMap<>(startedToolCalls) is allocated on every ToolUseBlock chunk, even when toolId is null or the tool was already registered (duplicate). Move this snapshot inside the if (toolId != null && startedToolCalls.putIfAbsent(...) == null) block to avoid unnecessary allocations for the common delta-only path.
| if (block instanceof TextBlock tb) { | ||
|
|
||
|
|
||
| if (textStarted.compareAndSet(false, true)) { |
There was a problem hiding this comment.
[minor] Missing test coverage: This PR changes the event-ordering state machine (a correctness-critical contract), but the existing StreamOrdering test suite in AgentEventStreamTest is @Disabled and no new tests are added. Please add at least the following test scenarios:
- Thinking → Text transition emits
ThinkingBlockEndbeforeTextBlockStart - Text → Thinking transition emits
TextBlockEndbeforeThinkingBlockStart - Tool call B starts after tool call A:
ToolCallEnd(A)is emitted beforeToolCallStart(B) endEventsclosure only emits end events for blocks still open (not already closed by transitions)
| List<AgentEvent> events) { | ||
|
|
||
| if (block instanceof TextBlock tb) { | ||
|
|
There was a problem hiding this comment.
[nit] Multiple consecutive blank lines (here lines 2110–2111, 2117–2118, 2160–2162) hurt readability. Run mvn spotless:apply to normalize formatting — the PR checklist item is unchecked.
3 participants
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.12, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Description
[Please describe the background, purpose, changes made, and how to test this PR]
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)