Merge pull request #282471 from microsoft/copilot/fix-auto-approve-commands

Fix auto-approve to suggest commands with flags when flags precede subcommand

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/runInTerminalHelpers.ts

@@ -106,27 +106,50 @@ export function generateAutoApproveActions(commandLine: string, subCommands: str
 		// instead of `foo`)
 		const commandsWithSubSubCommands = new Set(['npm run', 'yarn run']);
 
+		// Helper function to find the first non-flag argument after a given index
+		const findNextNonFlagArg = (parts: string[], startIndex: number): number | undefined => {
+			for (let i = startIndex; i < parts.length; i++) {
+				if (!parts[i].startsWith('-')) {
+					return i;
+				}
+			}
+			return undefined;
+		};
+
 		// For each unapproved sub-command (within the overall command line), decide whether to
 		// suggest new rules for the command, a sub-command, a sub-command of a sub-command or to
 		// not suggest at all.
+		//
+		// This includes support for detecting flags between the commands, so `mvn -DskipIT test a`
+		// would suggest `mvn -DskipIT test` as that's more useful than only suggesting the exact
+		// command line.
 		const subCommandsToSuggest = Array.from(new Set(coalesce(unapprovedSubCommands.map(command => {
 			const parts = command.trim().split(/\s+/);
 			const baseCommand = parts[0].toLowerCase();
-			const baseSubCommand = parts.length > 1 ? `${parts[0]} ${parts[1]}`.toLowerCase() : '';
 
 			// Security check: Never suggest auto-approval for dangerous interpreter commands
 			if (neverAutoApproveCommands.has(baseCommand)) {
 				return undefined;
 			}
 
-			if (commandsWithSubSubCommands.has(baseSubCommand)) {
-				if (parts.length >= 3 && !parts[2].startsWith('-')) {
-					return `${parts[0]} ${parts[1]} ${parts[2]}`;
-				}
-				return undefined;
-			} else if (commandsWithSubcommands.has(baseCommand)) {
-				if (parts.length >= 2 && !parts[1].startsWith('-')) {
-					return `${parts[0]} ${parts[1]}`;
+			if (commandsWithSubcommands.has(baseCommand)) {
+				// Find the first non-flag argument after the command
+				const subCommandIndex = findNextNonFlagArg(parts, 1);
+				if (subCommandIndex !== undefined) {
+					// Check if this is a sub-sub-command case
+					const baseSubCommand = `${parts[0]} ${parts[subCommandIndex]}`.toLowerCase();
+					if (commandsWithSubSubCommands.has(baseSubCommand)) {
+						// Look for the second non-flag argument after the first subcommand
+						const subSubCommandIndex = findNextNonFlagArg(parts, subCommandIndex + 1);
+						if (subSubCommandIndex !== undefined) {
+							// Include everything from command to sub-sub-command (including flags)
+							return parts.slice(0, subSubCommandIndex + 1).join(' ');
+						}
+						return undefined;
+					} else {
+						// Include everything from command to subcommand (including flags)
+						return parts.slice(0, subCommandIndex + 1).join(' ');
+					}
 				}
 				return undefined;
 			} else {

src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/runInTerminalHelpers.test.ts

@@ -4,7 +4,7 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { ok, strictEqual } from 'assert';
-import { TRUNCATION_MESSAGE, dedupeRules, isPowerShell, sanitizeTerminalOutput, truncateOutputKeepingTail } from '../../browser/runInTerminalHelpers.js';
+import { generateAutoApproveActions, TRUNCATION_MESSAGE, dedupeRules, isPowerShell, sanitizeTerminalOutput, truncateOutputKeepingTail } from '../../browser/runInTerminalHelpers.js';
 import { OperatingSystem } from '../../../../../../base/common/platform.js';
 import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js';
 import { ConfigurationTarget } from '../../../../../../platform/configuration/common/configuration.js';
@@ -292,3 +292,175 @@ suite('sanitizeTerminalOutput', () => {
 		ok(result.endsWith('line'));
 	});
 });
+
+suite('generateAutoApproveActions', () => {
+	ensureNoDisposablesAreLeakedInTestSuite();
+
+	function createMockRule(sourceText: string): IAutoApproveRule {
+		// Escape special regex characters for test purposes to prevent regex errors
+		const escapedText = sourceText.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+		return {
+			regex: new RegExp(escapedText),
+			regexCaseInsensitive: new RegExp(escapedText, 'i'),
+			sourceText,
+			sourceTarget: ConfigurationTarget.USER,
+			isDefaultRule: false
+		};
+	}
+
+	function createMockResult(result: 'approved' | 'denied' | 'noMatch', reason: string, rule?: IAutoApproveRule): ICommandApprovalResultWithReason {
+		return {
+			result,
+			reason,
+			rule
+		};
+	}
+
+	test('should suggest mvn test when command is mvn test', () => {
+		const commandLine = 'mvn test';
+		const subCommands = ['mvn test'];
+		const autoApproveResult = {
+			subCommandResults: [createMockResult('noMatch', 'not approved')],
+			commandLineResult: createMockResult('noMatch', 'not approved')
+		};
+
+		const actions = generateAutoApproveActions(commandLine, subCommands, autoApproveResult);
+		const subCommandAction = actions.find(action => action.label.includes('mvn test'));
+		ok(subCommandAction, 'Should suggest mvn test approval');
+	});
+
+	test('should suggest mvn -DskipIT test when flags appear before subcommand', () => {
+		const commandLine = 'mvn -DskipIT test';
+		const subCommands = ['mvn -DskipIT test'];
+		const autoApproveResult = {
+			subCommandResults: [createMockResult('noMatch', 'not approved')],
+			commandLineResult: createMockResult('noMatch', 'not approved')
+		};
+
+		const actions = generateAutoApproveActions(commandLine, subCommands, autoApproveResult);
+		const subCommandAction = actions.find(action => action.label.includes('mvn -DskipIT test'));
+		ok(subCommandAction, 'Should suggest mvn -DskipIT test approval (including flags)');
+	});
+
+	test('should suggest mvn -X -DskipIT test when multiple flags appear before subcommand', () => {
+		const commandLine = 'mvn -X -DskipIT test';
+		const subCommands = ['mvn -X -DskipIT test'];
+		const autoApproveResult = {
+			subCommandResults: [createMockResult('noMatch', 'not approved')],
+			commandLineResult: createMockResult('noMatch', 'not approved')
+		};
+
+		const actions = generateAutoApproveActions(commandLine, subCommands, autoApproveResult);
+		const subCommandAction = actions.find(action => action.label.includes('mvn -X -DskipIT test'));
+		ok(subCommandAction, 'Should suggest mvn -X -DskipIT test approval with multiple flags');
+	});
+
+	test('should suggest gradle --info build when flags appear before subcommand', () => {
+		const commandLine = 'gradle --info build';
+		const subCommands = ['gradle --info build'];
+		const autoApproveResult = {
+			subCommandResults: [createMockResult('noMatch', 'not approved')],
+			commandLineResult: createMockResult('noMatch', 'not approved')
+		};
+
+		const actions = generateAutoApproveActions(commandLine, subCommands, autoApproveResult);
+		const subCommandAction = actions.find(action => action.label.includes('gradle --info build'));
+		ok(subCommandAction, 'Should suggest gradle --info build approval');
+	});
+
+	test('should suggest npm --silent run test when flags appear before subcommand', () => {
+		const commandLine = 'npm --silent run test';
+		const subCommands = ['npm --silent run test'];
+		const autoApproveResult = {
+			subCommandResults: [createMockResult('noMatch', 'not approved')],
+			commandLineResult: createMockResult('noMatch', 'not approved')
+		};
+
+		const actions = generateAutoApproveActions(commandLine, subCommands, autoApproveResult);
+		const subCommandAction = actions.find(action => action.label.includes('npm --silent run test'));
+		ok(subCommandAction, 'Should suggest npm --silent run test approval (sub-sub-command with flags)');
+	});
+
+	test('should suggest npm --silent run --verbose test when flags appear between subcommands', () => {
+		const commandLine = 'npm --silent run --verbose test';
+		const subCommands = ['npm --silent run --verbose test'];
+		const autoApproveResult = {
+			subCommandResults: [createMockResult('noMatch', 'not approved')],
+			commandLineResult: createMockResult('noMatch', 'not approved')
+		};
+
+		const actions = generateAutoApproveActions(commandLine, subCommands, autoApproveResult);
+		const subCommandAction = actions.find(action => action.label.includes('npm --silent run --verbose test'));
+		ok(subCommandAction, 'Should suggest npm --silent run --verbose test with flags between subcommands');
+	});
+
+	test('should not suggest approval when only flags and no subcommand', () => {
+		const commandLine = 'mvn -X -DskipIT';
+		const subCommands = ['mvn -X -DskipIT'];
+		const autoApproveResult = {
+			subCommandResults: [createMockResult('noMatch', 'not approved')],
+			commandLineResult: createMockResult('noMatch', 'not approved')
+		};
+
+		const actions = generateAutoApproveActions(commandLine, subCommands, autoApproveResult);
+		const subCommandAction = actions.find(action => action.label.includes('Always Allow Command:') && action.label.includes('mvn'));
+		strictEqual(subCommandAction, undefined, 'Should not suggest mvn approval when no subcommand found');
+	});
+
+	test('should suggest exact command line when subcommand cannot be extracted', () => {
+		const commandLine = 'mvn -X -DskipIT';
+		const subCommands = ['mvn -X -DskipIT'];
+		const autoApproveResult = {
+			subCommandResults: [createMockResult('noMatch', 'not approved')],
+			commandLineResult: createMockResult('noMatch', 'not approved')
+		};
+
+		const actions = generateAutoApproveActions(commandLine, subCommands, autoApproveResult);
+		const exactCommandAction = actions.find(action => action.label.includes('Always Allow Exact Command Line'));
+		ok(exactCommandAction, 'Should suggest exact command line approval');
+	});
+
+	test('should handle multiple subcommands with flags', () => {
+		const commandLine = 'mvn -DskipIT test && gradle --info build';
+		const subCommands = ['mvn -DskipIT test', 'gradle --info build'];
+		const autoApproveResult = {
+			subCommandResults: [
+				createMockResult('noMatch', 'not approved'),
+				createMockResult('noMatch', 'not approved')
+			],
+			commandLineResult: createMockResult('noMatch', 'not approved')
+		};
+
+		const actions = generateAutoApproveActions(commandLine, subCommands, autoApproveResult);
+		const subCommandAction = actions.find(action =>
+			action.label.includes('mvn -DskipIT test') && action.label.includes('gradle --info build')
+		);
+		ok(subCommandAction, 'Should suggest both mvn -DskipIT test and gradle --info build');
+	});
+
+	test('should not suggest when commands are denied', () => {
+		const commandLine = 'mvn -DskipIT test';
+		const subCommands = ['mvn -DskipIT test'];
+		const autoApproveResult = {
+			subCommandResults: [createMockResult('denied', 'denied by rule', createMockRule('mvn test'))],
+			commandLineResult: createMockResult('noMatch', 'not approved')
+		};
+
+		const actions = generateAutoApproveActions(commandLine, subCommands, autoApproveResult);
+		const subCommandAction = actions.find(action => action.label.includes('Always Allow Command:'));
+		strictEqual(subCommandAction, undefined, 'Should not suggest approval for denied commands');
+	});
+
+	test('should not suggest when commands are already approved', () => {
+		const commandLine = 'mvn -DskipIT test';
+		const subCommands = ['mvn -DskipIT test'];
+		const autoApproveResult = {
+			subCommandResults: [createMockResult('approved', 'approved by rule', createMockRule('mvn test'))],
+			commandLineResult: createMockResult('noMatch', 'not approved')
+		};
+
+		const actions = generateAutoApproveActions(commandLine, subCommands, autoApproveResult);
+		const subCommandAction = actions.find(action => action.label.includes('mvn -DskipIT test') && action.label.includes('Always Allow Command:'));
+		strictEqual(subCommandAction, undefined, 'Should not suggest approval for already approved commands');
+	});
+});