skills/software-architecture/SKILL.md

@@ -0,0 +1,495 @@
+---
+name: software-architecture
+description: Software architecture best practices, patterns, and review guidance. Foundation for architecture-related agents and commands including repo audits, issue refinement, and PR reviews.
+user-invocable: false
+---
+
+# Software Architecture
+
+Best practices, patterns, and review guidance for software architecture. This skill serves as the knowledge base for architecture-related agents and commands.
+
+## Go-Specific Best Practices
+
+### Package Organization
+
+**Good package structure:**
+```
+project/
+├── cmd/                    # Application entry points
+│   └── server/
+│       └── main.go
+├── internal/               # Private packages
+│   ├── domain/             # Core business logic
+│   │   ├── user/
+│   │   └── order/
+│   ├── service/            # Application services
+│   ├── repository/         # Data access
+│   └── handler/            # HTTP/gRPC handlers
+├── pkg/                    # Public, reusable packages
+└── go.mod
+```
+
+**Package naming:**
+- Short, concise, lowercase: `user`, `order`, `auth`
+- Avoid generic names: `util`, `common`, `helpers`, `misc`
+- Name after what it provides, not what it contains
+- One package per concept, not per file
+
+**Package cohesion:**
+- A package should have a single, focused responsibility
+- Package internal files can use internal types freely
+- Minimize exported types - export interfaces, hide implementations
+
+### Interfaces
+
+**Accept interfaces, return structs:**
+```go
+// Good: Accept interface, return concrete type
+func NewUserService(repo UserRepository) *UserService {
+    return &UserService{repo: repo}
+}
+
+// Bad: Accept and return interface
+func NewUserService(repo UserRepository) UserService {
+    return &userService{repo: repo}
+}
+```
+
+**Define interfaces at point of use:**
+```go
+// Good: Interface defined where it's used (consumer owns the interface)
+package service
+
+type UserRepository interface {
+    FindByID(ctx context.Context, id string) (*User, error)
+}
+
+// Bad: Interface defined with implementation (producer owns the interface)
+package repository
+
+type UserRepository interface {
+    FindByID(ctx context.Context, id string) (*User, error)
+}
+```
+
+**Keep interfaces small:**
+- Prefer single-method interfaces
+- Large interfaces indicate missing abstraction
+- Compose small interfaces when needed
+
+### Error Handling
+
+**Wrap errors with context:**
+```go
+// Good: Wrap with context
+if err != nil {
+    return fmt.Errorf("fetching user %s: %w", id, err)
+}
+
+// Bad: Return bare error
+if err != nil {
+    return err
+}
+```
+
+**Use sentinel errors for expected conditions:**
+```go
+var ErrNotFound = errors.New("not found")
+var ErrConflict = errors.New("conflict")
+
+// Check with errors.Is
+if errors.Is(err, ErrNotFound) {
+    // handle not found
+}
+```
+
+**Error types for rich errors:**
+```go
+type ValidationError struct {
+    Field   string
+    Message string
+}
+
+func (e *ValidationError) Error() string {
+    return fmt.Sprintf("%s: %s", e.Field, e.Message)
+}
+
+// Check with errors.As
+var valErr *ValidationError
+if errors.As(err, &valErr) {
+    // handle validation error
+}
+```
+
+### Dependency Injection
+
+**Constructor injection:**
+```go
+type UserService struct {
+    repo   UserRepository
+    logger Logger
+}
+
+func NewUserService(repo UserRepository, logger Logger) *UserService {
+    return &UserService{
+        repo:   repo,
+        logger: logger,
+    }
+}
+```
+
+**Wire dependencies in main:**
+```go
+func main() {
+    // Create dependencies
+    db := database.Connect()
+    logger := slog.Default()
+
+    // Wire up services
+    userRepo := repository.NewUserRepository(db)
+    userService := service.NewUserService(userRepo, logger)
+    userHandler := handler.NewUserHandler(userService)
+
+    // Start server
+    http.Handle("/users", userHandler)
+    http.ListenAndServe(":8080", nil)
+}
+```
+
+**Avoid global state:**
+- No `init()` for service initialization
+- No package-level variables for dependencies
+- Pass context explicitly, don't store in structs
+
+### Testing
+
+**Table-driven tests:**
+```go
+func TestUserService_Create(t *testing.T) {
+    tests := []struct {
+        name    string
+        input   CreateUserInput
+        want    *User
+        wantErr error
+    }{
+        {
+            name:  "valid user",
+            input: CreateUserInput{Email: "test@example.com"},
+            want:  &User{Email: "test@example.com"},
+        },
+        {
+            name:    "invalid email",
+            input:   CreateUserInput{Email: "invalid"},
+            wantErr: ErrInvalidEmail,
+        },
+    }
+
+    for _, tt := range tests {
+        t.Run(tt.name, func(t *testing.T) {
+            // arrange, act, assert
+        })
+    }
+}
+```
+
+**Test doubles:**
+- Use interfaces for test doubles
+- Prefer hand-written mocks over generated ones for simple cases
+- Use `testify/mock` or `gomock` for complex mocking needs
+
+**Test package naming:**
+- `package user_test` for black-box testing (preferred)
+- `package user` for white-box testing when needed
+
+## Generic Architecture Patterns
+
+### Layered Architecture
+
+```
+┌─────────────────────────────────┐
+│         Presentation            │  HTTP handlers, CLI, gRPC
+├─────────────────────────────────┤
+│         Application             │  Use cases, orchestration
+├─────────────────────────────────┤
+│           Domain                │  Business logic, entities
+├─────────────────────────────────┤
+│        Infrastructure           │  Database, external services
+└─────────────────────────────────┘
+```
+
+**Rules:**
+- Dependencies point downward only
+- Upper layers depend on interfaces, not implementations
+- Domain layer has no external dependencies
+
+### SOLID Principles
+
+**Single Responsibility (S):**
+- Each module has one reason to change
+- Split code that changes for different reasons
+
+**Open/Closed (O):**
+- Open for extension, closed for modification
+- Add new behavior through new types, not changing existing ones
+
+**Liskov Substitution (L):**
+- Subtypes must be substitutable for their base types
+- Interfaces should be implementable without surprises
+
+**Interface Segregation (I):**
+- Clients shouldn't depend on interfaces they don't use
+- Prefer many small interfaces over few large ones
+
+**Dependency Inversion (D):**
+- High-level modules shouldn't depend on low-level modules
+- Both should depend on abstractions
+
+### Dependency Direction
+
+```
+                    ┌──────────────┐
+                    │    Domain    │
+                    │  (no deps)   │
+                    └──────────────┘
+                           ▲
+              ┌────────────┴────────────┐
+              │                         │
+      ┌───────┴───────┐         ┌───────┴───────┐
+      │  Application  │         │Infrastructure │
+      │ (uses domain) │         │(implements    │
+      └───────────────┘         │ domain intf)  │
+              ▲                 └───────────────┘
+              │
+      ┌───────┴───────┐
+      │ Presentation  │
+      │(calls app)    │
+      └───────────────┘
+```
+
+**Key insight:** Infrastructure implements domain interfaces, doesn't define them. This inverts the "natural" dependency direction.
+
+### Module Boundaries
+
+**Signs of good boundaries:**
+- Modules can be understood in isolation
+- Changes are localized within modules
+- Clear, minimal public API
+- Dependencies flow in one direction
+
+**Signs of bad boundaries:**
+- Circular dependencies between modules
+- "Shotgun surgery" - small changes require many file edits
+- Modules reach into each other's internals
+- Unclear ownership of concepts
+
+## Repository Health Indicators
+
+### Positive Indicators
+
+| Indicator | What to Look For |
+|-----------|------------------|
+| Clear structure | Obvious package organization, consistent naming |
+| Small interfaces | Most interfaces have 1-3 methods |
+| Explicit dependencies | Constructor injection, no globals |
+| Test coverage | Unit tests for business logic, integration tests for boundaries |
+| Error handling | Wrapped errors, typed errors for expected cases |
+| Documentation | CLAUDE.md accurate, code comments explain "why" |
+
+### Warning Signs
+
+| Indicator | What to Look For |
+|-----------|------------------|
+| God packages | `utils/`, `common/`, `helpers/` with 20+ files |
+| Circular deps | Package A imports B, B imports A |
+| Deep nesting | 4+ levels of directory nesting |
+| Huge files | Files with 500+ lines |
+| Interface pollution | Interfaces for everything, even single implementations |
+| Global state | Package-level variables, `init()` for setup |
+
+### Metrics to Track
+
+- **Package fan-out:** How many packages does each package import?
+- **Cyclomatic complexity:** How complex are the functions?
+- **Test coverage:** What percentage of code is tested?
+- **Import depth:** How deep is the import tree?
+
+## Review Checklists
+
+### Repository Audit Checklist
+
+Use this when evaluating overall repository health.
+
+**Structure:**
+- [ ] Clear package organization following Go conventions
+- [ ] No circular dependencies between packages
+- [ ] Appropriate use of `internal/` for private packages
+- [ ] `cmd/` for application entry points
+
+**Dependencies:**
+- [ ] Dependencies flow inward (toward domain)
+- [ ] Interfaces defined at point of use (not with implementation)
+- [ ] No global state or package-level dependencies
+- [ ] Constructor injection throughout
+
+**Code Quality:**
+- [ ] Consistent naming conventions
+- [ ] No "god" packages (utils, common, helpers)
+- [ ] Errors wrapped with context
+- [ ] Small, focused interfaces
+
+**Testing:**
+- [ ] Unit tests for domain logic
+- [ ] Integration tests for boundaries (DB, HTTP)
+- [ ] Tests are readable and maintainable
+- [ ] Test coverage for critical paths
+
+**Documentation:**
+- [ ] CLAUDE.md is accurate and helpful
+- [ ] vision.md explains the product purpose
+- [ ] Code comments explain "why", not "what"
+
+### Issue Refinement Checklist
+
+Use this when reviewing issues for architecture impact.
+
+**Scope:**
+- [ ] Issue is a vertical slice (user-visible value)
+- [ ] Changes are localized to specific packages
+- [ ] No cross-cutting concerns hidden in implementation
+
+**Design:**
+- [ ] Follows existing patterns in the codebase
+- [ ] New abstractions are justified
+- [ ] Interface changes are backward compatible (or breaking change is documented)
+
+**Dependencies:**
+- [ ] New dependencies are minimal and justified
+- [ ] No new circular dependencies introduced
+- [ ] Integration points are clearly defined
+
+**Testability:**
+- [ ] Acceptance criteria are testable
+- [ ] New code can be unit tested in isolation
+- [ ] Integration test requirements are clear
+
+### PR Review Checklist
+
+Use this when reviewing pull requests for architecture concerns.
+
+**Structure:**
+- [ ] Changes respect existing package boundaries
+- [ ] New packages follow naming conventions
+- [ ] No new circular dependencies
+
+**Interfaces:**
+- [ ] Interfaces are defined where used
+- [ ] Interfaces are minimal and focused
+- [ ] Breaking interface changes are justified
+
+**Dependencies:**
+- [ ] Dependencies injected via constructors
+- [ ] No new global state
+- [ ] External dependencies properly abstracted
+
+**Error Handling:**
+- [ ] Errors wrapped with context
+- [ ] Sentinel errors for expected conditions
+- [ ] Error types for rich error information
+
+**Testing:**
+- [ ] New code has appropriate test coverage
+- [ ] Tests are clear and maintainable
+- [ ] Edge cases covered
+
+## Anti-Patterns to Flag
+
+### God Packages
+
+**Problem:** Packages like `utils/`, `common/`, `helpers/` become dumping grounds.
+
+**Symptoms:**
+- 20+ files in one package
+- Unrelated functions grouped together
+- Package imported by everything
+
+**Fix:** Extract cohesive packages based on what they provide: `validation`, `httputil`, `timeutil`.
+
+### Circular Dependencies
+
+**Problem:** Package A imports B, and B imports A (directly or transitively).
+
+**Symptoms:**
+- Import cycle compile errors
+- Difficulty understanding code flow
+- Changes cascade unexpectedly
+
+**Fix:**
+- Extract shared types to a third package
+- Use interfaces to invert dependency
+- Merge packages if truly coupled
+
+### Leaky Abstractions
+
+**Problem:** Implementation details leak through abstraction boundaries.
+
+**Symptoms:**
+- Database types in domain layer
+- HTTP types in service layer
+- Framework types in business logic
+
+**Fix:** Define types at each layer, map between them explicitly.
+
+### Anemic Domain Model
+
+**Problem:** Domain objects are just data containers, logic is elsewhere.
+
+**Symptoms:**
+- Domain types have only getters/setters
+- All logic in "service" classes
+- Domain types can be in invalid states
+
+**Fix:** Put behavior with data. Domain types should enforce their own invariants.
+
+### Shotgun Surgery
+
+**Problem:** Small changes require editing many files across packages.
+
+**Symptoms:**
+- Feature adds touch 10+ files
+- Similar changes in multiple places
+- Copy-paste between packages
+
+**Fix:** Consolidate related code. If things change together, they belong together.
+
+### Feature Envy
+
+**Problem:** Code in one package is more interested in another package's data.
+
+**Symptoms:**
+- Many calls to another package's methods
+- Pulling data just to compute something
+- Logic that belongs elsewhere
+
+**Fix:** Move the code to where the data lives, or extract the behavior to a shared place.
+
+### Premature Abstraction
+
+**Problem:** Creating interfaces and abstractions before they're needed.
+
+**Symptoms:**
+- Interfaces with single implementations
+- "Factory" and "Manager" classes everywhere
+- Configuration for things that never change
+
+**Fix:** Write concrete code first. Extract abstractions when you have multiple implementations or need to break dependencies.
+
+### Deep Hierarchy
+
+**Problem:** Excessive layers of abstraction or inheritance.
+
+**Symptoms:**
+- 5+ levels of embedding/composition
+- Hard to trace code flow
+- Changes require understanding many layers
+
+**Fix:** Prefer composition over inheritance. Flatten hierarchies where possible.