Skip to content

fix(core): bound foreground shell output capture #4524

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

Merged
wenshao merged 2 commits into from
Jun 14, 2026
+417 −30
Merged

fix(core): bound foreground shell output capture #4524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16f2e34
fix(core): bound foreground shell output capture
Jerry2003826 May 26, 2026
a8cc894
Merge remote-tracking branch 'origin/main' into Jiarui/fix-large-fore…
Jerry2003826 Jun 4, 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
5 changes: 5 additions & 0 deletions packages/core/src/config/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1297,6 +1297,8 @@ export class Config {
terminalHeight: params.shellExecutionConfig?.terminalHeight ?? 24,
showColor: params.shellExecutionConfig?.showColor ?? false,
pager: params.shellExecutionConfig?.pager ?? 'cat',
maxBufferedOutputBytes:
params.shellExecutionConfig?.maxBufferedOutputBytes,
};
this.truncateToolOutputThreshold =
params.truncateToolOutputThreshold ??
Expand Down Expand Up @@ -3637,6 +3639,9 @@ export class Config {
config.terminalHeight ?? this.shellExecutionConfig.terminalHeight,
showColor: config.showColor ?? this.shellExecutionConfig.showColor,
pager: config.pager ?? this.shellExecutionConfig.pager,
maxBufferedOutputBytes:
config.maxBufferedOutputBytes ??
this.shellExecutionConfig.maxBufferedOutputBytes,
};
}
getScreenReader(): boolean {
Expand Down
209 changes: 207 additions & 2 deletions packages/core/src/services/shellExecutionService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { type ChildProcess } from 'node:child_process';
import pkg from '@xterm/headless';
import type {
ShellAbortReason,
ShellExecutionConfig,
ShellExecuteOptions,
ShellOutputEvent,
ShellPostPromoteSettleInfo,
Expand Down Expand Up @@ -122,7 +123,7 @@ const shellExecutionConfig = {
pager: 'cat',
showColor: false,
disableDynamicLineTrimming: true,
};
} satisfies ShellExecutionConfig;

const WINDOWS_SYSTEM_PATH = 'C:\\Windows\\System32;C:\\Shared\\Tools';
const WINDOWS_USER_PATH = 'C:\\Users\\tester\\bin;C:\\Shared\\Tools';
Expand Down Expand Up @@ -267,7 +268,7 @@ describe('ShellExecutionService', () => {
ptyProcess: typeof mockPtyProcess,
ac: AbortController,
) => void,
config = shellExecutionConfig,
config: ShellExecutionConfig = shellExecutionConfig,
options: ShellExecuteOptions = {},
) => {
const abortController = new AbortController();
Expand Down Expand Up @@ -337,6 +338,65 @@ describe('ShellExecutionService', () => {
expect(result.output.trim()).toBe('你好');
});

@wenshao wenshao May 26, 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 two new tests cover the "two chunks, second partially captured" happy path, but three important scenarios are untested:

  1. Exact-boundary case: send exactly maxBufferedOutputBytes (10) bytes, assert no truncation notice appears. Guards against off-by-one regressions in > vs >= comparisons.
  2. getMaxBufferedOutputBytes validation: 6 input classes (0, -1, NaN, Infinity, 'abc', undefined) should all return the 64MB default. Zero coverage on the validation fallback path.
  3. appendOutputCaptureLimitNotice with empty output: the output ? \${output}\n\n${notice}` : notice` branch is untested.

— qwen3.7-max via Qwen Code /review

@Jerry2003826 Jerry2003826 May 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in 9910ff4. The test file now covers exact-boundary behavior, invalid maxBufferedOutputBytes fallback values (0, -1, NaN, Infinity, abc, undefined), and the empty stripped-output branch where the capture-limit notice is returned by itself.

it('bounds buffered PTY output before building the final string', async () => {
const { result } = await simulateExecution(
'large-output',
(pty) => {
pty.onData.mock.calls[0][0]('12345678');
pty.onData.mock.calls[0][0]('abcdefg');
pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
},
{ ...shellExecutionConfig, maxBufferedOutputBytes: 10 },
);

expect(result.rawOutput.length).toBe(10);
expect(result.output).toContain('12345678ab');
expect(result.output).toContain(
'Output exceeded the maximum captured size',
);
expect(result.output).not.toContain('cdefg');
});

it('keeps PTY replay fallback bounded after the capture limit is exceeded', async () => {
mockSerializeTerminalToText.mockImplementationOnce(() => {
throw new Error('replay failed');
});

const { result } = await simulateExecution(
'large-output-replay-fallback',
(pty) => {
pty.onData.mock.calls[0][0]('12345678');
pty.onData.mock.calls[0][0]('abcdefg');
pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
},
{ ...shellExecutionConfig, maxBufferedOutputBytes: 10 },
);

expect(result.rawOutput.toString()).toBe('12345678ab');
expect(result.output).toContain('12345678ab');
expect(result.output).toContain(
'Output exceeded the maximum captured size',
);
expect(result.output).not.toContain('cdefg');
});

it('does not add a capture-limit notice at the exact PTY buffer boundary', async () => {
const { result } = await simulateExecution(
'exact-output',
(pty) => {
pty.onData.mock.calls[0][0]('1234567890');
pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
},
{ ...shellExecutionConfig, maxBufferedOutputBytes: 10 },
);

expect(result.rawOutput.length).toBe(10);
expect(result.output).toBe('1234567890');
expect(result.output).not.toContain(
'Output exceeded the maximum captured size',
);
});

it('should handle commands with no output', async () => {
await simulateExecution('touch file', (pty) => {
pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
Expand Down Expand Up @@ -1510,6 +1570,29 @@ describe('ShellExecutionService child_process fallback', () => {
return { result, handle, abortController };
};

const simulateExecutionWithConfig = async (
command: string,
simulation: (cp: typeof mockChildProcess, ac: AbortController) => void,
config: ShellExecutionConfig,
options: ShellExecuteOptions = {},
) => {
const abortController = new AbortController();
const handle = await ShellExecutionService.execute(
command,
'/test/dir',
onOutputEventMock,
abortController.signal,
true,
config,
options,
);

await new Promise((resolve) => process.nextTick(resolve));
simulation(mockChildProcess, abortController);
const result = await handle.result;
return { result, handle, abortController };
};

describe('Successful Execution', () => {
it('should execute a command and capture stdout and stderr', async () => {
const { result, handle } = await simulateExecution('ls -l', (cp) => {
Expand Down Expand Up @@ -1566,6 +1649,128 @@ describe('ShellExecutionService child_process fallback', () => {
expect(result.output.trim()).toBe('你好');
});

it('bounds buffered child_process output before building the final string', async () => {
const abortController = new AbortController();
const handle = await ShellExecutionService.execute(
'large-output',
'/test/dir',
onOutputEventMock,
abortController.signal,
false,
{ ...shellExecutionConfig, maxBufferedOutputBytes: 10 },
);

await new Promise((resolve) => process.nextTick(resolve));
mockChildProcess.stdout?.emit('data', Buffer.from('12345678'));
mockChildProcess.stdout?.emit('data', Buffer.from('abcdefg'));
mockChildProcess.emit('exit', 0, null);
mockChildProcess.emit('close', 0, null);

const result = await handle.result;

expect(result.rawOutput.length).toBe(10);
expect(result.output).toContain('12345678ab');
expect(result.output).toContain(
'Output exceeded the maximum captured size',
);
expect(result.output).not.toContain('cdefg');
expect(onOutputEventMock).toHaveBeenCalledWith({
type: 'data',
chunk: expect.stringContaining(
'Output exceeded the maximum captured size',
),
});
});

it('does not add a capture-limit notice at the exact child_process buffer boundary', async () => {
const { result } = await simulateExecutionWithConfig(
'exact-output',
(cp) => {
cp.stdout?.emit('data', Buffer.from('1234567890'));
cp.emit('exit', 0, null);
cp.emit('close', 0, null);
},
{ ...shellExecutionConfig, maxBufferedOutputBytes: 10 },
);

expect(result.rawOutput.length).toBe(10);
expect(result.output).toBe('1234567890');
expect(result.output).not.toContain(
'Output exceeded the maximum captured size',
);
});

it.each([
0,
0.5,
-1,
Number.NaN,
Number.POSITIVE_INFINITY,
'abc',
undefined,
])(
'falls back to the default capture limit for invalid maxBufferedOutputBytes: %s',
async (configuredValue) => {
const { result } = await simulateExecutionWithConfig(
'invalid-limit',
(cp) => {
cp.stdout?.emit('data', Buffer.from('1234567890abcde'));
cp.emit('exit', 0, null);
cp.emit('close', 0, null);
},
{
...shellExecutionConfig,
maxBufferedOutputBytes: configuredValue as unknown as number,
},
);

expect(result.rawOutput.length).toBe(15);
expect(result.output).toBe('1234567890abcde');
expect(result.output).not.toContain(
'Output exceeded the maximum captured size',
);
},
);

it('reports capture-limit notice for streaming child_process output', async () => {
const { result } = await simulateExecutionWithConfig(
'streaming-large-output',
(cp) => {
cp.stdout?.emit('data', Buffer.from('abcdef'));
cp.emit('exit', 0, null);
cp.emit('close', 0, null);
},
{ ...shellExecutionConfig, maxBufferedOutputBytes: 1 },
{ streamStdout: true },
);

expect(onOutputEventMock).toHaveBeenCalledWith({
type: 'data',
chunk: 'abcdef',
});
expect(result.rawOutput.length).toBe(1);
expect(result.output).toContain(
'Output exceeded the maximum captured size',
);
});

it('emits only the capture-limit notice when stripped captured output is empty', async () => {
const { result } = await simulateExecutionWithConfig(
'empty-captured-output',
(cp) => {
cp.stdout?.emit('data', Buffer.from('\nabc'));
cp.emit('exit', 0, null);
cp.emit('close', 0, null);
},
{ ...shellExecutionConfig, maxBufferedOutputBytes: 1 },
);

expect(result.rawOutput.length).toBe(1);
expect(result.output).toMatch(
/^\[Output exceeded the maximum captured size/,
);
});

it('should handle commands with no output', async () => {
const { result } = await simulateExecution('touch file', (cp) => {
cp.emit('exit', 0, null);
Expand Down
Loading
Loading