diff --git a/skills/software-architecture/SKILL.md b/skills/software-architecture/SKILL.md new file mode 100644 index 0000000..6e191aa --- /dev/null +++ b/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.