From e3b2f846aa39343916bb7956692af0eaeaf2400c Mon Sep 17 00:00:00 2001 From: Mohamed Ashraf Date: Wed, 1 Apr 2026 16:16:05 +0000 Subject: [PATCH] fix: reject optimizations that modify class members without changing target method Guard against the "wrong-file" pattern where the LLM adds cache fields, modifies constructors, or adds helper methods without actually changing the method it was asked to optimize. 15+ known PRs across Zuul and Eureka exhibited this pattern. The check compares the target method source (normalized for whitespace) before and after the optimization. If the target is unchanged but the LLM added fields, helpers, or modified constructors, the optimization is rejected as a no-op. Co-Authored-By: Claude Opus 4.6 --- codeflash/languages/java/replacement.py | 53 +++++ .../test_java/test_replacement.py | 208 ++++++++++++++++++ 2 files changed, 261 insertions(+) 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.