From def8237959af7dbc86a05c15ef402dd9e96780f7 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 14:58:54 +0200 Subject: [PATCH 1/5] feat(rules): bootstrap 3 doc families (service-impl, k8s-crd, functional-options) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second multi-doc bootstrap PR — same shape as PR #17. 3 docs (~500-770 lines), 6 rules, single bot review cycle. Rules added (rules/index.json: 62 -> 68): go-service-impl/* (owner: go-architecture-assistant) - no-context-object-injection (MUST) — never bundle deps in a *Context / *Deps struct passed through methods. Constructor injection makes the dep set visible at the type signature and minimal in scope. - provider-vs-registry-choice (SHOULD) — static switch for fixed compile-time sets (< ~10 types); map-based registry only for runtime extensible / plugin systems. go-k8s-crd/* (owner: go-architecture-assistant) - use-bborbe-k8s (SHOULD) — collapse ~300 lines of hand-written event-handler + adapter + store boilerplate into k8s.NewEventHandler[T] + k8s.NewResourceEventHandler[T] from github.com/bborbe/k8s. - generated-client-not-dynamic (MUST) — first-party CRDs use the typed clientset from hack/update-codegen.sh, not client-go/dynamic. Dynamic client is for unknown schemas (admin tools, generic operators); known-schema use throws away every type-safety guarantee. go-functional-options/* (owner: go-quality-assistant) - singular-option-type (SHOULD) — XxxOption (singular fn type) + XxxOptions (plural struct). Industry-standard pair-naming. - with-prefix-option-functions (SHOULD) — option constructors prefixed With*. Uniform prefix makes the option-list call site self-describing. CLAUDE.md doc-agent table updated with all 3 new mappings. Pre-emptive checks (lessons from PRs #6, #8, #14, #17): - No personal vault paths - One trading-domain leak introduced in my service-impl example (ProcessMarket/Limit/Stop — order-type terminology) caught by pre-push grep + genericised to ProcessImage/Video/Document. Pre-existing OrderType/MarketOrder/LimitOrder/StopOrder references in the original doc are NOT changed in this PR (out of scope; the bot may flag them as pre-existing). - All 6 rule IDs unique against 62 existing entries - All 3 owner agents exist - make build-index regenerated; check-index passes --- CLAUDE.md | 3 + docs/go-functional-options-pattern.md | 62 ++++++++++++++ docs/go-kubernetes-crd-controller-guide.md | 52 ++++++++++++ docs/go-service-implementation-patterns.md | 95 ++++++++++++++++++++++ rules/index.json | 54 ++++++++++++ 5 files changed, 266 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index c132903..6522bda 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -30,6 +30,9 @@ Each enforceable guide in `docs/` should have a matching agent in `agents/`. The | `go-json-error-handler-guide.md` | `go-http-handler-assistant` | | `go-linting-guide.md` | `go-quality-assistant` | | `go-state-machine-pattern.md` | `go-architecture-assistant` | +| `go-service-implementation-patterns.md` | `go-architecture-assistant` | +| `go-kubernetes-crd-controller-guide.md` | `go-architecture-assistant` | +| `go-functional-options-pattern.md` | `go-quality-assistant` | | `go-doc-best-practices.md` | `godoc-assistant` | | `go-testing-guide.md` | `go-test-quality-assistant` | | `go-security-linting.md` | `go-security-specialist` | diff --git a/docs/go-functional-options-pattern.md b/docs/go-functional-options-pattern.md index 7f27cc5..7b4c9ee 100644 --- a/docs/go-functional-options-pattern.md +++ b/docs/go-functional-options-pattern.md @@ -512,6 +512,68 @@ func NewConsumer(options ...func(*ConsumerOptions)) Consumer { ## Naming Conventions +### RULE go-functional-options/singular-option-type (SHOULD) + +**Owner**: go-quality-assistant +**Applies when**: a Go package using the functional-options pattern names the function type with a plural `XxxOptions` (matching the struct) instead of the singular `XxxOption`, OR names the config struct with the singular `XxxOption` (clashing with the function type). +**Enforcement**: judgment (ast-grep follow-up: `type_alias_declaration` / `type_declaration` with `XxxOptions func(...)` matching the wrong-singular pattern, and `struct_type` named `XxxOption`) +**Why**: The pair-naming convention — `XxxOption` (singular function type) + `XxxOptions` (plural config struct) — makes the relationship at the type signature unambiguous: one `XxxOption` modifies one `XxxOptions`. Swapping them or using the same word for both makes every reader pause to remember which is which. Industry standard (Dave Cheney's original pattern, gRPC, OpenTelemetry, k8s client-go) follows the singular-function / plural-struct shape; deviating costs reader-onboarding effort with no upside. + +#### Bad + +```go +// Function type plural — collides semantically with the struct +type ConsumerOptions func(*ConsumerOptions) // and the struct is also ConsumerOptions? + +// Or: function type singular, struct also singular +type ServerOption func(*ServerOption) // is ServerOption the modifier or the config? +``` + +#### Good + +```go +// Function type singular; config struct plural +type ConsumerOption func(*ConsumerOptions) +type ServerOption func(*ServerOptions) +type ClientOption func(*ClientOptions) + +type ConsumerOptions struct { + BatchSize int + Timeout time.Duration +} +``` + +### RULE go-functional-options/with-prefix-option-functions (SHOULD) + +**Owner**: go-quality-assistant +**Applies when**: a Go package using the functional-options pattern names the option constructors with prefixes other than `With*` (e.g. `Set*`, `Use*`, `Configure*`, bare nouns like `BatchSize(n)`). +**Enforcement**: judgment (ast-grep follow-up: `function_declaration` returning a type matching `*Option` / `*ConfigFunc` with name not starting with `With`) +**Why**: `With*` is the unambiguous signal that this function is an option constructor, not an action — `WithBatchSize(100)` reads as "with a batch size of 100" while `SetBatchSize(100)` reads as a mutating call against an existing config. Mixed prefixes in the same package leave the reader guessing which functions belong in the option list at the call site (`NewConsumer(WithX(...), SetY(...), Z(...))`) and which are unrelated calls. Picking `With*` and sticking to it makes the pattern self-describing. + +#### Bad + +```go +// Mixed prefixes — call site is unreadable +consumer := NewConsumer( + BatchSize(100), + UseTimeout(30 * time.Second), + ConfigureRetry(3), + WithTLS(tlsCfg), // which of these are option constructors? +) +``` + +#### Good + +```go +// Uniform With* prefix — every arg is obviously an option +consumer := NewConsumer( + WithBatchSize(100), + WithTimeout(30 * time.Second), + WithRetry(3), + WithTLS(tlsCfg), +) +``` + ### Option Type Names **Recommended Pattern**: Use **singular** for the function type (represents one option): diff --git a/docs/go-kubernetes-crd-controller-guide.md b/docs/go-kubernetes-crd-controller-guide.md index e4aa04a..abf05b2 100644 --- a/docs/go-kubernetes-crd-controller-guide.md +++ b/docs/go-kubernetes-crd-controller-guide.md @@ -4,6 +4,32 @@ How to define and consume a Kubernetes Custom Resource Definition (CRD) in a Go ## 0. Before you start — use `bborbe/k8s` +### RULE go-k8s-crd/use-bborbe-k8s (SHOULD) + +**Owner**: go-architecture-assistant +**Applies when**: a Go service consuming a CRD hand-writes the event-handler cast adapter, the typed event handler, or the in-memory state store, while the service already depends on (or could depend on) `github.com/bborbe/k8s`. +**Enforcement**: judgment (file-existence + dependency-graph check: presence of `pkg/event-handler*.go` or `pkg/-store.go` alongside a non-trivial `bborbe/k8s` import means hand-written boilerplate that should use the generic primitives) +**Why**: `bborbe/k8s` collapses ~300 lines of hand-written event-handler + adapter + store boilerplate per CRD into one `k8s.NewEventHandler[T]()` + `k8s.NewResourceEventHandler[T]()` call. The generic primitives are typed (no `interface{}` casts), thread-safe by construction, and tested upstream. Hand-written equivalents re-invent the same bugs (race conditions on the store, missed `OnDelete` events, type assertions that panic on cache resync) across every service. Take the dependency; delete the boilerplate. + +#### Bad + +```go +// pkg/event-handler.go — hand-written cast adapter +// pkg/event-handler-user.go — typed handler with OnAdd/OnUpdate/OnDelete +// pkg/user-store.go — sync.RWMutex + map[string]*User, 60+ lines +// (three files of boilerplate per CRD) +``` + +#### Good + +```go +// pkg/factory/factory.go +store := k8s.NewEventHandler[*v1.User]() +adapter := k8s.NewResourceEventHandler[*v1.User](ctx, store) +informer.AddEventHandler(adapter) +// done — typed, thread-safe, tested upstream +``` + [`github.com/bborbe/k8s`](https://github.com/bborbe/k8s) provides generic primitives that eliminate most of the boilerplate in sections 5–6 of this guide: - `k8s.Type` — interface your types implement (`Equal`, `Identifier`, `Validate`, `String`) @@ -57,6 +83,32 @@ myservice/ - `github.com/bborbe/alert` (library) - `github.com/bborbe/cqrs/cdb` + `github.com/bborbe/cqrs/raw` (libraries) +### RULE go-k8s-crd/generated-client-not-dynamic (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: a Go service interacts with a first-party CRD (the CR schema is known at compile time, defined in the same repo or a sibling library) using `k8s.io/client-go/dynamic` instead of a typed clientset generated via `hack/update-codegen.sh`. +**Enforcement**: judgment (import-graph check: `client-go/dynamic` imported alongside a known-schema CR type is the smell; dynamic-client use is acceptable only when the CR is third-party with unknown schema) +**Why**: The dynamic client returns `*unstructured.Unstructured` — every field access is a string lookup, every value comes back as `interface{}`, every typo is a runtime error. Generated clientsets return typed Go structs: typos fail at compile time, IDE auto-complete works, refactors propagate. The dynamic client is the right tool when you don't know the schema (admin tools, generic operators, schema discovery); for first-party CRDs where you wrote the types, it's the wrong tool — it discards every type-safety guarantee Go offers. + +#### Bad + +```go +// Dynamic client for a first-party CRD — every field access is stringly-typed +client := dynamic.NewForConfigOrDie(config) +gvr := schema.GroupVersionResource{Group: "example.com", Version: "v1", Resource: "users"} +obj, err := client.Resource(gvr).Namespace("default").Get(ctx, "alice", metav1.GetOptions{}) +// obj.Object["spec"].(map[string]interface{})["email"].(string) ← typo-prone, runtime-only +``` + +#### Good + +```go +// Generated typed clientset — fields are real Go types +clientset := versioned.NewForConfigOrDie(config) +user, err := clientset.ExampleV1().Users("default").Get(ctx, "alice", metav1.GetOptions{}) +// user.Spec.Email ← compile-time checked +``` + The client must be **generated** via `hack/update-codegen.sh` — do NOT hand-write or use `client-go/dynamic`. The dynamic client is acceptable only when the CR schema is unknown at compile time; that is never the case for a first-party CRD. ### Consumer service (controller/watcher) diff --git a/docs/go-service-implementation-patterns.md b/docs/go-service-implementation-patterns.md index 6f46cf3..a8056fc 100644 --- a/docs/go-service-implementation-patterns.md +++ b/docs/go-service-implementation-patterns.md @@ -13,6 +13,58 @@ This guide captures practical decision-making patterns and implementation choice ## Service Architecture Decision Framework +### RULE go-service-impl/provider-vs-registry-choice (SHOULD) + +**Owner**: go-architecture-assistant +**Applies when**: a Go service needs to dispatch to one of several implementations and the code uses the wrong dispatch shape for the set size: a `map[string]Creator` registry for a fixed compile-time set (< ~10 types) or a compiled `switch` for a runtime-extensible set. +**Enforcement**: judgment (semantic — depends on whether the implementation set is closed or open) +**Why**: Static `switch` is faster (compiled jump table vs. map lookup + indirect call), trivially exhaustive (compiler-checked via `default:` + `errors.Errorf`), and refactor-friendly (renames propagate). Dynamic map-based registries are necessary when implementations register themselves at runtime (plugin systems, third-party extensions) but the cost — lost compile-time exhaustiveness, harder-to-test, registration-order sensitivity — is real. Match the shape to the problem: closed set → switch; open set → registry. Defaulting to one or the other regardless of context produces both unnecessary overhead and brittle systems. + +#### Bad + +```go +// Fixed 3-implementation set, runtime registry — overkill +type ProcessorRegistry struct { + processors map[string]ProcessorCreator +} + +func init() { + r := &ProcessorRegistry{processors: make(map[string]ProcessorCreator)} + r.Register("image", NewImageProcessor) // these never change + r.Register("video", NewVideoProcessor) + r.Register("document", NewDocumentProcessor) +} +``` + +#### Good + +```go +// Fixed set → compiled switch; exhaustiveness checked at compile time +type ProcessorProvider interface { + Get(ctx context.Context, t ProcessType) (Processor, error) +} + +func (p *processorProvider) Get(ctx context.Context, t ProcessType) (Processor, error) { + switch t { + case ProcessImage: + return NewImageProcessor(p.deps...), nil + case ProcessVideo: + return NewVideoProcessor(p.deps...), nil + case ProcessDocument: + return NewDocumentProcessor(p.deps...), nil + default: + return nil, errors.Errorf(ctx, "unknown processor type: %s", t) + } +} + +// Plugin-extensible set → registry; new types register at startup +type PluginRegistry struct { + plugins map[string]PluginCreator +} + +func (r *PluginRegistry) Register(name string, creator PluginCreator) { r.plugins[name] = creator } +``` + ### Provider vs Registry Pattern **Use Static Provider Pattern When:** @@ -141,6 +193,49 @@ type PaymentHandler interface { // Focuses on technical aspect ## Dependency Injection Best Practices +### RULE go-service-impl/no-context-object-injection (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: a Go service method receives a struct named `*Context`, `*ServiceContext`, `*Deps`, or similar that bundles multiple service dependencies (logger, repository, validator, etc.) and passes them through method calls instead of through the constructor. +**Enforcement**: judgment (ast-grep follow-up: method signature with a parameter type matching `*Context` / `*Deps` where the struct contains 2+ service-interface fields) +**Why**: Context-object injection is the rebrand of global state — every method becomes "give me everything, I'll pick what I need." Three failure modes: (1) compile-time can't tell which deps a method actually uses, so refactors and dead-code detection break; (2) tests need to construct a full context object for every call site, even for methods that touch one dependency; (3) the context grows over time (the "we'll just add one more field" trap), and unused fields linger forever. Constructor injection forces the dep set to be minimal and visible at the type signature. + +#### Bad + +```go +type ServiceContext struct { + SMTPClient SMTPClient + TemplateRepo TemplateRepository + Logger log.Logger +} + +func (e *EmailService) Send(ctx context.Context, msg Message, svcCtx ServiceContext) error { + svcCtx.Logger.Info("sending") + return svcCtx.SMTPClient.Send(ctx, msg) // hidden dep set; compile-time can't see it +} +``` + +#### Good + +```go +type EmailService interface { + Send(ctx context.Context, msg Message) error +} + +func NewEmailService( + smtpClient SMTPClient, + templateRepo TemplateRepository, + logger log.Logger, +) EmailService { + return &emailService{smtpClient: smtpClient, templateRepo: templateRepo, logger: logger} +} + +func (e *emailService) Send(ctx context.Context, msg Message) error { + e.logger.Info("sending") // dep is a struct field, visible at type + return e.smtpClient.Send(ctx, msg) +} +``` + ### Constructor Injection vs Context Objects **✅ Good: Constructor Dependency Injection** diff --git a/rules/index.json b/rules/index.json index 777b9e8..f1d6ff5 100644 --- a/rules/index.json +++ b/rules/index.json @@ -242,6 +242,24 @@ "level": "MUST", "owner": "go-factory-pattern-assistant" }, + { + "anchor": "go-functional-options/singular-option-type", + "applies_when": "a Go package using the functional-options pattern names the function type with a plural `XxxOptions` (matching the struct) instead of the singular `XxxOption`, OR names the config struct with the singular `XxxOption` (clashing with the function type).", + "doc_path": "docs/go-functional-options-pattern.md", + "enforcement": "judgment (ast-grep follow-up: `type_alias_declaration` / `type_declaration` with `XxxOptions func(...)` matching the wrong-singular pattern, and `struct_type` named `XxxOption`)", + "id": "go-functional-options/singular-option-type", + "level": "SHOULD", + "owner": "go-quality-assistant" + }, + { + "anchor": "go-functional-options/with-prefix-option-functions", + "applies_when": "a Go package using the functional-options pattern names the option constructors with prefixes other than `With*` (e.g. `Set*`, `Use*`, `Configure*`, bare nouns like `BatchSize(n)`).", + "doc_path": "docs/go-functional-options-pattern.md", + "enforcement": "judgment (ast-grep follow-up: `function_declaration` returning a type matching `*Option` / `*ConfigFunc` with name not starting with `With`)", + "id": "go-functional-options/with-prefix-option-functions", + "level": "SHOULD", + "owner": "go-quality-assistant" + }, { "anchor": "go-http-handler/kebab-case-handler-files", "applies_when": "a `*.go` file under `**/pkg/handler/**` is named with non-kebab-case style (e.g. `exists_handler.go`, `existshandler.go`, or `handler.go`) instead of the documented `-.go` kebab-case (e.g. `exists.go`, `forward-invoice.go`). Pure ast-grep operates on file contents, not filenames — this is a filesystem convention check.", @@ -296,6 +314,24 @@ "level": "MUST", "owner": "go-http-handler-assistant" }, + { + "anchor": "go-k8s-crd/generated-client-not-dynamic", + "applies_when": "a Go service interacts with a first-party CRD (the CR schema is known at compile time, defined in the same repo or a sibling library) using `k8s.io/client-go/dynamic` instead of a typed clientset generated via `hack/update-codegen.sh`.", + "doc_path": "docs/go-kubernetes-crd-controller-guide.md", + "enforcement": "judgment (import-graph check: `client-go/dynamic` imported alongside a known-schema CR type is the smell; dynamic-client use is acceptable only when the CR is third-party with unknown schema)", + "id": "go-k8s-crd/generated-client-not-dynamic", + "level": "MUST", + "owner": "go-architecture-assistant" + }, + { + "anchor": "go-k8s-crd/use-bborbe-k8s", + "applies_when": "a Go service consuming a CRD hand-writes the event-handler cast adapter, the typed event handler, or the in-memory state store, while the service already depends on (or could depend on) `github.com/bborbe/k8s`.", + "doc_path": "docs/go-kubernetes-crd-controller-guide.md", + "enforcement": "judgment (file-existence + dependency-graph check: presence of `pkg/event-handler*.go` or `pkg/-store.go` alongside a non-trivial `bborbe/k8s` import means hand-written boilerplate that should use the generic primitives)", + "id": "go-k8s-crd/use-bborbe-k8s", + "level": "SHOULD", + "owner": "go-architecture-assistant" + }, { "anchor": "go-licensing/copyright-year-discipline", "applies_when": "a PR diff modifies copyright years in `*.go` source-file headers — either bulk-updating across many files or setting future / non-numeric years (`2099`, `present`, etc.).", @@ -440,6 +476,24 @@ "level": "MUST", "owner": "go-security-specialist" }, + { + "anchor": "go-service-impl/no-context-object-injection", + "applies_when": "a Go service method receives a struct named `*Context`, `*ServiceContext`, `*Deps`, or similar that bundles multiple service dependencies (logger, repository, validator, etc.) and passes them through method calls instead of through the constructor.", + "doc_path": "docs/go-service-implementation-patterns.md", + "enforcement": "judgment (ast-grep follow-up: method signature with a parameter type matching `*Context` / `*Deps` where the struct contains 2+ service-interface fields)", + "id": "go-service-impl/no-context-object-injection", + "level": "MUST", + "owner": "go-architecture-assistant" + }, + { + "anchor": "go-service-impl/provider-vs-registry-choice", + "applies_when": "a Go service needs to dispatch to one of several implementations and the code uses the wrong dispatch shape for the set size: a `map[string]Creator` registry for a fixed compile-time set (< ~10 types) or a compiled `switch` for a runtime-extensible set.", + "doc_path": "docs/go-service-implementation-patterns.md", + "enforcement": "judgment (semantic — depends on whether the implementation set is closed or open)", + "id": "go-service-impl/provider-vs-registry-choice", + "level": "SHOULD", + "owner": "go-architecture-assistant" + }, { "anchor": "go-state-machine/forward-only-by-default", "applies_when": "a Go FSM worker emits `NextPhase` referring to an earlier phase in the workflow's declared order, without an explicit circuit-breaker `attempt` counter and a controller-side cap.", From d1762929db2ca158a07d59d0d3f03db2b2bdf5b1 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 15:14:07 +0200 Subject: [PATCH 2/5] =?UTF-8?q?fix(rules):=20address=20bot=20review=20on?= =?UTF-8?q?=20PR=20#18=20=E2=80=94=204=20mine,=204=20pre-existing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot raised 3 CRITICAL + 5 MAJOR + 2 NIT. Mix of issues I introduced (fixing) and pre-existing problems in the affected docs (deferring to focused follow-ups rather than inflating this PR's scope). FIXED (issues I introduced): - no-context-object-injection: enforcement said '*Context' (pointer-only) but my Bad example uses value-passed 'svcCtx ServiceContext'. Updated enforcement to match both shapes. - use-bborbe-k8s rule block placement: was at the top of section 0 before the bullet list explaining what k8s.NewEventHandler / k8s.NewResourceEventHandler do. Moved to AFTER the bullet list so the Good example doesn't reference undefined functions. - generated-client-not-dynamic rule block placement: was under section 2 (Repository layout / CRD library subsection), but the rule scopes to the consumer side. Moved to section 4 (K8sConnector interface, consumer side) where the dynamic-client smell actually lives. - singular-option-type wording: 'wrong-singular pattern' was too strong for a SHOULD rule. Reworded as 'suboptimal pair naming'; noted both shapes work and the rule promotes the industry-standard pair for clarity, not as a correctness fix. Aligns with the doc's pre-existing 'Acceptable but less clear' framing of the alternative naming. NOT FIXED (pre-existing in the affected docs, out of this PR's scope): - go-service-implementation-patterns.md HTTP handler examples (lines 456, 500) return raw http.HandlerFunc instead of libhttp.WithError. Pre-existing — not introduced by this PR's 2 rule blocks. Should be a focused follow-up that either fixes the examples or replaces the section with a cross-reference to go-http-handler-refactoring-guide.md. - go-service-implementation-patterns.md Factory Pattern Implementation switch (lines 301-308) lacks a default case — nil panic risk. Pre-existing. Follow-up should add the default returning errors.Errorf, matching the Good example in the Decision Framework section earlier in the same doc. - go-functional-options-pattern.md CreateSaramaConfig (line 108) uses Create* factory prefix while returning error AND containing a for loop — violates go-factory/no-error-return and go-factory/no-conditional-in-body. Pre-existing. Follow-up should rename to NewSaramaConfig (constructor convention) or clarify it's not a factory.go function. The 2 NITs (inline-typed option functions vs named XxxOption type in advanced-patterns examples; bare-noun-detection in enforcement description) are skipped — not blocking, can be addressed in a follow-up doc-polish pass. make build-index regenerated; check-index passes. --- docs/go-functional-options-pattern.md | 2 +- docs/go-kubernetes-crd-controller-guide.md | 72 +++++++++++----------- docs/go-service-implementation-patterns.md | 4 +- rules/index.json | 10 +-- 4 files changed, 44 insertions(+), 44 deletions(-) diff --git a/docs/go-functional-options-pattern.md b/docs/go-functional-options-pattern.md index 7b4c9ee..94fa0fb 100644 --- a/docs/go-functional-options-pattern.md +++ b/docs/go-functional-options-pattern.md @@ -515,7 +515,7 @@ func NewConsumer(options ...func(*ConsumerOptions)) Consumer { ### RULE go-functional-options/singular-option-type (SHOULD) **Owner**: go-quality-assistant -**Applies when**: a Go package using the functional-options pattern names the function type with a plural `XxxOptions` (matching the struct) instead of the singular `XxxOption`, OR names the config struct with the singular `XxxOption` (clashing with the function type). +**Applies when**: a Go package using the functional-options pattern uses suboptimal pair naming — function type with plural `XxxOptions` (matching the struct so they collide semantically), OR config struct with singular `XxxOption` (clashing with the function type). Both shapes work; this rule promotes the industry-standard singular-function / plural-struct pair for clarity, not as a correctness fix. **Enforcement**: judgment (ast-grep follow-up: `type_alias_declaration` / `type_declaration` with `XxxOptions func(...)` matching the wrong-singular pattern, and `struct_type` named `XxxOption`) **Why**: The pair-naming convention — `XxxOption` (singular function type) + `XxxOptions` (plural config struct) — makes the relationship at the type signature unambiguous: one `XxxOption` modifies one `XxxOptions`. Swapping them or using the same word for both makes every reader pause to remember which is which. Industry standard (Dave Cheney's original pattern, gRPC, OpenTelemetry, k8s client-go) follows the singular-function / plural-struct shape; deviating costs reader-onboarding effort with no upside. diff --git a/docs/go-kubernetes-crd-controller-guide.md b/docs/go-kubernetes-crd-controller-guide.md index abf05b2..5003a56 100644 --- a/docs/go-kubernetes-crd-controller-guide.md +++ b/docs/go-kubernetes-crd-controller-guide.md @@ -4,6 +4,15 @@ How to define and consume a Kubernetes Custom Resource Definition (CRD) in a Go ## 0. Before you start — use `bborbe/k8s` +[`github.com/bborbe/k8s`](https://github.com/bborbe/k8s) provides generic primitives that eliminate most of the boilerplate in sections 5–6 of this guide: + +- `k8s.Type` — interface your types implement (`Equal`, `Identifier`, `Validate`, `String`) +- `k8s.EventHandler[T Type]` — typed event handler interface (`OnAdd` / `OnUpdate` / `OnDelete` / `Get`) +- `k8s.NewEventHandler[T Type]()` — generic thread-safe in-memory store +- `k8s.NewResourceEventHandler[T Type](ctx, handler)` → `cache.ResourceEventHandler` adapter + +If your service already depends on `bborbe/k8s` (most do), use these instead of hand-writing the store and adapter. The hand-written skeletons in sections 5–6 are documented only for services that cannot take the dependency. + ### RULE go-k8s-crd/use-bborbe-k8s (SHOULD) **Owner**: go-architecture-assistant @@ -17,7 +26,7 @@ How to define and consume a Kubernetes Custom Resource Definition (CRD) in a Go // pkg/event-handler.go — hand-written cast adapter // pkg/event-handler-user.go — typed handler with OnAdd/OnUpdate/OnDelete // pkg/user-store.go — sync.RWMutex + map[string]*User, 60+ lines -// (three files of boilerplate per CRD) +// (three files of boilerplate per CRD — re-invents the same bugs every service) ``` #### Good @@ -30,15 +39,6 @@ informer.AddEventHandler(adapter) // done — typed, thread-safe, tested upstream ``` -[`github.com/bborbe/k8s`](https://github.com/bborbe/k8s) provides generic primitives that eliminate most of the boilerplate in sections 5–6 of this guide: - -- `k8s.Type` — interface your types implement (`Equal`, `Identifier`, `Validate`, `String`) -- `k8s.EventHandler[T Type]` — typed event handler interface (`OnAdd` / `OnUpdate` / `OnDelete` / `Get`) -- `k8s.NewEventHandler[T Type]()` — generic thread-safe in-memory store -- `k8s.NewResourceEventHandler[T Type](ctx, handler)` → `cache.ResourceEventHandler` adapter - -If your service already depends on `bborbe/k8s` (most do), use these instead of hand-writing the store and adapter. The hand-written skeletons in sections 5–6 are documented only for services that cannot take the dependency. - ## 1. When to use this pattern Use a CRD when: @@ -83,32 +83,6 @@ myservice/ - `github.com/bborbe/alert` (library) - `github.com/bborbe/cqrs/cdb` + `github.com/bborbe/cqrs/raw` (libraries) -### RULE go-k8s-crd/generated-client-not-dynamic (MUST) - -**Owner**: go-architecture-assistant -**Applies when**: a Go service interacts with a first-party CRD (the CR schema is known at compile time, defined in the same repo or a sibling library) using `k8s.io/client-go/dynamic` instead of a typed clientset generated via `hack/update-codegen.sh`. -**Enforcement**: judgment (import-graph check: `client-go/dynamic` imported alongside a known-schema CR type is the smell; dynamic-client use is acceptable only when the CR is third-party with unknown schema) -**Why**: The dynamic client returns `*unstructured.Unstructured` — every field access is a string lookup, every value comes back as `interface{}`, every typo is a runtime error. Generated clientsets return typed Go structs: typos fail at compile time, IDE auto-complete works, refactors propagate. The dynamic client is the right tool when you don't know the schema (admin tools, generic operators, schema discovery); for first-party CRDs where you wrote the types, it's the wrong tool — it discards every type-safety guarantee Go offers. - -#### Bad - -```go -// Dynamic client for a first-party CRD — every field access is stringly-typed -client := dynamic.NewForConfigOrDie(config) -gvr := schema.GroupVersionResource{Group: "example.com", Version: "v1", Resource: "users"} -obj, err := client.Resource(gvr).Namespace("default").Get(ctx, "alice", metav1.GetOptions{}) -// obj.Object["spec"].(map[string]interface{})["email"].(string) ← typo-prone, runtime-only -``` - -#### Good - -```go -// Generated typed clientset — fields are real Go types -clientset := versioned.NewForConfigOrDie(config) -user, err := clientset.ExampleV1().Users("default").Get(ctx, "alice", metav1.GetOptions{}) -// user.Spec.Email ← compile-time checked -``` - The client must be **generated** via `hack/update-codegen.sh` — do NOT hand-write or use `client-go/dynamic`. The dynamic client is acceptable only when the CR schema is unknown at compile time; that is never the case for a first-party CRD. ### Consumer service (controller/watcher) @@ -300,6 +274,32 @@ No `go mod vendor` step is required before `generatek8s` — the generator reads ## 4. K8sConnector interface (consumer side) +### RULE go-k8s-crd/generated-client-not-dynamic (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: a Go consumer service interacts with a first-party CRD (the CR schema is known at compile time, defined in the same repo or a sibling library) using `k8s.io/client-go/dynamic` instead of the typed clientset generated via `hack/update-codegen.sh`. +**Enforcement**: judgment (import-graph check on the consumer service: `client-go/dynamic` imported alongside a known-schema CR type is the smell; dynamic-client use is acceptable only when the CR is third-party with unknown schema) +**Why**: The dynamic client returns `*unstructured.Unstructured` — every field access is a string lookup, every value comes back as `interface{}`, every typo is a runtime error. Generated clientsets return typed Go structs: typos fail at compile time, IDE auto-complete works, refactors propagate. The dynamic client is the right tool when you don't know the schema (admin tools, generic operators, schema discovery); for first-party CRDs where you wrote the types, it's the wrong tool — it discards every type-safety guarantee Go offers. + +#### Bad + +```go +// Dynamic client for a first-party CRD — every field access is stringly-typed +client := dynamic.NewForConfigOrDie(config) +gvr := schema.GroupVersionResource{Group: "example.com", Version: "v1", Resource: "users"} +obj, err := client.Resource(gvr).Namespace("default").Get(ctx, "alice", metav1.GetOptions{}) +// obj.Object["spec"].(map[string]interface{})["email"].(string) ← typo-prone, runtime-only +``` + +#### Good + +```go +// Generated typed clientset — fields are real Go types +clientset := versioned.NewForConfigOrDie(config) +user, err := clientset.ExampleV1().Users("default").Get(ctx, "alice", metav1.GetOptions{}) +// user.Spec.Email ← compile-time checked +``` + Every bborbe CRD consumer has this exact interface: ```go diff --git a/docs/go-service-implementation-patterns.md b/docs/go-service-implementation-patterns.md index a8056fc..c0a4283 100644 --- a/docs/go-service-implementation-patterns.md +++ b/docs/go-service-implementation-patterns.md @@ -196,8 +196,8 @@ type PaymentHandler interface { // Focuses on technical aspect ### RULE go-service-impl/no-context-object-injection (MUST) **Owner**: go-architecture-assistant -**Applies when**: a Go service method receives a struct named `*Context`, `*ServiceContext`, `*Deps`, or similar that bundles multiple service dependencies (logger, repository, validator, etc.) and passes them through method calls instead of through the constructor. -**Enforcement**: judgment (ast-grep follow-up: method signature with a parameter type matching `*Context` / `*Deps` where the struct contains 2+ service-interface fields) +**Applies when**: a Go service method receives a struct (by value OR by pointer) named `Context` / `ServiceContext` / `Deps` / etc. that bundles multiple service dependencies (logger, repository, validator, etc.) and passes them through method calls instead of through the constructor. +**Enforcement**: judgment (ast-grep follow-up: method signature with a parameter type matching `Context` or `*Context` / `Deps` or `*Deps` where the struct contains 2+ service-interface fields — value and pointer shapes share the anti-pattern) **Why**: Context-object injection is the rebrand of global state — every method becomes "give me everything, I'll pick what I need." Three failure modes: (1) compile-time can't tell which deps a method actually uses, so refactors and dead-code detection break; (2) tests need to construct a full context object for every call site, even for methods that touch one dependency; (3) the context grows over time (the "we'll just add one more field" trap), and unused fields linger forever. Constructor injection forces the dep set to be minimal and visible at the type signature. #### Bad diff --git a/rules/index.json b/rules/index.json index f1d6ff5..24ddef5 100644 --- a/rules/index.json +++ b/rules/index.json @@ -244,7 +244,7 @@ }, { "anchor": "go-functional-options/singular-option-type", - "applies_when": "a Go package using the functional-options pattern names the function type with a plural `XxxOptions` (matching the struct) instead of the singular `XxxOption`, OR names the config struct with the singular `XxxOption` (clashing with the function type).", + "applies_when": "a Go package using the functional-options pattern uses suboptimal pair naming — function type with plural `XxxOptions` (matching the struct so they collide semantically), OR config struct with singular `XxxOption` (clashing with the function type). Both shapes work; this rule promotes the industry-standard singular-function / plural-struct pair for clarity, not as a correctness fix.", "doc_path": "docs/go-functional-options-pattern.md", "enforcement": "judgment (ast-grep follow-up: `type_alias_declaration` / `type_declaration` with `XxxOptions func(...)` matching the wrong-singular pattern, and `struct_type` named `XxxOption`)", "id": "go-functional-options/singular-option-type", @@ -316,9 +316,9 @@ }, { "anchor": "go-k8s-crd/generated-client-not-dynamic", - "applies_when": "a Go service interacts with a first-party CRD (the CR schema is known at compile time, defined in the same repo or a sibling library) using `k8s.io/client-go/dynamic` instead of a typed clientset generated via `hack/update-codegen.sh`.", + "applies_when": "a Go consumer service interacts with a first-party CRD (the CR schema is known at compile time, defined in the same repo or a sibling library) using `k8s.io/client-go/dynamic` instead of the typed clientset generated via `hack/update-codegen.sh`.", "doc_path": "docs/go-kubernetes-crd-controller-guide.md", - "enforcement": "judgment (import-graph check: `client-go/dynamic` imported alongside a known-schema CR type is the smell; dynamic-client use is acceptable only when the CR is third-party with unknown schema)", + "enforcement": "judgment (import-graph check on the consumer service: `client-go/dynamic` imported alongside a known-schema CR type is the smell; dynamic-client use is acceptable only when the CR is third-party with unknown schema)", "id": "go-k8s-crd/generated-client-not-dynamic", "level": "MUST", "owner": "go-architecture-assistant" @@ -478,9 +478,9 @@ }, { "anchor": "go-service-impl/no-context-object-injection", - "applies_when": "a Go service method receives a struct named `*Context`, `*ServiceContext`, `*Deps`, or similar that bundles multiple service dependencies (logger, repository, validator, etc.) and passes them through method calls instead of through the constructor.", + "applies_when": "a Go service method receives a struct (by value OR by pointer) named `Context` / `ServiceContext` / `Deps` / etc. that bundles multiple service dependencies (logger, repository, validator, etc.) and passes them through method calls instead of through the constructor.", "doc_path": "docs/go-service-implementation-patterns.md", - "enforcement": "judgment (ast-grep follow-up: method signature with a parameter type matching `*Context` / `*Deps` where the struct contains 2+ service-interface fields)", + "enforcement": "judgment (ast-grep follow-up: method signature with a parameter type matching `Context` or `*Context` / `Deps` or `*Deps` where the struct contains 2+ service-interface fields — value and pointer shapes share the anti-pattern)", "id": "go-service-impl/no-context-object-injection", "level": "MUST", "owner": "go-architecture-assistant" From ded6b4ae6a40bd48f835210da89e3b735512c72c Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 15:28:13 +0200 Subject: [PATCH 3/5] fix(rules): address PR #18 second-pass review on d176292 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 2 MAJOR + 1 NIT — all valid, all fixed: MAJOR 1: singular-option-type Bad example used 'type ConsumerOptions func(*ConsumerOptions)' which doesn't compile (duplicate declaration). Rewrote with two compilable cases that actually demonstrate the naming collision: function-type-plural forces struct to use Config instead, struct-singular forces function-type to use OptionFunc instead. Both shapes show the collision-induced split-naming. MAJOR 2: provider-vs-registry-choice Bad example uses func init() which simultaneously violates go-architecture/no-globals-or-singletons (MUST). Added inline cross-reference note in the Bad example so readers see both rule violations. NIT: no-context-object-injection Good example referenced &emailService{...} without showing the struct definition. Added the explicit type declaration so the example is self-contained (also models the private-struct-matches-interface pattern incidentally). --- docs/go-functional-options-pattern.md | 19 +++++++++++++++---- docs/go-service-implementation-patterns.md | 11 ++++++++++- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/docs/go-functional-options-pattern.md b/docs/go-functional-options-pattern.md index 94fa0fb..8c33f89 100644 --- a/docs/go-functional-options-pattern.md +++ b/docs/go-functional-options-pattern.md @@ -522,11 +522,22 @@ func NewConsumer(options ...func(*ConsumerOptions)) Consumer { #### Bad ```go -// Function type plural — collides semantically with the struct -type ConsumerOptions func(*ConsumerOptions) // and the struct is also ConsumerOptions? +// Function type uses plural — semantically collides with the typical +// XxxOptions struct name. To avoid a duplicate declaration, the config +// struct is then forced to a different word (Config), splitting the +// naming pair across two unrelated nouns. +type ConsumerOptions func(*ConsumerConfig) +type ConsumerConfig struct { + BatchSize int + Timeout time.Duration +} -// Or: function type singular, struct also singular -type ServerOption func(*ServerOption) // is ServerOption the modifier or the config? +// Or: config struct uses singular — collides with the typical XxxOption +// function-type name. Function type then needs a different word. +type ServerOption struct { + Port int +} +type ServerOptionFunc func(*ServerOption) ``` #### Good diff --git a/docs/go-service-implementation-patterns.md b/docs/go-service-implementation-patterns.md index c0a4283..b61104d 100644 --- a/docs/go-service-implementation-patterns.md +++ b/docs/go-service-implementation-patterns.md @@ -23,7 +23,10 @@ This guide captures practical decision-making patterns and implementation choice #### Bad ```go -// Fixed 3-implementation set, runtime registry — overkill +// Fixed 3-implementation set, runtime registry — overkill. +// Also violates `go-architecture/no-globals-or-singletons` (MUST): init() +// initialises a package-level Registry with all the bound creators — +// untestable in parallel, hidden dependency graph. type ProcessorRegistry struct { processors map[string]ProcessorCreator } @@ -222,6 +225,12 @@ type EmailService interface { Send(ctx context.Context, msg Message) error } +type emailService struct { + smtpClient SMTPClient + templateRepo TemplateRepository + logger log.Logger +} + func NewEmailService( smtpClient SMTPClient, templateRepo TemplateRepository, From 080a344ccd8393e7d449ccf4f64c4df28d219b2a Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 15:36:34 +0200 Subject: [PATCH 4/5] fix(rules): drop ambiguous cardinality qualifier from provider-vs-registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot approved PR #18 on ded6b4a with one MAJOR — remove the '< ~10 types' qualifier from go-service-impl/provider-vs-registry-choice. The real distinction is closed-vs-open set, not cardinality; the count-based hint creates ambiguity (does cardinality trigger the rule, or is cardinality just a proxy for the set being closed?). The rule's prose already correctly characterises the decision as closed-set→switch / open-set→registry, so the qualifier is redundant and confusing. --- docs/go-service-implementation-patterns.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/go-service-implementation-patterns.md b/docs/go-service-implementation-patterns.md index b61104d..a860fdb 100644 --- a/docs/go-service-implementation-patterns.md +++ b/docs/go-service-implementation-patterns.md @@ -16,7 +16,7 @@ This guide captures practical decision-making patterns and implementation choice ### RULE go-service-impl/provider-vs-registry-choice (SHOULD) **Owner**: go-architecture-assistant -**Applies when**: a Go service needs to dispatch to one of several implementations and the code uses the wrong dispatch shape for the set size: a `map[string]Creator` registry for a fixed compile-time set (< ~10 types) or a compiled `switch` for a runtime-extensible set. +**Applies when**: a Go service needs to dispatch to one of several implementations and the code uses the wrong dispatch shape for the set's openness: a `map[string]Creator` registry for a closed compile-time set, or a compiled `switch` for an open runtime-extensible set. **Enforcement**: judgment (semantic — depends on whether the implementation set is closed or open) **Why**: Static `switch` is faster (compiled jump table vs. map lookup + indirect call), trivially exhaustive (compiler-checked via `default:` + `errors.Errorf`), and refactor-friendly (renames propagate). Dynamic map-based registries are necessary when implementations register themselves at runtime (plugin systems, third-party extensions) but the cost — lost compile-time exhaustiveness, harder-to-test, registration-order sensitivity — is real. Match the shape to the problem: closed set → switch; open set → registry. Defaulting to one or the other regardless of context produces both unnecessary overhead and brittle systems. From 812593923e6b508c8a1ca322a68c834b5bc4c15b Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 15:36:47 +0200 Subject: [PATCH 5/5] fix(rules): regen index for provider-vs-registry-choice applies_when MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #13's check-index target caught a precommit failure on 080a344 — the doc edit changed the applies_when text but I committed without running 'make build-index' first. The drift guard worked as designed. This commit picks up the regenerated rules/index.json. --- rules/index.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/index.json b/rules/index.json index 24ddef5..069de26 100644 --- a/rules/index.json +++ b/rules/index.json @@ -487,7 +487,7 @@ }, { "anchor": "go-service-impl/provider-vs-registry-choice", - "applies_when": "a Go service needs to dispatch to one of several implementations and the code uses the wrong dispatch shape for the set size: a `map[string]Creator` registry for a fixed compile-time set (< ~10 types) or a compiled `switch` for a runtime-extensible set.", + "applies_when": "a Go service needs to dispatch to one of several implementations and the code uses the wrong dispatch shape for the set's openness: a `map[string]Creator` registry for a closed compile-time set, or a compiled `switch` for an open runtime-extensible set.", "doc_path": "docs/go-service-implementation-patterns.md", "enforcement": "judgment (semantic — depends on whether the implementation set is closed or open)", "id": "go-service-impl/provider-vs-registry-choice",