feat(cli): import existing service account JSON in Android build init#2317
Conversation
Spec for adding an "I have my own service account JSON" path to the Android onboarding TUI, alongside the existing OAuth-driven auto-provision flow. Mirrors the existing keystore existing/generate fork. No backend or schema changes — pure CLI/TUI work.
Adds an "I have my own service account JSON" path to the Android onboarding TUI, alongside the existing OAuth auto-provision flow. The new fork lives between the keystore phase and the OAuth sign-in, mirroring the existing keystore existing/generate fork pattern. The import path: - Confirms the Android package name (reuses android-package-select) - Picks the SA JSON file via the macOS native file picker, with a text-input fallback on other platforms (same UX as the keystore picker) - Validates by parsing the JSON shape, exchanging a signed JWT for an OAuth access token, and round-tripping edits.insert(packageName) / edits.delete against the Google Play Developer API — the same auth path fastlane's supply takes at build time - On validation failure, offers three recovery options: try a different file, save anyway (skip validation), or fall back to the OAuth path No developer ID is collected — fastlane's supply doesn't need one for uploads (packageName resolves the developer account on Google's side), and the import path assumes the user has already invited their SA via Play Console. Resume routing preserves backward compatibility: legacy progress files without serviceAccountMethod default to the OAuth path so in-flight onboardings continue along the path they started on.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an Android onboarding Phase 2 fork to import an existing Google Play service-account JSON: types/state, macOS picker, JSON validation (JWT→token, edits.insert/delete probe), UI integration with recovery options, and resume-routing changes. ChangesAndroid Service Account Import Flow
Sequence DiagramsequenceDiagram
participant Caller
participant validateServiceAccountJson
participant parseServiceAccountKey
participant signSaAssertion
participant exchangeJwtForAccessToken
participant probeAppAccess
participant GoogleTokenEndpoint
participant PlayAPI
Caller->>validateServiceAccountJson: jsonBytes, packageName, signal
validateServiceAccountJson->>parseServiceAccountKey: JSON buffer
parseServiceAccountKey-->>validateServiceAccountJson: ServiceAccountKey
validateServiceAccountJson->>signSaAssertion: key + scopes
signSaAssertion-->>validateServiceAccountJson: RS256 JWT assertion
validateServiceAccountJson->>exchangeJwtForAccessToken: JWT assertion
exchangeJwtForAccessToken->>GoogleTokenEndpoint: POST jwt-bearer grant
GoogleTokenEndpoint-->>exchangeJwtForAccessToken: access_token or error
exchangeJwtForAccessToken-->>validateServiceAccountJson: token string
validateServiceAccountJson->>probeAppAccess: token, packageName
probeAppAccess->>PlayAPI: POST edits.insert for package
PlayAPI-->>probeAppAccess: editId or 401/403/404
probeAppAccess->>PlayAPI: DELETE edits/{editId}
PlayAPI-->>probeAppAccess: deleted or error (swallowed)
probeAppAccess-->>validateServiceAccountJson: access verified or no-app-access
validateServiceAccountJson-->>Caller: ValidationResult variant
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…7e00f # Conflicts: # cli/src/build/onboarding/android/ui/app.tsx
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/progress.ts`:
- Around line 168-175: Current logic sends users with
progress.serviceAccountMethod === undefined to 'service-account-method-select'
after completedSteps.keystoreReady, but the compatibility contract requires that
an absent serviceAccountMethod default to the generate/OAuth flow; change the
routing so that when progress.serviceAccountMethod is undefined and
completedSteps.keystoreReady is true, you treat it as the OAuth/generate default
(e.g., route to the OAuth flow like 'service-account-oauth' or continue the same
path as an explicit 'oauth' method) instead of returning
'service-account-method-select'; update the branch that checks
completedSteps.googleSignInComplete and the return that currently yields
'service-account-method-select' accordingly to use the OAuth default behavior
while keeping explicit serviceAccountMethod values unchanged.
In `@cli/src/build/onboarding/android/service-account-validation.ts`:
- Around line 367-384: The catch around exchangeJwtForAccessToken(...) currently
maps all failures to kind:'token-error'; change it to distinguish
network/abort/timeouts by inspecting the thrown error (e.g., check err.name ===
'AbortError' or err.message includes 'timed out' or known fetch/network error
indicators) and return a different kind such as 'network-error' (or
'timeout'/'cancelled' per your taxonomy) with the original message; otherwise
fall back to the existing token-error branch—update the catch that wraps
signSaAssertion, exchangeJwtForAccessToken, and uses
opts.signal/timeoutMs/fetchImpl to implement this branching.
In `@cli/src/build/onboarding/android/ui/app.tsx`:
- Line 550: The changed handlers violate the style/arrow-parens rule; update
each single-parameter arrow function used in persist callbacks to include
parentheses around the parameter (e.g., change p => ({...}) to (p) => ({...})).
Search for usages of persist in this file (and the impacted handlers referenced)
and normalize each callback (for example the callback passed to persist in the
handler using serviceAccountJsonPath) to use (p) => ... so all single-arg arrow
functions comply.
- Around line 574-577: The validation call to validateServiceAccountJson is
missing cancellation support: update the sa-json-validating branch to create/use
an AbortController (or the effect's signal) and pass its signal into
validateServiceAccountJson along with jsonBytes and
androidPackageChoice.packageName, and ensure you call controller.abort() (or
rely on effect cleanup) in the effect cleanup so the network validation is
aborted when the step exits or the component unmounts.
In `@docs/superpowers/specs/2026-05-21-android-import-service-account-design.md`:
- Around line 76-92: Two fenced code blocks in the spec diagram lack language
identifiers (causing markdownlint MD040); update each triple-backtick fence that
wraps the ASCII flow diagram (the block starting with "keystore phase
(unchanged: existing / generate)" and the other similar fenced block later near
the service-account import flow) to include a language tag such as text
(```text) or bash (```bash) so both fenced blocks are annotated and MD040 is
satisfied; leave the diagram content unchanged, only add the language tag to the
opening fence.
🪄 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: fc36d759-d28b-4d28-89ea-6036cc7e731c
📒 Files selected for processing (6)
cli/src/build/onboarding/android/progress.tscli/src/build/onboarding/android/service-account-validation.tscli/src/build/onboarding/android/types.tscli/src/build/onboarding/android/ui/app.tsxcli/src/build/onboarding/file-picker.tsdocs/superpowers/specs/2026-05-21-android-import-service-account-design.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baa827e884
ℹ️ 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".
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ui/app.tsx`:
- Around line 29-35: Import ordering in this module is violating ESLint rules:
move the type-only import CiSecretEntry, CiSecretSetupAdvice, CiSecretTarget so
it appears before any value imports (e.g., request-related imports), and reorder
the grouped value imports so UI component imports (Divider, ErrorLine,
FilteredTextInput, Header, SpinnerLine, SuccessLine) are correctly sorted
relative to adjacent imports; update the import block so type imports
(CiSecretEntry, CiSecretSetupAdvice, CiSecretTarget) come first, then related
ci-secrets functions (createCiSecretEntries, detectCiSecretTargets,
getCiSecretTargetLabel, listExistingCiSecretKeys, uploadCiSecrets), followed by
mapAndroidOnboardingError, file-picker functions (canUseFilePicker,
openKeystorePicker, openServiceAccountJsonPicker), telemetry
(trackBuilderOnboardingStep), validation (validateServiceAccountJson), and
finally the UI components (Divider, ErrorLine, FilteredTextInput, Header,
SpinnerLine, SuccessLine) to satisfy ESLint ordering.
🪄 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: 7e004379-89ae-4099-aae0-2a0137ed72f9
📒 Files selected for processing (2)
cli/src/build/onboarding/android/types.tscli/src/build/onboarding/android/ui/app.tsx
- progress.ts: legacy progress without `serviceAccountMethod` defaults to
the OAuth path (`google-sign-in`) rather than the new fork screen, so
in-flight onboardings that predate this PR aren't interrupted. Fresh
post-keystore transitions in this run still explicitly route to the
fork via the keystore-generating / keystore-existing-key-password
effects, which detect "no OAuth-side progress" and choose the fork.
- service-account-validation.ts: split error classification so 5xx /
non-JSON / aborted / fetch-level failures surface as `network-error`
instead of `token-error`. Only genuine 4xx credential rejections keep
the token-error label, so the recovery UI offers retry vs. "your key
is bad" appropriately.
- ui/app.tsx: wire an AbortController into the sa-json-validating effect
and abort it on cleanup. Validation no longer leaks after step exit
or Ctrl+C.
- ui/app.tsx: reorder imports to satisfy perfectionist/sort-imports
(type-parent before value-parent, ui/components.js before service-
account-validation.js).
- file-picker.ts: filter the SA JSON picker by `public.json` UTI rather
than the raw `"json"` extension hint, which macOS treats as a
four-char OSType code and greys out real JSON files.
- ui/app.tsx: per-step `<Select>` re-fire guard (`selectFiredRef`) to
work around the @inkjs/ui v2 bug that re-invokes `onChange` on every
parent re-render. Stops the duplicate package-pick log spam.
- ui/app.tsx: swap the order of the service-account fork options so
OAuth ("set one up for me") shows first.
- design doc: add `text` language tag to the two ASCII flow-diagram
fenced code blocks to satisfy markdownlint MD040.
There was a problem hiding this comment.
Actionable comments posted: 1
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/service-account-validation.ts (1)
221-253: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueEvent listeners not removed on successful fetch completion.
The
abortevent listeners added toargs.signalandtimeoutController.signal(lines 237-238) are never removed when the fetch completes successfully. While{ once: true }ensures each listener fires at most once, they remain in memory until the signals are garbage collected. For a short-lived validation call this is negligible, but ifargs.signalis a long-lived controller reused across many requests, listeners accumulate.Given the limited call sites (onboarding validation only) and short signal lifetimes, this is acceptable as-is but worth noting for future reuse.
🤖 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/service-account-validation.ts` around lines 221 - 253, The abort event listeners added inside fetchWithTimeout (the abortComposite handler attached to args.signal and timeoutController.signal) should be explicitly removed when the request finishes to avoid listener buildup; modify fetchWithTimeout to capture the abortComposite function references and, in the finally block (where timer is cleared), call removeEventListener on both args.signal and timeoutController.signal to detach abortComposite (guarding if args.signal exists), then clear the timeout as before; reference the composite/abortComposite variables, timeoutController, args.signal and timeoutController.signal to locate where to add the removals.
🤖 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/ui/app.tsx`:
- Line 29: The import line mixes value and type imports and has unsorted named
imports; change it to use top-level type-only imports for CiSecretEntry,
CiSecretSetupAdvice, and CiSecretTarget (e.g., import type { CiSecretEntry,
CiSecretSetupAdvice, CiSecretTarget } from '...') and ensure the remaining value
imports are alphabetically sorted (createCiSecretEntries, detectCiSecretTargets,
getCiSecretTargetLabel, listExistingCiSecretKeys, uploadCiSecrets) so
CiSecretEntry appears before listExistingCiSecretKeys in the combined ordering.
---
Outside diff comments:
In `@cli/src/build/onboarding/android/service-account-validation.ts`:
- Around line 221-253: The abort event listeners added inside fetchWithTimeout
(the abortComposite handler attached to args.signal and
timeoutController.signal) should be explicitly removed when the request finishes
to avoid listener buildup; modify fetchWithTimeout to capture the abortComposite
function references and, in the finally block (where timer is cleared), call
removeEventListener on both args.signal and timeoutController.signal to detach
abortComposite (guarding if args.signal exists), then clear the timeout as
before; reference the composite/abortComposite variables, timeoutController,
args.signal and timeoutController.signal to locate where to add the removals.
🪄 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: cd77b1ee-e22f-4564-a035-c9220dabc4e9
📒 Files selected for processing (5)
cli/src/build/onboarding/android/progress.tscli/src/build/onboarding/android/service-account-validation.tscli/src/build/onboarding/android/ui/app.tsxcli/src/build/onboarding/file-picker.tsdocs/superpowers/specs/2026-05-21-android-import-service-account-design.md
- ui/app.tsx: revert the inline `type` specifier merge back to a
separate `import type { ... }` line. The previous merge satisfied
`perfectionist/sort-imports` but violated
`import/consistent-type-specifier-style` (which requires top-level
type-only imports). Splitting them out satisfies both rules.
- service-account-validation.ts: capture the abort listener reference
in fetchWithTimeout and removeEventListener in the finally block.
Prevents listener buildup if a long-lived caller signal is reused
across many validation calls. No-op for the current single-shot use
in the onboarding flow but worth the tiny cost.
The step-transition telemetry useEffect added in PR #2287 already emits a `Builder Onboarding Step` event for every new `AndroidOnboardingStep`, so the new SA-import steps (`service-account-method-select`, `sa-json-existing-path`, `sa-json-existing-picker`, `sa-json-validating`, `sa-json-validation-failed`) automatically fire funnel events. This commit adds the missing failure-reason dimension. - Extend `AndroidOnboardingErrorCategory` with four SA-specific values: `sa_json_shape_invalid`, `sa_json_token_rejected`, `sa_json_no_app_access`, `sa_json_network_error`. Each mirrors a `ValidationResult.kind` from the validation module so PostHog funnels can split failures by recovery path (wrong file vs. SA not invited vs. transient network). - Add `mapSaValidationKindToCategory()` to `error-categories.ts` keeping the full Android error taxonomy in one place. - In the `sa-json-validating` effect, populate `errorCategoryRef.current` with the mapped category before transitioning to `sa-json-validation-failed`. The next telemetry-effect run reads it. - Extend the telemetry payload so `errorCategory` is attached for both `'error'` and `'sa-json-validation-failed'` step events. - Unit tests for the new mapper.
There was a problem hiding this comment.
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/ui/app.tsx (1)
2014-2034: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider clearing
errorCategoryRefafter "save anyway" to avoid stale category on subsequent failures.When the user picks "Save anyway",
errorCategoryRef.currentstill holds the mapped validation failure category from the previous attempt. If saving later fails (e.g., disk error atsaving-credentials), the new error would be emitted with the stale SA validation category instead of the actual save failure's category.Suggested fix
if (value === 'save-anyway') { ;(async () => { try { if (!serviceAccountJsonPath) throw new Error('No service account JSON path on record.') const bytes = await readFile(serviceAccountJsonPath) const base64 = bytes.toString('base64') setServiceAccountKeyBase64(base64) + errorCategoryRef.current = undefined await persist((p) => ({🤖 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/ui/app.tsx` around lines 2014 - 2034, errorCategoryRef.current can remain set to the previous validation category when the user chooses "save-anyway", causing subsequent failures (e.g., during persist or saving-credentials) to be reported with the stale category; inside the async handler for the 'save-anyway' branch (the IIFE handling value === 'save-anyway'), clear or reset errorCategoryRef.current (e.g., set to null or undefined) before calling setServiceAccountKeyBase64/persist/setStep so any later errors reported via handleError use the correct, fresh category.
🤖 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.
Outside diff comments:
In `@cli/src/build/onboarding/android/ui/app.tsx`:
- Around line 2014-2034: errorCategoryRef.current can remain set to the previous
validation category when the user chooses "save-anyway", causing subsequent
failures (e.g., during persist or saving-credentials) to be reported with the
stale category; inside the async handler for the 'save-anyway' branch (the IIFE
handling value === 'save-anyway'), clear or reset errorCategoryRef.current
(e.g., set to null or undefined) before calling
setServiceAccountKeyBase64/persist/setStep so any later errors reported via
handleError use the correct, fresh category.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0b7ae30-e22b-4520-a73a-ce4343bfb290
📒 Files selected for processing (4)
cli/src/build/onboarding/android/types.tscli/src/build/onboarding/android/ui/app.tsxcli/src/build/onboarding/error-categories.tstests/onboarding-error-categories.unit.test.ts
Addresses the P2 review observation that the 462-line validator and the new Android resume branches shipped with no direct automated coverage. tests/service-account-validation.unit.test.ts (20 tests): - parseServiceAccountKey: valid SA, malformed JSON, wrong `type`, missing private_key, empty client_email. - Shape-error branch of validateServiceAccountJson: malformed JSON, missing fields, wrong type. - Token-exchange classification: 401 → token-error (credentials), 500 → network-error (transient), non-JSON 200 → network-error, missing access_token → network-error, fetch rejection (ENOTFOUND) → network-error. - App-access probe: happy path asserts both insert + delete fire and the returned email/projectId are extracted from the JSON; 403 → no- app-access with both SA email and package name in the message; 404 → no-app-access; 503 from edits.insert → network-error. - Best-effort cleanup: edits.delete returning 500 still resolves the overall result as ok (draft auto-expires). - AbortSignal: pre-aborted signal short-circuits before any fetch; a live caller signal threads into the underlying fetch init. tests/android-onboarding-progress.unit.test.ts (12 tests): - Base routing for null/empty progress. - Legacy progress (no serviceAccountMethod) routes onto the OAuth path per the backward-compat contract: keystore-only → google-sign-in; past sign-in → play-developer-id-input; missing refresh token → re- auth. - Import path: package-not-chosen → android-package-select; package chosen → sa-json-existing-path; file picked → sa-json-validating; key bytes accepted → saving-credentials. The serviceAccountValidationSkipped flag does not affect routing. - Explicit 'generate' marker falls through to the OAuth-path rules identically to legacy progress. Mock fetch is wired via the `fetchImpl` injection point that was added to the validator for exactly this purpose. JWT signing runs for real against a freshly-generated 2048-bit RSA key per test file.
CodeRabbit pointed out that errorCategoryRef.current can persist across
the sa-json-validation-failed → saving-credentials transition when the
user picks "save anyway". Today this can't actually leak into a wrong-
category telemetry event because:
- The telemetry payload only attaches errorCategory for `'error'` and
`'sa-json-validation-failed'` steps; `'saving-credentials'` ignores
the ref entirely.
- `handleError` overwrites the ref before `setStep('error')`, so any
downstream failure carries its own freshly-mapped category.
It's still worth clearing for two reasons:
- Defense-in-depth: the ref's invariant should be "most recent
unresolved error context", and that's not true if a stale value sits
there after a recovery action.
- Consistency: the existing `error → retry` handler already clears the
ref (line 2593). The three recovery branches in
sa-json-validation-failed should follow the same pattern.
Clearing happens once at the top of the onChange handler so all three
branches (retry / save-anyway / oauth-fallback) get the same treatment
without duplicated bookkeeping.
hadessunxy-code
left a comment
There was a problem hiding this comment.
Reviewed the service-account import validation path. The validation layering and recovery states look solid overall; I left one hardening suggestion around the token endpoint trust boundary.
| client_email: obj.client_email as string, | ||
| private_key: obj.private_key as string, | ||
| project_id: obj.project_id as string, | ||
| token_uri: obj.token_uri as string, |
There was a problem hiding this comment.
Consider validating token_uri against Google's OAuth token endpoint before using it. Right now a syntactically valid service-account-shaped JSON can point token_uri at any URL, and the CLI will POST the signed JWT assertion there during validation. A normal Google-generated key will use https://oauth2.googleapis.com/token (or the legacy Google token host), so failing shape validation for anything else would avoid surprising outbound requests from a pasted/modified file and make the trust boundary clearer. A small unit test with a non-Google token_uri would lock this down.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|



Summary (AI generated)
build initonboarding TUI, alongside the existing Google OAuth auto-provisioning flowedits.insert→edits.delete) — same auth path fastlane'ssupplytakes at build timeMotivation (AI generated)
Capgo's Android onboarding currently has one path for setting up Google Play service-account credentials: a fully automated OAuth flow that creates a brand-new SA in GCP and invites it to the user's Play Console. Users who already manage their own SA — because they use fastlane outside Capgo, have a corporate GCP setup, or prefer to keep secrets self-managed — had no way to plug existing credentials into
build init. They had to fall back tobuild credentials save, which bypasses the entire onboarding TUI and offers no validation feedback.The existing keystore phase already exposes "existing vs generate" via
keystore-method-select. This PR brings the same affordance to the service-account phase.Business Impact (AI generated)
credentials savecommand (no guardrails)build initend-to-end without aborting to set things up manually, which directly improves the activation funnelTest Plan (AI generated)
tsc --noEmitclean (with--ignoreDeprecations 6.0to silence a pre-existing project config warning)eslint src/**/*.tsclean on changed filesbun run test:onboarding-progresspasses (covers the resume routing test suite)bun run test:onboarding-recoverypassesbun run test:onboarding-run-targetspassesbun run test:credentialspasses (17 of 17)bun build.mjssucceedsno-app-accesserror message names the SA email + package name → confirm "Try a different file" / "Save anyway" / "Set up via Google instead" all route correctlybuild init→ confirmgetAndroidResumeStepreturnssa-json-validatingand re-runs with saved file pathserviceAccountMethodfield → confirm resume continues on OAuth path (backward compatibility)Design Doc
docs/superpowers/specs/2026-05-21-android-import-service-account-design.md
Generated with AI
Summary by CodeRabbit