Finish open issues: global config (#8), remote doctor+verify (#6), shared picker (#11), key cleanup (#4)#12
Merged
Merged
Conversation
A globally-installed `launch` loads the user's launch.config.ts through jiti, which
resolved its `import { defineConfig } from "launch-store"` from the user's project —
so a project with no local launch-store dep threw "Cannot find package 'launch-store'".
Pin the specifier to the running CLI's own public entry via a jiti alias, so the
config loads with zero local dependency and always binds to the exact defineConfig
of the CLI consuming it (no dual-package version skew). Editor types stay an opt-in
devDependency. Covered by a no-node_modules loader test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e builds (#6) The remote-Mac build assumed every tool was present and grabbed the artifact with a blind `ls *.ipa` — a missing fastlane/pod failed cryptically mid-build, and a dead export was transferred (and submitted) unguarded. Doctor: generate a host preflight from the canonical REQUIRED_TOOLS (single source of truth) and run it before every remote build. An AWS host we own brew-installs any gaps; a BYO-SSH host is only asserted (never mutated) and fails with the same `brew install …` hints `launch doctor` prints. The one-time golden-AMI bootstrap is realigned to the same list so images stop being a hand-maintained subset. Verify: fail fast on the host when gym produced no non-empty .ipa (no wasted transfer), then reuse the exported assertDeviceArtifact after pull as the authoritative simulator/.app/empty guard — identical errors to local builds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…key pickers (#11) The app picker already fuzzy-searched past a threshold, but the same flat-select pattern was duplicated in the .p8 key picker. Extract a single core/prompt.ts `pickOne` (+ the `fuzzyMatch` filter) — flat select for short lists, fuzzy autocomplete past the threshold — and route both selectApp and the credentials key picker through it. The non-interactive policy is explicit per call site: selectApp now REFUSES to guess which app to build with no TTY and points at --app (it would previously hang on a prompt), while the key picker falls back to the newest match with a note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
set-key imported the .p8 into the keychain but left the plaintext key sitting in ~/Downloads — a sync'd, backed-up location where a private key shouldn't linger. After a verified import, offer to delete the source file when it's in a scanned discovery dir (~/Downloads, cwd, …), defaulting to yes; a key the user deliberately placed elsewhere (--p8 ~/vault/…) is never touched, and nothing is moved into ~/.launch (secrets stay only in the keychain). Non-interactive runs keep the file and print its path — a key is never deleted without consent. The iOS offer is gated on Apple actually verifying the stored key (teamId resolved), since a .p8 downloads from Apple exactly once; the re-downloadable Play service-account JSON gets the same offer without that gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Code Review by Qodo
Context used✅ Tickets:
🎫 Global install: load launch.config.ts without requiring launch-store as a local project dep 🎫 Remote AWS EC2 Mac build target: run the same doctor + build + verification on the remote host 🎫 wizard: app picker is unusable in large monorepos — add a search/filter input✅ Compliance rules (platform):
13 rules 1. remoteBuild.ts lacks colocated tests
|
📝 WalkthroughWalkthroughIntroduces a shared ChangesShared
Remote toolchain preflight and AWS bootstrap dynamic generation
Config
Sequence Diagram(s)sequenceDiagram
participant CLI as launch CLI
participant remotePipeline as runRemoteBuild
participant runDoctorOnHost
participant remoteToolchainPreflight as remoteToolchainPreflight(mode)
participant SSHHost as EC2 / BYO Mac
rect rgba(70, 130, 180, 0.5)
note over remotePipeline,SSHHost: Project sync
remotePipeline->>SSHHost: rsync project files
end
rect rgba(60, 179, 113, 0.5)
note over remotePipeline,SSHHost: Toolchain preflight (new)
remotePipeline->>runDoctorOnHost: runDoctorOnHost(session, "install"|"assert")
runDoctorOnHost->>remoteToolchainPreflight: generate bash script string
runDoctorOnHost->>runDoctorOnHost: write script to local tmp dir
runDoctorOnHost->>SSHHost: scp script to per-run creds dir
runDoctorOnHost->>SSHHost: bash <preflight-script>
SSHHost-->>runDoctorOnHost: LAUNCH_PREFLIGHT_OK or LAUNCH_PREFLIGHT_FAILED
runDoctorOnHost->>runDoctorOnHost: rm local tmp dir (finally)
runDoctorOnHost-->>remotePipeline: void or throw
end
rect rgba(210, 105, 30, 0.5)
note over remotePipeline,SSHHost: Sign + build + pull artifact
remotePipeline->>SSHHost: upload signing credentials
remotePipeline->>SSHHost: fastlane gym (REMOTE_BUILD_SCRIPT)
SSHHost-->>remotePipeline: .ipa (assertDeviceArtifact validates byte size)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:L
PR Summary by QodoFinish open issues: global config alias, remote doctor+verify, shared picker, key cleanup WalkthroughsDescription• **#8 (bug fix):** Adds a jiti alias pinning launch-store to the CLI's own entry point, so launch.config.ts loads correctly on global installs with no local launch-store dependency. • **#6 (enhancement):** Runs a generated toolchain preflight (remoteToolchainPreflight) on the remote Mac before every build — installs gaps on AWS hosts, asserts-only on BYO-SSH — and fails fast on the host when gym produces no non-empty .ipa, then validates the pulled artifact with assertDeviceArtifact. • **#11 (refactor):** Extracts pickOne + fuzzyMatch into core/prompt.ts as a shared generic picker; routes both selectApp (refuses to guess without TTY) and the .p8 key picker (falls back to newest) through it with explicit non-interactive policies. • **#4 (enhancement):** After a verified key import, offers to delete the plaintext source file when it sits in a scanned discovery dir (~/Downloads, cwd); gated on Apple verification for .p8 (downloaded once), ungated for re-downloadable Play JSON; never touches files outside discovery dirs. • All four changes are covered by new unit tests (config loader, remote preflight, shared picker, isInDiscoveryDir). Diagramgraph TD
subgraph Config["#8 — Global Config Fix"]
A["launch.config.ts"] -->|"import defineConfig"| B["jiti loader\n(config.ts)"]
B -->|"alias: launch-store → SELF_ENTRY"| C["CLI index.js\n(defineConfig)"]
end
subgraph Remote["#6 — Remote Doctor + Verify"]
D["remotePipeline.ts"] --> E["runDoctorOnHost\n(remoteBuild.ts)"]
E -->|"generates"| F["remoteToolchainPreflight\n(toolchain.ts)"]
F -->|"REQUIRED_TOOLS"| G[("Canonical\nTool List")]
E -->|"scp + ssh"| H["Remote Mac"]
D --> I["pullArtifact\n(remoteBuild.ts)"]
I -->|"assertDeviceArtifact"| J["fastlane.ts"]
G -->|"also drives"| K["awsEc2Mac.ts\nBOOTSTRAP_SCRIPT"]
end
subgraph Picker["#11 — Shared Picker"]
L["core/prompt.ts\npickOne + fuzzyMatch"] --> M["selectApp\n(pipeline.ts)"]
L --> N["resolveP8Path\n(creds.ts)"]
end
subgraph Cleanup["#4 — Key Cleanup"]
O["importIosKeyFromPath\n(creds.ts)"] -->|"teamId verified"| P["offerToRemoveImportedSecret"]
Q["setAndroidKey\n(creds.ts)"] --> P
P -->|"isInDiscoveryDir?"| R["rmSync source file"]
end
subgraph Legend
direction LR
_mod["Module/Function"] ~~~ _db[("Data/Config")] ~~~ _ext["External Host"]
end
High-Level AssessmentThe following are alternative approaches to this PR: 1. Bundle launch-store into the CLI dist (#8)
2. Run doctor as inline SSH commands instead of an uploaded script (#6)
Recommendation: The PR's approach is well-suited for each issue. The jiti alias is the correct minimal fix for #8 — alternatives like bundling the config or requiring a local dep would be heavier. For #6, generating the preflight from File ChangesEnhancement (4)
Bug fix (2)
Refactor (2)
Tests (5)
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+322
to
+326
| IPA="$(ls "$OUT"/*.ipa 2>/dev/null | head -1)" | ||
| if [ -z "$IPA" ] || [ ! -s "$IPA" ]; then | ||
| echo "LAUNCH_NO_ARTIFACT: gym produced no non-empty .ipa in $OUT" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Suggestion: With set -euo pipefail enabled, the command substitution in IPA="$(ls "$OUT"/*.ipa 2>/dev/null | head -1)" can terminate the script immediately when no .ipa exists, so the explicit LAUNCH_NO_ARTIFACT error branch may never run. Capture the path with a non-failing glob/check pattern (or temporarily disable errexit for that probe) so missing artifacts reliably produce the intended actionable error message. [logic error]
Severity Level: Major ⚠️
- ⚠️ Remote Mac builds with missing .ipa fail opaquely.
- ⚠️ LAUNCH_NO_ARTIFACT hint never reaches CLI users.
- ⚠️ Debugging remote fastlane export issues becomes harder.Steps of Reproduction ✅
1. Run a remote iOS build via the CLI, for example `launch build ios --remote aws` which
is wired in `src/cli/commands/build.ts:80-119` by calling `runBuild({... remote })` in
`src/core/pipeline.ts:10-27`.
2. `runBuild` dispatches to the remote pipeline (`src/core/pipeline.ts:17-21`), which
calls `runRemoteBuild` and then `runBuildOnHost` in `src/core/remotePipeline.ts:15-26`,
uploading and executing the `REMOTE_BUILD_SCRIPT` via `sshRun` as shown in
`src/core/remoteBuild.ts:14-27`.
3. On the remote Mac, the uploaded bash script (`REMOTE_BUILD_SCRIPT` at
`src/core/remoteBuild.ts:52-72`) runs with `set -euo pipefail` (`line 53`) and eventually
executes fastlane gym (`lines 45-55` in the script body) to produce an `.ipa` under
`$OUT`; assume a misconfiguration or upstream issue causes gym to complete without leaving
any `.ipa` in `$OUT`, which is precisely the situation the "Fail fast on the host if gym
produced no non-empty .ipa" guard (comments at `lines 61-62` of the script) is intended to
detect.
4. The guard executes `IPA="$(ls "$OUT"/*.ipa 2>/dev/null | head -1)"`
(`src/core/remoteBuild.ts:63`), but with no matching `.ipa` the `ls "$OUT"/*.ipa` command
in the pipeline exits non-zero; under `set -euo pipefail` this non-zero status causes the
script to terminate immediately, so the subsequent `if [ -z "$IPA" ] || [ ! -s "$IPA" ];
then ... echo "LAUNCH_NO_ARTIFACT: …" ... fi` block (`lines 64-67`) never runs, and the
remote build fails with a generic SSH/exit-code error instead of the intended actionable
`LAUNCH_NO_ARTIFACT` message.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/remoteBuild.ts
**Line:** 322:326
**Comment:**
*Logic Error: With `set -euo pipefail` enabled, the command substitution in `IPA="$(ls "$OUT"/*.ipa 2>/dev/null | head -1)"` can terminate the script immediately when no `.ipa` exists, so the explicit `LAUNCH_NO_ARTIFACT` error branch may never run. Capture the path with a non-failing glob/check pattern (or temporarily disable errexit for that probe) so missing artifacts reliably produce the intended actionable error message.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+137
to
+155
| /** | ||
| * Run the toolchain doctor ON the remote Mac before building — the remote twin of `launch doctor`. | ||
| * Uploads {@link remoteToolchainPreflight} and executes it: `"install"` for an AWS host we own (brew- | ||
| * installs any gaps) or `"assert"` for a BYO-SSH host (checks + fails with hints, never mutates the | ||
| * user's machine). A missing required tool exits the preflight non-zero, so `sshRun` rejects and the | ||
| * build fails fast with the gaps listed instead of a cryptic error deep inside fastlane. | ||
| */ | ||
| export async function runDoctorOnHost(session: RemoteSession, mode: "install" | "assert"): Promise<void> { | ||
| const staging = mkdtempSync(join(tmpdir(), "launch-remote-")); | ||
| const scriptLocal = join(staging, "doctor.sh"); | ||
| writeFileSync(scriptLocal, remoteToolchainPreflight(mode)); | ||
| const scriptRemote = `${session.credsDir}/doctor.sh`; | ||
| try { | ||
| await scpUp(session.target, scriptLocal, scriptRemote); | ||
| } finally { | ||
| rmSync(staging, { recursive: true, force: true }); | ||
| } | ||
| await sshRun(session.target, `bash ${shellQuote(scriptRemote)}`); | ||
| } |
There was a problem hiding this comment.
1. remotebuild.ts lacks colocated tests 📘 Rule violation ▣ Testability
src/core/remoteBuild.ts adds/changes non-trivial remote-build behavior (remote doctor execution and artifact verification) but there is no corresponding src/core/remoteBuild.test.ts file. This violates the requirement to co-locate tests with new/significantly modified logic, reducing change safety and regression detection.
Agent Prompt
## Issue description
`src/core/remoteBuild.ts` was significantly extended/modified (e.g., `runDoctorOnHost` and remote artifact verification behavior), but there is no co-located `src/core/remoteBuild.test.ts`.
## Issue Context
Compliance requires co-located `*.test.ts` files for new/significantly modified TypeScript modules containing executable logic.
## Fix Focus Areas
- src/core/remoteBuild.ts[137-155]
- src/core/remoteBuild.ts[220-224]
- src/core/remoteBuild.ts[320-326]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+20
to
+31
| const SELF_ENTRY = fileURLToPath(new URL("../index.js", import.meta.url)); | ||
|
|
||
| /** | ||
| * On-the-fly loader for the user's config. The compiled `launch` binary runs on plain Node, which | ||
| * can't `import()` a TypeScript file — jiti transpiles `launch.config.ts` in memory and resolves its | ||
| * `launch-store` import from the user's project. (Chosen over bundling a TS toolchain ourselves; it's | ||
| * the same loader Nuxt/ESLint use for config files.) | ||
| * can't `import()` a TypeScript file — jiti transpiles `launch.config.ts` in memory. The `alias` pins | ||
| * the config's `import { defineConfig } from "launch-store"` to {@link SELF_ENTRY}, so a globally | ||
| * installed `launch` loads the config even when the user's project has no local `launch-store` | ||
| * dependency (issue #8), and the config always binds to the exact `defineConfig` of the CLI consuming | ||
| * it — no dual-package version skew. (jiti chosen over bundling a TS toolchain ourselves; it's the | ||
| * same loader Nuxt/ESLint use for config files.) | ||
| */ | ||
| const jiti = createJiti(import.meta.url); | ||
| const jiti = createJiti(import.meta.url, { alias: { "launch-store": SELF_ENTRY } }); |
There was a problem hiding this comment.
3. Broken config self-alias 🐞 Bug ≡ Correctness
loadConfig pins import "launch-store" to SELF_ENTRY, but SELF_ENTRY is hard-coded as ../index.js relative to src/core/config.ts, while this repo’s public entry is src/index.ts (and source checkouts typically won’t have src/index.js). This makes config loading rely on nonstandard resolution and can fail in dev/test or any run-from-source environment.
Agent Prompt
### Issue description
`SELF_ENTRY` is computed as `../index.js` relative to `src/core/config.ts`, but in the source tree the public entry is `src/index.ts` (with TS importing `.js` specifiers). If `src/index.js` is absent, the jiti alias can point at a non-existent file and break loading a user `launch.config.ts` that imports `defineConfig` from `launch-store`.
### Issue Context
This alias is used by jiti when importing user configs, so it must resolve reliably both in production (`dist/`) and in source/test environments.
### Fix Focus Areas
- src/core/config.ts[14-32]
### Suggested fix
Implement a runtime fallback that selects an entry file that actually exists, e.g.:
- compute `indexJs = fileURLToPath(new URL("../index.js", import.meta.url))`
- if `!existsSync(indexJs)`, fall back to `fileURLToPath(new URL("../index.ts", import.meta.url))`
- use the resolved path in the jiti alias
(Alternatively, use a resolver-based approach like `import.meta.resolve` for the package entry if your Node target supports it.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+451
to
468
| const BOOTSTRAP_BREW_LINES = REQUIRED_TOOLS.flatMap((tool) => { | ||
| if (tool.install.kind !== "brew") return []; | ||
| const fallback = tool.command === "fastlane" ? " || sudo gem install fastlane" : ""; | ||
| return [`command -v ${tool.command} >/dev/null || brew install ${tool.install.formula}${fallback} || true`]; | ||
| }); | ||
|
|
||
| /** Toolchain bootstrap script run once on a base AMI before snapshotting a golden image. */ | ||
| const BOOTSTRAP_SCRIPT = [ | ||
| "set -e", | ||
| "command -v brew >/dev/null || echo LAUNCH_NO_BREW", | ||
| "command -v node >/dev/null || brew install node || true", | ||
| "command -v fastlane >/dev/null || brew install fastlane || sudo gem install fastlane || true", | ||
| ...BOOTSTRAP_BREW_LINES, | ||
| "xcodebuild -version >/dev/null 2>&1 || echo LAUNCH_NO_XCODE", | ||
| ].join("\n"); | ||
|
|
||
| /** Install node/fastlane and assert full Xcode is present (the one part Launch can't legally redistribute). */ | ||
| /** Install the brew-able toolchain and assert full Xcode is present (the one part Launch can't legally redistribute). */ | ||
| async function bootstrapToolchain(ssh: SshTarget): Promise<void> { | ||
| const output = await sshCapture(ssh, BOOTSTRAP_SCRIPT); | ||
| if (output.includes("LAUNCH_NO_XCODE")) { |
There was a problem hiding this comment.
5. Ami bootstrap misses brew failure 🐞 Bug ☼ Reliability
The golden-AMI bootstrap script emits LAUNCH_NO_BREW when Homebrew is missing, but bootstrapToolchain only throws on LAUNCH_NO_XCODE. This can snapshot an AMI that silently failed to install required brew tools, causing remote builds to fail later with harder-to-diagnose errors.
Agent Prompt
### Issue description
`BOOTSTRAP_SCRIPT` prints a sentinel when `brew` is missing, but `bootstrapToolchain()` doesn’t check it, so provisioning can appear successful even though none of the brew installs actually happened.
### Issue Context
The script uses `|| true` in install lines, so missing/failed installs won’t necessarily cause a non-zero exit. Relying on sentinels requires the caller to enforce them.
### Fix Focus Areas
- src/providers/compute/awsEc2Mac.ts[457-474]
### Suggested fix
Either:
- Make the script fail hard when brew is missing (e.g. `command -v brew >/dev/null || { echo LAUNCH_NO_BREW; exit 1; }`), or
- Add an explicit check in `bootstrapToolchain()`:
- if output includes `LAUNCH_NO_BREW`, throw an actionable error telling the user to ensure Homebrew is installed on the base AMI.
Optionally also verify at least one required brew tool is present after bootstrap to catch install failures early.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+123
to
+124
| rmSync(file, { force: true }); | ||
| console.log(`Removed ${tildify(file)}.`); |
There was a problem hiding this comment.
Suggestion: Deletion of the plaintext key file is performed with rmSync without error handling, so permission/filesystem errors after a successful import will throw and make the command fail even though credentials were already stored. Catch deletion errors and log a warning so post-import cleanup failures do not break the main operation. [logic error]
Severity Level: Major ⚠️
- ❌ `launch creds set-key` can fail after persisting account.
- ⚠️ Users see an error despite credentials stored successfully.
- ⚠️ Android `set-key` suffers identical fragile cleanup behavior.Steps of Reproduction ✅
1. Run the CLI command `launch creds set-key` in an interactive terminal so
`process.stdin.isTTY` is true and `--yes` is not set; this command is wired up in
`registerCredsCommand` at `src/cli/commands/creds.ts:223-251` and calls `setKey()` at
`src/cli/commands/creds.ts:8-13` for iOS.
2. Ensure the `.p8` file to import (e.g. `~/Downloads/AuthKey_XXXX.p8`) lives directly in
one of the discovery directories listed in `SEARCH_DIRS` at
`src/cli/commands/creds.ts:76-82`, so `isInDiscoveryDir()` at
`src/cli/commands/creds.ts:90-93` returns true.
3. During the run, `setKey()` calls `resolveP8Path()` at
`src/cli/commands/creds.ts:172-204` to find the `.p8`, then `importIosKeyFromPath()` at
`src/cli/commands/creds.ts:237-260` which validates the key, calls `addAccount(...)` at
line 254 to persist credentials, and logs success at line 260.
4. After a successful import, `importIosKeyFromPath()` invokes
`offerToRemoveImportedSecret(p8Path, "App Store Connect key", canPrompt)` at
`src/cli/commands/creds.ts:3-5`, which, in interactive mode, prompts the user and, if they
confirm, executes `rmSync(file, { force: true });` followed by `console.log(...)` at
`src/cli/commands/creds.ts:123-124`.
5. If deletion of the `.p8` file fails due to a filesystem or permission error (e.g.
directory not writable, ACL denies delete), `rmSync` throws synchronously; there is no
try/catch around this call in `offerToRemoveImportedSecret` or its callers, so the error
propagates and causes the overall `launch creds set-key` command to fail even though
`addAccount` already stored the credentials.
6. The same pattern affects the Android `set-key` flow: `setAndroidKey()` at
`src/cli/commands/creds.ts:15-30` stores the Play service-account JSON via
`storeServiceAccount` at line 26, then calls `offerToRemoveImportedSecret(path, "Play
service-account key", canPrompt)` at line 29, so a deletion error there will also turn a
successful secret import into a failed command.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/cli/commands/creds.ts
**Line:** 123:124
**Comment:**
*Logic Error: Deletion of the plaintext key file is performed with `rmSync` without error handling, so permission/filesystem errors after a successful import will throw and make the command fail even though credentials were already stored. Catch deletion errors and log a warning so post-import cleanup failures do not break the main operation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+191
to
+195
| nonInteractive: { | ||
| kind: "fallback", | ||
| value: first, | ||
| note: `Multiple API keys found; using ${tildify(first)}. Pass --p8 to choose another.`, | ||
| }, |
There was a problem hiding this comment.
Suggestion: In non-interactive mode with multiple discovered .p8 files, this silently picks first, but first is just search-order dependent and not guaranteed to be the intended key. That can import the wrong Apple account in automation. Either require explicit --p8 when multiple matches exist or deterministically choose by a strong criterion (e.g., newest mtime) and document that rule. [logic error]
Severity Level: Critical 🚨
- ❌ Non-interactive `set-key` may import the wrong Apple account.
- ⚠️ Builds that follow can target an unintended Apple team.
- ⚠️ CI runs rely on search-order-dependent, weak key selection.Steps of Reproduction ✅
1. Prepare a machine with at least two different `AuthKey_*.p8` files in the discovery
directories defined by `SEARCH_DIRS` at `src/cli/commands/creds.ts:76-82` (for example,
two keys in `~/Downloads` or one in `~/Downloads` and one in `~/.launch/credentials`).
2. Run the iOS onboarding command non-interactively, e.g. `launch creds set-key --yes` (or
from CI with no TTY), without specifying `--p8` and without `ASC_API_KEY_PATH` set; this
invokes `setKey()` at `src/cli/commands/creds.ts:8-13`, which sets `canPrompt =
options.yes !== true && process.stdin.isTTY` at line 10.
3. Inside `setKey()`, `resolveP8Path(options, canPrompt)` at
`src/cli/commands/creds.ts:172-204` is called; with no explicit path, it computes `found =
SEARCH_DIRS.flatMap(findAuthKeyFiles)` at line 180, and destructures `[first, ...rest]` at
line 181, where both `first` and `rest[0]` are real but different `.p8` files.
4. Because multiple keys are found, the `if (first)` branch at
`src/cli/commands/creds.ts:186-197` executes, calling `pickOne<string>({...})` from
`src/core/prompt.ts:69-105` with `canPrompt` (false in this non-interactive scenario) and
a `nonInteractive` policy of `{ kind: "fallback", value: first, note: ... }` at lines
191-195.
5. `pickOne()` in `src/core/prompt.ts:69-76` sees `!args.canPrompt` and
`nonInteractive.kind === "fallback"`, prints the note if present, and returns
`args.nonInteractive.value`, i.e. the `first` discovered path — which is purely
search-order dependent, not necessarily the intended or newest key.
6. `importIosKeyFromPath(p8Path, ...)` at `src/cli/commands/creds.ts:237-260` then imports
and activates the account associated with this arbitrary `first` key, so in automation the
wrong Apple account can be onboarded and set active without any explicit selection based
on a strong, documented rule like newest mtime or requiring `--p8`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/cli/commands/creds.ts
**Line:** 191:195
**Comment:**
*Logic Error: In non-interactive mode with multiple discovered `.p8` files, this silently picks `first`, but `first` is just search-order dependent and not guaranteed to be the intended key. That can import the wrong Apple account in automation. Either require explicit `--p8` when multiple matches exist or deterministically choose by a strong criterion (e.g., newest mtime) and document that rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const hint = app.bundleId ?? app.packageName; | ||
| return hint ? { value: app, label: app.name, hint } : { value: app, label: app.name }; | ||
| }), | ||
| canPrompt: process.stdin.isTTY, |
There was a problem hiding this comment.
Suggestion: selectApp now decides promptability with process.stdin.isTTY, which ignores CI. In CI environments that still expose a TTY, this can trigger an interactive picker and hang builds instead of failing fast with a --app hint. Use the same interactivity gate used elsewhere (isInteractive()) so CI is always treated as non-interactive. [incorrect condition logic]
Severity Level: Critical 🚨
- ❌ `launch build` can hang waiting for app selection in CI.
- ❌ `launch release` may also block on interactive app picking.
- ⚠️ Creds `setup` flows mis-handle CI interactivity similarly.Steps of Reproduction ✅
1. In a CI environment where `process.env.CI` is set (e.g. a hosted runner) but stdin is
attached to a TTY (so `process.stdin.isTTY === true`), invoke the main build entrypoint
`runBuild(options)` exported from `src/core/pipeline.ts:50-59` via the CLI `launch build`
command (wired to `prepareBuild`/`runBuild` — `prepareBuild` is at
`src/core/pipeline.ts:11-42`).
2. Run `launch build` without the `--app` flag while the project has multiple apps
discovered by `loadConfig()` (called inside `prepareBuild` at
`src/core/pipeline.ts:15-16`), so `apps.length > 1` and there is no explicit `appName`.
3. `prepareBuild()` calls `selectApp(apps, options.appName)` at
`src/core/pipeline.ts:15-17`; inside `selectApp` at `src/core/pipeline.ts:149-167`,
because `apps.length > 1` and `appName` is undefined, it falls through to the
`pickOne<AppDescriptor>({ ... })` call at lines 159-167.
4. The `pickOne` call sets `canPrompt: process.stdin.isTTY` at `src/core/pipeline.ts:165`,
which is true in this CI scenario, so `pickOne()` in `src/core/prompt.ts:69-105` goes down
the interactive `select`/`autocomplete` path instead of applying its `nonInteractive: {
kind: "require", flagHint: "— pass --app <name> to choose one non-interactively." }`
policy.
5. Because CI has no human to answer the interactive prompt, the process blocks waiting
for input, and the build hangs or eventually times out instead of failing fast with a
clear `--app` hint, in contrast to other interactive decisions (like account selection)
that correctly use the CI-aware `isInteractive()` helper from
`src/core/progress.ts:157-159`.
6. This affects all codepaths that rely on `selectApp`, including `prepareBuild` for
`launch build` (`src/core/pipeline.ts:11-42`), `runRelease` in
`src/cli/commands/release.ts:58-64`, and the iOS/Android `setup` flows in
`src/cli/commands/creds.ts:161-176` and `src/cli/commands/creds.ts:208-220`, all of which
can misclassify CI as interactive when a TTY is present.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/pipeline.ts
**Line:** 165:165
**Comment:**
*Incorrect Condition Logic: `selectApp` now decides promptability with `process.stdin.isTTY`, which ignores `CI`. In CI environments that still expose a TTY, this can trigger an interactive picker and hang builds instead of failing fast with a `--app` hint. Use the same interactivity gate used elsewhere (`isInteractive()`) so CI is always treated as non-interactive.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| if (tool.install.kind !== "brew") return []; | ||
| const fallback = tool.command === "fastlane" ? " || sudo gem install fastlane" : ""; | ||
| return [`command -v ${tool.command} >/dev/null || brew install ${tool.install.formula}${fallback} || true`]; | ||
| }); |
There was a problem hiding this comment.
Suggestion: The bootstrap commands always end with || true, so failed installs are silently ignored and the bootstrap can still succeed, after which a golden AMI is snapshotted without required tools. This creates a persistently broken base image that fails later during remote builds. Remove unconditional success swallowing for required tools or add an explicit post-check that aborts snapshot creation when required tools are still missing. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Golden AMI may miss required iOS build tools.
- ⚠️ Subsequent AWS remote builds fail doctor on every run.
- ⚠️ Debugging harder since bootstrap never reported failures.Steps of Reproduction ✅
1. Trigger an AWS remote build using `runRemoteBuild()` in
`src/core/remotePipeline.ts:92-139` with `remote.kind === "aws"`. This selects the
`aws-ec2-mac` compute host via `hostFor()` at `src/core/remotePipeline.ts:20-22` and calls
its `allocate()` implementation in `src/providers/compute/awsEc2Mac.ts:1-77`.
2. On the first run when no golden AMI id is stored, `allocate()` in
`src/providers/compute/awsEc2Mac.ts:15-26` detects `!goldenAmi` and invokes
`bootstrapToolchain(ssh)` at line 24 before snapshotting a golden AMI with
`snapshotGoldenAmi()`.
3. `bootstrapToolchain()` at `src/providers/compute/awsEc2Mac.ts:466-475` runs
`BOOTSTRAP_SCRIPT`, which is built from `BOOTSTRAP_BREW_LINES` at lines 52-56. Each
brew-install line is `command -v ${tool.command} >/dev/null || brew install
${tool.install.formula}${fallback} || true` (line 55) under a script-wide `set -e` (line
59), but `bootstrapToolchain()` only examines the output for `LAUNCH_NO_XCODE` and ignores
brew failures or missing tools.
4. If any `brew install` (and optional `sudo gem install fastlane` fallback) fails—e.g.,
due to network issues or a broken formula—the `|| true` at the end of the line causes that
failure to be silently swallowed, allowing `BOOTSTRAP_SCRIPT` to complete and
`snapshotGoldenAmi()` to snapshot an AMI without the required tool. Later AWS remote
builds allocate from this golden AMI and rely on `runDoctorOnHost(session, "install")` at
`src/core/remotePipeline.ts:200`, which will repeatedly find missing tools and fail the
doctor, even though the bootstrap step previously claimed success and persisted a broken
base image.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/providers/compute/awsEc2Mac.ts
**Line:** 455:455
**Comment:**
*Incomplete Implementation: The bootstrap commands always end with `|| true`, so failed installs are silently ignored and the bootstrap can still succeed, after which a golden AMI is snapshotted without required tools. This creates a persistently broken base image that fails later during remote builds. Remove unconditional success swallowing for required tools or add an explicit post-check that aborts snapshot creation when required tools are still missing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cli/commands/creds.ts (1)
180-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe non-interactive
.p8fallback is directory-first, not newest-first.
foundpreservesSEARCH_DIRSorder, sovalue: firstpicks the earliest directory hit, not the newest key across all matches. In a non-TTY run that can silently import an older key just because it lives in~/Downloadswhile a newer one exists later in the scan set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/commands/creds.ts` around lines 180 - 195, The non-interactive fallback in the pickOne function currently uses `first` as the fallback value, which picks the earliest directory in SEARCH_DIRS order rather than the newest key. Replace the `value: first` in the nonInteractive fallback object with logic that selects the most recently modified key from the `found` array by checking file modification times, ensuring that in non-TTY environments, the newest available key is used instead of the directory-ordered first match.src/core/pipeline.ts (1)
149-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
selectApp()still prompts under--yes.Promptability is derived from
process.stdin.isTTYonly, but bothprepareBuild()and thelaunch creds setuppaths callselectApp()without any way to thread theiryesflag through. On a real terminal,launch build --yescan still open the picker when multiple apps exist, which breaks the command’s non-interactive contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/pipeline.ts` around lines 149 - 167, The selectApp() function determines promptability only from process.stdin.isTTY without considering the yes flag from calling commands, causing it to still prompt even when --yes is passed. Add a boolean parameter to selectApp() to accept the yes flag, then use it to compute canPrompt as process.stdin.isTTY && !yes (or similar logic to disable prompting when yes is true). Update all callers of selectApp() (in prepareBuild() and launch creds setup paths) to pass their yes flag to this new parameter.src/providers/compute/awsEc2Mac.ts (1)
458-474:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail the AMI bootstrap when Homebrew is unavailable.
BOOTSTRAP_SCRIPTemitsLAUNCH_NO_BREW, butbootstrapToolchain()never checks it and every generatedbrew install …is swallowed with|| true. A base AMI without brew—or with brew only outside the default SSH PATH—can still be snapshotted as a “golden” image with none of the brew-managed tools installed.Suggested fix
const BOOTSTRAP_SCRIPT = [ "set -e", + `eval "$(/opt/homebrew/bin/brew shellenv 2>/dev/null || /usr/local/bin/brew shellenv 2>/dev/null || true)"`, "command -v brew >/dev/null || echo LAUNCH_NO_BREW", ...BOOTSTRAP_BREW_LINES, "xcodebuild -version >/dev/null 2>&1 || echo LAUNCH_NO_XCODE", ].join("\n"); @@ async function bootstrapToolchain(ssh: SshTarget): Promise<void> { const output = await sshCapture(ssh, BOOTSTRAP_SCRIPT); + if (output.includes("LAUNCH_NO_BREW")) { + throw new Error("The base AMI has no usable Homebrew on PATH. Provide an AMI with Homebrew installed, or fix the bootstrap PATH setup before snapshotting."); + } if (output.includes("LAUNCH_NO_XCODE")) { throw new Error( "The base AMI has no full Xcode (gym needs it). Provide a BYO golden AMI with Xcode preinstalled " + "via aws.amiId — Xcode can't be redistributed in a shared image.", ); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/compute/awsEc2Mac.ts` around lines 458 - 474, The bootstrapToolchain() function checks for LAUNCH_NO_XCODE in the output but does not check for LAUNCH_NO_BREW, which is emitted by BOOTSTRAP_SCRIPT when Homebrew is unavailable. This allows bootstrap to complete successfully even when Homebrew is missing or not in the PATH, resulting in a golden AMI without the necessary brew-managed tools installed. Add a check in bootstrapToolchain() to detect if the output includes LAUNCH_NO_BREW and throw an appropriate error message, mirroring the existing LAUNCH_NO_XCODE validation logic.
🧹 Nitpick comments (1)
src/core/pipeline.test.ts (1)
121-124: ⚡ Quick winAdd a
--yesregression here too.This only exercises the no-TTY branch, so the broken “TTY +
--yes” path inselectApp()is still uncovered. A dedicated regression would lock in the non-interactive contract once the picker fix lands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/pipeline.test.ts` around lines 121 - 124, Add a new regression test case in the test suite for selectApp() that exercises the TTY + --yes flag path with multiple apps. Create a test following the same pattern as the existing test (refusing to guess with multiple apps), but pass the --yes flag (as true or equivalent) instead of undefined for the second parameter, and verify it throws an error referencing the --app flag requirement. This will ensure the non-interactive contract is maintained when both TTY and --yes are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/commands/creds.ts`:
- Around line 95-125: The offerToRemoveImportedSecret function currently
implements only a two-way contract (delete or keep), but it needs to support a
three-way contract that includes moving the imported key to a managed location
(~/.launch/credentials/). Extend the function to offer three options: move to
managed location (default), delete, or keep. When moving, construct the target
path as ~/.launch/credentials/AuthKey_<KEYID>.p8 (extracting the key ID from the
file), create the directory if needed, move the file there, and enforce 700
permissions on the directory and 600 on the file. Update the prompts and logic
in offerToRemoveImportedSecret to reflect this three-way flow, ensuring the
managed copy is the default choice. This ensures the iOS path can satisfy the
requirements described in Issue `#4` for moving/deleting/keeping with proper
permission enforcement.
In `@src/core/remoteBuild.ts`:
- Around line 320-325: The code uses a wildcard glob to find any .ipa file in
$OUT and check it, which can pick up stale IPA files from previous builds if the
current gym export fails, and may cause the assignment to exit before the
LAUNCH_NO_ARTIFACT error message under set -euo pipefail. Before running gym,
remove the expected output file "$OUT/$APP_NAME.ipa" to ensure it doesn't exist
from a previous build. Then after gym completes, replace the ls wildcard pattern
and head command with a direct test of the specific expected file: validate
"$OUT/$APP_NAME.ipa" exists and is non-empty using the -s test operator, rather
than searching for any matching .ipa file.
In `@src/core/toolchain.ts`:
- Around line 121-126: The Homebrew shellenv setup is currently only executed
when canInstall is true, causing subsequent SSH processes (for assert runs,
builds, or when runBuildOnHost() starts a fresh shell) to fail to find
Homebrew-installed tools. Move the lines.push() call containing the brew
shellenv command outside and before the if (canInstall) block so that the
Homebrew PATH initialization runs unconditionally for all remote SSH scripts
regardless of the mode. This ensures that node, pod, fastlane, and other
Homebrew-installed tools are available on the PATH for every SSH process
initiated, not just during install mode.
---
Outside diff comments:
In `@src/cli/commands/creds.ts`:
- Around line 180-195: The non-interactive fallback in the pickOne function
currently uses `first` as the fallback value, which picks the earliest directory
in SEARCH_DIRS order rather than the newest key. Replace the `value: first` in
the nonInteractive fallback object with logic that selects the most recently
modified key from the `found` array by checking file modification times,
ensuring that in non-TTY environments, the newest available key is used instead
of the directory-ordered first match.
In `@src/core/pipeline.ts`:
- Around line 149-167: The selectApp() function determines promptability only
from process.stdin.isTTY without considering the yes flag from calling commands,
causing it to still prompt even when --yes is passed. Add a boolean parameter to
selectApp() to accept the yes flag, then use it to compute canPrompt as
process.stdin.isTTY && !yes (or similar logic to disable prompting when yes is
true). Update all callers of selectApp() (in prepareBuild() and launch creds
setup paths) to pass their yes flag to this new parameter.
In `@src/providers/compute/awsEc2Mac.ts`:
- Around line 458-474: The bootstrapToolchain() function checks for
LAUNCH_NO_XCODE in the output but does not check for LAUNCH_NO_BREW, which is
emitted by BOOTSTRAP_SCRIPT when Homebrew is unavailable. This allows bootstrap
to complete successfully even when Homebrew is missing or not in the PATH,
resulting in a golden AMI without the necessary brew-managed tools installed.
Add a check in bootstrapToolchain() to detect if the output includes
LAUNCH_NO_BREW and throw an appropriate error message, mirroring the existing
LAUNCH_NO_XCODE validation logic.
---
Nitpick comments:
In `@src/core/pipeline.test.ts`:
- Around line 121-124: Add a new regression test case in the test suite for
selectApp() that exercises the TTY + --yes flag path with multiple apps. Create
a test following the same pattern as the existing test (refusing to guess with
multiple apps), but pass the --yes flag (as true or equivalent) instead of
undefined for the second parameter, and verify it throws an error referencing
the --app flag requirement. This will ensure the non-interactive contract is
maintained when both TTY and --yes are present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 357a00d7-69a2-49f0-8c29-d78b1ba217ba
📒 Files selected for processing (13)
src/cli/commands/creds.test.tssrc/cli/commands/creds.tssrc/core/config.test.tssrc/core/config.tssrc/core/pipeline.test.tssrc/core/pipeline.tssrc/core/prompt.test.tssrc/core/prompt.tssrc/core/remoteBuild.tssrc/core/remotePipeline.tssrc/core/toolchain.test.tssrc/core/toolchain.tssrc/providers/compute/awsEc2Mac.ts
Comment on lines
+95
to
+125
| /** | ||
| * After a secret has been imported into the OS keychain (now the source of truth), offer to remove the | ||
| * plaintext source file when it's sitting in a discovery directory. Honors the project invariant that | ||
| * secrets live only in the keychain — it never relocates the file into `~/.launch`, only deletes it. | ||
| * | ||
| * Interactive: prompt, defaulting to delete. Non-interactive: keep it and print the exact path, so a key | ||
| * is never deleted without consent (and the user still learns it's safe to remove). | ||
| */ | ||
| async function offerToRemoveImportedSecret(file: string, label: string, canPrompt: boolean): Promise<void> { | ||
| if (!isInDiscoveryDir(file)) return; | ||
| if (!canPrompt) { | ||
| console.log( | ||
| `The plaintext ${label} is still at ${tildify(file)} — it's in your keychain now, so you can delete it.`, | ||
| ); | ||
| return; | ||
| } | ||
| const remove = await confirm({ | ||
| message: `Remove the plaintext ${label} from ${tildify(file)}? It's now stored securely in your keychain.`, | ||
| initialValue: true, | ||
| }); | ||
| if (isCancel(remove)) { | ||
| cancel("Cancelled."); | ||
| process.exit(0); | ||
| } | ||
| if (!remove) { | ||
| console.log(`Kept ${tildify(file)}.`); | ||
| return; | ||
| } | ||
| rmSync(file, { force: true }); | ||
| console.log(`Removed ${tildify(file)}.`); | ||
| } |
There was a problem hiding this comment.
This abstraction hard-codes the wrong .p8 cleanup contract.
The helper only offers delete-or-keep, and the implementation/comment explicitly rule out moving the imported key into ~/.launch/credentials/AuthKey_<KEYID>.p8. That means the iOS path still cannot satisfy Issue #4’s required move / delete / leave flow, default to the managed copy, or enforce the 700/600 permission contract for the retained plaintext file. This is called out in the linked Issue #4 objective.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/commands/creds.ts` around lines 95 - 125, The
offerToRemoveImportedSecret function currently implements only a two-way
contract (delete or keep), but it needs to support a three-way contract that
includes moving the imported key to a managed location (~/.launch/credentials/).
Extend the function to offer three options: move to managed location (default),
delete, or keep. When moving, construct the target path as
~/.launch/credentials/AuthKey_<KEYID>.p8 (extracting the key ID from the file),
create the directory if needed, move the file there, and enforce 700 permissions
on the directory and 600 on the file. Update the prompts and logic in
offerToRemoveImportedSecret to reflect this three-way flow, ensuring the managed
copy is the default choice. This ensures the iOS path can satisfy the
requirements described in Issue `#4` for moving/deleting/keeping with proper
permission enforcement.
Comment on lines
+320
to
+325
| # Fail fast on the host if gym produced no non-empty .ipa, so we don't waste a transfer on a dead | ||
| # export — the authoritative device-archive guard runs locally after pull. | ||
| IPA="$(ls "$OUT"/*.ipa 2>/dev/null | head -1)" | ||
| if [ -z "$IPA" ] || [ ! -s "$IPA" ]; then | ||
| echo "LAUNCH_NO_ARTIFACT: gym produced no non-empty .ipa in $OUT" >&2 | ||
| exit 1 |
There was a problem hiding this comment.
Check the exact output file, not “any IPA in $OUT”.
$OUT persists between runs, so this ls … | head -1 can pick yesterday’s ${APP_NAME}.ipa when the current export fails. pullArtifact() then downloads that stale file from the same directory and can store or submit it under the new build number. Under set -euo pipefail, the assignment can also exit before LAUNCH_NO_ARTIFACT when no file matches. Remove the expected output before gym and validate "$OUT/$APP_NAME.ipa" with -s afterwards.
Suggested fix
mkdir -p "$OUT"
+rm -f "$OUT/$APP_NAME.ipa" "$OUT/App Thinning Size Report.txt"
@@
-IPA="$(ls "$OUT"/*.ipa 2>/dev/null | head -1)"
-if [ -z "$IPA" ] || [ ! -s "$IPA" ]; then
+IPA="$OUT/$APP_NAME.ipa"
+if [ ! -s "$IPA" ]; then
echo "LAUNCH_NO_ARTIFACT: gym produced no non-empty .ipa in $OUT" >&2
exit 1
fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/remoteBuild.ts` around lines 320 - 325, The code uses a wildcard
glob to find any .ipa file in $OUT and check it, which can pick up stale IPA
files from previous builds if the current gym export fails, and may cause the
assignment to exit before the LAUNCH_NO_ARTIFACT error message under set -euo
pipefail. Before running gym, remove the expected output file
"$OUT/$APP_NAME.ipa" to ensure it doesn't exist from a previous build. Then
after gym completes, replace the ls wildcard pattern and head command with a
direct test of the specific expected file: validate "$OUT/$APP_NAME.ipa" exists
and is non-empty using the -s test operator, rather than searching for any
matching .ipa file.
Comment on lines
+121
to
+126
| if (canInstall) { | ||
| // Put Homebrew on PATH in a non-interactive SSH shell before any install attempt. | ||
| lines.push( | ||
| `eval "$(/opt/homebrew/bin/brew shellenv 2>/dev/null || /usr/local/bin/brew shellenv 2>/dev/null || true)"`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Normalize Homebrew PATH in every remote SSH script, not just install-mode doctor.
brew shellenv only runs here when mode === "install", and it only affects this one SSH process. BYO assert runs will falsely report missing node/pod/fastlane on hosts where Homebrew lives under /opt/homebrew/bin, and AWS builds can still fail later because runBuildOnHost() starts a fresh shell. Hoist this PATH bootstrap out of the install-only branch and prepend the same snippet to the build/bootstrap scripts as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/toolchain.ts` around lines 121 - 126, The Homebrew shellenv setup is
currently only executed when canInstall is true, causing subsequent SSH
processes (for assert runs, builds, or when runBuildOnHost() starts a fresh
shell) to fail to find Homebrew-installed tools. Move the lines.push() call
containing the brew shellenv command outside and before the if (canInstall)
block so that the Homebrew PATH initialization runs unconditionally for all
remote SSH scripts regardless of the mode. This ensures that node, pod,
fastlane, and other Homebrew-installed tools are available on the PATH for every
SSH process initiated, not just during install mode.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/core/remoteBuild.ts">
<violation number="1" location="src/core/remoteBuild.ts:322">
P2: The new IPA lookup can terminate the script before the explicit `LAUNCH_NO_ARTIFACT` check runs, due to `set -euo pipefail` and `ls` failing when no match exists.</violation>
</file>
<file name="src/core/pipeline.ts">
<violation number="1" location="src/core/pipeline.ts:165">
P1: `canPrompt: process.stdin.isTTY` does not account for CI environments that expose a TTY (e.g., some hosted runners set `CI=true` but still have a pseudo-TTY). This can trigger an interactive picker and hang CI builds. Use a CI-aware interactivity check (e.g., `process.stdin.isTTY && !process.env.CI`) to ensure CI is always treated as non-interactive.</violation>
</file>
<file name="src/cli/commands/creds.ts">
<violation number="1" location="src/cli/commands/creds.ts:123">
P2: `rmSync` can throw on permission or filesystem errors (e.g., read-only directory, ACL restrictions). Since the credential import already succeeded at this point, a deletion failure should not fail the command. Wrap in a try/catch and log a warning instead so post-import cleanup errors are non-fatal.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const hint = app.bundleId ?? app.packageName; | ||
| return hint ? { value: app, label: app.name, hint } : { value: app, label: app.name }; | ||
| }), | ||
| canPrompt: process.stdin.isTTY, |
There was a problem hiding this comment.
P1: canPrompt: process.stdin.isTTY does not account for CI environments that expose a TTY (e.g., some hosted runners set CI=true but still have a pseudo-TTY). This can trigger an interactive picker and hang CI builds. Use a CI-aware interactivity check (e.g., process.stdin.isTTY && !process.env.CI) to ensure CI is always treated as non-interactive.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/core/pipeline.ts, line 165:
<comment>`canPrompt: process.stdin.isTTY` does not account for CI environments that expose a TTY (e.g., some hosted runners set `CI=true` but still have a pseudo-TTY). This can trigger an interactive picker and hang CI builds. Use a CI-aware interactivity check (e.g., `process.stdin.isTTY && !process.env.CI`) to ensure CI is always treated as non-interactive.</comment>
<file context>
@@ -173,27 +156,15 @@ export async function selectApp(apps: AppDescriptor[], appName: string | undefin
+ const hint = app.bundleId ?? app.packageName;
+ return hint ? { value: app, label: app.name, hint } : { value: app, label: app.name };
+ }),
+ canPrompt: process.stdin.isTTY,
+ nonInteractive: { kind: "require", flagHint: "— pass --app <name> to choose one non-interactively." },
});
</file context>
Suggested change
| canPrompt: process.stdin.isTTY, | |
| canPrompt: process.stdin.isTTY && !process.env.CI, |
| IPA="$(ls "$OUT"/*.ipa | head -1)" | ||
| # Fail fast on the host if gym produced no non-empty .ipa, so we don't waste a transfer on a dead | ||
| # export — the authoritative device-archive guard runs locally after pull. | ||
| IPA="$(ls "$OUT"/*.ipa 2>/dev/null | head -1)" |
There was a problem hiding this comment.
P2: The new IPA lookup can terminate the script before the explicit LAUNCH_NO_ARTIFACT check runs, due to set -euo pipefail and ls failing when no match exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/core/remoteBuild.ts, line 322:
<comment>The new IPA lookup can terminate the script before the explicit `LAUNCH_NO_ARTIFACT` check runs, due to `set -euo pipefail` and `ls` failing when no match exists.</comment>
<file context>
@@ -292,7 +317,13 @@ fastlane gym \
-IPA="$(ls "$OUT"/*.ipa | head -1)"
+# Fail fast on the host if gym produced no non-empty .ipa, so we don't waste a transfer on a dead
+# export — the authoritative device-archive guard runs locally after pull.
+IPA="$(ls "$OUT"/*.ipa 2>/dev/null | head -1)"
+if [ -z "$IPA" ] || [ ! -s "$IPA" ]; then
+ echo "LAUNCH_NO_ARTIFACT: gym produced no non-empty .ipa in $OUT" >&2
</file context>
Suggested change
| IPA="$(ls "$OUT"/*.ipa 2>/dev/null | head -1)" | |
| IPA="$(find "$OUT" -maxdepth 1 -type f -name '*.ipa' -print -quit)" |
| console.log(`Kept ${tildify(file)}.`); | ||
| return; | ||
| } | ||
| rmSync(file, { force: true }); |
There was a problem hiding this comment.
P2: rmSync can throw on permission or filesystem errors (e.g., read-only directory, ACL restrictions). Since the credential import already succeeded at this point, a deletion failure should not fail the command. Wrap in a try/catch and log a warning instead so post-import cleanup errors are non-fatal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/commands/creds.ts, line 123:
<comment>`rmSync` can throw on permission or filesystem errors (e.g., read-only directory, ACL restrictions). Since the credential import already succeeded at this point, a deletion failure should not fail the command. Wrap in a try/catch and log a warning instead so post-import cleanup errors are non-fatal.</comment>
<file context>
@@ -80,6 +81,49 @@ const SEARCH_DIRS = [
+ console.log(`Kept ${tildify(file)}.`);
+ return;
+ }
+ rmSync(file, { force: true });
+ console.log(`Removed ${tildify(file)}.`);
+}
</file context>
Suggested change
| rmSync(file, { force: true }); | |
| try { | |
| rmSync(file, { force: true }); | |
| } catch (err) { | |
| console.log(`Warning: could not remove ${tildify(file)}: ${(err as Error).message}`); | |
| return; | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Closes #8. Closes #6. Closes #11. Closes #4.
Four open issues, each driven to a finishable spec via
/grill-me, then implemented. (#9 was already shipped in v0.5.0 and was closed separately.) Every commit passes the full gate:typecheck && lint && test && build.#8 —
fix(config): resolve thelaunch-storeimport on global installsA globally-installed
launchloaded the user'slaunch.config.tsthrough jiti, which resolved itsimport { defineConfig } from "launch-store"from the user's project — so a project with no locallaunch-storedep threwCannot find package 'launch-store'. Now a jiti alias pins the specifier to the running CLI's own public entry: zero local dep needed, and the config always binds to the exactdefineConfigof the CLI consuming it (no dual-package skew). Editor types stay an opt-indevDependency. Covered by a no-node_modulesloader test.#6 —
feat(remote): run the toolchain doctor + verify the artifact on remote buildsThe remote-Mac build assumed every tool was present and grabbed the artifact with a blind
ls *.ipa.REQUIRED_TOOLS(single source of truth) runs before every remote build — an AWS host we own brew-installs gaps; a BYO-SSH host is only asserted (never mutated) and fails with the samebrew install …hintslaunch doctorprints. The one-time golden-AMI bootstrap is realigned to the same list..ipa, then reuse the exported, testedassertDeviceArtifactafter pull as the authoritative simulator/.app/empty guard.#11 —
refactor(prompt): share one type-to-search pickerThe app picker already fuzzy-searched past a threshold (shipped in 83364b8), but the same flat-
selectpattern was duplicated in the.p8key picker. Extracted a singlecore/prompt.tspickOne(+ thefuzzyMatchfilter) and routed bothselectAppand the credentials key picker through it. The non-interactive policy is explicit per call site:selectAppnow refuses to guess which app to build with no TTY and points at--app(it previously hung on a prompt), while the key picker falls back to the newest match with a note.#4 —
feat(creds): offer to remove the imported key's plaintext sourceset-keyleft the plaintext.p8in~/Downloadsafter importing it into the keychain. Now, after a verified import, Launch offers to delete the source file when it sits in a scanned discovery dir (default yes); a key deliberately placed elsewhere (--p8 ~/vault/…) is never touched, and nothing is moved into~/.launch(which would break the secrets-only-in-keychain invariant — the issue's "relocate" option was unsafe). The iOS offer is gated on Apple verifying the stored key (a.p8downloads exactly once); the re-downloadable Play service-account JSON gets the same offer.🤖 Generated with Claude Code
Summary by cubic
Finishes four open items: fixes global
launch-storeconfig resolution, adds remote build doctor and artifact verification, unifies the picker UX, and offers to delete imported keys’ plaintext files for better security.New Features
.ipa, then re-verify after pull to block simulator/.app/empty archives..p8or Play service-account JSON when found in discovery dirs (default yes); never delete non-interactively and never relocate files.pickOnewith fuzzy search for large lists. App picker refuses to guess without a TTY and points to--app; key picker falls back to the newest match with a note.Bug Fixes
import { defineConfig } from "launch-store"via ajitialias to the running CLI’s entry, so global installs work without a locallaunch-storedependency (covered by a no-node_modules loader test).Written for commit ac75e28. Summary will update on new commits.
CodeAnt-AI Description
Finish open fixes for config loading, remote builds, shared pickers, and key cleanup
What Changed
launch.config.tseven when the project does not have a locallaunch-storedependency--apphint when running without a terminal.ipafiles before transfer, and the same artifact check is used after downloadImpact
✅ Fewer global install config failures✅ Clearer app and key selection✅ Fewer remote build and artifact issues💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
New Features
Improvements