Add Cursor provider sessions and model selection#1355
Add Cursor provider sessions and model selection#1355juliusmarminge wants to merge 1 commit intomainfrom
Conversation
- Introduce Cursor ACP adapter and model selection probe - Preserve cursor session resume state across model changes - Propagate provider and runtime tool metadata through orchestration and UI
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
size:XXL
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Merge never clears cached provider model options
- Replaced
??fallback withkey in incomingcheck so explicitly-present-but-undefined provider keys now clear cached values, and added cache deletion when merge produces undefined.
- Replaced
- ✅ Fixed: Mock agent test uses strict equal with extra fields
- Changed
toEqualtotoMatchObjectso the assertion tolerates the extramodesfield returned by the mock agent.
- Changed
Or push these changes by commenting:
@cursor push beb68c40d7
Preview (beb68c40d7)
diff --git a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
--- a/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
+++ b/apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
@@ -50,20 +50,17 @@
cached: ProviderModelOptions | undefined,
incoming: ProviderModelOptions | undefined,
): ProviderModelOptions | undefined {
- if (!cached && !incoming) {
- return undefined;
+ if (incoming === undefined) return cached;
+ if (cached === undefined) return incoming;
+
+ const providerKeys = ["codex", "claudeAgent", "cursor"] as const;
+ const next: Record<string, unknown> = {};
+ for (const key of providerKeys) {
+ const value = key in incoming ? incoming[key] : cached[key];
+ if (value !== undefined) {
+ next[key] = value;
+ }
}
- const next = {
- ...(incoming?.codex !== undefined || cached?.codex !== undefined
- ? { codex: incoming?.codex ?? cached?.codex }
- : {}),
- ...(incoming?.claudeAgent !== undefined || cached?.claudeAgent !== undefined
- ? { claudeAgent: incoming?.claudeAgent ?? cached?.claudeAgent }
- : {}),
- ...(incoming?.cursor !== undefined || cached?.cursor !== undefined
- ? { cursor: incoming?.cursor ?? cached?.cursor }
- : {}),
- } satisfies Partial<ProviderModelOptions>;
return Object.keys(next).length > 0 ? (next as ProviderModelOptions) : undefined;
}
@@ -405,8 +402,12 @@
threadModelOptions.get(input.threadId),
input.modelOptions,
);
- if (mergedModelOptions !== undefined) {
- threadModelOptions.set(input.threadId, mergedModelOptions);
+ if (input.modelOptions !== undefined) {
+ if (mergedModelOptions !== undefined) {
+ threadModelOptions.set(input.threadId, mergedModelOptions);
+ } else {
+ threadModelOptions.delete(input.threadId);
+ }
}
const normalizedInput = toNonEmptyProviderInput(input.messageText);
const normalizedAttachments = input.attachments ?? [];
diff --git a/apps/server/src/provider/acp/AcpJsonRpcConnection.test.ts b/apps/server/src/provider/acp/AcpJsonRpcConnection.test.ts
--- a/apps/server/src/provider/acp/AcpJsonRpcConnection.test.ts
+++ b/apps/server/src/provider/acp/AcpJsonRpcConnection.test.ts
@@ -32,7 +32,7 @@
cwd: process.cwd(),
mcpServers: [],
});
- expect(newResult).toEqual({ sessionId: "mock-session-1" });
+ expect(newResult).toMatchObject({ sessionId: "mock-session-1" });
const promptResult = yield* conn.request("session/prompt", {
sessionId: "mock-session-1",| : {}), | ||
| } satisfies Partial<ProviderModelOptions>; | ||
| return Object.keys(next).length > 0 ? (next as ProviderModelOptions) : undefined; | ||
| } |
There was a problem hiding this comment.
Merge never clears cached provider model options
Medium Severity
mergeThreadModelOptions uses incoming?.codex ?? cached?.codex for each provider key. The ?? operator treats both undefined and the absence of a key identically, so there's no way for a turn to clear a previously-cached provider's model options back to undefined. Once a provider's options are set (e.g., cursor: { fastMode: true }), subsequent turns that omit that provider key will always inherit the stale cached value. This matters because mergedModelOptions is forwarded to providerService.sendTurn, so traits like fastMode become permanently sticky at the orchestration layer even when the user explicitly removes them.
| cwd: process.cwd(), | ||
| mcpServers: [], | ||
| }); | ||
| expect(newResult).toEqual({ sessionId: "mock-session-1" }); |
There was a problem hiding this comment.
Mock agent test uses strict equal with extra fields
Low Severity
The test asserts expect(newResult).toEqual({ sessionId: "mock-session-1" }) but the mock agent's session/new handler returns { sessionId, modes: modeState() } which includes an additional modes property. toEqual performs a deep strict equality check, so this assertion will fail. It likely needs toMatchObject instead.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const nf = yield* Stream.runDrain( | ||
| Stream.mapEffect(conn.notifications, handleNotification), | ||
| ).pipe(Effect.forkChild); |
There was a problem hiding this comment.
🟢 Low Layers/CursorAdapter.ts:1217
The notification fiber nf is forked on line 1217 but its errors are never observed — if handleNotification throws (e.g., due to a failed offerRuntimeEvent), the fiber terminates silently and notification processing stops without any error reporting or recovery. Consider adding error handling or supervision to ensure the stream continues or failures are logged.
- const nf = yield* Stream.runDrain(
+ const nf = yield* Stream.runDrain(
Stream.mapEffect(conn.notifications, handleNotification),
- ).pipe(Effect.forkChild);
+ ).pipe(
+ Effect.tapError((e) => Effect.logError(`Notification stream error: ${e}`)),
+ Effect.retry({ schedule: Schedule.spaced(Duration.seconds(1)) }),
+ Effect.forkChild,
+ );🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around lines 1217-1219:
The notification fiber `nf` is forked on line 1217 but its errors are never observed — if `handleNotification` throws (e.g., due to a failed `offerRuntimeEvent`), the fiber terminates silently and notification processing stops without any error reporting or recovery. Consider adding error handling or supervision to ensure the stream continues or failures are logged.
Evidence trail:
apps/server/src/provider/Layers/CursorAdapter.ts lines 1217-1221 (fiber forked and stored without observation), line 672 (fiber typed as `Fiber.Fiber<void, never>`), lines 851-852 (fiber only interrupted, never joined/awaited), lines 702-703 (offerRuntimeEvent implementation with `never` error type)
| readonly notifications: Stream.Stream<AcpInboundMessage, never>; | ||
| } | ||
|
|
||
| export function spawnAcpChildProcess( |
There was a problem hiding this comment.
🟢 Low acp/AcpJsonRpcConnection.ts:43
spawnAcpChildProcess wraps spawn() in Effect.try, which only catches synchronous exceptions. spawn() failures like ENOENT emit asynchronously via the 'error' event, so the function returns success with a ChildProcess that will immediately fail. This causes the 'close' handler in attachAcpJsonRpcConnection to report AcpProcessExitedError instead of AcpSpawnError, and the unhandled 'error' event can emit an uncaught warning. Consider listening for the 'error' event and failing the Effect with AcpSpawnError when it fires.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/acp/AcpJsonRpcConnection.ts around line 43:
`spawnAcpChildProcess` wraps `spawn()` in `Effect.try`, which only catches synchronous exceptions. `spawn()` failures like ENOENT emit asynchronously via the 'error' event, so the function returns success with a `ChildProcess` that will immediately fail. This causes the 'close' handler in `attachAcpJsonRpcConnection` to report `AcpProcessExitedError` instead of `AcpSpawnError`, and the unhandled 'error' event can emit an uncaught warning. Consider listening for the 'error' event and failing the Effect with `AcpSpawnError` when it fires.
Evidence trail:
apps/ ser ver/ sr c/ pro vider/ ac p/ Ac pJs on Rp cC onne ction. ts: 42- 60 (`spa wn Ac pC hild Proc ess` function using `Eff ect. try` with `spa wn()`), lines 186- 194 (` clo se` event handler using `Ac pP rocess Exit ed Error`). No 'err or' event lis tener is attached to the child process in the entire file.
| const writeSerialized = (payload: Record<string, unknown>) => | ||
| writeLock.withPermits(1)(writeRawLine(payload)); | ||
|
|
||
| const sendRequest = (method: string, params?: unknown) => |
There was a problem hiding this comment.
🟢 Low acp/AcpJsonRpcConnection.ts:120
The deferred is added to the pending map on line 127 before writeRawLine executes on line 128. If writeRawLine throws an AcpSpawnError, the Effect fails and the caller receives the error, but the deferred remains in pending and is never resolved or removed. Over time with repeated write failures, orphaned deferreds accumulate in memory. Consider wrapping the write in a try/finally or restructuring so the deferred is only added to pending after the write succeeds, or ensure cleanup on failure.
Also found in 1 other location(s)
apps/server/src/provider/Layers/CursorAdapter.test.ts:281
Environment variable
process.env.T3_ACP_EMIT_TOOL_CALLSis set at line 281, but theEffect.ensuringcleanup (lines 405-415) only wrapsprogram, not the setup code at lines 283-313. If the adapter lookup,Deferred.make, orStream.runForEach(...).pipe(Effect.forkChild)fails beforeprogramruns, the environment variable will not be restored. This could pollute subsequent tests with the modifiedT3_ACP_EMIT_TOOL_CALLSvalue.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/acp/AcpJsonRpcConnection.ts around line 120:
The deferred is added to the `pending` map on line 127 before `writeRawLine` executes on line 128. If `writeRawLine` throws an `AcpSpawnError`, the Effect fails and the caller receives the error, but the deferred remains in `pending` and is never resolved or removed. Over time with repeated write failures, orphaned deferreds accumulate in memory. Consider wrapping the write in a try/finally or restructuring so the deferred is only added to `pending` after the write succeeds, or ensure cleanup on failure.
Evidence trail:
apps/server/src/provider/acp/AcpJsonRpcConnection.ts lines 120-133: sendRequest function where deferred is added to pending (line 127) before writeRawLine is called (line 128). Lines 104-114: writeRawLine implementation that can throw AcpSpawnError. Lines 93-100: failAllPending function. Lines 212-217: close event handler that calls failAllPending - only triggered on process exit, not on write failure.
Also found in 1 other location(s):
- apps/server/src/provider/Layers/CursorAdapter.test.ts:281 -- Environment variable `process.env.T3_ACP_EMIT_TOOL_CALLS` is set at line 281, but the `Effect.ensuring` cleanup (lines 405-415) only wraps `program`, not the setup code at lines 283-313. If the adapter lookup, `Deferred.make`, or `Stream.runForEach(...).pipe(Effect.forkChild)` fails before `program` runs, the environment variable will not be restored. This could pollute subsequent tests with the modified `T3_ACP_EMIT_TOOL_CALLS` value.
1 participant
Summary
Testing
bun fmtbun lintbun typecheckbun run testNote
Medium Risk
Introduces a new provider adapter and session lifecycle (spawned CLI process + JSON-RPC) and changes orchestration routing/session restart behavior, which can affect turn execution and session persistence across providers.
Overview
Adds Cursor as a first-class provider end-to-end: a new ACP JSON-RPC transport (
AcpJsonRpcConnection) plusCursorAdapterLivethat spawnsagent acp, managessession/new|load|prompt, maps ACP updates/tool calls/approvals into runtime events, and restarts sessions when the effective Cursor model changes.Updates orchestration to persist
thread.provider, route turns by explicit provider even for shared model slugs, treat Cursor model switching as restart-session, preserveresumeCursoracross Cursor restarts, and merge per-threadmodelOptionsacross turns.Extends runtime ingestion to include structured
dataontool.completedactivities, wires Cursor into the adapter registry, server layers, session directory persistence, and provider health checks.Updates the web app to support Cursor settings and UX: adds
customCursorModels, Cursor model-family picker + trait controls (reasoning/fast/thinking/opus tier), avoids redundant tool entry previews, and persists a sticky provider/model selection when creating new threads.Written by Cursor Bugbot for commit 12f825a. This will update automatically on new commits. Configure here.
Note
Add Cursor provider sessions and model selection across client and server
CursorAdapterLivespawns the Cursor agent via ACP JSON-RPC, maps ACP errors to provider adapter errors, and is registered in the defaultProviderAdapterRegistry.AcpJsonRpcConnection— a new JSON-RPC framing layer over a child process with serialized writes, in-flight request tracking, and a notifications stream.CursorModelOptionsschema (reasoning, fastMode, thinking, claudeOpusTier), and model slug/alias resolution inpackages/contractsandpackages/shared.stickyProviderpersistence and Cursor model options normalization; theProviderCommandReactornow routes turns by explicit provider and merges per-providermodelOptionsacross turns.CursorTraitsMenuContentandCursorTraitsPickerUI components for selecting Opus tier, reasoning level, and fast mode; theProviderModelPickernow displays and dispatches Cursor family slugs.checkCursorProviderStatus.resumeCursoris now preserved when only the model changes and cleared only on provider change.📊 Macroscope summarized 12f825a. 42 files reviewed, 14 issues evaluated, 7 issues filtered, 3 comments posted
🗂️ Filtered Issues
apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts — 0 comments posted, 2 evaluated, 2 filtered
projector.tschange unconditionally setsprovider: payload.providerwhen constructing theOrchestrationThreadobject forthread.createdevents. For events persisted before this change (or events where thethread.createcommand did not includeprovider),payload.providerwill beundefined. The decider indecider.tsconditionally spreadsproviderinto the event payload (...(command.provider !== undefined ? { provider: command.provider } : {})), meaning the key is intentionally absent when not specified. However, the projector now always injects the key with a potentiallyundefinedvalue. If theOrchestrationThreadschema definesprovideras anoptionalKey(key can be absent but must be a string when present) rather thanoptional(which also acceptsundefined), this will cause adecodeForEventfailure during event replay at startup for any existingthread.createdevents that lack aproviderfield. [ Out of scope (triage) ]"preserves completed tool metadata on projected tool activities"within the samedescribe("ProviderRuntimeIngestion", ...)block. The diff inserts a complete copy of the test (lines 682–737 after patch) immediately before the original test that already existed. Both tests have the same name, emit the same events, and assert the same expectations. While vitest will run both without crashing, this is an unintentional duplication that bloats test execution time and will cause confusion when one fails. [ Failed validation ]apps/server/src/provider/Layers/CursorAdapter.test.ts — 0 comments posted, 2 evaluated, 1 filtered
process.env.T3_ACP_EMIT_TOOL_CALLSis set at line 281, but theEffect.ensuringcleanup (lines 405-415) only wrapsprogram, not the setup code at lines 283-313. If the adapter lookup,Deferred.make, orStream.runForEach(...).pipe(Effect.forkChild)fails beforeprogramruns, the environment variable will not be restored. This could pollute subsequent tests with the modifiedT3_ACP_EMIT_TOOL_CALLSvalue. [ Cross-file consolidated ]apps/server/src/provider/Layers/CursorAdapter.ts — 1 comment posted, 4 evaluated, 2 filtered
aliasis only lowercased but not normalized (special characters not replaced with spaces), whilenormalizeModeSearchText(mode)replaces all non-alphanumeric characters with spaces. This asymmetry means an alias like"plan-mode"would fail to match a mode whose normalized text contains"plan mode"because"plan mode".includes("plan-mode")isfalse. If any of the alias constants (e.g.,ACP_PLAN_MODE_ALIASES) contain hyphens or other special characters, partial matches will silently fail. [ Out of scope (triage) ]parseSessionUpdate, whenparams.updateis a record containingmodeId/currentModeIdbutsessionUpdateis undefined andcontentis a record without atextstring property, the parsedmodeIdis discarded at line 649. The function returns{}even thoughmodeIdwas successfully parsed at lines 561-566. This causes the caller'sif (p.modeId)check to fail, missing a mode state update. The finalreturn {}at line 649 should includemodeIdlike the other return paths do. [ Cross-file consolidated ]apps/web/src/composerDraftStore.ts — 0 comments posted, 1 evaluated, 1 filtered
cursorClaudeOpusTiervalue"high"is validated as valid (line 431-432) but never included in the output. Line 445 only includesclaudeOpusTierwhen it equals"max", meaning"high"is silently dropped. The condition should becursorClaudeOpusTier ? { claudeOpusTier: cursorClaudeOpusTier } : {}to preserve both valid values. [ Out of scope (triage) ]packages/shared/src/model.ts — 0 comments posted, 1 evaluated, 1 filtered
normalizeCursorModelOptions, the logic for persistingthinkingonly handlessel.thinking === false, but forclaude-4.6-sonnetthedefaultThinkingisfalse(line 111). If a user enables thinking for sonnet (sel.thinking === true), this non-default value is not included in the returned options. The condition at line 352 should compare against the model's specific default:sel.thinking !== cap.defaultThinkingrather than just checking forfalse. [ Cross-file consolidated ]