[linter-miner] linter: add httpresponsebodyclose analyzer#40130
[linter-miner] linter: add httpresponsebodyclose analyzer#40130github-actions[bot] wants to merge 4 commits into
Conversation
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>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom Go go/analysis analyzer (httpresponsebodyclose) to flag HTTP response bodies that are closed manually (e.g. resp.Body.Close()) rather than deferred (defer resp.Body.Close()), and registers it in the cmd/linters multichecker.
Changes:
- Added
pkg/linters/httpresponsebodycloseanalyzer implementation that tracks*net/http.Responsevariables and reports whenBody.Close()is called without a corresponding defer. - Added
analysistest-based unit tests plus testdata fixtures for the new analyzer. - Registered
httpresponsebodyclose.Analyzerincmd/linters/main.go.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go | Implements the new analyzer and its AST/type-based detection logic. |
| pkg/linters/httpresponsebodyclose/httpresponsebodyclose_test.go | Runs the analyzer against analysistest fixtures. |
| pkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go | Provides positive/negative fixtures with // want diagnostics. |
| cmd/linters/main.go | Registers the new analyzer in the multichecker. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| 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()} | ||
| } |
| fileclosenotdeferred.Analyzer, | ||
| fmterrorfnoverbs.Analyzer, | ||
| hardcodedfilepath.Analyzer, | ||
| httpnoctx.Analyzer, | ||
| httpresponsebodyclose.Analyzer, | ||
| largefunc.Analyzer, |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (258 new lines across 📄 Draft ADR committed: 📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. For a growing in-repo linter suite, an ADR per analyzer records the detection approach and its known coverage limits — context future contributors (and your future self) will need when extending or debugging it. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( Scoring breakdown:
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /zoom-out — requesting changes on testdata coverage gaps before this lands in the global lint pass.
📋 Key Themes & Highlights
Correctness
The detection logic is sound. Type-checking for *net/http.Response, FuncLit boundary skipping, and the multi-pattern close detection (expression statement, assignment RHS, if-init) are all correctly implemented and closely mirror the established fileclosenotdeferred pattern.
Issues
- If-init fixture missing: The exact real-world shape identified in the PR (
if closeErr := resp.Body.Close(); closeErr != nil) has no// wantannotation. This is the stated primary motivation — it must be tested. - Closure boundary untested: The
*ast.FuncLitskip is a subtle correctness gate thatfileclosenotdeferredvalidates explicitly; this linter does not. - Reassignment path untested: The early-report-and-overwrite logic in
trackResponseAssignmenthas no fixture equivalent tofileclosenotdeferred'sReopenWithManualCloseThenDefer. - Body-aliasing false negative undocumented:
body := resp.Body; body.Close()silently escapes the linter — a known scope limitation that should be noted in a comment. - Design divergence undocumented: Tracking by LHS type (vs
fileclosenotdeferred's RHS call-site) is a deliberate and good choice, but deserves a brief comment for future maintainers.
Positive Highlights
- ✅ Accurate type-based detection using
*types.Pointer/*types.Named— no string-matching heuristics - ✅
FuncLitboundary guard prevents cross-closure false positives - ✅ Handles the three most common non-defer close shapes (expression, assignment, if-init)
- ✅ Properly mirrors the
fileclosenotdeferredstructure and style for easy maintainability - ✅ Tests pass (1.22s)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
| } | ||
| _, _ = io.ReadAll(resp.Body) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing test for the if-init close pattern — the exact real-world bug the PR identifies.
The PR body calls out pkg/cli/mcp_inspect_mcp_scripts_server.go:67 (if closeErr := resp.Body.Close(); closeErr != nil) as the real finding, yet no // want fixture exercises that path. The logic flows through IfStmt.Init → AssignStmt so it should work, but an untested code path is one refactor away from silently breaking.
💡 Suggested fixture to add
// flagged: Body.Close() in if-init, not deferred.
func fetchCloseInIfInit(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
}fileclosenotdeferred ships 7 fixtures; this linter covers 4. The if-init shape is the stated primary motivation — it deserves a test.
|
|
||
| // 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 { |
There was a problem hiding this comment.
[/tdd] Closure boundary isolation is a key correctness requirement with no testdata fixture.
Skipping *ast.FuncLit prevents false positives when a defer resp.Body.Close() inside a closure is mistakenly credited to the outer function's responseVars. 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)
// not flagged: the defer is inside a closure, independent execution context.
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()
}Also worth adding the inverse: manual close inside a closure that should be flagged within that closure.
| Message: "HTTP response Body.Close() should be deferred immediately after error check to prevent resource leaks", | ||
| }) | ||
| } | ||
| responseVars[obj] = &responseVarState{assignPos: assign.Pos()} |
There was a problem hiding this comment.
[/tdd] The response-variable reassignment path is untested.
When the same resp variable is reassigned via = (not :=) after a manual close, trackResponseAssignment reports the first violation immediately before overwriting the state — but there is no testdata fixture that exercises this sequence. fileclosenotdeferred covers the equivalent case with ReopenWithManualCloseThenDefer.
💡 Suggested fixture to add
// flagged: resp reassigned after manual close — first assignment 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() // manual close — violation
resp, err = http.DefaultClient.Get(url + "/retry")
if err != nil {
return err
}
defer resp.Body.Close() // deferred — ok for second request
_, _ = io.ReadAll(resp.Body)
return nil
}|
|
||
| // 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 { |
There was a problem hiding this comment.
[/tdd] Known false negative for body-aliasing is undocumented.
getHTTPBodyCloseObject only recognises the resp.Body.Close() selector chain directly on the response variable. If a caller aliases the body first — body := resp.Body; body.Close() — the linter produces no diagnostic, which is a silent resource leak. This is a reasonable scope limitation, but it should be called out in a comment so future maintainers understand the boundary and don't expect it to be covered.
💡 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 {| if obj == nil { | ||
| continue | ||
| } | ||
| if !isHTTPResponseType(obj.Type()) { |
There was a problem hiding this comment.
[/zoom-out] Tracking by LHS type vs by RHS call-site is a meaningful design difference from fileclosenotdeferred — worth a brief comment.
fileclosenotdeferred only tracks variables from explicit os.Open / os.Create / os.OpenFile calls. This linter tracks any assignment to a *net/http.Response-typed variable, including responses returned from helpers. That is intentionally broader and catches more real patterns, but it is a different philosophy that a future maintainer may not expect. A short comment on trackResponseAssignment explaining this choice (and that it is intentional) would preserve the decision context.
💡 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(...) {There was a problem hiding this comment.
REQUEST_CHANGES — three issues block merge
The linter logic is sound and the tests pass. Three things need to be addressed before this lands.
### Issues
1. pkg/linters/doc.go and pkg/linters/spec_test.go not updated (high)
pkg/linters/doc.go still says "All 29 active analyzers" — it must now say 30, and httpresponsebodyclose must be added to the list. More critically, spec_test.go's documentedAnalyzers() table does not include the new linter, so TestSpec_PublicAPI_SubpackageAnalyzers, TestSpec_DesignDecision_UniqueAnalyzerNames, and TestSpec_UsageExample_AnalyzersUsable never validate it. Every other linter in this repo is covered by those tests; this one silently isn't.
2. Diagnostic position at assignment LHS, not RHS call (medium — inline comment on line 137)
responseVarState.assignPos is set to assign.Pos(), which is the position of the first LHS token (resp), not the HTTP call on the RHS (client.Do(req)). The sibling fileclosenotdeferred linter stores call.Pos() for the open call, so editor squiggles land on the acquisition site. See inline comment.
3. No test case for the if-init close pattern (medium — inline comment on testdata line 53)
The PR description cites if closeErr := resp.Body.Close(); closeErr != nil { ... } as the actual finding in this repo, but that form is absent from the analysistest fixture. ast.Inspect does visit IfStmt.Init, so the behavior is likely correct — it's just unverified. See inline comment.
🔎 Code quality review by PR Code Quality Reviewer
| Message: "HTTP response Body.Close() should be deferred immediately after error check to prevent resource leaks", | ||
| }) | ||
| } | ||
| responseVars[obj] = &responseVarState{assignPos: assign.Pos()} |
There was a problem hiding this comment.
Diagnostic location points at the LHS variable, not the HTTP call — diverges from the fileclosenotdeferred pattern and gives editors a less precise squiggle.
💡 Suggested fix
fileclosenotdeferred stores call.Pos() (position of the os.Open(...) call on the RHS), so the warning highlights the acquisition site. Here assign.Pos() resolves to the first LHS identifier (resp), not client.Do(req).
For the common single-RHS case (resp, err := client.Do(req)), capture the RHS call position:
anchorPos := assign.Pos()
if len(assign.Rhs) == 1 {
if call, ok := assign.Rhs[0].(*ast.CallExpr); ok {
anchorPos = call.Pos()
}
}
responseVars[obj] = &responseVarState{assignPos: anchorPos}This matches the convention from fileclosenotdeferred and points the diagnostic at the HTTP call where defer resp.Body.Close() should be added.
| } | ||
| _, _ = io.ReadAll(resp.Body) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing test for the if-init close pattern — the primary real-world finding cited in the PR description is unverified.
💡 Suggested addition
The PR description specifically calls out pkg/cli/mcp_inspect_mcp_scripts_server.go:67 where the close appears as an if-init:
if closeErr := resp.Body.Close(); closeErr != nil { ... }Add a fixture function to prove the analyzer catches this form:
// flagged: Body.Close() in if-init assignment, 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
}ast.Inspect does descend into IfStmt.Init, so the logic handles this at runtime — but without a // want annotation the claim is unverified, and any future refactor of analyzeResponseNode could silently regress it.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Summary
Adds a new
httpresponsebodyclosecustom Go analysis linter that flags HTTP response bodies closed manually instead of viadefer.What it catches
When code calls
resp.Body.Close()directly—rather thandefer resp.Body.Close()—the body may not be closed if the function returns early due to an error, a panic, or areturnstatement added later. This is a resource leak.Flagged pattern:
Correct pattern:
Evidence from code scanning (Step 2)
The codebase was scanned for HTTP response body close patterns. All but one call site correctly uses
defer resp.Body.Close(). One real finding exists in the repository:Implementation
The linter mirrors the existing
fileclosenotdeferredlinter in structure and style:pkg/linters/httpresponsebodyclose/httpresponsebodyclose.go— analyzer usinggolang.org/x/tools/go/analysispkg/linters/httpresponsebodyclose/httpresponsebodyclose_test.go—analysistest-based unit testspkg/linters/httpresponsebodyclose/testdata/src/httpresponsebodyclose/httpresponsebodyclose.go— fixtures with// wantannotationscmd/linters/main.go— registershttpresponsebodyclose.Analyzerin the multicheckerDetection approach
*ast.FuncDeclnodes (skipping test files)*net/http.Responsedefer resp.Body.Close()is presentresp.Body.Close()is called without defer (expression statement orif-init assignment)Tests
Run #42 — workflow 27780121095
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
releaseassets.githubusercontent.comSee Network Configuration for more information.