Repository navigation
[skills] Add android-reviewer skill for PR code reviews #10918
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
2e01999
[skills] Add android-reviewer skill for PR code reviews
jonathanpeppers c2b305e
[skills] Incorporate MSBuild best practices into android-reviewer rules
jonathanpeppers 04ee065
[skills] Add CODE_REVIEW_POST_MORTEM.md from @jonpryor reviews
jonathanpeppers 10f37c2
[skills] Incorporate CODE_REVIEW_POST_MORTEM learnings into reviewer …
jonathanpeppers 1bf1dd8
[skills] Integrate dotnet/runtime code review patterns into android-r…
jonathanpeppers 5195217
[skills] Reduce android-reviewer frontmatter to minimize token usage
jonathanpeppers 5b3048d
Address code review feedback
jonathanpeppers eb37e1f
Restructure success output so catch block isn't misleading
jonathanpeppers c10b729
Fix broken review template link and silent catch block
jonathanpeppers 49811e3
Address Copilot PR reviewer feedback
jonathanpeppers 062d57f
Clarify null-forgiving rule applies to postfix ! not logical negation
jonathanpeppers 1877bd0
Add CI check step to reviewer workflow — never LGTM if CI fails
jonathanpeppers 8b4b55f
Reference azdo-build-investigator skill for CI failure investigation
jonathanpeppers File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| --- | ||
| name: android-reviewer | ||
| description: >- | ||
| Review dotnet/android PRs against established rules. Trigger on "review this PR", | ||
| a GitHub PR URL, or code review requests. Checks MSBuild, nullable, async, security, | ||
| error handling, formatting, performance, native code, and posts a review via gh CLI. | ||
| --- | ||
|
|
||
| # Android PR Reviewer | ||
|
|
||
| Review PRs against guidelines distilled from past reviews by senior maintainers of dotnet/android. | ||
|
|
||
| ## Review Mindset | ||
|
|
||
| Be polite but skeptical. Prioritize bugs, performance regressions, safety issues, and pattern violations over style nitpicks. **3 important comments > 15 nitpicks.** | ||
|
|
||
| Flag severity clearly in every comment: | ||
| - ❌ **error** — Must fix before merge. Bugs, security issues, missing error codes, broken incremental builds. | ||
| - ⚠️ **warning** — Should fix. Performance issues, missing validation, inconsistency with patterns. | ||
| - 💡 **suggestion** — Consider changing. Style, readability, optional improvements. | ||
|
|
||
| ## Workflow | ||
|
|
||
| ### 1. Parse the PR URL | ||
|
|
||
| Extract `owner`, `repo`, `pr_number` from the URL. | ||
| Formats: `https://github.com/{owner}/{repo}/pull/{number}`, `{owner}/{repo}#{number}`, or bare number (defaults to `dotnet/android`). | ||
|
|
||
| ### 2. Gather context (before reading PR description) | ||
|
|
||
| ``` | ||
| gh pr diff {number} --repo {owner}/{repo} | ||
| gh pr view {number} --repo {owner}/{repo} --json files | ||
| ``` | ||
|
|
||
| For each changed file, read the **full source file** (not just the diff) to understand surrounding invariants, call patterns, and data flow. If the change modifies a public/internal API or utility, search for callers. Check whether sibling types need the same fix. | ||
|
|
||
| **Form an independent assessment** of what the change does and what problems it has *before* reading the PR description. | ||
|
|
||
| ### 3. Incorporate PR narrative and reconcile | ||
|
|
||
| ``` | ||
| gh pr view {number} --repo {owner}/{repo} --json title,body | ||
| ``` | ||
|
|
||
| Now read the PR description and linked issues. Treat them as claims to verify, not facts to accept. Where your independent reading disagrees with the PR description, investigate further. If the PR claims a performance improvement, require evidence (benchmarks, profiling data). If it claims a bug fix, verify the bug exists and the fix addresses root cause — not symptoms. | ||
|
|
||
| ### 4. Check CI status | ||
|
|
||
| ``` | ||
| gh pr checks {number} --repo {owner}/{repo} | ||
| ``` | ||
|
|
||
| Review the CI results. **Never post ✅ LGTM if any required CI check is failing or if the code doesn't build.** If CI is failing: | ||
| - Investigate the failure using the **azdo-build-investigator** skill (for Azure DevOps pipeline failures) or GitHub Actions job logs. | ||
| - If the failure is caused by the PR's code changes, flag it as ❌ error. | ||
| - If the failure is a known infrastructure issue or pre-existing flake unrelated to the PR, note it in the summary but still use ⚠️ Needs Changes — the PR isn't mergeable until CI is green. | ||
| - If the PR description acknowledges the failure and documents a dependency (e.g., "blocked on X"), note it in the summary. | ||
|
|
||
| ### 5. Load review rules | ||
|
|
||
| Read `references/review-rules.md` from this skill's directory. | ||
|
|
||
| ### 6. Analyze the diff | ||
|
|
||
| For each changed file, check against the review rules. Record issues as: | ||
|
|
||
| ```json | ||
| { "path": "src/Example.cs", "line": 42, "side": "RIGHT", "body": "..." } | ||
| ``` | ||
|
|
||
| Constraints: | ||
| - Only comment on added/modified lines in the diff — the API rejects out-of-range lines. | ||
| - `line` = line number in the NEW file (right side). Double-check against the diff. | ||
| - One issue per comment. | ||
| - **Don't pile on.** If the same issue appears many times, flag it once with a note listing all affected files. | ||
| - **Don't flag what CI catches.** Skip compiler errors, formatting the linter will catch, etc. | ||
| - **Avoid false positives.** Verify the concern actually applies given the full context. If unsure, phrase it as a question rather than a firm claim. | ||
|
|
||
| ### 7. Build the review JSON | ||
|
|
||
| Write a temp JSON file: | ||
|
|
||
| ```json | ||
| { | ||
| "event": "COMMENT", | ||
| "body": "## 🤖 AI Review Summary\n\n**Verdict**: ✅ LGTM | ⚠️ Needs Changes | ❌ Reject\n\nFound **N issues**: ...\n\n- ❌ **Category**: description (`file:line`)\n- ⚠️ **Category**: description (`file:line`)\n\n👍 Positive callouts.\n\n---\n_Review generated by android-reviewer from [review guidelines](https://github.com/dotnet/android/blob/main/.github/skills/android-reviewer/references/review-rules.md)._", | ||
| "comments": [ | ||
| { | ||
| "path": "src/Example.cs", | ||
| "line": 42, | ||
| "side": "RIGHT", | ||
| "body": "🤖 ❌ **Error handling** — Every `catch` should capture the `Exception` and log it.\n\n_Rule: No empty catch blocks (Postmortem `#11`)_" | ||
| } | ||
| ] | ||
| } | ||
| ``` | ||
|
|
||
| If no issues found **and CI is green**, submit with empty `comments` and a positive summary. | ||
|
|
||
| ### 8. Submit as a single batch | ||
|
|
||
| ```powershell | ||
| dotnet run {skill-dir}/scripts/submit_review.cs {owner} {repo} {number} {path-to-json} | ||
| ``` | ||
|
|
||
| The script validates structure (required fields, 🤖 prefix, positive line numbers) then calls `gh api`. Delete the temp file after success. | ||
|
|
||
| ## Comment format | ||
|
|
||
| ``` | ||
| 🤖 {severity} **{Category}** — {What's wrong and what to do instead.} | ||
|
|
||
| _{Rule: Brief name (Postmortem `#N`)}_ | ||
| ``` | ||
|
|
||
| Where `{severity}` is ❌, ⚠️, or 💡. Always wrap `#N` in backticks so GitHub doesn't auto-link to issues. | ||
|
|
||
| **Categories:** MSBuild tasks · MSBuild targets · Nullable · Async pattern · Error handling · Resource management · Security · Formatting · Performance · Code organization · Patterns · Native memory · Native C++ · Symbol visibility · Platform-specific · Testing · YAGNI · API design · Documentation | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.