Skip to content

Disable build cache for Gradle/Maven and fix var-typed instrumentation#1970

Open
HeshamHM28 wants to merge 1 commit intomainfrom
cf-disable-maven-build-cache
Open

Disable build cache for Gradle/Maven and fix var-typed instrumentation#1970
HeshamHM28 wants to merge 1 commit intomainfrom
cf-disable-maven-build-cache

Conversation

@HeshamHM28
Copy link
Copy Markdown
Contributor

Summary

  • Adds --no-build-cache to all Gradle commands to prevent stale cached artifacts from affecting optimization runs
  • Adds -Dmaven.build.cache.enabled=false to Maven skip flags for the same reason
  • Fixes Java instrumentation to use var-typed capture instead of Object, preserving return types for method chaining (builder pattern support)
  • Updates instrumentation tests accordingly

Test plan

  • Run existing Java instrumentation tests
  • Verify Gradle builds no longer use cached artifacts
  • Verify Maven builds disable build cache
  • Test builder-pattern Java code instrumentation end-to-end

…trumentation capture

- Add --no-build-cache flag to all Gradle build commands to ensure fresh compilation
- Add -Dmaven.build.cache.enabled=false to Maven validation skip flags
- Fix Java instrumentation to use var-typed capture instead of Object for method chaining support
- Update instrumentation tests to reflect new var-based capture pattern

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 2, 2026

Claude finished @HeshamHM28's task in 4m 3s —— View job


PR Review Summary

  • Triage PR size — LARGE (64 additions, 21 deletions across 3 production files)
  • Run lint/typecheck checks
  • Resolve stale review threads — none found
  • Code review
  • Duplicate detection
  • Post summary

Prek Checks

✅ Ruff (lint + format): passed
✅ Mypy: no issues in changed files


Code Review

gradle_strategy.py / maven_strategy.py — Build Cache Disabling

Changes look correct. --build-cache--no-build-cache in all 6 Gradle command sites, and -Dmaven.build.cache.enabled=false added to the Maven skip flags. No issues.

instrumentation.py — var-typed capture

The approach is sound: using var inside the try block preserves the concrete type for method chaining, which Object loses.

Bug: _cf_obj is declared and assigned but never read

codeflash/languages/java/instrumentation.py:410-412, 426, 435

obj_var = f"_cf_obj{iter_id}_{call_counter}"          # declared
obj_assign = f"{obj_var} = {var_name};"                # assigned inside try

# pre_lines includes:
f"{line_indent_str}Object {obj_var} = null;",          # Java: Object _cf_obj1_1 = null;
...
f"{line_indent_str}    {obj_assign}",                  # Java: _cf_obj1_1 = _cf_result1_1;

_cf_obj1_1 is set but never referenced again — neither in the try block nor in the finally block. The serialization already uses _cf_result directly (typed_serialize references var_name, not obj_var). The inline comment says serialize(_cf_obj) but the code does serialize(_cf_result).

The generated Java code will contain an unused variable (Object _cf_obj1_1 = null; + _cf_obj1_1 = _cf_result1_1;). Java compilers don't error on unused locals by default, but with -Xlint:all or -Werror they will. The _cf_obj variable and its assignment can be removed entirely. Fix this →

Minor: test moved inside try block

tests/test_languages/test_java/test_instrumentation.py

assertEquals(4, _cf_result1_1) is now inside the try block. This is the expected behavior per the PR description (the original line with call replaced is placed inside try). Semantically correct — if serialization throws, the assertion won't run, but that's acceptable for instrumentation.


Duplicate Detection

No duplicates detected. The --no-build-cache flag is only applicable to Gradle (Maven uses a different property), so the parallel maven_strategy.py and gradle_strategy.py changes are not duplicates — they're intentionally distinct.


Test Coverage

The test update in test_instrumentation.py correctly reflects the new generated structure (renamed _cf_obj, var-typed capture, assertion moved inside try). The _cf_obj variable in the expected test output (Object _cf_obj1_1 = null; / _cf_obj1_1 = _cf_result1_1;) mirrors the dead code bug noted above — removing _cf_obj from the implementation should also remove these two lines from the expected output.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant