[skills] Improve android-reviewer skill based on eval findings (#11075)

Changes to SKILL.md:
- Add guidance that reviews should produce inline comments even on
  clean PRs (use suggestions for code consolidation, missing tests, etc.)
- Add prioritized checklist for what to look for in diff analysis
- Clarify CI verdict consistency: internal pipeline failures still
  require Needs Changes verdict
- Document --dry-run flag for submit_review.cs

Changes to review-rules.md:
- Rename section 5 to 'Async, Cancellation & Thread Safety Patterns'
- Add 'Double-checked locking requires volatile' rule with detailed
  ARM64 memory model guidance
- Add 'Singleton initialization completeness' rule for publish-before-
  setup bugs

Changes to submit_review.cs:
- Add --dry-run flag that validates and pretty-prints review JSON
  without posting to GitHub
- Use Utf8JsonWriter for AOT-compatible JSON serialization

## Replace volatile guidance with Lazy<T>/LazyInitializer recommendation

The volatile keyword is discouraged by Microsoft's own documentation.
Update the DCL review rule to recommend Lazy<T> or LazyInitializer
instead, with a link to the MS Learn docs.

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

Author: 3 people

.github/skills/android-reviewer/SKILL.md

@@ -19,6 +19,8 @@ Flag severity clearly in every comment:
 - ⚠️ **warning** — Should fix. Performance issues, missing validation, inconsistency with patterns.
 - 💡 **suggestion** — Consider changing. Style, readability, optional improvements.
 
+**Every review should produce at least one inline comment.** Even clean PRs have opportunities for improvement — code consolidation, missing edge-case tests, perf micro-optimizations, or documentation gaps. Use 💡 suggestions for these. A review with zero comments appears superficial and misses the chance to share knowledge. Only omit inline comments if the PR is truly trivial (e.g., a 1-line typo fix or dependency bump).
+
 ## Workflow
 
 ### 1. Parse the PR URL
@@ -55,6 +57,7 @@ Review the CI results. **Never post ✅ LGTM if any required CI check is failing
 - Investigate the failure using the **azdo-build-investigator** skill (for Azure DevOps pipeline failures) or GitHub Actions job logs.
 - If the failure is caused by the PR's code changes, flag it as ❌ error.
 - If the failure is a known infrastructure issue or pre-existing flake unrelated to the PR, note it in the summary but still use ⚠️ Needs Changes — the PR isn't mergeable until CI is green.
+- If **all public CI checks pass** but only the internal `Xamarin.Android-PR` check is failing, still use ⚠️ Needs Changes with a note that the internal pipeline may need a re-run. Do not give ✅ LGTM.
 - If the PR description acknowledges the failure and documents a dependency (e.g., "blocked on X"), note it in the summary.
 
 ### 5. Load review rules
@@ -69,6 +72,15 @@ For each changed file, check against the review rules. Record issues as:
 { "path": "src/Example.cs", "line": 42, "side": "RIGHT", "body": "..." }
 ```
 
+**What to look for (in priority order):**
+1. **Bugs & correctness** — race conditions, null dereferences, off-by-one, logic errors
+2. **Safety** — thread safety, resource leaks, security vulnerabilities
+3. **Performance** — O(n²) patterns, unnecessary allocations, missing caches
+4. **Missing tests** — untested error paths, edge cases, missing regression tests for bug fixes
+5. **Code duplication** — near-identical methods that should be consolidated
+6. **Consistency** — dedup patterns mixed within the same PR, API return types inconsistent with repo conventions
+7. **Documentation** — misleading comments, undocumented behavioral decisions
+
 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.
@@ -96,17 +108,17 @@ Write a temp JSON file:
 }
 ```
 
-If no issues found **and CI is green**, submit with empty `comments` and a positive summary.
+If no issues found **and CI is green**, submit with at most one or two 💡 suggestions and a positive summary. Truly trivial PRs (dependency bumps, 1-line typo fixes) may have empty `comments`.
 
 **Copilot-authored PRs:** If the PR author is `Copilot` (the GitHub Copilot coding agent) and the verdict is ⚠️ Needs Changes or ❌ Reject, prefix the review `body` with `@copilot ` so the comment automatically triggers Copilot to address the feedback. Do NOT add the prefix for ✅ LGTM verdicts.
 
 ### 8. Submit as a single batch
 
 ```powershell
-dotnet run {skill-dir}/scripts/submit_review.cs {owner} {repo} {number} {path-to-json}
+dotnet run {skill-dir}/scripts/submit_review.cs {owner} {repo} {number} {path-to-json} [--dry-run]
 ```
 
-The script validates structure (required fields, 🤖 prefix, positive line numbers) then calls `gh api`. Delete the temp file after success.
+The script validates structure (required fields, 🤖 prefix, positive line numbers) then calls `gh api`. Pass `--dry-run` to validate and print the review JSON without submitting to GitHub. Delete the temp file after success.
 
 ## Comment format
 

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

@@ -76,7 +76,7 @@ and merge conflicts.
 
 ---
 
-## 5. Async & Cancellation Patterns
+## 5. Async, Cancellation & Thread Safety Patterns
 
 | Check | What to look for |
 |-------|-----------------|
@@ -85,6 +85,8 @@ and merge conflicts.
 | **Honor the token** | If a method accepts `CancellationToken`, it must observe it — register a callback to kill processes, check `IsCancellationRequested` in loops, pass it downstream. Don't accept it just for API completeness. |
 | **Thread safety of shared state** | If a new field or property can be accessed from multiple threads (e.g., static caches, event handlers, `AsyncTask` callbacks), 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 — use `Lazy<T>` or `LazyInitializer`** | 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). It requires subtle memory model understanding and is easy to get wrong. Prefer `Lazy<T>` or `LazyInitializer.EnsureInitialized()` — they handle thread-safe initialization correctly. If a PR introduces or modifies a DCL pattern, flag it and suggest `Lazy<T>` instead. If DCL is truly necessary (e.g., for performance in a hot path with evidence), verify: (1) all initialization (including post-construction setup like `RegisterNatives()`) completes *before* the field is assigned, (2) there is no window where another thread can observe the singleton before initialization is fully done. |
+| **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. |
 
 ---
 

.github/skills/android-reviewer/scripts/submit_review.cs

@@ -1,24 +1,35 @@
-// Usage: dotnet run submit_review.cs <owner> <repo> <pr_number> <review_json_path>
+// Usage: dotnet run submit_review.cs <owner> <repo> <pr_number> <review_json_path> [--dry-run]
 //
 // Validates the review JSON structure, then submits it as a batched PR review
 // via `gh api`. Requires the `gh` CLI to be installed and authenticated.
+//
+// --dry-run: Validate and print the review JSON without submitting to GitHub.
 
 using System;
 using System.Diagnostics;
 using System.IO;
 using System.Text.Json;
 
-if (args.Length != 4)
+if (args.Length < 4 || args.Length > 5)
 {
-	Console.Error.WriteLine ($"Usage: dotnet run submit_review.cs <owner> <repo> <pr_number> <review_json_path>");
+	Console.Error.WriteLine ($"Usage: dotnet run submit_review.cs <owner> <repo> <pr_number> <review_json_path> [--dry-run]");
 	return 1;
 }
 
 var owner = args [0];
 var repo = args [1];
 var prNumber = args [2];
 var jsonPath = args [3];
+var dryRun = false;
+
+if (args.Length == 5) {
+	if (args [4] != "--dry-run") {
+		Console.Error.WriteLine ($"Usage: dotnet run submit_review.cs <owner> <repo> <pr_number> <review_json_path> [--dry-run]");
+		return 1;
+	}
 
+	dryRun = true;
+}
 if (!File.Exists (jsonPath)) {
 	Console.Error.WriteLine ($"❌ File not found: {jsonPath}");
 	return 1;
@@ -96,6 +107,18 @@
 
 var commentCount = root.TryGetProperty ("comments", out var cp) && cp.ValueKind == JsonValueKind.Array ? cp.GetArrayLength () : 0;
 Console.WriteLine ($"✅ Review validated: {commentCount} comment(s)");
+
+if (dryRun) {
+	Console.WriteLine ($"🔍 Dry run — review for {owner}/{repo}#{prNumber} would be submitted with the following JSON:");
+	Console.WriteLine ();
+	using var stream = new MemoryStream ();
+	using (var writer = new Utf8JsonWriter (stream, new JsonWriterOptions { Indented = true })) {
+		root.WriteTo (writer);
+	}
+	Console.WriteLine (System.Text.Encoding.UTF8.GetString (stream.ToArray ()));
+	return 0;
+}
+
 Console.WriteLine ($"📤 Submitting review to {owner}/{repo}#{prNumber}...");
 
 // Submit via gh api