review-library-new-request

name	review-library-new-request
description	Review pull requests with the `library-new-request` label in graalvm-reachability-metadata. Use when asked to review or triage a PR that adds metadata and tests for a new library, including PRs titled like `[GenAI] Add support for com.fasterxml:classmate:1.5.1 using gpt-5.4`. Focus on catching scaffold-only tests, version-pinned or package-bypassing tests, and PRs that push more than one library.
argument-hint	[pr-number-or-url]

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 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.

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, prefer asking for code changes over rerunning CI.
   • If CI is flaky but the content is otherwise solid, ask for a rerun after the review issues are resolved.

Output Style

Match the concise review style already used in this repository:

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