Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CLAUDE.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -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` |
Expand Down
65 changes: 64 additions & 1 deletion docs/go-json-error-handler-guide.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand All@@ -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 |
Expand DownExpand Up@@ -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"))
}
```
Expand Down
72 changes: 71 additions & 1 deletion docs/go-linting-guide.md
Original file line numberDiff line numberDiff line change
@@ -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

Expand DownExpand Up@@ -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 |
Expand DownExpand Up@@ -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 |
Expand Down
88 changes: 88 additions & 0 deletions docs/go-state-machine-pattern.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -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:
Expand Down
54 changes: 54 additions & 0 deletions rules/index.json
Original file line numberDiff line numberDiff line change
Expand Up@@ -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.).",
Expand DownExpand Up@@ -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.",
Expand DownExpand Up@@ -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/<Name>` fake produced by Counterfeiter.",
Expand Down
Loading