Cursor chat: surface SDK errors, sync permission mode across clients, consolidate mobile approvals#716
Conversation
…ents, consolidate mobile approvals Chat / Cursor SDK (desktop + CLI + iOS): - Surface the real Cursor run failure reason: read the run store's errorCode on a terminal ERROR and render "Cursor run failed: <code>" instead of the generic "Cursor SDK run failed."; classify NGHTTP2/ECONNRESET-class transport failures as errorInfo.category="network" and log agent_chat.cursor_sdk_run_error. - Broadcast permission/mode changes to every client: updateSession now emits a session_meta_updated carrying the permission/interaction/cursorMode fields (incl. recomputed cursorModeSnapshot) so desktop, TUI, and iOS composers update live when any client re-modes a session, instead of going stale until a turn ends. Desktop (AgentChatPane), TUI (tuiClient/app.tsx), and iOS all patch on receipt. iOS Work: - Composer no longer blanks its model/permission controls mid-session: the composer summary is coalesced + latched and the summary cache merges instead of replacing, so a push/deeplink nav @State reset can't drop the open session's summary. - Consolidated pending-input strip pinned above the composer ("Request 1 of N" + Accept-all sweep) replaces the scattered per-request inline approval cards. Sync / lane naming (concurrent-session work bundled per request): - Persist peer app provenance (appVersion/appBuild/bundleIdentifier) in device metadata. - Lane-name heuristic excludes "login history"; auto-title prefers the configured title model before the requested composer model. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThis PR adds application provenance metadata (appVersion/appBuild/bundleIdentifier) to sync peer handshakes, adds structured logging and retry-based orchestration to lane-name suggestion/auto-rename flows, propagates session permission/mode changes via extended session_meta_updated events across desktop and iOS, classifies Cursor SDK transport errors with a new errorCode field, and replaces iOS per-type pending-input timeline cards with a consolidated pending-input strip supporting optimistic answering and accept-all sweeping. ChangesApp identity metadata propagation
Lane and session naming suggestions
session_meta_updated mode propagation
Cursor SDK transport error classification
iOS consolidated pending input strip
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
@copilot review but do not make fixes |
…meta_updated Addresses two Greptile P1 findings on the mode-broadcast path: - Config values dropped: a cursorConfigValues-only updateSession emitted session_meta_updated without the new config (and the snapshot recompute is skipped for config-only changes), so other clients ran the next Cursor turn with stale options. The event now carries cursorConfigValues (incl. explicit null on clear); desktop AgentChatPane and iOS patch it, with an explicit value taking precedence over snapshot-derived config. - Explicit cursorModeId: null clear ignored on iOS: decodeIfPresent collapsed absent-key and explicit-null, and applyModeUpdate skipped nil, so a host mode-clear never reached the composer. iOS now distinguishes present-but-null (via container.contains) from absent and applies the clear, while absent-key still means "no change". Scoped to cursorModeId only (interactionMode maps to a default, not a distinct cleared state). Adds testAgentChatMetaModeUpdateAppliesCursorConfigAndExplicitClear. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
…pdated consumer Sweeps the remaining instances of two bug classes across all clients that re-seed composer state from session_meta_updated: - cursorConfigValues now applied by the TUI handler (app.tsx) too, matching desktop and iOS, so a config-only change reaches every composer live. - Explicit cursorModeId:null (a mode clear) now propagates end to end: desktop AgentChatPane keys on "cursorModeId" in meta (present-null clears, absent = no change) instead of a stale ?? fallback; the TUI applies the same present-vs-absent distinction; iOS threads the cleared intent through the open-view reconcile (SyncService cursorModeClearedSessionIds + mergeModeFields(cursorModeIdCleared:)) so the clear reaches the live chatSummary, not just the cache. Absent-key still means no-change on every hop. Extends the iOS mode-update test to assert the clear reaches the live summary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bff4c9d46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cursorModeIdWasCleared = false | ||
| } | ||
| cursorModeSnapshot = try c.decodeIfPresent(RemoteJSONValue.self, forKey: .cursorModeSnapshot) | ||
| cursorConfigValues = try c.decodeIfPresent([String: RemoteJSONValue].self, forKey: .cursorConfigValues) |
There was a problem hiding this comment.
Handle null cursorConfigValues clears
When another client clears Cursor config overrides, the host emits session_meta_updated with cursorConfigValues: null (the desktop emitter does this when the update explicitly clears the normalized values). This decode uses decodeIfPresent, so an explicit null is indistinguishable from an absent key; hasAnyField stays false for config-only clears and applyModeUpdate never removes stale cursorConfigValues. In that case the iOS composer/cache keeps showing the old Cursor config until a full refresh or another non-null config update arrives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts (1)
145-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate provenance-parsing logic across two sync entry points.
The same appVersion/appBuild/bundleIdentifier extraction pattern (plus the pre-existing dbVersionBySite/capabilities normalization) is now duplicated in
normalizePeerMetadatahere and inparseHelloPayloadinapps/ade-cli/src/services/sync/syncHostService.ts(Lines 977-979). Consider extracting a sharednormalizePeerAppProvenance(record)helper so future peer-metadata fields only need to be added in one place.♻️ Example shared helper
export function normalizePeerAppProvenance(record: Record<string, unknown>): { appVersion?: string; appBuild?: string; bundleIdentifier?: string; } { const appVersion = optionalString(record.appVersion); const appBuild = optionalString(record.appBuild); const bundleIdentifier = optionalString(record.bundleIdentifier); return { ...(appVersion ? { appVersion } : {}), ...(appBuild ? { appBuild } : {}), ...(bundleIdentifier ? { bundleIdentifier } : {}), }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts` around lines 145 - 173, The appVersion/appBuild/bundleIdentifier parsing is duplicated between normalizePeerMetadata and parseHelloPayload, so add a shared helper for peer provenance normalization and reuse it in both places. Extract the optionalString-based extraction into a helper like normalizePeerAppProvenance, keep the existing dbVersionBySite/capabilities normalization where it belongs, and update both normalizePeerMetadata and syncHostService's parseHelloPayload to call the shared helper so future peer-metadata fields are maintained in one place.apps/desktop/src/main/services/chat/cursorSdkWorker.ts (1)
141-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider preferring the SDK's own error taxonomy over text matching for thrown errors.
For the thrown-
errorpath (as opposed to the storederrorCodestring path, where no structured object exists),@cursor/sdk'sCursorAgentErroralready exposesisRetryableand aNetworkErrorsubclass. Falling back toisCursorSdkTransportErrorText(detail)string matching only when those aren't checked first means this classification stays coupled to specific wire-error substrings ("nghttp2","econnreset", etc.) that could shift across SDK releases, whereaserror.isRetryable/error instanceof NetworkErroris the documented, stable signal for this.♻️ Possible approach
+ if (error instanceof CursorAgentError && error.isRetryable) { + return { error: errorMessage(error), errorCode: error.code ?? "network" }; + } const detail = errorMessage(error); if (isCursorSdkTransportErrorText(detail) || isCursorSdkTransportErrorText(errorCode(error))) { return { error: detail, errorCode: "network" }; }Please confirm
CursorAgentError/isRetryable/NetworkErrorare exported and usable from the pinned@cursor/sdk@1.0.13before adopting this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/cursorSdkWorker.ts` around lines 141 - 170, The thrown-error path in classifyWorkerError currently relies on text matching for transport failures; prefer the SDK’s structured error types first. Update cursorSdkWorker’s classifyWorkerError to check the `@cursor/sdk` error taxonomy on the error object itself, using CursorAgentError’s isRetryable and the NetworkError subclass before falling back to isCursorSdkTransportErrorText or errorCode string matching. Keep the existing agent_busy and rate_limited handling, but make sure the network classification is driven by the exported SDK symbols rather than wire-error substrings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ios/ADE/Views/Work/WorkChatSessionView`+Actions.swift:
- Around line 985-1000: acceptAllPendingInputs should stop sweeping if the
primary acceptForSession fails. Update the acceptAllPendingInputs flow in
WorkChatSessionView+Actions so the loop over acceptAllSweepableInputs only runs
after sendPendingInputDecision(primary, decision: .acceptForSession) succeeds;
if that call fails or returns unsuccessfully, exit early and do not send .accept
for the remaining items. Use the existing acceptAllPendingInputs,
sendPendingInputDecision, and optimistic state checks to keep the sweep
consistent with the primary decision.
- Around line 953-967: The rollback check in dispatchPendingInputAnswer is
relying on the shared errorMessage state, which can be changed by unrelated chat
actions. Update dispatchPendingInputAnswer to use a per-action success/failure
result from the awaited op or runSessionAction instead of inspecting
errorMessage, and only remove the itemId from optimisticallyAnsweredInputIds
when that specific action reports failure. Use the dispatchPendingInputAnswer
and runSessionAction symbols to locate the flow and keep the answer handler
logic isolated from global chat errors.
---
Nitpick comments:
In `@apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.ts`:
- Around line 145-173: The appVersion/appBuild/bundleIdentifier parsing is
duplicated between normalizePeerMetadata and parseHelloPayload, so add a shared
helper for peer provenance normalization and reuse it in both places. Extract
the optionalString-based extraction into a helper like
normalizePeerAppProvenance, keep the existing dbVersionBySite/capabilities
normalization where it belongs, and update both normalizePeerMetadata and
syncHostService's parseHelloPayload to call the shared helper so future
peer-metadata fields are maintained in one place.
In `@apps/desktop/src/main/services/chat/cursorSdkWorker.ts`:
- Around line 141-170: The thrown-error path in classifyWorkerError currently
relies on text matching for transport failures; prefer the SDK’s structured
error types first. Update cursorSdkWorker’s classifyWorkerError to check the
`@cursor/sdk` error taxonomy on the error object itself, using CursorAgentError’s
isRetryable and the NetworkError subclass before falling back to
isCursorSdkTransportErrorText or errorCode string matching. Keep the existing
agent_busy and rate_limited handling, but make sure the network classification
is driven by the exported SDK symbols rather than wire-error substrings.
🪄 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: c0c7e8d3-a80e-4201-bb70-7187e8f1f3f5
📒 Files selected for processing (30)
apps/ade-cli/src/services/sync/brainProjectActionsSyncHandler.tsapps/ade-cli/src/services/sync/deviceRegistryService.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/cursorSdkEventMapper.test.tsapps/desktop/src/main/services/chat/cursorSdkEventMapper.tsapps/desktop/src/main/services/chat/cursorSdkPool.tsapps/desktop/src/main/services/chat/cursorSdkProtocol.tsapps/desktop/src/main/services/chat/cursorSdkWorker.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/shared/laneNameFallback.test.tsapps/desktop/src/shared/laneNameFallback.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Hub/HubComposerDrawer.swiftapps/ios/ADE/Views/Work/WorkChatSessionView+Actions.swiftapps/ios/ADE/Views/Work/WorkChatSessionView+Timeline.swiftapps/ios/ADE/Views/Work/WorkChatSessionView.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkRootScreen+Actions.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADE/Views/Work/WorkTimelineHelpers.swiftapps/ios/ADETests/ADETests.swift
| @MainActor | ||
| func dispatchPendingInputAnswer( | ||
| itemId: String, | ||
| _ op: @escaping @MainActor () async -> Void | ||
| ) async { | ||
| optimisticallyAnsweredInputIds.insert(itemId) | ||
| await runSessionAction(op) | ||
| // Every answer handler (`approveRequest`, `respondToPermission`, | ||
| // `submitQuestionAnswers`, `respondToQuestion`, `declineQuestion`) resets | ||
| // `errorMessage` to nil on success and sets a message on failure, so a | ||
| // non-nil value here means THIS command failed — roll the hide back. | ||
| if errorMessage != nil { | ||
| optimisticallyAnsweredInputIds.remove(itemId) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the five answer handlers reset errorMessage deterministically.
rg -n -A 15 'func (approveRequest|respondToPermission|submitQuestionAnswers|respondToQuestion|declineQuestion)\s*\(' apps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftRepository: arul28/ADE
Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and symbols.
git ls-files 'apps/ios/ADE/Views/Work/*' | sed -n '1,120p'
rg -n 'approveRequest|respondToPermission|submitQuestionAnswers|respondToQuestion|declineQuestion|errorMessage|optimisticallyAnsweredInputIds|dispatchPendingInputAnswer' apps/ios/ADE/Views/Work -SRepository: arul28/ADE
Length of output: 18337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the relevant files under apps/ios and inspect their outlines.
fd -a 'WorkSessionDestinationView.swift|WorkChatSessionView\+Actions.swift' apps/ios
printf '\n---\n'
git ls-files 'apps/ios/**/*.swift' | rg 'WorkSessionDestinationView\.swift|WorkChatSessionView\+Actions\.swift'
printf '\n---\n'
for f in $(git ls-files 'apps/ios/**/*.swift' | rg 'WorkSessionDestinationView\.swift|WorkChatSessionView\+Actions\.swift'); do
echo "FILE: $f"
wc -l "$f"
doneRepository: arul28/ADE
Length of output: 678
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/ios/ADE/Views/Work/WorkSessionDestinationView+Actions.swift"
chat="apps/ios/ADE/Views/Work/WorkChatSessionView+Actions.swift"
echo '--- ANSWER HANDLERS ---'
sed -n '80,390p' "$file" | cat -n
echo '--- ROLLBACK SITE ---'
sed -n '950,970p' "$chat" | cat -n
echo '--- CALL SITES ---'
sed -n '1000,1020p' "$chat" | cat -nRepository: arul28/ADE
Length of output: 15171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A 25 -B 10 'func runSessionAction|runSessionAction\(' apps/ios/ADE/Views/Work/WorkSessionDestinationView+Actions.swift apps/ios/ADE/Views/Work/WorkSessionDestinationView.swift apps/ios/ADE/Views/Work/WorkChatSessionView+Actions.swift -SRepository: arul28/ADE
Length of output: 5933
Use a per-action result for dispatchPendingInputAnswer. errorMessage is shared with other chat actions, so an unrelated error during the awaited action can leave it non-nil and roll back a successful answer. Return success/failure explicitly instead of reading the global binding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ios/ADE/Views/Work/WorkChatSessionView`+Actions.swift around lines 953 -
967, The rollback check in dispatchPendingInputAnswer is relying on the shared
errorMessage state, which can be changed by unrelated chat actions. Update
dispatchPendingInputAnswer to use a per-action success/failure result from the
awaited op or runSessionAction instead of inspecting errorMessage, and only
remove the itemId from optimisticallyAnsweredInputIds when that specific action
reports failure. Use the dispatchPendingInputAnswer and runSessionAction symbols
to locate the flow and keep the answer handler logic isolated from global chat
errors.
Review follow-ups (Codex P2, Greptile, CodeRabbit) on the mode-broadcast + approval work: - Cursor clear handling is now stateless. Removed the cursorModeClearedSessionIds marker (and the mergeModeFields cleared param) that could go stale — a bulk merge caching a host-restored mode left the marker set, wrongly re-clearing the composer. Clears are now applied once at receipt: an explicit cursorModeId/ cursorConfigValues null nulls the cached field, and the open view mirrors the cache (nil included) on reconcile. A restored value can never be re-cleared because no "was cleared" state is remembered. - cursorConfigValues: null clears now decode via container.contains (present-null vs absent), matching cursorModeId; hasAnyField/applyModeUpdate honor the clear. Desktop + TUI already treated present-null as a clear (no change needed). - Accept-all sweep: sendPendingInputDecision/dispatchPendingInputAnswer now return an explicit per-action success (captured immediately after the handler's write, not from shared errorMessage), and acceptAllPendingInputs stops early if the primary acceptForSession fails instead of sweeping the rest. Absent-key still means no-change on every hop. Extends the iOS mode-update test with clear-reaches-live-summary and restored-value-not-re-cleared assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Bundles the full lane per request — two workstreams:
1. Cursor chat reliability + cross-client permission-mode sync (desktop + CLI + iOS)
NGHTTP2_INTERNAL_ERRORtransport reset) previously showed only the generic "Cursor SDK run failed." — the real reason lives only in the SDK's run store. The worker now reads thaterrorCodeon a terminal ERROR and rendersCursor run failed: <code>, classifies transport-class failures aserrorInfo.category="network"(so the renderer can offer retry), and logsagent_chat.cursor_sdk_run_error. No auto-retry (a resend could duplicate side effects like a push/PR).updateSessionnow emits asession_meta_updatedcarrying the permission/interaction/cursor-mode fields (incl. recomputedcursorModeSnapshot); desktop, TUI, and iOS all patch their composer on receipt.2. iOS Work polish
3. Sync / lane-naming (concurrent-session work, bundled per request)
appVersion/appBuild/bundleIdentifier) in device metadata.Testing
/quality(dual-track) clean: 0 Blocker/High/Medium; 2 behavior-preserving cleanups applied./test: desktop changed-file tests 496 pass; ade-cli 1,506 pass; TUI 814 pass; iOS build + key tests green (incl. a newcacheChatSummariesmerge regression test).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR improves Cursor chat reliability, cross-client mode sync, and mobile chat controls. The main changes are:
Confidence Score: 4/5
The iOS Cursor summary merge can clear live composer state from an omitted field.
Explicit clear handling is improved across the changed sync path, but the new wholesale merge treats absent cached Cursor fields as clears.
apps/ios/ADE/Models/RemoteModels.swift
What T-Rex did
Important Files Changed
Comments Outside Diff (2)
apps/desktop/src/main/services/chat/cursorSdkWorker.ts, line 615-620 (link)This loop withholds the first terminal
ERRORstatus but keeps forwarding any later stream events before posting the held error afterwait(). If the SDK emits cleanup/final status events after the error status, the chat bridge observes those events before the failure and can update the transcript or run state in the wrong order.Prompt To Fix With AI
apps/ios/ADE/Models/RemoteModels.swift, line 978-980 (link)When a cached summary comes from a payload that omits
cursorModeIdorcursorConfigValues, those optionals decode asnil. This merge then copies those nils into the live summary, so a partial refresh can clear the open iOS composer's Cursor mode or config even though no clear event was received.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "iOS: stateless cursor mode/config clear ..." | Re-trigger Greptile