Перейти к содержанию

Чеклист ревьюера PR

Пробегай этот список, когда смотришь чужой PR. Не всё применимо к каждому изменению — пропускай пункты, которые к PR не относятся. Но ни один пункт не пропускай «на доверии».

Scope

  • PR сфокусирован. Одна логическая правка. Если в diff'е «заодно рефакторинг» — попроси вынести в отдельный PR.
  • Связь с задачей. В описании PR есть ссылка на issue/ticket (если есть trackers) и ответы на «зачем / что сделано / как тестировать».
  • Размер. Не более 400 изменённых строк (без сгенерированных). Больше — попроси декомпозицию, если только PR-описание не объясняет убедительно, почему неизбежно.

Тесты

  • Новое поведение покрыто тестами. Service — unit, repository — integration (testcontainers), handler — через httptest, Watermill — через gochannel.
  • Race detector. В Makefile и CI go test -race -count=1 ./....
  • Нет time.Sleep в тестах. Для async используется eventually или канал.
  • Моки не over-specified. Тест не ломается от любого рефакторинга, проверяет только то, что действительно важно.
  • t.Parallel() применён там, где тест не зависит от shared state, и с tc := tc в table-driven.

Стиль кода

  • golangci-lint в CI зелёный.
  • Именование консистентно: userID (не userId), HTTPClient (не HttpClient), sentinel-ошибки ErrXxx, конструкторы NewXxx.
  • Нет panic в прод-коде. Только в main.go/init при фатальной ошибке старта.
  • context.Context — первый параметр у всех функций, делающих I/O.
  • Ошибки обёрнуты через %w. Нет fmt.Errorf("error: %s", err).
  • Нет глобальных переменных кроме sentinel-ошибок.
  • Нет TODO/FIXME без ссылки на issue.

Безопасность

  • Новые секреты в .env.example как placeholder, не в коде, не в тестах, не в compose.
  • Валидация input на handler-boundary (validator теги, MaxBytesReader, DisallowUnknownFields).
  • Нет fmt.Sprintf в SQL с user input. Только $1, $2 через pgx.
  • PII не попадает в логи. Email/phone/token — замаскированы (pii.MaskEmail и т.п.) или не логируются.
  • Error-сообщения клиенту generic. Нет stack trace, нет DSN, нет полного err.Error() в ответ.
  • Новый /internal/* endpoint — под InternalToken middleware (subtle.ConstantTimeCompare).
  • Cookies/session — SameSite=Strict; Secure; HttpOnly. Если применимо.

База данных

  • Миграция имеет up + down файлы.
  • pg_advisory_xact_lock в начале up.sql (и down.sql если в транзакции).
  • Expand-contract при изменении существующего столбца/типа. Не combo «add + rename + drop» в одной миграции.
  • Индексы под новые WHERE/ORDER BY. CREATE INDEX CONCURRENTLY для hot-таблиц (вне транзакции).
  • EXPLAIN ANALYZE прогнан для новых запросов с индексами — результат упомянут в PR-описании.
  • Таблицы во множественном числе, есть created_at/updated_at.
  • Repository не возвращает pgx.ErrNoRows наружу — транслирует в pkgdb.ErrNotFound.

События

  • Publisher пишет в outbox, не напрямую в Kafka. Outbox-запись в той же транзакции, что и бизнес-write.
  • Consumer идемпотентен. UPSERT / unique constraint / Deduplicator.
  • Envelope заполнен через helper (eventmeta.New) — все обязательные поля на месте (Event-Type, Schema-Version, Correlation-Id, Source-Service, Published-At, traceparent).
  • Middleware stack применён: CorrelationID → Recoverer → PoisonQueue → Retry → Deduplicator.
  • Имя топика kazmaps.<service>.<entity>.<action>, action в прошедшем времени.
  • Payload без PII в открытом виде.

HTTP API

  • Версионирование: путь /v1/* для публичного, /internal/* для сервис-к-сервису.
  • Response shape стандартный: {"data":...} на успех, {"error":{"code","message","request_id"}} на ошибку.
  • Валидация на handler-уровне (validate(req) после decodeJSON).
  • Маппинг sentinel в HTTP — через mapServiceError, не хардкодом.
  • Content-Type проверен на POST/PUT/PATCH (415 иначе).
  • Endpoint в OpenAPI (если сервис поддерживает api/openapi.yaml).
  • Таймаут handler'а: Timeout middleware активен.
  • Пагинация cursor-based на публичных list endpoint'ах.

Docker

  • Dockerfile не регрессировал до root: USER app (uid 10001) остался.
  • Multistage build сохранён. CGO_ENABLED=0 если не нужен cgo.
  • -ldflags="-s -w" на builder-стадии.
  • Healthcheck в compose для postgres/redis/kafka, и сервис с depends_on: condition: service_healthy.

Конфиг

  • Новые env добавлены в .env.example с dev-placeholder.
  • Новые env загружаются в internal/config/.
  • Валидация на старте: fail-fast, если обязательное поле пустое.
  • README сервиса обновлён, если добавились env, которые надо знать разработчику.

Коммиты

  • Conventional commits: <type>(<scope>): <imperative>.
  • Нет --no-verify в истории. Если pre-commit hook падал — проблему починили.
  • Нет секретов в diff. Проверь глазами: JWT, DB-пароли, API-ключи. Даже закомментированные.
  • Нет мусора: bin/, .idea/, node_modules/, случайных файлов с локальной машины.

Документация

  • Публичный API изменился → обновлён README сервиса и/или api/openapi.yaml.
  • Новая доменная концепция → обновлён ../glossary.md.
  • Новый паттерн, которого нет в handbook → обсудить с автором, не вынести ли в conventions/. Не блокировать PR из-за этого, но записать задачу.

Поведение при отказе

  • Graceful shutdown не сломан: SIGINT/SIGTERM → ctx.Done() → srv.Shutdown с таймаутом → все long-running goroutine завершаются.
  • Таймаут на внешние вызовы: все HTTP-клиенты под ctx или с явным Timeout. Никаких «вечных» вызовов.
  • Retry там, где ошибка транзиентная. Без retry там, где ошибка бизнес-логическая (400, 409 — не retry).
  • Circuit breaker для внешних paid провайдеров (SMS gateway, push provider). Чтобы один сбойный провайдер не съел всех goroutine.
  • Kafka consumer при panic поднимает ошибку через Recoverer, а не валит процесс.

Как оставлять комментарии

Категоризируй комментарии — экономит всем время:

  • nit: — косметическая правка (пробел, порядок строк, стиль). Автор может игнорировать, reviewer может approve при остатках.
  • suggestion: — альтернативный подход, который, возможно, лучше. Автор решает, применять или нет.
  • question: — «я не понял, что тут происходит». Автор объясняет или правит код так, чтобы вопрос отпал.
  • issue: — блокер. Без фикса не approve. Пиши что не так и почему это проблема (не «плохо написано», а «здесь возможен NPE, если repo вернёт nil»).

Комментарий — конструктивный: предлагай альтернативу, не просто критикуй. Если не знаешь, как лучше — формулируй как question:.

Approve

Approve только если ты лично запустил бы этот код в прод. Иначе — request changes.

  • Approve с nit: комментариями — OK, если автор их явно увидит.
  • Approve с непомеченным question: — не OK. Сначала ответ.
  • «Approve, но я не смотрел миграцию / Dockerfile / крипто» — не OK. Либо смотри всё, либо попроси второго ревьюера на те части, которые вне твоей экспертизы.

Что не проверять

  • Не комментируй форматирование — это работа gofmt и golangci-lint. Если линтер зелёный, а код неудобен — issue на линтер, не на PR.
  • Не комментируй bikeshed: имена переменных длиной 3 vs 4 символа, if vs switch на двух ветках. Автор знает контекст лучше.
  • Не проси тесты на тривиальные геттеры / wiring в main.go. Тесты — на поведение, не на отсутствие опечаток.