lint: add SlogContextual cop and fix remaining bare slog calls#2674
lint: add SlogContextual cop and fix remaining bare slog calls#2674dgageot merged 6 commits intodocker:mainfrom
Conversation
The slog contextual cop had two bugs: 1. Anonymous context parameters (e.g., func(context.Context)) were not detected because the code only checked f.Names, which is empty for anonymous params. 2. Multi-return assignments with context not at position 0 (e.g., err, ctx := fn()) were missed because the code only checked lhs[0]. Fixed by: - Checking len(f.Names) == 0 as a special case for anonymous params - Using slices.ContainsFunc to check all LHS positions for named idents Also fixed newly-detected issues in pkg/acp/agent.go where anonymous context parameters were not being used in slog calls.
The 'err, ctx := fn()' case the previous commit aimed at can't be detected anyway: contextProducer only matches calls into the context package or zero-arg .Context() methods, none of which return (error, Context). Meanwhile, the broader check turned '_, cancel := context.WithCancel(...)' into a false positive — that form discards the context, so no usable name is in scope.
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR correctly adds the SlogContextual cop and converts 24 bare slog.X call sites to their *Context variants. The cop logic is sound for the syntactic approach chosen, all context bindings in the converted files are valid and in-scope, and the defer-closure captures (cmd/root/api.go) are safe.
Minor observation (lint/slog_contextual.go, contextProducer): The condition sel == "Context" && len(call.Args) == 0 is a heuristic that over-matches any zero-arg .Context() call regardless of return type — myWidget.Context() returning a custom non-context.Context type would set hasContext = true in the enclosing function, potentially causing a false positive lint offense. In practice this is safe given the strong Go convention that .Context() returns context.Context, but it's worth a brief note in the doc comment should a false positive appear in future code.
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The changes look correct. The new Lint/SlogContextual cop is well-structured, its AST-walking logic appropriately handles parameters, named results, anonymous params, locals derived from cmd.Context()/context.With*, var forms, and closures. All 24 bare slog.X conversions to their *Context variants use the correct in-scope ctx variable. The early ctx := cmd.Context() move in flags.go is safe (it's a pure accessor with no side effects). Anonymous parameter renames in pkg/acp/agent.go introduce no shadowing or naming conflicts. No bugs found in the added code.
Follow-up to #2669. That PR converted
slog.{Debug,Info,Warn,Error}to their*Contextvariants where acontext.Contextwas passed in as a parameter, but missed every site where the context was locally derived —ctx := cmd.Context(),ctx, cancel := context.WithTimeout(...), closures capturing an outerctx, etc. The OpenTelemetry handler we register onslogreads trace/span IDs from the active context, so any record emitted via the bare helper drops out of the trace.This PR adds a custom linter cop that catches those cases automatically, and fixes the ones that were still hiding in the codebase.
What's in the PR
Lint/SlogContextualcop (lint/slog_contextual.go)A custom rubocop-go cop that flags
slog.{Debug,Info,Warn,Error}(...)whenever acontext.Contextis reachable in the enclosing function. It recognises:context.Context(named or anonymous);ctx := cmd.Context(),ctx := context.Background()/TODO(), and the first return ofcontext.With*(...);var ctx context.Contextandvar ctx = context.Background()forms;defer func() { slog.Error(...) }()).Helpers without an in-scope context are intentionally not flagged — rewriting them would force callers to thread a context through APIs that don't otherwise need one. Per-line opt-out via
//rubocop:disable Lint/SlogContextual.I tried a type-aware implementation using
types.Scope.Innermost(pos), which would have collapsed the cop to ~60 lines, but the rubocop-go runner type-checks each package without anImporter, so cross-package types likecontext.Contextresolve to "invalid type" and the cop never matches. Stuck with the syntactic version.Conversions surfaced by the cop
The cop found 24 bare
slog.Xcalls in functions where a context was already in scope. All converted in this PR:cmd/root/api.go(4) — defer closures and the top-level "Starting server" logcmd/root/debug.go(2) — toolset listing errorscmd/root/eval.go(2) — session save errorscmd/root/pull.go,cmd/root/push.go— "Starting pull/push" debug logscmd/root/run.go::stopToolSets— error log inside the cancel-bound scopepkg/fake/proxy.go(3) — recording-proxy cleanup and VCR request errorpkg/tui/handlers.go(2) andpkg/tui/tui.go(7) — tab/session persistence warningspkg/acp/agent.go(5) — ACP method handlers; theircontext.Contextparameter was anonymous, so the linter fix that picks up anonymous params surfaced these too. Renamed each anonymous param toctxand switched toslog.DebugContext.cmd/root/flags.goalso movesctx := cmd.Context()above theloadUserConfig()call so the now-context-awareslog.WarnContexthas a context in scope.cmd.Context()is just a closure accessor on the capturedcmd, so calling it earlier is safe.Validation
task lint— clean (golangci-lint + 12 custom cops on 948 files)task test— all packages passtask build— binary built:=, multi-return,cmd.Context(),varforms, closures inheriting outer ctx, closures with their own ctx,_discard, no-ctx, already-using-Context-variant) — all expected offenses fired and all non-cases stayed silent.Commit history
Each commit is focused so the rationale is easy to follow:
lint: add slog contextual cop and fix remaining bare calls— the cop and the bulk of the conversions.simplify slog cop, drop positional scope tracking— movedctx := cmd.Context()to the top offlags.go::PersistentPreRunEso the cop no longer needs per-position scope analysis (1-bit-per-scope is enough).shorten slog cop using cop.MatchSelector and unifying helpers— removedstringsandcontext.With*-specific logic; any call into thecontextpackage counts.split slog cop helpers along ast node boundaries— extractedreportIfBareSlogandvalueSpecDeclaresContext; renamed for symmetry (signatureDeclaresContext/bodyDeclaresContext).fix linter: handle anonymous context params and multi-return contexts— handlesfunc(context.Context)and surfaces 5 cases inpkg/acp/agent.go.revert misguided multi-return change in slog cop— kept the anonymous-param fix from the previous commit; reverted the LHS-position widening, which was a latent false positive for_, cancel := context.WithCancel(...)and didn't address any case the cop could actually detect.