Review Design

Review Design — Design Principles Audit

Purpose

Evaluate whether code changes follow the 24 structural design principles. This skill activates only the relevant principle groups based on what changed — not all 24 on every file.

This audit complements /review-ai-compat (which checks agent readability and structural coding rules). This skill checks design quality — whether the code is composed correctly, whether schemas are the source of truth, whether layers respect their boundaries, and whether the code is anchored to concrete examples.

Expert Panel

You are a review board composed of:

• Software Architect — File cohesion, module boundaries, dependency direction
• Schema & Type Expert — Zod as source of truth, type derivation, contract compliance
• Agent Reliability Specialist — Constraints as infrastructure, golden examples, concrete terms
• Safety & Verification Lead — Error handling, evidence gates, test coverage, pre-flight checks
• Frontend State Specialist — State lifecycle routing, component composition (conditional)
• Protocol Engineer — MCP validation, critical path ownership (conditional)

Each expert speaks separately. No repetition between experts.

Principle Groups

Instructions

Step 0 — Scope Detection + Group Selection

Detect the base branch (auto: develop for GitFlow, main/master for trunk-based, release if used as integration branch):

BASE=$(git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@')
[ -z "$BASE" ] && for b in develop main master release; do
  git show-ref --verify --quiet "refs/heads/$b" && BASE="$b" && break
done
[ -z "$BASE" ] && BASE=$(git rev-parse --abbrev-ref HEAD)
echo "base=$BASE"

Run: git diff $BASE...HEAD --name-only

Use git commands only — do NOT use gh CLI or GitHub API.

Classify changed files and activate principle groups:

# Get changed files
CHANGED=$(git diff $BASE...HEAD --name-only)

# Check for new files (Group D trigger)
NEW_FILES=$(git diff $BASE...HEAD --diff-filter=A --name-only)

Activation rules:

• Group A: activated if ANY .ts or .tsx source file changed (excluding test files)
• Group B: activated if files match **/services/**, **/routes/**, **/repositories/**, or **/data-access/**
• Group C: activated if files match **/contracts/**, **/schemas/**, **/types/**, or **/interfaces/**
• Group D: activated if new files were created OR *config*, eslint*, tsconfig* changed
• Group E: activated if */tests/*, *error*, *AppError*, or pipeline files changed
• +P22: activated if any .tsx or .jsx file changed
• +P18, P19: activated if MCP server code changed (any file matching **/mcp-*/** or files importing @modelcontextprotocol/*)

List which groups are active and which are skipped. Skip inactive groups entirely.

Phase 1 — File & Module Design (Group A)

Skip if Group A is not active.

For each changed source file, evaluate:

Gather evidence (prefer codegraph, fall back to grep):

For each changed file, use codegraph_search to find its symbols, then:

• codegraph_callers on exported symbols → reverse dependency count (P12)
• codegraph_callees on the file's main function → dependency depth (P13)
• codegraph_impact on changed symbols → blast radius of the change

# File lengths (flag > 200 lines)
for file in <changed-source-files>; do
  wc -l "$file"
done

# Fallback: Import count per file (flag > 8 imports)
for file in <changed-source-files>; do
  echo "$(grep -c '^import ' "$file") imports — $file"
done

# Fallback: Reverse dependency count (how many files import this one?)
for file in <changed-source-files>; do
  BASENAME=$(basename "$file" .ts)
  grep -rl "from '.*/$BASENAME'" --include="*.ts" --include="*.tsx" | wc -l
done

Phase 2 — Separation of Concerns (Group B)

Skip if Group B is not active.

For each changed service, route, or dbhandler file, evaluate:

Layer responsibility reference (from P21):

Gather evidence:

# Check for DB/external client imports outside data access layer
grep -rEn "from ['\"]( oracledb|pg|mysql2|mssql|mongodb|mongoose|@aws-sdk/client-dynamodb|dynamoose|redis|ioredis|cassandra-driver|@elastic/elasticsearch|prisma|typeorm|sequelize|knex|drizzle)['\"]" <changed-files> \
  | grep -vE "(repositories|data-access|dbhandlers)/"

# Check for business logic in route handlers (flag functions > 10 lines in routes)
grep -c "function\|=>" <changed-route-files>

Phase 3 — Schema & Type Integrity (Group C)

Skip if Group C is not active.

For each changed contract, schema, or type file, evaluate:

Gather evidence:

# Find hand-written interfaces that duplicate schema fields
grep -rn "^export interface\|^export type" <changed-files>

# Find z.infer usage (good — types derived from schemas)
grep -rn "z\.infer" <changed-files>

# Find duplicate field names across schema files
grep -rn "z\.string()\|z\.number()\|z\.boolean()" --include="*.ts" \
  $(find . -type d \( -name schemas -o -name contracts \) -not -path '*/node_modules/*') \
  2>/dev/null | sed 's/.*: //' | sort | uniq -d

Phase 4 — Agent Reliability (Group D)

Skip if Group D is not active.

For new files and config changes, evaluate:

Gather evidence:

# Check for removed eslint-disable or lint rule changes
git diff $BASE...HEAD -- '*.ts' '*.tsx' | grep -i "eslint-disable"
git diff $BASE...HEAD -- '*eslint*' '*config*'

# Check new shared utils have multiple callers
for file in <new-files-in-utils-or-helpers>; do
  BASENAME=$(basename "$file" .ts)
  echo "--- $BASENAME ---"
  grep -rl "$BASENAME" --include="*.ts" --include="*.tsx" | grep -v "$file"
done

# Compare new file structure against project's declared golden example
# (path declared in CLAUDE.md / README / examples/; substitute below)
GOLDEN_EXAMPLE_PATH="<path declared by project>"
ls -la "$GOLDEN_EXAMPLE_PATH"

Phase 5 — Safety & Verification (Group E)

Skip if Group E is not active.

For error handling, test, and pipeline files, evaluate:

Gather evidence:

# Find empty catch blocks
grep -rn "catch" <changed-files> -A2 | grep -B1 "}"

# Find silent error swallowing
grep -rn "catch.*{" <changed-files> -A3 | grep "console\.\|// \|/\*"

# Check test mirror structure — try common conventions
for file in <changed-source-files>; do
  BASE_NO_EXT="${file%.ts}"
  CANDIDATES=(
    "$(echo "$file" | sed 's|/src/|/tests/unit/|; s|\.ts$|.test.ts|')"
    "$(echo "$file" | sed 's|/src/|/tests/|; s|\.ts$|.test.ts|')"
    "$(echo "$file" | sed 's|/src/|/__tests__/|; s|\.ts$|.test.ts|')"
    "${BASE_NO_EXT}.test.ts"
    "${BASE_NO_EXT}.spec.ts"
  )
  FOUND=""
  for c in "${CANDIDATES[@]}"; do
    [ -f "$c" ] && FOUND="$c" && break
  done
  [ -n "$FOUND" ] && echo "test exists: $FOUND" || echo "MISSING test for: $file"
done

# Check for explicit AppError status codes
grep -rEn "new [A-Z][A-Za-z]*Error\(" <changed-files> | grep -v "StatusCodes\."

Phase 6 — Conditional Principles

P22: State belongs where its lifecycle lives — only if frontend .tsx changed.

For each changed frontend component:

• Is server state in a server-state lib (TanStack Query / SWR / RTK Query / Apollo) — not useState?
• Is form state in a form lib (react-hook-form / Formik / React Final Form) — not manual onChange?
• Is UI-only state in useState — not Context?
• Is cross-component state in Context or a global store (Zustand / Redux / Jotai / Recoil) — not prop drilling?

# Find useState usage that might be server state
grep -rn "useState.*data\|useState.*loading\|useState.*error" <changed-tsx-files>

# Find manual form state management
grep -rn "useState.*value\|onChange.*setState" <changed-tsx-files>

P18 + P19: MCP principles — only if MCP server code changed.

• P18: Does the tool validate at both protocol (JSON Schema) and runtime (Zod) level?
• P19: Are auth, errors, and versioning built-in or delegated to third parties?

# Auto-discover MCP server dirs, then check for dual validation
MCP_DIRS=$(find . -type d -name 'mcp-*' 2>/dev/null)
[ -z "$MCP_DIRS" ] && MCP_DIRS=$(grep -rl "@modelcontextprotocol/" --include="*.ts" --include="*.js" . 2>/dev/null | xargs -I{} dirname {} | sort -u)
[ -n "$MCP_DIRS" ] && grep -rn "zodSchema\|jsonSchema\|inputSchema" $MCP_DIRS

Phase 7 — Design Principles Scorecard

Principle Evaluation Table — one row per checked principle:

Only include principles from active groups. Skip principles from inactive groups.

Group Summary Table:

Design Health Score: (total pass / total checked) * 10

Scoring guide:

• 8-10 — Strong design alignment. No action needed.
• 5-7 — Acceptable with warnings. Review flagged principles.
• 1-4 — Design drift detected. Address FAIL items before merge.

This score measures design principle adherence — it is distinct from the AI Patch Safety Score used by /review-ai-compat (which measures agent modifiability dimensions).

Phase 8 — Improvements

If any principles scored WARN or FAIL, provide:

Quick Fixes (per-principle, low disruption):

• The specific principle violated
• The exact file and line
• What to change and why

Structural Improvements (if 3+ FAILs in one group):

• Pattern-level fix that addresses the root cause
• Reference to the golden example that demonstrates the correct pattern

Prioritize by: Principle severity x Number of violations x Fix effort

Contract

Append this JSON block to every audit output — it is the verifiable contract:

{
  "agent": "review-design",
  "branch": "<branch>",
  "date": "<today>",
  "verdict": "PASS|FLAG|DRIFT",
  "groups": {
    "fileModuleDesign": { "checked": 0, "pass": 0, "warn": 0, "fail": 0 },
    "separationOfConcerns": { "checked": 0, "pass": 0, "warn": 0, "fail": 0 },
    "schemaTypeIntegrity": { "checked": 0, "pass": 0, "warn": 0, "fail": 0 },
    "agentReliability": { "checked": 0, "pass": 0, "warn": 0, "fail": 0 },
    "safetyVerification": { "checked": 0, "pass": 0, "warn": 0, "fail": 0 },
    "conditional": { "checked": 0, "pass": 0, "warn": 0, "fail": 0 }
  },
  "designHealthScore": 0,
  "findings": ["specific principle violations"],
  "improvements": ["specific recommendations"]
}

Verdict rules:

• PASS — Design Health Score ≥ 8
• FLAG — Design Health Score 5–7
• DRIFT — Design Health Score < 5

Output Constraints

• No vague "follow the principles" advice.
• Every finding must reference a specific file, function, line, or pattern.
• Every WARN or FAIL must cite the diagnostic question that triggered it.
• Separate confirmed violations from assumptions.
• If insufficient information to conclude, state: "Insufficient information to conclude".
• This audit measures design principle adherence — not general code quality or agent readability.

Optional: Self-Correction (Manual)

After reviewing the output, you may paste the findings into a new prompt:

"Here are the findings from my design principles audit. Which of these might be incorrect due to missing context? What additional data would increase confidence?"

IMPORTANT: This step must be human-initiated — never auto-dismiss findings. The human decides what to act on.