feat: add custom denial messages for auto-denied commands #8704
Conversation
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=80&v=4){kind=small}
- Update type definitions to support both string and object format for denied commands - Add UI for entering custom messages when denying commands - Use custom messages in ChatView when auto-denying commands - Add translations for new UI elements This allows users to provide specific guidance when denying commands, such as suggesting 'uv run python' when denying direct 'python' commands. Fixes #8703
…rmats for deniedCommands
dosubot
bot
added
size:L
Review SummaryI've reviewed the PR and found the following issues that should be addressed:
|
hannesrudolph
added
the
Issue/PR - Triage
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| className="grow" | ||
| data-testid="denied-command-input" | ||
| placeholder={t("settings:autoApprove.execute.customMessagePlaceholder", { | ||
| defaultValue: "Custom message (optional, e.g., 'Use uv run python instead')", |
There was a problem hiding this comment.
Remove the inline fallback English string (defaultValue) from the translation call for customMessagePlaceholder. The system automatically falls back to the English JSON if a translation is missing.
| defaultValue: "Custom message (optional, e.g., 'Use uv run python instead')", |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
| <Input | ||
| value={deniedCommandInput} | ||
| onChange={(e: any) => setDeniedCommandInput(e.target.value)} | ||
| onKeyDown={(e: any) => { |
There was a problem hiding this comment.
Consider using proper event types instead of any in onKeyDown handlers for better type safety.
| onKeyDown={(e: any) => { | |
| onKeyDown={(e: React.KeyboardEvent<HTMLInputElement>) => { |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| private mergeDeniedCommands( | ||
| globalStateCommands?: string[] | { command: string; message?: string }[], | ||
| ): { command: string; message?: string }[] { | ||
| // Handle both string[] and object[] formats | ||
| if (!globalStateCommands) { | ||
| return [] | ||
| } | ||
|
|
||
| // If it's already in the new format, return as-is | ||
| if (globalStateCommands.length > 0 && typeof globalStateCommands[0] === "object") { | ||
| return globalStateCommands as { command: string; message?: string }[] | ||
| } | ||
|
|
||
| // If it's in the old string format, convert to new format for compatibility | ||
| return (globalStateCommands as string[]).map((cmd) => ({ command: cmd })) | ||
| } |
There was a problem hiding this comment.
The new mergeDeniedCommands implementation no longer merges workspace-level denied commands with global-level ones. The original implementation delegated to mergeCommandLists which retrieved workspace configuration via vscode.workspace.getConfiguration(Package.name).get<string[]>(configKey) and merged it with global state. Now it only returns the converted global state commands without fetching workspace config. This breaks the documented merging behavior where workspace and global configurations are combined. The mergeAllowedCommands method on line 1671 still uses mergeCommandLists and correctly merges both sources. The fix should either call mergeCommandLists first or replicate its logic to fetch and merge workspace config.
|
|
||
| for (const denied of normalizedDenied) { | ||
| if (cmd.toLowerCase().startsWith(denied.command.toLowerCase())) { | ||
| if (denied.command.length > longestLength) { | ||
| longestMatch = denied | ||
| longestLength = denied.command.length | ||
| } |
There was a problem hiding this comment.
The getDeniedCommand function reimplements the longest prefix matching logic that already exists in findLongestPrefixMatch from the command-validation utility (which was removed from imports on line 29). This creates code duplication and maintenance burden. The manual loop on lines 1064-1070 duplicates the exact logic of findLongestPrefixMatch. Instead, this function should re-add findLongestPrefixMatch to imports and use it to find the matching command prefix, then look up the corresponding object from deniedCommands to get the custom message. This would reduce duplication and ensure consistent matching behavior.
| // If it's already in the new format, return as-is | ||
| if (globalStateCommands.length > 0 && typeof globalStateCommands[0] === "object") { | ||
| return globalStateCommands as { command: string; message?: string }[] |
There was a problem hiding this comment.
The type checking logic assumes that if the first element is an object, all elements are objects (line 1688). This is unsafe for arrays with mixed types. If globalStateCommands contains a mix like ["cmd1", {command: "cmd2"}], this code would incorrectly cast the entire array as objects, causing runtime errors when accessing .command on string elements. While the zod schema should prevent mixed arrays, defensive coding would handle this gracefully. Consider checking each element individually or using Array.every() to verify all elements match the expected type before casting.
2 participants
Description
This PR implements the ability to provide custom denial messages when commands are auto-denied, allowing users to provide helpful alternatives and guidance. For example, users can suggest "Use 'uv run python' instead" when denying the system-wide 'python' command.
Related Issue
Fixes #8703
Changes
Features
Testing
Screenshots
The new UI allows users to optionally add custom messages:
Important
Add custom denial messages for auto-denied commands, enhancing user guidance while maintaining backward compatibility.
AutoApproveSettings.ChatViewdisplays custom messages when commands are denied.ClineProviderandwebviewMessageHandlerto handle new command format.settings.json.This description was created by
for 9ade5a3. You can customize this summary. It will automatically update as commits are pushed.