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,18 @@ where
         env_map.retain(|k, _| matches_any(k, &policy.include_only));
     }
 
+    if policy.inherit != ShellEnvironmentPolicyInherit::None {
+        for (key, value) in preserved_vars {
+            let already_present = env_map
+                .keys()
+                .any(|existing| existing.eq_ignore_ascii_case(&key));
+
+            if !already_present {
+                env_map.insert(key, value);
+            }
+        }
+    }
+
     env_map
 }
 
@@ -172,6 +198,109 @@ mod tests {
         assert_eq!(result, expected);
     }
 
+    #[test]
+    fn test_path_preserved_after_include_only_filters() {
+        let vars = make_vars(&[
+            ("PATH", "/usr/local/bin:/opt/node/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:/opt/node/bin".to_string())
+        );
+        assert_eq!(result.get("NVM_DIR"), Some(&"/home/user/.nvm".to_string()));
+    }
+
+    fn count_case_insensitive(map: &HashMap<String, String>, needle: &str) -> usize {
+        map.keys()
+            .filter(|name| name.eq_ignore_ascii_case(needle))
+            .count()
+    }
+
+    #[test]
+    fn test_path_preserved_when_case_differs() {
+        let vars = make_vars(&[("Path", "C:/OldNode"), ("FOO", "bar")]);
+
+        let policy = ShellEnvironmentPolicy {
+            inherit: ShellEnvironmentPolicyInherit::Core,
+            ignore_default_excludes: true,
+            include_only: vec![EnvironmentVariablePattern::new_case_insensitive("FOO")],
+            ..Default::default()
+        };
+
+        let result = populate_env(vars, &policy);
+
+        assert_eq!(count_case_insensitive(&result, "PATH"), 1);
+        assert_eq!(
+            result
+                .iter()
+                .find(|(k, _)| k.eq_ignore_ascii_case("PATH"))
+                .map(|(_, v)| v.as_str()),
+            Some("C:/OldNode")
+        );
+    }
+
+    #[test]
+    fn test_path_override_keeps_new_value() {
+        let vars = make_vars(&[("Path", "C:/OldNode"), ("FOO", "bar")]);
+
+        let mut policy = ShellEnvironmentPolicy {
+            inherit: ShellEnvironmentPolicyInherit::Core,
+            ignore_default_excludes: true,
+            ..Default::default()
+        };
+        policy
+            .r#set
+            .insert("PATH".to_string(), "D:/Tools/node".to_string());
+
+        let result = populate_env(vars, &policy);
+
+        assert_eq!(count_case_insensitive(&result, "PATH"), 1);
+        assert_eq!(
+            result
+                .iter()
+                .find(|(k, _)| k.eq_ignore_ascii_case("PATH"))
+                .map(|(_, v)| v.as_str()),
+            Some("D:/Tools/node")
+        );
+    }
+
+    #[cfg(windows)]
+    #[test]
+    fn test_windows_separators_preserved() {
+        let vars = make_vars(&[(
+            "Path",
+            r"C:\Windows\System32;C:\Program Files\nodejs;D:\nvm",
+        )]);
+
+        let policy = ShellEnvironmentPolicy {
+            inherit: ShellEnvironmentPolicyInherit::Core,
+            ignore_default_excludes: true,
+            ..Default::default()
+        };
+
+        let result = populate_env(vars, &policy);
+
+        assert_eq!(count_case_insensitive(&result, "PATH"), 1);
+        assert_eq!(
+            result
+                .iter()
+                .find(|(k, _)| k.eq_ignore_ascii_case("PATH"))
+                .map(|(_, v)| v.as_str()),
+            Some(r"C:\Windows\System32;C:\Program Files\nodejs;D:\nvm")
+        );
+    }
+
     #[test]
     fn test_inherit_none() {
         let vars = make_vars(&[("PATH", "/usr/bin"), ("HOME", "/home")]);

code-rs/core/src/lib.rs

@@ -104,6 +104,7 @@ pub use command_safety::is_safe_command;
 pub use safety::get_platform_sandbox;
 pub use housekeeping::run_housekeeping_if_due;
 pub use housekeeping::CleanupOutcome;
+pub use exec_command::{result_into_payload, ExecCommandParams, ExecSessionManager};
 // Use our internal protocol module for crate-internal types and helpers.
 // External callers should rely on specific re-exports below.
 // Re-export protocol config enums to ensure call sites can use the same types

code-rs/core/tests/npm_command.rs

@@ -0,0 +1,83 @@
+#![cfg(unix)]
+
+use std::fs;
+use std::process::Command;
+use std::time::Duration;
+
+use code_core::{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.to_lowercase().contains("not found"),
+        "npm --version output should not indicate a missing binary"
+    );
+}
+
+#[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_root = temp.path();
+    let workspace = workspace_root.join("npm-workspace");
+    fs::create_dir(&workspace).expect("create npm workspace");
+
+    let manager = ExecSessionManager::default();
+    let params = make_params("npm init -y", Some(workspace.as_path()));
+
+    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));
+    let package_json = workspace.join("package.json");
+    assert!(package_json.exists(), "npm init should create package.json");
+}