diff --git a/.github/skills/android-reviewer/SKILL.md b/.github/skills/android-reviewer/SKILL.md index cb2a93cf890..49fd6825455 100644 --- a/.github/skills/android-reviewer/SKILL.md +++ b/.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 diff --git a/.github/skills/android-reviewer/references/review-rules.md b/.github/skills/android-reviewer/references/review-rules.md index e9b1bfc727f..5052250176f 100644 --- a/.github/skills/android-reviewer/references/review-rules.md +++ b/.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` 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` 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` or `LazyInitializer.EnsureInitialized()` — they handle thread-safe initialization correctly. If a PR introduces or modifies a DCL pattern, flag it and suggest `Lazy` 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. | --- diff --git a/.github/skills/android-reviewer/scripts/submit_review.cs b/.github/skills/android-reviewer/scripts/submit_review.cs index 7603d2ed050..5235714a3df 100644 --- a/.github/skills/android-reviewer/scripts/submit_review.cs +++ b/.github/skills/android-reviewer/scripts/submit_review.cs @@ -1,16 +1,18 @@ -// Usage: dotnet run submit_review.cs +// Usage: dotnet run submit_review.cs [--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 "); + Console.Error.WriteLine ($"Usage: dotnet run submit_review.cs [--dry-run]"); return 1; } @@ -18,7 +20,16 @@ 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 [--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