Skip to content

fix(automations): remove confirmation can open with focus on body text, and Systems and Automations fixture checks fail during re-renders - #2146

Merged
bkudiess merged 2 commits into
openclaw:mainfrom
bkudiess:bkudiess-fixture-focus-ownership-flakes
Nov 13, 2026
Merged

bkudiess merged 2 commits into
openclaw:mainfrom
bkudiess:bkudiess-fixture-focus-ownership-flakes

Conversation

@bkudiess

@bkudiess bkudiess commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

Related: #2124, #2132

What Problem This Solves

Fixes: the Automations remove confirmation can open with keyboard focus on its body text instead of Cancel, and the Systems and Automations mock Gateway UI tests fail intermittently on main while those pages re-render.

User Impact

User impact: when you remove an automation, the confirmation now always opens with keyboard focus on Cancel, the safe choice it was designed to focus, instead of sometimes on the body text. Contributors also get fewer false CI failures in the "Mock Gateway application and compatibility tests" job.

Why This Change Was Made

Automations remove confirmation (product fix). Before this change, AutomationsRoot queued a low-priority "focus Cancel" call from the dialog's Opened event. WinUI runs its own initial-focus step, which picks the first focusable element (the selectable body text). Whichever ran last won, so focus sometimes stayed on the body. CI caught this as "Timed out waiting for Cancel receives destructive initial focus" (run 43323497743, attempt 2).

The new DialogInitialFocus helper redirects the first focus that enters the dialog to Cancel inside WinUI's own focus step, the same approach SystemsPageView already uses. When a dialog opens right after another closes, Opened and even the first focus can arrive before the Cancel button exists. The helper then retries after each layout pass until Cancel takes focus. Later focus moves are never redirected.

Systems and Automations UI tests (test fix). #2124 made these tests check that every UI element they find belongs to the app's own window. Some of those checks ran inside polling waits as one-shot asserts. While a page re-renders, an element found a moment earlier can be torn down before the ownership check finishes, so the check could not prove ownership and the test failed. It should have looked again.

Ownership now has three results:

  • Owned: proven to belong to the app's window.
  • Unproven: no contrary evidence, but not provable yet, for example a detached or vanished element. Polling waits keep polling.
  • Foreign: positive evidence of another process or window. This still fails immediately.

IsOwnedDescendant still returns true only for Owned, so the ownership guarantee from #2124 is unchanged. The Systems capture helper also re-finds its controls inside its wait, instead of reusing elements that a re-render may have replaced.

The changes add no retries of actions, sleeps, fixed settle counts, skips, or weaker assertions.

Evidence

  • Automations focus race, deterministic negative: with the new redirect disabled, so Cancel focus comes only from a queued call after Opened as before, the new DialogInitialFocusTests fails with "confirmation-0: first focus after Cancel was realized went to TextBox". With the fix it passes.
  • Opened can precede the dialog's controls: diagnostics showed Opened firing with zero visual children before the focus step; on the CI runner this happens even on the first open. Before the retry-until-realized change, the body kept focus in about 1 of 25 quick reopens. The test therefore never samples focus at Opened. It records every focus change through the dialog's own GettingFocus and opens 11 confirmations per run, each right after the previous one closed. For each one it requires:
    • the first focus inside the dialog once Cancel exists is Cancel;
    • Cancel keeps focus;
    • nothing else inside the dialog takes focus afterwards.
      It also requires that a later focus move to the body is not taken back.
  • Ownership rules: NativeUiOwnershipClassificationTests checks Owned, Unproven and Foreign deterministically over synthetic ancestry, including:
    • missing evidence never hiding foreign evidence;
    • partial reads keeping a foreign process id;
    • an owner changing identity mid-walk.
      The existing live two-window test now also asserts Foreign for every rejection case.
  • SystemsDaily root cause: CI receipts (run 43191056837 and others) and local instrumented runs showed the ownership walk transiently unprovable during re-render, plus ElementNotAvailableException while saving a capture of re-rendered stat tiles. The same pattern was reproduced locally in Automations TryPageScrollPeer ("page marker must belong", Dark theme).
  • Review: dual-model Hanselman review (Claude Opus 5.5 and GPT-6 Astra, high effort), three rounds. All consensus findings and all MEDIUM findings are fixed; see Real Behavior Proof for the open LOW notes.

Change Type

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

Scope

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

Required proof pools

  • none: the product change is limited to keyboard focus in the Automations confirmation dialog. It is proven by native in-process and mock Gateway UI tests that CI runs directly; no custom capacity-dependent Windows proof applies.

Validation

All runs were local on Windows x64 at 175% DPI, with isolated app and test profiles. Metadata for the real %APPDATA%/%LOCALAPPDATA%\OpenClawTray was unchanged after every run. Fixture runs used OPENCLAW_RUN_KEYBOARD_PROOF=1 and screenshots, matching CI.

Final source (hashes recorded before the commit; this commit is that source rebased onto origin/main ee8edbc with no conflicts):

Test or set Result
DialogInitialFocusTests + NativeUiOwnershipTests + NativeUiOwnershipClassificationTests (8 tests; the dialog test opens 11 confirmations per run) 10/10 runs passed on the final test (7f120bc)
Automations_ScopesToAgentsAndCoversMainStates (Light + Dark) 5/5 runs passed
SystemsDaily_InsetAndExpandedNativeFrames 3/3 runs passed (plus 5/5 and 5/5 on earlier review revisions with the same Systems code path)
Deterministic negative: redirect disabled, Cancel focus only from the queued call after Opened the final dialog test fails: "confirmation-0: first focus after Cancel was realized went to TextBox"

Full fixture UI suite (filter Category=GatewayFixture&KeyboardProof!=Required, run directly because the script's 1800 s cap is shorter than the ~31 min local run at 175% DPI), on the final source together with the separate pointer/capture PR's test change: 122/124 passed. The two failures are in tests this PR does not change:

  • Automations_HeroWashScrollsWhileTheBackplateStaysAnchored(Dark): "Timed out waiting for theme choice". The theme ComboBox did not open after a one-shot UIA Expand() in the unchanged SelectAppThemeAsync. A rerun passed 3/3.
  • ReconnectKeepsDefinitelyUnsentQueuedMessageFailedAndVisible: MCP app.chat.send "Request canceled". This test is unchanged from main, and it also fails on main's own test code locally (2 of 3 reruns). Reported separately as a pre-existing issue.

After rebasing onto origin/main ee8edbc (#2141):

  • .\scripts\validate-test-timing-patterns.ps1: "Test timing pattern ratchet passed." No baseline-lowering suggestion.
  • Rebuilt the app and UI tests. Affected tests: dialog and ownership 3/3, Automations 2/2, SystemsDaily 2/2.

Closeout on the rebased commit:

  • ./build.ps1: all builds succeeded.
  • dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore: 5849 passed, 0 failed, 29 skipped.
  • dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore: 6402 passed, 0 failed.

Real Behavior Proof

  • Environment tested: local Windows x64 desktop, 175% DPI, isolated app and test profiles, mock Gateway fixture.
  • PR head or commit tested: 7f120bc (on ee8edbc). Product and fixture changes are unchanged since 8b8c875. The repeat runs used the identical pre-rebase source, and the affected tests were rerun after the rebase. The dialog test was revised in 7f120bc and rerun 10/10 together with its negative.
  • Exact steps or command run: dotnet test tests/OpenClaw.Tray.UITests -c Debug -r win-x64 -p:DevBuild=false --no-build --filter <test>, with the fixture environment from scripts/test-gateway-fixture.ps1 (OPENCLAW_RUN_GATEWAY_FIXTURE_UI=1, OPENCLAW_RUN_KEYBOARD_PROOF=1, screenshots on).
  • Evidence after fix: native focus assertions in DialogInitialFocusTests, plus the Automations_ScopesToAgentsAndCoversMainStates remove step, which waits for UI Automation to report keyboard focus on the dialog's Cancel button.
  • Observed result: see Validation.
  • Screenshot or artifact links verified? (Yes/No/N/A): N/A (local artifacts only).
  • Not verified or blocked:
    • First-attempt CI on 8b8c875 (run 43368190358) failed only in this PR's own new dialog test. It sampled focus at Opened, and on the runner Opened fired before Cancel existed. 7f120bc fixes the test; the product code is unchanged. The Mock Gateway job, which includes SystemsDaily and the Automations remove step, passed on that run. CI for 7f120bc is pending.
    • Open one-model LOW review notes, not applied:
      • Reaching the ancestry depth limit returns Unproven, not a clear failure.
      • AssertUnchanged still runs after the Systems capture wait.
      • The focus helper keeps Pending while a dialog without a focusable Cancel stays open.

Security Impact

  • New permissions or capabilities? (Yes/No): No
  • Secrets or tokens handling changed? (Yes/No): No
  • New or changed network calls? (Yes/No): No
  • Command or tool execution surface changed? (Yes/No): No
  • Data access scope changed? (Yes/No): No
  • If any answer is Yes, explain the risk and mitigation: N/A

Compatibility and Migration

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

Review Conversations

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

…t, and Systems and Automations fixture checks fail during re-renders

The Automations remove confirmation queued its Cancel focus from Opened, which
raced WinUI's own initial-focus step and sometimes left focus on the selectable
body text. DialogInitialFocus redirects the first focus entering the dialog to
Cancel inside WinUI's focus step, retries after layout while Cancel is not yet
realized or focusable (a dialog opened right after another closed), and stops
on real user input.

openclaw#2124's ownership checks ran as one-shot asserts inside polling waits. While
Systems and Automations re-render, a just-found element can be torn down before
ownership is proven. NativeUiOwnership.Classify now returns Owned, Unproven or
Foreign: polls keep polling on Unproven and still fail immediately on Foreign.
IsOwnedDescendant remains Owned-only. Systems captures re-find their controls
inside the wait.

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

clawsweeper Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Nov 13, 2026
@clawsweeper

clawsweeper Bot commented Nov 13, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge.

What this changes

The PR directs Automations confirmation focus to Cancel and makes UI ownership polling tolerate temporarily unavailable controls.

Example: A user opens an automation’s Remove confirmation immediately after closing another confirmation.

  • Before: Keyboard focus can remain on the selectable confirmation body.
  • After: The dialog redirects initial focus to Cancel and retries when that button has not yet been realized.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The implementation appears correct and the reported regression coverage is relevant, but required shipped-behavior proof is absent.
Proof confidence 🧂 unranked krab (1/6) Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: An automation confirmation can initially focus body text instead of Cancel, and re-rendering pages can produce false UI test failures.
Reason: The collaborator-authored direction restores the ledger’s explicit focus contract and follows documented tri-state polling rules without adding a setting or changing deletion behavior.

Merge readiness

✅ Ready for maintainer review

Keep open: this collaborator-authored PR addresses a documented focus defect and distinct re-render failures still present on main; no concrete introduced code defect was found.

Priority: P2
Reviewed head: 8b8c875f74bf85ab103456d21a1ddc66a51ac86a

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: Provide current-head visible or diagnostic evidence from the shipped Automations confirmation showing Cancel initially focused after a quick reopen and ordinary later keyboard navigation preserved.
Agent review details

How this fits together

AutomationsRoot renders native confirmation dialogs, while the UI test infrastructure observes controls and captures windows using process identity and UI Automation ancestry.

flowchart TD
  A[Automation action] --> B[AutomationsRoot confirmation]
  B --> C[DialogInitialFocus]
  C --> D[Native Cancel button]
  E[Fixture UI tests] --> F[NativeUiOwnership]
  F --> G[Owned window capture]
Loading

Technical review

Best possible solution:

Retain the focused dialog repair and tri-state observation changes while preserving explicit Cancel focus and strict final ownership checks.

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

The original queued focus implementation and one-shot ownership assertions match the reported failure paths, and the author records a deterministic focus negative control; this read-only review ran no target code.

Is this the best way to solve the issue?

The patch preserves the existing owners and invariants: replace queued-only initial focus, distinguish unavailable observations from foreign controls, and reacquire controls inside capture waits; the helper’s native lifecycle boundary justifies its separate name and state.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against d1a5b4609690.

Provenance checked

  • Automations confirmation focus keeps the original intent (994539b: The native Automations feature established explicit safe confirmation focus and retained mutation safeguards.)
  • Native UI ownership classification keeps the original intent (2a23706: The earlier repair required verified native window ancestry while accommodating windowless XAML peers.)
  • Systems observations and owned-window captures keeps the original intent (b57f121: The Systems repair qualified captures against retained native windows and visible controls rather than accepting unrelated or incomplete frames.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Preflight source

Source unknown. Source comparison at the observed PR snapshot only. Completion is self-reported; body, policy, tests, execution, and report bytes are not verified.

Review metrics

Metric Value Why it matters
Production and test delta Production +111/-14; tests and test helpers +487/-57 Growth covers the documented late-realization focus path, distinct native reopen behavior, and ownership classification during re-renders.

Labels

Label changes:

  • add P2: The PR repairs limited keyboard-focus friction and intermittent contributor test failures without evidence of lost data or a blocked core workflow.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P2: The PR repairs limited keyboard-focus friction and intermittent contributor test failures without evidence of lost data or a blocked core workflow.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed November 13, 2026, 12:04 AM ET / 05:04 UTC.

…ened fires

CI showed ContentDialog.Opened firing before the Cancel template part is
realized even on the first open. Every confirmation now uses the
timing-independent check: the first focus inside the dialog once Cancel exists
is Cancel, Cancel keeps it, and no other element takes focus afterwards. The
"later focus is never stolen" proof focuses the body after Cancel holds focus
and requires it to stay.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@bkudiess
bkudiess merged commit 85c8ffa into openclaw:main Nov 13, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant