Repository navigation
fix: keep accessibility failure diagnostics private and fail-closed - #2054
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. What this changesThis PR removes private UI content from accessibility failure diagnostics and rejects incomplete scan results before exclusions or retries. Example: A ClickablePointOnScreen violation contains Name='private conversation' and AutomationId='private dynamic id'.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this collaborator-authored draft open: current main retains the diagnostic disclosure, and the focused patch addresses it without an identified code defect. Priority: P2 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherThe accessibility test helper receives Axe.Windows scan results from Companion's UI Automation tree and produces assertions consumed by test reports and CI diagnostics. flowchart LR
A[Companion window] --> B[Axe Windows scanner]
B --> C[Metadata validation]
C --> D[Rule exclusions]
D --> E[Scoped stale layout retry]
E --> F[Bounded failure report]
Technical reviewBest possible solution: Keep validation and privacy-safe reporting in AxeHelper, preserving existing exclusions and the single narrowly admitted retry. Do we have a high-confidence way to reproduce the issue? Current-main source directly prints UI names, IDs, arbitrary properties, and global focus on scan failure; no runtime reproduction was performed during this read-only review. Is this the best way to solve the issue? The patch fixes the reporting owner directly, preserves real violations, and uses the dependency's actual count contract without introducing another framework. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 96ea36fe6066. Provenance checked
TestingProof path: unit tests only. SecurityNone. EvidenceWhat I checked:
Likely related people:
Preflight sourceSource 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
LabelsLabel changes:
Label justifications:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale6/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. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed November 6, 2026, 8:40 PM ET / November 7, 2026, 01:40 UTC. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve keyboard traversal, settled collapse and selection retention while replacing the elapsed-label fixed sleep with observable rendered progress. Co-authored-by: bkudiess <7658216+bkudiess@users.noreply.github.com>
Current-head CI follow-upHead UI job aborted after 198 passed / 6 skipped because the testhost crashed. The last active case was The same broad hosted-crash symptom has been reproduced during separate local candidates. A bounded native diagnostic narrowed it to immediately before The current 40-case helper proof, source-bound native accessibility result and required build/unit validation remain retained. No assertion was disabled, no crash was relabeled as a passing run, and no rerun-until-green was requested. Investigation is separate from this PR's privacy/fail-closed behavior. |
What Problem This Solves
Fixes accessibility-test failures that can expose private UI content or crash while reporting incomplete scan metadata.
User Impact
User impact: no shipping UI change. Developers receive bounded diagnostics without UI names, data-derived automation IDs, arbitrary properties or unrelated desktop focus state. Malformed results remain failures, including excluded rules.
Published for review, not an unconditional merge-readiness claim. Current native validation and independent review are recorded below; new-commit CI and the requested Opus pass remain separate gates.
Why This Change Was Made
Keep rule IDs, bounded descriptions and canonical control types with scan-result ordinals. Print at most 20 violations while retaining the total count. Reject incomplete/inconsistent scan output before exclusions or retry. The existing one-time stale-layout retry now requires an automation ID and verifies target process ownership, rather than falling back to names.
Author-side simplification retained the existing helper and distinct data-only regressions; no new framework or production owner.
Evidence
Commit
788b3fb33240728d5c9be9e188f7e80153adb74chas tree26e5a88b500e62fc6ba70e9ff122c577c312c25a. All 2,175 intended source files were checked byte-for-byte against the retained native validation manifest and the committed Git blobs. Tests ran on the source-identical uncommitted candidate based on6825288f4123439dcb3b39750fae3d76115e675e; committing did not constitute another test run. Source-manifest SHA256:f5f20ddf9014a8fbbf96c4b9fddbad2213153f487067b51a7f2c91d636b4f1af.Current proof: 40/40 AxeHelper cases, 33/33 real accessibility cases, and the required build/unit suites passed. The shared test prerequisite corrects standalone-radio keyboard expectations, observes actual hidden content before collapsed navigation, and waits for rendered elapsed-text advancement without increasing the existing deadline. It adds no shipping UI or new framework.
Independent source and rubber-duck implementation/proof-plan reviews by GPT-6 Astra found no P0-P3 blocking findings in the frozen candidate. The actual role was a native OpenClaw subagent; context tier and reasoning effort were not independently attested. The requested Opus 5.5 pass was unavailable in this environment, so no renewed dual-model Hanselman consensus is claimed.
Receipts (Crabbox access may require team sign-in):
Important count distinction: the current hosted diagnostic recorded 683 passed / 7 skipped. Four skipped startup cases lacked the fixture opt-in in that launcher; a separate correctly admitted run passed 4/4. These are two runs, not one 687-pass/3-skip execution.
Historical evidence, not current validation
The earlier PR description reported 15 pure cases on
895ac1ab86505c72c5bc87abc90265c365dd1409and a source-guard negative control against baseline122bda71. Those results and the earlier shared-review authentication failure remain historical. The current 40-case helper suite and real native scans below supersede the old current-evidence claims. No newly induced native stale-layout incident is claimed.Change Type
Scope
winnodeRequired proof pools
windows-winui-interactive: real accessibility scanner integration and native keyboard/rendering prerequisites. The selected accessibility command passed 33/33; its inventory-bound summary hasscope=selected-test-execution-onlyandpoolComplete=false.Retry-policy tests use the real helper with controlled scanner/layout inputs. Real page scans are separate evidence; a newly induced native stale-layout retry incident remains unverified. Other installer, Gateway, MXC and hardware pools are not selected for this test-only change.
Validation
Windows 11 Pro 25H2 x64, .NET SDK 10.0.403, Debug/non-dev identity. All four
OPENCLAW_TRAY_*data roots were isolated;OPENCLAW_STATE_DIRwas unset only for unit-test processes and restored to task-private state for UI runs. Fresh test-project builds prevented--no-restoreno-ops. Source and loaded binaries were verified before and after execution../build.ps1dotnet build ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csprojdotnet build ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csprojdotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restoredotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restoredotnet build ./tests/OpenClaw.Tray.UITests/OpenClaw.Tray.UITests.csproj -c Debug -r win-x64FullyQualifiedName~AxeHelperTestsFullyQualifiedName~ClipboardExpander_SupportsKeyboardSelectionAndCollapseFullyQualifiedName~ActiveRun_AfterAssistantPreamble_KeepsClawAndElapsedTimeVisibleCategory=AccessibilityCategory!=Accessibility&Category!=NativeOnboardingProof&Category!=GatewayFixtureFullyQualifiedName~StartupWindowUiTestswith fixture opt-ingit diff --checkFocused UI commands used
dotnet test ./tests/OpenClaw.Tray.UITests/OpenClaw.Tray.UITests.csproj -c Debug -r win-x64 --no-build --no-restore --filter <selection>with fresh invocation-specific TRX directories. Real accessibility used:The hosted diagnostic used
--diag,--blame,--blame-hang --blame-hang-timeout 300s --blame-hang-dump-type none. The fixture opt-in omission and separate four-case correction are retained, not hidden. Three native-input/compositor opt-ins remain unexecuted in that selection. No fresh Release or signed-distribution result is claimed. New GitHub CI is not implied by these local results.Real Behavior Proof
788b3fb33240728d5c9be9e188f7e80153adb74c; all 2,175 source blobs match the validation manifest.Retained proof archive SHA256:
6cf300795e42e414be76847e875c8f6e337cd6e58c1d8f0b7ea92a9e6fda73eb(52 independently verified files, eight parsed TRX reports). Raw archives remain in the team work session, not represented as public attachments.Security Impact
Yes/No) No.Yes/No) No.Yes/No) No.Yes/No) No.Yes/No) Yes, narrowed in the test helper.Yes, explain the risk and mitigation: removes arbitrary UI property logging and unrelated desktop-focus reads. Remaining stale-layout lookup verifies process ownership and no longer falls back to UI names. Static test-authored context and rule descriptions remain bounded diagnostic inputs.Compatibility and Migration
Yes/No) Yes for shipping behavior; diagnostic text changes intentionally. Incomplete scan metadata now produces an explicit failure, and stale-layout retry requires an automation ID.Yes/No) No shipped changes.Yes/No) No.Contributor credit
Original PR by @bkudiess. Follow-up fixes and native validation were prepared with Barbara Kudiess; the follow-up commit retains her co-author credit.
Work sessions
Linked for team reference; these sessions aren’t public.
Review Conversations
Live inline-review-thread lookup before this update returned no threads for this PR. The original checklist remains unclaimed rather than inventing resolved conversations.