Fix #133 SanitizeCommand removes comments #134
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SanitizeCommand
Caller->>SanitizeCommand: Provide multi-line command string
SanitizeCommand->>SanitizeCommand: Split input into lines
loop For each line
SanitizeCommand->>SanitizeCommand: Trim whitespace
alt Line is comment or blank
SanitizeCommand->>SanitizeCommand: Skip line
else Line ends with backslash
SanitizeCommand->>SanitizeCommand: Remove backslash, append space
else
SanitizeCommand->>SanitizeCommand: Append line as-is
end
end
SanitizeCommand->>SanitizeCommand: Rejoin cleaned lines
SanitizeCommand->>SanitizeCommand: Split into arguments (shlex)
SanitizeCommand-->>Caller: Return argument array or error
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
proxy/config.go(1 hunks)proxy/config_posix_test.go(1 hunks)proxy/config_windows_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests
🔇 Additional comments (2)
proxy/config_posix_test.go (1)
17-25: Good test additions for comment handling and whitespace!The test has been appropriately expanded to include various comment lines and whitespace formatting that should be handled by the updated sanitizer. This ensures the function correctly removes comment lines while preserving the actual command arguments.
proxy/config_windows_test.go (1)
14-25: Test modifications look good for Windows platformThe changes to the Windows test properly mirror the improvements made in the POSIX test, ensuring consistent behavior across platforms. The test now verifies that comments and whitespace are correctly handled in the Windows environment as well.
| func SanitizeCommand(cmdStr string) ([]string, error) { | ||
| // Remove trailing backslashes | ||
| cmdStr = strings.ReplaceAll(cmdStr, "\\ \n", " ") | ||
| cmdStr = strings.ReplaceAll(cmdStr, "\\\n", " ") | ||
| var cleanedLines []string | ||
| for _, line := range strings.Split(cmdStr, "\n") { | ||
| trimmed := strings.TrimSpace(line) | ||
| // Skip comment lines | ||
| if strings.HasPrefix(trimmed, "#") { | ||
| continue | ||
| } | ||
| // Handle trailing backslashes by replacing with space | ||
| if strings.HasSuffix(trimmed, "\\") { | ||
| cleanedLines = append(cleanedLines, strings.TrimSuffix(trimmed, "\\")+" ") | ||
| } else { | ||
| cleanedLines = append(cleanedLines, line) | ||
| } | ||
| } | ||
|
|
||
| // put it back together | ||
| cmdStr = strings.Join(cleanedLines, "\n") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good implementation for handling comments and line continuations
The refactored implementation properly addresses the issue by processing the command string line-by-line, which is more robust than a global replacement approach. The function now correctly:
- Skips lines that are comments (start with #)
- Handles trailing backslashes as line continuations
- Preserves original lines that don't need special handling
However, there's one inconsistency in how whitespace is handled:
- cleanedLines = append(cleanedLines, line)
+ cleanedLines = append(cleanedLines, trimmed)For non-continuation lines, you're preserving the original line with its whitespace, but for continuation lines, you're using the trimmed version. This inconsistency could lead to unexpected behavior with whitespace. Since you're trimming the whitespace for checking comments and backslashes, it would be better to consistently use the trimmed version for all lines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func SanitizeCommand(cmdStr string) ([]string, error) { | |
| // Remove trailing backslashes | |
| cmdStr = strings.ReplaceAll(cmdStr, "\\ \n", " ") | |
| cmdStr = strings.ReplaceAll(cmdStr, "\\\n", " ") | |
| var cleanedLines []string | |
| for _, line := range strings.Split(cmdStr, "\n") { | |
| trimmed := strings.TrimSpace(line) | |
| // Skip comment lines | |
| if strings.HasPrefix(trimmed, "#") { | |
| continue | |
| } | |
| // Handle trailing backslashes by replacing with space | |
| if strings.HasSuffix(trimmed, "\\") { | |
| cleanedLines = append(cleanedLines, strings.TrimSuffix(trimmed, "\\")+" ") | |
| } else { | |
| cleanedLines = append(cleanedLines, line) | |
| } | |
| } | |
| // put it back together | |
| cmdStr = strings.Join(cleanedLines, "\n") | |
| func SanitizeCommand(cmdStr string) ([]string, error) { | |
| var cleanedLines []string | |
| for _, line := range strings.Split(cmdStr, "\n") { | |
| trimmed := strings.TrimSpace(line) | |
| // Skip comment lines | |
| if strings.HasPrefix(trimmed, "#") { | |
| continue | |
| } | |
| // Handle trailing backslashes by replacing with space | |
| if strings.HasSuffix(trimmed, "\\") { | |
| cleanedLines = append(cleanedLines, strings.TrimSuffix(trimmed, "\\")+" ") | |
| } else { | |
| - cleanedLines = append(cleanedLines, line) | |
| + cleanedLines = append(cleanedLines, trimmed) | |
| } | |
| } | |
| // put it back together | |
| cmdStr = strings.Join(cleanedLines, "\n") |
🤖 Prompt for AI Agents
In proxy/config.go around lines 231 to 249, the SanitizeCommand function
inconsistently handles whitespace by appending trimmed lines for continuation
lines but original lines for others. To fix this, always use the trimmed version
of each line when appending to cleanedLines, ensuring consistent whitespace
handling throughout the function.
Summary by CodeRabbit
Bug Fixes
Tests