Skip to content

Commit 62f0505

Browse files
TyriarTyriar
authored
Merge pull request microsoft#264927 from microsoft/tyriar/264918
Prevent commands with sub commands being offered to auto approve
2 parents 6cb3870 + 0c599e2 commit 62f0505

2 files changed

Lines changed: 78 additions & 43 deletions

File tree

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

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import { Separator } from '../../../../../base/common/actions.js';
7+
import { coalesce } from '../../../../../base/common/arrays.js';
78
import { posix as pathPosix, win32 as pathWin32 } from '../../../../../base/common/path.js';
89
import { OperatingSystem } from '../../../../../base/common/platform.js';
910
import { removeAnsiEscapeCodes } from '../../../../../base/common/strings.js';
@@ -60,19 +61,25 @@ export function generateAutoApproveActions(commandLine: string, subCommands: str
6061
// suggest just the commnad or sub-command (with that sub-command line) to always allow.
6162
const commandsWithSubcommands = new Set(['git', 'npm', 'yarn', 'docker', 'kubectl', 'cargo', 'dotnet', 'mvn', 'gradle']);
6263
const commandsWithSubSubCommands = new Set(['npm run', 'yarn run']);
63-
const subCommandsToSuggest = Array.from(new Set(unapprovedSubCommands.map(command => {
64+
const subCommandsToSuggest = Array.from(new Set(coalesce(unapprovedSubCommands.map(command => {
6465
const parts = command.trim().split(/\s+/);
6566
const baseCommand = parts[0].toLowerCase();
6667
const baseSubCommand = parts.length > 1 ? `${parts[0]} ${parts[1]}`.toLowerCase() : '';
6768

68-
if (commandsWithSubSubCommands.has(baseSubCommand) && parts.length >= 3 && !parts[2].startsWith('-')) {
69-
return `${parts[0]} ${parts[1]} ${parts[2]}`;
70-
} else if (commandsWithSubcommands.has(baseCommand) && parts.length >= 2 && !parts[1].startsWith('-')) {
71-
return `${parts[0]} ${parts[1]}`;
69+
if (commandsWithSubSubCommands.has(baseSubCommand)) {
70+
if (parts.length >= 3 && !parts[2].startsWith('-')) {
71+
return `${parts[0]} ${parts[1]} ${parts[2]}`;
72+
}
73+
return undefined;
74+
} else if (commandsWithSubcommands.has(baseCommand)) {
75+
if (parts.length >= 2 && !parts[1].startsWith('-')) {
76+
return `${parts[0]} ${parts[1]}`;
77+
}
78+
return undefined;
7279
} else {
7380
return parts[0];
7481
}
75-
})));
82+
}))));
7683

7784
if (subCommandsToSuggest.length > 0) {
7885
let subCommandLabel: string;
@@ -96,9 +103,13 @@ export function generateAutoApproveActions(commandLine: string, subCommands: str
96103
}
97104

98105
// Allow exact command line, don't do this if it's just the first sub-command's first
99-
// word
106+
// word or if it's an exact match for special sub-commands
100107
const firstSubcommandFirstWord = unapprovedSubCommands.length > 0 ? unapprovedSubCommands[0].split(' ')[0] : '';
101-
if (firstSubcommandFirstWord !== commandLine) {
108+
if (
109+
firstSubcommandFirstWord !== commandLine &&
110+
!commandsWithSubcommands.has(commandLine) &&
111+
!commandsWithSubSubCommands.has(commandLine)
112+
) {
102113
actions.push({
103114
label: localize('autoApprove.exactCommand', 'Always Allow Exact Command Line'),
104115
data: {

‎src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/runInTerminalTool.test.ts‎

Lines changed: 59 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -485,8 +485,8 @@ suite('RunInTerminalTool', () => {
485485

486486
test('should generate custom actions for single word commands', async () => {
487487
const result = await executeToolTest({
488-
command: 'git',
489-
explanation: 'Run git command'
488+
command: 'foo',
489+
explanation: 'Run foo command'
490490
});
491491

492492
assertConfirmationRequired(result);
@@ -497,7 +497,7 @@ suite('RunInTerminalTool', () => {
497497
strictEqual(customActions.length, 3);
498498

499499
ok(!isSeparator(customActions[0]));
500-
strictEqual(customActions[0].label, 'Always Allow Command: git');
500+
strictEqual(customActions[0].label, 'Always Allow Command: foo');
501501
strictEqual(customActions[0].data.type, 'newRule');
502502
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
503503

@@ -724,8 +724,8 @@ suite('RunInTerminalTool', () => {
724724

725725
test('should not suggest subcommand for commands with flags', async () => {
726726
const result = await executeToolTest({
727-
command: 'npm --foo --bar',
728-
explanation: 'Run npm with flags'
727+
command: 'foo --foo --bar',
728+
explanation: 'Run foo with flags'
729729
});
730730

731731
assertConfirmationRequired(result);
@@ -735,35 +735,16 @@ suite('RunInTerminalTool', () => {
735735
strictEqual(customActions.length, 4);
736736

737737
ok(!isSeparator(customActions[0]));
738-
strictEqual(customActions[0].label, 'Always Allow Command: npm');
738+
strictEqual(customActions[0].label, 'Always Allow Command: foo');
739739
strictEqual(customActions[0].data.type, 'newRule');
740740
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
741-
strictEqual((customActions[0].data.rule as any)[0].key, 'npm');
742-
});
743-
744-
test('should not suggest subcommand for git commands with flags', async () => {
745-
const result = await executeToolTest({
746-
command: 'git --version',
747-
explanation: 'Check git version'
748-
});
749-
750-
assertConfirmationRequired(result);
751-
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
752-
753-
const customActions = result!.confirmationMessages!.terminalCustomActions!;
754-
strictEqual(customActions.length, 4);
755-
756-
ok(!isSeparator(customActions[0]));
757-
strictEqual(customActions[0].label, 'Always Allow Command: git');
758-
strictEqual(customActions[0].data.type, 'newRule');
759-
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
760-
strictEqual((customActions[0].data.rule as any)[0].key, 'git');
741+
strictEqual((customActions[0].data.rule as any)[0].key, 'foo');
761742
});
762743

763744
test('should not suggest subcommand for npm run with flags', async () => {
764745
const result = await executeToolTest({
765-
command: 'npm run --some-flag',
766-
explanation: 'Run npm run with flags'
746+
command: 'npm run abc --some-flag',
747+
explanation: 'Run npm run abc with flags'
767748
});
768749

769750
assertConfirmationRequired(result);
@@ -773,10 +754,10 @@ suite('RunInTerminalTool', () => {
773754
strictEqual(customActions.length, 4);
774755

775756
ok(!isSeparator(customActions[0]));
776-
strictEqual(customActions[0].label, 'Always Allow Command: npm run');
757+
strictEqual(customActions[0].label, 'Always Allow Command: npm run abc');
777758
strictEqual(customActions[0].data.type, 'newRule');
778759
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
779-
strictEqual((customActions[0].data.rule as any)[0].key, 'npm run');
760+
strictEqual((customActions[0].data.rule as any)[0].key, 'npm run abc');
780761
});
781762

782763
test('should handle mixed npm run and other commands', async () => {
@@ -874,13 +855,11 @@ suite('RunInTerminalTool', () => {
874855
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
875856

876857
const customActions = result!.confirmationMessages!.terminalCustomActions!;
877-
strictEqual(customActions.length, 3); // No full command line suggestion for single word
858+
strictEqual(customActions.length, 1);
878859

879860
ok(!isSeparator(customActions[0]));
880-
strictEqual(customActions[0].label, 'Always Allow Command: git');
881-
strictEqual(customActions[0].data.type, 'newRule');
882-
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
883-
strictEqual((customActions[0].data.rule as any)[0].key, 'git');
861+
strictEqual(customActions[0].label, 'Configure Auto Approve...');
862+
strictEqual(customActions[0].data.type, 'configure');
884863
});
885864

886865
test('should deduplicate identical subcommand suggestions', async () => {
@@ -904,6 +883,51 @@ suite('RunInTerminalTool', () => {
904883
strictEqual(rules[0].key, 'npm test');
905884
});
906885

886+
test('should handle flags differently than subcommands for suggestion logic', async () => {
887+
// Test that commands with actual subcommands get subcommand suggestions
888+
const resultWithSubcommand = await executeToolTest({
889+
command: 'npm install express',
890+
explanation: 'Install express package'
891+
});
892+
893+
assertConfirmationRequired(resultWithSubcommand);
894+
ok(resultWithSubcommand!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
895+
896+
const actionsWithSubcommand = resultWithSubcommand!.confirmationMessages!.terminalCustomActions!;
897+
strictEqual(actionsWithSubcommand.length, 4);
898+
899+
ok(!isSeparator(actionsWithSubcommand[0]));
900+
strictEqual(actionsWithSubcommand[0].label, 'Always Allow Command: npm install');
901+
strictEqual(actionsWithSubcommand[0].data.type, 'newRule');
902+
903+
ok(!isSeparator(actionsWithSubcommand[1]));
904+
strictEqual(actionsWithSubcommand[1].label, 'Always Allow Exact Command Line');
905+
strictEqual(actionsWithSubcommand[1].data.type, 'newRule');
906+
907+
// Test that commands with flags don't get subcommand suggestions
908+
const resultWithFlags = await executeToolTest({
909+
command: 'npm --version',
910+
explanation: 'Check npm version'
911+
});
912+
913+
assertConfirmationRequired(resultWithFlags);
914+
ok(resultWithFlags!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
915+
916+
const actionsWithFlags = resultWithFlags!.confirmationMessages!.terminalCustomActions!;
917+
strictEqual(actionsWithFlags.length, 3); // No subcommand suggestion, only exact command line
918+
919+
ok(!isSeparator(actionsWithFlags[0]));
920+
strictEqual(actionsWithFlags[0].label, 'Always Allow Exact Command Line');
921+
strictEqual(actionsWithFlags[0].data.type, 'newRule');
922+
ok(!Array.isArray(actionsWithFlags[0].data.rule), 'Expected rule to be an object for exact command line');
923+
924+
ok(isSeparator(actionsWithFlags[1]));
925+
926+
ok(!isSeparator(actionsWithFlags[2]));
927+
strictEqual(actionsWithFlags[2].label, 'Configure Auto Approve...');
928+
strictEqual(actionsWithFlags[2].data.type, 'configure');
929+
});
930+
907931
});
908932

909933
suite('chat session disposal cleanup', () => {

0 commit comments

Comments
 (0)