diff --git a/openspec/changes/specify-go-code-standards/.openspec.yaml b/openspec/changes/specify-go-code-standards/.openspec.yaml new file mode 100644 index 0000000..0c73c8f --- /dev/null +++ b/openspec/changes/specify-go-code-standards/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-15 diff --git a/openspec/changes/specify-go-code-standards/design.md b/openspec/changes/specify-go-code-standards/design.md new file mode 100644 index 0000000..b082ae9 --- /dev/null +++ b/openspec/changes/specify-go-code-standards/design.md @@ -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. diff --git a/openspec/changes/specify-go-code-standards/proposal.md b/openspec/changes/specify-go-code-standards/proposal.md new file mode 100644 index 0000000..fc0c60a --- /dev/null +++ b/openspec/changes/specify-go-code-standards/proposal.md @@ -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. diff --git a/openspec/changes/specify-go-code-standards/specs/go-code-standards/spec.md b/openspec/changes/specify-go-code-standards/specs/go-code-standards/spec.md new file mode 100644 index 0000000..786ca4c --- /dev/null +++ b/openspec/changes/specify-go-code-standards/specs/go-code-standards/spec.md @@ -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 diff --git a/openspec/changes/specify-go-code-standards/tasks.md b/openspec/changes/specify-go-code-standards/tasks.md new file mode 100644 index 0000000..5fbb332 --- /dev/null +++ b/openspec/changes/specify-go-code-standards/tasks.md @@ -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