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):

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()`
- [ ] `modules:prepare` needed after file additions
- [ ] No cross-module ORM relationships
- [ ] Encryption helpers used instead of raw `em.find`
- [ ] 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

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 findWithDecryption instead of raw em.find/em.findOne
• 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)

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 modules:prepare is needed. Verify auto-discovery paths are correct.
2. Entity changes: Check if yarn db:generate is 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 generated migration file. If missing, raise at least a High finding instructing to run yarn db:generate. 2b. Migration files: Inspect SQL content, not only filename. Autogenerated does not mean valid; reject oversized/unrelated churn.
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

Converted and distributed by TomeVault — claim your Tome and manage your conversions.