From 2d40c466286e004f4b0338e83bb3bfb3f3c853aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D7=A0=CF=85=CE=B1=CE=B7=20=D7=A0=CF=85=CE=B1=CE=B7=D1=95?= =?UTF-8?q?=CF=83=CE=B7?= Date: Sat, 15 Aug 2026 23:26:43 -0700 Subject: [PATCH] feat: record how Go is written across the organization Five repositories state the same conventions in five files, with nothing keeping them in agreement. Two rules are already universal in practice but written down in only two or three of the repositories that follow them. Measured against the code rather than against the other documents, which is what surfaced the gap: comparing documents finds what everyone wrote, comparing documents to code finds what everyone does. Co-Authored-By: Claude Opus 5 (1M context) --- .../specify-go-code-standards/.openspec.yaml | 2 + .../specify-go-code-standards/design.md | 112 ++++++++++++++ .../specify-go-code-standards/proposal.md | 46 ++++++ .../specs/go-code-standards/spec.md | 146 ++++++++++++++++++ .../specify-go-code-standards/tasks.md | 23 +++ 5 files changed, 329 insertions(+) create mode 100644 openspec/changes/specify-go-code-standards/.openspec.yaml create mode 100644 openspec/changes/specify-go-code-standards/design.md create mode 100644 openspec/changes/specify-go-code-standards/proposal.md create mode 100644 openspec/changes/specify-go-code-standards/specs/go-code-standards/spec.md create mode 100644 openspec/changes/specify-go-code-standards/tasks.md 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