Ios Fixes#229
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughReplaces Cursor ACP/CLI integration with a Cursor SDK + API-key flow; implements an SDK worker pool, SDK-based model discovery/probing, macOS Keychain API-key storage, shared modelCatalog utilities, runtime-driven session/title behaviors, iOS simulator helper build changes, audit/dev tooling, and many tests/docs updates. ChangesCursor SDK integration & runtime
Cursor model discovery & policy
API key storage & Keychain
AI integration, provider health & readiness
Agent chat service, titles & model catalog
OpenCode / PTY / session title hygiene
Shared registry / profiles
iOS simulator helpers & streaming
Audit & developer tooling
Preload, IPC, main startup
Sync & iOS host catalog
UI / tests / minor UX
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
@copilot review but do not make fixes |
Here's my review of the PR: OverviewThis PR ships three main things: (1) a new host-catalog sync path for iOS model discovery, (2) Cursor SDK hook scaffolding, and (3) a macOS Keychain backend for API key storage. The scope is large but the intent is clear. Below are the issues I found, roughly ordered by severity. Issues1.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code ReviewOverviewThis PR introduces three main features: (1) a unified Issues & Observations1. 2. Two 3. 4. Hook bridge script embedded as a template literal 5. 6. 7. 8. Minor Notes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/desktop/src/main/services/ai/providerConnectionStatus.ts (1)
95-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep Cursor
usageAvailablein sync with runtime-health downgrades.After this change, a Cursor
auth-failedhealth state flipsruntimeAvailabletofalsebut leavesusageAvailableastrue. That produces a contradictory status object for the same API-key path.Suggested fix
function applyRuntimeHealth( status: AiProviderConnectionStatus, health: ReturnType<typeof getProviderRuntimeHealth> | null, ): void { if (!health) return; if (health.state === "auth-failed" || health.state === "runtime-failed") { status.runtimeAvailable = false; + if (status.provider === "cursor") { + status.usageAvailable = false; + } status.blocker = health.message ?? (health.state === "auth-failed" ? `${status.provider} runtime was detected, but ADE chat reported that login is still required.` : `${status.provider} runtime was detected, but ADE could not launch it from this app session.`); } else if (health.state === "ready") { status.runtimeAvailable = true; status.authAvailable = true; + if (status.provider === "cursor") { + status.usageAvailable = true; + } status.blocker = null; } }Also applies to: 228-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/providerConnectionStatus.ts` around lines 95 - 105, The health-state handling updates runtimeAvailable (in providerConnectionStatus.ts) but fails to mirror that downgrade to usageAvailable, causing inconsistent status objects; update the branches that set status.runtimeAvailable = false for health.state values like "auth-failed" and "runtime-failed" (and the similar block around the other occurrence referenced) to also set status.usageAvailable = false and ensure when health.state === "ready" you set status.usageAvailable = true alongside runtimeAvailable and authAvailable; locate the status object mutations in the function handling health (look for the health.state checks and the status variable) and apply the same usageAvailable changes in both places.apps/desktop/src/main/services/ai/authDetector.ts (1)
791-833:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the stale Cursor auth failure on successful verification.
This path now records
auth-failedinto global runtime health, but the success path never marks Cursor ready again. After one bad key, a later successful re-check can still leavebuildProviderConnections()downgraded until some unrelated code resets that shared state.Suggested fix
-import { reportProviderRuntimeAuthFailure } from "./providerRuntimeHealth"; +import { + reportProviderRuntimeAuthFailure, + reportProviderRuntimeReady, +} from "./providerRuntimeHealth"; async function verifyCursorApiKey( key: string, verifiedAt: string, ): Promise<ApiKeyVerificationResult> { let timeoutHandle: ReturnType<typeof setTimeout> | null = null; try { const { Cursor } = await import("@cursor/sdk"); const user = await Promise.race([ Cursor.me({ apiKey: key }), new Promise<never>((_, reject) => { timeoutHandle = setTimeout( () => reject(new Error("Verification timed out.")), API_KEY_VERIFY_TIMEOUT_MS, ); }), ]); + reportProviderRuntimeReady("cursor"); return { provider: "cursor", ok: true, message: user.userEmail ? `Connection verified for ${user.userEmail}.`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/authDetector.ts` around lines 791 - 833, The success branch in verifyCursorApiKey currently never clears the prior "auth-failed" state, so add a call to clear the stale Cursor auth failure before returning on success; locate verifyCursorApiKey and, just before the successful return in the try block, invoke the existing runtime-health clearing helper (e.g., clearProviderRuntimeAuthFailure("cursor") or the counterpart to reportProviderRuntimeAuthFailure) to mark Cursor as healthy so buildProviderConnections() isn't left downgraded after a later successful verification.apps/desktop/src/renderer/components/settings/ProvidersSection.tsx (1)
451-467:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the previous verification badge before each new verify attempt.
If either IPC call rejects,
verificationByProviderkeeps its old value, so the panel can continue showing the last success/failure badge for a different key or an earlier attempt. Clearing that entry before the request avoids stale verification state on retries.💡 Localized fix
setError(null); setNotice(null); setVerifyingProvider(provider); + setVerificationByProvider((prev) => { + const next = { ...prev }; + delete next[provider]; + return next; + }); try { invalidateAiDiscoveryCache(); const result = await window.ade.ai.verifyApiKey(provider);setError(null); setNotice(null); setVerifyingProvider("cursor"); + setVerificationByProvider((prev) => { + const next = { ...prev }; + delete next.cursor; + return next; + }); try { await window.ade.ai.storeApiKey("cursor", trimmed);Also applies to: 470-492
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx` around lines 451 - 467, The verification badge for a provider can remain stale because verificationByProvider isn't cleared before making IPC calls; modify verifyApiKey to clear the entry for the target provider (use setVerificationByProvider to remove or set undefined for [provider]) immediately after setVerifyingProvider(provider) and before calling invalidateAiDiscoveryCache()/window.ade.ai.verifyApiKey, so any old success/failure badge is removed while the new request runs; apply the same change to the other verification routine covering lines 470-492 (the other function that also calls setVerificationByProvider/verifyApiKey IPCs) so both code paths clear the previous verification state up-front.apps/desktop/src/main/services/ai/apiKeyStore.ts (1)
381-395:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly update
cacheafter persistence succeeds.Both
storeApiKey()anddeleteApiKey()mutate the in-memory store before the Keychain/encrypted-store write completes. If that write throws, the current process reports the new state even though nothing was durably saved, and the mismatch only shows up after restart.Also applies to: 412-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/apiKeyStore.ts` around lines 381 - 395, storeApiKey and deleteApiKey currently mutate the in-memory store before performing durable writes; change both to perform persistence first and only update the in-memory cache on success: in storeApiKey, trim inputs and validate, then perform the platform write(s) (call writeMacosKeychainSecret and update provider index via readMacosKeychainProviderIndex/writeMacosKeychainProviderIndex for macOS, or call persistEncryptedStore for the encrypted path) using local variables and without touching ensureStore() result, and only after those calls succeed assign the key into the result of ensureStore(); apply the analogous approach to deleteApiKey (perform keychain/index removal or persistEncryptedStore first, then remove from in-memory store), and ensure any thrown errors leave the in-memory store unchanged.apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx (1)
1715-1725:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't overwrite service-managed fallback messaging with
lastError.This branch still copies
event.status.lastErrorintoliveVisual.errorduring service-managed fallback. The reconnect overlay readsliveVisual.errorfirst, so users will still see the raw transport error instead of the new degradation/fallback reason you added.Suggested fix
if ((event.type === "stream-stopped" || event.type === "stream-error") && !event.status.streamUrl) { setLiveVisual((current) => current?.kind === "mjpeg" ? { ...current, status: event.type === "stream-error" ? "reconnecting" : current.status, url: null, - error: event.status.lastError ?? current.error, + error: serviceManagedFallback + ? (event.status.degradationReason ?? event.status.fallbackReason ?? current.error) + : (event.status.lastError ?? current.error), } : current); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx` around lines 1715 - 1725, The code is copying event.status.lastError into liveVisual.error even when serviceManagedFallback is true, which causes the reconnect overlay to show the raw transport error; update the setLiveVisual call used when handling "stream-stopped"/"stream-error" so that inside the updater you only assign error = event.status.lastError ?? current.error when serviceManagedFallback is false, otherwise preserve current.error (or set the service-managed fallback message) so serviceManagedFallback prevents overwriting liveVisual.error; adjust the setMessage branch accordingly if needed (symbols: setLiveVisual, liveVisual, event, event.status.lastError, serviceManagedFallback).apps/ios/ADE/Views/Work/WorkModelCatalog.swift (1)
487-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize Claude aliases to one canonical equivalence key before adding shorthand names.
workDeduplicatedModelOptions()ultimately relies on the first non-registry key fromworkModelLookupKeys(). With these new Claude mappings, equivalent IDs can now diverge: e.g.anthropic/claude-sonnet-4-6yieldssonnet, whileclaude-sonnet-4-6yieldsclaude-sonnet-4-6. That means the same model can survive deduplication twice when the host mixes canonical and runtime IDs.A safer pattern here is to derive one canonical Claude key for equivalence/deduplication first, and only append shorthand aliases like
sonnet/haikufor lookup convenience after that.Also applies to: 510-538
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Work/WorkModelCatalog.swift` around lines 487 - 505, The code appends runtime and registry Claude IDs before adding shorthand aliases, causing mixed canonical/runtime keys to bypass deduplication in workDeduplicatedModelOptions()/workModelLookupKeys(); change the order so you first derive and append one canonical Claude equivalence key using workCanonicalClaudeRegistryId(for: trimmed) (or runtime->canonical mapping if needed), then append any shorthand aliases like the short names (sonnet/haiku) afterwards; apply the same canonical-first ordering to the second Claude block (lines ~510-538) and mirror the pattern for any analogous Codex/other model mapping code (workCanonicalCodexRegistryId(), workCodexRuntimeModelId()) so shorthand aliases are only added after a canonical key is present.
🟡 Minor comments (9)
apps/desktop/src/main/services/pty/ptyService.ts-2147-2149 (1)
2147-2149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrefer the in-memory chat binding over the persisted row here.
The non-live branch now returns
row.chatSessionIdbeforeterminalChatSessions.get(row.id), butterminalSessionFromSummary()does the opposite. That can makeenrichSessions()return a stale chat binding after a rebinding/reattach path updated only the in-memory map.Suggested fix
chatSessionId: live ? terminalChatSessions.get(row.id) ?? live[1].chatSessionId ?? row.chatSessionId ?? null - : row.chatSessionId ?? terminalChatSessions.get(row.id) ?? null, + : terminalChatSessions.get(row.id) ?? row.chatSessionId ?? null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/pty/ptyService.ts` around lines 2147 - 2149, The non-live branch currently prefers row.chatSessionId over the in-memory binding, which can return stale bindings; update the chatSessionId assignment so it checks terminalChatSessions.get(row.id) before row.chatSessionId (matching the live branch and terminalSessionFromSummary()), i.e. prefer terminalChatSessions.get(row.id) as the first fallback, then row.chatSessionId, and finally null; adjust the same logic used by enrichSessions()/terminalSessionFromSummary() to ensure consistent in-memory-first behavior.apps/desktop/scripts/dev.cjs-259-263 (1)
259-263:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a preflight availability check for the remote debugging port.
Line 259 always forces CDP to a fixed port, but there’s no check that it’s free. If occupied, dev tooling depending on CDP can fail unpredictably.
🔧 Suggested fix
const remoteDebugPort = Number.parseInt(remoteDebugPortRaw, 10); if (!Number.isFinite(remoteDebugPort) || remoteDebugPort <= 0 || remoteDebugPort > 65535) { throw new Error(`Invalid Electron remote debugging port: ${remoteDebugPortRaw}`); } + if (!(await isPortFree(remoteDebugPort))) { + throw new Error( + `Electron remote debugging port ${remoteDebugPort} is already in use. ` + + `Set ADE_APP_CONTROL_CDP_PORT (or ADE_ELECTRON_REMOTE_DEBUGGING_PORT) to a free port.` + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/scripts/dev.cjs` around lines 259 - 263, Add a preflight check to ensure the remote debugging port is free before adding `--remote-debugging-port=${remoteDebugPort}` to `electronArgs`: use a small port-availability probe (for example creating a temporary TCP server or using a get-free-port helper) to detect if `remoteDebugPort` is in use, and if it is either pick an available port and update `remoteDebugPort`/the argument or fail fast with a clear error; update the code that builds `electronArgs` and the `spawnProcess("electron", npxCommand, electronArgs, electronEnv)` call so the actual, verified port is passed to Electron.getting-started/connect-provider.mdx-58-63 (1)
58-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCursor is described as SDK/API-key based but still grouped under “CLI-based providers.”
This section grouping is now inconsistent and can mislead setup choices. Consider renaming the tab/category to include SDK providers.
Proposed doc tweak
- <Tab title="CLI-based providers"> + <Tab title="CLI and SDK-based providers">Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@getting-started/connect-provider.mdx` around lines 58 - 63, The "Cursor" entry is currently placed under the "CLI-based providers" grouping which is misleading because Cursor is SDK/API-key based; update the UI/markdown grouping in connect-provider.mdx so SDK-backed providers are clearly represented — either rename the "CLI-based providers" category to something like "CLI & SDK providers" (update the category header text used around the CardGroup) or move the Cursor Card out of that CardGroup into a new "SDK/API-key providers" section; also apply the same change where the duplicate occurs around line 75 to keep both instances consistent (look for the "Cursor" Card, "CardGroup", and "Note" elements and the literal "CLI-based providers" label).apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx-366-366 (1)
366-366:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace fixed real-time sleep with deterministic timer control.
Line 366 uses a wall-clock wait, which can make this test flaky on slower CI nodes.
Suggested stabilization patch
it("does not stop a service-managed fallback while it is switching backends", async () => { + vi.useFakeTimers(); const { api, emit } = installIosSimulatorApi(); @@ - await new Promise((resolve) => setTimeout(resolve, 700)); + await act(async () => { + await vi.advanceTimersByTimeAsync(700); + }); @@ await waitFor(() => expect(api.startStream).toHaveBeenCalledTimes(1)); expect(screen.queryByText(/Live stream failed/)).toBeNull(); + vi.useRealTimers(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx` at line 366, The test currently uses a real-time sleep via "await new Promise((resolve) => setTimeout(resolve, 700));" which causes flakiness; replace that with deterministic timer control by enabling fake timers (jest.useFakeTimers() or equivalent), trigger the actions that start the timeout, then advance timers with jest.advanceTimersByTime(700) or jest.runOnlyPendingTimers()/jest.runAllTimers(), and finally restore timers with jest.useRealTimers(); update the test to remove the real Promise sleep and use the fake-timer API around the code that relies on the timeout.apps/ios/ADE/Views/Work/WorkModelPickerSheet.swift-265-269 (1)
265-269:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDeduplicate normalized reasoning tiers before rendering pills.
Line 266 lowercases host-provided values, and
reasoningPillslater usesForEach(tiers, id: \.self). If the catalog ever sends duplicates that differ only by case or whitespace, SwiftUI gets duplicate IDs and the pill row becomes unstable.♻️ Suggested fix
private func supportedReasoningTiers(for model: WorkModelOption) -> [String] { - model.reasoningEfforts - .map { $0.effort.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() } - .filter { !$0.isEmpty } + var seen = Set<String>() + return model.reasoningEfforts + .map { $0.effort.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() } + .filter { !$0.isEmpty } + .filter { seen.insert($0).inserted } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ios/ADE/Views/Work/WorkModelPickerSheet.swift` around lines 265 - 269, supportedReasoningTiers(for:) currently normalizes reasoningEfforts but can return duplicate strings (differing only by case/whitespace), causing duplicate IDs in the ForEach used by reasoningPills; update supportedReasoningTiers(for:) to deduplicate the normalized values before returning (preserving original order) so tiers used in ForEach(tiers, id: \.self) are unique and stable.apps/desktop/src/main/services/chat/agentChatService.test.ts-911-917 (1)
911-917:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the title wait helper use a real timeout budget.
This helper only gives the test ~100 zero-delay polls. On slower CI runners, title propagation can legitimately land after that window, which makes all of the new runtime-title tests flaky.
Suggested change
async function waitForSessionTitle(sessionService: ReturnType<typeof createMockSessionService>, sessionId: string, title: string): Promise<void> { - for (let attempt = 0; attempt < 100; attempt += 1) { + const deadline = Date.now() + 2_000; + while (Date.now() < deadline) { if (sessionService.get(sessionId)?.title === title) return; - await new Promise((resolve) => setTimeout(resolve, 0)); + await new Promise((resolve) => setTimeout(resolve, 10)); } throw new Error(`Timed out waiting for session title '${title}'.`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.test.ts` around lines 911 - 917, The helper waitForSessionTitle currently polls 100 times with zero-delay sleeps which can flake on CI; change waitForSessionTitle to use a real timeout budget by tracking elapsed time (e.g., Date.now or performance.now) and looping with a small non-zero delay (e.g., 20–100ms) until elapsed exceeds a configurable maxTimeout (default e.g., 5000ms), returning when sessionService.get(sessionId)?.title matches or throwing a timeout error after the budget; update the function signature to accept an optional timeoutMs parameter if needed and ensure the polling uses the sessionService.get(...) check exactly as before.apps/desktop/src/main/services/chat/agentChatService.test.ts-3340-3413 (1)
3340-3413:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove scheduler-dependent races from the new title tests.
This block still relies on timing luck: Line 3366 uses a fixed 20 ms sleep, and Line 3404 dereferences
mockState.droidPooled.bridgebefore the async hook registration is proven to exist. Both can turn into flaky failures instead of testing the title-adoption behavior.Suggested change
await service.updateSession({ sessionId: session.id, title: "Manual Title", manuallyNamed: true }); mockState.emitCodexPayload({ jsonrpc: "2.0", method: "thread/name/updated", params: { threadId: "thread-1", name: "Should Not Win" }, }); - await new Promise((resolve) => setTimeout(resolve, 20)); + await waitForSessionTitle(sessionService, session.id, "Manual Title"); expect(sessionService.get(session.id)?.title).toBe("Manual Title"); @@ await service.sendMessage({ sessionId: session.id, text: "Use ACP title." }, { awaitDispatch: true }); + for (let attempt = 0; attempt < 100 && !mockState.droidPooled?.bridge.onSessionUpdate; attempt += 1) { + await new Promise((resolve) => setTimeout(resolve, 0)); + } + expect(mockState.droidPooled?.bridge.onSessionUpdate).toBeTypeOf("function"); mockState.droidPooled.bridge.onSessionUpdate?.({ sessionId: "droid-acp-session-1", update: { sessionUpdate: "session_info_update", title: "Droid Native Title",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.test.ts` around lines 3340 - 3413, The tests use fragile timing: replace the fixed sleep after the second Codex thread/name/updated emit with an explicit assertion/wait (e.g., await vi.waitFor or await waitForSessionTitle) that the session title remains "Manual Title" instead of setTimeout(20); and for the Droid test, ensure the mock bridge handler exists before calling onSessionUpdate by awaiting its registration (e.g., await vi.waitFor(() => mockState.droidPooled.bridge?.onSessionUpdate) or awaiting a condition on sessionService) and only then invoke mockState.droidPooled.bridge.onSessionUpdate; update the examples around sendMessage, mockState.emitCodexPayload, waitForSessionTitle, and mockState.droidPooled.bridge.onSessionUpdate accordingly.apps/desktop/src/main/services/chat/agentChatService.ts-1401-1411 (1)
1401-1411:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate Codex tiers against the descriptor too.
This helper is now used for both Claude and Codex, but Line 1408 only enforces
descriptor.reasoningTiersfor Claude. That means a Codex model that advertises a restricted tier set can still accept an unsupported effort here and only get corrected later by the runtime.Suggested fix
function validateReasoningEffortForDescriptor( provider: "codex" | "claude", effort: string | null | undefined, descriptor?: ModelDescriptor | null, ): string | null { const validated = validateReasoningEffort(provider, effort); if (!validated) return null; - if (provider === "claude" && descriptor?.reasoningTiers?.length && !descriptor.reasoningTiers.includes(validated)) { + if (descriptor?.reasoningTiers?.length && !descriptor.reasoningTiers.includes(validated)) { return null; } return validated; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 1401 - 1411, The function validateReasoningEffortForDescriptor improperly only restricts descriptor.reasoningTiers for provider "claude"; update it so that after calling validateReasoningEffort(provider, effort) you check descriptor?.reasoningTiers for both "claude" and "codex" (i.e., any provider using this helper) and return null if the validated tier is not included. Modify validateReasoningEffortForDescriptor (and keep calling validateReasoningEffort) to apply the descriptor.reasoningTiers inclusion check regardless of provider, using the existing descriptor.reasoningTiers?.includes(validated) logic.apps/desktop/src/shared/modelCatalog.ts-352-361 (1)
352-361:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDeduplicate injected OpenCode providers to prevent duplicate provider blocks.
If
opencodeProviderscontains repeated IDs, this loop can push duplicates becauseexistingFamiliesis never updated after insertion.Suggested fix
for (const { id, name } of potentialProviders) { - if (!existingFamilies.has(id)) { + if (!id || existingFamilies.has(id)) { + continue; + } + existingFamilies.add(id); + { providers.push({ key: id, label: PROVIDER_LABELS[id] ?? name, badgeColor: PROVIDER_BADGE_COLORS[id] ?? "#6B7280", subsections: [], modelCount: 0, }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/modelCatalog.ts` around lines 352 - 361, The loop over potentialProviders (involving variables potentialProviders, existingFamilies, and providers) can push duplicate provider entries when opencodeProviders contains repeated ids because existingFamilies is not updated after adding a new provider; fix this by adding the inserted id to existingFamilies (or otherwise marking it) immediately after pushing the new provider so subsequent iterations see it and skip duplicates.
🧹 Nitpick comments (4)
apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts (1)
1545-1549: ⚡ Quick winAlso add
chat.modelCatalogtoIOS_REMOTE_COMMAND_ACTIONSparity coverage.Line 1546 adds runtime coverage for the new action, but the shared iOS action list won’t catch registration drift unless this action is included there too.
✅ Suggested test-list update
const IOS_REMOTE_COMMAND_ACTIONS = [ @@ "chat.models", + "chat.modelCatalog", "chat.listSessions",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts` around lines 1545 - 1549, Add the new action string "chat.modelCatalog" to the IOS_REMOTE_COMMAND_ACTIONS constant so the iOS parity list matches runtime registration; locate the IOS_REMOTE_COMMAND_ACTIONS definition (the shared iOS action list) and append "chat.modelCatalog" to the array/enum that lists permitted remote commands, then run the test suite to confirm parity coverage passes for the new test in syncRemoteCommandService.test.ts which exercises service.execute("chat.modelCatalog", ...).apps/desktop/package.json (1)
190-198: ⚡ Quick winScope the iOS helper payload to macOS packaging.
build.extraResourcesapplies to every target, so Windows artifacts will now shipbuild.sh,sim-capture.swift, andsim-input.meven though only the mac runtime can use them. Moving this fileset under the mac-specific build config keeps unsupported releases smaller and avoids distributing Apple-private helper sources where they can never run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/package.json` around lines 190 - 198, The extraResources entry that currently sits under the global build.extraResources (the object with "from":"native/ios-sim-helpers", "to":"native/ios-sim-helpers", "filter":[...]) should be moved into the mac packaging config so only mac builds include those files; remove that entry from the top-level build.extraResources and add the same mapping under the mac-specific target (e.g., targets.mac or the mac build config block) so files like "build.sh", "sim-capture.swift", and "sim-input.m" are packaged only for macOS.apps/desktop/native/ios-sim-helpers/build.sh (1)
85-88: ⚡ Quick winDon't hide helper signing failures.
If ad-hoc signing fails here, the script still reports success and the packaged app only fails later when it tries to launch the helper. Emitting at least a warning makes those runtime failures diagnosable.
♻️ Suggested adjustment
if command -v codesign >/dev/null 2>&1; then - codesign --force --sign - "$CAPTURE" >/dev/null 2>&1 || true - codesign --force --sign - "$INPUT" >/dev/null 2>&1 || true + if ! codesign --force --sign - "$CAPTURE" >/dev/null 2>&1; then + echo "warning: failed to ad-hoc sign $CAPTURE" >&2 + fi + if ! codesign --force --sign - "$INPUT" >/dev/null 2>&1; then + echo "warning: failed to ad-hoc sign $INPUT" >&2 + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/native/ios-sim-helpers/build.sh` around lines 85 - 88, The codesign calls in build.sh are currently silenced with ">/dev/null 2>&1 || true" so helper signing failures are hidden; change the two lines that run codesign against "$CAPTURE" and "$INPUT" to run without "|| true", capture their exit status (e.g., if ! codesign --force --sign - "$CAPTURE" >/dev/null 2>&1; then echo "Warning: codesign failed for $CAPTURE" >&2; fi) and do the same for "$INPUT"; optionally set a nonfatal flag (e.g., SIGN_WARNED=1) so later packaging can decide to fail or emit a summary, but at minimum print a clear stderr warning including the file variable name when codesign exits nonzero.apps/desktop/src/main/services/ai/aiIntegrationService.test.ts (1)
482-486: ⚡ Quick winHarden verify-response assertions against accidental API-key leakage
These
toMatchObjectchecks will still pass if the response accidentally includes sensitive fields likekey. Add explicit negative assertions (or stricter equality) so leakage fails the tests.Proposed test hardening
- await expect(service.verifyApiKeyConnection("cursor")).resolves.toMatchObject({ - provider: "cursor", - ok: true, - source: "store", - }); + const verifyOkResult = await service.verifyApiKeyConnection("cursor"); + expect(verifyOkResult).toMatchObject({ + provider: "cursor", + ok: true, + source: "store", + }); + expect(verifyOkResult).not.toHaveProperty("key"); @@ - await expect(service.verifyApiKeyConnection("cursor")).resolves.toMatchObject({ - provider: "cursor", - ok: false, - source: "store", - }); + const verifyRejectedResult = await service.verifyApiKeyConnection("cursor"); + expect(verifyRejectedResult).toMatchObject({ + provider: "cursor", + ok: false, + source: "store", + }); + expect(verifyRejectedResult).not.toHaveProperty("key");As per coding guidelines, "Do not store secrets in plaintext project files when an encrypted store already exists."
Also applies to: 524-528
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/aiIntegrationService.test.ts` around lines 482 - 486, The current test for service.verifyApiKeyConnection("cursor") uses toMatchObject which would still pass if the response accidentally includes sensitive fields; update the assertions to explicitly forbid secret fields and/or assert exact equality: after calling service.verifyApiKeyConnection("cursor") assert the result strictly equals the expected object (provider: "cursor", ok: true, source: "store") or add negative assertions like expect(result).not.toHaveProperty("key") and expect(result).not.toHaveProperty("apiKey") (and similarly for "secret" or other sensitive names) for both the check around verifyApiKeyConnection("cursor") and the similar block at the later test (the one covering the same flow around lines 524-528).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/scripts/audit-chat-model-runtime.mjs`:
- Around line 1043-1079: The current ok computation treats empty results as
success; change it to fail when the audit produced zero models or zero cases
(and still check results for non-empty success): after building models, cases
and results, set ok to false if models.length === 0 or cases.length === 0;
otherwise set ok = results.length > 0 && results.every((r) => r.ok). Ensure this
logic is used where ok is computed (the variable named ok in this file) so
list/dry-run/smoke modes correctly return non-zero exit when nothing was
audited.
In `@apps/desktop/src/main/services/ai/aiIntegrationService.ts`:
- Around line 969-983: The current flow calls probeCursorSdkModelDiscovery after
verifyProviderApiKey and treats any probe failure as a verification failure;
wrap the probeCursorSdkModelDiscovery call in a try/catch and only convert the
successful verifyProviderApiKey result into an auth-failure when discovery
exists and discovery.failureKind === "auth"; on any probe timeout/throw or
non-auth discovery result, return the original verification response unchanged.
Keep the existing invalidateProviderReadinessCaches() and
invalidateInFinally=false behavior but ensure probe exceptions do not cause you
to overwrite or reject the successful verification (use apiEntry, providerName,
verifyProviderApiKey, and probeCursorSdkModelDiscovery identifiers to locate the
logic).
In `@apps/desktop/src/main/services/ai/apiKeyStore.ts`:
- Around line 162-181: In writeMacosKeychainSecret the API key is currently
passed on the command line via the "-w value" argument (making it visible to
process inspectors); change the call to runSecurity to pass "-w" "-" (i.e. "-w",
"-") and feed the secret via the child process stdin instead of the argv,
updating runSecurity (or its call site) to accept and write stdin data (the
value string) to the spawned process; keep the existing error handling
(rememberKeychainError/clearKeychainError) but ensure runSecurity returns error
details when its stdin-based invocation fails.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 16903-16926: descriptorInfo currently keys entries only by
patched.id which allows later providers to overwrite earlier ones when the same
descriptor id is surfaced by multiple providers; change the keying to include
the provider (e.g., use a composite key like `${provider.name}:${patched.id}` or
use a nested Map keyed by provider then id) wherever
descriptorInfo.set(patched.id, { provider, info }) is used (also update the
similar set at the other occurrence around lines 16950-16952) and adjust lookups
(descriptorInfo.get(...)) accordingly so each provider+descriptor pair is
preserved and the exported runtimeModelId/provider pair correctly reflects the
original backend.
- Around line 5381-5386: The current logger.info call
("agent_chat.runtime_title_adopted") writes the conversation-derived variable
title verbatim; remove title from structured logs and instead log a
non-sensitive surrogate (e.g., a stable hash of title, a redaction flag, or only
its length/boolean presence) to avoid creating a retention path for PII. Locate
the logger.info("agent_chat.runtime_title_adopted", { sessionId:
managed.session.id, provider: managed.session.provider, source, title }) and
replace the title field with a non-reversible identifier (e.g., titleHash) or a
redacted indicator while preserving sessionId, provider, and source for context.
Ensure any hashing uses a one-way algorithm and that no plaintext title is
written to logs or structured telemetry.
- Around line 2484-2495: On mismatch between requested and runtime Codex effort,
do not overwrite session with requestedReasoningEffort; instead preserve the
runtime-effective value. In the mismatch branch (where validateReasoningEffort,
normalizeReasoningEffort and onReasoningMismatch are used) keep the
onReasoningMismatch call but change managed.session.reasoningEffort =
requestedReasoningEffort to managed.session.reasoningEffort = reasoningEffort
(the runtimeReasoningEffort), and continue to update
managed.session.permissionMode via syncLegacyPermissionMode(managed.session) as
before so ADE state reflects the live thread.
In `@apps/desktop/src/main/services/chat/cursorModelsDiscovery.ts`:
- Around line 321-329: When the warm path (warmCursorModelsFromSdk) catches an
auth error it must persist that failure into the shared discovery failure state
so discoverCursorSdkModelDescriptors (in cached-or-fallback mode) will stop
returning synthetic fallbacks; update the catch handler that currently calls
recordCursorModelDiscoveryFailure(error) and returns [] to also write the error
into the same persistent/visible failure slot that
discoverCursorSdkModelDescriptors consults (or rethrow/resolve a special failure
sentinel that discoverCursorSdkModelDescriptors understands), and mirror the
same change for the second warm-path block (lines ~481-493) so sdkWarmInFlight,
recordCursorModelDiscoveryFailure, and discoverCursorSdkModelDescriptors all
consistently observe auth failures instead of silently returning fallback rows.
In `@apps/desktop/src/main/services/chat/cursorSdkPolicy.ts`:
- Around line 234-250: looksLikePathToken currently rejects Windows absolute
paths (e.g., C:\tmp\file or C:/tmp/file); update the function to detect Windows
drive-letter absolute paths (e.g., a regex like /^[A-Za-z]:[\\/]/) and return
true for them before the checks that require a forward slash and before the
URI-like regex (/^[a-zA-Z][a-zA-Z0-9+.-]*:/) so that C:/... is not misclassified
as a scheme; modify the logic inside looksLikePathToken to run this drive-letter
detection after trimming (trimShellToken) and before the slash/URI checks so
Windows absolute paths are accepted and still pass other existing exclusions
(leading -, env assignments, etc.).
In `@apps/desktop/src/main/services/chat/cursorSdkWorker.ts`:
- Around line 48-85: The fallback branch that consumes record.toJSON() can leak
sensitive fields; restrict errorMessage() to only include the explicit safe
allowlist (code, status, operation, endpoint, requestId) and remove or disable
the generic toJSON() traversal. Edit the function errorMessage to stop iterating
Object.entries(json) and instead only read those same allowed keys from record
or from json if present, ensuring you still trim and stringify values before
adding to detailEntries; keep the existing primary message logic and return
format unchanged.
In `@apps/desktop/src/main/services/ios/iosSimulatorService.ts`:
- Around line 386-398: Update iosSimHelperRoot so packaged apps do not search
launch-directory fallbacks: detect packaged mode (e.g., process.defaultApp is
falsy or NODE_ENV !== 'development') and when packaged exclude candidates that
use process.cwd() or apps/desktop (the entries that join process.cwd() and the
"apps/desktop" path); only allow ADE_IOS_SIM_HELPER_ROOT override and bundled
resource paths in packaged mode so the function iosSimHelperRoot returns null
instead of a CWD-based path, and ensure buildIosurfaceIndigoHelpers reacts to
null by failing cleanly rather than executing build.sh from CWD.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 3227-3231: Wrap the cache invalidation call in a try/catch so
failures are best-effort and do not turn a successful secret mutation into an
IPC error: after calling storeApiKey (in the ipcMain.handle for
IPC.aiStoreApiKey) call
ctx.aiIntegrationService.invalidateProviderReadinessCaches() inside a try block
and catch any error, logging it (e.g., ctx.processLogger.error or console.error)
instead of letting it throw; apply the same try/catch/log pattern to the
corresponding delete handler where invalidateProviderReadinessCaches() is
invoked.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 2445-2453: The code currently swallows errors from
getAgentChatModelsCached(...) and treats probe failures the same as an empty
cursor model list, causing all cursor IDs to be removed via isCursorModelId;
change this so you distinguish an actual empty result from an exception: call
getAgentChatModelsCached and capture errors separately (e.g., let cursorModels;
try { cursorModels = await getAgentChatModelsCached(...) } catch (err) {
cursorModels = null; /* probe failed */ }), then only compute withoutCursor =
orderModelIds(available.filter(id => !isCursorModelId(id))) and call
setAvailableModelIds(withoutCursor) when cursorModels is an explicit empty
array; if cursorModels === null (probe failure) preserve existing cursor IDs in
available (i.e., do not strip cursor IDs) and optionally log the error for
diagnostics.
In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx`:
- Around line 1681-1684: The pending restart timer must be canceled not only on
"stream-started" but also when a "stream-status" event shows the stream
recovered; update the handler that currently calls
cancelDeviceBackedStreamRestart() so it also calls it when event.type ===
"stream-status" and (event.status.streamUrl || event.status.lastFrameAt),
keeping the existing serviceManagedFallback branch; apply the same change to the
other similar handler that schedules/cancels device-backed stream restarts
(i.e., the locations that call scheduleDeviceBackedStreamRestart and
cancelDeviceBackedStreamRestart) so a recovered stream won't be flapped.
In `@apps/desktop/src/shared/types/sync.ts`:
- Line 568: Install the missing dev dependencies and update tsconfig so desktop
typechecks run: add eslint and tsup to devDependencies and run npm/yarn install
(so the tools at eslint/bin/eslint.js and tsup are present), add `@types/node` to
devDependencies and ensure the TypeScript config includes node definitions
(e.g., "types": ["node"] or otherwise references `@types/node`) to fix TS2688, and
set "ignoreDeprecations": "6.0" in tsconfig.json to address the baseUrl
deprecation warning; after these dependency and tsconfig changes, re-run the
standard desktop checks to validate the shared types change in sync.ts (the
change around "chat.modelCatalog").
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 3291-3295: The cachedChatModelCatalog() helper currently returns
cached.catalog without checking TTL; update cachedChatModelCatalog() to compute
the age using Date().timeIntervalSince(cached.fetchedAt) and compare it against
chatModelsCacheTTL, returning nil if the entry is older than the TTL (and
optionally remove the stale entry from chatModelCatalogCache[cacheKey]); use the
same cacheKey logic (chatModelsCacheKey(provider: "catalog")) and the existing
cached structure (cached.fetchedAt / cached.catalog) when implementing the
check.
---
Outside diff comments:
In `@apps/desktop/src/main/services/ai/apiKeyStore.ts`:
- Around line 381-395: storeApiKey and deleteApiKey currently mutate the
in-memory store before performing durable writes; change both to perform
persistence first and only update the in-memory cache on success: in
storeApiKey, trim inputs and validate, then perform the platform write(s) (call
writeMacosKeychainSecret and update provider index via
readMacosKeychainProviderIndex/writeMacosKeychainProviderIndex for macOS, or
call persistEncryptedStore for the encrypted path) using local variables and
without touching ensureStore() result, and only after those calls succeed assign
the key into the result of ensureStore(); apply the analogous approach to
deleteApiKey (perform keychain/index removal or persistEncryptedStore first,
then remove from in-memory store), and ensure any thrown errors leave the
in-memory store unchanged.
In `@apps/desktop/src/main/services/ai/authDetector.ts`:
- Around line 791-833: The success branch in verifyCursorApiKey currently never
clears the prior "auth-failed" state, so add a call to clear the stale Cursor
auth failure before returning on success; locate verifyCursorApiKey and, just
before the successful return in the try block, invoke the existing
runtime-health clearing helper (e.g., clearProviderRuntimeAuthFailure("cursor")
or the counterpart to reportProviderRuntimeAuthFailure) to mark Cursor as
healthy so buildProviderConnections() isn't left downgraded after a later
successful verification.
In `@apps/desktop/src/main/services/ai/providerConnectionStatus.ts`:
- Around line 95-105: The health-state handling updates runtimeAvailable (in
providerConnectionStatus.ts) but fails to mirror that downgrade to
usageAvailable, causing inconsistent status objects; update the branches that
set status.runtimeAvailable = false for health.state values like "auth-failed"
and "runtime-failed" (and the similar block around the other occurrence
referenced) to also set status.usageAvailable = false and ensure when
health.state === "ready" you set status.usageAvailable = true alongside
runtimeAvailable and authAvailable; locate the status object mutations in the
function handling health (look for the health.state checks and the status
variable) and apply the same usageAvailable changes in both places.
In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx`:
- Around line 1715-1725: The code is copying event.status.lastError into
liveVisual.error even when serviceManagedFallback is true, which causes the
reconnect overlay to show the raw transport error; update the setLiveVisual call
used when handling "stream-stopped"/"stream-error" so that inside the updater
you only assign error = event.status.lastError ?? current.error when
serviceManagedFallback is false, otherwise preserve current.error (or set the
service-managed fallback message) so serviceManagedFallback prevents overwriting
liveVisual.error; adjust the setMessage branch accordingly if needed (symbols:
setLiveVisual, liveVisual, event, event.status.lastError,
serviceManagedFallback).
In `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx`:
- Around line 451-467: The verification badge for a provider can remain stale
because verificationByProvider isn't cleared before making IPC calls; modify
verifyApiKey to clear the entry for the target provider (use
setVerificationByProvider to remove or set undefined for [provider]) immediately
after setVerifyingProvider(provider) and before calling
invalidateAiDiscoveryCache()/window.ade.ai.verifyApiKey, so any old
success/failure badge is removed while the new request runs; apply the same
change to the other verification routine covering lines 470-492 (the other
function that also calls setVerificationByProvider/verifyApiKey IPCs) so both
code paths clear the previous verification state up-front.
In `@apps/ios/ADE/Views/Work/WorkModelCatalog.swift`:
- Around line 487-505: The code appends runtime and registry Claude IDs before
adding shorthand aliases, causing mixed canonical/runtime keys to bypass
deduplication in workDeduplicatedModelOptions()/workModelLookupKeys(); change
the order so you first derive and append one canonical Claude equivalence key
using workCanonicalClaudeRegistryId(for: trimmed) (or runtime->canonical mapping
if needed), then append any shorthand aliases like the short names
(sonnet/haiku) afterwards; apply the same canonical-first ordering to the second
Claude block (lines ~510-538) and mirror the pattern for any analogous
Codex/other model mapping code (workCanonicalCodexRegistryId(),
workCodexRuntimeModelId()) so shorthand aliases are only added after a canonical
key is present.
---
Minor comments:
In `@apps/desktop/scripts/dev.cjs`:
- Around line 259-263: Add a preflight check to ensure the remote debugging port
is free before adding `--remote-debugging-port=${remoteDebugPort}` to
`electronArgs`: use a small port-availability probe (for example creating a
temporary TCP server or using a get-free-port helper) to detect if
`remoteDebugPort` is in use, and if it is either pick an available port and
update `remoteDebugPort`/the argument or fail fast with a clear error; update
the code that builds `electronArgs` and the `spawnProcess("electron",
npxCommand, electronArgs, electronEnv)` call so the actual, verified port is
passed to Electron.
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 911-917: The helper waitForSessionTitle currently polls 100 times
with zero-delay sleeps which can flake on CI; change waitForSessionTitle to use
a real timeout budget by tracking elapsed time (e.g., Date.now or
performance.now) and looping with a small non-zero delay (e.g., 20–100ms) until
elapsed exceeds a configurable maxTimeout (default e.g., 5000ms), returning when
sessionService.get(sessionId)?.title matches or throwing a timeout error after
the budget; update the function signature to accept an optional timeoutMs
parameter if needed and ensure the polling uses the sessionService.get(...)
check exactly as before.
- Around line 3340-3413: The tests use fragile timing: replace the fixed sleep
after the second Codex thread/name/updated emit with an explicit assertion/wait
(e.g., await vi.waitFor or await waitForSessionTitle) that the session title
remains "Manual Title" instead of setTimeout(20); and for the Droid test, ensure
the mock bridge handler exists before calling onSessionUpdate by awaiting its
registration (e.g., await vi.waitFor(() =>
mockState.droidPooled.bridge?.onSessionUpdate) or awaiting a condition on
sessionService) and only then invoke
mockState.droidPooled.bridge.onSessionUpdate; update the examples around
sendMessage, mockState.emitCodexPayload, waitForSessionTitle, and
mockState.droidPooled.bridge.onSessionUpdate accordingly.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 1401-1411: The function validateReasoningEffortForDescriptor
improperly only restricts descriptor.reasoningTiers for provider "claude";
update it so that after calling validateReasoningEffort(provider, effort) you
check descriptor?.reasoningTiers for both "claude" and "codex" (i.e., any
provider using this helper) and return null if the validated tier is not
included. Modify validateReasoningEffortForDescriptor (and keep calling
validateReasoningEffort) to apply the descriptor.reasoningTiers inclusion check
regardless of provider, using the existing
descriptor.reasoningTiers?.includes(validated) logic.
In `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 2147-2149: The non-live branch currently prefers row.chatSessionId
over the in-memory binding, which can return stale bindings; update the
chatSessionId assignment so it checks terminalChatSessions.get(row.id) before
row.chatSessionId (matching the live branch and terminalSessionFromSummary()),
i.e. prefer terminalChatSessions.get(row.id) as the first fallback, then
row.chatSessionId, and finally null; adjust the same logic used by
enrichSessions()/terminalSessionFromSummary() to ensure consistent
in-memory-first behavior.
In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx`:
- Line 366: The test currently uses a real-time sleep via "await new
Promise((resolve) => setTimeout(resolve, 700));" which causes flakiness; replace
that with deterministic timer control by enabling fake timers
(jest.useFakeTimers() or equivalent), trigger the actions that start the
timeout, then advance timers with jest.advanceTimersByTime(700) or
jest.runOnlyPendingTimers()/jest.runAllTimers(), and finally restore timers with
jest.useRealTimers(); update the test to remove the real Promise sleep and use
the fake-timer API around the code that relies on the timeout.
In `@apps/desktop/src/shared/modelCatalog.ts`:
- Around line 352-361: The loop over potentialProviders (involving variables
potentialProviders, existingFamilies, and providers) can push duplicate provider
entries when opencodeProviders contains repeated ids because existingFamilies is
not updated after adding a new provider; fix this by adding the inserted id to
existingFamilies (or otherwise marking it) immediately after pushing the new
provider so subsequent iterations see it and skip duplicates.
In `@apps/ios/ADE/Views/Work/WorkModelPickerSheet.swift`:
- Around line 265-269: supportedReasoningTiers(for:) currently normalizes
reasoningEfforts but can return duplicate strings (differing only by
case/whitespace), causing duplicate IDs in the ForEach used by reasoningPills;
update supportedReasoningTiers(for:) to deduplicate the normalized values before
returning (preserving original order) so tiers used in ForEach(tiers, id:
\.self) are unique and stable.
In `@getting-started/connect-provider.mdx`:
- Around line 58-63: The "Cursor" entry is currently placed under the "CLI-based
providers" grouping which is misleading because Cursor is SDK/API-key based;
update the UI/markdown grouping in connect-provider.mdx so SDK-backed providers
are clearly represented — either rename the "CLI-based providers" category to
something like "CLI & SDK providers" (update the category header text used
around the CardGroup) or move the Cursor Card out of that CardGroup into a new
"SDK/API-key providers" section; also apply the same change where the duplicate
occurs around line 75 to keep both instances consistent (look for the "Cursor"
Card, "CardGroup", and "Note" elements and the literal "CLI-based providers"
label).
---
Nitpick comments:
In `@apps/desktop/native/ios-sim-helpers/build.sh`:
- Around line 85-88: The codesign calls in build.sh are currently silenced with
">/dev/null 2>&1 || true" so helper signing failures are hidden; change the two
lines that run codesign against "$CAPTURE" and "$INPUT" to run without "||
true", capture their exit status (e.g., if ! codesign --force --sign -
"$CAPTURE" >/dev/null 2>&1; then echo "Warning: codesign failed for $CAPTURE"
>&2; fi) and do the same for "$INPUT"; optionally set a nonfatal flag (e.g.,
SIGN_WARNED=1) so later packaging can decide to fail or emit a summary, but at
minimum print a clear stderr warning including the file variable name when
codesign exits nonzero.
In `@apps/desktop/package.json`:
- Around line 190-198: The extraResources entry that currently sits under the
global build.extraResources (the object with "from":"native/ios-sim-helpers",
"to":"native/ios-sim-helpers", "filter":[...]) should be moved into the mac
packaging config so only mac builds include those files; remove that entry from
the top-level build.extraResources and add the same mapping under the
mac-specific target (e.g., targets.mac or the mac build config block) so files
like "build.sh", "sim-capture.swift", and "sim-input.m" are packaged only for
macOS.
In `@apps/desktop/src/main/services/ai/aiIntegrationService.test.ts`:
- Around line 482-486: The current test for
service.verifyApiKeyConnection("cursor") uses toMatchObject which would still
pass if the response accidentally includes sensitive fields; update the
assertions to explicitly forbid secret fields and/or assert exact equality:
after calling service.verifyApiKeyConnection("cursor") assert the result
strictly equals the expected object (provider: "cursor", ok: true, source:
"store") or add negative assertions like
expect(result).not.toHaveProperty("key") and
expect(result).not.toHaveProperty("apiKey") (and similarly for "secret" or other
sensitive names) for both the check around verifyApiKeyConnection("cursor") and
the similar block at the later test (the one covering the same flow around lines
524-528).
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts`:
- Around line 1545-1549: Add the new action string "chat.modelCatalog" to the
IOS_REMOTE_COMMAND_ACTIONS constant so the iOS parity list matches runtime
registration; locate the IOS_REMOTE_COMMAND_ACTIONS definition (the shared iOS
action list) and append "chat.modelCatalog" to the array/enum that lists
permitted remote commands, then run the test suite to confirm parity coverage
passes for the new test in syncRemoteCommandService.test.ts which exercises
service.execute("chat.modelCatalog", ...).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| function writeMacosKeychainSecret(account: string, value: string): void { | ||
| if (!isMacosKeychainAvailable()) { | ||
| throw new Error("macOS Keychain is unavailable. Cannot persist API keys."); | ||
| } | ||
| const result = runSecurity([ | ||
| "add-generic-password", | ||
| "-a", | ||
| account, | ||
| "-s", | ||
| MACOS_KEYCHAIN_SERVICE, | ||
| "-w", | ||
| value, | ||
| "-U", | ||
| ]); | ||
| if (!result.ok) { | ||
| rememberKeychainError("write", result); | ||
| throw new Error(macosKeychainError ?? "macOS Keychain write failed."); | ||
| } | ||
| clearKeychainError(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the implementation of runSecurity in the file
rg -A 20 "function runSecurity|const runSecurity" apps/desktop/src/main/services/ai/apiKeyStore.tsRepository: arul28/ADE
Length of output: 932
🏁 Script executed:
# Check for imports and encrypted-store usage in the file
rg "import|encrypted" apps/desktop/src/main/services/ai/apiKeyStore.tsRepository: arul28/ADE
Length of output: 1066
🏁 Script executed:
# Search for how storeApiKey and deleteApiKey work
rg -A 10 "function storeApiKey|function deleteApiKey|export.*storeApiKey|export.*deleteApiKey" apps/desktop/src/main/services/ai/apiKeyStore.tsRepository: arul28/ADE
Length of output: 1050
🏁 Script executed:
# Check if there are alternatives like stdin being used elsewhere in the codebase
rg -r "stdin|spawnSync.*password|security.*stdin" apps/desktop/src --type tsRepository: arul28/ADE
Length of output: 11793
🏁 Script executed:
# Search for encrypted-store usage in the codebase
fd -e ts -e tsx | xargs rg "encrypted-store|encryptedStore" | head -20Repository: arul28/ADE
Length of output: 1408
Don't pass the API key as a command-line argument to the security command.
Line 173 passes the raw key via -w value, making the secret observable through process inspection (e.g., ps, /proc/[pid]/cmdline) while the child process runs. Use stdin instead: the security command accepts -w - to read the password from stdin, eliminating plaintext exposure on the command line.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/ai/apiKeyStore.ts` around lines 162 - 181, In
writeMacosKeychainSecret the API key is currently passed on the command line via
the "-w value" argument (making it visible to process inspectors); change the
call to runSecurity to pass "-w" "-" (i.e. "-w", "-") and feed the secret via
the child process stdin instead of the argv, updating runSecurity (or its call
site) to accept and write stdin data (the value string) to the spawned process;
keep the existing error handling (rememberKeychainError/clearKeychainError) but
ensure runSecurity returns error details when its stdin-based invocation fails.
|
@copilot review but do not make fixes |
Here's my review of the follow-up commit ( What was addressed
Remaining / new issues1.
|
|
@copilot review but do not make fixes |
Here's my review of the third commit ( What was addressed
Remaining / new issues1.
|
arul28
force-pushed
the
ade/ios-fixes-13925e5f
branch
from
f7c957e to
f037e86
Compare
|
@copilot review but do not make fixes |
Here's my review of the PR in its current state (latest commit What Greptile raisedP1 —
|
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/chat/agentChatService.test.ts (1)
1-10506:⚠️ Potential issue | 🟠 MajorRun desktop validation and fix blocking errors before merge
Run:
npm --prefix apps/desktop install
npm --prefix apps/desktop run typecheck
npm --prefix apps/desktop run lint
npm --prefix apps/desktop run build
npm --prefix apps/desktop run testObserved blocking issues:
- tsconfig.json(7,5): TS5101 (baseUrl deprecated) and missing 'node' type definitions (TS2688).
- Dev tools missing: tsup and vitest reported "not found".
- ESLint startup/module resolution errors and many lint warnings surfaced.
Required fixes:
- Install devDependencies (npm --prefix apps/desktop install) or run in CI with deps provisioned.
- Add/verify
@types/nodeor adjust tsconfig to resolve TS2688/TS5101.- Ensure tsup, vitest, eslint and other tooling are available.
- Re-run the listed commands and merge only after all checks complete successfully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.test.ts` around lines 1 - 10506, The CI/dev errors are caused by missing dev tooling and Node types causing TS2688/TS5101; to fix, add the required devDependencies (tsup, vitest, eslint, `@types/node`, etc.) into package.json devDependencies and run npm --prefix apps/desktop install so tests/linters/build tools are available, update tsconfig.json to include Node typings (e.g. add "types": ["node"] or ensure "typeRoots" includes node_modules/@types) and remove or correct the deprecated baseUrl setting so the compilerOptions are valid, then re-run npm --prefix apps/desktop run typecheck, lint, build and test to confirm createAgentChatService/startOpenCodeSession/unstable_v2_createSession tests run cleanly before merging.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (1)
2439-2454: ⚡ Quick winApply ordering before the first state write.
setAvailableModelIds(available)on Line 2440 bypassesorderModelIds, so the early-return branches (Cursor not ready / probe error) return a differently ordered list than the success branches. Use one ordered value across all paths to keep picker ordering stable.Suggested patch
- const available = deriveConfiguredModelIds(status, { includeDroid: true }); - setAvailableModelIds(available); + const available = deriveConfiguredModelIds(status, { includeDroid: true }); + const orderedAvailable = orderModelIds(available); + setAvailableModelIds(orderedAvailable); const cursorReady = status.availableProviders?.cursor === true || status.providerConnections?.cursor?.runtimeAvailable === true; - if (!cursorReady) return available; + if (!cursorReady) return orderedAvailable; let cursorModels: Awaited<ReturnType<typeof getAgentChatModelsCached>>; try { cursorModels = await getAgentChatModelsCached({ projectRoot, provider: "cursor", activateRuntime: true, }); } catch { - return available; + return orderedAvailable; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx` around lines 2439 - 2454, The code calls setAvailableModelIds(available) before ordering, causing early-return branches to return an un-ordered list; compute a single ordered list by calling orderModelIds on the result of deriveConfiguredModelIds (use the same ordered value for both setAvailableModelIds and any returns), then proceed to check cursorReady and call getAgentChatModelsCached; update usages of available to the ordered variable so picker ordering is consistent across the cursor-ready, not-ready, and error paths (refer to deriveConfiguredModelIds, orderModelIds, setAvailableModelIds, cursorReady, and getAgentChatModelsCached).apps/desktop/src/main/services/chat/agentChatService.test.ts (1)
3316-3318: ⚡ Quick winReplace fixed-delay negative assertion with deterministic dispatch completion
Using a hardcoded
20mssleep here can make the test timing-sensitive on slower CI. Prefer awaiting deterministic dispatch completion and asserting call count directly.Suggested change
- await service.sendMessage({ + await service.sendMessage({ sessionId: session.id, text: "/review", - }); + }, { awaitDispatch: true }); @@ - await new Promise((resolve) => setTimeout(resolve, 20)); - expect(aiIntegrationService.summarizeTerminal).not.toHaveBeenCalled(); + expect(aiIntegrationService.summarizeTerminal).toHaveBeenCalledTimes(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/agentChatService.test.ts` around lines 3316 - 3318, Remove the hardcoded 20ms sleep and instead wait deterministically for all pending dispatches/async microtasks to complete before asserting; for example, replace the new Promise sleep with awaiting the relevant dispatch/promise that would trigger summarization (or call a helper like flushPromises() / await waitFor(() => {/* no-op */}) or advance fake timers via jest.runAllTimers() if using fake timers) so that aiIntegrationService.summarizeTerminal is asserted with expect(aiIntegrationService.summarizeTerminal).not.toHaveBeenCalled() only after all work has completed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/scripts/audit-chat-model-runtime.mjs`:
- Around line 291-303: buildDescriptorIndexes currently stores
descriptor.shortId and descriptor.aliases into the global byId map which allows
the same alias/shortId from different providers to collide; update the function
to namespace shortId and aliases by the resolved provider (use the same provider
key used for byProviderModelId) before set'ing into byId (e.g., store
`${provider}:${shortId}` and `${provider}:${alias}`), and apply the identical
change to the other index-building loop that mirrors lines 329-343 so lookups
are provider-scoped and cannot cross-resolve across providers; use
registry.resolveProviderGroupForModel(descriptor) to obtain the provider key for
each descriptor when namespacing.
- Around line 67-73: The cdpPort calculation currently parses the entire
fallback expression so a non-numeric ADE_MODEL_AUDIT_CDP_PORT yields NaN and
prevents fallback; change this to try each env var in order
(ADE_MODEL_AUDIT_CDP_PORT, ADE_ELECTRON_REMOTE_DEBUGGING_PORT,
ADE_APP_CONTROL_CDP_PORT) by parsing and validating each candidate individually
(Number.parseInt and then Number.isFinite/Number.isInteger and optional
port-range check 1–65535), return the first valid integer, otherwise use the
default 9222; apply the same fix to the duplicate logic referenced around the
cdpPort usage at the other occurrence (lines 215–217) so both places robustly
validate env-derived ports.
In `@apps/desktop/scripts/dev.cjs`:
- Around line 262-265: The current electronArgs array places flags after the app
path so --remote-debugging-port and macOS -ApplePersistenceIgnoreState are
treated as app args; update the construction of electronArgs to put flags before
the app path by creating electronArgs as ["electron",
`--remote-debugging-port=${remoteDebugPort}`, /* mac flags if any */, "."] and
when process.platform === "darwin" push "-ApplePersistenceIgnoreState" and "YES"
into electronArgs before appending the "." element (i.e., modify the
electronArgs creation and the process.platform branch around the electronArgs
variable to ensure flags are inserted prior to ".").
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 1423-1433: The current resolveCodexReasoningEffortForRuntime
unconditionally returns DEFAULT_REASONING_EFFORT even if
validateReasoningEffortForDescriptor previously rejected values for the provided
descriptor, which can reintroduce an unsupported tier; update the function (and
the analogous code at lines ~10783-10787) to run
validateReasoningEffortForDescriptor against DEFAULT_REASONING_EFFORT (after
normalizeReasoningEffort) and only return DEFAULT_REASONING_EFFORT if that
validation succeeds, otherwise return null/undefined (or propagate failure) so
an unsupported value is never sent to runtime; reference functions/values:
resolveCodexReasoningEffortForRuntime, validateReasoningEffortForDescriptor,
normalizeReasoningEffort, DEFAULT_REASONING_EFFORT, and
descriptor.reasoningTiers.
- Around line 1883-1887: The current authentication-detection expression (the
return that checks message.includes(...)) is too broad because matching any
occurrence of "api key" misclassifies unrelated errors; update the logic that
evaluates the message variable so it requires whole-word matches and an
authentication context (e.g., use case-insensitive regex with word boundaries
and require nearby auth qualifiers like "invalid", "missing", "not provided",
"revoked", "not found", "unauthorized", or "forbidden"); replace the simple
message.includes checks with combined regex tests such as /\b(api[- ]key)\b/i
together with patterns like /(invalid|missing|not provided|revoked|not found)/i
(or a single compound regex) so only true auth-related texts trigger the
auth-failure path.
In `@apps/desktop/src/main/services/chat/cursorModelsDiscovery.ts`:
- Around line 328-340: The probe function currently short-circuits and returns
cachedRows from getCachedCursorSdkModels(apiKey), which prevents a live
verification; change probeCursorSdkModelDiscovery to require explicit opt-in to
return cached results by adding an options flag (e.g., options.allowCached or
options.useCache defaulting to false) and only immediately return cachedRows
when that flag is true; otherwise always continue with the live SDK/API
discovery logic (keep getCachedCursorSdkModels, the new options flag, and the
rest of the function intact) so callers that need a fresh probe get a real
network check.
In `@apps/desktop/src/main/services/chat/cursorSdkHooks.ts`:
- Around line 150-156: The guard in main() incorrectly falls back to allow() on
any error even when a socket was explicitly provided; update the logic so that
the presence of socketPath (from parseArg("--socket") or ADE_CURSOR_SDK_SOCKET)
is treated as an explicit launch and must never result in allow() on failure.
Concretely, in the top-level check in main() (symbols: main, socketPath,
adeSession, allow(), deny()), change the branch so that if socketPath is truthy
and something goes wrong you call deny("ADE Cursor policy gate is unavailable.")
(or rethrow) instead of allow(); apply the same change to the analogous
error/fallback handling around the later block referenced (lines ~212-219) so
explicit --socket always yields deny on failure.
- Around line 239-242: The ADE gate is appended last currently; change
construction of nextPreToolUse so adeEntry is first and existing ADE entries are
removed: create nextPreToolUse = [adeEntry, ...existingPreToolUse.filter((entry)
=> !isAdeHookEntry(entry))] (use the same isAdeHookEntry predicate and adeEntry
symbol to avoid duplicates) so the ADE hook runs before any other preToolUse
hooks.
In `@apps/desktop/src/main/services/chat/cursorSdkPolicy.ts`:
- Around line 275-279: The collectPotentialPaths function currently only adds
the entire trimmed string when pathContext is true, which lets raw shell command
strings bypass lane guards; update collectPotentialPaths (and the same fallback
logic used later) to parse raw string shell inputs into tokens before pushing
paths: if pathContext && value is string, run a simple shell-like tokenizer that
splits on whitespace and common shell operators (&&, ||, ;, |, >, <) and extract
path-looking tokens (tokens starting with /, ./, ../, or containing a slash)
into out instead of the whole string; apply the same tokenization logic to the
fallback handling used later so plain command strings like "cd /etc && cat
/etc/passwd" yield the individual path tokens for hardGuard checks.
In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx`:
- Around line 1367-1382: The current early-return treats !nextStatus.running
like a soft fallback and can swallow hard startup failures when lastError is
null; change the condition so the reconnect branch only triggers when
nextStatus.degradationReason, nextStatus.fallbackReason, or nextStatus.lastError
are present (i.e., require a non-null error/reason) rather than triggering on
!nextStatus.running alone; keep the setLiveVisual/setMessage/refreshStatus
behavior for true reconnect cases but allow execution to continue (so the
restart effect that watches lastError can schedule recovery) when the service is
not running but there is no lastError.
---
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 1-10506: The CI/dev errors are caused by missing dev tooling and
Node types causing TS2688/TS5101; to fix, add the required devDependencies
(tsup, vitest, eslint, `@types/node`, etc.) into package.json devDependencies and
run npm --prefix apps/desktop install so tests/linters/build tools are
available, update tsconfig.json to include Node typings (e.g. add "types":
["node"] or ensure "typeRoots" includes node_modules/@types) and remove or
correct the deprecated baseUrl setting so the compilerOptions are valid, then
re-run npm --prefix apps/desktop run typecheck, lint, build and test to confirm
createAgentChatService/startOpenCodeSession/unstable_v2_createSession tests run
cleanly before merging.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 3316-3318: Remove the hardcoded 20ms sleep and instead wait
deterministically for all pending dispatches/async microtasks to complete before
asserting; for example, replace the new Promise sleep with awaiting the relevant
dispatch/promise that would trigger summarization (or call a helper like
flushPromises() / await waitFor(() => {/* no-op */}) or advance fake timers via
jest.runAllTimers() if using fake timers) so that
aiIntegrationService.summarizeTerminal is asserted with
expect(aiIntegrationService.summarizeTerminal).not.toHaveBeenCalled() only after
all work has completed.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 2439-2454: The code calls setAvailableModelIds(available) before
ordering, causing early-return branches to return an un-ordered list; compute a
single ordered list by calling orderModelIds on the result of
deriveConfiguredModelIds (use the same ordered value for both
setAvailableModelIds and any returns), then proceed to check cursorReady and
call getAgentChatModelsCached; update usages of available to the ordered
variable so picker ordering is consistent across the cursor-ready, not-ready,
and error paths (refer to deriveConfiguredModelIds, orderModelIds,
setAvailableModelIds, cursorReady, and getAgentChatModelsCached).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30384145-4ee9-4519-8978-3dc4a1b25a28
📒 Files selected for processing (33)
apps/desktop/native/ios-sim-helpers/build.shapps/desktop/package.jsonapps/desktop/scripts/audit-chat-model-runtime.mjsapps/desktop/scripts/dev.cjsapps/desktop/src/main/services/ai/aiIntegrationService.test.tsapps/desktop/src/main/services/ai/aiIntegrationService.tsapps/desktop/src/main/services/ai/apiKeyStore.test.tsapps/desktop/src/main/services/ai/apiKeyStore.tsapps/desktop/src/main/services/ai/authDetector.tsapps/desktop/src/main/services/ai/providerConnectionStatus.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/cursorModelsDiscovery.test.tsapps/desktop/src/main/services/chat/cursorModelsDiscovery.tsapps/desktop/src/main/services/chat/cursorSdkHooks.test.tsapps/desktop/src/main/services/chat/cursorSdkHooks.tsapps/desktop/src/main/services/chat/cursorSdkPolicy.test.tsapps/desktop/src/main/services/chat/cursorSdkPolicy.tsapps/desktop/src/main/services/chat/cursorSdkPool.tsapps/desktop/src/main/services/chat/cursorSdkWorker.tsapps/desktop/src/main/services/ios/iosSimulatorService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.test.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.tsxapps/desktop/src/shared/modelCatalog.tsapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkModelCatalog.swiftapps/ios/ADE/Views/Work/WorkModelPickerSheet.swiftgetting-started/connect-provider.mdx
✅ Files skipped from review due to trivial changes (3)
- apps/desktop/src/renderer/components/settings/ProvidersSection.test.tsx
- apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts
- apps/ios/ADE/Views/Work/WorkModelCatalog.swift
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/desktop/src/main/services/ai/authDetector.ts
- apps/desktop/package.json
- apps/desktop/native/ios-sim-helpers/build.sh
- getting-started/connect-provider.mdx
- apps/desktop/src/main/services/chat/cursorSdkHooks.test.ts
- apps/desktop/src/main/services/chat/cursorSdkPolicy.test.ts
- apps/desktop/src/renderer/components/settings/ProvidersSection.tsx
- apps/desktop/src/main/services/chat/cursorModelsDiscovery.test.ts
- apps/desktop/src/main/services/ipc/registerIpc.ts
- apps/ios/ADE/Views/Work/WorkModelPickerSheet.swift
|
@copilot review but do not make fixes |
|
@copilot review but do not make fixes |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/components/settings/ProvidersSection.tsx (1)
451-473:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
verifyingProviderset until the forced refresh completes.Both verification paths clear
verifyingProviderbeforeawait refreshStatus(...), which re-enables the action buttons while the post-verify status refresh is still in flight. That makes it easy to race a second verify/edit/delete against the refresh and briefly show stale connection state. Let the existingfinallyblock own the reset.Suggested fix
setVerificationByProvider((prev) => ({ ...prev, [provider]: result })); setNotice(result.ok ? `${provider} connection verified.` : `${provider} verification failed.`); - setVerifyingProvider(null); await refreshStatus({ force: true, refreshOpenCodeInventory: true }); @@ invalidateAiDiscoveryCache(); setVerificationByProvider((prev) => ({ ...prev, cursor: result })); setNotice(result.ok ? "Cursor connection verified." : "Cursor verification failed."); - setVerifyingProvider(null); await refreshStatus({ force: true, refreshOpenCodeInventory: true });Also applies to: 475-503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx` around lines 451 - 473, The code clears verifyingProvider early in the success path which re-enables UI before refreshStatus completes; in verifyApiKey remove the in-try call to setVerifyingProvider(null) so the finally block is the single place that resets it, and keep the await refreshStatus({ force: true, refreshOpenCodeInventory: true }) before the finally resets; apply the same change to the sibling function block around lines 475-503 (the other verification handler) so both verification paths rely on the finally to clear setVerifyingProvider.
🧹 Nitpick comments (2)
apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts (1)
133-147: 💤 Low valueLGTM — the assertion correctly targets the
title: null → body: {}path.The
expect.objectContaining({ body: {} })matcher is appropriate: the outerobjectContainingtolerates thequerysibling property, andbody: {}deep-equals the empty object the production ternary produces whentrimToUndefined(null)evaluates toundefined.One optional coverage gap worth noting: a whitespace-only
title(e.g.," ") is also collapsed toundefinedbytrimToUndefinedand therefore also omits the title field. Adding a second case for that input would give stronger confidence that the trim-then-omit path is fully exercised.🔬 Optional: extend the test to cover the whitespace-only title edge case
it("omits the session title when ADE wants OpenCode to auto-name", async () => { - await startOpenCodeSession({ + // null title + await startOpenCodeSession({ directory: "/repo", title: null, leaseKind: "shared", projectConfig: { ai: {} }, ownerKind: "chat", ownerId: "chat-1", ownerKey: "chat:chat-1", }); expect(mockState.createSession).toHaveBeenCalledWith(expect.objectContaining({ body: {}, })); + + vi.clearAllMocks(); + + // whitespace-only title is also auto-named + await startOpenCodeSession({ + directory: "/repo", + title: " ", + leaseKind: "shared", + projectConfig: { ai: {} }, + ownerKind: "chat", + ownerId: "chat-2", + ownerKey: "chat:chat-2", + }); + + expect(mockState.createSession).toHaveBeenCalledWith(expect.objectContaining({ + body: {}, + })); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts` around lines 133 - 147, Test currently verifies null title path; add a second assertion that a whitespace-only title also collapses and is omitted: call startOpenCodeSession with title: " " (or similar whitespace) and the same other args, then assert mockState.createSession was called with expect.objectContaining({ body: {} }) to ensure trimToUndefined behavior is exercised for whitespace-only strings; locate the test block using startOpenCodeSession and mockState.createSession to add this case.apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx (1)
366-366: 💤 Low value700 ms negative-assertion window is shorter than peer tests and may be flaky on slow CI runners.
The equivalent waits in this file are 1 600 ms (line 415) and 2 500 ms (line 447). If the component has any debounce or async retry logic with a boundary near 700 ms, the assertion on line 368 could pass spuriously. Either use
vi.useFakeTimers()for deterministic control or increase the delay to match the longest relevant component timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx` at line 366, The 700ms hard wait in the test is too short and can be flaky; update the test in ChatIosSimulatorPanel.test.tsx to either switch to deterministic timers by calling vi.useFakeTimers()/vi.runAllTimers() around the async debounce/timeout behavior (and restore timers after), or simply increase the await delay to match the longest peer wait (e.g., 1600–2500ms) so the negative assertion after the Promise settles is not spuriously passing; locate the Promise await (await new Promise((resolve) => setTimeout(resolve, 700));) and apply one of these fixes to ensure stability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/scripts/audit-chat-model-runtime.mjs`:
- Around line 864-866: The check in verifySessionFields() only runs when
expected.modelId is truthy, so it misses cases where expected.modelId is
explicitly null; change the condition to detect the presence of the expectation
(e.g., use 'modelId' in expected or
Object.prototype.hasOwnProperty.call(expected, "modelId")) and then push an
issue if actualModelId !== expected.modelId (so null vs non-null mismatches are
reported). Update the block referencing label, expected, actualModelId and
issues accordingly.
- Around line 1001-1005: The human summary in printHumanSummary currently marks
the audit "ok" solely when failed.length === 0, causing empty runs to appear
successful; update printHumanSummary to use the same success predicate as main
by treating the run as OK only when there are models, cases, and results
exercised and no failures (e.g., compute ok = models.length > 0 && cases.length
> 0 && results.length > 0 && failed.length === 0) and print "ok" or "failed"
based on that predicate instead of just failed.length.
In `@apps/desktop/src/main/services/chat/cursorModelsDiscovery.ts`:
- Around line 246-285: The function fetchCursorModelsFromSdk currently treats
any successful REST fallback as a full success and calls
recordCursorModelDiscoverySuccess(rows), which promotes provider runtime health
even if the packaged SDK (Cursor.models.list) failed; change the logic so that
only a successful SDK call (i.e., when sdkError is null and the SDK returned
sdkRows) triggers the runtime-ready/reporting path
(recordCursorModelDiscoverySuccess / reportProviderRuntimeReady("cursor")),
while still returning/using apiRows from fetchCursorModelsFromOfficialApi for
catalog population and caching behavior; update related state changes
(sdkCached, sdkLastFailure) and the placement of
recordCursorModelDiscoverySuccess so they only run when the SDK call succeeded
(reference symbols: fetchCursorModelsFromSdk, Cursor.models.list,
fetchCursorModelsFromOfficialApi, sdkError, sdkRows, apiRows, sdkCached,
sdkLastFailure, sdkCacheGeneration, recordCursorModelDiscoverySuccess,
reportProviderRuntimeReady).
In `@apps/desktop/src/main/services/chat/cursorSdkPool.ts`:
- Around line 442-459: cleanupCursorSdkStateRoot currently only removes the
per-call stateRoot, leaving behind the per-request cache/socket directories and
leaking filesystem entries; update the pool entry stored in pools.set to include
the per-request cacheRoot (and any socket dir if separately tracked), then
change cleanupCursorSdkStateRoot(entry: { stateRoot: string; cacheRoot?: string;
cleanupStateRoot: boolean }) to, when cleanupStateRoot is true, remove both
entry.stateRoot and entry.cacheRoot (using fs.rmSync with { recursive: true,
force: true }) inside the try/catch so failures remain best-effort; ensure
pools.set(...) sets cacheRoot when creating one-shot pools so cleanup has the
value to delete.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx`:
- Around line 451-473: The code clears verifyingProvider early in the success
path which re-enables UI before refreshStatus completes; in verifyApiKey remove
the in-try call to setVerifyingProvider(null) so the finally block is the single
place that resets it, and keep the await refreshStatus({ force: true,
refreshOpenCodeInventory: true }) before the finally resets; apply the same
change to the sibling function block around lines 475-503 (the other
verification handler) so both verification paths rely on the finally to clear
setVerifyingProvider.
---
Nitpick comments:
In `@apps/desktop/src/main/services/opencode/openCodeRuntime.test.ts`:
- Around line 133-147: Test currently verifies null title path; add a second
assertion that a whitespace-only title also collapses and is omitted: call
startOpenCodeSession with title: " " (or similar whitespace) and the same
other args, then assert mockState.createSession was called with
expect.objectContaining({ body: {} }) to ensure trimToUndefined behavior is
exercised for whitespace-only strings; locate the test block using
startOpenCodeSession and mockState.createSession to add this case.
In `@apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsx`:
- Line 366: The 700ms hard wait in the test is too short and can be flaky;
update the test in ChatIosSimulatorPanel.test.tsx to either switch to
deterministic timers by calling vi.useFakeTimers()/vi.runAllTimers() around the
async debounce/timeout behavior (and restore timers after), or simply increase
the await delay to match the longest peer wait (e.g., 1600–2500ms) so the
negative assertion after the Promise settles is not spuriously passing; locate
the Promise await (await new Promise((resolve) => setTimeout(resolve, 700));)
and apply one of these fixes to ensure stability.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10be0198-5fa4-4e18-af56-055df8327b1a
⛔ Files ignored due to path filters (9)
changelog/v1.0.11.mdxis excluded by!changelog/**changelog/v1.0.19.mdxis excluded by!changelog/**changelog/v1.1.7.mdxis excluded by!changelog/**changelog/v1.1.8.mdxis excluded by!changelog/**docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/ios-simulator/README.mdis excluded by!docs/**docs/features/memory/compaction.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-processes.mdis excluded by!docs/**
📒 Files selected for processing (70)
ai-tools/cursor.mdxai-tools/windsurf.mdxapps/desktop/native/ios-sim-helpers/README.mdapps/desktop/native/ios-sim-helpers/build.shapps/desktop/package.jsonapps/desktop/scripts/after-pack-runtime-fixes.cjsapps/desktop/scripts/audit-chat-model-runtime.mjsapps/desktop/scripts/dev.cjsapps/desktop/scripts/validate-mac-artifacts.mjsapps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/aiIntegrationService.test.tsapps/desktop/src/main/services/ai/aiIntegrationService.tsapps/desktop/src/main/services/ai/apiKeyStore.test.tsapps/desktop/src/main/services/ai/apiKeyStore.tsapps/desktop/src/main/services/ai/authDetector.tsapps/desktop/src/main/services/ai/providerConnectionStatus.test.tsapps/desktop/src/main/services/ai/providerConnectionStatus.tsapps/desktop/src/main/services/ai/providerRuntimeHealth.tsapps/desktop/src/main/services/ai/tools/systemPrompt.test.tsapps/desktop/src/main/services/ai/tools/systemPrompt.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/cursorModelsDiscovery.test.tsapps/desktop/src/main/services/chat/cursorModelsDiscovery.tsapps/desktop/src/main/services/chat/cursorSdkHooks.test.tsapps/desktop/src/main/services/chat/cursorSdkHooks.tsapps/desktop/src/main/services/chat/cursorSdkPolicy.test.tsapps/desktop/src/main/services/chat/cursorSdkPolicy.tsapps/desktop/src/main/services/chat/cursorSdkPool.test.tsapps/desktop/src/main/services/chat/cursorSdkPool.tsapps/desktop/src/main/services/chat/cursorSdkProtocol.tsapps/desktop/src/main/services/chat/cursorSdkWorker.tsapps/desktop/src/main/services/ios/iosSimulatorService.test.tsapps/desktop/src/main/services/ios/iosSimulatorService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/opencode/openCodeRuntime.test.tsapps/desktop/src/main/services/opencode/openCodeRuntime.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.test.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.test.tsxapps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.test.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.tsxapps/desktop/src/renderer/components/shared/providerModelSelectorGrouping.tsapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.test.tsxapps/desktop/src/renderer/lib/sessions.tsapps/desktop/src/shared/modelCatalog.tsapps/desktop/src/shared/modelProfiles.test.tsapps/desktop/src/shared/modelProfiles.tsapps/desktop/src/shared/modelRegistry.test.tsapps/desktop/src/shared/modelRegistry.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/config.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkModelCatalog.swiftapps/ios/ADE/Views/Work/WorkModelPickerSheet.swiftapps/ios/ADE/Views/Work/WorkNewChatSheet.swiftchat/overview.mdxgetting-started/connect-provider.mdxgetting-started/open-project.mdxplans/cursor-sdk-chat-integration.md
💤 Files with no reviewable changes (1)
- apps/desktop/src/shared/modelProfiles.ts
✅ Files skipped from review due to trivial changes (23)
- apps/desktop/src/renderer/components/terminals/SessionCard.tsx
- apps/desktop/src/shared/types/config.ts
- apps/desktop/src/main/services/ai/tools/systemPrompt.ts
- ai-tools/windsurf.mdx
- apps/desktop/src/renderer/components/settings/ProvidersSection.test.tsx
- apps/desktop/src/main/services/chat/cursorSdkPool.test.ts
- getting-started/connect-provider.mdx
- chat/overview.mdx
- apps/desktop/src/main/main.ts
- apps/desktop/src/main/services/sync/syncRemoteCommandService.ts
- apps/desktop/src/renderer/components/shared/providerModelSelectorGrouping.ts
- ai-tools/cursor.mdx
- apps/desktop/scripts/after-pack-runtime-fixes.cjs
- apps/ios/ADE/Views/Work/WorkNewChatSheet.swift
- apps/desktop/src/shared/types/sync.ts
- apps/desktop/src/main/services/sync/syncRemoteCommandService.test.ts
- apps/desktop/src/main/services/opencode/openCodeRuntime.ts
- apps/desktop/src/shared/modelRegistry.ts
- apps/desktop/src/main/services/ai/providerConnectionStatus.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/desktop/native/ios-sim-helpers/README.md
- apps/desktop/src/renderer/components/chat/ChatIosSimulatorPanel.tsx
- plans/cursor-sdk-chat-integration.md
🚧 Files skipped from review as they are similar to previous changes (26)
- getting-started/open-project.mdx
- apps/desktop/src/renderer/components/terminals/SessionListPane.test.tsx
- apps/desktop/src/renderer/lib/sessions.ts
- apps/desktop/src/shared/modelProfiles.test.ts
- apps/desktop/src/preload/preload.test.ts
- apps/desktop/src/main/services/chat/cursorSdkProtocol.ts
- apps/desktop/src/main/services/ai/authDetector.ts
- apps/desktop/src/shared/types/chat.ts
- apps/desktop/src/main/services/ipc/registerIpc.ts
- apps/ios/ADE/Services/SyncService.swift
- apps/desktop/scripts/validate-mac-artifacts.mjs
- apps/desktop/src/main/services/pty/ptyService.test.ts
- apps/desktop/src/preload/preload.ts
- apps/desktop/src/main/services/ios/iosSimulatorService.test.ts
- apps/desktop/src/main/services/ai/apiKeyStore.ts
- apps/desktop/src/main/services/chat/cursorSdkPolicy.ts
- apps/desktop/native/ios-sim-helpers/build.sh
- apps/desktop/src/shared/modelRegistry.test.ts
- apps/desktop/src/main/services/ai/providerRuntimeHealth.ts
- apps/desktop/src/main/services/ios/iosSimulatorService.ts
- apps/ios/ADE/Models/RemoteModels.swift
- apps/ios/ADE/Views/Work/WorkModelPickerSheet.swift
- apps/desktop/src/shared/modelCatalog.ts
- apps/desktop/src/main/services/pty/ptyService.ts
- apps/desktop/src/main/services/chat/agentChatService.test.ts
- apps/ios/ADE/Views/Work/WorkModelCatalog.swift
|
@copilot review but do not make fixes |
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
chat.modelCatalog).Improvements
Greptile Summary
This PR introduces Cursor SDK API-key authentication (replacing CLI-based auth), a shared model catalog API consumed by both the desktop UI and the iOS app, macOS Keychain storage for API keys, iOS simulator helper packaging improvements, and streaming fallback behaviour refinements. The most significant structural addition is
getModelCatalog/chat.modelCatalog, which aggregates models across all provider groups into a structured response used by the new iOS model picker.Confidence Score: 4/5
Safe to merge; all findings are P2 — no runtime errors or data-loss paths introduced.
No P0 or P1 issues found. The two P2 findings are: a dead ternary in
findContainingAppBundle(macOS-only path, no functional impact) and the absence of per-provider error fields in the catalog response (UX gap, not a crash). Previously-flagged concerns around keychain mutation ordering, TOCTOU socket paths, and@MainActorisolation have all been addressed in this PR.apps/desktop/src/main/services/ios/iosSimulatorService.ts (dead ternary), apps/desktop/src/main/services/chat/agentChatService.ts (catalog error propagation)
Important Files Changed
getModelCatalogwhich aggregates models across all 5 provider groups and serialises them into a structured catalog for iOS; provider errors are silently swallowed returning an empty group indistinguishable from a successful empty response.ensurePrivateSocketPathwith fd-based ownership/type checks,cleanupCursorSdkRuntimePathsfor one-shot catalog/cloud requests (addressing prior accumulation issue), and exportsresolveCursorSdkUserHome.~/.cursor/hooks.jsonin the user's real home directory; handles Electron/Node/Windows variants and guards non-ADE Cursor sessions with env-var checks in the bridge script.probeCursorSdkModelDiscovery;cached-onlymode correctly defers to warm-up on first access.findContainingAppBundlecontains an identical dead ternary in both branches.invalidateProviderReadinessCaches(exported for IPC handlers), extendsverifyApiKeyConnectionto probe Cursor model discovery after key verification, and prevents stale status cache from surviving a generation change.buildProviderGroupBlocksused by both the desktop UI and the newgetModelCatalogAPI.@MainActor-isolatedgetChatModelCatalogandcachedChatModelCatalogwith per-host TTL cache and in-flight deduplication;@MainActoronlistChatModelsaddresses the previously-flagged concurrency race.workModelCatalogGroupsoverload,workCatalogModelOption, and new registry/runtime ID lookup helpers for Claude and Codex models including gpt-5.3-codex-spark and gpt-5.2.sanitizeGeneratedCliTitleto strip emojis, ANSI, quotes, and generic provider command tokens from AI-generated session titles;enrichSessionsnow surfaces live PTY status/endedAt/exitCode overrides consistently.cursorSdkHooks.ts), improves error detail extraction, and enriches cloud run results with agent name viaAgent.get.Comments Outside Diff (3)
apps/desktop/src/main/services/chat/cursorSdkPool.ts, line 451-472 (link)stateRootdirectories accumulate per one-shot requestrunCursorSdkCatalogRequestandrunCursorSdkCloudRequestbuild theirpoolKeywithDate.now() + Math.random(), so every call passes a unique key tocreateCursorSdkConnection. Inside that function,fs.mkdirSync(paths.stateRoot, { recursive: true })creates{projectRoot}/.ade/cache/cursor-sdk/{unique_hash}/state/. These directories are never cleaned up — neither in thefinallyblock that callsreleaseCursorSdkConnection, nor anywhere else. The Cursor SDK worker may also write state files intostateRootbefore being disposed. Users who access the model catalog or cloud APIs frequently will accumulate a new directory subtree per call indefinitely.Prompt To Fix With AI
apps/desktop/src/main/services/chat/cursorSdkPool.ts, line 451-472 (link)/tmp/runCursorSdkCatalogRequest(andrunCursorSdkCloudRequestat line 500) build theirpoolKeywithDate.now() + Math.random(), giving each one-shot call a unique key.socketPathFornow maps that key to/tmp/ade-cursor-sdk-${uid}/${hashKey(poolKey)}/hook.sock, andensurePrivateSocketPathcreates the per-call hash subdirectory. On dispose,removeSocketIfNeededin the worker deletes only thehook.sockfile — the empty hash directory is never removed. Every catalog or cloud API call leaves a stale directory under/tmp/ade-cursor-sdk-${uid}/. Unlike the tmpdir socket files in the old flat layout, these new subdirectories require an explicitrmdirthat isn't currently issued.Prompt To Fix With AI
apps/desktop/src/main/services/chat/cursorSdkPool.ts, line 483-505 (link)runCursorSdkCatalogRequest(andrunCursorSdkCloudRequestat line 533) build a uniquepoolKeyper call viaDate.now() + Math.random(). This creates two temporary directory trees that survive beyond the request:{projectRoot}/.ade/cache/cursor-sdk/{hash}/—cleanupCursorSdkStateRootremoves only the nestedstate/subdirectory; the parent{hash}/dir remains empty after every catalog call./tmp/ade-cursor-sdk-{uid}/{hash}/—removeSocketIfNeededin the worker deletes onlyhook.sock; the parent{hash}/subdirectory is never removed.Both accumulate indefinitely. Fix: in
cleanupCursorSdkStateRootusecacheRoot(parent ofstateRoot) instead, and add anfs.rmSyncfor the socket's parent directory inreleaseCursorSdkConnection.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (7): Last reviewed commit: "ship: fix review and ci follow-up" | Re-trigger Greptile