Fragment przeglądu kodu z pytaniem recenzenta o brak zamówienia i sugestią testu.

Pull request czeka drugi dzień. Recenzent zostawił kilkanaście uwag: nazwa zmiennej, inny sposób zapisania warunku, propozycja nowej abstrakcji. Autor poprawia kod, ale nadal nie wie, które uwagi blokują scalenie. W kolejnej rundzie pojawiają się następne. Tymczasem błąd, przez który użytkownik może zmienić cudze zamówienie, pozostaje niezauważony.

Code review ma pomagać zespołowi bezpiecznie zmieniać system i rozumieć jego działanie. Wymaga sprawdzenia poprawności, ale również przekazania kontekstu i uzgodnienia decyzji. Rozmowa jest środkiem do tego celu. Nie każdy PR potrzebuje długiej dyskusji; mała, dobrze przygotowana zmiana może dostać krótką akceptację po uważnym sprawdzeniu.

Co psuje code review

Najwięcej problemów zaczyna się wtedy, gdy autor i recenzent inaczej rozumieją zadanie. Autor chce naprawić pojedynczy błąd. Recenzent oczekuje przebudowy modułu. Jedna osoba traktuje pytanie jako prośbę o wyjaśnienie, druga odczytuje je jako zakaz scalenia.

  • Uwagi bez uzasadnienia. „Zmień to” nie pokazuje ryzyka ani kryterium poprawności. Autor może zastosować inną konstrukcję i zachować ten sam błąd.
  • Mieszanie poziomów. Brak autoryzacji, decyzja o granicy transakcji i literówka wyglądają w interfejsie podobnie, choć mają inne konsekwencje.
  • Przepisywanie kodu na własny styl. Recenzent zastępuje poprawne rozwiązanie swoim ulubionym, bez wskazania korzyści dla zespołu.
  • Ruchomy cel. Kolejne rundy wprowadzają nowe oczekiwania, których nie było w wymaganiach ani w pierwszym przeglądzie.
  • Akceptacja bez zrozumienia. Zielone CI i znajomy autor zastępują ocenę zachowania zmiany.

Nie naprawisz tych problemów samą listą kontrolną. Potrzebne są wspólne zasady: co sprawdzamy, kto podejmuje decyzję i kiedy zmiana jest wystarczająco dobra do scalenia.

Dobre review zaczyna się przed wysłaniem PR-a

Autor odpowiada za przygotowanie zmiany do przeczytania. Recenzent nie powinien odtwarzać celu zadania z kilkudziesięciu plików ani dopiero po uruchomieniu aplikacji odkrywać, jaki problem rozwiązujesz.

Opis powinien wyjaśniać konkretne zachowanie przed zmianą i po niej. Dopisz zakres, istotne ograniczenia oraz sposób sprawdzenia. Link do zadania pomaga, ale nie zastępuje kilku zdań, które pozwolą zrozumieć PR również za pół roku.

Problem:
Anulowanie zamówienia sprawdza jego stan,
ale nie ogranicza odczytu do bieżącego konta.

Zmiana:
Handler sprawdza uprawnienie do anulowania,
a repozytorium wyszukuje zamówienie w obrębie konta.
Brak zamówienia w tym zakresie daje odpowiedź 404.

Sprawdzenie:
- własne zamówienie w stanie pozwalającym na anulowanie;
- zamówienie innego konta i nieistniejący identyfikator;
- użytkownik bez uprawnienia;
- zamówienie, którego nie wolno już anulować.

Przed poproszeniem o review przeczytaj własny diff. Usuń przypadkowe pliki, sprawdź testy, komunikaty błędów i zgodność opisu z implementacją. Dla zmiany interfejsu dołącz widok lub krótką instrukcję odtworzenia. Dla migracji opisz zgodność ze starą wersją aplikacji oraz zachowanie przy częściowym wdrożeniu.

Mały PR ma jeden zrozumiały cel

Liczba linii jest wskazówką, a nie uniwersalnym limitem. Wygenerowany plik może być duży i łatwy do sprawdzenia. Kilkanaście linii zmieniających rozliczenia, autoryzację lub kolejność operacji może wymagać znacznie więcej uwagi.

Oddziel mechaniczne formatowanie, zmianę nazw i porządkowanie niezwiązane z zadaniem od zmiany zachowania. Jeśli naprawa wymaga wcześniejszego refaktoringu, pokaż go w osobnym, zachowującym działanie kroku. Każdy scalany PR powinien pozostawiać system w poprawnym stanie; dzielenie pracy nie usprawiedliwia wdrożenia połowy zabezpieczenia.

Przy większej zmianie uzgodnij kierunek wcześniej, na szkicu lub krótkim opisie decyzji. Wątpliwość dotycząca odpowiedzialności modułu jest znacznie tańsza przed napisaniem kilkudziesięciu klas. Draft PR pomaga pokazać postęp, ale warto jasno wskazać, czy oczekujesz już pełnego review.

Jak czytać zmianę: od zachowania do szczegółów

Zacznij od problemu i kryteriów akceptacji. Potem przejdź przez główną ścieżkę wykonania oraz sytuacje, w których założenia przestają być prawdziwe. Dopiero na tej podstawie oceniaj podział metod i nazwy.

  1. Zachowanie: czy zmiana realizuje wymaganie? Co zobaczy użytkownik, a co zapisze system?
  2. Reguły i granice: kto może wykonać operację, do jakich danych ma dostęp i gdzie sprawdzane są warunki biznesowe?
  3. Awarie i współbieżność: co stanie się przy ponowieniu, dwóch jednoczesnych żądaniach, timeoutcie lub błędzie zapisu?
  4. Kontrakty i wdrożenie: czy stara aplikacja odczyta nowe dane? Czy zmiana API, zdarzenia lub konfiguracji wymaga koordynacji?
  5. Testy i utrzymanie: czy test odróżnia poprawne zachowanie od błędnego? Czy następna osoba zrozumie odpowiedzialność kodu?

Nie każdy punkt wymaga rozbudowanej analizy w każdym PR-ze. Dobierz głębokość do ryzyka. Korekta tekstu i zmiana sposobu autoryzacji potrzebują innego zestawu dowodów. W drugim przypadku samo przejrzenie zmienionych linii zwykle nie wystarczy: trzeba również sprawdzić tożsamość użytkownika, zapytanie do bazy i obsługę błędu.

Zielone CI potwierdza, że przeszły uruchomione kontrole. Nie dowodzi, że testy obejmują właściwe przypadki ani że wymaganie zostało dobrze zrozumiane. Z kolei ręczne uruchomienie aplikacji nie zastępuje testu regresji dla błędu, który łatwo odtworzyć automatycznie.

Przykład: poprawna metoda, niepełna operacja

Załóżmy, że system obsługuje wiele kont firmowych. Użytkownik może anulować zamówienia swojego konta, jeśli ma odpowiednie uprawnienie i pozwala na to stan zamówienia. W handlerze pojawia się taki fragment PHP:

public function __invoke(CancelOrder $command): void
{
    $order = $this->orders->get($command->orderId);

    $order->cancel();

    $this->orders->save($order);
}

Kod jest krótki. Metoda cancel() może poprawnie pilnować przejść stanu, a test „anulowanie aktywnego zamówienia” przechodzić. Nadal brakuje dowodu, że operację wykonuje uprawniony użytkownik na zasobie swojego konta. Ukrycie przycisku w interfejsie i trudny do odgadnięcia identyfikator nie ustanawiają takiej ochrony.

Uwaga „dodaj walidację” jest za mało precyzyjna. Lepszy komentarz wskazuje scenariusz i oczekiwany efekt:

Blokujące — dostęp do danych. Wyszukiwanie po samym orderId pozwala pobrać zamówienie innego konta. Jeśli nie ma osobnej kontroli tej granicy, użytkownik może je anulować. Potrzebujemy sprawdzenia uprawnienia oraz ograniczenia dostępu do konta użytkownika. Dodajmy test z dwoma kontami i potwierdźmy, że cudze zamówienie pozostaje bez zmian.

Autor może wskazać istniejącą kontrolę w innym miejscu. Recenzent powinien wtedy sprawdzić, czy obejmuje tę ścieżkę wywołania. Jeśli zabezpieczenia brakuje, jeden z wariantów poprawy wygląda tak:

public function __invoke(CancelOrder $command): void
{
    $this->authorization->requireCanCancelOrders();

    $accountId = $this->currentAccount->id();
    $order = $this->orders->findInAccount(
        $command->orderId,
        $accountId,
    );

    if ($order === null) {
        throw new OrderNotFound();
    }

    $order->cancel();
    $this->orders->save($order);
}

To fragment handlera, z pominiętym konstruktorem i implementacją zależności. Zakładamy, że currentAccount pochodzi ze zweryfikowanego kontekstu użytkownika, a jego członkostwo i uprawnienia sprawdza warstwa autoryzacji. Identyfikator konta przesłany swobodnie w żądaniu nie spełnia tego założenia. Repozytorium musi rzeczywiście ograniczać zapytanie, a warstwa HTTP mapować wyjątek na uzgodnioną odpowiedź.

W tym wariancie cudzy i nieistniejący identyfikator dają 404, żeby nie ujawniać istnienia zasobu poza dostępnym zakresem. To świadoma decyzja kontraktu API. Obsługa użytkownika bez uprawnienia jest osobną ścieżką i wymaga własnego sprawdzenia.

Jakie testy potwierdzają poprawkę

  • Uprawniony użytkownik anuluje własne zamówienie w dozwolonym stanie.
  • Podanie identyfikatora cudzego zamówienia nie zmienia jego stanu ani nie uruchamia powiadomienia czy zwrotu.
  • Cudzy i nieistniejący identyfikator dają uzgodnioną odpowiedź bez danych zasobu.
  • Użytkownik bez uprawnienia nie może anulować nawet własnego zamówienia.
  • Zamówienie w niedozwolonym stanie nadal odrzuca operację; poprawka dostępu nie omija reguły domenowej.

Test handlera może sprawdzić współpracę zależności, ale mock repozytorium nie udowodni, że SQL filtruje po koncie. Ten warunek warto objąć testem integracyjnym z rekordami dwóch kont. Test przez HTTP sprawdzi dodatkowo źródło tożsamości i odpowiedź API. Nie musisz powielać całego zestawu na każdym poziomie; każdy test powinien dostarczać konkretny dowód.

Jeśli anulowanie powoduje skutki zewnętrzne, potrzebujesz również decyzji o transakcji, ponowieniach i kolejności operacji. W systemie publikującym zdarzenia pomocny może być Outbox Pattern. Samo dodanie kontroli konta nie rozwiązuje współbieżnych zmian ani problemu podwójnego wykonania.

Komentarz powinien pokazać problem i jego wagę

Dobry komentarz wskazuje miejsce, opisuje konsekwencję i mówi, czego potrzeba przed scaleniem. Propozycja kodu może pomóc, ale warto zostawić autorowi możliwość innego rozwiązania tego samego problemu.

  • Blokujące: „Po błędzie zapisu nadal zwracamy sukces. Klient uzna zmianę za przyjętą, choć nie ma jej w bazie. Przed scaleniem potrzebujemy odpowiedzi błędu i testu tej ścieżki”.
  • Sugestia: „Nazwa result utrudnia rozpoznanie, czy chodzi o płatność, czy wynik walidacji. Proponuję paymentDecision. Nie blokuje scalenia”.
  • Pytanie: „Gdzie gwarantujemy unikalność tego identyfikatora? Nie znalazłem ograniczenia w schemacie. Jeśli go nie ma, dwa żądania mogą utworzyć duplikat”.
  • Drobiazg: „Literówka w komentarzu; można poprawić przy okazji”.

Pytanie również może dotyczyć blokującej niepewności. Zaznacz to wprost, zamiast oczekiwać, że autor odczyta intencję z tonu. Nie zamieniaj każdego polecenia w pozorne pytanie: „czy nie byłoby lepiej przepisać wszystkiego?” nadal może być żądaniem dużej zmiany.

Odwołuj się do wymagania, kontraktu, przykładu błędu albo uzgodnionej zasady. Samo „tak jest czyściej” nie wystarczy, żeby zablokować poprawne rozwiązanie. O nazwach, które pomagają rozpoznać odpowiedzialność i reguły, piszę szerzej w artykule Nazewnictwo w kodzie.

Co powinno blokować scalenie

Blokada jest uzasadniona, gdy możesz wskazać istotną wadę albo brak dowodu potrzebnego do oceny ryzykownej zmiany. Przykłady to naruszenie kontroli dostępu, możliwość utraty danych, błędna reguła biznesowa, niezgodna zmiana kontraktu lub migracja, która uniemożliwi bezpieczne wdrożenie.

Uzgodniony standard zespołu również może stanowić kryterium. Zasady dotyczące zależności między modułami czy ochrony danych mają zwykle konkretny powód. Wskaż go i upewnij się, że reguła rzeczywiście dotyczy tego przypadku.

Preferencja dotycząca równoważnego zapisu pętli, warunku czy nazwy pomocniczej zmiennej zwykle nie uzasadnia zatrzymania poprawki. Jeśli styl da się jednoznacznie opisać, powierz go formatterowi lub analizatorowi. Regularnie powracające uwagi warto zamienić w zasadę, przykład albo automatyczną kontrolę.

Review nie wymaga usunięcia całego zastanego długu technicznego. Nowa zmiana powinna być zrozumiała i utrzymywać akceptowalną jakość systemu. Znaleziony przy okazji problem możesz wydzielić, jeśli nie jest warunkiem poprawności bieżącej pracy. Zapisz wtedy konkretny problem i właściciela; samo „zrobimy później” nie stanowi planu. Krytycznej luki w zmienianej ścieżce nie odkładaj wyłącznie po to, żeby zmniejszyć PR.

Jak odpowiadać na uwagi

Autor nie musi bez dyskusji wdrażać każdej propozycji. Powinien jednak rozliczyć każdy istotny wątek: poprawić problem, pokazać istniejące zabezpieczenie albo wyjaśnić, dlaczego inny wariant lepiej spełnia wymaganie.

Zamiast samego „poprawione” napisz: „Zapytanie filtruje teraz po koncie z kontekstu sesji; test z dwoma kontami potwierdza brak zmiany cudzego rekordu”. Przy świadomym odstępstwie wskaż powód i uzgodnienie. Taka odpowiedź pozwala recenzentowi sprawdzić efekt, zamiast ponownie odtwarzać tok myślenia.

Po większych poprawkach podsumuj, co się zmieniło, i poproś o ponowne sprawdzenie. Recenzent powinien obejrzeć nowy diff oraz jego wpływ na wcześniejsze ustalenia. Zamknięty wątek i zielony check nie potwierdzają samodzielnie, że problem został rozwiązany.

Kiedy rozmowę przenieść poza komentarze

Gdy wymieniacie kolejne odpowiedzi, ale nadal inaczej rozumiecie wymaganie, zatrzymaj dyskusję pisemną. Krótka rozmowa przy kodzie często szybciej ujawnia brakujące założenie. Po spotkaniu zapisz w PR-ze decyzję, powód i dalszy krok, żeby pozostałe osoby mogły ją ocenić.

Jeśli spór dotyczy reguły biznesowej, potrzebna może być osoba odpowiedzialna za produkt. Jeśli dotyczy granicy modułów, odwołajcie się do uzgodnionej architektury lub zapiszcie nową decyzję. Zespół powinien znać sposób rozstrzygania sporów; sama przewaga stanowiska ani liczba komentarzy nie dowodzą poprawności rozwiązania.

Opisuj kod i skutek: „ta ścieżka pomija kontrolę uprawnienia”. Ocena autora, ironia i publiczne sprawdzanie jego wiedzy utrudniają ujawnienie błędu. Życzliwy ton nie wymaga łagodzenia informacji o poważnym ryzyku.

Jak nie zamienić review w kolejkę oczekiwania

Ustalcie czas pierwszej reakcji odpowiedni do godzin pracy i obowiązków zespołu. Może to być na przykład ten sam lub następny dzień roboczy. Reakcja może oznaczać informację, kiedy zajmiesz się zmianą, albo wskazanie innego recenzenta. Nie jest obietnicą natychmiastowej akceptacji.

Wybieraj osoby znające zmieniany obszar i ryzyko. Dodatkowy recenzent ma sens, jeśli wnosi inną potrzebną perspektywę, na przykład bezpieczeństwo lub utrzymanie integracji. Kilka akceptacji bez jasnego zakresu odpowiedzialności może tylko rozmyć sprawdzenie.

Przy pilnej poprawce ogranicz zakres do koniecznej zmiany, wybierz dostępnego recenzenta i sprawdź krytyczną ścieżkę. Miej plan wycofania. Uzgodniona procedura awaryjna powinna wskazywać również późniejszy przegląd; pośpiech sam w sobie nie jest dowodem, że kontrolę można pominąć.

Obserwujcie czas oczekiwania na pierwszą reakcję, liczbę rund i problemy, które wróciły po wdrożeniu. Szukajcie przyczyn: zbyt duży zakres, niejasne wymaganie, brak właściciela albo przeciążenie jednej osoby. Nie oceniaj ludzi liczbą komentarzy czy szybkością zatwierdzania; takie miary zachęcają do pozornej aktywności i pobieżnego czytania.

Co oddać automatom i narzędziom AI

Formatter, analiza statyczna, testy i skanowanie zależności powinny odciążać ludzi w powtarzalnych kontrolach. Jeśli narzędzie może jednoznacznie egzekwować uzgodnioną regułę, nie ma potrzeby odtwarzać jej ręcznie w każdym PR-ze. Jego konfiguracja i wynik nadal wymagają rozumienia.

Asystent AI może pomóc znaleźć pominiętą ścieżkę błędu, zaproponować przypadki testowe albo streścić duży diff. Jego komentarz traktuj jak hipotezę do sprawdzenia w kodzie. Narzędzie może nie znać reguły biznesowej, konfiguracji wdrożenia ani zachowania zależności.

Nie przenoś automatycznie wszystkich sugestii do review. Osoba publikująca uwagę powinna umieć wyjaśnić problem i sprawdzić proponowaną poprawkę. Korzystaj z narzędzi dopuszczonych do danego repozytorium i danych; fragment kodu, log lub opis zadania może zawierać informacje, których nie wolno przekazać zewnętrznej usłudze.

Praktyka na najbliższy PR

Jako autor opisz problem, nowe zachowanie i dowody sprawdzenia. Jako recenzent przejdź przez główną ścieżkę oraz najbardziej prawdopodobne błędy. Każdą istotną uwagę powiąż z konsekwencją i oznacz, czy wymaga działania przed scaleniem.

Przed akceptacją upewnijcie się, że blokujące problemy są rozwiązane, uzgodnione kontrole przeszły, a opis odpowiada końcowemu kodowi. Nie dopisujcie wymagań tylko dlatego, że PR jest jeszcze otwarty. Gdy odkryjecie nowe ryzyko, nazwijcie je i uzgodnijcie zakres potrzebnej zmiany.

Warto porównać własne zasady z przewodnikami Google dla recenzentów i dla autorów zmian. Pomagają uporządkować standard oceny, sposób komentowania i przygotowanie małych, czytelnych zmian.

Dobre code review daje zespołowi uzasadnioną podstawę do scalenia oraz wiedzę potrzebną do utrzymania zmiany. Czasem kończy się dłuższą rozmową, czasem jednym zdaniem akceptacji. Liczy się to, czy sprawdziliście właściwe rzeczy i potraficie wyjaśnić podjętą decyzję.