feat(onboarding): shared engine for iOS + Android, TUI as render-only wrapper (PR 1)#2495
Merged
Conversation
…ship test hooks from the bundle + CI leak guard Proven on the real CLI build: a dev-gated branch importing a src/__dev__/ module is fully DCE'd from dist/index.js (marker absent). Apple-API spoofing for AI tests (PR 2) will live in src/__dev__/ behind this gate, physically absent from the NPM build.
…ndroidOnboardingProgress Brings the platform-agnostic onboarding engine (mcp/*, flow.ts) + its additive shared deps (keystore/oauth-google/oauth-scopes/output-record/schemas) onto main. Unifies the data model: main's step set (incl. the 4 TUI-only ai-analysis-* + resume-prompt) plus the 3 MCP markers (activePlatform, keystorePasswordGenerated, keystorePasswordManual); flow.ts KIND_TABLE covers the TUI-only steps. main's android/ui/app.tsx kept verbatim (TUI render-only rewrite is a later plan). Behavior is moved/renamed only — NOT assumed correct; the android audit is Plan 2. Verified: typecheck 0 errors; engine 87 unit + e2e hermetic gate PASS; main's onboarding tests still pass. Covers plan Tasks 2-3 (one green commit; they don't split cleanly).
…stripped; shared engine still ships)
…non-empty) to match main; tighten gate
…gcp-project-create-name) to match main
…r-cancel gate (credentials-exist + backing-up) to match main — closes a data-safety gap
… engine (persistAndStep derives next via getAndroidResumeStep; behavior-preserving + dev mismatch guard) + sequencing smoke test
…rough the shared runAndroidEffect (behavior-preserving; logs kept)
…source-of-truth (remove redundant state mirrors; conservative)
…code now that the engine drives the flow
… android mapper (drop unsafe cast)
…oidEffect through the post-save tail)
saving-credentials now stashes the CI-secret entries (createCiSecretEntries) in transient before routing to ask-build, mirroring the Ink TUI's doSaveCredentials. Adds the additive post-save tail transient fields to AndroidStepCtx and the rebuildTailCredentials/tailCiSecretEntries helpers the later tail steps reuse to re-derive the same entries from progress.
detecting-ci-secrets discovers CI destinations and persists the single chosen target (GitHub → ask-github-actions-setup, GitLab → ask-ci-secrets, many → target-select). checking-ci-secrets resolves the GitHub repo label + existing keys and gates on confirm-secrets-push / confirm-ci-secret-overwrite. uploading-ci-secrets pushes the entries and branches with-workflow → pick-package-manager vs build-complete. Routing mirrors the Ink TUI tail.
…ep(s) exporting-env writes the 0o600 .env (defaultExportPath fallback) and routes an existing file to confirm-env-export-overwrite (persisting the resolved path); overwrite-and-export-env re-exports with overwrite=true. writing-workflow-file generates + writes .github/workflows/capgo-build.yml (overwrite) using the recorded build-script choice + secret keys. All finish at build-complete, mirroring the Ink TUI tail.
requesting-build fires capgo build request through the pre-bound requestBuildInternal dep (driver owns apikey/logger/silent). A successful build with pending CI entries routes to detecting-ci-secrets to offer the secret push; with no entries it finishes at build-complete. A failed build with a captured log surfaces the AI job id and routes to the TUI-only ai-analysis-prompt; otherwise build-complete. Mirrors the Ink TUI tail.
… transient (parity with TUI; no rebuild, no secrets on disk)
…e; android delegates
…mapIosViewToStepView)
…ient/deps surface + additive persisted fields
…ning/mobileprovision types (not placeholders)
…t-save tail through the shared tail module
… (caught by the private E2E suite) 1. backing-up: the engine's success log dropped the backup destination path — main's bespoke logged '✔ Backup saved · <path>' (app.tsx:1473) so the user can find the backup. Restored the path detail. 2. verifying-key: the engine's catch echoed the multi-line verification- failure message into the log pane AND returned the error route — the error screen renders the same message, so the advice list painted twice on one frame. Main routes to handleError only; dropped the onLog echo (the onInternalLog breadcrumb stays). Both pinned by the private e2e-tui goldens (setup-method-select.txt / verifying-key__invalid.txt): 79/79 journeys green after this.
Suite-side counterpart of this branch: hermetic CAPGO_SKIP_UPDATE_PROMPT
(npm published 8.1.9 and the self-update prompt blocked every journey's
first frame, on main too), the documented resume-overwrite journey flip
(the credentials-exist gate this branch adds), and the pre-authorized
1-line cert-limit-prompt golden re-record ('iOS distribution' → 'Apple
Distribution', user-approved). 79/79 journeys + drift guard green against
this branch's dist.
The merged ErrorStep now delegates its options to buildHelpMenuOptions (support first, 'Try again', 'Exit') — the old bare 'Retry' label this assertion pinned no longer exists on either branch. Assert the new menu shape (support entry + renamed retry) per the TUI-is-main's-reference rule. 25/25.
Brings in 20 commits from main, most notably PR #2458 (precompiled, signed & notarized macOS keychain helper packages) and PR #2489 (multi-channel bundle upload), plus release chores (cli 8.3.0 / app 12.164.0) and frontend/backend changes that do not touch onboarding. Conflict resolutions (playbook: engine-driven structure is the base; main's new features are ported in): - cli/package.json: UNION. Kept our script entries (android-tail-*, ios-*, dev-gate-stripped, platform-flow-contract, tail-engine-shared, ...) plus main's `test:helper-dce` entry, and merged the `test` chain as the union of both sides' segments — main's `bun run test:helper-dce` is inserted right after `bun run build`, matching main's position. The chain otherwise keeps every segment from both sides. Version stays at main's 8.3.0. - cli/build.mjs: UNION of both sides' `define` blocks (CLI + SDK builds): kept our `globalThis.__CAPGO_DEV__` / `globalThis.__CAPGO_MCP_ONBOARDING__` release gates AND main's `__CAPGO_ALLOW_HELPER_ENV_OVERRIDE__` DCE gate for the CAPGO_KEYCHAIN_HELPER_PATH dev override (PR #2458). - cli/src/build/onboarding/ui/app.tsx: ours is the engine-driven thin wrapper; main's bespoke import-effect bodies (deleted on our branch, modified by PR #2458 on main) were dropped, and main's actual change — the removal of the import-compiling-helper step — was ported into our engine-driven structure instead (see below). - bun.lock: took main's cli version stamp; `bun install` then reconciled it to 8.3.0. - private/cli-mcp-tests (submodule): fast-forwarded to main's pointer 71db228 ("test(compat): pin cert golden + resume journey to capgo main") — our pointer 8a615f2 is its direct ancestor, and 71db228 matches the merged code's helper contract. The local frozen suite reconciliation stays with the orchestrator. Port of PR #2458's import-compiling-helper removal (main is the reference for its feature; our engine consumes main's new precompiled-helper mechanism): - macos-signing.ts took main's version wholesale (resolveHelperBinary + signature-verified precompiled helper; precompileSwiftHelper/isHelperCached deleted). types.ts / error-categories.ts / ui/steps/ios-import.tsx merged clean to main's contract (step union, STEP_PROGRESS, error category, and ImportCompilingHelperStep component all gone). - ios/flow.ts (ours-only engine file): removed the import-compiling-helper effect case, the precompileSwiftHelper/isHelperCached deps, the helperCompiled transient + carried idempotency guard, and rerouted the import-export-warning resolver's 'go' branch straight to import-exporting. Docs updated to record the PR #2458 contract. - ui/app.tsx: dropped the removed imports, the engine-deps wiring for the two deleted callbacks, the step from IOS_ENGINE_IMPORT_EFFECT_STEPS, and the isHelperCached arg on the export-warning runIosEffect call. - Tests updated to main's contract (each pinned the removed step): - test-ios-import-export.mjs: collapsed the three 'go' resolver variants into one unconditional 'go' -> import-exporting case, deleted the import-compiling-helper effect section and the "NOT cached" driver journey, dropped the two deps from makeDeps. - test-ios-e2e.mjs: journey (b) no longer detours through the compile step; dropped the deps mocks, the isHelperCached overrides, and the step's error-route case in (e). - test-ios-tui-routing.mjs: replaced the cached/NOT-cached parity pair with a single unconditional 'go' MATCH case. - test-ios-tui-render.mjs: removed the ImportCompilingHelperStep render test + its import (component deleted by #2458). - test-ios-resume.mjs: dropped the step from EPHEMERAL_PICKER_STEPS. - test-ios-tail-handoff.mjs: comment-only update. Gates: tsc --noEmit clean; bun run build clean; test:helper-dce green (entry + chain segment + cli/scripts/check-helper-dce.sh all survive); test-dev-gate-stripped green (MCP onboarding tools still ABSENT — the PR-1 gate stays off); full `bun run test` chain exit 0. TUI E2E (frozen private suite @ 68a181d vs this dist): 79 journeys — 78 pass, 1 flaky (support flow → view logs → cancel; pass-on-retry). Runtime drift guard: ios 40/65 covered / 25 excluded / 0 missing; android 42/65 covered / 23 excluded / 0 missing. Known follow-up (private repo, orchestrator-owned): test:e2e-tui:harness fails at harness-tests/test-drift.mjs hygiene — "UNCOVERED key 'import-compiling-helper' is not a STEP_PROGRESS step id" — because the suite's restored exclusion (private PR #4) is orphaned now that this merge ports the step's removal.
Drift-guard bookkeeping for the main merge: this branch now ports main's import-compiling-helper step removal (PR #2458), so the private suite's restored exclusion (#4) became orphaned — 1044e70 re-applies #3 exactly. Harness self-tests + the 79-journey suite green against this branch's dist (78 pass + 1 pty flake, pass-on-retry; drift 0 missing both platforms).
Contributor
|
Warning Review limit reached
More reviews will be available in 3 hours, 34 minutes, and 53 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe CLI now uses build-time globals to gate dev and MCP onboarding behavior, adds shared onboarding contracts and tail-state types, refactors Android and iOS onboarding into engine-driven flows, and expands MCP onboarding tooling, validation, and CLI test coverage. ChangesCLI onboarding runtime
MCP onboarding tools
Sequence Diagram(s)sequenceDiagram
participant Server
participant OnboardingTools
participant Engine
participant OAuthSession
participant TerminalLaunch
Server->>OnboardingTools: registerOnboardingTools()
OnboardingTools->>Engine: gatherFacts()
OnboardingTools->>Engine: runStart() / runAdvance()
Engine->>OAuthSession: beginOAuthSession()
Engine->>TerminalLaunch: launchBuildInTerminal()
Engine-->>OnboardingTools: NextStepResult
OnboardingTools-->>Server: renderResult()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
|
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
🧪 Builder onboarding TUI preview — ✅ passed▶ Open the interactive HTML report (zoomable journey tree + cast playback) Commit: aa14642 · Job summary with the result table |
![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: 73fe414168
ℹ️ 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".
…read, bundle-id resolution, error truthfulness BLOCKER: buildIosSavedCredentials re-reads the .p8 from the persisted progress.p8Path via the injected deps.readFile when the carried key content was lost (crash/restart), and REFUSES to save an app_store credential map without the ASC key — pre-fix the resume silently wrote credentials missing APPLE_KEY_CONTENT/APPLE_KEY_ID/APPLE_ISSUER_ID and declared success (3-engine hostile-review consensus; regression vs main's guards). buildSavedCredentials may now be async (tail/flow.ts awaits it). Engine findings: - resolveIosBundleId: override → detected Release bundle id → appId, threaded into the provisioning-map key, the tail rebuild, and all six runIosEffect call sites (bundle id ≠ Capgo appId) - resolveP8Content wraps readFile: a stale p8Path surfaces the NeedP8-style re-provide error instead of a raw ENOENT - import-portal open-anyway: success log carries the URL; openExternal failure logs the visit-manually fallback instead of fabricating success - tail saving-credentials self-heal divert surfaces the underlying builder error (guidance line + internal log) - requesting-build honors deps.signal (pre/post/catch abort bails; limitation vs request.ts internals documented) - android backing-up: only ENOENT keeps the benign note; real copy failures log a truthful warning - gcp-setup-running strips _oauthRefreshToken from persisted progress after a successful revoke - createCertificate/createProfile rethrow the ORIGINAL Apple error when the follow-up list call also fails - rebuildIosTailCredentials restores APPLE_KEY_ID/APPLE_ISSUER_ID from progress (keyId/issuerId ?? apiKeyVerified) - preloadWorkflowScripts failures route through onInternalLog - test-ios-import-discovery: prefetch fixture uses the RAW AscProfileSummary shape so the synthesizeProfileFromAscSummary mapping is actually exercised
…tale closures, tail readFile wire - .p8 submit handlers no longer rewrite every failure to 'File not found': classifyP8SubmitError (new ui/p8-error.ts) keeps the exact copy for ENOENT only; other errors reach handleError with the real message, and every failure lands in the internal log - the three [step]-keyed drivers (create-new / import / tail) call handleError through a ref (handleErrorRef), eliminating stale-closure routing on error - NeedP8Error hoisted to module scope (stable instanceof across renders) - tail driver carried.savedCredentials built via an explicit filtered conversion instead of a lying Record<string,string> cast - tail driver deps now thread readFile (mirrors the create driver) so the engine's crash-recovery .p8 re-read works through the TUI — verified by the private e2e crash-recovery journeys (xfail pin promoted) - error sinks: support-flow and restart deleteProgress catches log instead of swallowing; cert-limit/duplicate-profile Select onChange guarded against the @inkjs/ui re-fire with one-shot refs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jinhongliang991013
left a comment
jinhongliang991013
left a comment
There was a problem hiding this comment.
I traced the post-save resume markers through both platform drivers. The resume router is implemented, but the production write path never creates the state it consumes.
| } | ||
|
|
||
| await deps.updateSavedCredentials(progress.appId, deps.platform, credentials) | ||
| await deps.deleteProgress(progress.appId) |
There was a problem hiding this comment.
[P1] Persist the tail milestones before deleting progress
This deletes progress.json immediately after credentials are written, but no production path persists completedSteps.credentialsSaved (or the later �uildRequested / ciSecretsUploaded markers). The only marker values under cli/src are the synthetic object in Android's ailEngineNext; iOS has no writer at all. The resume tests pass because they construct these markers directly in fixtures rather than exercising the real save/restart path.
As a result, exiting on �sk-build or any later tail screen leaves no progress file. The next �uild init cannot enter ailResumeStep, so the advertised total resume is lost and the user is sent through credential setup again; after a queued build or secret upload this can also repeat the remote side effect. Persist credentialsSaved plus the tail fields before leaving this step, persist �uildRequested/ciSecretsUploaded only after their side effects succeed, and defer deleting progress until �uild-complete.
- ui/app.tsx + android/ui/app.tsx: remove unused imports (BuildLogger, homedir, CertificateLimitError, DuplicateProfileError, bundleIdMatches) and _-prefix two set-but-never-read state values (pendingRecoveryAction, duplicateProfileOrigin) — oxlint no-unused-vars, zero behavior change - android/flow.ts: drop the unused androidStepView export (knip dead-code) - mcp/engine.ts: remove the dead keystoreFileWritten=true assignment in the first write-block (that block returns at mapAndroidView before the loop can re-read it; the live write-once guard is the second site) — code-quality nit - test-ios-import-recovery.mjs: assert the opened/warned portal URL via RegExp.test() instead of String.includes(<url-literal>) so it is no longer a CodeQL incomplete-URL-substring-sanitization sink (it is a log-message check, not URL validation) - typos: reword MATCH* comment false-positives + the SER1 cert fixture value
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/build/onboarding/android/oauth-google.ts (1)
444-462:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the loopback timer/server on a pre-aborted signal.
If
signal.abortedis already true, Line 458 returns beforeclose()andcodePromise.then(close, close)are wired. That leaves the 5-minute timer armed and the loopback server unclosed, so cancelling the flow can keep the CLI process alive until timeout.Suggested fix
const timer = setTimeout(() => { codeReject(new Error(`Timed out after ${Math.round(args.timeoutMs / 1000)}s waiting for browser sign-in`)) }, args.timeoutMs) + function close() { + clearTimeout(timer) + if (args.signal) + args.signal.removeEventListener('abort', onAbort) + server.close() + } + function onAbort() { const err = new Error('OAuth flow aborted') + close() codeReject(err) // Also reject the outer Promise. Before `server.listen` resolves this is // the only path to surface the abort to the caller; afterwards // `rejectHandle` is a no-op on an already-resolved promise. rejectHandle(err) @@ - function close() { - clearTimeout(timer) - if (args.signal) - args.signal.removeEventListener('abort', onAbort) - server.close() - } - // Always clean up on either outcome. codePromise.then(close, close)🤖 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 `@cli/src/build/onboarding/android/oauth-google.ts` around lines 444 - 462, If args.signal.aborted is true the function returns before the loopback server and timeout are cleaned up; ensure you close the server and cancel the timer when pre-aborted by calling the existing cleanup helpers before returning. Concretely, after calling onAbort() in the pre-abort branch, also invoke the cleanup/close function used later (close) and clear the timeout (clearTimeout(timer)) so the loopback server is closed and the timer cleared (or alternatively move the wiring of codePromise.then(close, close) and timeout setup to before the aborted check so the same cleanup path runs); reference the symbols timer, onAbort, close, codePromise.then(close, close), and args.signal.aborted to locate and apply the change.
♻️ Duplicate comments (1)
cli/src/build/onboarding/tail/flow.ts (1)
786-818:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPersist the tail milestones before deleting progress.json.
This still writes credentials and immediately deletes progress, but none of the shared tail effects record the milestone markers that resume logic consumes (
completedSteps.credentialsSaved,buildRequested,ciSecretsUploaded).cli/src/build/onboarding/ios/progress.tsresumes the post-save flow entirely from those markers, so exiting onask-buildor any later tail screen drops the user back out of the tail and reopens the double-build / double-secret-upload risk on retry.🤖 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 `@cli/src/build/onboarding/tail/flow.ts` around lines 786 - 818, The code deletes progress.json before persisting the tail milestone markers the resume logic needs; fix by setting the resume markers on the progress object (e.g. ensure progress.completedSteps = progress.completedSteps || {}; set progress.completedSteps.credentialsSaved = true; set progress.buildRequested and progress.ciSecretsUploaded as appropriate) and persist that updated progress before calling deps.deleteProgress; call the repository's persistence helper (e.g. deps.updateProgress(progress) or deps.saveProgress(progress) — or, if the codebase only exposes a specific persister, include those markers in the call that writes progress/credentials (ensure you persist them after deps.updateSavedCredentials(...) and before deps.deleteProgress), then continue to createCiSecretEntries(...) and return the same next/transient payload.
🤖 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 `@cli/src/build/onboarding/android/oauth-scopes.ts`:
- Around line 7-10: There’s a duplicate local OAUTH_SCOPES_FOR_ONBOARDING
defined in the Android onboarding UI; remove that local definition and import
the exported OAUTH_SCOPES_FOR_ONBOARDING (the constant exported alongside
GOOGLE_OAUTH_SCOPES_ANDROIDPUBLISHER) and use it in place of the local variable.
Add the import at the top of the module, delete the local constant (or replace
its references), and ensure all usages in the onboarding UI now reference the
imported OAUTH_SCOPES_FOR_ONBOARDING to prevent scope drift.
In `@cli/src/build/onboarding/android/ui/app.tsx`:
- Around line 841-848: The retry target on persistence failure is incorrectly
using nextStep (which may advance past the failed save) — change the error
routing so save/write failures always retry the current step: in the block that
calls handleErrorRef.current?.(err, nextStep ?? stepRef.current), pass
stepRef.current explicitly (handleErrorRef.current?.(err, stepRef.current)) so
the retry/UX returns to the step that failed to persist; leave
getAndroidResumeStep behavior unchanged.
In `@cli/src/build/onboarding/env-export.ts`:
- Around line 9-14: The comment claiming the `.env.capgo.<appId>.<platform>`
file is to be "committed manually" is incorrect; update the v1 contract comment
that contains the literal `.env.capgo.<appId>.<platform>` to state this file is
a local secret that MUST NOT be committed, that the code performs NO git
operations, and advise adding it to `.gitignore` (remove the phrase "that the
USER commits manually"); make the same change to the other identical comment
instance referenced (the one around lines 29-34) so both places consistently
indicate the file must remain local and uncommitted.
In `@cli/src/build/onboarding/mcp/app-id-validation.ts`:
- Around line 25-35: The regex SAFE_APP_ID used by isSafeAppIdForCommand
currently allows identifiers without a dot (e.g., "my-app"), which violates the
reverse-domain requirement; update SAFE_APP_ID to enforce at least one '.' by
adding a positive lookahead that requires a dot before the rest of the pattern
(so the pattern still enforces the same token rules but must contain a dot),
then keep isSafeAppIdForCommand unchanged to use the new SAFE_APP_ID.
In `@cli/src/build/onboarding/mcp/engine.ts`:
- Around line 1533-1538: resolveCurrentState currently only checks for a single
detected platform and otherwise returns 'platform-select', which misreports when
facts.platformsDetected is empty; update resolveCurrentState to explicitly
handle the no-platform case by returning 'no-platform' when
facts.platformsDetected.length === 0 (before the existing single-platform
checks), leaving the getAndroidResumeStep(androidProgress) and 'ios-api-key'
branches intact so behavior matches decideStart.
In `@cli/src/build/onboarding/tail/flow.ts`:
- Around line 423-427: The TAIL_KIND mapping incorrectly marks the
'ci-secrets-setup' TailStep as 'auto' while tailViewForStep() expects it to be a
'choice' (so retry/skip options render correctly); update the TAIL_KIND constant
to set 'ci-secrets-setup' => 'choice' (i.e., change the value for the
'ci-secrets-setup' key in the TAIL_KIND Record<TailStep, TailStepKind>).
- Around line 384-388: The options array OPTIONS_ASK_GITHUB_ACTIONS_SETUP emits
a value 'no' for the decline branch which conflicts with the expected 'declined'
used by TailEffectProgress.setupMode and applyTailInput(); change the third
entry's value from 'no' to 'declined' so the menu contract matches the shared
decline token and avoids ad-hoc remapping in drivers.
In `@cli/src/build/output-record.ts`:
- Around line 107-119: The function readBuildOutputRecord currently only
validates jobId, status and outputUrl before casting; extend its runtime
validation to verify the full BuildOutputRecord shape (at minimum ensure fields
platform and buildMode are strings and finishedAt is present with an appropriate
primitive type such as number or string) and only return parsed as
BuildOutputRecord when all required fields pass type checks; if validation
fails, return null. Update the checks inside readBuildOutputRecord to explicitly
test these additional properties rather than relying on a partial cast so
downstream consumers don’t receive malformed objects.
In `@cli/test/test-ios-import-discovery.mjs`:
- Around line 15-16: The test's routing expectation is inconsistent: the
'__cancel__' routing intent is documented to route to 'api-key-instructions' but
the assertion for getImportEntryStep(...) expects 'import-distribution-mode';
update the assertion(s) so they expect 'api-key-instructions' (or alternately
update the documented intent to match if that is the true contract) and make the
same change for the duplicate occurrences around lines 273-282; specifically,
find the test(s) that call getImportEntryStep(...) and replace the
'import-distribution-mode' expectation with 'api-key-instructions' so the spec
and assertion align with the router under test.
---
Outside diff comments:
In `@cli/src/build/onboarding/android/oauth-google.ts`:
- Around line 444-462: If args.signal.aborted is true the function returns
before the loopback server and timeout are cleaned up; ensure you close the
server and cancel the timer when pre-aborted by calling the existing cleanup
helpers before returning. Concretely, after calling onAbort() in the pre-abort
branch, also invoke the cleanup/close function used later (close) and clear the
timeout (clearTimeout(timer)) so the loopback server is closed and the timer
cleared (or alternatively move the wiring of codePromise.then(close, close) and
timeout setup to before the aborted check so the same cleanup path runs);
reference the symbols timer, onAbort, close, codePromise.then(close, close), and
args.signal.aborted to locate and apply the change.
---
Duplicate comments:
In `@cli/src/build/onboarding/tail/flow.ts`:
- Around line 786-818: The code deletes progress.json before persisting the tail
milestone markers the resume logic needs; fix by setting the resume markers on
the progress object (e.g. ensure progress.completedSteps =
progress.completedSteps || {}; set progress.completedSteps.credentialsSaved =
true; set progress.buildRequested and progress.ciSecretsUploaded as appropriate)
and persist that updated progress before calling deps.deleteProgress; call the
repository's persistence helper (e.g. deps.updateProgress(progress) or
deps.saveProgress(progress) — or, if the codebase only exposes a specific
persister, include those markers in the call that writes progress/credentials
(ensure you persist them after deps.updateSavedCredentials(...) and before
deps.deleteProgress), then continue to createCiSecretEntries(...) and return the
same next/transient payload.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13ff12bf-1780-4ef7-8c8c-d2c35d478593
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (61)
cli/build.mjscli/package.jsoncli/src/__dev__/README.mdcli/src/build/mobileprovision-parser.tscli/src/build/onboarding/android/flow.tscli/src/build/onboarding/android/keystore.tscli/src/build/onboarding/android/oauth-google.tscli/src/build/onboarding/android/oauth-scopes.tscli/src/build/onboarding/android/progress.tscli/src/build/onboarding/android/types.tscli/src/build/onboarding/android/ui/app.tsxcli/src/build/onboarding/apple-api.tscli/src/build/onboarding/env-export.tscli/src/build/onboarding/flow/android-flow.tscli/src/build/onboarding/flow/contract.tscli/src/build/onboarding/flow/ios-flow.tscli/src/build/onboarding/ios/flow.tscli/src/build/onboarding/ios/progress.tscli/src/build/onboarding/mcp/app-id-validation.tscli/src/build/onboarding/mcp/contract.tscli/src/build/onboarding/mcp/engine.tscli/src/build/onboarding/mcp/explanations.tscli/src/build/onboarding/mcp/oauth-session.tscli/src/build/onboarding/mcp/onboarding-tools.tscli/src/build/onboarding/mcp/step-input.tscli/src/build/onboarding/mcp/terminal-launch.tscli/src/build/onboarding/tail-types.tscli/src/build/onboarding/tail/flow.tscli/src/build/onboarding/types.tscli/src/build/onboarding/ui/app.tsxcli/src/build/onboarding/ui/p8-error.tscli/src/build/onboarding/ui/steps/ios-credentials.tsxcli/src/build/output-record.tscli/src/dev-flag.d.tscli/src/mcp/server.tscli/src/schemas/onboarding.tscli/test/dev-preload.mjscli/test/test-android-tail-engine.mjscli/test/test-android-tail-render.mjscli/test/test-android-tail-routing.mjscli/test/test-android-tui-sequencing.mjscli/test/test-apple-api-cert-create.mjscli/test/test-dev-gate-stripped.mjscli/test/test-ios-confirm-app-id.mjscli/test/test-ios-create-new.mjscli/test/test-ios-e2e.mjscli/test/test-ios-flow-contract.mjscli/test/test-ios-import-discovery.mjscli/test/test-ios-import-export.mjscli/test/test-ios-import-pickers.mjscli/test/test-ios-import-recovery.mjscli/test/test-ios-recovery.mjscli/test/test-ios-resume.mjscli/test/test-ios-tail-handoff.mjscli/test/test-ios-tui-render.mjscli/test/test-ios-tui-routing.mjscli/test/test-ios-verify-app.mjscli/test/test-p8-error.mjscli/test/test-platform-flow-contract.mjscli/test/test-tail-engine-shared.mjsprivate/cli-mcp-tests
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
…ule, tail decline mapping - env-export.ts: the .env.capgo.* file holds credentials; reword the docs so they no longer imply the user commits it — keep it out of version control - mcp/app-id-validation.ts: enforce the documented reverse-domain rule in code (require a dot) so single-label ids like `my-app` no longer pass the gate - tail/flow.ts: applyTailInput now maps the user-facing 'no' option to the canonical persisted setupMode 'declined' (mirrors the TUI drivers), so the shared-engine path's resume routing (setupMode === 'declined') matches; the ask-github-actions-setup input type now reflects the real 'no' value - test-ios-import-recovery.mjs: drop the now-unused PORTAL_PROFILES_URL const - test-ios-import-discovery.mjs: fix a stale __cancel__ doc/name that claimed api-key-instructions; the assertion verifies the distribution fork
|
WcaleNieWolny
added a commit
that referenced
this pull request
Jun 15, 2026
… squash-merge) PR-1 (#2495) was squash-merged into main, breaking shared-commit ancestry, so this merge surfaced ~20 add/add conflicts on the MCP/onboarding files even though PR-2 is a content superset of PR-1. Resolved by taking PR-2's version for all onboarding/MCP source + tests (verified: the only main-only commits touching them were the PR-1 squash + release bumps — no separate fix to fold in), keeping main's auto-merged package.json version (8.4.0) plus PR-2's test scripts, and pinning the cli-mcp-tests submodule to PR-2's c511d08. Also folds in the pre-existing dead-code gate fix (PR-2's own exports, never in main): delete unused clearTailParked; knip-ignore the test-only exports clearAllSessions / MCP_ONLY_STATES / MCP_UNREACHABLE_STEPS (consumed only by the private cli-mcp-tests suite knip can't see). Gates green post-merge: typecheck, lint, knip, build, instructions test, + 6 targeted onboarding/MCP tests.
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
3 participants
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.
What
The Builder onboarding mega-PR: both platforms' onboarding now runs on ONE shared headless engine, with the ink TUI as a render-only wrapper over the entire flow — including the post-save tail (CI secrets, env export, workflow file, build request). The MCP onboarding integration stays build-time gated OFF (
__CAPGO_MCP_ONBOARDING__= false; PR #2492 — stacked on this branch — completes and flips it).Architecture
flow/contract.ts(PlatformFlow/StepView) +ios/flow.ts+android/flow.ts+ the platform-neutraltail/flow.ts. Pure views/reducers/effects; zero ink imports.getIosResumeStep/android twin route gates (credentials-exist), the .p8 chain, verify-app, the import fork, and the marker-guarded tail.Deliberate behavior changes (3 — everything else is parity)
DISTRIBUTION-type certs instead of deprecatedIOS_DISTRIBUTION; the cert-limit revoke picker scopes to the pool that's actually full (the old mixed list defaulted the cursor onto the user's production cert).An adversarial audit verified these are the ONLY behavior changes: running the pre-change private TUI suite (which characterized main) against this branch's dist goes red on exactly those — nothing else. Test diff vs main is 100% additive (23 new files, +10.6k/-0).
Tested
test:helper-dcepreserved through the merge.Merge notes
1044e70.Summary by CodeRabbit