-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Add in-dialog command whitelisting with LLM suggestions (#5480) #5491
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
|
✅ No security or compliance issues detected. Reviewed everything up to 83b6f3c. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. Reply to this PR with |
- Fix hardcoded English strings by moving to translation files - Add missing ARIA attributes for accessibility compliance - Extract suggestion parsing logic to shared utils (src/shared/commandParsing.ts) - Move pattern extraction logic to shared utils (src/shared/commandPatterns.ts) - Extract CommandPatternSelector as a separate component for better modularity - Consolidate message types to use 'allowedCommands' consistently - Update tests to match new implementation All linters and tests now pass successfully.
2039a2a to
a65f5d7
Compare
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.
Review comments are below, generally speaking I think this could be improved by using a library that parses the original command into its components for suggestions; this allows us to exclude system instructions and command a tool changes altogether:
See the existing command parser here, which uses the shell-quote library that is only available in the code base:
using the existing parser (or another command line parsing library) you can more easily extract commands that are provided by the model without involving the model at all.
let's first try and implement this without system instructions at all. if that proves not to be possible, then please default the new system instructions to be disabled because users can already allow commands manually without affecting the length of their instruction tokens.
src/core/tools/executeCommandTool.ts
Outdated
| command = unescapeHtmlEntities(command) // Unescape HTML entities. | ||
| const didApprove = await askApproval("command", command) | ||
|
|
||
| // Parse suggestions if provided |
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.
I agree with this parsing should be centralized in Roo's core.
Alternatively, cherry-pick this commit which provides a generalized way of passing metadata down from the tool into the web view:
I disagree. I already attempted without the LLM and it was not robust. I like this implementation. Code needs work still. |
Can we leave added tokens disabled by default? |
|
Roo working better by default is more imperative than lower tokens. Our base system prompt still comes in lower than the base Claude Code one and they clearly recognize that a good system prompt is important. The added tokens are needed to get the suggestion no? |
| import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" | ||
| import * as vscode from "vscode" | ||
| import { webviewMessageHandler } from "../webviewMessageHandler" | ||
| import { ClineProvider } from "../ClineProvider" |
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.
The import for 'ClineProvider' is never used. Please remove it to keep the test file clean.
| import { ClineProvider } from "../ClineProvider" |
| "exited": "Exited ({{exitCode}})", | ||
| "addToAllowedCommands": "Add to Allowed Auto-Execute Commands", | ||
| "manageCommands": "Gerenciar Permissões de Comando", | ||
| "commandManagementDescription": "Gerencie as permissões de comando: Clique em ✓ para permitir a execução automática, ✗ para negar a execução. Os padrões possono ser ativados/desativados ou removidos das listas. <settingsLink>Ver todas as configurações</settingsLink>", |
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.
Typo found: In the text "Os padrões possono ser ativados/desativados ou removidos das listas.", consider replacing "possono" with "podem" to maintain consistency in Portuguese.
| "commandManagementDescription": "Gerencie as permissões de comando: Clique em ✓ para permitir a execução automática, ✗ para negar a execução. Os padrões possono ser ativados/desativados ou removidos das listas. <settingsLink>Ver todas as configurações</settingsLink>", | |
| "commandManagementDescription": "Gerencie as permissões de comando: Clique em ✓ para permitir a execução automática, ✗ para negar a execução. Os padrões podem ser ativados/desativados ou removidos das listas. <settingsLink>Ver todas as configurações</settingsLink>", |
- Added new configuration setting 'roo.disableLLMCommandSuggestions' - Made command suggestions conditional based on the setting - Updated system prompt to pass the setting value - Added comprehensive tests for the new functionality - Added localization support for the new setting Fixes #5491
- Added vscode import to fix TypeScript error - Added Package import from shared/package module - Fixes pre-push type checking failure
- Removed 'roo-code.commandWhitelist' from VS Code settings (package.json) - Added command whitelisting to Auto Approve settings in plugin UI - Migrated existing whitelist settings to new location on extension activation - Updated all related components, tests, and localization files - Maintains backward compatibility by migrating existing settings This change improves user experience by consolidating all auto-approval settings in one location within the plugin's settings interface.
- Fixed programmatic suggestion generation when LLM suggestions are disabled - CommandExecution component now properly generates suggestions from allowed patterns - Added proper handling for when llmGeneratedSuggestions setting is false - Fixed settings persistence for command allow/deny lists - AutoApproveSettings now correctly saves patterns to globalState - Fixed state management to properly update both local and global state - Improved LLM prompt to generate complete suggestions for chained commands - Updated execute-command prompt to handle && and || operators - Ensures suggestions include full command chains, not just the first part - Added tests to verify proper handling of complex command patterns
- Added English placeholder translations for all non-English locales - This fixes the check-translations CI failure - Translations can be properly localized in future updates
| "addButton": "Ekle", | ||
| "autoDenied": "`{{prefix}}` önekli komutlar kullanıcı tarafından yasaklandı. Başka bir komut çalıştırarak bu kısıtlamayı aşma." | ||
| "autoDenied": "`{{prefix}}` önekli komutlar kullanıcı tarafından yasaklandı. Başka bir komut çalıştırarak bu kısıtlamayı aşma.", | ||
| "disableLlmSuggestions": { |
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.
The new translation key 'disableLlmSuggestions' is added with its label and description remaining in English. For proper localization, please provide a Turkish translation rather than using the default English text. (Similar updates are needed in other locale files.)
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
…g merging - Fixed issue where removed command patterns would reappear in the UI - The problem was caused by workspace configuration being merged after UI state updates - Now properly filters out removed patterns when merging workspace config - Added comprehensive tests to verify the fix handles all edge cases - Ensures user's removal actions are preserved across config updates Fixes #5480
- Replace regex-based parsing with shell-quote library for deterministic parsing - Handle complex shell syntax including quotes, escapes, and special characters - Maintain backward compatibility with existing command patterns - Add comprehensive test coverage for edge cases - Addresses PR feedback about regex limitations and parsing reliability The shell-quote library provides proper shell command parsing that handles: - Single and double quotes with proper escape sequences - Environment variable expansion - Command operators (&&, ||, |, ;) - Glob patterns and special characters - Nested quotes and complex argument structures This ensures commands are extracted exactly as they would be interpreted by a shell.
Fixed the pattern extraction logic to only extract individual commands and their subcommands, never full command chains. This prevents issues where complex piped or chained commands would be incorrectly treated as single patterns. - Updated extractCommandPatterns to stop at command boundaries (&&, ||, |, ;) - Added comprehensive tests for command chain scenarios - Ensures only atomic commands are whitelisted, not entire command sequences
- Updated execute-command prompt to explicitly forbid suggesting full command chains - Added clear instruction that only the first command should be suggested for chained commands - This ensures the LLM will only suggest the initial command and let the user decide on subsequent commands
- Added extensive test coverage for shell-quote based command parsing - Included edge cases: quotes, escapes, special characters, redirections - Added real-world examples: git, npm, docker, curl, ssh commands - Validated robustness of parser against complex shell command patterns - Ensures reliable command execution across different shell syntaxes
…ll-quote parser This commit simplifies the command pattern extraction implementation by: - Removing all LLM-based command suggestion functionality - Always using the shell-quote parser for deterministic command pattern extraction - Eliminating the commandSuggestionsEnabled setting and related UI components - Removing unnecessary complexity from the codebase The shell-quote parser provides consistent and predictable results without requiring LLM calls, making the feature more reliable and performant.
This completes the removal of the LLM command suggestions feature by: - Removing the setting from Task.ts system prompt generation - Removing the message handler case from webviewMessageHandler.ts
- Replace hardcoded aria-labels in CommandExecution and CommandPatternSelector components - Add missing translation keys for aria-labels in all locale files - Update tests to use translation keys instead of hardcoded strings - Fix duplicate commandExecution sections in Italian translation file
| "didViewTopLevel": "Roo ha vist els fitxers de nivell superior en aquest directori:", | ||
| "wantsToViewRecursive": "Roo vol veure recursivament tots els fitxers en aquest directori:", | ||
| "didViewRecursive": "Roo ha vist recursivament tots els fitxers en aquest directori:", | ||
| "didViewRecursive": "Roo ha vist recursivament tots els fitxers en acest directori:", |
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.
Typo: 'acest directori' should be 'aquest directori'.
| "didViewRecursive": "Roo ha vist recursivament tots els fitxers en acest directori:", | |
| "didViewRecursive": "Roo ha vist recursivament tots els fitxers en aquest directori:", |
| "didViewTopLevelOutsideWorkspace": "Roo ha vist els fitxers de nivell superior en aquest directori (fora de l'espai de treball):", | ||
| "wantsToViewRecursiveOutsideWorkspace": "Roo vol veure recursivament tots els fitxers en aquest directori (fora de l'espai de treball):", | ||
| "didViewRecursiveOutsideWorkspace": "Roo ha vist recursivament tots els fitxers en aquest directori (fora de l'espai de treball):", | ||
| "wantsToViewRecursiveOutsideWorkspace": "Roo vol veure recursivament tots els fitxers en acest directori (fora de l'espai de treball):", |
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.
Typo: 'acest' should be corrected to 'aquest' in the value for 'wantsToViewRecursiveOutsideWorkspace'.
| "wantsToViewRecursiveOutsideWorkspace": "Roo vol veure recursivament tots els fitxers en acest directori (fora de l'espai de treball):", | |
| "wantsToViewRecursiveOutsideWorkspace": "Roo vol veure recursivament tots els fitxers en aquest directori (fora de l'espai de treball):", |
| "wantsToViewRecursiveOutsideWorkspace": "Roo vol veure recursivament tots els fitxers en aquest directori (fora de l'espai de treball):", | ||
| "didViewRecursiveOutsideWorkspace": "Roo ha vist recursivament tots els fitxers en aquest directori (fora de l'espai de treball):", | ||
| "wantsToViewRecursiveOutsideWorkspace": "Roo vol veure recursivament tots els fitxers en acest directori (fora de l'espai de treball):", | ||
| "didViewRecursiveOutsideWorkspace": "Roo ha vist recursivament tots els fitxers en acest directori (fora de l'espai de treball):", |
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.
Typo: 'acest' should be corrected to 'aquest' in the value for 'didViewRecursiveOutsideWorkspace'.
| "didViewRecursiveOutsideWorkspace": "Roo ha vist recursivament tots els fitxers en acest directori (fora de l'espai de treball):", | |
| "didViewRecursiveOutsideWorkspace": "Roo ha vist recursivament tots els fitxers en aquest directori (fora de l'espai de treball):", |
| "didViewRecursiveOutsideWorkspace": "Roo ha vist recursivament tots els fitxers en acest directori (fora de l'espai de treball):", | ||
| "wantsToViewDefinitionsOutsideWorkspace": "Roo vol veure noms de definicions de codi font utilitzats en aquest directori (fora de l'espai de treball):", | ||
| "didViewDefinitionsOutsideWorkspace": "Roo ha vist noms de definicions de codi font utilitzats en aquest directori (fora de l'espai de treball):" | ||
| "didViewDefinitionsOutsideWorkspace": "Roo ha vist noms de definicions de codi font utilitzats en acest directori (fora de l'espai de treball):" |
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.
Typo: 'acest' should be corrected to 'aquest' in the value for 'didViewDefinitionsOutsideWorkspace'.
| "didViewDefinitionsOutsideWorkspace": "Roo ha vist noms de definicions de codi font utilitzats en acest directori (fora de l'espai de treball):" | |
| "didViewDefinitionsOutsideWorkspace": "Roo ha vist noms de definicions de codi font utilitzats en aquest directori (fora de l'espai de treball):" |
| "ask": { | ||
| "autoApprovedRequestLimitReached": { | ||
| "title": "S'ha arribat al límit de sol·licituds aprovades automàticament", | ||
| "title": "S'ha arribat al límit de sol·licituds aprovades automàticamente", |
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.
Typographical error: In Catalan, the adverb should be "automàticament" rather than "automàticamente".
| "title": "S'ha arribat al límit de sol·licituds aprovades automàticamente", | |
| "title": "S'ha arribat al límit de sol·licituds aprovades automàticament", |
| "wantsToRead": "Roo इस फ़ाइल को पढ़ना चाहता है:", | ||
| "wantsToReadOutsideWorkspace": "Roo कार्यक्षेत्र के बाहर इस फ़ाइल को पढ़ना चाहता है:", | ||
| "didRead": "Roo ने इस फ़ाइल को पढ़ा:", | ||
| "didRead": "Roo ਨੇ इस फ़ाइल को पढ़ा:", |
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.
Typo: The string "Roo ਨੇ इस फ़ाइल को पढ़ा:" uses 'ਨੇ' (Gurmukhi) instead of the consistent Hindi 'ने'. Please verify if this is intentional or a typographical error.
| "didRead": "Roo ਨੇ इस फ़ाइल को पढ़ा:", | |
| "didRead": "Roo ने इस फ़ाइल को पढ़ा:", |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
| "shellIntegration": { | ||
| "title": "명령 실행 경고", | ||
| "description": "명령이 VSCode 터미널 쉘 통합 없이 실행되고 있습니다. 이 경고를 숨기려면 <settingsLink>Roo Code 설정</settingsLink>의 <strong>Terminal</strong> 섹션에서 쉘 통합을 비활성화하거나 아래 링크를 사용하여 VSCode 터미널 통합 문제를 해결하세요.", | ||
| "description": "명령이 VSCode 터미널 쉘 통합 없이 실행되고 있습니다. 이 경고를 숨기려면 <settingsLink>Roo Code 설정</settingsLink>의 <strong>Terminal</strong> 섹션에서 쉘 통합을 비활성화하거나 아래 링크를 사용하여 VSCode 터मि널 통합 문제를 해결하세요.", |
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.
Typographical error: The word "터मि널" seems to be a mix of characters. It should likely be corrected to "터미널".
| "description": "명령이 VSCode 터미널 쉘 통합 없이 실행되고 있습니다. 이 경고를 숨기려면 <settingsLink>Roo Code 설정</settingsLink>의 <strong>Terminal</strong> 섹션에서 쉘 통합을 비활성화하거나 아래 링크를 사용하여 VSCode 터मि널 통합 문제를 해결하세요.", | |
| "description": "명령이 VSCode 터미널 쉘 통합 없이 실행되고 있습니다. 이 경고를 숨기려면 <settingsLink>Roo Code 설정</settingsLink>의 <strong>Terminal</strong> 섹션에서 쉘 통합을 비활성화하거나 아래 링크를 사용하여 VSCode 터미널 통합 문제를 해결하세요.", |
- Standardized terminology from 'whitelisted/blacklisted' to 'allowed/denied' across all i18n files - Removed unused _isExpanded and terminalShellIntegrationDisabled variables in CommandExecution.tsx - Added comprehensive JSDoc documentation to complex algorithms in commandPatterns.ts - Consolidated redundant command parsing logic into unified commandUtils module - Updated all imports to use the new centralized utilities - Maintained backward compatibility with re-exports where needed All tests passing after refactoring.
- Remove redundant allowCommand/denyCommand message types from WebviewMessage - Remove corresponding handlers from webviewMessageHandler - Remove unused test file for allowCommand functionality - Remove unused i18n keys for command_allowed and command_denied - Simplify to use existing allowedCommands/deniedCommands infrastructure
By Default both the output of the terminal and the command permissions are zipped up.
Related GitHub Issue
Closes: #5480
Roo Code Task Context (Optional)
Description
This PR enhances the command approval dialog by adding an in-context whitelisting interface that allows users to quickly add command patterns to their auto-approve list without navigating away to settings.
Key Implementation Details:
shell-quotelibrary for proper command parsingCommandExecutioncomponent that renders the command approval UI with a collapsible whitelist sectionallowedCommandssettings mechanism for immediate persistenceDesign Choices:
shell-quotelibrary instead of LLM suggestions for reliable, deterministic command parsing (addresses reviewer feedback)Areas for Review Focus:
commandPatterns.tsutility that usesshell-quotefor command parsingCommandExecutioncomponent implementation and its integration with the settings systemTest Procedure
Unit Tests Added:
commandPatterns.spec.ts: Tests for the deterministic command pattern extraction logicCommandExecution.spec.tsx: Component tests for rendering suggestions, handling selections, and updating settingswebviewMessageHandler.spec.ts: Tests for handling the new message typesManual Testing Steps:
npm install)Testing Environment:
Pre-Submission Checklist
Screenshots / Videos
[Please add screenshots showing:
Documentation Updates
Additional Notes
Translation Updates:
This PR includes translations for all 18 supported languages. The new translation keys added are:
chat.command.addToWhitelist: Label for the collapsible sectionchat.command.whitelistDescription: Instructional text above checkboxeschat.command.addSelected: Button label for adding selected patternscommon.info.command_whitelisted: Confirmation message after whitelistingAll translations maintain the informal voice consistent with the project style.
Implementation Note:
Based on reviewer feedback, this PR now uses a deterministic approach with the
shell-quotelibrary for command parsing instead of relying on LLM suggestions. This ensures consistent and reliable pattern extraction that properly handles complex command structures, quoted arguments, and special characters.Get in Touch
[Your Discord username]
Important
Adds in-dialog command whitelisting with LLM suggestions, enhancing command parsing and UI for managing command permissions.
CommandExecution.tsxfor quick pattern addition to auto-approve list.shell-quotefor deterministic command parsing incommandPatterns.ts.allowedCommandssettings for immediate persistence.CommandPatternSelectorcomponent for managing command permissions.CommandExecution.tsxto include collapsible whitelist section.commandUtils.ts.extractCommandPatterns()incommandPatterns.tsfor pattern extraction.commandPatterns.spec.tsfor pattern extraction logic.executeCommandTool.spec.tsfor handling suggestions.allowedCommandsfrompackage.jsonconfiguration.This description was created by
for 83b6f3c. You can customize this summary. It will automatically update as commits are pushed.