pkg/linters: extract inspector helper, migrate to Cursor API, add autofixes

docs/adr/38317-standardize-linter-suite-helpers-cursor-api-autofixes.md

@@ -0,0 +1,42 @@
+# ADR-38317: Standardize the linter suite on shared astutil helpers, the Cursor API, and SuggestedFix autofixes
+
+**Date**: 2026-06-10
+**Status**: Draft
+
+## Context
+
+The `pkg/linters` suite contains 23 `golang.org/x/tools/go/analysis` analyzers. Every analyzer repeated the same three-line type assertion to extract `*inspector.Inspector` from `pass.ResultOf`, two analyzers (`regexpcompileinfunction`, `panic-in-library-code`) still traversed the AST with the older `WithStack` callback plus hand-rolled stack-scanning helpers, and several mechanically-fixable diagnostics (`fprintlnsprintf`, `tolowerequalfold`, `lenstringzero`) reported problems without offering an automatic fix. A `golang.org/x/tools` usage review flagged all three as drift from the modern, idiomatic way to write analyzers in this codebase. The constraint is that any change must be behavior-preserving for diagnostics (existing `analysistest` expectations must continue to hold) while reducing duplication and adopting the current traversal API.
+
+## Decision
+
+We will standardize the linter suite on three shared conventions. First, inspector extraction is centralized in `astutil.Inspector(pass)` (returning `(*inspector.Inspector, error)`) and reused by all 23 linters, alongside a new `astutil.NodeText(fset, node)` helper for rendering nodes during fix generation. Second, AST traversal uses the `inspector.Cursor` API (`insp.Root().Preorder()` + `cur.Enclosing()`) rather than `WithStack`, with explicit `break` in loops that only inspect the immediate enclosing node. Third, mechanically-fixable linters emit `analysis.SuggestedFix` so fixes are available through `multichecker`'s `-fix` flag and editor integrations, with fixes restricted to unambiguous direct-call patterns.
+
+## Alternatives Considered
+
+### Alternative 1: Leave per-linter boilerplate and keep `WithStack`
+Keeping the status quo avoids touching 23 files at once and the review surface that comes with it. It was rejected because the duplicated assertion is a recurring copy-paste hazard, and `WithStack` is the legacy traversal API — new linters were already being written against `Cursor`, so the two stragglers were a maintenance inconsistency.
+
+### Alternative 2: Generate the boilerplate via code generation
+A `go:generate` step could stamp out the inspector-extraction block per linter. It was rejected as heavier than the problem warrants: a single exported helper function gives the same de-duplication with no build-time tooling, no generated files to keep in sync, and a clearer call site.
+
+### Alternative 3: Emit autofixes for all matched patterns, including alias variables
+We could attempt fixes for indirect cases (e.g. `n := len(s); n == 0`, or aliased `strings.ToLower` results). This was rejected because rewriting through an intermediate variable risks changing semantics or producing incorrect edits; limiting fixes to direct call patterns keeps every emitted fix provably safe while leaving the harder cases diagnostic-only.
+
+## Consequences
+
+### Positive
+- Removes 23 copies of the inspector-extraction boilerplate, centralizing the error message in one place.
+- Brings every linter onto the same modern `Cursor` traversal API, easing future maintenance and onboarding.
+- `fprintlnsprintf`, `tolowerequalfold`, and `lenstringzero` now offer automatic fixes via `-fix` and editor integrations.
+
+### Negative
+- All 23 linters now depend on the `internal/astutil` package, increasing coupling to that shared helper; a change there affects every linter.
+- Autofix coverage is intentionally partial — alias/indirect patterns remain diagnostic-only, which may surprise users who expect every diagnostic to be auto-fixable.
+
+### Neutral
+- Tests for the autofixing linters switch from `analysistest.Run` to `analysistest.RunWithSuggestedFixes`, requiring `.golden` files to be kept in sync with the source testdata.
+- Diagnostics that previously used `pass.Reportf`/`pass.ReportRangef` now construct `analysis.Diagnostic` values explicitly to carry `SuggestedFixes`.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27270831408) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/linters/contextcancelnotdeferred/contextcancelnotdeferred.go

@@ -3,15 +3,14 @@
 package contextcancelnotdeferred
 
 import (
-	"fmt"
 	"go/ast"
 	"go/token"
 	"go/types"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/ast/inspector"
 
+	"github.com/github/gh-aw/pkg/linters/internal/astutil"
 	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
 )
 
@@ -25,9 +24,9 @@ var Analyzer = &analysis.Analyzer{
 }
 
 func run(pass *analysis.Pass) (any, error) {
-	insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	if !ok {
-		return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer])
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
 	}
 
 	nodeFilter := []ast.Node{

pkg/linters/ctxbackground/ctxbackground.go

@@ -4,14 +4,13 @@
 package ctxbackground
 
 import (
-	"fmt"
 	"go/ast"
 	"go/types"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/ast/inspector"
 
+	"github.com/github/gh-aw/pkg/linters/internal/astutil"
 	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
 )
 
@@ -25,9 +24,9 @@ var Analyzer = &analysis.Analyzer{
 }
 
 func run(pass *analysis.Pass) (any, error) {
-	insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	if !ok {
-		return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer])
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
 	}
 
 	for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) {

pkg/linters/errormessage/errormessage.go

@@ -3,7 +3,6 @@
 package errormessage
 
 import (
-	"fmt"
 	"go/ast"
 	"go/token"
 	"path/filepath"
@@ -12,7 +11,6 @@ import (
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/ast/inspector"
 
 	"github.com/github/gh-aw/pkg/linters/internal/astutil"
 	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
@@ -44,9 +42,9 @@ func run(pass *analysis.Pass) (any, error) {
 		return nil, nil
 	}
 
-	insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	if !ok {
-		return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer])
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
 	}
 	noLintLinesByFile := nolint.BuildLineIndex(pass, "errormessage")
 

pkg/linters/errstringmatch/errstringmatch.go

@@ -4,13 +4,11 @@
 package errstringmatch
 
 import (
-	"fmt"
 	"go/ast"
 	"go/types"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/ast/inspector"
 
 	"github.com/github/gh-aw/pkg/linters/internal/astutil"
 	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
@@ -27,9 +25,9 @@ var Analyzer = &analysis.Analyzer{
 }
 
 func run(pass *analysis.Pass) (any, error) {
-	insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	if !ok {
-		return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer])
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
 	}
 	noLintLinesByFile := nolint.BuildLineIndex(pass, "errstringmatch")
 

pkg/linters/excessivefuncparams/excessivefuncparams.go

@@ -3,12 +3,12 @@
 package excessivefuncparams
 
 import (
-	"fmt"
 	"go/ast"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/ast/inspector"
+
+	"github.com/github/gh-aw/pkg/linters/internal/astutil"
 )
 
 // DefaultMaxParams is the default maximum number of parameters allowed in a function declaration.
@@ -32,9 +32,9 @@ func init() {
 }
 
 func run(pass *analysis.Pass) (any, error) {
-	insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	if !ok {
-		return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer])
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
 	}
 
 	nodeFilter := []ast.Node{

pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go

@@ -11,8 +11,8 @@ import (
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/ast/inspector"
 
+	"github.com/github/gh-aw/pkg/linters/internal/astutil"
 	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
 )
 
@@ -26,9 +26,9 @@ var Analyzer = &analysis.Analyzer{
 }
 
 func run(pass *analysis.Pass) (any, error) {
-	insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	if !ok {
-		return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer])
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
 	}
 
 	for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) {

pkg/linters/fileclosenotdeferred/fileclosenotdeferred.go

@@ -3,15 +3,14 @@
 package fileclosenotdeferred
 
 import (
-	"fmt"
 	"go/ast"
 	"go/token"
 	"go/types"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/ast/inspector"
 
+	"github.com/github/gh-aw/pkg/linters/internal/astutil"
 	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
 )
 
@@ -25,9 +24,9 @@ var Analyzer = &analysis.Analyzer{
 }
 
 func run(pass *analysis.Pass) (any, error) {
-	insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	if !ok {
-		return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer])
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
 	}
 
 	nodeFilter := []ast.Node{

pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go

@@ -4,14 +4,12 @@
 package fmterrorfnoverbs
 
 import (
-	"fmt"
 	"go/ast"
 	"go/token"
 	"strings"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/ast/inspector"
 
 	"github.com/github/gh-aw/pkg/linters/internal/astutil"
 	"github.com/github/gh-aw/pkg/linters/internal/nolint"
@@ -27,9 +25,9 @@ var Analyzer = &analysis.Analyzer{
 }
 
 func run(pass *analysis.Pass) (any, error) {
-	insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	if !ok {
-		return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer])
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
 	}
 	noLintLinesByFile := nolint.BuildLineIndex(pass, "fmterrorfnoverbs")
 

pkg/linters/fprintlnsprintf/fprintlnsprintf.go

@@ -3,13 +3,15 @@
 package fprintlnsprintf
 
 import (
-	"fmt"
 	"go/ast"
+	"go/token"
+	"strconv"
+	"strings"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/ast/inspector"
 
+	"github.com/github/gh-aw/pkg/linters/internal/astutil"
 	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
 	"github.com/github/gh-aw/pkg/linters/internal/nolint"
 )
@@ -24,9 +26,9 @@ var Analyzer = &analysis.Analyzer{
 }
 
 func run(pass *analysis.Pass) (any, error) {
-	insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
-	if !ok {
-		return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer])
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
 	}
 	noLintLinesByFile := nolint.BuildLineIndex(pass, "fprintlnsprintf")
 
@@ -66,12 +68,69 @@ func run(pass *analysis.Pass) (any, error) {
 			return
 		}
 
-		pass.Reportf(call.Pos(), "use fmt.Fprintf instead of fmt.Fprintln(w, fmt.Sprintf(...))")
+		pass.Report(analysis.Diagnostic{
+			Pos:            call.Pos(),
+			End:            call.End(),
+			Message:        "use fmt.Fprintf instead of fmt.Fprintln(w, fmt.Sprintf(...))",
+			SuggestedFixes: buildFprintfFix(call, printedArg),
+		})
 	})
 
 	return nil, nil
 }
 
+// buildFprintfFix returns a SuggestedFix rewriting
+// fmt.Fprintln(w, fmt.Sprintf("format", args...)) to
+// fmt.Fprintf(w, "format\n", args...).
+// A fix is only emitted when the format argument is a plain double-quoted
+// string literal; other forms (raw strings, variables) are left unfixed.
+func buildFprintfFix(call *ast.CallExpr, sprintfCall *ast.CallExpr) []analysis.SuggestedFix {
+	if len(sprintfCall.Args) == 0 {
+		return nil
+	}
+	formatLit, ok := sprintfCall.Args[0].(*ast.BasicLit)
+	if !ok || formatLit.Kind != token.STRING {
+		return nil
+	}
+	raw := formatLit.Value
+	if len(raw) < 2 || raw[0] != '"' || raw[len(raw)-1] != '"' {
+		return nil // not a plain double-quoted literal
+	}
+
+	// Decode the literal to check for a trailing newline so we don't produce
+	// a double \n when the format string already ends with one.
+	decoded, err := strconv.Unquote(raw)
+	if err != nil {
+		return nil
+	}
+
+	// Build the replacement format literal: append \n only when not already present.
+	var newFormatLit []byte
+	if strings.HasSuffix(decoded, "\n") {
+		newFormatLit = []byte(raw) // already ends with \n; keep as-is
+	} else {
+		newFormatLit = []byte(raw[:len(raw)-1] + `\n"`)
+	}
+
+	outerSel, ok := call.Fun.(*ast.SelectorExpr)
+	if !ok {
+		return nil
+	}
+	return []analysis.SuggestedFix{{
+		Message: `Replace fmt.Fprintln with fmt.Fprintf`,
+		TextEdits: []analysis.TextEdit{
+			// 1. "Fprintln" → "Fprintf"
+			{Pos: outerSel.Sel.Pos(), End: outerSel.Sel.End(), NewText: []byte("Fprintf")},
+			// 2. Delete "fmt.Sprintf(" — from the start of sprintfCall to after its "("
+			{Pos: sprintfCall.Pos(), End: sprintfCall.Lparen + 1, NewText: nil},
+			// 3. Replace the format literal (adding \n if not already present).
+			{Pos: formatLit.Pos(), End: formatLit.End(), NewText: newFormatLit},
+			// 4. Delete the closing ")" of fmt.Sprintf(...)
+			{Pos: sprintfCall.Rparen, End: sprintfCall.Rparen + 1, NewText: nil},
+		},
+	}}
+}
+
 // isFmtFunc returns true if call is a call to fmt.<name>.
 func isFmtFunc(call *ast.CallExpr, name string) bool {
 	sel, ok := call.Fun.(*ast.SelectorExpr)

pkg/linters/fprintlnsprintf/fprintlnsprintf_test.go

@@ -11,5 +11,5 @@ import (
 )
 
 func TestAnalyzer(t *testing.T) {
-	analysistest.Run(t, analysistest.TestData(), fprintlnsprintf.Analyzer, "fprintlnsprintf")
+	analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), fprintlnsprintf.Analyzer, "fprintlnsprintf")
 }

pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf.go

@@ -9,6 +9,10 @@ func flagged(name string) {
 	fmt.Fprintln(os.Stderr, fmt.Sprintf("hello %s", name)) // want "use fmt.Fprintf"
 }
 
+func flaggedAlreadyNewline(name string) {
+	fmt.Fprintln(os.Stderr, fmt.Sprintf("hello %s\n", name)) // want "use fmt.Fprintf"
+}
+
 func notFlagged(name string) {
 	fmt.Fprintln(os.Stderr, "plain string")
 	fmt.Fprintln(os.Stderr, "prefix", fmt.Sprintf("hello %s", name))