Add java-interop-reviewer skill for PR code reviews

[jonathanpeppers]

Create a code reviewer skill modeled after dotnet/android and dotnet/android-tools reviewer skills, adapted for dotnet/java-interop.

Structure

• SKILL.md — Main workflow: gather diff → independent assessment → load conditional rules → analyze → post review
• references/ — Rule files loaded conditionally based on changed file types

Reusable reference files (globally applicable to any .NET repo)

File	Description
ai-pitfalls.md	Common AI code generation mistakes (formatting, ! operator, swallowed errors, over-engineering)
csharp-rules.md	Nullable, async/cancellation, error handling, performance, code organization
security-rules.md	Zip Slip, path traversal, command injection, elevation
testing-rules.md	NUnit conventions, regression tests, generator codegen tests
msbuild-rules.md	Task logging, process management, XML conventions
native-rules.md	C++ memory management, RAII, nullptr, const, standard headers

Repo-specific reference files (distilled from PR review history)

File	Description
repo-conventions.md	Mono formatting style, JNI naming disambiguation, actionable error messages, localization guidance, startup performance, trimmer/AOT compatibility, downstream consumer impact
interop-rules.md	JNI reference lifecycle, JniPeerMembers caching, virtual vs non-virtual dispatch, [Register] accuracy, JniTransition, P/Invoke marshalling

PR reviews analyzed

Repo-specific rules were distilled from the PRs with the most code review comments:

PR	Comments	Title
#849	112	[generator] Replace ApiXmlAdjuster with Java.Interop.Tools.JavaTypeSystem
#1179	50	[Java.Interop.Tools.Maven] Initial commit
#696	31	[jnimarshalmethod-gen] Localizable errors
#689	26	[generator] Make warning and error messages localizable
#459	21	[generator] Support default interface methods
#691	18	[java-interop, Java.Interop] Securely load native libs
#804	16	[Java.Runtime.Environment] Partial support for .NET Core
#756	19	[ApiXmlAdjuster] Use different data model structures for better performance
#458	15	Generator changes to support api level 29 and fixes to support extended events
#1153	14	[Hello-NativeAOTFromJNI] Add NativeAOT sample
#1168	12	[Java.Interop] Avoid Type.GetType() in ManagedPeer
#988	14	[build] Target net7.0
#1184	10	[java.interop] address some "easy" trimmer warnings

Key patterns distilled from these reviews:

• Naming ambiguity at Java/C# boundary (PR [generator] Replace ApiXmlAdjuster with Java.Interop.Tools.JavaTypeSystem #849: FullName, ReferenceType)
• Named constants for string lengths instead of magic numbers (PR [generator] Replace ApiXmlAdjuster with Java.Interop.Tools.JavaTypeSystem #849: "Invoker".Length not 7)
• StringComparison.Ordinal for identifier comparisons (PR [generator] Replace ApiXmlAdjuster with Java.Interop.Tools.JavaTypeSystem #849)
• Actionable error messages with codes and fix guidance (PRs [jnimarshalmethod-gen] Localizable errors #696, [generator] Make warning and error messages localizable. #689)
• Localization resource comments for translators (PRs [jnimarshalmethod-gen] Localizable errors #696, [generator] Make warning and error messages localizable. #689)
• JNI virtual vs non-virtual dispatch correctness (PR [generator] Support default interface methods. #459)
• C++ standard headers and nullptr consistency (PR [java-interop, Java.Interop] Securely load native libs #691)
• Trimmer/NativeAOT annotations ([DynamicallyAccessedMembers], [UnconditionalSuppressMessage]) (PRs [Hello-NativeAOTFromJNI] Add NativeAOT sample #1153, [java.interop] address some "easy" trimmer warnings #1184)
• Startup performance sensitivity — arrays vs dictionaries for small lookups with benchmarks (PR [Java.Interop] Avoid Type.GetType() in ManagedPeer #1168)
• Downstream consumer impact on dotnet/android (PRs [build] Target net7.0. #988, [Hello-NativeAOTFromJNI] Add NativeAOT sample #1153)
• HttpClient must be static, readonly for static fields (PR [Java.Interop.Tools.Maven] Initial commit. #1179)

Evaluation

Evaluated the skill against 4 PRs not used to build the rules:

PR	Title	Catches	Misses	Coverage
#555	[Java.Interop] remove IEnumerable iterators called at startup	4	1	80%
#323	[java-interop] Implemented Windows version of ji_realpath	4	1	80%
#1125	[Java.Interop.Tools.JavaSource] Improve <code> parsing	3	2	60%
#88	[Java.Interop.Tools.Cecil] Fix DirectoryAssemblyResolver.Dispose()	2	3	40%
Overall		13	7	65%

What the skill catches well (100% coverage):

• Error handling — actionable messages, assertions, fail-fast
• Performance — unnecessary allocations, static fields
• Resource management — using / IDisposable
• Code organization — indentation, duplication
• Memory safety — null checks in native code
• Testing — edge cases

What the skill can't catch (misses):

PR	Finding	Category
#88	.mdb is appended not extension change	Domain knowledge
#88	ReadSymbols=true behavior with missing symbols	Domain knowledge
#88	.ppdb extension clarification	Domain knowledge
#323	MAX_PATH limitations	Platform knowledge
#555	return default vs return null clarity	Readability
#1125	HTML tags need case-insensitive matching	Domain knowledge
#1125	Alternative design: regex vs parser	Design alternative

Verdict

Net positive, no significant false positive risk. The skill captures universal code quality patterns (65% of findings) that reviewers consistently flag. The 35% it misses is domain-specific expertise that no general ruleset can replace. The skill complements human reviewers — it handles mechanical checks so humans can focus on domain expertise.

[jonathanpeppers]

CI might be broken, but hopefully fixed by:

• [Hello-Java.Base] Add missing jcw-gen project dependency #1418

[Copilot]

Pull request overview

Adds a new .github/skills/java-interop-reviewer reviewer skill for dotnet/java-interop, defining a review workflow (SKILL.md) and a set of reusable + repo-specific rule/reference documents intended to guide automated PR reviews.

Changes:

• Introduce java-interop-reviewer skill definition + end-to-end review workflow guidance.
• Add repo-specific “conventions” reference and general C#/MSBuild/native/security/testing guidance rule files.
• Add an “AI pitfalls” checklist to reduce common AI-generated review/code issues.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
.github/skills/java-interop-reviewer/SKILL.md	Main skill definition + review workflow and output format
.github/skills/java-interop-reviewer/references/ai-pitfalls.md	Checklist of common AI-generated mistakes to watch for
.github/skills/java-interop-reviewer/references/csharp-rules.md	General C# review rules (nullable/async/error/perf/organization)
.github/skills/java-interop-reviewer/references/interop-rules.md	Managed/native boundary and JNI/PInvoke review checks
.github/skills/java-interop-reviewer/references/msbuild-rules.md	MSBuild targets/tasks review checks
.github/skills/java-interop-reviewer/references/native-rules.md	C/C++ and memory-management review checks
.github/skills/java-interop-reviewer/references/repo-conventions.md	Repo-specific formatting/style/perf/localization/downstream guidance
.github/skills/java-interop-reviewer/references/security-rules.md	Security checklist for archives/paths/process execution
.github/skills/java-interop-reviewer/references/testing-rules.md	NUnit/testing expectations and generator-test guidance