[linter-miner] feat(linters): add timesleepnocontext linter#38704
Conversation
Reports bare time.Sleep calls inside functions that already receive a
context.Context parameter. Such calls ignore cancellation signals, leaving
goroutines blocked for the full sleep duration even when the caller has
cancelled or timed out the operation.
The fix is to replace time.Sleep(d) with a context-aware select:
select {
case <-time.After(d):
case <-ctx.Done():
return ctx.Err()
}
Evidence gathered from 14 days of issues and code scanning:
- 13 bare time.Sleep calls found in non-test code inside context-receiving
functions (pkg/cli/logs_rate_limit.go, pkg/cli/run_workflow_tracking.go,
pkg/workflow/docker_validation.go, pkg/cli/mcp_inspect.go, and others)
- Immediately caught a real violation in pkg/cli/run_workflow_execution.go:574
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom Go analysis linter (timesleepnocontext) to the gh-aw linter suite to detect time.Sleep(...) calls inside functions that already accept a context.Context, encouraging context-cancellable waiting patterns.
Changes:
- Introduces
pkg/linters/timesleepnocontextanalyzer that detectstime.Sleepcalls and reports diagnostics when a context parameter exists in an enclosing function. - Adds
analysistest-based unit tests and fixtures covering bad/good cases (including closures closing over a context). - Registers the new analyzer in
cmd/lintersmultichecker.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/timesleepnocontext/timesleepnocontext.go | Implements the analyzer logic to detect time.Sleep under context-receiving functions. |
| pkg/linters/timesleepnocontext/timesleepnocontext_test.go | Adds analyzer test harness using analysistest. |
| pkg/linters/timesleepnocontext/testdata/src/timesleepnocontext/timesleepnocontext.go | Provides fixture cases with // want expectations for diagnostics. |
| cmd/linters/main.go | Registers timesleepnocontext.Analyzer in 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: 1
| sortslice.Analyzer, | ||
| strconvparseignorederror.Analyzer, | ||
| jsonmarshalignoredeerror.Analyzer, | ||
| lenstringzero.Analyzer, | ||
| timesleepnocontext.Analyzer, | ||
| tolowerequalfold.Analyzer, |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (212 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 MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you for capturing the decision, the alternatives, and the trade-offs while the context is still fresh. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
Test inflation: 16 test lines vs. 137 production lines — ratio 0.12 ✅ 📖 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. References: §27374710223
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — one false-positive bug and one undocumented behaviour gap.
### Blocking issues found
1. False positive — blank-context closure inside outer context-receiving function (line 55)
The continue in the enclosing-function loop escalates past a func(_ context.Context) closure to the outer function and emits an incorrect diagnostic. The ctxbackground linter (the direct analogue) uses break in the equivalent position to stop the search when the innermost scoped context is blank/unusable. Test data covers GoodBlankContext as a top-level function only; the nested case is untested and broken.
2. Unnamed context.Context parameters silently skipped, no test coverage (lines 116–120)
Parameters written as func Foo(context.Context) (no name, not even _) produce an empty field.Names slice; the inner loop never runs and contextParamName returns false. Whether this should be treated identically to _ (skip) or flagged differently is a design decision that needs a test and a doc comment. Currently neither exists.
🔎 Code quality review by PR Code Quality Reviewer · ⌖ 13.5 AIC
| } | ||
| ctxParamName, hasCtx := contextParamName(pass, funcType) | ||
| if !hasCtx { | ||
| continue |
There was a problem hiding this comment.
False positive: blank-context closure nested inside a context-receiving function is incorrectly flagged.
When the innermost enclosing function has _ context.Context, contextParamName returns false and the loop continues — escalating to the outer function. A time.Sleep inside func(_ context.Context) will be reported with the outer function's ctx, even though the blank _ is an explicit signal that this scope intentionally ignores context.
Compare the analogous ctxbackground linter (line 50): it uses break when no usable context name is found, stopping the search rather than climbing up.
�� Failing scenario (not in test data)
func Outer(ctx context.Context) {
doWork(func(_ context.Context, d time.Duration) {
time.Sleep(d) // incorrectly flagged: "use select with ctx.Done()"
// — but the blank _ signals deliberate context ignorance
})
}The fix is to distinguish no context param (keep climbing) from blank/unnamed context param (stop climbing). One approach:
ctxParamName, hasCtx := contextParamName(pass, funcType)
if !hasCtx {
// If this scope has a context.Context param but it is blank/unnamed,
// don't escalate — the blank signals deliberate non-use.
if hasAnyContextParam(pass, funcType) {
break
}
continue
}A companion hasAnyContextParam (checks type only, ignores names) can be extracted from contextParamName.
| for _, name := range field.Names { | ||
| if name.Name != "_" { | ||
| return name.Name, true | ||
| } | ||
| } |
There was a problem hiding this comment.
Unnamed context.Context parameters are silently skipped with no test coverage — behaviour is undocumented.
When a context.Context field has no names at all (field.Names is empty, which happens for unnamed parameters like func Foo(context.Context) {}), the inner for _, name := range field.Names loop never executes and the function falls through to return "", false. This is a false negative — time.Sleep in func Foo(context.Context) { time.Sleep(...) } is not flagged.
Unnamed context parameters are idiomatic in interface implementations and stubs. Whether the intent is to treat them the same as _ context.Context (skip, no flag) or to flag them differently, neither the code comment nor the test data addresses this case.
💡 Missing test cases to add
// Unnamed context.Context — decide: Good or Bad?
func UnnamedContext(context.Context, d time.Duration) {
time.Sleep(d)
}If the intent is not to flag (same as _): add the test case with no // want annotation and update the doc comment to say "blank or unnamed context parameters are exempt."
If the intent is to flag (the parameter could be re-named and made cancellation-aware): add a // want annotation and handle the case by substituting a placeholder name like ctx in the diagnostic message.
|
@copilot run pr-finisher skill |
Summary
Adds a new
timesleepnocontextstatic-analysis linter that flags baretime.Sleepcalls inside any function, method, or closure that receives a non-blank
context.Contextparameter. A pre-existing scan found 13 such violations across9 non-test files — this linter closes the gap by enforcing the pattern
automatically in CI rather than relying on manual review.
The companion ADR-38704 documents the decision, alternatives considered, and
known consequences (including the 13 pre-existing violations that remain as
follow-up work).
Changes
pkg/linters/timesleepnocontext/timesleepnocontext.go(new)Core analysis pass (
golang.org/x/tools/go/analysis).*ast.CallExprnodes viaastutil.Inspector; skips non-time.Sleepcalls.filecheck.IsTestFile.*ast.FuncDecl/*ast.FuncLitnodes and resolves whether the function signature contains acontext.Contextparam using type-identity comparison._ context.Context) — only named, usable contexts trigger the diagnostic.use select with <ctx>.Done() instead of time.Sleep to allow context cancellation.pkg/linters/timesleepnocontext/testdata/src/timesleepnocontext/timesleepnocontext.go(new)Golden-file fixtures covering all significant cases:
context.Contextparamcontext.Contextparam_ context.Contextselect/time.After/ctx.Done()selectpkg/linters/timesleepnocontext/timesleepnocontext_test.go(new)Single
analysistest.Rununit test (build-tagged!integration) targeting thetestdata fixtures.
cmd/linters/main.go(modified)Registers
timesleepnocontext.Analyzerin the multichecker, making it activeunder
make golint-custom.docs/adr/38704-enforce-context-aware-sleep-via-custom-linter.md(new)Draft ADR documenting context, decision, alternatives, and consequences.
Must be finalized before merge.
Testing
analysistest.Runexercises all golden-file cases (bad × 3, good × 4).make golint-custompipeline; no additional integration test was added for the wiring.
Notes / Follow-up
pkg/+cmd/) must befixed or individually suppressed before CI enforcement becomes blocking.
docs/adr/38704-enforce-context-aware-sleep-via-custom-linter.md.select { case <-time.After(d): ... case <-ctx.Done(): ... }rewrite._ context.Context) are explicitly out of scopeby design; callers that shadow
_with a real name in the body are notcurrently detected.