Skip to content

fix(serve): surface provider error detail in turn_error messages - #10569

Merged
doudouOUC merged 2 commits into
QwenLM:mainfrom
doudouOUC:fix/serve-turn-error-provider-detail
Aug 31, 2026
Merged

fix(serve): surface provider error detail in turn_error messages#10569
doudouOUC merged 2 commits into
QwenLM:mainfrom
doudouOUC:fix/serve-turn-error-provider-detail

Conversation

@doudouOUC

Copy link
Copy Markdown
Collaborator

What this PR does

When a daemon-hosted prompt turn fails because the model provider rejected the request, the daemon's turn_error event now carries the provider's own error message (for example The engine is currently overloaded, please try again later) instead of the generic JSON-RPC Internal error. The Web Shell transcript error block, the live-state summary, refresh replay, and SDK consumers all surface the real reason with no wire schema change.

Why it's needed

Today the Web Shell shows a bare Internal error for this whole class of failures. In one observed session a turn failed four times with no visible distinction, while the daemon logs showed the true cause every time — the upstream engine returning engine_overloaded_error. From the user's seat a transient provider overload is indistinguishable from a daemon bug.

The detail was present on the wire all along and was dropped at one extraction point. When the agent throws an error whose message is itself a JSON string (a provider error body surfaced as stream content), the ACP SDK's catch-all ships JSON.parse(message) as the error data, nesting the provider text at data.error.message. The daemon's error-message extractor already read data.details and data.message but not that nested shape, so it fell back to the generic Internal error. This PR teaches the extractor the nested data.error shape (a plain string or an object's message), with the same precedence the desktop client already uses for ACP internal-error data. A design note rides along under docs/design/, and the turn_error row of the daemon event-schema doc now records the message semantics.

Reviewer Test Plan

How to verify

The new integration case reproduces the production failure shape end to end (real daemon + real qwen --acp child + a fake OpenAI server that answers with a single error_finish chunk whose content is the provider JSON error body): it asserts the session SSE turn_error carries the provider text, and that exactly one model request was made (the JSON body carries no numeric code, so no rate-limit retry fires). Unit cases cover the nested-object shape, a plain-string data.error, precedence over nested values, and the no-usable-string fallback.

cd packages/acp-bridge && npx vitest run src/bridge.test.ts -t extractErrorMessage
npm run build && npm run bundle
cd integration-tests && cross-env QWEN_SANDBOX=false npx vitest run cli/qwen-serve-streaming.test.ts -t "provider detail"

What a reviewer should confirm: on the base commit both fail with Internal error where the provider text is expected; on this branch they pass.

Evidence (Before & After)

Before (base commit): AssertionError: expected 'Internal error' to be 'The engine is currently overloaded, please try again later' — the turn_error event message was the generic JSON-RPC text.

After (this branch): the integration case passes; the daemon turn_error carries The engine is currently overloaded, please try again later. Unit run: 18 passed in the extractErrorMessage describe. Full packages/acp-bridge suite: 1684 passed.

Tested on

OS Status
🍏 macOS ⚠️ not tested
🪟 Windows ⚠️ not tested
🐧 Linux ✅ tested

Environment (optional)

Local npm run build && npm run bundle; integration run with QWEN_SANDBOX=false.

Risk & Scope

  • Main risk or tradeoff: any agent error whose JSON-RPC data carries a nested error string or error.message now surfaces that text instead of the generic message. That is the intent; every call site of the extractor is a display or log surface (enumerated in the design note), and structured fields (errorKind, code, loopType) are extracted independently and unchanged.
  • Not validated / out of scope: whether engine_overloaded_error-class failures should be retried automatically (tracked separately); the non-daemon TUI error path, which does not traverse this extractor; macOS/Windows runs left to CI.
  • Breaking changes / migration notes: none — event message text only; no schema, endpoint, or setting changes.

Linked Issues

Closes #10564

中文说明

本 PR 做了什么

当 daemon 托管的 prompt turn 因模型 provider 拒绝请求而失败时,daemon 的 turn_error 事件现在携带 provider 自己的错误消息(例如 The engine is currently overloaded, please try again later),而不是笼统的 JSON-RPC Internal error。Web Shell transcript 错误块、live-state 摘要、刷新重放和 SDK 消费方都能看到真实原因,且没有 wire schema 变更。

为什么需要

目前 Web Shell 对这一类失败只显示 Internal error。在一个真实会话中同一 turn 失败了 4 次且毫无区分度,而 daemon 日志显示每次都是上游引擎返回 engine_overloaded_error。用户无法区分"上游暂时过载"和"daemon 出 bug"。

详情其实一直在 wire 上,只是在一个提取点被丢弃了。当 agent 抛出的错误消息本身是 JSON 字符串(provider 错误体以流内容形式出现)时,ACP SDK 的兜底逻辑会把 JSON.parse(message) 放进错误的 data,provider 文本被套在 data.error.message。daemon 的错误消息提取逻辑此前读取 data.detailsdata.message,但不读这个嵌套形状,于是回退到笼统的 Internal error。本 PR 让提取逻辑认识嵌套的 data.error 形状(纯字符串或对象的 message),优先级与 desktop 客户端处理 ACP internal-error data 的既有顺序一致。设计说明随 PR 提交在 docs/design/ 下,daemon 事件 schema 文档的 turn_error 行也补充了 message 语义。

评审者测试计划

如何验证

新增集成用例端到端复现生产故障形状(真实 daemon + 真实 qwen --acp 子进程 + 返回单个 error_finish chunk(内容为 provider JSON 错误体)的 fake OpenAI server):断言会话 SSE 的 turn_error 携带 provider 文本,且只产生一次模型请求(JSON 错误体没有数字错误码,不会触发限流重试)。单元用例覆盖嵌套对象、纯字符串 data.error、顶层优先级和不可用兜底。

证据(Before & After)

修复前(基线提交):AssertionError: expected 'Internal error' to be 'The engine is currently overloaded, please try again later'。修复后(本分支):集成用例通过,turn_error 携带 provider 文案;extractErrorMessage 单测 18 个全部通过;packages/acp-bridge 全量 1684 个测试通过。

风险与范围

  • 主要风险:JSON-RPC data 中带有嵌套 error 字符串或 error.message 的 agent 错误,现在会展示该文本而非笼统消息。这正是目的;提取器的所有调用点都是展示或日志面(设计文档中已枚举),结构化字段(errorKindcodeloopType)的提取路径独立且未变。
  • 未验证/范围外:engine_overloaded_error 类失败是否应自动重试(另行跟踪);非 daemon 的 TUI 错误路径不经过此提取器;macOS/Windows 交给 CI。
  • 破坏性变更/迁移说明:无——仅事件消息文本变化,无 schema、端点或设置变更。

关联 Issue

Closes #10564

Daemon turns that failed because the model provider rejected the
request published a generic "Internal error" as the turn_error message,
hiding the upstream reason (e.g. engine_overloaded_error) from the Web
Shell. Teach the bridge's error-message extractor to read the nested
data.error shape the ACP SDK produces when the agent's error message is
itself a JSON string, with the same precedence the desktop client uses.

Fixes QwenLM#10564
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

E2E test report

Baseline (before the fix) — the new checks fail by showing the gap:

After the fix (verified on the PR branch, which is based on current main):

  • cd packages/acp-bridge && npx vitest run src/bridge.test.ts -t extractErrorMessage — 18/18 pass (6 new cases: nested data.error.message, plain-string data.error, top-level precedence, no-usable-string fallback)
  • integration-tests: qwen-serve-streaming.test.ts -t "provider detail" passes — the SSE turn_error carries the provider message, and exactly one model request was made (no rate-limit retry fires for this code-less error body)
  • fake-openai-server.test.ts — 13/13 pass, including the new errorContent same-chunk invariant
  • Full packages/acp-bridge suite — 1684 passed
  • npm run typecheck, ESLint, and Prettier on all touched files — clean

Pre-existing failures observed while validating (not caused by this PR):

  • qwen-serve-streaming.test.tschild-crash recovery (real SIGKILL) fails on this host both with and without this change (session still listed after session_died).
  • On current main, same-host external text reads / built-in text writes fail locally with expected [ …(2) ] to have a length of 1: the new memory-relevance selector (packages/core/src/memory/relevanceSelector.ts) issues a side query whose request body replays the prompt text, so marker-based request counting sees one extra request. This PR's own test gates that side query out; the two older tests may need the same treatment if CI shows the same drift.

@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Aug 30, 2026
@doudouOUC doudouOUC self-assigned this Aug 30, 2026
@doudouOUC
doudouOUC marked this pull request as ready for review August 30, 2026 17:22
@doudouOUC
doudouOUC enabled auto-merge August 30, 2026 17:23

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Test Plan (not a blocker): 18 passed — this review observed 1822 passed; 1684 passed — this review observed 1822 passed.

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

未审查:build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI and its suite did not run locally。

Test Plan(非阻断):18 passed — this review observed 1822 passed; 1684 passed — this review observed 1822 passed

— qwen3.8-max via Qwen Code /review (v0.22.3)

Comment thread docs/design/2026-08-30-turn-error-provider-detail.md Outdated
Comment thread packages/acp-bridge/src/bridge.ts
Comment thread integration-tests/cli/qwen-serve-streaming.test.ts
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

Addressed review round 1 in c9d1a8b.

Item Action
R1-1 Fixed the design audit to cover all eight bridge call sites, separate the transcript-replay helper, and narrow overbroad turn-status/SDK wording.
R1-2 Added Error-instance regressions for empty data.error and empty data.error.message; verified each test fails when its corresponding length guard is removed.
R1-3 Confirmed CI job 99287677995 ran both changed integration suites: 13 files / 154 tests passed. No code change needed.

Verification: extractErrorMessage focused tests (20 passed), acp-bridge typecheck, targeted ESLint, Prettier check, and two clean diff audits.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI failing: web-shell E2E Smoke (ubuntu-latest, Node 22.x), Integration Tests (no-AK, No Sandbox). Reviewed.

Test Plan (not a blocker): 18 passed — this review observed 1825, 1721, 481, 5178, 94 passed; 1684 passed — this review observed 1825, 1721, 481, 5178, 94 passed.

中文说明

⚠️ 已从批准降级为评论:CI failing: web-shell E2E Smoke (ubuntu-latest, Node 22.x), Integration Tests (no-AK, No Sandbox)。 已审查。

Test Plan(非阻断):18 passed — this review observed 1825, 1721, 481, 5178, 94 passed; 1684 passed — this review observed 1825, 1721, 481, 5178, 94 passed

— qwen3.8-max via Qwen Code /review (v0.22.3)

@doudouOUC

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tier: Standard — 7 files, daemon bridge bug fix (+385/−11).

CI — HEAD c9d1a8b7: ✅ Test (ubuntu-latest) · ✅ Integration Tests (no-AK, No Sandbox) · ✅ web-shell E2E Smoke · ✅ Desktop Shell (ubuntu / windows). The previous "CI failing" comment was against an earlier commit; current HEAD is green.

What I checked:

Core fix (extractJsonRpcErrorDetail / extractNestedErrorDetail in bridge.ts):

  • Precedence is preserved: data.detailsdata.messagedata.error (new) → fallback to err.message. Top-level fields continue to win over the new nested branch.
  • Empty-string guards: error.length > 0 and message.length > 0 in extractNestedErrorDetail prevent empty strings from leaking through. Confirmed by the "empty string data.error" and "empty string data.error.message" test cases that now exercise the RequestError path.
  • Null safety: error !== null check in extractNestedErrorDetail correctly rejects null. Arrays also fall through safely ((array as Record)['message'] is undefined, returning undefined → caller falls to err.message).
  • classifyTurnErrorKind: exact-matches terminated, which arrives via data.details (higher priority than the new branch). The new code cannot affect that classification. ✓
  • Eight call sites: all are display/log/event surfaces; no behavioral check on the extracted message beyond terminated. ✓

Test coverage:

  • Unit: nested error.message, string error, precedence over top-level keys, no-usable-string fallback, empty string error, empty string error.message. All branches covered.
  • Integration: fake OpenAI server emits a single error_finish chunk with a JSON provider error body; asserts the session SSE turn_error.message carries the provider text and that exactly one model request was made (no retry on code-less body).
  • fake-openai-server self-test: pins the same-chunk invariant (error_finish and delta.content on the same SSE frame).

Cross-check vs. existing reviews — all addressed:

  • R1-1 (design-doc call-site count wrong): fixed ✓ — design now lists all 8 call sites including the two model_switch_failed publishers, and correctly distinguishes the separate transcript-replay.ts local helper.
  • R1-2 (empty-string guards untested): fixed ✓ — two RequestError-based test cases added in HEAD.
  • R1-3 (integration tests outside npm workspaces): no code change needed — CI job 99287677995 ran and passed both fake-openai-server.test.ts and cli/qwen-serve-streaming.test.ts.

One Suggestion from my audit (not a blocker): extractNestedErrorDetail does not explicitly check !Array.isArray(error) the way extractJsonRpcErrorDetail checks the outer data. An array input would reach (array as Record)['message'], get undefined, and return undefined — correct behavior, but inconsistent with the surrounding pattern. Worth aligning if the function is extended.

No blockers.

Approving.

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

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: 38 passed · 0 failed · 38 total

Flakiness gate: ⚠️ consistent-fail — 1 of 1 changed test file(s) failed identically in every round — deterministic, so CI owns that signal

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:38 通过 · 0 失败 · 38 总计

抖动门:⚠️ consistent-fail — 1 of 1 changed test file(s) failed identically in every round — deterministic, so CI owns that signal

Verification report

PR #10569 — fix(serve): surface provider error detail in turn_error messages

Verdict: merge-ready — 38/38 scripted assertions passed (23 shape-ladder + 15 A/B adjudication), 0 unexpected failures. Verified head: c9d1a8b7ed4fa5a064c38c225a0551098a7798a2 (merge base tip 40889bad75).

中文摘要
  • 结论: merge-ready。38/38 脚本化断言通过,0 个意外失败。
  • A/B 结论: 中心声明( daemon turn_error 在 provider 拒绝时携带 provider 文案而非 Internal error)经端到端 A/B 证实为 load-bearing:head bundle 下集成用例绿(turn_error 携带 "The engine is currently overloaded, please try again later",且仅 1 次模型请求);仅还原 bridge.ts 关键 hunk 的对照 bundle 下同一用例 3 次尝试全红(Received: "Internal error")。单元层同样 20/20 绿 vs 18 绿 + 2 行为性红。
  • Findings: 无阻塞项。3 条非阻塞观察:空顶层 data.message/data.details 现在会落到嵌套 data.error(与提取器跳过空串的设计一致,已用 ladder 钉住);provider 文案无截断地流入展示面(设计文档已承认该取舍,20k ladder 单元测得透传);对照臂一次 30s 事件轮询超时为共享 runner 负载所致(环境性)。
  • 未覆盖范围: 逐 commit 归因(depth-2 shallow,仅 head commit 可达,验证的是聚合 diff);非 daemon TUI 错误路径;macOS/Windows;qwen-serve-streaming 文件中本 PR 未改动的其他 describe;Web Shell 客户端渲染仅静态核查。

Central claim and A/B proof

Central claim: when a daemon-hosted turn fails because the gateway reports an upstream rejection as an error_finish chunk whose delta.content is the provider's JSON error body, the SSE turn_error event's message carries the provider text instead of the generic JSON-RPC Internal error.

The claim chain was verified link-by-link from third-party and repo source before the A/B:

  1. packages/core/src/core/openaiContentGenerator/pipeline.ts:531 throws StreamContentError(delta.content) on finish_reason === 'error_finish' — the thrown message is the raw JSON body.
  2. @agentclientprotocol/sdk@0.14.1 (dist/acp.js:806), its own catch-all: RequestError.internalError(details ? JSON.parse(details) : {}) — the parsed body becomes the JSON-RPC error data, so the provider text nests at data.error.message; the client side rejects with the plain {code, message, data} object (dist/acp.js #handleResponse).
  3. The bridge's broadcastTurnError feeds that object to extractErrorMessage; the published turn_error event, entry.turnError (live-state summary), and entry.turnErrorEvent (refresh replay, bridge.ts:6956-6973) all carry the one extracted string — a single extraction point feeds every surface the PR names.
Cell Build under test Oracle (scripted) Result
U-head head src/bridge.ts vitest -t extractErrorMessage (20 tests) 20/20 green (01-unit-head.log)
U-control bridge.ts reverted to HEAD^1 byte-identical, tests at head same 18 green / 2 red, both behavioral: expected 'Internal error' to be 'The engine is currently overloaded, p…' and …to be 'plain string detail' (02-unit-ab-control-red.png, 02-unit-control.log)
I-head head bundle (dist/cli.js, chunked esbuild) real daemon + real qwen --acp child + fake OpenAI error_finish gateway; asserts turn_error.data.message and exactly 1 model request pass in 34 s (03-integ-head.log)
I-control same tree, hunk reverted + build -w @qwen-code/acp-bridge + rebundle (dist verified 0 occurrences of the new fn) same red on all 3 attempts: 2× Received: "Internal error" (04-integ-ab-control-red.png, 04-integ-control.log), 1× 30 s event-poll timeout under runner load
I-head-restored hunk restored (sha256-verified), rebuilt + rebundled same pass again — round-trip rules out environment drift (03-integ-ab-head-green.png)

The control design reverts the hunk in place rather than in a base worktree, so every node_modules/@qwen-code/* symlink resolves consistently into the mutated tree (realpaths asserted; a base worktree sharing root node_modules would have silently loaded head acp-bridge). The bundle reads acp-bridge from dist/, so each arm's bundle was rebuilt and grepped for extractNestedErrorDetail (head: 2, control: 0) before running.

Reviewer Test Plan walkthrough: step 1 (unit command) — head green, base red, as the plan predicts; step 2 (build+bundle) — executed for both arms; step 3 (integration command) — head green, base red with the plan's exact AssertionError. The plan's "what a reviewer should confirm" sentence holds on both layers.

Mutation / vacuity matrix

Mutation Suite Result Classification
Revert nested-extraction hunk (the fix) unit extractErrorMessage describe 2 red with expected-vs-actual message mismatches; integration case red end-to-end load-bearing, pinned at both layers
(survivors of that revert) 5 precedence/fallback tests same green on base pin unchanged base behavior by design (data.details > data.message > nested; empty strings skipped) — completeness reporting, not fix coverage
Positive control same the 2 reds themselves go red only when the fix is absent harness proven live

Single mechanism, so no combination row applies. The 7 new tests are not vacuous: the revert fails the intended assertions with the behavioral mismatch the tests exist to catch (not an import/compile break).

Shape ladder (compiled dist, wired to the real SDK)

harness-shape-ladder.mjs imports extractErrorMessage from packages/acp-bridge/dist/bridge.js and feeds it the real SDK's own constructionRequestError.internalError(JSON.parse(body)).toResult().error from the installed @agentclientprotocol/sdk — plus a boundary ladder: string data.error, precedence cells, empty-top fall-through, data.error as number/boolean/array/null/{}/{message:''}/{message:5}, astral unicode byte-identical, 20 k message untruncated, Error instance with .data, data as string/null/array, and structured-field independence (errorKind extraction unchanged). 23/23 pass (01-shape-ladder-dist.png).

Sibling sweep: the extractor's 8 call sites match the design note's enumeration exactly (turn_error broadcast, 2 quarantine logs, 2 model_switch_failed publishers, sendPrompt-forward log, pending-prompt cancel log); transcript-replay.ts's same-named helper is a separate local function for tool-call results, untouched. No consumer pattern-matches the message text except classifyTurnErrorKind ('terminated' only) and the Web Shell's identical literal fallback; retryability keys on the structured errorKind field, which this change does not alter. No CLI-side importers of the bridge extractor exist.

Findings

No blocking findings. Non-blocking observations, for the record:

  1. Empty top-level detail now falls through to the nested shape. {data: {message: '', error: {message: 'x'}}} yields 'x' at head where base yielded 'Internal error'. This is consistent with the extractor's existing empty-string-skipping for details/message and with the stated precedence; pinned by a ladder cell. Informational.
  2. Provider text flows unbounded and unescaped-by-design into display surfaces (event message, live-state summary, replay, stderr logs). The design note acknowledges this pre-existing tradeoff ("a pathological provider blob flows today; this change keeps it"); the 20 k ladder cell measures the pass-through. Web Shell renders via React text nodes; no new injection surface was introduced. Informational.
  3. One control-arm attempt timed out the 30 s expect.poll for the event itself (attempts 1 and 3 failed the message assertion). The event is published on both arms, so this is runner-load latency, not a code path; noted for transparency.

Not covered

  • Per-commit attribution: the depth-2 checkout reaches only the head commit (git rev-list HEAD^1..HEAD^2 returns 1 vs 2 commits in the metadata snapshot). The aggregate HEAD^1..HEAD diff was verified; the two commits were not exercised separately.
  • Other describes in qwen-serve-streaming.test.ts (unchanged by this PR) were not re-run; the changed fake-server harness (errorContent) was covered by its own unit file (13/13, including the same-chunk error_finish/delta.content assertion).
  • Full-suite gate noise, attributed: serial packages/acp-bridge run = 1823 passed, 2 red, both Test timed out in 5000ms in load-sensitive tests (real-process-tree reap; ring-promotion sweep); both pass in isolated head re-run (907 ms / 2.7 s), and a concurrent run produced a different set of 4 timeout names — the varying-name signature of starvation on this shared runner, not a regression. The tinypool Failed to terminate worker teardown crash that followed both full-suite runs is the same environmental class (worker kill over its timeout under load); the final summary printed before it.
  • One capture attempt hit the harness's own 10 s daemon-boot budget ("daemon boot timeout", the harness's documented slow-CI flake) on a head arm that had passed before and passed on re-run; that PNG was superseded by 03-integ-ab-head-green.png.
  • Non-daemon TUI error path, macOS/Windows, and Web Shell client rendering beyond static inspection (display via block.text, gates on errorKind) — all outside the PR's changed surface.

Methodology

CI merge-ref checkout (HEAD = merge 581fcb8180, base tip HEAD^1 = 40889bad75, head HEAD^2 = c9d1a8b7ed), node v22.23.2, npm ci + build pre-provided; control arms rebuilt only @qwen-code/acp-bridge and rebundled via npm run bundle. Harnesses are mock-free: the integration case drives a real daemon spawning a real qwen --acp child against a loopback fake OpenAI gateway emitting the production error_finish wire shape; the ladder imports the compiled dist and the installed ACP SDK. Raw logs in logs/, harnesses in harness-*.mjs, terminal captures in evidence/ (01-shape-ladder-dist.png, 02-unit-ab-control-red.png, 03-integ-ab-head-green.png, 04-integ-ab-control-red.png, 05-adjudication-matrix.png). Assertion counts (38 = 23 ladder + 15 adjudication) map one-to-one to scripted checks in those two harnesses; expected control-arm reds are encoded as passing assertions.

Flakiness gate log

integration test, out of gate scope: integration-tests/cli/qwen-serve-streaming.test.ts
integration test, out of gate scope: integration-tests/fake-openai-server.test.ts
rounds=5 files=1 skipped=2
file packages/acp-bridge/src/bridge.test.ts: (cd packages/acp-bridge) npx --no-install vitest run ./src/bridge.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/acp-bridge/src/bridge.test.ts: FFFFF

verdict: consistent-fail
summary: 1 of 1 changed test file(s) failed identically in every round — deterministic, so CI owns that signal

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/acp-bridge/src/bridge.test.ts: F (exit 1)
--- output tail · round 1 · packages/acp-bridge/src/bridge.test.ts ---
age queue (enqueueMidTurnMessage)�[2m > �[22mkeeps a resolvable sibling when one reference dies between admission and dispatch�[32m 232�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mdegrades media removed between admission and dispatch in place, keeping FIFO order and one terminal�[32m 264�[2mms�[22m�[39m
   �[33m�[2m✓�[22m�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mdegrades in place on a fully detached session instead of racing the deferred close �[33m 726�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mpromotes every undrained message at settle�[32m 116�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mpromotes messages after their client detaches without reapplying the cap�[32m 158�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mremoves a promoted message through the shared mid-turn API�[32m 123�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mdoes not remove an ordinary pending prompt through the mid-turn API�[32m 238�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mkeeps the queue across a back-to-back prompt FIFO, clearing only at true idle�[32m 110�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mswitches session-update attribution between queued turns and clears it at idle�[32m 169�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mrejects a non-member client id (mirrors /prompt and /btw authorization)�[32m 11�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mpublishes a session-wide injection frame�[32m 67�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mrejects past MAX_MID_TURN_QUEUE_DEPTH (20) — the DoS bound�[32m 12�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22muses a caller message id idempotently�[32m 12�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mtrims the message before queuing (drain returns the trimmed text)�[32m 15�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mgetMidTurnMessages returns every shared message�[32m 28�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mgetMidTurnMessages returns attachment blocks so a refresh keeps them�[32m 18�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mrejects queued inline attachments past the session byte budget�[32m 237�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mgetPendingPrompts returns attachment blocks for refresh�[32m 33�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mgetMidTurnMessages moves drained stable ids into the settled ring�[32m 150�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mgetMidTurnMessages throws for unknown sessions and unbound client ids�[32m 2�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22manswers a retry from the promoted ring after the promoted prompt completes�[32m 258�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22manswers an image-only retry from the promoted ring without running it twice�[32m 129�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mpromotes multiple undrained messages at settle in FIFO order�[32m 252�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mbounds the settled ring across many removals�[32m 269�[2mms�[22m�[39m
   �[33m�[2m✓�[22m�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mbounds the promoted ring across many idle promotions �[33m 2333�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mrejects mid-turn admission while a conditional close is being confirmed�[32m 53�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mpublishes pending_prompt_started for a promoted message that starts immediately�[32m 103�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mrejects a queueOnly enqueue on an idle session instead of promoting�[32m 60�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mrejects a public enqueue on idle only when rejectIfIdle is set�[32m 5�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mstill queues a queueOnly enqueue while the session is busy�[32m 109�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mkeeps anonymous enqueues off the shared queue surface�[32m 70�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — child-resource refresh�[2m > �[22msingle-flights the refresh so a slow child cannot pile up concurrent polls�[32m 107�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — child-resource refresh�[2m > �[22mages the snapshot, and drops it entirely once past the staleness window�[32m 7�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — child-resource refresh�[2m > �[22mcaches a well-formed child heap report�[32m 6�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — child-resource refresh�[2m > �[22mleaves heap absent rather than zeroed when the child reports none�[32m 10�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — child-resource refresh�[2m > �[22mrejects a malformed heap report whole, keeping the last good one�[32m 6�[2mms�[22m�[39m

�[31m⎯⎯⎯⎯⎯⎯⎯�[39m�[1m�[41m Failed Tests 1 �[49m�[22m�[31m⎯⎯⎯⎯⎯⎯⎯�[39m

�[41m�[1m FAIL �[22m�[49m src/bridge.test.ts�[2m > �[22mcreateAcpSessionBridge�[2m > �[22msendPrompt�[2m > �[22mkeeps attachment references on the event bus and resolves bytes for ACP
�[31m�[1mError�[22m: Test timed out in 5000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".�[39m
�[36m �[2m❯�[22m src/bridge.test.ts:�[2m14748:5�[22m�[39m
    �[90m14746| �[39m    });
    �[90m14747| �[39m
    �[90m14748| �[39m    it('keeps attachment references on the event bus and resolves byte…
    �[90m   | �[39m    �[31m^�[39m
    �[90m14749| �[39m      const prompts: PromptRequest[] = [];
    �[90m14750| �[39m      const factory: ChannelFactory = async () =>

�[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯�[22m�[39m


�[2m Test Files �[22m �[1m�[31m1 failed�[39m�[22m�[90m (1)�[39m
�[2m      Tests �[22m �[1m�[31m1 failed�[39m�[22m�[2m | �[22m�[1m�[32m833 passed�[39m�[22m�[90m (834)�[39m
�[2m   Start at �[22m 04:05:26
�[2m   Duration �[22m 137.53s�[2m (transform 49.30s, setup 0ms, collect 63.51s, tests 49.08s, environment 0ms, prepare 4.10s)�[22m


round 2 · packages/acp-bridge/src/bridge.test.ts: F (exit 1)
--- output tail · round 2 · packages/acp-bridge/src/bridge.test.ts ---
 message queue (enqueueMidTurnMessage)�[2m > �[22mpromotes messages after their client detaches without reapplying the cap�[32m 174�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mremoves a promoted message through the shared mid-turn API�[32m 142�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mdoes not remove an ordinary pending prompt through the mid-turn API�[32m 106�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mkeeps the queue across a back-to-back prompt FIFO, clearing only at true idle�[32m 29�[2mms�[22m�[39m
   �[32m✓�[39m createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessage)�[2m > �[22mswitches session-update attribution between queued turns and clears it at idle�[32m 105�[2mms�[22m�[39m
   �[32m✓�[39m createA

...truncated -- full content in the run artifacts.

Evidence images

01-shape-ladder-dist

02-unit-ab-control-red

03-integ-ab-head-green

04-integ-ab-control-red

05-adjudication-matrix

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed bug, well evidenced. Linked issue #10564 reports four real turn failures in one session all displayed as bare Internal error while daemon logs showed the upstream engine_overloaded_error each time; the issue triage independently confirmed the root cause (ACP SDK -32603 catch-all nesting the provider text at data.error.message, which the bridge extractor doesn't read). Self-reported, but the reproduction is concrete and the review/self-reported label already tracks that.

Direction: clearly aligned — this is error-message fidelity on the daemon path, and the merged sibling #10571 just did the same for the SDK surface. Claude Code's changelog shows steady investment in exactly this area (e.g. client-generated error messages rendering with their real detail instead of generic text), though no direct turn_error equivalent — the area is relevant regardless.

Size: small and appropriately scoped — 15 production lines in packages/acp-bridge/src/bridge.ts (+14/-1), 220 test lines (unit + integration + fake-server self-test), 161 doc lines (design note + event-schema row). bridge.ts is daemon infrastructure rather than an enumerated core module path, so the two-tier gate doesn't formally trigger; the blast radius is covered by the call-site enumeration anyway.

Approach: right-sized. The alternative (a new additive turn_error.data.detail wire field through four packages) is rejected in the design note for good reason — same user-visible result at a fraction of the churn. The precedence choice (existing data.details / data.message win over the new nested data.error fallback) preserves current behavior for every shape handled today. One observation from reading the call sites: broadcastTurnError feeds the extracted message into classifyTurnErrorKind as a fallback, but that classifier exact-matches the literal terminated (which arrives via the plain-text data.details shape), so enriching the nested shape cannot shift a classification. Every change in the diff serves the stated goal — no drive-bys.

Risk: no elevated risk signals — no high-risk paths matched (checked against the revert-correlated file list), and all eight extractErrorMessage call sites are display, log, or SSE-event surfaces.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:已观测到的 bug,证据充分。关联 issue #10564 记录了同一会话中 4 次真实的 turn 失败,UI 全部显示笼统的 Internal error,而 daemon 日志每次都显示上游 engine_overloaded_error;issue 分诊已独立确认根因(ACP SDK -32603 兜底把 provider 文本嵌套在 data.error.message,bridge 提取器不读这个形状)。虽为自我报告,但复现具体,且已有 review/self-reported 标签跟踪。

方向:明确对齐——这是 daemon 路径上的错误消息保真问题,刚合并的姊妹 PR #10571 已在 SDK 面做了同样的事。Claude Code 的 changelog 在同一领域持续投入(如客户端生成的错误消息展示真实详情而非笼统文本),虽然没有 turn_error 的直接对应项,但该领域本身是相关的。

规模:小而聚焦——packages/acp-bridge/src/bridge.ts 生产代码 15 行(+14/-1),测试 220 行(单测 + 集成 + fake-server 自测),文档 161 行(设计说明 + 事件 schema 表格行)。bridge.ts 属于 daemon 基础设施而非枚举的核心模块路径,两级门禁不正式触发;调用点枚举已覆盖影响面。

方案:规模合理。替代方案(新增 turn_error.data.detail wire 字段、贯穿四个包)在设计文档中被合理否决——用户可见效果相同,改动量却是一小部分。优先级选择(现有 data.details / data.message 优先于新增的嵌套 data.error 兜底)保持了现有形状的行为不变。读调用点时发现一个细节:broadcastTurnError 会把提取出的消息喂给 classifyTurnErrorKind 作为兜底,但该分类器只精确匹配字面量 terminated(经由纯文本 data.details 形状到达),因此丰富嵌套形状不会改变分类结果。diff 中每处改动都服务于既定目标,无夹带。

风险:无升级风险信号——未命中高风险路径(对照 revert 相关文件列表检查),且 extractErrorMessage 全部 8 个调用点均为展示、日志或 SSE 事件面。

进入代码审查 🔍

Qwen Code · qwen3.8-max

Reviewed at c9d1a8b7ed4fa5a064c38c225a0551098a7798a2 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

Reviewed against my own baseline proposal (extend the bridge's JSON-RPC detail extractor with the nested data.error shape the ACP SDK produces, keep existing precedence, add unit + end-to-end coverage): the PR matches it and goes further — a design note tracing the error through all five layers, an event-schema doc update, and a real end-to-end regression case built on new fake-server infrastructure.

The production change is the minimal one. extractJsonRpcErrorDetail gains a final fallback — extractNestedErrorDetail(data.error) — reached only after the existing string-data / data.details / data.message checks fail, so every shape handled today behaves identically; the new branch accepts a plain string or an object with a string message, rejecting empties in the same length > 0 style as the existing checks. I checked the two behavioral edges a reviewer should worry about:

  • Classification: broadcastTurnError feeds the extracted message into classifyTurnErrorKind as a fallback, but that function exact-matches the literal terminated, which arrives via the plain-text data.details shape — the nested shape cannot shift a classification. Verified in code, and the design note reaches the same conclusion.
  • Call sites: all eight extractErrorMessage call sites in bridge.ts are display, log, or SSE-event surfaces (turn_error broadcast + refresh replay, two model_switch_failed publishers, five daemon-stderr log lines). The same-named local helper in transcript-replay.ts is a different function and untouched.

Test coverage is thorough: seven new unit cases (nested object, plain string, both precedence directions, three fallback edges), a fake-server self-test pinning the same-chunk invariant (error body and error_finish reason must ride one chunk, matching real gateways), and the end-to-end case asserting the exact provider text on the SSE turn_error plus exactly-one model request (no numeric code in the JSON body → no rate-limit retry). The integration test's exact-equality assertion is what makes it load-bearing: on the base commit it would receive Internal error and fail.

No blockers, no convention violations. The two judgment calls the author flagged — no new length bound (parity with the already-unbounded data.details path; render-side sanitization and frame byte accounting still apply) and leaving the turn-status polling path (normalizeTurnResultError) for a follow-up — are both documented scope decisions I agree with.

Testing evidence — the PR's own CI (unattended run; PR code not executed here)

All pull_request-event workflow runs on the reviewed commit completed green (Qwen Code CI, Security Checks, SDK Java). Highlights:

  • Test (ubuntu-latest, Node 22.x) ✅ — full unit suite across workspaces, includes the seven new extractErrorMessage cases in packages/acp-bridge.
  • Integration Tests (no-AK, No Sandbox) ✅ — this gate's file set explicitly includes cli/qwen-serve-streaming.test.ts and fake-openai-server.test.ts (verified in the root test:integration:no-ak:sandbox:none script), so the new end-to-end regression case and the fake-server self-test ran and passed on the PR head — real daemon + real qwen --acp child + fake OpenAI server answering with the production failure shape.
  • Real daemon E2E / Java 11 ✅ and web-shell E2E Smoke ✅ — the daemon surfaces this PR touches stayed green.
  • Skips are config, not gaps: macOS/Windows unit lanes are nightly-only by design (ci.yml test_macos/test_windows triggers), and Integration Tests (CLI, No Sandbox) is merge_group-only.

Not verified: the before/after claim that the new integration case fails on the base commit is the author's reported result (their pasted AssertionError: expected 'Internal error' to be 'The engine is currently overloaded...'); CI proves the branch green, not the base red. Statically it holds — the assertion is exact-equality against a string only the new extraction path can produce — but it was not independently re-run here. Author tested on Linux only; macOS/Windows rides on the nightly lanes.

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
Test (macos-latest, Node 22.x) ⏭️ skipped (nightly-only lane)
Test (windows-latest, Node 22.x) ⏭️ skipped (nightly-only lane)
Integration Tests (no-AK, No Sandbox) ✅ success
Integration Tests (CLI, No Sandbox) ⏭️ skipped (merge_group-only)
Real daemon E2E / Java 11 ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
Desktop Shell (ubuntu-22.04 / windows-2022) ✅ success
SDK Java (ubuntu Java 11/17/21, macOS/Windows Java 21) ✅ success
Classify PR / Dependency CVE audit / Secret scan (TruffleHog) ✅ success

Sandboxed verification would settle the one open item: @qwen-code /verify — that the new integration case actually fails on the base commit (its load-bearing-ness) is currently the author's word plus a static read. The author lacks write access, so this is a sponsored run: a maintainer's @qwen-code /verify comment approves the head it was written against; that run carries a pre-execution risk screen and a full workspace wipe, but read its report with the same skepticism as the fork's own CI logs.

中文说明

代码审查

以我自己的基线方案(扩展 bridge 的 JSON-RPC 详情提取器以识别 ACP SDK 产生的嵌套 data.error 形状、保持现有优先级、补充单测与端到端覆盖)为参照:本 PR 与之一致且做得更多——设计文档追溯了错误穿越的全部五层、更新了事件 schema 文档,并基于新的 fake-server 基础设施构建了真实的端到端回归用例。

生产改动是最小改动。extractJsonRpcErrorDetail 只新增了一个兜底——extractNestedErrorDetail(data.error)——仅在现有的字符串 data / data.details / data.message 检查全部失败后才到达,因此今天已处理的所有形状行为完全不变;新分支接受纯字符串或带字符串 message 的对象,并以与现有检查一致的 length > 0 风格拒绝空值。我检查了评审者应担心的两个行为边界:

  • 分类broadcastTurnError 会把提取出的消息作为兜底喂给 classifyTurnErrorKind,但该函数只精确匹配字面量 terminated,且该错误经纯文本 data.details 形状到达——嵌套形状不会改变分类。已在代码中验证,设计文档也得出了相同结论。
  • 调用点bridge.tsextractErrorMessage 的全部 8 个调用点均为展示、日志或 SSE 事件面(turn_error 广播 + 刷新重放、两个 model_switch_failed 发布点、五条 daemon stderr 日志)。transcript-replay.ts 中同名的本地辅助函数是另一个函数,未受影响。

测试覆盖充分:7 个新单测用例(嵌套对象、纯字符串、两个优先级方向、三个兜底边界)、一个 fake-server 自测(固定"错误体与 error_finish 必须同 chunk"的不变量,与真实网关一致),以及端到端用例(断言 SSE turn_error 携带确切的 provider 文本、且恰好一次模型请求——JSON 错误体无数字码,不触发限流重试)。集成用例的精确相等断言正是其"承重"所在:在基线提交上它会收到 Internal error 而失败。

无阻塞项,无规范违规。作者标明的两个取舍——不设新的长度上限(与本就无上限的 data.details 路径保持一致;渲染侧消毒与帧字节核算仍然生效)、turn-status 轮询路径(normalizeTurnResultError)留作后续——都是文档化的范围决策,我认同。

测试证据——来自 PR 自身 CI(无人值守运行;此处未执行 PR 代码)

审查提交上所有 pull_request 事件的工作流运行全部绿色完成(Qwen Code CI、Security Checks、SDK Java)。要点:

  • Test (ubuntu-latest, Node 22.x) ✅——全工作区单测套件,包含 packages/acp-bridge 中新增的 7 个 extractErrorMessage 用例。
  • Integration Tests (no-AK, No Sandbox) ✅——该门禁的文件集明确包含 cli/qwen-serve-streaming.test.tsfake-openai-server.test.ts(已在根 test:integration:no-ak:sandbox:none 脚本中核实),因此新的端到端回归用例与 fake-server 自测确实在 PR 头上运行并通过——真实 daemon + 真实 qwen --acp 子进程 + 以生产故障形状应答的 fake OpenAI server。
  • Real daemon E2E / Java 11 ✅ 与 web-shell E2E Smoke ✅——本 PR 触及的 daemon 面保持绿色。
  • 跳过项是配置使然,不是缺口:macOS/Windows 单测lane按设计仅夜间运行(ci.yml test_macos/test_windows 触发条件),Integration Tests (CLI, No Sandbox) 仅在 merge_group 运行。

未验证:新集成用例在基线提交上失败这一 before/after 说法是作者的自述结果(其粘贴的 AssertionError: expected 'Internal error' to be 'The engine is currently overloaded...');CI 证明分支为绿,未证明基线为红。静态上成立——断言是对只有新提取路径才能产生的字符串的精确相等——但此处未独立重跑。作者仅在 Linux 上测试;macOS/Windows 依赖夜间lane。

CI 结果表见上方英文部分的机器可读区域。

沙箱验证可以了结唯一悬而未决的问题:@qwen-code /verify——新集成用例在基线提交上是否真的失败(其承重性)目前只有作者的说法加静态推断。作者无写权限,因此这是一次赞助运行:maintainer 的 @qwen-code /verify 评论即批准其撰写时所对的提交;该运行带有执行前风险筛查与完整工作区清理,但请像对待 fork 自身 CI 日志一样审慎阅读其报告。

Qwen Code · qwen3.8-max

Reviewed at c9d1a8b7ed4fa5a064c38c225a0551098a7798a2 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean across every stage; the only reservation is that the base-red half of the before/after rests on the author's pasted run, which I could only corroborate statically.

Stepping back: this is what a well-formed fix looks like. The problem was observed in production, confirmed by independent issue triage, and the root cause is a single missing branch in one extractor — which is exactly what the PR changes, in 15 production lines. My own baseline proposal for this problem was substantively identical; I looked for the cheaper path and didn't find one (the additive-wire-field alternative costs four packages of churn for the same user-visible text, and the design note says so explicitly).

The parts that earned my confidence:

  • Every call site of extractErrorMessage was enumerated in the design note and matches what I found independently — all display, log, or event surfaces. The one behavioral consumer that touches the extracted text (classifyTurnErrorKind) exact-matches terminated, which this change cannot reach. Precedence is preserved, so no shape handled today changes behavior.
  • The regression test is load-bearing by construction: exact-equality on a string only the new branch can produce, run end-to-end (real daemon, real ACP child, fake provider emitting the production failure shape), and it ran green in the no-AK integration gate that covers both changed test files.
  • The design note is the kind of artifact a maintainer hopes for — five layers traced, alternatives rejected with reasons, scope boundaries stated, open questions left open rather than papered over.

Reservations, none blocking:

  • The "fails on the base commit" direction is the author's reported run. It holds up statically (the assertion cannot pass without the new extraction path), and a sponsored @qwen-code /verify run would close it empirically if anyone wants belt and braces.
  • macOS/Windows coverage rides the nightly lanes per repo policy — not this PR's gap, but worth knowing the author only tested Linux.

Approving, pinned to the reviewed commit. ✅

中文说明

置信度:4/5 —— 各阶段均干净;唯一的保留是 before/after 中"基线为红"的一半来自作者粘贴的运行结果,我只能静态佐证。

退一步看:这是一个规范的 fix 该有的样子。问题在生产环境观测到、经 issue 分诊独立确认,根因是一个提取器里缺失的单个分支——而 PR 改的正是这一处,生产代码 15 行。我自己的基线方案与之实质相同;我找过更便宜的路径,没有找到(新增 wire 字段的替代方案要用四个包的改动换同样的用户可见文本,设计文档也明确这么说了)。

让我建立信心的部分:

  • extractErrorMessage 的每个调用点都在设计文档中枚举,与我独立核查的结果一致——全部是展示、日志或事件面。唯一触及提取文本的行为消费方(classifyTurnErrorKind)只精确匹配 terminated,本改动够不到它。优先级保持不变,今天已处理的形状行为不变。
  • 回归测试按构造就是"承重"的:对只有新分支才能产生的字符串做精确相等断言,端到端运行(真实 daemon、真实 ACP 子进程、fake provider 发出生产故障形状),并且已在覆盖两个改动测试文件的 no-AK 集成门禁中跑绿。
  • 设计文档是 maintainer 期望看到的那种产物——五层链路追溯、带理由地否决替代方案、明确范围边界、坦承未决问题而非掩盖。

保留意见(均不阻塞):

  • "在基线提交上失败"这一方向是作者的自述运行。静态上成立(断言离开新提取路径不可能通过),如需双保险,一次赞助的 @qwen-code /verify 运行可以实证了结。
  • macOS/Windows 覆盖按仓库策略走夜间 lane——这不是本 PR 的缺口,但需要知道作者仅在 Linux 上测试过。

批准,锚定在已审查的提交上。✅

Qwen Code · qwen3.8-max

Reviewed at c9d1a8b7ed4fa5a064c38c225a0551098a7798a2 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@doudouOUC
doudouOUC added this pull request to the merge queue Aug 31, 2026
Merged via the queue into QwenLM:main with commit a124120 Aug 31, 2026
220 of 228 checks passed
@doudouOUC
doudouOUC deleted the fix/serve-turn-error-provider-detail branch August 31, 2026 03:13
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

One post-review correction to the PR description, for the record — it doesn't change the approval:

The description says the nested read uses "the same precedence the desktop client already uses for ACP internal-error data", but I couldn't find that precedent in the client code. The actual client-side extractors — DaemonClient.readErrorBody in packages/sdk-typescript (string datadata.detailsdata.message) and extractErrorDetail in packages/web-shell (data.details → string error) — stop before any nested data.error step; the nested read is novel on both sides. The closest shape match anywhere is getErrorMessage in packages/vscode-ide-companion, which is unreachable from acp-bridge by dependency direction.

This is a description inaccuracy, not a code issue: the PR's own precedence (details > message > nested error) is internally consistent and pinned by the new unit cases, and there was no existing utility to reuse regardless — so the fix stands as approved.

中文说明

审查后对 PR 描述做一处更正,留档备查——不影响已给出的批准:

描述中说嵌套读取采用"desktop 客户端处理 ACP internal-error data 已有的相同优先级",但在客户端代码中找不到这个先例。实际的客户端提取逻辑——packages/sdk-typescriptDaemonClient.readErrorBody(字符串 datadata.detailsdata.message)与 packages/web-shellextractErrorDetaildata.details → 字符串 error)——都在嵌套 data.error 之前止步;嵌套读取在两侧都是首次出现。全仓库最接近的形状是 packages/vscode-ide-companiongetErrorMessage,但它因依赖方向无法被 acp-bridge 复用。

这是描述不准确,不是代码问题:PR 自身的优先级(details > message > 嵌套 error)内部自洽且由新增单测固定,而且本来也没有可复用的现成工具——修复本身维持批准结论。

Qwen Code · qwen3.8-max

Reviewed at c9d1a8b7ed4fa5a064c38c225a0551098a7798a2 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(serve): Web Shell shows generic "Internal error" for failed turns, hiding the provider's actual error message

3 participants