Add goroutinemissingrecover linter: flag panic-unsafe goroutines - #48966
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds a custom analyzer to detect anonymous goroutines lacking panic recovery.
Changes:
- Implements and registers
goroutinemissingrecover. - Adds analyzer fixtures and tests.
- Updates linter documentation and registry synchronization.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go |
Implements detection logic. |
pkg/linters/goroutinemissingrecover/goroutinemissingrecover_test.go |
Runs analyzer fixtures. |
pkg/linters/goroutinemissingrecover/testdata/src/a/a.go |
Defines test cases. |
pkg/linters/registry.go |
Registers the analyzer. |
pkg/linters/spec_test.go |
Adds documentation synchronization coverage. |
pkg/linters/doc.go |
Documents the analyzer. |
pkg/linters/README.md |
Adds public linter documentation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (4)
pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go:124
- Matching only the identifier text lets a shadowed function named
recoversatisfy the guard, e.g.recover := func() {}; recover(). That call is not the predeclared builtin and cannot contain a panic. Verifypass.TypesInfo.Uses[ident]is the builtinrecover(as the existing builtin checks inmapclearloopandpanic-in-library-codedo), and add a shadowing fixture.
ident, ok := call.Fun.(*ast.Ident)
if ok && ident.Name == "recover" {
found = true
pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go:117
ast.Inspectcounts calls inside immediately invoked nested literals, so this acceptsdefer func() { func() { recover() }() }()as a guard. That nestedrecoveris not called directly by a deferred function and returns nil during a panic. Only count a recover whose enclosing function is executing as a deferred call (and add this false-negative fixture), rather than accepting every descendant call.
found := false
ast.Inspect(body, func(n ast.Node) bool {
if found {
return false
}
pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go:123
- The effective builtin call can also be parenthesized as
(recover)(). In that valid guardcall.Funis an*ast.ParenExpr, so this assertion misses it and emits a false positive. Unwrapcall.Funwithastutil.UnwrapParenExprbefore resolving the identifier, and cover this spelling in the fixture.
ident, ok := call.Fun.(*ast.Ident)
if ok && ident.Name == "recover" {
pkg/linters/goroutinemissingrecover/goroutinemissingrecover.go:102
- A valid parenthesized recovery defer,
defer (func() { recover() })(), has an*ast.ParenExprindeferStmt.Call.Fun, so this check rejects it and reports an otherwise guarded goroutine. Use the existingastutil.UnwrapParenExprhelper before asserting*ast.FuncLit, and add the parenthesized form to the fixture.
fn, ok := deferStmt.Call.Fun.(*ast.FuncLit)
if !ok {
continue
}
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Medium
| fileclosenotdeferred.Analyzer, | ||
| fmterrorfnoverbs.Analyzer, | ||
| hardcodedfilepath.Analyzer, | ||
| goroutinemissingrecover.Analyzer, |
| // Only flag goroutines started with a function literal, not named functions. | ||
| call, ok := goStmt.Call.Fun.(*ast.FuncLit) | ||
| if !ok { | ||
| return | ||
| } |
![github-actions[bot]](https://cdn.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: goroutinemissingrecover linter
Good addition overall — structure, nolint support, and test coverage follow established patterns.
Two blocking issues found:
-
False negative in
containsRecoverCall—ast.Inspectdescends into nested*ast.FuncLitbodies, so arecover()call inside a nested closure is incorrectly counted as a top-level guard for the enclosing goroutine. Fix by skipping descent into*ast.FuncLitnodes. -
Out-of-order entry in
allAnalyzers(registry.go) —goroutinemissingrecoveris placed afterhardcodedfilepathinstead of betweenfprintlnsprintfandhardcodedfilepathalphabetically.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 33.9 AIC · ⌖ 4.97 AIC · ⊞ 5.3K
|
|
||
| // containsRecoverCall reports whether body contains a call to the built-in | ||
| // recover() function at any depth. | ||
| func containsRecoverCall(body *ast.BlockStmt) bool { |
There was a problem hiding this comment.
False negative: recover() inside a nested function literal is counted as a guard.
ast.Inspect descends into all nested nodes, including nested *ast.FuncLit bodies. This means the following passes the check even though the outer goroutine body has no direct recover guard:
go func() {
defer func() {
_ = func() interface{} { return recover() }()
}()
panic("oops")
}()Fix: stop descending into *ast.FuncLit nodes to avoid counting recover() calls inside nested closures:
ast.Inspect(body, func(n ast.Node) bool {
if found { return false }
// Do not descend into nested closures — their recover() only guards the inner func.
if _, ok := n.(*ast.FuncLit); ok {
return false
}
...
})Add a corresponding test fixture case to cover this.
@copilot please address this.
| fileclosenotdeferred.Analyzer, | ||
| fmterrorfnoverbs.Analyzer, | ||
| hardcodedfilepath.Analyzer, | ||
| goroutinemissingrecover.Analyzer, |
There was a problem hiding this comment.
Insertion order is not alphabetical.
The allAnalyzers slice adds goroutinemissingrecover after hardcodedfilepath (line 94) instead of before it, breaking the alphabetical order maintained by the rest of the slice. Move the entry between fprintlnsprintf and hardcodedfilepath.
@copilot please address this.
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (1 test)
Test Coverage Details: The single behavioral test (
Build tag compliance: ✅ (has Scoring Notes:
Score reduced from 100 because the single test function lacks explicit table-driven subtests (minor code organization; does not affect coverage). Verdict
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (202 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
|
There was a problem hiding this comment.
Verdict: Request changes
The core detection logic has a false-negative bug that undermines the linter's entire purpose.
💡 Themes
containsRecoverCalluses an unrestrictedast.Inspectthat descends into nested function literals inside the defer body. Per Go semantics,recover()only halts a panic when called directly by the deferred function itself -- arecover()buried in a nested closure (e.g.defer func(){ func(){ recover() }() }()) does not protect the goroutine, but this analyzer currently reports it as safe. This is a silent false negative in a linter whose sole job is catching exactly this class of bug.- The test fixture doesn't cover this nested-closure case, so the bug has no regression guard.
- Everything else (nolint handling, generated-file skipping, named-function goroutine exclusion, doc/registry wiring) looks solid and consistent with the rest of the linters package.
🔎 Code quality review by PR Code Quality Reviewer · aut00 · 52.3 AIC · ⌖ 7.28 AIC · ⊞ 7.8K
Comment /review to run again
| // recover() function at any depth. | ||
| func containsRecoverCall(body *ast.BlockStmt) bool { | ||
| found := false | ||
| ast.Inspect(body, func(n ast.Node) bool { |
There was a problem hiding this comment.
Unbounded ast.Inspect descends into nested function literals, causing false negatives for the exact bug class this linter exists to catch.
💡 Details
Go semantics: recover() only stops a panic when called directly by the deferred function itself. containsRecoverCall walks the entire subtree of the defer body without stopping at nested *ast.FuncLit boundaries, so a pattern like:
go func() {
defer func() {
func() { recover() }() // does NOT stop the panic -- wrong stack frame
}()
panic("oops")
}()is incorrectly accepted as safe (no diagnostic), even though the panic still crashes the process. A linter whose whole purpose is flagging panic-unsafe goroutines producing a silent false negative here is worse than no linter at all.
Fix: stop descending into nested *ast.FuncLit nodes, mirroring the top-level-only restriction already applied in hasTopLevelRecoverDefer, and add a testdata case for defer func(){ func(){ recover() }() }() to lock in the fix.
| go func() { | ||
| panic("oops") | ||
| }() | ||
| } |
There was a problem hiding this comment.
Missing test case for the highest-risk failure mode: a recover() reachable only via a nested closure inside the defer literal.
💡 Details
All four fixture cases test direct top-level patterns (direct recover, no recover, named-function goroutine, unrelated defer). None exercise containsRecoverCall's unrestricted recursive descent, which is exactly where the false-negative bug lives (see companion comment on goroutinemissingrecover.go). Add:
// nestedClosureRecover has recover() inside a nested closure -- still
// unsafe (recover only works one frame deep) -- should be flagged once fixed.
func nestedClosureRecover() {
go func() { // want ...
defer func() {
func() { recover() }()
}()
panic("oops")
}()
}Without this, the fix for the false negative has no regression guard.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on a correctness issue and a minor consistency gap.
📋 Key Themes & Highlights
Key Themes
- False-negative risk (
goroutinemissingrecover.goline 123):containsRecoverCallidentifies the built-inrecoverby name only. A user-defined local function also namedrecoverwould satisfy the check and suppress the diagnostic incorrectly. Themapclearlooplinter in this same codebase usespass.TypesInfo.Uses[ident].(*types.Builtin)— the same pattern should be applied here. - Out-of-order registration (
registry.goline 95): imports are alphabetical but theallAnalyzersslice entry is placed afterhardcodedfilepathinstead of before it, breaking the ordering convention. - Missing edge-case fixture (
testdata/src/a/a.go): no test covers a goroutine whose onlydefer/recoveris in a nested inner function literal — this is the scenariohasTopLevelRecoverDeferis explicitly designed to handle, and it should be exercised.
Positive Highlights
- ✅ Clean use of the
insp.Preorder/astutil/filecheck/nolinthelper chain — consistent with every other linter in the package. - ✅ The scope decision (function literals only, named calls excluded) is well-reasoned and documented.
- ✅
hasTopLevelRecoverDefercorrectly avoids descending into nested function bodies, preventing false positives from inner recovers. - ✅ Full integration: registry, spec_test, doc.go, and README all updated in one PR.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 65.5 AIC · ⌖ 5.26 AIC · ⊞ 7K
Comment /matt to run again
| return true | ||
| } | ||
| ident, ok := call.Fun.(*ast.Ident) | ||
| if ok && ident.Name == "recover" { |
There was a problem hiding this comment.
[/tdd] containsRecoverCall matches by name only — a user-defined function named recover would suppress the diagnostic, producing a false negative.
💡 Suggested fix: verify via type info
Use pass.TypesInfo.Uses to confirm the identifier resolves to the built-in recover, matching the pattern already used by mapclearloop for delete:
// Instead of:
if ok && ident.Name == "recover" {
// Prefer (requires passing pass.TypesInfo into containsRecoverCall):
if ok {
if obj := pass.TypesInfo.Uses[ident]; obj != nil {
if _, isBuiltin := obj.(*types.Builtin); isBuiltin && obj.Name() == "recover" {
found = true
return false
}
}
}Also add a test fixture with a local function named recover() to lock this in.
@copilot please address this.
| fileclosenotdeferred.Analyzer, | ||
| fmterrorfnoverbs.Analyzer, | ||
| hardcodedfilepath.Analyzer, | ||
| goroutinemissingrecover.Analyzer, |
There was a problem hiding this comment.
[/codebase-design] goroutinemissingrecover.Analyzer is inserted after hardcodedfilepath in the allAnalyzers slice, but the import block correctly places it alphabetically before hardcodedfilepath. The slice order is inconsistent with the import order and with the pattern used by other entries in this file.
💡 Move it before hardcodedfilepath
// before:
fmterrorfnoverbs.Analyzer,
hardcodedfilepath.Analyzer,
goroutinemissingrecover.Analyzer, // ← out of order
// after:
fmterrorfnoverbs.Analyzer,
goroutinemissingrecover.Analyzer, // ← alphabetically correct
hardcodedfilepath.Analyzer,Keeping the slice in alphabetical order makes it easy to verify completeness and spot duplicates at a glance.
@copilot please address this.
|
|
||
| // unsafeGoroutine has no recover — should be flagged. | ||
| func unsafeGoroutine() { | ||
| go func() { // want `goroutine launched via a function literal without a top-level defer/recover` |
There was a problem hiding this comment.
[/tdd] Missing test fixture for a goroutine that contains a nested function literal with its own defer/recover — the linter should still flag the outer goroutine because the nested recover only protects the inner call.
💡 Suggested fixture to add
// nestedRecoverOnlyGoroutine — outer goroutine has no recover; inner func does.
// Should be flagged because the outer goroutine is unprotected.
func nestedRecoverOnlyGoroutine() {
go func() { // want `goroutine launched via a function literal without a top-level defer/recover`
func() {
defer func() {
if r := recover(); r != nil {}
}()
panic("inner")
}()
panic("outer") // this panic is unrecovered
}()
}This also validates that hasTopLevelRecoverDefer correctly restricts itself to the top level of body.List and does not descend into nested function bodies.
@copilot please address this.
|
@copilot please run the skill and address the blocking ADR gate. Review the generated ADR draft, complete it as needed, and link it from the PR body so the gate can pass. Run: https://github.com/github/gh-aw/actions/runs/30488631737
|
…es, type safety, ordering, production goroutines Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all unresolved threads in commit
|
Adds a new
goroutinemissingrecovercustom go/analysis linter that flagsgo func() { ... }()goroutine literals without a top-leveldefer func() { recover() }()guard. An unrecovered panic in a goroutine bypasses all caller recovery and kills the process — this linter enforces the pattern already used inpkg/console/spinner.goandpkg/cli/docker_images.go.What it flags
Named-function goroutines (
go f()) are intentionally out of scope; the callee can install its own recovery.//nolint:goroutinemissingrecoversuppresses individual sites.Changes
pkg/linters/goroutinemissingrecover/— analyzer (insp.Preorderover*ast.GoStmt, checks for top-leveldefer+recoverin the literal body),analysistest-based test, and fixtures covering: safe goroutine, unsafe goroutine, named-function goroutine, defer-without-recover, and nolint-suppressedpkg/linters/registry.go— registers the analyzer inAll()pkg/linters/doc.go— count 59 → 60, new bulletpkg/linters/README.md— overview entry and subpackage table rowpkg/linters/spec_test.go—documentedAnalyzers()entry kept in syncRun: https://github.com/github/gh-aw/actions/runs/30488631737