fix(core): fix PTY descriptor shell leak (#16773)

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

@@ -30,11 +30,20 @@ const mockIsBinary = vi.hoisted(() => vi.fn());
 const mockPlatform = vi.hoisted(() => vi.fn());
 const mockGetPty = vi.hoisted(() => vi.fn());
 const mockSerializeTerminalToObject = vi.hoisted(() => vi.fn());
+const mockResolveExecutable = vi.hoisted(() => vi.fn());
 
 // Top-level Mocks
 vi.mock('@lydell/node-pty', () => ({
   spawn: mockPtySpawn,
 }));
+vi.mock('../utils/shell-utils.js', async (importOriginal) => {
+  const actual =
+    await importOriginal<typeof import('../utils/shell-utils.js')>();
+  return {
+    ...actual,
+    resolveExecutable: mockResolveExecutable,
+  };
+});
 vi.mock('node:child_process', async (importOriginal) => {
   const actual = await importOriginal();
   return {
@@ -45,7 +54,7 @@ vi.mock('node:child_process', async (importOriginal) => {
 vi.mock('../utils/textUtils.js', () => ({
   isBinary: mockIsBinary,
 }));
-vi.mock('os', () => ({
+vi.mock('node:os', () => ({
   default: {
     platform: mockPlatform,
     constants: {
@@ -136,6 +145,7 @@ describe('ShellExecutionService', () => {
     onExit: Mock;
     write: Mock;
     resize: Mock;
+    destroy: Mock;
   };
   let mockHeadlessTerminal: {
     resize: Mock;
@@ -153,6 +163,8 @@ describe('ShellExecutionService', () => {
     mockSerializeTerminalToObject.mockReturnValue([]);
     mockIsBinary.mockReturnValue(false);
     mockPlatform.mockReturnValue('linux');
+    mockResolveExecutable.mockImplementation(async (exe: string) => exe);
+    process.env['PATH'] = '/test/path';
     mockGetPty.mockResolvedValue({
       module: { spawn: mockPtySpawn },
       name: 'mock-pty',
@@ -167,6 +179,7 @@ describe('ShellExecutionService', () => {
       onExit: Mock;
       write: Mock;
       resize: Mock;
+      destroy: Mock;
     };
     mockPtyProcess.pid = 12345;
     mockPtyProcess.kill = vi.fn();
@@ -174,6 +187,7 @@ describe('ShellExecutionService', () => {
     mockPtyProcess.onExit = vi.fn();
     mockPtyProcess.write = vi.fn();
     mockPtyProcess.resize = vi.fn();
+    mockPtyProcess.destroy = vi.fn();
 
     mockHeadlessTerminal = {
       resize: vi.fn(),
@@ -821,6 +835,31 @@ describe('ShellExecutionService', () => {
       );
     });
   });
+
+  describe('Resource Management', () => {
+    it('should destroy the PTY process and clear activePtys on exit', async () => {
+      await simulateExecution('ls -l', (pty) => {
+        pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
+      });
+
+      expect(mockPtyProcess.destroy).toHaveBeenCalled();
+      expect(ShellExecutionService['activePtys'].size).toBe(0);
+    });
+
+    it('should destroy the PTY process even if destroy throws', async () => {
+      mockPtyProcess.destroy.mockImplementation(() => {
+        throw new Error('Destroy failed');
+      });
+
+      await expect(
+        simulateExecution('ls -l', (pty) => {
+          pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
+        }),
+      ).resolves.not.toThrow();
+
+      expect(ShellExecutionService['activePtys'].size).toBe(0);
+    });
+  });
 });
 
 describe('ShellExecutionService child_process fallback', () => {

packages/core/src/services/shellExecutionService.ts

@@ -12,7 +12,11 @@ import { TextDecoder } from 'node:util';
 import os from 'node:os';
 import type { IPty } from '@lydell/node-pty';
 import { getCachedEncodingForBuffer } from '../utils/systemEncoding.js';
-import { getShellConfiguration, type ShellType } from '../utils/shell-utils.js';
+import {
+  getShellConfiguration,
+  resolveExecutable,
+  type ShellType,
+} from '../utils/shell-utils.js';
 import { isBinary } from '../utils/textUtils.js';
 import pkg from '@xterm/headless';
 import {
@@ -183,7 +187,7 @@ export class ShellExecutionService {
       const ptyInfo = await getPty();
       if (ptyInfo) {
         try {
-          return this.executeWithPty(
+          return await this.executeWithPty(
             commandToExecute,
             cwd,
             onOutputEvent,
@@ -445,14 +449,14 @@ export class ShellExecutionService {
     }
   }
 
-  private static executeWithPty(
+  private static async executeWithPty(
     commandToExecute: string,
     cwd: string,
     onOutputEvent: (event: ShellOutputEvent) => void,
     abortSignal: AbortSignal,
     shellExecutionConfig: ShellExecutionConfig,
     ptyInfo: PtyImplementation,
-  ): ShellExecutionHandle {
+  ): Promise<ShellExecutionHandle> {
     if (!ptyInfo) {
       // This should not happen, but as a safeguard...
       throw new Error('PTY implementation not found');
@@ -461,6 +465,14 @@ export class ShellExecutionService {
       const cols = shellExecutionConfig.terminalWidth ?? 80;
       const rows = shellExecutionConfig.terminalHeight ?? 30;
       const { executable, argsPrefix, shell } = getShellConfiguration();
+
+      const resolvedExecutable = await resolveExecutable(executable);
+      if (!resolvedExecutable) {
+        throw new Error(
+          `Shell executable "${executable}" not found in PATH or at absolute location. Please ensure the shell is installed and available in your environment.`,
+        );
+      }
+
       const guardedCommand = ensurePromptvarsDisabled(commandToExecute, shell);
       const args = [...argsPrefix, guardedCommand];
 
@@ -660,6 +672,12 @@ export class ShellExecutionService {
             exited = true;
             abortSignal.removeEventListener('abort', abortHandler);
             this.activePtys.delete(ptyProcess.pid);
+            // Attempt to destroy the PTY to ensure FD is closed
+            try {
+              (ptyProcess as IPty & { destroy?: () => void }).destroy?.();
+            } catch {
+              // Ignore errors during cleanup
+            }
 
             const finalize = () => {
               render(true);

packages/core/src/utils/shell-utils.test.ts

@@ -20,7 +20,9 @@ import {
   initializeShellParsers,
   stripShellWrapper,
   hasRedirection,
+  resolveExecutable,
 } from './shell-utils.js';
+import path from 'node:path';
 
 const mockPlatform = vi.hoisted(() => vi.fn());
 const mockHomedir = vi.hoisted(() => vi.fn());
@@ -33,6 +35,20 @@ vi.mock('os', () => ({
   homedir: mockHomedir,
 }));
 
+const mockAccess = vi.hoisted(() => vi.fn());
+vi.mock('node:fs', () => ({
+  default: {
+    promises: {
+      access: mockAccess,
+    },
+    constants: { X_OK: 1 },
+  },
+  promises: {
+    access: mockAccess,
+  },
+  constants: { X_OK: 1 },
+}));
+
 const mockSpawnSync = vi.hoisted(() => vi.fn());
 vi.mock('node:child_process', () => ({
   spawnSync: mockSpawnSync,
@@ -463,3 +479,65 @@ describe('hasRedirection (PowerShell via mock)', () => {
     expect(hasRedirection('echo "-> arrow"')).toBe(true);
   });
 });
+
+describe('resolveExecutable', () => {
+  const originalEnv = process.env;
+
+  beforeEach(() => {
+    process.env = { ...originalEnv };
+    mockAccess.mockReset();
+  });
+
+  afterEach(() => {
+    process.env = originalEnv;
+  });
+
+  it('should return the absolute path if it exists and is executable', async () => {
+    const absPath = path.resolve('/usr/bin/git');
+    mockAccess.mockResolvedValue(undefined); // success
+    expect(await resolveExecutable(absPath)).toBe(absPath);
+    expect(mockAccess).toHaveBeenCalledWith(absPath, 1);
+  });
+
+  it('should return undefined for absolute path if it does not exist', async () => {
+    const absPath = path.resolve('/usr/bin/nonexistent');
+    mockAccess.mockRejectedValue(new Error('ENOENT'));
+    expect(await resolveExecutable(absPath)).toBeUndefined();
+  });
+
+  it('should resolve executable in PATH', async () => {
+    const binDir = path.resolve('/bin');
+    const usrBinDir = path.resolve('/usr/bin');
+    process.env['PATH'] = `${binDir}${path.delimiter}${usrBinDir}`;
+    mockPlatform.mockReturnValue('linux');
+
+    const targetPath = path.join(usrBinDir, 'ls');
+    mockAccess.mockImplementation(async (p: string) => {
+      if (p === targetPath) return undefined;
+      throw new Error('ENOENT');
+    });
+
+    expect(await resolveExecutable('ls')).toBe(targetPath);
+  });
+
+  it('should try extensions on Windows', async () => {
+    const sys32 = path.resolve('C:\\Windows\\System32');
+    process.env['PATH'] = sys32;
+    mockPlatform.mockReturnValue('win32');
+    mockAccess.mockImplementation(async (p: string) => {
+      // Use includes because on Windows path separators might differ
+      if (p.includes('cmd.exe')) return undefined;
+      throw new Error('ENOENT');
+    });
+
+    expect(await resolveExecutable('cmd')).toContain('cmd.exe');
+  });
+
+  it('should return undefined if not found in PATH', async () => {
+    process.env['PATH'] = path.resolve('/bin');
+    mockPlatform.mockReturnValue('linux');
+    mockAccess.mockRejectedValue(new Error('ENOENT'));
+
+    expect(await resolveExecutable('unknown')).toBeUndefined();
+  });
+});

packages/core/src/utils/shell-utils.ts

@@ -5,6 +5,8 @@
  */
 
 import os from 'node:os';
+import fs from 'node:fs';
+import path from 'node:path';
 import { quote } from 'shell-quote';
 import {
   spawn,
@@ -37,6 +39,35 @@ export interface ShellConfiguration {
   shell: ShellType;
 }
 
+export async function resolveExecutable(
+  exe: string,
+): Promise<string | undefined> {
+  if (path.isAbsolute(exe)) {
+    try {
+      await fs.promises.access(exe, fs.constants.X_OK);
+      return exe;
+    } catch {
+      return undefined;
+    }
+  }
+  const paths = (process.env['PATH'] || '').split(path.delimiter);
+  const extensions =
+    os.platform() === 'win32' ? ['.exe', '.cmd', '.bat', ''] : [''];
+
+  for (const p of paths) {
+    for (const ext of extensions) {
+      const fullPath = path.join(p, exe + ext);
+      try {
+        await fs.promises.access(fullPath, fs.constants.X_OK);
+        return fullPath;
+      } catch {
+        continue;
+      }
+    }
+  }
+  return undefined;
+}
+
 let bashLanguage: Language | null = null;
 let treeSitterInitialization: Promise<void> | null = null;
 let treeSitterInitializationError: Error | null = null;