code-review

> Ported from oh-my-codex `code-review`. OMX runtime conventions (`$macro` invocation, `omx` CLI, `.omx/` state directory) are replaced with WorkBuddy idioms (Skill tool, Agent tool, task list, `.workbuddy/memory`). # Code Review Skill Conduct a thorough code review for quality, security, and maintainability with severity-rated feedback. ## When to Use - User requests "review this code", "code review". - Before merging a pull request. - After implementing a major feature. - User wants a quality assessment. ## Behavior - Default to outcome-first reporting: state the target result, evidence, validation status, and stop condition before adding process detail. - If correctness depends on additional inspection, retrieval, execution, or verification, keep using the relevant tools until the review is grounded; stop once enough evidence exists. - Continue through clear, low-risk, reversible next steps automatically; ask the user only when the next step is materially branching, destructive, credentialed, external-production, or preference-dependent. ## Workflow — Two Parallel Lanes Delegate to two independent agents via the **Agent tool**, run in parallel, for a two-lane review: 1. **Identify Changes** - Run `git diff` to find changed files. - Determine scope of review (specific files or entire PR). 2. **Launch Parallel Review Lanes** (use the Agent tool, two calls in one message) - **Code-reviewer lane** — owns spec compliance, security, code quality, performance, and maintainability findings. Prompt it to act as a senior code reviewer. - **Architect lane** — owns the devil's-advocate / design-tradeoff perspective. Prompt it to act as a skeptical architect. - Both lanes run on a clean context with explicit scope and artifacts, and produce distinct outputs before final synthesis. - If either lane cannot be launched or does not return evidence, report `independent review unavailable`; do **not** substitute your own authoring lane, and do **not** approve or mark the review merge-ready. 3. **Review Categories** - **Security** — hardcoded secrets, injection risks, XSS, CSRF. - **Code Quality** — function size, complexity, nesting depth. - **Performance** — algorithm efficiency, N+1 queries, caching. - **Best Practices** — naming, documentation, error handling. - **Maintainability** — duplication, coupling, testability. 4. **Severity Rating** - **CRITICAL** — security vulnerability (must fix before merge). - **HIGH** — bug or major code smell (should fix before merge). - **MEDIUM** — minor issue (fix when possible). - **LOW** — style/suggestion (consider fixing). 5. **Architectural Status Contract** - **CLEAR** — no unresolved architectural blocker was found. - **WATCH** — non-blocking design/tradeoff concern that must appear in the final synthesis. - **BLOCK** — unresolved design concern that prevents a merge-ready verdict. 6. **Specific Recommendations** - File:line locations for each issue. - Concrete fix suggestions. - Code examples where applicable. 7. **Final Synthesis** - Combine the code-reviewer recommendation and the architect status into one final verdict. - Approval requires explicit evidence from both independent lanes; missing or failed delegation is a blocking unavailable-review state, not an approval fallback. - Deterministic merge gating rules: - If architect status is **BLOCK**, final recommendation is **REQUEST CHANGES**. - Else if code-reviewer recommendation is **REQUEST CHANGES**, final recommendation is **REQUEST CHANGES**. - Else if architect status is **WATCH**, final recommendation is **COMMENT**. - Else final recommendation follows the code-reviewer lane. - The final report must make architect blockers impossible to miss. ## Agent Delegation Do not self-review as a fallback. If a lane is missing, unavailable, skipped, or fails, emit a clear unavailable-review result and block approval until independent lane evidence exists. Spawn the two lanes like this (parallel Agent tool calls): ``` Agent(code-reviewer lane): "Act as a senior code reviewer. Review the changes in <git diff / specific files> for quality, security, and maintainability. Checklist: OWASP Top 10 security, complexity/duplication, N+1/efficiency, naming/docs/ error handling, coupling/testability. Output: files reviewed count, issues by severity (CRITICAL/HIGH/MEDIUM/LOW), specific file:line locations, fix recommendations, and an approval recommendation (APPROVE / REQUEST CHANGES / COMMENT)." Agent(architect lane): "Act as a skeptical architect reviewing the same changes from the architecture/tradeoff perspective. Focus on system boundaries/interfaces, hidden coupling, long-term maintainability risks, and the strongest counterargument against approving as-is. Output: Architectural Status (CLEAR / WATCH / BLOCK), file:line evidence, and a concrete tradeoff/design recommendation." ``` Run both lanes in parallel, then synthesize with the deterministic rules above. ## Output Format ``` CODE REVIEW REPORT ================== Files Reviewed: 8 Total Issues: 12 Architectural Status: WATCH CRITICAL (0) ----------- (none) HIGH (0) -------- MEDIUM (7) ---------- 1. src/api/auth.ts:42 Issue: Email normalization logic is duplicated instead of reusing the shared helper Risk: Validation rules can drift between authentication paths Fix: Route both paths through the shared normalization helper ... LOW (5) ------- ARCHITECTURE WATCHLIST ---------------------- - src/review/orchestrator.ts:88 Concern: Review result synthesis relies on implicit ordering rather than an explicit blocker contract Status: WATCH Recommendation: Define deterministic merge gating before expanding reviewers SYNTHESIS --------- - code-reviewer recommendation: COMMENT - architect status: WATCH - final recommendation: COMMENT RECOMMENDATION: COMMENT Address any WATCH concerns before treating the change as merge-ready. ``` ## Review Checklists **Code-reviewer lane:** - Security: no hardcoded secrets; inputs sanitized; SQL/NoSQL injection prevented; XSS escaped; CSRF on state-changing ops; authz enforced. - Code Quality: functions < 50 lines (guideline); cyclomatic complexity < 10; no nesting > 4 levels; DRY; clear naming. - Performance: no N+1; appropriate caching; efficient algorithms; no unnecessary re-renders. - Best Practices: error handling; logging; public-API docs; tests for critical paths; no commented-out code. **Architect lane:** - Boundary/interface changes are explicit. - New coupling/tradeoff risks are surfaced. - Long-horizon maintainability concerns are evidence-backed. - Architectural status is CLEAR / WATCH / BLOCK. - Any BLOCK cites why merge-ready status should be withheld. ## Approval Criteria - **APPROVE** — code-reviewer returns APPROVE, architect status is CLEAR, and both independent lanes returned evidence. - **REQUEST CHANGES** — code-reviewer returns REQUEST CHANGES, architect status is BLOCK, or required independent review delegation is unavailable/skipped/failed. - **COMMENT** — code-reviewer returns COMMENT with architect status CLEAR, architect status is WATCH, or only LOW/MEDIUM improvements remain. ## Best Practices - Review early and often; small frequent reviews beat huge ones. - Address CRITICAL/HIGH first. - Consider context — some "issues" may be intentional trade-offs. - Learn from reviews to improve coding practices. > Note: `code-review` itself is read-only and does **not** auto-fix. To fix findings, hand off to execution via the Agent tool or a `team` skill.