Om Code Review

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.

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

Silent Upstream Workarounds (Critical)

• Flag silent OM upstream workarounds as Critical. When a PR contains code that compensates for @open-mercato/* behavior — guards around core helpers returning unexpected shapes, casts that mask wrong types, repeated logic that should live upstream, comments like "core does X wrong" / "hack:" / "workaround:" / "FIXME core" — the PR MUST reference (a) an upstream issue at open-mercato/open-mercato, AND (b) a downstream tracking task with a removal-trigger marker, AND the workaround code MUST contain a marker comment of the form // remove when @open-mercato/# ships. If any of those three is missing, flag as Critical and require the author to run om-cto with references/upstream-bug-triage.md before merge. Reason: silent workarounds hide real bugs from the OM core team and create unbounded downstream tech debt that no future refactor can find. This applies regardless of workaround size — minor workarounds also need the paper trail.

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)

• Never hand-write migrations — update entities, run yarn db:generate
• 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

…

Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

• Author: SHGrowth
• Source: SHGrowth/om-superpowers
• License: MIT

Install and usage instructions live in the source repository linked above.