Add deterministic explicit UI invoke actions - #846
Conversation
Implement exact-pattern actions, bounded idempotent toggles, strict element identity, structured results, and unit/live coverage. Fixes #821. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Partial UIA searches can still select the wrong duplicate, and an empty npm action silently enables automatic fallback.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds deterministic explicit actions to winapp ui invoke, including strict UIA pattern routing, toggle-state verification, API/npm exposure, tests, and documentation.
Changes:
- Adds seven explicit invoke actions with strict element identity.
- Reports requested and performed actions in JSON.
- Adds extensive CLI, API, UIA, and live-control coverage.
File summaries
| File | Description |
|---|---|
src/winapp-npm/src/winapp-commands.ts |
Forwards the action option. |
src/winapp-CLI/WinApp.UIAutomation/Services/UiAutomationService.cs |
Implements strict action resolution. |
src/winapp-CLI/WinApp.UIAutomation/Services/IUiAutomation.cs |
Exposes the explicit-action API. |
src/winapp-CLI/WinApp.UIAutomation/Services/ExplicitUiInvoker.cs |
Routes patterns and verifies toggles. |
src/winapp-CLI/WinApp.UIAutomation/Models/UiInvokeAction.cs |
Defines action and result models. |
src/winapp-CLI/WinApp.UIAutomation.TestSupport/FakeUiServices.cs |
Extends the UIA test fake. |
src/winapp-CLI/WinApp.UIAutomation.Tests/RealUiAutomationTests.ExplicitActions.cs |
Adds live UIA regressions. |
src/winapp-CLI/WinApp.UIAutomation.Tests/GestureTargetingTests.cs |
Updates the test implementation contract. |
src/winapp-CLI/WinApp.UIAutomation.Tests/ExplicitUiInvokerTests.cs |
Tests action routing and transitions. |
src/winapp-CLI/WinApp.Cli/Helpers/UiJsonContext.cs |
Extends JSON output. |
src/winapp-CLI/WinApp.Cli/Commands/UiInvokeCommand.cs |
Adds parsing, execution, and reporting. |
src/winapp-CLI/WinApp.Cli.Tests/UiCommandTests.Invoke.cs |
Tests CLI behavior and output. |
src/winapp-CLI/WinApp.Cli.Tests/RealRecordingTests.Helpers.cs |
Forwards the new API overload. |
src/winapp-CLI/WinApp.Cli.Tests/PublicApiSurfaceTests.cs |
Allows the new public types. |
plugins/winapp/skills/winapp-ui-automation/SKILL.md |
Documents explicit actions. |
plugins/winapp/skills/winapp-ui-automation/references/ui-json-envelope.md |
Documents invoke JSON. |
docs/ui-automation.md |
Adds canonical action guidance. |
docs/npm-usage.md |
Documents npm support. |
docs/cli-schema.json |
Adds generated option metadata. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Explicit mode can still rebind after waiting or trust partial uniqueness results, while the npm wrapper can convert an empty action into automatic invocation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/winapp-npm/src/winapp-commands.ts:1256
- What is wrong: The npm API exposes a closed seven-value option as unrestricted
string. Show me:uiInvoke({ action: 'selet' })type-checks and only fails at runtime, while existing closed options such asIfExists,SdkInstallMode, andManifestTemplatesare generated as string-literal unions. Why it matters: TypeScript callers lose compile-time validation on the new package surface. Smallest fix: emit a named union for the seven CLI spellings from the command schema, then regenerate this file and npm docs.
src/winapp-CLI/WinApp.UIAutomation/Services/UiAutomationService.cs:1435
- What is wrong: This uniqueness check assumes
FindAll(Descendants)returns every match, but this service already has to compensate for UIA providers returning nonzero partial results, and PR #842 documents that behavior. Show me: a provider has two matching AutomationIds but returns one bulk match ->count == 1-> the action is performed instead of rejected as ambiguous. Why it matters: explicit mode can invoke the wrong duplicate while reporting deterministic identity. Smallest fix: use a complete Control View traversal (or the completion helper from #842) capped at two matches before deciding the AutomationId is unique.
var matches = root.FindAll(TreeScope.TreeScope_Descendants, condition);
var count = matches?.get_Length() ?? 0;
src/winapp-npm/src/winapp-commands.ts:1271
- What is wrong: An empty npm
actionis silently omitted instead of forwarded for CLI validation. Show me:uiInvoke({ selector: 'Save', app: 'myapp', action: '' })-> no--actionargument -> automatic fallback may invoke an element; the native CLI rejects--action ''asinvalid_arguments. Why it matters: a malformed deterministic request can become a successful nondeterministic UI action. Smallest fix: distinguish omission from an explicitly supplied empty value.
if (options.action) args.push('--action', options.action);
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Verify explicit AutomationId uniqueness using a complete ControlView walk and reject unreadable trees. Preserve defined scalar options in generated npm wrappers so empty actions reach native validation. Add partial/deep/fault UIA and argv regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The cross-layer UIA and public API changes require final human review despite strong regression coverage.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Keep the initially selected model for strict identity resolution inside the turn. Only automatic mode re-runs broad selectors after the wait. Cover same-name replacements and queued stale targets. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Automated-review follow-up: all three actionable Copilot findings are fixed with regression coverage (partial identity searches, empty npm action forwarding, and queued text-query rebinding), and all inline threads have been answered and resolved. The suppressed TypeScript string-literal-union suggestion is not a correctness blocker for this change: the CLI preflight rejects unknown values before desktop acquisition, and the generated npm wrapper now preserves even empty values for that validation. Other string-valued UI options (for example scroll direction) use the same runtime-validation contract. A stricter generated TypeScript type is a separate API ergonomics improvement rather than necessary for #821. Final-head review and required CI remain pending; no merge will be performed. |
There was a problem hiding this comment.
🔵 Needs a closer look
The COM identity semantics and new public API surface warrant the requested final human review despite comprehensive coverage.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Build Metrics ReportBinary Sizes
Test Results✅ 7494 passed, 37 skipped out of 7531 tests in 1147.3s (+272 tests, +66.0s vs. baseline) Test Coverage✅ 86% line coverage, 80.2% branch coverage · ✅ no change vs. baseline CLI Startup Time66ms 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://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 846Switching between builds often?Put the tool on your PATH once: & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPathThen this build is just: winapp-pr 846Run Updated 2026-09-18 04:23:33 UTC · commit |
|
Pre-human-review gate complete for head a7656e0: fresh Copilot review 5205557992 covered 22/22 files with 0 new comments; all 5 inline review threads are answered and resolved. All 30 passing checks are green, with 0 failures and 0 pending (one neutral CodeQL summary). Required build-and-package and the final WinUI/UI-coordination E2E job passed in run 34929223926; sample matrix and Code Quality analyses passed. The branch is MERGEABLE and current with main. Local final targeted coverage: 77 explicit unit/live tests, 49 CLI/API cases, 265 npm tests; NativeAOT x64/arm64 and npm/NuGet/MSIX/schema generation completed successfully. Ready for human review; not merged. |
Integrate main 8e5f737. Reuse retained provider elements without re-resolution, retain strict serialized identity behavior, regenerate combined target/action npm bindings, and validate actions before remote target preparation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Duplicate AutomationIds can still cause explicit mode to invoke an arbitrary retained element instead of reporting ambiguity.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
winapp ui invoke --action resolved a plain-text/AutomationId selector to the first match and stamped it with a runtime slug, so the strict uniqueness walk was never reached and a duplicate AutomationId silently activated the first control. For a non-slug selector that matched a control with an AutomationId, commit to that AutomationId so the strict resolver proves it is unique and fails closed when it is ambiguous; a name-only match keeps its runtime slug and a user-supplied slug is used verbatim. Add CLI-level tests covering the identity handoff. Also reject an explicitly empty or whitespace cert password in 'cert generate' so the npm wrapper's empty-string forwarding cannot produce an unprotected PFX, and document explicit --action ambiguity behavior in the agent guide and usage docs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The previous attempt committed a non-slug selector to its AutomationId so the strict resolver would prove uniqueness, but that path is unreachable: after the merge of main, FindSingleElementAsync stamps a live COM Context on every resolved element and GetAutomationElement returns that retained provider before strict resolution runs, so a duplicate AutomationId still activated the first match. Instead, gate explicit --action at the command: for a non-slug selector, verify the selector matches exactly one element (SearchAsync, maxResults 2) before acting and fail closed with an ambiguous_selector error when more than one matches. A slug already names one element and is exempt; automatic invoke is unchanged, so legacy lenient rebinding is preserved. Add CLI tests (fake-backed) covering the ambiguous, unique, and slug cases, and route the ambiguity JSON error through the command's configured error stream. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve collaborator ambiguity guard and require complete AutomationId uniqueness against the retained provider. Compare live identities without replacing the selected element. Add nonactivating regressions and fix empty-password logging test placement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Zach Teutsch (@zateutsch) Fixed the owned-BSTR leak in f3aa8b3. Both new AutomationId/Name reads now use the existing throwing GetBstr helper, which copies the value and frees the BSTR in finally; no SafeGetBstr catch or weaker uniqueness behavior was added. Audited the other string reads introduced by this PR for the same issue. Verified ownership from the generated CsWin32 return projection and ran the same isolated long-lived public requireUnique API probe before/after (8 synthetic tree nodes, fresh 64-KiB BSTR per identity/name read, warmup then 160 calls): baseline private-memory growth 174,198,784 bytes; repeat 174,186,496. Fixed growth 4,096 bytes; repeat 0. Managed memory stayed about 1.55 MB. Every run completed the expected traversal and results, with 1,408 AutomationId and 1,408 Name getters. This is an amplified deterministic allocation probe, not a claim about typical app string sizes. Also passed 90 existing matching/provider-failure/cancellation/nonactivating tests and the full NativeAOT/package build. Requested your reassessment and a fresh Copilot review; required CI is still running. Human approval remains separate from automated checks. |
There was a problem hiding this comment.
🟡 Changes recommended
Strict slug resolution can select the wrong element when distinct RuntimeIds collide in the four-character slug hash.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Complete strict slug matching before choosing a provider, applying predicates and scope before ambiguity detection. Preserve automatic first-match behavior and cover real 16-bit RuntimeId hash collisions without changing the slug protocol. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
App-scoped external slug resolution is incomplete, and non-stale COM failures are reported as stale elements.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 30/30 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Resolve source-less app-scoped external slugs across all windows before materialization, preserving collision rejection and recorded HWND boundaries. Distinguish explicit stale-element HRESULT from other provider errors without changing automatic behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Strict HWND resolution can still misreport non-stale provider failures as stale-element errors.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/winapp-CLI/WinApp.UIAutomation/Services/UiAutomationService.cs:1603
- What is wrong: Strict source-window binding still calls
GetRootElementForHwndwithrequireCurrentIdentity == false, whose catch-all at line 1830 converts every provider error intonull. Show me: a serialized element with a recorded HWND reaches this branch andElementFromHandlethrowsE_ACCESSDENIED-> the explicit overload reports the element as stale; expected: preserve the access/provider failure (onlyUIA_E_ELEMENTNOTAVAILABLEis stale). Why it matters: package and JSON callers receive the wrong recovery advice and lose the real failure. Smallest fix: make strict HWND binding suppress only the stale HRESULT, or let COM failures propagate so the command’s HRESULT filter can classify them.
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Use throwing HWND resolution for explicit identity checks and suppress only the element-unavailable HRESULT. Cover access-denied and stale source/target bindings without fallback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed the suppressed provider-error finding from review 5242333274 in 3c48167. Strict source/target HWND binding now uses the throwing root helper; only UIA_E_ELEMENTNOTAVAILABLE keeps the prior stale outcome. E_ACCESSDENIED and other provider failures retain the original COM exception, with no fallback or invocation. Four access-denied cases failed before the fix; all 24 targeted source-window/error tests now pass. Independent fix review found no concrete blocker, and full NativeAOT/package build passed. Requesting a fresh latest-head assessment; this is not a claim of reviewer approval. |
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-window UIA identity and COM-provider behavior warrants final human review despite successful builds and targeted test suites.
Review details
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Zach Teutsch (zateutsch)
left a comment
Zach Teutsch (zateutsch)
left a comment
There was a problem hiding this comment.
Clean on my end. Approving.
Preserve the incoming run --aot option alongside explicit UI action and defined string argument forwarding. Regenerate wrappers and npm docs from the merged schema without changing UI identity or ownership behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The broad UIA identity and cross-window behavior warrants final human approval despite comprehensive coverage and no remaining critical or high findings.
Review details
- Files reviewed: 30/30 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
|
Updated #846 to current main 19212c0 in 91f7775. Resolved the sole conflict by regenerating npm bindings, preserving both run --aot and defined-string forwarding. All prior UI identity, BSTR, and error fixes are unchanged. Local validation: 311 npm tests, 149 CLI/AOT tests, 118 focused UIA cases, full NativeAOT/package build, and generated-doc validation passed. Exact-head CI run 35304228665 completed successfully, including WinUI/coordination E2E; all 30 passing checks are green with none pending/failing, and GitHub reports MERGEABLE. The superseded 3c build had remained in progress for over four hours; it was cancelled after this new head was pushed, and GitHub did not provide its job logs. The last completed historical failure was a signing diagnostic assertion that received a different manifest error; its root cause cannot be proven from the available runner records. No speculative test weakening was applied. All 24 inline threads are resolved. Latest Copilot review 5244074262 reports no remaining critical/high findings but explicitly defers to human approval; that is not affirmative approval. Zach Teutsch (@zateutsch) your approval was on 3c48167; please reassess the current main-merge head when available. pr-review-done remains off pending the requested review gate. Not merged. |
main added deterministic explicit UI invoke actions (#846), which also changed the npm command generator (generate-commands.mjs now emits `!== undefined` guards). That regenerated winapp-commands.ts conflicted with this branch's project-mode packageApp options. Regenerated winapp-commands.ts, cli-schema.json, and npm-usage.md from the merged CLI so the wrapper combines main's generator style and new UI actions with the pack project-mode options. All other main changes auto-merged. Verified with tsc --noEmit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: dcabfb3c-9ea1-44c5-b91b-3b04f86a4289
Description
Adds
--action invoke|select|toggle|toggle-on|toggle-off|expand|collapsetowinapp ui invoke. Explicit actions use only their corresponding UIA pattern on the selected element, with no pattern or ancestor fallback. Explicit resolution also refuses to rebind a removed element to a same-name sibling.Toggle-on/off read first, leave an already-correct state untouched, and verify each transition. Initially indeterminate controls allow at most two transitions; other starting states allow one. Omitted
--actionpreserves automatic behavior.JSON includes
requestedActionandperformedAction, includingnonefor an idempotent no-op. The package API, npm forwarding surface, canonical docs, shipped skill, and generated schema are updated together.Usage Example
Related Issue
Fixes #821. Independent of #841, #842, and #844; no batching or persistent handles.
Type of Change
Checklist
Validation
scripts\build-cli.ps1 -SkipTestspassed: NativeAOT x64/arm64, npm, NuGet, MSIX, and generated docs.validate-llm-docs.ps1 -FailOnDriftpassed.scripts\build-cli.ps1was run using a session-local corporate NuGet mirror. It reached 5,440 CLI and 453 UI successes plus 63 analyzer successes. A missing new-type API allowlist entry was fixed and retested. Remaining unrelated failures: two recordingDirectoryNotFoundExceptioncases, x64 dump analysis under the ARM64 host, and a process-cancellation timing case (the latter passed in isolation).Additional Notes
Automated review and required CI gates are being monitored. This PR must not be merged by the agent.