Guard against null workspaceFolders on initialize#2308
Open
andyleejordan wants to merge 3 commits into
Open
Conversation
When a client sends `workspaceFolders` on `initialize` and one of the entries has no `uri`, the `OnInitialize` handler threw a `NullReferenceException` while resolving the initial working directory (`workspaceService.WorkspaceFolders.FirstOrDefault()?.Uri.GetFileSystemPath()`). The `?.` only guarded the folder being null, not its `Uri`, so calling `GetFileSystemPath()` on a null `Uri` blew up, the `initialize` response was never returned, and the server was unusable for that client. The same hazard exists in every other place we dereference `folder.Uri` (`WorkspacePaths`, `GetRelativePath`, `FindFileInWorkspace`). The issue (#2300) reports this as Linux-only with a repro that includes a valid `uri`; that exact payload doesn't actually throw (I reproduced both ways by driving a built PSES over stdio). The real trigger is a workspace folder lacking a URI, which is what the captured stack trace (`PsesLanguageServer.cs:line 150`) points at. - Add `WorkspaceService.AddWorkspaceFolders`, which owns the invariant that every folder in `WorkspaceFolders` has a non-null `Uri`. It skips null folders and folders without a URI (logging a warning) and treats a null collection as "no folders yet", falling back to the existing single-root and CWD behavior. - Call it from `OnInitialize` instead of the inline null-check plus `AddRange`. - Add a regression test covering null input and uriless/null folders. Fixes #2300. Drafted by Copilot (Claude Opus 4.8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extract the null/URI-less filtering from `AddWorkspaceFolders` into a pure `GetValidWorkspaceFolders` helper. `WorkspaceService` remains the single chokepoint where client-supplied folders are ingested, so every downstream `Uri` dereference stays protected, but the filtering logic is now trivially unit-testable without constructing a service or logger. Drop the per-folder warning so the filter is a simple, allocation-free expression, and add direct tests covering null, empty, and mixed (null folder / URI-less folder / valid folder) inputs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #2300 where a NullReferenceException is thrown in the OnInitialize handler when a client sends workspaceFolders containing an entry without a valid Uri. The fix centralizes workspace folder validation into a new method that filters out null folders and those lacking a URI before adding them to the service's collection.
Changes:
- Introduces
WorkspaceService.AddWorkspaceFoldersand a staticGetValidWorkspaceFoldersfilter that guard against null/URI-less workspace folders. - Replaces the inline null-check +
AddRangecall inPsesLanguageServer.OnInitializewith the new guardedAddWorkspaceFoldersmethod. - Adds regression tests covering null collections, null folder entries, and folders missing URIs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/PowerShellEditorServices/Services/Workspace/WorkspaceService.cs |
Adds AddWorkspaceFolders (public) and GetValidWorkspaceFolders (internal static) to validate and filter workspace folders before adding them. |
src/PowerShellEditorServices/Server/PsesLanguageServer.cs |
Replaces the previous inline null-check + AddRange with a call to the new AddWorkspaceFolders method. |
test/PowerShellEditorServices.Test/Session/WorkspaceTests.cs |
Adds four unit tests covering the new filtering behavior for null inputs, empty collections, and folders without URIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Restore the per-folder warning that the previous refactor dropped. Silently discarding malformed workspace folders makes it hard for server operators to diagnose why a client's folder didn't take effect, and it matches the behavior the PR description advertises. Logging each skipped folder requires the explicit loop, so revert the pure `GetValidWorkspaceFolders` helper (and its dedicated tests); the existing `AddWorkspaceFoldersIgnoresNullAndUrilessFolders` regression test still covers null input, URI-less/null folders, and the downstream dereference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
@SeeminglyScience @JustinGrote bots solving bots' issues, but it's a real bug (from a user) and a real fix (a null check) with tests. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2300.
Problem
When a client sends
workspaceFoldersoninitializeand one of the entries has nouri, theOnInitializehandler threw aNullReferenceExceptionwhile resolving the initial working directory:The
?.only guarded the folder being null, not itsUri, so calling the instance methodGetFileSystemPath()on a nullUrithrew. Theinitializeresponse was never returned and the server was unusable for that client. The same hazard exists everywhere else we dereferencefolder.Uri(WorkspacePaths,GetRelativePath,FindFileInWorkspace).Investigation note
The issue describes this as Linux-only with a repro whose
workspaceFoldersentry has a validuri. I drove a locally-built PSES over stdio and that exact payload does not throw. The real trigger is a workspace folder lacking a URI, which matches the captured stack trace pointing atPsesLanguageServer.cs:line 150(theHostStartOptionsinitializer). Reproduced the NRE with a uriless folder and confirmed it's gone after this change.Fix
WorkspaceService.AddWorkspaceFolders, which owns the invariant that every folder inWorkspaceFoldershas a non-nullUri. It skips null folders and folders without a URI (logging a warning), and treats a null collection as "no folders yet", falling back to the existing single-root / CWD behavior.OnInitializeinstead of the inline null-check +AddRange.Validation
dotnet buildclean.Category=Workspacetests pass on net8.0 (7/7).workspaceFoldersentry: before, NRE inOnInitializeand noinitializeresponse; after, the handshake completes normally.🤖 Drafted by Copilot (Claude Opus 4.8) — please review/edit before merging.