Architectural code review — coding standards, SOLID, testability, performance concerns.
60
75%
Does it follow best practices?
Run evals on this skill
Adds up to 20 points to the overall score
Passed
No findings from the security scan
Fix and improve this skill with Tessl
tessl review fix ./.claude/skills/code-review/SKILL.md!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).
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.
FOUND or ABSENT — not "assumed present".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.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.
Read the target file(s) in full. Read CLAUDE.md for project coding standards.
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:
<engine>-specialist derived from engine.name (Godot→godot-specialist, Unity→unity-specialist, Unreal→unreal-specialist); used for architecture and broad engine concernsspecialists.code — used when reviewing the project's primary language filesspecialists.shader — used when reviewing shader filesspecialists.ui — used when reviewing UI codeA 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.name` unset in `project.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.
Argument: /code-review [file(s)] may optionally include a story file path as the last argument (e.g., /code-review Assets/Scripts/Combat/Attack.cs 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:
git log --oneline -- [file])Look for patterns like ADR-NNNN or docs/architecture/adr- (either case).
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. Use the same pattern as /dev-story and /create-stories:
Grep pattern="^## " path="[adr-file]" output_mode="content" -n## 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.A referenced ADR whose file is missing, or that cannot be read, makes ADR
Compliance NOT ASSESSED — [ADR] could not be read: name it. No reference at
all is NO ADRS FOUND, a different result.
From those two sections, classify any deviation:
Identify the system category (engine, gameplay, AI, networking, UI, tools) and evaluate:
Architecture:
SOLID:
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) orUNVERIFIED — 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. 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.
If an engine is configured, determine which specialist applies to each file and spawn in parallel:
.gd, .cs, .cpp) → Language/Code Specialist.gdshader, .hlsl, shader graph) → Shader SpecialistAlso spawn the Primary Specialist for any file touching engine architecture (scene structure, node hierarchy, lifecycle hooks).
For Logic and Integration stories, also spawn qa-tester via Agent in parallel with the engine specialists. Pass:
## QA Test Cases section (the pre-written test specs from qa-lead)## Acceptance CriteriaAsk the qa-tester to evaluate:
## QA Test Cases section map to testable code paths?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.
## 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 — no story path given / 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; a missing doc comment names the method or class]
### 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]Choose the verdict, first match wins:
NOT ASSESSED (ADR
Compliance with a referenced ADR that could not be read, Architecture, SOLID);
name which. NOT ASSESSED ranks below CHANGES REQUIRED, because a known
defect is more actionable than a skipped check, and above both approvals: a
review that skipped the engine check has not approved the code. (NO ADRS FOUND is not NOT ASSESSED: with no ADR to check, it does not stop an
approval.)This skill is read-only — no files are written.
Use AskUserQuestion:
[A] Run /story-done to mark the story complete[B] Stop here[A] Apply the suggestions, then run /story-done[B] Run /story-done now — suggestions noted for later[C] Stop here[A] Fix the issues and re-run /code-review[B] Run /story-done anyway with noted exceptions[C] Stop here[A] Fix what stopped the review (point me at the code, run /setup-engine, or fix the unreadable ADR reference), then re-run /code-review[B] Stop hereIf an ARCHITECTURAL VIOLATION is found:
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./architecture-decision to document the correct approach before fixing the code.b21fa0f
If you maintain this skill, you can claim it as your own. Once claimed, you can manage eval scenarios, bundle related skills, attach documentation or rules, and ensure cross-agent compatibility.