[skills] Incorporate review feedback from dotnet/android#10918

[jonathanpeppers]

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)

[Copilot]

Pull request overview

Updates the android-tools-reviewer skill’s submission script and guidance docs to incorporate review feedback from dotnet/android#10918, tightening null-safety and refining the review rules/checklists.

Changes:

• Harden submit_review.cs by removing null-forgiving operators, handling Process.Start() returning null, and narrowing JSON parse catch blocks.
• Expand review-rules.md with additional C#-focused review checks and a new “Testing” section.
• Update SKILL.md workflow to gather context earlier and standardize comment severity formatting.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
.github/skills/android-tools-reviewer/scripts/submit_review.cs	Improves robustness of review JSON validation and gh CLI submission output/error handling.
.github/skills/android-tools-reviewer/references/review-rules.md	Adds new review guidance (thread safety, exception details, performance checks, testing).
.github/skills/android-tools-reviewer/SKILL.md	Refines reviewer workflow steps and introduces severity markers for comments.

[jonathanpeppers]

🤖 AI Review Summary

Verdict: ✅ LGTM

Found 0 errors, 0 warnings, 1 suggestion.

All three files are clean improvements:

• submit_review.cs: The null-forgiving operator (!) removals are correct per repo convention. Replacing with ?? "" is safe since GetString() on a confirmed JsonValueKind.String won’t return null in practice, and the coalescing handles the theoretical case. The Process.Start null check is good defensive coding with an actionable error message. Narrowing bare catch to catch (JsonException) is the right call — JsonDocument.Parse only throws JsonException for bad input, and the guard !string.IsNullOrEmpty(stdout) before the try block means we won’t hit ArgumentException. The success-output restructure is cleaner: the user sees confirmation immediately, and the URL is a bonus.

• review-rules.md: The new rules are well-scoped and actionable. Thread safety, exception swallowing, pre-allocation, O(n²) warnings, and the testing section are all high-value additions.

• SKILL.md: Splitting step 2 (gather context) from step 3 (read PR narrative) is a meaningful workflow improvement — it prevents anchoring bias. The severity levels (❌/⚠️/💡) and verdict line make reviews more scannable.

👍 Clean, focused PR with no scope creep.

• 💡 Code organization: Missing postmortem refs line in new Testing section (review-rules.md:203)

---

Review generated by android-tools-reviewer from review guidelines by @jonathanpeppers.

[jonathanpeppers]

🤖 AI Review Summary

Verdict: ✅ LGTM

No issues found. Clean improvements across all three files.

submit_review.cs fixes:

• 👍 Null-forgiving ! correctly replaced with ?? "" throughout — consistent with repo convention.
• 👍 Process.Start null check + using var is the right pattern — proper resource management and a clear error message when gh is missing.
• 👍 Bare catch blocks narrowed to catch (JsonException) — aligns with the repo’s own "challenge exception swallowing" rule. The only paths to these catches are JsonDocument.Parse calls, so the narrowing is safe.
• 👍 Success output restructured to print "✅ Review posted." unconditionally before attempting URL extraction — eliminates the old pattern where the catch block was doing the same work as the try block.
• 👍 ValueKind checks added to path and body validation before calling GetString(), preventing potential InvalidOperationException on non-string JSON values.

review-rules.md: 20 new rules across 7 categories, all actionable and factually accurate. Good breadth — thread safety, O(n²) detection, closure allocations, and the new Testing section are particularly high-value additions.

SKILL.md workflow restructuring: Separating context-gathering (step 2) from PR description reading (step 3) is a meaningful improvement — it encourages the reviewer to form an independent assessment before being anchored by the author’s narrative.

---

Review generated by android-tools-reviewer from review guidelines by @jonathanpeppers.

[rmarinho]

🤖 AI Review Summary

Verdict: ✅ LGTM

Found 0 issues.

Excellent improvements to the reviewer skill:

👍 Review Mindset section — The severity tiers (❌/⚠️/💡) and "form an independent assessment before reading the PR description" workflow are exactly right. This prevents anchoring bias.

👍 New rules are high-signal — Thread safety, exception swallowing challenge, O(n²) detection, ThrowIf helpers, early returns, and the Testing section (#13) all address real patterns we've hit in recent PRs.

👍 Script hardening — Replacing ! null-forgiving with ?? "", adding using on Process, null-checking Process.Start(), and typed catch (JsonException) instead of bare catch — the script now follows its own rules.

👍 "Don't pile on" and "Avoid false positives" constraints will keep reviews focused.

One minor note: the submit_review.cs script still requires .NET 8 (dotnet-script), which may not be available on machines running only .NET 10+. But that's a pre-existing issue, not introduced by this PR.

---

Review generated by android-tools-reviewer from review guidelines by @jonathanpeppers.