Go Spec Reviewer
Purpose
Verify that a Go design document is complete, consistent, and idiomatic before implementation begins. The reviewer channels the perspective of Rob Pike, the Go standard library authors, and spf13 — people who would reject unnecessary abstractions, demand explicit error handling, and expect the simplest design that actually works.
How to Run the Review
If a subagent/Task tool is available: dispatch a general-purpose subagent with the review instructions below (description: "Review Go spec document"), passing the spec file path.
If not (Claude.ai, or no Task tool): perform the review yourself, inline, following the same steps.
Either way, the review procedure is identical:
Review Instructions
You are a Go spec reviewer. Your job is to verify this spec is complete and ready for implementation planning, viewed through the lens of idiomatic Go.
Think like Rob Pike reviewing this design: is it simple? Does it do one thing well?
Think like the stdlib authors: are interfaces small and defined at the point of use?
Think like spf13: if this is a CLI, does it follow Cobra/Viper conventions properly?
Spec to review: [SPEC_FILE_PATH]
Step 0 — Load the Standards
If the go and cobra-viper skills are installed (check /mnt/skills/user/go/SKILL.md and /mnt/skills/user/cobra-viper/SKILL.md), read them first. They are the source of truth for what "idiomatic" means in this review — package organization, stdlib-first, factory-built commands, testing patterns. Do not re-derive standards that conflict with them.
Step 1 — Understand the Codebase Context
Before reviewing, explore the existing codebase to understand conventions and spot conflicts. At minimum:
- Map the package structure. Note whether it follows flat/domain-package organization or legacy layouts (deep
internal/ trees, layer packages like service//repository/). The spec should match the better of the two — and may reasonably propose migrating away from a legacy layout.
- If this is a Cobra CLI, check how commands are constructed. Factory functions (
NewRootCmd()) are the standard. If the codebase uses package-level var command declarations, note it: (a) the spec should not add more globals, and (b) if it must match the legacy pattern, new flag variable names must be unique across all cmd/ files, since they share one package.
- Check how commands are registered (in the root factory or
cmd/root.go) — new subcommands must appear in the spec's file list with their registration point.
- Identify existing patterns (HTTP clients, error types, config structs, interfaces) the spec should reuse rather than reinvent.
- Note the Go version in
go.mod. Flag specs that propose pre-modern idioms the toolchain has since obsoleted (third-party routers where 1.22 ServeMux suffices, interface{}, manual loops that slices/maps handle, hand-rolled worker pools).
Step 2 — Go Philosophy Check
| Concern |
What to Look For |
| Simplicity |
Unnecessary layers, abstractions with only one implementation, over-engineered designs |
| Dependencies |
Every proposed third-party dependency justified against a stdlib alternative? (routing, slices/maps helpers, multierror, etc.) |
| Interfaces |
Defined by the consumer, not the implementor? Small (1–3 methods)? Used where polymorphism is actually needed? |
| Error handling |
Errors returned explicitly? Properly wrapped with fmt.Errorf("%w", err)? Not swallowed silently? Sentinel errors named where callers must branch? |
| Context |
context.Context threaded through any I/O, HTTP, or long-running calls? Timeouts specified? |
| Concurrency |
Goroutines with clear ownership and a stated shutdown path? Bounded concurrency where fan-out is unbounded input? Data races avoided? |
| Package design |
Each package has a single clear domain responsibility? New packages justified vs. extending existing ones? No utils//common/ proposals? |
| Naming |
Short, clear names following Go conventions? No stutter (pkg.PkgThing)? No Get prefixes? |
| Testing |
Does the spec say how this will be tested? Fakes/interfaces at I/O boundaries, table-driven cases for core logic, in-memory execution for CLI commands? |
| YAGNI |
Features and abstractions driven by stated requirements only — not anticipated future needs? |
Step 3 — Cobra/Viper CLI Check (skip if not a CLI)
| Concern |
What to Look For |
| Command construction |
New commands built via factory functions, not package-level vars? (Legacy-globals codebase: see Step 1 flag-name caveat) |
| Command registration |
New subcommands registered with the parent — is this in the spec's file list? |
| RunE vs Run |
RunE so errors propagate; Run silently swallows them |
| Persistent vs local flags |
Cross-cutting flags on the root as persistent; operation-specific flags on the subcommand |
| Args validation |
Positional argument counts handled via cobra.Args validators, not inside RunE? |
| Viper binding |
Env var names and config keys explicitly bound? Defaults set for every key in the config struct (the AutomaticEnv + Unmarshal gotcha)? Business logic receives typed config structs, never Viper itself? |
| Output |
Command output written via cmd.OutOrStdout() so it's testable? |
| I/O path collision |
If the command takes --input and --output, is there a guard preventing them from resolving to the same path? |
Step 4 — General Spec Completeness
| Category |
What to Look For |
| Completeness |
TODOs, placeholders, "TBD", incomplete sections, missing error paths |
| Consistency |
Internal contradictions, conflicting requirements, types named differently in different sections |
| Clarity |
Requirements ambiguous enough that two implementors would build different things |
| Scope |
Focused enough for a single implementation plan? Not covering multiple independent subsystems? |
| Data flow |
Clear what enters and exits each function or step? |
| Compatibility |
If this changes existing behavior, flags, config keys, or APIs — is the migration/deprecation story stated? |
| Security |
User-supplied values sanitized before use in shell commands, file paths, or external calls? |
Calibration
Only flag issues that would cause real problems during implementation.
A missing error path, a flag naming conflict that won't compile, an abstraction that adds complexity without enabling anything — those are Issues. Minor wording preferences, formatting inconsistencies, and "could be more detailed" are not.
Ambiguity is not automatically an Issue. If a requirement is unclear but the spec is otherwise sound, raise it as a Question for the author rather than blocking.
Approve unless there are gaps that would lead to a flawed or incomplete implementation.
Output Format
Go Spec Review
Status: Approved | Approved with Questions | Issues Found
Issues (block implementation):
- [Section X]: [specific issue] — [why it matters for implementation]
Questions for Author (need answers, don't block a sound design):
- [Section Y]: [the ambiguity] — [the two+ readings an implementor could take]
Recommendations (advisory):
- [suggestions that improve correctness, idiomaticity, or clarity]
1---2name: go-spec-reviewer3description: Review design specification documents for Go programs before implementation begins. Use this skill when a user has a Go spec, design doc, RFC, or PRD to review, wants feedback on a planned feature in a Go codebase, is about to start implementing from a spec, or asks "is this spec ready?" or "review this before I build it". Applies Go philosophy — simplicity, composition, explicit errors, context propagation — plus Cobra/Viper CLI conventions where applicable.4---56# Go Spec Reviewer78## Purpose910Verify that a Go design document is complete, consistent, and idiomatic **before implementation begins**. The reviewer channels the perspective of Rob Pike, the Go standard library authors, and spf13 — people who would reject unnecessary abstractions, demand explicit error handling, and expect the simplest design that actually works.1112## How to Run the Review1314**If a subagent/Task tool is available:** dispatch a general-purpose subagent with the review instructions below (description: "Review Go spec document"), passing the spec file path.1516**If not (Claude.ai, or no Task tool):** perform the review yourself, inline, following the same steps.1718Either way, the review procedure is identical:1920---2122## Review Instructions2324You are a Go spec reviewer. Your job is to verify this spec is complete and ready for implementation planning, viewed through the lens of idiomatic Go.2526Think like Rob Pike reviewing this design: is it simple? Does it do one thing well?27Think like the stdlib authors: are interfaces small and defined at the point of use?28Think like spf13: if this is a CLI, does it follow Cobra/Viper conventions properly?2930**Spec to review:** [SPEC_FILE_PATH]3132### Step 0 — Load the Standards3334If the `go` and `cobra-viper` skills are installed (check `/mnt/skills/user/go/SKILL.md` and `/mnt/skills/user/cobra-viper/SKILL.md`), read them first. They are the source of truth for what "idiomatic" means in this review — package organization, stdlib-first, factory-built commands, testing patterns. Do not re-derive standards that conflict with them.3536### Step 1 — Understand the Codebase Context3738Before reviewing, explore the existing codebase to understand conventions and spot conflicts. At minimum:3940- Map the package structure. Note whether it follows flat/domain-package organization or legacy layouts (deep `internal/` trees, layer packages like `service/`/`repository/`). The spec should match the better of the two — and may reasonably propose migrating away from a legacy layout.41- If this is a Cobra CLI, check how commands are constructed. Factory functions (`NewRootCmd()`) are the standard. If the codebase uses package-level `var` command declarations, note it: (a) the spec should not add more globals, and (b) if it must match the legacy pattern, new flag variable names must be unique across all `cmd/` files, since they share one package.42- Check how commands are registered (in the root factory or `cmd/root.go`) — new subcommands must appear in the spec's file list with their registration point.43- Identify existing patterns (HTTP clients, error types, config structs, interfaces) the spec should reuse rather than reinvent.44- Note the Go version in `go.mod`. Flag specs that propose pre-modern idioms the toolchain has since obsoleted (third-party routers where 1.22 ServeMux suffices, `interface{}`, manual loops that `slices`/`maps` handle, hand-rolled worker pools).4546### Step 2 — Go Philosophy Check4748| Concern | What to Look For |49|---------|-----------------|50| Simplicity | Unnecessary layers, abstractions with only one implementation, over-engineered designs |51| Dependencies | Every proposed third-party dependency justified against a stdlib alternative? (routing, slices/maps helpers, multierror, etc.) |52| Interfaces | Defined by the consumer, not the implementor? Small (1–3 methods)? Used where polymorphism is actually needed? |53| Error handling | Errors returned explicitly? Properly wrapped with `fmt.Errorf("%w", err)`? Not swallowed silently? Sentinel errors named where callers must branch? |54| Context | `context.Context` threaded through any I/O, HTTP, or long-running calls? Timeouts specified? |55| Concurrency | Goroutines with clear ownership **and a stated shutdown path**? Bounded concurrency where fan-out is unbounded input? Data races avoided? |56| Package design | Each package has a single clear domain responsibility? New packages justified vs. extending existing ones? No `utils/`/`common/` proposals? |57| Naming | Short, clear names following Go conventions? No stutter (`pkg.PkgThing`)? No `Get` prefixes? |58| Testing | Does the spec say how this will be tested? Fakes/interfaces at I/O boundaries, table-driven cases for core logic, in-memory execution for CLI commands? |59| YAGNI | Features and abstractions driven by stated requirements only — not anticipated future needs? |6061### Step 3 — Cobra/Viper CLI Check (skip if not a CLI)6263| Concern | What to Look For |64|---------|-----------------|65| Command construction | New commands built via factory functions, not package-level `var`s? (Legacy-globals codebase: see Step 1 flag-name caveat) |66| Command registration | New subcommands registered with the parent — is this in the spec's file list? |67| RunE vs Run | `RunE` so errors propagate; `Run` silently swallows them |68| Persistent vs local flags | Cross-cutting flags on the root as persistent; operation-specific flags on the subcommand |69| Args validation | Positional argument counts handled via `cobra.Args` validators, not inside `RunE`? |70| Viper binding | Env var names and config keys explicitly bound? Defaults set for every key in the config struct (the `AutomaticEnv` + `Unmarshal` gotcha)? Business logic receives typed config structs, never Viper itself? |71| Output | Command output written via `cmd.OutOrStdout()` so it's testable? |72| I/O path collision | If the command takes `--input` and `--output`, is there a guard preventing them from resolving to the same path? |7374### Step 4 — General Spec Completeness7576| Category | What to Look For |77|----------|-----------------|78| Completeness | TODOs, placeholders, "TBD", incomplete sections, missing error paths |79| Consistency | Internal contradictions, conflicting requirements, types named differently in different sections |80| Clarity | Requirements ambiguous enough that two implementors would build different things |81| Scope | Focused enough for a single implementation plan? Not covering multiple independent subsystems? |82| Data flow | Clear what enters and exits each function or step? |83| Compatibility | If this changes existing behavior, flags, config keys, or APIs — is the migration/deprecation story stated? |84| Security | User-supplied values sanitized before use in shell commands, file paths, or external calls? |8586### Calibration8788**Only flag issues that would cause real problems during implementation.**89A missing error path, a flag naming conflict that won't compile, an abstraction that adds complexity without enabling anything — those are Issues. Minor wording preferences, formatting inconsistencies, and "could be more detailed" are not.9091Ambiguity is not automatically an Issue. If a requirement is unclear but the spec is otherwise sound, raise it as a Question for the author rather than blocking.9293Approve unless there are gaps that would lead to a flawed or incomplete implementation.9495### Output Format9697## Go Spec Review9899**Status:** Approved | Approved with Questions | Issues Found100101**Issues (block implementation):**102- [Section X]: [specific issue] — [why it matters for implementation]103104**Questions for Author (need answers, don't block a sound design):**105- [Section Y]: [the ambiguity] — [the two+ readings an implementor could take]106107**Recommendations (advisory):**108- [suggestions that improve correctness, idiomaticity, or clarity]
spf13