From 9955aaa7e8f766104f104c849e63e8847f9441ed Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 20 Jun 2026 11:53:53 +0200 Subject: [PATCH 1/2] feat: add rule go-composition/no-same-package-private-helper-for-business-logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing go-composition/no-package-function-calls-in-business-logic rule only matches cross-package qualified calls (pkg.Func via ast-grep $PKG.$FN pattern). New business logic added as a same-package private helper escapes the funnel entirely — the syntactic pattern does not match, and adjudicators routinely dismiss adjacent findings as 'pre-existing pattern' (broken-windows fallacy). This new judgment-tier rule explicitly names the same-package case: new business logic (data transformation, validation, formatting) must be defined as interface + constructor + struct + method, with a counterfeiter directive and a dedicated *_test.go. Codified after PR bborbe/recurring-task-creator#16 shipped a private buildFrontmatter helper; local pr-review pass dismissed the related cross-package findings as pre-existing patterns. User caught the missing interface seam manually post-review and asked for the guide to be updated so it gets caught automatically next time. - docs/go-composition.md — new RULE section with Bad/Good code, exemption list, concrete trigger criteria, and codification note - rules/index.json — registers the rule (judgment-tier, owner go-architecture-assistant, trigger "**/*.go") --- CHANGELOG.md | 4 +++ docs/go-composition.md | 81 ++++++++++++++++++++++++++++++++++++++++++ rules/index.json | 13 +++++++ 3 files changed, 98 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50ee2e0..6729967 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ Please choose versions by [Semantic Versioning](http://semver.org/). * MINOR version when you add functionality in a backwards-compatible manner, and * PATCH version when you make backwards-compatible bug fixes. +## Unreleased + +- feat: add RULE `go-composition/no-same-package-private-helper-for-business-logic` (MUST, judgment-tier, owner go-architecture-assistant) — flags new business logic added as a same-package private helper instead of behind an interface + constructor + struct + method seam. Closes the gap left by the cross-package version (which only matches `pkg.Func` syntax). Codified after PR bborbe/recurring-task-creator#16 shipped a private `buildFrontmatter` helper that local pr-review dismissed as "pre-existing pattern" (broken-windows fallacy). + ## v0.22.0 - feat!: selector mode is now the DEFAULT for `/coding:pr-review` and `/coding:code-review` (was opt-in via `--selector`/`selector` token); callers passing `standard` now get selector behavior — BREAKING CHANGE for any tooling that relied on `standard` triggering per-owner Task dispatch diff --git a/docs/go-composition.md b/docs/go-composition.md index c5a13a9..0ccce9a 100644 --- a/docs/go-composition.md +++ b/docs/go-composition.md @@ -65,6 +65,87 @@ func NewRunner(scanner PromptScanner, executor Executor, releaser Releaser) Runn } ``` +### RULE go-composition/no-same-package-private-helper-for-business-logic (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: a Go service package introduces new business logic (data transformation, validation, formatting — anything beyond pure plumbing) as a same-package private helper function called from a method, instead of behind a small interface + constructor + struct + method seam. +**Enforcement**: judgment (semantic — distinguishing "business logic that needs a seam" from "trivial inline helper" requires reading the function body; ast-grep cannot reliably classify intent. Mechanical first-pass via `rules/go/no-package-function-calls-in-business-logic.yml` catches cross-package `pkg.Func` calls; this rule catches the same architectural smell in its same-package private-helper disguise). +**Why**: A private helper is **just as much a hidden dependency as a cross-package call** — the constructor doesn't surface it, tests can't mock it, replacing the implementation requires editing every call site instead of swapping one constructor argument. The cross-package version is easy to spot (`pkg.Func()` syntax stands out); the same-package version is invisible (`helperFunc()` looks like a local variable until you grep). Both ship the same untestability, both break the same way under refactor. The interface + constructor + struct + method pattern is the universal answer (see go-architecture-patterns.md): every new capability gets its own seam, its own counterfeiter mock, its own *_test.go. + +**Concrete trigger** — flag in code review when: +1. A new (or substantially-rewritten) function is added to a service package, AND +2. Its body contains conditionals, loops, or non-trivial data transformation (not just one line of glue), AND +3. It is called from a method on another type in the same package (the consumer would otherwise have to construct it or import a package-level reference), AND +4. There is no companion interface + constructor + struct exposing the function as a method. + +**Exempt**: +- Pure formatting helpers used only inside the same file by one caller, with no test surface (e.g. `func fmtIsoWeek(year, week int) string` in render.go). +- Test helpers in `*_test.go` files (these have no production seam to maintain). +- Generated code. + +#### Bad + +```go +// publisher.go — buildFrontmatter is a private helper doing business logic +// (default seeding, placeholder rendering, provenance override). It cannot +// be mocked, has no isolated test, and locks Publisher's tests into testing +// frontmatter behavior transitively through Publish(). + +func buildFrontmatter(operator lib.TaskFrontmatter, slug string, date schedule.Date) lib.TaskFrontmatter { + out := lib.TaskFrontmatter{"status": "in_progress", "page_type": "task"} + for k, v := range operator { + if s, ok := v.(string); ok { + out[k] = renderTemplate(s, slug, date) + continue + } + out[k] = v + } + out["created_by"] = "recurring-task-creator" + return out +} + +func (p *publisher) Publish(ctx context.Context, def schedule.TaskDefinition, date schedule.Date) error { + cmd := task.CreateCommand{ + // ... + Frontmatter: buildFrontmatter(def.Frontmatter, def.Slug, date), // hidden dep + } +} +``` + +#### Good + +```go +// frontmatter.go — interface + constructor + struct + method. Mockable, +// testable in isolation, swappable via constructor injection. + +//counterfeiter:generate -o ../../mocks/publisher-frontmatter-formatter.go --fake-name PublisherFrontmatterFormatter . FrontmatterFormatter +type FrontmatterFormatter interface { + Format(operator lib.TaskFrontmatter, slug string, date schedule.Date) lib.TaskFrontmatter +} + +func NewFrontmatterFormatter() FrontmatterFormatter { return &frontmatterFormatter{} } + +type frontmatterFormatter struct{} + +func (f *frontmatterFormatter) Format(/* ... */) lib.TaskFrontmatter { /* ... */ } + +// publisher.go — Publisher takes the formatter as a constructor dep. +func NewPublisher(sender task.CreateCommandSender, formatter FrontmatterFormatter, dryRun bool) Publisher { + return &publisher{sender: sender, formatter: formatter, dryRun: dryRun} +} + +func (p *publisher) Publish(ctx context.Context, def schedule.TaskDefinition, date schedule.Date) error { + cmd := task.CreateCommand{ + // ... + Frontmatter: p.formatter.Format(def.Frontmatter, def.Slug, date), // injected + } +} +``` + +**Test-file companion**: a `frontmatter_test.go` covers the formatter in isolation (every placeholder, every non-string passthrough, every default-vs-override edge); `publisher_test.go` can now either (a) keep using the real formatter for integration coverage, or (b) inject a `PublisherFrontmatterFormatter` counterfeiter mock to decouple Publisher's tests from formatter logic. Both are valid. + +**Codified 2026-06-20** after PR [bborbe/recurring-task-creator#16](https://github.com/bborbe/recurring-task-creator/pull/16) shipped `buildFrontmatter` as a private helper. The cross-package rule did not flag it (ast-grep pattern matches `pkg.Func`, not bare `func`). Local `/coding:pr-review` dismissed the related cross-package findings as "pre-existing patterns" — the *broken-windows fallacy* (existing pattern ≠ correct pattern; adding new code to a smelly file makes it smellier). User caught the missing interface seam manually post-review. + ## Anti-Pattern: God Object ```go diff --git a/rules/index.json b/rules/index.json index 667df9e..96b656b 100644 --- a/rules/index.json +++ b/rules/index.json @@ -1597,5 +1597,18 @@ "trigger": [ "**/*_test.go" ] + }, + { + "anchor": "go-composition/no-same-package-private-helper-for-business-logic", + "applies_when": "a Go service package introduces new business logic (data transformation, validation, formatting) as a same-package private helper function called from a method, instead of behind an interface + constructor + struct + method seam.", + "doc_path": "docs/go-composition.md", + "enforcement": "judgment (semantic — distinguishing business-logic helpers needing a seam from trivial inline helpers requires reading the function body)", + "enforcement_type": "judgment", + "id": "go-composition/no-same-package-private-helper-for-business-logic", + "level": "MUST", + "owner": "go-architecture-assistant", + "trigger": [ + "**/*.go" + ] } ] From cbff1de2a7c68f4c15b778c7a0d4649b65638b9d Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 20 Jun 2026 12:13:43 +0200 Subject: [PATCH 2/2] fix(rules): regenerate index from doc + add Trigger field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's check-index caught: rules/index.json was hand-edited and stale vs make build-index output. Two issues fixed: 1. enforcement_type was 'judgment' (manual) vs 'mechanical' (parser). derive_enforcement_type matches /\brules\/[a-z]+\/[a-z0-9_-]+\.yml\b/ against the enforcement text — my prose referenced the sibling rule no-package-function-calls-in-business-logic.yml as 'companion rule', which the heuristic mistook for THIS rule's own yml. Rephrased to reference the sibling by rule-id, not yml path. Parser now classifies correctly as judgment. 2. Doc was missing the **Trigger** field, so the generated entry lacked a trigger glob (rules without trigger are skipped by the dispatcher's active-judgment-rule computation in commands/pr-review.md Step 4b-i). Added '**Trigger**: **/*.go' matching the sibling rules' format (plain glob, no backticks — the field parser captures everything after the colon verbatim). Self-inflicted: I skipped 'make precommit' on the original PR commit ('it's just docs'). Same workflow violation the Development Guide warns about: doc-only carve-out doesn't mean skip-local-verification. Verified: make build-index + make precommit both green; the index entry now has enforcement_type=judgment + trigger=['**/*.go']. --- docs/go-composition.md | 3 ++- rules/index.json | 26 +++++++++++++------------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/docs/go-composition.md b/docs/go-composition.md index 0ccce9a..86674dc 100644 --- a/docs/go-composition.md +++ b/docs/go-composition.md @@ -69,7 +69,8 @@ func NewRunner(scanner PromptScanner, executor Executor, releaser Releaser) Runn **Owner**: go-architecture-assistant **Applies when**: a Go service package introduces new business logic (data transformation, validation, formatting — anything beyond pure plumbing) as a same-package private helper function called from a method, instead of behind a small interface + constructor + struct + method seam. -**Enforcement**: judgment (semantic — distinguishing "business logic that needs a seam" from "trivial inline helper" requires reading the function body; ast-grep cannot reliably classify intent. Mechanical first-pass via `rules/go/no-package-function-calls-in-business-logic.yml` catches cross-package `pkg.Func` calls; this rule catches the same architectural smell in its same-package private-helper disguise). +**Enforcement**: judgment (semantic — distinguishing "business logic that needs a seam" from "trivial inline helper" requires reading the function body; ast-grep cannot reliably classify intent because the call site is a bare identifier, not a `pkg.Func` selector). Companion rule `go-composition/no-package-function-calls-in-business-logic` handles the cross-package case via the mechanical funnel; this rule catches the same architectural smell in its same-package private-helper disguise. +**Trigger**: **/*.go **Why**: A private helper is **just as much a hidden dependency as a cross-package call** — the constructor doesn't surface it, tests can't mock it, replacing the implementation requires editing every call site instead of swapping one constructor argument. The cross-package version is easy to spot (`pkg.Func()` syntax stands out); the same-package version is invisible (`helperFunc()` looks like a local variable until you grep). Both ship the same untestability, both break the same way under refactor. The interface + constructor + struct + method pattern is the universal answer (see go-architecture-patterns.md): every new capability gets its own seam, its own counterfeiter mock, its own *_test.go. **Concrete trigger** — flag in code review when: diff --git a/rules/index.json b/rules/index.json index 96b656b..7c6e45f 100644 --- a/rules/index.json +++ b/rules/index.json @@ -333,6 +333,19 @@ "level": "MUST", "owner": "go-architecture-assistant" }, + { + "anchor": "go-composition/no-same-package-private-helper-for-business-logic", + "applies_when": "a Go service package introduces new business logic (data transformation, validation, formatting — anything beyond pure plumbing) as a same-package private helper function called from a method, instead of behind a small interface + constructor + struct + method seam.", + "doc_path": "docs/go-composition.md", + "enforcement": "judgment (semantic — distinguishing \"business logic that needs a seam\" from \"trivial inline helper\" requires reading the function body; ast-grep cannot reliably classify intent because the call site is a bare identifier, not a `pkg.Func` selector). Companion rule `go-composition/no-package-function-calls-in-business-logic` handles the cross-package case via the mechanical funnel; this rule catches the same architectural smell in its same-package private-helper disguise.", + "enforcement_type": "judgment", + "id": "go-composition/no-same-package-private-helper-for-business-logic", + "level": "MUST", + "owner": "go-architecture-assistant", + "trigger": [ + "**/*.go" + ] + }, { "anchor": "go-composition/small-interfaces-1-2-methods", "applies_when": "a Go interface declares 3+ methods spanning more than one logical capability — i.e. consumers of the interface use a subset of methods, and the unused-by-this-consumer methods are inferred from method-count plus call-site analysis.", @@ -1597,18 +1610,5 @@ "trigger": [ "**/*_test.go" ] - }, - { - "anchor": "go-composition/no-same-package-private-helper-for-business-logic", - "applies_when": "a Go service package introduces new business logic (data transformation, validation, formatting) as a same-package private helper function called from a method, instead of behind an interface + constructor + struct + method seam.", - "doc_path": "docs/go-composition.md", - "enforcement": "judgment (semantic — distinguishing business-logic helpers needing a seam from trivial inline helpers requires reading the function body)", - "enforcement_type": "judgment", - "id": "go-composition/no-same-package-private-helper-for-business-logic", - "level": "MUST", - "owner": "go-architecture-assistant", - "trigger": [ - "**/*.go" - ] } ]