Fix bug in the shellExecutionService resulting in both truncation and 3X bloat (#14545)

packages/core/src/services/shellExecutionService.test.ts

@@ -8,9 +8,12 @@ import { vi, describe, it, expect, beforeEach, type Mock } from 'vitest';
 import EventEmitter from 'node:events';
 import type { Readable } from 'node:stream';
 import { type ChildProcess } from 'node:child_process';
-import type { ShellOutputEvent } from './shellExecutionService.js';
+import type {
+  ShellOutputEvent,
+  ShellExecutionConfig,
+} from './shellExecutionService.js';
 import { ShellExecutionService } from './shellExecutionService.js';
-import type { AnsiOutput } from '../utils/terminalSerializer.js';
+import type { AnsiOutput, AnsiToken } from '../utils/terminalSerializer.js';
 
 // Hoisted Mocks
 const mockPtySpawn = vi.hoisted(() => vi.fn());
@@ -64,7 +67,7 @@ const mockProcessKill = vi
   .spyOn(process, 'kill')
   .mockImplementation(() => true);
 
-const shellExecutionConfig = {
+const shellExecutionConfig: ShellExecutionConfig = {
   terminalWidth: 80,
   terminalHeight: 24,
   pager: 'cat',
@@ -76,41 +79,37 @@ const createMockSerializeTerminalToObjectReturnValue = (
   text: string | string[],
 ): AnsiOutput => {
   const lines = Array.isArray(text) ? text : text.split('\n');
-  const expected: AnsiOutput = Array.from(
-    { length: shellExecutionConfig.terminalHeight },
-    (_, i) => [
-      {
-        text: (lines[i] || '').trim(),
-        bold: false,
-        italic: false,
-        underline: false,
-        dim: false,
-        inverse: false,
-        fg: '#ffffff',
-        bg: '#000000',
-      },
-    ],
-  );
+  const len = (shellExecutionConfig.terminalHeight ?? 24) as number;
+  const expected: AnsiOutput = Array.from({ length: len }, (_, i) => [
+    {
+      text: (lines[i] || '').trim(),
+      bold: false,
+      italic: false,
+      underline: false,
+      dim: false,
+      inverse: false,
+      fg: '#ffffff',
+      bg: '#000000',
+    },
+  ]);
   return expected;
 };
 
 const createExpectedAnsiOutput = (text: string | string[]): AnsiOutput => {
   const lines = Array.isArray(text) ? text : text.split('\n');
-  const expected: AnsiOutput = Array.from(
-    { length: shellExecutionConfig.terminalHeight },
-    (_, i) => [
-      {
-        text: expect.stringMatching((lines[i] || '').trim()),
-        bold: false,
-        italic: false,
-        underline: false,
-        dim: false,
-        inverse: false,
-        fg: '',
-        bg: '',
-      },
-    ],
-  );
+  const len = (shellExecutionConfig.terminalHeight ?? 24) as number;
+  const expected: AnsiOutput = Array.from({ length: len }, (_, i) => [
+    {
+      text: expect.stringMatching((lines[i] || '').trim()),
+      bold: false,
+      italic: false,
+      underline: false,
+      dim: false,
+      inverse: false,
+      fg: '',
+      bg: '',
+    } as AnsiToken,
+  ]);
   return expected;
 };
 
@@ -273,6 +272,95 @@ describe('ShellExecutionService', () => {
       );
     });
 
+    it('should capture large output (10000 lines)', async () => {
+      const lineCount = 10000;
+      const lines = Array.from({ length: lineCount }, (_, i) => `line ${i}`);
+      const expectedOutput = lines.join('\n');
+
+      const { result } = await simulateExecution(
+        'large-output-command',
+        (pty) => {
+          // Send data in chunks to simulate realistic streaming
+          // Use \r\n to ensure the terminal moves the cursor to the start of the line
+          const chunkSize = 1000;
+          for (let i = 0; i < lineCount; i += chunkSize) {
+            const chunk = lines.slice(i, i + chunkSize).join('\r\n') + '\r\n';
+            pty.onData.mock.calls[0][0](chunk);
+          }
+          pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
+        },
+      );
+
+      expect(result.exitCode).toBe(0);
+      // The terminal buffer output includes trailing spaces for each line (up to terminal width).
+      // We trim each line to match our expected simple string.
+      const processedOutput = result.output
+        .split('\n')
+        .map((l) => l.trimEnd())
+        .join('\n')
+        .trim();
+      expect(processedOutput).toBe(expectedOutput);
+      expect(result.output.split('\n').length).toBeGreaterThanOrEqual(
+        lineCount,
+      );
+    });
+
+    it('should not add extra padding but preserve explicit trailing whitespace', async () => {
+      const { result } = await simulateExecution('cmd', (pty) => {
+        // "value" should not get terminal-width padding
+        // "value2    " should keep its spaces
+        pty.onData.mock.calls[0][0]('value\r\nvalue2    ');
+        pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
+      });
+
+      expect(result.output).toBe('value\nvalue2    ');
+    });
+
+    it('should truncate output exceeding the scrollback limit', async () => {
+      const scrollbackLimit = 100;
+      const totalLines = 150;
+      // Generate lines: "line 0", "line 1", ...
+      const lines = Array.from({ length: totalLines }, (_, i) => `line ${i}`);
+
+      const { result } = await simulateExecution(
+        'overflow-command',
+        (pty) => {
+          const chunk = lines.join('\r\n') + '\r\n';
+          pty.onData.mock.calls[0][0](chunk);
+          pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
+        },
+        { ...shellExecutionConfig, scrollback: scrollbackLimit },
+      );
+
+      expect(result.exitCode).toBe(0);
+
+      // The terminal should keep the *last* 'scrollbackLimit' lines + lines in the viewport.
+      // xterm.js scrollback is the number of lines *above* the viewport.
+      // So total lines retained = scrollback + rows.
+      // However, our `getFullBufferText` implementation iterates the *active* buffer.
+      // In headless xterm, the buffer length grows.
+      // Let's verify that we have fewer lines than totalLines.
+
+      const outputLines = result.output
+        .trim()
+        .split('\n')
+        .map((l) => l.trimEnd());
+
+      // We expect the *start* of the output to be truncated.
+      // The first retained line should be > "line 0".
+      // Specifically, if we sent 150 lines and have space for roughly 100 + viewport(24),
+      // we should miss the first ~26 lines.
+
+      // Check that we lost some lines from the beginning
+      expect(outputLines.length).toBeLessThan(totalLines);
+      expect(outputLines[0]).not.toBe('line 0');
+
+      // Check that we have the *last* lines
+      expect(outputLines[outputLines.length - 1]).toBe(
+        `line ${totalLines - 1}`,
+      );
+    });
+
     it('should call onPid with the process id', async () => {
       const abortController = new AbortController();
       const handle = await ShellExecutionService.execute(