From 09eb37ceb34e72398dd6cb2b697da434159254a3 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 17:05:08 +0200 Subject: [PATCH 1/2] feat(rules): bootstrap 3 Go doc families (mod-replace, glog, concurrency) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third multi-doc batch shape (after PRs #17, #18, #21). 3 small Go docs (84-148 lines each), 4 rules. Bot review on one cycle. Rules added (rules/index.json: 80 -> 84): go-mod-replace/* (owner: go-quality-assistant) - no-cross-repo-replace (MUST) — replace directives must not point outside the current repo's working tree. Off-repo replaces only work on the author's machine; CI breaks; module graph becomes non-reproducible. Same-repo monorepo replaces ARE correct and remain exempt. go-glog/* (owner: go-quality-assistant) - use-v-for-debug-not-info (MUST) — V0 (bare glog.Info) is the always-on production level; debug/trace/internal-state logs use glog.V(1+). Inverting the levels produces gigabytes of noise in production and requires a deploy to change verbosity for troubleshooting. (Rule ID is all-lowercase per build-index.py regex '[a-z0-9-]+/[a-z0-9-]+' — initial attempt with capital V was rejected; walker caught it before push.) go-concurrency/* (owner: go-architecture-assistant) - no-raw-go-func (MUST) — never use 'go func()' outside main.go 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. - channel-closed-by-sender-only (MUST) — only the producer closes a channel. Closing from the receiver side panics on still-pending sends; receivers use 'for v := range ch' or comma-ok idiom. CLAUDE.md doc-agent table updated. Generic examples throughout. No personal vault paths, no trading- domain terms. Pre-emptive grep clean. make build-index regenerated; check-index passes. --- CLAUDE.md | 3 ++ docs/go-concurrency-patterns.md | 74 +++++++++++++++++++++++++++++++-- docs/go-glog-guide.md | 29 +++++++++++++ docs/go-mod-replace-guide.md | 31 ++++++++++++++ rules/index.json | 36 ++++++++++++++++ 5 files changed, 170 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4a0529e..ac134b5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,6 +45,9 @@ Each enforceable guide in `docs/` should have a matching agent in `agents/`. The | `python-pydantic-guide.md` | `python-quality-assistant` | | `python-project-structure.md` | `python-architecture-assistant` | | `python-makefile-commands.md` | `python-quality-assistant` | +| `go-mod-replace-guide.md` | `go-quality-assistant` | +| `go-glog-guide.md` | `go-quality-assistant` | +| `go-concurrency-patterns.md` | `go-architecture-assistant` | Reference-only docs (patterns, setup guides) don't need agents. diff --git a/docs/go-concurrency-patterns.md b/docs/go-concurrency-patterns.md index 7863f63..086d9a3 100644 --- a/docs/go-concurrency-patterns.md +++ b/docs/go-concurrency-patterns.md @@ -4,14 +4,39 @@ ## Core Rule: `go func()` Is a Smell -Raw goroutines leak, race, and are hard to test. Use `run.CancelOnFirstErrorWait` instead. +### 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`). +**Enforcement**: judgment (ast-grep partial: `go_statement` outside `main.go` / `cmd/**`. Test files exempt; `main` entry-point goroutine spawners exempt by path filter) +**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 ```go -// Bad — goroutine leaks on error +// Goroutine leaks on error — no cancellation, no wait, no error propagation go func() { results <- doWork(ctx) }() -// Good +// And worse — multiple raw goroutines with hand-rolled sync.WaitGroup +var wg sync.WaitGroup +for _, item := range items { + wg.Add(1) + go func(it Item) { + defer wg.Done() + _ = process(ctx, it) // error swallowed + }(item) +} +wg.Wait() +``` + +#### Good + +```go +// run.CancelOnFirstErrorWait — context cancellation, error propagation, deterministic wait return run.CancelOnFirstErrorWait(ctx, producer, consumer) + +// For parallel processing of a slice, use run.All +return run.All(ctx, fns...) ``` ## `run` Package @@ -81,6 +106,49 @@ func process(ctx context.Context) error { ## Channel Ownership +### RULE go-concurrency/channel-closed-by-sender-only (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: a Go file calls `close(ch)` on a channel that was passed in as a function parameter from elsewhere — i.e. closed by a consumer/receiver rather than by the goroutine that produces values into it. +**Enforcement**: judgment (ast-grep partial: `close(X)` where `X` is a parameter type `chan T` or `chan<- T`; the agent rules in whether the function is the producer or consumer based on whether it sends into `X`) +**Why**: Closing a channel from the receiver side is a textbook race — the sender may still be writing when the close happens, producing `send on closed channel` panic. The Go convention is: **the producer owns the channel and is the only one allowed to close it.** Receivers learn of "no more values" via `for v := range ch` or the `comma-ok` idiom (`v, ok := <-ch`), never by closing themselves. Multi-producer cases use `sync.WaitGroup` + a single dedicated closer goroutine, not concurrent closes (which also panic). + +#### Bad + +```go +// Consumer closes the channel — race against the producer's still-pending send +func consume(items <-chan Item) { + for item := range items { + process(item) + } + close(items) // ← wrong direction; would also be a compile error on `<-chan` +} +``` + +#### Good + +```go +// Producer closes; consumer ranges and exits naturally when the channel closes +func produce(ctx context.Context, out chan<- Item) error { + defer close(out) // ← producer owns the close + for _, raw := range source { + select { + case <-ctx.Done(): + return ctx.Err() + case out <- transform(raw): + } + } + return nil +} + +func consume(in <-chan Item) { + for item := range in { + process(item) + } +} +``` + + **Caller creates and owns the channel. Pass `chan<- T` into producer.** ```go diff --git a/docs/go-glog-guide.md b/docs/go-glog-guide.md index 0cfe091..cd6629a 100644 --- a/docs/go-glog-guide.md +++ b/docs/go-glog-guide.md @@ -36,6 +36,35 @@ glog.Warningf("High memory usage detected: %d%%", memoryPercent) ## Info V0 - Production Default / System Operator Information +### RULE go-glog/use-v-for-debug-not-info (MUST) + +**Owner**: go-quality-assistant +**Applies when**: a Go file calls `glog.Info` / `glog.Infof` for debug, trace, internal-state, or developer-troubleshooting log lines that should be filtered out in production — instead of `glog.V(N).Info...` / `glog.V(N).Infof...` with an appropriate verbosity level (1 for sysop debug, 2 for dev default, 3+ for deep debug). +**Enforcement**: judgment (ast-grep partial: `call_expression` matching `glog.Info(...)` / `glog.Infof(...)` outside startup/shutdown contexts; the agent rules in "is this production-default-worthy?" based on the log content) +**Why**: V0 (bare `glog.Info`) is the *always-on* production logging level. Every line at V0 ships to log aggregators, gets indexed, and burns CPU on every request whether anyone reads it or not. Using V0 for debug-style logs ("Cache miss for key %s", "Internal state: %+v") produces gigabytes of noise in production, drowns operators in irrelevant detail, and inflates log-storage cost. The `glog.V(N)` mechanism exists exactly to gate verbosity at runtime: V0 stays operator-readable (startup, shutdown, health changes), V1+ stays off by default and turns on for troubleshooting. Inverting the levels means production logs are useless and dev debugging requires a deploy. + +#### Bad + +```go +// V0 used for debug-style content — runs in production, drowns operators +glog.Infof("Cache miss for key %s, fetching from database", key) +glog.Infof("Processing batch of %d items", len(batch)) +glog.Infof("Internal state: %+v", internalStruct) +``` + +#### Good + +```go +// V0 reserved for production-default operator info +glog.Info("Service started successfully on port 8080") +glog.Infof("Recovered from panic: %v", recovered) + +// Debug-style content gated behind V(N) +glog.V(2).Infof("Cache miss for key %s, fetching from database", key) +glog.V(3).Infof("Processing batch of %d items", len(batch)) +glog.V(4).Infof("Internal state: %+v", internalStruct) +``` + This is the default production logging level. Only log information that IT operators need to see: - Application startup/shutdown events diff --git a/docs/go-mod-replace-guide.md b/docs/go-mod-replace-guide.md index 6236864..d2f352d 100644 --- a/docs/go-mod-replace-guide.md +++ b/docs/go-mod-replace-guide.md @@ -4,6 +4,37 @@ When to use `replace` in `go.mod`, and when not to. ## Rule +### RULE go-mod-replace/no-cross-repo-replace (MUST) + +**Owner**: go-quality-assistant +**Applies when**: a Go project's `go.mod` contains a `replace` directive whose right-hand side path points outside the current repo's working tree — relative paths that escape the repo root (`../../other-repo/lib`), absolute filesystem paths (`/Users//work/...`), or any other off-repo location. +**Enforcement**: judgment (ast-grep partial: pattern over `go.mod` `replace` lines with right-hand side starting with `../` reaching above the repo root or starting with `/`. Same-repo `replace github.com/acme/monorepo/libs/shared => ../../libs/shared` is fine — the rule fires only when the destination escapes the repo) +**Why**: An off-repo replace points to a path that exists only on the author's machine. Anyone else cloning the repo gets a broken build; CI can't resolve it; the module graph becomes non-reproducible. Worse, it hides the fact that consumers of your module require an unreleased change — the dependency looks fine locally but breaks the moment someone else pulls it. Within a monorepo, relative-path replaces ARE correct (every clone has the sibling module at the same path) — that's the same-repo exception. The rule fires only on the cross-repo escape. + +#### Bad + +```go +// consumer/go.mod +replace github.com/acme/producer/lib => ../../producer/lib +// Builds locally, breaks for everyone else. CI fails: path doesn't exist in their checkout. + +// Or worse: absolute path tied to one machine +replace github.com/acme/producer/lib => /Users/alice/work/producer/lib +``` + +#### Good + +```go +// consumer/go.mod — pin to a tagged or pseudo-version +require github.com/acme/producer/lib v0.0.0-20260403114524-913de8870914 +// No replace. Reproducible for everyone. + +// monorepo/services/api/go.mod — same-repo replace IS correct +replace github.com/acme/monorepo/libs/shared => ../../libs/shared +``` + +### Quick reference + - **Replace inside a single repo** → **OK**. Multi-module repos (monorepos with several `go.mod`) use relative-path replaces so sibling modules resolve to the working tree, not a released version. - **Replace across repo boundaries** → **NO**. Never use a `replace` that points outside the current repo's checkout. Consume cross-repo modules as released pseudo-versions or tagged releases. diff --git a/rules/index.json b/rules/index.json index 923d51d..0b5ddc1 100644 --- a/rules/index.json +++ b/rules/index.json @@ -116,6 +116,24 @@ "level": "SHOULD", "owner": "go-architecture-assistant" }, + { + "anchor": "go-concurrency/channel-closed-by-sender-only", + "applies_when": "a Go file calls `close(ch)` on a channel that was passed in as a function parameter from elsewhere — i.e. closed by a consumer/receiver rather than by the goroutine that produces values into it.", + "doc_path": "docs/go-concurrency-patterns.md", + "enforcement": "judgment (ast-grep partial: `close(X)` where `X` is a parameter type `chan T` or `chan<- T`; the agent rules in whether the function is the producer or consumer based on whether it sends into `X`)", + "id": "go-concurrency/channel-closed-by-sender-only", + "level": "MUST", + "owner": "go-architecture-assistant" + }, + { + "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 partial: `go_statement` outside `main.go` / `cmd/**`. Test files exempt; `main` entry-point goroutine spawners exempt by path filter)", + "id": "go-concurrency/no-raw-go-func", + "level": "MUST", + "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/`.", @@ -260,6 +278,15 @@ "level": "SHOULD", "owner": "go-quality-assistant" }, + { + "anchor": "go-glog/use-v-for-debug-not-info", + "applies_when": "a Go file calls `glog.Info` / `glog.Infof` for debug, trace, internal-state, or developer-troubleshooting log lines that should be filtered out in production — instead of `glog.V(N).Info...` / `glog.V(N).Infof...` with an appropriate verbosity level (1 for sysop debug, 2 for dev default, 3+ for deep debug).", + "doc_path": "docs/go-glog-guide.md", + "enforcement": "judgment (ast-grep partial: `call_expression` matching `glog.Info(...)` / `glog.Infof(...)` outside startup/shutdown contexts; the agent rules in \"is this production-default-worthy?\" based on the log content)", + "id": "go-glog/use-v-for-debug-not-info", + "level": "MUST", + "owner": "go-quality-assistant" + }, { "anchor": "go-http-handler/kebab-case-handler-files", "applies_when": "a `*.go` file under `**/pkg/handler/**` is named with non-kebab-case style (e.g. `exists_handler.go`, `existshandler.go`, or `handler.go`) instead of the documented `-.go` kebab-case (e.g. `exists.go`, `forward-invoice.go`). Pure ast-grep operates on file contents, not filenames — this is a filesystem convention check.", @@ -386,6 +413,15 @@ "level": "MUST", "owner": "go-quality-assistant" }, + { + "anchor": "go-mod-replace/no-cross-repo-replace", + "applies_when": "a Go project's `go.mod` contains a `replace` directive whose right-hand side path points outside the current repo's working tree — relative paths that escape the repo root (`../../other-repo/lib`), absolute filesystem paths (`/Users//work/...`), or any other off-repo location.", + "doc_path": "docs/go-mod-replace-guide.md", + "enforcement": "judgment (ast-grep partial: pattern over `go.mod` `replace` lines with right-hand side starting with `../` reaching above the repo root or starting with `/`. Same-repo `replace github.com/acme/monorepo/libs/shared => ../../libs/shared` is fine — the rule fires only when the destination escapes the repo)", + "id": "go-mod-replace/no-cross-repo-replace", + "level": "MUST", + "owner": "go-quality-assistant" + }, { "anchor": "go-prometheus/composed-metrics-interface", "applies_when": "a single `Metrics` interface aggregates methods spanning two or more distinct functional domains (handlers + senders + schedulers + …), forcing consumers to depend on methods they don't use.", From 1e89c078614016a40c007afd534db877a6f54d7a Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 17:15:37 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(rules):=20address=20PR=20#22=20review?= =?UTF-8?q?=20=E2=80=94=2010=20MAJOR=20+=201=20MINOR=20+=202=20NIT?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot's deep review caught 10 real issues across 3 docs + agent file + README/llms.txt + index.json. MAJOR (all fixed): 1. go-glog rule placement: was nested under '## Info V0' which made the scope look V0-only. Moved to top-level (before all '## Error/Warning/V0..V3+' sections) so the cross-V-level scope is obvious. Tightened Applies-when to explicitly state 'across ALL V levels' and removed the duplicate copy under Info V0. 2. README.md missing go-glog-guide.md — added entry under Go Code Quality table next to go-logging-guide.md. 3. llms.txt missing go-glog-guide.md — added entry next to logging. 4. go-concurrency orphan rule 6 'Capture loop variables by value: x := x' in the Rules numbered list. Outdated for Go 1.22+ which fixed loop-variable capture semantics (per-iteration scoping). Removed from the list and added a note explaining the Go 1.22+ fix + when x := x is still needed (pre-1.22 projects). 5. go-concurrency missing '## Antipatterns' section — added one cross-referencing the canonical #### Bad blocks under each ### RULE, plus 4 summary bullets covering raw go-func / consumer-close / unbounded-with-defer-close / hidden-Results-channel. 6-9. All 4 new rules' Enforcement field said 'ast-grep partial' but no YAML files exist in rules/go/. Reworded to 'judgment (ast-grep follow-up: ...)' matching the established phrasing across the repo's other judgment-tier rules. The pattern stays the same: the agent enforces judgment now, ast-grep YAML lands in a focused follow-up PR. 10. agents/go-quality-assistant.md duplicated the 6-line glog V-level table verbatim from go-glog-guide.md. Per CLAUDE.md 'rules live in docs, not duplicated in agents', replaced with a one-line reference pointing at the doc + the canonical rule ID. Future drift impossible. MINOR (deferred, pre-existing): V(2) is described as 'Developer Default' in go-glog-guide.md but as 'heartbeat (use sampling!)' in go-logging-guide.md. Inter-doc inconsistency that predates this PR; should be resolved in a focused follow-up that decides the canonical semantic for V(2). Out of scope for this PR's rule blocks. NITs skipped: prose duplication in go-mod-replace (RULE block + 'Why' section overlap is intentional — they serve different readers); blank- line cosmetic at concurrency line 150. make build-index regenerated; check-index passes. --- README.md | 1 + agents/go-quality-assistant.md | 8 +---- docs/go-concurrency-patterns.md | 22 +++++++++---- docs/go-glog-guide.md | 58 ++++++++++++++++----------------- docs/go-mod-replace-guide.md | 2 +- llms.txt | 1 + rules/index.json | 10 +++--- 7 files changed, 54 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index e8c3aec..ce59dcd 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,7 @@ All guides live in [`docs/`](docs/) and can be read standalone without the plugi | [Time Injection](docs/go-time-injection.md) | bborbe/time, CurrentDateTimeGetter | | [GoDoc](docs/go-doc-best-practices.md) | Documentation standards | | [Logging](docs/go-logging-guide.md) | Structured logging | +| [glog Levels](docs/go-glog-guide.md) | glog verbosity-level discipline (legacy projects) | | [Design Patterns](docs/go-patterns.md) | Common Go patterns | ### Go — Testing diff --git a/agents/go-quality-assistant.md b/agents/go-quality-assistant.md index 252e134..9994ea5 100644 --- a/agents/go-quality-assistant.md +++ b/agents/go-quality-assistant.md @@ -126,13 +126,7 @@ Code review categories: - WaitGroup patterns - Infinite loops must handle context cancellation -**Logging (glog)**: -- `glog.Error()` - genuine errors requiring immediate attention -- `glog.Warning()` - unexpected conditions that don't break functionality -- `glog.V(0).Info()` - production default, operator info only -- `glog.V(1).Info()` - operator debug, production troubleshooting -- `glog.V(2).Info()` - external communication, developer default -- `glog.V(3+).Info()` - developer debug, deep troubleshooting +**Logging (glog)**: See `docs/go-glog-guide.md` for level semantics. Canonical rule: `go-glog/use-v-for-debug-not-info` (V0 is production-default only; debug-shaped logs use `V(N)` with N≥1). Do not duplicate the level table here — the doc is the source of truth. **External-call logging (boundary rule)**: - Every call crossing the process boundary (HTTP, gRPC, DB, message bus, subprocess) MUST emit a log line on response. Without it, runtime debugging — "did the call go out? what was the status? did the response match what we expected?" — becomes guesswork. diff --git a/docs/go-concurrency-patterns.md b/docs/go-concurrency-patterns.md index 086d9a3..c46ff58 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 partial: `go_statement` outside `main.go` / `cmd/**`. Test files exempt; `main` entry-point goroutine spawners exempt by path filter) +**Enforcement**: judgment (ast-grep follow-up: `go_statement` outside `main.go` / `cmd/**`. Test files exempt; `main` entry-point goroutine spawners exempt by path filter) **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 @@ -110,7 +110,7 @@ func process(ctx context.Context) error { **Owner**: go-architecture-assistant **Applies when**: a Go file calls `close(ch)` on a channel that was passed in as a function parameter from elsewhere — i.e. closed by a consumer/receiver rather than by the goroutine that produces values into it. -**Enforcement**: judgment (ast-grep partial: `close(X)` where `X` is a parameter type `chan T` or `chan<- T`; the agent rules in whether the function is the producer or consumer based on whether it sends into `X`) +**Enforcement**: judgment (ast-grep follow-up: `close(X)` where `X` is a parameter type `chan T` or `chan<- T`; the agent rules in whether the function is the producer or consumer based on whether it sends into `X`) **Why**: Closing a channel from the receiver side is a textbook race — the sender may still be writing when the close happens, producing `send on closed channel` panic. The Go convention is: **the producer owns the channel and is the only one allowed to close it.** Receivers learn of "no more values" via `for v := range ch` or the `comma-ok` idiom (`v, ok := <-ch`), never by closing themselves. Multi-producer cases use `sync.WaitGroup` + a single dedicated closer goroutine, not concurrent closes (which also panic). #### Bad @@ -208,9 +208,19 @@ if errors.Is(err, errReachedUntil) { return nil } ## Rules -1. Never `go func()` — use `run.CancelOnFirstErrorWait` +1. Never `go func()` — use `run.CancelOnFirstErrorWait` (canonicalised as `go-concurrency/no-raw-go-func`) 2. Caller owns the channel, passes `chan<- T` as parameter -3. Producer closes bounded channels (`defer close(ch)`) -4. Always check `ctx.Done()` in consumer loops +3. Producer closes bounded channels (`defer close(ch)`) (canonicalised as `go-concurrency/channel-closed-by-sender-only`) +4. Always check `ctx.Done()` in consumer loops (cross-references `go-context/cancel-check-in-loop`) 5. Never close channel in unbounded/polling producers -6. Capture loop variables by value: `x := x` + +> Note: an earlier revision listed a sixth rule "Capture loop variables by value (`x := x`)". That rule was removed because Go 1.22+ fixed loop-variable capture semantics — the variable is now scoped per iteration. For pre-1.22 projects, the `x := x` workaround is still required; this guide assumes Go 1.22+. + +## Antipatterns + +See the `#### Bad` blocks under each `### RULE` above for the canonical antipattern shapes. Summary: + +- **Raw `go func()`** — `go-concurrency/no-raw-go-func` (leaks, races, hand-rolled `sync.WaitGroup` deadlocks). +- **Consumer closes channel** — `go-concurrency/channel-closed-by-sender-only` (panics on still-pending sends). +- **Unbounded producer with `defer close(ch)`** — bounded-shape primitive misapplied; the polling producer's `defer close` fires when the function returns on ctx-cancel, but the consumer goroutine may still be reading and panic on subsequent sends from anywhere else in the codebase. +- **Hidden `Results()` channel-returning method** — anti-pattern shown in Channel Ownership above; pass `chan<- T` as a parameter instead so ownership is explicit at the call site. diff --git a/docs/go-glog-guide.md b/docs/go-glog-guide.md index cd6629a..717c7da 100644 --- a/docs/go-glog-guide.md +++ b/docs/go-glog-guide.md @@ -4,6 +4,35 @@ Google's [glog](https://github.com/golang/glog) provides more granular logging levels than standard Go logging. This guide explains when to use each level for consistent logging across Go services. +### RULE go-glog/use-v-for-debug-not-info (MUST) + +**Owner**: go-quality-assistant +**Applies when**: a Go file calls `glog.Info` / `glog.Infof` for debug, trace, internal-state, or developer-troubleshooting log lines that should be filtered out in production — instead of `glog.V(N).Info...` / `glog.V(N).Infof...` with an appropriate verbosity level (1 for sysop debug, 2 for dev default, 3+ for deep debug). Scope: applies across ALL V levels — V0 is reserved for production-default operator info; everything debug-shaped goes through `V(N)`. +**Enforcement**: judgment (ast-grep follow-up: `call_expression` matching `glog.Info(...)` / `glog.Infof(...)` outside startup/shutdown contexts; the agent rules in "is this production-default-worthy?" based on the log content) +**Why**: V0 (bare `glog.Info`) is the *always-on* production logging level. Every line at V0 ships to log aggregators, gets indexed, and burns CPU on every request whether anyone reads it or not. Using V0 for debug-style logs ("Cache miss for key %s", "Internal state: %+v") produces gigabytes of noise in production, drowns operators in irrelevant detail, and inflates log-storage cost. The `glog.V(N)` mechanism exists exactly to gate verbosity at runtime: V0 stays operator-readable (startup, shutdown, health changes), V1+ stays off by default and turns on for troubleshooting. Inverting the levels means production logs are useless and dev debugging requires a deploy. + +#### Bad + +```go +// V0 used for debug-style content — runs in production, drowns operators +glog.Infof("Cache miss for key %s, fetching from database", key) +glog.Infof("Processing batch of %d items", len(batch)) +glog.Infof("Internal state: %+v", internalStruct) +``` + +#### Good + +```go +// V0 reserved for production-default operator info +glog.Info("Service started successfully on port 8080") +glog.Infof("Recovered from panic: %v", recovered) + +// Debug-style content gated behind V(N) +glog.V(2).Infof("Cache miss for key %s, fetching from database", key) +glog.V(3).Infof("Processing batch of %d items", len(batch)) +glog.V(4).Infof("Internal state: %+v", internalStruct) +``` + ## Error - Always an Error / Requires System Operator Action Use for genuine errors that require immediate attention: @@ -36,35 +65,6 @@ glog.Warningf("High memory usage detected: %d%%", memoryPercent) ## Info V0 - Production Default / System Operator Information -### RULE go-glog/use-v-for-debug-not-info (MUST) - -**Owner**: go-quality-assistant -**Applies when**: a Go file calls `glog.Info` / `glog.Infof` for debug, trace, internal-state, or developer-troubleshooting log lines that should be filtered out in production — instead of `glog.V(N).Info...` / `glog.V(N).Infof...` with an appropriate verbosity level (1 for sysop debug, 2 for dev default, 3+ for deep debug). -**Enforcement**: judgment (ast-grep partial: `call_expression` matching `glog.Info(...)` / `glog.Infof(...)` outside startup/shutdown contexts; the agent rules in "is this production-default-worthy?" based on the log content) -**Why**: V0 (bare `glog.Info`) is the *always-on* production logging level. Every line at V0 ships to log aggregators, gets indexed, and burns CPU on every request whether anyone reads it or not. Using V0 for debug-style logs ("Cache miss for key %s", "Internal state: %+v") produces gigabytes of noise in production, drowns operators in irrelevant detail, and inflates log-storage cost. The `glog.V(N)` mechanism exists exactly to gate verbosity at runtime: V0 stays operator-readable (startup, shutdown, health changes), V1+ stays off by default and turns on for troubleshooting. Inverting the levels means production logs are useless and dev debugging requires a deploy. - -#### Bad - -```go -// V0 used for debug-style content — runs in production, drowns operators -glog.Infof("Cache miss for key %s, fetching from database", key) -glog.Infof("Processing batch of %d items", len(batch)) -glog.Infof("Internal state: %+v", internalStruct) -``` - -#### Good - -```go -// V0 reserved for production-default operator info -glog.Info("Service started successfully on port 8080") -glog.Infof("Recovered from panic: %v", recovered) - -// Debug-style content gated behind V(N) -glog.V(2).Infof("Cache miss for key %s, fetching from database", key) -glog.V(3).Infof("Processing batch of %d items", len(batch)) -glog.V(4).Infof("Internal state: %+v", internalStruct) -``` - This is the default production logging level. Only log information that IT operators need to see: - Application startup/shutdown events diff --git a/docs/go-mod-replace-guide.md b/docs/go-mod-replace-guide.md index d2f352d..586cb6b 100644 --- a/docs/go-mod-replace-guide.md +++ b/docs/go-mod-replace-guide.md @@ -8,7 +8,7 @@ When to use `replace` in `go.mod`, and when not to. **Owner**: go-quality-assistant **Applies when**: a Go project's `go.mod` contains a `replace` directive whose right-hand side path points outside the current repo's working tree — relative paths that escape the repo root (`../../other-repo/lib`), absolute filesystem paths (`/Users//work/...`), or any other off-repo location. -**Enforcement**: judgment (ast-grep partial: pattern over `go.mod` `replace` lines with right-hand side starting with `../` reaching above the repo root or starting with `/`. Same-repo `replace github.com/acme/monorepo/libs/shared => ../../libs/shared` is fine — the rule fires only when the destination escapes the repo) +**Enforcement**: judgment (ast-grep follow-up: pattern over `go.mod` `replace` lines with right-hand side starting with `../` reaching above the repo root or starting with `/`. Same-repo `replace github.com/acme/monorepo/libs/shared => ../../libs/shared` is fine — the rule fires only when the destination escapes the repo) **Why**: An off-repo replace points to a path that exists only on the author's machine. Anyone else cloning the repo gets a broken build; CI can't resolve it; the module graph becomes non-reproducible. Worse, it hides the fact that consumers of your module require an unreleased change — the dependency looks fine locally but breaks the moment someone else pulls it. Within a monorepo, relative-path replaces ARE correct (every clone has the sibling module at the same path) — that's the same-repo exception. The rule fires only on the cross-repo escape. #### Bad diff --git a/llms.txt b/llms.txt index 540f24d..8183764 100644 --- a/llms.txt +++ b/llms.txt @@ -49,6 +49,7 @@ - [Error Wrapping](docs/go-error-wrapping-guide.md): Context-aware error handling - [Context Cancellation](docs/go-context-cancellation-in-loops.md): Non-blocking select in loops - [Logging](docs/go-logging-guide.md): Structured logging patterns +- [glog Levels](docs/go-glog-guide.md): glog verbosity-level discipline (legacy projects) ## Python - [Project Structure](docs/python-project-structure.md): src/ layout, pyproject.toml, uv diff --git a/rules/index.json b/rules/index.json index 0b5ddc1..df4ad11 100644 --- a/rules/index.json +++ b/rules/index.json @@ -120,7 +120,7 @@ "anchor": "go-concurrency/channel-closed-by-sender-only", "applies_when": "a Go file calls `close(ch)` on a channel that was passed in as a function parameter from elsewhere — i.e. closed by a consumer/receiver rather than by the goroutine that produces values into it.", "doc_path": "docs/go-concurrency-patterns.md", - "enforcement": "judgment (ast-grep partial: `close(X)` where `X` is a parameter type `chan T` or `chan<- T`; the agent rules in whether the function is the producer or consumer based on whether it sends into `X`)", + "enforcement": "judgment (ast-grep follow-up: `close(X)` where `X` is a parameter type `chan T` or `chan<- T`; the agent rules in whether the function is the producer or consumer based on whether it sends into `X`)", "id": "go-concurrency/channel-closed-by-sender-only", "level": "MUST", "owner": "go-architecture-assistant" @@ -129,7 +129,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 partial: `go_statement` outside `main.go` / `cmd/**`. Test files exempt; `main` entry-point goroutine spawners exempt by path filter)", + "enforcement": "judgment (ast-grep follow-up: `go_statement` outside `main.go` / `cmd/**`. Test files exempt; `main` entry-point goroutine spawners exempt by path filter)", "id": "go-concurrency/no-raw-go-func", "level": "MUST", "owner": "go-architecture-assistant" @@ -280,9 +280,9 @@ }, { "anchor": "go-glog/use-v-for-debug-not-info", - "applies_when": "a Go file calls `glog.Info` / `glog.Infof` for debug, trace, internal-state, or developer-troubleshooting log lines that should be filtered out in production — instead of `glog.V(N).Info...` / `glog.V(N).Infof...` with an appropriate verbosity level (1 for sysop debug, 2 for dev default, 3+ for deep debug).", + "applies_when": "a Go file calls `glog.Info` / `glog.Infof` for debug, trace, internal-state, or developer-troubleshooting log lines that should be filtered out in production — instead of `glog.V(N).Info...` / `glog.V(N).Infof...` with an appropriate verbosity level (1 for sysop debug, 2 for dev default, 3+ for deep debug). Scope: applies across ALL V levels — V0 is reserved for production-default operator info; everything debug-shaped goes through `V(N)`.", "doc_path": "docs/go-glog-guide.md", - "enforcement": "judgment (ast-grep partial: `call_expression` matching `glog.Info(...)` / `glog.Infof(...)` outside startup/shutdown contexts; the agent rules in \"is this production-default-worthy?\" based on the log content)", + "enforcement": "judgment (ast-grep follow-up: `call_expression` matching `glog.Info(...)` / `glog.Infof(...)` outside startup/shutdown contexts; the agent rules in \"is this production-default-worthy?\" based on the log content)", "id": "go-glog/use-v-for-debug-not-info", "level": "MUST", "owner": "go-quality-assistant" @@ -417,7 +417,7 @@ "anchor": "go-mod-replace/no-cross-repo-replace", "applies_when": "a Go project's `go.mod` contains a `replace` directive whose right-hand side path points outside the current repo's working tree — relative paths that escape the repo root (`../../other-repo/lib`), absolute filesystem paths (`/Users//work/...`), or any other off-repo location.", "doc_path": "docs/go-mod-replace-guide.md", - "enforcement": "judgment (ast-grep partial: pattern over `go.mod` `replace` lines with right-hand side starting with `../` reaching above the repo root or starting with `/`. Same-repo `replace github.com/acme/monorepo/libs/shared => ../../libs/shared` is fine — the rule fires only when the destination escapes the repo)", + "enforcement": "judgment (ast-grep follow-up: pattern over `go.mod` `replace` lines with right-hand side starting with `../` reaching above the repo root or starting with `/`. Same-repo `replace github.com/acme/monorepo/libs/shared => ../../libs/shared` is fine — the rule fires only when the destination escapes the repo)", "id": "go-mod-replace/no-cross-repo-replace", "level": "MUST", "owner": "go-quality-assistant"