[linter-miner] feat(linters): add ctxbackground linter — flag context.Background() when a ctx param exists#32865
Conversation
Reports calls to context.Background() inside functions that already receive a context.Context parameter. Such calls discard the caller's cancellation and deadline signals, which can cause goroutine leaks and unresponsive behaviour. Evidence: found in ~10+ files under pkg/ including pkg/cli/resources.go, pkg/cli/includes.go, pkg/workflow/safe_outputs_actions.go, and others. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new ctxbackground Go analyzer that flags context.Background() calls inside functions with a context.Context parameter and registers it with the custom multichecker.
Changes:
- Implements the new analyzer and helper logic for detecting context parameters.
- Adds analysistest coverage and fixtures.
- Registers the analyzer in
cmd/linters.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/ctxbackground/ctxbackground.go |
Defines the new analyzer implementation. |
pkg/linters/ctxbackground/ctxbackground_test.go |
Adds the analysistest entry point. |
pkg/linters/ctxbackground/testdata/src/ctxbackground/ctxbackground.go |
Adds fixture cases for expected diagnostics. |
cmd/linters/main.go |
Registers ctxbackground with the multichecker. |
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: 4
| nodeFilter := []ast.Node{ | ||
| (*ast.FuncDecl)(nil), | ||
| } |
| if ident.Name == "context" && sel.Sel.Name == "Background" { | ||
| pass.Reportf(call.Pos(), "use the context.Context parameter instead of context.Background()") |
| // Analyzer is the ctx-background analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ |
| // flagged: function receives ctx context.Context but calls context.Background() | ||
| func DoWork(ctx context.Context) { | ||
| _ = context.Background() // want `use the context.Context parameter instead of context.Background\(\)` |
🧪 Test Quality Sentinel ReportTest Quality Score: 100/100✅ Excellent test quality
Test Classification Details
Analysis Notes
The testdata covers four distinct scenarios:
This is strong behavioral coverage for a new linter: the testdata encodes the linter's exact contract (what it flags and what it intentionally ignores). 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.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — this is a new feature (linter) and the main risk is false positives and missing test coverage for boundary cases.
Key Themes
-
False-positive bug (
ast.Inspect+FuncLit): Theast.Inspectwalk on the function body descends into nested anonymous functions. Acontext.Background()call inside a closure that capturesctxfrom the outer scope will be incorrectly flagged, even when the detachment is intentional (e.g. background goroutines). The fix is to returnfalsewhenast.Inspectencounters a*ast.FuncLitnode. -
Missing test fixture for closures: The testdata has no case exercising the closure/anonymous-function scenario. A test fixture would both document the intended behaviour and catch regressions after the
FuncLitguard is added. -
FuncLitscope not analysed: Named-function analysis is done viaPreorderbutFuncLitnodes are never independently checked. If an anonymous function has its ownctx context.Contextparameter, it won't be analysed. Worth deciding intentionally (and documenting) rather than leaving it as an accidental gap. -
context.TODO()scope undocumented: The linter doesn't flagcontext.TODO(). That may be the right call, but adding a single not-flagged fixture makes the decision explicit and reviewable.
Positive Highlights
- ✅ Clean, idiomatic use of
golang.org/x/tools/go/analysis— inspector pre-order + type checking is the right approach - ✅ Correctly excludes blank-identifier params (
_ context.Context) — this is a subtle edge case that's easy to miss - ✅
analysistest-based tests are the gold standard for Go linters; good choice - ✅ Build tag on the test file follows the project convention
Verdict
Requesting changes primarily for the FuncLit false-positive bug (item 1) before merge. The test coverage gaps (items 2–4) should be addressed alongside the fix.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 5.2M
|
|
||
| // Walk the function body for context.Background() calls. | ||
| ast.Inspect(fn.Body, func(node ast.Node) bool { | ||
| call, ok := node.(*ast.CallExpr) |
There was a problem hiding this comment.
[/tdd] ast.Inspect recursively descends into nested *ast.FuncLit nodes. This means context.Background() inside a closure that captures ctx from the outer scope will be flagged — even when the closure deliberately detaches from the parent context (e.g. a background goroutine that must outlive the request). You should skip nested func literals:
ast.Inspect(fn.Body, func(node ast.Node) bool {
// Do not descend into nested function literals.
if _, ok := node.(*ast.FuncLit); ok {
return false
}
call, ok := node.(*ast.CallExpr)
...Without this guard, patterns like go func() { ctx := context.Background(); ... }() inside a function that has a ctx param produce false positives.
| // not flagged: init function | ||
| func init() { | ||
| _ = context.Background() | ||
| } |
There was a problem hiding this comment.
[/tdd] The testdata doesn't cover the anonymous-function/closure scenario, which is where the ast.Inspect descent issue above would surface. Consider adding:
// not flagged: context.Background() inside a func literal that has no ctx param
// (outer function has ctx, but the inner func literal is a distinct scope)
func DoWorkWithClosure(ctx context.Context) {
go func() {
_ = context.Background() // want? depends on the FuncLit guard decision
}()
}Having an explicit fixture for this case makes the intended behaviour clear and prevents regressions when the linter is later modified.
| // not flagged: init function | ||
| func init() { | ||
| _ = context.Background() | ||
| } |
There was a problem hiding this comment.
[/tdd] context.TODO() is a closely related anti-pattern ("I'll wire up a real context later") but isn't flagged by this linter. Either add a test fixture that explicitly marks it as not flagged (to document the intentional scope boundary), or extend the linter to also catch context.TODO(). Leaving it undocumented means the next person to maintain this linter won't know whether the omission was deliberate.
|
|
||
| nodeFilter := []ast.Node{ | ||
| (*ast.FuncDecl)(nil), | ||
| } |
There was a problem hiding this comment.
[/tdd] The nodeFilter only includes *ast.FuncDecl, so anonymous functions (*ast.FuncLit) that themselves receive a context.Context parameter are never checked. For example:
func Register(mux *http.ServeMux) {
mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
ctx := context.Background() // NOT flagged, but should be
doWork(ctx)
})
}If this is an intentional limitation (keeping the linter simple), add a comment to nodeFilter documenting that FuncLit is excluded on purpose. If the intent is to catch all cases, extend nodeFilter to also include (*ast.FuncLit)(nil) and apply hasContextParam to FuncLit.Type as well.
Summary
Adds a new custom Go analysis linter
ctxbackgroundthat reports calls tocontext.Background()inside functions that already receive acontext.Contextparameter.Calling
context.Background()discards the caller's cancellation signals and deadline, which can cause goroutine leaks and unresponsive behaviour when the parent context is cancelled.Evidence
The code-pattern scanner found this pattern in 10+ files under
pkg/, including:pkg/cli/resources.gopkg/cli/includes.gopkg/workflow/safe_outputs_actions.goWhat the linter catches
Files changed
pkg/linters/ctxbackground/ctxbackground.go— analyzer implementationpkg/linters/ctxbackground/ctxbackground_test.go— analysistest-based testspkg/linters/ctxbackground/testdata/src/ctxbackground/ctxbackground.go— test fixturescmd/linters/main.go— registered in multicheckerValidation
go build ./cmd/linters✅go test ./pkg/linters/ctxbackground/...✅go test ./pkg/linters/...✅ (all linter packages pass)