Split reviewer skill rules into conditional files

[jonathanpeppers]

Split the monolithic review-rules.md (82 rules) into 6 focused files that can be conditionally loaded based on which files changed in a PR. Also added 14 new rules from dotnet/android#11266.

New file structure

Always loaded (9,124 chars):

• repo-conventions.md — repo-specific patterns (ProcessUtils, FileUtil, naming, style)
• ai-pitfalls.md — common AI code generation mistakes
• security-rules.md — archive, path, and command safety

Conditionally loaded:

• csharp-rules.md — general C# guidance (when .cs files change)
• msbuild-rules.md — MSBuild task guidance (when .targets/.props change)
• testing-rules.md — test review guidance (when test files change)

Token usage by PR type

The old monolithic file was always loaded regardless of what changed. The new split loads only relevant rules:

Scenario	Old (chars)	New (chars)	Change
Docs-only PR	19,771	9,124	-54%
Test-only PR	19,771	10,161	-49%
MSBuild-only PR	19,771	11,179	-43%
Typical C# PR	19,771	20,851	+5%
C# + tests PR	19,771	21,888	+11%
Full PR (all types)	19,771	23,943	+21%

The +5-21% increase for C# PRs is due to 14 new rules added from dotnet/android. Non-code PRs see 43-54% savings.

14 new rules from dotnet/android#11266

Added to csharp-rules.md to align with the android repo's split:

Async (3): Lock ordering, avoid double-checked locking (prefer Lazy<T>), singleton initialization completeness

Error Handling (4): Log messages must have context, differentiate similar error messages, assert boundary invariants, initialize output parameters in all paths

Performance (5):HashSet.Add() already handles duplicates, don't wrap in $"{str}", Split() with count parameter, cache repeated accessor calls, extract throw helpers ([DoesNotReturn])

Code Organization (2):sealed classes skip full Dispose, use interfaces over concrete types

Evaluation against 3 merged PRs

Simulated reviews with old vs new rules on PRs #337, #338, #344:

PR	Author	Old Findings	New Findings	Net New	Verdict
#337 (concurrency fix)	Copilot	3	3	0	Tie — both catch incomplete lock
#338 (hash algorithm)	Human	4	5	0	Tie — same important findings
#344 (nullable enable)	Copilot	4	5	0	Marginal + — upgrades vague thread-safety to concrete Lazy<T> recommendation

Key findings:

• No regressions — new rules catch everything old rules did
• Better actionability — when rules fire, new rules give more specific fix recommendations
• No increase in false positives — new rules correctly stay silent when patterns don't match
• The 14 new rules target patterns absent from these 3 test PRs — they'll show value on PRs with concurrency code, HashSet usage, out parameters, or throw-heavy hot paths
• Shareability — csharp-rules.md, security-rules.md, ai-pitfalls.md, msbuild-rules.md, and testing-rules.md are generalized for reuse across .NET repos (paired with Split android-reviewer review-rules.md into conditional rule files android#11266)

All 96 rules verified

Every rule from the original 82 was mapped to a new file and verified present. 14 new rules added from dotnet/android alignment. Zero rules lost.

[jonathanpeppers]

/review

[github-actions]

✅ Android Tools PR Reviewer completed successfully!

[Copilot]

Pull request overview

Refactors the android-tools reviewer skill guidance by splitting the previously monolithic review-rules.md into smaller rule reference files that can be loaded selectively based on what a PR changes, reducing token usage for non-code PRs while adding new C# review rules.

Changes:

• Replaced references/review-rules.md with 6 focused rule files (repo-conventions, ai-pitfalls, security, plus conditional csharp, msbuild, testing).
• Updated SKILL.md to instruct conditional loading of the new rule files based on changed paths/extensions.
• Added the new C# rule set content (async/error-handling/performance/organization additions aligned with dotnet/android#11266).

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
.github/skills/android-tools-reviewer/SKILL.md	Updates the skill workflow to load the new split rule files conditionally.
.github/skills/android-tools-reviewer/references/repo-conventions.md	Adds always-loaded repo-specific conventions (ProcessUtils/FileUtil/logging/style).
.github/skills/android-tools-reviewer/references/ai-pitfalls.md	Adds always-loaded checklist of common AI-generated code pitfalls.
.github/skills/android-tools-reviewer/references/security-rules.md	Adds always-loaded security checklist for archives/paths/process execution.
.github/skills/android-tools-reviewer/references/csharp-rules.md	Adds conditionally-loaded general C#/.NET review guidance plus new rules from dotnet/android alignment.
.github/skills/android-tools-reviewer/references/msbuild-rules.md	Adds conditionally-loaded MSBuild task/targets guidance.
.github/skills/android-tools-reviewer/references/testing-rules.md	Adds conditionally-loaded test review guidance.
.github/skills/android-tools-reviewer/references/review-rules.md	Removes the old monolithic rules file in favor of the split structure.

[github-actions]

✅ LGTM

Verified: All 82 original rules are present in the new split files. 14 new rules from dotnet/android#11266 confirmed added (3 async, 4 error handling, 5 performance, 2 code organization). The new file total (100 table rows) is slightly higher than 96 due to a few editorial additions in msbuild-rules.md (explicit log-level guidance, stdout-in-diagnostics) that complement the reorganization.

CI: All checks passing.

What works well

• Clean separation: repo-specific rules (repo-conventions.md) vs. reusable rules (csharp-rules.md, security-rules.md, etc.) is a good design for sharing across repos
• Conditional loading in SKILL.md is well-documented and will save significant context for non-C# PRs (43-54% reduction)
• New rules are high-quality: double-checked locking → Lazy<T>, HashSet.Add() duplicate check, throw helper extraction — all target real patterns
• No regressions: the evaluation against 3 merged PRs is a solid validation approach

Summary

Severity	Count
💡 Suggestion	3

All suggestions are minor documentation improvements — no blocking issues found.

Generated by Android Tools PR Reviewer for issue #355 · ● 3M