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

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

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

Три точки (...) — дифф от точки ветвления; две точки подмешают чужие коммиты из base — главная причина обсуждать код, которого автор не писал. Исключённые файлы всё равно проверь по --stat: новая зависимость в package-lock.json / poetry.lock, автогенерённый файл, переписанный руками.

До чтения:

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

2. Пороги

Внимание не масштабируется: на 1000+ строках находят примерно столько же, сколько на 300. Миграции БД, права доступа и денежные расчёты ревьюются полностью при любом размере PR.

Порядок чтения — по стоимости ошибки: миграции и схема БД → контракты (публичные API, сериализаторы, схемы вебхуков) → деньги и права → бизнес-логика → тесты (последними, но обязательно: показывают, что автор считал важным) → конфиги, CI, зависимости → UI.

3. Каталог сигнатур: греп по диффу, каждое попадание — глазами

3.2 Деньги во float

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

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

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

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

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

5.2 Вопросы автору миграции

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

6. Автогенерённый код

Выглядит увереннее рукописного, поэтому ревьюется хуже. Признаки:

• Несуществующие поля/методы внешнего API (order.delivery_date, client.get_stocks_v3) — самое частое; проверяй каждый внешний вызов по документации, не по правдоподобию.
• Несуществующие пакеты. Новая строка в requirements.txt / package.json: пакет существует, имя не в одной букве от популярного, дата публикации не вчерашняя. Опечатка в имени — установка чужого кода.
• Дубли. Ищи грепом существующую утилиту перед одобрением нового хелпера.
• Тесты, повторяющие реализацию: сверка с той же формулой, что внутри функции, не проверяет ничего.
• Пустые try/except вокруг каждой строки — см. 3.5.
• TODO без адресата, комментарии об очевидном — не дефект, но маркер наименее читанного кода.
• Правка в автогенерённом файле (*.generated.*, клиент из OpenAPI) — блокируй: исчезнет при следующей генерации; правится генератор или схема.

«Тесты зелёные» здесь слабый сигнал — тесты часто сгенерированы тем же проходом; проверь мутацией (раздел 7).

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

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

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

8. Adversarial-проход

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

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

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

Четыре строки, без воды:

Категории:

• КРИТИЧНО — потеря/порча данных, утечка, дыра в правах, отказ сервиса, необратимая миграция. Блокирует мерж.
• ВАЖНО — проявится на реальных объёмах или в редком сценарии; отсутствие теста на новую логику. Исправить до мержа или задача с явным сроком.
• ПРЕДЛОЖЕНИЕ — читаемость, упрощение, производительность без доказанного влияния. Автор вправе не принять.
• ВОПРОС — не понял намерение. Спрашивай вместо правки по догадке.

Пропорции: больше двух КРИТИЧНО на средний PR — не расписывай остальное, сначала критичные. Больше десяти ПРЕДЛОЖЕНИЙ — придираешься, оставь три самых полезных.

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

Request Changes при любом из:

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

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

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