code-review

# Code Review Skill Standalone skill that evaluates PR code changes for correctness, safety, performance, and consistency with .NET MAUI conventions. Can be invoked directly by users or by other agents/skills. **Trigger phrases:** "review code for PR #XXXXX", "code review PR #XXXXX", "review this PR's code", "analyze code changes in PR", "check PR code quality" **Do NOT use for:** "what does PR #XXXXX do?", "summarize PR", "describe the changes", or any informational query — just answer those directly without invoking this skill. > **How this differs from other skills:** > - **`pr-review`** — End-to-end PR workflow (4 phases: pre-flight, gate, try-fix, report). Use when you want the full pipeline including test verification and fix attempts. > - **`pr-finalize`** — Verifies PR title/description match implementation + light code review. Use before merging. > - **`code-review`** (this skill) — Deep code-only review with MAUI domain rules. Use when you want a thorough code analysis without running tests or modifying the PR. ## Core Principles 1. **Independence-first** — Form your assessment from the code BEFORE reading the PR description. This prevents anchoring on the author's framing 2. **Full-context** — Read entire source files, not just diffs. Check callers, consumers, and git history 3. **Empirical grounding** — Reference specific code, line numbers, and call sites. No vague concerns 4. **Severity calibration** — Distinguish errors from warnings from suggestions. Not everything is critical 5. **Failure-mode probing** — Challenge your own conclusions with real failure scenarios, not softballs 6. **Propagation-aware guards** — For an early return, idempotency flag, or latch above downstream side effects, trace every set/clear path and a repeat call after recipients or state change. If that trace exposes concrete misbehavior, do not dismiss it as rare or rationalize it into `LGTM`: use `NEEDS_DISCUSSION` while the failure remains unresolved, or `NEEDS_CHANGES` when the exact state transition verifies a ❌ Error. 7. **Authentication is not availability** — A public GitHub PR remains reviewable when `gh` is unauthenticated. Never stop or ask for a token merely because a `gh` command failed; pivot immediately to anonymous public REST/web retrieval. ## Inputs | Input | Required | Description | |-------|----------|-------------| | `pr_number` | Conditional | GitHub PR number for a live-PR review | | `review_input` | Conditional | Materialized candidate diff plus supporting source files; use when no live PR is available | Exactly one review source is required. ## Outputs | Field | Description | |-------|-------------| | `verdict` | `LGTM`, `NEEDS_CHANGES`, or `NEEDS_DISCUSSION` | | `confidence` | `high`, `medium`, or `low` | | `findings` | Categorized findings with severity levels | --- ## Review Workflow ### Step 1: Gather Code Context (No PR Narrative) **Do NOT read the PR description or issue yet.** For a materialized `review_input`, read its candidate diff first, then every supporting source file in full. Trace callers, consumers, and producers available in the snapshot. Do not fetch PR narrative, external pages, or repository history 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. `gh api` also requires authentication, so it is not an unauthenticated fallback. For public `dotnet/maui` PRs, pivot to anonymous read-only retrieval: ```bash # Candidate patch: curl -fsSL -H 'Accept: application/vnd.github.patch' \ https://api.github.com/repos/dotnet/maui/pulls/<PR_NUMBER> # Changed-file metadata and patches: curl -fsSL \ 'https://api.github.com/repos/dotnet/maui/pulls/<PR_NUMBER>/files?per_page=100' ``` Equivalent `web_fetch` calls are acceptable. Use response `raw_url` values or `https://raw.githubusercontent.com/dotnet/maui/<HEAD_SHA>/<PATH>` for full changed-file contents. A local checkout with `git diff` is another valid route. Anonymous API rate limiting may require fewer targeted requests, but it is not an authentication blocker. Never ask the caller to provide a token or paste the diff until anonymous retrieval and local checkout routes have both failed. 1. **Get the diff:** ```bash gh pr diff <PR_NUMBER> --repo dotnet/maui ``` 2. **Read full source files** for every changed file (not just diff hunks): ```bash gh pr diff <PR_NUMBER> --repo dotnet/maui --name-only # Then read each file in full ``` 3. **Check callers and consumers** of changed methods/properties: - Use LSP `findReferences` and `incomingCalls` for modified symbols - Understand how the changed code is used 4. **Review git history** of changed files: ```bash git log --oneline -10 -- <changed-file> ``` ### Step 1.5: Trace External Output Contracts (Always Active) When changed code classifies external tool output with a regex or string literal: 1. Locate and read the producer, even when it is outside the diff. 2. State the exact condition under which the producer emits each matched token. Confirming that the text exists is not enough: compare the producer's emission condition with the consumer's semantic assumption. 3. Construct an ordinary negative case that must not trip the classifier, then trace it through every downstream guard, cap, veto, or early return. For an incompleteness classifier, the required negative case is a run that **completed with ordinary test failures**, not merely a successful run. A generic nonzero exit proves failure, not incompleteness. If the producer prints a completion token for every exit and the consumer treats its nonzero form as killed, hung, crashed, or incomplete, report the false positive unless an authoritative producer contract proves nonzero exits are exclusive to incomplete runs. 4. If the ordinary case reaches the restrictive path, report a correctness finding and do not return `LGTM` unless the over-restriction is explicitly intended and documented. Fail-closed direction does not make the behavior correct. Before the verdict, include an **External Output Contract** table with these columns: | Consumer token/pattern | Producer location | Producer emission condition | Consumer assumption | Ordinary negative case | Downstream effect | |---|---|---|---|---|---| A row that only confirms matching text, without comparing the two conditions, is incomplete analysis. These are the direct-execution form of the always-active Logic/Correctness and Regression Prevention CHECKs in `.github/agents/maui-expert-reviewer.md`. If the expert agent is unavailable in the current environment, apply these probes yourself rather than skipping them. ### Step 1.6: Trace Trim and NativeAOT Reachability (When Applicable) When a change touches `RequiresUnreferencedCode`, `RequiresDynamicCode`, `DynamicallyAccessedMembers`, `FeatureGuard`, `FeatureSwitchDefinition`, or IL2026/IL3050 suppression: 1. Trace the complete warning path from the guarded call through annotated helpers and generic registration methods. Do not classify a warning as a false positive without locating the annotation or dynamic-code operation that produced it. 2. Distinguish the property's ordinary runtime default from its trim-time contract. A getter that defaults to `true` does not by itself prove that a guarded branch remains reachable: `FeatureSwitchDefinition` can substitute the property value, and `FeatureGuard` communicates the resulting reachability to analysis. Verify the attributes and guard before deciding. 3. Treat an annotated helper called only inside the verified feature guard as structural isolation, not as warning suppression. The helper annotations move the trim/AOT contract to the direct guarded call; they do not make an unconditional call safe. 4. Accept a pragma only when it suppresses the specific diagnostics around the affected call, restores them immediately, and the supplied source proves the call unreachable in the affected configuration. A documented toolchain-specific analyzer limitation can justify that narrow exception. Reject a broad, unexplained, or reachable suppression. 5. Base the verdict on the actual guard and annotation chain. Do not infer reachability solely from a default value, a comment, or the presence of a pragma. Before the verdict, include a **Trim/AOT Evidence Chain** table with these columns: | Link | Source evidence | Reachability implication | |---|---|---| When those sources are available, trace the build-time feature-switch value, the runtime property and its attributes, the changed helper or suppression, the generic registration annotations, and the annotated handler/dynamic operation. Do not omit a link merely because the final verdict seems obvious. ### Step 2: Delegate to Expert Reviewer For a materialized `review_input`, do **not** delegate or invoke a sub-agent. The supplied snapshot is the complete evidence boundary, and the main reviewer must read every supporting file and apply the applicable `.github/agents/maui-expert-reviewer.md` dimension checks directly. Continue at Step 3 after those checks. For a live `pr_number`, delegate to the `maui-expert-reviewer` agent (`.github/agents/maui-expert-reviewer.md`) with model `gpt-5.3-codex`, which runs per-dimension sub-agent evaluation. Keeping the expert reviewer on a different GPT optimization profile from the GPT-5.6 Sol orchestrator reduces correlated review misses without crossing provider families. The agent's sole output is `inline-findings.json` — file:line comments in GitHub Review API format. **After the agent finishes:** - **If `COMMENTS_VIA_FILE=true`** (CI): Done. The pipeline calls `post-inline-review.ps1` to post findings using `GH_COMMENT_TOKEN`. - **If `COMMENTS_VIA_FILE` is unset** (local): Post inline findings directly: ```bash COMMIT_SHA=$(gh pr view $PR_NUMBER --repo dotnet/maui --json headRefOid --jq .headRefOid) gh api repos/dotnet/maui/pulls/$PR_NUMBER/reviews \ --method POST \ --input <(jq -n \ --arg sha "$COMMIT_SHA" \ --arg body "Expert review — see inline comments." \ --argjson comments "$(cat CustomAgentLogsTmp/PRState/$PR_NUMBER/PRAgent/inline-findings.json)" \ '{commit_id: $sha, body: $body, event: "COMMENT", comments: [$comments[] | {path, line, body, side: "RIGHT"}]}') ``` ### Step 3: Form Independent Assessment Based ONLY on the code (no PR description), answer: 1. **What does this change do?** Describe the behavioral change in your own words 2. **Why might it be needed?** Infer motivation from the code 3. **Is the approach sound?** Would a simpler alternative work? 4. **What problems do you see?** Run through the agent's dimension CHECKs for matched dimensions ### Step 4: Read PR Narrative and Reconcile Now read the PR description, linked issue, and comments. Treat these as **claims to verify**, not facts. 1. Where your assessment disagrees with the author's claims, investigate further 2. If the PR claims a bug fix, verify the root cause analysis matches the code 3. Check existing review comments to avoid duplicating feedback If authenticated `gh` retrieval failed during Step 1, use the same anonymous read-only fallback for these narrative surfaces now: ```bash curl -fsSL https://api.github.com/repos/dotnet/maui/pulls/<PR_NUMBER> curl -fsSL 'https://api.github.com/repos/dotnet/maui/pulls/<PR_NUMBER>/reviews?per_page=100' curl -fsSL 'https://api.github.com/repos/dotnet/maui/pulls/<PR_NUMBER>/comments?per_page=100' curl -fsSL 'https://api.github.com/repos/dotnet/maui/issues/<PR_NUMBER>/comments?per_page=100' ``` #### 🚨 Prior Review Reconciliation Check for prior reviews on the same PR — from the Copilot PR reviewer bot, other agents, or human reviewers. **You MUST query all THREE surfaces** — top-level review bodies, inline review comments, AND PR issue comments. Different reviewers post findings to different surfaces: review-API bots (MauiBot, Copilot bot, this skill's adversarial reviewer) post stubs like *"Expert Review — 3 findings, see inline comments"* at the top level with the actual `❌`/`⚠️`/`💡` markers in inline comments; AI Summary bots and prior-round wall-of-text summaries post to the **issue-comments** surface (which the review API does NOT return). Querying any subset silently misses findings. ```bash # Surface 1: top-level review bodies (review-API stubs, verdicts, human reviewer prose): gh pr view <PR_NUMBER> --repo dotnet/maui --json reviews --jq '.reviews[] | select((.body // "") != "") | "Reviewer: \(.author.login) | State: \(.state)\n\(.body)\n---"' # Surface 2: inline review comments (where MauiBot/Copilot/this skill post ❌/⚠️/💡 findings): gh api repos/dotnet/maui/pulls/<PR_NUMBER>/comments --paginate \ --jq '.[] | "\(.user.login) @ \(.path):\(.line // .original_line // 0)\n\(.body)\n---"' # Surface 3: PR issue comments (where AI Summary bots and prior round wall-of-text summaries post): gh api repos/dotnet/maui/issues/<PR_NUMBER>/comments --paginate \ --jq '.[] | "\(.user.login) @ \(.created_at)\n\(.body)\n---"' ``` Scan all three outputs for `❌` markers, `[major]`/`[moderate]` tags, or equivalent severity language from other reviewer formats. Do NOT slice/truncate the body fields — long bot reviews routinely exceed 10K chars and have severity markers in the tail (empirically observed on this skill's own PRs: MauiBot reviews of 26K+ chars with `❌` markers past char 10000); truncating silently drops them and causes false `LGTM`. **If prior reviews flagged ❌ Error-level issues:** - Verify whether each ❌ Error finding was addressed in subsequent commits - If unresolved → verdict must be `NEEDS_CHANGES` - If status cannot be determined → default to unresolved (caution over optimism) - **NEVER silently drop or contradict a prior ❌ Error finding** — confirm it no longer applies to current code before dismissing ### Step 5: Check CI Status Before delivering a verdict, **collect the required-check status for the PR**. Don't infer CI state from absence of evidence and don't rely on prior commits' status. ```bash gh pr checks <PR_NUMBER> --repo dotnet/maui --required ``` **Exit-code semantics (read this before classifying):** `gh pr checks --required` exit codes are NOT a reliable signal on their own — `gh` overloads them. **Always inspect stdout/stderr.** - Exit `0` is NOT a "clean pass" signal — checks marked `skipping` (e.g., `maui-pr skipping`) also exit `0`. Read the stdout rows for actual state. - Exit `1` is **overloaded** with three cases that look similar but require different responses: - **(a) Failing required check** — stdout lists one or more `fail` rows. - **(b) Zero required checks for the branch** — stdout is empty and stderr contains the substring `checks reported` (specifically either `no checks reported on the '<branch>' branch` when the PR has zero checks of any kind, or `no required checks reported on the '<branch>' branch` when checks exist but none are required). Both shapes mean the PR has no required gates, NOT a tool failure. Route to the *Skipped, pending, or empty result* bullet below. - **(c) `gh` itself errored** — stdout has no check rows and stderr contains `GraphQL:`, `Could not resolve`, `HTTP 4xx/5xx`, or `error:`. Route to the tool-unavailable fallback at the bottom of this section. - Exit `8` means required checks are pending and `gh` is reporting normally. - Other non-zero exits (e.g., auth failure: `gh auth login`, network failure, `command not found`) DO indicate tool unavailability and should trigger the fallback at the bottom of this section. Classify based on the stdout row content (`pass`/`fail`/`skipping`/`pending`) **and** the stderr message, not the exit code alone. If stdout has no check rows and stderr contains a `GraphQL:` / `Could not resolve` / `error:` message, treat as **tool-unavailable** (fallback). If stdout has no check rows and stderr contains `checks reported` (either spelling — see (b) above), treat as **empty result** (not a tool failure). - **PR-caused failing check** (compile/build errors, test failures in modified code) → flag as ❌ Error and `NEEDS_CHANGES`. Surface this in the CI Status / Verdict sections; do NOT also generate per-line inline comments duplicating compiler output (the inline-comment rule in *Review Output Format* still applies). - **Pre-existing infra flake or known issue** (cross-reference with `azdo-build-investigator` skill if uncertain) → note in summary but still cap confidence per the table in Step 6 - **Ambiguous** → invoke the `azdo-build-investigator` skill to determine root cause before finalizing - **PR description acknowledges the failure** → note that the author has documented the dependency; the failure still caps confidence - **Skipped, pending, or empty result** (required check listed as `skipping`/`pending`, or `gh` exits `1` with stderr containing `checks reported` — see Exit-1 case (b) — and no stdout rows) → treat CI coverage as **undetermined**. Do not interpret an empty/skipped result as a passing build. Cap confidence at **low** and **do NOT post `LGTM`** — use `NEEDS_DISCUSSION` (per Rule #6, which prohibits LGTM on pending/undetermined CI as strictly as on red CI). **Never claim "clean build" or `LGTM` without running this step.** Apply the *tool-unavailable* fallback when `gh` cannot determine CI state — either because `gh` itself is missing/unauthenticated (`command not found`, `gh: To get started with GitHub CLI, please run: gh auth login`), or because the command returned a tool/API error instead of check rows (stderr contains `GraphQL:` / `Could not resolve` / `error:` / `HTTP 4xx/5xx` and stdout has no `pass`/`fail`/`skipping`/`pending` rows). In any of those cases, record the gap explicitly and cap verdict confidence at **low**. ### Step 6: Blast Radius, Failure-Mode Probing, and Verdict #### Blast Radius Assessment **Required when PR modifies:** handlers, platform extensions, toolbar/navigation code, page registration, static state, `PropertyChanged` subscriptions, or startup paths.