-
Notifications
You must be signed in to change notification settings - Fork 37.3k
Fix auto-approve to suggest commands with flags when flags precede subcommand #282471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…mmand and subcommand Co-authored-by: Tyriar <[email protected]>
…caping in tests Co-authored-by: Tyriar <[email protected]>
Tyriar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot this isn't the solution I suggested. The auto approve command suggestion should include the exact flag as specified. So for the command line mvn -abc foo bar, mvn -abc foo should be the new rule.
Changed implementation to suggest the exact command with flags included (e.g., `mvn -DskipIT test` instead of just `mvn test`). This matches the requirement that for `mvn -abc foo bar`, the suggestion should be `mvn -abc foo` (including the flags between command and subcommand). Co-authored-by: Tyriar <[email protected]>
Fixed in 137cd8d. The implementation now includes flags in the suggestions as you specified. For |
Co-authored-by: Tyriar <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes the auto-approve feature to recognize and suggest commands when CLI flags appear before subcommands. Previously, commands like mvn -DskipIT test would fail to match the approved mvn test rule and wouldn't offer subcommand-based approval options.
Key changes:
- Added
findNextNonFlagArghelper function to locate the first non-flag argument after a given position - Updated the subcommand extraction logic to include all flags between the command and subcommand in the suggested approval rule
- Added comprehensive test coverage with 12 test cases covering single flags, multiple flags, sub-sub-commands, and edge cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/runInTerminalHelpers.ts |
Implements findNextNonFlagArg helper and updates subcommand detection logic to handle flags before subcommands, suggesting approval rules that include the flags (e.g., mvn -DskipIT test instead of just mvn test) |
src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/runInTerminalHelpers.test.ts |
Adds new test suite with 12 test cases covering various scenarios: single/multiple flags before subcommands, sub-sub-commands with flags, commands with only flags, and edge cases for denied/approved commands |
Auto-approve was failing to recognize subcommands when CLI flags appeared before them. Commands like
mvn -DskipIT testwouldn't match the approvedmvn testrule, and users wouldn't see an option to auto-approve—only "Always Allow Exact Command Line."This fix enables the system to detect subcommands even when flags appear before them, and suggests approval rules that include the flags as specified in the command.
Changes
runInTerminalHelpers.ts: AddedfindNextNonFlagArghelper that returns the index of the first non-flag argument after a given positionparts.slice(0, subCommandIndex + 1).join(' ')to include all arguments from command through subcommand (including flags)mvn -DskipIT testsuggestsmvn -DskipIT testnpm --silent run testsuggestsnpm --silent run testExample
Now correctly suggests (including flags):
mvn -DskipIT test→ "Always allowmvn -DskipIT test"mvn -abc foo bar→ "Always allowmvn -abc foo"gradle --info build→ "Always allowgradle --info build"npm --silent run test→ "Always allownpm --silent run test"Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.