feat(chat): Cursor SDK chat integration (replace Cursor ACP)#223
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughReplaces legacy Cursor CLI/ACP wiring with a Cursor TypeScript SDK + cloud integration: adds CLI cloud commands, SDK worker & pool, IPC/preload bindings, UI for Cursor Cloud, SDK-based model discovery/auth, event/policy mapping, and many renderer/main-service adaptations. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const CONTROL_BUDGET = 850; | ||
| const CLOUD_BUDGET = 900; | ||
| const TRUNCATED_MARKER = "\n[truncated]"; | ||
|
|
There was a problem hiding this comment.
SECRET_RE over-matches legitimate content
/[A-Za-z0-9+/=_\-]{40,}/ will strip any 40+ character alphanumeric run from project rules — including 40-character SHA-1 git commit hashes, long identifiers, and base64-encoded non-secret data. A git SHA like a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2 would be silently deleted from the project rules injected into the system prompt, leaving incomplete context for the agent.
Consider narrowing the pattern (e.g., require a common secret prefix heuristic or a minimum entropy score) or at minimum logging which lines were stripped so the behaviour is auditable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/chat/cursorSdkSystemPrompt.ts
Line: 10
Comment:
**`SECRET_RE` over-matches legitimate content**
`/[A-Za-z0-9+/=_\-]{40,}/` will strip any 40+ character alphanumeric run from project rules — including 40-character SHA-1 git commit hashes, long identifiers, and base64-encoded non-secret data. A git SHA like `a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2` would be silently deleted from the project rules injected into the system prompt, leaving incomplete context for the agent.
Consider narrowing the pattern (e.g., require a common secret prefix heuristic or a minimum entropy score) or at minimum logging which lines were stripped so the behaviour is auditable.
How can I resolve this? If you propose a fix, please make it concise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
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 (3)
apps/desktop/src/renderer/lib/modelOptions.test.ts (1)
154-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't require discovered Cursor IDs to exist in the static registry.
availableModelIdsis where dynamic Cursor models come from, sogetModelById(id)can legitimately returnundefinedfor a valid configured model likecursor/composer-2. That makes this test fail on the exact discovery path the PR is adding. UseresolveModelDescriptor(id)here, or assert on the returned IDs without going back through the static registry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/modelOptions.test.ts` around lines 154 - 160, The test incorrectly assumes discovered Cursor IDs must exist in the static registry by calling getModelById(id); replace that lookup with resolveModelDescriptor(id) (or simply assert the ID appears in availableModelIds) so dynamically discovered Cursor models (e.g., "cursor/composer-2") are allowed; update the assertions to use the resolved descriptor from resolveModelDescriptor(id) and then check descriptor.family === "cursor", descriptor.authTypes includes "api-key", and descriptor.isCliWrapped === false, or alternatively drop registry lookups and assert properties about the IDs themselves.apps/desktop/src/renderer/components/chat/AgentChatPane.tsx (2)
4525-4533:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire Cursor Cloud into the shared right-pane state machine.
These paths open/render the cloud pane, but they still don't treat it like the other mutually-exclusive right-side panes. In practice, App Control and Cursor Cloud can both stay open, and when Cursor Cloud is the only side pane it bypasses the shared divider/split sizing because
rightPaneOpennever includes it.A small fix here is to close sibling drawers whenever either pane opens, and include
cursorCloudPaneOpenin the same right-pane bookkeeping that proof/iOS/App Control already use.Also applies to: 5136-5141, 5429-5429, 5525-5525
🤖 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 4525 - 4533, The App Control click handler toggles only its own state (setAppControlOpen) but doesn't update the shared right-pane bookkeeping for cursorCloudPaneOpen or rightPaneOpen, allowing Cursor Cloud and App Control to be open simultaneously and bypass the shared divider sizing; update this handler (and the other similar handlers referenced) so that when opening App Control you also close sibling panes by calling setCursorCloudPaneOpen(false), setProofDrawerOpen(false), and setIosSimulatorOpen(false), and ensure cursorCloudPaneOpen is included wherever rightPaneOpen is computed/derived so the shared right-pane state machine treats Cursor Cloud as mutually exclusive with other right-side drawers.
1406-1442:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the seeded AI state when the project changes.
The new comments say this should recompute on project switches, but the
useStateinitializers on Lines 1415, 1427, and 1433 only run once. If this pane stays mounted whileprojectRootchanges, it will keep showing the previous project's models/provider status until the async refresh finishes, and a failed refresh leaves the stale state in place.♻️ Keep the seeded state in sync with
seedAiStatusconst [availableModelIds, setAvailableModelIds] = useState<string[]>(() => seedAiStatus ? deriveConfiguredModelIds(seedAiStatus, { includeDroid: true }) : [], ); @@ const [providerConnections, setProviderConnections] = useState<{ claude: AiProviderConnectionStatus | null; codex: AiProviderConnectionStatus | null; cursor: AiProviderConnectionStatus | null; droid: AiProviderConnectionStatus | null; } | null>(() => seedAiStatus ? { claude: seedAiStatus.providerConnections?.claude ?? null, codex: seedAiStatus.providerConnections?.codex ?? null, cursor: seedAiStatus.providerConnections?.cursor ?? null, droid: seedAiStatus.providerConnections?.droid ?? null, } : null, ); + + useEffect(() => { + setAvailableModelIds( + seedAiStatus ? deriveConfiguredModelIds(seedAiStatus, { includeDroid: true }) : [], + ); + setAiStatus(seedAiStatus); + setProviderConnections( + seedAiStatus + ? { + claude: seedAiStatus.providerConnections?.claude ?? null, + codex: seedAiStatus.providerConnections?.codex ?? null, + cursor: seedAiStatus.providerConnections?.cursor ?? null, + droid: seedAiStatus.providerConnections?.droid ?? null, + } + : null, + ); + }, [seedAiStatus]);🤖 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 1406 - 1442, The seeded AI state (seedAiStatus) is only used for initializers, so when projectRoot changes the pane can keep stale model/provider state; add a useEffect that watches projectRoot and/or seedAiStatus and calls setAvailableModelIds(seedAiStatus ? deriveConfiguredModelIds(seedAiStatus, { includeDroid: true }) : []), setProviderConnections(seedAiStatus ? { claude: seedAiStatus.providerConnections?.claude ?? null, codex: seedAiStatus.providerConnections?.codex ?? null, cursor: seedAiStatus.providerConnections?.cursor ?? null, droid: seedAiStatus.providerConnections?.droid ?? null } : null), and setAiStatus(seedAiStatus) to reset those pieces of state (refer to availableModelIds, providerConnections, aiStatus and the deriveConfiguredModelIds helper) whenever the project/seed changes so the UI doesn’t show stale data while async refresh runs.
🟡 Minor comments (9)
plans/cursor-sdk-chat-integration.md-670-671 (1)
670-671:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDefinition-of-done wording conflicts with the SDK-only statement.
Line 670 says ACP can still be enabled via override, but Line 6 states ACP fallback/transport were removed. Please align this to one source of truth.
Proposed wording fix
-- Cursor SDK is the default and only active Cursor chat path unless ACP override is explicitly set. +- Cursor SDK is the default and only active Cursor chat path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plans/cursor-sdk-chat-integration.md` around lines 670 - 671, The two statements conflict: one says ACP can be enabled via override and another says ACP fallback/transport were removed; reconcile by choosing a single truth and editing both occurrences — either explicitly state “Cursor SDK is the default and only active Cursor chat path; ACP fallback/transport has been removed and cannot be enabled” or instead allow ACP override by updating the SDK line to “Cursor SDK is the default; ACP may be enabled only via explicit override” — make the same wording change in the sentences that currently read “Cursor SDK is the default and only active Cursor chat path unless ACP override is explicitly set.” and “Local Cursor SDK chat can answer, inspect, edit, run shell, stream, and cancel.” (clarify whether ACP supports any of those features if you keep ACP override), and remove or adjust any other mentions of “ACP fallback/transport” to match the chosen single source of truth.apps/desktop/src/renderer/lib/modelOptions.ts-192-195 (1)
192-195:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize provider before Cursor auth check.
Line 193 uses a raw string comparison; if
provideris not normalized, dynamiccursor/*model IDs may be skipped.Suggested fix
- const cursorAuthed = status.detectedAuth?.some( - (a) => a.type === "api-key" && a.provider === "cursor", - ); + const cursorAuthed = status.detectedAuth?.some( + (a) => a.type === "api-key" && normalizeAuthProvider(a.provider) === "cursor", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/modelOptions.ts` around lines 192 - 195, The current cursor auth check uses a raw provider string and can miss variants like "Cursor" or "cursor/xyz"; update the cursorAuth computation (the const cursorAuthed that checks status.detectedAuth?.some) to normalize the provider (e.g., trim and toLowerCase()) and check for either exact "cursor" or startsWith("cursor") to cover dynamic providers, so the subsequent availableModelIds handling (status.availableModelIds) correctly includes cursor/* models.apps/ade-cli/src/cli.ts-4646-4652 (1)
4646-4652:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
--compactfor Cursor Cloud output.
parseCliArgs()consumes--compact, but this branch only forwards"text" | "json"torunCursorCloud(). That meansade cursor cloud ... --compactstill emits pretty JSON, unlike the rest of the CLI.Suggested fix
- const result = await runCursorCloud(plan.rest, parsed.options.text ? "text" : "json"); + const cursorArgs = parsed.options.pretty ? plan.rest : ["--compact", ...plan.rest]; + const result = await runCursorCloud(cursorArgs, parsed.options.text ? "text" : "json"); return result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cli.ts` around lines 4646 - 4652, The Cursor Cloud branch ignores the parsed --compact flag and always passes "text" or "json" to runCursorCloud, so preserve and forward compact behavior by detecting parsed.options.compact (in addition to parsed.options.text) and pass the appropriate output mode into runCursorCloud (e.g., "text", "json", or a "compact" variant) so runCursorCloud receives the same format selection as the rest of the CLI; update the call site where plan.kind === "cursor-cloud" and ensure parsed.options.compact is used when present.apps/desktop/src/main/services/chat/cursorSdkEventMapper.test.ts-184-190 (1)
184-190:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake this assertion match the test name.
This test says it verifies runtime forwarding, but it only checks
status. A regression that dropsruntime: "cloud"would still pass.🧪 Tighten the expectation
- // The current shape may or may not include runtime — just verify status is mapped. - expect(done.status).toBe("completed"); + expect(done).toMatchObject({ + status: "completed", + runtime: "cloud", + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/cursorSdkEventMapper.test.ts` around lines 184 - 190, The test currently only checks done.status but is named to verify runtime forwarding; update the expectation to assert that the runtime from the Cursor SDK input is preserved in the mapped event (e.g., verify that the result of mapCursorSdkRunResultToDoneEvent contains runtime === "cloud" in the appropriate meta/location—refer to the mapped object returned by mapCursorSdkRunResultToDoneEvent and the local variable done when adding the assertion).apps/desktop/src/main/services/chat/agentChatService.test.ts-9381-9397 (1)
9381-9397:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid fixed sleeps in the streaming-spacing test.
The
setTimeout(150)makes this spec timing-sensitive on slower CI workers. Since the test already derives the expected text fromevents,vi.waitForis a safer wait condition here.Suggested fix
for (const text of ["Publishing", " a short", " demo", " plan."]) { mockState.cursorSdkPooled.bridge.onEvent({ type: "assistant", message: { content: [{ type: "text", text }], }, }); } - await new Promise((resolve) => setTimeout(resolve, 150)); - - const streamedText = events - .filter((event): event is AgentChatEventEnvelope & { - event: Extract<AgentChatEventEnvelope["event"], { type: "text" }>; - } => event.event.type === "text") - .map((event) => event.event.text) - .join(""); - expect(streamedText).toContain("Publishing a short demo plan."); + await vi.waitFor(() => { + const streamedText = events + .filter((event): event is AgentChatEventEnvelope & { + event: Extract<AgentChatEventEnvelope["event"], { type: "text" }>; + } => event.event.type === "text") + .map((event) => event.event.text) + .join(""); + expect(streamedText).toContain("Publishing a short demo plan."); + });🤖 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 9381 - 9397, Replace the fixed sleep with a deterministic wait: instead of awaiting new Promise(setTimeout, 150) after calling mockState.cursorSdkPooled.bridge.onEvent, use vi.waitFor to poll until the events array has produced the joined streamedText containing the full expected string; locate the block that builds streamedText from events (the filter on event.event.type === "text" and the .map(...).join("")) and wrap the assertion in a vi.waitFor that waits for expect(streamedText).toContain("Publishing a short demo plan.") to pass, so the test reliably waits for the streaming events rather than a fixed timeout.apps/desktop/src/main/services/chat/agentChatService.test.ts-10085-10092 (1)
10085-10092:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe cloud prompt assertion can pass even when the user text is missing.
promptText.length <= promptText.indexOf("Hi cloud.") + 1024grows with the index, so it doesn't actually bound the injected prefix, and a short prompt with no"Hi cloud."still passes. This weakens the regression coverage for first-cloud-send prompt construction.Suggested fix
const sent = mockState.cursorSdkCloudRequests.find((r) => r.type === "cloud.send.stream"); expect(sent).toBeTruthy(); const promptText = String(sent!.payload.promptText ?? ""); // System-prompt sections should be present expect(promptText).toContain("ADE control protocol"); expect(promptText).toContain("Cursor Cloud capability"); expect(promptText).toContain("runtime: cloud"); - expect(promptText.length).toBeLessThanOrEqual(promptText.indexOf("Hi cloud.") + 1024); + expect(promptText).toContain("Hi cloud."); + const userPromptIndex = promptText.indexOf("Hi cloud."); + expect(userPromptIndex).toBeGreaterThanOrEqual(0); + expect(userPromptIndex).toBeLessThanOrEqual(1024);🤖 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 10085 - 10092, The test's length assertion is weak because promptText.indexOf("Hi cloud.") can be -1 and the inequality still pass; update the assertions around the sent payload (variable sent and promptText) to first assert that the user text marker "Hi cloud." is present (e.g. expect promptText toContain "Hi cloud.") and then compute the prefix length using promptText.indexOf("Hi cloud.") and assert that that index (i.e. the injected prefix length) is <= 1024; ensure you also keep the existing checks for the system-prompt sections ("ADE control protocol", "Cursor Cloud capability", "runtime: cloud") so the test fails if the user text is missing or the injected prefix is too large.apps/desktop/src/main/services/chat/agentChatService.test.ts-14-15 (1)
14-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
ADE_CURSOR_PROMPT_INJECTin teardown too.The new cloud prompt test mutates
process.env.ADE_CURSOR_PROMPT_INJECT, butafterEach()only restoresCURSOR_API_KEY. That leaks state into later specs and makes this file order-dependent.Suggested fix
-const ORIGINAL_CURSOR_API_KEY = process.env.CURSOR_API_KEY; +const ORIGINAL_CURSOR_API_KEY = process.env.CURSOR_API_KEY; +const ORIGINAL_ADE_CURSOR_PROMPT_INJECT = process.env.ADE_CURSOR_PROMPT_INJECT; … afterEach(() => { vi.restoreAllMocks(); if (ORIGINAL_CURSOR_API_KEY === undefined) { delete process.env.CURSOR_API_KEY; } else { process.env.CURSOR_API_KEY = ORIGINAL_CURSOR_API_KEY; } + if (ORIGINAL_ADE_CURSOR_PROMPT_INJECT === undefined) { + delete process.env.ADE_CURSOR_PROMPT_INJECT; + } else { + process.env.ADE_CURSOR_PROMPT_INJECT = ORIGINAL_ADE_CURSOR_PROMPT_INJECT; + } try {Also applies to: 942-948
🤖 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 14 - 15, The test saves ORIGINAL_CURSOR_API_KEY but fails to restore ADE_CURSOR_PROMPT_INJECT, leaking env state; update the test teardown (afterEach/teardown block) to capture the original process.env.ADE_CURSOR_PROMPT_INJECT at top of the file (similar to ORIGINAL_CURSOR_API_KEY) and restore it in the afterEach(), ensuring both ORIGINAL_CURSOR_API_KEY and the saved ADE_CURSOR_PROMPT_INJECT are reset; apply the same restore logic for the other teardown area referenced around the 942-948 block so no tests remain order-dependent.apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts-119-121 (1)
119-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential ID collision when
call_idis not provided.The fallback
cursor-sdk-tool-${Date.now()}could collide if multiple tool calls arrive within the same millisecond. Consider adding a random suffix:-const callId = readString(record.call_id) ?? readString(record.id) ?? `cursor-sdk-tool-${Date.now()}`; +const callId = readString(record.call_id) ?? readString(record.id) ?? `cursor-sdk-tool-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts` around lines 119 - 121, The fallback ID generation in the "tool_call" case (variable callId built from readString(record.call_id) ?? readString(record.id) ?? `cursor-sdk-tool-${Date.now()}`) can collide when multiple events share the same millisecond; change the fallback to append a high-entropy suffix (e.g., a UUID or secure random hex) to Date.now() to guarantee uniqueness (for example use crypto.randomUUID() or a secure random value) and keep the rest of the logic (readString(record.call_id) and readString(record.id)) intact; update callId assignment in the "tool_call" branch and ensure any downstream code that expects that ID continues to accept the new format.apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts-214-216 (1)
214-216:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError event omits runtime tagging unlike other events.
Line 215 creates an error event without calling
tagRuntime, while most other events use it. This could be intentional if errors should always appear as local, but for consistency with the rest of the mapper, consider adding runtime tagging:-return [{ type: "error", message: detail ?? "Cursor SDK run failed.", turnId }]; +return [tagRuntime({ type: "error", message: detail ?? "Cursor SDK run failed.", turnId }, runtime)];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts` around lines 214 - 216, The error branch for statusText === "ERROR" returns an event without applying tagRuntime; update that return to wrap the error event with tagRuntime just like other branches (use tagRuntime({ type: "error", message: detail ?? "Cursor SDK run failed.", turnId }) or the same call signature used elsewhere) so the runtime tagging is consistent; modify the code in the statusText === "ERROR" block to call tagRuntime on the constructed event.
🧹 Nitpick comments (9)
apps/ade-cli/src/cursorCloud.ts (1)
458-460: 💤 Low valueExtra newline after each assistant message may cause formatting issues.
Line 460 writes a newline after processing all text blocks in an assistant message. If the SDK sends multiple
assistantevents for a single response, this could produce excessive blank lines:if (printText) process.stderr.write("\n");Consider tracking whether any text was actually written before adding the newline, or removing it if the SDK already includes trailing newlines in the text content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ade-cli/src/cursorCloud.ts` around lines 458 - 460, The unconditional newline after processing assistant text blocks (the printText write at the end) can create extra blank lines when multiple assistant events arrive; update the assistant-event handling in cursorCloud.ts to only emit a trailing newline when actual text was written and it doesn't already end with a newline—e.g., add a boolean flag (wroteText) set when writing any chunk (or inspect the last written character) and change the final conditional to: if (wroteText && lastChar !== '\n') emit a single newline; alternatively remove the unconditional newline if the SDK guarantees trailing newlines in content. Ensure the change touches the block using the printText variable in the assistant response handling so you don't add newlines when nothing was printed.apps/desktop/src/main/services/ai/aiIntegrationService.ts (3)
1103-1125: 💤 Low valueThe cleanup pattern works correctly but is verbose.
The
finallyblocks with nested try-catch forSymbol.asyncDisposeand fallback to.close()are repeated. This is acceptable given the defensive error handling needed, but if this pattern grows, consider extracting a helper like:async function disposeAgent(agent: unknown): Promise<void> { try { await (agent as { [Symbol.asyncDispose]?: () => Promise<void> })[Symbol.asyncDispose]?.(); } catch { try { (agent as { close?: () => void }).close?.(); } catch { /* ignore */ } } }Also applies to: 1127-1153
🤖 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.ts` around lines 1103 - 1125, The cleanup try/catch pattern used in listCursorCloudArtifacts (and the similar block at 1127-1153) is repetitive; extract a helper (e.g., disposeAgent) that accepts the cloud agent object and encapsulates the defensively-ordered disposal logic: attempt await (agent as { [Symbol.asyncDispose]?: () => Promise<void> })[Symbol.asyncDispose]?.(), and on failure fallback to (agent as { close?: () => void }).close?.(), swallowing errors as before; then replace the duplicated finally blocks in listCursorCloudArtifacts (and the other occurrence) with a single call to disposeAgent(agent).
365-377: 💤 Low valueConsider consolidating or renaming
readNumberto clarify date-parsing behavior.
readNumberparses ISO date strings viaDate.parsein addition to plain numbers, while the existingtoNumberOrNull(line 272) only handles numeric values. This implicit date parsing could be surprising. Consider either:
- Renaming to
readNumberOrTimestampfor clarity- Separating the date-parsing logic into the callers that need it
The current implementation works correctly for the normalization use case where
lastModified/createdAtmay arrive as either timestamps or ISO strings.🤖 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.ts` around lines 365 - 377, The helper readNumber currently accepts numbers and ISO date strings via Date.parse, which is surprising compared to toNumberOrNull; either rename readNumber to readNumberOrTimestamp to make its date-parsing behavior explicit (update all callers like the normalization code handling lastModified/createdAt to use the new name), or remove date parsing from readNumber and move Date.parse logic into the specific callers that expect timestamps (e.g., the normalization path for lastModified/createdAt) so readNumber/toNumberOrNull remain consistent; pick one approach and update function name/usages or relocate the date parsing accordingly.
1027-1064: Consider adding a custom timeout wrapper to theagent.send()call.
createCursorCloudRuncallsagent.send(promptText)without a timeout. The@cursor/sdkdoes not provide built-in timeout options for the send method. If the Cursor Cloud service is slow or unresponsive, this could hang indefinitely. To mitigate this, wrap the call withPromise.race()or a similar timeout mechanism.🤖 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.ts` around lines 1027 - 1064, createCursorCloudRun currently calls agent.send(promptText) with no timeout, risking an indefinite hang; wrap the send call with a timeout wrapper (e.g., Promise.race between agent.send(...) and a timeout promise) that rejects after a sensible default (or an optional args.timeoutMs) so the function fails fast on unresponsive Cursor Cloud. Update createCursorCloudRun to use that wrapper when awaiting the run (preserve existing behavior on success), throw a clear timeout Error on expiry, and ensure downstream code still passes the resolved run into normalizeCursorCloudRun(agentId) as before.apps/desktop/src/main/services/chat/cursorSdkPool.ts (1)
289-305: 💤 Low valueHook request default deny is secure but consider logging when no handler is registered.
The default deny decision when
bridge.onHookRequestis null is the correct security posture. However, silently denying without logging could make debugging difficult when hooks aren't wired up as expected.💡 Suggested improvement
void (async () => { - const decision = bridge.onHookRequest - ? await bridge.onHookRequest(message.request) - : { + let decision: CursorSdkHookDecision; + if (bridge.onHookRequest) { + decision = await bridge.onHookRequest(message.request); + } else { + args.logger?.debug("agent_chat.cursor_sdk_hook_no_handler", { requestId: message.requestId }); + decision = { permission: "deny" as const, user_message: "ADE is not ready to approve Cursor tool calls.", agent_message: "ADE is not ready to approve Cursor tool calls.", }; + } child.send?.({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/cursorSdkPool.ts` around lines 289 - 305, When handling message.type === "hook_request" the code currently returns a default deny decision when bridge.onHookRequest is missing but doesn't log that no hook handler was registered; update the async block around bridge.onHookRequest to detect when bridge.onHookRequest is falsy and emit a concise log (e.g., via your service logger or console) indicating that no onHookRequest handler is registered and a default deny was returned, while keeping the existing decision variable, child.send payload, and behavior intact (refer to bridge.onHookRequest, decision, child.send and the "hook_request" branch to locate where to add the log).apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx (1)
1973-1991: 💤 Low valueCloud submit path clears draft only on success — verify UI feedback during async gap.
The cloud submit branch fires
onSubmitToCloudand clears the draft asynchronously after success. During the async gap (between submit andokcallback), the composer remains interactive with the draft still visible. IfonSubmitToCloudis slow, users might submit again or be confused about state.Consider disabling input or showing a loading indicator while the cloud submit is in flight, or document that
onSubmitToCloudshould be near-instant (returns synchronously or resolves quickly with optimistic UI handled upstream).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx` around lines 1973 - 1991, The cloud-submit branch currently calls onSubmitToCloud(trimmed) and only clears the draft on successful resolution, leaving the composer interactive during the async gap; add a short-lived "cloudSubmitting" state flag (or reuse/extend existing busy/parallelLaunchBusy) that is set true immediately before invoking onSubmitToCloud(trimmed) and reset in both then and catch, use that flag to disable input and prevent re-submits (include it in the early-return guard alongside busy/modelId/draft checks), and ensure onDraftChange("") is still called only on success; update the effect dependency array to include the new flag so the UI correctly reflects the submitting state.apps/desktop/src/renderer/components/chat/ChatCursorCloudPanel.tsx (1)
162-212: 💤 Low valueConsider memoizing the
refreshcallback's dependencies to avoid unnecessary recreations.The
refreshcallback depends on[defaultRepoUrl, includeArchived, laneGitRemote, modelOptions, repos]. Thereposdependency meansrefreshis recreated every timereposstate changes, which happens insiderefreshitself. This creates a new function identity on each refresh cycle.While functionally correct due to
refreshRef.currentpattern in the interval, this could cause subtle issues ifrefreshis used in other dependency arrays.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatCursorCloudPanel.tsx` around lines 162 - 212, The refresh callback is being recreated whenever the repos state changes because repos is listed in its dependency array; avoid this by removing repos from the dependencies and reading the latest repos via a ref instead—create a reposRef (e.g., const reposRef = useRef(repos)), update reposRef.current whenever you call setRepos or on mount, and inside refresh use reposRef.current wherever repos is currently referenced (keep reposLoadedRef for loaded-checks and leave other deps like defaultRepoUrl, includeArchived, laneGitRemote, modelOptions as-is or ensure modelOptions is stable). This preserves correct behavior while preventing refresh from being rebuilt on every setRepos.apps/desktop/src/main/services/chat/cursorSdkSystemPrompt.ts (2)
203-230: 💤 Low valueConsider moving the
requirestatement to module scope.The
require("node:fs")inside the function body works but is unconventional. Sincefsis already imported at the top as promises, you could import the sync API there as well:-import { promises as fs } from "node:fs"; +import { promises as fs, existsSync } from "node:fs";Then use
existsSyncdirectly instead of requiring inside the function. This is a minor style consistency improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/cursorSdkSystemPrompt.ts` around lines 203 - 230, Move the dynamic require("node:fs") out of findAdeCliHelpDigestFile and import or require the sync API at module scope (e.g., import * as fsSync from "node:fs" or const fsSync = require("node:fs") at top) so the function uses the top-level fsSync.existsSync directly; update findAdeCliHelpDigestFile to remove the inline require and call fsSync.existsSync(c) in the loop.
11-11: 💤 Low valueSECRET_RE may produce false positives on base64-encoded content or long identifiers.
The regex
/[A-Za-z0-9+/=_\-]{40,}/matches any 40+ character alphanumeric string, which could inadvertently strip legitimate content like:
- Long file paths or URLs
- Base64-encoded non-secret data (e.g., image placeholders in docs)
- Long identifier strings in code examples
Consider making the pattern more specific (e.g., requiring specific prefixes like
sk-,ghp_,AKIA) or documenting this known limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/cursorSdkSystemPrompt.ts` at line 11, The current SECRET_RE (/[A-Za-z0-9+/=_\-]{40,}/) is too permissive and causes false positives; replace it with a stricter matcher that targets known secret prefixes and formats (e.g., require prefixes like sk-, ghp_, AKIA, or other provider-specific tokens) or maintain an array of specific regexes (e.g., SECRET_PATTERNS = [ /^(sk-[A-Za-z0-9]{32,})$/, /^(ghp_[A-Za-z0-9]{36,})$/, /^(AKIA[0-9A-Z]{16})$/ ]) and use those to detect secrets instead of the broad SECRET_RE, and add a short comment above the pattern(s) documenting the limitation and rationale for chosen prefixes/patterns so legitimate long strings (base64 blobs, long URLs, file paths) are not stripped.
🤖 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/ade-cli/src/cursorCloud.ts`:
- Around line 183-188: The function runCursorCloud currently ignores the
_outputMode parameter and instead determines output from flags; either remove
the unused _outputMode parameter or use it as the fallback when no explicit
--text/--json flags are present: in runCursorCloud, keep resolving apiKey and
flags from cleaned, but change the output assignment logic (the variable output)
to prefer the --text/--json flags and fall back to the incoming _outputMode if
neither flag is present (or if you choose to remove dead code, delete the
_outputMode parameter and any references to it). Ensure CursorCloudOptions ({
apiKey, output, pretty }) uses the corrected output value so the function
signature and behavior are consistent.
In `@apps/desktop/package.json`:
- Around line 162-165: Add an explicit Cursor SDK x64 pattern to the
x64ArchFiles array so Intel macOS builds pick the correct artifacts: inside the
package.json x64ArchFiles entry, add
"Contents/Resources/app.asar.unpacked/node_modules/@cursor/sdk-darwin-x64/**/*"
(matching how sharp-darwin-* is listed) so mergeASARs picks the x64 Cursor SDK
rather than a wrong arch.
In `@apps/desktop/scripts/regen-ade-cli-help.cjs`:
- Around line 8-31: The SUBCOMMANDS array is missing desktop surfaces (e.g.,
"terminal" and "app-control") so their help sections are never generated; update
the SUBCOMMANDS constant in regen-ade-cli-help.cjs to include the missing
command names (at least "terminal" and "app-control") alongside the existing
entries so ade-cli-help.txt contains their detailed help sections for the Cursor
SDK; locate the SUBCOMMANDS array in the file and append the required strings to
the list.
- Around line 49-59: The current main() writes a stub when cliPath is missing;
instead fail fast by attempting to build the ADE CLI and/or exiting with a
non-zero status: when cliPath does not exist (check in main using cliPath and
outPath/outDir), run the build command "npm --prefix apps/ade-cli run build" and
if that build fails or the file still does not exist, log an error with the
cliPath and exit process with a non-zero code (do not write the stub); keep
references to main(), cliPath, outPath and outDir so the change is applied in
the correct block.
In `@apps/desktop/src/main/services/ai/authDetector.ts`:
- Around line 790-819: The verifyCursorApiKey function currently awaits
Cursor.me() directly and needs timeout protection like other providers; wrap the
Cursor.me({ apiKey: key }) call in a timeout (use the existing
API_KEY_VERIFY_TIMEOUT_MS constant) via a Promise.race or a helper
timeoutPromise so the call rejects after the timeout, then handle the rejection
in the existing try/catch flow (preserving the same returned shape and message
logic in verifyCursorApiKey). Ensure you import/use API_KEY_VERIFY_TIMEOUT_MS
and keep provider:"cursor", endpoint:"Cursor.me" and statusCode handling
unchanged.
In `@apps/desktop/src/main/services/ai/providerConnectionStatus.ts`:
- Around line 175-179: The catch block around getAllApiKeys() collapses
store-read failures into "no API key" — change this so errors from
getAllApiKeys() are handled separately: catch the exception, record/log a
distinct storeReadError (or similar flag) instead of leaving cursorStoredAuth
true/false ambiguous, and only set cursorStoredAuth based on the presence of
getAllApiKeys().cursor when no exception occurs; apply the same pattern for the
other similar block (lines referencing getAllApiKeys() later, e.g., the other
stored auth flag) so persistent store errors are distinguishable from a missing
key.
- Around line 184-203: The code conflates runtime detection with authentication
causing runtimeDetected and path to be cleared when cursorSdkAuth is false;
update the logic so runtimeDetected and runtimeAvailable reflect only the
presence of the bundled SDK (keep detection based on the existing runtime check,
not cursorSdkAuth) while authAvailable, cliAuthenticated,
cliExplicitlyUnauthenticated, and cursorBlocker continue to reflect
cursorSdkAuth; specifically adjust the cursorFlags object and the cursor
constant (symbols: cursorFlags, cursorBlocker, cursor, createUnavailableStatus,
cursorSdkAuth) so path remains "@cursor/sdk" and
runtimeDetected/runtimeAvailable remain true when the SDK is present even if
cursorSdkAuth is false, and only auth-related fields depend on cursorSdkAuth.
In `@apps/desktop/src/main/services/ai/providerTaskRunner.ts`:
- Around line 280-295: The Cursor SDK branch currently awaits run.wait() with no
timeout, dropping args.timeoutMs; wrap the wait call in a Promise.race so the
wait is raced against a timeoutPromise that, after args.timeoutMs, calls
run.cancel() and rejects with a timeout Error; replace the direct await of
run.wait() in the block after agent.send(combinedPrompt, ...) with the raced
promise and propagate the timeout rejection similarly to other providers (use
the same error text used elsewhere), then keep the existing result handling
(result.status checks, text, structuredOutput via parseStructuredOutput,
sessionId: agent.agentId).
- Around line 267-283: runCursorTask is missing timeout handling,
unconditionally disables the local sandbox, and misuses local.force; update the
Agent.resume/Agent.create calls to leave sandboxOptions.enabled true by default
and only set to false when args.permissionMode === "full-auto", remove or stop
setting local.force based on permissionMode (only set local.force for
crash-recovery semantics when explicitly needed), and implement an
AbortController timeout using args.timeoutMs (mirror the OpenCode pattern) and
pass its signal to agent.send / run.wait so the run is aborted on timeout.
Target the Agent.resume, Agent.create, sandboxOptions, local.force, agent.send,
and run.wait sites when making these changes.
In `@apps/desktop/src/main/services/chat/cursorModelsDiscovery.ts`:
- Around line 76-85: Wrap the SDK discovery in listCursorModelsFromSdk with a
try-catch around the dynamic import and Cursor.models.list call (the block that
produces rows via normalizeSdkModelRows) so transient or invalid-key errors
don't throw; on catch, return an empty array and avoid updating sdkCached,
optionally log the error, preserving the existing cache behavior (sdkCached,
keyHash, TTL_MS) and ensuring callers receive [] as the best-effort fallback.
In `@apps/desktop/src/main/services/chat/cursorSdkPolicy.ts`:
- Around line 181-225: collectPotentialPaths currently returns early when value
is a string, so top-level string inputs never become candidates and
pathGuardReason cannot check them; update collectPotentialPaths (used by
pathGuardReason) to treat a non-empty string value as a potential path by
trimming and pushing it into out (same validation as other string cases) instead
of returning immediately, ensuring top-level string hook inputs are validated
for outside-lane and ADE-protected paths.
In `@apps/desktop/src/main/services/chat/cursorSdkWorker.ts`:
- Around line 268-275: The initWorker function currently only sets
process.env.CURSOR_API_KEY when init.apiKey is provided, causing reused workers
to retain a previous key; update initWorker (the initWorker function, which sets
initState and calls ensureDir) to explicitly clear process.env.CURSOR_API_KEY
when init.apiKey is missing or blank (e.g., set it to empty string or delete the
env var) so a subsequent init without apiKey cannot reuse prior credentials,
while preserving the existing behavior of trimming and setting when a non-empty
apiKey is present.
- Around line 556-565: The cloud.send.stream handler currently awaits and
returns streamCloudRun(...) which blocks until the cloud run completes; instead,
immediately create the cloud agent (Agent.create with buildCloudCreateOptions),
start the streaming job in the background by calling streamCloudRun(...) without
awaiting/returning its promise, and return early with the agentId and runId
(from cloudAgent.agentId and the run created by cloudAgent.send) so the renderer
immediately receives agent/run identifiers; ensure you still pass the same
params to streamCloudRun (requestId, agentId, run, modelSdkId) but do not await
its completion.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 6485-6510: The handler registered for IPC.gitGetOriginRemote trims
incoming IPC fields without type checks, which throws if the renderer sends
non-strings; update the unsafe trims in the handler to a runtime string check
pattern (e.g., typeof arg?.laneId === "string" ? arg.laneId.trim() : "") instead
of (arg?.laneId ?? "").trim(); likewise apply the same fix for any other IPC
field trims in this region (e.g., arg?.branch) so the handler (the
ipcMain.handle callback for IPC.gitGetOriginRemote and its local variables like
laneId and knownBranch) gracefully handles non-string inputs and falls back
correctly.
- Around line 3266-3371: The Cursor Cloud IPC handlers (e.g., handlers for
IPC.aiCursorCloudListAgents, aiCursorCloudListRuns, aiCursorCloudCreateRun,
aiCursorCloudArchiveAgent/Unarchive/DeleteAgent, aiCursorCloudGetAgent,
aiCursorCloudListArtifacts, aiCursorCloudDownloadArtifact,
aiCursorCloudCancelRun, aiCursorCloudFollowUp, aiCursorCloudOpenChat,
aiCursorCloudStreamRun) lack validation and trust renderer input; add small
parse/validate helpers (patterned after the App Control/terminal IPC helpers)
and call them at the top of each handler to ensure required fields and types
(agentId/runId/path as non-empty strings, limit as a positive integer if
present, cursor as string|null/undefined, includeArchived as boolean, and full
shape for CursorCloudCreateRun/FollowUp/OpenChat requests) before invoking
ctx.*Service methods, and return/throw a clear IPC-safe error when validation
fails.
In `@apps/desktop/src/preload/preload.ts`:
- Around line 2637-2638: The getOriginRemote IPC handler (getOriginRemote)
currently returns remoteUrl which may include embedded credentials; sanitize the
value in the preload before returning to the renderer by stripping any userinfo
(username:password@) from the URL or returning only non-secret structured fields
(e.g., protocol, host, path, branch) instead; locate the getOriginRemote
implementation and replace the direct ipcRenderer.invoke(IPC.gitGetOriginRemote,
args) result with a sanitized object where remoteUrl has had userinfo removed
(or is null) while preserving branch and other safe fields, using
IPC.gitGetOriginRemote as the source of truth.
- Around line 1142-1195: Typecheck is failing because Node types are missing and
tsconfig is emitting a deprecation error for baseUrl; install and reference
`@types/node` (e.g., add to devDependencies and ensure tsconfig "types" or
typeRoots include "node") and update tsconfig.json compilerOptions to include
"ignoreDeprecations": "6.0" to silence the TypeScript 7.0 baseUrl deprecation,
then re-run npm --prefix apps/desktop run typecheck, lint, and build; while
doing this, verify the newly added preload methods (cursorCloudListRepositories,
cursorCloudListAgents, cursorCloudListRuns, cursorCloudCreateRun,
cursorCloudArchiveAgent, cursorCloudUnarchiveAgent, cursorCloudDeleteAgent,
cursorCloudGetAgent, cursorCloudStreamRun, cursorCloudCancelRun,
cursorCloudFollowUp, cursorCloudListArtifacts, cursorCloudDownloadArtifact,
cursorCloudOpenChat) have matching IPC constants (IPC.aiCursorCloud*) and
corresponding types in the preload types and shared IPC contract files so the
preload → IPC → renderer type contracts remain in sync.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 2794-2802: Replace the direct anchor rendering of event.prUrl with
a button that routes through the secure IPC helper: import openExternalUrl from
../../lib/openExternal and change the JSX in AgentChatMessageList where
event.prUrl is used (currently rendering <a href={event.prUrl} ...>PR</a>) to a
clickable element (e.g., a button or span with onClick={() =>
openExternalUrl(event.prUrl)}) and preserve styling and accessibility (role/aria
and target styling classes) while removing the raw href to ensure the link opens
via the validated external-link bridge.
In `@apps/desktop/src/renderer/components/chat/CursorCloudInlineLaunch.tsx`:
- Around line 229-267: launchWithPrompt currently reuses
detectedPrRef.current.prUrl even when the user changed repoUrl; before attaching
a PR to the run, verify the detected PR belongs to the currently selected repo
and only set attachToPr when it matches. In the launchWithPrompt callback
(references: launchWithPrompt, detectedPrRef, repoUrl, cursorCloudCreateRun),
add a guard after resolving resolvedPr that compares the detected PR's repo
identifier (e.g., parsed owner/repo or stored repoUrl on resolvedPr) against the
current repoUrl and set attachToPr to null if they differ so
cursorCloudCreateRun never receives a mismatched repoUrl/prUrl pair.
- Around line 110-132: Replace the effect-scoped cancellation with a local
boolean and ensure reposLoaded is set on failure: inside the useEffect that
calls window.ade.ai.cursorCloudListRepositories(), declare let cancelled = false
at the top (instead of resetting cancelledRef.current), check that cancelled is
false before calling setRepos, setRepoUrl and setReposLoaded in the .then
handler, and in the .catch handler check cancelled before calling setError but
always call setReposLoaded(true) when not cancelled; finally, in the cleanup
return set cancelled = true. Keep references to repoMatchKey and laneGitRemote
when computing setRepoUrl.
---
Outside diff comments:
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 4525-4533: The App Control click handler toggles only its own
state (setAppControlOpen) but doesn't update the shared right-pane bookkeeping
for cursorCloudPaneOpen or rightPaneOpen, allowing Cursor Cloud and App Control
to be open simultaneously and bypass the shared divider sizing; update this
handler (and the other similar handlers referenced) so that when opening App
Control you also close sibling panes by calling setCursorCloudPaneOpen(false),
setProofDrawerOpen(false), and setIosSimulatorOpen(false), and ensure
cursorCloudPaneOpen is included wherever rightPaneOpen is computed/derived so
the shared right-pane state machine treats Cursor Cloud as mutually exclusive
with other right-side drawers.
- Around line 1406-1442: The seeded AI state (seedAiStatus) is only used for
initializers, so when projectRoot changes the pane can keep stale model/provider
state; add a useEffect that watches projectRoot and/or seedAiStatus and calls
setAvailableModelIds(seedAiStatus ? deriveConfiguredModelIds(seedAiStatus, {
includeDroid: true }) : []), setProviderConnections(seedAiStatus ? { claude:
seedAiStatus.providerConnections?.claude ?? null, codex:
seedAiStatus.providerConnections?.codex ?? null, cursor:
seedAiStatus.providerConnections?.cursor ?? null, droid:
seedAiStatus.providerConnections?.droid ?? null } : null), and
setAiStatus(seedAiStatus) to reset those pieces of state (refer to
availableModelIds, providerConnections, aiStatus and the
deriveConfiguredModelIds helper) whenever the project/seed changes so the UI
doesn’t show stale data while async refresh runs.
In `@apps/desktop/src/renderer/lib/modelOptions.test.ts`:
- Around line 154-160: The test incorrectly assumes discovered Cursor IDs must
exist in the static registry by calling getModelById(id); replace that lookup
with resolveModelDescriptor(id) (or simply assert the ID appears in
availableModelIds) so dynamically discovered Cursor models (e.g.,
"cursor/composer-2") are allowed; update the assertions to use the resolved
descriptor from resolveModelDescriptor(id) and then check descriptor.family ===
"cursor", descriptor.authTypes includes "api-key", and descriptor.isCliWrapped
=== false, or alternatively drop registry lookups and assert properties about
the IDs themselves.
---
Minor comments:
In `@apps/ade-cli/src/cli.ts`:
- Around line 4646-4652: The Cursor Cloud branch ignores the parsed --compact
flag and always passes "text" or "json" to runCursorCloud, so preserve and
forward compact behavior by detecting parsed.options.compact (in addition to
parsed.options.text) and pass the appropriate output mode into runCursorCloud
(e.g., "text", "json", or a "compact" variant) so runCursorCloud receives the
same format selection as the rest of the CLI; update the call site where
plan.kind === "cursor-cloud" and ensure parsed.options.compact is used when
present.
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 9381-9397: Replace the fixed sleep with a deterministic wait:
instead of awaiting new Promise(setTimeout, 150) after calling
mockState.cursorSdkPooled.bridge.onEvent, use vi.waitFor to poll until the
events array has produced the joined streamedText containing the full expected
string; locate the block that builds streamedText from events (the filter on
event.event.type === "text" and the .map(...).join("")) and wrap the assertion
in a vi.waitFor that waits for expect(streamedText).toContain("Publishing a
short demo plan.") to pass, so the test reliably waits for the streaming events
rather than a fixed timeout.
- Around line 10085-10092: The test's length assertion is weak because
promptText.indexOf("Hi cloud.") can be -1 and the inequality still pass; update
the assertions around the sent payload (variable sent and promptText) to first
assert that the user text marker "Hi cloud." is present (e.g. expect promptText
toContain "Hi cloud.") and then compute the prefix length using
promptText.indexOf("Hi cloud.") and assert that that index (i.e. the injected
prefix length) is <= 1024; ensure you also keep the existing checks for the
system-prompt sections ("ADE control protocol", "Cursor Cloud capability",
"runtime: cloud") so the test fails if the user text is missing or the injected
prefix is too large.
- Around line 14-15: The test saves ORIGINAL_CURSOR_API_KEY but fails to restore
ADE_CURSOR_PROMPT_INJECT, leaking env state; update the test teardown
(afterEach/teardown block) to capture the original
process.env.ADE_CURSOR_PROMPT_INJECT at top of the file (similar to
ORIGINAL_CURSOR_API_KEY) and restore it in the afterEach(), ensuring both
ORIGINAL_CURSOR_API_KEY and the saved ADE_CURSOR_PROMPT_INJECT are reset; apply
the same restore logic for the other teardown area referenced around the 942-948
block so no tests remain order-dependent.
In `@apps/desktop/src/main/services/chat/cursorSdkEventMapper.test.ts`:
- Around line 184-190: The test currently only checks done.status but is named
to verify runtime forwarding; update the expectation to assert that the runtime
from the Cursor SDK input is preserved in the mapped event (e.g., verify that
the result of mapCursorSdkRunResultToDoneEvent contains runtime === "cloud" in
the appropriate meta/location—refer to the mapped object returned by
mapCursorSdkRunResultToDoneEvent and the local variable done when adding the
assertion).
In `@apps/desktop/src/main/services/chat/cursorSdkEventMapper.ts`:
- Around line 119-121: The fallback ID generation in the "tool_call" case
(variable callId built from readString(record.call_id) ?? readString(record.id)
?? `cursor-sdk-tool-${Date.now()}`) can collide when multiple events share the
same millisecond; change the fallback to append a high-entropy suffix (e.g., a
UUID or secure random hex) to Date.now() to guarantee uniqueness (for example
use crypto.randomUUID() or a secure random value) and keep the rest of the logic
(readString(record.call_id) and readString(record.id)) intact; update callId
assignment in the "tool_call" branch and ensure any downstream code that expects
that ID continues to accept the new format.
- Around line 214-216: The error branch for statusText === "ERROR" returns an
event without applying tagRuntime; update that return to wrap the error event
with tagRuntime just like other branches (use tagRuntime({ type: "error",
message: detail ?? "Cursor SDK run failed.", turnId }) or the same call
signature used elsewhere) so the runtime tagging is consistent; modify the code
in the statusText === "ERROR" block to call tagRuntime on the constructed event.
In `@apps/desktop/src/renderer/lib/modelOptions.ts`:
- Around line 192-195: The current cursor auth check uses a raw provider string
and can miss variants like "Cursor" or "cursor/xyz"; update the cursorAuth
computation (the const cursorAuthed that checks status.detectedAuth?.some) to
normalize the provider (e.g., trim and toLowerCase()) and check for either exact
"cursor" or startsWith("cursor") to cover dynamic providers, so the subsequent
availableModelIds handling (status.availableModelIds) correctly includes
cursor/* models.
In `@plans/cursor-sdk-chat-integration.md`:
- Around line 670-671: The two statements conflict: one says ACP can be enabled
via override and another says ACP fallback/transport were removed; reconcile by
choosing a single truth and editing both occurrences — either explicitly state
“Cursor SDK is the default and only active Cursor chat path; ACP
fallback/transport has been removed and cannot be enabled” or instead allow ACP
override by updating the SDK line to “Cursor SDK is the default; ACP may be
enabled only via explicit override” — make the same wording change in the
sentences that currently read “Cursor SDK is the default and only active Cursor
chat path unless ACP override is explicitly set.” and “Local Cursor SDK chat can
answer, inspect, edit, run shell, stream, and cancel.” (clarify whether ACP
supports any of those features if you keep ACP override), and remove or adjust
any other mentions of “ACP fallback/transport” to match the chosen single source
of truth.
---
Nitpick comments:
In `@apps/ade-cli/src/cursorCloud.ts`:
- Around line 458-460: The unconditional newline after processing assistant text
blocks (the printText write at the end) can create extra blank lines when
multiple assistant events arrive; update the assistant-event handling in
cursorCloud.ts to only emit a trailing newline when actual text was written and
it doesn't already end with a newline—e.g., add a boolean flag (wroteText) set
when writing any chunk (or inspect the last written character) and change the
final conditional to: if (wroteText && lastChar !== '\n') emit a single newline;
alternatively remove the unconditional newline if the SDK guarantees trailing
newlines in content. Ensure the change touches the block using the printText
variable in the assistant response handling so you don't add newlines when
nothing was printed.
In `@apps/desktop/src/main/services/ai/aiIntegrationService.ts`:
- Around line 1103-1125: The cleanup try/catch pattern used in
listCursorCloudArtifacts (and the similar block at 1127-1153) is repetitive;
extract a helper (e.g., disposeAgent) that accepts the cloud agent object and
encapsulates the defensively-ordered disposal logic: attempt await (agent as {
[Symbol.asyncDispose]?: () => Promise<void> })[Symbol.asyncDispose]?.(), and on
failure fallback to (agent as { close?: () => void }).close?.(), swallowing
errors as before; then replace the duplicated finally blocks in
listCursorCloudArtifacts (and the other occurrence) with a single call to
disposeAgent(agent).
- Around line 365-377: The helper readNumber currently accepts numbers and ISO
date strings via Date.parse, which is surprising compared to toNumberOrNull;
either rename readNumber to readNumberOrTimestamp to make its date-parsing
behavior explicit (update all callers like the normalization code handling
lastModified/createdAt to use the new name), or remove date parsing from
readNumber and move Date.parse logic into the specific callers that expect
timestamps (e.g., the normalization path for lastModified/createdAt) so
readNumber/toNumberOrNull remain consistent; pick one approach and update
function name/usages or relocate the date parsing accordingly.
- Around line 1027-1064: createCursorCloudRun currently calls
agent.send(promptText) with no timeout, risking an indefinite hang; wrap the
send call with a timeout wrapper (e.g., Promise.race between agent.send(...) and
a timeout promise) that rejects after a sensible default (or an optional
args.timeoutMs) so the function fails fast on unresponsive Cursor Cloud. Update
createCursorCloudRun to use that wrapper when awaiting the run (preserve
existing behavior on success), throw a clear timeout Error on expiry, and ensure
downstream code still passes the resolved run into
normalizeCursorCloudRun(agentId) as before.
In `@apps/desktop/src/main/services/chat/cursorSdkPool.ts`:
- Around line 289-305: When handling message.type === "hook_request" the code
currently returns a default deny decision when bridge.onHookRequest is missing
but doesn't log that no hook handler was registered; update the async block
around bridge.onHookRequest to detect when bridge.onHookRequest is falsy and
emit a concise log (e.g., via your service logger or console) indicating that no
onHookRequest handler is registered and a default deny was returned, while
keeping the existing decision variable, child.send payload, and behavior intact
(refer to bridge.onHookRequest, decision, child.send and the "hook_request"
branch to locate where to add the log).
In `@apps/desktop/src/main/services/chat/cursorSdkSystemPrompt.ts`:
- Around line 203-230: Move the dynamic require("node:fs") out of
findAdeCliHelpDigestFile and import or require the sync API at module scope
(e.g., import * as fsSync from "node:fs" or const fsSync = require("node:fs") at
top) so the function uses the top-level fsSync.existsSync directly; update
findAdeCliHelpDigestFile to remove the inline require and call
fsSync.existsSync(c) in the loop.
- Line 11: The current SECRET_RE (/[A-Za-z0-9+/=_\-]{40,}/) is too permissive
and causes false positives; replace it with a stricter matcher that targets
known secret prefixes and formats (e.g., require prefixes like sk-, ghp_, AKIA,
or other provider-specific tokens) or maintain an array of specific regexes
(e.g., SECRET_PATTERNS = [ /^(sk-[A-Za-z0-9]{32,})$/,
/^(ghp_[A-Za-z0-9]{36,})$/, /^(AKIA[0-9A-Z]{16})$/ ]) and use those to detect
secrets instead of the broad SECRET_RE, and add a short comment above the
pattern(s) documenting the limitation and rationale for chosen prefixes/patterns
so legitimate long strings (base64 blobs, long URLs, file paths) are not
stripped.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 1973-1991: The cloud-submit branch currently calls
onSubmitToCloud(trimmed) and only clears the draft on successful resolution,
leaving the composer interactive during the async gap; add a short-lived
"cloudSubmitting" state flag (or reuse/extend existing busy/parallelLaunchBusy)
that is set true immediately before invoking onSubmitToCloud(trimmed) and reset
in both then and catch, use that flag to disable input and prevent re-submits
(include it in the early-return guard alongside busy/modelId/draft checks), and
ensure onDraftChange("") is still called only on success; update the effect
dependency array to include the new flag so the UI correctly reflects the
submitting state.
In `@apps/desktop/src/renderer/components/chat/ChatCursorCloudPanel.tsx`:
- Around line 162-212: The refresh callback is being recreated whenever the
repos state changes because repos is listed in its dependency array; avoid this
by removing repos from the dependencies and reading the latest repos via a ref
instead—create a reposRef (e.g., const reposRef = useRef(repos)), update
reposRef.current whenever you call setRepos or on mount, and inside refresh use
reposRef.current wherever repos is currently referenced (keep reposLoadedRef for
loaded-checks and leave other deps like defaultRepoUrl, includeArchived,
laneGitRemote, modelOptions as-is or ensure modelOptions is stable). This
preserves correct behavior while preventing refresh from being rebuilt on every
setRepos.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+1142
to
+1195
| cursorCloudListRepositories: async (): Promise<CursorCloudRepository[]> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudListRepositories), | ||
| cursorCloudListAgents: async (args?: { | ||
| includeArchived?: boolean; | ||
| limit?: number; | ||
| cursor?: string | null; | ||
| }): Promise<CursorCloudListAgentsResult> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudListAgents, args ?? {}), | ||
| cursorCloudListRuns: async (args: { | ||
| agentId: string; | ||
| limit?: number; | ||
| cursor?: string | null; | ||
| }): Promise<CursorCloudListRunsResult> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudListRuns, args), | ||
| cursorCloudCreateRun: async ( | ||
| args: CursorCloudCreateRunRequest, | ||
| ): Promise<CursorCloudCreateRunResult> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudCreateRun, args), | ||
| cursorCloudArchiveAgent: async (agentId: string): Promise<void> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudArchiveAgent, { agentId }), | ||
| cursorCloudUnarchiveAgent: async (agentId: string): Promise<void> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudUnarchiveAgent, { agentId }), | ||
| cursorCloudDeleteAgent: async (agentId: string): Promise<void> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudDeleteAgent, { agentId }), | ||
| cursorCloudGetAgent: async ( | ||
| agentId: string, | ||
| ): Promise<CursorCloudAgentSummary | null> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudGetAgent, { agentId }), | ||
| cursorCloudStreamRun: async ( | ||
| args: CursorCloudStreamRunRequest, | ||
| ): Promise<CursorCloudStreamRunResult> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudStreamRun, args), | ||
| cursorCloudCancelRun: async (args: { | ||
| agentId: string; | ||
| runId: string; | ||
| }): Promise<void> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudCancelRun, args), | ||
| cursorCloudFollowUp: async ( | ||
| args: CursorCloudFollowUpRequest, | ||
| ): Promise<CursorCloudFollowUpResult> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudFollowUp, args), | ||
| cursorCloudListArtifacts: async ( | ||
| agentId: string, | ||
| ): Promise<CursorCloudArtifactSummary[]> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudListArtifacts, { agentId }), | ||
| cursorCloudDownloadArtifact: async (args: { | ||
| agentId: string; | ||
| path: string; | ||
| }): Promise<CursorCloudArtifactDownload> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudDownloadArtifact, args), | ||
| cursorCloudOpenChat: async ( | ||
| args: CursorCloudOpenChatRequest, | ||
| ): Promise<CursorCloudOpenChatResult> => | ||
| ipcRenderer.invoke(IPC.aiCursorCloudOpenChat, args), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Required desktop validations
npm --prefix apps/desktop run typecheck
npm --prefix apps/desktop run lint
npm --prefix apps/desktop run buildRepository: arul28/ADE
Length of output: 530
Fix TypeScript configuration errors before IPC contract changes can be validated.
Typecheck failed with two blocking errors that must be resolved:
- Missing type definitions for 'node' — Ensure
@types/nodeis installed and properly referenced in tsconfig.json - Deprecated
baseUrloption — Update tsconfig.json line 7 to include"ignoreDeprecations": "6.0"for TypeScript 7.0 compatibility
Once these are fixed, rerun npm --prefix apps/desktop run typecheck, npm --prefix apps/desktop run lint, and npm --prefix apps/desktop run build to validate that the 15+ new window.ade.ai.cursorCloud* preload methods are properly synced with IPC contracts, preload types, and shared types as required by the desktop app guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/preload/preload.ts` around lines 1142 - 1195, Typecheck is
failing because Node types are missing and tsconfig is emitting a deprecation
error for baseUrl; install and reference `@types/node` (e.g., add to
devDependencies and ensure tsconfig "types" or typeRoots include "node") and
update tsconfig.json compilerOptions to include "ignoreDeprecations": "6.0" to
silence the TypeScript 7.0 baseUrl deprecation, then re-run npm --prefix
apps/desktop run typecheck, lint, and build; while doing this, verify the newly
added preload methods (cursorCloudListRepositories, cursorCloudListAgents,
cursorCloudListRuns, cursorCloudCreateRun, cursorCloudArchiveAgent,
cursorCloudUnarchiveAgent, cursorCloudDeleteAgent, cursorCloudGetAgent,
cursorCloudStreamRun, cursorCloudCancelRun, cursorCloudFollowUp,
cursorCloudListArtifacts, cursorCloudDownloadArtifact, cursorCloudOpenChat) have
matching IPC constants (IPC.aiCursorCloud*) and corresponding types in the
preload types and shared IPC contract files so the preload → IPC → renderer type
contracts remain in sync.
Comment on lines
+2637
to
+2638
| getOriginRemote: async (args: { laneId: string }): Promise<{ remoteUrl: string | null; branch: string | null }> => | ||
| ipcRenderer.invoke(IPC.gitGetOriginRemote, args), |
There was a problem hiding this comment.
Redact credentials before returning remoteUrl to renderer.
remoteUrl can contain embedded credentials/userinfo. Exposing it over preload increases secret-leak risk in renderer space. Return a sanitized URL (strip userinfo) or structured non-secret fields instead.
As per coding guidelines: "Electron desktop app — check for IPC security, proper main/renderer process separation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/preload/preload.ts` around lines 2637 - 2638, The
getOriginRemote IPC handler (getOriginRemote) currently returns remoteUrl which
may include embedded credentials; sanitize the value in the preload before
returning to the renderer by stripping any userinfo (username:password@) from
the URL or returning only non-secret structured fields (e.g., protocol, host,
path, branch) instead; locate the getOriginRemote implementation and replace the
direct ipcRenderer.invoke(IPC.gitGetOriginRemote, args) result with a sanitized
object where remoteUrl has had userinfo removed (or is null) while preserving
branch and other safe fields, using IPC.gitGetOriginRemote as the source of
truth.
Review fixes (PR #223, 20 of 25 threads): - Extract repoMatchKey into shared cursorCloudUtils - Validate agentId/runId at aiCursorCloudStreamRun IPC boundary - Distinguish cursorStoreUnavailable from missing-key in providerConnectionStatus - Decouple runtimeAvailable/path from auth in cursor SDK status - Wrap run.wait() and Cursor.me with timeouts and proper cleanup - Use outputMode as fallback in runCursorCloud - Pack @cursor/sdk-darwin-x64 in mac.x64ArchFiles - Throw (not stub) when dist/cli.cjs missing in regen-ade-cli-help - Match repo before attaching PR url in CursorCloudInlineLaunch - Replace cancelledRef with effect-scoped flag and set reposLoaded on error - Guard typeof for laneId/branch in git IPC; strip credentials from remote URL - Route cloud-status PR link through openExternalUrl - Try/catch listCursorModelsFromSdk; treat top-level string as path candidate - Clear stale CURSOR_API_KEY when reused init has none - Drain hookWaiters with denyCursorHook on cancelRun ade prs comments bug: - pr_get_review_comments now also fetches getReviewThreads - bot-author filter scoped to source==issue (review-thread bots are the signal) - summary includes actionableReviewThreadCount
…t gate - cursorSdkPool: dispose forked worker if init request fails (P1 leak) - aiOrchestratorService.test: replace polling with Promise gate, raise budget to 60s wait + 90s test timeout for slow shard 8 runners
There was a problem hiding this comment.
Actionable comments posted: 8
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/orchestrator/aiOrchestratorService.test.ts (1)
3274-3307:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCancel the fallback timeout when the gated reconcile resolves.
Line 3289-Line 3294 creates a 180s timeout inside
Promise.racethat is never cleared whenfirstReconcileEnteredresolves first. This can leave a pending timer handle and cause test-process flakiness/open-handle drag in CI.Suggested fix
let reconcileCalls = 0; let resolveFirstReconcile!: () => void; const firstReconcileEntered = new Promise<void>((resolve) => { - resolveFirstReconcile = resolve; + const timeout = setTimeout( + () => { + throw new Error("first reconcile did not enter within 180s"); + }, + 180_000 + ); + resolveFirstReconcile = () => { + clearTimeout(timeout); + resolve(); + }; }); fixture.orchestratorService.onTrackedSessionEnded = (async (args: any) => { reconcileCalls += 1; if (reconcileCalls === 1) resolveFirstReconcile(); await firstSweepGate; return await originalReconcile(args); }) as typeof fixture.orchestratorService.onTrackedSessionEnded; const firstSweep = fixture.aiOrchestratorService.runHealthSweep("overlap-owner"); -// Wait deterministically for the gated reconcile to enter, with a hard -// ceiling well under the test timeout. CI runners can be very contended; -// a Promise-based wait is more reliable than polling. -await Promise.race([ - firstReconcileEntered, - new Promise<void>((_, reject) => - setTimeout(() => reject(new Error("first reconcile did not enter within 180s")), 180_000) - ), -]); +await firstReconcileEntered;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.ts` around lines 3274 - 3307, The Promise.race that waits for firstReconcileEntered creates a 180s setTimeout that is never cleared; store the timeout id (e.g., const timeoutId = setTimeout(...)) and ensure clearTimeout(timeoutId) runs when the race completes (whether the resolve or the timeout wins) — for example clear the timer immediately after awaiting the Promise.race or in a finally block; update the test around firstReconcileEntered / Promise.race in aiOrchestratorService.test.ts so the timeout is cancelled to avoid leaking a pending timer (references: firstReconcileEntered, Promise.race, runHealthSweep, firstSweep, releaseFirstSweep).
♻️ Duplicate comments (1)
apps/desktop/src/main/services/ai/providerTaskRunner.ts (1)
268-282:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the Cursor sandbox aligned with
permissionMode.Lines 272 and 278 disable sandboxing for every local Cursor run, and Line 282 repurposes
local.forceas a permission switch. That givesplan/editruns the same local privileges asfull-autoand can unexpectedly expire persisted runs just because the caller asked for full-auto.For the latest `@cursor/sdk` docs, what do `local.sandboxOptions.enabled` on `Agent.create` / `Agent.resume` and `local.force` on `agent.send()` control for local agents? Is `local.force` intended for crash recovery / expiring persisted runs rather than permission-mode selection, and is sandboxing recommended by default for local agents?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/providerTaskRunner.ts` around lines 268 - 282, The sandboxing and force flags are currently hard-disabled for all local Cursor runs; change Agent.create and Agent.resume to set local.sandboxOptions.enabled based on args.permissionMode (enable sandbox unless args.permissionMode === "full-auto"), and likewise pass sandboxOptions.enabled into agent.send so sandbox state matches the creation/resume call; keep local.force only for true crash/expiry semantics and set local.force = (args.permissionMode === "full-auto") if you need to preserve the existing behavior, but do not rely on local.force as the primary permission switch—use local.sandboxOptions.enabled driven by args.permissionMode instead (update references in Agent.create, Agent.resume, and agent.send).
🤖 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/ade-cli/src/adeRpcServer.ts`:
- Around line 5609-5615: The Promise.all call makes prSvc.getReviewThreads()
failure abort the whole operation; change this so review threads are treated as
additive: call prSvc.getComments(prId), prSvc.getReviews(prId),
prSvc.getChecks(prId) as before but isolate prSvc.getReviewThreads(prId) (either
via Promise.allSettled or a try/catch around prSvc.getReviewThreads) and on
error log the failure and substitute an empty array for reviewThreads before
calling summarizePrReviewComments(prId, comments, reviews, checks,
reviewThreads); keep references to the existing symbols prSvc.getReviewThreads,
prSvc.getComments, prSvc.getReviews, prSvc.getChecks, and
summarizePrReviewComments.
In `@apps/desktop/src/main/services/chat/cursorModelsDiscovery.ts`:
- Around line 78-85: Normalize the API key once and reuse it: compute a
trimmed/normalized value (e.g., normalizedApiKey = apiKey?.trim() || undefined),
pass that normalizedApiKey into hashKeyForCache instead of the raw apiKey, and
use the same normalizedApiKey when calling Cursor.models.list and when storing
sdkCached.keyHash; update references in the cache check (sdkCached.keyHash), the
hashKeyForCache call, and the Cursor.models.list invocation so whitespace-only
differences no longer bypass the cache.
In `@apps/desktop/src/main/services/chat/cursorSdkPool.ts`:
- Around line 289-304: The async IIFE handling message.type === "hook_request"
can throw and currently swallows errors causing the worker to never get a
response; wrap the body of that IIFE in a try/catch so any exception from
bridge.onHookRequest is caught and you still call child.send with a
hook_response (same shape as CursorSdkWorkerRequest) containing a safe deny
decision and include minimal error info (or a generic
agent_message/user_message) so the worker never hangs; ensure you reference the
bridge.onHookRequest call, the async IIFE, and child.send to locate and update
the code.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 6507-6510: The current Promise.all usage around runGit calls
(creating remoteRes and branchRes) will reject if either runGit throws,
converting this handler into an IPC error instead of returning the intended
fallback { remoteUrl: null, branch: null }; change this to independently await
or use Promise.allSettled for the two runGit calls (the remote get-url and the
rev-parse HEAD branch lookup) and then map failures to null so remoteRes and
branchRes are null on errors/timeouts; update the code paths around runGit([...
"remote", "get-url", "origin"]...) and runGit([... "rev-parse", "--abbrev-ref",
"HEAD"]...) so that any rejection results in a null value rather than throwing.
In `@apps/desktop/src/renderer/components/chat/AgentChatMessageList.tsx`:
- Around line 2753-2788: The code treats any event with status "creating" or
"running" as permanently live by using the single `live` flag; instead split
into `inProgress` = status === "creating" || status === "running" and `isLive` =
inProgress && options?.turnActive (or similar prop that indicates the current
active turn). Use `inProgress` for non-animated styling/tone/labels (so earlier
rows still show "in-progress" state visually) but gate the pulsing animation and
the live-only background override on `isLive`; update usages of `live` in the
component (the variables `live`, `tone`, the inline style and the pulsing span)
to reference `inProgress` where you want passive styling and `isLive` where you
want the animation/background.
In `@apps/desktop/src/renderer/components/chat/CursorCloudInlineLaunch.tsx`:
- Around line 127-151: When laneId becomes falsy the useEffect currently returns
early leaving stale branch state; update the early-return branch of the
useEffect to clear previous data by calling setBranches([]) and
setBranchesLoaded(false) (keep the same effect body, cancellation flag and
cleanup), so that when laneId disappears the dropdown and startingRef options
are reset; refer to the useEffect closure that reads laneId and calls
setBranches/setBranchesLoaded and the cancelled boolean for placement of these
calls.
- Around line 267-438: The UI still treats detectedPr as active even when the
user picks a different repo; compute a boolean like showDetectedPr that is true
only when detectedPr exists AND the selected repoUrl matches the repo that
produced the PR, then use showDetectedPr in place of detectedPr in the render
paths and in prPillLabel/prPillTitle calculation so the PR pill and PR-only
buttons are hidden when the selected repo no longer matches (check
detectedPr.repoUrl or whatever field identifies the PR's repo to compare against
repoUrl); no change to launchWithPrompt is required.
In `@apps/desktop/src/renderer/lib/cursorCloudUtils.ts`:
- Around line 21-22: The normalization currently removes a trailing ".git"
before stripping trailing slashes, which leaves inputs like "repo.git/"
unchanged; update the cleanup order in the cursorCloudUtils.ts normalization so
you strip trailing slashes first and then remove a trailing ".git" (i.e., call
replace(/\/+$/,"") before replace(/\.git$/i,"")), keeping the final
toLowerCase() call; adjust the single assignment to variable s accordingly to
ensure inputs like "https://github.com/org/repo.git/" normalize to
"https://github.com/org/repo".
---
Outside diff comments:
In `@apps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.ts`:
- Around line 3274-3307: The Promise.race that waits for firstReconcileEntered
creates a 180s setTimeout that is never cleared; store the timeout id (e.g.,
const timeoutId = setTimeout(...)) and ensure clearTimeout(timeoutId) runs when
the race completes (whether the resolve or the timeout wins) — for example clear
the timer immediately after awaiting the Promise.race or in a finally block;
update the test around firstReconcileEntered / Promise.race in
aiOrchestratorService.test.ts so the timeout is cancelled to avoid leaking a
pending timer (references: firstReconcileEntered, Promise.race, runHealthSweep,
firstSweep, releaseFirstSweep).
---
Duplicate comments:
In `@apps/desktop/src/main/services/ai/providerTaskRunner.ts`:
- Around line 268-282: The sandboxing and force flags are currently
hard-disabled for all local Cursor runs; change Agent.create and Agent.resume to
set local.sandboxOptions.enabled based on args.permissionMode (enable sandbox
unless args.permissionMode === "full-auto"), and likewise pass
sandboxOptions.enabled into agent.send so sandbox state matches the
creation/resume call; keep local.force only for true crash/expiry semantics and
set local.force = (args.permissionMode === "full-auto") if you need to preserve
the existing behavior, but do not rely on local.force as the primary permission
switch—use local.sandboxOptions.enabled driven by args.permissionMode instead
(update references in Agent.create, Agent.resume, and agent.send).
🪄 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: 8fd2f762-2e0a-49f1-95f8-886723f1963e
📒 Files selected for processing (18)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/cursorCloud.tsapps/desktop/package.jsonapps/desktop/scripts/regen-ade-cli-help.cjsapps/desktop/src/main/services/ai/authDetector.tsapps/desktop/src/main/services/ai/providerConnectionStatus.tsapps/desktop/src/main/services/ai/providerTaskRunner.tsapps/desktop/src/main/services/chat/cursorModelsDiscovery.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/ipc/registerIpc.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.tsapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/ChatCursorCloudPanel.tsxapps/desktop/src/renderer/components/chat/CursorCloudInlineLaunch.tsxapps/desktop/src/renderer/lib/cursorCloudUtils.ts
✅ Files skipped from review due to trivial changes (2)
- apps/ade-cli/src/cursorCloud.ts
- apps/desktop/src/renderer/components/chat/ChatCursorCloudPanel.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/scripts/regen-ade-cli-help.cjs
- apps/desktop/src/main/services/chat/cursorSdkWorker.ts
- agentChatService: replace spawnSync git with async runGit (P1) - cursorModelsDiscovery: normalize API key once before hash + SDK call - cursorSdkPool: try/catch around onHookRequest so failures still deny - registerIpc gitGetOriginRemote: null-safe runGit catches - AgentChatMessageList: split inProgress from live to gate pulsing - CursorCloudInlineLaunch: clear branches when laneId clears; gate PR pill visibility on selected repo matching lane repo - cursorCloudUtils: strip trailing slashes before .git in repoMatchKey - adeRpcServer: getReviewThreads catches to [] so summary doesn't fail
Comment on lines
+562
to
+583
| if (req.type === "cloud.send.stream") { | ||
| const cloudAgent = await Agent.create(buildCloudCreateOptions(req.payload)); | ||
| const sendOpts = req.payload.modelSdkId?.trim() ? { model: { id: req.payload.modelSdkId.trim() } } : undefined; | ||
| const run = await cloudAgent.send(req.payload.promptText, sendOpts); | ||
| return streamCloudRun({ | ||
| requestId: req.requestId, | ||
| agentId: cloudAgent.agentId, | ||
| run, | ||
| modelSdkId: req.payload.modelSdkId, | ||
| }); | ||
| } | ||
| if (req.type === "cloud.followup") { | ||
| const cloudAgent = await Agent.resume(req.payload.agentId, buildCloudResumeOptions(req.payload)); | ||
| const sendOpts = req.payload.modelSdkId?.trim() ? { model: { id: req.payload.modelSdkId.trim() } } : undefined; | ||
| const run = await cloudAgent.send(req.payload.promptText, sendOpts); | ||
| return streamCloudRun({ | ||
| requestId: req.requestId, | ||
| agentId: cloudAgent.agentId, | ||
| run, | ||
| modelSdkId: req.payload.modelSdkId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
cloudAgent SDK objects leaked in streaming paths
cloud.send.stream (line 563) and cloud.followup (line 574) each create a cloudAgent via Agent.create/Agent.resume, obtain a run from it, and then pass only the run into streamCloudRun. The cloudAgent reference is never disposed — no Symbol.asyncDispose or .close() call is made, even in the error path where cloudAgent.send() throws before streamCloudRun is entered.
The contrast with cloud.artifacts.list and cloud.artifacts.download (lines 620–657) is direct: both use a try/finally to call cloudAgent[Symbol.asyncDispose]?.(). If the SDK needs explicit cleanup (and Symbol.asyncDispose strongly implies it does), each cloud.send.stream or cloud.followup invocation leaks a connection/state object for the lifetime of the worker process.
Fix by wrapping both paths in a try/finally:
const cloudAgent = await Agent.create(buildCloudCreateOptions(req.payload));
try {
const run = await cloudAgent.send(req.payload.promptText, sendOpts);
return await streamCloudRun({ requestId: req.requestId, agentId: cloudAgent.agentId, run, modelSdkId: req.payload.modelSdkId });
} finally {
try { await cloudAgent[Symbol.asyncDispose]?.(); } catch { try { cloudAgent.close(); } catch { /* ignore */ } }
}Apply the same pattern to the cloud.followup branch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/chat/cursorSdkWorker.ts
Line: 562-583
Comment:
**`cloudAgent` SDK objects leaked in streaming paths**
`cloud.send.stream` (line 563) and `cloud.followup` (line 574) each create a `cloudAgent` via `Agent.create`/`Agent.resume`, obtain a `run` from it, and then pass only the run into `streamCloudRun`. The `cloudAgent` reference is never disposed — no `Symbol.asyncDispose` or `.close()` call is made, even in the error path where `cloudAgent.send()` throws before `streamCloudRun` is entered.
The contrast with `cloud.artifacts.list` and `cloud.artifacts.download` (lines 620–657) is direct: both use a `try/finally` to call `cloudAgent[Symbol.asyncDispose]?.()`. If the SDK needs explicit cleanup (and `Symbol.asyncDispose` strongly implies it does), each `cloud.send.stream` or `cloud.followup` invocation leaks a connection/state object for the lifetime of the worker process.
Fix by wrapping both paths in a `try/finally`:
```typescript
const cloudAgent = await Agent.create(buildCloudCreateOptions(req.payload));
try {
const run = await cloudAgent.send(req.payload.promptText, sendOpts);
return await streamCloudRun({ requestId: req.requestId, agentId: cloudAgent.agentId, run, modelSdkId: req.payload.modelSdkId });
} finally {
try { await cloudAgent[Symbol.asyncDispose]?.(); } catch { try { cloudAgent.close(); } catch { /* ignore */ } }
}
```
Apply the same pattern to the `cloud.followup` branch.
How can I resolve this? If you propose a fix, please make it concise.
This was referenced May 1, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cursorSdkWorker,cursorSdkProtocol).cursorAcpConfigState,cursorAcpEventMapper,cursorAcpPool,cursorAgentExecutable).maininto the lane and resolve conflicts (cli help, IPC, preload, AgentChat surfaces, ARCHITECTURE doc).Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
This PR replaces the Cursor ACP-based chat integration with a worker-based Cursor SDK implementation, adds a full Cursor Cloud panel and inline launch UI, drops the now-dead ACP files, and resolves merge conflicts. Several issues flagged in the prior review round have been addressed:
repoMatchKeyis now shared incursorCloudUtils.ts,hookWaitersare drained oncancelRun, the child-process leak on init failure is patched, anddetectLaneGitRemoteUrlis now fully async.cursorSdkWorker.tslines 562–582):cloud.send.streamandcloud.followupcreate acloudAgentviaAgent.create/Agent.resumebut never callSymbol.asyncDisposeorclose()on it, even thoughcloud.artifacts.listandcloud.artifacts.downloaduse an explicittry/finallyfor the same cleanup. Each streaming invocation leaks an SDK connection object for the lifetime of the worker process.Confidence Score: 4/5
Safe to merge with the cloud agent leak addressed; leaked objects accumulate per worker but won't cause immediate crashes.
One P1 present (cloud agent SDK objects never disposed in streaming paths). Several previously-flagged issues were resolved in this round. No P0s found.
apps/desktop/src/main/services/chat/cursorSdkWorker.ts — cloud.send.stream and cloud.followup branches need a try/finally to dispose the cloudAgent.
Important Files Changed
Sequence Diagram
sequenceDiagram participant R as Renderer participant IPC as registerIpc (Main) participant ACS as agentChatService participant Pool as cursorSdkPool participant W as cursorSdkWorker (fork) participant SDK as @cursor/sdk R->>IPC: aiCursorSendMessage(prompt) IPC->>ACS: runCursorSdkTurn() ACS->>Pool: acquireCursorSdkConnection(poolKey) Pool->>W: fork() + init(CursorSdkWorkerInit) W->>SDK: Agent.create() / Agent.resume() SDK-->>W: agent W-->>Pool: { agentId } Pool-->>ACS: { pooled, generation } ACS->>W: send(prompt) W->>SDK: agent.send(message) SDK-->>W: run loop stream events W-->>IPC: sdk_event (via process.send) IPC-->>R: aiCursorSessionEvent end W-->>IPC: run_result IPC-->>R: aiCursorSessionEvent(done) Note over R,IPC: Cloud path R->>IPC: cursorCloudCreateRun(prompt, repoUrl) IPC->>ACS: runCursorCloudTurn() ACS->>Pool: runCursorSdkCloudRequest(cloud.send.stream) Pool->>W: fork() [one-shot worker] W->>SDK: Agent.create({ cloud: { repos } }) SDK-->>W: cloudAgent W->>SDK: cloudAgent.send(prompt) SDK-->>W: run loop cloud stream W-->>IPC: sdk_event(runtime=cloud) IPC-->>R: aiCursorSessionEvent end W-->>IPC: run_result(cloud) Note over W: ⚠ cloudAgent never disposedPrompt To Fix All With AI
Reviews (6): Last reviewed commit: "fix(cursor-sdk): address re-review feedb..." | Re-trigger Greptile