fix(sdk): Surface daemon JSON-RPC error details - #10571
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Re-run against Template looks good ✓ Problem: observed bug with evidence — linked issue #10570 documents the opaque Direction: aligned. The actionable reason (e.g. missing credentials) is already present in the HTTP response body; the SDK just discards it while building the display message. CHANGELOG has no direct reference to JSON-RPC detail surfacing, but this is a straightforward internal bugfix and the area is clearly relevant. Size: not applicable — Approach: scope still right. The unwrap is confined to the exact shape the issue describes (5xx + top-level Risk: no elevated risk signals — no high-risk path hits. Moving on to code review. 🔍 中文说明针对 模板完整 ✓ 问题:已观测到的 bug,且有证据 —— 关联 issue #10570 记录了不透明的 方向:对齐。可操作的原因(如缺少凭据)本就在 HTTP 响应体中,SDK 在构建展示消息时把它丢掉了。CHANGELOG 中没有 JSON-RPC detail 透出的直接条目,但这是一个直接的内部 bugfix,领域明显相关。 规模:不适用 —— 方案:范围依然合理。unwrap 限定在 issue 描述的确切形状(5xx + 顶层 风险:无升级风险信号 —— 未命中高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Re-run against Code reviewIndependent proposal, re-derived before reading the diff: the unwrap belongs in Re-verified against the base tree:
Standing observation, unchanged and non-blocking: the same top-level-only formatting still lives at the two sibling sites this PR deliberately leaves alone — the XHR workspace-upload path in TestingUnattended CI run — the evidence here is the PR's own CI on the reviewed commit, fetched through the API; no PR code was built or executed locally. Everything substantive is green on
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Sandboxed verification is already in flight on this head — it A/Bs the one claim the unit tests cannot reach: that the unwrapped detail actually surfaces during a live Web Shell model switch, not just in the mocked SDK reproduction. Its report will land in the verification thread on this PR. 中文说明针对 代码审查:读 diff 前重新独立推导的方案是——把 unwrap 放在 已在基线代码树上复核:门槛精确且防御性强( 一贯的非阻塞观察:同样的"仅取顶层 error"格式仍存在于本 PR 有意未改的两处相邻位置—— 测试:无人值守 CI 运行 —— 以下证据为被审提交自身 CI 经 API 抓取的结果,未在本地构建或执行任何 PR 代码。重新运行 CI 后, — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — same read as the last pass, reinforced: a small, precisely gated fix whose production change was already reviewed clean, and whose two follow-up pushes added nothing but boundary tests pinning every gate. Remaining nits are unchanged — the sibling XHR/SSE formatting sites are intentionally left alone, and the live Web Shell leg rests on the mocked reproduction until the in-flight sandbox verification reports. Reflection: I re-derived the approach independently and the PR still matches it — same location ( 中文说明置信度:4/5 —— 与上次结论一致且更有把握:一个小而精确、门槛严格的修复,生产代码在上轮审查中已确认干净,此后两次推送只增加了钉住每道门槛的边界测试。遗留的小瑕疵不变——相邻的 XHR/SSE 格式化位置有意未改;Web Shell 实际显示效果在沙箱验证报告落地前仍只有 mock 复现支撑。 反思:我重新独立推导了方案,PR 依然与之吻合——同样的位置( — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 3c": verifying the daemon-side error serialization for GET /acp (whether AcpHttpTransport 's connect path can receive the JSON-RPC envelope) — I stopped at the tr….
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):"agent 3c":verifying the daemon-side error serialization for GET /acp (whether AcpHttpTransport 's connect path can receive the JSON-RPC envelope) — I stopped at the tr…。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Review follow-up for
Verification: full |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": runtime confirmation that a live AcpHttpTransport/AcpWsTransport-configured DaemonClient actually receives the code-less body (verified structurally from both t…; "agent 5": running the new vitest cases to confirm they are green — vitest was intermittently absent from the shared worktree's node_modules across calls (concurrent workt…; "agent 1a": executing the new vitest cases in packages/sdk-typescript/test/unit/DaemonClient.test.ts (worktree had no node_modules; npm ci failed with an npm script error….
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): 385 passed — this review observed 1733, 481, 5173, 94 passed; 11 passed — this review observed 1733, 481, 5173, 94 passed.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 1)":runtime confirmation that a live AcpHttpTransport/AcpWsTransport-configured DaemonClient actually receives the code-less body (verified structurally from both t…;"agent 5":running the new vitest cases to confirm they are green — vitest was intermittently absent from the shared worktree's node_modules across calls (concurrent workt…;"agent 1a":executing the new vitest cases in packages/sdk-typescript/test/unit/DaemonClient.test.ts (worktree had no node_modules; npm ci failed with an npm script error…。
未审查:反向审计——评审时间预算不足,未能开始第 4 轮。
Test Plan(非阻断):385 passed — this review observed 1733, 481, 5173, 94 passed; 11 passed — this review observed 1733, 481, 5173, 94 passed。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Review round 2 summary (394c4d7)
Verification: Prettier check, SDK typecheck, SDK build, and DaemonClient.test.ts (391/391) passed. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.8-max via Qwen Code /review (v0.22.3)
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2471 passed · 0 failed · 2471 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:2471 通过 · 0 失败 · 2471 总计 抖动门:✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10571 verification — fix(sdk): Surface daemon JSON-RPC error detailsVerdict: 中文摘要
Central claim + A/B proofCentral claim: for daemon HTTP 5xx responses whose top-level Oracle:
Head arm 67/67, base arm 67/67 — 6/16 cells flip generic→detail, and every boundary cell is identical on both arms (controls run on both sides). Witnesses: Production-shape trace (mock shapes are real): the ACP SDK's dispatch catch converts any handler error with a string message into Vacuity (tests vs unfixed source)PR test file run against base source: 6 failed | 385 passed (391); the 6 failures are exactly the head-dependent tests ( Mutation matrix (guards pinned?)Each mutant = one-point edit to head source, full 391-test file run, file restored (witness
Positive controls: five mutants killed by exactly the named tests; unmutated control green. Survivors M4/M4b are not coverage gaps — the full 16-cell wire harness passes 67/67 with each guard removed (and with head unmutated), proving no observable behavior depends on them; they mirror the bridge's own (equally redundant) array handling. No combination row needed: each guard closes a distinct dimension, none defend one hazard twice. Corrections
Findings (non-blocking)
Not covered
MethodologyEnvironment: CI verify container, Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
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.
|
Released in v0.23.0. |






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 不修复该问题。