Repository navigation
fix(vs): correct TFM and VSSDK interop DLL path for CI build - net8.0 → net8.0-windows in integration test project so the Microsoft.VisualStudio.Sdk.TestFramework.Xunit 17.11 package resolves its net8.0-windows assets (plain net8.0 fell back to a netstandard2.0 stub that omits VsFact / GlobalServiceProvider). - Fix HintPath for Microsoft.Internal.VisualStudio.Interop.dll in both projects; previous path (tools\vssdk\bin\lib\) does not exist in the VSSDK.BuildTools NuGet package. Correct path for the test project is tools\VSSDK\VisualStudioIntegration\Common\Assemblies\v4.0\ via $(NuGetPackageRoot); for the VSIX project use $(VsSDKInstall) which the already-imported Microsoft.VsSDK.targets sets to that same VisualStudioIntegration folder. https://claude.ai/code/session_01VRbhF1wSDh9FXKajAPUyqw - #12
Conversation
…, restore tests
## Why syntax highlighting / autocomplete broke
SnipperLanguageServerProvider used VS.Extensibility's LanguageServerProvider
(VSEXTPREVIEW_LSP — explicitly a preview API). In VS 17.14 this API routes
language-service requests for **/*.cs through snipper-lsp *instead of*
merging with Roslyn, so Roslyn semantic tokens and IntelliSense disappeared.
## Fixes
**ILanguageClient (SnipperLanguageServerProvider.cs rewritten)**
Replace LanguageServerProvider with the stable VSSDK ILanguageClient API
([ContentType("CSharp")][Export(typeof(ILanguageClient))]). VS merges
snipper-lsp's completions with Roslyn's — Roslyn semantic highlighting is
unaffected. Removes VSEXTPREVIEW_LSP noWarn; drops Nerdbank.Streams and
System.IO.Pipelines from the main project (no longer needed).
**Commands (SnipperCommands.cs + SnipperLspRpc.cs)**
workspace/executeCommand in snipper-lsp is stateless (just looks up a rule
by name — no document context). Commands now open a short-lived per-invocation
StreamJsonRpc connection: initialize → initialized → workspace/executeCommand
→ shutdown → exit. The missing Extensibility.LanguageServer() API was
fabricated in the original scaffold and never existed in any SDK version.
Adds SnipperLspRpc.cs with the extracted RPC helper (testable in isolation).
**TabstopInserter.cs** — restore IVsExpansionView / temp-snippet path
(the buggy commit had replaced it with a plain ReplaceTextOnLine stub).
**Tests**
- SnipperCommandRpcTests: 3 [Fact] tests — fake in-memory LSP server via
FullDuplexStream.CreatePair(); verifies initialize is sent first, correct
body is returned, null is handled cleanly.
- SnipperLanguageClientTests: 4 [Fact] tests — ActivateAsync null-path,
StartAsync event firing, no-subscriber guard, OnServerInitializeFailed.
- TabstopInserterTests restored with GlobalServiceProvider + [VsFact]
and VS SDK 17.14 interface signatures (TextSelMode, uint dwFlags).
- LspSnippetConverterVsTests / SnipperBinaryLocatorTests /
SnipperOptionsPageTests restored from pre-breakage commit.
- IntegrationTests.csproj: re-adds Sdk.TestFramework 17.11, adds
StreamJsonRpc + Nerdbank.Streams with runtime assets, links new files.
https://claude.ai/code/session_01VRbhF1wSDh9FXKajAPUyqw
Three independent issues caused both vs-integration-tests and vsix-package-vs to fail: 1. Microsoft.VisualStudio.Sdk.TestFramework.Xunit 17.11+ ships net8.0 assets only — the net472 test project couldn't resolve the Xunit sub-namespace. Fix: change TargetFramework net472 → net8.0 and drop the net472-only binding-redirect settings. 2. Microsoft.Internal.VisualStudio.Interop (IXMLDOMNode, IVsExpansionView, IVsXMLMemberIndexService, etc.) was not auto-referenced in either project. Fix: add explicit <Reference> with HintPath to both the VSIX project and the test project; add Microsoft.VSSDK.BuildTools to the test project so the DLL is restored to the NuGet cache before the HintPath is evaluated. 3. PlainTextViewStub was sealed, preventing ExpansionViewStub from deriving from it. Fix: remove the sealed modifier. Also adds SnipperPackage.cs to the test project's linked sources so that SnipperLanguageClient (SnipperLanguageServerProvider.cs) compiles. https://claude.ai/code/session_01VRbhF1wSDh9FXKajAPUyqw
- net8.0 → net8.0-windows in integration test project so the Microsoft.VisualStudio.Sdk.TestFramework.Xunit 17.11 package resolves its net8.0-windows assets (plain net8.0 fell back to a netstandard2.0 stub that omits VsFact / GlobalServiceProvider). - Fix HintPath for Microsoft.Internal.VisualStudio.Interop.dll in both projects; previous path (tools\vssdk\bin\lib\) does not exist in the VSSDK.BuildTools NuGet package. Correct path for the test project is tools\VSSDK\VisualStudioIntegration\Common\Assemblies\v4.0\ via $(NuGetPackageRoot); for the VSIX project use $(VsSDKInstall) which the already-imported Microsoft.VsSDK.targets sets to that same VisualStudioIntegration folder. https://claude.ai/code/session_01VRbhF1wSDh9FXKajAPUyqw
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 133cf9bc66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var handler = new HeaderDelimitedMessageHandler( | ||
| serverOutput, serverInput, new JsonMessageFormatter()); |
There was a problem hiding this comment.
When a VS command runs against the real snipper-lsp process, these streams are separate: StandardInput is the stream the client must write to and StandardOutput is the stream it must read from. HeaderDelimitedMessageHandler(Stream?, Stream?, ...) treats the first stream as the sending stream and the second as the receiving stream, so the current call tries to write requests to StandardOutput and read responses from StandardInput, preventing the per-command RPC path from completing. The in-memory tests pass the same duplex stream for both parameters, which masks this; swap the arguments here (or pass input as sending, output as receiving).
Useful? React with 👍 / 👎.
HeaderDelimitedMessageHandler(sending, receiving, formatter) — the first argument is the stream the client writes to (serverInput / StandardInput) and the second is the stream the client reads from (serverOutput / StandardOutput). The previous order sent requests to StandardOutput and read responses from StandardInput, so no per-command RPC call could ever complete against a real snipper-lsp process. https://claude.ai/code/session_01VRbhF1wSDh9FXKajAPUyqw
All three tests were passing the same FullDuplexStream for both the serverOutput and serverInput parameters of ExecuteCommandAsync, which meant swapping the HeaderDelimitedMessageHandler arguments had no observable effect — the stream-swap bug was completely masked. Replace with two System.IO.Pipelines.Pipe objects (client→server and server→client) so that read and write halves are physically separate streams. With directed pipes, passing the wrong stream as the sending or receiving end immediately breaks the JSON-RPC handshake, making the tests a genuine regression guard for the argument order. Nerdbank.Streams is no longer referenced and is removed from the csproj. https://claude.ai/code/session_01VRbhF1wSDh9FXKajAPUyqw
|
The @codex, whatcha say now? |
AssessmentThe review feedback appears to be addressed already on the current PR branch. The tests also now use two separate unidirectional pipes rather than a single duplex stream, specifically to catch swapped stream arguments instead of masking them. The helper names the streams so the client reads from I did not make code changes, commit, or open a new PR because the requested fix is already present in the checked-out branch. Reference checked: StreamJsonRpc’s Testing / Checks
|
No description provided.