Integrate the WinUI 3 XAML language service into the winapp extension#50
Draft
nmetulev wants to merge 9 commits into
Draft
Integrate the WinUI 3 XAML language service into the winapp extension#50nmetulev wants to merge 9 commits into
nmetulev wants to merge 9 commits into
Conversation
Folds the hardened XAML language service prototype (nmetulev/winui-vsc) into the shipping winapp VS Code extension as a single unified extension. Additive and non-breaking to the existing winapp CLI / debug / manifest features. Server: copy the .NET 10 LSP server (WinUiXaml.LanguageServer/Workspace/Xaml + 3 xUnit test projects + WinUiXaml.slnx + lsp-smoke) under server/. Builds 0/0; 489/489 unit tests pass. Client: add src/xaml/xamlLanguageService.ts (activateXaml/deactivateXaml, resolveServerDll bundled->dev fallback) wired additively from src/extension.ts; degrades to syntax-only on server-start failure. Contribute the xaml language, TextMate grammar, language-configuration, winui-xaml.showInfo|restartServer commands, winui-xaml.server.* config, onLanguage:xaml activation, and the vscode-languageclient ^9 dependency in package.json. Tests: port the 81-file integration harness to test/xaml-integration/ (kept as source of truth) with a clean committed WinAppSDK fixture at test/fixtures/xaml/fixture/ and a root .vscode-test.mjs. Full suite: 1596 passing / 28 pending / 0 failing. smoke.mjs and features.test.js (218) green. Packaging/CI: esbuild + package-vsc.ps1 publish the server into dist/server (bundled in the VSIX); .vscodeignore excludes C# sources/tests; release-vsc.yml adds UseDotNet@2, a server publish step, and a conditional ESRP DLL-signing step for our own WinUiXaml.*.dll. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c713cc-41ae-4c28-ab49-3c851aa8e2bd
Automated fix->review loop iteration 1 — resolves all 17 findings from the post-migration PR review (2 High, 9 Medium, 6 Low): - H1 security: machine-scope winui-xaml.server.path/dotnetPath + workspace-trust gate (isTrusted + onDidGrantWorkspaceTrust) + capabilities.untrustedWorkspaces - H2: deterministic fixture NuGet restore (restore:fixture in pretest) - M1: run XAML/server/smoke suites in CI with .NET 10 (setup-dotnet) - M2: document .NET-dependent suites; unit tests stay dotnet-free - M3/M4: README Features + Requirements for the XAML language service - M5: serialize client start/stop lifecycle (no torn-down pending start) - M6: handle workspace/didChangeWatchedFiles (invalidate stale Roslyn cache) - M7: user-facing "degraded to syntax-only" notification with actions - M8: client command/lifecycle/degradation integration tests - M9: fold server bundling into vscode:prepublish (ensure-server-bundle.mjs) - L1: share bundled-asset path resolver (firstExistingPath) - L2: tighten .vscodeignore + VSIX-contents assertion - L3/L4: README command table + CONTRIBUTING .NET 10 SDK prereq - L5/L6: docs/RELEASE.md signing checklist + server/nuget.config feed pin Verified: tsc clean, eslint 0 errors, server build 0/0, 489/489 server tests, smoke PASS, integration 1601 passing / 0 failing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c713cc-41ae-4c28-ab49-3c851aa8e2bd
Automated fix->review loop iteration 2 — resolves the 5 residual/new findings the independent GPT-5.6 review raised on iteration 1: - N1 (CRITICAL security): do not start the semantic server while the workspace is untrusted (Roslyn/MSBuild would evaluate an attacker-controlled .csproj and could execute code). Stay syntax-only until trust is granted; test host runs with --disable-workspace-trust so the feature suite still exercises the server. - N2 (correctness): replace the partial start-guard with a single lifecycle queue (runExclusive) so stop fully completes before any start — two servers can never coexist across concurrent restarts. - N3 (correctness): null-guard workspace/didChangeWatchedFiles (changes:null). - N4 (test-coverage): WINUI_XAML_FORCE_NO_SERVER test seam + a real absent-DLL degradation test (syntax-only, no throw). - N5 (packaging): ensure-server-bundle publishes fresh by default; reuse only under WINUI_REUSE_SIGNED_SERVER=1 (set by CI -SkipServerBuild) to preserve signed DLLs, so a bare vsce package can never ship a stale server. Verified: tsc clean, eslint 0 errors, server build 0/0, 489/489 server tests, smoke PASS, integration 1602 passing / 0 failing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c713cc-41ae-4c28-ab49-3c851aa8e2bd
…(P1-P3) P1 (M5 residual): guard against server resurrection when restart races deactivate. Add module-level `disposing` flag set synchronously at the top of deactivateXaml and reset in activateXaml; doStart early-returns when disposing. Replace the separate stop+start in the restartServer command and onDidGrantWorkspaceTrust with a single restartClient() that runs stop-then-start inside one runExclusive op (atomic; skips start if disposing). P2: rewrite package.json untrustedWorkspaces capability description to match actual behavior (syntax highlighting only until the workspace is trusted). P3 (M9 residual): package-vsc.ps1 -SkipServerBuild set WINUI_REUSE_SIGNED_SERVER into the caller's environment and never cleaned it up. Capture the prior value before the try and restore/remove it in the finally block. Full integration suite: 1602 passing / 0 failing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c713cc-41ae-4c28-ab49-3c851aa8e2bd
…packaging) Final 8-dimension PR-review gate over the whole branch surfaced 10 findings the per-iteration reviews (scoped to their own residuals) never looked at. Fix all. G1 (HIGH security): the isTrusted gate is necessary but not sufficient — an empty window is trusted, so opening a loose XAML file outside any workspace folder made the server warm up and MSBuild-evaluate its nearest .csproj (code-exec vector). Client now passes workspaceFolders as initializationOptions.allowedRoots; the server (ResolveAllowedRoots/IsPathUnderAllowedRoot/ResolveIfAllowedAsync) only runs project discovery + OpenProjectAsync for documents under a trusted root. Empty allow-list => no project evaluation; out-of-root/untitled files are served project-less (tolerant parser only). Boundary-aware, case-insensitive path check. allowedRoots is authoritative when present; legacy clients fall back to rootUri. G3 (HIGH test-coverage): add WINUI_XAML_FORCE_UNTRUSTED seam (mirrors WINUI_XAML_FORCE_NO_SERVER) so the trust gate + onDidGrantWorkspaceTrust recovery are exercised under the trusted harness. New client tests cover degrade+recover. G4 (docs): document winui-xaml.server.path in README. G5 (correctness): retain the synchronize.fileEvents FileSystemWatcher and dispose it in doStop / failed-start catch (was leaking one per start/restart/trust cycle). G6 (UX): untrusted degradation now shows a once-per-transition warning with a Manage Workspace Trust action (was log-only). G7 (test-coverage): extract pure buildDegradedNotification() + unit test message and action mapping. G8 (test-coverage): client tests for explicit server.path (valid -> semantic completion; invalid -> syntax-only, no throw). G9 (test-coverage): lsp-smoke sends didChangeWatchedFiles with omitted/null/empty changes and asserts the server stays responsive. G10 (packaging): drop the redundant bundle:server in package-vsc.ps1 (vscode: prepublish/ensure-server-bundle.mjs is the single publish path). G2 (packaging): narrow .vscodeignore to exclude only the apphost exe so Roslyn's net472 MSBuild BuildHost.exe ships; assert a BuildHost is present in the VSIX. Full integration 1605 passing / 0 failing; server 489, unit 595, smoke green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c713cc-41ae-4c28-ab49-3c851aa8e2bd
…indings) The final 8-dimension confirmation gate (incl. a cross-model check) found 5 issues the per-iteration reviews never scoped. Fix all 5. G15 (HIGH security): the allow-list was purely lexical — Path.GetFullPath does not resolve reparse points, so a junction/symlink placed inside a trusted workspace root that targets an external directory passed the containment check, and the resolver (FindOwningProject -> MSBuild GetOrLoadAsync) then evaluated the external .csproj. Fix: CanonicalizePath resolves the final physical path (GetFinalPathNameByHandle) for both the workspace roots (NormalizeRoots) and the document (IsPathUnderAllowedRoot) before containment; falls back to the lexical full path for non-existent/untitled docs or on any OS-call failure. A junction escaping the root now canonicalizes outside it and is served project-less. G11 (MEDIUM correctness): a workspace opened at a bare drive root (C:\) rejected all its children — NormalizeRoots keeps C:\ with its trailing separator, but PathIsWithin required path[root.Length] to be a separator (it was the first path-segment letter). Fix: when the root already ends in a separator (bare drive/UNC root) the prefix match is itself the boundary. The boundary-char check still guards C:\root vs C:\rootEvil. G12 (HIGH test-coverage): the security-critical boundary had no direct test. Made NormalizeRoots/PathIsWithin/CanonicalizePath internal (test project already has InternalsVisibleTo) and added AllowedRootBoundaryTests (exact/child/out-of-root/ sibling-prefix/drive-root/cross-drive/case/trailing-sep/normalize-trim + a non-admin mklink /J junction regression). Added a behavioral lsp-smoke case: a document under os.tmpdir (outside the sole allowedRoot) gets no project-backed resolution while the in-root fixture still resolves. G13 (MEDIUM UX): the degraded-notification guard was a single boolean reset only on a successful start, so after a user granted trust but the server still could not start (e.g. no dotnet) the server-missing guidance was suppressed. Fix: track the last degraded cause and notify once per distinct cause (untrusted vs server), still non-nagging; reset on successful start. G14 (LOW docs): README used the abbreviated server.dotnetPath; use the full winui-xaml.server.dotnetPath. Full integration 1605 passing / 0 failing; server 505, unit 595, smoke green; server debug build 0 warnings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c713cc-41ae-4c28-ab49-3c851aa8e2bd
…trust boundary
reviewer-iter5 (independent cross-model) found two HIGH residuals in the iter5
allow-list hardening. Both are closed here.
G15 (security) — the canonicalizer only resolved reparse points for an EXISTING
leaf (CreateFileW/OPEN_EXISTING). A not-yet-created XAML leaf under an in-root
junction (e.g. C:\trusted\link\Page.xaml, no file) failed that call and fell back
to the lexical path, which still passed lexical containment. XamlProjectResolver
.FindOwningProject only needs the DIRECTORY (new FileInfo(path).Directory +
dir.GetFiles("*.csproj")), so the external junction target's .csproj was still
MSBuild-evaluated — a code-exec bypass. CanonicalizePath now walks up to the
deepest EXISTING ancestor (which follows any junction/symlink in the ancestry),
resolves it via GetFinalPathNameByHandle, and re-appends the missing tail; lexical
fallback is used only when no ancestor exists or the OS call fails. Legit
not-yet-saved files under a real (non-junction) dir still resolve in-root, so
IntelliSense for new files is preserved.
G12 (test-coverage) — the junction regression test never created the linked file,
so it exercised the same missing-leaf fallback, and its Assert.False threw inside a
broad catch(Exception) that swallowed the failure: a green test over a live hole.
The test now creates the linked file and asserts BOTH the existing-leaf and the
missing-leaf paths canonicalize OUTSIDE the trusted root, with the assertions moved
out of the swallowing catch (it skips only on genuine junction-creation failure).
The out-of-root lsp-smoke case gains a sibling .csproj + code-behind so the null F12
result is attributable to the trust gate rather than to an absent project.
Verification: build:server:debug 0 warnings/0 errors; test:server 505/505 (junction
test genuinely exercised — junctions confirmed creatable in this env);
test:xaml-smoke PASS incl. the strengthened out-of-root case; full integration
1605 passing / 0 failing / 28 pending.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 03c713cc-41ae-4c28-ab49-3c851aa8e2bd
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- test:xaml-smoke now runs restore:fixture before the smoke harness so element completion has WinUI SDK metadata in CI. The fixture's obj/ is gitignored and was previously only restored by the integration pretest, so the standalone smoke step failed in CI with 0 completion items. This unblocks build-and-test and the packaged VSIX artifact. - redteam-gap4.test.js: use replaceAll for the caret-marker strip (CodeQL "incomplete string escaping or encoding"). - redteam19.test.js: drop the no-op self-replacement (CodeQL "replacement of a substring with itself"). - pr-description.yml: tolerate the REST diff API's 20k-line cap (HTTP 406) on very large PRs instead of failing; fall back to the changed-file list. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c713cc-41ae-4c28-ab49-3c851aa8e2bd
CodeQL's full scan of the migrated test files (features.test.js, smoke.mjs)
flagged sanitization patterns in test helpers. All fixes are behavior
preserving (smoke PASS; integration 1605 passing / 0 failing):
- caret-marker strip: .replace("|", "") -> .replaceAll("|", "")
(js/incomplete-sanitization).
- exact completion-label membership: array.includes(<uri>) ->
array.some((l) => l === <uri>) — these were array-membership checks that
CodeQL misread as URL substring checks (js/incomplete-url-substring-sanitization).
- diagnostic display string: drop the nested template literal in favor of
string concatenation (js/bad-code-sanitization).
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 03c713cc-41ae-4c28-ab49-3c851aa8e2bd
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
2 participants
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.
Description
Integrates the WinUI 3 XAML language service into the shipping
winappVS Code extension, bringing the WinUI developer experience in VS Code closer to parity with Visual Studio. This is folded into the existing extension as a single unified extension (not a second extension) and is purely additive — the existing winapp CLI commands,winappdebug provider, and AppxManifest custom editor are untouched.What lands:
src/xaml/) wired into the existingactivate()/deactivate(); activates ononLanguage:xaml, launches the .NET LSP server over stdio, resolves the server DLL in priority order (setting → env var → bundled → repo build), and registerswinui-xaml.showInfo/winui-xaml.restartServer.server/— 3 projects:WinUiXaml.LanguageServer,WinUiXaml.Workspace,WinUiXaml.Xaml) providing completion, hover, F12/definition, diagnostics, references, rename, code actions, formatting, semantic tokens, folding, links, etc. Backed by RoslynMSBuildWorkspace+ a tolerant XAML parser.xamllanguage, TextMate grammar, andlanguage-configuration.json(isolated under a subfolder), plus config props and thevscode-languageclientruntime dependency..vscodeignorehandling.lsp-smokeharness, and the Mocha/@vscode/test-electronintegration harness driving real LSP features against a committed WinAppSDK fixture.Usage Example
Opening any
.xamlfile in a trusted workspace activates the language service:Commands: WinUI XAML: Show Info (
winui-xaml.showInfo), WinUI XAML: Restart Server (winui-xaml.restartServer).Related Issue
N/A
Type of Change
Checklist
.\scripts\build-vsce.ps1locally — see Additional Notes for the exact build/test commands that were runpackage.jsoncontribution points / command/debugger/custom-editor docs updated (languages, grammars, commands, config props,onLanguage:xamlactivation)Additional Notes
Why it is a draft. Opening for review of the integration approach and, in particular, the workspace-trust security boundary (see below). A few decisions are still open and called out below.
Verification run locally (Windows, .NET 10 SDK):
build:server:debug— 0 warnings / 0 errorstest:server— 505 passing / 0 failingtest:unit— 595 passing / 0 failingtest:xaml-smoke— green (raw-stdio LSP smoke, incl. an out-of-root boundary case)npm test) — 1605 passing / 0 failing / 28 pendingReview history. This branch went through 6 automated fix→review iterations with an independent cross-model reviewer (a different model family from the author) plus two full multi-dimension review gates. The reviews drove convergence to zero findings at every severity. The final independent review is clean.
Security — workspace-trust boundary (please review closely). The server only MSBuild-evaluates
.csprojprojects that live under a trusted workspace-folder root (the client passes the roots asinitializationOptions.allowedRoots). Candidate paths are canonicalized with reparse points (junctions/symlinks) resolved before the containment check, including for not-yet-created files, so a link inside a trusted root cannot escape to evaluate an external project. In an untrusted or empty window, XAML gets syntax highlighting only (no project evaluation) with a "Manage Workspace Trust" prompt; full IntelliSense activates once trust is granted. This boundary took several review rounds to harden and is the area most worth a careful look.PR size. ~199 files / +51.7k lines is dominated by the vendored .NET server source and its test suites; the extension-side (TypeScript) change is small and additive.
Open decisions / follow-ups (not blocking this draft):
scripts/download-cli.ps1), and reflecting the server build/sign/publish steps in.pipelines/for CI + code signing.dotnet(current behavior, configurable viawinui-xaml.server.dotnetPath) vs. ship a self-contained server.vs-extension/experiment were intentionally left out of scope (phase 2); they depend on native surface bits and do not migrate cleanly.AI Description
This section is auto-generated by AI when the PR is opened or updated. To opt out, delete this entire section including the marker comments.