Skip to content

Use Arm64-aware wording when WSL reports virtualization off - #594

Merged
kmahone merged 3 commits into
openclaw:masterfrom
RBrid:user/rbrid/VirtualizationOffMessageOnArm2
Jun 5, 2026
Merged

Use Arm64-aware wording when WSL reports virtualization off#594
kmahone merged 3 commits into
openclaw:masterfrom
RBrid:user/rbrid/VirtualizationOffMessageOnArm2

Conversation

@RBrid

@RBrid RBrid commented May 30, 2026

Copy link
Copy Markdown
Contributor

Fixes #593.

Problem

On ARM64 devices (e.g. Surface), when wsl.exe reports virtualization is off, the SetupEngine preflight-wsl step surfaces x86-only firmware terminology — "VT-x/AMD-V (Intel VT or AMD SVM) in your computer's BIOS/UEFI" — which doesn't apply on ARM64 (no VT-x, no AMD-V, no SVM; UEFI has its own generic "Virtualization Support" label).

Fix

Split TryGetEnvironmentIssue into:

  • A public 1-arg method (existing API surface preserved) that delegates using RuntimeInformation.OSArchitecture.
  • A new internal 2-arg overload taking System.Runtime.InteropServices.Architecture, exposed to tests via the existing InternalsVisibleTo.

The Arm64 branch emits UEFI- and Intune-aware wording; all other architectures keep the existing x64 wording.

RuntimeInformation.OSArchitecture reflects OS architecture (not process), so this works correctly even when SetupEngine.UI runs as win-x64 under Prism on ARM64 Windows.

Tests

  • Existing DetectsFirmwareVirtualizationOff test pinned to Architecture.X64 (asserts BIOS + VT-x wording).
  • New UsesArm64WordingOnArm64 test exercises Architecture.Arm64 and asserts the message contains "ARM64" + "UEFI" + "virtualization" and does not contain "VT-x", "AMD-V", or "SVM".
  • Integration test PreflightWsl_FailsTerminalWhenVirtualizationDisabledInFirmware relaxed to assert "virtualization" (architecture-agnostic) instead of "BIOS".

Validation: ./build.ps1 ✅ · SetupEngine tests 233/0 ✅ · Shared 2045/29 skipped ✅ · Tray 936/0 ✅.

Locale dependency

This PR does not introduce any new dependency on English wsl.exe output. The guard condition ("virtualization is not enabled" substring match) is unchanged from master — only the remediation message string is branched on CPU architecture. If wsl.exe emits English here, both the old code and the new code fire the branch; if wsl.exe emits a localized string, neither fires (same as today).

Known limitation (out of scope for this PR — needs follow-up)

Per @shanselman's comment on #593: the detection itself is a literal English substring match ("virtualization is not enabled") against wsl.exe output. The existing code comment in SetupSteps.cs already flags this risk and prefers HRESULT matching where available (e.g. the sibling 0x80370102 branch is locale-stable), but no HRESULT is currently used for this firmware-off case.

On a non-English Windows install, wsl.exe is likely to emit localized text and this branch won't fire at all — users would fall through to the generic failure path with no remediation hint. This PR does not change that behavior; it only fixes the wording when the branch does fire.

Open questions worth confirming with @craigloewen-msft or Ben Hillis:

  1. Is this specific diagnostic string actually localized by wsl.exe, or hardcoded English?
  2. If localized, is there a stable HRESULT we could anchor on instead?
  3. If yes, a follow-up PR can drop the English match in favour of the HRESULT.

Review

Adversarial dual-model review (Claude Opus 4.6 + GPT-5.2 Codex) — both converged on "ship it"; no actionable findings. Critical question resolved: RuntimeInformation.OSArchitecture returns Arm64 under Prism (it reflects the machine, not the process).

The SetupEngine preflight-wsl step previously surfaced x86-only firmware
terminology (VT-x/AMD-V, BIOS/UEFI) when wsl.exe reported
'virtualization is not enabled'. On ARM64 devices (e.g. Surface) those
terms don't apply, leaving the remediation message confusing.

Split TryGetEnvironmentIssue into a public 1-arg method that delegates
to a new internal overload taking System.Runtime.InteropServices.
Architecture. The Arm64 branch emits UEFI-/Intune-aware wording;
other architectures keep the existing x64 wording. RuntimeInformation.
OSArchitecture reflects OS arch (not process arch), so this works even
when the app runs as win-x64 under Prism on ARM64 Windows.

Fixes openclaw#593

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@clawsweeper

clawsweeper Bot commented May 30, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed June 5, 2026, 2:24 PM ET / 18:24 UTC.

Summary
The PR makes WSL virtualization-off remediation architecture-aware by adding an internal Architecture overload, preserving existing non-Arm64 wording, and adding focused SetupEngine tests.

Reproducibility: yes. from source and linked issue evidence: current master always emits x86-specific VT-x/AMD-V wording for the matched WSL virtualization-off output, and the linked ARM64 report shows that wording surfacing in setup. I did not run a live Windows ARM64 setup flow in this read-only Linux checkout.

Review metrics: 1 noteworthy metric.

  • Patch surface: 2 files changed, +45/-8. The review scope is limited to one SetupEngine diagnostic helper and focused tests.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦞 diamond lobster
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted after-fix ARM64 setup proof showing the ARM64/UEFI remediation and absence of VT-x, AMD-V, and SVM wording.
  • Update the PR body after adding proof so ClawSweeper re-reviews automatically; if it does not, ask a maintainer to comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: No after-fix real ARM64 setup screenshot, terminal output, setup log, recording, or redacted runtime evidence is attached; private details should be redacted before proof is added to the PR body for re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A visible setup dialog, terminal, or setup-log proof on ARM64 would materially improve review confidence, and no transport-specific Mantis lane applies. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

visual task: verify on ARM64 Windows that preflight-wsl virtualization-off setup failure shows ARM64/UEFI wording and does not mention VT-x, AMD-V, or SVM.

Risk before merge

  • [P1] No after-fix real ARM64 setup screenshot, terminal output, setup log, or recording is attached, so reviewers cannot yet verify the exact dialog or log text on an affected ARM64 Windows setup.
  • [P2] The English WSL substring match remains a pre-existing localization/HRESULT follow-up; this PR does not worsen it, but it should not be treated as fixing localized WSL output.

Maintainer options:

  1. Decide the mitigation before merge
    Land the focused wording and regression-test change after redacted ARM64 setup proof confirms the remediation no longer names VT-x, AMD-V, or SVM, and track localized WSL-output detection separately.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The remaining blocker is contributor or maintainer real ARM64 behavior proof plus normal review; there is no narrow code repair for ClawSweeper to make on the branch.

Security
Cleared: The diff only changes local diagnostic wording and tests; it does not touch credentials, dependencies, CI, downloads, permissions, or code execution paths.

Review details

Best possible solution:

Land the focused wording and regression-test change after redacted ARM64 setup proof confirms the remediation no longer names VT-x, AMD-V, or SVM, and track localized WSL-output detection separately.

Do we have a high-confidence way to reproduce the issue?

Yes, from source and linked issue evidence: current master always emits x86-specific VT-x/AMD-V wording for the matched WSL virtualization-off output, and the linked ARM64 report shows that wording surfacing in setup. I did not run a live Windows ARM64 setup flow in this read-only Linux checkout.

Is this the best way to solve the issue?

Yes, the architecture-aware message split is a narrow maintainable fix that preserves the existing public helper and non-Arm64 wording while adding direct tests for the Arm64 branch. The localized WSL-output/HRESULT question is better handled as a follow-up because this PR does not change detection semantics.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against afa6218338d6.

Label changes

Label justifications:

  • P3: This is a low-risk setup UX wording fix for an ARM64-specific remediation message, not a crash, security, or core workflow outage.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦞 diamond lobster.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: No after-fix real ARM64 setup screenshot, terminal output, setup log, recording, or redacted runtime evidence is attached; private details should be redacted before proof is added to the PR body for re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

Acceptance criteria:

  • [P1] ./build.ps1.
  • [P1] dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore.
  • [P1] dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore.

What I checked:

Likely related people:

  • kmahone: Commit 9106725 introduced the WSL environment diagnostic branch and the current x86-specific remediation text in the affected source and tests. (role: introduced behavior; confidence: high; commits: 9106725567af; files: src/OpenClaw.SetupEngine/SetupSteps.cs, tests/OpenClaw.SetupEngine.Tests/SetupStepsTests.cs)
  • ranjeshj: Recent SetupEngine history includes adjacent WSL setup and gateway-preflight changes in the same source and test files. (role: recent area contributor; confidence: medium; commits: 3ddade7072b0; files: src/OpenClaw.SetupEngine/SetupSteps.cs, tests/OpenClaw.SetupEngine.Tests/SetupStepsTests.cs)
  • shanselman: The linked discussion raises the WSL localization/HRESULT question for this diagnostic path, and local history shows adjacent setup-engine work under Scott Hanselman. (role: adjacent reviewer context; confidence: medium; commits: 528f625edc5b; files: src/OpenClaw.SetupEngine/SetupSteps.cs)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels May 30, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels May 30, 2026
@RBrid
RBrid marked this pull request as ready for review May 30, 2026 00:45
@RBrid
RBrid marked this pull request as draft June 4, 2026 23:28
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@RBrid
RBrid marked this pull request as ready for review June 5, 2026 18:19
@kmahone
kmahone merged commit 4be0057 into openclaw:master Jun 5, 2026
20 checks passed
@github-actions github-actions Bot mentioned this pull request Jun 7, 2026
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setup: 'preflight-wsl' failure message uses x86 firmware terminology on ARM64

2 participants