Repository navigation
fix(sdk): Surface daemon JSON-RPC error details - #10571
Conversation
Co-authored-by: Qwen-Coder <[email protected]>
github-actions
Bot
added
the
review/self-reported
Co-authored-by: Qwen-Coder <[email protected]>
|
Review follow-up for
Verification: full |
Co-authored-by: Qwen-Coder <[email protected]>
|
Review round 2 summary (394c4d7)
Verification: Prettier check, SDK typecheck, SDK build, and DaemonClient.test.ts (391/391) passed. |
|
@qwen-code /triage |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Tier: Standard — 2 files, SDK bug fix (+196/−3).
CI — 30 checks; 29 completed / skipped, 1 route: success. No build or integration-test job ran on the HEAD SHA. The PR description states 391 DaemonClient tests passed locally plus SDK typecheck and build; those results are not machine-verified by CI on this commit.
What I checked:
Logic correctness in failOnError:
- Array safety:
!Array.isArray(body)and!Array.isArray(data)are both guarded — neither a body array nor a data array is mistaken for a plain object. - Null safety:
data && typeof data === 'object'correctly rejectsnull(null is falsy). Confirmed by the "null data" test case. - Gate correctness:
res.status >= 500,errorBody?.['error'] === 'Internal error', andtypeof errorBody['code'] === 'number'together limit the unwrap to opaque generic 5xx JSON-RPC responses. Client errors (400), specific top-level error strings, and non-numeric codes fall through to the existing display behavior. - Precedence: string
data→data.details→data.message→ top-levelerror, each gated by.length > 0. data: undefined: unreachable by the guards (failstypeof data === 'string'anddata && typeof data === 'object'); detail stays at top-level error. No test needed.
Tests: The 14-row parametric table covers all declared boundary cases. One observation:
The test named 'non-internal numeric code' uses code: -32000 and expects expected: 'Server is draining' — i.e., it verifies that a non--32603 numeric code does trigger unwrapping. The name says "non-internal" but the outcome is "yes, unwraps". The test body is correct; the name is inverted relative to the expectation and will mislead future readers. (Suggestion — not a blocker.)
Cross-check vs. existing reviews:
- R1-1 (extend to XHR upload path): author declined — out of scope of the reproduced model-switch path. Accepted.
- R1-2 (precedence + empty-string guard tests): fixed ✓ in earlier commit.
- R1-3 (daemon-side contract pin): author declined — cross-package scope. Accepted.
- R2-1 (ACP transport path): author declined — intentional scope boundary. Accepted.
- R2-2 (null data + non-internal numeric code boundary rows): fixed ✓ in HEAD commit (394c4d7).
No blockers. All suggestions addressed or intentionally declined with clear justification.
Approving.
2 participants
What this PR does
When a daemon HTTP 5xx response uses the opaque JSON-RPC message
Internal error, the TypeScript SDK now uses the first non-empty string available fromdata.details,data.message, or stringdata. The original HTTP status and parsed response body remain unchanged, and existing top-level error fallbacks are preserved.Why it's needed
A failed Web Shell model switch can already carry the actionable provider reason in its HTTP response body, but the SDK previously discarded that reason while building
DaemonHttpError.message. Users therefore saw onlySet model failed: POST /session/:id/model: Internal error, which made configuration and provider failures indistinguishable from a daemon defect. The extraction is deliberately limited to 5xx responses with the exact generic top-level error and a numeric JSON-RPC code so specific errors, client errors, and non-JSON-RPC responses retain their existing display behavior.Issue #10564 is related but covers the separate failed-turn event path and nested
data.error.message; this PR fixes the daemon HTTP client path described by #10570.Reviewer Test Plan
How to verify
Send
setSessionModela mocked HTTP 500 response shaped as{ error: "Internal error", code: -32603, data: { details: "Missing credentials" } }and confirm the resultingDaemonHttpError.messageends inMissing credentialswhile its status remains 500 and its body remains unchanged. Repeat withdata.messageand stringdata. Then confirm that an empty or non-string detail, a specific top-level error, a non-numeric code, and an HTTP 400 response all retain the previous top-level message.Automated verification completed with the full
DaemonClient.test.tssuite (385 passed), focusedsetSessionModeltests on the latest main (11 passed), SDK type checking, SDK build, andgit diff --check.Evidence (Before & After)
Before:
POST /session/:id/model: Internal errorAfter:
POST /session/:id/model: Missing credentialsThese messages were captured by a deterministic in-memory SDK reproduction using the exact
origin/mainsource and the same mocked daemon response. A live provider configuration was not mutated for reproduction.Tested on
Environment (optional)
Node.js 22.22.3 on macOS; Qwen Code 0.22.3 Web Shell baseline; SDK tests use a mocked fetch response.
Risk & Scope
[object Object]toast, nesteddata.error.messageturn failures from bug(serve): Web Shell shows generic "Internal error" for failed turns, hiding the provider's actual error message #10564, and automatic retries are out of scope. The repository root build remains blocked in this worktree by the pre-existing local Ink type mismatch (selectableand selection frame exports); the affected SDK package build and type check pass.Linked Issues
Fixes #10570
Related: #10564 covers a different failed-turn event path and is not fixed by this PR.
中文说明
本 PR 做了什么
当 daemon HTTP 5xx 响应使用不透明的 JSON-RPC 消息
Internal error时，TypeScript SDK 现在会依次使用data.details、data.message或字符串形式data中第一个非空字符串。原始 HTTP 状态码和解析后的响应体保持不变，现有顶层错误回退行为也予以保留。为什么需要这个改动
Web Shell 模型切换失败时，HTTP 响应体中可能已经带有可操作的 provider 原因，但 SDK 之前在构造
DaemonHttpError.message时丢弃了该原因。用户因此只能看到Set model failed: POST /session/:id/model: Internal error，无法区分配置或 provider 故障与 daemon 自身缺陷。提取逻辑被有意限制为：HTTP 5xx、顶层错误严格等于通用Internal error、且 JSON-RPC code 为数值；因此具体错误、客户端错误和非 JSON-RPC 响应仍保持原有展示行为。Issue #10564 与此相关，但它处理的是另一条失败 turn 事件链路以及嵌套的
data.error.message；本 PR 修复的是 #10570 描述的 daemon HTTP 客户端链路。Reviewer 测试计划
如何验证
让
setSessionModel接收一个形如{ error: "Internal error", code: -32603, data: { details: "Missing credentials" } }的模拟 HTTP 500 响应，确认生成的DaemonHttpError.message以Missing credentials结尾，同时状态码仍为 500、响应体保持不变。再分别验证data.message和字符串形式data。随后确认空或非字符串详情、具体顶层错误、非数值 code 和 HTTP 400 响应都继续使用原有顶层消息。自动化验证已完成：完整
DaemonClient.test.ts测试套件通过 385 项；在最新 main 上聚焦的setSessionModel测试通过 11 项；SDK 类型检查、SDK 构建和git diff --check均通过。证据（改动前后）
改动前：
POST /session/:id/model: Internal error改动后：
POST /session/:id/model: Missing credentials这些消息来自确定性的内存 SDK 复现：分别加载精确的
origin/main源码和当前改动，并使用相同的模拟 daemon 响应。复现过程中没有修改真实 provider 配置。测试平台
环境（可选）
macOS 上的 Node.js 22.22.3；Qwen Code 0.22.3 Web Shell 基线；SDK 测试使用模拟 fetch 响应。
风险与范围
[object Object]toast、bug(serve): Web Shell shows generic "Internal error" for failed turns, hiding the provider's actual error message #10564 中嵌套data.error.message的 turn 失败以及自动重试均不在本 PR 范围内。该 worktree 的仓库根构建仍被既有的本地 Ink 类型不匹配阻断（缺失selectable和 selection frame exports）；受影响 SDK 包的构建和类型检查均通过。关联 Issue
修复 #10570
相关：#10564 处理另一条失败 turn 事件链路，本 PR 不修复该问题。