# Review Library New Request > 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. Use when this capability is needed. - Skill: `tomevault-io/review-library-new-request` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add tomevault-io/review-library-new-request` - Raw SKILL.md: https://api.skillmd.com/api/skills/tomevault-io/review-library-new-request/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: tomevault-io (https://skillmd.com/u/tomevault-io) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/tomevault-io/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 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 `, `gh pr diff `, and `gh pr checks ` 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 7–10). 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. **No JVM toolchain/target/release overrides.** Do not accept newly added JVM toolchain, target, or release overrides when they move tests away from the TCK-resolved test JVM version. When a language plugin needs an explicit `JavaLanguageVersion.of(...)`, `jvmToolchain(...)`, `kotlinOptions.jvmTarget = ...`, or `options.release = ...` setting, it should use the resolved test JVM version instead of hardcoding a number. Reject old-JDK compatibility flags such as `-Djava.security.manager=allow`. 7. **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////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. 8. **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. 9. **`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. 10. **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. 11. **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`. 12. **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////reachability-metadata.json` - `metadata///index.json` - `stats////stats.json` - `tests/src////**` - 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: ```java class ExampleLibraryTest { @Test void test() throws Exception { System.out.println("This is just a placeholder, implement your test"); } } ``` - Native-image skip: ```java @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: ```java @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: ```java @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: ```java @Test void reportsVersion() { assertThat(LibraryVersion.current()).isEqualTo("1.2.3"); } ``` - Test placed inside the library package without a demonstrated need: ```java package org.example.library; class InternalAccessTest { @Test void bypassesPublicApiByCallingPackagePrivateConstructor() { DefaultPluginRegistry registry = new DefaultPluginRegistry(); InternalPlugin plugin = registry.create("json"); assertThat(plugin.name()).isEqualTo("json"); } } ``` --- > Source: [oracle/graalvm-reachability-metadata](https://github.com/oracle/graalvm-reachability-metadata) — distributed by [TomeVault](https://tomevault.io).