From 4958ea5100ebf8d4ff9fd818b7bc59d01989feb4 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Mon, 23 Mar 2026 08:08:57 +0200 Subject: fix: address all HIGH-severity code quality audit findings - lsp/server.go: track request goroutines in inflight WaitGroup to prevent use-after-close writes on shutdown - lsp/llm_client_registry.go: acquire write lock before calling build() to eliminate TOCTOU race on cache population - lsp/handlers_codeaction.go: resolveSimplifyCodeAction now uses PromptCodeActionSimplify{System,User} (was wrongly using rewrite prompts) - askcli/taskexport.go: remove exported MustParseTaskExport to prevent panic on malformed external input; move to unexported test helper - cmd/ask/main.go: print error to stderr before os.Exit - llm/{openai,ollama,openrouter}.go: add interface satisfaction assertions - integrationtests/ask_test.go: replace type assertions with errors.As for robust exec.ExitError unwrapping Co-Authored-By: Claude Sonnet 4.6 --- internal/lsp/llm_client_registry.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) (limited to 'internal/lsp/llm_client_registry.go') diff --git a/internal/lsp/llm_client_registry.go b/internal/lsp/llm_client_registry.go index 6b9c722..a018158 100644 --- a/internal/lsp/llm_client_registry.go +++ b/internal/lsp/llm_client_registry.go @@ -71,17 +71,28 @@ func (r *llmClientRegistry) clientFor(spec requestSpec, cfg appconfig.App, build if modelOverride == "" { modelOverride = strings.TrimSpace(spec.fallbackModel) } + + // Acquire write lock before calling build to prevent concurrent goroutines + // from all passing the read-lock cache-miss check and issuing duplicate + // build calls for the same provider (TOCTOU race). + r.clientsMu.Lock() + defer r.clientsMu.Unlock() + + // Re-check under the write lock; another goroutine may have populated the + // cache between our RUnlock and this Lock. + if provider == r.llmProvider && r.llmClient != nil { + return r.llmClient + } + if existing, ok := r.altClients[provider]; ok { + return existing + } + client, err := build(cfg, provider, modelOverride) if err != nil { logging.Logf("lsp ", "failed to build client for provider=%s: %v", provider, err) - if baseClient != nil { - return baseClient - } - return nil + return baseClient // may be nil; callers must handle nil } - r.clientsMu.Lock() - defer r.clientsMu.Unlock() if provider == r.llmProvider { if r.llmClient == nil { r.llmClient = client @@ -89,9 +100,6 @@ func (r *llmClientRegistry) clientFor(spec requestSpec, cfg appconfig.App, build } return r.llmClient } - if existing, ok := r.altClients[provider]; ok { - return existing - } if r.altClients == nil { r.altClients = make(map[string]llm.Client) } -- cgit v1.2.3