fix(exec): keep PATH for npm workspace commands (#5925)

COMMIT_MESSAGE_ISSUE_5925.txt

@@ -0,0 +1 @@
+fix(exec): keep PATH for npm workspace commands (#5925)

PR_BODY.md

@@ -1,9 +1,10 @@
-## Overview
-- AutoRunPhase now carries struct payloads; controller exposes helpers (`is_active`, `is_paused_manual`, `resume_after_submit`, `awaiting_coordinator_submit`, `awaiting_review`, `in_transient_recovery`).
-- ChatWidget hot paths (manual pause, coordinator routing, ESC handling, review exit) rely on helpers/`matches!` instead of raw booleans.
+## Summary
+- preserve `PATH` (and `NVM_DIR` when present) across shell environment filtering so workspace commands like `npm` remain available
+- continue to respect `use_profile` so commands run through the user's login shell when configured
+- add unit coverage for the environment builder and an integration-style npm smoke test (skips automatically if npm is unavailable)
 
-## Tests
-- `./build-fast.sh`
+## Testing
+- ./build-fast.sh
+- cargo test -p code-core --test npm_command *(fails: local cargo registry copy of `cc` 1.2.41 is missing generated modules; clear/update the registry and rerun)*
 
-## Follow-ups
-- See `docs/auto-drive-phase-migration-TODO.md` for remaining legacy-flag removals and snapshot coverage.
+Closes #5925.

PR_BODY_ISSUE_5925.md

@@ -0,0 +1,10 @@
+## Summary
+- always preserve `PATH` (and `NVM_DIR`, if present) through `ShellEnvironmentPolicy` filtering so npm remains discoverable
+- continue to wrap commands in the user shell when `use_profile` is enabled, ensuring profile-managed Node installations work
+- add unit coverage for the environment builder and integration-style npm smoke tests (skipped automatically when npm is absent)
+
+## Testing
+- ./build-fast.sh
+- cargo test -p code-core --test npm_command *(fails: local cargo registry copy of `cc` 1.2.41 is missing generated modules; clear/update the crate cache and rerun)*
+
+Closes #5925.

code-rs/core/src/exec_env.rs

@@ -19,18 +19,32 @@ fn populate_env<I>(vars: I, policy: &ShellEnvironmentPolicy) -> HashMap<String,
 where
     I: IntoIterator<Item = (String, String)>,
 {
+    let collected: Vec<(String, String)> = vars.into_iter().collect();
+
+    let mut preserved_vars: Vec<(String, String)> = Vec::new();
+    for key in ["PATH", "NVM_DIR"] {
+        if let Some((actual_key, value)) = collected
+            .iter()
+            .find(|(k, _)| k.eq_ignore_ascii_case(key))
+        {
+            preserved_vars.push((actual_key.clone(), value.clone()));
+        }
+    }
+
     // Step 1 – determine the starting set of variables based on the
     // `inherit` strategy.
     let mut env_map: HashMap<String, String> = match policy.inherit {
-        ShellEnvironmentPolicyInherit::All => vars.into_iter().collect(),
+        ShellEnvironmentPolicyInherit::All => collected.iter().cloned().collect(),
         ShellEnvironmentPolicyInherit::None => HashMap::new(),
         ShellEnvironmentPolicyInherit::Core => {
             const CORE_VARS: &[&str] = &[
                 "HOME", "LOGNAME", "PATH", "SHELL", "USER", "USERNAME", "TMPDIR", "TEMP", "TMP",
             ];
             let allow: HashSet<&str> = CORE_VARS.iter().copied().collect();
-            vars.into_iter()
+            collected
+                .iter()
                 .filter(|(k, _)| allow.contains(k.as_str()))
+                .cloned()
                 .collect()
         }
     };
@@ -65,6 +79,10 @@ where
         env_map.retain(|k, _| matches_any(k, &policy.include_only));
     }
 
+    for (key, value) in preserved_vars {
+        env_map.entry(key).or_insert(value);
+    }
+
     env_map
 }
 
@@ -172,6 +190,26 @@ mod tests {
         assert_eq!(result, expected);
     }
 
+    #[test]
+    fn test_path_preserved_after_include_only_filters() {
+        let vars = make_vars(&[
+            ("PATH", "/usr/local/bin"),
+            ("NVM_DIR", "/home/user/.nvm"),
+            ("FOO", "bar"),
+        ]);
+
+        let policy = ShellEnvironmentPolicy {
+            ignore_default_excludes: true,
+            include_only: vec![EnvironmentVariablePattern::new_case_insensitive("FOO")],
+            ..Default::default()
+        };
+
+        let result = populate_env(vars, &policy);
+
+        assert_eq!(result.get("PATH"), Some(&"/usr/local/bin".to_string()));
+        assert_eq!(result.get("NVM_DIR"), Some(&"/home/user/.nvm".to_string()));
+    }
+
     #[test]
     fn test_inherit_none() {
         let vars = make_vars(&[("PATH", "/usr/bin"), ("HOME", "/home")]);

code-rs/core/tests/npm_command.rs

@@ -0,0 +1,89 @@
+#![cfg(unix)]
+
+use std::process::Command;
+use std::time::Duration;
+
+use code_core::exec_command::{result_into_payload, ExecCommandParams, ExecSessionManager};
+use serde_json::json;
+use tempfile::tempdir;
+use tokio::time::timeout;
+
+fn make_params(cmd: &str, cwd: Option<&std::path::Path>) -> ExecCommandParams {
+    let mut value = json!({
+        "cmd": cmd,
+        "yield_time_ms": 10_000u64,
+        "max_output_tokens": 10_000u64,
+        "shell": "/bin/bash",
+        "login": true
+    });
+
+    if let Some(dir) = cwd {
+        value["cmd"] = json!(format!("cd {} && {cmd}", dir.display()));
+    }
+
+    serde_json::from_value(value).expect("deserialize ExecCommandParams")
+}
+
+fn npm_available() -> bool {
+    match Command::new("npm").arg("--version").output() {
+        Ok(output) => output.status.success(),
+        Err(_) => false,
+    }
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+async fn npm_version_executes() {
+    if !npm_available() {
+        eprintln!("skipping npm_version_executes: npm not available");
+        return;
+    }
+
+    let manager = ExecSessionManager::default();
+    let params = make_params("npm --version", None);
+
+    let summary = manager
+        .handle_exec_command_request(params)
+        .await
+        .map(|output| result_into_payload(Ok(output)))
+        .expect("exec request should succeed");
+
+    assert_eq!(summary.success, Some(true));
+    assert!(
+        summary.content.contains("Process exited with code 0"),
+        "npm --version should exit successfully"
+    );
+    assert!(
+        summary.content.to_lowercase().contains("npm"),
+        "version output should include npm"
+    );
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+async fn npm_init_creates_package_json() {
+    if !npm_available() {
+        eprintln!("skipping npm_init_creates_package_json: npm not available");
+        return;
+    }
+
+    let temp = tempdir().expect("create temp dir");
+    let workspace = temp.path();
+
+    let manager = ExecSessionManager::default();
+    let params = make_params("npm init -y", Some(workspace));
+
+    let exec_future = manager.handle_exec_command_request(params);
+    let summary = timeout(Duration::from_secs(30), exec_future)
+        .await
+        .expect("npm init should complete within timeout")
+        .map(|output| result_into_payload(Ok(output)))
+        .expect("exec request should succeed");
+
+    assert_eq!(summary.success, Some(true));
+    assert!(
+        summary.content.contains("Process exited with code 0"),
+        "npm init should exit successfully"
+    );
+
+    let package_json = workspace.join("package.json");
+    assert!(package_json.exists(), "npm init should create package.json");
+}