[Daman Mulye (damanm24)]

This change consumes the following collateral from the Wsl-deps repository:

• Microsoft.WSL.OpenVMM nuget package which contains:
• OpenVMM
• wslopenvmm.dll and wslopenvmm.h (a Rust based dll that provides an RPC client to communicate with OpenVMM and corresponding header file definitions)

[Copilot]

🟡 Changes recommended

Unresolved moderate findings remain in build gating, toolchain validation, import-library delivery, and VHDX suffix handling.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds an optional Rust C-ABI DLL for OpenVMM VM RPC communication over AF_UNIX, including lifecycle/resource operations, cancellation, recovery, diagnostics, and tests.

Changes:

• Adds the Rust client, C ABI, RPC transport, and VM operations.
• Integrates conditional Rust builds and test execution.
• Updates dependencies, packaging metadata, and build configuration.

File summaries

File	Reviewed changes and final comments
tools/test/test.bat.in	Runs Rust tests during the normal test pass.
src/windows/wslopenvmm/wslopenvmm.h	Defines the C ABI. Moderate (1 vote): The dllimport declarations require an import library that is not published.
src/windows/wslopenvmm/src/tests.rs	Adds DLL and RPC mock tests.
src/windows/wslopenvmm/src/rpc.rs	Handles deadlines, cancellation, and HRESULT mapping.
src/windows/wslopenvmm/src/lib.rs	Exports the DLL API. Nit (1 vote): Exported resource-control methods lack ABI-level fixture tests.
src/windows/wslopenvmm/src/diagnostics.rs	Adds diagnostic tracing.
src/windows/wslopenvmm/src/client.rs	Implements VM configuration and operations. Moderate (3 votes): VHDX suffix matching is case-sensitive. Nit (1 vote): New mutation entry points lack focused tests.
src/windows/wslopenvmm/src/af_unix.rs	Implements AF_UNIX transport.
src/windows/wslopenvmm/CMakeLists.txt	Builds the Rust DLL. Moderate (1 vote): Only the DLL is staged; the required import library is not exposed.
src/windows/wslopenvmm/Cargo.toml	Defines Rust dependencies and packaging.
src/windows/wslopenvmm/Cargo.lock	Locks Rust dependencies.
src/windows/wslopenvmm/build.rs	Generates protobuf bindings.
packages.config	Updates the DeviceHost package version.
msixinstaller/CMakeLists.txt	Refactors installer dependencies.
CMakeLists.txt	Gates OpenVMM builds. Moderate (2 votes): DEFINED enables the target even when INCLUDE_OPENVMM=OFF; toolchain validation does not verify the selected target or Rust edition support.
.gitignore	Nit (1 vote): Does not ignore the configured cargo-target output directory.

Review details

Suppressed comments (4)

src/windows/wslopenvmm/CMakeLists.txt:26

• The C header marks every entry point as __declspec(dllimport), so a normal C/C++ consumer also needs the MSVC import library. This command copies only the DLL into the advertised ${BIN} output and exposes no import-library output/target; either publish the generated import library alongside it or make the interface explicitly support dynamic loading instead.

    COMMAND ${CMAKE_COMMAND} -E copy_if_different ${CARGO_OUTPUT} ${WSLOPENVMM_DLL}

src/windows/wslopenvmm/src/client.rs:388

• The new VM mutation entry points beginning here are not exercised by the Rust suite, even though the mock service defines expectations for ModifyResource, AddVpciDevice, and RemoveVpciDevice. Request shapes, share bookkeeping, and recovery behavior can therefore regress without a test; add focused tests for these exported operations.

    pub fn attach_scsi_disk(
        &self,
        controller: u32,
        lun: u32,
        host_path: String,
        read_only: bool,
    ) -> HRESULT {

src/windows/wslopenvmm/src/lib.rs:281

• The new Rust tests exercise configuration construction and Resume/Teardown/Quit, but none of the exported resource-control methods below are invoked through the ABI. modify_disk, modify_port, and share bookkeeping can therefore regress while cargo test remains green; add fixture-backed tests for these operations, including request fields and success/failure state updates.

#[unsafe(no_mangle)]
pub unsafe extern "C" fn WslOpenVmmVmAttachScsiDisk(

src/windows/wslopenvmm/wslopenvmm.h:17

• __declspec(dllimport) makes these declarations require an MSVC import library for normal C/C++ linking, but the CMake rule only copies wslopenvmm.dll and does not publish Cargo's .dll.lib or a CMake target for it. A consumer using this header cannot link against the normal build output; either stage/expose the import library or make the header explicitly support a dynamically loaded API.

__declspec(dllimport) HRESULT WslOpenVmmCreateConfig(_Out_ WslOpenVmmConfig** Config);

• Files reviewed: 14/16 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved build integration, linking, schema-path, test coverage, and pipeline test execution issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

src/windows/wslopenvmm/src/client.rs:387

• The exported VM control surface introduced here is not exercised by the new RPC tests: only CreateVm/ResumeVm/TeardownVm/Quit are sent, while disk attach/detach, port bind/unbind, and share add/remove request construction and bookkeeping are untested. Add mock-server assertions for these methods, including failure/recovery paths, so an incorrect ModifyType/address or stale share state cannot ship unnoticed.
  tools/test/test.bat.in:9
• This only adds the Rust command to the developer test.bat. The normal pipeline test path invokes TAEF directly from cloudtest/TestGroup.xml.in and stages only the native test artifacts, so these Rust unit tests are never run by the normal WSL test pass described by the PR. Wire cargo test into the pipeline test job or stage and invoke it there as well.

CMakeLists.txt:600

• Cargo presence is the only gate here, so any normal build on a machine with Cargo unconditionally adds this ALL target. There is no OpenVMM opt-in variable, which contradicts the PR's stated build behavior and adds Rust compilation to ordinary WSL builds. Gate this target and the injected test command behind an explicit OpenVMM build option.

if (CARGO_EXECUTABLE)
    add_subdirectory(src/windows/wslopenvmm)
endif()

src/windows/wslopenvmm/CMakeLists.txt:31

• Because this is an ALL target and the only guard is Cargo being installed, every ordinary WSL build on a Rust-equipped machine now compiles the full tonic/protoc dependency graph, even when OpenVMM support was not requested. This contradicts the stated opt-in build behavior and adds a substantial build cost; gate the subdirectory/target behind an explicit OpenVMM option or remove ALL and build it explicitly.

add_custom_target(wslopenvmm ALL DEPENDS ${WSLOPENVMM_DLL})

• Files reviewed: 14/16 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

CMake requires Rust unconditionally, and the Rust tests lack the tokio-stream sync feature.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 14/16 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The Rust tests do not parse, CI omits the DLL target, a dependency feature is missing, coverage is incomplete, and locale edits must be reverted.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/windows/wslopenvmm/CMakeLists.txt:28

• This target is only declared with ALL, but the repository's CI invokes CMake with an explicit target list (.pipelines/build-job.yml:132-134) that does not include wslopenvmm. The x64/ARM64 build jobs therefore skip compiling this DLL, so CI can pass without validating the new library. Add the target to the pipeline target list (and any required artifact wiring), or make an existing CI target depend on it.

add_custom_target(wslopenvmm ALL DEPENDS ${WSLOPENVMM_DLL} ${WSLOPENVMM_IMPORT_LIB})

src/windows/wslopenvmm/Cargo.toml:29

• ReceiverStream is used by the test server in src/tests.rs, but tokio-stream exposes that wrapper behind its sync feature; enabling only net makes the Rust test target fail to compile. Add the sync feature or use a wrapper that is covered by the enabled features.

tokio-stream = { version = "0.1", features = ["net"] }

• Files reviewed: 20/22 changed files
• Comments generated: 7
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical CMake integration, GUID serialization, and test-fixture failures block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

tools/test/test.bat.in:9

• This only augments the locally generated test.bat; the normal CloudTest job does not stage or invoke that script. .pipelines/build-job.yml stages the TAEF artifacts and cloudtest/TestGroup.xml.in executes wsltests.dll directly, so these Rust tests will not run in CI as claimed. Add the Cargo invocation to the CloudTest execution path (or stage and run this script there).

src/windows/wslopenvmm/src/client.rs:409

• The new VM mutation surface is not covered by the integration fixtures: no test invokes Add/RemoveShare, Attach/DetachScsiDisk, or Bind/UnbindPort, even though MockVmService has request variants for these RPCs. Request encoding, local share bookkeeping, and recovery behavior can therefore regress without detection. Add success and uncertain/failure cases for these methods.

    pub fn add_share(&self, tag: String, host_path: String, read_only: bool) -> HRESULT {
        self.with_operation("AddShare", false, |inner, deadline, cancellation| {
            if inner.shares.contains_key(&tag) {
                return HRESULT::from_win32(ERROR_ALREADY_EXISTS.0);
            }

src/windows/wslopenvmm/src/client.rs:598

• Path::extension() returns None for a path with a trailing separator, so the test case C:\disks\archive.vhdx\ (and callers using that spelling) is classified as VHD1 instead of VHDX. Trim trailing separators or otherwise inspect the final filename component before checking the extension.

    if Path::new(path)
        .extension()
        .is_some_and(|extension| extension.eq_ignore_ascii_case("vhdx"))

• Files reviewed: 15/17 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Packaging/staging conflicts remain, and Rust test execution is not integrated into the normal test flow.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 6/7 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved packaging and client-integration issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

msipackage/CMakeLists.txt:20

• Release builds set PACKAGE_INPUT_DIR to packageStagingDir, but the build-stage target patterns do not copy openvmm.exe or wslopenvmm.dll there (see .pipelines/build-stage.yml:35-40). Consequently the new ${PACKAGE_INPUT_DIR} dependencies and WiX sources are missing when msipackage is built in a release pipeline; add these files to the staging/signing pattern or source them directly from the NuGet package.

set(WINDOWS_BINARIES wsl.exe;wslg.exe;wslhost.exe;wslrelay.exe;wslservice.exe;wslserviceproxystub.dll;wsldevicehostproxystub.dll;wslinstall.dll;wslc.exe;wslcsession.exe;openvmm.exe;wslopenvmm.dll)

msipackage/CMakeLists.txt:20

• Adding wslopenvmm.dll to WINDOWS_BINARIES makes it a required MSI input and causes the package target to depend on ${PACKAGE_INPUT_DIR}/wslopenvmm.dll, which contradicts the stated requirement that this DLL is not included in installer payloads. Remove it from the MSI input list when removing the Wix file entry; otherwise normal MSI packaging will also fail whenever the staging directory intentionally omits it.

set(WINDOWS_BINARIES wsl.exe;wslg.exe;wslhost.exe;wslrelay.exe;wslservice.exe;wslserviceproxystub.dll;wsldevicehostproxystub.dll;wslinstall.dll;wslc.exe;wslcsession.exe;openvmm.exe;wslopenvmm.dll)

msipackage/package.wix.in:298

• The PR description says the Rust DLL is not included in MSI/MSIX payloads, but this new WiX entry adds wslopenvmm.dll to the MSI. The CMake packaging list also stages it, so the shipped installer contradicts the stated packaging boundary; either remove the DLL from the packaging inputs or update the requirement.

                <File Id="wslopenvmm.dll" Source="${PACKAGE_INPUT_DIR}/wslopenvmm.dll" />

• Files reviewed: 6/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Four moderate findings remain in build, packaging, and dependency configuration.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

CMakeLists.txt:284

• The imported target is never referenced anywhere else in the tree (wslopenvmm_client only appears in this definition), so configuring CMake does not link the new client DLL into WSLService or any other WSL component. Wire this target to the intended consumer and add the corresponding call site, or defer adding the imported target until a consumer exists.

add_library(wslopenvmm_client SHARED IMPORTED GLOBAL)

msipackage/CMakeLists.txt:20

• Release builds set PACKAGE_INPUT_DIR to packageStagingDir, which is populated only from the target patterns in .pipelines/build-stage.yml; those patterns do not include either newly added binary. As a result, these new BINARIES_DEPENDENCIES paths are missing when WiX runs and the release msipackage target fails. Add/sign/stage both files in the release target pattern, or do not make them MSI inputs.

set(WINDOWS_BINARIES wsl.exe;wslg.exe;wslhost.exe;wslrelay.exe;wslservice.exe;wslserviceproxystub.dll;wsldevicehostproxystub.dll;wslinstall.dll;wslc.exe;wslcsession.exe;openvmm.exe;wslopenvmm.dll)

msipackage/package.wix.in:298

• This WiX entry puts wslopenvmm.dll in the MSI payload, contradicting the PR description that the DLL is not included in MSI or MSIX installers. Remove this file from the WiX component and from the corresponding WINDOWS_BINARIES/dependency list so the package does not ship it.

                <File Id="wslopenvmm.dll" Source="${PACKAGE_INPUT_DIR}/wslopenvmm.dll" />

• Files reviewed: 6/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Blue (OneBlue)]

LGTM, minor comments. Feel free to fix in a followup

[Blue (OneBlue)]

Do we need to create a target for this library ? I was originally thinking of just adding the include path & updating the linking options in wslservice / wslcsession