Conversation
|
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 |
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.
- 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 Made-with: Cursor
Replace the hardcoded client-side CURSOR_MODEL_CAPABILITY_BY_FAMILY map with server-provided ModelCapabilities, matching the Codex/Claude pattern. - Add CursorProvider snapshot service with BUILT_IN_MODELS and per-model capabilities; register it in ProviderRegistry alongside Codex/Claude. - Delete CursorTraitsPicker and route Cursor through the generic TraitsPicker, adding cursor support for the reasoning/effort key. - Add normalizeCursorModelOptionsWithCapabilities to providerModels. Made-with: Cursor
12f825a to
10129ed
Compare
apps/web/src/components/chat/CompactComposerControlsMenu.browser.tsx
Outdated
Show resolved
Hide resolved
| const defaultCursorReasoning = | ||
| DEFAULT_REASONING_EFFORT_BY_PROVIDER.cursor as CursorReasoningOption; | ||
|
|
||
| const cursor: CursorModelOptions | undefined = |
There was a problem hiding this comment.
🟢 Low src/composerDraftStore.ts:511
When cursorCandidate is non-null but contains no valid options, cursor becomes {} instead of undefined. The null check on line 523 (cursor === undefined) fails for empty objects, causing normalizeProviderModelOptions to return { cursor: {} } instead of null. This is inconsistent with codex and claude, which return undefined when no options exist. Consider checking Object.keys(cursor).length > 0 or using a definedness flag to ensure empty objects are treated as absent.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/composerDraftStore.ts around line 511:
When `cursorCandidate` is non-null but contains no valid options, `cursor` becomes `{}` instead of `undefined`. The null check on line 523 (`cursor === undefined`) fails for empty objects, causing `normalizeProviderModelOptions` to return `{ cursor: {} }` instead of `null`. This is inconsistent with `codex` and `claude`, which return `undefined` when no options exist. Consider checking `Object.keys(cursor).length > 0` or using a definedness flag to ensure empty objects are treated as absent.
Evidence trail:
apps/web/src/composerDraftStore.ts lines 459-466 (codex pattern), lines 486-492 (claude pattern), lines 511-521 (cursor pattern), lines 522-528 (return logic). Verified at commit REVIEWED_COMMIT.
…tion Instead of restarting the ACP process when the model changes mid-thread, use session/set_config_option to switch models within a live session. Update sessionModelSwitch to "in-session" and add probe tests to verify the real agent supports this method. Made-with: Cursor
Made-with: Cursor # Conflicts: # apps/web/src/components/chat/CompactComposerControlsMenu.browser.tsx # apps/web/src/components/chat/ProviderModelPicker.browser.tsx # apps/web/src/components/chat/ProviderModelPicker.tsx # apps/web/src/components/chat/TraitsPicker.tsx # apps/web/src/components/chat/composerProviderRegistry.test.tsx # apps/web/src/composerDraftStore.ts # packages/contracts/src/model.ts # packages/shared/src/model.test.ts # packages/shared/src/model.ts
apps/web/src/components/chat/CompactComposerControlsMenu.browser.tsx
Outdated
Show resolved
Hide resolved
- Removed unused CursorModelOptions and related logic from ChatView. - Updated model selection handling to map concrete Cursor slugs to server-provided options. - Simplified ProviderModelPicker by eliminating unnecessary cursor-related state and logic. - Adjusted tests to reflect changes in model selection behavior for Cursor provider. Made-with: Cursor
apps/web/src/components/chat/CompactComposerControlsMenu.browser.tsx
Outdated
Show resolved
Hide resolved
- Add a standalone ACP probe script for initialize/auth/session/new - Switch Cursor provider status checks to `agent about` for version and auth - Log the ACP session/new result in the probe test
- Canonicalize Claude and Cursor dispatch model slugs - Update provider model selection, defaults, and tests
- route Cursor commit/PR/branch generation through the agent CLI - resolve separate ACP and agent model IDs for Cursor models - improve git action failure logging and surface command output
| const interruptTurn: CursorAdapterShape["interruptTurn"] = (threadId) => | ||
| Effect.gen(function* () { | ||
| const ctx = yield* requireSession(threadId); | ||
| yield* settlePendingApprovalsAsCancelled(ctx.pendingApprovals); | ||
| yield* Effect.ignore( | ||
| ctx.acp.cancel.pipe( | ||
| Effect.mapError((error) => | ||
| mapAcpToAdapterError(PROVIDER, threadId, "session/cancel", error), | ||
| ), | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🟠 High Layers/CursorAdapter.ts:790
interruptTurn calls settlePendingApprovalsAsCancelled but not settlePendingUserInputsAsEmptyAnswers, so any pending cursor/ask_question handler waiting on Deferred.await(answers) will hang forever when the turn is interrupted. Consider adding yield* settlePendingUserInputsAsEmptyAnswers(ctx.pendingUserInputs); to match the behavior in stopSessionInternal.
const interruptTurn: CursorAdapterShape["interruptTurn"] = (threadId) =>
Effect.gen(function* () {
const ctx = yield* requireSession(threadId);
yield* settlePendingApprovalsAsCancelled(ctx.pendingApprovals);
+ yield* settlePendingUserInputsAsEmptyAnswers(ctx.pendingUserInputs);
yield* Effect.ignore(
ctx.acp.cancel.pipe(
Effect.mapError((error) =>
mapAcpToAdapterError(PROVIDER, threadId, "session/cancel", error),
),
),
);
});🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around lines 790-801:
`interruptTurn` calls `settlePendingApprovalsAsCancelled` but not `settlePendingUserInputsAsEmptyAnswers`, so any pending `cursor/ask_question` handler waiting on `Deferred.await(answers)` will hang forever when the turn is interrupted. Consider adding `yield* settlePendingUserInputsAsEmptyAnswers(ctx.pendingUserInputs);` to match the behavior in `stopSessionInternal`.
Evidence trail:
apps/server/src/provider/Layers/CursorAdapter.ts lines 790-801 (interruptTurn function - only calls settlePendingApprovalsAsCancelled at line 793), lines 305-322 (stopSessionInternal - calls both settlement functions at lines 309-310), lines 118-129 (settlePendingUserInputsAsEmptyAnswers function definition), lines 410-437 (cursor/ask_question handler with Deferred.make at line 420, Deferred.await at line 436)
| | ((method: string, params: unknown) => Effect.Effect<void, AcpError.AcpError>) | ||
| | undefined; | ||
|
|
||
| const dispatchNotification = (notification: AcpProtocol.AcpIncomingNotification) => { |
There was a problem hiding this comment.
🟢 Low src/client.ts:433
When multiple handlers are registered via handleSessionUpdate or handleElicitationComplete, Effect.forEach in dispatchNotification runs them sequentially. If any handler fails, forEach short-circuits and skips remaining handlers. Because the error is silently swallowed by Effect.catch(() => Effect.void) in the protocol layer, users receive no indication that some handlers were never executed. This makes multi-handler registration unreliable — a failing early handler silently drops later handlers.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/effect-acp/src/client.ts around line 433:
When multiple handlers are registered via `handleSessionUpdate` or `handleElicitationComplete`, `Effect.forEach` in `dispatchNotification` runs them sequentially. If any handler fails, `forEach` short-circuits and skips remaining handlers. Because the error is silently swallowed by `Effect.catch(() => Effect.void)` in the protocol layer, users receive no indication that some handlers were never executed. This makes multi-handler registration unreliable — a failing early handler silently drops later handlers.
Evidence trail:
1. packages/effect-acp/src/client.ts:414-418 - `notificationHandlers` is initialized with arrays for `sessionUpdate` and `elicitationComplete`
2. packages/effect-acp/src/client.ts:592-600 - `handleSessionUpdate` and `handleElicitationComplete` push handlers to arrays (allowing multiple handlers)
3. packages/effect-acp/src/client.ts:433-449 - `dispatchNotification` uses `Effect.forEach` to iterate over handlers
4. packages/effect-acp/src/protocol.ts:171-176 - `dispatchNotification` shows that `onNotification` errors are caught with `Effect.catch(() => Effect.void)`
5. Effect library documentation (https://github.com/Effect-TS/effect/blob/main/packages/effect/src/Effect.ts): "If any effect fails, the iteration stops immediately (short-circuiting), and the error is propagated."
| >; | ||
| } | ||
|
|
||
| export const make = Effect.fn("effect-acp/AcpClient.make")(function* ( |
There was a problem hiding this comment.
🟢 Low src/client.ts:310
dispatchNotification is passed to makeAcpPatchedProtocol at line 378, but notificationHandlers.sessionUpdate and notificationHandlers.elicitationComplete arrays are populated later when handleSessionUpdate/handleElicitationComplete are called. The protocol transport forks immediately, so session/update or session/elicitation/complete notifications that arrive before handlers are registered are dispatched to empty arrays and silently dropped — Effect.forEach([], ...) succeeds with no error or indication that messages were lost. Consider buffering notifications until handlers are registered, or provide a way to register handlers before protocol construction.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/effect-acp/src/client.ts around line 310:
`dispatchNotification` is passed to `makeAcpPatchedProtocol` at line 378, but `notificationHandlers.sessionUpdate` and `notificationHandlers.elicitationComplete` arrays are populated later when `handleSessionUpdate`/`handleElicitationComplete` are called. The protocol transport forks immediately, so `session/update` or `session/elicitation/complete` notifications that arrive before handlers are registered are dispatched to empty arrays and silently dropped — `Effect.forEach([], ...)` succeeds with no error or indication that messages were lost. Consider buffering notifications until handlers are registered, or provide a way to register handlers before protocol construction.
Evidence trail:
- packages/effect-acp/src/client.ts lines 316-319: `notificationHandlers` initialized with empty arrays
- packages/effect-acp/src/client.ts lines 334-349: `dispatchNotification` uses `Effect.forEach(notificationHandlers.sessionUpdate, ...)`
- packages/effect-acp/src/client.ts lines 369-381: `dispatchNotification` passed to `makeAcpPatchedProtocol` via `onNotification: dispatchNotification`
- packages/effect-acp/src/protocol.ts lines 363-420: stdin processing forked immediately with `Effect.forkScoped`
- packages/effect-acp/src/client.ts lines 492-500: handlers registered later via `handleSessionUpdate`/`handleElicitationComplete` which push to arrays
- Empty array iteration: `Effect.forEach([], handler)` succeeds with no processing
| const failAllExtPending = (error: AcpError.AcpError) => | ||
| Ref.get(extPending).pipe( | ||
| Effect.flatMap((pending) => | ||
| Effect.forEach([...pending.values()], (deferred) => Deferred.fail(deferred, error), { | ||
| discard: true, | ||
| }), | ||
| ), | ||
| Effect.andThen(Ref.set(extPending, new Map())), | ||
| ); |
There was a problem hiding this comment.
🟢 Low src/protocol.ts:159
failAllExtPending reads extPending at line 161, then clears it at line 166. Between these two operations, a concurrent sendRequest can add a new deferred to the map. The subsequent Ref.set overwrites the entire map with an empty one, dropping that deferred. The caller waiting on Deferred.await will hang until scope interruption instead of failing immediately with the termination error. Consider using Ref.getAndSet or Ref.modify to atomically read and clear the pending map.
- const failAllExtPending = (error: AcpError.AcpError) =>
- Ref.get(extPending).pipe(
- Effect.flatMap((pending) =>
- Effect.forEach([...pending.values()], (deferred) => Deferred.fail(deferred, error), {
- discard: true,
- }),
- ),
- Effect.andThen(Ref.set(extPending, new Map())),
- );
+ const failAllExtPending = (error: AcpError.AcpError) =>
+ Ref.getAndSet(extPending, new Map()).pipe(
+ Effect.flatMap((pending) =>
+ Effect.forEach([...pending.values()], (deferred) => Deferred.fail(deferred, error), {
+ discard: true,
+ }),
+ ),
+ );🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/effect-acp/src/protocol.ts around lines 159-167:
`failAllExtPending` reads `extPending` at line 161, then clears it at line 166. Between these two operations, a concurrent `sendRequest` can add a new deferred to the map. The subsequent `Ref.set` overwrites the entire map with an empty one, dropping that deferred. The caller waiting on `Deferred.await` will hang until scope interruption instead of failing immediately with the termination error. Consider using `Ref.getAndSet` or `Ref.modify` to atomically read and clear the pending map.
Evidence trail:
packages/effect-acp/src/protocol.ts lines 159-167 (failAllExtPending function showing Ref.get followed by Ref.set), packages/effect-acp/src/protocol.ts lines 475-495 (sendRequest function showing Ref.update adding deferred to extPending at line 481), commit REVIEWED_COMMIT
| Effect.forkScoped, | ||
| ); | ||
|
|
||
| let nextRpcRequestId = 1n; |
There was a problem hiding this comment.
🟡 Medium src/agent.ts:365
Request ID collision between the RPC client and extension request API causes responses to be misrouted. nextRpcRequestId on line 365 starts at 1n, the same value as the protocol's internal nextRequestId counter in makeAcpPatchedProtocol. Both generate identical ID sequences ("1", "2", "3", etc.), so handleExitEncoded in the protocol cannot distinguish which response belongs to which request. This causes RPC client responses to be incorrectly delivered to extension request Deferreds, or vice versa, resulting in hung requests or wrong results.
- let nextRpcRequestId = 1n;
+ let nextRpcRequestId = 1n << 32n;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/effect-acp/src/agent.ts around line 365:
Request ID collision between the RPC client and extension request API causes responses to be misrouted. `nextRpcRequestId` on line 365 starts at `1n`, the same value as the protocol's internal `nextRequestId` counter in `makeAcpPatchedProtocol`. Both generate identical ID sequences (`"1"`, `"2"`, `"3"`, etc.), so `handleExitEncoded` in the protocol cannot distinguish which response belongs to which request. This causes RPC client responses to be incorrectly delivered to extension request `Deferred`s, or vice versa, resulting in hung requests or wrong results.
Evidence trail:
- agent.ts line 365: `let nextRpcRequestId = 1n;`
- protocol.ts line 82: `const nextRequestId = yield* Ref.make(1n);`
- protocol.ts lines 313-333: `sendRequest` uses `nextRequestId` to generate extension request IDs stored in `extPending` as `String(requestId)`
- protocol.ts lines 239-257: `handleExitEncoded` routes responses based on `extPending.has(message.requestId)` - if found, resolves extension Deferred; otherwise forwards to clientQueue for RPC client
- Both ID generators produce identical sequences starting from '1', causing potential collision in the shared response routing logic
There was a problem hiding this comment.
🟢 Low
When the reactor's scope is closed during shutdown, the forked sendTurn fiber is interrupted. Effect.catchCause(handleTurnStartFailure) catches this interrupt cause and invokes handleTurnStartFailure, which sets the session's lastError to a rendered interrupt message and changes its status to "ready". This produces misleading session error states during graceful shutdown—sessions that were running fine get marked with interrupt-related errors. Consider filtering out interrupt-only causes in handleTurnStartFailure so shutdown interruptions don't get recorded as session errors.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts around line 566:
When the reactor's scope is closed during shutdown, the forked `sendTurn` fiber is interrupted. `Effect.catchCause(handleTurnStartFailure)` catches this interrupt cause and invokes `handleTurnStartFailure`, which sets the session's `lastError` to a rendered interrupt message and changes its `status` to `"ready"`. This produces misleading session error states during graceful shutdown—sessions that were running fine get marked with interrupt-related errors. Consider filtering out interrupt-only causes in `handleTurnStartFailure` so shutdown interruptions don't get recorded as session errors.
Evidence trail:
apps/server/src/orchestration/Layers/ProviderCommandReactor.ts lines 603-618 (`handleTurnStartFailure` function), lines 209-218 (`formatFailureDetail` function), lines 233-252 (`setThreadSessionErrorOnTurnStartFailure` function), line 643 (`Effect.catchCause(handleTurnStartFailure), Effect.forkScoped`). package.json line 11 shows Effect version 4.0.0-beta.42.
Co-authored-by: codex <codex@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Medium
When providerService.sendTurn fails, handleTurnStartFailure is now forked via Effect.forkScoped instead of being awaited. If handleTurnStartFailure itself fails—for example, because orchestrationEngine.dispatch rejects—the error is silently dropped rather than propagating to processDomainEventSafely for logging. This means secondary failures during error recovery leave no trace, making debugging difficult and potentially leaving the thread state inconsistent (no session error set, no failure activity appended, and no warning logged).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts around line 16:
When `providerService.sendTurn` fails, `handleTurnStartFailure` is now forked via `Effect.forkScoped` instead of being awaited. If `handleTurnStartFailure` itself fails—for example, because `orchestrationEngine.dispatch` rejects—the error is silently dropped rather than propagating to `processDomainEventSafely` for logging. This means secondary failures during error recovery leave no trace, making debugging difficult and potentially leaving the thread state inconsistent (no session error set, no failure activity appended, and no warning logged).
Evidence trail:
- apps/server/src/orchestration/Layers/ProviderCommandReactor.ts lines 640-643 (REVIEWED_COMMIT): `providerService.sendTurn(...).pipe(Effect.catchCause(handleTurnStartFailure), Effect.forkScoped)`
- apps/server/src/orchestration/Layers/ProviderCommandReactor.ts lines 603-622: `handleTurnStartFailure` calls `setThreadSessionErrorOnTurnStartFailure` and `appendProviderFailureActivity`, both using `orchestrationEngine.dispatch`
- apps/server/src/orchestration/Layers/ProviderCommandReactor.ts lines 821-832: `processDomainEventSafely` has error logging via `Effect.logWarning` but only catches errors from direct children, not forked fibers
- git_diff MERGE_BASE..REVIEWED_COMMIT showing before/after: previously `sendTurnForThread` was awaited directly, now forked via `Effect.forkScoped`
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com> # Conflicts: # apps/web/src/store.ts
Co-authored-by: codex <codex@users.noreply.github.com> # Conflicts: # apps/server/integration/OrchestrationEngineHarness.integration.ts # apps/server/package.json # apps/server/src/git/Errors.ts # apps/server/src/git/Layers/GitCore.test.ts # apps/server/src/main.test.ts # apps/server/src/orchestration/Layers/CheckpointReactor.test.ts # apps/server/src/serverLayers.ts # bun.lock # package.json # packages/contracts/src/orchestration.ts
Co-authored-by: codex <codex@users.noreply.github.com>
- Map parameterized Cursor ACP config options to model selections - Switch text generation and provider flows to ACP session runtime - Update tests and mock agent for new config handling
| yield* Effect.addFinalizer(() => | ||
| Effect.forEach(sessions.values(), stopSessionInternal, { discard: true }).pipe( | ||
| Effect.tap(() => Queue.shutdown(runtimeEventQueue)), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🟡 Medium Layers/CursorAdapter.ts:911
When nativeEventLogPath is provided but nativeEventLogger is not, the adapter creates an internal EventNdjsonLogger at lines 236-238. The finalizer at lines 911-915 shuts down the runtimeEventQueue but never calls nativeEventLogger.close(). The EventNdjsonLogger uses batched writers that require explicit closing to flush pending log entries, so this causes a resource leak and potential data loss where final log entries may never be written to disk.
yield* Effect.addFinalizer(() =>
- Effect.forEach(sessions.values(), stopSessionInternal, { discard: true }).pipe(
- Effect.tap(() => Queue.shutdown(runtimeEventQueue)),
- ),
+ Effect.forEach(sessions.values(), stopSessionInternal, { discard: true }).pipe(
+ Effect.tap(() => Queue.shutdown(runtimeEventQueue)),
+ Effect.tap(() => nativeEventLogger?.close() ?? Effect.void),
+ ),
);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around lines 911-915:
When `nativeEventLogPath` is provided but `nativeEventLogger` is not, the adapter creates an internal `EventNdjsonLogger` at lines 236-238. The finalizer at lines 911-915 shuts down the `runtimeEventQueue` but never calls `nativeEventLogger.close()`. The `EventNdjsonLogger` uses batched writers that require explicit closing to flush pending log entries, so this causes a resource leak and potential data loss where final log entries may never be written to disk.
Evidence trail:
CursorAdapter.ts lines 233-239: internal `EventNdjsonLogger` creation via `makeEventNdjsonLogger` when `nativeEventLogPath` provided but `nativeEventLogger` is not; CursorAdapter.ts lines 911-915: finalizer only calls `Queue.shutdown(runtimeEventQueue)`, no `nativeEventLogger.close()`; EventNdjsonLogger.ts lines 26-30: `EventNdjsonLogger` interface with `close` method; EventNdjsonLogger.ts lines 134-155: `Logger.batched` usage in `makeThreadWriter`; EventNdjsonLogger.ts lines 164-166: `ThreadWriter.close()` calls `Scope.close(scope, Exit.void)`; EventNdjsonLogger.ts lines 247-252: `EventNdjsonLogger.close()` calls `writer.close()` for all thread writers.
| }), | ||
| ); | ||
|
|
||
| yield* agent.handleCancel(({ sessionId }) => |
There was a problem hiding this comment.
🟢 Low scripts/acp-mock-agent.ts:266
handleCancel uses String(sessionId) without a fallback (line 268), so an undefined sessionId becomes the literal string "undefined". However handlePrompt falls back to the global sessionId when request.sessionId is undefined (line 274). This mismatch means cancelling a session with an undefined ID won't actually cancel the corresponding prompt—they use different ID strings.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/scripts/acp-mock-agent.ts around line 266:
`handleCancel` uses `String(sessionId)` without a fallback (line 268), so an undefined sessionId becomes the literal string `"undefined"`. However `handlePrompt` falls back to the global `sessionId` when `request.sessionId` is undefined (line 274). This mismatch means cancelling a session with an undefined ID won't actually cancel the corresponding prompt—they use different ID strings.
Evidence trail:
apps/server/scripts/acp-mock-agent.ts lines 23, 265-268, 271-274 at REVIEWED_COMMIT: Line 23 defines global `sessionId = "mock-session-1"`. Line 267 shows `handleCancel(({ sessionId }) => ...)` destructuring from callback, line 268 uses `String(sessionId)` without fallback. Line 274 shows `const requestedSessionId = String(request.sessionId ?? sessionId)` using nullish coalescing to fall back to the global.
- Parse Cursor CLI version/channel for ACP preview gating - Fall back with an explanatory status when requirements are not met - Co-authored-by: codex <codex@users.noreply.github.com>
- Parse `cursor about --format json` results - Fall back to plain `about` when JSON formatting is unsupported - Preserve subscription metadata in provider auth state
# Conflicts: # apps/server/src/checkpointing/Layers/CheckpointStore.test.ts # apps/server/src/git/Layers/GitManager.test.ts # apps/server/src/orchestration/Layers/CheckpointReactor.test.ts # apps/web/src/components/chat/MessagesTimeline.tsx # apps/web/src/components/chat/composerProviderRegistry.tsx # apps/web/src/composerDraftStore.ts # apps/web/src/rpc/client.test.ts # apps/web/src/session-logic.ts # apps/web/src/store.ts # packages/contracts/src/orchestration.ts # packages/contracts/src/settings.ts
# Conflicts: # apps/server/integration/orchestrationEngine.integration.test.ts # apps/server/src/checkpointing/Layers/CheckpointStore.test.ts # apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts # apps/web/src/components/ChatView.tsx # apps/web/src/components/chat/MessagesTimeline.tsx # bun.lock # package.json # packages/contracts/src/orchestration.ts # packages/contracts/src/settings.ts
| stopSession, | ||
| listSessions, | ||
| hasSession, | ||
| stopAll, | ||
| get streamEvents() { | ||
| return Stream.fromQueue(runtimeEventQueue); | ||
| }, | ||
| } satisfies CursorAdapterShape; |
There was a problem hiding this comment.
🟢 Low Layers/CursorAdapter.ts:927
The streamEvents getter creates a new Stream.fromQueue instance each access. Because Queue.take removes items destructively, multiple consumers will receive disjoint event subsets instead of each receiving all events. If multiple subscribers are expected, this silently drops events. Consider creating the stream once and sharing it, or replacing the Queue with a PubSub.
- get streamEvents() {
- return Stream.fromQueue(runtimeEventQueue);
- },
+ streamEvents: Stream.fromQueue(runtimeEventQueue),🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around lines 927-934:
The `streamEvents` getter creates a new `Stream.fromQueue` instance each access. Because `Queue.take` removes items destructively, multiple consumers will receive disjoint event subsets instead of each receiving all events. If multiple subscribers are expected, this silently drops events. Consider creating the stream once and sharing it, or replacing the `Queue` with a `PubSub`.
Evidence trail:
- CursorAdapter.ts:931-933: `get streamEvents() { return Stream.fromQueue(runtimeEventQueue); }` confirms getter creates new Stream.fromQueue each access
- CursorAdapter.ts:242: `const runtimeEventQueue = yield* Queue.unbounded<ProviderRuntimeEvent>();` - single shared queue
- ProviderService.ts:746-750: Shows the correct pattern using PubSub with comment explaining why: 'Each access creates a fresh PubSub subscription so that multiple consumers (ProviderRuntimeIngestion, CheckpointReactor, etc.) each independently receive all runtime events'
- Effect-TS Queue semantics: Queue.take is destructive (standard behavior, items removed when taken)
| yield* acp.handleRequestPermission((params) => | ||
| Effect.gen(function* () { | ||
| yield* logNative(input.threadId, "session/request_permission", params, "acp.jsonrpc"); | ||
| if (ctx?.session.runtimeMode === "full-access") { |
There was a problem hiding this comment.
🟠 High Layers/CursorAdapter.ts:499
In handleRequestPermission at line 499, the check ctx?.session.runtimeMode === "full-access" uses ctx which is not assigned until line 580. Since this callback is registered before acp.start() completes, permission requests during initialization will see ctx as undefined and skip auto-approval even when input.runtimeMode is "full-access". The condition should use input.runtimeMode instead, which is in scope and contains the correct value.
- if (ctx?.session.runtimeMode === "full-access") {
+ if (input.runtimeMode === "full-access") {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around line 499:
In `handleRequestPermission` at line 499, the check `ctx?.session.runtimeMode === "full-access"` uses `ctx` which is not assigned until line 580. Since this callback is registered before `acp.start()` completes, permission requests during initialization will see `ctx` as `undefined` and skip auto-approval even when `input.runtimeMode` is `"full-access"`. The condition should use `input.runtimeMode` instead, which is in scope and contains the correct value.
Evidence trail:
apps/server/src/provider/Layers/CursorAdapter.ts line 381: `let ctx!: CursorSessionContext;` declares ctx without initialization.
Line 495-555: `handleRequestPermission` callback registered, with line 499 checking `ctx?.session.runtimeMode === "full-access"`.
Line 497: `input.threadId` is used in the callback, confirming `input` is in scope.
Line 557: `return yield* acp.start();` starts ACP after handlers are registered.
Lines 579-590: `ctx` is assigned after `acp.start()` returns.
Line 570: `runtimeMode: input.runtimeMode` shows `input.runtimeMode` holds the same value that would be in `ctx.session.runtimeMode`.
| const contextWindowOptions = | ||
| contextOption?.type === "select" | ||
| ? flattenSessionConfigSelectOptions(contextOption).map((entry) => { | ||
| if (contextOption.currentValue === entry.value) { | ||
| return { | ||
| value: entry.value, | ||
| label: entry.name, | ||
| isDefault: true, | ||
| }; | ||
| } | ||
| return { | ||
| value: entry.value, | ||
| label: entry.name, | ||
| }; | ||
| }) |
There was a problem hiding this comment.
🟢 Low Layers/CursorProvider.ts:319
The comparison on line 322 compares contextOption.currentValue directly against entry.value, but entry.value was trimmed in flattenSessionConfigSelectOptions while currentValue was not. If currentValue contains whitespace, the strict equality fails and isDefault is never set on the matching option, so the UI won't highlight the correct default context window.
const contextWindowOptions =
contextOption?.type === "select"
? flattenSessionConfigSelectOptions(contextOption).map((entry) => {
- if (contextOption.currentValue === entry.value) {
+ if (contextOption.currentValue?.trim() === entry.value) {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorProvider.ts around lines 319-333:
The comparison on line 322 compares `contextOption.currentValue` directly against `entry.value`, but `entry.value` was trimmed in `flattenSessionConfigSelectOptions` while `currentValue` was not. If `currentValue` contains whitespace, the strict equality fails and `isDefault` is never set on the matching option, so the UI won't highlight the correct default context window.
Evidence trail:
apps/server/src/provider/Layers/CursorProvider.ts lines 211-227 (flattenSessionConfigSelectOptions function trims values at lines 219, 222-224); apps/server/src/provider/Layers/CursorProvider.ts line 322 (comparison does not trim currentValue); apps/server/src/provider/Layers/CursorProvider.ts line 308 (similar comparison properly normalizes both sides); apps/server/src/provider/Layers/CursorProvider.ts line 502 (similar usage properly trims currentValue)
- Keep false/explicit defaults in draft and dispatch state - Allow Cursor fast mode and thinking to be cleared on later turns - Add coverage for Cursor option reset behavior



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 f0d4415. 10 files reviewed, 4 issues evaluated, 0 issues filtered, 3 comments posted
🗂️ Filtered Issues