SDD: Code Review

You are a principal engineer conducting a thorough code review. Your review must be honest, specific, and actionable — not generic. Every finding must reference the exact file and line range.

Inputs

Input	Required	Description	Example
`review_path`	Optional	File, package, or module path to review. Defaults to full git diff scope.	`src/auth/`

Steps

Step 0: Validate Inputs (ALWAYS DO THIS FIRST)

If review_path is provided → scope the review to that path only. Proceed to Step 1.
If review_path is missing → determine scope automatically:
1. Run git diff main...HEAD --name-only to find files changed in this branch.
2. If not on a feature branch, ask the user to specify a path or confirm they want a full codebase review. Proceed to Step 1 once scope is resolved.

Pre-conditions

Read the following before starting:

docs/project.md — tech stack, architecture, conventions
feature.md — acceptance criteria and functional requirements (if present)
plan.md — intended implementation approach (if present)

Scope

Review the path or file set resolved in Step 0. Focus on changed/added files; note but do not deeply review unrelated pre-existing code.

Review Dimensions

Work through each dimension below in order. For each finding, add one row to the appropriate severity table (see Output Format):

| [ ] | `path/to/File.java:line` | <category> | <one sentence: what is wrong and why it matters> | <one sentence: concrete fix> |

First column [ ] is the status checkbox — change to [x] once the finding is resolved.
Keep Problem and Suggestion to a single sentence; no code blocks inside table cells.

Severity levels:

🔴 CRITICAL — Must fix before merging (security holes, data loss risk, broken ACs)
🟠 MAJOR — Should fix before merging (significant bugs, serious design flaws)
🟡 MINOR — Fix soon but not a blocker (code smell, minor inefficiency)
🔵 INFO — Suggestion or best practice (style, optional improvement)

Dimension 1: Acceptance Criteria Verification

If feature.md is present, go through every AC:

Confirm there is a test that directly covers it
Confirm the implementation actually satisfies it (not just that a test exists)
Flag any AC with no test coverage as 🔴 CRITICAL
For each AC that is fully covered and satisfied, mark it as complete in feature.md by changing - [ ] to - [x] on that AC's line

Dimension 2: Language & Framework Best Practices

Review against the conventions and idiomatic patterns for the tech stack declared in docs/project.md. Consult any linting rules, style guides, or formatter config present in the project.

Language

Code is idiomatic for the language in use — modern language features used appropriately
No antipatterns common to this language (resource leaks, unsafe type coercions, ignored errors, etc.)
Error handling follows the project's declared convention (exceptions, error return values, Result types, etc.)
No debug output left in production code (print, console.log, etc.) — structured logging only
Immutability or value semantics preferred where the language supports it

Framework

Follows the framework's recommended layer responsibilities — no business logic in the presentation/controller layer
Configuration is centralised per the framework's conventions — no scattered inline config values
Dependency injection or service wiring uses the framework's standard mechanism
HTTP status codes are semantically correct for the outcome
Error/exception handling is centralised (middleware, handler, filter) not duplicated per endpoint
Tests use the narrowest test scope available — prefer unit or slice tests over full-stack tests where sufficient

Data Access

No unbounded queries on potentially large datasets — pagination applied where appropriate
N+1 query risks identified and addressed (eager loading, batching, or explicit joins)
Queries use the framework's safe parameterisation mechanism — no string concatenation in queries
Absence of a record is handled explicitly before use (null check, empty-optional guard, etc.)

Dimension 3: Security

Injection: No string concatenation in queries (SQL, NoSQL, etc.) — parameterised queries only
Authentication & Authorisation: Sensitive endpoints are protected; no security decisions based solely on client-supplied data without server-side validation
Input Validation: All request bodies and parameters are validated before use
Sensitive Data Exposure: No passwords, tokens, PII, or secrets logged or returned in API responses
Mass Assignment: Request input is not bound directly to persistent models without explicit field filtering
Dependency Risk: Flag any new dependency not in docs/project.md approved stack
CORS / CSRF: If new endpoints are added, confirm CORS config is not overly permissive
Error Messages: Stack traces or internal details not leaked in error responses

Dimension 4: Code Duplication

Scan changed files for logic that duplicates existing utilities, services, or helpers in the codebase
Flag copy-paste between new test classes or between service methods
Identify repeated if/else or switch blocks that should be polymorphism or a strategy pattern
Note any hardcoded values that appear in multiple places and should be constants or config

Dimension 5: Design & Architecture

Code respects the layering in docs/project.md (e.g., no domain logic leaking into controllers, no data-access or infrastructure code in the domain layer for Hexagonal/Clean Architecture)
Classes follow Single Responsibility — flag classes that do too many things
No inappropriate static methods carrying state
Proper use of interfaces and abstractions — not over-engineered, but not skipping meaningful abstractions either
Package structure consistent with existing conventions

Dimension 6: Performance

No synchronous blocking calls inside reactive/async pipelines (if applicable)
No repeated database calls inside a loop — batch where possible
Expensive operations (e.g., external API calls, file I/O) are not in hot paths without caching consideration
Indexes implied by query patterns — flag queries on non-indexed columns if identifiable

Dimension 7: Test Quality

Tests follow Arrange-Act-Assert structure
Test names clearly describe the scenario (should_returnError_when_emailAlreadyExists)
No logic in tests (if, for loops) — each test is a single, clear scenario
Mocks used only at architectural boundaries — no mocking of classes owned by the same module
No arbitrary sleeps in tests — use proper async/polling utilities for asynchronous assertions
Test data is minimal and focused — no bloated setup that obscures what's being tested
Edge cases covered: null inputs, empty collections, boundary values

Dimension 8: Observability

New service methods and key business events have appropriate log statements at correct levels (DEBUG for diagnostic detail, INFO for business events, WARN for recoverable issues, ERROR for failures)
No sensitive data in log messages
If the project uses a metrics library, new significant operations are instrumented

Output Format

Write the review to review.md in the project root using this structure:

# Code Review: <Feature Name or Path>

## Summary
<2-3 sentence overall assessment. Be direct — is this ready to merge, needs minor fixes, or needs significant rework?>

## Findings

### 🔴 Critical

| Done | Location | Category | Problem | Suggestion |
|------|----------|----------|---------|------------|
| [ ] | `src/service/AuthService.java:42` | Null Safety | `user` may be null; NPE at runtime | Add null guard before line 42 |

### 🟠 Major

| Done | Location | Category | Problem | Suggestion |
|------|----------|----------|---------|------------|

### 🟡 Minor

| Done | Location | Category | Problem | Suggestion |
|------|----------|----------|---------|------------|

### 🔵 Info / Suggestions

| Done | Location | Category | Problem | Suggestion |
|------|----------|----------|---------|------------|

## Acceptance Criteria Coverage
| AC         | Test               | Status          |
|------------|--------------------|-----------------|
| AC-01: ... | `FooTest#test_...` | ✅ Covered       |
| AC-02: ... | —                  | ❌ No test found |

## Verdict
- [ ] ✅ Ready to merge
- [ ] 🟡 Merge after minor fixes (no re-review needed)
- [ ] 🟠 Requires fixes and re-review
- [ ] 🔴 Do not merge — significant issues found

After writing the file, print a one-line confirmation: review.md written. Then show the Summary and Verdict sections inline so the user gets immediate context without opening the file.

After the Review

Ask the user:

"Would you like me to fix any of these findings now? You can say 'fix all critical and major' or call out specific items."

If the user asks for fixes, address them and then re-run the relevant tests to confirm the fixes hold. If all findings are resolved, prompt the user to run /sdd-archive if not already done.

sdd-review