Document comparison review findings

2026-08-27 11:24:32 +03:00
parent 811be85efe
commit f9745e5c6b
3 changed files with 429 additions and 0 deletions
@@ -0,0 +1,81 @@
 # Ответ Соннета: code review pipeline сверки
 Дата получения: 2026-08-27  
 Источник: пользовательский attachment `Pasted text #1`  
 Статус: внешний отчёт, не подтверждённый текущим агентом
 ## Область
 Соннет анализировал pipeline собственно сверки: `process.py`, `pipeline_bp.py`, `compare.js`, `app.js`, `llm.py`, `llm_client.py`, `llm_prompt.py`, `spec_events.py`, `spec_current.py`, `supplements.py`, `connection.py`, `metrics.py` и связанные тесты. Upload, ZIP, парсинг, классификация и grouping заявлены как исключённые из scope.
 ## Заявленные findings
 ### BLOCKER
 - **B-1:** backend выдаёт событие `complete`, frontend обрабатывает только `done`; успешная сверка отображается как ошибка соединения.
 - **B-2:** `apply_ops()` якобы не имеет единой транзакции; при сбое возможен частичный commit и недостоверная summary.
 - **B-3:** UPDATE поля `name` якобы не пересчитывает `name_hash`, что может привести к дублированию строки при следующем документе.
 ### HIGH
 - **H-1:** ветка неизвестного action якобы не увеличивает `seq`.
 - **H-2:** summary `apply_ops()` якобы не содержит `unresolved`, поэтому счётчик недоступен UI.
 - **H-3:** результат `check_arithmetic()` игнорируется; арифметическая ошибка не попадает в SSE/UI.
 - **H-4:** `full_replace` с пустым `ops` якобы сначала очищает current state и затем успешно применяет ноль операций.
 ### MEDIUM
 - **M-1:** pipeline безусловно завершает работу событием `complete`, даже после ошибок всех документов.
 - **M-2:** отсутствует блокировка двух одновременных сравнений одного `contract_id`.
 - **M-3:** отсутствует клиентский timeout SSE.
 - **M-4:** `last_event_id` в `spec_current` якобы никогда не заполняется, поэтому cleanup supplement не работает.
 ### LOW
 - **L-1:** сортировка по `doc_date` и `created_at` в `process.py` использует поля, которые якобы не возвращаются `supplements.list_by_contract()`.
 - **L-2:** пустой `current_spec` не различает штатный первый документ и состояние после ошибки.
 ## Ответы Соннета на обязательные вопросы
 1. Допник без базового договора не фильтруется; при пустом current state получает extract prompt и может быть ошибочно обработан как базовый документ.
 2. `run_pipeline()` считает сверку успешной при наличии хотя бы одного supplement и безусловно выдаёт финальное событие.
 3. Да, финальное событие может прийти после `extract_error`, включая случай, когда ошиблись все документы.
 4. `complete` против `done` объявлено реальным frontend/backend багом.
 5. Пустой `full_replace` очищает спецификацию; это признано опасным.
 6. Частичный `apply_ops()` якобы остаётся в БД, но UI получает `extract_error` и неверную нулевую summary.
 7. Единой транзакции current state и event history, по мнению Соннета, нет.
 8. Повторный запуск сначала очищает состояние; последовательный запуск может дать чистый результат, конкурентный опасен.
 9. Конкурентные pipeline для одного договора создают race condition и риск коллизий `seq`.
 10. Пустой корректный ответ и повреждённый/неполный ответ не различаются.
 11. `UNRESOLVED` сохраняется в `spec_events`, но не отображается пользователю и не включается в summary.
 12. Арифметическая ошибка может остаться в БД, а pipeline всё равно завершиться успешно.
 13. Нужны тесты для финального события, rollback, смены name, пустого full_replace, unknown action, unresolved summary, arithmetic warning и concurrency.
 14. Главные риски: частичные commits, дублирование после смены name, уничтожение состояния при пустом full_replace, ложная ошибка UI и отсутствие защиты от concurrency.
 ## Предложенный Соннетом порядок исправлений
 1. `complete` -> `done`.
 2. Пересчёт `name_hash` при изменении `name`/`date_start`.
 3. Запрет пустого `full_replace` до очистки state.
 4. Единая транзакция для `apply_ops()`.
 5. Инкремент `seq` для неизвестного action.
 6. Счётчик `unresolved` и `total_unresolved`.
 7. SSE warning для arithmetic mismatch.
 8. `had_errors` в финальном событии.
 9. Заполнение `last_event_id`.
 ## Что не является подтверждённым фактом
 Этот файл фиксирует именно ответ Соннета. Ни один finding здесь не следует считать основанием для изменения кода до повторной проверки:
 - по исходникам;
 - по фактической схеме БД и реализации connection layer;
 - по тестам;
 - по реальному frontend/backend event contract;
 - по воспроизводимому сценарию.
 Следующий этап: критическая повторная проверка каждого finding и уточнение вопросов Соннету только там, где в его отчёте останется неразрешённое противоречие или отсутствует доказательство.
 ## Уточнение пользователя
 Соннет используется только для анализа. Он не должен писать код, патчи или diff, изменять файлы либо выполнять команды: это ограничение введено для контроля стоимости. Реализацию и проверки выполняем отдельно после критической перепроверки findings.
@@ -0,0 +1,231 @@
 # Промпт для Соннета: code review только собственно сверки
 Нужно провести строгий code review **только той части системы, которая выполняет собственно сверку документов после формирования группы**.
 Не анализируй и не ревьюируй upload, WebDAV/VM upload, ZIP-распаковку, выбор папки, дедупликацию файлов, парсинг DOC/DOCX, garbage-фильтры, классификацию документов, определение типа документа, matching/grouping документов и UI загрузки. Эти части находятся вне области данного ревью.
 ## Контекст
 Система получает уже сформированную группу договора и связанных документов. Затем она должна:
 1. определить порядок документов группы;
 2. получить текущую спецификацию;
 3. передать текущую спецификацию и текст очередного документа в LLM;
 4. получить операции `ADD`, `UPDATE`, `DELETE`, `UNRESOLVED` или режим `full_replace`;
 5. корректно транслировать ссылки LLM на строки текущей спецификации;
 6. применить операции к БД и сохранить историю событий;
 7. отдать прогресс и результат через SSE;
 8. показать пользователю фактический итог сверки.
 Особенно проверь, что система не выдаёт успешный результат, если часть операций или документов фактически не обработана.
 ## Файлы для обязательного изучения
 Изучи только эти файлы и их прямые зависимости, необходимые для понимания сверки:
 1. `contracts-flask/site/services/process.py`
   - основной pipeline сверки;
   - порядок документов;
   - получение текущей спецификации;
   - подготовка текста документа;
   - вызов LLM;
   - трансляция `target_id` в `target_hash`;
   - обработка `full_replace`;
   - применение операций;
   - арифметическая проверка;
   - SSE-события логического pipeline.
 2. `contracts-flask/site/routes/pipeline_bp.py`
   - endpoint `GET /process-v2`;
   - генерация SSE;
   - heartbeat;
   - закрытие/обрыв соединения;
   - преобразование исключений в SSE-события;
   - различие HTTP-ошибки до начала стрима и ошибки внутри стрима.
 3. `contracts-flask/site/static/compare.js`
   - `startCompareSSE()`;
   - обработка `extract_start`, `llm_done`, `applied`, `extract_error`, `apply_error`, `done`, `error`;
   - обработка `EventSource.onerror`;
   - закрытие EventSource;
   - соответствие frontend-событий backend-событиям.
 4. `contracts-flask/site/static/app.js`
   - функции запуска сверки группы;
   - вызов `/api/apply-groups`;
   - получение `contract_id`;
   - запуск `/process-v2`;
   - обработка success/error/connection error;
   - изменение состояния группы после завершения.
 5. `contracts-flask/site/services/llm.py`
   - фактический вызов LLM для сверки;
   - формат prompt и ответа;
   - timeout/retry;
   - обработка невалидного ответа;
   - возможная потеря или искажение операций.
 6. `contracts-flask/site/llm_prompt.py`
   - prompt сверки;
   - формат текущей спецификации;
   - формат ожидаемых операций;
   - ограничения для `ADD`, `UPDATE`, `DELETE`, `UNRESOLVED`, `full_replace`.
 7. `contracts-flask/site/db/spec_events.py`
   - `reset()`;
   - `clear_current()`;
   - `apply_ops()`;
   - транзакции и атомарность;
   - сохранение истории;
   - статусы применённых и неразрешённых операций;
   - поведение при частичной ошибке.
 8. `contracts-flask/site/db/spec_current.py`
   - получение текущих строк спецификации;
   - идентификаторы и `name_hash`;
   - получение `elements_json`;
   - согласованность данных между текущим состоянием и историей.
 9. `contracts-flask/site/db/supplements.py`
   - только функции, используемые `process.py` для получения документов уже сформированной группы;
   - порядок и фильтрация документов;
   - отсутствие/дублирование документов.
 10. `contracts-flask/site/services/metrics.py`
    - `check_arithmetic()`;
    - что именно проверяется;
    - может ли ошибка арифметики повлиять на результат;
    - почему проверка не должна маскировать ошибку применения.
 ## Тесты для обязательного изучения
 1. `contracts-flask/tests/test_process_pipeline.py`
   - какие сценарии реально покрыты;
   - корректность `full_replace`;
   - корректность трансляции `target_id`;
   - отсутствие тестов на реальные SSE и ошибки LLM.
 2. `contracts-flask/tests/test_grouping.py`
   - только часть, необходимая для понимания структуры группы, передаваемой в `apply_groups`.
 3. `contracts-flask/tests/test_metrics.py`
   - только тесты арифметической проверки, относящиеся к операциям сверки.
 4. Найди все остальные тесты, которые напрямую вызывают `run_pipeline`, `apply_ops`, `process-v2` или `startCompareSSE`, и включи их в анализ только если они действительно относятся к собственно сверке.
 ## Что проверять
 ### 1. Корректность алгоритма сверки
 - Не теряется ли первая спецификация или базовое состояние.
 - Правильно ли строится `current_spec` перед каждым следующим документом.
 - Гарантирован ли правильный порядок обработки документов.
 - Не зависит ли порядок от нестабильного `created_at` или формата даты.
 - Корректно ли работает переход между несколькими документами группы.
 - Не дублируются ли строки при повторном запуске.
 - Корректно ли работает `full_replace` при уже существующих строках.
 - Может ли `full_replace` удалить корректные данные при ошибочном/неполном ответе LLM.
 ### 2. Операции LLM
 - Как `target_id` переводится в фактический идентификатор строки.
 - Что происходит при `r0`, `r999`, `rX`, отсутствующем `target_id`, неверном `target_hash`.
 - Не может ли LLM обновить не ту строку из-за изменения порядка строк.
 - Что происходит с неизвестными действиями.
 - Что происходит с отсутствующими полями `name`, `price`, `qty`, `sum`, `date_start`.
 - Не приводит ли частично валидная операция к тихой потере данных.
 - Сохраняется ли исходный ответ LLM и prompt для аудита.
 - Как обрабатываются пустой, обрезанный, markdown-обёрнутый или частично повреждённый JSON-ответ.
 ### 3. БД, транзакции и атомарность
 - Атомарно ли применяются операции одного документа.
 - Что происходит, если пятая операция из десяти падает.
 - Может ли current state измениться, а event history не сохраниться, или наоборот.
 - Не приводит ли `reset()` к потере истории при повторном запуске.
 - Различаются ли `applied`, `unresolved`, `failed` и действительно ли эти статусы отражают результат.
 - Безопасен ли параллельный запуск двух сравнений одного договора.
 - Безопасен ли одновременный запуск сравнений разных групп.
 - Есть ли race condition между `apply-groups`, `/process-v2` и frontend state.
 ### 4. SSE и сетевые ошибки
 - Совпадает ли событие завершения backend (`complete` или `done`) с событием, которое ожидает frontend.
 - Может ли frontend навсегда оставить сравнение в состоянии `⏳`.
 - Что происходит при disconnect после `applied`, но до события завершения.
 - Что происходит при heartbeat без данных.
 - Может ли `EventSource.onerror` ошибочно объявить ошибку после нормального закрытия.
 - Показывает ли UI частичный результат как полный.
 - Есть ли таймаут на клиенте и сервере.
 - Возвращается ли пользователю причина ошибки LLM/API, а не только «Ошибка соединения».
 - Не теряются ли последние SSE-события из-за proxy buffering.
 ### 5. Числовая и предметная корректность
 - Проверяется ли арифметика `price * qty = sum` до применения и после применения.
 - Как обрабатываются `None`, строки с запятой, целые/дробные числа, отрицательные значения и большие числа.
 - Не изменяет ли арифметическая проверка данные или только логирует ошибку.
 - Может ли LLM вернуть арифметически неверную операцию, которая всё равно попадёт в current state.
 - Сохраняется ли точность Decimal/float.
 - Как обрабатываются даты и отсутствие даты.
 ### 6. Повторяемость и идемпотентность
 - Что произойдёт при повторном нажатии «Сравнить эту группу».
 - Можно ли безопасно повторить сравнение после сетевого обрыва.
 - Будут ли повторно добавлены те же строки.
 - Как отделяется новый запуск от предыдущей истории.
 - Есть ли идентификатор запуска и защита от повторного применения одного ответа.
 ## Обязательные вопросы от ревьюера
 Ответь отдельно на следующие вопросы, даже если для ответа придётся изучить прямую зависимость:
 1. Почему в реальном тесте один допник смог попасть в группу без базового договора, и может ли собственно pipeline сверки безопасно обработать такую неполную группу?
 2. При каком именно условии `run_pipeline()` считает сверку успешной?
 3. Может ли pipeline отправить `complete`, если один документ дал `extract_error` или часть операций не применилась?
 4. Почему backend использует событие `complete`, а frontend-код может ожидать `done`? Это реальный баг или только устаревший комментарий/другая ветка?
 5. Если LLM вернул `full_replace` с пустым `ops`, будет ли текущая спецификация очищена? Должно ли так происходить?
 6. Если `apply_ops()` применил только часть операций, как это отражается в SSE и UI?
 7. Есть ли транзакция, гарантирующая согласованность `spec_current` и `spec_events`?
 8. Можно ли повторно запустить `/process-v2` для того же `contract_id` без дублирования или повреждения результата?
 9. Что происходит при одновременном сравнении двух групп, относящихся к одному договору?
 10. Как система отличает «LLM вернул корректный пустой результат» от «LLM вернул повреждённый/неполный ответ»?
 11. Какие операции считаются `UNRESOLVED`, где они сохраняются и видит ли их пользователь?
 12. Может ли `check_arithmetic()` обнаружить ошибку, но pipeline всё равно завершиться успешно?
 13. Какой минимальный набор интеграционных тестов нужен для доказательства корректности реальной сверки?
 14. Какие риски остаются именно в собственно сверке после исключения upload/classify/grouping из области анализа?
 ## Формат отчёта
 Пиши отчёт в формате code review.
 Сначала findings, отсортированные по серьёзности:
 - `BLOCKER` — возможна потеря/порча данных или ложный успешный результат сверки;
 - `HIGH` — неверное применение операций, нарушение атомарности, повторное применение или потеря результата;
 - `MEDIUM` — ошибочное отображение статуса, частичный результат, нестабильность или отсутствие важной защиты;
 - `LOW` — локальная проблема качества, диагностики или сопровождаемости.
 Для каждого finding укажи:
 - severity;
 - файл и конкретный символ/участок кода;
 - точную последовательность, которая приводит к проблеме;
 - воспроизводимый сценарий;
 - фактический ущерб;
 - минимальное исправление;
 - обязательный тест.
 Не предлагай изменения вне области собственно сверки. Не исправляй код самостоятельно. Не делай общий обзор всего проекта. Если findings нет, напиши это явно и перечисли оставшиеся пробелы тестирования.
 ## Жёсткое ограничение по стоимости
 Не пиши код, патчи, diff и готовые реализации. Не изменяй файлы и не выполняй команды. Твоя задача — только code review: факты, findings, доказательства, вопросы и минимальные рекомендации словами. Любые предлагаемые исправления описывай концептуально, без реализации.
 В конце добавь:
 1. ответы на 14 обязательных вопросов;
 2. таблицу покрытия тестами;
 3. минимальный план исправлений, если они нужны;
 4. список файлов, которые действительно были изучены.
@@ -0,0 +1,117 @@
 # Критическая перепроверка ответа Соннета: pipeline сверки
 Дата: 2026-08-27  
 Основание: ответ Соннета из `History/sonnet-review-comparison-answer-2026-08-27.md`
 ## Подтверждено по исходникам
 ### B-1: `complete` против `done`
 Подтверждено.
 - `site/services/process.py` завершает `run_pipeline()` событием `{"type": "complete"}`.
 - `site/static/compare.js` завершает успешный SSE только в ветке `d.type === 'done'`.
 - При нормальном закрытии генератора после неизвестного события клиент получает `EventSource.onerror`, поэтому успешная сверка может отображаться как ошибка соединения.
 Это не предположение Соннета, а прямое несоответствие backend/frontend event contract.
 ### B-2: отдельные commits в `apply_ops()`
 Основная часть finding подтверждена.
 - `site/db/spec_events.py::apply_ops()` вызывает `execute()` для каждого события и изменения current state.
 - `site/db/connection.py::execute()` делает `conn.commit()` после каждого SQL-вызова.
 - В `apply_ops()` нет единой транзакции и rollback.
 - Поэтому исключение после уже выполненных операций оставляет предыдущие commits в БД.
 - `process.py` после исключения формирует нулевую summary и не показывает уже применённые операции как applied.
 Требует отдельной проверки формулировка о том, что history и current state обязательно расходятся при каждом сценарии: это зависит от того, на каком именно SQL-вызове возникает исключение. Но частичный commit и недостоверная summary подтверждены.
 ### B-3: stale `name_hash`
 Подтверждено.
 `site/db/spec_events.py::_update_spec_current()` обновляет `name`, `price`, `qty`, `sum`, `date_start`, но не пересчитывает и не обновляет `name_hash`. При последующем ADD с новым именем `_hash()` создаёт другой ключ, поэтому сценарий с дублированием реален.
 Нужно отдельно проверить бизнес-правило для изменения `date_start`: изменение даты может означать новый период и не во всех случаях должно менять identity строки. Автоматически объединять `name` и `date_start` в одно исправление нельзя без подтверждения модели идентичности.
 ### H-1: `seq` для неизвестного action
 Подтверждено.
 В ветке `else` `apply_ops()` вызывает `_log_unresolved(...)`, но не делает `seq += 1`. Следующая операция получает тот же `seq`.
 ### H-2: `unresolved` недоступен в summary
 Подтверждено по текущим участкам.
 `apply_ops()` возвращает только `added`, `updated`, `deleted`. При этом frontend использует `d.total_unresolved`, а `run_pipeline()` не формирует это поле в финальном событии.
 Требует проверки полный путь отображения `UNRESOLVED`: факт отсутствия счётчика в summary подтверждён, но следует проверить, нет ли другого endpoint/UI, который показывает события напрямую.
 ### H-3: результат `check_arithmetic()` игнорируется
 Подтверждено для `run_pipeline()`.
 `process.py` вызывает `check_arithmetic(ops)` и игнорирует возвращаемое значение. Требуется дополнительно проверить, логирует ли сама функция несоответствия и является ли её контракт предупреждением или валидатором, прежде чем выбирать severity и формат исправления.
 ### H-4: пустой `full_replace`
 Подтверждено по порядку операций.
 В `process.py` `clear_current(contract_id)` вызывается после получения `mode` и до `apply_ops()`, без проверки непустого `ops`. Пустой список при `mode == 'full_replace'` очищает current state.
 Нужно уточнить у Соннета, какие именно ответы считать повреждёнными: пустой `ops` может быть штатным ответом для пустой спецификации, хотя для существующего current state это опасный случай.
 ### M-1: финальное событие после ошибок
 Подтверждено.
 После цикла `run_pipeline()` безусловно выдаёт финальное событие, даже если каждый supplement завершился `extract_error`.
 ## Подтверждено частично или требует дополнительных доказательств
 ### M-2: concurrency
 Риск правдоподобен, но формулировку о конкретных коллизиях `seq` нужно доказать тестом. `WAL` сериализует записи, но `get_next_seq()` и последующие операции разделены во времени. Нужен воспроизводимый конкурентный тест с двумя pipeline на одном `contract_id`.
 ### M-3: SSE timeout
 Отсутствие клиентского timeout видно, но само по себе не доказывает пользовательский дефект: сервер отправляет heartbeat каждые 15 секунд. Нужно проверить proxy timeout, лимит длительности LLM и поведение при остановленном backend.
 ### M-4: `last_event_id`
 Подтверждена причина риска: `spec_current` вставляется без `last_event_id`, а UPDATE также его не заполняет; cleanup в `supplements.delete_by_document()` фильтрует по этому полю. Нужен тест удаления supplement после сверки, чтобы окончательно подтвердить наблюдаемое поведение.
 ### L-1: сортировка
 Подтверждено, что `list_by_contract()` не выбирает `doc_date` и `created_at` как поля результата, поэтому ключ сортировки в `process.py` фактически использует значения по умолчанию. При этом SQL уже сортирует по `s.created_at`, поэтому это скорее misleading code, а не доказанная поломка порядка.
 ### L-2: пустой current state
 Технически возможно, но вывод о неправильном prompt зависит от семантики группы и контракта `build_prompt()`. Нужна точная проверка prompt и отдельный сценарий сбоя/неполной группы.
 ## Дополнительные вопросы Соннету
 1. Для B-2: укажи точный SQL-вызов и сценарий исключения, при котором расходятся `spec_events` и `spec_current`; отдельно различи частичный commit и рассогласование двух таблиц.
 2. Для B-3: должна ли смена `name` менять identity строки, или `name_hash` является историческим ключом? Какие правила действуют при одновременной смене `name` и `date_start`?
 3. Для H-2: где именно пользователь должен видеть `UNRESOLVED`, если не через summary? Укажи полный frontend/backend путь и проверяемый сценарий.
 4. Для H-3: что возвращает `check_arithmetic()` и есть ли у него побочный logging? Приведи фактический пример mismatch и ожидаемый бизнес-статус.
 5. Для H-4: почему пустой `full_replace` однозначно считать повреждённым ответом, если документ может содержать пустую спецификацию? Как отличить штатный результат от обрыва/невалидного JSON?
 6. Для M-2: предоставь воспроизводимый тест или timeline с двумя потоками, который приводит к одинаковому `seq` или повреждённому current state.
 7. Для M-3: какой фактический timeout установлен на nginx/proxy и как он соотносится с heartbeat и максимальным временем обработки группы?
 8. Для M-4: есть ли штатный путь удаления supplement после сверки, и должен ли он удалять строки current state, если строка затронута несколькими supplements?
 9. Для L-2: приведи точный контракт `extract`/`diff` prompt и доказательство, что пустой current state после ошибки действительно меняет смысл следующего LLM-вызова.
 10. Какие findings Соннет считает подтверждёнными тестом, а какие являются только статическим риском?
 11. Проверь финальное событие по реальному frontend-коду: нет ли другой ветки, которая обрабатывает `complete` или завершение `EventSource` без `done`?
 12. Для каждого BLOCKER укажи минимальный regression test, который сначала падает на текущей версии и проходит после исправления.
 ## Предварительный вывод
 Без дополнительных ответов Соннета уже достаточно доказательств для регистрации B-1, B-2, B-3, H-1, H-2, H-3, H-4 и M-1 как реальных проблем текущего кода. M-2, M-3, M-4 и L-2 требуют воспроизводимых тестов или более точной трассировки. L-1 подтверждён как избыточная/вводящая в заблуждение сортировка, но не как текущая поломка порядка документов.
 Изменения production-кода по этим findings не выполнялись.
 ## Обязательное ограничение для дальнейшего общения с Соннетом
 Соннет не должен писать код, patch или diff и не должен изменять файлы. Нужно запрашивать только критический анализ, проверяемые доказательства, воспроизводимые сценарии, тестовые идеи в виде описания и дополнительные вопросы. Реализацию выполняем отдельно после собственной проверки findings.