| 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
- 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).
- 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.
- 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.
- 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.
- Backward compatibility gate: Check every change against
BACKWARD_COMPATIBILITY.md (linked from root AGENTS.md). Flag any violation as Critical. See section below.
- Run checklist: Apply all applicable rules from
references/review-checklist.md. Flag violations with severity, file, line, and fix suggestion.
- 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.
- Cross-module impact: If the change touches events, extensions, or widgets, verify the consuming side handles the contract correctly.
- 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:
- Compare migration statements against the PR intent/spec and touched entities.
- Flag as Critical if migration includes unrelated schema churn (especially mass
drop constraint, drop table, or broad alter table across many modules).
- Require regeneration/removal when scope is incorrect, even if file is autogenerated.
- 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:
- 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).
- New files added: Check if
yarn generate is needed. Verify auto-discovery paths are correct.
- 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.
- New API routes: Verify
openApi export, auth guards, zod validation, tenant filtering.
- Event emitters: Verify event is declared in
events.ts with as const. Check subscriber exists.
- Search config changes: Verify
checksumSource, fieldPolicy.excluded for sensitive fields, formatResult for token strategy.
- Cache usage: Verify DI resolution, tenant scoping, tag-based invalidation on writes.
- Queue workers: Verify idempotency,
metadata export, concurrency <= 20.
- Commands: Verify undoable, before/after snapshots,
withAtomicFlush for multi-phase mutations.
- 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.
- UI changes: Verify
CrudForm/DataTable usage, flash() for feedback, keyboard shortcuts, loading/error states.
- Behavior changes: Verify unit and/or integration tests cover new behavior, regressions, and edge cases.
- 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.
- 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.
- 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:
- Stale snapshots in
buildLog(): Always load via forked EntityManager or refresh: true
- Lost updates on flush: Flush scalar changes BEFORE relation syncs that query on same
EntityManager
- Undo payload duplication: Use centralized
extractUndoPayload() from @open-mercato/shared/lib/commands/undo.ts