Review Library New Request

Review library-new-request PRs

These PRs usually add reachability metadata and tests for one target library coordinate. Review them with extra scrutiny.

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 (for example, an open review tab, a PR URL mentioned earlier, or the current branch's PR from 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 Signals

Treat the following as hard review rules unless the PR provides a strong reason otherwise:

1. Single library per PR. Do not accept PRs that push more than one library. A library-new-request PR must stay scoped to one target library version plus its supporting test files.
2. No scaffold-only tests. A test is "scaffold" only when its body is still the unmodified placeholder emitted by the TCK scaffolding (tests/tck-build-logic/src/main/resources/scaffold/Test.{java,groovy,kt,scala}.template): a single test() method whose body is just the System.out.println("This is just a placeholder, implement your test") (or println / language equivalent) stub. That is the exact bar — do not call a test "scaffold" merely because it is short, asserts a single constant, or looks weak. Once the agent has replaced the placeholder body with code that actually invokes the target library, it is no longer a scaffold. Do not reject such a test as a scaffold, and do not reject it as "insufficient coverage" on your own judgment that it looks thin or weak — coverage rejection is driven only by the concrete coverage rules below (rules 6–9). Generated tests must be changed into library-specific tests that exercise the behavior that requires the metadata.
3. Native-image execution by default. 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 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.
4. No version-pinned tests. 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. One test should remain valid across multiple supported library versions.
5. Separate test and library packages. Keep test packages separate from library packages. Reject tests that live under the target library's package unless the PR clearly needs that package placement to exercise the library, since tests inside the library package can bypass visibility boundaries and give false confidence about what user code can access.
6. Dynamic-access coverage above 20%. Require reported dynamic-access coverage above 20% when there are dynamic-access calls to cover. Use the exploded stats files under stats/<group>/<artifact>/<metadata-version>/stats.json when available, and block the PR when dynamic-access coverage is 20% or lower or the PR provides no credible dynamic-access coverage evidence for non-zero dynamic-access calls.
7. Coverage is a minimum gate. Treat dynamic-access coverage counts as a minimum gate, not complete metadata evidence. They can miss metadata required through downstream libraries, so do not use high coverage alone to prove that the submitted metadata is complete or necessary.
8. 0/0 is a valid no-calls case. Treat 0/0 dynamic-access coverage as a valid no-calls case, not as failed coverage. Do not reject a PR only because the exploded stats report 0/0 dynamic-access calls while the PR adds metadata; those stats can miss metadata required through downstream libraries.
9. Do not read individual metadata entries. Do not explicitly read or review individual metadata entries. Use the metadata entry count reported in the PR description for the metadata/dynamic-access mismatch check; do not manually count entries from metadata files. When the PR reports both library metadata entries and test-only metadata entries, use their sum as the PR-reported metadata entry count. Only request investigation when the covered dynamic-access call count is at least 75% higher than that PR-reported total. Skip this mismatch check entirely when the PR reports zero dynamic-access calls, because 0/0 does not provide enough information to judge whether the metadata entry count is relevant.
10. Only reachability-metadata.json. 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.
11. Test-only is defined by package. Metadata is "test-only" when the type it describes belongs to a package from the test suite. A type from the library JAR or one of its dependencies is library metadata, even when a test is what exercised it.

Workflow

Work through the steps in order. Each step cites the Review Signals (by number, e.g. Review Signals #2) that apply; those rules are authoritative, so apply them there rather than restating them here.

1. Inspect the PR summary.

   • Resolve the target PR from the optional argument, or infer it from context when the user just says "review this PR". Ask the user only if it cannot be inferred.
   • Confirm the PR has label library-new-request.
   • Read the title and body to identify the target coordinates.
   • Gather files, reviews, inline comments, and CI checks.

2. Validate the diff shape — apply Review Signals #1 (single library) and #11 (only reachability-metadata.json).

   • Expected files are usually limited to:
     • metadata/<group>/<artifact>/<version>/reachability-metadata.json
     • metadata/<group>/<artifact>/index.json
     • stats/<group>/<artifact>/<new-version>/stats.json
     • tests/src/<group>/<artifact>/<version>/**
   • Be suspicious of changes to build logic, workflows, unrelated libraries, generated sources outside the target test directory, or wide refactors.

3. Review the test source — apply Review Signals #2–#6 (scaffold, native-image, version-pinning, package separation, JVM overrides) to the test body and its build script.

4. Review the metadata files only for presence and scope — apply Review Signals #11 (only reachability-metadata.json) and #10 (do not read individual entries).

   • Confirm the expected metadata files exist for the single target coordinate.
   • Treat reachability-metadata.json containing {} as acceptable when the rest of the PR is coherent and validation passes.

5. Compare the PR claims, test, and reported coverage as one unit — apply Review Signals #7–#10 (coverage gate, minimum-gate caveat, 0/0 handling, and the metadata/dynamic-access mismatch check).

   • Use the PR-reported numbers; when they do not line up with the diff, ask for investigation rather than counting entries yourself.

6. Check validation status.

   • Expected minimum: metadata validation and library test jobs for the target coordinates are green.
   • If the PR is small and the review concerns correctness rather than infra noise, repair the PR under Disposition rather than rerunning CI.
   • If CI is flaky but the content is otherwise solid, ask for a rerun after the review issues are resolved.

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.

Output Style

Match the concise review style already used in this repository. Each line below is what you say about the violation; Disposition above decides whether you say it in a repair note, in a blocked-on-infrastructure comment, in a close, or in a human-intervention comment.

• For scaffold-only tests: say that tests must differ from the scaffold and should not be accepted as-is.
• 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 multiple-library PRs: say that library-new-request PRs must push only one library and ask for the unrelated library additions to be removed.
• For insufficient dynamic-access coverage: say that new-library PRs need dynamic-access coverage above 20% when there are dynamic-access calls to cover, and ask for stronger tests or refreshed coverage evidence.
• For unsupported coverage claims: ask for refreshed coverage evidence when the PR makes concrete coverage claims that are not supported by the diff.
• For metadata/dynamic-access entry mismatch: ask for investigation only when the PR reports non-zero dynamic-access calls and covered dynamic-access calls are at least 75% higher than the PR-reported metadata entry count, summing library and test-only metadata entries when both are reported. Do not manually count or argue from metadata contents, and do not run this check for 0/0 reports.
• 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.

Keep comments short, factual, and blocking. Focus on the concrete defect, not a long explanation.

Blocking Test Examples

Use these examples as representative patterns, not exhaustive matchers:

• Scaffold-only placeholder. This is the exact body the TCK scaffolding generates; "scaffold" means the test still looks like this (only the class/package name filled in). Anything that replaces this println body with real library calls is no longer a scaffold:

class ExampleLibraryTest {
    @Test
    void test() throws Exception {
        System.out.println("This is just a placeholder, implement your test");
    }
}

• 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");
}

• Bare catch (Error) without verification:

@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) {
        // Swallowed without verifying the error type.
    }
}

• 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;
        }
        // Expected: Native Image cannot load classes from JARs discovered after build.
    }
}

• Version-pinned assertion:

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

• Test placed inside the library package without a demonstrated need:

package org.example.library;

class InternalAccessTest {
    @Test
    void bypassesPublicApiByCallingPackagePrivateConstructor() {
        DefaultPluginRegistry registry = new DefaultPluginRegistry();
        InternalPlugin plugin = registry.create("json");

        assertThat(plugin.name()).isEqualTo("json");
    }
}