self-review

name	self-review
description	Reviews code quality before staging. Triggers ONLY when: implementation of a feature or fix is complete and code is ready to stage. Do NOT trigger after every small edit — only when a logical unit of work is done.

Self-Review: Code Quality Gate

When this triggers: implementation complete, code written/modified, about to git add, or sub-agent returns code changes.

Non-negotiable: Never skip. Never assume correctness. Every line could contain a bug.

---

0. Process Compliance Check

Before reviewing code quality, verify the process was followed:

• Was pre-code invoked (via Skill tool) before implementation started?
• Was an issue created before code was written?
• Is the branch correctly named and created from dev?
• If ANY of these are NO: STOP. Do not proceed with self-review. Fix the process violation first — create the missing issue, fix the branch, or re-run pre-code.

---

Step 1: Read Your Own Diff

Run git diff and git diff --staged — read EVERY line.

• Read each hunk as if reviewing someone else's PR — do not skim
• Flag lines you did NOT intend to change (accidental whitespace, unrelated modifications)
• If diff is large, read file by file — never skip files
• Confirm every changed line is intentional and necessary

---

Step 2: Correctness

• Solves the stated problem? Re-read issue/request, compare against acceptance criteria
• Edge cases handled? Empty input, null/undefined, zero-length arrays, negative numbers, Unicode, concurrent access
• Off-by-one errors? Loop boundaries, array indexing, string slicing, pagination offsets
• Null checks present? Function params, API responses, DB results, optional chaining — every nullable value checked before use
• Race conditions? Missing await, shared mutable state, async operations that assume ordering
• Return values checked? Non-void function return values must be used or explicitly discarded — never silently ignored (NASA Power of 10, Rule 7)

---

Step 3: Design & Complexity (Google Engineering Practices + Clean Code + SOLID)

• Functions do ONE thing, max ~20 lines (Clean Code, Ch. 3)
• Function arguments: 0 ideal, 1 good, 2 acceptable, 3+ requires refactoring (Clean Code, Ch. 3)
• No boolean/flag arguments — they prove the function does more than one thing (Clean Code, Ch. 3)
• "Too complex" test: if a reader can't understand it quickly, simplify (Google: "too complex")
• No over-engineering — solve the problem NOW, not speculative future needs (Google: YAGNI)
• Single Responsibility: if you describe a class/module with "and", split it (SOLID-S)
• New API? Include a usage example so reviewers see how it's called (Google: small CLs)
• Names are intention-revealing — reader knows what it does from name alone (Clean Code, Ch. 2)
• Comments explain WHY, never WHAT — if code needs a "what" comment, simplify the code (Google)

---

Step 4: Code Hygiene (Clean Code + Boy Scout Rule)

• Delete dead code — don't comment it out, VCS has history (Clean Code, Ch. 4)
• Delete all unused imports, variables, functions — zero tolerance
• No silently swallowed errors: empty catch {}, .catch(() => {}), ignored rejections — every error must be logged or re-thrown
• No backward-compat hacks, alias re-exports, redundant naming (v2/New/Old prefixes)
• No copy-pasted code — if same logic appears twice, extract it (DRY)
• Types are correct and specific — no any in TypeScript unless truly unavoidable, no overly broad types
• Boy Scout Rule: leave the code cleaner than you found it (Clean Code, Ch. 1)

---

Step 5: Security (OWASP Top 10 2021)

• No hardcoded secrets — tokens, API keys, passwords, connection strings — not in code, comments, or config literals
• A03 Injection: parameterized queries for ALL database access — never string concatenation for SQL/NoSQL
• A03 XSS: escape/sanitize all user-supplied content before rendering in HTML
• A01 Broken Access Control: deny by default, enforce authorization on every endpoint/action
• A09 Security Logging: log auth attempts and access control failures, but NEVER log sensitive data (passwords, tokens, PII)
• No command injection: never pass user input to exec(), eval(), child_process, or shell commands
• Input validation at system boundaries: API endpoints, form handlers, webhooks — validate and sanitize before processing

---

6. Scope Check

• Run git diff --stat — are ALL changed files within the scope identified in pre-code?
• If any file was modified that is NOT in the expected scope: justify it or revert it.
• Out-of-scope improvements spotted during development belong in new issues, not this PR.

---

7. Agent Output Verification

• "Build passes" ≠ "product works." If the build/tests pass, that verifies CODE correctness, not PRODUCT correctness.
• Have you actually SEEN the output? Not read the code — seen the rendered result, clicked the buttons, observed the behavior?
• If a sub-agent reported success: verify it yourself. Agents can write code but cannot judge product quality.
• Rule: Never trust "it should work." Run it. See it. Click it. If you can't verify it visually, flag it for staging-verify or manual testing.

---

Step 7.5: Generalized Learning

If ANY issue was found and fixed during this self-review, extract a REUSABLE PRINCIPLE — not a log entry.

Process:

1. Identify the error type: What category does this belong to? (auth config, scope creep, missing test, data model flaw, etc.)
2. Generalize: Ask "what universal rule would have prevented this AND all similar future mistakes?" Strip away project-specific details.
3. De-duplicate: Read memory/principles.md — does a similar principle already exist? If yes → strengthen the wording. If no → add it.
4. Write one line to memory/principles.md:
   • Format: - [Category] [Universal principle]
   • ✅ Good: - [Auth] Any third-party auth integration must verify redirect URLs for ALL deployment environments in pre-code scope
   • ✅ Good: - [Testing] Never trust "build passes" — run the full user journey manually or with Playwright after every deploy
   • ❌ Bad: - [2026-04-05] Forgot OAuth redirect for staging (this is an event log, not a principle)
   • ❌ Bad: - Fixed the Supabase Site URL issue (this is project-specific, not reusable)
5. Prune during retro: In the retro skill, review memory/principles.md. Remove principles that haven't been relevant in 3+ months. Merge overlapping principles.

The goal: memory/principles.md is a CONCISE list of 10-30 universal principles that improve every future project — not a growing log of past incidents.

---

Step 8: Verdict

If ANY issue was found:

1. Fix it immediately — do not proceed with staging or committing
2. Re-run self-review from Step 1 on the new diff
3. Repeat until review is clean

When clean, report:

• Which steps completed, issues found and fixed
• State: "Self-review passed. Ready to stage/commit."

If uncertain about correctness:

• Run the code, write a test, or verify — do not guess
• "It should work" is NOT acceptable — prove it works

Next Steps

Report to user: "Self-review [PASSED/FAILED]. Issues found: [N]. Fixed: [N]. Remaining: [N]"

Suggested next steps (user decides):

• Passed → "Run test-gate"
• UI changes → "Plan ux-audit after PR"