Address review feedback: simplify error checking, reduce redundancy, prioritize .cmd

Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>

Author: Copilot

code/core/src/common/utils/command.test.ts

@@ -46,7 +46,7 @@ describe('command', () => {
       });
     });
 
-    it('should try .exe first for pnpm and succeed', async () => {
+    it('should try .cmd first for pnpm and succeed', async () => {
       mockedExeca.mockResolvedValueOnce({
         stdout: 'success',
         stderr: '',
@@ -59,7 +59,7 @@ describe('command', () => {
 
       expect(mockedExeca).toHaveBeenCalledTimes(1);
       expect(mockedExeca).toHaveBeenCalledWith(
-        'pnpm.exe',
+        'pnpm.cmd',
         ['--version'],
         expect.objectContaining({
           encoding: 'utf8',
@@ -68,12 +68,12 @@ describe('command', () => {
       );
     });
 
-    it('should try .cmd after .exe fails with "not recognized" error for pnpm', async () => {
+    it('should try .exe after .cmd fails with "not recognized" error for pnpm', async () => {
       // First call fails with "not recognized" error
       mockedExeca.mockRejectedValueOnce({
         stderr:
-          "'pnpm.exe' is not recognized as an internal or external command,\r\noperable program or batch file.",
-        message: 'Command failed: pnpm.exe --version',
+          "'pnpm.cmd' is not recognized as an internal or external command,\r\noperable program or batch file.",
+        message: 'Command failed: pnpm.cmd --version',
       });
 
       // Second call succeeds
@@ -88,19 +88,19 @@ describe('command', () => {
       });
 
       expect(mockedExeca).toHaveBeenCalledTimes(2);
-      expect(mockedExeca).toHaveBeenNthCalledWith(1, 'pnpm.exe', ['--version'], expect.anything());
-      expect(mockedExeca).toHaveBeenNthCalledWith(2, 'pnpm.cmd', ['--version'], expect.anything());
+      expect(mockedExeca).toHaveBeenNthCalledWith(1, 'pnpm.cmd', ['--version'], expect.anything());
+      expect(mockedExeca).toHaveBeenNthCalledWith(2, 'pnpm.exe', ['--version'], expect.anything());
     });
 
-    it('should try .ps1 after .cmd fails with "not recognized" error for pnpm', async () => {
+    it('should try .ps1 after .exe fails with "not recognized" error for pnpm', async () => {
       // First two calls fail with "not recognized" error
       mockedExeca.mockRejectedValueOnce({
-        stderr: "'pnpm.exe' is not recognized as an internal or external command",
+        stderr: "'pnpm.cmd' is not recognized as an internal or external command",
         message: 'Command failed',
       });
 
       mockedExeca.mockRejectedValueOnce({
-        stderr: "'pnpm.cmd' is not recognized as an internal or external command",
+        stderr: "'pnpm.exe' is not recognized as an internal or external command",
         message: 'Command failed',
       });
 
@@ -116,20 +116,20 @@ describe('command', () => {
       });
 
       expect(mockedExeca).toHaveBeenCalledTimes(3);
-      expect(mockedExeca).toHaveBeenNthCalledWith(1, 'pnpm.exe', ['--version'], expect.anything());
-      expect(mockedExeca).toHaveBeenNthCalledWith(2, 'pnpm.cmd', ['--version'], expect.anything());
+      expect(mockedExeca).toHaveBeenNthCalledWith(1, 'pnpm.cmd', ['--version'], expect.anything());
+      expect(mockedExeca).toHaveBeenNthCalledWith(2, 'pnpm.exe', ['--version'], expect.anything());
       expect(mockedExeca).toHaveBeenNthCalledWith(3, 'pnpm.ps1', ['--version'], expect.anything());
     });
 
     it('should try bare command after all extensions fail with "not recognized" error', async () => {
       // First three calls fail with "not recognized" error
       mockedExeca.mockRejectedValueOnce({
-        stderr: "'pnpm.exe' is not recognized as an internal or external command",
+        stderr: "'pnpm.cmd' is not recognized as an internal or external command",
         message: 'Command failed',
       });
 
       mockedExeca.mockRejectedValueOnce({
-        stderr: "'pnpm.cmd' is not recognized as an internal or external command",
+        stderr: "'pnpm.exe' is not recognized as an internal or external command",
         message: 'Command failed',
       });
 
@@ -201,7 +201,7 @@ describe('command', () => {
         args: ['--version'],
       });
 
-      expect(mockedExeca).toHaveBeenCalledWith('npm.exe', ['--version'], expect.anything());
+      expect(mockedExeca).toHaveBeenCalledWith('npm.cmd', ['--version'], expect.anything());
     });
 
     it('should work for yarn command', async () => {
@@ -215,7 +215,7 @@ describe('command', () => {
         args: ['--version'],
       });
 
-      expect(mockedExeca).toHaveBeenCalledWith('yarn.exe', ['--version'], expect.anything());
+      expect(mockedExeca).toHaveBeenCalledWith('yarn.cmd', ['--version'], expect.anything());
     });
 
     it('should not modify unknown commands on Windows', async () => {
@@ -301,7 +301,7 @@ describe('command', () => {
       });
     });
 
-    it('should try .exe first for pnpm and succeed', () => {
+    it('should try .cmd first for pnpm and succeed', () => {
       mockedExecaCommandSync.mockReturnValueOnce({
         stdout: '10.0.0',
         stderr: '',
@@ -315,19 +315,19 @@ describe('command', () => {
       expect(result).toBe('10.0.0');
       expect(mockedExecaCommandSync).toHaveBeenCalledTimes(1);
       expect(mockedExecaCommandSync).toHaveBeenCalledWith(
-        'pnpm.exe --version',
+        'pnpm.cmd --version',
         expect.objectContaining({
           encoding: 'utf8',
           cleanup: true,
         })
       );
     });
 
-    it('should try .cmd after .exe fails with "not recognized" error', () => {
+    it('should try .exe after .cmd fails with "not recognized" error', () => {
       // First call fails
       mockedExecaCommandSync.mockImplementationOnce(() => {
         throw {
-          stderr: "'pnpm.exe' is not recognized as an internal or external command",
+          stderr: "'pnpm.cmd' is not recognized as an internal or external command",
           message: 'Command failed',
         };
       });
@@ -347,12 +347,12 @@ describe('command', () => {
       expect(mockedExecaCommandSync).toHaveBeenCalledTimes(2);
       expect(mockedExecaCommandSync).toHaveBeenNthCalledWith(
         1,
-        'pnpm.exe --version',
+        'pnpm.cmd --version',
         expect.anything()
       );
       expect(mockedExecaCommandSync).toHaveBeenNthCalledWith(
         2,
-        'pnpm.cmd --version',
+        'pnpm.exe --version',
         expect.anything()
       );
     });

code/core/src/common/utils/command.ts

@@ -105,14 +105,13 @@ function isCommandNotFoundError(error: any): boolean {
     return false;
   }
 
-  // Check for Windows-specific "command not recognized" error
   const stderr = error.stderr || '';
-  const message = error.message || '';
+  return stderr.includes('is not recognized as an internal or external command');
+}
 
-  return (
-    stderr.includes('is not recognized as an internal or external command') ||
-    message.includes('is not recognized as an internal or external command')
-  );
+/** Helper to check if we should continue trying command variations. */
+function shouldRetry(error: any, isLastVariation: boolean): boolean {
+  return isCommandNotFoundError(error) && !isLastVariation;
 }
 
 /**
@@ -142,12 +141,10 @@ function tryCommandVariations(
     } catch (error: any) {
       lastError = error;
 
-      // If this is not a "command not found" error, or we're on the last variation, re-throw
-      if (!isCommandNotFoundError(error) || index === commandVariations.length - 1) {
+      if (!shouldRetry(error, index === commandVariations.length - 1)) {
         throw error;
       }
 
-      // Otherwise, try the next variation
       logger.debug(`Command "${cmd}" not found, trying next variation...`);
       return tryNext(index + 1);
     }
@@ -179,17 +176,14 @@ function tryCommandVariationsSync(
     } catch (error: any) {
       lastError = error;
 
-      // If this is not a "command not found" error, or we're on the last variation, re-throw
-      if (!isCommandNotFoundError(error) || i === commandVariations.length - 1) {
+      if (!shouldRetry(error, i === commandVariations.length - 1)) {
         throw error;
       }
 
-      // Otherwise, try the next variation
       logger.debug(`Command "${cmd}" not found, trying next variation...`);
     }
   }
 
-  // This should never be reached, but TypeScript needs it
   throw lastError;
 }
 
@@ -213,8 +207,8 @@ function tryCommandVariationsSync(
  *
  * - If on Windows:
  *
- *   - For known shim-based commands, return an array of variations to try in order: [command.exe,
- *       command.cmd, command.ps1, command]
+ *   - For known shim-based commands, return an array of variations to try in order: [command.cmd,
+ *       command.exe, command.ps1, command]
  *   - For everything else, return the name unchanged.
  * - On non-Windows, return command unchanged.
  *
@@ -243,12 +237,7 @@ function resolveCommand(command: string): string[] {
   }
 
   if (WINDOWS_SHIM_COMMANDS.has(command)) {
-    // On Windows, try multiple variations in order of likelihood:
-    // 1. .exe - native executable (e.g., pnpm installed via Scoop/Mise)
-    // 2. .cmd - CMD shim (most common for npm-installed packages)
-    // 3. .ps1 - PowerShell shim (less common but possible)
-    // 4. bare command - fallback
-    return [`${command}.exe`, `${command}.cmd`, `${command}.ps1`, command];
+    return [`${command}.cmd`, `${command}.exe`, `${command}.ps1`, command];
   }
 
   return [command];