review

# Code Review

Perform a rigorous, senior-engineer-level code review of all uncommitted changes in the working tree.

## Core Principles

- **You are a strict reviewer, not a rubber-stamper.** Flag real problems. Do not praise code just to be nice.
- **Never make changes directly.** Your output is a review report and an optional change plan. Wait for explicit user approval before editing any file.
- **Embed assumptions inline.** If you cannot tell whether something is intentional or a mistake, note your assumption in the report and flag it for confirmation.
- **Focus on substance over style.** Formatting issues caught by pre-commit hooks are low priority. **Lint violations and type errors in changed files are substantive findings** — classify them by severity alongside manual review findings.

## Review Depth

By default, run the full checklist. If the user requests a quick review (e.g., `/review --quick`), focus only on:
- Security
- Correctness & Logic
- API & Contract Design
- Error Handling & Resilience

Skip deeper analysis sections (Duplication, Missing Tests, Code Clarity) in quick mode.

## Workflow

### Phase 1 — Gather the diff

If `git diff HEAD` produces no output and `git status` shows no uncommitted changes and no untracked files, inform the user that there are no changes to review and stop.

Run these commands to understand the full scope of uncommitted work:

```bash
# Overview of changed files
git status

# Full diff of all tracked changes (staged + unstaged)
git diff HEAD

# List of untracked files that may need review
git ls-files --others --exclude-standard
```

Read the diff carefully. For every changed file, also read the **full file** (not just the diff hunk) so you understand the surrounding context — imports, class hierarchy, sibling functions, and call sites.

**Run automated checks on changed files (language-aware):**
1. Detect languages from changed file extensions.
2. For Python files (`*.py`): run `ruff check <files>` and `pyright <files>`.
3. For TypeScript/JavaScript files (`*.ts`, `*.tsx`, `*.js`, `*.jsx`, `*.mts`): run `npx tsc --noEmit` and `npx biome check <files>` from the relevant project root (`reflexio/website/` or `reflexio/public_docs/`).
4. Skip linters for languages without configured tooling.

Save the lint and type check output — these results feed into the review checklist below.

### Phase 2 — Understand context

For each changed file:

1. Read the file's component-level `README.md` if one exists (e.g., `reflexio/server/README.md`).
2. Identify the module's responsibility and how it fits into the overall architecture.
3. If the change touches an interface consumed by other modules (API schema, service base class, client method), find and read up to 3 representative consumers — prioritize callers that use the changed interface in different ways.
4. If tests are changed, read the application code under test. If application code is changed, check whether corresponding tests exist and whether they cover the new behavior.

### Phase 3 — Review checklist

Evaluate every change against the following categories. Only report findings that are **actionable** — skip categories where everything looks correct.

When you encounter ambiguity during the checklist, note your assumption inline (e.g., "assuming this is intentional — flagging for confirmation") and continue. Do not stop the review to ask questions.

#### 3.1 Security
- Is user input validated and sanitized before use?
- Are there SQL injection, command injection, or XSS risks?
- Are secrets or credentials hardcoded or logged?
- Are authorization checks in place for protected endpoints?
- Is sensitive data exposed in error messages or logs?
- Are file paths validated to prevent path traversal?

#### 3.2 Correctness & Logic
- Are there off-by-one errors, wrong comparisons, or logic inversions?
- Are edge cases handled (empty inputs, None/null, zero-length collections, boundary values)?
- Do conditional branches cover all expected states?
- Are return types consistent with what callers expect?
- Is async/await used correctly (no missing awaits, no blocking calls in async context)?

#### 3.3 Architecture & Design
- Does the change follow existing patterns in the codebase? If it deviates, is the deviation justified?
- Are responsibilities placed in the right layer (API route vs. service vs. utility)?
- Is there unnecessary coupling between modules that should be independent?
- Are new abstractions justified, or do they add complexity without benefit?
- Does the change violate separation of concerns?
- If a new service/extractor/endpoint is added, does it follow the established pattern?

#### 3.4 API & Contract Design
- Are request/response schemas complete and correct?
- Are field names consistent with existing conventions?
- Are optional vs. required fields set correctly?
- Are default values sensible?
- Is backward compatibility preserved where needed?
- Are enums used where a fixed set of values is expected?

#### 3.5 Error Handling & Resilience
- Are exceptions caught at the right level (not swallowed silently, not leaking implementation details)?
- Are error messages actionable for the caller?
- Are external service failures handled gracefully (LLM calls, database, third-party APIs)?
- Is retry logic appropriate and bounded?
- Are resources cleaned up on failure (connections, file handles)?

#### 3.6 Type Safety & Data Integrity
- Review lint and type check output from Phase 1. All type errors in changed files are findings — classify by severity.
- For TypeScript files, review `tsc` and Biome output from Phase 1 alongside pyright guidance.
- Are type hints present and correct on new/changed functions?
- Are Pydantic models used where structured validation is needed?
- Are there implicit type coercions that could cause subtle bugs?
- Are Optional types handled with proper None checks?
- Are union types narrowed before use?

#### 3.7 Performance
- Are there N+1 query patterns or unnecessary dat