Conversation
There was a problem hiding this comment.
All reported issues were addressed across 41 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
Findings only, at 426941e. packages/platform-apple/src/inventory-classification.ts:38 (https://github.com/callstack/agent-device/blob/426941e/packages/platform-apple/src/inventory-classification.ts#L38): isSupportedAppleRuntime now accepts watchOS runtimes, so parseSimctlAppleDevices emits watch rows with target 'mobile', and appleDeviceSelectionRank gives them phoneRank 0 (packages/kernel/src/device.ts#L588-L603), so matchesPlatformSelector('ios') keeps them. After src/daemon/handlers/session-state.ts:259 (https://github.com/callstack/agent-device/blob/426941e/src/daemon/handlers/session-state.ts#L259): the pair-wearable branch builds resolveCommandDevice flags only from input.phone and drops req.flags, so iosSimulatorDeviceSet and androidDeviceAllowlist never apply; the phone is looked up in the default simulator set, scopeSimctlArgsForDevice(phone) then scopes watch listing, pair, boot and unpair to that default set, and on Android discover() (packages/platform-android/src/wearable-pairing.ts#L77) lists every AVD and serial with no allowlist. A tenant- or lab-scoped caller can boot, pair, or terminate devices outside its declared scope, and with a custom simulator set the phone lookup fails with DEVICE_NOT_FOUND. Every device the command touches, phone and wearable, needs to resolve inside the request's isolation scope: merge req.flags with the phone udid/serial into resolveCommandDevice, and pass the allowlist and simulator set into wearable discovery. packages/platform-apple/src/wearable-pairing.ts:32 (https://github.com/callstack/agent-device/blob/426941e/packages/platform-apple/src/wearable-pairing.ts#L32): packages/platform-apple/src/wearable-pairing.ts:89 (https://github.com/callstack/agent-device/blob/426941e/packages/platform-apple/src/wearable-pairing.ts#L89): packages/platform-android/src/wearable-pairing.ts:48 (https://github.com/callstack/agent-device/blob/426941e/packages/platform-android/src/wearable-pairing.ts#L48): with boot:false and a stopped Wear AVD row (discover includes stopped devices), Not blocking: the rollback test seeds no pre-existing pair and doesn't cover boot/shutdown, cancellation, or the disconnected state (packages/platform-apple/src/wearable-pairing.test.ts#L57), parseWatchDevices duplicates parsing that listAppleSimulators already does (packages/platform-apple/src/wearable-pairing.ts#L122), the Proxy spread in the CLI test silently drops methods instead of throwing (src/tests/cli-client-commands.test.ts#L1171), the daemon's manual input validation silently drops non-string fields instead of rejecting them (src/daemon/handlers/session-state.ts#L460), and the change is +1155/-19 against the 1,000-line budget in docs/agents/pull-requests.md, so any of these can be taken or left. Is an Apple-only pair-wearable, selecting the watch from the existing listAppleSimulators inventory without a --boot path, simpler than what's here: drop the boot/rollback machinery entirely (unpair against a pre-request snapshot is the only undo needed), drop the Android half since it only returns a fixed string and a synthetic pairId today, and would that roughly halve the production diff? The PR body says no live run was done. To validate: run The packet reports one check and it is green, but on this cross-repo head the Integration Tests and Coverage jobs that cover the new provider scenarios and the daemon route are not reported, and no failing job overlaps the diff. I did not run simctl locally to confirm the exact |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 18 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 18 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Wearable pairing follow-up pushed in f57809b (including a sync with current upstream main). This addresses the review points in the current patch: iOS phone selection preserves simulator-set/request isolation flags; Android Wear discovery is constrained by the daemon-derived serial allowlist on both initial and refreshed inventory; a watch already in Booting is only waited on and is not later claimed/shut down as request-owned; and failure/cancellation rollback snapshots existing CoreSimulator pairs, removes every newly created pair, and restores the pre-request active pair. Added regression coverage for disconnected-but-active status, Booting ownership, cancellation rollback, and Android discovery scoping. Also split Android wearable selection/probing to pass Fallow and fixed the existing PID-exit race in the Node integration suite. Verification: focused wearable/handler tests passed (30 tests); full |
|
Thanks for the update. I reviewed f57809b against the earlier findings (#3002 (comment)). Some are fixed, but the Apple rollback still touches devices this request does not own, and the live runs are still missing. The rollback in wearable-pairing.ts lists every pair in the shared CoreSimulator set. It unpairs any pair missing from the snapshot, whichever phone and watch it joins (lines 136-148). It then re-activates every pair that was active before the request, for all phones (lines 149-160). Other daemons, Xcode, and manual simctl calls use the same set. So a pair that another actor creates or switches for a different phone between the snapshot and the failure can be unpaired or reverted. The new test 'cancellation after simctl pair' asserts that pair_activate runs on 'unrelated-active-pair', which locks in this over-reach. The same ownership rule is half-applied at line 41: bootedHere is set before The new path drives real devices through simctl and adb (boot, pair, pair_activate, unpair, emulator launch and terminate), but the only proof is stubbed-host unit tests with hand-written
The earlier question is still open: would it be simpler to ship Apple-only pairing with a snapshot-scoped unpair and drop the Android half? That half grew by about 214 lines in this update, and its result is still a fixed human-step string with a synthetic pairId. Could you say why it must ship in this PR, or split it into a follow-up? Not blocking, and you can take or leave it: androidWearablePairingFact refuses only TV targets, so a Wear emulator (target mobile) is still accepted as the phone. You could refuse a phone whose probe shows the watch feature. The one reported check passes. The Integration Tests and Coverage jobs, which would run the provider scenarios and the daemon pair-wearable route, are not reported on this cross-repo head. No failing job overlaps the diff, and there are no conflicts. I did not run simctl, so the real Before merge, please limit the Apple rollback (unpair, re-activate, shutdown) to effects this request caused on its own phone and watch, then share the live simulator and emulator runs. |
Summary
devices.pairWearable()typed client command with matching CLI, MCP, daemon registry, capability facts, and structured unsupported-operation behaviorhuman-step-requiredreporting when ADB transport exists but companion pairing is not yet provensimctlor ADB commandsContract
The result distinguishes
connected,paired, andhuman-step-required. Android does not claim pairing success from ADB connectivity alone.Verification
pnpm check:affected --runThe repository gate skipped GitHub-authoritative jobs locally as designed. No live hardware or simulator pairing green is claimed here; live device proof remains an operator acceptance step.