[skills] Add android-reviewer skill for PR code reviews by jonathanpeppers · Pull Request #10918 · dotnet/android

jonathanpeppers · 2026-03-11T18:01:44Z

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,
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, 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:

Ported from dotnet/android-tools android-tools-reviewer skill, 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. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Added section 2 (MSBuild Targets & XML) covering: - Underscore prefix naming for private targets/properties/items - Incremental builds with Inputs/Outputs - Stamp files in \ - FileWrites and IncrementalClean pitfalls - Item group transform deduplication - ->Count() for empty item group checks - BeforeTargets/AfterTargets vs DependsOn - XML 2-space indentation Expanded section 1 (MSBuild Task Conventions) with: - AsyncTask for background work (RunTaskAsync, WhenAll, Log* helpers) - RegisterTaskObject caching via RegisterTaskObjectAssemblyLocal() Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@jonpryor

Distills 170 review comments from @jonpryor across 5 PRs into 71 actionable findings across 16 categories: naming, error messages, memory management, C++ best practices, platform/API usage, symbol visibility, formatting, conditional compilation, performance, boundary validation, string comparison, MSBuild tasks, code organization, code cleanliness, architecture, and docs. PRs reviewed: #4914, #6427, #2515, #3992, #5748 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…rules Major expansion of native code rules (section 12) with 5 subsections: - 12a: Memory management (RAII, leak quantification, stack sizes) - 12b: C++ best practices (virtual dtors, =delete, const, EINTR, sizeof) - 12c: Symbol visibility (fvisibility=hidden, document callers, remove dead) - 12d: Platform-specific (W functions, return code checks) - 12e: Build & ABI Also incorporated postmortem findings into existing sections: - Error handling: contextual log messages, boundary assertions - Performance: HashSet.Add, CopyIfStringChanged, .Ordinal, Split count - Code organization: centralize algorithms, interfaces over concrete types - Patterns: collision-proof names, track TODOs, stale comments, vendored source - Formatting: #else/#endif comments, braces outside #if, line width - YAGNI: commit message content, typos, filler words Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…eviewer Adapted applicable patterns from dotnet/runtime's code review SKILL.md: SKILL.md changes: - Add Review Mindset section with severity levels (error/warning/suggestion) - Restructure workflow: gather context and form independent assessment BEFORE reading PR description (Steps 2-3) - Add guidance to treat PR claims as things to verify, not accept - Add constraints: don't pile on, don't flag what CI catches, avoid false positives - Add verdict line to review summary (LGTM/Needs Changes/Reject) review-rules.md additions (25 new checks): - Error handling: ThrowIf helpers, actionable exceptions, output param initialization, challenge exception swallowing - Async: thread safety of shared state, lock ordering - Performance: pre-allocate collections, avoid closures in hot paths, cheap checks first, cache accessor calls, O(n^2) detection, extract throw helpers with [DoesNotReturn] - Code organization: don't initialize to defaults, sealed Dispose, named constants over magic numbers - Testing: regression tests for bug fixes, specific assertions, deterministic test data, edge case coverage - Native interop: static_cast, nullptr, struct padding, bool/string marshalling - Patterns: comments explain why not what Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers

🤖 AI Review Summary

Verdict: ⚠️ Needs Changes

Found 4 issues: 1 warning, 3 suggestions.

⚠️ Nullable: Process.Start() can return null — ! suppresses a real warning (submit_review.cs:106)
💡 Nullable: Multiple GetString()! uses are banned per repo convention (submit_review.cs:46, :68, :72)
💡 Error handling: Bare catch blocks don't capture the exception (submit_review.cs:122, :135)
⚠️ Documentation: Section numbering in review-rules.md skips from 12 → 14 (no section 13) (review-rules.md:261)

👍 Positives:

CODE_REVIEW_POST_MORTEM.md is thorough and well-sourced — every entry links back to the actual PR comment
Review rules comprehensively cover MSBuild, nullable, security, native code, and AI-specific pitfalls
submit_review.cs correctly uses ArgumentList (not string interpolation) for process arguments — practicing what the rules preach
The SKILL.md workflow wisely mandates forming an independent assessment before reading the PR description

Review generated by android-reviewer from review guidelines.

.github/skills/android-reviewer/scripts/submit_review.cs

.github/skills/android-reviewer/references/review-rules.md

- Remove null-forgiving operator (!) from GetString() calls, use ?? "" instead - Add proper null check for Process.Start() which can return null - Replace bare catch blocks with catch (JsonException) to avoid swallowing unexpected exceptions - Fix section numbering in review-rules.md: 12 -> 13 (Testing), 13 -> 14 (YAGNI) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Print the success message unconditionally (exit code 0 already confirmed success), then optionally extract the URL from the response JSON. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers

🤖 AI Review Summary

Verdict: ✅ LGTM

Found 2 issues (0 errors, 1 warning, 1 suggestion):

⚠️ Documentation: Relative link in review template will be broken in PR comments (SKILL.md:75)
💡 Error handling: Silent catch (JsonException) — the review rules themselves prohibit this pattern (submit_review.cs:138)

👍 Positive callouts:

Excellent knowledge base — CODE_REVIEW_POST_MORTEM.md with 71 findings linked to actual PR discussions is incredibly valuable for onboarding and AI grounding.
Well-designed workflow — The "form an independent assessment before reading the PR description" pattern in the skill reduces anchoring bias. Great idea.
Security — submit_review.cs correctly uses ArgumentList instead of string interpolation for process arguments. ✓
Comprehensive rules — 14 categories covering everything from MSBuild incremental build correctness to native C++ memory management. The AI-specific pitfalls section (§14) is a particularly smart addition.
Solid validation — The submit script validates JSON structure, required fields, 🤖 prefix, and positive line numbers before posting. Good defense-in-depth.

Review generated by android-reviewer from review guidelines.

.github/skills/android-reviewer/SKILL.md

.github/skills/android-reviewer/scripts/submit_review.cs

- Use absolute URL for review guidelines link in PR comment template (relative links in PR comments don't resolve to repo files) - Log a message in catch (JsonException) instead of silently swallowing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Fix submit_review.cs issues found during porting: - Replace GetString()! with ?? "" to avoid null-forgiving operator - Add null check for Process.Start() instead of ! - Narrow bare catch blocks to catch (JsonException) - Restructure success output so catch block isn't misleading Merge C#-focused review rules back into review-rules.md: - Async: thread safety of shared state - Error handling: actionable exception details, challenge exception swallowing - Code org: no commented-out code, early returns, skip default field init - Performance: pre-allocate collections, avoid closures, cheap checks first, O(n²) - Patterns: method names reflect behavior, why-not-what comments, track TODOs - YAGNI: git commit --amend, commit message quality, typos, filler words - New section 13: Testing (regression tests, specific assertions, deterministic data, edge cases) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Adds a new android-reviewer GitHub skill to help standardize PR reviews in dotnet/android, based on established repository conventions and past maintainer feedback.

Changes:

Added a code review post-mortem document summarizing common review findings from prior PRs.
Introduced the android-reviewer skill, including a rules reference document and workflow guidance.
Added a C# helper script to validate and submit batched PR reviews via the GitHub CLI.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
`docs/CODE_REVIEW_POST_MORTEM.md`	New post-mortem doc capturing recurring review feedback patterns.
`.github/skills/android-reviewer/SKILL.md`	Skill definition and end-to-end workflow for running the reviewer and submitting results.
`.github/skills/android-reviewer/references/review-rules.md`	Centralized review rules adapted to `dotnet/android` conventions.
`.github/skills/android-reviewer/scripts/submit_review.cs`	Script to validate review JSON and submit it via `gh api`.

.github/skills/android-reviewer/scripts/submit_review.cs

.github/skills/android-reviewer/SKILL.md

.github/skills/android-reviewer/references/review-rules.md

docs/CODE_REVIEW_POST_MORTEM.md

- Add ValueKind checks for path/body in comment validation to prevent InvalidOperationException on non-string JSON values - Treat non-array 'comments' field as a validation error instead of silently skipping - Soften AndroidTask rule: simple wrapper tasks (AndroidError, AndroidWarning, AndroidMessage) may inherit from Task directly - Note ThrowIf helpers are unavailable in netstandard2.0 projects Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

submit_review.cs: - Add ValueKind checks for path/body in comment validation to prevent InvalidOperationException on non-string JSON values - Treat non-array 'comments' field as a validation error instead of silently skipping - Log a note when API response is not JSON instead of silent comment review-rules.md: - Add ThrowIf helpers rule noting unavailability on netstandard2.0 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- Dispose Process with 'using' to avoid leaking OS handles - Add source reference to Testing section (dotnet/android#10918) The ValueKind validation feedback was already addressed in the previous commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The rule was triggering false positives on logical negation like if (!someBool) and if (!string.IsNullOrEmpty(s)). Explicitly note the distinction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Fix submit_review.cs issues found during porting: - Replace GetString()! with ?? "" to avoid null-forgiving operator - Add null check for Process.Start() instead of ! - Narrow bare catch blocks to catch (JsonException) - Restructure success output so catch block isn't misleading Merge C#-focused review rules back into review-rules.md: - Async: thread safety of shared state - Error handling: actionable exception details, challenge exception swallowing - Code org: no commented-out code, early returns, skip default field init - Performance: pre-allocate collections, avoid closures, cheap checks first, O(n²) - Patterns: method names reflect behavior, why-not-what comments, track TODOs - YAGNI: git commit --amend, commit message quality, typos, filler words - New section 13: Testing (regression tests, specific assertions, deterministic data, edge cases) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Port review feedback from dotnet/android#10918 submit_review.cs: - Add ValueKind checks for path/body in comment validation to prevent InvalidOperationException on non-string JSON values - Treat non-array 'comments' field as a validation error instead of silently skipping - Log a note when API response is not JSON instead of silent comment review-rules.md: - Add ThrowIf helpers rule noting unavailability on netstandard2.0

The skill was posting LGTM verdicts even when CI checks were failing. Add Step 4 (Check CI status) requiring the reviewer to verify CI is green before approving. If CI is red, the verdict must be at least 'Needs Changes' even if the code itself looks fine. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers · 2026-03-12T14:02:11Z

I'm going to merge this, and we'll try it out.

jonathanpeppers and others added 6 commits March 11, 2026 12:59

[skills] Reduce android-reviewer frontmatter to minimize token usage

5195217

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers commented Mar 11, 2026

View reviewed changes

jonathanpeppers and others added 2 commits March 11, 2026 13:29

Restructure success output so catch block isn't misleading

eb37e1f

Print the success message unconditionally (exit code 0 already confirmed success), then optionally extract the URL from the response JSON. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers commented Mar 11, 2026

View reviewed changes

.github/skills/android-reviewer/SKILL.md Outdated Show resolved Hide resolved

.github/skills/android-reviewer/scripts/submit_review.cs Show resolved Hide resolved

jonathanpeppers marked this pull request as ready for review March 11, 2026 18:41

jonathanpeppers requested review from grendello and simonrozsival as code owners March 11, 2026 18:41

Copilot AI review requested due to automatic review settings March 11, 2026 18:41

Copilot started reviewing on behalf of jonathanpeppers March 11, 2026 18:42 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

Copilot AI mentioned this pull request Mar 11, 2026

[skills] Incorporate review feedback from dotnet/android#10918 dotnet/android-tools#299

Merged

Clarify null-forgiving rule applies to postfix ! not logical negation

062d57f

The rule was triggering false positives on logical negation like if (!someBool) and if (!string.IsNullOrEmpty(s)). Explicitly note the distinction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers and others added 2 commits March 11, 2026 14:25

Reference azdo-build-investigator skill for CI failure investigation

8b4b55f

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

jonathanpeppers merged commit 8e27f60 into main Mar 12, 2026
5 of 6 checks passed

jonathanpeppers deleted the dev/peppers/code-reviewer-skill branch March 12, 2026 14:02

jonathanpeppers added the copilot `copilot-cli` or other AIs were used to author this label Mar 12, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[skills] Add android-reviewer skill for PR code reviews#10918

[skills] Add android-reviewer skill for PR code reviews#10918
jonathanpeppers merged 13 commits intomainfrom
dev/peppers/code-reviewer-skill

jonathanpeppers commented Mar 11, 2026 •

edited

Loading

Uh oh!

jonathanpeppers left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonathanpeppers left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonathanpeppers commented Mar 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jonathanpeppers commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

🤖 AI Review Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

🤖 AI Review Summary

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonathanpeppers commented Mar 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jonathanpeppers commented Mar 11, 2026 •

edited

Loading