fix: support xctrace-only iOS physical devices - #1375
Conversation
Review: xctrace-only iOS physical-device supportReviewed the full diff (26 files, +745/−234). This is a clean, well-scoped feature. The CoreDevice path is refactored out verbatim into Overview
Findings1. [design, medium] Backend selection is process-global side-channel state, decoupled from I traced the reachable paths:
The deeper point is the latent invariant "discovery must precede command execution, in-process" that nothing enforces by type. Since the resolved 2. [maintainability, low] case .activate:
return Response(ok: true, data: DataPayload(message: "app activated"))This is correct only because
3. [dead-code, low] 4. [nit] Duplicated constants. 5. [nit] Things I checked that are fine
ValidationLive physical-device validation is still pending (noted in the description). Given the whole feature targets rarely-available iOS 15/16 hardware, I'd treat finding #1's injected-inventory gap and a real-device smoke of open → screenshot → terminate as the two things to close before relying on this in the field. |
|
P1: The new xctrace-only backend cannot reach its XCTest runner. |
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
|
Re-reviewed at Readiness is still blocked by the confirmed Fallow failure: |
|
Addressed the remaining review blocker in Also repeated the live source-built smoke on unlocked |
|
Re-reviewed exact head The remaining blocker is practical validation: the |
|
Addressed the final readiness comment. The maintainer explicitly chose to undraft this PR after the lack of iOS 15/16 hardware and the resulting residual validation risk were disclosed; treating that as acceptance of the documented risk rather than claiming live xctrace/usbmux evidence we do not have. The fallback path remains covered by the end-to-end fake-usbmuxd regression, the available CoreDevice physical-device flow passed open → selector interaction → screenshot → close, every CI check is green, and |
|
Coordinator correction: removing |
|
Acknowledged the coordinator correction. |
|
Branch readiness update: exact head 293e902 is now reported CONFLICTING with current main after recent merges. The reviewed code head has not changed, so this is an owner-action branch blocker rather than a new code finding. Rebase onto current main, resolve the conflict, and let checks rerun before readiness can be reassessed. |
293e902 to
9f52c6a
Compare
|
Review at Two contract/docs gaps also need resolution: #1361 explicitly requires backend selection through The branch conflict is resolved and all current checks are green, but this code finding blocks readiness. After fixing it, the new path still needs a real xctrace-only iOS 15/16 open → interaction → screenshot → close run, or explicit maintainer acceptance of that residual risk. |
|
Addressed the review at ec311d0. Default physical-device screenshot dispatch now goes through IosPhysicalDeviceControl, and the xctest backend captures inline through the runner without touching devicectl; the regression exercises the public Apple interactor path and asserts no devicectl invocation. App inventory/install, process lookup, recording artifact copy, and physical-device console probes were also moved behind the physical-device/CoreDevice adapters. The remaining devicectl references are confined to those adapters, device discovery (which selects CoreDevice vs xctrace), the low-level devicectl module, and the Apple tool provider. Backend-specific command admission now flows through src/core/capabilities.ts: apps/install/reinstall/install-from-source/logs/perf/record reject xctest-only devices with a CoreDevice hint, while open/close/snapshot/screenshot remain supported. CLI help and command docs describe the limitation. Validation: full affected suite passed; final chained format, typecheck, lint, Fallow, build, and 148 focused tests passed. |
|
Follow-up: the first CI pass caught a macOS smoke regression in the shared runner screenshot helper: macOS host targets also have kind=device, so they were accidentally sent through physical-iOS control. Fixed in 1d1d0ca by excluding macOS before physical-device dispatch, with a regression proving host runner screenshots copy directly and never invoke devicectl. The focused screenshot suite now passes 101 tests locally. |
|
P1 — macOS runner screenshots regress at this head. Also retain the acceptance risk: #1355/#1361 require the xctrace-visible, devicectl-absent iOS 15/16 path. The modern CoreDevice smoke plus fake-usbmux unit coverage do not live-validate launch + usbmux + inline screenshot + interaction + close on a legacy device; require that scenario before merge-readiness, or explicitly carry it as residual risk. |
|
Coverage also caught the two strict capability-parity oracles missing the new backend-specific keys. Updated both independent reference matrices (including an xctest DeviceInfo fixture) in ed78e68; the exact parity tests now pass 20/20 locally. |
ed78e68 to
bd9d4f7
Compare
|
Rebased onto current |
|
Re-reviewed exact head One readiness blocker remains: the PR still has no live evidence on the actual xctrace-visible, devicectl-absent iOS 15/16 route required by #1355/#1361. Fake-usbmux and capability tests are good contract coverage, but they do not replace an actual legacy-device |
|
we don't have that device for testing, so we'll ship it and see from the live adoption |
Summary
Support xctrace-only iOS 15/16 physical devices through a small backend seam while keeping CoreDevice as the default path.
DeviceInfodiscriminant without exposing it publicly.Closes #1361
Closes #1355
Validation
pnpm check:affected --base origin/main --run(format, lint, typecheck, layering, fallow, and build passed; nine unrelated concurrent timeout cases passed in isolated reruns)pnpm test:coverage --maxWorkers=4(540 files; 4,568/4,569 passed, with the sole unrelated timeout passing immediately in isolation; all provider-integration tests passed)pnpm build:xcuitestthymikee-iphonesmoke with the source-built CLI: opened Settings, resolved and tappedid="com.apple.settings.general", settled on General, found the hittable Software Update row, and captured a post-interaction screenshot; session closed cleanly