code-review

name	code-review
description	Review code changes for Open Mercato compliance with architecture, security, conventions, and quality rules. Covers module structure, naming, data security, UI patterns, event/cache/queue rules, and anti-patterns. Use for reviewing PRs, diffs, or commits.

Code Review

Review code changes against Open Mercato's architecture rules, security requirements, naming conventions, and quality standards. Produce actionable, categorized findings.

Review Workflow

1. Scope: Identify changed files. Classify each file by layer (API route, entity, validator, backend page, frontend page, subscriber, worker, command, search config, setup, ACL, events, DI, widget, test).
2. Gather context: Read relevant AGENTS.md for each touched module/package. Check .ai/specs/ for active specs on the module. Read .ai/lessons.md for known pitfalls.
3. CI/CD verification gate (MANDATORY): Run the same checks that CI runs, in order. Every gate MUST pass before the review can conclude. If any gate fails, fix the issue first — do NOT mark the review as passing. See CI/CD Verification Gate section below.
4. Template parity gate: Run yarn template:sync. If drift is reported, ask the user whether to sync now; if approved, run yarn template:sync:fix and include synced files in the change.
5. Backward compatibility gate: Check every change against BACKWARD_COMPATIBILITY.md (linked from root AGENTS.md). Flag any violation as Critical. See section below.
6. Run checklist: Apply all applicable rules from references/review-checklist.md. Flag violations with severity, file, line, and fix suggestion.
7. Test coverage: Verify changed behavior is covered by unit tests and/or integration tests. If coverage is missing, flag it with severity, file references, and exact test cases to add.
8. Cross-module impact: If the change touches events, extensions, or widgets, verify the consuming side handles the contract correctly.
9. Output: Produce the review report in the format below.

CI/CD Verification Gate (MANDATORY)

NEVER claim code is "ready to ship", "ready to merge", or "CI will pass" without running these checks first and confirming they all pass. This gate mirrors exactly what .github/workflows/ci.yml runs on every PR to develop/main. If any step fails, it MUST be fixed before the review can pass.

Gate Steps (run in order)

Run these commands and verify each one exits successfully (exit code 0):

#	Command	What it checks	If it fails
1	yarn build:packages	All packages compile	Fix TypeScript/build errors in the changed package
2	yarn generate	Module auto-discovery files are up to date	Run it — it generates missing files
3	yarn build:packages	Rebuild with generated files included	Fix any type errors exposed by generated files
4	yarn i18n:check-sync	All 4 locale files (en, de, es, pl) + template locales are in sync	Add missing i18n keys to all locale files
5	yarn i18n:check-usage	No unused or missing i18n keys	Remove unused keys or add missing ones (CI uses continue-on-error so non-blocking, but still report)
6	yarn typecheck	TypeScript types are correct across all 14+ packages	Fix type errors — do NOT dismiss as "pre-existing"
7	yarn test	All unit tests pass across all packages	Fix failing tests — do NOT dismiss as "flaky" or "pre-existing"
8	yarn build:app	The Next.js app builds successfully	Fix build errors

Rules

• Run steps 6 and 7 in parallel (they are independent) to save time.
• Every failure is a finding: If yarn typecheck or yarn test fails, it is a Critical finding in the review — even if the failure appears unrelated to the current changes. The PR will fail CI regardless of whose fault it is.
• No excuses: "Pre-existing on develop", "flaky test", "not our code" are not valid reasons to skip. If it fails on your branch, it will fail on CI. Fix it or flag it as a blocker.
• Evidence required: The review output MUST include the actual pass/fail result of each gate step. Do not assume — run the commands and report what happened.

Frontend Performance Blocking Gate

For PRs touching Next.js routes, generated frontend, shared providers, backend shell UI, or heavy interactive widgets, the reviewer has blocking power for performance regressions. Request changes when any of these are true:

• a Server Component was converted to a Client Component without an accepted Frontend Architecture Contract / "use client" ledger entry,
• page.tsx, layout.tsx, or a route shell became a large client-side blob instead of server root + small client islands,
• global providers/bootstrap import route-specific dashboards, injections, notifications, messages, payments, editors, calendars, graphs, or browser SDKs,
• bundle/runtime footprint grows without measurement, explanation, and explicit acceptance,
• changed interactions lack tests for hydration, accessibility, loading state, or error state,
• yarn check:client-boundaries output is missing for generated frontend/app-shell changes.

Add the client-boundary report and any bundle/RAM/per-route evidence to the review summary.

Output Format

Use this structure for every review:

# Code Review: {PR title or change description}

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

## CI/CD Verification

| Gate | Status | Notes |
|------|--------|-------|
| `yarn build:packages` | PASS/FAIL | |
| `yarn generate` | PASS/FAIL | |
| `yarn build:packages` (rebuild) | PASS/FAIL | |
| `yarn i18n:check-sync` | PASS/FAIL | |
| `yarn i18n:check-usage` | PASS/FAIL/WARN | (non-blocking in CI) |
| `yarn typecheck` | PASS/FAIL | |
| `yarn test` | PASS/FAIL | |
| `yarn build:app` | PASS/FAIL | |

## Findings

### Critical
{Violations that MUST be fixed before merge — security, data integrity, tenant isolation}

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

### Medium
{Style issues, missing best practices, suboptimal patterns}

### Low
{Suggestions, minor improvements, nits}

## Backward Compatibility
- [ ] No contract surface removed or renamed without deprecation bridge
- [ ] No event IDs renamed or removed
- [ ] No widget injection spot IDs renamed or removed
- [ ] No API route URLs renamed or removed
- [ ] No existing response schema fields removed
- [ ] No database columns/tables renamed or removed
- [ ] No DI service names renamed or removed
- [ ] No ACL feature IDs renamed or removed
- [ ] No public import paths removed without re-export bridge
- [ ] No required type fields removed or narrowed
- [ ] No function signatures changed in a breaking way
- [ ] Deprecation protocol followed (if applicable): `@deprecated` JSDoc, bridge re-export, spec with migration section

## Checklist
- [ ] No `any` types introduced
- [ ] All API routes export `openApi`
- [ ] Validators in `data/validators.ts` (not inline)
- [ ] Tenant isolation: queries filter by `organization_id`
- [ ] No hardcoded user-facing strings
- [ ] CRUD routes use `makeCrudRoute` with `indexer`
- [ ] Events declared in `events.ts` before emitting
- [ ] Workers/subscribers export `metadata`
- [ ] Custom fields use `collectCustomFieldValues()`
- [ ] `yarn generate` needed after file additions
- [ ] No cross-module ORM relationships
- [ ] Encryption helpers used instead of raw `em.find`/`em.findOne` — grep diff for `em.findOne(` and `em.find(` in non-test files; every hit is a blocker
- [ ] Forms use `CrudForm`, tables use `DataTable`
- [ ] Non-`CrudForm` backend writes use `useGuardedMutation(...).runMutation(...)` with `retryLastMutation` in context
- [ ] `apiCall` used instead of raw `fetch`
- [ ] ACL features mirrored in `setup.ts` `defaultRoleFeatures`
- [ ] ACL features use object format `{ id, title, module }` (not string arrays)
- [ ] `yarn template:sync` passes for `apps/mercato/src/{app,modules}` vs `packages/create-app/template/src/{app,modules}`
- [ ] Behavior changes covered by unit and/or integration tests (or explicitly justified as not applicable)
- [ ] No empty `catch` blocks (all catches must handle, log, rethrow, or explicitly document intentional ignore)
- [ ] New migrations are scoped to intended entities only (no unrelated bulk drop/alter/create statements)
- [ ] New or renamed spec files use `{YYYY-MM-DD}-{slug}.md` in `.ai/specs` or `.ai/specs/enterprise`
- [ ] No two spec files collapse to the same normalized `{YYYY-MM-DD}-{slug}.md` target when legacy `SPEC-*` / `SPEC-ENT-*` prefixes are removed

Omit empty severity sections. Mark passing checklist items with [x] and failing with [ ] plus explanation.

Severity Classification

Severity	Criteria	Action
Critical	Security vulnerability, cross-tenant data leak, data corruption risk, missing auth guard, backward compatibility violation (breaking contract surface without deprecation bridge)	MUST fix before merge
High	Architecture violation, missing required export (openApi, metadata), broken module contract, missing deprecation annotation on contract change	MUST fix before merge
Medium	Convention violation, suboptimal pattern, missing best practice	Should fix
Low	Style suggestion, minor improvement, readability	Nice to have

Quick Rule Reference

These are the highest-impact rules. For the full checklist, see references/review-checklist.md.

Backward Compatibility (Critical)

• MUST NOT remove/rename any contract surface (event IDs, spot IDs, API routes, type fields, function signatures, DI names, feature IDs, import paths, DB columns, convention file exports) — see BACKWARD_COMPATIBILITY.md for the full list
• Deprecate first: @deprecated JSDoc → bridge re-export/alias → removal after one minor version
• Additive-only DB changes: new columns with defaults OK; rename/remove/narrow columns is BREAKING
• Event payloads: may add optional fields; MUST NOT remove existing fields
• Widget spot context: may add optional fields; MUST NOT remove or change type of existing fields
• API responses: may add fields; MUST NOT remove existing fields
• Any PR touching a contract surface MUST reference a spec with a "Migration & Backward Compatibility" section

Architecture (Critical/High)

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

Security (Critical)

• Validate all inputs with zod in data/validators.ts — never trust raw input
• Use findOneWithDecryption/findWithDecryption instead of raw em.findOne/em.find for ALL tenant-scoped entity queries in production code (not tests). This is a HARD BLOCKER — grep the diff for em.findOne( and em.find( in non-test files. Import from @open-mercato/shared/lib/encryption/find or @open-mercato/shared/lib/data/encryption.
• Hash passwords with bcryptjs (cost >= 10) — never log credentials
• Auth endpoints: return minimal error messages — never reveal if email exists
• Every endpoint MUST declare guards (requireAuth, requireRoles, requireFeatures)
• Sensitive fields: MUST define fieldPolicy.excluded in search config
• MUST NOT cache passwords, tokens, PII without encryption

Data Integrity (Critical/High)

• Migration files and snapshots must match entity intent — prefer yarn db:generate, but scoped manual SQL is allowed when generation emits unrelated churn; in that case the module's .snapshot-open-mercato.json MUST be updated too.
• Autogenerated migration sanity is mandatory — generated files can be wrong; reviewers MUST validate migration diff scope
• Use withAtomicFlush when mutating entities across phases that include queries
• Flush scalar changes BEFORE relation syncs — avoid __originalEntityData reset
• Workers/subscribers MUST be idempotent — they may be retried
• Commands MUST be undoable — include before/after snapshots

Migration Sanity Gate (Critical)

For every migration in the diff, reviewer MUST:

1. Compare migration statements against the PR intent/spec and touched entities.
2. Flag as Critical if migration includes unrelated schema churn (especially mass drop constraint, drop table, or broad alter table across many modules).
3. Require regeneration/removal when scope is incorrect, even if file is autogenerated.
4. Block merge until migration contains only expected schema changes.

Examples of suspicious patterns that MUST be flagged:

• Migration touches many tables outside module scope for a focused feature PR.
• Migration mostly contains destructive statements (drop, bulk constraint removals) without matching entity changes.
• Snapshot/migration files appear due to local drift and are not required for feature behavior.

Naming & Structure (High/Medium)

• Modules: plural, snake_case (folders and id)
• JS/TS identifiers: camelCase
• Database tables/columns: snake_case, table names plural
• Common columns: id, created_at, updated_at, deleted_at, is_active, organization_id, tenant_id
• UUID PKs, explicit FKs, junction tables for many-to-many
• Code MUST NOT be added directly in apps/mercato/src/ — use apps/mercato/src/modules/
• Shared package (@open-mercato/shared) has zero domain dependencies — MUST NOT import from @open-mercato/core

Required Exports (High)

File	Required Export	Rule
API routes	openApi	MUST export for doc generation
API routes	metadata	MUST declare auth guards
Subscribers	metadata with { event, persistent?, id? }	MUST for auto-discovery
Workers	metadata with { queue, id?, concurrency? }	MUST for auto-discovery
events.ts	eventsConfig via createModuleEvents() with as const	MUST for type-safe events
acl.ts	features	MUST use object entries { id, title, module } and mirror IDs in setup.ts defaultRoleFeatures
search.ts	searchConfig with checksumSource in every buildSource	MUST for change detection

UI & HTTP (Medium/High)

• Forms: CrudForm — never custom form implementations
• If a backend page cannot use CrudForm, every write (POST/PUT/PATCH/DELETE) MUST go through useGuardedMutation(...).runMutation(...)
• useGuardedMutation context MUST include retryLastMutation so conflict resolution can auto-retry without extra save prompts
• Tables: DataTable — never manual table markup
• Notifications: flash() — never alert() or custom toast
• API calls: apiCall/apiCallOrThrow — never raw fetch
• JSON reading: readJsonSafe(response, fallback) — never .json().catch()
• CRUD errors: createCrudFormError(message, fieldErrors?) — never raw throw
• Dialogs: MUST support Cmd/Ctrl+Enter (submit), Escape (cancel)
• RowActions items MUST have stable id values (edit, open, delete)
• pageSize MUST be <= 100
• i18n: useT() client-side, resolveTranslations() server-side — never hardcode strings

Code Quality (Medium)

• No any types — use zod + z.infer, narrow with runtime checks
• No empty catch blocks — catch blocks MUST handle, log, rethrow, or include explicit rationale for intentional ignore
• No one-letter variable names
• No inline comments — code should be self-documenting
• Boolean parsing: use parseBooleanToken/parseBooleanWithDefault from @open-mercato/shared/lib/boolean
• Prefer functional, data-first utilities over classes
• Don't add docstrings/comments/annotations to code you didn't change

Testing Coverage (High/Medium)

• Behavioral changes MUST include test coverage through unit tests, integration tests, or both
• Risk-heavy paths MUST include integration coverage (permissions, tenant isolation, workflows, billing, undo/redo, events)
• Missing tests are findings: report exact files/areas lacking coverage and list the tests to add
• If tests are intentionally skipped, reviewer MUST verify a documented rationale and residual risk

Review Heuristics

When reviewing, pay special attention to:

0. Backward compatibility: For EVERY changed file, ask: "Does this touch a contract surface?" Check against BACKWARD_COMPATIBILITY.md. If a type field is removed, a function signature changed, an event ID renamed, a spot ID removed, a DB column dropped, an import path moved, or a DI name changed — flag as Critical unless the deprecation protocol is followed (bridge + @deprecated + spec).
1. New files added: Check if yarn generate is needed. Verify auto-discovery paths are correct.
2. Entity changes: Check if migration and snapshot updates are needed. Look for missing tenant scoping columns. 2a. Migration presence for entity changes: If any entity schema file changed (for example data/entities.ts), verify the diff also contains a corresponding migration file or a documented no-op explanation, plus the module's .snapshot-open-mercato.json when schema changed. 2b. Migration files: Inspect SQL content, not only filename. Autogenerated does not mean valid; reject oversized/unrelated churn. 2c. Snapshot files: Verify .snapshot-open-mercato.json represents the post-migration schema. Missing snapshot updates are blockers because they make future yarn db:generate recreate already-committed SQL.
3. New API routes: Verify openApi export, auth guards, zod validation, tenant filtering.
4. Event emitters: Verify event is declared in events.ts with as const. Check subscriber exists.
5. Search config changes: Verify checksumSource, fieldPolicy.excluded for sensitive fields, formatResult for token strategy.
6. Cache usage: Verify DI resolution, tenant scoping, tag-based invalidation on writes.
7. Queue workers: Verify idempotency, metadata export, concurrency <= 20.
8. Commands: Verify undoable, before/after snapshots, withAtomicFlush for multi-phase mutations.
9. Setup changes: Verify defaultRoleFeatures matches acl.ts features. Hooks MUST be idempotent. 9a. ACL shape validation: Verify acl.ts exports feature objects (with id, title, module) rather than raw string IDs; flag wrong shape as High because permission UI and role assignment can break.
10. UI changes: Verify CrudForm/DataTable usage, flash() for feedback, keyboard shortcuts, loading/error states.
11. Behavior changes: Verify unit and/or integration tests cover new behavior, regressions, and edge cases.
12. Mutation guard coverage: For backend pages with manual save/delete logic (non-CrudForm), verify useGuardedMutation is wired for all writes and context includes retryLastMutation; for custom write API routes, verify validateCrudMutationGuard + runCrudMutationGuardAfterSuccess are both wired.
13. Spec filename hygiene: If specs were added or renamed, verify new files use {YYYY-MM-DD}-{slug}.md, legacy numbered files are not copied forward into new work, and no two files would resolve to the same normalized date+slug target.
14. Template sync prompt: If yarn template:sync finds drift in src/app or src/modules (especially layout/routes), ask the user whether to sync; when approved, run yarn template:sync:fix and include those updates.

Lessons Learned

Check these known pitfalls from .ai/lessons.md:

1. Stale snapshots in buildLog(): Always load via forked EntityManager or refresh: true
2. Lost updates on flush: Flush scalar changes BEFORE relation syncs that query on same EntityManager
3. Undo payload duplication: Use centralized extractUndoPayload() from @open-mercato/shared/lib/commands/undo.ts