Skip to content

Add Cursor provider sessions and model selection#1355

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

Add Cursor provider sessions and model selection#1355
juliusmarminge wants to merge 38 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 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
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: c3aca71f-1354-43ec-9c95-dd115390b5de

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

cwd: process.cwd(),
mcpServers: [],
});
expect(newResult).toEqual({ sessionId: "mock-session-1" });
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.

Mock agent test uses strict equal with extra fields

Low Severity

The test asserts expect(newResult).toEqual({ sessionId: "mock-session-1" }) but the mock agent's session/new handler returns { sessionId, modes: modeState() } which includes an additional modes property. toEqual performs a deep strict equality check, so this assertion will fail. It likely needs toMatchObject instead.

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
(CURSOR_REASONING_OPTIONS as readonly string[]).includes(cursorReasoningRaw)
? (cursorReasoningRaw as CursorReasoningOption)
: undefined;
const cursorFastMode = cursorCandidate?.fastMode === true;
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: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

- 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
});
});

const startSession: CursorAdapterShape["startSession"] = (input) =>
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: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.

Comment on lines +21 to +33
@@ -3018,6 +3025,14 @@ export function fromJsonSchemaMultiDocument(document: JsonSchema.MultiDocument<"
return 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)
+ }
+
+ if (hasOneOf) {
+ out = combine({ _tag: "Union", types: js.oneOf.map((type) => recur(type)), mode: "oneOf" }, out)
+ }
+
return out
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 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.

juliusmarminge and others added 2 commits March 27, 2026 13:07
- add request and protocol logging for ACP sessions
- validate session config values before sending them
- keep provider turn-start failures on the thread session
- add coverage for ACP runtime and orchestration errors

Co-authored-by: codex <codex@users.noreply.github.com>
Comment on lines +406 to +414
switch (upd.sessionUpdate) {
case "current_mode_update": {
modeId = upd.currentModeId;
events.push({
_tag: "ModeChanged",
modeId,
});
break;
}
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 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)

Comment on lines +884 to +889
<Tooltip>
<TooltipTrigger
className="block min-w-0 w-full text-left"
title={displayText}
aria-label={displayText}
>
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 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.

- propagate session cancel into pending ACP approvals
- emit cancelled turn state when interruptions resolve as cancelled
- stop adding default ACP config traits for built-in Cursor models
- move ACP runtime event mapping into shared helpers
- keep unrecognized Cursor ACP model slugs unchanged
- derive ACP adapter raw sources from shared runtime event sources
- keep canonical request mapping typed consistently
);
};

const handleRequestEncoded = (message: RpcMessage.RequestEncoded) => {
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/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)

juliusmarminge and others added 6 commits March 27, 2026 17:09
Co-authored-by: codex <codex@users.noreply.github.com>
- Cancel pending approvals when stopping a Cursor session
- Resolve pending user-input waits so sendTurn can exit cleanly
- Add regression coverage for both stop paths
- Emit decoded and raw protocol log events for notifications
- Cover logOutgoing behavior with a protocol test
- Drop the `./child-process` subpath export
- Stop building `src/child-process.ts` in package scripts
- Encode outgoing messages through ACP request envelopes
- Add coverage for notification encoding failures
- Thread child-process exit codes into protocol shutdown
- Distinguish stream end, decode failure, and process exit errors
- Add protocol coverage for pending requests and late responses
Comment on lines +287 to +295
processExit: handle.exitCode.pipe(
Effect.map(Number),
Effect.mapError(
(cause) =>
new AcpError.AcpProcessExitedError({
cause,
}),
),
),
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/client.ts:287

Effect.map(Number) converts null to 0, so processes killed by signal (which report null exit code) are incorrectly reported as exiting cleanly with code 0. handleTermination in protocol.ts explicitly distinguishes null from 0null creates AcpProcessExitedError({}) while 0 creates AcpProcessExitedError({ code: 0 }). Consider preserving the null value so signal-terminated processes are correctly identified.

-    processExit: handle.exitCode.pipe(
-      Effect.map(Number),
-      Effect.mapError(
+    processExit: handle.exitCode.pipe(
+      Effect.mapError(
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/effect-acp/src/client.ts around lines 287-295:

`Effect.map(Number)` converts `null` to `0`, so processes killed by signal (which report `null` exit code) are incorrectly reported as exiting cleanly with code 0. `handleTermination` in `protocol.ts` explicitly distinguishes `null` from `0` — `null` creates `AcpProcessExitedError({})` while `0` creates `AcpProcessExitedError({ code: 0 })`. Consider preserving the `null` value so signal-terminated processes are correctly identified.

Evidence trail:
- packages/effect-acp/src/client.ts lines 287-288 (commit REVIEWED_COMMIT): `processExit: handle.exitCode.pipe(Effect.map(Number), ...)` converts exit code to Number
- packages/effect-acp/src/protocol.ts line 45: `processExit?: Effect.Effect<number | null, AcpError.AcpProcessExitedError>` explicitly supports `null`
- packages/effect-acp/src/protocol.ts lines 452-456: explicit `code === null` check creates different error objects for `null` vs `0`
- JavaScript behavior: `Number(null) === 0`

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