Merge pull request #1906 from ksylvan/kayvan/christmas-2025-code-cleanups

Code Quality: Optimize HTTP client reuse + simplify error formatting

cmd/generate_changelog/incoming/1906.txt

@@ -0,0 +1,7 @@
+### PR [#1906](https://github.com/danielmiessler/Fabric/pull/1906) by [ksylvan](https://github.com/ksylvan): Code Quality: Optimize HTTP client reuse + simplify error formatting
+
+- Refactor: optimize HTTP client reuse and simplify error formatting
+- Simplify error wrapping by removing redundant Sprintf calls in CLI
+- Pass HTTP client to FetchModelsDirectly to enable connection reuse
+- Store persistent HTTP client instance inside the OpenAI provider struct
+- Update compatible AI providers to match the new function signature

internal/cli/output.go

@@ -14,19 +14,19 @@ import (
 
 func CopyToClipboard(message string) (err error) {
 	if err = clipboard.WriteAll(message); err != nil {
-		err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("could_not_copy_to_clipboard"), err))
+		err = fmt.Errorf(i18n.T("could_not_copy_to_clipboard"), err)
 	}
 	return
 }
 
 func CreateOutputFile(message string, fileName string) (err error) {
 	if _, err = os.Stat(fileName); err == nil {
-		err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("file_already_exists_not_overwriting"), fileName))
+		err = fmt.Errorf(i18n.T("file_already_exists_not_overwriting"), fileName)
 		return
 	}
 	var file *os.File
 	if file, err = os.Create(fileName); err != nil {
-		err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("error_creating_file"), err))
+		err = fmt.Errorf(i18n.T("error_creating_file"), err)
 		return
 	}
 	defer file.Close()
@@ -34,7 +34,7 @@ func CreateOutputFile(message string, fileName string) (err error) {
 		message += "\n"
 	}
 	if _, err = file.WriteString(message); err != nil {
-		err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("error_writing_to_file"), err))
+		err = fmt.Errorf(i18n.T("error_writing_to_file"), err)
 	} else {
 		debuglog.Log("\n\n[Output also written to %s]\n", fileName)
 	}
@@ -51,13 +51,13 @@ func CreateAudioOutputFile(audioData []byte, fileName string) (err error) {
 	// File existence check is now done in the CLI layer before TTS generation
 	var file *os.File
 	if file, err = os.Create(fileName); err != nil {
-		err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("error_creating_audio_file"), err))
+		err = fmt.Errorf(i18n.T("error_creating_audio_file"), err)
 		return
 	}
 	defer file.Close()
 
 	if _, err = file.Write(audioData); err != nil {
-		err = fmt.Errorf("%s", fmt.Sprintf(i18n.T("error_writing_audio_data"), err))
+		err = fmt.Errorf(i18n.T("error_writing_audio_data"), err)
 	}
 	// No redundant output message here - the CLI layer handles success messaging
 	return

internal/plugins/ai/openai/direct_models.go

@@ -30,7 +30,8 @@ const maxResponseSize = 10 * 1024 * 1024 // 10MB
 // standard OpenAI SDK method fails due to a nonstandard format. This is useful
 // for providers that return a direct array of models (e.g., GitHub Models) or
 // other OpenAI-compatible implementations.
-func FetchModelsDirectly(ctx context.Context, baseURL, apiKey, providerName string) ([]string, error) {
+// If httpClient is nil, a new client with default settings will be created.
+func FetchModelsDirectly(ctx context.Context, baseURL, apiKey, providerName string, httpClient *http.Client) ([]string, error) {
 	if ctx == nil {
 		ctx = context.Background()
 	}
@@ -52,10 +53,12 @@ func FetchModelsDirectly(ctx context.Context, baseURL, apiKey, providerName stri
 	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", apiKey))
 	req.Header.Set("Accept", "application/json")
 
-	// TODO: Consider reusing a single http.Client instance (e.g., as a field on Client) instead of allocating a new one for
-	// each request.
-	client := &http.Client{
-		Timeout: 10 * time.Second,
+	// Reuse provided HTTP client, or create a new one if not provided
+	client := httpClient
+	if client == nil {
+		client = &http.Client{
+			Timeout: 10 * time.Second,
+		}
 	}
 	resp, err := client.Do(req)
 	if err != nil {

internal/plugins/ai/openai/openai.go

@@ -3,8 +3,10 @@ package openai
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"slices"
 	"strings"
+	"time"
 
 	"github.com/danielmiessler/fabric/internal/chat"
 	"github.com/danielmiessler/fabric/internal/domain"
@@ -65,6 +67,7 @@ type Client struct {
 	ApiBaseURL          *plugins.SetupQuestion
 	ApiClient           *openai.Client
 	ImplementsResponses bool // Whether this provider supports the Responses API
+	httpClient          *http.Client
 }
 
 // SetResponsesAPIEnabled configures whether to use the Responses API
@@ -79,6 +82,11 @@ func (o *Client) configure() (ret error) {
 	}
 	client := openai.NewClient(opts...)
 	o.ApiClient = &client
+
+	// Initialize HTTP client for direct API calls (reused across requests)
+	o.httpClient = &http.Client{
+		Timeout: 10 * time.Second,
+	}
 	return
 }
 
@@ -96,7 +104,7 @@ func (o *Client) ListModels() (ret []string, err error) {
 	// Some providers (e.g., GitHub Models) return non-standard response formats
 	// that the SDK fails to parse.
 	debuglog.Debug(debuglog.Basic, "SDK Models.List failed for %s: %v, falling back to direct API fetch\n", o.GetName(), err)
-	return FetchModelsDirectly(context.Background(), o.ApiBaseURL.Value, o.ApiKey.Value, o.GetName())
+	return FetchModelsDirectly(context.Background(), o.ApiBaseURL.Value, o.ApiKey.Value, o.GetName(), o.httpClient)
 }
 
 func (o *Client) SendStream(

internal/plugins/ai/openai/openai_models_test.go

@@ -20,7 +20,7 @@ func TestFetchModelsDirectly_DirectArray(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider")
+	models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider", nil)
 	assert.NoError(t, err)
 	assert.Equal(t, 1, len(models))
 	assert.Equal(t, "github-model", models[0])
@@ -36,7 +36,7 @@ func TestFetchModelsDirectly_OpenAIFormat(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider")
+	models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider", nil)
 	assert.NoError(t, err)
 	assert.Equal(t, 1, len(models))
 	assert.Equal(t, "openai-model", models[0])
@@ -52,7 +52,7 @@ func TestFetchModelsDirectly_EmptyArray(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider")
+	models, err := FetchModelsDirectly(context.Background(), srv.URL, "test-key", "TestProvider", nil)
 	assert.NoError(t, err)
 	assert.Equal(t, 0, len(models))
 }

internal/plugins/ai/openai_compatible/direct_models_call.go

@@ -9,5 +9,5 @@ import (
 // DirectlyGetModels is used to fetch models directly from the API when the
 // standard OpenAI SDK method fails due to a nonstandard format.
 func (c *Client) DirectlyGetModels(ctx context.Context) ([]string, error) {
-	return openai.FetchModelsDirectly(ctx, c.ApiBaseURL.Value, c.ApiKey.Value, c.GetName())
+	return openai.FetchModelsDirectly(ctx, c.ApiBaseURL.Value, c.ApiKey.Value, c.GetName(), nil)
 }

internal/plugins/ai/openai_compatible/providers_config.go

@@ -47,7 +47,7 @@ func (c *Client) ListModels() ([]string, error) {
 		}
 		// TODO: Handle context properly in Fabric by accepting and propagating a context.Context
 		// instead of creating a new one here.
-		return openai.FetchModelsDirectly(context.Background(), c.modelsURL, c.Client.ApiKey.Value, c.GetName())
+		return openai.FetchModelsDirectly(context.Background(), c.modelsURL, c.Client.ApiKey.Value, c.GetName(), nil)
 	}
 
 	// First try the standard OpenAI SDK approach

internal/plugins/db/fsdb/patterns.go

@@ -65,7 +65,9 @@ func (o *PatternsEntity) loadPattern(source string) (pattern *Pattern, err error
 		}
 
 		// Use the resolved absolute path to get the pattern
-		pattern, _ = o.getFromFile(absPath)
+		if pattern, err = o.getFromFile(absPath); err != nil {
+			return nil, fmt.Errorf("could not load pattern from file %s: %w", absPath, err)
+		}
 	} else {
 		// Otherwise, get the pattern from the database
 		pattern, err = o.getFromDB(source)