[linter-miner] Add file-close-not-deferred linter#33834
Conversation
This linter detects file operations (os.Open, os.Create, os.OpenFile) where Close() is not immediately deferred, which can lead to file descriptor leaks if early returns or panics occur. Evidence from codebase scan: - pkg/cli/workflows.go:307-314 - os.Open with manual close - pkg/cli/workflows.go:396-402 - os.Open with manual close - pkg/cli/mcp_logs_guardrail.go:87-93 - os.OpenFile with manual close The linter uses AST inspection to: 1. Track file variable assignments from os.Open/Create/OpenFile 2. Check if Close() is deferred or called manually 3. Report cases where manual close is used without defer Co-authored-by: Copilot <[email protected]>
github-actions
Bot
added
automation
cookie
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds a new custom Go analysis linter to detect os.Open/os.Create/os.OpenFile patterns where files are closed manually instead of using an immediate defer, and wires the analyzer into the linter multichecker with analysistest-based fixtures.
Changes:
- Implement
fileclosenotdeferredanalyzer to track opened file variables and detect non-deferredClose()usage. - Add analysistest harness + testdata fixtures for positive/negative cases.
- Register the analyzer in
cmd/lintersmultichecker.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/file-close-not-deferred/file-close-not-deferred.go | New analyzer implementation that tracks open calls and reports manual Close() without defer. |
| pkg/linters/file-close-not-deferred/file-close-not-deferred_test.go | analysistest runner for the analyzer. |
| pkg/linters/file-close-not-deferred/testdata/src/fileclosenotdeferred/fileclosenotdeferred.go | Fixtures covering manual close, deferred close, and error-assigned close patterns. |
| cmd/linters/main.go | Registers the new analyzer in the multichecker binary. |
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: 6
| // Track file variables: varName -> (open position, has defer) | ||
| fileVars := make(map[string]*fileVarState) | ||
|
|
| // Walk all statements in the function body, including nested blocks | ||
| ast.Inspect(fn.Body, func(node ast.Node) bool { | ||
| if node == nil { | ||
| return false | ||
| } |
| openPos token.Pos | ||
| hasDefer bool | ||
| hasManuaClose bool |
| openPos: call.Pos(), | ||
| hasDefer: 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) { |
| "github.com/github/gh-aw/pkg/linters/errormessage" | ||
| "github.com/github/gh-aw/pkg/linters/errstringmatch" | ||
| "github.com/github/gh-aw/pkg/linters/excessivefuncparams" | ||
| fileclosenotdeferred "github.com/github/gh-aw/pkg/linters/file-close-not-deferred" |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose based on this linter implementation (a diagnostic tool with test-driven development approach).
Key Themes
1. Typo in Core Field Name (hasManuaClose → hasManualClose)
This typo appears 5 times in the code and will confuse future maintainers. A simple find-and-replace will fix it.
2. Scope Ambiguity
The linter only flags files with manual close (no defer). Files that are never closed at all are silently ignored. This may be intentional, but it's not documented. Consider either:
- Clarifying in the doc string that this only catches "manual close without defer"
- OR expanding the scope to catch "any file not properly deferred"
3. Missing Edge Case Tests
The test fixtures cover the happy path well, but don't include a "file never closed" case. Adding this would clarify the linter's intended scope and prevent future confusion.
4. Test Fixtures as Teaching Tools
The test code could better demonstrate why manual close is dangerous (early returns, panics, etc.). Adding explanatory comments would help developers understand the bug pattern, not just detect it.
Positive Highlights
✅ Clear motivation: The PR description explains the problem with concrete examples from the codebase
✅ Good test coverage: Tests cover manual close, deferred close, and error-checking patterns
✅ Follows project conventions: Uses analysistest framework, skips test files correctly
✅ Practical value: Targets a real bug pattern found in 3 places in the codebase
Verdict
The linter implementation is solid and follows good patterns. The issues are minor (typo + scope clarification) and don't block merge, but fixing the typo before merge would prevent technical debt.
Matt Pocock Skills Applied:
/tdd— Test-driven development principles/diagnose— Disciplined debugging and bug pattern detection
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 759.8K
Comments that could not be inline-anchored
pkg/linters/file-close-not-deferred/file-close-not-deferred.go:87
[/tdd] Typo in field name: hasManuaClose should be hasManualClose. This typo propagates throughout the code (lines 87, 109, 121, 133, 148) and will confuse future maintainers.
- hasManuaClose: false,
+ hasManualClose: false,Consider adding a test that exercises the field name explicitly in a table-driven test to catch typos like this.
pkg/linters/file-close-not-deferred/testdata/src/fileclosenotdeferred/fileclosenotdeferred.go:55
[/tdd] Missing edge case: What happens when a file is opened but never closed at all (neither deferred nor manual)? This is a real-world bug pattern that the linter should probably flag.
Consider adding a test fixture:
// not flagged: file never closed (different issue - out of scope?)
func ReadFileNeverClosed() error {
file, err := os.Open("test.txt")
if err != nil {
return err
}
// ... code but no close at all ...
return nil
}This clarifies the li…
pkg/linters/file-close-not-deferred/file-close-not-deferred.go:132
[/diagnose] The logic only reports if hasManuaClose && !hasDefer. This means files that are never closed at all are silently ignored. Is this intentional?
If the goal is strictly "flag manual close without defer", the current logic is correct but the documentation should clarify this narrow scope. If the goal is broader ("flag any file not properly closed"), you'd want:
if !state.hasDefer {
pass.Report(analysis.Diagnostic{
Pos: state.openPos,
Message: "file…
</details>
<details><summary>pkg/linters/file-close-not-deferred/testdata/src/fileclosenotdeferred/fileclosenotdeferred.go:207</summary>
**[/tdd]** The test fixture shows a realistic bug scenario, but it doesn't demonstrate *why* this is dangerous. Consider adding a comment that shows the leak:
```go
// flagged: file.Close() not deferred
func ReadFileManualClose() error {
file, err := os.Open("test.txt") // want `file Close\(\) should be deferred immediately after successful open to prevent resource leaks`
if err != nil {
return err
}
// BUG: early return here would leak the file descriptor
if someCo…
</details>
🧪 Test Quality Sentinel ReportTest Quality Score: 100/100✅ Excellent test quality
Test Classification Details
Analysis Notes✅
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (220 new lines in A draft ADR has been generated from the PR diff and committed to this branch to help you get started: 📄 Draft ADR: 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 this PR needs an ADRAdding a new in-house static-analysis linter is the kind of decision that benefits from a written record: there are real alternatives (third-party linter, broader "resource hygiene" check, plain code review) and real trade-offs (false-negative scope, registry compile cost, deferred cleanup of the 3 pre-existing violations). Capturing the decision now makes it greppable when the next contributor asks "why do we have so many tiny linter packages?" 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
References: §26245378772
|
|
@copilot review all comments and reviews |
Co-authored-by: pelikhan <[email protected]>
Co-authored-by: pelikhan <[email protected]>
Addressed all review comments in the latest commits:
|
|
@copilot review all comments and reviews |
…-close issues Co-authored-by: pelikhan <[email protected]>
One remaining issue was outstanding: when the same variable is reassigned via plain |
|
``
|
pelikhan
deleted the
linter-miner/file-close-not-deferred-f8e2ce8262f5fe49
branch
Summary
Adds a new static analysis linter that detects file operations where
Close()is called manually instead of being deferred, preventing potential resource leaks from early returns or panics.Changes
New Linter Implementation
file.Close()outside of defer statementstypes.Objectto correctly handle variable shadowingos.Fileoperations where missing defer is a common resource leak patternIntegration
cmd/linters/main.go: Registered
fileclosenotdeferredanalyzer in the multichecker analyzer listlinters: Updated compiled binary executable
Testing
pkg/linters/fileclosenotdeferred/fileclosenotdeferred_test.go: Added analysistest-based unit test suite
pkg/linters/fileclosenotdeferred/testdata/src/fileclosenotdeferred/fileclosenotdeferred.go: Created comprehensive test fixtures covering:
Close()calls that should be flaggeddefer Close()usage that should not be flaggedDocumentation
Impact
Technical Notes
The linter uses
types.Objectas the tracking key rather than variable names to correctly handle shadowing scenarios where the same variable name is reused in inner scopes. Violations are reported immediately when a file variable is reassigned after a manual close, ensuring the first open's missing defer is not hidden by subsequent operations.Commits
3ebe0e0ffReport violation immediately on reassignment to prevent hidden manual-close issuesac12035cbAddress all PR review comments for file-close-not-deferred linter888078785Address PR review feedback for file-close-not-deferred linterf775d6eddMerge branch 'main' into linter-miner/file-close-not-deferred-f8e2ce8262f5fe49ab58db206docs(adr): add draft ADR for file-close-not-deferred linterfb914db41Merge branch 'main' into linter-miner/file-close-not-deferred-f8e2ce8262f5fe496d168821aAdd file-close-not-deferred linter