From 76c95de579f1412bfdf74261f95524e14179b350 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Fri, 25 Jul 2025 13:40:50 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20don=E2=80=99t=20panic=20when=20fetching?= =?UTF-8?q?=20document=20URI=20path=20fails?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: remove hardcoded panic when fetching document URI path * chore: add structured logging to LSP URI conversion errors --- 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(-) diff --git a/internal/llm/tools/diagnostics.go b/internal/llm/tools/diagnostics.go index 6d2da798a6547b11f861e8e5051d297809cbb9af..fc9bd211735afd4d1f8a536a90d5705d88bd9790 100644 --- a/internal/llm/tools/diagnostics.go +++ b/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) diff --git a/internal/lsp/client.go b/internal/lsp/client.go index f7298a0d90240c020d451ace1548bef699d1b36d..219a5df5fb87197f0490f218cddc24ab3b138371 100644 --- a/internal/lsp/client.go +++ b/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() diff --git a/internal/lsp/protocol/pattern_interfaces.go b/internal/lsp/protocol/pattern_interfaces.go index 9ee48cb4565cef6bc9168467aaa7e4fdc21a36a8..5cb5dbb84ea385d96ac33fa2075d6590872da3cd 100644 --- a/internal/lsp/protocol/pattern_interfaces.go +++ b/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) } diff --git a/internal/lsp/protocol/uri.go b/internal/lsp/protocol/uri.go index a63ed406cc319c2f293406fe4cdd83c237c0c74a..ccc45f23e46b3ea41ac28c525eca6c39c201695e 100644 --- a/internal/lsp/protocol/uri.go +++ b/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) { diff --git a/internal/lsp/util/edit.go b/internal/lsp/util/edit.go index b32eac0c66cffe7d50066a3e5db3e81332e4fa83..12d8e428a7214338bd7ef66c6d71dd512484b243 100644 --- a/internal/lsp/util/edit.go +++ b/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 { diff --git a/internal/lsp/watcher/watcher.go b/internal/lsp/watcher/watcher.go index 080870937bee98be852979748dab456fa6a53b66..58b6d41decd1f551c4a474a6921e075b54e75a6e 100644 --- a/internal/lsp/watcher/watcher.go +++ b/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) {