|
| 1 | +# Skill: Branch Review |
| 2 | + |
| 3 | +## Purpose |
| 4 | +Review the current branch against `develop` and report only material issues with high signal: bugs, regressions, API breaks, resource/concurrency risks, and behavior changes. |
| 5 | + |
| 6 | +## When to Use |
| 7 | +- Before opening a pull request to `develop` |
| 8 | +- After major refactoring or API changes |
| 9 | +- When you want a strict, impact-focused review |
| 10 | + |
| 11 | +## Review Scope (RLib-specific) |
| 12 | +1. **Public API compatibility first** |
| 13 | + - Treat removals/signature changes in public API as high priority by default |
| 14 | + - If branch is intentionally alpha-breaking, mark these as accepted and continue |
| 15 | + |
| 16 | +2. **Prioritize API surface over internals** |
| 17 | + - Start with changed files in `src/main/java` |
| 18 | + - Prioritize non-`impl/` packages |
| 19 | + - Review `impl/` changes when they can affect exposed behavior |
| 20 | + |
| 21 | +3. **Javadoc quality is part of API review** |
| 22 | + - Check public API javadocs for: |
| 23 | + - `@since 10.0.0` |
| 24 | + - Active voice descriptions |
| 25 | + - No trailing periods in `@param`/`@return` |
| 26 | + - No unnecessary getter/setter javadocs |
| 27 | + |
| 28 | +4. **Collections performance pattern** |
| 29 | + - Verify empty-state fast paths (`isEmpty()`) in collection operations |
| 30 | + - Ensure no unnecessary allocation/iteration when collections are empty |
| 31 | + |
| 32 | +5. **Runtime/integration constraints** |
| 33 | + - Many integration tests use Testcontainers |
| 34 | + - If Docker is unavailable, call out that integration validation is limited |
| 35 | + |
| 36 | +6. **Testing conventions awareness** |
| 37 | + - Prefer AssertJ assertions |
| 38 | + - Keep test naming and style aligned with repo conventions |
| 39 | + |
| 40 | +## Process |
| 41 | + |
| 42 | +### 1. Gather branch diff |
| 43 | +```bash |
| 44 | +git --no-pager diff --name-only develop...HEAD |
| 45 | +git --no-pager diff --stat develop...HEAD |
| 46 | +git --no-pager diff develop...HEAD |
| 47 | +``` |
| 48 | + |
| 49 | +### 2. Triage files |
| 50 | +- Group into: |
| 51 | + - Public API changes |
| 52 | + - Internal behavior/refactoring |
| 53 | + - Documentation-only changes |
| 54 | + |
| 55 | +### 3. Review by risk |
| 56 | +- Public API contract changes |
| 57 | +- Behavioral logic changes |
| 58 | +- Concurrency/resource handling |
| 59 | +- Performance-sensitive paths |
| 60 | +- Documentation contract gaps (for public API) |
| 61 | + |
| 62 | +### 4. Report findings |
| 63 | +Use this format: |
| 64 | +- **Severity:** High / Medium / Low |
| 65 | +- **File:** path |
| 66 | +- **Issue:** concise technical problem |
| 67 | +- **Impact:** why it matters in real usage |
| 68 | +- **Fix:** concrete recommendation |
| 69 | + |
| 70 | +Only include findings that are actionable and materially important. |
| 71 | + |
| 72 | +## Output Expectations |
| 73 | +- If no material issues: explicitly state no material issues found |
| 74 | +- If issues exist: provide a short prioritized list |
| 75 | +- Do not include style-only or trivial nits |
0 commit comments