Skip to content

Commit 7cadd2d

Browse files
wenshaowenshao
authored andcommitted
fix(core): address PR #3642 second-round review feedback
- shellExecutionService streaming: drop stdout/stderr buffer + outputChunks accumulation in streaming mode. Each decoded chunk goes straight to onOutputEvent and is GC-eligible immediately. Long-running background commands (dev servers, watchers) no longer accumulate unbounded memory proportional to total output. Buffered (foreground) mode is unchanged. - shell.ts executeBackground: stripAnsi each chunk before writing to the output file. Dev servers / build tools spam color codes and cursor-move sequences that would render as garbage in the file the agent reads. - bashesCommand: command description "List and manage" → "List background tasks" — current implementation only supports listing, cancellation follows when the unified task_stop tool from #3471 is wired in. Replace the hand-rolled formatRuntime helper with the shared formatDuration utility (uses hideTrailingZeros for parity with the previous output). - backgroundShellRegistry: add a comment documenting the lack of an eviction policy as a known limitation. LRU / age-based / capped-size eviction (and on-disk output rotation) is left as a follow-up alongside the broader output-file lifecycle story.
1 parent cd630c1 commit 7cadd2d

4 files changed

Lines changed: 31 additions & 21 deletions

File tree

packages/cli/src/ui/commands/bashesCommand.ts

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,7 @@ import type { BackgroundShellEntry } from '@qwen-code/qwen-code-core';
88
import type { SlashCommand } from './types.js';
99
import { CommandKind } from './types.js';
1010
import { t } from '../../i18n/index.js';
11-
12-
function formatRuntime(ms: number): string {
13-
if (ms < 0) ms = 0;
14-
const seconds = Math.floor(ms / 1000);
15-
if (seconds < 60) return `${seconds}s`;
16-
const minutes = Math.floor(seconds / 60);
17-
const remainingSeconds = seconds % 60;
18-
return `${minutes}m${remainingSeconds.toString().padStart(2, '0')}s`;
19-
}
11+
import { formatDuration } from '../utils/formatters.js';
2012

2113
function statusLabel(entry: BackgroundShellEntry): string {
2214
switch (entry.status) {
@@ -37,7 +29,7 @@ export const bashesCommand: SlashCommand = {
3729
name: 'tasks',
3830
altNames: ['bashes'],
3931
get description() {
40-
return t('List and manage background tasks');
32+
return t('List background tasks');
4133
},
4234
kind: CommandKind.BUILT_IN,
4335
supportedModes: ['interactive', 'non_interactive', 'acp'] as const,
@@ -68,7 +60,9 @@ export const bashesCommand: SlashCommand = {
6860
];
6961
for (const entry of entries) {
7062
const endTime = entry.endTime ?? now;
71-
const runtime = formatRuntime(endTime - entry.startTime);
63+
const runtime = formatDuration(endTime - entry.startTime, {
64+
hideTrailingZeros: true,
65+
});
7266
const pidPart = entry.pid !== undefined ? ` pid=${entry.pid}` : '';
7367
lines.push(
7468
`[${entry.shellId}] ${statusLabel(entry)} ${runtime}${pidPart} ${entry.command}`,

packages/core/src/services/backgroundShellRegistry.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,12 @@ export interface BackgroundShellEntry {
4848
}
4949

5050
export class BackgroundShellRegistry {
51+
// Entries persist for the session lifetime — no automatic eviction of
52+
// terminal entries. For typical interactive sessions (tens of background
53+
// shells over an hour) this is fine, but long-running sessions that spawn
54+
// many short-lived background commands will see the map and the on-disk
55+
// output files grow without bound. Eviction policy (LRU? age-based? cap?)
56+
// is left as a follow-up alongside output-file rotation.
5157
private readonly entries = new Map<string, BackgroundShellEntry>();
5258

5359
register(entry: BackgroundShellEntry): void {

packages/core/src/services/shellExecutionService.ts

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -418,6 +418,20 @@ export class ShellExecutionService {
418418
}
419419
}
420420

421+
if (streamStdout) {
422+
// Streaming mode: decode + push through immediately. No buffering
423+
// of stdout/stderr strings or outputChunks — long-running
424+
// background commands (dev servers, watchers) would otherwise
425+
// accumulate unbounded memory until exit. The consumer is
426+
// expected to write each chunk to its own sink (e.g. a file).
427+
const decoder = stream === 'stdout' ? stdoutDecoder : stderrDecoder;
428+
const decodedChunk = decoder.decode(data, { stream: true });
429+
onOutputEvent({ type: 'data', chunk: decodedChunk });
430+
return;
431+
}
432+
433+
// Buffered mode (foreground): accumulate for binary sniff +
434+
// a single cleaned-blob emit at exit.
421435
outputChunks.push(data);
422436

423437
if (isStreamingRawContent && sniffedBytes < MAX_SNIFF_SIZE) {
@@ -438,15 +452,6 @@ export class ShellExecutionService {
438452
} else {
439453
stderr += decodedChunk;
440454
}
441-
442-
// Push each chunk through to the consumer in streaming mode so
443-
// long-running background commands (dev servers, watchers) don't
444-
// accumulate output in memory until exit. Default mode (foreground)
445-
// still emits the cleaned final blob in handleExit, preserving
446-
// existing behavior for the in-line shell tool path.
447-
if (streamStdout) {
448-
onOutputEvent({ type: 'data', chunk: decodedChunk });
449-
}
450455
}
451456
};
452457

packages/core/src/tools/shell.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import type {
3030
} from '../services/shellExecutionService.js';
3131
import { ShellExecutionService } from '../services/shellExecutionService.js';
3232
import type { BackgroundShellEntry } from '../services/backgroundShellRegistry.js';
33+
import stripAnsi from 'strip-ansi';
3334
import { formatMemoryUsage } from '../utils/formatters.js';
3435
import type { AnsiOutput } from '../utils/terminalSerializer.js';
3536
import { isSubpaths } from '../utils/paths.js';
@@ -462,7 +463,11 @@ export class ShellToolInvocation extends BaseToolInvocation<
462463
cwd,
463464
(event: ShellOutputEvent) => {
464465
if (event.type === 'data' && typeof event.chunk === 'string') {
465-
outputStream.write(event.chunk);
466+
// Strip ANSI escape codes (color, cursor-move, clear-screen) before
467+
// writing — agents read the file as plain text, and dev servers /
468+
// build tools spam plenty of escape sequences that would render as
469+
// garbage. Costs ~one regex per chunk; cheap relative to disk I/O.
470+
outputStream.write(stripAnsi(event.chunk));
466471
}
467472
// ANSI array chunks and binary streams are not written to the output
468473
// file: agents read the file as plain text and binary spam would be

0 commit comments

Comments
 (0)