[linter-miner] feat(linters): add httprespbodyclose linter#42816
Conversation
Add a new custom Go analysis linter that flags HTTP response Body.Close() calls that are made directly (without defer), which risks resource leaks when the function returns early due to an error. The linter tracks *http.Response variables assigned from HTTP calls (using type information) and reports when resp.Body.Close() is called without defer. Files added: - pkg/linters/httprespbodyclose/httprespbodyclose.go - pkg/linters/httprespbodyclose/httprespbodyclose_test.go - pkg/linters/httprespbodyclose/testdata/src/httprespbodyclose/httprespbodyclose.go Also registers the new analyzer in cmd/linters/main.go. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom go/analysis linter (httprespbodyclose) to gh-aw’s in-repo linter suite, intended to prevent HTTP response-body leaks by flagging non-deferred resp.Body.Close() calls.
Changes:
- Introduces
pkg/linters/httprespbodycloseanalyzer and hooks it into thecmd/lintersmultichecker. - Adds analysistest-based unit tests + testdata fixtures for the new analyzer.
- Extends the linter runner (
cmd/linters/main.go) to include the new analyzer in the default linter set.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/httprespbodyclose/httprespbodyclose.go |
New analyzer implementation that tracks *http.Response variables and detects non-deferred Body.Close() calls. |
pkg/linters/httprespbodyclose/httprespbodyclose_test.go |
Adds analysistest harness for the new analyzer. |
pkg/linters/httprespbodyclose/testdata/src/httprespbodyclose/httprespbodyclose.go |
Test fixtures for expected diagnostics and non-diagnostic cases. |
cmd/linters/main.go |
Registers httprespbodyclose.Analyzer in the default multichecker set. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Low
| fmterrorfnoverbs.Analyzer, | ||
| hardcodedfilepath.Analyzer, | ||
| httpnoctx.Analyzer, | ||
| httprespbodyclose.Analyzer, | ||
| httpstatuscode.Analyzer, |
| "github.com/github/gh-aw/pkg/linters/hardcodedfilepath" | ||
| "github.com/github/gh-aw/pkg/linters/httpnoctx" | ||
| "github.com/github/gh-aw/pkg/linters/httprespbodyclose" | ||
| "github.com/github/gh-aw/pkg/linters/httpstatuscode" | ||
| "github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror" |
| // GoodNoClose returns the response to the caller, which is responsible for closing — not flagged. | ||
| func GoodNoClose(client *http.Client, req *http.Request) (*http.Response, error) { | ||
| return client.Do(req) | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Notes:
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (248 new lines in 📄 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 Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
Skills-Based Review 🧠Applied 📋 Key Themes & HighlightsKey Themes
Positive Highlights
@copilot please address the review comments above.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 76 AIC · ⌖ 12.8 AIC · ⊞ 6.6K
Comment /matt to run again
| // GoodNoClose returns the response to the caller, which is responsible for closing — not flagged. | ||
| func GoodNoClose(client *http.Client, req *http.Request) (*http.Response, error) { | ||
| return client.Do(req) | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing test case for closure/FuncLit boundary — the implementation explicitly stops traversal at *ast.FuncLit (line 78 of the analyzer) but there is no fixture verifying that a resp.Body.Close() inside a closure does not trigger a diagnostic on the outer response variable.
💡 Suggested test fixture
The analogous fileclosenotdeferred testdata has an OpenInClosure case for exactly this boundary. Add a similar case:
// GoodCloseInsideClosure — resp.Body.Close() inside a closure should not be
// attributed to the outer function's resp variable.
func GoodCloseInsideClosure(client *http.Client, req *http.Request) {
resp, err := client.Do(req)
if err != nil {
return
}
go func() { resp.Body.Close() }()
}Without this test, a regression in the FuncLit guard would go undetected.
@copilot please address this.
| // GoodNoClose returns the response to the caller, which is responsible for closing — not flagged. | ||
| func GoodNoClose(client *http.Client, req *http.Request) (*http.Response, error) { | ||
| return client.Do(req) | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing test case for variable shadowing — the analogous fileclosenotdeferred testdata has a ShadowedVar case, but there is none here. A shadowed resp variable (inner block re-declaring with :=) could interact with the types.Object keying in unexpected ways.
💡 Suggested test fixture
// GoodShadowedResp — inner resp shadows outer resp; both are deferred — not flagged.
func GoodShadowedResp(client *http.Client, req1, req2 *http.Request) error {
resp, err := client.Do(req1)
if err != nil {
return err
}
defer resp.Body.Close()
if true {
resp, err := client.Do(req2) // shadows outer resp
if err != nil {
return err
}
defer resp.Body.Close()
_ = resp
}
return nil
}@copilot please address this.
| // GoodNoClose returns the response to the caller, which is responsible for closing — not flagged. | ||
| func GoodNoClose(client *http.Client, req *http.Request) (*http.Response, error) { | ||
| return client.Do(req) | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing test case for response reassignment — the fileclosenotdeferred testdata has ReopenWithManualCloseThenDefer to cover the variable-reuse path in trackHTTPAssignment (lines 148–155 of the analyzer). No equivalent fixture is present here, leaving the "report prior unresolved violation before overwriting" logic untested.
💡 Suggested test fixture
// BadReopenManualCloseThenDefer — first response manually closed (violation),
// second response deferred (ok). First should be flagged.
func BadReopenManualCloseThenDefer(client *http.Client, req1, req2 *http.Request) error {
resp, err := client.Do(req1) // want `HTTP response Body\.Close\(\) should be deferred.*`
if err != nil {
return err
}
resp.Body.Close() // manual close — violation for first call
resp, err = client.Do(req2)
if err != nil {
return err
}
defer resp.Body.Close()
return nil
}@copilot please address this.
| // GoodNoClose returns the response to the caller, which is responsible for closing — not flagged. | ||
| func GoodNoClose(client *http.Client, req *http.Request) (*http.Response, error) { | ||
| return client.Do(req) | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing nolint suppression test cases — the testdata has no fixture for //nolint:httprespbodyclose, despite the analyzer supporting suppression. Both code paths that check nolint.HasDirective (line 64 and line 149) are left untested.
💡 Suggested test fixtures
// SuppressedManualClose — nolint directive suppresses the violation — not flagged.
func SuppressedManualClose(client *http.Client, req *http.Request) ([]byte, error) {
resp, err := client.Do(req) (nolint/redacted):httprespbodyclose
if err != nil {
return nil, err
}
data, _ := io.ReadAll(resp.Body)
resp.Body.Close()
return data, nil
}Without this, a bug in the nolint wiring would only be caught by the broader integration test suite (if it covers this linter at all).
@copilot please address this.
| if !nolint.HasDirective(pass.Fset.PositionFor(state.assignPos, false), noLintLinesByFile) { | ||
| pass.Report(analysis.Diagnostic{ | ||
| Pos: state.assignPos, | ||
| Message: "HTTP response Body.Close() should be deferred immediately after receiving the response to prevent resource leaks", |
There was a problem hiding this comment.
[/tdd] Diagnostic reported at the call-site position, not at the non-deferred Body.Close() line — this may be confusing to users. The message says "should be deferred immediately after receiving the response" but the caret in the IDE/CLI points to the HTTP call, not the manual Body.Close().
💡 Discussion
Pointing to the assignment line is reasonable (it's where the resource is acquired), but the existing fileclosenotdeferred analyzer follows the same convention, so this is consistent within the codebase. Worth considering whether a secondary note pointing to the actual Body.Close() line (using analysis.Diagnostic.Related) would improve developer experience, similar to how the Go compiler uses "note" annotations. Not a blocking issue but a UX improvement opportunity.
@copilot please address this.
| if obj := bodyCloseReceiver(pass, deferStmt.Call); obj != nil { | ||
| if state, found := respVars[obj]; found { | ||
| state.hasDeferClose = true | ||
| } |
There was a problem hiding this comment.
[/codebase-design] defer seen before the response is tracked — potential false negative. The inspectNode function processes ast.AssignStmt (line 82) and ast.DeferStmt (line 92) in AST traversal order within the same ast.Inspect walk. For the happy path (resp, err := ... followed by defer resp.Body.Close()) this works correctly because assignments always appear before the defer in source order. However, consider: what happens if code does defer resp.Body.Close() before resp is assigned in the same scope? The respVars[obj] lookup at line 94 will not find the key, silently ignoring the defer. This is an unlikely anti-pattern but worth a comment explaining the ordering assumption.
💡 Suggested improvement
Add a brief inline comment at line 92 clarifying the traversal order assumption:
// defer statements are processed in source order; if a defer appears before
// the assignment (unusual) the state lookup below will be a no-op, which is safe.
if deferStmt, ok := node.(*ast.DeferStmt); ok {@copilot please address this.
|
|
||
| if assign, ok := node.(*ast.AssignStmt); ok { | ||
| trackHTTPAssignment(pass, respVars, assign, noLintLinesByFile) | ||
| // Also check RHS for manual Body.Close() in assignments like: err := resp.Body.Close() |
There was a problem hiding this comment.
[/tdd] Missing test case for err := resp.Body.Close() — the comment on line 84 says the RHS-of-assignment path handles err := resp.Body.Close(), but the testdata has no fixture exercising it. This path is distinct from the exprStmt path (line 100) and a bug here would go undetected.
💡 Suggested test fixture
// BadManualCloseAssigned — resp.Body.Close() result captured in a variable — also flagged.
func BadManualCloseAssigned(client *http.Client, req *http.Request) ([]byte, error) {
resp, err := client.Do(req) // want `HTTP response Body\.Close\(\) should be deferred.*`
if err != nil {
return nil, err
}
data, _ := io.ReadAll(resp.Body)
err = resp.Body.Close()
return data, err
}@copilot please address this.
|
@copilot please run the
Run: https://github.com/github/gh-aw/actions/runs/28555444682
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in
I also attempted to update the PR body to add the ADR link, but this environment’s |
Summary
Adds a new custom Go analysis linter
httprespbodyclosethat flags HTTP responseBody.Close()calls made directly (withoutdefer), which risks resource leaks when a function returns early due to an error.Problem
A common Go mistake is closing an HTTP response body manually instead of deferring it:
The correct pattern is:
Evidence
Code pattern scan
The
pkg/andcmd/directories contain multiple files making HTTP calls (resp.Bodyreads inpkg/parser/remote_fetch.go,pkg/cli/import_url_fetcher.go, etc.). This linter enforces the safe deferred-close pattern consistently.This pattern is not covered by any existing linter:
fileclosenotdeferredonly handlesos.Open/os.Create/os.OpenFilehttpnoctxflags context-free HTTP calls but not body close patternsChanges
pkg/linters/httprespbodyclose/httprespbodyclose.go— analyzer using type info to detect*http.Responsevariables, flags non-deferred Body.Close()pkg/linters/httprespbodyclose/httprespbodyclose_test.go— unit tests with build tag!integrationpkg/linters/httprespbodyclose/testdata/src/httprespbodyclose/httprespbodyclose.go— test fixtures with// wantannotationscmd/linters/main.go— registers the new analyzer in multicheckerHow it works
The linter uses Go type information (
*net/http.Response) to identify HTTP response variables, then checks whetherresp.Body.Close()is called withdefer. It flags the anti-pattern of direct (non-deferred) body close. Suppression is available via//nolint:httprespbodyclosedirectives.