ags-code-review

name	ags-code-review
description	Performs an architectural and quality code review on a specified file or set of files. Checks for coding standard compliance, architectural pattern adherence, SOLID principles, testability, and performance concerns.
argument-hint	[path-to-file-or-directory]
user-invocable	true
allowed-tools	Read, Glob, Grep, Bash, Task, AskUserQuestion
agent	lead-programmer

Language: Talk to user in language from .ags/project/user-interaction.md. Fall back to English if file missing. Files on disk always English per .ags/rules/user-interaction.md.

Phase 0: Prerequisites

Artifact	Created by	If missing
Target file/directory argument	user	STOP. "No target. Usage: /ags-code-review path/to/file.cs."
Target file exists and is readable	—	STOP. "Path [X] not found."
CLAUDE.md (project standards)	template	STOP. "Project not initialized. Run /ags-start."
design/architecture/control-manifest.md (recommended)	/ags-create-control-manifest	WARN: review proceeds without rules sheet. Suggest /ags-create-control-manifest seed.

If STOP triggers, exit with verdict BLOCKED — missing prerequisite.

---

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 .ags/rules/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

Search for ADR references in the story file, commit messages, and header comments. Look for patterns like ADR-NNN or design/architecture/ADR-.

If no ADR references found, note: "No ADR references found — skipping ADR compliance check."

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

Design Principles (per .ags/rules/design-principles.md):

• §3 SoC — code stays inside designated layer (gameplay / UI / audio / data / save / AI); no logic leaked across boundaries
• §4 Loose Coupling / High Cohesion — cross-module access only through interface / event bus; single responsibility per type
• §5 SSoT — only one system owns each mutable runtime fact; no duplicated authoritative state
• §6 Fail Fast — content / config / data load validated; loud failure on missing-ref or out-of-range, no silent fallback
• §7 Observability — perf-sensitive or gameplay-critical code exposes logs / counters / debug hooks per coding.md §9
• §8 Backward Compatibility — save format / mod API / content schema changes carry migration; version field bumped if format changed

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 (.cs) → Language/Code Specialist
• Shader files (.shader, .hlsl, .shadergraph) → 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-lead 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-lead 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-lead 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-lead 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

• If verdict is APPROVED: run /ags-story-done [story-path] to close the story.
• If verdict is CHANGES REQUIRED: fix the issues and re-run /ags-code-review.
• If an ARCHITECTURAL VIOLATION is found: run /ags-architecture-decision to record the correct approach.

---

Combined Review Loop (parallel external Codex)

Per .ags/rules/review-workflow.md. The review phases run in parallel with external Codex inside one loop. Each iteration:

1. Resolve severity floor: iter 1-2 → keep all severities; iter 3-4 → critical/high; iter 5+ → critical only.
2. Spawn in one message, in parallel:
   • All internal reviewer Tasks (lead-programmer + relevant specialists).
   • /ags-external-review code [diff-path-or-branch] --embedded-parallel --iteration [N] --min-severity [floor] — Codex reviews the diff. Codex unavailable → skipped: codex-unavailable; aggregator logs skip in decisions-log and continues with internal pool only.
3. Aggregator (lead-programmer) merges findings from internal + Codex, drops nitpicks + below-floor.
4. Loop exit: filtered set empty → emit final verdict. Non-empty → surface aggregated kept findings, user resolves, N++, repeat.

No iteration cap. No user-confirm gate before external — it runs every iteration automatically. Record final iteration count in the verdict report and decisions-log entry. Codex reviews the source diff, NOT this review report.