From 71a396c2d3f936dfc97f11e52bc72689943370f0 Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Wed, 17 Dec 2025 09:22:10 -0800 Subject: [PATCH 1/3] Be less aggressive about removing Python hooks in shell profile --- .../terminal/shells/bash/bashStartup.ts | 6 - .../terminal/shells/fish/fishStartup.ts | 6 - .../terminal/shells/pwsh/pwshStartup.ts | 10 -- src/features/terminal/terminalManager.ts | 17 +-- ...inalManager.shellStartupCache.unit.test.ts | 68 +++++++++++ .../terminal/terminalManager.unit.test.ts | 108 ++++++++++++++++++ 6 files changed, 182 insertions(+), 33 deletions(-) create mode 100644 src/test/features/terminal/terminalManager.shellStartupCache.unit.test.ts create mode 100644 src/test/features/terminal/terminalManager.unit.test.ts diff --git a/src/features/terminal/shells/bash/bashStartup.ts b/src/features/terminal/shells/bash/bashStartup.ts index db9be723..bbfb198c 100644 --- a/src/features/terminal/shells/bash/bashStartup.ts +++ b/src/features/terminal/shells/bash/bashStartup.ts @@ -5,7 +5,6 @@ import which from 'which'; import { traceError, traceInfo, traceVerbose } from '../../../../common/logging'; import { ShellConstants } from '../../../common/shellConstants'; import { hasStartupCode, insertStartupCode, removeStartupCode } from '../common/editUtils'; -import { getShellIntegrationEnabledCache, isWsl, shellIntegrationForActiveTerminal } from '../common/shellUtils'; import { ShellScriptEditState, ShellSetupState, ShellStartupScriptProvider } from '../startupProvider'; import { BASH_ENV_KEY, BASH_OLD_ENV_KEY, BASH_SCRIPT_VERSION, ZSH_ENV_KEY, ZSH_OLD_ENV_KEY } from './bashConstants'; @@ -69,11 +68,6 @@ async function isStartupSetup(profile: string, key: string): Promise { - const shellIntegrationEnabled = await getShellIntegrationEnabledCache(); - if ((shellIntegrationEnabled || (await shellIntegrationForActiveTerminal(name, profile))) && !isWsl()) { - removeStartup(profile, key); - return true; - } const activationContent = getActivationContent(key); try { if (await fs.pathExists(profile)) { diff --git a/src/features/terminal/shells/fish/fishStartup.ts b/src/features/terminal/shells/fish/fishStartup.ts index 40de3e98..139e4497 100644 --- a/src/features/terminal/shells/fish/fishStartup.ts +++ b/src/features/terminal/shells/fish/fishStartup.ts @@ -6,7 +6,6 @@ import which from 'which'; import { traceError, traceInfo, traceVerbose } from '../../../../common/logging'; import { ShellConstants } from '../../../common/shellConstants'; import { hasStartupCode, insertStartupCode, removeStartupCode } from '../common/editUtils'; -import { getShellIntegrationEnabledCache, isWsl, shellIntegrationForActiveTerminal } from '../common/shellUtils'; import { ShellScriptEditState, ShellSetupState, ShellStartupScriptProvider } from '../startupProvider'; import { FISH_ENV_KEY, FISH_OLD_ENV_KEY, FISH_SCRIPT_VERSION } from './fishConstants'; @@ -58,11 +57,6 @@ async function isStartupSetup(profilePath: string, key: string): Promise { try { - const shellIntegrationEnabled = await getShellIntegrationEnabledCache(); - if ((shellIntegrationEnabled || (await shellIntegrationForActiveTerminal('fish', profilePath))) && !isWsl()) { - removeFishStartup(profilePath, key); - return true; - } const activationContent = getActivationContent(key); await fs.mkdirp(path.dirname(profilePath)); diff --git a/src/features/terminal/shells/pwsh/pwshStartup.ts b/src/features/terminal/shells/pwsh/pwshStartup.ts index 45bb33d5..7ad86f44 100644 --- a/src/features/terminal/shells/pwsh/pwshStartup.ts +++ b/src/features/terminal/shells/pwsh/pwshStartup.ts @@ -13,11 +13,8 @@ import { ShellConstants } from '../../../common/shellConstants'; import { hasStartupCode, insertStartupCode, removeStartupCode } from '../common/editUtils'; import { extractProfilePath, - getShellIntegrationEnabledCache, - isWsl, PROFILE_TAG_END, PROFILE_TAG_START, - shellIntegrationForActiveTerminal, } from '../common/shellUtils'; import { POWERSHELL_ENV_KEY, POWERSHELL_OLD_ENV_KEY, PWSH_SCRIPT_VERSION } from './pwshConstants'; @@ -169,13 +166,6 @@ async function isPowerShellStartupSetup(shell: string, profile: string): Promise } async function setupPowerShellStartup(shell: string, profile: string): Promise { - const shellIntegrationEnabled = await getShellIntegrationEnabledCache(); - - if ((shellIntegrationEnabled || (await shellIntegrationForActiveTerminal(shell, profile))) && !isWsl()) { - removePowerShellStartup(shell, profile, POWERSHELL_OLD_ENV_KEY); - removePowerShellStartup(shell, profile, POWERSHELL_ENV_KEY); - return true; - } const activationContent = getActivationContent(); try { diff --git a/src/features/terminal/terminalManager.ts b/src/features/terminal/terminalManager.ts index 394cc8db..34fd2dde 100644 --- a/src/features/terminal/terminalManager.ts +++ b/src/features/terminal/terminalManager.ts @@ -185,27 +185,22 @@ export class TerminalManagerImpl implements TerminalManager { ); if (shellIntegrationLikelyAvailable && !shouldUseProfileActivation(p.shellType)) { - // Shell integration available and NOT in WSL - skip setup - await p.teardownScripts(); + // Shell integration available and NOT in WSL - skip setup. + // NOTE: We intentionally do NOT teardown scripts here. If the user stays in + // shellStartup mode, be less aggressive about clearing profile modifications. this.shellSetup.set(p.shellType, true); traceVerbose( - `Shell integration available for ${p.shellType} (not WSL), skipping prompt, and profile modification.`, + `Shell integration likely available. Skipping setup of shell profile for ${p.shellType}.`, ); } else { - // WSL (regardless of integration) OR no shell integration - needs setup + // WSL (regardless of integration) OR no/disabled shell integration - needs setup this.shellSetup.set(p.shellType, false); shellsToSetup.push(p); traceVerbose( - `Shell integration is NOT available. Shell profile for ${p.shellType} is not setup.`, + `Shell integration is NOT available or disabled. Shell profile for ${p.shellType} is not setup.`, ); } } else if (state === ShellSetupState.Setup) { - if (shellIntegrationLikelyAvailable && !shouldUseProfileActivation(p.shellType)) { - await p.teardownScripts(); - traceVerbose( - `Shell integration available for ${p.shellType}, removed profile script in favor of shell integration.`, - ); - } this.shellSetup.set(p.shellType, true); traceVerbose(`Shell profile for ${p.shellType} is setup.`); } else if (state === ShellSetupState.NotInstalled) { diff --git a/src/test/features/terminal/terminalManager.shellStartupCache.unit.test.ts b/src/test/features/terminal/terminalManager.shellStartupCache.unit.test.ts new file mode 100644 index 00000000..c841ddd2 --- /dev/null +++ b/src/test/features/terminal/terminalManager.shellStartupCache.unit.test.ts @@ -0,0 +1,68 @@ +import * as sinon from 'sinon'; +import { EventEmitter } from 'vscode'; +import { TerminalManagerImpl } from '../../../features/terminal/terminalManager'; +import { ShellSetupState, ShellStartupScriptProvider } from '../../../features/terminal/shells/startupProvider'; +import * as workspaceApis from '../../../common/workspace.apis'; + +suite('TerminalManager - shellStartup caching/clearing', () => { + let disposables: sinon.SinonStub[] = []; + + teardown(() => { + disposables.forEach((d) => d.restore()); + disposables = []; + }); + + function createProvider(shellType: string, state: ShellSetupState): ShellStartupScriptProvider { + return { + name: shellType, + shellType, + isSetup: sinon.stub().resolves(state), + setupScripts: sinon.stub().resolves(undefined), + teardownScripts: sinon.stub().resolves(undefined), + clearCache: sinon.stub().resolves(undefined), + } as unknown as ShellStartupScriptProvider; + } + + test('does not teardown scripts just because shell integration setting changes', async () => { + const configEmitter = new EventEmitter<{ affectsConfiguration: (s: string) => boolean }>(); + + const onDidChangeConfigurationStub = sinon + .stub(workspaceApis, 'onDidChangeConfiguration') + .callsFake((listener) => { + const sub = configEmitter.event(listener); + return { dispose: () => sub.dispose() }; + }); + disposables.push(onDidChangeConfigurationStub); + + // TerminalManager constructor wires a bunch of window event listeners too; stub them to no-ops. + const windowApis = require('../../../common/window.apis') as typeof import('../../../common/window.apis'); + disposables.push( + sinon.stub(windowApis, 'onDidOpenTerminal').returns({ dispose: () => undefined } as any), + sinon.stub(windowApis, 'onDidCloseTerminal').returns({ dispose: () => undefined } as any), + sinon.stub(windowApis, 'onDidChangeWindowState').returns({ dispose: () => undefined } as any), + sinon.stub(windowApis, 'terminals').returns([] as any), + ); + + const shellUtils = + require('../../../features/terminal/shells/common/shellUtils') as typeof import('../../../features/terminal/shells/common/shellUtils'); + disposables.push(sinon.stub(shellUtils, 'getShellIntegrationEnabledCache').resolves(true)); + + const ta = { + onDidChangeTerminalActivationState: () => ({ dispose: () => undefined }), + getEnvironment: () => undefined, + } as any; + + const provider = createProvider('bash', ShellSetupState.NotSetup); + const tm = new TerminalManagerImpl(ta, [], [provider]); + + // Trigger shell integration setting change + configEmitter.fire({ + affectsConfiguration: (s: string) => s === 'terminal.integrated.shellIntegration.enabled', + }); + + // Previously we would sometimes teardown scripts here; now we should not. + sinon.assert.notCalled(provider.teardownScripts as any); + + tm.dispose(); + }); +}); diff --git a/src/test/features/terminal/terminalManager.unit.test.ts b/src/test/features/terminal/terminalManager.unit.test.ts new file mode 100644 index 00000000..194f0a20 --- /dev/null +++ b/src/test/features/terminal/terminalManager.unit.test.ts @@ -0,0 +1,108 @@ +import * as assert from 'assert'; +import * as sinon from 'sinon'; + +import { EventEmitter } from 'vscode'; + +import * as windowApis from '../../../common/window.apis'; +import * as workspaceApis from '../../../common/workspace.apis'; +import { TerminalManagerImpl } from '../../../features/terminal/terminalManager'; +import { + ShellScriptEditState, + ShellSetupState, + ShellStartupScriptProvider, +} from '../../../features/terminal/shells/startupProvider'; +import { ACT_TYPE_COMMAND } from '../../../features/terminal/utils'; +import * as terminalUtils from '../../../features/terminal/utils'; +import { + DidChangeTerminalActivationStateEvent, + TerminalActivationInternal, +} from '../../../features/terminal/terminalActivationState'; + +type DisposableLike = { dispose(): void }; + +suite('TerminalManager shellStartup profile behavior', () => { + let sandbox: sinon.SinonSandbox; + let onDidChangeConfigurationHandler: + | ((e: { affectsConfiguration: (section: string) => boolean }) => void | Promise) + | undefined; + + setup(() => { + sandbox = sinon.createSandbox(); + onDidChangeConfigurationHandler = undefined; + + const disposable: DisposableLike = { dispose() {} }; + + sandbox.stub(windowApis, 'onDidOpenTerminal').returns(disposable as any); + sandbox.stub(windowApis, 'onDidCloseTerminal').returns(disposable as any); + sandbox.stub(windowApis, 'onDidChangeWindowState').returns(disposable as any); + + sandbox.stub(workspaceApis, 'onDidChangeConfiguration').callsFake((handler: any) => { + onDidChangeConfigurationHandler = handler; + return disposable as any; + }); + + // Avoid any real window focus concerns. + sandbox.stub(windowApis, 'terminals').returns([] as any); + }); + + teardown(() => { + sandbox.restore(); + }); + + function createTerminalManager(startupScriptProviders: ShellStartupScriptProvider[]): TerminalManagerImpl { + const emitter = new EventEmitter(); + const ta: TerminalActivationInternal = { + onDidChangeTerminalActivationState: emitter.event, + isActivated: () => false, + getEnvironment: () => undefined, + activate: async () => undefined, + deactivate: async () => undefined, + updateActivationState: () => undefined, + dispose: () => emitter.dispose(), + }; + + return new TerminalManagerImpl(ta, [], startupScriptProviders); + } + + test('does not tear down profile scripts when shellStartup is setup', async () => { + const provider: ShellStartupScriptProvider = { + name: 'bash', + shellType: 'bash', + isSetup: sandbox.stub().resolves(ShellSetupState.Setup), + setupScripts: sandbox.stub().resolves(ShellScriptEditState.Edited), + teardownScripts: sandbox.stub().resolves(ShellScriptEditState.Edited), + clearCache: sandbox.stub().resolves(), + }; + + const tm = createTerminalManager([provider]); + await (tm as any).handleSetupCheck('bash'); + + sinon.assert.notCalled(provider.teardownScripts as sinon.SinonStub); + assert.strictEqual((tm as any).shellSetup.get('bash'), true); + }); + + test('clears profile scripts when switching from shellStartup to command', async () => { + const provider: ShellStartupScriptProvider = { + name: 'bash', + shellType: 'bash', + isSetup: sandbox.stub().resolves(ShellSetupState.Setup), + setupScripts: sandbox.stub().resolves(ShellScriptEditState.Edited), + teardownScripts: sandbox.stub().resolves(ShellScriptEditState.Edited), + clearCache: sandbox.stub().resolves(), + }; + + sandbox.stub(terminalUtils, 'getAutoActivationType').returns(ACT_TYPE_COMMAND); + + const tm = createTerminalManager([provider]); + // Seed a cached setup state so we can verify it is cleared. + (tm as any).shellSetup.set('bash', true); + + assert.ok(onDidChangeConfigurationHandler, 'Expected onDidChangeConfiguration handler to be registered'); + await onDidChangeConfigurationHandler!({ + affectsConfiguration: (section: string) => section === 'python-envs.terminal.autoActivationType', + }); + + sinon.assert.calledOnce(provider.teardownScripts as sinon.SinonStub); + assert.strictEqual((tm as any).shellSetup.size, 0); + }); +}); From 8a0dede2c42c50c8497e558d11558ff68194b97a Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Wed, 17 Dec 2025 09:25:00 -0800 Subject: [PATCH 2/3] linter --- ...inalManager.shellStartupCache.unit.test.ts | 68 ----------- .../terminal/terminalManager.unit.test.ts | 108 ------------------ 2 files changed, 176 deletions(-) delete mode 100644 src/test/features/terminal/terminalManager.shellStartupCache.unit.test.ts delete mode 100644 src/test/features/terminal/terminalManager.unit.test.ts diff --git a/src/test/features/terminal/terminalManager.shellStartupCache.unit.test.ts b/src/test/features/terminal/terminalManager.shellStartupCache.unit.test.ts deleted file mode 100644 index c841ddd2..00000000 --- a/src/test/features/terminal/terminalManager.shellStartupCache.unit.test.ts +++ /dev/null @@ -1,68 +0,0 @@ -import * as sinon from 'sinon'; -import { EventEmitter } from 'vscode'; -import { TerminalManagerImpl } from '../../../features/terminal/terminalManager'; -import { ShellSetupState, ShellStartupScriptProvider } from '../../../features/terminal/shells/startupProvider'; -import * as workspaceApis from '../../../common/workspace.apis'; - -suite('TerminalManager - shellStartup caching/clearing', () => { - let disposables: sinon.SinonStub[] = []; - - teardown(() => { - disposables.forEach((d) => d.restore()); - disposables = []; - }); - - function createProvider(shellType: string, state: ShellSetupState): ShellStartupScriptProvider { - return { - name: shellType, - shellType, - isSetup: sinon.stub().resolves(state), - setupScripts: sinon.stub().resolves(undefined), - teardownScripts: sinon.stub().resolves(undefined), - clearCache: sinon.stub().resolves(undefined), - } as unknown as ShellStartupScriptProvider; - } - - test('does not teardown scripts just because shell integration setting changes', async () => { - const configEmitter = new EventEmitter<{ affectsConfiguration: (s: string) => boolean }>(); - - const onDidChangeConfigurationStub = sinon - .stub(workspaceApis, 'onDidChangeConfiguration') - .callsFake((listener) => { - const sub = configEmitter.event(listener); - return { dispose: () => sub.dispose() }; - }); - disposables.push(onDidChangeConfigurationStub); - - // TerminalManager constructor wires a bunch of window event listeners too; stub them to no-ops. - const windowApis = require('../../../common/window.apis') as typeof import('../../../common/window.apis'); - disposables.push( - sinon.stub(windowApis, 'onDidOpenTerminal').returns({ dispose: () => undefined } as any), - sinon.stub(windowApis, 'onDidCloseTerminal').returns({ dispose: () => undefined } as any), - sinon.stub(windowApis, 'onDidChangeWindowState').returns({ dispose: () => undefined } as any), - sinon.stub(windowApis, 'terminals').returns([] as any), - ); - - const shellUtils = - require('../../../features/terminal/shells/common/shellUtils') as typeof import('../../../features/terminal/shells/common/shellUtils'); - disposables.push(sinon.stub(shellUtils, 'getShellIntegrationEnabledCache').resolves(true)); - - const ta = { - onDidChangeTerminalActivationState: () => ({ dispose: () => undefined }), - getEnvironment: () => undefined, - } as any; - - const provider = createProvider('bash', ShellSetupState.NotSetup); - const tm = new TerminalManagerImpl(ta, [], [provider]); - - // Trigger shell integration setting change - configEmitter.fire({ - affectsConfiguration: (s: string) => s === 'terminal.integrated.shellIntegration.enabled', - }); - - // Previously we would sometimes teardown scripts here; now we should not. - sinon.assert.notCalled(provider.teardownScripts as any); - - tm.dispose(); - }); -}); diff --git a/src/test/features/terminal/terminalManager.unit.test.ts b/src/test/features/terminal/terminalManager.unit.test.ts deleted file mode 100644 index 194f0a20..00000000 --- a/src/test/features/terminal/terminalManager.unit.test.ts +++ /dev/null @@ -1,108 +0,0 @@ -import * as assert from 'assert'; -import * as sinon from 'sinon'; - -import { EventEmitter } from 'vscode'; - -import * as windowApis from '../../../common/window.apis'; -import * as workspaceApis from '../../../common/workspace.apis'; -import { TerminalManagerImpl } from '../../../features/terminal/terminalManager'; -import { - ShellScriptEditState, - ShellSetupState, - ShellStartupScriptProvider, -} from '../../../features/terminal/shells/startupProvider'; -import { ACT_TYPE_COMMAND } from '../../../features/terminal/utils'; -import * as terminalUtils from '../../../features/terminal/utils'; -import { - DidChangeTerminalActivationStateEvent, - TerminalActivationInternal, -} from '../../../features/terminal/terminalActivationState'; - -type DisposableLike = { dispose(): void }; - -suite('TerminalManager shellStartup profile behavior', () => { - let sandbox: sinon.SinonSandbox; - let onDidChangeConfigurationHandler: - | ((e: { affectsConfiguration: (section: string) => boolean }) => void | Promise) - | undefined; - - setup(() => { - sandbox = sinon.createSandbox(); - onDidChangeConfigurationHandler = undefined; - - const disposable: DisposableLike = { dispose() {} }; - - sandbox.stub(windowApis, 'onDidOpenTerminal').returns(disposable as any); - sandbox.stub(windowApis, 'onDidCloseTerminal').returns(disposable as any); - sandbox.stub(windowApis, 'onDidChangeWindowState').returns(disposable as any); - - sandbox.stub(workspaceApis, 'onDidChangeConfiguration').callsFake((handler: any) => { - onDidChangeConfigurationHandler = handler; - return disposable as any; - }); - - // Avoid any real window focus concerns. - sandbox.stub(windowApis, 'terminals').returns([] as any); - }); - - teardown(() => { - sandbox.restore(); - }); - - function createTerminalManager(startupScriptProviders: ShellStartupScriptProvider[]): TerminalManagerImpl { - const emitter = new EventEmitter(); - const ta: TerminalActivationInternal = { - onDidChangeTerminalActivationState: emitter.event, - isActivated: () => false, - getEnvironment: () => undefined, - activate: async () => undefined, - deactivate: async () => undefined, - updateActivationState: () => undefined, - dispose: () => emitter.dispose(), - }; - - return new TerminalManagerImpl(ta, [], startupScriptProviders); - } - - test('does not tear down profile scripts when shellStartup is setup', async () => { - const provider: ShellStartupScriptProvider = { - name: 'bash', - shellType: 'bash', - isSetup: sandbox.stub().resolves(ShellSetupState.Setup), - setupScripts: sandbox.stub().resolves(ShellScriptEditState.Edited), - teardownScripts: sandbox.stub().resolves(ShellScriptEditState.Edited), - clearCache: sandbox.stub().resolves(), - }; - - const tm = createTerminalManager([provider]); - await (tm as any).handleSetupCheck('bash'); - - sinon.assert.notCalled(provider.teardownScripts as sinon.SinonStub); - assert.strictEqual((tm as any).shellSetup.get('bash'), true); - }); - - test('clears profile scripts when switching from shellStartup to command', async () => { - const provider: ShellStartupScriptProvider = { - name: 'bash', - shellType: 'bash', - isSetup: sandbox.stub().resolves(ShellSetupState.Setup), - setupScripts: sandbox.stub().resolves(ShellScriptEditState.Edited), - teardownScripts: sandbox.stub().resolves(ShellScriptEditState.Edited), - clearCache: sandbox.stub().resolves(), - }; - - sandbox.stub(terminalUtils, 'getAutoActivationType').returns(ACT_TYPE_COMMAND); - - const tm = createTerminalManager([provider]); - // Seed a cached setup state so we can verify it is cleared. - (tm as any).shellSetup.set('bash', true); - - assert.ok(onDidChangeConfigurationHandler, 'Expected onDidChangeConfiguration handler to be registered'); - await onDidChangeConfigurationHandler!({ - affectsConfiguration: (section: string) => section === 'python-envs.terminal.autoActivationType', - }); - - sinon.assert.calledOnce(provider.teardownScripts as sinon.SinonStub); - assert.strictEqual((tm as any).shellSetup.size, 0); - }); -}); From a4e80b76b7dceecccb73aad336b0220b26ac8a76 Mon Sep 17 00:00:00 2001 From: anthonykim1 Date: Wed, 17 Dec 2025 10:09:44 -0800 Subject: [PATCH 3/3] Remove listener for shell integration setting change --- src/features/terminal/terminalManager.ts | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/features/terminal/terminalManager.ts b/src/features/terminal/terminalManager.ts index 34fd2dde..79e31769 100644 --- a/src/features/terminal/terminalManager.ts +++ b/src/features/terminal/terminalManager.ts @@ -142,20 +142,6 @@ export class TerminalManagerImpl implements TerminalManager { this.shellSetup.clear(); } } - if (e.affectsConfiguration('terminal.integrated.shellIntegration.enabled')) { - traceInfo('Shell integration setting changed, invalidating cache'); - const updatedShellIntegrationSetting = await getShellIntegrationEnabledCache(); - if (!updatedShellIntegrationSetting) { - const shells = new Set( - terminals() - .map((t) => identifyTerminalShell(t)) - .filter((t) => t !== 'unknown'), - ); - if (shells.size > 0) { - await this.handleSetupCheck(shells); - } - } - } }), onDidChangeWindowState((e) => { this.hasFocus = e.focused;