Skip to content

Add ACP support with Cursor provider#1355

Draft
juliusmarminge wants to merge 60 commits intomainfrom
t3code/greeting
Draft

Add ACP support with Cursor provider#1355
juliusmarminge wants to merge 60 commits intomainfrom
t3code/greeting

Conversation

@juliusmarminge
Copy link
Copy Markdown
Member

@juliusmarminge juliusmarminge commented Mar 24, 2026

Summary

  • Adds Cursor as a first-class provider with ACP session lifecycle support, health checks, and adapter wiring in the server.
  • Implements Cursor model selection, including fast/plan mode mapping and session restart behavior when model options change.
  • Preserves provider/thread model state through orchestration, projection, and turn dispatch paths.
  • Updates the web app to surface Cursor traits, provider/model selection, and session drafting behavior.
  • Expands runtime ingestion so completed tool events retain structured tool metadata.

Testing

  • bun fmt
  • bun lint
  • bun typecheck
  • Added and updated tests across server, contracts, shared, and web layers for Cursor adapter behavior, orchestration routing, session model changes, and UI state handling.
  • Not run: bun run test

Note

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) plus CursorAdapterLive that spawns agent acp, manages session/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, preserve resumeCursor across Cursor restarts, and merge per-thread modelOptions across turns.

Extends runtime ingestion to include structured data on tool.completed activities, 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

  • Adds a full Cursor provider integration: server-side CursorAdapterLive spawns the Cursor agent via ACP JSON-RPC, maps ACP errors to provider adapter errors, and is registered in the default ProviderAdapterRegistry.
  • Introduces AcpJsonRpcConnection — a new JSON-RPC framing layer over a child process with serialized writes, in-flight request tracking, and a notifications stream.
  • Adds Cursor model family constants, CursorModelOptions schema (reasoning, fastMode, thinking, claudeOpusTier), and model slug/alias resolution in packages/contracts and packages/shared.
  • Extends the composer draft store with stickyProvider persistence and Cursor model options normalization; the ProviderCommandReactor now routes turns by explicit provider and merges per-provider modelOptions across turns.
  • Adds CursorTraitsMenuContent and CursorTraitsPicker UI components for selecting Opus tier, reasoning level, and fast mode; the ProviderModelPicker now displays and dispatches Cursor family slugs.
  • Extends provider health checks to report Cursor CLI status (installed, authenticated, ready) via checkCursorProviderStatus.
  • Risk: session restart behavior changed — resumeCursor is 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

>

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 24, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 31bdf44a-6aff-410e-82d2-f45181658aed

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/greeting

Comment @coderabbitai help to get the list of available commands and usage tips.

@juliusmarminge juliusmarminge marked this pull request as draft March 24, 2026 07:29
@github-actions github-actions bot added size:XXL 1,000+ changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Mar 24, 2026
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Merge never clears cached provider model options
    • Replaced ?? fallback with key in incoming check so explicitly-present-but-undefined provider keys now clear cached values, and added cache deletion when merge produces undefined.
  • ✅ Fixed: Mock agent test uses strict equal with extra fields
    • Changed toEqual to toMatchObject so the assertion tolerates the extra modes field returned by the mock agent.

Create PR

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;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

- 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
const defaultCursorReasoning =
DEFAULT_REASONING_EFFORT_BY_PROVIDER.cursor as CursorReasoningOption;

const cursor: CursorModelOptions | undefined =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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
- 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
- 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
Comment on lines +790 to +801
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),
),
),
);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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* (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Comment on lines +159 to +167
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())),
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

juliusmarminge and others added 3 commits March 27, 2026 21:39
Co-authored-by: codex <codex@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low

expect(harness.generateBranchName.mock.calls[0]?.[0]).toMatchObject({

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>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium

import { Deferred, Effect, Exit, Layer, ManagedRuntime, PubSub, Scope, Stream } from "effect";

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
juliusmarminge and others added 2 commits April 1, 2026 00:19
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>
@CanRau CanRau mentioned this pull request Apr 6, 2026
4 tasks
- 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
Comment on lines +911 to +915
yield* Effect.addFinalizer(() =>
Effect.forEach(sessions.values(), stopSessionInternal, { discard: true }).pipe(
Effect.tap(() => Queue.shutdown(runtimeEventQueue)),
),
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 }) =>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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.

juliusmarminge and others added 5 commits April 8, 2026 17:50
- 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
Comment on lines +927 to +934
stopSession,
listSessions,
hasSession,
stopAll,
get streamEvents() {
return Stream.fromQueue(runtimeEventQueue);
},
} satisfies CursorAdapterShape;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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") {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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`.

Comment on lines +319 to +333
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,
};
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant