From dc8f0ab28276fac6aa16c0cf3591c322367036b7 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Thu, 11 Jun 2026 08:37:31 +0300 Subject: Fix LSP panic: bounds-check stale line index in async chat apply When handleChatPrompt detects an in-editor chat prompt it captures the line index, then spawns a goroutine (requestChatResponse) that later calls applyChatEdits/buildChatHistory. If a concurrent didChange shrinks the document in the meantime, the captured lineIdx can exceed len(d.lines), causing an index-out-of-range panic in the chat goroutine. - applyChatEdits: skip the stale edit (and log) when lineIdx is < 0 or >= len(d.lines), rather than indexing d.lines[lineIdx] and panicking (or corrupting the already-changed document at the wrong position). - buildChatHistory: clamp the starting index to len(d.lines)-1 so the upward walk over d.lines[i] cannot read past the end. Adds regression test TestChatEdits_StaleLineIndexAfterShrink covering the shrink-then-apply race for both functions, including the one-past-the-end boundary and a negative index. Confirmed the test panics without the fix and passes with it; full `mage test` (-race) green. Co-Authored-By: Claude Opus 4.8 --- internal/lsp/chat_handlers.go | 20 ++++++++++++++++++ internal/lsp/chat_history_test.go | 43 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/internal/lsp/chat_handlers.go b/internal/lsp/chat_handlers.go index 6fffaca..320be45 100644 --- a/internal/lsp/chat_handlers.go +++ b/internal/lsp/chat_handlers.go @@ -171,6 +171,17 @@ func (c *chatService) applyChatEdits(uri string, lineIdx int, lastNonSpace int, if d == nil { return } + // Guard against a stale line index. The chat response is produced + // asynchronously: handleChatPrompt detects the trigger line, then a goroutine + // calls requestChatResponse -> applyChatEdits. In the meantime a didChange + // notification may have shrunk the document, so lineIdx can now point past the + // end of d.lines. Indexing d.lines[lineIdx] in that case panics with + // index-out-of-range. We bail out (skip the stale edit) rather than risk + // corrupting the (already-changed) document at the wrong position. + if lineIdx < 0 || lineIdx >= len(d.lines) { + logging.Logf("lsp ", "chat skip stale edit: lineIdx=%d len=%d", lineIdx, len(d.lines)) + return + } // 1) Delete the trailing punctuation (1 or 2 chars) delStart := Position{Line: lineIdx, Character: lastNonSpace + 1 - removeCount} delEnd := Position{Line: lineIdx, Character: lastNonSpace + 1} @@ -238,7 +249,16 @@ func (c *chatService) buildChatHistory(uri string, lineIdx int, currentPrompt st } type pair struct{ q, a string } pairs := []pair{} + // Clamp the starting index to the current document bounds. lineIdx is derived + // from a position captured when the chat prompt was detected, but the chat + // response is applied asynchronously (see applyChatEdits): a concurrent + // didChange may have shrunk the document so that lineIdx-1 now exceeds + // len(d.lines)-1. Without clamping, the d.lines[i] accesses below would panic + // with index-out-of-range. We start from the last valid line instead. i := lineIdx - 1 + if i >= len(d.lines) { + i = len(d.lines) - 1 + } for i >= 0 && len(pairs) < 3 { for i >= 0 && strings.TrimSpace(d.lines[i]) == "" { i-- diff --git a/internal/lsp/chat_history_test.go b/internal/lsp/chat_history_test.go index a6d6266..00d00ec 100644 --- a/internal/lsp/chat_history_test.go +++ b/internal/lsp/chat_history_test.go @@ -1,6 +1,9 @@ package lsp -import "testing" +import ( + "io" + "testing" +) func TestStripTrailingTrigger(t *testing.T) { s := newTestServer() @@ -36,3 +39,41 @@ func TestBuildChatHistory_OrderAndLimit(t *testing.T) { t.Fatalf("unexpected contents: %+v", msgs) } } + +// TestChatEdits_StaleLineIndexAfterShrink is a regression test for the LSP +// server panic that occurred when an async chat response was applied after a +// concurrent didChange shrank the document. The chat prompt was detected at a +// line index that is now past the end of the (smaller) document, so the stale +// lineIdx exceeds len(d.lines). Before the fix, both applyChatEdits and +// buildChatHistory indexed d.lines without an upper-bound check and panicked +// with index-out-of-range. The fix makes applyChatEdits skip the stale edit and +// buildChatHistory clamp its starting index, so neither must panic. +func TestChatEdits_StaleLineIndexAfterShrink(t *testing.T) { + s := newTestServer() + // io.Discard avoids a nil-writer panic on the (valid-index) clientApplyEdit + // path; the stale-index path bails out before reaching it. + s.out = io.Discard + uri := "file:///shrink.txt" + // Originally the prompt sat on line 8, but the document has since shrunk to + // just three lines. lineIdx=8 is now well past len(d.lines). + s.setDocument(uri, "line0\nline1\nline2\n") + staleLineIdx := 8 + + // applyChatEdits must not panic on a stale (out-of-range) line index. + s.chatSvc().applyChatEdits(uri, staleLineIdx, 12, 1, "> reply") + + // buildChatHistory must not panic and must still return the current prompt. + msgs := s.chatSvc().buildChatHistory(uri, staleLineIdx, "current?") + if len(msgs) == 0 || msgs[len(msgs)-1].Content != "current?" { + t.Fatalf("expected history to end with current prompt, got: %+v", msgs) + } + + // Boundary case: lineIdx exactly equals len(d.lines) (one past the last line). + s.chatSvc().applyChatEdits(uri, 3, 4, 1, "> reply") + if msgs := s.chatSvc().buildChatHistory(uri, 3, "q"); len(msgs) == 0 { + t.Fatalf("expected non-empty history at boundary index") + } + + // Negative index must also be handled gracefully. + s.chatSvc().applyChatEdits(uri, -1, 0, 1, "> reply") +} -- cgit v1.2.3