Skip to content

fix: node starts on Windows Server when MXC probe is unsupported - #1277

Draft
shanselman with Copilot wants to merge 2 commits into
mainfrom
copilot/version-check-windows-server
Draft

fix: node starts on Windows Server when MXC probe is unsupported#1277
shanselman with Copilot wants to merge 2 commits into
mainfrom
copilot/version-check-windows-server

Conversation

Copilot AI commented Aug 31, 2026

Copy link
Copy Markdown
Contributor
Additional instructions

MUST: Keep Allow edits from maintainers enabled for this PR so maintainers
can help update the branch when needed.

What Problem This Solves

Fixes Windows Server node startup failures caused by attempting to launch the unsupported MXC availability probe.

Why This Change Was Made

  • Detect Windows Server by SKU using Microsoft’s IsWindowsServer product-type check, not build numbers.
  • Skip wxc-exec --probe on Server SKUs and report containment as definitively unavailable.
  • Preserve normal probing on client Windows and existing host-fallback policy.

User Impact

Windows Server users can continue using the node through configured host fallback. Client Windows behavior is unchanged.

Evidence

  • Server SKU coverage confirms the MXC probe callback is never invoked.
  • Client SKU coverage confirms MXC probing continues normally.
  • Existing fallback coverage confirms unavailable containment routes through the host runner unless strict blocking is enabled.

Change Type

  • Bug fix
  • Feature
  • Refactor
  • Docs or instructions
  • Tests or validation
  • Security hardening
  • Chore or infrastructure

Scope

  • Tray or WinUI UX
  • Windows node capability
  • Local MCP or winnode
  • Gateway, connection, or pairing
  • Setup or onboarding
  • Permissions, privacy, or security
  • Tests, CI, or docs

Required proof pools

  • windows-wsl-mxc: MXC availability and system.run fallback routing changed.

Validation

  • Focused MXC availability and command-runner suite: 80 passed.
  • Windows-only build and MXC E2E validation require the declared proof pool.

Real Behavior Proof

  • Environment tested: Linux runner using injected Windows and SKU detection seams.
  • PR head or commit tested: 71a86284
  • Exact steps or command run: Focused MxcAvailabilityTests and MxcCommandRunnerTests.
  • Evidence after fix: 80 passed, 0 failed, 0 skipped.
  • Observed result: Server skips the probe; client runs it; unavailable containment preserves configured fallback.
  • Screenshot or artifact links verified? N/A
  • Not verified or blocked: Native Windows Server execution requires windows-wsl-mxc.

Security Impact

  • New permissions or capabilities? No
  • Secrets or tokens handling changed? No
  • New or changed network calls? No
  • Command or tool execution surface changed? Yes
  • Data access scope changed? No
  • If any answer is Yes, explain the risk and mitigation: Windows Server enters the existing unavailable-containment policy without launching MXC. Existing strict-blocking and host-fallback controls remain authoritative.

Compatibility and Migration

  • Backward compatible? Yes
  • Config or environment changes? No
  • Migration needed? No
  • If yes, list the exact upgrade steps: N/A

Review Conversations

  • I replied to or resolved every bot review conversation addressed by this PR.
  • I left unresolved only conversations that still need maintainer judgment.

Copilot AI and others added 2 commits August 31, 2026 21:03
Co-authored-by: shanselman <2892+shanselman@users.noreply.github.com>
Co-authored-by: shanselman <2892+shanselman@users.noreply.github.com>
@clawsweeper

clawsweeper Bot commented Aug 31, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

Copilot AI changed the title fix: Windows Server no longer launches the MXC probe fix: node starts on Windows Server when MXC probe is unsupported Aug 31, 2026
Copilot AI requested a review from shanselman August 31, 2026 21:07
@clawsweeper clawsweeper Bot added merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. 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. labels Aug 31, 2026
@clawsweeper

clawsweeper Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 31, 2026, 5:12 PM ET / 21:12 UTC.

ClawSweeper review

What this changes

The PR detects Windows Server SKUs before running the MXC probe, marks containment unavailable there, and adds cross-platform unit coverage for the client and server branches.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked until real behavior proof from a real setup is added - 3 items remain

Keep this PR open. The SKU gate is a narrow, coherent way to avoid the unsupported probe and retains the existing strict-block versus host-fallback policy, but the submitted evidence is injected unit coverage only and does not prove the native Windows Server behavior or required MXC validation.

Priority: P2
Reviewed head: 71a862843f16742fda6c2f979be4621f084e5c7d

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The focused patch and seam tests are credible, but merge readiness is capped by missing real Windows behavior proof for the changed execution-boundary decision.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The changed production owner is MxcAvailability.Probe, but the PR supplies only injected Windows/SKU callbacks and a claimed focused test count. It does not show a current-head Windows Server node executing the native SKU check, skipping wxc-exec --probe, and then observing configured system.run fallback or strict blocking. 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.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The changed production owner is MxcAvailability.Probe, but the PR supplies only injected Windows/SKU callbacks and a claimed focused test count. It does not show a current-head Windows Server node executing the native SKU check, skipping wxc-exec --probe, and then observing configured system.run fallback or strict blocking. 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 6 items Introduced Server decision: The PR resolves wxc-exec, then returns a definitive unavailable verdict and skips the probe when the Server SKU provider returns true.
Existing execution policy: When MXC is unavailable, the command runner retains host fallback by default and denies only when the user enabled strict fallback blocking.
Tests use injected platform seams: The added Server test supplies both Windows and Server results through delegates and asserts only that the injected probe callback is skipped; it does not execute DetectWindowsServerSku on Windows Server.
Findings None None.
Security None None.

How this fits together

MXC availability is determined during Windows node startup and feeds the command runner’s choice between contained execution and its configured host fallback. This change alters the availability decision before any MXC probe process is launched.

flowchart LR
  A[Windows node startup] --> B[MXC availability check]
  B --> C{Windows Server SKU?}
  C -->|Yes| D[Mark containment unavailable]
  C -->|No| E[Run native MXC probe]
  D --> F[Existing fallback policy]
  E --> F
  F --> G[Command execution route]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The changed production owner is MxcAvailability.Probe, but the PR supplies only injected Windows/SKU callbacks and a claimed focused test count. It does not show a current-head Windows Server node executing the native SKU check, skipping wxc-exec --probe, and then observing configured system.run fallback or strict blocking. 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.
  • Resolve merge risk (P2) - The native SKU interop is not exercised by the supplied proof. A real detection failure or misclassification could route Windows commands through the existing uncontained fallback by default rather than MXC, so both Server fallback and strict-block behavior need current-head validation before merge.
  • Complete next step (P2) - Human-only merge blocker: the external PR needs real Windows Server and required MXC proof, not an automated code repair.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch scope production +82/-3, tests +51/-13 across 2 files The production change is focused on one availability decision but introduces native platform interop that needs target-host proof.

Merge-risk options

Maintainer options:

  1. Prove the native Server branch before merge (recommended)
    Run the declared Windows Server fallback scenario and the required MXC E2E validation on current head, then attach redacted output showing the probe skip and resulting execution mode.
  2. Pause pending suitable Windows hosts
    Keep the draft open until the maintainer-scheduled proof pool can exercise both the unsupported Server path and MXC-capable containment path.

Technical review

Best possible solution:

Land after posting redacted current-head proof from a real Windows Server node that no probe is launched and approved system.run follows the configured fallback or strict-block policy, plus the required MXC E2E result from an MXC-capable Windows host.

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

No. The supplied injected-delegate tests exercise the intended branches, but neither they nor this Linux review checkout reproduce the current-main failure on a real Windows Server host.

Is this the best way to solve the issue?

Yes, conditionally. An early SKU-specific unavailable verdict is the narrowest path and preserves the existing runner policy, but its native interop and end-to-end behavior require real-host proof.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against f46400aab24e.

Labels

Label changes:

  • add P2: This is a bounded Windows Server node-startup and command-routing repair with limited platform blast radius.
  • add merge-risk: 🚨 security-boundary: The native SKU verdict controls whether approved system.run reaches contained execution or the existing uncontained fallback.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed production owner is MxcAvailability.Probe, but the PR supplies only injected Windows/SKU callbacks and a claimed focused test count. It does not show a current-head Windows Server node executing the native SKU check, skipping wxc-exec --probe, and then observing configured system.run fallback or strict blocking. 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.

Label justifications:

  • P2: This is a bounded Windows Server node-startup and command-routing repair with limited platform blast radius.
  • merge-risk: 🚨 security-boundary: The native SKU verdict controls whether approved system.run reaches contained execution or the existing uncontained fallback.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed production owner is MxcAvailability.Probe, but the PR supplies only injected Windows/SKU callbacks and a claimed focused test count. It does not show a current-head Windows Server node executing the native SKU check, skipping wxc-exec --probe, and then observing configured system.run fallback or strict blocking. 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

What I checked:

Likely related people:

  • Barbara Kudiess: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Scott Hanselman: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Attach redacted current-head Windows Server output showing no MXC probe and the observed host-fallback or strict-block result.
  • Run and report the repository-required MXC E2E validation from an MXC-capable Windows host.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants