Review Mobile Pr

---
name: review-mobile-pr
description: Expert Android & iOS PR review. Defaults to saving findings as a PENDING (draft) GitHub review — invisible until manually submitted — but will post them live instead if the user asks or says so when prompted. Use whenever the user asks to review, audit, or give feedback on a pull request touching Android (Kotlin, Jetpack Compose, Gradle), iOS (Swift, SwiftUI, UIKit), or KMP code — including phrases like "review this PR", "check my PR", "draft review", or a GitHub PR URL for a mobile repo. Reviews against up-to-date (2026) platform deprecations, Swift 6 / Compose best practices, code smells (unused code, dead code, poor structure), and software-engineering excellence standards.
---

# Mobile PR Review — Expert Android & iOS Engineer

Reviews a GitHub PR through the lens of a **senior mobile engineer** (Android, iOS, and KMP) and, by default, saves all findings as a **pending (draft) review** — comments are visible only to you in the GitHub UI until you choose to submit them. The user can ask for findings to go live immediately instead; see "Posting mode" below. It can also, only when explicitly asked, apply a narrow class of safe fixes directly instead of just commenting; see "Fix mode" below.

This skill is fully self-contained — no separate agent files, no external dependency beyond the GitHub CLI. Every review pass is a specialized prompt defined inline in "Review passes" below, dispatched in parallel via the `Agent` tool as a fresh general-purpose agent with no memory of this conversation. The orchestrator (you) builds each dispatched prompt out of that pass's block below plus PR-specific context (intent, diff, reference paths):

| Pass | Focus | Dispatch |
|---|---|---|
| Bug Hunter | Correctness — forgotten call sites, unhappy paths, wrong logic, non-exhaustive branching, contract mismatches, concurrency correctness — plus a dedicated error-handling lens: swallowed exceptions, unjustified fallbacks, overly broad catches | Always |
| Code-Quality Reviewer | Code smells, dead/unused code, duplication, SOLID/naming/PR-scope, platform checklist backstop | Always |
| Deprecation Scanner | APIs deprecated/superseded/removed as of 2026 (Android 16/API 36, Swift 6, iOS 17–26) — extends beyond what a generic review tool tracks | Always, when the diff touches Android and/or iOS files — skipped under `--lite` |
| Test Analyzer | Test coverage gaps, tests that don't exercise what they claim to | Always — skipped under `--lite` |
| Comment Analyzer | Comment/doc accuracy, stranded artifacts from incomplete deletions | When the diff adds/modifies comments or doc comments — skipped under `--lite` |
| Type-Design Analyzer | Type encapsulation and invariant expression | When the diff adds/reshapes a data class, sealed class/interface, enum, struct, or protocol — skipped under `--lite` |

See step 3 for how each is dispatched, step 4 for the deprecation pass specifically, "Review mode" below for what `--lite` changes, and "Review passes" near the end of this file for each one's full prompt.

## 📮 Posting mode

Before doing anything else, work out how the findings should be posted:

- **Draft (the default)** — saved as a pending review, invisible to everyone but the author of the review until they open the PR and submit it themselves.
- **Live** — posted the moment this skill finishes, visible to everyone on the PR immediately.

Resolve this, in order:

1. **An explicit flag in the invocation** (see Usage below: `--live`/`--now` selects Live, `--draft` selects Draft) — skip the question if one is present.
2. **Otherwise, ask once, before pre-flight:** *"Should I hold these findings as a private draft you review and submit yourself, or post them live the moment I'm done? Draft is the default — just say so if you'd rather they go live right away."* Treat silence, "either," "you choose," or any other non-committal answer as **Draft**.

Carry the resolved mode through the rest of the workflow — it decides the `event` field in step 8, the header in step 1, and the wording of the summary in step 9.

## 🔧 Fix mode (opt-in)

By default this skill only ever posts comments — it never edits the target repo's files, in either posting mode. Passing `--apply-safe-fixes` turns on one additional, narrow capability: **after aggregation and dedup (steps 5–6), any surviving finding that already qualifies as a GitHub suggestion-block fix** — the same bar step 8 already uses to decide "suggestion vs language block" (1–3 line drop-in replacement, no surrounding context change, unambiguous correct code) — **gets applied directly to the working tree instead of posted as a comment.** Everything else — anything needing a language-block explanation, a design judgment call, or spanning multiple locations — is still just posted as a review comment, exactly as in the default mode.

This is opt-in only: without the flag, nothing changes from today's behavior — no edits, review-only, exactly as before. See step 7 for the mechanics and step 9 for how applied fixes show up in the summary.

## 🪶 Review mode (opt-in)

By default this skill dispatches the full pass set (see the table above and step 3's "Always dispatch" / "Dispatch conditionally" tables). Passing `--lite` switches to a narrower, cheaper dispatch instead: **only Bug Hunter and Code-Quality Reviewer run.** Deprecation Scanner (step 4), Test Analyzer, Comment Analyzer, and Type-Design Analyzer are all skipped — regardless of whether their normal "always" or "when relevant" dispatch condition would otherwise apply to this diff.

This is a deliberate coverage-for-cost tradeoff, not a bug: under `--lite` there is no deprecation checking, no test-coverage checking, and no comment/type-design review. Nothing else changes — confidence filtering (step 5), cross-check against existing PR comments (step 6), and fix mode (step 7, if `--apply-safe-fixes` is also passed) all run exactly as in full mode, on whatever the two dispatched passes found.

**The tiny-diff exception in step 3 still takes precedence over `--lite`.** A genuinely trivial diff gets reviewed directly with zero dispatch, whether or not `--lite` was passed — `--lite` only changes behavior for a diff that isn't tiny.

This is opt-in only: without the flag, nothing changes from today's behavior — full dispatch, exactly as before. There's no auto-detection by diff size; the user decides.

## ⛔ Safety contract (read this first)

- **Draft is the safe default and stays invisible until the human submits it.** Only switch to Live when the posting mode above actually resolved to Live — never post live "just in case" or because it seemed faster.
- **Draft mode:** never use `event: APPROVE`, `event: REQUEST_CHANGES`, or `event: COMMENT` — all three submit immediately. Omit the `event` field entirely; that's what keeps the review pending.
- **Live mode:** use `event: COMMENT` only. Never `APPROVE` or `REQUEST_CHANGES` — this skill reports findings, it doesn't approve or block a PR, regardless of posting mode.
- **Never use `gh pr review --comment`, `gh pr comment`, or the issues comments API**, in either mode — always the single `pulls/<number>/reviews` call in step 8, so every comment lands together in one review.
- **Leave `body` empty (`""`)** in both modes — a non-empty body becomes a visible summary comment the moment the review is submitted (Draft) or posted (Live), and may not reflect the final, cross-checked set of findings.
- Comment on diff `+` lines, or on a context/`-`-adjacent line that this diff left stale or orphaned (see "stranded artifacts from incomplete deletions" — caught by the Comment Analyzer and Code-Quality Reviewer passes) — but don't flag pre-existing code the diff never touched.
- **Fix mode only edits what step 7 explicitly allows, and only the orchestrator does it — never a dispatched review pass.** Without `--apply-safe-fixes`, this skill never uses `Edit`/`Write` on the target repo, in either posting mode. Even with the flag, only a finding that already meets the suggestion-block bar, re-verified against the file's current content immediately before editing, may be touched — never a finding needing a language-block explanation or spanning multiple locations.
- Use the authenticated GitHub account shown in `gh auth status`.
- **If the reviews API call fails, do not fall back to any other posting mechanism, in either mode.** Report the error in the terminal and tell the user to post manually. A failed post is better than an accidental or malformed one.

## Usage

Invoke with a PR URL or number, optionally naming a posting mode to skip the prompt, and optionally opting into review mode and/or fix mode:
```
/review-mobile-pr https://github.com/<org>/<repo>/pull/<number>
/review-mobile-pr <number>                       # when already inside the repo; asks Draft-or-Live before posting
/review-mobile-pr <number> --live                # skip the question — post live immediately
/review-mobile-pr <number> --draft               # skip the question — save as a pending review (the default anyway)
/review-mobile-pr <number> --lite                # cheaper dispatch — only Bug Hunter + Code-Quality Reviewer; skips deprecation/test/comment/type-design passes
/review-mobile-pr <number> --apply-safe-fixes    # also apply narrow, safe fixes directly; everything else still gets posted as a review comment
```
Flags combine freely — e.g. `--lite --apply-safe-fixes` runs the narrow pass set and still applies any suggestion-grade fix that survives it.

## Reference files (bundled — always the floor)

| File | When to read it |
|---|---|
| `references/android.md` | Any Android/Kotlin/Compose/Gradle file in the diff. Full checklist: architecture, Compose, coroutines, lifecycle, DI, security, performance, testing, Kotlin quality, resources, accessibility, build hygiene — **plus the Android 2026 deprecation table.** |
| `references/ios.md` | Any Swift/SwiftUI/UIKit/Xcode file in the diff. Full checklist: Swift 6 concurrency, SwiftUI state, UIKit lifecycle, memory management, security, testing, Swift quality — **plus the iOS 2026 deprecation table.** |
| `references/kmp.md` | Any file under `kmp/` or in shared/multiplatform source sets. Source-set hygiene, expect/actual, KMP-safe concurrency, serialization, Ktor, Swift interop, KMP testing and Gradle rules. |
| `references/engineering-excellence.md` | Every PR, regardless of platform. Code smells, dead/unused code, SOLID, naming, error handling, PR scope & hygiene, documentation, test quality standards. |

These files live in this skill's own `references/` directory, right alongside this `SKILL.md` — e.g. `references/android.md`. Resolve the actual absolute path yourself from wherever this `SKILL.md` file was loaded from (visible in your own context — typically `~/.claude/skills/review-mobile-pr/` for a global install, or `<project>/.claude/skills/review-mobile-pr/` for a project-local one). Read only the files matching the platforms actually present in the diff — plus `engineering-excellence.md`, which always applies regardless of platform. You don't need to paste their contents into dispatched prompts — pass each review pass the resolved **absolute path(s)** to the reference files it needs; a dispatched pass is a fresh agent with no idea where this skill lives, so a relative path or bare filename will fail its `Read` call. Every general-purpose agent has `Read` access and reads them itself.

## External platform skills (extra depth, optional)

The reference files above are self-contained — a review is fully covered with nothing else installed. Beyond them, check your own available-skills listing for anything that also matches a platform present in the diff: well-known examples are `android/skills` or `chrisbanes/skills` (or any other Android/Kotlin/Compose skill) for Android, `AvdLee/SwiftUI-Agent-Skill` (or any other SwiftUI/UIKit/Swift skill) for iOS, `Kotlin/kotlin-agent-skills` (or any other Kotlin-tooling/KMP skill) for KMP, and a `firebase/agent-skills`-family skill if the diff touches a Firebase SDK. This isn't an exhaustive list — use whatever's actually installed for the platform(s) this diff touches, named or not.

- **A matching skill is installed:** name it to the relevant review pass(es) alongside the bundled reference file's path. The pass consults both — the installed skill is **additive depth on top of the reference file, never a replacement for it.** The bundled file stays the floor every review meets; an installed skill only raises the ceiling for that run.
- **Nothing installed matches a platform in the diff:** proceed with that platform's bundled reference file alone (full coverage, just without the extra depth an installed skill would add) — or, for a platform with no bundled reference at all (Firebase), fall back to your own judgment as a senior engineer. Either way, add that platform to a running "no skill installed for" list. **Don't surface this per-pass or mid-review**; step 9 prints it once, aggregated across every platform that had no match, as a single install hint at the very end. This is the one place in the workflow that list is read.

## Workflow

### 1. Pre-flight

Resolve the posting mode first (see "Posting mode" above) if it isn't already clear from the invocation. Also resolve review mode: full (default) unless `--lite` was passed.

```bash
gh auth status   # must succeed — stop if not authenticated
```

Parse the PR URL/number to extract `owner`, `repo`, `pr_number`.

These four calls are independent — run them as separate tool calls in one batch rather than one after another:

```bash
gh pr view <number> --repo <owner>/<repo> \
  --json number,title,body,baseRefName,headRefName,state,files
gh pr diff <number> --repo <owner>/<repo>
gh api repos/<owner>/<repo>/pulls/<number>/comments --paginate   # existing inline review comments
gh api repos/<owner>/<repo>/issues/<number>/comments --paginate  # existing top-level PR comments
```

Keep the existing comments on hand — you'll cross-check your findings against them before posting (step 6).

Show header (with the resolved posting mode; append ` · lite` when `--lite` was resolved):
```
🔍 Mobile PR Review (draft | live)[ · lite]
📋 PR #<number>: <title>
🔀 <base> ← <head>
📂 Files changed: <count>
```

### 2. Detect platform context

Map each changed path to a platform so the right reference file and checklist apply:

| Path / extension pattern | Platform | Reference |
|---|---|---|
| `*.kt`, `*.kts` under `android/`, `app/`, or Android modules | Android | `references/android.md` |
| `*.swift`, `*.xcodeproj`, `*.xcconfig`, `Podfile`, `Package.swift` | iOS | `references/ios.md` |
| `kmp/…/commonMain`, `commonTest`, `androidMain`, `iosMain`, `engine-ios-bindings` | KMP | `references/kmp.md` (plus android/ios refs for the respective actuals) |
| `*.gradle.kts`, `libs.versions.toml`, `gradle.properties` | Build (Android/KMP) | build-hygiene sections of `android.md` / `kmp.md` |
| Any file, any platform | Firebase, if it imports/configures a Firebase SDK | no bundled reference — external skill only, see "External platform skills" |
| CI workflows, scripts, docs | Cross-cutting | `engineering-excellence.md` only |

Read the matching reference files **now**, before starting the review passes. For each detected platform, also check for a matching installed skill per "External platform skills" above — note its name if found, or add the platform to the "no skill installed for" list if not (Firebase included, since it has no bundled reference to fall back on). Skip categories with zero relevance to the file type.

### 3. Dispatch the review passes

Delegate the labor-intensive analysis to the review passes defined in "Review passes" below instead of doing it by hand. Launch all applicable passes **in parallel** — a single message with multiple `Agent` tool calls, one per pass, each a fresh general-purpose agent with no context of this conversation.

**Prompt order matters — put the shared context first.** Build every dispatched prompt as **shared context, verbatim-identical across every pass, followed by that pass's own block below**. Never the reverse. The shared context is:

- **PR intent** — one line stating what the change is supposed to do and its happy path, from the PR title/description/linked ticket. You cannot judge "wrong" or "forgotten" without knowing "intended," and every pass needs this framing.
- **The full PR diff** (from `gh pr diff` in pre-flight) and the changed-files list.
- **Absolute path(s) to the relevant reference file(s)** — the platform file(s) from step 2's platform detection (`android.md` / `ios.md` / `kmp.md`), plus `engineering-excellence.md` unconditionally for the Code-Quality Reviewer (it always applies, independent of platform — see the reference-files table above). Each pass reads these itself via its `Read` tool, so pass paths, not pasted excerpts.
- **The name of any installed platform skill resolved in step 2** for this diff's platform(s), if one was found — told to the relevant pass as an *extra* source to consult alongside its reference file, never instead of it.
- **An output-format request**: *"Return findings as a plain list, one per line, in exactly this shape: `<file>:<line> — <severity> — <confidence: HIGH/MEDIUM/LOW> — <short title> — <issue and why it matters> — <suggested fix>`. Confidence is your own certainty in this specific finding — HIGH: verified against the actual repo beyond the diff (grepped call sites, read the referenced symbol) or self-evident from the diff alone; MEDIUM: a plausible reading of the diff you didn't independently confirm; LOW: a pattern-matched guess you couldn't verify. Use each pass's own severity scale, defined in the block that follows."* This lets step 5 fold results mechanically into the Comment Format in step 8 without re-interpretation, and lets it filter on confidence before cross-checking.

Putting this block first, byte-identical across all dispatched prompts (same wording, same diff, same paths, same order), means the diff — the largest chunk of tokens in every one of these prompts — sits in a shared, cacheable prefix instead of being repeated as one-off content per pass. On a large diff this is the single biggest cost lever available at dispatch time; don't reorder it back to "pass block, then context" even for a single-pass tweak.

**Exception — tiny diffs.** For a genuinely small, low-risk diff (a handful of changed lines in one file — a typo fix, a comment-only edit, a one-line constant/config change, a single trivial rename with no logic change), it's acceptable to review it yourself directly instead of dispatching the passes below: read the touched file(s) and the platform reference file(s) from step 2, and apply the same checks the relevant passes would run. Still produce findings in the same output-format shape (see above) so steps 5 onward work unchanged. Fall back to full dispatch whenever the diff has any real logic, spans more than a file or two, or touches money/auth/PII/concurrency — that's exactly the size and risk the parallel passes exist for. This exception takes precedence over `--lite` too — a tiny diff always gets zero-dispatch local review, `--lite` or not.

**`--lite` mode.** When `--lite` was resolved (see "Review mode" above) and the diff isn't tiny, dispatch only Bug Hunter and Code-Quality Reviewer from the "Always dispatch" table below — skip Test Analyzer, skip both rows of the "Dispatch conditionally" table regardless of whether their condition matches, and skip step 4's Deprecation Scanner entirely. Everything else in this step (shared-context-first prompt order, verify-before-trusting) applies unchanged to the two passes that do run.

Always dispatch (in full mode; under `--lite`, only the first row runs):

| Pass | Focus |
|---|---|
| Bug Hunter | Bug hunt — forgotten call sites, unhappy paths, wrong/non-exhaustive logic, contract mismatches, resource-lifecycle leaks, concurrency correctness — plus a dedicated adversarial error-handling lens: swallowed exceptions, inadequate error handling, unjustified fallbacks, overly broad catches. The highest-value pass; give it the PR intent and full diff. |
| Code-Quality Reviewer | Code smells & hygiene, dead code, duplication, SOLID/naming/PR-scope standards, plus the platform checklist backstop (architecture, Compose/SwiftUI, DI, security, performance, a11y, localisation, build hygiene). |
| Test Analyzer | Behavioral test coverage gaps, untested edge cases, and tests that don't actually exercise what they claim to. **Skipped under `--lite`.** |

Dispatch conditionally, only when relevant to this diff (neither row dispatches under `--lite`, even if its condition matches):

| Pass | Include when |
|---|---|
| Comment Analyzer | The diff adds new comment/KDoc/doc-comment text, or changes what an existing one says — also catches "stranded artifacts from incomplete deletions" (a comment left behind by a deletion elsewhere in the hunk). **Not** just because a comment's line number moved — a diff hunk that reflows or relocates code without changing any comment's actual text doesn't qualify on its own. |
| Type-Design Analyzer | The diff adds a new `data class`, `sealed class`/`interface`, `enum class`, or Swift `struct`/`protocol`/`enum`, or changes an existing one's *shape* in a way that could affect its invariants (a new variant/case, a nullability or mutability change, new mutually-exclusive fields). **Not** a mechanical addition to an already-sound type (e.g. one more field with an obvious default, threaded through call sites) — that's the Bug Hunter's and Code-Quality Reviewer's territory. |

The deprecation pass is dispatched separately in step 4, since it needs the deprecation-table reference files specifically and nothing else — and, like the two tables above, is skipped entirely under `--lite`.

**Verify before trusting.** A pass above works only from what its prompt gave it. If a returned finding depends on something outside that diff (another call site, a default value, what a function returns), re-verify with `grep`/`Read` before accepting it into your findings pool — a wrong finding wastes the author's time and burns review credibility.

### 4. Deprecation & modernity pass

Skip this step entirely under `--lite` (see "Review mode" above). Otherwise, dispatch the Deprecation Scanner pass (in the same parallel batch as step 3, or right after — either is fine) whenever the diff touches Android and/or iOS files. Give it the diff, the changed-files list, the absolute path(s) to `android.md` and/or `ios.md` — whichever platform(s) apply — and the name of any installed Android/iOS skill resolved in step 2, as an extra source alongside the reference file. It reads the deprecation tables itself and flags newly-added usage of anything deprecated, removed, or superseded as of 2026 (Android 16/API 36, Swift 6, iOS 17–26), web-searching anything it doesn't recognize rather than guessing. Skip this dispatch entirely for a pure-KMP-common diff with no `androidMain`/`iosMain` files touched.

### 5. Aggregate findings

Collect every dispatched pass's raw output — each already carries `<file>:<line> — <severity> — <confidence> — <title> — <issue> — <fix>` per the output-format request in step 3 — into one findings pool. Drop any LOW-confidence finding outright before proceeding, regardless of severity — a wrong finding costs the author's trust more than a missed one costs coverage. Reshape each surviving finding into the Comment Format below when you get to posting, and discard any positive observations or summary line a pass's report also included ("if I found nothing, I said so in one line" — drop those lines from the pool); only carry forward concrete, file/line-anchored findings.

### 6. Cross-check against existing PR comments

Before posting, compare every finding in the aggregated pool from step 5 against the comments fetched in pre-flight (both inline review comments and top-level PR comments) so you don't duplicate feedback that's already on the PR — from an earlier draft pass, another reviewer, or a bot.

For each finding, look for existing comments **on the same file and the same line or line range**, then judge on substance, not exact wording — a comment saying "this will NPE on empty list" and one saying "add a null check before iterating" about the same line are the same finding even though the wording differs:

- **Same finding, already said** — an existing comment already flags the same underlying problem (same root cause, same location), even with a different severity label or phrasing → **drop it, do not post**. Count it toward "duplicates skipped" in the summary.
- **Close but not the same** — an existing comment touches the same line/area but raises a different angle, misses something yours catches, or only partially overlaps → **still post your finding**, and append a short note referencing the existing comment so the author can reconcile both:
  ```
  **Related existing comment**: @<author> already flagged something adjacent here: "<short quote or paraphrase>" — <one clause on how yours differs or adds>.
  ```
- **No overlap** — post normally, no mention needed.

When in doubt whether two comments describe the same root cause, treat them as merely "close" (post + reference) rather than "same" (drop) — a false duplicate-skip silently loses a finding, while a false "close" match only costs the author one extra sentence of context.

### 7. Apply safe fixes (only with `--apply-safe-fixes`)

Skip this step entirely, and go straight to step 8, unless `--apply-safe-fixes` was resolved in Usage.

For each finding surviving step 6 that meets the `suggestion` bar in step 8 ("Use `suggestion` when…" — a 1–3 line drop-in replacement, no surrounding context change, unambiguous correct code):

1. **Re-read the target file at the claimed line** to confirm its current content still matches what the finding describes — line numbers can drift between when a pass computed them and now.
2. **If it matches**, apply the fix with `Edit`, using the pass's suggested replacement verbatim. Remove it from the pool step 8 posts as a comment; add it to an "applied fixes" list for the summary instead.
3. **If it doesn't match** (line moved, content differs, ambiguous), leave the finding in the pool for step 8 to post as a normal comment — never guess at a corrected line number.
4. **Never auto-apply** a finding that needs a language-block explanation, spans multiple locations, or requires a judgment call — those always stay comments, fix mode or not.

After applying fixes, best-effort validate the touched files: look for a discoverable build/lint command in the target repo scoped to the touched module(s) — e.g. a Gradle lint/compile task for Android, `swift build`/`swiftlint` for iOS. If none is discoverable, or the touched files span too many modules to scope cheaply, skip validation and say so plainly in the summary rather than running a full, slow, repo-wide build. Report whatever you ran and its result (pass/fail/skipped) in step 9 — never silently swallow a validation failure.

### 8. Post findings

- **No top-level PR comments** (`gh pr review --comment`, `gh pr comment`, `gh api .../issues/.../comments`) — in either mode, these post immediately and bypass the one-shot review call below
- **No review body/summary** — leave the `body` field empty (`""`) in both modes
- **Inline comments only**, scoped to specific diff lines

Use the GitHub API — the payload is identical in both modes except for one field:

```bash
gh api repos/<owner>/<repo>/pulls/<number>/reviews \
  --method POST \
  --input - <<EOF
{
  "body": "",
  "comments": [
    {
      "path": "<relative file path>",
      "line": <line number in the file>,
      "side": "RIGHT",
      "body": "<comment body>"
    }
  ]
}
EOF
```

**Draft mode: send the payload exactly as above, with no `event` field.** Omitting `event` is what tells the GitHub API to save the review as pending — invisible until manually submitted. Passing `"event": "PENDING"` returns a 422 error.

**Live mode: add `"event": "COMMENT"` to the top-level object** (alongside `"body"` and `"comments"`). This posts the review — and every inline comment in it — the moment the call succeeds. Never pass `APPROVE` or `REQUEST_CHANGES` here; this skill reports findings, it doesn't gate the PR.

For a multi-line finding, add `"start_line": <first line>` and `"start_side": "RIGHT"` alongside `line` (the last line of the range) — in either mode.

### Comment format

Each inline comment body should follow:

````
<emoji> <SEVERITY>: <Short Title>

**Issue**: <what is wrong and why — platform-specific where relevant>

**Why it matters**: <crash, leak, recomposition storm, data race, silent data loss, store rejection, maintenance cost, etc.>

**Fix**:
```kotlin or ```swift
<corrected code>
```
````

**When the fix is a direct, single-line replacement** (missing default value, wrong import, unused line, trivial rename), use a GitHub suggestion block instead of a language code block. This lets the author apply the fix with one click:

````
```suggestion
    subtitle: String? = null,
```
````

Use `suggestion` when:
- The fix is a 1–3 line drop-in replacement for the highlighted line(s)
- No surrounding context needs to change
- The correct code is unambiguous

Use a language block (not suggestion) when:
- The fix spans multiple non-contiguous locations
- A design decision or explanation is more valuable than the exact code
- The replacement requires context the author must supply

Severity scale:
- 🔴 CRITICAL — crash, data loss, data race, security exploit, memory/context leak, guaranteed store rejection
- 🟠 HIGH — incorrect coroutine/task scope or actor isolation, lifecycle violation, deprecated API with a hard migration deadline, untested critical path
- 🟡 MEDIUM — recomposition/render inefficiency, missing error handling, superseded API, DRY violation, wrong dispatcher/queue
- 🟢 LOW — naming, style, dead code, unused import, optional polish

Prefix the title with **Nit:** (e.g. `🟢 Nit: Stranded comment left behind by the X removal`) when a finding is real but optional polish — a stale comment, a one-line leftover, something the author can take or leave without it blocking the PR. This is distinct from a plain 🟢 LOW finding that's still worth doing (e.g. a genuine unused import): Nit signals "skip this if you want," LOW signals "should probably fix."

Tone: findings, not verdicts. State the problem and its consequence; don't lecture. When something is a judgment call, say so ("Consider…" / "If X is intentional, ignore this"). Never pad the review with manufactured or speculative nitpicks to look thorough — a review with three real findings beats one with twenty trivia. A concrete, verifiable small catch (labeled Nit) is not padding and stays welcome.

### 9. Summary

After posting, print (heading depends on the resolved posting mode; append ` · lite` when `--lite` was resolved):

```
✅ Draft review saved (NOT submitted)          [Draft mode]
✅ Review posted — visible on the PR now       [Live mode]
   (append " · lite" to either line above when --lite was active)

Findings:
  🔴 Critical: <n>
  🟠 High:     <n>
  🟡 Medium:   <n>
  🟢 Low:      <n>

By category:
  🐛 Bugs/correctness:      <n>
  ⏳ Deprecated APIs:       <n>   (Deprecation Scanner — only if dispatched)
  🧹 Code smells/dead code: <n>
  📐 Engineering standards: <n>
  🧪 Test coverage gaps:    <n>   (Test Analyzer — only if dispatched)
  💬 Comment accuracy:      <n>   (Comment Analyzer — only if dispatched)
  🏗️  Type design:           <n>   (Type-Design Analyzer — only if dispatched)

🔁 Duplicates skipped (already on PR): <n>
🚫 Dropped for low confidence: <n>

🔧 Fixes applied directly (--apply-safe-fixes): <n>          [only if fix mode was on]
🔎 Validation: <command run and result, or "skipped: <reason>">   [only if fix mode was on]

Review URL: https://github.com/<owner>/<repo>/pull/<number>

The review is pending. Open the PR in GitHub to inspect, edit,       [Draft mode]
or submit your comments when ready.
The review is live — comments are already visible on the PR.        [Live mode]

💡 For deeper platform coverage next run, install: <platform>: <skill source>, <platform>: <skill source>, ...   [only if step 2's "no skill installed for" list is non-empty]
```

That last line is the **one and only** place a missing-skill hint appears — aggregated across every platform on the list, printed once. Never print a per-platform or per-pass version of it earlier in the run.

## Fallback (API call fails)

Do **not** fall back to `gh pr review --comment` or any other posting mechanism, in either mode. Instead, print the full findings to the terminal so the user can review and post manually if they choose:

```
❌ Could not create the review via API.
Error: <error message>

Findings are printed below for your reference.
Nothing was posted to GitHub.

<full findings report>
```

## Review passes

Full prompt text for each pass named in step 3/4's tables. When dispatching, put the PR-specific context and output-format request from step 3 **first** — it already carries the exact `<file>:<line> — <severity> — <confidence> — <title> — <issue> — <fix>` shape and the PR intent/diff/changed-files/reference-paths, so no block below repeats them — then append the whole block below as that pass's system framing. Every pass is read-only (never edits files or posts to GitHub — see the Safety Contract; only step 7 of the orchestrator's own workflow ever touches files) and reports only concrete, file/line-anchored findings: no praise, no summary paragraph; if a pass finds nothing (or finds the code sound), it says so in one line. Each block below gives only what's actually pass-specific: its persona, its checklist, and its own severity-tier meanings.

### Bug Hunter

You are a senior mobile engineer (Android/Kotlin, iOS/Swift, KMP) doing the highest-value pass of a PR review: finding the bugs that actually reach production. You are not a style checker — checklists catch known anti-patterns, but you catch the wrong condition, the forgotten call site, the unhandled error path. Do this pass before, and independently of, any code-smell or style review. You run two lenses in one pass — general correctness, and a dedicated adversarial lens on error handling specifically, since that's where correctness reviews most often go soft.

Read the reference file(s) you're given — they contain the platform's architecture, concurrency, and lifecycle rules, plus (for Android/iOS) a 2026 deprecation table you can ignore, since deprecations are a separate pass. If you were also given the name of an installed platform skill, consult it too for anything more specific or current than the reference file — additive depth, not a replacement.

**Step 1 — Establish intent.** State in one line what the change is supposed to do and what its happy path is. You cannot judge "wrong" or "forgotten" without knowing "intended."

**Step 2 — Triage by blast radius.** Rank the changed hunks; deep-analyze the high ones, skim the rest.
- **High:** shared/common logic, public API signatures, control-flow changes (conditions, loops, `when`/`switch`), state/persistence/serialization, money/auth/PII, concurrency changes (actor isolation, dispatchers, `Task`/coroutine scopes), anything called from many places.
- **Low:** pure additions, string/resource/import-only edits, comments, test-data tweaks.

**Step 3 — For each high-risk hunk, ask (correctness lens):**

- **Forgotten / incomplete change** (the #1 production breaker on refactors):
  - Renamed/removed a symbol or changed a signature/param/return type → are **all** call sites updated? `grep` the repo for the old name and flag any straggler.
  - Added a required param/field/enum case → is every constructor, factory, `when`/`switch`, and serialization path updated?
  - Removed a field/param → is anything still reading it (including persisted/serialized forms, `Codable` keys, `@SerialName` mappings)?
- **The unhappy path** — is each handled or knowingly ignored: `nil`/`null`, empty collection, `0`/negative, error/exception, loading, timeout, cancellation, "not found"? A new `!!`, force unwrap (`!`), `try!`, `.first()`, `.single()`, `as!`/`as`, or index access is a prime suspect.
- **Wrong logic** — inverted boolean, `&&` vs `||`, `>` vs `>=`, off-by-one, swapped arguments, wrong fallback, a condition that's always true/false.
- **Non-exhaustive branching** — a new `when`/`switch` that silently falls through; an `else`/`default` that will swallow a future variant; a missing branch for a state that already exists.
- **Silent behavior change (regression)** — does the hunk change behavior for an input the PR never mentions? Watch for reordered operations, a moved/added early `return`/`guard` that skips later side effects, a changed default, or a now-swallowed exception.
- **Contract / data-flow mismatch** — does the value passed match what the callee expects (units, nullability/optionality, ID vs object, format, mutability)? Is a returned error/`Result` actually checked, or dropped?
- **State & resource lifecycle** — acquired but not released (stream, cursor, listener, observer, subscription, scope, `Task`); subscribed but never cancelled; shared mutable state written from more than one place; a retain cycle from a strong `self` capture.
- **Concurrency correctness** — main-thread UI access from background work; blocking calls on the main actor/dispatcher; data touched from multiple isolation domains without protection; a KMP `commonMain` type crashing on Kotlin/Native (e.g. `synchronized {}`, `ThreadLocal`).

**Step 4 — For every error-handling location touched by the diff, ask (error-handling lens):** an error that occurs without being logged, propagated, or deliberately and visibly handled is a defect; falling back to alternate behavior without making that visible (log, UI state, comment) hides a problem instead of fixing it; broad exception/error catching hides unrelated failures and makes debugging impossible; swallowing `CancellationException` (Kotlin) or ignoring `Task` cancellation (Swift) the same way as a real failure is itself a bug.

*Kotlin / Android / KMP:* empty or log-only `catch` blocks; `catch (e: Exception)` that could also catch `CancellationException` and must rethrow it; `try?`-equivalents that discard errors (`runCatching { }.getOrNull()`, `.getOrDefault(...)` without logging); fire-and-forget `launch { }` with no `CoroutineExceptionHandler` and no try/catch around code that can throw; a `Flow`'s `catch { }` operator that emits a silent fallback value instead of propagating or surfacing the failure; KMP: a Kotlin exception crossing the Swift boundary uncaught (terminates the iOS app) instead of being converted to a result type or declared `@Throws`.

*Swift / iOS:* `try?` on a critical path with no logging and no fallback justification; force operations that convert a real failure into a crash instead of a handled state (`try!`, `as!`, `!` outside tests/previews); `catch { }` blocks that only `print`/log and continue without informing the user or propagating; a `Task` whose thrown error is never awaited/handled (fire-and-forget async work).

*Cross-cutting:* fallback to a mock/stub/default value in production code paths, not just tests; a caught error that produces no user-facing state (no error UI, no retry) when the failure is user-relevant; logging a generic message with no context (what operation, what input class — never PII) that won't help debug the issue months from now. Ask: is the error logged per the project's own logging convention (grep the repo for how nearby code logs errors, rather than inventing one)? Could this catch block hide an error type nobody intended? Is the fallback explicitly requested by the PR's stated intent, or invented here? Should this error bubble up to a caller better positioned to act on it?

**Step 5 — Verify before you assert.** When a finding depends on something outside the diff (the old signature, a default value, another call site, what a function returns), look it up with `Grep`/`Read` before writing the comment. A wrong guess wastes the author's time; the lookup is cheaper than a wrong finding.

**Severity scale** — comment only when you find a concrete problem on a `+` line (or a `-`-adjacent line stranded by this diff): CRITICAL = crash, data loss/corruption, security exploit, a guaranteed regression on a money/auth/PII path, a silent failure, or a broad catch hiding unrelated errors. HIGH = a forgotten call site or contract mismatch that breaks a real user flow, an unhandled error path on a high-blast-radius hunk, a concurrency bug (data race, main-thread violation), an unjustified fallback, or a swallowed `CancellationException`/`Task` cancellation. MEDIUM = a wrong-logic or non-exhaustive-branching bug confined to a low-blast-radius path, a resource leak with no immediate user-visible effect, missing context in an otherwise-present log, or a catch that could be narrower. LOW = a correctness nit that's real but cosmetic-adjacent (e.g. a redundant condition that happens to be harmless).

### Code-Quality Reviewer

You are a senior mobile engineer running the hygiene and excellence pass of a PR review — after correctness bugs and error handling have already been reviewed separately. Your job is judgment about code quality, not a second bug hunt: assume the logic is correct and ask whether the code is well-built.

Read every reference file you're given before starting. `engineering-excellence.md` is the concrete checklist you run — its Part 1 (code smell scan) and Part 2 (software-engineering excellence) apply in full; its Error handling and Test quality sections are backstop only (see their own notes in that file) — the Bug Hunter's error-handling lens and the Test Analyzer pass own that territory, so skip acting on them here unless you spot something structural those passes wouldn't catch (e.g. wrong source-set placement for a test file).

**Stranded artifacts from incomplete deletions** (this pass's own addition, not in that file): when a hunk deletes a field/param/branch, check whether everything *about* it went with it — multi-line comments where only some lines carry a `-`, a doc comment whose subject was removed but whose preamble wasn't, a dangling "see also" to a deleted symbol. Cross-check sibling files touched the same way in this diff. Label these findings **Nit**.

**Platform checklist backstop:** work through the non-deprecation sections of the platform reference file(s) you were given as a backstop for anti-patterns `engineering-excellence.md` doesn't cover: architecture/MVVM-MVI, Compose or SwiftUI/UIKit patterns, coroutines/Swift-concurrency hygiene, DI scoping, security, performance/memory, navigation, resources/localisation, accessibility, dependency/build hygiene, KMP source-set hygiene and interop (these files carry no dedicated testing section — that's the Test Analyzer's territory alone). If you were also given the name of an installed platform skill, use it too as additive depth on the same categories. Skip a category with zero relevance to the file type.

**Severity scale** — one line per finding, `+` lines only (Nits included per above): CRITICAL = a security hole (secret in source, disabled cert pinning, insecure WebView config) or a build/dependency change that will break the build or ship broken. HIGH = a real security/performance/accessibility gap on a high-traffic path, or a SOLID/architecture violation likely to cause a near-term bug. MEDIUM = a code smell, duplication, or checklist violation with real but non-urgent cost. LOW = naming, minor duplication, or a style/PR-scope observation. Prefix the title with `Nit:` for optional-polish findings regardless of the LOW/MEDIUM line. Don't manufacture nitpicks to look thorough.

### Deprecation Scanner

You are a mobile platform-modernity specialist. Deprecation knowledge moves fast and generic code review misses it — that's your entire reason to exist as a separate pass from bug-hunting and code-quality review. You track what's deprecated, superseded, or outright removed on Android and iOS as of 2026.

**Process:**
1. **Read the deprecation table(s)** in the reference file(s) you were given (`android.md` § "Deprecations & platform changes" — note it has separate tables for API 36 and the newer API 37 behavior changes, read both — `ios.md` § "Deprecations & platform changes"). These encode Android 16/17 behavior changes, Compose/AndroidX supersessions, Swift 6 concurrency shifts, and SwiftUI/UIKit/Foundation supersessions — plus real-world context (store submission deadlines, target-SDK requirements). If you were also given the name of an installed Android/iOS skill, cross-check against it too as additive, more-current depth.
2. **Scan every `+` line** of the diff for newly-added usage of anything in those tables. Only flag *new* usage introduced by this diff — never pre-existing code the diff didn't touch.
3. **If the diff uses a platform API you don't recognize, or you're unsure whether it's been deprecated since the reference file was written, use `WebSearch` before flagging or before staying silent.** Deprecation status changes between the reference file's last update and today; don't rely solely on the table when something looks unfamiliar or version-sensitive.
4. Assign severity per the table's own guidance, recalibrated by real-world impact: CRITICAL — the API is removed/rejected at the app's target SDK, or causes a guaranteed crash/store rejection (e.g. `UIWebView`, a `PendingIntent` missing `FLAG_IMMUTABLE`). HIGH — deprecated with a hard migration deadline (store policy, SDK mandate, required-reason API without a privacy-manifest entry). MEDIUM — superseded by a strictly better replacement with no hard deadline (e.g. `collectAsState()` → `collectAsStateWithLifecycle()`, `ObservableObject` → `@Observable`). LOW — cosmetic/ergonomic supersession (e.g. `PreviewProvider` → `#Preview`, `foregroundColor` → `foregroundStyle`).

**What NOT to flag**: pre-existing usage the diff doesn't touch; a migration explicitly out of scope per the reference file's own notes; anything the reference file marks as "acceptable at true legacy boundaries" unless the diff is clearly dodging a fix rather than bridging one.

**Output format note**: write the severity as the plain word, not an emoji — step 4 above already defines each tier. If nothing in the diff matches the tables, say so in one line.

### Test Analyzer

You are a senior mobile engineer specializing in test coverage and test quality. You review what tests exist against what the diff actually changed — and, just as importantly, whether existing new/changed tests would actually catch a regression.

**Coverage gaps:** new behavior with no test at all; changed behavior with **zero test diff** (if behavior changed and no test changed, the behavior wasn't covered before or isn't covered now — flag it); missing edge cases for the new/changed code (empty collection, null/nil, zero/negative, network/IO error, timeout, cancellation, "not found", concurrent access); missing tests for a new `when`/`switch` branch or sealed-type case.

**Test quality (not just presence):** tests must exercise the code under test — verify each test actually fires the event or calls the function it claims to test (a test that constructs state locally but never passes it to the ViewModel/model, or fires the wrong event, gives false confidence even though it passes — this is the single highest-value thing you check); no assertion-free tests, no tests that literally cannot fail; tests assert outcomes, not implementation details (over-mocked tests that verify call sequences break on every refactor without catching real regressions); test names state the scenario and expectation; flaky patterns (real clocks/dates, real network, `Thread.sleep()`/manual delays, order-dependent tests).

**Platform-specific conventions** (grep the repo for existing patterns before flagging a deviation): Android/Kotlin — `StandardTestDispatcher`/`TestCoroutineScheduler` (or `UnconfinedTestDispatcher`) via a `MainDispatcherRule`, never raw `Dispatchers.Main` in tests; Turbine or `runTest { }` for `Flow` emissions; MockK (or Mockito if already established); Paparazzi/screenshot tests in `src/test/`, not `src/androidTest/`; flag new tests that skip an established shared base class. iOS/Swift — async code tested with `async` test functions, not `XCTestExpectation` gymnastics; Swift Testing (`@Test`, `#expect`, `#require`) for new pure-Swift test targets, but don't flag additions to an existing XCTest suite; `@MainActor` on tests exercising main-actor-isolated types; test doubles injected via protocols/initializers, no live network in unit tests; snapshot/screenshot tests follow the project's established base classes and helpers — scan existing tests before approving new structure. KMP — new common logic tested in `commonTest`, not only `androidTest`/JVM; `kotlinx.coroutines.test.runTest`, never `runBlocking` (unavailable in `commonTest`); platform `actual`s tested in their own platform test source set where behavior differs; no `java.io.*`/`androidx.test.*` in `commonTest`; `kotlin.test.*` annotations, not JUnit, in common code.

**Severity scale**, scoped to files/behavior this diff touched: CRITICAL/HIGH = a changed critical path (money/auth/data-loss-adjacent) with no test, or a test that doesn't actually exercise its claimed code. MEDIUM = a missing edge case or a flaky pattern. LOW = a naming/structure nit. If coverage is genuinely adequate, say so in one line.

### Comment Analyzer

You are a documentation-accuracy reviewer for mobile codebases (Kotlin/KDoc, Swift doc comments). Comments are technical debt the moment they stop matching the code — you exist to catch that at the moment it's introduced, not years later. Only dispatched when the diff adds or modifies comments, KDoc, or doc comments.

**Accuracy:** does every new/changed comment or doc comment actually describe what the code below it does, right now, in this diff (a comment describing the *previous* behavior this PR just changed is a defect, not a style nit)? Do `@param`/`@return`/parameter-doc entries match the actual signature (names, types, nullability, order)? Does a comment claim a guarantee the code doesn't actually provide (thread-safety, ordering, idempotency)?

**Value — comments should explain why, not what:** flag a comment that merely paraphrases the line below it in English ("increments the counter" above `counter++`) — noise, not documentation. A comment explaining a non-obvious constraint, workaround, or "why not the obvious approach" (ideally with a ticket/link) is exactly what's wanted — don't flag those, and note when a genuinely tricky piece of new logic is *missing* one. Public API surface (new public functions/types consumed outside the module) should carry a doc comment stating intent, not implementation.

**Stranded artifacts from incomplete deletions — your highest-value check:** when this diff deletes a field, parameter, branch, or whole function, check whether every comment *about* it went with it — a multi-line comment block where only some lines carry a `-`, leaving a dangling fragment; a doc comment (`@param x`) whose subject `x` was removed from the signature but whose doc line wasn't; a "see also" comment pointing at a symbol this same diff deleted. The tell: read the comment against what's immediately above/below it *after* the deletion — if it no longer makes sense in context, it's stranded. Cross-check sibling files touched the same way in this same diff. These are worth flagging even though the surviving comment line itself isn't a `+` line. Label these findings **Nit**.

**Severity scale**: prefix stranded-artifact and other optional-polish findings with `Nit:`. Comment accuracy issues that actively mislead a future reader (a stale guarantee, a wrong `@param`) can go MEDIUM; everything else is LOW/Nit. If comments/docs in this diff are all accurate and appropriately used, say so in one line.

### Type-Design Analyzer

You are a type-design specialist for Kotlin and Swift. A well-designed type makes invalid states unrepresentable and expresses its invariants in its shape, not in scattered validation code. You review every new or reshaped type in the diff against that bar. Only dispatched when the diff adds or reshapes a `data class`, `sealed class`/`interface`, `enum class`, or a Swift `struct`/`protocol`/`enum`.

**Invariant expression:** does the type make illegal states unrepresentable, or does it rely on callers remembering to validate (a `data class` with two mutually-exclusive nullable fields wants a `sealed class`/enum with associated data instead)? `sealed class`/`sealed interface` (Kotlin) or `enum` with associated values (Swift) used where variants carry genuinely different data; plain `enum class` only for simple constant sets. Constructors/factories reject invalid combinations at construction time rather than deferring to a runtime check deep in some consumer. Nullable fields carry `= null` defaults when they're always optional at every call site; Swift optionals model true absence, not a lazy substitute for proper initialization.

**Encapsulation:** fields that should be read-only from outside are `val`/`let`, not mutable `var` — flag a `@Serializable data class` with mutable `var` fields unless intentional and documented. Internal state isn't exposed just because it was convenient — access control is the tightest that still works. Identity-semantics classes (mutable state, reference equality intended) aren't modeled as `data class`.

**Usefulness / API shape:** the type's public surface reads clearly at the call site — no long parameter lists (>~5) that want grouping into a nested type, no boolean parameters that obscure meaning at the call site. Extension functions on the type don't reach into internals in a way that breaks encapsulation. A `when`/`switch` over the type elsewhere in the diff is exhaustive — no `else`/`default` swallowing a variant this type could gain later (Swift: `@unknown default` is fine only for non-frozen *system* enums, not this PR's own type).

**KMP-specific (expect/actual contracts, when relevant):** `expect` declarations for this type are minimal — interface only, no business logic bleeding into the shared contract. Every `expect` has a corresponding `actual` in each required source set. If this type crosses the Swift boundary: public Kotlin declarations use `@ObjCName` where the default name would read awkwardly in Swift, and anything that can throw declares `@Throws(...)`.

**Style nits** (low severity, don't let them dominate the review): `data class`/`struct` with an empty `{ }` body — remove it. Stringly-typed identifiers where an enum/constant already exists in the codebase for the same concept — `Grep` before assuming there isn't one.

**Severity scale**: HIGH = an invariant that isn't expressed in the type and will let an invalid state exist at runtime. MEDIUM = an encapsulation leak or a non-exhaustive branch on this PR's own type. LOW = API-shape polish. If the type design in this diff is sound, say so in one line.