Skip to content

Conversation

@zemaj
Copy link

@zemaj zemaj commented Oct 30, 2025

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 openai#5925.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines 82 to 84
for (key, value) in preserved_vars {
env_map.entry(key).or_insert(value);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Copy link
Author

@zemaj zemaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary:

  • Keep PATH/NVM_DIR only when the shell profile inherits env vars, preventing inherit = none profiles from being silently repopulated.
  • Prevent case-insensitive duplicates when re-inserting preserved vars so Windows overrides like PATH vs Path stay 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_only filters 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.

@zemaj zemaj closed this Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Codex unable to run simple npm commands on the workspace

2 participants