Pull Request Standards¶

Status: 🟢 Active  |  Owner: Engineering Enablement  |  Last Reviewed: 2025-Q4

Overview¶

A pull request is both a code change and a communication artefact. The diff shows what changed; the description explains why and how to verify it. A PR that arrives with an empty description, a title of "fix stuff", and a 1,500-line diff is not ready to review — it is a time tax on the reviewer and a quality risk for the codebase.

These standards define the minimum quality bar for a PR to be considered ready for review. They are not bureaucratic requirements; they are the preconditions for a useful code review.

PR Size¶

The most important property of a PR is its size. Reviewers can give thorough, high-quality feedback on a small PR. They struggle to give meaningful feedback on a large one — the cognitive load of holding the context of 800 changed lines in mind while reviewing is simply too high.

Excluded from line counts: generated code, lock files, database migrations, vendored dependencies, and test fixture data files.

Breaking Up Large PRs¶

Large PRs almost always can be split. Common techniques:

• Refactor first, feature second: submit a refactoring PR with no behaviour change, then a separate feature PR on top.
• Infrastructure first, logic second: set up the scaffold (new class, endpoint stub, DB table) in one PR; add the business logic in the next.
• Feature flag: merge an incomplete feature behind a flag in one PR; complete it in subsequent small PRs.

If a PR genuinely cannot be split (e.g. a coordinated schema + application change), include an explicit explanation in the description and tag the Tech Lead for review.

PR Title Format¶

PR titles follow Conventional Commits format:

<type>(<scope>): <short description>

feat(orders): add discount code validation at checkout
fix(payments): prevent double-charge on idempotency key collision
chore(deps): upgrade Spring Boot to 3.2.4
refactor(auth): extract token validation into shared middleware
docs(api): document rate limiting behaviour in OpenAPI spec

The title is used as the squash commit message on merge. A clear title means a clear git history.

Required PR Description¶

All PRs must use the team PR template. The template is pre-loaded in GitHub for every repository via .github/pull_request_template.md. Complete every section.

## Summary
<!-- One paragraph: what this PR does and why. Link to the Jira ticket. -->
<!-- If this PR is part of a series, explain where it fits. -->

## Type of Change
- [ ] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that causes existing behaviour to change)
- [ ] Refactoring (no behaviour change)
- [ ] Documentation only
- [ ] Dependency update
- [ ] Infrastructure / configuration change

## How to Test
<!-- Step-by-step instructions to verify this change works. -->
<!-- Include: any required environment setup, the specific steps, and what to observe. -->
<!-- "Run the tests" is not sufficient — describe the manual or exploratory verification. -->

## Screenshots or Recordings
<!-- Required for UI changes. Optional but welcomed for non-UI changes. -->

## Checklist
- [ ] Tests added or updated — coverage does not regress
- [ ] Documentation updated (README, OpenAPI spec, ADR, runbook as applicable)
- [ ] No new linting warnings introduced
- [ ] No secrets or credentials committed
- [ ] Feature flag added for incomplete features
- [ ] Dependent PRs listed and linked below
- [ ] Breaking changes documented in CHANGELOG.md

## Dependent PRs
<!-- List any PRs in other repositories that must merge alongside this one. -->
<!-- Tag the owning engineer. -->

## Linked Issues
Closes PROJ-XXXX

Merge Requirements¶

The following conditions must all be true before merge. GitHub branch protection enforces them automatically:

Critical services (defined in the service catalogue) require a minimum of 3 approvals, with at least one from a member of Security Engineering for changes that touch authentication, authorisation, cryptography, or PII handling.

Merge Strategy¶

Merge commits are prohibited on main except for release merges. A merge-commit-heavy history is difficult to bisect, difficult to read, and difficult to cherry-pick from.

Draft PRs¶

Use Draft PRs for work in progress that needs early feedback or visibility but is not ready for formal review:

• Convert to Ready for Review only when the PR is complete, tests pass, and the description is filled in.
• Do not request reviews on a draft PR — it wastes reviewer time.
• Draft PRs older than 5 days without activity are flagged in the weekly engineering digest as potential blockers.

Stacked PRs¶

For complex features that benefit from incremental review, stacked PRs are permitted:

main ← PR-1 (infrastructure) ← PR-2 (core logic) ← PR-3 (API layer)

When using stacked PRs: - Each PR must be independently valid (not break main if merged alone). - The base of each PR targets the previous PR in the stack, not main. - Rebase the stack onto main before merging PR-1, then rebase PR-2 onto main, and so on. - Each PR description must link to the others and explain the stack order.

Hotfix PRs¶

During an active production incident, the normal review process may be accelerated:

• The on-call engineer may self-approve and merge with a single approval (or self-merge in a P1 with SRE acknowledgement in the incident channel).
• A post-facto review must be completed within 24 hours of the incident being resolved.
• Hotfix PRs are tagged hotfix and post-facto-review-required in GitHub.
• The post-facto review must confirm: the fix is correct, tests cover the regression, and no shortcuts were taken that introduce new risk.

References¶

• Code Review Guidelines
• Git Standards & Commit Conventions
• Branching Strategies
• Production Readiness Checklist

Last reviewed: 2025-Q4  |  Owner: Engineering Enablement