Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions src/features/terminal/terminalManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
AutoActivationType,
getAutoActivationType,
getEnvironmentForTerminal,
shouldActivateInCurrentTerminal,
waitForShellIntegration,
} from './utils';

Expand Down Expand Up @@ -405,8 +406,21 @@ export class TerminalManagerImpl implements TerminalManager {

public async initialize(api: PythonEnvironmentApi): Promise<void> {
const actType = getAutoActivationType();

// When activateEnvInCurrentTerminal is explicitly false,
Copy link
Member

Choose a reason for hiding this comment

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

@karthiknadig can you confirm the meaning of activateEnvInCurrentTerminal here- does that mean all that are currently open or the one the user is in?

Copy link
Member

Choose a reason for hiding this comment

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

As far as I remember it is one which VSCode says is the focused terminal.

We should confirm this by seeing what Python ext specifically looks at.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed focused terminal.
The thing is Python extension did not activate any terminal is activateEnvInCurrentTerminal is false.

Environment tries to activate ALL terminal by default, and with this PR (see the gif), we won't activate current (focused AND previous terminals in background) to more smoothly match Python ext behavior.

// skip activation for ALL pre-existing terminals (terminals open before extension load).
// New terminals opened after extension load are still activated via autoActivateOnTerminalOpen.
const skipPreExistingTerminals = !shouldActivateInCurrentTerminal() && terminals().length > 0;
if (skipPreExistingTerminals) {
traceVerbose(
'python.terminal.activateEnvInCurrentTerminal is explicitly disabled, skipping activation for pre-existing terminals',
);
}

if (actType === ACT_TYPE_COMMAND) {
await Promise.all(terminals().map(async (t) => this.activateUsingCommand(api, t)));
if (!skipPreExistingTerminals) {
await Promise.all(terminals().map(async (t) => this.activateUsingCommand(api, t)));
}
Comment on lines +413 to +423
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

initialize() calls terminals() multiple times (including while computing skipPreExistingTerminals and later when iterating). Capture a single snapshot (e.g., const existing = terminals();) and use it throughout initialize() so behavior is consistent and avoids repeated lookups while async work is in-flight.

Copilot uses AI. Check for mistakes.
} else if (actType === ACT_TYPE_SHELL) {
const shells = new Set(
terminals()
Expand All @@ -415,14 +429,16 @@ export class TerminalManagerImpl implements TerminalManager {
);
if (shells.size > 0) {
await this.handleSetupCheck(shells);
await Promise.all(
terminals().map(async (t) => {
// If the shell is not set up, we activate using command fallback.
if (this.shellSetup.get(identifyTerminalShell(t)) === false) {
await this.activateUsingCommand(api, t);
}
}),
);
if (!skipPreExistingTerminals) {
await Promise.all(
terminals().map(async (t) => {
// If the shell is not set up, we activate using command fallback.
if (this.shellSetup.get(identifyTerminalShell(t)) === false) {
await this.activateUsingCommand(api, t);
}
}),
);
}
}
}
}
Expand Down
46 changes: 46 additions & 0 deletions src/features/terminal/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,52 @@ export async function setAutoActivationType(value: AutoActivationType): Promise<
return await config.update('terminal.autoActivationType', value, true);
}

/**
* Determines whether activation commands should be sent to pre-existing terminals
* (terminals open before extension load).
*
* Checks the legacy `python.terminal.activateEnvInCurrentTerminal` setting using `inspect()`
* to distinguish between the default value and an explicitly user-set value.
*
* Priority: workspaceFolderValue > workspaceValue > globalRemoteValue > globalLocalValue > globalValue
* (matches the precedence used by getShellIntegrationEnabledCache and getAutoActivationType)
*
* - If the user has explicitly set the value to `false` at any scope, returns `false`.
* - Otherwise (default or explicitly `true`), returns `true`.
*
* @returns `false` only when the user has explicitly set the setting to `false`; `true` otherwise.
*/
export function shouldActivateInCurrentTerminal(): boolean {
const pythonConfig = getConfiguration('python');
const inspected = pythonConfig.inspect<boolean>('terminal.activateEnvInCurrentTerminal');

if (!inspected) {
return true;
}

// Only respect `false` when the user has deliberately set it.
// Priority: workspaceFolder > workspace > globalRemote > globalLocal > global
const inspectValue = inspected as Record<string, unknown>;

if (inspected.workspaceFolderValue === false) {
return false;
}
if (inspected.workspaceValue === false) {
return false;
}
if ('globalRemoteValue' in inspected && inspectValue.globalRemoteValue === false) {
return false;
}
if ('globalLocalValue' in inspected && inspectValue.globalLocalValue === false) {
return false;
}
if (inspected.globalValue === false) {
return false;
}
Comment on lines +292 to +306
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

shouldActivateInCurrentTerminal() does not apply VS Code settings precedence correctly: a lower-precedence globalValue === false will force false even when a higher-precedence scope (workspace/workspaceFolder) explicitly sets true. Instead, follow the same pattern as getShellIntegrationEnabledCache(): return the first defined boolean in precedence order (workspaceFolderValue > workspaceValue > globalRemoteValue > globalLocalValue > globalValue), and default to true only when none are defined.

This issue also appears on line 281 of the same file.

Copilot uses AI. Check for mistakes.

return true;
}

export async function getAllDistinctProjectEnvironments(
api: PythonProjectGetterApi & PythonProjectEnvironmentApi,
): Promise<PythonEnvironment[] | undefined> {
Expand Down
126 changes: 126 additions & 0 deletions src/test/features/terminal/terminalManager.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -421,3 +421,129 @@ suite('TerminalManager - terminal naming', () => {
}
});
});

suite('TerminalManager - initialize() with activateEnvInCurrentTerminal', () => {
let terminalActivation: TestTerminalActivation;
let terminalManager: TerminalManagerImpl;
let mockGetAutoActivationType: sinon.SinonStub;
let mockShouldActivateInCurrentTerminal: sinon.SinonStub;
let mockTerminals: sinon.SinonStub;
let mockGetEnvironmentForTerminal: sinon.SinonStub;

const createMockTerminal = (name: string): Terminal =>
({
name,
creationOptions: {} as TerminalOptions,
shellIntegration: undefined,
show: sinon.stub(),
sendText: sinon.stub(),
}) as unknown as Terminal;

const createMockEnvironment = (): PythonEnvironment => ({
envId: { id: 'test-env-id', managerId: 'test-manager' },
name: 'Test Environment',
displayName: 'Test Environment',
shortDisplayName: 'TestEnv',
displayPath: '/path/to/env',
version: '3.9.0',
environmentPath: Uri.file('/path/to/python'),
sysPrefix: '/path/to/env',
execInfo: {
run: { executable: '/path/to/python' },
activation: [{ executable: '/path/to/activate' }],
},
});

setup(() => {
terminalActivation = new TestTerminalActivation();

mockGetAutoActivationType = sinon.stub(terminalUtils, 'getAutoActivationType');
mockShouldActivateInCurrentTerminal = sinon.stub(terminalUtils, 'shouldActivateInCurrentTerminal');
mockGetEnvironmentForTerminal = sinon.stub(terminalUtils, 'getEnvironmentForTerminal');
sinon.stub(terminalUtils, 'waitForShellIntegration').resolves(false);
sinon.stub(activationUtils, 'isActivatableEnvironment').returns(true);
sinon.stub(shellDetector, 'identifyTerminalShell').returns('bash');

sinon.stub(windowApis, 'createTerminal').callsFake(() => createMockTerminal('new'));
sinon.stub(windowApis, 'onDidOpenTerminal').returns(new Disposable(() => {}));
sinon.stub(windowApis, 'onDidCloseTerminal').returns(new Disposable(() => {}));
sinon.stub(windowApis, 'onDidChangeWindowState').returns(new Disposable(() => {}));
sinon.stub(windowApis, 'activeTerminal').returns(undefined);

mockTerminals = sinon.stub(windowApis, 'terminals');

sinon.stub(windowApis, 'withProgress').callsFake(async (_options, task) => {
const mockProgress = { report: () => {} };
const mockToken = {
isCancellationRequested: false,
onCancellationRequested: () => new Disposable(() => {}),
};
return task(mockProgress as never, mockToken as never);
});
sinon.stub(workspaceApis, 'onDidChangeConfiguration').returns(new Disposable(() => {}));
});

teardown(() => {
sinon.restore();
terminalActivation.dispose();
});

function createTerminalManager(): TerminalManagerImpl {
return new TerminalManagerImpl(terminalActivation, [], []);
}

test('initialize activates all pre-existing terminals when shouldActivateInCurrentTerminal returns true', async () => {
const terminal1 = createMockTerminal('terminal1');
const terminal2 = createMockTerminal('terminal2');
const env = createMockEnvironment();

mockGetAutoActivationType.returns(terminalUtils.ACT_TYPE_COMMAND);
mockShouldActivateInCurrentTerminal.returns(true);
mockTerminals.returns([terminal1, terminal2]);
mockGetEnvironmentForTerminal.resolves(env);
Comment on lines +495 to +503
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

initialize() behavior was changed for both ACT_TYPE_COMMAND and ACT_TYPE_SHELL, but tests only exercise the command path. Add coverage for ACT_TYPE_SHELL to ensure activation fallback is also skipped (or performed) correctly when shouldActivateInCurrentTerminal() is false/true.

Copilot uses AI. Check for mistakes.

terminalManager = createTerminalManager();
await terminalManager.initialize({} as never);

assert.strictEqual(
terminalActivation.activateCalls,
2,
'Should activate all pre-existing terminals when activateEnvInCurrentTerminal is true/default',
);
});

test('initialize skips all pre-existing terminals when shouldActivateInCurrentTerminal returns false', async () => {
const terminal1 = createMockTerminal('terminal1');
const terminal2 = createMockTerminal('terminal2');
const env = createMockEnvironment();

mockGetAutoActivationType.returns(terminalUtils.ACT_TYPE_COMMAND);
mockShouldActivateInCurrentTerminal.returns(false);
mockTerminals.returns([terminal1, terminal2]);
mockGetEnvironmentForTerminal.resolves(env);

terminalManager = createTerminalManager();
await terminalManager.initialize({} as never);

assert.strictEqual(
terminalActivation.activateCalls,
0,
'Should skip all pre-existing terminals when activateEnvInCurrentTerminal is explicitly false',
);
});

test('initialize proceeds normally when shouldActivateInCurrentTerminal returns false but no pre-existing terminals', async () => {
mockGetAutoActivationType.returns(terminalUtils.ACT_TYPE_COMMAND);
mockShouldActivateInCurrentTerminal.returns(false);
mockTerminals.returns([]);

terminalManager = createTerminalManager();
await terminalManager.initialize({} as never);

assert.strictEqual(
terminalActivation.activateCalls,
0,
'Should have no activations when there are no terminals',
);
});
});
Loading
Loading