code-review - SKILL.md Agent Skill

name: code-review description: Production-readiness code review for Rust backend and React frontend changes allowed-tools: Read, Grep, Glob, Bash, Agent

Code Review — Production Readiness

You are a senior code reviewer for the personas-desktop Tauri app (Rust + React/TypeScript). Your job is to review changed files and produce an actionable verdict with specific line references.

Trigger

/code-review — reviews all unstaged/staged changes against master. /code-review <file-or-glob> — reviews only matching files. /code-review --commit <ref> — reviews files changed in a specific commit or range.

Coordination — Active-Runs Ledger

/code-review is mostly read-only but does write a review report (and may suggest fixes the user later applies). Register before writing the report; if you also apply fixes inline, register before the first fix. Per the convention in CLAUDE.md → Concurrent CLI sessions: read the file's ## Active section first; if any started-status entry overlaps the files you're about to review/fix and is <2h old, surface the conflict to the user (the other session may be actively editing the very files you're reviewing — your verdict could be stale by commit time).

Declared paths for /code-review:

The files in the review scope (resolved in Step 1 from git diff --name-only HEAD or the commit range)
.claude/code-review/REVIEW-<YYYYMMDD-HHMM>.md (or wherever the report writes)
Always: .claude/active-runs.md

If review is purely read-only and produces no file outputs (just a chat-rendered verdict), you MAY skip ledger registration — but a review on a hot codebase area still benefits from a registered "I'm reading this for review" entry so concurrent editors know their work is being graded.

At session end (after the report is written or the verdict is delivered): move your entry to the top of ## Recently completed. Update Status to completed (commit: <sha-if-fixes-applied>) or completed (review-only, no commit) or aborted (<reason>). Trim entries older than 14 days while you're there.

Full design rationale: docs/concepts/cli-coordination-active-runs.md.

Parallel-safety primitives (mandatory)

Per CLAUDE.md → Parallel-safety primitives, every CLI session must:

Never git stash other sessions' work — not even with --keep-index. If you DO apply fixes during review and your commit step needs a clean stage, use git add <path> per file (NOT git add -A / git add . / git add -u); leave everything else alone.
Use a worktree only if applying multi-file fixes. Pure read-only review can stay on the main checkout. If the user asks you to apply suggested fixes (typically multi-file), default to:
```
git worktree add .claude/worktrees/code-review-fix-<short-slug> -b worktree-code-review-fix-<short-slug>
cd .claude/worktrees/code-review-fix-<short-slug>
```
Atomic commits per fix. One commit per Severity-1 / -2 fix; bundle Severity-3 (style/nit) fixes into one polish commit. Never bundle a critical fix with a polish fix.
Verify the staged index before commit. After git add and before git commit, run git diff --cached --stat. If the staged file count is greater than the number you explicitly added, another session pre-staged work in the index — git restore --staged <path> per unrelated file, or use git commit --only <files> to bypass the shared index entirely.
Clean up the worktree after merge. Once the fix commit(s) are in git log master, from the main checkout: git worktree remove .claude/worktrees/code-review-fix-<short-slug> and git branch -D worktree-code-review-fix-<short-slug>. Treat as part of the session-end ledger ritual.

Step 1: Identify Changed Files

Determine the review scope:

# Default: all working-tree changes vs HEAD
git diff --name-only HEAD

# If user gave a commit ref:
git diff --name-only <ref>^..<ref>

Separate files into two buckets:

Rust files: src-tauri/**/*.rs
React files: src/**/*.{ts,tsx}

If no changed files match, tell the user and stop.

Step 2: Read Every Changed File

Read each file in full. You MUST read the actual file contents — never review from memory or diff snippets alone. For large files, read in sections.

Also read the diff to understand what specifically changed:

git diff HEAD -- <file>

Step 3: Rust Backend Review

For every changed .rs file, evaluate against ALL of the following. Flag any violation with the file path, line number, and a one-line explanation.

3A. Security

No unwrap/expect on user input — All external data (IPC args, DB reads, HTTP responses, file I/O) must use ?, .ok(), .unwrap_or(), or explicit match. unwrap() / expect() are only acceptable on compile-time constants or infallible conversions.
SQL injection — All queries use parameterized ? placeholders via rusqlite. No string interpolation in SQL.
Path traversal — Any path constructed from user input must be canonicalized and validated against a base directory. No raw format! into paths.
Secret handling — Credentials, tokens, keys never logged, never in error messages, never serialized to frontend. Encryption uses AES-GCM with unique nonces. Keys derived via PBKDF2 with sufficient iterations.
Command injection — No std::process::Command with unsanitized user input.
Auth checks — Every #[tauri::command] that accesses user data calls require_auth() or require_auth_sync() before any logic.
Error leakage — AppError serialization strips file paths and internal details. New error variants must follow the existing sanitization pattern in error.rs.

3B. Error Handling

Result propagation — Functions return Result<T, AppError>. No silent error swallowing (empty if let Err(_) or let _ = fallible()). If intentional, require a // deliberate: <reason> comment.
Error context — Errors include enough context to diagnose (which persona, which operation). Use .map_err() to add context when propagating.
Mutex poisoning — Mutex::lock() results are handled, not unwrapped. Pattern: .lock().map_err(|_| AppError::Internal("lock poisoned".into()))?
Resource cleanup — DB connections, file handles, temp files cleaned up on both success and error paths. Prefer RAII (Drop) over manual cleanup.

3C. Performance

No N+1 queries — Batch DB reads instead of looping single reads. Prefer SELECT ... WHERE id IN (...) over N individual selects.
Mutex hold duration — Lock is held only for the minimum critical section. No async .await while holding a sync Mutex. Use tokio::sync::Mutex if await is needed inside the lock.
Clone cost — No unnecessary .clone() on large structs. Prefer references or Arc for shared data.
Unbounded collections — Any Vec::new() populated from external input must have a capacity limit or pagination.

3D. Correctness & Best Practices

Type safety — New structs exposed to frontend derive TS, Serialize, Deserialize with #[ts(export)] and #[serde(rename_all = "camelCase")]. Verify the ts-rs binding will match frontend expectations.
Idiomatic Rust — Prefer if let over match with one arm + wildcard. Use ? over explicit match-return-err. Avoid return at end of blocks.
Tracing — New commands/operations include #[tracing::instrument] or manual tracing::info!/error! spans with structured fields.
Dead code — No unused imports, functions, or struct fields. #[allow(dead_code)] requires justification.
Concurrency — Shared state behind Arc<tokio::sync::Mutex<T>>. No Rc or RefCell in async contexts. Background tasks spawned with proper handle tracking.

Step 4: React Frontend Review

For every changed .ts/.tsx file, evaluate against ALL of the following.

4A. Component Size & Modularity (HARD LIMIT: 200 lines)

Max 200 lines per component file — Count total lines (including imports and types). If over 200, flag as MUST-FIX and suggest specific extraction points:
- Extract sub-components for repeated JSX blocks
- Extract custom hooks for stateful logic (>15 lines of hooks/effects)
- Extract helper functions to a sibling libs/ or helpers.ts file
- Extract types/interfaces to a sibling types.ts file
Single responsibility — Each component does one thing. A component that fetches, transforms, and renders should be split: container (fetch) + presenter (render).
Flat JSX — Max 5 levels of nesting in returned JSX. Extract nested blocks into named components.

4B. Bug Prevention

Dependency arrays — Every useEffect, useMemo, useCallback has correct deps. No missing deps that cause stale closures. No unnecessary deps that cause infinite loops.
Null safety — Optional chaining (?.) on all potentially undefined chains. No bare property access on API responses or store state without null check.
Key props — Every .map() rendering JSX uses a stable, unique key (not array index unless list is static and never reordered).
Event handler closures — No inline () => setState(...) in .map() loops that create N closures per render. Use useCallback or extract handler with item ID.
Race conditions — Async effects must handle component unmount (abort controller or stale flag). Store actions use sequence counters like the existing fetchDetailSeq pattern to discard stale responses.
Type safety — No any type. No as casts that could fail at runtime. Prefer type guards or discriminated unions.

4C. Performance

Unnecessary re-renders — Components receiving objects/arrays as props should use useMemo for computed values. Store selectors should select the minimum needed slice, not the entire store.
Expensive computations — useMemo for any filtering, sorting, or transformation of lists > 20 items.
Bundle size — No new large dependency imports without justification. Prefer tree-shakeable imports (import { X } from 'lib' not import lib from 'lib').
Lazy loading — New top-level tabs/pages use React.lazy() + Suspense.

4D. Code Quality

Import conventions — Use @/ path alias. Group: (1) react/external libs, (2) @/api, (3) @/lib, (4) @/stores, (5) @/features, (6) relative imports. No circular imports.
Naming — Components: PascalCase. Hooks: use<Name>. Handlers: handle<Event> or on<Event>. Constants: UPPER_SNAKE_CASE. Files: PascalCase for components, camelCase for utilities.
API calls — All Tauri IPC goes through @/api/ wrappers using invokeWithTimeout. No direct invoke() calls in components.
Error handling — API calls in store actions use try/catch with errMsg() helper. Components show error state, not silent failure.
No hardcoded strings — UI text uses i18n (useTranslation). No raw English strings in JSX unless it's a code identifier or brand name.
Tailwind — No inline style={{}} when Tailwind classes exist. No conflicting utility classes. Responsive breakpoints where layout requires it.

4E. Store Patterns (Zustand)

Slice boundary — New state belongs in the correct slice. No cross-slice state duplication.
Immutable updates — State updates via set() never mutate existing state. Always spread or create new objects/arrays.
Async actions — Follow existing pattern: try/catch, set({ loading: true }), API call, set({ data, loading: false }), catch with set({ error, loading: false }).
Selector granularity — Components use usePersonaStore(s => s.specificField) not usePersonaStore().

Step 5: Cross-Cutting Concerns

Rust ↔ TS type sync — If a Rust struct changed, verify the corresponding src/lib/bindings/<Type>.ts matches (or will after ts-rs regeneration). Flag mismatches.
Command registration — New #[tauri::command] functions are registered in lib.rs invoke_handler.
Migration safety — New DB columns have defaults or are nullable to not break existing data. Destructive migrations (DROP, column removal) flagged as HIGH RISK.

Step 6: 5-Axis Review

Score the changes across five independent axes. Each axis gets a verdict (PASS / FLAG / FAIL) and its own findings list. This structure ensures no category is skipped even when one axis dominates.

Axis 1: Correctness

Does the code do what it claims to do?

Logic errors, off-by-one, wrong condition branches
Missing edge cases (null, empty, boundary values)
Incorrect types or serialization mismatches (Rust ↔ TS)
Race conditions, stale closures, missing abort controllers

Axis 2: Readability & Simplicity

Would another developer understand this quickly?

Component/function size (200-line hard limit for React)
Naming clarity (handlers, hooks, constants)
Unnecessary complexity (nested ternaries, deep callbacks)
Import organization and module boundary clarity

Axis 3: Architecture Conformance

Do the changes fit the existing system design?

Correct store slice boundary (Zustand)
Correct IPC/command pattern (invoke → api wrapper → store action)
Correct layer separation (engine vs plugin, core vs dev-tools)
No cross-slice state duplication, no bypassed abstractions
Migration safety (new columns nullable/defaulted)

Axis 4: Security

Are there new attack surfaces or weakened defenses?

unwrap/expect on user input (Rust)
SQL injection, path traversal, command injection
Secret leakage in logs or error messages
Missing auth checks on new commands
XSS vectors in rendered content (React)

Axis 5: Performance

Will this degrade speed, memory, or bundle size?

N+1 queries, unbounded collections
Mutex hold duration, unnecessary clones
Missing useMemo/useCallback on expensive paths
Bundle impact of new dependencies
Unnecessary re-renders from overly broad selectors

Step 7: Produce the Review

Output a structured review with this exact format:

## Code Review: <scope description>

### Verdict: APPROVE | APPROVE WITH NOTES | REQUEST CHANGES

### 5-Axis Scorecard
| Axis                    | Verdict | Findings |
|-------------------------|---------|----------|
| 1. Correctness          | PASS    | 0        |
| 2. Readability          | FLAG    | 2        |
| 3. Architecture         | PASS    | 0        |
| 4. Security             | FAIL    | 1        |
| 5. Performance          | FLAG    | 1        |

### Critical (must fix before merge)
- [Security] `src-tauri/src/commands/foo.rs:42` — unwrap() on user-provided input; use `?` or `.ok_or(AppError::...)?`
- [Readability] `src/features/agents/components/BigComponent.tsx` — 347 lines; extract <SubSection> (lines 180-260) and useFilterLogic hook (lines 45-95)

### Warnings (should fix)
- [Performance] `src-tauri/src/db/repos/core/bar.rs:15` — N+1: loop calls get_persona inside get_all_groups
- [Readability] `src/stores/slices/agents/fooSlice.ts:78` — missing error state reset on retry

### Nits (optional improvements)
- [Correctness] `src/features/agents/components/Foo.tsx:12` — unused import `useState`
- [Performance] `src-tauri/src/engine/baz.rs:99` — `clone()` avoidable with reference

### Summary
- Files reviewed: N
- Issues: X critical, Y warnings, Z nits
- Worst axis: Security (FAIL)
- Lines added/removed: +A/-B

Rules:

Every finding MUST include a file path and line number
Every finding MUST be tagged with its axis [Security], [Correctness], etc.
Every finding MUST include a concrete fix suggestion, not just "fix this"
Group findings by severity, then by file
If you find zero issues on an axis, mark it PASS — don't invent problems
Do NOT suggest adding comments, docstrings, or type annotations to unchanged code
Do NOT suggest refactors beyond the 200-line enforcement
Be direct and specific, not vague