# Code Review

> 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, JNI interop, generator codegen, and trimmer/AOT compatibility.

- Skill: `dotnet/code-review-3` (Agent Skill, multi-file: 11 files)
- Install (CLI): `npx skillmds@latest add dotnet/code-review-3`
- Raw SKILL.md: https://api.skillmd.com/api/skills/dotnet/code-review-3/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Security, Coding & Dev Tools
- Author: .NET (Microsoft) (https://skillmd.com/u/dotnet)
- Updated: 2026-09-22
- Page: https://skillmd.com/skills/dotnet/code-review-3

---


# Android PR Reviewer

Review PRs against guidelines distilled from past reviews by senior maintainers of dotnet/android, including the in-tree shared Android tooling under `src/`.

## 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, JNI reference leaks, broken codegen.
- ⚠️ **warning** — Should fix. Performance issues, missing validation, inconsistency with patterns.
- 💡 **suggestion** — Consider changing. Style, readability, optional improvements.

**Every review should produce at least one inline comment.** Even clean PRs have opportunities for improvement — code consolidation, missing edge-case tests, perf micro-optimizations, or documentation gaps. Use 💡 suggestions for these. A review with zero inline comments appears superficial and misses the chance to share knowledge. Only omit inline comments if the PR is truly trivial (e.g., a 1-line typo fix or dependency bump). **Do NOT summarize suggestions only in the review body — post them as inline comments on the relevant line.** If a suggestion cannot be posted inline (e.g., it's about pre-existing code or missing code), either find the closest relevant changed line to attach it to, or omit it entirely rather than burying it in the summary.

## Workflow

### 1. Identify the PR

If triggered from an agentic workflow (slash command on a PR), use the PR from the event context. Otherwise, extract `owner`, `repo`, `pr_number` from a URL or reference provided by the user.
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)

Use the GitHub `pull_request_read` tool:

- `method: get_diff` to read the diff
- `method: get_files` with `perPage: 100` to list changed files; increment `page` until a page returns fewer than 100 files

When the GitHub MCP tool is unavailable, fall back to:

```
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

Use `pull_request_read` with `method: get` to read the title and description. When the GitHub MCP tool is unavailable, fall back to:

```
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

Use `pull_request_read` with `method: get_check_runs` and `perPage: 100` to read CI checks. Increment `page` until a page returns fewer than 100 check runs so failures are not missed. When the GitHub MCP tool is unavailable, fall back to:

```
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

Based on the changed files and diff content identified in step 2, read the appropriate rule files from this skill's `references/` directory.

**Always load:**
- `references/repo-conventions.md` — Formatting, style, and patterns specific to this repository.
- `references/ai-pitfalls.md` — Common AI-generated code mistakes.

**Conditionally load based on changed file types:**
- `references/csharp-rules.md` — When any `.cs` files changed. Covers nullable, async, error handling, performance, and code organization.
- `references/msbuild-rules.md` — When `.targets`, `.props`, `.projitems`, or `.csproj` files changed, or when MSBuild task C# files changed (e.g., files under `src/Xamarin.Android.Build.Tasks/` or `src/Microsoft.Android.Build.BaseTasks/`).
- `references/native-rules.md` — When `.c`, `.cpp`, `.h`, or `.hpp` files changed. Covers memory management, C++ best practices, symbol visibility, and platform-specific code.
- `references/interop-rules.md` — When interop markers or Java/JNI name conversions appear, or files under `external/Java.Interop/`, `src/Mono.Android/`, `src/native/`, or `src/Microsoft.Android.Sdk.TrimmableTypeMap/` change.
- `references/testing-rules.md` — When test files changed (e.g., files under `tests/`, `**/Tests/`, or test project directories).
- `references/security-rules.md` — When any code files changed (C#, C/C++, or MSBuild).

### 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": "..." }
```

**What to look for (in priority order):**
1. **Bugs & correctness** — race conditions, null dereferences, off-by-one, logic errors, JNI reference leaks
2. **Safety** — thread safety, resource leaks, security vulnerabilities
3. **Performance** — O(n²) patterns, unnecessary allocations, missing caches, startup-time regressions
4. **Trimmer/NativeAOT compatibility** — reflection without `[DynamicallyAccessedMembers]`, `Type.GetType()` misuse
5. **Missing tests** — untested error paths, edge cases, missing regression tests for bug fixes
6. **Code duplication** — near-identical methods that should be consolidated
7. **Consistency** — dedup patterns mixed within the same PR, API return types inconsistent with repo conventions
8. **Documentation** — misleading comments, undocumented behavioral decisions

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.
- **Verify project context before applying rules.** Some rules are scoped to specific project types (e.g., `netstandard2.0` vs modern .NET, MSBuild tasks vs console apps). Check the project's `TargetFramework`, references, and available utilities before flagging a rule violation. A rule about an extension method is irrelevant if the project doesn't reference the assembly that defines it.

### 7. Post the review

Post your findings directly:

- **Inline comments** on specific lines of the diff with the severity, category, and explanation.
- **Review summary** with the overall verdict (✅ LGTM, ⚠️ Needs Changes, or ❌ Reject), issue counts by severity, and positive callouts.

If no issues found **and CI is green**, submit with at most one or two 💡 suggestions and a positive summary. Truly trivial PRs (dependency bumps, 1-line typo fixes) may have no inline comments.

**Copilot-authored PRs:** If the PR author is `Copilot` (the GitHub Copilot coding agent) and the verdict is ⚠️ Needs Changes or ❌ Reject, prefix the review summary with `@copilot ` so the comment automatically triggers Copilot to address the feedback. Do NOT add the prefix for ✅ LGTM verdicts.

## 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 · Naming · Native memory · Native C++ · Symbol visibility · Platform-specific · JNI interop · JNI references · Generator codegen · Trimmer/AOT · Testing · YAGNI · API design · Documentation