Cooperative UI turns for concurrent winapp ui agents - #767
Cooperative UI turns for concurrent winapp ui agents#767Nikola Metulev (nmetulev) wants to merge 27 commits into
winapp ui agents#767Conversation
Concurrent winapp.exe processes share one Windows desktop, so a command could steal focus or dismiss another workflow's transient UI even though the existing foreground guards stopped wrong-window injection. Add an owner-aware coordination service so desktop-driving commands take turns. Coordinator (Services/InteractiveDesktop): - Owner identity: WINAPP_UI_OWNER_ID, else the immediate parent process, else a one-command anonymous owner. Only a domain-separated SHA-256 key is persisted; the raw value never reaches disk, logs, or telemetry. - state.lock, atomically published state.json with schema versioning and unknown-field preservation, active.lock, and DeleteOnClose participant leases scoped per user and Windows session. - A pure, clock-injected scheduler implementing the forward barrier, FIFO promotion, four-second idle grace, and handoff. - Liveness is proven only by a held lease plus PID/start match: there are no heartbeats, so a suspended process keeps its place in the queue. Command integration: - New two-phase UiCoordinatedAction: all local validation runs in Preflight, so a malformed command never opens a lease, takes a ticket, or joins the queue. - All 21 ui commands declare a mode. active.lock is held only across the desktop-sensitive section, never across output formatting, PNG encoding or file publication. - Every DesktopExclusive command resolves and validates the HWND, PID and element it acts on inside that section, so nothing acts on state captured before an unbounded queue wait. - IDesktopForegroundService is now the only path to SetForegroundWindow and window restore. - send-keys no longer focuses its --target before foreground validation. - record is TurnShared so same-owner input interleaves; screenshot starts observational and escalates the whole invocation, discarding buffered captures and recapturing from the beginning. Also: privacy-minimized bucketed coordination telemetry, npm AbortSignal threaded through both spawn helpers, and documentation across the UI guide, usage, telemetry, JSON envelope reference, shipped skill, agent guidance, sample, and npm README. Refs #764
…ce tests Review round 2 findings, each with a regression test proven to fail without the fix: 1. Prior-boot idle deadline stranded the turn. Environment.TickCount64 resets on reboot, so a persisted deadline from a long-uptime session could hold the desktop for days. Normalize now clamps a deadline beyond now + IdleGraceMs, and the UTC diagnostic is overflow-safe. 2. Screenshot escalation swallowed OperationCanceledException and UiCoordinationException in its catch-all, reporting internal_error while the coordinator saw a normal completion and renewed the grace. All ten UI handlers now filter their catch-all with UiCoordinatedAction.IsCoordinationFault, and the coordinator no longer treats a cancelled token as a normal completion. 3. CompleteCommand rewrote the idle deadline without checking that the completing owner is the current owner, letting a foreign completion revoke or extend a stranger's grace. It now takes the full UiOwnerIdentity and mutates the deadline only on a match. 4. RegisterObserve ignored a Detached admission, leaving the lease open and completing against a foreign owner when ownership lapsed mid-registration. 5. IDesktopSection.EnterAsync documented reentrancy the implementation deliberately does not provide. 6. Missing state.json was treated as fresh unconditionally, so an external deletion while a participant was live could mint a second owner. It is fresh only with no live participant and a free active.lock. Also adds spec 18.3 real-app acceptance coverage: a tight burst protecting transient menu UI, a >4s reasoning gap forcing handover and replay, and a recording pinning its owner while same-owner input continues and another owner waits - driven by real separate winapp.exe processes against a real window. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…daries Recording/cancellation correction (reviewer): - bodyCompletedNormally now means "the body returned rather than threw", not "the token is unset". `ui record` observes Ctrl+C deliberately, finalizes the MP4 and returns success; per spec that is a completed command and must renew the owner's grace. The earlier generic token check wrongly denied renewal. Commands that must not renew let cancellation propagate instead, which is what the handler catch-all filters are for. - Added coverage on both sides: a queued screenshot escalation that is cancelled removes its ticket and renews nothing, while an active recording that finalizes on cancellation does renew. - Added handler-level coverage that UiScreenshotCommand lets a cancelled or refused escalation escape instead of flattening it to internal_error. Additional hardening: 1. InteractiveDesktopPaths.IsCurrentUserOnly now also requires the directory owner to be the current user: an owner implicitly holds WRITE_DAC and can rewrite even a protected, current-user-only DACL. The repair path re-reads and fails closed if ownership could not actually be taken. 2. UiSendKeysCommand now runs the same in-section HWND/PID validation as click and invoke. Without --target it could act on a recycled session handle; with --target the re-resolved element is verified to still belong to the session process before foreground, focus, post or send. 3. Lock acquisition no longer retries every IOException. Only ERROR_SHARING_VIOLATION (32) and ERROR_LOCK_VIOLATION (33) are contention; anything else fails desktop_coordination_unavailable instead of spinning forever on a failure that will never clear. ParticipantRegistry keeps its fail-safe-as-live probe (documented why it differs) but no longer reports a vanished lease as held. 4. Screenshot blank-retry foreground now goes through IDesktopForegroundService rather than a direct PInvoke.SetForegroundWindow seam, so every foreground change has one choke point. Added a guard test that scans production sources for direct SetForegroundWindow/ShowWindow calls outside that service, plus a self-check that the guard's pattern still matches. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds owner-aware cooperative turns so concurrent winapp ui workflows safely share the Windows desktop.
Changes:
- Adds lease-backed scheduling, desktop locking, recovery, cancellation, and telemetry.
- Coordinates all UI commands with post-wait target revalidation.
- Adds npm
AbortSignalsupport, documentation, and extensive tests.
Reviewed changes
Copilot reviewed 87 out of 87 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/winapp-npm/test/abort-signal.test.ts |
Tests abort propagation. |
src/winapp-npm/src/winapp-commands.ts |
Adds generated signal support. |
src/winapp-npm/src/winapp-cli-utils.ts |
Forwards abort signals. |
src/winapp-npm/src/ui-record-guard.ts |
Forwards recording signals. |
src/winapp-npm/scripts/generate-commands.mjs |
Generates signal options. |
src/winapp-npm/README.md |
Documents cancellation and ownership. |
docs/npm-usage.md |
Adds generated signal documentation. |
src/winapp-CLI/WinApp.Cli/Services/InteractiveDesktop/* |
Implements coordination state, scheduling, leases, locking, recovery, output, and telemetry. |
src/winapp-CLI/WinApp.Cli/Services/UiAutomationService.cs |
Injects foreground coordination. |
src/winapp-CLI/WinApp.Cli/Services/UiAutomationService.Screenshot.cs |
Coordinates screenshot escalation. |
src/winapp-CLI/WinApp.Cli/Services/UiAutomationService.Record.cs |
Coordinates recording operations. |
src/winapp-CLI/WinApp.Cli/Services/IUiAutomationService.cs |
Extends capture contracts. |
src/winapp-CLI/WinApp.Cli/Helpers/UiCoordinatedAction.cs |
Adds coordinated command lifecycle. |
src/winapp-CLI/WinApp.Cli/Helpers/IDesktopForegroundService.cs |
Centralizes foreground operations. |
src/winapp-CLI/WinApp.Cli/Helpers/DesktopTargetValidation.cs |
Validates targets after waiting. |
src/winapp-CLI/WinApp.Cli/Helpers/PointerCommandSupport.cs |
Routes foreground requests safely. |
src/winapp-CLI/WinApp.Cli/Helpers/UiJsonError.cs |
Adds coordination errors. |
src/winapp-CLI/WinApp.Cli/Helpers/UiJsonContext.cs |
Serializes coordination details. |
src/winapp-CLI/WinApp.Cli/Helpers/HostBuilderExtensions.cs |
Registers coordination services. |
src/winapp-CLI/WinApp.Cli/Commands/Ui*Command.cs |
Classifies and coordinates UI commands. |
src/winapp-CLI/WinApp.Cli/Program.cs |
Opens telemetry scope. |
src/winapp-CLI/WinApp.Cli/NativeMethods.txt |
Adds process-enumeration APIs. |
src/winapp-CLI/WinApp.Cli/Telemetry/Events/CommandCompletedEvent.cs |
Reports coordination telemetry. |
src/winapp-CLI/WinApp.Cli.Tests/InteractiveDesktopSchedulerTests.cs |
Tests scheduler semantics. |
src/winapp-CLI/WinApp.Cli.Tests/InteractiveDesktopStoreTests.cs |
Tests storage and recovery. |
src/winapp-CLI/WinApp.Cli.Tests/InteractiveDesktopLockTests.cs |
Tests lock lifecycle. |
src/winapp-CLI/WinApp.Cli.Tests/InteractiveDesktopMultiprocessTests.cs |
Tests cross-process coordination. |
src/winapp-CLI/WinApp.Cli.Tests/InteractiveDesktopRealAppTests.cs |
Tests real desktop workflows. |
src/winapp-CLI/WinApp.Cli.Tests/UiCommandTests.Coordination.cs |
Tests command classifications. |
src/winapp-CLI/WinApp.Cli.Tests/FakeInteractiveDesktopLock.cs |
Provides fake turns. |
src/winapp-CLI/WinApp.Cli.Tests/FakeDesktopForegroundService.cs |
Provides fake foreground operations. |
src/winapp-CLI/WinApp.Cli.Tests/DesktopPrimitiveGuardTests.cs |
Guards foreground-call conventions. |
src/winapp-CLI/WinApp.Cli.Tests/FakeUiServices.cs |
Extends UI test fakes. |
src/winapp-CLI/WinApp.Cli.Tests/UiCommandTests.cs |
Registers coordination fakes. |
src/winapp-CLI/WinApp.Cli.Tests/UiCommandTests.Record.Stdin.cs |
Updates handler construction. |
src/winapp-CLI/WinApp.Cli.Tests/UiAutomationServicePureTests.cs |
Tests coordinated blank retries. |
src/winapp-CLI/WinApp.Cli.Tests/RealUiAutomationTests*.cs |
Updates capture and recording tests. |
src/winapp-CLI/WinApp.Cli.Tests/GestureTargetingTests.cs |
Updates service test contract. |
src/winapp-CLI/WinApp.Cli.Tests/UiaTestFixture.cs |
Adds transient-menu fixtures. |
docs/ui-automation.md |
Documents cooperative turns. |
docs/usage.md |
Documents workflow identity. |
docs/telemetry.md |
Documents coordination telemetry. |
plugins/winapp/skills/winapp-ui-automation/SKILL.md |
Updates agent UI guidance. |
plugins/winapp/skills/winapp-ui-automation/references/ui-json-envelope.md |
Documents coordination errors. |
plugins/winapp/agents/winapp.agent.md |
Adds ownership guidance. |
samples/winui-app/README.md |
Demonstrates owner setup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Build Metrics ReportBinary Sizes
Test Results✅ 4972 passed, 18 skipped out of 4990 tests in 1059.5s (+221 tests, +200.6s vs. baseline) Test Coverage✅ 88.7% line coverage, 82% branch coverage · CLI Startup Time44ms median (x64, Try This BuildInstalls 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://github.com/ghraw/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 767Switching between builds often?Put the tool on your PATH once: & ([scriptblock]::Create((irm https://github.com/ghraw/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPathThen this build is just: winapp-pr 767Run Updated 2026-09-05 07:44:34 UTC · commit |
… npm docs Fixes the five substantive review findings on the cooperative UI turns PR: - UiTurnAgeBucket was populated from the queue-wait stopwatch, so it duplicated the wait bucket and was always 0 for an immediate acquisition. The turn's claim tick is now persisted in state (TurnStartedTick64, written only by ClaimTurn alongside TurnId) so the bucket reports how long the owning workflow has held the desktop across all of its commands. - UiTurnAction.HandoffAfterIdle was unreachable: BeginParticipating normalized an expired owner away and then reported New. The previous owner key is now captured before normalization, so taking over a lapsed turn is distinguishable from finding a genuinely free desktop. - ui record's catch-all swallowed coordination faults, turning an active.lock failure into internal_error and letting the coordinator renew the owner's grace on a command that never ran. It is now filtered with IsCoordinationFault like the other eleven desktop-sensitive handlers. - The ui-json-envelope reference claimed an npm AbortSignal produces the native cancelled envelope and exit 130. Node force-terminates the child and rejects with AbortError; that row is now native Ctrl+C only. - CommonOptions.signal has a multi-paragraph JSDoc that was emitted verbatim into every generated Markdown table, terminating the row at its first newline. Table cells are now flattened, and signal is treated as a common option so it is documented once instead of in all 43 command tables. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ff5ec0b-09ea-4a4e-baef-2e5d807feb1e
H1 CI gating. InteractiveDesktopMultiprocessTests and InteractiveDesktopRealAppTests are gated on WINAPP_UI_MULTIPROCESS_TESTS=1, but no workflow ever set it, so all 11 skipped everywhere and the coordination protocol was effectively unverified in CI. They now run in the existing interactive e2e-test-ui job, after the CLI artifacts are downloaded, with the gate set explicitly. The step asserts against the TRX counters rather than the exit code, because a run that skips everything still exits 0: zero matched tests, any skip, or any failure fails the job. Verified both ways locally - 11/11 pass with the gate set, and with it unset the guard trips on 11 skips. The existing WinUI E2E step is untouched. H2 recording pre-start cancellation. The catch at UiRecordCommand ~400 swallowed a native Ctrl+C that arrived before capture started and returned 1. The coordinator decides whether to renew the owner's idle grace from whether the body returned or threw, so a recording that produced nothing looked like a completed command and kept the desktop reserved. It now propagates when the caller's token is cancelled, and InteractiveDesktopLock handles post-acquisition cancellation with the documented contract - exit 130, the `cancelled` envelope, no grace renewal - instead of letting it escape as "an unexpected error occurred". The established behaviour is preserved and pinned by its own test: an ACTIVE recording that observes cancellation, finalizes its MP4 and returns success is a completed command and still renews. H3 send-input focus drift. The foreground was verified before an awaited FocusAsync and never rechecked before injection. Setting focus can itself change the foreground - a focus handler activating another window, or any app stealing focus during the await - and the published repro exited 0 while HELLO landed in a decoy window. send-input now re-verifies after focusing; nothing between that check and keyboardInput.Send awaits. post-message posts straight to the target HWND's queue and is deliberately unaffected. H4 legitimate cross-process owned windows. DesktopTargetValidation rejected any target HWND whose PID differed from the session's, but GetAllAppWindows intentionally discovers cross-process windows the app OWNS - common-item file pickers and system dialogs - and tags elements with those foreign HWNDs, so every such target became unreachable after a queue wait. Validation now accepts a live target whose GW_OWNER chain reaches a live window belonging to the expected PID, mirroring the exact association discovery uses. Recycled-handle protection is preserved: each owner link is checked for liveness AND for belonging to the expected process, so a dead or reused owner cannot launder an unrelated window in. The walk is depth-capped and cycle-guarded. H5 future state version. InteractiveDesktopStateStore deserialized strongly before checking the version, so a newer schema that changes field SHAPES rather than merely adding fields threw JsonException and was classified as corruption - quarantined and replaced with a v1 document. The published repro had `owner` as a string and `ui list-windows` silently downgraded the file. The root version is now read with JsonDocument first and an unknown newer version returns UnknownNewerVersion before any typed deserialization, leaving the bytes untouched. Malformed JSON and a non-numeric version still take the existing corruption-recovery path. H6 foreground capture verification. screenshot --capture-screen and record --capture-screen requested the foreground and then BitBlt'd the screen without confirming activation took. SetForegroundWindow is advisory, so a refusal produced a PNG/MP4 of whatever window was actually in front while the command exited 0 - the repro captured a magenta decoy for a green target. Both paths now verify after the activation delay and immediately before capture, through one shared helper, and report the existing foreground_not_target contract rather than internal_error. No artifact is written on refusal. Window-targeted capture (WGC/PrintWindow) is unaffected. Regression tests fail before their fix and pass after: 10 for the owner chain, 3 for the version probe, 3 for focus drift, 2 for pre-start cancellation plus the positive finalized-recording case, 1 for post-acquisition cancellation, and 2 service-level plus 2 command-level for capture verification. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ot loop Follow-up to H6, caught by self-review. CaptureForegroundNotTargetException derives from InvalidOperationException and is therefore not a coordination fault, so the per-window catch-all in CaptureMultipleWindows swallowed it, recorded it as a per-window failure, and ended with 'No windows could be captured' as internal_error - exactly the outcome H6 exists to replace. The path is reachable with plain --capture-screen whenever discovery finds more than one top-level window, or any owned dialog for a single-window session, which is the common case the flag is used for. The foreground is a property of the desktop rather than of one window, so a refused activation is not a per-window condition; it now propagates out of the loop the way DesktopEscalationRequiredException already does, and lands on the handler that reports foreground_not_target. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
The canonical build publishes both win-x64 and win-arm64, and the CI lane downloads
the whole cli-binaries artifact, so BOTH runtime identifiers are on disk when the
newly enabled gated step runs. Both InteractiveDesktopMultiprocessTests and
InteractiveDesktopRealAppTests resolved the executable by probing a fixed
{ win-arm64, win-x64 } order and taking the first hit, which is a choice by luck
rather than by architecture: on the x64 windows-latest runner they would
deterministically pick the ARM64 binary and every child Process.Start would fail.
The bug was invisible locally because this dev box is arm64, where the wrong-order
probe happens to land on the right file - the same reason the gated suite passed
11/11 here while being broken for CI.
Resolution now lives in one shared WinappTestBinary helper rather than being
duplicated in both suites, and is driven by RuntimeInformation.OSArchitecture: an
arm64 host runs arm64 natively (x64 only under emulation) and an x64 host cannot run
arm64 at all, so the OS architecture is the one that is always executable. It is
fail-closed - only the matching RID is ever returned, never a different one. A build
that produced only the other architecture now fails with a message naming both the
required and the found RIDs, instead of skipping and reporting the suite as merely
"not run"; no build at all stays inconclusive, which is the ordinary local-dev state
and which the CI step's skip guard already turns into a job failure.
The RID choice is a pure function so both architectures are asserted from one
machine, which is what makes this regression testable at all: WinappTestBinaryTests
covers x64, arm64, that the two differ, that an unpublished architecture throws
rather than guessing, and that resolution never hands back a binary built for the
other architecture.
Also corrects a comment on the H4 owner-chain walk. It claimed to mirror "the exact
association" used by discovery, but GetAllAppWindows checks a single GW_OWNER hop
while the validator follows up to eight. The walk is a deliberate superset - a picker
can parent a nested dialog - and is safe because every hop enforces the same
property: the link must resolve to a live window whose PID equals the expected
process. Depth changes how far the chain is followed, never what qualifies as a
match.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The agent half of the protocol was missing, so the command channel had nothing real to talk to and the update rules the spec requires had no implementation at all. Guest agent mode. A hidden `guest-agent` verb runs winapp as the persistent agent, and `GuestCommandServer` serves the same `IGuestTransport` the host channel consumes. Because both halves depend only on that interface, they are now run against each other in one process over an in-memory pair -- which proves the two implementations agree rather than each agreeing separately with a fixture. The agent implements no application semantics; every operation becomes an ordinary guest winapp child process, which is what keeps guest behaviour identical to local behaviour instead of a second implementation that drifts. Readiness is verified rather than assumed. `GuestSessionProbe` reads the real session, window station, and input desktop. Opening the input desktop is the load-bearing check: a closed Sandbox client leaves the guest session and UI Automation working while real input and Windows Graphics Capture stop, so capabilities report inspection as available in exactly the state where input must be refused. Agent versioning. `GuestAgentUpdatePlanner` decides reuse, stage, install, or fail using the stamped version *and* the binary hash: version alone cannot separate two builds of the same version, and the hash alone carries no ordering, so neither can tell an upgrade from a downgrade. A newer guest is never moved backwards -- it is reused when protocols overlap and reported incompatible when they do not, even under an explicit force-repair. `GuestAgentInstaller` stages, verifies the hash of what actually landed, self-tests the candidate in its own process, then activates by rename with the previous binary retained as last-known-good. The self-test runs before anything is swapped, so the common failure never leaves a target needing recovery. Owner-context forwarding. The guest agent is the parent of every guest child, so parent-derived owner identity would collapse every host workflow into one owner and let two workflows drive the same desktop at once. The host now resolves its owner with Cooperative UI Turns precedence and forwards an opaque token scoped to target and epoch; the raw explicit owner never leaves the host. This is only the forwarding contract -- the feature itself remains #767. 346 execution-target tests pass, up from 306. Verified against the real binary: `guest-agent --self-test` reports ready on an interactive host and the verb stays out of `--help`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
An independent review raised ten findings against the guest agent and deployment work. Nine were valid; each is fixed with a regression test that states the rule rather than covering a line. Readiness is a property of the moment, not of the connection. The capability handshake happened when the channel opened; a user can close the Sandbox window at any point after that, which silently removes real input while leaving UI Automation working. Requests now declare whether they inject input, and the guest re-probes immediately before starting one -- refusing rather than launching a process that would report input it never delivered. A cancelled upload left the guest holding the destination open. The host never sent a cancel for a file transfer at all: only execution had that path. It does now, and the guest disposes the partial write on receipt. The test that proves it cancels mid-transfer and immediately retries the same file, which is exactly what failed before. The no-downgrade rule had an architecture-shaped hole. Architecture was compared first, so a *newer* guest reporting a different architecture -- running emulated, for instance -- was replaced with an older host binary, a downgrade reached without ever comparing versions. Version ordering now comes first and has no exceptions, including under force-repair. An unorderable version also fails closed rather than being replaced: it cannot be proven older, so replacing it might be a downgrade. A forwarded owner must never be silently altered. The resolver trimmed whitespace and truncated oversized values; #767 preserves valid values byte for byte and rejects the rest. Trimming would merge two locally distinct workflows into one guest owner, and truncating would merge two distinct long ones -- both breaking the property forwarding exists to preserve. Invalid values are now refused, and the value itself never appears in the failure. Managed roots were walked and written through links. Enumeration skipped reparse *files* but recursed through reparse *directories*, so a junction made an entire outside subtree look like managed content. Enumeration no longer descends through any reparse point, and writes verify every ancestor, creating missing ones as they go so nothing can introduce a link between the check and the write. Reconciliation had two ordering bugs. A clean wipe ran before the dirty commit, so a crash between them left state claiming a complete deployment over an empty folder. And writes ran before deletes, which cannot work for a path that changed between file and directory -- the case reconciliation exists to handle. Dirty is now committed first, and removals precede writes. Job containment is layered. `Process.Start` cannot create a process already inside a job, so assigning afterwards leaves a window. The agent now places *itself* in a job at startup, and Windows puts every descendant of a job member into that job at creation with no window at all -- so nothing can escape the agent under any timing. The per-operation job still provides the finer-grained kill. The test now captures a spawned grandchild's own process ID and asserts on that. Also: the self-test never drained its candidate's output, so a candidate printing more than a pipe buffer would deadlock and look like a hang; it is now drained and the tree killed on timeout, which also releases the staged file. And standard input could overtake the request it belonged to, because the operation ID was published before the request was sent. Finding 1 was superseded: the hidden verb now constructs and serves a real agent through `GuestAgentRunner`. With the fixes in place, `sandbox exec` and `sandbox cp` land on top of them. Both are built on the primitives deployment already uses, so hashing, verification, atomic replacement, and containment behave identically however a file arrived -- a second transfer path would be a second set of bugs. Two of my own earlier tests asserted the pre-fix behaviour and were rewritten to state the corrected rules; leaving them would have pinned the bugs in place. 397 execution-target tests pass, up from 373. CLI schema regenerated for the new public commands, plus reference and concept documentation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…perative UI turns Takes latest main as the baseline for every conflicting file - all 21 command handlers, the extracted WinApp.UIAutomation / .Recording packages and their tests, the shared test support fakes, and HostBuilderExtensions - and keeps the CLI-internal coordination infrastructure to be re-applied on top. WinApp.Cli/Services/IUiAutomationService.cs stays deleted; the CLI now consumes the package's IUiAutomation, IUiTargetResolver, IUiSelectorParser, UiTarget and UiSelector. The package surface is intentionally left coordination-blind, so this commit drops the IDesktopSection / observeOnly plumbing the branch had threaded through it. This resolves the merge only. The coordination layer is ported onto the new APIs in the commits that follow. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Two tiers replace three owner kinds. Collision arbitration stays unconditional for every desktop-sensitive mutation; continuity between commands is now opt-in. WINAPP_UI_OWNER_ID becomes WINAPP_UI_WORKFLOW_ID, because the value names one logical workflow rather than an agent, an app or a process. When it is absent the command gets a unique anonymous one-command owner: it queues and arbitrates like anyone else, but banks no idle grace and hands the desktop off the instant it finishes, so a one-shot can never strand the desktop waiting for a follow-up that is never coming. The parent-derived owner is deleted outright - UiOwnerKind.Parent, the parent PID/start fields on OwnerRecord and WaiterEntry, ReleaseDeadParentReservation, ICoordinationLivenessProbe.IsParentAlive, IProcessInspector.TryGetParentProcessId/TryGetProcessStartTicksUtc, the Toolhelp snapshot walk and its five NativeMethods entries, and the diagnostic parent PID in --verbose waiting output. Inferring a workflow from process ancestry silently grouped unrelated commands that merely shared a shell and just as silently split commands of one workflow that did not; continuity you cannot see is worse than none. The grace rule now reads off UiOwnerIdentity.HasContinuity, so the two-tier decision lives in one place instead of being re-derived per call site. Also restores IsIconic/ShowWindow to the CLI's NativeMethods after PR #799 moved the window P/Invokes into the package, and reads foreground state through the package's public ForegroundGuard.ForegroundBelongsTo. Both keep coordination CLI-side without adding anything to the package surface. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Re-applies cooperative-turn integration on top of main's package-based handlers, which the merge had reset. Every handler regains the two-phase shape: Preflight does local-only validation so a malformed command never opens a lease or joins a queue, and ExecuteAsync runs under the workflow turn. Mutating handlers take one desktop section around the work that touches the shared desktop, re-resolve their target inside it, and validate the HWND with DesktopTargetValidation before acting - a queued command may have waited an unbounded time, so anything resolved before the wait is advisory. Screenshot is now unconditionally DesktopExclusive and the entire Observe-to-exclusive escalation mechanism is gone: IUiTurn.EscalateToDesktopExclusiveAsync, the scheduler's EscalateObserveToExclusive transition, the observeOnly plumbing, the discard-and-recapture loop and DesktopEscalationRequiredException. Every capture path restores or foregrounds a window, so a screenshot was desktop-sensitive whatever its arguments; escalation bought a non-blocking start for a command that virtually always ended up blocking anyway, at the cost of a whole second capture pass and a mode that could change mid-command. The capture pass now runs under ONE section spanning discovery, revalidation and every window's pixels - compositing several windows only means something if they were captured against the same desktop state, and a per-window section would let another workflow foreground something between two frames. Encoding, PNG compression and the disk write sit outside the section: they are the slowest part of the command and touch no shared state. Recording holds the desktop only for as long as its capture mode needs it. For WGC and screen-DC it acquires the section before RecordAsync and releases it once the first frame is committed, so a workflow can record itself typing. The started callback only completes a TaskCompletionSource created with RunContinuationsAsynchronously - it never disposes the section, because disposal is async and would otherwise run on the engine's capture thread inside its own callback. Task.WhenAny races the started signal against the recording task so a run that faults, cancels or produces no frame still releases, and release is idempotent. PrintWindow is different: any frame there can trigger the engine's blank-frame foreground retry, so the section is held for the whole recording and the caller gets an actionable text + JSON warning. The predicted mode is asserted against the reported one so future engine drift surfaces instead of silently releasing the desktop mid-recording. Package-internal capture safety (not coordination): both ScreenshotAsync and RecordAsync now verify the target actually reached the foreground before a screen-DC capture and throw the package's existing ForegroundLostException if not. SetForegroundWindow is advisory, so without this a refused activation returned a valid-looking image of the wrong window while reporting success. Commands map it to the established foreground_not_target contract and write no artifact. No coordination type crosses into either package. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CommonOptions gains workflowId, applied to the spawned child's environment only. The wrapper never touches process.env: mutating it would silently enrol every later call in the process - including unrelated ones - into a workflow the caller meant for a single command, and would race across concurrent calls. Two tests pin that, one for an explicit id and one for an id inherited from the environment. Arbitration does not depend on this. Every desktop-sensitive winapp ui command takes a turn whether or not an id is set; what the id buys is continuity between invocations - a shared idle grace, permission to overlap (a recording and the clicks it records), and never being interleaved with another workflow's input. Documented that way in the README rather than as a switch that turns coordination on. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…y-opt-in The old docs led with 'set this variable so the CLI can tell your commands apart', which read like coordination was something you switch on. It never was: arbitration is unconditional and a workflow id only buys continuity between commands. Every surface now says that in the same order - arbitration first, then the opt-in. Also corrected across all surfaces: - WINAPP_UI_OWNER_ID -> WINAPP_UI_WORKFLOW_ID, invalid_ui_owner_id -> invalid_ui_workflow_id, telemetry identity values Explicit/Parent/Anonymous -> Workflow/Anonymous. - Dropped the claim that direct scripts are grouped automatically. There is no process-ancestry fallback any more, so without an id every command is its own one-shot - including several launched from one shell. - Ordering is described honestly as owner affinity first, then FIFO among the remaining workflows once the owner yields or its grace expires. It was previously implied to be strict FIFO, which it has never been. - screenshot is documented as always exclusive, with the escalation story removed, plus the fact that a multi-window composite is captured under one turn and encoded after the desktop is released. - record: a no-id recording blocks every other workflow for its duration, and PrintWindow hosts hold the desktop for the whole recording. - Both package READMEs state plainly that direct NuGet consumers are outside the CLI's coordination guarantee and must serialize themselves. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Retargets every coordination test onto the two-tier owner model and the package APIs, and deletes the tests for concepts that no longer exist rather than adapting them: parent-derived owner identity and liveness, and the screenshot observe-to-exclusive escalation including its cancellation and unknown-version paths. New coverage for the semantics the redesign actually promises: a named workflow banks the four-second grace while an anonymous one banks none; two no-id resolutions produce different owners; an anonymous completion hands off on the next normalization; a no-id recording blocks a no-id click while the same workflow id lets a recording and a click overlap; owner affinity lets the current owner continue ahead of waiting foreign owners, with FIFO applying among those waiters only after it yields; and a persisted grace survives with no live participant lease. Screenshot tests assert it is always DesktopExclusive, that a multi-window composite is captured under exactly one desktop section, that the section is released before encoding and writing, and that a foreground refusal writes no artifact. Record tests cover the first-frame release, release on failure or cancellation before the callback, a late or duplicate callback, the PrintWindow full-duration hold and its warning, and predicted-versus-actual mode disagreement. Package foreground-loss safety is tested in WinApp.UIAutomation.Tests using no coordination type, and PublicApiSurfaceTests proves neither package exports one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…cording test RecordAsync_CaptureScreen_UsesConsentedScreenPath drives the real recording service against a real fixture window, so the engine's new pre-capture foreground check applies to it. The test now brings the fixture to the foreground the way a user would, via the existing DesktopTestHelpers.ForceForeground, and reports inconclusive only if the session refuses activation outright - the refusal path itself is covered directly by CaptureForegroundSafetyTests. The foreground question is asked through the shipped ForegroundGuard rather than a private P/Invoke so the test asks exactly what the engine asks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
An independent review of the integrated feature found six defects that all share a shape: something is checked once, and then acted on later as if the check still held. Each is fixed at the point of use, and each fix has a test that fails without it. A blocked "recording started" write no longer holds the desktop. The engine raises its first-frame callback on the capture thread, and the CLI both signalled its own "the desktop is free" gate and wrote the liveness JSON from inside it. Writing first meant a caller slow to drain stderr — a full pipe, a paused consumer — pinned the capture thread in the callback, so the gate never fired and the section stayed open for every other winapp ui command on the machine. The signal now goes first, which is what makes the release independent of the reader. Recording revalidates its target inside the section. The target is resolved before the command queues; by the time the turn is granted the window may have closed and Windows may have reused its handle. Without the check the command recorded the new occupant and exited 0 — a video of the wrong application is indistinguishable from a correct one. The multi-window screenshot composite had the same hole and never validated any handle, because it branches to the composite path above the single-window check; each handle is now classified before capture and a recycled one is reported as not captured instead of being spliced into the image. ui wait-for propagates cancellation. Its dedicated catch returned an exit code, and the coordinator decides whether to renew the owner's idle grace from whether the body returned or threw — so an interrupted wait renewed grace for a workflow that had already stopped. Worse, the inner polling catch swallowed cancellation too, so --gone could report the element as gone: a success, invented from a Ctrl+C. Screen recording checks the foreground once more before any file exists. The existing check ran right after the activation delay, with selector resolution still to come; it now sits immediately above encoder creation, which is the last moment a refusal leaves no MP4 behind — the contract the foreground_not_target path states. npm uiRecord forwards workflowId. Both wrappers passed only cwd and signal, so a recording started through the SDK became an anonymous one-shot and blocked its own workflow's clicks for the whole recording, which is the exact scenario workflow ids exist for. Also removes the last two references to the deleted owner-id contract — a sample that still set WINAPP_UI_OWNER_ID and an unused error constant mirroring it — and states on WINAPP_UI_LOCK_DIRECTORY that cooperating processes must agree on it, since the recovery hints that mention it previously did not say so. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ation Both readmes already told direct consumers they are outside the CLI's cooperative desktop turns. Two gaps remained against what a reader needs to act on. Neither said the words a reader is looking for - that package calls do not participate in the CLI's workflow arbitration - so the boundary read as a general caution rather than a specific one. And only the UIAutomation readme offered the alternative to serializing yourself: running somewhere nothing else is driving the desktop. That advice is most useful to the recording package, which is the one that can capture a whole screen, where another workflow's clicks and menus land in the video rather than merely racing it. The UIAutomation note points at its own 'Input injection drives the real mouse and keyboard' section instead of restating the desktop guidance, so the advice stays on one canonical surface. No API, behavior, or package contents change: readme text only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
UiJsonError.CodeCancelled had no readers: the cancellation envelope is emitted from InteractiveDesktopLock through UiCoordinationErrorCodes.Cancelled. It was the fourth and last of the duplicated coordination codes, kept in the previous pass only because it had not been named explicitly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
AReasoningGapHandsOverTheTurnAndForcesReplay failed on the hosted lane at its last assertion: the menu was closed again by the time it looked. The timing was the symptom. The test had agent A recover by calling _fixture.OpenFileMenu() directly and then running ui inspect. Neither step participates in coordination. Opening the menu on the fixture bypasses arbitration entirely, and a non-owner's Observe is detached by design - no ticket, no lease, nothing holding the desktop - so agent A never reacquired anything. Agent B still held the turn and its idle grace throughout, which means the menu was surviving on luck rather than on the property the test claims to prove. On a slower hosted agent the luck ran out. Agent A now replays through the same coordinated mutation it used to open the menu the first time, which is the only move actually available to a returning agent: it queues behind agent B, waits out the grace, reacquires the turn, and reopens the menu. The following inspect then means something, because an owner's Observe pins where a stranger's does not, and a new assertion pins the ownership the old test only assumed. Restoring the out-of-band recovery makes the ownership assertion fail with agent B's key, which is the root cause stated directly rather than inferred from a flake. Also corrects the neighbouring remark on the burst test, which still described opening the menu directly on the fixture after that test had moved to the coordinated helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A sweep of hand-written docs, plugin skills, package readmes, samples, npm sources and production comments for text describing behavior this branch changed. The biggest correction is about screenshot. Both the CLI remarks and the docs claimed every capture path restores or foregrounds the window, which is simply not true: an ordinary visible window captured through Windows Graphics Capture is untouched. The real reason screenshot is exclusive is narrower and worth stating accurately - the engine may restore a minimized target, and falls back to foregrounding when frame capture is unavailable or --capture-screen reads the live screen. Neither need is knowable until capture is under way, and the package deliberately offers no coordination hook to react to them, so the lean policy is to take the turn up front rather than guess. Overstating it made the rule look arbitrary; understating it would invite someone to make screenshot observational again. The skill still listed screenshot among the headless and locked-session friendly verbs, which is now wrong twice over: it queues for an exclusive turn, and its capture can need a usable interactive desktop. The same list in docs/ui-automation.md had the same problem. Both now name screenshot as the exception among the non-injecting verbs and say why. Smaller stale text: the skill asked for an owner id when the variable users set is a workflow id; its turn-taking list repeated the exclusive marker twice; the JSON envelope reference and a coordination outcome comment still said owner id where the external concept is the workflow id; the verbose wait line no longer prints a parent PID; and three comments still described the escalation path and the ticket it used to assign, which no longer exist. Deliberately left alone: owner as the internal scheduler term, the historical note in ResolveMode explaining why escalation was rejected, the note in UiOwnerResolver explaining why there is no ancestry fallback, and strict FIFO where it is scoped to ordering among waiters, which owner affinity does not weaken. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
One critical abort-handling defect and five moderate coordination and capture-safety issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 100/101 changed files
- Comments generated: 6
- Review effort level: Balanced
Five findings from the Copilot review, each with a test that fails without its fix. An aborted npm call could kill the host process. Node emits 'error' with the AbortError and then 'close' with a non-zero code for one aborted spawn, so both handlers ran. The promise ignores the second settle, but process.exit does not: a caller with exitOnError got a rejection to handle and then had the whole process torn down underneath it. Both entry points now route every outcome through one exactly-once gate. An ill-formed workflow id silently merged unrelated workflows. The owner key is SHA-256 over UTF-8, and the default encoder substitutes U+FFFD for anything it cannot encode - so "\uD800", "\uD801" and a literal "\uFFFD" all produced the same key, and three unrelated workflows shared one owner and one desktop. The encoding is now strict and an unpaired surrogate is refused as invalid_ui_workflow_id. It also has to be refused on the npm side, because Node performs that same substitution while building the child environment: by the time the value reaches the CLI the distinction is already gone. Screenshot and record took the desktop before checking where they would write. Screenshot claims the desktop exclusively, so a command whose output path was already impossible - a directory, a trailing separator, a parent that cannot be created - queued for a turn, foregrounded a window and captured pixels, then failed on the last step, having made every other workflow wait for nothing. Record checked for an existing output only under --frames, so an ordinary recording queued and was then refused by the engine's no-clobber check. Both now resolve and validate the path in Preflight. The later checks stay: the file system can change while a command waits, and the engine's no-clobber check is still the final word. Overwriting an existing screenshot remains deliberate, and record's generated default is unique by construction so it is not preflighted. A recycled dialog handle could be composited into another app's screenshot. Owned dialogs - file pickers, print dialogs - run in a shared system host, so validating one against its own current PID accepts any live window in that host, including a handle reused after the real dialog closed. Windows now carry the process they were discovered FOR alongside the process that owns them, so an owned dialog is validated against the originating application and its owner chain has to still reach it. A dialog whose owner cannot be matched back to the app is dropped rather than guessed at. The app's own windows are unaffected: they expect their own process. Two test fakes had to start modelling reality to keep passing - an owned dialog really does report its app window as GW_OWNER - which is the same reason the earlier multi-window tests needed per-HWND PIDs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The agent half of the protocol was missing, so the command channel had nothing real to talk to and the update rules the spec requires had no implementation at all. Guest agent mode. A hidden `guest-agent` verb runs winapp as the persistent agent, and `GuestCommandServer` serves the same `IGuestTransport` the host channel consumes. Because both halves depend only on that interface, they are now run against each other in one process over an in-memory pair -- which proves the two implementations agree rather than each agreeing separately with a fixture. The agent implements no application semantics; every operation becomes an ordinary guest winapp child process, which is what keeps guest behaviour identical to local behaviour instead of a second implementation that drifts. Readiness is verified rather than assumed. `GuestSessionProbe` reads the real session, window station, and input desktop. Opening the input desktop is the load-bearing check: a closed Sandbox client leaves the guest session and UI Automation working while real input and Windows Graphics Capture stop, so capabilities report inspection as available in exactly the state where input must be refused. Agent versioning. `GuestAgentUpdatePlanner` decides reuse, stage, install, or fail using the stamped version *and* the binary hash: version alone cannot separate two builds of the same version, and the hash alone carries no ordering, so neither can tell an upgrade from a downgrade. A newer guest is never moved backwards -- it is reused when protocols overlap and reported incompatible when they do not, even under an explicit force-repair. `GuestAgentInstaller` stages, verifies the hash of what actually landed, self-tests the candidate in its own process, then activates by rename with the previous binary retained as last-known-good. The self-test runs before anything is swapped, so the common failure never leaves a target needing recovery. Owner-context forwarding. The guest agent is the parent of every guest child, so parent-derived owner identity would collapse every host workflow into one owner and let two workflows drive the same desktop at once. The host now resolves its owner with Cooperative UI Turns precedence and forwards an opaque token scoped to target and epoch; the raw explicit owner never leaves the host. This is only the forwarding contract -- the feature itself remains #767. 346 execution-target tests pass, up from 306. Verified against the real binary: `guest-agent --self-test` reports ready on an interactive host and the verb stays out of `--help`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
An independent review raised ten findings against the guest agent and deployment work. Nine were valid; each is fixed with a regression test that states the rule rather than covering a line. Readiness is a property of the moment, not of the connection. The capability handshake happened when the channel opened; a user can close the Sandbox window at any point after that, which silently removes real input while leaving UI Automation working. Requests now declare whether they inject input, and the guest re-probes immediately before starting one -- refusing rather than launching a process that would report input it never delivered. A cancelled upload left the guest holding the destination open. The host never sent a cancel for a file transfer at all: only execution had that path. It does now, and the guest disposes the partial write on receipt. The test that proves it cancels mid-transfer and immediately retries the same file, which is exactly what failed before. The no-downgrade rule had an architecture-shaped hole. Architecture was compared first, so a *newer* guest reporting a different architecture -- running emulated, for instance -- was replaced with an older host binary, a downgrade reached without ever comparing versions. Version ordering now comes first and has no exceptions, including under force-repair. An unorderable version also fails closed rather than being replaced: it cannot be proven older, so replacing it might be a downgrade. A forwarded owner must never be silently altered. The resolver trimmed whitespace and truncated oversized values; #767 preserves valid values byte for byte and rejects the rest. Trimming would merge two locally distinct workflows into one guest owner, and truncating would merge two distinct long ones -- both breaking the property forwarding exists to preserve. Invalid values are now refused, and the value itself never appears in the failure. Managed roots were walked and written through links. Enumeration skipped reparse *files* but recursed through reparse *directories*, so a junction made an entire outside subtree look like managed content. Enumeration no longer descends through any reparse point, and writes verify every ancestor, creating missing ones as they go so nothing can introduce a link between the check and the write. Reconciliation had two ordering bugs. A clean wipe ran before the dirty commit, so a crash between them left state claiming a complete deployment over an empty folder. And writes ran before deletes, which cannot work for a path that changed between file and directory -- the case reconciliation exists to handle. Dirty is now committed first, and removals precede writes. Job containment is layered. `Process.Start` cannot create a process already inside a job, so assigning afterwards leaves a window. The agent now places *itself* in a job at startup, and Windows puts every descendant of a job member into that job at creation with no window at all -- so nothing can escape the agent under any timing. The per-operation job still provides the finer-grained kill. The test now captures a spawned grandchild's own process ID and asserts on that. Also: the self-test never drained its candidate's output, so a candidate printing more than a pipe buffer would deadlock and look like a hang; it is now drained and the tree killed on timeout, which also releases the staged file. And standard input could overtake the request it belonged to, because the operation ID was published before the request was sent. Finding 1 was superseded: the hidden verb now constructs and serves a real agent through `GuestAgentRunner`. With the fixes in place, `sandbox exec` and `sandbox cp` land on top of them. Both are built on the primitives deployment already uses, so hashing, verification, atomic replacement, and containment behave identically however a file arrived -- a second transfer path would be a second set of bugs. Two of my own earlier tests asserted the pre-fix behaviour and were rewritten to state the corrected rules; leaving them would have pinned the bugs in place. 397 execution-target tests pass, up from 373. CLI schema regenerated for the new public commands, plus reference and concept documentation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Pull request overview
Copilot reviewed 103 out of 104 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
src/winapp-CLI/WinApp.Cli/Commands/UiPenCommand.cs:1
- The return values from
PointerGesturePlanner.TryParsePath/TryParsePointare ignored. If parsing fails, this can silently treat invalid--path/--atinput as absent (or worse, propagate a default point ifPointerPointis a struct), changing behavior from 'reject invalid arguments' to 'inject somewhere anyway'. This validation also belongs inPreflightso invalid input never enters coordination. Fix by checking the boolean return and rejecting withinvalid_arguments(and an actionable message) when parsing fails, beforeRunCoordinatedAsyncis entered.
src/winapp-CLI/WinApp.Cli/Services/InteractiveDesktop/ParticipantRegistry.cs:1 IProcessInspector.IsProcessAliveexplicitly documents thatnullmeans 'liveness cannot be determined' and callers must treat that as 'assume alive'. This implementation returnsfalsefornull, which can incorrectly prune a live participant when start time is unreadable (e.g., permission/Win32Exception scenarios). Fix by treatingalive is nullas live (returntrue), so the registry fails safe and avoids evicting live owners.
src/winapp-npm/test/workflow-id.test.ts:1mock.restoreAll()won’t run if one of theassert.rejectsassertions fails, which can leak a mockedchild_process.spawninto later tests and make failures harder to diagnose. Wrap the mocking + assertions in atry { ... } finally { mock.restoreAll(); }(or useafterEach) so mocks are always cleaned up.
src/winapp-npm/test/npm-usage-doc.test.ts:1- This test relies on
process.cwd()beingsrc/winapp-npm, which is easy to break if the test runner is invoked from the repo root or a different working directory in CI. To make the test robust, resolvedocs/npm-usage.mdrelative to the test file location (e.g., viaimport.meta.url/fileURLToPath) or a repo-root environment/config value rather thancwd.
src/winapp-npm/test/markdown-table-cell.test.ts:1 - Like the docs test, this assumes
process.cwd()issrc/winapp-npm. If the test runner executes from a different working directory, the module won’t be found and all table-cell tests will fail. Prefer resolving thescripts/markdown-table-cell.mjspath relative to the test file’s directory (or a known package root) instead ofcwd.
src/winapp-CLI/WinApp.Cli.Tests/WinappTestBinary.cs:1 otherRidscan accumulate duplicates because the loop walks up multiple parent levels and can encounter the sameartifacts/clilayout more than once (or intermediate directories with the same structure). Consider using aHashSet<string>(orDistinct()before reporting) so the failure message is stable and doesn’t repeat the same RID multiple times.
src/winapp-CLI/WinApp.Cli/Services/InteractiveDesktop/UiCoordinationTelemetryScope.cs:1- The remark about
AsyncLocal<T>assignments made inside an async method not being visible to its caller is misleading in the commonawait-same-flow case (AsyncLocal flows through awaits). If the real issue here is that the summary is set from a different async flow (e.g.,Task.Run) or after the scope is read, document that specific constraint. Otherwise, this comment risks confusing future maintainers aboutAsyncLocalsemantics.
The agent half of the protocol was missing, so the command channel had nothing real to talk to and the update rules the spec requires had no implementation at all. Guest agent mode. A hidden `guest-agent` verb runs winapp as the persistent agent, and `GuestCommandServer` serves the same `IGuestTransport` the host channel consumes. Because both halves depend only on that interface, they are now run against each other in one process over an in-memory pair -- which proves the two implementations agree rather than each agreeing separately with a fixture. The agent implements no application semantics; every operation becomes an ordinary guest winapp child process, which is what keeps guest behaviour identical to local behaviour instead of a second implementation that drifts. Readiness is verified rather than assumed. `GuestSessionProbe` reads the real session, window station, and input desktop. Opening the input desktop is the load-bearing check: a closed Sandbox client leaves the guest session and UI Automation working while real input and Windows Graphics Capture stop, so capabilities report inspection as available in exactly the state where input must be refused. Agent versioning. `GuestAgentUpdatePlanner` decides reuse, stage, install, or fail using the stamped version *and* the binary hash: version alone cannot separate two builds of the same version, and the hash alone carries no ordering, so neither can tell an upgrade from a downgrade. A newer guest is never moved backwards -- it is reused when protocols overlap and reported incompatible when they do not, even under an explicit force-repair. `GuestAgentInstaller` stages, verifies the hash of what actually landed, self-tests the candidate in its own process, then activates by rename with the previous binary retained as last-known-good. The self-test runs before anything is swapped, so the common failure never leaves a target needing recovery. Owner-context forwarding. The guest agent is the parent of every guest child, so parent-derived owner identity would collapse every host workflow into one owner and let two workflows drive the same desktop at once. The host now resolves its owner with Cooperative UI Turns precedence and forwards an opaque token scoped to target and epoch; the raw explicit owner never leaves the host. This is only the forwarding contract -- the feature itself remains #767. 346 execution-target tests pass, up from 306. Verified against the real binary: `guest-agent --self-test` reports ready on an interactive host and the verb stays out of `--help`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
An independent review raised ten findings against the guest agent and deployment work. Nine were valid; each is fixed with a regression test that states the rule rather than covering a line. Readiness is a property of the moment, not of the connection. The capability handshake happened when the channel opened; a user can close the Sandbox window at any point after that, which silently removes real input while leaving UI Automation working. Requests now declare whether they inject input, and the guest re-probes immediately before starting one -- refusing rather than launching a process that would report input it never delivered. A cancelled upload left the guest holding the destination open. The host never sent a cancel for a file transfer at all: only execution had that path. It does now, and the guest disposes the partial write on receipt. The test that proves it cancels mid-transfer and immediately retries the same file, which is exactly what failed before. The no-downgrade rule had an architecture-shaped hole. Architecture was compared first, so a *newer* guest reporting a different architecture -- running emulated, for instance -- was replaced with an older host binary, a downgrade reached without ever comparing versions. Version ordering now comes first and has no exceptions, including under force-repair. An unorderable version also fails closed rather than being replaced: it cannot be proven older, so replacing it might be a downgrade. A forwarded owner must never be silently altered. The resolver trimmed whitespace and truncated oversized values; #767 preserves valid values byte for byte and rejects the rest. Trimming would merge two locally distinct workflows into one guest owner, and truncating would merge two distinct long ones -- both breaking the property forwarding exists to preserve. Invalid values are now refused, and the value itself never appears in the failure. Managed roots were walked and written through links. Enumeration skipped reparse *files* but recursed through reparse *directories*, so a junction made an entire outside subtree look like managed content. Enumeration no longer descends through any reparse point, and writes verify every ancestor, creating missing ones as they go so nothing can introduce a link between the check and the write. Reconciliation had two ordering bugs. A clean wipe ran before the dirty commit, so a crash between them left state claiming a complete deployment over an empty folder. And writes ran before deletes, which cannot work for a path that changed between file and directory -- the case reconciliation exists to handle. Dirty is now committed first, and removals precede writes. Job containment is layered. `Process.Start` cannot create a process already inside a job, so assigning afterwards leaves a window. The agent now places *itself* in a job at startup, and Windows puts every descendant of a job member into that job at creation with no window at all -- so nothing can escape the agent under any timing. The per-operation job still provides the finer-grained kill. The test now captures a spawned grandchild's own process ID and asserts on that. Also: the self-test never drained its candidate's output, so a candidate printing more than a pipe buffer would deadlock and look like a hang; it is now drained and the tree killed on timeout, which also releases the staged file. And standard input could overtake the request it belonged to, because the operation ID was published before the request was sent. Finding 1 was superseded: the hidden verb now constructs and serves a real agent through `GuestAgentRunner`. With the fixes in place, `sandbox exec` and `sandbox cp` land on top of them. Both are built on the primitives deployment already uses, so hashing, verification, atomic replacement, and containment behave identically however a file arrived -- a second transfer path would be a second set of bugs. Two of my own earlier tests asserted the pre-fix behaviour and were rewritten to state the corrected rules; leaving them would have pinned the bugs in place. 397 execution-target tests pass, up from 373. CLI schema regenerated for the new public commands, plus reference and concept documentation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A queued `winapp ui` command used to re-read coordination state every 50-75 ms
until its turn came. That is fine for two or three commands and quadratic-feeling
by sixty: every waiter woke twenty times a second, took state.lock, and asked the
OS whether each of the other waiters was still alive. Seventy concurrent commands
spent 299 seconds of CPU to do 16 seconds of work, and took two and a half minutes
to drain.
Waiting is now push-based. Each process opens a named auto-reset event before it
publishes any state entry naming itself, and whoever changes the state wakes
exactly the participants that change made runnable. The set is computed by
comparing what the state said was runnable before a transaction with what it says
after, so it is a property of the state rather than of any one transition —
promotion, same-owner absorption, barrier release, cancellation cleanup and crash
pruning all reach it, and a transition added later cannot forget to wake anyone.
The scheduler stays the only authority. A signal is a hint that something may have
changed; every waiter still re-reads state under state.lock and re-checks its own
status before doing anything, so a duplicate, stale or misdirected wake costs one
lock acquisition and nothing else. Auto-reset semantics close the other half:
a wake-up delivered while a waiter is between its state read and its next wait
stays latched and is consumed immediately, so there is no window in which a
promotion can be missed.
Pure push would strand people, because the interesting failures are the ones where
nobody is left to send anything. A killed owner publishes no completion; a killed
queue head blocks everyone behind it; a promoter can die between publishing and
signalling. So waiters keep a deadline. Only the head of the queue — and a command
at the front of its own owner's barrier — recheck briskly, because they are the
ones whose progress can depend on a process that died silently; everyone else
cannot run before the head does anyway and keeps a much longer backstop. The head
also wakes exactly at an idle grace expiry, which is a deadline nobody announces.
Removing the polling exposed the second half of the cost. Inside one state.lock
transaction the coordinator normalized — which prunes every dead participant — and
then asked the OS about those same participants twice more, to count live waiters
for the cap and to compute a queue position. A normalized list is already only live
entries, so those answers could not differ; at a full queue it was 192 process
handles per admitted command instead of 64. Admission, promotion, diagnostics and
observed depth now read the pruned lists. The probing queue-position overload stays
for cancellation teardown, which is the one caller with no preceding normalization.
Verbose diagnostics are also built only when a status line is actually due, which
at the old poll rate was invisible and is now most of what a waiter would do.
Measured on this machine against the same commands, an isolated coordination
directory and the same harness, with the pre-change binary as the baseline:
8 anonymous 1.88s -> 1.89s CPU 1.41s -> 1.62s
32 anonymous 7.56s -> 6.24s CPU 14.28s -> 7.08s
32 same workflow 5.13s -> 6.89s CPU 12.48s -> 7.00s
70 anonymous 152.88s -> 16.52s CPU 299.72s -> 16.95s
Nine times faster to drain and eighteen times less CPU at seventy, no change worth
claiming at eight, and no stranded processes anywhere. Peak observed queue depth at
seventy fell from 60 to 50 — not because fewer commands queued, but because the
queue now drains faster than a shell can launch into it.
MaxGlobalWaiters stays 64. The docs now say what it has always counted: live
waiters belonging to other workflows, not processes started and not a workflow's
own commands queued behind each other.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The coordination telemetry summary reports the deepest queue a command ever saw while waiting. Moving diagnostics behind an is-a-status-line-due check took that sampling with it, and under --json or --quiet no status line is ever due — so the depth froze at whatever it was when the command registered and every command that piled up behind it went unrecorded. Those are precisely the runs where queue depth is worth knowing: a script or agent driving the CLI is the thing that produces a deep queue, and it is also the thing that passes --json. The depth is now taken on every state read instead. Counting an already-normalized list is a list length, so doing it unconditionally costs nothing, and the telemetry no longer depends on whether anyone happened to be watching. BuildDiagnostics keeps computing the depth it renders and no longer records it. Also corrects a test whose name claimed the opposite of what it asserts: a registering observation IS in the newly-runnable difference, which is right, and it is not woken because PublishAndSignal skips the participant doing the publishing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
A critical state-validation flaw can permit concurrent desktop turns and must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 108/109 changed files
- Comments generated: 1
- Review effort level: Balanced
…ree desktop
Every property on the persisted state carried a defaulting initializer, so `{}`
deserialized into version 1, ticket 1, no owner, no commands, no waiters — which is
byte-for-byte what CreateFresh() produces. Structural validation then found nothing
wrong with it, because there genuinely is nothing wrong with an empty desktop. So a
file truncated to `{}` by a torn write, a full disk or a stray editor did not look
damaged; it looked like nobody was using the desktop.
The consequence is the part that matters. Reading it never reached
RecoverCorruptState, so HasLiveTurnEvidence never got the chance to fail closed, and
the next command minted itself a fresh turn while other processes still held their
leases — two workflows driving one desktop, which is the single thing this feature
exists to prevent.
Every property the v1 writer always emits is now [JsonRequired], on the root and on
the nested records, so a partial document raises JsonException and takes the path it
should always have taken. The nested records matter for the same reason as the root,
one level down: an entry missing ownerKind or status does not fail, it defaults —
and those fields decide whether a promoted waiter earns an idle grace and whether a
command counts as running. Genuinely optional fields stay optional: owner and
diagnosticIdleExpiresUtc are omitted when null, and an Observe entry carries no
ticket by design.
Verified against a real published file rather than assumed, because required-field
sets are easy to get wrong in the strict direction: version, turnId,
turnStartedTick64, nextTicket, idleExpiresTick64, ownerCommands and waiters are all
present in what the writer produces. One existing round-trip fixture was hand-built
and omitted two of them; it now carries the full set, which is what a newer writer's
document would also contain.
The raw version probe still runs before typed deserialization, so a v99 document
whose field shapes this binary cannot bind is still diverted as a newer schema
rather than being quarantined and downgraded.
Demonstrated end to end on the NativeAOT build, same truncated file and a held
lease. Before: exit 1 from the app-not-found path, meaning the command had already
taken a turn, and the state file rewritten with a brand-new owner. After:
desktop_coordination_unavailable, and the `{}` bytes left exactly as they were.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review comments remain, and validation covers coordination, recovery, foreground safety, npm, and packaging.
Review details
- Files reviewed: 108/109 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Closes #764.
Multiple
winapp.exeprocesses share one signed-in Windows desktop but Windows exposes only one foreground window, keyboard focus, cursor, andSendInputstream. Without coordination, concurrent agents can steal focus, dismiss transient UI, invalidate resolved targets, or act on the wrong app.This PR adds cooperative UI turns with two deliberately separate guarantees:
winapp uicommand participates automatically.WINAPP_UI_WORKFLOW_ID(or npmworkflowId) form one logical workflow and retain the turn for a four-second tight-burst grace.A command without a workflow ID is a unique anonymous one-shot. It still waits safely, but receives no post-command grace and hands off immediately. There is no parent-process inference.
Command model
Observestatus,list-windows,inspect,search,get-*,wait-for,set-value,scroll-into-view, UIA scrollTurnSharedrecordDesktopExclusiveactive.lockaround physical desktop workinvoke,click,drag,hover, wheel scroll,touch,pen,focus,send-keys,screenshotOrdering is owner affinity first, then FIFO among live foreign waiters. Continuous activity by one explicit workflow can delay others indefinitely. Anonymous commands receive zero grace; explicit workflows receive four seconds after normal completion. There is no hard live-command duration limit.
Runtime protocol
Per current user and Windows session:
state.lockprotects short metadata transitions only.state.jsonstores the current owner, commands, grace deadline, tickets, and foreign FIFO.active.lockcovers only restore, foreground, focus, cursor, input, live-screen capture, or equivalent physical-desktop work.FileShare.None + DeleteOnCloselease as crash-safe liveness proof.Waiting is push-based. A transaction that makes commands runnable publishes authoritative state and signals exactly those participants. A signal is only a doorbell: every wake reacquires
state.lockand verifies that the participant isRunningbefore executing.Sparse recovery deadlines cover cases where nobody can signal:
The foreign FIFO admits at most 64 live commands from other workflows. The cap does not count processes that have not registered or commands waiting behind their own current workflow. Dead lease entries are pruned before capacity is evaluated.
Safety and recovery
{}or incomplete nested command/waiter is corruption, not a fresh desktop; with any live lease oractive.lock, recovery fails closed and leaves the bytes untouched.Screenshot and recording
Every CLI screenshot is conservatively
DesktopExclusive. One active section spans rediscovery, validation, and the complete multi-window pixel-capture pass; composition, PNG encoding, disk writes, and output occur after release. Foreground refusal writes no artifact.recordisTurnSharedfor its full lifetime:active.lockthrough the first committed frame and then releases it;active.lockfor the full recording because any frame may need foreground recovery.Package and npm boundary
Coordination remains entirely in
WinApp.Cli. The reusable UI Automation and Recording NuGet packages expose no workflow, owner, turn, queue, lock, or signal API. Direct package consumers retain ordinary behavior and remain outside thewinapp.exearbitration guarantee.Generated npm options add
signal?: AbortSignalandworkflowId?: string.workflowIdis copied only into the spawned child's environment; globalprocess.envis never mutated. Abort force-terminates the child on Windows, while leases let other coordinators recover stale state.Regular-command overhead versus pre-feature main
A dedicated ARM64 NativeAOT A/B compared coordination head
fc9ef956with pre-feature main1567b08a, which has no coordination implementation. Both direct binaries and disposablepackagedClassicApp/mediumILidentities used the same real WinForms fixture and successfulInvokePatterncommand. The final41143f39follow-up only adds required-member validation for malformed persisted state; it adds no normal-path I/O or waiting.ui invokeAcross repeated packaged runs, measured overhead was 7.7–15.3 ms (7–15%), not the separately reported 2.3–2.5 seconds. The fixed cost breaks down to roughly 4 ms of added service construction, 7 ms of turn admission/state publication, and 5 ms of deliberate post-wait target revalidation. A lazy event-creation variant saved less than 1 ms and did not improve queue throughput, so it was not adopted.
The same harness drained 64 final-head commands in 3.89 seconds with 64/64 success and no lost wakes. Raw samples, exit codes, interleaved A/B scripts, and the full report are retained in the session artifact
files/perf-767-final/.Queue performance
Measured A/B with NativeAOT binaries, identical harness/commands, and isolated lock directories:
At 70 processes, targeted wakes delivered a 9.3× faster drain and 17.7× less CPU, with no stranded process. Admission and promotion also reuse already-normalized state instead of repeatedly reopening every participant lease.
Validation
Final head
41143f39:Documentation covers the workflow contract, targeted wake/recovery protocol, exact queue-capacity semantics, JSON errors, npm behavior, package guarantee boundary, and shipped agent guidance.