PatchCreationTool now accepts patches w/o headers #154
Conversation
my OCD couldn't bare it Signed-off-by: Tomas Tomecek <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the PatchCreationTool to gracefully handle patches that lack author information, which previously caused git am to fail. The core logic change in wicked_git.py correctly detects this specific error and recovers by creating a commit manually. A new test case has been added to validate this new behavior. My review focuses on improving the clarity and robustness of the new test, as well as suggesting a refactoring in the implementation to improve maintainability by reducing code duplication.
| """ Since git-am is strict about the patch format, let's verify we can still apply a patch | ||
| with `patch` command even when git-am fails. | ||
| """ |
There was a problem hiding this comment.
The docstring is misleading. It states that the test verifies applying a patch with the patch command, but the implementation uses git am. The test's purpose is to validate the tool's recovery mechanism when git am fails due to a patch missing author headers. Please update the docstring to accurately describe the test's objective.
| """ Since git-am is strict about the patch format, let's verify we can still apply a patch | |
| with `patch` command even when git-am fails. | |
| """ | |
| """Verifies that the tool can handle a patch file without a proper header, | |
| which causes 'git am' to fail during the commit phase. | |
| """ |
| "2.51.0\n" | ||
| ) | ||
| # Now apply the patch with git am | ||
| subprocess.run(["git", "am", str(patch_file)], cwd=git_repo) |
There was a problem hiding this comment.
The test correctly anticipates that git am will fail, so check=True is omitted. However, to make the test more robust, it would be beneficial to explicitly assert that git am fails and does so with the expected error message. This ensures the test is precisely targeting the intended failure-recovery scenario.
Consider replacing line 377 with the following to make the test's preconditions more explicit:
# Now apply the patch with git am
proc = subprocess.run(
["git", "am", str(patch_file)], cwd=git_repo, capture_output=True, text=True
)
assert proc.returncode != 0, "git am was expected to fail but succeeded"
assert "fatal: empty ident name" in proc.stderr, "git am failed with an unexpected error"| result = output.result | ||
| assert "Successfully created a patch file" in result | ||
| assert output_patch.exists() | ||
| # The patch file should contain the commit message "Add line 3" |
There was a problem hiding this comment.
This comment is inaccurate for the current test case. The 'hideous patch' doesn't include a commit message like "Add line 3". The tool generates a generic commit message. The assertion on the next line correctly verifies the content of the patch, not the commit message. The comment should be updated to reflect this.
| # The patch file should contain the commit message "Add line 3" | |
| # The patch file should contain the added line from the original patch. |
beeai/agents/tools/wicked_git.py
Outdated
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "commit", "-m", f"Patch {tool_input.patch_file_path.name}"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| return StringToolOutput(result=f"ERROR: Git command failed: {stderr}") | ||
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "am", "--skip"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| return StringToolOutput(result=f"ERROR: Git command failed: {stderr}") |
There was a problem hiding this comment.
This block contains duplicated logic for running a subprocess and checking its exit code. The pattern of calling await run_subprocess(...) followed by if exit_code != 0: return ... is repeated for both git commit and git am --skip.
To improve maintainability and reduce this duplication, you could use the check_subprocess helper function from utils.py, which is already available in the project. This function raises a subprocess.CalledProcessError on failure, which can be caught to return the appropriate error output. This would make the recovery logic cleaner.
Here's an example of how it could be refactored:
if "fatal: empty ident name " in stderr:
try:
await check_subprocess(
["git", "commit", "-m", f"Patch {tool_input.patch_file_path.name}"], cwd=tool_input_path)
await check_subprocess(
["git", "am", "--skip"], cwd=tool_input_path)
except subprocess.CalledProcessError as e:
return StringToolOutput(result=f"ERROR: Git command failed: {e.stderr}")You would need to add import subprocess and import check_subprocess at the top of the file.
TomasTomecek
force-pushed
the
issue-158-weird-patches
branch
2 times, most recently
from
d251912 to
2911df3
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new tool, insert_after_substring, to the beeai framework, enhancing text manipulation capabilities. It also addresses an issue with git am failing on patches without headers by adding a workaround. The changes include modifications to beeai/agents/backport_agent.py, beeai/agents/tests/unit/test_tools.py, beeai/agents/tools/text.py, and beeai/agents/tools/wicked_git.py. The review focuses on correctness, efficiency, and maintainability of the new tool and the fix.
| content.replace( | ||
| tool_input.insert_after_substring, | ||
| tool_input.insert_after_substring + "\n" + tool_input.new_string | ||
| ) |
There was a problem hiding this comment.
The current implementation of content.replace will replace ALL occurrences of tool_input.insert_after_substring with the new string. This might not be the desired behavior if the substring appears multiple times in the file. It would be better to only replace the first occurrence.
Consider using content.replace(tool_input.insert_after_substring, tool_input.insert_after_substring + "\n" + tool_input.new_string, 1) to limit the replacement to the first occurrence.
await asyncio.to_thread(
tool_input.file.write_text,
content.replace(
tool_input.insert_after_substring,
tool_input.insert_after_substring + "\n" + tool_input.new_string,
1 # Replace only the first occurrence
)
)
| f" out={stdout}") | ||
| # if the patch file doesn't have the header, this will fail | ||
| # let's verify in the error message | ||
| if "fatal: empty ident name " in stderr: |
There was a problem hiding this comment.
The check for fatal: empty ident name in stderr is fragile. The exact error message might change in different versions of Git, causing the check to fail and the workaround not to be applied. It's better to rely on a more robust method to determine if the patch has a header or not.
Consider inspecting the patch file content directly to check for the presence of a header before attempting to apply it with git am.
There was a problem hiding this comment.
in theory we could parse it, but... this gives me a headache already
beeai/agents/tools/wicked_git.py
Outdated
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "commit", "-m", f"Patch {tool_input.patch_file_path.name}"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| return StringToolOutput(result=f"ERROR: Git command failed: {stderr}") | ||
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "am", "--skip"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| return StringToolOutput(result=f"ERROR: Git command failed: {stderr}") |
There was a problem hiding this comment.
If git commit fails, the code returns an error, but it doesn't handle the case where git am --skip fails. This could leave the repository in an inconsistent state. It's important to handle both potential errors to ensure the repository remains in a usable state.
Consider adding error handling for the git am --skip command to ensure that any failures are properly handled and reported.
| exit_code, stdout, stderr = await run_subprocess( | |
| ["git", "commit", "-m", f"Patch {tool_input.patch_file_path.name}"], cwd=tool_input_path) | |
| if exit_code != 0: | |
| return StringToolOutput(result=f"ERROR: Git command failed: {stderr}") | |
| exit_code, stdout, stderr = await run_subprocess( | |
| ["git", "am", "--skip"], cwd=tool_input_path) | |
| if exit_code != 0: | |
| return StringToolOutput(result=f"ERROR: Git command failed: {stderr}") | |
| exit_code, stdout, stderr = await run_subprocess( | |
| ["git", "commit", "-m", f"Patch {tool_input.patch_file_path.name}"], cwd=tool_input_path) | |
| if exit_code != 0: | |
| return StringToolOutput(result=f"ERROR: Git command failed: {stderr}") | |
| exit_code, stdout, stderr = await run_subprocess( | |
| ["git", "am", "--skip"], cwd=tool_input_path) | |
| if exit_code != 0: | |
| return StringToolOutput(result=f"ERROR: Git command failed: {stderr} after commit, possible data loss, manual intervention needed.") |
| @pytest.mark.parametrize( | ||
| "insert_after_substring, final_content", | ||
| [ | ||
| ( | ||
| "Line 2", | ||
| "Line 1\nLine 2\nInserted line\nLine 3\n", | ||
| ), | ||
| ( | ||
| "Line 3", | ||
| "Line 1\nLine 2\nLine 3\nInserted line\n", | ||
| ), | ||
| ( | ||
| "Line 2", | ||
| "Line 1\nLine 2\nInserted line\nLine 3\n", | ||
| ), | ||
| ], |
There was a problem hiding this comment.
The test cases for test_insert_after_substring are limited and do not cover edge cases such as:
- Empty
insert_after_substring. insert_after_substringat the beginning or end of the file.- Overlapping substrings.
insert_after_substringnot found in the file.
Consider adding more comprehensive test cases to ensure the tool functions correctly under various conditions.
There was a problem hiding this comment.
I added one more case and added a check for if not insert_after_substring
| # if the patch file doesn't have the header, this will fail | ||
| # let's verify in the error message | ||
| if "fatal: empty ident name " in stderr: | ||
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "commit", "-m", f"Patch {tool_input.patch_file_path.name}"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| return StringToolOutput(result=f"ERROR: Git command failed: {stderr}") | ||
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "am", "--skip"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| return StringToolOutput(result=f"ERROR: Git command failed: {stderr}") | ||
| else: |
There was a problem hiding this comment.
The code attempts to handle the case where the patch file doesn't have a header by committing and skipping the git am step. However, this approach might not be robust and could lead to unexpected behavior or data loss if the commit fails or if there are other issues with the patch application.
Consider using the patch command instead of git am when the header is missing. This would provide a more reliable way to apply the patch without relying on git am internals.
beeai/agents/tools/wicked_git.py
Outdated
| if exit_code != 0: | ||
| return StringToolOutput(result=f"ERROR: Git command failed: {stderr}") | ||
| else: | ||
| return StringToolOutput(result=f"ERROR: git-am failed: {stderr} out={stdout}") |
There was a problem hiding this comment.
Just a note that we should raise exceptions from tools now, it will provide a bit more context to the model and it's generally more pythonic. But feel free to leave it as it is, I plan to fix this in all tools in the future.
There was a problem hiding this comment.
nice, didn't know that, will do
| content.replace( | ||
| tool_input.insert_after_substring, | ||
| tool_input.insert_after_substring + "\n" + tool_input.new_string | ||
| ) |
beeai/agents/tools/text.py
Outdated
| class InsertAfterSubstringTool(Tool[InsertAfterSubstringToolInput, ToolRunOptions, StringToolOutput]): | ||
| name = "insert_after_substring" | ||
| description = """ | ||
| Inserts the specified text after the first occurrence of the specified substring in a file as a new line. |
There was a problem hiding this comment.
This is still a bit ambiguous - should the text to insert be ended with a newline?
There was a problem hiding this comment.
yeah, understood, I am trying to figure out how to make it foolproof because even I was surprised all these additions are new lines
TomasTomecek
force-pushed
the
issue-158-weird-patches
branch
from
2911df3 to
caad300
Compare
beeai/agents/tools/text.py
Outdated
| content.replace( | ||
| tool_input.insert_after_substring, | ||
| tool_input.insert_after_substring + "\n" + tool_input.new_string, | ||
| 1 # Replace only the first occurrence |
There was a problem hiding this comment.
Just a nitpick, I think this is better than a comment:
| 1 # Replace only the first occurrence | |
| count=1, |
EDIT: Oh, wait, that's new in Python 3.13, maybe we should be safe 🙂
There was a problem hiding this comment.
hah, so that's why gemini didn't suggest the count then
TomasTomecek
force-pushed
the
issue-158-weird-patches
branch
3 times, most recently
from
fbe132d to
d625e99
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable new tool, InsertAfterSubstringTool, which will help the LLM make more precise code insertions. The changes also include a crucial fix for GitPatchCreationTool to handle patches that lack headers, preventing git am failures. The implementation is solid and includes relevant tests. I've provided a few suggestions for improvement, mainly concerning error message formatting and test case cleanup, to enhance the code's robustness and maintainability.
beeai/agents/tools/wicked_git.py
Outdated
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "commit", "-m", f"Patch {tool_input.patch_file_path.name}"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| raise ToolError("Command git-commit failed: {stderr}") |
There was a problem hiding this comment.
The error message here is not an f-string, so {stderr} will be treated as a literal string instead of being replaced by the variable's value. This will result in unhelpful error messages. You should use an f-string.
| raise ToolError("Command git-commit failed: {stderr}") | |
| raise ToolError(f"Command git-commit failed: {stderr}") |
beeai/agents/tools/wicked_git.py
Outdated
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "am", "--skip"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| raise ToolError("Command git-am failed: {stderr}") |
There was a problem hiding this comment.
Similar to the comment above, this error message is not an f-string. The {stderr} will not be interpolated. Please use an f-string to include the actual error in the message.
| raise ToolError("Command git-am failed: {stderr}") | |
| raise ToolError(f"Command git-am failed: {stderr}") |
beeai/agents/tools/wicked_git.py
Outdated
| if exit_code != 0: | ||
| raise ToolError("Command git-am failed: {stderr}") | ||
| else: | ||
| raise ToolError("Command git-am failed: {stderr} out={stdout}") |
There was a problem hiding this comment.
This error message is also not an f-string. The {stderr} and {stdout} variables will not be interpolated. Please use an f-string to provide a meaningful error message.
| raise ToolError("Command git-am failed: {stderr} out={stdout}") | |
| raise ToolError(f"Command git-am failed: {stderr} out={stdout}") |
| ( | ||
| "Line 2", | ||
| "Line 1\nLine 2\nInserted line\nLine 3\n", | ||
| ), |
There was a problem hiding this comment.
This test case is a duplicate of the first one in this parametrize block (lines 274-277). You can remove it to avoid redundancy.
| content.replace( | ||
| tool_input.insert_after_substring, | ||
| tool_input.insert_after_substring + "\n" + tool_input.new_string, | ||
| 1 # Replace only the first occurrence, 'count' kw introduced in Python 3.13 |
There was a problem hiding this comment.
The comment here is incorrect. The count parameter for str.replace() has been part of Python for a long time and was not introduced in Python 3.13. You can correct the comment or remove the part about Python 3.13.
| 1 # Replace only the first occurrence, 'count' kw introduced in Python 3.13 | |
| 1 # Replace only the first occurrence |
There was a problem hiding this comment.
sorry Gemini, Nikola is right
Allow the count argument of str.replace() to be a keyword. (Contributed by Hugo van Kemenade in gh-106487.)
TomasTomecek
force-pushed
the
issue-158-weird-patches
branch
from
d625e99 to
ceb2722
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new InsertAfterSubstringTool and enhances GitPatchCreationTool to handle patches without headers by providing a fallback commit mechanism. The changes are generally well-implemented and include corresponding tests. My review focuses on improving test clarity and removing redundancy, ensuring consistent error handling within the modified tools, and clarifying the documentation for the new tool to ensure it's used correctly by the agent.
| if exit_code != 0: | ||
| return StringToolOutput(result=f"ERROR: git-am failed: {stderr}," | ||
| f" out={stdout}") | ||
| # if the patch file doesn't have the header, this will fail | ||
| # let's verify in the error message | ||
| if "fatal: empty ident name " in stderr: | ||
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "commit", "-m", f"Patch {tool_input.patch_file_path.name}"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| raise ToolError(f"Command git-commit failed: {stderr}") | ||
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "am", "--skip"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| raise ToolError(f"Command git-am failed: {stderr}") | ||
| else: | ||
| raise ToolError(f"Command git-am failed: {stderr} out={stdout}") |
There was a problem hiding this comment.
You've correctly used raise ToolError for failures within the new logic for handling headerless patches. This is a great improvement over returning an error message in StringToolOutput. However, the rest of the _run method in GitPatchCreationTool still returns StringToolOutput for other errors. For consistency and more robust error handling, you should refactor all error paths in this method to raise ToolError instead. This will make the tool's behavior more predictable for the agent.
TomasTomecek
force-pushed
the
issue-158-weird-patches
branch
from
56ee892 to
b984690
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces two main changes. First, it enhances GitPatchCreationTool to handle patches without headers, which previously caused git am to fail. The new logic correctly commits the changes and continues the process. Second, it adds a new InsertAfterSubstringTool to allow inserting text after a specific substring, which is a useful addition for the LLM.
My review focuses on improving the robustness and quality of the new code. I've pointed out a redundant test case that should be removed and an issue in the exception handling of the new tool that could mask specific error messages. Addressing these points will make the code more robust and maintainable.
TomasTomecek
force-pushed
the
issue-158-weird-patches
branch
from
b984690 to
bc3da49
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the PatchCreationTool to gracefully handle patches that lack proper headers, preventing git am failures. It also introduces a new InsertAfterSubstringTool for more flexible text manipulation. The changes are well-tested and address a real-world issue. My review includes a few suggestions to improve test quality by removing a duplicate case, enhance exception handling in the new tool to prevent error masking, and improve consistency in error handling within the wicked_git tool.
if the name/email is not specified, it fails with > fatal: empty ident name (for <>) not allowed not a fatal error actually, the patch is still applied but git doesn't know how to commit it, so we have to do it instead Signed-off-by: Tomas Tomecek <[email protected]> Assisted-by: Cursor(Claude) Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Unfortunately LLMs can't count line numbers so when they insert new
'Patch' lines, they make mistakes easily.
This new tool should make the 'Patch' line addition more deterministic.
Example:
```
"input": {
"file": "/git-repos/RHEL-107890/apr/apr.spec",
"insert_after_substring": "Patch4: apr-1.7.2-autoconf.patch",
"new_string": "Patch5: RHEL-107890.patch"
},
"output": "Successfully inserted the specified text into /git-repos/RHEL-107890/apr/apr.spec"
```
Signed-off-by: Tomas Tomecek <[email protected]>
Assisted-by: Cursor(Claude)
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
TomasTomecek
force-pushed
the
issue-158-weird-patches
branch
from
bc3da49 to
02ea1e8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces two main changes. First, it enhances GitPatchCreationTool to handle patches that lack proper headers, preventing git am failures by manually committing the changes. This is a good improvement in robustness. The error handling in this tool has also been partially refactored to use ToolError exceptions instead of returning error strings, which is a positive step towards more consistent and predictable tool behavior. Second, a new InsertAfterSubstringTool is added, allowing text insertion based on content rather than line numbers, which is less error-prone for LLM agents. The new tool is well-implemented and comes with good unit tests. My review includes a couple of suggestions to improve consistency in error handling and clarify a code comment.
| content.replace( | ||
| tool_input.insert_after_substring, | ||
| tool_input.insert_after_substring + "\n" + tool_input.new_string, | ||
| 1 # Replace only the first occurrence, 'count' kw introduced in Python 3.13 |
There was a problem hiding this comment.
This comment is slightly misleading. The count parameter for str.replace() has existed for a long time as a positional argument. It's in Python 3.13 that its signature changes to become keyword-only. The current code is correct, but the comment could be simplified to avoid potential confusion for future maintainers.
| 1 # Replace only the first occurrence, 'count' kw introduced in Python 3.13 | |
| 1 # Replace only the first occurrence |
There was a problem hiding this comment.
see above, Gemini is incorrect here, the keyword was truly introduced in 3.13
| if exit_code != 0: | ||
| return StringToolOutput(result=f"ERROR: git-am failed: {stderr}," | ||
| f" out={stdout}") | ||
| # if the patch file doesn't have the header, this will fail | ||
| # let's verify in the error message | ||
| if "fatal: empty ident name " in stderr: | ||
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "commit", "-m", f"Patch {tool_input.patch_file_path.name}"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| raise ToolError(f"Command git-commit failed: {stderr}") | ||
| exit_code, stdout, stderr = await run_subprocess( | ||
| ["git", "am", "--skip"], cwd=tool_input_path) | ||
| if exit_code != 0: | ||
| raise ToolError(f"Command git-am failed: {stderr}") | ||
| else: | ||
| raise ToolError(f"Command git-am failed: {stderr} out={stdout}") |
There was a problem hiding this comment.
Using raise ToolError here is a great improvement for robust error handling, and it's good to see this pattern applied in the new exception handling block at the end of the function as well.
However, this change introduces an inconsistency within the _run method. Several earlier checks in this function still return an error message via StringToolOutput instead of raising a ToolError.
For example:
- Line 102:
return StringToolOutput(result=f"ERROR: Repository path does not exist: {tool_input_path}") - Line 106:
return StringToolOutput(result=f"ERROR: Not a git repository: {tool_input_path}") - And several others for failed
gitcommands.
To improve consistency and maintainability, consider refactoring all error-handling paths in this tool to raise ToolError. This would make the tool's behavior more predictable for the agent framework.
There was a problem hiding this comment.
we will do that in the future for all of these
| new_string: str = Field(description="Text to insert") | ||
|
|
||
|
|
||
| class InsertAfterSubstringTool(Tool[InsertAfterSubstringToolInput, ToolRunOptions, StringToolOutput]): |
| name = "insert" | ||
| description = """ | ||
| Inserts the specified text at a specific location in a file. | ||
| Inserts the specified text at a specific location in a file |
| Inserts the provided text new_string on a new line after the first | ||
| occurrence of the specified substring insert_after_substring. The insertion |
There was a problem hiding this comment.
| Inserts the provided text new_string on a new line after the first | |
| occurrence of the specified substring insert_after_substring. The insertion | |
| Inserts the provided text `new_string` on a new line after the first | |
| occurrence of the specified substring `insert_after_substring`. The insertion |
| if not tool_input.insert_after_substring: | ||
| raise ToolError("No insertion was done because the specified substring wasn't provided") |
There was a problem hiding this comment.
You could just type the argument as NonEmptyString:
ai-workflows/beeai/common/validators.py
Line 20 in 7cd29d3
3 participants
if the name/email is not specified, it fails with
not a fatal error actually, the patch is still applied but git doesn't
know how to commit it, so we have to do it instead
The root cause is solved, but I still can't get a good backport.
This PR also adds a new tool
insert_after_substringso the LLM can reference specific text as a location instead of a line number where they can easily make mistakes if they read the whole file.Fixes https://github.com/packit/jotnar/issues/158