fix(patch): reject wrong-region matches in the fuzzy edit fallback - #54575
fix(patch): reject wrong-region matches in the fuzzy edit fallback#54575MaxFreedomPollard wants to merge 2 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused guard. The premise is confirmed on current main: block_anchor accepts shared-boundary candidates at the 0.50 middle-similarity threshold (tools/fuzzy_match.py:665-691), while context_aware accepts a block when only half its lines are highly similar (tools/fuzzy_match.py:716-726). fuzzy_find_and_replace then applies the selected spans (tools/fuzzy_match.py:85-153).
Problems
- The new coverage is limited to
fuzzy_find_and_replaceintests/tools/test_fuzzy_match.py. The externally visible mutation path isShellFileOperations.patch_replace(tools/file_operations.py:1590-1616), so the no-write guarantee is not exercised through real file I/O.
Suggested changes
- Add a live
patch_replaceregression intests/tools/test_file_tools_live.pyusing the wrong-region fixture and assert both an error/no replacement and unchanged on-disk contents. The neighboring live tests establish this convention attests/tools/test_file_tools_live.py:281-301.
Automated hermes-sweeper review.
| "def handler(request):\n" | ||
| " rate_limit(request)\n" | ||
| " return process(request)" | ||
| ) |
There was a problem hiding this comment.
Please also cover this fixture through ShellFileOperations.patch_replace and assert the file remains byte-identical. This is the user-visible mutation path, whereas this test only verifies the pure matcher.
febd4b3 to
3f0c8be
Compare
|
Wrote a live test for the no-write guarantee:
Commit: Likivik@7610ac4ca Disclosure: I wrote this test (and this comment) with help from an LLM. Reasoning is mine; wording has been polished. Cherry-pick 7610ac4, or merge Likivik:fix/fuzzy-match-wrong-region — whatever's easier. Thanks. |
The patch tool's replace mode falls through to block_anchor and context_aware when the seven formatting-tolerant strategies fail. Those two accept a region on a shared first/last line or on half its lines, so they can overwrite a region whose content differs from old_string and still report a successful single match, with no signal that an unrelated line was destroyed. Add a content-similarity guard (unicode + whitespace normalized, floor 0.90) applied to those two strategies, after the existing escape-drift check. A region that only shares an anchor line falls below the floor and is discarded, so the chain falls through to the no-match path and the caller re-reads instead of editing the wrong place. The seven formatting strategies are untouched and a genuine single-line drift still applies.
Adds a real-file-I/O test to tests/tools/test_file_tools_live.py that exercises the patch_replace code path with a wrong-region pattern (blocks anchored on first/last lines only). Before PR NousResearch#54575, the block_anchor / context_aware strategies could silently overwrite a different region and report success; the patch_replace post-write verification only confirmed the bytes landed, not that the right region was matched. This test: * asserts patch_replace returns a PatchResult with error set when old_string shares only the first/last lines with the file, * asserts the on-disk file is byte-identical to its pre-patch state (audit_log line preserved, validate / rate_limit lines absent), * includes a positive sanity test that a real edit (indentation drift + value change) still applies so the guard does not regress. Without the fix this test fails on main with the silent-replace diff ('audit_log(request.user_id)' -> 'rate_limit(request)'). Refs NousResearch#54572.
3f0c8be to
c6c85d8
Compare
|
@Likivik thank you, this is exactly the coverage the review asked for and it is a nicer test than the one I would have written. Cherry-picked as c6c85d8 with your authorship on the commit, and rebased the fix onto current main at aae5298. I checked your claim rather than taking it on trust, and it holds: with the fix reverted, That closes @teknium1's point directly. The gap was that the coverage stopped at The rebase conflict was a clean one, main added 81 tests pass across test_file_tools_live.py and test_fuzzy_match.py. Thanks also for the note about how you wrote it. Appreciated, and it did not change how I reviewed it, the test either reproduces the bug or it does not, and this one does. |
Fixes #54572
Problem
The
patchtool in replace mode locatesold_stringthrough the nine strategy chain intools/fuzzy_match.py. The last two strategies,block_anchorandcontext_aware, accept a region based on a shared first and last line (with a 0.50 middle similarity) or on half of its lines matching. They run only after the seven formatting-tolerant strategies have failed, which meansold_stringis genuinely not in the file at that point. When one of them fires, the tool replaces a region whose content differs fromold_stringand returns a successful single match.patch_replaceverifies that the bytes it wrote landed, but not that the matched region was the one described, so the wrong edit is not caught.A concrete case: a file whose middle line is
audit_log(request.user_id), edited with anold_stringthat namesvalidate(request.token)(a line not in the file) but shares the surrounding two lines.block_anchormatches, the audit_log line is replaced, and the call reports success.Fix
After a match is produced by one of the two content divergent strategies, drop any matched region that is not substantially the same text as
old_string, comparing with unicode form and whitespace runs normalized away (_normalized_similarity, floor 0.90). Legitimate reflow, indentation and smart quote drift normalize to about 1.0 and still match. A region that merely shares an anchor line falls below the floor, so the match is discarded and the chain falls through to the existing no match path, which already attaches a "did you mean" hint that prompts a re-read.The guard runs after the existing escape-drift check, so the more specific escape-drift error still wins where it applies.
This is intentionally narrow. The seven formatting-tolerant strategies are untouched, since they already guarantee the region equals
old_stringapart from formatting. A genuine near miss where a single line drifted stays well above the floor and still applies, so real edits are not regressed.Tests
tests/tools/test_fuzzy_match.py:block_anchorwrong-region case leaves the file unchanged and reports no match,context_awarehalf-matching case preserves the two unrelated lines,The full file suite passes (53 tests).