feat(mcp-onboarding): Capgo Builder MCP onboarding — engine + Android/iOS flow#2394
Draft
WcaleNieWolny wants to merge 1 commit into
Draft
feat(mcp-onboarding): Capgo Builder MCP onboarding — engine + Android/iOS flow#2394WcaleNieWolny wants to merge 1 commit into
WcaleNieWolny wants to merge 1 commit into
Conversation
…/iOS flow Production code only. All tests (unit + the AI-judge e2e harness) and design docs live in the private Cap-go/cli-mcp-tests repo and are overlaid locally for development. Based on the feature's merge-base with main; reconciling with main's onboarding TUI work is a follow-up.
Contributor
📝 WalkthroughWalkthroughThis PR introduces a complete MCP onboarding system for Capgo Builder that refactors Android onboarding into a pure state machine, implements a headless orchestration engine, and registers three MCP tools (start, next-step, explain) to guide users through credential setup, app registration, and first-build execution. ChangesAndroid Onboarding Core & Refactoring
MCP Onboarding Engine & Orchestration
MCP Tool Registration & Infrastructure
Estimated code review effort🎯 5 (Critical) | ⏱️ ~110 minutes This PR introduces substantial new onboarding infrastructure spanning 1,365 lines of Android state machine logic, 1,468 lines of headless orchestration engine, and extensive MCP tool wiring. The changes are heterogeneous, covering state machines, effect dispatch, OAuth session management, GCP/Play provisioning, credential finalization, and terminal integration. While individual functions are well-structured and purpose-driven, the system requires understanding end-to-end flows across Android effect handling, iOS credential collection, preflight routing, build phase transitions, and MCP result rendering. The refactoring of the Android TUI to centralize effects introduces 30+ individual step handler migrations, each following a similar pattern but requiring verification of correct progression routing and state preservation. Possibly related PRs
Suggested labels
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. ✨ Finishing Touches📝 Generate docstrings
⚔️ Resolve merge conflicts
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| && progress._keystoreBase64 | ||
| && deps.writeKeystoreFile | ||
| ) { | ||
| keystoreFileWritten = true |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bffe1955e
ℹ️ 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 on lines
+1028
to
+1032
| if (input.gcpProjectId && input.gcpProjectId !== '__new__') { | ||
| updated = applyAndroidInput('gcp-projects-select', updated, { | ||
| step: 'gcp-projects-select', | ||
| gcpProject: { projectId: input.gcpProjectId, name: input.gcpProjectId }, | ||
| }) |
There was a problem hiding this comment.
When the user selects the advertised __new__ option from gcp-projects-select, this branch deliberately skips applying any progress update, so completedSteps.gcpProjectChosen remains unset and the next call just reloads gcp-projects-select again. That makes the “Create a new project” path impossible to advance to gcp-project-create-name; persist a created-by-onboarding choice or route to the name-collection step when gcpProjectId === '__new__.
Useful? React with 👍 / 👎.
Comment on lines
+1313
to
+1315
| const recordPath = deps.buildRecordPath(buildAppId, buildPlatform) | ||
| const command = `npx @capgo/cli@latest build request ${buildAppId} --platform ${buildPlatform} --output-upload --output-record "${recordPath}"` | ||
| if (deps.canLaunchTerminal()) { |
There was a problem hiding this comment.
Because defaultBuildRecordPath is deterministic for an app/platform, a prior successful /tmp/capgo-build-record-...json can still be present when this launches a new build. If the user checks before the new build overwrites it, or if the new build fails and never writes a success record, checkBuild will read the stale record and report the old build as complete; remove the record before launching or use a per-run path/job marker.
Useful? React with 👍 / 👎.
|
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/src/build/onboarding/android/ui/app.tsx (2)
1924-1935:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the new keystore password-method markers here.
This branch still skips the new shared-state fields: the manual path only changes local UI state, and the random path writes passwords without
keystorePasswordGenerated. After an interruption, resume loses the distinction this PR added, so manual users bounce back to the choice screen and generated-password runs no longer carry the “generated” marker for cross-driver recovery.Suggested fix
onChange={(value) => { if (value === 'random') { const pw = generateRandomPassword() setKeystoreStorePassword(pw) setKeystoreKeyPassword(pw) setRandomPasswordGenerated(true) addLog('✔ Store + key passwords generated') - persistAndStep((p) => ({ ...p, keystoreStorePassword: pw, keystoreKeyPassword: pw }), 'keystore-new-cn') + persistAndStep( + p => ({ + ...p, + keystoreStorePassword: pw, + keystoreKeyPassword: pw, + keystorePasswordGenerated: true, + keystorePasswordManual: false, + }), + 'keystore-new-cn', + ) } else { - setStep('keystore-new-store-password') + persistAndStep( + p => ({ + ...p, + keystorePasswordManual: true, + keystorePasswordGenerated: false, + }), + 'keystore-new-store-password', + ) } }}🤖 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 1924 - 1935, The branch that generates a random password updates UI state but does not persist the new keystorePasswordGenerated marker, and the manual branch never persists that flag either; update the onChange handler so the random branch calls persistAndStep with keystorePasswordGenerated: true alongside keystoreStorePassword/keystoreKeyPassword (in the persistAndStep call that currently writes pw) and update the else/manual branch (where setStep('keystore-new-store-password') is invoked) to persist keystorePasswordGenerated: false to the shared state so both paths record the new marker for cross-driver resume; refer to the onChange handler, setRandomPasswordGenerated, persistAndStep, setStep, setKeystoreStorePassword and setKeystoreKeyPassword.
1004-1054:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAbort the OAuth loopback session when this step is cancelled.
oauth-google.tsnow supports abortable flows, but this effect never creates or passes a signal. If the user exits, retries, or the component unmounts during Google sign-in, the local callback server keeps running until the five-minute timeout instead of being torn down with the step.🤖 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 1004 - 1054, Create an AbortController inside the async IIFE (before calling runOAuthFlow) and pass its signal into the abortable OAuth flow so the local loopback server can be torn down when the step is cancelled; specifically, in the function that starts Google sign-in (the async block guarded by oauthStartedRef and calling getCapgoConfig → runAndroidEffect → runOAuthFlow), instantiate controller = new AbortController(), pass controller.signal into the runOAuthFlow invocation (e.g., as part of the options object or callbacks parameter), and ensure you call controller.abort() whenever you early-return due to cancelled or in the effect cleanup/unmount path so the OAuth loopback is aborted promptly.
🤖 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/flow.ts`:
- Around line 492-497: When handling the 'android-package-select' branch, the
code updates/saves the package choice but drops the serviceAccountMethod field
from the progress object; update the progress update so it copies the incoming
serviceAccountMethod into the saved progress (e.g., include
serviceAccountMethod: serviceAccountMethod or progress.serviceAccountMethod =
serviceAccountMethod when constructing the new progress/state) so the resume
flow honors the caller's chosen 'generate'|'existing' path; apply the same
change to the other identical package-selection branch later in the file (the
second android-package-select handling).
- Around line 412-418: The exported function androidStepView is unused and
causing dead-code lint failures; either remove the export or wire it up to a
caller. To fix, locate the function androidStepView (which calls
getAndroidResumeStep and androidViewForStep) and either (a) remove the export
keyword to make it internal/private if there is no external consumer, or (b)
connect it from the appropriate consumer (e.g., the onboarding rendering flow)
so it is called with an AndroidOnboardingProgress and AndroidStepCtx. Ensure
imports for getAndroidResumeStep and androidViewForStep remain correct after the
change.
In `@cli/src/build/onboarding/mcp/app-id-validation.ts`:
- Around line 24-35: The regex SAFE_APP_ID currently allows
dot/underscore/hyphen as segment separators so strings like "com-example" or
"foo_bar" pass; update SAFE_APP_ID so it requires one or more dot-separated
segments (i.e. at least one literal '.' between domain parts) while still
allowing internal segment characters like underscores/hyphens; replace the
pattern assigned to SAFE_APP_ID (and keep isSafeAppIdForCommand unchanged) with
a regex that enforces segments separated by '.' such as using the form
/^[a-z0-9]+(?:[._-][a-z0-9]+)*(?:\.[a-z0-9]+(?:[._-][a-z0-9]+)*)+$/i so values
without a dot no longer validate.
In `@cli/src/build/onboarding/mcp/contract.ts`:
- Around line 81-117: The code currently prints secret-bearing fields (e.g.,
result.context.keystorePassword) and then appends JSON.stringify(result),
leaking any secrets; update the rendering to never print raw context or known
secret fields: remove the explicit keystorePassword line and replace
JSON.stringify(result, null, 2) with a sanitized copy (e.g., clone result to a
new object and delete or mask result.context and any secret keys like
keystorePassword before serializing) so only non-secret parts are shown; ensure
references to result.context, result.human, result.options, result.roadmap,
result.collect, and result.next remain unchanged except that the final dump uses
the sanitizedResult variable instead of result.
In `@cli/src/build/onboarding/mcp/engine.ts`:
- Around line 1028-1033: The handler currently ignores the sentinel value
"__new__" and never advances the flow; update the logic around
input.gcpProjectId so when its value === '__new__' you call applyAndroidInput to
advance to the GCP creation step (use the same applyAndroidInput call flow but
set step: 'gcp-project-create-name' and supply an empty or placeholder
gcpProject payload), otherwise keep the existing branch that sets step:
'gcp-projects-select' with the selected project; modify the code paths
referencing input.gcpProjectId and applyAndroidInput to ensure the user is
routed to 'gcp-project-create-name' when "__new__" is chosen.
- Around line 1359-1374: The current logic lets validateStepInput run when
loadAndroidProgress() returns null because getAndroidResumeStep(null) yields
'welcome'; to prevent clients from bypassing the Android gate, change the branch
in the if (gateAppId) block to check for a missing/null gateProgress (or
currentStep === 'welcome') before calling validateStepInput: if gateProgress is
null (or currentStep is 'welcome'), immediately gather facts and call
decideAndroid(deps) to compute and return the corrective response (same pattern
used later) instead of running validateStepInput; reference
getAndroidResumeStep, loadAndroidProgress, validateStepInput, gatherFacts,
decideAndroid and NEXT_STEP_TOOL so the code path enforces the "no skipping"
contract and does not accept keystoreMethod/serviceAccountMethod etc. when
progress is absent.
- Around line 1435-1467: explainOnboarding currently reconstructs state solely
from persisted facts (via gatherFacts and resolveCurrentState) so it cannot
explain ephemeral build-phase states like
build-ready/build-launched/build-waiting/build-failed; update explainOnboarding
to first check for any live/session onboarding state and use that before falling
back to resolveCurrentState. Specifically, modify explainOnboarding (and
EngineDeps if necessary) to read a session-level state (e.g.,
deps.session.getCurrentState or deps.getLiveBuildState) and set const state =
input?.state ?? sessionState ?? resolveCurrentState(facts) so explainForState
receives the real-time build-phase state when present. Ensure the new lookup
covers build-phase symbols (build-ready, build-launched, build-waiting,
build-failed) and keep existing behavior when no session state exists.
In `@cli/src/build/onboarding/mcp/oauth-session.ts`:
- Around line 70-79: Replace the .then/.catch chain on session.result with an
async fire-and-forget helper (e.g., an immediately-invoked async function) that
awaits session.result inside a try/catch; on success set entry.status = 'done'
and entry.tokens = tokens, and on error set entry.status = 'error' and
entry.error = err instanceof Error ? err : new Error(String(err)). Keep the
non-blocking behavior by not awaiting the IIFE from the caller so the settlement
task runs asynchronously.
In `@cli/src/build/onboarding/mcp/onboarding-tools.ts`:
- Around line 291-307: writeKeystoreFile currently hardcodes the target dir to
android/app so keystores are written to the wrong place for projects with custom
Capacitor Android directories; update writeKeystoreFile to resolve the Android
app directory via the existing Capacitor-aware helper (e.g. call
getPlatformDirFromCapacitorConfig or the same platform-dir resolution used by
detectPlatforms/Android-effect) instead of join(cwd, 'android', 'app'), then use
that resolved androidAppDir for mkdir, path resolution, and writing the file
(keep sanitizeKeystoreAlias, the path traversal check using resolve(...) and
sep, and the return value unchanged).
In `@cli/src/build/output-record.ts`:
- Around line 107-124: The function readBuildOutputRecord should only return
null when the record truly doesn't exist; change the catch and validation logic
in readBuildOutputRecord so that it returns null only for a missing file (e.g.,
ENOENT from readFile), but throws or rethrows errors for JSON.parse failures,
permission errors, or schema mismatches. Concretely: after await readFile(path,
'utf-8') keep JSON.parse and the schema checks (the typeof checks for
jobId/status/outputUrl), and if the schema check fails throw a descriptive
Error; in the catch block, detect and return null only when the error indicates
file-not-found, otherwise rethrow the error so callers (e.g.,
readBuildOutputRecord consumers) can surface failures instead of treating them
as “still waiting.”
---
Outside diff comments:
In `@cli/src/build/onboarding/android/ui/app.tsx`:
- Around line 1924-1935: The branch that generates a random password updates UI
state but does not persist the new keystorePasswordGenerated marker, and the
manual branch never persists that flag either; update the onChange handler so
the random branch calls persistAndStep with keystorePasswordGenerated: true
alongside keystoreStorePassword/keystoreKeyPassword (in the persistAndStep call
that currently writes pw) and update the else/manual branch (where
setStep('keystore-new-store-password') is invoked) to persist
keystorePasswordGenerated: false to the shared state so both paths record the
new marker for cross-driver resume; refer to the onChange handler,
setRandomPasswordGenerated, persistAndStep, setStep, setKeystoreStorePassword
and setKeystoreKeyPassword.
- Around line 1004-1054: Create an AbortController inside the async IIFE (before
calling runOAuthFlow) and pass its signal into the abortable OAuth flow so the
local loopback server can be torn down when the step is cancelled; specifically,
in the function that starts Google sign-in (the async block guarded by
oauthStartedRef and calling getCapgoConfig → runAndroidEffect → runOAuthFlow),
instantiate controller = new AbortController(), pass controller.signal into the
runOAuthFlow invocation (e.g., as part of the options object or callbacks
parameter), and ensure you call controller.abort() whenever you early-return due
to cancelled or in the effect cleanup/unmount path so the OAuth loopback is
aborted promptly.
🪄 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: e67945f6-27ef-4524-9690-93b45db31538
📒 Files selected for processing (19)
cli/.gitignorecli/src/build/onboarding/android/flow.tscli/src/build/onboarding/android/keystore.tscli/src/build/onboarding/android/oauth-google.tscli/src/build/onboarding/android/oauth-scopes.tscli/src/build/onboarding/android/progress.tscli/src/build/onboarding/android/types.tscli/src/build/onboarding/android/ui/app.tsxcli/src/build/onboarding/mcp/app-id-validation.tscli/src/build/onboarding/mcp/contract.tscli/src/build/onboarding/mcp/engine.tscli/src/build/onboarding/mcp/explanations.tscli/src/build/onboarding/mcp/oauth-session.tscli/src/build/onboarding/mcp/onboarding-tools.tscli/src/build/onboarding/mcp/step-input.tscli/src/build/onboarding/mcp/terminal-launch.tscli/src/build/output-record.tscli/src/mcp/server.tscli/src/schemas/onboarding.ts
Comment on lines
+412
to
+418
| export function androidStepView( | ||
| progress: AndroidOnboardingProgress | null, | ||
| ctx: AndroidStepCtx, | ||
| ): AndroidStepView { | ||
| const step = progress ? getAndroidResumeStep(progress) : 'keystore-method-select' | ||
| return androidViewForStep(step, progress, ctx) | ||
| } |
Contributor
There was a problem hiding this comment.
Remove or wire androidStepView before merge.
bun run lint:deadcode is already failing because androidStepView is exported but unused. Either consume it from a caller or make it internal so the PR stops breaking CI.
Suggested fix
-export function androidStepView(
+function androidStepView(
progress: AndroidOnboardingProgress | null,
ctx: AndroidStepCtx,
): AndroidStepView {📝 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
| export function androidStepView( | |
| progress: AndroidOnboardingProgress | null, | |
| ctx: AndroidStepCtx, | |
| ): AndroidStepView { | |
| const step = progress ? getAndroidResumeStep(progress) : 'keystore-method-select' | |
| return androidViewForStep(step, progress, ctx) | |
| } | |
| function androidStepView( | |
| progress: AndroidOnboardingProgress | null, | |
| ctx: AndroidStepCtx, | |
| ): AndroidStepView { | |
| const step = progress ? getAndroidResumeStep(progress) : 'keystore-method-select' | |
| return androidViewForStep(step, progress, ctx) | |
| } |
🤖 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/flow.ts` around lines 412 - 418, The
exported function androidStepView is unused and causing dead-code lint failures;
either remove the export or wire it up to a caller. To fix, locate the function
androidStepView (which calls getAndroidResumeStep and androidViewForStep) and
either (a) remove the export keyword to make it internal/private if there is no
external consumer, or (b) connect it from the appropriate consumer (e.g., the
onboarding rendering flow) so it is called with an AndroidOnboardingProgress and
AndroidStepCtx. Ensure imports for getAndroidResumeStep and androidViewForStep
remain correct after the change.
Comment on lines
+492
to
+497
| // Phase 4.5 — Android package select. | ||
| // serviceAccountMethod is required so the pure function can route correctly | ||
| // (progress.serviceAccountMethod may already be set but is also passed | ||
| // explicitly here to keep the function self-contained). | ||
| | { step: 'android-package-select'; packageName: string; source: 'gradle' | 'capacitor-config' | 'user-input'; serviceAccountMethod: 'generate' | 'existing' } | ||
|
|
Contributor
There was a problem hiding this comment.
Persist serviceAccountMethod when saving the package choice.
The input shape requires serviceAccountMethod, but this branch drops it. If progress reaches android-package-select with that field missing or stale, the next resume can fall back onto the OAuth route instead of the JSON-import route even though the caller supplied the correct method.
Suggested fix
case 'android-package-select': {
const i = input as Extract<AndroidInput, { step: 'android-package-select' }>
const choice = {
packageName: i.packageName,
source: i.source,
}
return {
...progress,
+ serviceAccountMethod: i.serviceAccountMethod,
completedSteps: { ...progress.completedSteps, androidPackageChosen: choice },
}
}Also applies to: 705-715
🤖 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/flow.ts` around lines 492 - 497, When
handling the 'android-package-select' branch, the code updates/saves the package
choice but drops the serviceAccountMethod field from the progress object; update
the progress update so it copies the incoming serviceAccountMethod into the
saved progress (e.g., include serviceAccountMethod: serviceAccountMethod or
progress.serviceAccountMethod = serviceAccountMethod when constructing the new
progress/state) so the resume flow honors the caller's chosen
'generate'|'existing' path; apply the same change to the other identical
package-selection branch later in the file (the second android-package-select
handling).
Comment on lines
+24
to
+35
| /** Strict reverse-domain package identifier pattern (command-injection safe). */ | ||
| const SAFE_APP_ID = /^[a-z0-9]+(?:[._-][a-z0-9]+)+$/i | ||
|
|
||
| /** | ||
| * Returns true only when `appId` is a safe reverse-domain package identifier | ||
| * that can be embedded in a shell command string without risk of injection. | ||
| * | ||
| * Valid examples: com.example.app io.capgo.app_1 com.acme.my-app | ||
| * Invalid examples: com.x; rm -rf ~ com.x$(cmd) nodots "" | ||
| */ | ||
| export function isSafeAppIdForCommand(appId: string): boolean { | ||
| return SAFE_APP_ID.test(appId) |
Contributor
There was a problem hiding this comment.
Require an actual dot-separated app id here.
SAFE_APP_ID currently accepts values like com-example or foo_bar, even though the comment and the caller messages say this validator enforces a reverse-domain id such as com.example.app. That lets malformed app ids through the build handoff check and shifts the failure later into the build flow.
🤖 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/app-id-validation.ts` around lines 24 - 35, The
regex SAFE_APP_ID currently allows dot/underscore/hyphen as segment separators
so strings like "com-example" or "foo_bar" pass; update SAFE_APP_ID so it
requires one or more dot-separated segments (i.e. at least one literal '.'
between domain parts) while still allowing internal segment characters like
underscores/hyphens; replace the pattern assigned to SAFE_APP_ID (and keep
isSafeAppIdForCommand unchanged) with a regex that enforces segments separated
by '.' such as using the form
/^[a-z0-9]+(?:[._-][a-z0-9]+)*(?:\.[a-z0-9]+(?:[._-][a-z0-9]+)*)+$/i so values
without a dot no longer validate.
Comment on lines
+81
to
+117
| if (result.context && typeof result.context.keystorePassword === 'string') { | ||
| lines.push(`Keystore password: ${result.context.keystorePassword} (save this with the keystore — you need both for every release)`) | ||
| } | ||
|
|
||
| if (result.roadmap?.length) { | ||
| lines.push('') | ||
| lines.push('PLAN (show the user):') | ||
| for (const item of result.roadmap) | ||
| lines.push(` • ${item}`) | ||
| } | ||
| if (result.human?.instruction) { | ||
| lines.push('') | ||
| lines.push(`ACTION FOR THE USER:\n${result.human.instruction}`) | ||
| if (result.human.resourceUri) | ||
| lines.push(`(Detailed guide: ${result.human.resourceUri})`) | ||
| } | ||
| if (result.options?.length) { | ||
| lines.push('') | ||
| lines.push('OPTIONS:') | ||
| for (const o of result.options) | ||
| lines.push(` - ${o.value}${o.label ? ` (${o.label})` : ''}${o.note ? ` — ${o.note}` : ''}`) | ||
| } | ||
| if (result.collect?.length) { | ||
| lines.push('') | ||
| lines.push('COLLECT FROM THE USER (never via chat if secret — use file paths / local login):') | ||
| for (const c of result.collect) | ||
| lines.push(` - ${c.field}: ${c.desc}`) | ||
| } | ||
| if (result.next) { | ||
| lines.push('') | ||
| lines.push(`DO THIS NEXT: ${result.next.instruction}`) | ||
| if (result.next.call) | ||
| lines.push(`Example call: ${result.next.call}`) | ||
| } | ||
| lines.push('') | ||
| lines.push('---') | ||
| lines.push(JSON.stringify(result, null, 2)) |
Contributor
There was a problem hiding this comment.
Redact secret-bearing context before rendering the MCP response.
This path prints result.context.keystorePassword and then serializes the full result, so any secret placed in context is leaked into the MCP transcript. That breaks the PR’s “local channels, not chat” boundary and makes future secret-bearing context fields easy to expose by accident.
🔒 Proposed fix
export function renderResult(result: NextStepResult): string {
+ const safeResult: NextStepResult = {
+ ...result,
+ context: result.context
+ ? {
+ ...result.context,
+ keystorePassword: result.context.keystorePassword ? '[redacted]' : result.context.keystorePassword,
+ }
+ : result.context,
+ }
+
const lines: string[] = []
- lines.push(`Capgo Builder onboarding — phase: ${result.phase} · step: ${result.state} · ${result.progress}%`)
+ lines.push(`Capgo Builder onboarding — phase: ${safeResult.phase} · step: ${safeResult.state} · ${safeResult.progress}%`)
lines.push('')
- lines.push(result.summary)
+ lines.push(safeResult.summary)
- if (result.context && typeof result.context.keystorePath === 'string') {
+ if (safeResult.context && typeof safeResult.context.keystorePath === 'string') {
lines.push('')
- lines.push(`Saved keystore: ${result.context.keystorePath} (keep this file — you need it for every release)`)
+ lines.push(`Saved keystore: ${safeResult.context.keystorePath} (keep this file — you need it for every release)`)
}
- if (result.context && typeof result.context.keystorePassword === 'string') {
- lines.push(`Keystore password: ${result.context.keystorePassword} (save this with the keystore — you need both for every release)`)
- }
-
…
lines.push('')
lines.push('---')
- lines.push(JSON.stringify(result, null, 2))
+ lines.push(JSON.stringify(safeResult, null, 2))
return lines.join('\n')
}🤖 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/contract.ts` around lines 81 - 117, The code
currently prints secret-bearing fields (e.g., result.context.keystorePassword)
and then appends JSON.stringify(result), leaking any secrets; update the
rendering to never print raw context or known secret fields: remove the explicit
keystorePassword line and replace JSON.stringify(result, null, 2) with a
sanitized copy (e.g., clone result to a new object and delete or mask
result.context and any secret keys like keystorePassword before serializing) so
only non-secret parts are shown; ensure references to result.context,
result.human, result.options, result.roadmap, result.collect, and result.next
remain unchanged except that the final dump uses the sanitizedResult variable
instead of result.
Comment on lines
+134
to
+141
| function activePlatform(facts: PreflightFacts): Platform | null { | ||
| const a = facts.androidProgress | ||
| if (a && (a.activePlatform === 'android' || Boolean(a.completedSteps?.keystoreReady) || Boolean(a.serviceAccountForkSeen))) | ||
| return 'android' | ||
| const i = facts.iosProgress | ||
| if (i && (Boolean(i.keyId) || Boolean(i.p8Path) || Boolean(i.completedSteps?.apiKeyVerified))) | ||
| return 'ios' | ||
| return null |
Contributor
There was a problem hiding this comment.
Don't let stale activePlatform pin every future session to Android.
Once Line 669 saves activePlatform: 'android', this helper will keep returning 'android' forever, even after Android onboarding is complete. On a project with both platforms, a later start_capgo_builder_onboarding can no longer reach iOS setup because decideStart() is always forced back into decideAndroid().
Comment on lines
+1359
to
+1374
| if (androidInputPresent) { | ||
| const gateAppId = await deps.getAppId() | ||
| if (gateAppId) { | ||
| const gateProgress = await deps.loadAndroidProgress(gateAppId) | ||
| const currentStep = getAndroidResumeStep(gateProgress) | ||
| const check = validateStepInput(currentStep, input as unknown as Record<string, unknown>) | ||
| if (!check.ok) { | ||
| const facts = await gatherFacts(deps) | ||
| const corrective = await decideAndroid(facts, deps) // re-returns the current step; no input applied | ||
| const allowed = check.allowedFields | ||
| const correction = allowed && allowed.length > 0 | ||
| ? `This step accepts only one of: ${allowed.map(f => `{ ${f}: ... }`).join(' or ')}. Call ${NEXT_STEP_TOOL} with EXACTLY ONE of those fields and no others${check.extras.length ? ` — remove: ${check.extras.join(', ')}` : ''}. Do not batch multiple answers.` | ||
| : `Call ${NEXT_STEP_TOOL} with only the field this step asks for.` | ||
| return { ...corrective, summary: `${correction}\n\n${corrective.summary}` } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The strict Android input gate is bypassed before Android onboarding actually starts.
If loadAndroidProgress() returns null, currentStep becomes 'welcome', which is ungoverned in validateStepInput(). A client can therefore send keystoreMethod, serviceAccountMethod, etc. while the real flow is still at platform-select or login, and the server will persist it anyway. That breaks the “no skipping” contract described for this PR.
🤖 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/engine.ts` around lines 1359 - 1374, The current
logic lets validateStepInput run when loadAndroidProgress() returns null because
getAndroidResumeStep(null) yields 'welcome'; to prevent clients from bypassing
the Android gate, change the branch in the if (gateAppId) block to check for a
missing/null gateProgress (or currentStep === 'welcome') before calling
validateStepInput: if gateProgress is null (or currentStep is 'welcome'),
immediately gather facts and call decideAndroid(deps) to compute and return the
corrective response (same pattern used later) instead of running
validateStepInput; reference getAndroidResumeStep, loadAndroidProgress,
validateStepInput, gatherFacts, decideAndroid and NEXT_STEP_TOOL so the code
path enforces the "no skipping" contract and does not accept
keystoreMethod/serviceAccountMethod etc. when progress is absent.
Comment on lines
+1435
to
+1467
| export function resolveCurrentState(facts: PreflightFacts): string { | ||
| if (!facts.capacitorProject || !facts.appId) | ||
| return 'no-capacitor-project' | ||
| if (!facts.authenticated) | ||
| return 'login-required' | ||
| if (!facts.appRegistered) | ||
| return 'registering-app' | ||
| const appId = facts.appId | ||
| // Seed an empty android progress the way decideAndroid does so the resume step | ||
| // resolves to 'keystore-method-select' (not 'welcome') for a fresh android flow. | ||
| const androidProgress = facts.androidProgress | ||
| ?? { platform: 'android' as const, appId, startedAt: new Date().toISOString(), completedSteps: {} } | ||
| // An in-flight android credential flow (or a fresh android-only project) → resume step. | ||
| if (facts.androidProgress) | ||
| return getAndroidResumeStep(facts.androidProgress) | ||
| if (facts.iosProgress) | ||
| return 'ios-api-key' | ||
| if (facts.platformsDetected.length === 1 && facts.platformsDetected[0] === 'android') | ||
| return getAndroidResumeStep(androidProgress) | ||
| if (facts.platformsDetected.length === 1 && facts.platformsDetected[0] === 'ios') | ||
| return 'ios-api-key' | ||
| return 'platform-select' | ||
| } | ||
|
|
||
| /** | ||
| * Read-only "explain the current step" entry point backing the | ||
| * capgo_builder_onboarding_explain tool. Gathers facts (read-only) and returns a | ||
| * plain-language explanation string. Never advances the flow or runs effects. | ||
| */ | ||
| export async function explainOnboarding(deps: EngineDeps, input?: { state?: string }): Promise<string> { | ||
| const facts = await gatherFacts(deps) | ||
| const state = input?.state ?? resolveCurrentState(facts) | ||
| return explainForState(state) |
Contributor
There was a problem hiding this comment.
explainOnboarding() can't explain build-phase results.
This resolver reconstructs state only from persisted facts/progress, but steps like build-ready, build-launched, build-waiting, and build-failed are session results, not persisted onboarding states. Because the registered explain tool takes no state argument, asking for an explanation during those steps will resolve to an older saved state instead of the step the user is actually looking at.
🤖 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/engine.ts` around lines 1435 - 1467,
explainOnboarding currently reconstructs state solely from persisted facts (via
gatherFacts and resolveCurrentState) so it cannot explain ephemeral build-phase
states like build-ready/build-launched/build-waiting/build-failed; update
explainOnboarding to first check for any live/session onboarding state and use
that before falling back to resolveCurrentState. Specifically, modify
explainOnboarding (and EngineDeps if necessary) to read a session-level state
(e.g., deps.session.getCurrentState or deps.getLiveBuildState) and set const
state = input?.state ?? sessionState ?? resolveCurrentState(facts) so
explainForState receives the real-time build-phase state when present. Ensure
the new lookup covers build-phase symbols (build-ready, build-launched,
build-waiting, build-failed) and keep existing behavior when no session state
exists.
Comment on lines
+70
to
+79
| // Advance the status when the result settles (this does NOT block the caller). | ||
| session.result | ||
| .then((tokens: GoogleOAuthTokens) => { | ||
| entry.status = 'done' | ||
| entry.tokens = tokens | ||
| }) | ||
| .catch((err: unknown) => { | ||
| entry.status = 'error' | ||
| entry.error = err instanceof Error ? err : new Error(String(err)) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use async/await for the non-blocking settlement task.
The fire-and-forget status updater is using a .then().catch() chain. Please wrap it in an async helper/IIFE with try/catch instead so this file stays consistent with the repo's TS conventions.
As per coding guidelines Always use async/await instead of promises with .then() chains.
🤖 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/oauth-session.ts` around lines 70 - 79, Replace
the .then/.catch chain on session.result with an async fire-and-forget helper
(e.g., an immediately-invoked async function) that awaits session.result inside
a try/catch; on success set entry.status = 'done' and entry.tokens = tokens, and
on error set entry.status = 'error' and entry.error = err instanceof Error ? err
: new Error(String(err)). Keep the non-blocking behavior by not awaiting the
IIFE from the caller so the settlement task runs asynchronously.
Comment on lines
+291
to
+307
| writeKeystoreFile: async (appId: string, base64: string, alias: string): Promise<string> => { | ||
| // Write the generated/loaded keystore to android/app/<alias>.p12, mirroring | ||
| // the Ink wizard which uses the same path (keystore-generating hardcodes it). | ||
| // `alias` comes from user input — sanitize it for the ON-DISK filename only | ||
| // (the crypto alias / KEYSTORE_KEY_ALIAS keep the user's exact value). | ||
| const androidAppDir = join(cwd, 'android', 'app') | ||
| await mkdir(androidAppDir, { recursive: true }) | ||
| const safe = sanitizeKeystoreAlias(alias) | ||
| const filePath = join(androidAppDir, `${safe}.p12`) | ||
| // Defense-in-depth: the resolved path must stay inside androidAppDir. | ||
| const resolvedDir = resolve(androidAppDir) | ||
| const resolvedFile = resolve(filePath) | ||
| if (resolvedFile !== resolvedDir && !resolvedFile.startsWith(resolvedDir + sep)) | ||
| throw new Error('Refusing to write keystore outside the android/app directory.') | ||
| const bytes = Buffer.from(base64, 'base64') | ||
| await writeFile(filePath, bytes) | ||
| return filePath |
Contributor
There was a problem hiding this comment.
Respect custom Capacitor Android directories when writing the keystore.
detectPlatforms() and the Android-effect wiring already consult getPlatformDirFromCapacitorConfig(...), but writeKeystoreFile() hardcodes android/app. For projects with a custom android dir, onboarding will save the keystore into the wrong location and the subsequent build flow won’t find the artifact it just created.
📦 Proposed fix
writeKeystoreFile: async (appId: string, base64: string, alias: string): Promise<string> => {
- const androidAppDir = join(cwd, 'android', 'app')
+ let androidDir = 'android'
+ try {
+ const ext = await getConfig(true)
+ androidDir = getPlatformDirFromCapacitorConfig(ext?.config, 'android') || 'android'
+ }
+ catch {
+ // fall back to the default Capacitor layout
+ }
+ const androidAppDir = join(cwd, androidDir, 'app')
await mkdir(androidAppDir, { recursive: true })
const safe = sanitizeKeystoreAlias(alias)
const filePath = join(androidAppDir, `${safe}.p12`)📝 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
| writeKeystoreFile: async (appId: string, base64: string, alias: string): Promise<string> => { | |
| // Write the generated/loaded keystore to android/app/<alias>.p12, mirroring | |
| // the Ink wizard which uses the same path (keystore-generating hardcodes it). | |
| // `alias` comes from user input — sanitize it for the ON-DISK filename only | |
| // (the crypto alias / KEYSTORE_KEY_ALIAS keep the user's exact value). | |
| const androidAppDir = join(cwd, 'android', 'app') | |
| await mkdir(androidAppDir, { recursive: true }) | |
| const safe = sanitizeKeystoreAlias(alias) | |
| const filePath = join(androidAppDir, `${safe}.p12`) | |
| // Defense-in-depth: the resolved path must stay inside androidAppDir. | |
| const resolvedDir = resolve(androidAppDir) | |
| const resolvedFile = resolve(filePath) | |
| if (resolvedFile !== resolvedDir && !resolvedFile.startsWith(resolvedDir + sep)) | |
| throw new Error('Refusing to write keystore outside the android/app directory.') | |
| const bytes = Buffer.from(base64, 'base64') | |
| await writeFile(filePath, bytes) | |
| return filePath | |
| writeKeystoreFile: async (appId: string, base64: string, alias: string): Promise<string> => { | |
| // Write the generated/loaded keystore to android/app/<alias>.p12, mirroring | |
| // the Ink wizard which uses the same path (keystore-generating hardcodes it). | |
| // `alias` comes from user input — sanitize it for the ON-DISK filename only | |
| // (the crypto alias / KEYSTORE_KEY_ALIAS keep the user's exact value). | |
| let androidDir = 'android' | |
| try { | |
| const ext = await getConfig(true) | |
| androidDir = getPlatformDirFromCapacitorConfig(ext?.config, 'android') || 'android' | |
| } | |
| catch { | |
| // fall back to the default Capacitor layout | |
| } | |
| const androidAppDir = join(cwd, androidDir, 'app') | |
| await mkdir(androidAppDir, { recursive: true }) | |
| const safe = sanitizeKeystoreAlias(alias) | |
| const filePath = join(androidAppDir, `${safe}.p12`) | |
| // Defense-in-depth: the resolved path must stay inside androidAppDir. | |
| const resolvedDir = resolve(androidAppDir) | |
| const resolvedFile = resolve(filePath) | |
| if (resolvedFile !== resolvedDir && !resolvedFile.startsWith(resolvedDir + sep)) | |
| throw new Error('Refusing to write keystore outside the android/app directory.') | |
| const bytes = Buffer.from(base64, 'base64') | |
| await writeFile(filePath, bytes) | |
| return filePath |
🤖 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/onboarding-tools.ts` around lines 291 - 307,
writeKeystoreFile currently hardcodes the target dir to android/app so keystores
are written to the wrong place for projects with custom Capacitor Android
directories; update writeKeystoreFile to resolve the Android app directory via
the existing Capacitor-aware helper (e.g. call getPlatformDirFromCapacitorConfig
or the same platform-dir resolution used by detectPlatforms/Android-effect)
instead of join(cwd, 'android', 'app'), then use that resolved androidAppDir for
mkdir, path resolution, and writing the file (keep sanitizeKeystoreAlias, the
path traversal check using resolve(...) and sep, and the return value
unchanged).
Comment on lines
+107
to
+124
| export async function readBuildOutputRecord(path: string): Promise<BuildOutputRecord | null> { | ||
| try { | ||
| const raw = await readFile(path, 'utf-8') | ||
| const parsed: unknown = JSON.parse(raw) | ||
| if ( | ||
| typeof parsed === 'object' | ||
| && parsed !== null | ||
| && typeof (parsed as Record<string, unknown>).jobId === 'string' | ||
| && typeof (parsed as Record<string, unknown>).status === 'string' | ||
| && ((parsed as Record<string, unknown>).outputUrl === null || typeof (parsed as Record<string, unknown>).outputUrl === 'string') | ||
| ) { | ||
| return parsed as BuildOutputRecord | ||
| } | ||
| return null | ||
| } | ||
| catch { | ||
| return null | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t treat corrupt or unreadable build records as “no record yet.”
readBuildOutputRecord() returns null for every failure mode, but cli/src/build/onboarding/mcp/engine.ts:1277-1299 interprets null as “still waiting for the build”. A truncated JSON file, permission error, or schema mismatch will therefore look like a pending build forever instead of a surfaced failure.
🛠️ Proposed fix
export async function readBuildOutputRecord(path: string): Promise<BuildOutputRecord | null> {
try {
const raw = await readFile(path, 'utf-8')
const parsed: unknown = JSON.parse(raw)
if (
typeof parsed === 'object'
&& parsed !== null
&& typeof (parsed as Record<string, unknown>).jobId === 'string'
&& typeof (parsed as Record<string, unknown>).status === 'string'
&& ((parsed as Record<string, unknown>).outputUrl === null || typeof (parsed as Record<string, unknown>).outputUrl === 'string')
) {
return parsed as BuildOutputRecord
}
- return null
+ throw new Error('Invalid build output record shape')
}
- catch {
- return null
+ catch (error) {
+ if ((error as NodeJS.ErrnoException)?.code === 'ENOENT')
+ return null
+ throw error
}
}🤖 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/output-record.ts` around lines 107 - 124, The function
readBuildOutputRecord should only return null when the record truly doesn't
exist; change the catch and validation logic in readBuildOutputRecord so that it
returns null only for a missing file (e.g., ENOENT from readFile), but throws or
rethrows errors for JSON.parse failures, permission errors, or schema
mismatches. Concretely: after await readFile(path, 'utf-8') keep JSON.parse and
the schema checks (the typeof checks for jobId/status/outputUrl), and if the
schema check fails throw a descriptive Error; in the catch block, detect and
return null only when the error indicates file-not-found, otherwise rethrow the
error so callers (e.g., readBuildOutputRecord consumers) can surface failures
instead of treating them as “still waiting.”
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
1 participant
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.
Summary
Production code for the MCP-conducted Capgo Builder onboarding — an AI agent drives the full native cloud-build setup (Android keystore, Google Play service account, Apple credentials) through a 2-tool MCP spine backed by a server-owned, resumable state machine.
Highlights:
start_capgo_builder_onboarding(orient/resume) andcapgo_builder_onboarding_next_step(advance). Every result carries akind(auto/human_gate/choice/done/error/info) plus an explicitnext. Heavy work (Apple/Google APIs, keystore, build) runs inside the server; secrets travel via local channels, never the chat.step-input.ts) — each step accepts exactly its expected field; mega-calls / wrong-field calls are rejected with a correction (no auto-advance, no skipping).keystore-new-store-passwordstep (distinct state) instead of silently staying on the choice screen, which previously confused the AI into re-driving the flow.explainread-only tool — surfaces a plain-language explanation of the current state when the user is confused, advertised on every step.Scope / file boundary
This branch contains production
cli/srconly (18 files + a.gitignoreguard). The test suite — unit tests and the AI-judge e2e harness — plus the design docs live in the privateCap-go/cli-mcp-testsrepo and are overlaid onto a checkout for local/CI runs (see that repo's README). The.gitignoreguard keeps an overlaid harness from being committed here.This branch is based on the feature's merge-base with
main, not the latestmain. Since it forked,mainmerged a separate onboarding TUI (#2376) that overlapscli/src/build/onboarding/. This PR will show merge conflicts and must be reconciled with that work before merge — a known, intentionally-deferred follow-up.Test plan
Tests run from the private
Cap-go/cli-mcp-testsrepo, overlaid onto this checkout:bun test/test-mcp-onboarding.mjs— engine unit suite (87 passing locally)cd test/e2e-mcp && bun verify-tree.mjs— hermetic e2e gate, no LLM (PASS locally)bun run typecheck— passes at the feature basemain's onboarding TUI (feat(cli): build init onboarding TUI — wizard, AI build-debug, adaptive sizing, robust log viewer #2376); resolve conflicts before mergeSummary by CodeRabbit
Release Notes
New Features
Refactor