From c849b7f3502f291294bdb074b884daa942ce88fc Mon Sep 17 00:00:00 2001 From: David Gageot Date: Sat, 25 Apr 2026 19:59:47 +0200 Subject: [PATCH] lint: enable noctx and deduplicate touched code Adds noctx to the enabled golangci-lint set and threads context through the HTTP, SQL, exec and net APIs it flags. - Tests use t.Context() with the *Context / WithContext variants. - Production code passes ctx where it is already in scope, uses context.Background() for genuine background work (DB schema migrations, sqlite Ping at startup, fire-and-forget audio playback) and adds //nolint:noctx for the few intentionally non-context-bound calls (tea.ExecProcess-driven interactive shells/editors and the explicit kill+WaitDelay flow in the shell tool). While here, factor out the patterns that ended up duplicated: - pkg/sqliteutil.CheckpointAndClose, used by the three RAG strategy databases. - pkg/skills.httpGet, used by both cache.go and remote.go. - pkg/sound.runDetached, used by all platform-specific play funcs. - pkg/tools/mcp listenTCPRetry test helper, used in three places. - pkg/tui handlers split into newInteractiveShellCmd + a single execCmd wrapper carrying the only //nolint:noctx (was four). No behavior change. Assisted-By: docker-agent --- .golangci.yml | 1 + e2e/api_test.go | 4 +- pkg/fake/proxy_test.go | 12 ++--- pkg/httpclient/client_test.go | 2 +- pkg/model/provider/anthropic/wrap_test.go | 15 +++--- pkg/model/provider/bedrock/client_test.go | 4 +- pkg/model/provider/oaistream/adapter_test.go | 4 +- pkg/model/provider/oaistream/wrap_test.go | 15 +++--- pkg/rag/strategy/bm25_database.go | 7 +-- .../strategy/chunked_embeddings_database.go | 7 +-- .../strategy/semantic_embeddings_database.go | 9 ++-- pkg/remote/transport_test.go | 4 +- pkg/session/store_test.go | 2 +- pkg/skills/cache.go | 24 ++++++++-- pkg/skills/remote.go | 8 +--- pkg/sound/sound.go | 15 ++++-- pkg/sqliteutil/sqlite.go | 15 +++++- pkg/telemetry/http.go | 11 ++--- pkg/toolinstall/registry_test.go | 4 +- pkg/tools/builtin/lsp_test.go | 6 +-- pkg/tools/builtin/shell.go | 7 ++- pkg/tools/builtin/shell_test.go | 4 +- pkg/tools/mcp/oauth_server.go | 3 +- pkg/tools/mcp/oauth_test.go | 6 ++- pkg/tools/mcp/reconnect_test.go | 34 +++++++------- pkg/tui/handlers.go | 47 +++++++++++-------- pkg/tui/service/tuistate/store.go | 11 +++-- pkg/tui/tui.go | 3 +- pkg/upstream/headers_test.go | 2 +- 29 files changed, 167 insertions(+), 119 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 6faf59942..861a57fbf 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -41,6 +41,7 @@ linters: - mirror - modernize - nakedret + - noctx - nolintlint - nosprintfhostport - nilnesserr diff --git a/e2e/api_test.go b/e2e/api_test.go index 966eb5da7..ed51b5196 100644 --- a/e2e/api_test.go +++ b/e2e/api_test.go @@ -47,7 +47,9 @@ func TestCagentAPI_ListSessions(t *testing.T) { }, } - resp, err := client.Get("http://localhost/api/sessions") + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://localhost/api/sessions", http.NoBody) + require.NoError(t, err) + resp, err := client.Do(req) require.NoError(t, err) defer resp.Body.Close() diff --git a/pkg/fake/proxy_test.go b/pkg/fake/proxy_test.go index a0bb90c91..752681474 100644 --- a/pkg/fake/proxy_test.go +++ b/pkg/fake/proxy_test.go @@ -61,7 +61,7 @@ func TestAPIKeyHeaderUpdater(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Setenv(tt.envKey, tt.envValue) - req, err := http.NewRequest(http.MethodPost, "https://example.com", http.NoBody) + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, "https://example.com", http.NoBody) require.NoError(t, err) APIKeyHeaderUpdater(tt.host, req) @@ -72,7 +72,7 @@ func TestAPIKeyHeaderUpdater(t *testing.T) { } func TestAPIKeyHeaderUpdater_UnknownHost(t *testing.T) { - req, err := http.NewRequest(http.MethodPost, "https://example.com", http.NoBody) + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, "https://example.com", http.NoBody) require.NoError(t, err) APIKeyHeaderUpdater("https://unknown.host.com", req) @@ -222,7 +222,7 @@ func TestStreamCopy_ContextCancellation(t *testing.T) { // Create an echo context with a request that has a cancelable context e := echo.New() - req := httptest.NewRequest(http.MethodGet, "/", http.NoBody) + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", http.NoBody) rec := &readerFromRecorder{httptest.NewRecorder()} ctx, cancel := context.WithCancel(t.Context()) req = req.WithContext(ctx) @@ -258,7 +258,7 @@ func TestStreamCopy_NormalCompletion(t *testing.T) { // Create an echo context with a wrapped recorder e := echo.New() - req := httptest.NewRequest(http.MethodGet, "/", http.NoBody) + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", http.NoBody) rec := &readerFromRecorder{httptest.NewRecorder()} c := e.NewContext(req, rec) @@ -279,7 +279,7 @@ func TestSimulatedStreamCopy_SSEEvents(t *testing.T) { // Create an echo context e := echo.New() - req := httptest.NewRequest(http.MethodGet, "/", http.NoBody) + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", http.NoBody) rec := httptest.NewRecorder() c := e.NewContext(req, rec) @@ -340,7 +340,7 @@ func TestSimulatedStreamCopy_ContextCancellation(t *testing.T) { } e := echo.New() - req := httptest.NewRequest(http.MethodGet, "/", http.NoBody) + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", http.NoBody) rec := httptest.NewRecorder() ctx, cancel := context.WithCancel(t.Context()) req = req.WithContext(ctx) diff --git a/pkg/httpclient/client_test.go b/pkg/httpclient/client_test.go index e24b1d028..511c9a83b 100644 --- a/pkg/httpclient/client_test.go +++ b/pkg/httpclient/client_test.go @@ -76,7 +76,7 @@ func doRequest(t *testing.T, opts ...Opt) http.Header { defer srv.Close() client := NewHTTPClient(t.Context(), opts...) - req, err := http.NewRequest(http.MethodGet, srv.URL, http.NoBody) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL, http.NoBody) require.NoError(t, err) resp, err := client.Do(req) diff --git a/pkg/model/provider/anthropic/wrap_test.go b/pkg/model/provider/anthropic/wrap_test.go index 25f27d4eb..5d7192184 100644 --- a/pkg/model/provider/anthropic/wrap_test.go +++ b/pkg/model/provider/anthropic/wrap_test.go @@ -17,7 +17,8 @@ import ( // makeTestAnthropicError creates an *anthropic.Error with the given status code and // optional Retry-After header value for testing. -func makeTestAnthropicError(statusCode int, retryAfterValue string) *anthropic.Error { +func makeTestAnthropicError(t *testing.T, statusCode int, retryAfterValue string) *anthropic.Error { + t.Helper() header := http.Header{} if retryAfterValue != "" { header.Set("Retry-After", retryAfterValue) @@ -26,7 +27,7 @@ func makeTestAnthropicError(statusCode int, retryAfterValue string) *anthropic.E resp.StatusCode = statusCode resp.Header = header // anthropic.Error.Error() dereferences Request, so we must provide a non-nil one. - req, _ := http.NewRequest(http.MethodPost, "https://api.anthropic.com/v1/messages", http.NoBody) + req, _ := http.NewRequestWithContext(t.Context(), http.MethodPost, "https://api.anthropic.com/v1/messages", http.NoBody) return &anthropic.Error{ StatusCode: statusCode, Response: resp, @@ -53,7 +54,7 @@ func TestWrapAnthropicError(t *testing.T) { t.Run("429 without Retry-After wraps with zero RetryAfter", func(t *testing.T) { t.Parallel() - apiErr := makeTestAnthropicError(429, "") + apiErr := makeTestAnthropicError(t, 429, "") result := wrapAnthropicError(apiErr) var se *modelerrors.StatusError require.ErrorAs(t, result, &se) @@ -65,7 +66,7 @@ func TestWrapAnthropicError(t *testing.T) { t.Run("429 with Retry-After header sets RetryAfter", func(t *testing.T) { t.Parallel() - apiErr := makeTestAnthropicError(429, "20") + apiErr := makeTestAnthropicError(t, 429, "20") result := wrapAnthropicError(apiErr) var se *modelerrors.StatusError require.ErrorAs(t, result, &se) @@ -75,7 +76,7 @@ func TestWrapAnthropicError(t *testing.T) { t.Run("500 wraps with correct status code", func(t *testing.T) { t.Parallel() - apiErr := makeTestAnthropicError(500, "") + apiErr := makeTestAnthropicError(t, 500, "") result := wrapAnthropicError(apiErr) var se *modelerrors.StatusError require.ErrorAs(t, result, &se) @@ -85,7 +86,7 @@ func TestWrapAnthropicError(t *testing.T) { t.Run("wrapped error is classified correctly by ClassifyModelError", func(t *testing.T) { t.Parallel() - apiErr := makeTestAnthropicError(429, "15") + apiErr := makeTestAnthropicError(t, 429, "15") result := wrapAnthropicError(apiErr) retryable, rateLimited, retryAfter := modelerrors.ClassifyModelError(result) assert.False(t, retryable) @@ -95,7 +96,7 @@ func TestWrapAnthropicError(t *testing.T) { t.Run("wrapped in fmt.Errorf still classified correctly", func(t *testing.T) { t.Parallel() - apiErr := makeTestAnthropicError(429, "5") + apiErr := makeTestAnthropicError(t, 429, "5") wrapped := fmt.Errorf("stream error: %w", wrapAnthropicError(apiErr)) retryable, rateLimited, retryAfter := modelerrors.ClassifyModelError(wrapped) assert.False(t, retryable) diff --git a/pkg/model/provider/bedrock/client_test.go b/pkg/model/provider/bedrock/client_test.go index 7f91a19b0..f299d3b1e 100644 --- a/pkg/model/provider/bedrock/client_test.go +++ b/pkg/model/provider/bedrock/client_test.go @@ -247,7 +247,9 @@ func TestBearerTokenTransport(t *testing.T) { // Make a request through the transport client := &http.Client{Transport: transport} - resp, err := client.Get(server.URL) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, server.URL, http.NoBody) + require.NoError(t, err) + resp, err := client.Do(req) require.NoError(t, err) defer resp.Body.Close() diff --git a/pkg/model/provider/oaistream/adapter_test.go b/pkg/model/provider/oaistream/adapter_test.go index 28a74da45..604d58f89 100644 --- a/pkg/model/provider/oaistream/adapter_test.go +++ b/pkg/model/provider/oaistream/adapter_test.go @@ -21,7 +21,9 @@ func newTestStream(t *testing.T, sseData string) *ssestream.Stream[openai.ChatCo })) t.Cleanup(srv.Close) - resp, err := http.Get(srv.URL) //nolint:gosec,bodyclose // body is closed by the stream + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL, http.NoBody) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) //nolint:bodyclose // body is closed by the stream require.NoError(t, err) return ssestream.NewStream[openai.ChatCompletionChunk](ssestream.NewDecoder(resp), nil) } diff --git a/pkg/model/provider/oaistream/wrap_test.go b/pkg/model/provider/oaistream/wrap_test.go index eeb43608d..1e88922f9 100644 --- a/pkg/model/provider/oaistream/wrap_test.go +++ b/pkg/model/provider/oaistream/wrap_test.go @@ -17,7 +17,8 @@ import ( // makeTestOpenAIError creates an *openai.Error with the given status code and // optional Retry-After header value for testing. -func makeTestOpenAIError(statusCode int, retryAfterValue string) *openaisdk.Error { +func makeTestOpenAIError(t *testing.T, statusCode int, retryAfterValue string) *openaisdk.Error { + t.Helper() header := http.Header{} if retryAfterValue != "" { header.Set("Retry-After", retryAfterValue) @@ -26,7 +27,7 @@ func makeTestOpenAIError(statusCode int, retryAfterValue string) *openaisdk.Erro resp.StatusCode = statusCode resp.Header = header // openai.Error.Error() dereferences Request, so we must provide a non-nil one. - req, _ := http.NewRequest(http.MethodPost, "https://api.openai.com/v1/chat/completions", http.NoBody) + req, _ := http.NewRequestWithContext(t.Context(), http.MethodPost, "https://api.openai.com/v1/chat/completions", http.NoBody) return &openaisdk.Error{ StatusCode: statusCode, Response: resp, @@ -53,7 +54,7 @@ func TestWrapOpenAIError(t *testing.T) { t.Run("429 without Retry-After wraps with zero RetryAfter", func(t *testing.T) { t.Parallel() - apiErr := makeTestOpenAIError(429, "") + apiErr := makeTestOpenAIError(t, 429, "") result := WrapOpenAIError(apiErr) var se *modelerrors.StatusError require.ErrorAs(t, result, &se) @@ -65,7 +66,7 @@ func TestWrapOpenAIError(t *testing.T) { t.Run("429 with Retry-After header sets RetryAfter", func(t *testing.T) { t.Parallel() - apiErr := makeTestOpenAIError(429, "30") + apiErr := makeTestOpenAIError(t, 429, "30") result := WrapOpenAIError(apiErr) var se *modelerrors.StatusError require.ErrorAs(t, result, &se) @@ -75,7 +76,7 @@ func TestWrapOpenAIError(t *testing.T) { t.Run("500 wraps with correct status code", func(t *testing.T) { t.Parallel() - apiErr := makeTestOpenAIError(500, "") + apiErr := makeTestOpenAIError(t, 500, "") result := WrapOpenAIError(apiErr) var se *modelerrors.StatusError require.ErrorAs(t, result, &se) @@ -84,7 +85,7 @@ func TestWrapOpenAIError(t *testing.T) { t.Run("wrapped error is classified correctly by ClassifyModelError", func(t *testing.T) { t.Parallel() - apiErr := makeTestOpenAIError(429, "10") + apiErr := makeTestOpenAIError(t, 429, "10") result := WrapOpenAIError(apiErr) retryable, rateLimited, retryAfter := modelerrors.ClassifyModelError(result) assert.False(t, retryable) @@ -94,7 +95,7 @@ func TestWrapOpenAIError(t *testing.T) { t.Run("wrapped in fmt.Errorf still classified correctly", func(t *testing.T) { t.Parallel() - apiErr := makeTestOpenAIError(500, "") + apiErr := makeTestOpenAIError(t, 500, "") wrapped := fmt.Errorf("stream error: %w", WrapOpenAIError(apiErr)) retryable, rateLimited, _ := modelerrors.ClassifyModelError(wrapped) assert.True(t, retryable) diff --git a/pkg/rag/strategy/bm25_database.go b/pkg/rag/strategy/bm25_database.go index c78cee67f..e89990b94 100644 --- a/pkg/rag/strategy/bm25_database.go +++ b/pkg/rag/strategy/bm25_database.go @@ -81,7 +81,7 @@ func (d *bm25DB) createSchema() error { d.metadataTable, d.tablePrefix, d.metadataTable) - _, err := d.db.Exec(schema) + _, err := d.db.ExecContext(context.Background(), schema) return err } @@ -208,10 +208,7 @@ func (d *bm25DB) DeleteFileMetadata(ctx context.Context, sourcePath string) erro } func (d *bm25DB) Close() error { - if _, err := d.db.Exec("PRAGMA wal_checkpoint(TRUNCATE)"); err != nil { - slog.Warn("Failed to checkpoint WAL before close", "error", err) - } - return d.db.Close() + return sqliteutil.CheckpointAndClose(d.db) } // ensureDir creates the parent directory for a file path if it doesn't exist diff --git a/pkg/rag/strategy/chunked_embeddings_database.go b/pkg/rag/strategy/chunked_embeddings_database.go index d64895b97..5fa707302 100644 --- a/pkg/rag/strategy/chunked_embeddings_database.go +++ b/pkg/rag/strategy/chunked_embeddings_database.go @@ -77,7 +77,7 @@ func (d *chunkedVectorDB) createSchema() error { ); `, d.filesTable, d.tablePrefix, d.filesTable, d.chunksTable, d.filesTable) - _, err := d.db.Exec(schema) + _, err := d.db.ExecContext(context.Background(), schema) return err } @@ -244,8 +244,5 @@ func (d *chunkedVectorDB) DeleteFileMetadata(ctx context.Context, sourcePath str } func (d *chunkedVectorDB) Close() error { - if _, err := d.db.Exec("PRAGMA wal_checkpoint(TRUNCATE)"); err != nil { - slog.Warn("Failed to checkpoint WAL before close", "error", err) - } - return d.db.Close() + return sqliteutil.CheckpointAndClose(d.db) } diff --git a/pkg/rag/strategy/semantic_embeddings_database.go b/pkg/rag/strategy/semantic_embeddings_database.go index 3f35a8aa9..c8a83f638 100644 --- a/pkg/rag/strategy/semantic_embeddings_database.go +++ b/pkg/rag/strategy/semantic_embeddings_database.go @@ -79,12 +79,12 @@ func (d *semanticVectorDB) createSchema() error { ); `, d.filesTable, d.tablePrefix, d.filesTable, d.chunksTable, d.filesTable) - if _, err := d.db.Exec(schema); err != nil { + if _, err := d.db.ExecContext(context.Background(), schema); err != nil { return err } // Migration for existing databases that don't have embedding_input column - _, _ = d.db.Exec(fmt.Sprintf(`ALTER TABLE %s ADD COLUMN embedding_input TEXT`, d.chunksTable)) + _, _ = d.db.ExecContext(context.Background(), fmt.Sprintf(`ALTER TABLE %s ADD COLUMN embedding_input TEXT`, d.chunksTable)) return nil } @@ -254,8 +254,5 @@ func (d *semanticVectorDB) DeleteFileMetadata(ctx context.Context, sourcePath st } func (d *semanticVectorDB) Close() error { - if _, err := d.db.Exec("PRAGMA wal_checkpoint(TRUNCATE)"); err != nil { - slog.Warn("Failed to checkpoint WAL before close", "error", err) - } - return d.db.Close() + return sqliteutil.CheckpointAndClose(d.db) } diff --git a/pkg/remote/transport_test.go b/pkg/remote/transport_test.go index b67da73ac..345eeb4a4 100644 --- a/pkg/remote/transport_test.go +++ b/pkg/remote/transport_test.go @@ -48,7 +48,9 @@ func TestNewTransport_WorksWithoutDesktopProxy(t *testing.T) { // Make a simple HTTP request to verify the transport works client := &http.Client{Transport: transport} - resp, err := client.Get(server.URL) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, server.URL, http.NoBody) + require.NoError(t, err) + resp, err := client.Do(req) require.NoError(t, err) defer resp.Body.Close() diff --git a/pkg/session/store_test.go b/pkg/session/store_test.go index b313bcb37..033765b47 100644 --- a/pkg/session/store_test.go +++ b/pkg/session/store_test.go @@ -647,7 +647,7 @@ func TestNewSQLiteSessionStore_RejectsNewerDatabase(t *testing.T) { // Inject a future migration into the database to simulate a newer version db, err := sql.Open("sqlite", dbPath) require.NoError(t, err) - _, err = db.Exec( + _, err = db.ExecContext(t.Context(), "INSERT INTO migrations (id, name, description, applied_at) VALUES (?, ?, ?, ?)", 9999, "9999_future_migration", "Added by a newer version", "2099-01-01T00:00:00Z") require.NoError(t, err) diff --git a/pkg/skills/cache.go b/pkg/skills/cache.go index c0ba466a0..0d0a6ee26 100644 --- a/pkg/skills/cache.go +++ b/pkg/skills/cache.go @@ -18,6 +18,24 @@ import ( "github.com/docker/docker-agent/pkg/remote" ) +// remoteHTTPTimeout caps each HTTP request made to a remote skills source. +const remoteHTTPTimeout = 30 * time.Second + +// httpGet performs a GET request using the standard remote transport so that +// Docker Desktop proxy/SSL settings are honoured. The returned response body +// must be closed by the caller. +func httpGet(ctx context.Context, url string) (*http.Response, error) { + client := &http.Client{ + Timeout: remoteHTTPTimeout, + Transport: remote.NewTransport(ctx), + } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) + if err != nil { + return nil, fmt.Errorf("creating request for %s: %w", url, err) + } + return client.Do(req) +} + type diskCache struct { baseDir string } @@ -70,11 +88,7 @@ func (c *diskCache) Get(baseURL, skillName, filePath string) (string, bool) { func (c *diskCache) FetchAndStore(ctx context.Context, baseURL, skillName, filePath, fileURL string) (string, error) { slog.Debug("Fetching remote skill file", "url", fileURL) - httpClient := &http.Client{ - Timeout: 30 * time.Second, - Transport: remote.NewTransport(ctx), - } - resp, err := httpClient.Get(fileURL) + resp, err := httpGet(ctx, fileURL) if err != nil { return "", fmt.Errorf("fetching %s: %w", fileURL, err) } diff --git a/pkg/skills/remote.go b/pkg/skills/remote.go index 7150f4359..2f3a6d003 100644 --- a/pkg/skills/remote.go +++ b/pkg/skills/remote.go @@ -10,10 +10,8 @@ import ( "path/filepath" "regexp" "strings" - "time" "github.com/docker/docker-agent/pkg/paths" - "github.com/docker/docker-agent/pkg/remote" ) // remoteIndex represents the index.json served at /.well-known/skills/index.json @@ -45,11 +43,7 @@ func loadRemoteSkillsWithCache(ctx context.Context, baseURL string, cache *diskC slog.Debug("Fetching remote skills index", "url", indexURL) - httpClient := &http.Client{ - Timeout: 30 * time.Second, - Transport: remote.NewTransport(ctx), - } - resp, err := httpClient.Get(indexURL) + resp, err := httpGet(ctx, indexURL) if err != nil { slog.Warn("Failed to fetch remote skills index", "url", indexURL, "error", err) return nil diff --git a/pkg/sound/sound.go b/pkg/sound/sound.go index 7b5a23ffe..7fd4d9b6a 100644 --- a/pkg/sound/sound.go +++ b/pkg/sound/sound.go @@ -3,6 +3,7 @@ package sound import ( + "context" "log/slog" "os/exec" "runtime" @@ -42,6 +43,12 @@ func playSound(event Event) error { } } +// runDetached executes a command for fire-and-forget audio playback. The +// process is short-lived and not tied to any caller-provided context. +func runDetached(name string, args ...string) error { + return exec.CommandContext(context.Background(), name, args...).Run() +} + func playDarwin(event Event) error { // Use macOS built-in system sounds via afplay var soundFile string @@ -51,7 +58,7 @@ func playDarwin(event Event) error { case Failure: soundFile = "/System/Library/Sounds/Basso.aiff" } - return exec.Command("afplay", soundFile).Run() + return runDetached("afplay", soundFile) } func playLinux(event Event) error { @@ -65,11 +72,11 @@ func playLinux(event Event) error { } if path, err := exec.LookPath("paplay"); err == nil { - return exec.Command(path, soundFile).Run() + return runDetached(path, soundFile) } // Fallback: terminal bell via printf - return exec.Command("printf", `\a`).Run() + return runDetached("printf", `\a`) } func playWindows(event Event) error { @@ -81,5 +88,5 @@ func playWindows(event Event) error { case Failure: script = `[System.Media.SystemSounds]::Hand.Play()` } - return exec.Command("powershell", "-NoProfile", "-NonInteractive", "-Command", script).Run() + return runDetached("powershell", "-NoProfile", "-NonInteractive", "-Command", script) } diff --git a/pkg/sqliteutil/sqlite.go b/pkg/sqliteutil/sqlite.go index b22b0e53f..f98d4a3da 100644 --- a/pkg/sqliteutil/sqlite.go +++ b/pkg/sqliteutil/sqlite.go @@ -1,9 +1,11 @@ package sqliteutil import ( + "context" "database/sql" "errors" "fmt" + "log/slog" "os" "path/filepath" @@ -40,7 +42,7 @@ func OpenDB(path string) (*sql.DB, error) { db.SetConnMaxLifetime(0) // Verify connection works (this will trigger file creation/open) - if err := db.Ping(); err != nil { + if err := db.PingContext(context.Background()); err != nil { db.Close() if IsCantOpenError(err) { return nil, DiagnoseDBOpenError(path, err) @@ -78,3 +80,14 @@ func DiagnoseDBOpenError(path string, originalErr error) error { return fmt.Errorf("cannot create database at %q: permission denied or file cannot be created in %q (original error: %w)", path, dir, originalErr) } + +// CheckpointAndClose runs a final WAL checkpoint and closes the database. +// The TRUNCATE checkpoint folds the -wal file back into the main database +// so it isn't left behind on disk after shutdown. A checkpoint failure is +// logged but does not prevent the close. +func CheckpointAndClose(db *sql.DB) error { + if _, err := db.ExecContext(context.Background(), "PRAGMA wal_checkpoint(TRUNCATE)"); err != nil { + slog.Warn("Failed to checkpoint WAL before close", "error", err) + } + return db.Close() +} diff --git a/pkg/telemetry/http.go b/pkg/telemetry/http.go index 3d802e7ed..28f83dcd3 100644 --- a/pkg/telemetry/http.go +++ b/pkg/telemetry/http.go @@ -123,8 +123,12 @@ func (tc *Client) performHTTPRequest(event *EventPayload, version string) error return fmt.Errorf("failed to marshal request to JSON: %w", err) } + // Send request with timeout context + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + // Create HTTP request - req, err := http.NewRequest(http.MethodPost, tc.endpoint, bytes.NewBuffer(jsonData)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, tc.endpoint, bytes.NewBuffer(jsonData)) if err != nil { return fmt.Errorf("failed to create HTTP request: %w", err) } @@ -148,11 +152,6 @@ func (tc *Client) performHTTPRequest(event *EventPayload, version string) error "payload", string(jsonData), ) - // Send request with timeout context - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - req = req.WithContext(ctx) - resp, err := tc.httpClient.Do(req) if err != nil { return fmt.Errorf("HTTP request failed: %w", err) diff --git a/pkg/toolinstall/registry_test.go b/pkg/toolinstall/registry_test.go index ad1c5d3d4..694eb55f6 100644 --- a/pkg/toolinstall/registry_test.go +++ b/pkg/toolinstall/registry_test.go @@ -312,7 +312,7 @@ func TestGithubToken_Empty(t *testing.T) { func TestSetGitHubAuth_WithToken(t *testing.T) { t.Setenv("GITHUB_TOKEN", "ghp_test_token") - req, err := http.NewRequest(http.MethodGet, "https://api.github.com/repos/test/test", http.NoBody) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://api.github.com/repos/test/test", http.NoBody) require.NoError(t, err) setGitHubAuth(req) @@ -323,7 +323,7 @@ func TestSetGitHubAuth_WithoutToken(t *testing.T) { t.Setenv("GITHUB_TOKEN", "") t.Setenv("GH_TOKEN", "") - req, err := http.NewRequest(http.MethodGet, "https://api.github.com/repos/test/test", http.NoBody) + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://api.github.com/repos/test/test", http.NoBody) require.NoError(t, err) setGitHubAuth(req) diff --git a/pkg/tools/builtin/lsp_test.go b/pkg/tools/builtin/lsp_test.go index 495804b85..64a1fff85 100644 --- a/pkg/tools/builtin/lsp_test.go +++ b/pkg/tools/builtin/lsp_test.go @@ -275,7 +275,7 @@ func TestLSPHandler_GetDiagnostics_NoDiagnostics(t *testing.T) { tool.handler.initialized.Store(true) // Pretend we have a running server by setting a non-nil cmd // We use exec.Command which creates a valid *exec.Cmd without running anything - tool.handler.cmd = exec.Command("true") + tool.handler.cmd = exec.CommandContext(t.Context(), "true") ctx := t.Context() @@ -295,7 +295,7 @@ func TestLSPHandler_GetDiagnostics_WithDiagnostics(t *testing.T) { // Mark as initialized to test the diagnostics retrieval path tool.handler.initialized.Store(true) // Pretend we have a running server - tool.handler.cmd = exec.Command("true") + tool.handler.cmd = exec.CommandContext(t.Context(), "true") // Manually set some diagnostics tool.handler.diagnostics["file:///test.go"] = []lspDiagnostic{ @@ -756,7 +756,7 @@ func TestLSPHandler_Workspace(t *testing.T) { // Mark as initialized and set server info/capabilities tool.handler.initialized.Store(true) - tool.handler.cmd = exec.Command("true") + tool.handler.cmd = exec.CommandContext(t.Context(), "true") tool.handler.serverInfo = &lspServerInfo{ Name: "gopls", Version: "v0.14.0", diff --git a/pkg/tools/builtin/shell.go b/pkg/tools/builtin/shell.go index 5d26bb61c..510e4c009 100644 --- a/pkg/tools/builtin/shell.go +++ b/pkg/tools/builtin/shell.go @@ -220,7 +220,10 @@ func (h *shellHandler) RunShell(ctx context.Context, params RunShellArgs) (*tool const waitDelayAfterShellExit = 500 * time.Millisecond func (h *shellHandler) runNativeCommand(timeoutCtx, ctx context.Context, command, cwd string, timeout time.Duration) *tools.ToolCallResult { - cmd := exec.Command(h.shell, append(h.shellArgsPrefix, command)...) + // Cancellation is handled manually below (timeoutCtx + Process.Kill + + // process group + WaitDelay), so we use exec.Command rather than + // exec.CommandContext to keep that flow in one place. + cmd := exec.Command(h.shell, append(h.shellArgsPrefix, command)...) //nolint:noctx // see comment above cmd.Env = h.env cmd.Dir = cwd cmd.SysProcAttr = platformSpecificSysProcAttr() @@ -275,7 +278,7 @@ func (h *shellHandler) RunShellBackground(_ context.Context, params RunShellBack counter := h.jobCounter.Add(1) jobID := fmt.Sprintf("job_%d_%d", time.Now().Unix(), counter) - cmd := exec.Command(h.shell, append(h.shellArgsPrefix, params.Cmd)...) + cmd := exec.Command(h.shell, append(h.shellArgsPrefix, params.Cmd)...) //nolint:noctx // RunShellBackground intentionally outlives the request context cmd.Env = h.env cmd.Dir = h.resolveWorkDir(params.Cwd) cmd.SysProcAttr = platformSpecificSysProcAttr() diff --git a/pkg/tools/builtin/shell_test.go b/pkg/tools/builtin/shell_test.go index 09f114578..08f5fde4b 100644 --- a/pkg/tools/builtin/shell_test.go +++ b/pkg/tools/builtin/shell_test.go @@ -359,7 +359,7 @@ func TestReapSpawnedChild(t *testing.T) { t.Skip("POSIX-specific: relies on /bin/sh and ProcessState.Exited()") } - cmd := exec.Command("/bin/sh", "-c", "sleep 60") + cmd := exec.CommandContext(t.Context(), "/bin/sh", "-c", "sleep 60") cmd.SysProcAttr = platformSpecificSysProcAttr() require.NoError(t, cmd.Start()) @@ -381,7 +381,7 @@ func TestReapSpawnedChild_HandlesAlreadyExited(t *testing.T) { t.Skip("POSIX-specific") } - cmd := exec.Command("/bin/sh", "-c", "exit 0") + cmd := exec.CommandContext(t.Context(), "/bin/sh", "-c", "exit 0") require.NoError(t, cmd.Start()) // Give the child a moment to exit on its own. time.Sleep(50 * time.Millisecond) diff --git a/pkg/tools/mcp/oauth_server.go b/pkg/tools/mcp/oauth_server.go index e6b1f1d0c..5a355ccb4 100644 --- a/pkg/tools/mcp/oauth_server.go +++ b/pkg/tools/mcp/oauth_server.go @@ -37,7 +37,8 @@ func NewCallbackServer() (*CallbackServer, error) { // NewCallbackServerOnPort creates a new OAuth callback server on a specific port. // Use port 0 to let the OS pick a random available port. func NewCallbackServerOnPort(port int) (*CallbackServer, error) { - listener, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", port)) + var lc net.ListenConfig + listener, err := lc.Listen(context.Background(), "tcp", fmt.Sprintf("127.0.0.1:%d", port)) if err != nil { return nil, fmt.Errorf("failed to find available port: %w", err) } diff --git a/pkg/tools/mcp/oauth_test.go b/pkg/tools/mcp/oauth_test.go index 573203898..7fa8b8617 100644 --- a/pkg/tools/mcp/oauth_test.go +++ b/pkg/tools/mcp/oauth_test.go @@ -355,7 +355,11 @@ func TestCallbackServer_RejectsCallbackBeforeStateSet(t *testing.T) { defer func() { _ = cs.Shutdown(t.Context()) }() // Send a callback before SetExpectedState has been called. - resp, err := http.Get(cs.GetRedirectURI() + "?code=authcode&state=anything") + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, cs.GetRedirectURI()+"?code=authcode&state=anything", http.NoBody) + if err != nil { + t.Fatal(err) + } + resp, err := http.DefaultClient.Do(req) if err != nil { t.Fatal(err) } diff --git a/pkg/tools/mcp/reconnect_test.go b/pkg/tools/mcp/reconnect_test.go index dfe837108..accb010d6 100644 --- a/pkg/tools/mcp/reconnect_test.go +++ b/pkg/tools/mcp/reconnect_test.go @@ -22,6 +22,21 @@ import ( "github.com/docker/docker-agent/pkg/tools" ) +// listenTCPRetry retries Listen on addr until it succeeds or a short +// deadline passes (the port may be momentarily busy after a previous +// server shutdown). The returned listener should be closed by the caller. +func listenTCPRetry(t *testing.T, addr string) net.Listener { + t.Helper() + var lc net.ListenConfig + var ln net.Listener + require.Eventually(t, func() bool { + var err error + ln, err = lc.Listen(t.Context(), "tcp", addr) + return err == nil + }, 2*time.Second, 50*time.Millisecond, "port %s not available in time", addr) + return ln +} + // startMCPServer creates a minimal MCP server on addr with the given tools // and returns a function to shut it down. func startMCPServer(t *testing.T, addr string, mcpTools ...*gomcp.Tool) (shutdown func()) { @@ -36,14 +51,7 @@ func startMCPServer(t *testing.T, addr string, mcpTools ...*gomcp.Tool) (shutdow }) } - // Retry Listen until the port is available (e.g. after a server shutdown). - var srvLn net.Listener - require.Eventually(t, func() bool { - var listenErr error - srvLn, listenErr = net.Listen("tcp", addr) - return listenErr == nil - }, 2*time.Second, 50*time.Millisecond, "port %s not available in time", addr) - + srvLn := listenTCPRetry(t, addr) srv := &http.Server{ Handler: gomcp.NewStreamableHTTPHandler(func(*http.Request) *gomcp.Server { return s }, nil), } @@ -55,8 +63,7 @@ func startMCPServer(t *testing.T, addr string, mcpTools ...*gomcp.Tool) (shutdow // allocateAddr returns a free TCP address on localhost. func allocateAddr(t *testing.T) string { t.Helper() - ln, err := net.Listen("tcp", "127.0.0.1:0") - require.NoError(t, err) + ln := listenTCPRetry(t, "127.0.0.1:0") addr := ln.Addr().String() ln.Close() return addr @@ -99,12 +106,7 @@ func TestRemoteReconnectAfterServerRestart(t *testing.T) { }) // Retry Listen until the port is available (e.g. after a server shutdown). - var srvLn net.Listener - require.Eventually(t, func() bool { - var listenErr error - srvLn, listenErr = net.Listen("tcp", addr) - return listenErr == nil - }, 2*time.Second, 50*time.Millisecond, "port %s not available in time", addr) + srvLn := listenTCPRetry(t, addr) srv := &http.Server{ Handler: gomcp.NewStreamableHTTPHandler(func(*http.Request) *gomcp.Server { return s }, nil), diff --git a/pkg/tui/handlers.go b/pkg/tui/handlers.go index d0585711f..f4477e1ad 100644 --- a/pkg/tui/handlers.go +++ b/pkg/tui/handlers.go @@ -640,28 +640,35 @@ func (m *appModel) handleElicitationResponse(action tools.ElicitationAction, con } func (m *appModel) startShell() (tea.Model, tea.Cmd) { - exitMsg := "Type 'exit' to return to " + m.appName - - var cmd *exec.Cmd - if goruntime.GOOS == "windows" { - if path, err := exec.LookPath("pwsh.exe"); err == nil { - cmd = exec.Command(path, "-NoLogo", "-NoExit", "-Command", - `Write-Host ""; Write-Host "`+exitMsg+`"`) - } else if path, err := exec.LookPath("powershell.exe"); err == nil { - cmd = exec.Command(path, "-NoLogo", "-NoExit", "-Command", - `Write-Host ""; Write-Host "`+exitMsg+`"`) - } else { - // Use absolute path to cmd.exe to prevent PATH hijacking (CWE-426). - shell := shellpath.WindowsCmdExe() - cmd = exec.Command(shell, "/K", "echo. & echo "+exitMsg) - } - } else { - shell := shellpath.DetectUnixShell() - cmd = exec.Command(shell, "-i", "-c", - `echo -e "\n`+exitMsg+`"; exec `+shell) - } + cmd := newInteractiveShellCmd("Type 'exit' to return to " + m.appName) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr return m, tea.ExecProcess(cmd, nil) } + +// newInteractiveShellCmd returns a command that launches the user's preferred +// interactive shell. The command is owned by tea.ExecProcess, not by any +// request-scoped context, so exec.Command is intentional. +func newInteractiveShellCmd(exitMsg string) *exec.Cmd { + if goruntime.GOOS != "windows" { + shell := shellpath.DetectUnixShell() + return execCmd(shell, "-i", "-c", `echo -e "\n`+exitMsg+`"; exec `+shell) + } + + psArgs := []string{"-NoLogo", "-NoExit", "-Command", `Write-Host ""; Write-Host "` + exitMsg + `"`} + if path, err := exec.LookPath("pwsh.exe"); err == nil { + return execCmd(path, psArgs...) + } + if path, err := exec.LookPath("powershell.exe"); err == nil { + return execCmd(path, psArgs...) + } + // Use absolute path to cmd.exe to prevent PATH hijacking (CWE-426). + return execCmd(shellpath.WindowsCmdExe(), "/K", "echo. & echo "+exitMsg) +} + +// execCmd is a thin wrapper around exec.Command used for interactive +// processes whose lifecycle is owned by tea.ExecProcess (not a context). +func execCmd(name string, args ...string) *exec.Cmd { + return exec.Command(name, args...) //nolint:noctx // owned by tea.ExecProcess +} diff --git a/pkg/tui/service/tuistate/store.go b/pkg/tui/service/tuistate/store.go index de0e33ddb..2797809e4 100644 --- a/pkg/tui/service/tuistate/store.go +++ b/pkg/tui/service/tuistate/store.go @@ -35,7 +35,8 @@ func New() (*Store, error) { // migrate runs database migrations. func (s *Store) migrate() error { - _, err := s.db.Exec(` + ctx := context.Background() + _, err := s.db.ExecContext(ctx, ` CREATE TABLE IF NOT EXISTS tabs ( session_id TEXT PRIMARY KEY, working_dir TEXT NOT NULL, @@ -63,18 +64,18 @@ func (s *Store) migrate() error { } // Add sidebar_collapsed column if it doesn't exist (migration for existing databases). - _, _ = s.db.Exec(`ALTER TABLE tabs ADD COLUMN sidebar_collapsed BOOLEAN NOT NULL DEFAULT 0`) + _, _ = s.db.ExecContext(ctx, `ALTER TABLE tabs ADD COLUMN sidebar_collapsed BOOLEAN NOT NULL DEFAULT 0`) // Add position column for explicit tab ordering. - _, _ = s.db.Exec(`ALTER TABLE tabs ADD COLUMN position INTEGER NOT NULL DEFAULT 0`) + _, _ = s.db.ExecContext(ctx, `ALTER TABLE tabs ADD COLUMN position INTEGER NOT NULL DEFAULT 0`) // Backfill position for databases that predate the column: assign positions // based on existing created_at order so the visible order is preserved. // Only runs when all positions are 0 (i.e. column was just added). var maxPos int - _ = s.db.QueryRow(`SELECT COALESCE(MAX(position), 0) FROM tabs`).Scan(&maxPos) + _ = s.db.QueryRowContext(ctx, `SELECT COALESCE(MAX(position), 0) FROM tabs`).Scan(&maxPos) if maxPos == 0 { - _, _ = s.db.Exec(` + _, _ = s.db.ExecContext(ctx, ` UPDATE tabs SET position = ( SELECT COUNT(*) FROM tabs t2 WHERE t2.created_at < tabs.created_at ) diff --git a/pkg/tui/tui.go b/pkg/tui/tui.go index 38c3bc422..6cb8fd9e2 100644 --- a/pkg/tui/tui.go +++ b/pkg/tui/tui.go @@ -2461,7 +2461,8 @@ func (m *appModel) openExternalEditor() (tea.Model, tea.Cmd) { // Parse editor command (may include arguments like "code --wait") parts := strings.Fields(editorCmd) args := append(parts[1:], tmpPath) - cmd := exec.Command(parts[0], args...) + // External editor is owned by tea.ExecProcess, so exec.Command is intentional. + cmd := exec.Command(parts[0], args...) //nolint:noctx // owned by tea.ExecProcess ed := m.editor return m, tea.ExecProcess(cmd, func(err error) tea.Msg { diff --git a/pkg/upstream/headers_test.go b/pkg/upstream/headers_test.go index f70c4d5b6..bf994e469 100644 --- a/pkg/upstream/headers_test.go +++ b/pkg/upstream/headers_test.go @@ -40,7 +40,7 @@ func TestHandler_InjectsHeaders(t *testing.T) { }) handler := Handler(inner) - req := httptest.NewRequest(http.MethodGet, "/", http.NoBody) + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", http.NoBody) req.Header.Set("X-Test", "hello") handler.ServeHTTP(httptest.NewRecorder(), req)