Skip to content

perf(ios): skip the simulator accessibility write when this host's preferences already allow it - #4300

Merged
thymikee merged 3 commits into
mainfrom
perf/sim-accessibility-guard
Nov 7, 2026
Merged

thymikee merged 3 commits into
mainfrom
perf/sim-accessibility-guard

Conversation

@thymikee

@thymikee thymikee commented Nov 7, 2026 •

Copy link
Copy Markdown
Member

Summary

#4276 made every iOS simulator open run simctl spawn … defaults write com.apple.Accessibility ApplicationAccessibilityEnabled -bool YES before the launch, which costs about 0.85–1.05 s (a simctl spawn read costs the same, ~0.9 s). The maintainer review on #4276 flagged it.

The launch guard now reads the simulator's preferences file from this host (<set>/<udid>/data/Library/Preferences/com.apple.Accessibility.plist, parsed with the existing parseApplePlist, ~0.3 ms warm) and writes only when the key is explicitly off. It skips the write when the key is on, 1, or absent (a never-used simulator has no key and its apps expose a full tree). A missing or unreadable file, or a scoped tool provider (Limrun), still writes. Teardown repair stays unconditional.

Race accepted: after XCTest teardown the file lags the guest by under 200 ms (measured: +0 ms disk 1 / guest 0; +200 ms both 0). Our own teardown repairs the key; only a foreign teardown within that window is missed.

3 files. Design reviewed with Codex.

Validation

  • Tested SHA: dbf6f508726424a703fa1cd2cedb97dc98a93d5b; pnpm check:affected --run --head dbf6f508726424a703fa1cd2cedb97dc98a93d5b passed. Timing runs below were taken at 2909056; later commits only add tests and move the host path lookup inside the guard's try.
  • New ios-simulator-accessibility.test.ts: skip on on/1/absent; write on off/0/malformed/missing; re-read on every launch.
  • Live, fresh iPhone 17 simulator (iOS 26.2), isolated state and lease dirs, key = 1, 5× open --relaunch: main 3188/2407/3458/2566/2703 ms, this PR 2898/1320/1134/1181/1185 ms (first run includes runner startup).
  • Live, key forced to NO on disk: open --relaunch wrote it back to 1 before launch.
  • Live, absent key (fresh iPhone 17 simulator, iOS 26.2; plutil -p showed the preferences file without ApplicationAccessibilityEnabled): open --relaunch left the key absent (no write), AXe read 155 nodes, and the first snapshot -i had 29 nodes. A key=1 run on the same simulator gave 155 and 29.

@github-actions

github-actions Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 6.17 MB 6.17 MB +549 B
Package (unpacked) 6.17 MB 6.17 MB +549 B
Package (download) 1.87 MB 1.87 MB +230 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 18.5 ms 18.4 ms -0.1 ms
CLI --help 40.3 ms 38.8 ms -1.6 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

View guided diff | Turn on auto-fix | Re-trigger cubic

@apex-sentinel-staging

apex-sentinel-staging Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Apex Sentinel

Verdict: approve

Since the last review: 0 new · 0 still open · 1 no longer raised

No findings.

Not in the reviewers' prompt (2 files)
  • packages/platform-apple/src/core/__tests__/fixtures/accessibility-false.plist: no text diff (a binary, empty or mode-only change)
  • packages/platform-apple/src/core/__tests__/fixtures/accessibility-true.plist: no text diff (a binary, empty or mode-only change)

Reviewed with this repository's own Sentinel configuration (0e7df74a1ad7).

Feedback goes in the review comment threads, as a reply under the inline comment of a finding:

Reply @apex-sentinel-staging note ... to add context, or @apex-sentinel-staging pivot ... if it is wrong — I will re-check the cited code.

Useful? Reply @apex-sentinel-staging like, or @apex-sentinel-staging dislike <why> if it is not.

Comment @apex-sentinel-staging review on this conversation to run a full review.

Earlier reviews

Review of d64b5e42 · 07.11.2026 08:23 UTC

Apex Sentinel

Verdict: approve

Since the last review: 1 new · 0 still open · 1 no longer raised

1 finding(s): 1 minor.

🟡 Minor (1)

Where Finding
packages/platform-apple/src/core/ios-simulator-accessibility.ts:69 🆕 Guard treats an unrecognised or unreadable preferences plist as "enabled" and skips the write

1 finding below minor not shown (user_settings.min_severity).

Not in the reviewers' prompt (2 files)
  • packages/platform-apple/src/core/__tests__/fixtures/accessibility-false.plist: no text diff (a binary, empty or mode-only change)
  • packages/platform-apple/src/core/__tests__/fixtures/accessibility-true.plist: no text diff (a binary, empty or mode-only change)

Reviewed with this repository's own Sentinel configuration (83945b7a9f40).

Review of 2909056b · 07.11.2026 08:09 UTC

Apex Sentinel

Verdict: request_changes

1 finding(s): 1 major.

Required fixes

  • Guard never skips on a real simulator: parseApplePlist cannot read binary plists

🟠 Major (1)

Where Finding
packages/platform-apple/src/core/ios-simulator-accessibility.ts:70 Guard never skips on a real simulator: parseApplePlist cannot read binary plists

1 finding below minor not shown (user_settings.min_severity).

Reviewed with this repository's own Sentinel configuration (83945b7a9f40).

@apex-sentinel-staging apex-sentinel-staging Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apex Sentinel: 1 new finding on 2909056. The verdict and the full list are in the summary comment.

@apex-sentinel-staging apex-sentinel-staging Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apex Sentinel: 1 new finding on d64b5e4. The verdict and the full list are in the summary comment.

@thymikee

thymikee commented Nov 7, 2026

Copy link
Copy Markdown
Member Author

I found no code defects at d64b5e4, and CI is green (17 checks, 0 not passing). I need one more live run before merge: the absent-key case. This PR also skips the guest write when the preferences file exists but has no ApplicationAccessibilityEnabled key, and the live runs only cover key=1 (skip) and key forced NO (write). If an absent key gives a sparse first tree, every simulator that has booted once without the repo's write or an XCTest runner session regresses from the guaranteed write in #4276. Please run open <app> --relaunch on a simulator whose com.apple.Accessibility.plist exists without the key (check with plutil -p; plutil -remove ApplicationAccessibilityEnabled on a shut-down simulator if needed), then snapshot -i. The output should show the same full node count as the key=1 run and no simctl spawn ... defaults call. Add it to the PR body. If the tree is sparse, treat an absent key as unknown and write. The live numbers in the PR were taken at 2909056, and d64b5e4 only added test fixtures. I did not run the simulator path or the unit tests here, so I rely on the CI result and on reading the code.

Not blocking, and you can take or leave these: the hasScopedAppleToolProvider() early return in ios-simulator-accessibility.ts has no test, so add a scoped-provider case with an on-key plist that asserts the write, or drop the branch since the missing-file path already covers it; simulatorAddressFor, hostHomeDirectory and path.join run before the try, so a throwing os.homedir() could reject the prepare step and block a launch, so move them inside the try; and the test casts contents as Parameters<typeof build>[0] and the node:path import sits between workspace imports, so type the record with the plist value type and reorder the imports.

The two earlier inline threads, #4300 (comment) and #4300 (comment), no longer apply and can be resolved. The first claimed bplist00 cannot be parsed, but the binary fixtures and their skip assertions only pass if the bytes parse. The second claimed an unreadable plist counts as enabled, but the code returns unknown for a null document and for any throw. The absent-key question above is the remaining real concern.

@thymikee

thymikee commented Nov 7, 2026

Copy link
Copy Markdown
Member Author

Absent-key run done, at d64b5e4. I used a fresh iPhone 17 simulator (iOS 26.2), the PR build, and isolated state and lease dirs. plutil -p showed com.apple.Accessibility.plist without ApplicationAccessibilityEnabled.

  • open <app> --relaunch: the key was still absent afterwards, so no guest write happened.
  • AXe right after: 155 nodes.
  • First snapshot -i: 29 nodes.
  • On the same simulator with key=1, the results were 155 and 29.

An absent key exposes the full tree, so it stays a skip. I added this to the PR body.

The non-blocking items are in dbf6f50:

  • The scoped-provider branch has a test: an on-key plist under withAppleToolProvider still writes.
  • simulatorAddressFor, hostHomeDirectory and path.join moved inside the try, so a throwing home lookup falls back to the write and never rejects the launch.
  • The test types its record as Record<string, PlistValue> with no cast, and node:path imports come first.

pnpm check:affected --run --head dbf6f50872 passed. Both earlier Apex threads are resolved.

@thymikee

thymikee commented Nov 7, 2026

Copy link
Copy Markdown
Member Author

I found no code problems at dbf6f50, and the points from my earlier review (#4300 (comment)) are now covered by the live skip run and the binary-fixture tests. I read the head files, the delta diff and your run output. I did not run the simulator or the unit tests, and I did not decode the binary plist fixtures or reproduce the timing numbers. The absent-key run covers iOS 26.2 on an iPhone 17 simulator only, so other OS versions with an absent key are not shown.

The two earlier inline threads no longer apply: the binary-plist claim was withdrawn and your binary-fixture tests answer it (#4300 (comment)), and the lookup now sits inside the try (#4300 (comment)). Please resolve both.

Of the 17 checks, 16 pass, including check:affected. Smoke Tests is still running (run 42377141314) and has not failed. This change touches the launch guard on the iOS simulator open route, so if that job fails, please check whether it is an iOS simulator open and look for the ios_simulator_accessibility_prepare_failed diagnostic. No conflicts are known. Before merge, Smoke Tests needs to finish green.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Nov 7, 2026
@thymikee
thymikee merged commit 8dbf4b1 into main Nov 7, 2026
17 checks passed
@thymikee
thymikee deleted the perf/sim-accessibility-guard branch November 7, 2026 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant