Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
CLA Not Signed The Contributor License Agreement (CLA) check is currently pending on this PR ( @wh-whnb please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/agentscope-ai/agentscope-java. Once signed, the Automated check by github-manager-bot |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Verified the three failure modes described in the issue against the code: before this change write_text_file only checked start > originalLines.size(), so "5,2" took the end < size tail branch and duplicated lines, and "[-3,-1]" reached subList(-1, …) and threw IndexOutOfBoundsException. Both new guards sit before the first mutation (Files.writeString), so the "file untouched on rejection" claim in the new tests is real, and the tests assert it rather than just asserting the return value — that is the right shape for a destructive tool.
Commenting rather than approving: the license/cla status is still pending on 53681262 (and build (windows-latest) is still running), so this cannot be approved from my side until the CLA check goes green. The two [Warning] items below are independent of that.
Findings
- [Warning]
WriteFileTool.java:331— the 1-based / no-negative rule is not stated in therangesparameter description, while the siblingview_text_filedocuments and supports negative indices (ReadFileTool.java:96, normalisation at:166-176) — the model is being told two different grammars for the samerangesargument - [Warning]
WriteFileToolTest.java:52— the assertion pins a second silent mutation:String.join("\n", …)(WriteFileTool.java:379) drops the file's trailing newline on any range replace - [Info]
WriteFileTool.java:341-352—startbeyond EOF errors butendbeyond EOF is silently clamped; worth one test to record the intent - [Info]
WriteFileToolTest.java:87-95—assertNotEquals(ERROR, …)and theFiles.exists(...) ? … : nullternary are weaker than they need to be
Suggestions
The schema fix is one string. In writeTextFile's @ToolParam(description = …):
"The range of lines to be replaced as [start, end], e.g., '[1,5]' or '1,5'. "
+ "Lines are 1-based and inclusive; negative indices are NOT supported for writes "
+ "(view_text_file accepts them, this tool does not). "
+ "If null or empty, the entire file will be overwritten."If you would rather the two tools agree than diverge deliberately, lift ReadFileTool's negative-index block into FileToolUtils next to parseRanges and call it from both — write_text_file can then treat [-3,-1] as "the last three lines" with the same guards, and start < 1 only needs to reject the genuinely unusable cases.
Automated review by github-manager-bot
| logger.debug( | ||
| "Replacing lines {}-{} in file: {}", start, end, filePath); | ||
|
|
||
| if (start < 1) { |
There was a problem hiding this comment.
[Warning] The guard itself is correct — but nothing the model can see says so. The ranges @ToolParam description of write_text_file still reads only "The range of lines to be replaced as [start, end], e.g., '[1,5]' or '5'. If null or empty, the entire file will be overwritten.": no 1-based statement, no "negatives rejected". Meanwhile the sibling read tool actively advertises the opposite — ReadFileTool.java:96 ("Supports negative indices to view from the end") — and normalises them at ReadFileTool.java:166-176 (start = totalLines + start + 1). An agent that legally read the tail of a file with "[-3,-1]" and now rewrites that same range is following the documented schema, so it will keep hitting this error.
Cheapest fix: state the constraint where the model reads it, e.g. "1-based inclusive line range; negative indices are not supported for writes". More consistent fix: move ReadFileTool's normalisation into FileToolUtils (which already owns parseRanges) and share it, so [-3,-1] means the same thing in both tools and a write that only replaces the last 3 lines is possible at all. Either way, please don't leave the two tools documenting different range grammars.
| ToolResultBlock result = tool.writeTextFile(file.toString(), "NEW", "2,3").block(); | ||
|
|
||
| assertEquals(ToolResultState.RUNNING, result.getState()); | ||
| assertEquals("one\nNEW\nfour\nfive", readFile()); |
There was a problem hiding this comment.
[Warning] This assertion pins a different silent-mutation bug than the one the PR fixes. ORIGINAL_CONTENT ends with \n, and the rewrite path builds the file with String.join("\n", newContent) (WriteFileTool.java:379), which never appends a terminator — so any range replace drops the file's final newline, and "one\nNEW\nfour\nfive" (no trailing \n) is now the blessed expected value.
For a line-range editor that is the same class of harm as #3206-style corruption: the line count the agent saw on its previous view_text_file no longer matches, insert_text_file line numbering shifts, and git diff reports the last line as modified. Suggest preserving whatever terminator the original had:
String joinedContent = String.join("\n", newContent);
if (Files.readString(path, StandardCharsets.UTF_8).endsWith("\n")) {
joinedContent += "\n";
}If dropping it really is intended, then rename the test (replacesSpecifiedRange_dropsTrailingNewline) so the next contributor fixes it deliberately instead of tripping over the assertion.
| if (start > end) { | ||
| logger.warn( | ||
| "Invalid range: start {} > end {} for file: {}", | ||
| start, | ||
| end, | ||
| filePath); | ||
| return ToolResultBlock.error( | ||
| String.format( | ||
| "Invalid range: start line %d is greater than" | ||
| + " end line %d.", | ||
| start, end)); | ||
| } |
There was a problem hiding this comment.
[Info] Ordering is right (cheap, most-likely model error first, before the existing start > originalLines.size() check). One asymmetry worth pinning down: start beyond EOF errors, but end beyond EOF is silently clamped by the tail condition if (end < originalLines.size()) further below, so "3,999" on a 5-line file succeeds and replaces 3..EOF. I suspect that's what you want — it just isn't covered, and the two bounds behaving differently is exactly the kind of thing that gets "fixed" into a regression later. One test asserting the clamp would settle it.
| @Test | ||
| void createsNewFileWhenFileDoesNotExist() { | ||
| Path newFile = tempDir.resolve("new.txt"); | ||
|
|
||
| ToolResultBlock result = | ||
| tool.writeTextFile(newFile.toString(), "fresh content", null).block(); | ||
|
|
||
| assertNotEquals(ToolResultState.ERROR, result.getState()); | ||
| assertEquals("fresh content", Files.exists(newFile) ? readFile(newFile) : null); |
There was a problem hiding this comment.
[Info] Two small test-strength points: (1) assertNotEquals(ToolResultState.ERROR, result.getState()) is weaker than the positive assertEquals(ToolResultState.RUNNING, ...) used in the first test — worth matching it so an unexpected future state is caught; (2) the Files.exists(newFile) ? readFile(newFile) : null ternary duplicates what assertEquals already proves, and if the file is missing the failure message reads like a content mismatch rather than "file was not created" — assertTrue(Files.exists(newFile)) first, then assertEquals("fresh content", readFile(newFile)) says it more clearly.
…rruption The write_text_file tool only checked that the start line did not exceed the file length. Ranges such as [5,2] (reversed), [0,2] (zero-based) or [-3,-1] (negative) passed validation and reached the content assembly logic, which silently duplicated or deleted lines and still reported success; negative indexes could surface an IndexOutOfBoundsException. Add start >= 1 and start <= end checks before mutating the file, matching the validation already performed by the view_text_file tool, and add WriteFileToolTest covering invalid ranges and the valid path.
5368126 to
4c102dd
Compare
Address review feedback on agentscope-ai#3208: - State in the ranges parameter description that line numbers are 1-based and inclusive and that negative indices are not supported for writes, so write_text_file no longer advertises a different range grammar than view_text_file. - Preserve the original file's trailing line terminator on range replacement; String.join("\n", ...) previously dropped it, which shifted line counts on subsequent reads/edits. - Add tests for end-beyond-EOF clamping and for a file without a trailing newline, and strengthen the new-file test assertions.
|
Thanks for the thorough review, all four points are addressed in the latest push: [Warning] ranges grammar (WriteFileTool.java) — took the documentation option: the [Warning] trailing newline dropped — good catch, fixed. The range-replace path now restores the original file's terminator after [Info] end beyond EOF — confirmed the clamp to EOF is intended and pinned it with [Info] weak new-file assertions — strengthened to
On the CLA note: that comment was against the old commit |
Address review feedback on agentscope-ai#3208: - State in the ranges parameter description that line numbers are 1-based and inclusive and that negative indices are not supported for writes, so write_text_file no longer advertises a different range grammar than view_text_file. - Preserve the original file's trailing line terminator on range replacement; String.join("\n", ...) previously dropped it, which shifted line counts on subsequent reads/edits. - Add tests for end-beyond-EOF clamping and for a file without a trailing newline, and strengthen the new-file test assertions.
1035152 to
7be1204
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed the new head 7be12043. All four items from the previous pass are genuinely addressed: the ranges description now states 1-based/inclusive and that negatives are rejected for writes (and explicitly contrasts with view_text_file), the range-replace path restores the original file's trailing terminator, the 3,999 EOF clamp is pinned by clampsEndBeyondFileLengthToEndOfFile, and the new-file assertions are now assertEquals(RUNNING, …) + assertTrue(Files.exists(…)). The two guards still sit before the first mutation, so the "file untouched on rejection" assertions remain meaningful. CLA is green on this head and Check License / Check Module Sync / build (ubuntu-latest) / build (windows-latest) / codecov/patch all pass.
Approved with two non-blocking follow-up notes below — neither affects the corruption fix this PR is about.
Findings
- [Warning]
WriteFileTool.java:384— the trailing-newline check re-reads the whole file just to derive one boolean that is already implied by the bytes read at:310 - [Info]
WriteFileTool.java:383— terminator is preserved but separator style still is not (readAllLines+String.join("\n", …)rewrites CRLF files to LF); pre-existing, worth an explicit decision in a follow-up
Suggestions
For the re-read, deriving both values from a single snapshot is cheaper and keeps the check consistent with the line list:
String originalText = Files.readString(path, StandardCharsets.UTF_8);
boolean endsWithNewline = originalText.endsWith("\n");
List<String> originalLines = originalText.lines().toList();Happy to see this merged as-is; the follow-up is a perf/normalisation nicety, not a blocker.
Automated review by github-manager-bot
| // Write the new content, preserving the original file's | ||
| // trailing line terminator so line counts stay stable. | ||
| String joinedContent = String.join("\n", newContent); | ||
| if (Files.readString(path, StandardCharsets.UTF_8).endsWith("\n")) { |
There was a problem hiding this comment.
[Warning] Non-blocking, but this re-reads the whole file to learn one boolean. The same bytes were already read into originalLines a few lines above (Files.readAllLines(path, UTF_8) at :310), so a large file is now buffered twice per range-replace, and the readString result is discarded right after. Cheap fix: compute the flag from the text you already have, or read once and derive both, e.g.
String originalText = Files.readString(path, StandardCharsets.UTF_8);
boolean endsWithNewline = originalText.endsWith("\n");
List<String> originalLines = originalText.lines().toList();That also keeps the terminator check and the line list guaranteed to come from the same snapshot.
| String joinedContent = String.join("\n", newContent); | ||
| if (Files.readString(path, StandardCharsets.UTF_8).endsWith("\n")) { | ||
| joinedContent += "\n"; | ||
| } |
There was a problem hiding this comment.
[Info] Terminator is now preserved, but the separator style still is not: originalLines comes from Files.readAllLines (which strips \r on CRLF files) and the rewrite joins with "\n", so a CRLF file has every line ending rewritten while this new check appends a bare "\n". That is pre-existing behaviour, not a regression from this PR — flagging only so a follow-up can decide explicitly (preserve original separator, or normalise to LF and say so in the tool description). No action needed to land this fix.
|
Thanks @oss-maintainer for the re-review and the approval! Both follow-up notes make sense, and I agree they belong outside this corruption fix:
No further changes pushed to this PR — leaving the approval intact. Happy for it to merge as-is. |
gmjneko
left a comment
gmjneko
left a comment
There was a problem hiding this comment.
The range validation LGTM, but the new trailing-newline handling introduces a regression when the replacement reaches EOF and already ends with a newline. Please address the issue described in the inline comment and add regression tests before merging.
| // trailing line terminator so line counts stay stable. | ||
| String joinedContent = String.join("\n", newContent); | ||
| if (Files.readString(path, StandardCharsets.UTF_8).endsWith("\n")) { | ||
| joinedContent += "\n"; |
There was a problem hiding this comment.
Avoid appending a duplicate trailing newline
When the replacement range reaches EOF and content already ends with a newline, this block appends another newline if the original file also had one. For example, replacing line 2 of "one\ntwo\n" with "NEW\n" produces "one\nNEW\n\n", adding an unintended blank line while reporting success. Even replacing the entire file with identical content changes the file.
Please avoid appending a duplicate newline when the replacement already supplies one, and add regression tests for both replacing the last line and replacing the entire file with identical content.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Range validation (start < 1, start > end, clamp beyond EOF) and the new WriteFileToolTest coverage look solid. Re-reviewing to respond to the open inline feedback from @gmjneko: the duplicate-trailing-newline report is valid against the current head, and the same root cause (content added verbatim as a single element) also affects mid-file replacements. Recommending a small strip-at-source fix plus one regression test before this is merge-ready; the re-read at the append site is a minor efficiency nit.
Automated review by github-manager-bot
| // trailing line terminator so line counts stay stable. | ||
| String joinedContent = String.join("\n", newContent); | ||
| if (Files.readString(path, StandardCharsets.UTF_8).endsWith("\n")) { | ||
| joinedContent += "\n"; |
There was a problem hiding this comment.
[Warning] Confirms @gmjneko's point, and it is broader than the EOF case: content is added verbatim as one list element, so any trailing \n in it survives the join. File "one\ntwo\nthree\n" + write_text_file("NEW\n", "2,2") produces "one\nNEW\n\nthree\n" — an unintended blank line reported as success. Suggested fix: strip exactly one trailing \n from content before newContent.add(...), letting the EOF-preserving block below be the sole authority on the final terminator, and add a regression test for newline-suffixed content.
| // Write the new content, preserving the original file's | ||
| // trailing line terminator so line counts stay stable. | ||
| String joinedContent = String.join("\n", newContent); | ||
| if (Files.readString(path, StandardCharsets.UTF_8).endsWith("\n")) { |
There was a problem hiding this comment.
[Info] The trailing-terminator flag requires one full pass over the file; compute it from a Files.readString(path, UTF_8).endsWith("\n") read taken before the edit (right where originalLines is loaded) and reuse the flag here, instead of a second Files.readString on the same path. Also note multi-line content ending in \n reaches endsWith("\n")==true and silently skips the append, so mid-range replacements of a whole trailing region can still drop the file's final newline — the strip-at-source fix above removes this ambiguity.
4 participants
AgentScope-Java Version
2.0.4-SNAPSHOT
Description
Background
The
write_text_filetool accepted invalidrangesvalues. After parsing[start, end]it only checkedstart <= fileLength; it did not checkstart >= 1orstart <= end, and it did not handle negative indexes (even though the siblingview_text_filetool documents and supports-100,-1style ranges).Because the
rangesargument is generated by the model during autonomous file editing, malformed values occur in practice:[5,2]on a 5-line file silently duplicated lines (result:one two three four NEW three four five) and still reported success.[0,2]silently deleted the first lines.[-3,-1]threw anIndexOutOfBoundsExceptionfromsubList(-1, 5)with an unhelpful message.This is silent data corruption in a destructive tool, which makes the agent keep working on a damaged file.
Changes
start >= 1andstart <= endbefore any file mutation; return a clear error result and leave the file untouched otherwise, matching the validation already performed byview_text_file.WriteFileToolTestcovering valid replacement, reversed range, zero-based start, negative range, start beyond file length, and new-file creation. The class previously had no unit tests.How to test
mvn -pl agentscope-core test -Dtest='io.agentscope.core.tool.file.*Test'— 11 tests pass (6 new + 5 existing).Checklist
mvn spotless:applymvn test)