diff --git a/CLAUDE.md b/CLAUDE.md index ac134b5..c31dcf2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,6 +48,9 @@ Each enforceable guide in `docs/` should have a matching agent in `agents/`. The | `go-mod-replace-guide.md` | `go-quality-assistant` | | `go-glog-guide.md` | `go-quality-assistant` | | `go-concurrency-patterns.md` | `go-architecture-assistant` | +| `go-http-service-guide.md` | `go-http-handler-assistant` | +| `go-cqrs.md` | `go-architecture-assistant` | +| `go-cli-guide.md` | `go-quality-assistant` | Reference-only docs (patterns, setup guides) don't need agents. diff --git a/docs/go-cli-guide.md b/docs/go-cli-guide.md index b21b6d2..959a425 100644 --- a/docs/go-cli-guide.md +++ b/docs/go-cli-guide.md @@ -2,7 +2,59 @@ ## Flag Parsing: cobra + slog -**NEVER use stdlib `flag` package** — transitive dependencies like `github.com/golang/glog` register flags via `init()`, polluting `--help` output with unwanted flags (`-alsologtostderr`, `-log_dir`, `-v`, etc.). +### RULE go-cli/cobra-not-stdlib-flag (MUST) + +**Owner**: go-quality-assistant +**Applies when**: a Go CLI binary's `main.go` / `pkg/cli/...` imports `flag` (stdlib) and calls `flag.String` / `flag.Bool` / `flag.Parse`, instead of using `github.com/spf13/cobra` (with its `pflag` library). +**Enforcement**: judgment (ast-grep follow-up: `import "flag"` in any `main` package + `call_expression` matching `flag.Parse` / `flag.String`. Test files exempt.) +**Why**: Stdlib `flag` uses a process-global `flag.CommandLine` FlagSet. Any transitive dependency that calls `flag.String(...)` in its `init()` adds flags to this global set — and `github.com/golang/glog` is the most common offender, adding 8+ flags (`-alsologtostderr`, `-log_dir`, `-log_backtrace_at`, `-stderrthreshold`, `-v`, `-vmodule`, …) to every binary that transitively imports it. The result: `my-tool --help` displays a wall of irrelevant glog flags before your three actual flags, and the binary accepts those flags at runtime even though no one wanted them. Cobra uses `pflag` which is isolated from `flag.CommandLine` — the global pollution can't reach it, `--help` shows only your flags, and your flag namespace stays under your control. + +#### Bad + +```go +// main.go — stdlib flag pollutes --help with transitive glog flags +package main + +import ( + "flag" + "fmt" +) + +func main() { + var config string + flag.StringVar(&config, "config", "", "Path to config") + flag.Parse() + fmt.Println(config) +} +// my-tool --help prints --config AND -alsologtostderr, -log_dir, -v, -vmodule, ... +``` + +#### Good + +```go +// main.go +package main + +import "github.com/bborbe/my-tool/pkg/cli" + +func main() { + cli.Execute() +} + +// pkg/cli/cli.go — cobra/pflag, isolated from flag.CommandLine +func Run(ctx context.Context, args []string) error { + var config string + rootCmd := &cobra.Command{ + Use: "my-tool", + SilenceUsage: true, + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + rootCmd.Flags().StringVar(&config, "config", "", "Path to config") + rootCmd.SetArgs(args) + return rootCmd.ExecuteContext(ctx) +} +// my-tool --help prints only --config (and cobra's --help itself) +``` **Always use `github.com/spf13/cobra`** for CLI flag parsing, even for single-command binaries. @@ -117,6 +169,43 @@ func Run(ctx context.Context, args []string) error { ## Logging +### RULE go-cli/slog-not-glog-in-new-projects (MUST) + +**Owner**: go-quality-assistant +**Applies when**: a *new* Go CLI binary (created after Go 1.21 release; no prior glog usage in the same module) imports `github.com/golang/glog`. Existing glog-using projects are exempt — they should not introduce slog and glog side by side. +**Enforcement**: judgment (semantic — distinguishing "new project" from "existing project mid-migration" requires checking git history / module age; ast-grep partial: `import "github.com/golang/glog"` in any main module without prior glog usage) +**Why**: glog has two structural problems slog doesn't: (1) it registers 8+ flags via stdlib `flag.init()` which pollutes every binary's `--help` output (see `go-cli/cobra-not-stdlib-flag`); (2) it predates structured logging — every log line is a free-form string, so log aggregators can't reliably parse `user_id=` / `request_id=` fields. `log/slog` (stdlib Go 1.21+) emits structured key-value logs, integrates with `context.Context` for request-scoped fields, and has no `flag` pollution. For *existing* glog projects, the migration cost is real and not always worth it — but new projects should not pay the glog tax. + +#### Bad + +```go +// New CLI binary in 2026 importing glog +import ( + "github.com/golang/glog" + "github.com/spf13/cobra" +) + +func runE(cmd *cobra.Command, args []string) error { + glog.Infof("starting with config=%s", configPath) // free-form, no structured fields + return nil +} +``` + +#### Good + +```go +// New CLI binary using log/slog +import ( + "log/slog" + "github.com/spf13/cobra" +) + +func runE(cmd *cobra.Command, args []string) error { + slog.Info("starting", "config", configPath, "verbose", verbose) + return nil +} +``` + Use `log/slog` (stdlib Go 1.21+). See [go-glog-guide.md](go-glog-guide.md) for legacy glog projects only. ## Why Not stdlib `flag`? diff --git a/docs/go-cqrs.md b/docs/go-cqrs.md index cdb9733..4fa8819 100644 --- a/docs/go-cqrs.md +++ b/docs/go-cqrs.md @@ -102,9 +102,86 @@ In normal error-handling paths, the only case where offsets do NOT commit is whe ## Rules +### RULE go-cqrs/auto-tx-wrapper-no-manual-wrap (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: a Go CQRS consumer in this framework manually wraps its command executor with `kv.NewTransactionMiddleware` / similar transaction-management code instead of using `RunCommandConsumerTx` (which auto-wraps). +**Enforcement**: judgment (ast-grep partial: `call_expression` matching `NewTransactionMiddleware` / `Wrap...` patterns adjacent to a CQRS command consumer registration) +**Why**: `RunCommandConsumerTx` is the framework's transaction-management entry point. It opens a kv transaction per command, hands the txn-bound store to the executor, commits on success, rolls back on error — exactly once per message, in the exact order the framework expects. Manual wrapping duplicates that logic and introduces subtle drift: the manual wrapper may rollback while the framework also rolls back (double-rollback panic on closed txn), or may commit while the framework expects rollback semantics on a downstream failure. The bug surfaces as "this CQRS consumer occasionally double-applies state changes" — hard to reproduce, harder to diagnose. + +#### Bad + +```go +// Manual transaction wrapping — duplicates RunCommandConsumerTx's contract +wrappedExecutor := kv.NewTransactionMiddleware(db, executor) +err := cdb.RunCommandConsumerTx(saramaClientProvider, syncProducer, db, + schemaID, wrappedExecutor) // double-wrapping smell — Tx variant already wraps +``` + +#### Good + +```go +// Tx auto-wrapped — framework owns the transaction lifecycle +err := cdb.RunCommandConsumerTx(saramaClientProvider, syncProducer, db, + schemaID, executor) +``` + +### RULE go-cqrs/skipped-not-nil-for-non-retryable (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: a Go CQRS executor handles a *non-retryable* condition (idempotent skip, command targets an already-terminal entity, duplicate detected, validation against immutable state failed) by returning `nil` or an arbitrary `err` instead of `ErrCommandObjectSkipped`. +**Enforcement**: judgment (semantic — distinguishing "non-retryable skip" from "transient error worth a Failure result" requires reading the executor's intent) +**Why**: Three different result-topic outcomes hinge on the executor's return: +- `nil` → `ResultObjectSuccess` published. The publisher thinks the command succeeded. +- `err` → `ResultObjectFailure` published, one per occurrence. Duplicate publisher emissions produce N Failure entries + N error log lines + N noisy alerts. +- `ErrCommandObjectSkipped` → no result sent, offset commits silently. + +For non-retryable conditions (terminal state, duplicate, immutable-validation failure), only `Skipped` is correct: publishing `Success` lies to the publisher; publishing `Failure` spams the result topic. The classic bug: an order processor returns `InvalidStateError` for `Completed` orders. Publisher retries 50× on a network blip → result topic gets 50 Failure entries and the on-call sees 50 alerts for an idempotent no-op. + +#### Bad (variant A — `return err`) + +```go +func (e *Executor) Execute(ctx context.Context, cmd Command) error { + order := e.store.Get(cmd.OrderID) + if order.Status == Completed { + // produces noisy Failure result; N duplicate commands → N Failure entries + + // N error log lines + N alerts on the on-call's pager + return errors.New("order already completed") + } + // ... real work +} +``` + +#### Bad (variant B — `return nil`) + +```go +func (e *Executor) Execute(ctx context.Context, cmd Command) error { + order := e.store.Get(cmd.OrderID) + if order.Status == Completed { + // publishes Success — lies to the publisher; downstream code thinks + // the command actually advanced the entity state + return nil + } + // ... real work +} +``` + +#### Good + +```go +func (e *Executor) Execute(ctx context.Context, cmd Command) error { + order := e.store.Get(cmd.OrderID) + if order.Status == Completed { + // silent idempotent skip, no result published, offset commits cleanly + return cdb.ErrCommandObjectSkipped + } + // ... real work +} +``` + +### Other rules (judgment-tier, not yet canonicalised as RULE blocks) + - Never consume event topic to wait for command results — use result topic -- `RunCommandConsumerTx` wraps executors automatically — don't wrap manually -- `ErrCommandObjectSkipped` skips silently (no result sent) — use for non-retryable situations - Normal `err` returns are NOT retried by the framework; they emit one Failure result and commit the offset — same offset behaviour as Skipped, different result-topic behaviour - `SendResultEnabled() == false` + no error → no result sent - Context timeout → `ResultFor()` returns `Success: false` diff --git a/docs/go-http-service-guide.md b/docs/go-http-service-guide.md index ec7a0dc..5fbb10d 100644 --- a/docs/go-http-service-guide.md +++ b/docs/go-http-service-guide.md @@ -49,6 +49,37 @@ func (a *application) createHTTPServer( } ``` +### RULE go-http-service/canonical-admin-endpoints (MUST) + +**Owner**: go-http-handler-assistant +**Applies when**: a Go service's admin HTTP server (port 9090, mounted under `/admin//...` by the gateway) is missing any of the five always-required endpoints: `/healthz`, `/readiness`, `/metrics`, `/setloglevel/{level}`, `/gc`. +**Enforcement**: judgment (route-table inspection in `main.go` / handler factory; ast-grep partial: `router.Handle(...)` / `mux.Handle(...)` patterns checked against the five-endpoint set) +**Why**: The five endpoints are the cross-service contract between every bborbe Go service and the supervisor (Kubernetes probes, Prometheus scrapes, on-call debugging, manual GC inspection). A service missing `/healthz` blocks Kubernetes liveness probes; missing `/readiness` breaks rollout coordination; missing `/setloglevel` forces a StatefulSet edit + pod restart for every debug session. The endpoints are cheap (a few lines each, factory-built) and the cost of omitting one shows up at the worst possible time — usually during an incident. + +#### Bad + +```go +// main.go — admin server missing /setloglevel and /gc +router := mux.NewRouter() +router.Path("/healthz").Handler(libhttp.NewPrintHandler("OK")) +router.Path("/readiness").Handler(libhttp.NewPrintHandler("OK")) +router.Path("/metrics").Handler(promhttp.Handler()) +// debug session means: edit StatefulSet -v=, restart pod, wait — every time +``` + +#### Good + +```go +// main.go — all five canonical endpoints registered +router := mux.NewRouter() +router.Path("/healthz").Handler(libhttp.NewPrintHandler("OK")) +router.Path("/readiness").Handler(libhttp.NewPrintHandler("OK")) +router.Path("/metrics").Handler(promhttp.Handler()) +router.Path("/setloglevel/{level}"). + Handler(log.NewSetLoglevelHandler(ctx, log.NewLogLevelSetter(2, 5*time.Minute))) +router.Path("/gc").Handler(libhttp.NewGarbageCollectorHandler()) +``` + ## Endpoint Catalog | Endpoint | Required | Purpose | Library | @@ -66,6 +97,29 @@ func (a *application) createHTTPServer( ## Port Conventions +### RULE go-http-service/admin-port-9090 (MUST) + +**Owner**: go-http-handler-assistant +**Applies when**: a Go service's admin HTTP server (the one serving `/healthz`, `/metrics`, etc.) defaults to a port other than `9090` — either hardcoded or via a `--listen` flag/env-var whose default isn't `:9090`. +**Enforcement**: judgment (config inspection in `main.go` / `application` struct; ast-grep partial: struct-tag `default:":9090"` on the Listen field) +**Why**: 9090 is the cross-service contract for the admin endpoint. Prometheus scrape configs, the gateway's `admin/port: '9090'` annotation, the operator's muscle memory for `kubectl port-forward 9090`, and shared tooling all assume it. A custom port per service breaks scrape configs (Prometheus probes the wrong port → no metrics), breaks the gateway's auto-routing (admin URL doesn't resolve), and forces every operator to look up the per-service port before they can curl an endpoint at 3am. The deviation cost is real; the standardisation cost is zero. + +#### Bad + +```go +type application struct { + Listen string `required:"false" arg:"listen" env:"LISTEN" default:":8080"` // custom port +} +``` + +#### Good + +```go +type application struct { + Listen string `required:"false" arg:"listen" env:"LISTEN" default:":9090"` +} +``` + - **Always `9090`** for the admin HTTP server. Mirrors the standard across all bborbe services and the Prometheus scrape annotations. - Listen address comes from a flag/env: `Listen string \`required:"false" arg:"listen" env:"LISTEN" default:":9090"\``. - Public API (frontend-accessible data) lives on a different prefix (`/api/1.0/...`) — typically same port via the same router, or a separate listener. diff --git a/rules/index.json b/rules/index.json index df4ad11..50d3398 100644 --- a/rules/index.json +++ b/rules/index.json @@ -116,6 +116,24 @@ "level": "SHOULD", "owner": "go-architecture-assistant" }, + { + "anchor": "go-cli/cobra-not-stdlib-flag", + "applies_when": "a Go CLI binary's `main.go` / `pkg/cli/...` imports `flag` (stdlib) and calls `flag.String` / `flag.Bool` / `flag.Parse`, instead of using `github.com/spf13/cobra` (with its `pflag` library).", + "doc_path": "docs/go-cli-guide.md", + "enforcement": "judgment (ast-grep follow-up: `import \"flag\"` in any `main` package + `call_expression` matching `flag.Parse` / `flag.String`. Test files exempt.)", + "id": "go-cli/cobra-not-stdlib-flag", + "level": "MUST", + "owner": "go-quality-assistant" + }, + { + "anchor": "go-cli/slog-not-glog-in-new-projects", + "applies_when": "a *new* Go CLI binary (created after Go 1.21 release; no prior glog usage in the same module) imports `github.com/golang/glog`. Existing glog-using projects are exempt — they should not introduce slog and glog side by side.", + "doc_path": "docs/go-cli-guide.md", + "enforcement": "judgment (semantic — distinguishing \"new project\" from \"existing project mid-migration\" requires checking git history / module age; ast-grep partial: `import \"github.com/golang/glog\"` in any main module without prior glog usage)", + "id": "go-cli/slog-not-glog-in-new-projects", + "level": "MUST", + "owner": "go-quality-assistant" + }, { "anchor": "go-concurrency/channel-closed-by-sender-only", "applies_when": "a Go file calls `close(ch)` on a channel that was passed in as a function parameter from elsewhere — i.e. closed by a consumer/receiver rather than by the goroutine that produces values into it.", @@ -143,6 +161,24 @@ "level": "SHOULD", "owner": "go-context-assistant" }, + { + "anchor": "go-cqrs/auto-tx-wrapper-no-manual-wrap", + "applies_when": "a Go CQRS consumer in this framework manually wraps its command executor with `kv.NewTransactionMiddleware` / similar transaction-management code instead of using `RunCommandConsumerTx` (which auto-wraps).", + "doc_path": "docs/go-cqrs.md", + "enforcement": "judgment (ast-grep partial: `call_expression` matching `NewTransactionMiddleware` / `Wrap...` patterns adjacent to a CQRS command consumer registration)", + "id": "go-cqrs/auto-tx-wrapper-no-manual-wrap", + "level": "MUST", + "owner": "go-architecture-assistant" + }, + { + "anchor": "go-cqrs/skipped-not-nil-for-non-retryable", + "applies_when": "a Go CQRS executor handles a *non-retryable* condition (idempotent skip, command targets an already-terminal entity, duplicate detected, validation against immutable state failed) by returning `nil` or an arbitrary `err` instead of `ErrCommandObjectSkipped`.", + "doc_path": "docs/go-cqrs.md", + "enforcement": "judgment (semantic — distinguishing \"non-retryable skip\" from \"transient error worth a Failure result\" requires reading the executor's intent)", + "id": "go-cqrs/skipped-not-nil-for-non-retryable", + "level": "MUST", + "owner": "go-architecture-assistant" + }, { "anchor": "go-doc/comment-starts-with-name", "applies_when": "an exported identifier has a doc comment whose first word does not match the identifier's name exactly (case-sensitive).", @@ -323,6 +359,24 @@ "level": "MUST", "owner": "go-http-handler-assistant" }, + { + "anchor": "go-http-service/admin-port-9090", + "applies_when": "a Go service's admin HTTP server (the one serving `/healthz`, `/metrics`, etc.) defaults to a port other than `9090` — either hardcoded or via a `--listen` flag/env-var whose default isn't `:9090`.", + "doc_path": "docs/go-http-service-guide.md", + "enforcement": "judgment (config inspection in `main.go` / `application` struct; ast-grep partial: struct-tag `default:\":9090\"` on the Listen field)", + "id": "go-http-service/admin-port-9090", + "level": "MUST", + "owner": "go-http-handler-assistant" + }, + { + "anchor": "go-http-service/canonical-admin-endpoints", + "applies_when": "a Go service's admin HTTP server (port 9090, mounted under `/admin//...` by the gateway) is missing any of the five always-required endpoints: `/healthz`, `/readiness`, `/metrics`, `/setloglevel/{level}`, `/gc`.", + "doc_path": "docs/go-http-service-guide.md", + "enforcement": "judgment (route-table inspection in `main.go` / handler factory; ast-grep partial: `router.Handle(...)` / `mux.Handle(...)` patterns checked against the five-endpoint set)", + "id": "go-http-service/canonical-admin-endpoints", + "level": "MUST", + "owner": "go-http-handler-assistant" + }, { "anchor": "go-json-error-handler/structured-response-shape", "applies_when": "an HTTP handler in a Go service emits an error response that is not a JSON object with the canonical `{error: {code, message, details}}` shape — e.g. plain-text bodies, top-level `{message: ...}` without an `error` wrapper, or `details` as a string instead of a `map[string]string`.",