Menu
Comprehensive code review process, checklists, and best practices
Installer avec Codex ou Claude Copiez ce prompt, collez-le dans Codex, Claude ou un autre assistant, puis laissez-le vérifier la page du skill et l'installer pour vous.
Basé sur la classification professionnelle SOC
ZGO API development standards including pagination, error handling, and RESTful design
Test patterns, mocking strategies, and organization best practices
Standardized error format, error code clusters, and API client usage for consistent error handling.
Specifications for Zustand stores, React Query hooks, and the Service-Hook-Type pattern with optimistic updates.
Strict rules for environment variable management using Zod validation and src/config/env.ts.
Guidelines for managing internationalization (i18n) in the project using next-intl and unified translation patterns.
| name | code-review-guide |
| description | Comprehensive code review process, checklists, and best practices |
| version | 1.0.0 |
| category | development |
| tags | ["code-review","quality","collaboration","pr"] |
| author | ZGO Team |
| updated | "2026-01-24T00:00:00.000Z" |
This skill provides a comprehensive guide to performing effective code reviews in the ZGO project, ensuring code quality, knowledge sharing, and team collaboration.
Before creating a PR, authors must:
# 1. Run all automated checks
make test # All tests must pass
make lint # No linting errors
go fmt ./... # Code formatted
# 2. Run skill-specific validations
.agent/skills/coding-standards/scripts/verify-standards.sh <module>
.agent/skills/api-development/scripts/validate-api.sh <module>
# 3. Self-review changes
git diff main...HEAD # Review your own changes
# 4. Update documentation
# - Update CHANGELOG.md
# - Update README.md if needed
# - Add/update code comments
Pre-Review Checklist:
Focus: High-level design, architecture, approach
What to Check:
✅ Good PR Description:
## What
Implemented user authentication with JWT tokens
## Why
Users need secure login without storing passwords in session
## How
- Added JWT middleware
- Created auth service with token generation
- Added login/logout endpoints
- Migrated password hashing to bcrypt
## Testing
- Unit tests for auth service
- Integration tests for login flow
- Manual testing with Postman
## Screenshots
[Login flow screenshot]
## Checklist
- [x] Tests passing
- [x] Documentation updated
- [x] Breaking changes documented
// ✅ Good: Follows layered architecture
func (h *Handler) Create(c *gin.Context) {
var req CreateRequest
if !handler.BindJSON(c, &req) {
return
}
user, err := h.service.Create(c.Request.Context(), &req) // Uses service
response.Success(c, ToResponse(user))
}
// ❌ Bad: Violates architecture
func (h *Handler) Create(c *gin.Context) {
var po UserPO
c.BindJSON(&po)
h.db.Create(&po) // Direct DB access in handler!
c.JSON(200, po)
}
Review Questions:
Comments to Leave:
🏗️ **Architecture**: This violates layer separation. Handlers should not
access repositories directly. Please route through the service layer.
💡 **Suggestion**: Consider using the Circuit Breaker pattern for this
external API call to prevent cascading failures.
⚠️ **Concern**: This approach will create N+1 query problem. Consider
using eager loading or batch fetching.
Focus: Code quality, correctness, edge cases
// ✅ Good naming
type UserPO struct { ... } // PO suffix for models
type CreateUserRequest struct { ... } // Request suffix
func GetByEmail(email string) { ... } // Clear, specific
// ❌ Bad naming
type User struct { ... } // Missing PO suffix in model.go
type CreateUserDTO struct { ... } // Should be "Request"
func Get(param string) { ... } // Vague, what does it get?
Check:
// ✅ Good: Proper error handling
user, err := s.repo.GetByID(ctx, id)
if err != nil {
if errors.Is(err, repository.ErrNotFound) {
return nil, ErrUserNotFound
}
return nil, fmt.Errorf("failed to get user %d: %w", id, err)
}
// ❌ Bad: Ignoring errors
user, _ := s.repo.GetByID(ctx, id) // Don't ignore errors!
// ❌ Bad: No context
user, err := s.repo.GetByID(ctx, id)
if err != nil {
return nil, err // No wrapping, no context
}
// ❌ Bad: Swallowing errors
user, err := s.repo.GetByID(ctx, id)
if err != nil {
log.Println(err) // Logged but not returned!
return nil, nil
}
Check:
_, _ :=)// ✅ Good: Secure password handling
hash, err := crypto.HashPassword(req.Password)
if err != nil {
return nil, err
}
user.Password = hash // Store hash, not plaintext
// JSON tag protects from exposure
type User struct {
Password string `json:"-"` // Never exposed in responses
}
// ❌ Bad: Security issues
user.Password = req.Password // Storing plaintext!
log.Printf("User password: %s", password) // Logging password!
db.Where("username = '" + username + "'") // SQL injection!
Check:
json:"-" tag// ✅ Good: Efficient query
users, err := db.Preload("Profile").Find(&users)
// ❌ Bad: N+1 query problem
users, _ := db.Find(&users)
for _, user := range users {
profile, _ := db.Where("user_id = ?", user.ID).First(&profile) // N queries!
}
// ✅ Good: Use pagination
paginator, err := pagination.PaginateFromContext[*domain.User](c, db)
// ❌ Bad: Load everything
var users []User
db.Find(&users) // Could be millions of rows!
Check:
// ✅ Good: Comprehensive test
func TestService_Create_Success(t *testing.T) {
// Setup
mockRepo := new(MockRepository)
service := NewService(mockRepo)
req := &CreateUserRequest{
Email: "test@example.com",
Username: "testuser",
}
// Expectations
mockRepo.On("Create", mock.Anything, mock.MatchedBy(func(user *domain.User) bool {
return user.Email == req.Email
})).Return(nil)
// Execute
user, err := service.Create(context.Background(), req)
// Assert
assert.NoError(t, err)
assert.NotNil(t, user)
assert.Equal(t, req.Email, user.Email)
mockRepo.AssertExpectations(t)
}
// ❌ Bad: Weak test
func TestCreate(t *testing.T) {
service.Create(context.Background(), &req) // No assertions!
}
Check:
Focus: Documentation, readability, maintainability
// ✅ Good: Helpful comments
// HashPassword generates a bcrypt hash from plaintext password.
// Returns error if password is empty or hashing fails.
func HashPassword(password string) (string, error) {
if password == "" {
return "", errors.New("password cannot be empty")
}
// Use cost=10 for balance between security and performance
hash, err := bcrypt.GenerateFromPassword([]byte(password), 10)
return string(hash), err
}
// ❌ Bad: Useless comments
// Hash password
func HashPassword(password string) (string, error) { // What does it do?
hash, err := bcrypt.GenerateFromPassword([]byte(password), 10) // Missing validation
return string(hash), err
}
// ❌ Bad: Commented-out code
func GetUser(id uint) {
// user, _ := db.Find(id)
// return user
return db.First(id) // Remove dead code!
}
Check:
// ✅ Good: Complete Swagger docs
// CreateUser godoc
// @Summary Create a new user
// @Description Creates a new user account with email and password
// @Tags users
// @Accept json
// @Produce json
// @Param request body CreateUserRequest true "User creation request"
// @Success 201 {object} UserResponse
// @Failure 400 {object} response.ErrorResponse "Invalid request"
// @Failure 409 {object} response.ErrorResponse "Email already exists"
// @Router /api/users [post]
func (h *Handler) Create(c *gin.Context) {
// ...
}
// ❌ Bad: Missing or incomplete Swagger
func (h *Handler) Create(c *gin.Context) { // No docs!
// ...
}
// ✅ Good: Readable
func (s *service) IsEligibleForDiscount(user *domain.User, order *domain.Order) bool {
isPremiumMember := user.Tier == "premium"
isLargeOrder := order.Total > 100
isFirstOrder := user.OrderCount == 0
return isPremiumMember || isLargeOrder || isFirstOrder
}
// ❌ Bad: Hard to read
func (s *service) IsEligibleForDiscount(u *domain.User, o *domain.Order) bool {
return u.Tier == "premium" || o.Total > 100 || u.OrderCount == 0 // What does this mean?
}
Check:
1. Be Specific
❌ Bad: "This is wrong"
✅ Good: "The error is not being handled on line 45. This could cause a panic
if the database connection fails. Consider wrapping with an error check."
2. Be Constructive
❌ Bad: "This code is terrible"
✅ Good: "This approach works, but could be improved. Consider using the
repository pattern to separate data access concerns. See module-creation
skill for examples."
3. Ask Questions
✅ "Could you explain why you chose to use a goroutine here? I'm concerned
about potential race conditions."
✅ "Have you considered using the Circuit Breaker pattern for this external
API call? See coding-standards skill section 9.2."
4. Provide Context
✅ "According to our API standards (api-development skill), all list endpoints
must use pagination. Can you add pagination.PaginateFromContext() here?"
✅ "This violates our naming convention. Database entities should have 'PO'
suffix. See coding-standards Level 2 for details."
5. Praise Good Work
✅ "Great use of the Circuit Breaker pattern here! This will prevent cascading
failures if the payment gateway goes down."
✅ "Excellent test coverage! I appreciate the edge case tests."
✅ "This is a clean implementation of the repository pattern."
Example:
🔴 **MUST FIX**: SQL injection vulnerability on line 67. User input is
concatenated directly into query. Use parameterized queries instead:
db.Where("email = ?", email) // ✅ Safe
not: db.Where("email = '" + email + "'") // ❌ Vulnerable
Example:
🟡 **Should Fix**: Error is ignored on line 34. This could hide failures.
Please add error handling:
if err != nil {
return fmt.Errorf("failed to create user: %w", err)
}
Example:
🟢 **Consider**: This function could be simplified using the MultiError
pattern from coding-standards skill section 9.5. Not blocking, but would
improve error visibility in batch operations.
For small changes (< 100 lines):
For significant changes:
Architecture (5 min):
Code Quality (10 min):
Testing (5 min):
Documentation (5 min):
API Standards (5 min):
response.* for all responsesbinding tagsFor critical or complex changes:
Deep Architecture Review:
Security Audit:
Performance Analysis:
# View PR diff locally
gh pr checkout <pr-number>
gh pr diff <pr-number>
# Leave review comments
gh pr review <pr-number> --comment -b "Review comments here"
# Approve PR
gh pr review <pr-number> --approve
# Request changes
gh pr review <pr-number> --request-changes -b "Please fix X"
# .github/workflows/pr.yml
name: PR Checks
on: [pull_request]
jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: actions/setup-go@v4
- run: make test
- run: make lint
- run: go test -coverprofile=coverage.out ./...
- run: go tool cover -func=coverage.out | grep total
# Quick standards check
.agent/skills/coding-standards/scripts/verify-standards.sh <module>
# API standards check
.agent/skills/api-development/scripts/validate-api.sh <module>
# Logging standards check
.agent/skills/logging-standards/scripts/validate-logging.sh <module>
coding-standards - Code quality standardsapi-development - API best practicesmodule-creation - Module structurelogging-standards - Logging practicesBefore creating PR:
make test && make lint
.agent/skills/coding-standards/scripts/verify-standards.sh <module>
git diff main...HEAD # Self-review
When reviewing:
Feedback levels:
Version: 1.0.0
Last Updated: 2026-01-24
Maintainer: ZGO Team