From fc1faa6d749930a6d6dd1e95859c36b9c68fc477 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 14:27:16 +0200 Subject: [PATCH 1/3] feat(rules): bootstrap 3 doc families (linting, state-machine, json-error-handler) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First multi-doc bootstrap PR — 3 small docs (~450 lines each) in one worktree, single bot review cycle, single walker regen. Faster than serial PRs without parallel-worktree merge-conflict risk on rules/index.json. Rules added (rules/index.json: 56 -> 62): go-linting/* (owner: go-quality-assistant) - complexity-limits-enforced (MUST) — funlen/gocognit/nestif/maintidx configured at standard thresholds (80/50/20/4/20). - banned-packages-via-depguard (MUST) — pkg/errors, io/ioutil, deprecated v1 packages explicitly banned via depguard config. go-state-machine/* (owner: go-architecture-assistant) - status-phase-separation (MUST) — Phase (WHERE) and Status (HOW) must be independent fields. Conflating them produces the workflow-stall bug (worker emits 'done' for first phase, controller marks task completed, stops spawning). - forward-only-by-default (SHOULD) — workers emit NextPhase referring to later phases; backward edges require an attempts counter and controller-side cap (circuit breaker pattern). go-json-error-handler/* (owner: go-http-handler-assistant) - structured-response-shape (MUST) — error responses use {error: {code, message, details}} canonical shape, never plain text or top-level {message}. - use-error-code-constants (MUST) — use libhttp.ErrorCodeXxx constants, never raw string literals (typo-resistance, single source of truth). All examples generic (User, Order, columnGroup). No personal vault paths, no trading-domain terms. Pre-emptive grep clean. Multi-doc PR scope rationale: - Single bot review cycle covers all 6 rules - rules/index.json walker-regen happens once, no parallel-worktree conflicts - ~3x faster than serial PRs with same review thoroughness make build-index regenerated; check-index passes. --- docs/go-json-error-handler-guide.md | 63 +++++++++++++++++++++ docs/go-linting-guide.md | 69 +++++++++++++++++++++++ docs/go-state-machine-pattern.md | 87 +++++++++++++++++++++++++++++ rules/index.json | 54 ++++++++++++++++++ 4 files changed, 273 insertions(+) diff --git a/docs/go-json-error-handler-guide.md b/docs/go-json-error-handler-guide.md index f8716d9..d4863d9 100644 --- a/docs/go-json-error-handler-guide.md +++ b/docs/go-json-error-handler-guide.md @@ -21,6 +21,40 @@ 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 +return libhttp.WrapWithCode( + 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 +78,35 @@ 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, $$)` 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 + nil, +) +``` + +#### Good + +```go +return libhttp.WrapWithCode( + errors.Errorf(ctx, "invalid input"), + http.StatusBadRequest, + libhttp.ErrorCodeValidation, // typo fails at compile time + nil, +) +``` + | Code | HTTP Status | Usage | |------|-------------|-------| | `VALIDATION_ERROR` | 400 | Invalid request parameters, malformed input | diff --git a/docs/go-linting-guide.md b/docs/go-linting-guide.md index bf12a4e..47f7971 100644 --- a/docs/go-linting-guide.md +++ b/docs/go-linting-guide.md @@ -147,6 +147,41 @@ 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 +linters: + enable: + - funlen + - gocognit + - nestif + - maintidx + - revive + - errcheck +linters-settings: + funlen: { lines: 80, statements: 50 } + gocognit: { min-complexity: 20 } + nestif: { min-complexity: 4 } + maintidx: { under: 20 } +``` + | Linter | Limit | Description | |--------|-------|-------------| | `funlen` | 80 lines / 50 statements | Max function length | @@ -210,6 +245,40 @@ 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 +linters: + enable: [depguard, revive, errcheck] +linters-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..a391281 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,46 @@ 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 + Metadata: map[string]string{"attempts": fmt.Sprint(attempts + 1)}, + }, nil + } + 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..f4172bf 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, $$)` 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.", From b9ea87fbc56b57fa6d4ba7ccdb79448fda90bb28 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 14:37:32 +0200 Subject: [PATCH 2/3] fix(rules): address bot review on PR #17 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MAJOR: operator-precedence bug in status-phase-separation Good example. Wrote 'result.Status == "done" && result.NextPhase == "" || result.NextPhase == PhaseDone' which evaluates as '(done && empty) || PhaseDone' — fires on PhaseDone regardless of Status, exactly the kind of state-machine bug the rule is supposed to teach readers to avoid. Parenthesised the OR. NIT: maintidx YAML param renamed from 'under: 20' to 'min-maintainability-index: 20' to match golangci-lint v2 and the guide's own reference config earlier in the file. --- docs/go-linting-guide.md | 2 +- docs/go-state-machine-pattern.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/go-linting-guide.md b/docs/go-linting-guide.md index 47f7971..a37640d 100644 --- a/docs/go-linting-guide.md +++ b/docs/go-linting-guide.md @@ -179,7 +179,7 @@ linters-settings: funlen: { lines: 80, statements: 50 } gocognit: { min-complexity: 20 } nestif: { min-complexity: 4 } - maintidx: { under: 20 } + maintidx: { min-maintainability-index: 20 } ``` | Linter | Limit | Description | diff --git a/docs/go-state-machine-pattern.md b/docs/go-state-machine-pattern.md index a391281..8e04102 100644 --- a/docs/go-state-machine-pattern.md +++ b/docs/go-state-machine-pattern.md @@ -165,7 +165,7 @@ type Task struct { func persist(task *Task, result *Result) { switch { - case result.Status == "done" && result.NextPhase == "" || result.NextPhase == PhaseDone: + case result.Status == "done" && (result.NextPhase == "" || result.NextPhase == PhaseDone): task.Status = "completed" // workflow terminates case result.Status == "done": task.Phase = result.NextPhase From b75eb2fbffd00483b11d13b20b3ec6235d2eab70 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 14:52:06 +0200 Subject: [PATCH 3/3] =?UTF-8?q?fix(rules):=20address=20bot=20review=20on?= =?UTF-8?q?=20PR=20#17=20=E2=80=94=208=20MAJOR=20+=202=20NIT?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MAJOR (all from bot review on b9ea87f, all real): 1. WrapWithCode signature: I called it with 4 args (err, status, code, details) but the actual signature is 3-arg (err, status, code). Fixed both Bad and Good examples in use-error-code-constants. The details-map case now uses WrapWithDetails (4-arg, the function actually designed for that shape). Enforcement note updated to mention both function names. 2. CLAUDE.md doc-agent alignment table missing entries — added rows for 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. Per project CLAUDE.md's 'Doc-Agent Alignment' rule. 3-4. golangci-lint v1 'linters-settings:' vs v2 'settings:' inconsistency. My new YAML examples used v1 form while the canonical config example earlier in the doc uses v2. Converted both new examples to v2 + added 'version: "2"' header to match. 5. Dead external reference to bborbe/go-skeleton repo. Self-contained principle: replaced with pointer to 'templates/.golangci.yml' inside this plugin. 6. depguard 'main:' (lowercase) vs 'Main:' (capitalized). v2 is case-sensitive; the canonical config uses Main: so my new example was wrong. Aligned to Main:. 7-8. State-machine circuit-breaker example: referenced Result.Metadata field that doesn't exist in the Result struct, and used fmt.Sprint without importing fmt. Removed the Metadata write; added a comment explaining the controller maintains the attempts counter externally (which is closer to the actual pattern anyway). NIT: - Test assertion in go-json-error-handler-guide.md used 'Equal("VALIDATION_ERROR")' contradicting use-error-code-constants. Replaced with libhttp.ErrorCodeValidation. NOT changed (deliberate): - State-machine rule ownership stays go-architecture-assistant. Bot argued go-quality-assistant fits better, but status-phase-separation is about persisted-state shape across worker/controller boundaries (cross-unit) and forward-only-by-default is about FSM control flow (cross-unit). Both fit the architecture agent's stated scope. - Rule-ID 2-component vs 3-component schema — bot's NIT claims 3 is required, but build-index.py's regex allows 2-3 components ('([a-z0-9-]+/[a-z0-9-]+(?:/[a-z0-9-]+)?)'). 2-component IDs are valid; consistent with all 11 existing families. --- CLAUDE.md | 3 ++ docs/go-json-error-handler-guide.md | 12 ++++---- docs/go-linting-guide.md | 47 +++++++++++++++-------------- docs/go-state-machine-pattern.md | 3 +- rules/index.json | 2 +- 5 files changed, 36 insertions(+), 31 deletions(-) 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 d4863d9..0da6901 100644 --- a/docs/go-json-error-handler-guide.md +++ b/docs/go-json-error-handler-guide.md @@ -38,8 +38,10 @@ http.Error(w, "columnGroup '' is unknown", http.StatusBadRequest) #### Good ```go -// Canonical JSON shape via libhttp.NewJSONErrorHandler -return libhttp.WrapWithCode( +// 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, @@ -82,7 +84,7 @@ All JSON errors follow this structure: **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, $$)` with `$CODE` constrained to be a `interpreted_string_literal` — see PR #11 recipe for the metavariable-constraint shape) +**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 @@ -92,7 +94,6 @@ return libhttp.WrapWithCode( errors.Errorf(ctx, "invalid input"), http.StatusBadRequest, "VAIDATION_ERROR", // typo — client dispatch silently misses this - nil, ) ``` @@ -103,7 +104,6 @@ return libhttp.WrapWithCode( errors.Errorf(ctx, "invalid input"), http.StatusBadRequest, libhttp.ErrorCodeValidation, // typo fails at compile time - nil, ) ``` @@ -413,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 a37640d..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 @@ -167,19 +167,14 @@ linters: #### Good ```yaml +version: "2" linters: - enable: - - funlen - - gocognit - - nestif - - maintidx - - revive - - errcheck -linters-settings: - funlen: { lines: 80, statements: 50 } - gocognit: { min-complexity: 20 } - nestif: { min-complexity: 4 } - maintidx: { min-maintainability-index: 20 } + 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 | @@ -265,18 +260,24 @@ linters: #### Good ```yaml +version: "2" linters: enable: [depguard, revive, errcheck] -linters-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" } + 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 | diff --git a/docs/go-state-machine-pattern.md b/docs/go-state-machine-pattern.md index 8e04102..918f9c2 100644 --- a/docs/go-state-machine-pattern.md +++ b/docs/go-state-machine-pattern.md @@ -293,8 +293,9 @@ func runReviewingPhase(ctx context.Context, attempts int) (*Result, error) { return &Result{ Status: "done", NextPhase: PhaseDrafting, // backward edge — circuit breaker exists above - Metadata: map[string]string{"attempts": fmt.Sprint(attempts + 1)}, }, 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 } diff --git a/rules/index.json b/rules/index.json index f4172bf..777b9e8 100644 --- a/rules/index.json +++ b/rules/index.json @@ -291,7 +291,7 @@ "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, $$)` with `$CODE` constrained to be a `interpreted_string_literal` — see PR #11 recipe for the metavariable-constraint shape)", + "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"