From c6d19760ca17fd080dd9ce8a71f561838787e5ba Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 17:30:40 +0200 Subject: [PATCH 1/2] feat(rules): bootstrap 3 Go doc families (http-service, cqrs, cli) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth Go bootstrap batch. 3 docs (118-215 lines after additions), 6 rules. Single bot review cycle. Rules added (rules/index.json: 84 -> 90): go-http-service/* (owner: go-http-handler-assistant) - canonical-admin-endpoints (MUST) — every bborbe service's admin HTTP server has /healthz, /readiness, /metrics, /setloglevel/{level}, /gc. Missing any one breaks the cross-service contract with Kubernetes probes, Prometheus scrapes, on-call debugging. - admin-port-9090 (MUST) — admin port is always 9090. Custom ports break Prometheus scrape configs, gateway auto-routing, and operator muscle memory. go-cqrs/* (owner: go-architecture-assistant) - auto-tx-wrapper-no-manual-wrap (MUST) — use RunCommandConsumerTx; never manually wrap executors with NewTransactionMiddleware. Manual wrapping duplicates the framework's transaction lifecycle and produces double-rollback panics on closed txns. - skipped-not-nil-for-non-retryable (MUST) — non-retryable conditions return ErrCommandObjectSkipped, never nil (publishes false Success) or arbitrary err (publishes N Failure entries on N duplicate commands → noisy result topic + N error log lines + N alerts). go-cli/* (owner: go-quality-assistant) - cobra-not-stdlib-flag (MUST) — use cobra/pflag for CLI flag parsing. Stdlib flag uses global flag.CommandLine which transitive deps (most famously glog's init) pollute with 8+ unwanted flags (-alsologtostderr, -log_dir, -v, etc). - slog-not-glog-in-new-projects (MUST) — new CLI binaries use log/slog (stdlib Go 1.21+). Legacy glog projects exempt — they should not mix slog and glog. slog is structured by default and has no flag.init() pollution. CLAUDE.md doc-agent table updated with all 3 new mappings. All examples generic (config/verbose flags, Order/User entities, etc). No personal vault paths, no trading-domain terms. Pre-emptive grep clean. make build-index regenerated; check-index passes. --- CLAUDE.md | 3 ++ docs/go-cli-guide.md | 91 ++++++++++++++++++++++++++++++++++- docs/go-cqrs.md | 65 ++++++++++++++++++++++++- docs/go-http-service-guide.md | 53 ++++++++++++++++++++ rules/index.json | 54 +++++++++++++++++++++ 5 files changed, 263 insertions(+), 3 deletions(-) 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..0cedc9f 100644 --- a/docs/go-cqrs.md +++ b/docs/go-cqrs.md @@ -102,9 +102,70 @@ 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 +wrapped := kv.NewTransactionMiddleware(db, executor) +err := cqrs.RunCommandConsumer(ctx, consumer, wrapped) // double-wrapping smell +``` + +#### Good + +```go +// Tx auto-wrapped — framework owns the transaction lifecycle +err := cqrs.RunCommandConsumerTx(ctx, consumer, 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 + +```go +func (e *Executor) Execute(ctx context.Context, cmd Command) error { + order := e.store.Get(cmd.OrderID) + if order.Status == Completed { + return errors.New("order already completed") // wrong — produces noisy Failure result + } + if order.Status == Completed { + return nil // also wrong — publishes Success for something we didn't actually do + } + // ... real work +} +``` + +#### Good + +```go +func (e *Executor) Execute(ctx context.Context, cmd Command) error { + order := e.store.Get(cmd.OrderID) + if order.Status == Completed { + return cqrs.ErrCommandObjectSkipped // silent idempotent skip, no result published + } + // ... 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..5ac865b 100644 --- a/docs/go-http-service-guide.md +++ b/docs/go-http-service-guide.md @@ -49,6 +49,36 @@ 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.Handle("/healthz", libhttp.NewPrintHandler("ok")) +router.Handle("/readiness", libhttp.NewPrintHandler("ok")) +router.Handle("/metrics", 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.Handle("/healthz", libhttp.NewPrintHandler("ok")) +router.Handle("/readiness", libhttp.NewPrintHandler("ok")) +router.Handle("/metrics", promhttp.Handler()) +router.Handle("/setloglevel/{level}", log.NewSetLoglevelHandler(2, 5*time.Minute)) +router.Handle("/gc", libhttp.NewGarbageCollectorHandler()) +``` + ## Endpoint Catalog | Endpoint | Required | Purpose | Library | @@ -66,6 +96,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`.", From c0e18c9f7cfd3f6d51a83a09098c9518b969fcf2 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 17:41:55 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(rules):=20address=20PR=20#23=20review?= =?UTF-8?q?=20=E2=80=94=205=20MAJOR=20+=204=20NIT?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot's review caught real API-shape inconsistencies between my new rule examples and the canonical reference blocks in the same docs. All 5 MAJORs traceable to my examples; bot was right on each. MAJOR (fixed): 1. go-http-service-guide Bad example used 'router.Handle("/x", h)' but the canonical block earlier in the same doc uses 'router.Path("/x").Handler(h)'. Both APIs work in gorilla/mux but the doc's convention is Path().Handler(). Aligned both Bad and Good. 2. go-http-service-guide log.NewSetLoglevelHandler signature: my example passed (int, duration) but the canonical block uses (ctx, log.NewLogLevelSetter(2, 5*time.Minute)). Updated to match. 3-4. go-cqrs Bad example used 'cqrs.*' package alias but the rest of the doc uses 'cdb.*'. Also the Bad function call was RunCommandConsumer (no Tx) — the rule is about RunCommandConsumerTx so the example should call the Tx variant to demonstrate the double-wrap smell. Aligned to cdb.RunCommandConsumerTx in both the manual-wrap Bad and the Good example. 5. go-cqrs skipped-not-nil Bad example had unreachable code: two 'if order.Status == Completed' branches in sequence (second was dead code). Split into two distinct '#### Bad (variant A)' and '#### Bad (variant B)' blocks — one for 'return err' shape (noisy Failure result), one for 'return nil' shape (lies Success). Good remains a single block. NOT fixed (deliberate): 6. Bot flagged go-cli Execute() calling os.Exit(1) as untestable. Pre-existing canonical pattern in the doc, not introduced by this PR. The split is intentional: Execute() is the un-testable wrapper that exits on error, Run() returns the error and IS testable. The doc explicitly notes this design (line ~116 of original). Out of scope. NITs (skipped — not blocking): - go-skeleton bare-backtick reference (cosmetic markdown style) - Endpoint Catalog incomplete LogLevelSetter mention (covered by the separate '/setloglevel/{level} — Constructor Args' section below) - slog-not-glog 'ast-grep partial' vs git-history reasoning (the caveat is in the enforcement field already — agent makes final call) - /gc and /setloglevel MUST vs SHOULD split (defensible either way; the doc's Endpoint Catalog marks all 5 as 'always required', so MUST matches the doc's own classification) make build-index regenerated; check-index passes. --- docs/go-cqrs.md | 30 +++++++++++++++++++++++------- docs/go-http-service-guide.md | 17 +++++++++-------- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/docs/go-cqrs.md b/docs/go-cqrs.md index 0cedc9f..4fa8819 100644 --- a/docs/go-cqrs.md +++ b/docs/go-cqrs.md @@ -113,15 +113,17 @@ In normal error-handling paths, the only case where offsets do NOT commit is whe ```go // Manual transaction wrapping — duplicates RunCommandConsumerTx's contract -wrapped := kv.NewTransactionMiddleware(db, executor) -err := cqrs.RunCommandConsumer(ctx, consumer, wrapped) // double-wrapping smell +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 := cqrs.RunCommandConsumerTx(ctx, consumer, executor) +err := cdb.RunCommandConsumerTx(saramaClientProvider, syncProducer, db, + schemaID, executor) ``` ### RULE go-cqrs/skipped-not-nil-for-non-retryable (MUST) @@ -136,16 +138,29 @@ err := cqrs.RunCommandConsumerTx(ctx, consumer, executor) 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 +#### 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 { - return errors.New("order already completed") // wrong — produces noisy Failure result + // 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 { - return nil // also wrong — publishes Success for something we didn't actually do + // publishes Success — lies to the publisher; downstream code thinks + // the command actually advanced the entity state + return nil } // ... real work } @@ -157,7 +172,8 @@ func (e *Executor) Execute(ctx context.Context, cmd Command) error { func (e *Executor) Execute(ctx context.Context, cmd Command) error { order := e.store.Get(cmd.OrderID) if order.Status == Completed { - return cqrs.ErrCommandObjectSkipped // silent idempotent skip, no result published + // silent idempotent skip, no result published, offset commits cleanly + return cdb.ErrCommandObjectSkipped } // ... real work } diff --git a/docs/go-http-service-guide.md b/docs/go-http-service-guide.md index 5ac865b..5fbb10d 100644 --- a/docs/go-http-service-guide.md +++ b/docs/go-http-service-guide.md @@ -61,9 +61,9 @@ func (a *application) createHTTPServer( ```go // main.go — admin server missing /setloglevel and /gc router := mux.NewRouter() -router.Handle("/healthz", libhttp.NewPrintHandler("ok")) -router.Handle("/readiness", libhttp.NewPrintHandler("ok")) -router.Handle("/metrics", promhttp.Handler()) +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 ``` @@ -72,11 +72,12 @@ router.Handle("/metrics", promhttp.Handler()) ```go // main.go — all five canonical endpoints registered router := mux.NewRouter() -router.Handle("/healthz", libhttp.NewPrintHandler("ok")) -router.Handle("/readiness", libhttp.NewPrintHandler("ok")) -router.Handle("/metrics", promhttp.Handler()) -router.Handle("/setloglevel/{level}", log.NewSetLoglevelHandler(2, 5*time.Minute)) -router.Handle("/gc", libhttp.NewGarbageCollectorHandler()) +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