# 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. ``` ():