ADE-72: Chat/AI runtime: Keychain key in argv + SDK pool ref-count race, plus chat/AI/ade-action god-file & typed-dispatch decomposition

[arul28]

Refs ADE-72

Summary

Describe the change.

What Changed

Key files and behaviors.

Validation

How you tested.

Risks

Anything to watch.

Linked Linear issues

• ADE-72: Chat/AI runtime: Keychain key in argv + SDK pool ref-count race, plus chat/AI/ade-action god-file & typed-dispatch decomposition - referenced

  Open in ADE  ·  ade-72-chat-ai-runtime-keychain-key-in-argv-sdk-pool-ref-count-race-plus-chat-ai-ade-action-god-file-typed-dispatch-decomposition branch  ·  PR #494

Greptile Summary

This PR fixes two distinct runtime bugs: (1) API keys were being passed as clear-text CLI arguments to /usr/bin/security (visible in ps output), replaced now by writing exclusively to Electron safeStorage; (2) concurrent acquireSdkConnection callers shared a single init promise but only the non-initOwner caller incremented the ref-count — a race that could produce a premature dispose. The migration direction is also inverted (Keychain → encrypted store instead of encrypted store → Keychain), and the provider-index write path is removed entirely.

• apiKeyStore: writeMacosKeychainSecret and its provider-index helpers are deleted; storeApiKey and deleteApiKey now always target safeStorage; a new migrateLegacyMacosKeychainIntoEncryptedStore runs on first cold start to absorb any pre-existing Keychain entries into the encrypted store, with encrypted-store values taking precedence over Keychain values in the merge.
• cursorSdkPool / droidSdkPool: Unbounded tail-recursion replaced by a bounded for loop (STALE_INIT_RETRY_LIMIT = 2); both initOwner and non-initOwner callers check process liveness after await init; initOwner throws immediately on post-init worker exit; non-initOwner retries up to the limit before throwing.
• Tests: New pool tests assert that two concurrent acquires share one fork call and each hold an independent ref that must be released before dispose fires; apiKeyStore tests updated to assert zero add-generic-password calls and verify the new merge semantics.

Confidence Score: 5/5

Safe to merge; the security fix and ref-count corrections are sound, and the two noted observations are edge-case quality improvements.

All three core changes (removing Keychain argv writes, inverting migration direction, bounding SDK pool retries) are implemented correctly and are covered by new tests. The two flagged items are resilience and cleanup concerns that surface only under unusual conditions — a transient disk-write failure during startup migration, and repeated but idempotent migration writes for users who upgrade without changing their keys. Neither represents incorrect behaviour under normal operating conditions.

apps/desktop/src/main/services/ai/apiKeyStore.ts — the migration path in ensureStore deserves a second look for error-handling resilience.

Important Files Changed

Filename	Overview
apps/desktop/src/main/services/ai/apiKeyStore.ts	Security fix removing writeMacosKeychainSecret (which passed keys as -w key CLI args visible in process list); rewrites migration direction (Keychain to encrypted store); migrateLegacyMacosKeychainIntoEncryptedStore lacks the try/catch that guarded the old migration path.
apps/desktop/src/main/services/ai/apiKeyStore.test.ts	Tests updated to reflect new behaviour: no Keychain writes, encrypted-store wins merge, deleteApiKey soft-fails gracefully when storage is unavailable; covers the new migration direction and stale-Keychain cleanup.
apps/desktop/src/main/services/chat/cursorSdkPool.ts	Converts unbounded recursive retry into a bounded for-loop with STALE_INIT_RETRY_LIMIT=2; correctly increments ref only for non-initOwner callers after a shared init settles; initOwner now throws immediately on post-init worker exit.
apps/desktop/src/main/services/chat/cursorSdkPool.test.ts	New test validates the ref-count race fix: two concurrent acquires share one fork, each acquire holds an independent ref, and dispose fires only after both refs are released.
apps/desktop/src/main/services/chat/droidSdkPool.ts	Mirrors the cursorSdkPool bounded-retry pattern; same ref-count race fix applied consistently.
apps/desktop/src/main/services/chat/droidSdkPool.test.ts	New test file, mirrors the cursorSdkPool ref-count test; FakeSdkChild correctly simulates the init RPC response and dispose accounting.

Sequence Diagram

sequenceDiagram
    participant App
    participant ensureStore
    participant loadEncryptedStore
    participant migrateLegacyKeychain
    participant Keychain
    participant safeStorage

    App->>ensureStore: "first call (cache=null)"
    ensureStore->>loadEncryptedStore: read encrypted store
    loadEncryptedStore->>safeStorage: decryptString(blob)
    safeStorage-->>loadEncryptedStore: stored keys map
    loadEncryptedStore-->>ensureStore: encryptedStore
    alt canPersistEncryptedStore() and isMacosKeychainAvailable()
        ensureStore->>migrateLegacyKeychain: encryptedStore
        migrateLegacyKeychain->>Keychain: readProviderIndex and readStore
        Keychain-->>migrateLegacyKeychain: legacy entries
        Note over migrateLegacyKeychain: merge keychainStore then encryptedStore encrypted store wins on conflict
        migrateLegacyKeychain->>safeStorage: persistEncryptedStore(merged)
        migrateLegacyKeychain-->>ensureStore: mergedStore becomes cache
    else safeStorage unavailable and Keychain available
        ensureStore->>Keychain: readProviderIndex and readStore read-only
        Keychain-->>ensureStore: cached keys
    end

    App->>storeApiKey: provider and new value
    storeApiKey->>safeStorage: persistEncryptedStore
    storeApiKey->>Keychain: deleteMacosKeychainSecretBestEffort
    Note over storeApiKey: mark provider in missingMacosKeychainProviders

    App->>releaseSdkConnection: poolKey and generation
    Note over releaseSdkConnection: ref minus 1 if zero then dispose and cleanup

Loading

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
apps/desktop/src/main/services/ai/apiKeyStore.ts:461-465
Missing error resilience around `persistEncryptedStore` in migration. The old `ensureStore` path that called `migrateEncryptedStoreToMacosKeychain` was wrapped in a `try/catch` so that a transient disk-write failure would silently fall back to the encrypted store as-is. The new path calls `migrateLegacyMacosKeychainIntoEncryptedStore` without any guard, so any `persistEncryptedStore` failure (full disk, permission error, brief safeStorage blip) propagates out of `ensureStore()` and breaks every subsequent `getApiKey` / `storeApiKey` / `deleteApiKey` call for users who have legacy Keychain entries at startup.

```suggestion
  if (isMacosKeychainAvailable()) {
    if (canPersistEncryptedStore()) {
      try {
        cache = migrateLegacyMacosKeychainIntoEncryptedStore(encryptedStore);
      } catch {
        // A transient write failure during migration should not break all
        // API key operations — fall back to the in-memory encrypted store.
        cache = encryptedStore;
      }
      return cache;
    }
```

### Issue 2 of 2
apps/desktop/src/main/services/ai/apiKeyStore.ts:433-447
**Migration reruns on every startup until Keychain entries are manually evicted**

`migrateLegacyMacosKeychainIntoEncryptedStore` calls `persistEncryptedStore` on every cold `ensureStore()` (i.e. every app restart) for as long as any legacy Keychain entry survives. Keychain entries are only deleted by a subsequent `storeApiKey` call via `deleteMacosKeychainSecretBestEffort`. A user who upgrades but never re-saves their API keys will trigger an unnecessary safeStorage disk-write on every launch. Consider deleting each migrated provider's Keychain entry inside the migration itself (or recording a completion flag in the encrypted store) to make the migration a true one-shot operation.

_{Reviews (2): Last reviewed commit: "fix: address chat runtime review finding..." | Re-trigger Greptile}

[linear-code]

ADE-72

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
ade	Ignored	Preview	May 31, 2026 11:59pm

[coderabbitai]

Warning

Review limit reached

@arul28, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 31 minutes and 9 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7e6b1bbe-2bbc-4ecd-b018-e8ac1a3c58d7

📥 Commits

Reviewing files that changed from the base of the PR and between 840acb7 and e62d38c.

📒 Files selected for processing (6)

• apps/desktop/src/main/services/ai/apiKeyStore.test.ts
• apps/desktop/src/main/services/ai/apiKeyStore.ts
• apps/desktop/src/main/services/chat/cursorSdkPool.test.ts
• apps/desktop/src/main/services/chat/cursorSdkPool.ts
• apps/desktop/src/main/services/chat/droidSdkPool.test.ts
• apps/desktop/src/main/services/chat/droidSdkPool.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ade-72-chat-ai-runtime-keychain-key-in-argv-sdk-pool-ref-count-race-plus-chat-ai-ade-action-god-file-typed-dispatch-decomposition

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[capy-ai]

Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews.

[arul28]

@copilot review but do not make fixes