Add ACP support with Cursor provider

[juliusmarminge]

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

• line 255: The projector.ts change unconditionally sets provider: payload.provider when constructing the OrchestrationThread object for thread.created events. For events persisted before this change (or events where the thread.create command did not include provider), payload.provider will be undefined. The decider in decider.ts conditionally spreads provider into 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 potentially undefined value. If the OrchestrationThread schema defines provider as an optionalKey (key can be absent but must be a string when present) rather than optional (which also accepts undefined), this will cause a decodeForEvent failure during event replay at startup for any existing thread.created events that lack a provider field. [ Out of scope (triage) ]
• line 682: The test file now contains two identical test cases named "preserves completed tool metadata on projected tool activities" within the same describe("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

• line 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. [ Cross-file consolidated ]

apps/server/src/provider/Layers/CursorAdapter.ts — 1 comment posted, 4 evaluated, 2 filtered

• line 478: In the partial match phase (line 478), the alias is only lowercased but not normalized (special characters not replaced with spaces), while normalizeModeSearchText(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") is false. 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) ]
• line 649: In parseSessionUpdate, when params.update is a record containing modeId/currentModeId but sessionUpdate is undefined and content is a record without a text string property, the parsed modeId is discarded at line 649. The function returns {} even though modeId was successfully parsed at lines 561-566. This causes the caller's if (p.modeId) check to fail, missing a mode state update. The final return {} at line 649 should include modeId like the other return paths do. [ Cross-file consolidated ]

apps/web/src/composerDraftStore.ts — 0 comments posted, 1 evaluated, 1 filtered

• line 445: The cursorClaudeOpusTier value "high" is validated as valid (line 431-432) but never included in the output. Line 445 only includes claudeOpusTier when it equals "max", meaning "high" is silently dropped. The condition should be cursorClaudeOpusTier ? { claudeOpusTier: cursorClaudeOpusTier } : {} to preserve both valid values. [ Out of scope (triage) ]

packages/shared/src/model.ts — 0 comments posted, 1 evaluated, 1 filtered

• line 352: In normalizeCursorModelOptions, the logic for persisting thinking only handles sel.thinking === false, but for claude-4.6-sonnet the defaultThinking is false (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.defaultThinking rather than just checking for false. [ Cross-file consolidated ]

[coderabbitai]

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: e150e699-a3c3-4b53-a7cd-31d90eb4004c

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

[cursor]

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

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",

[cursor]

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.

 

[macroscopeapp]

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

[macroscopeapp]

🟢 Low src/composerDraftStore.ts:509

cursorFastMode is set via cursorCandidate?.fastMode === true, so explicit fastMode: false values are silently discarded instead of being preserved. Contrast with codexFastMode and claudeFastMode which preserve both true and false via ternary chains. Consider using the same ternary pattern for cursorFastMode so that false values are retained.

Suggested change

	const cursorFastMode = cursorCandidate?.fastMode === true;
	const cursorFastMode =
	cursorCandidate?.fastMode === true
	? true
	: cursorCandidate?.fastMode === false
	? false
	: undefined;

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file apps/web/src/composerDraftStore.ts around line 509:

`cursorFastMode` is set via `cursorCandidate?.fastMode === true`, so explicit `fastMode: false` values are silently discarded instead of being preserved. Contrast with `codexFastMode` and `claudeFastMode` which preserve both `true` and `false` via ternary chains. Consider using the same ternary pattern for `cursorFastMode` so that `false` values are retained.

Evidence trail:
apps/web/src/composerDraftStore.ts lines 509 (cursorFastMode definition), 525 (usage), 449-457 (codexFastMode ternary chain), 462 (codexFastMode usage), 480-485 (claudeFastMode ternary chain), 498 (claudeFastMode usage) at REVIEWED_COMMIT

[macroscopeapp]

🟡 Medium Layers/CursorAdapter.ts:310

In sendTurn, if promptParts is empty and validation fails at line 712, the function returns an error after already mutating ctx.activeTurnId, updating ctx.session, and emitting a turn.started event. This leaves the session with an orphaned activeTurnId and a dangling turn.started event with no matching completion event. Consider moving the validation before any state mutations and event emissions.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file apps/server/src/provider/Layers/CursorAdapter.ts around line 310:

In `sendTurn`, if `promptParts` is empty and validation fails at line 712, the function returns an error after already mutating `ctx.activeTurnId`, updating `ctx.session`, and emitting a `turn.started` event. This leaves the session with an orphaned `activeTurnId` and a dangling `turn.started` event with no matching completion event. Consider moving the validation before any state mutations and event emissions.

Evidence trail:
apps/server/src/provider/Layers/CursorAdapter.ts lines 624-720 at REVIEWED_COMMIT:
- Line 642-648: State mutations (`ctx.activeTurnId = turnId`, `ctx.session = ...`)
- Line 668-677: `turn.started` event emitted via `offerRuntimeEvent`
- Line 679-709: `promptParts` array built
- Line 711-717: Validation check `if (promptParts.length === 0)` returns error
- Line 737-746: `turn.completed` event only emitted after successful prompt

The validation check at line 711 happens AFTER state mutations (line 642-648) and event emission (line 668-677), confirming the claim.

[macroscopeapp]

🟢 Low patches/effect@4.0.0-beta.41.patch:21

When a JSON Schema contains both allOf and anyOf/oneOf, the anyOf/oneOf constraints are silently dropped. At lines 21-22, the allOf handling returns early with return js.allOf.reduce(...), so the new anyOf/oneOf handling at lines 25-31 never executes. Consider restructuring so allOf, anyOf, and oneOf are all processed before returning.

     if (Array.isArray(js.allOf)) {
-      return js.allOf.reduce((acc, curr) => combine(acc, recur(curr)), out)
+      out = js.allOf.reduce((acc, curr) => combine(acc, recur(curr)), out)
     }
 
     if (hasAnyOf) {
-      out = combine({ _tag: "Union", types: js.anyOf.map((type) => recur(type)), mode: "anyOf" }, out)
+      out = combine(out, { _tag: "Union", types: js.anyOf.map((type) => recur(type)), mode: "anyOf" })
     }
 
     if (hasOneOf) {
-      out = combine({ _tag: "Union", types: js.oneOf.map((type) => recur(type)), mode: "oneOf" }, out)
+      out = combine(out, { _tag: "Union", types: js.oneOf.map((type) => recur(type)), mode: "oneOf" })
     }
 
     return out

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file patches/effect@4.0.0-beta.41.patch around lines 21-33:

When a JSON Schema contains both `allOf` and `anyOf`/`oneOf`, the `anyOf`/`oneOf` constraints are silently dropped. At lines 21-22, the `allOf` handling returns early with `return js.allOf.reduce(...)`, so the new `anyOf`/`oneOf` handling at lines 25-31 never executes. Consider restructuring so `allOf`, `anyOf`, and `oneOf` are all processed before returning.

[macroscopeapp]

🟡 Medium acp/AcpRuntimeModel.ts:406

In parseSessionUpdateEvent, the current_mode_update case assigns modeId directly from upd.currentModeId without trimming whitespace. This causes a mismatch when the server sends a padded mode ID like " code " — the returned modeId won't match the trimmed IDs stored in the session state from parseSessionModeState, so mode state updates may fail. Consider trimming the value: modeId = upd.currentModeId.trim();

    case "current_mode_update": {
-      modeId = upd.currentModeId;
+      modeId = upd.currentModeId.trim();
      events.push({

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file apps/server/src/provider/acp/AcpRuntimeModel.ts around lines 406-414:

In `parseSessionUpdateEvent`, the `current_mode_update` case assigns `modeId` directly from `upd.currentModeId` without trimming whitespace. This causes a mismatch when the server sends a padded mode ID like `" code "` — the returned `modeId` won't match the trimmed IDs stored in the session state from `parseSessionModeState`, so mode state updates may fail. Consider trimming the value: `modeId = upd.currentModeId.trim();`

Evidence trail:
apps/server/src/provider/acp/AcpRuntimeModel.ts lines 406-414: `parseSessionUpdateEvent` - `modeId = upd.currentModeId;` without trim (line 408)
apps/server/src/provider/acp/AcpRuntimeModel.ts lines 114-141: `parseSessionModeState` - `modes.currentModeId.trim()` (line 119) and `mode.id.trim()` (line 124)

[macroscopeapp]

🟢 Low chat/MessagesTimeline.tsx:884

The title={displayText} attribute on TooltipTrigger creates a native browser tooltip that appears alongside the custom TooltipPopup, causing duplicate overlapping tooltips on hover. Since TooltipPopup handles the tooltip functionality and aria-label is already provided for accessibility, the title attribute duplicates functionality and should be removed.

-            <TooltipTrigger
-              className="block min-w-0 w-full text-left"
-              title={displayText}
-              aria-label={displayText}
-            >

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file apps/web/src/components/chat/MessagesTimeline.tsx around lines 884-889:

The `title={displayText}` attribute on `TooltipTrigger` creates a native browser tooltip that appears alongside the custom `TooltipPopup`, causing duplicate overlapping tooltips on hover. Since `TooltipPopup` handles the tooltip functionality and `aria-label` is already provided for accessibility, the `title` attribute duplicates functionality and should be removed.

Evidence trail:
apps/web/src/components/chat/MessagesTimeline.tsx lines 885-904 at REVIEWED_COMMIT: Shows `TooltipTrigger` with `title={displayText}` (line 888), `aria-label={displayText}` (line 889), and `TooltipPopup` component (lines 902-904). The `title` attribute is standard HTML that triggers native browser tooltips, while `TooltipPopup` provides custom tooltip functionality - both would display simultaneously on hover.

[macroscopeapp]

🟡 Medium src/protocol.ts:218

Malformed session/update or session/elicitation_complete notification payloads terminate the entire protocol connection instead of being logged and skipped. When decodeSessionUpdate (line 221) or decodeElicitationComplete (line 241) fails, the AcpProtocolParseError propagates through Effect.forEach (line 373), causing Stream.runForEach (line 336) to fail and trigger the Effect.catch block (line 379) that ends stdin processing. Extension notifications (lines 260-264) are dispatched without validation and survive malformed payloads, but these known notification types tear down the connection due to a single bad message. Consider catching and logging the parse error for known notification types instead of letting them propagate, similar to how Effect.tapErrorTag (line 362) handles logging for the general case.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file packages/effect-acp/src/protocol.ts around line 218:

Malformed `session/update` or `session/elicitation_complete` notification payloads terminate the entire protocol connection instead of being logged and skipped. When `decodeSessionUpdate` (line 221) or `decodeElicitationComplete` (line 241) fails, the `AcpProtocolParseError` propagates through `Effect.forEach` (line 373), causing `Stream.runForEach` (line 336) to fail and trigger the `Effect.catch` block (line 379) that ends stdin processing. Extension notifications (lines 260-264) are dispatched without validation and survive malformed payloads, but these known notification types tear down the connection due to a single bad message. Consider catching and logging the parse error for known notification types instead of letting them propagate, similar to how `Effect.tapErrorTag` (line 362) handles logging for the general case.

Evidence trail:
packages/effect-acp/src/protocol.ts lines 211-255 (handleRequestEncoded - session_update decode at 211-232 with Effect.mapError creating AcpProtocolParseError, elicitation_complete at 234-250 similarly, extension notifications at 251-255 with no validation), line 373 (Effect.forEach with routeDecodedMessage that propagates errors), lines 336-378 (Stream.runForEach on stdin with Effect.tapErrorTag that only logs but doesn't catch), lines 379-393 (Effect.catch that posts ClientProtocolError ending processing)

[macroscopeapp]

🟠 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)

[macroscopeapp]

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

[macroscopeapp]

🟠 High Layers/CursorAdapter.ts:382

acpContextScope created via Scope.make("sequential") at line 382 is never closed. The scope is passed to Layer.build for the ACP runtime but isn't stored in CursorSessionContext, so stopSessionInternal has no reference to close it. This leaves finalizers registered during layer building dangling, causing resource leaks. Consider storing the scope in the session context and closing it in stopSessionInternal.

Also found in 1 other location(s)

apps/server/src/provider/acp/AcpSessionRuntime.ts:150

The eventQueue created on line 150 is never shut down when close is called. Stream.fromQueue(eventQueue) on line 461 will hang indefinitely waiting for more events after the runtime is closed, because Scope.close(runtimeScope, Exit.void) on line 230 closes the child process and ACP connection but does not shut down the queue. Consumers that iterate the events stream without using Stream.take will block forever. A finalizer should be added after queue creation, e.g., yield* Scope.addFinalizer(runtimeScope, Queue.shutdown(eventQueue));

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file apps/server/src/provider/Layers/CursorAdapter.ts around line 382:

`acpContextScope` created via `Scope.make("sequential")` at line 382 is never closed. The scope is passed to `Layer.build` for the ACP runtime but isn't stored in `CursorSessionContext`, so `stopSessionInternal` has no reference to close it. This leaves finalizers registered during layer building dangling, causing resource leaks. Consider storing the scope in the session context and closing it in `stopSessionInternal`.

Evidence trail:
apps/server/src/provider/Layers/CursorAdapter.ts lines 92-103 (CursorSessionContext interface), line 382 (acpContextScope creation), line 397 (scope passed to Layer.build), lines 305-323 (stopSessionInternal - no scope close), lines 569-583 (ctx assignment - no scope stored). git_grep for 'acpContextScope' shows only 2 occurrences at lines 382 and 397.

Also found in 1 other location(s):
- apps/server/src/provider/acp/AcpSessionRuntime.ts:150 -- The `eventQueue` created on line 150 is never shut down when `close` is called. `Stream.fromQueue(eventQueue)` on line 461 will hang indefinitely waiting for more events after the runtime is closed, because `Scope.close(runtimeScope, Exit.void)` on line 230 closes the child process and ACP connection but does not shut down the queue. Consumers that iterate the `events` stream without using `Stream.take` will block forever. A finalizer should be added after queue creation, e.g., `yield* Scope.addFinalizer(runtimeScope, Queue.shutdown(eventQueue));`

[macroscopeapp]

🟢 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

[macroscopeapp]

🟢 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

[macroscopeapp]

🟡 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