fix(dashscope): preserve SSE error response body#2278
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes DashScope streaming (SSE) error propagation so callers can access the original error payload (including request_id) via DashScopeHttpException#getResponseBody(), addressing #2197.
Changes:
- Preserve the raw (post-decryption) SSE event payload alongside the parsed
DashScopeResponseduring streaming. - Populate
DashScopeHttpExceptionwith the preserved response body when an SSE error event is received. - Add a regression test asserting
request_idis present in the exception response body for streaming errors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
agentscope-extensions/agentscope-extensions-model/agentscope-extensions-model-dashscope/src/main/java/io/agentscope/extensions/model/dashscope/DashScopeHttpClient.java |
Wraps parsed SSE responses with their original payload and forwards that payload into DashScopeHttpException on error. |
agentscope-extensions/agentscope-extensions-model/agentscope-extensions-model-dashscope/src/test/java/io/agentscope/extensions/model/dashscope/DashScopeHttpClientTest.java |
Extends the streaming error test to verify request_id is preserved in getResponseBody(). |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
AgentScopeJavaBot
added
bug
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This is a clean, focused bug fix that addresses issue #2197. The streaming path in DashScopeHttpClient.stream() was discarding the raw SSE error payload and passing null as the response body to DashScopeHttpException, while the synchronous execute() path already correctly propagated it. The fix introduces a lightweight ParsedStreamResponse record to carry both the raw response string and the parsed DashScopeResponse through the reactive pipeline, making the streaming error path symmetric with the sync path. The regression test properly verifies that request_id is preserved in the exception's response body. No logical defects, concurrency issues, or architectural concerns found.
| @@ -834,6 +837,8 @@ public DashScopeHttpClient build() { | |||
| } | |||
| } | |||
There was a problem hiding this comment.
[praise] Clean use of a private record to thread the raw response body alongside the parsed response through the reactive pipeline. Minimal change surface, no side channels, and makes the streaming error path symmetric with the sync execute() path at line 215. Well done.
| .equals("DashScope API error: " + errorMessage)) | ||
| .equals("DashScope API error: " + errorMessage) | ||
| && dashScopeHttpException | ||
| .getResponseBody() |
There was a problem hiding this comment.
[praise] Good regression test — asserting getResponseBody().contains("request_id") directly validates the fix for #2197 and guards against future regressions.
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This is a clean, focused bug fix that addresses issue #2197. The streaming path in DashScopeHttpClient.stream() was discarding the raw SSE error payload and passing null as the response body to DashScopeHttpException, while the synchronous execute() path already correctly propagated it. The fix introduces a lightweight ParsedStreamResponse record to carry both the raw response string and the parsed DashScopeResponse through the reactive pipeline, making the streaming error path symmetric with the sync path. The regression test properly verifies that request_id is preserved in the exception's response body. No logical defects, concurrency issues, or architectural concerns found.
| @@ -834,6 +837,8 @@ public DashScopeHttpClient build() { | |||
| } | |||
| } | |||
There was a problem hiding this comment.
[praise] Clean use of a private record to thread the raw response body alongside the parsed response through the reactive pipeline. Minimal change surface, no side channels, and makes the streaming error path symmetric with the sync execute() path at line 215. Well done.
| .equals("DashScope API error: " + errorMessage)) | ||
| .equals("DashScope API error: " + errorMessage) | ||
| && dashScopeHttpException | ||
| .getResponseBody() |
There was a problem hiding this comment.
[praise] Good regression test — asserting getResponseBody().contains("request_id") directly validates the fix for #2197 and guards against future regressions.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
This PR contains 91 lines of changes. Review in progress.
Automated review by github-manager-bot
AgentScope-Java Version
2.0.1-SNAPSHOT
Description
Fixes #2197.
DashScope SSE error events contain the model
request_id, butDashScopeHttpClient.stream()previously discarded the raw error payload and constructedDashScopeHttpExceptionwith anullresponse body.This change preserves the parsed response together with its raw SSE payload, then propagates that payload through
DashScopeHttpException#getResponseBody(). Callers can therefore retrieve the DashScoperequest_idfrom the error response body.A regression test verifies that a streaming error preserves
request_idin the exception response body.Checklist
mvn test)