Онбординг в чужой проект: ревью и рефакторинг без риска

Первый pull request в чужом проекте часто ломается не из-за синтаксиса, а из-за неверной модели системы: разработчик исправляет очевидный участок, не видит скрытые зависимости и получает длинное ревью с замечаниями по архитектуре, тестам и стилю. Безопасный онбординг строится иначе: сначала карта проекта и его ограничений, затем маленькое изменение, после этого — отдельный рефакторинг с измеримым эффектом.

Сначала понять правила проекта, а не переписывать код

Эта схема подходит для проекта на любом стеке: конкретные команды и проверки зависят от репозитория, версии языка и CI. Перед чтением бизнес-логики найдите файлы с описанием запуска, сборки, тестов и линтеров. Посмотрите, какие проверки запускаются автоматически, а какие команда делает вручную.

• Запишите версию языка, фреймворка, базы данных и ключевых инструментов из конфигурации проекта.
• Найдите точку входа приложения: HTTP-обработчик, команду, consumer очереди или cron-задачу.
• Проверьте, где лежат тесты и какие сценарии они покрывают.
• Откройте последние принятые pull request: по ним видно допустимый размер diff, стиль сообщений и уровень детализации ревью.
• Спросите владельца компонента о границах изменений: что нельзя менять в рамках первой задачи и какие интеграции считаются критичными.

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

Три рабочих подхода к первой задаче

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

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

Хороший первый pull request доказывает не то, что вы умеете переписать модуль. Он показывает, что вы умеете менять его границы, не нарушая договорённости команды.

Как читать код перед ревью

Начните с контракта

Сначала ответьте на четыре вопроса: что приходит на вход, какой результат считается успешным, какие ошибки видит вызывающий код и какие побочные эффекты происходят. Контракт может быть описан типами, тестами, схемой API, миграциями или только поведением в соседних модулях.

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

Отделите обязательное от привычного

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

В ревью полезно указывать последствие, а не просто желаемую форму:

• «Здесь ошибка внешнего сервиса превращается в успешный пустой ответ. В результате вызывающий код не сможет отличить сбой от отсутствия данных».
• «Проверка выполняется после записи в базу. Если следующий шаг завершится ошибкой, повторный запуск создаст дубликат».
• «Этот запрос выполняется внутри цикла. При нескольких связанных объектах число обращений к базе растёт вместе с размером списка; стоит проверить план и журнал запросов».
• «Это предложение относится к стилю. Если форматтер уже исправляет такой случай, ручная правка не нужна».

Как делать рефакторинг, который можно принять

Рефакторинг — это изменение внутренней структуры без изменения наблюдаемого поведения. На практике это утверждение нужно подтвердить тестами, логами или другим способом проверки, иначе под видом уборки легко протащить новую бизнес-логику.

1. Сформулируйте границу. Назовите один модуль, один поток или одну проблему: длинная функция, смешение доступа к данным и правил расчёта, дублирование преобразования. Если граница звучит как «привести проект в порядок», её ещё нет.
2. Зафиксируйте поведение до изменения. Запустите существующие проверки и добавьте тест только для риска, который иначе не виден. Запишите вход, ожидаемый результат и ошибки, которые нельзя менять.
3. Меняйте структуру небольшими шагами. Сначала выделите функцию или объект, затем переименуйте, затем удалите дублирование. Не меняйте одновременно формат ответа, схему базы и способ вызова.
4. Сравните результат с исходным поведением. Проверьте тесты, статический анализ и сценарий, ради которого началась работа. Если есть метрики, сравните их на одинаковых данных и при одинаковой нагрузке.
5. Оформите отдельный diff. Уберите автоматические перестановки строк, массовое форматирование и несвязанные исправления. Ревьюер должен увидеть смысл изменения, а не шум.

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

Как проводить ревью pull request новичка

Ревью — не проверка того, похож ли код на код ревьюера. Его задача — найти риск до слияния и передать контекст, которого не хватает автору. Поэтому комментарии стоит делить по приоритету.

• Блокер. Ошибка меняет контракт, нарушает безопасность, портит данные или ломает обязательный сценарий. Такой комментарий должен содержать условие, последствие и способ проверки.
• Существенное замечание. Решение трудно поддерживать или оно создаёт заметный риск, но проблему можно вынести в отдельную задачу, если команда согласна.
• Неблокирующее предложение. Вариант упрощения или вопрос о стиле. Его нельзя маскировать под обязательное требование.

Не просите «переписать всё красивее». Лучше спросить: «Какой контракт у этого метода при тайм-ауте?» или «Можно ли проверить этот случай тестом до слияния?». Такие вопросы помогают автору понять систему, а не только исправить конкретную строку.

Что ломается чаще всего

Рефакторинг смешан с изменением поведения

В одном diff меняются имена, SQL-запрос, формат ответа и обработка ошибок. В итоге ревьюер не может понять, что было намеренным, а что появилось случайно. Разделите изменения или хотя бы перечислите их в описании pull request.

Автотесты проходят, но сценарий пользователя сломан

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

Команда спорит о стиле вместо риска

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

Нет владельца решения

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

Когда выбирать каждый подход

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

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

Что делать дальше

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

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

Сноска. Pull request — запрос на включение набора изменений в основную ветку репозитория. В разных системах он может называться merge request, но смысл тот же: изменения проходят проверку до слияния.