diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index 2d0e48a3382c..a81c58eb3566 100644 --- a/.github/skills/code-review/SKILL.md +++ b/.github/skills/code-review/SKILL.md @@ -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//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 --repo dotnet/maui @@ -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 (``, `#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 | @@ -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]