chore: clean up stale and dangerous //nolint comments (#23643)

## Changes

- **Commit 1**: Remove 17 unnecessary `//nolint` directives:
  - `//nolint:varnamelen` — linter not active
  - `//nolint:unused` on exported `SlimUnsupported`
  - `//nolint:govet` in `coderd/httpmw/csrf` — no longer fires
  - `//nolint:revive` on functions refactored since the nolint was added
- `//nolint:paralleltest` citing Go 1.22 loop variable capture
(obsolete)
- Bare `//nolint` narrowed to specific `//nolint:gocritic` with
justification

- **Commit 2**: Fix root causes behind 5 dangerous nolint suppressions:
- Add `MinVersion: tls.VersionTLS12` to TLS client config (removes
`gosec` G402)
- Delete trivial unexported wrappers `apiKey()`/`normalizeProvider()` in
chatprovider (removes `revive` confusing-naming)
- Add doc comments to `StartWithAssert` and `Router` (removes `revive`
exported)
  - Rename unused parameters to `_` in integration test helpers

> 🤖 This PR was created using Coder Agents and reviewed by me.

agent/agentcontainers/containers_internal_test.go

@@ -159,7 +159,6 @@ func TestConvertDockerVolume(t *testing.T) {
 func TestConvertDockerInspect(t *testing.T) {
 	t.Parallel()
 
-	//nolint:paralleltest // variable recapture no longer required
 	for _, tt := range []struct {
 		name        string
 		expect      []codersdk.WorkspaceAgentContainer
@@ -388,7 +387,6 @@ func TestConvertDockerInspect(t *testing.T) {
 			},
 		},
 	} {
-		// nolint:paralleltest // variable recapture no longer required
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 			bs, err := os.ReadFile(filepath.Join("testdata", tt.name, "docker_inspect.json"))

agent/agentcontainers/containers_test.go

@@ -166,7 +166,6 @@ func TestDockerEnvInfoer(t *testing.T) {
 
 	pool, err := dockertest.NewPool("")
 	require.NoError(t, err, "Could not connect to docker")
-	// nolint:paralleltest // variable recapture no longer required
 	for idx, tt := range []struct {
 		image             string
 		labels            map[string]string
@@ -223,7 +222,6 @@ func TestDockerEnvInfoer(t *testing.T) {
 			expectedUserShell: "/bin/bash",
 		},
 	} {
-		//nolint:paralleltest // variable recapture no longer required
 		t.Run(fmt.Sprintf("#%d", idx), func(t *testing.T) {
 			// Start a container with the given image
 			// and environment variables

cli/clitest/clitest.go

@@ -173,7 +173,10 @@ func Start(t *testing.T, inv *serpent.Invocation) {
 	StartWithAssert(t, inv, nil)
 }
 
-func StartWithAssert(t *testing.T, inv *serpent.Invocation, assertCallback func(t *testing.T, err error)) { //nolint:revive
+// StartWithAssert starts the given invocation and calls assertCallback
+// with the resulting error when the invocation completes. If assertCallback
+// is nil, expected shutdown errors are silently tolerated.
+func StartWithAssert(t *testing.T, inv *serpent.Invocation, assertCallback func(t *testing.T, err error)) {
 	t.Helper()
 
 	closeCh := make(chan struct{})

cli/cliui/select.go

@@ -173,7 +173,6 @@ func (selectModel) Init() tea.Cmd {
 	return nil
 }
 
-//nolint:revive // The linter complains about modifying 'm' but this is typical practice for bubbletea
 func (m selectModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 	var cmd tea.Cmd
 
@@ -463,7 +462,6 @@ func (multiSelectModel) Init() tea.Cmd {
 	return nil
 }
 
-//nolint:revive // For same reason as previous Update definition
 func (m multiSelectModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 	var cmd tea.Cmd
 

cli/root.go

@@ -1414,7 +1414,6 @@ func tailLineStyle() pretty.Style {
 	return pretty.Style{pretty.Nop}
 }
 
-//nolint:unused
 func SlimUnsupported(w io.Writer, cmd string) {
 	_, _ = fmt.Fprintf(w, "You are using a 'slim' build of Coder, which does not support the %s subcommand.\n", pretty.Sprint(cliui.DefaultStyles.Code, cmd))
 	_, _ = fmt.Fprintln(w, "")

cli/server.go

@@ -305,7 +305,6 @@ func enablePrometheus(
 	}
 	options.ProvisionerdServerMetrics = provisionerdserverMetrics
 
-	//nolint:revive
 	return ServeHandler(
 		ctx, logger, promhttp.InstrumentMetricHandler(
 			options.PrometheusRegistry, promhttp.HandlerFor(options.PrometheusRegistry, promhttp.HandlerOpts{}),
@@ -1637,8 +1636,6 @@ var defaultCipherSuites = func() []uint16 {
 // configureServerTLS returns the TLS config used for the Coderd server
 // connections to clients. A logger is passed in to allow printing warning
 // messages that do not block startup.
-//
-//nolint:revive
 func configureServerTLS(ctx context.Context, logger slog.Logger, tlsMinVersion, tlsClientAuth string, tlsCertFiles, tlsKeyFiles []string, tlsClientCAFile string, ciphers []string, allowInsecureCiphers bool) (*tls.Config, error) {
 	tlsConfig := &tls.Config{
 		MinVersion: tls.VersionTLS12,
@@ -2055,7 +2052,6 @@ func getGithubOAuth2ConfigParams(ctx context.Context, db database.Store, vals *c
 	return &params, nil
 }
 
-//nolint:revive // Ignore flag-parameter: parameter 'allowEveryone' seems to be a control flag, avoid control coupling (revive)
 func configureGithubOAuth2(instrument *promoauth.Factory, params *githubOAuth2ConfigParams) (*coderd.GithubOAuth2Config, error) {
 	redirectURL, err := params.accessURL.Parse("/api/v2/users/oauth2/github/callback")
 	if err != nil {

cli/server_test.go

@@ -2123,7 +2123,6 @@ func TestServer_TelemetryDisable(t *testing.T) {
 	// Set the default telemetry to true (normally disabled in tests).
 	t.Setenv("CODER_TEST_TELEMETRY_DEFAULT_ENABLE", "true")
 
-	//nolint:paralleltest // No need to reinitialise the variable tt (Go version).
 	for _, tt := range []struct {
 		key  string
 		val  string

cli/templateedit_test.go

@@ -828,7 +828,7 @@ func TestTemplateEdit(t *testing.T) {
 			"--require-active-version",
 		}
 		inv, root := clitest.New(t, cmdArgs...)
-		//nolint
+		//nolint:gocritic // Using owner client is required for template editing.
 		clitest.SetupConfig(t, client, root)
 
 		ctx := testutil.Context(t, testutil.WaitLong)
@@ -858,7 +858,7 @@ func TestTemplateEdit(t *testing.T) {
 			"--name", "something-new",
 		}
 		inv, root := clitest.New(t, cmdArgs...)
-		//nolint
+		//nolint:gocritic // Using owner client is required for template editing.
 		clitest.SetupConfig(t, client, root)
 
 		ctx := testutil.Context(t, testutil.WaitLong)

coderd/agentapi/connectionlog_test.go

@@ -101,7 +101,6 @@ func TestConnectionLog(t *testing.T) {
 			reason: "because error says so",
 		},
 	}
-	//nolint:paralleltest // No longer necessary to reinitialise the variable tt.
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()

coderd/httpmw/csrf.go

@@ -73,7 +73,6 @@ func CSRF(cookieCfg codersdk.HTTPCookieConfig) func(next http.Handler) http.Hand
 
 			// CSRF only affects requests that automatically attach credentials via a cookie.
 			// If no cookie is present, then there is no risk of CSRF.
-			//nolint:govet
 			sessCookie, err := r.Cookie(codersdk.SessionTokenCookie)
 			if xerrors.Is(err, http.ErrNoCookie) {
 				return true

coderd/x/chatd/chatprovider/chatprovider.go

@@ -118,11 +118,6 @@ func (k ProviderAPIKeys) APIKey(provider string) string {
 	}
 }
 
-//nolint:revive // Intentional: apiKey is the unexported helper for APIKey.
-func (k ProviderAPIKeys) apiKey(provider string) string {
-	return k.APIKey(provider)
-}
-
 // BaseURL returns the configured base URL for a provider.
 func (k ProviderAPIKeys) BaseURL(provider string) string {
 	normalized := NormalizeProvider(provider)
@@ -219,7 +214,7 @@ func (c *ModelCatalog) ListConfiguredModels(
 	providerSet := make(map[string]struct{})
 
 	for _, provider := range configuredProviders {
-		normalized := normalizeProvider(provider.Provider)
+		normalized := NormalizeProvider(provider.Provider)
 		if normalized == "" {
 			continue
 		}
@@ -264,7 +259,7 @@ func (c *ModelCatalog) ListConfiguredModels(
 			Provider: provider,
 			Models:   models,
 		}
-		if keys.apiKey(provider) == "" {
+		if keys.APIKey(provider) == "" {
 			result.Available = false
 			result.UnavailableReason = codersdk.ChatModelProviderUnavailableMissingAPIKey
 		} else {
@@ -292,7 +287,7 @@ func (c *ModelCatalog) ListConfiguredProviderAvailability(
 			Provider: provider,
 			Models:   []codersdk.ChatModel{},
 		}
-		if keys.apiKey(provider) == "" {
+		if keys.APIKey(provider) == "" {
 			result.Available = false
 			result.UnavailableReason = codersdk.ChatModelProviderUnavailableMissingAPIKey
 		} else {
@@ -371,11 +366,6 @@ func NormalizeProvider(provider string) string {
 	}
 }
 
-//nolint:revive // Intentional: normalizeProvider is the unexported helper for NormalizeProvider.
-func normalizeProvider(provider string) string {
-	return NormalizeProvider(provider)
-}
-
 func ResolveModelWithProviderHint(modelName, providerHint string) (provider string, model string, err error) {
 	modelName = strings.TrimSpace(modelName)
 	if modelName == "" {
@@ -386,7 +376,7 @@ func ResolveModelWithProviderHint(modelName, providerHint string) (provider stri
 		return provider, modelID, nil
 	}
 
-	if provider := normalizeProvider(providerHint); provider != "" {
+	if provider := NormalizeProvider(providerHint); provider != "" {
 		return provider, modelName, nil
 	}
 
@@ -422,7 +412,7 @@ func parseCanonicalModelRef(modelRef string) (provider string, model string, ok
 			continue
 		}
 
-		provider := normalizeProvider(parts[0])
+		provider := NormalizeProvider(parts[0])
 		modelID := strings.TrimSpace(parts[1])
 		if provider != "" && modelID != "" {
 			return provider, modelID, true
@@ -433,7 +423,7 @@ func parseCanonicalModelRef(modelRef string) (provider string, model string, ok
 }
 
 func isChatModelForProvider(provider, modelID string) bool {
-	normalizedProvider := normalizeProvider(provider)
+	normalizedProvider := NormalizeProvider(provider)
 	normalizedModel := strings.ToLower(strings.TrimSpace(modelID))
 	switch normalizedProvider {
 	case fantasyopenai.Name:

cryptorand/strings.go

@@ -41,8 +41,6 @@ const (
 //
 // See more details on this algorithm here:
 // https://lemire.me/blog/2016/06/27/a-fast-alternative-to-the-modulo-reduction/
-//
-//nolint:varnamelen
 func unbiasedModulo32(v uint32, n int32) (int32, error) {
 	// #nosec G115 - These conversions are safe within the context of this algorithm
 	// The conversions here are part of an unbiased modulo algorithm for random number generation

enterprise/audit/audit.go

@@ -56,7 +56,9 @@ func (a *auditor) Export(ctx context.Context, alog database.AuditLog) error {
 		return xerrors.Errorf("filter check: %w", err)
 	}
 
-	actor, err := a.db.GetUserByID(dbauthz.AsSystemRestricted(ctx), alog.UserID) //nolint
+	// AsSystemRestricted is used to look up the actor name even
+	// when the caller lacks read access to the user.
+	actor, err := a.db.GetUserByID(dbauthz.AsSystemRestricted(ctx), alog.UserID) //nolint:gocritic // see above
 	if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
 		return err
 	}

tailnet/test/integration/integration.go

@@ -174,7 +174,8 @@ func (s *derpServer) Close() {
 	s.closeFn()
 }
 
-//nolint:revive
+// Router constructs a chi.Mux wired with a coordinator, DERP server,
+// and the networking routes needed for integration tests.
 func (o SimpleServerOptions) Router(t *testing.T, logger slog.Logger) *chi.Mux {
 	coord := tailnet.NewCoordinator(logger)
 	var coordPtr atomic.Pointer[tailnet.Coordinator]
@@ -195,7 +196,7 @@ func (o SimpleServerOptions) Router(t *testing.T, logger slog.Logger) *chi.Mux {
 				Regions: map[int]*tailcfg.DERPRegion{},
 			}
 		},
-		NetworkTelemetryHandler: func(batch []*tailnetproto.TelemetryEvent) {},
+		NetworkTelemetryHandler: func(_ []*tailnetproto.TelemetryEvent) {},
 		ResumeTokenProvider:     tailnet.NewInsecureTestResumeTokenProvider(),
 	})
 	require.NoError(t, err)
@@ -244,10 +245,10 @@ func (o SimpleServerOptions) Router(t *testing.T, logger slog.Logger) *chi.Mux {
 
 			derpServer.Load().ServeHTTP(w, r)
 		})
-		r.Get("/latency-check", func(w http.ResponseWriter, r *http.Request) {
+		r.Get("/latency-check", func(w http.ResponseWriter, _ *http.Request) {
 			w.WriteHeader(http.StatusOK)
 		})
-		r.Post("/restart", func(w http.ResponseWriter, r *http.Request) {
+		r.Post("/restart", func(w http.ResponseWriter, _ *http.Request) {
 			oldServer := derpServer.Swap(newDerpServer(t, logger))
 			oldServer.Close()
 			w.WriteHeader(http.StatusOK)

vpn/dns_internal_test.go

@@ -57,7 +57,6 @@ func TestConvertDNSConfig(t *testing.T) {
 		},
 	}
 
-	//nolint:paralleltest // outdated rule
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()

vpn/router_internal_test.go

@@ -57,7 +57,6 @@ func TestConvertRouterConfig(t *testing.T) {
 			},
 		},
 	}
-	//nolint:paralleltest // outdated rule
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()