Skip to content

Fix global routing after clearing explicit selections - #1979

Merged
Eduardo Villalpando Mello (edvilme) merged 1 commit into
microsoft:mainfrom
StellaHuang95:stellahuang95-fuzzy-meme
Nov 4, 2026
Merged

Eduardo Villalpando Mello (edvilme) merged 1 commit into
microsoft:mainfrom
StellaHuang95:stellahuang95-fuzzy-meme

Conversation

@StellaHuang95

Copy link
Copy Markdown
Contributor

Summary

A global environment selection is cached together with the manager that owns it. When an explicit system interpreter was later cleared, the stale cache still sent reads back to the system manager instead of the configured venv manager, so the old system-312 value remained visible and no change event announced the correct system-314 fallback.

This change lets cached global-manager routing override the configured/default manager only while an intentional explicit global selection is active. It also marks the existing cross-manager test fixture as explicit, preserving that scenario while leaving the two failing regression tests unchanged.

Validation

  • npm run lint
  • npm run compile-tests
  • npm run unittest (3,496 passing, 9 pending)
  • npm run compile
  • npm run smoke-test -- --install-extensions ms-python.python (31 passing, 1 pending)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are recorded for the change restoring configured/default manager routing when an explicit global selection is cleared.

No open issues 🎉

What changed in this PR

Fixes global environment routing so clearing an explicit selection restores the configured/default manager instead of retaining the cached manager.

Changes:

  • Limits cached global-manager routing to active explicit global selections.
  • Marks the cross-manager test selection as explicit to preserve its intended scenario.
File Description
src/​test/​features/​envManagers.unit.test.ts Makes the cross-manager test fixture explicitly select its environment.
src/​features/​envManagers.ts Requires an explicit global selection before routing to the cached manager.

Review effort: Balanced

@StellaHuang95 Stella Huang (StellaHuang95) added the bug Issue identified by VS Code Team member as probable bug label Nov 4, 2026
@rchiodo

Copy link
Copy Markdown
Contributor

🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR.

@rchiodo

Copy link
Copy Markdown
Contributor

Result: ✅ verified

Verification details

Verification: Automated verification is best effort. The PR author remains responsible for testing.

Summary: All 29 environment-manager unit tests passed in the Linux container, including fallback reads after clearing explicit selections, change-event publication, and explicit cross-manager routing. Test compilation, lint, and the extension build also passed. Missing dependencies were restored successfully with `npm ci --offline`. The PR adds no new tests; it updates one existing fixture. Verification was targeted and did not include the full suite or Electron smoke tests.

Test runs: 6 passed

  • ✅ Passed | Test compilation | npm run compile-tests
  • ✅ Passed | Environment-manager unit tests | node ./node_modules/mocha/bin/mocha.js --no-config --no-package --require source-map-support/register --require ./out/test/unittests.js --ui tdd --timeout 180000 ./out/test/features/envManagers.unit.test.js
  • ✅ Passed | Lint | npm run lint
  • ✅ Passed | Extension build | npm run compile
  • ✅ Passed | Dependency and test discovery | printf 'Sandbox profile: %s\n' "$AUTOMATION_SANDBOX_PROFILE"; uname -s; node --version; npm --version; git diff HEAD1 HEAD --stat; git diff HEAD1 HEAD -- src/features/envManagers.ts src/test/features/envManagers.unit.test.ts; node -e 'const fs=require("fs"); const p=require("./package.json"); console.log("scripts:",JSON.stringify(p.scripts)); for(const m of ["typescript","mocha","sinon","ts-mockito","source-map-support","@renovatebot/pep440"]){try{console.log(m,require.resolve(m))}catch{console.log("MISSING",m,p.devDependencies?.[m]||p.dependencies?.[m]||"not directly declared")}} console.log("node_modules present:",fs.existsSync("node_modules")); console.log("lockfile present:",fs.existsSync("package-lock.json"));'
  • ✅ Passed | Offline dependency preparation | npm ci --offline

@rchiodo Rich Chiodo (rchiodo) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved via Review Center.

@rchiodo Rich Chiodo (rchiodo) added the review-auto:approved Automated review: no blocking findings (approval posted). label Nov 4, 2026
@edvilme
Eduardo Villalpando Mello (edvilme) merged commit a4de778 into microsoft:main Nov 4, 2026
60 of 62 checks passed
Stella Huang (StellaHuang95) added a commit that referenced this pull request Nov 4, 2026
## Summary

- map `python-envs.inlineScripts.enabled` to the dedicated
`/pythonenvs/config.python-envs.inlineScripts.enabled` treatment
- preserve the default-off setting and explicit user/workspace override
behavior
- run smoke tests in separate explicit-off and explicit-on VS Code hosts

## Validation

- `npm run lint`
- `npm run compile-tests`
- `npm run compile`
- `npm run smoke-test -- --fail-zero` — 31 passing in the default-off
host and 1 passing in the explicit-on host
- full Node 22 unit suite — 3,496 passing, 9 pending

The commit is SSH-signed and rebased onto the current `main`, including
the global-selection routing fix from #1979.

Relates to #1602

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issue identified by VS Code Team member as probable bug review-auto:approved Automated review: no blocking findings (approval posted).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants