From fc0fd2d9dcc32a9c3459fe3d39f5e97598230282 Mon Sep 17 00:00:00 2001 From: James Peter Date: Fri, 31 Oct 2025 03:01:48 +1000 Subject: [PATCH 1/2] fix(exec): keep PATH for npm workspace commands (#5925) --- COMMIT_MESSAGE_ISSUE_5925.txt | 1 + PR_BODY.md | 15 +++--- PR_BODY_ISSUE_5925.md | 10 ++++ code-rs/core/src/exec_env.rs | 42 ++++++++++++++- code-rs/core/tests/npm_command.rs | 89 +++++++++++++++++++++++++++++++ 5 files changed, 148 insertions(+), 9 deletions(-) create mode 100644 COMMIT_MESSAGE_ISSUE_5925.txt create mode 100644 PR_BODY_ISSUE_5925.md create mode 100644 code-rs/core/tests/npm_command.rs diff --git a/COMMIT_MESSAGE_ISSUE_5925.txt b/COMMIT_MESSAGE_ISSUE_5925.txt new file mode 100644 index 00000000000..f324980ae81 --- /dev/null +++ b/COMMIT_MESSAGE_ISSUE_5925.txt @@ -0,0 +1 @@ +fix(exec): keep PATH for npm workspace commands (#5925) diff --git a/PR_BODY.md b/PR_BODY.md index f9128ce12ac..0896e721414 100644 --- a/PR_BODY.md +++ b/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. diff --git a/PR_BODY_ISSUE_5925.md b/PR_BODY_ISSUE_5925.md new file mode 100644 index 00000000000..3d0c8c6963f --- /dev/null +++ b/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. diff --git a/code-rs/core/src/exec_env.rs b/code-rs/core/src/exec_env.rs index 88246b063c3..22b31b91c2d 100644 --- a/code-rs/core/src/exec_env.rs +++ b/code-rs/core/src/exec_env.rs @@ -19,18 +19,32 @@ fn populate_env(vars: I, policy: &ShellEnvironmentPolicy) -> HashMap, { + 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 = 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")]); diff --git a/code-rs/core/tests/npm_command.rs b/code-rs/core/tests/npm_command.rs new file mode 100644 index 00000000000..0c4500bec0b --- /dev/null +++ b/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"); +} From d92efe1ad6e10f1a1317f3054a203fac0102af29 Mon Sep 17 00:00:00 2001 From: James Peter Date: Fri, 7 Nov 2025 06:22:07 +1000 Subject: [PATCH 2/2] fix(exec/env): dedupe preserved PATH/NVM entries --- code-rs/core/src/exec_env.rs | 99 +++++++++++++++++++++++++++++-- code-rs/core/src/lib.rs | 1 + code-rs/core/tests/npm_command.rs | 22 +++---- 3 files changed, 104 insertions(+), 18 deletions(-) diff --git a/code-rs/core/src/exec_env.rs b/code-rs/core/src/exec_env.rs index 22b31b91c2d..a69f7070907 100644 --- a/code-rs/core/src/exec_env.rs +++ b/code-rs/core/src/exec_env.rs @@ -79,8 +79,16 @@ where env_map.retain(|k, _| matches_any(k, &policy.include_only)); } - for (key, value) in preserved_vars { - env_map.entry(key).or_insert(value); + 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 @@ -193,7 +201,7 @@ mod tests { #[test] fn test_path_preserved_after_include_only_filters() { let vars = make_vars(&[ - ("PATH", "/usr/local/bin"), + ("PATH", "/usr/local/bin:/opt/node/bin"), ("NVM_DIR", "/home/user/.nvm"), ("FOO", "bar"), ]); @@ -206,10 +214,93 @@ mod tests { let result = populate_env(vars, &policy); - assert_eq!(result.get("PATH"), Some(&"/usr/local/bin".to_string())); + 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, 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")]); diff --git a/code-rs/core/src/lib.rs b/code-rs/core/src/lib.rs index 0d1b356fcc2..75d76eabcc1 100644 --- a/code-rs/core/src/lib.rs +++ b/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 diff --git a/code-rs/core/tests/npm_command.rs b/code-rs/core/tests/npm_command.rs index 0c4500bec0b..bbd4a24e321 100644 --- a/code-rs/core/tests/npm_command.rs +++ b/code-rs/core/tests/npm_command.rs @@ -1,9 +1,10 @@ #![cfg(unix)] +use std::fs; use std::process::Command; use std::time::Duration; -use code_core::exec_command::{result_into_payload, ExecCommandParams, ExecSessionManager}; +use code_core::{result_into_payload, ExecCommandParams, ExecSessionManager}; use serde_json::json; use tempfile::tempdir; use tokio::time::timeout; @@ -49,12 +50,8 @@ async fn npm_version_executes() { 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" + !summary.content.to_lowercase().contains("not found"), + "npm --version output should not indicate a missing binary" ); } @@ -66,10 +63,12 @@ async fn npm_init_creates_package_json() { } let temp = tempdir().expect("create temp dir"); - let workspace = temp.path(); + 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)); + 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) @@ -79,11 +78,6 @@ async fn npm_init_creates_package_json() { .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"); }