Fix shell integration API reliability (#22446)

Kartik Raj · web-flow · commit 8d174a821a31 · 2023-11-08T23:20:29.000+01:00
#22440 It leads to terminals activating forever.
diff --git a/package.json b/package.json
@@ -23,7 +23,8 @@
         "testObserver",
         "quickPickItemTooltip",
         "saveEditor",
-        "terminalDataWriteEvent"
+        "terminalDataWriteEvent",
+        "terminalExecuteCommandEvent"
     ],
     "author": {
         "name": "Microsoft Corporation"
diff --git a/src/client/common/application/applicationShell.ts b/src/client/common/application/applicationShell.ts
@@ -39,7 +39,7 @@ import {
     WorkspaceFolderPickOptions,
 } from 'vscode';
 import { traceError } from '../../logging';
-import { IApplicationShell, TerminalDataWriteEvent } from './types';
+import { IApplicationShell, TerminalDataWriteEvent, TerminalExecutedCommand } from './types';
 
 @injectable()
 export class ApplicationShell implements IApplicationShell {
@@ -182,4 +182,12 @@ export class ApplicationShell implements IApplicationShell {
             return new EventEmitter<TerminalDataWriteEvent>().event;
         }
     }
+    public get onDidExecuteTerminalCommand(): Event<TerminalExecutedCommand> | undefined {
+        try {
+            return window.onDidExecuteTerminalCommand;
+        } catch (ex) {
+            traceError('Failed to get proposed API TerminalExecutedCommand', ex);
+            return undefined;
+        }
+    }
 }
diff --git a/src/client/common/application/types.ts b/src/client/common/application/types.ts
@@ -78,8 +78,42 @@ export interface TerminalDataWriteEvent {
     readonly data: string;
 }
 
+export interface TerminalExecutedCommand {
+    /**
+     * The {@link Terminal} the command was executed in.
+     */
+    terminal: Terminal;
+    /**
+     * The full command line that was executed, including both the command and the arguments.
+     */
+    commandLine: string | undefined;
+    /**
+     * The current working directory that was reported by the shell. This will be a {@link Uri}
+     * if the string reported by the shell can reliably be mapped to the connected machine.
+     */
+    cwd: Uri | string | undefined;
+    /**
+     * The exit code reported by the shell.
+     */
+    exitCode: number | undefined;
+    /**
+     * The output of the command when it has finished executing. This is the plain text shown in
+     * the terminal buffer and does not include raw escape sequences. Depending on the shell
+     * setup, this may include the command line as part of the output.
+     */
+    output: string | undefined;
+}
+
 export const IApplicationShell = Symbol('IApplicationShell');
 export interface IApplicationShell {
+    /**
+     * An event that is emitted when a terminal with shell integration activated has completed
+     * executing a command.
+     *
+     * Note that this event will not fire if the executed command exits the shell, listen to
+     * {@link onDidCloseTerminal} to handle that case.
+     */
+    readonly onDidExecuteTerminalCommand: Event<TerminalExecutedCommand> | undefined;
     /**
      * An [event](#Event) which fires when the focus state of the current window
      * changes. The value of the event represents whether the window is focused.
diff --git a/src/client/terminals/envCollectionActivation/service.ts b/src/client/terminals/envCollectionActivation/service.ts
@@ -37,8 +37,7 @@ import { TerminalShellType } from '../../common/terminal/types';
 import { OSType } from '../../common/utils/platform';
 import { normCase } from '../../common/platform/fs-paths';
 import { PythonEnvType } from '../../pythonEnvironments/base/info';
-import { ITerminalEnvVarCollectionService } from '../types';
-import { ShellIntegrationShells } from './shellIntegration';
+import { IShellIntegrationService, ITerminalEnvVarCollectionService } from '../types';
 import { ProgressService } from '../../common/application/progressService';
 
 @injectable()
@@ -80,6 +79,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
         @inject(IWorkspaceService) private workspaceService: IWorkspaceService,
         @inject(IConfigurationService) private readonly configurationService: IConfigurationService,
         @inject(IPathUtils) private readonly pathUtils: IPathUtils,
+        @inject(IShellIntegrationService) private readonly shellIntegrationService: IShellIntegrationService,
     ) {
         this.separator = platform.osType === OSType.Windows ? ';' : ':';
         this.progressService = new ProgressService(this.shell);
@@ -121,7 +121,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
                     this.disposables,
                 );
                 const { shell } = this.applicationEnvironment;
-                const isActive = this.isShellIntegrationActive(shell);
+                const isActive = await this.shellIntegrationService.isWorking(shell);
                 const shellType = identifyShellFromShellPath(shell);
                 if (!isActive && shellType !== TerminalShellType.commandPrompt) {
                     traceWarn(`Shell integration is not active, environment activated maybe overriden by the shell.`);
@@ -139,7 +139,10 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
             location: ProgressLocation.Window,
             title: Interpreters.activatingTerminals,
         });
-        await this._applyCollectionImpl(resource, shell);
+        await this._applyCollectionImpl(resource, shell).catch((ex) => {
+            traceError(`Failed to apply terminal env vars`, shell, ex);
+            return Promise.reject(ex); // Ensures progress indicator does not disappear in case of errors, so we can catch issues faster.
+        });
         this.progressService.hideProgress();
     }
 
@@ -184,7 +187,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
 
         // PS1 in some cases is a shell variable (not an env variable) so "env" might not contain it, calculate it in that case.
         env.PS1 = await this.getPS1(shell, resource, env);
-        const prependOptions = this.getPrependOptions(shell);
+        const prependOptions = await this.getPrependOptions(shell);
 
         // Clear any previously set env vars from collection
         envVarCollection.clear();
@@ -277,7 +280,7 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
                 // PS1 should be set but no PS1 was set.
                 return;
             }
-            const config = this.isShellIntegrationActive(shell);
+            const config = await this.shellIntegrationService.isWorking(shell);
             if (!config) {
                 traceVerbose('PS1 is not set when shell integration is disabled.');
                 return;
@@ -332,8 +335,8 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
         }
     }
 
-    private getPrependOptions(shell: string): EnvironmentVariableMutatorOptions {
-        const isActive = this.isShellIntegrationActive(shell);
+    private async getPrependOptions(shell: string): Promise<EnvironmentVariableMutatorOptions> {
+        const isActive = await this.shellIntegrationService.isWorking(shell);
         // Ideally we would want to prepend exactly once, either at shell integration or process creation.
         // TODO: Stop prepending altogether once https://github.com/microsoft/vscode/issues/145234 is available.
         return isActive
@@ -347,21 +350,6 @@ export class TerminalEnvVarCollectionService implements IExtensionActivationServ
               };
     }
 
-    private isShellIntegrationActive(shell: string): boolean {
-        const isEnabled = this.workspaceService
-            .getConfiguration('terminal')
-            .get<boolean>('integrated.shellIntegration.enabled')!;
-        if (isEnabled && ShellIntegrationShells.includes(identifyShellFromShellPath(shell))) {
-            // Unfortunately shell integration could still've failed in remote scenarios, we can't know for sure:
-            // https://code.visualstudio.com/docs/terminal/shell-integration#_automatic-script-injection
-            return true;
-        }
-        if (!isEnabled) {
-            traceVerbose('Shell integrated is disabled in user settings.');
-        }
-        return false;
-    }
-
     private getEnvironmentVariableCollection(scope: EnvironmentVariableScope = {}) {
         const envVarCollection = this.context.environmentVariableCollection as GlobalEnvironmentVariableCollection;
         return envVarCollection.getScoped(scope);
diff --git a/src/client/terminals/envCollectionActivation/shellIntegration.ts b/src/client/terminals/envCollectionActivation/shellIntegration.ts
diff --git a/src/client/terminals/envCollectionActivation/shellIntegrationService.ts b/src/client/terminals/envCollectionActivation/shellIntegrationService.ts
@@ -0,0 +1,81 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+import { injectable, inject } from 'inversify';
+import { IApplicationShell, ITerminalManager, IWorkspaceService } from '../../common/application/types';
+import { identifyShellFromShellPath } from '../../common/terminal/shellDetectors/baseShellDetector';
+import { TerminalShellType } from '../../common/terminal/types';
+import { createDeferred, sleep } from '../../common/utils/async';
+import { cache } from '../../common/utils/decorators';
+import { traceError, traceVerbose } from '../../logging';
+import { IShellIntegrationService } from '../types';
+
+/**
+ * This is a list of shells which support shell integration:
+ * https://code.visualstudio.com/docs/terminal/shell-integration
+ */
+const ShellIntegrationShells = [
+    TerminalShellType.powershell,
+    TerminalShellType.powershellCore,
+    TerminalShellType.bash,
+    TerminalShellType.zsh,
+    TerminalShellType.fish,
+];
+
+@injectable()
+export class ShellIntegrationService implements IShellIntegrationService {
+    constructor(
+        @inject(ITerminalManager) private readonly terminalManager: ITerminalManager,
+        @inject(IApplicationShell) private readonly appShell: IApplicationShell,
+        @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService,
+    ) {}
+
+    public async isWorking(shell: string): Promise<boolean> {
+        return this._isWorking(shell).catch((ex) => {
+            traceError(`Failed to determine if shell supports shell integration`, shell, ex);
+            return false;
+        });
+    }
+
+    @cache(-1, true)
+    public async _isWorking(shell: string): Promise<boolean> {
+        const isEnabled = this.workspaceService
+            .getConfiguration('terminal')
+            .get<boolean>('integrated.shellIntegration.enabled')!;
+        if (!isEnabled) {
+            traceVerbose('Shell integrated is disabled in user settings.');
+        }
+        const isSupposedToWork = isEnabled && ShellIntegrationShells.includes(identifyShellFromShellPath(shell));
+        if (!isSupposedToWork) {
+            return false;
+        }
+        const deferred = createDeferred<void>();
+        const timestamp = new Date().getTime();
+        const name = `Python ${timestamp}`;
+        const onDidExecuteTerminalCommand = this.appShell.onDidExecuteTerminalCommand?.bind(this.appShell);
+        if (!onDidExecuteTerminalCommand) {
+            // Proposed API is not available, assume shell integration is working at this point.
+            return true;
+        }
+        try {
+            const disposable = onDidExecuteTerminalCommand((e) => {
+                if (e.terminal.name === name) {
+                    deferred.resolve();
+                }
+            });
+            const terminal = this.terminalManager.createTerminal({
+                name,
+                shellPath: shell,
+                hideFromUser: true,
+            });
+            terminal.sendText(`echo ${shell}`);
+            const success = await Promise.race([sleep(3000).then(() => false), deferred.promise.then(() => true)]);
+            disposable.dispose();
+            return success;
+        } catch (ex) {
+            traceVerbose(`Proposed API is not available, failed to subscribe to onDidExecuteTerminalCommand`, ex);
+            // Proposed API is not available, assume shell integration is working at this point.
+            return true;
+        }
+    }
+}
diff --git a/src/client/terminals/serviceRegistry.ts b/src/client/terminals/serviceRegistry.ts
@@ -12,13 +12,15 @@ import {
     ICodeExecutionHelper,
     ICodeExecutionManager,
     ICodeExecutionService,
+    IShellIntegrationService,
     ITerminalAutoActivation,
     ITerminalEnvVarCollectionService,
 } from './types';
 import { TerminalEnvVarCollectionService } from './envCollectionActivation/service';
 import { IExtensionActivationService, IExtensionSingleActivationService } from '../activation/types';
 import { TerminalDeactivateLimitationPrompt } from './envCollectionActivation/deactivatePrompt';
 import { TerminalIndicatorPrompt } from './envCollectionActivation/indicatorPrompt';
+import { ShellIntegrationService } from './envCollectionActivation/shellIntegrationService';
 
 export function registerTypes(serviceManager: IServiceManager): void {
     serviceManager.addSingleton<ICodeExecutionHelper>(ICodeExecutionHelper, CodeExecutionHelper);
@@ -50,5 +52,6 @@ export function registerTypes(serviceManager: IServiceManager): void {
         IExtensionSingleActivationService,
         TerminalDeactivateLimitationPrompt,
     );
+    serviceManager.addSingleton<IShellIntegrationService>(IShellIntegrationService, ShellIntegrationService);
     serviceManager.addBinding(ITerminalEnvVarCollectionService, IExtensionActivationService);
 }
diff --git a/src/client/terminals/types.ts b/src/client/terminals/types.ts
@@ -41,3 +41,8 @@ export interface ITerminalEnvVarCollectionService {
      */
     isTerminalPromptSetCorrectly(resource?: Resource): boolean;
 }
+
+export const IShellIntegrationService = Symbol('IShellIntegrationService');
+export interface IShellIntegrationService {
+    isWorking(shell: string): Promise<boolean>;
+}
diff --git a/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts b/src/test/interpreters/activation/terminalEnvVarCollectionService.unit.test.ts
@@ -12,7 +12,6 @@ import {
     GlobalEnvironmentVariableCollection,
     ProgressLocation,
     Uri,
-    WorkspaceConfiguration,
     WorkspaceFolder,
 } from 'vscode';
 import {
@@ -38,6 +37,7 @@ import { IInterpreterService } from '../../../client/interpreter/contracts';
 import { PathUtils } from '../../../client/common/platform/pathUtils';
 import { PythonEnvType } from '../../../client/pythonEnvironments/base/info';
 import { PythonEnvironment } from '../../../client/pythonEnvironments/info';
+import { IShellIntegrationService } from '../../../client/terminals/types';
 
 suite('Terminal Environment Variable Collection Service', () => {
     let platform: IPlatformService;
@@ -50,29 +50,28 @@ suite('Terminal Environment Variable Collection Service', () => {
     let applicationEnvironment: IApplicationEnvironment;
     let environmentActivationService: IEnvironmentActivationService;
     let workspaceService: IWorkspaceService;
-    let workspaceConfig: WorkspaceConfiguration;
     let terminalEnvVarCollectionService: TerminalEnvVarCollectionService;
     const progressOptions = {
         location: ProgressLocation.Window,
         title: Interpreters.activatingTerminals,
     };
     let configService: IConfigurationService;
+    let shellIntegrationService: IShellIntegrationService;
     const displayPath = 'display/path';
     const customShell = 'powershell';
     const defaultShell = defaultShells[getOSType()];
 
     setup(() => {
         workspaceService = mock<IWorkspaceService>();
-        workspaceConfig = mock<WorkspaceConfiguration>();
         when(workspaceService.getWorkspaceFolder(anything())).thenReturn(undefined);
         when(workspaceService.workspaceFolders).thenReturn(undefined);
-        when(workspaceService.getConfiguration('terminal')).thenReturn(instance(workspaceConfig));
-        when(workspaceConfig.get<boolean>('integrated.shellIntegration.enabled')).thenReturn(true);
         platform = mock<IPlatformService>();
         when(platform.osType).thenReturn(getOSType());
         interpreterService = mock<IInterpreterService>();
         context = mock<IExtensionContext>();
         shell = mock<IApplicationShell>();
+        shellIntegrationService = mock<IShellIntegrationService>();
+        when(shellIntegrationService.isWorking(anything())).thenResolve(true);
         globalCollection = mock<GlobalEnvironmentVariableCollection>();
         collection = mock<EnvironmentVariableCollection>();
         when(context.environmentVariableCollection).thenReturn(instance(globalCollection));
@@ -108,6 +107,7 @@ suite('Terminal Environment Variable Collection Service', () => {
             instance(workspaceService),
             instance(configService),
             new PathUtils(getOSType() === OSType.Windows),
+            instance(shellIntegrationService),
         );
     });
 
@@ -445,8 +445,8 @@ suite('Terminal Environment Variable Collection Service', () => {
     });
 
     test('Correct track that prompt was set for PS1 if shell integration is disabled', async () => {
-        reset(workspaceConfig);
-        when(workspaceConfig.get<boolean>('integrated.shellIntegration.enabled')).thenReturn(false);
+        reset(shellIntegrationService);
+        when(shellIntegrationService.isWorking(anything())).thenResolve(false);
         when(platform.osType).thenReturn(OSType.Linux);
         const envVars: NodeJS.ProcessEnv = { VIRTUAL_ENV: 'prefix/to/venv', PS1: '(.venv)', ...process.env };
         const ps1Shell = 'bash';
diff --git a/src/test/terminals/serviceRegistry.unit.test.ts b/src/test/terminals/serviceRegistry.unit.test.ts
@@ -13,11 +13,13 @@ import { TerminalCodeExecutionProvider } from '../../client/terminals/codeExecut
 import { TerminalDeactivateLimitationPrompt } from '../../client/terminals/envCollectionActivation/deactivatePrompt';
 import { TerminalIndicatorPrompt } from '../../client/terminals/envCollectionActivation/indicatorPrompt';
 import { TerminalEnvVarCollectionService } from '../../client/terminals/envCollectionActivation/service';
+import { ShellIntegrationService } from '../../client/terminals/envCollectionActivation/shellIntegrationService';
 import { registerTypes } from '../../client/terminals/serviceRegistry';
 import {
     ICodeExecutionHelper,
     ICodeExecutionManager,
     ICodeExecutionService,
+    IShellIntegrationService,
     ITerminalAutoActivation,
     ITerminalEnvVarCollectionService,
 } from '../../client/terminals/types';
@@ -35,6 +37,7 @@ suite('Terminal - Service Registry', () => {
             [ITerminalEnvVarCollectionService, TerminalEnvVarCollectionService],
             [IExtensionSingleActivationService, TerminalIndicatorPrompt],
             [IExtensionSingleActivationService, TerminalDeactivateLimitationPrompt],
+            [IShellIntegrationService, ShellIntegrationService],
         ].forEach((args) => {
             if (args.length === 2) {
                 services
diff --git a/typings/vscode-proposed/vscode.proposed.terminalExecuteCommandEvent.d.ts b/typings/vscode-proposed/vscode.proposed.terminalExecuteCommandEvent.d.ts

Original file line number	Diff line number	Diff line change
`@@ -41,3 +41,8 @@ export interface ITerminalEnvVarCollectionService {`
`41`	`41`	`*/`
`42`	`42`	`isTerminalPromptSetCorrectly(resource?: Resource): boolean;`
`43`	`43`	`}`
	`44`	`+`
	`45`	`+export const IShellIntegrationService = Symbol('IShellIntegrationService');`
	`46`	`+export interface IShellIntegrationService {`
	`47`	`+ isWorking(shell: string): Promise<boolean>;`
	`48`	`+}`