diff --git a/codeflash/languages/java/replacement.py b/codeflash/languages/java/replacement.py index 5ed9bf8f1..2a37b095d 100644 --- a/codeflash/languages/java/replacement.py +++ b/codeflash/languages/java/replacement.py @@ -365,6 +365,39 @@ def _replace_constructors( return result +def _normalized_equal(a: str, b: str) -> bool: + """Compare two method sources ignoring whitespace differences.""" + + def normalize(s: str) -> str: + return "\n".join(line.strip() for line in s.strip().splitlines() if line.strip()) + + return normalize(a) == normalize(b) + + +def _extract_original_method_source( + source: str, func_name: str, function: FunctionToOptimize, analyzer: JavaAnalyzer +) -> str | None: + """Extract the original method source from the file for comparison.""" + methods = analyzer.find_methods(source) + matching = [ + m + for m in methods + if m.name == func_name and (function.class_name is None or m.class_name == function.class_name) + ] + if not matching: + return None + target = matching[0] + if len(matching) > 1 and function.starting_line and function.ending_line: + for m in matching: + if abs(m.start_line - function.starting_line) <= 5: + target = m + break + start = (target.javadoc_start_line or target.start_line) - 1 + end = target.end_line + lines = source.splitlines(keepends=True) + return "".join(lines[start:end]) + + def replace_function( source: str, function: FunctionToOptimize, new_source: str, analyzer: JavaAnalyzer | None = None ) -> str: @@ -405,6 +438,26 @@ def replace_function( logger.warning("No valid replacement found for method '%s'. Returning original source.", func_name) return source + # Guard: reject optimizations that don't actually change the target method but modify surrounding class members. + # This catches the "wrong-file" pattern where the LLM adds cache fields, modifies constructors, or adds helpers + # without changing the method it was asked to optimize (15+ known PRs exhibit this pattern). + has_class_modifications = bool( + parsed.new_fields or parsed.helpers_before_target or parsed.helpers_after_target or parsed.modified_constructors + ) + if has_class_modifications: + original_target = _extract_original_method_source(source, func_name, function, analyzer) + if original_target is not None and _normalized_equal(parsed.target_method_source, original_target): + logger.warning( + "Rejecting optimization for '%s': target method is unchanged but LLM modified surrounding class members " + "(fields=%d, helpers_before=%d, helpers_after=%d, constructors=%d). This is a no-op optimization.", + func_name, + len(parsed.new_fields), + len(parsed.helpers_before_target), + len(parsed.helpers_after_target), + len(parsed.modified_constructors), + ) + return source + # Find the method in the original source methods = analyzer.find_methods(source) target_method = None diff --git a/tests/test_languages/test_java/test_replacement.py b/tests/test_languages/test_java/test_replacement.py index 1bd4f7abb..b3baabc8f 100644 --- a/tests/test_languages/test_java/test_replacement.py +++ b/tests/test_languages/test_java/test_replacement.py @@ -1784,6 +1784,214 @@ def test_class_wrapper_with_wrong_target_method_leaves_source_unchanged(self, tm assert java_file.read_text(encoding="utf-8") == original_code +class TestUnchangedTargetWithClassModifications: + """Tests that optimizations modifying class members without changing the target method are rejected. + + This guards against the "wrong-file" pattern where the LLM adds cache fields, + modifies constructors, or adds helpers without changing the method it was asked to optimize. + """ + + def test_unchanged_target_with_unused_field_rejected(self, tmp_path, java_support): + """LLM adds a field but leaves the target method unchanged — should reject.""" + java_file = tmp_path / "SessionContext.java" + original_code = """\ +public class SessionContext { + private String routeVIP; + + public String getRouteVIP() { + return routeVIP; + } +} +""" + java_file.write_text(original_code, encoding="utf-8") + + # LLM adds a cached field but doesn't change getRouteVIP + optimized_markdown = f"""```java:{java_file.relative_to(tmp_path)} +public class SessionContext {{ + private String routeVIP; + private static final String DEFAULT_VIP = ""; + + public String getRouteVIP() {{ + return routeVIP; + }} +}} +```""" + + optimized_code = CodeStringsMarkdown.parse_markdown_code(optimized_markdown, expected_language="java") + result = replace_function_definitions_for_language( + function_names=["getRouteVIP"], + optimized_code=optimized_code, + module_abspath=java_file, + project_root_path=tmp_path, + lang_support=java_support, + ) + + assert result is False + assert java_file.read_text(encoding="utf-8") == original_code + + def test_unchanged_target_with_modified_constructor_rejected(self, tmp_path, java_support): + """LLM modifies constructor but leaves target method unchanged — should reject. + + Reproduces Zuul #52: Header.getValue — title says getValue but diff changes constructor. + """ + java_file = tmp_path / "Header.java" + original_code = """\ +public class Header { + private final String name; + private final String value; + + public Header(String name, String value) { + this.name = name; + this.value = value; + } + + public String getValue() { + return value; + } +} +""" + java_file.write_text(original_code, encoding="utf-8") + + # LLM modifies constructor to cache but doesn't change getValue + optimized_markdown = f"""```java:{java_file.relative_to(tmp_path)} +public class Header {{ + private final String name; + private final String value; + + public Header(String name, String value) {{ + this.name = name; + this.value = value != null ? value.intern() : null; + }} + + public String getValue() {{ + return value; + }} +}} +```""" + + optimized_code = CodeStringsMarkdown.parse_markdown_code(optimized_markdown, expected_language="java") + result = replace_function_definitions_for_language( + function_names=["getValue"], + optimized_code=optimized_code, + module_abspath=java_file, + project_root_path=tmp_path, + lang_support=java_support, + ) + + assert result is False + assert java_file.read_text(encoding="utf-8") == original_code + + def test_unchanged_target_with_helper_rejected(self, tmp_path, java_support): + """LLM adds a helper method but leaves target method unchanged — should reject.""" + java_file = tmp_path / "FilterRegistry.java" + original_code = """\ +public class FilterRegistry { + private final java.util.Map filters = new java.util.HashMap<>(); + + public int size() { + return filters.size(); + } +} +""" + java_file.write_text(original_code, encoding="utf-8") + + # LLM adds a helper but doesn't change size() + optimized_markdown = f"""```java:{java_file.relative_to(tmp_path)} +public class FilterRegistry {{ + private final java.util.Map filters = new java.util.HashMap<>(); + + private int cachedSize() {{ + return filters.size(); + }} + + public int size() {{ + return filters.size(); + }} +}} +```""" + + optimized_code = CodeStringsMarkdown.parse_markdown_code(optimized_markdown, expected_language="java") + result = replace_function_definitions_for_language( + function_names=["size"], + optimized_code=optimized_code, + module_abspath=java_file, + project_root_path=tmp_path, + lang_support=java_support, + ) + + assert result is False + assert java_file.read_text(encoding="utf-8") == original_code + + def test_changed_target_with_field_accepted(self, tmp_path, java_support): + """LLM changes target method AND adds a field — should accept (valid optimization).""" + java_file = tmp_path / "Fibonacci.java" + original_code = """\ +public class Fibonacci { + public static long fib(int n) { + if (n <= 1) return n; + return fib(n - 1) + fib(n - 2); + } +} +""" + java_file.write_text(original_code, encoding="utf-8") + + optimized_markdown = f"""```java:{java_file.relative_to(tmp_path)} +public class Fibonacci {{ + private static final long[] CACHE = new long[100]; + + public static long fib(int n) {{ + if (n <= 1) return n; + if (n < 100 && CACHE[n] != 0) return CACHE[n]; + long result = fib(n - 1) + fib(n - 2); + if (n < 100) CACHE[n] = result; + return result; + }} +}} +```""" + + optimized_code = CodeStringsMarkdown.parse_markdown_code(optimized_markdown, expected_language="java") + result = replace_function_definitions_for_language( + function_names=["fib"], + optimized_code=optimized_code, + module_abspath=java_file, + project_root_path=tmp_path, + lang_support=java_support, + ) + + assert result is True + + def test_changed_target_only_accepted(self, tmp_path, java_support): + """LLM changes only the target method (no class modifications) — should accept.""" + java_file = tmp_path / "Calculator.java" + original_code = """\ +public class Calculator { + public int add(int a, int b) { + return a + b; + } +} +""" + java_file.write_text(original_code, encoding="utf-8") + + optimized_markdown = f"""```java:{java_file.relative_to(tmp_path)} +public class Calculator {{ + public int add(int a, int b) {{ + return Math.addExact(a, b); + }} +}} +```""" + + optimized_code = CodeStringsMarkdown.parse_markdown_code(optimized_markdown, expected_language="java") + result = replace_function_definitions_for_language( + function_names=["add"], + optimized_code=optimized_code, + module_abspath=java_file, + project_root_path=tmp_path, + lang_support=java_support, + ) + + assert result is True + + class TestAnonymousInnerClassMethods: """Tests that methods inside anonymous inner classes are not hoisted as helpers.