Fix/issue 2325 bracket duplication

[MuriloFP]

Related GitHub Issue

Closes: #2325

Roo Code Task Context (Optional)

N/A

Description

This PR fixes a bug in the apply_diff tool where closing brackets were being duplicated when appending content before them. The issue occurred when:

• A search used :start_line: to target a specific line
• The search content didn't include the closing bracket on the next line
• The replacement content included the closing bracket

Key implementation details:

• Added two helper methods: isClosingBracketLine() to detect lines containing only closing brackets, and getLastNonEmptyLine() to find the last non-empty line in content
• Modified the applyDiff method in MultiSearchReplaceDiffStrategy to detect when replacement content ends with a closing bracket that wasn't in the search
• When detected, the algorithm extends the replacement scope to include the original closing bracket line, preventing duplication
• The fix handles all bracket types (}, ], ), >) and accounts for whitespace variations

Design choices:

• Minimal change approach: Only modifies behavior for the specific edge case, maintaining backward compatibility
• The fix is localized to the bracket duplication scenario and doesn't affect the core diff algorithm
• Handles multiple consecutive bracket lines to support nested structures

Areas for review focus:

• The bracket detection logic in lines 577-619 of multi-search-replace.ts
• The regression tests added to verify the fix
• Edge case handling for different bracket types and whitespace variations

Test Procedure

Unit tests added:

• Added regression test suite "Issue Unclosed parentheses frequently appear during editing #2325 - Closing bracket duplication" with 3 comprehensive tests
• Tests cover the main bug scenario and edge cases
• All tests pass successfully

To reproduce and verify:

1. Run the specific test file: cd src && npx vitest core/diff/strategies/__tests__/multi-search-replace.spec.ts
2. Run the full diff strategies test suite: cd src && npx vitest core/diff/strategies
3. All 58 tests should pass

Manual testing:

1. Use the apply_diff tool with a search that doesn't include a closing bracket but a replacement that does
2. Verify that the closing bracket is not duplicated in the result
3. Test with different bracket types (}, ], ), >)

Testing results:

• ✅ All regression tests pass
• ✅ No existing tests broken (58/58 tests pass)
• ✅ ESLint checks pass
• ✅ TypeScript compilation successful

Pre-Submission Checklist

• Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
• Scope: My changes are focused on the linked issue (one major feature/fix per PR).
• Self-Review: I have performed a thorough self-review of my code.
• Testing: New and/or updated tests have been added to cover my changes (if applicable).
• Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
• Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

N/A - No UI changes

Documentation Updates

• No documentation updates are required.
• Yes, documentation updates are required. (Please describe what needs to be updated or link to a PR in the docs repository).

Additional Notes

This fix only modifies internal backend logic with no user-facing strings or UI changes, so no translations were required.

Get in Touch

@MuriloFP

---

Important

Fixes bracket duplication in applyDiff by extending replacement scope to include original closing bracket lines, with tests added for validation.

• Behavior:
  • Fixes bracket duplication in applyDiff in multi-search-replace.ts by extending replacement scope to include original closing bracket lines when necessary.
  • Handles all bracket types (}, ], ), >) and accounts for whitespace variations.
• Functions:
  • Adds isClosingBracketLine() and getLastNonEmptyLine() to assist in detecting and handling closing brackets.
• Tests:
  • Adds regression tests in multi-search-replace.spec.ts to cover bracket duplication scenarios and edge cases.
  • Tests ensure no duplicate brackets are present and handle various bracket types and whitespace.

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

[ellipsis-dev]

In the bracket duplication logic within applyDiff, the boolean variable 'extendReplacement' is set to true when a matching extra closing bracket is found, but it isn’t used later (only additionalLinesToReplace is used). Consider removing 'extendReplacement' to simplify the code.

[daniel-lxs]

Thanks for working on this @MuriloFP. Looking at the implementation, I still have concerns similar to what I mentioned in PR #3997 and later reiterated here.

We're adding processing logic to every search/replace operation to handle a quirk that mostly affects certain models like DeepSeek. I think we should be cautious about introducing special cases or guardrails for every model behavior. In cases like this, we might be better off improving the tool prompt or tweaking how it's described, rather than baking in logic changes that increase overall complexity.

I personally haven't seen this issue recently since that PR was opened so it seems like it mostly affected the old deepseek v3.

I'll close it for now, we can discuss in the issue if this is still happening, if not then we can close the issue.