Модуль 5. Code Review: проверка инженерных решений и развитие команды

Введение

Основной тезис модуля

Code review — это не поиск запятых, не проверка знания style guide и не церемония получения разрешения на merge.

Code review — это независимая проверка инженерного изменения другим субъектом. Reviewer восстанавливает замысел изменения по его описанию и реализации, проверяет основания решения и последствия для системы.

Слабый review оценивает отдельные строки и личные предпочтения reviewer.

Сильный review устанавливает, сохраняет ли изменение требуемые свойства системы и можно ли обосновать его принятие.

Для лида code review имеет ещё одну функцию: инженерное различение не должно оставаться индивидуальной способностью одного сильного reviewer. Основания решений должны становиться видимыми и воспроизводимыми внутри команды.

Главный вопрос модуля:

Что должен увидеть reviewer, чтобы обоснованно принять или отклонить изменение?

Объект review

Pull request показывает diff, но объект review не равен diff.

За добавленными и удалёнными строками находится изменение системы:

замысел → решение → код → изменение поведения → последствия

Reviewer движется через эту конструкцию в обратную сторону: от реализации восстанавливает принятое решение, его основания и то, какое поведение системы из него следует.

Если reviewer видит только синтаксис, он может оценить форму кода, но не само инженерное изменение.

1. Сначала восстановить замысел изменения

Reviewer не может оценить решение, не понимая его цели.

До чтения отдельных строк нужно установить:

какое поведение меняется;
почему изменение необходимо;
какие ограничения нельзя нарушить;
что сознательно не входит в scope;
как будет проверен результат;
какой уровень риска у изменения.

Минимальное описание pull request

Чтобы reviewer мог восстановить замысел изменения, описание pull request должно содержать необходимый для этого контекст:

Проблема: что требует изменения.

Изменяемое поведение: что именно должно начать происходить иначе.

Выбранное решение: каким способом реализовано изменение.

Ключевые ограничения: какие свойства и условия должны быть сохранены.

Неочевидные компромиссы: чем пришлось пожертвовать или какую цену принять при выборе решения.

Способ проверки: как установить, что требуемое изменение действительно произошло.

Deployment / migration / rollback: что необходимо учитывать при развёртывании, изменении данных и откате.

Связанный контекст: требования, ADR, инциденты или другие материалы, из которых следует это изменение.

Описание не должно пересказывать diff. Оно должно дать reviewer модель изменения, относительно которой можно проверять код.

Если замысел не удаётся восстановить

Невозможность восстановить замысел уже является результатом review.

Если автор не может точно объяснить, какое поведение изменяется, или PR содержит несколько несвязанных целей, reviewer не обязан угадывать замысел по коду. Нужно остановиться и прояснить границу изменения.

Плохой комментарий:

Непонятный PR.

Точный комментарий:

Сейчас PR одновременно меняет правила расчёта тарифа, формат API и механизм кеширования. Я не могу независимо проверить причинность этих трёх изменений и понять, какое из них создаёт наблюдаемое поведение. Давай разделим изменение либо явно опишем, почему они образуют одну неделимую транзакцию.

2. Уровни code review

Сильный review движется от смысла и риска к деталям. Если сначала потратить внимание на naming и стиль, можно не заметить, что само изменение решает не ту задачу или находится не в той границе системы.

Уровень 1. Необходимость и соответствие задаче

Решает ли изменение заявленную проблему?
Соответствует ли изменяемое поведение требуемому результату?
Не добавляет ли изменение лишний scope?
Не существует ли уже нужной способности в системе?

Уровень 2. Корректность поведения

Правильно ли работает основной сценарий?
Что происходит на границах входных данных?
Что происходит при повторе, конкурентном вызове и частичном отказе?
Не нарушается ли существующее поведение?

Уровень 3. Дизайн и архитектурные границы

Находится ли ответственность в правильном модуле?
Соответствует ли модель предметной области?
Не создаётся ли лишняя связанность?
Не протекает ли инфраструктурная деталь в бизнесовую логику?
Не дублируется ли правило в нескольких местах?

Уровень 4. Данные и состояние

Как меняется состояние?
Какие переходы между состояниями допустимы?
Где проходит транзакционная граница?
Что происходит при повторной доставке?
Как обеспечиваются совместимость и миграция данных?

Уровень 5. Надёжность и эксплуатация

Что произойдёт при timeout?
Что происходит при необходимости повторить действие?
Можно ли безопасно откатить изменение?
Как отказ будет обнаружен в production?
Не создаётся ли бесконечный retry или лавина запросов?

Уровень 6. Безопасность и приватность

Кто имеет право выполнить действие?
Проверяется ли подлинность входящих данных и запросов?
Не раскрываются ли чувствительные сведения?
Можно ли обойти проверку через другой путь?
Какая новая поверхность атаки появляется?

Уровень 7. Производительность и масштабирование

Какова вычислительная и ресурсная стоимость операции?
Не появился ли N+1?
Не загружается ли неограниченный объём данных?
Как изменение ведёт себя на production-нагрузке?
Какой ресурс становится ограничением при росте нагрузки?

Уровень 8. Тесты

Доказывают ли тесты требуемое поведение?
Проверяют ли они границы и отказные сценарии?
Не закрепляют ли тесты внутреннюю реализацию вместо контракта?
Сломается ли тест при реальной регрессии?

Уровень 9. Понятность и сопровождаемость

Можно ли восстановить причинность решения из кода?
Не скрыта ли сложность за слишком общей абстракцией?
Соответствуют ли имена предметному смыслу?
Нужно ли сохранить причину решения в ADR или комментарии?

Уровень 10. Стиль

Соответствует ли код установленным и автоматизируемым правилам?
Создаёт ли локальное отклонение реальную стоимость чтения и сопровождения?
Или reviewer просто предпочитает другую форму записи?

Google Engineering Practices также предлагает проводить review от общего к частному: сначала оценивать дизайн и функциональность изменения, а затем переходить к сложности, тестам, naming, комментариям, стилю и документации. (Google: What to Look For)

3. Review должен быть риск-ориентированным

Не каждый pull request требует одинаковой глубины review.

Глубина проверки зависит не от размера diff самого по себе, а от того, какие свойства системы затрагивает изменение и к каким последствиям может привести ошибка. Однострочное изменение условия авторизации может быть опаснее нового внутреннего класса на триста строк.

Факторы риска

критичность пользовательского сценария;
деньги, права доступа или персональные данные;
необратимое изменение данных;
публичный API или межкомандный контракт;
конкурентность и распределённое состояние;
сложность rollback;
blast radius;
недостаточная наблюдаемость;
новая технология или зависимость;
отсутствие тестового окружения;
изменение старого кода с неявными контрактами;
объём неизвестности.

Условная матрица

Риск	Пример	Режим review
Низкий	Локальный рефакторинг под существующими тестами	Один компетентный reviewer, автоматические проверки
Средний	Новый endpoint внутри существующего домена	Проверка контракта, ошибок, security, тестов и observability
Высокий	Платёж, миграция данных, авторизация, межсервисный протокол	Предварительный design review, профильные reviewers, план rollout и rollback

Риск определяет не только глубину review, но и то, какие компетенции, проверки и дополнительные контуры принятия решения необходимо подключить.

4. Отличать проблему от вкусовщины

Одна из главных обязанностей лида — не позволить личному предпочтению маскироваться под инженерную необходимость.

Если reviewer требует изменить код, у этого требования должно быть основание. Можно различить несколько основных типов.

Нарушение корректности

Изменение создаёт неверное поведение относительно требования или контракта.

При повторной доставке события баланс будет уменьшен второй раз, потому что обработчик не проверяет идентификатор операции.

Здесь основанием является конкретное нарушение требуемого поведения.

Нарушение установленного стандарта

Изменение нарушает явное командное или организационное правило.

Публичные endpoint должны проверять authorization policy X. Это закреплено в security standard и используется соседними обработчиками.

Здесь требование reviewer основано не на его личном предпочтении, а на уже принятом стандарте.

Материальный риск

Ошибка ещё не произошла, но существует понятный механизм отказа и значимое последствие.

Этот запрос не имеет timeout. При деградации партнёра рабочие потоки будут удерживаться без верхней границы, что может исчерпать pool всего сервиса.

Reviewer должен уметь назвать не только возможную проблему, но и механизм, через который она возникает.

Архитектурный аргумент

Код может корректно выполнять текущий сценарий, но размещение ответственности или форма связи увеличивает стоимость изменения системы.

Правило доступности тарифа теперь продублировано в controller и domain service. Следующее изменение потребует синхронно менять два пути, поэтому правило должно иметь одного владельца.

Архитектурное замечание требует такой же причинности, как замечание о непосредственной ошибке. Само утверждение «так архитектурно правильнее» основанием не является.

Вкусовое предпочтение

Оба варианта допустимы, а различие между ними не связано с нарушением требования, стандарта, свойства системы или материальным последствием.

Я бы записал это через stream.

Такое предпочтение не должно превращаться в обязательное требование только потому, что reviewer имеет право approve.

Проверка основания

Перед требованием переделать код reviewer должен установить:

какое свойство системы нарушено или подвергается риску;
какой механизм создаёт негативное последствие;
существует ли установленное командное или организационное правило;
отличается ли реальная стоимость рассматриваемых вариантов;
готов ли reviewer одобрить код, если автор сохранит текущий вариант.

Последний вопрос проводит важную границу. Если reviewer готов одобрить текущий вариант, замечание не должно выглядеть как обязательное условие merge.

Если же изменение действительно недопустимо, reviewer должен явно назвать основание и не заставлять автора угадывать, является комментарий требованием или необязательным обсуждением.

5. Язык комментария: наблюдение, причинность, последствие, действие

Сильный review-комментарий обычно содержит четыре элемента.

Наблюдение: что именно reviewer видит в коде.

Причинность: какой механизм связывает наблюдаемую конструкцию с проблемой или риском.

Последствие: какое свойство системы нарушается или оказывается под риском.

Действие: что необходимо изменить, прояснить или решить.

Эти элементы позволяют отделить инженерное требование от личного указания. Автор видит не только то, что reviewer хочет изменить, но и почему изменение необходимо.

Полезные метки

Помимо основания комментария важно сделать явным его статус. Команда может договориться о семантике меток:

Метка	Значение
`[blocker]`	Без разрешения проблемы merge недопустим
`[risk]`	Обнаружен механизм возможного отказа; необходимо принять решение
`[question]`	Reviewer запрашивает контекст; это не скрытое требование переделки
`[suggestion]`	Возможное улучшение, не являющееся условием текущего merge
`[nit]`	Незначительная полировка, полностью необязательная
`[follow-up]`	Проблема реальна, но её исправление может быть вынесено в отдельное явно зафиксированное изменение

Google в своём стандарте review также предлагает явно помечать незначительные необязательные замечания, чтобы автор не принимал полировку за условие approval. (Google: Standard of Code Review)

Метка показывает статус комментария, но не создаёт его основание.

[blocker] Мне так не нравится остаётся вкусовщиной, только снабжённой административной силой.

Пример

Плохо:

Переделай, так некрасиво.

Точно:

Здесь в одном методе смешаны валидация входа, применение бизнесового правила и вызов внешнего сервиса.

Из-за этого бизнесовое правило оказывается связано с controller-логикой, а ошибка внешнего сервиса обрабатывается внутри той же конструкции, что и domain validation.

При повторном использовании правила придётся воспроизводить эту связь или вызывать transport-логику из другого контекста.

[blocker] Нужно разделить transport-валидацию, бизнесовое правило и внешнюю интеграцию так, чтобы domain-правило не зависело от controller и вызова партнёра.

Здесь комментарий показывает всю причинную цепочку:

наблюдение → механизм → последствие → требуемое изменение.

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

Развёрнутая причинность нужна там, где без неё требование превращается в личное указание reviewer: автор видит требование переделать код, но не видит свойства системы, ради которого изменение необходимо.

6. Как объяснять trade-offs

Инженерное решение почти никогда не является выбором между абсолютным добром и абсолютным злом. Обычно разные варианты по-разному распределяют стоимость между несколькими свойствами системы:

скорость реализации;
простота текущего решения;
расширяемость;
производительность;
надёжность;
согласованность данных;
операционная сложность;
обратимость;
стоимость владения;
cognitive load команды.

Фраза «это плохое решение» скрывает саму структуру выбора. Reviewer должен показать, что именно даёт выбранный вариант, какую цену он создаёт и почему эта цена допустима или недопустима.

Формула разбора компромисса

Контекст: какую проблему решаем и какие ограничения действуют?

Варианты: какие реальные альтернативы существуют?

Выигрыш: что даёт текущий вариант?

Цена: какое свойство ухудшается?

Горизонт: когда и при каких условиях эта цена проявится?

Обратимость: насколько трудно будет изменить решение позже?

Решение: почему в данном контексте эта цена допустима или недопустима?

Пример

Слабый комментарий:

Здесь обязательно нужен Kafka.

Точный комментарий:

Синхронный вызов оставляет реализацию простой и даёт вызывающей стороне немедленный результат.

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

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

Какое из этих требований действует здесь?

Так reviewer не проталкивает знакомую технологию, а делает видимыми выигрыш и цену каждого варианта и возвращает выбор к требованиям системы.

7. Архитектурные запахи, видимые в review

Архитектурный запах — не доказанная ошибка, а локальный признак того, что границы или зависимости системы могут быть устроены неверно.

Смешение ответственностей

Один модуль одновременно разбирает transport, применяет бизнесовые правила, управляет транзакцией и вызывает внешние системы.

Дублирование политики

Одно бизнесовое правило реализовано в нескольких endpoint, consumer или сервисах. Текст кода различается, но смысл должен изменяться совместно.

Протекание границы

Внутренняя модель базы данных становится публичным API, инфраструктурное исключение проходит в domain layer, внешний статус партнёра используется как внутреннее состояние без перевода.

Неявная временная связанность

Методы должны вызываться в определённом порядке, но этот порядок не выражен типами, состоянием или интерфейсом.

Общая изменяемая структура

Несколько частей системы изменяют один объект, и невозможно локально установить владельца его инвариантов.

Флаговая модель вместо состояния

Набор boolean-полей допускает сочетания, которые не соответствуют допустимым состояниям системы:

paid = true
cancelled = true
refunded = false
paymentPending = true

Если такие сочетания не должны существовать, состояние необходимо выразить через явную модель допустимых состояний и переходов между ними.

Абстракция раньше различения

Два похожих фрагмента объединяются в общий механизм до того, как установлено, совпадает ли их смысл. Следующее изменение создаёт параметры, флаги и исключения внутри «универсальной» абстракции.

Сервис без самостоятельной границы

Код вынесен в отдельный deployable unit, но данные, релизы и изменения остаются синхронно связанными с исходной системой. Распределённость добавилась, автономность — нет.

Внешний вызов внутри локальной транзакции

Локальная база и удалённая система участвуют в одном логическом действии без явного механизма обработки частичного отказа.

Архитектурный запах не означает автоматического требования применить конкретный pattern. Reviewer должен сначала установить, какое последствие создаёт обнаруженная конструкция, и только после этого обсуждать способ её изменения.

8. Как находить скрытую сложность

Скрытая сложность находится не в количестве строк, а в числе состояний, предположений и последствий, которые код должен удерживать.

Reviewer должен мысленно провести изменение через несколько осей.

Повтор

Что произойдёт, если запрос, событие или job выполнятся второй раз?

Конкурентность

Что произойдёт, если два экземпляра выполнят условие одновременно?
Может ли состояние измениться между чтением и записью?

Частичный отказ

Что уже произошло к моменту отказа следующего шага?
Можно ли продолжить, компенсировать или безопасно повторить действие?

Timeout и неопределённый результат

Если клиент не получил ответ, означает ли это, что операция не произошла?
Как будет установлено фактическое состояние операции?

Изменение порядка

Могут ли события прийти не в том порядке?
Как система отличит более старое состояние от более нового?

Совместимость

Что произойдёт, пока старая и новая версии работают одновременно?
В каком порядке могут быть развёрнуты код и миграция данных?

Объём

Что произойдёт не на десяти, а на десяти миллионах объектов?
Есть ли верхняя граница потребления памяти, размера результата и времени выполнения?

Время

Что произойдёт при смене часового пояса или переходе даты?
Что произойдёт при истечении TTL или изменении системных часов?
Что произойдёт с задержанным событием или повтором после длительной паузы?

Доступ

Может ли пользователь выполнить действие с чужим объектом?
Проверяется ли право доступа на каждом альтернативном пути выполнения действия?

Эксплуатация

Как будет обнаружено зависшее состояние?
Что увидит on-call при отказе?
Есть ли correlation ID, метрики и диагностический контекст, позволяющие восстановить происходящее?

Большинство этих рисков невозможно полностью закрыть общим checklist. Checklist задаёт оси проверки, но конкретный механизм риска определяется относительно поведения и контекста данного изменения.

9. «Работает» не равно «можно принимать»

Код может проходить тесты и при этом быть неприемлемым для merge.

Нужно различать несколько горизонтов приемлемости изменения.

Сейчас

Решение выполняет требуемый сценарий на существующих данных.

При отказе

Система сохраняет управляемое состояние при timeout, повторе и частичной недоступности.

При следующем изменении

Новую бизнесовую ветку можно добавить без синхронной правки нескольких скрыто связанных мест.

В production

Изменение можно наблюдать, ограничивать, отключать и восстанавливать.

Во времени

Команда сможет восстановить причину решения и безопасно изменить его в будущем.

Фраза «работает, не трогай» учитывает только первый горизонт. Сильный review проверяет изменение и в остальных горизонтах, не требуя при этом абстрактного совершенства.

Google формулирует стандарт review через улучшение общего code health: изменение следует принимать, когда оно определённо улучшает состояние кодовой базы, даже если оно не идеально. Review не должно останавливать продвижение ради бесконечной полировки, но и не должно пропускать постепенное ухудшение системы. (Google: Standard of Code Review)

Технический долг внутри review

Не вся существующая проблема должна быть исправлена автором текущего PR.

Reviewer должен различать происхождение проблемы и то, как текущее изменение на неё влияет.

Проблема создана текущим изменением → исправляется до merge.

Текущее изменение существенно ухудшает существующую проблему → проблема исправляется либо само изменение пересматривается до merge.

Существующая проблема лишь стала видимой при чтении кода → фиксируется отдельно, если она не блокирует корректность текущего изменения.

Текущее изменение является безопасным шагом улучшения → не блокируется ожиданием идеального конечного состояния.

Иначе review превращается в механизм, который возлагает весь исторический долг участка кода на человека, случайно открывшего файл.

10. Что должны проверять инструменты, а что — человек

Человеческое внимание дорого. Его не следует тратить на проверки, которые система способна выполнить однозначно и автоматически.

Автоматизировать

форматирование;
linting;
сборку;
выполнение unit и integration tests;
проверку типов;
формализуемые правила style guide;
поиск известных уязвимостей в зависимостях;
часть SAST-проверок;
проверку coverage-порога, если команда осмысленно его использует;
проверку совместимости контрактов и схем, где это возможно.

Оставить человеческому review

соответствие решения смыслу задачи;
качество модели;
границы ответственности;
неявные предположения;
trade-offs;
бизнесовые инварианты;
контекстные security-риски;
поведение при частичном отказе;
операционные последствия;
стоимость следующего изменения;
необходимость самого изменения.

Автоматический анализ может обнаружить потенциальную SQL injection. Но он не установит из одного только синтаксиса, что пользователь с ролью менеджера получил возможность подтвердить собственную финансовую операцию в обход разделения полномочий. Для этого необходимо понимать бизнесовые роли, допустимые действия и инварианты системы.

OWASP рассматривает manual secure code review как дополнение к SAST и DAST именно там, где необходимы понимание бизнесовой логики, data flow и контекстных уязвимостей. (OWASP Secure Code Review)

11. Защищать качество без токсичности

Токсичность review заключается не в самом несогласии и не в жёсткости технического стандарта.

Она появляется, когда административная сила reviewer подменяет инженерное основание:

комментарий направлен на свойства человека, а не на изменение;
требование не объясняется;
вкус выдаётся за обязательный стандарт;
критерии меняются после каждой итерации;
автор высмеивается за незнание;
reviewer демонстрирует превосходство вместо передачи способа мышления;
approval удерживается из-за необязательной полировки;
одинаковая проблема оценивается по-разному в зависимости от статуса автора;
дискуссия превращается в борьбу за право последнего слова.

Точность вместо унижения

Унижение:

Ты опять не понимаешь транзакции.

Сглаживание без смысла:

Возможно, если тебе будет удобно, может быть, стоит немного подумать о транзакции.

Точный review:

[blocker] Внешний вызов выполняется до commit локальной транзакции. Если commit упадёт после успешного ответа партнёра, локальная система зафиксирует отказ, а внешняя операция уже произойдёт. Нужен явный механизм согласования этих состояний.

Прямота не равна токсичности. Точный блокирующий комментарий честнее, чем вежливая формулировка, из которой автор должен угадывать требование.

Google рекомендует направлять критические комментарии на код, а не на разработчика, объяснять причину замечания и давать достаточное направление для исправления, сохраняя ответственность за решение у автора. (Google: Review Comments)

Не писать код за автора

Reviewer не обязан проектировать и реализовывать всё исправление в комментариях. Его задача состоит в том, чтобы сделать проблему, её основание и критерий приемлемого решения видимыми.

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

12. Несогласие и принятие решения

Review создаёт реальное несогласие, если два инженера по-разному оценивают последствия и цену решений.

Разногласие должно двигаться по основаниям:

какое требование действует;
какой механизм отказа предполагается;
какие данные подтверждают это предположение;
какова цена каждого варианта;
насколько решение обратимо;
какой командный стандарт уже существует;
кто владеет затрагиваемой границей.

Когда продолжать асинхронно

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

Когда переходить к разговору

Стоит переходить к синхронному обсуждению, если:

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

После разговора решение и его основание должны вернуться в PR, ADR или другой доступный артефакт. Иначе команда получает принятое решение без сохранённой причинности.

Кто принимает окончательное решение

Право принятия решения должно быть определено до возникновения конфликта:

автор владеет способом локальной реализации в допустимых границах;
владелец компонента отвечает за сохранение его инвариантов;
профильный владелец может блокировать нарушение security, data или platform standard;
межкомандное изменение требует решения владельцев затронутых контрактов;
нерешённый конфликт поднимается на уровень, где находится соответствующая ответственность.

«Лид всегда прав» не является механизмом принятия инженерного решения. Но и бесконечный консенсус без определённого владельца разрушает delivery.

13. Review как обучение младших

Review обучает не тогда, когда junior получает много замечаний. Он обучает, когда junior начинает видеть причинность, которую раньше видел только reviewer.

Передавать не только исправление, но и ось наблюдения

Слабое обучение:

Добавь retry.

Сильное обучение:

Какое фактическое состояние операции остаётся после timeout?

Если мы не знаем, произошёл ли внешний вызов, слепой retry может повторить необратимое действие.

Сначала нужно определить идемпотентность операции или способ reconciliation.

В первом случае автор получает готовое действие для конкретного места. Во втором он получает ось наблюдения: при неопределённом результате необходимо сначала установить состояние операции и только затем решать, допустим ли повтор.

Такой способ мышления можно перенести на следующую интеграцию, где конкретный механизм будет другим.

Не превращать каждый PR в учебник

Если reviewer одновременно объясняет все известные ему принципы, существенные замечания растворяются среди второстепенных.

Полезно различать:

что блокирует текущий merge;
какой принцип в этом изменении является ключевым;
что можно разобрать отдельно;
какую часть автор должен исследовать самостоятельно;
где эффективнее перейти к pairing.

Проверять перенос способа мышления

Признак обучения — не исправленная строка сама по себе, а способность автора объяснить:

какой риск существовал;
почему выбранное изменение его устраняет;
где тот же принцип может потребоваться снова.

Если автор исправил конкретное место, но при следующей аналогичной ситуации снова не видит тот же механизм риска, review передал решение, но не способ мышления.

Не снижать стандарт до уровня junior

Обучение не означает пропускать опасный код ради «самостоятельного опыта». Меняется способ сопровождения человека, а не обязательные свойства системы.

14. Review сильного, но разрушительного инженера

Сильный senior может монополизировать review:

комментировать каждый PR;
навязывать личные паттерны;
блокировать изменения без объяснения;
превращать дискуссию в экзамен;
требовать от других знания незафиксированных правил;
переписывать код за авторов;
создавать зависимость команды от своего approval.

Техническая компетентность не отменяет системного ущерба. Если после каждого review качество конкретного изменения растёт, но способность остальных инженеров самостоятельно видеть проблемы и принимать решения уменьшается, reviewer создаёт локальное качество ценой снижения способности всей команды.

Лид должен вмешиваться не общей просьбой «быть мягче», а изменением правил review:

каждый blocker должен содержать техническое основание;
вкусовые замечания не должны блокировать merge;
командные стандарты должны быть зафиксированы вне личной памяти senior;
для спорных решений должен быть определён владелец;
reviewer не должен переписывать код за автора без согласования с ним;
review должно распределяться между несколькими инженерами;
повторяющиеся замечания должны превращаться в правило, автоматическую проверку, тест или документацию.

15. Как лиду не стать bottleneck

Лид легко превращается в единственного reviewer сложных изменений. Сначала это повышает качество. Затем система начинает ждать его на каждом переходе.

Признаки bottleneck:

большинство PR требует approval лида;
очередь растёт во время его встреч или отсутствия;
другие reviewers избегают принимать окончательное решение;
авторы заранее адаптируют код к личным вкусам лида;
критическое знание не распространяется;
review начинается через несколько дней после готовности PR;
лид проверяет даже изменения низкого риска;
работа стоит, хотя в команде есть другие инженеры, способные провести review.

Распределить владение

У компонентов и областей должны быть несколько людей, способных проводить review и принимать решения в их границах.

Code ownership должен распределять ответственность и выращивать компетенцию, а не создавать пожизненную монополию одного reviewer.

Сделать стандарт внешним

Повторяющиеся требования должны переходить из головы лида во внешние механизмы:

formatter;
lint rule;
test;
checklist;
ADR;
security standard;
пример допустимого решения.

То, что можно формализовать и сохранить вне памяти конкретного человека, не должно требовать его постоянного присутствия в каждом PR.

Калибровать reviewers

Команда совместно разбирает несколько PR и сравнивает основания решений:

что каждый счёл blocker;
где обнаружилась вкусовщина;
какой риск был пропущен;
как сформулировать общий критерий.

Цель калибровки — не добиться одинаковых комментариев, а сделать воспроизводимыми основания, по которым принимаются решения.

Разделять уровни риска

Лид не обязан смотреть каждый PR. Его участие требуется там, где изменение затрагивает системную границу, критический инвариант или область, в которой ещё нет другого владельца с необходимой компетенцией.

Уменьшать размер PR

Большой PR требует более длинного непрерывного внимания и большего восстановления контекста. Малые осмысленные изменения легче передавать другим reviewers и проверять независимо.

Размер при этом не должен разрушать смысловую границу изменения: маленький PR остаётся полезным только пока представляет связное и проверяемое изменение.

Переносить дизайн раньше

Если главная дискуссия о направлении решения начинается после написания кода, review становится длинным и требует перепроектирования уже готовой реализации.

Архитектурные и системные решения с высоким риском лучше обсуждать до того, как они превратятся в большой diff. Тогда review проверяет реализацию принятого направления, а не впервые обнаруживает необходимость выбрать само направление.

Установить время реакции

Google Engineering Practices предлагает отвечать на запрос review не позднее одного рабочего дня, не требуя при этом немедленного переключения reviewer с текущей сфокусированной работы. (Google: Speed of Reviews)

Это не универсальная цифра для любой команды. Лид должен установить ожидаемый ритм review, соответствующий delivery-системе команды.

Важно, чтобы у ожидающего review был понятный владелец и чтобы PR не оставался в очереди неопределённое время.

16. Ответственность автора

Качество review зависит не только от reviewer. Автор должен сделать изменение проверяемым.

Подготовка PR

До отправки PR автор:

проверяет собственный diff;
удаляет случайный шум;
объясняет замысел и границу изменения;
связывает изменение с требованием или принятым решением;
показывает способ проверки;
обозначает рискованные части изменения;
указывает необходимые migration, rollout и rollback;
не скрывает неизвестность;
разделяет несвязанные изменения;
отвечает на комментарии через понимание проблемы и изменение решения, а не через механическое исправление указанной строки.

Self-review

Автор должен прочитать собственный diff как чужой:

все ли изменения относятся к заявленной цели;
что reviewer не сможет понять без дополнительного контекста;
где принято неочевидное предположение;
какие ошибки автор искал бы в таком же изменении другого инженера;
как изменение ведёт себя при отказе;
какие временные или отладочные элементы остались в diff.

Self-review не заменяет независимую проверку другим reviewer, но убирает шум и освобождает его внимание для существенного анализа.

17. Размер и конструкция pull request

PR должен быть не просто маленьким, а осмысленно ограниченным.

Признаки хорошей границы

одна объяснимая цель;
связное изменение поведения;
возможность независимо проверить результат;
отсутствие случайного рефакторинга;
явные зависимости от других PR;
понятный порядок rollout.

Механическое требование «не больше 300 строк» может разрезать целостное изменение посередине и скрыть его причинность. Но слишком большой PR тоже снижает глубину review: reviewer начинает сканировать diff вместо того, чтобы восстанавливать решение и проверять его последствия.

Подготовительные изменения

Большую feature можно разложить на последовательность самостоятельных изменений:

безопасный рефакторинг без изменения поведения;
расширение внутреннего контракта;
новый путь за feature flag;
миграция данных;
ограниченное включение;
удаление старого пути после проверки.

Каждый PR в такой последовательности должен оставлять систему в целостном и проверяемом состоянии. Основная ветка не должна становиться сломанной или требовать будущего PR для восстановления корректности.

18. Метрики code review

Метрики должны показывать поведение review-системы, а не превращаться в рейтинг reviewers или авторов.

Время до первого содержательного ответа

Показывает, сколько изменение ожидает начала review.

Автоматическое уведомление или сообщение «посмотрю позже» не является содержательным ответом: работа остаётся в очереди до момента, когда reviewer фактически начинает её разбирать.

Полное время review

Время от запроса review до approval, request changes или другого решения, после которого определяется дальнейшее движение PR.

Само значение полезно рассматривать вместе с размером и риском изменения: одинаковое время review может означать разные состояния системы для небольшого локального изменения и сложной миграции данных.

Возраст ожидающего PR

Показывает, сколько времени конкретный PR находится в ожидании review или решения.

Эта метрика делает видимыми старые элементы очереди, которые могут скрываться за приемлемым средним временем review.

Число циклов до решения

Показывает, сколько раз изменение проходит последовательность комментарий → исправление → повторная проверка до окончательного решения.

Большое число циклов может указывать на разные проблемы:

позднее изменение требований;
неясный стандарт;
чрезмерный размер PR;
недостаточный контекст в исходном описании;
выдачу новых блокирующих замечаний по одному после каждой итерации.

Количество циклов само по себе не является оценкой качества автора.

Размер PR

Размер PR можно использовать для исследования связи между объёмом изменения, временем review, числом итераций и обнаруженными после merge проблемами.

Он не должен превращаться в абсолютный норматив количества строк. Важна не только величина diff, но и смысловая граница изменения.

Распределение reviewers

Показывает, не сосредоточено ли большинство review и решений в небольшой группе людей или на одном человеке.

Так можно обнаружить bottleneck, монополию на знание или область системы, для которой команда ещё не вырастила нескольких компетентных reviewers.

Доля автоматизируемых замечаний

Показывает, какую часть человеческого review занимают замечания, которые можно проверять автоматически.

Если reviewers постоянно комментируют форматирование, imports, naming по формальному правилу или другое однозначно проверяемое условие, это основание перенести проверку в formatter, lint rule, test или другой автоматический механизм.

Дефекты, прошедшие review

После обнаружения дефекта в production полезно разбирать не только сам факт того, что он прошёл review, но и отсутствовавший способ проверки:

какой риск не был представлен;
рассматривался ли только happy path вместо модели состояний;
была ли пропущена конкурентность;
была ли пропущена проверка авторизации;
был ли неясен контракт;
отсутствовала ли необходимая observability;
какой вопрос мог бы сделать проблему видимой до merge.

Так post-incident анализ используется для изменения самой review-системы: checklist, стандарта, автоматической проверки, документации или способа рассуждения reviewer.

Метрики, которые легко разрушают смысл

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

количество комментариев reviewer;
количество найденных ошибок на человека;
доля отклонённых PR;
скорость approval без учёта риска;
количество строк, просмотренных за день.

Такие показатели стимулируют производство комментариев, поиск поводов для отклонения или поверхностные approvals вместо качественного инженерного решения.

Метрика review полезна тогда, когда помогает обнаружить ограничение или изменить систему. Если она превращается в оценку индивидуальной производительности, люди начинают оптимизировать измеряемое число вместо качества review.

Итоги модуля

Code review начинается не с оценки отдельных строк. Reviewer восстанавливает замысел изменения, проверяет его поведение и последствия, различает инженерное основание и личное предпочтение, делает риски видимыми и устанавливает, допустимо ли изменение к merge.

Лид отвечает не только за качество отдельных review. Он формирует систему, в которой:

глубина проверки соответствует риску изменения;
комментарии имеют явное инженерное основание и понятный статус;
автоматизируемые проверки не расходуют человеческое внимание;
архитектурные проблемы обсуждаются через последствия, а не через вкусовые предпочтения;
знание не остаётся собственностью одного сильного reviewer;
авторы учатся самостоятельно видеть состояния, зависимости, отказы и компромиссы;
качество review не зависит от постоянного присутствия лида.

Главное различение модуля:

Слабая review-система улучшает конкретный PR за счёт способности reviewer.

Сильная review-система превращает способность reviewer в способность команды.

Если после review автор только выполнил последовательность указаний, конкретное изменение могло стать лучше, но способ инженерного мышления остался у reviewer.

Если автор способен восстановить основание замечания, увидеть механизм риска и перенести тот же способ различения на следующее изменение, review передал не готовое решение, а инженерную способность.

В этом code review становится инструментом leadership. Лид не просто удерживает качество кодовой базы. Он делает инженерные основания решений видимыми, воспроизводимыми и доступными команде без собственного постоянного участия.