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
2 changes: 2 additions & 0 deletions openspec/changes/specify-go-code-standards/.openspec.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-08-15
112 changes: 112 additions & 0 deletions openspec/changes/specify-go-code-standards/design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
## Context

Five repositories write Go. Each states its conventions in its own
`CONTRIBUTING.md`, except `osapi`, which states them in `CLAUDE.md` and again in
`docs/docs/sidebar/development/development.md`.

The survey that preceded this change compared what each repository states
against what its code does, rather than comparing the documents to each other.
That distinction produced the finding: nothing conflicts. Where a repository
does not state a rule, it follows the rule anyway.

## Goals / Non-Goals

**Goals.** State the shared conventions once. Leave each repository free to
state what is genuinely its own.

**Non-Goals.** Changing any code. Every requirement here describes what the five
repositories already do.

## Decisions

### Measured against the code, not the documents

Each rule was checked by counting:

| Rule | Measured |
| --------------------------------- | ----------------------------------------------------------------- |
| Table-driven suites | every test package in all five repositories — 70, 1, 1, 2, and 72 |
| `types.go` holds only types | 64 files, none containing a function |
| A file is named for what it holds | no `helpers.go` or `utils.go` in any repository |
| Generated mocks | four repositories mock; all four generate |

Two of these are stated in only two or three of the five repositories, and
followed by all five. That is what the capability is for: a rule surviving on
memory in the repositories that never wrote it down.

*Alternative considered:* reconcile the five documents against each other and
write the intersection. Rejected — the intersection would have dropped
`types.go` and the file-naming rule, which only some repositories state and all
five obey. Comparing documents finds what everyone wrote; comparing documents to
code finds what everyone does.

### Mocking is conditional, because one repository legitimately has none

`osapi-orchestrator` declares no mocking library and mocks nothing. It tests
against `httptest.Server` — a real HTTP server on a local port — so there is no
interface to substitute.

Written as "SHALL use gomock", the requirement would make a compliant repository
non-compliant, and the fix would be to introduce a dependency it does not need.
The requirement therefore binds the choice of mock rather than the choice to
mock: where an interface is replaced, the replacement is generated.

This is the second time this repository's testing approach has been mistaken for
an omission. A task in `correct-documentation-drift` asked it to "state that it
uses no mocking library", on the premise that silence meant something was
missing.

*Alternative considered:* require a mocking library everywhere for consistency.
Rejected — consistency in what a rule permits is not the same as consistency in
what a repository does, and the second is not worth a dependency.

### `export_test.go` is constrained by what it may expose

The restrictive form came from `gohai`, where aliases exposing unexported
functions had produced tests that re-covered paths the caller's own test already
exercised, and pinned intermediate steps so they could not be changed without
rewriting tests.

The reasoning is not specific to collectors. An alias makes an internal step
directly callable, and a directly callable step attracts a test. The requirement
therefore applies organization-wide.

It constrains what such a file may contain rather than requiring one to exist,
so the two repositories that do not use the pattern are unaffected, and the two
that use it heavily cannot use it to introduce test-only seams.

*Alternative considered:* leave it to `gohai`, since only two repositories use
the file. Rejected — a rule stated only where it was learned is a rule the next
repository discovers by making the same mistake.

### The reason is stated with the rule

Several requirements carry a sentence explaining what goes wrong without them —
why a hand-written mock is worse than a generated one, why an untagged seam
attracts a duplicate test, why `helpers.go` accumulates.

A rule whose cost is invisible is one a future contributor relaxes in good
faith. Stating the failure makes the rule arguable on its merits rather than
enforced by authority.

## Risks / Trade-offs

- **A repository may need an exception.** The requirements state conditions
rather than absolutes where a legitimate exception exists — mocking is the
worked example.
- **These rules are testable only by reading code.** Nothing here is enforced by
a linter. That is a reason to write them down, not a reason not to.

## Migration Plan

No code changes. Each repository's `CONTRIBUTING.md` drops the shared
conventions and points at the capability. `osapi` additionally resolves the
duplication between `CLAUDE.md` and `development.md`, which
`specify-documentation-homes` task 3.6 requires before a root `CONTRIBUTING.md`
can be written.

## Open Questions

- Should the suite naming convention (`{Name}PublicTestSuite`) be a requirement?
All five follow it, but it is a naming detail rather than a structural rule,
and the capability is already long.
46 changes: 46 additions & 0 deletions openspec/changes/specify-go-code-standards/proposal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
## Why

Five repositories write Go, and each states its own conventions. The rules are
the same rules — they are simply written down five times, in five files, with no
mechanism keeping them in agreement.

Surveying what each repository states against what its code does found no
conflict, and two rules already universal in practice but recorded in only some
of the repositories that follow them:

| Rule | Stated in | Followed by |
| ----------------------------------------------------------- | --------- | ----------------------------------- |
| Multi-line signatures, gofumpt, early returns, import order | 5 of 5 | all |
| Table-driven testify suites | 5 of 5 | every test package in all five |
| `types.go` holds only types | 2 of 5 | all — no violation in 64 files |
| A file is named for what it holds | 3 of 5 | all — no `helpers.go` or `utils.go` |
| Generated mocks rather than hand-written | 2 of 5 | the four that mock anything |
| `export_test.go` exposes only setters | 2 of 5 | where the pattern is used |

A convention followed everywhere and written down twice is a convention that
survives by memory. The next repository copies whichever file it was started
from, and the rules that were only in the other file are lost without anyone
deciding to drop them.

## What Changes

- Add a `go-code-standards` capability recording the conventions all five
repositories already follow.
- Each repository's `CONTRIBUTING.md` states what is specific to it and points
at the capability for the rest.

## Capabilities

### Added Capabilities

- `go-code-standards`: how Go is written across the organization — signatures,
file naming, test structure, mocking, and the style baseline.

## Impact

- `gohai`, `nats-client`, `nats-server`, `osapi-orchestrator`: `CONTRIBUTING.md`
stops restating shared conventions.
- `osapi`: `CLAUDE.md` drops its `Code Standards` section, and the testing
conventions duplicated into `development.md` resolve to one source — which is
what `specify-documentation-homes` task 3.6 requires before a root
`CONTRIBUTING.md` can be written.
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
## Purpose

Records the Go conventions every repository in the organization already follows,
so they are stated once rather than restated in each repository's contributor
documentation.

## ADDED Requirements

### Requirement: A function signature with parameters spans lines

A function declaration taking one or more parameters SHALL place each parameter
on its own line, and the closing parenthesis with the return types on a line of
its own. A function taking no parameters SHALL stay on one line.

#### Scenario: A parameter is added

- **WHEN** a parameter is added to a function
- **THEN** the diff shows one added line, rather than a rewritten signature

### Requirement: A file is named for what it holds

A file SHALL be named for its contents. `helpers.go`, `utils.go`, and names of
that kind SHALL NOT be used: they describe where code was put rather than what
it is, and they accumulate whatever has no other home.

A file named `types.go` SHALL contain only type declarations — structs,
interfaces, constants, and aliases. A function belongs in a file named for what
it does.

A test file SHALL be named for the production file it tests. Where tests need
splitting by concern, the production file is split first, so each test file
keeps a counterpart.

#### Scenario: A function has no obvious home

- **WHEN** a contributor cannot decide where a function belongs
- **THEN** a file is named for what the function does, rather than adding it to
a general-purpose one

#### Scenario: Tests for one file grow too large

- **WHEN** a test file covers more than is comfortable to read
- **THEN** the production file is split and each part gets its own test file,
rather than tests being split away from the file they cover

### Requirement: Tests are table-driven suites

Tests SHALL use `testify/suite` with table-driven cases.

Each function under test SHALL have one suite method. Every scenario for that
function — success, each error, each edge case — SHALL be a row in that method's
table rather than a separate method.

Tests SHALL be external by default, in a `_test` package, exercising the
exported surface. An internal test is for what the exported surface cannot
reach.

#### Scenario: A function gains an error path

- **WHEN** a new failure mode is added to a function under test
- **THEN** it is a row in the existing table, rather than a new test method

#### Scenario: A behavior is reachable from outside

- **WHEN** a behavior can be exercised through the exported surface
- **THEN** the test does so, rather than reaching inside the package

### Requirement: Mocks are generated

Where a test replaces an interface with a mock, the mock SHALL be generated by
`mockgen` from `go.uber.org/mock` and committed. A mock SHALL NOT be written by
hand.

A hand-written mock drifts from its interface silently: adding a method breaks
the generated mock at compile time and leaves the hand-written one satisfying an
interface it no longer matches.

This requirement binds a repository that mocks an interface. A repository that
tests against a real implementation — a filesystem in memory, a local HTTP
server — is not required to introduce mocking.

#### Scenario: An interface gains a method

- **WHEN** a method is added to a mocked interface
- **THEN** regenerating the mock is the only step needed, and skipping it fails
the build

#### Scenario: A test needs a substitute implementation

- **WHEN** a test needs to stand in for a dependency
- **THEN** it uses a generated mock or a real implementation, rather than a
struct written to satisfy the interface

### Requirement: Test-only seams stay out of production code

Where a test needs access to an unexported symbol, the package MAY provide an
`export_test.go` exposing it. That file SHALL expose setters that replace a
value and return a function restoring it.

It SHALL NOT expose an alias to an unexported function. An alias makes an
internal step directly callable, which invites a test that exercises it in
isolation — duplicating coverage the caller's own test already provides, and
pinning an implementation detail so it cannot be changed without rewriting
tests.

A seam SHALL be placed at the boundary with a dependency, not partway through
the code under test, so that the logic between the entry point and the boundary
runs in every case.

#### Scenario: A test needs to force an error from a dependency

- **WHEN** a test needs a library call to fail
- **THEN** it replaces that call at the boundary, and the code between the entry
point and the boundary executes

#### Scenario: An internal step looks worth testing directly

- **WHEN** an unexported function seems to warrant its own test
- **THEN** it is covered through the exported surface that calls it, rather than
exposed to be called directly

### Requirement: Style baseline

Go SHALL be formatted with `gofumpt` and linted with `golangci-lint`, both
enforced by continuous integration.

An error crossing a function boundary SHALL be wrapped with context using `%w`,
so the chain names each layer it passed through and remains inspectable with
`errors.Is` and `errors.As`.

Code SHALL return early rather than nesting the successful path inside
conditionals. An unused parameter SHALL be named `_`.

Imports SHALL be grouped standard library, third party, then local, separated by
blank lines.

#### Scenario: An error surfaces several layers up

- **WHEN** an error from a dependency reaches a caller
- **THEN** the message names the operations it passed through, and the original
error is still recoverable

#### Scenario: A function grows a precondition

- **WHEN** a check is added before the main work
- **THEN** it returns early on failure, rather than wrapping the remaining body
23 changes: 23 additions & 0 deletions openspec/changes/specify-go-code-standards/tasks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
## 1. Record the capability

- [x] 1.1 Survey what each repository states against what its code does
- [x] 1.2 Write the `go-code-standards` capability
- [x] 1.3 Record the decisions and their rejected alternatives in design.md

## 2. Point each repository at the capability

- [ ] 2.1 `gohai` — `CONTRIBUTING.md` keeps its collector-specific conventions
- [ ] 2.2 `nats-client` — `CONTRIBUTING.md`
- [ ] 2.3 `nats-server` — `CONTRIBUTING.md`
- [ ] 2.4 `osapi-orchestrator` — `CONTRIBUTING.md`
- [ ] 2.5 `osapi` — `CLAUDE.md` drops `Code Standards`, and the conventions
duplicated into `development.md` and `testing.md` resolve to one source

## 3. Verification

- [ ] 3.1 Confirm no `types.go` contains a function
- [ ] 3.2 Confirm no repository holds a generically named file
- [ ] 3.3 Confirm every test package uses a table-driven suite
- [ ] 3.4 Confirm no mock is hand-written where an interface is mocked
- [ ] 3.5 Confirm no `export_test.go` exposes an alias to an unexported function
- [ ] 3.6 Confirm no shared convention is stated in two places