tomevault-io Code Review — Workflow
Structured code review.
Canonical rules — open the instruction files for the layers you touch (agent mode can read them directly):
instructions/java.instructions.md— Java 8 language boundaryinstructions/spring-hibernate.instructions.md— Spring 3.2 + Hibernate 4.2instructions/sql.instructions.md— SQL injection, indexing, JDBC resourcesinstructions/security.instructions.md— OWASP Top 10instructions/jsp.instructions.md— JSP / JSTL, XSSinstructions/xml-config.instructions.md— Spring XML, hbm.xml, Maven POMinstructions/no-heredoc.instructions.md— edit files with tools, not terminal redirection
If you cannot open files, Key rules (fallback for agent chat):
- Java 8: no
var, noList.of(), no records — checked exceptions must be handled or declared - Spring 3.2: XML config +
<tx:advice>only, no@Transactional, no Spring Boot - Hibernate 4.2:
getCurrentSession()only,hbm.xmlmappings, no JPA annotations - SQL (JDBC):
PreparedStatementwith?— zero string concatenation - SQL (HQL): named parameters (
:param) — never concatenate into query strings - Security:
<c:out>for all JSP output;HttpOnly+Securecookie flags
Phase 1 — Understand the Change
- Read the diff / files under review
- Understand the intent: what problem does this solve?
- Check if the approach matches existing patterns
Phase 2 — Review by Category
Correctness: logic errors, off-by-one, null handling, edge cases (empty collections, zero values), concurrency (shared mutable state), resource leaks (unclosed connections)
Security: SQL injection (all queries parameterized?), XSS (all JSP output encoded?), auth (access control on every endpoint?), secrets (no hardcoded credentials?)
Performance: N+1 queries (SQL inside loops)? SELECT * or missing indexes? Unbounded result sets? Expensive ops in hot paths?
Convention Compliance: Java 8 only? getCurrentSession() + hbm.xml + no JPA? <tx:advice> only, no @Transactional? SLF4J parameterized? Proper exception hierarchy?
Maintainability: clear naming? No duplication? Methods ≤30 lines? Comments explain WHY?
Phase 3 — Classify Findings
| Severity | Definition | Action |
|---|---|---|
| 🔴 CRITICAL | Security vuln, data loss, crash | Must fix before merge |
| 🟠 MAJOR | Bug, perf issue, convention violation | Should fix |
| 🟡 MINOR | Style, naming, minor improvement | Nice to fix |
| ⚪ NIT | Preference, trivial | Optional |
Phase 4 — Verdict
Classify all findings, then format using the Output Template below.
Output Template
Per finding: [SEVERITY] Category — description @ file:line → suggestion
## Verdict: APPROVE / REQUEST CHANGES / NEEDS DISCUSSION
Findings: N critical, N major, N minor, N nit
Summary: <one-sentence assessment>
Handoffs
- →
@implementer— to fix findings - →
security-auditskill — security concerns warrant deeper audit - →
sql-reviewskill — SQL issues warrant dedicated review - ←
@reviewer— default activation
Source: zexion7873/copilot-setting — distributed by TomeVault.