[linter-miner] linter: add stringsindexcontains — flag strings.Index comparisons that should use strings.Contains

cmd/linters/main.go

@@ -51,6 +51,7 @@ import (
 	"github.com/github/gh-aw/pkg/linters/ssljson"
 	"github.com/github/gh-aw/pkg/linters/strconvparseignorederror"
 	"github.com/github/gh-aw/pkg/linters/stringreplaceminusone"
+	"github.com/github/gh-aw/pkg/linters/stringsindexcontains"
 	"github.com/github/gh-aw/pkg/linters/timeafterleak"
 	"github.com/github/gh-aw/pkg/linters/timesleepnocontext"
 	"github.com/github/gh-aw/pkg/linters/tolowerequalfold"
@@ -92,6 +93,7 @@ func main() {
 		sprintfint.Analyzer,
 		strconvparseignorederror.Analyzer,
 		stringreplaceminusone.Analyzer,
+		stringsindexcontains.Analyzer,
 		jsonmarshalignoredeerror.Analyzer,
 		lenstringzero.Analyzer,
 		lenstringsplit.Analyzer,

docs/adr/43253-add-stringsindexcontains-linter.md

@@ -0,0 +1,44 @@
+# ADR-43253: Add stringsindexcontains Custom Go Analysis Linter
+
+**Date**: 2026-07-04
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+The `gh-aw` codebase uses a custom Go static analysis framework (`pkg/linters/`) to enforce code style and correctness automatically. A scan of `pkg/` and `cmd/` revealed 42 occurrences of the pattern `strings.Index(s, substr) != -1` (and equivalent variants such as `>= 0`, `== -1`, `< 0`). These comparisons are semantically equivalent to `strings.Contains(s, substr)` or `!strings.Contains(s, substr)`, but are less readable and more error-prone to write correctly. The codebase already ships several analogous custom linters (e.g., `stringreplaceminusone`, `lenstringzero`) that follow the same `go/analysis` pass pattern.
+
+### Decision
+
+We will add a new `stringsindexcontains` custom `go/analysis` linter to `pkg/linters/stringsindexcontains/` that reports `strings.Index(s, substr)` comparisons against `-1` or `0` and suggests replacing them with `strings.Contains(s, substr)` or `!strings.Contains(s, substr)`. The linter will emit `SuggestedFix` text edits to enable automated batch repair. It will be registered in `cmd/linters/main.go` alongside existing analyzers.
+
+### Alternatives Considered
+
+#### Alternative 1: Rely on manual code review
+
+Code reviewers would be expected to flag `strings.Index` containment-check patterns during PR review. This approach requires no new tooling and imposes no maintenance burden. It was not chosen because it is inconsistent — reviewers can miss patterns, especially in large diffs — and it does not help with the 42 existing occurrences already in the codebase.
+
+#### Alternative 2: Enable an existing third-party linter
+
+Linters such as `gocritic` (via `sloppyReassign` or similar heuristics) or `staticcheck` cover some idiomatic Go patterns. Using a pre-built linter avoids writing and maintaining custom code. This was not chosen because no widely-adopted third-party linter precisely covers all six operator/literal combinations (`!= -1`, `>= 0`, `> -1`, `== -1`, `< 0`, `<= -1`) with yoda-order variants and automated fix suggestions, and integrating a new external dependency would require vetting and approval across the toolchain.
+
+### Consequences
+
+#### Positive
+- Automatically detects all six semantic variants of the `strings.Index` containment-check anti-pattern, including yoda-order forms.
+- Provides `SuggestedFix` text edits that allow the linter runner to apply fixes automatically, enabling bulk remediation of the 42 known occurrences.
+- Follows the established pattern for custom linters in this codebase, making future maintenance and extension straightforward.
+
+#### Negative
+- Adds a new package (`pkg/linters/stringsindexcontains/`) that must be maintained when upstream `go/analysis` APIs or internal utilities change.
+- The linter will flag only the specific operator/literal combinations listed; any variant that was intentionally left as `strings.Index` for a non-containment reason (e.g., comparing against a non-`-1`/`0` threshold) is already excluded by design, but edge cases may surface over time.
+
+#### Neutral
+- The linter is registered in `cmd/linters/main.go` alongside all other custom analyzers; it participates in the same execution pipeline with no special orchestration.
+- Test fixtures in `pkg/linters/stringsindexcontains/testdata/` document both flagged and acceptable patterns, serving as living documentation of the linter's intent.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/linters/README.md

@@ -41,6 +41,7 @@ This package currently provides custom Go analyzers in the following subpackages
 - `ssljson` — validates `ssl.json` skill artifacts found in `.github/skills/` against the SSL spec (enum membership, graph integrity, transition targets, entry pointer validity).
 - `strconvparseignorederror` — reports `strconv` parsing calls (`Atoi`, `ParseInt`, etc.) where the error return is discarded with `_`.
 - `stringreplaceminusone` — reports `strings.Replace` calls whose `n` argument is `-1`, which should use the more readable `strings.ReplaceAll`.
+- `stringsindexcontains` — reports `strings.Index(s, substr)` comparisons with `-1` or `0` (e.g. `!= -1`, `>= 0`, `> -1`, `== -1`, `< 0`, `<= -1`) and their yoda-order variants that should use `strings.Contains(s, substr)` or `!strings.Contains(s, substr)` instead.
 - `timeafterleak` — reports `time.After` calls used as the channel-receive expression in a `select` case inside a `for` or `range` loop that leak a timer channel on each iteration when another case fires first.
 - `timesleepnocontext` — reports `time.Sleep` calls inside functions that already receive a `context.Context`, where a context-aware `select` should be used instead.
 - `tolowerequalfold` — reports case-insensitive string comparisons using `strings.ToLower`/`ToUpper` that should use `strings.EqualFold`.
@@ -89,6 +90,7 @@ This package currently provides custom Go analyzers in the following subpackages
 | `ssljson` | Custom `go/analysis` analyzer that validates SSL JSON skill artifacts in `.github/skills/` |
 | `strconvparseignorederror` | Custom `go/analysis` analyzer that flags `strconv` parsing calls where the error return is discarded with `_` |
 | `stringreplaceminusone` | Custom `go/analysis` analyzer that flags `strings.Replace` calls with `n=-1` that should use `strings.ReplaceAll` |
+| `stringsindexcontains` | Custom `go/analysis` analyzer that flags `strings.Index(s, substr)` comparisons with `-1` or `0` that should use `strings.Contains` or `!strings.Contains` |
 | `timeafterleak` | Custom `go/analysis` analyzer that flags `time.After` in `select` cases inside loops that leak a timer channel on each iteration when another case fires first |
 | `timesleepnocontext` | Custom `go/analysis` analyzer that flags `time.Sleep` calls in context-aware functions |
 | `tolowerequalfold` | Custom `go/analysis` analyzer that flags case-insensitive comparisons via `strings.ToLower`/`ToUpper` that should use `strings.EqualFold` |
@@ -197,6 +199,7 @@ _ = timesleepnocontext.Analyzer
 - `github.com/github/gh-aw/pkg/linters/ssljson` — ssl-json analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/strconvparseignorederror` — strconv-parse-ignored-error analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/stringreplaceminusone` — string-replace-minus-one analyzer subpackage
+- `github.com/github/gh-aw/pkg/linters/stringsindexcontains` — strings-index-contains analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/timeafterleak` — time-after-leak analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/timesleepnocontext` — time-sleep-no-context analyzer subpackage
 - `github.com/github/gh-aw/pkg/linters/tolowerequalfold` — to-lower-equal-fold analyzer subpackage

pkg/linters/spec_test.go

@@ -42,6 +42,7 @@ import (
 	"github.com/github/gh-aw/pkg/linters/ssljson"
 	"github.com/github/gh-aw/pkg/linters/strconvparseignorederror"
 	"github.com/github/gh-aw/pkg/linters/stringreplaceminusone"
+	"github.com/github/gh-aw/pkg/linters/stringsindexcontains"
 	"github.com/github/gh-aw/pkg/linters/timeafterleak"
 	"github.com/github/gh-aw/pkg/linters/timesleepnocontext"
 	"github.com/github/gh-aw/pkg/linters/tolowerequalfold"
@@ -62,7 +63,7 @@ type docAnalyzer struct {
 }
 
 // documentedAnalyzers returns the analyzer subpackages documented in the README
-// "Public API > Subpackages" table. The README documents 36 analyzers
+// "Public API > Subpackages" table. The README documents 37 analyzers
 // subpackages (the non-analyzer `internal` helper subpackage is excluded because
 // it exposes no Analyzer).
 //
@@ -73,7 +74,7 @@ type docAnalyzer struct {
 //	hardcodedfilepath, httpnoctx, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero,
 //	manualmutexunlock, osexitinlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib,
 //	regexpcompileinfunction, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, ssljson,
-//	strconvparseignorederror, stringreplaceminusone, timeafterleak, timesleepnocontext,
+//	strconvparseignorederror, stringreplaceminusone, stringsindexcontains, timeafterleak, timesleepnocontext,
 //	tolowerequalfold, uncheckedtypeassertion, wgdonenotdeferred
 func documentedAnalyzers() []docAnalyzer {
 	return []docAnalyzer{
@@ -108,6 +109,7 @@ func documentedAnalyzers() []docAnalyzer {
 		{"ssljson", ssljson.Analyzer},
 		{"strconvparseignorederror", strconvparseignorederror.Analyzer},
 		{"stringreplaceminusone", stringreplaceminusone.Analyzer},
+		{"stringsindexcontains", stringsindexcontains.Analyzer},
 		{"timeafterleak", timeafterleak.Analyzer},
 		{"timesleepnocontext", timesleepnocontext.Analyzer},
 		{"tolowerequalfold", tolowerequalfold.Analyzer},

pkg/linters/stringsindexcontains/stringsindexcontains.go

@@ -0,0 +1,242 @@
+// Package stringsindexcontains implements a Go analysis linter that flags
+// strings.Index(s, substr) comparisons with -1 or 0 (e.g. != -1, >= 0, > -1,
+// == -1, < 0, <= -1) and their yoda-order variants that should use the more
+// readable strings.Contains(s, substr) or !strings.Contains(s, substr) instead.
+package stringsindexcontains
+
+import (
+	"go/ast"
+	"go/constant"
+	"go/token"
+
+	"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"
+	"github.com/github/gh-aw/pkg/linters/internal/nolint"
+)
+
+// Analyzer is the strings-index-contains analysis pass.
+var Analyzer = &analysis.Analyzer{
+	Name:     "stringsindexcontains",
+	Doc:      "reports strings.Index(s, substr) comparisons with -1 or 0 (e.g. != -1, >= 0, > -1, == -1, < 0, <= -1) and their yoda-order variants that should use strings.Contains(s, substr) or !strings.Contains(s, substr)",
+	URL:      "https://github.com/github/gh-aw/tree/main/pkg/linters/stringsindexcontains",
+	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
+	}
+	noLintLinesByFile := nolint.BuildLineIndex(pass, "stringsindexcontains")
+
+	nodeFilter := []ast.Node{(*ast.BinaryExpr)(nil)}
+
+	insp.Preorder(nodeFilter, func(n ast.Node) {
+		expr, ok := n.(*ast.BinaryExpr)
+		if !ok {
+			return
+		}
+
+		pos := pass.Fset.PositionFor(expr.Pos(), false)
+		if filecheck.IsTestFile(pos.Filename) {
+			return
+		}
+		if nolint.HasDirective(pos, noLintLinesByFile) {
+			return
+		}
+
+		// Match patterns:
+		//   strings.Index(s, sub) != -1  → strings.Contains(s, sub)
+		//   strings.Index(s, sub) >= 0   → strings.Contains(s, sub)
+		//   strings.Index(s, sub) == -1  → !strings.Contains(s, sub)
+		//   strings.Index(s, sub) < 0    → !strings.Contains(s, sub)
+		// (and yoda variants: -1 != strings.Index(...), etc.)
+
+		indexCall, negated, matched := matchIndexComparison(pass, expr)
+		if !matched {
+			return
+		}
+
+		if len(indexCall.Args) != 2 {
+			return
+		}
+
+		sText := astutil.NodeText(pass.Fset, indexCall.Args[0])
+		subText := astutil.NodeText(pass.Fset, indexCall.Args[1])
+		pkgText := indexPkgText(pass, indexCall)
+		if sText == "" || subText == "" || pkgText == "" {
+			return
+		}
+
+		var msg string
+		if negated {
+			msg = "use !strings.Contains(" + sText + ", " + subText + ") instead of strings.Index comparison"
+		} else {
+			msg = "use strings.Contains(" + sText + ", " + subText + ") instead of strings.Index comparison"
+		}
+
+		fix := buildContainsFix(pass, expr, pkgText, sText, subText, negated)
+		pass.Report(analysis.Diagnostic{
+			Pos:            expr.Pos(),
+			End:            expr.End(),
+			Message:        msg,
+			SuggestedFixes: fix,
+		})
+	})
+
+	return nil, nil
+}
+
+// matchIndexComparison reports whether expr is a strings.Index comparison with -1 or 0.
+// It returns the strings.Index call, whether the result is negated (i.e., checks for absence),
+// and whether the pattern matched.
+//
+// Matched patterns (contains → negated=false):
+//   - strings.Index(s, sub) != -1
+//   - strings.Index(s, sub) >= 0
+//   - -1 != strings.Index(s, sub)
+//   - 0 <= strings.Index(s, sub)
+//
+// Matched patterns (not-contains → negated=true):
+//   - strings.Index(s, sub) == -1
+//   - strings.Index(s, sub) < 0
+//   - -1 == strings.Index(s, sub)
+//   - 0 > strings.Index(s, sub)
+func matchIndexComparison(pass *analysis.Pass, expr *ast.BinaryExpr) (call *ast.CallExpr, negated bool, matched bool) {
+	// Normalize so that the strings.Index call is on the left side.
+	left, right, flipped := normalizeOperands(pass, expr)
+
+	indexCall, ok := asStringsIndexCall(pass, left)
+	if !ok {
+		return nil, false, false
+	}
+
+	op := expr.Op
+	if flipped {
+		op = flipOp(op)
+	}
+
+	litVal, ok := constIntValue(pass, right)
+	if !ok {
+		return nil, false, false
+	}
+
+	// Check supported operator/literal combinations.
+	switch op {
+	case token.NEQ:
+		// strings.Index(...) != -1  →  contains
+		if litVal == -1 {
+			return indexCall, false, true
+		}
+	case token.GEQ:
+		// strings.Index(...) >= 0  →  contains
+		if litVal == 0 {
+			return indexCall, false, true
+		}
+	case token.GTR:
+		// strings.Index(...) > -1  →  contains (less common but valid)
+		if litVal == -1 {
+			return indexCall, false, true
+		}
+	case token.EQL:
+		// strings.Index(...) == -1  →  !contains
+		if litVal == -1 {
+			return indexCall, true, true
+		}
+	case token.LSS:
+		// strings.Index(...) < 0  →  !contains
+		if litVal == 0 {
+			return indexCall, true, true
+		}
+	case token.LEQ:
+		// strings.Index(...) <= -1  →  !contains (less common but valid)
+		if litVal == -1 {
+			return indexCall, true, true
+		}
+	}
+
+	return nil, false, false
+}
+
+// normalizeOperands returns (left, right) such that if the strings.Index call
+// is on the right side, the operands are swapped and flipped=true.
+func normalizeOperands(pass *analysis.Pass, expr *ast.BinaryExpr) (left, right ast.Expr, flipped bool) {
+	if _, ok := asStringsIndexCall(pass, expr.X); ok {
+		return expr.X, expr.Y, false
+	}
+	return expr.Y, expr.X, true
+}
+
+// asStringsIndexCall returns the *ast.CallExpr if expr is a call to strings.Index.
+func asStringsIndexCall(pass *analysis.Pass, expr ast.Expr) (*ast.CallExpr, bool) {
+	call, ok := expr.(*ast.CallExpr)
+	if !ok {
+		return nil, false
+	}
+	sel, ok := call.Fun.(*ast.SelectorExpr)
+	if !ok || sel.Sel.Name != "Index" {
+		return nil, false
+	}
+	if !astutil.IsPkgSelector(pass, sel, "strings") {
+		return nil, false
+	}
+	return call, true
+}
+
+// constIntValue returns the integer constant value of expr, if it is a constant integer.
+func constIntValue(pass *analysis.Pass, expr ast.Expr) (int64, bool) {
+	tv, ok := pass.TypesInfo.Types[expr]
+	if !ok || tv.Value == nil || tv.Value.Kind() != constant.Int {
+		return 0, false
+	}
+	v, exact := constant.Int64Val(tv.Value)
+	return v, exact
+}
+
+// flipOp returns the comparison operator with left and right operands swapped.
+func flipOp(op token.Token) token.Token {
+	switch op {
+	case token.LSS:
+		return token.GTR
+	case token.GTR:
+		return token.LSS
+	case token.LEQ:
+		return token.GEQ
+	case token.GEQ:
+		return token.LEQ
+	default:
+		return op
+	}
+}
+
+// indexPkgText returns the package selector text (e.g., "strings") from a strings.Index call.
+func indexPkgText(pass *analysis.Pass, call *ast.CallExpr) string {
+	sel, ok := call.Fun.(*ast.SelectorExpr)
+	if !ok {
+		return ""
+	}
+	return astutil.NodeText(pass.Fset, sel.X)
+}
+
+// buildContainsFix builds the suggested fix rewriting the comparison to strings.Contains.
+func buildContainsFix(pass *analysis.Pass, expr *ast.BinaryExpr, pkgText, sText, subText string, negated bool) []analysis.SuggestedFix {
+	var replacement string
+	if negated {
+		replacement = "!" + pkgText + ".Contains(" + sText + ", " + subText + ")"
+	} else {
+		replacement = pkgText + ".Contains(" + sText + ", " + subText + ")"
+	}
+
+	return []analysis.SuggestedFix{{
+		Message: "Replace strings.Index comparison with strings.Contains",
+		TextEdits: []analysis.TextEdit{{
+			Pos:     expr.Pos(),
+			End:     expr.End(),
+			NewText: []byte(replacement),
+		}},
+	}}
+}

pkg/linters/stringsindexcontains/stringsindexcontains_test.go

@@ -0,0 +1,16 @@
+//go:build !integration
+
+package stringsindexcontains_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+
+	"github.com/github/gh-aw/pkg/linters/stringsindexcontains"
+)
+
+func TestAnalyzer(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, stringsindexcontains.Analyzer, "stringsindexcontains")
+}