-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add 'Add and Run' button for allowed commands (#5290) #5322
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 3e5cc31. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. Reply to this PR with |
19417f9 to
f8f49dd
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.
Pull Request Overview
This PR adds an "Add and Run" button to chat command blocks, enabling users to whitelist and execute commands in one step.
- Introduces
extractCommandPatternutility to generalize commands for whitelisting - Adds a tertiary button in
ChatViewfor the new feature and updates handling logic - Extends settings and message handler to sync allowed commands and add i18n entries
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| webview-ui/src/utils/extract-command-pattern.ts | New utility to extract patterns from commands |
| webview-ui/src/components/chat/ChatView.tsx | UI and logic for three-button "Add & Run" mode |
| webview-ui/src/components/settings/SettingsView.tsx | Sync external updates to allowed commands in settings |
| src/core/webview/webviewMessageHandler.ts | Handle allowedCommands messages and post state |
| setTertiaryButtonText(undefined) | ||
| setTertiaryButtonText(undefined) | ||
| setTertiaryButtonText(undefined) |
Copilot
AI
Jul 1, 2025
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.
Duplicate calls to setTertiaryButtonText(undefined) can be consolidated into a single statement to reduce redundancy.
| setTertiaryButtonText(undefined) | |
| setTertiaryButtonText(undefined) | |
| setTertiaryButtonText(undefined) |
| // Three button mode - this shouldn't be called | ||
| console.warn("Secondary button clicked in three-button command mode") |
Copilot
AI
Jul 1, 2025
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.
In three-button command mode, the secondary button (Reject) logs a warning and does not send the noButtonClicked message. Replace this warning with logic to send the reject response so the button actually works.
| // Three button mode - this shouldn't be called | |
| console.warn("Secondary button clicked in three-button command mode") | |
| // Three button mode - send reject response | |
| vscode.postMessage({ type: "askResponse", askResponse: "reject" }) | |
| setInputValue("") | |
| setSelectedImages([]) |
| // Update cached state when allowedCommands changes from external sources (e.g., "Add & Run") | ||
| useEffect(() => { | ||
| // Only update if the allowedCommands have actually changed | ||
| if (JSON.stringify(cachedState.allowedCommands) !== JSON.stringify(extensionState.allowedCommands)) { |
Copilot
AI
Jul 1, 2025
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.
Using JSON.stringify for deep comparison on every state update may be inefficient for larger command lists. Consider using a dedicated deep-equality check or memoized comparison to improve performance.
| appearance="secondary" | ||
| disabled={!enableButtons} | ||
| className="flex-1" | ||
| onClick={() => handleTertiaryButtonClick(inputValue, selectedImages)}> |
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 noticed the button click handlers appear to be swapped. The "Add & Run" button (which displays secondaryButtonText) is calling handleTertiaryButtonClick, while the "Reject" button (displaying tertiaryButtonText) calls handleSecondaryButtonClick.
This mismatch is likely the root cause of why the Reject button only logs a warning instead of working properly. Should these handlers be aligned with their button labels for clarity?
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.
Makes sense.
| .update("allowedCommands", updatedCommands, vscode.ConfigurationTarget.Global) | ||
|
|
||
| // Post state update to webview | ||
| await provider.postStateToWebview() |
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 PR description mentions "automatic navigation to Settings view after adding commands," but I don't see any navigation logic here - only state updates via postStateToWebview(). Was the automatic navigation feature intentionally removed, or should it be implemented?
If navigation is desired, you might need to send a specific message to trigger the Settings view.
| // This might be an environment variable like NODE_ENV=production | ||
| const envMatch = baseCommand.match(/^([A-Z_]+)=/) | ||
| if (envMatch) { | ||
| // Don't include the value, just the variable name pattern |
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 current depth limit of 3 for chained commands might be too restrictive for legitimate use cases. For example, a common pattern like cat file.txt | grep pattern | sort | uniq | head -10 would exceed this limit.
While I understand the security concern about overly complex patterns, would it make sense to:
- Increase the limit to 5 or 6?
- Make it configurable?
- Show a warning instead of truncating?
This could improve the user experience for developers who commonly use longer command pipelines.
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.
Maybe the LLM could be coaxed to make more optimal commands with less chaining? When I handwrite oneliners like that I write it the same way, but the more optimal version takes only 3 chained commands grep pattern file.txt | sort -u | head -10
| const pattern = extractCommandPattern(commandText) | ||
| const description = getPatternDescription(pattern) | ||
| return pattern | ||
| ? `${t("chat:addAndRunCommand.tooltip")} Will whitelist: "${pattern}" (${description})` |
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 tooltip for complex command patterns could become quite long and difficult to read. For example: "Add command pattern to whitelist and run" Will whitelist: "npm run build:prod && npm run test:* && npm run deploy:*" (Runs npm build:prod, then any test script, then any deploy script)
Consider:
- Truncating long patterns with ellipsis (e.g., show first 50 chars)
- Using a multi-line tooltip format
- Showing just the pattern without the description in the tooltip
- Adding a "Show details" link that expands the full pattern
This would improve readability, especially on smaller screens.
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.
| * - "npm run build" -> "npm run" | ||
| * - "git commit -m 'message'" -> "git commit" | ||
| * - "echo 'hello world'" -> "echo" | ||
| * - "python script.py --arg value" -> "python" |
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 think this is dangerous; That I want to allow script a.py typically does not mean I want to execute b.py; to me that's if I allow one executable I allow all of them
| * - "echo 'hello world'" -> "echo" | ||
| * - "python script.py --arg value" -> "python" | ||
| * - "./scripts/deploy.sh production" -> "./scripts/deploy.sh" | ||
| * - "cd /path/to/dir && npm install" -> "cd * && npm install" |
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 think this is dangerous, as it would also allow cd ../; I would propose something like cd ./* only, or maybe combined with Kilo-Org/kilocode#770
| * - "python script.py --arg value" -> "python" | ||
| * - "./scripts/deploy.sh production" -> "./scripts/deploy.sh" | ||
| * - "cd /path/to/dir && npm install" -> "cd * && npm install" | ||
| * - "rm -rf node_modules" -> "rm" |
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 think in this specific example I'd prefer the exact rm command; I would never fully whitelist rm
|
Converted this one to draft while we discuss |
|
ok, some thoughts:
But my most burning question is why you decided to switch away from the LLM approach |
- Added new 'Add and Run' button in ChatView that appears when hovering over code blocks containing allowed commands - Button extracts commands from code blocks and adds them to allowed commands list - Automatically switches to Settings view after adding commands - Added comprehensive test coverage for the new functionality - Updated all locale files with new translation keys - Added utility function to extract command patterns from code blocks This implementation allows users to quickly add commands from AI responses to their allowed commands list with a single click.
- Fix npm/yarn/pnpm commands with additional arguments to avoid overly broad wildcards - Improve handling of chained commands with proper quote-aware parsing - Add security limits for deeply nested command chains - Enhance detection of command injection patterns - Update tests to reflect more secure pattern extraction behavior This addresses the critical security issue where commands like 'npm run build:prod -- --env=production' were creating patterns that were too permissive.
f8f49dd to
d2a0390
Compare
- Remove three-button layout (Run, Add & Run, Reject) - Implement two-row design with Run Command/Reject buttons on top - Add pattern display and 'Always allow' button on bottom row - 'Always allow' only whitelists the pattern without running it - Remove addAndRunButtonClicked handling from backend - Add new addToWhitelist message type and handler - Update translations for new UI elements - Remove obsolete tests for old Add & Run functionality
- Modified addToWhitelist handler to trigger command execution after whitelisting - Fixes issue where chat was stuck after clicking 'Always allow' - Command now gets whitelisted AND executed in one action
…4398) (#5404) * fix: add embedder validation to prevent misleading status indicators (#4398) * fix: address PR feedback and fix critical issues - Fixed settings-save flow to save before validation - Fixed Error constructor usage in scanner.ts - Fixed segment identification in file-watcher.ts - Added missing translation keys for embedder validation errors * fix: add missing Ollama translation keys - Added missing ollama.title, description, and settings keys - Fixed translation check failure in CI/CD pipeline - Synchronized all 17 non-English locale files * feat: add proactive embedder validation on provider switch - Validate embedder connection when switching providers - Prevent misleading 'Indexed' status when embedder is unavailable - Show immediate error feedback for invalid configurations - Add comprehensive test coverage for validation flow This ensures users get immediate feedback when configuring embedders, preventing confusion when providers like Ollama are not accessible. * fix: improve error handling and validation in code indexing process * refactor: extract common embedder validation and error handling logic - Created shared/validation-helpers.ts with centralized error handling utilities - Refactored OpenAI, OpenAI-Compatible, and Ollama embedders to use shared helpers - Eliminated duplicate error handling code across embedders - Improved maintainability and consistency of error handling - Fixed test compatibility in manager.spec.ts - All 2721 tests passing * refactor: simplify validation helpers by removing unnecessary wrapper functions - Removed getErrorMessageForConnectionError and inlined logic into handleValidationError - Removed isRateLimitError, logRateLimitRetry, and logEmbeddingError wrapper functions - Updated openai.ts and openai-compatible.ts to inline rate limit checking and logging - Reduced code complexity while maintaining all functionality - All 311 tests continue to pass * fix: add missing invalidResponse i18n key and fix French translation - Added missing 'invalidResponse' key to all locale files - Fixed French translation: changed 'and accessible' to 'et accessible' - Ensures proper error messages are displayed when embedder returns invalid responses * fix: restore removed score settings in webviewMessageHandler - Restored codebaseIndexSearchMaxResults and codebaseIndexSearchMinScore settings that were unintentionally removed - Keep embedder validation related changes * fix: revert unintended changes to file-watcher and scanner - Reverted point ID generation back to using line numbers instead of segmentHash - Restored { cause: deleteError } parameter in scanner error handling - These changes were unrelated to the embedder validation feature --------- Co-authored-by: Daniel Riccio <[email protected]>
….com/RooCodeInc/Roo-Code into feat/issue-5290-add-and-run-commands
- Remove 'Always Allow' button from initial command approval dialog - Add integrated whitelist functionality to CommandExecution component - Implement collapsible 'Add to Allowed Auto-Execute Commands' section - Support granular command patterns for npm and other commands - Handle chained commands with individual checkboxes - Update extractCommandPattern to avoid wildcards for better control - Add comprehensive tests for command pattern extraction
|
Closing this draft in favour of a new implementation outlined in #5480 |
|
@markijbema going with the |

Description
Fixes #5290
This PR implements the "Add and Run" button feature that allows users to quickly add commands from code blocks to their allowed commands list.
This PR is currently in draft because I believe the UI implementation could be improved. While the functionality works as specified, I have concerns about:
I would appreciate feedback on these UI/UX aspects before finalizing the implementation.
Changes Made
extractCommandPatternsto handle command parsingTesting
ChatView.addAndRun.spec.tsxfor the button functionalitySettingsView.allowedCommands.spec.tsxfor settings integrationVerification of Acceptance Criteria
Screenshots/Demo
(Note: Screenshots would be added here showing the button in action, but as this is a draft, I'm focusing on getting feedback first)
Checklist
Next Steps
I'm looking for feedback on:
Once we agree on the UI approach, I'll update the implementation accordingly.
Important
Adds 'Add and Run' button for commands in code blocks, enabling users to add commands to an allowed list and execute them, with comprehensive testing and UI updates.
extractCommandPatternandgetPatternDescriptioninextract-command-pattern.tsfor parsing command patterns.extract-command-pattern.spec.tsfor command pattern extraction and description.ChatView.addAndRun.spec.tsxandSettingsView.allowedCommands.spec.tsxfor button functionality and settings integration.This description was created by
for f8f49dd. You can customize this summary. It will automatically update as commits are pushed.