Address review feedback for PR #359 (#361)

Author: data-douser

extractors/cds/tools/dist/cds-extractor.bundle.js

@@ -4,6 +4,7 @@ import { basename, delimiter, dirname, join, relative, resolve, sep } from 'path
 import { CdsCompilationResult } from './types';
 import { getCdsVersion } from './version';
 import { modelCdsJsonFile } from '../../constants';
+import { getPlatformInfo } from '../../environment';
 import {
   fileExists,
   dirExists,
@@ -248,19 +249,23 @@ function createSpawnOptions(
   cdsCommand: string,
   cacheDir?: string,
 ): SpawnSyncOptions {
-  const spawnOptions: SpawnSyncOptions = {
-    cwd: projectBaseDir, // CRITICAL: Always use project base directory as cwd to ensure correct path generation
-    shell: true, // Required on Windows where npm/npx are .cmd files
-    stdio: 'pipe',
-    env: { ...process.env },
-  };
-
   // Check if we're using a direct binary path (contains node_modules/.bin/ or node_modules\.bin\) or npx-style command
   // Check both platform-native separator and forward slash for cross-platform compatibility
   const binPathNative = `node_modules${sep}.bin${sep}`;
   const binPathPosix = 'node_modules/.bin/';
   const isDirectBinary = cdsCommand.includes(binPathNative) || cdsCommand.includes(binPathPosix);
 
+  // Only enable shell on Windows for npx-style commands where .cmd resolution is needed.
+  // Direct binary paths (node_modules/.bin/cds) don't need shell on any platform.
+  const useShell = getPlatformInfo().isWindows && !isDirectBinary;
+
+  const spawnOptions: SpawnSyncOptions = {
+    cwd: projectBaseDir, // CRITICAL: Always use project base directory as cwd to ensure correct path generation
+    shell: useShell,
+    stdio: 'pipe',
+    env: { ...process.env },
+  };
+
   // Only set up Node.js environment for npx-style commands, not for direct binary execution
   if (cacheDir && !isDirectBinary) {
     const nodePath = join(cacheDir, 'node_modules');

extractors/cds/tools/dist/cds-extractor.bundle.js.map

@@ -4,6 +4,7 @@ import { spawnSync } from 'child_process';
 import { delimiter, join } from 'path';
 
 import { addCdsIndexerDiagnostic } from '../diagnostics';
+import { npxExecutable } from '../environment';
 import { cdsExtractorLog } from '../logging';
 import { projectInstallDependencies } from '../packageManager';
 import type { CdsDependencyGraph, CdsProject } from './parser/types';
@@ -110,12 +111,11 @@ export function runCdsIndexer(
       `Running ${CDS_INDEXER_PACKAGE} for project '${project.projectDir}'...`,
     );
 
-    const spawnResult = spawnSync('npx', ['--yes', CDS_INDEXER_PACKAGE], {
+    const spawnResult = spawnSync(npxExecutable(), ['--yes', CDS_INDEXER_PACKAGE], {
       cwd: projectAbsPath,
       env,
       stdio: 'pipe',
       timeout: CDS_INDEXER_TIMEOUT_MS,
-      shell: true,
     });
 
     result.durationMs = Date.now() - startTime;

extractors/cds/tools/src/cds/compiler/compile.ts

@@ -48,6 +48,28 @@ export function getPlatformInfo(): PlatformInfo {
   };
 }
 
+/**
+ * Returns the platform-specific `npm` executable name.
+ * On Windows, npm is a `.cmd` script that requires the full name for
+ * `execFileSync`/`spawnSync` when `shell` is `false`.
+ *
+ * @returns `'npm.cmd'` on Windows, `'npm'` elsewhere.
+ */
+export function npmExecutable(): string {
+  return getPlatformInfo().isWindows ? 'npm.cmd' : 'npm';
+}
+
+/**
+ * Returns the platform-specific `npx` executable name.
+ * On Windows, npx is a `.cmd` script that requires the full name for
+ * `execFileSync`/`spawnSync` when `shell` is `false`.
+ *
+ * @returns `'npx.cmd'` on Windows, `'npx'` elsewhere.
+ */
+export function npxExecutable(): string {
+  return getPlatformInfo().isWindows ? 'npx.cmd' : 'npx';
+}
+
 /**
  * Get the path to the CodeQL executable.
  * Prioritizes CODEQL_DIST if set and valid. Otherwise, tries to find CodeQL via system PATH.

extractors/cds/tools/src/cds/indexer.ts

@@ -6,6 +6,7 @@ import { dirname, join, resolve } from 'path';
 import type { CdsDependencyCombination } from './types';
 import { CdsDependencyGraph, CdsProject } from '../cds/parser/types';
 import { DiagnosticSeverity } from '../diagnostics';
+import { npmExecutable } from '../environment';
 import { cdsExtractorLog } from '../logging';
 import { resolveCdsVersions } from './versionResolver';
 
@@ -494,10 +495,9 @@ function installDependenciesInCache(
   }
 
   try {
-    execFileSync('npm', ['install', '--quiet', '--no-audit', '--no-fund'], {
+    execFileSync(npmExecutable(), ['install', '--quiet', '--no-audit', '--no-fund'], {
       cwd: cacheDir,
       stdio: 'inherit',
-      shell: true,
     });
 
     // Add warning diagnostic if using fallback versions

extractors/cds/tools/src/environment.ts

@@ -3,6 +3,7 @@
 import { execFileSync } from 'child_process';
 import { join } from 'path';
 
+import { npmExecutable } from '../environment';
 import type { FullDependencyInstallationResult } from './types';
 import type { CdsProject } from '../cds/parser';
 import { cdsExtractorLog } from '../logging';
@@ -71,12 +72,15 @@ export function projectInstallDependencies(
     );
 
     try {
-      execFileSync('npm', ['install', '--quiet', '--no-audit', '--no-fund'], {
-        cwd: projectPath,
-        stdio: 'inherit',
-        timeout: 120000, // 2-minute timeout
-        shell: true,
-      });
+      execFileSync(
+        npmExecutable(),
+        ['install', '--ignore-scripts', '--quiet', '--no-audit', '--no-fund'],
+        {
+          cwd: projectPath,
+          stdio: 'inherit',
+          timeout: 120000, // 2-minute timeout
+        },
+      );
 
       result.success = true;
       cdsExtractorLog(

extractors/cds/tools/src/packageManager/cacheInstaller.ts

@@ -6,6 +6,8 @@ import * as path from 'path';
 
 import {
   getPlatformInfo,
+  npmExecutable,
+  npxExecutable,
   getCodeQLExePath,
   getJavaScriptExtractorRoot,
   setupJavaScriptExtractorEnv,
@@ -125,6 +127,40 @@ describe('environment', () => {
     });
   });
 
+  describe('npmExecutable', () => {
+    it('should return npm.cmd on Windows', () => {
+      (os.platform as jest.Mock).mockReturnValue('win32');
+      expect(npmExecutable()).toBe('npm.cmd');
+    });
+
+    it('should return npm on Linux', () => {
+      (os.platform as jest.Mock).mockReturnValue('linux');
+      expect(npmExecutable()).toBe('npm');
+    });
+
+    it('should return npm on macOS', () => {
+      (os.platform as jest.Mock).mockReturnValue('darwin');
+      expect(npmExecutable()).toBe('npm');
+    });
+  });
+
+  describe('npxExecutable', () => {
+    it('should return npx.cmd on Windows', () => {
+      (os.platform as jest.Mock).mockReturnValue('win32');
+      expect(npxExecutable()).toBe('npx.cmd');
+    });
+
+    it('should return npx on Linux', () => {
+      (os.platform as jest.Mock).mockReturnValue('linux');
+      expect(npxExecutable()).toBe('npx');
+    });
+
+    it('should return npx on macOS', () => {
+      (os.platform as jest.Mock).mockReturnValue('darwin');
+      expect(npxExecutable()).toBe('npx');
+    });
+  });
+
   describe('getCodeQLExePath', () => {
     it('should resolve codeql.exe path on Windows when CODEQL_DIST is set and valid', () => {
       // Setup platform

extractors/cds/tools/src/packageManager/projectInstaller.ts

@@ -64,12 +64,11 @@ describe('CDS Compiler Installer', () => {
 
       expect(mockExecFileSync).toHaveBeenCalledWith(
         'npm',
-        ['install', '--quiet', '--no-audit', '--no-fund'],
+        ['install', '--ignore-scripts', '--quiet', '--no-audit', '--no-fund'],
         {
           cwd: '/test/source/test-project',
           stdio: 'inherit',
           timeout: 120000,
-          shell: true,
         },
       );
     });

extractors/cds/tools/test/src/environment.test.ts

@@ -15,7 +15,8 @@
     "noImplicitReturns": true,
     "noFallthroughCasesInSwitch": true,
     "resolveJsonModule": true,
-    "skipLibCheck": true
+    "skipLibCheck": true,
+    "types": ["jest", "node"]
   },
   "include": [
     "*.ts",