fix(cli/build init): don't re-ask saved import-flow answers on resume#2309
Conversation
A resumed `build init --platform ios` import flow was asking the user again for their distribution mode AND for their .p8 file path, even when both were already saved in the per-app onboarding progress file — producing screens like "✔ API Key verified — Key: 66FGQZB566" (hydrated log from progress) sitting right next to "How do you want to provide the .p8 file?" (the prompt that has no business being there). Two unconditional setStep() calls were responsible: 1. `import-scanning` useEffect always routed to `import-distribution-mode` regardless of `progress.importDistribution`. 2. The `import-distribution-mode` Select's app_store branch always routed to `api-key-instructions`, regardless of `progress.completedSteps.apiKeyVerified`. Both now consult the new `getImportEntryStep` helper in progress.ts, which centralizes the resume-routing decision: - no progress / no importDistribution → import-distribution-mode - importDistribution = ad_hoc → import-pick-identity - app_store + apiKeyVerified → import-pick-identity - app_store + .p8 + keyId + issuerId → verifying-key - app_store + .p8 + keyId → input-issuer-id - app_store + .p8 → input-key-id - app_store + nothing → api-key-instructions 12 unit tests cover the full decision table including the exact real-world bug shape (apiKeyVerified set, .p8 already on disk, was being re-asked). No change to `getResumeStep` itself or to first-run behavior — `getImportEntryStep` only fires after a successful Keychain scan.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR centralizes onboarding step routing for iOS imports into a pure helper, integrates it into the UI so resumed sessions skip already-completed import steps, adds defensive ChangesResume-aware onboarding step routing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b8a7b8ca0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (progress.completedSteps.apiKeyVerified) | ||
| return 'import-pick-identity' |
There was a problem hiding this comment.
This branch skips the entire App Store Connect key path whenever apiKeyVerified is present, but it does not verify that the saved key material is still usable (for example, the .p8 file was moved/deleted between runs). In that resume scenario, the user is routed straight to identity/profile selection and only fails later in saving-credentials (or can even save without APPLE_KEY_* if no path is present), with no step that lets them refresh the key path. Before this change, app_store always re-entered the key flow and recovered from stale local paths.
Useful? React with 👍 / 👎.
…sting apiKeyVerified Code-review caught a real regression: the previous commit's apiKeyVerified short-circuit routed users straight from import-scanning to import-pick-identity, skipping verifying-key. If the saved .p8 file had been moved/deleted (or the key revoked on Apple's side) between runs, the user would only fail later inside saving-credentials — after identity selection, profile selection, AND the Keychain ACL prompt that fires during the .p12 export. Worse: a malformed progress file with no p8Path could silently save credentials missing APPLE_KEY_* entirely. Three fixes: 1. getImportEntryStep no longer short-circuits on apiKeyVerified. When all three inputs (p8Path / keyId / issuerId) are saved we go to verifying-key — a ~500ms Apple round-trip that catches both stale local files and Apple-side revocation, then routes to import-pick-identity on success. The user still doesn't see the .p8 picker UI on resume (which was the original bug). 2. getFreshToken now wraps readFile in try/catch and rethrows as NeedP8Error. handleError already routes NeedP8Error back to api-key-instructions, so a stale path produces a clean re-prompt instead of an ENOENT-decorated support-bundle screen. 3. doSaveCredentials adds a defensive guard: throws when needsAscKey but keyContent is empty after the recovery read. Prevents the silent save-without-APPLE_KEY_* path on malformed progress files. Updated test names + the bug-shape test to assert verifying-key (not import-pick-identity) as the expected resume target.
|
1 participant
Summary
A resumed
build init --platform iosimport flow was asking the user again for their distribution mode AND for their.p8file path, even when both were already saved in the per-app onboarding progress file. The most visible symptom: the screen showed "✔ API Key verified — Key: 66FGQZB566" (hydrated log, replayed from progress) sitting right next to "How do you want to provide the .p8 file?" (the prompt that has no business being there).Concrete trace against a real
~/.capgo-credentials/onboarding/<appId>.json:{ "setupMethod": "import-existing", "importDistribution": "app_store", "completedSteps": { "apiKeyVerified": { "keyId": "…", "issuerId": "…" } }, "p8Path": "/path/to/AuthKey_….p8", "keyId": "…", "issuerId": "…" }getResumeStepreturned'import-scanning'(correct).import-scanninguseEffect ran the Keychain scan, then unconditionallysetStep('import-distribution-mode')— ignoringimportDistribution: "app_store"in progress.app_storebranch unconditionallysetStep('api-key-instructions')— ignoring the saved key inputs.So the user re-picked distribution, then was thrown to the .p8 file picker even though
APPLE_KEY_CONTENTand friends were already known.Fix
New
getImportEntryStep(progress)helper inprogress.tscentralizes the resume-routing decision after a successful Keychain scan:importDistributionimport-distribution-modeimportDistribution: ad_hocimport-pick-identityapp_store+ .p8 + keyId + issuerIdverifying-keyapp_store+ .p8 + keyIdinput-issuer-idapp_store+ .p8input-key-idapp_store+ nothingapi-key-instructionsBoth call sites now use the helper:
import-scanninguseEffect:setStep(getImportEntryStep(await loadProgress(appId)))app_storebranch:setStep(getImportEntryStep(existing))(uses the just-saved progress so a re-picked mode still flows correctly)Why we don't short-circuit on
apiKeyVerifiedAn earlier version of this PR routed
apiKeyVerifiedresumes directly toimport-pick-identity, skippingverifying-key. Code review correctly flagged this as a regression: the saved key material could have gone stale (the .p8 file moved/deleted between runs, or the key revoked on Apple's side), and a short-circuit would only surface that failure insidesaving-credentials— after identity selection, profile selection, and the Keychain ACL prompt that fires during the .p12 export. A malformed progress file with nop8Pathcould even silently save credentials missingAPPLE_KEY_*entirely.Going through
verifying-keyon every resume is a ~500ms Apple round-trip that catches both failure modes early. The user still doesn't see the .p8 picker UI on resume (which was the original bug) — they see a brief spinner, then land at the identity picker.Three defense-in-depth changes back this up:
getImportEntrySteproutes(app_store + .p8 + keyId + issuerId)toverifying-key, regardless ofapiKeyVerified.getFreshTokenwrapsreadFilein try/catch and rethrows asNeedP8Error.handleErroralready routesNeedP8Errorback toapi-key-instructions, so a stale path produces a clean re-prompt instead of an ENOENT-decorated support-bundle screen.doSaveCredentialsadds a defensive guard: throws whenneedsAscKey && !keyContentafter the recovery read. Prevents the silent save-without-APPLE_KEY_*path on malformed progress files.Test plan
bun run typecheck— cleanbunx eslinton touched files — only pre-existing main errors remain, none newbun test/test-onboarding-progress.mjs— 12 tests covering the full decision table, including a test case shaped exactly like the real-world bug report (assertsverifying-key, notimport-pick-identity)bun test/test-onboarding-recovery.mjs— all passbun test/test-macos-signing.mjs— all passbun run build— clean~/.capgo-credentials/onboarding/<appId>.json(apiKeyVerifiedset,importDistribution: app_store), re-runbuild init --platform iosand verify the flow lands onverifying-key(brief spinner) thenimport-pick-identity— no distribution-mode picker, no .p8 file picker.p8moved/deleted — verify the user gets routed back toapi-key-instructionsfor a clean re-prompt (NeedP8Error path), not a support-bundle error screenimportDistribution: ad_hocand nothing else, verify direct landing onimport-pick-identityRelated
import-pick-profile(different bug, same flow)Summary by CodeRabbit