Add support for multi-line git commit messages in auto-approve

[markijbema]

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.

• Behavior:
  • Adds protectNewlinesInQuotes() in command-validation-quote-protection.ts to replace newlines in quotes with placeholders.
  • Updates parseCommand() in command-validation.ts to use protectNewlinesInQuotes() for handling multi-line commands.
  • Ensures multi-line git commit messages are treated as single commands in getCommandDecision().
• Tests:
  • Adds command-validation-quote-protection.spec.ts to test newline protection in quotes.
  • Updates command-validation.spec.ts to test multi-line command handling and validation logic.
• Misc:
  • Introduces placeholders NEWLINE_PLACEHOLDER and CARRIAGE_RETURN_PLACEHOLDER for newline handling.

^{This description was created by for 7bdd67b. You can customize this summary. It will automatically update as commits are pushed.}

[roomote]

Review Summary

I'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

• Escaped backslash handling in quote detection may not correctly handle consecutive backslashes before quotes (e.g., "test\\")

Positive Aspects

• Clean separation of concerns with new command-validation-quote-protection.ts file
• Comprehensive test coverage including edge cases
• Proper handling of both \n and \r line endings
• Correct implementation of quote concatenation rules

Follow Along on Roo Code Cloud

[ellipsis-dev]

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.

Suggested change

	if (quoteChar === '"' && prevChar !== "\\") {
	if (quoteChar === '"' && (i === 0 || command.slice(0, i).match(/\\*$/)[0].length % 2 === 0)) {

[roomote]

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.

[roomote]

Review complete. Found 1 issue that should be addressed before approval. The escaped backslash handling edge case is a low-severity issue but should be fixed to ensure correctness in all scenarios.

[markijbema]

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

[daniel-lxs]

Thank you @markijbema for taking the time to work on this issue! 🙏

However, after further analysis, we realized that the complex placeholder substitution approach doesn't address the core UX problem. Adding long multi-line commands to the approve list doesn't make sense from a user experience perspective - users don't want to see unwieldy full git commit messages in their pattern selector.

We've created a simpler solution in #9064 that addresses this more directly:

1. Fixed the root cause - Enhanced parseCommand to properly handle quoted multi-line strings
2. Fixed the UI issue - Simply filter out multi-line patterns from the UI display so users only see clean, actionable patterns like 'git' and 'git commit'

This approach is much cleaner and addresses both the validation logic AND the UX concerns without the complexity of placeholder substitution.

Thanks again for your contribution! The analysis you provided helped us understand the issue better.