fix(execcommandwithoutcontext): stop enclosing-scope walk at non-go/defer FuncLit boundaries#43692
Conversation
…uncLit boundaries Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. This PR fixes execcommandwithoutcontext linter by updating production code and test data/golden files only. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a correctness issue in the execcommandwithoutcontext analyzer’s enclosing-scope walk so it no longer attributes an outer function’s context.Context to exec.Command calls inside ordinary callback closures (non-go/defer FuncLit), preventing false positives and harmful autofixes.
Changes:
- Update
execcommandwithoutcontextenclosing-scope traversal to stop at non-go/deferFuncLitboundaries without acontext.Contextparameter. - Add
isGoOrDeferClosurehelper to detectgo/defer-launched closures, including parenthesized forms. - Extend testdata to cover suppressed callback cases and preserved
go/defercases (including parenthesized variants).
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go | Adjusts enclosing-scope walk to avoid crossing callback FuncLit boundaries unless the closure is launched via go/defer. |
| pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go | Adds new fixtures covering callback suppression and go/defer closure reporting expectations. |
| pkg/linters/execcommandwithoutcontext/testdata/src/execcommandwithoutcontext/execcommandwithoutcontext.go.golden | Updates golden output to reflect expected suggested-fix output for newly added Bad* fixtures. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
| if !hasCtx { | ||
| if _, isFuncLit := funcNode.(*ast.FuncLit); isFuncLit && !isGoOrDeferClosure(encl) { | ||
| break | ||
| } | ||
| continue |
… linter scope walk
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (147 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.
Review: execcommandwithoutcontext FuncLit boundary fix
This is a correct and well-tested port of the isGoOrDeferClosure boundary-discrimination logic from timesleepnocontext (#42901).
What was checked:
-
isGoOrDeferClosurelogic — The AST walk is correct for all four cases:go func(){...}()(non-paren) →GoStmtgrandparent → flagged ✓go (func(){...})()(paren) →ParenExprunwrap →GoStmtgrandparent → flagged ✓defer func(){...}()→DeferStmtgrandparent → flagged ✓defer (func(){...})()(paren) →DeferStmtgrandparent → flagged ✓- Regular callback (e.g.,
mux.HandleFunc("/run", func(){...})) →callee != funcLitCur.Node()→false→ suppressed ✓
-
breakvscontinue— Thebreakon a non-go/deferFuncLitwithout ctx correctly stops the enclosing walk, suppressing the false-positive diagnostic. The priorcontinuewas the root cause of the bug. -
Golden file — Correctly reflects autofix output for the three
Bad*cases;Good*cases are unchanged (no autofix applied). -
Test coverage — All new edge cases are covered:
GoodHTTPHandleFuncCallbackInCtxFunc,GoodSyncCallbackInCtxFunc,BadDeferWithCtx,BadDeferParenWithCtx,BadGoParenWithCtx. The pre-existingOuterCtxInnerClosurecovers the non-parengo funccase.
The implementation is a clean, faithful port with no deviations from the reference in timesleepnocontext. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 87 AIC · ⌖ 8.04 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting (not blocking). The fix is correct and well-motivated; two non-blocking improvement suggestions below.
📋 Key Themes & Highlights
Key Themes
- Duplicate helper:
isGoOrDeferClosureis now identical byte-for-byte in bothexeccommandwithoutcontextandtimesleepnocontext. The PR description explicitly calls it a "direct port", meaning the codebase now has two copies of the exact helper whose absence caused this bug — creating the same class of drift risk for future linters. - Missing test case: The new Bad test matrix is
BadDeferWithCtx,BadDeferParenWithCtx,BadGoParenWithCtxbut omits plainBadGoWithCtx(non-parenthesized goroutine).OuterCtxInnerClosure(pre-existing) covers it implicitly, but not as a newly-named regression case.
Positive Highlights
- ✅ Root cause correctly identified and documented — the analysis matches
timesleepnocontext's fix (#42901). - ✅ Clean boundary logic:
breakfor non-go/deferFuncLit,continueotherwise — easy to follow. - ✅ Parenthesized-form handling (
defer (func(){})()) is properly covered in both implementation and tests. - ✅ Good test naming:
GoodHTTPHandleFuncCallbackInCtxFuncandGoodSyncCallbackInCtxFuncread as specifications.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 108.4 AIC · ⌖ 7.24 AIC · ⊞ 6.7K
Comment /matt to run again
| return nil, nil | ||
| } | ||
|
|
||
| func isGoOrDeferClosure(funcLitCur inspector.Cursor) bool { |
There was a problem hiding this comment.
[/codebase-design] isGoOrDeferClosure is a verbatim copy of the same function in timesleepnocontext — the PR description even calls it a "direct port". Duplicating it recreates the exact divergence window that caused this bug: the next linter ported from this pattern will likely be missing the boundary fix again.
💡 Suggested refactor
Extract to pkg/linters/internal/astutil so all linters share one implementation:
// astutil/astutil.go
// IsGoOrDeferClosure reports whether the FuncLit at cur is the direct callee of a
// go or defer statement (handling parenthesized forms like defer (func(){})()).
func IsGoOrDeferClosure(funcLitCur inspector.Cursor) bool { ... }Then both execcommandwithoutcontext and timesleepnocontext call astutil.IsGoOrDeferClosure(encl). A single fix, once, propagates everywhere.
The bug history is a strong argument for doing this now rather than later — the third linter to carry this pattern will land without the fix, and we'll be here again.
@copilot please address this.
There was a problem hiding this comment.
Extracted isGoOrDeferClosure to pkg/linters/internal/astutil as the exported IsGoOrDeferClosure, and updated both execcommandwithoutcontext and timesleepnocontext to call astutil.IsGoOrDeferClosure. The private copies are removed. Commit: refactor(astutil): extract IsGoOrDeferClosure to shared astutil.
|
@copilot run pr-finisher skill |
…BadGoWithCtx test - Add IsGoOrDeferClosure to pkg/linters/internal/astutil to eliminate duplication - Remove local isGoOrDeferClosure from execcommandwithoutcontext and timesleepnocontext - Both linters now call astutil.IsGoOrDeferClosure - Add BadGoWithCtx test case (plain go func() closure) to complete boundary matrix - Update golden file to include BadGoWithCtx expected fixed output Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
execcommandwithoutcontextunconditionally walked past ctx-lessFuncLitnodes during its enclosing-scope search, falsely attributing an outer function'sctxtoexec.Commandcalls inside detached callbacks — producing wrong diagnostics and actively harmful autofixes.Root cause
The
if !hasCtx { continue }in the enclosing walk crossed everyFuncLitboundary regardless of whether the closure actually shares the outer context lifetime.timesleepnocontextalready fixed this same class of bug (#42901);execcommandwithoutcontextwas left behind.Changes
execcommandwithoutcontext.go— port the boundary discrimination fromtimesleepnocontext:FuncLit(regular callback) →break(stop walking, suppress diagnostic)go/deferFuncLit(inherits outer ctx lifetime) →continue(keep walking)isGoOrDeferClosurehelper (direct port; handles parenthesizeddefer (func(){})()forms)Testdata — add coverage for the previously untested cases:
GoodHTTPHandleFuncCallbackInCtxFunc—http.HandleFunccallback inside a ctx func → no diagnosticGoodSyncCallbackInCtxFunc— sync callback inside a ctx func → no diagnosticBadDeferWithCtx/BadDeferParenWithCtx/BadGoParenWithCtx— deferred/goroutine closures still flaggedFalse-positive example (now suppressed)