[linter-miner] feat(linters): add execcommandwithoutcontext linter #38185
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| # ADR-38185: Enforce context propagation to subprocesses via a dedicated linter | ||
|
|
||
| **Date**: 2026-06-09 | ||
| **Status**: Draft | ||
|
|
||
| ## Context | ||
|
|
||
| The codebase spawns subprocesses with `os/exec`, but does so inconsistently: there are roughly 122 bare `exec.Command(...)` calls versus only 27 `exec.CommandContext(ctx, ...)` calls in production code, and that ratio had not improved since 2026-06-03 (per the deep-report tracked in discussion #38123). Bare `exec.Command` inside a function that already receives a `context.Context` produces a subprocess that cannot be cancelled when the context is cancelled (timeout, shutdown, request abort), leaking work and slowing graceful termination. Issue #38143 explicitly requested a sibling analyzer to the existing `ctxbackground` linter to catch this pattern. The repository already maintains a family of ~22 custom `golang.org/x/tools/go/analysis` analyzers wired into a multichecker in `cmd/linters/main.go`, so there is an established convention for adding type-accurate static checks. | ||
|
|
||
| ## Decision | ||
|
|
||
| We will add a new custom static-analysis linter, `execcommandwithoutcontext`, that flags `exec.Command(...)` calls occurring inside a function that already has a `context.Context` parameter and recommends `exec.CommandContext(ctx, ...)` instead. The analyzer follows the existing linter family conventions: it lives under `pkg/linters/execcommandwithoutcontext/`, uses cursor-based AST traversal with `pass.TypesInfo` for type-accurate matching (resolving the actual `os/exec` and `context` package identities rather than matching on names), reuses the shared `internal/filecheck` helper to skip test files, and is registered in the `cmd/linters/main.go` multichecker. We chose a compile-time linter (rather than runtime enforcement or documentation) so that new un-cancellable subprocess spawns are blocked at CI time before they land. | ||
|
|
||
| ## Alternatives Considered | ||
|
|
||
| ### Alternative 1: Extend the existing `ctxbackground` analyzer | ||
| Issue #38143 framed this as extending `ctxbackground`. We rejected folding the check into that analyzer because the two checks target distinct call shapes (`context.Background()` misuse vs. `exec.Command` cancellation propagation) with different reporting messages and fixtures. Keeping them as separate single-purpose analyzers matches the existing one-concern-per-linter convention and keeps each analyzer's tests focused. | ||
|
|
||
| ### Alternative 2: Rely on documentation, code review, or a `golangci-lint` community rule | ||
| We could document the convention and enforce it through review, or look for an off-the-shelf rule. This was rejected because review-based enforcement is exactly what allowed the 122-vs-27 imbalance to accumulate, and no existing community linter encodes the "context parameter is in scope" precondition. The repository's established pattern is custom `go/analysis` passes, which give precise, type-aware, CI-enforceable detection. | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
| - New `exec.Command` calls in context-receiving functions are caught automatically at CI time, preventing further accumulation of un-cancellable subprocess spawns. | ||
| - Immediately surfaces 3 live violations (`pkg/workflow/github_cli.go:32`, `pkg/cli/mcp_inspect_mcp.go:150`, `pkg/cli/mcp_inspect_mcp.go:155`) for follow-up. | ||
| - Consistent with the existing analyzer family, so it is low-friction for maintainers to understand and maintain. | ||
|
|
||
| ### Negative | ||
| - Adds another analyzer to the multichecker, marginally increasing lint runtime and the surface area of custom tooling to maintain. | ||
| - The check is intentionally narrow: it only fires when a `context.Context` parameter is directly in the enclosing `FuncDecl`. It will not catch `exec.Command` calls that could obtain a context transitively (e.g., from a struct field or a closure capture), so some un-cancellable spawns remain undetected. | ||
| - Existing violations are not auto-fixed; they must be remediated separately, and the linter may need temporary suppression or a backlog until they are addressed. | ||
|
|
||
| ### Neutral | ||
| - Blank-identifier context parameters (`_ context.Context`) are deliberately not flagged, since there is no in-scope context value to pass. | ||
| - The analyzer reports a diagnostic only; it does not provide an automated suggested fix (no `analysis.SuggestedFix`). | ||
|
|
||
| --- | ||
|
|
||
| *This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27227016743) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| // Package execcommandwithoutcontext implements a Go analysis linter that flags | ||
| // calls to exec.Command inside functions that already receive a context.Context | ||
| // parameter, where exec.CommandContext should be used instead to propagate | ||
| // cancellation. | ||
| package execcommandwithoutcontext | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "go/ast" | ||
| "go/types" | ||
|
|
||
| "golang.org/x/tools/go/analysis" | ||
| "golang.org/x/tools/go/analysis/passes/inspect" | ||
| "golang.org/x/tools/go/ast/inspector" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/internal/filecheck" | ||
| ) | ||
|
|
||
| // Analyzer is the exec-command-without-context analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "execcommandwithoutcontext", | ||
| Doc: "reports exec.Command calls inside context-receiving functions where exec.CommandContext should be used to propagate cancellation", | ||
| URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/execcommandwithoutcontext", | ||
| Requires: []*analysis.Analyzer{inspect.Analyzer}, | ||
| Run: run, | ||
| } | ||
|
|
||
| func run(pass *analysis.Pass) (any, error) { | ||
| insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) | ||
| if !ok { | ||
| return nil, fmt.Errorf("inspect analyzer result has unexpected type %T", pass.ResultOf[inspect.Analyzer]) | ||
| } | ||
|
|
||
| for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) { | ||
| call, ok := cur.Node().(*ast.CallExpr) | ||
| if !ok { | ||
| continue | ||
| } | ||
| sel, ok := execCommandSelector(pass, call) | ||
| if !ok { | ||
| continue | ||
| } | ||
|
|
||
| pos := pass.Fset.PositionFor(call.Pos(), false) | ||
| if filecheck.IsTestFile(pos.Filename) { | ||
| continue | ||
| } | ||
|
|
||
| for encl := range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) { | ||
| funcType := enclosingFuncType(encl.Node()) | ||
| if funcType == nil { | ||
| continue | ||
| } | ||
| ctxParamName, hasCtx := contextParamName(pass, funcType) | ||
| if !hasCtx { | ||
| continue | ||
| } | ||
| pass.Report(analysis.Diagnostic{ | ||
|
Contributor
Author
There was a problem hiding this comment. [/tdd] No 💡 Suggested implementationReturn the selector from pass.Report(analysis.Diagnostic{
Pos: call.Pos(),
End: call.End(),
Message: fmt.Sprintf("use exec.CommandContext(%s, ...) ...", ctxParamName),
SuggestedFixes: []analysis.SuggestedFix{
{
Message: "Replace exec.Command with exec.CommandContext",
TextEdits: []analysis.TextEdit{
// rename Command → CommandContext
{Pos: sel.Sel.Pos(), End: sel.Sel.End(), NewText: []byte("CommandContext")},
// insert ctx as first argument
{Pos: call.Lparen + 1, End: call.Lparen + 1, NewText: []byte(ctxParamName + ", ")},
},
},
},
})The test should then use |
||
| Pos: call.Pos(), | ||
| End: call.End(), | ||
| Message: fmt.Sprintf("use exec.CommandContext(%s, ...) instead of exec.Command to propagate context cancellation", ctxParamName), | ||
|
Contributor
Author
There was a problem hiding this comment. No 💡 Suggested implementationsel := call.Fun.(*ast.SelectorExpr) // safe: isExecCommandCall verified this
pass.Report(analysis.Diagnostic{
Pos: call.Pos(),
End: call.End(),
Message: fmt.Sprintf("use exec.CommandContext(%s, ...) instead of exec.Command to propagate context cancellation", ctxParamName),
SuggestedFixes: []analysis.SuggestedFix{{
Message: fmt.Sprintf("Replace exec.Command with exec.CommandContext(%s, ...)", ctxParamName),
TextEdits: []analysis.TextEdit{
// Rename selector: Command → CommandContext
{
Pos: sel.Sel.Pos(),
End: sel.Sel.End(),
NewText: []byte("CommandContext"),
},
// Insert ctx as first argument (position right after opening paren)
{
Pos: call.Lparen + 1,
End: call.Lparen + 1,
NewText: []byte(ctxParamName + ", "),
},
},
}},
})To make |
||
| SuggestedFixes: []analysis.SuggestedFix{ | ||
| { | ||
| Message: fmt.Sprintf("Replace exec.Command with exec.CommandContext(%s, ...)", ctxParamName), | ||
| TextEdits: []analysis.TextEdit{ | ||
| { | ||
| Pos: sel.Sel.Pos(), | ||
| End: sel.Sel.End(), | ||
| NewText: []byte("CommandContext"), | ||
| }, | ||
| { | ||
| Pos: call.Lparen + 1, | ||
| End: call.Lparen + 1, | ||
| NewText: []byte(ctxParamName + ", "), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return nil, nil | ||
| } | ||
|
|
||
| // execCommandSelector reports the selector expression for calls to | ||
| // exec.Command from os/exec. | ||
| func execCommandSelector(pass *analysis.Pass, call *ast.CallExpr) (*ast.SelectorExpr, bool) { | ||
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| if !ok || sel.Sel.Name != "Command" { | ||
| return nil, false | ||
| } | ||
| ident, ok := sel.X.(*ast.Ident) | ||
| if !ok { | ||
| return nil, false | ||
| } | ||
| obj := pass.TypesInfo.ObjectOf(ident) | ||
| if obj == nil { | ||
| return nil, false | ||
| } | ||
| pkgName, ok := obj.(*types.PkgName) | ||
| if !ok { | ||
| return nil, false | ||
| } | ||
| if pkgName.Imported().Path() != "os/exec" { | ||
| return nil, false | ||
| } | ||
| return sel, true | ||
| } | ||
|
|
||
| // contextParamName returns the name of the first context.Context parameter | ||
| // in fn, and true, or "", false if none exists. | ||
| func contextParamName(pass *analysis.Pass, fn *ast.FuncType) (string, bool) { | ||
| if fn == nil || fn.Params == nil { | ||
| return "", false | ||
| } | ||
| ctxType := contextContextType(pass) | ||
| if ctxType == nil { | ||
| return "", false | ||
| } | ||
| for _, field := range fn.Params.List { | ||
| t := pass.TypesInfo.TypeOf(field.Type) | ||
| if t == nil || !types.Identical(t, ctxType) { | ||
| continue | ||
| } | ||
| for _, name := range field.Names { | ||
| if name.Name != "_" { | ||
| return name.Name, true | ||
| } | ||
| } | ||
| } | ||
| return "", false | ||
| } | ||
|
|
||
| func enclosingFuncType(node ast.Node) *ast.FuncType { | ||
| switch fn := node.(type) { | ||
| case *ast.FuncDecl: | ||
| return fn.Type | ||
| case *ast.FuncLit: | ||
| return fn.Type | ||
| default: | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // contextContextType returns the types.Type for context.Context, or nil if | ||
| // the context package is not imported. | ||
| func contextContextType(pass *analysis.Pass) types.Type { | ||
| for _, pkg := range pass.Pkg.Imports() { | ||
| if pkg.Path() == "context" { | ||
| obj := pkg.Scope().Lookup("Context") | ||
| if obj != nil { | ||
| return obj.Type() | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| //go:build !integration | ||
|
|
||
| package execcommandwithoutcontext_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "golang.org/x/tools/go/analysis/analysistest" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/execcommandwithoutcontext" | ||
| ) | ||
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.RunWithSuggestedFixes(t, testdata, execcommandwithoutcontext.Analyzer, "execcommandwithoutcontext") | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| package execcommandwithoutcontext | ||
|
|
||
| import ( | ||
| "context" | ||
| "os/exec" | ||
| ) | ||
|
|
||
| // Bad: exec.Command inside a function that already receives a context. | ||
| func BadRunCommand(ctx context.Context, name string) error { | ||
| cmd := exec.Command(name) // want `use exec\.CommandContext\(ctx, \.\.\.\) instead of exec\.Command to propagate context cancellation` | ||
| return cmd.Run() | ||
| } | ||
|
|
||
| // Bad: exec.Command with extra args inside a context-receiving function. | ||
| func BadRunCommandArgs(ctx context.Context, name string, args ...string) { | ||
| cmd := exec.Command(name, args...) // want `use exec\.CommandContext\(ctx, \.\.\.\) instead of exec\.Command to propagate context cancellation` | ||
| _ = cmd | ||
| } | ||
|
|
||
| // Good: exec.CommandContext is used correctly. | ||
| func GoodRunCommandContext(ctx context.Context, name string) error { | ||
| cmd := exec.CommandContext(ctx, name) | ||
| return cmd.Run() | ||
| } | ||
|
|
||
| // Good: no context parameter, exec.Command is fine. | ||
| func GoodNoContext(name string) error { | ||
| cmd := exec.Command(name) | ||
| return cmd.Run() | ||
| } | ||
|
|
||
| // Good: context parameter is blank, exec.Command is acceptable. | ||
| func GoodBlankContext(_ context.Context, name string) error { | ||
| cmd := exec.Command(name) | ||
| return cmd.Run() | ||
|
Contributor
Author
There was a problem hiding this comment. Test data is missing all closure/anonymous-function scenarios, leaving the linter's behavior in those cases entirely undocumented and unverified. 💡 Cases to addfunc doWork(fn func(context.Context, string)) { fn(context.Background(), "ls") }
// Bad: exec.Command inside a function literal that itself receives context.
func BadFuncLitCtx() {
doWork(func(ctx context.Context, name string) {
_ = exec.Command(name) // want `use exec\.CommandContext\(ctx, \.\.\.\)`
})
}
// Good: function literal with context that correctly uses CommandContext.
func GoodFuncLitCtx() {
doWork(func(ctx context.Context, name string) {
_ = exec.CommandContext(ctx, "ls")
})
}
// Behaviour documentation: exec.Command inside an unlabelled closure nested
// in a context-receiving named function — the outer ctx name is reported.
func OuterCtxInnerClosure(ctx context.Context) {
go func() {
_ = exec.Command("ls") // want `use exec\.CommandContext\(ctx, \.\.\.\)`
}()
}The first case ( |
||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. [/tdd] Missing method-receiver test case — the sibling type Runner struct{}
func (r *Runner) Run(ctx context.Context, name string) error {
cmd := exec.Command(name) // want `use exec\.CommandContext\(ctx, \.\.\.\).*`
return cmd.Run()
}Since |
||
|
|
||
| type Runner struct{} | ||
|
|
||
| // Bad: method with context parameter should use CommandContext. | ||
| func (r *Runner) Run(ctx context.Context, name string) error { | ||
| cmd := exec.Command(name) // want `use exec\.CommandContext\(ctx, \.\.\.\) instead of exec\.Command to propagate context cancellation` | ||
| return cmd.Run() | ||
| } | ||
|
|
||
| func doWork(fn func(context.Context, string)) { | ||
| fn(context.Background(), "ls") | ||
| } | ||
|
|
||
| // Bad: function literal with context parameter should use CommandContext. | ||
| func BadFuncLitCtx() { | ||
| doWork(func(ctx context.Context, name string) { | ||
| _ = exec.Command(name) // want `use exec\.CommandContext\(ctx, \.\.\.\) instead of exec\.Command to propagate context cancellation` | ||
| }) | ||
| } | ||
|
|
||
| // Good: function literal with context parameter already uses CommandContext. | ||
| func GoodFuncLitCtx() { | ||
| doWork(func(ctx context.Context, name string) { | ||
| _ = exec.CommandContext(ctx, name) | ||
| }) | ||
| } | ||
|
|
||
| // Bad: closure in context-receiving function should use outer ctx. | ||
| func OuterCtxInnerClosure(ctx context.Context) { | ||
| go func() { | ||
| _ = exec.Command("ls") // want `use exec\.CommandContext\(ctx, \.\.\.\) instead of exec\.Command to propagate context cancellation` | ||
| }() | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code:
encl.Node().(*ast.FuncDecl)can never returnok == falsehere becausecur.Enclosing((*ast.FuncDecl)(nil))only yields nodes whose dynamic type matches the filter, making theif !ok { continue }guard unreachable and misleading about whether the assertion can fail.💡 Simplified
Note also: since Go does not permit named function declarations to nest inside other named functions, this loop iterates at most once. The double-
break+ surroundingforis a byproduct of defensive coding against a topological impossibility.