[linter-miner] feat(linters): add osexitinlibrary linter#32448
Conversation
Reports os.Exit calls inside library (pkg/) packages. Calling os.Exit in a library package bypasses all deferred cleanup (file closes, mutex unlocks, cleanup goroutines) and makes the code path impossible to test. Only cmd/ entry-points should terminate the process. Evidence from code scan: - pkg/cli/upgrade_command.go:178 — os.Exit(0) after successful upgrade - pkg/cli/upgrade_command.go:376 — os.Exit(exitErr.ExitCode()) The linter skips packages whose import path contains /cmd/ or ends in /main so that true entry-points are never flagged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
There was a problem hiding this comment.
Pull request overview
Adds a new custom Go analyzer osexitinlibrary that flags os.Exit calls in non-cmd/ packages, and registers it with the cmd/linters multichecker driver.
Changes:
- New analyzer that walks
ast.CallExprnodes and reportsos.Exit(...)calls, skipping packages whose import path contains/cmd/or ends in/main. - New
analysistest-based unit test with a positive and negative fixture. - Registered the new analyzer alongside
excessivefuncparamsandlargefuncin the multichecker entry-point.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/osexitinlibrary/osexitinlibrary.go |
Implements the analyzer with package-path-based skip logic. |
pkg/linters/osexitinlibrary/osexitinlibrary_test.go |
analysistest-driven test, gated by !integration build tag. |
pkg/linters/osexitinlibrary/testdata/src/osexitinlibrary/osexitinlibrary.go |
Fixture file with a flagged and a clean function. |
cmd/linters/main.go |
Registers the new analyzer in multichecker.Main. |
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: 0
🧪 Test Quality Sentinel ReportTest Quality Score: 100/100✅ Excellent test quality
Test Classification Details
Flagged Tests — Requires ReviewNo tests flagged — all tests passed quality review. Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
![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 · ● 4.9M
| "github.com/github/gh-aw/pkg/linters/osexitinlibrary" | ||
| ) | ||
|
|
||
| func TestOsExitInLibrary(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The test only exercises the "flagged" case. The skip logic (strings.Contains(pkgPath, "/cmd/")) is the other code path and it's untested. A second testdata package (e.g. testdata/src/cmd/main/main.go) containing an os.Exit call that should not be flagged would verify that the allow-list works correctly and prevent regressions if the skip condition is ever changed.
// testdata/src/cmd/main/main.go
package main
import "os"
func main() {
os.Exit(0) // should NOT be flagged
}Then add to the test:
analysistest.Run(t, testdata, osexitinlibrary.Analyzer, "osexitinlibrary", "cmd/main")| if !ok { | ||
| return | ||
| } | ||
| if ident.Name == "os" && sel.Sel.Name == "Exit" { |
There was a problem hiding this comment.
[/tdd] The check ident.Name == "os" matches by identifier name only — it doesn't verify that the os identifier actually refers to the standard library os package. If a file imports a third-party package with the alias os (unusual but valid Go), the linter would produce a false positive. The golang.org/x/tools/go/analysis framework provides type information via pass.TypesInfo that lets you confirm the import path:
if obj := pass.TypesInfo.ObjectOf(sel.Sel); obj != nil {
if fn, ok := obj.(*types.Func); ok {
if fn.Pkg().Path() == "os" && fn.Name() == "Exit" {
pass.Reportf(call.Pos(), ...)
}
}
}This is the recommended pattern in the golang.org/x/tools/go/analysis documentation and makes the linter robust to aliased imports.
|
Could you confirm the new linter is covered by tests and ready for another review pass?
|
|
@copilot lint go, review all comments and reviews |
|
Please address the unresolved TDD feedback, then summarize the remaining blockers or next steps.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 235ceb0. Ran Go lint/test checks:
I also fixed a linting issue uncovered during review: |
Summary
Adds a new
osexitinlibrarycustom linter that reportsos.Exitcalls inside library (pkg/) packages.What it catches
Calling
os.Exitin a library package:cmd/entry-points should terminate the processThe linter flags any
os.Exit(...)call in a package whose import path does not contain/cmd/and does not end in/main.Evidence
Found during automated code-pattern scan (run #4):
pkg/cli/upgrade_command.goos.Exit(0)after successful upgrade — skips all deferred cleanuppkg/cli/upgrade_command.goos.Exit(exitErr.ExitCode())propagating a child process exit codeImplementation
pkg/linters/osexitinlibrary/osexitinlibrary.go— AST walk overast.CallExprnodes; skipscmd/packagespkg/linters/osexitinlibrary/osexitinlibrary_test.go—!integration-tagged test usinganalysistestpkg/linters/osexitinlibrary/testdata/src/osexitinlibrary/osexitinlibrary.go— fixture with one flagged and one clean casecmd/linters/main.go— registeredosexitinlibrary.Analyzerinmultichecker.MainVerification
Opened by linter-miner workflow run #25933000380
pr-sous-chef: updated branch during run https://github.com/github/gh-aw/actions/runs/25942653140