[linter-miner] feat: add panic-in-library-code linter#34268
Conversation
Add new static analysis linter to detect panic() calls in library code under pkg/ that should return errors instead. Evidence: Found 6+ occurrences in the codebase: - pkg/workflow/domains.go (lines 324, 785) - pkg/workflow/strings.go (line 176) - pkg/workflow/pi_engine.go (line 139) - pkg/workflow/engine_definition_loader.go (line 137) - pkg/workflow/cache.go (line 48) This follows Go best practices where library code should return errors to allow callers to handle failures gracefully, rather than using panic which terminates the program. Similar to the existing osexitinlibrary linter that prevents os.Exit() in library code. The linter: - Detects builtin panic() calls in pkg/ packages - Excludes test files and cmd/ entry-points - Distinguishes builtin panic from user-defined functions - Reports: 'avoid panic in library code; return an error instead' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
|
|
❌ Test Quality Sentinel failed during test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom go/analysis analyzer to the cmd/linters multichecker that reports calls to the builtin panic() in non-cmd/ (library) packages, with analysistest-based fixtures.
Changes:
- Added
panicinlibrarycodeanalyzer that scans for builtinpanic()calls and reports a diagnostic. - Added analysistest unit test + testdata fixture for expected diagnostics.
- Registered the analyzer in
cmd/linters/main.go.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/panic-in-library-code/panic-in-library-code.go | Implements the analyzer and filtering logic (skip cmd/ and tests) and reports on builtin panic() calls. |
| pkg/linters/panic-in-library-code/panic-in-library-code_test.go | Runs analysistest for the new analyzer. |
| pkg/linters/panic-in-library-code/testdata/src/panicinlibrarycode/panicinlibrarycode.go | Provides fixtures demonstrating “bad” builtin panic() and “ok” non-builtin cases. |
| 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: 3
| // Verify it's the builtin panic, not a user-defined function | ||
| if obj := pass.TypesInfo.Uses[ident]; obj != nil { | ||
| if _, ok := obj.(*types.Builtin); !ok { | ||
| return // Not the builtin panic | ||
| } | ||
| } |
| // ok: user-defined panic function (not the builtin) | ||
| type myType struct{} | ||
|
|
||
| func (m myType) panic(msg string) { | ||
| // This is a custom method, not builtin panic |
| "github.com/github/gh-aw/pkg/linters/largefunc" | ||
| "github.com/github/gh-aw/pkg/linters/manualmutexunlock" | ||
| "github.com/github/gh-aw/pkg/linters/osexitinlibrary" | ||
| panicinlibrarycode "github.com/github/gh-aw/pkg/linters/panic-in-library-code" | ||
| "github.com/github/gh-aw/pkg/linters/rawloginlib" |
![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 /grill-with-docs — overall solid implementation that mirrors osexitinlibrary, with a few gaps worth addressing before merging.
📋 Key Themes & Highlights
Key Themes
- Doc/implementation scope mismatch:
Docsayspkg/but the linter fires on all non-cmd/packages — aligning these avoids surprises. Must*false positives: Common Go library pattern (MustCompile,MustParse) uses intentional panics and will generate noise. This is the highest-risk issue for adoption.- Thin test fixture: Edge cases (closures,
init(),panic(nil)) are undocumented — it's unclear whether they should be flagged or silently allowed. IsTestFilecalled per-node: Minor efficiency issue, but easy to fix by collecting test-file names once upfront.
Positive Highlights
- ✅ Correctly distinguishes builtin
panicfrom user-defined functions with the same name viatypes.Builtincheck - ✅ Build tag on test file,
analysistestharness, correct package naming - ✅ Clean structural match with
osexitinlibrary— easy to review and maintain - ✅
URLfield populated — good for tooling that surfaces linter docs
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 1.5M
|
|
||
| // Analyzer is the panic-in-library-code analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "panicinlibrarycode", |
There was a problem hiding this comment.
[/grill-with-docs] The Doc says under pkg/ but the implementation flags all non-cmd/ packages — any top-level package (e.g. internal/, tools/) would also be checked.
💡 Suggestion
Either tighten the guard to match the documented scope:
if !strings.Contains(pkgPath, "/pkg/") {
return nil, nil
}...or update the Doc string to reflect the actual behavior (e.g. "reports panic() calls in non-entrypoint packages"). The osexitinlibrary linter has the same pattern, so aligning wording with it would keep the codebase consistent.
| func run(pass *analysis.Pass) (any, error) { | ||
| pkgPath := pass.Pkg.Path() | ||
| // Skip packages under cmd/ entry-points — they are allowed to call panic. | ||
| if strings.HasSuffix(pkgPath, "/main") || strings.Contains(pkgPath, "/cmd/") { |
There was a problem hiding this comment.
[/tdd] No allowance for the idiomatic Go Must* pattern (MustCompile, MustParse, etc.). These functions intentionally panic on invalid input, are common in library packages, and are considered good style — the linter will generate false positives for them.
💡 Suggestion
Add a check to skip calls inside functions whose names start with Must:
// findEnclosingFuncName returns the name of the innermost named function
// containing pos, or "" for anonymous functions.
func findEnclosingFuncName(file *ast.File, pos token.Pos) string {
for _, decl := range file.Decls {
fd, ok := decl.(*ast.FuncDecl)
if !ok || fd.Body == nil {
continue
}
if fd.Body.Pos() <= pos && pos <= fd.Body.End() {
return fd.Name.Name
}
}
return ""
}Then inside the callback:
if strings.HasPrefix(findEnclosingFuncName(file, call.Pos()), "Must") {
return
}Alternatively, explicitly document the expected false-positive pattern and show an //nolint:panicinlibrarycode example in the testdata.
| } | ||
|
|
||
| insp.Preorder(nodeFilter, func(n ast.Node) { | ||
| call := n.(*ast.CallExpr) |
There was a problem hiding this comment.
[/tdd] IsTestFile and pass.Fset.Position(call.Pos()).Filename are called on every CallExpr node, not once per file. For packages with many call expressions, this is repeated work.
💡 Suggestion
Collect the set of non-test filenames once before entering the walk, then check membership:
testFiles := make(map[string]bool)
for _, f := range pass.Files {
name := pass.Fset.Position(f.Pos()).Filename
if filecheck.IsTestFile(name) {
testFiles[name] = true
}
}
insp.Preorder(nodeFilter, func(n ast.Node) {
call := n.(*ast.CallExpr)
filename := pass.Fset.Position(call.Pos()).Filename
if testFiles[filename] {
return
}
// ...
})This follows a pattern common in go/analysis passes and avoids the repeated string computation.
| import "errors" | ||
|
|
||
| // bad: panic in a pkg/ package. | ||
| func riskyFunction() { |
There was a problem hiding this comment.
[/tdd] The fixture is missing several edge-case scenarios that are important to validate for a panic linter.
💡 Missing test cases
// Should this be flagged? panic inside init() is common for "fail-fast" startup validation.
func init() {
panic("startup failure") // want `avoid panic...` (or explicitly allowed?)
}
// Should be flagged: panic inside an anonymous function in pkg/ code.
func withClosure() {
fn := func() {
panic("inside closure") // want `avoid panic...`
}
fn()
}
// Should be flagged: panic(nil) — less common but valid Go.
func nilPanic() {
panic(nil) // want `avoid panic...`
}Declaring intent (even if the answer is "yes, flag all of these") prevents future contributors from being surprised.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
Test Analysis
What design invariant does this enforce? The linter correctly identifies builtin What would break if deleted? The linter could regress by failing to detect panic calls, incorrectly flagging custom panic methods, or missing edge cases. 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. References:
|
Summary
This PR adds a new static analysis linter
panic-in-library-codethat detectspanic()calls in library code underpkg/that should return errors instead.Evidence
Found 6+ occurrences in the codebase during automated mining (Run #16):
pkg/workflow/domains.go(lines 324, 785)pkg/workflow/strings.go(line 176)pkg/workflow/pi_engine.go(line 139)pkg/workflow/engine_definition_loader.go(line 137)pkg/workflow/cache.go(line 48)Rationale
This follows Go best practices where library code should return errors to allow callers to handle failures gracefully, rather than using
panic()which terminates the program. This pattern is similar to the existingosexitinlibrarylinter that preventsos.Exit()in library code.From the Uber Go Style Guide:
Implementation Details
The linter:
panic()calls inpkg/packagescmd/entry-points (main packages can panic)panicfrom user-defined functions namedpanic"avoid panic in library code; return an error instead"Files created:
pkg/linters/panic-in-library-code/panic-in-library-code.go- Main analyzerpkg/linters/panic-in-library-code/panic-in-library-code_test.go- Testspkg/linters/panic-in-library-code/testdata/src/panicinlibrarycode/panicinlibrarycode.go- Test fixturescmd/linters/main.go- Registered new analyzerTesting
✅ Unit tests passing:
✅ Compilation successful:
go build ./cmd/linters # Build successful (9.5MB binary)✅ Real-world validation: Correctly detects panic calls in actual codebase files
Mining Process
This linter was discovered through automated daily mining:
pkg/andcmd/for error-prone patternsRelated Linters
osexitinlibrary- Preventsos.Exit()in library code (similar pattern)largefunc- Reference implementation for linter structurectxbackground- Example of context-aware lintingAutomated by: Linter Miner workflow (Run #16 - 26339347390)