Code Review

Code Review

Review code changes against Open Mercato architecture rules, security requirements, and quality standards.

Review Workflow

1. Scope: Identify changed files. Classify by layer (entity, API route, validator, backend page, subscriber, worker, command, widget).
2. Gather context: Read AGENTS.md for module conventions. Check .ai/specs/ for active specs. Read .ai/lessons.md for known pitfalls.
3. CI/CD verification gate (MANDATORY): Run the checks below. Every gate MUST pass. See CI/CD Gate section.
4. Run checklist: Apply rules from references/review-checklist.md. Flag violations with severity, file, and fix suggestion.
5. Test coverage: Verify changed behavior is covered by tests. Flag missing coverage.
6. Cross-module impact: If the change touches events, extensions, or widgets, verify consumers handle the contract correctly.
7. Output: Produce the review report.

CI/CD Verification Gate (MANDATORY)

NEVER claim code is "ready to merge" without running these checks. If any step fails, it MUST be fixed before the review can pass.

Rules:

• Steps 2 and 3 can run in parallel.
• Every failure is a Critical finding — even if it appears unrelated to the current changes.
• The review output MUST include actual pass/fail results. Do not assume — run and report.

Output Format

# Code Review: {change description}

## Summary
{1-3 sentences: what the change does, overall assessment}

## CI/CD Verification

| Gate | Status | Notes |
|------|--------|-------|
| `yarn generate` | PASS/FAIL | |
| `yarn typecheck` | PASS/FAIL | |
| `yarn test` | PASS/FAIL | |
| `yarn build` | PASS/FAIL | |

## Findings

### Critical
{Security, data integrity, tenant isolation violations}

### High
{Architecture violations, missing required exports}

### Medium
{Convention violations, suboptimal patterns}

### Low
{Suggestions, minor improvements}

## Checklist
{From references/review-checklist.md — mark [x] passing, [ ] failing with explanation}

Omit empty severity sections.

Severity Classification

Quick Rule Reference

Architecture

• NO direct ORM relationships between modules — use FK IDs, fetch separately
• Always filter by organization_id for tenant-scoped entities
• Use DI (Awilix) to inject services — never new directly
• NO direct module-to-module calls for side effects — use events
• Cross-module data: use extension entities + data/extensions.ts

Security

• Validate all inputs with zod in data/validators.ts
• Use findWithDecryption instead of raw em.find/em.findOne
• Hash passwords with bcryptjs (cost >= 10) — never log credentials
• Every endpoint MUST declare auth guards (requireAuth, requireRoles, requireFeatures)

Data Integrity

• Migration files and snapshots must match entity intent — prefer yarn db:generate; scoped manual SQL is allowed only to avoid unrelated generated churn and must include .snapshot-open-mercato.json
• Validate migration scope — autogenerated doesn't mean correct
• Workers/subscribers MUST be idempotent
• Commands MUST be undoable — include before/after snapshots

Naming

• Modules: plural, snake_case (folders and id)
• JS/TS identifiers: camelCase
• Database: snake_case, table names plural
• Standard columns: id, created_at, updated_at, deleted_at, is_active, organization_id

UI & HTTP

• Forms: CrudForm — never custom
• Tables: DataTable — never manual markup
• Notifications: flash() — never alert() or custom toast
• API calls: apiCall/apiCallOrThrow — never raw fetch
• Dialogs: Cmd/Ctrl+Enter (submit), Escape (cancel)
• pageSize MUST be <= 100

Code Quality

• No any types — use zod + z.infer
• No empty catch blocks
• No one-letter variable names
• Boolean parsing: use parseBooleanToken/parseBooleanWithDefault
• Don't add docstrings/comments to code you didn't change

Review Heuristics

1. New files: Check if yarn generate is needed. Verify auto-discovery paths.
2. Entity changes: Check if migration and snapshot updates are needed. Look for missing tenant columns.
3. Migration sanity: Inspect SQL content and .snapshot-open-mercato.json. Reject unrelated schema churn.
4. New API routes: Verify openApi export, auth guards, zod validation, tenant filtering.
5. Event emitters: Verify event is declared in events.ts with as const.
6. Commands: Verify undoable, before/after snapshots.
7. UI changes: Verify CrudForm/DataTable, flash(), keyboard shortcuts, loading/error states.
8. Test coverage: Verify unit/integration tests cover new behavior.

Reference Materials

• Review Checklist
• AGENTS.md