From fb8d4b25e1832d89a672f9a6d5e363ab360441ae Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Wed, 11 Mar 2026 13:38:52 -0500 Subject: [PATCH 1/4] [skills] Incorporate review feedback from dotnet/android#10918 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix submit_review.cs issues found during porting: - Replace GetString()! with ?? "" to avoid null-forgiving operator - Add null check for Process.Start() instead of ! - Narrow bare catch blocks to catch (JsonException) - Restructure success output so catch block isn't misleading Merge C#-focused review rules back into review-rules.md: - Async: thread safety of shared state - Error handling: actionable exception details, challenge exception swallowing - Code org: no commented-out code, early returns, skip default field init - Performance: pre-allocate collections, avoid closures, cheap checks first, O(n²) - Patterns: method names reflect behavior, why-not-what comments, track TODOs - YAGNI: git commit --amend, commit message quality, typos, filler words - New section 13: Testing (regression tests, specific assertions, deterministic data, edge cases) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../skills/android-tools-reviewer/SKILL.md | 47 ++++++++++++++----- .../references/review-rules.md | 29 ++++++++++++ .../scripts/submit_review.cs | 23 +++++---- 3 files changed, 77 insertions(+), 22 deletions(-) diff --git a/.github/skills/android-tools-reviewer/SKILL.md b/.github/skills/android-tools-reviewer/SKILL.md index 188c4048..5c222cf8 100644 --- a/.github/skills/android-tools-reviewer/SKILL.md +++ b/.github/skills/android-tools-reviewer/SKILL.md @@ -12,6 +12,15 @@ description: >- Review PRs against guidelines distilled from past reviews by senior maintainers. +## Review Mindset + +Be polite but skeptical. Prioritize bugs, performance regressions, safety issues, and pattern violations over style nitpicks. **3 important comments > 15 nitpicks.** + +Flag severity clearly in every comment: +- ❌ **error** — Must fix before merge. Bugs, security issues, broken patterns. +- ⚠️ **warning** — Should fix. Performance issues, missing validation, inconsistency with patterns. +- 💡 **suggestion** — Consider changing. Style, readability, optional improvements. + ## Workflow ### 1. Parse the PR URL @@ -19,18 +28,30 @@ Review PRs against guidelines distilled from past reviews by senior maintainers. Extract `owner`, `repo`, `pr_number` from the URL. Formats: `https://github.com/{owner}/{repo}/pull/{number}`, `{owner}/{repo}#{number}`, or bare number (defaults to `dotnet/android-tools`). -### 2. Fetch PR data +### 2. Gather context (before reading PR description) ``` -gh pr view {number} --repo {owner}/{repo} --json title,body,files gh pr diff {number} --repo {owner}/{repo} +gh pr view {number} --repo {owner}/{repo} --json files +``` + +For each changed file, read the **full source file** (not just the diff) to understand surrounding invariants, call patterns, and data flow. If the change modifies a public/internal API or utility, search for callers. Check whether sibling types need the same fix. + +**Form an independent assessment** of what the change does and what problems it has *before* reading the PR description. + +### 3. Incorporate PR narrative and reconcile + ``` +gh pr view {number} --repo {owner}/{repo} --json title,body +``` + +Now read the PR description and linked issues. Treat them as claims to verify, not facts to accept. Where your independent reading disagrees with the PR description, investigate further. If the PR claims a performance improvement, require evidence. If it claims a bug fix, verify the bug exists and the fix addresses root cause — not symptoms. -### 3. Load review rules +### 4. Load review rules Read `references/review-rules.md` from this skill's directory. -### 4. Analyze the diff +### 5. Analyze the diff For each changed file, check against the review rules. Record issues as: @@ -41,23 +62,25 @@ For each changed file, check against the review rules. Record issues as: Constraints: - Only comment on added/modified lines in the diff — the API rejects out-of-range lines. - `line` = line number in the NEW file (right side). Double-check against the diff. -- High signal only: 3 important comments > 15 nitpicks. Skip style/formatting unless it violates a specific rule. - One issue per comment. +- **Don't pile on.** If the same issue appears many times, flag it once with a note listing all affected files. +- **Don't flag what CI catches.** Skip compiler errors, formatting the linter will catch, etc. +- **Avoid false positives.** Verify the concern actually applies given the full context. If unsure, phrase it as a question rather than a firm claim. -### 5. Build the review JSON +### 6. Build the review JSON Write a temp JSON file: ```json { "event": "COMMENT", - "body": "## 🤖 AI Review Summary\n\nFound **N issues**: ...\n\n- **Category**: description (`file:line`)\n\n👍 Positive callouts.\n\n---\n_Review generated by android-tools-reviewer from [review guidelines](../../../docs/CODE_REVIEW_POSTMORTEM.md) by @jonathanpeppers._", + "body": "## 🤖 AI Review Summary\n\n**Verdict**: ✅ LGTM | ⚠️ Needs Changes | ❌ Reject\n\nFound **N issues**: ...\n\n- ❌ **Category**: description (`file:line`)\n- ⚠️ **Category**: description (`file:line`)\n\n👍 Positive callouts.\n\n---\n_Review generated by android-tools-reviewer from [review guidelines](../../../docs/CODE_REVIEW_POSTMORTEM.md) by @jonathanpeppers._", "comments": [ { "path": "src/Example.cs", "line": 42, "side": "RIGHT", - "body": "🤖 **Error handling** — Every `catch` should capture the `Exception` and log it.\n\n_Rule: No empty catch blocks (Postmortem `#11`)_" + "body": "🤖 ❌ **Error handling** — Every `catch` should capture the `Exception` and log it.\n\n_Rule: No empty catch blocks (Postmortem `#11`)_" } ] } @@ -65,7 +88,7 @@ Write a temp JSON file: If no issues found, submit with empty `comments` and a positive summary. -### 6. Submit as a single batch +### 7. Submit as a single batch ```powershell dotnet run {skill-dir}/scripts/submit_review.cs {owner} {repo} {number} {path-to-json} @@ -76,11 +99,11 @@ The script validates structure (required fields, 🤖 prefix, positive line numb ## Comment format ``` -🤖 **{Category}** — {What's wrong and what to do instead.} +🤖 {severity} **{Category}** — {What's wrong and what to do instead.} _{Rule: Brief name (Postmortem `#N`)}_ ``` -Always wrap `#N` in backticks so GitHub doesn't auto-link to issues. +Where `{severity}` is ❌, ⚠️, or 💡. Always wrap `#N` in backticks so GitHub doesn't auto-link to issues. -**Categories:** Target framework · Async pattern · Resource management · Error handling · Security · Code organization · Naming · Performance · Pattern · YAGNI · API design · Code duplication · Documentation +**Categories:** Target framework · Async pattern · Resource management · Error handling · Security · Code organization · Naming · Performance · Pattern · YAGNI · API design · Code duplication · Testing · Documentation diff --git a/.github/skills/android-tools-reviewer/references/review-rules.md b/.github/skills/android-tools-reviewer/references/review-rules.md index f1a9b67e..fc7e0839 100644 --- a/.github/skills/android-tools-reviewer/references/review-rules.md +++ b/.github/skills/android-tools-reviewer/references/review-rules.md @@ -27,6 +27,7 @@ Framework. Every API call must work on both targets. | **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` read concurrently with a write is undefined behavior. | **Postmortem refs:** #5, #37 @@ -53,6 +54,8 @@ Framework. Every API call must work on both targets. | **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 ("permission denied" when the real problem was chmod). | | **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. | +| **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. | **Postmortem refs:** #11, #20, #22, #35 @@ -80,6 +83,9 @@ Framework. Every API call must work on both targets. | **No #region directives** | `#region` hides code and makes reviews harder. Remove them. This also applies to banner/section-separator comments (e.g., `// --- Device Tests ---`) — they serve the same purpose as `#region` and signal the file should be split instead. | | **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())` 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. | **Postmortem refs:** #9, #12, #25, #28, #56 @@ -106,6 +112,10 @@ Framework. Every API call must work on both targets. | **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(capacity)` or `new Dictionary(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`, or LINQ `.Where()` inside a loop are O(n²). Switch to `HashSet` or `Dictionary` for lookups. | **Postmortem refs:** #8, #14, #21, #31 @@ -121,6 +131,10 @@ Framework. Every API call must work on both targets. | **Version-based directories** | Install SDK/JDK to versioned paths (`cmdline-tools/19.0/`, not `cmdline-tools/latest/`). Versioned paths are self-documenting and allow side-by-side installs. | | **Safe directory replacement** | Use move-with-rollback: rename existing → temp, move new in place, validate, delete temp only after validation succeeds. Never delete the backup before confirming the new install works. | | **Cross-volume moves** | `Directory.Move` is really a rename — it fails across filesystems. Extract archives near the target path (same parent directory), or catch `IOException` and fall back to recursive copy + delete. | +| **Method names must reflect behavior** | If `CreateFoo()` sometimes returns an existing instance, rename it `GetOrCreateFoo()` or `GetFoo()`. | +| **Comments explain "why", not "what"** | `// increment i` adds nothing. `// skip the BOM — aapt2 chokes on it` explains intent. If a comment restates the code, delete it. | +| **Track TODOs as issues** | A `// TODO` hidden in code will be forgotten. File an issue and reference it in the comment. | +| **Remove stale comments** | If the code changed, update the comment. Comments that describe old behavior are misleading. | **Postmortem refs:** #15, #16, #23, #36, #38 @@ -142,6 +156,10 @@ These are patterns that AI-generated code consistently gets wrong: | **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 (`!`)** | Never use `!` to suppress nullable warnings. 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 make the compiler happy — this turns compile-time warnings into runtime `NullReferenceException`s. Use `IsNullOrEmpty()` extension methods or null-coalescing instead. | +| **`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. | **Postmortem refs:** #7, #28, #29, #40, #41, #42, #49, #50, #51, #52, #54 @@ -172,3 +190,14 @@ These are patterns that AI-generated code consistently gets wrong: | **Check exit codes consistently** | If one operation (`ListDevicesAsync`) checks the process exit code, ALL similar operations (`StopEmulatorAsync`, `WaitForDeviceAsync`) must too. Inconsistent error checking creates a false sense of safety. | **Postmortem refs:** #42, #43, #44, #45, #48 + +--- + +## 13. Testing + +| Check | What to look for | +|-------|-----------------| +| **Bug fixes need regression tests** | Every PR that fixes a bug should include a test that fails without the fix and passes with it. If the PR description says "fixes #N" but adds no test, ask for one. | +| **Test assertions must be specific** | `Assert.IsNotNull(result)` or `Assert.IsTrue(success)` don't tell you what went wrong. Prefer `Assert.AreEqual(expected, actual)` or NUnit constraints (`Assert.That` with `Does.Contain`, `Is.EqualTo`, etc.) for richer failure messages. | +| **Deterministic test data** | Tests should not depend on system locale, timezone, or current date. Use explicit `CultureInfo.InvariantCulture` and hardcoded dates when testing formatting. | +| **Test edge cases** | Empty collections, null inputs, boundary values, and very large inputs should all be considered. If the PR only tests the happy path, suggest edge cases. | diff --git a/.github/skills/android-tools-reviewer/scripts/submit_review.cs b/.github/skills/android-tools-reviewer/scripts/submit_review.cs index 66314d14..2a970259 100644 --- a/.github/skills/android-tools-reviewer/scripts/submit_review.cs +++ b/.github/skills/android-tools-reviewer/scripts/submit_review.cs @@ -43,7 +43,7 @@ if (!root.TryGetProperty ("event", out var eventProp) || eventProp.ValueKind != JsonValueKind.String) { errors.Add ("Missing or invalid 'event' field — must be COMMENT, APPROVE, or REQUEST_CHANGES"); } else { - var ev = eventProp.GetString ()!; + var ev = eventProp.GetString () ?? ""; if (ev != "COMMENT" && ev != "APPROVE" && ev != "REQUEST_CHANGES") errors.Add ($"Invalid event '{ev}' — must be COMMENT, APPROVE, or REQUEST_CHANGES"); } @@ -65,11 +65,11 @@ if (!c.TryGetProperty ("body", out var cbody) || string.IsNullOrWhiteSpace (cbody.GetString ())) errors.Add ($"{prefix}: missing or empty 'body'"); - else if (!cbody.GetString ()!.StartsWith ("🤖")) + else if (!(cbody.GetString () ?? "").StartsWith ("🤖")) errors.Add ($"{prefix}: body must start with 🤖 prefix"); if (c.TryGetProperty ("side", out var sideProp) && sideProp.ValueKind == JsonValueKind.String) { - var side = sideProp.GetString ()!; + var side = sideProp.GetString () ?? ""; if (side != "LEFT" && side != "RIGHT") errors.Add ($"{prefix}: 'side' must be LEFT or RIGHT, got '{side}'"); } @@ -103,7 +103,11 @@ psi.ArgumentList.Add ("--input"); psi.ArgumentList.Add (jsonPath); -var process = Process.Start (psi)!; +var process = Process.Start (psi); +if (process is null) { + Console.Error.WriteLine ("❌ Failed to start 'gh' — is it installed and on PATH?"); + return 1; +} var stdoutTask = process.StandardOutput.ReadToEndAsync (); var stderrTask = process.StandardError.ReadToEndAsync (); process.WaitForExit (); @@ -119,21 +123,20 @@ using var errDoc = JsonDocument.Parse (stdout); if (errDoc.RootElement.TryGetProperty ("message", out var msg)) Console.Error.WriteLine ($" GitHub says: {msg.GetString ()}"); - } catch { + } catch (JsonException) { Console.Error.WriteLine (stdout); } } return 1; } +Console.WriteLine ("✅ Review posted."); try { using var resp = JsonDocument.Parse (stdout); if (resp.RootElement.TryGetProperty ("html_url", out var url)) - Console.WriteLine ($"✅ Review posted: {url.GetString ()}"); - else - Console.WriteLine ("✅ Review posted."); -} catch { - Console.WriteLine ("✅ Review posted."); + Console.WriteLine ($" {url.GetString ()}"); +} catch (JsonException) { + // Response wasn't JSON — the review was still posted, we just can't show the URL. } } // using (doc) From 429ef768b28b088903a53ab43d091a05c22d81c0 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Wed, 11 Mar 2026 13:52:56 -0500 Subject: [PATCH 2/4] Port review feedback from dotnet/android#10918 submit_review.cs: - Add ValueKind checks for path/body in comment validation to prevent InvalidOperationException on non-string JSON values - Treat non-array 'comments' field as a validation error instead of silently skipping - Log a note when API response is not JSON instead of silent comment review-rules.md: - Add ThrowIf helpers rule noting unavailability on netstandard2.0 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../references/review-rules.md | 1 + .../scripts/submit_review.cs | 55 +++++++++++-------- 2 files changed, 33 insertions(+), 23 deletions(-) diff --git a/.github/skills/android-tools-reviewer/references/review-rules.md b/.github/skills/android-tools-reviewer/references/review-rules.md index fc7e0839..6f8c6971 100644 --- a/.github/skills/android-tools-reviewer/references/review-rules.md +++ b/.github/skills/android-tools-reviewer/references/review-rules.md @@ -55,6 +55,7 @@ Framework. Every API call must work on both targets. | **Fail fast on critical ops** | If an operation like `chmod` or checksum verification fails, throw immediately. Silently continuing leads to confusing downstream failures ("permission denied" when the real problem was chmod). | | **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. | +| **Use `ThrowIf` helpers where available** | In .NET 10+ projects, prefer `ArgumentOutOfRangeException.ThrowIfNegative`, `ArgumentNullException.ThrowIfNull`, etc. over manual if-then-throw patterns. 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. | **Postmortem refs:** #11, #20, #22, #35 diff --git a/.github/skills/android-tools-reviewer/scripts/submit_review.cs b/.github/skills/android-tools-reviewer/scripts/submit_review.cs index 2a970259..7603d2ed 100644 --- a/.github/skills/android-tools-reviewer/scripts/submit_review.cs +++ b/.github/skills/android-tools-reviewer/scripts/submit_review.cs @@ -52,29 +52,38 @@ errors.Add ("Missing or empty 'body' field (the review summary)"); } -if (root.TryGetProperty ("comments", out var commentsProp) && commentsProp.ValueKind == JsonValueKind.Array) { - int i = 0; - foreach (var c in commentsProp.EnumerateArray ()) { - var prefix = $"comments[{i}]"; - - if (!c.TryGetProperty ("path", out var pathProp) || string.IsNullOrEmpty (pathProp.GetString ())) - errors.Add ($"{prefix}: missing 'path'"); - - if (!c.TryGetProperty ("line", out var lineProp) || lineProp.ValueKind != JsonValueKind.Number || lineProp.GetInt32 () < 1) - errors.Add ($"{prefix}: 'line' must be a positive integer"); - - if (!c.TryGetProperty ("body", out var cbody) || string.IsNullOrWhiteSpace (cbody.GetString ())) - errors.Add ($"{prefix}: missing or empty 'body'"); - else if (!(cbody.GetString () ?? "").StartsWith ("🤖")) - errors.Add ($"{prefix}: body must start with 🤖 prefix"); - - if (c.TryGetProperty ("side", out var sideProp) && sideProp.ValueKind == JsonValueKind.String) { - var side = sideProp.GetString () ?? ""; - if (side != "LEFT" && side != "RIGHT") - errors.Add ($"{prefix}: 'side' must be LEFT or RIGHT, got '{side}'"); +if (root.TryGetProperty ("comments", out var commentsProp)) { + if (commentsProp.ValueKind != JsonValueKind.Array) { + errors.Add ("Invalid 'comments' field — must be an array"); + } else { + int i = 0; + foreach (var c in commentsProp.EnumerateArray ()) { + var prefix = $"comments[{i}]"; + + if (!c.TryGetProperty ("path", out var pathProp) || pathProp.ValueKind != JsonValueKind.String || string.IsNullOrEmpty (pathProp.GetString ())) + errors.Add ($"{prefix}: missing 'path'"); + + if (!c.TryGetProperty ("line", out var lineProp) || lineProp.ValueKind != JsonValueKind.Number || lineProp.GetInt32 () < 1) + errors.Add ($"{prefix}: 'line' must be a positive integer"); + + if (!c.TryGetProperty ("body", out var cbody) || cbody.ValueKind != JsonValueKind.String) { + errors.Add ($"{prefix}: missing or empty 'body'"); + } else { + var commentBody = cbody.GetString () ?? ""; + if (string.IsNullOrWhiteSpace (commentBody)) + errors.Add ($"{prefix}: missing or empty 'body'"); + else if (!commentBody.StartsWith ("🤖")) + errors.Add ($"{prefix}: body must start with 🤖 prefix"); + } + + if (c.TryGetProperty ("side", out var sideProp) && sideProp.ValueKind == JsonValueKind.String) { + var side = sideProp.GetString () ?? ""; + if (side != "LEFT" && side != "RIGHT") + errors.Add ($"{prefix}: 'side' must be LEFT or RIGHT, got '{side}'"); + } + + i++; } - - i++; } } @@ -136,7 +145,7 @@ if (resp.RootElement.TryGetProperty ("html_url", out var url)) Console.WriteLine ($" {url.GetString ()}"); } catch (JsonException) { - // Response wasn't JSON — the review was still posted, we just can't show the URL. + Console.WriteLine ("Note: API response was not JSON; review was posted but URL is unavailable."); } } // using (doc) From 8bfaa01ae7366bb93588b887cfda61150e40fb98 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Wed, 11 Mar 2026 13:53:57 -0500 Subject: [PATCH 3/4] Address PR review feedback - Dispose Process with 'using' to avoid leaking OS handles - Add source reference to Testing section (dotnet/android#10918) The ValueKind validation feedback was already addressed in the previous commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../skills/android-tools-reviewer/references/review-rules.md | 2 ++ .github/skills/android-tools-reviewer/scripts/submit_review.cs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/skills/android-tools-reviewer/references/review-rules.md b/.github/skills/android-tools-reviewer/references/review-rules.md index 6f8c6971..f1d87e13 100644 --- a/.github/skills/android-tools-reviewer/references/review-rules.md +++ b/.github/skills/android-tools-reviewer/references/review-rules.md @@ -202,3 +202,5 @@ These are patterns that AI-generated code consistently gets wrong: | **Test assertions must be specific** | `Assert.IsNotNull(result)` or `Assert.IsTrue(success)` don't tell you what went wrong. Prefer `Assert.AreEqual(expected, actual)` or NUnit constraints (`Assert.That` with `Does.Contain`, `Is.EqualTo`, etc.) for richer failure messages. | | **Deterministic test data** | Tests should not depend on system locale, timezone, or current date. Use explicit `CultureInfo.InvariantCulture` and hardcoded dates when testing formatting. | | **Test edge cases** | Empty collections, null inputs, boundary values, and very large inputs should all be considered. If the PR only tests the happy path, suggest edge cases. | + +**Source:** dotnet/android`#10918` diff --git a/.github/skills/android-tools-reviewer/scripts/submit_review.cs b/.github/skills/android-tools-reviewer/scripts/submit_review.cs index 7603d2ed..0cac123b 100644 --- a/.github/skills/android-tools-reviewer/scripts/submit_review.cs +++ b/.github/skills/android-tools-reviewer/scripts/submit_review.cs @@ -112,7 +112,7 @@ psi.ArgumentList.Add ("--input"); psi.ArgumentList.Add (jsonPath); -var process = Process.Start (psi); +using var process = Process.Start (psi); if (process is null) { Console.Error.WriteLine ("❌ Failed to start 'gh' — is it installed and on PATH?"); return 1; From 715e29fc445153f6dba106acbf920b78e5e6f22a Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Wed, 11 Mar 2026 14:14:29 -0500 Subject: [PATCH 4/4] Clarify null-forgiving rule applies to postfix ! not logical negation The rule was triggering false positives on logical negation like if (!someBool) and if (!string.IsNullOrEmpty(s)). Explicitly note the distinction. Ported from dotnet/android@062d57f4d214228fd706f9f8684b1ff6c75414eb Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../skills/android-tools-reviewer/references/review-rules.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/skills/android-tools-reviewer/references/review-rules.md b/.github/skills/android-tools-reviewer/references/review-rules.md index f1d87e13..4af9bee8 100644 --- a/.github/skills/android-tools-reviewer/references/review-rules.md +++ b/.github/skills/android-tools-reviewer/references/review-rules.md @@ -156,7 +156,7 @@ These are patterns that AI-generated code consistently gets wrong: | **Over-mocking** | Not everything needs to be mocked. Network 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 (`!`)** | Never use `!` to suppress nullable warnings. 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 make the compiler happy — this turns compile-time warnings into runtime `NullReferenceException`s. Use `IsNullOrEmpty()` extension methods or null-coalescing instead. | +| **Null-forgiving operator (`!`)** | The postfix `!` null-forgiving operator (e.g., `foo!.Bar`) is banned. 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 make the compiler happy — 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))`). | | **`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. |