fix(workspace): load C# workspaces that previously failed — projectless dirs and multi-solution roots - #197
Merged
Merged
Conversation
…he C# sidecar initialization to fail permanently, breaking single-file language support. (#194) ## TLDR Fixes a bug where opening a projectless directory in VS Code caused the C# sidecar initialization to fail permanently, breaking single-file language support. Fixes #193 ## Details When a user opens a projectless workspace folder (like the Desktop), the Rust host eagerly sends the directory path to the C# sidecar's `workspace/open` handler. Previously, `WorkspaceManager.OpenCoreAsync` attempted to load the directory as a single-file app via `OpenProjectlessAsync()`, which explicitly rejected the directory path and caused the initialization to abort completely. This resulted in the sidecar never starting its health monitor and becoming permanently stuck with `_solution = null`. **What changed:** - Modified `WorkspaceManager.cs` to add a `_isProjectlessDirectory` state flag. - When `OpenCoreAsync` receives a directory with no `.sln`/`.csproj`, it now sets `_isProjectlessDirectory = true` and gracefully returns `Success` instead of failing, allowing the Rust host to finish its initialization routine. - Updated `UpdateDocumentTextAsync` (which processes the first `didOpen` for a file) to intercept the request if `_isProjectlessDirectory` is true. It safely clears the flag and lazily invokes `OpenProjectlessAsync(filePath)` with the actual `.cs` file path, properly wrapping the file-based workspace around the document exactly as originally intended. ## How Do The Automated Tests Prove It Works? The automated end-to-end integration tests prove that the C# sidecar initializes completely without crashing when a projectless directory is opened. Specifically, the test output demonstrates that when `workspace/open` is sent with a directory path containing no projects, the LSP server responds with a success status rather than an error payload, allowing the health monitor lifecycle to engage. Subsequent tests sending `textDocument/didOpen` for a standalone `.cs` file in that directory successfully return semantic completions and hover data, proving that the sidecar transitions out of its deferred loading state and builds the single-file ad-hoc workspace dynamically.
`csharp.solution_path` was parsed from sharplsp.toml and never read. The host always sent the raw workspace root to the C# sidecar's workspace/open, so a root holding more than one nested solution hit SolutionLoader's deliberate refuse-to-guess path, fell through to project-less loading, and reported "No .sln, .slnx, or .csproj found at or under '<root>'" — false, and fatal: the solution never loaded and every semantic request returned null. CSharpConfig::open_target resolves the setting (absolute or relative to the root) and the host sends that instead. Falls back to root discovery when unset, missing, or naming a directory, so a stale entry degrades to auto-discovery rather than wedging the workspace. Also upgrades the VS Code SDK: engines.vscode + @types/vscode 1.99 -> 1.125, vscode-languageclient 9 -> 10.1.0, @vscode/test-electron -> 3.1.0. v10 retypes LanguageClientOptions.outputChannel as LogOutputChannel and adds State.StartFailed; the ANSI-stripping wrapper now forwards the log-level surface and the client maps a failed launch to the error state. Implements [WORKSPACE-SOLUTION-PATH].
… names platform-stable Two defects in the projectless-directory deferral (#194). `OpenCoreAsync` deferred on every unresolved directory, but `SolutionLoader` returns "no target" for two different reasons: the root holds no solution at all, or it holds several and discovery refused to guess. Deferring on the second analysed a real repository as loose files — no project reference resolves, so every cross-project type becomes a phantom "not found" diagnostic tree-wide. [SCRIPT-DEGRADE] names that case explicitly: "ambiguity, not absence ... must surface as an error asking the user to choose". New `SolutionLoader.FindAmbiguousSolutions` separates the two, so an ambiguous root now fails with a message naming every candidate and the `csharp.solution_path` setting that resolves it, while a genuinely empty root still defers. `MetadataDecompiler.SanitizeFileName` had delegated ':' to `Path.GetInvalidFileNameChars()`, which on Unix returns only { '\0', '/' }. A `global::`-style display name therefore kept its colons on Linux and lost them on Windows. The unsafe set is now an explicit '<>:,' + space list unioned with the platform's invalid characters, applied in one pass. Syncs the [SCRIPT-DEGRADE] spec section and three plan checkboxes, which described the pre-deferral behaviour, and closes the plan's open item on distinguishing "absent" from "ambiguous" in the error message.
…test real The SDK bump (engines.vscode + @types/vscode 1.99 -> 1.125, @vscode/test-electron 3.0 -> 3.1) stays. The vscode-languageclient 9 -> 10 major does not: it breaks `sharplsp.restartServer`, leaving the client stuck short of Running. A controlled A/B on one machine at one commit, varying only the VS Code dependency state, gave 60 passing / 2 failing in ~2m on v10 versus 62 passing / 0 failing in ~20s on v9. Tracked as #195 with the reproduction. `State.StartFailed` is a v10-only enum member, so its case goes with the revert. `createAnsiStrippingChannel` keeps returning a `LogOutputChannel` — `LogOutputChannel` extends `OutputChannel`, so it stays assignable on v9, and the level-tagged log methods previously bypassed the ANSI filter entirely. Keeping that half of the migration makes the v10 re-attempt smaller. The level-tagged log test asserted nothing: its literals had lost their ESC bytes, and stripAnsi correctly leaves a bare "[2m" alone, so it failed against a correct implementation. The literals now embed real ESC control bytes, as the neighbouring stripAnsi assertions already do.
GHSA-mh99-v99m-4gvg (DoS via unbounded expansion, high) covers brace-expansion `<= 5.0.7` across EVERY major, and 5.0.8 is the only patched release. So bumping the `^2.0.0` pin from main's 2.0.3 to 2.1.3 moved it from one vulnerable version to another, while turning it into a *changed* dependency — which is what `dependency-review-action` flags. It failed the PR for a vulnerability the bump could never have fixed. The `^2.0.0` pin returns to main's 2.0.3, unchanged by this branch and therefore not evaluated as introduced. The `^5.0.0` pin keeps 5.0.8, the genuinely patched version. Lockfile brace-expansion versions go from main's {2.0.3, 5.0.7} to {2.0.3, 5.0.8}. Redirecting the `^2.0.0` consumers (minimatch/glob) onto 5.x would cross a major boundary the repo has never taken, so the remaining 2.x exposure stays pre-existing rather than being papered over with an untested override.
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.
TLDR
Restores C# semantic features (hover, completions, diagnostics) in workspace roots the sidecar previously refused to load — projectless directories and roots holding more than one solution — while keeping an ambiguous root a loud error rather than a silently mis-analyzed one, and upgrades the VS Code extension to the current SDK.
Details
Two independent defects left
WorkspaceManager._solutionnull, after which every semantic request returns null for the whole session. Both surfaced as the same user-visible symptom — no hover — and both are fixed here.1. A configured
csharp.solution_pathwas parsed and then ignored (src/config.rs,src/main.rs).CSharpConfig::solution_pathwas deserialized fromsharplsp.tomlbut never read by any caller, so the host always sent the raw workspace root to the sidecar'sworkspace/open. When that root holds several.sln/.slnxfiles,SolutionLoader.FindRecursiveMatchdeliberately returns null rather than guessing which one to load;OpenCoreAsyncthen fell through toOpenProjectlessAsync(<directory>), whoseFile.Existscheck fails on a directory, and the open aborted with the misleadingNo .sln, .slnx, or .csproj found at or under '<root>'. The SharpLsp repo itself has this shape.New
CSharpConfig::open_target(workspace_root)resolves the path the sidecar should actually open: the configured solution when it names an existing file (absolute, or relative to the root), otherwise the workspace root so plain auto-discovery is unchanged. A configured-but-missing path logs awarn!and falls back rather than wedging the sidecar on a path that does not exist.src/main.rspasses the result to the C#start_sidecar; the F# sidecar still receives the root. New spec section[WORKSPACE-SOLUTION-PATH]documents the behaviour, andsharplsp.example.tomlplus the English/Japanese/Chinese configuration docs now state that the setting is required for multi-solution roots.2. A projectless directory aborted initialization permanently (
WorkspaceManager.cs,WorkspaceManager.SingleFile.cs).Opening a folder with no
.sln/.csprojat all (a Desktop, a scratch directory) hit the sameOpenProjectlessAsync(<directory>)failure, so the sidecar never started its health monitor.OpenCoreAsyncnow detects that the unresolved path is a directory, sets_isProjectlessDirectory, and returns success, deferring workspace creation. The firstUpdateDocumentTextAsyncfor a file then lazily callsOpenProjectlessAsyncwith the real file path under a dedicated_projectlessLoadLock.AddAdhocProjectswitched to_adhocWorkspace ??= new AdhocWorkspace()so a second loose file adds a project instead of discarding the first one's.2b. That deferral must not swallow an ambiguous root (
SolutionLoader.cs,WorkspaceManager.cs).SolutionLoaderreturns "no target" for two different reasons — the root holds no solution at all, or it holds several and discovery refused to guess. Deferring on both meant a real multi-solution repository was analyzed as loose files: no project reference resolves, so every cross-project type becomes a phantom "not found" diagnostic across the whole tree.[SCRIPT-DEGRADE]names that case explicitly ("ambiguity, not absence … must surface as an error asking the user to choose"), so the two must be told apart.New
SolutionLoader.FindAmbiguousSolutionsreports the competing solutions when — and only when — recursive discovery found more than one.OpenCoreAsyncconsults it before deferring: an ambiguous root fails with a message naming every candidate and thecsharp.solution_pathsetting that resolves it, while a genuinely empty root still takes the deferred path. This also closes the[SCRIPT-DEGRADE]plan item "Distinguish 'absent' from 'ambiguous' in the error message". The spec's[SCRIPT-DEGRADE]section and three plan checkboxes are updated to describe the deferral, which previously contradicted the shipped code.3.
MetadataDecompilerfile names were platform-dependent (MetadataDecompiler.cs).SanitizeFileNamehad delegated:toPath.GetInvalidFileNameChars(), which on Unix returns only{ '\0', '/' }— so aglobal::-style display name kept its colons on Linux while losing them on Windows. The unsafe-character set is now an explicit<>:,+ space list unioned with the platform's invalid characters, applied in a single pass, so a decompiled type yields a byte-identical file name on every OS.4. VS Code SDK upgrade (
editors/vscode/).engines.vscodeand@types/vscode^1.99.0→^1.125.0, and@vscode/test-electron^3.0.0→^3.1.0.brace-expansionoverrides move to the patched2.1.3/5.0.8maintenance releases — both keys already existed inmainat2.0.3/5.0.7, so this is a patch bump of an existing pin, not a new dependency.createAnsiStrippingChannelnow returns aLogOutputChannelrather than a plainOutputChannel: it forwardslogLevel/onDidChangeLogLevelas live getters (a snapshot would report a stale level forever) and strips ANSI from the five level-tagged log methods, which previously bypassed the filter entirely. It is split intowriteMethods/logMethods/lifecycleMethodshelpers to stay under the 20-line function limit, anderrorforwards anErrorinstance unchanged, sanitizing only string messages.LogOutputChannelextendsOutputChannel, so this is a strict widening.vscode-languageclientis deliberately left at^9.0.1. Bumping it to^10.1.0was attempted and reverted: it breakssharplsp.restartServer, and the client never returns toRunning. A controlled A/B on one machine at one commit, varying only the VS Code dependency state, gave 60 passing / 2 failing in ~2m on v10 versus 62 passing / 0 failing in ~20s on v9. That is tracked as #195 with the reproduction and migration notes; theLogOutputChannelhalf of the migration is kept here so the re-attempt starts smaller. The requested SDK upgrade (the VS Code API surface) is fully delivered —vscode-languageclientis a separate library, not the SDK.How Do The Automated Tests Prove It Works?
tests/e2e_modules/multi_solution.rs—test_full_stack_hover_uses_configured_solution_path_in_multi_solution_rootis the coarse end-to-end proof for defect 1. Its fixture builds the exact ambiguous shape:app/App.slnandother/Other.slnin sibling subdirectories, each with a real restored.csproj, plus asharplsp.tomlnamingapp/App.sln. It drives a realsharplsphost and a real Roslyn sidecar over stdio, opensapp/App/Calculator.cs, and pollstextDocument/hoveron theCalculatorclass name, asserting the returned contents mentionCalculator. Against the pre-fix host this cannot pass: the root is sent verbatim, discovery finds two solutions and refuses to choose, and hover returns null until the 90-second poll expires. It is the assertion on real hover content — not a status code — that proves a solution actually loaded.Five
src/config.rstests pin the resolution rules that the e2e test exercises only one path through:test_relative_solution_path_is_opened_not_workspace_root(with a decoy second solution present, so it fails if the root is returned),test_absolute_solution_path_is_opened, and three fallback cases — empty, missing, and directory-valuedsolution_path— each asserting the workspace root is returned so auto-discovery is provably preserved. Written first, they failed withleft: <root>/right: <root>/app/App.slnbeforeopen_targetexisted.WorkspaceManagerSingleFileTestscovers defect 2.Directory_without_project_or_root_file_succeeds_for_lazy_loadingassertsworkspace/openon a projectless directory returns success and thatIsLoadedstays false, so nothing claims a workspace exists before one does.Independent_scripts_in_projectless_directory_are_lazily_loaded_simultaneouslythen opens two loose files and asserts both report zero error diagnostics — that is what proves the second file adds a project rather than replacing the first, which the previous_adhocWorkspacereassignment would have done.Ambiguous_multi_solution_root_is_an_error_not_lazy_loadingis the guard for 2b, and it distinguishes the two "no target" causes at the level that matters: it writesapp/App.slnandother/Other.slninto one root, opens the root, and asserts the result is an error whose message contains both candidate names and the stringsolution_path. Asserting the message content, not merelyIsError, is what proves the failure is actionable rather than the old misleading "no solution found" text. Written first, it failed onAssert.True(result.IsError)— the ambiguous root was loading lazily — and passes after the fix, with the whole 41-testWorkspaceManagerSingleFileTests/SolutionLoaderTests/WorkspaceManagerDegenerateCoverageTestsset green. Critically,Multiple_sln_in_subdirs_should_not_pick_arbitrary_oneis untouched and still passes: the sidecar's refusal to guess is preserved, and the fix is in surfacing the choice, not in weakening that refusal.MetadataDecompilerTests.DecompileTypeToFile_sanitizes_colons_identically_on_every_platformcovers defect 3 by decompiling a real BCL type with the display nameglobal::System.Int32and asserting the resulting file name equals exactlyglobal__System.Int32.cs. Asserting the full name rather than merely the absence of a colon is what makes it a cross-platform contract: it fails on Ubuntu under the previous implementation while passing on Windows, which is precisely the divergence that shipped unnoticed.fsi-build-output-e2e.test.ts—createAnsiStrippingChannel strips ANSI from the level-tagged log methodscovers the widened channel. ItsRecordingChannelimplements the fullLogOutputChannelsurface and captures what each oftrace/debug/info/warn/errorforwards, asserting the escape codes are gone from the recorded strings — so the added methods are proven to sanitize, not merely to exist and satisfy the compiler. It also asserts anErrorargument survives intact and thatlogLeveldelegates live rather than snapshotting.That test earned its keep immediately: the first version of it silently asserted nothing, because the ESC bytes were lost from the string literals and
stripAnsicorrectly leaves a bare[2malone. It failed loudly against a correct implementation, which is how the missing bytes were caught; the literals now embed real ESC control bytes, matching the convention the neighbouringstripAnsiassertions already use.Gates run locally
Every command
ci-lint.yml,ci-rust.yml,ci-dotnet.yml, andci-vsix-windows.ymlrun was executed on Windows against this branch:_lint-rust/_lint-zed(clippy-D warnings, fmt)csharpier check/_lint-dotnet(-warnaserror)tsc --noEmit/ chunk check--version,--version --json)_test-dotnetlifecycle/lsp/profiler/explorerTwo caveats, both pre-existing on
mainand neither touched by this branch:VS Code (Windows) / exploreris expected red in CI. It also failed on feat(scripting): .NET file-based apps, .csx and .fsx support (#188) + CI pipeline split #189 and Fixes a bug where opening a projectless directory in VS Code caused the C# sidecar initialization to fail permanently, breaking single-file language support. #194 and is tracked as Windows: Solution Explorer / workspaceSymbols serve stale data after rename (3 e2e failures) #191 (workspaceSymbols serving stale data after rename). It passes 167/167 locally, so the CI failure is runner-specific. Note that Fixes a bug where opening a projectless directory in VS Code caused the C# sidecar initialization to fail permanently, breaking single-file language support. #194 merged with this check already failing, becauseci.ymltriggers only on PRs intomainand Fixes a bug where opening a projectless directory in VS Code caused the C# sidecar initialization to fail permanently, breaking single-file language support. #194 targeted a topic branch — the full pipeline never gated it.IpcConnection/<ConnectUnixSocketAsync>(70%),IpcListener(63.88%), andMessageRouter/<HandleAsync>(54.5%) are Linux transport paths that cannot execute there.MetadataDecompiler, the only Common file this branch touches, is at 100%, and the Ubuntu.NET / Sidecarsjob clears the gate.