refactor: replace execFile and spawn with execCli and spawnCli in CLI and TeamProvisioning services

- Updated CliInstallerService and TeamProvisioningService to use execCli and spawnCli for improved error handling and compatibility, particularly on Windows. - Enhanced child process utility functions to better manage non-ASCII paths and provide consistent behavior across different platforms. - Adjusted tests to mock new child process utilities and verify correct usage in service methods.
2026-02-27 19:59:26 +02:00 · 2026-02-27 19:59:26 +02:00 · 69adf3488e
commit 69adf3488e
parent d948cc6fb4
5 changed files with 164 additions and 65 deletions
--- a/src/main/services/infrastructure/CliInstallerService.ts
+++ b/src/main/services/infrastructure/CliInstallerService.ts
@ -17,17 +17,16 @@
 * - Human-readable error messages per phase
 */

+import { execCli, spawnCli } from '@main/utils/childProcess';
 import { getHomeDir } from '@main/utils/pathDecoder';
 import { getErrorMessage } from '@shared/utils/errorHandling';
 import { createLogger } from '@shared/utils/logger';
-import { execFile, spawn } from 'child_process';
 import { createHash } from 'crypto';
 import { createWriteStream, existsSync, promises as fsp } from 'fs';
 import http from 'http';
 import https from 'https';
 import { tmpdir } from 'os';
 import { join } from 'path';
-import { promisify } from 'util';

 import { ClaudeBinaryResolver } from '../team/ClaudeBinaryResolver';

@ -37,10 +36,6 @@ import type { IncomingMessage } from 'http';

 const logger = createLogger('CliInstallerService');

-// Note: execFile (not exec) is used intentionally — no shell injection risk.
-// Arguments are passed as arrays, never interpolated into shell strings.
-const execFileAsync = promisify(execFile);
-
 // =============================================================================
 // Constants
 // =============================================================================
@ -222,7 +217,7 @@ export class CliInstallerService {
      result.binaryPath = binaryPath;

      try {
-        const { stdout } = await execFileAsync(binaryPath, ['--version'], {
+        const { stdout } = await execCli(binaryPath, ['--version'], {
          timeout: VERSION_TIMEOUT_MS,
          env: buildChildEnv(),
        });
@ -236,7 +231,7 @@ export class CliInstallerService {

      // Check auth status
      try {
-        const { stdout: authStdout } = await execFileAsync(binaryPath, ['auth', 'status'], {
+        const { stdout: authStdout } = await execCli(binaryPath, ['auth', 'status'], {
          timeout: VERSION_TIMEOUT_MS,
          env: buildChildEnv(),
        });
@ -466,7 +461,7 @@ export class CliInstallerService {
   */
  private async runInstallWithStreaming(binaryPath: string, attempt = 1): Promise<void> {
    return new Promise<void>((resolve, reject) => {
-      const child = spawn(binaryPath, ['install'], {
+      const child = spawnCli(binaryPath, ['install'], {
        env: { ...buildChildEnv(), CLAUDE_SKIP_ANALYTICS: '1' },
        stdio: ['ignore', 'pipe', 'pipe'],
      });
--- a/src/main/services/team/TeamProvisioningService.ts
+++ b/src/main/services/team/TeamProvisioningService.ts
@ -1,5 +1,6 @@
 /* eslint-disable no-param-reassign -- ProvisioningRun object is intentionally mutated as a state tracker throughout the provisioning lifecycle */
 import { ConfigManager } from '@main/services/infrastructure/ConfigManager';
+import { spawnCli } from '@main/utils/childProcess';
 import {
  encodePath,
  extractBaseDir,
@ -930,7 +931,7 @@ export class TeamProvisioningService {
      );
    }
    try {
-      child = spawn(
+      child = spawnCli(
        claudePath,
        [
          '--input-format',
@ -1252,7 +1253,7 @@ export class TeamProvisioningService {
    // --resume is for existing sessions and may show an interactive picker if not found.

    try {
-      child = spawn(claudePath, launchArgs, {
+      child = spawnCli(claudePath, launchArgs, {
        cwd: request.cwd,
        env: {
          ...shellEnv,
@ -3127,7 +3128,7 @@ export class TeamProvisioningService {
    timeoutMs: number
  ): Promise<{ exitCode: number | null; stdout: string; stderr: string }> {
    return new Promise((resolve, reject) => {
-      const child = spawn(claudePath, args, {
+      const child = spawnCli(claudePath, args, {
        cwd,
        env,
        stdio: ['ignore', 'pipe', 'pipe'],
--- a/src/main/utils/childProcess.ts
+++ b/src/main/utils/childProcess.ts
@ -1,15 +1,59 @@
-import { spawn, execFile, exec, SpawnOptions, ExecFileOptions } from 'child_process';
-import { promisify } from 'util';
+import {
+  exec,
+  execFile,
+  type ExecFileOptions,
+  type ExecOptions,
+  spawn,
+  type SpawnOptions,
+} from 'child_process';

-// re-exported helpers used throughout the codebase
-export const execFileAsync = promisify(execFile);
-export const execAsync = promisify(exec);
+/**
+ * Promise wrapper for execFile that always returns { stdout, stderr }.
+ * Unlike promisify(execFile), this works correctly with mocked execFile
+ * (promisify relies on a custom symbol that mocks don't have).
+ */
+function execFileAsync(
+  cmd: string,
+  args: string[],
+  options: ExecFileOptions = {}
+): Promise<{ stdout: string; stderr: string }> {
+  return new Promise((resolve, reject) => {
+    execFile(cmd, args, options, (err, stdout, stderr) => {
+      if (err)
+        reject(
+          err instanceof Error ? err : new Error(typeof err === 'string' ? err : 'Unknown error')
+        );
+      else resolve({ stdout: String(stdout), stderr: String(stderr) });
+    });
+  });
+}
+
+/**
+ * Promise wrapper for exec.  Used exclusively as a Windows shell fallback
+ * when execFile fails with EINVAL on non-ASCII binary paths.  The command
+ * string is built from a known binary path + args, NOT from user input.
+ */
+function execShellAsync(
+  cmd: string,
+  options: ExecOptions = {}
+): Promise<{ stdout: string; stderr: string }> {
+  return new Promise((resolve, reject) => {
+    // eslint-disable-next-line sonarjs/os-command, security/detect-child-process -- cmd from known binaryPath+args, not user input (Windows EINVAL fallback)
+    exec(cmd, options, (err, stdout, stderr) => {
+      if (err)
+        reject(
+          err instanceof Error ? err : new Error(typeof err === 'string' ? err : 'Unknown error')
+        );
+      else resolve({ stdout: String(stdout), stderr: String(stderr) });
+    });
+  });
+}

 /**
 * Returns true if the string contains any non-ASCII character.
 */
 function containsNonAscii(str: string): boolean {
-  return /[^\x00-\x7F]/.test(str);
+  return [...str].some((c) => c.charCodeAt(0) > 127);
 }

 /**
@ -30,7 +74,7 @@ function needsShell(binaryPath: string): boolean {
 * the shell itself will handle most quoting rules.
 */
 function quoteArg(arg: string): string {
-  if (/[^A-Za-z0-9_\-\/.]/.test(arg)) {
+  if (/[^A-Za-z0-9_\-/.]/.test(arg)) {
    return `"${arg.replace(/"/g, '\\"')}"`;
  }
  return arg;
@ -56,10 +100,14 @@ export async function execCli(
    try {
      const result = await execFileAsync(target, args, options);
      return { stdout: String(result.stdout), stderr: String(result.stderr) };
-    } catch (err: any) {
+    } catch (err: unknown) {
      // fall through to shell fallback only when the error matches the
      // Windows "invalid argument" problem; otherwise rethrow.
-      if (!(err && err.code === 'EINVAL')) {
+      const code =
+        err && typeof err === 'object' && 'code' in err
+          ? (err as { code?: string }).code
+          : undefined;
+      if (code !== 'EINVAL') {
        throw err;
      }
    }
@ -67,7 +115,7 @@ export async function execCli(

  // shell fallback (Windows only; others shouldn't reach here)
  const cmd = [target, ...args].map(quoteArg).join(' ');
-  const shellResult = await execAsync(cmd, options as unknown as import('child_process').ExecOptions);
+  const shellResult = await execShellAsync(cmd, options as unknown as ExecOptions);
  return { stdout: String(shellResult.stdout), stderr: String(shellResult.stderr) };
 }

@ -81,17 +129,21 @@ export function spawnCli(
  binaryPath: string,
  args: string[],
  options: SpawnOptions = {}
-) {
+): ReturnType<typeof spawn> {
  if (process.platform === 'win32' && needsShell(binaryPath)) {
    const cmd = [binaryPath, ...args].map(quoteArg).join(' ');
+    // eslint-disable-next-line sonarjs/os-command -- cmd from known binaryPath+args, not user input (Windows EINVAL fallback)
    return spawn(cmd, { shell: true, ...options });
  }

  try {
    return spawn(binaryPath, args, options);
-  } catch (err: any) {
-    if (process.platform === 'win32' && err && err.code === 'EINVAL') {
+  } catch (err: unknown) {
+    const code =
+      err && typeof err === 'object' && 'code' in err ? (err as { code?: string }).code : undefined;
+    if (process.platform === 'win32' && code === 'EINVAL') {
      const cmd = [binaryPath, ...args].map(quoteArg).join(' ');
+      // eslint-disable-next-line sonarjs/os-command -- cmd from known binaryPath+args, not user input (Windows EINVAL fallback)
      return spawn(cmd, { shell: true, ...options });
    }
    throw err;
--- a/test/main/services/infrastructure/CliInstallerService.test.ts
+++ b/test/main/services/infrastructure/CliInstallerService.test.ts
@ -1,11 +1,11 @@
 import { beforeEach, describe, expect, it, vi } from 'vitest';

 // Mock dependencies before importing service
-vi.mock('child_process', async (importOriginal) => {
-  const actual = await importOriginal<typeof import('child_process')>();
+vi.mock('@main/utils/childProcess', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('@main/utils/childProcess')>();
  return {
    ...actual,
-    execFile: vi.fn(),
+    execCli: vi.fn().mockRejectedValue(new Error('execCli not configured')),
  };
 });

@ -63,6 +63,7 @@ import {
  normalizeVersion,
 } from '@main/services/infrastructure/CliInstallerService';
 import { ClaudeBinaryResolver } from '@main/services/team/ClaudeBinaryResolver';
+import { execCli } from '@main/utils/childProcess';

 /**
 * Helper: allow expected console.error/warn calls in tests where service logs errors.
@ -111,23 +112,20 @@ describe('CliInstallerService', () => {
      const fakePath = 'C:\\Users\\Алексей\\AppData\\Roaming\\npm\\claude.cmd';
      vi.mocked(ClaudeBinaryResolver.resolve).mockResolvedValue(fakePath);

-      // mock execFile to throw EINVAL first
-      const err: any = new Error('spawn EINVAL');
-      err.code = 'EINVAL';
-      const childProcess = await import('child_process');
-      vi.spyOn(childProcess, 'execFile').mockImplementation((cmd, args, opts, cb) => {
-        cb(err, '', '');
-        return {} as any;
-      });
-      // mock exec to succeed as fallback
-      vi.spyOn(childProcess, 'exec').mockImplementation((cmd, opts, cb) => {
-        cb(null, '2.3.4', '');
-        return {} as any;
-      });
+      // execCli handles the EINVAL → shell fallback internally;
+      // here we just verify the service delegates to execCli correctly.
+      vi.mocked(execCli)
+        .mockResolvedValueOnce({ stdout: '2.3.4', stderr: '' }) // --version
+        .mockResolvedValueOnce({ stdout: '{}', stderr: '' }); // auth status

      const status = await service.getStatus();
      expect(status.installed).toBe(true);
      expect(status.installedVersion).toBe('2.3.4');
+      expect(execCli).toHaveBeenCalledWith(
+        fakePath,
+        ['--version'],
+        expect.objectContaining({ timeout: expect.any(Number) })
+      );
    });
  });

--- a/test/main/utils/childProcess.test.ts
+++ b/test/main/utils/childProcess.test.ts
@ -1,12 +1,17 @@
+// @vitest-environment node
 import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';

 // Mock the entire child_process module so that we can inspect how our helpers
 // invoke spawn/exec without hitting the real filesystem or spawning anything.
-vi.mock('child_process', () => ({
-  spawn: vi.fn(),
-  execFile: vi.fn(),
-  exec: vi.fn(),
-}));
+vi.mock('child_process', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('child_process')>();
+  return {
+    ...actual,
+    spawn: vi.fn(),
+    execFile: vi.fn(),
+    exec: vi.fn(),
+  };
+});

 // Import after the mock call so that the mocked module is returned.
 import * as child from 'child_process';
@ -16,6 +21,8 @@ import { spawnCli, execCli } from '@main/utils/childProcess';
 function setPlatform(value: string) {
  Object.defineProperty(process, 'platform', {
    value,
+    configurable: true,
+    writable: true,
  });
 }

@ -52,13 +59,31 @@ describe('cli child process helpers', () => {
      });
      spawnMock.mockImplementationOnce(() => fake);

-      const result = spawnCli('C:\\Users\\Àëåêñåé\\AppData\\Roaming\\npm\\claude.cmd', ['a', 'b'], {
+      // Use ASCII path so needsShell returns false and we go through the try/catch EINVAL path
+      const result = spawnCli('C:\\bin\\claude.exe', ['a', 'b'], {
        env: { FOO: 'bar' },
      });
      expect(spawnMock).toHaveBeenCalledTimes(2);
      const secondArg0 = spawnMock.mock.calls[1][0] as string;
-      expect(secondArg0).toMatch(/claude\.cmd/);
-      expect(spawnMock.mock.calls[1][2]).toMatchObject({ shell: true, env: { FOO: 'bar' } });
+      expect(secondArg0).toMatch(/claude\.exe/);
+      expect(spawnMock.mock.calls[1][1]).toMatchObject({ shell: true, env: { FOO: 'bar' } });
+      expect(result).toBe(fake);
+    });
+
+    it('uses shell directly when path contains non-ASCII on windows', () => {
+      setPlatform('win32');
+      const fake = {} as any;
+      const spawnMock = child.spawn as unknown as vi.Mock;
+      spawnMock.mockReturnValue(fake);
+
+      const result = spawnCli('C:\\Users\\Алексей\\AppData\\Roaming\\npm\\claude.cmd', ['a', 'b'], {
+        env: { FOO: 'bar' },
+      });
+      // Non-ASCII detected upfront — single spawn call with shell: true
+      expect(spawnMock).toHaveBeenCalledTimes(1);
+      const shellCmd = spawnMock.mock.calls[0][0] as string;
+      expect(shellCmd).toMatch(/claude\.cmd/);
+      expect(spawnMock.mock.calls[0][1]).toMatchObject({ shell: true, env: { FOO: 'bar' } });
      expect(result).toBe(fake);
    });

@ -72,37 +97,65 @@ describe('cli child process helpers', () => {
  });

  describe('execCli', () => {
-    it('invokes execFile when path is normal', async () => {
+    it('invokes execFile when path is ASCII on windows', async () => {
      setPlatform('win32');
      const execFileMock = child.execFile as unknown as vi.Mock;
-      execFileMock.mockImplementation((cmd, args, opts, cb) => {
-        cb(null, 'ok', '');
-        return {} as any;
-      });
+      execFileMock.mockImplementation(
+        (_cmd: string, _args: string[], _opts: unknown, cb: Function) => {
+          cb(null, 'ok', '');
+          return {} as any;
+        }
+      );
      const result = await execCli('C:\\bin\\claude.exe', ['--version']);
-      expect(execFileMock).toHaveBeenCalledWith('C:\\bin\\claude.exe', ['--version'], {}, expect.any(Function));
+      expect(execFileMock).toHaveBeenCalledWith(
+        'C:\\bin\\claude.exe',
+        ['--version'],
+        {},
+        expect.any(Function)
+      );
      expect(result.stdout).toBe('ok');
    });

-    it('falls back to exec shell when execFile throws EINVAL or path contains non-ascii', async () => {
+    it('skips straight to shell when path contains non-ASCII on windows', async () => {
      setPlatform('win32');
      const execFileMock = child.execFile as unknown as vi.Mock;
-      execFileMock.mockImplementation((cmd, args, opts, cb) => {
-        const err: any = new Error('spawn EINVAL');
-        err.code = 'EINVAL';
-        cb(err, '', '');
-        return {} as any;
-      });
      const execMock = child.exec as unknown as vi.Mock;
-      execMock.mockImplementation((cmd, opts, cb) => {
+      execMock.mockImplementation((_cmd: string, _opts: unknown, cb: Function) => {
        cb(null, '1.2.3', '');
        return {} as any;
      });

-      const result = await execCli('C:\\Users\\Àëåêñåé\\AppData\\Roaming\\npm\\claude.cmd', ['--version']);
-      expect(execFileMock).toHaveBeenCalled();
+      const result = await execCli('C:\\Users\\Алексей\\AppData\\Roaming\\npm\\claude.cmd', [
+        '--version',
+      ]);
+      // non-ASCII path detected upfront — execFile should NOT be called
+      expect(execFileMock).not.toHaveBeenCalled();
      expect(execMock).toHaveBeenCalled();
      expect(result.stdout).toBe('1.2.3');
    });
+
+    it('falls back to shell when execFile throws EINVAL on windows', async () => {
+      setPlatform('win32');
+      const execFileMock = child.execFile as unknown as vi.Mock;
+      execFileMock.mockImplementation(
+        (_cmd: string, _args: string[], _opts: unknown, cb: Function) => {
+          const err: any = new Error('spawn EINVAL');
+          err.code = 'EINVAL';
+          cb(err, '', '');
+          return {} as any;
+        }
+      );
+      const execMock = child.exec as unknown as vi.Mock;
+      execMock.mockImplementation((_cmd: string, _opts: unknown, cb: Function) => {
+        cb(null, '2.3.4', '');
+        return {} as any;
+      });
+
+      // ASCII path — goes through execFile first, gets EINVAL, falls back to shell
+      const result = await execCli('C:\\bin\\claude.exe', ['--version']);
+      expect(execFileMock).toHaveBeenCalled();
+      expect(execMock).toHaveBeenCalled();
+      expect(result.stdout).toBe('2.3.4');
+    });
  });
 });