[linter-miner] Add file-close-not-deferred linter

cmd/linters/main.go

@@ -20,6 +20,7 @@ import (
 	"github.com/github/gh-aw/pkg/linters/errormessage"
 	"github.com/github/gh-aw/pkg/linters/errstringmatch"
 	"github.com/github/gh-aw/pkg/linters/excessivefuncparams"
+	"github.com/github/gh-aw/pkg/linters/fileclosenotdeferred"
 	"github.com/github/gh-aw/pkg/linters/largefunc"
 	"github.com/github/gh-aw/pkg/linters/osexitinlibrary"
 	"github.com/github/gh-aw/pkg/linters/rawloginlib"
@@ -33,6 +34,7 @@ func main() {
 		errormessage.Analyzer,
 		errstringmatch.Analyzer,
 		excessivefuncparams.Analyzer,
+		fileclosenotdeferred.Analyzer,
 		largefunc.Analyzer,
 		osexitinlibrary.Analyzer,
 		rawloginlib.Analyzer,

docs/adr/33834-add-file-close-not-deferred-linter.md

@@ -0,0 +1,84 @@
+# ADR-33834: Add file-close-not-deferred Linter
+
+**Date**: 2026-05-21
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+An automated scan of the codebase (linter-miner run #12) found 3 instances where files opened via `os.Open` / `os.OpenFile` are closed manually instead of with `defer file.Close()` — `pkg/cli/workflows.go:307`, `pkg/cli/workflows.go:396`, and `pkg/cli/mcp_logs_guardrail.go:87`. Manual close is the canonical Go anti-pattern for file handles: early returns, panics, or new branches added later between open and close cause descriptor leaks, and forgetting `defer` is easy to miss in review. The repository already houses a family of small, focused, in-house analyzers under `pkg/linters/` (e.g., `largefunc`, `osexitinlibrary`, `regexpcompileinfunction`) registered through `cmd/linters/main.go`, so the established convention is to add another analyzer rather than rely on review discipline or external tooling.
+
+### Decision
+
+We will add a new static-analysis linter, `fileclosenotdeferred`, that flags functions where a file opened by `os.Open`, `os.Create`, or `os.OpenFile` is closed via a non-deferred `Close()` call. The linter lives under `pkg/linters/fileclosenotdeferred/`, is registered in `cmd/linters/main.go` alongside the existing analyzers, walks each `*ast.FuncDecl` body (excluding nested `*ast.FuncLit` closures to avoid false positives) tracking per-variable state keyed by `types.Object` (to correctly handle variable shadowing), and reports the open position when a variable has a manual `Close()` and no matching `defer`. Test files are excluded via the shared `pkg/linters/internal/filecheck.IsTestFile` helper.
+
+### Alternatives Considered
+
+#### Alternative 1: Fix the 3 known instances and rely on review
+
+Patch the three flagged call sites to use `defer file.Close()` and trust reviewers to catch new instances. Rejected because the same anti-pattern was already present in three independent files, indicating that human review alone has not been sufficient; a mechanical check on every PR is cheaper and cannot be forgotten as the codebase grows.
+
+#### Alternative 2: Use a third-party linter (e.g., `gocritic`, `bodyclose`)
+
+General-purpose Go linters offer resource-leak rules with broader coverage. Rejected to stay consistent with the project's convention of small, focused, in-house analyzers under `pkg/linters/`, each as its own package with custom logic. Pulling in an external linter for a single rule introduces a new dependency surface, inconsistent rule configuration, and noise from rules the project has not opted into.
+
+#### Alternative 3: Combine with a broader "resource hygiene" linter
+
+Bundle this rule with future checks (e.g., `http.Response.Body` close, `sql.Rows` close, `io.Closer` in general) into one analyzer. Rejected because the existing convention is one analyzer per rule, which keeps each rule's `Analyzer.Doc` URL narrow, independently disable-able, and easy to extend without coupling unrelated checks.
+
+### Consequences
+
+#### Positive
+- New non-deferred `Close()` patterns introduced after merge are caught by `make golint-custom` and fail in CI rather than landing on `main`.
+- The linter follows the same `pkg/linters/<name>/` layout, `Analyzer` shape, and `testdata` convention as the sibling analyzers, so contributors can extend it without learning a new pattern.
+- Creates incentive to clean up the 3 pre-existing manual-close sites in `pkg/cli/` to maintain a clean linter signal.
+
+#### Negative
+- Detection is structural and intentionally narrow: it matches only direct calls to `os.Open` / `os.Create` / `os.OpenFile` and direct `<var>.Close()` calls. It will miss aliased imports (`import o "os"`), files returned from helper functions, and `Close()` called on a field or selector. False negatives are accepted in exchange for zero false positives.
+- The 3 pre-existing violations are not fixed in this PR, so the linter cannot be made a blocking CI gate without follow-up work or suppression.
+- Adds one more analyzer to the registry, marginally increasing `cmd/linters/main.go` compile and run time.
+
+#### Neutral
+- Test files are deliberately excluded via `filecheck.IsTestFile`, matching the convention used by the sibling linters. Test fixtures may legitimately close files inline.
+- The `hasManuaClose` typo has been corrected to `hasManualClose`; the state struct is internal only.
+- The diagnostic reports at the open position, not the manual-close position, so the warning points the reader to the place where `defer` should be inserted.
+
+---
+
+## Part 2 — Normative Specification (RFC 2119)
+
+> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
+
+### Linter Behaviour
+
+1. The analyzer **MUST** be exported as `fileclosenotdeferred.Analyzer` with `Name` equal to `"fileclosenotdeferred"`.
+2. The analyzer **MUST** report a diagnostic for every local variable assigned from a call to `os.Open`, `os.Create`, or `os.OpenFile` when that variable's `Close()` method is invoked without a matching `defer` statement in the same `*ast.FuncDecl` body.
+3. The analyzer **MUST NOT** report a diagnostic when the file variable's `Close()` is invoked via a `defer` statement anywhere in the same function body.
+4. The analyzer **MUST NOT** report a diagnostic when the result of the open call is discarded with the blank identifier (`_`).
+5. The analyzer **MUST NOT** report a diagnostic when the containing file is a Go test file as determined by `pkg/linters/internal/filecheck.IsTestFile`.
+6. The diagnostic `Pos` **MUST** be the position of the originating open call (`os.Open` / `os.Create` / `os.OpenFile`), not the position of the manual `Close()`.
+7. The diagnostic `Message` **SHOULD** read `"file Close() should be deferred immediately after successful open to prevent resource leaks"` so downstream tooling can match on a stable string.
+8. The analyzer **MUST** declare `inspect.Analyzer` in its `Requires` list.
+
+### Registration
+
+1. The analyzer **MUST** be registered in `cmd/linters/main.go` via the `multichecker.Main` argument list alongside the existing custom analyzers.
+2. The package import in `cmd/linters/main.go` **MUST** use the path `github.com/github/gh-aw/pkg/linters/fileclosenotdeferred`.
+
+### Package Layout
+
+1. The linter source **MUST** live under `pkg/linters/fileclosenotdeferred/`.
+2. Test fixtures **MUST** live under `pkg/linters/fileclosenotdeferred/testdata/src/fileclosenotdeferred/` and **MUST** use `// want` comments compatible with `golang.org/x/tools/go/analysis/analysistest`.
+3. The test fixtures **MUST** include at least one positive case (manual `Close()` flagged), one negative case (`defer file.Close()` not flagged), and one blank-identifier case (`_, err := os.Open(...)` not flagged).
+
+### Conformance
+
+An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26245378772) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/linters/fileclosenotdeferred/fileclosenotdeferred.go

@@ -0,0 +1,169 @@
+// Package fileclosenotdeferred implements a Go analysis linter that flags
+// file operations where Close() is not immediately deferred.
+package fileclosenotdeferred
+
+import (
+	"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/filecheck"
+)
+
+// Analyzer is the file-close-not-deferred analysis pass.
+var Analyzer = &analysis.Analyzer{
+	Name:     "fileclosenotdeferred",
+	Doc:      "reports file operations where Close() is not immediately deferred, which can lead to resource leaks",
+	URL:      "https://github.com/github/gh-aw/tree/main/pkg/linters/fileclosenotdeferred",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	nodeFilter := []ast.Node{
+		(*ast.FuncDecl)(nil),
+	}
+
+	insp.Preorder(nodeFilter, func(n ast.Node) {
+		fn, ok := n.(*ast.FuncDecl)
+		if !ok || fn.Body == nil {
+			return
+		}
+
+		pos := pass.Fset.PositionFor(fn.Pos(), false)
+		if filecheck.IsTestFile(pos.Filename) {
+			return
+		}
+
+		// Track file variables: types.Object -> *fileVarState (open position, hasDefer, hasManualClose)
+		// Keyed by types.Object so variable shadowing across inner scopes is handled correctly.
+		fileVars := make(map[types.Object]*fileVarState)
+
+		// Walk all statements in the function body, including nested blocks,
+		// but stop at function literals so closures are analysed independently.
+		ast.Inspect(fn.Body, func(node ast.Node) bool {
+			if node == nil {
+				return false
+			}
+
+			// Do not descend into function literals — closures are independent execution
+			// contexts and should be analyzed separately to avoid false positives.
+			if _, ok := node.(*ast.FuncLit); ok {
+				return false
+			}
+
+			// Look for assignments like: file, err := os.Open(...)
+			if assign, ok := node.(*ast.AssignStmt); ok {
+				for i, rhs := range assign.Rhs {
+					if call, ok := rhs.(*ast.CallExpr); ok && isFileOpenCall(call) {
+						if i < len(assign.Lhs) {
+							if ident, ok := assign.Lhs[i].(*ast.Ident); ok && ident.Name != "_" {
+								obj := pass.TypesInfo.ObjectOf(ident)
+								if obj != nil {
+									// If this object was already tracked from a prior open on the
+									// same binding (plain = reassignment), report any unresolved
+									// violation immediately before overwriting the state.
+									if prev, exists := fileVars[obj]; exists && prev.hasManualClose && !prev.hasDefer {
+										pass.Report(analysis.Diagnostic{
+											Pos:     prev.openPos,
+											Message: "file Close() should be deferred immediately after successful open to prevent resource leaks",
+										})
+									}
+									fileVars[obj] = &fileVarState{
+										openPos: call.Pos(),
+									}
+								}
+							}
+						}
+					}
+				}
+			}
+
+			// Look for defer file.Close()
+			if deferStmt, ok := node.(*ast.DeferStmt); ok {
+				if obj := getCloseCallObj(pass, deferStmt.Call); obj != nil {
+					if state, found := fileVars[obj]; found {
+						state.hasDefer = true
+					}
+				}
+			}
+
+			// Look for non-deferred file.Close() in expression statements
+			if exprStmt, ok := node.(*ast.ExprStmt); ok {
+				if call, ok := exprStmt.X.(*ast.CallExpr); ok {
+					if obj := getCloseCallObj(pass, call); obj != nil {
+						if state, found := fileVars[obj]; found {
+							state.hasManualClose = true
+						}
+					}
+				}
+			}
+
+			// Look for non-deferred file.Close() in assignments (e.g., closeErr := fd.Close())
+			if assign, ok := node.(*ast.AssignStmt); ok {
+				for _, rhs := range assign.Rhs {
+					if call, ok := rhs.(*ast.CallExpr); ok {
+						if obj := getCloseCallObj(pass, call); obj != nil {
+							if state, found := fileVars[obj]; found {
+								state.hasManualClose = true
+							}
+						}
+					}
+				}
+			}
+
+			return true
+		})
+
+		// Report files with manual close but no defer
+		for _, state := range fileVars {
+			if state.hasManualClose && !state.hasDefer {
+				pass.Report(analysis.Diagnostic{
+					Pos:     state.openPos,
+					Message: "file Close() should be deferred immediately after successful open to prevent resource leaks",
+				})
+			}
+		}
+	})
+
+	return nil, nil
+}
+
+type fileVarState struct {
+	openPos        token.Pos
+	hasDefer       bool
+	hasManualClose bool
+}
+
+// isFileOpenCall returns true if the call is os.Open, os.Create, or os.OpenFile
+func isFileOpenCall(call *ast.CallExpr) bool {
+	sel, ok := call.Fun.(*ast.SelectorExpr)
+	if !ok {
+		return false
+	}
+	ident, ok := sel.X.(*ast.Ident)
+	if !ok || ident.Name != "os" {
+		return false
+	}
+	return sel.Sel.Name == "Open" || sel.Sel.Name == "Create" || sel.Sel.Name == "OpenFile"
+}
+
+// getCloseCallObj returns the types.Object for the receiver if call is like file.Close(),
+// enabling correct identification across variable shadowing.
+func getCloseCallObj(pass *analysis.Pass, call *ast.CallExpr) types.Object {
+	sel, ok := call.Fun.(*ast.SelectorExpr)
+	if !ok || sel.Sel.Name != "Close" {
+		return nil
+	}
+	ident, ok := sel.X.(*ast.Ident)
+	if !ok {
+		return nil
+	}
+	return pass.TypesInfo.ObjectOf(ident)
+}

pkg/linters/fileclosenotdeferred/fileclosenotdeferred_test.go

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