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
34 changes: 23 additions & 11 deletions openspec/changes/specify-go-code-standards/tasks.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,21 @@

## 2. Point each repository at the capability

The capability has to exist in the corpus before a repository can rely on the
pointer alone. It did not until task 2.0 synced it, so the four conversions
below kept their local copy and the pointer led nowhere.

- [x] 2.0 `specs` — sync `go-code-standards` into `openspec/specs/` so the
pointer resolves. Every repository named it as the source while it existed
only inside this change, where a reader following the link would not find it
- [x] 2.1 `gohai` — `CONTRIBUTING.md` keeps its collector-specific conventions
- [x] 2.2 `nats-client` — `CONTRIBUTING.md`
- [x] 2.3 `nats-server` — `CONTRIBUTING.md`
- [x] 2.4 `osapi-orchestrator` — `CONTRIBUTING.md`
and drops the shared ones (osapi-io/gohai#163)
- [ ] 2.2 `nats-client` — `CONTRIBUTING.md` still restates `Function signatures`
and `Go patterns` under `Code style`
- [ ] 2.3 `nats-server` — `CONTRIBUTING.md` still restates `Function signatures`
and `Go patterns` under `Code style`
- [ ] 2.4 `osapi-orchestrator` — `CONTRIBUTING.md`
(osapi-io/osapi-orchestrator#76, open)
- [x] 2.5 `osapi` — `CLAUDE.md` dropped `Code Standards`, and the conventions
duplicated into `development.md` and `testing.md` now resolve to the root
`CONTRIBUTING.md`, which points at this capability rather than restating it
Expand All @@ -35,11 +46,12 @@
files exist (32 in `gohai`, 32 in `osapi`, 1 in `osapi-orchestrator`), and the
requirement turns on what each exposure is *for*, which no search can decide.
This needs a file-by-file audit
- [ ] 3.6 Confirm no shared convention is stated in two places. `nats-client`
and `nats-server` point at the capability without restating it, and `osapi`
now does the same. `gohai` and `osapi-orchestrator` point at it *and* restate
it — `osapi-orchestrator` under `Function Signatures`, `Testing`,
`Go Patterns`, and `Linting`; `gohai` under `Function Signatures`. Both say
the specification wins where they disagree, which acknowledges the duplication
rather than removing it. Resolving this means deleting the restatements, which
reverses a deliberate choice recorded when those repositories were converted
- [ ] 3.6 Confirm no shared convention is stated in two places. All four Go
libraries restated the capability rather than only pointing at it, each
closing with "the specification wins where they disagree" — which acknowledges
the duplication instead of removing it. Their conversions under tasks 2.1 to
2.4 did half of what design.md's migration asks: they added the pointer and
kept the copy. `osapi` points without restating (osapi-io/osapi#450), and
`gohai` now does too (osapi-io/gohai#163). `osapi-orchestrator` is in flight;
`nats-client` and `nats-server` restate under `Code style` as
`Function signatures` and `Go patterns`, and have no pull request yet
171 changes: 171 additions & 0 deletions openspec/specs/go-code-standards/spec.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
# go-code-standards Specification

## 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.

## 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: A suite is named for the surface it tests

A suite exercising the exported surface SHALL be named `{Name}PublicTestSuite`.
A suite exercising a package's internals SHALL be named `{Name}TestSuite`.

The name states which surface the suite covers, so a reader knows without
opening the file whether it tests the package as a consumer sees it.

A suite that is neither — an integration suite driving a built binary — is named
for what it drives rather than forced into this pair.

#### Scenario: A reader looks for the tests of an exported function

- **WHEN** a reader wants the tests covering a package's exported surface
- **THEN** the suite name identifies them, rather than requiring each file to be
opened

### 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: A test does not duplicate coverage through an internal seam

Where a test needs access to an unexported symbol, the package MAY provide an
`export_test.go` exposing it — by alias or by setter. The file carries the
`_test.go` suffix, so nothing it declares ships in the built package.

What the exposure is for matters more than its form. An exported alias SHALL NOT
be used to test an internal step whose behavior the caller's own test already
covers. Such a test adds no coverage and pins an implementation detail, so
changing how the caller reaches its result means rewriting tests that were never
about the result.

A seam replacing a dependency SHALL be placed at the boundary with that
dependency, not partway through the code under test, so 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 is exported for a test

- **WHEN** an unexported function is exposed through `export_test.go`
- **THEN** the test exercises behavior the caller's test does not already reach,
rather than re-covering the same path through a shorter route

#### Scenario: An unexported helper is pure and self-contained

- **WHEN** an unexported function has its own contract, independent of the
callers that use it
- **THEN** exposing and testing it directly is appropriate

### 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