[jihtsan]

Preserve and restore fish activation variables for #399, including unset/empty values and repeated activation. Active NODE_PATH remains isolated to the environment, and all six saved NODE_PATH/npm variables and their backups are restored together.

The latest review exposed a second case: NODE_VIRTUAL_ENV may be removed while the environment is still active. Restore under the existing saved-PATH guard so deactivation still restores the caller's variables in that case. The generated-script test checks that --isolate-npm restoration is inside that guard, and a mutation moving it outside fails the test. Backup assertions identify any leaked variable.

Merged current upstream master (0c67226) to resolve CHANGES while preserving published history. The #419 entry is retained. Corrected the migration guidance: existing environments retain the old activate.fish; recreate with the same options. With -p, remove the old nodeenv part from the Python virtualenv's activate.fish first. --force alone is not a safe migration instruction.

Validation on macOS with fish 4.9.3 and actual sh/dash/bash/zsh/fish, NODEENV_REQUIRE_SHELLS=1:

• New erase-marker regressions against the merged prior implementation: 8 failed. Corrected implementation passes.
• Python 3.10.21: python -m pytest -m 'not integration' tests -q — 431 passed, 11 skipped, 5 deselected.
• Python 3.13.12: same command — 432 passed, 10 skipped, 5 deselected.
• flake8 --extend-ignore=E127 nodeenv.py tests setup.py and git diff --check passed.

Real Node download/build integration, other Python/fish versions and Linux/Windows execution were not run locally. Hosted CI for this new head is not yet verified. AI assistance: OpenAI Codex inspected the Issue/review, implemented the correction, ran these checks and reviewed Standards/Spec.

Closes #399.

[ekalinin]

Thanks, this fixes #399. Two changes are needed before merge, plus two small ones.

• [P1] Keep fish replacing NODE_PATH - nodeenv.py:2227

  This prepend branch has never run since fish support landed in 2017: the initial deactivate_node nondestructive pass erased NODE_PATH first, so fish replaced it like the other shells. Now it runs, and modules from the user's NODE_PATH resolve inside the env, under fish only. Please replace it, as the prefix variables below do:

  if set -q NODE_PATH
      set -gx _OLD_NODE_PATH $NODE_PATH
  end
  set -gx NODE_PATH "$NODE_VIRTUAL_ENV/__MOD_NAME__"

  Then test_activate_sets_env (tests/test_activate_shells.py:283) can compare the whole NODE_PATH, not just its first element.

• [P2] Same fix for the --isolate-npm restore - nodeenv.py:1768

  It still uses test -n, so an empty npm_config_* becomes unset and leaves an exported empty _OLD_npm_config_* behind. Please move __NPM_UNISOLATE__ inside the new guard, drop its duplicate guard, switch to set -q, and add the three npm_config_* names to names in test_fish_restores_original_variables.

• [P3] The REQUIRE_SHELLS comment (tests/test_activate_shells.py:144) still refers to xfail markers, which this PR removes.

• [P3] Please add an [unreleased] entry for #399 to CHANGES, noting that existing envs need --force to get the new activate.fish.

[ekalinin]

Thanks for the update, all four points from the last round are in. A few more things before merge.

• Please rebase on master: CHANGES conflicts with the #419 entry, and CI hasn't run on 306eb92.

• The new if set -q NODE_VIRTUAL_ENV guard (nodeenv.py:2127) skips the restore when NODE_VIRTUAL_ENV is already gone:

  set -gx NODE_PATH /old/np
  source env/bin/activate.fish
  set -e NODE_VIRTUAL_ENV
  deactivate_node

  After this, NODE_PATH, NPM_CONFIG_PREFIX and npm_config_prefix still point at the env, so npm i -g keeps installing there, and _OLD_NODE_PATH=/old/np stays exported. On master these three are at least erased. activate keeps all of these restores under one guard, _OLD_NODE_VIRTUAL_PATH, and fish can do the same: move them into the existing PATH block and drop the new guard.

      if test -n "$_OLD_NODE_VIRTUAL_PATH"
          set -gx PATH $_OLD_NODE_VIRTUAL_PATH
          set -e _OLD_NODE_VIRTUAL_PATH
  
          # NODE_PATH, NPM_CONFIG_PREFIX and npm_config_prefix restores as now
  __NPM_UNISOLATE__
      end

  The nondestructive pass at the top still skips them unless another env is active, because only activation sets _OLD_NODE_VIRTUAL_PATH. With this change the case above ends with NODE_PATH=/old/np, and the shell tests pass except test_isolate_npm_fish_content (tests/test_install_activate.py:411), which looks for the if set -q NODE_VIRTUAL_ENV string. When you update it, please make its order check fail if __NPM_UNISOLATE__ ends up outside the guard. Right now the guard is in every activate.fish and always comes first, so the check passes wherever the restores are.

• My --force suggestion for CHANGES was wrong, sorry. With -p, nodeenv appends the new code to the virtualenv's activate.fish, so the old block still runs first and erases NODE_PATH. Without -p, --force reinstalls node at the default version and drops --isolate-npm and --prompt unless they are passed again. Something like "existing environments keep the old activate.fish; recreate them with the same options to get the fix (with -p, remove the old nodeenv part from the virtualenv's activate.fish first)" would be closer.

• Nit: assert all(...) at tests/test_activate_shells.py:340 doesn't say which backup leaked. A dict comparison like the line above it would.

[ekalinin]

Thanks!