code-review

# Code Review Run a structured review of branch changes covering security, language strictness, performance, and conventions. ## Workflow ### Step 1: Detect changes ```bash # Default base branch is main. User can override: /code-review --base staging BASE=${1:-main} git fetch origin "$BASE" --quiet 2>/dev/null || true MERGE_BASE=$(git merge-base "origin/$BASE" HEAD) BRANCH=$(git rev-parse --abbrev-ref HEAD) # Changed files and stats git diff --name-status "$MERGE_BASE"..HEAD git diff --stat "$MERGE_BASE"..HEAD git log --oneline "$MERGE_BASE"..HEAD ``` Use `$MERGE_BASE..HEAD` for ALL diffs. Never use `origin/main..HEAD` directly -- it includes changes from other merged branches. ### Step 2: Auto-detect languages Map file extensions to rule sets: | Extension | Rule set | Skill | |---|---|---| | `.ts`, `.tsx` | TypeScript | `typescript-strict` | | `.rs` | Rust | `rust-strict` | | `.swift` | Swift | `swift-strict` | | `.go` | Go | `go-strict` | | `.js`, `.mjs`, `.cjs` | JavaScript | `javascript-strict` | Only apply rules for languages present in the diff. ### Step 3: Run checks Run in this order. For each finding, record file, line number, description, and rule ID. #### 3a. Security scan (security-audit-standard) Scan the diff output (not the full repo) for: - **Secrets**: API keys, tokens, passwords, connection strings (`sk-`, `ghp_`, `AKIA`, `-----BEGIN PRIVATE KEY`) - **Input validation**: unvalidated user input reaching SQL, HTML, shell commands, file paths - **Injection risks**: string concatenation in queries, `eval()`, `innerHTML`, unsanitized template literals - **Auth issues**: missing auth checks on endpoints, weak JWT config, secrets in client-side code - **File patterns**: `.env`, `*.pem`, `credentials.json` committed #### 3b. Language strictness checks Apply the top rules for each detected language (see Quick Reference Tables below). Read the changed lines and flag violations. #### 3c. Performance scan (performance-audit-standard) Look for these anti-patterns in the diff: - O(n) membership checks where Set/Map should be used - O(n^2) nested loops (`.filter()` inside `.map()`, `includes()` inside loop) - Sort to find min/max (O(n log n) vs O(n) single pass) - Sync file I/O in async context (`writeFileSync`, `std::fs::write` under async) - Lock held across `.await` or disk I/O (Rust) - Per-request client/regex creation instead of singleton - Unbounded caches without capacity or TTL - Multiple array passes where a single pass works #### 3d. Style and conventions (github-standards) - Commit messages follow `type(scope): subject` format - No `console.log`/`print`/`println!` debugging left in code - No commented-out code blocks - No TODO/FIXME without a ticket reference - Error handling present (no empty catch blocks, no swallowed errors) - Naming conventions match language idioms ### Step 4: Generate review summary ## Output Format ``` ## Code Review: [branch-name] **Base**: [base-branch] | **Files changed**: N | **Commits**: N ### CRITICAL (must fix before merge) - [file:line] Finding description (RULE-ID) ### HIGH (should fix) - [file:line] Finding description (RULE-ID) ### MEDIUM (consider fixing) - [file:line] Finding description (RULE-ID) ### LOW (nitpick) - [file:line] Finding description (RULE-ID) ### Passed checks - No secrets detected in diff - Error handling present - Input validated at boundaries - Async operations properly awaited - Resources cleaned up - Commit messages follow conventions ``` Omit empty severity sections. Always show "Passed checks" to confirm what was verified. ## Quick Reference: Top 5 Rules Per Language ### TypeScript | ID | Rule | What to flag | |---|---|---| | TS-01 | No `any` | `any` type annotation without justification comment | | TS-02 | No `as` assertions | `as Type` without preceding type guard | | TS-04 | No `@ts-ignore` | Use `@ts-expect-error` with explanation instead | | TS-12 | Narrow catch errors | `catch (err)` without `instanceof Error` check | | TS-13 | No silent catch | Empty `catch {}` or catch without logging | ### Rust | ID | Rule | What to flag | |---|---|---| | -- | No `.unwrap()` | `.unwrap()` in non-test code without `// BUG IF:` comment | | -- | No `.expect()` | `.expect()` outside static init (`LazyLock`, `OnceLock`) | | -- | SAFETY comments | `unsafe` block without `// SAFETY:` comment | | -- | Lock across await | `RwLock`/`Mutex` guard held across `.await` | | -- | Bounded caches | `HashMap` used as cache without capacity limit | ### Swift | ID | Rule | What to flag | |---|---|---| | SW-01 | No force unwrap | `!` on optionals in production code | | SW-03 | No silent `try?` | `try?` on critical operations (file creation, auth, data save) | | SW-09 | `@MainActor` on VMs | ViewModel missing `@MainActor` annotation | | SW-12 | Task cancellation | `Task { }` without `[weak self]` or cancellation check | | SW-15 | `[weak self]` | Async closures with strong `self` capture | ### Go | ID | Rule | What to flag | |---|---|---| | GO-01 | Wrap errors | `return err` without `fmt.Errorf("context: %w", err)` | | GO-02 | Check errors | Ignored error return value | | GO-03 | Drain body | `resp.Body.Close()` without `io.Copy(io.Discard, ...)` on error paths | | GO-07 | RWMutex | Write lock where read lock suffices, or missing double-check | | GO-17 | No hardcoded secrets | `const apiKey = "..."` or similar | ### JavaScript | ID | Rule | What to flag | |---|---|---| | JS-01 | No `var` | `var` declaration anywhere | | JS-03 | No silent catch | Empty catch block or catch without action | | JS-05 | Bounded retries | Recursive retry without max attempt limit | | JS-08 | Async file I/O | `writeFileSync`, `readFileSync` in server code | | JS-17 | No `eval()` | `eval()` or `new Function()` with dynamic input | ## Common Anti-Patterns Checklist Quick-scan checklist applied to ALL languages: - [ ] No hardcoded secrets or API keys in diff - [ ] All errors handled (not swallowed with empty catch) - [ ] No `console.log`/`print`/`println!`/`NSLog` debugging left - [ ] No commented-out code blocks - [ ] No TODO/FIXME without ticket reference - [ ] Input validated at system boundaries - [ ] No force-unwrap / force-cast / `any` type escape - [ ] Async operations properly awaited (no fire-and-forget) - [ ] Resources cleaned up (listeners, subscriptions, file handles, DB connections) - [ ] No unbounded collections in hot paths (use bounded cache, Set, or Map) ## Branch Comparison Options ### Review against a different base branch ``` /code-review --base staging /code-review --base develop ``` Override the default `main` base branch. ### Review specific files only ``` /code-review --files src/auth.ts src/middleware.ts ``` Limits the review to listed files. Use `git diff $MERGE_BASE..HEAD -- <file>` per file. ### Review a PR by number ```bash # Fetch PR metadata and diff gh pr view <NUMBER> --json title,body,headRefName,baseRefName,files gh pr diff <NUMBER> ``` Use `gh pr diff` output as the diff source instead of `git diff`. ### Review a specific commit range ``` /code-review --range abc123..def456 ``` Uses the provided range instead of computing merge-base.