insert after tool: followups from Nikola's review #157
Conversation
![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 includes several follow-up changes from a previous review, such as improving input validation by using NonEmptyString and refining tool descriptions. While these are positive changes, a critical issue has been introduced by removing a validation check in the InsertAfterSubstringTool. This removal causes the tool to fail silently if the target substring is not found in the file, which is a regression and also breaks an existing unit test. My review includes a high-severity comment to restore this check. I have also included a medium-severity suggestion to improve consistency in how tool descriptions are formatted.
| if not tool_input.insert_after_substring: | ||
| raise ToolError("No insertion was done because the specified substring wasn't provided") | ||
| try: | ||
| content = await asyncio.to_thread(tool_input.file.read_text) |
There was a problem hiding this comment.
Removing the check for the substring's existence introduces a silent failure. If insert_after_substring is not found in the file content, content.replace(...) will do nothing, and the file will be overwritten with its original content. The tool will report success, which is misleading for the user as no insertion happened.
This change also appears to break the test_insert_after_substring_missing unit test, which specifically checks for a ToolError in this scenario. Please restore the check to ensure the tool provides accurate feedback.
content = await asyncio.to_thread(tool_input.file.read_text)
if tool_input.insert_after_substring not in content:
raise ToolError("No insertion was done because the specified substring wasn't present")| description = """ | ||
| 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 | ||
| happens only once. | ||
| """ |
There was a problem hiding this comment.
For consistency with other tools in this file (like InsertTool, which was changed in this PR to have a single-line description), it would be better to make this multi-line description a single-line string as well.
description = "Inserts the provided text `new_string` on a new line after the first occurrence of the specified substring `insert_after_substring`. The insertion happens only once."
Signed-off-by: Tomas Tomecek <[email protected]> Assisted-by: Cursor(Claude)
TomasTomecek
force-pushed
the
insert-after-followups
branch
from
5ddc51f to
f6398f9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request follows up on a previous review, introducing several good improvements. The use of NonEmptyString for validating tool inputs is a great change, making the code cleaner by allowing the removal of redundant manual checks. The descriptions for tools have also been updated for better clarity. I have one minor suggestion regarding the formatting of a multi-line description string to improve consistency and maintainability.
| description = """ | ||
| 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 | ||
| happens only once. | ||
| """ |
There was a problem hiding this comment.
To improve consistency and avoid potential whitespace issues with triple-quoted strings, it's better to define this long description using implicit string concatenation inside parentheses. This is a common Python idiom for long strings and aligns with the style of other descriptions in the file.
| description = """ | |
| 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 | |
| happens only once. | |
| """ | |
| description = ( | |
| "Inserts the provided text `new_string` on a new line after the first " | |
| "occurrence of the specified substring `insert_after_substring`. The insertion " | |
| "happens only once." | |
| ) |
There was a problem hiding this comment.
I don't think the extra whitespace matters.
2 participants
No description provided.