Skip to content

feat(cli): import existing service account JSON in Android build init#2317

Merged
WcaleNieWolny merged 9 commits into
mainfrom
claude/cool-babbage-57e00f
May 22, 2026
Merged

feat(cli): import existing service account JSON in Android build init#2317
WcaleNieWolny merged 9 commits into
mainfrom
claude/cool-babbage-57e00f

Conversation

@WcaleNieWolny
Copy link
Copy Markdown
Contributor

@WcaleNieWolny WcaleNieWolny commented May 21, 2026

Summary (AI generated)

  • Adds a new "I have my own service account JSON" path to the Android build init onboarding TUI, alongside the existing Google OAuth auto-provisioning flow
  • New fork sits between the keystore phase and the OAuth sign-in, mirroring the existing keystore existing/generate pattern
  • Imported SA is validated end-to-end against Google Play (edits.insertedits.delete) — same auth path fastlane's supply takes at build time
  • macOS native file picker with text-input fallback on other platforms
  • Soft-failure recovery: on validation failure the user can pick a different file, save anyway, or fall back to OAuth — no dead ends

Motivation (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 to build 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)

  • Removes a friction point for established Capgo customers who already have Play Console SAs in production — they no longer need to choose between Capgo onboarding (force re-provision) or the bare credentials save command (no guardrails)
  • Increases the share of users who finish build init end-to-end without aborting to set things up manually, which directly improves the activation funnel
  • Surfaces credential failures during onboarding instead of at first build, reducing time-to-first-success for the cloud build feature

Test Plan (AI generated)

  • tsc --noEmit clean (with --ignoreDeprecations 6.0 to silence a pre-existing project config warning)
  • eslint src/**/*.ts clean on changed files
  • bun run test:onboarding-progress passes (covers the resume routing test suite)
  • bun run test:onboarding-recovery passes
  • bun run test:onboarding-run-targets passes
  • bun run test:credentials passes (17 of 17)
  • bun build.mjs succeeds
  • Manual e2e on macOS — happy path: keystore → "I have my JSON" → package select → file picker → validation succeeds → save
  • Manual e2e — sad path: same flow with an SA that isn't invited to the test app → confirm no-app-access error message names the SA email + package name → confirm "Try a different file" / "Save anyway" / "Set up via Google instead" all route correctly
  • Manual e2e — non-macOS path: confirm text-input fallback with drag-and-drop hint
  • Manual resume: crash mid-validation → re-run build init → confirm getAndroidResumeStep returns sa-json-validating and re-runs with saved file path
  • Manual legacy progress: progress file without serviceAccountMethod field → 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

  • New Features
    • Import an existing Google Play service account JSON during Android setup (macOS picker + manual-path fallback).
    • Built-in JSON validation (shape, token exchange, Play Console access) with retry, “save anyway”, or fallback to OAuth.
    • Smarter onboarding routing, resume behavior, progress milestones, and cancellation for in-flight validations.
  • Documentation
    • Added design spec describing the import/validation flow, failure modes, telemetry, and recovery UX.
  • Tests
    • Added unit tests mapping validation failure kinds to onboarding error categories.

Review Change Stack

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.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 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 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.

Changes

Android Service Account Import Flow

Layer / File(s) Summary
State machine and type extensions
cli/src/build/onboarding/android/types.ts
New ServiceAccountMethod type and AndroidOnboardingStep variants for method selection and JSON import/validation; extended AndroidOnboardingProgress with method, path, and skipped fields; updated progress mappings and phase labels.
Service account JSON validation module
cli/src/build/onboarding/android/service-account-validation.ts
Parses service-account JSON, signs RS256 JWT assertions, exchanges for OAuth tokens with timeout/AbortSignal, probes Play Console via edits.insert/edits.delete, and returns structured non-throwing ValidationResult variants.
Service account JSON file picker
cli/src/build/onboarding/file-picker.ts
Exports openServiceAccountJsonPicker() to open the macOS native file picker filtered to .json files for importing service accounts.
Resume routing for service account method
cli/src/build/onboarding/android/progress.ts
Routes resume into the new service-account method fork, handling existing-import progress, package gating, and legacy partial-completion recovery.
Android onboarding UI: service account method fork
cli/src/build/onboarding/android/ui/app.tsx
Adds state and refs for method selection and validation, implements picker/manual-path handlers, validates JSON bytes against package, persists base64 key on success, exposes validation-failed recovery (retry/save-anyway/fallback), aborts/cleans validation on transitions, updates telemetry, and updates keystore-completion smart-routes.
Design specification and testing plan
docs/superpowers/specs/2026-05-21-android-import-service-account-design.md
Spec describes the import flow, validation contract, UX recovery options, resume rules, telemetry events, and testing strategy.
Error mapping and tests
cli/src/build/onboarding/error-categories.ts, tests/onboarding-error-categories.unit.test.ts
Adds mapSaValidationKindToCategory to map validation failure kinds to onboarding error categories and unit tests asserting the mappings.

Sequence Diagram

sequenceDiagram
  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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • Cap-go/capgo#2064: Introduced the core Android onboarding step-machine and resume logic that this PR extends with the service-account method fork and JSON import path.

Suggested reviewers

  • zinc-builds
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 69.23% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the primary change: adding the ability to import existing service account JSON during Android build initialization.
Description check ✅ Passed The description includes Summary, Motivation, Business Impact, Test Plan (with checkboxes), and a Design Doc link. All required sections are present and substantive.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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


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

…7e00f

# Conflicts:
#	cli/src/build/onboarding/android/ui/app.tsx
Copy link
Copy Markdown
Contributor

@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: 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9523f13 and baa827e.

📒 Files selected for processing (6)
  • cli/src/build/onboarding/android/progress.ts
  • cli/src/build/onboarding/android/service-account-validation.ts
  • cli/src/build/onboarding/android/types.ts
  • cli/src/build/onboarding/android/ui/app.tsx
  • cli/src/build/onboarding/file-picker.ts
  • docs/superpowers/specs/2026-05-21-android-import-service-account-design.md

Comment thread cli/src/build/onboarding/android/progress.ts Outdated
Comment thread cli/src/build/onboarding/android/service-account-validation.ts Outdated
Comment thread cli/src/build/onboarding/android/ui/app.tsx
Comment thread cli/src/build/onboarding/android/ui/app.tsx
Comment thread docs/superpowers/specs/2026-05-21-android-import-service-account-design.md Outdated
Copy link
Copy Markdown

@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: 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".

Comment thread cli/src/build/onboarding/android/progress.ts Outdated
Comment thread cli/src/build/onboarding/android/service-account-validation.ts
@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq Bot commented May 21, 2026

Merging this PR will not alter performance

✅ 43 untouched benchmarks
⏩ 2 skipped benchmarks1


Comparing claude/cool-babbage-57e00f (bfa23f9) with main (44e889b)

Open in CodSpeed

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Copy link
Copy Markdown
Contributor

@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

🤖 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

📥 Commits

Reviewing files that changed from the base of the PR and between baa827e and d82ddce.

📒 Files selected for processing (2)
  • cli/src/build/onboarding/android/types.ts
  • cli/src/build/onboarding/android/ui/app.tsx

Comment thread cli/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.
Copy link
Copy Markdown
Contributor

@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

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 value

Event listeners not removed on successful fetch completion.

The abort event listeners added to args.signal and timeoutController.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 if args.signal is 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

📥 Commits

Reviewing files that changed from the base of the PR and between d82ddce and e3abb2f.

📒 Files selected for processing (5)
  • cli/src/build/onboarding/android/progress.ts
  • cli/src/build/onboarding/android/service-account-validation.ts
  • cli/src/build/onboarding/android/ui/app.tsx
  • cli/src/build/onboarding/file-picker.ts
  • docs/superpowers/specs/2026-05-21-android-import-service-account-design.md

Comment thread cli/src/build/onboarding/android/ui/app.tsx Outdated
- 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.
Copy link
Copy Markdown
Contributor

@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.

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 value

Consider clearing errorCategoryRef after "save anyway" to avoid stale category on subsequent failures.

When the user picks "Save anyway", errorCategoryRef.current still holds the mapped validation failure category from the previous attempt. If saving later fails (e.g., disk error at saving-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

📥 Commits

Reviewing files that changed from the base of the PR and between 9d17d20 and 5754c49.

📒 Files selected for processing (4)
  • cli/src/build/onboarding/android/types.ts
  • cli/src/build/onboarding/android/ui/app.tsx
  • cli/src/build/onboarding/error-categories.ts
  • tests/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.
Copy link
Copy Markdown

@hadessunxy-code hadessunxy-code left a comment

Choose a reason for hiding this comment

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

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,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@socket-security
Copy link
Copy Markdown

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

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedbun-types@​1.3.13100100859870
Addedbetter-qr@​0.1.17610010086100
Addedcountry-code-to-flag-emoji@​2.1.0801008286100
Addedcron-schedule@​6.0.010010010081100
Addedchartjs-chart-funnel@​4.2.59610010081100
Addedci-info@​4.4.010010010084100
Addedcommander@​14.0.39810010084100
Addedarktype@​2.2.010010010085100
Addedchart.js@​4.5.1961008686100

View full report

@sonarqubecloud
Copy link
Copy Markdown

@WcaleNieWolny WcaleNieWolny merged commit 97ee6e6 into main May 22, 2026
44 checks passed
@WcaleNieWolny WcaleNieWolny deleted the claude/cool-babbage-57e00f branch May 22, 2026 05:53
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