[Nikola Metulev (nmetulev)]

Description

Windows Sandbox can leave an error-only viewer beside its real desktop. Previously, winapp target snapshot sandbox --json could report sandbox_target_ambiguous for that pair, or report an error window alone as a rendered desktop.

Recognized terminal-error windows no longer count as usable desktops. Unknown, starting, inaccessible, or unfamiliar windows remain conservative: a remembered viewer cannot bypass ambiguity, including when another window appears during restoration. Vanished identities are discarded; replacement windows retain fresh identities without inheriting ownership.

Access-denied and invalid-window inspection failures now report an unavailable desktop in structured JSON rather than escaping as an unexpected error. A missing native root is rejected before dereferencing it. The exception handling stays narrow and does not silently swallow unrelated programming errors.

Detection is read-only. It does not close viewers, reconnect, start/stop a guest, or repair ownership state. The classifier uses bounded, nonlocalized native/UI Automation structure rather than English error messages.

Usage Example

winapp target snapshot sandbox --json

Observed with one real desktop and one singleton-error dialog: the installed baseline reported sandbox_target_ambiguous; an earlier candidate reported the real desktop as rendered, adopted, nonminimized, and input/capture-ready without changing either viewer or the ownership record. If the selected desktop cannot be verified, readiness remains unavailable.

Related Issue

Closes #887

Type of Change

• Bug fix
• Test update
• Documentation

Checklist

• Regressions cover terminal errors, unknown viewers, stale identities, restoration races, real HRESULT exception mappings, and missing roots.
• Snapshot-command tests verify parseable unavailable-desktop JSON and no preparation/repair calls.
• Focused Windows tests, NativeAOT/package validation, and current integration CI pass.
• Canonical Sandbox documentation and shipped skill guidance updated.
• Existing maintainer merges and unrelated main changes preserved.

Screenshots / Demo

Textual CLI behavior above is the relevant evidence; no UI redesign or new screenshots are required. Tests use scripted viewer states and do not create or close live Sandbox windows.

Validation and limitations

• 280 focused non-UI tests passed for the production fix and merged source. The final test-data correction also passed its 19 focused tests; both HRESULT cases enumerate during discovery and retain exact exception-type assertions.
• Full current-head CI passes, including strict discovery/execution accounting, all test lanes, samples, UI E2E, docs and packaging. Local NativeAOT/package builds used -SkipTests after focused tests to avoid unauthorized UI activity on the shared desktop.
• The private viewer structure was observed with Windows Sandbox 0.8.107.0; unknown layouts fail closed. Earlier live evidence includes a mixed error/healthy-viewer comparison and an authorized healthy-main-guest snapshot preserving ownership, viewer identity/bounds and foreground. Captured error/healthy structures were replayed through the classifier and selection policy.
• Those live artifacts precede the final native-exception fix; no final-binary live terminal-error acquisition is claimed. Replay tests classification/selection, not native provider acquisition or failure. Ordinary guest heartbeat/log writes are not claimed unchanged.

Additional Notes

The caller responsible for the observed viewer process with --help remains unidentified; its argv does not prove direct invocation rather than a wrapper. This PR does not redesign cross-installation ownership, change Sandbox lifecycle, or claim command-performance improvements. The readiness label reflects technical checks, not human approval or permission to merge.

[Copilot]

Copilot review overview

🟡 Changes recommended

A viewer whose identity changes during inspection remains a candidate and can still cause false ambiguity.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Skip stale window candidates after post-probe identity changes · New

What changed in this PR

Adds fail-closed Windows Sandbox viewer classification so terminal-error windows are not treated as usable desktops.

Changes:

• Classifies session, terminal-error, and unknown surfaces via bounded UI Automation.
• Applies classification to ownership, readiness, and placement.
• Adds regression tests and user guidance.

File	Description
NativeMethods.txt	Adds UI Automation interop types.
WindowsSandboxWindowController.cs	Filters and validates viewer candidates.
SandboxClientErrorProbe.cs	Implements surface classification.
WindowsSandboxWindowControllerTests.cs	Tests selection and readiness behavior.
SandboxClientErrorProbeTests.cs	Tests classifier safety boundaries.
SKILL.md	Adds troubleshooting guidance.
sandbox-execution.md	Documents snapshot readiness behavior.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The version-specific native UI Automation classifier could not be independently exercised against a live Sandbox viewer here.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Skip stale window candidates after post-probe identity changes

[github-actions]

Build Metrics Report

Validation passed. All required build and validation jobs succeeded.

Binary Sizes

Artifact	Baseline	Current	Delta
CLI (ARM64)	57.06 MB	57.27 MB	📈 +223.0 KB (+0.38%)
CLI (x64)	57.10 MB	57.32 MB	📈 +224.0 KB (+0.38%)
MSIX (ARM64)	23.73 MB	23.78 MB	📈 +46.1 KB (+0.19%)
MSIX (x64)	25.17 MB	25.24 MB	📈 +70.5 KB (+0.27%)
NPM Package	49.48 MB	49.61 MB	📈 +137.5 KB (+0.27%)
NuGet Package	49.59 MB	49.71 MB	📈 +125.0 KB (+0.25%)

.NET Test Results (TRX reports)

Other suites are reflected in the overall validation status above.

✅ 7756 passed, 37 skipped out of 7793 tests in 1137.1s (+45 tests, -224.6s vs. baseline)

Test Coverage

✅ 85.6% line coverage, 80.1% branch coverage · ⚠️ -0.1% vs. baseline

CLI Startup Time

62ms median (x64, winapp --version) · ✅ no change vs. baseline

Try This Build

Installs the MSIX for your architecture, replacing any previously installed build. Needs the GitHub CLI — the command offers to install it and sign you in if it is missing.

& ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 889

Switching between builds often?

Put the tool on your PATH once:

& ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPath

Then this build is just:

winapp-pr 889

Run winapp-pr with no arguments to pick from a list of open PRs.

---

Updated 2026-09-22 03:19:10 UTC · commit ddd2155 · workflow run

[Copilot]

Copilot review overview

🟡 Changes recommended

A remembered session can still bypass ambiguity when another live viewer is unknown.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Suppress remembered session when an unknown candidate exists · New

[Copilot]

Copilot review overview

🟡 Changes recommended

Post-restore validation can still report readiness when an unknown viewer appears during window restoration.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Post-restore check ignores newly appeared unknown viewers · New

Resolved since last review (1)

• Suppress remembered session when an unknown candidate exists

[Copilot]

Copilot review overview

🔵 Needs a closer look

The private Windows Sandbox UI structure requires final human review and exact-head live validation despite the successful build and 62 focused tests.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Post-restore check ignores newly appeared unknown viewers

[Nikola Metulev (nmetulev)]

State: agent-preparing — current CI and independent delta coverage; addressed-feedback re-review alone is not a readiness blocker under the user's updated policy.
Assessed: head d6eb8175db97e4a331fc8c7a9f0ce726d58851d9 against main@3520ee6ff3525c9af1eb5b279930225eae8e8876; latest normal merge includes #892/#904/#901/#905 without changing the ownership fix. Preserved maintainer merge c4440890 and normal main update 9568bf97; nondraft, no rewrite.
Feedback disposition: Human changes-request review 5272604150 is not dismissed. All three comments were fixed in ancestor 4c811839 and answered before resolution: mapped exceptions, missing native root, real HRESULT test mapping. Zach Teutsch (@zateutsch) was explicitly re-requested. This is a request for reassessment, not an approval.
Evidence: Three behavioral regressions failed before the fix. Known UnauthorizedAccessException/ArgumentException mappings now return Unknown with diagnostics; explicit root validation handles null, and unrelated programming errors still propagate. Actual snapshot-command tests verify structured unavailable-desktop JSON and zero Ensure/repair calls. 280 focused non-UI tests pass, and the full scripts\build-cli.ps1 -SkipTests NativeAOT/npm/NuGet/MSIX/docs build passed at 4c811839. The 280 tests also pass after the new main merge; the current full build has passed (-SkipTests, no live UI suite), and current-head CI is running. Local passes are not CI passes.
Independent review / limits: Existing reviewer is extending prior seven-file coverage through the #884/#878 main merges and this three-file exception-boundary fix. Historical healthy .75 smoke at be2e06b3 and captured error replay remain historical, not execution of this new artifact. No current-binary live terminal-error acquisition is claimed. No new live Sandbox/host UI action was performed. The earlier COM-disposal disagreement remains resolved by evidence and reassessment; no unsafe/no-op release was introduced.
Pending / owner: Current required CI/integration, independent final delta coverage, complete fresh discussion/policy/head/base checks. Human/code-owner/last-push approval and requested human re-review remain separate; no approval is fabricated. Agent owns technical follow-through.
Policy / resume: Final label is ready-for-review; agent-blocked only when the agent cannot finish feedback/CI or needs author input. Maintain this one comment and 10-minute same-session monitoring, including requested re-review follow-through. No merge/auto-merge.

[Copilot]

Copilot review overview

🟡 Changes recommended

The polling classifier does not deterministically release UI Automation COM wrappers, allowing native references to accumulate during connection waits.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Dispose UI Automation COM wrappers after probing · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

Builds and 62 focused tests pass, but no running Sandbox was available to validate the private UI Automation structure against a real viewer.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Dispose UI Automation COM wrappers after probing

[Copilot]

Copilot review overview

🔵 Needs a closer look

The native classifier depends on private live Sandbox UI structure and was not validated here against a live terminal-error provider.

Review effort: Balanced
Findings: None

[Zach Teutsch (zateutsch)]

🤖 AI-generated review (winappcli pr-review skill) — verify before acting.

Changes requested — the terminal-error rejection logic, ownership filtering, docs, and tests all hold up, and a NativeAOT publish + the 62 sandbox tests pass. One high-severity gap remains: SandboxClientErrorProbe.Inspect is meant to fail closed to Unknown on any inspection failure, but two real paths escape its catch and can crash the read-only winapp target snapshot sandbox command outside its --json contract. Details inline. The remembered-window adoption/ambiguity behaviors look intentional (each is pinned by a test) and are safe given only one Sandbox VM runs at a time, so nothing else is blocking.

[Zach Teutsch (zateutsch)]

🤖 AI-generated re-review (winappcli pr-review skill) — verify before acting.

Approving — the blocking finding from my earlier review is fully resolved by 4c811839 ("Handle mapped UIA failures without breaking snapshot JSON"), and I re-validated the current head d6eb8175.

What was fixed in SandboxClientErrorProbe:

• The catch now also swallows UnauthorizedAccessException and ArgumentException, the real types that E_ACCESSDENIED (0x80070005) and E_INVALIDARG (0x80070057) map to — so an inaccessible or racing UIA provider now fails closed to Unknown instead of escaping and crashing winapp target snapshot sandbox --json outside its JSON contract.
• A missing native root is guarded by the new IsExpectedRoot(root, processId) helper ([NotNullWhen(true)]), closing the NullReferenceException path when ElementFromHandle returns null on a closing window.
• The tests were corrected and strengthened: the bogus hand-built COMException(0x80070005) was replaced, and Inspect_MappedProviderFailure_RetainsUnknown now drives the real runtime mapping via Marshal.ThrowExceptionForHR, pinning the regression that previously slipped through. Good restraint too — Inspect_ProgrammingFailure_IsNotSilentlyConvertedToUnknown verifies InvalidOperationException still propagates, so the catch wasn't over-broadened to swallow programming errors.

Validation on d6eb8175: dotnet build WinApp.Cli.Tests — succeeded, 0 warnings / 0 errors; ran SandboxClientErrorProbeTests + WindowsSandboxWindowControllerTests — 66/66 passed (was 62; +4 targeted tests).

The merge from main also pulled in unrelated changes (cert-generate DN validation, find-ui/find-api, control-snippet) — those are separately merged PRs, out of scope for this review. Nothing blocking remains on the Sandbox change.

Trivial nit, non-blocking: the fix left var walker = automation.get_RawViewWalker(); in ReadSurface with stray extra indentation. No analyzer flags it; clean up if you touch the file again.

[Zach Teutsch (zateutsch)]

🤖 AI-generated re-review (winappcli pr-review skill) — verify before acting.

Still approving — re-validated the current head c3c010e6. The only change to the Sandbox code since my last approval is a test-quality refinement in SandboxClientErrorProbeTests:

• Inspect_MappedProviderFailure_RetainsUnknown's [DataRow] args moved from typeof(...) to nameof(...) string constants, so the rows are enumerable during MSTest discovery (a Type argument isn't a discovery-time constant). The assertion was also tightened from IsInstanceOfType to an exact-type Assert.AreEqual(expectedType, mapped.GetType()). Both are strict improvements; the production fix from 4c811839 is unchanged.

Validation on c3c010e6: dotnet build WinApp.Cli.Tests — succeeded, 0 warnings / 0 errors; SandboxClientErrorProbeTests + WindowsSandboxWindowControllerTests — 66/66 passed.

Everything else in the range (get-focused ownership #880, lifecycle-docs #915, ui-automation doc/service edits) came in via merges from main and is out of scope for this PR's Sandbox change. Nothing blocking.