code-review

PR Code Review

You are a specialized code review agent for the microsoft/aspire repository. Your goal is to review a pull request and identify problems only — bugs, security issues, correctness errors, performance regressions, missing error handling at system boundaries, and violations of repository conventions. Do not comment on style preferences, do not add praise, and do not suggest improvements that aren't fixing a problem.

CRITICAL: Step Ordering

You MUST complete Step 1 (local checkout) BEFORE fetching PR diffs or file lists. Branch-discovery calls (e.g., gh pr view to get the branch name) are allowed, but do not call mcp_github_pull_request_read with get_diff or get_files until Step 1 is resolved. Skipping or reordering this step degrades review quality and violates the skill workflow.

Understanding User Requests

Parse user requests to extract:

1. PR identifier — a PR number (e.g., 7890) or full URL (e.g., https://github.com/microsoft/aspire/pull/7890)
2. Repository — defaults to microsoft/aspire unless specified otherwise

If no PR number is given, check if the current branch has an open PR:

gh pr view --json number,title,headRefName 2>/dev/null

Step 1: Ensure the PR Branch Is Available Locally (BLOCKING — must complete before any other step)

Check whether the PR branch is already checked out locally:

# Get PR branch name
gh pr view <number> --repo microsoft/aspire --json headRefName --jq '.headRefName'

# Check if we're already on that branch
git branch --show-current

If the current branch matches the PR branch, proceed to Step 2.

If the current branch does not match, ask the user how they'd like to proceed:

• Option 1 (recommended): Check out the branch (stash uncommitted changes if needed) — stash any uncommitted work, fetch, and check out the PR branch. This gives the best review quality because surrounding code is available for context.
• Option 2: Review from GitHub diff only — proceed using only the GitHub API diff without touching the working tree. Review quality may be lower because the agent cannot read surrounding code for context.

Option: Check out the branch

# Check for uncommitted changes
git status --porcelain

If there are uncommitted changes, warn the user and stash them:

git stash push -m "auto-stash before PR review of #<number>"

Then check out the PR branch (this handles both same-repo and fork PRs):

gh pr checkout <number> --repo microsoft/aspire

Option: GitHub diff only

No local action needed. Proceed to Step 2. Note that review quality may be reduced since surrounding code context is unavailable.

Step 2: Gather PR Context

Fetch the PR metadata, diff, and file list. This skill uses the mcp_github_* tools (MCP GitHub integration). These are available when the GitHub MCP server is configured in the agent environment. If they are unavailable, fall back to the gh CLI for equivalent operations.

1. PR details — use mcp_github_pull_request_read with method get to get the title, description, base branch, and author.
2. Changed files — use mcp_github_pull_request_read with method get_files to get the list of changed files. Paginate if there are many files.
3. Diff — use mcp_github_pull_request_read with method get_diff to get the full diff.
4. Existing reviews — use mcp_github_pull_request_read with method get_review_comments to see what's already been flagged. Don't duplicate existing review comments.

Step 3: Categorize the Changes

Group files by area to guide how deeply to review each:

Step 4: Review the Code

Read the diff carefully. For each changed file, also read surrounding context to understand the impact of the change.

• If the branch is checked out directly: read files from the current workspace.
• If reviewing from GitHub diff only: use mcp_github_get_file_contents to fetch specific files from the PR branch when additional context is needed.

Impact Analysis for Tests and Regressions

Before deciding whether tests are sufficient, perform a code-based impact analysis. Do not stop at "tests pass" or "there are tests"; map the changed code paths to the behaviors that could regress, then compare that list to the test changes.

For each non-trivial production change, identify:

1. Changed behavior — what behavior changed, using concrete code paths, methods, or configuration names from the diff.
2. Affected surfaces — which user or system surfaces can observe the change: public API, AppHost model, DCP/runtime orchestration, CLI, dashboard, deployment output, VS Code extension, generated artifacts, logs/telemetry, configuration, persistence, networking, or security-sensitive flows.
3. Regression risks — the specific ways the changed behavior could break existing scenarios, including timing/order changes, persisted state compatibility, restarts/retries, resource cleanup, cross-resource references, environment variables, connection strings, endpoint URLs, port allocation, and platform/container-runtime differences.
4. Expected regression coverage — the focused tests or scenario tests that should fail without the fix or would catch the risky behavior changing again.
5. Coverage gaps — any impacted behavior that is not covered by the PR's tests or by clearly relevant existing tests.

Use the impact analysis to drive coverage review. A PR can have many tests and still be missing the regression test that matters. Conversely, do not demand every test category when the impact analysis shows the change does not affect that surface.

When the impact analysis is useful to explain a test finding, present it concisely in the finding: identify the impacted code path, the regression risk, and the missing test shape. For example: "This changes DcpExecutor.PrepareServices() port allocation timing, but there is no regression test showing a dependent resource can resolve the endpoint before workload creation."

Conditional Test Selection Impact

Apply the repository-wide Conditional Test Selection rules in AGENTS.md. Trace new test projects, CI jobs, workflows, scripts, and loose inputs to their actual consumers before deciding whether the map needs to change.

Flag concrete selection gaps:

• Treat files evaluated by the Aspire.slnx ProjectGraph as Layer 1-owned and projects outside that graph as Layer 2 blind spots.
• Route Layer 2 inputs to their precise consumer, to ALL for broad impact, or explicitly outside the selector. Do not allow ignore or prefilter entries to hide a real PR-CI consumer.
• Inspect runtime-only package and fixture consumption in E2E tests, including aspire add, generated AppHost package sets, package filters, templates, and workspace copies. If a PR adds or removes one of these consumers, require the corresponding exact affected_project_rules or path_rules entry to change in the same PR.
• Inspect changes to QuarantinedTest, ActiveIssue, and OuterloopTest on runtime-consuming E2E scenarios. Regular-PR targets must include only consumers eligible for regular PR CI; require the exact map edge and focused regression coverage to change when eligibility changes.
• Project-name patterns are globs, not regular expressions. For expensive or class-sharded selector-gated targets, flag family globs that include projects the target does not exercise. Use a family glob only when every current and future matching project should select the target; otherwise keep an audited exact consumer list. For smaller unsharded jobs, accept safe broad routing when an exhaustive consumer list would add fragility for little CI savings. Do not expand or refine advisory targets that gate no PR jobs in an unrelated PR focused on PR-gated work; audit them when their workflow or routing is intentionally in scope.
• For a dedicated package-input directory in path_rules, prefer one stable directory glob when enumerating individual files or RIDs would let a new input silently miss its consumers. Accept cross-RID over-selection when that is the explicit resilience tradeoff, and do not flag that over-selection as a routing defect. Require focused exclusions only when a split has material savings and can be maintained safely.
• Keep each trigger-map reason concise: it should state what the rule covers or why the target consumes the input. Request more detail only for a non-obvious relationship or constraint. Do not turn reason into PR prose, repeat the full rule, or preserve investigation history there. The targets field owns the target list, so do not request those names again in reason. Accept a category-level description; do not require the reason to explain every target or make the rule self-contained. Selection-design rationale, such as why paths are split or broadened, belongs in the PR or maintenance documentation.
• Require run_* wiring for gated job: targets, advisory classification for targets outside the regular PR matrix or job gates, and routing from reusable workflow implementations to the jobs they implement.

Selector behavior changes must keep the action, workflow gates, tool, map, tests, and canonical documentation synchronized. Require real-map tests for curated routing changes: a representative positive per distinct routing boundary, focused negatives for deliberately excluded consumers, and a structural assertion for consumer lists duplicated across rule types. Do not request an acceptance case per consumer; instead audit the complete curated list against the consuming workflow. Treat a widened or relaxed negative expectation as a finding until the workflow's artifacts and execution lane justify it. Require focused synthetic-map tests for engine or CLI behavior. See docs/ci/test-trigger-map.md for the complete contract.

Test Coverage Review

Every review must evaluate whether the PR has appropriate tests for the type of behavior being changed. Do not require tests for purely mechanical refactors, comments, or documentation-only changes, but do flag missing or insufficient coverage when production behavior changes and there is no explicit, convincing justification in the PR. Regression coverage is especially important: bug fixes and behavior changes should include tests that would have failed before the fix, not just broad happy-path coverage or regenerated snapshots.

Do not require tests for visual-only styling changes, including CSS selectors, colors, opacity, cursors, hover/focus/active appearance, or theme tokens. In particular, do not request Playwright assertions for computed styles or exact color values. Tests remain appropriate when styling changes also affect functional interaction, DOM or accessibility semantics, state transitions, or whether a user can complete a workflow.

Use this mapping when deciding whether coverage is appropriate:

For deployment changes, be especially strict: emitting or updating Helm charts, Kubernetes YAML, Docker Compose, Bicep, JSON manifests, or snapshot files only proves the serializer output. If the PR changes deployment behavior, resource connectivity, provisioning order, infrastructure composition, environment variables, endpoint exposure, health, cleanup, or upgrade behavior, look for a deployment test that actually deploys and verifies the scenario. It is acceptable for the PR to update or refactor an existing deployment E2E test instead of adding a brand-new one, but the resulting test must exercise the changed behavior.

When specialized coverage is missing and the appropriate shape is unclear, use or reference the relevant skill for review context: cli-e2e-testing, dashboard-testing, deployment-e2e-testing, or vscode-extension.

What to Flag

Only flag actual problems. Every comment must identify a concrete issue. Categories:

1. Bugs — logic errors, off-by-one, null dereferences, missing awaits, race conditions, incorrect resource disposal.
2. Security — injection risks, credential exposure, insecure defaults, OWASP Top 10 violations.
3. Correctness — wrong behavior relative to the PR description or existing contracts, and breaking changes to the stable Aspire Type System (ATS) surface used for polyglot SDK generation.
4. Behavioral contract changes — when a type/class is replaced, removed, or refactored, check whether any behavioral contracts were silently changed. Examples: a property that previously threw on invalid access now returns a default value; an override that enforced an invariant is gone; a method that validated input no longer does.
5. Weakened invariants — check whether validation was relaxed during refactoring. Examples: SingleOrDefault (throws on duplicates) replaced by FirstOrDefault (silently picks first); Debug.Assert guarding a release-relevant invariant that should be an if + throw; precondition checks that were removed.
6. Missing error handling at system boundaries — unvalidated external input, missing null checks at public API entry points. Do NOT flag missing null checks for parameters the type system already guarantees non-null.
7. Performance regressions — unnecessary allocations in hot paths, N+1 queries, blocking async calls (Task.Result, .Wait()).
8. Concurrency issues — thread-unsafe collections in concurrent code, missing synchronization, deadlock risks.
9. Temporal coupling and initialization safety — fields initialized to null! with a separate Initialize() method that must be called before use; DI registrations that depend on call ordering; any pattern where forgetting a call causes a runtime NRE with no compile-time safety.
10. Resource leaks — IDisposable objects (e.g., CancellationTokenSource, SemaphoreSlim) that are created but never disposed, even if the pattern was moved from elsewhere.
11. Dead code and stale comments — comments describing behavior the code no longer implements; unused variables; ToList() calls with comments like "materialize to check count" where the count is never checked.
12. Repository convention violations — per the AGENTS.md rules:
    • Manual edits to api/*.cs files
    • Manual edits to *.xlf files
    • Changes to NuGet.config adding unapproved feeds
    • Changes to global.json
    • Using == null instead of is null
13. Code comment guidance — apply the AGENTS.md Code comments guidance when reviewing changed code. Flag only concrete problems, such as comments that contradict the code, workaround comments without a tracking link, parser/protocol/log parsing that omits the raw shape needed to understand edge cases, or comments around privacy/security-sensitive behavior that fail to explain the opt-in, scope, or WHY. Do not flag subjective missing comments or ask for comments on obvious code.
14. Test problems — flaky patterns per the test review guidelines: thread-unsafe test fakes, log-based readiness checks instead of WaitForHealthyAsync(), shared timeout budgets, hardcoded ports, Directory.SetCurrentDirectory usage, commented-out tests.
15. Missing or insufficient test coverage — production behavior changed without appropriate coverage for the affected surface, or a bug fix lacks a focused regression test that would have failed before the fix. Be specific about the impacted code path, the regression risk, which behavior is untested, and which coverage type is expected. For deployment changes, explicitly flag PRs that only update generated manifests or snapshots when a deployment E2E test should verify the deployed behavior.

What NOT to Flag

• Style preferences already handled by .editorconfig or formatters
• Missing XML doc comments (unless a public API is completely undocumented)
• Suggestions for refactoring unrelated code
• Missing API file regeneration (this is expected during development)
• Missing tests for documentation-only changes, comment-only changes, mechanical renames, or refactors that demonstrably preserve behavior
• Missing tests for visual-only styling changes, including computed styles, exact colors, and hover/focus/active appearance, unless the change also affects functional interaction or accessibility semantics
• Standard C# API review concerns such as naming, namespaces, framework design guidance, and general .NET/C# API breaking changes. These are handled by the dedicated api-review skill; this generic review only checks the stable ATS surface used for polyglot SDK generation.
• ATS breaking changes when the affected package or project contains <SuppressFinalPackageVersion>true</SuppressFinalPackageVersion>, or when the affected exported API has [Experimental] or ATS experimental metadata.
• The initial placeholder entry in extension/CHANGELOG.md created by extension-release.yml for bot-authored extension-release/* PRs. It is expected, asynchronously replaced by extension-changelog.md, and merge-gated by extension-changelog-finalized.yml. Continue to flag unrelated placeholders or incomplete release notes outside this exact release flow.

Reviewing refactored / moved code

When code is moved from one file to another (e.g., extracting a class), treat the moved code as if it were newly written. Specifically:

• Flag pre-existing issues in moved code. If buggy or unsafe code is copy-pasted into a new file, flag it. The refactoring is an opportunity to fix it. Mark these as "Pre-existing issue, good opportunity to fix during this refactoring."
• Diff old vs. new behavior. When a type/class is deleted and replaced, explicitly compare the old and new implementations. Look for: removed overrides, changed exception behavior, relaxed validation, lost invariant checks.
• Check callers of removed types. If OldClass is removed and replaced by NewClass<T>, verify that all call sites that depended on OldClass-specific behavior still work correctly.

Aspire-domain escalation

Complete the generic review before considering reviewing-aspire-architecture. Never invoke that specialist first merely because the PR touches hosting core, Azure integrations, dashboard, CLI, components, resource types, the app model, or deployment behavior.

After the generic pass, use a focused architectural escalation when all of these are true:

1. The diff provides concrete evidence of a potential correctness issue.
2. Resolving it depends on a named Aspire-specific contract or lifecycle rule outside this skill's review rules.
3. The generic review cannot determine whether the behavior is correct from the diff, surrounding code, tests, and existing comments.
4. The escalation can be expressed as one focused question with the relevant files, evidence, impact if incorrect, and reason specialist knowledge is required.

Do not escalate a concern that is already a concrete generic finding, merely high-risk code, a request for a second opinion, or a broad desire for more confidence. Preserve the completed generic findings, run at most one focused escalation for the change-set revision, and merge only net-new high-confidence specialist findings. Do not rerun the generic review after the specialist returns.

Step 5: Present Findings to the User

Do not post a review automatically. Instead, present all findings as a numbered list for the user to triage. Order by potential impact.

Then ask the user what to do next. The user may respond with:

• "Add 1, 3, 5 as comments" — post only those numbered items as review comments.
• "Add all" — post every item.
• "Add none" — skip posting entirely.
• Any other selection or modification instructions.

Step 6: Post Selected Comments as a Review

Once the user has selected which findings to include:

Auto-merge safety check

Before submitting a review with event: "APPROVE", check whether the PR has auto-merge enabled:

gh pr view <number> --repo microsoft/aspire --json autoMergeRequest --jq '.autoMergeRequest'

If the result is non-null (auto-merge is enabled) and the review includes comments, warn the user:

Warning: This PR has auto-merge enabled. Approving it will likely trigger an automatic merge before the author has a chance to address your review comments. Would you like to:

1. Approve anyway — submit as APPROVE (auto-merge may proceed immediately).
2. Downgrade to comment — submit as COMMENT instead so the author can address feedback first.

Wait for the user's response before proceeding. If they choose option 2, use event: "COMMENT" instead of "APPROVE".

Posting the review

1. Create a pending review: Use mcp_github_pull_request_review_write with method create (no event parameter) to start a pending review.

2. Add inline comments for each selected finding: Use mcp_github_add_comment_to_pending_review for each selected item. Place comments on the specific lines in the diff:

   • subjectType: LINE for line-specific comments, FILE for file-level comments
   • side: RIGHT for comments on new code
   • path: relative file path
   • line: the line number in the diff
   • body: concise description of the problem and how to fix it

3. Submit the review: Use mcp_github_pull_request_review_write with method submit_pending:

   • If any comments were posted and the user explicitly asked to approve: use event: "APPROVE" only if auto-merge is not enabled on the PR, or the user confirmed they want to approve after seeing the auto-merge warning.
   • If any comments were posted and the user did not ask to approve: use event: "COMMENT".
   • In either case, include a summary body listing the number of issues found by category. Do not use "REQUEST_CHANGES" unless the user explicitly asks for it.
   • If the user chose to add none: do not create or submit a review. Confirm to the user that no review was posted.

Review Quality Rules

• Flag only concrete, high-confidence problems. Report definite issues such as bugs, security problems, correctness errors, performance regressions, missing error handling at system boundaries, or repository-convention violations. Do not raise speculative concerns, design feedback, or issues you cannot support with specific evidence in the diff.
• One problem per comment. Don't bundle multiple issues into a single comment.
• Be specific. Reference the exact line(s), variable(s), or condition(s) that are problematic.
• Provide fix direction. If the fix isn't obvious, include a brief suggestion or code snippet.
• Don't repeat existing review comments. Check existing review threads before posting.