Skip to content

fix(dws): include command output in failures #10279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AaronZ345 wants to merge 4 commits into
base: main
Choose a base branch
from
+174 −2
Open

fix(dws): include command output in failures #10279

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aaa215a
fix(dws): include command stderr in failures
Aug 27, 2026
17a6505
fix(dws): avoid control character regex lint failure
Aug 27, 2026
57d1211
fix(dws): harden command failure details
Aug 27, 2026
67c781a
fix(dws): preserve tail diagnostics in failure details
Aug 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions packages/channels/dws/src/dws-client-process.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,35 @@ describe('DWS command process', () => {
expect(child.kill).toHaveBeenCalledWith('SIGKILL');
await expect(result).resolves.toBeInstanceOf(DwsCommandError);
});

it('includes sanitized stderr details when a DWS command exits non-zero', async () => {
vi.mocked(execFile).mockImplementation(((
_file,
_args,
_options,
callback,
) => {
queueMicrotask(() => {
callback(
Object.assign(new Error('exit 1'), { code: 1 }),
'',
'\u001b[31mHTTP 400\n{"errorCode":"InvalidArgs"}\u001b[0m',
);
});
return {
exitCode: 1,
kill: vi.fn(),
};
}) as typeof execFile);

const error = await new DwsClient({ executable: '/opt/dws' })
.assertCompatible()
.catch((caught: unknown) => caught);

expect(error).toBeInstanceOf(DwsCommandError);
expect((error as Error).message).toContain(
'DWS command failed (1): HTTP 400\\n{"errorCode":"InvalidArgs"}',
);
expect((error as Error).message).not.toContain('[31m');
});
});
20 changes: 19 additions & 1 deletion packages/channels/dws/src/dws-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import { execFile } from 'node:child_process';
import { sanitizeLogText } from '@qwen-code/channel-base';
import { dwsProcessEnvironment } from './dws-environment.js';
import {
startDwsEventProcess,
Expand All @@ -16,6 +17,8 @@
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;

@qwen-code-ci-bot qwen-code-ci-bot Aug 27, 2026

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.

[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)

@qwen-code-ci-bot qwen-code-ci-bot Aug 27, 2026

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.

[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 = /\u001b\[[0-?]*[ -/]*[@-~]/g;

Check failure on line 21 in packages/channels/dws/src/dws-client.ts

View workflow job for this annotation

GitHub Actions / Test (ubuntu-latest, Node 22.x) 

Unexpected control character(s) in regular expression: \x1b

@qwen-code-ci-bot qwen-code-ci-bot Aug 27, 2026

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.

[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)

const MAX_MESSAGE_PAGES = 100;
const MAX_TODO_PAGES = 50;
const TODO_PAGE_SIZE = 20;
Expand Down Expand Up @@ -187,6 +190,21 @@
: 'unknown';
}

function dwsCommandFailureMessage(
code: unknown,
stdout: unknown,
stderr: unknown,
): string {
const base = `DWS command failed${code === undefined ? '' : ` (${String(code)})`}`;

@qwen-code-ci-bot qwen-code-ci-bot Aug 27, 2026

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.

[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.)

Suggested change
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)

@qwen-code-ci-bot qwen-code-ci-bot Aug 27, 2026

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.

[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.)

Suggested change
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 output = String(stderr ?? '').trim() || String(stdout ?? '').trim();

@qwen-code-ci-bot qwen-code-ci-bot Aug 27, 2026

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.

[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)

@qwen-code-ci-bot qwen-code-ci-bot Aug 27, 2026

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.

[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)

if (!output) return `${base}.`;
const details = sanitizeLogText(
output.replace(ANSI_ESCAPE_SEQUENCE, ''),
DWS_ERROR_OUTPUT_MAX_CHARS,
).trim();

@qwen-code-ci-bot qwen-code-ci-bot Aug 27, 2026

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.

[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)

@qwen-code-ci-bot qwen-code-ci-bot Aug 27, 2026

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.

[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)

return details ? `${base}: ${details}` : `${base}.`;
}

function runDwsProcess(
executable: string,
args: string[],
Expand All @@ -212,7 +230,7 @@
const outcome = classifyDwsCommandFailure(code);
reject(
new DwsCommandError(
`DWS command failed${code === undefined ? '' : ` (${String(code)})`}.`,
dwsCommandFailureMessage(code, stdout, stderr),
outcome,
),
);
Expand Down
Loading