124 lines
7.1 KiB
Markdown
124 lines
7.1 KiB
Markdown
# Deep Review — Phase 3: Семь измерений
|
||
|
||
> Supporting reference. Применяй к файлам P0/P1 минимум, к P2/P3 — выборочно.
|
||
|
||
## A. Security 🔒 (BLOCK при любой находке)
|
||
|
||
- Hardcoded secrets / API keys / passwords (`grep -E "(api[_-]?key|password|token|secret).*=.*['\"][a-zA-Z0-9]{16,}"` по diff)
|
||
- SQL injection: f-string / `.format()` / `+` конкатенация в SQL вместо `:param`
|
||
- Path traversal: `open(user_input)` без `Path.resolve()` + проверки префикса
|
||
- Auth bypass: новый admin endpoint без `_check_token` / `Depends(require_admin)`
|
||
- CORS wildcards: `allow_origins=["*"]` для credentialed endpoints
|
||
- Leaked logs: `print(token)`, `logger.info(f"...{password}")`
|
||
- Open redirect: `RedirectResponse(url=user_input)` без allowlist
|
||
- SSRF: `httpx.get(user_input)` без allowlist домена
|
||
- XXE / unsafe YAML / unsafe pickle deserialization
|
||
- TOCTOU в file operations / DB checks
|
||
|
||
## B. Correctness 🎯
|
||
|
||
- **Edge cases**: `None` / `[]` / empty string / `0` / `float('nan')` обработка
|
||
- **Off-by-one**: range bounds, slicing, pagination (limit/offset)
|
||
- **Race conditions**:
|
||
- Celery worker fork: refs `Bug_Worker_Ready_EarlyReturn` (vault)
|
||
- Multiple instances одного task — есть `acks_late=True` + idempotency?
|
||
- DB: `SELECT ... FOR UPDATE` где нужен row lock?
|
||
- **Transaction symmetry**: каждый `BEGIN` → `COMMIT` или `ROLLBACK`? Nested `with db.begin()` корректны?
|
||
- **Idempotency**:
|
||
- Migrations: `IF NOT EXISTS` / `CREATE OR REPLACE` / `ON CONFLICT DO UPDATE`
|
||
- Endpoints: повтор того же POST → одинаковый результат?
|
||
- Scrapers: rerun → нет дубликатов в БД?
|
||
- **Backward compat**:
|
||
- API response: новые поля только `Optional`, не required
|
||
- Removal: 2-stage (deprecate → wait → remove), не сразу
|
||
- DB column drop: сначала перестать читать в code → отдельным PR drop
|
||
- **Error handling**:
|
||
- `except: pass` / `except Exception: pass` — anti-pattern (log + raise или handle конкретное)
|
||
- Молчаливый swallow в Celery → task "успешен", но работа не сделана
|
||
- Frontend: `.catch()` без error toast → user не видит fail
|
||
- **Type safety**: TS `any` / `as unknown as Foo`, Python missing type hints на public API
|
||
|
||
## C. Performance ⚡
|
||
|
||
- **N+1 queries**: цикл с `db.execute(...)` или `await session.execute(...)` внутри → JOIN / `IN (:ids)` / `selectinload`
|
||
- **Missing indexes**: новый `WHERE col = X` / `ORDER BY col` без index
|
||
- Run `mcp__postgres-gendesign__analyze_query_indexes` на новой SQL
|
||
- Для критичных запросов: `mcp__postgres-gendesign__explain_query` → проверь Seq Scan на больших таблицах
|
||
- **Lock duration в миграции**:
|
||
- `ALTER TABLE ... ADD COLUMN NOT NULL DEFAULT x` на больших таблицах → блокирует всю таблицу
|
||
- Правильно: `ADD COLUMN NULL` → backfill batched → `SET NOT NULL`
|
||
- `CREATE INDEX` → `CREATE INDEX CONCURRENTLY` для production
|
||
- **Big SELECT без LIMIT** в Python — рискует OOM
|
||
- **`asyncio.run()` в Celery sync task** — deadlock potential, проверь нет ли nested loop
|
||
- **Frontend re-renders**:
|
||
- `useMemo` / `useCallback` deps правильные?
|
||
- `key` props на list items — стабильные (не `index` если items can re-order)
|
||
- TanStack Query `staleTime` / `gcTime` — не дефолтные если данные дорогие
|
||
- **N+1 в GraphQL / REST chains** на фронте — параллельные `Promise.all` или batch endpoint
|
||
|
||
## D. Project conventions 📋
|
||
|
||
Python (`.claude/rules/backend.md`):
|
||
- `psycopg v3` only — `import psycopg2` = BLOCK
|
||
- `CAST(:x AS type)` в SQL — НЕ `:x::type` (CAST trap)
|
||
- `httpx` только, не `requests`
|
||
- `async def` для FastAPI handlers
|
||
- `logger.*` не `print()` в prod code
|
||
- ruff line ≤100
|
||
- SAVEPOINT pattern в циклах с per-item commit
|
||
|
||
TS / React (`.claude/rules/frontend.md` + `ui-tokens.md` + `ui-conventions.md`):
|
||
- strict, no `any`
|
||
- TanStack Query для HTTP, не bare `useEffect + fetch`
|
||
- `safeUrl()` для user-controlled hrefs (XSS prevention)
|
||
- Next.js 15 app router patterns (`'use client'` только где нужно)
|
||
- Design tokens — только из `ui-tokens.md` списка (нет inline `#hex`)
|
||
|
||
SQL (`.claude/rules/sql.md`):
|
||
- `data/sql/NN_topic.sql` — numbered, sequential
|
||
- `BEGIN; ... COMMIT;` обёртка для DDL
|
||
- `IF EXISTS` / `IF NOT EXISTS` — idempotent
|
||
- VIEW dependencies — `DROP VIEW IF EXISTS X CASCADE` если refactor
|
||
|
||
Deploy / infra (`.claude/rules/deploy.md`):
|
||
- `.env.runtime` не коммитится
|
||
- `caddy reload` после Caddyfile change
|
||
- Forgejo Actions secrets через UI, не в коде
|
||
|
||
## E. Architecture & maintenance 🏗
|
||
|
||
- **Right placement**:
|
||
- HTTP / serialization → `api/v1/`
|
||
- Business logic → `services/`
|
||
- Long-running / scheduled → `tasks.py` (Celery) / `services/<area>/`
|
||
- Не запихивай business logic в FastAPI handler
|
||
- **DRY**: дубль логики в parcels.py vs analytics_queries.py vs services/?
|
||
- **Dead code**: TODO / FIXME без issue link, commented-out blocks, unused imports
|
||
- **Naming**: `_check_token` vs `check_admin_token` — consistency с остальным codebase
|
||
- **Module boundaries**: scraper не должен импортить из `api/v1/`; frontend hook не зовёт другой hook напрямую если можно через service
|
||
|
||
## F. Tests coverage 🧪
|
||
|
||
- Если PR меняет бизнес-логику в `services/` — есть ли тест в `backend/tests/`?
|
||
- Если added endpoint — есть ли smoke test?
|
||
- Если fix bug — есть ли regression test покрывающий именно тот сценарий?
|
||
- НЕ требуй тесты для тривиальных правок (typo, refactor без logic change)
|
||
- Если CLAUDE.md / project явно говорит "не пиши тесты пока не попросят" — не настаивай
|
||
|
||
## G. Vault & decisions cross-check 📚
|
||
|
||
**Обязательный шаг для P0-P1 PR.**
|
||
|
||
```
|
||
mcp__obsidian__obsidian_simple_search "<keyword из diff>"
|
||
mcp__obsidian__obsidian_simple_search "<имя функции / эндпоинта>"
|
||
```
|
||
|
||
Проверь:
|
||
- [ ] `fixes/` — была ли уже эта же ошибка? Новый код не повторяет старый bug?
|
||
- [ ] `decisions/` — есть ли архитектурный decision который этот PR нарушает?
|
||
- [ ] `limitations/` — known issue который этот код должен учитывать?
|
||
- [ ] `code/patterns/` — есть ли established pattern для этого типа изменения?
|
||
|
||
Если PR fix bug — должна быть entry в `fixes/<bug>.md` (per CLAUDE.md rule #6).
|
||
Если PR architectural change — должна быть entry в `decisions/<decision>.md`.
|