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 all commits
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
140 changes: 139 additions & 1 deletion 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 @@ -11,6 +11,31 @@ import { DwsClient, DwsCommandError } from './dws-client.js';
vi.mock('node:child_process', () => ({ execFile: vi.fn() }));

describe('DWS command process', () => {
function mockFailedDwsCommand({
code = 1,
stdout = '',
stderr = '',
}: {
code?: unknown;
stdout?: string;
stderr?: string;
}) {
vi.mocked(execFile).mockImplementation(((
_file,
_args,
_options,
callback,
) => {
queueMicrotask(() => {
callback(Object.assign(new Error('exit'), { code }), stdout, stderr);
});
return {
exitCode: typeof code === 'number' ? code : null,
kill: vi.fn(),
};
}) as typeof execFile);
}

afterEach(() => {
vi.useRealTimers();
vi.mocked(execFile).mockReset();
Expand Down Expand Up @@ -49,6 +74,119 @@ describe('DWS command process', () => {
await vi.advanceTimersByTimeAsync(50_000);

expect(child.kill).toHaveBeenCalledWith('SIGKILL');
await expect(result).resolves.toBeInstanceOf(DwsCommandError);
const error = await result;
expect(error).toBeInstanceOf(DwsCommandError);
expect((error as Error).message).toBe('DWS command failed.');
});

it('includes sanitized stderr details when a DWS command exits non-zero', async () => {
mockFailedDwsCommand({
stderr: '\u001b[31mHTTP 400\n{"errorCode":"InvalidArgs"}\u001b[0m',
});

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');
});

it('uses stdout details when stderr is empty', async () => {
mockFailedDwsCommand({ stdout: 'detail on stdout' });

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): detail on stdout',
);
});

it('prefers stderr details over stdout details', async () => {
mockFailedDwsCommand({
stdout: 'detail on stdout',
stderr: 'detail on stderr',
});

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): detail on stderr',
);
expect((error as Error).message).not.toContain('detail on stdout');
});

it('falls back to stdout when stderr sanitizes to empty', async () => {
mockFailedDwsCommand({
stdout: 'error: quota exceeded',
stderr: '\u001b[2K\r',
});

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): error: quota exceeded',
);
});

it('uses a bare message when stderr and stdout sanitize to empty', async () => {
mockFailedDwsCommand({ stderr: '\u001b[2K\r' });

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

expect(error).toBeInstanceOf(DwsCommandError);
expect((error as Error).message).toBe('DWS command failed (1).');
});

it('strips OSC terminal control sequences from failure details', async () => {
mockFailedDwsCommand({
stderr: `${String.fromCharCode(27)}]0;evil${String.fromCharCode(7)}boom`,
});

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

expect(error).toBeInstanceOf(DwsCommandError);
expect((error as Error).message).toContain('boom');
expect((error as Error).message).not.toContain(']0;evil');
});

it('caps failure details before exposing them in the error message', async () => {
mockFailedDwsCommand({ stderr: `${'x'.repeat(300)}tail` });

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

const prefix = 'DWS command failed (1): ';
expect(error).toBeInstanceOf(DwsCommandError);
expect((error as Error).message).toBe(`${prefix}${'x'.repeat(252)}tail`);
expect((error as Error).message).not.toContain('x'.repeat(253));
});

it('keeps later diagnostics from long command output', async () => {
mockFailedDwsCommand({ stderr: `${'noise'.repeat(200)}fatal: denied` });

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

expect(error).toBeInstanceOf(DwsCommandError);
expect((error as Error).message).toContain('fatal: denied');
});
});
36 changes: 35 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,8 @@
*/

import { execFile } from 'node:child_process';
import { stripVTControlCharacters } from 'node:util';
import { sanitizeLogText, truncateCodePoints } from '@qwen-code/channel-base';
import { dwsProcessEnvironment } from './dws-environment.js';
import {
startDwsEventProcess,
Expand All @@ -16,6 +18,8 @@ const DWS_PROCESS_TIMEOUT_MS = 45_000;
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 = 256;
const DWS_ERROR_OUTPUT_WINDOW_CHARS = DWS_ERROR_OUTPUT_MAX_CHARS * 2;
const MAX_MESSAGE_PAGES = 100;
const MAX_TODO_PAGES = 50;
const TODO_PAGE_SIZE = 20;
Expand Down Expand Up @@ -187,6 +191,36 @@ export function classifyDwsCommandFailure(code: unknown): DwsCommandOutcome {
: 'unknown';
}

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

@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] 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)

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

function dwsCommandFailureDetails(output: unknown): string {
const window = String(output ?? '').slice(-DWS_ERROR_OUTPUT_WINDOW_CHARS);
if (!window.trim()) return '';
const details = truncateCodePointsFromEnd(
sanitizeLogText(
stripVTControlCharacters(window),
DWS_ERROR_OUTPUT_WINDOW_CHARS,
),
DWS_ERROR_OUTPUT_MAX_CHARS,
).trim();
return details;
}

function truncateCodePointsFromEnd(str: string, max: number): string {
const truncated = truncateCodePoints(str, max);
if (truncated === str) return str;
return Array.from(str).slice(-max).join('');
}

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