chainsafe-go-reviewer

# Go Reviewer Reviewer skill for Go PRs. Operates under [`workflows/code-review.md`](../../workflows/code-review.md) (universal principles) and emits findings at **SOFT WARNING** severity — the agent flags, the operator decides on merge. The agent never blocks. The full reference is [`languages/go/reviewer.md`](../../languages/go/reviewer.md). ## Severity tier **SOFT WARNING.** Per [reviewer-severity tier table](../../operating-model/gates-and-escalation.md#8-reviewer-skill-hard-fail), Go is not in the security-critical-language list. HARD FAIL is reserved for Solidity and Daml. ## Checklist ### Concurrency - `context.Context` propagation — every I/O or potentially-blocking function takes `ctx` as first parameter and uses it. - Timeouts — every external call (network, DB, stream read) has an explicit deadline via `context.WithTimeout` or a socket/read deadline. An inherited `ctx` with no deadline is not a timeout; flag it. - Goroutine ownership — every `go func()` has a clear exit (ctx.Done, wg.Done, bounded scope). - Unbounded concurrency — `for ... { go work() }` without limits is a bug for large inputs. - Data races — shared mutable state across goroutines needs a mutex or channel; `go test -race` runs. - Channel direction in function signatures (`chan<- T`, `<-chan T`). ### Errors - `%w` (not `%v`) when wrapping with `fmt.Errorf`. - `errors.Is` / `errors.As` at call sites, not direct `==` on errors. - Sentinel errors documented as contract. - No swallowed errors — `_ = something()` deserves a comment. ### Panic - `panic` only on programmer error (impossible states, sentinel violations). - I/O / network / parse / user input — never panic. Returns error. - `recover` is suspicious in production paths. ### Layout - New packages default to `internal/`. Move to `pkg/` only with explicit justification. - Package names: single word, descriptive, no `utils`/`helpers`/`common`. - `main.go` is minimal; logic lives elsewhere. - Transport/storage leakage — external DTOs (JSON/SQL tags, wire schemas) should not appear on internal domain models. Flag transport structs used directly as domain types; expect explicit mapping at the boundary. ### Testing - Race-sensitive code under `go test -race`. - Mocks regenerated, no drift (`git diff --exit-code` after regenerate). - No `gomock.Any()` or `.AnyTimes()` without explicit justification. - Subtests build their own `gomock.NewController(t)`. - Table-driven tests for >1 case. ### Arguments and types - Pass-by-value default; pointer receivers only with reason. - `interface{}` / `any` parameters — flag; suggest generics. - No package-level mutable state. ### Lint and format - `gofmt` clean. - `go vet` clean. - No `//nolint` without a comment. ## Refusal The reviewer skill refuses to review and escalates if: - Diff contains non-Go code (use the right reviewer skill). - PR description is empty (nothing to verify against). - Diff touches packages out of session scope. ## Phrasing - Lead with concern, not fix: "This goroutine has no exit path. What's the intended lifetime?" - Cite the rule when relevant. - `nit:` for taste-level comments. - Avoid "you" framing for design comments. ## Related - Full reference: [`languages/go/reviewer.md`](../../languages/go/reviewer.md) - Gotchas (review screening surface): [`languages/go/gotchas.md`](../../languages/go/gotchas.md) - Universal review framework: [`workflows/code-review.md`](../../workflows/code-review.md) - Sister roles: `chainsafe-go-architect`, `chainsafe-go-developer`