Add support for multi-line git commit messages in auto-approve #8792
Conversation
…approve - Add test case demonstrating current behavior where multi-line commit messages fail auto-approval due to newline splitting before quote handling - Simplified reproduction of real-world scenario with git add && git commit -m - Documents expected behavior for future fix
…sing - Fix parseCommand() to protect newlines inside quoted strings before splitting - Track quote state (single/double) and handle escaped quotes - Use placeholder to preserve newlines in quotes during line splitting - Restore actual newlines after parsing completes - Update tests to expect newlines preserved in quoted strings - Fixes auto-approve for multi-line git commit messages This resolves the issue where multi-line git commit messages like: git commit -m "feat: title - point a - point b" Were incorrectly split into separate commands, causing auto-approve to fail.
- Extract protectNewlinesInQuotes() as a separate, testable function - Fix quote concatenation handling (e.g., echo '"'A'"' where A is NOT quoted) - Properly track quote boundaries without using toggle-based state - Handle escaped quotes correctly in double-quoted strings - Add comprehensive test suite for protectNewlinesInQuotes(): - Basic quote handling (single/double quotes) - Quote concatenation edge cases - Escaped quotes in different contexts - Unclosed quotes, empty strings, multiple newlines - Real-world git commit message examples - All 153 tests pass
hannesrudolph
added
the
Issue/PR - Triage
dosubot
bot
added
size:L
Review SummaryI've reviewed the changes for adding support for multi-line git commit messages in auto-approve. The implementation is well-designed with comprehensive test coverage. Issues Found
Positive Aspects
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| const quoteChar = command[i] | ||
| const prevChar = i > 0 ? command[i - 1] : "" | ||
|
|
||
| if (quoteChar === '"' && prevChar !== "\\") { |
There was a problem hiding this comment.
The escape check (prev char !== '\') only looks at the immediate previous character. For robust handling of multiple consecutive backslashes, consider counting the number of preceding backslashes.
| if (quoteChar === '"' && prevChar !== "\\") { | |
| if (quoteChar === '"' && (i === 0 || command.slice(0, i).match(/\\*$/)[0].length % 2 === 0)) { |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| if (quoteChar === '"' && prevChar !== "\\") { | ||
| // Found closing quote | ||
| result += quoteChar | ||
| i++ | ||
| break |
There was a problem hiding this comment.
The escaped quote detection doesn't handle consecutive backslashes correctly. When you have an escaped backslash followed by a quote (e.g., "test\\" representing the shell command "test\"), the function checks only if the previous character is a backslash but doesn't verify if that backslash itself is escaped. This means "test\\" would be treated as having an escaped quote when the quote should actually close the string. While this edge case may be rare in git commit messages, it could cause newlines outside quotes to be incorrectly protected if a command contains escaped backslashes before quotes. Consider counting consecutive backslashes to properly determine if a quote is escaped.
hannesrudolph
removed
the
Issue/PR - Triage
|
Sure, I could improve the backslash detection I disagree with roobot that 'correctness in all scenarios' is achievable though, because we are running this for all possible shells, and they might handle some string escaping different. In the end parsing shell commands ourselves will always have an inherent risk |
2 participants
Fixes this problem
After:
upstreamed from kilo ( Kilo-Org/kilocode#3104 );
Important
Adds support for multi-line git commit messages by handling newlines within quotes in command parsing and validation.
protectNewlinesInQuotes()incommand-validation-quote-protection.tsto replace newlines in quotes with placeholders.parseCommand()incommand-validation.tsto useprotectNewlinesInQuotes()for handling multi-line commands.getCommandDecision().command-validation-quote-protection.spec.tsto test newline protection in quotes.command-validation.spec.tsto test multi-line command handling and validation logic.NEWLINE_PLACEHOLDERandCARRIAGE_RETURN_PLACEHOLDERfor newline handling.This description was created by
for 7bdd67b. You can customize this summary. It will automatically update as commits are pushed.