From e3b2f846aa39343916bb7956692af0eaeaf2400c Mon Sep 17 00:00:00 2001 From: Mohamed Ashraf Date: Wed, 1 Apr 2026 16:16:05 +0000 Subject: [PATCH 1/3] 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. From acb61e5e0da36f4143e8bd30c30a61d43275dc7f Mon Sep 17 00:00:00 2001 From: Mohamed Ashraf Date: Tue, 28 Apr 2026 15:21:37 +0000 Subject: [PATCH 2/3] test: annotate test_replacement.py for mypy prek hook Add -> None return annotations and Path / JavaSupport parameter annotations to every test method + fixture so the prek mypy hook passes when the file is in the CI diff. --- .../test_java/test_replacement.py | 80 +++++++++---------- 1 file changed, 38 insertions(+), 42 deletions(-) diff --git a/tests/test_languages/test_java/test_replacement.py b/tests/test_languages/test_java/test_replacement.py index b3baabc8f..d71f66789 100644 --- a/tests/test_languages/test_java/test_replacement.py +++ b/tests/test_languages/test_java/test_replacement.py @@ -15,14 +15,14 @@ @pytest.fixture -def java_support(): +def java_support() -> JavaSupport: return JavaSupport() class TestReplaceFunctionDefinitionsInModule: """Tests for replace_function_definitions_for_language with Java (basic cases).""" - def test_replace_simple_method(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_simple_method(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test replacing a simple method in a Java class.""" java_file = tmp_path / "Calculator.java" original_code = """public class Calculator { @@ -61,7 +61,7 @@ def test_replace_simple_method(self, tmp_path: Path, java_support: JavaSupport): """ assert new_code == expected - def test_replace_method_preserves_other_methods(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_method_preserves_other_methods(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test that replacing one method preserves other methods.""" java_file = tmp_path / "Calculator.java" original_code = """public class Calculator { @@ -124,7 +124,7 @@ def test_replace_method_preserves_other_methods(self, tmp_path: Path, java_suppo """ assert new_code == expected - def test_replace_method_with_javadoc(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_method_with_javadoc(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test replacing a method that has Javadoc comments.""" java_file = tmp_path / "MathUtils.java" original_code = """public class MathUtils { @@ -193,7 +193,7 @@ def test_replace_method_with_javadoc(self, tmp_path: Path, java_support: JavaSup """ assert new_code == expected - def test_no_change_when_code_identical(self, tmp_path: Path, java_support: JavaSupport): + def test_no_change_when_code_identical(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test that no change is made when optimized code is identical.""" java_file = tmp_path / "Identity.java" original_code = """public class Identity { @@ -230,7 +230,7 @@ def test_no_change_when_code_identical(self, tmp_path: Path, java_support: JavaS class TestReplaceFunctionDefinitionsForLanguage: """Tests for replace_function_definitions_for_language with Java.""" - def test_replace_static_method(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_static_method(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test replacing a static method.""" java_file = tmp_path / "Utils.java" original_code = """public class Utils { @@ -269,7 +269,7 @@ def test_replace_static_method(self, tmp_path: Path, java_support: JavaSupport): """ assert new_code == expected - def test_replace_method_with_annotations(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_method_with_annotations(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test replacing a method with annotations.""" java_file = tmp_path / "Service.java" original_code = """public class Service { @@ -311,7 +311,7 @@ def test_replace_method_with_annotations(self, tmp_path: Path, java_support: Jav """ assert new_code == expected - def test_replace_method_in_interface(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_method_in_interface(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test replacing a default method in an interface.""" java_file = tmp_path / "Processor.java" original_code = """public interface Processor { @@ -350,7 +350,7 @@ def test_replace_method_in_interface(self, tmp_path: Path, java_support: JavaSup """ assert new_code == expected - def test_replace_method_in_enum(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_method_in_enum(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test replacing a method in an enum.""" java_file = tmp_path / "Color.java" original_code = """public enum Color { @@ -395,7 +395,7 @@ def test_replace_method_in_enum(self, tmp_path: Path, java_support: JavaSupport) """ assert new_code == expected - def test_replace_generic_method(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_generic_method(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test replacing a method with generics.""" java_file = tmp_path / "Container.java" original_code = """import java.util.List; @@ -453,7 +453,7 @@ def test_replace_generic_method(self, tmp_path: Path, java_support: JavaSupport) """ assert new_code == expected - def test_replace_method_with_throws(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_method_with_throws(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test replacing a method with throws clause.""" java_file = tmp_path / "FileReader.java" original_code = """import java.io.IOException; @@ -508,7 +508,7 @@ def test_replace_method_with_throws(self, tmp_path: Path, java_support: JavaSupp class TestRealWorldOptimizationScenarios: """Real-world optimization scenarios with complete valid Java code.""" - def test_optimize_string_concatenation(self, tmp_path: Path, java_support: JavaSupport): + def test_optimize_string_concatenation(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test optimizing string concatenation to StringBuilder.""" java_file = tmp_path / "StringJoiner.java" original_code = """public class StringJoiner { @@ -559,7 +559,7 @@ def test_optimize_string_concatenation(self, tmp_path: Path, java_support: JavaS """ assert new_code == expected - def test_optimize_list_iteration(self, tmp_path: Path, java_support: JavaSupport): + def test_optimize_list_iteration(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test optimizing list iteration with streams.""" java_file = tmp_path / "ListProcessor.java" original_code = """import java.util.List; @@ -608,7 +608,7 @@ def test_optimize_list_iteration(self, tmp_path: Path, java_support: JavaSupport """ assert new_code == expected - def test_optimize_null_checks(self, tmp_path: Path, java_support: JavaSupport): + def test_optimize_null_checks(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test optimizing null checks with Objects utility.""" java_file = tmp_path / "NullChecker.java" original_code = """public class NullChecker { @@ -655,7 +655,7 @@ def test_optimize_null_checks(self, tmp_path: Path, java_support: JavaSupport): """ assert new_code == expected - def test_optimize_collection_creation(self, tmp_path: Path, java_support: JavaSupport): + def test_optimize_collection_creation(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test optimizing collection creation with factory methods.""" java_file = tmp_path / "CollectionFactory.java" original_code = """import java.util.ArrayList; @@ -711,7 +711,7 @@ def test_optimize_collection_creation(self, tmp_path: Path, java_support: JavaSu class TestMultipleClassesAndMethods: """Tests for files with multiple classes or multiple methods being optimized.""" - def test_replace_method_in_first_class(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_method_in_first_class(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test replacing a method in the first class when multiple classes exist.""" java_file = tmp_path / "MultiClass.java" original_code = """public class Calculator { @@ -768,7 +768,7 @@ class Helper { """ assert new_code == expected - def test_replace_multiple_methods(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_multiple_methods(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test replacing multiple methods in the same class.""" java_file = tmp_path / "MathOps.java" original_code = """public class MathOps { @@ -835,7 +835,7 @@ def test_replace_multiple_methods(self, tmp_path: Path, java_support: JavaSuppor class TestNestedClasses: """Tests for nested class scenarios.""" - def test_replace_method_in_nested_class(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_method_in_nested_class(self, tmp_path: Path, java_support: JavaSupport) -> None: """Nested class methods are skipped by discovery (PR #1726), so replacement returns False.""" java_file = tmp_path / "Outer.java" original_code = """public class Outer { @@ -882,7 +882,7 @@ def test_replace_method_in_nested_class(self, tmp_path: Path, java_support: Java class TestPreservesStructure: """Tests that verify code structure is preserved during replacement.""" - def test_preserves_fields_and_constructors(self, tmp_path: Path, java_support: JavaSupport): + def test_preserves_fields_and_constructors(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test that fields and constructors are preserved.""" java_file = tmp_path / "Counter.java" original_code = """public class Counter { @@ -952,7 +952,7 @@ def test_preserves_fields_and_constructors(self, tmp_path: Path, java_support: J class TestEdgeCases: """Edge cases and error handling tests.""" - def test_empty_optimized_code_returns_false(self, tmp_path: Path, java_support: JavaSupport): + def test_empty_optimized_code_returns_false(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test that empty optimized code returns False.""" java_file = tmp_path / "Empty.java" original_code = """public class Empty { @@ -980,7 +980,7 @@ def test_empty_optimized_code_returns_false(self, tmp_path: Path, java_support: new_code = java_file.read_text(encoding="utf-8") assert new_code == original_code - def test_function_not_found_returns_false(self, tmp_path: Path, java_support: JavaSupport): + def test_function_not_found_returns_false(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test that function not found returns False.""" java_file = tmp_path / "NotFound.java" original_code = """public class NotFound { @@ -1011,7 +1011,7 @@ def test_function_not_found_returns_false(self, tmp_path: Path, java_support: Ja assert result is False - def test_unicode_in_code(self, tmp_path: Path, java_support: JavaSupport): + def test_unicode_in_code(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test handling of unicode characters in code.""" java_file = tmp_path / "Unicode.java" original_code = """public class Unicode { @@ -1054,7 +1054,7 @@ def test_unicode_in_code(self, tmp_path: Path, java_support: JavaSupport): class TestOptimizationWithStaticFields: """Tests for optimizations that add new static fields to the class.""" - def test_add_static_lookup_table(self, tmp_path: Path, java_support: JavaSupport): + def test_add_static_lookup_table(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test optimization that adds a static lookup table.""" java_file = tmp_path / "Buffer.java" original_code = """public class Buffer { @@ -1114,7 +1114,7 @@ def test_add_static_lookup_table(self, tmp_path: Path, java_support: JavaSupport """ assert new_code == expected - def test_add_precomputed_array(self, tmp_path: Path, java_support: JavaSupport): + def test_add_precomputed_array(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test optimization that adds a precomputed static array.""" java_file = tmp_path / "Encoder.java" original_code = """public class Encoder { @@ -1174,7 +1174,7 @@ def test_add_precomputed_array(self, tmp_path: Path, java_support: JavaSupport): """ assert new_code == expected - def test_preserve_existing_fields(self, tmp_path: Path, java_support: JavaSupport): + def test_preserve_existing_fields(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test that existing fields are preserved when adding new ones.""" java_file = tmp_path / "Calculator.java" original_code = """public class Calculator { @@ -1260,7 +1260,7 @@ def test_preserve_existing_fields(self, tmp_path: Path, java_support: JavaSuppor class TestOptimizationWithHelperMethods: """Tests for optimizations that add new helper methods.""" - def test_add_private_helper_method(self, tmp_path: Path, java_support: JavaSupport): + def test_add_private_helper_method(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test optimization that adds a private helper method.""" java_file = tmp_path / "StringUtils.java" original_code = """public class StringUtils { @@ -1330,7 +1330,7 @@ def test_add_private_helper_method(self, tmp_path: Path, java_support: JavaSuppo """ assert new_code == expected - def test_add_multiple_helpers(self, tmp_path: Path, java_support: JavaSupport): + def test_add_multiple_helpers(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test optimization that adds multiple helper methods.""" java_file = tmp_path / "MathUtils.java" original_code = """public class MathUtils { @@ -1395,7 +1395,7 @@ def test_add_multiple_helpers(self, tmp_path: Path, java_support: JavaSupport): class TestOptimizationWithFieldsAndHelpers: """Tests for optimizations that add both static fields and helper methods.""" - def test_add_field_and_helper_together(self, tmp_path: Path, java_support: JavaSupport): + def test_add_field_and_helper_together(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test optimization that adds both a static field and helper method.""" java_file = tmp_path / "Fibonacci.java" original_code = """public class Fibonacci { @@ -1464,7 +1464,7 @@ def test_add_field_and_helper_together(self, tmp_path: Path, java_support: JavaS """ assert new_code == expected - def test_real_world_bytes_to_hex_optimization(self, tmp_path: Path, java_support: JavaSupport): + def test_real_world_bytes_to_hex_optimization(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test the actual bytesToHexString optimization pattern from aerospike.""" java_file = tmp_path / "Buffer.java" original_code = """package com.example; @@ -1561,7 +1561,7 @@ def test_real_world_bytes_to_hex_optimization(self, tmp_path: Path, java_support class TestOverloadedMethods: """Tests for handling overloaded methods (same name, different signatures).""" - def test_replace_specific_overload_by_line_number(self, tmp_path: Path, java_support: JavaSupport): + def test_replace_specific_overload_by_line_number(self, tmp_path: Path, java_support: JavaSupport) -> None: """Test replacing a specific overload when multiple exist.""" java_file = tmp_path / "Buffer.java" original_code = """public final class Buffer { @@ -1615,7 +1615,6 @@ def test_replace_specific_overload_by_line_number(self, tmp_path: Path, java_sup starting_line=13, # Line where 3-arg version starts (1-indexed) ending_line=18, parents=[FunctionParent(name="Buffer", type="ClassDef")], - qualified_name="Buffer.bytesToHexString", is_method=True, ) @@ -1670,7 +1669,7 @@ class TestWrongMethodNameGeneration: source file unchanged. """ - def test_standalone_wrong_method_name_leaves_source_unchanged(self, tmp_path, java_support): + def test_standalone_wrong_method_name_leaves_source_unchanged(self, tmp_path: Path, java_support: JavaSupport) -> None: """Standalone generated method with wrong name must not replace the target. Reproduces the Unpacker.unpackObjectMap bug: the LLM was asked to optimise @@ -1710,7 +1709,6 @@ def test_standalone_wrong_method_name_leaves_source_unchanged(self, tmp_path, ja starting_line=2, ending_line=4, parents=[FunctionParent(name="Unpacker", type="ClassDef")], - qualified_name="Unpacker.unpackObjectMap", is_method=True, ) @@ -1726,7 +1724,7 @@ def test_standalone_wrong_method_name_leaves_source_unchanged(self, tmp_path, ja assert result is False assert java_file.read_text(encoding="utf-8") == original_code - def test_class_wrapper_with_wrong_target_method_leaves_source_unchanged(self, tmp_path, java_support): + def test_class_wrapper_with_wrong_target_method_leaves_source_unchanged(self, tmp_path: Path, java_support: JavaSupport) -> None: """Class-wrapped generated code missing the target method must not modify source. Reproduces the Command.estimateKeySize bug: the LLM generated a class that @@ -1767,7 +1765,6 @@ def test_class_wrapper_with_wrong_target_method_leaves_source_unchanged(self, tm starting_line=2, ending_line=4, parents=[FunctionParent(name="Command", type="ClassDef")], - qualified_name="Command.estimateKeySize", is_method=True, ) @@ -1791,7 +1788,7 @@ class TestUnchangedTargetWithClassModifications: 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): + def test_unchanged_target_with_unused_field_rejected(self, tmp_path: Path, java_support: JavaSupport) -> None: """LLM adds a field but leaves the target method unchanged — should reject.""" java_file = tmp_path / "SessionContext.java" original_code = """\ @@ -1829,7 +1826,7 @@ def test_unchanged_target_with_unused_field_rejected(self, tmp_path, java_suppor 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): + def test_unchanged_target_with_modified_constructor_rejected(self, tmp_path: Path, java_support: JavaSupport) -> None: """LLM modifies constructor but leaves target method unchanged — should reject. Reproduces Zuul #52: Header.getValue — title says getValue but diff changes constructor. @@ -1881,7 +1878,7 @@ def test_unchanged_target_with_modified_constructor_rejected(self, tmp_path, jav 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): + def test_unchanged_target_with_helper_rejected(self, tmp_path: Path, java_support: JavaSupport) -> None: """LLM adds a helper method but leaves target method unchanged — should reject.""" java_file = tmp_path / "FilterRegistry.java" original_code = """\ @@ -1922,7 +1919,7 @@ def test_unchanged_target_with_helper_rejected(self, tmp_path, 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): + def test_changed_target_with_field_accepted(self, tmp_path: Path, java_support: JavaSupport) -> None: """LLM changes target method AND adds a field — should accept (valid optimization).""" java_file = tmp_path / "Fibonacci.java" original_code = """\ @@ -1960,7 +1957,7 @@ def test_changed_target_with_field_accepted(self, tmp_path, java_support): assert result is True - def test_changed_target_only_accepted(self, tmp_path, java_support): + def test_changed_target_only_accepted(self, tmp_path: Path, java_support: JavaSupport) -> None: """LLM changes only the target method (no class modifications) — should accept.""" java_file = tmp_path / "Calculator.java" original_code = """\ @@ -2003,7 +2000,7 @@ class TestAnonymousInnerClassMethods: enclosing method scope. """ - def test_anonymous_iterator_methods_not_hoisted_to_class(self, tmp_path, java_support): + def test_anonymous_iterator_methods_not_hoisted_to_class(self, tmp_path: Path, java_support: JavaSupport) -> None: """Reproduces the LuaMap.keySetIterator bug. The LLM optimised ``keySetIterator`` by returning an anonymous @@ -2084,7 +2081,6 @@ def test_anonymous_iterator_methods_not_hoisted_to_class(self, tmp_path, java_su starting_line=11, ending_line=13, parents=[FunctionParent(name="LuaMap", type="ClassDef")], - qualified_name="LuaMap.keySetIterator", is_method=True, ) From 9dd52d187fa2d8dcfffc0dadc814e9bb8e395148 Mon Sep 17 00:00:00 2001 From: Mohamed Ashraf Date: Tue, 28 Apr 2026 16:39:00 +0000 Subject: [PATCH 3/3] fix: decode help-banner test subprocess output as UTF-8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rich renders the banner panel with box-drawing characters (╭, ╮, │, etc.) that cp1252 cannot decode. On Windows, subprocess.run(..., text=True) uses cp1252 by default, so decoding the child stdout raises UnicodeDecodeError and subprocess sets result.stdout to None — breaking the assertion with a misleading "argument of type 'NoneType' is not iterable". Pass encoding="utf-8" explicitly so the test passes on every platform. --- tests/test_help_banner.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_help_banner.py b/tests/test_help_banner.py index c5d801b23..27d749790 100644 --- a/tests/test_help_banner.py +++ b/tests/test_help_banner.py @@ -4,7 +4,10 @@ def test_help_displays_logo() -> None: result = subprocess.run( - [sys.executable, "-c", "from codeflash.main import main; main()", "--help"], capture_output=True, text=True + [sys.executable, "-c", "from codeflash.main import main; main()", "--help"], + capture_output=True, + text=True, + encoding="utf-8", ) assert result.returncode == 0 assert "codeflash.ai" in result.stdout @@ -12,7 +15,10 @@ def test_help_displays_logo() -> None: def test_help_short_flag_displays_logo() -> None: result = subprocess.run( - [sys.executable, "-c", "from codeflash.main import main; main()", "-h"], capture_output=True, text=True + [sys.executable, "-c", "from codeflash.main import main; main()", "-h"], + capture_output=True, + text=True, + encoding="utf-8", ) assert result.returncode == 0 assert "codeflash.ai" in result.stdout