[linter-miner] linter: add sprintferrorsnew — flag errors.New(fmt.Sprintf(...))

cmd/linters/main.go

@@ -40,6 +40,7 @@ import (
 	"github.com/github/gh-aw/pkg/linters/seenmapbool"
 	"github.com/github/gh-aw/pkg/linters/sortslice"
 	"github.com/github/gh-aw/pkg/linters/sprintferrdot"
+	"github.com/github/gh-aw/pkg/linters/sprintferrorsnew"
 	"github.com/github/gh-aw/pkg/linters/ssljson"
 	"github.com/github/gh-aw/pkg/linters/strconvparseignorederror"
 	"github.com/github/gh-aw/pkg/linters/timeafterleak"
@@ -73,6 +74,7 @@ func main() {
 		seenmapbool.Analyzer,
 		sortslice.Analyzer,
 		sprintferrdot.Analyzer,
+		sprintferrorsnew.Analyzer,
 		strconvparseignorederror.Analyzer,
 		jsonmarshalignoredeerror.Analyzer,
 		lenstringzero.Analyzer,

docs/adr/40490-add-sprintferrorsnew-linter.md

@@ -0,0 +1,39 @@
+# ADR-40490: Add `sprintferrorsnew` linter for `errors.New(fmt.Sprintf(...))`
+
+**Date**: 2026-06-20
+**Status**: Draft
+
+## Context
+
+The codebase contains a custom `go/analysis` linter suite (`pkg/linters/*`, registered in `cmd/linters/main.go`) used to enforce house style and catch anti-patterns that off-the-shelf golangci-lint rules miss. A recurring pattern was found in `pkg/workflow` where error values are built with `errors.New(fmt.Sprintf(...))` instead of the idiomatic `fmt.Errorf(...)`. The inline form is longer, allocates an intermediate string on the heap, and cannot use `%w` for error wrapping. Existing enabled rules (`errcheck`, `errorlint`, `nilerr`) do not detect it. The constraint is that any new rule must integrate with the established analyzer framework and keep false positives low.
+
+## Decision
+
+We will add a new `go/analysis` analyzer package `pkg/linters/sprintferrorsnew` and register it in `cmd/linters/main.go`. The analyzer flags only the **inline** form `errors.New(fmt.Sprintf(...))` — a single-argument `errors.New` call whose sole argument is a direct `fmt.Sprintf` call — using AST inspection plus the shared `astutil.IsPkgSelector` helper to confirm package identity. Test files are skipped via `filecheck.IsTestFile`, and the variable-assignment form (`msg := fmt.Sprintf(...); errors.New(msg)`) is deliberately not flagged to preserve a high signal-to-noise ratio.
+
+## Alternatives Considered
+
+### Alternative 1: Extend an existing golangci-lint rule / use an off-the-shelf linter
+No commonly enabled rule (`errcheck`, `errorlint`, `nilerr`, etc.) detects this pattern, and adding a third-party dependency or upstream change is heavier and slower than a focused local analyzer that fits the existing `pkg/linters` framework. Rejected as higher cost with less control.
+
+### Alternative 2: Also flag the variable-assignment form
+The analyzer could trace `errors.New(msg)` back to a nearby `msg := fmt.Sprintf(...)`. This requires dataflow tracking, increases false-positive risk (the variable may be reused or conditionally assigned), and adds complexity. Rejected to keep the rule simple and high-precision; only the unambiguous inline form is flagged.
+
+## Consequences
+
+### Positive
+- Catches a real, repeated anti-pattern (4 instances in `pkg/workflow`) that existing linters miss.
+- Encourages `fmt.Errorf`, enabling `%w` wrapping and avoiding a redundant allocation.
+- Follows the established analyzer pattern, so it is consistent with the rest of the suite and covered by `analysistest`-based tests.
+
+### Negative
+- Adds another analyzer to maintain and run, marginally increasing lint time and the registration surface in `cmd/linters/main.go`.
+- The deliberate scope limit means the variable-assignment form goes undetected, so some occurrences of the anti-pattern remain unflagged.
+
+### Neutral
+- Only the inline form is reported; authors who prefer the assignment form for readability are unaffected.
+- The rule is purely diagnostic (`pass.Reportf`); it offers no automated fix/suggested edit.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27880137982) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/linters/sprintferrorsnew/sprintferrorsnew.go

@@ -0,0 +1,73 @@
+// Package sprintferrorsnew implements a Go analysis linter that flags
+// errors.New(fmt.Sprintf(...)) calls that should use fmt.Errorf instead.
+package sprintferrorsnew
+
+import (
+	"go/ast"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+
+	"github.com/github/gh-aw/pkg/linters/internal/astutil"
+	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
+)
+
+// Analyzer is the sprintferrorsnew analysis pass.
+var Analyzer = &analysis.Analyzer{
+	Name:     "sprintferrorsnew",
+	Doc:      "reports errors.New(fmt.Sprintf(...)) calls that should use fmt.Errorf instead",
+	URL:      "https://github.com/github/gh-aw/tree/main/pkg/linters/sprintferrorsnew",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	insp, err := astutil.Inspector(pass)
+	if err != nil {
+		return nil, err
+	}
+
+	nodeFilter := []ast.Node{
+		(*ast.CallExpr)(nil),
+	}
+
+	insp.Preorder(nodeFilter, func(n ast.Node) {
+		call, ok := n.(*ast.CallExpr)
+		if !ok {
+			return
+		}
+
+		if filecheck.IsTestFile(pass.Fset.Position(call.Pos()).Filename) {
+			return
+		}
+
+		// Match errors.New(...)
+		sel, ok := call.Fun.(*ast.SelectorExpr)
+		if !ok || sel.Sel.Name != "New" {
+			return
+		}
+		if !astutil.IsPkgSelector(pass, sel, "errors") {
+			return
+		}
+		if len(call.Args) != 1 {
+			return
+		}
+
+		// Check if the sole argument is a direct fmt.Sprintf(...) call.
+		argCall, ok := call.Args[0].(*ast.CallExpr)
+		if !ok {
+			return
+		}
+		argSel, ok := argCall.Fun.(*ast.SelectorExpr)
+		if !ok || argSel.Sel.Name != "Sprintf" {
+			return
+		}
+		if !astutil.IsPkgSelector(pass, argSel, "fmt") {
+			return
+		}
+
+		pass.Reportf(call.Pos(), "use fmt.Errorf instead of errors.New(fmt.Sprintf(...))")
+	})
+
+	return nil, nil
+}

pkg/linters/sprintferrorsnew/sprintferrorsnew_test.go

@@ -0,0 +1,17 @@
+//go:build !integration
+
+// Package sprintferrorsnew_test provides tests for the sprintferrorsnew analyzer.
+package sprintferrorsnew_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+
+	"github.com/github/gh-aw/pkg/linters/sprintferrorsnew"
+)
+
+func TestSprintfErrorsNew(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, sprintferrorsnew.Analyzer, "sprintferrorsnew")
+}

pkg/linters/sprintferrorsnew/testdata/src/sprintferrorsnew/sprintferrorsnew.go

@@ -0,0 +1,33 @@
+// Package sprintferrorsnew is the test fixture for the sprintferrorsnew analyzer.
+package sprintferrorsnew
+
+import (
+	"errors"
+	"fmt"
+)
+
+// bad demonstrates the flagged pattern: errors.New wrapping a fmt.Sprintf call.
+func bad() error {
+	return errors.New(fmt.Sprintf("invalid engine: %s", "claude")) // want `use fmt\.Errorf instead of errors\.New\(fmt\.Sprintf\(\.\.\.\)\)`
+}
+
+// badWithMultipleArgs demonstrates the flagged pattern with multiple format args.
+func badWithMultipleArgs() error {
+	return errors.New(fmt.Sprintf("invalid value %q for flag %s", "x", "-n")) // want `use fmt\.Errorf instead of errors\.New\(fmt\.Sprintf\(\.\.\.\)\)`
+}
+
+// goodErrorf uses fmt.Errorf directly — no diagnostic expected.
+func goodErrorf() error {
+	return fmt.Errorf("invalid engine: %s", "claude")
+}
+
+// goodVariable uses a pre-built string variable — not flagged by this linter.
+func goodVariable() error {
+	msg := fmt.Sprintf("invalid engine: %s", "claude")
+	return errors.New(msg)
+}
+
+// goodPlainString uses a string literal — no diagnostic expected.
+func goodPlainString() error {
+	return errors.New("something went wrong")
+}