Skip to content

feat: add build onboarding for automated iOS credential setup#547

Merged
WcaleNieWolny merged 15 commits intomainfrom
feat/ios-onboarding
Mar 20, 2026
Merged

feat: add build onboarding for automated iOS credential setup#547
WcaleNieWolny merged 15 commits intomainfrom
feat/ios-onboarding

Conversation

@WcaleNieWolny
Copy link
Contributor

@WcaleNieWolny WcaleNieWolny commented Mar 19, 2026

Summary

  • Adds npx @capgo/cli build onboarding — an interactive Ink-based CLI flow that automates iOS certificate and provisioning profile creation via the App Store Connect API
  • User provides ONE API key (.p8 file), and the CLI handles everything: CSR generation, certificate creation, bundle ID registration, provisioning profile creation, credential saving, and optional first build
  • Refactors requestBuildInternal to use a BuildLogger callback interface instead of direct @clack/prompts logging, enabling clean integration with the Ink UI

What it does

Before: ~10 manual steps across Apple Developer Portal, App Store Connect, and the CLI before a user can run their first iOS cloud build.

After: 1 manual step (create API key in App Store Connect) + 1 command (build onboarding).

Features

  • macOS native file picker for .p8 selection (falls back to path input on other OSes)
  • Auto-detects Key ID from .p8 filename (AuthKey_XXXX.p8)
  • Progress persistence in ~/.capgo-credentials/onboarding/<appId>.json — resume from interruption
  • Smart conflict resolution: cert limit reached → ask user which to revoke; duplicate profiles → auto-cleanup
  • Ctrl+O to open App Store Connect in browser during input steps
  • Live build output streaming in the terminal UI
  • Auto-exit after build completion

Architecture

src/build/onboarding/
  ├── command.ts          ← Commander.js entry point, launches Ink
  ├── types.ts            ← Shared types, step progress mapping
  ├── progress.ts         ← Per-app progress persistence
  ├── apple-api.ts        ← JWT auth + App Store Connect API (verify, certs, profiles, bundle IDs)
  ├── csr.ts              ← CSR generation + P12 creation via node-forge
  ├── file-picker.ts      ← macOS native file picker via osascript
  └── ui/
      ├── app.tsx          ← Main Ink app (state machine)
      └── components.tsx   ← Reusable UI pieces (spinner, success line, etc.)

BuildLogger refactoring

requestBuildInternal now accepts an optional BuildLogger callback interface with info, error, warn, success, buildLog, and uploadProgress methods. This replaces all direct @clack/prompts and console.log calls, making the function usable from any UI (CLI, Ink, tests, etc.).

Test plan

  • Run build onboarding in a Capacitor project with an ios/ directory
  • Verify full flow: .p8 input → key verification → cert creation → profile creation → credential save → build
  • Test resume: interrupt mid-flow (Ctrl+C), re-run, verify it picks up where it left off
  • Test error recovery: provide invalid .p8, verify retry prompt works
  • Test cert limit: trigger cert limit error, verify revoke-and-retry flow
  • Test profile duplicate: verify duplicate detection and cleanup
  • Verify build request still works normally (BuildLogger backwards compatibility)
  • Verify non-macOS: file picker option should not appear

Summary by CodeRabbit

  • New Features

    • Interactive "build onboarding" CLI to guide iOS App Store Connect setup, create/revoke certificates and provisioning profiles, and optionally trigger a build.
    • Guided onboarding UI with resumable progress, retries, conflict-resolution prompts, and macOS .p8 file picker.
    • CSR and PKCS#12 generation plus team-id extraction to support certificate workflows.
    • Enhanced build logging with structured progress updates and streaming integration.
  • Documentation

    • Expanded onboarding usage and behavior in native-builds skill.

Interactive Ink-based CLI flow that automates iOS certificate and
provisioning profile creation via the App Store Connect API.

User provides ONE API key (.p8 file), and the CLI handles:
- CSR generation and iOS Distribution certificate creation
- Bundle ID registration (or reuse)
- App Store provisioning profile creation
- Credential saving to ~/.capgo-credentials/credentials.json
- Optional first build kick-off with live log streaming

Key features:
- macOS native file picker for .p8 selection
- Auto-detect Key ID from .p8 filename
- Progress persistence in ~/.capgo-credentials/onboarding/<appId>.json
- Resume from interruption without losing progress
- Smart cert/profile conflict resolution (revoke & recreate)
- Ctrl+O to open App Store Connect in browser

Also refactors requestBuildInternal to use a BuildLogger callback
interface instead of direct @clack/prompts logging, enabling clean
integration with the Ink UI without stdout interception hacks.
@coderabbitai
Copy link

coderabbitai bot commented Mar 19, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds an interactive iOS App Store Connect onboarding flow: JWT auth, CSR/P12 utilities, certificate/profile lifecycle (create/revoke), macOS .p8 picker, resumable progress persistence, an Ink-based UI, BuildLogger refactor, small build plugin stub, and JSX compiler enablement.

Changes

Cohort / File(s) Summary
Build infra & config
build.mjs, tsconfig.json, package.json
Added Bun/esbuild plugin stub for react-devtools-core, enabled jsx: "react-jsx", and introduced runtime/dev deps (react, ink, jsonwebtoken, node-forge, etc.).
App Store Connect client
src/build/onboarding/apple-api.ts
New ASC client: ES256 JWT generation, authenticated requests, cert listing/creation/revocation, bundle ID and provisioning profile operations, and domain errors (CertificateLimitError, DuplicateProfileError).
CSR / crypto utilities
src/build/onboarding/csr.ts
RSA-2048 CSR generation, team-id extraction from cert, PKCS#12 creation (base64) and exported types/constants.
File picker
src/build/onboarding/file-picker.ts
macOS .p8 file picker via osascript; canUseFilePicker() and openFilePicker() (returns null on cancel/non-darwin).
Onboarding types & persistence
src/build/onboarding/types.ts, src/build/onboarding/progress.ts
New onboarding types/steps, STEP_PROGRESS mapping, secure disk-backed per-app progress load/save/delete and resume-step logic.
Ink UI & components
src/build/onboarding/ui/app.tsx, src/build/onboarding/ui/components.tsx
Full Ink onboarding app (state machine, retries, token refresh, cert/profile flows, optional build kickoff) and presentational components (spinner, success/error, filtered input, header).
CLI command & wiring
src/index.ts, src/build/onboarding/command.ts
Registered build onboarding Commander command and implemented onboardingCommand to load progress and render the Ink app.
Build request logging refactor
src/build/request.ts
Introduced BuildLogger abstraction and threaded logger through fetchWithRetry, streamBuildLogs, pollBuildStatus, and requestBuildInternal; replaced many silent/console branches with logger-driven outputs.

Sequence Diagram

sequenceDiagram
    autonumber
    actor User
    participant CLI as CLI
    participant FS as Filesystem
    participant UI as Ink_UI
    participant Picker as macOS_FilePicker
    participant ASC as App_Store_Connect
    participant Build as Build_Service

    User->>CLI: run `capgo build onboarding`
    CLI->>FS: loadProgress(appId)
    CLI->>UI: render OnboardingApp(appId, initialProgress)

    UI->>User: prompt platform & .p8 entry
    alt use macOS picker
        User->>Picker: open file chooser
        Picker-->>UI: selected .p8 path
        UI->>FS: read .p8 content
    else manual entry
        User-->>UI: enter path & IDs
        UI->>FS: read .p8 content
    end

    UI->>ASC: verify JWT / list certs
    ASC-->>UI: teamId / certs

    UI->>UI: generate CSR (local)
    UI->>ASC: POST CSR -> create certificate
    ASC-->>UI: certificate + content
    UI->>FS: save progress/certs

    UI->>ASC: ensure bundle id / create profile
    ASC-->>UI: profile data
    UI->>FS: save profile/progress

    opt user requests build
        UI->>CLI: requestBuildInternal(with BuildLogger)
        CLI->>Build: upload & poll build
        Build-->>CLI: stream build logs/status
        CLI-->>UI: stream logs via BuildLogger
    end

    UI->>User: onboarding complete
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • ToriChanIntegration

Poem

🐰 I nibble bytes and sign with glee,

A tiny JWT tucked under my tree.
I hop through certs and profiles bright,
Ink panels glow through onboarding night.
Hooray — credentials spring to flight!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main feature: adding a new build onboarding command for automated iOS credential setup, which aligns with the substantial changes across multiple onboarding modules.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ios-onboarding
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

@socket-security
Copy link

socket-security bot commented Mar 19, 2026

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​types/​node-forge@​1.3.141001007682100
Added@​types/​react@​18.3.281001007992100
Addednode-forge@​1.3.3951001008570
Addedreact@​18.3.11001008497100
Addedjsonwebtoken@​9.0.39910010085100

View full report

@WcaleNieWolny WcaleNieWolny marked this pull request as ready for review March 20, 2026 09:18
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a5bc214d82

ℹ️ 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".

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/build/request.ts (1)

113-127: ⚠️ Potential issue | 🟡 Minor

Fix JSDoc parameter name to match refactored signature.

The JSDoc says @param silent but the parameter is now logger. This triggers the pipeline failure.

Proposed fix
 /**
  * Fetch with retry logic for build requests
  * Retries failed requests with exponential backoff, logging each failure
  *
  * `@param` url - The URL to fetch
  * `@param` options - Fetch options
  * `@param` maxRetries - Maximum number of retry attempts (default: 3)
- * `@param` silent - Suppress log output
+ * `@param` logger - Optional BuildLogger for output
  * `@returns` The fetch Response if successful
  * `@throws` Error if all retries are exhausted
  */
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/request.ts` around lines 113 - 127, The JSDoc for fetchWithRetry is
outdated: it documents a `@param` named "silent" but the function signature now
uses "logger"; update the JSDoc to replace or remove the "silent" param and
document the "logger?: BuildLogger" parameter instead, ensuring the `@param` name
and description match the function signature for fetchWithRetry so the docs and
types stay consistent.
🧹 Nitpick comments (2)
src/build/request.ts (2)

969-969: High cognitive complexity is pre-existing; consider future refactoring.

SonarCloud flags cognitive complexity of 132 (allowed: 15). This is primarily pre-existing complexity in requestBuildInternal. While not introduced by this PR, consider extracting logical sections (credential validation, zip/upload, log streaming) into helper functions in a future refactor.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/request.ts` at line 969, The function requestBuildInternal has very
high cognitive complexity; split its large body into smaller focused helper
functions to reduce complexity and improve maintainability: extract credential
validation logic into a function (e.g., validateBuildCredentials or
ensureCredentials), move archive/zip and upload steps into a
buildArchiveAndUpload or createAndUploadArchive helper, and isolate log
streaming/monitoring into a streamBuildLogs or followBuildLogs helper; update
requestBuildInternal to orchestrate these helpers (calling
validateBuildCredentials, createAndUploadArchive, streamBuildLogs, etc.) and
pass necessary parameters (appId, options, logger) so behavior is unchanged
while lowering cognitive complexity.

55-64: Consider newline style for consistency with linter.

ESLint flags antfu/if-newline violations. While the current style is readable, aligning with the project's linter configuration would resolve the pipeline warnings.

Optional fix for linter compliance
 function createDefaultLogger(silent: boolean): BuildLogger {
   return {
-    info: (msg: string) => { if (!silent) clackLog.info(msg) },
-    error: (msg: string) => { if (!silent) clackLog.error(msg) },
-    warn: (msg: string) => { if (!silent) clackLog.warn(msg) },
-    success: (msg: string) => { if (!silent) clackLog.success(msg) },
-    buildLog: (msg: string) => { if (!silent) console.log(msg) },
+    info: (msg: string) => {
+      if (!silent)
+        clackLog.info(msg)
+    },
+    error: (msg: string) => {
+      if (!silent)
+        clackLog.error(msg)
+    },
+    warn: (msg: string) => {
+      if (!silent)
+        clackLog.warn(msg)
+    },
+    success: (msg: string) => {
+      if (!silent)
+        clackLog.success(msg)
+    },
+    buildLog: (msg: string) => {
+      if (!silent)
+        console.log(msg)
+    },
     uploadProgress: (_percent: number) => {},
   }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/request.ts` around lines 55 - 64, The logger methods in
createDefaultLogger use inline single-line if statements that trigger the
antfu/if-newline linter; update each arrow function (info, error, warn, success,
buildLog) to use explicit block-style if statements with braces and a newline
(e.g., change "if (!silent) clackLog.info(msg)" to "if (!silent) {
clackLog.info(msg) }") so they comply with the project's linter while preserving
existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@package.json`:
- Around line 118-125: Add documentation for the new onboarding workflow to the
TanStack Intent skill docs under skills/native-builds/SKILL.md to match the new
onboardingCommand() implementation in src/build/onboarding/command.ts; describe
the command name, usage/examples, required flags/options, and note the runtime
dependencies used (ink, `@inkjs/ui`, react, jsonwebtoken, node-forge) so the
published skill reflects the CLI behavior. Ensure the text references
onboardingCommand() behavior (what it prompts/outputs and expected flow) and any
configuration or setup required to run the onboarding flow within the skill
documentation.

In `@src/build/onboarding/command.ts`:
- Around line 2-3: The import order in this file violates ESLint alphabetical
ordering: swap the two import statements so that the ink import (import { render
} from 'ink') appears before the React import (import React from 'react');
update the top of src/build/onboarding/command.ts to import from 'ink' first,
then 'react' to satisfy the linter.
- Around line 19-23: The code checks appId and currently calls console.error and
uses the global process; change this to import process explicitly (import
process from 'process') and use `@clack/prompts` for CLI output (import { outro }
from '@clack/prompts'), then replace the two console.error calls with a single
outro(...) call that shows the descriptive error message and finally call
process.exit(1); update the block that references appId in onboarding/command.ts
accordingly.

In `@src/build/onboarding/progress.ts`:
- Around line 28-33: The current catch block around readFile(filePath, 'utf-8')
and JSON.parse silently swallows all errors; change it so only a missing file is
treated as "no progress" and all other errors surface: when reading the file in
the function that calls readFile(filePath, 'utf-8') and JSON.parse(...) (the
code creating an OnboardingProgress), check the caught error's code (e.g.,
'ENOENT') and return null only for that case, otherwise rethrow or propagate the
error so JSON.parse failures or permission/IO errors are not ignored.

In `@src/build/onboarding/ui/app.tsx`:
- Around line 287-293: The code always calls generateCsr() before checking saved
progress, which overwrites any previously saved private key and prevents proper
resume; change the logic in the onboarding flow around
generateCsr()/loadProgress()/saveProgress() so you first call
loadProgress(appId) and, if existing._privateKeyPem is present, reuse that
private key (and any saved csrPem) instead of calling generateCsr(); only call
generateCsr() when no saved _privateKeyPem exists, then save the newly generated
privateKeyPem via saveProgress(appId, existing) and avoid overwriting an
existing _privateKeyPem when resuming the "creating-certificate" step.
- Around line 504-512: The current 'build-complete' branch (checking step ===
'build-complete') unconditionally clears buildOutput and shows the "building"
completion screen even for skipped/failed/no-key cases; update the logic in the
UI handler around step to distinguish real started/successful builds from
skipped/failed ones (e.g., introduce or check an explicit flag like buildStarted
or buildStatus/success, or treat specific steps such as
'build-failed'/'build-skipped' separately) and only call setBuildOutput([]) and
run the 4s exit timer when a build was actually started/succeeded; also ensure
requestBuildInternal error paths and the "build later" path set the step to a
distinct non-'build-complete' value so the UI does not render a running build
message.
- Around line 884-894: The skip branch currently skips calling
doSaveCredentials() so deleteProgress(appId) never runs; update the else branch
handling in the onChange handler to call deleteProgress(appId) (or the
appropriate progress-clear helper) before setting ask/step and logging, so
onboarding state is cleared when the user keeps existing credentials;
references: onChange handler, overwriteConfirmedRef, setAskOverwrite, addLog,
setStep('ask-build'), and deleteProgress(appId).

In `@src/index.ts`:
- Line 13: The new CLI export onboardingCommand added in src/index.ts must be
documented: open skills/usage/SKILL.md, locate the "Project setup and
diagnostics" section and add an entry for the onboarding command (name, short
description and usage example) consistent with existing command entries;
reference the onboardingCommand symbol and follow the same formatting and
conventions used for the other commands in that section so docs stay aligned
with the CLI.

---

Outside diff comments:
In `@src/build/request.ts`:
- Around line 113-127: The JSDoc for fetchWithRetry is outdated: it documents a
`@param` named "silent" but the function signature now uses "logger"; update the
JSDoc to replace or remove the "silent" param and document the "logger?:
BuildLogger" parameter instead, ensuring the `@param` name and description match
the function signature for fetchWithRetry so the docs and types stay consistent.

---

Nitpick comments:
In `@src/build/request.ts`:
- Line 969: The function requestBuildInternal has very high cognitive
complexity; split its large body into smaller focused helper functions to reduce
complexity and improve maintainability: extract credential validation logic into
a function (e.g., validateBuildCredentials or ensureCredentials), move
archive/zip and upload steps into a buildArchiveAndUpload or
createAndUploadArchive helper, and isolate log streaming/monitoring into a
streamBuildLogs or followBuildLogs helper; update requestBuildInternal to
orchestrate these helpers (calling validateBuildCredentials,
createAndUploadArchive, streamBuildLogs, etc.) and pass necessary parameters
(appId, options, logger) so behavior is unchanged while lowering cognitive
complexity.
- Around line 55-64: The logger methods in createDefaultLogger use inline
single-line if statements that trigger the antfu/if-newline linter; update each
arrow function (info, error, warn, success, buildLog) to use explicit
block-style if statements with braces and a newline (e.g., change "if (!silent)
clackLog.info(msg)" to "if (!silent) { clackLog.info(msg) }") so they comply
with the project's linter while preserving existing behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9ce58ef2-b42d-4529-83fd-af01edd63582

📥 Commits

Reviewing files that changed from the base of the PR and between 9217035 and a5bc214.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (13)
  • build.mjs
  • package.json
  • src/build/onboarding/apple-api.ts
  • src/build/onboarding/command.ts
  • src/build/onboarding/csr.ts
  • src/build/onboarding/file-picker.ts
  • src/build/onboarding/progress.ts
  • src/build/onboarding/types.ts
  • src/build/onboarding/ui/app.tsx
  • src/build/onboarding/ui/components.tsx
  • src/build/request.ts
  • src/index.ts
  • tsconfig.json

- Fix ESLint import order: ink before react in command.ts
- Use explicit process import and @clack/prompts log instead of
  console.error for consistent CLI output
- Only treat ENOENT as "no progress" in loadProgress, rethrow
  JSON corruption and permission errors
- Clear onboarding progress when user skips credential overwrite
  to prevent stale resume state
- Fix stale JSDoc: @param silent → @param logger in fetchWithRetry
When silent=true and no custom logger is provided, pass undefined
to streamBuildLogs so the early return kicks in. This restores
the pre-refactor behavior where silent SDK callers go directly
to REST polling without WebSocket overhead.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/build/request.ts (1)

969-1615: 🛠️ Refactor suggestion | 🟠 Major

requestBuildInternal needs helper boundaries now.

This routine now owns config loading, credential merging/validation, request creation, upload, signal handling, log streaming, polling, and analytics. Sonar is already flagging it, and the new BuildLogger paths will stay brittle until those phases are split into separately testable helpers.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/request.ts` around lines 969 - 1615, The function
requestBuildInternal is too large and mixes multiple responsibilities (config
loading, credential merging/validation, payload creation, upload, signal
handling, log streaming, polling, and analytics); refactor by extracting clear
helpers for at least: loadAndValidateConfig (handles getConfig, resolve
projectDir, getPlatformDirFromCapacitorConfig), collectAndMergeCredentials
(collect CLI/env, call mergeCredentials, normalize iOS distribution and
validation), createBuildRequest (calls fetchWithRetry to POST /build/request and
returns buildRequest), performUpload (wraps zipDirectory, reading file, and
tus.Upload logic), and orchestrateBuildLifecycle (start via /build/start, handle
SIGINT/cancelBuild, call streamBuildLogs and pollBuildStatus, compute
finalStatus and sendEvent). Keep existing helper names (mergeCredentials,
splitPayload, zipDirectory, fetchWithRetry, streamBuildLogs, pollBuildStatus,
sendEvent, createSupabaseClient, verifyUser) and move their usage into those new
functions so requestBuildInternal becomes a thin orchestrator that calls and
tests each helper independently.
♻️ Duplicate comments (2)
src/build/onboarding/ui/app.tsx (2)

448-452: ⚠️ Potential issue | 🟠 Major

build-complete still mixes successful, skipped, and failed outcomes.

The no-key, build-later, and failed-build branches all land in build-complete, but that state clears buildOutput, auto-exits, and always renders “Your iOS app is building in the cloud.” Split these into distinct terminal states, or track buildStarted/buildStatus, so skipped and failed runs keep their real message.

Also applies to: 488-499, 504-512, 917-919, 991-1013

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/onboarding/ui/app.tsx` around lines 448 - 452, Current logic
funnels no-key, skipped, and failed branches into the single 'build-complete'
step (via setStep('build-complete')), which clears buildOutput and shows the
success message; change this by introducing distinct terminal states (eg.
'build-skipped' and 'build-failed') or a build state object (eg.
buildStarted:boolean and buildStatus:'success'|'skipped'|'failed') and use those
instead of reusing 'build-complete'. Update every branch that currently calls
setStep('build-complete') (the capgoKey check, the build-later branch,
failed-build branches around setBuildOutput/setStep at lines shown) to set the
appropriate new state (or set buildStatus) and ensure the UI rendering logic
reads the new state(s) so skipped/failed branches do not clear buildOutput or
show the “Your iOS app is building in the cloud.” success text.

286-293: ⚠️ Potential issue | 🟠 Major

Resume still overwrites the saved signing key.

This generates a fresh CSR/key pair before reading persisted progress, so a resumed run discards _privateKeyPem and can consume another Apple distribution certificate if the first createCertificate() already succeeded on Apple’s side. Load progress first and reuse the saved key/CSR pair when resuming creating-certificate.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/onboarding/ui/app.tsx` around lines 286 - 293, The code generates a
new CSR/private key before loading persisted progress, causing resumed runs to
overwrite an existing _privateKeyPem and potentially reconsume an Apple
certificate; fix by calling loadProgress(appId) before generateCsr(), and if
existing._privateKeyPem (and existing.csrPem if stored) exist reuse them instead
of calling generateCsr(), otherwise generate a new pair and then save it to
progress via saveProgress(appId, ...); update the logic around getFreshToken(),
generateCsr(), loadProgress(), and saveProgress() so creating-certificate
resumes without replacing the saved _privateKeyPem.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/build/onboarding/command.ts`:
- Around line 10-35: Add a TTY guard at the start of onboardingCommand to fail
fast before mounting the Ink UI: check process.stdin.isTTY and
process.stdout.isTTY (or a single TTY check) and if false, log a plain error and
process.exit(1) instead of calling render; update onboardingCommand to perform
this check before invoking render(React.createElement(OnboardingApp...)) so
render and waitUntilExit are never started on non-interactive CI/pipes.

In `@src/build/onboarding/progress.ts`:
- Around line 16-17: The getProgressPath function currently concatenates appId
directly into a filename which allows path traversal; sanitize appId before
using it by validating and normalizing it (e.g., strip path separators and
sequences like "../", reject or replace disallowed characters) and/or enforce a
whitelist/regex (only allow alphanumerics, dashes, underscores) before composing
the path; update getProgressPath to perform this validation/normalization for
the appId parameter and only then call join(getOnboardingDir(baseDir),
`${sanitizedAppId}.json`) so getProgressPath, appId usage, and getOnboardingDir
remain safe.

In `@src/build/onboarding/ui/app.tsx`:
- Around line 206-210: Replace the hardcoded existsSync(join(process.cwd(),
'ios')) check inside the platform-select branch with the same platform directory
resolution used elsewhere in the build flow (i.e., use the project's
capacitor/config-derived platform path rather than process.cwd()/ios); locate
the platform resolution utility or config access used elsewhere in this codebase
and call that to get the iOS directory, then test existsSync on that resolved
path and call setStep('no-platform') only if that resolved directory does not
exist.

In `@src/build/request.ts`:
- Around line 55-63: createDefaultLogger's uploadProgress is currently a no-op,
causing upload progress to be hidden; implement uploadProgress in
createDefaultLogger to emit plain progress output (e.g., a simple "Uploading...
X%" via console.log or clackLog.info) when not silent or when stdout is non-TTY
so CLI shows progress for large archives, and mirror the same change in the
other logger instantiation used for builds (the alternate logger block
referenced in the diff) so callers using log.uploadProgress(...) (upload path)
produce visible fallback output.

---

Outside diff comments:
In `@src/build/request.ts`:
- Around line 969-1615: The function requestBuildInternal is too large and mixes
multiple responsibilities (config loading, credential merging/validation,
payload creation, upload, signal handling, log streaming, polling, and
analytics); refactor by extracting clear helpers for at least:
loadAndValidateConfig (handles getConfig, resolve projectDir,
getPlatformDirFromCapacitorConfig), collectAndMergeCredentials (collect CLI/env,
call mergeCredentials, normalize iOS distribution and validation),
createBuildRequest (calls fetchWithRetry to POST /build/request and returns
buildRequest), performUpload (wraps zipDirectory, reading file, and tus.Upload
logic), and orchestrateBuildLifecycle (start via /build/start, handle
SIGINT/cancelBuild, call streamBuildLogs and pollBuildStatus, compute
finalStatus and sendEvent). Keep existing helper names (mergeCredentials,
splitPayload, zipDirectory, fetchWithRetry, streamBuildLogs, pollBuildStatus,
sendEvent, createSupabaseClient, verifyUser) and move their usage into those new
functions so requestBuildInternal becomes a thin orchestrator that calls and
tests each helper independently.

---

Duplicate comments:
In `@src/build/onboarding/ui/app.tsx`:
- Around line 448-452: Current logic funnels no-key, skipped, and failed
branches into the single 'build-complete' step (via setStep('build-complete')),
which clears buildOutput and shows the success message; change this by
introducing distinct terminal states (eg. 'build-skipped' and 'build-failed') or
a build state object (eg. buildStarted:boolean and
buildStatus:'success'|'skipped'|'failed') and use those instead of reusing
'build-complete'. Update every branch that currently calls
setStep('build-complete') (the capgoKey check, the build-later branch,
failed-build branches around setBuildOutput/setStep at lines shown) to set the
appropriate new state (or set buildStatus) and ensure the UI rendering logic
reads the new state(s) so skipped/failed branches do not clear buildOutput or
show the “Your iOS app is building in the cloud.” success text.
- Around line 286-293: The code generates a new CSR/private key before loading
persisted progress, causing resumed runs to overwrite an existing _privateKeyPem
and potentially reconsume an Apple certificate; fix by calling
loadProgress(appId) before generateCsr(), and if existing._privateKeyPem (and
existing.csrPem if stored) exist reuse them instead of calling generateCsr(),
otherwise generate a new pair and then save it to progress via
saveProgress(appId, ...); update the logic around getFreshToken(),
generateCsr(), loadProgress(), and saveProgress() so creating-certificate
resumes without replacing the saved _privateKeyPem.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4f26fc3e-0d05-4dd0-a221-babf2ce65885

📥 Commits

Reviewing files that changed from the base of the PR and between a5bc214 and dcacc85.

📒 Files selected for processing (4)
  • src/build/onboarding/command.ts
  • src/build/onboarding/progress.ts
  • src/build/onboarding/ui/app.tsx
  • src/build/request.ts

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/build/request.ts (1)

969-1616: Split requestBuildInternal() into phase helpers.

The logger refactor pushed even more control flow into one method that now handles credential normalization, validation, payload construction, upload, log streaming, polling, SIGINT cancellation, cleanup, and analytics in a single block. Extracting those phases into internal helpers would make the CLI/onboarding path much easier to test and much less brittle to keep extending.

As per coding guidelines, "Prefer silent/internal helper variants for reusable business logic when the same operation is needed by CLI, SDK, onboarding, or MCP flows."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/request.ts` around lines 969 - 1616, The requestBuildInternal
function is doing too many things; extract its major phases into small internal
helpers to improve testability and reuse: (1) credential handling: move
CLI/env/file merge + normalization/validation into a helper (e.g.,
buildCredentialsForRequest(appId, platform, options) that calls mergeCredentials
and writes normalized fields like CAPGO_IOS_DISTRIBUTION and returns
mergedCredentials or a structured list of missingCreds), (2) payload
construction: move splitPayload + requestPayload creation into
buildRequestPayload(mergedCredentials, options, pack.version), (3) archive &
upload: move zipping and TUS upload into uploadProjectZip(projectDir, platform,
buildRequest.upload_url, options.apikey, logger) that uses zipDirectory and
tus.Upload and returns when complete, (4) job control & logs: move
start/poll/stream/cancel logic into runBuildJob(host, jobId, appId, platform,
apikey, startResult.logs_url, startResult.logs_token, logger) that encapsulates
SIGINT handling, abort controller, streamBuildLogs and pollBuildStatus and
returns finalStatus, and (5) analytics + cleanup: move sendEvent and tempDir
cleanup into finalizeBuildResult(result, orgId, buildTime) so
requestBuildInternal becomes a thin orchestrator calling these helpers (retain
existing helpers like fetchWithRetry, mergeCredentials, splitPayload,
zipDirectory, streamBuildLogs, pollBuildStatus, sendEvent and reference them in
tests). Ensure each helper has a silent/internal variant signature to accept a
logger and/or silent flag for reuse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/build/request.ts`:
- Around line 55-63: The createDefaultLogger function currently uses single-line
inline if bodies (tripping antfu/if-newline) and calls console.log in buildLog
(tripping no-console); update createDefaultLogger (returning BuildLogger) to use
block-style if statements for info, error, warn, and success handlers (e.g., if
(!silent) { clackLog.info(msg); }) and replace buildLog's console.log with a
logger method (e.g., clackLog.info or clackLog.log) wrapped in a block if
(!silent) { ... } so no inline single-line ifs remain and no direct console
usage is present; keep uploadProgress as-is.
- Around line 1239-1242: The hardcoded recovery messages use the wrong platform
and flag/env names; update the log.error lines in src/build/request.ts to use
the runtime options.platform value (instead of always "ios"), show the correct
iOS flag names (--apple-key-id, --apple-issuer-id, --apple-key-content) and
corresponding env vars (APPLE_KEY_ID, APPLE_ISSUER_ID, APPLE_KEY_CONTENT) for
the CLI/environment examples, and ensure the "Saved credentials" hint uses
options.platform so it reflects Android when appropriate; locate the logging
block that calls log.error and references options.platform to make these
replacements.

---

Nitpick comments:
In `@src/build/request.ts`:
- Around line 969-1616: The requestBuildInternal function is doing too many
things; extract its major phases into small internal helpers to improve
testability and reuse: (1) credential handling: move CLI/env/file merge +
normalization/validation into a helper (e.g., buildCredentialsForRequest(appId,
platform, options) that calls mergeCredentials and writes normalized fields like
CAPGO_IOS_DISTRIBUTION and returns mergedCredentials or a structured list of
missingCreds), (2) payload construction: move splitPayload + requestPayload
creation into buildRequestPayload(mergedCredentials, options, pack.version), (3)
archive & upload: move zipping and TUS upload into uploadProjectZip(projectDir,
platform, buildRequest.upload_url, options.apikey, logger) that uses
zipDirectory and tus.Upload and returns when complete, (4) job control & logs:
move start/poll/stream/cancel logic into runBuildJob(host, jobId, appId,
platform, apikey, startResult.logs_url, startResult.logs_token, logger) that
encapsulates SIGINT handling, abort controller, streamBuildLogs and
pollBuildStatus and returns finalStatus, and (5) analytics + cleanup: move
sendEvent and tempDir cleanup into finalizeBuildResult(result, orgId, buildTime)
so requestBuildInternal becomes a thin orchestrator calling these helpers
(retain existing helpers like fetchWithRetry, mergeCredentials, splitPayload,
zipDirectory, streamBuildLogs, pollBuildStatus, sendEvent and reference them in
tests). Ensure each helper has a silent/internal variant signature to accept a
logger and/or silent flag for reuse.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3ad86d69-6c3a-44bc-9ba5-2a5f394ea97c

📥 Commits

Reviewing files that changed from the base of the PR and between dcacc85 and 57069f9.

📒 Files selected for processing (1)
  • src/build/request.ts

Documents the new `build onboarding` command, its architecture,
conflict resolution behavior, and the BuildLogger callback interface.
…write check

- Save progress incrementally at each input step (p8Path, keyId, issuerId)
  so interrupted onboarding resumes at the right step
- Show completed partial steps as checkmarks when resuming
- Check for existing credentials at start (after platform select) instead
  of at the end — avoids creating orphaned certs/profiles
- Backup existing credentials to credentials-DATE.copy.json before proceeding
- Remove old overwrite prompt at save step
- Fix: don't save keyId from filename extraction until user confirms
- Fix: move async backup to useEffect to avoid Select onChange re-fire
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (5)
src/build/onboarding/ui/app.tsx (3)

328-338: ⚠️ Potential issue | 🟠 Major

_privateKeyPem is overwritten before checking saved progress — breaks resume.

generateCsr() is called unconditionally (line 332) before checking if existing._privateKeyPem is already saved (line 334-336). On resume after a crash, this overwrites the saved private key, potentially consuming another certificate slot if Apple already created the cert.

🛠️ Suggested fix — check for saved key before generating
     if (step === 'creating-certificate') {
       ;(async () => {
         try {
           const token = await getFreshToken()
+          // Check for saved private key from interrupted attempt
+          const existing = await loadProgress(appId)
+          let csrPem: string
+          let privateKeyPem: string
+          if (existing?._privateKeyPem) {
+            // Reuse saved key — regenerate CSR from it
+            privateKeyPem = existing._privateKeyPem
+            // You may need a helper to regenerate CSR from existing key
+            const result = generateCsr(existing._privateKeyPem)
+            csrPem = result.csrPem
+          } else {
+            const result = generateCsr()
+            csrPem = result.csrPem
+            privateKeyPem = result.privateKeyPem
+            // Save private key to progress in case of crash
+            if (existing) {
+              existing._privateKeyPem = privateKeyPem
+              await saveProgress(appId, existing)
+            }
+          }
-          const { csrPem, privateKeyPem } = generateCsr()
-          // Save private key to progress in case of crash
-          const existing = await loadProgress(appId)
-          if (existing) {
-            existing._privateKeyPem = privateKeyPem
-            await saveProgress(appId, existing)
-          }
           const cert = await createCertificate(token, csrPem)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/onboarding/ui/app.tsx` around lines 328 - 338, The code calls
generateCsr() unconditionally which overwrites any previously saved key and
breaks resume; modify the creating-certificate branch so you first call
loadProgress(appId) and check existing._privateKeyPem, and only call
generateCsr() when no saved private key is present; if an existing private key
is found, reuse it (and avoid regenerating csrPem/privateKeyPem), otherwise
generate and then persist the privateKeyPem via saveProgress(appId, existing) as
is currently attempted; adjust variable flow so csrPem/privateKeyPem are
assigned conditionally based on that check.

1044-1070: ⚠️ Potential issue | 🟠 Major

Build-complete screen shows "building in the cloud" even when no build was started.

The UI reaches build-complete in multiple scenarios:

  • User chose "build later" (line 971)
  • No Capgo API key found (line 487)
  • Build request failed (line 534)

Yet line 1059 always displays "Your iOS app is building in the cloud." This is misleading when no build was started.

🛠️ Suggested approach

Track whether a build was actually started:

+  const [buildStarted, setBuildStarted] = useState(false)

// In 'requesting-build' success path (around line 519):
+  setBuildStarted(true)

// In 'build-complete' UI (around line 1059):
-  <Text>Your iOS app is building in the cloud.</Text>
+  {buildStarted
+    ? <Text>Your iOS app is building in the cloud.</Text>
+    : <Text>Your credentials are ready for future builds.</Text>
+  }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/onboarding/ui/app.tsx` around lines 1044 - 1070, The
"build-complete" UI always says "Your iOS app is building in the cloud." even
when no build was started; add and use a boolean flag (e.g., buildStarted) in
the component state that is set to true only when a build request actually
succeeds (where build is initiated/requested), then update the JSX in the step
=== 'build-complete' branch to conditionally render the "Your iOS app is
building in the cloud." / build tracking UI (and show buildUrl) only when
buildStarted is true, and render a different message (e.g., "No build was
started" or instructions to run npx ...) when buildStarted is false; reference
the existing variables/JSX nodes buildUrl and the step === 'build-complete'
block to make the change.

250-255: ⚠️ Potential issue | 🟠 Major

Don't hardcode process.cwd()/ios — use Capacitor config resolution.

Capacitor projects can override the iOS directory in capacitor.config.ts. This hardcoded check will reject valid projects with custom platform paths even though the rest of the build flow supports them.

#!/bin/bash
# Search for how other parts of the codebase resolve platform directories
rg -n --type=ts 'ios.*directory|webDir|iosDir|platformDir' -g '!node_modules'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/onboarding/ui/app.tsx` around lines 250 - 255, The check in the
platform selection branch currently hardcodes process.cwd()/ios and should
instead resolve the iOS platform directory via the Capacitor config resolution
used elsewhere; replace the existsSync(join(process.cwd(), 'ios')) check with a
resolution that reads the project's Capacitor config (e.g., use the shared
utility/function your repo uses to get iosDir or webDir) and test existence of
that resolved path, then call setStep('no-platform') only if the resolved iosDir
is missing or empty; update code references around step and setStep in app.tsx
to use the resolved iosDir value rather than the hardcoded 'ios'.
src/build/onboarding/progress.ts (2)

16-18: ⚠️ Potential issue | 🔴 Critical

Sanitize appId before using it as a filename.

appId comes from project config and is used directly in a file path. A malformed or malicious app ID containing ../ could enable arbitrary file read/write/delete outside the onboarding directory.

🔒 Suggested hardening
+function sanitizeAppId(appId: string): string {
+  // Only allow alphanumeric, dots, dashes, underscores
+  // Alternatively: return encodeURIComponent(appId)
+  if (!/^[\w.-]+$/.test(appId)) {
+    throw new Error(`Invalid appId for progress file: ${appId}`)
+  }
+  return appId
+}
+
 function getProgressPath(appId: string, baseDir?: string): string {
-  return join(getOnboardingDir(baseDir), `${appId}.json`)
+  return join(getOnboardingDir(baseDir), `${sanitizeAppId(appId)}.json`)
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/onboarding/progress.ts` around lines 16 - 18, getProgressPath
currently concatenates appId into a filename and can be exploited with path
traversal; sanitize and validate appId before using it in getProgressPath:
restrict to a safe pattern (e.g., allow only [A-Za-z0-9_-] and reject or throw
on anything else), or derive the filename with a safe transformation (e.g., use
path.basename or hex/URL-encode a validated id) to remove any directory
separators, then build the path using join(getOnboardingDir(baseDir),
`${sanitizedAppId}.json`) and (optionally) assert the resulting path starts with
getOnboardingDir(baseDir) to guarantee no escape. Ensure changes are applied in
getProgressPath and any callers expecting raw appId.

63-68: ⚠️ Potential issue | 🟡 Minor

Handle only ENOENT in deleteProgress, consistent with loadProgress.

The bare catch {} swallows all errors including permission failures and IO issues. This is inconsistent with loadProgress which properly checks for ENOENT and rethrows other errors.

♻️ Suggested fix for consistency
   try {
     await unlink(filePath)
   }
-  catch {
-    // File doesn't exist, that's fine
+  catch (error: unknown) {
+    // File doesn't exist is fine; rethrow other errors
+    if (error instanceof Error && 'code' in error && (error as NodeJS.ErrnoException).code === 'ENOENT') {
+      return
+    }
+    throw error
   }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/onboarding/progress.ts` around lines 63 - 68, The deleteProgress
function currently swallows all errors when calling unlink(filePath); change its
catch to mirror loadProgress by capturing the thrown error object and only
ignore it if error.code === 'ENOENT', otherwise rethrow the error so
permission/IO errors bubble up; update the catch block in deleteProgress to test
the error.code and rethrow for non-ENOENT cases (referencing deleteProgress and
its unlink(filePath) call).
🧹 Nitpick comments (3)
src/build/onboarding/ui/app.tsx (3)

106-109: Move regex patterns to module scope to avoid recompilation.

The regex on line 107 (and similar ones at lines 235, 712, 752) is recreated on every function call. Moving to module scope is a minor optimization.

♻️ Example
+const P8_FILENAME_REGEX = /(?:Auth|Api)Key_([A-Z0-9]+)\.p8$/i

 // Inside component:
 function extractKeyIdFromPath(filePath: string): string {
-  const match = filePath.match(/(?:Auth|Api)Key_([A-Z0-9]+)\.p8$/i)
+  const match = filePath.match(P8_FILENAME_REGEX)
   return match?.[1] || ''
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/onboarding/ui/app.tsx` around lines 106 - 109, The regex is being
recompiled on every call inside functions like extractKeyIdFromPath; move the
pattern(s) to module scope as named constants (e.g. AUTH_API_KEY_REGEX) and
replace the inline /(?:Auth|Api)Key_([A-Z0-9]+)\.p8$/i usage in
extractKeyIdFromPath and the similar occurrences at the other locations with
those constants to reuse the compiled RegExp and avoid repeated recompilation.

118-123: Move NeedP8Error class to module scope.

Defining the class inside the component recreates the class prototype on every render. Move it outside the component for efficiency.

♻️ Suggested refactor

Move before OnboardingApp:

+/**
+ * Special error to signal the UI should redirect to .p8 input.
+ */
+class NeedP8Error extends Error {
+  constructor() {
+    super('Need .p8 file')
+    this.name = 'NeedP8Error'
+  }
+}
+
 const OnboardingApp: FC<AppProps> = ({ appId, initialProgress }) => {

Then remove lines 115-123 from inside the component.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/onboarding/ui/app.tsx` around lines 118 - 123, The NeedP8Error
class is currently declared inside the OnboardingApp component causing a new
prototype to be created on every render; move the NeedP8Error class declaration
to module scope (e.g., place it above the OnboardingApp function/component) and
remove the inline declaration inside OnboardingApp so all references in
OnboardingApp still use the module-scoped NeedP8Error.

647-652: Prefer exit() from useApp() over process.exit() for consistent shutdown.

Multiple places use process.exit() directly (lines 649, 888, 933, 1034) while other paths use exit() from useApp(). The Ink exit() hook allows for graceful cleanup.

♻️ Example replacement
             onChange={(value) => {
               if (value === 'backup') {
                 setStep('backing-up')
               }
               else {
-                process.stderr.write('\nExiting onboarding.\n')
-                process.exit(0)
+                addLog('Exiting onboarding.', 'yellow')
+                setTimeout(() => exit(), 100)
               }
             }}

Apply similar changes to other process.exit() calls.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/build/onboarding/ui/app.tsx` around lines 647 - 652, Replace direct calls
to process.exit(...) with the Ink-provided exit() from useApp() to allow
graceful shutdown: import and call const { exit } = useApp() (or reuse existing
useApp() usage in the component) and replace process.stderr.write('\nExiting
onboarding.\n'); process.exit(0) with a call that writes the same message then
calls exit(); also update the other occurrences that currently call
process.exit() (they should call exit() instead) so all shutdown paths use the
same graceful exit hook.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/build/onboarding/progress.ts`:
- Line 2: Remove the unused imported symbol "readdir" from the import statement
that currently reads "import { readFile, readdir, unlink } from
'node:fs/promises'"; update the import to only include the used symbols (e.g.,
"readFile" and "unlink") so the module no longer imports the unused "readdir".

---

Duplicate comments:
In `@src/build/onboarding/progress.ts`:
- Around line 16-18: getProgressPath currently concatenates appId into a
filename and can be exploited with path traversal; sanitize and validate appId
before using it in getProgressPath: restrict to a safe pattern (e.g., allow only
[A-Za-z0-9_-] and reject or throw on anything else), or derive the filename with
a safe transformation (e.g., use path.basename or hex/URL-encode a validated id)
to remove any directory separators, then build the path using
join(getOnboardingDir(baseDir), `${sanitizedAppId}.json`) and (optionally)
assert the resulting path starts with getOnboardingDir(baseDir) to guarantee no
escape. Ensure changes are applied in getProgressPath and any callers expecting
raw appId.
- Around line 63-68: The deleteProgress function currently swallows all errors
when calling unlink(filePath); change its catch to mirror loadProgress by
capturing the thrown error object and only ignore it if error.code === 'ENOENT',
otherwise rethrow the error so permission/IO errors bubble up; update the catch
block in deleteProgress to test the error.code and rethrow for non-ENOENT cases
(referencing deleteProgress and its unlink(filePath) call).

In `@src/build/onboarding/ui/app.tsx`:
- Around line 328-338: The code calls generateCsr() unconditionally which
overwrites any previously saved key and breaks resume; modify the
creating-certificate branch so you first call loadProgress(appId) and check
existing._privateKeyPem, and only call generateCsr() when no saved private key
is present; if an existing private key is found, reuse it (and avoid
regenerating csrPem/privateKeyPem), otherwise generate and then persist the
privateKeyPem via saveProgress(appId, existing) as is currently attempted;
adjust variable flow so csrPem/privateKeyPem are assigned conditionally based on
that check.
- Around line 1044-1070: The "build-complete" UI always says "Your iOS app is
building in the cloud." even when no build was started; add and use a boolean
flag (e.g., buildStarted) in the component state that is set to true only when a
build request actually succeeds (where build is initiated/requested), then
update the JSX in the step === 'build-complete' branch to conditionally render
the "Your iOS app is building in the cloud." / build tracking UI (and show
buildUrl) only when buildStarted is true, and render a different message (e.g.,
"No build was started" or instructions to run npx ...) when buildStarted is
false; reference the existing variables/JSX nodes buildUrl and the step ===
'build-complete' block to make the change.
- Around line 250-255: The check in the platform selection branch currently
hardcodes process.cwd()/ios and should instead resolve the iOS platform
directory via the Capacitor config resolution used elsewhere; replace the
existsSync(join(process.cwd(), 'ios')) check with a resolution that reads the
project's Capacitor config (e.g., use the shared utility/function your repo uses
to get iosDir or webDir) and test existence of that resolved path, then call
setStep('no-platform') only if the resolved iosDir is missing or empty; update
code references around step and setStep in app.tsx to use the resolved iosDir
value rather than the hardcoded 'ios'.

---

Nitpick comments:
In `@src/build/onboarding/ui/app.tsx`:
- Around line 106-109: The regex is being recompiled on every call inside
functions like extractKeyIdFromPath; move the pattern(s) to module scope as
named constants (e.g. AUTH_API_KEY_REGEX) and replace the inline
/(?:Auth|Api)Key_([A-Z0-9]+)\.p8$/i usage in extractKeyIdFromPath and the
similar occurrences at the other locations with those constants to reuse the
compiled RegExp and avoid repeated recompilation.
- Around line 118-123: The NeedP8Error class is currently declared inside the
OnboardingApp component causing a new prototype to be created on every render;
move the NeedP8Error class declaration to module scope (e.g., place it above the
OnboardingApp function/component) and remove the inline declaration inside
OnboardingApp so all references in OnboardingApp still use the module-scoped
NeedP8Error.
- Around line 647-652: Replace direct calls to process.exit(...) with the
Ink-provided exit() from useApp() to allow graceful shutdown: import and call
const { exit } = useApp() (or reuse existing useApp() usage in the component)
and replace process.stderr.write('\nExiting onboarding.\n'); process.exit(0)
with a call that writes the same message then calls exit(); also update the
other occurrences that currently call process.exit() (they should call exit()
instead) so all shutdown paths use the same graceful exit hook.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 23cf44a5-a65d-438c-8b9c-4de30e680633

📥 Commits

Reviewing files that changed from the base of the PR and between 861bab3 and 41c1a60.

📒 Files selected for processing (3)
  • src/build/onboarding/progress.ts
  • src/build/onboarding/types.ts
  • src/build/onboarding/ui/app.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/build/onboarding/types.ts

Instead of hardcoding `ios/`, use `getPlatformDirFromCapacitorConfig`
to respect custom `ios.path` settings in capacitor.config.ts.
Branch on buildUrl: if set, show "building in the cloud" + track link.
If not (skipped, failed, no API key), show "credentials saved and ready".
- Fix import ordering (perfectionist/sort-imports, sort-named-imports)
- Split multi-statement lines (style/max-statements-per-line)
- Add explicit node:process and node:buffer imports
- Use top-level type-only imports (import/consistent-type-specifier-style)
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an interactive build onboarding flow to automate iOS credential setup (certificates + provisioning profiles via App Store Connect), and refactors the native build request path to support pluggable UI logging via a BuildLogger interface (enabling Ink-based UI integration).

Changes:

  • Introduces npx @capgo/cli build onboarding (Ink UI) with resumable progress, Apple API integration, CSR/P12 generation, and optional first build.
  • Refactors requestBuildInternal to accept an optional BuildLogger and routes build log/progress output through callbacks.
  • Updates TypeScript/Bundling/docs/deps to support TSX + Ink/React + onboarding documentation.

Reviewed changes

Copilot reviewed 13 out of 15 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
tsconfig.json Enables react-jsx to compile the new Ink TSX UI.
src/index.ts Registers the new build onboarding subcommand.
src/build/request.ts Adds BuildLogger, uses it for build log/progress output, and adjusts retry/log streaming paths accordingly.
src/build/onboarding/ui/components.tsx Adds reusable Ink UI components, including a custom filtered text input.
src/build/onboarding/ui/app.tsx Implements the main onboarding state machine UI, progress persistence/resume, Apple setup, and optional build kickoff.
src/build/onboarding/types.ts Defines onboarding step types, progress model, and progress percentage mapping.
src/build/onboarding/progress.ts Implements secure per-app progress persistence under ~/.capgo-credentials/onboarding/.
src/build/onboarding/file-picker.ts Adds macOS osascript-based .p8 file picker integration.
src/build/onboarding/csr.ts Adds CSR generation, P12 creation, and team-id extraction from certs.
src/build/onboarding/command.ts Commander entrypoint that detects appId/iosDir and launches the Ink app.
src/build/onboarding/apple-api.ts Adds App Store Connect API client helpers (JWT auth, cert/profile/bundle operations, conflict helpers).
skills/native-builds/SKILL.md Documents the new onboarding flow and the BuildLogger callback interface.
package.json Adds Ink/React/JWT/forge dependencies and relevant type packages.
bun.lock Locks newly added dependencies.
build.mjs Stubs react-devtools-core to keep the Bun bundle build working with Ink.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Replace process.exit() with Ink's exit() inside the Ink app to ensure
  proper terminal teardown
- Fix verifyApiKey: include HTTP status in ascFetch error messages so
  401/403 matching actually works
- Fix P12 password JSDoc to match actual DEFAULT_P12_PASSWORD behavior
- Update skill docs: existing credentials flow is backup+proceed or exit
- Implement uploadProgress in default BuildLogger (10% interval logging)
  to restore upload feedback for non-onboarding build requests
- Remove duplicate ❯ prompt wrappers around FilteredTextInput
- Fix FilteredTextInput paste handling: strip forbidden chars from
  accumulated string, not just individual keystrokes
Replace the log.info() upload progress (printed separate lines at 10%
intervals) with the original @clack/prompts spinner that shows a live
animated percentage update on a single line.
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
3.6% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@WcaleNieWolny WcaleNieWolny merged commit d8c7a1e into main Mar 20, 2026
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants