fix: don’t panic when fetching document URI path fails

Christian Rocha created

* fix: remove hardcoded panic when fetching document URI path
* chore: add structured logging to LSP URI conversion errors

Change summary

internal/llm/tools/diagnostics.go           | 18 ++++++++++--
internal/lsp/client.go                      | 13 +++++++-
internal/lsp/protocol/pattern_interfaces.go | 20 ++++++++++++-
internal/lsp/protocol/uri.go                | 25 ++++++++++++-----
internal/lsp/util/edit.go                   | 32 +++++++++++++++++++---
internal/lsp/watcher/watcher.go             | 14 ++++++++-
6 files changed, 101 insertions(+), 21 deletions(-)

Detailed changes

internal/llm/tools/diagnostics.go πŸ”—

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"log/slog"
 	"maps"
 	"sort"
 	"strings"
@@ -118,7 +119,13 @@ func waitForLspDiagnostics(ctx context.Context, filePath string, lsps map[string
 				return
 			}
 
-			if diagParams.URI.Path() == filePath || hasDiagnosticsChanged(client.GetDiagnostics(), originalDiags) {
+			path, err := diagParams.URI.Path()
+			if err != nil {
+				slog.Error("Failed to convert diagnostic URI to path", "uri", diagParams.URI, "error", err)
+				return
+			}
+
+			if path == filePath || hasDiagnosticsChanged(client.GetDiagnostics(), originalDiags) {
 				select {
 				case diagChan <- struct{}{}:
 				default:
@@ -216,10 +223,15 @@ func getDiagnostics(filePath string, lsps map[string]*lsp.Client) string {
 		diagnostics := client.GetDiagnostics()
 		if len(diagnostics) > 0 {
 			for location, diags := range diagnostics {
-				isCurrentFile := location.Path() == filePath
+				path, err := location.Path()
+				if err != nil {
+					slog.Error("Failed to convert diagnostic location URI to path", "uri", location, "error", err)
+					continue
+				}
+				isCurrentFile := path == filePath
 
 				for _, diag := range diags {
-					formattedDiag := formatDiagnostic(location.Path(), diag, lspName)
+					formattedDiag := formatDiagnostic(path, diag, lspName)
 
 					if isCurrentFile {
 						fileDiagnostics = append(fileDiagnostics, formattedDiag)

internal/lsp/client.go πŸ”—

@@ -449,7 +449,12 @@ func (c *Client) pingTypeScriptServer(ctx context.Context) error {
 
 	// If we have any open files, try to get document symbols for one
 	for uri := range c.openFiles {
-		filePath := protocol.DocumentURI(uri).Path()
+		filePath, err := protocol.DocumentURI(uri).Path()
+		if err != nil {
+			slog.Error("Failed to convert URI to path for TypeScript symbol collection", "uri", uri, "error", err)
+			continue
+		}
+
 		if strings.HasSuffix(filePath, ".ts") || strings.HasSuffix(filePath, ".js") ||
 			strings.HasSuffix(filePath, ".tsx") || strings.HasSuffix(filePath, ".jsx") {
 			var symbols []protocol.DocumentSymbol
@@ -712,7 +717,11 @@ func (c *Client) CloseAllFiles(ctx context.Context) {
 	// First collect all URIs that need to be closed
 	for uri := range c.openFiles {
 		// Convert URI back to file path using proper URI handling
-		filePath := protocol.DocumentURI(uri).Path()
+		filePath, err := protocol.DocumentURI(uri).Path()
+		if err != nil {
+			slog.Error("Failed to convert URI to path for file closing", "uri", uri, "error", err)
+			continue
+		}
 		filesToClose = append(filesToClose, filePath)
 	}
 	c.openFilesMu.Unlock()

internal/lsp/protocol/pattern_interfaces.go πŸ”—

@@ -2,6 +2,7 @@ package protocol
 
 import (
 	"fmt"
+	"log/slog"
 )
 
 // PatternInfo is an interface for types that represent glob patterns
@@ -36,21 +37,36 @@ func (g *GlobPattern) AsPattern() (PatternInfo, error) {
 		return nil, fmt.Errorf("nil pattern")
 	}
 
+	var err error
+
 	switch v := g.Value.(type) {
 	case string:
 		return StringPattern{Pattern: v}, nil
+
 	case RelativePattern:
 		// Handle BaseURI which could be string or DocumentUri
 		basePath := ""
 		switch baseURI := v.BaseURI.Value.(type) {
 		case string:
-			basePath = DocumentURI(baseURI).Path()
+			basePath, err = DocumentURI(baseURI).Path()
+			if err != nil {
+				slog.Error("Failed to convert URI to path", "uri", baseURI, "error", err)
+				return nil, fmt.Errorf("invalid URI: %s", baseURI)
+			}
+
 		case DocumentURI:
-			basePath = baseURI.Path()
+			basePath, err = baseURI.Path()
+			if err != nil {
+				slog.Error("Failed to convert DocumentURI to path", "uri", baseURI, "error", err)
+				return nil, fmt.Errorf("invalid DocumentURI: %s", baseURI)
+			}
+
 		default:
 			return nil, fmt.Errorf("unknown BaseURI type: %T", v.BaseURI.Value)
 		}
+
 		return RelativePatternInfo{RP: v, BasePath: basePath}, nil
+
 	default:
 		return nil, fmt.Errorf("unknown pattern type: %T", g.Value)
 	}

internal/lsp/protocol/uri.go πŸ”—

@@ -70,7 +70,7 @@ func (uri *DocumentURI) UnmarshalText(data []byte) (err error) {
 // DocumentUri("").Path() returns the empty string.
 //
 // Path panics if called on a URI that is not a valid filename.
-func (uri DocumentURI) Path() string {
+func (uri DocumentURI) Path() (string, error) {
 	filename, err := filename(uri)
 	if err != nil {
 		// e.g. ParseRequestURI failed.
@@ -79,22 +79,33 @@ func (uri DocumentURI) Path() string {
 		// direct string manipulation; all DocumentUris
 		// received from the client pass through
 		// ParseRequestURI, which ensures validity.
-		panic(err)
+		return "", fmt.Errorf("invalid URI %q: %w", uri, err)
 	}
-	return filepath.FromSlash(filename)
+	return filepath.FromSlash(filename), nil
 }
 
 // Dir returns the URI for the directory containing the receiver.
-func (uri DocumentURI) Dir() DocumentURI {
+func (uri DocumentURI) Dir() (DocumentURI, error) {
+	// XXX: Legacy comment:
 	// This function could be more efficiently implemented by avoiding any call
 	// to Path(), but at least consolidates URI manipulation.
-	return URIFromPath(uri.DirPath())
+
+	path, err := uri.DirPath()
+	if err != nil {
+		return "", fmt.Errorf("invalid URI %q: %w", uri, err)
+	}
+
+	return URIFromPath(path), nil
 }
 
 // DirPath returns the file path to the directory containing this URI, which
 // must be a file URI.
-func (uri DocumentURI) DirPath() string {
-	return filepath.Dir(uri.Path())
+func (uri DocumentURI) DirPath() (string, error) {
+	path, err := uri.Path()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Dir(path), nil
 }
 
 func filename(uri DocumentURI) (string, error) {

internal/lsp/util/edit.go πŸ”—

@@ -11,7 +11,10 @@ import (
 )
 
 func applyTextEdits(uri protocol.DocumentURI, edits []protocol.TextEdit) error {
-	path := uri.Path()
+	path, err := uri.Path()
+	if err != nil {
+		return fmt.Errorf("invalid URI: %w", err)
+	}
 
 	// Read the file content
 	content, err := os.ReadFile(path)
@@ -148,7 +151,11 @@ func applyTextEdit(lines []string, edit protocol.TextEdit) ([]string, error) {
 // applyDocumentChange applies a DocumentChange (create/rename/delete operations)
 func applyDocumentChange(change protocol.DocumentChange) error {
 	if change.CreateFile != nil {
-		path := change.CreateFile.URI.Path()
+		path, err := change.CreateFile.URI.Path()
+		if err != nil {
+			return fmt.Errorf("invalid URI: %w", err)
+		}
+
 		if change.CreateFile.Options != nil {
 			if change.CreateFile.Options.Overwrite {
 				// Proceed with overwrite
@@ -164,7 +171,11 @@ func applyDocumentChange(change protocol.DocumentChange) error {
 	}
 
 	if change.DeleteFile != nil {
-		path := change.DeleteFile.URI.Path()
+		path, err := change.DeleteFile.URI.Path()
+		if err != nil {
+			return fmt.Errorf("invalid URI: %w", err)
+		}
+
 		if change.DeleteFile.Options != nil && change.DeleteFile.Options.Recursive {
 			if err := os.RemoveAll(path); err != nil {
 				return fmt.Errorf("failed to delete directory recursively: %w", err)
@@ -177,8 +188,19 @@ func applyDocumentChange(change protocol.DocumentChange) error {
 	}
 
 	if change.RenameFile != nil {
-		oldPath := change.RenameFile.OldURI.Path()
-		newPath := change.RenameFile.NewURI.Path()
+		var newPath, oldPath string
+		var err error
+
+		oldPath, err = change.RenameFile.OldURI.Path()
+		if err != nil {
+			return err
+		}
+
+		newPath, err = change.RenameFile.NewURI.Path()
+		if err != nil {
+			return err
+		}
+
 		if change.RenameFile.Options != nil {
 			if !change.RenameFile.Options.Overwrite {
 				if _, err := os.Stat(newPath); err == nil {

internal/lsp/watcher/watcher.go πŸ”—

@@ -617,7 +617,11 @@ func (w *WorkspaceWatcher) matchesPattern(path string, pattern protocol.GlobPatt
 		return false
 	}
 	// For relative patterns
-	basePath = protocol.DocumentURI(basePath).Path()
+	if basePath, err = protocol.DocumentURI(basePath).Path(); err != nil {
+		// XXX: Do we want to return here, or send the error up the stack?
+		slog.Error("Error converting base path to URI", "basePath", basePath, "error", err)
+	}
+
 	basePath = filepath.ToSlash(basePath)
 
 	// Make path relative to basePath for matching
@@ -660,7 +664,13 @@ func (w *WorkspaceWatcher) debounceHandleFileEvent(ctx context.Context, uri stri
 // handleFileEvent sends file change notifications
 func (w *WorkspaceWatcher) handleFileEvent(ctx context.Context, uri string, changeType protocol.FileChangeType) {
 	// If the file is open and it's a change event, use didChange notification
-	filePath := protocol.DocumentURI(uri).Path()
+	filePath, err := protocol.DocumentURI(uri).Path()
+	if err != nil {
+		// XXX: Do we want to return here, or send the error up the stack?
+		slog.Error("Error converting URI to path", "uri", uri, "error", err)
+		return
+	}
+
 	if changeType == protocol.FileChangeType(protocol.Deleted) {
 		w.client.ClearDiagnosticsForURI(protocol.DocumentURI(uri))
 	} else if changeType == protocol.FileChangeType(protocol.Changed) && w.client.IsFileOpen(filePath) {