diff options
| author | Paul Buetow <paul@buetow.org> | 2026-03-23 08:27:18 +0200 |
|---|---|---|
| committer | Paul Buetow <paul@buetow.org> | 2026-03-23 08:27:18 +0200 |
| commit | 3ea11bc5d671d962d01b57fa0fba0bda611025fe (patch) | |
| tree | 4aa8c9d8a380a2a176a0a7302d07452a6996e2ef | |
| parent | 2d03ad0ba42bade8579578d12aecbf9a73d9af07 (diff) | |
fix: code quality improvements across lsp, askcli, appconfig, integrationtests
- lsp/handlers_completion.go: track collectFirstCompletion goroutine in
inflight WaitGroup (goroutine leak fix)
- lsp/transport.go: use %w instead of %v for error wrapping
- askcli/command_list.go: extract handleListWithFilters shared helper;
handleList/handleAll/handleReady are now single-liners
- askcli/command_list.go, urgency.go, dep.go: log ParseTaskExport errors
to stderr instead of returning 1 silently
- appconfig/config_load.go: rename 'any' variable to 'found' to avoid
shadowing the built-in identifier
- llm/provider.go: add explanatory comment for package-level registry
- integrationtests/ask_test.go: add //go:build integration tag; move
repoRoot init from init() to TestMain with diagnostic error message
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| -rw-r--r-- | integrationtests/ask_test.go | 9 | ||||
| -rw-r--r-- | internal/appconfig/config_load.go | 10 | ||||
| -rw-r--r-- | internal/askcli/command_dep.go | 1 | ||||
| -rw-r--r-- | internal/askcli/command_list.go | 69 | ||||
| -rw-r--r-- | internal/askcli/command_urgency.go | 2 | ||||
| -rw-r--r-- | internal/llm/provider.go | 8 | ||||
| -rw-r--r-- | internal/lsp/handlers_completion.go | 8 | ||||
| -rw-r--r-- | internal/lsp/transport.go | 2 |
8 files changed, 42 insertions, 67 deletions
diff --git a/integrationtests/ask_test.go b/integrationtests/ask_test.go index a762b9d..b0f574f 100644 --- a/integrationtests/ask_test.go +++ b/integrationtests/ask_test.go @@ -1,3 +1,5 @@ +//go:build integration + package integrationtests import ( @@ -17,6 +19,7 @@ import ( "codeberg.org/snonux/hexai/internal/askcli" ) +// repoRoot is set in TestMain before any test runs. var repoRoot string func findRepoRoot() string { @@ -40,10 +43,6 @@ func findRepoRoot() string { return "" } -func init() { - repoRoot = findRepoRoot() -} - func askBinaryPath() string { return filepath.Join(repoRoot, "cmd", "ask", "ask") } @@ -216,7 +215,9 @@ func getTaskInfoRaw(ctx context.Context, uuid string) (string, bool) { } func TestMain(m *testing.M) { + repoRoot = findRepoRoot() if repoRoot == "" { + fmt.Fprintln(os.Stderr, "integration tests: cannot find repo root (go.mod or .git)") os.Exit(1) } // Always rebuild the binary so tests reflect the current source. diff --git a/internal/appconfig/config_load.go b/internal/appconfig/config_load.go index 99461ca..cb02a2e 100644 --- a/internal/appconfig/config_load.go +++ b/internal/appconfig/config_load.go @@ -570,7 +570,7 @@ func parseSurfaceModels(raw map[string]any, logger *log.Logger) *App { *dest = append(*dest, entries...) return true } - any := appendEntries(&out.CompletionConfigs, "models.completion", table["completion"]) + found := appendEntries(&out.CompletionConfigs, "models.completion", table["completion"]) if ok := appendEntries(&out.CodeActionConfigs, "models.code_action", table["code_action"]); ok { if len(out.CodeActionConfigs) > 1 { if logger != nil { @@ -578,11 +578,11 @@ func parseSurfaceModels(raw map[string]any, logger *log.Logger) *App { } out.CodeActionConfigs = out.CodeActionConfigs[:1] } - any = true + found = true } - any = appendEntries(&out.ChatConfigs, "models.chat", table["chat"]) || any - any = appendEntries(&out.CLIConfigs, "models.cli", table["cli"]) || any - if !any { + found = appendEntries(&out.ChatConfigs, "models.chat", table["chat"]) || found + found = appendEntries(&out.CLIConfigs, "models.cli", table["cli"]) || found + if !found { return nil } return &out diff --git a/internal/askcli/command_dep.go b/internal/askcli/command_dep.go index 7f51fdc..6e3198c 100644 --- a/internal/askcli/command_dep.go +++ b/internal/askcli/command_dep.go @@ -74,6 +74,7 @@ func (d Dispatcher) handleDepList(ctx context.Context, args []string, stdout, st } tasks, err := ParseTaskExport(&outBuf) if err != nil { + fmt.Fprintf(stderr, "error: failed to parse task data: %v\n", err) return 1, nil } if len(tasks) == 0 { diff --git a/internal/askcli/command_list.go b/internal/askcli/command_list.go index 64ec2fb..54eaafc 100644 --- a/internal/askcli/command_list.go +++ b/internal/askcli/command_list.go @@ -3,74 +3,30 @@ package askcli import ( "bytes" "context" + "fmt" "io" "sort" "strings" ) func (d Dispatcher) handleList(ctx context.Context, args []string, stdout, stderr io.Writer) (int, error) { - filterArgs := []string{"status:pending"} - for _, arg := range args[1:] { - if strings.HasPrefix(arg, "limit:") || strings.HasPrefix(arg, "sort:") || - strings.HasPrefix(arg, "+") || arg == "started" { - filterArgs = append(filterArgs, arg) - } - } - filterArgs = append(filterArgs, "export") - var outBuf bytes.Buffer - code, err := d.runner.Run(ctx, filterArgs, nil, &outBuf, stderr) - if code != 0 { - return code, err - } - tasks, err := ParseTaskExport(&outBuf) - if err != nil { - return 1, nil - } - sort.Slice(tasks, func(i, j int) bool { - pi := priorityOrder(tasks[i].Priority) - pj := priorityOrder(tasks[j].Priority) - if pi != pj { - return pi < pj - } - return tasks[i].Urgency > tasks[j].Urgency - }) - io.WriteString(stdout, FormatTaskList(tasks)) - return 0, nil + return d.handleListWithFilters(ctx, []string{"status:pending"}, args[1:], stdout, stderr) } func (d Dispatcher) handleAll(ctx context.Context, args []string, stdout, stderr io.Writer) (int, error) { - filterArgs := []string{} - for _, arg := range args[1:] { - if strings.HasPrefix(arg, "limit:") || strings.HasPrefix(arg, "sort:") || - strings.HasPrefix(arg, "+") || arg == "started" { - filterArgs = append(filterArgs, arg) - } - } - filterArgs = append(filterArgs, "export") - var outBuf bytes.Buffer - code, err := d.runner.Run(ctx, filterArgs, nil, &outBuf, stderr) - if code != 0 { - return code, err - } - tasks, err := ParseTaskExport(&outBuf) - if err != nil { - return 1, nil - } - sort.Slice(tasks, func(i, j int) bool { - pi := priorityOrder(tasks[i].Priority) - pj := priorityOrder(tasks[j].Priority) - if pi != pj { - return pi < pj - } - return tasks[i].Urgency > tasks[j].Urgency - }) - io.WriteString(stdout, FormatTaskList(tasks)) - return 0, nil + return d.handleListWithFilters(ctx, nil, args[1:], stdout, stderr) } func (d Dispatcher) handleReady(ctx context.Context, args []string, stdout, stderr io.Writer) (int, error) { - filterArgs := []string{"+READY"} - for _, arg := range args[1:] { + return d.handleListWithFilters(ctx, []string{"+READY"}, args[1:], stdout, stderr) +} + +// handleListWithFilters is the shared implementation for list/all/ready. +// initialFilters seeds the taskwarrior filter; extraArgs are user-supplied +// filter modifiers (limit:, sort:, +tag, started). +func (d Dispatcher) handleListWithFilters(ctx context.Context, initialFilters, extraArgs []string, stdout, stderr io.Writer) (int, error) { + filterArgs := append([]string(nil), initialFilters...) + for _, arg := range extraArgs { if strings.HasPrefix(arg, "limit:") || strings.HasPrefix(arg, "sort:") || strings.HasPrefix(arg, "+") || arg == "started" { filterArgs = append(filterArgs, arg) @@ -84,6 +40,7 @@ func (d Dispatcher) handleReady(ctx context.Context, args []string, stdout, stde } tasks, err := ParseTaskExport(&outBuf) if err != nil { + fmt.Fprintf(stderr, "error: failed to parse task data: %v\n", err) return 1, nil } sort.Slice(tasks, func(i, j int) bool { diff --git a/internal/askcli/command_urgency.go b/internal/askcli/command_urgency.go index f2be17a..9dd162e 100644 --- a/internal/askcli/command_urgency.go +++ b/internal/askcli/command_urgency.go @@ -3,6 +3,7 @@ package askcli import ( "bytes" "context" + "fmt" "io" "sort" ) @@ -15,6 +16,7 @@ func (d Dispatcher) handleUrgency(ctx context.Context, stdout, stderr io.Writer) } tasks, err := ParseTaskExport(&outBuf) if err != nil { + fmt.Fprintf(stderr, "error: failed to parse task data: %v\n", err) return 1, nil } sort.Slice(tasks, func(i, j int) bool { diff --git a/internal/llm/provider.go b/internal/llm/provider.go index afc126b..3c72181 100644 --- a/internal/llm/provider.go +++ b/internal/llm/provider.go @@ -103,6 +103,14 @@ type ProviderKeys struct { // ProviderFactory builds an LLM client for a named provider. type ProviderFactory func(cfg Config, keys ProviderKeys) (Client, error) +// providerRegistry is a package-level singleton populated by init() calls in +// each provider file (anthropic.go, openai.go, etc.). It must be a +// package-level var — rather than a constructor argument — because Go's +// init() mechanism runs before any application code, and the alternative +// (an explicit RegisterAll() in main) would require every binary that uses +// the llm package to manually enumerate all providers. The RWMutex makes the +// map safe for the rare case where RegisterProvider is called from a test +// goroutine after init() has completed. var ( providerRegistryMu sync.RWMutex providerRegistry = map[string]ProviderFactory{} diff --git a/internal/lsp/handlers_completion.go b/internal/lsp/handlers_completion.go index aa22fc2..527d020 100644 --- a/internal/lsp/handlers_completion.go +++ b/internal/lsp/handlers_completion.go @@ -216,7 +216,13 @@ func collectCompletionResults(results <-chan completionJobResult) []CompletionIt func (s *Server) firstCompletionAndStore(results <-chan completionJobResult, cacheKey string, end func()) ([]CompletionItem, bool) { firstCh := make(chan []CompletionItem, 1) - go s.collectFirstCompletion(results, cacheKey, firstCh, end) + // Track this goroutine in inflight so Run's deferred Wait() catches it + // and prevents use-after-close writes on shutdown. + s.inflight.Add(1) + go func() { + defer s.inflight.Done() + s.collectFirstCompletion(results, cacheKey, firstCh, end) + }() firstItems, ok := <-firstCh if !ok || len(firstItems) == 0 { return nil, false diff --git a/internal/lsp/transport.go b/internal/lsp/transport.go index bca2c37..3547dfa 100644 --- a/internal/lsp/transport.go +++ b/internal/lsp/transport.go @@ -33,7 +33,7 @@ func (s *Server) readMessage() ([]byte, error) { case "content-length": n, err := strconv.Atoi(val) if err != nil { - return nil, fmt.Errorf("invalid Content-Length: %v", err) + return nil, fmt.Errorf("invalid Content-Length: %w", err) } contentLength = n } |
