go-bananas/docs/standards.md

# Coding standards

This document is normative. Where it says "must", a change that does not follow it
will not merge. Where it says "should", there is a reason to deviate and the reason
belongs in the pull request.

The standards exist to make the codebase readable by someone who has never seen it.
Every rule here is one that, in review, someone would otherwise have to enforce by
hand.

---

## Contents

1. [General principles](#1-general-principles)
2. [Go](#2-go)
3. [TypeScript and React](#3-typescript-and-react)
4. [CSS](#4-css)
5. [API design](#5-api-design)
6. [Error handling](#6-error-handling)
7. [Security](#7-security)
8. [Testing](#8-testing)
9. [Database](#9-database)
10. [Documentation](#10-documentation)
11. [Commits and pull requests](#11-commits-and-pull-requests)
12. [Dependencies](#12-dependencies)
13. [Enforcement](#13-enforcement)

---

## 1. General principles

### 1.1 Comment the decision, not the mechanism

A comment that restates the code is noise and will rot. A comment that explains
*why* something is done that way is one of the most valuable things in a codebase,
because the reasoning is the part that is lost first.

```go
// Bad: restates the code
// Increment the counter
count++

// Good: explains why the obvious thing is wrong
// The window is fixed rather than sliding: a fixed window needs one counter per
// client rather than a list of timestamps, so its memory is O(clients) and a
// client cannot make it degrade by sending requests at a high rate. The cost is a
// burst of up to 2x the limit across a boundary, which is acceptable for the abuse
// it defends against.
```

Every non-obvious constant, every workaround for a library quirk, and every place
where the straightforward implementation would be wrong or insecure gets a
comment. Everything else stays uncommented.

### 1.2 Prefer the boring, explicit solution

This codebase does not need to be clever. It needs to be correct and legible. A
reader should never have to reconstruct an invariant from a clever trick.

When two implementations are equally correct, prefer the one that a reviewer can
verify in one reading.

### 1.3 Fail loudly, fail early

A silent fallback that produces a plausible-looking wrong result is worse than a
crash, because it will not be found. This is why:

- configuration errors are reported in full at start-up, not one per deploy;
- a malformed theme is rejected with the path that failed;
- the token engine never silently drops a token it cannot parse;
- a development authentication bypass refuses to initialise outside development.

### 1.4 Handle the boring cases exhaustively

Empty input, zero values, single-element slices, a nil map, a closed channel, a
context that is already cancelled. Handle them in one place with a clear rule rather
than at each call site.

### 1.5 Do not leak abstractions

A package exposes what it needs to expose. Internal helpers stay unexported. An
interface with one implementation, used from one package, is indirection without
benefit — unless it exists to make the caller testable, which is a real reason and
is called out in a comment when used.

---

## 2. Go

### 2.1 Layout

```
cmd/go-bananas/       the binary. main() must stay trivial.
internal/
  app/                dependency injection and the process lifecycle
  config/             environment parsing and validation
  domain/             entities and invariants. No HTTP, no SQL.
  store/              persistence. The only package that writes SQL.
  tokenengine/        pure logic: colour maths, validation, rendering
  httpapi/            transport: routing, middleware, handlers
  auth/               authentication and authorisation
  logging/            slog configuration
  seed/               built-in themes
```

The dependency rule, enforced by review:

```
cmd → app → { httpapi, store, auth, config, logging }
                    ↓         ↓
              tokenengine  domain ← everything may depend on this
```

`domain` depends on nothing. `tokenengine` depends only on `domain`. `store`
depends only on `domain`. `httpapi` may depend on `store`, `tokenengine` and
`domain`, never on `app`. Nothing imports `cmd`.

### 2.2 Naming

| Kind | Convention | Example |
| --- | --- | --- |
| Exported identifier | `CamelCase`, with the initialism capitalised | `HTTPAddr`, `ParseColor`, `IDToken` |
| Unexported identifier | `camelCase` | `themeColumns`, `renderCache` |
| Interface | Noun, no `I` prefix | `Resolver`, `KeyStore` |
| Error variable | `Err` prefix | `ErrNotFound` |
| Test double | `stub`/`fake` prefix, in `_test.go` | `stubResolver` |
| File | `snake_case` | `token_control.go` |
| Package | One word, lowercase, no underscores | `tokenengine`, `httpapi` |

Initialisms follow the Go convention exactly: `ID`, `URL`, `HTTP`, `API`, `TLS`,
`CSS`, `JSON`, `SQL`, `UUID`. Not `Id`, not `Url`.

### 2.3 Comments

Every exported identifier has a doc comment beginning with the identifier itself:

```go
// ContrastRatio returns the WCAG 2.1 contrast ratio between the two colours,
// always >= 1.
func ContrastRatio(a, b Color) float64 {
```

Unexported identifiers get a comment when the *why* is not obvious from the name.
A comment restating an unexported function's name is still noise.

### 2.4 Errors

- Wrap with context and `%w` so `errors.Is` works through the chain.
- Error strings are lowercase and do not end in punctuation. They read as part of a
  sentence: `invalid configuration`, not `Invalid configuration!`.
- Sentinel errors live in `domain` and are matched with `errors.Is`. Never compare
  error strings.
- A function returns a bare sentinel when the caller already knows the context, and
  a wrapped one when it adds information.
- Never discard an error silently. If an error genuinely does not matter, say so in
  a comment explaining why.

```go
// Good
return nil, fmt.Errorf("open store: %w", err)

// Good: the caller cannot add anything, so return the sentinel unwrapped
return domain.ErrNotFound

// Bad: loses the chain, so errors.Is cannot match it
return nil, fmt.Errorf("failed: %s", err.Error())
```

### 2.5 Context

- `ctx context.Context` is the first parameter of any function that does I/O.
- Never store a context in a struct.
- Every blocking operation takes a context. A network call without one cannot be
  cancelled, and a request handler that cannot be cancelled is a resource leak on
  shutdown.

### 2.6 Concurrency

- Make a type safe for concurrent use, or say clearly that it is not.
- Prefer `sync.RWMutex` over `sync.Mutex` when reads dominate.
- Never copy a type containing a mutex.
- Every goroutine must have an owner and a termination condition. A goroutine
  started in a constructor needs a matching stop path or it leaks on every reload.
- Pre-allocate with a known capacity in hot paths: `make([]T, 0, len(src))`.

### 2.7 Zero values

The zero value of a type must be useful or invalid, never subtly wrong. A
`Cache` whose zero value has no map is fine. A `RateLimiter` whose zero value
silently allows everything is not.

### 2.8 Imports

Standard library first, then third party, then internal, each group separated by a
blank line. `goimports` enforces the grouping; `gci` enforces the ordering within
groups.

Never import a package only to use one constant when a local definition would be
clearer. Never import a package in a blank form to check for a build tag without a
comment saying why.

### 2.9 `defer`

`defer` immediately after the call that can fail, not at the top of the function.
A defer fifty lines below the resource acquisition is a defer that will be attached
to the wrong variable.

`defer file.Close()` and check the error on a read-only close only where it matters.
`defer rows.Close()` needs no error check; `defer resp.Body.Close()` usually does.

---

## 3. TypeScript and React

### 3.1 Strictness

`strict`, `noUncheckedIndexedAccess`, `noImplicitOverride`,
`noFallthroughCasesInSwitch`, `noUnusedLocals`, `noUnusedParameters`,
`verbatimModuleSyntax` and `isolatedModules` are all on. A type error is a build
failure, not a warning.

TypeScript is pinned to **6.0.3**, not 7.x. TypeScript 7 is the native compiler
and ships no JavaScript compiler API, so no type-aware linting tool can use it yet.
This is revisited when the ecosystem catches up; see
[ADR 0004](adr/0004-typescript-version.md).

### What is deliberately *not* enforced

Two rules are turned off in `eslint.config.js`, on purpose:

- **`explicit-function-return-type`**
- **`explicit-module-boundary-types`**

Both demand a written type where `tsc --strict` already enforces one. A component
with a wrongly-typed prop is a compile error either way; what the rules add is
annotation churn — one per callback, and `(): JSX.Element =>` on components that
legitimately start returning `null` and then need the annotation widened. That is a
maintenance cost with no defect class behind it.

What *is* enforced instead, and what this means in practice:

| Rule | Where |
| --- | --- |
| Every component declares a typed props `type` | `tsc` |
| Every exported non-component function has a JSDoc `@returns` | Review, and the JSDoc the docs are generated from |
| `no-floating-promises`, `no-misused-promises` | ESLint, error |
| `switch-exhaustiveness-check` | ESLint, error |
| `no-unnecessary-condition` (in `web/`) | ESLint, error |

The one place it *is* relaxed is the SDK, where `no-unnecessary-condition` is off
because the DOM type definitions describe a fully featured browser and the runtime
guards that protect Safari below 14 and embedded webviews are load-bearing. That is
recorded in `sdk/eslint.config.js` next to the rule.

### 3.2 `any` is not permitted

`any` disables checking for everything it touches. Use `unknown` at a boundary and
narrow it. When a genuinely untyped value arrives — a JSON body, a third-party
library — declare the shape you expect rather than casting to `any`.

An `any` with a comment explaining exactly why it cannot be avoided is acceptable
only in a test double or at the boundary of an untyped library.

### 3.3 Types

- `type` over `interface` for unions, mapped types and anything conditional.
  `interface` over `type` for object shapes meant to be extended or implemented.
- Prefer a named union over a union of inline literals. `type Appearance =
  'light' | 'dark'` is a domain concept and deserves a name.
- Model optionality honestly. An optional field means the value may genuinely be
  absent; a field that is always present should not be marked optional to silence a
  type error.
- Discriminated unions over optional fields for state that varies together:
  `{ status: 'loading' } | { status: 'ready', theme: Theme }` cannot be represented
  wrongly, whereas `{ status: 'ready', theme?: Theme }` can.

### 3.4 Components

- One component per file unless they are trivially small and only used together.
- Props are typed with an interface or type, never inline.
- A component is a pure function of its props unless it has a stateful reason to
  exist. If a `useMemo` is there purely for readability, it is noise.
- Destructure props in the parameter list when more than about three are used.
- Every component that renders user-visible text must be able to render it from a
  prop or a translation table. Hard-coded English in a leaf component is not
  acceptable; a page title in the default language is.
- No component over 400 lines. A component that long is doing several jobs and the
  split is usually along the line of "container" and "presentational".

### 3.5 Hooks

- Hooks are called at the top level, unconditionally. A hook inside a condition is a
  bug waiting for the condition to change.
- Every effect that subscribes, times out or fetches returns a cleanup function.
- An effect that only computes derived state is wrong: compute it during render.
- Dependency arrays must be complete. If one looks wrong, the component is probably
  deriving state that should not be state.
- `useMemo` and `useCallback` are for referential stability and genuinely expensive
  computation, not for clarity. Do not use them to satisfy an exhaustive-deps warning
  without understanding the consequence.

### 3.6 State

- Server state lives in TanStack Query, not in `useState`. A `useState` holding
  something a fetcher owns will inevitably disagree with the fetcher.
- Local form state is `useState`. A form with more than about ten fields, or one that
  needs validation on blur, belongs in a form library or in a reducer.
- State is declared at the lowest component that needs it and not lifted until two
  siblings genuinely share it.

### 3.7 Imports

Alias `@/` resolves to `src/`. Relative imports within a directory, aliased imports
across directories. Named imports, never namespace imports, unless the namespace is
genuinely used as one.

---

## 4. CSS

### 4.1 Tailwind

- Utility classes in JSX. A stylesheet is for things utilities cannot express:
  keyframes, cascade layers, and generated design tokens.
- Class order follows the Prettier plugin, so formatting is never a review topic.
- Arbitrary values (`bg-[#123456]`) are a last resort. If the same value appears
  twice it becomes a theme token.
- Colour is never hard-coded in a component. It comes from a semantic token, which is
  the entire premise of the project.

### 4.2 The token layer

The admin UI is itself a consumer of go-bananas. Runtime variables are mapped onto
Tailwind's namespaces in `index.css` and nowhere else. A component that needs a new
colour extends the mapping, not its own className.

### 4.3 Accessibility

- A visible focus indicator on every interactive element. `:focus-visible` with at
  least a 2:1 contrast ratio against its background.
- Interactive elements are `button`, `a`, `input`, `select` or `textarea`. A `div`
  with `onClick` is not a control: it cannot be focused, does not appear in the
  accessibility tree, and does not respond to the keyboard.
- Every icon-only control has an accessible name.
- Colour is never the only signal. A status badge has text as well as a colour.
- Motion respects `prefers-reduced-motion`.
- Interactive targets are at least 44×44 CSS pixels on touch, or have sufficient
  spacing to the standard.

---

## 5. API design

### 5.1 URLs

- Plural nouns for collections, kebab-case for members: `/api/v1/themes`,
  `/api/v1/themes/{slug}/versions`.
- The resource identity is in the path. The query string holds filtering, sorting
  and pagination.
- `PATCH` for a partial update, `PUT` for a full replacement. A partial update must
  ignore absent fields rather than writing their zero values.

### 5.2 Status codes

| Code | When |
| --- | --- |
| `200` | Success with a body |
| `201` | Resource created, with a body |
| `204` | Success, no body. A delete or revoke |
| `304` | Conditional request matched |
| `400` | Malformed or invalid input |
| `401` | No usable credential |
| `403` | Authenticated but insufficient role |
| `404` | Not found, **or** deliberately not disclosed |
| `409` | Uniqueness or optimistic-concurrency conflict |
| `413` | Body too large |
| `422` | Reserved for a future validation-error shape |
| `429` | Rate limited, with `Retry-After` |
| `500` | Unexpected fault |

A `404` rather than a `403` is returned when confirming existence would itself leak
information. A draft theme returns `404` from the runtime API for exactly this
reason.

### 5.3 Error bodies

One shape everywhere:

```json
{
  "code": "validation_failed",
  "message": "the theme cannot be published because its tokens are invalid",
  "details": [{ "field": "color.semantic.primary", "message": "not a valid colour" }],
  "requestId": "0f90d3cb9ab8e449"
}
```

`code` is a stable machine-readable identifier and is part of the contract: clients
branch on it, so its meaning must not change between releases. `message` is for a
human. `requestId` correlates with the server log and is always present on errors.

### 5.4 Versioning

`/api/v1` and `/rt/v1` are independent contracts, each with an `apiVersion`. A
breaking change is a new major version served alongside the old one for at least one
release cycle. Adding an optional field, a new endpoint or a new query parameter is
not a breaking change.

### 5.5 Pagination

Cursor-based for collections that grow without bound (audit), offset-based for
collections a human pages through (themes). Both return `{ items, total, limit,
offset }` or the cursor equivalent, so the shape is predictable.

### 5.6 Idempotency

`DELETE` and revoke operations are idempotent: repeating them succeeds. A `PUT`
or `PATCH` that would change nothing still succeeds. Only non-idempotent operations
require an idempotency key, and only where a client genuinely cannot retry.

---

## 6. Error handling

### 6.1 The layers

| Layer | Responsibility |
| --- | --- |
| Domain | Returns typed sentinel errors. Knows nothing about HTTP. |
| Store | Translates driver-specific errors into domain sentinels, once, in one place. |
| Handlers | Translate domain sentinels into status codes, once, in `writeProblem`. |
| Frontend client | Turns a non-2xx into an `ApiError` carrying the code and the fields. |

A handler never chooses a status code itself, and no repository ever matches on an
error string.

### 6.2 Never swallow

A swallowed error is a bug that will be found by a user. If an error genuinely
cannot be handled at a call site, log it at the right level and say why in a comment.

```go
// The credentials were already validated, so a failure here means the database is
// unreachable; failing the login would be worse than a missing directory entry.
if err := a.keys.UpsertUser(ctx, identity); err != nil {
	log.Error("record login", "error", err)
}
```

### 6.3 Panic

Only for programmer errors: an invariant that cannot be true, or a nil where the
type says non-nil. Never for bad input. The HTTP layer recovers from any panic and
returns a 500, because a panic on a request goroutine must not take the process
down.

---

## 7. Security

### 7.1 Secrets

- Secrets come from the environment, injected by Kubernetes from a Secret. Never
  from a values file, a ConfigMap, a command-line flag or a committed file.
- A secret is never logged, never rendered into a ConfigMap, and never included in an
  error message. `config.Redacted()` exists for logging the configuration.
- Anything that generates source for someone else to paste is an output boundary: the
  value is escaped for the *target language*, not for HTML, and the language check is
  on the input rather than trusted.
- `crypto/rand` for anything security-relevant. Never `math/rand`.

### 7.2 Authentication

- Fail closed. An unmapped group is viewer, never admin.
- A development bypass refuses to initialise outside a development environment. This
  is enforced in code and refused again by the configuration validator, because an
  accidentally deployed bypass is a total authentication bypass.
- Session cookies are `HttpOnly`, `Secure`, `SameSite` and signed. The signature is
  compared in constant time.
- The post-login redirect is restricted to a local path. An absolute `returnTo` is an
  open redirect that also primes a phishing flow.

### 7.3 Input

- Every query parameter that reaches SQL goes through an allow-list of column names.
  Never interpolate a caller's string into a statement.
- `LIKE` patterns are escaped so a user-supplied `%` is a literal.
- Request bodies are size-limited. A theme document is a few kilobytes; the limit is
  2 MiB.
- JSON with unknown fields is rejected, so a client typo is reported rather than
  silently ignored.
- Rendered HTML is escaped. Interpolation into a template goes through
  `html.EscapeString` even when the value is operator-controlled.

### 7.4 HTTP

- TLS terminated at the ingress. HSTS advertised only when the deployment is
  actually served over HTTPS.
- `Content-Security-Policy` with no `unsafe-eval` and no wildcard script source.
- `X-Content-Type-Options: nosniff` and `X-Frame-Options: DENY` on every response.
- CORS is an explicit allow-list. A wildcard origin with credentials is never valid.
- `X-Forwarded-For` is believed only from a configured proxy CIDR. Without that,
  anyone can spoof the client address in the audit log.

### 7.5 Audit

Every mutation records an actor, an action, a target and a timestamp. Actor comes
from the authenticated identity, never from the request body. An audit write
failure is logged but does not fail the user's operation: losing an audit row is bad,
failing a publish because of it is worse.

---

## 8. Testing

### 8.1 What to test

- **Domain and tokenengine logic:** exhaustively. This is where a subtle mistake is
  most expensive and most invisible in review.
- **Store behaviour:** the contract, not the implementation. Uniqueness, cascade
  behaviour, optimistic concurrency, the error sentinels.
- **HTTP handlers:** the status code and the error body for each branch, plus the
  authorisation decision for each role.
- **Frontend logic:** the pure helpers, exhaustively. Component tests focus on
  behaviour a user depends on, not on structure.
- **Security behaviour:** every guard, as a test that fails when the guard is
  removed. A guard with no test is a guard that will be removed by accident.

### 8.2 What not to test

Implementation details, private function structure, and anything that would break on
a refactor without a behaviour change. A test that fails when a private helper is
renamed is a test that will be deleted rather than fixed.

### 8.3 Style

Table-driven where the cases share a shape. One assertion topic per test, with the
assertion message explaining the expectation rather than restating the code.
Prefer `require` over `expect` for preconditions that make the rest of the test
meaningful.

Name the test after the behaviour, not the function: `TestContrastRatioMatchesWCAGReferenceValues`,
not `TestContrast`.

A test that cannot fail is worse than no test. If a bug is found in production, add
the test that would have caught it, in the same change.

### 8.4 Required coverage

| Package | Minimum |
| --- | --- |
| `tokenengine` | 90%, including WCAG reference values |
| `domain` | 90% |
| `store` | 80% |
| `auth` | 85%, including every security guard |
| `httpapi` | 75%, with every authorisation branch |
| `web/src/lib` | 90% |

---

## 9. Database

- All SQL lives in `internal/store`. No handler writes SQL.
- Column names are never interpolated from caller input. Sorting and filtering go
  through an allow-list (`domain.SortFields`).
- Every foreign key has an explicit `ON DELETE` behaviour. Cascade is a decision, not
  an accident.
- Every index exists because a query needs it, and its justification is in the
  migration comment.
- Timestamps are TEXT in RFC 3339, which is sortable lexicographically, so `ORDER
  BY` and range filters need no date parsing.
- Migrations are embedded in the binary and applied at start-up. They must be
  idempotent: `CREATE TABLE IF NOT EXISTS` and friends.
- A schema change that is destructive or irreversible gets a snapshot step in the
  Helm chart and an entry in the runbook.

---

## 10. Documentation

- A package comment states what the package is for, what it depends on, and what
  depends on it. Every package has one.
- `README.md` is for someone who has never seen the project. Its quick start must
  work as written, from a clean clone.
- `docs/standards.md` is this document. It changes by the same review as the code.
- An architecture decision that could reasonably have gone another way is recorded in
  `docs/adr/`, including the option that was rejected and why. An ADR is never
  edited to change the decision; it is superseded.
- A comment explaining a workaround names the issue or the version that caused it, so
  a future reader knows when to remove it.

---

## 11. Commits and pull requests

### 11.1 Commit messages

[Conventional Commits](https://www.conventionalcommits.org/), because the release
notes and the changelog are generated from them.

```
<type>(<scope>): <subject>

<body>

<footer>
```

Types: `feat`, `fix`, `perf`, `refactor`, `docs`, `test`, `build`, `ci`, `chore`.

The subject is imperative, under 72 characters, and describes what the change does
rather than which files it touches. The body explains why, and what was considered
and rejected.

### 11.2 Pull requests

- One concern per pull request. A refactor and a behaviour change are two pull
  requests.
- The description states what changed, why, and how it was verified. If a decision
  was not obvious, it is explained.
- The test plan is concrete: not "tests pass" but "a built-in theme rendering with
  an unreadable foreground still passes the contrast assertion".
- Screenshots for any visual change.
- Every CI check green. A failing check that is "expected" is not merged.

---

## 12. Dependencies

- The standard library first. A dependency is a permanent maintenance obligation
  and a permanent supply-chain surface.
- Every new dependency is justified in the pull request: what it does, why the
  standard library cannot, and its maintenance status.
- Prefer a dependency already in the tree over a new one for a similar job.
- Pin versions exactly. A caret range in an application is how a build becomes
  unreproducible.
- `go mod tidy` and `npm ci` in CI. Never `npm install` in a pipeline.
- Renovate or Dependabot handles updates; they merge on a green build.

Current stack, and why each is present, is in
[docs/architecture.md](architecture.md#the-stack).

---

## 13. Enforcement

| Rule | Enforced by |
| --- | --- |
| Go formatting | `gofmt -l`, `make fmt-check` |
| Go lint | `golangci-lint`, `make lint-go` |
| Go vet | `go vet`, part of `make check` |
| Go import grouping | `gci`, in `.golangci.yml` |
| Go tests, with the race detector | `go test -race`, `make test-go` |
| TypeScript strictness | `tsc --build`, `make lint-web` |
| Frontend lint | `eslint --max-warnings 0`, `make lint-web` |
| SDK lint | `eslint`, `make lint-sdk` |
| Frontend formatting | `prettier`, `make format-web` |
| Frontend tests | `vitest`, `make test-web` |
| Chart validity | `helm lint --strict`, `make helm-lint` |
| Chart values | `values.schema.json` |
| Chart configuration | `_helpers.tpl` guards, at render time |
| Container build | `docker build`, `make image` |
| Everything | `make verify` |

`make verify` is the pre-merge gate. It is the same command CI runs.