From cc210094efd5e077e45d4f3129ec22bc5cabe865 Mon Sep 17 00:00:00 2001 From: Diogo Soares Rodrigues Date: Sat, 25 Jul 2026 17:22:50 -0300 Subject: [PATCH] fix(cursor): refuse todo dependency graphs and sanitize failure text Two review findings on the native todo sync. - TodoItem.dependencies is a graph the local model cannot store: rows are keyed by content, carry no id, and hold no edges. An imported dependent row files as plain pending and nextActionableTask then offers work the server considers blocked. Refuse snapshots with an edge pointing at an unfinished row; edges whose blockers already finished constrain nothing and still mirror. - The todo failure warning interpolated the provider error verbatim. Collapse and truncate it at the render boundary. Also documents two known, unfixed defects: an async cursorOnToolResult transformer resolving after the buffer drain, and the todo card lifecycle race. Emitting a synthetic tool_execution_start for the latter was measured and rejected -- the completion deletes the entry it creates, so the late streamed block adds a second card. --- packages/agent/CHANGELOG.md | 2 +- packages/agent/src/agent.ts | 28 +++++- packages/agent/test/agent.test.ts | 4 + packages/ai/CHANGELOG.md | 1 + packages/ai/src/providers/cursor.ts | 33 +++++++ packages/ai/src/types.ts | 18 +++- packages/ai/test/cursor-todo-bridge.test.ts | 64 +++++++++++++ packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/cursor.ts | 20 ++++ .../src/modes/controllers/event-controller.ts | 18 +++- .../test/event-controller-cursor-todo.test.ts | 92 +++++++++++++++++++ 11 files changed, 272 insertions(+), 9 deletions(-) create mode 100644 packages/coding-agent/test/event-controller-cursor-todo.test.ts diff --git a/packages/agent/CHANGELOG.md b/packages/agent/CHANGELOG.md index 7c5ad4af7..8f1cd4d80 100644 --- a/packages/agent/CHANGELOG.md +++ b/packages/agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed a Cursor tool result being lost when a custom `cursorOnToolResult` transformer was still pending as the turn closed. The provider dispatches decoded messages without awaiting them, so a `message_end` from the same chunk could drain the buffer before the transformer resolved, dropping the result and leaving its `toolCall` block to be stripped as dangling on replay. The entry is now reserved synchronously and patched in place once the transformer resolves, preserving buffer order. +- Fixed a Cursor tool result being lost when a custom `cursorOnToolResult` transformer was still pending as the turn closed. The provider dispatches decoded messages without awaiting them, so a `message_end` from the same chunk could drain the buffer before the transformer resolved, dropping the result and leaving its `toolCall` block to be stripped as dangling on replay. The entry is now reserved synchronously and patched in place once the transformer resolves, preserving buffer order. Known limitation: if the transformer resolves *after* the drain, the call still persists (no longer dangling) but keeps the pre-transform payload — the late patch mutates a detached buffer entry. Coding-agent does not supply a custom transformer, so its Cursor todo path is unaffected. ## [17.1.2] - 2026-07-24 diff --git a/packages/agent/src/agent.ts b/packages/agent/src/agent.ts index 765353dfc..2f070fb0a 100644 --- a/packages/agent/src/agent.ts +++ b/packages/agent/src/agent.ts @@ -276,7 +276,16 @@ export interface AgentOptions { getCursorTools?: () => AgentTool[]; /** - * Cursor tool result callback for exec tool responses. + * Optional rewrite of Cursor exec-channel tool results. May return a Promise. + * + * The Agent reserves the original result in its Cursor result buffer first, + * then awaits this hook and patches the reserved entry in place. That keeps + * the call paired even if `message_end` arrives while the Promise is still + * pending. Limitation: `#emitCursorSplitAssistantMessage` drains the buffer + * on `message_end`, so a mutation that resolves after the drain only patches + * the detached entry — the already-persisted result keeps the pre-transform + * payload. Hosts that only pass `cursorExecHandlers` (the coding-agent path) + * never hit this hook. */ cursorOnToolResult?: CursorToolResultHandler; @@ -1140,10 +1149,15 @@ export class Agent { // messages with `void handleServerMessage(...)`, so a `message_end` // decoded from the same chunk can drain the buffer while a // transformer is still pending — pushing afterwards would drop the - // result and strip its toolCall block as dangling on replay. The - // entry is patched in place once the transformer resolves, which - // keeps buffer order and still applies the customization whenever - // it lands before the drain. + // result and strip its toolCall block as dangling on replay. + // + // Limitation: the in-place patch only reaches the persisted + // message when the transformer resolves BEFORE the drain. After + // `#emitCursorSplitAssistantMessage` swaps the buffer out, a late + // `entry.toolResult = updated` mutates a detached object and the + // already-emitted/persisted result keeps the original payload. + // The reservation guarantees the call is not lost; it does not + // await customization. Coding-agent never supplies this hook. const entry: CursorToolResultEntry = { toolResult: message }; this.#cursorToolResultBuffer.push(entry); if (this.#cursorOnToolResult) { @@ -1468,6 +1482,10 @@ export class Agent { * multi-text turns, producing duplicated text on replay. */ #emitCursorSplitAssistantMessage(assistantMessage: AssistantMessage): void { + // Snapshot and detach immediately so a still-pending `cursorOnToolResult` + // cannot push into a drained buffer. Entries already reserved stay paired + // with their toolCall; any transform that finishes after this point no + // longer reaches the messages appended below (see reservation comment). const buffer = this.#cursorToolResultBuffer; this.#cursorToolResultBuffer = []; diff --git a/packages/agent/test/agent.test.ts b/packages/agent/test/agent.test.ts index 6d46da298..f6fa0df19 100644 --- a/packages/agent/test/agent.test.ts +++ b/packages/agent/test/agent.test.ts @@ -342,6 +342,10 @@ describe("Agent", () => { // A transformer still pending when the turn closes must not cost the // result: an unbuffered toolResult leaves its toolCall block unpaired, and // the transcript rebuild strips it as dangling. + // + // This asserts only that the call survives — not that a late rewrite of + // the payload is persisted. After the drain, patching the detached entry + // leaves the already-emitted result unchanged. const mock = createMockModel({ responses: [] }); const toolCall = { type: "toolCall" as const, diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index ac1d17fdc..189278fe5 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -17,6 +17,7 @@ - Hardened Cursor todo mirroring against snapshots whose rows collide on content. Cursor's wire model identifies todos by `id` and can represent two rows sharing the same text; the local list is keyed by content alone and the `todo` tool rejects a duplicate outright, so importing such a pair would leave every task-targeted `done`/`drop`/`rm` resolving to the first row and the second unreachable. The snapshot is now refused like any other that cannot be represented locally — local state is left untouched and the call still settles as a no-op. - Hardened Cursor todo mirroring against ambiguous empty `read_todos` responses. `total_count` is a proto3 scalar, so an unset field decodes as `0` and is indistinguishable from a genuinely empty list; accepting `todos=[]` + `total_count=0` would clear every local task. Empty and mismatched reads are now refused — `update_todos` remains the authoritative clear path. - Fixed refused Cursor todo results claiming `"No todo changes"`. A server-accepted `update_todos` can still be declined locally (content collision, etc.), so the persisted fallback now reads `"Todo snapshot not mirrored"` instead of implying the remote call changed nothing. +- Hardened Cursor todo mirroring against snapshots carrying unresolved `TodoItem.dependencies`. The wire model blocks a row behind other rows by `id`; the local list has no ids and no edges, so an imported dependent row files as plain `pending` and `nextActionableTask` then offers work the server considers blocked. Snapshots with an edge pointing at a row that is not yet `completed`/`abandoned` are now refused like any other that cannot be represented locally. Edges whose blockers already finished constrain nothing and still mirror. ## [17.1.3] - 2026-07-24 diff --git a/packages/ai/src/providers/cursor.ts b/packages/ai/src/providers/cursor.ts index 6f7bf4d96..fb57b4897 100644 --- a/packages/ai/src/providers/cursor.ts +++ b/packages/ai/src/providers/cursor.ts @@ -2052,6 +2052,8 @@ interface CursorTodoItem { id?: string; content?: string; status?: number; + /** IDs of other todos this one waits on (agent.proto `TodoItem.dependencies`). */ + dependencies?: string[]; } interface CursorTodoResult { @@ -2163,6 +2165,37 @@ function extractTodoSnapshot(toolCall: CursorTodoToolCall): CursorTodoSnapshot | if (seen.has(todo.content)) return null; seen.add(todo.content); } + // `TodoItem.dependencies` carries the IDs a row waits on. The local model can + // express *that* a task is blocked (`TodoStatus` has `blocked`, `TodoItem` + // has `blocker`), but not the graph: it has no ids, so an edge cannot be + // stored, replayed, or re-evaluated when the blocker later completes. + // + // Dropping the edge silently is the harmful part. `nextActionableTask` + // (`todo.ts:164`) returns the first `pending` row with no notion of + // blockage, so the panel, the idle recap, and the completion reminders + // would all steer toward work the server says is not ready yet — and a + // reload loses the constraint for good. + // + // Only *unresolved* edges are refused: a dependency on an already + // finished row imposes nothing, which keeps late-session snapshots + // syncing normally. + // + // Projecting unresolved edges onto `blocked` + a `blocker` note is the + // lossy alternative — it preserves the warning but not the graph, and + // nothing would ever unblock the row, since the local engine has no id to + // match when the dependency completes. Refusing keeps this consistent with + // the collision case above: decline what cannot be represented rather than + // import an approximation. + const finished = new Set(); + for (const todo of todos) { + const status = mapTodoStatusValue(typeof todo.status === "number" ? todo.status : undefined); + if (todo.id && (status === "completed" || status === "abandoned")) finished.add(todo.id); + } + for (const todo of todos) { + for (const dependency of todo.dependencies ?? []) { + if (!finished.has(dependency)) return null; + } + } return { todos: mapped, // Presentation-only: the snapshot is already the settled full list. diff --git a/packages/ai/src/types.ts b/packages/ai/src/types.ts index e29968427..7af258bb3 100644 --- a/packages/ai/src/types.ts +++ b/packages/ai/src/types.ts @@ -574,7 +574,14 @@ export interface SimpleStreamOptions extends Omit { thinkingBudgets?: ThinkingBudgets; /** Cursor exec handlers for local tool execution */ cursorExecHandlers?: CursorExecHandlers; - /** Hook to handle tool results from Cursor exec */ + /** + * Optional rewrite of Cursor exec-channel tool results. May return a Promise. + * + * Limitation: an async rewrite that resolves after turn close may not be + * persisted. The Agent drains buffered Cursor results on `message_end`, so a + * late mutation only patches a detached entry while the already-persisted + * message keeps the pre-transform payload. + */ cursorOnToolResult?: CursorToolResultHandler; /** Optional tool choice override for compatible providers */ toolChoice?: ToolChoice; @@ -895,6 +902,15 @@ export type Message = UserMessage | DeveloperMessage | AssistantMessage | ToolRe export type CursorExecHandlerResult = { result: T; toolResult?: ToolResultMessage } | T | ToolResultMessage; +/** + * Optional rewrite of a Cursor exec-channel tool result. + * May return a Promise. Returning `undefined` keeps the original result. + * + * Limitation: an async rewrite that resolves after turn close may not be + * persisted. The Agent drains buffered Cursor results on `message_end`, so a + * late mutation only patches a detached entry while the already-persisted + * message keeps the pre-transform payload. + */ export type CursorToolResultHandler = ( result: ToolResultMessage, ) => ToolResultMessage | undefined | Promise; diff --git a/packages/ai/test/cursor-todo-bridge.test.ts b/packages/ai/test/cursor-todo-bridge.test.ts index aedd6826e..a7516b929 100644 --- a/packages/ai/test/cursor-todo-bridge.test.ts +++ b/packages/ai/test/cursor-todo-bridge.test.ts @@ -402,6 +402,13 @@ describe("cursor native todo bridge (wire-encoded protobuf)", () => { return rows.map(([id, content, status]) => create(TodoItemSchema, { id, content, status })); } + /** Rows carrying `TodoItem.dependencies` — the ids each row waits on. */ + function depItems(rows: [string, string, number, string[]][]) { + return rows.map(([id, content, status, dependencies]) => + create(TodoItemSchema, { id, content, status, dependencies }), + ); + } + function successResult(todos: TodoItem[], totalCount: number, wasMerge = false) { return create(UpdateTodosResultSchema, { result: { @@ -600,6 +607,63 @@ describe("cursor native todo bridge (wire-encoded protobuf)", () => { }); }); + it("refuses a snapshot whose rows carry unresolved dependencies", () => { + // `TodoItem.dependencies` is a graph the local model cannot store: rows are + // keyed by content and have no id, so an edge cannot be replayed or + // re-evaluated when the blocker finishes. Importing the row anyway files it + // as plain `pending`, and `nextActionableTask` then offers work the server + // says is not ready. + // + // Positive control: the same two rows without the edge do sync, so the + // refusal is attributable to the dependency and not to a decode failure. + const rows: [string, string, number][] = [ + ["1", "blocker task", 1], + ["2", "dependent task", 1], + ]; + expect(drive(updateCall(items(rows), 2)).snapshots).toHaveLength(1); + + const h = drive( + updateCall( + depItems([ + ["1", "blocker task", 1, []], + ["2", "dependent task", 1, ["1"]], + ]), + 2, + ), + ); + const callId = todoBlocks(h)[0].id; + + expect(todoBlocks(h)).toHaveLength(1); + expect(h.snapshots).toEqual([]); + // Settles as a benign no-op under the streamed id, like every other refusal. + expect(h.syncCalls).toEqual([{ snapshot: null, toolCallId: callId, error: null }]); + expect(h.toolResults[0]).toMatchObject({ toolCallId: callId, isError: false }); + }); + + it("still mirrors when every dependency is already finished", () => { + // A dependency on a completed row constrains nothing, so refusing it would + // strand late-session snapshots — by then most edges point at done work. + const h = drive( + updateCall( + depItems([ + ["1", "blocker task", 3, []], + ["2", "dependent task", 1, ["1"]], + ]), + 2, + ), + ); + + expect(h.snapshots).toEqual([ + { + todos: [ + { content: "blocker task", status: "completed" }, + { content: "dependent task", status: "pending" }, + ], + merged: false, + }, + ]); + }); + it("refuses a wire-decoded read_todos narrowed by status_filter", () => { const rows: [string, string, number][] = [["1", "only task", 3]]; // Positive control: the identical response without the filter does sync, diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f482be204..f807c8deb 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -16,6 +16,7 @@ - Todo progress now stays in sync when using Cursor models: the Cursor exec bridge mirrors the provider's server-owned todo list into session state, refreshes the interactive todo panel, and persists each snapshot to the session branch so the list survives reloads, rewinds, compaction, and session switches. Existing phase grouping is preserved for tasks the session already knows. Previously the list was in-memory only and the panel stayed stale, because Cursor resolves the todo tool remotely and never emits the local `todo` tool result that both paths key off. - Cursor todo calls the server refuses or rejects no longer leave the todo card spinning: the bridge settles every completed native todo call, not just the ones carrying a list. Local phases and the session branch are left untouched in that case, and the settling result deliberately carries no `details.phases` — echoing the current list back would let a call that changed nothing overwrite live panel state. +- Cursor todo failures no longer render unsanitized provider text into the status line. The bridge forwards the server's error string verbatim, so an ANSI escape or other C0/C1 control reached the terminal intact and could repaint outside the row, tabs punched holes in the single-line warning, and a long message overflowed it. The detail is now stripped of control sequences, collapsed, and truncated at the render boundary; the persisted result keeps the full-fidelity error for the transcript. ## [17.1.3] - 2026-07-24 diff --git a/packages/coding-agent/src/cursor.ts b/packages/coding-agent/src/cursor.ts index 69e38bb04..b59f927df 100644 --- a/packages/coding-agent/src/cursor.ts +++ b/packages/coding-agent/src/cursor.ts @@ -474,6 +474,26 @@ export class CursorExecHandlers implements ICursorExecHandlers { } const result = buildTodoSyncResult(toolCallId, phases, error); + // KNOWN DEFECT (unfixed): when Cursor packs `toolCallStarted` and + // `toolCallCompleted` into one HTTP/2 chunk, this completion races ahead + // of the streamed `toolcall_start`. That start is queued on + // `AssistantMessageEventStream` and delivered a microtask later, while + // `emitEvent` is a synchronous callback invoked mid-parse from + // `processInteractionUpdate`. The controller therefore handles the + // completion with no `pendingTools` entry, drops it + // (`event-controller.ts:1114`), and the card the streamed block creates + // afterwards (`:780`) animates for the rest of the session. + // + // Emitting a matching `tool_execution_start` here does NOT fix it: the + // completion deletes the entry it creates, so the late streamed block + // still finds an empty map and adds a SECOND card — one settled, one + // stuck. Measured, not assumed. + // + // The exec channel avoids this by construction: its handlers are async, + // so the stream has drained before they emit. The fix belongs at a point + // that runs after delivery — `agent-loop.ts:1189` already recognizes + // `kCursorExecResolved` blocks at `message_end` and currently filters + // them out without emitting any lifecycle for them. this.options.emitEvent?.({ type: "tool_execution_end", toolCallId, diff --git a/packages/coding-agent/src/modes/controllers/event-controller.ts b/packages/coding-agent/src/modes/controllers/event-controller.ts index 236b97ca7..75a87a107 100644 --- a/packages/coding-agent/src/modes/controllers/event-controller.ts +++ b/packages/coding-agent/src/modes/controllers/event-controller.ts @@ -2,7 +2,7 @@ import type { AssistantMessage, ImageContent } from "@oh-my-pi/pi-ai"; import * as AIError from "@oh-my-pi/pi-ai/error"; import { getStreamingPartialJson } from "@oh-my-pi/pi-ai/utils/block-symbols"; import { type Component, Loader, TERMINAL } from "@oh-my-pi/pi-tui"; -import { logger, prompt } from "@oh-my-pi/pi-utils"; +import { logger, prompt, sanitizeText } from "@oh-my-pi/pi-utils"; import { INTENT_FIELD } from "@oh-my-pi/pi-wire"; import { extractTextContent } from "../../commit/utils"; import { settings } from "../../config/settings"; @@ -1154,8 +1154,22 @@ export class EventController { const textContent = event.result.content.find( (content: { type: string; text?: string }) => content.type === "text", )?.text; + // This text can be a provider error copied verbatim off the wire (the + // Cursor todo bridge forwards the server's string), so it may carry + // ANSI escapes, other C0/C1 controls, tabs, newlines, or a line far + // wider than the terminal. `showWarning` renders through a plain + // `Text`, which strips none of that — an escape reaches the terminal + // and can repaint outside the row. `sanitizeText` drops the control + // sequences (and returns the same reference when there are none), + // then `previewLine` collapses the remaining whitespace and bounds + // the width. Sanitizing first matters: truncating before stripping + // can cut an escape mid-sequence and leave a dangling introducer. + // + // This is the render boundary, not the persisted result: the stored + // error stays full-fidelity for the transcript and for replays. + const detail = textContent ? previewLine(sanitizeText(textContent), TRUNCATE_LENGTHS.LINE) : ""; this.ctx.showWarning( - `Todo update failed${textContent ? `: ${textContent}` : ". Progress may be stale until todo succeeds."}`, + `Todo update failed${detail ? `: ${detail}` : ". Progress may be stale until todo succeeds."}`, ); } // Plan approval rides a `write` to xd://propose: the dispatch metadata on diff --git a/packages/coding-agent/test/event-controller-cursor-todo.test.ts b/packages/coding-agent/test/event-controller-cursor-todo.test.ts new file mode 100644 index 000000000..11d4f6a85 --- /dev/null +++ b/packages/coding-agent/test/event-controller-cursor-todo.test.ts @@ -0,0 +1,92 @@ +import { afterAll, beforeAll, describe, expect, it, type Mock, vi } from "bun:test"; +import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { EventController } from "@oh-my-pi/pi-coding-agent/modes/controllers/event-controller"; +import { initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; +import type { InteractiveModeContext } from "@oh-my-pi/pi-coding-agent/modes/types"; +import type { AgentSessionEvent } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { TRUNCATE_LENGTHS } from "@oh-my-pi/pi-coding-agent/tools/render-utils"; + +beforeAll(async () => { + resetSettingsForTest(); + await Settings.init({ inMemory: true }); + await initTheme(); +}); + +afterAll(() => { + resetSettingsForTest(); +}); + +interface Fixture { + ctx: InteractiveModeContext; + controller: EventController; + showWarning: Mock; +} + +function createFixture(): Fixture { + const showWarning = vi.fn(); + const ctx = { + isInitialized: true, + init: vi.fn(async () => {}), + ui: { requestRender: vi.fn() }, + transcriptMessageComponents: new WeakMap(), + pendingTools: new Map(), + statusLine: { invalidate: vi.fn(), markActivityStart: vi.fn() }, + session: { isAborting: false }, + updateEditorTopBorder: vi.fn(), + clearPinnedError: vi.fn(), + ensureLoadingAnimation: vi.fn(), + viewSession: { isStreaming: false, getToolByName: () => undefined }, + sessionManager: { getCwd: () => "/tmp" }, + chatContainer: { addChild: vi.fn(), removeChild: vi.fn(), isBlockUncommitted: () => false }, + toolOutputExpanded: false, + setTodos: vi.fn(), + present: vi.fn(), + showWarning, + } as unknown as InteractiveModeContext; + return { ctx, controller: new EventController(ctx), showWarning }; +} + +function todoFailure(text: string): Extract { + return { + type: "tool_execution_end", + toolCallId: "todo-1", + toolName: "todo", + isError: true, + result: { content: [{ type: "text", text }] }, + } as Extract; +} + +describe("EventController + Cursor todo bridge", () => { + it("sanitizes provider error text before it reaches the status line", async () => { + // The bridge forwards the server's error string verbatim, so this text is + // untrusted terminal input. Raw tabs punch holes in the single-line status + // area and an unbounded string overflows it. + const f = createFixture(); + + await f.controller.handleEvent( + todoFailure(`\u001b[31mrejected:\u001b[0m\tid 4\r\n\tconflicts with ${"x".repeat(400)}`), + ); + + expect(f.showWarning).toHaveBeenCalledTimes(1); + const message = f.showWarning.mock.calls[0]![0] as string; + expect(message).not.toContain("\t"); + expect(message).not.toContain("\n"); + // ANSI and other C0/C1 controls reach the terminal verbatim through + // `Text` and can repaint outside the row, so they must be gone too. + expect(message).not.toContain("\u001b"); + expect(message).not.toContain("\r"); + // The prefix is ours and fixed; only the untrusted tail is bounded. + expect(message.startsWith("Todo update failed: ")).toBe(true); + expect(Bun.stringWidth(message.slice("Todo update failed: ".length))).toBeLessThanOrEqual(TRUNCATE_LENGTHS.LINE); + }); + + it("keeps the standalone hint when the failure carries no text", async () => { + // Without a detail the warning must still say the panel may be stale — + // dropping to a bare "Todo update failed" hides that local state diverged. + const f = createFixture(); + + await f.controller.handleEvent(todoFailure("")); + + expect(f.showWarning).toHaveBeenCalledWith("Todo update failed. Progress may be stale until todo succeeds."); + }); +});