docs: Sonnet review of migration plan — 2 critical, 3 important, 5 improvements + my analysis
This commit is contained in:
“Naeel”
@@ -0,0 +1,132 @@
|
|||||||
|
# Sonnet Review: план миграции ВМ → Flask — анализ и ответ
|
||||||
|
|
||||||
|
**Дата:** 2026-07-14
|
||||||
|
**Источник:** Sonnet (Claude) — ревью `History/migration-vm-to-flask-plan-2026-07-14.md`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Резюме Соннета
|
||||||
|
|
||||||
|
План **одобрен архитектурно**. Критичных блокеров — 2, важных дополнений — 3, улучшений — 5.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Критичные проблемы (Соннет)
|
||||||
|
|
||||||
|
### 1. `site/` — конфликт с Python stdlib ⛔
|
||||||
|
|
||||||
|
> `from site.db import ...` — `site` это встроенный модуль Python. При запуске не из корня Python найдёт встроенный `site`, а не локальный.
|
||||||
|
|
||||||
|
**Моё мнение:** ✅ **Согласен полностью.** Это реальная проблема. `site` — built-in модуль Python, импортируется при старте интерпретатора. Если `PYTHONPATH` или `sys.path` поставит корень раньше чем `site-packages` — получим наш пакет. Если наоборот — stdlib. Нестабильно.
|
||||||
|
|
||||||
|
**Решение:** переименовать `site/` → `app/` или `contracts_app/`. Я за `app/` — короче, семантически понятно (Flask application factory), не конфликтует ни с чем.
|
||||||
|
|
||||||
|
### 2. `/api/cleanup` — нулевая аутентификация ⛔
|
||||||
|
|
||||||
|
> Любой может POST /api/cleanup → потеря всех данных.
|
||||||
|
|
||||||
|
**Моё мнение:** ✅ **Согласен.** Но с нюансом. Текущий `convert_server.py` тоже имеет этот эндпоинт без auth — и он на внешнем домене `contracts.kube5s.ru`. Так что это не регресс, а существующая дыра.
|
||||||
|
|
||||||
|
**Решение:**
|
||||||
|
- Минимум: `X-Api-Key` в заголовке, сверка с `os.environ["API_KEY"]`
|
||||||
|
- Средний: nginx `allow 127.0.0.1; deny all;` на этот location (если cleanup вызывается только с самого Flask-контейнера)
|
||||||
|
- Правильный: убрать cleanup вообще, заменить на автоочистку по TTL (cron/APScheduler)
|
||||||
|
|
||||||
|
Но это **отдельная задача**, не часть миграции. В рамках миграции — просто не ухудшить ситуацию.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Важные дополнения (Соннет)
|
||||||
|
|
||||||
|
### 3. Файловый lock для classify — хрупкий
|
||||||
|
|
||||||
|
> `/tmp/classify_{batch_id}.lock` — если Flask упадёт, lock остаётся навсегда.
|
||||||
|
|
||||||
|
**Моё мнение:** ✅ **Согласен.** Lock-файл — антипаттерн в stateless-приложении. Сейчас в `convert_server.py` та же схема (subprocess + lock-файл), и она работает только потому что ВМ не рестартит.
|
||||||
|
|
||||||
|
**Решение:** in-memory `dict[batch_id] → thread` + проверка `thread.is_alive()`. При старте Flask никаких lock-файлов нет. Лучше: store `classify_status='processing'` в БД и сбрасывать `processing → pending` при старте приложения.
|
||||||
|
|
||||||
|
### 4. SSE — нет heartbeat
|
||||||
|
|
||||||
|
> При медленном LLM nginx и браузеры режут соединение.
|
||||||
|
|
||||||
|
**Моё мнение:** ✅ **Согласен.** Добавить `yield ": heartbeat\n\n"` раз в 15 секунд. SSE-комментарий (строка начинается с `:`) игнорируется EventSource, но сбрасывает таймауты nginx/браузера.
|
||||||
|
|
||||||
|
### 5. Старые proxy-роуты — явно не сказано удалить
|
||||||
|
|
||||||
|
> В плане написано «создать новый app.py», но не сказано явно «удалить старые /api/* proxy-роуты».
|
||||||
|
|
||||||
|
**Моё мнение:** ✅ **Согласен, но это очевидно.** Новый `app.py` полностью заменяет старый — старых роутов `/chat`, `/api/prompts` (proxy на ВМ) не будет, потому что логика теперь локальная. Но в плане стоит написать явно: «удалить ВСЕ proxy-роуты, они больше не нужны».
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Улучшения (Соннет)
|
||||||
|
|
||||||
|
### 6. `prompts_bp.py` и `pages_bp.py` — не раскрыты
|
||||||
|
|
||||||
|
**Моё мнение:** ✅ **Справедливо.** Но они тривиальны: `prompts_bp.py` = CRUD по таблице `prompts` (4 ручки), `pages_bp.py` = `render_template("index.html")` + `render_template("architect.html")`. Не расписывал детально потому что там нечего расписывать. Можно добавить один абзац для полноты.
|
||||||
|
|
||||||
|
### 7. `drhider/` — не упоминается
|
||||||
|
|
||||||
|
**Моё мнение:** ⚠️ **Справедливо, но осознанно.** `drhider/` — это отдельный сервис (обфускация персональных данных), не часть пайплайна сверки договоров. Он уже работает на Flask в `site/services/drhider.py` и `site/routes/drhider_bp.py`. В план миграции сверки он не входит — это другой сервис на том же хосте. Но стоит упомянуть это явно: «drhider не трогаем, он уже на Flask».
|
||||||
|
|
||||||
|
### 8. `repository.py` — не объяснено как расширить
|
||||||
|
|
||||||
|
**Моё мнение:** ⚠️ **Справедливо, но слишком рано.** Protocol-паттерн в `repository.py` уже есть, `PgRepository` частично реализован. Полное внедрение DI через репозиторий во все services — это Фаза 2, после того как базовая миграция заработает. Сейчас services используют `from site.db import documents` напрямую, и это ОК для первого шага. Переписывать всё на DI сразу = риск сломать работающий код.
|
||||||
|
|
||||||
|
**План:** после миграции — отдельная задача «внедрить Repository DI во все services».
|
||||||
|
|
||||||
|
### 9. Graceful shutdown — in-flight classify потоки
|
||||||
|
|
||||||
|
**Моё мнение:** ✅ **Справедливо.** При `docker stop` daemon-потоки убиваются, документы застревают в `classify_status='processing'`.
|
||||||
|
|
||||||
|
**Решение (уже в коде ВМ!):** `db/documents.reset_classify_status(batch_id)` сбрасывает `processing → pending`. Добавить вызов в `app.py` при старте:
|
||||||
|
|
||||||
|
```python
|
||||||
|
# При старте: сбросить все застрявшие processing → pending
|
||||||
|
from site.db.connection import execute
|
||||||
|
execute("UPDATE documents SET classify_status='pending', error_message=NULL WHERE classify_status='processing'")
|
||||||
|
```
|
||||||
|
|
||||||
|
### 10. Тесты — только curl, нет pytest
|
||||||
|
|
||||||
|
**Моё мнение:** ✅ **Справедливо.** В репо уже есть `tests/` с `conftest.py`. Нужен шаг «адаптировать тесты под `app.test_client()`». Но это **не блокер для миграции** — тесты можно дописать после.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Моя итоговая оценка
|
||||||
|
|
||||||
|
| Категория | Пунктов | Критичность |
|
||||||
|
|-----------|---------|-------------|
|
||||||
|
| Критичные (надо исправить ДО миграции) | 2 | `site/` → `app/`, auth на cleanup |
|
||||||
|
| Важные (надо добавить в план) | 3 | lock→in-memory, heartbeat, явное удаление proxy |
|
||||||
|
| Улучшения (можно после миграции) | 5 | prompts/pages детали, drhider, repository DI, graceful, тесты |
|
||||||
|
|
||||||
|
### Что делаем ДО начала миграции:
|
||||||
|
|
||||||
|
1. **Переименовать `site/` → `app/`** — везде в плане, во всех импортах
|
||||||
|
2. **Добавить auth на `/api/cleanup`** — `X-Api-Key` или `require_local`
|
||||||
|
|
||||||
|
### Что добавляем в план:
|
||||||
|
|
||||||
|
3. **in-memory lock** для classify вместо файлового
|
||||||
|
4. **SSE heartbeat** раз в 15 сек
|
||||||
|
5. **Упомянуть явно:** старые proxy-роуты удалить, drhider не трогать
|
||||||
|
6. **Graceful shutdown:** сброс `processing → pending` при старте
|
||||||
|
7. **prompts_bp / pages_bp** — один абзац что там
|
||||||
|
8. **repository DI** — отложить на пост-миграцию
|
||||||
|
|
||||||
|
### Что НЕ блокер:
|
||||||
|
|
||||||
|
- `drhider` — уже на Flask, не часть миграции
|
||||||
|
- Repository DI — Фаза 2
|
||||||
|
- pytest — после миграции
|
||||||
|
- prompts/pages — тривиальны
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Вердикт
|
||||||
|
|
||||||
|
План **рабочий**. Соннет подтвердил архитектурные решения (blueprints, разделение db/services/routes, генератор для SSE, DI). Критичные замечания — реальные и требуют правки плана. Остальное — улучшения надёжности, которые можно добавить в план сейчас или отложить.
|
||||||
|
|
||||||
|
**Готовность к миграции:** 90%. После правки 2 критичных пунктов → можно начинать.
|
||||||
Reference in New Issue
Block a user