chore: add golangci new-code quality gates (#30622)

Stack context: second layer of the Go lint gate stack, built on the
exclusion fix below.

Why: adds commented-out code detection and a `log/slog` ban to the main
config, plus a new-code-only pass requiring reasons on lint
suppressions, flagging ignored errors (including `_, _ = w.Write(...)`),
and banning `time.Sleep` in tests. Fixing the hits surfaced a real bug:
GitLab warnings went to the wrong logger. The new-code pass resolves
symlinked checkout paths so it does not silently report nothing.

Validation: both configs report 0 issues; each seeded violation is
caught; `make pre-commit` passed.

> Xum acted for Mike to open this PR.
This commit is contained in:
Michael Suchacz
2026-10-13 07:57:56 +00:00
committed by GitHub
parent e8dcf9a3f1
commit 7517301ffa
31 changed files with 115 additions and 131 deletions
+8 -2
View File
@@ -333,8 +333,8 @@ jobs:
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
# Depth 2 makes the PR merge commit's first parent (the base
# branch tip) available so lint/emdash can diff against HEAD^
# without fetching the base branch at runtime.
# branch tip) available so lint/emdash and lint/go/new-code can
# diff against HEAD^ without fetching the base branch at runtime.
fetch-depth: 2
persist-credentials: false
@@ -387,6 +387,12 @@ jobs:
- name: make lint
# Bound concurrent lint targets to keep peak memory below the runner limit.
run: make --output-sync=line -j"$(nproc)" lint
env:
# lint/go/new-code only checks lines changed since this revision.
# Pull requests check out the merge commit, whose first parent is
# the base branch tip (kept by fetch-depth: 2). Other events compare
# HEAD with itself, so the pass reports nothing.
GO_LINT_NEW_CODE_BASE: ${{ github.event_name == 'pull_request' && 'HEAD^1' || 'HEAD' }}
- name: Save golangci-lint cache
# Only the default branch is trusted to write the cache, so PR
+49
View File
@@ -0,0 +1,49 @@
# Lint rules that the existing tree does not pass yet. `make lint/go/new-code`
# runs this config with --new-from-merge-base, so it only reports issues on
# lines changed since the merge base. Keep the full-tree rules in
# .golangci.yaml.
version: "2"
linters:
default: none
enable:
- errcheck
- forbidigo
- nolintlint
settings:
errcheck:
check-blank: true
forbidigo:
analyze-types: true
forbid:
- pattern: ^time\.Sleep$
pkg: ^time$
msg: use a quartz mock clock or testutil.Eventually instead of sleeping in tests
nolintlint:
require-explanation: true
require-specific: true
# Unused directives can only be detected with every linter from
# .golangci.yaml enabled, which this config does not do.
allow-unused: true
exclusions:
generated: lax
presets:
- comments
- common-false-positives
- legacy
- std-error-handling
rules:
- linters:
- errcheck
path: _test\.go
- linters:
- forbidigo
path-except: _test\.go
paths:
- scripts/rules.go
- node_modules
- third_party$
- builtin$
- examples$
issues:
max-issues-per-linter: 0
max-same-issues: 0
+6
View File
@@ -50,6 +50,11 @@ linters:
desc:
aibridge code must not import coder packages outside aibridge;
buildinfo is the only exception
no_stdlib_slog:
list-mode: lax
deny:
- pkg: log/slog
desc: use cdr.dev/slog/v3 instead
dupl:
threshold: 412
exhaustruct:
@@ -67,6 +72,7 @@ linters:
- badRegexp
- boolExprSimplify
- builtinShadowDecl
- commentedOutCode
- commentedOutImport
- deferUnlambda
- dupImport
+17 -2
View File
@@ -781,7 +781,7 @@ gen/docs-manifest: fmt/docs-manifest site/node_modules/.installed | _gen
# GitHub Actions linters are run in a separate CI job (lint-actions) that only
# triggers when workflow files change, so we skip them here when CI=true.
LINT_ACTIONS_TARGETS := $(if $(CI),,lint/actions/actionlint)
lint: lint/shellcheck lint/go lint/ts lint/examples lint/helm lint/site-icons lint/markdown lint/docs-html lint/docs-manifest lint/docs-redirects lint/style-claims lint/check-scopes lint/check-experiment-keys lint/migrations lint/bootstrap lint/architecture lint/emdash lint/agents lint/mise-versions lint/docs-gen-filter $(LINT_ACTIONS_TARGETS)
lint: lint/shellcheck lint/go lint/go/new-code lint/ts lint/examples lint/helm lint/site-icons lint/markdown lint/docs-html lint/docs-manifest lint/docs-redirects lint/style-claims lint/check-scopes lint/check-experiment-keys lint/migrations lint/bootstrap lint/architecture lint/emdash lint/agents lint/mise-versions lint/docs-gen-filter $(LINT_ACTIONS_TARGETS)
.PHONY: lint
# Fast lint subset for lightweight hooks. Some targets use mise-managed tools.
@@ -801,13 +801,28 @@ lint/ts: site/node_modules/.installed
GO_LINT_CONCURRENCY := $(shell n=$$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 1); echo $$(( n < 8 ? n : 8 )))
GO_LINT_MEMLIMIT ?= 8GiB
# --allow-serial-runners makes lint/go and lint/go/new-code wait for each
# other's golangci-lint lock when `make -j lint` starts them together.
lint/go:
./scripts/check_golangci_exclusions.sh
GOMEMLIMIT="$${GOMEMLIMIT:-$(GO_LINT_MEMLIMIT)}" golangci-lint run --concurrency="$(GO_LINT_CONCURRENCY)"
GOMEMLIMIT="$${GOMEMLIMIT:-$(GO_LINT_MEMLIMIT)}" golangci-lint run --allow-serial-runners --concurrency="$(GO_LINT_CONCURRENCY)"
paralleltestctx -custom-funcs="testutil.Context,chatdTestContext" ./...
go run ./scripts/intxcheck ./...
.PHONY: lint/go
# Revision whose merge base with HEAD bounds the lines lint/go/new-code checks.
GO_LINT_NEW_CODE_BASE ?= origin/main
# Rules the existing tree does not pass yet, applied only to changed lines.
# golangci-lint matches changed lines against symlink-resolved file paths
# made relative to PWD, so a checkout reached through a symlink would
# silently report nothing; `pwd -P` resolves the working directory first.
lint/go/new-code:
cd "$$(pwd -P)"
GOMEMLIMIT="$${GOMEMLIMIT:-$(GO_LINT_MEMLIMIT)}" golangci-lint run --allow-serial-runners --concurrency="$(GO_LINT_CONCURRENCY)" \
--config=.golangci.new-code.yaml --new-from-merge-base="$(GO_LINT_NEW_CODE_BASE)"
.PHONY: lint/go/new-code
lint/examples: | _gen/bin/examplegen
_gen/bin/examplegen -lint
.PHONY: lint/examples
@@ -515,7 +515,6 @@ func BenchmarkOpenAI_CreateInterceptor_Responses(b *testing.B) {
messagesPerRequest := 50
requestCount := 100
maxConcurrentRequests := 10
// payloadSizes := []int{2000, 10000, 50000, 100000, 2000000}
payloadSizes := []int{2000000}
for _, payloadSize := range payloadSizes {
for _, stream := range []bool{true, false} {
+1 -1
View File
@@ -95,7 +95,7 @@ func TestStatCPUCmd(t *testing.T) {
s := buf.String()
tmp := clistat.Result{}
require.NoError(t, json.NewDecoder(strings.NewReader(s)).Decode(&tmp))
// require.NotZero(t, tmp.Used) // Host CPU can sometimes be zero.
// tmp.Used is not asserted because host CPU usage can be zero.
require.NotNil(t, tmp.Total)
require.NotZero(t, *tmp.Total)
require.Equal(t, "cores", tmp.Unit)
+1 -27
View File
@@ -107,35 +107,9 @@ func TestUserDelete(t *testing.T) {
stdout.ExpectMatch(ctx, "coolin")
})
// TODO: reenable this test case. Fetching users without perms returns a
// TODO: add a NoPerms test case. Fetching users without perms returns a
// "user "testuser@coder.com" must be a member of at least one organization"
// error.
// t.Run("NoPerms", func(t *testing.T) {
// t.Parallel()
// ctx := context.Background()
// client := coderdtest.New(t, nil)
// aUser := coderdtest.CreateFirstUser(t, client)
// pw, err := cryptorand.String(16)
// require.NoError(t, err)
// toDelete, err := client.CreateUserWithOrgs(ctx, codersdk.CreateUserRequestWithOrgs{
// Email: "colin5@coder.com",
// Username: "coolin",
// Password: pw,
// UserLoginType: codersdk.LoginTypePassword,
// OrganizationID: aUser.OrganizationID,
// })
// require.NoError(t, err)
// uClient, _ := coderdtest.CreateAnotherUser(t, client, aUser.OrganizationID)
// _ = uClient
// _ = toDelete
// inv, root := clitest.New(t, "users", "delete", "coolin")
// clitest.SetupConfig(t, uClient, root)
// require.ErrorContains(t, inv.Run(), "...")
// })
t.Run("DeleteSelf", func(t *testing.T) {
t.Parallel()
-1
View File
@@ -344,7 +344,6 @@ func TestUpdateStats(t *testing.T) {
// need to overwrite the cached fields for this test, but the struct has a lock
ws := agentapi.CachedWorkspaceFields{}
ws.UpdateValues(workspace)
// ws.AutostartSchedule = workspace.AutostartSchedule
api := agentapi.StatsAPI{
AgentID: agent.ID,
-3
View File
@@ -68,9 +68,6 @@ type (
// has not expired, and belongs to a non-deleted, non-system user. It does not
// verify the key secret, because the caller never has it.
func WithDelegatedAPIKeyID(ctx context.Context, id string) context.Context {
/*
return WithDelegatedRequest(ctx, id, Attribution{})
*/
return context.WithValue(ctx, deletedAPIKeyIDCtxKey{}, id)
}
+1 -1
View File
@@ -102,7 +102,7 @@ func TestComputeCost(t *testing.T) {
price: database.AIModelPrice{
InputPrice: nullInt64(999),
},
inputTokens: 1002, // 1002 * 999 = 1_000_998
inputTokens: 1002, // 1002 times 999 is 1_000_998.
want: 1,
},
{
@@ -89,7 +89,6 @@ func TestCompressorEncodings(t *testing.T) {
ts := httptest.NewServer(compressor)
defer ts.Close()
// ctx := testutil.Context(t, testutil.WaitShort)
ctx := context.Background()
header, respString := testRequestWithAcceptedEncodings(ctx, t, ts, "GET", tc.path, tc.acceptedEncodings...)
if respString != "textstring" {
-1
View File
@@ -1450,7 +1450,6 @@ func New(options *Options) *API {
r.Get("/regions", api.regions)
})
r.Route("/derp-map", func(r chi.Router) {
// r.Use(apiKeyMiddleware)
r.Get("/", api.derpMapUpdates)
})
r.Route("/deployment", func(r chi.Router) {
+1 -1
View File
@@ -4,7 +4,7 @@ import (
"context"
"database/sql"
"io/fs"
"log/slog"
"log/slog" //nolint:depguard // preview.Input.Logger requires a *log/slog.Logger.
"sync"
"time"
+1 -1
View File
@@ -212,7 +212,7 @@ func (c *Config) Git() (gitprovider.Provider, error) {
if c.gitProvider != nil {
return c.gitProvider, nil
}
p, err := gitprovider.New(norm, c.APIBaseURL, c.HTTPClient)
p, err := gitprovider.New(norm, c.APIBaseURL, c.HTTPClient, gitprovider.WithLogger(c.Logger.Named("gitprovider")))
if err != nil {
return nil, err
}
+8 -6
View File
@@ -7,7 +7,6 @@ import (
"errors"
"fmt"
"io"
"log/slog"
"net/http"
"net/url"
"strconv"
@@ -16,6 +15,7 @@ import (
gitlab "gitlab.com/gitlab-org/api/client-go"
"golang.org/x/xerrors"
"cdr.dev/slog/v3"
"github.com/coder/quartz"
)
@@ -23,9 +23,10 @@ type gitlabProvider struct {
webBaseURL string
client *gitlab.Client
clock quartz.Clock
logger slog.Logger
}
func newGitLab(baseURL string, httpClient *http.Client, clock quartz.Clock) (Provider, error) {
func newGitLab(baseURL string, httpClient *http.Client, clock quartz.Clock, logger slog.Logger) (Provider, error) {
if baseURL == "" {
baseURL = "https://gitlab.com"
}
@@ -48,6 +49,7 @@ func newGitLab(baseURL string, httpClient *http.Client, clock quartz.Clock) (Pro
webBaseURL: baseURL,
client: client,
clock: clock,
logger: logger,
}, nil
}
@@ -388,10 +390,10 @@ func (g *gitlabProvider) FetchBranchDiff(
sb.Grow(estimated)
for _, d := range compare.Diffs {
if d.Collapsed || d.TooLarge {
slog.WarnContext(ctx, "gitlab compare: file diff truncated",
slog.String("path", d.NewPath),
slog.Bool("collapsed", d.Collapsed),
slog.Bool("too_large", d.TooLarge),
g.logger.Warn(ctx, "gitlab compare: file diff truncated",
slog.F("path", d.NewPath),
slog.F("collapsed", d.Collapsed),
slog.F("too_large", d.TooLarge),
)
}
_, _ = fmt.Fprintf(&sb, "diff --git a/%s b/%s\n", d.OldPath, d.NewPath)
+12 -2
View File
@@ -11,13 +11,15 @@ import (
"golang.org/x/xerrors"
"cdr.dev/slog/v3"
"github.com/coder/quartz"
)
// providerOptions holds optional configuration for provider
// construction.
type providerOptions struct {
clock quartz.Clock
clock quartz.Clock
logger slog.Logger
}
// Option configures optional behavior for a Provider.
@@ -31,6 +33,14 @@ func WithClock(c quartz.Clock) Option {
}
}
// WithLogger sets the logger used by the provider. Defaults to a
// logger that discards all output.
func WithLogger(l slog.Logger) Option {
return func(o *providerOptions) {
o.logger = l
}
}
// PRState is the normalized state of a pull/merge request across
// all providers.
type PRState string
@@ -195,7 +205,7 @@ func New(providerType string, apiBaseURL string, httpClient *http.Client, opts .
case "github":
return newGitHub(apiBaseURL, httpClient, o.clock)
case "gitlab":
return newGitLab(apiBaseURL, httpClient, o.clock)
return newGitLab(apiBaseURL, httpClient, o.clock, o.logger)
default:
// Other providers (bitbucket-cloud, etc.) will be
// added here as they are implemented.
+1 -1
View File
@@ -179,7 +179,7 @@ func TestOrganizationParam(t *testing.T) {
org := httpmw.OrganizationParam(r)
assert.NotZero(t, org)
assert.NotZero(t, org.CreatedAt)
// assert.NotZero(t, org.Description) // not supported
// org.Description is not asserted because it is not supported.
assert.NotZero(t, org.ID)
assert.NotEmpty(t, org.Name)
orgMem := httpmw.OrganizationMemberParam(r)
+1 -1
View File
@@ -4,7 +4,7 @@ import (
"context"
"encoding/json"
"errors"
stdslog "log/slog"
stdslog "log/slog" //nolint:depguard // mcp.ServerOptions.Logger requires a *log/slog.Logger; slogHandler forwards to cdr.dev/slog.
"net/http"
"github.com/modelcontextprotocol/go-sdk/mcp"
@@ -495,8 +495,7 @@ func TestReportFailedWorkspaceBuilds(t *testing.T) {
// Setup
ctx, logger, db, ps, notifEnq, clk := setup(t)
// Given
// Organization
// Given an organization
org := dbgen.Organization(t, db, database.Organization{})
// Template admins
@@ -775,7 +775,7 @@ func TestAgentStats(t *testing.T) {
"coderd_agentstats_session_count_ssh",
"coderd_agentstats_session_count_vscode":
for _, m := range metric.Metric {
// username:workspace:agent:metric = value
// Keys have the form username:workspace:agent:metric.
collected[m.Label[1].GetValue()+":"+m.Label[2].GetValue()+":"+m.Label[0].GetValue()+":"+metric.GetName()] = int(m.Gauge.GetValue())
}
default:
@@ -1979,8 +1979,7 @@ func TestCompleteJob(t *testing.T) {
Source: "test-source2",
Resource: "test-resource2",
Action: "test-action2",
// Start: omitted
// End: omitted
// Start and End are omitted.
},
{
Stage: "test3",
+1 -1
View File
@@ -10,7 +10,7 @@ import (
"errors"
"fmt"
"io/fs"
stdslog "log/slog"
stdslog "log/slog" //nolint:depguard // preview.Input.Logger requires a *log/slog.Logger.
"net/http"
"os"
-6
View File
@@ -1868,12 +1868,6 @@ func (api *API) postWorkspaceUsage(rw http.ResponseWriter, r *http.Request) {
return
}
// template, err := api.Database.GetTemplateByID(ctx, workspace.TemplateID)
// if err != nil {
// httpapi.InternalServerError(rw, err)
// return
// }
err = api.statsReporter.ReportAgentStats(ctx, dbtime.Now(), database.WorkspaceIdentityFromWorkspace(workspace), agent.ID, agent.Name, stat, true)
if err != nil {
httpapi.InternalServerError(rw, err)
+1
View File
@@ -126,6 +126,7 @@ func TestEstimatePromptSize(t *testing.T) {
}
size := chatloop.EstimatePromptSize(messages)
// The estimate sums the text lengths:
// "You are a helpful assistant." (28) + "Hello world" (11) +
// "thinking..." (11) + "filedata" (8) +
// "Hi there!" (9) + `{"file":"main.go"}` (18) +
+2 -2
View File
@@ -866,7 +866,7 @@ func TestInjectMissingToolUses_DropsProviderExecutedOrphans(t *testing.T) {
resultC := mustMarshalToolResult(t,
"srvtoolu_C", "web_search",
json.RawMessage(`{}`),
false, false, true, // provider_executed = true
false, false, true, // provider_executed is true.
)
resultD := mustMarshalToolResult(t,
"toolu_D", "wait_agent",
@@ -1112,7 +1112,7 @@ func TestProviderExecutedResult_LegacyToolRow(t *testing.T) {
peResult := mustMarshalToolResult(t,
"srvtoolu_WS", "web_search",
json.RawMessage(`{"results":"cached"}`),
false, false, true, // providerExecuted = true
false, false, true, // providerExecuted is true.
)
execResult := mustMarshalToolResult(t,
"toolu_exec", "execute",
-59
View File
@@ -173,65 +173,6 @@ func Test_diffValues(t *testing.T) {
},
})
})
// We currently don't support nested structs.
// t.Run("NestedStruct", func(t *testing.T) {
// t.Parallel()
// type bar struct {
// Baz string `json:"baz"`
// }
// type foo struct {
// Bar *bar `json:"bar"`
// }
// table := auditMap(map[any]map[string]Action{
// &foo{}: {
// "bar": ActionTrack,
// },
// &bar{}: {
// "baz": ActionTrack,
// },
// })
// runDiffValuesTests(t, table, []diffTest{
// {
// name: "LeftEmpty",
// left: foo{Bar: &bar{}}, right: foo{Bar: &bar{Baz: "baz"}},
// exp: audit.Map{
// "bar": audit.Map{
// "baz": audit.OldNew{Old: "", New: "baz"},
// },
// },
// },
// {
// name: "RightEmpty",
// left: foo{Bar: &bar{Baz: "baz"}}, right: foo{Bar: &bar{}},
// exp: audit.Map{
// "bar": audit.Map{
// "baz": audit.OldNew{Old: "baz", New: ""},
// },
// },
// },
// {
// name: "LeftNil",
// left: foo{Bar: nil}, right: foo{Bar: &bar{}},
// exp: audit.Map{
// "bar": audit.Map{},
// },
// },
// {
// name: "RightNil",
// left: foo{Bar: &bar{Baz: "baz"}}, right: foo{Bar: nil},
// exp: audit.Map{
// "bar": audit.Map{
// "baz": audit.OldNew{Old: "baz", New: ""},
// },
// },
// },
// })
// })
}
type diffTest struct {
@@ -3623,8 +3623,6 @@ func TestAIGovernanceAddon(t *testing.T) {
aibridgeFeature := entitlements.Features[codersdk.FeatureAIBridge]
require.True(t, aibridgeFeature.Enabled, "AI Bridge should be enabled when addon is present and enablements are set")
// require.Equal(t, codersdk.EntitlementEntitled, aibridgeFeature.Entitlement, "AI Bridge should be entitled when addon is present")
// TODO: Readd this test once Boundary is enforced as an add-on license.
// boundaryFeature := entitlements.Features[codersdk.FeatureBoundary]
// require.True(t, boundaryFeature.Enabled, "Boundary should be enabled when addon is present and enablements are set")
@@ -135,7 +135,6 @@ func ingestAllSpans(t *testing.T, input []byte, aggregator *timingAggregator) {
ts, span, err := extractTimingSpan(log)
if err != nil {
// t.Logf("%s: failed span extraction on line: %q", err, line)
continue
}
-1
View File
@@ -176,7 +176,6 @@ func Test_TimeoutExecutionStrategy(t *testing.T) {
//nolint:paralleltest // this tests uses timings to determine if it's working
func Test_ShuffleExecutionStrategyWrapper(t *testing.T) {
runs, fns := strategyTestData(100000, func(_ context.Context, i int, _ io.Writer) error {
// t.Logf("run %d", i)
return nil
})
strategy := harness.ShuffleExecutionStrategyWrapper{
+1 -1
View File
@@ -400,7 +400,7 @@ func collectDecls(file *ast.File, packageStrings map[string]map[string]string) d
// String literal: const name = "value"
decls.strings[name.Name] = strings.Trim(v.Value, `"`)
case *ast.BinaryExpr:
// Concatenation: const name = prefix + "suffix"
// String concatenation: const name = prefix + "suffix"
if resolved := resolveBinaryExpr(v, decls); resolved != "" {
decls.strings[name.Name] = resolved
}
-1
View File
@@ -131,7 +131,6 @@ func populateValue(v reflect.Value, r *Random) (reflect.Value, error) {
s.Index(0).Set(sv)
s.Index(1).Set(sv)
// reflect.AppendSlice(s, sv)
return s, nil
case reflect.Uintptr, reflect.UnsafePointer, reflect.Chan, reflect.Func, reflect.Interface: