メニュー
Review code changes in dotnet/roslyn for correctness, performance, and consistency with project conventions. Use when reviewing PRs or code changes.
Codex または Claude でインストール この Prompt をコピーして Codex、Claude、または他のアシスタントに貼り付けると、Skill ページを確認してインストールできます。
SOC 職業分類に基づく
| name | code-review |
| description | Review code changes in dotnet/roslyn for correctness, performance, and consistency with project conventions. Use when reviewing PRs or code changes. |
Review code changes against conventions and patterns established by dotnet/roslyn maintainers and the broader .NET compiler platform team. These rules are adapted from dotnet/runtime review practices and tailored to Roslyn's compiler and IDE infrastructure.
Reviewer mindset: Be polite but very skeptical. Your job is to help speed the review process for maintainers, which includes not only finding problems the PR author may have missed but also questioning the value of the PR in its entirety. Treat the PR description and linked issues as claims to verify, not facts to accept. Question the stated direction, probe edge cases, and don't hesitate to flag concerns even when unsure.
Use this skill when:
Before analyzing anything, collect as much relevant code context as you can. Critically, do NOT read the PR description, linked issues, or existing review comments yet. You must form your own independent assessment of what the code does, why it might be needed, what problems it has, and whether the approach is sound — before being exposed to the author's framing. Reading the author's narrative first anchors your judgment and makes you less likely to find real problems.
git log --oneline -20 -- <file>). Look for related recent changes, reverts, or prior attempts to fix the same problem. This reveals whether the area is actively churning, whether a similar fix was tried and reverted, or whether the current change conflicts with recent work.Based only on the code context gathered above (without the PR description or issue), answer these questions:
Write down your independent assessment before proceeding. You must produce a holistic assessment (see Holistic PR Assessment) at this stage.
Now read the PR description, labels, linked issues (in full), author information, existing review comments, and any related open issues in the same area. Treat all of this as claims to verify, not facts to accept.
If the PR is a pure or targeted revert of an earlier commit, adjust the review scope before leaving feedback:
Code changes frequently outdate the agent knowledge base, but it's easy to miss during a code-focused review. Delegate this to a dedicated sub-agent so it doesn't dilute the primary review's focus. If the environment cannot launch sub-agents, perform this check inline instead.
Launch a sub-agent (e.g., the explore or task agent) with the PR diff and list of changed files, and instruct it to verify whether the change should have updated — but didn't — any of the repo's living documentation:
.github/memory/): FILE_MAP.md (files added/moved/renamed), API_MAP.md, ARCHITECTURE.md, CONVENTIONS.md (new patterns), KNOWN_ISSUES.md (repo-wide surprising behavior/workarounds), TESTING_STRATEGY.md (repo-wide test layout), and INDEX.md (if memory files were added/removed/renamed)..github/memory/known-issues/{compiler,ide,razor}.md (layer-specific quirks/workarounds) and .github/memory/testing/{compiler,ide,razor}.md (layer-specific test base classes/conventions) for the area the change touches..github/instructions/{Compiler,IDE,Razor}.instructions.md): layer-specific directory detail, key files/APIs, diagnostic IDs, and coding conventions for the area the change touches..github/copilot-instructions.md and AGENTS.md when build/test/orientation guidance changes.The sub-agent should cross-check the diff against the update-agent-docs skill checklist (.github/skills/update-agent-docs/SKILL.md) and report, for each gap, which doc file is now stale or incomplete and what specific update is needed. Only flag genuine omissions — do not invent busywork. If the PR already updated the relevant docs, confirm that and move on.
Fold the sub-agent's findings into the review output as ⚠️ (or 💡 for minor) Documentation findings, and ask the author to make the updates. If a required doc update is missing for a behavior/API change, treat it as merge-blocking per the verdict rules.
When the environment supports launching sub-agents with different models (e.g., the task tool with a model parameter), run the review in parallel across multiple model families to get diverse perspectives. Different models catch different classes of issues. If the environment does not support this, proceed with a single-model review.
How to execute (when supported):
gpt-5 and gpt-5.1), pick the one with the highest version number.When presenting the final review (whether as a PR comment or as output to the user), use the following structure. This ensures consistency across reviews and makes the output easy to scan.
## 🤖 Copilot Code Review — PR #<number>
### Holistic Assessment
**Motivation**: <1-2 sentences on whether the PR is justified and the problem is real>
**Approach**: <1-2 sentences on whether the fix/change takes the right approach>
**Summary**: <✅ LGTM / ⚠️ Needs Human Review / ⚠️ Needs Changes / ❌ Reject>. <2-3 sentence summary of the overall verdict and key points. If "Needs Human Review," explicitly state which findings you are uncertain about and what a human reviewer should focus on.>
---
### Detailed Findings
#### ✅/⚠️/❌ <Category Name> — <Brief description>
<Explanation with specifics. Reference code, line numbers, interleavings, etc.>
(Repeat for each finding category. Group related findings under a single heading.)
The summary verdict must be consistent with the findings in the body. Follow these rules:
The verdict must reflect your most severe finding. If you have any ⚠️ findings, the verdict cannot be "LGTM." Use "Needs Human Review" or "Needs Changes" instead. Only use "LGTM" when all findings are ✅ or 💡 and you are confident the change is correct and complete.
When uncertain, always escalate to human review. If you are unsure whether a concern is valid, whether the approach is sufficient, or whether you have enough context to judge, the verdict must be "Needs Human Review" — not LGTM. Your job is to surface concerns for human judgment, not to give approval when uncertain. A false LGTM is far worse than an unnecessary escalation.
Separate code correctness from approach completeness. A change can be correct code that is an incomplete approach. If you believe the code is right for what it does but the approach is insufficient (e.g., treats symptoms without investigating root cause, silently masks errors that should be diagnosed, fixes one instance but not others), the verdict must reflect the gap — do not let "the code itself looks fine" collapse into LGTM.
Classify each ⚠️ and ❌ finding as merge-blocking or advisory. Before writing your summary, decide for each finding: "Would I be comfortable if this merged as-is?" If any answer is "no," the verdict must be "Needs Changes." If any answer is "I'm not sure," the verdict must be "Needs Human Review."
Devil's advocate check before finalizing. Re-read all your ⚠️ findings. For each one, ask: does this represent an unresolved concern about the approach, scope, or risk of masking deeper issues? If so, the verdict must reflect that tension. Do not default to optimism because the diff is small or the code is obviously correct at a syntactic level.
Before reviewing individual lines of code, evaluate the PR as a whole. Consider whether the change is justified, whether it takes the right approach, and whether it will be a net positive for the codebase.
Every PR must articulate what problem it solves and why. Don't accept vague or absent motivation. Ask "What's the rationale?" and block progress until the contributor provides a clear answer.
Challenge every addition with "Do we need this?" New code, APIs, abstractions, and flags must justify their existence. If an addition can be avoided without sacrificing correctness or meaningful capability, it should be.
Demand real-world use cases and customer scenarios. Hypothetical benefits are insufficient motivation for expanding API surface area or adding features. Require evidence that real users need this.
Require large or mixed PRs to be split into focused changes. Each PR should address one concern. Mixed concerns make review harder and increase regression risk.
Defer tangential improvements to follow-up PRs. Police scope creep by asking contributors to separate concerns. Even good ideas should wait if they're not part of the PR's core purpose.
Consider separating bug fixes from feature additions. It's not uncommon for feature work to reveal existing issues. When that happens, consider whether the bug fix should be merged independently of the feature work and directly to the main branch. It's also good to record the existence of the bug in an issue if it doesn't already exist, so that the fix can be tracked and backported as needed.
Require measurable performance data before accepting optimization PRs. Demand BenchmarkDotNet results or equivalent proof — never accept performance claims at face value.
Distinguish real performance wins from micro-benchmark noise. Trivial benchmarks with predictable inputs overstate gains from jump tables, branch elimination, and similar tricks. Require evidence from realistic, varied inputs.
Investigate and explain regressions before merging. Even if a PR shows a net improvement, regressions in specific scenarios must be understood and explicitly addressed — not hand-waved.
Check whether the PR solves the right problem at the right layer. Look for whether it addresses root cause or applies a band-aid. Prefer fixing the actual source of an issue over adding workarounds to production code.
Proactively consult domain experts for risky areas. When a change touches safety-critical, complex, or historically-problematic code areas (e.g., overload resolution, IVT checks, flow analysis), consult the domain expert rather than waiting for them to discover the issue during review. IL optimizations could break patterns recognized by the JIT, so consult the runtime team.
When a PR takes a fundamentally wrong approach, redirect early. Don't iterate on implementation details of a flawed design. Push back on the overall direction before the contributor invests more time.
Ask "Why not just X?" — always prefer the simplest solution. When a PR uses a complex approach, challenge it with the simplest alternative that could work. The burden of proof is on the complex solution.
Fix root cause, not symptoms. Investigate and fix the root cause rather than adding workarounds or suppressing warnings. Don't just catch and ignore exceptions without understanding why they occur.
Explicitly weigh whether the change is a net positive. A performance trade-off that shifts costs around is not automatically beneficial. Demand clarity that the change is a win in the typical configuration, not just in a narrow scenario.
Reject overengineering — complexity is a first-class cost. Unnecessary abstraction, extra indirections, and elaborate solutions for marginal gains are actively rejected.
Every addition creates a maintenance obligation. Long-term maintenance cost outweighs short-term convenience. Code that is hard to maintain, increases surface area, or creates technical debt needs stronger justification.
No style-only changes to the compiler. Every compiler code change needs to be reviewed for correctness, so style-only changes are not acceptable. It is okay for added or changed code to take advantage of new C# features, but we do not broadly revisit existing code to adopt new features or change style.
Separate refactorings from other changes. When possible, it is preferrable to separate refactorings into their own commits, especially when restructuring code to separate files, from functional changes. If PR feedback leads to renaming a type, renaming the file for that type should be done as a separate commit after the PR has been approved. This allows the reviewer to diff the functional change effectively.
Use the null-forgiving operator (!) in compiler product code only when null has already been validated. Nullability suppressions (!) should only appear inside assertions or when the null check was already performed earlier in the same method but the compiler couldn't track it through the control flow. In all other cases, prefer Debug.Assert(value != null) over silencing the warning.
Use throw ExceptionUtilities.Unreachable() for error paths that should not be reachable, throw ExceptionUtilities.UnexpectedValue(val) for exhaustive switches. When a code path should not be reachable, throw an exception rather than asserting. Use throw ExceptionUtilities.UnexpectedValue(val) for default cases in exhaustive switches.
Delete dead code and unnecessary wrappers. Remove dead code, unnecessary wrappers, obsolete fields, and unused variables when encountered or when the only caller changes.
Prefer allowlists over denylists for safety-critical checks. To be correct by construction, it is better to only allow recognized/safe constructs than disallow a list of known unsafe constructs.
Don't bail out early on HasErrors. It's normal for a node to be erroneous in some way but for analysis to still produce useful information. Removing overly broad HasErrors checks often reveals better diagnostics and nullability warnings that were being suppressed.
Question every suspicious change. When a diff contains changes that look unrelated or accidental (e.g., a variable name change, a reordered parameter), call it out explicitly. Accidental pastes and unintended modifications happen.
Prefer abstract/sealed override over virtual for symbol properties. Using abstract (or sealed override) instead of virtual helps catch derived types that need special handling — the compiler will force implementors to explicitly consider the property.
Use proper symbol equality checks. Only use the equality operator (==) for symbols unless it is for reference equality ((object)symbol == otherSymbol). Use SymbolEqualityComparer.Default or the Equals method. Be aware of generic substitution.
Avoid LINQ in compiler produ code. In src/Compilers/, except for trival operations like .Any(...), use manual loops instead of LINQ. We want to make the computation/complexity and allocation costs apparent. LINQ is acceptable in IDE features and tests, but avoid in performance-critical code.
Avoid foreach over collections without struct enumerators. Prefer iterations that avoid allocations.
Avoid closures in hot paths. When a lambda captures locals creating a closure, consider refactoring to avoid the capture or use a static lambda with explicit parameters. Prefer static lambdas when possible.
Use ArrayBuilder<T> and PooledObjects helpers. Roslyn provides pooled collection builders in Microsoft.CodeAnalysis.PooledObjects. Use these in hot paths instead of allocating new collections. But be careful to return pooled objects to the pool and avoid leaks. Specify a requested capacity when it is known to avoid unnecessary resizes.
Avoid O(n²) patterns in collections and hot paths. Watch for linear scans inside loops.
Place cheap checks before expensive operations. Check simple conditions (null checks, kind checks) before calling into semantic models or expensive operations.
Short-circuit tree visitors once the result is determined. When a visitor is searching for a condition (e.g., "does any node match X?"), stop visiting other nodes as soon as the answer is known. Override the common Visit method to check the result flag and bail out early.
Use StringComparer.Ordinal for internal comparisons. Use ordinal comparison for identifiers and internal strings. Only use culture-aware comparison for user-facing text.
In compiler code, use var only when the type is obvious from context. Use explicit types for casts, method returns, and async infrastructure. Never use var for numeric types.
Declare each local variable in its own statement. Don't combine multiple variable declarations on a single line (e.g., bool a = false, b = false;). Use separate declaration statements for clarity.
Place local functions at method end and avoid nested them. Local functions go at the end of the containing method to avoid interrupting the logical flow.
Place fields first in types. Fields are the first members declared in a type.
Narrow warning suppression to smallest scope. Avoid file-wide #pragma suppressions. Disable only around the specific line that triggers the warning.
Extract duplicated logic into shared helper methods. Fix improvements inside shared helpers so all callers benefit.
Use existing APIs instead of creating parallel ones. Before introducing new types, enums, or helpers, check if existing ones serve the same purpose. Fix existing utilities rather than introducing duplicates.
Always add regression tests for bug fixes and behavior changes. Prefer adding test cases to existing test files rather than creating new files.
Test all relevant LangVersion boundaries. When testing language-version-specific behavior, include: the version before the feature (a specific version string that disallows it), the version after (using TestOptions.RegularNext or a specific version that allows it), and the preview version (which always rolls forward with nightly builds). Similarily for IDE features, completions and refactorings may be gated on language version, so test the same boundaries.
Avoid unnecessary intermediary assertions. Tests should do the minimal amount of work to validate just the core issue being addressed.
For error scenarios, prefer VerifyEmitDiagnostics over VerifyDiagnostics on the compilation. VerifyEmitDiagnostics exercises a greater portion of the compilation pipeline.
For non-error scenarios, verify execution if possible. Use CompileAndVerify when the test doesn't produce error diagnostics and VerifyDiagnostics() on the resulting verifier.
Since CompileAndVerify already emits and collects diagnostics, VerifyEmitDiagnostics on the compilation would involve redundant work.
Avoid ConditionalFact/Theory. Parts of the test body may be conditional, but we prefer to execute as much of the test as possible in each configuration.
Suppress expected unrelated warnings in tests with #pragma. When test code intentionally triggers unrelated warnings (e.g., CS0649 for unassigned fields), suppress them with #pragma warning disable to keep test output clean.
Every code path in the changed function should have dedicated test coverage. Include both positive and negative outcomes. If logic is copied from another place, mention the source.
Verify that test assertions are strong enough to detect regressions. Use SequenceEqual when order matters, SetEqual when it doesn't, but be aware that SetEqual may not flag duplicate entries.
Format test diagnostics as they come from test output. Diagnostics typically have 2 lines of comments and 1 line of code. Keep diagnostics formatted with .WithLocation on the same line as Diagnostic(, matching the style produced by test infrastructure. This keeps style consistent and makes future updates produce smaller diffs.
For example:
// (1,12): error CS0029: Cannot implicitly convert type 'int' to 'string'
// string s = 42;
Diagnostic(ErrorCode.ERR_NoImplicitConv, "42").WithArguments("int", "string").WithLocation(1, 12)
Test metadata scenarios that can't be produced by C#. Consider testing scenarios with metadata containing combinations that C# can't produce.
Comments should explain why, not restate code. Delete comments like // Get the symbol that just duplicate the code in English. Focus on explaining non-obvious decisions, trade-offs, or algorithm choices.
Delete or update obsolete comments when code changes. Stale comments describing old behavior are worse than no comments.
Move important implementation comments to doc comments on the property/method. If a comment explains critical behavior of a property or method (e.g., "returns true until X is called"), it belongs in the XML doc comment, not buried inside the implementation.
Track deferred work with GitHub issues. No // TODO comment should be added or merged. Instead use a comment like // Tracked by <URL>: description to link to a GitHub issue tracking the follow-up work.
Add XML doc comments on all new public APIs in product code. These seed the official API documentation on learn.microsoft.com.
New public APIs require approved proposals before PR submission. All new compiler or workspace API surface must go through API review. PRs cannot be merged until the APIs are approved. See more details about the process at API Review Process
Public API changes direct tests at the definition layer. When modifying or adding public APIs, include tests that exercise the definition layer directly.
Flag breaking changes and require formal process. Any time a change causes code that compiled with a shipped version of the compiler to now produce a diagnostic, we should consider whether it should be reviewed and documented as a breaking change. Depending on the impact, such as how much time passed for users to adopt the code pattern, we may need explicit approval from the compat council.
File breaking change documentation for behavioral changes. Open an issue in dotnet/docs if changing public API behavior, even in preview releases.
Install and validate a .NET SDK from an Azure DevOps internal build. Use this when asked to install, validate, or set up a .NET SDK from a dnceng/internal Azure DevOps build link or build ID.
Update the agent knowledge base after making code changes in the Roslyn repo. Run at the end of every task that modifies code, adds files, changes public APIs or diagnostics, or establishes new patterns. Keeps .github/memory/ fresh and reliable.
Create or modify Roslyn IDE analyzers, code fixes, and code refactorings. Use when: adding a new IDE diagnostic (IDE0xxx), implementing a CodeFixProvider, implementing a CodeRefactoringProvider, writing analyzer/fixer tests, or working with AbstractBuiltInCodeStyleDiagnosticAnalyzer. Also use for: diagnostic analyzer, code action, FixAllProvider, TestInRegularAndScriptAsync, TestMissingInRegularAndScriptAsync.
Perform a branch snap (release branch cut) for dotnet repos like dotnet/roslyn. Use when: snapping a branch, cutting a release branch, creating a release branch, merging main into release, updating VS insertion config, updating darc subscriptions for a snap, moving milestones, or asked about snap workflow.
Import Razor formatting log zips that the user has already downloaded from Azure DevOps feedback tickets or GitHub issues into FormattingLogTest, validate whether the captured problem still reproduces, and if needed drive a minimal repro plus fix workflow.
Set up tracking for a new C# compiler feature: create test plan issue, feature label, update Language Feature Status page, and link related PRs. Use when: setting up a new language feature, creating a test plan, tracking a new csharplang proposal in roslyn, or asked to 'set up feature tracking'.