# Code Review

> Review code changes for architecture, security, conventions, and quality compliance. Use when reviewing pull requests, code changes, or auditing code quality.

- Skill: `open-mercato/code-review` (Agent Skill, multi-file: 2 files)
- Install (CLI): `npx skillmds@latest add open-mercato/code-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/open-mercato/code-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Security
- Author: open-mercato (https://skillmd.com/u/open-mercato)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/open-mercato/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.

| # | Command | What it checks | If it fails |
|---|---------|----------------|-------------|
| 1 | `yarn generate` | Module registries are up to date | Run it — it generates missing files |
| 2 | `yarn typecheck` | TypeScript types are correct | Fix type errors |
| 3 | `yarn test` | All unit tests pass | Fix failing tests |
| 4 | `yarn build` | The app builds successfully | Fix build errors |

**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

```markdown
# 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

| Severity | Criteria | Action |
|----------|----------|--------|
| **Critical** | Security vulnerability, cross-tenant leak, data corruption, missing auth | MUST fix before merge |
| **High** | Architecture violation, missing required export, broken module contract | MUST fix before merge |
| **Medium** | Convention violation, suboptimal pattern, missing best practice | Should fix |
| **Low** | Style suggestion, minor improvement | Nice to have |

## 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](references/review-checklist.md)
- [AGENTS.md](../../../AGENTS.md)