[linter-miner] Add buffer-reset-before-reuse linter

docs/adr/60026-add-buffer-reset-before-reuse-linter.md

@@ -0,0 +1,50 @@
+# ADR-60026: Add Buffer Reset Before Reuse Linter
+
+**Date**: 2026-09-10
+**Status**: Draft
+**Deciders**: gh-aw maintainers
+
+---
+
+### Context
+
+This pull request adds a new Go analysis linter under `pkg/linters/bufferresetbeforereuse/` and registers it in the shared linter registry. The implementation scans function bodies for `bytes.Buffer` and `strings.Builder` variables that are written to, read from, and then written to again without an intervening `Reset()`. The PR includes analyzer test coverage and test fixtures that show both accepted and rejected usage patterns, including support for pointer buffers and `nolint` suppression. The architectural question is whether this buffer-reuse bug pattern should be enforced as a first-class repository linter rather than left to code review or broader existing lint rules.
+
+### Decision
+
+We will add a dedicated `bufferresetbeforereuse` analyzer to the repository's linter suite and register it in the global analyzer registry. We decided to model this as a custom AST and type-based linter because the PR evidence shows a specific correctness bug pattern involving stateful reuse of `bytes.Buffer` and `strings.Builder` that is not covered by the existing standard lint set. This makes the rule reusable across the codebase, testable with fixture-based cases, and suppressible through the existing `nolint` mechanism when a caller intentionally accepts the pattern.
+
+### Alternatives Considered
+
+#### Alternative 1: Rely on manual code review for buffer and builder reuse
+
+The team could continue to detect improper `bytes.Buffer` and `strings.Builder` reuse during human review. This was considered because the misuse pattern is understandable to experienced Go reviewers and does not require new analyzer infrastructure. It was not chosen because the PR explicitly introduces automated detection, test fixtures, and registry integration, indicating that the bug is subtle enough to escape review and valuable enough to check systematically.
+
+#### Alternative 2: Depend only on existing general-purpose Go linters
+
+Another option would be to keep the current linter set unchanged and assume existing upstream analyzers are sufficient. This was considered because adding a custom analyzer increases maintenance cost and the repository already ships many lint rules. It was not chosen because the PR description and implementation are centered on a repository-specific gap: writes after reads on buffers/builders without `Reset()` are treated as a correctness hazard that standard lint tooling does not already flag.
+
+#### Alternative 3: Detect the pattern with simpler text or grep-based heuristics
+
+The project could try to catch buffer reuse with lightweight string matching on method calls rather than AST and type inspection. This was considered because it would be simpler to implement initially. It was not chosen because the analyzer needs to distinguish actual `bytes.Buffer` and `strings.Builder` variables, support both value and pointer forms, and respect existing `nolint` and generated-file behavior, which are better served by structured analysis.
+
+### Consequences
+
+#### Positive
+- The repository gains automated detection of a concrete stale-data bug pattern involving `bytes.Buffer` and `strings.Builder` reuse.
+- The rule is centralized in the linter registry, so the check can run consistently across the codebase rather than depending on reviewer memory.
+- Fixture-based tests document expected behavior for valid resets, invalid reuse, pointer buffers, and suppression directives.
+
+#### Negative
+- The project takes on long-term maintenance for another custom analyzer, including updates if supported write/read APIs or repository linter infrastructure evolve.
+- A block-level event model may still miss some control-flow-sensitive cases or produce behavior that needs future refinement as real code patterns appear.
+- Contributors now face another lint rule that may require code changes or explicit suppression in edge cases.
+
+#### Neutral
+- The implementation follows the repository's existing analyzer utility, generated-file skipping, logging, and `nolint` integration patterns.
+- The rule is added as a new package plus registry wiring, without changing the broader linter execution architecture.
+- The PR expands testdata-based analyzer coverage alongside the production linter code.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/linters/README.md

@@ -8,6 +8,7 @@ This package currently provides custom Go analyzers in the following subpackages
 
 - `appendbytestring` — reports `append(b, []byte(s)...)` calls where `b` is `[]byte` and `s` is a string, which can be simplified to `append(b, s...)`.
 - `appendoneelement` — reports `append(s, []T{x}...)` calls where a single-element slice literal is spread and can be simplified to `append(s, x)`.
+- `bufferresetbeforereuse` — reports `bytes.Buffer` or `strings.Builder` writes after a read without an intervening `Reset()`, which can accumulate stale content.
 - `bytescomparestring` — reports `string(a) == string(b)` and `string(a) != string(b)` comparisons where `a` and `b` are `[]byte` values; use `bytes.Equal(a, b)` for `==` and `!bytes.Equal(a, b)` for `!=`.
 - `bytesbufferstring` — reports `string(buf.Bytes())` calls where `buf` is a `bytes.Buffer` value receiver, suggesting `buf.String()` instead.
 - `contextcancelnotdeferred` — reports context cancel functions that are called directly instead of deferred.
@@ -103,6 +104,7 @@ environment variable and gates findings on the recorded execution hit count for
 |------------|-------------|
 | `appendbytestring` | Custom `go/analysis` analyzer that flags `append(b, []byte(s)...)` calls where `s` is a string that can be simplified to `append(b, s...)` |
 | `appendoneelement` | Custom `go/analysis` analyzer that flags `append(s, []T{x}...)` calls where a single-element slice literal is spread and can be simplified to `append(s, x)` |
+| `bufferresetbeforereuse` | Custom `go/analysis` analyzer that flags `bytes.Buffer` or `strings.Builder` writes after a read without an intervening `Reset()` |
 | `bytescomparestring` | Custom `go/analysis` analyzer that flags `string(a) == string(b)` / `!=` comparisons on `[]byte` values; use `bytes.Equal(a, b)` for `==` and `!bytes.Equal(a, b)` for `!=` |
 | `bytesbufferstring` | Custom `go/analysis` analyzer that flags `string(buf.Bytes())` calls where `buf` is a `bytes.Buffer` value and suggests `buf.String()` instead |
 | `contextcancelnotdeferred` | Custom `go/analysis` analyzer that flags context cancel functions called directly instead of deferred |

pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse.go

@@ -0,0 +1,260 @@
+// Package bufferresetbeforereuse implements a Go analysis linter that flags
+// reuse of bytes.Buffer or strings.Builder without calling Reset() between writes,
+// which can accumulate previous data.
+package bufferresetbeforereuse
+
+import (
+	"go/ast"
+	"go/token"
+	"go/types"
+
+	"golang.org/x/tools/go/analysis"
+
+	"github.com/github/gh-aw/pkg/linters/internal/analyzerutil"
+	"github.com/github/gh-aw/pkg/linters/internal/filecheck"
+	"github.com/github/gh-aw/pkg/linters/internal/nolint"
+	"github.com/github/gh-aw/pkg/logger"
+)
+
+var pkgLog = logger.New("linters:bufferresetbeforereuse")
+
+// Analyzer is the buffer-reset-before-reuse analysis pass.
+var Analyzer = analyzerutil.New("bufferresetbeforereuse", "reports reuse of bytes.Buffer or strings.Builder without calling Reset() between writes, which can accumulate previous data", run)
+
+func run(pass *analysis.Pass) (any, error) {
+	pkgLog.Printf("analyzing package %s", pass.Pkg.Path())
+
+	noLintIndex, generatedFiles, err := analyzerutil.Indexes(pass)
+	if err != nil {
+		return nil, err
+	}
+
+	// Process function declarations and function literals
+	nodeFilter := []ast.Node{(*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)}
+	return analyzerutil.Preorder(pass, nodeFilter, func(n ast.Node) {
+		checkBufferReuse(pass, n, generatedFiles, noLintIndex)
+	})
+}
+
+// checkBufferReuse analyzes a function body for buffer/builder reuse without Reset.
+func checkBufferReuse(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) {
+	var body *ast.BlockStmt
+	switch fn := n.(type) {
+	case *ast.FuncDecl:
+		body = fn.Body
+	case *ast.FuncLit:
+		body = fn.Body
+	default:
+		return
+	}
+
+	if body == nil {
+		return
+	}
+
+	pos := pass.Fset.PositionFor(body.Pos(), false)
+	if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) {
+		return
+	}
+
+	// Analyze the function body for buffer write patterns
+	analyzeBlockForBufferReuse(pass, body, generatedFiles, noLintIndex)
+}
+
+// event represents a write, read, or reset operation on a buffer/builder
+type event struct {
+	varName string
+	obj     types.Object
+	typ     string // "write", "read", or "reset"
+	pos     token.Pos
+	node    ast.Node
+}
+
+// analyzeBlockForBufferReuse examines a block statement for improper buffer reuse
+func analyzeBlockForBufferReuse(pass *analysis.Pass, block *ast.BlockStmt, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) {
+	ast.Inspect(block, func(n ast.Node) bool {
+		switch node := n.(type) {
+		case *ast.FuncLit:
+			return false
+		case *ast.BlockStmt:
+			analyzeStraightLineBlock(pass, node, generatedFiles, noLintIndex)
+		}
+		return true
+	})
+}
+
+func analyzeStraightLineBlock(pass *analysis.Pass, block *ast.BlockStmt, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) {
+	// Collect all events (writes, reads, resets) in order
+	var events []*event
+	collectEvents(pass, block, &events)
+
+	// For each variable, check if writes follow reads without Reset
+	written := make(map[types.Object]bool) // has this variable been written?
+	read := make(map[types.Object]bool)    // has this variable been read?
+
+	for _, e := range events {
+		switch e.typ {
+		case "write":
+			if read[e.obj] {
+				// This is a write after the buffer has been read, without Reset
+				pkgLog.Printf("flagging %s reuse at line %d", e.varName, pass.Fset.Position(e.pos).Line)
+
+				// Check nolint directive
+				filePos := pass.Fset.PositionFor(e.pos, false)
+				if filecheck.ShouldSkipFilename(filePos.Filename, generatedFiles) {
+					continue
+				}
+				if nolint.HasDirectiveForLinter(filePos, noLintIndex, "bufferresetbeforereuse") {
+					pkgLog.Printf("suppressed diagnostic for %s at line %d", e.varName, filePos.Line)
+					continue
+				}
+
+				pass.ReportRangef(
+					e.node,
+					"%s is reused without calling Reset() between writes, which can accumulate previous data",
+					e.varName,
+				)
+			}
+			written[e.obj] = true
+
+		case "read":
+			if written[e.obj] {
+				read[e.obj] = true
+			}
+
+		case "reset":
+			read[e.obj] = false
+			written[e.obj] = false
+		}
+	}
+}
+
+// collectEvents collects all write, read, and reset operations on buffers in order
+func collectEvents(pass *analysis.Pass, block *ast.BlockStmt, events *[]*event) {
+	ast.Inspect(block, func(n ast.Node) bool {
+		if n != block {
+			switch n.(type) {
+			case *ast.BlockStmt, *ast.FuncLit, *ast.IfStmt, *ast.ForStmt, *ast.RangeStmt, *ast.SwitchStmt, *ast.TypeSwitchStmt, *ast.SelectStmt:
+				return false
+			}
+		}
+
+		if _, ok := n.(*ast.FuncLit); ok {
+			return false
+		}
+
+		call, ok := n.(*ast.CallExpr)
+		if !ok {
+			return true
+		}
+
+		appendCallEvent(pass, call, events)
+
+		return true
+	})
+}
+
+func appendCallEvent(pass *analysis.Pass, call *ast.CallExpr, events *[]*event) {
+	sel, ok := call.Fun.(*ast.SelectorExpr)
+	if !ok {
+		return
+	}
+
+	ident, ok := sel.X.(*ast.Ident)
+	if !ok {
+		return
+	}
+
+	obj := bufferOrBuilderObject(pass, ident)
+	if obj == nil {
+		return
+	}
+
+	appendMethodEvent(pass, call, ident.Name, obj, sel.Sel.Name, events)
+}
+
+func appendMethodEvent(pass *analysis.Pass, call *ast.CallExpr, varName string, obj types.Object, methodName string, events *[]*event) {
+	switch {
+	case methodName == "Reset":
+		*events = append(*events, newEvent(varName, obj, "reset", call))
+		pkgLog.Printf("found reset on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line)
+	case isWriteMethod(methodName):
+		*events = append(*events, newEvent(varName, obj, "write", call))
+		pkgLog.Printf("found write on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line)
+	case isReadMethod(methodName):
+		*events = append(*events, newEvent(varName, obj, "read", call))
+		pkgLog.Printf("found read on %s at line %d", varName, pass.Fset.Position(call.Pos()).Line)
+	}
+}
+
+func newEvent(varName string, obj types.Object, typ string, call *ast.CallExpr) *event {
+	return &event{
+		varName: varName,
+		obj:     obj,
+		typ:     typ,
+		pos:     call.Pos(),
+		node:    call,
+	}
+}
+
+// bufferOrBuilderObject returns the object for identifiers that refer to a bytes.Buffer or strings.Builder variable.
+func bufferOrBuilderObject(pass *analysis.Pass, ident *ast.Ident) types.Object {
+	obj := pass.TypesInfo.ObjectOf(ident)
+	if obj == nil {
+		return nil
+	}
+
+	t := obj.Type()
+	if t == nil {
+		return nil
+	}
+
+	// Handle pointer types
+	if ptr, ok := t.(*types.Pointer); ok {
+		t = ptr.Elem()
+	}
+
+	// Check if it's bytes.Buffer or strings.Builder
+	if isNamedType(t, "bytes", "Buffer") || isNamedType(t, "strings", "Builder") {
+		return obj
+	}
+	return nil
+}
+
+// isNamedType checks if a type is named type from specified package and name
+func isNamedType(t types.Type, pkgPath, name string) bool {
+	named, ok := t.(*types.Named)
+	if !ok {
+		return false
+	}
+
+	obj := named.Obj()
+	if obj == nil {
+		return false
+	}
+
+	pkg := obj.Pkg()
+	if pkg == nil {
+		return false
+	}
+
+	return pkg.Path() == pkgPath && obj.Name() == name
+}
+
+// isWriteMethod checks if a method is a write operation
+func isWriteMethod(name string) bool {
+	switch name {
+	case "Write", "WriteString", "WriteRune", "WriteByte":
+		return true
+	}
+	return false
+}
+
+// isReadMethod checks if a method is a read operation
+func isReadMethod(name string) bool {
+	switch name {
+	case "String", "Bytes", "Len":
+		return true
+	}
+	return false
+}

pkg/linters/bufferresetbeforereuse/bufferresetbeforereuse_test.go

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