From eb6b2eb345d01c6332fe2f5b3676fccdc5c1d401 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 13:35:25 +0200 Subject: [PATCH 1/3] feat(architecture): bootstrap 6 rule blocks in go-architecture-patterns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restructures the 6 enforceable conventions in docs/go-architecture-patterns.md into canonical `### RULE` blocks. Mirrors PRs #2-5, #8, #10, #14, #15. Rules added (rules/index.json: 50 -> 56, new go-architecture/* family): - go-architecture/counterfeiter-directive-on-interface (MUST) Every substitutable interface needs a //counterfeiter:generate directive so go generate regenerates the fake when the interface drifts. - go-architecture/new-prefix-constructor-naming (MUST) Constructors start with New (not Create/Make/etc). Partial overlap with go-factory/no-impl-in-factory-pkg for Create*. - go-architecture/constructor-returns-interface (MUST) New* returns the interface, not *concrete struct. Hides implementation, keeps dependency direction one-way. - go-architecture/private-struct-matches-interface (SHOULD) UserService -> userService (first letter lowered). godoc pairs them; IDE outlines pair them; refactors find them with one rename. - go-architecture/no-globals-or-singletons (MUST) Service deps via constructor injection, never package-level vars. Globals break parallel tests, hide the dep graph, make refactors fragile. - go-architecture/business-logic-not-in-main (MUST) main.go is wiring; domain operations live in pkg/. Mixed main.go is untestable and unreachable from other binaries. Cross-references to existing rules (already in index, not duplicated): - go-errors/no-context-background-in-business-logic — covers context discipline - go-context/cancel-check-in-loop — covers infinite-loop ctx.Done() checks - go-time/* family — covers libtime injection All examples generic (User, UserService, Worker, log/db deps). No trading-domain terms (KafkaBrokers grep hit is server infrastructure, not trading-broker terminology — pre-existing in main.go example). No personal vault paths. Pre-emptive checks (lessons from PRs #6, #8, #14, #15): clean grep, unique rule IDs (6 new vs 50 existing), go-architecture-assistant agent exists at agents/go-architecture-assistant.md, make build-index regenerated, check-index passes. --- docs/go-architecture-patterns.md | 193 +++++++++++++++++++++++++++++++ rules/index.json | 54 +++++++++ 2 files changed, 247 insertions(+) diff --git a/docs/go-architecture-patterns.md b/docs/go-architecture-patterns.md index 994b464..04bb1af 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**: a Go service package defines an `interface` declaration intended to be substitutable (mocked, stubbed, or replaced for testing) without a preceding `//counterfeiter:generate` directive. +**Enforcement**: judgment (ast-grep follow-up: `kind: interface_declaration` + `not.precedes` on a comment matching `//counterfeiter:generate`) +**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 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 type registered as a service interface; partial overlap with `go-factory/no-impl-in-factory-pkg` for the `Create*` family) +**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?". + +#### Bad + +```go +// Constructor doesn't carry the New* signal +func CreateUserService(db bolt.DB, logger log.Logger) UserService { ... } +func UserSvc(db bolt.DB, logger log.Logger) UserService { ... } +func MakeUserService(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,33 @@ 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 interface (e.g. `UserService`) implemented by a single struct whose name does not match the interface name with the first letter lowercased (`userService`). +**Enforcement**: judgment (paired-declaration check; ast-grep can detect the shape but the "single implementation" trigger is package-scope) +**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 obvious +type userServiceImpl struct { ... } +type defaultUserService struct { ... } +type internalUserService struct { ... } +``` + +#### Good + +```go +type UserService interface { ... } + +// Same name, first letter lowercased — pairing self-evident +type userService struct { ... } +``` + ### Private Struct Implementation ```go @@ -446,6 +552,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 declares a package-level `var` holding a service dependency (logger, DB, HTTP client, time getter, etc.) instead of receiving it through a `New*` constructor. +**Enforcement**: judgment (ast-grep follow-up: `kind: var_spec` at file scope with a type matching a known service interface; full enforcement needs package-scope reasoning) +**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 Worker interface { + DoWork(ctx context.Context) error +} + +func NewWorker( + logger log.Logger, + db bolt.DB, + currentDateTime libtime.CurrentDateTime, +) Worker { + return &worker{logger: logger, db: db, currentDateTime: currentDateTime} +} + +func (w *worker) DoWork(ctx context.Context) error { + w.logger.Info("starting") // explicit dep — caller controls + return w.db.Update(ctx, ...) +} +``` + ### Service Composition - Inject all dependencies through constructors - Use interfaces for all dependencies @@ -638,6 +788,49 @@ 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` 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) +**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) + return service.ProcessUsers(ctx) +} + +// 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..5d315a9 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` 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)", + "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": "a Go service package defines an `interface` declaration intended to be substitutable (mocked, stubbed, or replaced for testing) without a preceding `//counterfeiter:generate` directive.", + "doc_path": "docs/go-architecture-patterns.md", + "enforcement": "judgment (ast-grep follow-up: `kind: interface_declaration` + `not.precedes` on a comment matching `//counterfeiter:generate`)", + "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 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 type registered as a service interface; partial overlap with `go-factory/no-impl-in-factory-pkg` for the `Create*` family)", + "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 declares a package-level `var` holding a service dependency (logger, DB, HTTP client, time getter, etc.) instead of receiving it through a `New*` constructor.", + "doc_path": "docs/go-architecture-patterns.md", + "enforcement": "judgment (ast-grep follow-up: `kind: var_spec` at file scope with a type matching a known service interface; full enforcement needs package-scope reasoning)", + "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 interface (e.g. `UserService`) implemented by a single struct whose name does not match the interface name with the first letter lowercased (`userService`).", + "doc_path": "docs/go-architecture-patterns.md", + "enforcement": "judgment (paired-declaration check; ast-grep can detect the shape but the \"single implementation\" trigger is package-scope)", + "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/`.", From 6d9105c10a2b6863f8f8dc727cb84dcfb62d4d85 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 13:48:41 +0200 Subject: [PATCH 2/3] fix(architecture): address bot review on PR #16 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 7 MAJOR + 3 NIT findings from the bot — all valid, all fixed: MAJOR: - counterfeiter-directive-on-interface: removed invalid 'not.precedes' ast-grep claim. Real path is the PR #11 surrounding-comment recipe; agent handles the absence check. - counterfeiter-directive-on-interface: scoped applies_when to a concrete trigger ('exported interfaces in non-main packages with *_test.go importing mocks') instead of the intent-based 'intended to be substitutable'. - new-prefix-constructor-naming: exempted pkg/factory/** explicitly (Create* is the factory prefix per go-factory rule). Cross-reference added; the Bad example drops the CreateUserService case (it's a factory, not a service constructor). - private-struct-matches-interface: tightened to single-impl trigger in the applies_when (was 'a struct'; now 'exactly one struct implementing every method'). Bad example reduced to one mismatch (was showing three alternatives, conflicting with the trigger). - no-globals-or-singletons: applies_when now covers init() and sync.Once patterns alongside package-level vars. Same test-ordering and parallelism problems; MUST-level rule needs the complete trigger. - business-logic-not-in-main: explicit *_test.go exemption added; coarse ast-grep filter described (main.go importing bborbe/errors/validation = strong signal). - doc-agent alignment: CLAUDE.md table now maps go-architecture-patterns.md to both go-architecture-assistant (the new rule-block owner) and go-quality-assistant (broader review). Reflects the agent's cross-unit-concerns scope and resolves the bot's alignment gap. NIT: - new-prefix Bad example mislabeled CreateUserService as 'Constructor' — Create* is the factory prefix per section 3. Removed that case. - business-logic-not-in-main Good example added errors.Wrap to match the doc's error-wrapping pattern throughout. - no-globals-or-singletons Good example: Worker -> UserService for consistency with the doc's running example. make build-index regenerated; check-index passes. --- CLAUDE.md | 2 +- docs/go-architecture-patterns.md | 50 ++++++++++++++++---------------- rules/index.json | 20 ++++++------- 3 files changed, 36 insertions(+), 36 deletions(-) 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 04bb1af..a431909 100644 --- a/docs/go-architecture-patterns.md +++ b/docs/go-architecture-patterns.md @@ -16,8 +16,8 @@ The standard pattern for Go packages in the Services follows this structure: ### RULE go-architecture/counterfeiter-directive-on-interface (MUST) **Owner**: go-architecture-assistant -**Applies when**: a Go service package defines an `interface` declaration intended to be substitutable (mocked, stubbed, or replaced for testing) without a preceding `//counterfeiter:generate` directive. -**Enforcement**: judgment (ast-grep follow-up: `kind: interface_declaration` + `not.precedes` on a comment matching `//counterfeiter:generate`) +**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 @@ -64,17 +64,16 @@ type UserService interface { ### RULE go-architecture/new-prefix-constructor-naming (MUST) **Owner**: go-architecture-assistant -**Applies when**: a Go function 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 type registered as a service interface; partial overlap with `go-factory/no-impl-in-factory-pkg` for the `Create*` family) -**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?". +**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 -// Constructor doesn't carry the New* signal -func CreateUserService(db bolt.DB, logger log.Logger) UserService { ... } -func UserSvc(db bolt.DB, logger log.Logger) UserService { ... } +// Service-package construction site — should be New* func MakeUserService(db bolt.DB, logger log.Logger) UserService { ... } +func UserSvc(db bolt.DB, logger log.Logger) UserService { ... } ``` #### Good @@ -142,8 +141,8 @@ func NewUserService( ### RULE go-architecture/private-struct-matches-interface (SHOULD) **Owner**: go-architecture-assistant -**Applies when**: a Go package exposes an interface (e.g. `UserService`) implemented by a single struct whose name does not match the interface name with the first letter lowercased (`userService`). -**Enforcement**: judgment (paired-declaration check; ast-grep can detect the shape but the "single implementation" trigger is package-scope) +**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 @@ -151,10 +150,8 @@ func NewUserService( ```go type UserService interface { ... } -// Mismatched name — pairing not obvious +// Mismatched name — pairing not self-evident type userServiceImpl struct { ... } -type defaultUserService struct { ... } -type internalUserService struct { ... } ``` #### Good @@ -555,8 +552,8 @@ func TestUserService_Create_Success(t *testing.T) { ### RULE go-architecture/no-globals-or-singletons (MUST) **Owner**: go-architecture-assistant -**Applies when**: a Go service package declares a package-level `var` holding a service dependency (logger, DB, HTTP client, time getter, etc.) instead of receiving it through a `New*` constructor. -**Enforcement**: judgment (ast-grep follow-up: `kind: var_spec` at file scope with a type matching a known service interface; full enforcement needs package-scope reasoning) +**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 @@ -578,21 +575,21 @@ func DoWork(ctx context.Context) error { #### Good ```go -type Worker interface { +type UserService interface { DoWork(ctx context.Context) error } -func NewWorker( +func NewUserService( logger log.Logger, db bolt.DB, currentDateTime libtime.CurrentDateTime, -) Worker { - return &worker{logger: logger, db: db, currentDateTime: currentDateTime} +) UserService { + return &userService{logger: logger, db: db, currentDateTime: currentDateTime} } -func (w *worker) DoWork(ctx context.Context) error { - w.logger.Info("starting") // explicit dep — caller controls - return w.db.Update(ctx, ...) +func (u *userService) DoWork(ctx context.Context) error { + u.logger.Info("starting") // explicit dep — caller controls + return u.db.Update(ctx, ...) } ``` @@ -791,8 +788,8 @@ 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` 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) +**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 @@ -821,7 +818,10 @@ func (a *application) Run(ctx context.Context, sentryClient sentry.Client) error func (a *application) Run(ctx context.Context, sentryClient sentry.Client) error { // ... setup code ... service := pkg.NewUserService(a.db, a.logger, a.currentDateTime) - return service.ProcessUsers(ctx) + if err := service.ProcessUsers(ctx); err != nil { + return errors.Wrap(ctx, err, "process users failed") + } + return nil } // pkg/user-service.go has the domain diff --git a/rules/index.json b/rules/index.json index 5d315a9..3acfe98 100644 --- a/rules/index.json +++ b/rules/index.json @@ -64,9 +64,9 @@ }, { "anchor": "go-architecture/business-logic-not-in-main", - "applies_when": "`main.go` or `application.Run` contains domain operations (validation, business rules, data transformation, decision logic) instead of delegating to a service in `pkg/`.", + "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)", + "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" @@ -82,36 +82,36 @@ }, { "anchor": "go-architecture/counterfeiter-directive-on-interface", - "applies_when": "a Go service package defines an `interface` declaration intended to be substitutable (mocked, stubbed, or replaced for testing) without a preceding `//counterfeiter:generate` directive.", + "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 follow-up: `kind: interface_declaration` + `not.precedes` on a comment matching `//counterfeiter:generate`)", + "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 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`.", + "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 type registered as a service interface; partial overlap with `go-factory/no-impl-in-factory-pkg` for the `Create*` family)", + "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 declares a package-level `var` holding a service dependency (logger, DB, HTTP client, time getter, etc.) instead of receiving it through a `New*` constructor.", + "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 with a type matching a known service interface; full enforcement needs package-scope reasoning)", + "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 interface (e.g. `UserService`) implemented by a single struct whose name does not match the interface name with the first letter lowercased (`userService`).", + "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 check; ast-grep can detect the shape but the \"single implementation\" trigger is package-scope)", + "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" From efa890ffcc6cf7b9fd9fa3761af91b15e35af7ca Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 13:56:17 +0200 Subject: [PATCH 3/3] fix(architecture): address 2 MAJOR review findings on PR #16 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot verdict was approve but PR stayed CHANGES_REQUESTED due to 2 MAJOR example-polish findings. Both legit; fixing rather than burning an admin-merge cycle. MAJOR 1 — new-prefix-constructor-naming Bad example shrunk to 2 variants after CreateUserService was removed (factory overlap). Restored illustrative breadth: added BuildUserService alongside MakeUserService and UserSvc. MAJOR 2 — no-globals-or-singletons Good example used UserService interface name with a DoWork method — semantic mismatch with the doc's CRUD identity (Create/Get used everywhere else). Renamed DoWork to Create to match. --- docs/go-architecture-patterns.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/go-architecture-patterns.md b/docs/go-architecture-patterns.md index a431909..fe4ccd2 100644 --- a/docs/go-architecture-patterns.md +++ b/docs/go-architecture-patterns.md @@ -71,8 +71,9 @@ type UserService interface { #### Bad ```go -// Service-package construction site — should be New* +// 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 { ... } ``` @@ -576,7 +577,7 @@ func DoWork(ctx context.Context) error { ```go type UserService interface { - DoWork(ctx context.Context) error + Create(ctx context.Context, user User) error } func NewUserService( @@ -587,8 +588,8 @@ func NewUserService( return &userService{logger: logger, db: db, currentDateTime: currentDateTime} } -func (u *userService) DoWork(ctx context.Context) error { - u.logger.Info("starting") // explicit dep — caller controls +func (u *userService) Create(ctx context.Context, user User) error { + u.logger.Info("creating user") // explicit dep — caller controls return u.db.Update(ctx, ...) } ```