|
| 1 | +--- |
| 2 | +excludeAgent: coding-agent |
| 3 | +--- |
| 4 | + |
| 5 | +# Code Review Instructions for dotnet/runtime |
| 6 | + |
| 7 | +These instructions guide code reviews for the dotnet/runtime repository. A compiler runs on every PR (as do a wealth of static analyzers for C# changes), so focus on higher-level concerns that require expert judgment rather than stylistic or syntactic issues. |
| 8 | + |
| 9 | +## Review Priorities |
| 10 | + |
| 11 | +### 1. Security |
| 12 | + |
| 13 | +**Critical Security Concerns:** |
| 14 | +- **Input Validation & Sanitization**: Ensure all external inputs (user data, file I/O, network data) are properly validated and sanitized before use |
| 15 | +- **Injection Vulnerabilities**: Check for potential SQL injection, command injection, path traversal, or code injection risks |
| 16 | +- **Cryptographic Operations**: Verify proper use of cryptographic APIs, secure random number generation, and correct handling of keys/certificates |
| 17 | +- **Buffer Overflows**: In native code, check for proper bounds checking and safe memory operations |
| 18 | +- **Authentication & Authorization**: Ensure proper access control checks are in place and cannot be bypassed |
| 19 | +- **Information Disclosure**: Watch for accidental logging or exposure of sensitive data (credentials, keys, PII) |
| 20 | +- **Denial of Service**: Check for potential infinite loops, resource exhaustion, or algorithmic complexity attacks |
| 21 | +- **Deserialization**: Ensure safe deserialization practices, especially with untrusted data |
| 22 | +- **Race Conditions**: Identify potential TOCTOU (time-of-check-time-of-use) vulnerabilities in security-sensitive operations |
| 23 | + |
| 24 | +### 2. Performance |
| 25 | + |
| 26 | +**Performance Considerations:** |
| 27 | +- **Algorithmic Complexity**: Identify inefficient algorithms (O(n²) where O(n) is possible, unnecessary allocations) |
| 28 | +- **Memory Allocations**: Watch for excessive allocations in hot paths, consider stack allocation (stackalloc) or object pooling where appropriate |
| 29 | +- **Boxing**: Identify unnecessary boxing of value types |
| 30 | +- **String Operations**: Check for string concatenation in loops (use StringBuilder), excessive string allocations |
| 31 | +- **LINQ Performance**: Evaluate LINQ usage in performance-critical paths; consider more direct alternatives |
| 32 | +- **Async/Await Overhead**: Ensure async/await is used appropriately (not for trivial synchronous operations) |
| 33 | +- **Collection Choices**: Verify appropriate collection types for access patterns (List vs. HashSet vs. Dictionary) |
| 34 | +- **Lazy Initialization**: Check for opportunities to defer expensive operations |
| 35 | +- **Caching**: Identify repeated expensive computations that could be cached |
| 36 | +- **Span<T> and Memory<T>**: Encourage use of modern memory-efficient types for buffer manipulation |
| 37 | +- **Native Interop**: Ensure P/Invoke calls are efficient and properly marshaled |
| 38 | + |
| 39 | +### 3. Backwards Compatibility |
| 40 | + |
| 41 | +**Compatibility Requirements:** |
| 42 | +- **Public API Changes**: Any change to public APIs requires careful scrutiny |
| 43 | + - Breaking changes are generally not acceptable |
| 44 | + - New optional parameters, overloads, and interface implementations need careful consideration |
| 45 | + - Verify that API additions follow existing patterns and naming conventions |
| 46 | +- **Serialization Compatibility**: Ensure changes don't break serialization/deserialization of persisted data |
| 47 | +- **Configuration Changes**: Changes to configuration format or behavior must maintain backwards compatibility |
| 48 | +- **Binary Compatibility**: IL-level changes must preserve binary compatibility for existing assemblies |
| 49 | +- **Behavioral Changes**: Even non-breaking API changes can break consumers if behavior changes unexpectedly |
| 50 | + - Document behavioral changes clearly |
| 51 | + - Consider feature flags or opt-in mechanisms for significant behavior changes |
| 52 | +- **Obsolete APIs**: Check that proper obsolescence process is followed (attributes, documentation, migration path) |
| 53 | + |
| 54 | +### 4. Cross-Component Interactions |
| 55 | + |
| 56 | +**Integration Points:** |
| 57 | +- **Runtime/Library Boundaries**: Changes affecting CoreCLR, Mono, or NativeAOT must work across all runtimes |
| 58 | +- **Platform Differences**: Ensure changes work correctly across Windows, Linux, macOS, and other supported platforms |
| 59 | +- **Architecture Considerations**: Verify behavior is correct on x86, x64, ARM32, and ARM64 |
| 60 | +- **Dependencies**: Changes to core libraries may impact higher-level libraries; consider cascading effects |
| 61 | +- **Threading Models**: Ensure thread-safety is maintained and synchronization primitives are used correctly |
| 62 | +- **Lifecycle Management**: Verify proper initialization, disposal patterns, and cleanup across component boundaries |
| 63 | +- **Shared State**: Carefully review any shared mutable state for thread-safety and consistency |
| 64 | +- **Error Handling**: Ensure exceptions and error codes are properly propagated across component boundaries |
| 65 | + |
| 66 | +### 5. Correctness and Edge Cases |
| 67 | + |
| 68 | +**Code Correctness:** |
| 69 | +- **Null Handling**: While the compiler enforces nullable reference types, verify runtime null checks are appropriate |
| 70 | +- **Boundary Conditions**: Test for off-by-one errors, empty collections, null inputs, maximum values |
| 71 | +- **Error Paths**: Ensure error handling is correct and complete; resources are properly cleaned up |
| 72 | +- **Concurrency**: Identify race conditions, deadlocks, or improper synchronization |
| 73 | +- **Exception Safety**: Verify operations maintain invariants even when exceptions occur |
| 74 | +- **Resource Management**: Ensure IDisposable is implemented correctly and resources are not leaked |
| 75 | +- **Numeric Overflow**: Check for potential integer overflow/underflow in calculations |
| 76 | +- **Culture/Locale Issues**: Verify culture-invariant operations where appropriate, proper localization otherwise |
| 77 | +- **Time Handling**: Check for timezone, daylight saving, and leap second handling issues |
| 78 | + |
| 79 | +### 6. Design and Architecture |
| 80 | + |
| 81 | +**Design Quality:** |
| 82 | +- **API Design**: Ensure new APIs are intuitive, follow framework design guidelines, and are hard to misuse |
| 83 | +- **Abstraction Level**: Verify abstractions are at the appropriate level and don't leak implementation details |
| 84 | +- **Separation of Concerns**: Check that responsibilities are properly separated |
| 85 | +- **Extensibility**: Consider whether the design allows for future extension without breaking changes |
| 86 | +- **SOLID Principles**: Evaluate adherence to single responsibility, open/closed, and other design principles |
| 87 | +- **Code Duplication**: Identify opportunities to reduce duplication while maintaining clarity |
| 88 | +- **Testability**: Ensure the code is designed to be testable (proper dependency injection, separation of concerns) |
| 89 | + |
| 90 | +### 7. Testing |
| 91 | + |
| 92 | +**Test Quality:** |
| 93 | +- **Coverage**: Ensure new functionality has appropriate test coverage |
| 94 | +- **Test Scenarios**: Verify tests cover happy paths, error paths, and edge cases |
| 95 | +- **Test Reliability**: Watch for flaky tests (timing dependencies, environmental assumptions) |
| 96 | +- **Test Performance**: Ensure tests run efficiently and don't unnecessarily slow down CI |
| 97 | +- **Platform-Specific Tests**: Verify platform-specific tests are properly conditioned |
| 98 | +- **Regression Tests**: Check that bugs being fixed have corresponding regression tests |
| 99 | + |
| 100 | +### 8. Documentation and Code Clarity |
| 101 | + |
| 102 | +**Documentation:** |
| 103 | +- **XML Documentation**: New public APIs must have clear XML documentation explaining purpose, parameters, return values, and exceptions. Do not comment on existing APIs that lack documentation. |
| 104 | +- **Complex Logic**: Comments should explain the "why" behind non-obvious decisions, not restate what the code does |
| 105 | +- **TODOs and FIXMEs**: Ensure they are tracked with issues and are appropriate for the change |
| 106 | +- **Breaking Changes**: Must be clearly documented with migration guidance |
| 107 | + |
| 108 | +## What NOT to Focus On |
| 109 | + |
| 110 | +The following are handled by automated tooling and don't need review comments: |
| 111 | + |
| 112 | +- Code formatting and style (handled by `.editorconfig` and analyzers) |
| 113 | +- Naming convention violations (handled by analyzers) |
| 114 | +- Missing using directives (handled by compiler) |
| 115 | +- Most syntax errors (handled by compiler) |
| 116 | +- Simple code style preferences without technical merit |
| 117 | + |
| 118 | +## Review Approach |
| 119 | + |
| 120 | +1. **Understand the Context**: Read the PR description and linked issues to understand the goal. Consider as much relevant code from the containing project as possible. For public APIs, review any code in the repo that consumes the method. |
| 121 | +2. **Assess the Scope**: Verify the change is focused and not mixing unrelated concerns |
| 122 | +3. **Evaluate Risk**: Consider the risk level based on what components are affected and how widely used they are |
| 123 | +4. **Think Like an Attacker**: For security-sensitive code, consider how it might be exploited |
| 124 | +5. **Think Like a Consumer**: Consider how the API will be used and potentially misused |
| 125 | +6. **Consider Maintenance**: Think about long-term maintenance burden and technical debt |
| 126 | + |
| 127 | +## Severity Guidelines |
| 128 | + |
| 129 | +- **Critical**: Security vulnerabilities, data corruption, crashes, breaking changes |
| 130 | +- **High**: Performance regressions, resource leaks, incorrect behavior in common scenarios |
| 131 | +- **Medium**: Edge case bugs, suboptimal design, missing documentation |
| 132 | +- **Low**: Code clarity issues, minor inefficiencies, nice-to-have improvements |
0 commit comments