Repository navigation
feat(cursor): Cursor ACP reliability, tuning, and prompt hygiene - #432
Merged
Merged
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Cursor ACP utilities (ask-question auto-answering, sticky instruction resolver, transient-failure heuristic, guidance sizing), centralizes provider error normalization, extends CursorProvider session/state and runTurn logic (retry, stderr tail, model dedupe, sticky instructions), adjusts event mapper, wires skill-watcher callbacks to CursorProvider, updates settings/schema/docs, and integrates normalization and CursorProvider wiring in server startup. ChangesCursor Provider ACP Enhancements
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant SkillWatcher
participant CursorProvider
participant Normalizer
Client->>Server: request / turn
Server->>CursorProvider: runTurn(...)
CursorProvider->>CursorProvider: resolveStickyInstructions / build prompt
CursorProvider->>Server: connection.prompt -> may fail
Server->>Normalizer: normalizeAgentProviderError(provider, error)
Normalizer-->>Server: normalized error
Server->>Client: broadcast enriched agent.event
SkillWatcher->>Server: debounced invalidation
Server->>CursorProvider: onSkillRegistryDebouncedInvalidation()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/providers/cursor/cursor-agent-guidance.ts (1)
38-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAttach TSDoc directly to exported
buildCursorAgentGuidanceMarkdown.Line 56 exports a function, but its descriptive TSDoc is no longer directly attached after introducing constants/helpers in between.
✏️ Proposed fix
-/** - * Builds layered agent instructions for Cursor: global user rules (`~/.cursor/AGENTS.md`), - * then workspace {`@link` mergeCursorWorkspaceAgentMarkdown}. - * - * `@param` cwd - Session working directory (composer branch or worktree root). - * `@returns` Combined Markdown or `undefined` when no files are readable. - */ const MAX_AGENT_GUIDANCE_CHARS = 48_000; const MAX_SKILL_COMMAND_ROWS = 200; const PER_SKILL_DESC_CHARS = 400; /** Truncates stitched AGENTS content so catastrophic repo rules files cannot saturate input. */ function capAgentMarkdown(markdown: string): string { @@ } +/** + * Builds layered agent instructions for Cursor: global user rules (`~/.cursor/AGENTS.md`), + * then workspace {`@link` mergeCursorWorkspaceAgentMarkdown}. + * + * `@param` cwd - Session working directory (composer branch or worktree root). + * `@returns` Combined Markdown or `undefined` when no files are readable. + */ export function buildCursorAgentGuidanceMarkdown(cwd: string): string | undefined {As per coding guidelines,
**/*.{ts,tsx,js,jsx}: Always add JSDoc/TSDoc docstrings to all exported functions, components, types, and interfaces with at minimum a one-line summary of what the symbol does.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/providers/cursor/cursor-agent-guidance.ts` around lines 38 - 56, The exported function buildCursorAgentGuidanceMarkdown no longer has its TSDoc directly attached (constants/helpers like MAX_AGENT_GUIDANCE_CHARS, MAX_SKILL_COMMAND_ROWS, PER_SKILL_DESC_CHARS and capAgentMarkdown were inserted above it); add a one-line TSDoc/TSDoc-style comment immediately above the export of buildCursorAgentGuidanceMarkdown describing its purpose (e.g., "Builds layered agent instructions for Cursor and returns combined Markdown or undefined") so the exported symbol has a docstring per guidelines.apps/server/src/providers/cursor/cursor-provider.ts (1)
205-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't evict sessions while a turn is still running.
lastUsedAtis only bumped when the turn is queued, but the idle eviction path only exempts pending permissions. Any prompt/tool run that lasts longer thanidleSessionTtlMinutescan be torn down mid-turn even though the session is busy. Please skip eviction whileentry.activeTurnStateis non-null.Suggested guard
for (const [id, entry] of this.sessions) { + if (entry.activeTurnState) continue; if (now - entry.lastUsedAt <= ttlMs) continue; if (this.sessionHasPendingPermissions(id)) continue; void this.teardownSessionEntry(id, entry, true); this.sessions.delete(id); }Also applies to: 810-818
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/providers/cursor/cursor-provider.ts` at line 205, The eviction logic currently only exempts pending permissions but can still evict sessions that are actively running a turn because lastUsedAt is only bumped when queued; update the idle-eviction checks in the cursor provider to also skip eviction when entry.activeTurnState is non-null (i.e., treat entries with a non-null activeTurnState as active/busy), and ensure any places where entry.lastUsedAt is set (e.g., where entry.lastUsedAt = Date.now()) remain unchanged—just add the guard into the idle eviction path that checks and returns/continues if entry.activeTurnState !== null so active turns are never torn down mid-run (apply same change to the other eviction block referenced around the 810-818 region).
🧹 Nitpick comments (1)
apps/server/src/providers/cursor/cursor-provider.ts (1)
375-383: ⚡ Quick winScope the stderr tail to a single turn.
stderrTailLinesis session-wide and never reset, so a failure on a long-lived session can log unrelated stderr from earlier turns. That makes the new failure diagnostics misleading. Clear the tail before each turn; ifverboseFailureLogsis meant to be the switch for raw diagnostics, gate the per-line debug logging behind it too.Possible cleanup
child.stderr?.on("data", (chunk: Buffer) => { + const verboseFailureLogs = + this.settingsService.get().provider.cursor.verboseFailureLogs; for (const line of chunk.toString().split("\n")) { const trimmed = line.trim(); if (trimmed) { - logger.debug("cursor-agent acp stderr", { threadId, line: trimmed }); + if (verboseFailureLogs) { + logger.debug("cursor-agent acp stderr", { threadId, line: trimmed }); + } const tail = entry.stderrTailLines; tail.push(trimmed.slice(0, 2000)); while (tail.length > CURSOR_STDERR_TAIL_MAX) tail.shift(); } } @@ const { message, model, resume, attachments } = opts; const cursorCfg = this.settingsService.get().provider.cursor; + entry.stderrTailLines.length = 0; entry.activeTurnState = createCursorAcpTurnState();Also applies to: 659-660, 754-757
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/providers/cursor/cursor-provider.ts` around lines 375 - 383, The stderr tail is session-scoped (entry.stderrTailLines) and never cleared, causing old stderr to bleed into later turns; update the child.stderr "data" handler (and the analogous handlers around lines noted) to: on turn start clear entry.stderrTailLines, only push per-line trimmed slices up to CURSOR_STDERR_TAIL_MAX, and wrap the per-line logger.debug call with the verboseFailureLogs guard so raw stderr is only logged when verboseFailureLogs is true. Ensure the reset happens at the beginning of each new turn before any stderr lines are appended and apply the same change to the other occurrences (the handlers referenced at the other locations).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/providers/cursor/cursor-provider.ts`:
- Around line 660-728: The current code assigns entry.activeTurnState once
(createCursorAcpTurnState()) and then reuses it across prompt retry attempts,
causing partial sessionUpdate output from a failed attempt to be mixed into the
retry; fix by creating an isolated fresh turn state for each prompt attempt and
ensuring sessionUpdate handling uses that fresh state so partial output from a
failed attempt is not appended to the retried attempt. Concretely, move the
createCursorAcpTurnState() call (or call it again) inside the prompt retry loop
(before calling entry.connection.prompt) so entry.activeTurnState is
reinitialized per attempt, and ensure any logic that consumes sessionUpdate
events references entry.activeTurnState so the failed attempt's events are
discarded rather than merged into the retry.
---
Outside diff comments:
In `@apps/server/src/providers/cursor/cursor-agent-guidance.ts`:
- Around line 38-56: The exported function buildCursorAgentGuidanceMarkdown no
longer has its TSDoc directly attached (constants/helpers like
MAX_AGENT_GUIDANCE_CHARS, MAX_SKILL_COMMAND_ROWS, PER_SKILL_DESC_CHARS and
capAgentMarkdown were inserted above it); add a one-line TSDoc/TSDoc-style
comment immediately above the export of buildCursorAgentGuidanceMarkdown
describing its purpose (e.g., "Builds layered agent instructions for Cursor and
returns combined Markdown or undefined") so the exported symbol has a docstring
per guidelines.
In `@apps/server/src/providers/cursor/cursor-provider.ts`:
- Line 205: The eviction logic currently only exempts pending permissions but
can still evict sessions that are actively running a turn because lastUsedAt is
only bumped when queued; update the idle-eviction checks in the cursor provider
to also skip eviction when entry.activeTurnState is non-null (i.e., treat
entries with a non-null activeTurnState as active/busy), and ensure any places
where entry.lastUsedAt is set (e.g., where entry.lastUsedAt = Date.now()) remain
unchanged—just add the guard into the idle eviction path that checks and
returns/continues if entry.activeTurnState !== null so active turns are never
torn down mid-run (apply same change to the other eviction block referenced
around the 810-818 region).
---
Nitpick comments:
In `@apps/server/src/providers/cursor/cursor-provider.ts`:
- Around line 375-383: The stderr tail is session-scoped (entry.stderrTailLines)
and never cleared, causing old stderr to bleed into later turns; update the
child.stderr "data" handler (and the analogous handlers around lines noted) to:
on turn start clear entry.stderrTailLines, only push per-line trimmed slices up
to CURSOR_STDERR_TAIL_MAX, and wrap the per-line logger.debug call with the
verboseFailureLogs guard so raw stderr is only logged when verboseFailureLogs is
true. Ensure the reset happens at the beginning of each new turn before any
stderr lines are appended and apply the same change to the other occurrences
(the handlers referenced at the other locations).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 994edcec-fb60-4751-bc07-ee6285f913dd
📒 Files selected for processing (19)
apps/server/src/index.tsapps/server/src/providers/cursor/__tests__/cursor-acp-ask-question.test.tsapps/server/src/providers/cursor/__tests__/cursor-acp-event-mapper.test.tsapps/server/src/providers/cursor/__tests__/cursor-acp-sticky-instructions.test.tsapps/server/src/providers/cursor/__tests__/cursor-acp-transient-retry.test.tsapps/server/src/providers/cursor/__tests__/cursor-agent-guidance.test.tsapps/server/src/providers/cursor/cursor-acp-ask-question.tsapps/server/src/providers/cursor/cursor-acp-event-mapper.tsapps/server/src/providers/cursor/cursor-acp-sticky-instructions.tsapps/server/src/providers/cursor/cursor-acp-transient-retry.tsapps/server/src/providers/cursor/cursor-agent-guidance.tsapps/server/src/providers/cursor/cursor-provider.tsapps/server/src/services/__tests__/provider-agent-error-normalize.test.tsapps/server/src/services/agent-service.tsapps/server/src/services/provider-agent-error-normalize.tsapps/server/src/services/skill-watcher-service.tsdocs/settings/reference.mdpackages/contracts/src/models/__tests__/settings.test.tspackages/contracts/src/models/settings.ts
…err) Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/services/skill-watcher-service.ts`:
- Around line 226-237: The current try block groups this.skills.invalidate(),
broadcast("skills.changed", {}), and this.debouncedListener?.() so if
broadcast("skills.changed", {}) throws the debounced listener never runs; change
the control flow in SkillWatcherService so invalidate() is called first, then
run this.debouncedListener?.() in its own try/catch, and only then call
broadcast("skills.changed", {}) (or catch broadcast errors separately) so a
broadcast failure cannot prevent the debounced listener from executing; ensure
each operation logs its own errors (e.g., listenerErr and broadcast error) and
does not short-circuit the others.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 724289b6-2aa5-42c6-bd79-5b7bf0fe6f70
📒 Files selected for processing (19)
apps/server/src/index.tsapps/server/src/providers/cursor/__tests__/cursor-acp-ask-question.test.tsapps/server/src/providers/cursor/__tests__/cursor-acp-event-mapper.test.tsapps/server/src/providers/cursor/__tests__/cursor-acp-sticky-instructions.test.tsapps/server/src/providers/cursor/__tests__/cursor-acp-transient-retry.test.tsapps/server/src/providers/cursor/__tests__/cursor-agent-guidance.test.tsapps/server/src/providers/cursor/cursor-acp-ask-question.tsapps/server/src/providers/cursor/cursor-acp-event-mapper.tsapps/server/src/providers/cursor/cursor-acp-sticky-instructions.tsapps/server/src/providers/cursor/cursor-acp-transient-retry.tsapps/server/src/providers/cursor/cursor-agent-guidance.tsapps/server/src/providers/cursor/cursor-provider.tsapps/server/src/services/__tests__/provider-agent-error-normalize.test.tsapps/server/src/services/agent-service.tsapps/server/src/services/provider-agent-error-normalize.tsapps/server/src/services/skill-watcher-service.tsdocs/settings/reference.mdpackages/contracts/src/models/__tests__/settings.test.tspackages/contracts/src/models/settings.ts
…ails Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Improves the Cursor CLI (ACP) provider with user-tunable behaviour, clearer failures, tighter stuck instruction handling for token use, protocol fixes around tool lifecycle, plus a merge from upstream main.
Why
ACP sessions benefit from pacing full preambles, retrying flaky prompts, resetting skills after registry changes, and answering blocking Cursor extension calls so threads do not wedge. Operators need settings rather than constants. Redundant RPCs to Cursor should stay minimal.
Key Changes
provider.cursorsettings in contracts (sticky vs full preamble, idle TTL, transient retry, verbose logs,cursor/ask_questiondefaults, timeline echo option) and document them indocs/settings/reference.md.CursorProvider: prompt retry heuristic, eviction TTL, richer error logs with stderr tails, periodic full preamble,cursor/ask_questionauto/skip behaviour, and skill registry debounced invalidation to refresh sticky preamble.perf(cursor): skipunstable_setSessionModelwhen MCP session id and model id unchanged after a successful apply.tool_callmarkers so ToolUse events surface correctly (fix(cursor)).fix(server)).perf(cursor)).origin/mainto keep CI and product changes current.No issue linked.
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
Configuration
Bug Fixes
Tests