Skip to content

Commit c162220

Browse files
TyriarTyriar
authored
Merge pull request microsoft#261270 from microsoft/tyriar/261126_subsubcommand
Handle things like npm test and npm run build specially
2 parents 97bc9a4 + d04cd41 commit c162220

File tree

3 files changed

+249
-23
lines changed

3 files changed

+249
-23
lines changed

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

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -892,25 +892,42 @@ export class RunInTerminalTool extends Disposable implements IToolImpl {
892892
return autoApproveResult.subCommandResults[index].result !== 'approved';
893893
});
894894

895-
const subCommandsFirstWordOnly = Array.from(new Set(unapprovedSubCommands.map(command => command.split(' ')[0])));
896-
let subCommandLabel: string;
897-
let subCommandTooltip: string;
898-
if (subCommandsFirstWordOnly.length === 1) {
899-
subCommandLabel = localize('autoApprove.baseCommandSingle', 'Always Allow Command: {0}', subCommandsFirstWordOnly[0]);
900-
subCommandTooltip = localize('autoApprove.baseCommandSingleTooltip', 'Always allow command starting with `{0}` to run without confirmation', subCommandsFirstWordOnly[0]);
901-
} else {
902-
const commandSeparated = subCommandsFirstWordOnly.join(', ');
903-
subCommandLabel = localize('autoApprove.baseCommand', 'Always Allow Commands: {0}', commandSeparated);
904-
subCommandTooltip = localize('autoApprove.baseCommandTooltip', 'Always allow commands starting with `{0}` to run without confirmation', commandSeparated);
905-
}
895+
// For each unapproved sub-command (within the overall command line), decide whether to
896+
// suggest just the commnad or sub-command (with that sub-command line) to always allow.
897+
const commandsWithSubcommands = new Set(['git', 'npm', 'yarn', 'docker', 'kubectl', 'cargo', 'dotnet', 'mvn', 'gradle']);
898+
const commandsWithSubSubCommands = new Set(['npm run', 'yarn run']);
899+
const subCommandsToSuggest = Array.from(new Set(unapprovedSubCommands.map(command => {
900+
const parts = command.trim().split(/\s+/);
901+
const baseCommand = parts[0].toLowerCase();
902+
const baseSubCommand = parts.length > 1 ? `${parts[0]} ${parts[1]}`.toLowerCase() : '';
903+
904+
if (commandsWithSubSubCommands.has(baseSubCommand) && parts.length >= 3) {
905+
return `${parts[0]} ${parts[1]} ${parts[2]}`;
906+
} else if (commandsWithSubcommands.has(baseCommand) && parts.length >= 2) {
907+
return `${parts[0]} ${parts[1]}`;
908+
} else {
909+
return parts[0];
910+
}
911+
})));
912+
913+
if (subCommandsToSuggest.length > 0) {
914+
let subCommandLabel: string;
915+
let subCommandTooltip: string;
916+
if (subCommandsToSuggest.length === 1) {
917+
subCommandLabel = localize('autoApprove.baseCommandSingle', 'Always Allow Command: {0}', subCommandsToSuggest[0]);
918+
subCommandTooltip = localize('autoApprove.baseCommandSingleTooltip', 'Always allow command starting with `{0}` to run without confirmation', subCommandsToSuggest[0]);
919+
} else {
920+
const commandSeparated = subCommandsToSuggest.join(', ');
921+
subCommandLabel = localize('autoApprove.baseCommand', 'Always Allow Commands: {0}', commandSeparated);
922+
subCommandTooltip = localize('autoApprove.baseCommandTooltip', 'Always allow commands starting with `{0}` to run without confirmation', commandSeparated);
923+
}
906924

907-
if (unapprovedSubCommands.length > 0) {
908925
actions.push({
909926
label: subCommandLabel,
910927
tooltip: subCommandTooltip,
911928
data: {
912929
type: 'newRule',
913-
rule: subCommandsFirstWordOnly.map(key => ({
930+
rule: subCommandsToSuggest.map(key => ({
914931
key,
915932
value: true
916933
}))
@@ -920,12 +937,13 @@ export class RunInTerminalTool extends Disposable implements IToolImpl {
920937

921938
// Allow exact command line, don't do this if it's just the first sub-command's first
922939
// word
923-
if (subCommandsFirstWordOnly[0] !== commandLine) {
940+
const firstSubcommandFirstWord = unapprovedSubCommands.length > 0 ? unapprovedSubCommands[0].split(' ')[0] : '';
941+
if (firstSubcommandFirstWord !== commandLine) {
924942
const truncatedCommandLine = commandLine.length > 40 ? commandLine.substring(0, 40) + '\u2026' : commandLine;
925943
actions.push({
926944
// Add an extra & since it's treated as a mnemonic
927-
label: localize('autoApprove.exactCommand', 'Always Allow Full Command Line: {0}', truncatedCommandLine.replaceAll('&&', '&&&')),
928-
tooltip: localize('autoApprove.exactCommandTooltip', 'Always allow this exact command to run without confirmation'),
945+
label: localize('autoApprove.exactCommand', 'Always Allow Exact Command Line: {0}', truncatedCommandLine.replaceAll('&&', '&&&')),
946+
tooltip: localize('autoApprove.exactCommandTooltip', 'Always allow this exact command line to run without confirmation'),
929947
data: {
930948
type: 'newRule',
931949
rule: {

‎src/vs/workbench/contrib/terminalContrib/chatAgentTools/common/terminalChatAgentToolsConfiguration.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ export const terminalChatAgentToolsConfiguration: IStringDictionary<IConfigurati
8787
]
8888
},
8989
default: {
90-
// Safe and common readonly commands (automatically approved)
90+
// Safe and common readonly commands
9191
cd: true,
9292
echo: true,
9393
ls: true,
@@ -125,6 +125,13 @@ export const terminalChatAgentToolsConfiguration: IStringDictionary<IConfigurati
125125
jq: true,
126126
sleep: true,
127127
'Start-Sleep': true,
128+
129+
// Safe and common sub-commands
130+
'git status': true,
131+
'git log': true,
132+
'git show': true,
133+
'git diff': true,
134+
128135
// While these PowerShell verbs can have side effects, they are generally innocuous (eg.
129136
// updating OS-level file access info) and and often have prompts if they're more
130137
// involved (eg. Get-Credential)

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

Lines changed: 207 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -296,12 +296,12 @@ suite('RunInTerminalTool', () => {
296296

297297

298298
ok(!isSeparator(customActions[0]));
299-
strictEqual(customActions[0].label, 'Always Allow Command: npm');
299+
strictEqual(customActions[0].label, 'Always Allow Command: npm run build');
300300
strictEqual(customActions[0].data.type, 'newRule');
301301
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
302302

303303
ok(!isSeparator(customActions[1]));
304-
strictEqual(customActions[1].label, 'Always Allow Full Command Line: npm run build');
304+
strictEqual(customActions[1].label, 'Always Allow Exact Command Line: npm run build');
305305
strictEqual(customActions[1].data.type, 'newRule');
306306
ok(!Array.isArray(customActions[1].data.rule), 'Expected rule to be an object');
307307

@@ -384,11 +384,11 @@ suite('RunInTerminalTool', () => {
384384
strictEqual(customActions.length, 4);
385385

386386
ok(!isSeparator(customActions[0]));
387-
strictEqual(customActions[0].label, 'Always Allow Command: npm');
387+
strictEqual(customActions[0].label, 'Always Allow Commands: npm install, npm run build');
388388
strictEqual(customActions[0].data.type, 'newRule');
389389

390390
ok(!isSeparator(customActions[1]));
391-
strictEqual(customActions[1].label, 'Always Allow Full Command Line: npm install &&& npm run build');
391+
strictEqual(customActions[1].label, 'Always Allow Exact Command Line: npm install &&& npm run build');
392392
strictEqual(customActions[1].data.type, 'newRule');
393393

394394
ok(isSeparator(customActions[2]));
@@ -419,7 +419,7 @@ suite('RunInTerminalTool', () => {
419419
strictEqual(customActions[0].data.type, 'newRule');
420420

421421
ok(!isSeparator(customActions[1]));
422-
strictEqual(customActions[1].label, 'Always Allow Full Command Line: foo | head -20');
422+
strictEqual(customActions[1].label, 'Always Allow Exact Command Line: foo | head -20');
423423
strictEqual(customActions[1].data.type, 'newRule');
424424

425425
ok(isSeparator(customActions[2]));
@@ -465,7 +465,7 @@ suite('RunInTerminalTool', () => {
465465
strictEqual(customActions[0].data.type, 'newRule');
466466

467467
ok(!isSeparator(customActions[1]));
468-
strictEqual(customActions[1].label, 'Always Allow Full Command Line: foo | head -20 &&& bar | tail -10');
468+
strictEqual(customActions[1].label, 'Always Allow Exact Command Line: foo | head -20 &&& bar | tail -10');
469469
strictEqual(customActions[1].data.type, 'newRule');
470470

471471
ok(isSeparator(customActions[2]));
@@ -475,6 +475,207 @@ suite('RunInTerminalTool', () => {
475475
strictEqual(customActions[3].data.type, 'configure');
476476
});
477477

478+
test('should suggest subcommand for git commands', async () => {
479+
const result = await executeToolTest({
480+
command: 'git status',
481+
explanation: 'Check git status'
482+
});
483+
484+
assertConfirmationRequired(result);
485+
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
486+
487+
const customActions = result!.confirmationMessages!.terminalCustomActions!;
488+
strictEqual(customActions.length, 4);
489+
490+
ok(!isSeparator(customActions[0]));
491+
strictEqual(customActions[0].label, 'Always Allow Command: git status');
492+
strictEqual(customActions[0].data.type, 'newRule');
493+
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
494+
strictEqual((customActions[0].data.rule as any)[0].key, 'git status');
495+
});
496+
497+
test('should suggest subcommand for npm commands', async () => {
498+
const result = await executeToolTest({
499+
command: 'npm test',
500+
explanation: 'Run npm tests'
501+
});
502+
503+
assertConfirmationRequired(result);
504+
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
505+
506+
const customActions = result!.confirmationMessages!.terminalCustomActions!;
507+
strictEqual(customActions.length, 4);
508+
509+
ok(!isSeparator(customActions[0]));
510+
strictEqual(customActions[0].label, 'Always Allow Command: npm test');
511+
strictEqual(customActions[0].data.type, 'newRule');
512+
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
513+
strictEqual((customActions[0].data.rule as any)[0].key, 'npm test');
514+
});
515+
516+
test('should suggest 3-part subcommand for npm run commands', async () => {
517+
const result = await executeToolTest({
518+
command: 'npm run build',
519+
explanation: 'Run build script'
520+
});
521+
522+
assertConfirmationRequired(result);
523+
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
524+
525+
const customActions = result!.confirmationMessages!.terminalCustomActions!;
526+
strictEqual(customActions.length, 4);
527+
528+
ok(!isSeparator(customActions[0]));
529+
strictEqual(customActions[0].label, 'Always Allow Command: npm run build');
530+
strictEqual(customActions[0].data.type, 'newRule');
531+
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
532+
strictEqual((customActions[0].data.rule as any)[0].key, 'npm run build');
533+
});
534+
535+
test('should suggest 3-part subcommand for yarn run commands', async () => {
536+
const result = await executeToolTest({
537+
command: 'yarn run test',
538+
explanation: 'Run test script'
539+
});
540+
541+
assertConfirmationRequired(result);
542+
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
543+
544+
const customActions = result!.confirmationMessages!.terminalCustomActions!;
545+
strictEqual(customActions.length, 4);
546+
547+
ok(!isSeparator(customActions[0]));
548+
strictEqual(customActions[0].label, 'Always Allow Command: yarn run test');
549+
strictEqual(customActions[0].data.type, 'newRule');
550+
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
551+
strictEqual((customActions[0].data.rule as any)[0].key, 'yarn run test');
552+
});
553+
554+
test('should handle mixed npm run and other commands', async () => {
555+
const result = await executeToolTest({
556+
command: 'npm run build && git status',
557+
explanation: 'Build and check status'
558+
});
559+
560+
assertConfirmationRequired(result);
561+
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
562+
563+
const customActions = result!.confirmationMessages!.terminalCustomActions!;
564+
strictEqual(customActions.length, 4);
565+
566+
ok(!isSeparator(customActions[0]));
567+
strictEqual(customActions[0].label, 'Always Allow Commands: npm run build, git status');
568+
strictEqual(customActions[0].data.type, 'newRule');
569+
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
570+
const rules = customActions[0].data.rule as any;
571+
strictEqual(rules.length, 2);
572+
strictEqual(rules[0].key, 'npm run build');
573+
strictEqual(rules[1].key, 'git status');
574+
});
575+
576+
test('should suggest mixed subcommands and base commands', async () => {
577+
const result = await executeToolTest({
578+
command: 'git push && echo "done"',
579+
explanation: 'Push and print done'
580+
});
581+
582+
assertConfirmationRequired(result);
583+
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
584+
585+
const customActions = result!.confirmationMessages!.terminalCustomActions!;
586+
strictEqual(customActions.length, 4);
587+
588+
ok(!isSeparator(customActions[0]));
589+
strictEqual(customActions[0].label, 'Always Allow Commands: git push, echo');
590+
strictEqual(customActions[0].data.type, 'newRule');
591+
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
592+
const rules = customActions[0].data.rule as any;
593+
strictEqual(rules.length, 2);
594+
strictEqual(rules[0].key, 'git push');
595+
strictEqual(rules[1].key, 'echo');
596+
});
597+
598+
test('should suggest subcommands for multiple git commands', async () => {
599+
const result = await executeToolTest({
600+
command: 'git status && git log --oneline',
601+
explanation: 'Check status and log'
602+
});
603+
604+
assertConfirmationRequired(result);
605+
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
606+
607+
const customActions = result!.confirmationMessages!.terminalCustomActions!;
608+
strictEqual(customActions.length, 4);
609+
610+
ok(!isSeparator(customActions[0]));
611+
strictEqual(customActions[0].label, 'Always Allow Commands: git status, git log');
612+
strictEqual(customActions[0].data.type, 'newRule');
613+
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
614+
const rules = customActions[0].data.rule as any;
615+
strictEqual(rules.length, 2);
616+
strictEqual(rules[0].key, 'git status');
617+
strictEqual(rules[1].key, 'git log');
618+
});
619+
620+
test('should suggest base command for non-subcommand tools', async () => {
621+
const result = await executeToolTest({
622+
command: 'curl https://example.com',
623+
explanation: 'Download from example.com'
624+
});
625+
626+
assertConfirmationRequired(result);
627+
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
628+
629+
const customActions = result!.confirmationMessages!.terminalCustomActions!;
630+
strictEqual(customActions.length, 4);
631+
632+
ok(!isSeparator(customActions[0]));
633+
strictEqual(customActions[0].label, 'Always Allow Command: curl');
634+
strictEqual(customActions[0].data.type, 'newRule');
635+
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
636+
strictEqual((customActions[0].data.rule as any)[0].key, 'curl');
637+
});
638+
639+
test('should handle single word commands from subcommand-aware tools', async () => {
640+
const result = await executeToolTest({
641+
command: 'git',
642+
explanation: 'Run git command'
643+
});
644+
645+
assertConfirmationRequired(result);
646+
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
647+
648+
const customActions = result!.confirmationMessages!.terminalCustomActions!;
649+
strictEqual(customActions.length, 3); // No full command line suggestion for single word
650+
651+
ok(!isSeparator(customActions[0]));
652+
strictEqual(customActions[0].label, 'Always Allow Command: git');
653+
strictEqual(customActions[0].data.type, 'newRule');
654+
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
655+
strictEqual((customActions[0].data.rule as any)[0].key, 'git');
656+
});
657+
658+
test('should deduplicate identical subcommand suggestions', async () => {
659+
const result = await executeToolTest({
660+
command: 'npm test && npm test --verbose',
661+
explanation: 'Run tests twice'
662+
});
663+
664+
assertConfirmationRequired(result);
665+
ok(result!.confirmationMessages!.terminalCustomActions, 'Expected custom actions to be defined');
666+
667+
const customActions = result!.confirmationMessages!.terminalCustomActions!;
668+
strictEqual(customActions.length, 4);
669+
670+
ok(!isSeparator(customActions[0]));
671+
strictEqual(customActions[0].label, 'Always Allow Command: npm test');
672+
strictEqual(customActions[0].data.type, 'newRule');
673+
ok(Array.isArray(customActions[0].data.rule), 'Expected rule to be an array');
674+
const rules = customActions[0].data.rule as any;
675+
strictEqual(rules.length, 1); // Should be deduplicated
676+
strictEqual(rules[0].key, 'npm test');
677+
});
678+
478679
});
479680

480681
suite('command re-writing', () => {

0 commit comments

Comments
 (0)