From 21e6118b0c433f157efed93592f04f13a158181e Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sat, 1 Nov 2025 07:39:17 -0700 Subject: [PATCH] Resolve cwd for analyzers/rewriters in single place Part of #274496 --- .../commandLineFileWriteAnalyzer.ts | 9 +-- .../commandLineCdPrefixRewriter.ts | 53 +++++---------- .../browser/tools/runInTerminalTool.ts | 22 +++++-- .../commandLineCdPrefixRewriter.test.ts | 65 +++++++------------ 4 files changed, 60 insertions(+), 89 deletions(-) diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts index 367a1c918ab..6365137c2bd 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineAnalyzer/commandLineFileWriteAnalyzer.ts @@ -8,7 +8,6 @@ import { URI } from '../../../../../../../base/common/uri.js'; import { localize } from '../../../../../../../nls.js'; import { IConfigurationService } from '../../../../../../../platform/configuration/common/configuration.js'; import { IWorkspaceContextService } from '../../../../../../../platform/workspace/common/workspace.js'; -import { IHistoryService } from '../../../../../../services/history/common/history.js'; import { TerminalChatAgentToolsSettingId } from '../../../common/terminalChatAgentToolsConfiguration.js'; import type { TreeSitterCommandParser } from '../../treeSitterCommandParser.js'; import type { ICommandLineAnalyzer, ICommandLineAnalyzerOptions, ICommandLineAnalyzerResult } from './commandLineAnalyzer.js'; @@ -18,7 +17,6 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand private readonly _treeSitterCommandParser: TreeSitterCommandParser, private readonly _log: (message: string, ...args: unknown[]) => void, @IConfigurationService private readonly _configurationService: IConfigurationService, - @IHistoryService private readonly _historyService: IHistoryService, @IWorkspaceContextService private readonly _workspaceContextService: IWorkspaceContextService, ) { super(); @@ -34,12 +32,7 @@ export class CommandLineFileWriteAnalyzer extends Disposable implements ICommand // TODO: Handle environment variables https://github.com/microsoft/vscode/issues/274166 // TODO: Handle command substitions/complex destinations https://github.com/microsoft/vscode/issues/274167 if (capturedFileWrites.length) { - let cwd = options.cwd; - if (!cwd) { - const activeWorkspaceRootUri = this._historyService.getLastActiveWorkspaceRoot(); - const workspaceFolder = activeWorkspaceRootUri ? this._workspaceContextService.getWorkspaceFolder(activeWorkspaceRootUri) ?? undefined : undefined; - cwd = workspaceFolder?.uri; - } + const cwd = options.cwd; if (cwd) { fileWrites = capturedFileWrites.map(e => URI.joinPath(cwd, e)); } else { diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineRewriter/commandLineCdPrefixRewriter.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineRewriter/commandLineCdPrefixRewriter.ts index b990d05aa4e..9fece148ea2 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineRewriter/commandLineCdPrefixRewriter.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineRewriter/commandLineCdPrefixRewriter.ts @@ -5,19 +5,15 @@ import { Disposable } from '../../../../../../../base/common/lifecycle.js'; import { OperatingSystem } from '../../../../../../../base/common/platform.js'; -import type { URI } from '../../../../../../../base/common/uri.js'; -import { IWorkspaceContextService } from '../../../../../../../platform/workspace/common/workspace.js'; import { isPowerShell } from '../../runInTerminalHelpers.js'; import type { ICommandLineRewriter, ICommandLineRewriterOptions, ICommandLineRewriterResult } from './commandLineRewriter.js'; export class CommandLineCdPrefixRewriter extends Disposable implements ICommandLineRewriter { - constructor( - @IWorkspaceContextService private readonly _workspaceContextService: IWorkspaceContextService, - ) { - super(); - } - rewrite(options: ICommandLineRewriterOptions): ICommandLineRewriterResult | undefined { + if (!options.cwd) { + return undefined; + } + const isPwsh = isPowerShell(options.shell, options.os); // Re-write the command if it starts with `cd && ` or `cd ; ` @@ -31,34 +27,21 @@ export class CommandLineCdPrefixRewriter extends Disposable implements ICommandL const cdDir = cdPrefixMatch?.groups?.dir; const cdSuffix = cdPrefixMatch?.groups?.suffix; if (cdDir && cdSuffix) { - let cwd: URI | undefined = options.cwd; - - // If a terminal is not available, use the workspace root - if (!cwd) { - const workspaceFolders = this._workspaceContextService.getWorkspace().folders; - if (workspaceFolders.length === 1) { - cwd = workspaceFolders[0].uri; - } + // Remove any surrounding quotes + let cdDirPath = cdDir; + if (cdDirPath.startsWith('"') && cdDirPath.endsWith('"')) { + cdDirPath = cdDirPath.slice(1, -1); } - - // Re-write the command if it matches the cwd - if (cwd) { - // Remove any surrounding quotes - let cdDirPath = cdDir; - if (cdDirPath.startsWith('"') && cdDirPath.endsWith('"')) { - cdDirPath = cdDirPath.slice(1, -1); - } - // Normalize trailing slashes - cdDirPath = cdDirPath.replace(/(?:[\\\/])$/, ''); - let cwdFsPath = cwd.fsPath.replace(/(?:[\\\/])$/, ''); - // Case-insensitive comparison on Windows - if (options.os === OperatingSystem.Windows) { - cdDirPath = cdDirPath.toLowerCase(); - cwdFsPath = cwdFsPath.toLowerCase(); - } - if (cdDirPath === cwdFsPath) { - return { rewritten: cdSuffix, reasoning: 'Removed redundant cd command' }; - } + // Normalize trailing slashes + cdDirPath = cdDirPath.replace(/(?:[\\\/])$/, ''); + let cwdFsPath = options.cwd.fsPath.replace(/(?:[\\\/])$/, ''); + // Case-insensitive comparison on Windows + if (options.os === OperatingSystem.Windows) { + cdDirPath = cdDirPath.toLowerCase(); + cwdFsPath = cwdFsPath.toLowerCase(); + } + if (cdDirPath === cwdFsPath) { + return { rewritten: cdSuffix, reasoning: 'Removed redundant cd command' }; } } return undefined; diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts index 37471db5eba..bd57554d1cd 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts @@ -48,6 +48,8 @@ import { LocalChatSessionUri } from '../../../../chat/common/chatUri.js'; import type { ICommandLineRewriter } from './commandLineRewriter/commandLineRewriter.js'; import { CommandLineCdPrefixRewriter } from './commandLineRewriter/commandLineCdPrefixRewriter.js'; import { CommandLinePwshChainOperatorRewriter } from './commandLineRewriter/commandLinePwshChainOperatorRewriter.js'; +import { IWorkspaceContextService } from '../../../../../../platform/workspace/common/workspace.js'; +import { IHistoryService } from '../../../../../services/history/common/history.js'; // #region Tool data @@ -272,15 +274,17 @@ export class RunInTerminalTool extends Disposable implements IToolImpl { } constructor( - @IInstantiationService private readonly _instantiationService: IInstantiationService, + @IChatService private readonly _chatService: IChatService, @IConfigurationService private readonly _configurationService: IConfigurationService, + @IHistoryService private readonly _historyService: IHistoryService, + @IInstantiationService private readonly _instantiationService: IInstantiationService, @ILanguageModelToolsService private readonly _languageModelToolsService: ILanguageModelToolsService, + @IRemoteAgentService private readonly _remoteAgentService: IRemoteAgentService, @IStorageService private readonly _storageService: IStorageService, + @ITerminalChatService private readonly _terminalChatService: ITerminalChatService, @ITerminalLogService private readonly _logService: ITerminalLogService, @ITerminalService private readonly _terminalService: ITerminalService, - @ITerminalChatService private readonly _terminalChatService: ITerminalChatService, - @IRemoteAgentService private readonly _remoteAgentService: IRemoteAgentService, - @IChatService private readonly _chatService: IChatService, + @IWorkspaceContextService private readonly _workspaceContextService: IWorkspaceContextService, ) { super(); @@ -331,11 +335,17 @@ export class RunInTerminalTool extends Disposable implements IToolImpl { async prepareToolInvocation(context: IToolInvocationPreparationContext, token: CancellationToken): Promise { const args = context.parameters as IRunInTerminalInputParams; - const instance = context.chatSessionId ? this._sessionTerminalAssociations.get(context.chatSessionId)?.instance : undefined; const os = await this._osBackend; const shell = await this._profileFetcher.getCopilotShell(); const language = os === OperatingSystem.Windows ? 'pwsh' : 'sh'; - const cwd = await instance?.getCwdResource(); + + const instance = context.chatSessionId ? this._sessionTerminalAssociations.get(context.chatSessionId)?.instance : undefined; + let cwd = await instance?.getCwdResource(); + if (!cwd) { + const activeWorkspaceRootUri = this._historyService.getLastActiveWorkspaceRoot(); + const workspaceFolder = activeWorkspaceRootUri ? this._workspaceContextService.getWorkspaceFolder(activeWorkspaceRootUri) ?? undefined : undefined; + cwd = workspaceFolder?.uri; + } const terminalToolSessionId = generateUuid(); // Generate a custom command ID to link the command between renderer and pty host diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineCdPrefixRewriter.test.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineCdPrefixRewriter.test.ts index 8e3b5da0407..d440896b361 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineCdPrefixRewriter.test.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineCdPrefixRewriter.test.ts @@ -5,13 +5,10 @@ import { strictEqual } from 'assert'; import { isWindows, OperatingSystem } from '../../../../../../base/common/platform.js'; +import { URI } from '../../../../../../base/common/uri.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js'; import type { TestInstantiationService } from '../../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; -import { TestContextService } from '../../../../../test/common/workbenchTestServices.js'; -import { IWorkspaceContextService, toWorkspaceFolder } from '../../../../../../platform/workspace/common/workspace.js'; import { workbenchInstantiationService } from '../../../../../test/browser/workbenchTestServices.js'; -import { URI } from '../../../../../../base/common/uri.js'; -import { Workspace } from '../../../../../../platform/workspace/test/common/testWorkspace.js'; import { CommandLineCdPrefixRewriter } from '../../browser/tools/commandLineRewriter/commandLineCdPrefixRewriter.js'; import type { ICommandLineRewriterOptions } from '../../browser/tools/commandLineRewriter/commandLineRewriter.js'; @@ -19,7 +16,6 @@ suite('CommandLineCdPrefixRewriter', () => { const store = ensureNoDisposablesAreLeakedInTestSuite(); let instantiationService: TestInstantiationService; - let workspaceService: TestContextService; let rewriter: CommandLineCdPrefixRewriter; function createRewriteOptions(command: string, cwd: URI | undefined, shell: string, os: OperatingSystem): ICommandLineRewriterOptions { @@ -31,22 +27,16 @@ suite('CommandLineCdPrefixRewriter', () => { }; } - function setWorkspaceFolders(folders: URI[]) { - workspaceService.setWorkspace(new Workspace(folders.reduce((prev, curr) => { - return `${prev},${curr}`; - }, ''), folders.map(e => toWorkspaceFolder(e)))); - } - setup(() => { instantiationService = workbenchInstantiationService({}, store); - workspaceService = instantiationService.get(IWorkspaceContextService) as TestContextService; rewriter = store.add(instantiationService.createInstance(CommandLineCdPrefixRewriter)); }); suite('cd && -> ', () => { (!isWindows ? suite : suite.skip)('Posix', () => { - function t(commandLine: string, cwd: URI | undefined, workspaceFolders: URI[], shell: string, expectedResult: string | undefined) { - setWorkspaceFolders(workspaceFolders); + const cwd = URI.file('/test/workspace'); + + function t(commandLine: string, shell: string, expectedResult: string | undefined) { const options = createRewriteOptions(commandLine, cwd, shell, OperatingSystem.Linux); const result = rewriter.rewrite(options); strictEqual(result?.rewritten, expectedResult); @@ -55,21 +45,20 @@ suite('CommandLineCdPrefixRewriter', () => { } } - test('should return undefined when no cd prefix pattern matches', () => t('echo hello world', undefined, [], 'bash', undefined)); - test('should return undefined when cd pattern does not have suffix', () => t('cd /some/path', undefined, [], 'bash', undefined)); - test('should rewrite command with ; separator when directory matches cwd', () => t('cd /test/workspace; npm test', undefined, [URI.file('/test/workspace')], 'pwsh', 'npm test')); - test('should rewrite command with && separator when directory matches cwd', () => t('cd /test/workspace && npm install', undefined, [URI.file('/test/workspace')], 'bash', 'npm install')); - test('should rewrite command when the path is wrapped in double quotes', () => t('cd "/test/workspace" && npm install', undefined, [URI.file('/test/workspace')], 'bash', 'npm install')); - test('should not rewrite command when directory does not match cwd', () => t('cd /different/path && npm install', undefined, [URI.file('/test/workspace')], 'bash', undefined)); - test('should return undefined when no workspace folders available', () => t('cd /some/path && npm install', undefined, [], 'bash', undefined)); - test('should return undefined when multiple workspace folders available', () => t('cd /some/path && npm install', undefined, [URI.file('/workspace1'), URI.file('/workspace2')], 'bash', undefined)); - test('should handle commands with complex suffixes', () => t('cd /test/workspace && npm install && npm test && echo "done"', undefined, [URI.file('/test/workspace')], 'bash', 'npm install && npm test && echo "done"')); - test('should ignore any trailing forward slash', () => t('cd /test/workspace/ && npm install', undefined, [URI.file('/test/workspace')], 'bash', 'npm install')); + test('should return undefined when no cd prefix pattern matches', () => t('echo hello world', 'bash', undefined)); + test('should return undefined when cd pattern does not have suffix', () => t('cd /some/path', 'bash', undefined)); + test('should rewrite command with ; separator when directory matches cwd', () => t('cd /test/workspace; npm test', 'pwsh', 'npm test')); + test('should rewrite command with && separator when directory matches cwd', () => t('cd /test/workspace && npm install', 'bash', 'npm install')); + test('should rewrite command when the path is wrapped in double quotes', () => t('cd "/test/workspace" && npm install', 'bash', 'npm install')); + test('should not rewrite command when directory does not match cwd', () => t('cd /different/path && npm install', 'bash', undefined)); + test('should handle commands with complex suffixes', () => t('cd /test/workspace && npm install && npm test && echo "done"', 'bash', 'npm install && npm test && echo "done"')); + test('should ignore any trailing forward slash', () => t('cd /test/workspace/ && npm install', 'bash', 'npm install')); }); (isWindows ? suite : suite.skip)('Windows', () => { - function t(commandLine: string, cwd: URI | undefined, workspaceFolders: URI[], shell: string, expectedResult: string | undefined) { - setWorkspaceFolders(workspaceFolders); + const cwd = URI.file('C:\\test\\workspace'); + + function t(commandLine: string, shell: string, expectedResult: string | undefined) { const options = createRewriteOptions(commandLine, cwd, shell, OperatingSystem.Windows); const result = rewriter.rewrite(options); strictEqual(result?.rewritten, expectedResult); @@ -78,20 +67,16 @@ suite('CommandLineCdPrefixRewriter', () => { } } - test('should ignore any trailing back slash', () => t('cd c:\\test\\workspace\\ && npm install', undefined, [URI.file('c:\\test\\workspace')], 'cmd', 'npm install')); - test('should prioritize cwd option over workspace service', () => t('cd C:\\cwd\\workspace && npm test', URI.file('C:\\cwd\\workspace'), [URI.file('C:\\workspace\\service')], 'cmd', 'npm test')); - test('should prioritize cwd option over workspace service - PowerShell style', () => t('cd C:\\cwd\\workspace; npm test', URI.file('C:\\cwd\\workspace'), [URI.file('C:\\workspace\\service')], 'pwsh', 'npm test')); - test('should not rewrite when cwd differs from cd path', () => t('cd C:\\different\\path && npm test', URI.file('C:\\cwd\\workspace'), [URI.file('C:\\workspace\\service')], 'cmd', undefined)); - test('should fallback to workspace service when cwd is undefined', () => t('cd C:\\workspace\\service && npm test', undefined, [URI.file('C:\\workspace\\service')], 'cmd', 'npm test')); - test('should prioritize cwd over workspace service even when both match cd path', () => t('cd C:\\shared\\workspace && npm build', URI.file('C:\\shared\\workspace'), [URI.file('C:\\shared\\workspace')], 'cmd', 'npm build')); - test('should handle case-insensitive comparison on Windows', () => t('cd c:\\cwd\\workspace && npm test', URI.file('C:\\Cwd\\Workspace'), [], 'cmd', 'npm test')); - test('should handle quoted paths with cwd priority', () => t('cd "C:\\cwd\\workspace" && npm test', URI.file('C:\\cwd\\workspace'), [URI.file('C:\\different\\workspace')], 'cmd', 'npm test')); - test('should handle cd /d flag when directory matches cwd', () => t('cd /d C:\\test\\workspace && echo hello', undefined, [URI.file('C:\\test\\workspace')], 'pwsh', 'echo hello')); - test('should handle cd /d flag with quoted paths when directory matches cwd', () => t('cd /d "C:\\test\\workspace" && echo hello', undefined, [URI.file('C:\\test\\workspace')], 'pwsh', 'echo hello')); - test('should handle cd /d flag with quoted paths from issue example', () => t('cd /d "d:\\microsoft\\vscode" && .\\scripts\\test.bat', undefined, [URI.file('d:\\microsoft\\vscode')], 'pwsh', '.\\scripts\\test.bat')); - test('should not rewrite cd /d when directory does not match cwd', () => t('cd /d C:\\different\\path ; echo hello', undefined, [URI.file('C:\\test\\workspace')], 'pwsh', undefined)); - test('should handle cd /d flag with cwd priority', () => t('cd /d C:\\cwd\\workspace && npm test', URI.file('C:\\cwd\\workspace'), [URI.file('C:\\workspace\\service')], 'pwsh', 'npm test')); - test('should handle cd /d flag with semicolon separator', () => t('cd /d C:\\test\\workspace; echo hello', undefined, [URI.file('C:\\test\\workspace')], 'pwsh', 'echo hello')); + test('should ignore any trailing back slash', () => t('cd c:\\test\\workspace\\ && npm install', 'cmd', 'npm install')); + test('should rewrite command with && separator when directory matches cwd', () => t('cd C:\\test\\workspace && npm test', 'cmd', 'npm test')); + test('should rewrite command with ; separator when directory matches cwd - PowerShell style', () => t('cd C:\\test\\workspace; npm test', 'pwsh', 'npm test')); + test('should not rewrite when cwd differs from cd path', () => t('cd C:\\different\\path && npm test', 'cmd', undefined)); + test('should handle case-insensitive comparison on Windows', () => t('cd c:\\test\\workspace && npm test', 'cmd', 'npm test')); + test('should handle quoted paths', () => t('cd "C:\\test\\workspace" && npm test', 'cmd', 'npm test')); + test('should handle cd /d flag when directory matches cwd', () => t('cd /d C:\\test\\workspace && echo hello', 'pwsh', 'echo hello')); + test('should handle cd /d flag with quoted paths when directory matches cwd', () => t('cd /d "C:\\test\\workspace" && echo hello', 'pwsh', 'echo hello')); + test('should not rewrite cd /d when directory does not match cwd', () => t('cd /d C:\\different\\path ; echo hello', 'pwsh', undefined)); + test('should handle cd /d flag with semicolon separator', () => t('cd /d C:\\test\\workspace; echo hello', 'pwsh', 'echo hello')); }); }); });