From d80dd0996a12c69064b1778e1df036451d55faaf Mon Sep 17 00:00:00 2001 From: Ben Duncan <5178243+benduncan@users.noreply.github.com> Date: Sat, 14 Feb 2026 06:54:00 +1000 Subject: [PATCH] Fixed gosec and added go skills --- .claude/skills/go-best-practices/SKILL.md | 75 +++++++++++++++++ .../rules/concurrency-channel-ownership.md | 54 ++++++++++++ .../rules/concurrency-goroutine-leaks.md | 77 +++++++++++++++++ .../rules/concurrency-mutex-patterns.md | 82 +++++++++++++++++++ .../rules/errors-always-check.md | 61 ++++++++++++++ .../rules/errors-sentinel.md | 57 +++++++++++++ .../rules/errors-wrap-context.md | 52 ++++++++++++ .../rules/http-handle-write-errors.md | 52 ++++++++++++ .../rules/http-input-validation.md | 52 ++++++++++++ .../rules/http-server-timeouts.md | 50 +++++++++++ .../rules/resources-close-bodies.md | 47 +++++++++++ .../rules/resources-context-propagation.md | 49 +++++++++++ .../rules/resources-defer-cleanup.md | 54 ++++++++++++ .../rules/style-error-types.md | 40 +++++++++ .../rules/style-slog-logging.md | 42 ++++++++++ .../rules/style-table-driven-tests.md | 67 +++++++++++++++ hive/formation/formation.go | 16 +++- 17 files changed, 923 insertions(+), 4 deletions(-) create mode 100644 .claude/skills/go-best-practices/SKILL.md create mode 100644 .claude/skills/go-best-practices/rules/concurrency-channel-ownership.md create mode 100644 .claude/skills/go-best-practices/rules/concurrency-goroutine-leaks.md create mode 100644 .claude/skills/go-best-practices/rules/concurrency-mutex-patterns.md create mode 100644 .claude/skills/go-best-practices/rules/errors-always-check.md create mode 100644 .claude/skills/go-best-practices/rules/errors-sentinel.md create mode 100644 .claude/skills/go-best-practices/rules/errors-wrap-context.md create mode 100644 .claude/skills/go-best-practices/rules/http-handle-write-errors.md create mode 100644 .claude/skills/go-best-practices/rules/http-input-validation.md create mode 100644 .claude/skills/go-best-practices/rules/http-server-timeouts.md create mode 100644 .claude/skills/go-best-practices/rules/resources-close-bodies.md create mode 100644 .claude/skills/go-best-practices/rules/resources-context-propagation.md create mode 100644 .claude/skills/go-best-practices/rules/resources-defer-cleanup.md create mode 100644 .claude/skills/go-best-practices/rules/style-error-types.md create mode 100644 .claude/skills/go-best-practices/rules/style-slog-logging.md create mode 100644 .claude/skills/go-best-practices/rules/style-table-driven-tests.md diff --git a/.claude/skills/go-best-practices/SKILL.md b/.claude/skills/go-best-practices/SKILL.md new file mode 100644 index 000000000..0fa177e3d --- /dev/null +++ b/.claude/skills/go-best-practices/SKILL.md @@ -0,0 +1,75 @@ +--- +name: go-best-practices +description: Go best practices and security guidelines. This skill should be used when writing, reviewing, or refactoring Go code to ensure correct error handling, secure HTTP servers, proper concurrency patterns, and idiomatic Go. Triggers on tasks involving Go backend code, HTTP handlers, goroutines, or system-level programming. +--- + +# Go Best Practices + +Comprehensive best practices for writing secure, correct, and idiomatic Go code. Contains rules prioritized by impact, covering security (gosec compliance), error handling, HTTP servers, concurrency, and common pitfalls. All rules are enforced by `make security` (go vet, gosec, govulncheck, staticcheck). + +## When to Apply + +Reference these guidelines when: +- Writing new Go code (handlers, services, packages) +- Creating or modifying HTTP servers +- Working with goroutines, channels, or mutexes +- Writing functions that return errors +- Reviewing Go code for correctness and security +- Implementing AWS-compatible service handlers + +## Rule Categories by Priority + +| Priority | Category | Impact | Prefix | +|----------|----------|--------|--------| +| 1 | HTTP Server Security | CRITICAL | `http-` | +| 2 | Error Handling | HIGH | `errors-` | +| 3 | Concurrency | HIGH | `concurrency-` | +| 4 | Resource Management | MEDIUM | `resources-` | +| 5 | Code Style & Idioms | LOW-MEDIUM | `style-` | + +## Quick Reference + +### 1. HTTP Server Security (CRITICAL) + +- `http-server-timeouts` - Always configure all timeouts on http.Server +- `http-handle-write-errors` - Always handle errors from ResponseWriter.Write and JSON encoding +- `http-input-validation` - Validate and limit request bodies, never trust client input + +### 2. Error Handling (HIGH) + +- `errors-always-check` - Never ignore returned errors (gosec G104) +- `errors-wrap-context` - Wrap errors with fmt.Errorf and %w for context +- `errors-sentinel` - Use sentinel errors and errors.Is/errors.As for comparison + +### 3. Concurrency (HIGH) + +- `concurrency-goroutine-leaks` - Ensure goroutines can always exit +- `concurrency-mutex-patterns` - Correct mutex usage and defer unlock +- `concurrency-channel-ownership` - Clear channel ownership and closing conventions + +### 4. Resource Management (MEDIUM) + +- `resources-close-bodies` - Always close http response bodies and file handles +- `resources-context-propagation` - Pass and respect context.Context for cancellation +- `resources-defer-cleanup` - Use defer for cleanup to ensure execution on all paths + +### 5. Code Style & Idioms (LOW-MEDIUM) + +- `style-slog-logging` - Use log/slog instead of log, with appropriate levels +- `style-error-types` - Use awserrors package for AWS error responses +- `style-table-driven-tests` - Use table-driven tests with subtests + +## How to Use + +Read individual rule files for detailed explanations and code examples: + +``` +rules/http-server-timeouts.md +rules/errors-always-check.md +``` + +Each rule file contains: +- Brief explanation of why it matters +- Incorrect code example with explanation +- Correct code example with explanation +- gosec/vet rule ID where applicable diff --git a/.claude/skills/go-best-practices/rules/concurrency-channel-ownership.md b/.claude/skills/go-best-practices/rules/concurrency-channel-ownership.md new file mode 100644 index 000000000..1381887ee --- /dev/null +++ b/.claude/skills/go-best-practices/rules/concurrency-channel-ownership.md @@ -0,0 +1,54 @@ +--- +title: Clear Channel Ownership and Closing +impact: HIGH +impactDescription: prevents panics from double-close and sends on closed channels +tags: go, concurrency, channels, ownership +--- + +## Clear Channel Ownership and Closing + +Follow clear ownership rules for channels: only the sender (producer) should close a channel, never the receiver. Closing a channel from the wrong goroutine causes panics. + +**Incorrect (receiver closes, or double close):** + +```go +func producer(ch chan<- int) { + for i := 0; i < 10; i++ { + ch <- i + } + // forgot to close — receiver blocks forever +} + +func consumer(ch <-chan int, done chan struct{}) { + for val := range ch { + process(val) + } + close(done) + close(done) // panic: close of closed channel +} +``` + +**Correct (sender closes, clear ownership):** + +```go +func producer(ch chan<- int) { + defer close(ch) // sender closes when done + for i := 0; i < 10; i++ { + ch <- i + } +} + +func consumer(ch <-chan int) { + for val := range ch { + process(val) + } + // channel was closed by producer, range exits naturally +} +``` + +**Key points:** +- The goroutine that creates/sends on a channel should close it +- Use `defer close(ch)` at the top of the sending function +- Use directional channel types (`chan<-`, `<-chan`) in function signatures to enforce ownership +- For signaling (done channels), use `chan struct{}` and close it once — don't send values +- Use `sync.Once` if multiple goroutines might try to close the same channel diff --git a/.claude/skills/go-best-practices/rules/concurrency-goroutine-leaks.md b/.claude/skills/go-best-practices/rules/concurrency-goroutine-leaks.md new file mode 100644 index 000000000..41123d937 --- /dev/null +++ b/.claude/skills/go-best-practices/rules/concurrency-goroutine-leaks.md @@ -0,0 +1,77 @@ +--- +title: Prevent Goroutine Leaks +impact: HIGH +impactDescription: prevents memory leaks and resource exhaustion from orphaned goroutines +tags: go, concurrency, goroutines, context, leaks +--- + +## Prevent Goroutine Leaks + +Every goroutine must have a clear exit path. Goroutines that block forever on channels or I/O without a cancellation mechanism leak memory and resources. + +**Incorrect (goroutine can never exit):** + +```go +func startWorker(jobs <-chan Job) { + go func() { + for job := range jobs { + process(job) + } + }() + // If no one closes jobs channel and no context, this goroutine lives forever +} + +func pollStatus(url string, interval time.Duration) { + go func() { + for { + resp, _ := http.Get(url) + // runs forever, no way to stop + time.Sleep(interval) + } + }() +} +``` + +**Correct (goroutine exits via context or done channel):** + +```go +func startWorker(ctx context.Context, jobs <-chan Job) { + go func() { + for { + select { + case <-ctx.Done(): + return + case job, ok := <-jobs: + if !ok { + return + } + process(job) + } + } + }() +} + +func pollStatus(ctx context.Context, url string, interval time.Duration) { + go func() { + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + // use context-aware HTTP request + req, _ := http.NewRequestWithContext(ctx, "GET", url, nil) + http.DefaultClient.Do(req) + } + } + }() +} +``` + +**Key points:** +- Always pass `context.Context` to functions that spawn goroutines +- Use `select` with `ctx.Done()` to enable cancellation +- Use `time.NewTicker` (and defer `Stop()`) instead of `time.Sleep` in loops +- Close channels from the sender side to signal completion +- In server shutdown, use `server.Shutdown(ctx)` to drain connections gracefully diff --git a/.claude/skills/go-best-practices/rules/concurrency-mutex-patterns.md b/.claude/skills/go-best-practices/rules/concurrency-mutex-patterns.md new file mode 100644 index 000000000..fe88dac0c --- /dev/null +++ b/.claude/skills/go-best-practices/rules/concurrency-mutex-patterns.md @@ -0,0 +1,82 @@ +--- +title: Correct Mutex Usage Patterns +impact: HIGH +impactDescription: prevents data races, deadlocks, and race condition bugs +tags: go, concurrency, mutex, sync, data-race +--- + +## Correct Mutex Usage Patterns + +Use mutexes correctly to protect shared state. Common mistakes include forgetting to unlock, holding locks across blocking operations, and using the wrong lock type. + +**Incorrect (common mutex mistakes):** + +```go +// Forgetting to unlock on error paths +func (s *Store) Get(key string) (string, error) { + s.mu.Lock() + val, ok := s.data[key] + if !ok { + return "", ErrNotFound // mutex never unlocked! + } + s.mu.Unlock() + return val, nil +} + +// Holding lock across blocking operation +func (s *Store) Refresh() { + s.mu.Lock() + defer s.mu.Unlock() + data, _ := http.Get(s.url) // blocks while holding lock! + s.data = parse(data) +} + +// Using RWMutex wrong — write lock where read lock suffices +func (s *Store) Count() int { + s.mu.Lock() // should be RLock for read-only access + defer s.mu.Unlock() + return len(s.data) +} +``` + +**Correct:** + +```go +// Always use defer Unlock +func (s *Store) Get(key string) (string, error) { + s.mu.RLock() + defer s.mu.RUnlock() + val, ok := s.data[key] + if !ok { + return "", ErrNotFound + } + return val, nil +} + +// Don't hold lock across blocking ops +func (s *Store) Refresh() { + data, err := http.Get(s.url) // fetch without lock + if err != nil { + return + } + parsed := parse(data) + + s.mu.Lock() + s.data = parsed // only hold lock for assignment + s.mu.Unlock() +} + +// Use RLock for read-only access +func (s *Store) Count() int { + s.mu.RLock() + defer s.mu.RUnlock() + return len(s.data) +} +``` + +**Key points:** +- Always use `defer mu.Unlock()` immediately after `mu.Lock()` to prevent missed unlocks +- Use `sync.RWMutex` when reads vastly outnumber writes — `RLock()` for reads, `Lock()` for writes +- Never hold a lock while doing I/O, network calls, or channel operations +- Keep critical sections as small as possible +- Document which fields a mutex protects with a comment above the mutex field diff --git a/.claude/skills/go-best-practices/rules/errors-always-check.md b/.claude/skills/go-best-practices/rules/errors-always-check.md new file mode 100644 index 000000000..a2632920e --- /dev/null +++ b/.claude/skills/go-best-practices/rules/errors-always-check.md @@ -0,0 +1,61 @@ +--- +title: Always Check Returned Errors +impact: HIGH +impactDescription: prevents silent failures and data corruption (gosec G104) +tags: go, errors, gosec, reliability +--- + +## Always Check Returned Errors + +Never ignore error return values. Every function that returns an error must have that error checked. This is the single most common source of bugs in Go code. + +**Incorrect (errors discarded):** + +```go +file, _ := os.Open(path) // crash if file doesn't exist +json.Unmarshal(data, &config) // silent corruption if data is invalid +db.Exec("DELETE FROM users WHERE id = ?", id) // silent failure +conn.Close() // resource leak on error +``` + +**Correct (errors handled):** + +```go +file, err := os.Open(path) +if err != nil { + return fmt.Errorf("open config: %w", err) +} + +if err := json.Unmarshal(data, &config); err != nil { + return fmt.Errorf("parse config: %w", err) +} + +if _, err := db.Exec("DELETE FROM users WHERE id = ?", id); err != nil { + return fmt.Errorf("delete user %s: %w", id, err) +} + +if err := conn.Close(); err != nil { + slog.Error("Failed to close connection", "error", err) +} +``` + +**Acceptable exceptions:** + +```go +// In deferred cleanup where you can't return the error +defer func() { + if err := resp.Body.Close(); err != nil { + slog.Debug("Failed to close response body", "error", err) + } +}() + +// fmt.Fprintf to a bytes.Buffer never fails — but document why +_, _ = fmt.Fprintf(&buf, "value: %d", n) // bytes.Buffer.Write never returns error +``` + +**Key points:** +- Use `_ =` explicitly when you intentionally discard an error, with a comment explaining why +- Log errors in cleanup paths (defer) where you can't propagate them +- Wrap errors with `%w` to preserve the error chain for callers + +**gosec rule:** G104 (CWE-703) — Errors unhandled diff --git a/.claude/skills/go-best-practices/rules/errors-sentinel.md b/.claude/skills/go-best-practices/rules/errors-sentinel.md new file mode 100644 index 000000000..5f7af63be --- /dev/null +++ b/.claude/skills/go-best-practices/rules/errors-sentinel.md @@ -0,0 +1,57 @@ +--- +title: Use Sentinel Errors and errors.Is/As +impact: HIGH +impactDescription: enables correct error matching across wrapped error chains +tags: go, errors, sentinel, comparison +--- + +## Use Sentinel Errors and errors.Is/As + +Never compare errors with `==`. Use `errors.Is()` for sentinel error comparison and `errors.As()` for type assertion. Direct comparison breaks when errors are wrapped. + +**Incorrect (direct comparison):** + +```go +if err == sql.ErrNoRows { + return nil, nil // breaks if err is wrapped +} + +if err.Error() == "not found" { + // fragile string comparison +} + +switch err.(type) { +case *os.PathError: + // breaks with wrapped errors +} +``` + +**Correct (errors.Is and errors.As):** + +```go +if errors.Is(err, sql.ErrNoRows) { + return nil, nil // works even if wrapped +} + +var pathErr *os.PathError +if errors.As(err, &pathErr) { + slog.Error("Path error", "path", pathErr.Path, "op", pathErr.Op) +} +``` + +**Defining sentinel errors:** + +```go +var ( + ErrNotFound = errors.New("not found") + ErrUnauthorized = errors.New("unauthorized") +) + +// Usage +return fmt.Errorf("get volume %s: %w", id, ErrNotFound) + +// Checking +if errors.Is(err, ErrNotFound) { + w.WriteHeader(http.StatusNotFound) +} +``` diff --git a/.claude/skills/go-best-practices/rules/errors-wrap-context.md b/.claude/skills/go-best-practices/rules/errors-wrap-context.md new file mode 100644 index 000000000..0424ffbd9 --- /dev/null +++ b/.claude/skills/go-best-practices/rules/errors-wrap-context.md @@ -0,0 +1,52 @@ +--- +title: Wrap Errors with Context +impact: HIGH +impactDescription: enables debugging by preserving error chains and adding call context +tags: go, errors, debugging, fmt +--- + +## Wrap Errors with Context + +When returning an error from a called function, wrap it with `fmt.Errorf` and `%w` to add context about what operation failed. Bare error returns make debugging difficult because you lose the call chain. + +**Incorrect (bare error returns):** + +```go +func LoadConfig(path string) (*Config, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, err // caller gets "open /etc/config.yaml: no such file or directory" + } + + var cfg Config + if err := json.Unmarshal(data, &cfg); err != nil { + return nil, err // caller gets "invalid character '}'" — no idea what file + } + + return &cfg, nil +} +``` + +**Correct (wrapped with context):** + +```go +func LoadConfig(path string) (*Config, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("read config %s: %w", path, err) + } + + var cfg Config + if err := json.Unmarshal(data, &cfg); err != nil { + return nil, fmt.Errorf("parse config %s: %w", path, err) + } + + return &cfg, nil +} +``` + +**Key points:** +- Use `%w` (not `%v`) to wrap errors — this preserves `errors.Is()` and `errors.As()` checks +- Add the operation name and key parameters (file path, ID, etc.) +- Keep wrap messages lowercase, no trailing punctuation (Go convention) +- Don't prefix with "failed to" or "error" — the error itself communicates failure diff --git a/.claude/skills/go-best-practices/rules/http-handle-write-errors.md b/.claude/skills/go-best-practices/rules/http-handle-write-errors.md new file mode 100644 index 000000000..fbc9cbe3c --- /dev/null +++ b/.claude/skills/go-best-practices/rules/http-handle-write-errors.md @@ -0,0 +1,52 @@ +--- +title: Handle HTTP Response Write Errors +impact: CRITICAL +impactDescription: prevents silent failures and satisfies gosec G104 +tags: go, http, error-handling, gosec, handlers +--- + +## Handle HTTP Response Write Errors + +Always check errors returned by `http.ResponseWriter.Write()`, `io.Writer.Write()`, `json.Encoder.Encode()`, and `fmt.Fprintf(w, ...)` in HTTP handlers. Ignoring these errors means you won't know when clients disconnect or responses fail to send. + +**Incorrect (errors ignored — gosec G104):** + +```go +func handleHealth(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte("ok")) // error ignored +} + +func writeJSON(w http.ResponseWriter, status int, v any) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + json.NewEncoder(w).Encode(v) // error ignored +} +``` + +**Correct (errors handled):** + +```go +func handleHealth(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + if _, err := w.Write([]byte("ok")); err != nil { + slog.Error("Failed to write health response", "error", err) + } +} + +func writeJSON(w http.ResponseWriter, status int, v any) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + if err := json.NewEncoder(w).Encode(v); err != nil { + slog.Error("Failed to encode JSON response", "error", err) + } +} +``` + +**Key points:** +- Log write errors at error level — they indicate client disconnects or network issues +- Don't try to write an HTTP error response after a write failure (the connection is likely broken) +- This applies to all `io.Writer` methods, not just `http.ResponseWriter` +- `fmt.Fprintf(w, ...)` also returns an error that must be checked + +**gosec rule:** G104 (CWE-703) — Errors unhandled diff --git a/.claude/skills/go-best-practices/rules/http-input-validation.md b/.claude/skills/go-best-practices/rules/http-input-validation.md new file mode 100644 index 000000000..8a106bab0 --- /dev/null +++ b/.claude/skills/go-best-practices/rules/http-input-validation.md @@ -0,0 +1,52 @@ +--- +title: Validate and Limit HTTP Request Input +impact: CRITICAL +impactDescription: prevents denial of service via oversized payloads and injection attacks +tags: go, http, security, validation, input +--- + +## Validate and Limit HTTP Request Input + +Always limit request body size and validate input in HTTP handlers. An unbounded `json.NewDecoder(r.Body).Decode()` allows attackers to send arbitrarily large payloads, exhausting server memory. + +**Incorrect (unbounded request body):** + +```go +func handleCreate(w http.ResponseWriter, r *http.Request) { + var req CreateRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + http.Error(w, "bad request", http.StatusBadRequest) + return + } + // process req... +} +``` + +**Correct (limited and validated):** + +```go +const maxRequestBodySize = 1 << 20 // 1 MB + +func handleCreate(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, maxRequestBodySize) + + var req CreateRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + http.Error(w, "bad request", http.StatusBadRequest) + return + } + + // Validate required fields + if req.Name == "" { + http.Error(w, "name is required", http.StatusBadRequest) + return + } + // process req... +} +``` + +**Key points:** +- Use `http.MaxBytesReader` to limit body size — it returns a 413 error if exceeded +- Validate all required fields after decoding +- Sanitize string inputs if they'll be used in commands, queries, or file paths +- Use early returns for validation failures diff --git a/.claude/skills/go-best-practices/rules/http-server-timeouts.md b/.claude/skills/go-best-practices/rules/http-server-timeouts.md new file mode 100644 index 000000000..327da2645 --- /dev/null +++ b/.claude/skills/go-best-practices/rules/http-server-timeouts.md @@ -0,0 +1,50 @@ +--- +title: Configure HTTP Server Timeouts +impact: CRITICAL +impactDescription: prevents Slowloris attacks and resource exhaustion (gosec G112) +tags: go, http, security, gosec, server, timeouts +--- + +## Configure HTTP Server Timeouts + +Every `http.Server` must have `ReadHeaderTimeout` configured at minimum. Omitting timeouts allows Slowloris-style attacks where an attacker opens connections and sends headers slowly, exhausting server resources. + +**Incorrect (no timeouts — gosec G112):** + +```go +server := &http.Server{ + Addr: ":8080", + Handler: mux, +} +``` + +This server has no timeout protection. A malicious client can hold connections open indefinitely. + +**Correct (all timeouts configured):** + +```go +server := &http.Server{ + Addr: ":8080", + Handler: mux, + ReadTimeout: 10 * time.Second, + WriteTimeout: 10 * time.Second, + IdleTimeout: 30 * time.Second, + ReadHeaderTimeout: 5 * time.Second, +} +``` + +**Timeout guidelines:** + +| Timeout | Purpose | Typical Value | +|---------|---------|---------------| +| `ReadHeaderTimeout` | Time to read request headers (Slowloris protection) | 5s | +| `ReadTimeout` | Time to read entire request including body | 10-30s | +| `WriteTimeout` | Time to write the response | 10-30s | +| `IdleTimeout` | Time to keep idle keep-alive connections open | 30-120s | + +**Adjust for your use case:** +- Short-lived internal APIs (formation, health): 10s read/write, 30s idle +- User-facing APIs with uploads: 30s+ read, 15s write +- Long-polling endpoints: increase WriteTimeout accordingly + +**gosec rule:** G112 (CWE-400) — Potential Slowloris Attack diff --git a/.claude/skills/go-best-practices/rules/resources-close-bodies.md b/.claude/skills/go-best-practices/rules/resources-close-bodies.md new file mode 100644 index 000000000..c737ed5d4 --- /dev/null +++ b/.claude/skills/go-best-practices/rules/resources-close-bodies.md @@ -0,0 +1,47 @@ +--- +title: Close Response Bodies and File Handles +impact: MEDIUM +impactDescription: prevents resource leaks that exhaust file descriptors and connections +tags: go, resources, http, files, defer, leak +--- + +## Close Response Bodies and File Handles + +Always close `http.Response.Body`, files, and other `io.Closer` resources. Failing to close HTTP response bodies leaks TCP connections; failing to close files leaks file descriptors. + +**Incorrect (response body not closed):** + +```go +func fetchData(url string) ([]byte, error) { + resp, err := http.Get(url) + if err != nil { + return nil, err + } + // resp.Body is never closed — leaks TCP connection + return io.ReadAll(resp.Body) +} +``` + +**Correct (defer close immediately after error check):** + +```go +func fetchData(url string) ([]byte, error) { + resp, err := http.Get(url) + if err != nil { + return nil, fmt.Errorf("fetch %s: %w", url, err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("fetch %s: status %d", url, resp.StatusCode) + } + + return io.ReadAll(resp.Body) +} +``` + +**Key points:** +- Place `defer Close()` immediately after the nil-error check, before any other logic +- For files: `defer f.Close()` right after `os.Open`/`os.Create` +- The body must be fully read and closed even for non-2xx responses — otherwise the connection can't be reused +- If you need the close error (e.g., for writes): use a named return and check in defer diff --git a/.claude/skills/go-best-practices/rules/resources-context-propagation.md b/.claude/skills/go-best-practices/rules/resources-context-propagation.md new file mode 100644 index 000000000..09663a365 --- /dev/null +++ b/.claude/skills/go-best-practices/rules/resources-context-propagation.md @@ -0,0 +1,49 @@ +--- +title: Propagate and Respect context.Context +impact: MEDIUM +impactDescription: enables cancellation, timeouts, and graceful shutdown across the call chain +tags: go, context, cancellation, timeout, graceful-shutdown +--- + +## Propagate and Respect context.Context + +Pass `context.Context` as the first parameter to functions that do I/O, run goroutines, or may need cancellation. Never store contexts in structs — they represent a single request lifecycle. + +**Incorrect (context ignored or stored):** + +```go +type Service struct { + ctx context.Context // don't store context in structs +} + +func (s *Service) FetchData() ([]byte, error) { + resp, err := http.Get(s.url) // no context — can't cancel + return io.ReadAll(resp.Body) +} +``` + +**Correct (context flows through the call chain):** + +```go +func (s *Service) FetchData(ctx context.Context) ([]byte, error) { + req, err := http.NewRequestWithContext(ctx, "GET", s.url, nil) + if err != nil { + return nil, fmt.Errorf("create request: %w", err) + } + + resp, err := s.client.Do(req) + if err != nil { + return nil, fmt.Errorf("fetch data: %w", err) + } + defer resp.Body.Close() + + return io.ReadAll(resp.Body) +} +``` + +**Key points:** +- `context.Context` is always the first parameter, named `ctx` +- Use `context.WithTimeout` or `context.WithCancel` to set deadlines +- Check `ctx.Err()` or `select` on `ctx.Done()` in long-running loops +- Use `http.NewRequestWithContext` instead of `http.Get`/`http.Post` +- In tests, use `context.Background()` or `t.Context()` (Go 1.24+) diff --git a/.claude/skills/go-best-practices/rules/resources-defer-cleanup.md b/.claude/skills/go-best-practices/rules/resources-defer-cleanup.md new file mode 100644 index 000000000..889edc9a4 --- /dev/null +++ b/.claude/skills/go-best-practices/rules/resources-defer-cleanup.md @@ -0,0 +1,54 @@ +--- +title: Use defer for Cleanup +impact: MEDIUM +impactDescription: ensures resources are released on all exit paths including panics +tags: go, defer, cleanup, resources +--- + +## Use defer for Cleanup + +Use `defer` to ensure cleanup code runs on all function exit paths, including early returns and panics. Place the defer immediately after acquiring the resource. + +**Incorrect (cleanup missed on error paths):** + +```go +func processFile(path string) error { + f, err := os.Open(path) + if err != nil { + return err + } + + data, err := io.ReadAll(f) + if err != nil { + return err // f is never closed! + } + + f.Close() + return process(data) +} +``` + +**Correct (defer immediately after acquire):** + +```go +func processFile(path string) error { + f, err := os.Open(path) + if err != nil { + return fmt.Errorf("open %s: %w", path, err) + } + defer f.Close() + + data, err := io.ReadAll(f) + if err != nil { + return fmt.Errorf("read %s: %w", path, err) + } + + return process(data) +} +``` + +**Key points:** +- Place `defer` on the line immediately after the error check for the acquire operation +- Defers execute in LIFO order — last deferred runs first +- For mutexes: `mu.Lock()` then `defer mu.Unlock()` on the next line +- For write operations where you need the close error, use a named return diff --git a/.claude/skills/go-best-practices/rules/style-error-types.md b/.claude/skills/go-best-practices/rules/style-error-types.md new file mode 100644 index 000000000..87e252e37 --- /dev/null +++ b/.claude/skills/go-best-practices/rules/style-error-types.md @@ -0,0 +1,40 @@ +--- +title: Use awserrors Package for AWS Error Responses +impact: LOW-MEDIUM +impactDescription: ensures consistent AWS-compatible error responses across all handlers +tags: go, aws, errors, handlers, compatibility +--- + +## Use awserrors Package for AWS Error Responses + +When returning AWS-compatible error responses, use the `awserrors` package instead of manually constructing error strings or XML. This ensures consistent error formatting across all service handlers. + +**Incorrect (manual error strings):** + +```go +func handleDescribeInstances(w http.ResponseWriter, r *http.Request) { + // ... + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintf(w, `InternalError%s`, err) + return + } +} +``` + +**Correct (awserrors package):** + +```go +func handleDescribeInstances(w http.ResponseWriter, r *http.Request) { + // ... + if err != nil { + awserrors.WriteError(w, awserrors.InternalError(err.Error())) + return + } +} +``` + +**Key points:** +- The `awserrors` package handles XML formatting, status codes, and AWS error code conventions +- Use predefined error constructors where available +- This ensures AWS SDK clients can parse error responses correctly diff --git a/.claude/skills/go-best-practices/rules/style-slog-logging.md b/.claude/skills/go-best-practices/rules/style-slog-logging.md new file mode 100644 index 000000000..90c2667b6 --- /dev/null +++ b/.claude/skills/go-best-practices/rules/style-slog-logging.md @@ -0,0 +1,42 @@ +--- +title: Use log/slog for Structured Logging +impact: LOW-MEDIUM +impactDescription: enables consistent, queryable structured logs across the codebase +tags: go, logging, slog, structured +--- + +## Use log/slog for Structured Logging + +Use `log/slog` instead of `log` or `fmt.Println` for all logging. Use appropriate log levels and structured key-value pairs. + +**Incorrect:** + +```go +import "log" + +log.Printf("failed to connect to %s: %v", addr, err) +fmt.Println("starting server on", addr) +log.Fatal("database connection failed") +``` + +**Correct:** + +```go +import "log/slog" + +slog.Error("Failed to connect", "addr", addr, "error", err) +slog.Info("Server starting", "addr", addr) +slog.Debug("Cache hit", "key", key, "ttl", ttl) +``` + +**Log levels:** +- `slog.Debug` — verbose diagnostic info, disabled in production +- `slog.Info` — normal operational events (startup, shutdown, config loaded) +- `slog.Warn` — unexpected but recoverable situations +- `slog.Error` — failures that need attention (lost connections, failed operations) + +**Key points:** +- Key-value pairs must alternate: key (string), value (any) +- Keep keys lowercase, snake_case: `"node_id"`, `"bind_ip"`, `"error"` +- Always include `"error", err` when logging an error +- Don't log and return the same error — pick one (usually return) diff --git a/.claude/skills/go-best-practices/rules/style-table-driven-tests.md b/.claude/skills/go-best-practices/rules/style-table-driven-tests.md new file mode 100644 index 000000000..f1cd85d07 --- /dev/null +++ b/.claude/skills/go-best-practices/rules/style-table-driven-tests.md @@ -0,0 +1,67 @@ +--- +title: Use Table-Driven Tests +impact: LOW-MEDIUM +impactDescription: improves test coverage, readability, and makes it easy to add new cases +tags: go, testing, table-driven, subtests +--- + +## Use Table-Driven Tests + +Use table-driven tests with `t.Run` subtests for functions with multiple input/output cases. This makes tests easy to extend and provides clear failure messages. + +**Incorrect (repetitive test functions):** + +```go +func TestValidatePort_Valid(t *testing.T) { + if err := ValidatePort(8080); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestValidatePort_Zero(t *testing.T) { + if err := ValidatePort(0); err == nil { + t.Fatal("expected error for port 0") + } +} + +func TestValidatePort_Negative(t *testing.T) { + if err := ValidatePort(-1); err == nil { + t.Fatal("expected error for port -1") + } +} +``` + +**Correct (table-driven):** + +```go +func TestValidatePort(t *testing.T) { + tests := []struct { + name string + port int + wantErr bool + }{ + {"valid port", 8080, false}, + {"min valid", 1, false}, + {"max valid", 65535, false}, + {"zero", 0, true}, + {"negative", -1, true}, + {"too high", 65536, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidatePort(tt.port) + if (err != nil) != tt.wantErr { + t.Errorf("ValidatePort(%d) error = %v, wantErr %v", tt.port, err, tt.wantErr) + } + }) + } +} +``` + +**Key points:** +- Name each test case descriptively — it appears in test output on failure +- Use `t.Run` for subtests so individual cases can be run with `-run` +- Keep test struct fields minimal — only what varies between cases +- For complex expected outputs, add a `want` field with the expected value +- Use `t.Parallel()` in subtests when cases are independent diff --git a/hive/formation/formation.go b/hive/formation/formation.go index 2f5cb04f3..0e2969ca7 100644 --- a/hive/formation/formation.go +++ b/hive/formation/formation.go @@ -160,8 +160,12 @@ func (fs *FormationServer) Start(bindAddr string) error { mux.HandleFunc("GET /formation/health", fs.handleHealth) fs.server = &http.Server{ - Addr: bindAddr, - Handler: mux, + Addr: bindAddr, + Handler: mux, + ReadTimeout: 10 * time.Second, + WriteTimeout: 10 * time.Second, + IdleTimeout: 30 * time.Second, + ReadHeaderTimeout: 5 * time.Second, } ln, err := net.Listen("tcp", bindAddr) @@ -243,11 +247,15 @@ func (fs *FormationServer) handleStatus(w http.ResponseWriter, r *http.Request) func (fs *FormationServer) handleHealth(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) - w.Write([]byte("ok")) + if _, err := w.Write([]byte("ok")); err != nil { + slog.Error("Failed to write health response", "error", err) + } } func writeJSON(w http.ResponseWriter, status int, v any) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(status) - json.NewEncoder(w).Encode(v) + if err := json.NewEncoder(w).Encode(v); err != nil { + slog.Error("Failed to encode JSON response", "error", err) + } }