Repository navigation
feat(cursor): Cursor subagent ACP mapping, diagnostics, and desktop demo tour - #470
Merged
Merged
Conversation
…try tuning Map delegation-style Cursor ACP tool calls to Agent toolUse with parent linkage. Add optional traceSessionUpdates logging for session/update vs mapped events. Treat HTTP/2 stream cancel patterns as transient for optional retry. Co-authored-by: Cursor <cursoragent@cursor.com>
Wait for app shell, optional thread open, capture Changes, Terminal, Preview, and header toggle. Update harness command docs. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Cursor subagent detection and parent tool-call propagation, ACP task/input enrichment and session-update tracing, upstream-cancel retry heuristics, provider wiring and capture tooling/fixtures; updates Electron demo script with optional --tour screenshots, runbooks, settings, web UI/store types, and tests. ChangesCursor Subagent Delegation & Provider Tracing
Desktop Demo Electron Updates & Documentation
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
|
![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 (1)
apps/server/src/providers/cursor/cursor-acp-event-mapper.ts (1)
310-373:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist parent linkage across deferred ACP ToolUse emission.
When
tool_calldefers ToolUse (Line 371) andparent_tool_call_idis only present on that first update, the latertool_call_updateToolUse (Line 477) dropsparentToolCallId. Store parent ID in turn state keyed bytoolCallIdand use it as fallback in updates.🔧 Suggested fix
export interface CursorAcpTurnState { accumulator: CursorStreamAccumulator; /** Tool call IDs whose data arrives via ext methods, not session updates. */ suppressedToolCallIds: Set<string>; /** Tracks the ACP tool name (kind/title) per tool call ID for enriching updates. */ toolNameByCallId: Map<string, string>; + /** Parent linkage captured from tool_call when update payloads omit it later. */ + parentToolCallIdByCallId: Map<string, string>; } export function createCursorAcpTurnState(): CursorAcpTurnState { return { @@ suppressedToolCallIds: new Set(), toolNameByCallId: new Map(), + parentToolCallIdByCallId: new Map(), }; } @@ const parentToolCallId = extractCursorParentToolCallId(update as unknown as Record<string, unknown>); + if (parentToolCallId) { + state.parentToolCallIdByCallId.set(update.toolCallId, parentToolCallId); + } @@ - const parentToolCallId = extractCursorParentToolCallId(update as unknown as Record<string, unknown>); + const parentToolCallId = + extractCursorParentToolCallId(update as unknown as Record<string, unknown>) ?? + state.parentToolCallIdByCallId.get(update.toolCallId); @@ if (state.suppressedToolCallIds.has(update.toolCallId)) { @@ if (update.status === "completed" || update.status === "failed") { state.suppressedToolCallIds.delete(update.toolCallId); acc.pendingToolCalls.delete(update.toolCallId); + state.parentToolCallIdByCallId.delete(update.toolCallId); } return []; } @@ acc.toolStartTimes.delete(update.toolCallId); acc.pendingToolCalls.delete(update.toolCallId); state.toolNameByCallId.delete(update.toolCallId); + state.parentToolCallIdByCallId.delete(update.toolCallId);Also applies to: 411-491
🤖 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-acp-event-mapper.ts` around lines 310 - 373, The deferred ACP ToolUse loses parentToolCallId because the initial tool_call handler only suppresses emission (state.suppressedToolCallIds.add(update.toolCallId)) and returns, but doesn't persist the extracted parent (from extractCursorParentToolCallId) for later tool_call_update emission; add a map on the turn state (e.g. state.parentToolCallIdByCallId or similar) and when you detect a parentToolCallId in the tool_call branch store state.parentToolCallIdByCallId.set(update.toolCallId, parentToolCallId), then in the tool_call_update emission path (where ToolUse is actually created) use parentToolCallId || state.parentToolCallIdByCallId.get(toolCallId) to populate the emitted event, and remove the entry afterwards to avoid leaks.
🧹 Nitpick comments (2)
packages/contracts/src/models/__tests__/settings.test.ts (1)
36-49: ⚡ Quick winAdd a partial-override assertion for
traceSessionUpdates.The new Cursor knob is asserted in defaults, but not in
PartialSettingsSchemaparse coverage. Adding one assertion here prevents silent regressions in incremental updates.Proposed test tweak
it("accepts PartialSettings overrides for Cursor ACP knobs", () => { const parsed = PartialSettingsSchema().parse({ provider: { cursor: { alwaysSendFullInstructions: true, fullPreambleEveryNTurns: 0, idleSessionTtlMinutes: 60, + traceSessionUpdates: true, }, }, }); expect(parsed.provider?.cursor?.alwaysSendFullInstructions).toBe(true); expect(parsed.provider?.cursor?.fullPreambleEveryNTurns).toBe(0); expect(parsed.provider?.cursor?.idleSessionTtlMinutes).toBe(60); + expect(parsed.provider?.cursor?.traceSessionUpdates).toBe(true); });🤖 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 `@packages/contracts/src/models/__tests__/settings.test.ts` around lines 36 - 49, The test "accepts PartialSettings overrides for Cursor ACP knobs" is missing an assertion for the new Cursor knob traceSessionUpdates; update the test that parses PartialSettingsSchema() (the parsed variable) to include traceSessionUpdates in the input and assert parsed.provider?.cursor?.traceSessionUpdates equals the provided value; locate this in the settings.test.ts test case and add the input key under provider.cursor and a corresponding expect(...) assertion to prevent regressions.apps/server/src/providers/cursor/__tests__/cursor-acp-event-mapper.test.ts (1)
246-277: ⚡ Quick winAdd a deferred-path parent propagation test.
Please add a case where
tool_callhas emptyrawInput(so ToolUse is emitted fromtool_call_update) and only the initialtool_callcarriesparent_tool_call_id. That will lock in parent linkage behavior for the lifecycle-deferred flow.🤖 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/__tests__/cursor-acp-event-mapper.test.ts` around lines 246 - 277, Add a new test that simulates the lifecycle-deferred flow: call mapCursorAcpSessionNotification twice for the same toolCallId (e.g., "c-exp-deferred") — first with a "tool_call" update that includes parent_tool_call_id but has an empty rawInput, then with a "tool_call_update" that carries the actual rawInput (e.g., exploreWorkspaceToolCall args) and status that should cause a ToolUse event to be emitted; assert the resulting AgentEventType.ToolUse event has toolCallId "c-exp-deferred", the expected toolInput.goal, the same threadId, and that parentToolCallId equals the parent_tool_call_id from the initial "tool_call" update so parent linkage is preserved. Use existing helpers createCursorAcpTurnState and mapCursorAcpSessionNotification and the AgentEventType.ToolUse symbol to locate where to add the test.
🤖 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 815-817: The retry logic allows one more retry even when a user
Stop was requested because pendingUserStopAbort is only checked in the outer
catch; to fix, short-circuit retries when a pending user stop is detected:
inside the error handling path that inspects
looksLikeUpstreamStreamCancel(errMsg) and the retry guard (the logic around
retrying the call to prompt), check pendingUserStopAbort (or compute
userStoppedStream) first and immediately rethrow/abort instead of allowing the
retry; update the decision in the retry guard that controls calling prompt again
(the code that references pendingUserStopAbort, looksLikeUpstreamStreamCancel,
and prompt) so that userStoppedStream prevents any further invocation of prompt.
---
Outside diff comments:
In `@apps/server/src/providers/cursor/cursor-acp-event-mapper.ts`:
- Around line 310-373: The deferred ACP ToolUse loses parentToolCallId because
the initial tool_call handler only suppresses emission
(state.suppressedToolCallIds.add(update.toolCallId)) and returns, but doesn't
persist the extracted parent (from extractCursorParentToolCallId) for later
tool_call_update emission; add a map on the turn state (e.g.
state.parentToolCallIdByCallId or similar) and when you detect a
parentToolCallId in the tool_call branch store
state.parentToolCallIdByCallId.set(update.toolCallId, parentToolCallId), then in
the tool_call_update emission path (where ToolUse is actually created) use
parentToolCallId || state.parentToolCallIdByCallId.get(toolCallId) to populate
the emitted event, and remove the entry afterwards to avoid leaks.
---
Nitpick comments:
In `@apps/server/src/providers/cursor/__tests__/cursor-acp-event-mapper.test.ts`:
- Around line 246-277: Add a new test that simulates the lifecycle-deferred
flow: call mapCursorAcpSessionNotification twice for the same toolCallId (e.g.,
"c-exp-deferred") — first with a "tool_call" update that includes
parent_tool_call_id but has an empty rawInput, then with a "tool_call_update"
that carries the actual rawInput (e.g., exploreWorkspaceToolCall args) and
status that should cause a ToolUse event to be emitted; assert the resulting
AgentEventType.ToolUse event has toolCallId "c-exp-deferred", the expected
toolInput.goal, the same threadId, and that parentToolCallId equals the
parent_tool_call_id from the initial "tool_call" update so parent linkage is
preserved. Use existing helpers createCursorAcpTurnState and
mapCursorAcpSessionNotification and the AgentEventType.ToolUse symbol to locate
where to add the test.
In `@packages/contracts/src/models/__tests__/settings.test.ts`:
- Around line 36-49: The test "accepts PartialSettings overrides for Cursor ACP
knobs" is missing an assertion for the new Cursor knob traceSessionUpdates;
update the test that parses PartialSettingsSchema() (the parsed variable) to
include traceSessionUpdates in the input and assert
parsed.provider?.cursor?.traceSessionUpdates equals the provided value; locate
this in the settings.test.ts test case and add the input key under
provider.cursor and a corresponding expect(...) assertion to prevent
regressions.
🪄 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: d4351440-e165-495b-ab61-05177a5aaa54
📒 Files selected for processing (22)
.claude/commands/demo-desktop.md.codex/prompts/demo-desktop.md.cursor/commands/demo-desktop.md.opencode/command/demo-desktop.mdapps/server/src/providers/cursor/__tests__/cursor-acp-event-mapper.test.tsapps/server/src/providers/cursor/__tests__/cursor-acp-session-trace.test.tsapps/server/src/providers/cursor/__tests__/cursor-acp-transient-retry.test.tsapps/server/src/providers/cursor/__tests__/cursor-stream-event-mapper.test.tsapps/server/src/providers/cursor/__tests__/cursor-subagent-detection.test.tsapps/server/src/providers/cursor/cursor-acp-event-mapper.tsapps/server/src/providers/cursor/cursor-acp-session-trace.tsapps/server/src/providers/cursor/cursor-acp-transient-retry.tsapps/server/src/providers/cursor/cursor-provider.tsapps/server/src/providers/cursor/cursor-stream-event-mapper.tsapps/server/src/providers/cursor/cursor-stream-json-types.tsapps/server/src/providers/cursor/cursor-subagent-detection.tsdocs/agents/demo.mddocs/agents/runtime.mddocs/settings/reference.mdpackages/contracts/src/models/__tests__/settings.test.tspackages/contracts/src/models/settings.tsscripts/agent/demo-desktop.mjs
Live capture showed Task subagents arrive on cursor/task after tool_call completion. Add mapper module, suite capture script, golden fixtures, and execute kind to Bash mapping. Co-authored-by: Cursor <cursoragent@cursor.com>
Cursor Task delegations have no child tool_call events on the parent ACP session. Show delegation tags and hide the expand control when children.length is zero. Keep collapsible rows for Claude paths with nested tools. Co-authored-by: Cursor <cursoragent@cursor.com>
Read paths, Grep match summaries, and Bash commands are built on tool_call_update. Extend capture suite with write_create and todos_plan scenarios. 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>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/server/src/providers/cursor/cursor-provider.ts (1)
792-797:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRetry guard may issue a second prompt after user Stop.
The retry logic doesn't check
entry.pendingUserStopAbortbefore retrying, so when a user clicks Stop during the first prompt attempt and the prompt fails with a transient-looking error, the code will retry and issue a second prompt even though the user explicitly requested cancellation.🛡️ Proposed fix
Add a
pendingUserStopAbortcheck to the retry guard:} catch (attemptErr) { const raw = attemptErr instanceof Error ? attemptErr.message : String(attemptErr); if ( + entry.pendingUserStopAbort || attempt >= maxAttempts || !cursorCfg.retryTransientFailuresOnce || !isLikelyTransientCursorPromptFailure(raw) ) { throw attemptErr; }🤖 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 792 - 797, The retry guard must also respect user cancellation: update the conditional that decides to throw (currently checking attempt >= maxAttempts || !cursorCfg.retryTransientFailuresOnce || !isLikelyTransientCursorPromptFailure(raw)) to additionally check entry.pendingUserStopAbort so retries are prevented when a user has requested Stop; in other words, include a check for entry.pendingUserStopAbort in the OR list (or its logical equivalent) so the code throws attemptErr immediately instead of issuing another prompt when pendingUserStopAbort is true.
🧹 Nitpick comments (3)
apps/web/src/components/panels/RightPanel.tsx (1)
307-318: ⚡ Quick winUse the shadcn
Buttonprimitive for the new Terminal tab trigger.The newly added tab trigger is a custom
<button>, while this component already imports and uses the sharedButtonprimitive. Please align this new control with the existing UI primitive.As per coding guidelines,
apps/web/src/components/**/*.{ts,tsx}: Always use existing shadcn primitives from apps/web/src/components/ui/ before creating custom UI elements.🤖 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/web/src/components/panels/RightPanel.tsx` around lines 307 - 318, Replace the custom <button> in RightPanel.tsx with the shared shadcn Button primitive (imported from apps/web/src/components/ui/) and wire the same behavior: call setRightPanelTab(activeThreadId!, "terminal") on click, preserve the Terminal icon and "Terminal" label, and move the existing className/styling into the Button props so visual state (activeTab === "terminal") remains identical; ensure Button receives any needed type/aria attributes and that activeTab, activeThreadId, setRightPanelTab, and Terminal are used exactly as before.apps/web/src/components/chat/narrative/__tests__/SubagentRow.test.tsx (1)
38-38: ⚡ Quick winStrengthen expand-control assertions to avoid false positives.
Current role queries are broad and can pass due to unrelated buttons. Assert against a specific accessible name/label (or exact button count) for the expand toggle.
Also applies to: 57-57
🤖 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/web/src/components/chat/narrative/__tests__/SubagentRow.test.tsx` at line 38, The assertion using screen.queryByRole("button", { expanded: false }) is too broad and can match unrelated buttons; update the test(s) in SubagentRow.test.tsx to target the specific expand-control by asserting its accessible name or exact count: replace the broad queryByRole call with a role query that includes the button's accessible name/label (e.g., queryByRole("button", { name: /expand|toggle/i }) or the exact label used in the component) or use getAllByRole("button") and assert the expected length for the expand toggle; update both occurrences referenced (the line around the first expect and the second at 57) so the test only passes for the actual expand control.apps/web/src/stores/threadStore.ts (1)
600-600: 💤 Low valueConsider moving
HYDRATION_TTL_MSto module level.The constant is defined inside
loadMessages, but similar constants (MESSAGE_WINDOW_SIZE,MESSAGE_FETCH_SIZE,OLDER_PAGE_SIZE) are defined at module level (lines 336-345). Moving it would improve consistency and testability.♻️ Suggested placement
Move the constant to line 346:
export const MESSAGE_FETCH_SIZE = 100; +/** TTL for side-effect refresh gating (listPendingPermissions, getThreadTasks, listSnapshots). */ +const HYDRATION_TTL_MS = 2000;Then remove the local definition at line 600.
🤖 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/web/src/stores/threadStore.ts` at line 600, Move the HYDRATION_TTL_MS constant out of the loadMessages function and declare it with the other module-level constants (near MESSAGE_WINDOW_SIZE, MESSAGE_FETCH_SIZE, OLDER_PAGE_SIZE) so it's consistent and testable; remove the local definition inside loadMessages and update any usage there to reference the new module-level HYDRATION_TTL_MS identifier.
🤖 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/scripts/capture-cursor-acp.ts`:
- Around line 190-198: The readTextFile and writeTextFile handlers currently
call resolve(REPO_ROOT, r.path) which still allows absolute paths and ../
traversal to escape the repo; before calling
readFileSync/mkdirSync/writeFileSync, compute the resolved path and validate it
is contained within REPO_ROOT (e.g., compare path.resolve(REPO_ROOT) with the
resolved target using startsWith or path.relative and ensure no leading '..');
if the check fails throw an error; apply this check to both readTextFile and
writeTextFile (the code referencing resolve(REPO_ROOT, r.path), readFileSync,
mkdirSync(dirname(path)), and writeFileSync) so no file I/O can operate outside
the repository root.
In
`@apps/server/src/providers/cursor/__tests__/fixtures/cursor-acp-suite-mapped.jsonl`:
- Line 6: The fixture contains a machine-local absolute path
("C:\\Users\\...\\scratch.txt") embedded in the JSONL payload; replace any
occurrences of that absolute path in the keys toolInput.file_path and
toolResult.output with a reproducible sanitized placeholder or relative path
(e.g., "fixture-workspace/scratch.txt" or
"<SANITIZED_PATH>/fixture-workspace/scratch.txt"), and scan the rest of the file
for other "C:\\Users\\" or full absolute path strings and update them similarly
so the fixture contains no user-specific local paths.
In `@apps/web/src/components/chat/narrative/subagent-delegation-tags.ts`:
- Around line 32-33: The current check allows whitespace-only model values
because it only tests input.model.length; update the guard around adding tags so
you trim the model string first and ensure the trimmed value is non-empty before
calling formatModelLabel. Specifically, for the branch that checks typeof
input.model === "string", compute a trimmedModel = input.model.trim() (or use
input.model?.trim()) and only call tags.push(formatModelLabel(trimmedModel))
when trimmedModel.length > 0 so blank/whitespace-only labels are skipped.
---
Duplicate comments:
In `@apps/server/src/providers/cursor/cursor-provider.ts`:
- Around line 792-797: The retry guard must also respect user cancellation:
update the conditional that decides to throw (currently checking attempt >=
maxAttempts || !cursorCfg.retryTransientFailuresOnce ||
!isLikelyTransientCursorPromptFailure(raw)) to additionally check
entry.pendingUserStopAbort so retries are prevented when a user has requested
Stop; in other words, include a check for entry.pendingUserStopAbort in the OR
list (or its logical equivalent) so the code throws attemptErr immediately
instead of issuing another prompt when pendingUserStopAbort is true.
---
Nitpick comments:
In `@apps/web/src/components/chat/narrative/__tests__/SubagentRow.test.tsx`:
- Line 38: The assertion using screen.queryByRole("button", { expanded: false })
is too broad and can match unrelated buttons; update the test(s) in
SubagentRow.test.tsx to target the specific expand-control by asserting its
accessible name or exact count: replace the broad queryByRole call with a role
query that includes the button's accessible name/label (e.g.,
queryByRole("button", { name: /expand|toggle/i }) or the exact label used in the
component) or use getAllByRole("button") and assert the expected length for the
expand toggle; update both occurrences referenced (the line around the first
expect and the second at 57) so the test only passes for the actual expand
control.
In `@apps/web/src/components/panels/RightPanel.tsx`:
- Around line 307-318: Replace the custom <button> in RightPanel.tsx with the
shared shadcn Button primitive (imported from apps/web/src/components/ui/) and
wire the same behavior: call setRightPanelTab(activeThreadId!, "terminal") on
click, preserve the Terminal icon and "Terminal" label, and move the existing
className/styling into the Button props so visual state (activeTab ===
"terminal") remains identical; ensure Button receives any needed type/aria
attributes and that activeTab, activeThreadId, setRightPanelTab, and Terminal
are used exactly as before.
In `@apps/web/src/stores/threadStore.ts`:
- Line 600: Move the HYDRATION_TTL_MS constant out of the loadMessages function
and declare it with the other module-level constants (near MESSAGE_WINDOW_SIZE,
MESSAGE_FETCH_SIZE, OLDER_PAGE_SIZE) so it's consistent and testable; remove the
local definition inside loadMessages and update any usage there to reference the
new module-level HYDRATION_TTL_MS identifier.
🪄 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: fbae1339-5b29-4ee5-91eb-dd09ec4e6713
📒 Files selected for processing (25)
apps/server/scripts/capture-cursor-acp.tsapps/server/src/providers/cursor/__tests__/cursor-acp-event-mapper.test.tsapps/server/src/providers/cursor/__tests__/cursor-acp-task.test.tsapps/server/src/providers/cursor/__tests__/cursor-acp-tool-input-enrichment.test.tsapps/server/src/providers/cursor/__tests__/fixtures/README.mdapps/server/src/providers/cursor/__tests__/fixtures/cursor-acp-suite-mapped.jsonlapps/server/src/providers/cursor/__tests__/fixtures/cursor-acp-suite-raw.jsonlapps/server/src/providers/cursor/cursor-acp-event-mapper.tsapps/server/src/providers/cursor/cursor-acp-task.tsapps/server/src/providers/cursor/cursor-acp-tool-input-enrichment.tsapps/server/src/providers/cursor/cursor-provider.tsapps/web/src/__tests__/agent-event-branches.test.tsapps/web/src/components/chat/narrative/NarrativeIndicator.tsxapps/web/src/components/chat/narrative/StackedLayersIcon.tsxapps/web/src/components/chat/narrative/SubagentRow.tsxapps/web/src/components/chat/narrative/__tests__/SubagentRow.test.tsxapps/web/src/components/chat/narrative/__tests__/extract-subagent-description.test.tsapps/web/src/components/chat/narrative/__tests__/subagent-delegation-tags.test.tsapps/web/src/components/chat/narrative/extract-subagent-description.tsapps/web/src/components/chat/narrative/subagent-delegation-tags.tsapps/web/src/components/panels/RightPanel.tsxapps/web/src/stores/threadStore.tsapps/web/src/transport/types.tsdocs/agents/runtime.mddocs/settings/reference.md
✅ Files skipped from review due to trivial changes (4)
- apps/web/src/components/chat/narrative/tests/extract-subagent-description.test.ts
- apps/server/src/providers/cursor/tests/fixtures/README.md
- docs/agents/runtime.md
- apps/server/src/providers/cursor/tests/fixtures/cursor-acp-suite-raw.jsonl
Stop tracking gitignored plan docs under docs/plans and docs/specs. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/server/scripts/capture-cursor-acp.ts (1)
38-50: 💤 Low valuePath containment check may not catch Windows absolute paths on different drives.
relative("C:\\repo", "D:\\file")returns"D:\\file"(the absolute path), which passes thestartsWith("..")check. For this development capture script the risk is low, but consider adding an explicit check thatcandidate.startsWith(root)as a belt-and-suspenders guard.Optional additional guard
function resolveWithinRepo(requestPath: string): string { const root = resolve(REPO_ROOT); const candidate = resolve(root, requestPath); const rel = relative(root, candidate); - if (rel.startsWith("..") || rel.split(/[/\\]/).includes("..")) { + // On Windows, relative() returns an absolute path when crossing drives. + const escapesRoot = + rel.startsWith("..") || + rel.split(/[/\\]/).includes("..") || + (candidate !== root && !candidate.startsWith(root + (process.platform === "win32" ? "\\" : "/"))); + if (escapesRoot) { throw new Error(`Path escapes repo root: ${requestPath}`); } return candidate; }🤖 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/scripts/capture-cursor-acp.ts` around lines 38 - 50, The resolveWithinRepo function's relative-based check can miss Windows absolute paths on other drives (e.g., root "C:\repo" vs candidate "D:\file"); update resolveWithinRepo to also verify that the resolved candidate is actually under root by comparing normalized absolute prefixes: compute root and candidate via resolve(REPO_ROOT) and resolve(root, requestPath), normalize case/format (e.g., toLowerCase() on Windows) and check that candidate === root or candidate startsWith(root + path.sep); if not, throw the same Path escapes repo root error. Ensure you reference REPO_ROOT, resolveWithinRepo, root and candidate when applying the additional prefix check.
🤖 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.
Nitpick comments:
In `@apps/server/scripts/capture-cursor-acp.ts`:
- Around line 38-50: The resolveWithinRepo function's relative-based check can
miss Windows absolute paths on other drives (e.g., root "C:\repo" vs candidate
"D:\file"); update resolveWithinRepo to also verify that the resolved candidate
is actually under root by comparing normalized absolute prefixes: compute root
and candidate via resolve(REPO_ROOT) and resolve(root, requestPath), normalize
case/format (e.g., toLowerCase() on Windows) and check that candidate === root
or candidate startsWith(root + path.sep); if not, throw the same Path escapes
repo root error. Ensure you reference REPO_ROOT, resolveWithinRepo, root and
candidate when applying the additional prefix check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01b17b80-52dc-416e-902d-3f0a61e6f528
📒 Files selected for processing (12)
.gitignoreapps/server/scripts/capture-cursor-acp.tsapps/server/src/providers/cursor/__tests__/fixtures/cursor-acp-suite-mapped.jsonlapps/server/src/providers/cursor/__tests__/fixtures/cursor-acp-suite-raw.jsonlapps/server/src/providers/cursor/cursor-acp-tool-input-enrichment.tsapps/server/src/providers/cursor/cursor-provider.tsapps/web/src/components/chat/narrative/__tests__/subagent-delegation-tags.test.tsapps/web/src/components/chat/narrative/subagent-delegation-tags.tsdocs/plans/2026-04-11-gh-copilot-provider-design.mddocs/plans/2026-04-11-gh-copilot-provider-plan.mddocs/specs/2026-04-13-md-rendering-mermaid-plan.mddocs/specs/2026-04-14-usage-tracking-plan.md
💤 Files with no reviewable changes (4)
- docs/plans/2026-04-11-gh-copilot-provider-design.md
- docs/specs/2026-04-14-usage-tracking-plan.md
- docs/specs/2026-04-13-md-rendering-mermaid-plan.md
- docs/plans/2026-04-11-gh-copilot-provider-plan.md
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- apps/server/src/providers/cursor/tests/fixtures/cursor-acp-suite-mapped.jsonl
Co-authored-by: Cursor <cursoragent@cursor.com>
Ran agent models locally (111 models). Regenerate with bun run sync:cursor-models. Co-authored-by: Cursor <cursoragent@cursor.com>
Lookup order: live catalog, CLI snapshot, Cursor id heuristics, then native registry. Covers Composer, GPT, Codex, Opus, Sonnet, Grok, Gemini, and Kimi ids from agent models. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/lib/format-model-label.ts (1)
64-64: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a direct JSDoc block for
formatModelLabel.
formatModelLabelis exported at Line 64 but does not have an attached JSDoc block; the current comment is separated by constants and won’t document this function.As per coding guidelines, "
**/*.{ts,tsx}: Always add JSDoc/TSDoc docstrings to all exported functions, components, types, and interfaces, including at minimum a one-line summary".🤖 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/web/src/lib/format-model-label.ts` at line 64, The exported function formatModelLabel is missing a direct JSDoc/TSDoc comment; add a JSDoc block immediately above the exported declaration for formatModelLabel that includes at minimum a one-line summary and tags for `@param` {string} modelId and `@returns` {string} (or `@returns` description) so the function is documented per guidelines; ensure the comment is attached directly above the "export function formatModelLabel" line rather than separated by constants or other code.
🤖 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.
Outside diff comments:
In `@apps/web/src/lib/format-model-label.ts`:
- Line 64: The exported function formatModelLabel is missing a direct
JSDoc/TSDoc comment; add a JSDoc block immediately above the exported
declaration for formatModelLabel that includes at minimum a one-line summary and
tags for `@param` {string} modelId and `@returns` {string} (or `@returns` description)
so the function is documented per guidelines; ensure the comment is attached
directly above the "export function formatModelLabel" line rather than separated
by constants or other code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e200332-aa51-479f-9745-2d3d9c6d49b0
📒 Files selected for processing (9)
apps/server/scripts/sync-cursor-models-snapshot.tsapps/web/src/lib/__tests__/format-cursor-model-id.test.tsapps/web/src/lib/__tests__/format-model-label.test.tsapps/web/src/lib/format-cursor-model-id.tsapps/web/src/lib/format-model-label.tspackage.jsonpackages/contracts/src/index.tspackages/contracts/src/providers/cursor-cli-models-snapshot.tspackages/contracts/src/providers/cursor-static-fallback.ts
✅ Files skipped from review due to trivial changes (1)
- packages/contracts/src/providers/cursor-cli-models-snapshot.ts
2 of 4 tasks
Merged
This was referenced Jun 11, 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
Improve Cursor ACP handling for delegations, add opt-in server tracing for ACP
session/updatetraffic vs mapped agent events, harden cancel and transient-retry behavior, and extend/demo-desktopwith a--tourscreenshot path.Why
Cursor subagent-style tool calls need to flow through the same
Agent/parentToolCallIdpath as other providers for correct narrative grouping. Operators also need a supported way to compare raw ACP envelopes with Mcode output when tool labels or parent links look wrong. The demo script should cover Changes and related panels, not only the first window capture.Key Changes
toolName: "Agent"with parent linkage in ACP and stream JSON mappers; add detection helper plus tests.provider.cursor.traceSessionUpdatessetting andCursor ACP session/update tracestructured logs (streamingagent_message_chunkis skipped to avoid log spam).scripts/agent/demo-desktop.mjswith--tour(shell connect wait, open thread when possible, panel hotkeys, header toggle) and sync harness command docs.Test plan
cd apps/server && bun run test src/providers/cursor/__tests__/(134 passed)cd packages/contracts && bun test src/models/__tests__/settings.test.ts(4 passed)bun run verifybefore merge if required by CI policyNeed help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
--tourmode for extra UI screenshotsDocumentation
Bug Fixes
Tests