Set shell: true when spawning npm commands (#732)

.github/workflows/pr.yml

@@ -16,7 +16,7 @@ jobs:
     strategy:
       matrix:
         node-version: [16.x]
-        os: [ubuntu-latest]
+        os: [ubuntu-latest, windows-latest]
 
     runs-on: ${{ matrix.os }}
 
@@ -33,19 +33,23 @@ jobs:
       - run: yarn
 
       - name: Code Format Check
+        if: ${{ matrix.os == 'ubuntu-latest' }}
         run: yarn format:check
 
       - name: Check Change Files
+        if: ${{ matrix.os == 'ubuntu-latest' }}
         run: yarn checkchange
 
       # @see https://www.npmjs.com/package/syncpack
       - name: Check consistent package.json dep versions
+        if: ${{ matrix.os == 'ubuntu-latest' }}
         run: yarn syncpack list-mismatches
 
       - name: Dependency checks
+        if: ${{ matrix.os == 'ubuntu-latest' }}
         run: yarn lage depcheck
 
-      - name: Build, Test, Lint (Linux)
+      - name: Build, Test, Lint
         run: yarn ci --concurrency 2 --verbose
         env:
           BACKFILL_CACHE_PROVIDER: ${{ secrets.backfill_cache_provider }}

change/change-96165148-6e4f-44e7-932b-1ef0bec91c3e.json

@@ -0,0 +1,18 @@
+{
+  "changes": [
+    {
+      "type": "minor",
+      "comment": "Set shell: true when spawning npm commands, due to Node security fix. Also remove custom npm client resolution logic, which should be handled based on the PATH in the shell.",
+      "packageName": "@lage-run/cli",
+      "email": "elcraig@microsoft.com",
+      "dependentChangeType": "patch"
+    },
+    {
+      "type": "minor",
+      "comment": "Set shell: true when spawning npm commands, due to Node security fix",
+      "packageName": "@lage-run/scheduler",
+      "email": "elcraig@microsoft.com",
+      "dependentChangeType": "patch"
+    }
+  ]
+}

packages/cli/package.json

@@ -22,7 +22,6 @@
   "dependencies": {
     "@lage-run/cache": "^1.1.5",
     "@lage-run/config": "^0.3.5",
-    "@lage-run/find-npm-client": "^0.1.4",
     "@lage-run/hasher": "^1.1.0",
     "@lage-run/logger": "^1.3.0",
     "@lage-run/reporters": "^1.2.7",

packages/cli/src/commands/init/action.ts

@@ -102,7 +102,7 @@ async function installLage(cwd: string, workspaceManager: WorkspaceManager, pipe
     packageJson.devDependencies.lage = lageVersion;
     writePackageJson(cwd, packageJson);
 
-    await execa(workspaceManager, ["install"], { stdio: "inherit" });
+    await execa(workspaceManager, ["install"], { stdio: "inherit", shell: true });
   }
 }
 

packages/cli/src/commands/run/runAction.ts

@@ -2,7 +2,6 @@ import type { Command } from "commander";
 import { createTargetGraph } from "./createTargetGraph.js";
 import { filterArgsForTasks } from "./filterArgsForTasks.js";
 import { filterPipelineDefinitions } from "./filterPipelineDefinitions.js";
-import { findNpmClient } from "@lage-run/find-npm-client";
 import { getConfig, getMaxWorkersPerTask, getMaxWorkersPerTaskFromOptions, getConcurrency } from "@lage-run/config";
 import { getPackageInfos, getWorkspaceRoot } from "workspace-tools";
 import { initializeReporters } from "../initializeReporters.js";
@@ -97,7 +96,7 @@ export async function runAction(options: RunOptions, command: Command) {
           options: {
             nodeArg: options.nodeArg,
             taskArgs,
-            npmCmd: findNpmClient(config.npmClient),
+            npmCmd: config.npmClient,
           },
         },
         worker: {

packages/cli/src/commands/run/watchAction.ts

@@ -1,7 +1,6 @@
 import type { Command } from "commander";
 import { createTargetGraph } from "./createTargetGraph.js";
 import { filterArgsForTasks } from "./filterArgsForTasks.js";
-import { findNpmClient } from "@lage-run/find-npm-client";
 import { getConfig, getMaxWorkersPerTask, getMaxWorkersPerTaskFromOptions, getConcurrency } from "@lage-run/config";
 import { getPackageInfosAsync, getWorkspaceRoot } from "workspace-tools";
 import { filterPipelineDefinitions } from "./filterPipelineDefinitions.js";
@@ -93,7 +92,7 @@ export async function watchAction(options: RunOptions, command: Command) {
           options: {
             nodeArg: options.nodeArg,
             taskArgs,
-            npmCmd: findNpmClient(config.npmClient),
+            npmCmd: config.npmClient,
           },
         },
         worker: {

packages/e2e-tests/src/basic.test.ts

@@ -2,8 +2,15 @@ import { Monorepo } from "./mock/monorepo.js";
 import { filterEntry, parseNdJson } from "./parseNdJson.js";
 
 describe("basics", () => {
+  let repo: Monorepo | undefined;
+
+  afterEach(() => {
+    repo?.cleanup();
+    repo = undefined;
+  });
+
   it("basic test case", () => {
-    const repo = new Monorepo("basics");
+    repo = new Monorepo("basics");
 
     repo.init();
     repo.addPackage("a", ["b"]);
@@ -20,12 +27,10 @@ describe("basics", () => {
     expect(jsonOutput.find((entry) => filterEntry(entry.data, "a", "build", "success"))).toBeTruthy();
     expect(jsonOutput.find((entry) => filterEntry(entry.data, "a", "test", "success"))).toBeTruthy();
     expect(jsonOutput.find((entry) => filterEntry(entry.data, "a", "lint", "success"))).toBeFalsy();
-
-    repo.cleanup();
   });
 
   it("basic with missing script names - logging should not include those targets", () => {
-    const repo = new Monorepo("basics-missing-scripts");
+    repo = new Monorepo("basics-missing-scripts");
 
     repo.init();
     repo.addPackage("a", ["b"]);
@@ -47,12 +52,10 @@ describe("basics", () => {
     expect(jsonOutput.find((entry) => filterEntry(entry.data, "a", "build", "success"))).toBeFalsy();
     expect(jsonOutput.find((entry) => filterEntry(entry.data, "a", "test", "success"))).toBeFalsy();
     expect(jsonOutput.find((entry) => filterEntry(entry.data, "a", "lint", "success"))).toBeFalsy();
-
-    repo.cleanup();
   });
 
   it("basic test case - with task args", () => {
-    const repo = new Monorepo("basics-with-task-args");
+    repo = new Monorepo("basics-with-task-args");
 
     repo.init();
     repo.addPackage("a", ["b"]);
@@ -106,7 +109,25 @@ describe("basics", () => {
     expect(jsonOutput4.find((entry) => filterEntry(entry.data, "a", "build", "skipped"))).toBeTruthy();
     expect(jsonOutput4.find((entry) => filterEntry(entry.data, "a", "test", "skipped"))).toBeTruthy();
     expect(jsonOutput4.find((entry) => filterEntry(entry.data, "a", "lint", "skipped"))).toBeFalsy();
+  });
+
+  it("works in repo with spaces", () => {
+    repo = new Monorepo("spaces why");
+
+    repo.init();
+    repo.addPackage("a", ["b"]);
+    repo.addPackage("b");
 
-    repo.cleanup();
+    repo.install();
+
+    const results = repo.run("test");
+    const output = results.stdout + results.stderr;
+    const jsonOutput = parseNdJson(output);
+
+    expect(jsonOutput.find((entry) => filterEntry(entry.data, "b", "build", "success"))).toBeTruthy();
+    expect(jsonOutput.find((entry) => filterEntry(entry.data, "b", "test", "success"))).toBeTruthy();
+    expect(jsonOutput.find((entry) => filterEntry(entry.data, "a", "build", "success"))).toBeTruthy();
+    expect(jsonOutput.find((entry) => filterEntry(entry.data, "a", "test", "success"))).toBeTruthy();
+    expect(jsonOutput.find((entry) => filterEntry(entry.data, "a", "lint", "success"))).toBeFalsy();
   });
 });

packages/e2e-tests/src/mock/monorepo.ts

@@ -44,26 +44,27 @@ export class Monorepo {
       fs.cpSync(packagePath, path.join(this.root, "node_modules", name), { recursive: true });
     }
 
-    fs.cpSync(path.join(__dirname, "..", "..", "yarn"), path.dirname(this.yarnPath), { recursive: true });
-    execa.sync("node", [this.yarnPath, "install"], { cwd: this.root });
+    fs.cpSync(path.resolve(__dirname, "..", "..", "yarn"), path.dirname(this.yarnPath), { recursive: true });
+    execa.sync("yarn", ["install"], { cwd: this.root });
   }
 
   generateRepoFiles() {
     this.commitFiles({
+      ".yarnrc": `yarn-path "${this.yarnPath}"`,
       "package.json": {
-        name: this.name,
+        name: this.name.replace(/ /g, "-"),
         version: "0.1.0",
         private: true,
         workspaces: ["packages/*"],
         scripts: {
-          bundle: `node ${this.yarnPath} lage bundle --reporter json --log-level silly`,
-          transpile: `node ${this.yarnPath} lage transpile --reporter json --log-level silly`,
-          build: `node ${this.yarnPath} lage build --reporter json --log-level silly`,
-          writeInfo: `node ${this.yarnPath} lage info`,
-          test: `node ${this.yarnPath} lage test --reporter json --log-level silly`,
-          lint: `node ${this.yarnPath} lage lint --reporter json --log-level silly`,
-          clear: `node ${this.yarnPath} lage cache --clear --reporter json --log-level silly`,
-          extra: `node ${this.yarnPath} lage extra --clear --reporter json --log-level silly`,
+          bundle: `lage bundle --reporter json --log-level silly`,
+          transpile: `lage transpile --reporter json --log-level silly`,
+          build: `lage build --reporter json --log-level silly`,
+          writeInfo: `lage info`,
+          test: `lage test --reporter json --log-level silly`,
+          lint: `lage lint --reporter json --log-level silly`,
+          clear: `lage cache --clear --reporter json --log-level silly`,
+          extra: `lage extra --clear --reporter json --log-level silly`,
         },
         devDependencies: {
           lage: path.resolve(__dirname, "..", "..", "..", "lage"),
@@ -75,7 +76,8 @@ export class Monorepo {
           test: ['build'],
           lint: [],
           extra: []
-        }
+        },
+        npmClient: 'yarn'
       };`,
       ".gitignore": "node_modules",
     });
@@ -151,6 +153,7 @@ export class Monorepo {
   run(command: string, args?: string[], silent?: boolean) {
     return execa.sync("yarn", [...(silent === true ? ["--silent"] : []), command, ...(args || [])], {
       cwd: this.root,
+      shell: true,
     });
   }