| name | review |
| description | Review code changes in pull requests or local diffs for S3Mock. Use when asked to review a PR, inspect changes, or provide structured feedback on correctness, conventions, test quality, and documentation. Outputs findings structured as Must fix / Should fix / Nit. |
Code Review Skill
Entry Criteria
Use this skill when asked to:
- Review a pull request
- Inspect a local diff or branch
- Provide structured feedback on code quality
Before Starting
Read AGENTS.md (root) and the relevant module AGENTS.md — especially the DO/DON'T, Architecture, and Code Style sections.
Review Scope
Evaluate in this priority order:
1. Correctness
- Does the code do what it claims? Check edge cases and error paths.
- Do XML element/attribute names match the AWS S3 API exactly? See
INVARIANTS.md.
- Are
S3Exception constants used — not custom exception classes? See INVARIANTS.md.
- Does layering hold — business logic only in services, HTTP mapping only in controllers, persistence only in stores? See
INVARIANTS.md.
2. Convention Violations
Flag as Must fix:
- Copyright header not updated to
2017-<current year> in every modified file
- AWS SDK v1 usage
- JUnit 4 usage
- Dependency versions declared in sub-module POMs
- Business logic in controllers; raw string responses from controllers
- Controllers catching exceptions
- Legacy Jackson XML annotations from
com.fasterxml.jackson.dataformat.xml.annotation — must use tools.jackson packages in 5.x
@Autowired or field injection in production code (see docs/SPRING.md)
@Mock/@InjectMocks/@ExtendWith(MockitoExtension::class) in tests — use @SpringBootTest + @MockitoBean (see docs/SPRING.md)
var on public API properties (see docs/KOTLIN.md)
- Apache Commons usage where Kotlin stdlib suffices
3. Test Quality
- Are new/changed code paths covered by tests?
- Correct base class used (
ServiceTestBase, StoreTestBase, BaseControllerTest, or S3TestBase)?
- No shared state;
testInfo-based or UUID-based resource names?
- Backtick names,
internal class, AssertJ assertions?
- Integration tests use
givenBucket(testInfo)?
4. Kotlin Idioms
See docs/KOTLIN.md for the full list of idioms and anti-patterns to check (null safety, expression bodies, scope functions, isEmpty()/isNotEmpty(), when expressions, naming).
5. Documentation
- Is
CHANGELOG.md updated for user-facing changes?
- Is the
README.md operations table updated for new S3 operations?
- Do new public APIs have KDoc?
Output Format
Structure feedback as:
- Must fix — blocks merge: correctness issues,
INVARIANTS.md constraint violations, missing tests
- Should fix — strongly recommended: idiom improvements, missing docs. Suggest invoking the
refactor skill for purely style improvements rather than asking the author to add more code.
- Nit — optional style suggestions
For each finding, reference the specific AGENTS.md rule, INVARIANTS.md constraint, or AWS API doc.
Execution Steps
- Read root + relevant module
AGENTS.md.
- Evaluate all five categories above in priority order.
- If style issues found, suggest the author invoke the
lint skill.
- If readability-only issues found, suggest the author invoke the
refactor skill.
- Confirm
CHANGELOG.md is updated if the change is user-facing.
- Produce structured output using the Must fix / Should fix / Nit format with file/line references.
Completion Criteria
Resources