fix(exec): keep PATH for npm workspace commands (#5925)#382
fix(exec): keep PATH for npm workspace commands (#5925)#382
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
code-rs/core/src/exec_env.rs
Outdated
| for (key, value) in preserved_vars { | ||
| env_map.entry(key).or_insert(value); | ||
| } |
There was a problem hiding this comment.
Preserve PATH only when some variables are inherited
The new preserved_vars logic re‑adds PATH/NVM_DIR after all filtering even when the policy is ShellEnvironmentPolicyInherit::None. In that mode callers explicitly request a completely clean environment and the existing test_inherit_none asserts that only variables set via r#set remain. With the unconditional for (key, value) in preserved_vars { env_map.entry(key).or_insert(value); } block, PATH is inserted back into the map, breaking that contract and causing the inherit None test to fail as well as leaking host PATH into supposedly sandboxed processes. Consider skipping the reinsertion when inherit is None or when the key was not present in env_map prior to filtering.
Useful? React with 👍 / 👎.
zemaj
left a comment
There was a problem hiding this comment.
Summary:
- Keep PATH/NVM_DIR only when the shell profile inherits env vars, preventing
inherit = noneprofiles from being silently repopulated. - Prevent case-insensitive duplicates when re-inserting preserved vars so Windows overrides like
PATHvsPathstay authoritative. - Harden coverage with new env-builder unit tests (mixed case, Unix/Windows separators, override retention) and stabilize the npm smoke tests by creating a clean workspace directory.
Validation:
- cargo test -p code-core --lib exec_env::tests
- cargo test -p code-core --test npm_command
- ./build-fast.sh
Risks / follow-ups:
- PATH/NVM_DIR preservation still bypasses
include_onlyfilters for Core/All inheritance; consider exposing a policy switch or audit. - Windows-specific unit coverage depends on CI hosts with
target_os = "windows"; no runtime smoke test for PowerShell/cmd wrappers yet. - npm smoke tests continue to skip when npm is absent; a lightweight stub would remove that external dependency.
Summary
PATH(andNVM_DIR, if present) throughShellEnvironmentPolicyfiltering so npm remains discoverableuse_profileis enabled, ensuring profile-managed Node installations workTesting
cc1.2.41 is missing generated modules; clear/update the crate cache and rerun)Closes openai#5925.