Review Library Update Request

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.
• Keep test packages separate from library packages unless the PR clearly needs package placement to exercise the library. Tests placed inside the library package can bypass visibility boundaries and give false confidence.
• 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, including under test-only metadata.
• Prefer concrete evidence from the linked issue, generated tests, metadata, stats, and CI over speculation.

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 reachability-metadata.json.

4. Check dynamic-access coverage percentage.

   • Compare the previous supported version and the requested version using stats files when available:
     • stats/<group>/<artifact>/<old-version>/stats.json
     • stats/<group>/<artifact>/<new-version>/stats.json
   • Use versions[0].dynamicAccess.coverageRatio or the equivalent ratio shown in the PR description.
   • Compare the top-level dynamic-access percentage/ratio. Do not require covered call counts or total call counts to match, and do not block just because one version has fewer dynamic-access calls.
   • A lower coverage percentage for the requested version is blocking unless the PR gives a concrete explanation, such as upstream removal of reachable dynamic-access sites or an intentionally narrower supported surface.
   • Small formatting or rounding differences are not blocking; compare using the precision reported by the stats or PR body.
   • If stats are missing or stale, ask for regenerated stats or CI evidence before approval.
   • Do not apply the library-new-request rule that dynamic-access coverage must be above 20%.

5. Verify issue-requested metadata and tests.

   • If the linked issue requests specific metadata, inspect the PR's reachability-metadata.json and test-only metadata to confirm the requested metadata exists.
   • It is acceptable to inspect metadata entries for this issue-requested metadata check. Avoid broader entry-by-entry review beyond the reporter request unless there is a concrete failure.
   • Confirm metadata conditions are present and appropriate when the issue did not specify them. Prefer condition.typeReached scoped to the library path that needs the metadata.
   • Confirm tests exercise the requested metadata through public library API behavior:
     • Reflection metadata should be reached by library behavior that reflectively calls, constructs, reads, writes, proxies, serializes, or loads the target.
     • Resource metadata should be reached by library initialization or APIs that load the resource, not by a test directly calling ClassLoader.getResource only to prove the file exists.
     • Proxy/serialization/JNI metadata should be reached by the corresponding public feature.
   • Reject tests that satisfy the request with direct reflection against the metadata target, no-op class literals, assertions that only mention the target class/resource, or test-only code that bypasses the library path.
   • If the agent adds metadata requested by the issue but no test exercises it, request changes.
   • If the issue request is ambiguous, ask for a concrete explanation in the PR and ensure the tests cover the most plausible reporter-described failure path.

6. Review test quality.

   • The generated or modified tests must be library-specific and not a scaffold with superficial edits.
   • Tests should run under both JVM and native lanes unless the feature fundamentally depends on unsupported open-ended dynamic class loading and uses the approved NativeImageSupport.isUnsupportedFeatureError(e) pattern.
   • Reject native skips, early returns, disabled native tests, swallowed exceptions, or weakened assertions that hide the failing path.
   • Reject version-pinned assertions unless the version value itself is the behavior under test.
   • Reject tests placed in the target library package without a clear need.
   • Existing baseline tests may be version-compatible and contain branches for old/new behavior, but added branches must still assert meaningful library behavior.
   • When the requested version reports zero dynamic-access calls, do not escalate test-depth concerns (a shallow test, or one exercising behavior outside the library's responsibility) to a blocking issue or human-intervention. The zero-call report gives too little information to judge metadata relevance, so depth-of-coverage objections carry little signal. The native execution gate and any issue-requested metadata gate still apply.

7. Check CI before deciding.

   • Expected minimum: metadata validation, compile, JVM tests, native-image compile, and native-image runtime tests are green for the target coordinate.
   • If current-defaults and future-defaults lanes both run, both should pass unless the PR clearly explains an unrelated infrastructure issue.
   • If CI is flaky but the diff, issue-requested metadata, and coverage comparison are sound, ask for a rerun instead of blocking on speculation.

Disposition

Finding a violation is half the review. Read §FS-contribution-contract.5 and take the disposition it assigns — grund FS-contribution-contract.5 --full, or section 5 of docs/functional-spec/contribution-contract.md in the checkout. That section is authoritative and this table is only a dispatch: it tells you which case you are in, not what the case permits. You are the party that acts, because the PR was generated and no author is waiting to answer a review comment.

Never close a PR under any case but 5.4. Read §FS-contribution-contract.5.1 before you repair: it bounds what a repair may do, and it carries the one exception that lets you drop a test scenario rather than fix it.

Decision Rules

These decide the verdict. Disposition above decides what you then do with it: a violation you can correct inside the PR's own files is repaired and pushed, not requested from an author who is not there.

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. A transient external failure is waited out; a reproducible defect in the repository's own code is case 5.3 of Disposition (§FS-contribution-contract.5.3).

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;
        }
    }
}