diff --git a/tests/tools/test_file_operations.py b/tests/tools/test_file_operations.py index 9e9ffa8ad33e..26792271076a 100644 --- a/tests/tools/test_file_operations.py +++ b/tests/tools/test_file_operations.py @@ -552,6 +552,38 @@ def side_effect(command, stdin_data=None, **kwargs): assert result.success is True assert state["content"] == "hi world\n", f"File not actually updated: {state['content']!r}" + def test_patch_replace_identical_old_new_reports_noop(self, mock_env): + """Identical old/new replacement should be a successful no-op, not a fuzzy-match failure.""" + state = {"content": "hello world\n"} + writes = [] + + def side_effect(command, stdin_data=None, **kwargs): + if command.startswith("cat >"): + writes.append(stdin_data) + state["content"] = stdin_data or "" + return {"output": "", "returncode": 0} + if command.startswith("cat "): + return {"output": state["content"], "returncode": 0} + if command.startswith("mkdir "): + return {"output": "", "returncode": 0} + if command.startswith("wc -c"): + return {"output": str(len(state["content"].encode())), "returncode": 0} + return {"output": "", "returncode": 0} + + mock_env.execute.side_effect = side_effect + ops = ShellFileOperations(mock_env) + result = ops.patch_replace("/tmp/test/a.py", "hello", "hello") + + assert result.success is True + assert result.noop is True + assert result.error is None + assert result.diff == "" + assert result.files_modified == [] + assert result.message is not None + assert "old_string and new_string are identical" in result.message + assert writes == [] + assert state["content"] == "hello world\n" + def test_patch_replace_fails_when_verify_read_errors(self, mock_env): """If the verify-read step itself fails (exit code != 0), return an error.""" call_count = {"cat": 0} diff --git a/tests/tools/test_patch_parser.py b/tests/tools/test_patch_parser.py index 8c4a0c80a375..1bc826d16afa 100644 --- a/tests/tools/test_patch_parser.py +++ b/tests/tools/test_patch_parser.py @@ -186,6 +186,42 @@ def write_file(self, path, content): ' return result + 1' ) + def test_pure_context_update_reports_noop_without_writing(self): + """A V4A update hunk with no effective change should be a clear no-op.""" + patch = """\ +*** Begin Patch +*** Update File: sample.py +@@ def run @@ + def run(): + return 1 +*** End Patch""" + operations, err = parse_v4a_patch(patch) + assert err is None + + class FakeFileOps: + written = False + + def read_file_raw(self, path): + return SimpleNamespace( + content="def run():\n return 1\n", + error=None, + ) + + def write_file(self, path, content): + self.written = True + return SimpleNamespace(error=None) + + file_ops = FakeFileOps() + result = apply_v4a_operations(operations, file_ops) + + assert result.success is True + assert result.noop is True + assert result.message is not None + assert "No files modified" in result.message + assert result.files_modified == [] + assert result.diff == "" + assert file_ops.written is False + class TestAdditionOnlyHunks: """Regression tests for #3081 — addition-only hunks were silently dropped.""" diff --git a/tools/file_operations.py b/tools/file_operations.py index 4b64421622fc..185e946545bb 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -146,9 +146,11 @@ class PatchResult: # See :class:`WriteResult.lsp_diagnostics`. lsp_diagnostics: Optional[str] = None error: Optional[str] = None + noop: bool = False + message: Optional[str] = None def to_dict(self) -> dict: - result = {"success": self.success} + result: Dict[str, Any] = {"success": self.success} if self.diff: result["diff"] = self.diff if self.files_modified: @@ -163,6 +165,10 @@ def to_dict(self) -> dict: result["lsp_diagnostics"] = self.lsp_diagnostics if self.error: result["error"] = self.error + if self.noop: + result["noop"] = True + if self.message: + result["message"] = self.message return result @@ -1014,6 +1020,17 @@ def patch_replace(self, path: str, old_string: str, new_string: str, return PatchResult(error=f"Failed to read file: {path}") content = read_result.stdout + + if old_string == new_string: + return PatchResult( + success=True, + noop=True, + message=( + "No files modified: old_string and new_string are identical. " + "If you expected a change, re-read the file and provide a " + "replacement that differs from the matched text." + ), + ) # Import and use fuzzy matching from tools.fuzzy_match import fuzzy_find_and_replace diff --git a/tools/patch_parser.py b/tools/patch_parser.py index dacc6e855c34..bda6d089f16e 100644 --- a/tools/patch_parser.py +++ b/tools/patch_parser.py @@ -285,6 +285,14 @@ def _validate_operations( replace_lines = [l.content for l in hunk.lines if l.prefix in {' ', '+'}] replacement = '\n'.join(replace_lines) + if search_pattern == replacement: + # Pure-context/no-op hunk. Treat as valid without calling + # fuzzy_find_and_replace(), which deliberately reports + # identical old/new strings as a no-op. This keeps V4A + # patch validation from surfacing harmless no-op hunks as + # alarming validation failures. + continue + new_simulated, count, _strategy, match_error = fuzzy_find_and_replace( simulated, search_pattern, replacement, replace_all=False ) @@ -394,8 +402,9 @@ def apply_v4a_operations(operations: List[PatchOperation], elif op.operation == OperationType.UPDATE: result = _apply_update(op, file_ops) if result[0]: - files_modified.append(op.file_path) - all_diffs.append(result[1]) + if result[1]: + files_modified.append(op.file_path) + all_diffs.append(result[1]) else: errors.append(f"Failed to update {op.file_path}: {result[1]}") @@ -423,6 +432,7 @@ def apply_v4a_operations(operations: List[PatchOperation], + "\n".join(f" • {e}" for e in errors), ) + changed = bool(files_modified or files_created or files_deleted) return PatchResult( success=True, diff=combined_diff, @@ -430,6 +440,11 @@ def apply_v4a_operations(operations: List[PatchOperation], files_created=files_created, files_deleted=files_deleted, lint=lint_results if lint_results else None, + noop=not changed, + message=( + "No files modified: patch contained no effective changes. " + "If you expected a change, re-read the file and regenerate the patch." + ) if not changed else None, ) @@ -519,6 +534,10 @@ def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]: search_pattern = '\n'.join(search_lines) replacement = '\n'.join(replace_lines) + if search_pattern == replacement: + # Pure-context/no-op hunk. Nothing to apply. + continue + new_content, count, _strategy, error = fuzzy_find_and_replace( new_content, search_pattern, replacement, replace_all=False ) @@ -575,6 +594,9 @@ def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str]: else: new_content = new_content.rstrip('\n') + '\n' + insert_text + '\n' + if new_content == current_content: + return True, "" + # Write new content write_result = file_ops.write_file(op.file_path, new_content) if write_result.error: