fix(tools): @insert_edit_into_file tool: Allow patch starting by empty line before a @@ identifier. #1663
+33
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is especially usefull when the LLM forget to add the begin and end markers. It indeed uses to start with for example "\\n@@Focus()" we previously assumed that this was the start of a new focus and we then inserted the ongoing 'patch' in the table. But in this case, the current patch is empty and trying to parse an empty patch returned an error. With this change, we only insert the ongoing patch into the table if it is not empty. A new test case has been added to validate the correct handling of patches that start with empty lines. A duplicate test name was also resolved for clarity.
pratyushmittal
approved these changes
Jun 18, 2025
Comment on lines
105
to
106
| -- only when the next line uses @@ identifier | ||
| table.insert(changes, change) | ||
| change = get_new_change() | ||
| elseif line:sub(1, 1) == "-" then |
There was a problem hiding this comment.
This is a good catch.
Just a small nitpick, can we add a noop do end to show we need to do nothing in this else block?
Something like:
elseif line == "" and lines[i + 1] and lines[i + 1]:match("^@@") then
-- empty lines can be part of pre/post context
-- we treat empty lines as new change block and not as post context
-- only when the next line uses @@ identifier
-- skip this line and do nothing
do
end
elseif line:sub(1, 1) == "-" then
There was a problem hiding this comment.
good idea. thank you @pratyushmittal .
✅
|
I'm viewing this on my mobile so may have missed some detail but can we also update the tests to be patch tests after @pratyushmittal's test refactor was merged. |
… need to do nothing in this else block
Hi @olimorris , |
This was referenced Jun 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is especially usefull when the LLM forget to add the begin and end markers. It indeed uses to start with for example "\n@@Focus()" we previously assumed that this was the start of a new focus and we then inserted the ongoing 'patch' in the table. But in this case, the current patch is empty and trying to parse an empty patch returned an error. With this change, we only insert the ongoing patch into the table if it is not empty.
A new test case has been added to validate the correct handling of patches that start with empty lines. A duplicate test name was also resolved for clarity.
Description
This PR is a little complement to the nice work done by @pratyushmittal in this #1653
It allows patches starting by empty line before a @@ identifier.
Checklist
CodeCompanion.hasin the init.lua file for my new featuremake allto ensure docs are generated, tests pass and my formatting is applied