feat(reviewing-code): add code review skill

Amolith created 1 month ago

Reviews code for correctness, security, reliability, performance, and
quality with anti-noise rules to reduce false positives.

Assisted-by: Claude Opus 4.5 via Amp

Change summary

README.md                      |  20 ++++-
skills/reviewing-code/SKILL.md | 129 ++++++++++++++++++++++++++++++++++++
2 files changed, 145 insertions(+), 4 deletions(-)

Detailed changes

README.md

@@ -50,6 +50,9 @@ Refer to [§ Token stats](#token-stats) for a detailed breakdown of each skill's
   programming. Covers both full applications and one-off scripts.
 - [resolving-secrets](skills/resolving-secrets/SKILL.md): Wraps shell
   commands to resolve secret references in environment variables.
+- [reviewing-code](skills/reviewing-code/SKILL.md): Reviews code for
+  correctness, security, reliability, performance, and quality. Use when
+  asked to review code, a PR, a diff, or changes.
 
 ## Installation
 
@@ -259,6 +262,15 @@ Token breakdown:
   ───────────────────────────────────────────────
   Total:        2088 tokens
 
+=== reviewing-code ===
+
+Token breakdown:
+  Name:           18 tokens
+  Description:   125 tokens
+  Body:         1880 tokens (121 lines)
+  ───────────────────────────────────────────────
+  Total:        2023 tokens
+
 === scripting-with-go ===
 
 Token breakdown:
@@ -285,10 +297,10 @@ Token breakdown:
 SUMMARY
 ============================================================
 
-Skills: 11
-Metadata: 1543 tokens
-Combined bodies: 16672 tokens
-Overall: 121864 tokens
+Skills: 12
+Metadata: 1686 tokens
+Combined bodies: 18552 tokens
+Overall: 123887 tokens
 Validation errors: 0
 
 Largest skills (by total tokens):

skills/reviewing-code/SKILL.md

@@ -0,0 +1,129 @@
+---
+name: reviewing-code
+description: Reviews code for correctness, security, reliability, performance, and quality. Use when asked to review code, a PR, a diff, changes, or when the user says "review", "code review", or "look over this".
+license: AGPL-3.0-or-later
+metadata:
+  author: Amolith <[e-mail]>
+---
+
+Review code thoroughly, providing constructive and actionable feedback.
+
+## Workflow
+
+1. **Gather context**: Understand intent (PR description, commit message, surrounding code, PR comments or email thread)
+2. **Identify risk areas**: Auth, payments, migrations, concurrency, external integrations
+3. **Analyze systematically**: Prioritize correctness and reliability over style
+4. **Report findings**: Use output format; require evidence for each issue
+
+## Review Areas
+
+### Correctness / Behavior (highest priority)
+
+- Does the change match stated intent? Any unintended behavior changes?
+- Edge cases: null/empty, off-by-one, boundary conditions, unicode/encoding
+- State transitions: can objects reach impossible states?
+- Concurrency: races, ordering assumptions, atomicity, async cancellation
+- Backwards compatibility: API contracts, wire formats, schema changes
+- Error semantics: fail open vs closed? Errors propagated correctly?
+
+### Security
+
+Flag only with concrete evidence (source → sink flow or missing boundary check).
+
+- Input validation and sanitization
+- Authentication and authorization at boundaries
+- Injection: SQL, XSS, command, SSRF, path traversal
+- Insecure deserialization, open redirects
+- Crypto misuse: weak randomness, homegrown crypto, token handling
+- Secrets/credentials: hardcoded or logged
+- Multi-tenant isolation, authorization gaps
+- New dependencies: overly broad permissions, supply chain risk
+
+### Reliability / Operability
+
+- Timeouts, retries, backoff, circuit breaking
+- Idempotency (jobs, webhooks, payments)
+- Resource cleanup: files, sockets, connections, cancellation
+- Logging/metrics: enough to debug? No secrets/PII logged?
+- Configuration: safe defaults, env var handling
+
+### Data / Persistence
+
+- Transaction boundaries, isolation, partial writes
+- Schema/migrations: locking, backfills, rollback safety
+- Query efficiency: N+1, missing indexes, incorrect joins
+
+### Performance
+
+Flag only with clear hotspot evidence (loops over large input, repeated I/O, known N+1).
+
+- Algorithm complexity and bottlenecks
+- Memory allocation patterns
+- Unnecessary computations
+
+### Code Quality
+
+Style issues are **nits**—only mention if they violate local conventions or materially harm readability.
+
+- Naming and style consistency with codebase
+- Function/class size and single responsibility
+- Error handling completeness
+- Dead code, unused imports
+
+### Testing
+
+- Coverage for new/changed code paths
+- Edge cases and error paths tested
+- Test clarity
+
+## Output Format
+
+```markdown
+## 🔴 Must Fix
+
+### [Issue title]
+
+**Location:** `path/to/file.ts:42`
+
+**Impact**
+
+[What breaks or could break]
+
+**Evidence**
+
+[Concrete code path or condition that triggers this]
+
+**Fix**
+
+[Suggested solution]
+
+## 🟠 Should Fix
+
+[Medium confidence or meaningful impact]
+
+## 🟡 Consider
+
+[Trade-offs, optional improvements]
+
+## 💬 Questions
+
+[When intent is unclear: ask, don't assert]
+
+## 📝 Nits
+
+[Style-only, keep brief or omit]
+
+## ✅ Strengths
+
+[What's done well, keep brief]
+```
+
+Omit empty sections. For small clean changes, "Looks good" suffices.
+
+## Anti-Noise Rules
+
+- **Correctness over style**: Don't request refactors unless they reduce bug risk
+- **Evidence required**: Each must-fix needs a concrete trigger condition
+- **Ask vs assert**: When uncertain, ask a question and describe the risk
+- **No hypotheticals**: Avoid speculative performance/security concerns without evidence
+- **Respect local conventions**: Check surrounding code before flagging style