From 003a73ef1252cc77147c8272c81435a058d9063c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 9 Jun 2026 18:16:44 +0000 Subject: [PATCH 1/3] feat(linters): add execcommandwithoutcontext linter Add a new custom Go analysis linter that flags exec.Command calls inside functions that already receive a context.Context parameter, suggesting exec.CommandContext should be used instead to propagate context cancellation. Motivated by issue #38143 (and the underlying context-propagation debt tracked in discussion #38123): 122 bare exec.Command calls vs 27 exec.CommandContext calls in production code. The linter immediately identifies 3 real violations in pkg/workflow/github_cli.go and pkg/cli/mcp_inspect_mcp.go. Changes: - pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go - pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext_test.go - pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go - cmd/linters/main.go: register new analyzer Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cmd/linters/main.go | 2 + .../execcommandwithoutcontext.go | 123 ++++++++++++++++++ .../execcommandwithoutcontext_test.go | 16 +++ .../execcommandwithoutcontext.go | 36 +++++ 4 files changed, 177 insertions(+) create mode 100644 pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go create mode 100644 pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext_test.go create mode 100644 pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go diff --git a/cmd/linters/main.go b/cmd/linters/main.go index 2dd41e9e5ba..2fac9a2fb3f 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -21,6 +21,7 @@ import ( "github.com/github/gh-aw/pkg/linters/errormessage" "github.com/github/gh-aw/pkg/linters/errstringmatch" "github.com/github/gh-aw/pkg/linters/excessivefuncparams" + "github.com/github/gh-aw/pkg/linters/execcommandwithoutcontext" "github.com/github/gh-aw/pkg/linters/fileclosenotdeferred" "github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs" "github.com/github/gh-aw/pkg/linters/fprintlnsprintf" @@ -48,6 +49,7 @@ func main() { errormessage.Analyzer, fprintlnsprintf.Analyzer, errstringmatch.Analyzer, + execcommandwithoutcontext.Analyzer, excessivefuncparams.Analyzer, fileclosenotdeferred.Analyzer, fmterrorfnoverbs.Analyzer, diff --git a/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go new file mode 100644 index 00000000000..1e91ec99063 --- /dev/null +++ b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go @@ -0,0 +1,123 @@ +// 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 || !isExecCommandCall(pass, call) { + continue + } + + pos := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.IsTestFile(pos.Filename) { + continue + } + + for encl := range cur.Enclosing((*ast.FuncDecl)(nil)) { + fn, ok := encl.Node().(*ast.FuncDecl) + if !ok { + continue + } + ctxParamName, hasCtx := contextParamName(pass, fn) + if !hasCtx { + break + } + 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), + }) + break + } + } + + return nil, nil +} + +// isExecCommandCall reports whether call is a call to exec.Command from os/exec. +func isExecCommandCall(pass *analysis.Pass, call *ast.CallExpr) bool { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Command" { + return false + } + ident, ok := sel.X.(*ast.Ident) + if !ok { + return false + } + obj := pass.TypesInfo.ObjectOf(ident) + if obj == nil { + return false + } + pkgName, ok := obj.(*types.PkgName) + if !ok { + return false + } + return pkgName.Imported().Path() == "os/exec" +} + +// 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.FuncDecl) (string, bool) { + if fn.Type.Params == nil { + return "", false + } + ctxType := contextContextType(pass) + if ctxType == nil { + return "", false + } + for _, field := range fn.Type.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 +} + +// 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 +} diff --git a/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext_test.go b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext_test.go new file mode 100644 index 00000000000..61a3a6c0e3b --- /dev/null +++ b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext_test.go @@ -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.Run(t, testdata, execcommandwithoutcontext.Analyzer, "execcommandwithoutcontext") +} diff --git a/pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go b/pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go new file mode 100644 index 00000000000..e287222b6f9 --- /dev/null +++ b/pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go @@ -0,0 +1,36 @@ +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() +} From 13af0aa6334031074dfbe49fcf9510c020dfa36f Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 9 Jun 2026 18:32:44 +0000 Subject: [PATCH 2/3] docs(adr): add draft ADR-38185 for execcommandwithoutcontext linter Co-Authored-By: Claude Opus 4.8 (1M context) --- ...85-add-execcommandwithoutcontext-linter.md | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 docs/adr/38185-add-execcommandwithoutcontext-linter.md diff --git a/docs/adr/38185-add-execcommandwithoutcontext-linter.md b/docs/adr/38185-add-execcommandwithoutcontext-linter.md new file mode 100644 index 00000000000..05d3921cf92 --- /dev/null +++ b/docs/adr/38185-add-execcommandwithoutcontext-linter.md @@ -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.* From 0a1cf0b237efb65d035679493296589ee797fa03 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 9 Jun 2026 18:57:40 +0000 Subject: [PATCH 3/3] fix(linters): complete execcommandwithoutcontext review follow-ups Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/linters/README.md | 5 ++ pkg/linters/doc.go | 3 +- .../execcommandwithoutcontext.go | 68 +++++++++++++----- .../execcommandwithoutcontext_test.go | 2 +- .../execcommandwithoutcontext.go | 33 +++++++++ .../execcommandwithoutcontext.go.golden | 69 +++++++++++++++++++ pkg/linters/spec_test.go | 6 +- 7 files changed, 166 insertions(+), 20 deletions(-) create mode 100644 pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go.golden diff --git a/pkg/linters/README.md b/pkg/linters/README.md index c8581c9af88..c4e1c9950f5 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -12,6 +12,7 @@ This package currently provides custom Go analyzers in the following subpackages - `errormessage` — reports non-actionable error-message patterns in changed files. - `errstringmatch` — reports `strings.Contains(err.Error(), "...")` patterns and recommends `errors.Is` / `errors.As`. - `fileclosenotdeferred` — reports non-deferred file `Close()` calls that can leak resources. +- `execcommandwithoutcontext` — reports `exec.Command(...)` calls inside functions that already receive `context.Context` and should use `exec.CommandContext(...)`. - `fmterrorfnoverbs` — reports `fmt.Errorf` calls whose format string contains no verbs, recommending `errors.New` instead. - `fprintlnsprintf` — reports `fmt.Fprintln(..., fmt.Sprintf(...))` patterns and recommends direct formatting calls. - `jsonmarshalignoredeerror` — reports `json.Marshal` and `json.Unmarshal` calls where the error return is discarded with `_`. @@ -42,6 +43,7 @@ This package currently provides custom Go analyzers in the following subpackages | `excessivefuncparams` | Custom `go/analysis` analyzer that flags function declarations with too many positional parameters | | `errormessage` | Custom `go/analysis` analyzer that flags non-actionable error message patterns in changed files | | `errstringmatch` | Custom `go/analysis` analyzer that flags brittle `strings.Contains(err.Error(), "...")` checks | +| `execcommandwithoutcontext` | Custom `go/analysis` analyzer that flags `exec.Command(...)` calls that should use `exec.CommandContext(...)` in context-receiving functions | | `fileclosenotdeferred` | Custom `go/analysis` analyzer that flags file `Close()` calls that are not deferred immediately | | `fmterrorfnoverbs` | Custom `go/analysis` analyzer that flags `fmt.Errorf` calls with no format verbs, recommending `errors.New` | | `fprintlnsprintf` | Custom `go/analysis` analyzer that flags `fmt.Fprintln(..., fmt.Sprintf(...))` patterns | @@ -76,6 +78,7 @@ import ( "github.com/github/gh-aw/pkg/linters/excessivefuncparams" "github.com/github/gh-aw/pkg/linters/errormessage" "github.com/github/gh-aw/pkg/linters/errstringmatch" + "github.com/github/gh-aw/pkg/linters/execcommandwithoutcontext" "github.com/github/gh-aw/pkg/linters/fileclosenotdeferred" "github.com/github/gh-aw/pkg/linters/largefunc" "github.com/github/gh-aw/pkg/linters/lenstringzero" @@ -93,6 +96,7 @@ _ = ctxbackground.Analyzer _ = excessivefuncparams.Analyzer _ = errormessage.Analyzer _ = errstringmatch.Analyzer +_ = execcommandwithoutcontext.Analyzer _ = fileclosenotdeferred.Analyzer _ = largefunc.Analyzer _ = lenstringzero.Analyzer @@ -112,6 +116,7 @@ _ = ssljson.Analyzer - `github.com/github/gh-aw/pkg/linters/ctxbackground` — context-background analyzer subpackage - `github.com/github/gh-aw/pkg/linters/errormessage` — error-message analyzer subpackage (also re-exported as `ErrorMessageAnalyzer`) - `github.com/github/gh-aw/pkg/linters/errstringmatch` — err-string-match analyzer subpackage +- `github.com/github/gh-aw/pkg/linters/execcommandwithoutcontext` — exec-command-without-context analyzer subpackage - `github.com/github/gh-aw/pkg/linters/excessivefuncparams` — excessive-func-params analyzer subpackage - `github.com/github/gh-aw/pkg/linters/fileclosenotdeferred` — file-close-not-deferred analyzer subpackage - `github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs` — fmt-errorf-no-verbs analyzer subpackage diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 6ab0af7c844..986ed0639cf 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,12 +1,13 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// The actual analyzers are implemented in subpackages. All 23 active analyzers: +// The actual analyzers are implemented in subpackages. All 24 active analyzers: // // - contextcancelnotdeferred — flags context cancel functions called directly instead of deferred // - ctxbackground — flags context.Background() inside functions that already receive a context // - errormessage — flags non-actionable error message patterns in changed files // - errstringmatch — flags brittle strings.Contains(err.Error(), "...") checks // - excessivefuncparams — flags function declarations with too many positional parameters +// - execcommandwithoutcontext — flags exec.Command calls inside functions that already receive context.Context // - fileclosenotdeferred — flags file Close() calls that are not deferred // - fmterrorfnoverbs — flags fmt.Errorf calls with no format verbs, recommending errors.New // - fprintlnsprintf — flags fmt.Fprintln(..., fmt.Sprintf(...)) patterns diff --git a/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go index 1e91ec99063..d9b6ae2e472 100644 --- a/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go +++ b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go @@ -33,7 +33,11 @@ func run(pass *analysis.Pass) (any, error) { for cur := range insp.Root().Preorder((*ast.CallExpr)(nil)) { call, ok := cur.Node().(*ast.CallExpr) - if !ok || !isExecCommandCall(pass, call) { + if !ok { + continue + } + sel, ok := execCommandSelector(pass, call) + if !ok { continue } @@ -42,19 +46,36 @@ func run(pass *analysis.Pass) (any, error) { continue } - for encl := range cur.Enclosing((*ast.FuncDecl)(nil)) { - fn, ok := encl.Node().(*ast.FuncDecl) - if !ok { + for encl := range cur.Enclosing((*ast.FuncDecl)(nil), (*ast.FuncLit)(nil)) { + funcType := enclosingFuncType(encl.Node()) + if funcType == nil { continue } - ctxParamName, hasCtx := contextParamName(pass, fn) + ctxParamName, hasCtx := contextParamName(pass, funcType) if !hasCtx { - break + continue } 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{ + { + Pos: sel.Sel.Pos(), + End: sel.Sel.End(), + NewText: []byte("CommandContext"), + }, + { + Pos: call.Lparen + 1, + End: call.Lparen + 1, + NewText: []byte(ctxParamName + ", "), + }, + }, + }, + }, }) break } @@ -63,38 +84,42 @@ func run(pass *analysis.Pass) (any, error) { return nil, nil } -// isExecCommandCall reports whether call is a call to exec.Command from os/exec. -func isExecCommandCall(pass *analysis.Pass, call *ast.CallExpr) bool { +// 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 false + return nil, false } ident, ok := sel.X.(*ast.Ident) if !ok { - return false + return nil, false } obj := pass.TypesInfo.ObjectOf(ident) if obj == nil { - return false + return nil, false } pkgName, ok := obj.(*types.PkgName) if !ok { - return false + return nil, false + } + if pkgName.Imported().Path() != "os/exec" { + return nil, false } - return pkgName.Imported().Path() == "os/exec" + 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.FuncDecl) (string, bool) { - if fn.Type.Params == nil { +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.Type.Params.List { + for _, field := range fn.Params.List { t := pass.TypesInfo.TypeOf(field.Type) if t == nil || !types.Identical(t, ctxType) { continue @@ -108,6 +133,17 @@ func contextParamName(pass *analysis.Pass, fn *ast.FuncDecl) (string, bool) { 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 { diff --git a/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext_test.go b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext_test.go index 61a3a6c0e3b..db4b7e53b95 100644 --- a/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext_test.go +++ b/pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext_test.go @@ -12,5 +12,5 @@ import ( func TestAnalyzer(t *testing.T) { testdata := analysistest.TestData() - analysistest.Run(t, testdata, execcommandwithoutcontext.Analyzer, "execcommandwithoutcontext") + analysistest.RunWithSuggestedFixes(t, testdata, execcommandwithoutcontext.Analyzer, "execcommandwithoutcontext") } diff --git a/pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go b/pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go index e287222b6f9..e96bda2244e 100644 --- a/pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go +++ b/pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go @@ -34,3 +34,36 @@ func GoodBlankContext(_ context.Context, name string) error { cmd := exec.Command(name) return cmd.Run() } + +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` + }() +} diff --git a/pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go.golden b/pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go.golden new file mode 100644 index 00000000000..99b1f03cd5d --- /dev/null +++ b/pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go.golden @@ -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.CommandContext(ctx, 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.CommandContext(ctx, 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() +} + +type Runner struct{} + +// Bad: method with context parameter should use CommandContext. +func (r *Runner) Run(ctx context.Context, name string) error { + cmd := exec.CommandContext(ctx, 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.CommandContext(ctx, 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.CommandContext(ctx, "ls") // want `use exec\.CommandContext\(ctx, \.\.\.\) instead of exec\.Command to propagate context cancellation` + }() +} diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 175f59106d5..fac2268888c 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -16,6 +16,7 @@ import ( "github.com/github/gh-aw/pkg/linters/errormessage" "github.com/github/gh-aw/pkg/linters/errstringmatch" "github.com/github/gh-aw/pkg/linters/excessivefuncparams" + "github.com/github/gh-aw/pkg/linters/execcommandwithoutcontext" "github.com/github/gh-aw/pkg/linters/fileclosenotdeferred" "github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs" "github.com/github/gh-aw/pkg/linters/fprintlnsprintf" @@ -49,14 +50,14 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 23 analyzer +// "Public API > Subpackages" table. The README documents 24 analyzer // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // // Spec (README "Public API > Subpackages"): // // contextcancelnotdeferred, ctxbackground, excessivefuncparams, errormessage, -// errstringmatch, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, +// errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, // jsonmarshalignoredeerror, largefunc, lenstringzero, manualmutexunlock, // osexitinlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib, // regexpcompileinfunction, seenmapbool, sortslice, ssljson, @@ -68,6 +69,7 @@ func documentedAnalyzers() []docAnalyzer { {"excessivefuncparams", excessivefuncparams.Analyzer}, {"errormessage", errormessage.Analyzer}, {"errstringmatch", errstringmatch.Analyzer}, + {"execcommandwithoutcontext", execcommandwithoutcontext.Analyzer}, {"fileclosenotdeferred", fileclosenotdeferred.Analyzer}, {"fmterrorfnoverbs", fmterrorfnoverbs.Analyzer}, {"fprintlnsprintf", fprintlnsprintf.Analyzer},