Fix Crush smoke test: redirect npm global install to writable $RUNNER_TEMP to avoid EROFS

pkg/workflow/crush_engine.go

@@ -10,6 +10,13 @@ import (
 
 var crushLog = logger.New("workflow:crush_engine")
 
+// crushNpmGlobalPrefix is the writable npm global prefix used when installing
+// Crush CLI. GitHub-hosted runners mount the Node.js toolcache at
+// /opt/hostedtoolcache with EROFS (read-only); npm install -g without a custom
+// prefix fails. Pointing npm to $RUNNER_TEMP keeps the install writable while
+// making the resulting binary findable via GetCrushNpmBinPathSetup.
+const crushNpmGlobalPrefix = "${RUNNER_TEMP}/npm-global"
+
 // CrushEngine represents the Crush CLI agentic engine.
 // Crush is a provider-agnostic, open-source AI coding agent with broader BYOK
 // (Bring Your Own Key) support, but gh-aw currently supports a subset of
@@ -59,16 +66,58 @@ func (e *CrushEngine) GetInstallationSteps(workflowData *WorkflowData) []GitHubA
 		return []GitHubActionStep{}
 	}
 
-	npmSteps := BuildStandardNpmEngineInstallSteps(
-		"@charmland/crush",
-		string(constants.DefaultCrushVersion),
-		"Install Crush CLI",
-		"crush",
-		workflowData,
-	)
+	// Determine version to install
+	version := string(constants.DefaultCrushVersion)
+	if workflowData.EngineConfig != nil && workflowData.EngineConfig.Version != "" {
+		version = workflowData.EngineConfig.Version
+	}
+
+	// Use Node.js setup + custom install step that redirects npm's global prefix
+	// to a writable directory. GitHub-hosted runners mount the Node.js toolcache
+	// at /opt/hostedtoolcache with EROFS (read-only); npm install -g without a
+	// custom prefix fails with EROFS.
+	npmSteps := []GitHubActionStep{
+		GenerateNodeJsSetupStep(),
+		e.buildCrushInstallStep(version),
+	}
 	return BuildNpmEngineInstallStepsWithAWF(npmSteps, workflowData)
 }
 
+// buildCrushInstallStep generates an npm install step for the Crush CLI that
+// redirects npm's global prefix to a writable directory under $RUNNER_TEMP.
+// GitHub-hosted runners mount /opt/hostedtoolcache with EROFS (read-only
+// filesystem) so a plain `npm install -g` fails. By setting NPM_CONFIG_PREFIX
+// we avoid the read-only toolcache while keeping the binary accessible via
+// GetCrushNpmBinPathSetup at execution time.
+func (e *CrushEngine) buildCrushInstallStep(version string) GitHubActionStep {
+	baseCmd := "export NPM_CONFIG_PREFIX=\"" + crushNpmGlobalPrefix + "\"\nmkdir -p \"" + crushNpmGlobalPrefix + "/bin\"\n"
+
+	var command string
+	var env map[string]string
+
+	if ExpressionPattern.MatchString(version) {
+		// Version is a GitHub Actions expression – pass through an env var to
+		// prevent shell injection if the expression evaluates to an attacker-
+		// controlled string.
+		command = baseCmd + `npm install --ignore-scripts -g @charmland/crush@"${ENGINE_VERSION}"`
+		env = map[string]string{"ENGINE_VERSION": version}
+	} else {
+		command = baseCmd + "npm install --ignore-scripts -g @charmland/crush@" + version
+	}
+
+	stepLines := []string{"      - name: Install Crush CLI"}
+	stepLines = FormatStepWithCommandAndEnv(stepLines, command, env)
+	return GitHubActionStep(stepLines)
+}
+
+// GetCrushNpmBinPathSetup returns a shell command that prepends Crush's writable
+// npm global bin directory to PATH, followed by the standard hostedtoolcache bin
+// directories. Call this before executing the crush binary so it is found
+// regardless of the runner's Node.js toolcache layout.
+func GetCrushNpmBinPathSetup() string {
+	return `export PATH="` + crushNpmGlobalPrefix + `/bin:$(find /opt/hostedtoolcache /home/runner/work/_tool -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true`
+}
+
 // GetSecretValidationStep returns the secret validation step for the Crush engine.
 // Returns an empty step if copilot-requests feature is enabled (uses GitHub Actions token).
 func (e *CrushEngine) GetSecretValidationStep(workflowData *WorkflowData) GitHubActionStep {
@@ -153,7 +202,7 @@ func (e *CrushEngine) GetExecutionSteps(workflowData *WorkflowData, logFile stri
 			panic(fmt.Sprintf("BUG: invalid model %q reached domain computation (should have been caught by validation): %v", model, err))
 		}
 
-		npmPathSetup := GetNpmBinPathSetup()
+		npmPathSetup := GetCrushNpmBinPathSetup()
 		crushCommandWithPath := fmt.Sprintf("%s && %s", npmPathSetup, crushCommand)
 		if mcpCLIPath := GetMCPCLIPathSetup(workflowData); mcpCLIPath != "" {
 			crushCommandWithPath = fmt.Sprintf("%s && %s", mcpCLIPath, crushCommandWithPath)
@@ -168,7 +217,9 @@ func (e *CrushEngine) GetExecutionSteps(workflowData *WorkflowData, logFile stri
 			AllowedDomains: allowedDomains,
 		})
 	} else {
-		command = fmt.Sprintf("set -o pipefail\n%s 2>&1 | tee -a %s", crushCommand, logFile)
+		// Add PATH setup so crush is found from its writable npm global prefix.
+		pathSetup := GetCrushNpmBinPathSetup()
+		command = fmt.Sprintf("set -o pipefail\n%s\n%s 2>&1 | tee -a %s", pathSetup, crushCommand, logFile)
 	}
 
 	env := map[string]string{

pkg/workflow/crush_engine_test.go

@@ -165,6 +165,43 @@ func TestCrushEngineInstallation(t *testing.T) {
 		assert.GreaterOrEqual(t, len(steps), 2, "Should have at least 2 installation steps")
 	})
 
+	t.Run("install step uses writable npm prefix to avoid EROFS", func(t *testing.T) {
+		workflowData := &WorkflowData{
+			Name: "test-workflow",
+		}
+
+		steps := engine.GetInstallationSteps(workflowData)
+		require.GreaterOrEqual(t, len(steps), 2, "Should have at least 2 installation steps")
+
+		// Find the Install Crush CLI step (last step, after Node.js setup)
+		installStep := steps[len(steps)-1]
+		installContent := strings.Join(installStep, "\n")
+
+		assert.Contains(t, installContent, "Install Crush CLI", "Should be the install step")
+		assert.Contains(t, installContent, "NPM_CONFIG_PREFIX", "Should set NPM_CONFIG_PREFIX to redirect from read-only toolcache")
+		assert.Contains(t, installContent, "${RUNNER_TEMP}/npm-global", "Should redirect npm prefix to writable RUNNER_TEMP directory")
+		assert.Contains(t, installContent, "--ignore-scripts", "Should use --ignore-scripts for supply-chain safety")
+		assert.Contains(t, installContent, "@charmland/crush", "Should install the crush package")
+	})
+
+	t.Run("install step with expression version uses ENGINE_VERSION env var", func(t *testing.T) {
+		workflowData := &WorkflowData{
+			Name: "test-workflow",
+			EngineConfig: &EngineConfig{
+				Version: "${{ inputs.crush-version }}",
+			},
+		}
+
+		steps := engine.GetInstallationSteps(workflowData)
+		require.GreaterOrEqual(t, len(steps), 2, "Should have at least 2 installation steps")
+
+		installStep := steps[len(steps)-1]
+		installContent := strings.Join(installStep, "\n")
+
+		assert.Contains(t, installContent, "ENGINE_VERSION:", "Should pass expression version via env var for injection safety")
+		assert.Contains(t, installContent, `"${ENGINE_VERSION}"`, "Should reference ENGINE_VERSION in install command")
+	})
+
 	t.Run("custom command skips installation", func(t *testing.T) {
 		workflowData := &WorkflowData{
 			Name: "test-workflow",
@@ -345,6 +382,18 @@ func TestCrushEngineExecution(t *testing.T) {
 		assert.Contains(t, stepContent, "CUSTOM_VAR: custom-value", "engine.env non-secret vars should be included")
 	})
 
+	t.Run("non-firewall execution includes npm-global/bin in PATH", func(t *testing.T) {
+		workflowData := &WorkflowData{
+			Name: "test-workflow",
+		}
+
+		steps := engine.GetExecutionSteps(workflowData, "/tmp/test.log")
+		require.Len(t, steps, 2, "Should generate config step and execution step")
+
+		stepContent := strings.Join(steps[1], "\n")
+		assert.Contains(t, stepContent, "${RUNNER_TEMP}/npm-global/bin", "Non-firewall path should include writable npm global bin in PATH")
+	})
+
 	t.Run("config step is first", func(t *testing.T) {
 		workflowData := &WorkflowData{
 			Name: "test-workflow",
@@ -390,6 +439,7 @@ func TestCrushEngineFirewallIntegration(t *testing.T) {
 		assert.Contains(t, stepContent, "allowDomains", "Should include allowDomains in config JSON")
 		assert.Contains(t, stepContent, `"enabled":true`, "Should include apiProxy enabled in config JSON")
 		assert.Contains(t, stepContent, "GITHUB_COPILOT_BASE_URL: http://host.docker.internal:10002", "Should route copilot/* fallback through Copilot LLM gateway URL")
+		assert.Contains(t, stepContent, "${RUNNER_TEMP}/npm-global/bin", "Firewall path should include writable npm global bin in PATH")
 	})
 
 	t.Run("firewall enabled adds mounted MCP CLI path setup", func(t *testing.T) {