2G

Use this checklist on every PR. Each item is a potential production incident waiting to happen.

Author Self-Review (Before Requesting Review)

Do this yourself before asking anyone else to review. A PR that fails this self-check shouldn’t be opened.

• ☐ I have read every line of my own diff
• ☐ No debug logs, commented-out code, or TODO left in
• ☐ No PII, credentials, or secrets in any file
• ☐ Tests cover positive, negative, and edge cases
• ☐ UI automation is green on my local machine
• ☐ Backward compatibility confirmed – see Backward Compatibility if changing API/DB/queue

Reviewer Checklist

🔵 Code Quality

• ☐ Code Quality => clean code principles and coding standards
• ☐ Backward Compatibility => API, database, and message queue changes are safe
• ☐ Security => input validation, no sensitive data in logs, secrets scanned
• ☐ Kubernetes / Infra => manifests and configurations are correct
• ☐ Tests => unit and integration tests present and passing
• ☐ Performance => no chatty API/DB calls; indexes present; caching used where appropriate
• ☐ Function Correctness => change does what the story requires
• ☐ Feature Toggles=> new business logic changes are behind a toggle
• ☐ External Configurations => new config keys/secrets are in the secret manager
• ☐ API Spec => new or versioned APIs have corresponding spec changes
• ☐ Logging => correct log levels; no PII or sensitive data in logs
• ☐ Integration Points => connection, read, and write timeouts configured

Detail: Clean Code

What to Look For in Code Review

// ❌ RED FLAG: Method doing too many things
public void saveUser(User user) {
    validate(user);
    encryptPassword(user);
    sendWelcomeEmail(user);
    repository.save(user);
    auditLog.record("USER_CREATED", user.getId());
}
// ✅ Better: name the orchestration step, extract concerns
public void registerUser(User user) {
    validateRegistration(user);
    userRepository.save(prepareForStorage(user));
    eventPublisher.publish(new UserRegisteredEvent(user.getId()));
}

Detail: Database Changes

Tip

All schema changes must follow backward compatibility principles- see Database Backward Compatibility.

• New columns are nullable or have a default value
• Column type changes include a data migration plan
• Tables/columns are not removed without a deprecation strategy
• Migrations are idempotent (safe to run multiple times)
• Rollback scripts are provided for each migration
• Indexes added for columns used in WHERE or JOIN clauses
• CREATE INDEX CONCURRENTLY used on non-empty production tables
• Foreign key constraints have appropriate ON DELETE/ON UPDATE actions

Detail: API Changes

• Follows API style guide (RESTful URLs, correct HTTP verbs)
• Service boundaries respected – no direct DB access into another service’s data
• Backward compatible, or version introduced – see API Backward Compatibility
• Business logic changes are behind a feature toggle
• Logging and distributed traces are present on new endpoints
• HTTP status codes are correct (200 vs 201, 400 vs 404 vs 409)
• OneSpec / APIM changes implemented for new or versioned APIs
• Authentication and authorization checks are in place

Detail: Message Queue Changes

• Message format changes are backward compatible – see Message Queue Backward Compatibility
• New fields are optional with sensible defaults
• Consumer handles both old and new message formats during transition
• DLQ configured and monitored

Detail: Security

Danger: ANTI-PATTERN

Never log sensitive data: passwords, account numbers, card numbers, PII.

Once in a log, it’s in your log aggregation system, accessible to anyone with log access, forever.

• No PII, passwords, or account numbers in logs
• Sensitive data encrypted at rest and in transit
• Talisman pre-commit hook passed – no secrets committed to code
• Auth/authorization checks implemented
• All user inputs are sanitised to prevent SQL injection and XSS
• Secret rotation implemented for database credentials
• External inputs validated at system boundaries – trust nothing from outside

Detail: Testing

Follow the Test Pyramid: unit tests are the base (many, fast, cheap); integration tests are the middle; E2E tests are the tip (few, slow, expensive). Don’t invert it.

Code Review Etiquette

For reviewers:

• Ask questions rather than making demands: “Have you considered X?” vs “You must do X”
• Distinguish blocking issues from suggestions: use nit: prefix for cosmetic feedback
• Review the code, not the person
• Acknowledge good work: “Nice use of the Strategy pattern here”
• Respond within 1 business day; don’t block others

For authors:

• Don’t take review feedback personally- it’s about the code, not you
• Respond to every comment (even just “done” or “won’t fix because…”)
• If you disagree, explain your reasoning with data or references
• Keep PRs small – under 400 lines of diff is reviewable in one sitting

Tip

Aim for < 400 lines per PR. Large PRs take 2–4× longer to review and have proportionally more defects that slip through. Split large features using feature toggles.

Written by

Balaji G