--- name: Code Review slug: code-review-7 category: Quality description: Code Review performs architectural and quality code review on a specified file or set of files, checking coding standards, architecture patterns, SOLID principles, testability, and performance concerns in game development projects. Use it before merging code to catch violations and drift. github: "https://github.com/Donchitos/Claude-Code-Game-Studios/tree/main/.claude/skills/code-review" language: Shell stars: 23864 forks: 3411 install: "npx degit https://github.com/Donchitos/Claude-Code-Game-Studios/tree/main/.claude/skills/code-review ~/.claude/skills/code-review" installs_to: ~/.claude/skills/code-review source_path: .claude/skills/code-review/SKILL.md collection_size: 25 category_size: 1354 collection_url: "https://dirskills.com/collections/Donchitos/Claude-Code-Game-Studios" added: 2026-08-14T07:12:28.647Z last_synced: 2026-08-14T07:12:28.647Z canonical_url: "https://dirskills.com/skills/code-review-7" --- # Code Review Code Review performs architectural and quality code review on a specified file or set of files, checking coding standards, architecture patterns, SOLID principles, testability, and performance concerns in game development projects. Use it before merging code to catch violations and drift. **Install:** ```bash npx degit https://github.com/Donchitos/Claude-Code-Game-Studios/tree/main/.claude/skills/code-review ~/.claude/skills/code-review ``` ## README ## Phase 1: Load Target Files Read the target file(s) in full. Read CLAUDE.md for project coding standards. --- ## Phase 2: Identify Engine Specialists Read `.claude/docs/technical-preferences.md`, section `## Engine Specialists`. Note: - The **Primary** specialist (used for architecture and broad engine concerns) - The **Language/Code Specialist** (used when reviewing the project's primary language files) - The **Shader Specialist** (used when reviewing shader files) - The **UI Specialist** (used when reviewing UI code) If the section reads `[TO BE CONFIGURED]`, no engine is pinned — skip engine specialist steps. --- ## Phase 3: ADR Compliance Check **Argument:** `/code-review [file(s)]` may optionally include a story file path as the last argument (e.g., `/code-review src/combat/attack.gd production/epics/combat/story-001.md`). If a story path is provided, read it to extract the governing ADR reference. Search for ADR references in, in priority order: 1. The story file (if provided as argument) 2. Header comments at the top of the implementation files 3. Commit messages referencing these files (`git log --oneline -- [file]`) Look for patterns like `ADR-NNN` or `docs/architecture/ADR-`. If no ADR references found, note: "No ADR references found — ADR compliance check skipped. For full ADR compliance review, provide the story path: `/code-review [files] [story-path]`." For each referenced ADR: read the file, extract the **Decision** and **Consequences** sections, then classify any deviation: - **ARCHITECTURAL VIOLATION** (BLOCKING): Uses a pattern explicitly rejected in the ADR - **ADR DRIFT** (WARNING): Meaningfully diverges from the chosen approach without using a forbidden pattern - **MINOR DEVIATION** (INFO): Small difference from ADR guidance that doesn't affect overall architecture --- ## Phase 4: Standards Compliance Identify the system category (engine, gameplay, AI, networking, UI, tools) and evaluate: - [ ] Public methods and classes have doc comments - [ ] Cyclomatic complexity under 10 per method - [ ] No method exceeds 40 lines (excluding data declarations) - [ ] Dependencies are injected (no static singletons for game state) - [ ] Configuration values loaded from data files - [ ] Systems expose interfaces (not concrete class dependencies) --- ## Phase 5: Architecture and SOLID **Architecture:** - [ ] Correct dependency direction (engine <- gameplay, not reverse) - [ ] No circular dependencies between modules - [ ] Proper layer separation (UI does not own game state) - [ ] Events/signals used for cross-system communication - [ ] Consistent with established patterns in the codebase **SOLID:** - [ ] Single Responsibility: Each class has one reason to change - [ ] Open/Closed: Extendable without modification - [ ] Liskov Substitution: Subtypes substitutable for base types - [ ] Interface Segregation: No fat interfaces - [ ] Dependency Inversion: Depends on abstractions, not concretions --- ## Phase 6: Game-Specific Concerns - [ ] Frame-rate independence (delta time usage) - [ ] No allocations in hot paths (update loops) - [ ] Proper null/empty state handling - [ ] Thread safety where required - [ ] Resource cleanup (no leaks) --- ## Phase 7: Specialist Reviews (Parallel) Spawn all applicable specialists simultaneously via Task — do not wait for one before starting the next. ### Engine Specialists If an engine is configured, determine which specialist applies to each file and spawn in parallel: - Primary language files (`.gd`, `.cs`, `.cpp`) → Language/Code Specialist - Shader files (`.gdshader`, `.hlsl`, shader graph) → Shader Specialist - UI screen/widget code → UI Specialist - Cross-cutting or unclear → Primary Specialist Also spawn the **Primary Specialist** for any file touching engine architecture (scene structure, node hierarchy, lifecycle hooks). ### QA Testability Review For Logic and Integration stories, also spawn `qa-tester` via Task in parallel with the engine specialists. Pass: - The implementation files being reviewed - The story's `## QA Test Cases` section (the pre-written test specs from qa-lead) - The story's `## Acceptance Criteria` Ask the qa-tester to evaluate: - [ ] Are all test hooks and interfaces exposed (not hidden behind private/internal access)? - [ ] Do the QA test cases from the story's `## QA Test Cases` section map to testable code paths? - [ ] Are any acceptance criteria untestable as implemented (e.g., hardcoded values, no seam for injection)? - [ ] Does the implementation introduce any new edge cases not covered by the existing QA test cases? - [ ] Are there any observable side effects that should have a test but don't? For Visual/Feel and UI stories: qa-tester reviews whether the manual verification steps in `## QA Test Cases` are achievable with the implementation as written — e.g., "is the state the manual checker needs to reach actually reachable?" Collect all specialist findings before producing output. --- ## Phase 8: Output Review ``` ## Code Review: [File/System Name] ### Engine Specialist Findings: [N/A — no engine configured / CLEAN / ISSUES FOUND] [Findings from engine specialist(s), or "No engine configured." if skipped] ### Testability: [N/A — Visual/Feel or Config story / TESTABLE / GAPS / BLOCKING] [qa-tester findings: test hooks, coverage gaps, untestable paths, new edge cases] [If BLOCKING: implementation must expose [X] before tests in ## QA Test Cases can run] ### ADR Compliance: [NO ADRS FOUND / COMPLIANT / DRIFT / VIOLATION] [List each ADR checked, result, and any deviations with severity] ### Standards Compliance: [X/6 passing] [List failures with line references] ### Architecture: [CLEAN / MINOR ISSUES / VIOLATIONS FOUND] [List specific architectural concerns] ### SOLID: [COMPLIANT / ISSUES FOUND] [List specific violations] ### Game-Specific Concerns [List game development specific issues] ### Positive Observations [What is done well -- always include this section] ### Required Changes [Must-fix items before approval — ARCHITECTURAL VIOLATIONs always appear here] ### Suggestions [Nice-to-have improvements] ### Verdict: [APPROVED / APPROVED WITH SUGGESTIONS / CHANGES REQUIRED] ``` This skill is read-only — no files are written. --- ## Phase 9: Next Steps Use `AskUserQuestion`: - Prompt: "Code review complete — verdict: [APPROVED / CHANGES REQUIRED / MAJOR REVISION]. How would you like to proceed?" - Options (adjust based on verdict): - If APPROVED: - `[A] Run /story-done to mark the story complete` - `[B] Stop here` - If CHANGES REQUIRED or MAJOR REVISION: - `[A] Fix the issues and re-run /code-review` - `[B] Run /story-done anyway with noted exceptions` - `[C] Stop here` If an ARCHITECTURAL VIOLATION is found: - If the violation contradicts an **existing ADR**: fix the implementation to comply with `docs/architecture/[adr-file].md`. If the design has legitimately changed, run `/architecture-decision` to formally *revise* the existing ADR — do not create a competing one. - If **no ADR exists** for the pattern that was violated: run `/architecture-decision` to document the correct approach before fixing the code.