review-library-update-request

name

review-library-update-request

description

Review pull requests with the `library-update-request` label in graalvm-reachability-metadata. Use when asked to review or triage a PR that updates metadata/tests for an existing library version, especially generated PRs titled like `[GenAI] Improve coverage for group:artifact:version using gpt-5.5`. Focus on preserving dynamic-access coverage percentage between versions, verifying reporter-requested metadata is present and exercised by tests, and applying the relevant test/scope rules from new-library reviews without requiring a minimum new-library dynamic-access coverage threshold.

Review library-update-request PRs

These PRs update support for an existing library version. Review them as existing-library updates: the target library is already supported, so the PR must preserve coverage quality while adding any metadata or tests requested by the linked issue.

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 percentage-based dynamic-access coverage threshold and regression rule 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 library-update-request.
• The PR should stay scoped to one target existing-library update and its generated support files.
• Dynamic-access coverage percentage must not drop between the previously supported version and the requested version unless the PR gives a concrete, credible reason. Compare the percentage/ratio only; do not block because the absolute covered or total call count changed.
• Do not require library-new-request's minimum dynamic-access threshold. A library update can be acceptable with low coverage if the percentage did not regress and the requested issue work is covered.
• A requested version that reports zero dynamic-access calls (for example dynamicAccess.totalCalls == 0, or a {} reachability-metadata.json with no claimed dynamic-access behavior) does not give enough information to judge whether metadata is unnecessary or whether a metadata-entry-count drop is meaningful. Dynamic-access stats can miss metadata required through transitive dependencies. Do not escalate a shallow test, a test that exercises behavior outside the library's responsibility, or a metadata-entry-count drop to a blocking issue or human-intervention for such a library. The native execution gate and any issue-requested metadata gate still apply.
• If the linked issue requests specific metadata, the PR must contain that metadata and include tests that exercise it through public library API paths.
• Issue-requested metadata may be described in prose, logs, error snippets, JSON, or links. Infer the request from the linked issue body; do not rely only on the PR body summary.
• When issue-requested metadata lacks conditions, require appropriate conditions in the PR metadata, preferably the narrowest valid typeReached condition.
• Tests should execute under native image by default. Do not accept tests that disable themselves under native image. 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 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.
• Do not accept scaffold-only tests. Existing baseline tests can remain, but generated or modified tests must be library-specific and must exercise real behavior.
• Do not accept tests that reference the exact library version in test code or assertions unless the version check is itself the behavior under test.

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 library-update-request.
   • Identify the target coordinate, previous metadata/test version, and requested version from the title, body, changed index.json, metadata path, stats path, and test path.
   • Gather files, reviews, inline comments, issue comments, and CI checks.

2. Resolve and inspect the linked issue.

   • Prefer closingIssuesReferences from gh pr view <pr> --json closingIssuesReferences.
   • Also inspect the PR body for issue references such as Fixes #1234, Closes #1234, or a raw issue URL.
   • Fetch the linked issue with gh issue view <issue> --json number,title,body,labels,url.
   • Confirm the linked issue has or had the library-update-request intent and that its coordinate matches the PR target.
   • Read the issue body for specific metadata requests. Look for explicit reachability metadata JSON, GraalVM missing-registration errors, resource paths, proxy/serialization/JNI requests, class names, method names, and prose such as "this file needs to be available as a resource".

3. Validate the diff scope.

   • Expected files are usually limited to:
     • metadata/<group>/<artifact>/<requested-or-resolved-version>/reachability-metadata.json
     • metadata/<group>/<artifact>/index.json
     • stats/<group>/<artifact>/<requested-or-resolved-version>/stats.json
     • stats/<group>/<artifact>/<requested-or-resolved-version>/execution-metrics.json
     • tests/src/<group>/<artifact>/<requested-or-resolved-version>/**
   • 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.
   • Accept a split from an older shared metadata version to a requested-version metadata/test directory when the issue targets a newer version.
   • Be suspicious of unrelated build logic, workflows, generated sources, other libraries, broad refactors, or changes outside the target coordinate.
   • Reject legacy native-image metadata config files. Metadata for generated support and test-only metadata must use .

Decision Rules

Approve when all of these are true:

• The PR is scoped to the target existing-library update.
• Dynamic-access coverage percentage did not drop between the previous version and requested version, or any drop is convincingly explained by an upstream surface change.
• Any linked-issue metadata request is present in metadata and exercised by tests through public library APIs.
• Tests are not scaffold-only, version-pinned without reason, native-skipped, or placed in the library package without need.
• No legacy native-image metadata config files are introduced.
• Required CI checks are green or only blocked by clearly unrelated infrastructure noise.

Request changes when any of these are true:

• Dynamic-access coverage percentage drops without a credible explanation.
• The linked issue requests metadata that is missing from the PR.
• Requested metadata is added but not exercised through tests.
• Tests use direct reflection, no-op class literals, or direct resource checks to fake requested-metadata coverage.
• The PR fixes native behavior by skipping, disabling, swallowing, or weakening the failing path.
• The PR changes unrelated libraries or infrastructure.
• Legacy config files such as reflect-config.json or resource-config.json are added.

Ask for follow-up instead of rejecting when:

• Previous/new stats needed for the percentage comparison are missing or stale.
• The linked issue request is ambiguous and the PR needs a short explanation tying metadata and tests to the reporter's failure.
• CI failed in a way that looks like infrastructure noise.

Output Style

Keep comments short, factual, and blocking:

• For coverage percentage drops: say that library-update-request PRs must not reduce dynamic-access coverage percentage between versions, cite old and new percentages, and ask for restored coverage or a concrete explanation.
• For missing issue-requested metadata: cite the linked issue request and say the PR must include that metadata.
• For unexercised requested metadata: say the PR adds the requested metadata but does not test it through the library's public API path.
• For fake requested-metadata coverage: say direct reflection/class literals/direct resource lookup prove only the test code, not the library path that needs metadata.
• For native skips: say the PR avoids the native path instead of proving the library behavior works under native image.
• For unverified catch (Error): say dynamic class loading tests should verify Native Image failures with NativeImageSupport.isUnsupportedFeatureError(e) and re-throw any other error.
• For version-pinned tests: say tests should not reference the exact library version because the same test should support multiple library versions.
• For package-bypassing tests: say tests should not live in the library package unless the PR demonstrates why that is necessary.
• For legacy metadata files: say metadata must use reachability-metadata.json and ask for old config files to be replaced.

Examples

Use these examples as representative patterns, not exhaustive matchers.

• Bad issue-requested metadata coverage with direct reflection:

@Test
void requestedMethodExists() throws Exception {
    Method method = HikariConfig.class.getMethod("setPassword", String.class);

    assertThat(method.getName()).isEqualTo("setPassword");
}

• Better issue-requested metadata coverage through public API:

@Test
void configuresPasswordThroughHikariConfig() {
    HikariConfig config = new HikariConfig();
    config.setJdbcUrl("jdbc:h2:mem:test");
    config.setUsername("sa");
    config.setPassword("secret");

    assertThat(config.getPassword()).isEqualTo("secret");
}

• Bad resource coverage with direct lookup only:

@Test
void versionResourceExists() {
    URL resource = getClass().getClassLoader()
            .getResource("ch/qos/logback/core/logback-core-version.properties");

    assertThat(resource).isNotNull();
}

• Better resource coverage through library initialization:

@Test
void initializesLoggerContextWithoutUnknownVersionWarning() {
    LoggerContext context = new LoggerContext();
    context.setName("test");
    context.start();

    assertThat(context.isStarted()).isTrue();
}

• Bad native-image skip:

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

    LibraryConfig config = LibraryConfig.parse("name=value");
    assertThat(config.get("name")).isEqualTo("value");
}

• Approved dynamic-class-loading pattern:

@Test
void loadsPluginFromExternalJar() throws Exception {
    Path pluginJar = compilePluginJar();
    try (URLClassLoader loader = new URLClassLoader(new URL[] { pluginJar.toUri().toURL() })) {
        Plugin plugin = PluginRegistry.load(loader, "example.Plugin");
        assertThat(plugin.name()).isEqualTo("example");
    } catch (Error e) {
        if (!NativeImageSupport.isUnsupportedFeatureError(e)) {
            throw e;
        }
    }
}