Add software-architecture skill
Creates the foundational skill that encodes software architecture best practices, review checklists, and patterns for Go and generic architecture guidance. Closes #56 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit was merged in pull request #61.
This commit is contained in:
495
skills/software-architecture/SKILL.md
Normal file
495
skills/software-architecture/SKILL.md
Normal file
@@ -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.
|
||||
Reference in New Issue
Block a user