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

Exactly one review source is required.

Outputs

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:

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

   gh pr diff <PR_NUMBER> --repo dotnet/maui

2. Read full source files for every changed file (not just diff hunks):

   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:

   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:

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:

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:

  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:

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.

# 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.

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