Разберите дифф ветки до мержа за два прохода

1. Первый шаг: получить настоящий дифф

Не ревьюй по описанию задачи, по тексту PR и не по одному файлу, который прислали в чат. Клонируй и смотри сам:

Три точки (...) дают дифф от точки ветвления, две точки покажут ещё и чужие коммиты из base — это самая частая причина, по которой ревьюер обсуждает код, которого автор не писал.

Исключённые из чтения файлы всё равно проверь по --stat: не появилась ли новая зависимость в package-lock.json / poetry.lock и не переписан ли автогенерённый файл руками.

1.1 Что должно быть под рукой до чтения

• git log -S"<имя изменённой функции>" --oneline — почему эта строка выглядит именно так. Половина «явно лишних» проверок — это следы прошлых инцидентов.
• git blame на строку, которую предлагаешь удалить.
• Тесты и линтер запущены на затронутых путях, а не на всём наборе. «У меня зелёное» без запуска — не аргумент.

2. Пороги: когда ревью в принципе возможно

Внимание ревьюера линейно не масштабируется: на диффе в 1000+ строк находят примерно столько же замечаний, сколько на 300, — то есть остальные дефекты просто не находят.

Отдельный порог: любая миграция БД, любое изменение прав доступа, любое изменение денежных расчётов ревьюются полностью, независимо от размера PR.

3. Каталог сигнатур: что искать грепом

Не «проверь производительность», а конкретные строки. Прогони по диффу и посмотри глазами на каждое попадание.

3.2 Деньги во float

Рубли и копейки во float — дефект, а не стилистика. Комиссия 16.5% от 1 999.99 ₽ во float даёт 329.99835000000005; после round(..., 2) в одном месте и усечения в другом сумма акта и сумма проводок расходятся на копейки, и расхождение накапливается по строкам отчёта. Правильно — Decimal со строкой в конструкторе и один quantize, либо целые копейки. Отдельно смотри округление НДС: оно делается по документу, а не по каждой строке, иначе итог не сойдётся со счётом-фактурой.

3.4 Строковая сборка SQL

Параметр, склеенный в запрос, — критично всегда, даже если «сюда приходит только внутренний id»: завтра этот id приедет из вебхука. Не дефект — имя таблицы или колонки из белого списка констант; параметризовать его нельзя, но проверь, что список закрытый.

3.6 Ретраи без джиттера и без потолка

Признак: for attempt in range(...) + sleep(2 ** attempt) без случайной добавки и без потолка общего времени. Без джиттера все клиенты, упавшие на одном 429, вернутся одновременно. Отдельно проверь, читается ли при 429 заголовок Retry-After: своя лестница задержек поверх честного ответа сервера — отказ, замаскированный под настойчивость.

3.7 Вебхук без идемпотентности

Сюда же: проверка подписи вебхука. Если её нет — критично, ручка публичная.

4. Разобранные дефекты

4.3 Индекс, которого нет

Изменение добавило фильтр по новому полю:

5. Ревью миграций БД

Миграция — единственная часть PR, которую нельзя откатить кнопкой. Ревьюй по этому списку целиком.

5.2 Что спросить у автора миграции

Сколько строк в таблице сейчас и через год (оценка времени без числа строк — не оценка); совместима ли миграция со старым кодом, работающим во время выката; проверялась ли она на копии продовых объёмов, а не на пустой локальной базе; что будет, если она упадёт посередине. Проверяемый ответ — прогон в транзакции с откатом на копии схемы с приложенным временем выполнения.

6. Ревью автогенерённого кода

Код от генератора моделей, scaffolding и LLM выглядит увереннее рукописного, поэтому ревьюется хуже. Отдельные признаки:

7. Ревью тестов

Тест полезен, если его падение указывает на поломку. Проверяется за минуту: испорти одну строку в новом коде (поменяй знак, верни константу) и запусти тесты. Зелено — тест бесполезен, это замечание уровня ВАЖНО с конкретной строкой.

Что смотреть в диффе тестов:

• Есть ли тест на новый путь. Ветка if без нового теста — повод спросить; > 100 строк новой логики без тестов — повод блокировать.
• Проверяются ли границы: пустой список, один элемент, дубликаты, ноль, отрицательное, None.
• Не завязан ли тест на текущую дату или реальную сеть — такой упадёт в CI в другой день.
• Если PR чинит баг и не добавляет тест, воспроизводящий его, баг вернётся.

8. Adversarial-проход

После того как логика понята, попробуй сломать её намеренно. Формулируй не «а что если», а конкретный сценарий с числом:

• Пустой вход: продавец без заказов, отчёт за день без продаж — не делится ли что-нибудь на ноль в расчёте средней цены.
• Дубликаты: два одинаковых артикула в выгрузке — что станет с суммой.
• Максимум: выгрузка на 200 000 строк — не собирается ли весь ответ в список в памяти.
• Параллельность: два одновременных вызова одной ручки — защищено ли уникальным ключом в БД, а не проверкой в коде.
• Внешний сервис отвечает 20 секунд: есть ли таймаут вообще. Клиент без таймаута — это зависание навсегда.
• Злонамеренный ввод: чужой org_id в теле запроса — проверяются ли права на объект, а не только факт аутентификации. Отсутствие проверки принадлежности объекта организации — критично и встречается регулярно.
• Процесс умер между двумя записями — останется ли система в валидном состоянии.

9. Формат замечания

Каждое замечание — четыре строки, без воды:

Категории и их смысл:

10. Критерии блокировки мержа

Блокируй (Request Changes) при любом из:

1. Найдено хотя бы одно КРИТИЧНО.
2. Миграция без downgrade, либо с блокирующей операцией на большой таблице, либо несовместимая со старым кодом во время выката.
3. Секрет в диффе.
4. Публичная ручка без проверки прав или вебхук без проверки подписи.
5. Изменение денежной логики без теста на числовой пример.
6. Правка в автогенерённом файле вместо источника генерации.
7. Дифф больше 1000 строк без объяснения, почему его нельзя разбить.
8. Тесты не запускаются или падают.

Не блокируй за: стиль, который ловит линтер; отсутствие рефакторинга за пределами диффа; несогласие с уже принятым и зафиксированным архитектурным решением; предпочтения по именам, если имя не вводит в заблуждение.

Needs Discussion — дефекта нет, но подход создаёт долг, который дешевле обсудить сейчас, чем переделывать.