code-review

> **GUIDANCE SKILL — INSTRUCTIONS ONLY** > Code review is a systematic examination of source code quality across multiple dimensions: logic, security, performance, maintainability, and standards compliance. > This skill provides patterns, conventions, and constraints. It contains no executable code — apply the guidance to your context, adapting specifics as needed per C2-B. ## CRITICAL: Guidance Principles The instructions below define the **REQUIRED APPROACH** — you MUST follow the prescribed patterns, conventions, and constraints. **Mandatory adherence:** - Follow all stated conventions and naming patterns - Respect all constraints and boundaries - Apply the recommended patterns to your implementation - Do NOT deviate from prescribed architectural decisions - Never modify any code; the review is strictly read-only - Every issue reported MUST include a concrete, actionable fix suggestion ## Correct Application Pattern 1. Read and absorb all guidance sections below 2. Map the prescribed patterns to your current context 3. Implement following the stated conventions — adapt specifics to fit, but preserve the intent 4. Validate against the C3 checklist at the end ## Argument Details | Arg | Type | Required | Default | Description | |-----|------|----------|---------|-------------| | target | string | No | (empty) | File or directory to review; leave empty to review git staged changes | ## Design Patterns & Conventions ### Architectural Pattern Multi-dimensional assessment: every review MUST examine the target code through all five review dimensions — Logic Correctness, Security, Performance, Maintainability, and Standards Compliance. Issues from each dimension are independently classified and then merged into a single ranked report. ### Naming Conventions Issues are classified by severity using four standard levels: | Severity | Meaning | |----------|---------| | Critical | Security vulnerability, data loss risk, or production-breaking bug | | High | Logic error with significant impact, or major performance regression | | Medium | Maintainability concern, minor duplication, or unclear naming | | Low | Style nit, non-critical lint warning, or cosmetic improvement | ### File / Module Organization The output is a single structured Markdown report composed of: 1. An **issue table** with columns: Severity, File:Line, Description, Fix Suggestion 2. An **overall score** (1–10) with key areas for improvement Issues in the table are ordered by severity (Critical first, then High, Medium, Low). ### Data Flow ``` Determine scope (staged changes or {{target}}) → Read all changed/in-scope files → Analyze against each review dimension → Classify each finding by severity → Merge and rank all issues → Produce structured Markdown report (table + score) ``` ## Constraints & Boundaries ### Hard Constraints (non-negotiable) - **Read-only**: Never modify any code, files, or configuration. The review is purely observational. - **Concrete fix suggestions**: Every reported issue MUST include a specific, actionable fix suggestion — never flag a problem without offering a remedy. - **All five dimensions**: Every review MUST examine Logic, Security, Performance, Maintainability, and Standards Compliance. If a dimension yields no findings, explicitly note that. - **Severity ranking**: Every issue MUST be assigned exactly one severity level (Critical, High, Medium, Low). ### Soft Guidelines (preferred but flexible) - Prefer concrete code examples in fix suggestions over abstract advice. - Prioritize security issues over performance issues when ranking within the same severity tier. - Include the project's lint command output when available; run lint as part of the review process. - Review surrounding context (not just the diff) to catch integration issues. ## Implementation Guidance ### Step-by-step Approach 1. **Determine scope**: If `{{target}}` is provided, scope to that file or directory. Otherwise, obtain git staged changes via `git diff --staged`. 2. **Read all target files**: Load every file in scope into context. For diffs, also read the full file to understand surrounding code. 3. **Lint check**: Run the project's lint command (e.g., `npm run lint`, `eslint`, `golangci-lint`) and capture violations. 4. **Dimension-by-dimension analysis**: - **Logic Correctness**: Examine edge cases, null handling, off-by-one errors, concurrency issues, incorrect conditions. - **Security**: Check for injection risks (SQL, XSS, command), sensitive data exposure, missing permission checks, hardcoded secrets. - **Performance**: Look for unnecessary loops, N+1 queries, potential memory leaks, blocking I/O, unindexed queries. - **Maintainability**: Assess naming clarity, function length, duplicated code, comment quality, test coverage gaps. - **Standards Compliance**: Map lint violations to the report; flag any project convention deviations. 5. **Classify and rank**: Assign a severity to each finding. Sort Critical → High → Medium → Low. 6. **Produce report**: Build the Markdown issue table, compute the overall score (1–10), and list key improvement areas. ### Key Decisions | Decision | Rationale | Trade-offs | |----------|-----------|------------| | Default to staged changes | Most common review use case; minimizes noise | May miss unstaged work in progress | | Five review dimensions | Balanced coverage of quality attributes | Some overlap between Maintainability and Standards Compliance | | Severity-ranked table format | Actionable, scannable, and easy to integrate into tooling | Less narrative context than prose | | Table before score | Allows reviewer to derive score from issue inventory | Score may feel disconnected if table is long | | Read-only enforcement | Prevents accidental modification during review | Cannot auto-fix trivial issues | ## Usage Notes ### Input / Output **Input:** A file path, directory path, or nothing (defaults to git staged changes). When `{{target}}` is specified, all files within that scope are reviewed. When omitted, `git diff --staged` determines the change set. **Output:** A Markdown report containing: 1. An issue table with columns: **Severity**, **File:Line**, **Description**, **Fix Suggestion** — sorted by severity descending. 2. An **Overall Score** (1–10). 3. **Key Areas for Improvement** — a bullet list of the highest-priority action items. ### Feature Support | Feature | Input | Output | |---------|-------|--------| | Staged changes review | (empty target) | Diff-based report | | Single file review | File path | Targeted report | | Directory review | Directory path | Multi-file aggregated report | | Lint integration | Project lint command | Lint violations in Standards Compliance | ### Edge Cases - **Empty diff / no changes**: Report "No changes to review" with score N/A — do not fabricate issues. - **Binary files in scope**: Skip binary files; note them as "skipped (binary)" in the report. - **Very large files**: If a file exceeds reasonable context size, review the diff portion only and note the limitation. - **Files outside workspace**: If `{{target}}` points outside the workspace root, report the error and halt. - **Missing lint command**: If no lint configuration is found, skip the Standards Compliance lint step and note that lint was unavailable. ### Self-Check Before C3 Before running the formal C3 verification, confirm: - All five dimensions have been examined (even if some yielded no findings). - Every issue row has all four columns populated (Severity, File:Line, Description, Fix Suggestion). - The severity classification is internally consistent (no Critical issue ranked below a Low issue). - The overall score and key improvement areas are present. - No code has been modified. ## Common Pitfalls > **Over-adaptation**: "Adapt specifics to fit" does not mean rewrite the core patterns. When in doubt, preserve the prescribed approach over local convenience — the patterns exist for a reason. > **Pattern drift**: As implementation progresses, it's easy to gradually deviate from conventions. Regularly re-read the Design Patterns section to catch drift early. > **Surface-level review**: Do not limit findings to style and lint issues. Deeply examine logic correctness and security — those are the highest-value dimensions of a code review. > **Missing context**: Reviewing only the diff lines without reading surrounding code can miss integration bugs. Always read enough context to understand how the change fits into the broader module. > **Vague fix suggestions**: "Fix the bug" or "Improve this" are not actionable. Every fix suggestion must be concrete — propose a specific code pattern, refactor, or library usage. ## C3 Verification | Check Item | Constraint | Common Omission | Method | |------------|-----------|----------------|--------| | Pattern adherence | Prescribed patterns followed | Divergent implementation | Compare against Design Patterns section | | Naming conventions | Conventions applied consistently | Inconsistent naming | Grep / code review | | Hard constraints | All non-negotiable constraints met | Constraint violation | Manual checklist verification | | File organization | Modules/files structured as prescribed | Misplaced or missing files | Directory structure review | | All dimensions covered | Five review dimensions examined | Missing Security or Performance | Cross-reference report against Review Dimensions | | Issue format complete | Every issue has Severity + File:Line + Description + Fix | Missing fix suggestion | Table cell inspection | | Overall score present | Score (1–10) + key areas provided | Score without justification | Verify both elements in output | | Read-only enforced | No files modified during review | Accidental edit or write | Confirm no write/edit tools were invoked |