review-code

name	review-code
description	Perform comprehensive csharp/dotnet code reviews focusing on clean code, security, testing, performance, and documentation

Code Review - C# and .NET

Overview

This skill provides comprehensive code review guidelines for GitHub Copilot focused on C# and .NET development. It follows industry best practices and provides a structured approach to evaluating code quality, security, testing, performance, and architecture.

What This Skill Does:

• Performs systematic code reviews using priority-based checklists
• Identifies critical security vulnerabilities and correctness issues
• Evaluates test coverage and quality
• Checks for performance bottlenecks and anti-patterns
• Validates documentation and code clarity
• Ensures compliance with project .editorconfig standards

When to Apply: Use this skill when reviewing pull requests, conducting code audits, or validating code quality before merge. This skill is on-demand (not automatic) - invoke it explicitly when performing code reviews.

Review Priorities

When performing a code review, prioritize issues in the following order:

🔴 CRITICAL (Block merge)

Issues that must be fixed before merging. These represent serious risks to security, correctness, or system stability.

• Security Vulnerabilities:

  • Exposed secrets (API keys, passwords, connection strings hardcoded in code)
  • Authentication/authorization bypasses or weaknesses
  • Injection vulnerabilities (SQL injection, command injection)
  • Insecure cryptography or weak random number generation
  • Missing input validation on external data

• Correctness Issues:

  • Logic errors that cause incorrect behavior
  • Data corruption risks (improper transaction handling, race conditions)
  • Null reference exceptions (missing null checks on nullable types)
  • Off-by-one errors, incorrect loop bounds
  • Type conversion errors that lose data

• Breaking Changes:

  • Public API contract changes without versioning
  • Breaking interface modifications without migration path
  • Removal of public members still in use

• Data Loss Risks:

  • Operations that can permanently delete data without confirmation
  • Missing database transactions for multi-step operations
  • Improper error handling that could corrupt state

• Resource Management:

  • Undisposed IDisposable objects (DbContext, streams, HttpClient)
  • Memory leaks in long-running processes (event handler leaks, static references)
  • Connection pool exhaustion (improper connection management)

• .editorconfig Violations (ERROR level):

  • File-scoped namespace violations (csharp_style_namespace_declarations = file_scoped:error)
  • var usage violations (csharp_style_var_*:error)
  • Using directive placement violations (csharp_using_directive_placement = inside_namespace:error)

🟡 IMPORTANT (Requires discussion)

Issues that should be addressed but may not block merge if there's a valid reason to defer.

• Code Quality:

  • Severe SOLID principle violations (God classes, tight coupling)
  • Excessive code duplication (copy-paste code in multiple locations)
  • Overly complex methods (cyclomatic complexity > 10, nesting > 3 levels)
  • Anemic domain models (classes with only properties, no behavior)

• Test Coverage:

  • Missing tests for critical business logic paths
  • No tests for new functionality being added
  • Tests that don't actually verify behavior (assertion-free tests)
  • Brittle tests with tight coupling to implementation details

• Performance Issues:

  • Obvious bottlenecks (N+1 queries, synchronous I/O in loops)
  • Inefficient algorithms (O(n²) when O(n) is possible)
  • Boxing/unboxing in hot paths
  • Excessive allocations (string concatenation in loops)

• Architecture Violations:

  • Cross-layer dependencies (breaking Clean Architecture boundaries)
  • Circular dependencies between components
  • Improper separation of concerns

• Async/Await Issues:

  • Blocking on async code (.Result, .Wait()) - can cause deadlocks
  • Missing async propagation (sync wrapper around async code)
  • Async void methods (except event handlers)
  • Missing CancellationToken parameters in async methods

🟢 SUGGESTION (Non-blocking improvements)

Improvements that enhance code quality but don't require immediate action.

• Readability:

  • Poor naming (unclear variable/method names)
  • Complex expressions that could be simplified
  • Missing intermediate variables for clarity
  • Long parameter lists (> 4 parameters)

• Optimization Opportunities:

  • Places where Span<T> could improve performance
  • Opportunities for ValueTask<T> over Task<T>
  • Collection expressions (C# 12+) for cleaner syntax
  • ReadOnlySpan<T> for string operations

• Best Practices:

  • Minor deviations from C# conventions
  • Opportunities for readonly fields
  • Missing const for compile-time constants
  • Inconsistent code style (handled by formatters)

• Documentation:

  • Missing XML documentation comments for public APIs
  • Inadequate <summary> descriptions
  • Missing <param> or <returns> tags
  • No examples for complex APIs

• Modern C# Features:

  • Opportunities to use pattern matching
  • Record types for immutable data
  • File-scoped namespaces (if not already enforced)
  • Collection expressions
  • Primary constructors

General Review Principles

Follow these principles when conducting code reviews:

1. Be Specific

• Reference exact locations: File names, line numbers, method names
• Quote the code: Include the problematic code snippet in your comment
• Avoid vague statements: Instead of "this could be better", say "this method has cyclomatic complexity of 15, consider extracting the validation logic into separate methods"

2. Provide Context

• Explain WHY: Don't just say what's wrong, explain the impact
• Describe consequences: "This can cause a deadlock in ASP.NET contexts"
• Link to resources: Reference documentation, ADRs, or best practice articles
• Consider the bigger picture: How does this fit into the overall architecture?

3. Suggest Solutions

• Show corrected code: Provide a "CORRECT" example alongside the "WRONG" code
• Offer alternatives: "Consider using Pattern A or Pattern B"
• Explain trade-offs: "This approach is faster but uses more memory"
• Make it actionable: Give clear steps to fix the issue

4. Be Constructive

• Focus on the code, not the person: "This method could be simplified" not "You wrote this poorly"
• Assume positive intent: The author likely has reasons for their approach
• Ask questions: "Have you considered...?" rather than "You should..."
• Acknowledge constraints: Time, knowledge, or requirements may limit options

5. Recognize Good Practices

• Highlight excellent code: "Great use of the specification pattern here!"
• Acknowledge improvements: "This is much cleaner than the previous version"
• Learn from others: Good code reviews benefit both reviewer and author
• Build team knowledge: Share interesting patterns you discover

6. Be Pragmatic

• Not everything needs immediate fixing: Distinguish between must-fix and nice-to-have
• Consider the PR scope: Don't demand unrelated refactoring
• Balance perfection and progress: "This could be optimized further, but it's acceptable for now"
• Technical debt is okay: Sometimes it's appropriate to defer improvements

7. Group Related Comments

• Avoid scattered feedback: Group all comments about the same issue
• Provide a summary: "There are 5 places with similar error handling issues"
• Use checklists: Link to relevant checklists for systematic issues
• Offer to pair program: For complex issues, offer to work together

Quick Reference to Checklists

Use these checklists for systematic code review:

Code Quality Checklist

File: checklists/01-code-quality.md

Focus Areas:

• ✅ Naming conventions (PascalCase, camelCase, interfaces with I prefix)
• ✅ File-scoped namespaces (🔴 MANDATORY - enforced by .editorconfig)
• ✅ var usage (🔴 MANDATORY - enforced by .editorconfig)
• ✅ Using directive placement (🔴 MANDATORY - inside namespace)
• ✅ Method size and complexity (< 20 lines, < 3 nesting levels)
• ✅ DRY principle (no code duplication)
• ✅ Magic numbers/strings (extract to constants)
• ✅ Modern C# features (pattern matching, records, collection expressions)

When to Use: Every code review should check these fundamentals.

Security Checklist

File: checklists/02-security.md

Focus Areas:

• ✅ No hardcoded secrets (🔴 CRITICAL - connection strings, API keys, passwords)
• ✅ Configuration usage (IConfiguration, IOptions)
• ✅ Input validation (validate all external inputs)
• ✅ SQL injection prevention (🔴 CRITICAL - parameterized queries only)
• ✅ Cryptography best practices (use BCL classes, no custom crypto)
• ✅ Dependency vulnerabilities (keep packages updated)

When to Use: Always check when code handles sensitive data, external inputs, or database operations.

Testing Checklist

File: checklists/03-testing.md

Focus Areas:

• ✅ Test coverage (> 80% for critical paths)
• ✅ Test naming (Should_ExpectedBehavior_When_Condition)
• ✅ AAA pattern (Arrange-Act-Assert with clear separation)
• ✅ Test independence (no shared state)
• ✅ Edge cases (null, empty, boundary conditions)
• ✅ Mocking strategies (NSubstitute for dependencies)
• ✅ xUnit patterns (Fact vs Theory, Shouldly assertions)

When to Use: When reviewing test code or checking if new features have adequate tests.

Performance Checklist

File: checklists/04-performance.md

Focus Areas:

• ✅ Async/await usage (for I/O-bound operations)
• ✅ No blocking calls (🔴 CRITICAL - avoid .Result, .Wait())
• ✅ CancellationToken propagation (include in async methods)
• ✅ Resource disposal (🔴 CRITICAL - using statements for IDisposable)
• ✅ String operations (StringBuilder in loops)
• ✅ Collection choices (List, Dictionary, HashSet)
• ✅ Span opportunities (high-performance scenarios)

When to Use: When reviewing performance-critical code, async code, or long-running operations.

Documentation Checklist

File: checklists/05-documentation.md

Focus Areas:

• ✅ XML documentation on public APIs (🔴 MANDATORY - <summary>, <param>, <returns>)
• ✅ Exception documentation (<exception> tags) only for public methods and methods that throw and do not return a Result or Result
• ✅ Complex logic comments (explain WHY, not WHAT)
• ✅ README updates (document new features)
• ✅ API documentation (OpenAPI/Swagger summaries)

When to Use: When reviewing public APIs or complex domain logic.

.editorconfig Integration

This project enforces code style through .editorconfig rules. The following rules are MANDATORY (error level) and violations are 🔴 CRITICAL:

File-Scoped Namespaces (🔴 CRITICAL)

Rule: csharp_style_namespace_declarations = file_scoped:error

What it means: All C# files must use file-scoped namespace syntax (not block-scoped).

Example:

// 🔴 WRONG: Block-scoped namespace (CRITICAL VIOLATION)
namespace MyApp.Services {
    public class PaymentService { }
}

// ✅ CORRECT: File-scoped namespace (MANDATORY)
namespace MyApp.Services;

public class PaymentService { }

Why it matters: File-scoped namespaces reduce indentation, improve readability, and are the modern C# convention (C# 10+).

How to fix: Convert all block-scoped namespaces to file-scoped. Run dotnet format to auto-fix.

Var Usage (🔴 CRITICAL)

Rules:

• csharp_style_var_elsewhere = true:error
• csharp_style_var_for_built_in_types = true:error
• csharp_style_var_when_type_is_apparent = true:error

What it means: Always use var for local variables (unless there's a specific reason not to).

Example:

// 🔴 WRONG: Explicit type when obvious (CRITICAL VIOLATION)
Customer customer = new Customer();
int count = 42;
List<string> names = new List<string>();

// ✅ CORRECT: Use var (MANDATORY)
var customer = new Customer();
var count = 42;
var names = new List<string>();

Why it matters: var reduces verbosity, improves maintainability (when types change), and is the modern C# convention.

How to fix: Replace explicit types with var. Run dotnet format to auto-fix.

Using Directive Placement (🔴 CRITICAL)

Rule: csharp_using_directive_placement = inside_namespace:error

What it means: using directives must be placed inside the namespace (after the namespace declaration).

Example:

// 🔴 WRONG: Using directives outside namespace (CRITICAL VIOLATION)
using System;
using System.Collections.Generic;

namespace MyApp.Services;

public class PaymentService { }

// ✅ CORRECT: Using directives inside namespace (MANDATORY)
namespace MyApp.Services;

using System;
using System.Collections.Generic;

public class PaymentService { }

Why it matters: Placement inside namespace prevents naming conflicts and follows project conventions.

How to fix: Move using directives after the namespace declaration. Run dotnet format to auto-fix.

How to Verify Compliance

Run formatter:

dotnet format

This will automatically fix most .editorconfig violations including the three CRITICAL rules above.

Check for violations (dry run):

dotnet format --verify-no-changes

This reports violations without modifying files. Use in CI/CD pipelines to block PRs with violations.

Common .editorconfig Violations

See examples/editorconfig-compliance.md for detailed examples of:

• File-scoped namespace violations and fixes
• Var usage violations and fixes
• Using directive placement violations and fixes
• Other common style violations

Quick Reference to Examples

Use these example files for code patterns:

Clean Code Examples

File: examples/clean-code-examples.md

Contains:

• File-scoped namespace patterns (WRONG vs CORRECT)
• var usage patterns
• Expression-bodied members
• Pattern matching
• Guard clauses
• Private setters (encapsulation)
• Null-conditional operators
• String interpolation
• Collection expressions

When to Use: Reference when reviewing code style, naming, or C# idioms.

Security Examples

File: examples/security-examples.md

Contains:

• Hardcoded secrets (WRONG vs CORRECT)
• SQL injection prevention
• Configuration patterns (IConfiguration, IOptions)
• Input validation (FluentValidation)
• API key management

When to Use: Reference when reviewing security-sensitive code.

Testing Examples

File: examples/testing-examples.md

Contains:

• Test naming patterns (WRONG vs CORRECT)
• AAA pattern examples
• Fact vs Theory usage
• Shouldly assertions
• NSubstitute mocking patterns

When to Use: Reference when reviewing test code or suggesting test improvements.

.editorconfig Compliance Examples

File: examples/editorconfig-compliance.md

Contains:

• Detailed .editorconfig rule explanations
• File-scoped namespace violations and fixes
• Var usage violations and fixes
• Using directive placement violations and fixes
• How to verify compliance with dotnet format

When to Use: Reference when identifying or fixing .editorconfig violations.

Review Templates

Use these templates for consistent, actionable review comments.

Comment Template

File: templates/review-comment-template.md

Provides:

• 🔴 CRITICAL comment format (Issue, Why This Matters, Suggested Fix, Reference)
• 🟡 IMPORTANT comment format
• 🟢 SUGGESTION comment format
• Examples for each priority level

When to Use: When writing review comments to ensure they're specific, contextual, and actionable.

Summary Template

File: templates/review-summary-template.md

Provides:

• Issues Found (count by priority: 🔴🟡🟢)
• Top 3 Priorities
• Overall Assessment
• Next Steps checklist

When to Use: At the end of a review to summarize findings and provide clear next steps.

Example Workflow

Here's a step-by-step workflow for conducting a comprehensive code review:

Step 1: Start with Critical Issues (🔴)

Focus: Security, correctness, resource management, .editorconfig ERROR violations

Process:

1. Scan for hardcoded secrets (search for "password", "apikey", "connectionstring")
2. Check for SQL injection risks (look for string concatenation in queries)
3. Verify resource disposal (IDisposable objects have using statements)
4. Check for blocking async calls (search for .Result, .Wait())
5. Verify .editorconfig compliance (file-scoped namespaces, var usage, using placement)

Action: Flag any CRITICAL issues immediately. These must be fixed before merge.

Step 2: Review Code Quality (🟡)

Focus: SOLID principles, test coverage, architecture, async patterns

Process:

1. Check method complexity (are methods < 20 lines, < 3 nesting levels?)
2. Look for code duplication (is the same logic repeated in multiple places?)
3. Verify test coverage (are new features tested? Are critical paths covered?)
4. Check async/await usage (is CancellationToken propagated? Is async used for I/O?)
5. Review architecture boundaries (are layer dependencies correct?)

Action: Discuss IMPORTANT issues with the author. Decide if they block merge or can be deferred.

Step 3: Suggest Improvements (🟢)

Focus: Readability, modern C# features, documentation, optimizations

Process:

1. Check naming (are variables/methods clearly named?)
2. Look for modern C# opportunities (pattern matching, records, collection expressions)
3. Verify documentation (do public APIs have XML comments?)
4. Identify optimization opportunities (Span, ValueTask)
5. Check for readability improvements (complex expressions that could be simplified)

Action: Provide SUGGESTIONS for improvements. Mark as non-blocking.

Step 4: Use Checklists

Reference the appropriate checklists based on the code being reviewed:

• All reviews: Code Quality Checklist
• Sensitive data/external inputs: Security Checklist
• Test code: Testing Checklist
• Async code/performance-critical: Performance Checklist
• Public APIs: Documentation Checklist

Step 5: Provide Examples

When suggesting changes, reference the example files:

• Style issues: Reference examples/clean-code-examples.md
• Security issues: Reference examples/security-examples.md
• Test issues: Reference examples/testing-examples.md
• .editorconfig violations: Reference examples/editorconfig-compliance.md

Step 6: Write Comments Using Templates

Use the comment template (templates/review-comment-template.md) to structure feedback:

• Start with priority emoji (🔴🟡🟢)
• Include: Issue, Why This Matters, Suggested Fix, Reference
• Provide corrected code examples
• Link to documentation or examples

Step 7: Summarize Findings

Use the summary template (templates/review-summary-template.md) to wrap up:

• List issues found by priority
• Highlight top 3 priorities
• Provide overall assessment
• List actionable next steps

Example Review Comment

🔴 CRITICAL - Security: Hardcoded Database Password

**Issue**: Line 42 contains a hardcoded database password in the connection string.

**Why This Matters**: Hardcoded secrets in source code are a critical security vulnerability. They are visible in version control, can be accidentally exposed, and cannot be rotated without code changes.

**Suggested Fix**:
```csharp
// Current (WRONG):
var connectionString = "Server=myserver;Database=mydb;User Id=admin;Password=secret123;";

// Corrected (CORRECT):
var connectionString = this.configuration.GetConnectionString("MyDatabase");

Then store the actual connection string in:

• appsettings.json (for local dev, not committed)
• Azure Key Vault (for production)
• Environment variables (for containers)

Reference: See examples/security-examples.md for more secure configuration patterns.

## Integration with Other Skills

This skill works well with other repository skills:

### domain-add-aggregate
When reviewing new domain aggregates, use both skills:
- **review-code**: Check code quality, security, testing
- **domain-add-aggregate**: Verify DDD patterns, layer boundaries, aggregate structure

### review-architecture
For architectural reviews, use both skills:
- **review-code**: Focus on implementation details, security, performance
- **review-architecture**: Focus on DDD patterns, Clean Architecture boundaries, CQRS

### adr-writer
When identifying architectural issues, reference ADRs or create new ones:
- Use **review-code** to identify patterns that should be documented
- Use **adr-writer** to document architectural decisions

## Core Rules

1. **ALWAYS start with CRITICAL issues** (🔴): Security and correctness come first
2. **ALWAYS check .editorconfig compliance**: File-scoped namespaces, var usage, using placement are MANDATORY
3. **ALWAYS provide context**: Explain WHY something is an issue, not just WHAT
4. **ALWAYS suggest solutions**: Show corrected code, don't just point out problems
5. **ALWAYS be specific**: Reference exact locations, methods, line numbers
6. **ALWAYS use priority indicators**: 🔴🟡🟢 to clearly communicate severity
7. **ALWAYS be constructive**: Focus on improving code, not criticizing the author
8. **NEVER demand unrelated refactoring**: Keep feedback scoped to the PR
9. **NEVER block on style issues**: Use formatters (`dotnet format`) for style consistency
10. **NEVER forget to recognize good code**: Acknowledge excellent practices when you see them

## When to Use This Skill

**Use this skill when**:
- Reviewing pull requests before merge
- Conducting code quality audits
- Onboarding new team members (teach review standards)
- Preparing for production releases (final quality check)
- Identifying technical debt (catalog issues for future work)

**DO NOT use this skill when**:
- Code is generated (migrations, scaffolding)
- Code is third-party/vendored (not under your control)
- Review is purely architectural (use `review-architecture` instead)
- Quick bug fix in emergency (defer review to post-fix PR)

## Success Criteria

A successful code review includes:

✅ **All CRITICAL issues identified and fixed** (🔴)
✅ **IMPORTANT issues discussed with author** (🟡)
✅ **Suggestions provided for future improvements** (🟢)
✅ **.editorconfig compliance verified** (file-scoped namespaces, var usage, using placement)
✅ **Security vulnerabilities caught** (hardcoded secrets, SQL injection, input validation)
✅ **Test coverage adequate** (critical paths tested, new features have tests)
✅ **Performance bottlenecks identified** (no blocking async, proper resource disposal)
✅ **Documentation complete** (public APIs have XML comments)
✅ **Feedback is specific and actionable** (includes corrected code examples)
✅ **Review summary provided** (using template, with prioritized next steps)

## Additional Resources

- **Checklists**: See `checklists/` for systematic review guides
- **Examples**: See `examples/` for WRONG vs CORRECT code patterns
- **Templates**: See `templates/` for structured comment and summary formats
- **Project .editorconfig**: See root `.editorconfig` for complete style rules
- **C# Coding Conventions**: [Microsoft Docs](https://docs.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions)
- **.NET Best Practices**: [Microsoft Docs](https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/)