Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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` |
Expand Down
194 changes: 194 additions & 0 deletions docs/go-architecture-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
54 changes: 54 additions & 0 deletions rules/index.json
Original file line number Diff line number Diff line change
Expand Up @@ -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/`.",
Expand Down
Loading