chore: bootstrap skills library — 19 skills + installer + CI auto-tag
Some checks failed
release / tag (push) Has been cancelled
Some checks failed
release / tag (push) Has been cancelled
Phase 1 of mathias/skills extraction (infra#62 Track D — homelab next-step plan addendum). Imports ~/dev/.skills/ verbatim (19 skill dirs + SKILLS_INDEX.md) and adds the installation surface: - Taskfile.yml — install / update / list / release / check targets - install.sh — bootstrap installer for hosts without Task. Idempotent symlink wirer; default checkout at ~/.local/share/skills/ on every host; SKILLS_REF env var pins a tag (default: main). - .gitea/workflows/release.yml — auto-tag every push to main by Bump-Type footer (major/minor/patch, default patch). Skipped when commit contains [skip-release]. - README — usage, versioning, contribution flow, secret-hygiene rule. Phase 1 wires Claude Code only (~/.claude/skills/<name> global + <repo>/.claude/skills/<name> per-repo). Phase 2 adds Crush, opencode, antigravity, and gitea-resident agents (cobalt-dingo, agentsquad) once their skill conventions are researched. Public repo, markdown-only — no secrets, no client names. Verified via pre-push grep before initial push. [skip-release]
This commit is contained in:
Mathias
284
code-review/SKILL.md
Normal file
284
code-review/SKILL.md
Normal file
@@ -0,0 +1,284 @@
|
||||
---
|
||||
name: code-review
|
||||
description: Systematic code smell detection and Go-specific review checklist. Load before any pre-merge review.
|
||||
---
|
||||
|
||||
# Code Review
|
||||
|
||||
## Overview
|
||||
|
||||
Code review is smell detection and principle verification, not style policing. The goal is to identify patterns that make code hard to understand, change, test, or safely operate.
|
||||
|
||||
**This skill is read-only analysis.** When you find issues, document them with specific file:line references and prioritize by architectural impact. For fixes, load the `refactoring` or `clean-code` skill.
|
||||
|
||||
## When to Use
|
||||
|
||||
- Pre-merge review of any pull request
|
||||
- Before committing a significant change
|
||||
- When asked to "review this code"
|
||||
- After the GREEN phase of TDD (use with `clean-code` skill)
|
||||
|
||||
## Review Process
|
||||
|
||||
### Phase 0: Iron Laws (block on any violation)
|
||||
|
||||
Before reading the diff, scan for these. Any violation is a blocking finding regardless of how clean the rest of the change looks.
|
||||
|
||||
1. **No security vulnerabilities** — command injection, SQL injection, credential exposure, path traversal, unchecked input at system boundaries
|
||||
2. **No silently swallowed errors** — `err != nil` without wrapping, logging, or surfacing is always wrong; `_ = f()` on a fallible call is wrong
|
||||
3. **No missing validation at system boundaries** — user input, external API responses, and file reads must be validated before use
|
||||
|
||||
If any iron law is violated, flag it as **Blocking** before any other phase. The other phases still run, but the iron-law findings come first in the report. See Phase 3 for detailed patterns and code examples covering iron laws 2 and 3.
|
||||
|
||||
### Phase 1: Context
|
||||
|
||||
Before examining code:
|
||||
1. Understand what the change is trying to accomplish
|
||||
2. Identify the affected packages and their purpose
|
||||
3. Check test coverage — is the behavior tested?
|
||||
4. Read the commit message and PR description
|
||||
|
||||
### Phase 2: Architectural Analysis (Highest Priority)
|
||||
|
||||
Check for structural problems first. These are expensive to fix later.
|
||||
|
||||
**Dependency violations:**
|
||||
- Does domain logic import infrastructure packages?
|
||||
- Are there circular imports between packages?
|
||||
- Does a type do things that multiple different stakeholders would request changes to?
|
||||
|
||||
**Abstraction problems:**
|
||||
- Is there a large switch/if-else on a type string that should be an interface?
|
||||
- Are there God types that know too much about too many things?
|
||||
- Is there Shotgun Surgery — one logical change requires editing many files?
|
||||
|
||||
### Phase 3: Go-Specific Checklist
|
||||
|
||||
#### Error Handling
|
||||
|
||||
```go
|
||||
// Required: wrap with context
|
||||
return fmt.Errorf("parse invoice: %w", err)
|
||||
|
||||
// Forbidden: naked return
|
||||
return err
|
||||
|
||||
// Forbidden: log and return (pick one)
|
||||
log.Error("failed", "err", err)
|
||||
return err
|
||||
|
||||
// Forbidden: silently ignore
|
||||
_ = doSomething()
|
||||
|
||||
// Suspicious: sentinel errors without documentation
|
||||
var ErrFoo = errors.New("foo") // Is this exported? Is it documented?
|
||||
```
|
||||
|
||||
#### Goroutine Safety
|
||||
|
||||
- Is shared state protected by a mutex or accessed only via channels?
|
||||
- Are goroutines guaranteed to terminate (no goroutine leaks)?
|
||||
- Is `sync.WaitGroup` used correctly (Add before go, Done via defer)?
|
||||
- Is there a `go test -race` passing?
|
||||
|
||||
```go
|
||||
// Check: is this safe for concurrent use?
|
||||
type Cache struct {
|
||||
mu sync.RWMutex // Good: explicit lock
|
||||
items map[string]Item
|
||||
}
|
||||
|
||||
// Red flag: map access without lock in concurrent context
|
||||
type BadCache struct {
|
||||
items map[string]Item // Unsafe if accessed from multiple goroutines
|
||||
}
|
||||
```
|
||||
|
||||
#### Exported API Minimalism
|
||||
|
||||
- Only export what callers outside the package actually need
|
||||
- Internal implementation details should be unexported
|
||||
- Does the exported API make sense as a standalone contract?
|
||||
|
||||
```go
|
||||
// Good: minimal exported surface
|
||||
type UserService struct { /* unexported fields */ }
|
||||
func NewUserService(...) *UserService { ... }
|
||||
func (s *UserService) GetUser(ctx context.Context, id UserID) (User, error) { ... }
|
||||
|
||||
// Bad: over-exported internals
|
||||
type UserService struct {
|
||||
Store UserStore // Exported field — callers can replace it, breaks encapsulation
|
||||
Logger *slog.Logger // Exported — callers can mutate it
|
||||
}
|
||||
```
|
||||
|
||||
#### Dependency Safety
|
||||
|
||||
Before adding any new dependency:
|
||||
- Has `govulncheck` been run? (`govulncheck ./...`)
|
||||
- Is there a justification in the commit message for non-stdlib additions?
|
||||
- Pre-approved without justification: `testify`, `slog`, `templ`, `sqlc`
|
||||
- Everything else needs a reason
|
||||
|
||||
#### Generic/interface{} Usage
|
||||
|
||||
```go
|
||||
// Bad: any/interface{} when a concrete type or generic works
|
||||
func Process(data interface{}) error { ... }
|
||||
|
||||
// Good: typed
|
||||
func Process[T Processor](data T) error { ... }
|
||||
|
||||
// Good: concrete type when only one type is expected
|
||||
func ProcessOrder(o *Order) error { ... }
|
||||
```
|
||||
|
||||
#### Context Propagation
|
||||
|
||||
- Every function that does I/O (DB, HTTP, file) should accept `ctx context.Context` as the first parameter
|
||||
- Context should be passed down, not stored in structs
|
||||
- `context.Background()` should only appear at the top-level (main, test setup)
|
||||
|
||||
```go
|
||||
// Good
|
||||
func (s *Store) GetUser(ctx context.Context, id UserID) (User, error) { ... }
|
||||
|
||||
// Bad: no context — can't cancel, can't trace, can't timeout
|
||||
func (s *Store) GetUser(id UserID) (User, error) { ... }
|
||||
|
||||
// Bad: context stored in struct
|
||||
type Handler struct {
|
||||
ctx context.Context // Don't do this
|
||||
}
|
||||
```
|
||||
|
||||
#### Logging with slog
|
||||
|
||||
```go
|
||||
// Good: structured, leveled
|
||||
slog.InfoContext(ctx, "user created", "user_id", u.ID, "email", u.Email)
|
||||
|
||||
// Bad: unstructured
|
||||
log.Printf("user created: %s", u.Email)
|
||||
|
||||
// Bad: logging errors that are also returned
|
||||
slog.Error("failed to save", "err", err)
|
||||
return fmt.Errorf("save user: %w", err) // caller is double-counting
|
||||
```
|
||||
|
||||
### Phase 4: Code Smell Detection
|
||||
|
||||
Apply the catalog from `clean-code/references/code-smells.md`. Key smells to prioritize:
|
||||
|
||||
**Architectural impact (fix immediately):**
|
||||
- Global Data — package-level variables modified at runtime
|
||||
- Shotgun Surgery — one change touches many unrelated files
|
||||
- Feature Envy — a method uses another package's internals more than its own
|
||||
- God Object — a type that does everything
|
||||
|
||||
**Design impact (fix before merge if possible):**
|
||||
- Long Method — functions > ~50 lines in Go (Go can be longer than OOP, but be honest)
|
||||
- Long Parameter List — > 4-5 parameters; consider a config struct
|
||||
- Duplicate Code — same logic copy-pasted (wait for Rule of Three)
|
||||
- Message Chain — `a.B().C().D()` — violates Law of Demeter
|
||||
- Primitive Obsession — using `string` for email, user ID, currency
|
||||
|
||||
**Readability (note in review, fix if cheap):**
|
||||
- Magic Numbers — bare literals without named constants
|
||||
- Uncommunicative Names — `data`, `info`, `x`, `tmp`
|
||||
- Fallacious Comments — comments that don't match the code
|
||||
|
||||
### Phase 5: Test Quality
|
||||
|
||||
- Are new behaviors covered by tests?
|
||||
- Are tests table-driven where appropriate?
|
||||
- Do tests test behavior, not implementation?
|
||||
- Are there mock-only tests that test nothing real?
|
||||
|
||||
For comprehensive test review, load the `test-design` skill.
|
||||
|
||||
## Severity Classification
|
||||
|
||||
| Severity | Description | Action |
|
||||
|----------|-------------|--------|
|
||||
| **Blocking** | Security issue, data corruption risk, broken behavior | Must fix before merge |
|
||||
| **High** | Architectural violation, goroutine safety, missing context | Fix before merge |
|
||||
| **Medium** | Design smell, missing tests for changed behavior | Fix in this PR or create issue |
|
||||
| **Low** | Naming, style, minor duplication | Note in review, optional |
|
||||
|
||||
## Output Format
|
||||
|
||||
When documenting findings:
|
||||
|
||||
```markdown
|
||||
## Code Review: [package/file]
|
||||
|
||||
### Blocking
|
||||
- `internal/store/user.go:47`: Error swallowed with `_ =` — callers can't know if Save failed
|
||||
|
||||
### High
|
||||
- `internal/handler/order.go:12`: Domain logic in HTTP handler — moves tax calculation out of service layer
|
||||
- `cmd/server/main.go:89`: Missing context propagation — `GetUser` call has no timeout
|
||||
|
||||
### Medium
|
||||
- `internal/service/invoice.go:34`: Long method (87 lines) — extract `calculateLineItems` and `applyTaxRules`
|
||||
|
||||
### Low
|
||||
- `internal/domain/user.go:5`: Field `Data string` — name reveals nothing about what data
|
||||
```
|
||||
|
||||
## Brain MCP Integration
|
||||
|
||||
The brain MCP holds prior review decisions across sessions. Use it to avoid re-litigating the same tradeoffs.
|
||||
|
||||
**At review start:**
|
||||
- Run `brain_query` with the package or feature name + "review" to surface prior review findings, recurring smells, or architectural decisions for this area. This prevents flagging the same smell that was already accepted as a tradeoff.
|
||||
|
||||
**After review completes:**
|
||||
- For findings the author accepts as a tradeoff (with rationale), run `brain_write` to record the decision. Future reviews will not re-flag it.
|
||||
|
||||
**Never:**
|
||||
- Embed brain queries in the review output. They are context for you, not findings for the author.
|
||||
|
||||
### Logging
|
||||
|
||||
Call `session_log` once at the end of every phase to record the outcome.
|
||||
Pass-rate is computed downstream by the `/pass-rate` HTTP endpoint, which
|
||||
treats `pass` as success, `fail` as failure, `skip` as neither.
|
||||
|
||||
**At end of each phase:**
|
||||
- `session_log` with `{skill: "code-review", phase: "<phase-name>", final_status: "pass" | "fail" | "skip", message: "<one-line summary>", duration_ms: <wall-clock>, project_root: "<absolute path>"}`
|
||||
|
||||
**Phases for this skill:** phase-0-iron-laws, phase-1-context, phase-2-architectural-analysis, phase-3-go-checklist, phase-4-code-smell-detection, phase-5-test-quality
|
||||
|
||||
**Status semantics:**
|
||||
- `pass` — the phase's intended outcome was reached.
|
||||
- `fail` — the phase's intended outcome was NOT reached.
|
||||
- `skip` — phase was skipped intentionally.
|
||||
|
||||
**Why this matters:** the routing pod (Plan 6) reads pass-rate to decide whether to route a future call to a local model. If your skill never logs, the routing pod sees no data.
|
||||
|
||||
## Common Go Anti-Patterns
|
||||
|
||||
| Anti-Pattern | What to Look For |
|
||||
|-------------|-----------------|
|
||||
| Goroutine leak | `go func()` without a done channel or WaitGroup |
|
||||
| Error ignored | `_ = f()` or missing `if err != nil` after a fallible call |
|
||||
| Nil map write | `m[k] = v` without `m = make(map[...]{})` initialization |
|
||||
| Race on map | Concurrent map read/write without mutex |
|
||||
| ctx in struct | `type T struct { ctx context.Context }` |
|
||||
| Naked panic | `panic("something went wrong")` in non-initialization code |
|
||||
| fmt.Println in library | Use `slog` or accept a logger; never print to stdout from lib code |
|
||||
| init() abuse | `init()` with side effects that make testing hard |
|
||||
|
||||
## References
|
||||
|
||||
- `clean-code/references/code-smells.md` — comprehensive smell catalog
|
||||
- Load `refactoring` skill when recommending fixes
|
||||
- Load `test-design` skill for reviewing test quality
|
||||
- Load `solid` skill for SOLID principle violations
|
||||
|
||||
## Mode 2 Routing Note
|
||||
|
||||
This skill is invoked identically in Mode 1 (cloud Claude) and Mode 2 (client-local with supervisor routing). The routing layer (Plan 6) does not exist yet; treat this skill as Mode 1 only for now. Findings format and severity classification are unchanged across modes.
|
||||
Reference in New Issue
Block a user