Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion .github/skills/code-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ that the fixture does not provide. Then continue at Step 1.5.

For a live `pr_number`:

**If a retrieval command fails, the PR is still available.** A failing or unauthenticated
command is a fact about that one tool, not about the review. Retry through another
read-only route — `gh api repos/dotnet/maui/pulls/<PR_NUMBER>/files`, or the local
checkout with `git diff` — and report an inability to review only after those also fail.
Never ask the caller to paste the diff.

1. **Get the diff:**
```bash
gh pr diff <PR_NUMBER> --repo dotnet/maui
Expand Down Expand Up @@ -197,6 +203,12 @@ Classify based on the stdout row content (`pass`/`fail`/`skipping`/`pending`) **

**Required when PR modifies:** handlers, platform extensions, toolbar/navigation code, page registration, static state, `PropertyChanged` subscriptions, or startup paths.

**Also required — both the assessment below and Failure-Mode Probing — for behavioral changes to these frequently-regressed component families:** CollectionView, CarouselView, Image/Graphics, Theme/Style, Gesture/Tap, Button/Entry, Toolbar, and Shell/TabBar. This list is complete and sufficient on its own. The `Frequently Regressed Components` table in `.github/agents/maui-expert-reviewer.md` (under the Regression Prevention dimension) mirrors it and adds per-family risk areas; read it for that extra detail when it is present. For these families the usual miss is an untested *adjacent* scenario: a spacing fix that also runs on scroll-position restoration, a `CurrentItem` or loop-mode change that also affects `ScrollTo`, or a touch-handling fix that also affects tap/swipe/gesture.

The Step 2 expert reviewer reports findings only, with no per-dimension activation record, so its output cannot distinguish "Regression Prevention ran and found nothing" from "it never ran" — and a finding from some *other* dimension is not evidence it ran either. Never claim to have confirmed that a dimension fired. For every family that triggers this section, run the Failure-Mode Probing questions below yourself regardless of what the expert reported.

A prose-only change — documentation or comments — **need not** carry the family escalation above, provided the edited text is genuinely inert. It is not inert if it alters a public API doc, an analyzer or compiler directive (`<auto-generated/>`, `#pragma warning`, suppression attributes), an agent-instruction file this repo executes, or a comment stating a precondition other code relies on without re-verifying ("caller must dispose", "always called on the UI thread", "assumes sorted input"). Non-inert prose still gets a full review, but the Blast Radius table below asks runtime questions — startup ordering, static state, `PlatformView` nullity — that a text edit cannot answer. Probe the contract the text actually encodes instead: for a documented precondition, whether the code relying on it still holds; for a directive, which warnings or generated-code handling it now suppresses; for an agent-instruction file, whether the new wording fires on invocations it was not meant to reach, contradicts an instruction elsewhere in the same file, or states a condition the agent cannot evaluate from what it already has.

| Question | Why It Matters |
|----------|---------------|
| Does this code run for ALL instances, or only when the new feature is used? | Feature code that runs unconditionally is the #1 cause of startup crashes |
Expand Down Expand Up @@ -270,7 +282,7 @@ Classify based on the stdout row content (`pass`/`fail`/`skipping`/`pending`) **
*(If no prior reviews with ❌ Error findings, state "No prior ❌ Error findings found.")*

### Blast Radius Assessment
*(Required for infrastructure/handler/platform changes; omit for simple fixes)*
*(Required for infrastructure/handler/platform changes, or for a frequently-regressed component family or non-inert prose per Step 6; omit for simple fixes)*
- Runs for all instances: [yes/no — explanation]
- Startup impact: [yes/no]
- Static/shared state: [yes/no]
Expand Down
Loading