Repository navigation
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4684 +/- ##
==========================================
+ Coverage 75.43% 75.44% +0.02%
==========================================
Files 1055 1056 +1
Lines 52404 52455 +51
Branches 11437 11446 +9
==========================================
+ Hits 39526 39571 +45
- Misses 11891 11897 +6
Partials 987 987 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Codex implemented and tested the app-bundle packaging, unsupported-format rejection, and action-specific folder guidance under Gabe’s direction. Gabe confirmed review of the final changes at CI passed on the current head, including Node 22/24; Devin completed analysis on the same head, with no review threads. The description retains the dependency on released host support from expo/expo-device-hub#389 and the hosted validation boundary. — Codex |
|
Subscribed to pull request
Generated by CodeMention Warning: The preamble and epilogue options in commentConfiguration are deprecated. Use template instead. |
|
Hosted QA: Codex installed local — Codex |
brentvatne
left a comment
There was a problem hiding this comment.
Reviewed at 23c737a with Codex, Grok and Claude reviewers; I checked each finding below against the code.
1. tar.create can hang forever on bundles with hard links (medium-low)
control.ts:313-322:
await tar.create(
{ cwd: bundle, file: archive, prefix: path.basename(path.resolve(value)), gzip: true, portable: true },
['.']
);With tar 7.5.19 on Node 24, a bundle containing a hard-linked file next to framework Versions/Current symlinks hung in 29 of 30 runs; with jobs: 1 it completed 30 of 30. The cause is in node-tar's pack: the pending-links loop in [JOBDONE] starts new jobs without checking the job limit, after which [PROCESS] never reaches the head job. Upgrading tar does not fix it.
Because packaging runs before the spinner starts and the WebSocket keeps the process alive, the CLI stops silently after "Connected to simulator session". Xcode output has no hard links (the simulator bundles I checked had none and packaged fine); it takes a bundle that went through a hard-link-preserving copy or dedup step.
Fix: pass jobs: 1, add a regression test with a hard link, and start the spinner before packaging.
2. Interrupting leaves the temporary archive (low)
Cleanup runs only in the catch at :324 and the finally in runActionAsync. Ctrl-C during packaging or a long upload leaves $TMPDIR/eas-simulator-app-*/app.tar.gz, a compressed copy of the app. A one-shot SIGINT/exit handler that removes the directory synchronously would cover it.
3. Smaller items (low)
uploadArchive: z.string().optional()(hostActions.ts:34) makes a non-string value failCatalog.safeParse, which breaks every action, not onlyapp.install. No current host sends one;z.unknown()with the=== 'tar.gz'check at packaging time would keep the catalog tolerant.- A prebuilt
.tar.gz/.tgzfile skips the capability check (it runs only for directories), so an older host fails insimctl. That matches the previous behavior for files, but the description's "The CLI checks the host's capability" covers.appdirectories only. - The host rejects bundles with absolute symlinks, over 2 GiB expanded, or over 100,000 entries only after the full upload. A local pre-check would save the upload.
Checked and fine: the archive keeps the supplied .app name (including through a symlinked bundle), executable bits and internal symlinks; links pointing outside the bundle are archived as links, not followed; temp directories come from mkdtemp; packaging and upload stream rather than buffer.
This PR and #4683 both rewrite findLocalUploadAsync from main, so whichever lands second needs a rebase.
Start progress before packaging, require archive support for local archives, and keep unknown capability metadata from rejecting the action catalog. Add child-process interruption and packaging coverage. Co-authored-by: Codex <codex@openai.com>
|
/devin review Thanks @brentvatne for the archive review. Codex addressed the follow-up at Codex ran 529 simulator/help tests, build, typecheck and changed-file lint/format checks. Child-process packaging and SIGINT tests also pass on Node 24. Cancellation and capability regressions failed before the fix. Our bounded hard-link/framework fixture completes with and without Local adversarial review checked interruption during packaging and upload, handler removal, archive name preservation, capability fallback and progress ordering. A local integration with #4682 and #4683 passes 548 tests plus typecheck; those PRs remain independent. No new hosted QA. Gabe approved the direction; human inspection of this follow-up is pending. Approvals and reviewer requests were preserved. Please review the current head, especially cancellation ownership, archive cleanup and capability parsing. — Codex |
23c737a to
17b990c
Compare
|
✅ Thank you for adding a changeset! |
Why
simulator:control app.installrejects local.appdirectories even though they are the native Simulator app format.Splits app-bundle uploads out of #4679; targets
main.Dependency
Requires expo/expo-device-hub#389 to be merged and released in the session's serve-sim version for local
.appuploads to work. The CLI checks the host'suploadArchive: 'tar.gz'capability and reports an unsupported-host error when it is absent.expo/expo-device-hub#393 is the host staging/concurrency follow-up; #389 provides the archive-install capability this CLI feature depends on.
How
.appdirectory as a gzip tar archive when the host advertisesuploadArchive: 'tar.gz'..appname in the archive..appin Simulator install examples and retain the existing file-path policy..ipaand.zipinstall files before uploading, with guidance to use a Simulator.appor.tar.gz/.tgz.app.install; other upload actions ask for a file.Test Plan
Codex ran 486 simulator/help tests across 21 suites, the CLI build, package typecheck, and changed-file lint/format checks. A combined checkout with #4681, #4682, and #4683 passed 498 tests. Tests inspect real archives and cover symlinks, host capability checks, and cleanup after failures.
Earlier local validation installed four app sources on an owned iOS Simulator through real host upload/install actions with stubbed cloud discovery/transport. That device validation was not repeated for this split. CI passed on
69e6e8c5b(including Node 22 and Node 24); Devin reviewed the same head and reported no issues. The unsupported-format follow-up is at492a80772. The action-specific folder-message correction at23c737a0cpassed all 38 control tests, package typecheck, and changed-file lint/format checks; CI passed on that head, including Node 22/24, GraphQL, changeset and coverage checks. Devin completed analysis on the same head; there are no review threads. Hosted QA: Codex installed local.app,.tgzand literalpath=fixture.appinputs through the four-fix CLI build5f147368f, then verified exact launch arguments/deep links from the app itself. A JSON import also installed a local.appthrough the unpublished all-CLI integrationdc6bfa8d6. Both used real EAS with host tip expo/expo-device-hub#393, which includes the required archive support from expo/expo-device-hub#389.Authorship and LLM use
Gabe directed the fixes and requested separate PRs. Codex wrote, split, reviewed, and tested the implementation and description. Devin's earlier symlink finding is retained as a regression; Codex added packaging-failure cleanup coverage during the split. Gabe subsequently requested early rejection of unsupported formats; Codex added the guard and failing-first regressions. Gabe reviewed the final changes at
23c737a0cand requested Brent and Krystof for review.Review follow-up at
17b990cf8: Codex serialized tar packaging, added scoped cancellation cleanup and visible packaging progress, checked known local archive capabilities and kept catalog parsing tolerant. Codex ran 529 tests plus build/typecheck/lint/format, Node 24 child tests, and a 548-test local integration with #4682/#4683; all pass. The hard-link fixture completes locally but does not reproduce Brent’s reported hang. Gabe reviewed the final changes at17b990cf8, including this follow-up. Current-head CI passed, and Devin completed analysis with no open findings. Its last written no-issues report is on the earlier head.— Codex