From 3e5e80d08e1447ab501bebf8d34c62f269b2bc4f Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 18 Jun 2026 18:36:05 +0000 Subject: [PATCH 1/4] linter: add httpresponsebodyclose analyzer Adds a new custom Go analysis linter that flags HTTP response Body.Close() calls that are not deferred. Deferring the close is the idiomatic Go pattern because it ensures the body is closed even when the function returns early due to an error or panic. The linter mirrors the existing fileclosenotdeferred linter in structure: - Walks FuncDecl nodes - Tracks variables of type *net/http.Response - Detects manual resp.Body.Close() calls (including if-init assignments) - Reports when a manual close exists but no defer close is present Real finding in this repo: pkg/cli/mcp_inspect_mcp_scripts_server.go:67 where resp.Body.Close() is called inside an if-init assignment without defer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/linters/main.go | 2 + .../httpresponsebodyclose.go | 189 ++++++++++++++++++ .../httpresponsebodyclose_test.go | 16 ++ .../httpresponsebodyclose.go | 53 +++++ 4 files changed, 260 insertions(+) create mode 100644 pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go create mode 100644 pkg/linters/httpresponsebodyclose/httpresponsebodyclose_test.go create mode 100644 pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go diff --git a/cmd/linters/main.go b/cmd/linters/main.go index fd3486fe000..5b568945da9 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -28,6 +28,7 @@ import ( "github.com/github/gh-aw/pkg/linters/fprintlnsprintf" "github.com/github/gh-aw/pkg/linters/hardcodedfilepath" "github.com/github/gh-aw/pkg/linters/httpnoctx" + "github.com/github/gh-aw/pkg/linters/httpresponsebodyclose" "github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror" "github.com/github/gh-aw/pkg/linters/largefunc" "github.com/github/gh-aw/pkg/linters/lenstringzero" @@ -61,6 +62,7 @@ func main() { fmterrorfnoverbs.Analyzer, hardcodedfilepath.Analyzer, httpnoctx.Analyzer, + httpresponsebodyclose.Analyzer, largefunc.Analyzer, manualmutexunlock.Analyzer, osexitinlibrary.Analyzer, diff --git a/pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go b/pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go new file mode 100644 index 00000000000..c0e34cb7542 --- /dev/null +++ b/pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go @@ -0,0 +1,189 @@ +// Package httpresponsebodyclose implements a Go analysis linter that flags +// HTTP response bodies that are closed manually instead of via defer. +package httpresponsebodyclose + +import ( + "go/ast" + "go/token" + "go/types" + + "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 http-response-body-close analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "httpresponsebodyclose", + Doc: "reports HTTP response Body.Close() calls that are not deferred, which can cause resource leaks on early return or panic", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/httpresponsebodyclose", + 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.FuncDecl)(nil), + } + + insp.Preorder(nodeFilter, func(n ast.Node) { + inspectHTTPResponseFuncDecl(pass, n) + }) + + return nil, nil +} + +type responseVarState struct { + assignPos token.Pos + hasDefer bool + hasManualClose bool +} + +func inspectHTTPResponseFuncDecl(pass *analysis.Pass, 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 + } + + responseVars := make(map[types.Object]*responseVarState) + + ast.Inspect(fn.Body, func(node ast.Node) bool { + return analyzeResponseNode(pass, responseVars, node) + }) + + for _, state := range responseVars { + if state.hasManualClose && !state.hasDefer { + pass.Report(analysis.Diagnostic{ + Pos: state.assignPos, + Message: "HTTP response Body.Close() should be deferred immediately after error check to prevent resource leaks", + }) + } + } +} + +func analyzeResponseNode(pass *analysis.Pass, responseVars map[types.Object]*responseVarState, 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 + } + + if assign, ok := node.(*ast.AssignStmt); ok { + trackResponseAssignment(pass, responseVars, assign) + } + + if deferStmt, ok := node.(*ast.DeferStmt); ok { + if obj := getHTTPBodyCloseObject(pass, deferStmt.Call); obj != nil { + if state, found := responseVars[obj]; found { + state.hasDefer = true + } + } + } + + // Non-deferred resp.Body.Close() in expression statements. + if exprStmt, ok := node.(*ast.ExprStmt); ok { + if call, ok := exprStmt.X.(*ast.CallExpr); ok { + markManualBodyClose(pass, responseVars, call) + } + } + + // Non-deferred resp.Body.Close() captured in an assignment (e.g. closeErr := resp.Body.Close()). + if assign, ok := node.(*ast.AssignStmt); ok { + for _, rhs := range assign.Rhs { + if call, ok := rhs.(*ast.CallExpr); ok { + markManualBodyClose(pass, responseVars, call) + } + } + } + + return true +} + +func trackResponseAssignment(pass *analysis.Pass, responseVars map[types.Object]*responseVarState, assign *ast.AssignStmt) { + for _, lhs := range assign.Lhs { + ident, ok := lhs.(*ast.Ident) + if !ok || ident.Name == "_" { + continue + } + obj := pass.TypesInfo.ObjectOf(ident) + if obj == nil { + continue + } + if !isHTTPResponseType(obj.Type()) { + continue + } + if prev, exists := responseVars[obj]; exists && prev.hasManualClose && !prev.hasDefer { + pass.Report(analysis.Diagnostic{ + Pos: prev.assignPos, + Message: "HTTP response Body.Close() should be deferred immediately after error check to prevent resource leaks", + }) + } + responseVars[obj] = &responseVarState{assignPos: assign.Pos()} + } +} + +func markManualBodyClose(pass *analysis.Pass, responseVars map[types.Object]*responseVarState, call *ast.CallExpr) { + obj := getHTTPBodyCloseObject(pass, call) + if obj == nil { + return + } + if state, found := responseVars[obj]; found { + state.hasManualClose = true + } +} + +// getHTTPBodyCloseObject returns the types.Object for the *http.Response variable +// in a resp.Body.Close() call, or nil if the call does not match that pattern. +func getHTTPBodyCloseObject(pass *analysis.Pass, call *ast.CallExpr) types.Object { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Close" { + return nil + } + // The receiver must be an expression of the form resp.Body. + bodyExpr, ok := sel.X.(*ast.SelectorExpr) + if !ok || bodyExpr.Sel.Name != "Body" { + return nil + } + ident, ok := bodyExpr.X.(*ast.Ident) + if !ok { + return nil + } + obj := pass.TypesInfo.ObjectOf(ident) + if obj == nil { + return nil + } + if !isHTTPResponseType(obj.Type()) { + return nil + } + return obj +} + +// isHTTPResponseType reports whether t is *net/http.Response. +func isHTTPResponseType(t types.Type) bool { + ptr, ok := t.(*types.Pointer) + if !ok { + return false + } + named, ok := ptr.Elem().(*types.Named) + if !ok { + return false + } + obj := named.Obj() + return obj.Name() == "Response" && obj.Pkg() != nil && obj.Pkg().Path() == "net/http" +} diff --git a/pkg/linters/httpresponsebodyclose/httpresponsebodyclose_test.go b/pkg/linters/httpresponsebodyclose/httpresponsebodyclose_test.go new file mode 100644 index 00000000000..c18820b5915 --- /dev/null +++ b/pkg/linters/httpresponsebodyclose/httpresponsebodyclose_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package httpresponsebodyclose_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/httpresponsebodyclose" +) + +func TestHTTPResponseBodyClose(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, httpresponsebodyclose.Analyzer, "httpresponsebodyclose") +} diff --git a/pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go b/pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go new file mode 100644 index 00000000000..18a033a16e4 --- /dev/null +++ b/pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go @@ -0,0 +1,53 @@ +package httpresponsebodyclose + +import ( + "io" + "net/http" +) + +// flagged: Body.Close() called manually, not deferred. +func fetchManualClose(url string) error { + req, _ := http.NewRequest(http.MethodGet, url, nil) + resp, err := http.DefaultClient.Do(req) // want `HTTP response Body\.Close\(\) should be deferred immediately after error check to prevent resource leaks` + if err != nil { + return err + } + _, _ = io.ReadAll(resp.Body) + resp.Body.Close() + return nil +} + +// not flagged: Body.Close() is deferred correctly. +func fetchDeferred(url string) error { + req, _ := http.NewRequest(http.MethodGet, url, nil) + resp, err := http.DefaultClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + _, _ = io.ReadAll(resp.Body) + return nil +} + +// flagged: Body.Close() return value captured but not deferred. +func fetchManualCloseWithErr(url string) error { + req, _ := http.NewRequest(http.MethodGet, url, nil) + resp, err := http.DefaultClient.Do(req) // want `HTTP response Body\.Close\(\) should be deferred immediately after error check to prevent resource leaks` + if err != nil { + return err + } + _, _ = io.ReadAll(resp.Body) + closeErr := resp.Body.Close() + return closeErr +} + +// not flagged: no Body.Close() call at all (different concern, out of scope). +func fetchNoClose(url string) error { + req, _ := http.NewRequest(http.MethodGet, url, nil) + resp, err := http.DefaultClient.Do(req) + if err != nil { + return err + } + _, _ = io.ReadAll(resp.Body) + return nil +} From 55174e1d8ef03b19c9d5de0695a9f0dcc21f0134 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 18 Jun 2026 21:08:47 +0000 Subject: [PATCH 2/4] docs(adr): add draft ADR-40130 for httpresponsebodyclose linter Co-Authored-By: Claude Opus 4.8 (1M context) --- ...linter-for-non-deferred-http-body-close.md | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 docs/adr/40130-custom-linter-for-non-deferred-http-body-close.md diff --git a/docs/adr/40130-custom-linter-for-non-deferred-http-body-close.md b/docs/adr/40130-custom-linter-for-non-deferred-http-body-close.md new file mode 100644 index 00000000000..e7ece1c9197 --- /dev/null +++ b/docs/adr/40130-custom-linter-for-non-deferred-http-body-close.md @@ -0,0 +1,42 @@ +# ADR-40130: Ship a Custom `httpresponsebodyclose` Linter for Non-Deferred HTTP Response Body Close + +**Date**: 2026-06-18 +**Status**: Draft + +## Context + +Calling `resp.Body.Close()` directly — rather than `defer resp.Body.Close()` — risks leaking the underlying connection: if the function returns early on a later error, panics, or gains a new `return` during maintenance, the close never runs. A scan of this repository (run #42) found a real occurrence at `pkg/cli/mcp_inspect_mcp_scripts_server.go:67`, where the body is closed inside an `if`-init assignment without a defer. No commonly enabled linter flags this: `go vet` does not track response-body lifetime, and the standard `bodyclose` linter is not part of this project's enabled set. The project maintains an in-repo `go/analysis` linter suite (`pkg/linters/*`, registered in `cmd/linters/main.go`) that is the established home for such repository-specific correctness guards. + +## Decision + +We will add a bespoke `go/analysis` analyzer, `httpresponsebodyclose`, to the project's custom linter suite rather than adopting an external linter. The analyzer walks each non-test `*ast.FuncDecl`, tracks variables whose type is `*net/http.Response` (verified via `types.Object` identity against `net/http`, not a syntactic name match), and records for each whether its `Body.Close()` is deferred versus called manually (either as a bare expression statement or captured in an assignment such as `closeErr := resp.Body.Close()`). When a response variable has a manual close but no deferred close, it reports a diagnostic at the assignment line. It does not descend into function literals, so closures are analyzed independently to avoid false positives, and it is registered in the multichecker in `cmd/linters/main.go`. + +## Alternatives Considered + +### Alternative 1: Adopt the external `bodyclose` linter +The community `bodyclose` analyzer (via `golangci-lint`) detects unclosed response bodies. Rejected because this project deliberately curates a small, self-contained in-repo linter suite with shared `internal` helpers (`astutil`, `filecheck`); pulling in an external analyzer with broader, differently-scoped semantics would not compose with that suite and would target "never closed" rather than this project's specific "closed but not deferred" concern. + +### Alternative 2: Document the convention and rely on code review +A contributor guideline plus manual review requires no new code. Rejected because the early-return/panic leak is subtle and easy to miss — an instance already exists in the codebase — and manual review provides no repeatable, enforceable guard in CI. + +### Alternative 3: Syntactic detection by identifier name (`resp.Body.Close()`) +A simpler analyzer could flag any `Body.Close()` whose receiver is named `resp`. Rejected because it would miss response variables under other names and false-positive on unrelated types that happen to expose a `Body.Close()`; type-based `*net/http.Response` identity checking is more precise at modest extra complexity. + +## Consequences + +### Positive +- Catches a real, otherwise-undetected resource-leak pattern automatically in CI. +- Follows the established in-repo linter convention, mirroring `fileclosenotdeferred` in structure, so it composes with the existing `cmd/linters` multichecker and shared `internal` helpers. +- Precise: type-identity checking against `net/http.Response` targets genuine response bodies while ignoring same-named methods on unrelated types. + +### Negative +- Adds a custom analyzer the team must maintain, including AST handling for both expression-statement and assignment-captured close calls. +- Intra-procedural and pattern-bound: it only recognizes the `resp.Body.Close()` receiver shape and does not follow a body passed across function boundaries or aliased, so some manual-close leaks remain uncaught — which may create a false sense of full coverage. + +### Neutral +- Test files are excluded from analysis, matching the suite's existing conventions. +- The analyzer reports at the assignment line (where the fix — inserting `defer` — belongs), not at the manual `Close()` call site. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27789122704) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* From 03535714a2a69fb69d1b0e94c2973c23f7d009ed Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 18 Jun 2026 21:46:08 +0000 Subject: [PATCH 3/4] Address linter review feedback Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- ...linter-for-non-deferred-http-body-close.md | 4 +- pkg/linters/README.md | 5 +++ pkg/linters/doc.go | 3 +- .../httpresponsebodyclose.go | 15 ++++++- .../httpresponsebodyclose.go | 45 +++++++++++++++++++ pkg/linters/spec_test.go | 9 ++-- 6 files changed, 74 insertions(+), 7 deletions(-) diff --git a/docs/adr/40130-custom-linter-for-non-deferred-http-body-close.md b/docs/adr/40130-custom-linter-for-non-deferred-http-body-close.md index e7ece1c9197..7087f7e18cd 100644 --- a/docs/adr/40130-custom-linter-for-non-deferred-http-body-close.md +++ b/docs/adr/40130-custom-linter-for-non-deferred-http-body-close.md @@ -9,7 +9,7 @@ Calling `resp.Body.Close()` directly — rather than `defer resp.Body.Close()` ## Decision -We will add a bespoke `go/analysis` analyzer, `httpresponsebodyclose`, to the project's custom linter suite rather than adopting an external linter. The analyzer walks each non-test `*ast.FuncDecl`, tracks variables whose type is `*net/http.Response` (verified via `types.Object` identity against `net/http`, not a syntactic name match), and records for each whether its `Body.Close()` is deferred versus called manually (either as a bare expression statement or captured in an assignment such as `closeErr := resp.Body.Close()`). When a response variable has a manual close but no deferred close, it reports a diagnostic at the assignment line. It does not descend into function literals, so closures are analyzed independently to avoid false positives, and it is registered in the multichecker in `cmd/linters/main.go`. +We will add a bespoke `go/analysis` analyzer, `httpresponsebodyclose`, to the project's custom linter suite rather than adopting an external linter. The analyzer walks each non-test `*ast.FuncDecl`, tracks variables whose type is `*net/http.Response` (verified via `types.Object` identity against `net/http`, not a syntactic name match), and records for each whether its `Body.Close()` is deferred versus called manually (either as a bare expression statement or captured in an assignment such as `closeErr := resp.Body.Close()`). When a response variable has a manual close but no deferred close, it reports a diagnostic at the response-acquisition site so the warning highlights where `defer resp.Body.Close()` should be added. It does not descend into function literals, so closures are analyzed independently to avoid false positives, and it is registered in the multichecker in `cmd/linters/main.go`. ## Alternatives Considered @@ -35,7 +35,7 @@ A simpler analyzer could flag any `Body.Close()` whose receiver is named `resp`. ### Neutral - Test files are excluded from analysis, matching the suite's existing conventions. -- The analyzer reports at the assignment line (where the fix — inserting `defer` — belongs), not at the manual `Close()` call site. +- The analyzer reports at the response-acquisition site, not at the later manual `Close()` call site. --- diff --git a/pkg/linters/README.md b/pkg/linters/README.md index ef563841a15..373ef59171f 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -18,6 +18,7 @@ This package currently provides custom Go analyzers in the following subpackages - `fprintlnsprintf` — reports `fmt.Fprintln(..., fmt.Sprintf(...))` patterns and recommends direct formatting calls. - `hardcodedfilepath` — reports hard-coded file path string literals that match known path constants or should be extracted into named constants; also annotates paths that appear in log/print calls. - `httpnoctx` — reports HTTP client and package-level HTTP calls that do not accept a `context.Context`. +- `httpresponsebodyclose` — reports HTTP response `Body.Close()` calls that are not deferred immediately after a successful request. - `jsonmarshalignoredeerror` — reports `json.Marshal` and `json.Unmarshal` calls where the error return is discarded. - `largefunc` — reports function bodies that exceed a configurable line-count threshold. - `lenstringzero` — reports `len(s) == 0` / `len(s) != 0` comparisons on string values that should use `s == ""` / `s != ""`. @@ -55,6 +56,7 @@ This package currently provides custom Go analyzers in the following subpackages | `fprintlnsprintf` | Custom `go/analysis` analyzer that flags `fmt.Fprintln(..., fmt.Sprintf(...))` patterns | | `hardcodedfilepath` | Custom `go/analysis` analyzer that flags hard-coded file path string literals that match known path constants or should be extracted as named constants; annotates paths in log/print calls | | `httpnoctx` | Custom `go/analysis` analyzer that flags HTTP calls that do not accept a `context.Context` | +| `httpresponsebodyclose` | Custom `go/analysis` analyzer that flags HTTP response `Body.Close()` calls that are not deferred immediately | | `jsonmarshalignoredeerror` | Custom `go/analysis` analyzer that flags `json.Marshal`/`json.Unmarshal` calls where the error return is discarded | | `largefunc` | Custom `go/analysis` analyzer that flags large functions with actionable diagnostics | | `lenstringzero` | Custom `go/analysis` analyzer that flags `len(s) == 0` / `len(s) != 0` on string values that should use `s == ""` / `s != ""` | @@ -94,6 +96,7 @@ import ( "github.com/github/gh-aw/pkg/linters/largefunc" "github.com/github/gh-aw/pkg/linters/lenstringzero" "github.com/github/gh-aw/pkg/linters/manualmutexunlock" + "github.com/github/gh-aw/pkg/linters/httpresponsebodyclose" "github.com/github/gh-aw/pkg/linters/osexitinlibrary" panicinlibrarycode "github.com/github/gh-aw/pkg/linters/panic-in-library-code" "github.com/github/gh-aw/pkg/linters/rawloginlib" @@ -110,6 +113,7 @@ _ = errstringmatch.Analyzer _ = execcommandwithoutcontext.Analyzer _ = fileclosenotdeferred.Analyzer _ = hardcodedfilepath.Analyzer +_ = httpresponsebodyclose.Analyzer _ = largefunc.Analyzer _ = lenstringzero.Analyzer _ = manualmutexunlock.Analyzer @@ -134,6 +138,7 @@ _ = ssljson.Analyzer - `github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs` — fmt-errorf-no-verbs analyzer subpackage - `github.com/github/gh-aw/pkg/linters/fprintlnsprintf` — fprintln-sprintf analyzer subpackage - `github.com/github/gh-aw/pkg/linters/hardcodedfilepath` — hard-coded-file-path analyzer subpackage +- `github.com/github/gh-aw/pkg/linters/httpresponsebodyclose` — HTTP-response-body-close analyzer subpackage - `github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror` — json-marshal-ignored-error analyzer subpackage - `github.com/github/gh-aw/pkg/linters/largefunc` — large-func analyzer subpackage - `github.com/github/gh-aw/pkg/linters/lenstringzero` — len-string-zero analyzer subpackage diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 0bab5358155..8803302e20b 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,6 +1,6 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// The actual analyzers are implemented in subpackages. All 29 active analyzers: +// The actual analyzers are implemented in subpackages. All 30 active analyzers: // // - contextcancelnotdeferred — flags context cancel functions called directly instead of deferred // - ctxbackground — flags context.Background() inside functions that already receive a context @@ -13,6 +13,7 @@ // - fmterrorfnoverbs — flags fmt.Errorf calls with no format verbs, recommending errors.New // - fprintlnsprintf — flags fmt.Fprintln(..., fmt.Sprintf(...)) patterns // - httpnoctx — flags HTTP calls that do not accept a context.Context +// - httpresponsebodyclose — flags HTTP response Body.Close() calls that are not deferred // - jsonmarshalignoredeerror — flags json.Marshal/Unmarshal calls where the error is discarded with _ // - largefunc — flags function bodies that exceed a configurable line-count threshold // - lenstringzero — flags len(s) == 0 / len(s) != 0 on string values that should use s == "" / s != "" diff --git a/pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go b/pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go index c0e34cb7542..95d451bb35f 100644 --- a/pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go +++ b/pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go @@ -116,6 +116,10 @@ func analyzeResponseNode(pass *analysis.Pass, responseVars map[types.Object]*res } func trackResponseAssignment(pass *analysis.Pass, responseVars map[types.Object]*responseVarState, assign *ast.AssignStmt) { + // Track any binding whose LHS has type *net/http.Response. Unlike + // fileclosenotdeferred (which tracks explicit os.Open/os.Create call sites), + // this intentionally keys on the assigned variable's type so responses + // returned by helper functions are also covered. for _, lhs := range assign.Lhs { ident, ok := lhs.(*ast.Ident) if !ok || ident.Name == "_" { @@ -134,7 +138,13 @@ func trackResponseAssignment(pass *analysis.Pass, responseVars map[types.Object] Message: "HTTP response Body.Close() should be deferred immediately after error check to prevent resource leaks", }) } - responseVars[obj] = &responseVarState{assignPos: assign.Pos()} + assignPos := ident.Pos() + if len(assign.Rhs) == 1 { + if call, ok := assign.Rhs[0].(*ast.CallExpr); ok { + assignPos = call.Pos() + } + } + responseVars[obj] = &responseVarState{assignPos: assignPos} } } @@ -150,6 +160,9 @@ func markManualBodyClose(pass *analysis.Pass, responseVars map[types.Object]*res // getHTTPBodyCloseObject returns the types.Object for the *http.Response variable // in a resp.Body.Close() call, or nil if the call does not match that pattern. +// Known limitation: body aliasing (body := resp.Body; body.Close()) is not +// detected because the selector chain no longer starts from the *http.Response +// variable directly. func getHTTPBodyCloseObject(pass *analysis.Pass, call *ast.CallExpr) types.Object { sel, ok := call.Fun.(*ast.SelectorExpr) if !ok || sel.Sel.Name != "Close" { diff --git a/pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go b/pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go index 18a033a16e4..4471e2385cf 100644 --- a/pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go +++ b/pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go @@ -41,6 +41,51 @@ func fetchManualCloseWithErr(url string) error { return closeErr } +// flagged: Body.Close() captured in if-init, not deferred. +func fetchIfInitClose(url string) error { + req, _ := http.NewRequest(http.MethodGet, url, nil) + resp, err := http.DefaultClient.Do(req) // want `HTTP response Body\.Close\(\) should be deferred immediately after error check to prevent resource leaks` + if err != nil { + return err + } + if closeErr := resp.Body.Close(); closeErr != nil { + return closeErr + } + return nil +} + +// not flagged: closure is analyzed independently and its defer must not be +// attributed to the outer function. +func fetchInClosure(url string) { + fn := func() error { + resp, err := http.DefaultClient.Get(url) + if err != nil { + return err + } + defer resp.Body.Close() + _, _ = io.ReadAll(resp.Body) + return nil + } + _ = fn() +} + +// flagged: same response variable reassigned after a manual close; the first +// response acquisition must still be reported. +func fetchReassignAfterManualClose(url string) error { + resp, err := http.DefaultClient.Get(url) // want `HTTP response Body\.Close\(\) should be deferred immediately after error check to prevent resource leaks` + if err != nil { + return err + } + resp.Body.Close() + resp, err = http.DefaultClient.Get(url + "/retry") + if err != nil { + return err + } + defer resp.Body.Close() + _, _ = io.ReadAll(resp.Body) + return nil +} + // not flagged: no Body.Close() call at all (different concern, out of scope). func fetchNoClose(url string) error { req, _ := http.NewRequest(http.MethodGet, url, nil) diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index f53401eb72d..c485873d1fe 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -23,6 +23,7 @@ import ( "github.com/github/gh-aw/pkg/linters/fprintlnsprintf" "github.com/github/gh-aw/pkg/linters/hardcodedfilepath" "github.com/github/gh-aw/pkg/linters/httpnoctx" + "github.com/github/gh-aw/pkg/linters/httpresponsebodyclose" "github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror" "github.com/github/gh-aw/pkg/linters/largefunc" "github.com/github/gh-aw/pkg/linters/lenstringzero" @@ -55,15 +56,16 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 29 analyzer +// "Public API > Subpackages" table. The README documents 30 analyzer // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // // Spec (README "Public API > Subpackages"): // // contextcancelnotdeferred, ctxbackground, errorfwrapv, excessivefuncparams, errormessage, -// errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, -// hardcodedfilepath, httpnoctx, jsonmarshalignoredeerror, largefunc, lenstringzero, +// errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, +// fprintlnsprintf, hardcodedfilepath, httpnoctx, httpresponsebodyclose, +// jsonmarshalignoredeerror, largefunc, lenstringzero, // manualmutexunlock, osexitinlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib, // regexpcompileinfunction, seenmapbool, sortslice, ssljson, strconvparseignorederror, // timeafterleak, timesleepnocontext, tolowerequalfold, uncheckedtypeassertion @@ -81,6 +83,7 @@ func documentedAnalyzers() []docAnalyzer { {"fprintlnsprintf", fprintlnsprintf.Analyzer}, {"hardcodedfilepath", hardcodedfilepath.Analyzer}, {"httpnoctx", httpnoctx.Analyzer}, + {"httpresponsebodyclose", httpresponsebodyclose.Analyzer}, {"jsonmarshalignoredeerror", jsonmarshalignoredeerror.Analyzer}, {"largefunc", largefunc.Analyzer}, {"lenstringzero", lenstringzero.Analyzer}, From 8b486276a09aaafd64da6f4638e39ff86bc8f8a8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 18 Jun 2026 21:47:59 +0000 Subject: [PATCH 4/4] Order linter README imports Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/linters/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/linters/README.md b/pkg/linters/README.md index 373ef59171f..448d8a68f79 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -93,10 +93,10 @@ import ( "github.com/github/gh-aw/pkg/linters/execcommandwithoutcontext" "github.com/github/gh-aw/pkg/linters/fileclosenotdeferred" "github.com/github/gh-aw/pkg/linters/hardcodedfilepath" + "github.com/github/gh-aw/pkg/linters/httpresponsebodyclose" "github.com/github/gh-aw/pkg/linters/largefunc" "github.com/github/gh-aw/pkg/linters/lenstringzero" "github.com/github/gh-aw/pkg/linters/manualmutexunlock" - "github.com/github/gh-aw/pkg/linters/httpresponsebodyclose" "github.com/github/gh-aw/pkg/linters/osexitinlibrary" panicinlibrarycode "github.com/github/gh-aw/pkg/linters/panic-in-library-code" "github.com/github/gh-aw/pkg/linters/rawloginlib"