review-pr

name	review-pr
description	Review a pull request for a CPP or HMCTS Crime Common Platform repository against CPP coding standards. Use when asked to review a PR or code changes specifically in a CP or HMCTS context service.

PR Review for Common Platform

Reviews pull requests against CP coding standards, architectural patterns, and platform conventions.

When to Use

• User asks to "review this PR", "check this pull request", "review PR #123"
• User pastes a PR URL or number
• User asks for feedback on changes before creating a PR

Process

Step 1: Gather Changes

If given a PR URL or number:

gh pr view <number> --json title,body,files,additions,deletions,baseRefName,headRefName
gh pr diff <number>

If reviewing local changes:

git diff main...HEAD
git log main..HEAD --oneline

Step 2: Identify Repo Type

Determine the repo type from the directory name and build files:

• cpp-context-* with pom.xml → Legacy context service (Maven, CQRS, Java EE)
• cpp-mbd-* with build.gradle → Modern by Default service (Gradle, Spring Boot)
• cpp-ui-* with package.json → Angular UI application
• cpp-terraform-* with *.tf → Terraform module
• cpp-helm-chart → Helm chart changes
• cp-c4-architecture with .c4 files → Architecture model changes
• cpp-ui-e2e-serenity with serenity.properties → UI E2E test repo (Serenity BDD + Cucumber)
• cpp-apitests with api-integration-test/ → API integration test repo (JUnit 5 + REST Assured)

Step 3: Apply Repo-Specific Review Criteria

Context Services (Maven/CQRS)

• Commands produce domain events (not direct state mutation)
• Queries are read-only (no state changes in query handlers)
• New viewstore entities have Liquibase migrations with rollback
• Event handlers are idempotent
• JSON schemas follow naming convention: {context}.{type}.{name}.json
• RAML API definitions are updated for new/changed endpoints
• At least one integration test per new/changed endpoint (<context>-integration-test/), exercising the real persistence/SQL + event path — not just unit tests that mock the repository
• Integration-test suite is green locally (mvn clean && ./runIntegrationTests.sh when present); summary pasted in the PR
• No domain logic in controllers or repositories (belongs in aggregates/handlers)
• Framework interceptors not bypassed without justification

Modern by Default Services (Gradle/Spring Boot)

• Constructor injection used (no @Autowired fields)
• Java records used for DTOs and event payloads
• @Operation and @ApiResponse on controller methods
• RestClient used for outbound HTTP (not RestTemplate)
• Structured logging with MDC context (correlationId, eventId)
• E2E tests with WireMock for external service verification
• SecurityConfig reviewed for appropriate access controls
• Service Bus consumers handle poison messages gracefully

Angular UI Apps

• Components follow govuk-frontend design patterns
• State management uses ngrx (not component-level state for shared data)
• Tests cover component behavior (not just template rendering)
• No hardcoded API URLs (use environment configuration)
• Accessibility: WCAG 2.1 AA compliance (aria labels, keyboard navigation)
• Lazy loading for feature modules

Terraform Modules

• Variables have descriptions and type constraints
• Outputs documented
• State is not stored locally
• Sensitive values marked as sensitive = true
• Tags applied consistently
• Pre-commit hooks pass (format, validate, docs)

UI E2E Tests (cpp-ui-e2e-serenity)

• Feature file in correct src/test/resources/features/<domain>/ folder
• Step definitions extend the relevant page object (no duplicated locators)
• New locators added to src/test/resources/locators/custom-locators.json (not hard-coded in Java)
• No Thread.sleep — explicit waits only
• Scenarios tagged for the intended pipeline profile (regression / BPO / migration / DLRM)
• No PII, court refs, or real personal data in testdata/ or scenario examples
• TestRunner glue covers any new step-def package
• Steps annotated with @Step; cross-step state held in the Serenity session (no static fields)

API Integration Tests (cpp-apitests)

• Test class name ends IT.java (Failsafe convention — *Test.java is silently skipped)
• Extends the closest abstract base (AbstractTest / ApplicationsAbstractTest / AuditAbstractTest / AuthorizationAbstractTest)
• Reuses existing helpers (JsonUtil, DbUtil, ApplicationUtil, RestAssuredFileUploadUtil, domain helpers) and builders rather than adding new ones
• REST Assured given().when().then() with Hamcrest / AssertJ — no raw assertEquals on JSON paths
• JSON fixtures under src/test/resources/<domain>/; no inline payload strings
• No hard-coded bearer tokens, URLs, PII, or court refs
• Assertions align with the producing service's RAML / OpenAPI contract — see api-contract-check

For authoring guidance in either test repo, see cpp-test-authoring.

Architecture Model (.c4)

• Relationship titles read as natural language sentences (lowercase first letter)
• Container-level relationships (not product-level when containers exist)
• JEE microservices have [auth] and [audit] relationships
• Technology metadata present on all components
• Views updated for new elements
• npm run build && npm run test passes

Step 4: Cross-Cutting Concerns

Always check regardless of repo type:

• No secrets, API keys, or credentials in code
• No hardcoded environment-specific values (use configuration)
• Breaking changes to APIs are clearly documented
• Changes are backward compatible (or migration path documented)
• Test coverage is adequate for the change
• New endpoints (any repo type) ship with at least one integration test, and the IT suite is green
• An implementation-plan artifact exists at docs/pipeline/artifacts/ and the change matches it
• Commit messages are meaningful

Step 5: Generate Review

## PR Review: [title]

### Summary
One paragraph describing what the PR does and whether it achieves its goal.

### Findings

#### Blocking
- [issue]: description and suggested fix

#### Suggestions
- [improvement]: description and rationale

#### Positive
- [what's good]: acknowledge well-done aspects

### Verdict
APPROVE / REQUEST CHANGES / NEEDS DISCUSSION

### Test Checklist
- [ ] Unit tests pass
- [ ] Integration tests pass (if applicable)
- [ ] Manual verification steps (if applicable)