review-fixes-javac-fail

name	review-fixes-javac-fail
description	Review pull requests with the `fixes-javac-fail` label in graalvm-reachability-metadata. Use when asked to review or triage a PR that fixes Java compilation failures for an existing library version update. Focus on validating the compile fix, keeping the diff scoped, and applying the repair coverage gate to overall and breakdown dynamic-access reports.

Review fixes-javac-fail PRs

These PRs repair test sources or test dependencies after an existing library is bumped and the new version no longer compiles with the old test code. Review them more lightly than library-new-request PRs: the library already exists, so the goal is to preserve working coverage across the version update rather than prove a brand-new metadata contribution from scratch.

The rules below implement the contribution contract: block only on a concrete violation of a must — the shape limits (§FS-contribution-contract.2), the coverage gates (§FS-contribution-contract.3), the cheating patterns (§FS-contribution-contract.4), and the test contract's musts (§FS-test-contract) — while shoulds stay advisory in review (§FS-contribution-contract.1).

Chunked dynamic-access PRs

When the PR has the chunked-dynamic-access label, skip every repair coverage comparison in this skill, including for the final chunk. Request changes for coverage only when the reported dynamicAccess.coveredCalls is 0. Keep every non-coverage rule. §FS-contribution-contract.3

The PR number or URL can be passed as an optional argument (for example, 1234, https://github.com/oracle/graalvm-reachability-metadata/pull/1234). If the user says "review this PR" without an argument, infer the PR from the surrounding conversation or gh pr status; only ask the user when it cannot be inferred. Use gh pr view <pr>, gh pr diff <pr>, and gh pr checks <pr> against the resolved PR throughout the workflow below.

Review Principles

• Confirm the PR has label fixes-javac-fail.
• Expect compile-focused changes plus the normal generated support files for the newly tested version: test source updates, imports, renamed APIs, dependency adjustments, a new metadata-version directory, stats, and metadata index changes.
• Be more relaxed than library-new-request: do not reject only because a test resembles older coverage, stays in an existing package layout, or contains compatibility branches for multiple supported versions.
• Do not accept changes that remove meaningful test coverage just to make javac pass.
• Native-image execution should remain enabled by default when the repaired test reaches the native lane. Do not accept changes that skip or short-circuit native-image behavior. When a test exercises behavior that fundamentally relies on open-ended dynamic class loading that native image cannot support, such as loading classes, JARs, generated bytecode, plugin implementations, or other class definitions that are only discovered after the native executable is built, accept the NativeImageSupport.isUnsupportedFeatureError(e) catch pattern from org.graalvm.internal.tck. Reject raw native-only skips, bare catch (Error) blocks, and use of this pattern for ordinary reflection, resources, serialization, dynamic proxies, JNI, or missing reachability metadata.
• Treat dynamic-access coverage preservation as the main quality gate. Apply the ordered repair comparison in the workflow below independently to the overall report and every breakdown present in either version.
• Use the reported stats evidence as-is. Do not inspect generation filters, agent configuration, or metadata contents to second-guess the reported totalCalls, coveredCalls, or coverage ratios.
• Treat metadata entry counts as telemetry only. Never request changes or apply human-intervention because the new version reports fewer metadata entries.
• A new version that reports zero total dynamic-access calls has no comparable call surface and passes the numeric coverage gate. The compile fix itself must still pass and must not be made green by deleting meaningful coverage or disabling native-image behavior.
• Accept only reachability-metadata.json files as metadata files. Reject legacy native-image metadata config files such as reflect-config.json, resource-config.json, proxy-config.json, serialization-config.json, jni-config.json, or predefined-classes-config.json.
• Prefer small, targeted review comments. This label is for repair work, not a full redesign of historical tests.

Workflow

1. Inspect the PR summary.

   • Resolve the target PR from the optional argument, or infer it from context when possible.
   • Confirm the PR has label fixes-javac-fail.
   • Identify the target coordinate, the previous tested version, and the new tested version from the PR body, title, changed index.json, and changed test path.
   • Gather files, reviews, inline comments, and CI checks.

2. Validate the diff scope.

   • Expected files are usually limited to the target coordinate's generated new-version support:
     • metadata/<group>/<artifact>/<new-version>/reachability-metadata.json
     • metadata/<group>/<artifact>/index.json
     • stats/<group>/<artifact>/<new-version>/stats.json
     • tests/src/<group>/<artifact>/<new-version>/**
   • Accept compatibility edits that keep one test source working across multiple tested versions.
   • Treat a new reachability-metadata.json for the tested version as normal for this label, including {} when validation and stats are coherent.
   • Treat generated test project files such as .gitignore, build.gradle, gradle.properties, settings.gradle, and user-code-filter.json as normal when they live under the target version's test directory.
   • Be suspicious of unrelated build logic, workflows, generated sources, other libraries, or broad refactors.
   • Reject legacy native-image metadata config files. Metadata for generated support and test-only metadata must use reachability-metadata.json.
   • Reject or request changes if the PR removes tests, disables test classes, catches and ignores the failing exception, or disables native-image behavior.
   • Accept the NativeImageSupport.isUnsupportedFeatureError(e) catch pattern for tests that exercise behavior fundamentally requiring unsupported open-ended dynamic class loading. Reject bare catch (Error) blocks without the isUnsupportedFeatureError verification.

3. Review the compile fix.

   • Confirm the edit addresses the actual Java compilation failure, such as renamed classes, changed method signatures, module boundaries, annotation processors, or dependency coordinates.
   • Compatibility branches are acceptable when they keep older and newer tested versions covered by the same test.

Decision Rules

Approve when all of these are true:

• The PR is scoped to the target existing library and the compile failure it fixes.
• Tests still exercise the same meaningful library behavior after the compile repair.
• The overall report and every breakdown pass the ordered repair coverage gate, or any failing scope is convincingly explained by a changed upstream surface.
• Required compile and metadata test checks are green.

Request changes when any of these are true:

• The fix makes compilation pass by weakening or bypassing the test instead of adapting it to the changed API while preserving meaningful coverage.
• The fix disables native-image behavior instead of using the NativeImageSupport.isUnsupportedFeatureError(e) catch pattern for unsupported open-ended dynamic class loading.
• The overall report or a breakdown fails the ordered repair coverage gate without a credible explanation and replacement coverage.
• CI failures indicate the compile problem is not actually fixed.

Ask for follow-up instead of rejecting when:

• Stats needed for the old/new version comparison are missing or stale.
• CI failed in a way that looks like infrastructure noise.
• A failing repair-coverage scope may reflect a plausible upstream API or runtime change, but the PR does not explain it.

Output Style

Keep comments short and factual:

• For coverage failures: name the failing overall or breakdown scope, report old and new coveredCalls, totalCalls, and percentages, and identify whether it failed the zero-covered, small-report, or large-report rule. Ask for restored coverage or a concrete explanation. Explain why the regression happened using evidence from the stats, test diff, upstream changes, or CI. If the cause cannot be established, say so explicitly and request an explanation; never guess. Do not report a change that an earlier step of the ordered gate accepts.
• For deleted coverage: say that the PR fixes compilation by removing coverage and should instead adapt the test to the new API.
• For native skips that does not depend on the open-ended dynamic class loading: say that the PR avoids the failing native path instead of fixing it, so it does not demonstrate native-image runtime coverage.
• For unverified catch (Error): say that dynamic class loading tests should verify Native Image failures with NativeImageSupport.isUnsupportedFeatureError(e) and re-throw any other error.
• For version-pinned tests: say that tests should not reference the exact library version because the same test should support multiple library versions.
• For unrelated changes: say the PR should stay scoped to the fixes-javac-fail repair and remove unrelated files.
• For legacy metadata files: say that metadata must use reachability-metadata.json and ask for old config files such as reflect-config.json or resource-config.json to be replaced.
• For missing stats: ask for regenerated library stats or CI evidence before approval.

Examples

Use these examples as representative patterns, not exhaustive matchers.

• Bad compile repair that removes coverage:

@Test
void parsesDocument() {
    // Removed assertions because the new API no longer compiles.
}

• Bad native-image skip:

@Test
void parsesDocument() {
    assumeFalse("runtime".equals(System.getProperty("org.graalvm.nativeimage.imagecode")));

    Document document = Parser.parse("name=value");
    assertThat(document.get("name")).isEqualTo("value");
}

• Approved dynamic-class-loading pattern:

@Test
void loadsGeneratedHandlerClass() throws Exception {
    try {
        Class<?> handlerClass = compileAndLoadHandler();
        assertThat(HandlerRegistry.register(handlerClass).name()).isEqualTo("generated");
    } catch (Error e) {
        if (!NativeImageSupport.isUnsupportedFeatureError(e)) {
            throw e;
        }
        // Expected: Native Image cannot load generated classes discovered after build.
    }
}

• Bad version-pinned assertion:

@Test
void reportsVersion() {
    assertThat(LibraryVersion.current()).isEqualTo("1.2.3");
}