~/frelikh
← Все статьи
RU EN

Ревью AI-кода, где бага никогда нет там, куда смотришь

У AI-кода нет следов борьбы, и это переворачивает то, как ревьюер ищет баги. Какие ошибки он повторяет и куда тратить дефицитное внимание.

Самый чистый pull request, что я ревьюил этой весной, держал свой баг в самой безобидной строке. Это была ручка удаления файла, строк сорок, и коллега сгенерировал бо́льшую её часть ассистентом. Диф читался как документация: типизирован насквозь, аккуратно поименован, рядом лежал зелёный тест. Я едва не заапрувил его с первого прохода. Проверка прав стояла прямо наверху, и та часть, на которой я обычно притормаживаю, выглядела закрытой.

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

// выглядит как проверка прав. она и есть проверка прав. просто охраняет не то.
async function deleteFile(fileId: string, user: User) {
  const file = await files.get(fileId);
  if (!file) throw new NotFound();
  if (!user.canDelete) throw new Forbidden();  // аккаунтная возможность, а не «этот файл»
  await files.remove(fileId);                  // file.ownerId в решение вообще не входит
}

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

Подсказки больше нет

Я отревьюил много кода, и годами мои инстинкты были настроены на человека-автора. Люди сигналят, где их баги. Функция, с которой кто-то мучился, выглядит измученной: комментарий, спорящий сам с собой, переменная, переименованная дважды, // TODO: вернуться, сообщение коммита из одного слова «наконец». Учишься притормаживать там, где притормозил автор, и чаще всего это работает.

Модель пишет сложную часть и тривиальную одним ровным голосом. Сомнения, которое можно прочесть, нет, и след, по которому я шёл, пропал. Баг не там, где автор боролся, потому что в тексте никто не боролся; он там, где правдоподобный код случайно неверен, а это обычно та часть, что выглядит самой законченной. Старая эвристика, читай мутные места внимательно и доверяй чистым, теперь работает наоборот. Чистые я читаю первыми.

В каких формах приходят промахи

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

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

API и пакеты, которых не существует. Модель пишет под усреднение всех версий всех библиотек, а не под ту, что в вашем lock-файле. Поэтому она зовёт метод, переименованный два мажора назад, или импортирует пакет, чьё имя звучит правдоподобно и отдаёт 404 на npm. Обе формы реальны, и обе из тех, что машина ловит лучше ревьюера, к чему я ещё вернусь.

// дебаунс для проверки доступности домена. имя пакета звучит правдоподобно,
// но отдаёт 404 на npm — выдумано по аналогии с реальными вроде `p-debounce`.
import { debounceAsync } from 'debounce-async-promise';

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

// безупречно, если печатать медленно. печатай быстро — и медленная проверка «ma»
// может прийти позже быстрой «mail», вернув полю «доступно» для имени, которое
// пользователь уже дописал. устаревший запрос никто не отменяет.
useEffect(() => {
  checkDomain(name).then(setAvailable);
}, [name]);

Код, который ложится в общий паттерн, но не в этот кодбейс. Он тянется к fetch и рукописному try/catch, когда в трёх импортах есть типизированный клиент, уже умеющий ретраи, авторизацию и нормализацию ошибок. Он заново выводит селектор, который уже существует. Каждая строка локально разумна, а в кодбейсе становится на один способ делать то же самое больше.

Оверинжиниринг. Попроси джуна про разовую штуку — обычно получишь что-то слишком простое. Попроси модель — получишь фабрику, конфиг-объект, интерфейс стратегии и точку расширения, которую никто не просил. Она оптимизирует под «выглядит как зрелый код», потому что в обучении награждали за это, а не под наименьшее, что решило бы задачу. И джуна, и модель надо разворачивать, просто в разные стороны.

Тонкая логика на границах. Off-by-one, включающий диапазон там, где должен быть исключающий, >= вместо >. Их трудно заметить ровно потому, что код вокруг достаточно опрятен, чтобы глаз с него соскальзывал.

Ревью PR, который не прочитать построчно

Модель выдаёт PR на 900 строк за день, и все 900 выглядят готовыми. Читай каждую строку с равным весом — и бо́льшая часть внимания уходит на бойлерплейт, пока единственная важная строка проскальзывает мимо. Поэтому я перестал читать такое сверху вниз.

Я стартую от замысла. Что это должно было делать и где на самом деле живёт корректность? Для ручки удаления это решение об авторизации и то, что она читает, чтобы его принять. Для списка — ветки пустоты, загрузки и ошибки плюс ключи. Эти несколько мест я читаю внимательно, а остальное просматриваю, чтобы убедиться, что оно настолько же скучное, насколько выглядит. Часто я сперва выписываю в описание PR три-четыре инварианта, которые обязаны держаться, и иду искать каждый в коде. Если не нахожу, где инвариант обеспечивается, это отсутствие обычно и есть баг, и найти его так быстрее, чем вычитывать всё подряд.

Тесты — это гейт, но с подвохом, который большинство упускает: тест, написанный той же моделью, что и код, — это тавтология. Он утверждает то поведение, которое у кода случайно есть, вместе с багом. Поэтому тест я читаю жёстче самой реализации и читаю враждебно. Трогает ли он пустой случай, устаревший ответ, неавторизованного пользователя? Если утверждается только счастливый путь, зелёный прогон говорит очень мало.

// модель написала и код, и этот тест. он зелёный — и проверяет лишь счастливый путь:
// напечатать свободное имя, ожидать «available». гонку из примера выше он не трогает,
// так что «все тесты прошли» значит «мой баг не покрыт», а не «бага нет».
test('показывает домен как доступный', async () => {
  render(<DomainField />);
  await userEvent.type(screen.getByRole('textbox'), 'mail');
  expect(await screen.findByText('available')).toBeVisible();
});

На практике утверждения пишу я, или хотя бы задаю случаи, а модель достраивает обвязку. Часть, которая говорит, что значит «правильно», должна оставаться за человеком.

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

Доверие идёт по радиусу поражения, а не по чистоте

Не весь код заслуживает одинаковой придирчивости, и опрятность — неправильная ось для калибровки. Чистая функция форматирования с property-тестом: доверю модели и пойду дальше, худший случай тут — неверная подпись. Граница авторизации, денежный поток, миграция данных, правило инвалидации кэша: не доверяю ничему, потому что худший случай тут — удалённый файл, двойное списание или данные, что неделю тихо неверны, пока никто не заметит.

Я сделал достаточно работы по биллингу и подпискам, чтобы получить вечный рефлекс на денежные пути. У модели рефлекса нет; логику возврата средств она пишет с тем же спокойствием, что и цвет кнопки. Поэтому я калибрую придирчивость по тому, где ошибка дорогая. Сгенерированный код тянет в обратную сторону — к тому, что выглядит самым завершённым, и вот этой тяге и стоит сопротивляться.

Вынеси повторяемые отловы в CI

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

Эти первые два гейта заодно обезвреживают форму сбоя, что звучит страшнее всех. Выдуманный импорт выглядит как риск supply chain: имя пакета, свободное сегодня, — тайпсквот, который ждёт своего часа. Но из ошибок модели его отловить дешевле всего: метод, которого нет, валит компиляцию, пакет, которого нет в lock-файле, валит установку, и замечать это человеку не нужно. Самая тревожная на слух категория — та, которой машина уже владеет.

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

Самый плодовитый джун, что у тебя будет

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

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

Под всем этим есть напряжение, которое я не разрешил, и, думаю, инструментарий тоже. Генерация подешевела, а ревью — нет. CI забирает механическую половину: выдуманные импорты, недостающие типы. Дорогая половина — тот ли это субъект авторизации, стоит ли эта абстракция своего веса, что будет, если на входе пусто — это ровно та половина, которую инструмент не может рассудить, и дешевле она не становится оттого, что код пришёл быстрее. Лёгкий код уезжает раньше, а узкое место сползает на те несколько решений, что и были настоящей работой. В спокойный день я даю сгенерированному PR полный разбор. В день, когда их шесть, я триажу по радиусу поражения и мёржу что-то, что год назад вычитал бы жёстче, и отдаю себе в этом отчёт. Делать вид, будто суждение масштабировалось вместе с генерацией, — это и есть способ мёржить быстро сейчас и платить потом.

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

Что я проверяю, прежде чем заапрувить AI-PR

Меня всё ещё проводят. Пару недель назад сгенерированный PR обернул вполне разумный с виду ретрай вокруг вызова, который не был идемпотентным, и это пролежало в проде день, пока двойные записи не всплыли в обращениях в поддержку. В коде, который написал бы сам, я бы вспомнил, что вызов небезопасно повторять; в чужом чистом дифе не догадался спросить. Цель никогда не была в нуле промахов, я промахивался и когда код был целиком человеческий. Она в том, чтобы держать промахи подальше от дорогих мест и не давать опрятному дифу уговорить себя ему довериться. Свой баг модель не подсветит, так что эта часть работы остаётся моей.