From 189263febb9c3de37fa054f26e38b3f6a48cdf75 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 22:16:18 +0200 Subject: [PATCH 1/3] =?UTF-8?q?feat(rules):=202=20mechanical=20ast-grep=20?= =?UTF-8?q?YAMLs=20=E2=80=94=20no-raw-go-func=20+=20cobra-not-stdlib-flag?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds mechanical enforcement for two high-leverage MUST rules previously in judgment-only mode. Both smoke-tested against fixtures before push. go-concurrency/no-raw-go-func.yml (new): - Pattern: 'kind: go_statement' — matches 'go func() {...}()' / 'go someMethod()' / 'go expr()' uniformly via Go's tree-sitter grammar. No metavariable constraints; the path-based ignores carry the main.go / cmd/** / _test.go exemptions. - Verified: catches both 'go func() {...}()' AND 'go doWork()' shapes in a test fixture. go-cli/cobra-not-stdlib-flag.yml (new): - Pattern: 'any:' over (a) import_spec with path '"flag"' (catches the standard import line shape) + (b) call_expression patterns for flag.Parse / flag.String / flag.Bool / flag.Int / flag.StringVar / flag.BoolVar / flag.IntVar (catches usage via dot-import or transitive aliasing). - Verified: matches stdlib flag usage; clean on cobra-using test fixture. Enforcement fields updated: - docs/go-concurrency-patterns.md: judgment (ast-grep follow-up) -> 'rules/go/no-raw-go-func.yml' - docs/go-cli-guide.md: same shape for cobra-not-stdlib-flag. rules/index.json regenerated; check-coverage clean (124 rules, 17 mechanical YAMLs — was 15 before this commit). check-index passes. No personal vault paths. --- docs/go-cli-guide.md | 2 +- docs/go-concurrency-patterns.md | 2 +- rules/go/cobra-not-stdlib-flag.yml | 33 ++++++++++++++++++++++++++++++ rules/go/no-raw-go-func.yml | 24 ++++++++++++++++++++++ rules/index.json | 4 ++-- 5 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 rules/go/cobra-not-stdlib-flag.yml create mode 100644 rules/go/no-raw-go-func.yml diff --git a/docs/go-cli-guide.md b/docs/go-cli-guide.md index 959a425..187e1e1 100644 --- a/docs/go-cli-guide.md +++ b/docs/go-cli-guide.md @@ -6,7 +6,7 @@ **Owner**: go-quality-assistant **Applies when**: a Go CLI binary's `main.go` / `pkg/cli/...` imports `flag` (stdlib) and calls `flag.String` / `flag.Bool` / `flag.Parse`, instead of using `github.com/spf13/cobra` (with its `pflag` library). -**Enforcement**: judgment (ast-grep follow-up: `import "flag"` in any `main` package + `call_expression` matching `flag.Parse` / `flag.String`. Test files exempt.) +**Enforcement**: `rules/go/cobra-not-stdlib-flag.yml` **Why**: Stdlib `flag` uses a process-global `flag.CommandLine` FlagSet. Any transitive dependency that calls `flag.String(...)` in its `init()` adds flags to this global set — and `github.com/golang/glog` is the most common offender, adding 8+ flags (`-alsologtostderr`, `-log_dir`, `-log_backtrace_at`, `-stderrthreshold`, `-v`, `-vmodule`, …) to every binary that transitively imports it. The result: `my-tool --help` displays a wall of irrelevant glog flags before your three actual flags, and the binary accepts those flags at runtime even though no one wanted them. Cobra uses `pflag` which is isolated from `flag.CommandLine` — the global pollution can't reach it, `--help` shows only your flags, and your flag namespace stays under your control. #### Bad diff --git a/docs/go-concurrency-patterns.md b/docs/go-concurrency-patterns.md index c46ff58..7da1a7a 100644 --- a/docs/go-concurrency-patterns.md +++ b/docs/go-concurrency-patterns.md @@ -8,7 +8,7 @@ **Owner**: go-architecture-assistant **Applies when**: a Go file uses the raw `go func() { ... }()` / `go someMethod(...)` syntax outside `main.go` / top-level entry points — instead of one of the `github.com/bborbe/run` strategies (`CancelOnFirstErrorWait`, `CancelOnFirstFinishWait`, `All`, `Sequential`). -**Enforcement**: judgment (ast-grep follow-up: `go_statement` outside `main.go` / `cmd/**`. Test files exempt; `main` entry-point goroutine spawners exempt by path filter) +**Enforcement**: `rules/go/no-raw-go-func.yml` **Why**: Raw goroutines have three failure modes the `run` package solves: (1) they leak when the parent context is cancelled but the goroutine doesn't observe it; (2) they race when the parent function returns before the goroutine writes its result; (3) error propagation requires hand-rolled channels + `sync.WaitGroup` that drift toward subtle deadlocks. `run.CancelOnFirstErrorWait` wires context cancellation, error aggregation, and synchronization in one call — every consumer learns the same primitives, refactors stay safe, and goroutine lifetimes are explicit at the type signature. #### Bad diff --git a/rules/go/cobra-not-stdlib-flag.yml b/rules/go/cobra-not-stdlib-flag.yml new file mode 100644 index 0000000..4371c12 --- /dev/null +++ b/rules/go/cobra-not-stdlib-flag.yml @@ -0,0 +1,33 @@ +id: go-cli/cobra-not-stdlib-flag +language: go +severity: error +message: | + stdlib 'flag' package must not be used for CLI parsing. + Transitive dependencies (most famously github.com/golang/glog) register + flags via init() and pollute --help output. Use github.com/spf13/cobra + with pflag instead. + See docs/go-cli-guide.md (RULE go-cli/cobra-not-stdlib-flag). +rule: + # ast-grep 0.43.0 shape: match `import_spec` nodes whose path text + # equals "flag" — Go's tree-sitter grammar represents each import line + # (in single or grouped form) as an import_spec node with a `path` + # field whose value is the quoted package path. + any: + - kind: import_spec + has: + field: path + regex: '^"flag"$' + # Also catch `flag.Parse()` / `flag.String(...)` call sites — in case + # someone uses the type-imported form via dot-import or transitively. + - pattern: flag.Parse() + - pattern: 'flag.String($$$ARGS)' + - pattern: 'flag.Bool($$$ARGS)' + - pattern: 'flag.Int($$$ARGS)' + - pattern: 'flag.StringVar($$$ARGS)' + - pattern: 'flag.BoolVar($$$ARGS)' + - pattern: 'flag.IntVar($$$ARGS)' +ignores: + - "**/*_test.go" + - "vendor/**" + - "**/vendor/**" + - "**/mocks/**" diff --git a/rules/go/no-raw-go-func.yml b/rules/go/no-raw-go-func.yml new file mode 100644 index 0000000..a262c42 --- /dev/null +++ b/rules/go/no-raw-go-func.yml @@ -0,0 +1,24 @@ +id: go-concurrency/no-raw-go-func +language: go +severity: error +message: | + Raw 'go func()' / 'go expr()' is forbidden outside main entry points. + Use github.com/bborbe/run strategies (CancelOnFirstErrorWait, All, + Sequential) — raw goroutines leak, race, and require hand-rolled + sync.WaitGroup that drift toward deadlocks. + See docs/go-concurrency-patterns.md (RULE go-concurrency/no-raw-go-func). +rule: + # ast-grep 0.43.0 shape: Go's tree-sitter grammar uses `go_statement` + # for `go expr()` statements. Direct kind match — no metavariable + # constraints needed; the path-based `ignores` carry the + # main.go / cmd/** / _test.go exemptions. + kind: go_statement +ignores: + - "main.go" + - "**/main.go" + - "cmd/**" + - "**/cmd/**" + - "**/*_test.go" + - "vendor/**" + - "**/vendor/**" + - "**/mocks/**" diff --git a/rules/index.json b/rules/index.json index c3a35d3..3908fbb 100644 --- a/rules/index.json +++ b/rules/index.json @@ -192,7 +192,7 @@ "anchor": "go-cli/cobra-not-stdlib-flag", "applies_when": "a Go CLI binary's `main.go` / `pkg/cli/...` imports `flag` (stdlib) and calls `flag.String` / `flag.Bool` / `flag.Parse`, instead of using `github.com/spf13/cobra` (with its `pflag` library).", "doc_path": "docs/go-cli-guide.md", - "enforcement": "judgment (ast-grep follow-up: `import \"flag\"` in any `main` package + `call_expression` matching `flag.Parse` / `flag.String`. Test files exempt.)", + "enforcement": "`rules/go/cobra-not-stdlib-flag.yml`", "id": "go-cli/cobra-not-stdlib-flag", "level": "MUST", "owner": "go-quality-assistant" @@ -237,7 +237,7 @@ "anchor": "go-concurrency/no-raw-go-func", "applies_when": "a Go file uses the raw `go func() { ... }()` / `go someMethod(...)` syntax outside `main.go` / top-level entry points — instead of one of the `github.com/bborbe/run` strategies (`CancelOnFirstErrorWait`, `CancelOnFirstFinishWait`, `All`, `Sequential`).", "doc_path": "docs/go-concurrency-patterns.md", - "enforcement": "judgment (ast-grep follow-up: `go_statement` outside `main.go` / `cmd/**`. Test files exempt; `main` entry-point goroutine spawners exempt by path filter)", + "enforcement": "`rules/go/no-raw-go-func.yml`", "id": "go-concurrency/no-raw-go-func", "level": "MUST", "owner": "go-architecture-assistant" From f6c2e4488b21613909c10a2c4d196bee4a0b89c9 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 22:24:57 +0200 Subject: [PATCH 2/3] =?UTF-8?q?fix(rules):=20address=20PR=20#29=20review?= =?UTF-8?q?=20=E2=80=94=20expand=20cobra=20patterns=20+=20exempt=20pkg/cli?= =?UTF-8?q?/=20from=20no-raw-go-func?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot caught 2 MAJOR coverage gaps: 1. cobra-not-stdlib-flag missed the rest of the flag.* family. Original covered String/Bool/Int + their *Var variants. Expanded to cover the complete primitive set: - Float64 + Float64Var - Duration + DurationVar - Int64 + Int64Var - Uint + UintVar - Uint64 + Uint64Var - Var (the generic flag.Var registration) - NewFlagSet (the known workaround for flag.CommandLine pollution) - CommandLine (direct global access) The NewFlagSet gap was the highest-leverage miss — it's how authors try to avoid the pollution problem the rule itself prevents. 2. no-raw-go-func had no exemption for the signal-listener pattern in pkg/cli/ — the canonical Execute() shape in go-cli-guide.md uses 'go func() { <-sigCh; cancel() }()' which is the documented correct pattern for that location. Added 'pkg/cli/**' and '**/pkg/cli/**' to ignores. Verified the fixture now passes (exit 0) while a non-pkg/cli/ raw goroutine still fires. NITs skipped: - bot's 'redundant main.go + **/main.go' comment is wrong (this duplication was added in PR #11 specifically because ast-grep's '**/main.go' doesn't match a root-level main.go; documented in docs/ast-grep-rule-writing-guide.md). Keeping both. make precommit clean. --- rules/go/cobra-not-stdlib-flag.yml | 19 +++++++++++++++++-- rules/go/no-raw-go-func.yml | 2 ++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/rules/go/cobra-not-stdlib-flag.yml b/rules/go/cobra-not-stdlib-flag.yml index 4371c12..3f3606a 100644 --- a/rules/go/cobra-not-stdlib-flag.yml +++ b/rules/go/cobra-not-stdlib-flag.yml @@ -17,15 +17,30 @@ rule: has: field: path regex: '^"flag"$' - # Also catch `flag.Parse()` / `flag.String(...)` call sites — in case - # someone uses the type-imported form via dot-import or transitively. + # Also catch flag.* call sites — covers the full primitive family + # (String/Bool/Int/Float64/Duration/Int64/Uint/Uint64) + their *Var + # variants, plus NewFlagSet (known workaround for the global pollution + # this rule prevents) and CommandLine direct access. - pattern: flag.Parse() - pattern: 'flag.String($$$ARGS)' - pattern: 'flag.Bool($$$ARGS)' - pattern: 'flag.Int($$$ARGS)' + - pattern: 'flag.Int64($$$ARGS)' + - pattern: 'flag.Uint($$$ARGS)' + - pattern: 'flag.Uint64($$$ARGS)' + - pattern: 'flag.Float64($$$ARGS)' + - pattern: 'flag.Duration($$$ARGS)' - pattern: 'flag.StringVar($$$ARGS)' - pattern: 'flag.BoolVar($$$ARGS)' - pattern: 'flag.IntVar($$$ARGS)' + - pattern: 'flag.Int64Var($$$ARGS)' + - pattern: 'flag.UintVar($$$ARGS)' + - pattern: 'flag.Uint64Var($$$ARGS)' + - pattern: 'flag.Float64Var($$$ARGS)' + - pattern: 'flag.DurationVar($$$ARGS)' + - pattern: 'flag.Var($$$ARGS)' + - pattern: 'flag.NewFlagSet($$$ARGS)' + - pattern: 'flag.CommandLine' ignores: - "**/*_test.go" - "vendor/**" diff --git a/rules/go/no-raw-go-func.yml b/rules/go/no-raw-go-func.yml index a262c42..0494ff5 100644 --- a/rules/go/no-raw-go-func.yml +++ b/rules/go/no-raw-go-func.yml @@ -18,6 +18,8 @@ ignores: - "**/main.go" - "cmd/**" - "**/cmd/**" + - "pkg/cli/**" + - "**/pkg/cli/**" - "**/*_test.go" - "vendor/**" - "**/vendor/**" From 15993280c3524b932bf40e8331cf766262403601 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 22:36:11 +0200 Subject: [PATCH 3/3] =?UTF-8?q?fix(rules):=20address=20PR=20#29=20second?= =?UTF-8?q?=20review=20=E2=80=94=20applies=5Fwhen=20alignment=20+=20flag.F?= =?UTF-8?q?unc?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot raised 3 MAJOR + 4 MINOR. Two MAJOR + all 4 MINOR are real scope-alignment issues; the third MAJOR is a hallucination (PR #29 does not touch commands/code-review.md). REAL FIXES: 1. cobra-not-stdlib-flag: applies_when text scoped to 'CLI binary's main.go / pkg/cli/...' but the YAML matches any non-test Go file. The YAML scope is the correct one — the failure mode this rule prevents (transitive flag.init() pollution) is broader than CLI binaries; a library calling flag.String() in init() pollutes every binary that imports it. Broadened applies_when text to match the YAML scope. 2. no-raw-go-func: pkg/cli/** exemption added in the prior commit wasn't documented in applies_when. Added the rationale to both the doc's applies_when text AND a comment block in the YAML itself, so the reader sees the exemption justification at both levels. The exemption is intentional: the canonical pkg/cli/Execute() pattern uses 'go func() { <-sigCh; cancel() }()' for signal-listener wiring; without the exemption every CLI's bootstrap fires a false positive. 3. flag.Func added to the cobra YAML pattern list. Go 1.16+ API for registering a custom-value-setter flag; was missing from the original primitive enumeration. NOT FIXED (bot hallucination): - 'commands/code-review.md was significantly simplified [removing the dispatcher pipeline]' is wrong. PR #29's diff does not touch commands/code-review.md at all. Bot is comparing against a stale base or confusing this PR with a different one. PR #28 (already merged) ADDED the dispatcher to code-review.md; PR #29 only touches rules/go/*.yml + docs/go-{cli,concurrency}-*.md + rules/index.json (the walker output). make precommit clean; check-coverage: OK (124 rules, 17 mechanical YAMLs). --- docs/go-cli-guide.md | 2 +- docs/go-concurrency-patterns.md | 2 +- rules/go/cobra-not-stdlib-flag.yml | 1 + rules/go/no-raw-go-func.yml | 7 +++++++ rules/index.json | 4 ++-- 5 files changed, 12 insertions(+), 4 deletions(-) diff --git a/docs/go-cli-guide.md b/docs/go-cli-guide.md index 187e1e1..4c27ce6 100644 --- a/docs/go-cli-guide.md +++ b/docs/go-cli-guide.md @@ -5,7 +5,7 @@ ### RULE go-cli/cobra-not-stdlib-flag (MUST) **Owner**: go-quality-assistant -**Applies when**: a Go CLI binary's `main.go` / `pkg/cli/...` imports `flag` (stdlib) and calls `flag.String` / `flag.Bool` / `flag.Parse`, instead of using `github.com/spf13/cobra` (with its `pflag` library). +**Applies when**: any non-test Go file imports the stdlib `flag` package or calls `flag.*` functions (Parse / String / Bool / Int / Float64 / Duration + all *Var variants + Var / Func / NewFlagSet / CommandLine). Scope is intentionally broader than "CLI binaries only" because the Why this rule prevents is **transitive `flag.init()` pollution** — a library calling `flag.String()` in its `init()` adds flags to every binary that imports it, which is the actual failure mode this rule guards against. Test files are exempt. **Enforcement**: `rules/go/cobra-not-stdlib-flag.yml` **Why**: Stdlib `flag` uses a process-global `flag.CommandLine` FlagSet. Any transitive dependency that calls `flag.String(...)` in its `init()` adds flags to this global set — and `github.com/golang/glog` is the most common offender, adding 8+ flags (`-alsologtostderr`, `-log_dir`, `-log_backtrace_at`, `-stderrthreshold`, `-v`, `-vmodule`, …) to every binary that transitively imports it. The result: `my-tool --help` displays a wall of irrelevant glog flags before your three actual flags, and the binary accepts those flags at runtime even though no one wanted them. Cobra uses `pflag` which is isolated from `flag.CommandLine` — the global pollution can't reach it, `--help` shows only your flags, and your flag namespace stays under your control. diff --git a/docs/go-concurrency-patterns.md b/docs/go-concurrency-patterns.md index 7da1a7a..85490d8 100644 --- a/docs/go-concurrency-patterns.md +++ b/docs/go-concurrency-patterns.md @@ -7,7 +7,7 @@ ### RULE go-concurrency/no-raw-go-func (MUST) **Owner**: go-architecture-assistant -**Applies when**: a Go file uses the raw `go func() { ... }()` / `go someMethod(...)` syntax outside `main.go` / top-level entry points — instead of one of the `github.com/bborbe/run` strategies (`CancelOnFirstErrorWait`, `CancelOnFirstFinishWait`, `All`, `Sequential`). +**Applies when**: a Go file uses the raw `go func() { ... }()` / `go someMethod(...)` syntax outside top-level entry points — instead of one of the `github.com/bborbe/run` strategies (`CancelOnFirstErrorWait`, `CancelOnFirstFinishWait`, `All`, `Sequential`). Exempt paths: `main.go` / `cmd/**` (binary entry points where goroutine spawn-and-cancel is the canonical pattern), `pkg/cli/**` (where the canonical `Execute()` signal-listener uses `go func() { <-sigCh; cancel() }()` per the `go-cli-guide.md` pattern), and `*_test.go` / `vendor/` / `mocks/`. **Enforcement**: `rules/go/no-raw-go-func.yml` **Why**: Raw goroutines have three failure modes the `run` package solves: (1) they leak when the parent context is cancelled but the goroutine doesn't observe it; (2) they race when the parent function returns before the goroutine writes its result; (3) error propagation requires hand-rolled channels + `sync.WaitGroup` that drift toward subtle deadlocks. `run.CancelOnFirstErrorWait` wires context cancellation, error aggregation, and synchronization in one call — every consumer learns the same primitives, refactors stay safe, and goroutine lifetimes are explicit at the type signature. diff --git a/rules/go/cobra-not-stdlib-flag.yml b/rules/go/cobra-not-stdlib-flag.yml index 3f3606a..6084fa5 100644 --- a/rules/go/cobra-not-stdlib-flag.yml +++ b/rules/go/cobra-not-stdlib-flag.yml @@ -39,6 +39,7 @@ rule: - pattern: 'flag.Float64Var($$$ARGS)' - pattern: 'flag.DurationVar($$$ARGS)' - pattern: 'flag.Var($$$ARGS)' + - pattern: 'flag.Func($$$ARGS)' - pattern: 'flag.NewFlagSet($$$ARGS)' - pattern: 'flag.CommandLine' ignores: diff --git a/rules/go/no-raw-go-func.yml b/rules/go/no-raw-go-func.yml index 0494ff5..649af95 100644 --- a/rules/go/no-raw-go-func.yml +++ b/rules/go/no-raw-go-func.yml @@ -18,6 +18,13 @@ ignores: - "**/main.go" - "cmd/**" - "**/cmd/**" + # pkg/cli/** exempted because the canonical Execute() shape in + # go-cli-guide.md uses 'go func() { <-sigCh; cancel() }()' for + # signal-listener wiring — that's the documented correct pattern + # for that location. Subcommand RunE goroutines that don't fit + # run.* primitives are rare enough that the false-negative cost + # is lower than the false-positive cost of catching every + # signal-listener bootstrap. - "pkg/cli/**" - "**/pkg/cli/**" - "**/*_test.go" diff --git a/rules/index.json b/rules/index.json index 3908fbb..2746d01 100644 --- a/rules/index.json +++ b/rules/index.json @@ -190,7 +190,7 @@ }, { "anchor": "go-cli/cobra-not-stdlib-flag", - "applies_when": "a Go CLI binary's `main.go` / `pkg/cli/...` imports `flag` (stdlib) and calls `flag.String` / `flag.Bool` / `flag.Parse`, instead of using `github.com/spf13/cobra` (with its `pflag` library).", + "applies_when": "any non-test Go file imports the stdlib `flag` package or calls `flag.*` functions (Parse / String / Bool / Int / Float64 / Duration + all *Var variants + Var / Func / NewFlagSet / CommandLine). Scope is intentionally broader than \"CLI binaries only\" because the Why this rule prevents is **transitive `flag.init()` pollution** — a library calling `flag.String()` in its `init()` adds flags to every binary that imports it, which is the actual failure mode this rule guards against. Test files are exempt.", "doc_path": "docs/go-cli-guide.md", "enforcement": "`rules/go/cobra-not-stdlib-flag.yml`", "id": "go-cli/cobra-not-stdlib-flag", @@ -235,7 +235,7 @@ }, { "anchor": "go-concurrency/no-raw-go-func", - "applies_when": "a Go file uses the raw `go func() { ... }()` / `go someMethod(...)` syntax outside `main.go` / top-level entry points — instead of one of the `github.com/bborbe/run` strategies (`CancelOnFirstErrorWait`, `CancelOnFirstFinishWait`, `All`, `Sequential`).", + "applies_when": "a Go file uses the raw `go func() { ... }()` / `go someMethod(...)` syntax outside top-level entry points — instead of one of the `github.com/bborbe/run` strategies (`CancelOnFirstErrorWait`, `CancelOnFirstFinishWait`, `All`, `Sequential`). Exempt paths: `main.go` / `cmd/**` (binary entry points where goroutine spawn-and-cancel is the canonical pattern), `pkg/cli/**` (where the canonical `Execute()` signal-listener uses `go func() { <-sigCh; cancel() }()` per the `go-cli-guide.md` pattern), and `*_test.go` / `vendor/` / `mocks/`.", "doc_path": "docs/go-concurrency-patterns.md", "enforcement": "`rules/go/no-raw-go-func.yml`", "id": "go-concurrency/no-raw-go-func",