Skip to content

Conversation

@mrubens
Copy link
Collaborator

@mrubens mrubens commented Apr 13, 2025

Fixes #2556 by using start line as a hint for where to start searching for diffs, versus previously looking for the chunk specified by start and end line. I think this could have caused incorrectly applied diffs in lots of cases depending on how the LLM interpreted the end line parameter.

This also removes the ability to pass in an empty search block, since we have no way to verify that the line number from the LLM is correct. The model should always be required to pass in a search block.


Important

Disallow empty search content (insertions) in MultiSearchReplaceDiffStrategy, remove related tests, and add a fuzzy matching test for issue #2556.

  • Behavior change in multi-search-replace.ts:
    • Disallows empty search content in MultiSearchReplaceDiffStrategy.applyDiff(). Now returns an error if search content is empty, removing support for insertions via empty search blocks.
    • Updates getSimilarity() to return 0 for empty search content (was 1).
  • Tests in multi-search-replace.test.ts:
    • Removes all tests for insertions (empty search block with non-empty replace block).
    • Adds test case for fuzzy matching with slightly off line numbers to reproduce Incorrect applying of apply_diff #2556.
    • Renames test group from "insertion/deletion" to "deletion" to reflect removal of insertion tests.
  • No changes to deletion or replacement logic.

This description was created by Ellipsis for 842bb2b. It will automatically update as commits are pushed.

@changeset-bot
Copy link

changeset-bot bot commented Apr 13, 2025

⚠️ No Changeset found

Latest commit: 842bb2b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Apr 13, 2025
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Apr 14, 2025
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 14, 2025
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Apr 14, 2025
@mrubens mrubens changed the title Add test for issue #2556 Fix overdependence on start/end lines in diff strategy Apr 14, 2025
const replacements = matches
.map((match) => ({
startLine: Number(match[2] ?? 0),
endLine: Number(match[4] ?? resultLines.length),
Copy link
Collaborator Author

@mrubens mrubens Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not using the passed in endLine anymore (just taking the start line plus the length of the search block), but I figure we can clean that up as a follow-up if this doesn't cause any unforeseen problems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@cte cte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 14, 2025
@mrubens mrubens merged commit b196489 into main Apr 14, 2025
16 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Apr 14, 2025
@mrubens mrubens deleted the issue_2556_test branch April 14, 2025 21:09
@KJ7LNW
Copy link
Contributor

KJ7LNW commented Apr 14, 2025

The model should always be required to pass in a search block.

I think this means it can no longer modify an empty file. maybe we do not care and that should be the responsibility of write_to_file, but heads up ...

@mrubens
Copy link
Collaborator Author

mrubens commented Apr 14, 2025

Thanks yeah, that’s what I was thinking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect applying of apply_diff

4 participants