dotnet · jonathanpeppers · May 1, 2026 · May 1, 2026 · May 1, 2026 · May 1, 2026
@@ -60,7 +60,17 @@ Review the CI results. **Never post ✅ LGTM if any required CI check is failing
 
 ### 5. Load review rules
 
-Read `references/review-rules.md` from this skill's directory.
+Based on the changed files from step 2, load the appropriate rule files from `references/`:
+
+**Always load:**
+- `references/repo-conventions.md` — repo-specific patterns and conventions
+- `references/ai-pitfalls.md` — common AI code generation mistakes
+- `references/security-rules.md` — security review checklist
+
+**Conditionally load:**
+- `references/csharp-rules.md` — if any `.cs` files changed
+- `references/msbuild-rules.md` — if any `.targets`, `.props`, or `.projitems` files changed, or if changed `.cs` files are under `src/Microsoft.Android.Build.BaseTasks/`
+- `references/testing-rules.md` — if any files under `tests/` changed or files with `Test` in the path changed
 
 ### 6. Analyze the diff
 

@@ -0,0 +1,23 @@
+# AI Code Generation Pitfalls
+
+Patterns that AI-generated code consistently gets wrong. Always loaded during reviews.
+
+---
+
+| Pattern | What to watch for |
+|---------|------------------|
+| **Reinventing the wheel** | AI creates new infrastructure instead of using existing utilities. ALWAYS check if a similar utility exists before accepting new wrapper code. This is the most expensive AI pattern — hundreds of lines of plausible code that duplicates what's already there. |
+| **Over-engineering** | HttpClient injection "for testability", elevation auto-detection, speculative helper classes, unused overloads. If no caller needs it today, remove it. |
+| **Swallowed errors** | AI catch blocks love to eat exceptions silently. Check EVERY catch block. Also check that exit codes are checked consistently. |
+| **Ignoring target framework** | AI generates code for the newest .NET. Check every API call against the oldest supported target framework. |
+| **Sloppy structure** | Multiple types in one file, block-scoped namespaces, #region directives, classes where records would do. New helpers marked `public` when `internal` suffices. |
+| **Confidently wrong domain facts** | AI once claimed `ANDROID_SDK_ROOT` was the recommended variable (it's deprecated). Always verify domain-specific claims against official docs. |
+| **Over-mocking** | Not everything needs to be mocked. Integration tests with `Assert.Ignore` on failure are fine and catch real API changes that mocks never will. |
+| **Docs describe intent not reality** | AI doc comments often describe what the code *should* do, not what it *actually* does. Review doc comments against the implementation. |
+| **Unused parameters** | AI adds `CancellationToken` parameters but never observes them, or accepts `additionalArgs` as a string and interpolates it into a command. Unused CancellationToken is a broken contract; string args are injection risks. |
+| **Null-forgiving operator (`!`)** | The postfix `!` null-forgiving operator (e.g., `foo!.Bar`) is banned in most codebases. If the value can be null, add a proper null check. If it can't be null, make the parameter/variable non-nullable. AI frequently sprinkles `!` to silence the compiler — this turns compile-time warnings into runtime `NullReferenceException`s. Use `IsNullOrEmpty()` extension methods or null-coalescing instead. Note: this rule is about the postfix `!` operator, not the logical negation `!` (e.g., `if (!someBool)` or `if (!string.IsNullOrEmpty (s))`). |
+| **`Debug.WriteLine` for logging** | AI catch blocks often log with `System.Diagnostics.Debug.WriteLine()` or `Console.WriteLine()` — neither integrates with most codebase logger patterns. |
+| **`git commit --amend`** | AI uses `--amend` on commits that are already pushed or belong to another author. Always create new commits — the maintainer will squash as needed. |
+| **Commit messages omit non-obvious choices** | Behavioral decisions and known limitations belong in the commit message, not just the code. |
+| **Typos in user-visible strings** | Users copy-paste error messages into bug reports. Get them right. |
+| **Filler words in docs** | "So" at the start of a sentence adds nothing. Be direct. |
@@ -0,0 +1,106 @@
+# C# Review Rules
+
+General C# and .NET guidance for code reviews. Applicable to any .NET repository.
+
+---
+
+## 1. Target Framework Compatibility
+
+When a library multi-targets (e.g., `netstandard2.0` + a modern TFM), every API call must work on all targets.
+
+| Check | What to look for |
+|-------|-----------------|
+| **netstandard2.0 API surface** | Methods/overloads that only exist on net5+. Common traps: `HttpContent.ReadAsStringAsync(CancellationToken)`, `ProcessStartInfo.ArgumentList`, `Environment.IsPrivilegedProcess`, `ArrayPool<T>` (needs `System.Buffers` package on ns2.0). When unsure, check MS Learn docs. |
+| **C# language features** | `init` accessors, `required` keyword, file-scoped types, raw string literals — may need polyfills or `#if` guards. |
+| **Conditional compilation** | New API usage should be behind `#if NET5_0_OR_GREATER` (or similar) with a fallback for netstandard2.0. |
+
+---
+
+## 2. Async & Cancellation Patterns
+
+| Check | What to look for |
+|-------|-----------------|
+| **CancellationToken propagation** | Every `async` method that accepts a `CancellationToken` must pass it to ALL downstream async calls (`GetAsync`, `ReadAsStreamAsync`, `SendAsync`, etc.). A token that's accepted but never used is a broken contract. |
+| **OperationCanceledException** | Catch-all blocks (`catch (Exception)`) must NOT swallow `OperationCanceledException`. Either catch it explicitly first and rethrow, or use a type filter. |
+| **GetStringAsync** | On netstandard2.0, `GetStringAsync(url)` doesn't accept a `CancellationToken`. Use `GetAsync(url, ct)` + `ReadAsStringAsync()` instead. |
+| **Thread safety of shared state** | If a new field or property can be accessed from multiple threads (e.g., static caches, event handlers), verify thread-safe access: `ConcurrentDictionary`, `Interlocked`, or explicit locks. A `Dictionary<K,V>` read concurrently with a write is undefined behavior. |
+| **Lock ordering** | If code acquires multiple locks, the order must be consistent everywhere. Document the ordering. Inconsistent ordering → deadlock. |
+| **Avoid double-checked locking** | The double-checked locking (DCL) pattern is error-prone and [discouraged by Microsoft](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile). Prefer `Lazy<T>` or `LazyInitializer.EnsureInitialized()` — they handle thread-safe initialization correctly. If DCL is truly necessary (e.g., for performance in a hot path with evidence), verify all initialization completes before the field is assigned and no thread can observe a partially-initialized instance. |
+| **Singleton initialization completeness** | When a singleton is initialized behind a lock, ensure ALL setup steps (not just construction) complete before publishing the instance. If `Initialize()` does `instance = new Foo(); instance.Setup();`, another thread can see `instance != null` and use it before `Setup()` runs. Either do all setup in the constructor, or don't publish the reference until all setup is done. |
+
+---
+
+## 3. Resource Management
+
+| Check | What to look for |
+|-------|-----------------|
+| **HttpClient must be static** | `HttpClient` instances should be `static readonly` fields, not per-instance. Creating/disposing `HttpClient` leads to socket exhaustion via `TIME_WAIT` accumulation. See [Microsoft guidelines](https://learn.microsoft.com/dotnet/fundamentals/networking/http/httpclient-guidelines). |
+| **No HttpClient injection (YAGNI)** | Don't add `HttpClient` constructor parameters "for testability" unless a caller actually needs it today. The AI tends to over-engineer this. |
+| **ArrayPool for large buffers** | Buffers ≥ 1KB (especially 80KB+ download buffers) should use `ArrayPool<byte>.Shared.Rent()` with `try/finally` return. Large allocations go to the LOH and are expensive to GC. |
+| **IDisposable** | Classes that own unmanaged resources or expensive managed resources must implement `IDisposable` with a dispose guard (`ThrowIfDisposed`). |
+
+---
+
+## 4. Error Handling
+
+| Check | What to look for |
+|-------|-----------------|
+| **No empty catch blocks** | Every `catch` must capture the `Exception` and log it (or rethrow). No silent swallowing. Even "expected" exceptions should be logged for diagnostics. |
+| **Validate parameters** | Enum parameters and string-typed "mode" values must be validated — throw `ArgumentException` or `NotSupportedException` for unexpected values. Don't silently accept garbage. |
+| **Fail fast on critical ops** | If an operation like `chmod` or checksum verification fails, throw immediately. Silently continuing leads to confusing downstream failures. |
+| **Mandatory verification** | Checksum/hash verification must NOT be optional. If the checksum can't be fetched, the operation must fail — not proceed unverified. |
+| **Include actionable details in exceptions** | Use `nameof` for parameter names. Include the unsupported value or unexpected type. Never throw empty exceptions. |
+| **Log messages must have context** | A bare `"Operation failed"` could be anything. Include *what* you were doing and relevant identifiers so the message is actionable without a debugger. |
+| **Differentiate similar error messages** | Two messages saying `"X failed"` for different operations are impossible to debug. Make each unique so log output is unambiguous. |
+| **Assert boundary invariants** | If a name=value pair array must have even length, assert `(length % 2) == 0` before indexing `[i+1]`. Validate structural assumptions at the boundary, not deep inside the logic. |
+| **Initialize output parameters in all paths** | Methods with `out` parameters must initialize them in all error paths, not just the success path. |
+| **Use `ThrowIf` helpers where available** | Prefer `ArgumentOutOfRangeException.ThrowIfNegative`, `ArgumentNullException.ThrowIfNull`, etc. over manual if-then-throw patterns where these helpers are available. In `netstandard2.0` projects where these helpers are unavailable, use explicit checks such as `if (x is null) throw new ArgumentNullException (nameof (x));`. |
+| **Challenge exception swallowing** | When a PR adds `catch { continue; }` or `catch { return null; }`, question whether the exception is truly expected or masking a deeper problem. The default should be to let unexpected exceptions propagate. |
+
+---
+
+## 5. Performance
+
+| Check | What to look for |
+|-------|-----------------|
+| **XmlReader over LINQ XML** | For forward-only XML parsing (manifests, config files), use `XmlReader` — it's streaming and allocation-free. `XElement`/`XDocument` builds a full DOM tree. |
+| **p/invoke over process spawn** | For single syscalls like `chmod`, use `[DllImport("libc")]` instead of spawning a child process. Process creation is orders of magnitude more expensive. |
+| **Avoid intermediate collections** | Don't create two lists and `AddRange()` one to the other. Build a single list, or use LINQ to chain. |
+| **Cache reusable arrays** | Char arrays for `string.Split()` (like whitespace chars) should be `static readonly` fields, not allocated on each call. |
+| **Pre-allocate collections when size is known** | Use `new List<T>(capacity)` or `new Dictionary<TK, TV>(count)` when the size is known or estimable. Repeated resizing is O(n) allocation waste. |
+| **Avoid closures in hot paths** | Lambdas that capture local variables allocate a closure object on every call. In loops or frequently-called methods, extract the lambda to a static method or cache the delegate. |
+| **Place cheap checks before expensive ones** | In validation chains, test simple conditions (null checks, boolean flags) before allocating strings or doing I/O. Short-circuit with `&&`/`||`. |
+| **Watch for O(n²)** | Nested loops over the same or related collections, repeated `.Contains()` on a `List<T>`, or LINQ `.Where()` inside a loop are O(n²). Switch to `HashSet<T>` or `Dictionary<TK, TV>` for lookups. |
+| **`HashSet.Add()` already handles duplicates** | Calling `.Contains()` before `.Add()` does the hash lookup twice. Just call `.Add()` — it returns `false` if the item already exists. |
+| **Don't wrap a value in an interpolated string** | `$"{someString}"` creates an unnecessary `string.Format` call when `someString` is already a string. Just use `someString` directly. |
+| **`Split()` with count parameter** | `line.Split(new char[]{'='}, 2)` prevents values containing `=` from being split incorrectly. Follow existing patterns that limit the split count. |
+| **Cache repeated accessor calls** | If `foo.Bar.Baz` is used multiple times in a block, assign it to a local. This avoids repeated property evaluation and makes intent clearer. |
+| **Extract throw helpers** | Code like `if (x) throw new SomeException(...)` in a frequently-called method prevents inlining. Extract into a `[DoesNotReturn]` helper so the JIT can inline the happy path. |
+
+---
+
+## 6. Code Organization
+
+| Check | What to look for |
+|-------|-----------------|
+| **File-scoped namespaces** | New files should use `namespace Foo;` (not `namespace Foo { ... }`). Don't reformat existing files. |
+| **Use `record` for data types** | Immutable data-carrier types (progress, version info, license info) should be `record` types. They get value equality, `ToString()`, and deconstruction for free. |
+| **Remove unused code** | Dead methods, speculative helpers, and code "for later" should be removed. Ship only what's needed. |
+| **No commented-out code** | Commented-out code should be deleted — Git has history. |
+| **Reduce indentation with early returns** | Invert conditions with `continue` or early `return` to reduce nesting. `foreach (var x in items ?? Array.Empty<T>())` eliminates null-check nesting. |
+| **Don't initialize fields to default values** | `bool flag = false;` and `int count = 0;` are noise. The CLR zero-initializes all fields. Only assign when the initial value is non-default. |
+| **`sealed` classes skip full Dispose** | A `sealed` class doesn't need `Dispose(bool)` + `GC.SuppressFinalize`. Just implement `IDisposable.Dispose()` directly. The full pattern is only for unsealed base classes. |
+| **Use interfaces over concrete types** | Fields and parameters should prefer interfaces (`IMetadataResolver`) over concrete classes. When the implementation changes, you swap the implementation — not every call site. |
+
+---
+
+## 7. API Design
+
+| Check | What to look for |
+|-------|-----------------|
+| **Return `IReadOnlyList<T>` not `List<T>`** | Public methods should return `IReadOnlyList<T>` (or `IReadOnlyCollection<T>`) instead of mutable `List<T>`. Prevents callers from mutating internal state. |
+| **New helpers default to `internal`** | New utility methods should be `internal` unless a confirmed external consumer needs them. Use `InternalsVisibleTo` for test access. |
+| **Structured args, not string interpolation** | Additional arguments to processes should be `IEnumerable<string>`, not a single `string` that gets interpolated. |
+| **Honor `CancellationToken`** | If a method accepts a `CancellationToken`, it MUST observe it — register a callback to kill processes, check `IsCancellationRequested` in loops, pass it to downstream async calls. Don't just accept it for API completeness. |
+| **Add overloads to reduce caller ceremony** | If every caller performs the same conversion before calling a method, the method should have an overload that accepts the unconverted type directly. |
+| **Prefer C# pattern matching** | Use `is`, `switch` expressions, and property patterns instead of `if`/`else` type-check chains. Pattern matching is more concise, avoids casts, and enables exhaustiveness checks. |
@@ -0,0 +1,31 @@
+# MSBuild Review Rules
+
+Guidance for MSBuild tasks, targets, and props files. Applicable to any .NET repository with custom MSBuild infrastructure.
+
+---
+
+## 1. Task Logging
+
+| Check | What to look for |
+|-------|-----------------|
+| **Don't use `Debug.WriteLine` or `Console.WriteLine`** | MSBuild tasks must use the task's logging facilities (e.g., `Log.LogMessage`, `Log.LogWarning`, `Log.LogError`). `Debug.WriteLine()` only reaches attached debuggers (invisible in CI). `Console.WriteLine()` bypasses MSBuild's logging pipeline entirely. |
+| **Use appropriate log levels** | Use `MessageImportance.Low` for verbose diagnostics, `MessageImportance.Normal` for progress, and `MessageImportance.High` for important status. Don't spam high-importance messages. |
+
+---
+
+## 2. Process Management in Tasks
+
+| Check | What to look for |
+|-------|-----------------|
+| **Don't redirect stdout/stderr without draining** | Background processes with `RedirectStandardOutput = true` must have async readers draining the output. Otherwise the OS pipe buffer fills and the child process deadlocks. For fire-and-forget processes, set `Redirect* = false`. |
+| **Check exit codes consistently** | If one task operation checks the process exit code, ALL similar operations must too. Inconsistent error checking creates a false sense of safety. |
+| **Include stdout in error diagnostics** | When a task captures stdout, pass it to error reporting so failure messages include all output, not just stderr. |
+
+---
+
+## 3. Downstream Coordination
+
+| Check | What to look for |
+|-------|-----------------|
+| **Port, don't rewrite** | If a downstream consumer already has working logic for the same task, port it rather than writing new code. The existing code has real-world edge cases already handled. |
+| **Draft downstream PR before merging** | Shared library changes should be accompanied by a draft PR in the consuming repo that proves the API actually works. Merge the library first, update the submodule pointer, then merge the consumer. |