# Review Code

> Code review workflow with five modes — SELF (self-review local branch before push), PR (review someone's pull request + post to GitHub), ARCHITECT (deep DDD architecture audit with scoring), TDD (red-green-refactor test-first), ADDRESS (fix PR review comments). Stack-aware. Use when user says "review changes", "review branch", "check branch", "review my code", "review before pr", "review pr", "ch…

- **Type:** Skill
- **Install:** `agentstack add skill-phuthuycoding-moicle-code`
- **Verified:** Yes — security-reviewed for prompt injection and unsafe behavior
- **Seller:** [phuthuycoding](https://agentstack.voostack.com/s/phuthuycoding)
- **Installs:** 0
- **Category:** [Developer Tools](https://agentstack.voostack.com/c/developer-tools)
- **Latest version:** 0.1.0
- **License:** MIT
- **Upstream author:** [phuthuycoding](https://github.com/phuthuycoding)
- **Source:** https://github.com/phuthuycoding/moicle/tree/master/assets/skills/review/code

## Install

```sh
agentstack add skill-phuthuycoding-moicle-code
```

Requires the [AgentStack CLI](https://agentstack.voostack.com/docs/cli). Works with Claude Code, Cursor, and any MCP-compatible agent.

## About

# Code Review Workflow

One skill covering the full review surface. Pick the mode that matches your situation.

## Pick your mode

| Situation | Mode | Jump to |
|-----------|------|---------|
| Self-review your branch before pushing / opening a PR | **SELF** | [Mode SELF](#mode-self) |
| Reviewing someone else's PR + posting feedback to GitHub | **PR** | [Mode PR](#mode-pr) |
| Deep DDD architecture audit of a domain (with scoring) | **ARCHITECT** | [Mode ARCHITECT](#mode-architect) |
| Writing code test-first (red → green → refactor) | **TDD** | [Mode TDD](#mode-tdd) |
| Addressing review comments on your own PR | **ADDRESS** | [Mode ADDRESS](#mode-address) |

- ❌ Security-only sweep → use `@security-audit` agent
- ❌ Bug surfaced by review → use `/fix-bug`

## Read Architecture First (all modes)

Detect stack via `~/.claude/architecture/_shared/stack-detection.md`. Load `ddd-architecture.md` + the stack doc — extract forbidden imports + conventions before reviewing. Severity definitions: `~/.claude/architecture/_shared/severity-levels.md` (code severity table).

---
---

# Mode SELF

Self-review your branch vs a base branch before pushing or opening a PR. Checks architecture compliance, stack conventions, and code quality — on **changed files only**, not the whole codebase.

**ARGUMENTS:** (optional) base branch. Default: `main` (fallback to `master`).

## Workflow

```
0 DETECT → 1 COLLECT → 2 BUILD+LINT → 3 ARCH → 4 CONVENTIONS → 5 QUALITY → 6 TESTS → 7 REPORT → 8 FIX
```

## Phase 0: DETECT
- [ ] Stack detected (ask user if ambiguous, e.g., monorepo)
- [ ] Architecture doc loaded
- [ ] Forbidden-imports list extracted

## Phase 1: COLLECT

```bash
BASE=${1:-main}
git rev-parse --verify "$BASE" >/dev/null 2>&1 || BASE=master

git log "$BASE"..HEAD --oneline
git diff "$BASE"...HEAD --stat
git diff "$BASE"...HEAD --name-only --diff-filter=ACMR
```

Categorize changed files by layer:

| Layer | Typical paths |
|-------|---------------|
| Domain | `domain/`, `internal/domain/`, `src/domain/`, `lib/domain/` |
| Application | `application/`, `internal/application/`, `src/application/` |
| Infrastructure | `infrastructure/`, `internal/infrastructure/`, `src/infrastructure/` |
| Presentation / UI | `controllers/`, `pages/`, `components/`, `views/`, `ports/http/` |
| Persistence | `models/`, `entities/` (ORM), `prisma/`, `migrations/` |
| Config / Bootstrap | `config/`, `bootstrap/`, `cmd/`, `main.*` |

Read **all** changed files before reviewing — never skim.

## Phase 2: BUILD + LINT

Run the stack's build + typecheck + lint commands. If any fail → mark **CRITICAL** and stop further review until they pass.

```bash
# Go:       go build ./... && go vet ./...
# NestJS:   pnpm typecheck && pnpm lint
# Laravel:  composer dump-autoload && ./vendor/bin/phpstan analyse
# Flutter:  dart analyze
# React/Remix: pnpm typecheck && pnpm lint
```

## Phase 3: ARCHITECTURE (changed files only)

### 3.1 Domain (if changed)
| # | Rule |
|---|------|
| D1 | Domain purity — no forbidden imports (ORM, HTTP, cache, queue, auth SDK) |
| D2 | No cross-domain imports (only shared kernel allowed) |
| D3 | No persistence-model imports in domain |
| D4 | Entities have behavior (not anemic data bags) |
| D5 | Entities raise events on state change (if architecture uses events) |
| D6 | Ports in `ports/` dir (not inline in usecases) |
| D7 | One port per file |
| D8 | Ports return domain types, not primitives |
| D9 | Value objects stdlib-only |
| D10 | Usecases have no infra imports |

```bash
CHANGED_DOMAIN=$(git diff "$BASE"...HEAD --name-only --diff-filter=ACMR \
  | grep -E '^(src|internal|lib)/domain/')
[ -n "$CHANGED_DOMAIN" ] && echo "$CHANGED_DOMAIN" \
  | xargs grep -lEn '' 2>/dev/null \
  && echo FAIL || echo PASS
```

### 3.2 Application (if changed)
| # | Rule |
|---|------|
| A1 | Handler is thin (parse → service → respond, no business logic) |
| A2 | Service justified only when ≥2 usecases orchestrated |
| A3 | Listener is side-effect only (no business logic) |
| A4 | Listener registered in event bus |
| A5 | Event name string matches registry |
| A6 | DTOs validated at boundary |
| A7 | Composition root only — no inline wiring in handlers |

### 3.3 Infrastructure (if changed)
| # | Rule |
|---|------|
| I1 | Repository has no business logic |
| I2 | Mappers exist (domain ↔ ORM model) |
| I3 | Implements port interface (returns domain types) |
| I4 | Context / transaction propagation correct |

### 3.4 Persistence models (if changed)
| # | Rule |
|---|------|
| M1 | ORM models in infrastructure, NOT domain |
| M2 | Schema change → matching migration |
| M3 | Nullable columns use nullable types |

## Phase 4: CONVENTIONS (cross-stack)

| # | Rule |
|---|------|
| G1 | No swallowed errors (no empty catch / `if err != nil {}`) |
| G2 | Async work uses background context, NOT request context |
| G3 | API-facing types have serialization tags (`json:`, decorators, etc.) |
| G4 | No hardcoded secrets / tokens / keys |
| G5 | Parameterized queries only — no string-interpolated SQL |
| G6 | Input validation at boundary before reaching domain |

Plus any stack-specific Hard Rules from the architecture doc.

## Phase 5: QUALITY (manual)

Read the diff. Look for:

| # | Area | What to look for |
|---|------|------------------|
| Q1 | Logic correctness | Off-by-one, nil deref, wrong condition, missed edge case |
| Q2 | Error handling | Errors propagated/wrapped, not silently ignored |
| Q3 | Concurrency | Race conditions, shared mutable state, async leaks |
| Q4 | Resource leaks | Unclosed connections, HTTP bodies, file handles |
| Q5 | Naming | Reveals intent (no `data`, `info`, `manager`, `helper`) |
| Q6 | Dead code | Unreachable, unused, commented-out |
| Q7 | Duplication | Real duplication across changed files (not coincidental) |
| Q8 | Breaking change | API contract change, removed field, behavior change |
| Q9 | Over-engineering | Abstraction not justified by the change |
| Q10 | Test coverage | New logic has tests; bug fixes have regression tests |

## Phase 6: TESTS

```bash
CHANGED_DOMAINS=$(git diff "$BASE"...HEAD --name-only --diff-filter=ACMR \
  | grep -E '/(domain|modules|features)/' \
  | sed -E 's|.*(domain\|modules\|features)/([^/]+)/.*|\2|' | sort -u)

for d in $CHANGED_DOMAINS; do
  # Go: go test ./internal/domain/$d/...  · NestJS: npx jest src/domain/$d
  # Laravel: ./vendor/bin/phpunit --filter $d  · Flutter: flutter test test/domain/$d
  echo "Test $d"
done

{full_test_command}
```

## Phase 7: REPORT

```markdown
## Code Review: {branch} → {base}
**Stack:** {stack} · **Commits:** {N} · **Files:** {N} (+{add} / -{del})

### Build / Lint / Types
| Check | Status |  → Build / Lint / Types: PASS/FAIL

### Issues (sorted by severity)
| # | Severity | File:line | Issue | Suggested fix |
|---|----------|-----------|-------|---------------|
| 1 | CRITICAL | config/db.ts:42 | hardcoded token | move to env |
| 2 | HIGH | handlers/user.ts:88 | business logic in handler | extract to usecase |

### Verdict: {APPROVED / CHANGES REQUESTED}
```

### Verdict rules
- **CRITICAL or HIGH found** → CHANGES REQUESTED
- **MEDIUM only** → CHANGES REQUESTED (should fix)
- **LOW only or nothing** → APPROVED (with suggestions if any)

## Phase 8: FIX (if user confirms)
1. Fix in order: CRITICAL → HIGH → MEDIUM → LOW
2. Re-run build + lint + tests after each batch
3. Re-run full review when all fixed
4. Report final status

---
---

# Mode PR

Review someone else's PR across 5 dimensions and post structured feedback to GitHub.

## When to use
- ✅ Reviewing someone else's open PR (`gh pr view ` accessible)
- ✅ Need to post structured feedback (APPROVE / REQUEST CHANGES / COMMENT)
- ❌ Self-review of own branch before push → **Mode SELF**
- ❌ Addressing comments on your own PR → **Mode ADDRESS**

## Workflow

```
FETCH → ANALYZE → REVIEW (5 dims) → FEEDBACK → POST
```

## Phase 1: FETCH

```bash
PR={number}
gh pr view $PR --json number,title,body,author,state,headRefName,baseRefName,commits,files
gh pr diff $PR
gh pr checks $PR
gh pr view $PR --comments
```

### Gate
- [ ] PR metadata + diff + CI status loaded
- [ ] PR description explains "what / why" — if missing, ask author before deep review
- [ ] CI status known (don't review red builds — ask author to fix first)

## Phase 2: ANALYZE
1. **Scope check**: does the diff match the PR title / description? Mixed-scope PRs → ask author to split.
2. **Risk profile**: domain logic, infra config, migrations, auth → high risk; UI tweak → low risk.
3. **Test delta**: are new tests in the diff? coverage % up or down?
4. **Reference module**: open 1 existing similar module to compare conventions.

### Gate
- [ ] PR scope is single-purpose (or split requested)
- [ ] Risk level identified · Test delta noted

## Phase 3: REVIEW — 5 dimensions

### 3.1 Architecture (CRITICAL / HIGH)
- Domain has zero framework imports; no cross-domain imports
- Business logic in usecases, not handlers / stores
- Ports in `ports/` dir; entities raise events on state changes
- Listeners use background context, not request context
- **For deep DDD audit:** switch to **Mode ARCHITECT** and link result.

### 3.2 Security (CRITICAL / HIGH)
- Input validation at trust boundary (handler / DTO)
- AuthN + AuthZ checked before sensitive ops
- No secrets / tokens in code or logs
- SQL via parameterized queries / ORM — no string concatenation
- File paths / shell commands sanitized
- Rate limiting + idempotency on state-mutating endpoints
- Crypto: standard library, no roll-your-own

### 3.3 Performance (HIGH / MEDIUM)
- No N+1 queries (eager-load relations the handler uses)
- Indexes match new query patterns (check `EXPLAIN`)
- Pagination on list endpoints (cursor preferred)
- Background work for slow operations (don't block request)
- Caching where appropriate, invalidation thought through
- Synchronous external API calls have timeouts

### 3.4 Testing (HIGH / MEDIUM)
- New business logic has unit tests (usecases, entities, value objects)
- New endpoints have at least 1 integration test (happy + error)
- Tests assert on behavior, not implementation
- Tests don't depend on order, time, or env
- Coverage didn't drop for touched files

### 3.5 Code quality (MEDIUM / LOW)
- Names reveal intent (no `data`, `info`, `manager`, `helper`)
- Functions do one thing; no dead branches or commented-out code
- DRY only where the duplication is real (not coincidental)
- Errors handled at boundary, not swallowed mid-flow

## Phase 4: FEEDBACK

| Decision | When |
|----------|------|
| **APPROVE** | 0 CRITICAL / HIGH; LOW only |
| **REQUEST CHANGES** | Any CRITICAL or HIGH; or multiple MEDIUM in same area |
| **COMMENT** | Only style / nit comments; or asking questions before final review |

### Inline comment format
Be specific. File + line + rationale + suggested fix:

```
file: internal/wallet/usecases/withdraw.go:42
severity: HIGH
issue: Business logic in handler — `if wallet.Balance  [domain]` — e.g. `go-backend wallet`, `react-frontend`. No arg → auto-detect stack. Aliases: `ddd → ddd-architecture`, `go → go-backend`, `react → react-frontend`, `flutter → flutter-mobile`, `laravel → laravel-backend`, `remix → remix-fullstack`, `nestjs → nodejs-nestjs`, `mono → monorepo`.

## Workflow

```
RESOLVE → LOAD RULES → AUTOMATED CHECKS → MANUAL REVIEW → REPORT → FIX LOOP
```

## Phase 0: RESOLVE ARCHITECTURE
- **Arg provided:** normalize via alias table → search project (`.claude/architecture/{name}.md`) then global (`~/.claude/architecture/{name}.md`). Not found → reject with available list, STOP.
- **No arg:** detect stack → confirm with user. Not detected → list options, ask.

### Gate
- [ ] Architecture file loaded · Domain identified (if scoped)

## Phase 1: LOAD RULES

Read `ddd-architecture.md` (core) + the stack doc. Extract: DDD directory layout, layer import rules + forbidden imports, hard rules (HR1-HR15), stack-specific check scripts, wiring + test patterns.

## Phase 2: AUTOMATED CHECKS

```bash
echo "R1: Build"        ; {build}        && echo PASS || echo FAIL
echo "R2: Lint/Vet"     ; {lint}         && echo PASS || echo FAIL
echo "R3: Domain pure"  ; {grep_forbidden in domain/} && echo FAIL || echo PASS
echo "R4: No cross-dom" ; {grep_domain_A in domain_B} && echo FAIL || echo PASS
echo "R5: No cycles"    ; {cycle_check}  && echo FAIL || echo PASS
echo "R6: Tests exist"  ; {find_tests_in_domain} | wc -l
echo "R7: Tests pass"   ; {test}         && echo PASS || echo FAIL
echo "R8: Wiring reg"   ; {check_routes_registered}
echo "R9: Event names"  ; {check_event_consistency}
echo "R10: Async ctx"   ; {check_no_request_context_in_goroutines}
```

Record PASS/FAIL per check. Continue to Phase 3 either way — manual review catches what automated misses.

## Phase 3: MANUAL REVIEW

Focus on **architecture structure**, not business correctness. 10 areas:

- **D — Directory:** D1 `domain/{domain}/` proper subdirs · D2 has `entities/`,`ports/`,`usecases/` · D3 `valueobjects/` separate · D4 `events/` separate, 1/file · D5 app layer `ports/{transport}/`,`services/`,`listeners/` · D6 infra implements ports · D7 no legacy dirs
- **E — Entities:** E1 constructor · E2 behavior (not anemic) · E3 raises events · E4 no framework imports · E5 has mappers
- **VO — Value Objects:** VO1 in `valueobjects/` · VO2 stdlib only · VO3 immutable+behavior · VO4 used by entities+ports
- **P — Ports:** P1 `ports/` dir (no inline) · P2 one/file · P3 interface+DTOs · P4 domain types in sigs · P5 platform-agnostic naming · P6 no infra imports
- **EV — Events:** EV1 one/file · EV2 extends base · EV3 carries data · EV4 name matches registry
- **U — UseCases:** U1 uses ports · U2 split ≤200 lines · U3 business logic here · U4 no infra imports · U5 dispatches events after persistence · U6 no inline interfaces
- **SVC — Services:** SVC1 thin delegates · SVC2 no infra imports
- **H — Handlers:** H1 registration fn · H2 thin · H3 no business logic · H4 DTOs separate
- **L — Listeners:** L1 one/event · L2 side-effects only · L3 registered · L4 background context
- **I — Infrastructure:** I1 implements port · I2 mappers · I3 no business logic · I4 compile-time interface check

### Gate
- [ ] All 10 areas reviewed · Findings categorized by severity

## Phase 4: REPORT + SCORE

```markdown
## Architecture Review: {architecture} / {domain}
### Automated (R1-R10) | ### Manual review (D/E/VO/P/EV/U/SVC/H/L/I)
### Violations: [SEVERITY] code:file:line — description
### Overall Score: {A/B/C/D/F}
```

| Score | Criteria |
|-------|----------|
| **A** | 0 violations, all R1-R10 PASS |
| **B** | 0 CRITICAL/HIGH, max 3 MEDIUM |
| **C** | 0 CRITICAL, max 2 HIGH |
| **D** | Has CRITICAL or 3+ HIGH |
| **F** | Multiple CRITICAL — architecture broken |

## Phase 5: FIX LOOP (if user confirms)

```
LOOP: 1. Fix all violations → 2. Re-run automated (Phase 2) → 3. Re-run manual (Phase 3)
      4. IF violations ≥ MEDIUM → GOTO 1 · 5. IF only LOW/none → BREAK, final report
```

**Called from `/feature-build`:** skip Phase 0 (architecture known), skip user confirmation for fixes (auto-fix in loop), report final score back to caller.

---
---

# Mode TDD

Red-Green-Refactor cycle: write failing test → write minimal code to pass → refactor while green.

## When to use
- ✅ Logic with clear input → output behavior (parsers, validators, business rules, usecases)
- ✅ Fixing a bug — failing test first, then fix (regression guard)
- ✅ Refactoring critical code where you need a safety net
- ❌ UI prototyping / visual tweaks → manual is faster
- ❌ Exploratory spike → use `/research-explore` (SPIKE — throwaway, no tests)

## The TDD Cycle

```
   ┌──▶ RED (fail) ──▶ GREEN (minimal) ──▶ REFACTOR (cleanup) ──┐
   └────────────────────  next requirement  ────────────────────┘
```

## Phase 1: RED — failing test
1. Pick ONE small requirement (smallest verifiable behavior)
2. Name the test as a sentence describing behavior
3. Arrange (inputs + mocks) → Act (call method) → Assert (output / state / interact

…

## Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

- **Author:** [phuthuycoding](https://github.com/phuthuycoding)
- **Source:** [phuthuycoding/moicle](https://github.com/phuthuycoding/moicle)
- **License:** MIT

Install and usage instructions live in the source repository linked above.

## Pricing

- **Free** — Free

## Security capabilities

Automated source analysis of v0.1.0 — what this tool can access:

- **Network access:** no
- **Filesystem access:** yes
- **Shell / process execution:** no
- **Environment & secrets:** no
- **Dynamic code execution:** no

*"Yes" means the capability is present in the source — more access means more to trust, not that it is unsafe.*


## Versions

- **0.1.0** — security scan: passed — Imported from the upstream source.

## Links

- Listing page: https://agentstack.voostack.com/l/skill-phuthuycoding-moicle-code
- Seller: https://agentstack.voostack.com/s/phuthuycoding
- Browse the marketplace: https://agentstack.voostack.com/browse

---
Listed on AgentStack — the marketplace for AI agent skills and MCP servers. Every listing is security-reviewed. Creators keep 70%.