Skip to content

Git – Pull Requests & Code Review

A Pull Request (PR) is the formal mechanism through which code moves from a developer's branch into a shared branch. It is not just a technical merge operation — it is a quality gate, a knowledge-sharing session, and a communication tool. Every production line of code at Cygnus Dynamics passes through at least one PR review before merging.

All PRs are managed in Azure DevOps Repos under the Pull Requests section of your repository.

Why Code Review Matters

  • Catches bugs before they reach QA, UAT, or production
  • Enforces coding standards — a second pair of eyes spots what automation misses
  • Spreads knowledge — reviewers learn the codebase; authors learn from feedback
  • Produces better designs — explaining your change often reveals its own problems
  • Shared ownership — the team owns the code, not individual developers

Every PR is merged by a reviewer, not by the author

The author raises the PR and responds to feedback. The reviewer approves and merges after all checks pass. Authors must never merge their own PRs.

Before You Raise a PR

Complete this checklist before clicking Create Pull Request. A PR that fails these checks will be sent back — resolving issues before review saves everyone time.

Author Pre-PR Checklist

  • [ ] Branch is up-to-date with the latest develop (merge or rebase)
  • [ ] Code compiles and the application runs locally
  • [ ] All existing tests pass — no new failures introduced
  • [ ] New tests written for new logic and bug fixes
  • [ ] No console.log, System.out.println, print() debug statements left in
  • [ ] No hardcoded secrets, API keys, or credentials
  • [ ] No commented-out code — delete it; git history preserves it
  • [ ] Self-reviewed the diff line by line — read it as if you are the reviewer
  • [ ] Commit messages follow Conventional Commits
  • [ ] PR is focused on one logical change — not multiple unrelated things
  • [ ] PR description is filled out using the template

PR Size — Keep It Small

The single most impactful PR quality improvement is keeping PRs small and focused. Large PRs receive worse reviews and introduce more bugs.

PR Size Lines Changed Review Quality
Small < 200 lines Thorough — every line examined
Medium 200–500 lines Acceptable — may miss subtle issues
Large 500–1000 lines Surface-level — reviewers scan, not read
Enormous > 1000 lines Rubber-stamped — effectively unreviewed

A PR should represent one logical change. If you find yourself writing "and also..." in the PR description, that is a signal to split it.

✅ Good PR scope
- "Add order cancellation endpoint"
- "Fix null pointer in payment processor"
- "Refactor UserService to extract EmailValidator"

❌ Bad PR scope
- "User feature work" (too vague)
- "Fix several bugs and add new dashboard and update dependencies" (three PRs in one)
- Sprint dump — everything from a two-week sprint in one PR

Creating a PR in Azure DevOps

Via the UI

  1. Navigate to your repository: https://dev.azure.com/<org>/<project>/_git/<repo>
  2. Go to Repos → Pull Requests
  3. Click New Pull Request
  4. Select your source branch (e.g., feature/rk/login-page)
  5. Select the target branch (develop for features and bug fixes)
  6. Fill in the title and description using the template below
  7. Add required reviewers
  8. Click Create

Source → Target Branch Rules

Branch Type Source Target
Feature feature/<initials>/<name> develop
Bug fix bugfix/<initials>/<name> develop
Hotfix hotfix/<initials>/<name> master
Promote to QA develop qa
Promote to UAT qa uat
Release uat master

PR Title

Follow the same Conventional Commits format as commit messages:

feat(auth): add JWT refresh token rotation
fix(payments): prevent duplicate charge on gateway timeout
refactor(user): extract email validation into shared validator

PR Description Template

Every PR must include a description. Copy this template and fill it in:

## What does this PR do?

[One to three sentences describing the change. What problem does it solve?
What feature does it add? Be specific.]

## Why is this change needed?

[Context the reviewer needs. Reference the ticket, user story, or bug report.
What was broken or missing? What are the business/technical motivations?]

## How was this implemented?

[Key technical decisions made. If you chose approach A over approach B, explain why.
If there are trade-offs, call them out here.]

## How to test this

[Step-by-step instructions for the reviewer to verify the change works correctly.
Include any test data, credentials (reference to secrets — never the actual values),
or environment setup needed.]

## Screenshots / recordings (if UI change)

[Attach before/after screenshots for any visual or UX change.]

## Related tickets

Closes: PROJ-XXXX
Refs: PROJ-YYYY

Example of a Well-Written PR Description

## What does this PR do?

Adds rate limiting to the POST /api/v1/auth/login endpoint to prevent
brute-force password attacks. Returns 429 Too Many Requests after
10 failed attempts within a 1-hour window per IP address.

## Why is this change needed?

Identified in the March 2026 security audit (SEC-2026-003). The login
endpoint had no protection against automated credential stuffing.
Closes: PROJ-2847

## How was this implemented?

Uses Redis-backed sliding window rate limiting via the express-rate-limit
library with a custom Redis store. The window and limit are configurable
via environment variables (RATE_LIMIT_WINDOW_MS, RATE_LIMIT_MAX_ATTEMPTS).

Chose sliding window over fixed window to prevent the "burst at boundary"
problem where an attacker can make 10 attempts at 11:59 and 10 more at 12:00.

## How to test this

1. Start the service with Redis running locally
2. Send 10 POST /api/v1/auth/login requests with invalid credentials
3. Verify the 11th request returns 429 with Retry-After header
4. Wait for the window to reset (or manually clear the Redis key)
5. Verify login works again after the window resets

Unit tests are in src/middleware/rateLimiter.test.ts — run with npm test.

## Related tickets

Closes: PROJ-2847
Refs: SEC-2026-003

Reviewers

Who Reviews

  • Feature and bug fix PRs — at least 1 reviewer, recommended to be a peer developer or Tech Lead
  • Promotions (develop → qa, qa → uat) — Tech Lead review required
  • Merges to master — minimum 2 reviewers including the Tech Lead
  • Hotfixes to master — Tech Lead approval required, expedited review

Assigning Reviewers

In Azure DevOps, add reviewers when creating the PR. For features, add your immediate teammates. For promotions and releases, always include the Tech Lead.

If a review has not received a response after 1 business day, follow up in the team Slack channel #code-reviews. Do not let PRs go stale.

Reviewer Responsibilities

What to Look For

A review is not just a syntax check. Reviewers examine:

Correctness - Does the code do what the PR description says it does? - Are there edge cases that are not handled? - Could this fail in production in ways that weren't tested?

Code Quality - Does it follow the coding standards for the language (Java, Python, TypeScript, etc.)? - Is it readable — would a developer unfamiliar with this area understand it? - Are there obvious simplifications that would improve clarity?

Security - Are user inputs validated? - Are there any hardcoded secrets, tokens, or credentials? - Could this introduce an injection vulnerability?

Tests - Do the new tests actually test the new logic? - Are the test names descriptive — do they explain what behaviour they cover? - Are there important scenarios missing from the test suite?

Architecture - Does this change fit the existing patterns in the codebase? - Is there coupling that should be avoided? - Is anything being added that should be shared but isn't?

Comment Labels

Use these labels at the start of review comments to make their intent immediately clear:

Label Meaning Author Action Required
[blocking] Must be fixed before approval Fix and re-request review
[suggestion] Worth considering, not mandatory Address or explain why not
[question] Seeking clarification — not a change request Answer the question
[nit] Minor style point, trivial — author's call Optional — address or ignore
[praise] Positive feedback — good pattern, good naming None 
[blocking] This query has an N+1 problem — fetching the user inside the
           loop will execute one DB query per order. Use a JOIN or batch
           the user IDs and fetch them all at once.

[suggestion] The validation logic in lines 45–72 could be extracted into
             a separate validatePaymentRequest() function, which would make
             it easier to unit test independently.

[question] Why is `retryCount` initialised to -1 here rather than 0? Is
           the first attempt counted as a retry?

[nit] Variable name `d` on line 23 could be more descriptive — `discountRate`
      or `discount` would make the intent clearer.

[praise] Nice use of the Builder pattern here — much cleaner than the
         previous constructor with 8 parameters.

Responding to Review Comments

Authors must respond to every review comment — even to say "done" or "disagree — here's why":

# ✅ Clear responses
"Fixed — extracted into validatePaymentRequest() as suggested."
"Addressed — added test case for the empty cart scenario."
"I'll leave this for now — the function is only called in one place
 and I'd rather keep it simple. Happy to revisit if it grows."
"Good catch — this was definitely a bug. Fixed and added a regression test."

# ❌ No response — reviewer cannot tell if the comment was seen or ignored

When all comments are addressed, request a re-review from the original reviewer. In Azure DevOps, use the "Re-request review" option.

Merge Strategies

Cygnus Dynamics uses Squash Merge for feature and bug fix branches merging into develop:

  • All commits on the feature branch are squashed into a single commit on develop
  • The squashed commit message is the PR title (which follows Conventional Commits)
  • This keeps the develop history clean and readable — one commit per feature

Merge Commit (no squash) is used for environment promotions (develop → qa → uat → master) to preserve the full merge history between environments.

# Azure DevOps applies these policies — no manual action needed
# Feature/bugfix → develop   = Squash merge
# develop → qa              = Merge commit
# qa → uat                  = Merge commit
# uat → master              = Merge commit (tagged)

Definition of Done — PR Checklist

A PR may only be approved and merged when all of the following are true:

  • [ ] PR description is complete — what, why, how, how to test
  • [ ] All CI pipeline checks pass (build, lint, tests, security scan)
  • [ ] All [blocking] review comments are resolved
  • [ ] All [question] comments are answered
  • [ ] Reviewer has approved (not just commented)
  • [ ] No unresolved merge conflicts
  • [ ] Branch is up-to-date with the target branch

Code Review Culture

Code review is a professional conversation, not a judgment of the author as a person. These principles guide how we give and receive feedback:

For reviewers: - Assume good intent — the author solved a real problem with the knowledge they had - Be specific — "this could be cleaner" is not actionable; "extract this into a named function so it can be unit tested" is - Explain the why behind a request — don't just say "change this to X" - Use [suggestion] and [nit] generously — not everything needs to block a merge - Approve promptly — a PR sitting unreviewed for 2+ days blocks the entire team

For authors: - Do not take feedback personally — the reviewer is critiquing the code, not you - Ask for clarification if a comment is unclear — do not guess the intent - Disagreement is healthy — explain your reasoning; the team decides together - Make the reviewer's job easy — a clear description and small PRs get faster, better reviews

The goal of code review is to ship better software, not to find fault.

A review with zero comments does not mean the code was perfect — it may mean the reviewer did not look carefully. The best reviews produce dialogue, share knowledge, and make both the author and reviewer better engineers.