feat: add automated PR review skill

.claude/skills/review-pr/SKILL.md

@@ -0,0 +1,43 @@
+---
+name: review-pr
+description: Review the current branch's changes against SDK architectural patterns and report violations grouped by severity.
+---
+
+# PR Review for Privy Unity SDK
+
+Review the current branch's changes against our SDK architectural patterns and code correctness rules. For each file changed, check the rules in `agent_docs/pr_review_rules.md` and report violations. Be strict — our SDK's consistency depends on this.
+
+## Instructions
+
+1. Determine the parent branch:
+   - If `$ARGUMENTS` is provided, use that as the base branch.
+   - Otherwise, detect the parent branch by running: `git log --decorate --simplify-by-decoration --oneline --first-parent HEAD | grep -v "HEAD" | head -1` to find the nearest branch point. Alternatively, check `git config branch.$(git branch --show-current).merge` for the upstream tracking branch, or fall back to the merge-base with `main`.
+2. Run `git diff <parent-branch>...HEAD` to get only this branch's changes (excluding the parent's commits).
+3. Read `agent_docs/pr_review_rules.md` to load the full rule set.
+4. For each changed/added `.cs` file, evaluate against the rules.
+5. Report findings grouped by severity: **Blocking** (must fix), **Warning** (should fix), **Nit** (style preference).
+6. If no violations found, confirm the PR looks good.
+
+## Output Format
+
+For each violation found:
+
+```
+### [Severity] File: path/to/file.cs
+
+**Rule**: [Rule name]
+**Line(s)**: [line numbers]
+**Issue**: [What's wrong]
+**Fix**: [What to do instead]
+```
+
+At the end, provide a summary: number of blocking/warning/nit issues, and an overall verdict (Approve, Request Changes, or Approve with Nits).
+
+## Comment Style
+
+- Be terse. One sentence per finding is the norm. Two is long. Never write a paragraph when a sentence will do.
+- Ask questions over demands. "Can we make this `internal`?" lands better than "Make this `internal`."
+- Prefix minor style comments with `nit:` to signal they won't block merge.
+- Explain the "why" only when non-obvious. A missing `I` prefix is self-explanatory; a subtle async deadlock is not.
+- No filler. Don't open with "Great work!" or pad with "Love the approach, however...". Go straight to the point.
+- Only comment on lines with real issues — do not comment for the sake of it.

agent_docs/pr_review_rules.md

@@ -0,0 +1,96 @@
+# PR Review Rules
+
+These are the architectural patterns and rules for the Privy Unity SDK. Any new code must adhere to these.
+
+## Architecture (Blocking)
+
+- **Interface + Implementation separation**: Every public service has an interface prefixed with `I` (e.g., `ILoginWithEmail`, `IPrivy`) and a separate `internal` implementation class (e.g., `LoginWithEmail`, `PrivyImpl`). Interfaces are `public`, implementations are `internal`.
+- **Folder structure**: Features live in `SDK/Runtime/<Feature>/`. Interfaces and implementations live in the same feature folder. Models get a `Models/` subfolder within the feature.
+- **Namespace per folder**: Each folder has its own namespace matching the path — `Privy.Auth.Email`, `Privy.Core`, `Privy.Wallets`, `Privy.Utils`, `Privy.Internal.Networking`, etc.
+- **Internal namespaces**: Implementation-only types use `Privy.Internal.*` namespaces (e.g., `Privy.Internal.Networking`, `Privy.Internal.Storage`).
+
+## Naming Conventions (Blocking)
+
+- **Interfaces**: `I` prefix + PascalCase — `IPrivy`, `ILoginWithEmail`, `IHttpRequestHandler`, `IAuthDelegator`.
+- **Implementation classes**: No prefix, PascalCase matching the concept — `PrivyImpl`, `LoginWithEmail`, `HttpRequestHandler`, `AuthDelegator`.
+- **Private fields**: `_camelCase` — `_authDelegator`, `_httpRequestHandler`.
+- **Public properties/methods**: PascalCase — `Email`, `GetUser()`, `AuthStateChanged`.
+- **Constants**: PascalCase — `MaxRetries`, `ApiVersion`.
+- **File names**: Match the type name — `ILoginWithEmail.cs`, `LoginWithEmail.cs`, `PrivyException.cs`.
+
+## Access Control (Blocking)
+
+- **Public interfaces only**: Only interfaces (`IPrivy`, `ILoginWithEmail`, etc.) and types needed by SDK consumers are `public`.
+- **Internal implementations**: All implementation classes are `internal`. They must not be directly accessible to SDK consumers.
+- **Public models/enums for consumers**: Data types returned to consumers (`AuthState`, `PrivyException`, `AuthenticationError` enum) are `public`.
+- **Internal networking/storage**: `IHttpRequestHandler`, `PlayerPrefsDataManager`, repositories are `internal`.
+
+## Dependency Injection (Blocking)
+
+- **Constructor injection**: All dependencies are passed via constructor. No service locators or static singletons (except the `PrivyManager` entry point).
+- **Wiring in PrivyImpl**: All service instantiation happens in `PrivyImpl`'s constructor. New services must be instantiated there with their dependencies.
+- **Null-check constructor params**: Constructor parameters for required dependencies should include `?? throw new ArgumentNullException(nameof(param))`.
+- **PrivyManager as entry point**: `PrivyManager.Initialize(config)` is the only way to create an SDK instance. No other public constructors.
+
+## Async Patterns (Blocking)
+
+- **Task-based async**: All async operations return `Task<T>`. Use `async/await` throughout.
+- **TaskCompletionSource for initialization**: SDK initialization uses `TaskCompletionSource` to allow `GetAuthState()`/`GetUser()` to await readiness.
+- **SafeFireAndForget for background work**: Fire-and-forget tasks use the `.SafeFireAndForget()` extension with error logging.
+- **No blocking calls**: Never use `.Result` or `.Wait()` on tasks. Always `await`.
+
+## Error Handling (Blocking)
+
+- **Typed exceptions**: Use `PrivyAuthenticationException` (with `AuthenticationError` enum) for auth failures and `PrivyWalletException` (with `EmbeddedWalletError` enum) for wallet failures. Base class is `PrivyException`.
+- **Error enums**: Add new error cases to the appropriate enum (`AuthenticationError` or `EmbeddedWalletError`) rather than using generic error messages.
+- **No swallowed exceptions**: Every catch block must either rethrow (wrapped), log, or handle meaningfully. Never empty catch blocks.
+- **Guard clauses**: Validate inputs at the top of methods with descriptive exceptions.
+
+## Layered Architecture (Warning)
+
+- **IPrivy → LoginWith* → AuthDelegator → AuthRepository → HttpRequestHandler**: Public API delegates to feature modules, which use the auth delegator, which calls repositories, which use the HTTP handler. Don't skip layers.
+- **AuthDelegator for state management**: Authentication state changes flow through `AuthDelegator`. Modules should not directly mutate auth state.
+- **Repositories for network calls**: Repositories (`AuthRepository`, `AppConfigRepository`) handle HTTP requests and deserialization. Business logic belongs in delegators/managers.
+
+## Network Layer (Warning)
+
+- **Use IHttpRequestHandler**: All API calls go through `IHttpRequestHandler.SendRequestAsync()`. Never use `UnityWebRequest` directly in feature code.
+- **JSON serialization**: Use `JsonUtility` or the project's JSON approach consistently. Request/response models should be serializable.
+- **Custom headers via parameter**: Pass additional headers (e.g., MFA tokens) through the `customHeaders` dictionary parameter, not by modifying the handler.
+
+## Events (Warning)
+
+- **C# events for state changes**: Use `event Action<T>` for state change notifications (e.g., `AuthStateChanged`). Forward events from internal components to the public interface.
+- **No Unity-specific patterns in SDK core**: Don't use `UnityEvent`, `MonoBehaviour`, or coroutines in the SDK Runtime. Use standard C# async/await and events.
+
+## Correctness & Logic (Warning)
+
+- **No unhandled error paths**: Every `async Task` call site must handle exceptions. Don't let raw exceptions propagate past the delegator layer unhandled.
+- **No `.Result` or `.Wait()`**: Never block on async tasks. This causes deadlocks in Unity's single-threaded synchronization context. Always `await`.
+- **No sensitive data in logs**: Never log tokens, private keys, seed phrases, wallet addresses, or user credentials at any log level.
+- **Dead code**: Flag unused methods, parameters, and imports. Remove them rather than leaving commented-out code.
+- **Null reference safety**: Check for null before dereferencing objects from external sources (API responses, deserialized JSON, user input). Use null-conditional operators (`?.`) or explicit guards.
+- **Race conditions in async code**: Ensure that state checks followed by state mutations don't have `await` points between them that could allow interleaving.
+- **Guard against invalid inputs**: Validate inputs at method boundaries before performing work. Throw `ArgumentException`/`ArgumentNullException` early rather than proceeding with invalid state.
+- **No redundant work**: Don't re-fetch data that's already available in scope. Don't re-compute values inside loops when they can be hoisted.
+- **Dispose resources**: `IDisposable` resources (HTTP clients, streams, WebViews) must be disposed when no longer needed.
+- **TaskCompletionSource safety**: Always set a result or exception on `TaskCompletionSource` — unresolved TCS instances will hang awaiting callers forever.
+
+## Documentation (Nit)
+
+- **XML docs on public types**: All `public` interfaces, methods, properties, and classes need `/// <summary>` documentation.
+- **Document parameters**: Use `/// <param name="">` for method parameters.
+- **Document exceptions**: Use `/// <exception cref="">` for thrown exceptions.
+- **No over-documentation**: Internal implementation classes don't need XML docs unless behavior is non-obvious.
+
+## Style (Nit)
+
+- **Allman brace style**: Opening braces on a new line (enforced by `.editorconfig`).
+- **4-space indentation**: No tabs.
+- **Format with `dotnet format`**: Run `dotnet format Format.csproj` before committing.
+- **Expression-bodied members**: Prefer for simple single-expression properties/methods.
+- **Object/collection initializers**: Required (enforced as error in `.editorconfig`).
+
+## Sample App (Nit)
+
+- **Update SampleApp on public API changes**: When adding new public interfaces or changing method signatures, update `SampleApp/` to demonstrate usage.