Add the shared Go lint policy and make kit follow it - #87
Conversation
Every kenn-io Go repository carried its own golangci-lint configuration.
Nineteen configs had drifted apart: enabled linters ranged from two to sixty,
pinned versions spanned five releases, and the testify helper analyzer had
been copied into six repositories with divergent edits. Agents moving between
repositories kept re-learning the same rules, and the failures that reviewers
and CI caught late were the ones no repository enforced consistently: stdlib
test assertions, wall-clock sleeps in tests, error identity decided by
matching err.Error() text, context-free calls, and raw net/http route
registration.
golangci-lint has no configuration inheritance, so the shared policy lives in
kit as a canonical file plus a renderer. A repository commits only an overlay
with its local additions, generates .golangci.yml from the two, and a drift
check keeps the committed file honest. The custom analyzers ship as a
golangci-lint module plugin so //nolint and path exclusions work like any other
linter, and kennlint also runs them directly for editors and repositories
without a custom build.
Migrations kept as .sql files are outside golangci-lint's reach, so the enum
CHECK constraint check (CHECK (status IN ('queued', 'done'))) also runs as a
standalone scanner. Those constraints turn every new value into a schema
migration that rewrites the constraint; the allowed set belongs in application
code or a lookup table.
Kit itself now lints clean against the full policy. Most of the roughly two
thousand pre-existing findings were converted mechanically; the few
suppressions that remain each carry a reason, which the policy now requires.
Two consequences for callers: Endpoint.Listen takes a context so the listener
is created through net.ListenConfig, and the testify helper analyzer accepts
any variable bound to assert.New(t) or require.New(t), because the convention
of shadowing the package cannot express a nested subtest that needs its own
helper.
Generated with Claude Code
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
roborev: Combined Review (
|
roborev: Combined Review (
|
CI lints the merge with main on Linux, which exposed findings in files my macOS run never compiled: the newly merged huma-check tool, the Linux-only daemon and packstore tests, and the Windows-only sources. Bring all of them under the policy and check the Windows build too, so the lint stays green on every platform kit builds for. The S3 conformance test cleaned its prefix from a t.Cleanup closure using the test context, which is already cancelled by then; run cleanup under context.WithoutCancel so the delete calls actually reach the service. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Surveying the downstream migration directories showed that most hard-coded value sets are not bare `col IN (...)` expressions: they hide behind a nullable prefix (`col IS NULL OR col IN (...)`), sit inside a larger AND/OR expression as `NOT IN`, or spell the set out one state per branch (`(state = 'a' AND ...) OR (state = 'b' AND ...)`). Every one of those still forces a migration when a value is added, which is the thing the check exists to prevent, so match the enum shapes anywhere inside the expression instead of requiring the whole constraint to be one. Also recognize the PostgreSQL `= ANY (ARRAY[...])` spelling and `CREATE TYPE ... AS ENUM`, which locks the set in the same way. Single-literal invariants, range and length checks, function-derived subjects such as SUBSTR(...), and subqueries stay unflagged; the negative table test pins that down. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d bubbles Endpoint.Listen gained a context parameter to satisfy noctx, which broke every downstream caller for a lint-policy change. Keep Listen() and add ListenContext for callers that have a context. The SQL scanner searched raw text, so CHECK constraints quoted in comments or string literals were reported. Mask line comments, block comments, and single-quoted strings before locating keywords, keeping offsets intact. sleeptest only exempted function literals written inline in synctest.Test; callbacks passed by name (declared functions or function-valued variables) were reported even though they run inside the bubble. Resolve identifiers to their bodies and exempt those too. Fix the analyzer count in the adoption guide and state the Go version the policy assumes, since some remediations need Go 1.26 APIs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
roborev: Combined Review (
|
The point of the SQL check is to keep validation rules out of the schema, because any rule baked into a CHECK needs a migration to change. Matching only value-list shapes let range checks and cross-column invariants through, and in the downstream migrations most of those exist to police a set of states anyway. Report every CHECK (...) and CREATE TYPE ... AS ENUM outside comments and string literals, and rename the analyzer to sqlcheck since it is no longer about enums. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
roborev: Combined Review (
|
The SQL analyzer treated prose and PostgreSQL lexical regions as schema constraints, and comment parentheses could hide real constraints. Limit Go literals to DDL and match SQL structure against masked text so diagnostics track schema statements. A named synctest callback is safe only when every use enters synctest.Test. Keep mixed-use callbacks reportable, and compare YAML mappings without treating key order as meaningful. Generated with Codex Co-authored-by: Codex <noreply@openai.com>
roborev: Combined Review (
|
…ixtures The analyzer already reported time.Sleep in _test.go files outside a synctest.Test bubble. Close the gaps against the issue's acceptance list: also treat synctest.Run as a bubble, check test helper packages (testutil or a name ending in "test") by default, and add an off-by-default check for testify Eventually, EventuallyWithT, and Never, which poll the wall clock the same way. Both switches are exposed as analyzer flags and plugin settings. Fixtures now pin every acceptance case: sleep in a goroutine inside a bubble (not reported), sleep through a package alias (reported), sleep in a helper package (reported), and a helper called from a bubble (reported; documented limitation). Running the analyzer alone over a downstream repository reports its pre-migration sleeps and nothing inside a bubble. Closes kit#gzyk. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
1 participant
Kit now owns the Go lint policy for kenn-io repositories, and kit itself lints clean against it.
Nineteen repositories had drifted to nineteen golangci-lint configurations, with enabled linters ranging from two to sixty, five different pinned versions, and the testify helper analyzer copy-pasted into six repositories with divergent edits. The failures that reviewers and CI caught late were exactly the ones nobody enforced consistently: stdlib assertions in tests, wall-clock sleeps, error identity decided by matching
err.Error()text, context-free subprocess and network calls, and rawnet/httproute registration.golangci-lint has no configuration inheritance, so
lint/configholds the canonical file andkennlint configrenders a repository's.golangci.ymlfrom it plus a small overlay of local additions;-checkfails CI when the committed file is stale. The five analyzers (testifyhelper,sleeptest,errtext,nohttpmux,sqlcheck) ship as thekennlintgolangci-lint module plugin so//nolintand path exclusions behave like any other linter.kennlint runexecutes them directly for editors, andkennlint sqlapplies theCHECKconstraint check to.sqlmigration files that golangci-lint cannot see.docs/adopting-kennlint.mdcovers adoption and staged rollout.Review effort concentrates in a few places; the rest of the diff is mechanical migration of kit's tests and code to the policy.
lint/configdisableprunes) and the linter set.contextcheckandcontainedctxwere left out because kit's cleanup contexts and ctx-carrying readers are deliberate; gocritic is limited to diagnostic checks.lint/sqlcheckCHECKconstraint andCREATE TYPE ... AS ENUMoutside comments and string literals. Any rule baked into a CHECK needs a migration to change, and in the downstream migrations nearly all of them police a set of states, so there are no shape-based exemptions. Validate in application code or use a lookup table.lint/sleeptestsynctest.Test, inline or by name; helpers called from a bubble are reported on purpose. Test helper packages (testutil,*test) are checked by default; the testifyEventuallycheck is opt-in per repository. Run alone over Forge it reports the 51 pre-migration sleeps and nothing inside a bubble.lint/testifyhelperassert.New(t)orrequire.New(t), because shadowing the package cannot express a nested subtest that needs its own helper.daemon/endpoint.goEndpoint.Listenkeeps its signature; a newListenContextcreates the listener throughnet.ListenConfigand kit's own callers use it, so downstream callers ofListenare unaffected.//nolintsitesProof's value-receiverFormatthat keeps redaction working.One trap surfaced during migration and is documented for other repositories:
t.Context()is already cancelled insidet.Cleanup, and helper subprocesses started with it are killed at cleanup, so those sites derivecontext.WithoutCancel(t.Context()).🤖 Generated with Claude Code