[linter-miner] feat(linters): add logfatallibrary linter to flag log.Fatal calls in library packages#45115
Conversation
Detects log.Fatal, log.Fatalf, and log.Fatalln calls in library (pkg/) packages. These functions call os.Exit(1) internally, which: - Bypasses deferred cleanup (e.g., open files, db connections) - Makes the calling package untestable in isolation - Prevents callers from handling errors gracefully The linter mirrors osexitinlibrary but targets the log package's fatal functions. cmd/ entry-points are excluded, as are test files and calls suppressed with //nolint:logfatallibrary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
Great addition! 🎉 The The PR is well-structured: the analyzer, unit tests with
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds a Go analyzer intended to prevent process termination from library code.
Changes:
- Detects standard-library
log.Fatal*calls. - Supports
nolintsuppression and tests core cases. - Registers the analyzer with the multichecker.
Show a summary per file
| File | Description |
|---|---|
cmd/linters/main.go |
Registers the analyzer. |
pkg/linters/logfatallibrary/logfatallibrary.go |
Implements detection and reporting. |
pkg/linters/logfatallibrary/logfatallibrary_test.go |
Runs analyzer tests. |
pkg/linters/logfatallibrary/testdata/src/logfatallibrary/logfatallibrary.go |
Provides test fixtures. |
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: Medium
| httprespbodyclose.Analyzer, | ||
| httpstatuscode.Analyzer, | ||
| largefunc.Analyzer, | ||
| logfatallibrary.Analyzer, |
| // Analyzer is the log-fatal-in-library analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "logfatallibrary", | ||
| Doc: "reports log.Fatal, log.Fatalf, and log.Fatalln calls inside library packages where they implicitly call os.Exit and bypass deferred cleanup", | ||
| URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/logfatallibrary", |
| if strings.HasSuffix(pkgPath, ".test") || filecheck.IsTestFile(pass.Fset.Position(call.Pos()).Filename) { | ||
| return |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is correct and consistent with the existing osexitinlibrary linter pattern. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 16.1 AIC · ⌖ 5.13 AIC · ⊞ 4.8K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
✅ VerdictPassed. 0% implementation tests (threshold: 30%). The test uses Go's standard
Excellent test quality: behavioral design contract well-supported by edge-case and suppression coverage. 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.
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 28.9 AIC · ⌖ 4.42 AIC · ⊞ 6.6K
Comment /matt to run again
| // ok: nolint suppression on same line. | ||
| func suppressedSameLine() { | ||
| log.Fatal("suppressed") //nolint:logfatallibrary | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing alias-import test case — osexitinlibrary has a dedicated osexitinlibrary_alias.go fixture that proves aliased imports (e.g. xlog "log") are still flagged and local shadowed vars are not. Without it, the alias-handling path of IsPkgSelector is untested for this linter.
💡 Suggested addition: `testdata/src/logfatallibrary/logfatallibrary_alias.go`
package logfatallibrary
import xlog "log"
// flagged: aliased log import still resolves to log package
func stopWithAliasedFatal() {
xlog.Fatal("aliased") // want `log.Fatal called in library package`
}
// not flagged: local type named "log"
type fakeLog struct{}
func (fakeLog) Fatal(args ...any) {}
func notFlaggedShadowedLog() {
var log fakeLog
log.Fatal("not the stdlib log")
}This mirrors pkg/linters/osexitinlibrary/testdata/src/osexitinlibrary/osexitinlibrary_alias.go exactly.
@copilot please address this.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 50.2 AIC · ⌖ 4.56 AIC · ⊞ 5.4K
Comment /review to run again
| if !ok { | ||
| return | ||
| } | ||
| if strings.HasSuffix(pkgPath, ".test") || filecheck.IsTestFile(pass.Fset.Position(call.Pos()).Filename) { |
There was a problem hiding this comment.
Redundant per-node evaluation of a constant check: strings.HasSuffix(pkgPath, ".test") is re-evaluated on every AST node visit, but pkgPath is constant for the entire analysis pass.
💡 Suggested fix
Hoist the package-level skip guards to the top of run(), alongside the existing cmd/ guard:
func run(pass *analysis.Pass) (any, error) {
pkgPath := pass.Pkg.Path()
if strings.HasSuffix(pkgPath, "/main") ||
strings.Contains(pkgPath, "/cmd/") ||
strings.HasSuffix(pkgPath, ".test") {
return nil, nil
}
// ...
insp.Preorder(nodeFilter, func(n ast.Node) {
call, ok := n.(*ast.CallExpr)
if !ok {
return
}
if filecheck.IsTestFile(pass.Fset.Position(call.Pos()).Filename) {
return
}
// ...
})
}filecheck.IsTestFile still needs to stay inside the closure because it is node-dependent (per filename), but the package-path suffix check is package-scoped and executing it per-node is wasteful. The same pattern would be a fix for the sibling osexitinlibrary linter too.
| if !astutil.IsPkgSelector(pass, sel, "log") { | ||
| return | ||
| } | ||
| position := pass.Fset.PositionFor(call.Pos(), false) |
There was a problem hiding this comment.
Inconsistent position resolution between test-file guard and nolint check: line 57 uses pass.Fset.Position() (adjusted=true) while line 70 uses pass.Fset.PositionFor(call.Pos(), false) (adjusted=false). In files with //line directives (e.g. generated code), these resolve to different physical lines — so the two guards could disagree about which line is the 'real' one.
💡 Suggested fix
Use PositionFor(..., false) consistently in both places so physical-line resolution is always used:
// line 57 — change Position to PositionFor
if strings.HasSuffix(pkgPath, ".test") || filecheck.IsTestFile(pass.Fset.PositionFor(call.Pos(), false).Filename) {
return
}The sibling osgetenvlibrary linter already uses PositionFor(..., false) in both its node-level guard and its nolint check, so this aligns the new linter with the established pattern.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (128 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
|
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new
logfatallibraryGo static analysis linter that flagslog.Fatal,log.Fatalf, andlog.Fatallncalls inside library packages (pkg/). These functions callos.Exit(1)internally, bypassing deferred cleanup and making packages untestable in isolation. This extends the existing process-termination linter family (osexitinlibrary,panicinlibrarycode).Changes
pkg/linters/logfatallibrary/logfatallibrary.golog.Fatal*in non-cmd/packagespkg/linters/logfatallibrary/logfatallibrary_test.goanalysistest-based unit testpkg/linters/logfatallibrary/testdata/src/logfatallibrary/logfatallibrary.gocmd/linters/main.gologfatallibrary.Analyzerin the linter binarydocs/adr/45115-logfatallibrary-linter-for-pkg-packages.mdBehaviour
log.Fatal,log.Fatalf,log.Fatallnin packages whose import path does not contain/cmd/and does not end with/main.cmd/or with suffix/main(entry-point executables)._test.go) and test packages (path suffix.test).//nolint:logfatallibrarydirective on the same or preceding line.Diagnostic
Alternatives considered
osexitinlibrary: rejected — keeps conceptually distinct constructs separate, limits blast radius.References
docs/adr/45115-logfatallibrary-linter-for-pkg-packages.mdosexitinlibrary,panicinlibrarycode