Add the shared Go lint policy and make kit follow it - #87
mariusvniekerk wants to merge 5 commits into
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 (
|
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,sqlenum) ship as thekennlintgolangci-lint module plugin so//nolintand path exclusions behave like any other linter.kennlint runexecutes them directly for editors, andkennlint sqlapplies the enumCHECKconstraint 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/sqlenumCHECKexpression (col IN (...),col IS NULL OR col IN (...), nestedNOT IN, per-branchstate = 'a' ... OR state = 'b' ..., PostgreSQL= ANY (ARRAY[...])) plusCREATE TYPE ... AS ENUM. A survey of the downstream migration directories showed most hard-coded sets use the nullable or per-branch forms rather than a bareINlist. Range checks, single-literal invariants, and subqueries stay unflagged.lint/testifyhelperassert.New(t)orrequire.New(t), because shadowing the package cannot express a nested subtest that needs its own helper.daemon/endpoint.goEndpoint.Listentakes a context so the listener is created throughnet.ListenConfig. Its only caller already had one.//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