fix(dws): include command output in failures - #10279
Conversation
Expose sanitized DWS child-process output in command errors so delivery failures carry enough detail to diagnose rejected sends. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @AaronZ345!
The underlying fix looks sensible, but the description doesn't follow this repo's pull request template — none of the required sections are present: What this PR does, Why it's needed, Reviewer Test Plan (with How to verify, Evidence (Before & After), and the Tested on table), Risk & Scope, Linked Issues, and the Chinese translation in <details>.
Could you rewrite the description against the template? Two things worth covering while you're at it:
- The linked issue #10276 raises three things: the swallowed error detail (this PR), retries replaying the whole model turn instead of re-sending the already-generated answer, and the silent drop after the final failed attempt.
Fixes #10276would auto-close the issue on merge with the other two still open — please spell out inLinked Issuesthat this PR addresses only the error-detail part. - In
How to verify/Tested on: which OS you ran the listed vitest commands on, and what a reviewer should expect in the daemon logs when a DWS command fails — e.g.DWS command failed (1): HTTP 400 ...instead of justDWS command failed (1).
Once the description is updated, push the change (or re-run triage with @qwen-code /triage) and we'll pick it back up.
中文说明
感谢贡献，@AaronZ345！
这个修复本身看起来是合理的，但 PR 描述没有按照本仓库的 pull request 模板填写——缺少所有必需章节：What this PR does、Why it's needed、Reviewer Test Plan（含 How to verify、Evidence (Before & After) 和 Tested on 表格）、Risk & Scope、Linked Issues，以及 <details> 里的中文翻译。
能否按模板重写描述？顺便建议覆盖这两点：
- 关联的 issue #10276 实际提出了三个问题：错误细节被吞掉（本 PR 解决的）、重试时重跑整轮模型对话而不是重发已生成的答案、以及最终失败后静默丢弃消息。
Fixes #10276会在合并时自动关闭 issue，而另外两个问题仍未解决——请在Linked Issues中说明本 PR 只处理错误细节这一部分。 - 在
How to verify/Tested on中：说明你列出的 vitest 命令是在哪个操作系统上运行的，以及 reviewer 在 DWS 命令失败时应在 daemon 日志里看到什么——例如DWS command failed (1): HTTP 400 ...，而不是只有DWS command failed (1).。
描述更新后，推送新提交（或用 @qwen-code /triage 重新触发）即可继续审查。
— Qwen Code · qwen3.8-max
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Test Plan (not a blocker): src/dws-client-process.test.ts — no such file or directory; src/dws-client.test.ts — no such file or directory; src/dws-channel.test.ts — no such file or directory.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const DWS_PROCESS_FORCE_KILL_DELAY_MS = 5_000; | ||
| const MINIMUM_DWS_VERSION = [1, 0, 57] as const; | ||
| const DWS_MAX_OUTPUT_BYTES = 16 * 1024 * 1024; | ||
| const DWS_ERROR_OUTPUT_MAX_CHARS = 1000; |
There was a problem hiding this comment.
[Suggestion] The 1000-code-point detail budget exceeds every consumer cap in this package. All 15 error.message log sites in dws-channel.ts re-cap with sanitizeLogText(..., 300) (logReactionFailure at 200), and ChannelBase.lifecycleError caps at 200 — so, including the ~24-char DWS command failed (N): prefix, at most ~276 chars of detail ever surface. Concretely: an ~800-char JSON error body on stderr is run through the ANSI regex and sanitizeLogText at full cost to build a 1000-code-point detail, and then every site that logs it truncates the whole message to <=300 — ~70-80% of the prepared detail is discarded on every failure while the O(maxBuffer) processing cost is paid in full. The log excerpts in the linked issue are exactly these 300-capped sites. Consider aligning the budget with the consumer caps (e.g. 256, matching GithubAdapter's stderr-hint budget) and windowing the raw output before per-character work — or documenting that the extra budget is intentional for the Error object itself. If the budget changes, extend dws-client-process.test.ts with a stderr detail longer than the budget and assert the detail segment is capped — removing the cap must make that test red.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const MINIMUM_DWS_VERSION = [1, 0, 57] as const; | ||
| const DWS_MAX_OUTPUT_BYTES = 16 * 1024 * 1024; | ||
| const DWS_ERROR_OUTPUT_MAX_CHARS = 1000; | ||
| const ANSI_ESCAPE_SEQUENCE = /\u001b\[[0-?]*[ -/]*[@-~]/g; |
There was a problem hiding this comment.
[Suggestion] This hand-rolled CSI-only regex duplicates stripping the repo already centralizes, and misses non-CSI sequences. OSC payloads — a window title �]0;title�, an OSC 8 hyperlink, or shell-integration marks �]133;A� — pass this regex untouched, and sanitizeLogText then folds ESC/BEL to spaces, leaving the payload text (]0;title) in the message as visible residue — the same residue class this PR removes for SGR colors. Verified by running this exact pipeline against the real sanitizeLogText: the three OSC payloads above produced DWS command failed (1): ]0;title, ...: ]8;;https://example.com click ]8;; and DWS command failed (1): ]133;A, while Node's built-in stripVTControlCharacters returned clean output for all three. GithubAdapter.ts embeds gh child stderr via sanitizeLogText(stderr, 256) with no ANSI strip at all, so this is the repo's fourth ANSI-strip variant and every future adapter must independently remember to copy it. Either fold CSI removal into sanitizeLogText in channel-base (whose docstring already names ANSI injection as its threat model), or use the built-in:
import { stripVTControlCharacters } from 'node:util';
// ...
const details = sanitizeLogText(
stripVTControlCharacters(output),
DWS_ERROR_OUTPUT_MAX_CHARS,
).trim();The dws package depends only on channel-base, so node:util adds no dependency. The existing not.toContain('[31m') assertion in dws-client-process.test.ts goes red if stripping is removed; an added OSC case (stderr '�]0;evil�boom', assert the message does not contain ]0;evil) would pin the broadened strip.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| stdout: unknown, | ||
| stderr: unknown, | ||
| ): string { | ||
| const base = `DWS command failed${code === undefined ? '' : ` (${String(code)})`}`; |
There was a problem hiding this comment.
[Suggestion] The (code) suffix is omitted only for undefined, so killed commands render DWS command failed (null). Any DWS one-shot command that hits DWS_PROCESS_TIMEOUT_MS (45 s) is killed, and Node's callback error then carries code: null — verified with a real execFile timeout (code: null, signal: 'SIGTERM'), and the existing 'escalates a timed-out command to SIGKILL' test stages exactly this shape. The log reader then sees a meaningless (null) token, while the sibling processError in dws-event-stream.ts deliberately omits the suffix via code === undefined || code === null. (The replaced line had the same check, so the rendering predates this PR — but the logic is re-authored in this new helper, so it is worth aligning here.)
| const base = `DWS command failed${code === undefined ? '' : ` (${String(code)})`}`; | |
| const base = `DWS command failed${code === undefined || code === null ? '' : ` (${String(code)})`}`; |
The SIGKILL test already stages code: null — add expect(error.message).not.toContain('(null)') there; it goes red without this fix.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| stderr: unknown, | ||
| ): string { | ||
| const base = `DWS command failed${code === undefined ? '' : ` (${String(code)})`}`; | ||
| const output = String(stderr ?? '').trim() || String(stdout ?? '').trim(); |
There was a problem hiding this comment.
[Suggestion] The stdout-fallback and empty-output branches of this helper have no test. The added test exercises only non-empty stderr, and the pre-existing SIGKILL test that traverses the empty path asserts only instanceof. A future edit deleting or inverting || String(stdout ?? '').trim() — or breaking the empty-details fall-through to the bare base message — ships green, and the silent regression is exactly the diagnostic blind spot this PR removes: a DWS failure that writes its diagnostics only to stdout once again surfaces as bare DWS command failed (1).. Consider adding two cases: one where the callback delivers ('', 'detail on stdout') and the message must contain the stdout detail, and one where stderr is only control characters and the message must be exactly the bare base form. Each new test must go red when the corresponding branch is removed.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| stderr: unknown, | ||
| ): string { | ||
| const base = `DWS command failed${code === undefined ? '' : ` (${String(code)})`}`; | ||
| const output = String(stderr ?? '').trim() || String(stdout ?? '').trim(); |
There was a problem hiding this comment.
[Suggestion] The stderr->stdout fallback selects on the pre-sanitize raw text, so control-only stderr suppresses a usable stdout detail. If dws exits non-zero with stderr made only of terminal housekeeping (e.g. �[2K progress-clear sequences or C1/NEL-only noise) and the real diagnostic on stdout, String(stderr ?? '').trim() is truthy (ESC is not JS whitespace), stdout is never consulted, and after the CSI strip and sanitizeLogText's control folding the detail trims to empty — the error surfaces as bare DWS command failed (1)., losing exactly the diagnostic this PR exists to add. Probed on the unmodified PR through the real runDwsProcess path: stderr '�[2K ' + stdout 'error: quota exceeded' gave "DWS command failed (1)." (same for a bare '�' byte); with the sanitize-first selection below the same inputs give "DWS command failed (1): error: quota exceeded" and this PR's own tests stay green. Select on the sanitized detail instead: sanitize stderr's detail first, and if it trims empty, sanitize and use stdout's detail before falling back to the bare base message. A test staging control-only stderr plus diagnostic stdout (asserting the message contains : error: quota exceeded) goes red if the raw-text selection is restored.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const details = sanitizeLogText( | ||
| output.replace(ANSI_ESCAPE_SEQUENCE, ''), | ||
| DWS_ERROR_OUTPUT_MAX_CHARS, | ||
| ).trim(); |
There was a problem hiding this comment.
[Suggestion] The entire output — up to the 16 MiB DWS_MAX_OUTPUT_BYTES maxBuffer — is ANSI-scanned and then passed whole into sanitizeLogText, whose truncateCodePoints runs Array.from(str) over the entire string before keeping 1000 code points. Before this diff the failure message was built O(1) from code alone. Measured on a 16,777,202-char string (~maxBuffer): the regex replace took 8.7 ms, Array.from 109.4 ms with a ~100 MB transient heap allocation — against 0.02 ms for the capped slice. A dws command that exits non-zero with multi-MB output — verbose CLI errors, or an ERR_CHILD_PROCESS_STDIO_MAXBUFFER overrun, which delivers a partial buffer near maxBuffer size (the JSDoc names that case) — pays that synchronous stall and allocation inside the channel daemon while it polls live IM sessions. Consider bounding the work before the per-character passes, e.g. window the raw output first:
const window = output.slice(0, 2 * DWS_ERROR_OUTPUT_MAX_CHARS);
const details = sanitizeLogText(
window.replace(ANSI_ESCAPE_SEQUENCE, ''),
DWS_ERROR_OUTPUT_MAX_CHARS,
).trim();2000 UTF-16 units always contain the first 1000 code points, and a mid-CSI window cut stays safe because sanitizeLogText deletes any surviving bare ESC.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const details = sanitizeLogText( | ||
| output.replace(ANSI_ESCAPE_SEQUENCE, ''), | ||
| DWS_ERROR_OUTPUT_MAX_CHARS, | ||
| ).trim(); |
There was a problem hiding this comment.
[Suggestion] The 1000-code-point cap is pinned by no test — the added test feeds only ~45 chars of stderr, and a repo-wide grep finds DWS_ERROR_OUTPUT_MAX_CHARS only at its declaration and its single read site. A future edit that drops or mis-applies the cap (replacing the constant with Infinity or removing the truncate step) ships green, and the regression reaching production is multi-MB payloads embedded in exception messages that surface in logs and UI. Consider adding a case to dws-client-process.test.ts where the mocked stderr exceeds 1000 chars and asserting the details portion is capped (e.g. char 1001 absent); removing the cap must make that test red.
— qwen3.8-max via Qwen Code /review (v0.22.2)
Build the ANSI escape pattern at runtime so the DWS error output stripping logic passes lint while preserving behavior. Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-4 stdout-fallback and empty-output branches have no test — already reported (comment 3871734973)
- R1-5 stderr->stdout fallback selects on pre-sanitize raw text — already reported (comment 3871734983)
- R1-6 whole-output ANSI scan before the 1000-code-point cut — already reported (comment 3871734988)
- R1-7 1000-code-point cap pinned by no test — already reported (comment 3871734998)
Test Plan (not a blocker): src/dws-client-process.test.ts — no such file or directory; src/dws-client.test.ts — no such file or directory; src/dws-channel.test.ts — no such file or directory.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/channels/dws/src/dws-client.ts:204 — [probe] failure detail truncated at 1000 code points with no truncation marker
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const DWS_PROCESS_FORCE_KILL_DELAY_MS = 5_000; | ||
| const MINIMUM_DWS_VERSION = [1, 0, 57] as const; | ||
| const DWS_MAX_OUTPUT_BYTES = 16 * 1024 * 1024; | ||
| const DWS_ERROR_OUTPUT_MAX_CHARS = 1000; |
There was a problem hiding this comment.
[Suggestion] R1-1: The 1000-code-point detail budget exceeds every consumer cap in this package. All 15 error.message log sites in dws-channel.ts re-cap with sanitizeLogText(..., 300) (logReactionFailure at 200), and ChannelBase.lifecycleError caps at 200 — so, including the ~24-char DWS command failed (N): prefix, at most ~276 chars of detail ever surface. Concretely: an ~800-char JSON error body on stderr is run through the ANSI regex and sanitizeLogText at full cost to build a 1000-code-point detail, and then every site that logs it truncates the whole message to <=300 — ~70-80% of the prepared detail is discarded on every failure while the O(maxBuffer) processing cost is paid in full. The log excerpts in the linked issue are exactly these 300-capped sites. Consider aligning the budget with the consumer caps (e.g. 256, matching GithubAdapter's stderr-hint budget) and windowing the raw output before per-character work — or documenting that the extra budget is intentional for the Error object itself. If the budget changes, extend dws-client-process.test.ts with a stderr detail longer than the budget and assert the detail segment is capped — removing the cap must make that test red.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const ANSI_ESCAPE_SEQUENCE = new RegExp( | ||
| `${String.fromCharCode(27)}\\[[0-?]*[ -/]*[@-~]`, | ||
| 'g', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-2: This hand-rolled CSI-only regex duplicates stripping the repo already centralizes, and misses non-CSI sequences. OSC payloads — a window title ^[]0;title^G, an OSC 8 hyperlink, or shell-integration marks ^[]133;A^G — pass this regex untouched, and sanitizeLogText then folds ESC/BEL to spaces, leaving the payload text (]0;title) in the message as visible residue — the same residue class this PR removes for SGR colors. Verified by running this exact pipeline against the real sanitizeLogText: the three OSC payloads above produced DWS command failed (1): ]0;title, ...: ]8;;https://example.com click ]8;; and DWS command failed (1): ]133;A, while Node's built-in stripVTControlCharacters returned clean output for all three. GithubAdapter.ts embeds gh child stderr via sanitizeLogText(stderr, 256) with no ANSI strip at all, so this is the repo's fourth ANSI-strip variant and every future adapter must independently remember to copy it. Either fold CSI removal into sanitizeLogText in channel-base (whose docstring already names ANSI injection as its threat model), or use the built-in:
import { stripVTControlCharacters } from 'node:util';
// ...
const details = sanitizeLogText(
stripVTControlCharacters(output),
DWS_ERROR_OUTPUT_MAX_CHARS,
).trim();The dws package depends only on channel-base, so node:util adds no dependency. The existing not.toContain('[31m') assertion in dws-client-process.test.ts goes red if stripping is removed; an added OSC case (stderr '^[]0;evil^Gboom', assert the message does not contain ]0;evil) would pin the broadened strip.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| stdout: unknown, | ||
| stderr: unknown, | ||
| ): string { | ||
| const base = `DWS command failed${code === undefined ? '' : ` (${String(code)})`}`; |
There was a problem hiding this comment.
[Suggestion] R1-3: The (code) suffix is omitted only for undefined, so killed commands render DWS command failed (null). Any DWS one-shot command that hits DWS_PROCESS_TIMEOUT_MS (45 s) is killed, and Node's callback error then carries code: null — verified with a real execFile timeout (code: null, signal: 'SIGTERM'), and the existing 'escalates a timed-out command to SIGKILL' test stages exactly this shape. The log reader then sees a meaningless (null) token, while the sibling processError in dws-event-stream.ts deliberately omits the suffix via code === undefined || code === null. (The replaced line had the same check, so the rendering predates this PR — but the logic is re-authored in this new helper, so it is worth aligning here.)
| const base = `DWS command failed${code === undefined ? '' : ` (${String(code)})`}`; | |
| const base = `DWS command failed${code === undefined || code === null ? '' : ` (${String(code)})`}`; |
The SIGKILL test already stages code: null — add expect(error.message).not.toContain('(null)') there; it goes red without this fix.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| const DWS_MAX_OUTPUT_BYTES = 16 * 1024 * 1024; | ||
| const DWS_ERROR_OUTPUT_MAX_CHARS = 1000; | ||
| const ANSI_ESCAPE_SEQUENCE = new RegExp( | ||
| `${String.fromCharCode(27)}\\[[0-?]*[ -/]*[@-~]`, |
There was a problem hiding this comment.
[Suggestion] R2-1: This String.fromCharCode(27) construction exists solely to dodge ESLint's no-control-regex — the commit that introduced it ("fix(dws): avoid control character regex lint failure") says so, but nothing in the code does. Verified by lint probe: the round-1 literal form /\u001b\[[0-?]*[ -/]*[@-~]/g fails this repo's config with Unexpected control character(s) in regular expression: \x1b no-control-regex, while this file lints clean. A maintainer cleaning this up later "simplifies" the construction back to that canonical literal, and CI fails with an error whose cause is invisible at the failure site, forcing git-history archaeology to reconstruct the reason. Add a one-line comment above the constant — e.g. // Built via String.fromCharCode so no literal control character appears in source; a regex literal with \u001b fails no-control-regex. — or return to the literal with the // eslint-disable-next-line no-control-regex convention that sanitize.ts and textUtils.ts already use for this exact rule.
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): src/dws-client-process.test.ts — no such file or directory; src/dws-client.test.ts — no such file or directory; src/dws-channel.test.ts — no such file or directory.
Convergence: round 3 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 4 (1 new). Findings keep coming back to the same files: packages/channels/dws/src/dws-client.ts (findings in rounds 1, 2; 2 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：src/dws-client-process.test.ts — no such file or directory; src/dws-client.test.ts — no such file or directory; src/dws-channel.test.ts — no such file or directory。
收敛情况：第 3 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 4 条（其中 1 条首次提出）。发现反复回到同一批文件：packages/channels/dws/src/dws-client.ts（第 1、2 轮已出过发现，本轮又有 2 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
| } | ||
|
|
||
| function dwsCommandFailureDetails(output: unknown): string { | ||
| const window = String(output ?? '').slice(0, DWS_ERROR_OUTPUT_WINDOW_CHARS); |
There was a problem hiding this comment.
[Suggestion] R1-1: (fix-induced) The round-2 fix for this thread did both things it asked — the budget is now 256 code points, aligned with the consumer caps, and the raw output is windowed before per-character work — but the new window is taken from the HEAD of the stream, and that choice opened a new defect at this site: .slice(0, DWS_ERROR_OUTPUT_WINDOW_CHARS) keeps only the first 512 units, so diagnostic content written later never surfaces. A dws command that writes more than 512 chars of progress/banner/diagnostics to stderr before failing — consistent with the line-oriented stderr diagnostics this channel's own event-stream code models — leaves the actual reason (typically one of the last lines a CLI prints) outside the window, and the operator sees DWS command failed (1): <early noise> — plausible-looking detail without the reason, which is exactly the diagnosability gap this PR exists to close. The unconditional stderr-over-stdout preference compounds it: one printable line of stderr decoration also suppresses a real error that arrived on stdout. Probed against the unmodified PR code: with stderr of ~690 units of diagnostics ending in error: the real failure reason (token expired) and stdout stdout fallback detail, the message contained neither — contains real reason = false, contains stdout fallback = false; it was 256 code points of early noise. Consider excerpting from the tail of the cleaned output so the most recent lines survive, e.g. const window = stripVTControlCharacters(String(output ?? '')).slice(-DWS_ERROR_OUTPUT_WINDOW_CHARS); — note the existing cap test pins head behaviour via not.toContain('tail') and would need updating with the switch. A test feeding 'x'.repeat(600) + 'token expired' and asserting the message contains 'token expired' goes red against the current head-excerpt; please keep it red when the windowing is removed or flipped back to the head.
中文说明
上一轮针对本线程的修复做到了它要求的两点——预算已对齐消费者上限（256 个码点）、且在对输出逐字符处理前先取了窗口——但新窗口取的是输出的头部，这个选择在同一位置引入了新的缺陷：.slice(0, DWS_ERROR_OUTPUT_WINDOW_CHARS) 只保留前 512 个单元，之后写出的诊断内容永远不会出现。如果 dws 命令在失败前向 stderr 写入超过 512 字符的进度/横幅/诊断信息（这与本 channel 的 event-stream 代码所建模的按行 stderr 诊断一致），真正的原因（CLI 通常最后才打印失败行）会落在窗口之外，运维只会看到 DWS command failed (1): <前面的噪音>——看起来有细节、却没有原因，而这正是本 PR 要消除的可诊断性缺口。无条件的 stderr 优先、stdout 兜底还会放大该问题：只要 stderr 有一行可打印的装饰内容，到达 stdout 的真正错误就会被压掉。已在未改动的 PR 代码上实测：约 690 单元、以 error: the real failure reason (token expired) 结尾的 stderr 加上 stdout stdout fallback detail 时，消息两者都不含——contains real reason = false、contains stdout fallback = false，只有 256 个码点的前段噪音。建议改为从清理后输出的尾部取窗口，让最近的行得以保留，例如 const window = stripVTControlCharacters(String(output ?? '')).slice(-DWS_ERROR_OUTPUT_WINDOW_CHARS);——注意现有上限测试通过 not.toContain('tail') 固定了头部行为，切换时需要同步更新。建议补一个测试：输入 'x'.repeat(600) + 'token expired'，断言消息包含 'token expired'；该测试对当前头部取窗会失败（红）。如果窗口被移除或改回头部取值，请确保它仍然失败。
— qwen3.8-max via Qwen Code /review (v0.22.2)
| ): string { | ||
| const base = `DWS command failed${code === undefined || code === null ? '' : ` (${String(code)})`}`; | ||
| const details = | ||
| dwsCommandFailureDetails(stderr) || dwsCommandFailureDetails(stdout); |
There was a problem hiding this comment.
[Suggestion] R3-1: The stderr-over-stdout precedence of this fallback chain is pinned by no test: mockFailedDwsCommand defaults both streams to '' and every test populates at most one of them. A one-line mutation swapping the operands (dwsCommandFailureDetails(stdout) || dwsCommandFailureDetails(stderr)) survives the whole file — verified by mutation run in a scratch tree at this commit: with the operands flipped, npx vitest run src/dws-client-process.test.ts still reports Tests 7 passed (7). So a DWS command that prints diagnostic noise on stdout while the real error lands on stderr — the common case this ordering exists for — could silently start surfacing the stdout noise in DwsCommandError.message after a future refactor, with no test ever going red. Add one test that calls mockFailedDwsCommand({ stdout: 'stdout noise', stderr: 'real error' }) and asserts the message contains real error and not stdout noise; it must go red if the || operand order is flipped (under the mutation it fails with expected 'DWS command failed (1): stdout noise' to contain 'real error').
中文说明
这个兜底链中 stderr 优先于 stdout 的顺序没有任何测试固定：mockFailedDwsCommand 默认两个流都是 ''，而每个测试至多只填充其中一个。把两个操作数对调（dwsCommandFailureDetails(stdout) || dwsCommandFailureDetails(stderr)）的单行变异可以通过整个测试文件——已在本提交的临时树中实测：对调后 npx vitest run src/dws-client-process.test.ts 仍报告 Tests 7 passed (7)。因此，一个在 stdout 打印诊断噪音、而真正错误落在 stderr 的 DWS 命令（正是这个顺序存在的常见场景），在未来重构后可能会悄无声息地在 DwsCommandError.message 中呈现 stdout 噪音，而没有任何测试变红。请补一个测试：调用 mockFailedDwsCommand({ stdout: 'stdout noise', stderr: 'real error' })，断言消息包含 real error 且不包含 stdout noise；当 || 操作数顺序被对调时它必须失败（变异下报错为 expected 'DWS command failed (1): stdout noise' to contain 'real error'）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): "agent 1a": run packages/channels/dws/src/dws-client-process.test.ts itself — the review worktree has no node_modules or built dist/ , and installing/building would foul….
Test Plan (not a blocker): src/dws-client-process.test.ts — no such file or directory; src/dws-client.test.ts — no such file or directory; src/dws-channel.test.ts — no such file or directory.
3 Suggestion(s) were drafted inline past the resolved critical posting floor — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s); the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 4, not a blocker) — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s) — recorded, not requested in this round:
packages/channels/dws/src/dws-client.ts:206 — [review] R1-1: (fix-induced) The round-3 fix for this thread did what it asked — the window now excerpts from the tail, so late diagnostics survive, and the new 'keeps later diagnostics from lo…packages/channels/dws/src/dws-client.ts:208 — [review] The tail cut added in this commit runs AFTER sanitizeLogText expands newlines into two-character \n escapes, so the 256-code-point cut can land between the \ and the n of an esc…packages/channels/dws/src/dws-client.ts:221 — [review] The code-point-safe tail truncation in truncateCodePointsFromEnd is pinned by no test: every input fed through the detail pipeline in this file is ASCII, and a str.slice(-max) mutan…
中文说明
无阻断问题。LGTM！✅
未探索到全部深度（达到工具调用预算）："agent 1a"：run packages/channels/dws/src/dws-client-process.test.ts itself — the review worktree has no node_modules or built dist/ , and installing/building would foul…。
Test Plan（非阻断）：src/dws-client-process.test.ts — no such file or directory; src/dws-client.test.ts — no such file or directory; src/dws-channel.test.ts — no such file or directory。
3 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论——发布下限因首次发现速率连续 2 轮未下降而提前生效；CLI 已将其移入下方延后清单（下限强制执行）。
收敛姿态下延后（第 4 轮，非阻断）——发布下限因首次发现速率连续 2 轮未下降而提前生效——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
2 participants
What this PR does
Includes sanitized stderr/stdout details in DWS command failure errors instead of reporting only the exit code. ANSI color sequences are stripped before the detail is attached, and the diagnostic text is capped so command failures remain readable.
Why it's needed
When a DWS command exits non-zero, the current daemon log only says
DWS command failed (1).That hides the actionable failure detail from operators and makes debugging require reproducing the command outside the daemon. This PR preserves the relevant command output so failures surface as messages likeDWS command failed (1): HTTP 400 ....Reviewer Test Plan
How to verify
Trigger or review the regression test for a DWS command that exits non-zero with ANSI-colored stderr. The expected behavior is that the thrown error and daemon log include the sanitized stderr/stdout detail, for example
DWS command failed (1): HTTP 400 ..., rather than onlyDWS command failed (1).Evidence (Before & After)
Before: non-zero DWS exits surfaced only the exit code, so the model/operator saw
DWS command failed (1).with no stderr/stdout detail.After: non-zero DWS exits include a sanitized, bounded command-output excerpt in the error message; ANSI control codes are removed before the excerpt is appended.
Local verification:
NO_COLOR=1 TERM=dumb npx vitest run src/dws-client-process.test.ts -t "includes sanitized stderr" --reporter verbose NO_COLOR=1 TERM=dumb npx vitest run src/dws-client-process.test.ts NO_COLOR=1 TERM=dumb npx vitest run src/dws-client.test.ts NO_COLOR=1 TERM=dumb npx vitest run src/dws-channel.test.ts NO_COLOR=1 TERM=dumb npx vitest run src/dws-client-process.test.ts src/dws-client.test.ts src/dws-channel.test.ts NO_COLOR=1 TERM=dumb npm run buildTested on
Environment (optional)
Local tests were run in
packages/channels/dwson macOS withNO_COLOR=1 TERM=dumb.Risk & Scope
Linked Issues
Related to #10276. This PR addresses only the swallowed error-detail portion of that issue; retry replay behavior and final failed-attempt handling remain out of scope.
中文说明
What this PR does
在 DWS 命令失败错误里保留经过清理的 stderr/stdout 细节，而不是只返回退出码。错误详情会先去掉 ANSI 颜色控制序列，并做长度限制，避免失败信息过长。
Why it's needed
DWS 命令非零退出时，当前 daemon 日志只显示
DWS command failed (1).，会把真正可用于排查的失败原因藏起来，排障时只能到 daemon 外重新复现命令。本 PR 会把相关命令输出带回错误信息，例如DWS command failed (1): HTTP 400 ...。Reviewer Test Plan
How to verify
运行或检查新增回归测试：构造一个带 ANSI 彩色 stderr 的非零退出 DWS 命令。预期错误和 daemon 日志包含清理后的 stderr/stdout 摘要，例如
DWS command failed (1): HTTP 400 ...，而不是只有DWS command failed (1).。Evidence (Before & After)
Before：DWS 非零退出只暴露退出码，模型/操作者只能看到
DWS command failed (1).，看不到 stderr/stdout 里的具体错误。After：DWS 非零退出会在错误信息里附带经过清理和截断的命令输出摘要；追加前会移除 ANSI 控制码。
本地验证：
NO_COLOR=1 TERM=dumb npx vitest run src/dws-client-process.test.ts -t "includes sanitized stderr" --reporter verbose NO_COLOR=1 TERM=dumb npx vitest run src/dws-client-process.test.ts NO_COLOR=1 TERM=dumb npx vitest run src/dws-client.test.ts NO_COLOR=1 TERM=dumb npx vitest run src/dws-channel.test.ts NO_COLOR=1 TERM=dumb npx vitest run src/dws-client-process.test.ts src/dws-client.test.ts src/dws-channel.test.ts NO_COLOR=1 TERM=dumb npm run buildTested on
Environment (optional)
本地测试在 macOS 的
packages/channels/dws目录下运行，环境变量为NO_COLOR=1 TERM=dumb。Risk & Scope
Linked Issues
关联 #10276。本 PR 只处理该 issue 中“错误详情被吞掉”的部分；重试重跑整轮对话和最终失败处理不在本 PR 范围内。