/noop:review - Architecture Review

Review code for compliance with the noop architectural framework. Identifies anti-patterns, suggests improvements, and ensures consistency.

Framework Context

Core Philosophy (MUST ENFORCE)

@docs/universal-framework/PHILOSOPHY.md

Coding Conventions (CHECK AGAINST)

@docs/universal-framework/CONVENTIONS.md

Quality Checklist

@docs/universal-framework/CHECKLIST.md

---

What This Reviews

• Handler patterns (orchestration only, no business logic)
• Service layer patterns (stateless, explicit dependencies)
• Database operations (organization scoping, parameterized queries)
• Error handling (no silent fallbacks, proper error factories)
• Type safety (no any, proper narrowing)
• Import conventions (ESM extensions, type imports)
• Naming conventions (files, variables, types)

Arguments

$ARGUMENTS - Optional file or directory to review. Defaults to src/.

Execution Steps

Step 1: Identify Files to Review

If $ARGUMENTS specifies a path, review that. Otherwise:

• Review all .ts files in src/
• Prioritize: handlers, services, database ops

Step 2: Check Critical Anti-Patterns

FLAG THESE IMMEDIATELY:

1. Silent Fallbacks (CRITICAL - NEVER ALLOWED)

// VIOLATION
catch { return defaultValue }

// MUST BE
catch { throw new Error(...) }
// OR
catch (error) { throw createError.xxx(...) }

2. Any Types (CRITICAL - NEVER ALLOWED)

// VIOLATION
function process(data: any) { }

// MUST BE
function process(data: ProcessInput) { }

3. Missing Organization Scoping (CRITICAL - SECURITY ISSUE)

// VIOLATION - no organizationId
async getById(id: string) { }

// MUST BE
async getById(id: string, organizationId: string) { }

4. Business Logic in Handlers (HIGH)

// VIOLATION - logic in handler
const enriched = data.map(x => transform(x))

// MUST BE - delegate to service
const result = await service.process(data, dbStore)

5. Multiple Field Fallbacks (HIGH - HIDES API ISSUES)

// VIOLATION
id = response.id || response.data.id || response.modelId

// MUST BE - expect consistent API
id = response.id

6. Missing ESM Extensions (MEDIUM)

// VIOLATION
import { x } from './utils'

// MUST BE
import { x } from './utils.js'

Step 3: Check Handler Patterns

For each handler file:

• Wrapped with asyncHandler
• Uses createError factories
• Uses sendSuccess / sendPaginatedResponse
• No business logic (only orchestration)
• Gets dbStore via getDbStore()

Step 4: Check Database Ops

For each Ops class:

• Every method requires organizationId
• Throws if organizationId missing
• Uses parameterized queries (no string interpolation in SQL)
• Has mapRowToX private method
• Returns domain types, not DB rows

Step 5: Check Services

For each service:

• Functions are stateless
• Dependencies passed as parameters
• Security checks at function start
• Returns typed data

Step 6: Generate Report

# Architecture Review Report

## Summary
- Files reviewed: X
- Violations found: Y
- Severity: [PASS | WARNINGS | CRITICAL]

## Critical Issues (MUST FIX)
[List severe violations]

## Warnings (SHOULD FIX)
[List moderate issues]

## Suggestions
[List minor improvements]

## Files Reviewed
- src/handlers/... ✓
- src/db/pg/... ⚠ (2 warnings)
- src/services/... ✗ (1 critical)

Step 7: Offer Fixes

For each violation, offer to fix it using the Edit tool.