Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 32 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
Reviewed at d11abc3. This has findings that need to be fixed before merge. assertLaunchEnvironmentSupport (packages/contracts/src/application-lifecycle-interaction.ts#L208) admits a device only when resolveDeviceAppleOs(device) === 'ios', but discovery classifies iPad simulators as appleOs 'ipados' (packages/platform-apple/src/inventory-classification.ts#L62). An iPad simulator user hits UNSUPPORTED_OPERATION even though the simctl path serves iPad simulators fine. The kernel already has the right predicate for this: isHandheldAppleSimulator (packages/kernel/src/device.ts#L120-L126). The openIosApp guard (app-launch.ts#L75) only checks kind === 'simulator' and would admit tvOS and visionOS simulators, so the two gates disagree with each other. One predicate should decide which leaf supports launch environment: use isHandheldAppleSimulator in both assertLaunchEnvironmentSupport and the openIosApp guard, and add an iPad simulator success row to the contracts test table. The gate at application-lifecycle-interaction.ts#L325 reads only device facts. A Limrun iOS device is built as {platform:'apple', appleOs:'ios', kind:'simulator'} (packages/provider-limrun/src/device.ts#L22-L26), so it passes the gate. openDirectApplication then calls the Limrun interactor, whose open(app, {url}) (packages/provider-limrun/src/ios.ts#L184) never reads launchEnvironment, so the app launches without it and the request still returns success. A caller asking for launch environment on a Limrun cloud iOS simulator gets a silent success with the environment missing, even though the PR body and docs say this case fails closed. Launch environment should be accepted only by the owner that applies it: reject a defined launchEnvironment with UNSUPPORTED_OPERATION in openDirectApplication/bindDirectApplicationLifecycle, which covers every direct and provider owner in one place, and add a Limrun lifecycle test that expects the typed error. This PR changes a device-facing simctl launch path (packages/platform-apple/src/core/app-launch.ts#L292), and the PR body says live simulator lanes were not claimed; every test here stubs runXcrun or the simctl provider, so nothing shows that a real simctl actually delivers the variables to the app process. Can we get a live run on a booted local iPhone simulator, and after f1 an iPad simulator too: Not blocking, can be taken or left: recording with --launch-env drops the flag silently on replay since sanitizeFlags only copies recorded keys (src/daemon/session-action-recorder.ts#L365); the three guards in app-launch.ts#L115/138/170 are unreachable now that line 75 throws first, and the message literal is duplicated instead of using one constant like LAUNCH_CONSOLE_IOS_SIMULATOR_ONLY_MESSAGE; three separate sites (Apple lifecycle.ts#L223, Android lifecycle.ts#L141-155, contracts openDirectApplication#L351-356) each keep their own launch-only field strip list and only the Apple one learned about launchEnvironment; the ios-world.ts#L1127 test helper only checks two hard-coded keys and the doesNotMatch assertion can't fail since open responses never echo flags, and app.test.ts checks .toThrow(regex) without asserting the INVALID_ARGS code; app.ts#L36 builds env objects as plain {} so a Could this be simpler by admitting launch environment only in the owner that applies it: the Apple lifecycle's local handheld-simulator path using isHandheldAppleSimulator, plus one rejection in openDirectApplication? With that, the device-fact gate in contracts and the four guards in openIosApp could go, along with the one-line contracts subpath, collapsing to one message constant. The host-kit envPatch field looks justified on its own, since without it a provider would receive the daemon's whole environment. Nothing needs to change first, since bindDirectApplicationLifecycle and isHandheldAppleSimulator both already exist. No live simulator run was done, so whether simctl actually delivers SIMCTL_CHILD_ vars to the app, and whether it restarts an already-running app without --relaunch, is unverified (launchArgs has this same open question). I did not trace whether MCP batch steps carrying launchEnvironment go through readLaunchEnvironment validation. I found no diagnostic that emits req.flags, so the redaction change looks defensive, but I could not enumerate every structured diagnostic producer. The behavior of out-of-tree appleToolProvider implementations that receive envPatch but don't handle it is unknown. CI shows one check and it's green, but this is a cross-repo PR and the Apple runner and live simulator lanes that would exercise app-launch.ts did not run. The launch-env gate needs to become owner-scoped: use isHandheldAppleSimulator so iPad simulators pass, and reject the environment in openDirectApplication so Limrun and other provider owners fail closed, then back it with a live simulator run showing the app actually receives the variable. |
|
One direction note on top of the review above. The overall shape looks right: Could you add one or two sentences to the PR body on why environment variables are needed here, rather than launch arguments? For Android, please don't add a second path in this PR. |
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Added the App Clip rationale to the PR description: _XCAppClipURL must be present in the launched process environment, which launch arguments/UserDefaults do not replace. Also updated --launch-env CLI help and the command docs to direct Android intent extras to --launch-args. The updated command-doc coverage test and typecheck pass. Live simulator delivery remains explicitly unverified on this host. |
|
Follow-up in 8971eba: refactored launch-environment redaction into focused helpers (redaction tests still pass), removed six stale Fallow suppressions, and stabilized the web-shutdown integration test by awaiting the direct child’s exit notification before checking PID release. The full |
…environment # Conflicts: # test/wire-compat/ledger.json
|
The code findings from d11abc3 are fixed in db28079. One finding remains, and it needs a live run. The change adds a device-facing simctl launch path. It passes SIMCTL_CHILD_* through envPatch on runXcrun and on runIosSimulatorConsoleLaunch (app-launch.ts#L284). Every test stubs runXcrun or the Apple tool provider, and the PR says live delivery is unverified. So nothing shows that a real Not blocking, and you can take or leave these: application-lifecycle-interaction.ts#L196 moves the launchConsole gate to isHandheldAppleSimulator, so The one reported check is green. The Apple runner and live simulator jobs are still action_required and did not run for this cross-repo PR, so the green check does not cover the simctl launch path. I did not run a live simulator, so I could not verify that simctl passes SIMCTL_CHILD_ variables to the app, with or without --relaunch. I also did not enumerate every emitDiagnostic producer, trace the daemon side for raw RPC or MCP requests that skip the client-side readLaunchEnvironment validation, or check how the merge resolved the wire-compat ledger entries and Fallow suppression removals from 8971eba. No conflicts. Before merge, please post the live iPhone and iPad run showing the app reads the nonce from its environment and the nonce is absent from the debug and event logs. |
This branch has not been deployed
2 participants
Summary
Validation
Based on upstream 0.21.15 / 16e0757.