lsp: add comprehensive unit tests for findFirstInstructionInLine and strict semicolon tag

2 files changed, 148 insertions, 335 deletions

diff --git a/IDEAS.md b/IDEAS.md
deleted file mode 100644
index a455e7e..0000000
--- a/IDEAS.md
+++ /dev/null
@@ -1,53 +0,0 @@
-# Ideas
-
-## Code quality
-
-### Refactor
-
-* [ ] Refactor existing code in a more modular way
-* [ ] Add unit tests
-
-## Features
-
-### Improvements
-
-* [ ] TODO's in the code to be addressed
-
-### New features
-
-* [ ] implement a code action for selected code block the way via a unix pipe as faster access in helix
-* [x] Use hexai as a gh copilot... CLI replacemant for command line questions
-* [ ] Resolve diagnostics code action feature
-* [X] LSP server to be used with the Helix text editor
-* [X] Code completion using LLMs
-* [X] Text completion in general
-* [/] Code generation using LLMs text
-* [ ] Be a replacement for 'github copilot cli'
-* [ ] Be able to perform inline chats (keeping history in the document)
-* [ ] Be able to switch the underlying model via a prompt
-* [ ] Fine tune when Large Language Model (LLM) completions trigger, as it seems that there are some cases where the Large Language Model (LLM) receives a request but Helix isn't suggesting any completions. There seems to be something odd with the in logic. Investigate the TriggerChar logic and make sure it matches Helix's expectations.
-* [ ] Only one code completion should run at a time, even if multiple triggers occur simultaneously
-* [ ] Create "generate unit test" code action for selected code block
-
-Be able to select code blocks and perform code actions on them
-
-* [ ] Commenting exiting code
-* [ ] Code refactoring
-
-Be able to chat with the LLM
-
-* [ ] Have a dialog with the LLM, like in lsp-ai
-
-Be able to switch LLMs. 
-
-* [ ] Ollama local LLM models (e.g. Qwen Coder vs Deepseek-R1 for different purposes)
-* [ ] OpenAI models
-* [ ] Claude models
-* [ ] Gemini models
-
-## More
-
-* [ ] Useful: https://deepwiki.com/helix-editor/helix/4.3-language-server-protocol` 
-
-## Usage notes
-
diff --git a/internal/lsp/handlers_test.go b/internal/lsp/handlers_test.go
index 10b704b..35d0651 100644
--- a/internal/lsp/handlers_test.go
+++ b/internal/lsp/handlers_test.go
@@ -1,310 +1,176 @@
-// Summary: Tests for LSP handlers and request processing, including diagnostics and code actions.
+// Summary: Tests for instruction extraction helpers in handlers.go
 // Not yet reviewed by a human
 package lsp
 
-import (
-	"encoding/json"
-	"strings"
-	"testing"
-)
+import "testing"
 
-func TestInParamList(t *testing.T) {
-    line := "func foo(a int, b string) int {"
-    cases := []struct{
-        name string
-        cursor int
-        want bool
-    }{
-        {"inside-params", 15, true},
-        {"before-func", 2, false},
-        {"after-paren", len(line), false},
-        {"at-open-paren", strings.Index(line, "(")+1, true},
-        {"at-close-paren", strings.Index(line, ")"), true},
-    }
-    for _, tc := range cases {
-        t.Run(tc.name, func(t *testing.T) {
-            got := inParamList(line, tc.cursor)
-            if got != tc.want {
-                t.Fatalf("cursor=%d got %v want %v", tc.cursor, got, tc.want)
-            }
-        })
+func TestFindFirstInstructionInLine_NoMarker(t *testing.T) {
+    line := "fmt.Println(\"hello\")"
+    instr, cleaned, ok := findFirstInstructionInLine(line)
+    if ok {
+        t.Fatalf("expected ok=false; got ok=true with instr=%q cleaned=%q", instr, cleaned)
+    }
+    if instr != "" || cleaned != line {
+        t.Fatalf("unexpected outputs: instr=%q cleaned=%q", instr, cleaned)
     }
 }
 
-func TestComputeWordStart(t *testing.T) {
-	current := "fmt.Prin"
-	// Cursor after the word (index 8)
-	got := computeWordStart(current, 8)
-	// should stop after the dot at index 4
-	if want := 4; got != want {
-		t.Fatalf("computeWordStart got %d want %d", got, want)
-	}
-}
-
-func TestComputeTextEditAndFilter_InParams(t *testing.T) {
-	current := "func foo(a int, b string) {" // ')' at index 26
-	p := CompletionParams{Position: Position{Line: 10, Character: 20}}
-	te, filter := computeTextEditAndFilter("x int, y string", true, current, p)
-
-	if te == nil {
-		t.Fatalf("expected TextEdit")
-	}
-	// left should be after '(' which is at index 8
-	if te.Range.Start.Line != 10 || te.Range.Start.Character != 9 {
-		t.Fatalf("start got line=%d char=%d want line=10 char=9", te.Range.Start.Line, te.Range.Start.Character)
-	}
-	// right should clamp to cursor (20)
-	if te.Range.End.Line != 10 || te.Range.End.Character != 20 {
-		t.Fatalf("end got line=%d char=%d want line=10 char=20", te.Range.End.Line, te.Range.End.Character)
-	}
-	if filter == "" {
-		t.Fatalf("expected non-empty filter inside params")
-	}
-}
-
-func TestComputeTextEditAndFilter_Word(t *testing.T) {
-	current := "fmt.Prin"
-	p := CompletionParams{Position: Position{Line: 2, Character: len(current)}}
-	te, filter := computeTextEditAndFilter("Println", false, current, p)
-	if te == nil {
-		t.Fatalf("expected TextEdit")
-	}
-	if te.Range.Start.Character != 4 || te.Range.End.Character != len(current) {
-		t.Fatalf("range chars got %d..%d want 4..%d", te.Range.Start.Character, te.Range.End.Character, len(current))
-	}
-	if filter != "Prin" {
-		t.Fatalf("filter got %q want %q", filter, "Prin")
-	}
-}
-
-func TestLabelForCompletion(t *testing.T) {
-	if got := labelForCompletion("Println", "Pri"); got != "Println" {
-		t.Fatalf("label mismatch got %q want %q", got, "Println")
-	}
-	if got := labelForCompletion("Println", "X"); got != "X" {
-		t.Fatalf("label mismatch with filter got %q want %q", got, "X")
-	}
-	if got := labelForCompletion("Println\nmore", ""); got != "Println" {
-		t.Fatalf("label firstLine got %q want %q", got, "Println")
-	}
-}
-
-func TestBuildPrompts_InParams(t *testing.T) {
-	p := CompletionParams{TextDocument: TextDocumentIdentifier{URI: "file:///t.go"}, Position: Position{Line: 1, Character: 12}}
-	sys, user := buildPrompts(true, p, "above", "func foo(", "below", "func foo(")
-	if sys == "" || user == "" {
-		t.Fatalf("expected non-empty prompts")
-	}
-	if want := "function signatures"; !contains(sys, want) {
-		t.Fatalf("system prompt missing %q: %q", want, sys)
-	}
-	if want := "parameter list"; !contains(user, want) {
-		t.Fatalf("user prompt missing %q: %q", want, user)
-	}
-}
-
-func TestBuildPrompts_Outside(t *testing.T) {
-	p := CompletionParams{TextDocument: TextDocumentIdentifier{URI: "file:///t.go"}, Position: Position{Line: 1, Character: 5}}
-	sys, user := buildPrompts(false, p, "ab", "cur", "be", "fnctx")
-	if sys == "" || user == "" {
-		t.Fatalf("expected non-empty prompts")
-	}
-	if want := "completion engine"; !contains(sys, want) {
-		t.Fatalf("system prompt missing %q: %q", want, sys)
-	}
-	if want := "Provide the next likely code"; !contains(user, want) {
-		t.Fatalf("user prompt missing %q: %q", want, user)
-	}
-}
-
-func TestComputeTextEditAndFilter_NoParensFallback(t *testing.T) {
-	current := "func foo bar" // no parentheses
-	cursor := len(current)
-	p := CompletionParams{Position: Position{Line: 0, Character: cursor}}
-	te, filter := computeTextEditAndFilter("baz", true, current, p)
-	if te == nil {
-		t.Fatalf("expected TextEdit from fallback path")
-	}
-	// fallback should behave like word edit; start at last space + 1
-	lastSpace := strings.LastIndex(current, " ")
-	if te.Range.Start.Character != lastSpace+1 || te.Range.End.Character != cursor {
-		t.Fatalf("range got %d..%d want %d..%d", te.Range.Start.Character, te.Range.End.Character, lastSpace+1, cursor)
-	}
-	if filter != "bar" {
-		t.Fatalf("filter got %q want %q", filter, "bar")
-	}
-}
-
-// small helper to avoid importing strings
-func contains(s, sub string) bool {
-	return len(s) >= len(sub) && (func() bool {
-		i := 0
-		for i+len(sub) <= len(s) {
-			if s[i:i+len(sub)] == sub {
-				return true
-			}
-			i++
-		}
-		return false
-	})()
+func TestFindFirstInstructionInLine_StrictSemicolon_Basic(t *testing.T) {
+    line := "prefix ;rename var; suffix"
+    instr, cleaned, ok := findFirstInstructionInLine(line)
+    if !ok {
+        t.Fatalf("expected ok=true")
+    }
+    if instr != "rename var" {
+        t.Fatalf("instr got %q want %q", instr, "rename var")
+    }
+    // Removal preserves inner spacing; trailing right spaces trimmed only.
+    if cleaned != "prefix  suffix" {
+        t.Fatalf("cleaned got %q want %q", cleaned, "prefix  suffix")
+    }
 }
 
-func TestCollectPromptRemovalEdits(t *testing.T) {
-	s := newTestServer()
-	uri := "file:///x.go"
-	src := `keep ;tag; this and ;another; that
-no markers here`
-	s.setDocument(uri, src)
-	edits := s.collectPromptRemovalEdits(uri)
-	if len(edits) != 2 {
-		t.Fatalf("expected 2 edits, got %d", len(edits))
-	}
-	// First occurrence ;tag;
-	e0 := edits[0]
-	if e0.Range.Start.Line != 0 {
-		t.Fatalf("e0 start line=%d want 0", e0.Range.Start.Line)
-	}
-	if s.getDocument(uri).lines[0][e0.Range.Start.Character:e0.Range.Start.Character+1] != ";" {
-		t.Fatalf("e0 start not at ;")
-	}
+func TestFindFirstInstructionInLine_StrictSemicolon_TrailingSpacesTrimmed(t *testing.T) {
+    line := "code;fix;   \t\t"
+    instr, cleaned, ok := findFirstInstructionInLine(line)
+    if !ok {
+        t.Fatalf("expected ok=true")
+    }
+    if instr != "fix" {
+        t.Fatalf("instr got %q want %q", instr, "fix")
+    }
+    if cleaned != "code" {
+        t.Fatalf("cleaned got %q want %q", cleaned, "code")
+    }
 }
 
-func TestCollectPromptRemovalEdits_SkipSpacedMarkers(t *testing.T) {
-	s := newTestServer()
-	uri := "file:///y.go"
-	// Only ;ok; should be removed; "; spaced ;" must be ignored
-	src := `prefix ;ok; middle ; spaced ; suffix`
-	s.setDocument(uri, src)
-	edits := s.collectPromptRemovalEdits(uri)
-	if len(edits) != 1 {
-		t.Fatalf("expected 1 edit (only ;ok;), got %d", len(edits))
-	}
-	// Ensure the removed region starts at the first ';' of ;ok;
-	line := s.getDocument(uri).lines[0]
-	wantStart := strings.Index(line, ";ok;")
-	if wantStart < 0 {
-		t.Fatalf("test setup: could not find ;ok; in %q", line)
-	}
-	if edits[0].Range.Start.Line != 0 || edits[0].Range.Start.Character != wantStart {
-		t.Fatalf("unexpected first edit start: got line=%d char=%d want line=0 char=%d", edits[0].Range.Start.Line, edits[0].Range.Start.Character, wantStart)
-	}
+func TestFindFirstInstructionInLine_Semicolon_InvalidPatterns(t *testing.T) {
+    cases := []string{
+        "prefix ; bad; suffix",  // space after first ';' ⇒ invalid
+        "prefix ;bad ; suffix",  // space before closing ';' ⇒ invalid
+        "prefix ; ; suffix",     // empty inner ⇒ invalid
+    }
+    for _, line := range cases {
+        if instr, _, ok := findFirstInstructionInLine(line); ok && instr != "" {
+            t.Fatalf("%q: expected no semicolon instruction; got instr=%q", line, instr)
+        }
+    }
 }
 
-func TestCollectPromptRemovalEdits_DoubleSemicolonRemovesWholeLine(t *testing.T) {
-	s := newTestServer()
-	uri := "file:///z.go"
-	line0 := "keep"
-	line1 := ";;todo; remove this whole line"
-	line2 := "keep ;ok; end"
-	src := strings.Join([]string{line0, line1, line2}, "\n")
-	s.setDocument(uri, src)
-	edits := s.collectPromptRemovalEdits(uri)
-	if len(edits) != 2 {
-		t.Fatalf("expected 2 edits (whole line + ;ok;), got %d", len(edits))
-	}
-	// Find the whole-line removal for line1
-	found := false
-	for _, e := range edits {
-		if e.Range.Start.Line == 1 && e.Range.Start.Character == 0 && e.Range.End.Line == 1 && e.Range.End.Character == len(line1) {
-			found = true
-			break
-		}
-	}
-	if !found {
-		t.Fatalf("did not find whole-line removal edit for line 1")
-	}
+func TestFindFirstInstructionInLine_CBlockComment(t *testing.T) {
+    line := "foo /* update this part */ bar"
+    instr, cleaned, ok := findFirstInstructionInLine(line)
+    if !ok {
+        t.Fatalf("expected ok=true")
+    }
+    if instr != "update this part" {
+        t.Fatalf("instr got %q want %q", instr, "update this part")
+    }
+    if cleaned != "foo  bar" {
+        t.Fatalf("cleaned got %q want %q", cleaned, "foo  bar")
+    }
 }
 
-func TestCollectPromptRemovalEdits_SkipSpacedDouble(t *testing.T) {
-	s := newTestServer()
-	uri := "file:///w.go"
-	src := "prefix ;; spaced ; suffix"
-	s.setDocument(uri, src)
-	edits := s.collectPromptRemovalEdits(uri)
-	if len(edits) != 0 {
-		t.Fatalf("expected 0 edits for spaced double-semicolon trigger, got %d", len(edits))
-	}
+func TestFindFirstInstructionInLine_HTMLComment(t *testing.T) {
+    line := "foo <!--  do x  --> bar"
+    instr, cleaned, ok := findFirstInstructionInLine(line)
+    if !ok {
+        t.Fatalf("expected ok=true")
+    }
+    if instr != "do x" {
+        t.Fatalf("instr got %q want %q", instr, "do x")
+    }
+    if cleaned != "foo  bar" {
+        t.Fatalf("cleaned got %q want %q", cleaned, "foo  bar")
+    }
 }
 
-func TestInstructionFromSelection_OrderPreference(t *testing.T) {
-	// Earliest wins within a line
-	line := "code /*block first*/ // later ;tag;"
-	instr, cleaned := instructionFromSelection(line)
-	if instr != "block first" {
-		t.Fatalf("want block comment instr, got %q", instr)
-	}
-	if strings.Contains(cleaned, "block first") {
-		t.Fatalf("cleaned should not contain the block comment")
-	}
+func TestFindFirstInstructionInLine_SlashSlash(t *testing.T) {
+    line := "val // do this change"
+    instr, cleaned, ok := findFirstInstructionInLine(line)
+    if !ok {
+        t.Fatalf("expected ok=true")
+    }
+    if instr != "do this change" {
+        t.Fatalf("instr got %q want %q", instr, "do this change")
+    }
+    if cleaned != "val" {
+        t.Fatalf("cleaned got %q want %q", cleaned, "val")
+    }
 }
 
-func TestInstructionFromSelection_SemicolonBeatsCommentIfEarlier(t *testing.T) {
-	line := ";do this;// later"
-	instr, cleaned := instructionFromSelection(line)
-	if instr != "do this" {
-		t.Fatalf("want semicolon instr, got %q", instr)
-	}
-	if strings.Contains(cleaned, ";do this;") {
-		t.Fatalf("cleaned should have semicolon tag removed")
-	}
+func TestFindFirstInstructionInLine_Hash(t *testing.T) {
+    line := "val # do this"
+    instr, cleaned, ok := findFirstInstructionInLine(line)
+    if !ok {
+        t.Fatalf("expected ok=true")
+    }
+    if instr != "do this" {
+        t.Fatalf("instr got %q want %q", instr, "do this")
+    }
+    if cleaned != "val" {
+        t.Fatalf("cleaned got %q want %q", cleaned, "val")
+    }
 }
 
-func TestInstructionFromSelection_HTMLAndLineComments(t *testing.T) {
-	line := "prefix <!-- html note --> suffix"
-	instr, cleaned := instructionFromSelection(line)
-	if instr != "html note" {
-		t.Fatalf("want html note, got %q", instr)
-	}
-	if strings.Contains(cleaned, "<!--") || strings.Contains(cleaned, "-->") {
-		t.Fatalf("cleaned should remove html comment markers")
-	}
+func TestFindFirstInstructionInLine_DoubleDash(t *testing.T) {
+    line := "SQL -- fix query"
+    instr, cleaned, ok := findFirstInstructionInLine(line)
+    if !ok {
+        t.Fatalf("expected ok=true")
+    }
+    if instr != "fix query" {
+        t.Fatalf("instr got %q want %q", instr, "fix query")
+    }
+    if cleaned != "SQL" {
+        t.Fatalf("cleaned got %q want %q", cleaned, "SQL")
+    }
 }
 
-func TestStripDuplicateAssignmentPrefix(t *testing.T) {
-	prefix := "matrix := "
-	sug := "matrix := NewMatrix(2,2)"
-	got := stripDuplicateAssignmentPrefix(prefix, sug)
-	if got != "NewMatrix(2,2)" {
-		t.Fatalf("dup strip failed: got %q", got)
-	}
-	// '=' variant
-	prefix2 := "x = "
-	sug2 := "x = y + 1"
-	got2 := stripDuplicateAssignmentPrefix(prefix2, sug2)
-	if got2 != "y + 1" {
-		t.Fatalf("dup strip '=' failed: got %q", got2)
-	}
+func TestFindFirstInstructionInLine_EarliestWins_CommentOverSemicolon(t *testing.T) {
+    line := "aa // comment ;not this; trailing"
+    instr, cleaned, ok := findFirstInstructionInLine(line)
+    if !ok {
+        t.Fatalf("expected ok=true")
+    }
+    if instr != "comment ;not this; trailing" {
+        t.Fatalf("instr got %q want %q", instr, "comment ;not this; trailing")
+    }
+    if cleaned != "aa" {
+        t.Fatalf("cleaned got %q want %q", cleaned, "aa")
+    }
 }
 
-func TestRangesOverlap(t *testing.T) {
-	a := Range{Start: Position{Line: 1, Character: 2}, End: Position{Line: 3, Character: 0}}
-	b := Range{Start: Position{Line: 2, Character: 0}, End: Position{Line: 4, Character: 1}}
-	if !rangesOverlap(a, b) {
-		t.Fatalf("expected overlap")
-	}
-	c := Range{Start: Position{Line: 4, Character: 1}, End: Position{Line: 5, Character: 0}}
-	if rangesOverlap(a, c) {
-		t.Fatalf("expected no overlap")
-	}
+func TestFindFirstInstructionInLine_EarliestWins_SemicolonOverComment(t *testing.T) {
+    line := "aa ;short; // comment"
+    instr, cleaned, ok := findFirstInstructionInLine(line)
+    if !ok {
+        t.Fatalf("expected ok=true")
+    }
+    if instr != "short" {
+        t.Fatalf("instr got %q want %q", instr, "short")
+    }
+    // Only the earliest marker is removed; the later comment remains.
+    if cleaned != "aa  // comment" {
+        t.Fatalf("cleaned got %q want %q", cleaned, "aa  // comment")
+    }
 }
 
-func TestDiagnosticsInRange_Filtering(t *testing.T) {
-	s := newTestServer()
-	sel := Range{Start: Position{Line: 10, Character: 0}, End: Position{Line: 12, Character: 5}}
-	// Build a fake context payload with three diagnostics: one inside, one outside, one touching boundary
-	ctx := CodeActionContext{Diagnostics: []Diagnostic{
-		{Range: Range{Start: Position{Line: 11, Character: 0}, End: Position{Line: 11, Character: 10}}, Message: "inside"},
-		{Range: Range{Start: Position{Line: 2, Character: 0}, End: Position{Line: 3, Character: 0}}, Message: "outside"},
-		{Range: Range{Start: Position{Line: 12, Character: 5}, End: Position{Line: 12, Character: 8}}, Message: "touch"},
-	}}
-	data, _ := json.Marshal(ctx)
-	got := s.diagnosticsInRange(json.RawMessage(data), sel)
-	if len(got) != 2 {
-		t.Fatalf("expected 2 diagnostics in range, got %d", len(got))
-	}
-	msgs := []string{got[0].Message, got[1].Message}
-	joined := strings.Join(msgs, ",")
-	if !strings.Contains(joined, "inside") || !strings.Contains(joined, "touch") {
-		t.Fatalf("unexpected diagnostics: %v", msgs)
-	}
+func TestFindStrictSemicolonTag_Various(t *testing.T) {
+    // basic
+    if text, l, r, ok := findStrictSemicolonTag("pre;do it;post"); !ok || text != "do it" || l != 3 || r != 10 {
+        t.Fatalf("unexpected: ok=%v text=%q l=%d r=%d", ok, text, l, r)
+    }
+    // at start
+    if text, l, r, ok := findStrictSemicolonTag(";x;"); !ok || text != "x" || l != 0 || r != 3 {
+        t.Fatalf("unexpected at start: ok=%v text=%q l=%d r=%d", ok, text, l, r)
+    }
+    // double opening ';' should still allow a tag starting at the second ';'
+    if text, _, _, ok := findStrictSemicolonTag("prefix ;;bad; suffix"); !ok || text != "bad" {
+        t.Fatalf("unexpected double-open handling: ok=%v text=%q", ok, text)
+    }
+    // inner spaces directly after first ';' or before last ';' invalidate the tag
+    if _, _, _, ok := findStrictSemicolonTag("a;  inner  ;b"); ok {
+        t.Fatalf("expected invalid strict tag due to spaces at boundaries")
+    }
 }