Repository navigation
Conversation
|
@muvaf is attempting to deploy a commit to the Callstack Team on Vercel. A member of the Team first needs to authorize it. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
18 issues found across 183 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/platform-apple/src/core/plist.ts">
<violation number="1" location="packages/platform-apple/src/core/plist.ts:68">
P1: `plist` is only a devDependency of `@agent-device/platform-apple`, but this new runtime import loads it in production. Move `plist` to `dependencies` so installed packages can resolve it; otherwise `readLimrunAppInfo` fails with a module-not-found error.</violation>
</file>
<file name="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Lifecycle.swift">
<violation number="1" location="apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Lifecycle.swift:130">
P2: This fixes TMPDIR for screenshots only. `resolveRecordingOutPath` still uses `NSTemporaryDirectory()` on iOS, so device recordings can be written outside the container while the host reads `tmp/<name>`. Resolve the recording output under `NSHomeDirectory()/tmp` on iOS too, using the same helper.</violation>
</file>
<file name="packages/platform-apple/src/core/simctl-output.ts">
<violation number="1" location="packages/platform-apple/src/core/simctl-output.ts:3">
P3: The doc comment says this throws unless the input is a JSON object, but non-object JSON such as `[]`, `5`, or `"x"` returns `{}` without throwing. Reword it to say it throws on unparseable JSON and returns `{}` when no `devices` key is present.</violation>
</file>
<file name="packages/daemon/src/sessions/session-lifecycle/internal/session-close-lifecycle-teardown.ts">
<violation number="1" location="packages/daemon/src/sessions/session-lifecycle/internal/session-close-lifecycle-teardown.ts:86">
P2: The new `inspectSessionLeaseRetainedOnClose` call runs before any teardown and outside the `attemptCleanup` and `releaseProviderLeaseForClose` error handling. If the registry lease fails the owner or scope check, the throw now aborts close before the app, runner, and runtime hints are cleaned up. Previously that error surfaced after teardown as a retriable response. Consider catching the error here, or moving the retention check after teardown.</violation>
</file>
<file name="packages/apple-runner/src/runner-transport.ts">
<violation number="1" location="packages/apple-runner/src/runner-transport.ts:117">
P1: The forwarding-unavailable error is thrown without dispatch disclosure, so a 502/503 after the forwarder wrote the command reads as a pre-send failure. The connect loop then re-POSTs it because this error is `connectRetry: true`, which can replay a mutation such as a tap. Disclose `dispatched: 'unknown'` here, and make sure the connect loop does not resend a mutation on this error.</violation>
</file>
<file name="packages/platform-apple/src/core/runner-client.ts">
<violation number="1" location="packages/platform-apple/src/core/runner-client.ts:16">
P3: This re-export block adds host-free helpers and types (e.g. `RUNNER_STARTUP_TIMEOUT_MS`, `callerDeadlineExpired`, `mintRunnerToken`, `runnerListenerEnv`, `RUNNER_XCTESTRUN_CAPTURE_OPTIONS`). The header comment above says host-free helpers and types should come from the package façade directly. Either drop these names from this entry and import them from `@agent-device/apple-runner`, or update the header comment so the rule matches what the file exports.</violation>
</file>
<file name="scripts/package-ios-simulator-runner.sh">
<violation number="1" location="scripts/package-ios-simulator-runner.sh:98">
P2: The interop library and `_Testing_*` framework copies are optional here, so a packaging run on an Xcode without them still succeeds and publishes an archive that cannot load on a remote simulator. Fail the packaging step when the required library is missing, and create `Frameworks/` before copying into it.</violation>
</file>
<file name="packages/provider-limrun/src/deployment-runtime.ts">
<violation number="1" location="packages/provider-limrun/src/deployment-runtime.ts:49">
P3: The session-liveness predicate is now written out twice, and the helper that owned its contract and doc comment was removed. Restore a single `isActiveLimrunRuntimeSession(options, device)` helper and call it from both places so the fallback rule lives in one spot.</violation>
</file>
<file name="packages/platform-apple/src/core/tool-provider-types.ts">
<violation number="1" location="packages/platform-apple/src/core/tool-provider-types.ts:9">
P2: `requestCancellation: 'ignore'` does not stop cancellation if a caller also passes `signal`, because the Limrun `operation()` still combines `options.signal`. Add `signal?: undefined` to the `'ignore'` branch so the type enforces the guarantee in the doc comment.</violation>
</file>
<file name="src/core/interactors/limrun-ios-runner.ts">
<violation number="1" location="src/core/interactors/limrun-ios-runner.ts:413">
P2: If `finalizeRunnerSessionsForShutdown()` throws, `lifetime.abort()` and the staging cleanup never run. Wrap the finalize call in try/finally so the abort and `fs.rm` still happen.</violation>
</file>
<file name="packages/platform-apple/src/core/local-simulator-files.ts">
<violation number="1" location="packages/platform-apple/src/core/local-simulator-files.ts:37">
P2: The container lookup runs even when `sourcePath` is absolute, and then the absolute copy never uses the container path. If `get_app_container` fails for the bundle, the copy throws before it tries the absolute host path. Resolve the container only when `sourcePath` is relative.</violation>
</file>
<file name="packages/provider-limrun/src/facts-runtime.ts">
<violation number="1" location="packages/provider-limrun/src/facts-runtime.ts:43">
P3: The new message gives no reason for the refusal, and the comment that explained the captured-tree rationale was removed. Restore the original message so Android element-text refusals stay actionable.</violation>
</file>
<file name="src/core/interactors/limrun-ios-files.ts">
<violation number="1" location="src/core/interactors/limrun-ios-files.ts:118">
P2: The memoized `readLimrunAppInfo` promise is bound to the first caller's abort signal and timeout. A second concurrent caller for the same bundle inherits that cancellation or short budget. Key the cache on the bundle alone and detach each caller's signal, or retry with the caller's own signal when the shared promise rejects.</violation>
</file>
<file name="packages/platform-apple/src/core/screenshot.ts">
<violation number="1" location="packages/platform-apple/src/core/screenshot.ts:373">
P3: When the runner screenshot is missing, this loop runs `simctl get_app_container` once per bundle id and candidate path pair, because the container path is cached only after a successful copy. Resolve the container once per bundle id and try the candidate paths against it, as the removed `tryCopySimulatorRunnerScreenshot` did.</violation>
</file>
<file name="src/core/interactors/limrun-ios-runner-products.ts">
<violation number="1" location="src/core/interactors/limrun-ios-runner-products.ts:145">
P3: Each launch re-reads and hashes the entire runner tree to send only `Runner.xctestrun`. Hash the files once during preparation and reuse the digests, or stream-hash with `createReadStream` instead of `readFile`.</violation>
<violation number="2" location="src/core/interactors/limrun-ios-runner-products.ts:190">
P2: Check `response.ok` before parsing JSON. A non-JSON error body currently surfaces as a SyntaxError, and the HTTP status is lost.</violation>
</file>
<file name="packages/platform-apple/src/core/app-settings.ts">
<violation number="1" location="packages/platform-apple/src/core/app-settings.ts:307">
P2: The Limrun provider reuses this 15-second timeout for listing, every removal, and directory recreation, so a large container can abort partway through clearing. Give container mutations per-operation deadlines or budget the entire clear explicitly.</violation>
</file>
<file name="packages/platform-apple/src/launch-confirmation.ts">
<violation number="1" location="packages/platform-apple/src/launch-confirmation.ts:286">
P2: Provider-owned iOS simulators leave `resolveUrlOwner` unset, so this fallback queries local `simctl` for a remote Limrun device when an Open-in alert appears. Route the URL-owner lookup through the provider instead; otherwise the confirmation remains unanswered.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
|
||
| /** Parses downloaded app metadata on hosts without Apple's plist tools. */ | ||
| export async function parseApplePlist(bytes: Uint8Array): Promise<Record<string, unknown> | null> { | ||
| const { parse } = await import('plist'); |
There was a problem hiding this comment.
P1: plist is only a devDependency of @agent-device/platform-apple, but this new runtime import loads it in production. Move plist to dependencies so installed packages can resolve it; otherwise readLimrunAppInfo fails with a module-not-found error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/platform-apple/src/core/plist.ts, line 68:
<comment>`plist` is only a devDependency of `@agent-device/platform-apple`, but this new runtime import loads it in production. Move `plist` to `dependencies` so installed packages can resolve it; otherwise `readLimrunAppInfo` fails with a module-not-found error.</comment>
<file context>
@@ -62,3 +62,13 @@ function readXmlPlistString(plist: string, key: string): string | undefined {
+
+/** Parses downloaded app metadata on hosts without Apple's plist tools. */
+export async function parseApplePlist(bytes: Uint8Array): Promise<Record<string, unknown> | null> {
+ const { parse } = await import('plist');
+ const binary = new TextDecoder().decode(bytes.subarray(0, 8)) === 'bplist00';
+ const value = parse(binary ? bytes : new TextDecoder().decode(bytes));
</file context>
| throw new AppError('COMMAND_FAILED', 'Runner HTTP forwarding is unavailable', { | ||
| reason: 'runner_forwarding_unavailable', | ||
| status: response.status, | ||
| }); |
There was a problem hiding this comment.
P1: The forwarding-unavailable error is thrown without dispatch disclosure, so a 502/503 after the forwarder wrote the command reads as a pre-send failure. The connect loop then re-POSTs it because this error is connectRetry: true, which can replay a mutation such as a tap. Disclose dispatched: 'unknown' here, and make sure the connect loop does not resend a mutation on this error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/apple-runner/src/runner-transport.ts, line 117:
<comment>The forwarding-unavailable error is thrown without dispatch disclosure, so a 502/503 after the forwarder wrote the command reads as a pre-send failure. The connect loop then re-POSTs it because this error is `connectRetry: true`, which can replay a mutation such as a tap. Disclose `dispatched: 'unknown'` here, and make sure the connect loop does not resend a mutation on this error.</comment>
<file context>
@@ -102,16 +102,24 @@ export async function postRunnerCommandOverRoute(
);
+ if (endpoint.http && [502, 503, 504].includes(response.status)) {
+ await response.body?.cancel();
+ throw new AppError('COMMAND_FAILED', 'Runner HTTP forwarding is unavailable', {
+ reason: 'runner_forwarding_unavailable',
+ status: response.status,
</file context>
| throw new AppError('COMMAND_FAILED', 'Runner HTTP forwarding is unavailable', { | |
| reason: 'runner_forwarding_unavailable', | |
| status: response.status, | |
| }); | |
| throw discloseDispatch( | |
| new AppError('COMMAND_FAILED', 'Runner HTTP forwarding is unavailable', { | |
| reason: 'runner_forwarding_unavailable', | |
| status: response.status, | |
| }), | |
| 'unknown', | |
| ); |
| #if os(iOS) | ||
| // A managed XCTest host can override TMPDIR outside the app container. | ||
| // The returned tmp/ path must resolve inside the runner's data container. | ||
| let temporaryDirectory = URL(fileURLWithPath: NSHomeDirectory(), isDirectory: true) |
There was a problem hiding this comment.
P2: This fixes TMPDIR for screenshots only. resolveRecordingOutPath still uses NSTemporaryDirectory() on iOS, so device recordings can be written outside the container while the host reads tmp/<name>. Resolve the recording output under NSHomeDirectory()/tmp on iOS too, using the same helper.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Lifecycle.swift, line 130:
<comment>This fixes TMPDIR for screenshots only. `resolveRecordingOutPath` still uses `NSTemporaryDirectory()` on iOS, so device recordings can be written outside the container while the host reads `tmp/<name>`. Resolve the recording output under `NSHomeDirectory()/tmp` on iOS too, using the same helper.</comment>
<file context>
@@ -124,8 +124,17 @@ extension RunnerTests {
+#if os(iOS)
+ // A managed XCTest host can override TMPDIR outside the app container.
+ // The returned tmp/ path must resolve inside the runner's data container.
+ let temporaryDirectory = URL(fileURLWithPath: NSHomeDirectory(), isDirectory: true)
+ .appendingPathComponent("tmp", isDirectory: true)
+#else
</file context>
| const bundleIds = resolveRunnerContainerBundleIds(env); | ||
| if (cachedBundleId) bundleIds.unshift(cachedBundleId); | ||
| for (const bundleId of new Set(bundleIds)) { | ||
| for (const sourcePath of resolveRunnerScreenshotFilePaths(remoteFileName)) { |
There was a problem hiding this comment.
P3: When the runner screenshot is missing, this loop runs simctl get_app_container once per bundle id and candidate path pair, because the container path is cached only after a successful copy. Resolve the container once per bundle id and try the candidate paths against it, as the removed tryCopySimulatorRunnerScreenshot did.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/platform-apple/src/core/screenshot.ts, line 373:
<comment>When the runner screenshot is missing, this loop runs `simctl get_app_container` once per bundle id and candidate path pair, because the container path is cached only after a successful copy. Resolve the container once per bundle id and try the candidate paths against it, as the removed `tryCopySimulatorRunnerScreenshot` did.</comment>
<file context>
@@ -351,73 +362,50 @@ async function copyRunnerScreenshotFromSimulator(
+ const bundleIds = resolveRunnerContainerBundleIds(env);
+ if (cachedBundleId) bundleIds.unshift(cachedBundleId);
+ for (const bundleId of new Set(bundleIds)) {
+ for (const sourcePath of resolveRunnerScreenshotFilePaths(remoteFileName)) {
+ try {
+ await copySimulatorFileToHost({
</file context>
| throw new AppError('INVALID_ARGS', 'Runner products must contain regular files'); | ||
| const file = path.join(directory, name); | ||
| const stat = await fs.stat(file); | ||
| const sha256 = createHash('sha256') |
There was a problem hiding this comment.
P3: Each launch re-reads and hashes the entire runner tree to send only Runner.xctestrun. Hash the files once during preparation and reuse the digests, or stream-hash with createReadStream instead of readFile.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/core/interactors/limrun-ios-runner-products.ts, line 145:
<comment>Each launch re-reads and hashes the entire runner tree to send only `Runner.xctestrun`. Hash the files once during preparation and reuse the digests, or stream-hash with `createReadStream` instead of `readFile`.</comment>
<file context>
@@ -0,0 +1,198 @@
+ throw new AppError('INVALID_ARGS', 'Runner products must contain regular files');
+ const file = path.join(directory, name);
+ const stat = await fs.stat(file);
+ const sha256 = createHash('sha256')
+ .update(await fs.readFile(file, { signal }))
+ .digest('hex');
</file context>
|
I read this at 50ed204. The Limrun iOS runner design looks sound, but this PR needs live device evidence and a smaller shape before it can merge. The PR has merge conflicts with main. The only check that ran is a Vercel fork deployment waiting for a team member to authorize it. It did not build, and no unit, integration or device lane has run on this head. The next step is to rebase, get CI approved, and post the evidence below at the new head. This change affects live device behavior on two routes. On Limrun iOS, the whole interaction path now goes through a remote XCTest runner. On local Apple, runner-lifecycle.ts has about 1,230 lines of churn, and the screenshot, app-resolution and settings paths are refactored. The PR body states Limrun results without commands, output, instance, runtime or SHA, and only the physical iPhone run is tied to this head. A rebase also invalidates anything recorded at 50ed204. Please post a transcript or gist at the new head that shows (a) Limrun iOS with no The change is +6,994/-6,725 across 183 files, against the 2,500-line single-change limit. It carries layers that can merge on their own: (1) the apple-runner session-owner refactor and the The PR adds a second runner-session lifecycle owner in limrun-ios-runner.ts next to the local one. It has its own Not blocking, and fine to take or leave: (1) The open inline threads from the other review: the first-read log timeout still applies (#4174 (comment)). These P2 threads still apply: listener bind tests (#4174 (comment)), openContainer null guard (#4174 (comment)), cancel test timing (#4174 (comment)), signal option type (#4174 (comment)), shared container signal (#4174 (comment)), dispose try/finally (#4174 (comment)), abort reason type (#4174 (comment)), memoized first signal (#4174 (comment)), container deadline (#4174 (comment)), missing Info.plist (#4174 (comment)), JSON before ok (#4174 (comment)), undefined bundle reset (#4174 (comment)), shared deadline duplicate (#4174 (comment)), abort listener throws (#4174 (comment)), connect error reason (#4174 (comment)). Nine lower-priority threads still apply too. These do not apply, so you can resolve them: plist bundling, since platform-apple is private and the published root bundles plist through tsdown (#4174 (comment)); mutation retry, since mutations use I could not run Swift or TypeScript tests or a device in this review, so the new tests are judged from the diff only. I could not observe the Limrun server, so whether closing the |
3821a2c to
722f072
Compare
muvaf
changed the title
722f072 to
053fa0f
Compare
|
Thanks for the update. At 053fa0f the code reads well, but I can't call it ready yet because the evidence is incomplete. I read the diff only and ran no Swift, TypeScript or device tests. I also did not reproduce the mutation checks, and the local simulator transcript is the gist you shared, which I did not observe. Since the review at 50ed204 (#4174 (comment)), the bind-address, forwarded-admission and local-endpoint cases now have Swift tests, the forwarding error now passes through unchanged when no launch-log cause exists, and the screenshot frame and missing-config messages are fixed. Several files that drew comments have also left the PR. What remains is the launcher port asked for in that review (see the question below). Apple Smoke Tests failed in the iOS phase at "Preflight iOS runner through public CLI". Live Limrun evidence cannot exist yet, because nothing in production reaches Not blocking, take or leave: (1) Question: should this PR finish the launcher port before the Limrun layer stacks on it? Concretely, would On the other open threads: the P3 thread on re-exports still applies (#4174 (comment)). The ones that no longer apply and can be resolved: plist.ts is out of the diff (#4174 (comment)); an undisclosed error resolves to dispatch After the iOS preflight rerun passes, the open item is your answer on the launcher shape. |
Summary
Support managed XCTest launchers through the existing Apple runner session owner. Local and forwarded runners share command recovery, cancellation, retention, shutdown, and log flushing. Add authenticated HTTP endpoints and explicit listener binding while preserving local Xcode launch mechanics and loopback defaults.
This is the first layer requested in review, across 42 files. The Apple provider hooks and Limrun AX-to-native-runner switch follow in separate branches.
Validation
Tested
053fa0f2ee1c369b7b330d1e4829b7c553e36fe6, based on7b3f63fea.pnpm check: passed, exact-commit receipt. 17,000 tests passed; one skipped.Commands, output, device details, regression proof, and receipts. The same evidence includes the follow-up branches and Limrun/local flows with explicit source SHAs.
GitHub lint/typecheck/package, coverage, integration, and repository checks passed. Apple smoke failed when
simctl list devices -jtimed out before runner startup; native iOS tests and the macOS phase passed. GitHub requires upstream permissions to rerun it. Vercel also needs team authorization.