fix(node-module-collector): suppress expected npm list stderr from user-visible warnings when collecting dependencies for Yarn projects using resolutions (#9810)

Author: mmaietta

.changeset/many-lamps-fix.md

@@ -0,0 +1,5 @@
+---
+"app-builder-lib": patch
+---
+
+fix(node-module-collector): suppress expected `npm list` stderr from user-visible warnings when collecting dependencies for Yarn projects using resolutions

packages/app-builder-lib/src/node-module-collector/nodeModulesCollector.ts

@@ -357,29 +357,15 @@ export abstract class NodeModulesCollector<ProdDepType extends Dependency<ProdDe
    * @throws {Error} If the child process spawn fails or exits with a non-zero code
    */
   protected async streamCollectorCommandToFile(command: string, args: string[], cwd: string, tempOutputFile: string) {
+    // Capture execName from the original command before resolveWindowsCommand may rewrite it to
+    // cmd.exe — shouldIgnore must key off the original invocation (e.g. "npm", not "cmd").
     const execName = path.basename(command, path.extname(command))
-    const ext = path.extname(command).toLowerCase()
-    // Wrap .cmd files in a .bat file for correct cmd.exe execution.
-    // Also wrap any Windows executable path that contains spaces (e.g. extensionless shims
-    // from tools like Volta installed at "C:\Program Files\Volta\") to ensure the path is
-    // quoted correctly when passed to `spawn(..., { shell: true })`.
-    const isWindowsScriptFile = process.platform === "win32" && (ext === ".cmd" || command.includes(" "))
-    if (isWindowsScriptFile) {
-      // We need to wrap it in a .bat file to ensure it runs correctly with cmd.exe
-      // This is necessary because some files (like .cmd) are not directly executable in the same way as .bat files.
-      // We create a temporary .bat file that calls the script-like file with the provided arguments. The .bat file will be executed by cmd.exe.
-      // Note: This is a workaround for Windows command execution quirks when using `shell: true`
-      const tempBatFile = await this.tempDirManager.getTempFile({
-        prefix: execName + "-" + randomBytes(8).toString("hex"),
-        suffix: ".bat",
-      })
-      const escapedCommand = command.replace(/"/g, `""`)
-      const batScript = `@echo off\r\n"${escapedCommand}" %*\r\n` // <-- CRLF required for .bat
-      await fs.writeFile(tempBatFile, batScript, { encoding: "utf8", mode: 0o600 })
-      command = "cmd.exe"
-      args = ["/c", `"${tempBatFile}"`, ...args]
-    }
 
+    if (process.platform === "win32") {
+      const resolved = await this.resolveWindowsCommand(command, args)
+      command = resolved.command
+      args = resolved.args
+    }
     const isWindows = process.platform === "win32"
 
     // shell: true is required on Windows — see https://github.com/electron-userland/electron-builder/issues/9488
@@ -424,11 +410,47 @@ export abstract class NodeModulesCollector<ProdDepType extends Dependency<ProdDe
         }
         if (stderr.length > 0) {
           log.debug({ stderr }, "note: there was node module collector output on stderr")
-          this.cache.logSummary[LogMessageByKey.PKG_COLLECTOR_OUTPUT].push(stderr)
+          // Only surface stderr as a user-visible warning when the exit code itself is unexpected.
+          // When shouldIgnore is true (npm list exit code 1) the stderr is an anticipated side
+          // effect of package-manager features like yarn resolutions or npm overrides that cause
+          // npm to report ELSPROBLEMS for aliased packages it considers "invalid".
+          if (!shouldIgnore) {
+            this.cache.logSummary[LogMessageByKey.PKG_COLLECTOR_OUTPUT].push(stderr)
+          }
         }
         const shouldResolve = code === 0 || shouldIgnore
         return shouldResolve ? resolve() : reject(new Error(`Node module collector process exited with code ${code}:\n${stderr}`))
       })
     })
   }
+
+  /**
+   * Windows-only. Wraps .cmd files and executables at paths containing spaces in a temporary .bat
+   * file so that cmd.exe spawns them correctly. Returns the original command/args unchanged when
+   * no wrapping is required. Must only be called on win32.
+   */
+  private async resolveWindowsCommand(command: string, args: string[]): Promise<{ command: string; args: string[] }> {
+    if (process.platform !== "win32") {
+      throw new Error("resolveWindowsCommand should only be called on Windows platforms")
+    }
+
+    const ext = path.extname(command).toLowerCase()
+    // Wrap .cmd files and any Windows executable path that contains spaces (e.g. extensionless
+    // shims from tools like Volta installed at "C:\Program Files\Volta\") to ensure the path is
+    // quoted correctly when passed to `spawn(..., { shell: true })`.
+    const needsWrap = ext === ".cmd" || command.includes(" ")
+    if (!needsWrap) {
+      return { command, args }
+    }
+
+    const execName = path.basename(command, ext)
+    const tempBatFile = await this.tempDirManager.getTempFile({
+      prefix: execName + "-" + randomBytes(8).toString("hex"),
+      suffix: ".bat",
+    })
+    const escapedCommand = command.replace(/"/g, `""`)
+    const batScript = `@echo off\r\n"${escapedCommand}" %*\r\n` // <-- CRLF required for .bat
+    await fs.writeFile(tempBatFile, batScript, { encoding: "utf8", mode: 0o600 })
+    return { command: "cmd.exe", args: ["/c", `"${tempBatFile}"`, ...args] }
+  }
 }

test/src/node-module-collector/streamCollectorTest.ts

@@ -1,5 +1,6 @@
 import { afterEach, beforeEach, describe, test, vi } from "vitest"
 import { NodeModulesCollector } from "app-builder-lib/src/node-module-collector/nodeModulesCollector"
+import { LogMessageByKey } from "app-builder-lib/src/node-module-collector/moduleManager"
 import { PM } from "app-builder-lib/src/node-module-collector/packageManager"
 import * as childProcess from "child_process"
 import * as fsExtra from "fs-extra"
@@ -19,10 +20,13 @@ class TestCollector extends NodeModulesCollector<any, any> {
   }
   protected async extractProductionDependencyGraph() {}
   protected async collectAllDependencies() {}
-  // expose protected method for testing
+  // expose protected members for testing
   streamCollectorCommandToFile(command: string, args: string[], cwd: string, tempOutputFile: string) {
     return super.streamCollectorCommandToFile(command, args, cwd, tempOutputFile)
   }
+  get logSummary() {
+    return this.cache.logSummary
+  }
 }
 
 const BAT_PATH = "C:\\Temp\\pnpm-deadbeef.bat"
@@ -36,6 +40,7 @@ function setPlatform(p: NodeJS.Platform) {
 
 let collector: TestCollector
 let closeCb: ((code: number) => void) | undefined
+let stderrDataCb: ((chunk: string) => void) | undefined
 
 async function waitForCloseCb() {
   for (let i = 0; i < 50 && closeCb === undefined; i++) {
@@ -48,9 +53,14 @@ async function waitForCloseCb() {
 
 beforeEach(() => {
   closeCb = undefined
+  stderrDataCb = undefined
   const mockChild = {
     stdout: { pipe: vi.fn() },
-    stderr: { on: vi.fn() },
+    stderr: {
+      on: vi.fn((ev: string, cb: (chunk: string) => void) => {
+        if (ev === "data") stderrDataCb = cb
+      }),
+    },
     on: vi.fn((ev: string, cb: (code: number) => void) => {
       if (ev === "close") closeCb = cb
     }),
@@ -142,6 +152,7 @@ describe.sequential("streamCollectorCommandToFile", () => {
       setPlatform("win32")
       const cmd = "C:\\tools\\pnpm.exe"
       const p = collector.streamCollectorCommandToFile(cmd, [], "/cwd", OUTPUT_FILE)
+      await waitForCloseCb()
       closeCb!(0)
       await p
       expect(vi.mocked(childProcess.spawn).mock.calls[0][0]).toBe(cmd)
@@ -151,6 +162,7 @@ describe.sequential("streamCollectorCommandToFile", () => {
     test("extensionless at path without spaces: spawn receives original command", async ({ expect }) => {
       setPlatform("win32")
       const p = collector.streamCollectorCommandToFile("pnpm", [], "/cwd", OUTPUT_FILE)
+      await waitForCloseCb()
       closeCb!(0)
       await p
       expect(vi.mocked(childProcess.spawn).mock.calls[0][0]).toBe("pnpm")
@@ -161,10 +173,72 @@ describe.sequential("streamCollectorCommandToFile", () => {
       setPlatform("darwin")
       const cmd = "/Applications/Volta/pnpm.exe"
       const p = collector.streamCollectorCommandToFile(cmd, [], "/cwd", OUTPUT_FILE)
+      await waitForCloseCb()
       closeCb!(0)
       await p
       expect(vi.mocked(childProcess.spawn).mock.calls[0][0]).toBe(cmd)
       expect(vi.mocked(fsExtra.writeFile)).not.toHaveBeenCalled()
     })
   })
+
+  describe("stderr and exit code behavior", () => {
+    test("npm list exit code 1: stderr is NOT surfaced as a collector warning", async ({ expect }) => {
+      const p = collector.streamCollectorCommandToFile("npm", ["list", "--json"], "/cwd", OUTPUT_FILE)
+      await waitForCloseCb()
+      stderrDataCb?.("npm error code ELSPROBLEMS\nnpm error invalid: canvas@npm:npm-empty-stub@1.0.1\n")
+      closeCb!(1)
+      await p
+      expect(collector.logSummary[LogMessageByKey.PKG_COLLECTOR_OUTPUT]).toHaveLength(0)
+    })
+
+    test("npm list exit code 0 with stderr: stderr IS surfaced as a collector warning", async ({ expect }) => {
+      const p = collector.streamCollectorCommandToFile("npm", ["list", "--json"], "/cwd", OUTPUT_FILE)
+      await waitForCloseCb()
+      stderrDataCb?.("npm warn some unexpected warning\n")
+      closeCb!(0)
+      await p
+      const output = collector.logSummary[LogMessageByKey.PKG_COLLECTOR_OUTPUT]
+      expect(output).toHaveLength(1)
+      expect(output[0]).toContain("npm warn some unexpected warning")
+    })
+
+    test("non-npm command exit code 1: rejects with stderr in error message", async ({ expect }) => {
+      const p = collector.streamCollectorCommandToFile("pnpm", ["list", "--json"], "/cwd", OUTPUT_FILE)
+      await waitForCloseCb()
+      stderrDataCb?.("pnpm error: something went wrong\n")
+      closeCb!(1)
+      await expect(p).rejects.toThrow("pnpm error: something went wrong")
+    })
+
+    test("npm command (not list) exit code 1: rejects", async ({ expect }) => {
+      const p = collector.streamCollectorCommandToFile("npm", ["install"], "/cwd", OUTPUT_FILE)
+      await waitForCloseCb()
+      stderrDataCb?.("npm error: install failed\n")
+      closeCb!(1)
+      await expect(p).rejects.toThrow()
+    })
+
+    test("npm list with full path exit code 1: stderr suppressed (shouldIgnore applies to basename)", async ({ expect }) => {
+      const p = collector.streamCollectorCommandToFile("/usr/local/bin/npm", ["list", "--json"], "/cwd", OUTPUT_FILE)
+      await waitForCloseCb()
+      stderrDataCb?.("npm error code ELSPROBLEMS\nnpm error invalid: some-pkg@npm:stub@1.0.0\n")
+      closeCb!(1)
+      await p
+      expect(collector.logSummary[LogMessageByKey.PKG_COLLECTOR_OUTPUT]).toHaveLength(0)
+    })
+
+    test("win32 npm.cmd wrapped via cmd.exe: exit code 1 still triggers shouldIgnore", async ({ expect }) => {
+      // Regression: execName must be derived from the original command ("npm"), not the rewritten
+      // cmd.exe invocation, otherwise shouldIgnore never fires and npm list exit code 1 rejects.
+      // Use "npm.cmd" without a backslash-prefixed directory so path.basename works cross-platform
+      // in the test environment (macOS path.basename ignores backslash separators).
+      setPlatform("win32")
+      const p = collector.streamCollectorCommandToFile("npm.cmd", ["list", "--json"], "/cwd", OUTPUT_FILE)
+      await waitForCloseCb()
+      stderrDataCb?.("npm error code ELSPROBLEMS\nnpm error invalid: canvas@npm:npm-empty-stub@1.0.1\n")
+      closeCb!(1)
+      await p
+      expect(collector.logSummary[LogMessageByKey.PKG_COLLECTOR_OUTPUT]).toHaveLength(0)
+    })
+  })
 })