Split reviewer skill rules into conditional files by jonathanpeppers · Pull Request #355 · dotnet/android-tools

jonathanpeppers · 2026-05-01T21:10:40Z

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	Verdict
#337 (concurrency fix)	Copilot	3	3	Tie — both catch incomplete lock
#338 (hash algorithm)	Human	4	5	Tie — same important findings
#344 (nullable enable)	Copilot	4	5	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.

Split the monolithic review-rules.md into focused files that can be conditionally loaded based on which files changed in a PR: Always loaded: - repo-conventions.md: repo-specific patterns (ProcessUtils, FileUtil, etc.) - 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) Updated skill.md step 5 to describe the conditional loading logic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- Add back "No HttpClient injection (YAGNI)" rule to csharp-rules.md - Restore netstandard2.0 fallback example in ThrowIf rule - Restore "recommend creating one" guidance in null-object pattern - Restore IsNullOrEmpty/null-coalescing guidance and logical negation examples in null-forgiving operator rule Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

New rules added to csharp-rules.md from dotnet/android PR #11266: Async section (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 a value in an interpolated string - 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 Also: - Fixed ThrowIf rule to drop specific version number - Added "concurrent calls" to testing edge cases Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Cross-referenced with dotnet/android-tools#355 to align shared rules and adopt new relevant ones. New rules added (6): - ai-pitfalls: Confidently wrong domain facts - ai-pitfalls: Over-mocking - ai-pitfalls: Debug.WriteLine for logging - msbuild-rules: Use appropriate log levels (MessageImportance) - msbuild-rules: Don't redirect stdout/stderr without draining - msbuild-rules: Include stdout in error diagnostics Wording aligned with android-tools for sharing: - security: Zip Slip adds ZipFile.ExtractToDirectory() warning - security: Path traversal adds directory boundary example - security: Command injection adds "never interpolate" note - security: Reorganized into Archive & Process subsections - ai-pitfalls: Reinventing the wheel adds "hundreds of lines" emphasis - ai-pitfalls: Swallowed errors adds exit code checking - ai-pitfalls: Null-forgiving expanded with IsNullOrEmpty suggestion - ai-pitfalls: Unused parameters adds injection risk note - testing: Bug fixes uses generic "#N" placeholder - testing: Assertions adds NUnit constraint examples Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers · 2026-05-01T21:32:39Z

/review

github-actions · 2026-05-01T21:33:18Z

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

- security-rules.md: Remove Process.Start/ArgumentList references since this is a shareable file; repo-specific ProcessUtils guidance stays in repo-conventions.md - SKILL.md: Replace vague "MSBuild task implementations" with concrete path check: src/Microsoft.Android.Build.BaseTasks/ - android-tools-reviewer.md workflow: Update to reference SKILL.md (which has conditional loading logic) instead of deleted review-rules.md; fix step numbering Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…11266) * Split android-reviewer review-rules.md into conditional rule files Split the monolithic review-rules.md (297 lines) into 8 focused files that the skill loads conditionally based on which file types changed in the PR: Always loaded: - repo-conventions.md: formatting, style, localization, repo patterns - ai-pitfalls.md: common AI-generated code mistakes Conditionally loaded: - csharp-rules.md: nullable, async, error handling, performance, code org - msbuild-rules.md: MSBuild task conventions and target/XML rules - native-rules.md: C/C++ memory, best practices, symbols, platform code - interop-rules.md: managed-native boundary (P/Invoke, JNI, marshalling) - testing-rules.md: test conventions and best practices - security-rules.md: zip slip, command injection, path traversal Updated SKILL.md step 5 to describe the conditional loading logic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Broaden test file detection to include src/**/Tests/ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Improve interop-rules.md loading: trigger on diff content, not just file presence The previous heuristic loaded interop rules only when both C# and native files changed. This missed cases where a C#-only diff touches JNI/P/Invoke boundary code (e.g., DllImport, [Register], JNIEnv, [MarshalAs], [StructLayout]). Now triggers based on actual interop markers in the diff. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Align rule wording with dotnet/android-tools and add 6 new rules Cross-referenced with dotnet/android-tools#355 to align shared rules and adopt new relevant ones. New rules added (6): - ai-pitfalls: Confidently wrong domain facts - ai-pitfalls: Over-mocking - ai-pitfalls: Debug.WriteLine for logging - msbuild-rules: Use appropriate log levels (MessageImportance) - msbuild-rules: Don't redirect stdout/stderr without draining - msbuild-rules: Include stdout in error diagnostics Wording aligned with android-tools for sharing: - security: Zip Slip adds ZipFile.ExtractToDirectory() warning - security: Path traversal adds directory boundary example - security: Command injection adds "never interpolate" note - security: Reorganized into Archive & Process subsections - ai-pitfalls: Reinventing the wheel adds "hundreds of lines" emphasis - ai-pitfalls: Swallowed errors adds exit code checking - ai-pitfalls: Null-forgiving expanded with IsNullOrEmpty suggestion - ai-pitfalls: Unused parameters adds injection risk note - testing: Bug fixes uses generic "#N" placeholder - testing: Assertions adds NUnit constraint examples Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback: update workflow and align interop-rules header - android-reviewer.md: Remove reference to deleted review-rules.md, point to SKILL.md which handles rule file loading (step 5) - interop-rules.md: Align header load-condition wording with SKILL.md to mention interop markers (DllImport, [Register], etc.) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Deduplicate null-forgiving rule between ai-pitfalls and csharp-rules Keep ai-pitfalls.md concise with a cross-reference to csharp-rules.md for the full NRT details. Saves ~500 tokens of duplicate context since ai-pitfalls is always loaded and csharp-rules is loaded for most PRs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Address review feedback: - Remove incorrect UTF-8 string literal guidance (ReadOnlySpan<byte> exists on netstandard2.0 via System.Memory) - Remove "No string intermediates in protocols" rule (overly prescriptive) - Remove "No stackalloc in async I/O" rule (compiler enforces this) - Remove Encoding.ASCII recommendation (wrong for non-ASCII content) - Move valid rules to split files (csharp-rules.md) after PR #355 split - Keep: buffer reuse, thread-safety docs, loop helper reuse, prefer existing repo patterns Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Adds performance and allocation review learnings from PR #327 to the reviewer skill and Copilot instructions. ## Changes **`.github/copilot-instructions.md`** - Add **Reuse buffers** guidance (`ArrayPool<byte>.Shared` + reusable `readonly byte[]` fields) - Add **Document thread-safety** guidance (`<remarks>This class is not thread-safe.</remarks>`) **`.github/skills/android-tools-reviewer/SKILL.md`** - Add "Prefer existing repo patterns" workflow step — grep for `ArrayPool`, `ObjectPool`, `MemoryStreamPool`, `ProcessUtils` before suggesting new infrastructure **`.github/skills/android-tools-reviewer/references/csharp-rules.md`** - Add **Reuse hot-path buffers** and **Reuse loop helpers** to the Performance section - Add **Document thread-safety invariants** to the Code Organization section ## Removed from original PR (per review feedback) - UTF-8 string literal guidance — `ReadOnlySpan<byte>` works on netstandard2.0 via `System.Memory` - `Encoding.ASCII.GetBytes()` recommendation — wrong for non-ASCII content - "No string intermediates in protocols" — overly prescriptive - "No stackalloc in async I/O" — already a compiler error, not a review concern Also rebased onto main to resolve merge conflicts from PR #355 (which split `review-rules.md` into per-category files). Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers and others added 3 commits May 1, 2026 16:07

jonathanpeppers mentioned this pull request May 1, 2026

Split android-reviewer review-rules.md into conditional rule files dotnet/android#11266

Merged

jonathanpeppers marked this pull request as ready for review May 1, 2026 21:32

jonathanpeppers requested a review from simonrozsival as a code owner May 1, 2026 21:32

Copilot AI review requested due to automatic review settings May 1, 2026 21:32

Copilot started reviewing on behalf of jonathanpeppers May 1, 2026 21:33 View session

Copilot AI reviewed May 1, 2026

View reviewed changes

Comment thread .github/skills/android-tools-reviewer/references/security-rules.md Outdated

Comment thread .github/skills/android-tools-reviewer/SKILL.md

Comment thread .github/skills/android-tools-reviewer/SKILL.md Outdated

github-actions Bot reviewed May 1, 2026

View reviewed changes

Comment thread .github/skills/android-tools-reviewer/references/repo-conventions.md

Comment thread .github/skills/android-tools-reviewer/SKILL.md

Comment thread .github/skills/android-tools-reviewer/references/csharp-rules.md

jonathanpeppers mentioned this pull request May 1, 2026

Add code-reviewer skill with conditional rule files jonathanpeppers/dotnes#482

Merged

Add postmortem provenance comment to repo-conventions.md

fa1bc70

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers added the ready-to-review This PR is ready to review/merge. label May 1, 2026

simonrozsival approved these changes May 4, 2026

View reviewed changes

jonathanpeppers merged commit c624ad0 into main May 4, 2026
2 checks passed

jonathanpeppers deleted the jonathanpeppers/split-reviewer-rules branch May 4, 2026 14:19

jonathanpeppers mentioned this pull request May 4, 2026

Add performance and allocation review learnings from PR #327 #354

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split reviewer skill rules into conditional files#355

Split reviewer skill rules into conditional files#355
jonathanpeppers merged 5 commits intomainfrom
jonathanpeppers/split-reviewer-rules

jonathanpeppers commented May 1, 2026 •

edited

Loading

Uh oh!

jonathanpeppers commented May 1, 2026

Uh oh!

github-actions Bot commented May 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jonathanpeppers commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New file structure

Token usage by PR type

14 new rules from dotnet/android#11266

Evaluation against 3 merged PRs

All 96 rules verified

Uh oh!

jonathanpeppers commented May 1, 2026

Uh oh!

github-actions Bot commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

✅ LGTM

What works well

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonathanpeppers commented May 1, 2026 •

edited

Loading

github-actions Bot commented May 1, 2026 •

edited

Loading