Split android-reviewer review-rules.md into conditional rule files#11266
Open
jonathanpeppers wants to merge 6 commits intomainfrom
Open
Split android-reviewer review-rules.md into conditional rule files#11266jonathanpeppers wants to merge 6 commits intomainfrom
jonathanpeppers wants to merge 6 commits intomainfrom
Conversation
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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ile 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>
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 PR Reviewer completed successfully! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the android-reviewer skill’s rule reference material by replacing the single monolithic review-rules.md with multiple focused rule files intended to be loaded conditionally based on what a PR changes.
Changes:
- Delete the legacy monolithic
references/review-rules.md. - Add 8 focused rule reference files (repo conventions, AI pitfalls, and per-area rule sets like C#, MSBuild, native, interop, testing, security).
- Update
SKILL.mdto describe the new always/conditional rule-loading behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/skills/android-reviewer/SKILL.md | Updates step 5 to load the new split rule files (always + conditional sets). |
| .github/skills/android-reviewer/references/review-rules.md | Removes the legacy monolithic rule file. |
| .github/skills/android-reviewer/references/repo-conventions.md | Adds always-loaded repo-specific conventions (formatting/localization/patterns). |
| .github/skills/android-reviewer/references/ai-pitfalls.md | Adds always-loaded AI pitfall guidance. |
| .github/skills/android-reviewer/references/csharp-rules.md | Adds conditional C# rules (nullable/async/error handling/perf/organization). |
| .github/skills/android-reviewer/references/msbuild-rules.md | Adds conditional MSBuild rules (tasks/targets/process management). |
| .github/skills/android-reviewer/references/native-rules.md | Adds conditional native rules (memory/C++/visibility/platform). |
| .github/skills/android-reviewer/references/interop-rules.md | Adds conditional interop boundary rules (P/Invoke/JNI/shared structs). |
| .github/skills/android-reviewer/references/testing-rules.md | Adds conditional test guidance. |
| .github/skills/android-reviewer/references/security-rules.md | Adds conditional security checks (zip slip/path traversal/command injection). |
- 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>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ LGTM
Well-structured refactoring that splits the monolithic 297-line review-rules.md into 8 focused files with conditional loading. Verified:
- 0 rules lost — All 150 original rules present across the 8 new files
- 6 new rules added — Domain facts, over-mocking, logging patterns, process management (matches PR description)
- 8 enhanced rules — Security rules (Zip Slip, path traversal, command injection) meaningfully improved with concrete examples and additional guidance
- SKILL.md updated correctly — Conditional loading instructions match the actual file set
- Postmortem references — All consistently backtick-wrapped (
#N) to prevent GitHub auto-linking - CI is green — All 6 checks passing
Positive callouts:
- The conditional loading strategy (21-33% savings per the evaluation) is a pragmatic way to reduce context for the AI reviewer without losing coverage
- Grouping security rules into "Archive & Path Safety" and "Process & Command Safety" subsections improves scannability
- Generalizing
#1234→#Nin testing-rules.md makes the rules portable across repos
1 suggestion filed inline about reducing duplication between always-loaded and conditionally-loaded null-forgiving operator rules.
Generated by Android PR Reviewer for issue #11266 · ● 11M
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>
jonathanpeppers
added
the
ready-to-review
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(297 lines, 150 rules) 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-specific patternsai-pitfalls.md— common AI-generated code mistakesConditionally loaded:
csharp-rules.md(46 rules) — nullable, async, error handling, performance, code organization — when.csfiles changedmsbuild-rules.md(20 rules) — MSBuild task conventions, process management, and target/XML rules — when.targets/.props/.csprojchangednative-rules.md(27 rules) — memory management, C++ best practices, symbol visibility, platform-specific code — when.c/.cpp/.h/.hppchangedinterop-rules.md(5 rules) — managed-native boundary (P/Invoke, JNI, marshalling) — when diff contains interop markers (DllImport,[Register],JNIEnv,[MarshalAs],[StructLayout])testing-rules.md(7 rules) — test conventions and best practices — whentests/or**/Tests/files changedsecurity-rules.md(3 rules) — zip slip, command injection, path traversal — when any code files changedDeep Audit
All 150 original rules verified preserved via word-for-word content comparison:
6 new rules (from cross-referencing with dotnet/android-tools#355)
Debug.WriteLinefor logging8 intentionally modified rules (aligned wording with android-tools)
ZipFile.ExtractToDirectory()warning#1234→#N(generalized for sharing)Does.Contain,Is.EqualTo)Evaluation
Tested the split against 3 real merged PRs with different file-type profiles:
Key findings:
DllImport,[Register]) rather than just file presence