|
| 1 | +--- |
| 2 | +name: review |
| 3 | +description: Review code changes in PRs or local diffs. Use when asked to review a PR, inspect changes, or provide feedback on code quality. |
| 4 | +--- |
| 5 | + |
| 6 | +# Code Review Skill — S3Mock |
| 7 | + |
| 8 | +Read `AGENTS.md` (root + relevant module) before reviewing — especially DO/DON'T, Code Style, and Architecture sections. |
| 9 | + |
| 10 | +## Review Scope |
| 11 | + |
| 12 | +Evaluate changes against these categories, in priority order: |
| 13 | + |
| 14 | +### 1. Correctness |
| 15 | +- Does the code do what it claims? Check edge cases and error paths. |
| 16 | +- Do XML element/attribute names match the [AWS S3 API](https://docs.aws.amazon.com/AmazonS3/latest/API/Welcome.html) exactly? |
| 17 | +- Are `S3Exception` constants used correctly (not new exception classes)? |
| 18 | +- Does the layering hold? Business logic in services, HTTP mapping in controllers, persistence in stores. |
| 19 | + |
| 20 | +### 2. Convention Violations (from AGENTS.md DO/DON'T) |
| 21 | +- `@Autowired` or field injection in production code |
| 22 | +- `@Mock` / `@InjectMocks` / `@ExtendWith(MockitoExtension::class)` instead of `@SpringBootTest` + `@MockitoBean` |
| 23 | +- `var` on public API properties |
| 24 | +- AWS SDK v1 usage |
| 25 | +- Apache Commons dependencies where Kotlin stdlib suffices |
| 26 | +- Dependency versions declared in sub-module POMs |
| 27 | +- Business logic in controllers or raw string responses |
| 28 | +- Legacy `testSomething` naming in new or touched tests |
| 29 | + |
| 30 | +### 3. Test Quality |
| 31 | +- Are new/changed code paths covered by tests? |
| 32 | +- Unit tests (`*Test.kt`): extend correct base class (`ServiceTestBase`, `StoreTestBase`, `BaseControllerTest`)? |
| 33 | +- Integration tests (`*IT.kt`): extend `S3TestBase`, use `givenBucket(testInfo)`? |
| 34 | +- Test independence: no shared state, UUID bucket names? |
| 35 | +- Backtick names, `internal class`, AssertJ assertions? |
| 36 | + |
| 37 | +### 4. Kotlin Idioms |
| 38 | +- Null safety (`?.`, `?:`) over explicit null checks |
| 39 | +- Expression bodies for single-expression functions |
| 40 | +- Named `it` in nested/non-trivial lambdas |
| 41 | +- `when` over long if-else chains |
| 42 | +- `isEmpty()`/`isNotEmpty()` over size checks |
| 43 | +- String templates over concatenation |
| 44 | + |
| 45 | +### 5. Documentation & Changelog |
| 46 | +- Is `CHANGELOG.md` updated for user-facing changes? |
| 47 | +- Is `README.md` operations table updated for new S3 operations? |
| 48 | +- Do public APIs have KDoc? |
| 49 | + |
| 50 | +## Review Output Format |
| 51 | + |
| 52 | +Structure feedback as: |
| 53 | + |
| 54 | +- **Must fix** — blocks merge (correctness issues, convention violations, missing tests) |
| 55 | +- **Should fix** — strongly recommended (idiom improvements, missing docs) |
| 56 | +- **Nit** — optional style suggestions |
| 57 | + |
| 58 | +For each finding, reference the specific AGENTS.md rule or AWS API doc where applicable. |
| 59 | + |
| 60 | +## Checklist |
| 61 | + |
| 62 | +- [ ] Read root + relevant module `AGENTS.md` |
| 63 | +- [ ] Check all categories above in priority order |
| 64 | +- [ ] Verify CI gates will pass (ktlint, checkstyle, tests, Docker build) |
| 65 | +- [ ] Confirm `CHANGELOG.md` is updated if needed |
| 66 | +- [ ] Provide actionable feedback with specific file/line references |
0 commit comments