123 lines
4.4 KiB
Markdown
123 lines
4.4 KiB
Markdown
---
|
|
name: code-review-excellence
|
|
description: Systematic code review for correctness, security, performance and maintainability. Produces structured review reports.
|
|
source: community (pinned 2026-03-19)
|
|
---
|
|
|
|
# Code Review Excellence
|
|
|
|
## Overview
|
|
Transform code reviews from rubber-stamping into knowledge sharing through constructive feedback, systematic analysis, and collaborative improvement.
|
|
|
|
## When to Use
|
|
- Reviewing pull requests
|
|
- Establishing code review standards
|
|
- Mentoring developers through review
|
|
- Auditing for correctness, security, or performance
|
|
- Pre-delivery validation against architectural patterns
|
|
|
|
## Instructions
|
|
|
|
### 1. Read Context First
|
|
Before touching code:
|
|
- Read the PR description, linked issues, and requirements
|
|
- Understand the architectural pattern being followed
|
|
- Check if there's a reference implementation to compare against
|
|
- Read test signals (CI status, coverage reports)
|
|
|
|
### 2. Review Systematically
|
|
Check in this order:
|
|
|
|
**Correctness:**
|
|
- Does the code do what the requirements ask?
|
|
- Are edge cases handled?
|
|
- Are error paths covered?
|
|
- Does it match the reference implementation pattern?
|
|
|
|
**Security:**
|
|
- Input validation present? (`@Valid`, `@NotNull`, `@Pattern`)
|
|
- Sensitive data masked in logs? (`authorization`, `customerId`)
|
|
- No secrets hardcoded?
|
|
- Authentication/authorization checks present?
|
|
|
|
**Performance:**
|
|
- No blocking I/O in hot paths (check `@PostConstruct` usage)
|
|
- No N+1 queries or repeated file reads
|
|
- Appropriate timeouts on external calls
|
|
- Async operations use proper context propagation (MDC)
|
|
|
|
**Maintainability:**
|
|
- Clear naming conventions
|
|
- Single responsibility per class
|
|
- Dependencies injected, not constructed
|
|
- Proper layer separation (no business logic in adapters)
|
|
|
|
### 3. Provide Actionable Feedback
|
|
For each issue:
|
|
- State the severity: BLOCKING | IMPORTANT | MINOR
|
|
- Explain WHY it's a problem, not just WHAT is wrong
|
|
- Suggest a concrete fix or alternative
|
|
- Ask clarifying questions when intent is unclear
|
|
|
|
### 4. Acknowledge Good Patterns
|
|
- Call out well-implemented patterns explicitly
|
|
- Recognize clean abstractions and good test coverage
|
|
- Note when the code follows architectural guidelines correctly
|
|
|
|
## Output Format
|
|
|
|
```markdown
|
|
## Code Review Summary
|
|
|
|
### Overall Assessment
|
|
[1-2 sentence summary of the change quality]
|
|
|
|
### Blocking Issues (must fix before merge)
|
|
- **[FILE:LINE]** — [Description] → [Suggested fix]
|
|
|
|
### Important Issues (should fix)
|
|
- **[FILE:LINE]** — [Description] → [Suggested fix]
|
|
|
|
### Minor Issues (nice to have)
|
|
- **[FILE:LINE]** — [Description] → [Suggested fix]
|
|
|
|
### Questions
|
|
- [Clarifying questions about intent or requirements]
|
|
|
|
### Positive Patterns Noted
|
|
- [Good patterns worth highlighting]
|
|
|
|
### Test Coverage
|
|
- [Coverage assessment and gaps]
|
|
```
|
|
|
|
## Review Checklist for V4 BIAN APIs
|
|
|
|
When reviewing an API implementation against `guia-4-implemented-pattern.md`:
|
|
|
|
### REC Layer
|
|
- [ ] Resource only has `@Valid` and delegates to UseCase — no logic
|
|
- [ ] Request DTOs use `@NotNull(message = "VDE01")` and `@Valid` on nested objects
|
|
- [ ] BusXxxClient is an **interface** with `@RegisterRestClient(configKey = "...")`
|
|
- [ ] BusXxxAdapter injects with `@RestClient` and reads `agencyCode` from MDC
|
|
- [ ] `ValidationExceptionMapper` present in `common/infrastructure/interceptor/`
|
|
- [ ] `RestClientExceptionMapper` present and registered via `@RegisterProvider`
|
|
- [ ] `InboundLoggingFilter` uses `SequenceInputStream` to reconstruct the stream
|
|
|
|
### BUS Layer
|
|
- [ ] Service orchestrates Ports — does not implement HTTP directly
|
|
- [ ] Service availability checked **before** business logic
|
|
- [ ] Security trace executes in `finally` using `AsyncMdcRunner.run()`
|
|
- [ ] DomXxxClient has `@RegisterProvider(RestClientLoggingFilter.class)` and `@RegisterProvider(RestClientExceptionMapper.class)`
|
|
- [ ] Mapper uses `@Mapper(componentModel = "cdi")` with `@AfterMapping` for complex fields
|
|
- [ ] `MessageHelper` uses `@PostConstruct` — no file reading in constructor
|
|
- [ ] Error responses always include `status = "error"` and `traceId`
|
|
|
|
## Common Review Anti-Patterns
|
|
| Anti-Pattern | Better Approach |
|
|
|---|---|
|
|
| "Looks good to me" (LGTM without review) | Run through the checklist systematically |
|
|
| Nitpicking style while ignoring logic bugs | Prioritize correctness over formatting |
|
|
| Rewriting the PR in comments | Suggest one concrete alternative, not a full rewrite |
|
|
| Blocking on subjective preferences | Only block on correctness, security, performance |
|