4b019ebd67
All checks were successful
CI Trade-In / changes (pull_request) Successful in 8s
CI / changes (pull_request) Successful in 9s
CI Trade-In / backend-tests (pull_request) Has been skipped
CI Trade-In / browser-tests (pull_request) Has been skipped
CI Trade-In / frontend-checks (pull_request) Has been skipped
CI / backend-tests (pull_request) Has been skipped
CI / frontend-tests (pull_request) Has been skipped
CI / openapi-codegen-check (pull_request) Has been skipped
Ролевые окна (claude --strict-mcp-config --mcp-config .claude/mcp/<role>.json)
не используются — отказ по стоимости: слишком много токенов на простых задачах.
Удалено (25 файлов):
- scripts/: start-bot, start-{analyst,backend,frontend,qa,reviewer}, setup-bot-env
- .claude/mcp/: 5 per-role MCP-конфигов
- .claude/commands/: 6 work-as-*
- .claude/agents/: 6 auto-* + _autonomous_pickup
Ссылки в выживших файлах поправлены: CLAUDE.md, README.md, git-pr.md,
deep-review-phases/phase-5-verdict.md, deep-code-reviewer.md,
cleanup-stale-claims.sh. Self-extending guard сохранён — из него убраны
только имена удалённых файлов, сам запрет на self-merge правил пайплайна цел.
Универсальные воркеры не тронуты: backend-engineer, frontend-engineer,
database-expert, devops-engineer, tech-analyst, code-reviewer, qa-tester,
deep-code-reviewer.
126 lines
6.3 KiB
Markdown
126 lines
6.3 KiB
Markdown
# Deep Review — Phase 5: Verdict + merge
|
||
|
||
> Supporting reference. Verdict шаблон + auto-merge policy.
|
||
|
||
## Verdict format (строго)
|
||
|
||
```markdown
|
||
## Deep Code Review — verdict
|
||
|
||
### Summary
|
||
- **Status**: ✅ APPROVE / ⚠️ MINOR / 🟡 MEDIUM / 🟠 HIGH / 🔴 BLOCK
|
||
- **Files reviewed**: N (P0: a, P1: b, P2: c, P3: d)
|
||
- **Lines**: +X / -Y
|
||
- **Time spent**: ~M min
|
||
- **PR**: #N (если есть)
|
||
|
||
### 🔴 Critical (BLOCK push/merge)
|
||
- [ ] `path/file.py:42` — описание + конкретный fix (code snippet)
|
||
- [ ] `data/sql/95_foo.sql:12` — `ALTER TABLE ... NOT NULL DEFAULT 0` на 50M rows заблокирует таблицу. Split: ADD NULL → backfill batched → SET NOT NULL отдельным PR
|
||
|
||
### 🟠 High (должен быть пофиксен до merge)
|
||
- [ ] `path/file.ts:84` — race condition: state update в onSettled() при unmounted component
|
||
- [ ] `backend/app/api/v1/parcels.py:201` — N+1: цикл с `await db.execute(...)`, переделать на `IN (:ids)`
|
||
|
||
### 🟡 Medium (желательно fix, но не блокирующее)
|
||
- [ ] `file.py:120` — `except Exception: pass` swallow'ит ошибки
|
||
|
||
### 🟢 Low / nits (можно отдельным PR)
|
||
- [ ] `file.py:50` — naming `data` слишком generic, `parcels_by_district`
|
||
|
||
### Cross-file impact analysis
|
||
- `backend/app/services/site_finder/foo.py` изменил signature `find_parcels()` →
|
||
callers обновлены: `api/v1/parcels.py:88` ✅, но `tasks.py:142` всё ещё передаёт старый kwarg ❌
|
||
- `frontend/src/types/site-finder.ts` regen нужен — `npm run codegen` не запущен в этом PR
|
||
|
||
### Vault cross-check
|
||
- Похожий fix был в `fixes/Bug_Worker_Ready_EarlyReturn.md` — паттерн использован корректно ✅
|
||
- Decision `decisions/2026-05-12-no-mutable-defaults.md` — этот PR не нарушает
|
||
- Если bug fix, но нет entry в `fixes/` → ⚠️ нужна docs (medium severity)
|
||
|
||
### Performance findings
|
||
- `EXPLAIN ANALYZE` на новом query `parcels.find_by_district`:
|
||
- Seq Scan на `rosreestr_parcels` (5.2M rows) — нужен index `(district_id, status)`
|
||
- Estimated cost 12.4k → с индексом 0.8
|
||
|
||
### Positive observations
|
||
- ✅ `ON CONFLICT DO UPDATE` использован корректно в bulk_upsert
|
||
- ✅ `CREATE INDEX CONCURRENTLY` в миграции
|
||
|
||
### Recommended next steps
|
||
1. Worker (backend-engineer): fix 🔴 #1 + 🟠 #1, push fixup commit
|
||
2. После fixup: run `npm run codegen` в frontend
|
||
3. Перед merge: проверить EXPLAIN на проде через MCP postgres
|
||
4. Создать vault entry `fixes/<this-bug>.md`
|
||
|
||
### Сomplexity / blast radius score
|
||
- **Risk**: Low / Medium / High / Critical
|
||
- **Reversibility**: Easy revert / DB migration needs manual rollback / Data loss on revert
|
||
- **Recommended merge window**: anytime / business hours only / pre-deploy freeze
|
||
```
|
||
|
||
## Когда BLOCK (🔴) — обязательные триггеры
|
||
|
||
1. Hardcoded secret / token / password в commit (даже test files)
|
||
2. SQL injection vector
|
||
3. Auth bypass на admin endpoint
|
||
4. Migration без rollback plan (если меняет / удаляет данные)
|
||
5. Breaking API change без deprecation / migration path
|
||
6. Удаление prod данных без явного approval
|
||
7. `--no-verify` / `--force` / `--amend` в недавних коммитах ветки
|
||
8. **Прямой push в main** (HEAD == main, не feature branch)
|
||
9. `ALTER TABLE ... NOT NULL DEFAULT` на таблице >1M rows без batched backfill
|
||
10. `CREATE INDEX` (не CONCURRENTLY) на prod таблице >1M rows
|
||
11. `DROP COLUMN` / `DROP TABLE` без 2-stage rollout
|
||
12. Celery task без idempotency маркера (вернёт дубли при retry)
|
||
13. Scraper без dedup ключа в `ON CONFLICT`
|
||
|
||
## Когда APPROVE (✅)
|
||
|
||
- Нет critical / high issues
|
||
- Cross-file impact проверен — все callers consistent
|
||
- Conventions OK (psycopg v3, httpx, ruff, TS strict)
|
||
- Backward compat сохранён
|
||
- Performance не регрессирует (или регрессия документирована и acceptable)
|
||
- Vault entry создана для нетривиальных fixes / decisions
|
||
- Tests есть для новой бизнес-логики (если applicable per scope)
|
||
- Pre-flight: на feature branch, нет force / no-verify в истории
|
||
|
||
## Auto-merge policy (PR review mode)
|
||
|
||
При review открытых Forgejo PR — если verdict **✅ APPROVE** (нет 🔴/🟠/🟡):
|
||
**мержи сам, любой scope** (политика 2026-06-27, git-pr.md § Auto-merge policy).
|
||
|
||
### Pre-merge checks (все обязательны)
|
||
|
||
1. `curl -sH "$H" "$REPO/pulls/<N>" | jq '{state,mergeable,draft,head:.head.sha}'`
|
||
→ `state=open` (не уже merged), `mergeable=true` (no conflicts), `draft=false`
|
||
2. Head SHA не изменился после твоего review (нет fixup push'ей с момента scan'а файлов)
|
||
3. CI status passing — Forgejo Actions UI или `curl -sH "$H" "$REPO/commits/<sha>/status"`
|
||
|
||
### Merge команда (Forgejo)
|
||
|
||
```bash
|
||
# Через MCP (preferred):
|
||
mcp__forgejo__merge_pull_request (Do=squash, delete_branch_after_merge=true)
|
||
|
||
# Или curl:
|
||
curl -sH "$H" -H "Content-Type: application/json" \
|
||
-X POST "$REPO/pulls/<N>/merge" \
|
||
-d '{"Do":"squash","delete_branch_after_merge":true}'
|
||
|
||
# Post-merge confirmation comment:
|
||
curl -sH "$H" -H "Content-Type: application/json" \
|
||
-X POST "$REPO/issues/<N>/comments" \
|
||
-d '{"body":"Merged via deep-code-reviewer — verdict ✅ APPROVE."}'
|
||
```
|
||
|
||
Forgejo API возвращает пустой body при успехе merge → verify через GET pulls/<N> что `state == merged`.
|
||
|
||
### Когда НЕ мержить даже при APPROVE
|
||
|
||
- `mergeable=false` (conflicts) → comment "approved but has conflicts — rebase needed"
|
||
- CI failing → comment "approved but CI red — wait for green"
|
||
- Draft PR → comment "approved, ready when undrafted"
|
||
- Head SHA changed после твоего scan'а → НЕ мержь stale verdict, re-review нужен
|
||
- Diff меняет правила пайплайна: git-pr.md § Auto-merge policy, CLAUDE.md Critical rules → НЕ merge, label `needs-human` (self-extending guard)
|