Align rule wording with dotnet/android-tools and add 6 new rules

Cross-referenced with dotnet/android-tools#355 to align shared rules
and adopt new relevant ones.

New rules added (6):
- ai-pitfalls: Confidently wrong domain facts
- ai-pitfalls: Over-mocking
- ai-pitfalls: Debug.WriteLine for logging
- msbuild-rules: Use appropriate log levels (MessageImportance)
- msbuild-rules: Don't redirect stdout/stderr without draining
- msbuild-rules: Include stdout in error diagnostics

Wording aligned with android-tools for sharing:
- security: Zip Slip adds ZipFile.ExtractToDirectory() warning
- security: Path traversal adds directory boundary example
- security: Command injection adds "never interpolate" note
- security: Reorganized into Archive & Process subsections
- ai-pitfalls: Reinventing the wheel adds "hundreds of lines" emphasis
- ai-pitfalls: Swallowed errors adds exit code checking
- ai-pitfalls: Null-forgiving expanded with IsNullOrEmpty suggestion
- ai-pitfalls: Unused parameters adds injection risk note
- testing: Bug fixes uses generic "#N" placeholder
- testing: Assertions adds NUnit constraint examples

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: jonathanpeppers

.github/skills/android-reviewer/references/ai-pitfalls.md

@@ -1,23 +1,26 @@
-# AI-Specific Pitfalls
+# AI Code Generation Pitfalls
 
-Patterns that AI-generated code consistently gets wrong. Applicable to any
-repository using AI-assisted development. Always loaded during reviews.
+Patterns that AI-generated code consistently gets wrong. Always loaded during
+reviews.
 
 ---
 
 ## Common AI Mistakes
 
 | 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. |
+| **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", 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. |
-| **Null-forgiving operator** | AI sprinkles `!` everywhere to silence nullable warnings. This is banned in this repo. Use null checks, `IsNullOrEmpty()`, or make types non-nullable. |
+| **Swallowed errors** | AI catch blocks love to eat exceptions silently. Check EVERY catch block. Also check that exit codes are checked consistently. |
+| **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 type 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))`). |
 | **Wrong formatting** | AI generates standard C# formatting (no space before parens). This repo requires Mono style: `Foo ()`, `array [0]`. |
 | **`string.Empty` and `Array.Empty<T>()`** | AI defaults to these. Use `""` and `[]` instead. |
 | **Sloppy structure** | Multiple types in one file, block-scoped namespaces, `#region` directives, classes where records would do. New helpers marked `public` when `internal` suffices. |
 | **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. (Postmortem `#59`) |
-| **Unused parameters** | AI adds `CancellationToken` parameters but never observes them. Unused CancellationToken is a broken contract. |
+| **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. |
+| **Confidently wrong domain facts** | AI makes authoritative claims about platform-specific behavior that are wrong (e.g., claiming a deprecated env var is recommended). 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. |
+| **`Debug.WriteLine` for logging** | AI catch blocks often log with `System.Diagnostics.Debug.WriteLine()` or `Console.WriteLine()` — neither integrates with MSBuild or codebase logger patterns. Use the task's logging facilities instead. |
 | **Modifying localization files** | AI modifies non-English `.resx` or `.lcl` files. Only the main English resource files should be edited. |
 | **`git commit --amend`** | AI uses `--amend` on commits. Always create new commits — the maintainer will squash as needed. |
 | **Commit messages omit non-obvious choices** | Behavioral decisions ("styleable arrays are cached, not copied per-access") and known limitations ("this leaks N bytes on Android 9") belong in the commit message. (Postmortem `#13`, `#69`) |

.github/skills/android-reviewer/references/msbuild-rules.md

@@ -20,6 +20,16 @@ causes broken builds for every .NET Android developer.
 | **`[Required]` properties** | `[Required]` properties must be non-nullable with a default: `public string Foo { get; set; } = "";` or `public ITaskItem[] Bar { get; set; } = [];`. Non-`[Required]` and `[Output]` properties must be nullable (`string?`, `ITaskItem[]?`). Mark properties `[Required]` when the task crashes without them. (Postmortem `#51`) |
 | **`UsingTask` for internal tasks** | `<UsingTask/>` elements for `xa-prep-tasks` and `BootstrapTasks` (internal, not shipped) must use `TaskFactory="TaskHostFactory"` and `Runtime="NET"`. Do NOT add these attributes to shipped task definitions in `Xamarin.Android.Common.targets` or `Microsoft.Android.Sdk/*.targets`. |
 | **Caching with `RegisterTaskObject`** | Use `BuildEngine4.RegisterTaskObject()` (via the `RegisterTaskObjectAssemblyLocal()` extension method) instead of `static` variables for sharing data between tasks or across builds. Use `as` for casts to avoid `InvalidCastException`. Cache keys should include context that invalidates properly (device target, file path, version). Cache primitive/small values only. |
+| **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. |
+
+---
+
+## 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`. |
+| **Include stdout in error diagnostics** | When a task captures stdout, pass it to error reporting so failure messages include all output, not just stderr. |
 
 ---
 

.github/skills/android-reviewer/references/security-rules.md

@@ -1,14 +1,21 @@
 # Security Review Rules
 
-Security checks applicable to any repository handling file I/O, process
-execution, or user-supplied paths.
+Security checklist for code reviews. Applicable to any repository handling file
+I/O, archives, or process execution.
 
 ---
 
-## Security Checks
+## Archive & Path Safety
 
 | Check | What to look for |
 |-------|-----------------|
-| **Zip Slip protection** | Archive extraction must validate that every entry path, after `Path.GetFullPath()`, resolves under the destination directory. |
-| **Command injection** | Arguments passed to `Process.Start` must be sanitized. Use `ArgumentList` (not string interpolation into command strings). |
-| **Path traversal** | `StartsWith()` checks on paths must normalize with `Path.GetFullPath()` first. |
+| **Zip Slip protection** | Archive extraction must validate that every entry path, after `Path.GetFullPath()`, resolves under the destination directory. Never use `ZipFile.ExtractToDirectory()` for untrusted archives without entry-by-entry validation. |
+| **Path traversal** | `StartsWith()` checks on paths must normalize with `Path.GetFullPath()` first. A path like `C:\Program Files\..\Users\evil` bypasses naive prefix checks. Also check for directory boundary issues (`C:\Program FilesX` matching `C:\Program Files`). |
+
+---
+
+## Process & Command Safety
+
+| Check | What to look for |
+|-------|-----------------|
+| **Command injection** | Arguments passed to `Process.Start` must be sanitized. Use `ArgumentList` (not string interpolation into command strings). Never interpolate user/external input into command strings. |

.github/skills/android-reviewer/references/testing-rules.md

@@ -12,7 +12,7 @@ Guidance for test code. The repo-specific conventions (e.g., `BaseTest`,
 | **Inherit from `BaseTest`** | Test fixtures should inherit from `BaseTest` (provides `Root`, `TestName`, SDK paths, platform helpers). |
 | **NUnit conventions** | Use `[TestFixture]`, `[Test]`, `[NonParallelizable]` (for tests that hang without it). |
 | **Test with `dotnet-local`** | Tests must run via `dotnet-local.cmd`/`dotnet-local.sh` to use the locally built SDK. |
-| **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 #1234" 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 `StringAssert.Contains`. Use `Assert.That` with constraints for richer failure messages. |
+| **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, concurrent calls, and very large inputs should all be considered. If the PR only tests the happy path, suggest edge cases. |