Repository navigation
Conversation
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Preserves the Anthropic msg_... id across streaming chunks by widening the per-subscription holder (StreamUsageState → StreamState) and carrying ChatResponse.getId() forward between events. The root cause is real and the fix is in the right place: previously messageId was a local variable reset to null on every event, so only the message_start-derived chunk carried an id and every text/tool/usage chunk fell back to a freshly generated UUID — which in turn made ReasoningContext accumulate chunks under mismatched ids. Structure, javadoc and the en/zh docs pages are all consistent, and the new AnthropicStreamingResponseIdTest covers provider id, absent/blank/whitespace id, per-subscription fallback isolation and overlapping subscriptions. CI: compile + all 6 new tests and the 221 module tests pass (BUILD SUCCESS); the build (ubuntu-latest) red X is the codecov/codecov-action@v4 upload failing with EPROTO ... ssl/tls alert handshake failure on a tokenless fork upload, and the windows-latest job was cancelled as a consequence — not caused by this change.
Two concurrency/scoping points below are worth addressing before merge, hence COMMENT rather than APPROVE (they are not correctness blockers for the common single-message_start path).
Findings
- [Warning]
AnthropicResponseParser.java:152-158— cross-event mutable state is read/written throughflatMap, which does not document per-subscription sequencing;handle(orconcatMap) expresses the intended 1:0..1 sequential mapping. - [Warning]
AnthropicResponseParser.java:332— unconditional write-back makes the id last-writer-wins; a stream with more than onemessage_start(gateway resumption /pause_turncontinuation) can have the new message tagged with the previous id. - [Info]
AnthropicResponseParser.java:142-147— rename and javadoc look good; considerprivatefields and/or scoping the id hand-off tomessage_startexplicitly.
Suggestions
Replace the flatMap + filter pair with a single handle (see inline comment) so the state carry-over is unambiguously per-subscription and sequential, and add one test with two message_start events in a single stream to lock down the message-boundary behaviour.
Notes for the author
- No CLA bot status is reported for this repo (
cla_status=unknown), so nothing to sign on our side — thanks for opening this, and for the well-scoped tests. - Please consider re-running the pipeline (
/rerunor a maintainer-approved re-run) for the codecov step; the coverage upload failure is unrelated to your change and usually clears on retry.
Automated review by github-manager-bot
| .flatMap( | ||
| event -> { | ||
| try { | ||
| return Flux.just( | ||
| parseStreamEvent(event, startTime, usageState)); | ||
| parseStreamEvent( | ||
| event, startTime, streamState)); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
[Warning] StreamState is now read and written from every event of a subscription, but the events still flow through flatMap (default concurrency 256). flatMap is not guaranteed to be sequential the way map is, so the cross-event carry-over (streamState.messageId written at L332, read at L186) relies on a happens-before edge that flatMap does not document; the same applies to the existing token counters.
Since each event maps to at most one ChatResponse (Flux.just(...) / Flux.empty()), the whole thing can be expressed with handle, which is strictly per-subscription and sequential:
return eventFlux.handle(
(event, sink) -> {
try {
ChatResponse response = parseStreamEvent(event, startTime, streamState);
if (response != null
&& (!response.getContent().isEmpty() || response.getUsage() != null)) {
sink.next(response);
}
} catch (Exception e) {
log.warn("Error parsing stream event: {}", e.getMessage());
}
});That also folds the filter(...) into one operator. If you prefer to keep flatMap, please note the per-subscription/single-thread confinement assumption in the javadoc of StreamState.
| ChatResponse response = | ||
| ChatResponse.builder().id(messageId).content(contentBlocks).usage(usage).build(); | ||
| // Retain the generated fallback as well when the provider omits the message id. | ||
| streamState.messageId = response.getId(); |
There was a problem hiding this comment.
[Warning] Writing the resolved id back on every event makes StreamState.messageId last-writer-wins across message boundaries. In a stream that carries more than one message_start (gateway stream_resumption / pause_turn continuations, or a replayed/concatenated stream), a content event emitted before the second message_start has been processed writes its old id back and the new message can end up tagged with the previous msg_... id.
Two cheap hardening options:
- keep the write here, but re-scope the state on
message_stopso a new message generation never inherits the previous id; or - only let
resolveMessageId(...)onmessage_startestablish the id, and never write back an id derived from a filtered-out event.
The new tests cover re-subscription and overlapping subscriptions nicely — a case with two message_start events in one stream would lock down this boundary as well.
|
Thanks for the careful review!
Multiple
Both pass on the original implementation as well. I didn't take option 2 (never writing back an id that came from a filtered-out event). When the provider omits the id, the fallback is generated on the Field visibility: I kept the fields as they were so the diff stays focused on the id fix. The outer class can access them either way. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
5d40df4 to
dc7d704
Compare
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
AgentScope-Java Version
2.1.0-SNAPSHOT, based on
main@960d7e46c88ed8567398361b69be6fb8c24e81c6.Description
Anthropic provides the message ID in
message_start, but the parser only kept it in a local variable for that event. Later text, tool-call, and usage chunks received generated UUIDs fromChatResponse.Builder. AsReasoningContextadopts each chunk's ID, the final assistant message lost the provider ID.Retain the ID in the existing per-subscription stream state so all chunks and the final accumulated message keep the same ID. Missing or blank IDs continue to work as in #3394: retain the builder-generated fallback within a subscription and generate a separate fallback for each new subscription.
Related to #2231; this fixes only its Anthropic streaming parser gap. The OpenTelemetry response attribute, model-call event payload, and HTTP request-id headers are outside this change. #2233 also changes the private stream state class, so that overlap may need coordination.
Scope discussion: #2231 (comment).
The patch adds six fixture-based regression cases through the public parser API and
ReasoningContext, covering provider ID propagation, missing/empty/blank IDs, repeated subscriptions, and overlapping subscriptions. All six fail on the original parser and pass with the fix. English and Chinese Anthropic documentation now describe the response ID behavior.Validation on JDK 21 (Java 17 compilation target):
mvn -T1 -pl :agentscope-extensions-model-anthropic -am test: Core 2,966 tests, 9 existing skips; Anthropic 238 tests, 1 existing skip. No failures or errors. This also runs the configured Spotless check.npm testindocs: 13 passing tests.npm run validateindocs: passed, including the 432-page/584-redirect check and Mintlify build validation.npm run broken-linksindocs: passed.git diff --check: passed.The full repository reactor and live Anthropic API calls were not run. Maven used an isolated local repository and public repository settings; no build files or dependency versions changed.
Checklist
mvn spotless:apply(limited to the changed Java files).