fix: prevent start_line/end_line in apply_diff REPLACE

[KJ7LNW]

Context

This pull request addresses an issue where :start_line: and :end_line: markers could be erroneously included in the REPLACE section of an apply_diff operation. These markers are intended only for the SEARCH section to specify the target lines for modification. Including them in the REPLACE section could lead to unexpected behavior or errors.

This change ensures that apply_diff operations are more robust by validating the placement of these markers.

Fixes: #4013

Implementation

The MultiSearchReplaceDiffStrategy in src/core/diff/strategies/multi-search-replace.ts has been updated to include a validation step within the validateMarkerSequencing method. This new validation specifically checks if :start_line: or :end_line: markers (when not escaped) are present in the content between ======= and >>>>>>> REPLACE.

If such markers are found, the diff operation will fail with an informative error message guiding the user on the correct format and the allowed placement of these markers.

Corresponding unit tests have been added to src/core/diff/strategies/__tests__/multi-search-replace.test.ts to verify this new validation logic, ensuring that invalid placements are rejected and valid placements (including escaped markers in the REPLACE section) are correctly processed.

How to Test

The core functionality can be tested by attempting apply_diff operations:

1. With :start_line: or :end_line: incorrectly placed in the REPLACE section (should fail with an error).
2. With these markers correctly placed in the SEARCH section (should succeed).
3. With escaped line markers (e.g., \:start_line:) in the REPLACE section (should succeed, treating them as literal text).

Get in Touch

Discord: KJ7LNW

---

Important

Adds validation to prevent :start_line: and :end_line: markers in REPLACE section of apply_diff, with tests to verify behavior.

• Behavior:
  • Prevents :start_line: and :end_line: markers in REPLACE section of apply_diff.
  • Adds validation in validateMarkerSequencing() in multi-search-replace.ts.
  • If markers are found, operation fails with error message.
• Tests:
  • Adds tests in multi-search-replace.test.ts to verify validation logic.
  • Tests include cases for invalid and valid marker placements, including escaped markers.

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

[daniel-lxs]

Hey @KJ7LNW, is there a reason to prevent the tool from executing instead of just removing the incorrect :start_line: or :end_line:?

We already handle some errors by the models this way, for example, when the closing tag of a tool call is omitted, does it make sense to do the same here? Maybe in the future we can call a tool but also provide feedback to the model at the same time.

[KJ7LNW]

Hey @KJ7LNW, is there a reason to prevent the tool from executing instead of just removing the incorrect :start_line: or :end_line:?

We already handle some errors by the models this way, for example, when the closing tag of a tool call is omitted, does it make sense to do the same here? Maybe in the future we can call a tool but also provide feedback to the model at the same time.

This pull requests fixes an unlikely event. It happens so rarely that I do not think it justifies intentionally breaking data integrity. This is why:

When designing these tools we should always support creation of any arbitrary string: what if it really needs to create :end_line: for some unknown future use case by some random developer? For that case the model must receive feedback and be told to use \:end_line:, otherwise it would never be able to perform the action.

[daniel-lxs]

For that case the model must receive feedback and be told to use \:end_line:, otherwise it would never be able to perform the action.

I agree, this looks good to me then, since I've had this happen a couple of times with Gemini.