refactor as per manual code reviews

2 files changed, 182 insertions, 183 deletions

diff --git a/internal/hexailsp/run.go b/internal/hexailsp/run.go
index 5a0ab4a..8a79b12 100644
--- a/internal/hexailsp/run.go
+++ b/internal/hexailsp/run.go
@@ -1,18 +1,17 @@
 // Summary: Hexai LSP runner; configures logging, loads config, builds the LLM client,
 // and constructs/runs the LSP server (with injectable factory for tests).
-// Not yet reviewed by a human
 package hexailsp
 
 import (
-    "log"
-    "os"
-    "strings"
-    "io"
+	"io"
+	"log"
+	"os"
+	"strings"
 
-    "hexai/internal/appconfig"
-    "hexai/internal/llm"
-    "hexai/internal/logging"
-    "hexai/internal/lsp"
+	"hexai/internal/appconfig"
+	"hexai/internal/llm"
+	"hexai/internal/logging"
+	"hexai/internal/lsp"
 )
 
 // ServerRunner is the minimal interface satisfied by lsp.Server.
@@ -24,68 +23,68 @@ type ServerFactory func(r io.Reader, w io.Writer, logger *log.Logger, opts lsp.S
 // Run configures logging, loads config, builds the LLM client and runs the LSP server.
 // It is thin and delegates to RunWithFactory for testability.
 func Run(logPath string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error {
-    logger := log.New(stderr, "hexai-lsp ", log.LstdFlags|log.Lmsgprefix)
-    if strings.TrimSpace(logPath) != "" {
-        f, err := os.OpenFile(logPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
-        if err != nil {
-            logger.Fatalf("failed to open log file: %v", err)
-        }
-        defer f.Close()
-        logger.SetOutput(f)
-    }
-    logging.Bind(logger)
-    cfg := appconfig.Load(logger)
-    return RunWithFactory(logPath, stdin, stdout, logger, cfg, nil, nil)
+	logger := log.New(stderr, "hexai-lsp ", log.LstdFlags|log.Lmsgprefix)
+	if strings.TrimSpace(logPath) != "" {
+		f, err := os.OpenFile(logPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
+		if err != nil {
+			logger.Fatalf("failed to open log file: %v", err)
+		}
+		defer f.Close()
+		logger.SetOutput(f)
+	}
+	logging.Bind(logger)
+	cfg := appconfig.Load(logger)
+	return RunWithFactory(logPath, stdin, stdout, logger, cfg, nil, nil)
 }
 
 // RunWithFactory is the testable entrypoint. When client is nil, it is built from cfg+env.
 // When factory is nil, lsp.NewServer is used.
 func RunWithFactory(logPath string, stdin io.Reader, stdout io.Writer, logger *log.Logger, cfg appconfig.App, client llm.Client, factory ServerFactory) error {
-    // Normalize and apply logging config
-    cfg.ContextMode = strings.ToLower(strings.TrimSpace(cfg.ContextMode))
-    if cfg.LogPreviewLimit >= 0 {
-        logging.SetLogPreviewLimit(cfg.LogPreviewLimit)
-    }
+	// Normalize and apply logging config
+	cfg.ContextMode = strings.ToLower(strings.TrimSpace(cfg.ContextMode))
+	if cfg.LogPreviewLimit >= 0 {
+		logging.SetLogPreviewLimit(cfg.LogPreviewLimit)
+	}
 
-    // Build LLM client if not provided
-    if client == nil {
-        llmCfg := llm.Config{
-            Provider:       cfg.Provider,
-            OpenAIBaseURL:  cfg.OpenAIBaseURL,
-            OpenAIModel:    cfg.OpenAIModel,
-            OllamaBaseURL:  cfg.OllamaBaseURL,
-            OllamaModel:    cfg.OllamaModel,
-            CopilotBaseURL: cfg.CopilotBaseURL,
-            CopilotModel:   cfg.CopilotModel,
-        }
-        oaKey := os.Getenv("OPENAI_API_KEY")
-        cpKey := os.Getenv("COPILOT_API_KEY")
-        if c, err := llm.NewFromConfig(llmCfg, oaKey, cpKey); err != nil {
-            logging.Logf("lsp ", "llm disabled: %v", err)
-        } else {
-            client = c
-            logging.Logf("lsp ", "llm enabled provider=%s model=%s", c.Name(), c.DefaultModel())
-        }
-    }
+	// Build LLM client if not provided
+	if client == nil {
+		llmCfg := llm.Config{
+			Provider:       cfg.Provider,
+			OpenAIBaseURL:  cfg.OpenAIBaseURL,
+			OpenAIModel:    cfg.OpenAIModel,
+			OllamaBaseURL:  cfg.OllamaBaseURL,
+			OllamaModel:    cfg.OllamaModel,
+			CopilotBaseURL: cfg.CopilotBaseURL,
+			CopilotModel:   cfg.CopilotModel,
+		}
+		oaKey := os.Getenv("OPENAI_API_KEY")
+		cpKey := os.Getenv("COPILOT_API_KEY")
+		if c, err := llm.NewFromConfig(llmCfg, oaKey, cpKey); err != nil {
+			logging.Logf("lsp ", "llm disabled: %v", err)
+		} else {
+			client = c
+			logging.Logf("lsp ", "llm enabled provider=%s model=%s", c.Name(), c.DefaultModel())
+		}
+	}
 
-    if factory == nil {
-        factory = func(r io.Reader, w io.Writer, logger *log.Logger, opts lsp.ServerOptions) ServerRunner {
-            return lsp.NewServer(r, w, logger, opts)
-        }
-    }
+	if factory == nil {
+		factory = func(r io.Reader, w io.Writer, logger *log.Logger, opts lsp.ServerOptions) ServerRunner {
+			return lsp.NewServer(r, w, logger, opts)
+		}
+	}
 
-    server := factory(stdin, stdout, logger, lsp.ServerOptions{
-        LogContext:        strings.TrimSpace(logPath) != "",
-        MaxTokens:         cfg.MaxTokens,
-        ContextMode:       cfg.ContextMode,
-        WindowLines:       cfg.ContextWindowLines,
-        MaxContextTokens:  cfg.MaxContextTokens,
-        
-        Client:            client,
-        TriggerCharacters: cfg.TriggerCharacters,
-    })
-    if err := server.Run(); err != nil {
-        logger.Fatalf("server error: %v", err)
-    }
-    return nil
+	server := factory(stdin, stdout, logger, lsp.ServerOptions{
+		LogContext:       strings.TrimSpace(logPath) != "",
+		MaxTokens:        cfg.MaxTokens,
+		ContextMode:      cfg.ContextMode,
+		WindowLines:      cfg.ContextWindowLines,
+		MaxContextTokens: cfg.MaxContextTokens,
+
+		Client:            client,
+		TriggerCharacters: cfg.TriggerCharacters,
+	})
+	if err := server.Run(); err != nil {
+		logger.Fatalf("server error: %v", err)
+	}
+	return nil
 }
diff --git a/internal/hexailsp/run_test.go b/internal/hexailsp/run_test.go
index 7af9cb8..df810e2 100644
--- a/internal/hexailsp/run_test.go
+++ b/internal/hexailsp/run_test.go
@@ -1,145 +1,145 @@
 // Summary: Tests for the Hexai LSP runner using a fake server factory and environment keys.
-// Not yet reviewed by a human
 package hexailsp
 
 import (
-    "bytes"
-    "log"
-    "io"
-    "os"
-    "path/filepath"
-    "testing"
+	"bytes"
+	"io"
+	"log"
+	"os"
+	"path/filepath"
+	"testing"
 
-    "hexai/internal/appconfig"
-    "hexai/internal/llm"
-    "hexai/internal/lsp"
-    "hexai/internal/logging"
+	"hexai/internal/appconfig"
+	"hexai/internal/llm"
+	"hexai/internal/logging"
+	"hexai/internal/lsp"
 )
 
 // fake server capturing options and recording run calls
-type fakeServer struct{
-    ran bool
-    opts lsp.ServerOptions
+type fakeServer struct {
+	ran  bool
+	opts lsp.ServerOptions
 }
+
 func (f *fakeServer) Run() error { f.ran = true; return nil }
 
 func TestRunWithFactory_UsesDefaultsAndCallsServer(t *testing.T) {
-    old := os.Getenv("OPENAI_API_KEY")
-    t.Cleanup(func(){ _ = os.Setenv("OPENAI_API_KEY", old) })
-    _ = os.Setenv("OPENAI_API_KEY", "")
+	old := os.Getenv("OPENAI_API_KEY")
+	t.Cleanup(func() { _ = os.Setenv("OPENAI_API_KEY", old) })
+	_ = os.Setenv("OPENAI_API_KEY", "")
+
+	var stderr bytes.Buffer
+	logger := log.New(&stderr, "hexai-lsp ", 0)
+	cfg := appconfig.Load(nil) // defaults
+	var gotOpts lsp.ServerOptions
+	factory := func(r io.Reader, w io.Writer, logger *log.Logger, opts lsp.ServerOptions) ServerRunner {
+		gotOpts = opts
+		return &fakeServer{opts: opts}
+	}
+	if err := RunWithFactory("", bytes.NewBuffer(nil), bytes.NewBuffer(nil), logger, cfg, nil, factory); err != nil {
+		t.Fatalf("RunWithFactory error: %v", err)
+	}
+	if gotOpts.MaxTokens != cfg.MaxTokens {
+		t.Fatalf("MaxTokens want %d got %d", cfg.MaxTokens, gotOpts.MaxTokens)
+	}
+	if gotOpts.ContextMode != cfg.ContextMode {
+		t.Fatalf("ContextMode want %q got %q", cfg.ContextMode, gotOpts.ContextMode)
+	}
+	if gotOpts.WindowLines != cfg.ContextWindowLines {
+		t.Fatalf("WindowLines want %d got %d", cfg.ContextWindowLines, gotOpts.WindowLines)
+	}
+	if gotOpts.MaxContextTokens != cfg.MaxContextTokens {
+		t.Fatalf("MaxContextTokens want %d got %d", cfg.MaxContextTokens, gotOpts.MaxContextTokens)
+	}
 
-    var stderr bytes.Buffer
-    logger := log.New(&stderr, "hexai-lsp ", 0)
-    cfg := appconfig.Load(nil) // defaults
-    var gotOpts lsp.ServerOptions
-    factory := func(r io.Reader, w io.Writer, logger *log.Logger, opts lsp.ServerOptions) ServerRunner {
-        gotOpts = opts
-        return &fakeServer{opts: opts}
-    }
-    if err := RunWithFactory("", bytes.NewBuffer(nil), bytes.NewBuffer(nil), logger, cfg, nil, factory); err != nil {
-        t.Fatalf("RunWithFactory error: %v", err)
-    }
-    if gotOpts.MaxTokens != cfg.MaxTokens {
-        t.Fatalf("MaxTokens want %d got %d", cfg.MaxTokens, gotOpts.MaxTokens)
-    }
-    if gotOpts.ContextMode != cfg.ContextMode {
-        t.Fatalf("ContextMode want %q got %q", cfg.ContextMode, gotOpts.ContextMode)
-    }
-    if gotOpts.WindowLines != cfg.ContextWindowLines {
-        t.Fatalf("WindowLines want %d got %d", cfg.ContextWindowLines, gotOpts.WindowLines)
-    }
-    if gotOpts.MaxContextTokens != cfg.MaxContextTokens {
-        t.Fatalf("MaxContextTokens want %d got %d", cfg.MaxContextTokens, gotOpts.MaxContextTokens)
-    }
-    
-    if gotOpts.Client != nil { // with no env, openai client fails to build
-        t.Fatalf("expected nil client when API key missing")
-    }
+	if gotOpts.Client != nil { // with no env, openai client fails to build
+		t.Fatalf("expected nil client when API key missing")
+	}
 }
 
 func TestRunWithFactory_BuildsClientWhenKeysPresent(t *testing.T) {
-    // Set a dummy OpenAI key to allow client creation
-    old := os.Getenv("OPENAI_API_KEY")
-    t.Cleanup(func(){ _ = os.Setenv("OPENAI_API_KEY", old) })
-    _ = os.Setenv("OPENAI_API_KEY", "dummy")
+	// Set a dummy OpenAI key to allow client creation
+	old := os.Getenv("OPENAI_API_KEY")
+	t.Cleanup(func() { _ = os.Setenv("OPENAI_API_KEY", old) })
+	_ = os.Setenv("OPENAI_API_KEY", "dummy")
 
-    var stderr bytes.Buffer
-    logger := log.New(&stderr, "hexai-lsp ", 0)
-    cfg := appconfig.Load(nil) // defaults, provider=openai by default
-    var got llm.Client
-    factory := func(r io.Reader, w io.Writer, logger *log.Logger, opts lsp.ServerOptions) ServerRunner {
-        got = opts.Client
-        return &fakeServer{opts: opts}
-    }
-    if err := RunWithFactory("", bytes.NewBuffer(nil), bytes.NewBuffer(nil), logger, cfg, nil, factory); err != nil {
-        t.Fatalf("RunWithFactory error: %v", err)
-    }
-    if got == nil {
-        t.Fatalf("expected non-nil client when OPENAI_API_KEY is set")
-    }
+	var stderr bytes.Buffer
+	logger := log.New(&stderr, "hexai-lsp ", 0)
+	cfg := appconfig.Load(nil) // defaults, provider=openai by default
+	var got llm.Client
+	factory := func(r io.Reader, w io.Writer, logger *log.Logger, opts lsp.ServerOptions) ServerRunner {
+		got = opts.Client
+		return &fakeServer{opts: opts}
+	}
+	if err := RunWithFactory("", bytes.NewBuffer(nil), bytes.NewBuffer(nil), logger, cfg, nil, factory); err != nil {
+		t.Fatalf("RunWithFactory error: %v", err)
+	}
+	if got == nil {
+		t.Fatalf("expected non-nil client when OPENAI_API_KEY is set")
+	}
 }
 
 func TestRun_RespectsLogPathFlag(t *testing.T) {
-    tmp := t.TempDir()
-    logFile := filepath.Join(tmp, "hexai-lsp.log")
-    // Run with real Run but nil env key so client disabled; ensure no panic and file created
-    if err := Run(logFile, bytes.NewBuffer(nil), bytes.NewBuffer(nil), bytes.NewBuffer(nil)); err != nil {
-        t.Fatalf("Run error: %v", err)
-    }
-    if _, err := os.Stat(logFile); err != nil {
-        t.Fatalf("expected log file to be created: %v", err)
-    }
+	tmp := t.TempDir()
+	logFile := filepath.Join(tmp, "hexai-lsp.log")
+	// Run with real Run but nil env key so client disabled; ensure no panic and file created
+	if err := Run(logFile, bytes.NewBuffer(nil), bytes.NewBuffer(nil), bytes.NewBuffer(nil)); err != nil {
+		t.Fatalf("Run error: %v", err)
+	}
+	if _, err := os.Stat(logFile); err != nil {
+		t.Fatalf("expected log file to be created: %v", err)
+	}
 }
 
 func TestRunWithFactory_NormalizesContextMode_AndSetsPreviewLimit(t *testing.T) {
-    t.Cleanup(func(){ logging.SetLogPreviewLimit(0) })
-    var stderr bytes.Buffer
-    logger := log.New(&stderr, "hexai-lsp ", 0)
-    cfg := appconfig.App{
-        ContextMode:     "  File-On-New-Func  ",
-        LogPreviewLimit: 3,
-    }
-    var gotOpts lsp.ServerOptions
-    factory := func(r io.Reader, w io.Writer, logger *log.Logger, opts lsp.ServerOptions) ServerRunner {
-        gotOpts = opts
-        return &fakeServer{opts: opts}
-    }
-    if err := RunWithFactory("", bytes.NewBuffer(nil), bytes.NewBuffer(nil), logger, cfg, nil, factory); err != nil {
-        t.Fatalf("RunWithFactory error: %v", err)
-    }
-    if gotOpts.ContextMode != "file-on-new-func" {
-        t.Fatalf("ContextMode not normalized: %q", gotOpts.ContextMode)
-    }
-    if logging.PreviewForLog("abcdef") != "abc…" {
-        t.Fatalf("PreviewForLog not respecting limit: %q", logging.PreviewForLog("abcdef"))
-    }
+	t.Cleanup(func() { logging.SetLogPreviewLimit(0) })
+	var stderr bytes.Buffer
+	logger := log.New(&stderr, "hexai-lsp ", 0)
+	cfg := appconfig.App{
+		ContextMode:     "  File-On-New-Func  ",
+		LogPreviewLimit: 3,
+	}
+	var gotOpts lsp.ServerOptions
+	factory := func(r io.Reader, w io.Writer, logger *log.Logger, opts lsp.ServerOptions) ServerRunner {
+		gotOpts = opts
+		return &fakeServer{opts: opts}
+	}
+	if err := RunWithFactory("", bytes.NewBuffer(nil), bytes.NewBuffer(nil), logger, cfg, nil, factory); err != nil {
+		t.Fatalf("RunWithFactory error: %v", err)
+	}
+	if gotOpts.ContextMode != "file-on-new-func" {
+		t.Fatalf("ContextMode not normalized: %q", gotOpts.ContextMode)
+	}
+	if logging.PreviewForLog("abcdef") != "abc…" {
+		t.Fatalf("PreviewForLog not respecting limit: %q", logging.PreviewForLog("abcdef"))
+	}
 }
 
 func TestRunWithFactory_LogContextFlag(t *testing.T) {
-    var stderr bytes.Buffer
-    logger := log.New(&stderr, "hexai-lsp ", 0)
-    cfg := appconfig.App{}
-    var got1, got2 lsp.ServerOptions
-    first := true
-    factory := func(r io.Reader, w io.Writer, logger *log.Logger, opts lsp.ServerOptions) ServerRunner {
-        if first {
-            got1 = opts
-            first = false
-        } else {
-            got2 = opts
-        }
-        return &fakeServer{opts: opts}
-    }
-    if err := RunWithFactory("/tmp/some.log", bytes.NewBuffer(nil), bytes.NewBuffer(nil), logger, cfg, nil, factory); err != nil {
-        t.Fatalf("RunWithFactory error: %v", err)
-    }
-    if !got1.LogContext {
-        t.Fatalf("expected LogContext true when logPath is non-empty")
-    }
-    if err := RunWithFactory("", bytes.NewBuffer(nil), bytes.NewBuffer(nil), logger, cfg, nil, factory); err != nil {
-        t.Fatalf("RunWithFactory error: %v", err)
-    }
-    if got2.LogContext {
-        t.Fatalf("expected LogContext false when logPath is empty")
-    }
+	var stderr bytes.Buffer
+	logger := log.New(&stderr, "hexai-lsp ", 0)
+	cfg := appconfig.App{}
+	var got1, got2 lsp.ServerOptions
+	first := true
+	factory := func(r io.Reader, w io.Writer, logger *log.Logger, opts lsp.ServerOptions) ServerRunner {
+		if first {
+			got1 = opts
+			first = false
+		} else {
+			got2 = opts
+		}
+		return &fakeServer{opts: opts}
+	}
+	if err := RunWithFactory("/tmp/some.log", bytes.NewBuffer(nil), bytes.NewBuffer(nil), logger, cfg, nil, factory); err != nil {
+		t.Fatalf("RunWithFactory error: %v", err)
+	}
+	if !got1.LogContext {
+		t.Fatalf("expected LogContext true when logPath is non-empty")
+	}
+	if err := RunWithFactory("", bytes.NewBuffer(nil), bytes.NewBuffer(nil), logger, cfg, nil, factory); err != nil {
+		t.Fatalf("RunWithFactory error: %v", err)
+	}
+	if got2.LogContext {
+		t.Fatalf("expected LogContext false when logPath is empty")
+	}
 }