fix(serve): keep prompt-turn failure logs from degrading to [object Object] - #10732
Conversation
…bject] When a prompt turn rejects with a bare JSON-RPC error object (the shape the bridge forwards for non-Error failures), the daemon logged `prompt turn failed: [object Object]`, hiding the failure cause. Route the log through the bridge's extractErrorMessage/extractErrorCode helpers so the structured message and code survive. Evidence: a production daemon incident where a killed turn logged only `prompt turn failed: [object Object]` (see QwenLM#10710). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
|
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. The linked incident investigation (#10710) captured a production daemon log line Direction: aligned — this is daemon-log observability, and the same class of fix shows up in the reference agent's changelog too (a fix for images rendering as "[object Object]", and one surfacing error detail instead of a degraded status line). Size: not applicable — Approach: scope feels right. It reuses the bridge's existing Risk: no elevated risk signals — neither changed file matches the repo's revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 已观测到的真实问题，不是理论性加固。关联的事故排查（#10710）中，生产 daemon 日志出现了 方向： 对齐——属于 daemon 日志可观测性改进；参考 agent 的 changelog 里也有同类修复（如图片渲染成 "[object Object]" 的修复、用错误详情替代退化状态行的修复）。 规模： 不适用—— 方案： 范围合理。复用了 bridge 已有的 风险： 无升级风险信号——两个改动文件均未命中本仓库与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe approach matches what I'd have proposed independently: don't write new parsing — reuse the bridge's existing I traced every test expectation against the helper implementations in Two non-blocking notes:
No blockers found. Test evidence (from the PR's own CI via API — no PR code executed here)At review time no check on the reviewed commit has failed; the primary unit suite and the daemon-side suites are still running, so this table reflects a mid-flight snapshot: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The macOS/Windows unit suites are skipped for this fork PR, so Linux is the OS coverage — consistent with what the author marked under "Tested on". The author's reported Sandboxed verification would settle the one remaining gap: Real-scenario (tmux) testing: N/A on this unattended run — and the surface here is a daemon log line, not the TUI; a maintainer can trigger the isolated 中文说明代码审查：方案与我独立设想的一致——不新写解析逻辑，直接复用 bridge 已有的 两条非阻塞备注：一是 PR 描述称 测试证据（来自该 PR 自身的 CI，通过 API 读取；本审查未执行任何 PR 代码）：截至审查时刻，被审提交上无失败检查；主单测套件与 daemon 侧套件仍在运行，上表为中途快照。macOS/Windows 单测对该 fork PR 被跳过，覆盖操作系统为 Linux，与作者"仅在 Linux 测试"的标注一致。作者报告的"7 个用例通过"是其自述，未被独立复跑；以进行中的 沙箱验证可补上最后一个缺口： 真实场景（tmux）测试：本次无人值守运行不适用；且本改动面是 daemon 日志行而非 TUI，如需实机覆盖可由 maintainer 触发隔离的 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a small, reuse-first fix for a failure mode that cost real debuggability in a production incident; the only open items are the still-running unit suite and the end-to-end log line that unit tests can't pin. Going back to my independent proposal for this problem — reuse the bridge's existing JSON-RPC error extraction at the log site rather than writing new parsing, keep the The two reservations from the review are minor and non-blocking: the "byte-identical for Verdict: approve. Two 中文说明回到我对这个问题的独立设想——在日志点复用 bridge 已有的 JSON-RPC 错误提取而不是新写解析、保持 审查中的两点保留意见均属轻微、非阻塞："对 结论：通过。被审提交上还有两个 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): src/serve/routes/session-turn-failure.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
Test Plan（非阻断）：src/serve/routes/session-turn-failure.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Review + E2E report (head
|
| Build | Rejection | daemon log line |
|---|---|---|
| base (daemon built from #10731 head, which lacks this fix) | connection error (gateway down) | prompt turn failed: [object Object] (reproduced twice) |
| this PR head | connection error (gateway down) | prompt turn failed: [code -32603] Connection error. |
| this PR head | mock gateway HTTP 400 {"error":{"message":"upstream 429 rate limited"}} |
prompt turn failed: [code -32603] 400 upstream 429 rate limited |
Zero [object Object] occurrences in the head daemon's log across both failure modes; the structured code and provider detail survive.
Unit + mutation at head
- New suite
session-turn-failure.test.ts: 7/7 pass (Error name/message, code extraction,data.detailspreference, nesteddata.error.message, plain-object message, no-[object Object]sweep, primitive rejection). - Mutation A/B: reverse-applying only the
session.tshunk makes all 7 fail (describePromptTurnFailure is not a function) — the suite is coupled to the change, not vacuously green.
CI note
The red Test (ubuntu-latest) lane is pre-existing on main, not caused by this PR: components/ChatEditor.test.tsx:425:4: ERROR: The symbol "language" has already been declared (the duplicate declaration that #10729 fixes). All 5468 executable tests pass; only that suite's esbuild transform fails. A rebase onto a fixed main should turn it green.
A stale merge left two `language` interface members and two destructured
defaults in ChatEditor.test.tsx, breaking esbuild transform
("The symbol language has already been declared") in CI.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-conflict/jmtj0ojdzar
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-1 formatter private to the patched log line; six sibling sites still degrade to [object Object] — still standing, already reported (comment 3906856737)
- R1-2 degenerate non-Error shapes ({ code } / { message: '' }) still produce uninformative output — still standing, already reported (comment 3906856743)
- R1-3 Error branch pinned only for bare Errors; two mutants survive the 7 current tests — still standing, already reported (comment 3906856757)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and the qwen serve integration suite did not run locally.
Not explored to full depth (tool budget reached): "agent 6a": none — no checks were cut short at the tool ceiling..
Test Plan (not a blocker): src/serve/routes/session-turn-failure.test.ts — no such file or directory; Tests 7 passed — this review observed 5527, 503 passed; 14 passed — this review observed 5527, 503 passed.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and the qwen serve integration suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 6a"：none — no checks were cut short at the tool ceiling.。
Test Plan（非阻断）：src/serve/routes/session-turn-failure.test.ts — no such file or directory; Tests 7 passed — this review observed 5527, 503 passed; 14 passed — this review observed 5527, 503 passed。
— qwen3.8-max via Qwen Code /review (v0.22.3)
…ut-20260902 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Serialize structured non-Error rejections when message extraction is empty or would degrade to the default object string, while preserving Error formatting and extracted details. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The JSON.stringify fallback throws on circular rejections, and the catch then kept the extracted detail — which for a plain object is String(err), i.e. the [object Object] this function exists to eliminate. util.inspect never throws, handles circular refs and getters, and removes the try/catch, so the "never degrades" contract now holds for every object shape. Add the circular case the existing guard test omitted.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Reviewed at head 662105a7.
describePromptTurnFailureis a contained logging fix: Errors keep the[name] messageshape, JSON-RPC-shaped rejections surfacedata.details/ nested provider error /code, and plain objects fall through toinspect(never throws on circular or non-serializable values) so the incident-relevant cause can no longer degrade to[object Object].- The old call site's information loss is exactly what the new tests enumerate — the matrix covers Error, bare object, code-only, empty-message, circular, and primitive rejections.
- 3/3 threads resolved; CI has no failures (lanes still running); per the channel convention the call is on the review itself.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Review findings (self-PR, cannot self-approve):
describePromptTurnFailurefixes the #10710 symptom at the right seam: Error rejections keep '[Name] message', bare JSON-RPC error objects forwarded by the bridge get their message/code extracted via the existing acp-bridge helpers instead of degrading to '[object Object]', and data.details / nested provider error text are surfaced.- The
inspectfallback is the right choice over JSON.stringify — it never throws, so circular/non-serializable rejections stay readable instead of degrading again; it only kicks in when extractErrorMessage would render '[object Object]'. - Call site updated in the daemon-log rejection handler; structured fields (sessionId/promptId/clientId) unchanged.
- Nine unit tests cover the extraction matrix (Error name, RequestError data details, code prefix fallback, bare JSON-RPC objects, nested provider text, plain-object message).
CI on this head is still running; no blocking issues found.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Reviewed head 662105a720daa292d4e7dbff3d030ba6f2e08710, round 1.
No blocking findings. Approval blockers: none.
Checked:
extractErrorMessageandextractErrorCodeare exported from@qwen-code/acp-bridgeviabridge.ts:1797,1845→index.ts: export * from './bridge.js'.- The
inspectfallback condition (detail === '' || detail === String(err)) correctly catches both degenerate non-Error shapes: for{ code: -32603 },String(err)is'[object Object]'sodetail === String(err)fires andinspectproduces'{ code: -32603 }'; for{ message: '' },detail === ''fires andinspectproduces"{ message: '' }". Both shapes are in the "never degrades" test candidate list (line 66) and both assertions would fail if the reviewer's described output were the actual output. - All 10 tests are non-vacuous: each would fail if
describePromptTurnFailurewere removed or any branch were regressed. - No persisted format, auth/credential, or API compatibility concerns.
Existing reviews cross-check (freeze → diff):
R1-1 — CONFIRMED: The six sibling sites (create-sub-session.ts:1066, scheduled-tasks.ts:594, dispatch.ts:5802, etc.) do still degrade. The PR description explicitly scopes them out as follow-up work; that scoping is appropriate — this PR fixes the one confirmed incident log site.
R1-2 — REFUTED: The witness describePromptTurnFailure({ code: -32603 }) => "[code -32603] [object Object]" is incorrect. The inspect fallback at line 478 fires for this input (detail === String(err) is true when both equal '[object Object]'), yielding '[code -32603] { code: -32603 }'. Similarly, { message: '' } triggers detail === '' → inspect → "{ message: '' }", not an empty string. The PR's own "never degrades" test (line 66) includes both shapes and its assertions would fail under the claimed outputs — yet the PR reports 10/10 passing.
R1-3 — REFUTED: The PR contains 10 tests, not 7. The two cases the prior reviewer recommends adding already exist: 'extracts data details from Error rejections carrying JSON-RPC data' (test file line 19) and 'keeps the Error name prefix over a code prefix' (test file line 29). Both mutations the reviewer describes are already caught.
Not covered: No working tree available, so execution rungs not run locally. Windows/macOS behavior not exercised. Neither is a coverage gap for this change — the fix is pure string construction with no OS-dependent path.
Reviewed with AI assistance.
|
Released in v0.23.0. |
5 participants
What this PR does
When a prompt turn in
qwen serverejects with something that is not anErrorinstance — most notably the bare JSON-RPC error objects the bridge forwards ({ code, message, data }) — the daemon'sprompt turn failedlog line degraded toprompt turn failed: [object Object], erasing the failure cause. This PR routes that log line through the bridge's existingextractErrorMessage/extractErrorCodehelpers (the same ones thesendPrompt: forward failedpath already uses), so structured message,data.details, and error code survive into the daemon log. Error instances keep the[ErrorName] …prefix; note thatextractErrorMessageprefersdata.detailsovermessagefor Errors too, so an Error carrying JSON-RPCdata(ACP'sRequestError) now logs the detail rather than the generic message — an intentional enrichment, not an unchanged path. Rejections that cannot be serialized (circular objects) fall back toutil.inspect, which never throws.Why it's needed
During a production incident investigation (#10710, teardown half in #10162) a killed prompt turn logged only
prompt turn failed: [object Object], which made it impossible to tell from the daemon log why the turn died. The bridge already extracts these details elsewhere; the turn-failure log site simply bypassed that extraction.Reviewer Test Plan
How to verify
New unit test
packages/cli/src/serve/routes/session-turn-failure.test.tscoversdescribePromptTurnFailure:Errorinstance →[BridgeChannelClosedError] agent channel closed mid-request(unchanged shape){ code: -32603, message: 'Internal error' }→[code -32603] Internal errordata.details/ nesteddata.error.messagepreferred over the genericInternal errormessage, primitive rejections, and a guard that none of the structured shapes ever render[object Object]Also ran
npm run typecheck(clean),eslinton both touched files (clean), and the sibling suites that importsession.js(session-prompt-terminals,session-telemetry: 14 passed).Evidence (Before & After)
Production daemon log, before (v0.22.2, same code path):
After (same rejection shapes):
Tested on
Environment (optional)
Unit tests only; no runtime needed.
Risk & Scope
[ErrorName]prefix, but are not byte-identical: an Error carrying JSON-RPCdatanow rendersdata.detailsinstead ofmessage. Bare Errors (nodata) are unchanged.String(err)log sites in this file (different contexts, not proven lossy in this incident); the client-side replay question in serve: reloading a session whose turn was killed mid-flight hides already-persisted assistant messages #10710 and the channel teardown behavior in follow-up(serve): Degrade gracefully when the ACP NDJSON channel queue saturates instead of tearing down the whole channel #10162.Linked Issues
Part of the investigation in #10710 (does not close it); related to #10162.
中文说明
这个 PR 做了什么
qwen serve里 prompt 轮次失败时，如果 rejection 不是Error实例——典型是 bridge 透传的裸 JSON-RPC 错误对象（{ code, message, data }）——daemon 日志会退化成prompt turn failed: [object Object]，故障原因直接丢失。本 PR 把这条日志改走 bridge 已有的extractErrorMessage/extractErrorCode（sendPrompt: forward failed路径已经在用同一套提取逻辑），让结构化 message、data.details和错误码都能进日志。Error实例保留[错误名] …前缀；注意extractErrorMessage对 Error 同样优先取data.details，因此携带 JSON-RPCdata的 Error（ACP 的RequestError）现在会打出 detail 而非笼统 message —— 这是有意的信息增强，而非未改动的路径。无法序列化的 rejection（循环引用对象）回退到util.inspect，不会抛异常。为什么需要
在一次生产事故排查（#10710，拆 channel 的根因在 #10162）中，被杀的 prompt 轮次在 daemon 日志里只留下一行
prompt turn failed: [object Object]，无法判断轮次为什么死。bridge 里本来就有这些细节的提取函数，只是这处日志没有用上。审阅者测试计划
如何验证
新增单测
packages/cli/src/serve/routes/session-turn-failure.test.ts覆盖describePromptTurnFailure：Error实例 →[BridgeChannelClosedError] agent channel closed mid-request（原格式不变）{ code: -32603, message: 'Internal error' }→[code -32603] Internal errordata.details/ 嵌套data.error.message优先于笼统的Internal errormessage的普通对象、原始值 rejection，并断言所有结构化形态都不会渲染出[object Object]10 个用例全部通过；
npm run typecheck干净；两个改动文件eslint干净；同样导入session.js的邻近测试套件（session-prompt-terminals、session-telemetry，14 个用例）全部通过。前后证据
生产 daemon 日志，修复前（v0.22.2，同一代码路径）：
修复后（同样的 rejection 形态）：
测试环境
Linux ✅（仅单测，无需运行时）。
风险与范围
[错误名]前缀，但并非逐字节一致：携带 JSON-RPCdata的 Error 现在渲染data.details而非message；不带data的 Error 不变。String(err)日志点（上下文不同，本次事故未证明丢信息）；serve: reloading a session whose turn was killed mid-flight hides already-persisted assistant messages #10710 的客户端回放问题与 follow-up(serve): Degrade gracefully when the ACP NDJSON channel queue saturates instead of tearing down the whole channel #10162 的 channel 拆除行为。关联 Issue
#10710 排查的一部分（不关闭该 issue）；与 #10162 相关。