diff --git a/CLAUDE.md b/CLAUDE.md index be3ffb6..59a1598 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -20,7 +20,7 @@ Each enforceable guide in `docs/` should have a matching agent in `agents/`. The | Doc | Agent | |-----|-------| -| `go-architecture-patterns.md` | `go-quality-assistant` | +| `go-architecture-patterns.md` | `go-architecture-assistant` (rule-block owner; cross-unit concerns) + `go-quality-assistant` (broader review) | | `go-context-cancellation-in-loops.md` | `go-context-assistant` | | `go-error-wrapping-guide.md` | `go-error-assistant` | | `go-time-injection.md` | `go-time-assistant` | diff --git a/docs/go-architecture-patterns.md b/docs/go-architecture-patterns.md index 994b464..fe4ccd2 100644 --- a/docs/go-architecture-patterns.md +++ b/docs/go-architecture-patterns.md @@ -13,6 +13,33 @@ The standard pattern for Go packages in the Services follows this structure: ## 1. Interface → Constructor → Struct → Method Pattern +### RULE go-architecture/counterfeiter-directive-on-interface (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: an exported `interface` declaration in a non-`main` Go service package (i.e. likely substituted via mocks in tests) has no preceding `//counterfeiter:generate` line. Concrete trigger: any package with `*_test.go` files that import a `mocks` package, plus every exported interface in that package. +**Enforcement**: judgment (ast-grep partial: pattern over `interface_declaration` with surrounding-comment context per the PR #11 struct-literal recipe — negative-precedes relations are awkward in ast-grep 0.43.0, so the agent does the absence check) +**Why**: Hand-written mocks drift silently — when the interface gains a method, the mock keeps satisfying the old surface and tests pass against a stale contract. The `//counterfeiter:generate` directive forces `go generate ./...` to regenerate the fake, so any drift surfaces immediately at code-gen time. Missing the directive means the fake isn't regenerated, the test doesn't exercise the new method, and the bug ships. + +#### Bad + +```go +// No counterfeiter directive — mock won't regenerate when interface changes +type UserService interface { + Create(ctx context.Context, user User) error + Get(ctx context.Context, id UserID) (*User, error) +} +``` + +#### Good + +```go +//counterfeiter:generate -o ../mocks/service-user-service.go --fake-name ServiceUserService . UserService +type UserService interface { + Create(ctx context.Context, user User) error + Get(ctx context.Context, id UserID) (*User, error) +} +``` + ### Interface Definition Always start with a clear interface definition with counterfeiter comments for mock generation: @@ -34,6 +61,58 @@ type UserService interface { - Use descriptive interface names ending with the service purpose - Document the interface purpose clearly +### RULE go-architecture/new-prefix-constructor-naming (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: a Go function outside `pkg/factory/**` returns an exported interface or struct type and is intended to be the canonical construction site, but the function name does not start with `New`. +**Enforcement**: judgment (ast-grep follow-up: `function_declaration` returning a service-interface type with `name` not matching `^New`. `Create*` factories under `pkg/factory/**` are covered by `go-factory/no-impl-in-factory-pkg` instead and MUST be excluded from this rule's scope). +**Why**: `New*` is the universal Go signal for "this is the constructor — give it deps, get back a ready-to-use object". Without it, consumers can't tell `UserService(...)` from a regular function call; tooling (IDE search, godoc grouping, godoc renderers) treats it as ordinary; refactors don't surface the construction site. The convention is cheap; ignoring it costs every consumer a second of "wait, is this the constructor?". The `Create*` factory prefix is a deliberate exception scoped to `pkg/factory/**` — that's the factory pattern's home, not the service-construction site this rule covers. + +#### Bad + +```go +// Service-package construction sites — should all be New* +func MakeUserService(db bolt.DB, logger log.Logger) UserService { ... } +func BuildUserService(db bolt.DB, logger log.Logger) UserService { ... } +func UserSvc(db bolt.DB, logger log.Logger) UserService { ... } +``` + +#### Good + +```go +func NewUserService( + db bolt.DB, + logger log.Logger, + currentDateTime libtime.CurrentDateTime, + userValidator UserValidator, +) UserService { ... } +``` + +### RULE go-architecture/constructor-returns-interface (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: a Go `New*` constructor returns the concrete struct type (`*userService`) instead of the interface it implements (`UserService`). +**Enforcement**: judgment (ast-grep follow-up: `function_declaration` with name `^New` and result containing the concrete struct, paired with the corresponding interface declaration in the same package) +**Why**: Returning the concrete struct leaks implementation: callers can reach for non-interface methods, type-assert downstream, depend on private struct fields via reflection. Returning the interface forces every consumer through the contract surface — refactors stay contained, mocks stay drop-in, dependency direction stays one-way. + +#### Bad + +```go +// Returns concrete struct — leaks implementation +func NewUserService(...) *userService { + return &userService{...} +} +``` + +#### Good + +```go +// Returns interface — consumers see only the contract +func NewUserService(...) UserService { + return &userService{...} +} +``` + ### Constructor Function Create constructor functions using the `New*` pattern: @@ -60,6 +139,31 @@ func NewUserService( - Return the interface type, not the concrete struct - Order dependencies logically (db, external services, utilities) +### RULE go-architecture/private-struct-matches-interface (SHOULD) + +**Owner**: go-architecture-assistant +**Applies when**: a Go package exposes an exported interface (e.g. `UserService`) and the package contains exactly one struct implementing every method of that interface, but the struct's name is not the interface name with the first letter lowercased (`userService`). +**Enforcement**: judgment (paired-declaration scan: for each exported interface, find structs with matching method sets in the same package and check name correspondence — single-implementation packages only) +**Why**: When `UserService`'s implementation is `userService`, every reader knows at a glance which struct backs which interface — godoc renders them adjacent, IDE outlines pair them, refactors find them with one rename. When the implementation is `defaultUserService` / `userServiceImpl` / `internalUser`, every reader has to grep to find the link, and "which struct implements this interface" becomes a research task. + +#### Bad + +```go +type UserService interface { ... } + +// Mismatched name — pairing not self-evident +type userServiceImpl struct { ... } +``` + +#### Good + +```go +type UserService interface { ... } + +// Same name, first letter lowercased — pairing self-evident +type userService struct { ... } +``` + ### Private Struct Implementation ```go @@ -446,6 +550,50 @@ func TestUserService_Create_Success(t *testing.T) { ## 7. Dependency Injection Best Practices +### RULE go-architecture/no-globals-or-singletons (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: a Go service package introduces a service dependency (logger, DB, HTTP client, time getter, etc.) via any of: (a) a package-level `var` declaration, (b) initialisation inside `func init()`, or (c) lazy initialisation via `sync.Once` keyed to a package-level pointer. All three patterns share the same test-ordering and parallelism problems. +**Enforcement**: judgment (ast-grep follow-up: `kind: var_spec` at file scope + `kind: function_declaration` named `init` + `sync.Once` patterns with package-level state; full enforcement needs package-scope reasoning to distinguish service deps from constants and config values). +**Why**: Package-level service deps are global state. They (1) make tests order-dependent (one test mutates the global, the next sees it), (2) prevent parallelism (`go test -p N` shares the var), (3) hide the dependency graph (callers can't see what's used), and (4) make refactors fragile (changing the dep means tracing every package that imports the global). Constructor injection makes the graph explicit and the lifecycle controllable. + +#### Bad + +```go +// Package-level globals — every consumer shares them +var ( + defaultLogger = log.New() + defaultDB = bolt.MustOpen("data.db") + defaultNow = libtime.NewCurrentDateTime() +) + +func DoWork(ctx context.Context) error { + defaultLogger.Info("starting") // implicit dep — hidden from caller + return defaultDB.Update(ctx, ...) +} +``` + +#### Good + +```go +type UserService interface { + Create(ctx context.Context, user User) error +} + +func NewUserService( + logger log.Logger, + db bolt.DB, + currentDateTime libtime.CurrentDateTime, +) UserService { + return &userService{logger: logger, db: db, currentDateTime: currentDateTime} +} + +func (u *userService) Create(ctx context.Context, user User) error { + u.logger.Info("creating user") // explicit dep — caller controls + return u.db.Update(ctx, ...) +} +``` + ### Service Composition - Inject all dependencies through constructors - Use interfaces for all dependencies @@ -638,6 +786,52 @@ func (s *service) ProcessDocument(ctx context.Context, document *Document) error } ``` +### RULE go-architecture/business-logic-not-in-main (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: `main.go` (production code only — `main_test.go` is exempt) or `application.Run` contains domain operations (validation, business rules, data transformation, decision logic) instead of delegating to a service in `pkg/`. +**Enforcement**: judgment (semantic check — what counts as "business logic" requires reading the code). Coarse ast-grep filter is possible: `main.go` containing imports of `bborbe/errors` or `bborbe/validation` is a strong signal that domain logic leaked out of `pkg/`; production-file-scoped, `_test.go` excluded. +**Why**: `main.go` is for wiring: parsing flags, building dependencies, starting goroutines, handling shutdown. Business logic in `main.go` is untestable (Ginkgo suites can't exercise it without `gexec.Build` overhead), unreachable from other binaries (CLI tool vs HTTP server vs worker — they should share `pkg/` code), and impossible to refactor without touching the entry-point. Keep `main.go` thin; push every domain operation into a service. + +#### Bad + +```go +// Domain logic mixed into main.go +func (a *application) Run(ctx context.Context, sentryClient sentry.Client) error { + // ... setup code ... + + // Business logic — wrong place + user := User{Name: "John"} + if user.Name == "" { + return errors.New("invalid user") + } + if err := persistUser(user); err != nil { + return err + } + return nil +} +``` + +#### Good + +```go +// main.go wires; pkg/ owns domain +func (a *application) Run(ctx context.Context, sentryClient sentry.Client) error { + // ... setup code ... + service := pkg.NewUserService(a.db, a.logger, a.currentDateTime) + if err := service.ProcessUsers(ctx); err != nil { + return errors.Wrap(ctx, err, "process users failed") + } + return nil +} + +// pkg/user-service.go has the domain +func (s *userService) ProcessUsers(ctx context.Context) error { + // All validation, persistence, decision logic here + return nil +} +``` + ### DON'T: Mix business logic in main.go ```go // DON'T DO THIS - business logic in main.go diff --git a/rules/index.json b/rules/index.json index 2e594d2..3acfe98 100644 --- a/rules/index.json +++ b/rules/index.json @@ -62,6 +62,60 @@ "level": "SHOULD", "owner": "agent-auditor" }, + { + "anchor": "go-architecture/business-logic-not-in-main", + "applies_when": "`main.go` (production code only — `main_test.go` is exempt) or `application.Run` contains domain operations (validation, business rules, data transformation, decision logic) instead of delegating to a service in `pkg/`.", + "doc_path": "docs/go-architecture-patterns.md", + "enforcement": "judgment (semantic check — what counts as \"business logic\" requires reading the code). Coarse ast-grep filter is possible: `main.go` containing imports of `bborbe/errors` or `bborbe/validation` is a strong signal that domain logic leaked out of `pkg/`; production-file-scoped, `_test.go` excluded.", + "id": "go-architecture/business-logic-not-in-main", + "level": "MUST", + "owner": "go-architecture-assistant" + }, + { + "anchor": "go-architecture/constructor-returns-interface", + "applies_when": "a Go `New*` constructor returns the concrete struct type (`*userService`) instead of the interface it implements (`UserService`).", + "doc_path": "docs/go-architecture-patterns.md", + "enforcement": "judgment (ast-grep follow-up: `function_declaration` with name `^New` and result containing the concrete struct, paired with the corresponding interface declaration in the same package)", + "id": "go-architecture/constructor-returns-interface", + "level": "MUST", + "owner": "go-architecture-assistant" + }, + { + "anchor": "go-architecture/counterfeiter-directive-on-interface", + "applies_when": "an exported `interface` declaration in a non-`main` Go service package (i.e. likely substituted via mocks in tests) has no preceding `//counterfeiter:generate` line. Concrete trigger: any package with `*_test.go` files that import a `mocks` package, plus every exported interface in that package.", + "doc_path": "docs/go-architecture-patterns.md", + "enforcement": "judgment (ast-grep partial: pattern over `interface_declaration` with surrounding-comment context per the PR #11 struct-literal recipe — negative-precedes relations are awkward in ast-grep 0.43.0, so the agent does the absence check)", + "id": "go-architecture/counterfeiter-directive-on-interface", + "level": "MUST", + "owner": "go-architecture-assistant" + }, + { + "anchor": "go-architecture/new-prefix-constructor-naming", + "applies_when": "a Go function outside `pkg/factory/**` returns an exported interface or struct type and is intended to be the canonical construction site, but the function name does not start with `New`.", + "doc_path": "docs/go-architecture-patterns.md", + "enforcement": "judgment (ast-grep follow-up: `function_declaration` returning a service-interface type with `name` not matching `^New`. `Create*` factories under `pkg/factory/**` are covered by `go-factory/no-impl-in-factory-pkg` instead and MUST be excluded from this rule's scope).", + "id": "go-architecture/new-prefix-constructor-naming", + "level": "MUST", + "owner": "go-architecture-assistant" + }, + { + "anchor": "go-architecture/no-globals-or-singletons", + "applies_when": "a Go service package introduces a service dependency (logger, DB, HTTP client, time getter, etc.) via any of: (a) a package-level `var` declaration, (b) initialisation inside `func init()`, or (c) lazy initialisation via `sync.Once` keyed to a package-level pointer. All three patterns share the same test-ordering and parallelism problems.", + "doc_path": "docs/go-architecture-patterns.md", + "enforcement": "judgment (ast-grep follow-up: `kind: var_spec` at file scope + `kind: function_declaration` named `init` + `sync.Once` patterns with package-level state; full enforcement needs package-scope reasoning to distinguish service deps from constants and config values).", + "id": "go-architecture/no-globals-or-singletons", + "level": "MUST", + "owner": "go-architecture-assistant" + }, + { + "anchor": "go-architecture/private-struct-matches-interface", + "applies_when": "a Go package exposes an exported interface (e.g. `UserService`) and the package contains exactly one struct implementing every method of that interface, but the struct's name is not the interface name with the first letter lowercased (`userService`).", + "doc_path": "docs/go-architecture-patterns.md", + "enforcement": "judgment (paired-declaration scan: for each exported interface, find structs with matching method sets in the same package and check name correspondence — single-implementation packages only)", + "id": "go-architecture/private-struct-matches-interface", + "level": "SHOULD", + "owner": "go-architecture-assistant" + }, { "anchor": "go-context/cancel-check-in-loop", "applies_when": "Go `for` loop body lacks a non-blocking `select { case <-ctx.Done(): ...; default: }` check, outside `*_test.go` and `vendor/`.",