feat(mcp): full MCP-conducted Builder onboarding — iOS + android + tail, gate flipped (PR 2)#2492
Open
WcaleNieWolny wants to merge 127 commits into
Open
feat(mcp): full MCP-conducted Builder onboarding — iOS + android + tail, gate flipped (PR 2)#2492WcaleNieWolny wants to merge 127 commits into
WcaleNieWolny wants to merge 127 commits into
Conversation
…ship test hooks from the bundle + CI leak guard Proven on the real CLI build: a dev-gated branch importing a src/__dev__/ module is fully DCE'd from dist/index.js (marker absent). Apple-API spoofing for AI tests (PR 2) will live in src/__dev__/ behind this gate, physically absent from the NPM build.
…ndroidOnboardingProgress Brings the platform-agnostic onboarding engine (mcp/*, flow.ts) + its additive shared deps (keystore/oauth-google/oauth-scopes/output-record/schemas) onto main. Unifies the data model: main's step set (incl. the 4 TUI-only ai-analysis-* + resume-prompt) plus the 3 MCP markers (activePlatform, keystorePasswordGenerated, keystorePasswordManual); flow.ts KIND_TABLE covers the TUI-only steps. main's android/ui/app.tsx kept verbatim (TUI render-only rewrite is a later plan). Behavior is moved/renamed only — NOT assumed correct; the android audit is Plan 2. Verified: typecheck 0 errors; engine 87 unit + e2e hermetic gate PASS; main's onboarding tests still pass. Covers plan Tasks 2-3 (one green commit; they don't split cleanly).
…stripped; shared engine still ships)
…non-empty) to match main; tighten gate
…gcp-project-create-name) to match main
…r-cancel gate (credentials-exist + backing-up) to match main — closes a data-safety gap
… engine (persistAndStep derives next via getAndroidResumeStep; behavior-preserving + dev mismatch guard) + sequencing smoke test
…rough the shared runAndroidEffect (behavior-preserving; logs kept)
…source-of-truth (remove redundant state mirrors; conservative)
…code now that the engine drives the flow
… android mapper (drop unsafe cast)
…oidEffect through the post-save tail)
saving-credentials now stashes the CI-secret entries (createCiSecretEntries) in transient before routing to ask-build, mirroring the Ink TUI's doSaveCredentials. Adds the additive post-save tail transient fields to AndroidStepCtx and the rebuildTailCredentials/tailCiSecretEntries helpers the later tail steps reuse to re-derive the same entries from progress.
detecting-ci-secrets discovers CI destinations and persists the single chosen target (GitHub → ask-github-actions-setup, GitLab → ask-ci-secrets, many → target-select). checking-ci-secrets resolves the GitHub repo label + existing keys and gates on confirm-secrets-push / confirm-ci-secret-overwrite. uploading-ci-secrets pushes the entries and branches with-workflow → pick-package-manager vs build-complete. Routing mirrors the Ink TUI tail.
…ep(s) exporting-env writes the 0o600 .env (defaultExportPath fallback) and routes an existing file to confirm-env-export-overwrite (persisting the resolved path); overwrite-and-export-env re-exports with overwrite=true. writing-workflow-file generates + writes .github/workflows/capgo-build.yml (overwrite) using the recorded build-script choice + secret keys. All finish at build-complete, mirroring the Ink TUI tail.
requesting-build fires capgo build request through the pre-bound requestBuildInternal dep (driver owns apikey/logger/silent). A successful build with pending CI entries routes to detecting-ci-secrets to offer the secret push; with no entries it finishes at build-complete. A failed build with a captured log surfaces the AI job id and routes to the TUI-only ai-analysis-prompt; otherwise build-complete. Mirrors the Ink TUI tail.
… transient (parity with TUI; no rebuild, no secrets on disk)
…e; android delegates
…mapIosViewToStepView)
…ient/deps surface + additive persisted fields
…ning/mobileprovision types (not placeholders)
…t-save tail through the shared tail module
…der-setup prompt Discoverability fixes for MCP clients (e.g. Codex) that explore-first instead of reaching for the onboarding tool: - start_capgo_builder_onboarding description is now imperative + anti-explore: 'ALWAYS call this FIRST ... Do NOT inspect the repo, read config files, or web-search to do this yourself', with more trigger words (set up/configure/ connect/enable/troubleshoot · Capgo Builder/native builds/cloud builds/signing) - add an MCP prompt 'capgo-builder-setup' (server.prompt, optional-chained so the 2-tool test mock is unaffected) — clients that support prompts surface it as a slash command; invoking it injects a message that kicks off the tool-driven flow so the user never has to name the tool
The MCP server was constructed with only {name, version}, so clients (Codex,
Claude Code) got zero connect-time guidance on when to reach for the onboarding
tools and fell back to inspecting the repo themselves.
Add buildServerInstructions(): always describes the general Capgo Cloud tools,
and — only when the onboarding flag is on (same gate as tool registration) —
appends a steer to call start_capgo_builder_onboarding first. Kept under the
512-char cap some clients (Codex) apply; pinned by test-mcp-instructions.
… squash-merge) PR-1 (#2495) was squash-merged into main, breaking shared-commit ancestry, so this merge surfaced ~20 add/add conflicts on the MCP/onboarding files even though PR-2 is a content superset of PR-1. Resolved by taking PR-2's version for all onboarding/MCP source + tests (verified: the only main-only commits touching them were the PR-1 squash + release bumps — no separate fix to fold in), keeping main's auto-merged package.json version (8.4.0) plus PR-2's test scripts, and pinning the cli-mcp-tests submodule to PR-2's c511d08. Also folds in the pre-existing dead-code gate fix (PR-2's own exports, never in main): delete unused clearTailParked; knip-ignore the test-only exports clearAllSessions / MCP_ONLY_STATES / MCP_UNREACHABLE_STEPS (consumed only by the private cli-mcp-tests suite knip can't see). Gates green post-merge: typecheck, lint, knip, build, instructions test, + 6 targeted onboarding/MCP tests.
Codex (a strict MCP client) dropped the connection with "Transport closed"
the moment it called start_capgo_builder_onboarding. Root cause: sdk.listApps()
called listAppInternal(opts, false) — silent=false — so clack intro/log UI
('List apps in Capgo', 'Apps not found', …) was written to stdout, the same
stream the stdio transport frames JSON-RPC on. The non-JSON bytes corrupt the
framing and the client closes the transport. (This also affected the existing
capgo_list_apps tool; lenient clients just tolerated the garbage.)
- sdk.listApps: pass silent=true (every other SDK method already does). The SDK
is a programmatic API and must never write to stdout.
- Defense-in-depth: installMcpStdoutGuard() routes ALL ambient stdout writes to
stderr and hands the transport a dedicated stream to the real stdout, so no
stray clack/console output from any future tool or dependency can corrupt the
protocol mid-flow. Wired into startMcpServer.
- Tests: test-mcp-stdout-guard pins both halves of the guard contract.
Verified against the real server: start_capgo_builder_onboarding now emits only
valid JSON-RPC on stdout (0 pollution) and returns its result.
…ogress
A fresh start_capgo_builder_onboarding silently resumed whichever platform had
leftover progress on disk: decideStart/decideAdvance called activePlatform(facts),
which scanned the ios/android progress files (android-checked-first). Two problems:
1. On a dual-platform project, a fresh start skipped the platform picker and
dropped the user straight into (e.g.) the iOS credentials-replace gate — they
were never asked which platform they wanted. (Reported from a live Codex run.)
2. The MCP server is long-lived per client, so platform is session state — but
reading it from shared disk means two concurrent sessions onboarding the same
app (one iOS, one Android) would clobber each other (the iOS session would get
'android' the moment the Android session wrote any progress).
This matches what the TUI already does: platform comes from the user's pick (or
single-platform auto-detect), never from leftover progress; disk only resumes the
STEP within the chosen platform.
- session-state.ts: track the chosen platform in the process-local registry
(getSessionPlatform / setSessionPlatform). Process-local ⇒ concurrent sessions
are isolated; a restart loses it and the flow re-asks the picker.
- engine.ts: decideStart / decideAdvance / checkBuild resolve platform from session
memory; commit it on the picker answer / explicit {platform} / single-platform
auto-route. runStart clears it so a fresh start always re-offers the picker.
Removed the disk-scanning activePlatform() helper (no longer reachable).
- test-mcp-platform-select: pins ask-on-fresh-start, android-pick-ignores-iOS-disk,
bare-next_step-uses-session, and start-re-asks.
NOTE: private cli-mcp-tests cases that seed progress and call runStart/runAdvance({})
expecting a silent disk resume encode the OLD behavior and must be updated to pick a
platform first.
…, not listApps isAppRegistered called sdk.listApps() — an unfiltered apps.select() that times out on large databases (full-table RBAC RLS scan; see app-list fix PR #2497). On timeout it false-negated, so the engine tried to re-register an app the user OWNS, and addApp's 'already exists' was rendered as a bogus 'already exists and is not in your account' conflict — blocking onboarding before the platform picker even appeared. - sdk: add appHasAccess(appId) — a targeted existence+permission check via checkAppExistsAndHasPermissionOrgErr (the same one app set / channel / bundle use): single-app, no full-table scan, RBAC-correct, silent (no stdout on the MCP channel). - isAppRegistered now uses sdk.appHasAccess: owned app -> true -> skip registration -> proceed; missing -> register; genuinely-not-yours -> real conflict. Verified end-to-end against the real backend: start_capgo_builder_onboarding on an owned app now reaches platform-select instead of the false app-id-conflict.
…ign-in explanation The google-sign-in explanation told the user to 'approve the permissions Google shows' without naming them. List the exact scopes Capgo requests so the user knows what they're granting before approving: - https://www.googleapis.com/auth/cloud-platform (create the GCP service account) - https://www.googleapis.com/auth/androidpublisher (invite it to Play Console, release-only) - https://www.googleapis.com/auth/userinfo.email (identify the signed-in account) Plus the local/revocable trust framing (tokens never reach Capgo servers; revoke at myaccount.google.com/permissions). test-mcp-explain-scopes pins this AND ties it to OAUTH_SCOPES_FOR_ONBOARDING, so a scope added to the flow but left undocumented fails the build instead of under-disclosing.
…(never stuck)
If the OAuth browser never opened, was closed, or the flow stalled on 'still waiting',
the MCP only re-rendered 'still waiting' forever — there was no way to reopen it, so the
user got stuck (Capgo saw the sign-in as in progress but never reopened the browser).
Add a reopenSignIn input: at google-sign-in it clears the in-flight OAuth session and
begins a fresh flow (a new browser window). Threaded through the schema (so the MCP SDK
doesn't strip the field), the engine OnboardingInput type, decideAndroid (clear-before-
poll → re-begin), and drive. The 'opened browser' and 'still waiting' gates now tell the
agent it can call next_step({ reopenSignIn: true }) to reopen.
test-mcp-oauth-reopen pins it: reopen clears + re-begins the session, direct (decideAndroid)
and end-to-end (runAdvance → schema + drive threading).
… directly; fix cross-platform gate misroute
Picking iOS then saying 'android' had no clean recovery: the agent improvised a cancel,
and the engine misrouted the iOS credentials-exist answer into a leftover ANDROID
progress file on disk (the shared gate was disambiguated by disk shape, not the session).
- start_capgo_builder_onboarding now accepts an optional platform ('ios'|'android'):
it commits to that platform and skips the picker — handles 'set up Capgo Builder for
iOS' up front AND switching after a wrong pick (it resets the session platform, so it
subsumes a cancel->start). runStart(deps, platform?) threads it through.
- The SHARED credentials-exist gate is disambiguated by the SESSION platform now, not a
disk heuristic, so a stray answer can't misroute into a leftover other-platform file.
- ONBOARDING_RULES tells the agent to switch via start({platform}) instead of cancelling
the current step or answering it with the other platform in mind.
Verified end-to-end on the real backend: start({platform:'ios'}) -> iOS, then
start({platform:'android'}) -> Android, a clean switch despite both platforms having
leftover progress on disk. test-mcp-platform-select pins start(platform) + the switch.
…_step
The first cloud build is run only by the start_capgo_build tool (build-job
registry + capgo_build_wait/capgo_build_logs). next_step({ runBuild: true })
is the wrong tool and is now rejected with a corrective that points at
start_capgo_build (state build-use-start-tool, kind error), keeping the
appid-unsafe and credentials-not-ready guards. runBuild:false still skips.
checkBuild stays record-only (reads the finished BuildOutputRecord, enters
the CI tail). Removes the dead buildHandoffResult / build-run-handoff
terminal-command path and terminal-launch.ts; updates MCP_ONLY_STATES and
explanations.ts for the new state. Also carries the resume-prompt session
state (resumeResolvedFor) that engine.ts depends on.
A failed build must not be escaped by restarting onboarding or fake-advancing. The rules already said so, but only in the JSON blob — the prominent prose didn't, and start_capgo_builder_onboarding's description even invited calling it to "troubleshoot cloud builds". Strengthen the build-failed result's summary + next.instruction to state the required-gate rule, forbid start_capgo_builder_onboarding / capgo_builder_onboarding_next_step, and name the only retry path (start_capgo_build, after the user agrees — no self-edit/rebuild). Add matching carve-outs to the start/next_step tool descriptions. Driven by the build-fail-no-escape agentic e2e: Codex previously escaped via start/next_step (and self-edited files); with this guidance it holds the line (retries via start_capgo_build, stays truthful, refuses to fake completion).
… + agentic build-fail journey)
… build-phase migration)
… RPC stubs → verify-tree green)
…clearer keystore UX Three keystore-onboarding UX fixes: - Random password was generated but only surfaced ~2 steps later (folded into the Google-Play question), so users often never saw it and were locked out of their own keystore. Surface it AT keystore-new-cn — the moment the user picks "random" — via context.keystorePassword + an explicit instruction to relay it. - When the user has no keystore and picks "generate", keystore-new-alias now reassures them a keystore FILE will be created and saved to disk on their machine (they will receive one; it won't disappear; they keep full control). - Passwords in the chat are fine: drop the "never paste secrets in the chat" friction from ONBOARDING_RULES and the manual-password gates, and strongly tell the AI it is 100% OK for the user to paste keystore passwords directly in chat. File-based credentials (.p8, service-account JSON) are still collected as paths.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/test/test-mcp.mjs (1)
66-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert the full onboarding MCP runtime contract (include explain tool).
This test validates runtime
listTools, but it currently checks only two onboarding tools. Per the shipped contract,capgo_builder_onboarding_explainshould also be required; otherwise a registration regression can slip through.Suggested fix
const requiredTools = [ 'capgo_list_apps', 'capgo_upload_bundle', 'capgo_update_channel', 'capgo_get_stats', 'start_capgo_builder_onboarding', 'capgo_builder_onboarding_next_step', + 'capgo_builder_onboarding_explain', ]🤖 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/test/test-mcp.mjs` around lines 66 - 73, The `requiredTools` array in the test is missing the `capgo_builder_onboarding_explain` tool, which is part of the shipped onboarding MCP runtime contract. Add `capgo_builder_onboarding_explain` to the `requiredTools` array to ensure the test validates all required onboarding tools and prevent registration regressions from going undetected.cli/src/build/onboarding/ios/flow.ts (1)
995-1033:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winForward
detectPackageManagerthroughtoTailDeps.Line 469 adds
detectPackageManagertoIosEffectDeps, but the adapter at Line 995-1033 does not pass it into the shared tail deps. This silently drops iOS lockfile-based package-manager recommendation behavior in tail routing.Suggested patch
function toTailDeps(deps: IosEffectDeps): TailEffectDeps<OnboardingProgress> { return { @@ exportCredentialsToEnv: deps.exportCredentialsToEnv, defaultExportPath: deps.defaultExportPath, + detectPackageManager: deps.detectPackageManager, generateWorkflow: deps.generateWorkflow, writeWorkflowFile: deps.writeWorkflowFile,🤖 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/ios/flow.ts` around lines 995 - 1033, The toTailDeps function does not forward the detectPackageManager property from deps to the returned TailEffectDeps object, even though detectPackageManager was added to IosEffectDeps. Add detectPackageManager: deps.detectPackageManager to the object returned by toTailDeps, following the same pattern as other forwarded properties like getPackageScripts and findProjectType, to ensure the iOS lockfile-based package-manager recommendation behavior is preserved in tail routing.
🤖 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/mcp/build-job.ts`:
- Around line 185-192: The status check in the build job record evaluation is
comparing against the wrong string value. The code at line 188 checks if
rec.status === 'success', but the actual status value written by
writeBuildOutputRecord in request.ts is 'succeeded'. Change the string
comparison from 'success' to 'succeeded' to match the actual status value that
is written to the build record. This will fix the issue where builds that
succeed without an output URL are incorrectly reported as failed instead of
completed.
In `@cli/src/build/onboarding/mcp/step-input.ts`:
- Around line 330-349: The validation logic in the verify step input handler is
missing a required check: when verifyAction is set to 'pick', verifyAppId must
be provided and cannot be undefined or null. Add a new validation condition
after the existing verifyAppId checks that returns an error with an appropriate
message if verifyAction equals 'pick' but verifyAppId is undefined or null. This
ensures incomplete verify-app answers are caught at the gate rather than
deferred downstream.
In `@cli/test/test-android-oauth.mjs`:
- Around line 294-304: When the session is closed in the finally block at the
session.close() call, the session.result promise may reject if the close
operation fails. Currently, this rejection is never observed, which can cause an
unhandled promise rejection that intermittently fails CI. In the finally block
where session.close() is called, also await or handle the session.result promise
rejection (using a catch clause or similar error handling) to ensure all
rejections from closing the session are properly handled and do not become
unhandled rejections.
---
Outside diff comments:
In `@cli/src/build/onboarding/ios/flow.ts`:
- Around line 995-1033: The toTailDeps function does not forward the
detectPackageManager property from deps to the returned TailEffectDeps object,
even though detectPackageManager was added to IosEffectDeps. Add
detectPackageManager: deps.detectPackageManager to the object returned by
toTailDeps, following the same pattern as other forwarded properties like
getPackageScripts and findProjectType, to ensure the iOS lockfile-based
package-manager recommendation behavior is preserved in tail routing.
In `@cli/test/test-mcp.mjs`:
- Around line 66-73: The `requiredTools` array in the test is missing the
`capgo_builder_onboarding_explain` tool, which is part of the shipped onboarding
MCP runtime contract. Add `capgo_builder_onboarding_explain` to the
`requiredTools` array to ensure the test validates all required onboarding tools
and prevent registration regressions from going undetected.
🪄 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: 2b00d8f9-a5ff-4e03-99a1-e7a604a51555
📒 Files selected for processing (39)
cli/build.mjscli/package.jsoncli/src/build/onboarding/android/flow.tscli/src/build/onboarding/android/progress.tscli/src/build/onboarding/ios/flow.tscli/src/build/onboarding/ios/progress.tscli/src/build/onboarding/mcp/app-id-validation.tscli/src/build/onboarding/mcp/build-job.tscli/src/build/onboarding/mcp/build-tools.tscli/src/build/onboarding/mcp/contract.tscli/src/build/onboarding/mcp/engine.tscli/src/build/onboarding/mcp/explanations.tscli/src/build/onboarding/mcp/oauth-session.tscli/src/build/onboarding/mcp/onboarding-tools.tscli/src/build/onboarding/mcp/session-state.tscli/src/build/onboarding/mcp/step-input.tscli/src/build/onboarding/mcp/tail-progress.tscli/src/build/onboarding/mcp/terminal-launch.tscli/src/build/output-record.tscli/src/dev-flag.d.tscli/src/mcp/instructions.tscli/src/mcp/server.tscli/src/mcp/stdout-guard.tscli/src/schemas/onboarding.tscli/src/sdk.tscli/src/support/contact-support.tscli/test/test-android-keystore.mjscli/test/test-android-oauth.mjscli/test/test-android-tail-routing.mjscli/test/test-dev-gate-stripped.mjscli/test/test-init-guardrails.mjscli/test/test-mcp-explain-scopes.mjscli/test/test-mcp-instructions.mjscli/test/test-mcp-oauth-reopen.mjscli/test/test-mcp-platform-select.mjscli/test/test-mcp-stdout-guard.mjscli/test/test-mcp.mjsknip.jsonprivate/cli-mcp-tests
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
💤 Files with no reviewable changes (1)
- cli/src/build/onboarding/mcp/terminal-launch.ts
Comment on lines
+185
to
+192
| if (rec) { | ||
| if (rec.jobId) | ||
| entry.cloudJobId = rec.jobId | ||
| const done = rec.status === 'success' || Boolean(rec.outputUrl) | ||
| if (done) | ||
| return { ...base, status: 'completed', outputUrl: rec.outputUrl ?? undefined, qrCodeAscii: rec.qrCodeAscii ?? undefined } | ||
| return { ...base, status: 'failed', error: `The build did not succeed (status: ${rec.status}).` } | ||
| } |
Contributor
There was a problem hiding this comment.
Build status check uses wrong value: 'success' vs 'succeeded'
Line 188 checks rec.status === 'success', but writeBuildOutputRecord (called from request.ts) writes status: finalStatus where finalStatus is 'succeeded' (see context snippet 1, line 1984). The check will never match.
Currently this is masked because Boolean(rec.outputUrl) is also checked, so builds with --output-upload work. However, if a build succeeds without an output URL (e.g., --output-upload not passed), the status check fails and deriveStatus incorrectly reports 'failed'.
Proposed fix
- const done = rec.status === 'success' || Boolean(rec.outputUrl)
+ const done = rec.status === 'succeeded' || Boolean(rec.outputUrl)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (rec) { | |
| if (rec.jobId) | |
| entry.cloudJobId = rec.jobId | |
| const done = rec.status === 'success' || Boolean(rec.outputUrl) | |
| if (done) | |
| return { ...base, status: 'completed', outputUrl: rec.outputUrl ?? undefined, qrCodeAscii: rec.qrCodeAscii ?? undefined } | |
| return { ...base, status: 'failed', error: `The build did not succeed (status: ${rec.status}).` } | |
| } | |
| if (rec) { | |
| if (rec.jobId) | |
| entry.cloudJobId = rec.jobId | |
| const done = rec.status === 'succeeded' || Boolean(rec.outputUrl) | |
| if (done) | |
| return { ...base, status: 'completed', outputUrl: rec.outputUrl ?? undefined, qrCodeAscii: rec.qrCodeAscii ?? undefined } | |
| return { ...base, status: 'failed', error: `The build did not succeed (status: ${rec.status}).` } | |
| } |
🤖 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/mcp/build-job.ts` around lines 185 - 192, The status
check in the build job record evaluation is comparing against the wrong string
value. The code at line 188 checks if rec.status === 'success', but the actual
status value written by writeBuildOutputRecord in request.ts is 'succeeded'.
Change the string comparison from 'success' to 'succeeded' to match the actual
status value that is written to the build record. This will fix the issue where
builds that succeed without an output URL are incorrectly reported as failed
instead of completed.
Comment on lines
+330
to
+349
| if (verify.length > 0) { | ||
| if (currentStep !== 'verify-app') { | ||
| return { | ||
| ok: false, | ||
| message: 'verifyAction answers only the verify-app step, which is not the current step. Answer the current step instead.', | ||
| } | ||
| } | ||
| if (input.verifyAction === undefined || input.verifyAction === null) { | ||
| return { | ||
| ok: false, | ||
| message: 'verifyAppId is only valid together with verifyAction: "pick". Call next_step with verifyAction (and verifyAppId only when picking an app).', | ||
| } | ||
| } | ||
| if (input.verifyAppId !== undefined && input.verifyAppId !== null && input.verifyAction !== 'pick') { | ||
| return { | ||
| ok: false, | ||
| message: 'verifyAppId is only valid together with verifyAction: "pick" — remove verifyAppId, or use verifyAction: "pick".', | ||
| } | ||
| } | ||
| return { ok: true } |
Contributor
There was a problem hiding this comment.
Enforce verifyAppId when verifyAction is pick.
Line 349 currently allows verifyAction: 'pick' without verifyAppId, which lets an incomplete verify-app answer pass the strict gate and defers failure downstream.
Proposed fix
if (verify.length > 0) {
if (currentStep !== 'verify-app') {
return {
ok: false,
message: 'verifyAction answers only the verify-app step, which is not the current step. Answer the current step instead.',
}
}
@@
if (input.verifyAppId !== undefined && input.verifyAppId !== null && input.verifyAction !== 'pick') {
return {
ok: false,
message: 'verifyAppId is only valid together with verifyAction: "pick" — remove verifyAppId, or use verifyAction: "pick".',
}
}
+ if (input.verifyAction === 'pick' && (input.verifyAppId === undefined || input.verifyAppId === null || String(input.verifyAppId).trim().length === 0)) {
+ return {
+ ok: false,
+ message: 'verifyAction: "pick" requires verifyAppId. Provide the selected app bundle id in verifyAppId.',
+ }
+ }
return { ok: true }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (verify.length > 0) { | |
| if (currentStep !== 'verify-app') { | |
| return { | |
| ok: false, | |
| message: 'verifyAction answers only the verify-app step, which is not the current step. Answer the current step instead.', | |
| } | |
| } | |
| if (input.verifyAction === undefined || input.verifyAction === null) { | |
| return { | |
| ok: false, | |
| message: 'verifyAppId is only valid together with verifyAction: "pick". Call next_step with verifyAction (and verifyAppId only when picking an app).', | |
| } | |
| } | |
| if (input.verifyAppId !== undefined && input.verifyAppId !== null && input.verifyAction !== 'pick') { | |
| return { | |
| ok: false, | |
| message: 'verifyAppId is only valid together with verifyAction: "pick" — remove verifyAppId, or use verifyAction: "pick".', | |
| } | |
| } | |
| return { ok: true } | |
| if (verify.length > 0) { | |
| if (currentStep !== 'verify-app') { | |
| return { | |
| ok: false, | |
| message: 'verifyAction answers only the verify-app step, which is not the current step. Answer the current step instead.', | |
| } | |
| } | |
| if (input.verifyAction === undefined || input.verifyAction === null) { | |
| return { | |
| ok: false, | |
| message: 'verifyAppId is only valid together with verifyAction: "pick". Call next_step with verifyAction (and verifyAppId only when picking an app).', | |
| } | |
| } | |
| if (input.verifyAppId !== undefined && input.verifyAppId !== null && input.verifyAction !== 'pick') { | |
| return { | |
| ok: false, | |
| message: 'verifyAppId is only valid together with verifyAction: "pick" — remove verifyAppId, or use verifyAction: "pick".', | |
| } | |
| } | |
| if (input.verifyAction === 'pick' && (input.verifyAppId === undefined || input.verifyAppId === null || String(input.verifyAppId).trim().length === 0)) { | |
| return { | |
| ok: false, | |
| message: 'verifyAction: "pick" requires verifyAppId. Provide the selected app bundle id in verifyAppId.', | |
| } | |
| } | |
| return { ok: true } |
🤖 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/mcp/step-input.ts` around lines 330 - 349, The
validation logic in the verify step input handler is missing a required check:
when verifyAction is set to 'pick', verifyAppId must be provided and cannot be
undefined or null. Add a new validation condition after the existing verifyAppId
checks that returns an error with an appropriate message if verifyAction equals
'pick' but verifyAppId is undefined or null. This ensures incomplete verify-app
answers are caught at the gate rather than deferred downstream.
Comment on lines
+294
to
+304
| const session = await startOAuthFlow(config, { onAuthUrl: () => {}, onStatus: () => {} }) | ||
| try { | ||
| const parsedUrl = new URL(session.authUrl) | ||
| assertEquals(parsedUrl.searchParams.get('client_id'), 'my-client-id.apps.googleusercontent.com') | ||
| const scope = parsedUrl.searchParams.get('scope') | ||
| assert(scope.includes('openid'), 'scope must include openid') | ||
| assert(scope.includes('userinfo.email'), 'scope must include userinfo.email') | ||
| } | ||
| finally { | ||
| session.close() | ||
| } |
Contributor
There was a problem hiding this comment.
Handle session.result rejection in the forced-close path.
At Line 303, this test closes the session but never observes session.result. If close causes result to reject, that can surface as an unhandled rejection and intermittently fail CI.
Suggested fix
await test('startOAuthFlow authUrl contains clientId and expected scopes', async () => {
const { startOAuthFlow } = await importOAuth()
const config = {
clientId: 'my-client-id.apps.googleusercontent.com',
scopes: ['openid', 'https://www.googleapis.com/auth/userinfo.email'],
}
const session = await startOAuthFlow(config, { onAuthUrl: () => {}, onStatus: () => {} })
+ const resultHandled = session.result.catch(() => {})
try {
const parsedUrl = new URL(session.authUrl)
assertEquals(parsedUrl.searchParams.get('client_id'), 'my-client-id.apps.googleusercontent.com')
const scope = parsedUrl.searchParams.get('scope')
assert(scope.includes('openid'), 'scope must include openid')
assert(scope.includes('userinfo.email'), 'scope must include userinfo.email')
}
finally {
session.close()
+ await resultHandled
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const session = await startOAuthFlow(config, { onAuthUrl: () => {}, onStatus: () => {} }) | |
| try { | |
| const parsedUrl = new URL(session.authUrl) | |
| assertEquals(parsedUrl.searchParams.get('client_id'), 'my-client-id.apps.googleusercontent.com') | |
| const scope = parsedUrl.searchParams.get('scope') | |
| assert(scope.includes('openid'), 'scope must include openid') | |
| assert(scope.includes('userinfo.email'), 'scope must include userinfo.email') | |
| } | |
| finally { | |
| session.close() | |
| } | |
| const session = await startOAuthFlow(config, { onAuthUrl: () => {}, onStatus: () => {} }) | |
| const resultHandled = session.result.catch(() => {}) | |
| try { | |
| const parsedUrl = new URL(session.authUrl) | |
| assertEquals(parsedUrl.searchParams.get('client_id'), 'my-client-id.apps.googleusercontent.com') | |
| const scope = parsedUrl.searchParams.get('scope') | |
| assert(scope.includes('openid'), 'scope must include openid') | |
| assert(scope.includes('userinfo.email'), 'scope must include userinfo.email') | |
| } | |
| finally { | |
| session.close() | |
| await resultHandled | |
| } |
🤖 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/test/test-android-oauth.mjs` around lines 294 - 304, When the session is
closed in the finally block at the session.close() call, the session.result
promise may reject if the close operation fails. Currently, this rejection is
never observed, which can cause an unhandled promise rejection that
intermittently fails CI. In the finally block where session.close() is called,
also await or handle the session.result promise rejection (using a catch clause
or similar error handling) to ensure all rejections from closing the session are
properly handled and do not become unhandled rejections.
…d Play app Generating a fresh keystore only makes sense for a first build. If the app was already published to Google Play, signing a new build with a different keystore makes Play REJECT the upload (signing-key mismatch) unless the user resets the upload key in the Play Console (App integrity → App signing). The keystore "do you already have one?" decision (keystore-method-select + keystore-explainer) now spells this out and steers already-published apps to their existing keystore; the explain-tool entry carries the same guidance.
…-app warning test)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
PR 2 of the MCP/TUI onboarding integration (stacked on #PR1-branch
wolny/mcp-tui-integration): the MCP now conducts the COMPLETE Builder onboarding for both platforms — every journey the TUI can do is completable through the three MCP tools — and__CAPGO_MCP_ONBOARDING__is flipped ON, shipping the tools in the release bundle (+141 KB, +4.8%).How it's built
The MCP is a thin driver over the SAME shared headless engines the TUI uses (ios/flow.ts, android/flow.ts, tail/flow.ts) — no duplicated flow logic:
Found by the live AI e2e (fixed here)
Tested
bun run testgreen on the flipped bundle; tsc clean; dev-gate test now POSITIVELY asserts the 3 tools ship while dev markers stay forbidden.Notes for review
c3e682band run locally via run-tests.sh.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests