Split reviewer skill rules into conditional files#355
Open
jonathanpeppers wants to merge 5 commits intomainfrom
Open
Split reviewer skill rules into conditional files#355jonathanpeppers wants to merge 5 commits intomainfrom
jonathanpeppers wants to merge 5 commits intomainfrom
Conversation
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>
jonathanpeppers
added a commit
to dotnet/android
that referenced
this pull request
May 1, 2026
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>
Member
Author
|
/review |
|
✅ Android Tools PR Reviewer completed successfully! |
Contributor
There was a problem hiding this comment.
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.mdwith 6 focused rule files (repo-conventions,ai-pitfalls,security, plus conditionalcsharp,msbuild,testing). - Updated
SKILL.mdto 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[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 mistakessecurity-rules.md— archive, path, and command safetyConditionally loaded:
csharp-rules.md— general C# guidance (when.csfiles change)msbuild-rules.md— MSBuild task guidance (when.targets/.propschange)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:
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.mdto align with the android repo's split:Async (3): Lock ordering, avoid double-checked locking (prefer
Lazy<T>), singleton initialization completenessError 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):
sealedclasses skip full Dispose, use interfaces over concrete typesEvaluation against 3 merged PRs
Simulated reviews with old vs new rules on PRs #337, #338, #344:
Lazy<T>recommendationKey findings:
HashSetusage,outparameters, or throw-heavy hot pathscsharp-rules.md,security-rules.md,ai-pitfalls.md,msbuild-rules.md, andtesting-rules.mdare 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.