code-review: Скилл для анализа и улучшения кода

--- name: code-review description: Review code changes for Open Mercato compliance with architecture, security, conventions, and quality rules. Use this skill when reviewing pull requests, reviewing code changes, performing code review, auditing code quality, or when asked to review files, diffs, or commits. Covers module structure, naming conventions, data security, UI patterns, event/cache/queue rules, and anti-patterns. --- # 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: ```markdown # 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 | 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 `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) | 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 `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`