diff --git a/CHANGELOG.md b/CHANGELOG.md index bdc115f..b45805d 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 + +- fix: stop false-positive counterfeiter and Parse*Default findings (counterfeiter-directive-on-interface is now an over-inclusive mechanical anchor + judgment-tier adjudication; new-prefix-constructor-naming exempts ParseDefault) + ## v0.45.0 - feat: promote 1 m3 finding, three-model-agreed (m3+m2.7+deepseek all flagged `tts-mcp#13` `CHANGELOG.md` conventional-prefix violation) and deepseek-verified with runnable evidence, into the golden set (`golden-curated-2` → `golden-curated-3`, 157 → 158 entries). Of the 16 three-way agreement locations surfaced by [[Triangulate the Golden Set Across Models]], only 1 was genuinely new and verified — 3 others were re-statements of existing entries (caught by the aliasing guard, correctly dropped), 12 were rejected on defensible grounds (DRY, documented contract, pre-existing). Net effect on scores vs `golden-curated-2` (re-scored against the enlarged 158-entry set): m3 recall 0.066 → **0.072** (+1 hit, m3's own finding now counts), deepseek 0.109 → 0.109 (no change), opus 0.832 → **0.826** (−1 hit, the new entry is a finding opus missed — the self-match tautology loosening, as expected when the set grows). The triangulation principle's doing its job: agreement across models is necessary but not sufficient; aliasing checks remain essential to avoid silent double-counting diff --git a/docs/go-architecture-patterns.md b/docs/go-architecture-patterns.md index 4eb752a..aa956a2 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**: 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**: `rules/go/counterfeiter-directive-on-interface.yml` +**Applies when**: an exported `interface` declaration in a non-`main` Go service package (i.e. likely substituted via mocks in tests). The ast-grep anchor fires on every exported interface — over-inclusive by design, because the `//counterfeiter:generate` directive commonly sits above the interface's doc comment (a block of several lines), which ast-grep's immediate-sibling relations cannot see. The agent adjudicates: reads the comment block directly above the interface and reports only when no `//counterfeiter:generate` directive is present. +**Enforcement**: `rules/go/counterfeiter-directive-on-interface.yml` (mechanical first-pass — flags every exported interface declaration in production code) + judgment-tier LLM adjudication for whether a `//counterfeiter:generate` directive exists above the interface (the directive may sit above the interface's doc comment, which ast-grep's immediate-sibling relations cannot see) **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 @@ -65,7 +65,7 @@ type UserService interface { **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**: `rules/go/new-prefix-constructor-naming.yml` (mechanical first-pass flags exported functions returning exported types without `New` prefix; `pkg/factory/**` excluded) + judgment-tier LLM adjudication to rule out `Create*` factories, helper functions, and non-constructor uses. +**Enforcement**: `rules/go/new-prefix-constructor-naming.yml` (mechanical first-pass flags exported functions returning exported types without `New` prefix; `pkg/factory/**` excluded, and `ParseDefault` paired-default functions exempted so this rule cannot contradict go-parse/paired-parse-and-parsedefault) + judgment-tier LLM adjudication to rule out `Create*` factories, helper functions, and non-constructor uses. **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 diff --git a/rule-tests/__snapshots__/go-architecture/counterfeiter-directive-on-interface-snapshot.yml b/rule-tests/__snapshots__/go-architecture/counterfeiter-directive-on-interface-snapshot.yml new file mode 100644 index 0000000..125cc95 --- /dev/null +++ b/rule-tests/__snapshots__/go-architecture/counterfeiter-directive-on-interface-snapshot.yml @@ -0,0 +1,108 @@ +id: go-architecture/counterfeiter-directive-on-interface +snapshots: + ? | + package pkg + import "context" + // UserService is the user-facing surface. + type Service interface { + Create(ctx context.Context, user User) error + Get(ctx context.Context, id string) (*User, error) + } + : labels: + - source: |- + type Service interface { + Create(ctx context.Context, user User) error + Get(ctx context.Context, id string) (*User, error) + } + style: primary + start: 72 + end: 198 + - source: |- + interface { + Create(ctx context.Context, user User) error + Get(ctx context.Context, id string) (*User, error) + } + style: secondary + start: 85 + end: 198 + - source: Service + style: secondary + start: 77 + end: 84 + - source: |- + Service interface { + Create(ctx context.Context, user User) error + Get(ctx context.Context, id string) (*User, error) + } + style: secondary + start: 77 + end: 198 + ? | + package pkg + import "context" + //counterfeiter:generate -o mocks/service.go --fake-name Service . Service + + // Service handles the user lifecycle. + // Implementations MUST be concurrency-safe. + type Service interface { + Create(ctx context.Context, user User) error + } + : labels: + - source: |- + type Service interface { + Create(ctx context.Context, user User) error + } + style: primary + start: 189 + end: 262 + - source: |- + interface { + Create(ctx context.Context, user User) error + } + style: secondary + start: 202 + end: 262 + - source: Service + style: secondary + start: 194 + end: 201 + - source: |- + Service interface { + Create(ctx context.Context, user User) error + } + style: secondary + start: 194 + end: 262 + ? | + package pkg + import "context" + //counterfeiter:generate -o mocks/service.go --fake-name Service . Service + type Service interface { + Create(ctx context.Context, user User) error + } + : labels: + - source: |- + type Service interface { + Create(ctx context.Context, user User) error + } + style: primary + start: 104 + end: 177 + - source: |- + interface { + Create(ctx context.Context, user User) error + } + style: secondary + start: 117 + end: 177 + - source: Service + style: secondary + start: 109 + end: 116 + - source: |- + Service interface { + Create(ctx context.Context, user User) error + } + style: secondary + start: 109 + end: 177 diff --git a/rule-tests/counterfeiter-directive-on-interface-test.yml b/rule-tests/counterfeiter-directive-on-interface-test.yml new file mode 100644 index 0000000..1b5d05a --- /dev/null +++ b/rule-tests/counterfeiter-directive-on-interface-test.yml @@ -0,0 +1,46 @@ +id: go-architecture/counterfeiter-directive-on-interface +valid: + # Struct type — not an interface, not flagged + - | + package pkg + type UserService struct { db *DB } + # Unexported interface — no mockability obligation + - | + package pkg + type userService interface { + Get(ctx context.Context, id string) (User, error) + } +invalid: + # Exported interface without directive — flagged; agent verifies no + # //counterfeiter:generate sits above it (through its doc comment) + - | + package pkg + import "context" + // UserService is the user-facing surface. + type Service interface { + Create(ctx context.Context, user User) error + Get(ctx context.Context, id string) (*User, error) + } + # Exported interface whose directive sits one line above — still anchored + # (over-inclusive first pass); the agent reads the block above and confirms + # the directive exists, so it is NOT reported as a finding. + - | + package pkg + import "context" + //counterfeiter:generate -o mocks/service.go --fake-name Service . Service + type Service interface { + Create(ctx context.Context, user User) error + } + # Exported interface with directive above its multi-line doc comment — + # still anchored (over-inclusive first pass); the agent confirms the + # directive exists above the doc block, so it is NOT reported. + - | + package pkg + import "context" + //counterfeiter:generate -o mocks/service.go --fake-name Service . Service + + // Service handles the user lifecycle. + // Implementations MUST be concurrency-safe. + type Service interface { + Create(ctx context.Context, user User) error + } diff --git a/rule-tests/new-prefix-constructor-naming-test.yml b/rule-tests/new-prefix-constructor-naming-test.yml index 1a5f4f4..3f4a0fa 100644 --- a/rule-tests/new-prefix-constructor-naming-test.yml +++ b/rule-tests/new-prefix-constructor-naming-test.yml @@ -11,6 +11,12 @@ valid: package p type userService struct{} func newUserService() *userService { return nil } + # Correct: ParseDefault paired-default function — exempt, naming mandated + # by paired-parse-and-parsedefault + - | + package p + type Config struct{ Host string } + func ParseConfigDefault(ctx context.Context, value string, def Config) Config { return def } invalid: # Make* prefix - | diff --git a/rules/go/counterfeiter-directive-on-interface.yml b/rules/go/counterfeiter-directive-on-interface.yml index 67f4af6..3a7448e 100644 --- a/rules/go/counterfeiter-directive-on-interface.yml +++ b/rules/go/counterfeiter-directive-on-interface.yml @@ -2,26 +2,36 @@ id: go-architecture/counterfeiter-directive-on-interface language: go severity: error message: | - Every Go interface in a service package needs a //counterfeiter:generate - directive so go generate regenerates the fake when the interface drifts. - Without it, hand-written mocks drift silently — tests pass against a - stale contract while the production code adds methods. + An exported Go interface in a service package was found. Each interface + substituted via mocks in tests needs a //counterfeiter:generate directive + so go generate regenerates the fake when the interface drifts. Without + it, hand-written mocks drift silently — tests pass against a stale + contract while the production code adds methods. + EXEMPT: interfaces that already carry a //counterfeiter:generate + directive above them (possibly separated from the interface by its doc + comment), and unexported/internal interfaces not used across package + boundaries. The agent checks the comment block directly above the + interface before reporting. See docs/go-architecture-patterns.md (RULE go-architecture/counterfeiter-directive-on-interface). rule: - # ast-grep 0.43.0 shape: match a type_declaration that wraps a type_spec - # wrapping an interface_type, with the type name starting with an - # uppercase letter (Go exported convention), AND is NOT preceded - # (follows in tree-sitter 'previous-sibling' direction) by a comment - # containing 'counterfeiter:generate'. + # Anchor: an exported interface type declaration in production code. + # The agent adjudicates: reads the comment block directly above the + # interface and confirms whether a //counterfeiter:generate directive is + # present. The directive commonly sits above the interface's doc comment, + # and that block can be several lines long — ast-grep's `follows`/`precedes` + # relations only reach the IMMEDIATE previous sibling, so a doc comment + # between the directive and the interface makes the directive invisible to + # a mechanical `not.follows` check. The rule is therefore intentionally + # over-inclusive (flags every exported interface) and delegates the + # present-or-missing verdict to the adjudicator, matching the design of the + # sibling rule go-testing/counterfeiter-mocks-required and of the other + # judgment-backed detectors (main-test-with-compiles, external-call-logs- + # response, secret-fields-need-display-length, slog-not-glog-in-new-projects). # - # Verified the relation direction during smoke: 'not.follows' means - # "this node does NOT come after the matched sibling" — which for a - # type_declaration whose comment is the previous sibling is what we want. - # - # Exportedness filter (added after PR #31 review): the rule only - # applies to exported interfaces (those substituted via mocks in - # tests). Unexported / internal interfaces (lowercase names) are - # package-private and don't carry mockability obligations. + # Exportedness filter (kept from PR #31): the rule only anchors exported + # interfaces (uppercase name) — those substituted via mocks in tests. + # Unexported / internal interfaces are package-private and don't carry + # mockability obligations. kind: type_declaration has: kind: type_spec @@ -31,10 +41,6 @@ rule: - has: kind: type_identifier regex: '^[A-Z]' - not: - follows: - kind: comment - regex: 'counterfeiter:generate' ignores: - "main.go" - "**/main.go" diff --git a/rules/go/new-prefix-constructor-naming.yml b/rules/go/new-prefix-constructor-naming.yml index ec75848..54fcc63 100644 --- a/rules/go/new-prefix-constructor-naming.yml +++ b/rules/go/new-prefix-constructor-naming.yml @@ -5,10 +5,15 @@ message: | Exported function returns an exported type but name does not start with 'New'. 'New*' is the universal Go signal for "this is the constructor". Without it consumers cannot tell UserService(...) from a regular function; tooling cannot surface it as a constructor. + EXEMPT: ParseDefault paired-default functions — go-parse/paired-parse-and-parsedefault + mandates that exact name; a New prefix would contradict it. Also exempt helper functions + that happen to return an exported type, and test helpers. See docs/go-architecture-patterns.md (RULE go-architecture/new-prefix-constructor-naming). rule: # Anchor: exported function_declaration whose name does NOT start with "New" but whose result - # is an exported type_identifier (interface or struct type alias). + # is an exported type_identifier (interface or struct type alias), AND is not a + # ParseDefault paired-default function (that naming is mandated by the + # paired-parse-and-parsedefault rule — firing on it here would contradict it). # The LLM adjudicates: rules out Create* factories in pkg/factory/** (covered by a different rule), # helper functions that happen to return an exported type, and test helpers. kind: function_declaration @@ -18,7 +23,9 @@ rule: kind: identifier regex: '^[A-Z]' not: - regex: '^New' + any: + - regex: '^New' + - regex: '^Parse[A-Z].*Default$' - has: field: result kind: type_identifier diff --git a/rules/index.json b/rules/index.json index 170c687..98766c8 100644 --- a/rules/index.json +++ b/rules/index.json @@ -241,9 +241,9 @@ }, { "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.", + "applies_when": "an exported `interface` declaration in a non-`main` Go service package (i.e. likely substituted via mocks in tests). The ast-grep anchor fires on every exported interface — over-inclusive by design, because the `//counterfeiter:generate` directive commonly sits above the interface's doc comment (a block of several lines), which ast-grep's immediate-sibling relations cannot see. The agent adjudicates: reads the comment block directly above the interface and reports only when no `//counterfeiter:generate` directive is present.", "doc_path": "docs/go-architecture-patterns.md", - "enforcement": "`rules/go/counterfeiter-directive-on-interface.yml`", + "enforcement": "`rules/go/counterfeiter-directive-on-interface.yml` (mechanical first-pass — flags every exported interface declaration in production code) + judgment-tier LLM adjudication for whether a `//counterfeiter:generate` directive exists above the interface (the directive may sit above the interface's doc comment, which ast-grep's immediate-sibling relations cannot see)", "enforcement_type": "mechanical", "id": "go-architecture/counterfeiter-directive-on-interface", "level": "MUST", @@ -253,7 +253,7 @@ "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": "`rules/go/new-prefix-constructor-naming.yml` (mechanical first-pass flags exported functions returning exported types without `New` prefix; `pkg/factory/**` excluded) + judgment-tier LLM adjudication to rule out `Create*` factories, helper functions, and non-constructor uses.", + "enforcement": "`rules/go/new-prefix-constructor-naming.yml` (mechanical first-pass flags exported functions returning exported types without `New` prefix; `pkg/factory/**` excluded, and `ParseDefault` paired-default functions exempted so this rule cannot contradict go-parse/paired-parse-and-parsedefault) + judgment-tier LLM adjudication to rule out `Create*` factories, helper functions, and non-constructor uses.", "enforcement_type": "mechanical", "id": "go-architecture/new-prefix-constructor-naming", "level": "MUST",