refactor: command-mode architecture, Firebase googleapis migration, and task-based install flow#22
Merged
Merged
Conversation
- Add IAM API client for Service Account operations (list, create, download key) - Add GCP project API for listing available projects and adding Firebase - Extend OAuth scopes to include iam and cloud-platform permissions - Create Zod-based Service Account JSON validator with real-time feedback - Extend FirebaseDownloader with SA management methods - Add new wizard phases: no_projects, service_account_menu, paste_json - Support JSON paste with real-time validation and error display - Save service account keys to .clix/service-account.json
Replace direct Firebase Management and IAM REST API implementations with @googleapis/firebase and @googleapis/iam packages. This reduces code by 57% while maintaining full backward compatibility through existing public APIs. Key improvements: - Reduced firebase-api.ts from 366 to 223 lines (-39%) - Reduced iam-api.ts from 219 to 126 lines (-42%) - Removed duplicate pagination and token handling logic - Type safety improved with generated types from googleapis - Automatic token refresh via OAuth2Client - Cleaner error handling with googleapis's built-in support All 588 unit tests and 20 E2E tests pass.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR comprehensively transforms the Clix CLI from an interactive chat-driven architecture to a command-first model. It removes chat, debug, firebase, iOS setup, and resume commands while introducing new command-based skills (install, doctor) and external tool integrations (mcp, skills). Firebase integration now uses the Google APIs library with enhanced OAuth and service account handling. A preparation context system tracks project readiness. Embedded skill metadata is replaced with local command prompts. The CLI dispatches directly to agent processes instead of rendering interactive UI. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 1c184418fc
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 52-54: Update the invalid dependency versions in package.json:
change "@googleapis/cloudresourcemanager" from ^6.0.1 to ^3.0.1,
"@googleapis/firebase" from ^12.0.1 to ^10.0.1, and "@googleapis/iam" from
^36.0.0 to ^30.1.0 so they match published npm releases; leave
"google-auth-library" at ^10.5.0 as-is, then run npm install (or yarn) and a
quick build/test to verify no further dependency conflicts.
In `@src/commands/skill/preparation.ts`:
- Around line 159-169: The readiness check incorrectly treats projectType.target
=== 'unknown' as not needing Firebase/iOS setup; update the logic in the
preparation routine so that when projectType.target === 'unknown' it is treated
as needing setup (i.e., needed = true or explicitly block readiness) instead of
returning configured true. Modify the branch around the needed variable and the
blocks that set configured, androidConfigured, and iosConfigured (and the
similar checks referenced later) so unknown target forces configured:false (or
prevents ready:true) and flows through normal setup checks for Firebase and iOS.
In `@src/lib/services/firebase/downloader.ts`:
- Around line 417-427: The saveServiceAccountJson function currently writes the
service-account.json with default permissions; change it to enforce owner-only
read/write permissions like credentials.json does by creating the .clix dir as
before, writing the file, then calling fs.chmod(savePath, 0o600) (and
handle/await any errors) so the file is restricted; reference
saveServiceAccountJson and mimic the chmod pattern used in
src/lib/auth/credentials.ts for credentials.json.
In `@src/lib/services/firebase/oauth/config.ts`:
- Around line 157-168: Update the OAuth scopes array in the scopes property so
it uses least-privilege Cloud Resource Manager scope(s) instead of the broad
cloud-platform scope: replace 'https://www.googleapis.com/auth/cloud-platform'
in the scopes array with the more specific
'https://www.googleapis.com/auth/cloudplatformprojects' (or
'https://www.googleapis.com/auth/cloudplatformprojects.readonly' if only
listing/reading projects) while keeping the existing firebase and iam scopes;
locate this change in the scopes definition in config.ts.
In `@src/ui/components/FirebaseWizard.tsx`:
- Around line 208-215: The menu item for "setup-service-account" is incorrectly
gated only on Android; change the condition in the FirebaseWizard component so
the item is shown when OAuth is configured AND a project ID or valid platform
config exists (use getProjectIdFromResult(result) or check result.ios?.valid)
instead of relying solely on result.android?.valid; update the conditional that
calls items.push({ id: 'setup-service-account', ... }) to use
isOAuthConfigured() && (getProjectIdFromResult(result) != null ||
result.ios?.valid || result.android?.valid) so iOS-only projects with a project
ID will see the menu entry.
In `@src/ui/components/InstallPreparationUI.tsx`:
- Around line 418-423: The render branch for phase 'ios_setup' calls
handleIosSkip() directly which triggers setPhase during render; instead remove
that direct call and create a useEffect that watches the phase (or a derived
boolean), context, and handleIosSkip, and invokes handleIosSkip() inside the
effect when phase === 'ios_setup' and context is present; keep the component
returning <CheckingPhase /> from the render path and ensure the effect's
dependency array includes handleIosSkip and context to avoid stale closures.
Remove backward compatibility code for V1→V2 migration: - Unify ProjectConfigV1Schema and ProjectConfigV2Schema into single ProjectConfigSchema - Change version field from z.literal(2) to z.number() for flexibility - Remove isConfigV1(), isConfigV2(), migrateV1ToV2() functions - Keep ensureLatestVersion() framework for future migrations - Remove loadV2() method, use load() directly - Update CURRENT_PROJECT_CONFIG_VERSION to 1 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/config/project-config-manager.ts`:
- Around line 167-192: The updateSetup method currently merges a
Partial<SetupStatus> and saves without validating, which can produce an invalid
ProjectConfig; modify updateSetup to merge updates into config.setup and then
validate or normalize the resulting config before calling save: use an existing
config validation/normalization utility (or add one) to fill missing default
fields for SetupStatus (e.g., getDefaultSetup or schema validate) and run it
against the merged object, and if validation fails throw a ConfigError; ensure
you reference updateSetup, load, save, SetupStatus and ProjectConfig when
locating and changing the code.
🧹 Nitpick comments (1)
src/lib/config/project-config-schema.ts (1)
123-123: Constrain config version to a non‑negative integer.
Currentlyz.number()allows decimals (e.g., 1.5), which complicates migrations and version comparisons. Versions should be integers.♻️ Proposed fix
- version: z.number(), + version: z.number().int().nonnegative(),
- Migrate Firebase API clients from REST to googleapis library - Remove IAM API (SA keys now downloaded from Firebase Console) - Add Sender Config API integration (check/register push notification config) - Extract FirebaseWizard PHASE_TRANSITIONS state machine with transition() validation - Add Organization/Projects concurrent fetching service - Add ProjectSelector search/filter functionality - Improve InstallPreparationUI with explicit action menus - Add OAuth debug logging infrastructure - Simplify OAuth scopes to firebase-only - Add exponential backoff retry logic for API calls
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/lib/debug/logger.ts`:
- Around line 201-226: The writeToFile method currently calls JSON.stringify on
optional data in both the file-write branch (building variable line) and the
stderr fallback, which can throw on circular or unserializable values; change
writeToFile (and any helper uses of this.namespace within it) to first attempt
JSON.stringify(data) inside a try/catch (or use a small safeStringify helper)
and on failure replace with a safe placeholder (e.g., "[unserializable data]" or
inspect fallback) so that neither the appendFileSync branch nor the
console.error fallback can throw — update the construction of line and the
console.error call to use the safe stringified value instead of calling
JSON.stringify directly.
In `@src/lib/services/firebase/api/firebase-api.ts`:
- Around line 91-97: The code currently type-asserts p.state to 'ACTIVE' |
'DELETED' which can let unexpected strings through; instead normalize p.state
explicitly before pushing into projects (e.g., compute const normalizedState =
p.state === 'DELETED' ? 'DELETED' : 'ACTIVE' and use that). Update the push that
builds the FirebaseProject (the projects.push block using p.state) to use this
normalized value, and apply the same change to the other occurrence (the similar
block around lines 273-277) so the FirebaseProject union is always respected.
In `@src/lib/services/firebase/oauth/auth-client.ts`:
- Around line 336-345: The current refresh error handler in the auth client
unconditionally rewraps any refreshAccessToken failure as "invalid_grant",
causing unnecessary re-auth flows; update the catch in the token refresh block
(where tokenStore.isExpired(tokens) is checked and refreshAccessToken is called)
to inspect the caught error (e.g., error.message or error.code) and only throw a
new Error prefixed with "invalid_grant: " when the underlying error actually
indicates "invalid_grant"; for other errors rethrow the original error (or throw
a generic refresh error without the invalid_grant prefix) so transient
network/IO errors are not misclassified.
In `@src/lib/services/organization-projects.ts`:
- Around line 22-27: The normalizeConcurrency function currently floors
fractional inputs which can produce 0 for values like 0.5; update
normalizeConcurrency to first validate the input, then Math.floor the value and
clamp the result to a minimum of 1 (and fall back to
DEFAULT_PROJECT_FETCH_CONCURRENCY when input is missing/invalid) so the returned
concurrency is always at least 1; reference the normalizeConcurrency function
and DEFAULT_PROJECT_FETCH_CONCURRENCY constant when making the change.
In `@src/lib/utils/oauth.ts`:
- Around line 261-283: The debug write currently persists full_url and
all_params which can leak secrets; before calling oauthLogger.writeToFile (from
within the OAuth error branch), sanitize the data by redacting sensitive
parameter values (e.g., code, state, access_token, refresh_token, client_secret,
refresh_token, id_token) and replace them with a presence flag or "[REDACTED]"
and reconstruct a sanitized full_url without secret values; then pass
sanitized_all_params and sanitized_full_url to oauthLogger.writeToFile (keep use
of findProjectRoot() and existing metadata) so only non-sensitive info or
presence indicators are persisted.
In `@src/ui/components/FirebaseWizard.tsx`:
- Around line 2087-2091: The setTimeout in the FirebaseWizard component
schedules setPhase(...) and onComplete(...) after 1500ms but doesn't clear the
timer, which can lead to state updates after unmount; capture the timeout id
when calling setTimeout (e.g., store it in a ref or local variable tied to the
component), and ensure you call clearTimeout(timeoutId) in a useEffect cleanup
or when the component is unmounted/cancelled so that the scheduled setPhase and
onComplete are not invoked on an unmounted component.
In `@src/ui/components/InstallPreparationUI.tsx`:
- Around line 62-67: getPostFirebasePhase currently only checks
ios.entitlementsConfigured and skips ios_setup if entitlements are present,
which ignores missing NSE; update getPostFirebasePhase (and the related
handleContinue logic) to require both ios.entitlementsConfigured and
ios.nseConfigured to be true before returning 'ready' when ios.needed is true so
that missing NSE keeps the flow in 'ios_setup'. Locate the PreparationContext
usage in getPostFirebasePhase and the function/method named handleContinue and
change their conditional from checking ios.entitlementsConfigured to checking
(ios.entitlementsConfigured && ios.nseConfigured), preserving the ios.needed
guard and returning 'ios_setup' otherwise.
🧹 Nitpick comments (5)
src/ui/components/ProjectSelector.tsx (2)
6-9: Avoid duplicatingOrgWithProjects.This type already exists in
src/lib/services/organization-projects.ts; redefining it here risks drift. Consider importing and re-exporting the shared type.♻️ Suggested refactor
-import type { Organization, Project } from '@/lib/api'; +import type { Organization, Project } from '@/lib/api'; +import type { OrgWithProjects } from '@/lib/services/organization-projects'; @@ -export interface OrgWithProjects { - org: Organization; - projects: Project[]; -} +export type { OrgWithProjects };
84-159: Align selector input handling withGenericSelector+useCancelInput.This is a selector component but handles ESC/Ctrl+C manually; the UI guidelines prefer the shared selector abstraction and cancellation hook.
As per coding guidelines, “Import
useCancelInputfrom@/ui/hooksfor ESC/Ctrl+C cancellation in new components; for selector-based components, extendGenericSelector.”src/lib/services/firebase/api/firebase-api.ts (1)
49-68: Consider replacing the class with a plain-object factory.The codebase guidelines prefer plain objects with interfaces over classes for TS modules.
As per coding guidelines, “Prefer plain objects with interfaces over classes; use ES module exports for encapsulation in TypeScript code.”
src/lib/api/internal-client.ts (1)
71-73: Token resolved once before retry loop may become stale during long retry sequences.The access token is resolved once at line 71, but if
maxRetriesis high and the server returns 5xx repeatedly, the token could expire during the retry attempts. Consider refreshing the token before each retry attempt, or at least after a 401 response within the loop.However, since the default
maxRetriesis 0 and the total retry window is short (150ms + 300ms + ... backoff), this is unlikely to be a practical issue for most use cases.src/ui/components/FirebaseWizard.tsx (1)
799-823: Clipboard reading lacks Windows support.The
readClipboardfunction supports macOS (pbpaste) and Linux (xclip/xsel) but returnsnullon Windows. If Windows users are expected, consider addingpowershell -command "Get-Clipboard"as a fallback.🪟 Optional: Add Windows clipboard support
if (platform === 'linux') { try { const { stdout } = await execFileAsync('xclip', ['-selection', 'clipboard', '-o']); return stdout; } catch { const { stdout } = await execFileAsync('xsel', ['--clipboard', '--output']); return stdout; } } + if (platform === 'win32') { + const { stdout } = await execFileAsync('powershell', ['-command', 'Get-Clipboard']); + return stdout; + } return null;
…ditional workflows Add Agent Behavior, Conditional Workflows, and Common Mistakes sections inspired by OpenAI Codex and OpenCode AGENTS.md best practices. Expand TypeScript Patterns with BAD/GOOD code examples and Testing with mock patterns. Condense FirebaseWizard, Safe Rendering, OAuth, and UI Architecture sections. Merge Code Quality and Commits into single section.
Flatten install preparation into sequential leaf tasks for Firebase, APNS, iOS entitlements, and Notification Service Extension. Remove obsolete wizard/state-machine files and deprecated push setup aliases, and add dedicated task modules plus regression tests. Update related docs (README, llms, AGENTS) and local ignore entries for the new workflow.
…d refine related UI and Firebase integration.
Prevent command-mode install preparation from treating runtime tasks (project_build/install_skill) as completed. Hide project-build from user-facing skill lists while keeping internal resolution available for install flow orchestration. Remove GCP project add flow and cloudresourcemanager dependency so Firebase OAuth uses only Firebase API scope.
Drop project_build from install preparation tasks and UI runtime flow so the checklist moves directly from setup tasks to SDK installation. Also validate registered Firebase sender config service account project IDs against local Firebase config files and surface mismatch/decode errors as incomplete service-account setup. Update install skill wiring, embedded skill content, and related tests to reflect unified install behavior and new validation rules.
Drop chat UI, slash command registry, and session/context persistence layers to make clix run in command mode only. Simplify executor interfaces to standalone one-shot execution by removing history, compaction, and session resume paths. Update agent selection, docs, and tests to align with the new command-only workflow.
Remove Vercel Skills checks and guidance from install/doctor command execution paths. Both commands now proceed directly to agent handoff after agent selection. Also remove the vercel-skills service, drop the open dependency, and update docs to match the new behavior.
…skill installation workflow, and remove explicit installation phases.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/push/types.ts (1)
20-56:⚠️ Potential issue | 🟠 MajorPreserve backward‑compatibility for renamed push setup types. Removing
PushSetupPhase/PushSetupResultcan break TypeScript consumers; consider aliasing the old names to the new ones.♻️ Suggested compatibility aliases
export type PushSetupTaskPhase = | 'detecting' // Analyzing project | 'status' // Showing current status | 'key_source' // Asking if user has existing key | 'apple_login' // Apple account login for auto key creation | 'apple_guide' // Apple Portal guide (manual) | 'p8_input' // P8 + Key ID + Team ID input | 'validation' // Validating inputs | 'firebase_auth' // Authenticating with Firebase | 'firebase_projects' // Selecting Firebase project | 'firebase_upload' // Firebase upload guide | 'complete' // Setup complete | 'error'; // Error state + +/** `@deprecated` Use PushSetupTaskPhase */ +export type PushSetupPhase = PushSetupTaskPhase; @@ export interface PushSetupTaskResult { success: boolean; message: string; context?: PushSetupContext; } + +/** `@deprecated` Use PushSetupTaskResult */ +export type PushSetupResult = PushSetupTaskResult;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/push/types.ts` around lines 20 - 56, The public types PushSetupContext, PushSetupTaskPhase, and PushSetupTaskResult were renamed and removing the old aliases (PushSetupPhase, PushSetupResult) can break downstream TypeScript consumers; restore backward compatibility by exporting type aliases mapping the old names to the new ones (e.g., alias PushSetupPhase -> PushSetupTaskPhase and PushSetupResult -> PushSetupTaskResult) alongside the existing definitions so both the new and old type names resolve to the same interfaces/union types.src/lib/utils/oauth.ts (1)
343-386:⚠️ Potential issue | 🟠 MajorAvoid leaving
waitForCallback()unresolved whenstop()is called.stop()currently clears the pending callback without rejecting, which can hang callers if they usestop()to abort. Consider rejecting pending waits.🔧 Suggested fix
stop(): void { - this.clearPendingCallback(); + if (this.pendingCallback) { + this.rejectPendingCallback(new Error('OAuth callback server stopped')); + } if (this.server) { this.server.close(); this.server = null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/oauth.ts` around lines 343 - 386, The stop() method currently calls clearPendingCallback() which leaves any Promise returned by waitForCallback() unresolved; update stop() to reject any pending wait by calling rejectPendingCallback(new Error('Server stopped')) (or use a dedicated Abort/Error type) before nulling the server so callers don't hang; change the stop() implementation to invoke rejectPendingCallback(...) instead of clearPendingCallback(), keeping clearPendingCallback(), rejectPendingCallback(), resolvePendingCallback(), getPort() unchanged.src/cli.tsx (1)
21-60:⚠️ Potential issue | 🟡 MinorDocument the
--startTaskflag in help output.The flag is exposed but isn’t shown in the help text, so users won’t discover it.
🛠️ Suggested help text addition
Options --help Show this help message --version Show version number --platform <val> Target platform (ios, android, react-native, flutter) + --startTask <val> Begin install at a specific task (advanced)Also applies to: 71-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.tsx` around lines 21 - 60, The generateHelpText function's returned help string is missing documentation for the --startTask flag; update generateHelpText to include a line under "Options" documenting --startTask (e.g., "--startTask <id> Start a specific task on startup") and add an example invocation (such as "$ clix --startTask <id>" or "$ clix install --startTask <id>") in the "Examples" section so the flag is discoverable; modify the string returned by generateHelpText accordingly.
🧹 Nitpick comments (8)
src/commands/skills.ts (1)
16-19: Add-yto thenpxinvocation to avoid install prompts in non-interactive runs.
npxcan prompt to installskillswhen not cached, which can hang scripted usage. Consider adding-yso the handoff stays unattended.♻️ Proposed tweak
- args: ['skills', 'add', DEFAULT_SKILLS_REPOSITORY], + args: ['-y', 'skills', 'add', DEFAULT_SKILLS_REPOSITORY],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/skills.ts` around lines 16 - 19, The npx call inside the runHandoff invocation is missing the non-interactive flag; modify the args array passed to runHandoff (the call where exitCode = await runHandoff({...}) with command: 'npx') to include '-y' (e.g., ['-y', 'skills', 'add', DEFAULT_SKILLS_REPOSITORY']) so the npx invocation runs non-interactively and won’t prompt during scripted runs; update the args array in that runHandoff call referencing DEFAULT_SKILLS_REPOSITORY accordingly.src/lib/services/__tests__/agent-handoff.test.ts (1)
46-126: Consider adding test coverage for gemini and copilot agents.The
buildAgentHandoffInvocationtests cover claude, codex, cursor, and opencode, but gemini and copilot are missing. These agents have specific argument patterns (e.g., gemini uses-m gemini-3-flash-preview -y, copilot uses--allow-all-tools) that would benefit from explicit test coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/services/__tests__/agent-handoff.test.ts` around lines 46 - 126, Add unit tests for gemini and copilot to cover their specific arg patterns: extend the buildAgentHandoffInvocation test suite by calling buildAgentHandoffInvocation with createAgent('gemini', ...) and asserting args include the gemini model flags (e.g., '-m gemini-3-flash-preview' and '-y' or other expected gemini flags), and another case with createAgent('copilot', ...) asserting args include '--allow-all-tools' (and any other copilot-specific flags). Keep test structure consistent with existing tests (use invocation.args and invocation.env checks) and restore any modified process.env values after each test.src/lib/services/mcp-install-service.ts (1)
131-140: Heuristic output detection may be fragile.The
isAlreadyConfiguredOutputfunction relies on substring matching (already,exist,configured,registered) which could produce false positives or miss legitimate cases ifadd-mcp's output format changes.Consider documenting the expected output patterns or adding a more robust detection mechanism (e.g., specific error codes or structured output from add-mcp).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/services/mcp-install-service.ts` around lines 131 - 140, The isAlreadyConfiguredOutput function uses naive substring checks which can yield false positives; update isAlreadyConfiguredOutput to use stricter matching (e.g., regex with word boundaries like /\balready\b/, /\bexist(s|ing)?\b/, /\bconfigured\b/, /\bregistered\b/) or parse structured output/error codes if add-mcp can emit them, and document the expected add-mcp output patterns in a comment above the function (or add a TODO to switch to structured checks once add-mcp supports machine-readable output); reference the function name isAlreadyConfiguredOutput when making the change so callers keep the same API.src/lib/ios/pbxproj-modifier.ts (1)
574-584: Unreachable code path infindAppTargetUuid.The condition at line 579-581 is logically unreachable. If
descriptors.length === 1and!isLikelyNonMainTarget(descriptors[0].name), thennonAuxiliaryTargetat line 574 would have already found it (since it's the only descriptor and not a non-main target), making this branch never execute.🧹 Suggested simplification
const nonAuxiliaryTarget = descriptors.find((target) => !isLikelyNonMainTarget(target.name)); if (nonAuxiliaryTarget) { return nonAuxiliaryTarget.uuid; } - if (descriptors.length === 1 && !isLikelyNonMainTarget(descriptors[0].name)) { - return descriptors[0].uuid; - } - return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ios/pbxproj-modifier.ts` around lines 574 - 584, The check in findAppTargetUuid contains an unreachable branch: after computing nonAuxiliaryTarget = descriptors.find(target => !isLikelyNonMainTarget(target.name)), the subsequent if (descriptors.length === 1 && !isLikelyNonMainTarget(descriptors[0].name)) can never run; remove that redundant condition and simply return nonAuxiliaryTarget?.uuid ?? null (or return null if not found) to simplify logic while keeping the same behavior for descriptors, nonAuxiliaryTarget, and isLikelyNonMainTarget.src/commands/skill/preparation.ts (1)
207-210: Simplify redundant condition check.Line 208 checks
configuredPublicApiKeywhich is always truthy whenconfig.project.public_api_keyis truthy (sinceconfiguredPublicApiKey = config.project.public_api_key ?? config.project.publicKey).♻️ Suggested simplification
- if (config.project.public_api_key && !config.project.publicKey && configuredPublicApiKey) { + if (config.project.public_api_key && !config.project.publicKey) { return config; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/skill/preparation.ts` around lines 207 - 210, The condition in the preparation logic is redundant: since configuredPublicApiKey is defined as config.project.public_api_key ?? config.project.publicKey, checking configuredPublicApiKey when config.project.public_api_key is truthy is unnecessary. Update the if in src/commands/skill/preparation.ts (the block using configuredPublicApiKey and the condition checking config.project.public_api_key && !config.project.publicKey && configuredPublicApiKey) to simply check config.project.public_api_key && !config.project.publicKey and return config; remove the redundant configuredPublicApiKey check (and remove or repurpose the configuredPublicApiKey variable if it becomes unused).src/lib/executors/__tests__/opencode-executor.test.ts (1)
40-75: Consider simplifying environment restoration logic.The
finallyblocks have redundant branches—both paths assignoriginal(orundefined) back toprocess.env.OPENCODE_PERMISSION. This can be simplified.♻️ Suggested simplification
} finally { - if (original === undefined) { - process.env.OPENCODE_PERMISSION = undefined; - } else { - process.env.OPENCODE_PERMISSION = original; - } + process.env.OPENCODE_PERMISSION = original; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/__tests__/opencode-executor.test.ts` around lines 40 - 75, The finally blocks in the two tests redundantly branch to restore process.env.OPENCODE_PERMISSION; simplify both by replacing the conditional restore with a single assignment setting process.env.OPENCODE_PERMISSION = original so the environment is restored unconditionally after calling (executor as any).getSpawnEnv() in the permission env tests that reference OPENCODE_PERMISSION.src/lib/config/project-config-schema.ts (1)
206-225: Simplify redundant condition in migration logic.The condition on lines 209-211 is unreachable because
publicApiKeyis already derived fromconfig.project.public_api_key ?? config.project.publicKey, so ifpublicApiKeyis falsy, both source fields are already falsy.♻️ Proposed simplification
export function ensureLatestVersion(config: ProjectConfig): ProjectConfig { const publicApiKey = config.project.public_api_key ?? config.project.publicKey; - if (!publicApiKey && !config.project.public_api_key && !config.project.publicKey) { - return config; - } - - if (config.project.public_api_key && !config.project.publicKey) { + // Already migrated or no key to migrate + if (!publicApiKey || (config.project.public_api_key && !config.project.publicKey)) { return config; } return { ...config, project: { id: config.project.id, name: config.project.name, - ...(publicApiKey && { public_api_key: publicApiKey }), + public_api_key: publicApiKey, }, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/config/project-config-schema.ts` around lines 206 - 225, The migration function ensureLatestVersion contains an unreachable check that repeats what publicApiKey already represents: remove the redundant if block that tests (!publicApiKey && !config.project.public_api_key && !config.project.publicKey) and instead rely on the single publicApiKey presence test; keep the early return when publicApiKey is falsy, retain the existing branch that returns when config.project.public_api_key exists but config.project.publicKey is missing, and otherwise return the normalized project object—refer to ensureLatestVersion, publicApiKey, config.project.public_api_key, and config.project.publicKey when making the change.src/lib/services/firebase/api/firebase-api.ts (1)
161-215: Add logApiError wrappers for config/create calls.These public methods currently skip the error logging used elsewhere; wrapping them improves diagnostics consistency.
✍️ Suggested pattern (apply similarly to getIosConfig/createAndroidApp/createIosApp)
async getAndroidConfig(projectId: string, appId: string): Promise<string> { - await this.updateCredentials(); - const res = await this.fb.projects.androidApps.getConfig({ - name: `projects/${projectId}/androidApps/${appId}/config`, - }); - return Buffer.from(res.data.configFileContents ?? '', 'base64').toString('utf-8'); + try { + await this.updateCredentials(); + const res = await this.fb.projects.androidApps.getConfig({ + name: `projects/${projectId}/androidApps/${appId}/config`, + }); + return Buffer.from(res.data.configFileContents ?? '', 'base64').toString('utf-8'); + } catch (error) { + logApiError('getAndroidConfig', error); + throw error; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/services/firebase/api/firebase-api.ts` around lines 161 - 215, The methods getAndroidConfig, getIosConfig, createAndroidApp, and createIosApp currently call the Firebase client directly and miss the standardized error logging wrapper; wrap the API calls (the fb.projects.androidApps.getConfig, fb.projects.iosApps.getConfig, fb.projects.androidApps.create, and fb.projects.iosApps.create invocations) with the existing logApiError helper so any thrown errors are logged consistently, while preserving the surrounding await this.updateCredentials() and the subsequent handling (Buffer decoding in get*Config and the check + waitForOperation() flow in create*App); ensure you call logApiError with the original async call (or as a wrapper returning the same result) and keep existing return values and error checks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 3-118: The README was updated to change the command/agent surface
and config paths but the companion llms.txt was not; update llms.txt to mirror
all changes made in README.md (including the list of AI agents and any
renamed/added commands such as "clix agent", "clix install", "clix doctor",
"clix mcp", "clix skills", "clix update", and any config path changes or
skill/autonomy flags), ensuring entries for slash commands, AI agents, skills
(interactive/autonomous), CLI commands, and config paths are kept in sync;
search for the symbols "clix agent", "clix install", "clix doctor", "clix mcp",
"clix skills", and "llms.txt" to locate where to apply the updates.
In `@scripts/embed-skills.ts`:
- Around line 3-7: The embed-skills.ts change now embeds only local prompts
(install/doctor) and removes external package-based skills, so update
documentation to reflect this local-only embedding behavior: edit README.md to
describe the new embedding model and any changes to slash commands, AI agents,
interactive/autonomous skills, CLI commands, and config path behavior, and
update llms.txt to reflect supported LLMs/skill sources under the new local-only
scheme; ensure references to the script name embed-skills.ts and the
embedded-skills export (or src/lib/embedded-skills.ts) are updated so consumers
know where prompts now live and what is no longer auto-embedded.
In `@src/lib/constants/system-prompt.ts`:
- Around line 67-77: The slash-command list in
src/lib/constants/system-prompt.ts was changed but README.md and llms.txt were
not updated; update both README.md and llms.txt to reflect the new/edited slash
commands (e.g. /install, /integration, /event-tracking, /user-management,
/personalization, /api-triggered-campaigns, /doctor, /debug, /mcp) and ensure
descriptions match the entries in the system-prompt constants; verify any
related sections that document AI agents, skills, CLI commands, or config paths
are consistent with these changes.
In `@src/lib/executors/cli-process-manager.ts`:
- Around line 80-88: The spawnCLIProcess function currently passes the
caller-provided env directly to spawn which replaces the process environment
(dropping PATH); change it to merge process.env with the provided env so callers
can override keys but default variables (like PATH) are preserved — e.g. build a
mergedEnv from process.env and CLIProcessOptions.env and pass mergedEnv into
spawn; update symbols: spawnCLIProcess, CLIProcessOptions, and the spawn call to
use the merged environment.
In `@src/lib/ios/podfile-modifier.ts`:
- Around line 54-60: The extractTargetBlock function's targetRegex fails when
the closing "end" is indented; update the regex used in extractTargetBlock to
allow optional leading whitespace before the closing end (e.g., match "\n\s*end"
instead of "\nend") so indented blocks are captured; keep the non-greedy
([\s\S]*?) grouping and case-insensitive flag as-is, and optionally note in
comments near extractTargetBlock (or consider replacing it later) that a small
parser should be used if nested do/end blocks inside the target are expected.
In `@src/lib/services/__tests__/agent-handoff.test.ts`:
- Around line 88-106: The test incorrectly manipulates process.env by assigning
'' and undefined (which becomes the string "undefined") and has a redundant
assignment; update the test "injects OPENCODE_PERMISSION by default for
opencode" to delete process.env.OPENCODE_PERMISSION (use delete
process.env.OPENCODE_PERMISSION) instead of assigning undefined or repeating
assignments when preparing the environment before calling
buildAgentHandoffInvocation (and remove the duplicate assignment), and when
restoring state after the test, if previous is undefined delete
process.env.OPENCODE_PERMISSION else restore it via
process.env.OPENCODE_PERMISSION = previous; keep references to
OPENCODE_PERMISSION, buildAgentHandoffInvocation, and createAgent to locate the
change.
In `@src/lib/services/agent-handoff.ts`:
- Around line 192-195: The execve call obtained from getExecve() can throw on
failure; wrap the execve(invocation.command, [invocation.command,
...invocation.args], invocation.env) call in a try/catch, catch the error, log a
clear error including invocation.command, invocation.args and the caught error
details, and then exit or propagate a non-zero failure (e.g., process.exit(1) or
rethrow a wrapped error) so failures don’t silently propagate; update the code
around getExecve/execve to implement this handling.
In `@src/lib/skills/doctor/SKILL.md`:
- Line 101: Replace the incorrect slash-command reference "/install" in SKILL.md
with the supported CLI invocation "clix install" (or remove the slash-command
mention altogether) so the doc aligns with README.md and llms.txt; search for
the literal string "/install" in the document and update it to "clix install"
and, if the project intends to support a slash-command flow, instead add a note
referencing README.md and llms.txt and ensure those files are updated first to
document the feature.
In `@src/lib/utils/__tests__/oauth-callback-server.test.ts`:
- Around line 5-29: Tests currently bind a real HTTP server via
OAuthCallbackServer.start(), which can be flaky; modify the tests to avoid real
network binding by mocking the node:http server creation using mock.module() (or
by injecting a fake server into OAuthCallbackServer). Specifically, in the
failing tests that call OAuthCallbackServer.start(), replace the real start
behavior with a stubbed server that implements the same lifecycle and request
handling used by OAuthCallbackServer so that waitForCallback(), cancel(), and
stop() exercise the same logic without opening a port; keep references to
OAuthCallbackServer, start, waitForCallback, cancel, and stop so the tests still
validate cancellation and concurrent-wait behavior deterministically.
---
Outside diff comments:
In `@src/cli.tsx`:
- Around line 21-60: The generateHelpText function's returned help string is
missing documentation for the --startTask flag; update generateHelpText to
include a line under "Options" documenting --startTask (e.g., "--startTask <id>
Start a specific task on startup") and add an example invocation (such as "$
clix --startTask <id>" or "$ clix install --startTask <id>") in the "Examples"
section so the flag is discoverable; modify the string returned by
generateHelpText accordingly.
In `@src/lib/push/types.ts`:
- Around line 20-56: The public types PushSetupContext, PushSetupTaskPhase, and
PushSetupTaskResult were renamed and removing the old aliases (PushSetupPhase,
PushSetupResult) can break downstream TypeScript consumers; restore backward
compatibility by exporting type aliases mapping the old names to the new ones
(e.g., alias PushSetupPhase -> PushSetupTaskPhase and PushSetupResult ->
PushSetupTaskResult) alongside the existing definitions so both the new and old
type names resolve to the same interfaces/union types.
In `@src/lib/utils/oauth.ts`:
- Around line 343-386: The stop() method currently calls clearPendingCallback()
which leaves any Promise returned by waitForCallback() unresolved; update stop()
to reject any pending wait by calling rejectPendingCallback(new Error('Server
stopped')) (or use a dedicated Abort/Error type) before nulling the server so
callers don't hang; change the stop() implementation to invoke
rejectPendingCallback(...) instead of clearPendingCallback(), keeping
clearPendingCallback(), rejectPendingCallback(), resolvePendingCallback(),
getPort() unchanged.
---
Duplicate comments:
In `@src/commands/skill/preparation.ts`:
- Around line 354-366: The early return treats projectType.target === 'unknown'
as "not needed", which lets preparation report configured:true incorrectly;
update the logic in preparation.ts (where const needed = projectType.target !==
'unknown') to avoid marking Firebase/iOS as configured when target is unknown —
instead treat unknown as "needs validation" and run the normal checks (or return
needed: true) so functions that rely on configured flags (e.g., the iOS/Firebase
status checks) cannot short-circuit to configured:true; adjust any callers
expecting needed to be false for 'unknown' so readiness reflects actual setup.
In `@src/lib/config/project-config-manager.ts`:
- Around line 175-193: The updateSetup method merges partial SetupStatus into
the existing config but may persist invalid nested state; before calling
save(updatedConfig) merge the updates into config.setup (as already done) then
run the project's validation/normalization routine (e.g., validateProjectConfig
or a new validateSetup/normalizeSetup function) against the merged setup and/or
full ProjectConfig, throw a ConfigError with ERROR_CODES.INVALID_PROJECT_CONFIG
if validation fails, and only call this.save(updatedConfig) when
validation/normalization succeeds; updateSetup, SetupStatus, ProjectConfig, and
save/load are the relevant symbols to locate and modify.
In `@src/lib/services/firebase/api/firebase-api.ts`:
- Around line 74-95: In listProjects(), avoid casting p.state and instead
normalize it into the allowed union; update the loop that builds FirebaseProject
(inside async listProjects()) to map the incoming p.state string to either
'ACTIVE' or 'DELETED' (e.g., check if p.state === 'DELETED' then 'DELETED' else
'ACTIVE') so you return a properly typed state property on each project; locate
this change where res.data.results is iterated and replace the state: (p.state
as 'ACTIVE' | 'DELETED') ?? 'ACTIVE' with a small normalization expression that
derives 'ACTIVE'|'DELETED' from p.state.
In `@src/lib/services/firebase/downloader.ts`:
- Around line 331-342: In saveServiceAccountJson, harden filesystem permissions:
create the .clix directory (clixDir) with mode 0o700 when calling fs.mkdir, and
write the service-account.json (savePath) with mode 0o600 when calling
fs.writeFile (pass the mode option alongside 'utf-8' or as the options object)
so the directory and file are restricted to the owner; update the calls in
saveServiceAccountJson accordingly.
In `@src/lib/services/firebase/oauth/auth-client.ts`:
- Around line 337-344: The catch block around
refreshAccessToken(tokens.refresh_token) currently unconditionally rewraps every
error as "invalid_grant", causing unnecessary re-auth flows; change the handler
in the method in auth-client.ts to inspect the thrown error (e.g., check
error.message, error.code, or error.response?.data?.error) and only throw new
Error(`invalid_grant: ...`) when the underlying error actually indicates an
invalid_grant; otherwise rethrow the original error (or preserve its
message/stack) so transient/network errors are not treated as authentication
failures.
In `@src/lib/utils/oauth.ts`:
- Around line 272-285: The OAuth error logging currently writes raw full_url and
all_params which may contain sensitive query values; before calling
oauthLogger.writeToFile (in the OAuth error handling block where variables url,
req.url, error, errorDescription and all_params are prepared), sanitize by
redacting values for sensitive parameter names (e.g., code, state, token,
access_token, refresh_token, id_token, auth, secret) — produce a
sanitizedFullUrl (reconstruct the URL with sensitive params replaced by
"[REDACTED]") and sanitizedParams (Object.fromEntries with values replaced for
those keys) and pass those sanitized values instead of full_url and all_params
to oauthLogger.writeToFile.
---
Nitpick comments:
In `@src/commands/skill/preparation.ts`:
- Around line 207-210: The condition in the preparation logic is redundant:
since configuredPublicApiKey is defined as config.project.public_api_key ??
config.project.publicKey, checking configuredPublicApiKey when
config.project.public_api_key is truthy is unnecessary. Update the if in
src/commands/skill/preparation.ts (the block using configuredPublicApiKey and
the condition checking config.project.public_api_key &&
!config.project.publicKey && configuredPublicApiKey) to simply check
config.project.public_api_key && !config.project.publicKey and return config;
remove the redundant configuredPublicApiKey check (and remove or repurpose the
configuredPublicApiKey variable if it becomes unused).
In `@src/commands/skills.ts`:
- Around line 16-19: The npx call inside the runHandoff invocation is missing
the non-interactive flag; modify the args array passed to runHandoff (the call
where exitCode = await runHandoff({...}) with command: 'npx') to include '-y'
(e.g., ['-y', 'skills', 'add', DEFAULT_SKILLS_REPOSITORY']) so the npx
invocation runs non-interactively and won’t prompt during scripted runs; update
the args array in that runHandoff call referencing DEFAULT_SKILLS_REPOSITORY
accordingly.
In `@src/lib/config/project-config-schema.ts`:
- Around line 206-225: The migration function ensureLatestVersion contains an
unreachable check that repeats what publicApiKey already represents: remove the
redundant if block that tests (!publicApiKey && !config.project.public_api_key
&& !config.project.publicKey) and instead rely on the single publicApiKey
presence test; keep the early return when publicApiKey is falsy, retain the
existing branch that returns when config.project.public_api_key exists but
config.project.publicKey is missing, and otherwise return the normalized project
object—refer to ensureLatestVersion, publicApiKey,
config.project.public_api_key, and config.project.publicKey when making the
change.
In `@src/lib/executors/__tests__/opencode-executor.test.ts`:
- Around line 40-75: The finally blocks in the two tests redundantly branch to
restore process.env.OPENCODE_PERMISSION; simplify both by replacing the
conditional restore with a single assignment setting
process.env.OPENCODE_PERMISSION = original so the environment is restored
unconditionally after calling (executor as any).getSpawnEnv() in the permission
env tests that reference OPENCODE_PERMISSION.
In `@src/lib/ios/pbxproj-modifier.ts`:
- Around line 574-584: The check in findAppTargetUuid contains an unreachable
branch: after computing nonAuxiliaryTarget = descriptors.find(target =>
!isLikelyNonMainTarget(target.name)), the subsequent if (descriptors.length ===
1 && !isLikelyNonMainTarget(descriptors[0].name)) can never run; remove that
redundant condition and simply return nonAuxiliaryTarget?.uuid ?? null (or
return null if not found) to simplify logic while keeping the same behavior for
descriptors, nonAuxiliaryTarget, and isLikelyNonMainTarget.
In `@src/lib/services/__tests__/agent-handoff.test.ts`:
- Around line 46-126: Add unit tests for gemini and copilot to cover their
specific arg patterns: extend the buildAgentHandoffInvocation test suite by
calling buildAgentHandoffInvocation with createAgent('gemini', ...) and
asserting args include the gemini model flags (e.g., '-m gemini-3-flash-preview'
and '-y' or other expected gemini flags), and another case with
createAgent('copilot', ...) asserting args include '--allow-all-tools' (and any
other copilot-specific flags). Keep test structure consistent with existing
tests (use invocation.args and invocation.env checks) and restore any modified
process.env values after each test.
In `@src/lib/services/firebase/api/firebase-api.ts`:
- Around line 161-215: The methods getAndroidConfig, getIosConfig,
createAndroidApp, and createIosApp currently call the Firebase client directly
and miss the standardized error logging wrapper; wrap the API calls (the
fb.projects.androidApps.getConfig, fb.projects.iosApps.getConfig,
fb.projects.androidApps.create, and fb.projects.iosApps.create invocations) with
the existing logApiError helper so any thrown errors are logged consistently,
while preserving the surrounding await this.updateCredentials() and the
subsequent handling (Buffer decoding in get*Config and the check +
waitForOperation() flow in create*App); ensure you call logApiError with the
original async call (or as a wrapper returning the same result) and keep
existing return values and error checks intact.
In `@src/lib/services/mcp-install-service.ts`:
- Around line 131-140: The isAlreadyConfiguredOutput function uses naive
substring checks which can yield false positives; update
isAlreadyConfiguredOutput to use stricter matching (e.g., regex with word
boundaries like /\balready\b/, /\bexist(s|ing)?\b/, /\bconfigured\b/,
/\bregistered\b/) or parse structured output/error codes if add-mcp can emit
them, and document the expected add-mcp output patterns in a comment above the
function (or add a TODO to switch to structured checks once add-mcp supports
machine-readable output); reference the function name isAlreadyConfiguredOutput
when making the change so callers keep the same API.
Remove the mcp positional agent argument and make mcp run with a\nfixed add-mcp invocation.\n\nDrop the global --platform option from install/doctor command paths\nand related help/docs/tests.\n\nRequire install and doctor prompts to end with an explicit final\nresult block so agents always print a terminal summary.
…I before agent handoff Doctor command now gathers PreparationContext (reusing install's gatherPreparationContext) to verify Firebase, APNS, iOS entitlements, and NSE status before agent handoff. Shows colored status checklist using shared StatusLine/getStatusRows components. Missing items block handoff with clix install guidance. Ready state prompts user to accept AI diagnosis via GenericSelector. SKILL.md updated to remove JSON output in favor of readable text format, with pre-verified context as ground truth.
Improve install preparation behavior for Firebase and iOS setup tasks. - add Android/iOS identifier mismatch handling and app creation paths - detect Android package matches across all google-services clients - show Apple Team bundle IDs when bundle identifier is unavailable - tighten iOS entitlements detection and Apple auth UX handling - remove generated config header comments and align setup status checks
Ensure clix terminates with the handoff process exit code to prevent requiring extra Ctrl+C after agent/CLI handoff completes. Also simplify Firebase auth UI by removing the duplicated Direct URL section and showing a clickable full URL inline.
Critical fixes: - Add null guard in waitForOperation response handling - Reset installSkillRunStartedRef on retry to prevent stuck state - Classify invalid_grant vs transient token refresh errors correctly Code deduplication: - Extract shared firebase-detection-utils module - Export createDefaultOpenCodeEnv from opencode-executor for reuse - Move extractTextDelta to BaseExecutor (shared by Claude/Cursor) Consistency improvements: - Add streamMode to result path text emissions - Use AgentError instead of generic Error in command modules - Add useCancelInput to IosSetupUI authenticating phase - Wrap execve call with try/catch fallback in agent-handoff - Prevent double addTargetDependency in pbxproj-modifier Dead code removal: - Remove GeminiCLIInitEvent, OpenCodeCLISessionEvent dead types - Remove unused allowedTools from ExecuteOptions - Remove SkillMetadata and related dead exports - Inline single-use buildMcpHandoffArgs helper - Clean up dead test fixtures and cancelled phase
Restore supported agents table with free/paid tiers, add commands summary table, improve command descriptions, and add back development, links, and license sections removed during chat mode removal.
pitzcarraldo
changed the title
Security: - Harden service-account key storage with 0o600 file permissions - Redact sensitive OAuth params (code, state) before writing to debug.log Bug fixes: - Normalize Firebase project state instead of type-asserting - Clamp fractional concurrency to at least 1 - Merge provided env with process.env in CLI process spawn - Handle indented end lines in Podfile target extraction - Wrap execve in try/catch for spawn fallback Robustness: - Add safeStringify in logger to handle circular/unserializable data - Clean up redundant env var assignment in handoff test - Add supported agents list to llms.txt
The open dependency was intentionally removed but browser.ts still attempted to import it. Use platform-specific fallback directly.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/executors/base-executor.ts (1)
58-139:⚠️ Potential issue | 🟠 MajorReset delta state per execution.
lastTextContentpersists across runs, so deltas can be computed against the previous request’s output, yielding missing or mangled chunks.🐛 Suggested fix (reset per execution)
async *execute(prompt: string, options?: ExecuteOptions): AsyncGenerator<AgentMessage> { if (!(await this.isAvailable())) { yield { type: 'error', content: this.notFoundMessage }; return; } + this.lastTextContent = ''; const preparedPrompt = this.getPreparedPrompt(prompt); const args = this.buildArgs(preparedPrompt, options);If you expect concurrent executions on a single executor instance, consider storing the delta state in
StreamContextinstead of a class field.Also applies to: 200-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/base-executor.ts` around lines 58 - 139, lastTextContent is stored as a class field and persists across executions causing incorrect deltas; reset or relocate that state per run. Fix by clearing this.lastTextContent = '' at the start of the execution flow (e.g., in the method that begins a run such as execute or runProcess) before any calls to getPreparedPrompt/extractTextDelta/processStreamData, or move the delta state into StreamContext (add e.g., streamContext.lastTextContent and update extractTextDelta to use it) so concurrent runs don’t share mutable state; update usages of extractTextDelta/lastTextContent accordingly.
♻️ Duplicate comments (1)
src/commands/skill/preparation.ts (1)
377-389:⚠️ Potential issue | 🟠 MajorUnknown targets can still report “ready”.
Treatingtarget === 'unknown'as “not needed” can incorrectly mark setup as complete. This risks false positives when project detection fails.🐛 Suggested guard for unknown targets
- const needed = projectType.target !== 'unknown'; - - if (!needed) { - return { - configured: true, - androidConfigured: true, - iosConfigured: true, - senderConfigConfigured: true, - senderConfigProjectMatched: true, - needed: false, - }; - } + if (projectType.target === 'unknown') { + return { + configured: false, + androidConfigured: false, + iosConfigured: false, + senderConfigConfigured: false, + senderConfigProjectMatched: false, + needed: true, + }; + }- const needed = projectType.target === 'ios' || projectType.target === 'ios-android'; - - if (!needed) { - return { - needed: false, - entitlementsConfigured: true, - nseConfigured: true, - }; - } + if (projectType.target === 'unknown') { + return { + needed: true, + entitlementsConfigured: false, + nseConfigured: false, + }; + } + const needed = projectType.target === 'ios' || projectType.target === 'ios-android'; + if (!needed) { + return { + needed: false, + entitlementsConfigured: true, + nseConfigured: true, + }; + }- const needed = projectType.target === 'ios' || projectType.target === 'ios-android'; - - if (!needed) { - return { - needed: false, - registeredWithFirebase: true, - }; - } + if (projectType.target === 'unknown') { + return { + needed: true, + registeredWithFirebase: false, + }; + } + const needed = projectType.target === 'ios' || projectType.target === 'ios-android'; + if (!needed) { + return { + needed: false, + registeredWithFirebase: true, + }; + }Also applies to: 592-625
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/skill/preparation.ts` around lines 377 - 389, The early-return treats projectType.target === 'unknown' as "not needed" and thus marks setup as configured; remove or change that shortcut so unknown targets are treated as needing setup. Update the logic around the needed variable and the early return in preparation.ts (the block using projectType.target and the const needed) so that when projectType.target === 'unknown' you do NOT return the configured:true object — instead set needed = true (or configured = false) and let the normal checks flow so the caller sees the project as not configured and requiring attention.
🧹 Nitpick comments (5)
.gitignore (3)
81-85: Consider adding a section comment for better organization.The new entries would benefit from a descriptive section header for clarity and consistency with the rest of the file.
📝 Suggested organization
# Clix CLI local config .clix/ + +# Development artifacts .omc .ralph-tui llm-council plan.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 81 - 85, Add a descriptive section header above the new .gitignore entries (e.g., a comment like "# Local tools / project-specific" or "# Generated files and local artifacts") to match the file's existing organization; place this header immediately above the block containing ".omc", ".ralph-tui", "llm-council", and "plan.md" so it's clear these are local/tooling or generated artifacts.
82-84: Consider moving tool-specific artifacts to global gitignore.The entries
.omc,.ralph-tui, andllm-councilappear to be artifacts from personal development tools or AI coding assistants. Unless these tools are standardized across the entire team, these entries might be better suited for each developer's global gitignore (~/.gitignore_global) rather than the project's.gitignore.This helps keep the project
.gitignorefocused on project-specific patterns that apply to all contributors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 82 - 84, The three entries `.omc`, `.ralph-tui`, and `llm-council` in the project .gitignore look like personal/tool-specific artifacts; remove these lines from the project .gitignore and instead add them to each developer's global gitignore (e.g. ~/.gitignore_global) or document them as optional in CONTRIBUTING.md so the repo only contains project-wide ignore patterns; reference the `.gitignore` file and the specific entries `.omc`, `.ralph-tui`, and `llm-council` when making the change.
85-85:plan.mdmight be too generic to ignore project-wide.The filename
plan.mdis quite common and some developers might want to commit actual project planning documents with this name. Consider either:
- Using a more specific pattern like
.plan.mdor*.plan.mdfor personal planning docs- Moving this to personal global gitignore if it's for individual developer notes
- Documenting this convention if it's an intentional team standard
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 85, The .gitignore currently ignores the generic filename "plan.md", which may block legitimate project planning files; update the ignore rule by replacing "plan.md" with a more specific pattern (for example ".plan.md" or "*.plan.md") or remove it and add the entry to each developer's global gitignore, and if it's an intentional team convention, document this decision in the repository so contributors know which planning files are considered personal versus project assets.src/lib/services/firebase/api/firebase-api.ts (1)
161-175: Inconsistent error handling in config retrieval methods.
getAndroidConfigandgetIosConfigdon't wrap calls withtry/catchandlogApiError, unlikelistProjects,listAndroidApps, andlistIosApps. This creates inconsistent observability when these operations fail.♻️ Proposed fix for consistent error logging
async getAndroidConfig(projectId: string, appId: string): Promise<string> { + try { await this.updateCredentials(); const res = await this.fb.projects.androidApps.getConfig({ name: `projects/${projectId}/androidApps/${appId}/config`, }); return Buffer.from(res.data.configFileContents ?? '', 'base64').toString('utf-8'); + } catch (error) { + logApiError('getAndroidConfig', error); + throw error; + } } async getIosConfig(projectId: string, appId: string): Promise<string> { + try { await this.updateCredentials(); const res = await this.fb.projects.iosApps.getConfig({ name: `projects/${projectId}/iosApps/${appId}/config`, }); return Buffer.from(res.data.configFileContents ?? '', 'base64').toString('utf-8'); + } catch (error) { + logApiError('getIosConfig', error); + throw error; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/services/firebase/api/firebase-api.ts` around lines 161 - 175, getAndroidConfig and getIosConfig lack the try/catch + logApiError pattern used elsewhere, so wrap each method body in a try/catch: call await this.updateCredentials() and the fb API inside the try, return the decoded config on success, and in the catch call logApiError(this.logger, err, 'getAndroidConfig') / logApiError(this.logger, err, 'getIosConfig') (matching the existing logApiError usage), then rethrow the error so callers still receive it; this makes error logging consistent with listProjects/listAndroidApps/listIosApps.src/commands/skill/index.tsx (1)
168-173: Potential race condition with immediate unmount.When
!context.ready, thesetTimeout(..., 0)schedules an immediate unmount. However, the component has already been rendered and may be in the process of painting. Consider usingsetImmediateor moving this check before rendering to avoid the flash of content.♻️ Suggested refactor
async function runDoctorPrecheck(context: PreparationContext): Promise<'accepted' | 'cancelled'> { + // Early exit if context is not ready - no need to render the UI + if (!context.ready) { + return 'cancelled'; + } + return new Promise((resolve) => { const { unmount } = safeRender( <DoctorStatusDisplay context={context} onAccept={ - context.ready - ? () => { + () => { unmount(); resolve('accepted'); } - : undefined } onCancel={() => { unmount(); resolve('cancelled'); }} />, ); - - if (!context.ready) { - setTimeout(() => { - unmount(); - resolve('cancelled'); - }, 0); - } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/skill/index.tsx` around lines 168 - 173, The immediate unmount scheduled with setTimeout when checking `if (!context.ready)` can cause a render flash; instead perform this readiness check before rendering or bail out synchronously so the component never paints. Locate the `if (!context.ready)` block and either move that conditional above the render/mount logic (so you call `unmount()` and `resolve('cancelled')` before mounting), or replace the delayed `setTimeout(..., 0)` with a synchronous early-return/unmount sequence (call `unmount()` then `resolve('cancelled')` immediately) to avoid the race; reference `context.ready`, `setTimeout`, `unmount`, and `resolve` when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/config/project-config-manager.ts`:
- Around line 131-142: The migrated config returned by ensureLatestVersion keeps
the old version, causing repeated saves; after calling
ensureLatestVersion(assign to migratedConfig) update migratedConfig.version to
CURRENT_PROJECT_CONFIG_VERSION (or make ensureLatestVersion set it) before
calling this.save(...) so the persisted config reflects the new version; ensure
you still assign this.cachedConfig = migratedConfig and return it from load().
In `@src/lib/constants/system-prompt.ts`:
- Around line 67-70: Update the section header and entries in the system prompt
constant so they no longer reference “Slash Commands” or include the slash
prefix; specifically replace the header string "## Available Slash Commands"
with something like "## Available Commands" and change the list items
"/install", "/doctor", and "/mcp" to "install", "doctor", and "mcp" respectively
in the exported system prompt (the constant in system-prompt.ts) so the wording
matches the command-only UX.
In `@src/lib/ios/apple-auth.ts`:
- Around line 118-134: The code currently returns lastAppleId early which
prevents switching accounts; instead remove the early return in the if
(lastAppleId) block and keep the interactive prompt flow: show the cached value
as the default to promptFn (pass lastAppleId as the second argument to
promptFn), still call removeControlCharacters on the response and validate
username.trim() in the while loop, and optionally log that a cached Apple ID is
being used as the default; update references in this block (lastAppleId,
promptFn, removeControlCharacters, username) so the prompt allows overriding the
cached value rather than returning it immediately.
In `@src/lib/ios/pbxproj-modifier.ts`:
- Around line 1100-1104: The current logic only calls
addTargetDependencyToMainApp when result.targetAdded is true, so existing
targets that lack the dependency are never fixed; change the check to enforce
dependency whenever targetDependencyAlreadyAdded is false (i.e., call
addTargetDependencyToMainApp(project, targetUuid, result.warnings) when
targetDependencyAlreadyAdded is false regardless of result.targetAdded), and if
you still want to distinguish creation vs existing-target paths emit a clear
warning when skipping adding the dependency; use the existing symbols
result.targetAdded, targetDependencyAlreadyAdded, addTargetDependencyToMainApp,
project and targetUuid to locate and update the condition and any warning
messages.
In `@src/lib/ios/podfile-modifier.ts`:
- Around line 138-141: The replacement currently uses
updatedBlock.replace(/\nend$/i, ...) which fails when the block ends with
indented "end" and can leave clixPodAdded=true despite no insertion; update the
regex in the call to updatedBlock.replace to capture any indentation (e.g.
/\n(\s*)end$/i) and use the captured indentation in the replacement (e.g. `\n$1
pod '${options.clixPodSpec || 'Clix'}'\n$1end'`) so the pod line keeps the same
indentation, and/or perform a .test with the new regex before replacing to
ensure the replacement actually occurred before setting clixPodAdded
(references: updatedBlock.replace, extractTargetBlock, clixPodAdded,
options.clixPodSpec).
---
Outside diff comments:
In `@src/lib/executors/base-executor.ts`:
- Around line 58-139: lastTextContent is stored as a class field and persists
across executions causing incorrect deltas; reset or relocate that state per
run. Fix by clearing this.lastTextContent = '' at the start of the execution
flow (e.g., in the method that begins a run such as execute or runProcess)
before any calls to getPreparedPrompt/extractTextDelta/processStreamData, or
move the delta state into StreamContext (add e.g., streamContext.lastTextContent
and update extractTextDelta to use it) so concurrent runs don’t share mutable
state; update usages of extractTextDelta/lastTextContent accordingly.
---
Duplicate comments:
In `@src/commands/skill/preparation.ts`:
- Around line 377-389: The early-return treats projectType.target === 'unknown'
as "not needed" and thus marks setup as configured; remove or change that
shortcut so unknown targets are treated as needing setup. Update the logic
around the needed variable and the early return in preparation.ts (the block
using projectType.target and the const needed) so that when projectType.target
=== 'unknown' you do NOT return the configured:true object — instead set needed
= true (or configured = false) and let the normal checks flow so the caller sees
the project as not configured and requiring attention.
---
Nitpick comments:
In @.gitignore:
- Around line 81-85: Add a descriptive section header above the new .gitignore
entries (e.g., a comment like "# Local tools / project-specific" or "# Generated
files and local artifacts") to match the file's existing organization; place
this header immediately above the block containing ".omc", ".ralph-tui",
"llm-council", and "plan.md" so it's clear these are local/tooling or generated
artifacts.
- Around line 82-84: The three entries `.omc`, `.ralph-tui`, and `llm-council`
in the project .gitignore look like personal/tool-specific artifacts; remove
these lines from the project .gitignore and instead add them to each developer's
global gitignore (e.g. ~/.gitignore_global) or document them as optional in
CONTRIBUTING.md so the repo only contains project-wide ignore patterns;
reference the `.gitignore` file and the specific entries `.omc`, `.ralph-tui`,
and `llm-council` when making the change.
- Line 85: The .gitignore currently ignores the generic filename "plan.md",
which may block legitimate project planning files; update the ignore rule by
replacing "plan.md" with a more specific pattern (for example ".plan.md" or
"*.plan.md") or remove it and add the entry to each developer's global
gitignore, and if it's an intentional team convention, document this decision in
the repository so contributors know which planning files are considered personal
versus project assets.
In `@src/commands/skill/index.tsx`:
- Around line 168-173: The immediate unmount scheduled with setTimeout when
checking `if (!context.ready)` can cause a render flash; instead perform this
readiness check before rendering or bail out synchronously so the component
never paints. Locate the `if (!context.ready)` block and either move that
conditional above the render/mount logic (so you call `unmount()` and
`resolve('cancelled')` before mounting), or replace the delayed `setTimeout(...,
0)` with a synchronous early-return/unmount sequence (call `unmount()` then
`resolve('cancelled')` immediately) to avoid the race; reference
`context.ready`, `setTimeout`, `unmount`, and `resolve` when applying the
change.
In `@src/lib/services/firebase/api/firebase-api.ts`:
- Around line 161-175: getAndroidConfig and getIosConfig lack the try/catch +
logApiError pattern used elsewhere, so wrap each method body in a try/catch:
call await this.updateCredentials() and the fb API inside the try, return the
decoded config on success, and in the catch call logApiError(this.logger, err,
'getAndroidConfig') / logApiError(this.logger, err, 'getIosConfig') (matching
the existing logApiError usage), then rethrow the error so callers still receive
it; this makes error logging consistent with
listProjects/listAndroidApps/listIosApps.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
.gitignoreREADME.mdllms.txtscripts/embed-skills.tssrc/cli.tsxsrc/commands/__tests__/mcp.test.tssrc/commands/__tests__/skills.test.tssrc/commands/doctor.tsxsrc/commands/install.tsxsrc/commands/mcp.tssrc/commands/skill/__tests__/preparation.test.tssrc/commands/skill/index.tsxsrc/commands/skill/preparation.tssrc/commands/skills.tssrc/lib/__tests__/embedded-skills.test.tssrc/lib/__tests__/skills.test.tssrc/lib/__tests__/test-utils/fixtures.tssrc/lib/config/project-config-manager.tssrc/lib/constants/system-prompt.tssrc/lib/debug/logger.tssrc/lib/embedded-skills.tssrc/lib/executor.tssrc/lib/executors/__tests__/gemini-executor.test.tssrc/lib/executors/__tests__/opencode-executor.test.tssrc/lib/executors/base-executor.tssrc/lib/executors/claude-executor.tssrc/lib/executors/cli-process-manager.tssrc/lib/executors/cursor-executor.tssrc/lib/executors/gemini-executor.tssrc/lib/executors/opencode-executor.tssrc/lib/executors/types.tssrc/lib/ios/apple-auth.tssrc/lib/ios/apple-portal.tssrc/lib/ios/pbxproj-modifier.tssrc/lib/ios/podfile-modifier.tssrc/lib/services/__tests__/agent-handoff.test.tssrc/lib/services/agent-handoff.tssrc/lib/services/firebase/api/firebase-api.tssrc/lib/services/firebase/downloader.tssrc/lib/services/firebase/oauth/auth-client.tssrc/lib/services/organization-projects.tssrc/lib/services/project-detector.tssrc/lib/skills.tssrc/lib/skills/doctor/SKILL.mdsrc/lib/skills/install/SKILL.mdsrc/lib/utils/oauth.tssrc/ui/IosSetupUI.tsxsrc/ui/components/FirebaseConfigFilesSetup.tsxsrc/ui/components/FirebaseServiceAccountSetup.tsxsrc/ui/components/InstallPreparationUI.tsxsrc/ui/components/firebase-detection-utils.tssrc/ui/components/ios-setup/IosEntitlementsTask.tsxtests/e2e/cli.test.ts
💤 Files with no reviewable changes (2)
- src/lib/executors/types.ts
- src/lib/tests/test-utils/fixtures.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/executors/tests/gemini-executor.test.ts
- src/lib/executors/tests/opencode-executor.test.ts
- src/lib/services/tests/agent-handoff.test.ts
- src/commands/tests/mcp.test.ts
- src/lib/services/organization-projects.ts
- src/commands/skills.ts
- Set version to CURRENT_PROJECT_CONFIG_VERSION in ensureLatestVersion to prevent needless re-save on every startup - Rename "Slash Commands" to "Available Commands" in system prompt - Allow Apple ID override from cache instead of early return - Enforce NSE target dependency for existing targets missing it - Fix pod insertion regex to handle indented end blocks
…I help, and improve README table formatting.
Previously ensureGitignore was only called after project selection in SetupUI/LoginUI. If the user interrupted (Ctrl+C) after login but before project selection, the .clix directory was created without a matching .gitignore entry. Extracted gitignore logic to shared utility and moved the call into directory creation (ensureStateDir/ensureConfigDir) so it runs exactly once at the earliest possible moment.
…rebase iOS app Team ID management - Introduce setExitCode() utility and centralize process.exit() to a single main().finally() guard to prevent terminal hang on command cancellation - Fix P8 file path input corruption when drag-dropping files from macOS Finder by replacing ink-text-input with ref-based accumulation (avoids React state batching race condition with multi-chunk stdin paste) - Add Firebase iOS app patchIosApp API for updating teamId/appStoreId - Detect Team ID mismatch between APNS key and Firebase iOS app after registration, with option to auto-update - Refactor IosSetupUI Apple login from terminal readline prompts to Ink-based TextInput UI with session restore support - Pass appName from Xcode project analysis to Firebase app creation
This was referenced Feb 24, 2026
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
@googleapis/firebaseand@googleapis/iammcpandskillscommands, simplifydoctorwith pre-check verificationKey Changes
Architecture: Command-Mode Only
ChatApp, session management, slash commands, and interactive skillsclix(no args) now shows help and exitsinstallanddoctoruse agent handoff pattern (preparation → launch agent CLI)Firebase API Migration
@googleapis/firebaseand@googleapis/iampackagesinvalid_grantvs transient error classificationInstall Preparation Flow
FirebaseWizard+IosSetupFlowwith sequential task componentssrc/ui/components/New Commands
clix mcp— install Clix MCP Server (renamed frominstall-mcp, removed agent parameter)clix skills— install Clix skill package via skills CLIDoctor Pre-Check
Code Quality (Review Fixes)
AgentErrorconsistently instead of genericErrorin command modulesaddTargetDependencyin pbxproj modifierStats
179 files changed, ~14.4K insertions, ~17.1K deletions (net -2.7K lines)
How to Validate
```bash
bun install
bun run check # lint + typecheck (0 warnings, 0 errors)
bun test # 473 unit tests pass
bun run build && bun test tests/e2e/ # 18 E2E tests pass
```
Manual smoke tests:
Summary by CodeRabbit
New Features
clix installcommand with pre-flight setup verification and preparation workflow.clix doctorcommand for diagnosing project configuration status.Bug Fixes
Documentation