Skip to content

Commit 8e27f60

Browse files
[skills] Add android-reviewer skill for PR code reviews (#10918)
Add a Copilot skill that reviews dotnet/android PRs against established conventions, and a post-mortem document capturing the review knowledge it's grounded in. **New files:** - `.github/skills/android-reviewer/SKILL.md` — Skill definition with a 7-step workflow: parse PR URL, gather context (read full source files, not just diffs), form an independent assessment, incorporate the PR narrative, load review rules, analyze the diff, and submit a batched review via `gh api`. - `.github/skills/android-reviewer/references/review-rules.md` — 14 categories of review checks (MSBuild tasks, MSBuild targets, nullable reference types, formatting, async/cancellation, error handling, resources/localization, security, performance, code organization, patterns, native C/C++, testing, AI-specific pitfalls). - `.github/skills/android-reviewer/scripts/submit_review.cs` — A .NET file-based program that validates review JSON structure and submits it as a single batched PR review via `gh api`. - `docs/CODE_REVIEW_POST_MORTEM.md` — 71 findings distilled from ~170 review comments across 5 PRs. **Sources:** Ported from [dotnet/android-tools android-tools-reviewer skill](https://github.com/dotnet/android-tools/tree/main/.github/skills/android-tools-reviewer), adapted for dotnet/android conventions: MSBuild task patterns (`AndroidTask`, `XA` error codes), nullable reference types, Mono formatting style, localization rules, and repo-specific AI pitfalls. Integrated 25 additional review checks from [dotnet/runtime's code review skill](https://github.com/dotnet/runtime), including: review mindset with severity levels, `[DoesNotReturn]` throw helpers, pre-allocate collections, avoid closures in hot paths, O(n²) detection, thread safety of shared state, lock ordering, `static_cast`/`nullptr` for native interop, bool/string marshalling, regression tests for bug fixes, and the "assess independently before reading PR description" workflow pattern. Post-mortem findings from reviews by @jonpryor: - #2515 - #3992 - #4914 - #5748 - #6427
1 parent f715c14 commit 8e27f60

4 files changed

Lines changed: 758 additions & 0 deletions

File tree

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
---
2+
name: android-reviewer
3+
description: >-
4+
Review dotnet/android PRs against established rules. Trigger on "review this PR",
5+
a GitHub PR URL, or code review requests. Checks MSBuild, nullable, async, security,
6+
error handling, formatting, performance, native code, and posts a review via gh CLI.
7+
---
8+
9+
# Android PR Reviewer
10+
11+
Review PRs against guidelines distilled from past reviews by senior maintainers of dotnet/android.
12+
13+
## Review Mindset
14+
15+
Be polite but skeptical. Prioritize bugs, performance regressions, safety issues, and pattern violations over style nitpicks. **3 important comments > 15 nitpicks.**
16+
17+
Flag severity clearly in every comment:
18+
-**error** — Must fix before merge. Bugs, security issues, missing error codes, broken incremental builds.
19+
- ⚠️ **warning** — Should fix. Performance issues, missing validation, inconsistency with patterns.
20+
- 💡 **suggestion** — Consider changing. Style, readability, optional improvements.
21+
22+
## Workflow
23+
24+
### 1. Parse the PR URL
25+
26+
Extract `owner`, `repo`, `pr_number` from the URL.
27+
Formats: `https://github.com/{owner}/{repo}/pull/{number}`, `{owner}/{repo}#{number}`, or bare number (defaults to `dotnet/android`).
28+
29+
### 2. Gather context (before reading PR description)
30+
31+
```
32+
gh pr diff {number} --repo {owner}/{repo}
33+
gh pr view {number} --repo {owner}/{repo} --json files
34+
```
35+
36+
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.
37+
38+
**Form an independent assessment** of what the change does and what problems it has *before* reading the PR description.
39+
40+
### 3. Incorporate PR narrative and reconcile
41+
42+
```
43+
gh pr view {number} --repo {owner}/{repo} --json title,body
44+
```
45+
46+
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.
47+
48+
### 4. Check CI status
49+
50+
```
51+
gh pr checks {number} --repo {owner}/{repo}
52+
```
53+
54+
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:
55+
- Investigate the failure using the **azdo-build-investigator** skill (for Azure DevOps pipeline failures) or GitHub Actions job logs.
56+
- If the failure is caused by the PR's code changes, flag it as ❌ error.
57+
- 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.
58+
- If the PR description acknowledges the failure and documents a dependency (e.g., "blocked on X"), note it in the summary.
59+
60+
### 5. Load review rules
61+
62+
Read `references/review-rules.md` from this skill's directory.
63+
64+
### 6. Analyze the diff
65+
66+
For each changed file, check against the review rules. Record issues as:
67+
68+
```json
69+
{ "path": "src/Example.cs", "line": 42, "side": "RIGHT", "body": "..." }
70+
```
71+
72+
Constraints:
73+
- Only comment on added/modified lines in the diff — the API rejects out-of-range lines.
74+
- `line` = line number in the NEW file (right side). Double-check against the diff.
75+
- One issue per comment.
76+
- **Don't pile on.** If the same issue appears many times, flag it once with a note listing all affected files.
77+
- **Don't flag what CI catches.** Skip compiler errors, formatting the linter will catch, etc.
78+
- **Avoid false positives.** Verify the concern actually applies given the full context. If unsure, phrase it as a question rather than a firm claim.
79+
80+
### 7. Build the review JSON
81+
82+
Write a temp JSON file:
83+
84+
```json
85+
{
86+
"event": "COMMENT",
87+
"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)._",
88+
"comments": [
89+
{
90+
"path": "src/Example.cs",
91+
"line": 42,
92+
"side": "RIGHT",
93+
"body": "🤖 ❌ **Error handling** — Every `catch` should capture the `Exception` and log it.\n\n_Rule: No empty catch blocks (Postmortem `#11`)_"
94+
}
95+
]
96+
}
97+
```
98+
99+
If no issues found **and CI is green**, submit with empty `comments` and a positive summary.
100+
101+
### 8. Submit as a single batch
102+
103+
```powershell
104+
dotnet run {skill-dir}/scripts/submit_review.cs {owner} {repo} {number} {path-to-json}
105+
```
106+
107+
The script validates structure (required fields, 🤖 prefix, positive line numbers) then calls `gh api`. Delete the temp file after success.
108+
109+
## Comment format
110+
111+
```
112+
🤖 {severity} **{Category}** — {What's wrong and what to do instead.}
113+
114+
_{Rule: Brief name (Postmortem `#N`)}_
115+
```
116+
117+
Where `{severity}` is ❌, ⚠️, or 💡. Always wrap `#N` in backticks so GitHub doesn't auto-link to issues.
118+
119+
**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

0 commit comments

Comments
 (0)