-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: skip full command suggestion when it contains separators #6622
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
…andPatternSelector
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 reviewed my own code and found it surprisingly coherent. Must be a bug in the review system.
| const fullCommandPattern: CommandPattern = { pattern: trimmedCommand } | ||
|
|
||
| // Check if command contains separators (&&, ||, ;, |, &) | ||
| const containsSeparators = /(?:&&|\|\||;|\||&)/.test(trimmedCommand) |
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.
Is this regex pattern robust enough? It might miss edge cases with whitespace variations like npm install &&npm test (no space before operator). The command-validation module might handle these cases differently.
Also, since command separators are used in multiple places (I see similar patterns in command-validation.ts), would it make sense to extract this as a shared constant to ensure consistency?
| const trimmedCommand = command.trim() | ||
| const fullCommandPattern: CommandPattern = { pattern: trimmedCommand } | ||
|
|
||
| // Check if command contains separators (&&, ||, ;, |, &) |
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.
Consider enhancing this comment to explicitly list all the separators being checked: &&, ||, ;, |, &. This would make it immediately clear what patterns trigger the separator detection.
| expect(screen.getByText("npm")).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it("should handle various separator types correctly", () => { |
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.
Great test coverage for the main scenarios! Consider adding a few edge case tests:
- Commands with separators but no extracted patterns
- Commands with multiple consecutive separators (e.g.,
cmd1 && || cmd2) - Commands with separators at the beginning or end (e.g.,
&& cmdorcmd &&)
These edge cases might not be common, but they could help ensure the component behaves predictably in all situations.
This PR fixes an issue in the CommandPatternSelector component where the full command was being suggested as a pattern even when it contained command separators (&&, ||, ;, |, &).
Problem
When a command like
npm install && npm testwas executed, the manage command permissions UI would suggest the entire command string as a pattern that could be allowed/denied. This was problematic because:Solution
Modified the
CommandPatternSelectorcomponent to check if the command contains separators before adding it as the first pattern. If separators are detected, only the extracted individual command patterns are shown.Changes
CommandPatternSelector.tsxto add separator detection logicTesting
Important
CommandPatternSelectornow skips suggesting full commands with separators as patterns, showing only individual commands instead.CommandPatternSelectornow skips suggesting full commands with separators (&&, ||, ;, |, &) as patterns.CommandPatternSelector.tsxto include separator detection logic.CommandPatternSelector.spec.tsxfor commands with and without separators.This description was created by
for c2082fd. You can customize this summary. It will automatically update as commits are pushed.