[linter-miner] linter: add httpresponsebodyclose analyzer #40130
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 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 | ||
|
|
||
| ### 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 response-acquisition site, not at the later 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.* |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,202 @@ | ||
| // 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) { | ||
| // 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 == "_" { | ||
| continue | ||
| } | ||
| obj := pass.TypesInfo.ObjectOf(ident) | ||
| if obj == nil { | ||
| continue | ||
| } | ||
| if !isHTTPResponseType(obj.Type()) { | ||
|
Contributor
Author
There was a problem hiding this comment. [/zoom-out] Tracking by LHS type vs by RHS call-site is a meaningful design difference from
💡 Suggested comment// trackResponseAssignment registers any assignment whose LHS has type *net/http.Response.
// Unlike fileclosenotdeferred (which tracks only explicit os.Open/os.Create calls),
// we key on the variable's type rather than the call site so that responses returned
// by helper functions are also tracked.
func trackResponseAssignment(...) { |
||
| 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", | ||
| }) | ||
| } | ||
| 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} | ||
| } | ||
| } | ||
|
|
||
| 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. | ||
| // 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 { | ||
|
Contributor
Author
There was a problem hiding this comment. [/tdd] Known false negative for body-aliasing is undocumented.
💡 Suggested doc comment addition on `getHTTPBodyCloseObject`// 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" { | ||
| 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" | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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") | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] Closure boundary isolation is a key correctness requirement with no testdata fixture.
Skipping
*ast.FuncLitprevents false positives when adefer resp.Body.Close()inside a closure is mistakenly credited to the outer function'sresponseVars. Without a test, a future change to the walk could silently re-introduce those false positives.💡 Suggested fixture to add (mirrors `fileclosenotdeferred`'s `OpenInClosure` case)
Also worth adding the inverse: manual close inside a closure that should be flagged within that closure.