code-review · @donchitos | Agent Skill Hub

!bash "${CLAUDE_SKILL_DIR}/../../hooks/yaml-helper.sh" resolve_config --keys automation

Every AskUserQuestion call follows .claude/docs/automation-modes.md (collaborative asks always · guided major-only · autonomous logs and proceeds; automation_always_ask categories always prompt).

Insufficient input — check this before producing any report

If the inputs this skill needs do not exist, the answer is "could not run" — not a filled-in report. Check first, and stop if the check fails.

List the inputs this skill reads (data files, prior reports, profiler output, test results, registries, source code).
For each, record FOUND or ABSENT — not "assumed present".
If any input required for a section is ABSENT, that section is NOT ASSESSED — NO DATA. Do not estimate it, do not infer it from an adjacent artifact, and do not leave a mandated cell to be filled by whoever reads the template next.
If every required input is ABSENT, stop and report NOT ASSESSED — NO DATA as the whole verdict, naming what was missing and which skill produces it.

A verdict of NOT ASSESSED is a success. It is the correct, useful answer to "what does the data say?" when there is no data. The failure mode this prevents is specific and has been observed in practice: report templates whose verdict enum had no "could not run" state produced false clean passes — an asset audit returning COMPLIANT on a project with no assets and no standards, and a performance profile reporting ">99% headroom against a 16.67ms budget" with zero profiler data and no budget ever set.

Absence of evidence is never evidence of absence. A scan that finds no matches because there are no files to scan has not verified anything. Say which of the two happened — a reader cannot tell from a green result.

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 the specialists block from project.yaml; if it is absent, fall back to the ## Engine Specialists section of .claude/docs/technical-preferences.md. Note:

The Primary specialist — <engine>-specialist derived from engine.name (Godot→godot-specialist, Unity→unity-specialist, Unreal→unreal-specialist); used for architecture and broad engine concerns
The Language/Code Specialist — specialists.code — used when reviewing the project's primary language files
The Shader Specialist — specialists.shader — used when reviewing shader files
The UI Specialist — specialists.ui — used when reviewing UI code

A value of null means UNSET — treat that key as absent and skip its specialist. Never spawn it as an agent name. The v1.0 migration writes null for any specialist the legacy file did not name, and it writes the whole block whenever one member is set — so a project that configured only its code specialist carries shader: null and ui: null. The config reader returns the four-character string "null" for these, which is not empty and therefore reads as configured. null, empty, and missing are the same state here.

If no engine is configured (no engine.name in project.yaml, and technical-preferences.md reads [TO BE CONFIGURED] or is missing), skip engine specialist steps. Record Engine validation: NOT ASSESSED — no engine configured (engine.nameunset inproject.yaml) in this run's output. A skipped check that says nothing is indistinguishable from a check that passed; the reader cannot tell engine guidance was never sought.

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:

The story file (if provided as argument)
Header comments at the top of the implementation files
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, load only the sections this check needs — never an unbounded full read. A substantial ADR exceeds the 25k-token Read cap, and a capped read's only recovery is paging the remainder — the most expensive way to read a file (measured ~103k vs ~54k tokens on a 34k-token ADR). Use the same pattern as /dev-story and /create-stories:

Map the headings (cheap — line numbers only): Grep pattern="^## " path="[adr-file]" output_mode="content" -n
Bounded-read only ## Decision and ## Consequences, using the line numbers to set Read(offset, limit) spans that end where the next heading begins. If the heading map is empty (a nonstandard ADR predating the template), fall back to one full Read; if that truncates at the cap, grep for the decision/consequence content directly rather than paging the remainder.

From those two sections, 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 Agent — do not wait for one before starting the next.

Verify every specialist finding before reporting it. Do not pass findings through unchecked. For each finding, record in the report:

File and line it refers to.

Evidence — the quoted code, or the concrete input/state that triggers it.

Confidence — VERIFIED (you checked it yourself) or UNVERIFIED — specialist claim (you could not).

A finding you could not verify is reported as unverified or dropped, never promoted to a defect on the strength of confident phrasing.

Why this is mandatory. Agents are reliable when deriving and unreliable when diagnosing existing code. Measured in practice: three separate agents produced three different wrong claims about the same six-line function, every one fluent enough to pass a skim — including a spawned specialist here alleging a float-precision bug that enumerating the inputs disproves. Without this step the parent review is a laundering channel: a guess enters as a specialist finding and leaves as a reviewed defect.

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 Agent 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: [NOT ASSESSED / 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: [NOT ASSESSED / CLEAN / MINOR ISSUES / VIOLATIONS FOUND]
[List specific architectural concerns]

### SOLID: [NOT ASSESSED / 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: [NOT ASSESSED / 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: [NOT ASSESSED / 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.