|
| 1 | +## Description |
| 2 | + |
| 3 | +Fixes #5633 |
| 4 | + |
| 5 | +This PR resolves the critical HTML entity escaping/unescaping issue that was causing diff search/replace operations to fail or produce unintended content modifications. |
| 6 | + |
| 7 | +## Problem |
| 8 | + |
| 9 | +The apply_diff tools were unescaping HTML entities (e.g., & → &) in search content for non-Claude models, but the actual file content still contained the escaped entities. This caused: |
| 10 | + |
| 11 | +1. **Search failures**: When LLMs provided search content with HTML entities, the unescaping made it not match the actual file content |
| 12 | +2. **Unintended modifications**: Partial matches could lead to incorrect replacements in files |
| 13 | + |
| 14 | +## Solution |
| 15 | + |
| 16 | +- **Removed HTML entity unescaping logic** from applyDiffTool.ts and multiApplyDiffTool.ts for non-Claude models |
| 17 | +- **Preserved HTML entities** in search content to ensure exact matching with file content |
| 18 | +- **Removed unused imports** of unescapeHtmlEntities from both files |
| 19 | +- **Added comprehensive test coverage** to prevent regression |
| 20 | + |
| 21 | +## Changes Made |
| 22 | + |
| 23 | +- src/core/tools/applyDiffTool.ts: |
| 24 | + |
| 25 | + - Removed HTML entity unescaping for non-Claude models (lines 26-28) |
| 26 | + - Removed unused import of unescapeHtmlEntities (line 13) |
| 27 | + |
| 28 | +- src/core/tools/multiApplyDiffTool.ts: |
| 29 | + |
| 30 | + - Replaced HTML entity unescaping logic with simple assignment (lines 414-418) |
| 31 | + - Removed unused import of unescapeHtmlEntities (line 13) |
| 32 | + |
| 33 | +- src/core/tools/**tests**/applyDiffHtmlEntity.spec.ts: |
| 34 | + - Added comprehensive test suite with 5 test cases |
| 35 | + - Tests verify HTML entities are preserved in search/replace operations |
| 36 | + - Tests cover various entity types: &, <, >, ', " |
| 37 | + - Tests verify both Claude and non-Claude model behavior |
| 38 | + |
| 39 | +## Testing |
| 40 | + |
| 41 | +- [x] All existing tests pass |
| 42 | +- [x] Added comprehensive tests for HTML entity handling scenarios: |
| 43 | + - [x] HTML entities preserved in search content for non-Claude models |
| 44 | + - [x] HTML entities still unescaped for Claude models (backward compatibility) |
| 45 | + - [x] Various entity types tested (&, <, >, ', ") |
| 46 | + - [x] Both single and multiple entity scenarios covered |
| 47 | +- [x] Manual testing completed: |
| 48 | + - [x] Verified search operations work with HTML entities in content |
| 49 | + - [x] Confirmed no unintended content modifications occur |
| 50 | +- [x] All linting and type checking passes |
| 51 | + |
| 52 | +## Verification of Acceptance Criteria |
| 53 | + |
| 54 | +- [x] **Search failures resolved**: HTML entities in search content now match file content exactly |
| 55 | +- [x] **Unintended modifications prevented**: No more partial matches causing incorrect replacements |
| 56 | +- [x] **Backward compatibility maintained**: Claude model behavior unchanged |
| 57 | +- [x] **No regressions**: All existing functionality preserved |
| 58 | +- [x] **Comprehensive test coverage**: Edge cases and scenarios covered |
| 59 | + |
| 60 | +## Related Issues |
| 61 | + |
| 62 | +This issue was previously attempted to be fixed in: |
| 63 | + |
| 64 | +- PR #5608 (closed without merging) |
| 65 | +- Issue #4077 (related HTML entity handling) |
| 66 | + |
| 67 | +This PR provides a complete and tested solution that addresses the root cause. |
| 68 | + |
| 69 | +## Checklist |
| 70 | + |
| 71 | +- [x] Code follows project style guidelines |
| 72 | +- [x] Self-review completed |
| 73 | +- [x] Comments added for complex logic |
| 74 | +- [x] Documentation updated (test documentation) |
| 75 | +- [x] No breaking changes |
| 76 | +- [x] All tests passing |
| 77 | +- [x] Linting and type checking passes |
0 commit comments