diff --git a/CLAUDE.md b/CLAUDE.md index 59a1598..c132903 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,6 +27,9 @@ Each enforceable guide in `docs/` should have a matching agent in `agents/`. The | `go-prometheus-metrics-guide.md` | `go-metrics-assistant` | | `go-factory-pattern.md` | `go-factory-pattern-assistant` | | `go-http-handler-refactoring-guide.md` | `go-http-handler-assistant` | +| `go-json-error-handler-guide.md` | `go-http-handler-assistant` | +| `go-linting-guide.md` | `go-quality-assistant` | +| `go-state-machine-pattern.md` | `go-architecture-assistant` | | `go-doc-best-practices.md` | `godoc-assistant` | | `go-testing-guide.md` | `go-test-quality-assistant` | | `go-security-linting.md` | `go-security-specialist` | diff --git a/docs/go-json-error-handler-guide.md b/docs/go-json-error-handler-guide.md index f8716d9..0da6901 100644 --- a/docs/go-json-error-handler-guide.md +++ b/docs/go-json-error-handler-guide.md @@ -21,6 +21,42 @@ This guide covers the standardized JSON error handler pattern from `github.com/b ## Error Response Structure +### RULE go-json-error-handler/structured-response-shape (MUST) + +**Owner**: go-http-handler-assistant +**Applies when**: an HTTP handler in a Go service emits an error response that is not a JSON object with the canonical `{error: {code, message, details}}` shape — e.g. plain-text bodies, top-level `{message: ...}` without an `error` wrapper, or `details` as a string instead of a `map[string]string`. +**Enforcement**: judgment (response-shape inspection; ast-grep can detect `http.Error` calls and inline JSON writes but the full contract needs request/response review) +**Why**: Clients deserialise error responses against a stable shape. When some handlers return plain text and others return JSON, every client needs branching parse logic and an "if response.Status >= 400 try-string-then-try-JSON" fallback — exactly the kind of fragility that breaks on the first new error path. The canonical `{error: {code, message, details}}` shape is the lingua franca: `code` for programmatic dispatch, `message` for logging and human readers, `details` (string-map) for structured context (the field that failed, the expected vs. actual value, etc.) without committing to a per-error-type schema. + +#### Bad + +```go +// Plain text body — clients can't dispatch on it +http.Error(w, "columnGroup '' is unknown", http.StatusBadRequest) +``` + +#### Good + +```go +// Canonical JSON shape via libhttp.NewJSONErrorHandler. +// Use WrapWithDetails when adding a structured details map; WrapWithCode is +// for the simpler (err, status, code) shape without details. +return libhttp.WrapWithDetails( + errors.Errorf(ctx, "columnGroup '%s' is unknown", g), + http.StatusBadRequest, + libhttp.ErrorCodeValidation, + map[string]string{ + "field": "columnGroup", + "expected": "day|week|month|year", + }, +) +// Response body: +// { "error": { "code": "VALIDATION_ERROR", +// "message": "columnGroup '' is unknown", +// "details": { "field": "columnGroup", +// "expected": "day|week|month|year" } } } +``` + All JSON errors follow this structure: ```json @@ -44,6 +80,33 @@ All JSON errors follow this structure: ## Standard Error Codes +### RULE go-json-error-handler/use-error-code-constants (MUST) + +**Owner**: go-http-handler-assistant +**Applies when**: a Go HTTP handler passes a raw string literal as the error-code argument to `libhttp.WrapWithCode` / `libhttp.NewJSONError` instead of the `libhttp.ErrorCodeXxx` constants. +**Enforcement**: judgment (ast-grep follow-up: pattern over `libhttp.WrapWithCode($$, $$, $CODE)` / `WrapWithDetails($$, $$, $CODE, $$)` with `$CODE` constrained to be a `interpreted_string_literal` — see PR #11 recipe for the metavariable-constraint shape) +**Why**: Error codes are the dispatch surface clients pattern-match on. A typo in `"VAIDATION_ERROR"` ships silently — the client's `if code == "VALIDATION_ERROR"` branch never fires, the error falls through to the generic handler, and the bug surfaces as "validation errors don't show the inline form-field highlight." Constants make typos fail at compile time, give grep a single source of truth for which codes exist, and let the constant's godoc anchor the HTTP-status / semantic contract per code. + +#### Bad + +```go +return libhttp.WrapWithCode( + errors.Errorf(ctx, "invalid input"), + http.StatusBadRequest, + "VAIDATION_ERROR", // typo — client dispatch silently misses this +) +``` + +#### Good + +```go +return libhttp.WrapWithCode( + errors.Errorf(ctx, "invalid input"), + http.StatusBadRequest, + libhttp.ErrorCodeValidation, // typo fails at compile time +) +``` + | Code | HTTP Status | Usage | |------|-------------|-------| | `VALIDATION_ERROR` | 400 | Invalid request parameters, malformed input | @@ -350,7 +413,7 @@ func TestSearchHandler_ValidationError(t *testing.T) { var errResp libhttp.ErrorResponse json.NewDecoder(resp.Body).Decode(&errResp) - g.Expect(errResp.Error.Code).To(Equal("VALIDATION_ERROR")) + g.Expect(errResp.Error.Code).To(Equal(libhttp.ErrorCodeValidation)) g.Expect(errResp.Error.Details["field"]).To(Equal("q")) } ``` diff --git a/docs/go-linting-guide.md b/docs/go-linting-guide.md index bf12a4e..42a6d15 100644 --- a/docs/go-linting-guide.md +++ b/docs/go-linting-guide.md @@ -1,6 +1,6 @@ # Go Linting Guide -Comprehensive guide for golangci-lint v2 configuration, linter rules, and fix strategies used across all Go projects. The canonical config lives in [go-skeleton/.golangci.yml](https://github.com/bborbe/go-skeleton). +Comprehensive guide for golangci-lint v2 configuration, linter rules, and fix strategies used across all Go projects. The canonical config lives in `templates/.golangci.yml` within this plugin. ## Table of Contents @@ -147,6 +147,36 @@ formatters: ## Complexity Limits +### RULE go-linting/complexity-limits-enforced (MUST) + +**Owner**: go-quality-assistant +**Applies when**: a Go project's `.golangci.yaml` (or `.golangci.yml`) does not enable `funlen`, `gocognit`, `nestif`, and `maintidx` with the bborbe standard thresholds (80 lines / 50 statements / 20 complexity / 4 nesting / 20 maintainability). +**Enforcement**: judgment (config-file presence + threshold inspection; ast-grep does not parse YAML reliably for nested numeric thresholds) +**Why**: Complexity is the silent killer of long-lived services. Without enforced caps, functions grow until they're untestable, cognitive load makes refactors fragile, and naive line-count-driven extractions (the kind `go-architecture-assistant` flags) appear only at the end of long change-lists. Catching this at lint time forces incremental hygiene — the diff stays bounded, the reviewer's attention stays on the change instead of the bloat. + +#### Bad + +```yaml +# .golangci.yaml — no complexity gate +linters: + enable: + - revive + - errcheck +``` + +#### Good + +```yaml +version: "2" +linters: + enable: [funlen, gocognit, nestif, maintidx, revive, errcheck] + settings: + funlen: { lines: 80, statements: 50 } + gocognit: { min-complexity: 20 } + nestif: { min-complexity: 4 } + maintidx: { min-maintainability-index: 20 } +``` + | Linter | Limit | Description | |--------|-------|-------------| | `funlen` | 80 lines / 50 statements | Max function length | @@ -210,6 +240,46 @@ func (s *Service) processItem(ctx context.Context, item Item) error { ## Banned Packages (depguard) +### RULE go-linting/banned-packages-via-depguard (MUST) + +**Owner**: go-quality-assistant +**Applies when**: a Go project's `.golangci.yaml` does not configure `depguard` with the bborbe banned-package list (deprecated stdlib alternatives like `io/ioutil`, v1 of versioned packages where v2+ exists, `pkg/errors` superseded by `bborbe/errors`). +**Enforcement**: judgment (config inspection; ast-grep does not parse golangci YAML structure) +**Why**: Banned-package lists encode hard-won lessons that would otherwise re-surface in every PR review (`fmt.Errorf` vs `errors.Wrap` is its own rule via `go-errors/no-fmt-errorf`; here we close the depguard-level gap on full-package bans). Without depguard, deprecated imports silently land via "auto-import" tools and the codebase fragments — half the files use `io/ioutil`, half use `io+os`, refactors trip over both. Catching the import at lint time forces the right choice before the deprecated path takes root. + +#### Bad + +```yaml +# .golangci.yaml — depguard not configured +linters: + enable: + - revive + - errcheck +``` + +#### Good + +```yaml +version: "2" +linters: + enable: [depguard, revive, errcheck] + settings: + depguard: + rules: + Main: + deny: + - pkg: "github.com/pkg/errors" + desc: "use github.com/bborbe/errors" + - pkg: "github.com/bborbe/argument" + desc: "use bborbe/argument/v2" + - pkg: "golang.org/x/net/context" + desc: "use stdlib context" + - pkg: "io/ioutil" + desc: "deprecated; use io + os" + - pkg: "golang.org/x/lint/golint" + desc: "use revive or staticcheck" +``` + | Banned Package | Use Instead | Reason | |---------------|-------------|--------| | `github.com/pkg/errors` | `github.com/bborbe/errors` | Context-aware error wrapping | diff --git a/docs/go-state-machine-pattern.md b/docs/go-state-machine-pattern.md index b699a6e..918f9c2 100644 --- a/docs/go-state-machine-pattern.md +++ b/docs/go-state-machine-pattern.md @@ -132,6 +132,53 @@ func runShippingPhase(ctx context.Context) (*Result, error) { ## Status vs. Phase — the critical distinction +### RULE go-state-machine/status-phase-separation (MUST) + +**Owner**: go-architecture-assistant +**Applies when**: a Go workflow / FSM controller persists a single field that conflates "where in the workflow" with "how the last invocation ended" — e.g. a `state` enum mixing `validating`/`charging`/`done`/`failed` together, or marking a task `completed` while `NextPhase` is still non-terminal. +**Enforcement**: judgment (semantic — requires reading the controller's persist logic) +**Why**: Status and phase are independent dimensions. Phase = WHERE; Status = HOW the last run ended. Collapsing them into one field produces the textbook workflow stall: the controller marks the task `completed` after the first phase, stops spawning, and the workflow halts with no error and no signal. The two-field shape makes the controller's transition logic explicit and the persisted state legible: a paused human-review task reads as `(phase=human_review, status=in_progress)` rather than the ambiguous `state=human_review` (does that mean blocked? completed? failed?). + +#### Bad + +```go +// Single conflated field — the workflow stalls when the worker emits 'done' +// for the first phase, even though NextPhase points at the next phase. +type Task struct { + State string // "validating" / "charging" / "done" / "failed" +} + +func persist(task *Task, result *Result) { + if result.Status == "done" { + task.State = "done" // wrong — should advance to NextPhase + } +} +``` + +#### Good + +```go +type Task struct { + Phase string // WHERE: validating / charging / shipping / done + Status string // HOW the last invocation ended: in_progress / completed / failed +} + +func persist(task *Task, result *Result) { + switch { + case result.Status == "done" && (result.NextPhase == "" || result.NextPhase == PhaseDone): + task.Status = "completed" // workflow terminates + case result.Status == "done": + task.Phase = result.NextPhase + task.Status = "in_progress" // advance to next phase + case result.Status == "needs_input": + task.Phase = "human_review" + task.Status = "in_progress" + case result.Status == "failed": + task.Status = "failed" + } +} +``` + These are **two independent dimensions**: - **Phase** = WHERE in the workflow we are @@ -213,6 +260,47 @@ Typical controller loop: ## Loops and backward edges +### RULE go-state-machine/forward-only-by-default (SHOULD) + +**Owner**: go-architecture-assistant +**Applies when**: a Go FSM worker emits `NextPhase` referring to an earlier phase in the workflow's declared order, without an explicit circuit-breaker `attempt` counter and a controller-side cap. +**Enforcement**: judgment (semantic — requires reading both the worker's NextPhase-emission logic and the controller's persist logic) +**Why**: Backward edges are how long-running agents accidentally burn unbounded budgets. A re-planning loop that emits `NextPhase=draft` from `review` looks reasonable in isolation; in production it spins for hours until someone notices. Forward-only-by-default makes the rare backward edge the conspicuous choice — when it's needed (re-planning, retries, operator-driven requeues), the worker explicitly tracks an `attempts` counter and the controller fails the workflow when it exceeds the cap. Four legitimate patterns: interventional reset (operator flips state), phase unrolling (bounded loop linearised into distinct phases), sub-phase loops (iteration in-memory inside one worker), controlled loop with circuit breaker (worker emits backward + attempts counter). + +#### Bad + +```go +// Worker emits a backward edge unconditionally — no attempt counter, no cap +func runReviewingPhase(ctx context.Context) (*Result, error) { + if !approved { + return &Result{Status: "done", NextPhase: PhaseDrafting}, nil + // ← will loop forever if drafting never produces an approvable result + } + return &Result{Status: "done", NextPhase: PhaseFinalized}, nil +} +``` + +#### Good + +```go +// Backward edge with circuit breaker — attempts counter + controller cap +func runReviewingPhase(ctx context.Context, attempts int) (*Result, error) { + if attempts >= 3 { + return &Result{Status: "failed"}, errors.Errorf(ctx, + "reviewing exceeded %d attempts", 3) + } + if !approved { + return &Result{ + Status: "done", + NextPhase: PhaseDrafting, // backward edge — circuit breaker exists above + }, nil + // Controller increments the persisted attempts counter on each backward edge + // and passes it back as the attempts argument on the next invocation. + } + return &Result{Status: "done", NextPhase: PhaseFinalized}, nil +} +``` + The pattern is **forward-only by default**: a worker emits `NextPhase` referring to a *later* phase, never an earlier one. This prevents autonomous infinite loops in long-running agents and bounds total work per task. But real workflows need iteration: retries on transient failures, agent re-planning, operator-driven requeues. There are four ways to handle these without breaking forward-only-by-default: diff --git a/rules/index.json b/rules/index.json index 3acfe98..777b9e8 100644 --- a/rules/index.json +++ b/rules/index.json @@ -278,6 +278,24 @@ "level": "MUST", "owner": "go-http-handler-assistant" }, + { + "anchor": "go-json-error-handler/structured-response-shape", + "applies_when": "an HTTP handler in a Go service emits an error response that is not a JSON object with the canonical `{error: {code, message, details}}` shape — e.g. plain-text bodies, top-level `{message: ...}` without an `error` wrapper, or `details` as a string instead of a `map[string]string`.", + "doc_path": "docs/go-json-error-handler-guide.md", + "enforcement": "judgment (response-shape inspection; ast-grep can detect `http.Error` calls and inline JSON writes but the full contract needs request/response review)", + "id": "go-json-error-handler/structured-response-shape", + "level": "MUST", + "owner": "go-http-handler-assistant" + }, + { + "anchor": "go-json-error-handler/use-error-code-constants", + "applies_when": "a Go HTTP handler passes a raw string literal as the error-code argument to `libhttp.WrapWithCode` / `libhttp.NewJSONError` instead of the `libhttp.ErrorCodeXxx` constants.", + "doc_path": "docs/go-json-error-handler-guide.md", + "enforcement": "judgment (ast-grep follow-up: pattern over `libhttp.WrapWithCode($$, $$, $CODE)` / `WrapWithDetails($$, $$, $CODE, $$)` with `$CODE` constrained to be a `interpreted_string_literal` — see PR #11 recipe for the metavariable-constraint shape)", + "id": "go-json-error-handler/use-error-code-constants", + "level": "MUST", + "owner": "go-http-handler-assistant" + }, { "anchor": "go-licensing/copyright-year-discipline", "applies_when": "a PR diff modifies copyright years in `*.go` source-file headers — either bulk-updating across many files or setting future / non-numeric years (`2099`, `present`, etc.).", @@ -314,6 +332,24 @@ "level": "MUST", "owner": "license-assistant" }, + { + "anchor": "go-linting/banned-packages-via-depguard", + "applies_when": "a Go project's `.golangci.yaml` does not configure `depguard` with the bborbe banned-package list (deprecated stdlib alternatives like `io/ioutil`, v1 of versioned packages where v2+ exists, `pkg/errors` superseded by `bborbe/errors`).", + "doc_path": "docs/go-linting-guide.md", + "enforcement": "judgment (config inspection; ast-grep does not parse golangci YAML structure)", + "id": "go-linting/banned-packages-via-depguard", + "level": "MUST", + "owner": "go-quality-assistant" + }, + { + "anchor": "go-linting/complexity-limits-enforced", + "applies_when": "a Go project's `.golangci.yaml` (or `.golangci.yml`) does not enable `funlen`, `gocognit`, `nestif`, and `maintidx` with the bborbe standard thresholds (80 lines / 50 statements / 20 complexity / 4 nesting / 20 maintainability).", + "doc_path": "docs/go-linting-guide.md", + "enforcement": "judgment (config-file presence + threshold inspection; ast-grep does not parse YAML reliably for nested numeric thresholds)", + "id": "go-linting/complexity-limits-enforced", + "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.", @@ -404,6 +440,24 @@ "level": "MUST", "owner": "go-security-specialist" }, + { + "anchor": "go-state-machine/forward-only-by-default", + "applies_when": "a Go FSM worker emits `NextPhase` referring to an earlier phase in the workflow's declared order, without an explicit circuit-breaker `attempt` counter and a controller-side cap.", + "doc_path": "docs/go-state-machine-pattern.md", + "enforcement": "judgment (semantic — requires reading both the worker's NextPhase-emission logic and the controller's persist logic)", + "id": "go-state-machine/forward-only-by-default", + "level": "SHOULD", + "owner": "go-architecture-assistant" + }, + { + "anchor": "go-state-machine/status-phase-separation", + "applies_when": "a Go workflow / FSM controller persists a single field that conflates \"where in the workflow\" with \"how the last invocation ended\" — e.g. a `state` enum mixing `validating`/`charging`/`done`/`failed` together, or marking a task `completed` while `NextPhase` is still non-terminal.", + "doc_path": "docs/go-state-machine-pattern.md", + "enforcement": "judgment (semantic — requires reading the controller's persist logic)", + "id": "go-state-machine/status-phase-separation", + "level": "MUST", + "owner": "go-architecture-assistant" + }, { "anchor": "go-testing/counterfeiter-mocks-required", "applies_when": "a test file declares a hand-written struct that satisfies a production interface and is used in place of a real implementation under test, instead of importing a `mocks/` fake produced by Counterfeiter.",